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

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

On Thu, Mar 09, 2023 at 01:43:02PM +0200, Andy Shevchenko wrote:
> 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!)

The below misplaced, so here is the answer to the followup.
The _DSD heavily relies on the DT schemas and other standards such as MIPI.
For many cases there are no standards or any developed approaches.

Feel free to add a piece of documentation for the devices that are utilising
_ADR in ACPI (we have at least I²C/GPIO controller on Intel Quark —
drivers/mfd/intel_quark_i2c_gpio.c, and mentioned earlier the Diolan DLN-2 —
drivers/mfd/dln2.c).

> 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