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

[Marcus Folkesson <marcus.folkesson@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3511 bytes --]

Hello Javier,

On Wed, Apr 09, 2025 at 11:43:54AM +0200, Javier Martinez Canillas wrote:
> Marcus Folkesson <marcus.folkesson@gmail.com> writes:
> 
> Hello Marcus,
> 
> [...]
> 
> >> 
> >> That's a god question, I don't really know...
> >> 
> >> But fbdev does support XRGB8888, which may be another good reason to add
> >> it and make it the default format. Yes, it will cause an unnecessary pixel
> >> format conversion but the I2C transport is so slow anyways that compute is
> >> not the bottleneck when using these small displays.
> >
> > Hrm, I now realised that I have another issue.
> > Not all LCDs that will be attached to the ST7571 controller will be
> > grayscale.
> > The display I've attached to the ST7571 is a monochrome LCD for example.
> >
> 
> Oh, that's very interesting. This means that vendors are using a more capable IC
> (i.e: ST7571) for display controllers + LCD panels board designs, even where they
> could had used a less capable one (i.e: ST7765). That is, using an IC that supports
> 2-bit grayscale when they could just used one that only supported monochrome pixels.
> 
> From a quick search, I found for example this one from SinoCrystal:
> 
> https://displaysino.com/product_details/SC128128012-V01.html
> 
> > Maybe the right way to do it is to only support XRGB8888 and specify 
> > if the display is monochrome or grayscale in the device tree.
> >
> > Or do you have any good suggestions?
> >
> 
> I don't know the proper way to handle this, but what I would do is to include
> the actual boards as entries in the OF device ID table instead of just the ICs.
> 
> And then for each entry you can specify what formats are supported, e.g:
> 
> static const uint32_t monochrome_formats[] = {
> 	DRM_FORMAT_XRGB8888,
>         DRM_FORMAT_R1
> };
> 
> static const uint32_t grayscale_formats[] = {
> 	DRM_FORMAT_XRGB8888,
>         DRM_FORMAT_R1
>         DRM_FORMAT_R2
> };
> 
> static const struct of_device_id st7571_of_match[] = {
> 	/* monochrome displays */
> 	{
> 		.compatible = "sinocrystal,sc128128012-v01",
> 		.data = monochrome_formats,
> 	},
> ...
>         /* grayscale displays */
> 	{
> 		.compatible = "foo,bar",
> 		.data = grayscale_formats,
> 	},
> };
> 
> and then in your probe callback, you can get the correct format list for
> the device matched. Something like the following for example:
> 
> static int st7571_probe(struct i2c_client *client) {
>        const uint32_t *formats = device_get_match_data(client->dev);
>        ...
> 
>        ret = drm_universal_plane_init(..., formats, ...);
>        ...
> };
> 
> Likely you will need to define more stuff to be specific for each entry, maybe
> you will need different primary plane update handlers too. Similar to how I had  
> to do it the ssd130x driver to support all the Solomon OLED controller families:
> 
> https://elixir.bootlin.com/linux/v6.11/source/drivers/gpu/drm/solomon/ssd130x.c#L1439

Thanks, that sounds like a good idea.

I've now experimenting with XRGB8888, and, well, it works. I guess.
The thresholds levels in the all conversion steps for  XRGB8888 -> 8 bit grayscale -> monochrome
makes my penguin look a bit boring.

But I guess that is expected.

Please compare
https://www.marcusfolkesson.se/xrgb8888.png
and
https://www.marcusfolkesson.se/c1.png

> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
> 

Best regards,
Marcus Folkesson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]