Hello folks, wanted to give one more follow up on this patch/discussion. Would a reasonable next step for me to help nudge this forward be to submit a v10 addressing Andy's most recent code comments? Again hoping I'm not being rude or stepping on toes; just want to make sure I'm doing my dilligence to move things forward. I'll assume that going ahead and submitting a v10 with unresolved discussion here isn't a terrible offense if I don't end up getting a response here in the next week or so. Leaving some links to some of the more key points of the discussion across the versions of this patch, since it's been ~5 months since the last activity here. Discussion began with discussion of using child nodes by name across DT with ACPI, for binding fwnodes for the CP2112's I2C and GPIO controllers; since ACPI requires uppercase names (and names should specifically not be meaningful in ACPI) https://lore.kernel.org/all/Y%2F9oO1AE6GK6CQmp@xxxxxxxxxxxxxxxxxx/ Andy suggested I use _ADR to identify the child node by index for ACPI https://lore.kernel.org/all/ZAi4NjqXTbLpVhPo@xxxxxxxxxxxxxxxxxx/ v9 implemented matching by child node name OR by address depnding on the type of firmware used https://lore.kernel.org/all/20230319204802.1364-4-kaehndan@xxxxxxxxx/ Some additional discussion on whether matching child nodes by name is the best approach even for the DT side (also within the in-line body of this email) https://lore.kernel.org/all/ZBhoHzTr5l38u%2FkX@xxxxxxxxxxxxxxxxxx/ The DT binding patch in question https://lore.kernel.org/all/20230319204802.1364-2-kaehndan@xxxxxxxxx/ Thanks, Danny Kaehn On Mon, Jul 3 2023 at 13:57:22 +0300 Andy Shevchenko write: > On Mon, May 01, 2023 at 06:35:44PM -0500, Daniel Kaehn wrote: > > On Mon, Mar 20, 2023 at 9:10 AM Andy Shevchenko > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > > On Mon, Mar 20, 2023 at 08:40:07AM -0500, Daniel Kaehn wrote: > > > > On Mon, Mar 20, 2023 at 8:00 AM Andy Shevchenko > > > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > > > > On Mon, Mar 20, 2023 at 02:58:07PM +0200, Andy Shevchenko Wrote: > > > > > > On Sun, Mar 19, 2023 at 03:48:02PM -0500, Danny Kaehn wrote: > +Cc: Niyas, who is working a lot on filling the gaps in ACPI in comparison > to DT in the Linux kernel. Perhaps he has some ideas or even better > solutions. > > > ... > > > > > > > > + device_for_each_child_node(&hdev->dev, child) { > > > > > > > + name = fwnode_get_name(child); > > > > > > > + ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr); > > > > > > > + > > > > > > > + if ((name && strcmp("i2c", name) == 0) || (!ret && addr == 0)) > > > > > > > + device_set_node(&dev->adap.dev, child); > > > > > > > + else if ((name && strcmp("gpio", name)) == 0 || > > > > > > > + (!ret && addr == 1)) > > > > > > > + dev->gc.fwnode = child; > > > > > > > + } > > > > > > > > > > > > Please, make addresses defined explicitly. You may also do it with node naming > > > > > > schema: > > > > > > > > > > > > #define CP2112_I2C_ADR 0 > > > > > > #define CP2112_GPIO_ADR 1 > > > > > > > > > > > > static const char * const cp2112_cell_names[] = { > > > > > > [CP2112_I2C_ADR] = "i2c", > > > > > > [CP2112_GPIO_ADR] = "gpio", > > > > > > }; > > > > > > > > > > > > device_for_each_child_node(&hdev->dev, child) { > > > > > > name = fwnode_get_name(child); > > > > > > if (name) { > > > > > > ret = match_string(cp2112_cell_names, ARRAY_SIZE(cp2112_cell_names), name); > > > > > > if (ret >= 0) > > > > > > addr = ret; > > > > > > } else > > > > > > ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr); > > > > > > if (ret < 0) > > > > > > ...error handling if needed... > > > > > > > > > > > > switch (addr) { > > > > > > case CP2112_I2C_ADR: > > > > > > device_set_node(&dev->adap.dev, child); > > > > > > break; > > > > > > case CP2112_GPIO_ADR: > > > > > > dev->gc.fwnode = child; > > > > > > break; > > > > > > default: > > > > > > ...error handling... > > > > > > } > > > > > > } > > > > > > > > > > Btw, don't you use "reg" property for the child nodes? It would be better from > > > > > de facto used patterns (we have a couple of mode drivers that have a common > > > > > code to read "reg" or _ADR() and that code can be split into a helper and used > > > > > here). > > > > > > > > Named nodes _seem_ to be preferred in DT for when there isn't a logical / > > > > natural numbering to the child nodes. A.e. for USB, reg is used to specify > > > > which port, for I2C, which address on the bus, but for two parallel and > > > > independent functions on the same device, it seems named nodes would make > > > > more sense in DT. Many examples exist in mainline where named nodes are used > > > > in DT in this way. > > > > > > Okay, I'm not an expert in the DT preferable schemas, so I believe DT people > > > should answer on this. > > > > Hello, > > > > Thanks for all the time spent reviewing this thus far. Following up to > > see what my next steps might be. > > > > It sounds like we might want some DT folks to weigh in on the strategy > > used for identifying the child I2C and GPIO nodes for the CP2112 > > device before moving further toward applying this. > > > > Since the DT list is on this thread (as well as Rob+Krzystof), and > > this has sat for a little while, I'm assuming that the ball is in my > > court to seek out an answer/opinion here. (I know folks get a lot of > > email, so apologies if the correct move would have been to wait a bit > > longer before following up! Not intending to be rude.) > > > > Would it be appropriate / expected that I send a separate email thread > > to the DT mailing list on their opinion here? Or would that create > > more confusion/complexity in adding yet another thread? I did create a > > separate email thread for the initial DT vs. ACPI conversation we had > > about accessing children by name or index in a unified way due to the > > differences in upper/lower case and use-cases, but that > > (understandably) didn't seem to gain any traction. > > > > Thanks for any insights! > > > > Thanks, > > Danny Kaehn > > > > > > One example is network cards which provide an mdio bus > > > > bind through the child "mdio". One example of a specifically a > > > > child i2c controller being bound to "i2c" can be found in > > > > pine64,pinephone-keyboard.yaml. > > > > But it's certainly possible this isn't the desired direction moving forward > > > > in DT -- my opinion should definitely be taken with a grain of salt. Maybe > > > > this is something I should follow up on with DT folks on that DT vs. ACPI > > > > thread made earlier. > > > > > > > > One thing I did notice when looking at the mfd subsystem is that most DT > > > > drivers actually match on the compatible string of the child nodes, a.e. > > > > "silabs,cp2112", "silabs,cp2112-gpio". "silabs,cp2112-i2c". We could > > > > implement that here, but I think that would make more sense if we were to > > > > actually split the cp2112 into mfd & platform drivers, and additionally split > > > > the DT binding by function. > > > > > > IIRC (but might be very well mistaken) the compatible strings for children > > > are discouraged. >
