gpiolib: acpi: use the fwnode in acpi_gpiochip_find()

  While trying to set up an SSDT override for a USB-2-I2C chip [0],
I realized that the function acpi_gpiochip_find() was using the parent
of the gpio_chip to do the ACPI matching.

This works fine on my icelake laptop because AFAICT, the DSDT presents
the PCI device INT3455 as the "Device (GPI0)", but is in fact handled
by the pinctrl driver in Linux.
The pinctrl driver then creates a gpio_chip device. This means that the
gc->parent device in that case is the GPI0 device from ACPI and everything
works.

However, in the hid-cp2112 case, the parent is the USB device, and the
gpio_chip is directly under that USB device. Which means that in this case
gc->parent points at the USB device, and so we can not do an ACPI match
towards the GPIO device.

I think it is safe to resolve the ACPI matching through the fwnode
because when we call gpiochip_add_data(), the first thing it does is
setting a proper gc->fwnode: if it is not there, it borrows the fwnode
of the parent.

So in my icelake case, gc->fwnode is the one from the parent, meaning
that the ACPI handle we will get is the one from the GPI0 in the DSDT
(the pincrtl one). And in the hid-cp2112 case, we get the actual
fwnode from the gpiochip we created in the HID device, making it working.

Link: https://lore.kernel.org/linux-input/20230227140758.1575-1-kaehndan@gmail.com/T/#m592f18081ef3b95b618694a612ff864420c5aaf3 [0]
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
Hi,

As mentioned on the commit, I believe there is a bug on
the gpiolib-acpi matching. It relies on the parent of the gpiochip
when it should IMO trust the fwnode that was given to it.

Tested on both the hid-cp2112 I am refering in the commit
description and my XPS on Intel Icelake.

Cheers,
Benjamin
---
 drivers/gpio/gpiolib-acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


---
base-commit: 6c71297eaf713ece684a367ce9aff06069d715b9
change-id: 20230309-fix-acpi-gpio-ab2af3344e7b

Best regards,
  

On Thu, Mar 09, 2023 at 02:40:51PM +0100, Benjamin Tissoires wrote:
> While trying to set up an SSDT override for a USB-2-I2C chip [0],
> I realized that the function acpi_gpiochip_find() was using the parent
> of the gpio_chip to do the ACPI matching.
> 
> This works fine on my icelake laptop because AFAICT, the DSDT presents

Ice Lake

> the PCI device INT3455 as the "Device (GPI0)", but is in fact handled
> by the pinctrl driver in Linux.
> The pinctrl driver then creates a gpio_chip device. This means that the
> gc->parent device in that case is the GPI0 device from ACPI and everything
> works.
> 
> However, in the hid-cp2112 case, the parent is the USB device, and the
> gpio_chip is directly under that USB device. Which means that in this case
> gc->parent points at the USB device, and so we can not do an ACPI match
> towards the GPIO device.
> 
> I think it is safe to resolve the ACPI matching through the fwnode
> because when we call gpiochip_add_data(), the first thing it does is
> setting a proper gc->fwnode: if it is not there, it borrows the fwnode
> of the parent.
> 
> So in my icelake case, gc->fwnode is the one from the parent, meaning

Ice Lake

> that the ACPI handle we will get is the one from the GPI0 in the DSDT
> (the pincrtl one). And in the hid-cp2112 case, we get the actual
> fwnode from the gpiochip we created in the HID device, making it working.

Thinking more about it. In ACPI we have those nodes defined as devices, right?
So, strictly speaking the platform tells us that they _are_ devices.

The question here is what this device node in ACPI means:
1) the physical device or subdevice of the physical device OR
2) the physical device even if it's a part of combined (Multi-Functional)
   device.

Second question is, does Device Tree specification allows something
that is not a device node, but can be enumerated as a subdevice of
a physical one?

P.S. I don't have objections against the patch, but I would like to
have a clear picture on what the semantics of the two specifications
WRT possibilities of device enumeration. It might be that we actually
abuse ACPI specification in cases of Diolan DLN-2 or other way around
trying to abuse it with this patch.
  

On Thu, Mar 09, 2023 at 04:03:19PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 09, 2023 at 02:40:51PM +0100, Benjamin Tissoires wrote:
> > While trying to set up an SSDT override for a USB-2-I2C chip [0],
> > I realized that the function acpi_gpiochip_find() was using the parent
> > of the gpio_chip to do the ACPI matching.
> > 
> > This works fine on my icelake laptop because AFAICT, the DSDT presents
> 
> Ice Lake
> 
> > the PCI device INT3455 as the "Device (GPI0)", but is in fact handled
> > by the pinctrl driver in Linux.
> > The pinctrl driver then creates a gpio_chip device. This means that the
> > gc->parent device in that case is the GPI0 device from ACPI and everything
> > works.
> > 
> > However, in the hid-cp2112 case, the parent is the USB device, and the
> > gpio_chip is directly under that USB device. Which means that in this case
> > gc->parent points at the USB device, and so we can not do an ACPI match
> > towards the GPIO device.
> > 
> > I think it is safe to resolve the ACPI matching through the fwnode
> > because when we call gpiochip_add_data(), the first thing it does is
> > setting a proper gc->fwnode: if it is not there, it borrows the fwnode
> > of the parent.
> > 
> > So in my icelake case, gc->fwnode is the one from the parent, meaning
> 
> Ice Lake
> 
> > that the ACPI handle we will get is the one from the GPI0 in the DSDT
> > (the pincrtl one). And in the hid-cp2112 case, we get the actual
> > fwnode from the gpiochip we created in the HID device, making it working.
> 
> Thinking more about it. In ACPI we have those nodes defined as devices, right?
> So, strictly speaking the platform tells us that they _are_ devices.
> 
> The question here is what this device node in ACPI means:
> 1) the physical device or subdevice of the physical device OR
> 2) the physical device even if it's a part of combined (Multi-Functional)
>    device.
> 
> Second question is, does Device Tree specification allows something
> that is not a device node, but can be enumerated as a subdevice of
> a physical one?
> 
> P.S. I don't have objections against the patch, but I would like to
> have a clear picture on what the semantics of the two specifications
> WRT possibilities of device enumeration. It might be that we actually
> abuse ACPI specification in cases of Diolan DLN-2 or other way around
> trying to abuse it with this patch.

I have read the ACPI specification and it doesn't tell that Device is allowed
to describe non-hardware entity. It means that in the Linux driver model,
whenever we use Device() in the ACPI, we have an accompanying struct device.

For GPIO chip case, we have physical device (parent) and GPIO device, which
is Linux internal representation. So, physical device should have a description
in the ACPI table, or other way around: any Device() in ACPI has to be
considered as physical. That said, I think that Daniel was right and we need
to have parent properly assigned (to the Device(GPI0) node).

Another way, is to drop children from the descripton and use a single device
node for entire box.

TL;DR: if Device() is present we must use it as a parent to Linux
representation.

I would like also to hear Mika's opinion on this.
  

Hi,

On Thu, Mar 09, 2023 at 04:47:30PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 09, 2023 at 04:03:19PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 09, 2023 at 02:40:51PM +0100, Benjamin Tissoires wrote:
> > > While trying to set up an SSDT override for a USB-2-I2C chip [0],
> > > I realized that the function acpi_gpiochip_find() was using the parent
> > > of the gpio_chip to do the ACPI matching.
> > > 
> > > This works fine on my icelake laptop because AFAICT, the DSDT presents
> > 
> > Ice Lake
> > 
> > > the PCI device INT3455 as the "Device (GPI0)", but is in fact handled
> > > by the pinctrl driver in Linux.
> > > The pinctrl driver then creates a gpio_chip device. This means that the
> > > gc->parent device in that case is the GPI0 device from ACPI and everything
> > > works.
> > > 
> > > However, in the hid-cp2112 case, the parent is the USB device, and the
> > > gpio_chip is directly under that USB device. Which means that in this case
> > > gc->parent points at the USB device, and so we can not do an ACPI match
> > > towards the GPIO device.
> > > 
> > > I think it is safe to resolve the ACPI matching through the fwnode
> > > because when we call gpiochip_add_data(), the first thing it does is
> > > setting a proper gc->fwnode: if it is not there, it borrows the fwnode
> > > of the parent.
> > > 
> > > So in my icelake case, gc->fwnode is the one from the parent, meaning
> > 
> > Ice Lake
> > 
> > > that the ACPI handle we will get is the one from the GPI0 in the DSDT
> > > (the pincrtl one). And in the hid-cp2112 case, we get the actual
> > > fwnode from the gpiochip we created in the HID device, making it working.
> > 
> > Thinking more about it. In ACPI we have those nodes defined as devices, right?
> > So, strictly speaking the platform tells us that they _are_ devices.
> > 
> > The question here is what this device node in ACPI means:
> > 1) the physical device or subdevice of the physical device OR
> > 2) the physical device even if it's a part of combined (Multi-Functional)
> >    device.
> > 
> > Second question is, does Device Tree specification allows something
> > that is not a device node, but can be enumerated as a subdevice of
> > a physical one?
> > 
> > P.S. I don't have objections against the patch, but I would like to
> > have a clear picture on what the semantics of the two specifications
> > WRT possibilities of device enumeration. It might be that we actually
> > abuse ACPI specification in cases of Diolan DLN-2 or other way around
> > trying to abuse it with this patch.
> 
> I have read the ACPI specification and it doesn't tell that Device is allowed
> to describe non-hardware entity. It means that in the Linux driver model,
> whenever we use Device() in the ACPI, we have an accompanying struct device.
> 
> For GPIO chip case, we have physical device (parent) and GPIO device, which
> is Linux internal representation. So, physical device should have a description
> in the ACPI table, or other way around: any Device() in ACPI has to be
> considered as physical. That said, I think that Daniel was right and we need
> to have parent properly assigned (to the Device(GPI0) node).
> 
> Another way, is to drop children from the descripton and use a single device
> node for entire box.
> 
> TL;DR: if Device() is present we must use it as a parent to Linux
> representation.
> 
> I would like also to hear Mika's opinion on this.

Agree with the patch. We should be using whatever the gc->fwnode points
to.
  

On Fri, Mar 10, 2023 at 08:43:08AM +0200, Mika Westerberg wrote:
> On Thu, Mar 09, 2023 at 04:47:30PM +0200, Andy Shevchenko wrote:

...

> Agree with the patch. We should be using whatever the gc->fwnode points
> to.

Thank you for looking into it, can you provide a formal tag?
  

On Thu, Mar 09, 2023 at 02:40:51PM +0100, Benjamin Tissoires wrote:
> While trying to set up an SSDT override for a USB-2-I2C chip [0],
> I realized that the function acpi_gpiochip_find() was using the parent
> of the gpio_chip to do the ACPI matching.
> 
> This works fine on my icelake laptop because AFAICT, the DSDT presents
> the PCI device INT3455 as the "Device (GPI0)", but is in fact handled
> by the pinctrl driver in Linux.
> The pinctrl driver then creates a gpio_chip device. This means that the
> gc->parent device in that case is the GPI0 device from ACPI and everything
> works.
> 
> However, in the hid-cp2112 case, the parent is the USB device, and the
> gpio_chip is directly under that USB device. Which means that in this case
> gc->parent points at the USB device, and so we can not do an ACPI match
> towards the GPIO device.
> 
> I think it is safe to resolve the ACPI matching through the fwnode
> because when we call gpiochip_add_data(), the first thing it does is
> setting a proper gc->fwnode: if it is not there, it borrows the fwnode
> of the parent.
> 
> So in my icelake case, gc->fwnode is the one from the parent, meaning
> that the ACPI handle we will get is the one from the GPI0 in the DSDT
> (the pincrtl one). And in the hid-cp2112 case, we get the actual
> fwnode from the gpiochip we created in the HID device, making it working.
> 
> Link: https://lore.kernel.org/linux-input/20230227140758.1575-1-kaehndan@gmail.com/T/#m592f18081ef3b95b618694a612ff864420c5aaf3 [0]
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
  

On Fri, Mar 10, 2023 at 12:42 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Thu, Mar 09, 2023 at 02:40:51PM +0100, Benjamin Tissoires wrote:
> > While trying to set up an SSDT override for a USB-2-I2C chip [0],
> > I realized that the function acpi_gpiochip_find() was using the parent
> > of the gpio_chip to do the ACPI matching.
> >
> > This works fine on my icelake laptop because AFAICT, the DSDT presents
> > the PCI device INT3455 as the "Device (GPI0)", but is in fact handled
> > by the pinctrl driver in Linux.
> > The pinctrl driver then creates a gpio_chip device. This means that the
> > gc->parent device in that case is the GPI0 device from ACPI and everything
> > works.
> >
> > However, in the hid-cp2112 case, the parent is the USB device, and the
> > gpio_chip is directly under that USB device. Which means that in this case
> > gc->parent points at the USB device, and so we can not do an ACPI match
> > towards the GPIO device.
> >
> > I think it is safe to resolve the ACPI matching through the fwnode
> > because when we call gpiochip_add_data(), the first thing it does is
> > setting a proper gc->fwnode: if it is not there, it borrows the fwnode
> > of the parent.
> >
> > So in my icelake case, gc->fwnode is the one from the parent, meaning
> > that the ACPI handle we will get is the one from the GPI0 in the DSDT
> > (the pincrtl one). And in the hid-cp2112 case, we get the actual
> > fwnode from the gpiochip we created in the HID device, making it working.
> >
> > Link: https://lore.kernel.org/linux-input/20230227140758.1575-1-kaehndan@gmail.com/T/#m592f18081ef3b95b618694a612ff864420c5aaf3 [0]
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>

Thanks to both of you for the reviews.

Andy, should I resend a v2 with the rev-by from Mika and the Ice Lake changes?

Cheers,
Benjamin
  

On Fri, Mar 10, 2023 at 01:51:38PM +0100, Benjamin Tissoires wrote:
> On Fri, Mar 10, 2023 at 12:42 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:

...


> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> Thanks to both of you for the reviews.
> 
> Andy, should I resend a v2 with the rev-by from Mika and the Ice Lake changes?

Yes, please.
  

On Fri, Mar 10, 2023 at 2:36 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Mar 10, 2023 at 01:51:38PM +0100, Benjamin Tissoires wrote:
> > On Fri, Mar 10, 2023 at 12:42 PM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
>
> ...
>
>
> > > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> >
> > Thanks to both of you for the reviews.
> >
> > Andy, should I resend a v2 with the rev-by from Mika and the Ice Lake changes?
>
> Yes, please.
>

Alright, v2 sent just now :)

Cheers,
Benjamin