| Message ID | 20230309-fix-acpi-gpio-v1-1-b392d225efe8@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpiolib: acpi: use the fwnode in acpi_gpiochip_find() | expand |
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
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; } /**
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,