Date:   Fri, 10 Mar 2023 08:43:08 +0200
From:   Mika Westerberg <mika.westerberg@linux.intel.com>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        Daniel Kaehn <kaehndan@gmail.com>, linux-gpio@vger.kernel.org,
        linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] gpiolib: acpi: use the fwnode in acpi_gpiochip_find()
Message-ID: <20230310064308.GW62143@black.fi.intel.com>
References: <20230309-fix-acpi-gpio-v1-1-b392d225efe8@redhat.com>
 <ZAnnJpdtlEOS4tiS@smile.fi.intel.com>
 <ZAnxgooAixXAJvDn@smile.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <ZAnxgooAixXAJvDn@smile.fi.intel.com>
Precedence: bulk

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.