Attention is currently required from: Subrata Banik, Matt DeVillier, Tim Wawrzynczak, Karthik Ramasubramanian. Julius Werner 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:
copy that. […]
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.)
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.)
-
Julius Werner (Code Review)