Message-ID: <ffe5c7d4-d27f-ccb6-932e-e027e1ae14da@redhat.com>
Date:   Fri, 8 Apr 2022 21:19:24 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Rob Herring <robh@...nel.org>
Cc:     linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Chen-Yu Tsai <wens@...nel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Mark Brown <broonie@...nel.org>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Maxime Ripard <mripard@...nel.org>, devicetree@...r.kernel.org
Subject: Re: [PATCH 1/5] dt-bindings: display: ssd1307fb: Deprecate fbdev
 compatible strings

Hello Rob,

On 4/8/22 20:22, Rob Herring wrote:
> On Thu, Apr 07, 2022 at 10:02:00PM +0200, Javier Martinez Canillas wrote:
>> The current compatible strings for SSD130x I2C controllers contain an -fb
>> suffix, this seems to indicate that are for a fbdev driver. But the DT is
>> supposed to describe the hardware and not Linux implementation details.
> 
> True, but compatible is just an identifier. There's no reason to 
> deprecate unless the binding as a whole needs to be redone.
> 
> I imagine you also want 2 compatibles for 2 drivers. That's saying you 
> should change your firmware to switch drivers. The fact that we have 2 
> drivers for the same h/w is a kernel problem. Don't bring DT into it.
>

No, that's not what I meant. In fact, we currently have two drivers that
match against the same set of compatible strings. These drivers are:

* drivers/video/fbdev/ssd1307fb.c
* drivers/gpu/drm/solomon/ssd130x-i2c.c

what I don't personally like about the current compatible strings is that
the *driver* name was encoded on those, rather than the IC names. So for
instance there's a "solomon,ssd1307fb-i2c" (notice the fb suffix) instead
of just "solomon,ssd1307-i2c" or "solomon,ssd1307".

When I ported the fbdev driver to DRM, I considered using different values
for the compatible strings but decided to just use the same for backward
compatibility.

But now I want to add compatible strings for OLED controllers that use a
SPI interface instead, and I don't really want to add a compatible string
"solomon,ssd1307fb-spi" but just without the "fb".

I want the SPI compatible strings to be consistent with the I2C ones though,
hence the deprecation so new DTS could just use the ones without a "fb".

Now, if you say that I can't do add new ones for I2C, then I will just add
"solomon,ssd1307fb-spi" and similar even though I don't like the "fb" there.

And just to make clear, the DRM driver will continue matching against both
compatible strings, but the fbdev will only match the old "fb" ones.

[snip]

>> +
>> +      # SSD130x I2C controllers
>> +      - items:
>> +          - enum:
>> +              - sinowealth,sh1106-i2c
>> +              - solomon,ssd1305-i2c
>> +              - solomon,ssd1306-i2c
>> +              - solomon,ssd1307-i2c
>> +              - solomon,ssd1309-i2c
> 
> There's also no reason to put the bus interface into the compatible as 
> the same compatible will work on different buses. But since you want to 
> add SPI, just using the 'i2c' one will confuse people. For that reason 
> you could add 'solomon,ssd1305', etc. for both SPI support and I2C DRM.

That's not really true. There's a reason to add per bus compatible strings
at least in Linux. And is that there's no information about the bus types
in module aliases that are reported to user-space for module auto-loading.

For example, 

$ cat /sys/devices/platform/soc/fe804000.i2c/i2c-1/1-003c/modalias 
of:NoledT(null)Csolomon,ssd1306fb-i2c

$ cat /sys/devices/platform/soc/fe804000.i2c/i2c-1/1-003c/uevent 
DRIVER=ssd130x-i2c
OF_NAME=oled
OF_FULLNAME=/soc/i2c@...04000/oled@3c
OF_COMPATIBLE_0=solomon,ssd1306fb-i2c
OF_COMPATIBLE_N=1
MODALIAS=of:NoledT(null)Csolomon,ssd1306fb-i2c

and

$ modinfo ssd130x-i2c | grep alias
alias:          of:N*T*Csolomon,ssd1309fb-i2cC*
alias:          of:N*T*Csolomon,ssd1309fb-i2c
alias:          of:N*T*Csolomon,ssd1307fb-i2cC*
alias:          of:N*T*Csolomon,ssd1307fb-i2c
alias:          of:N*T*Csolomon,ssd1306fb-i2cC*
alias:          of:N*T*Csolomon,ssd1306fb-i2c
alias:          of:N*T*Csolomon,ssd1305fb-i2cC*
alias:          of:N*T*Csolomon,ssd1305fb-i2c
alias:          of:N*T*Csinowealth,sh1106-i2cC*
alias:          of:N*T*Csinowealth,sh1106-i2c

this module will match against any MODALIAS uevent that has one of the
listed compatible strings in "C" and any node name in "N". But also for
any type "T".

And even if the module alias was more restrictive and say only matched
against 'of:N*Ti2cCsolomon,ssd1307fb-i2c', the type information is not
filled by the bus drivers.

So, if we just had a "solomon,ssd1307" compatible string, then a device
registered through OF could lead to the wrong kernel module to be loaded.

In other words, it's true that having a single compatible strings for all
bus drivers will work for device -> driver matching but may not work for
module auto-loading.

> (You should also support the 'fb-i2c' variant in DRM IMO, but doubtful 
> that I'll review that.)
>

As mentioned above, it does even after adding support for the new strings,
for backward compatibility.
 
> Rob
> 

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Powered by blists - more mailing lists