Message-ID: <ZAnxgooAixXAJvDn@smile.fi.intel.com>
Date:   Thu, 9 Mar 2023 16:47:30 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc:     Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Daniel Kaehn <kaehndan@...il.com>, linux-gpio@...r.kernel.org,
        linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] gpiolib: acpi: use the fwnode in acpi_gpiochip_find()

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.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists