Hi Danny, On Jan 17 2024, Danny Kaehn wrote: > 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. Submitting a v10, even if there are still undecided points is definitely the way forward. People probably have forgot a lot of things there and need a refresh on the latest state of it :) > > 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/ I think the DT part is fine. Please resubmit it in v10, but probably drop the previous rev-by and explicitly mention you did so after the first '---' below your signed-off-by. This should re-trigger a review from them. Things may have changed since last year, and having another review would be beneficial IMO. > > Andy suggested I use _ADR to identify the child node by index > for ACPI > https://lore.kernel.org/all/ZAi4NjqXTbLpVhPo@xxxxxxxxxxxxxxxxxx/ I think I still prefer the "_DSD" approach with the cell-names, but OTOH, it's not like there is an official ACPI description for this, and we can basically define whatever we want. So please go ahead with the _ADR approach IMO, with a couple of changes: - mention about that in the DT bindings documentation - please add an enum with those 2 addresses (with kernel doc), to document it in the code and not have magic constants in your checks > > 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/ See my 2 comments above. FWIW, I think 2/3 could go directly in as well, but the timing is not ideal, we are in the middle of the Merge Window. > > 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/ Honestly, not sure we'll have too many users on the ACPI side (besides myself). So if you really feel uncomfortable, you can always put a warning that we are using _ADR in the ACPI world as a fallback, but that we might revisit that in the future (with naming, if we reach to an agreement). Cheers, Benjamin > > > 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. > > >
