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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Mar 01, 2023 at 04:59:07PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 28, 2023 at 01:05:54PM -0600, Daniel Kaehn wrote:
> > On Mon, Feb 27, 2023 at 5:07 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Feb 27, 2023 at 08:07:58AM -0600, Danny Kaehn wrote:
> > > > Bind I2C and GPIO interfaces to subnodes with names
> > > > "i2c" and "gpio" if they exist, respectively. This
> > > > allows the GPIO and I2C controllers to be described
> > > > in firmware as usual. Additionally, support configuring the
> > > > I2C bus speed from the clock-frequency device property.
> > >
> > > A bit shorten indentation...
> > >
> > > Nevertheless what I realized now is that this change, despite being OF
> > > independent by used APIs, still OF-only.
> > 
> > I assumed this would practically be the case -- not because of the casing
> > reason you gave (wasn't aware of that, thanks for the FYI), but because it
> > doesn't seem that there's any way to describe USB devices connected to
> > a USB port in ACPI, at least as far as I can tell (ACPI is still largely a black
> > box to me).
> 
> That's not true :-)
> 
> Microsoft created a schema that is not part of the specification, but let's
> say a standard de facto. Linux supports that and I even played with it [1]
> to get connection of the arbitrary device to USB-2-GPIO/I²C/SPI adapter.
> 
> > But it seems reasonable that we should try to use the interface
> > in a way so that it could be described using ACPI at some point (assuming
> > that it isn't currently possible).
> > 
> > > Would it be possible to allow indexed access to child nodes as well, so if
> > > there are no names, we may still be able to use firmware nodes from the correct
> > > children?
> > 
> > Sure, you mean to fallback to using child nodes by index rather than by name
> > in the case that that device_get_named_child_node() fails?  Would we need to
> > somehow verify that those nodes are the nodes we expect them to be? (a.e.
> > node 0 is actually the i2c-controller, node 1 is actually the
> > gpio-controller).
> 
> Something like that, but I don't know if we can actually validate this without
> having a preliminary agreement (hard-coded values) that index 0 is always let's
> say I²C controller and so on.
> 
> > I don't see a reason why not, though I am curious if there is
> > precedence for this
> > strategy, a.e. in other drivers that use named child nodes. In my initial search
> > through the kernel, I don't think I found anything like this -- does that mean
> > those drivers also inherently won't work with ACPI?
> 
> If they are relying on names, yes, they won't work. It might be that some of them
> have specific ACPI approach where different description is in use.
> 
> > The only driver I can find which uses device_get_named_child_node and has
> > an acpi_device_id is drivers/platform/x86/intel/chtwc_int33fe.c
> 
> Yes, and you may notice the capitalization of the name. Also note, that relying
> on the name in ACPI like now is quite fragile due to no common standard between
> OEMs on how to name the same hardware nodes, they are free in that.

Forgot to mention, that you need to take into consideration the drivers that are:
1) can be compiled with CONFIG_OF=n (i.e. using agnostic APIs);
2) having OF ID table;
3) being platform independent (no Kconfig dependency to something that's known
   to be an ARCH/CPU/etc one).

All of those can be instantiated in ACPI via special PRP0001 ACPI ID [2].

> > > P.S. The problem with ACPI is that "name" of the child node will be in capital
> > > letters as it's in accordance with the specification.
> > 
> > Knowing that this is the limitation, some other potential resolutions
> > to potentially
> > consider might be:
> > 
> > - Uppercase the names of the child nodes for the DT binding -- it appears
> >   that the child node that chtwc_int33fe.c (the driver mentioned earlier)
> >   accesses does indeed have an upper-cased name -- though that driver doesn't
> >   have an of_device_id (makes sense, x86...). It seems named child nodes are
> >   always lowercase in DT bindings -- not sure if that's a rule, or just how
> >   it currently happens to be.
> 
> Not an option AFAIK, the DT and ACPI specifications are requiring conflicting
> naming schema.
> 
> Possible way is to lowering case for ACPI names, but I dunno. We need more
> opinions on this.
> 
> > - Do a case invariant compare on the names (and/or check for both lowercase
> >   and uppercase)
> 
> For ACPI it will be always capital. For DT I have no clue if they allow capital
> letters there, but I assume they don't.
> 
> > - Remove the use of child nodes, and combine the i2c and gpio nodes into the
> >     cp2112's fwnode. I didn't do this initially because I wanted to avoid
> >     namespace collisions between GPIO hogs and i2c child devices, and thought
> >     that logically made sense to keep them separate, but that was before
> >     knowing this limitation of ACPI.
> 
> Seems to me not an option at all, we need to define hardware as is. If it
> combines two devices under a hood, should be two devices under a hood in
> DT/ACPI.
> 
> [1]: https://stackoverflow.com/a/60855157/2511795

[2]: https://docs.kernel.org/firmware-guide/acpi/enumeration.html#device-tree-namespace-link-device-id

-- 
With Best Regards,
Andy Shevchenko