Hi Am 11.06.23 um 01:18 schrieb Javier Martinez Canillas: > Conor Dooley writes: > >> On Sat, Jun 10, 2023 at 07:51:35PM +0200, Javier Martinez Canillas wrote: >>> Conor Dooley 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)