Re: [PATCH v9 3/3] HID: cp2112: Fwnode Support

[Benjamin Tissoires <bentiss@kernel.org>]

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@smile.fi.intel.com/

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@smile.fi.intel.com/

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@gmail.com/

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@smile.fi.intel.com/

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@gmail.com/
> 
> 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@linux.intel.com> 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@linux.intel.com> 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.
> > 
>