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.
