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

From: Danny Kaehn <Danny.Kaehn@plexus.com>
To: "andriy.shevchenko@linux.intel.com" <andriy.shevchenko@linux.intel.com>
Cc: "kaehndan@gmail.com" <kaehndan@gmail.com>,
	"bartosz.golaszewski@linaro.org" <bartosz.golaszewski@linaro.org>,
	"benjamin.tissoires@redhat.com" <benjamin.tissoires@redhat.com>,
	Ethan Twardy <Ethan.Twardy@plexus.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"dmitry.torokhov@gmail.com" <dmitry.torokhov@gmail.com>,
	"jikos@kernel.org" <jikos@kernel.org>,
	"linux-input@vger.kernel.org" <linux-input@vger.kernel.org>,
	"niyas.sait@linaro.org" <niyas.sait@linaro.org>,
	"krzysztof.kozlowski+dt@linaro.org"
	<krzysztof.kozlowski+dt@linaro.org>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>
Subject: Re: [PATCH v9 3/3] HID: cp2112: Fwnode Support
Date: Wed, 17 Jan 2024 18:48:23 +0000	[thread overview]
Message-ID: <3e0fc6fa95f19d39711ca66808a2cff344652985.camel@plexus.com> (raw)
In-Reply-To: <ZKKpknBH3Pa9mLS1@smile.fi.intel.com>

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

Andy suggested I use _ADR to identify the child node by index
for ACPI
https://lore.kernel.org/all/ZAi4NjqXTbLpVhPo@smile.fi.intel.com/

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/

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/

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.
> 