Re: [PATCH v8 3/3] HID: cp2112: Fwnode Support

From: Daniel Kaehn <kaehndan@gmail.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>,
	Hans de Goede <hdegoede@redhat.com>,
	robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	jikos@kernel.org, bartosz.golaszewski@linaro.org,
	dmitry.torokhov@gmail.com, devicetree@vger.kernel.org,
	linux-input@vger.kernel.org, ethan.twardy@plexus.com
Subject: Re: [PATCH v8 3/3] HID: cp2112: Fwnode Support
Date: Wed, 8 Mar 2023 12:32:07 -0600	[thread overview]
Message-ID: <CAP+ZCCdziub809WKJ8-tAhYvg+axsiuXrvrZczj_x2K0bGzd7w@mail.gmail.com> (raw)
In-Reply-To: <ZAi5esmc158Bd2oL@smile.fi.intel.com>

On Wed, Mar 8, 2023 at 10:36 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Mar 08, 2023 at 06:30:46PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote:
> > > On Mar 08 2023, Daniel Kaehn wrote:
> > > > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires
> > > > <benjamin.tissoires@redhat.com> wrote:
>
> ...
>
> > >                     ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > >                     Package () {
> > >                             Package () { "cell-names", Package () { "i2c", "gpio" }
> > >                     }
> >
> > Yeah, looking at this, I think it still fragile. First of all, either this is
> > missing, or simply wrong. We would need to access indices. ACPI _ADR is in the
> > specification. As much as with PCI it may be considered reliable.
> >
> > So, that said, forget about it, and simply use _ADR as indicator of the node.
> > See how MFD (in the Linux kernel) cares about this. Ex. Diolan DLN-2 driver.
>
> And that said, maybe CP2112 should simply re-use what MFD _already_ provides?

Great point -- it definitely seems like this driver belongs in the mfd
directory to begin with.

It seems like aside from rewriting the CP2112 driver into an mfd
driver and two platform drivers,
my route forward for now would be to just do something like this (not
yet tested):

+ struct acpi_device *adev = ACPI_COMPANION(&hdev->dev);
+ if (adev)
+    ACPI_COMPANION_SET(&dev->adap.dev, acpi_find_child_device(adev,
0x0, false));
+ else
+     device_set_node(&dev->adap.dev,
device_get_named_child_node(&hdev->dev, "i2c"));

(and the same for the gpiochip)

The follow-up question -- does there exist something analogous to DT
bindings for ACPI devices,
other than the ACPI spec itself, where this should be documented? Or
will consumers truly have to
read the driver code to determine that _ADR 0 is I2C and _ADR 1 is
GPIO? (I haven't seen anything
in my search so far -- but knowing that it truly doesn't exist would
make me respect people developing
embedded ACPI-based systems all the more!)

Thanks for your patience in working through all of this, especially
considering how long of an email
chain this has become!

Thanks,

Danny Kaehn