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

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

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi,
>
> lots of good points in the review.
>
> Am 08.04.25 um 12:44 schrieb Javier Martinez Canillas:
> [...]
>>> 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.
>>
>> Maybe there's even common code among these drivers and could be reused?
>>
>> Just a thought though, it's OK to keep your driver as-is and we could do
>> refactor / move drivers around as follow-up if agreed that is desirable.
>
> That sounds like a good idea. But the other existing drivers are based 
> on mipi-dbi helpers, while this one isn't. Not sure if that's important 
> somehow.
>

Yeah, I don't know. In any case, the driver / module name is not an ABI so
we can always move around the files later if needed.

>>
>>>   3 files changed, 733 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
>>> index 94cbdb1337c07f1628a33599a7130369b9d59d98..33a69aea4232c5ca7a04b1fe18bb424e0fded697 100644
>>> --- a/drivers/gpu/drm/tiny/Kconfig
>>> +++ b/drivers/gpu/drm/tiny/Kconfig
>>> @@ -232,6 +232,17 @@ config TINYDRM_ST7586
>>>   
> [...]
>>> +
>>> +static const uint32_t st7571_primary_plane_formats[] = {
>>> +	DRM_FORMAT_C1,
>>> +	DRM_FORMAT_C2,
>>> +};
>>> +
>> I would add a DRM_FORMAT_XRGB8888 format. This will allow your display to
>> be compatible with any user-space. Your st7571_fb_blit_rect() can then do
>> a pixel format conversion from XRGB8888 to the native pixel format.
>
> It would be a starting point for XRGB8888 on C1/R1. I always wanted to 
> reimplement drm_fb_xrgb8888_to_mono() [1] with the generic _xfrm_ 
> helpers. Once the generic helpers can do such low-bit formats, C2 would 
> also work easily.
>
> [1] 
> https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/gpu/drm/drm_format_helper.c#L1114
>

Agreed. But even in its current form that helper is what I had in mind and
what is used by the ssd130x driver too for XRGB8888 -> R1 conversion. There
is no drm_fb_xrgb8888_to_gray2(), but that could be added as a part of this
driver series.

> Best regards
> Thomas
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat