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 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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat