Re: [PATCH v3 2/3] drm/st7571-i2c: add support for Sitronix ST7571 LCD controller

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

Marcus Folkesson <marcus.folkesson@gmail.com> writes:

> Hello Javier,
>
> On Tue, Apr 08, 2025 at 12:44:46PM +0200, Javier Martinez Canillas wrote:
>> Marcus Folkesson <marcus.folkesson@gmail.com> writes:
>> 
>> Hello Marcus,
>> 
>> > Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller.
>> > The controller has a SPI, I2C and 8bit parallel interface, this
>> > driver is for the I2C interface only.
>> >
>> 
>> I would structure the driver differently. For example, what was done
>> for the Solomon SSD130X display controllers, that also support these
>> three interfaces:
>> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/solomon
>> 
>> Basically, it was split in a ssd130x.c module that's agnostic of the
>> transport interface and implements all the core logic for the driver.
>> 
>> And a set of different modules that have the interface specific bits:
>> ssd130x-i2c.c and ssd130x-spi.c.
>> 
>> That way, adding for example SPI support to your driver would be quite
>> trivial and won't require any refactoring. Specially since you already
>> are using regmap, which abstracts away the I2C interface bits.
>> 
>> > Reviewed-by: Thomas Zimmermann <tzimmrmann@suse.de>
>> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
>> > ---
>> >  drivers/gpu/drm/tiny/Kconfig      |  11 +
>> >  drivers/gpu/drm/tiny/Makefile     |   1 +
>> >  drivers/gpu/drm/tiny/st7571-i2c.c | 721 ++++++++++++++++++++++++++++++++++++++
>> 
>> I personally think that the tiny sub-directory is slowly becoming a
>> dumping ground for small drivers. Instead, maybe we should create a
>> drivers/gpu/drm/sitronix/ sub-dir and put all Sitronix drivers there?
>> 
>> So far we have drivers in tiny for: ST7735R, ST7586 and ST7571 with
>> your driver. And also have a few more Sitronix drivers in the panel
>> sub-directory (although those likely should remain there).
>> 
>> I have a ST7565S and plan to write a driver for it. And I know someone
>> who is working on a ST7920 driver. That would be 5 Sitronix drivers and
>> the reason why I think that a dedicated sub-dir would be more organized.
>
> I'm looking into moving all the (tiny) Sitronix drivers into their own

Great!

> subdirectory.
> When doing that, should I replace the TINYDRM part with DRM for those drivers?
> E.g. CONFIG_TINYDRM_ST7735R -> CONFIG_DRM_ST7735R.
>

I would drop the TINY prefix. That's also why I thought that your new
driver not having TINY in its Kconfig symbol name was the correct call. 

> Or do we want to keep the config name intact?
>
> Best regards,
> Marcus Folkesson

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat