Hello Kieran, On 7/10/19 9:39 PM, Kieran Bingham wrote: > I2C drivers match against an I2C ID table, an OF table, and an ACPI > table. It is now also possible to match against an OF table entry > without the vendor prefix to support backwards compatibility, and allow > simplification of the i2c probe functions. > > As part of this matching, the probe function is being converted to > remove the need to specify the i2c_device_id table, but to support > module aliasing, we still require to have the MODULE_DEVICE_TABLE entry. > My opinion on this is that I2C drivers should register the tables of the firmware interfaces that support. That is, if a driver is only used in a platform that supports OF then it should only require an OF device table. But if the driver supports devices that can also be present in platforms that use ACPI, then should also require to have an ACPI device ID table. So there should be consistency about what table is used for both matching a device with a driver and reporting a modalias to user-space for module auto-loading. If a I2C device was instantiated by OF, then the OF table should be used for both reporting a modalias uevent and matching a driver. Now, the i2c_of_match_device() function attempts to match by first calling of_match_device() and if fails fallbacks to i2c_of_match_device_sysfs(). The latter attempts to match the I2C device by striping the vendor prefix of the compatible strings on the OF device ID table. That means that you could instantiate an I2C device ID through the sysfs interface and the OF table would be used to match the driver. But i2c_device_uevent() would had reported an I2C modalias and not an OF modalias (since the registered device won't have an associated of_node) so there's inconsistency in that case since a table is used to match but no to report modaliases. > Facilitate generating the I2C aliases directly from the of_device_id > tables, by stripping the vendor prefix prefix from the compatible string > and using that as an alias just as the i2c-core supports. > I see two ways to solve this issue. One is with $SUBJECT since we can argue that if a OF-only driver is able to match devices that were instantiated through the sysfs interface that only have a device name, then a modalias of the form i2c:<foo> is needed. Since the compatible strings without the vendor prefix is used to match, then I think that makes sense to also use them without the vendor prefix to populate I2C modaliases as $SUBJECT does. The other option is to remove i2c_of_match_device() and don't make OF match to fallback to i2c_of_match_device_sysfs(). This is what happens in the ACPI case, since i2c_device_match() just calls acpi_driver_match_device() directly and doesn't have a wrapper function that fallbacks to sysfs matching. In this case an I2C device ID table would be required if the devices have to be instantiated through sysfs. That way the I2C table would be used both for auto-loading and also to match the device when it doesn't have an of_node. If the former is the correct way to solve this then the patch looks good to me. Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat
