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

On Mar 08 2023, Andy Shevchenko wrote:

On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote:

quoted

On Mar 08 2023, Daniel Kaehn wrote:

quoted

On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires
[off-list ref] wrote:

quoted

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.

quoted

quoted

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.

quoted

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

quoted

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