Attention is currently required from: Subrata Banik, Matt DeVillier, Julius Werner, Karthik Ramasubramanian. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63209 )
Change subject: device/i2c_bus: Add detect function pointer in i2c_bus_ops ......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
My concern here is that the I2C interface is implemented by many different drivers in coreboot, so adding a whole new primitive to it would cause a lot of churn if we actually wanted it supported widely. You're just implementing it for the DesignWare driver here but if people wanted to use it with any other driver they'd have to implement it in all of those too. Basing this feature off the existing primitive we already have automatically brings in support for all the other drivers for free. (I guess for drivers that don't implement 0-length writes correctly at the moment that's not that useful, but there might still be some of them which do.)
Am I missing the ones that do? I only see designware (src/drivers/i2c/designware/dw_i2c.c) and kempld EC (src/ec/kontron/kempld/kempld_i2c.c)
And then there's the code duplication concern, as I mentioned. We don't want each of these drivers to essentially duplicate their transfer function for a special purpose. If you want to simplify things for the caller, you can still just write a helper function above the driver interface layer that's called ...detect() but calls the ->transfer() method with length 0 under the hood.
(And then there's the question if we even need 0-length write support? There may be other ways of I2C probing that our existing drivers are more likely to support, e.g. it seems like the i2cdetect utility can also use the "read 1 byte (without writing register address first)" mode.)
If you're really against adding a new CB, we could try making the Designware driver work in this case.
Right now, this is duplicating the functionality of the "linux,probed" patch that ChromeOS has kept in the kernel for many years (NAKed upstream but we just kept using it, and it breaks TPs & TS on non-ChromeOS), and the patch always did a 0-byte write to the address and used ACK/NAK to decide present/not present.
-
Tim Wawrzynczak (Code Review)