Add a new DRM/KMS driver for displays using the Sitronix ST7920
controller connected via the SPI bus. This provides a standard
framebuffer interface for these common monochrome LCDs.

Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
---
 drivers/gpu/drm/sitronix/Kconfig  |  10 +
 drivers/gpu/drm/sitronix/Makefile |   1 +
 drivers/gpu/drm/sitronix/st7920.c | 905 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 916 insertions(+)

diff --git a/drivers/gpu/drm/sitronix/Kconfig b/drivers/gpu/drm/sitronix/Kconfig
index 6de7d92d9b74c72746915b945869dba91f161d2b..f14115350069849b668d75188f6d504d56fa3dda 100644
--- a/drivers/gpu/drm/sitronix/Kconfig
+++ b/drivers/gpu/drm/sitronix/Kconfig
@@ -40,3 +40,13 @@ config DRM_ST7735R
 
 	  If M is selected the module will be called st7735r.
 
+config DRM_ST7920
+	tristate "DRM support for Sitronix ST7920 LCD displays"
+	depends on DRM && SPI && MMU
+	select DRM_GEM_SHMEM_HELPER
+	select DRM_KMS_HELPER
+	select REGMAP_SPI
+	help
+	  DRM driver for the ST7920 Sitronix LCD controllers.
+
+	  If M is selected the module will be called st7920.
diff --git a/drivers/gpu/drm/sitronix/Makefile b/drivers/gpu/drm/sitronix/Makefile
index bd139e5a6995fa026cc635b3c29782473d1efad7..2f064a518121bfee3cca73acd42589e8c54cd4d7 100644
--- a/drivers/gpu/drm/sitronix/Makefile
+++ b/drivers/gpu/drm/sitronix/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_DRM_ST7571_I2C)		+= st7571-i2c.o
 obj-$(CONFIG_DRM_ST7586)		+= st7586.o
 obj-$(CONFIG_DRM_ST7735R)		+= st7735r.o
+obj-$(CONFIG_DRM_ST7920))		+= st7920.o
diff --git a/drivers/gpu/drm/sitronix/st7920.c b/drivers/gpu/drm/sitronix/st7920.c
new file mode 100644
index 0000000000000000000000000000000000000000..15b6f99e64313cbeba474231c4ef8649e8e50f9d
--- /dev/null
+++ b/drivers/gpu/drm/sitronix/st7920.c
@@ -0,0 +1,905 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * DRM driver for Sitronix ST7920 LCD displays
+ *
+ * Copyright 2025 Iker Pedrosa <ikerpedrosam@gmail.com>
+ *
+ */
+
+#include <linux/bitrev.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_client_setup.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fbdev_shmem.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_plane.h>
+#include <drm/drm_probe_helper.h>
+
+#define DRIVER_NAME	"sitronix_st7920"
+#define DRIVER_DESC	"DRM driver for Sitronix ST7920 LCD displays"
+#define DRIVER_DATE	"20250723"
+#define DRIVER_MAJOR	1
+#define DRIVER_MINOR	0
+
+/* Display organization */
+#define DRM_PITCH		16
+#define DRM_SCANLINES		64
+#define BYTES_IN_DISPLAY	(DRM_PITCH * DRM_SCANLINES)
+#define BYTES_IN_SEGMENT	2
+#define PIXELS_PER_SEGMENT	(BYTES_IN_SEGMENT * 8)
+
+/* Sync sequence */
+#define SYNC_BITS			0xF8
+#define RW_HIGH				0x04
+#define RS_HIGH				0x02
+
+/* Commands */
+#define SET_DISPLAY_ON			0x0C
+#define SET_DISPLAY_OFF			0x08
+#define SET_DISPLAY_CLEAR		0x01
+#define SET_BASIC_INSTRUCTION_SET	0x30
+#define SET_EXT_INSTRUCTION_SET		0x34
+#define SET_GRAPHICS_DISPLAY		0x36
+#define SET_GDRAM_ADDRESS		0x80
+#define SET_GDRAM_DATA			0xFF /* Driver internal command */
+
+/* Masks */
+#define HIGH_DATA_MASK			0xF0
+#define LOW_DATA_MASK			0x0F
+#define TOP_VERTICAL_ADDRESS		0x80
+#define BOTTOM_VERTICAL_ADDRESS		0x60
+#define TOP_HORIZONTAL_ADDRESS		0x00
+#define BOTTOM_HORIZONTAL_ADDRESS	0x80
+
+#define CMD_SIZE			35
+
+enum st7920_variants {
+	/* st7920 family */
+	ST7920_ID
+};
+
+struct spi7920_error {
+	int errno;
+};
+
+struct st7920_deviceinfo {
+	u32 default_dclk_div;
+	u32 default_dclk_frq;
+	u32 default_width;
+	u32 default_height;
+};
+
+struct st7920_device {
+	struct drm_device drm;
+	struct device *dev;
+	struct drm_display_mode mode;
+	struct drm_plane primary_plane;
+	struct drm_crtc crtc;
+	struct drm_encoder encoder;
+	struct drm_connector connector;
+	struct spi_device *spi;
+
+	struct regmap *regmap;
+
+	const struct st7920_deviceinfo *device_info;
+
+	struct gpio_desc *reset_gpio;
+
+	u32 height;
+	u32 width;
+};
+
+const struct st7920_deviceinfo st7920_variants[] = {
+	[ST7920_ID] = {
+		.default_width = 128,
+		.default_height = 64,
+	}
+};
+EXPORT_SYMBOL_NS_GPL(st7920_variants, DRM_ST7920);
+
+struct st7920_plane_state {
+	struct drm_shadow_plane_state base;
+	/* Intermediate buffer to convert pixels from XRGB8888 to HW format */
+	u8 *buffer;
+};
+
+struct st7920_crtc_state {
+	struct drm_crtc_state base;
+	/* Buffer to store pixels in HW format and written to the panel */
+	u8 *data_array;
+};
+
+static inline struct st7920_plane_state *to_st7920_plane_state(struct drm_plane_state *state)
+{
+	return container_of(state, struct st7920_plane_state, base.base);
+}
+
+static inline struct st7920_crtc_state *to_st7920_crtc_state(struct drm_crtc_state *state)
+{
+	return container_of(state, struct st7920_crtc_state, base);
+}
+
+static inline struct st7920_device *drm_to_st7920(struct drm_device *drm)
+{
+	return container_of(drm, struct st7920_device, drm);
+}
+
+static int st7920_store_gdram_address(const void *data, u8 *reg)
+{
+	const u8 y_addr = *(const u8 *)data;
+	bool bottom_screen = (y_addr >= 32);
+	int i = 0;
+
+	reg[i++] = SYNC_BITS;
+	/* Set vertical address */
+	if (!bottom_screen)
+		reg[i++] = TOP_VERTICAL_ADDRESS + (*(uint8_t *)data & HIGH_DATA_MASK);
+	else
+		reg[i++] = BOTTOM_VERTICAL_ADDRESS + (*(uint8_t *)data & HIGH_DATA_MASK);
+
+	reg[i++] = *(uint8_t *)data << 4;
+	/* Set horizontal address */
+	reg[i++] = SET_GDRAM_ADDRESS;
+	if (!bottom_screen)
+		reg[i++] = TOP_HORIZONTAL_ADDRESS;
+	else
+		reg[i++] = BOTTOM_HORIZONTAL_ADDRESS;
+
+	return i;
+}
+
+static int st7920_store_gdram_data(const void *data, u8 *reg)
+{
+	const u8 *line_data = data;
+	int i = 0, j = 0;
+
+	reg[i++] = SYNC_BITS | RS_HIGH;
+
+	for (j = 0; j < 16; j++) {
+		reg[i++] = line_data[j] & 0xF0;
+		reg[i++] = (line_data[j] << 4) & 0xF0;
+	}
+
+	return i;
+}
+
+static int st7920_store_others(int cmd, const void *data, u8 *reg)
+{
+	int i = 0;
+
+	reg[i++] = SYNC_BITS;
+	reg[i++] = cmd & HIGH_DATA_MASK;
+	reg[i++] = (cmd & LOW_DATA_MASK) << 4;
+
+	return i;
+}
+
+static void st7920_spi_write(struct spi_device *spi, int cmd, const void *data,
+			     int delay_us, struct spi7920_error *err)
+{
+	u8 reg[CMD_SIZE] = {0};
+	int size = 0;
+	int ret;
+
+	if (err->errno)
+		return;
+
+	/*
+	 * First the sync bits are sent: 11111WS0.
+	 * Where W is the read/write (RW) bit and S is the register/data (RS) bit.
+	 * Then, every 8 bits instruction/data will be separated into 2 groups.
+	 * Higher 4 bits (DB7~DB4) will be placed in the first section followed by
+	 * 4 '0's. And lower 4 bits (DB3~DB0) will be placed in the second section
+	 * followed by 4 '0's.
+	 */
+	if (cmd == SET_GDRAM_ADDRESS)
+		size = st7920_store_gdram_address(data, reg);
+	else if (cmd == SET_GDRAM_DATA)
+		size = st7920_store_gdram_data(data, reg);
+	else
+		size = st7920_store_others(cmd, data, reg);
+
+	ret = spi_write(spi, reg, size);
+	if (ret) {
+		err->errno = ret;
+		return;
+	}
+
+	if (delay_us)
+		udelay(delay_us);
+}
+
+static const struct regmap_config st7920_spi_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static const struct of_device_id st7920_of_match[] = {
+	/* st7920 family */
+	{
+		.compatible = "sitronix,st7920",
+		.data = &st7920_variants[ST7920_ID],
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, st7920_of_match);
+
+/*
+ * The SPI core always reports a MODALIAS uevent of the form "spi:<dev>", even
+ * if the device was registered via OF. This means that the module will not be
+ * auto loaded, unless it contains an alias that matches the MODALIAS reported.
+ *
+ * To workaround this issue, add a SPI device ID table. Even when this should
+ * not be needed for this driver to match the registered SPI devices.
+ */
+static const struct spi_device_id st7920_spi_id[] = {
+	/* st7920 family */
+	{ "st7920",  ST7920_ID },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(spi, st7920_spi_id);
+
+static void st7920_power_on(struct st7920_device *st7920,
+			    struct spi7920_error *err)
+{
+	st7920_spi_write(st7920->spi, SET_DISPLAY_ON, NULL, 72, err);
+}
+
+static void st7920_power_off(struct st7920_device *st7920,
+			     struct spi7920_error *err)
+{
+	st7920_spi_write(st7920->spi, SET_DISPLAY_CLEAR, NULL, 1600, err);
+	st7920_spi_write(st7920->spi, SET_DISPLAY_OFF, NULL, 72, err);
+}
+
+static void st7920_hw_reset(struct st7920_device *st7920)
+{
+	if (!st7920->reset_gpio)
+		return;
+
+	gpiod_set_value_cansleep(st7920->reset_gpio, 0);
+	usleep_range(15, 20);
+	gpiod_set_value_cansleep(st7920->reset_gpio, 1);
+	msleep(40);
+}
+
+static int st7920_init(struct st7920_device *st7920)
+{
+	struct spi7920_error err = {0};
+
+	st7920_spi_write(st7920->spi, SET_BASIC_INSTRUCTION_SET, NULL, 72, &err);
+	st7920_power_on(st7920, &err);
+	st7920_spi_write(st7920->spi, SET_GRAPHICS_DISPLAY, NULL, 72, &err);
+	st7920_spi_write(st7920->spi, SET_DISPLAY_CLEAR, NULL, 1600, &err);
+
+	return err.errno;
+}
+
+static int st7920_update_rect(struct st7920_device *st7920,
+			       struct drm_rect *rect, u8 *buf,
+			       u8 *data_array)
+{
+	struct spi7920_error err;
+	u32 array_idx = 0;
+	int i, j;
+
+	/*
+	 * The screen is divided in 64(Y)x8(X) segments and each segment is
+	 * further divided in 2 bytes (D15~D0).
+	 *
+	 * Segment 0x0 is in the top-right corner, while segment 63x15 is in the
+	 * bottom-left. They would be displayed in the screen in the following way:
+	 * 0x0  0x1  0x2  ... 0x15
+	 * 1x0  1x1  1x2  ... 1x15
+	 * ...
+	 * 63x0 63x1 63x2 ... 63x15
+	 *
+	 * The data in each byte is big endian.
+	 */
+
+	for (i = 0; i < DRM_SCANLINES; i++) {
+		u8 *line_start = buf + (i * DRM_PITCH);
+		u8 line_buffer[DRM_PITCH];
+
+		for (j = 0; j < DRM_PITCH; j++) {
+			line_buffer[j] = bitrev8(line_start[j]);
+			data_array[array_idx++] = line_buffer[j];
+		}
+
+		st7920_spi_write(st7920->spi, SET_GDRAM_ADDRESS, &i, 72, &err);
+		st7920_spi_write(st7920->spi, SET_GDRAM_DATA, line_buffer, 72, &err);
+	}
+
+	return err.errno;
+}
+
+static void st7920_clear_screen(struct st7920_device *st7920, u8 *data_array)
+{
+	struct spi7920_error err;
+
+	memset(data_array, 0, BYTES_IN_DISPLAY);
+
+	st7920_spi_write(st7920->spi, SET_DISPLAY_CLEAR, NULL, 1600, &err);
+}
+
+static int st7920_fb_blit_rect(struct drm_framebuffer *fb,
+				const struct iosys_map *vmap,
+				struct drm_rect *rect,
+				u8 *buf, u8 *data_array,
+				struct drm_format_conv_state *fmtcnv_state)
+{
+	struct st7920_device *st7920 = drm_to_st7920(fb->dev);
+	struct iosys_map dst;
+	unsigned int dst_pitch;
+	int ret;
+
+	/* Align y to display page boundaries */
+	rect->y1 = round_down(rect->y1, PIXELS_PER_SEGMENT);
+	rect->y2 = min_t(unsigned int, round_up(rect->y2, PIXELS_PER_SEGMENT), st7920->height);
+
+	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
+
+	iosys_map_set_vaddr(&dst, buf);
+	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);
+
+	ret = st7920_update_rect(st7920, rect, buf, data_array);
+
+	return ret;
+}
+
+static int st7920_primary_plane_atomic_check(struct drm_plane *plane,
+					      struct drm_atomic_state *state)
+{
+	struct drm_device *drm = plane->dev;
+	struct st7920_device *st7920 = drm_to_st7920(drm);
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct st7920_plane_state *st7920_state = to_st7920_plane_state(plane_state);
+	struct drm_shadow_plane_state *shadow_plane_state = &st7920_state->base;
+	struct drm_crtc *crtc = plane_state->crtc;
+	struct drm_crtc_state *crtc_state = NULL;
+	const struct drm_format_info *fi;
+	unsigned int pitch;
+	int ret;
+
+	if (crtc)
+		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+	ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
+						  DRM_PLANE_NO_SCALING,
+						  DRM_PLANE_NO_SCALING,
+						  false, false);
+	if (ret)
+		return ret;
+	else if (!plane_state->visible)
+		return 0;
+
+	fi = drm_format_info(DRM_FORMAT_R1);
+	if (!fi)
+		return -EINVAL;
+
+	pitch = drm_format_info_min_pitch(fi, 0, st7920->width);
+
+	if (plane_state->fb->format != fi) {
+		void *buf;
+
+		/* format conversion necessary; reserve buffer */
+		buf = drm_format_conv_state_reserve(&shadow_plane_state->fmtcnv_state,
+						    pitch, GFP_KERNEL);
+		if (!buf)
+			return -ENOMEM;
+	}
+
+	st7920_state->buffer = kcalloc(pitch, st7920->height, GFP_KERNEL);
+	if (!st7920_state->buffer)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static void st7920_primary_plane_atomic_update(struct drm_plane *plane,
+						struct drm_atomic_state *state)
+{
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
+	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
+	struct st7920_crtc_state *st7920_crtc_state =  to_st7920_crtc_state(crtc_state);
+	struct st7920_plane_state *st7920_plane_state = to_st7920_plane_state(plane_state);
+	struct drm_framebuffer *fb = plane_state->fb;
+	struct drm_atomic_helper_damage_iter iter;
+	struct drm_device *drm = plane->dev;
+	struct drm_rect dst_clip;
+	struct drm_rect damage;
+	int idx;
+	int ret;
+
+	if (!drm_dev_enter(drm, &idx))
+		return;
+
+	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE))
+		goto out_drm_dev_exit;
+
+	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
+	drm_atomic_for_each_plane_damage(&iter, &damage) {
+		dst_clip = plane_state->dst;
+
+		if (!drm_rect_intersect(&dst_clip, &damage))
+			continue;
+
+		ret = st7920_fb_blit_rect(fb, &shadow_plane_state->data[0], &dst_clip,
+				st7920_plane_state->buffer,
+				st7920_crtc_state->data_array,
+				&shadow_plane_state->fmtcnv_state);
+		if (ret)
+			drm_err_once(plane->dev, "Failed to write to device: %d.\n", ret);
+	}
+
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+
+out_drm_dev_exit:
+	drm_dev_exit(idx);
+}
+
+static void st7920_primary_plane_atomic_disable(struct drm_plane *plane,
+						 struct drm_atomic_state *state)
+{
+	struct drm_device *drm = plane->dev;
+	struct st7920_device *st7920 = drm_to_st7920(drm);
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_crtc_state *crtc_state;
+	struct st7920_crtc_state *st7920_crtc_state;
+	int idx;
+
+	if (!plane_state->crtc)
+		return;
+
+	crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
+	st7920_crtc_state = to_st7920_crtc_state(crtc_state);
+
+	if (!drm_dev_enter(drm, &idx))
+		return;
+
+	st7920_clear_screen(st7920, st7920_crtc_state->data_array);
+
+	drm_dev_exit(idx);
+}
+
+/* Called during init to allocate the plane's atomic state. */
+static void st7920_primary_plane_reset(struct drm_plane *plane)
+{
+	struct st7920_plane_state *st7920_state;
+
+	drm_WARN_ON_ONCE(plane->dev, plane->state);
+
+	st7920_state = kzalloc(sizeof(*st7920_state), GFP_KERNEL);
+	if (!st7920_state)
+		return;
+
+	__drm_gem_reset_shadow_plane(plane, &st7920_state->base);
+}
+
+static struct drm_plane_state *st7920_primary_plane_duplicate_state(struct drm_plane *plane)
+{
+	struct drm_shadow_plane_state *new_shadow_plane_state;
+	struct st7920_plane_state *old_st7920_state;
+	struct st7920_plane_state *st7920_state;
+
+	if (drm_WARN_ON_ONCE(plane->dev, !plane->state))
+		return NULL;
+
+	old_st7920_state = to_st7920_plane_state(plane->state);
+	st7920_state = kmemdup(old_st7920_state, sizeof(*st7920_state), GFP_KERNEL);
+	if (!st7920_state)
+		return NULL;
+
+	/* The buffer is not duplicated and is allocated in .atomic_check */
+	st7920_state->buffer = NULL;
+
+	new_shadow_plane_state = &st7920_state->base;
+
+	__drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state);
+
+	return &new_shadow_plane_state->base;
+}
+
+static void st7920_primary_plane_destroy_state(struct drm_plane *plane,
+						struct drm_plane_state *state)
+{
+	struct st7920_plane_state *st7920_state = to_st7920_plane_state(state);
+
+	kfree(st7920_state->buffer);
+
+	__drm_gem_destroy_shadow_plane_state(&st7920_state->base);
+
+	kfree(st7920_state);
+}
+
+static const struct drm_plane_helper_funcs st7920_primary_plane_helper_funcs = {
+	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+	.atomic_check = st7920_primary_plane_atomic_check,
+	.atomic_update = st7920_primary_plane_atomic_update,
+	.atomic_disable = st7920_primary_plane_atomic_disable,
+};
+
+static const struct drm_plane_funcs st7920_primary_plane_funcs = {
+	.update_plane = drm_atomic_helper_update_plane,
+	.disable_plane = drm_atomic_helper_disable_plane,
+	.reset = st7920_primary_plane_reset,
+	.atomic_duplicate_state = st7920_primary_plane_duplicate_state,
+	.atomic_destroy_state = st7920_primary_plane_destroy_state,
+	.destroy = drm_plane_cleanup,
+};
+
+static enum drm_mode_status st7920_crtc_mode_valid(struct drm_crtc *crtc,
+						    const struct drm_display_mode *mode)
+{
+	struct st7920_device *st7920 = drm_to_st7920(crtc->dev);
+
+	return drm_crtc_helper_mode_valid_fixed(crtc, mode, &st7920->mode);
+}
+
+static int st7920_crtc_atomic_check(struct drm_crtc *crtc,
+				     struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+	struct st7920_crtc_state *st7920_state = to_st7920_crtc_state(crtc_state);
+	int ret;
+
+	ret = drm_crtc_helper_atomic_check(crtc, state);
+	if (ret)
+		return ret;
+
+	st7920_state->data_array = kmalloc(BYTES_IN_DISPLAY, GFP_KERNEL);
+	if (!st7920_state->data_array)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static void st7920_crtc_atomic_enable(struct drm_crtc *crtc,
+				      struct drm_atomic_state *state)
+{
+	struct drm_device *drm = crtc->dev;
+	struct st7920_device *st7920 = drm_to_st7920(drm);
+	int idx;
+	int ret;
+
+	if (!drm_dev_enter(drm, &idx))
+		return;
+
+	st7920_hw_reset(st7920);
+
+	ret = st7920_init(st7920);
+	if (ret)
+		dev_err(st7920->dev, "Failed to init hardware: %d\n", ret);
+
+	drm_dev_exit(idx);
+}
+
+static void st7920_crtc_atomic_disable(struct drm_crtc *crtc,
+				       struct drm_atomic_state *state)
+{
+	struct spi7920_error err = {0};
+	struct drm_device *drm = crtc->dev;
+	struct st7920_device *st7920 = drm_to_st7920(drm);
+	int idx;
+
+	drm_dev_enter(drm, &idx);
+
+	st7920_power_off(st7920, &err);
+
+	drm_dev_exit(idx);
+}
+
+/* Called during init to allocate the CRTC's atomic state. */
+static void st7920_crtc_reset(struct drm_crtc *crtc)
+{
+	struct st7920_crtc_state *st7920_state;
+
+	drm_WARN_ON_ONCE(crtc->dev, crtc->state);
+
+	st7920_state = kzalloc(sizeof(*st7920_state), GFP_KERNEL);
+	if (!st7920_state)
+		return;
+
+	__drm_atomic_helper_crtc_reset(crtc, &st7920_state->base);
+}
+
+static struct drm_crtc_state *st7920_crtc_duplicate_state(struct drm_crtc *crtc)
+{
+	struct st7920_crtc_state *old_st7920_state;
+	struct st7920_crtc_state *st7920_state;
+
+	if (drm_WARN_ON_ONCE(crtc->dev, !crtc->state))
+		return NULL;
+
+	old_st7920_state = to_st7920_crtc_state(crtc->state);
+	st7920_state = kmemdup(old_st7920_state, sizeof(*st7920_state), GFP_KERNEL);
+	if (!st7920_state)
+		return NULL;
+
+	/* The buffer is not duplicated and is allocated in .atomic_check */
+	st7920_state->data_array = NULL;
+
+	__drm_atomic_helper_crtc_duplicate_state(crtc, &st7920_state->base);
+
+	return &st7920_state->base;
+}
+
+static void st7920_crtc_destroy_state(struct drm_crtc *crtc,
+						struct drm_crtc_state *state)
+{
+	struct st7920_crtc_state *st7920_state = to_st7920_crtc_state(state);
+
+	kfree(st7920_state->data_array);
+
+	__drm_atomic_helper_crtc_destroy_state(state);
+
+	kfree(st7920_state);
+}
+
+/*
+ * The CRTC is always enabled. Screen updates are performed by
+ * the primary plane's atomic_update function. Disabling clears
+ * the screen in the primary plane's atomic_disable function.
+ */
+static const struct drm_crtc_helper_funcs st7920_crtc_helper_funcs = {
+	.mode_valid = st7920_crtc_mode_valid,
+	.atomic_check = st7920_crtc_atomic_check,
+	.atomic_enable = st7920_crtc_atomic_enable,
+	.atomic_disable = st7920_crtc_atomic_disable,
+};
+
+static const struct drm_crtc_funcs st7920_crtc_funcs = {
+	.reset = st7920_crtc_reset,
+	.destroy = drm_crtc_cleanup,
+	.set_config = drm_atomic_helper_set_config,
+	.page_flip = drm_atomic_helper_page_flip,
+	.atomic_duplicate_state = st7920_crtc_duplicate_state,
+	.atomic_destroy_state = st7920_crtc_destroy_state,
+};
+
+static const struct drm_encoder_funcs st7920_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
+static int st7920_connector_get_modes(struct drm_connector *connector)
+{
+	struct st7920_device *st7920 = drm_to_st7920(connector->dev);
+
+	return drm_connector_helper_get_modes_fixed(connector, &st7920->mode);
+}
+
+static const struct drm_connector_helper_funcs st7920_connector_helper_funcs = {
+	.get_modes = st7920_connector_get_modes,
+};
+
+static const struct drm_connector_funcs st7920_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 const struct drm_mode_config_funcs st7920_mode_config_funcs = {
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+static const uint32_t st7920_formats[] = {
+	DRM_FORMAT_XRGB8888,
+};
+
+DEFINE_DRM_GEM_FOPS(st7920_fops);
+
+static const struct drm_driver st7920_drm_driver = {
+	DRM_GEM_SHMEM_DRIVER_OPS,
+	DRM_FBDEV_SHMEM_DRIVER_OPS,
+	.name			= DRIVER_NAME,
+	.desc			= DRIVER_DESC,
+	.date			= DRIVER_DATE,
+	.major			= DRIVER_MAJOR,
+	.minor			= DRIVER_MINOR,
+	.driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
+	.fops			= &st7920_fops,
+};
+
+static int st7920_init_modeset(struct st7920_device *st7920)
+{
+	struct drm_display_mode *mode = &st7920->mode;
+	struct device *dev = st7920->dev;
+	struct drm_device *drm = &st7920->drm;
+	unsigned long max_width, max_height;
+	struct drm_plane *primary_plane;
+	struct drm_crtc *crtc;
+	struct drm_encoder *encoder;
+	struct drm_connector *connector;
+	int ret;
+
+	/*
+	 * Modesetting
+	 */
+
+	ret = drmm_mode_config_init(drm);
+	if (ret) {
+		dev_err(dev, "DRM mode config init failed: %d\n", ret);
+		return ret;
+	}
+
+	mode->type = DRM_MODE_TYPE_DRIVER;
+	mode->clock = 1;
+	mode->hdisplay = st7920->device_info->default_width;
+	mode->htotal = st7920->device_info->default_width;
+	mode->hsync_start = st7920->device_info->default_width;
+	mode->hsync_end = st7920->device_info->default_width;
+	mode->vdisplay = st7920->device_info->default_height;
+	mode->vtotal = st7920->device_info->default_height;
+	mode->vsync_start = st7920->device_info->default_height;
+	mode->vsync_end = st7920->device_info->default_height;
+	mode->width_mm = 27;
+	mode->height_mm = 27;
+
+	max_width = max_t(unsigned long, mode->hdisplay, DRM_SHADOW_PLANE_MAX_WIDTH);
+	max_height = max_t(unsigned long, mode->vdisplay, DRM_SHADOW_PLANE_MAX_HEIGHT);
+
+	drm->mode_config.min_width = mode->hdisplay;
+	drm->mode_config.max_width = max_width;
+	drm->mode_config.min_height = mode->vdisplay;
+	drm->mode_config.max_height = max_height;
+	drm->mode_config.preferred_depth = 24;
+	drm->mode_config.funcs = &st7920_mode_config_funcs;
+
+	/* Primary plane */
+
+	primary_plane = &st7920->primary_plane;
+	ret = drm_universal_plane_init(drm, primary_plane, 0, &st7920_primary_plane_funcs,
+				    st7920_formats, ARRAY_SIZE(st7920_formats),
+				    NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret) {
+		dev_err(dev, "DRM primary plane init failed: %d\n", ret);
+		return ret;
+	}
+
+	drm_plane_helper_add(primary_plane, &st7920_primary_plane_helper_funcs);
+
+	drm_plane_enable_fb_damage_clips(primary_plane);
+
+	/* CRTC */
+
+	crtc = &st7920->crtc;
+	ret = drm_crtc_init_with_planes(drm, crtc, primary_plane, NULL,
+					&st7920_crtc_funcs, NULL);
+	if (ret) {
+		dev_err(dev, "DRM crtc init failed: %d\n", ret);
+		return ret;
+	}
+
+	drm_crtc_helper_add(crtc, &st7920_crtc_helper_funcs);
+
+	/* Encoder */
+
+	encoder = &st7920->encoder;
+	ret = drm_encoder_init(drm, encoder, &st7920_encoder_funcs,
+			       DRM_MODE_ENCODER_NONE, NULL);
+	if (ret) {
+		dev_err(dev, "DRM encoder init failed: %d\n", ret);
+		return ret;
+	}
+
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+
+	/* Connector */
+
+	connector = &st7920->connector;
+	ret = drm_connector_init(drm, connector, &st7920_connector_funcs,
+				 DRM_MODE_CONNECTOR_Unknown);
+	if (ret) {
+		dev_err(dev, "DRM connector init failed: %d\n", ret);
+		return ret;
+	}
+
+	drm_connector_helper_add(connector, &st7920_connector_helper_funcs);
+
+	ret = drm_connector_attach_encoder(connector, encoder);
+	if (ret) {
+		dev_err(dev, "DRM attach connector to encoder failed: %d\n", ret);
+		return ret;
+	}
+
+	drm_mode_config_reset(drm);
+
+	return 0;
+}
+
+static int st7920_probe(struct spi_device *spi)
+{
+	struct st7920_device *st7920;
+	struct regmap *regmap;
+	struct device *dev = &spi->dev;
+	struct drm_device *drm;
+	int ret;
+
+	regmap = devm_regmap_init_spi(spi, &st7920_spi_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	st7920 = devm_drm_dev_alloc(dev, &st7920_drm_driver,
+				    struct st7920_device, drm);
+	if (IS_ERR(st7920))
+		return PTR_ERR(st7920);
+
+	drm = &st7920->drm;
+
+	st7920->dev = dev;
+	st7920->regmap = regmap;
+	st7920->spi = spi;
+	st7920->device_info = device_get_match_data(dev);
+	st7920->width = st7920->device_info->default_width;
+	st7920->height = st7920->device_info->default_height;
+
+	st7920->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(st7920->reset_gpio)) {
+		ret = PTR_ERR(st7920->reset_gpio);
+		dev_err(dev, "Unable to retrieve reset GPIO: %d\n", ret);
+		return ret;
+	}
+
+	spi_set_drvdata(spi, st7920);
+
+	ret = st7920_init_modeset(st7920);
+	if (ret)
+		return ret;
+
+	ret = drm_dev_register(drm, 0);
+	if (ret)
+		return dev_err_probe(dev, ret, "DRM device register failed\n");
+
+	drm_client_setup(drm, NULL);
+
+	return 0;
+}
+
+static void st7920_remove(struct spi_device *spi)
+{
+	struct st7920_device *st7920 = spi_get_drvdata(spi);
+
+	drm_dev_unplug(&st7920->drm);
+	drm_atomic_helper_shutdown(&st7920->drm);
+}
+
+static void st7920_shutdown(struct spi_device *spi)
+{
+	struct st7920_device *st7920 = spi_get_drvdata(spi);
+
+	drm_atomic_helper_shutdown(&st7920->drm);
+}
+
+static struct spi_driver st7920_spi_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = st7920_of_match,
+	},
+	.id_table = st7920_spi_id,
+	.probe = st7920_probe,
+	.remove = st7920_remove,
+	.shutdown = st7920_shutdown,
+};
+module_spi_driver(st7920_spi_driver);
+
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_AUTHOR("Iker Pedrosa <ipedrosam@gmail.com>");
+MODULE_LICENSE("GPL v2");

-- 
2.51.0

Hi,

please see my review below. The overall driver looks good. There are 
some points that need to be discussed.

Am 31.10.25 um 10:05 schrieb Iker Pedrosa:
> Add a new DRM/KMS driver for displays using the Sitronix ST7920
> controller connected via the SPI bus. This provides a standard
> framebuffer interface for these common monochrome LCDs.
>
> Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
> ---
>   drivers/gpu/drm/sitronix/Kconfig  |  10 +
>   drivers/gpu/drm/sitronix/Makefile |   1 +
>   drivers/gpu/drm/sitronix/st7920.c | 905 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 916 insertions(+)
>
> diff --git a/drivers/gpu/drm/sitronix/Kconfig b/drivers/gpu/drm/sitronix/Kconfig
> index 6de7d92d9b74c72746915b945869dba91f161d2b..f14115350069849b668d75188f6d504d56fa3dda 100644
> --- a/drivers/gpu/drm/sitronix/Kconfig
> +++ b/drivers/gpu/drm/sitronix/Kconfig
> @@ -40,3 +40,13 @@ config DRM_ST7735R
>   
>   	  If M is selected the module will be called st7735r.
>   
> +config DRM_ST7920
> +	tristate "DRM support for Sitronix ST7920 LCD displays"
> +	depends on DRM && SPI && MMU
> +	select DRM_GEM_SHMEM_HELPER
> +	select DRM_KMS_HELPER
> +	select REGMAP_SPI
> +	help
> +	  DRM driver for the ST7920 Sitronix LCD controllers.
> +
> +	  If M is selected the module will be called st7920.
> diff --git a/drivers/gpu/drm/sitronix/Makefile b/drivers/gpu/drm/sitronix/Makefile
> index bd139e5a6995fa026cc635b3c29782473d1efad7..2f064a518121bfee3cca73acd42589e8c54cd4d7 100644
> --- a/drivers/gpu/drm/sitronix/Makefile
> +++ b/drivers/gpu/drm/sitronix/Makefile
> @@ -1,3 +1,4 @@
>   obj-$(CONFIG_DRM_ST7571_I2C)		+= st7571-i2c.o
>   obj-$(CONFIG_DRM_ST7586)		+= st7586.o
>   obj-$(CONFIG_DRM_ST7735R)		+= st7735r.o
> +obj-$(CONFIG_DRM_ST7920))		+= st7920.o
> diff --git a/drivers/gpu/drm/sitronix/st7920.c b/drivers/gpu/drm/sitronix/st7920.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..15b6f99e64313cbeba474231c4ef8649e8e50f9d
> --- /dev/null
> +++ b/drivers/gpu/drm/sitronix/st7920.c
> @@ -0,0 +1,905 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * DRM driver for Sitronix ST7920 LCD displays
> + *
> + * Copyright 2025 Iker Pedrosa <ikerpedrosam@gmail.com>
> + *
> + */
> +
> +#include <linux/bitrev.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_client_setup.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_damage_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fbdev_shmem.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_plane.h>
> +#include <drm/drm_probe_helper.h>
> +
> +#define DRIVER_NAME	"sitronix_st7920"
> +#define DRIVER_DESC	"DRM driver for Sitronix ST7920 LCD displays"
> +#define DRIVER_DATE	"20250723"
> +#define DRIVER_MAJOR	1
> +#define DRIVER_MINOR	0
> +
> +/* Display organization */
> +#define DRM_PITCH		16
> +#define DRM_SCANLINES		64

The DRM_ prefix is only for DRM. You cannot use it. Prefixing everything 
with ST7920_ would make sense.

> +#define BYTES_IN_DISPLAY	(DRM_PITCH * DRM_SCANLINES)
> +#define BYTES_IN_SEGMENT	2
> +#define PIXELS_PER_SEGMENT	(BYTES_IN_SEGMENT * 8)
> +
> +/* Sync sequence */
> +#define SYNC_BITS			0xF8
> +#define RW_HIGH				0x04
> +#define RS_HIGH				0x02
> +
> +/* Commands */
> +#define SET_DISPLAY_ON			0x0C
> +#define SET_DISPLAY_OFF			0x08
> +#define SET_DISPLAY_CLEAR		0x01
> +#define SET_BASIC_INSTRUCTION_SET	0x30
> +#define SET_EXT_INSTRUCTION_SET		0x34
> +#define SET_GRAPHICS_DISPLAY		0x36
> +#define SET_GDRAM_ADDRESS		0x80
> +#define SET_GDRAM_DATA			0xFF /* Driver internal command */
> +
> +/* Masks */
> +#define HIGH_DATA_MASK			0xF0
> +#define LOW_DATA_MASK			0x0F
> +#define TOP_VERTICAL_ADDRESS		0x80
> +#define BOTTOM_VERTICAL_ADDRESS		0x60
> +#define TOP_HORIZONTAL_ADDRESS		0x00
> +#define BOTTOM_HORIZONTAL_ADDRESS	0x80
> +
> +#define CMD_SIZE			35
> +
> +enum st7920_variants {
> +	/* st7920 family */
> +	ST7920_ID
> +};
> +
> +struct spi7920_error {
> +	int errno;
> +};
> +
> +struct st7920_deviceinfo {
> +	u32 default_dclk_div;
> +	u32 default_dclk_frq;
> +	u32 default_width;
> +	u32 default_height;
> +};
> +
> +struct st7920_device {
> +	struct drm_device drm;

> +	struct device *dev;

Please remove this field. You can use drm.dev instead.

> +	struct drm_display_mode mode;
> +	struct drm_plane primary_plane;
> +	struct drm_crtc crtc;
> +	struct drm_encoder encoder;
> +	struct drm_connector connector;
> +	struct spi_device *spi;
> +
> +	struct regmap *regmap;
> +
> +	const struct st7920_deviceinfo *device_info;
> +
> +	struct gpio_desc *reset_gpio;
> +
> +	u32 height;
> +	u32 width;
> +};
> +
> +const struct st7920_deviceinfo st7920_variants[] = {
> +	[ST7920_ID] = {
> +		.default_width = 128,
> +		.default_height = 64,
> +	}
> +};
> +EXPORT_SYMBOL_NS_GPL(st7920_variants, DRM_ST7920);

It's new and there are no other users in the series. Why do you export 
this symbol?

Another problem is that you hard-code constants for the display above, 
but here you introduce display variants. Your driver should only contain 
one of these. Either it's all hard coded or all flexible.

> +
> +struct st7920_plane_state {
> +	struct drm_shadow_plane_state base;
> +	/* Intermediate buffer to convert pixels from XRGB8888 to HW format */
> +	u8 *buffer;
> +};
> +
> +struct st7920_crtc_state {
> +	struct drm_crtc_state base;
> +	/* Buffer to store pixels in HW format and written to the panel */
> +	u8 *data_array;
> +};
> +
> +static inline struct st7920_plane_state *to_st7920_plane_state(struct drm_plane_state *state)
> +{
> +	return container_of(state, struct st7920_plane_state, base.base);
> +}
> +
> +static inline struct st7920_crtc_state *to_st7920_crtc_state(struct drm_crtc_state *state)
> +{
> +	return container_of(state, struct st7920_crtc_state, base);
> +}
> +
> +static inline struct st7920_device *drm_to_st7920(struct drm_device *drm)
> +{
> +	return container_of(drm, struct st7920_device, drm);
> +}
> +
> +static int st7920_store_gdram_address(const void *data, u8 *reg)
> +{
> +	const u8 y_addr = *(const u8 *)data;
> +	bool bottom_screen = (y_addr >= 32);
> +	int i = 0;
> +
> +	reg[i++] = SYNC_BITS;
> +	/* Set vertical address */
> +	if (!bottom_screen)
> +		reg[i++] = TOP_VERTICAL_ADDRESS + (*(uint8_t *)data & HIGH_DATA_MASK);
> +	else
> +		reg[i++] = BOTTOM_VERTICAL_ADDRESS + (*(uint8_t *)data & HIGH_DATA_MASK);
> +
> +	reg[i++] = *(uint8_t *)data << 4;
> +	/* Set horizontal address */
> +	reg[i++] = SET_GDRAM_ADDRESS;
> +	if (!bottom_screen)
> +		reg[i++] = TOP_HORIZONTAL_ADDRESS;
> +	else
> +		reg[i++] = BOTTOM_HORIZONTAL_ADDRESS;
> +
> +	return i;
> +}
> +
> +static int st7920_store_gdram_data(const void *data, u8 *reg)
> +{
> +	const u8 *line_data = data;
> +	int i = 0, j = 0;
> +
> +	reg[i++] = SYNC_BITS | RS_HIGH;
> +
> +	for (j = 0; j < 16; j++) {
> +		reg[i++] = line_data[j] & 0xF0;
> +		reg[i++] = (line_data[j] << 4) & 0xF0;
> +	}
> +
> +	return i;
> +}
> +
> +static int st7920_store_others(int cmd, const void *data, u8 *reg)
> +{
> +	int i = 0;
> +
> +	reg[i++] = SYNC_BITS;
> +	reg[i++] = cmd & HIGH_DATA_MASK;
> +	reg[i++] = (cmd & LOW_DATA_MASK) << 4;
> +
> +	return i;
> +}
> +
> +static void st7920_spi_write(struct spi_device *spi, int cmd, const void *data,
> +			     int delay_us, struct spi7920_error *err)
> +{
> +	u8 reg[CMD_SIZE] = {0};
> +	int size = 0;
> +	int ret;
> +
> +	if (err->errno)
> +		return;
> +
> +	/*
> +	 * First the sync bits are sent: 11111WS0.
> +	 * Where W is the read/write (RW) bit and S is the register/data (RS) bit.
> +	 * Then, every 8 bits instruction/data will be separated into 2 groups.
> +	 * Higher 4 bits (DB7~DB4) will be placed in the first section followed by
> +	 * 4 '0's. And lower 4 bits (DB3~DB0) will be placed in the second section
> +	 * followed by 4 '0's.
> +	 */
> +	if (cmd == SET_GDRAM_ADDRESS)
> +		size = st7920_store_gdram_address(data, reg);
> +	else if (cmd == SET_GDRAM_DATA)
> +		size = st7920_store_gdram_data(data, reg);
> +	else
> +		size = st7920_store_others(cmd, data, reg);
> +
> +	ret = spi_write(spi, reg, size);
> +	if (ret) {
> +		err->errno = ret;
> +		return;
> +	}
> +
> +	if (delay_us)
> +		udelay(delay_us);
> +}
> +
> +static const struct regmap_config st7920_spi_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static const struct of_device_id st7920_of_match[] = {
> +	/* st7920 family */
> +	{
> +		.compatible = "sitronix,st7920",
> +		.data = &st7920_variants[ST7920_ID],
> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, st7920_of_match);
> +
> +/*
> + * The SPI core always reports a MODALIAS uevent of the form "spi:<dev>", even
> + * if the device was registered via OF. This means that the module will not be
> + * auto loaded, unless it contains an alias that matches the MODALIAS reported.
> + *
> + * To workaround this issue, add a SPI device ID table. Even when this should
> + * not be needed for this driver to match the registered SPI devices.
> + */
> +static const struct spi_device_id st7920_spi_id[] = {
> +	/* st7920 family */
> +	{ "st7920",  ST7920_ID },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(spi, st7920_spi_id);
> +
> +static void st7920_power_on(struct st7920_device *st7920,
> +			    struct spi7920_error *err)
> +{
> +	st7920_spi_write(st7920->spi, SET_DISPLAY_ON, NULL, 72, err);
> +}
> +
> +static void st7920_power_off(struct st7920_device *st7920,
> +			     struct spi7920_error *err)
> +{
> +	st7920_spi_write(st7920->spi, SET_DISPLAY_CLEAR, NULL, 1600, err);
> +	st7920_spi_write(st7920->spi, SET_DISPLAY_OFF, NULL, 72, err);
> +}
> +
> +static void st7920_hw_reset(struct st7920_device *st7920)
> +{
> +	if (!st7920->reset_gpio)
> +		return;
> +
> +	gpiod_set_value_cansleep(st7920->reset_gpio, 0);
> +	usleep_range(15, 20);
> +	gpiod_set_value_cansleep(st7920->reset_gpio, 1);
> +	msleep(40);
> +}
> +
> +static int st7920_init(struct st7920_device *st7920)
> +{
> +	struct spi7920_error err = {0};
> +
> +	st7920_spi_write(st7920->spi, SET_BASIC_INSTRUCTION_SET, NULL, 72, &err);
> +	st7920_power_on(st7920, &err);
> +	st7920_spi_write(st7920->spi, SET_GRAPHICS_DISPLAY, NULL, 72, &err);
> +	st7920_spi_write(st7920->spi, SET_DISPLAY_CLEAR, NULL, 1600, &err);
> +
> +	return err.errno;
> +}
> +
> +static int st7920_update_rect(struct st7920_device *st7920,
> +			       struct drm_rect *rect, u8 *buf,
> +			       u8 *data_array)
> +{
> +	struct spi7920_error err;
> +	u32 array_idx = 0;
> +	int i, j;
> +
> +	/*
> +	 * The screen is divided in 64(Y)x8(X) segments and each segment is
> +	 * further divided in 2 bytes (D15~D0).
> +	 *
> +	 * Segment 0x0 is in the top-right corner, while segment 63x15 is in the
> +	 * bottom-left. They would be displayed in the screen in the following way:
> +	 * 0x0  0x1  0x2  ... 0x15
> +	 * 1x0  1x1  1x2  ... 1x15
> +	 * ...
> +	 * 63x0 63x1 63x2 ... 63x15
> +	 *
> +	 * The data in each byte is big endian.
> +	 */
> +
> +	for (i = 0; i < DRM_SCANLINES; i++) {
> +		u8 *line_start = buf + (i * DRM_PITCH);
> +		u8 line_buffer[DRM_PITCH];
> +
> +		for (j = 0; j < DRM_PITCH; j++) {
> +			line_buffer[j] = bitrev8(line_start[j]);
> +			data_array[array_idx++] = line_buffer[j];
> +		}
> +
> +		st7920_spi_write(st7920->spi, SET_GDRAM_ADDRESS, &i, 72, &err);
> +		st7920_spi_write(st7920->spi, SET_GDRAM_DATA, line_buffer, 72, &err);
> +	}
> +
> +	return err.errno;
> +}
> +
> +static void st7920_clear_screen(struct st7920_device *st7920, u8 *data_array)
> +{
> +	struct spi7920_error err;
> +
> +	memset(data_array, 0, BYTES_IN_DISPLAY);
> +
> +	st7920_spi_write(st7920->spi, SET_DISPLAY_CLEAR, NULL, 1600, &err);
> +}
> +
> +static int st7920_fb_blit_rect(struct drm_framebuffer *fb,
> +				const struct iosys_map *vmap,
> +				struct drm_rect *rect,
> +				u8 *buf, u8 *data_array,
> +				struct drm_format_conv_state *fmtcnv_state)
> +{
> +	struct st7920_device *st7920 = drm_to_st7920(fb->dev);
> +	struct iosys_map dst;
> +	unsigned int dst_pitch;
> +	int ret;
> +
> +	/* Align y to display page boundaries */
> +	rect->y1 = round_down(rect->y1, PIXELS_PER_SEGMENT);
> +	rect->y2 = min_t(unsigned int, round_up(rect->y2, PIXELS_PER_SEGMENT), st7920->height);
> +
> +	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
> +
> +	iosys_map_set_vaddr(&dst, buf);
> +	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);
> +
> +	ret = st7920_update_rect(st7920, rect, buf, data_array);
> +
> +	return ret;
> +}
> +
> +static int st7920_primary_plane_atomic_check(struct drm_plane *plane,
> +					      struct drm_atomic_state *state)
> +{
> +	struct drm_device *drm = plane->dev;
> +	struct st7920_device *st7920 = drm_to_st7920(drm);
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct st7920_plane_state *st7920_state = to_st7920_plane_state(plane_state);
> +	struct drm_shadow_plane_state *shadow_plane_state = &st7920_state->base;
> +	struct drm_crtc *crtc = plane_state->crtc;
> +	struct drm_crtc_state *crtc_state = NULL;
> +	const struct drm_format_info *fi;
> +	unsigned int pitch;
> +	int ret;
> +
> +	if (crtc)
> +		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +
> +	ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> +						  DRM_PLANE_NO_SCALING,
> +						  DRM_PLANE_NO_SCALING,
> +						  false, false);
> +	if (ret)
> +		return ret;
> +	else if (!plane_state->visible)
> +		return 0;
> +
> +	fi = drm_format_info(DRM_FORMAT_R1);
> +	if (!fi)
> +		return -EINVAL;
> +
> +	pitch = drm_format_info_min_pitch(fi, 0, st7920->width);
> +
> +	if (plane_state->fb->format != fi) {
> +		void *buf;
> +
> +		/* format conversion necessary; reserve buffer */
> +		buf = drm_format_conv_state_reserve(&shadow_plane_state->fmtcnv_state,
> +						    pitch, GFP_KERNEL);
> +		if (!buf)
> +			return -ENOMEM;
> +	}
> +
> +	st7920_state->buffer = kcalloc(pitch, st7920->height, GFP_KERNEL);
> +	if (!st7920_state->buffer)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +static void st7920_primary_plane_atomic_update(struct drm_plane *plane,
> +						struct drm_atomic_state *state)
> +{
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
> +	struct st7920_crtc_state *st7920_crtc_state =  to_st7920_crtc_state(crtc_state);
> +	struct st7920_plane_state *st7920_plane_state = to_st7920_plane_state(plane_state);
> +	struct drm_framebuffer *fb = plane_state->fb;
> +	struct drm_atomic_helper_damage_iter iter;
> +	struct drm_device *drm = plane->dev;
> +	struct drm_rect dst_clip;
> +	struct drm_rect damage;
> +	int idx;
> +	int ret;
> +
> +	if (!drm_dev_enter(drm, &idx))
> +		return;
> +
> +	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE))
> +		goto out_drm_dev_exit;

This works, but I think it might be better to adopt the pattern used at 
[1]. It integrates begin_cpu_access and the damage-update loop into a 
single nice code block.  The failure returned by begin_cpu_access is 
transitory. Not making this an error is much closer to what happens 
here. It's also easier to follow when reading the code.

[1] 
https://gitlab.freedesktop.org/drm/misc/kernel/-/blob/drm-misc-next-2025-11-14-1/drivers/gpu/drm/ast/ast_mode.c?ref_type=tags#L561

> +
> +	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
> +	drm_atomic_for_each_plane_damage(&iter, &damage) {
> +		dst_clip = plane_state->dst;
> +
> +		if (!drm_rect_intersect(&dst_clip, &damage))
> +			continue;
> +
> +		ret = st7920_fb_blit_rect(fb, &shadow_plane_state->data[0], &dst_clip,
> +				st7920_plane_state->buffer,
> +				st7920_crtc_state->data_array,
> +				&shadow_plane_state->fmtcnv_state);
> +		if (ret)
> +			drm_err_once(plane->dev, "Failed to write to device: %d.\n", ret);
> +	}
> +
> +	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +
> +out_drm_dev_exit:
> +	drm_dev_exit(idx);
> +}
> +
> +static void st7920_primary_plane_atomic_disable(struct drm_plane *plane,
> +						 struct drm_atomic_state *state)
> +{
> +	struct drm_device *drm = plane->dev;
> +	struct st7920_device *st7920 = drm_to_st7920(drm);
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_crtc_state *crtc_state;
> +	struct st7920_crtc_state *st7920_crtc_state;
> +	int idx;
> +
> +	if (!plane_state->crtc)
> +		return;
> +
> +	crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
> +	st7920_crtc_state = to_st7920_crtc_state(crtc_state);
> +
> +	if (!drm_dev_enter(drm, &idx))
> +		return;
> +
> +	st7920_clear_screen(st7920, st7920_crtc_state->data_array);
> +
> +	drm_dev_exit(idx);
> +}
> +
> +/* Called during init to allocate the plane's atomic state. */
> +static void st7920_primary_plane_reset(struct drm_plane *plane)
> +{
> +	struct st7920_plane_state *st7920_state;
> +
> +	drm_WARN_ON_ONCE(plane->dev, plane->state);
> +
> +	st7920_state = kzalloc(sizeof(*st7920_state), GFP_KERNEL);
> +	if (!st7920_state)
> +		return;
> +
> +	__drm_gem_reset_shadow_plane(plane, &st7920_state->base);
> +}
> +
> +static struct drm_plane_state *st7920_primary_plane_duplicate_state(struct drm_plane *plane)
> +{
> +	struct drm_shadow_plane_state *new_shadow_plane_state;
> +	struct st7920_plane_state *old_st7920_state;
> +	struct st7920_plane_state *st7920_state;
> +
> +	if (drm_WARN_ON_ONCE(plane->dev, !plane->state))
> +		return NULL;
> +
> +	old_st7920_state = to_st7920_plane_state(plane->state);
> +	st7920_state = kmemdup(old_st7920_state, sizeof(*st7920_state), GFP_KERNEL);
> +	if (!st7920_state)
> +		return NULL;

Pleas don't use kmemdup here. The state has meaning and not all fields 
should be copied. You even address this in the next line. The call to 
__drm_gem_duplicate_shadow_plane_state() also relies on getting zero'd 
memory. Rather use kzalloc() and duplicate what's needed.


> +
> +	/* The buffer is not duplicated and is allocated in .atomic_check */
> +	st7920_state->buffer = NULL;
> +
> +	new_shadow_plane_state = &st7920_state->base;
> +
> +	__drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state);
> +
> +	return &new_shadow_plane_state->base;
> +}
> +
> +static void st7920_primary_plane_destroy_state(struct drm_plane *plane,
> +						struct drm_plane_state *state)
> +{
> +	struct st7920_plane_state *st7920_state = to_st7920_plane_state(state);
> +
> +	kfree(st7920_state->buffer);
> +
> +	__drm_gem_destroy_shadow_plane_state(&st7920_state->base);
> +
> +	kfree(st7920_state);
> +}
> +
> +static const struct drm_plane_helper_funcs st7920_primary_plane_helper_funcs = {
> +	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> +	.atomic_check = st7920_primary_plane_atomic_check,
> +	.atomic_update = st7920_primary_plane_atomic_update,
> +	.atomic_disable = st7920_primary_plane_atomic_disable,
> +};
> +
> +static const struct drm_plane_funcs st7920_primary_plane_funcs = {
> +	.update_plane = drm_atomic_helper_update_plane,
> +	.disable_plane = drm_atomic_helper_disable_plane,
> +	.reset = st7920_primary_plane_reset,
> +	.atomic_duplicate_state = st7920_primary_plane_duplicate_state,
> +	.atomic_destroy_state = st7920_primary_plane_destroy_state,
> +	.destroy = drm_plane_cleanup,
> +};
> +
> +static enum drm_mode_status st7920_crtc_mode_valid(struct drm_crtc *crtc,
> +						    const struct drm_display_mode *mode)
> +{
> +	struct st7920_device *st7920 = drm_to_st7920(crtc->dev);
> +
> +	return drm_crtc_helper_mode_valid_fixed(crtc, mode, &st7920->mode);
> +}
> +
> +static int st7920_crtc_atomic_check(struct drm_crtc *crtc,
> +				     struct drm_atomic_state *state)
> +{
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +	struct st7920_crtc_state *st7920_state = to_st7920_crtc_state(crtc_state);
> +	int ret;
> +
> +	ret = drm_crtc_helper_atomic_check(crtc, state);
> +	if (ret)
> +		return ret;
> +
> +	st7920_state->data_array = kmalloc(BYTES_IN_DISPLAY, GFP_KERNEL);
> +	if (!st7920_state->data_array)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +static void st7920_crtc_atomic_enable(struct drm_crtc *crtc,
> +				      struct drm_atomic_state *state)
> +{
> +	struct drm_device *drm = crtc->dev;
> +	struct st7920_device *st7920 = drm_to_st7920(drm);
> +	int idx;
> +	int ret;
> +
> +	if (!drm_dev_enter(drm, &idx))
> +		return;
> +
> +	st7920_hw_reset(st7920);
> +
> +	ret = st7920_init(st7920);
> +	if (ret)
> +		dev_err(st7920->dev, "Failed to init hardware: %d\n", ret);
> +
> +	drm_dev_exit(idx);
> +}
> +
> +static void st7920_crtc_atomic_disable(struct drm_crtc *crtc,
> +				       struct drm_atomic_state *state)
> +{
> +	struct spi7920_error err = {0};
> +	struct drm_device *drm = crtc->dev;
> +	struct st7920_device *st7920 = drm_to_st7920(drm);
> +	int idx;
> +
> +	drm_dev_enter(drm, &idx);
> +
> +	st7920_power_off(st7920, &err);
> +
> +	drm_dev_exit(idx);
> +}
> +
> +/* Called during init to allocate the CRTC's atomic state. */
> +static void st7920_crtc_reset(struct drm_crtc *crtc)
> +{
> +	struct st7920_crtc_state *st7920_state;
> +
> +	drm_WARN_ON_ONCE(crtc->dev, crtc->state);
> +
> +	st7920_state = kzalloc(sizeof(*st7920_state), GFP_KERNEL);
> +	if (!st7920_state)
> +		return;
> +
> +	__drm_atomic_helper_crtc_reset(crtc, &st7920_state->base);
> +}
> +
> +static struct drm_crtc_state *st7920_crtc_duplicate_state(struct drm_crtc *crtc)
> +{
> +	struct st7920_crtc_state *old_st7920_state;
> +	struct st7920_crtc_state *st7920_state;
> +
> +	if (drm_WARN_ON_ONCE(crtc->dev, !crtc->state))
> +		return NULL;
> +
> +	old_st7920_state = to_st7920_crtc_state(crtc->state);
> +	st7920_state = kmemdup(old_st7920_state, sizeof(*st7920_state), GFP_KERNEL);
> +	if (!st7920_state)
> +		return NULL;

Same problem with kmemdup() here as outlined above. Please use kzalloc 
instead.

> +
> +	/* The buffer is not duplicated and is allocated in .atomic_check */
> +	st7920_state->data_array = NULL;
> +
> +	__drm_atomic_helper_crtc_duplicate_state(crtc, &st7920_state->base);
> +
> +	return &st7920_state->base;
> +}
> +
> +static void st7920_crtc_destroy_state(struct drm_crtc *crtc,
> +						struct drm_crtc_state *state)
> +{
> +	struct st7920_crtc_state *st7920_state = to_st7920_crtc_state(state);
> +
> +	kfree(st7920_state->data_array);
> +
> +	__drm_atomic_helper_crtc_destroy_state(state);
> +
> +	kfree(st7920_state);
> +}
> +
> +/*
> + * The CRTC is always enabled. Screen updates are performed by
> + * the primary plane's atomic_update function. Disabling clears
> + * the screen in the primary plane's atomic_disable function.
> + */
> +static const struct drm_crtc_helper_funcs st7920_crtc_helper_funcs = {
> +	.mode_valid = st7920_crtc_mode_valid,
> +	.atomic_check = st7920_crtc_atomic_check,
> +	.atomic_enable = st7920_crtc_atomic_enable,
> +	.atomic_disable = st7920_crtc_atomic_disable,
> +};
> +
> +static const struct drm_crtc_funcs st7920_crtc_funcs = {
> +	.reset = st7920_crtc_reset,
> +	.destroy = drm_crtc_cleanup,
> +	.set_config = drm_atomic_helper_set_config,
> +	.page_flip = drm_atomic_helper_page_flip,
> +	.atomic_duplicate_state = st7920_crtc_duplicate_state,
> +	.atomic_destroy_state = st7920_crtc_destroy_state,
> +};
> +
> +static const struct drm_encoder_funcs st7920_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +
> +static int st7920_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct st7920_device *st7920 = drm_to_st7920(connector->dev);
> +
> +	return drm_connector_helper_get_modes_fixed(connector, &st7920->mode);
> +}
> +
> +static const struct drm_connector_helper_funcs st7920_connector_helper_funcs = {
> +	.get_modes = st7920_connector_get_modes,
> +};
> +
> +static const struct drm_connector_funcs st7920_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 const struct drm_mode_config_funcs st7920_mode_config_funcs = {
> +	.fb_create = drm_gem_fb_create_with_dirty,
> +	.atomic_check = drm_atomic_helper_check,
> +	.atomic_commit = drm_atomic_helper_commit,
> +};
> +
> +static const uint32_t st7920_formats[] = {
> +	DRM_FORMAT_XRGB8888,
> +};
> +
> +DEFINE_DRM_GEM_FOPS(st7920_fops);
> +
> +static const struct drm_driver st7920_drm_driver = {
> +	DRM_GEM_SHMEM_DRIVER_OPS,
> +	DRM_FBDEV_SHMEM_DRIVER_OPS,
> +	.name			= DRIVER_NAME,
> +	.desc			= DRIVER_DESC,
> +	.date			= DRIVER_DATE,
> +	.major			= DRIVER_MAJOR,
> +	.minor			= DRIVER_MINOR,
> +	.driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
> +	.fops			= &st7920_fops,
> +};
> +
> +static int st7920_init_modeset(struct st7920_device *st7920)
> +{
> +	struct drm_display_mode *mode = &st7920->mode;
> +	struct device *dev = st7920->dev;
> +	struct drm_device *drm = &st7920->drm;
> +	unsigned long max_width, max_height;
> +	struct drm_plane *primary_plane;
> +	struct drm_crtc *crtc;
> +	struct drm_encoder *encoder;
> +	struct drm_connector *connector;
> +	int ret;
> +
> +	/*
> +	 * Modesetting
> +	 */
> +
> +	ret = drmm_mode_config_init(drm);
> +	if (ret) {
> +		dev_err(dev, "DRM mode config init failed: %d\n", ret);

It should be save to use drm_ logging here. Please do. Also below.

> +		return ret;
> +	}
> +
> +	mode->type = DRM_MODE_TYPE_DRIVER;
> +	mode->clock = 1;

Userspace might synchronize its output to the display frequency. 1 Hz 
looks kinda sluggish :)  For maximum compatibility better use 60 Hz 
here. Or some fraction/multiple that the display can realistically 
perform. The data sheet at [2] says that a frame takes 18.3 ms (~50 Hz). 
So a display refresh of 30 Hz should be fine.

[2] https://www.hpinfotech.ro/ST7920.pdf


> +	mode->hdisplay = st7920->device_info->default_width;
> +	mode->htotal = st7920->device_info->default_width;
> +	mode->hsync_start = st7920->device_info->default_width;
> +	mode->hsync_end = st7920->device_info->default_width;
> +	mode->vdisplay = st7920->device_info->default_height;
> +	mode->vtotal = st7920->device_info->default_height;
> +	mode->vsync_start = st7920->device_info->default_height;
> +	mode->vsync_end = st7920->device_info->default_height;
> +	mode->width_mm = 27;
> +	mode->height_mm = 27;
> +
> +	max_width = max_t(unsigned long, mode->hdisplay, DRM_SHADOW_PLANE_MAX_WIDTH);
> +	max_height = max_t(unsigned long, mode->vdisplay, DRM_SHADOW_PLANE_MAX_HEIGHT);
> +
> +	drm->mode_config.min_width = mode->hdisplay;
> +	drm->mode_config.max_width = max_width;
> +	drm->mode_config.min_height = mode->vdisplay;
> +	drm->mode_config.max_height = max_height;
> +	drm->mode_config.preferred_depth = 24;
> +	drm->mode_config.funcs = &st7920_mode_config_funcs;
> +
> +	/* Primary plane */
> +
> +	primary_plane = &st7920->primary_plane;
> +	ret = drm_universal_plane_init(drm, primary_plane, 0, &st7920_primary_plane_funcs,
> +				    st7920_formats, ARRAY_SIZE(st7920_formats),
> +				    NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> +	if (ret) {
> +		dev_err(dev, "DRM primary plane init failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	drm_plane_helper_add(primary_plane, &st7920_primary_plane_helper_funcs);
> +
> +	drm_plane_enable_fb_damage_clips(primary_plane);
> +
> +	/* CRTC */
> +
> +	crtc = &st7920->crtc;
> +	ret = drm_crtc_init_with_planes(drm, crtc, primary_plane, NULL,
> +					&st7920_crtc_funcs, NULL);
> +	if (ret) {
> +		dev_err(dev, "DRM crtc init failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	drm_crtc_helper_add(crtc, &st7920_crtc_helper_funcs);
> +
> +	/* Encoder */
> +
> +	encoder = &st7920->encoder;
> +	ret = drm_encoder_init(drm, encoder, &st7920_encoder_funcs,
> +			       DRM_MODE_ENCODER_NONE, NULL);
> +	if (ret) {
> +		dev_err(dev, "DRM encoder init failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	encoder->possible_crtcs = drm_crtc_mask(crtc);
> +
> +	/* Connector */
> +
> +	connector = &st7920->connector;
> +	ret = drm_connector_init(drm, connector, &st7920_connector_funcs,
> +				 DRM_MODE_CONNECTOR_Unknown);
> +	if (ret) {
> +		dev_err(dev, "DRM connector init failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	drm_connector_helper_add(connector, &st7920_connector_helper_funcs);
> +
> +	ret = drm_connector_attach_encoder(connector, encoder);
> +	if (ret) {
> +		dev_err(dev, "DRM attach connector to encoder failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	drm_mode_config_reset(drm);
> +
> +	return 0;
> +}
> +
> +static int st7920_probe(struct spi_device *spi)
> +{
> +	struct st7920_device *st7920;
> +	struct regmap *regmap;
> +	struct device *dev = &spi->dev;
> +	struct drm_device *drm;
> +	int ret;
> +
> +	regmap = devm_regmap_init_spi(spi, &st7920_spi_regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	st7920 = devm_drm_dev_alloc(dev, &st7920_drm_driver,
> +				    struct st7920_device, drm);
> +	if (IS_ERR(st7920))
> +		return PTR_ERR(st7920);
> +
> +	drm = &st7920->drm;
> +
> +	st7920->dev = dev;
> +	st7920->regmap = regmap;
> +	st7920->spi = spi;
> +	st7920->device_info = device_get_match_data(dev);
> +	st7920->width = st7920->device_info->default_width;
> +	st7920->height = st7920->device_info->default_height;
> +
> +	st7920->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(st7920->reset_gpio)) {
> +		ret = PTR_ERR(st7920->reset_gpio);
> +		dev_err(dev, "Unable to retrieve reset GPIO: %d\n", ret);
> +		return ret;
> +	}
> +
> +	spi_set_drvdata(spi, st7920);
> +
> +	ret = st7920_init_modeset(st7920);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_dev_register(drm, 0);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "DRM device register failed\n");
> +
> +	drm_client_setup(drm, NULL);
> +
> +	return 0;
> +}
> +
> +static void st7920_remove(struct spi_device *spi)
> +{
> +	struct st7920_device *st7920 = spi_get_drvdata(spi);
> +
> +	drm_dev_unplug(&st7920->drm);
> +	drm_atomic_helper_shutdown(&st7920->drm);
> +}
> +
> +static void st7920_shutdown(struct spi_device *spi)
> +{
> +	struct st7920_device *st7920 = spi_get_drvdata(spi);
> +
> +	drm_atomic_helper_shutdown(&st7920->drm);
> +}
> +
> +static struct spi_driver st7920_spi_driver = {
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = st7920_of_match,
> +	},
> +	.id_table = st7920_spi_id,
> +	.probe = st7920_probe,
> +	.remove = st7920_remove,
> +	.shutdown = st7920_shutdown,
> +};
> +module_spi_driver(st7920_spi_driver);
> +
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_AUTHOR("Iker Pedrosa <ipedrosam@gmail.com>");
> +MODULE_LICENSE("GPL v2");
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

On 31/10/2025 10:05, Iker Pedrosa wrote:
> +
> +static void st7920_hw_reset(struct st7920_device *st7920)
> +{
> +	if (!st7920->reset_gpio)
> +		return;
> +
> +	gpiod_set_value_cansleep(st7920->reset_gpio, 0);
> +	usleep_range(15, 20);
> +	gpiod_set_value_cansleep(st7920->reset_gpio, 1);

This is odd. Why are you keeping device in reset state?

I don't think you tested your driver with DTS.

> +	msleep(40);
> +}
> +

> +
> +	regmap = devm_regmap_init_spi(spi, &st7920_spi_regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	st7920 = devm_drm_dev_alloc(dev, &st7920_drm_driver,
> +				    struct st7920_device, drm);
> +	if (IS_ERR(st7920))
> +		return PTR_ERR(st7920);
> +
> +	drm = &st7920->drm;
> +
> +	st7920->dev = dev;
> +	st7920->regmap = regmap;
> +	st7920->spi = spi;
> +	st7920->device_info = device_get_match_data(dev);
> +	st7920->width = st7920->device_info->default_width;
> +	st7920->height = st7920->device_info->default_height;
> +
> +	st7920->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);

So you keep device in reset state, then why do you toggle it in
st7920_hw_reset()? Remember that this is the logical, not line level.

> +	if (IS_ERR(st7920->reset_gpio)) {
> +		ret = PTR_ERR(st7920->reset_gpio);
> +		dev_err(dev, "Unable to retrieve reset GPIO: %d\n", ret);

return dev_err_probe, didn't you get comment on that?

> +		return ret;
> +	}
> +
> +	spi_set_drvdata(spi, st7920);
> +
> +	ret = st7920_init_modeset(st7920);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_dev_register(drm, 0);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "DRM device register failed\n");
> +
> +	drm_client_setup(drm, NULL);
> +

Best regards,
Krzysztof

El lun, 17 nov 2025 a las 11:57, Krzysztof Kozlowski
(<krzk@kernel.org>) escribió:
>
> On 31/10/2025 10:05, Iker Pedrosa wrote:
> > +
> > +static void st7920_hw_reset(struct st7920_device *st7920)
> > +{
> > +     if (!st7920->reset_gpio)
> > +             return;
> > +
> > +     gpiod_set_value_cansleep(st7920->reset_gpio, 0);
> > +     usleep_range(15, 20);
> > +     gpiod_set_value_cansleep(st7920->reset_gpio, 1);
>
> This is odd. Why are you keeping device in reset state?
>
> I don't think you tested your driver with DTS.
>
> > +     msleep(40);
> > +}
> > +
>
> > +
> > +     regmap = devm_regmap_init_spi(spi, &st7920_spi_regmap_config);
> > +     if (IS_ERR(regmap))
> > +             return PTR_ERR(regmap);
> > +
> > +     st7920 = devm_drm_dev_alloc(dev, &st7920_drm_driver,
> > +                                 struct st7920_device, drm);
> > +     if (IS_ERR(st7920))
> > +             return PTR_ERR(st7920);
> > +
> > +     drm = &st7920->drm;
> > +
> > +     st7920->dev = dev;
> > +     st7920->regmap = regmap;
> > +     st7920->spi = spi;
> > +     st7920->device_info = device_get_match_data(dev);
> > +     st7920->width = st7920->device_info->default_width;
> > +     st7920->height = st7920->device_info->default_height;
> > +
> > +     st7920->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>
> So you keep device in reset state, then why do you toggle it in
> st7920_hw_reset()? Remember that this is the logical, not line level.

You are right. I was confused between the logical state and the
physical line level regarding gpiod.

I have fixed the logic in st7920_hw_reset() to correctly assert the
reset (logical 1) and then de-assert it (logical 0). Accordingly, I
have also changed the probe initialization to GPIOD_OUT_LOW to ensure
the device starts in a non-reset state.

I have also addressed the other comments. These fixes will be
available in the next version which I will submit shortly.

>
> > +     if (IS_ERR(st7920->reset_gpio)) {
> > +             ret = PTR_ERR(st7920->reset_gpio);
> > +             dev_err(dev, "Unable to retrieve reset GPIO: %d\n", ret);
>
> return dev_err_probe, didn't you get comment on that?
>
> > +             return ret;
> > +     }
> > +
> > +     spi_set_drvdata(spi, st7920);
> > +
> > +     ret = st7920_init_modeset(st7920);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = drm_dev_register(drm, 0);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "DRM device register failed\n");
> > +
> > +     drm_client_setup(drm, NULL);
> > +
>
> Best regards,
> Krzysztof