Date:   Thu, 9 Mar 2023 16:03:18 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc:     Mika Westerberg <mika.westerberg@linux.intel.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: <ZAnnJpdtlEOS4tiS@smile.fi.intel.com>
References: <20230309-fix-acpi-gpio-v1-1-b392d225efe8@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230309-fix-acpi-gpio-v1-1-b392d225efe8@redhat.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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.

-- 
With Best Regards,
Andy Shevchenko