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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx> 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
