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

[Marcus Folkesson]

On Fri, Apr 11, 2025 at 10:26:47AM +0200, Javier Martinez Canillas wrote:
> Marcus Folkesson <marcus.folkesson@xxxxxxxxx> writes:
> 
> Hello Marcus,
> 
> [...]
> 
> >> 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.
> 
> Sure, that makes sense to me.
> 
> Can I ask if you could still add reasonable default values that could be set
> in the device ID .data fields ?
> 
> As mentioned, I've a ST7567 based LCD and a WIP driver for it. But I could
> just drop that and use your driver, since AFAICT the expected display data
> RAM format is exactly the same than when using monochrome for the ST7571.
> 
> But due the ST7567 only supporting R1, it would be silly to always have to
> define a property in the DT node given that does not support other format.

Sure!
I've looked at the ST7567 datasheet and it seems indeed to be a very similar.
Both in pixel format and registers are the same.

I think specify a init-function (as those will differ) and constraints will
be enough to handle both chips.

I will prepare .data with something like this

struct st7571_panel_constraints {
	u32 min_nlines;
	u32 max_nlines;
	u32 min_ncols;
	u32 max_ncols;
	bool support_grayscale;
};

struct st7571_panel_data {
	int (*init)(struct st7571_device *st7571);
	struct st7571_panel_constraints constraints;
};

struct st7571_panel_data st7571_data = {
	.init = st7571_lcd_init,
	.constraints = {
		.min_nlines = 1,
		.max_nlines = 128,
		.min_ncols = 128,
		.max_ncols = 128,
		.support_grayscale = true,
	},
};

static const struct of_device_id st7571_of_match[] = {
	{ .compatible = "sitronix,st7571", .data = &st7571_data },
	{},
};


I can add an entry for the ST7567 when everything is in place.
The chip does not support the I2C interface, so it has to wait until
I've split up the driver though, which will be right after this series.

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

Best regards,
Marcus Folkesson
>

Attachment: signature.asc
Description: PGP signature