Re: [PATCH v3 5/7] (WIP) drm/solomon: Add SSD130X OLED displays SPI support

[Javier Martinez Canillas <javierm@redhat.com>]

Sorry for the long reply but I don't really know how to make it shorter.

On 2/9/22 17:25, Geert Uytterhoeven wrote:
> Hi Javier,

[snip]

>> Did you see my comment about automatic module loading ? I think
>> that would be an issue if we use the same compatible for both
>> I2C and SPI drivers.
> 
> We have several drivers that have a core and separate i2c and spi
> wrappers, see e.g.
> 
> $ git grep -w ltc2947_of_match
> drivers/hwmon/ltc2947-core.c:const struct of_device_id ltc2947_of_match[] = {
> drivers/hwmon/ltc2947-core.c:EXPORT_SYMBOL_GPL(ltc2947_of_match);
> drivers/hwmon/ltc2947-core.c:MODULE_DEVICE_TABLE(of, ltc2947_of_match);
> drivers/hwmon/ltc2947-i2c.c:            .of_match_table = ltc2947_of_match,
> drivers/hwmon/ltc2947-spi.c:            .of_match_table = ltc2947_of_match,
> drivers/hwmon/ltc2947.h:extern const struct of_device_id ltc2947_of_match[];
> 
> Are they all broken?
>

It doesn't have an easy answer. It depends on what device ID tables have to
match, what modalias are present in the kernel modules and for what buses
are since not all subsystem reports the modalias uevent in the same manner.

Let's take this particular driver for example, the module aliases are the
following for each kernel module:

$ modinfo drivers/hwmon/ltc2947-core.ko | grep alias
alias:          of:N*T*Cadi,ltc2947C*
alias:          of:N*T*Cadi,ltc2947

$ modinfo drivers/hwmon/ltc2947-i2c.ko | grep alias
alias:          i2c:ltc2947

$ modinfo drivers/hwmon/ltc2947-spi.ko | grep alias
alias:          spi:ltc2947

The DT node will just contain a node with compatible = "adi,ltc2947", and
depending if the device is on a I2C or SPI bus, the OF subsystem will do
a device registration with the proper bus_type.

If is SPI, the subsystem always report a modalias uevent of the type
"spi:ltc2947", even if the device was registered by OF. So for SPI the
proper module will be loaded since it contains an alias "spi:ltc2947".

But I2C does report a proper OF modalias, so it will report instead
"of:N*T*Cadi,ltc2947" which will match the core module but not the I2C.

The I2C ltc2947-i2c.ko module is missing a "of:N*T*Cadi,ltc2947" alias
and the module loading likely isn't working and needs to be done manually.

Conversely, if ltc2947-spi.ko only add "of:N*T*Cadi,ltc2947", then the
automatic module loading wouldn't work because the modalias uevent will
be "spi:ltc2947" which won't match "of:N*T*Cadi,ltc2947". So everything
is really fragile.

Note also that the "T*" in the alias, that's to denote the device type
according to the ePAPR spec but that's only filled in the DT node has a
device_type DT property, and most DT nodes don't fill this.

So the driver module is greedy and will match any device type. And also
the modalias reported by the kernel won't set this, i.e:

$ cat /sys/bus/i2c/drivers/ssd130x-i2c/1-003c/modalias 
of:NoledT(null)Csolomon,ssd1306fb-i2c

The only way I found to make this robust and always working is with each
driver to define its own tables depending on what is used to match and
report the modalias to user-space. That means having an of_device_id and
spi_device_id (just for modalias reporting and alias table in module) for
SPI and a of_device_id table for I2C and platform devices.

And also having different compatible strings with a "-i2c" and "-spi" as
suffixes to allow kmod / udev to differentiate and match the modules.

Otherwise you will get "of:N(null)T(null)Cadi,ltc2947" in both cases and
user-space won't be able to figure out which module to match AFAICT.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat