Re: [PATCH v2 2/5] dt-bindings: display: ssd1307fb: Remove default width and height values

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3705 bytes --]

Hi

Am 11.06.23 um 01:18 schrieb Javier Martinez Canillas:
> Conor Dooley <conor@kernel.org> writes:
> 
>> On Sat, Jun 10, 2023 at 07:51:35PM +0200, Javier Martinez Canillas wrote:
>>> Conor Dooley <conor@kernel.org> writes:
>>>
>>>> On Fri, Jun 09, 2023 at 07:09:37PM +0200, Javier Martinez Canillas wrote:
>>>>> A default resolution in the ssd130x driver isn't set to an arbitrary 96x16
>>>>> anymore. Instead is set to a width and height that's controller dependent.
>>>>
>>>> Did that change to the driver not break backwards compatibility with
>>>> existing devicetrees that relied on the default values to get 96x16?
>>>>
>>>
>>> It would but I don't think it is an issue in pratice. Most users of these
>>> panels use one of the multiple libraries on top of the spidev interface.
>>>
>>> For the small userbase that don't, I believe that they will use the rpif
>>> kernel and ssd1306-overlay.dtbo DTB overlay, which defaults to width=128
>>> and height=64 [1]. So those users will have to explicitly set a width and
>>> height for a 96x16 panel anyways.
>>>
>>> The intersection of users that have a 96x16 panel, assumed that default
>>> and consider the DTB a stable ABI, and only update their kernel but not
>>> the  DTB should be very small IMO.
>>
>> It's the adding of new defaults that makes it a bit messier, since you
>> can't even revert without potentially breaking a newer user. I'd be more
>> inclined to require the properties, rather change their defaults in the
>> binding, lest there are people relying on them.
> 
> I think that's OK, the old drivers/video/fbdev/ssd1307fb.c fbdev driver
> still has the old behaviour so it will only be a problem for users that
> want to move to the new ssd130x driver as well.
> 
> By looking at the git log history, the 96x16 resolution was chosen when
> the driver was merged because Maxime tested it on a CFA10036 board [1]
> that has a 96x16 panel that uses an SSD1307 controller.
> 
> But as mentioned, that chip can drive up to 128x39 pixels so the maximum
> makes more sense as default to me.
> 
> [1]: https://www.crystalfontz.com/product/cfa10036
> 
>> If you and the other knowledgeable folk in the area really do know such
>> users do not exist then I suppose it is fine to do.
> 
> I believe is fine, since as explained above that change was only done in
> the ssd130x DRM driver, not the ssd1307fb fbdev driver whose default is
> still 96x16. Both drivers share the same DT binding scheme, I was asked
> to do that to make it as much backward compatible as possible with the
> old fbdev driver.
> 
> But I will be OK to drop the "solomon,ssd130?fb-i2c" compatible strings
> from the DRM driver and only match against the new "solomon,ssd130?-i2c"
> compatible strings. And add a different DT binding schema for the ssd130x
> driver, if that would mean being able to fix things like the one mentioned
> in this patch.
> 
> In my opinion, trying to always make the drivers backward compatible with
> old DTBs only makes the drivers code more complicated for unclear benefit.
> 
> Usually this just ends being code that is neither used nor tested. Because
> in practice most people update the DTBs and kernels, instead of trying to
> make the DTB a stable ABI like firmware.
> 

 From my understanding, fixing the resolution is the correct thing to do 
here. Userspace needs to be able to handle these differences.

Best regards
Thomas

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]