[iicbus] Add support for ACPI-based children enumeration

[wulf]

Summary

When iicbus is attached as child of controller[1] that have an ACPI handle, it scans all ACPI children for "I2C Serial Bus Connection Resource Descriptor" described in section 19.6.57 of ACPI specs.
If such a descriptor is found, I2C child is added to iicbus, its I2C address and ACPI handle are added to child ivars and all newbus resources including IRQs are copied from ACPI-hosted device to IICbus-hosted one [2].
This effectively makes a copy of existing ACPI device on top of I2C bus that allows to implement child device identification through ACPI HID/CID comparison

[1] currently it is restricted to Designware I2C controllers only.
[2] I2C bus speed is calculated as well. It is equal to speed of slowest device among the children.

Test Plan

devinfo -rv shows detected child devices and their ACPI-derived pnpinfos and location strings

[markj]

This looks mostly ok to me, though I am not very comfortable with device(9).

One other general nit is that many of the comments do not end in a period.

sys/dev/iicbus/acpi_iicbus.c

203

You can just return (AcpiWalkNamespace(...)).

322

Why is plain strlen() unsafe?

357

"So we do not either." ?

I guess this should probably be fixed in the long term.

358

Why is it not safe to assume that drivers will return valid C strings?

sys/dev/iicbus/iicbus.c

137

Missing space after return.

[imp]

Couple of nits, but otherwise I think this is good if you address my and Mark's comments

sys/dev/iicbus/acpi_iicbus.c

128

enumerate_child would be more grammatically correct.

140

Not sure any comment is needed here.

144

/* Check that the device is a child of the ACPI bus */

might be better.

322

I don't think it's possible to have a string longer that buflen or that buf is not NUL terminated in the first buflen bytes at this point. At least without iicbus_child_location_str overflowing the buffer.

[• ian]

sys/dev/iicbus/acpi_iicbus.c

381

There is no detach method. If the module is unloaded, will the base iicbus detach release everything this bus allocated? What about the dummy driver instances in that case?

Related question: what if the i2c slave driver is in a module that gets unloaded? The iicbus child will get detached, but what about the dummy driver instance that's related to it? (All in all, I don't quite understand what the dummy driver is needed for.)

sys/dev/iicbus/iicbus.h

60

s/reserverd/reserved/

[wulf]

sys/dev/iicbus/acpi_iicbus.c

144

It is unusual that children of other buses than ACPI have _HID/_CID.

322

strnlen replaced with plain strlen(). EOVERFLOW is respected.

357

As above

358

As above.

381

There is no detach method. If the module is unloaded, will the base iicbus detach release everything this bus allocated?

detach method is inherited from generic iicbus driver. DEFINE_CLASS_1 macro is responsible for that. ofw_iicbus(4) driver does exactly the same.

What about the dummy driver instances in that case?
Related question: what if the i2c slave driver is in a module that gets unloaded? The iicbus child will get detached, but what about the dummy driver instance that's related to it?

dummy driver will keep its place. It is attached before i2c child driver is probed and should not be detached/deleted at all.

(All in all, I don't quite understand what the dummy driver is needed for.)

1. dummy driver is a resource donor for other drivers like some others in the tree: e.g. psmcpnp for psm or acpi_sysresource for acpi bus. We can just disable ACPI bus child instead of attaching of dummy driver, but:
2. dummy driver presence triggers ACPI _PS0/3 methods execution on suspend/resume. Our acpi bus is acting this way. It tests if child is attached or detached and than do a ACPI method call in first case.

[markj]

sys/dev/iicbus/acpi_iicbus.c

84

This can just return (AcpiWalkResources(...)).

[jhb]

So it would be helpful to see an acpidump of a system with these devices.

If I understand correctly, you leave acpi_iicdev around to attach to the device enumerated by acpi0? For PCI we mostly avoided this since we ignored devices without _HID avoiding the need to copy resources over. It's not clear to me why you need to leave this device around for suspend/resume to work. The iicbus device will be invoked during suspend/resume and can pass those events down to children as needed. The acpi_iicbus driver should be responsible for invoking any needed ACPI methods on handles as part of that. You really want that behavior so you can control when the ACPI methods are invoked in relation to the driver-specific methods (e.g. if an iic child device driver had its own suspend/resume methods). You wouldn't want _PS3 to get run before the driver has a chance to save state for example. For PCI we manage this by having the ACPI PCI bus driver invoke any needed ACPI methods in the right order explicitly related to the PCI device driver methods. I think you probably don't want device_t nodes for acpi_iicdev around at all, but instead probably want to just delete them outright when you encounter one unless it is already attached by some other driver.

sys/dev/iicbus/acpi_iicbus.c

383

This probably belongs in the ig4iic driver (and any other i2c controller drivers) rather than this file. You would just export the acpi_iicbus_driver publicly.

[wulf]

In D22901#501848, @jhb wrote:

So it would be helpful to see an acpidump of a system with these devices.

I do not have such a devices. I can provide dumps made by someone else only. See e.g. https://github.com/major/xps-13-9343-dsdt/blob/master/DSDT.dsl . Interesting device is _SB_.PCI0.I2C0.TPD4. It has _PS0, _PS3 and even _PRW methods defined.

If I understand correctly, you leave acpi_iicdev around to attach to the device enumerated by acpi0?

Yes. It is done to reuse acpi bus resource parser and suspend/resume support.

For PCI we mostly avoided this since we ignored devices without _HID avoiding the need to copy resources over. It's not clear to me why you need to leave this device around for suspend/resume to work. The iicbus device will be invoked during suspend/resume and can pass those events down to children as needed. The acpi_iicbus driver should be responsible for invoking any needed ACPI methods on handles as part of that. You really want that behavior so you can control when the ACPI methods are invoked in relation to the driver-specific methods (e.g. if an iic child device driver had its own suspend/resume methods). You wouldn't want _PS3 to get run before the driver has a chance to save state for example. For PCI we manage this by having the ACPI PCI bus driver invoke any needed ACPI methods in the right order explicitly related to the PCI device driver methods.

I think you probably don't want device_t nodes for acpi_iicdev around at all, but instead probably want to just delete them outright when you encounter one unless it is already attached by some other driver.

Ok. It looks like splitting functionality between 2 drivers makes things unclean. I will rewrite the driver to not use acpi_iicdev for the cost of some code duplication.

[wulf]

Driver reworked to not depend on existing ACPI newbus device.
It deletes ACPI-hosted child and performs resource (IRQ) parsing and children power state (S0<->S3) management on his own.
No dummy driver required anymore.

"I2C address space handler" has been added also. It is disabled by default with loader tunable as nontested. It is required to query e.g. battery charge level via I2C bus by ACPI subsystem.