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

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

On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote:
> On Mar 08 2023, Daniel Kaehn wrote:
> > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > > But if I refer "\\_SB_.PCI0.USB0.RHUB.CP21.GPIO", the IRQ is never assigned.
> > > With the parent (CP21), it works.
> > >
> > > So I wonder if the cp2112 driver is correctly assigning the gc->parent
> > > field.

> > Did you make a change to the CP2112 driver patch to look for uppercase
> > "I2C" and "GPIO"?
> 
> yes, sorry I should have mentioned it. This is the only modification I
> have compared to the upstream kernel plus your patch series.
> 
> > Otherwise, it won't assign those child nodes appropriately, and the
> > gpiochip code will use
> > the parent node by default if the gpiochip's fwnode isn't assigned (I believe).
> 
> I don't think it's a fwnode issue, but a problem with the assignment of
> the parent of the gc:
> ---
> dev->gc.parent = &hdev->dev;
> ---

I don't think so. The parent should point to the _physical_ device, which is
CP2112, which is correct in my opinion.

> Because the function acpi_gpiochip_find() in drivers/gpio/gpiolib-acpi.c
> compares the acpi handle returned by fetching the ACPI path
> ("\\_SB_.PCI0.USB0.RHUB.CP21.GPIO") and the one of gc->parent, which in
> the hid-cp2112 case is the HID device itself.

We have specifically gc->fwnode for cases like this.

...

>         Device (CP21) // the USB-hid & CP2112 shared node
>         {
>           Name (_ADR, One)
> 		Name (_DSD, Package ()
> 		{
> 			ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> 			Package () {
> 				Package () { "cell-names", Package () { "i2c", "gpio" }
> 			}

Yeah, looking at this, I think it still fragile. First of all, either this is
missing, or simply wrong. We would need to access indices. ACPI _ADR is in the
specification. As much as with PCI it may be considered reliable.

So, that said, forget about it, and simply use _ADR as indicator of the node.
See how MFD (in the Linux kernel) cares about this. Ex. Diolan DLN-2 driver.

> 		})
> 
>           Device (I2C)
>           {
>             Name (_ADR, Zero)
>             Name (_STA, 0x0F)
>           }
> 
>           Device (GPIO)
>           {
>             Name (_ADR, One)
>             Name (_STA, 0x0F)
> 
>             Name (_DSD, Package () {
>               ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>               Package () {
>                 Package () { "gpio-hog", 1 },
>                 Package () { "gpios", Package () { 4, 0 } },
>                 Package () { "output-high", 1 },
>                 Package () { "line-name", "gpio4-pullup" },
>               },
>               ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>               Package () {
>                 Package () { "gpio-line-names", Package () {
>                             "",
>                             "",
>                             "irq-rmi4",
>                             "",
>                             "power", // set to 1 with gpio-hog above
>                             "",
>                             "",
>                             "",
>                             ""}},
>               }
>             })
>           }
>         }

-- 
With Best Regards,
Andy Shevchenko