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

[Marcus Folkesson]

Hello,

On Wed, Apr 09, 2025 at 11:43:54AM +0200, Javier Martinez Canillas wrote:
> Marcus Folkesson <marcus.folkesson@xxxxxxxxx> 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,
> 	},
> };

A comment for v4:

I think I will go for a property in the device tree. I've implemented
board entries as above, but I'm not satisfied for two reasons:

1. All other properties like display size and resolution are already
   specified in the device tree. If I add entries for specific boards,
   these properties will be somehow redundant and not as generic.

2. I could not find a ST7571 with a grayscale display as a off-the-shelf
   product.
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat

Best regards,
Marcus Folkesson

Attachment: signature.asc
Description: PGP signature