On Wed, Mar 08, 2023 at 12:32:07PM -0600, Daniel Kaehn wrote:

On Wed, Mar 8, 2023 at 10:36 AM Andy Shevchenko
[off-list ref] wrote:

quoted

On Wed, Mar 08, 2023 at 06:30:46PM +0200, Andy Shevchenko wrote:

quoted

On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote:

quoted

On Mar 08 2023, Daniel Kaehn wrote:

quoted

On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires
[off-list ref] wrote:

...

quoted

quoted

quoted

                    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