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

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:

quoted

On Mon, Feb 27, 2023 at 5:07 PM Andy Shevchenko
[off-list ref] wrote:

quoted

On Mon, Feb 27, 2023 at 08:07:58AM -0600, Danny Kaehn wrote:

quoted

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.

quoted

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

quoted

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.

quoted

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.

quoted

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

quoted

quoted

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.

quoted

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

quoted

- 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