Add support for Sitronix ST7571 LCD controller

Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller.

Signed-off-by Marcus Folkesson <[email protected]>

Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller. The controller has a SPI, I2C and 8bit parallel interface, this driver is for the I2C interface only.

Reviewed-by Thomas Zimmermann <[email protected]>

Signed-off-by Marcus Folkesson <[email protected]>

---
 drivers/gpu/drm/tiny/Kconfig      |  11 +
 drivers/gpu/drm/tiny/Makefile     |   1 +
 drivers/gpu/drm/tiny/st7571-i2c.c | 721 ++++++++++++++++++++++++++++++++++++++

 3 files changed, 733 insertions(+)

drivers/gpu/drm/tiny/Kconfig +0 -0

index 94cbdb1337c07f1628a33599a7130369b9d59d98..33a69aea4232c5ca7a04b1fe18bb424e0fded697 100644

drivers/gpu/drm/tiny/Kconfig +4 -0

+struct st7571_device {
+	struct drm_device dev;
+
+	struct drm_plane primary_plane;

drivers/gpu/drm/tiny/Kconfig +11 -0

+static void st7571_primary_plane_helper_atomic_update(struct drm_plane *plane,
+						   struct drm_atomic_state *state)
+{
+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
+	struct drm_framebuffer *fb = plane_state->fb;
+	struct drm_atomic_helper_damage_iter iter;
+	struct drm_device *dev = plane->dev;
+	struct drm_rect damage;
+	struct st7571_device *st7571 = drm_to_st7571(plane->dev);

drivers/gpu/drm/tiny/Kconfig +7 -0

+		return;
+	}
+
+	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
+	drm_atomic_for_each_plane_damage(&iter, &damage) {
+		st7571_fb_blit_rect(fb, &shadow_plane_state->data[0], &damage);
+	}

drivers/gpu/drm/tiny/Kconfig +4 -0

+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+

drivers/gpu/drm/tiny/Kconfig +24 -0

+static int st7571_probe(struct i2c_client *client)
+{
+	struct st7571_device *st7571;
+	struct drm_device *dev;
+	int ret;
+
+	st7571 = devm_drm_dev_alloc(&client->dev, &st7571_driver,
+				    struct st7571_device, dev);
+	if (IS_ERR(st7571))
+		return PTR_ERR(st7571);
+
+	dev = &st7571->dev;
+	st7571->client = client;
+	i2c_set_clientdata(client, st7571);
+
+	ret = st7571_parse_dt(st7571);
+	if (ret)
+		return ret;
+
+	st7571->mode = st7571_mode(st7571);
+
+	/*
+	 * The chip nacks some messages but still works as expected.
+	 * If the adapter does not support protocol mangling do

Quoted 6 lines

Reviewed-by Thomas Zimmermann <[email protected]>

Signed-off-by Marcus Folkesson <[email protected]>

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

Quoted 9 lines

  3 files changed, 733 insertions(+)

drivers/gpu/drm/tiny/Kconfig +0 -0

index 94cbdb1337c07f1628a33599a7130369b9d59d98..33a69aea4232c5ca7a04b1fe18bb424e0fded697 100644
@@ -232,6 +232,17 @@ config TINYDRM_ST7586
 

drivers/gpu/drm/tiny/Kconfig

Quoted 6 lines

drivers/gpu/drm/tiny/Kconfig +6 -0

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

...

Quoted 11 lines

drivers/gpu/drm/tiny/Kconfig +11 -0

+static void st7571_primary_plane_helper_atomic_update(struct drm_plane *plane,
+						   struct drm_atomic_state *state)
+{
+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
+	struct drm_framebuffer *fb = plane_state->fb;
+	struct drm_atomic_helper_damage_iter iter;
+	struct drm_device *dev = plane->dev;
+	struct drm_rect damage;
+	struct st7571_device *st7571 = drm_to_st7571(plane->dev);

drivers/gpu/drm/tiny/Kconfig

Quoted 10 lines

drivers/gpu/drm/tiny/Kconfig +10 -0

+	int ret, idx;
+
+	if (!fb)
+		return; /* no framebuffer; plane is disabled */
+
+	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+	if (ret)
+		return;
+
+	if (!drm_dev_enter(dev, &idx))

Should do a drm_gem_fb_end_cpu_access() here before returning.

drivers/gpu/drm/tiny/Kconfig

Quoted 5 lines

drivers/gpu/drm/tiny/Kconfig +5 -0

+		return;
+
+	ret = st7571_set_pixel_format(st7571, fb->format->format);
+	if (ret) {
+		dev_err(dev->dev, "Failed to set pixel format: %d\n", ret);

And here I think you need to do both drm_gem_fb_end_cpu_access() and drm_dev_exit().

Quoted 7 lines

drivers/gpu/drm/tiny/Kconfig +7 -0

+		return;
+	}
+
+	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
+	drm_atomic_for_each_plane_damage(&iter, &damage) {
+		st7571_fb_blit_rect(fb, &shadow_plane_state->data[0], &damage);
+	}

drivers/gpu/drm/tiny/Kconfig

Quoted 10 lines

drivers/gpu/drm/tiny/Kconfig +10 -0

+
+	drm_dev_exit(idx);
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+}
+
+static const struct drm_plane_helper_funcs st7571_primary_plane_helper_funcs = {
+	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+	.atomic_check = st7571_primary_plane_helper_atomic_check,
+	.atomic_update = st7571_primary_plane_helper_atomic_update,
+};

Maybe you want an .atomic_disable callback that clears your screen ?

drivers/gpu/drm/tiny/Kconfig

Quoted 7 lines

drivers/gpu/drm/tiny/Kconfig +7 -0

+
+/*
+ * CRTC
+ */
+
+static const struct drm_crtc_helper_funcs st7571_crtc_helper_funcs = {
+	.atomic_check = drm_crtc_helper_atomic_check,

I think you could have an .mode_valid callback that just checks the fixed mode.

drivers/gpu/drm/tiny/Kconfig

Quoted 7 lines

drivers/gpu/drm/tiny/Kconfig +7 -0

+/*
+ * Encoder
+ */
+
+static const struct drm_encoder_funcs st7571_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};

I recommend to have an encoder .atomic_{en,dis}able callbacks to init and turn off your display respectively. That way, the driver can call st7571_lcd_init() only when the display is going to be used instead of at probe time.

...

drivers/gpu/drm/tiny/Kconfig

Quoted 7 lines

drivers/gpu/drm/tiny/Kconfig +7 -0

+static enum drm_mode_status st7571_mode_config_mode_valid(struct drm_device *dev,
+						       const struct drm_display_mode *mode)
+{
+	struct st7571_device *st7571 = drm_to_st7571(dev);
+
+	return drm_crtc_helper_mode_valid_fixed(&st7571->crtc, mode, &st7571->mode);
+}

The fact that you are calling a drm_crtc_helper here is an indication that probably this should be done in a struct drm_crtc_helper_funcs .mode_valid callback instead, as mentioned above.

drivers/gpu/drm/tiny/Kconfig

Quoted 4 lines

drivers/gpu/drm/tiny/Kconfig +4 -0

+
+static const struct drm_mode_config_funcs st7571_mode_config_funcs = {
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.mode_valid = st7571_mode_config_mode_valid,

And that way you could just drop this handler.

Quoted 4 lines

drivers/gpu/drm/tiny/Kconfig +4 -0

+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+

...

Quoted 24 lines

drivers/gpu/drm/tiny/Kconfig +24 -0

+static int st7571_probe(struct i2c_client *client)
+{
+	struct st7571_device *st7571;
+	struct drm_device *dev;
+	int ret;
+
+	st7571 = devm_drm_dev_alloc(&client->dev, &st7571_driver,
+				    struct st7571_device, dev);
+	if (IS_ERR(st7571))
+		return PTR_ERR(st7571);
+
+	dev = &st7571->dev;
+	st7571->client = client;
+	i2c_set_clientdata(client, st7571);
+
+	ret = st7571_parse_dt(st7571);
+	if (ret)
+		return ret;
+
+	st7571->mode = st7571_mode(st7571);
+
+	/*
+	 * The chip nacks some messages but still works as expected.
+	 * If the adapter does not support protocol mangling do

drivers/gpu/drm/tiny/Kconfig

Quoted 10 lines

drivers/gpu/drm/tiny/Kconfig +10 -0

+	 * not set the I2C_M_IGNORE_NAK flag at the expense * of possible
+	 * cruft in the logs.
+	 */
+	if (i2c_check_functionality(client->adapter, I2C_FUNC_PROTOCOL_MANGLING))
+		st7571->ignore_nak = true;
+
+	st7571->regmap = devm_regmap_init(&client->dev, &st7571_regmap_bus,
+					   client, &st7571_regmap_config);
+	if (IS_ERR(st7571->regmap)) {
+		dev_err(&client->dev, "Failed to initialize regmap\n");

If you use dev_err_probe(), you can give some indication to users about what failed. It prints messages in the /sys/kernel/debug/devices_deferred debugfs entry.

drivers/gpu/drm/tiny/Kconfig

Quoted 6 lines

drivers/gpu/drm/tiny/Kconfig +6 -0

+
+static void st7571_remove(struct i2c_client *client)
+{
+	struct st7571_device *st7571 = i2c_get_clientdata(client);
+
+	drm_dev_unplug(&st7571->dev);

I think you are missing a drm_atomic_helper_shutdown() here.

And also a struct i2c_driver .shutdown callback to call to drm_atomic_helper_shutdown() as well.

Hi,

lots of good points in the review.

Am 08.04.25 um 12:44 schrieb Javier Martinez Canillas:
[...]

Quoted 18 lines

Quoted 6 lines

Reviewed-by Thomas Zimmermann <[email protected]>

Signed-off-by Marcus Folkesson <[email protected]>

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

Quoted 4 lines

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.

Quoted 9 lines

Quoted 9 lines

  3 files changed, 733 insertions(+)

drivers/gpu/drm/tiny/Kconfig +0 -0

index 94cbdb1337c07f1628a33599a7130369b9d59d98..33a69aea4232c5ca7a04b1fe18bb424e0fded697 100644
@@ -232,6 +232,17 @@ config TINYDRM_ST7586
 

[...]

Quoted 9 lines

drivers/gpu/drm/tiny/Kconfig

Quoted 6 lines

drivers/gpu/drm/tiny/Kconfig +6 -0

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

Best regards
Thomas

Marcus Folkesson <[email protected]> writes:

Hello Marcus,

Quoted 4 lines

Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller. The controller has a SPI, I2C and 8bit parallel interface, this driver is for the I2C interface only.

I would structure the driver differently. For example, what was done for the Solomon SSD130X display controllers, that also support these three interfaces:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/solomon

Basically, it was split in a ssd130x.c module that's agnostic of the transport interface and implements all the core logic for the driver.

And a set of different modules that have the interface specific bits:
ssd130x-i2c.c and ssd130x-spi.c.

That way, adding for example SPI support to your driver would be quite trivial and won't require any refactoring. Specially since you already are using regmap, which abstracts away the I2C interface bits.

Quoted 7 lines

Reviewed-by Thomas Zimmermann <[email protected]>

Signed-off-by Marcus Folkesson <[email protected]>

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

Review

Quoted 3 lines

(quoted diff) +2 -0

+#define drm_to_st7571(_dev) container_of(_dev, struct st7571_device, dev)
+

I usually prefer these to be static inline functions instead of a macro. That way you get type checking and the end result should be basically the same.

Quoted 5 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +4 -0

+struct st7571_device {
+	struct drm_device dev;
+
+	struct drm_plane primary_plane;

drivers/gpu/drm/tiny/st7571-i2c.c line 99

Quoted 10 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +10 -0

+	struct drm_crtc crtc;
+	struct drm_encoder encoder;
+	struct drm_connector connector;
+
+	struct drm_display_mode mode;
+
+	struct i2c_client *client;
+	struct regmap *regmap;
+	bool ignore_nak;
+

I know you mentioned that the chip sometimes nacks some I2C messages but maybe we want to better understand why that is the case before adding a flag like this?

In particular, I see in the "6.4 MICROPROCESSOR INTERFACE" section of the datasheet the following note:

"By connecting SDA_OUT to SDA_IN externally, the SDA line becomes fully I2C interface compatible. Separating acknowledge-output from serial data input is advantageous for chip-on-glass (COG) applications. In COG applications, the ITO resistance and the pull-up resistor will form a voltage divider, which affects acknowledge-signal level. Larger ITO resistance will raise the acknowledged-signal level and system cannot recognize this level as a valid logic “0” level. By separating SDA_IN from SDA_OUT, the IC can be used in a mode that ignores the acknowledge-bit. For applications which check acknowledge-bit, it is necessary to minimize the ITO resistance of the SDA_OUT trace to guarantee a valid low level."

...

drivers/gpu/drm/tiny/st7571-i2c.c line 240

Quoted 11 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +11 -0

+static int st7571_set_pixel_format(struct st7571_device *st7571,
+				   u32 pixel_format)
+{
+	switch (pixel_format) {
+	case DRM_FORMAT_C1:
+		return st7571_set_color_mode(st7571, ST7571_COLOR_MODE_BLACKWHITE);
+	case DRM_FORMAT_C2:
+		return st7571_set_color_mode(st7571, ST7571_COLOR_MODE_GRAY);
+	default:
+		return -EINVAL;
+	}

These should be DRM_FORMAT_R1 and DRM_FORMAT_R2 and not C{1,2}. The former is for displays have a single color (i.e: grey) while the latter is when a pixel can have different color, whose values are defined by a CLUT table.

...

drivers/gpu/drm/tiny/st7571-i2c.c line 258

Quoted 6 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +6 -0

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

drivers/gpu/drm/tiny/st7571-i2c.c line 322

Quoted 6 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +6 -0

+
+static const struct drm_plane_helper_funcs st7571_primary_plane_helper_funcs = {
+	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+	.atomic_check = st7571_primary_plane_helper_atomic_check,
+	.atomic_update = st7571_primary_plane_helper_atomic_update,
+};

Maybe you want an .atomic_disable callback that clears your screen ?

drivers/gpu/drm/tiny/st7571-i2c.c line 336

Quoted 9 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +7 -0

+
+/*
+ * CRTC
+ */
+
+static const struct drm_crtc_helper_funcs st7571_crtc_helper_funcs = {
+	.atomic_check = drm_crtc_helper_atomic_check,

I think you could have an .mode_valid callback that just checks the fixed mode.

drivers/gpu/drm/tiny/st7571-i2c.c line 354

Quoted 8 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +7 -0

+/*
+ * Encoder
+ */
+
+static const struct drm_encoder_funcs st7571_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};

I recommend to have an encoder .atomic_{en,dis}able callbacks to init and turn off your display respectively. That way, the driver can call st7571_lcd_init() only when the display is going to be used instead of at probe time.

...

drivers/gpu/drm/tiny/st7571-i2c.c line 374

Quoted 7 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +7 -0

+static enum drm_mode_status st7571_mode_config_mode_valid(struct drm_device *dev,
+						       const struct drm_display_mode *mode)
+{
+	struct st7571_device *st7571 = drm_to_st7571(dev);
+
+	return drm_crtc_helper_mode_valid_fixed(&st7571->crtc, mode, &st7571->mode);
+}

The fact that you are calling a drm_crtc_helper here is an indication that probably this should be done in a struct drm_crtc_helper_funcs .mode_valid callback instead, as mentioned above.

drivers/gpu/drm/tiny/st7571-i2c.c line 378

Quoted 5 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +4 -0

+
+static const struct drm_mode_config_funcs st7571_mode_config_funcs = {
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.mode_valid = st7571_mode_config_mode_valid,

And that way you could just drop this handler.

Quoted 5 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +4 -0

+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+

...

Quoted 30 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +30 -0

+static int st7571_probe(struct i2c_client *client)
+{
+	struct st7571_device *st7571;
+	struct drm_device *dev;
+	int ret;
+
+	st7571 = devm_drm_dev_alloc(&client->dev, &st7571_driver,
+				    struct st7571_device, dev);
+	if (IS_ERR(st7571))
+		return PTR_ERR(st7571);
+
+	dev = &st7571->dev;
+	st7571->client = client;
+	i2c_set_clientdata(client, st7571);
+
+	ret = st7571_parse_dt(st7571);
+	if (ret)
+		return ret;
+
+	st7571->mode = st7571_mode(st7571);
+
+	/*
+	 * The chip nacks some messages but still works as expected.
+	 * If the adapter does not support protocol mangling do
+	 * not set the I2C_M_IGNORE_NAK flag at the expense * of possible
+	 * cruft in the logs.
+	 */
+	if (i2c_check_functionality(client->adapter, I2C_FUNC_PROTOCOL_MANGLING))
+		st7571->ignore_nak = true;
+

drivers/gpu/drm/tiny/st7571-i2c.c line 637

Quoted 4 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +4 -0

+	st7571->regmap = devm_regmap_init(&client->dev, &st7571_regmap_bus,
+					   client, &st7571_regmap_config);
+	if (IS_ERR(st7571->regmap)) {
+		dev_err(&client->dev, "Failed to initialize regmap\n");

If you use dev_err_probe(), you can give some indication to users about what failed. It prints messages in the /sys/kernel/debug/devices_deferred debugfs entry.

drivers/gpu/drm/tiny/st7571-i2c.c line 691

Quoted 7 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +6 -0

+
+static void st7571_remove(struct i2c_client *client)
+{
+	struct st7571_device *st7571 = i2c_get_clientdata(client);
+
+	drm_dev_unplug(&st7571->dev);

I think you are missing a drm_atomic_helper_shutdown() here.

And also a struct i2c_driver .shutdown callback to call to drm_atomic_helper_shutdown() as well.

Signature · 5 lines

Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Quoted 16 lines

drivers/gpu/drm/tiny/st7571-i2c.c line 240

Quoted 12 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +11 -0

+static int st7571_set_pixel_format(struct st7571_device *st7571,
+				   u32 pixel_format)
+{
+	switch (pixel_format) {
+	case DRM_FORMAT_C1:
+		return st7571_set_color_mode(st7571, ST7571_COLOR_MODE_BLACKWHITE);
+	case DRM_FORMAT_C2:
+		return st7571_set_color_mode(st7571, ST7571_COLOR_MODE_GRAY);
+	default:
+		return -EINVAL;
+	}

These should be DRM_FORMAT_R1 and DRM_FORMAT_R2 and not C{1,2}. The former is for displays have a single color (i.e: grey) while the latter is when a pixel can have different color, whose values are defined by a CLUT table.

I see.
Does fbdev only works with CLUT formats? I get this error when I switch to DRM_FORMAT_R{1,2}:

[drm] Initialized st7571 1.0.0 for 0-003f on minor 0
st7571 0-003f: [drm] format C1 little-endian (0x20203143) not supported
st7571 0-003f: [drm] No compatible format found
st7571 0-003f: [drm] *ERROR* fbdev: Failed to setup emulation (ret=-22)

Quoted 4 lines

...

Quoted 9 lines

drivers/gpu/drm/tiny/st7571-i2c.c line 258

Quoted 6 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +6 -0

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

This were discussed in v2, but there were limitations in the helper functions that we currently have.

I will look into how this could be implemented in a generic way, but maybe that is something for a follow up patch?

[...]

Quoted 7 lines

drivers/gpu/drm/tiny/st7571-i2c.c line 322

Quoted 6 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +6 -0

+
+static const struct drm_plane_helper_funcs st7571_primary_plane_helper_funcs = {
+	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+	.atomic_check = st7571_primary_plane_helper_atomic_check,
+	.atomic_update = st7571_primary_plane_helper_atomic_update,
+};

Maybe you want an .atomic_disable callback that clears your screen ?

Good point, yes, I will add that.

Quoted 9 lines

drivers/gpu/drm/tiny/st7571-i2c.c line 336

Quoted 8 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +7 -0

+
+/*
+ * CRTC
+ */
+
+static const struct drm_crtc_helper_funcs st7571_crtc_helper_funcs = {
+	.atomic_check = drm_crtc_helper_atomic_check,

I think you could have an .mode_valid callback that just checks the fixed mode.

Got it.

Quoted 10 lines

drivers/gpu/drm/tiny/st7571-i2c.c line 354

Quoted 7 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +7 -0

+/*
+ * Encoder
+ */
+
+static const struct drm_encoder_funcs st7571_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};

I recommend to have an encoder .atomic_{en,dis}able callbacks to init and turn off your display respectively. That way, the driver can call st7571_lcd_init() only when the display is going to be used instead of at probe time.

I will look into this as well.

Quoted 12 lines

...

drivers/gpu/drm/tiny/st7571-i2c.c line 374

Quoted 7 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +7 -0

+static enum drm_mode_status st7571_mode_config_mode_valid(struct drm_device *dev,
+						       const struct drm_display_mode *mode)
+{
+	struct st7571_device *st7571 = drm_to_st7571(dev);
+
+	return drm_crtc_helper_mode_valid_fixed(&st7571->crtc, mode, &st7571->mode);
+}

The fact that you are calling a drm_crtc_helper here is an indication that probably this should be done in a struct drm_crtc_helper_funcs .mode_valid callback instead, as mentioned above.

I will move it to drm_crtc_helper_funcs.

Quoted 5 lines

drivers/gpu/drm/tiny/st7571-i2c.c line 378

Quoted 4 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +4 -0

+
+static const struct drm_mode_config_funcs st7571_mode_config_funcs = {
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.mode_valid = st7571_mode_config_mode_valid,

And that way you could just drop this handler.

Yep, thanks.

Quoted 35 lines

Quoted 4 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +4 -0

+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+

...

Quoted 29 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +29 -0

+static int st7571_probe(struct i2c_client *client)
+{
+	struct st7571_device *st7571;
+	struct drm_device *dev;
+	int ret;
+
+	st7571 = devm_drm_dev_alloc(&client->dev, &st7571_driver,
+				    struct st7571_device, dev);
+	if (IS_ERR(st7571))
+		return PTR_ERR(st7571);
+
+	dev = &st7571->dev;
+	st7571->client = client;
+	i2c_set_clientdata(client, st7571);
+
+	ret = st7571_parse_dt(st7571);
+	if (ret)
+		return ret;
+
+	st7571->mode = st7571_mode(st7571);
+
+	/*
+	 * The chip nacks some messages but still works as expected.
+	 * If the adapter does not support protocol mangling do
+	 * not set the I2C_M_IGNORE_NAK flag at the expense * of possible
+	 * cruft in the logs.
+	 */
+	if (i2c_check_functionality(client->adapter, I2C_FUNC_PROTOCOL_MANGLING))
+		st7571->ignore_nak = true;

Quoted 8 lines

drivers/gpu/drm/tiny/st7571-i2c.c line 637

Quoted 5 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +5 -0

+
+	st7571->regmap = devm_regmap_init(&client->dev, &st7571_regmap_bus,
+					   client, &st7571_regmap_config);
+	if (IS_ERR(st7571->regmap)) {
+		dev_err(&client->dev, "Failed to initialize regmap\n");

If you use dev_err_probe(), you can give some indication to users about what failed. It prints messages in the /sys/kernel/debug/devices_deferred debugfs entry.

Got it, thanks.

Quoted 7 lines

drivers/gpu/drm/tiny/st7571-i2c.c line 691

Quoted 6 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +6 -0

+
+static void st7571_remove(struct i2c_client *client)
+{
+	struct st7571_device *st7571 = i2c_get_clientdata(client);
+
+	drm_dev_unplug(&st7571->dev);

I think you are missing a drm_atomic_helper_shutdown() here.

This is a change for v3. As the device has been unplugged already, it won't do anything, so I removed it.

Isn't it right to do so?

Quoted 10 lines

And also a struct i2c_driver .shutdown callback to call to drm_atomic_helper_shutdown() as well.

Signature · 5 lines

Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Best regards,
Marcus Folkesson

Hi

Am 08.04.25 um 15:20 schrieb Marcus Folkesson:
[...]

Quoted 16 lines

Quoted 16 lines

drivers/gpu/drm/tiny/st7571-i2c.c line 240

Quoted 12 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +11 -0

+static int st7571_set_pixel_format(struct st7571_device *st7571,
+				   u32 pixel_format)
+{
+	switch (pixel_format) {
+	case DRM_FORMAT_C1:
+		return st7571_set_color_mode(st7571, ST7571_COLOR_MODE_BLACKWHITE);
+	case DRM_FORMAT_C2:
+		return st7571_set_color_mode(st7571, ST7571_COLOR_MODE_GRAY);
+	default:
+		return -EINVAL;
+	}

These should be DRM_FORMAT_R1 and DRM_FORMAT_R2 and not C{1,2}. The former is for displays have a single color (i.e: grey) while the latter is when a pixel can have different color, whose values are defined by a CLUT table.

Quoted 8 lines

I see.
Does fbdev only works with CLUT formats? I get this error when I switch to DRM_FORMAT_R{1,2}:

[drm] Initialized st7571 1.0.0 for 0-003f on minor 0
st7571 0-003f: [drm] format C1 little-endian (0x20203143) not supported
st7571 0-003f: [drm] No compatible format found
st7571 0-003f: [drm] *ERROR* fbdev: Failed to setup emulation (ret=-22)

For testing purposes, you can add the _R formats to the switch case at

https://elixir.bootlin.com/linux/v6.13.7/source/drivers/gpu/drm/drm_fb_helper.c#L1246

and see how it goes.

Best regards
Thomas

Hi,

Quoted 29 lines · Thomas Zimmermann

Hi

Am 08.04.25 um 15:20 schrieb Marcus Folkesson:
[...]

Quoted 15 lines

Quoted 6 lines

Quoted 6 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +6 -0

+static int st7571_set_pixel_format(struct st7571_device *st7571,
+				   u32 pixel_format)
+{
+	switch (pixel_format) {
+	case DRM_FORMAT_C1:
+		return st7571_set_color_mode(st7571, ST7571_COLOR_MODE_BLACKWHITE);

Quoted 9 lines

drivers/gpu/drm/tiny/st7571-i2c.c line 240

Quoted 5 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +5 -0

+	case DRM_FORMAT_C2:
+		return st7571_set_color_mode(st7571, ST7571_COLOR_MODE_GRAY);
+	default:
+		return -EINVAL;
+	}

These should be DRM_FORMAT_R1 and DRM_FORMAT_R2 and not C{1,2}. The former is for displays have a single color (i.e: grey) while the latter is when a pixel can have different color, whose values are defined by a CLUT table.

Quoted 8 lines

I see.
Does fbdev only works with CLUT formats? I get this error when I switch to DRM_FORMAT_R{1,2}:

[drm] Initialized st7571 1.0.0 for 0-003f on minor 0
st7571 0-003f: [drm] format C1 little-endian (0x20203143) not supported
st7571 0-003f: [drm] No compatible format found
st7571 0-003f: [drm] *ERROR* fbdev: Failed to setup emulation (ret=-22)

For testing purposes, you can add the _R formats to the switch case at

Quoted 3 lines

https://elixir.bootlin.com/linux/v6.13.7/source/drivers/gpu/drm/drm_fb_helper.c#L1246

and see how it goes.

Still no penguin (same error as above).

The problem is that drm_mode_legacy_fb_format(), which is called from drm_fbdev_shmem_driver_fbdev_probe -> drm_driver_legacy_fb_format -> drm_mode_legacy_fb_format

Sets the pixel format DRM_FORMAT_C{1,2} when bpp is 1 or 2. So I don't think it is possible to use the _R formats with fbdev. But I'm not sure?

Quoted 2 lines

Best regards
Thomas

Best regards,
Marcus Folkesson

Hello Javier,

Thank you for your review and suggestions.

Quoted 21 lines · Javier Martinez Canillas

Marcus Folkesson <[email protected]> writes:

Hello Marcus,

Quoted 4 lines

Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller. The controller has a SPI, I2C and 8bit parallel interface, this driver is for the I2C interface only.

I would structure the driver differently. For example, what was done for the Solomon SSD130X display controllers, that also support these three interfaces:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/solomon

Basically, it was split in a ssd130x.c module that's agnostic of the transport interface and implements all the core logic for the driver.

And a set of different modules that have the interface specific bits:
ssd130x-i2c.c and ssd130x-spi.c.

Quoted 3 lines

That way, adding for example SPI support to your driver would be quite trivial and won't require any refactoring. Specially since you already are using regmap, which abstracts away the I2C interface bits.

Yes, I had in mind to start looking into this after the initial version. The driver is writtin in this in mind, everything that is common for all interfaces is easy to move out.

Quoted 7 lines

Quoted 7 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +7 -0

+static int st7571_set_pixel_format(struct st7571_device *st7571,
+				   u32 pixel_format)
+{
+	switch (pixel_format) {
+	case DRM_FORMAT_C1:
+		return st7571_set_color_mode(st7571, ST7571_COLOR_MODE_BLACKWHITE);
+	case DRM_FORMAT_C2:

Quoted 9 lines

drivers/gpu/drm/tiny/st7571-i2c.c line 240

Quoted 4 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +4 -0

+		return st7571_set_color_mode(st7571, ST7571_COLOR_MODE_GRAY);
+	default:
+		return -EINVAL;
+	}

These should be DRM_FORMAT_R1 and DRM_FORMAT_R2 and not C{1,2}. The former is for displays have a single color (i.e: grey) while the latter is when a pixel can have different color, whose values are defined by a CLUT table.

I see.
Does fbdev only works with CLUT formats? I get this error when I switch to DRM_FORMAT_R{1,2}:

[drm] Initialized st7571 1.0.0 for 0-003f on minor 0
st7571 0-003f: [drm] format C1 little-endian (0x20203143) not supported
st7571 0-003f: [drm] No compatible format found
st7571 0-003f: [drm] *ERROR* fbdev: Failed to setup emulation (ret=-22)

Quoted 12 lines

...

drivers/gpu/drm/tiny/st7571-i2c.c line 258

Quoted 6 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +6 -0

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

This were discussed in v2, but there were limitations in the helper functions that we currently have.

I will look into how this could be implemented in a generic way, but maybe that is something for a follow up patch?

Quoted 8 lines

drivers/gpu/drm/tiny/st7571-i2c.c line 691

Quoted 6 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +6 -0

+
+static void st7571_remove(struct i2c_client *client)
+{
+	struct st7571_device *st7571 = i2c_get_clientdata(client);
+
+	drm_dev_unplug(&st7571->dev);

I think you are missing a drm_atomic_helper_shutdown() here.

This is a change for v3. As the device has been unplugged already, it won't do anything, so I removed it.

Isn't it right to do so?

Quoted 17 lines

Quoted 7 lines

Quoted 7 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +7 -0

+static int st7571_set_pixel_format(struct st7571_device *st7571,
+				   u32 pixel_format)
+{
+	switch (pixel_format) {
+	case DRM_FORMAT_C1:
+		return st7571_set_color_mode(st7571, ST7571_COLOR_MODE_BLACKWHITE);
+	case DRM_FORMAT_C2:

Quoted 9 lines

drivers/gpu/drm/tiny/st7571-i2c.c line 240

Quoted 4 lines

drivers/gpu/drm/tiny/st7571-i2c.c new file +4 -0

+		return st7571_set_color_mode(st7571, ST7571_COLOR_MODE_GRAY);
+	default:
+		return -EINVAL;
+	}

These should be DRM_FORMAT_R1 and DRM_FORMAT_R2 and not C{1,2}. The former is for displays have a single color (i.e: grey) while the latter is when a pixel can have different color, whose values are defined by a CLUT table.

Quoted 10 lines

I see.
Does fbdev only works with CLUT formats? I get this error when I switch to DRM_FORMAT_R{1,2}:

[drm] Initialized st7571 1.0.0 for 0-003f on minor 0
st7571 0-003f: [drm] format C1 little-endian (0x20203143) not supported
st7571 0-003f: [drm] No compatible format found
st7571 0-003f: [drm] *ERROR* fbdev: Failed to setup emulation (ret=-22)

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.

Signature · 6 lines

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Quoted 7 lines

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.

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?

Marcus Folkesson <[email protected]> writes:

Hello Marcus,

[...]

Quoted 8 lines

Quoted 7 lines

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.

Quoted 5 lines

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

Quoted 5 lines

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

Signature · 5 lines

Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Hello Javier,

On Wed, Apr 09, 2025 at 11:43:54AM +0200, Javier Martinez Canillas wrote:

Quoted 6 lines

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

Quoted 7 lines

Signature · 5 lines

Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Best regards,
Marcus Folkesson

Quoted 1 line · Marcus Folkesson

On Wed, Apr 09, 2025 at 11:43:54AM +0200, Javier Martinez Canillas wrote:

[...]

Quoted 9 lines

Quoted 6 lines

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.

Quoted 5 lines

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.

Yeah, the XRGB8888 version is a boring indeed comparing with the C1 one...

The drm_fb_xrgb8888_to_mono() helper is very naive and just uses a very naive midpoint thresholding to choose if the pixel should be 1 or 0. So there is a lot of information lost there unfortunately.

But that's what I did for ssd130x, to at least have a working driver. Then support for R1 (for ssd130x family) and R4 (for the ssd132x family) could be added as follow-ups. I never did it but Geert has some patches for this.

Quoted 4 lines

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

Marcus Folkesson <[email protected]> writes:

Hello Marcus,

[...]

Quoted 8 lines

Quoted 7 lines

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.

Quoted 5 lines

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

Quoted 5 lines

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,
	},
};

Signature · 6 lines

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Quoted 5 lines

static const struct of_device_id st7571_of_match[] = {
	/* monochrome displays */
	{
		.compatible = "sinocrystal,sc128128012-v01",
		.data = monochrome_formats,

Quoted 8 lines

	},
...
        /* 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.

Marcus Folkesson <[email protected]> writes:

Hello Marcus,

[...]

Quoted 19 lines

Quoted 5 lines

static const struct of_device_id st7571_of_match[] = {
	/* monochrome displays */
	{
		.compatible = "sinocrystal,sc128128012-v01",
		.data = monochrome_formats,

Quoted 8 lines

	},
...
        /* 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:

Quoted 6 lines

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.

Signature · 6 lines

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Quoted 22 lines

Quoted 6 lines

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:

Quoted 6 lines

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.

Quoted 2 lines

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.

Marcus Folkesson <[email protected]> writes:

Hello Marcus,

Quoted 4 lines

Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller. The controller has a SPI, I2C and 8bit parallel interface, this driver is for the I2C interface only.

I would structure the driver differently. For example, what was done for the Solomon SSD130X display controllers, that also support these three interfaces:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/solomon

Basically, it was split in a ssd130x.c module that's agnostic of the transport interface and implements all the core logic for the driver.

And a set of different modules that have the interface specific bits:
ssd130x-i2c.c and ssd130x-spi.c.

That way, adding for example SPI support to your driver would be quite trivial and won't require any refactoring. Specially since you already are using regmap, which abstracts away the I2C interface bits.

Quoted 6 lines

Reviewed-by Thomas Zimmermann <[email protected]>

Signed-off-by Marcus Folkesson <[email protected]>

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

Hello Javier,

Quoted 40 lines · Javier Martinez Canillas

Marcus Folkesson <[email protected]> writes:

Hello Marcus,

Quoted 4 lines

Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller. The controller has a SPI, I2C and 8bit parallel interface, this driver is for the I2C interface only.

I would structure the driver differently. For example, what was done for the Solomon SSD130X display controllers, that also support these three interfaces:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/solomon

Basically, it was split in a ssd130x.c module that's agnostic of the transport interface and implements all the core logic for the driver.

And a set of different modules that have the interface specific bits:
ssd130x-i2c.c and ssd130x-spi.c.

That way, adding for example SPI support to your driver would be quite trivial and won't require any refactoring. Specially since you already are using regmap, which abstracts away the I2C interface bits.

Quoted 6 lines

Reviewed-by Thomas Zimmermann <[email protected]>

Signed-off-by Marcus Folkesson <[email protected]>

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

Quoted 3 lines

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.

I'm looking into moving all the (tiny) Sitronix drivers into their own

subdirectory.
When doing that, should I replace the TINYDRM part with DRM for those drivers? E.g. CONFIG_TINYDRM_ST7735R -> CONFIG_DRM_ST7735R.

Or do we want to keep the config name intact?

Best regards,
Marcus Folkesson