[PATCH v3 0/3] Add support for Sitronix ST7571 LCD controller

[PATCH v3 0/3] Add support for Sitronix ST7571 LCD controller

[Marcus Folkesson]

This series add support for the ST7571 LCD Controller.
It is a 4 gray scale dot matrix LCD controller that supports several
interfaces such as SPI, I2C and a 8bit parallell port.

This driver only supports the I2C interface, but all common parts could
easily be put into a common file to be used with other interfaces.
I only have I2C to test with.

The device is a little defiant, it tends to NAK some commands, but all
commands takes effect, hence the I2C_M_IGNORE_NAK flag.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
Changes in v3:
- (dt-binding) Use 'Controller' rather than 'Panel' in texts
- (dt-binding) Constrain the reg property
- (dt-binding) Remove panel-timing description
- (dt-binding) Change description
- Mostly cosmetic changes in the driver code
- Don't call drm_atomic_helper_shutdown() in remove()
- Link to v2: https://lore.kernel.org/r/20250404-st7571-v2-0-4c78aab9cd5a@gmail.com

Changes in v2:
- Reworked pretty much the whole driver to not use obsolete code.
- Use panel and timing bindings to specify resolution and panel size
- Link to v1: https://lore.kernel.org/r/20250402-st7571-v1-0-351d6b9eeb4a@gmail.com

---
Marcus Folkesson (3):
      dt-bindings: display: Add Sitronix ST7571 LCD Controller
      drm/st7571-i2c: add support for Sitronix ST7571 LCD controller
      MAINTAINERS: add entry for Sitronix ST7571 LCD Controller

 .../bindings/display/sitronix,st7571.yaml          |  70 ++
 MAINTAINERS                                        |   6 +
 drivers/gpu/drm/tiny/Kconfig                       |  11 +
 drivers/gpu/drm/tiny/Makefile                      |   1 +
 drivers/gpu/drm/tiny/st7571-i2c.c                  | 721 +++++++++++++++++++++
 5 files changed, 809 insertions(+)
---
base-commit: 1e26c5e28ca5821a824e90dd359556f5e9e7b89f
change-id: 20250401-st7571-9382b9cfc03f

Best regards,
-- 
Marcus Folkesson <marcus.folkesson@gmail.com>

[PATCH v3 1/3] dt-bindings: display: Add Sitronix ST7571 LCD Controller

[Marcus Folkesson]

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

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 .../bindings/display/sitronix,st7571.yaml          | 70 ++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/sitronix,st7571.yaml b/Documentation/devicetree/bindings/display/sitronix,st7571.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..a25d2820be27fe580b6d1aaddd9c610383b73b3a
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/sitronix,st7571.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/sitronix,st7571.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sitronix ST7571 Display Controller
+
+maintainers:
+  - Marcus Folkesson <marcus.folkesson@gmail.com>
+
+description:
+  Sitronix ST7571 is a driver and controller for up to 4-level gray
+  scale dot matrix LCD panels.
+  The controller is designed for use with LCD panels that have
+  a resolution of up to 128x128 pixels.
+
+allOf:
+  - $ref: panel/panel-common.yaml#
+
+properties:
+  compatible:
+    const: sitronix,st7571
+
+  reg:
+    maxItems: 1
+
+  reset-gpios: true
+  width-mm: true
+  height-mm: true
+  panel-timing: true
+
+required:
+  - compatible
+  - reg
+  - reset-gpios
+  - width-mm
+  - height-mm
+  - panel-timing
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      display@3f {
+        compatible = "sitronix,st7571";
+        reg = <0x3f>;
+        reset-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>;
+        width-mm = <37>;
+        height-mm = <27>;
+
+        panel-timing {
+          hactive = <128>;
+          vactive = <96>;
+          hback-porch = <0>;
+          vback-porch = <0>;
+          clock-frequency = <0>;
+          hfront-porch = <0>;
+          hsync-len = <0>;
+          vfront-porch = <0>;
+          vsync-len = <0>;
+        };
+      };
+    };

-- 
2.49.0

Re: [PATCH v3 1/3] dt-bindings: display: Add Sitronix ST7571 LCD Controller

[Conor Dooley]

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

On Tue, Apr 08, 2025 at 09:04:28AM +0200, Marcus Folkesson wrote:
> Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

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

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

[Marcus Folkesson]

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 <tzimmrmann@suse.de>
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 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(+)

diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index 94cbdb1337c07f1628a33599a7130369b9d59d98..33a69aea4232c5ca7a04b1fe18bb424e0fded697 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -232,6 +232,17 @@ config TINYDRM_ST7586
 
 	  If M is selected the module will be called st7586.
 
+config DRM_ST7571_I2C
+	tristate "DRM support for Sitronix ST7571 display panels (I2C)"
+	depends on DRM && I2C
+	select DRM_GEM_SHMEM_HELPER
+	select DRM_KMS_HELPER
+	select REGMAP_I2C
+	help
+	  DRM driver for Sitronix ST7571 panels controlled over I2C.
+
+	  if M is selected the module will be called st7571-i2c.
+
 config TINYDRM_ST7735R
 	tristate "DRM support for Sitronix ST7715R/ST7735R display panels"
 	depends on DRM && SPI
diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
index 60816d2eb4ff93b87228ed8eadd60a0a33a1144b..eab7568c92c880cfdf7c2f0b9c4bfac4685dbe95 100644
--- a/drivers/gpu/drm/tiny/Makefile
+++ b/drivers/gpu/drm/tiny/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_DRM_GM12U320)		+= gm12u320.o
 obj-$(CONFIG_DRM_OFDRM)			+= ofdrm.o
 obj-$(CONFIG_DRM_PANEL_MIPI_DBI)	+= panel-mipi-dbi.o
 obj-$(CONFIG_DRM_SIMPLEDRM)		+= simpledrm.o
+obj-$(CONFIG_DRM_ST7571_I2C)		+= st7571-i2c.o
 obj-$(CONFIG_TINYDRM_HX8357D)		+= hx8357d.o
 obj-$(CONFIG_TINYDRM_ILI9163)		+= ili9163.o
 obj-$(CONFIG_TINYDRM_ILI9225)		+= ili9225.o
diff --git a/drivers/gpu/drm/tiny/st7571-i2c.c b/drivers/gpu/drm/tiny/st7571-i2c.c
new file mode 100644
index 0000000000000000000000000000000000000000..9c54ac02f712b8c44ef21878edbf9569a6992915
--- /dev/null
+++ b/drivers/gpu/drm/tiny/st7571-i2c.c
@@ -0,0 +1,721 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Driver for Sitronix ST7571, a 4 level gray scale dot matrix LCD controller
+ *
+ * Copyright (C) 2025 Marcus Folkesson <marcus.folkesson@gmail.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include <drm/clients/drm_client_setup.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_encoder.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_fbdev_shmem.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_module.h>
+#include <drm/drm_plane.h>
+#include <drm/drm_probe_helper.h>
+
+#include <video/display_timing.h>
+#include <video/of_display_timing.h>
+
+#define ST7571_COMMAND_MODE			(0x00)
+#define ST7571_DATA_MODE			(0x40)
+
+/* Normal mode command set */
+#define ST7571_DISPLAY_OFF			(0xae)
+#define ST7571_DISPLAY_ON			(0xaf)
+#define ST7571_OSC_ON				(0xab)
+#define ST7571_SET_COLUMN_LSB(c)		(0x00 | ((c) & 0xf))
+#define ST7571_SET_COLUMN_MSB(c)		(0x10 | ((c) >> 4))
+#define ST7571_SET_COM0_LSB(x)			((x) & 0x7f)
+#define ST7571_SET_COM0_MSB			(0x44)
+#define ST7571_SET_COM_SCAN_DIR(d)		(0xc0 | (((d) << 3) & 0x8))
+#define ST7571_SET_CONTRAST_LSB(c)		((c) & 0x3f)
+#define ST7571_SET_CONTRAST_MSB			(0x81)
+#define ST7571_SET_DISPLAY_DUTY_LSB(d)		((d) & 0xff)
+#define ST7571_SET_DISPLAY_DUTY_MSB		(0x48)
+#define ST7571_SET_ENTIRE_DISPLAY_ON(p)		(0xa4 | ((p) & 0x1))
+#define ST7571_SET_LCD_BIAS(b)			(0x50 | ((b) & 0x7))
+#define ST7571_SET_MODE_LSB(m)			((m) & 0xfc)
+#define ST7571_SET_MODE_MSB			(0x38)
+#define ST7571_SET_PAGE(p)			(0xb0 | (p))
+#define ST7571_SET_POWER(p)			(0x28 | ((p) & 0x7))
+#define ST7571_SET_REGULATOR_REG(r)		(0x20 | ((r) & 0x7))
+#define ST7571_SET_REVERSE(r)			(0xa6 | ((r) & 0x1))
+#define ST7571_SET_SEG_SCAN_DIR(d)		(0xa0 | ((d) & 0x1))
+#define ST7571_SET_START_LINE_LSB(l)		((l) & 0x3f)
+#define ST7571_SET_START_LINE_MSB		(0x40)
+
+/* Extension command set 3 */
+#define ST7571_COMMAND_SET_3			(0x7b)
+#define ST7571_SET_COLOR_MODE(c)		(0x10 | ((c) & 0x1))
+#define ST7571_COMMAND_SET_NORMAL		(0x00)
+
+
+/* hactive is fixed to 128 */
+#define ST7571_HACTIVE				(128)
+
+#define DRIVER_NAME "st7571"
+#define DRIVER_DESC "ST7571 DRM driver"
+#define DRIVER_MAJOR 1
+#define DRIVER_MINOR 0
+
+enum st7571_color_mode {
+	ST7571_COLOR_MODE_GRAY = 0,
+	ST7571_COLOR_MODE_BLACKWHITE = 1,
+};
+
+#define drm_to_st7571(_dev) container_of(_dev, struct st7571_device, dev)
+
+struct st7571_device {
+	struct drm_device dev;
+
+	struct drm_plane primary_plane;
+	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;
+
+	struct gpio_desc *reset;
+
+	u32 width_mm;
+	u32 height_mm;
+	u32 nlines;
+	u32 startline;
+};
+
+static int st7571_regmap_write(void *context, const void *data, size_t count)
+{
+	struct i2c_client *client = context;
+	struct st7571_device *st7571 = i2c_get_clientdata(client);
+	int ret;
+
+	struct i2c_msg msg = {
+		.addr = st7571->client->addr,
+		.flags = st7571->ignore_nak ? I2C_M_IGNORE_NAK : 0,
+		.len = count,
+		.buf = (u8 *)data
+	};
+
+	ret = i2c_transfer(st7571->client->adapter, &msg, 1);
+	/*
+	 * This is a workaround for the ST7571, which sometimes fails to acknowledge
+	 *
+	 * Unfortunately, there is no way to check if the transfer failed because of
+	 * a NAK or something else as I2C bus drivers use different return values for NAK.
+	 *
+	 * However, if the transfer fails and ignore_nak is set, we know it is an error.
+	 */
+	if (ret < 0 && st7571->ignore_nak)
+		return ret;
+
+	return 0;
+}
+
+static int st7571_regmap_read(void *context, const void *reg_buf,
+			       size_t reg_size, void *val_buf, size_t val_size)
+{
+	return -EOPNOTSUPP;
+}
+
+static int st7571_send_command_list(struct st7571_device *st7571,
+				    const u8 *cmd_list, size_t len)
+{
+	int ret;
+
+	for (int i = 0; i < len; i++) {
+		ret = regmap_write(st7571->regmap, ST7571_COMMAND_MODE, cmd_list[i]);
+		if (ret < 0)
+			return ret;
+	}
+
+	return ret;
+}
+
+static inline u8 st7571_transform_xy(const char *p, int x, int y)
+{
+	int xrest = x % 8;
+	u8 result = 0;
+
+	/*
+	 * Transforms an (x, y) pixel coordinate into a vertical 8-bit
+	 * column from the framebuffer. It calculates the corresponding byte in the
+	 * framebuffer, extracts the bit at the given x position across 8 consecutive
+	 * rows, and packs those bits into a single byte.
+	 *
+	 * Return an 8-bit value representing a vertical column of pixels.
+	 */
+	x = x / 8;
+	y = (y / 8) * 8;
+
+	for (int i = 0; i < 8; i++) {
+		int row_idx = y + i;
+		u8 byte = p[row_idx * 16 + x];
+		u8 bit = (byte >> xrest) & 1;
+
+		result |= (bit << i);
+	}
+
+	return result;
+}
+
+static int st7571_set_position(struct st7571_device *st7571, int x, int y)
+{
+	u8 cmd_list[] = {
+		ST7571_SET_COLUMN_LSB(x),
+		ST7571_SET_COLUMN_MSB(x),
+		ST7571_SET_PAGE(y / 8),
+	};
+
+	return st7571_send_command_list(st7571, cmd_list, ARRAY_SIZE(cmd_list));
+}
+
+static int st7571_fb_blit_rect(struct drm_framebuffer *fb,
+			       const struct iosys_map *vmap,
+			       struct drm_rect *rect)
+{
+	struct st7571_device *st7571 = drm_to_st7571(fb->dev);
+	int bpp = fb->format->format == DRM_FORMAT_C1 ? 1 : 2;
+	char *pixel = vmap->vaddr;
+	int x1 = rect->x1 * bpp;
+	int x2 = rect->x2 * bpp;
+	char row[256];
+
+	for (int y = rect->y1; y < rect->y2; y += 8) {
+		for (int x = x1; x < x2; x++)
+			row[x] = st7571_transform_xy(pixel, x, y);
+
+		st7571_set_position(st7571, rect->x1, y);
+
+		/* TODO: Investige why we can't write multiple bytes at once */
+		for (int x = x1; x < x2; x++)
+			regmap_bulk_write(st7571->regmap, ST7571_DATA_MODE, row + x, 1);
+	}
+
+	return 0;
+}
+
+static int st7571_set_color_mode(struct st7571_device *st7571, enum st7571_color_mode  mode)
+{
+	u8 cmd_list[] = {
+		ST7571_COMMAND_SET_3,
+		ST7571_SET_COLOR_MODE(mode),
+		ST7571_COMMAND_SET_NORMAL,
+	};
+
+	return st7571_send_command_list(st7571, cmd_list, ARRAY_SIZE(cmd_list));
+}
+
+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;
+	}
+}
+
+static int st7571_connector_get_modes(struct drm_connector *conn)
+{
+	struct st7571_device *st7571 = drm_to_st7571(conn->dev);
+
+	return drm_connector_helper_get_modes_fixed(conn, &st7571->mode);
+}
+
+static const struct drm_connector_helper_funcs st7571_connector_helper_funcs = {
+	.get_modes = st7571_connector_get_modes,
+};
+
+static const uint32_t st7571_primary_plane_formats[] = {
+	DRM_FORMAT_C1,
+	DRM_FORMAT_C2,
+};
+
+static const uint64_t st7571_primary_plane_fmtmods[] = {
+	DRM_FORMAT_MOD_LINEAR,
+	DRM_FORMAT_MOD_INVALID
+};
+
+static int st7571_primary_plane_helper_atomic_check(struct drm_plane *plane,
+						 struct drm_atomic_state *state)
+{
+	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_crtc *new_crtc = new_plane_state->crtc;
+	struct drm_crtc_state *new_crtc_state = NULL;
+
+	if (new_crtc)
+		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
+
+	return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
+						   DRM_PLANE_NO_SCALING,
+						   DRM_PLANE_NO_SCALING,
+						   false, false);
+}
+
+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);
+	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))
+		return;
+
+	ret = st7571_set_pixel_format(st7571, fb->format->format);
+	if (ret) {
+		dev_err(dev->dev, "Failed to set pixel format: %d\n", ret);
+		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);
+	}
+
+	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,
+};
+
+static const struct drm_plane_funcs st7571_primary_plane_funcs = {
+	.update_plane = drm_atomic_helper_update_plane,
+	.disable_plane = drm_atomic_helper_disable_plane,
+	.destroy = drm_plane_cleanup,
+	DRM_GEM_SHADOW_PLANE_FUNCS,
+};
+
+/*
+ * CRTC
+ */
+
+static const struct drm_crtc_helper_funcs st7571_crtc_helper_funcs = {
+	.atomic_check = drm_crtc_helper_atomic_check,
+};
+
+static const struct drm_crtc_funcs st7571_crtc_funcs = {
+	.reset = drm_atomic_helper_crtc_reset,
+	.destroy = drm_crtc_cleanup,
+	.set_config = drm_atomic_helper_set_config,
+	.page_flip = drm_atomic_helper_page_flip,
+	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+};
+
+/*
+ * Encoder
+ */
+
+static const struct drm_encoder_funcs st7571_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
+/*
+ * Connector
+ */
+
+static const struct drm_connector_funcs st7571_connector_funcs = {
+	.reset = drm_atomic_helper_connector_reset,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+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);
+}
+
+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,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+static struct drm_display_mode st7571_mode(struct st7571_device *st7571)
+{
+	struct drm_display_mode mode = {
+		DRM_SIMPLE_MODE(ST7571_HACTIVE, st7571->nlines,
+				st7571->width_mm, st7571->height_mm),
+	};
+
+	return mode;
+}
+
+static int st7571_mode_config_init(struct st7571_device *st7571)
+{
+	struct drm_device *dev = &st7571->dev;
+	int ret;
+
+	ret = drmm_mode_config_init(dev);
+	if (ret)
+		return ret;
+
+	dev->mode_config.min_width = ST7571_HACTIVE;
+	dev->mode_config.min_height = 1;
+	dev->mode_config.max_width = ST7571_HACTIVE;
+	dev->mode_config.max_height = 128;
+	dev->mode_config.preferred_depth = 1;
+	dev->mode_config.funcs = &st7571_mode_config_funcs;
+
+	return 0;
+}
+
+static int st7571_plane_init(struct st7571_device *st7571)
+{
+	struct drm_plane *primary_plane = &st7571->primary_plane;
+	struct drm_device *dev = &st7571->dev;
+	int ret;
+
+	ret = drm_universal_plane_init(dev, primary_plane, 1,
+				       &st7571_primary_plane_funcs,
+				       st7571_primary_plane_formats,
+				       ARRAY_SIZE(st7571_primary_plane_formats),
+				       st7571_primary_plane_fmtmods,
+				       DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret)
+		return ret;
+
+	drm_plane_helper_add(primary_plane, &st7571_primary_plane_helper_funcs);
+	drm_plane_enable_fb_damage_clips(primary_plane);
+
+	return 0;
+}
+
+static int st7571_crtc_init(struct st7571_device *st7571)
+{
+	struct drm_plane *primary_plane = &st7571->primary_plane;
+	struct drm_crtc *crtc = &st7571->crtc;
+	struct drm_device *dev = &st7571->dev;
+	int ret;
+
+	ret = drm_crtc_init_with_planes(dev, crtc, primary_plane, NULL,
+					&st7571_crtc_funcs, NULL);
+	if (ret)
+		return ret;
+
+	drm_crtc_helper_add(crtc, &st7571_crtc_helper_funcs);
+
+	return 0;
+}
+
+static int st7571_encoder_init(struct st7571_device *st7571)
+{
+	struct drm_encoder *encoder = &st7571->encoder;
+	struct drm_crtc *crtc = &st7571->crtc;
+	struct drm_device *dev = &st7571->dev;
+	int ret;
+
+	ret = drm_encoder_init(dev, encoder, &st7571_encoder_funcs, DRM_MODE_ENCODER_NONE, NULL);
+	if (ret)
+		return ret;
+
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+
+	return 0;
+}
+
+static int st7571_connector_init(struct st7571_device *st7571)
+{
+	struct drm_connector *connector = &st7571->connector;
+	struct drm_encoder *encoder = &st7571->encoder;
+	struct drm_device *dev = &st7571->dev;
+	int ret;
+
+	ret = drm_connector_init(dev, connector, &st7571_connector_funcs,
+				 DRM_MODE_CONNECTOR_Unknown);
+	if (ret)
+		return ret;
+
+	drm_connector_helper_add(connector, &st7571_connector_helper_funcs);
+
+	return drm_connector_attach_encoder(connector, encoder);
+}
+
+DEFINE_DRM_GEM_FOPS(st7571_fops);
+
+static const struct drm_driver st7571_driver = {
+	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
+
+	.name		 = DRIVER_NAME,
+	.desc		 = DRIVER_DESC,
+	.major		 = DRIVER_MAJOR,
+	.minor		 = DRIVER_MINOR,
+
+	.fops		 = &st7571_fops,
+	DRM_GEM_SHMEM_DRIVER_OPS,
+	DRM_FBDEV_SHMEM_DRIVER_OPS,
+};
+
+static const struct regmap_bus st7571_regmap_bus = {
+	.read = st7571_regmap_read,
+	.write = st7571_regmap_write,
+};
+
+static const struct regmap_config st7571_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.use_single_write = true,
+};
+
+static int st7571_parse_dt(struct st7571_device *st7571)
+{
+	struct device *dev = &st7571->client->dev;
+	struct device_node *np = dev->of_node;
+	struct display_timing dt;
+	int ret;
+
+	ret = of_get_display_timing(np, "panel-timing", &dt);
+	if (ret) {
+		dev_err(dev, "Failed to get display timing from DT\n");
+		return ret;
+	}
+
+	of_property_read_u32(np, "width-mm", &st7571->width_mm);
+	of_property_read_u32(np, "height-mm", &st7571->height_mm);
+
+	st7571->startline = dt.vfront_porch.typ;
+	st7571->nlines = dt.vactive.typ;
+
+	if (dt.hactive.typ != ST7571_HACTIVE) {
+		dev_err(dev, "Invalid timing configuration (hactive != %i).\n",
+			ST7571_HACTIVE);
+		return -EINVAL;
+	}
+
+	if (st7571->width_mm == 0 || st7571->height_mm == 0) {
+		dev_err(dev, "Invalid panel dimensions.\n");
+		return -EINVAL;
+	}
+
+	if (st7571->startline + st7571->nlines > 128) {
+		dev_err(dev, "Invalid timing configuration.\n");
+		return -EINVAL;
+	}
+
+	st7571->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(st7571->reset))
+		return PTR_ERR(st7571->reset);
+
+	return 0;
+}
+
+static void st7571_reset(struct st7571_device *st7571)
+{
+	gpiod_set_value_cansleep(st7571->reset, 1);
+	udelay(20);
+	gpiod_set_value_cansleep(st7571->reset, 0);
+}
+
+static int st7571_lcd_init(struct st7571_device *st7571)
+{
+	/*
+	 * Most of the initialization sequence is taken directly from the
+	 * referential initial code in the ST7571 datasheet.
+	 */
+	u8 commands[] = {
+		ST7571_DISPLAY_OFF,
+		ST7571_SET_MODE_MSB,
+
+		ST7571_SET_MODE_LSB(0x94),
+		ST7571_SET_SEG_SCAN_DIR(0),
+		ST7571_SET_COM_SCAN_DIR(1),
+
+		ST7571_SET_COM0_MSB,
+		ST7571_SET_COM0_LSB(0x00),
+
+		ST7571_SET_START_LINE_MSB,
+		ST7571_SET_START_LINE_LSB(st7571->startline),
+
+		ST7571_OSC_ON,
+		ST7571_SET_REGULATOR_REG(5),
+		ST7571_SET_CONTRAST_MSB,
+		ST7571_SET_CONTRAST_LSB(0x33),
+		ST7571_SET_LCD_BIAS(0x04),
+		ST7571_SET_DISPLAY_DUTY_MSB,
+		ST7571_SET_DISPLAY_DUTY_LSB(st7571->nlines),
+
+
+		ST7571_SET_POWER(0x4),	/* Power Control, VC: ON, VR: OFF, VF: OFF */
+		ST7571_SET_POWER(0x6),	/* Power Control, VC: ON, VR: ON, VF: OFF */
+		ST7571_SET_POWER(0x7),	/* Power Control, VC: ON, VR: ON, VF: ON */
+
+		ST7571_SET_REVERSE(0),
+		ST7571_SET_ENTIRE_DISPLAY_ON(0),
+
+		ST7571_DISPLAY_ON,
+	};
+
+	/* Perform a reset before initializing the controller */
+	st7571_reset(st7571);
+
+	return st7571_send_command_list(st7571, commands, ARRAY_SIZE(commands));
+}
+
+
+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;
+
+	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");
+		return PTR_ERR(st7571->regmap);
+	}
+
+	ret = st7571_lcd_init(st7571);
+	if (ret)
+		return ret;
+
+	ret = st7571_mode_config_init(st7571);
+	if (ret) {
+		dev_err(&client->dev, "Failed to initialize mode config\n");
+		return ret;
+	}
+
+	ret = st7571_plane_init(st7571);
+	if (ret) {
+		dev_err(dev->dev, "Failed to initialize primary plane\n");
+		return ret;
+	}
+
+	ret = st7571_crtc_init(st7571);
+	if (ret < 0) {
+		dev_err(dev->dev, "Failed to initialize CRTC\n");
+		return ret;
+	}
+
+	ret = st7571_encoder_init(st7571);
+	if (ret < 0) {
+		dev_err(dev->dev, "Failed to initialize encoder\n");
+		return ret;
+	}
+
+	ret = st7571_connector_init(st7571);
+	if (ret < 0) {
+		dev_err(dev->dev, "Failed to initialize connector\n");
+		return ret;
+	}
+
+	drm_mode_config_reset(dev);
+
+	ret = drm_dev_register(dev, 0);
+	if (ret) {
+		dev_err(dev->dev, "Failed to register DRM device\n");
+		return ret;
+	}
+
+	drm_client_setup(dev, NULL);
+	return 0;
+}
+
+static void st7571_remove(struct i2c_client *client)
+{
+	struct st7571_device *st7571 = i2c_get_clientdata(client);
+
+	drm_dev_unplug(&st7571->dev);
+}
+
+static const struct of_device_id st7571_of_match[] = {
+	{ .compatible = "sitronix,st7571" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, st7571_of_match);
+
+
+static const struct i2c_device_id st7571_id[] = {
+	{ "st7571", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, st7571_id);
+
+static struct i2c_driver st7571_i2c_driver = {
+	.driver = {
+		.name = "st7571",
+		.of_match_table = st7571_of_match,
+	},
+	.probe = st7571_probe,
+	.remove = st7571_remove,
+	.id_table = st7571_id,
+};
+
+module_i2c_driver(st7571_i2c_driver);
+
+MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@gmail.com>");
+MODULE_DESCRIPTION("DRM Driver for Sitronix ST7571 LCD controller");
+MODULE_LICENSE("GPL");

-- 
2.49.0

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

[Javier Martinez Canillas]

Marcus Folkesson <marcus.folkesson@gmail.com> writes:

Hello Marcus,

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

> Reviewed-by: Thomas Zimmermann <tzimmrmann@suse.de>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
>  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.

>  3 files changed, 733 insertions(+)
>
> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
> index 94cbdb1337c07f1628a33599a7130369b9d59d98..33a69aea4232c5ca7a04b1fe18bb424e0fded697 100644
> --- a/drivers/gpu/drm/tiny/Kconfig
> +++ b/drivers/gpu/drm/tiny/Kconfig
> @@ -232,6 +232,17 @@ config TINYDRM_ST7586
>  
>  	  If M is selected the module will be called st7586.
>  
> +config DRM_ST7571_I2C
> +	tristate "DRM support for Sitronix ST7571 display panels (I2C)"
> +	depends on DRM && I2C
> +	select DRM_GEM_SHMEM_HELPER

DRM_GEM_SHMEM_HELPER depends on MMU and your driver is selecting it,
so it should also depend on MMU, i.e: depends on DRM && MMU && I2C.



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

> +struct st7571_device {
> +	struct drm_device dev;
> +
> +	struct drm_plane primary_plane;
> +	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."

...

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

...

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

...

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

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

> +		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);
> +	}
> +
> +	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 ?


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

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

...

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

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

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

...

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

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Thomas Zimmermann]

Hi,

lots of good points in the review.

Am 08.04.25 um 12:44 schrieb Javier Martinez Canillas:
[...]
>> Reviewed-by: Thomas Zimmermann <tzimmrmann@suse.de>
>> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
>> ---
>>   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.

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.

>
>>   3 files changed, 733 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
>> index 94cbdb1337c07f1628a33599a7130369b9d59d98..33a69aea4232c5ca7a04b1fe18bb424e0fded697 100644
>> --- a/drivers/gpu/drm/tiny/Kconfig
>> +++ b/drivers/gpu/drm/tiny/Kconfig
>> @@ -232,6 +232,17 @@ config TINYDRM_ST7586
>>   
[...]
>> +
>> +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

>
> ...
>
>> +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);
>> +	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.
>
>> +		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().
>
>> +		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);
>> +	}
>> +
>> +	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 ?
>
>
>> +
>> +/*
>> + * 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.
>
>> +/*
>> + * 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.
>
> ...
>
>> +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.
>
>> +
>> +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.
>
>> +	.atomic_check = drm_atomic_helper_check,
>> +	.atomic_commit = drm_atomic_helper_commit,
>> +};
>> +
> ...
>
>> +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;
>> +
>> +	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.
>
>> +
>> +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.
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi,
>
> lots of good points in the review.
>
> Am 08.04.25 um 12:44 schrieb Javier Martinez Canillas:
> [...]
>>> Reviewed-by: Thomas Zimmermann <tzimmrmann@suse.de>
>>> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
>>> ---
>>>   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.
>
> 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.
>

Yeah, I don't know. In any case, the driver / module name is not an ABI so
we can always move around the files later if needed.

>>
>>>   3 files changed, 733 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
>>> index 94cbdb1337c07f1628a33599a7130369b9d59d98..33a69aea4232c5ca7a04b1fe18bb424e0fded697 100644
>>> --- a/drivers/gpu/drm/tiny/Kconfig
>>> +++ b/drivers/gpu/drm/tiny/Kconfig
>>> @@ -232,6 +232,17 @@ config TINYDRM_ST7586
>>>   
> [...]
>>> +
>>> +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
>

Agreed. But even in its current form that helper is what I had in mind and
what is used by the ssd130x driver too for XRGB8888 -> R1 conversion. There
is no drm_fb_xrgb8888_to_gray2(), but that could be added as a part of this
driver series.

> Best regards
> Thomas
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Marcus Folkesson]

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

Hello Javier,

Thank you for your review and suggestions.

On Tue, Apr 08, 2025 at 12:44:46PM +0200, Javier Martinez Canillas wrote:
> Marcus Folkesson <marcus.folkesson@gmail.com> writes:
> 
> Hello Marcus,
> 
> > 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.

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.

> 
> > Reviewed-by: Thomas Zimmermann <tzimmrmann@suse.de>
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > ---
> >  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.

That sounds like a good idea.

[...]

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

I agree, I will change this to a static inline function.

> 
> > +struct st7571_device {
> > +	struct drm_device dev;
> > +
> > +	struct drm_plane primary_plane;
> > +	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."

This has completely flown under the radar, thank you for pointing it out.

I will put the text from the datasheet together with ignore_nak.

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


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


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

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

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

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

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

> 
> > +	.atomic_check = drm_atomic_helper_check,
> > +	.atomic_commit = drm_atomic_helper_commit,
> > +};
> > +
> 
> ...
> 
> > +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;
> > +
> > +	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.

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


> 
> And also a struct i2c_driver .shutdown callback to call to
> drm_atomic_helper_shutdown() as well.
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
> 

Best regards,
Marcus Folkesson

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

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

[Thomas Zimmermann]

Hi

Am 08.04.25 um 15:20 schrieb Marcus Folkesson:
[...]
>>
>>> +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)

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

>
>
>> ...
>>
>>> +
>>> +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?
>
>
> [...]
>>> +
>>> +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.
>
>>
>>> +
>>> +/*
>>> + * 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.
>
>>> +/*
>>> + * 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.
>
>> ...
>>
>>> +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.
>
>>> +
>>> +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.
>
>>> +	.atomic_check = drm_atomic_helper_check,
>>> +	.atomic_commit = drm_atomic_helper_commit,
>>> +};
>>> +
>> ...
>>
>>> +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;
>>> +
>>> +	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.
>
>>> +
>>> +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?
>
>
>> And also a struct i2c_driver .shutdown callback to call to
>> drm_atomic_helper_shutdown() as well.
>>
>> -- 
>> Best regards,
>>
>> Javier Martinez Canillas
>> Core Platforms
>> Red Hat
>>
> Best regards,
> Marcus Folkesson

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

[Marcus Folkesson]

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

Hi,

On Tue, Apr 08, 2025 at 03:57:22PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 08.04.25 um 15:20 schrieb Marcus Folkesson:
> [...]
> > > 
> > > > +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)
> 
> 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.

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?

> 
> Best regards
> Thomas

Best regards,
Marcus Folkesson

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

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

[Thomas Zimmermann]

Hi

Am 08.04.25 um 16:58 schrieb Marcus Folkesson:
> Hi,
>
> On Tue, Apr 08, 2025 at 03:57:22PM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 08.04.25 um 15:20 schrieb Marcus Folkesson:
>> [...]
>>>>> +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)
>> 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.
> 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

In addition to the other change, you could change the _C formats to _R 
formats in that helper for testing.

Best regards
Thomas

>
> 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?
>
>> Best regards
>> Thomas
> Best regards,
> Marcus Folkesson

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

[Javier Martinez Canillas]

Marcus Folkesson <marcus.folkesson@gmail.com> writes:

> Hello Javier,
>
> Thank you for your review and suggestions.
>
> On Tue, Apr 08, 2025 at 12:44:46PM +0200, Javier Martinez Canillas wrote:
>> Marcus Folkesson <marcus.folkesson@gmail.com> writes:
>> 
>> Hello Marcus,
>> 
>> > 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.
>
> 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.
>

Yes, I noticed that and the reason why I mentioned the file layout used in
the ssd130x driver. If was meant to only be an I2C driver then I think it
would be a good candidate for the tiny sub-dir (that is for small drivers
that can be implemented in a single file).

But as said, it's OK for me too if you start in tiny and then refactor it
to be moved to a sitronix vendor sub-dir.

[...]

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

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.

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

Indeed, will need a drm_fb_xrgb8888_to_gray2() for R2. There is already a
drm_fb_xrgb8888_to_mono() as mentioned that you can use for R1. 

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

Yes, it could be a follow-up patch. It just helps to have XRGB8888 support for
compatibility reasons (the fbdev issue you found is another example of this).

[...]

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

It seems I was wrong on this and your implementation is correct. I talked
with Thomas yesterday and he clarified it to me.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Marcus Folkesson]

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

Hello Javier,

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

[...]
> >> > +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)
> >
> >
> 
> 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?

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

Best regards,
Marcus Folkesson

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

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

[Javier Martinez Canillas]

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

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

[Marcus Folkesson]

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

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

[Javier Martinez Canillas]

Marcus Folkesson <marcus.folkesson@gmail.com> writes:

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

[...]

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

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.

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

Nice pictures!

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Geert Uytterhoeven]

On Wed, 9 Apr 2025 at 16:16, Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Marcus Folkesson <marcus.folkesson@gmail.com> writes:
> > On Wed, Apr 09, 2025 at 11:43:54AM +0200, Javier Martinez Canillas wrote:
>
> [...]
>
> >>
> >> 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.
>
> Yeah, the XRGB8888 version is a boring indeed comparing with the C1 one...

To see the nice monochrome penguin logo, fb-helper needs to gain
support for R1 first...

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

In theory, it could use Floyd–Steinberg dithering, like I did for modetest.
However, given the display receives partial updates from damage clips,
it will look ugly, and may flicker.

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

[1] needs an update, and still more rework to make them acceptable.

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

Yep, the latter is using the correct logo.

[1]  "[PATCH v2 0/8] drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1"
    https://lore.kernel.org/all/cover.1692888745.git.geert@linux-m68k.org/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Marcus Folkesson]

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

Hello,

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

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Javier Martinez Canillas]

Marcus Folkesson <marcus.folkesson@gmail.com> 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.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Marcus Folkesson]

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

On Fri, Apr 11, 2025 at 10:26:47AM +0200, Javier Martinez Canillas wrote:
> Marcus Folkesson <marcus.folkesson@gmail.com> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Javier Martinez Canillas]

Marcus Folkesson <marcus.folkesson@gmail.com> writes:

Hello Marcus,

[...]

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

Thanks for confirming, that was my understanding too.

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

That's great! Exactly what I had in mind.

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

Yes, but there are designs with carrier boards that support I2C. For
example, I have  [1] and [2]. The former comes with an I2C interface
and uses the ST7567S IC variant, while the latter comes with a 4-wire
SPI interface and uses a ST7567P IC variant.

But don't worry about it. Since I've these displays and your driver now
allows for different IC families after adding the mentioned indirection
layer, it should be very trivial for me to add support for these on top.

> I've split up the driver though, which will be right after this series.
>

Nice, thanks again.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Marcus Folkesson]

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

Hello Javier,

On Tue, Apr 08, 2025 at 12:44:46PM +0200, Javier Martinez Canillas wrote:
> Marcus Folkesson <marcus.folkesson@gmail.com> writes:
> 
> Hello Marcus,
> 
> > 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.
> 
> > Reviewed-by: Thomas Zimmermann <tzimmrmann@suse.de>
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > ---
> >  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.

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






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

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

[Javier Martinez Canillas]

Marcus Folkesson <marcus.folkesson@gmail.com> writes:

> Hello Javier,
>
> On Tue, Apr 08, 2025 at 12:44:46PM +0200, Javier Martinez Canillas wrote:
>> Marcus Folkesson <marcus.folkesson@gmail.com> writes:
>> 
>> Hello Marcus,
>> 
>> > 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.
>> 
>> > Reviewed-by: Thomas Zimmermann <tzimmrmann@suse.de>
>> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
>> > ---
>> >  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.
>
> I'm looking into moving all the (tiny) Sitronix drivers into their own

Great!

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

I would drop the TINY prefix. That's also why I thought that your new
driver not having TINY in its Kconfig symbol name was the correct call. 

> Or do we want to keep the config name intact?
>
> Best regards,
> Marcus Folkesson

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH v3 3/3] MAINTAINERS: add entry for Sitronix ST7571 LCD Controller

[Marcus Folkesson]

Add MAINTAINERS entry for the Sitronix ST7571 dot matrix LCD
controller.

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 889bd4a59551c9bc125f94944a6e1c7e3ef2de83..eeae24fda846b9f63400ebb08c3fa7f02f3f4b19 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7572,6 +7572,12 @@ T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
 F:	Documentation/devicetree/bindings/display/sitronix,st7586.txt
 F:	drivers/gpu/drm/tiny/st7586.c
 
+DRM DRIVER FOR SITRONIX ST7571 PANELS
+M:	Marcus Folkesson <marcus.folkesson@gmail.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/display/sitronix,st7571.yaml
+F:	drivers/gpu/drm/tiny/st7571-i2c.c
+
 DRM DRIVER FOR SITRONIX ST7701 PANELS
 M:	Jagan Teki <jagan@amarulasolutions.com>
 S:	Maintained

-- 
2.49.0