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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Mar 08, 2023 at 12:32:07PM -0600, Daniel Kaehn wrote:
> 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 can be iteratively converted later on.

> 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));

ACPI_COMPANION_SET() is something different to simple device_set_node().
I would expect that in this driver we simply use the child fwnode as is.
But since you are not using so called secondary fwnode, I believe it's
fine for now.

> + 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!)

See how the acpi_get_local_address() is used in the 3 users of it.

Ideally we need a new callback in the fwnode ops to return either
(least) 32-bit of _ADR or "reg" property.

Dunno, if "reg" is actually what suits here.

That said, I would do something like (pseudo-code)

device_for_each_child_node() {
	if (name == $NAME)
		$NAME->fwnode = child;
	else if (_ADR = $INDEX)
		$NAME->fwnode = child;
}


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

You're welcome!

-- 
With Best Regards,
Andy Shevchenko