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

[Javier Martinez Canillas]

On 2/9/22 13:25, Geert Uytterhoeven wrote:
> Hi Javier,
> 
> On Wed, Feb 9, 2022 at 10:12 AM Javier Martinez Canillas
> <javierm@xxxxxxxxxx> wrote:
>> The ssd130x driver only provides the core support for these devices but it
>> does not have any bus transport logic. Add a driver to interface over SPI.
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> 
> Thanks for your patch!
> 
>> --- /dev/null
>> +++ b/drivers/gpu/drm/solomon/ssd130x-spi.c
> 
>> +static const struct of_device_id ssd130x_of_match[] = {
>> +       {
>> +               .compatible = "solomon,ssd1305fb-spi",
> 
> This needs an update to the DT bindings.

Yes, I know. Didn't feel like it, because the patch is a WIP anyways
(I haven't tested it but was included just for illustration purposes).

If someone confirms that works then I will include a proper DT binding
in the next revision.

> Hence this may be a good time to deprecate the existing
> "solomon,ssd130*fb-i2c" compatible values, and switch to
> "solomon,ssd130*fb" instead, for both I2C and SPI.

Is this the preferred approach ? Asking because most of the drivers I
know use this -$bus suffix. From a device <--> driver matching point
of view, shouldn't be an issue to have two different drivers to use
the same compatible strings, as long as these are for different buses.

Since AFAIK the match only happens within the same struct bus_type. But
I wonder if this could cause issues in other places, for example the
module loading. IIRC the OF modaliases don't include the device type.

If instead the drivers were old platform drivers and have an i2c_device_id
and spi_device_id tables, then using the same device name would not be an
issue due the modalias having a i2c: and spi: prefix to make a distinction.

What I think we should do is drop the "fb" part, since that seemed to me
that was included because it was an fbdev driver. And not really hardware
description.

> Of course the I2C subdriver still has to bind against the old values,
> too, for backwards compatibility.
>

Yes, agreed.
 
>> +               .data = (void *)&ssd130x_ssd1305_deviceinfo,
> 
> The casts are not needed.
>

Ok.

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