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

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

On Mar 08 2023, Andy Shevchenko wrote:
> 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.
> 

Right. I tend to agree, and then the problem seems to be relying in
gpiolib-acpi.c

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

Looks like gpiolib-acpi.c doesn't care about fwnode at all.

if I do the following:

---
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index d8a421ce26a8..5aebc266426b 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -126,7 +126,7 @@ static bool acpi_gpio_deferred_req_irqs_done;
 
 static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
 {
-       return gc->parent && device_match_acpi_handle(gc->parent, data);
+       return ACPI_HANDLE_FWNODE(gc->fwnode) == data;
 }
 
 /**
---

I can now directly reference the GPIO ACPI node in my GpioInt()
declaration. And AFAICT this should be safe to do because gpiolib ensure
that gc->fwnode is set, using the one from the parent if it is not set
previously.

I need to check if this works with my icelake laptop, and if so I'll
send it to the list.

The reason the intel gpios are working (the only one I checked) is
because the \\SB.GPI0 node refers to the pinctrl controller (driver
pinctrl-icelake.c in my case, which then creates a subdevice for
handling the gpio).

> 
> ...
> 

Cheers,
Benjamin