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

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

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.

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