[PATCH 0/2] Add support for st75156 in st7571 display driver

[PATCH 0/2] Add support for st75156 in st7571 display driver

[Lars Rask]

To: Marcus Folkesson <marcus.folkesson@gmail.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Maxime Ripard <mripard@kernel.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: kernel@axis.com

The Sitronix st7571 driver previously supported the st7567 and st7571
chips. This commit adds support for the st75156 chip used in SPI 4 wire mode.
In order to use the st75156 chip, you need to specify
CONFIG_DRM_ST7571=m and CONFIG_DRM_ST7571_SPI=m in the .config file, and
also use the compatible string "sitronix,st75156" in the device tree.

Signed-off-by: Lars Rask <lars.rask@axis.com>
---
Lars Rask (2):
      drm/sitronix/st7571-spi: Add support for ST75156 controller
      dt-bindings: display: sitronix,st75156: add example for SPI

 .../bindings/display/sitronix,st75156.yaml         |  67 ++++
 MAINTAINERS                                        |   1 +
 drivers/gpu/drm/sitronix/st7571-spi.c              | 118 +++++-
 drivers/gpu/drm/sitronix/st7571.c                  | 420 ++++++++++++++++++++-
 drivers/gpu/drm/sitronix/st7571.h                  |  20 +
 5 files changed, 617 insertions(+), 9 deletions(-)
---
base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
change-id: 20261015-add_st75156_support-00ec2c7f63a6

Best regards,
-- 
Lars Rask <lars.rask@axis.com>

[PATCH 1/2] drm/sitronix/st7571-spi: Add support for ST75156 controller

[Lars Rask]

From: Lars Rask <lars.rask@axis.com>

Made additions to st7571 display driver to support st75156.

Signed-off-by: Lars Rask <lars.rask@axis.com>
---
 drivers/gpu/drm/sitronix/st7571-spi.c | 118 +++++++++-
 drivers/gpu/drm/sitronix/st7571.c     | 420 +++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/sitronix/st7571.h     |  20 ++
 3 files changed, 549 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/sitronix/st7571-spi.c b/drivers/gpu/drm/sitronix/st7571-spi.c
index 0206e9162f1c..724c0eb5e7da 100644
--- a/drivers/gpu/drm/sitronix/st7571-spi.c
+++ b/drivers/gpu/drm/sitronix/st7571-spi.c
@@ -17,23 +17,133 @@ static const struct regmap_config st7571_spi_regmap_config = {
 	.can_multi_write = true,
 };
 
+struct spi_device_data {
+	struct spi_device *spi;
+	struct gpio_desc *dc_gpio;
+};
+
+static int regmap_spi_write(void *context, const void *data, size_t count);
+static int regmap_spi_read(void *context, const void *reg, size_t reg_size,
+			   void *val, size_t val_size);
+
+static const struct regmap_bus regmap_spi_dc_bus = {
+	.write = regmap_spi_write,
+	.read = regmap_spi_read,
+};
+
+static int regmap_spi_write(void *context, const void *data, size_t count)
+{
+	struct spi_device_data *spi_data = context;
+	const u8 *buf = data;
+	u8 mode;
+	size_t num_data_bytes;
+	const u8 *data_bytes;
+	int write_result;
+	const u8 *cmdbuf;
+	const u8 *databuf;
+
+	/*
+	 * count is required to be at least two bytes. The first byte is mode,
+	 * which can be either ST7571_COMMAND_MODE or ST7571_DATA_MODE. If the
+	 * first byte is ST7571_COMMAND_MODE, the second byte is a command
+	 * (register address). If the first byte is ST7571_DATA_MODE the second
+	 * byte is a data byte.
+	 */
+
+	if (count < 2)
+		return -EINVAL;
+
+	mode = buf[0];
+
+	if (mode == ST7571_COMMAND_MODE) {
+		gpiod_set_value_cansleep(spi_data->dc_gpio, 0);
+
+		cmdbuf = kmemdup(&buf[1], 1, GFP_KERNEL);
+		if (!cmdbuf)
+			return -ENOMEM;
+
+		write_result = spi_write(spi_data->spi, cmdbuf, 1);
+		kfree(cmdbuf);
+
+		if (write_result != 0)
+			return write_result;
+
+		num_data_bytes = count - 2;
+
+		if (num_data_bytes > 0) {
+			gpiod_set_value_cansleep(spi_data->dc_gpio, 1);
+			data_bytes = &buf[2];
+			databuf = kmemdup(data_bytes, num_data_bytes, GFP_KERNEL);
+
+			write_result = spi_write(spi_data->spi, databuf, num_data_bytes);
+			kfree(databuf);
+
+			if (write_result != 0)
+				return write_result;
+		}
+	} else if (mode == ST7571_DATA_MODE) {
+		num_data_bytes = count - 1;
+
+		if (num_data_bytes > 0) {
+			gpiod_set_value_cansleep(spi_data->dc_gpio, 1);
+			data_bytes = &buf[1];
+			databuf = kmemdup(data_bytes, num_data_bytes, GFP_KERNEL);
+			write_result = spi_write(spi_data->spi, databuf, num_data_bytes);
+			kfree(databuf);
+
+			if (write_result != 0)
+				return write_result;
+		}
+
+	} else {
+		/* Unsupported mode */
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/* The st7571 driver does not read registers but regmap expects a .read */
+static int regmap_spi_read(void *context, const void *reg, size_t reg_size,
+			   void *val, size_t val_size)
+{
+	return -EOPNOTSUPP;
+}
+
 static int st7571_spi_probe(struct spi_device *spi)
 {
 	struct st7571_device *st7571;
 	struct regmap *regmap;
+	struct spi_device_data *spi_data;
+
+	spi_data = devm_kzalloc(&spi->dev, sizeof(*spi_data), GFP_KERNEL);
+	if (IS_ERR(spi_data))
+		return dev_err_probe(&spi->dev, PTR_ERR(regmap),
+				     "Failed to initialize spi data\n");
 
-	regmap = devm_regmap_init_spi(spi, &st7571_spi_regmap_config);
-	if (IS_ERR(regmap)) {
+	spi_data->spi = spi;
+	spi_data->dc_gpio = devm_gpiod_get(&spi->dev, "dc", GPIOD_OUT_LOW);
+
+	if (IS_ERR(spi_data->dc_gpio))
+		/* init regmap for 3 line SPI */
+		regmap = devm_regmap_init_spi(spi, &st7571_spi_regmap_config);
+	else
+		/* init regmap for 4 line SPI */
+		regmap = devm_regmap_init(&spi->dev, &regmap_spi_dc_bus,
+					  spi_data, &st7571_spi_regmap_config);
+
+	if (IS_ERR(regmap))
 		return dev_err_probe(&spi->dev, PTR_ERR(regmap),
 				     "Failed to initialize regmap\n");
-	}
 
 	st7571 = st7571_probe(&spi->dev, regmap);
 	if (IS_ERR(st7571))
 		return dev_err_probe(&spi->dev, PTR_ERR(st7571),
 				     "Failed to initialize regmap\n");
 
+	st7571->spi_data = spi_data;
 	spi_set_drvdata(spi, st7571);
+
 	return 0;
 }
 
@@ -47,6 +157,7 @@ static void st7571_spi_remove(struct spi_device *spi)
 static const struct of_device_id st7571_of_match[] = {
 	{ .compatible = "sitronix,st7567", .data = &st7567_config },
 	{ .compatible = "sitronix,st7571", .data = &st7571_config },
+	{ .compatible = "sitronix,st75156", .data = &st75156_config },
 	{},
 };
 MODULE_DEVICE_TABLE(of, st7571_of_match);
@@ -54,6 +165,7 @@ MODULE_DEVICE_TABLE(of, st7571_of_match);
 static const struct spi_device_id st7571_spi_id[] = {
 	{ "st7567", 0 },
 	{ "st7571", 0 },
+	{ "st75156", 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, st7571_spi_id);
diff --git a/drivers/gpu/drm/sitronix/st7571.c b/drivers/gpu/drm/sitronix/st7571.c
index 20954c33eca9..2dd52f2214d2 100644
--- a/drivers/gpu/drm/sitronix/st7571.c
+++ b/drivers/gpu/drm/sitronix/st7571.c
@@ -11,6 +11,7 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
+#include <linux/backlight.h>
 
 #include <drm/clients/drm_client_setup.h>
 #include <drm/drm_atomic.h>
@@ -37,9 +38,6 @@
 
 #include "st7571.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)
@@ -73,6 +71,46 @@
 /* ST7567 commands */
 #define ST7567_SET_LCD_BIAS(m) (0xa2 | FIELD_PREP(GENMASK(0, 0), (m)))
 
+/* ST75156 commands */
+#define ST75156_EXTENSION_COMMAND_1			(0x30)
+#define ST75156_EXTENSION_COMMAND_2			(0x31)
+
+/* ST75156 Extension command 1 */
+#define ST75156_DISPLAY_OFF					(0xae)
+#define ST75156_DISPLAY_ON					(0xaf)
+#define ST75156_SLEEP_OUT					(0x94)
+#define ST75156_POWER_CONTROL				(0x20)
+#define ST75156_SET_VOP						(0x81)
+#define ST75156_DISPLAY_MODE				(0xf0)
+#define ST75156_DISPLAY_CONTROL				(0xca)
+#define ST75156_DATA_SCAN_DIRECTION			(0xbc)
+#define ST75156_NORMAL_DISPLAY				(0xa6)
+#define ST75156_COLUMN_ADDRESS_SETTING		(0x15)
+#define ST75156_PAGE_ADDRESS_SETTING		(0x75)
+#define ST75156_ENTER_WRITE_DDRAM_MODE		(0x5c)
+
+/* ST75156 Extension command 2 */
+#define ST75156_SET_AUTO_READ_INSTRUCTION	(0xd7)
+#define ST75156_DISABLE_AUTO_READ			(0x9f)
+#define ST75156_OTP_WR_RD_CONTROL			(0xe0)
+#define ST75156_OTP_UPLOAD					(0xe3)
+#define ST75156_OTP_CONTROL_OUT				(0xe1)
+#define ST75156_ANALOG_CIRCUIT_SET			(0x32)
+#define ST75156_BOOSTER_LEVEL_X10			(0x51)
+#define ST75156_INTERAL_POWER_SUPPLY		(0x40)
+
+/* Special command used to indicate to the st75156_send_command_list
+ * function that we should sleep for a number of milliseconds before
+ * the next command when sending the init sequence for ST75156.
+ * NOTE: This command is not part of the ST75156 instruction set.
+ */
+#define ST75156_SLEEP_MS					(0x00)
+
+/* ST75156 Command buffer */
+#define ST75156_COMMAND_LEN 1
+#define ST75156_MAX_PARAMETERS 3
+#define ST75156_COMMAND_BUFFER_LEN (ST75156_COMMAND_LEN + ST75156_MAX_PARAMETERS)
+
 #define ST7571_PAGE_HEIGHT 8
 
 #define DRIVER_NAME "st7571"
@@ -99,6 +137,89 @@ static int st7571_send_command_list(struct st7571_device *st7571,
 	return ret;
 }
 
+static int st75156_send_command_list(struct st7571_device *st75156,
+				     const u8 *cmd_list, size_t len)
+{
+	/* Each command in the command list is formatted using the following format:
+	 *
+	 * +---------+--------------------------+-------------+------+-------------+
+	 * | command | number of parameters (n) | parameter 1 | .... | parameter n |
+	 * +---------+--------------------------+-------------+------+-------------+
+	 *
+	 * For example, if a command has 2 parameters it is stored like this:
+	 *
+	 * +---------+------+-------------+--------------+
+	 * | command | 0x02 | parameter 1 |  parameter 2 |
+	 * +---------+------+-------------+--------------+
+	 *
+	 * If a command does not have any parameters it is stored like this:
+	 *
+	 * +---------+------+
+	 * | command | 0x00 |
+	 * +---------+------+
+	 *
+	 * NOTE: The command ST75156_SLEEP_MS (0x00) is a special command defined like this:
+	 *
+	 * +------------------+------+----+
+	 * | ST75156_SLEEP_MS | 0x01 | ms |
+	 * +------------------+------+----+
+	 *
+	 * When this command is sent, the code will sleep for ms milliseconds before
+	 * sending the next command in the command list.
+	 */
+
+	int ret;
+	int i = 0;
+
+	while (i < len) {
+		int command = cmd_list[i];
+		int num_params = cmd_list[i + 1];
+		int current_param = 0;
+		int param;
+		int bufpos = 0;
+		u8 *buf = st75156->commandbuf;
+
+		i = i + 2;
+
+		if (command == ST75156_SLEEP_MS) {
+			msleep(cmd_list[i]);
+		} else {
+			/* Add command to command buffer */
+			buf[bufpos] = command;
+			bufpos++;
+
+			if (bufpos >= ST75156_COMMAND_BUFFER_LEN)
+				return -EINVAL;
+
+			if (num_params > 0) {
+				if (num_params > ST75156_MAX_PARAMETERS)
+					return -EINVAL;
+
+				for (param = 0; param < num_params; param++) {
+					current_param = cmd_list[i + param];
+
+					/* Add parameter to command buffer */
+
+					if (bufpos >= ST75156_COMMAND_BUFFER_LEN)
+						return -EINVAL;
+
+					buf[bufpos] = current_param;
+					bufpos++;
+				}
+			}
+
+			/* Write command buffer */
+			ret = regmap_bulk_write(st75156->regmap, ST7571_COMMAND_MODE, buf, bufpos);
+			if (ret < 0)
+				return ret;
+		}
+
+		i = i + num_params;
+	}
+
+	return 0;
+}
+
 static inline u8 st7571_transform_xy(const char *p, int x, int y, u8 bpp)
 {
 	int xrest = x % 8;
@@ -127,6 +248,37 @@ static inline u8 st7571_transform_xy(const char *p, int x, int y, u8 bpp)
 	return result;
 }
 
+static inline u8 st75156_transform_xy(struct st7571_device *st75156, int x, int y)
+{
+	int xrest = x % 8;
+	u8 result = 0;
+	u8 row_len = (st75156->ncols / 8) * st75156->bpp;
+	int i;
+	const char *p = st75156->hwbuf;
+
+	/*
+	 * 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 (i = 0; i < 8; i++) {
+		int row_idx = y + i;
+		u8 byte = p[row_idx * row_len + x];
+		/* LSB in input is left most pixel */
+		u8 bit = (byte >> xrest) & 1;
+		/* LSB in output is 7th row */
+		result |= (bit << (7 - i));
+	}
+
+	return result;
+}
+
 static int st7571_set_position(struct st7571_device *st7571, int x, int y)
 {
 	u8 cmd_list[] = {
@@ -138,6 +290,23 @@ static int st7571_set_position(struct st7571_device *st7571, int x, int y)
 	return st7571_send_command_list(st7571, cmd_list, ARRAY_SIZE(cmd_list));
 }
 
+static int st75156_set_position(struct st7571_device *st75156, int x, int y)
+{
+	u8 xs_param = st75156->startcol + x;
+	u8 xe_param = st75156->startcol + st75156->ncols - 1;
+	u8 ys_param = y / ST7571_PAGE_HEIGHT;
+	u8 ye_param = ys_param;
+
+	u8 cmd_list[] = {
+		ST75156_EXTENSION_COMMAND_1, 0x00,
+		ST75156_COLUMN_ADDRESS_SETTING, 0x02, xs_param, xe_param,
+		ST75156_PAGE_ADDRESS_SETTING, 0x02, ys_param, ye_param,
+		ST75156_ENTER_WRITE_DDRAM_MODE, 0x00,
+	};
+
+	return st75156_send_command_list(st75156, cmd_list, ARRAY_SIZE(cmd_list));
+}
+
 static int st7571_fb_clear_screen(struct st7571_device *st7571)
 {
 	u32 npixels = st7571->ncols * round_up(st7571->nlines, ST7571_PAGE_HEIGHT) * st7571->bpp;
@@ -150,6 +319,21 @@ static int st7571_fb_clear_screen(struct st7571_device *st7571)
 	return 0;
 }
 
+static int st75156_fb_clear_screen(struct st7571_device *st75156)
+{
+	u32 npixels = st75156->ncols * st75156->bpp;
+	int y;
+
+	memset(st75156->row, 0, npixels);
+
+	for (y = 0; y < st75156->nlines; y += ST7571_PAGE_HEIGHT) {
+		st75156_set_position(st75156, 0, y);
+		regmap_bulk_write(st75156->regmap, ST7571_DATA_MODE, st75156->row, npixels);
+	}
+
+	return 0;
+}
+
 static void st7571_prepare_buffer_monochrome(struct st7571_device *st7571,
 					     const struct iosys_map *vmap,
 					     struct drm_framebuffer *fb,
@@ -284,6 +468,37 @@ static int st7571_fb_update_rect_grayscale(struct drm_framebuffer *fb, struct dr
 	return 0;
 }
 
+static int st75156_fb_update_rect_monochrome(struct drm_framebuffer *fb, struct drm_rect *rect)
+{
+	struct st7571_device *st75156 = drm_to_st7571(fb->dev);
+	struct device *dev = st75156->dev;
+
+	char *row = st75156->row;
+	int x;
+	int y;
+	int num_bytes;
+	int ret;
+
+	/* Align y to display page boundaries */
+	rect->y1 = round_down(rect->y1, ST7571_PAGE_HEIGHT);
+	rect->y2 = min_t(unsigned int, round_up(rect->y2, ST7571_PAGE_HEIGHT), st75156->nlines);
+
+	for (y = rect->y1; y < rect->y2; y += ST7571_PAGE_HEIGHT) {
+		for (x = rect->x1; x < rect->x2; x++)
+			row[x] = st75156_transform_xy(st75156, x, y);
+
+		st75156_set_position(st75156, rect->x1, y);
+
+		num_bytes = (rect->x2) - (rect->x1);
+		ret = regmap_bulk_write(st75156->regmap, ST7571_DATA_MODE,
+					row + rect->x1, num_bytes);
+		if (ret < 0)
+			dev_err(dev, "Failed to write data to display (error code: %i", ret);
+	}
+
+	return 0;
+}
+
 static int st7571_connector_get_modes(struct drm_connector *conn)
 {
 	struct st7571_device *st7571 = drm_to_st7571(conn->dev);
@@ -318,6 +533,17 @@ static const struct st7571_panel_format st7571_grayscale = {
 	.nformats = 3,
 };
 
+static const struct st7571_panel_format st75156_monochrome = {
+	.prepare_buffer = st7571_prepare_buffer_monochrome,
+	.update_rect = st75156_fb_update_rect_monochrome,
+	.mode = ST7571_COLOR_MODE_BLACKWHITE,
+	.formats = {
+		DRM_FORMAT_XRGB8888,
+		DRM_FORMAT_R1,
+	},
+	.nformats = 2,
+};
+
 static const u64 st7571_primary_plane_fmtmods[] = {
 	DRM_FORMAT_MOD_LINEAR,
 	DRM_FORMAT_MOD_INVALID
@@ -388,7 +614,7 @@ static void st7571_primary_plane_helper_atomic_disable(struct drm_plane *plane,
 	if (!drm_dev_enter(drm, &idx))
 		return;
 
-	st7571_fb_clear_screen(st7571);
+	st7571->pdata->fb_clear_screen(st7571);
 	drm_dev_exit(idx);
 }
 
@@ -444,6 +670,13 @@ static void st7571_encoder_atomic_enable(struct drm_encoder *encoder,
 	u8 command = ST7571_DISPLAY_ON;
 	int ret;
 
+	/* ST75156 needs init function to be run earlier than ST7571
+	 * so we need to call it in the probe function instead of
+	 * the encoder_atomic_enable function.
+	 */
+	if (st7571->device_type == DEVICE_TYPE_ST75156)
+		return;
+
 	ret = st7571->pdata->init(st7571);
 	if (ret)
 		return;
@@ -458,6 +691,9 @@ static void st7571_encoder_atomic_disable(struct drm_encoder *encoder,
 	struct st7571_device *st7571 = drm_to_st7571(drm);
 	u8 command = ST7571_DISPLAY_OFF;
 
+	if (st7571->device_type == DEVICE_TYPE_ST75156)
+		return;
+
 	st7571_send_command_list(st7571, &command, 1);
 }
 
@@ -672,6 +908,8 @@ static int st7567_parse_dt(struct st7571_device *st7567)
 	st7567->nlines = dt.vactive.typ;
 	st7567->ncols = dt.hactive.typ;
 
+	st7567->device_type = DEVICE_TYPE_ST7567;
+
 	return 0;
 }
 
@@ -710,6 +948,52 @@ static int st7571_parse_dt(struct st7571_device *st7571)
 		return dev_err_probe(dev, PTR_ERR(st7571->reset),
 				     "Failed to get reset gpio\n");
 
+	st7571->device_type = DEVICE_TYPE_ST7571;
+
+	return 0;
+}
+
+static int st75156_parse_dt(struct st7571_device *st75156)
+{
+	struct device *dev = st75156->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", &st75156->width_mm);
+	of_property_read_u32(np, "height-mm", &st75156->height_mm);
+	st75156->inverted = of_property_read_bool(np, "sitronix,inverted");
+
+	st75156->pformat = &st75156_monochrome;
+	st75156->bpp = 1;
+
+	st75156->startline = dt.vback_porch.typ;
+	st75156->startcol = dt.hback_porch.typ;
+
+	st75156->nlines = dt.vactive.typ;
+	st75156->ncols = dt.hactive.typ;
+
+	st75156->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+
+	if (IS_ERR(st75156->reset)) {
+		return dev_err_probe(dev, PTR_ERR(st75156->reset),
+				     "Failed to get reset gpio from DT\n");
+	}
+
+	st75156->device_type = DEVICE_TYPE_ST75156;
+
+	st75156->backlight = devm_of_find_backlight(dev);
+	if (IS_ERR(st75156->backlight))
+		return dev_err_probe(dev, PTR_ERR(st75156->backlight),
+			"Failed to get backlight from DT\n");
+
 	return 0;
 }
 
@@ -720,6 +1004,14 @@ static void st7571_reset(struct st7571_device *st7571)
 	gpiod_set_value_cansleep(st7571->reset, 0);
 }
 
+static void st75156_reset(struct st7571_device *st7571)
+{
+	gpiod_set_value_cansleep(st7571->reset, 0);
+	usleep_range(20, 10000);
+	gpiod_set_value_cansleep(st7571->reset, 1);
+	msleep(120);
+}
+
 static int st7567_lcd_init(struct st7571_device *st7567)
 {
 	/*
@@ -799,8 +1091,86 @@ static int st7571_lcd_init(struct st7571_device *st7571)
 	return st7571_send_command_list(st7571, commands, ARRAY_SIZE(commands));
 }
 
-struct st7571_device *st7571_probe(struct device *dev,
-				   struct regmap *regmap)
+static int st75156_lcd_init(struct st7571_device *st75156)
+{
+	int ret;
+
+	/* Each command in the command list is formatted using the following format:
+	 *
+	 * +---------+--------------------------+-------------+------+-------------+
+	 * | command | number of parameters (n) | parameter 1 | .... | parameter n |
+	 * +---------+--------------------------+-------------+------+-------------+
+	 *
+	 * For example, if a command has 2 parameters it is stored like this:
+	 *
+	 * +---------+------+-------------+--------------+
+	 * | command | 0x02 | parameter 1 |  parameter 2 |
+	 * +---------+------+-------------+--------------+
+	 *
+	 * If a command does not have any parameters it is stored like this:
+	 *
+	 * +---------+------+
+	 * | command | 0x00 |
+	 * +---------+------+
+	 *
+	 * NOTE: The command ST75156_SLEEP_MS (0x00) is a special command defined like this:
+	 *
+	 * +------------------+------+----+
+	 * | ST75156_SLEEP_MS | 0x01 | ms |
+	 * +------------------+------+----+
+	 *
+	 * When this command is sent, the code will sleep for ms milliseconds before
+	 * sending the next command in the command list.
+	 */
+
+	u8 commands[] = {
+		ST75156_EXTENSION_COMMAND_2, 0x00,
+		ST75156_SET_AUTO_READ_INSTRUCTION, 0x01, ST75156_DISABLE_AUTO_READ,
+		ST75156_OTP_WR_RD_CONTROL, 0x01, 0x10,
+		ST75156_SLEEP_MS, 0x01, 0x0a,
+		ST75156_OTP_UPLOAD, 0x00,
+		ST75156_SLEEP_MS, 0x01, 0x14,
+		ST75156_OTP_CONTROL_OUT, 0x00,
+		ST75156_EXTENSION_COMMAND_1, 0x00,
+		ST75156_SLEEP_OUT, 0x00,
+		ST75156_DISPLAY_OFF, 0x00,
+		ST75156_SLEEP_MS, 0x01, 0x32,
+		ST75156_POWER_CONTROL, 0x01, 0x0b,
+		ST75156_SLEEP_MS, 0x01, 0x64,
+		ST75156_SET_VOP, 0x02, 0x06, 0x03,
+		ST75156_EXTENSION_COMMAND_2, 0x00,
+		ST75156_ANALOG_CIRCUIT_SET, 0x03, 0x00, 0x01, 0x05,
+		ST75156_BOOSTER_LEVEL_X10, 0x01, 0xfb,
+		ST75156_EXTENSION_COMMAND_1, 0x00,
+		ST75156_DISPLAY_MODE, 0x01, 0x10,
+		ST75156_DISPLAY_CONTROL, 0x03, 0x00, 0x3f, 0x00,
+		ST75156_DATA_SCAN_DIRECTION, 0x01, 0x02,
+		ST75156_NORMAL_DISPLAY, 0x00,
+		ST75156_EXTENSION_COMMAND_2, 0x00,
+		ST75156_INTERAL_POWER_SUPPLY, 0x00,
+		ST75156_EXTENSION_COMMAND_1, 0x00,
+		ST75156_COLUMN_ADDRESS_SETTING, 0x02, 0x00, 0xef,
+		ST75156_PAGE_ADDRESS_SETTING, 0x02, 0x00, 0x07,
+		ST75156_DISPLAY_ON, 0x00,
+	};
+
+	/* Perform a reset before initializing the controller */
+	st75156_reset(st75156);
+
+	ret = st75156_send_command_list(st75156, commands, ARRAY_SIZE(commands));
+	if (ret < 0)
+		return ret;
+
+	/* Clear the screen after the controller has been initialized */
+	st75156_fb_clear_screen(st75156);
+
+	/* Enable backlight */
+	backlight_enable(st75156->backlight);
+
+	return 0;
+}
+
+struct st7571_device *st7571_probe(struct device *dev, struct regmap *regmap)
 {
 	struct st7571_device *st7571;
 	struct drm_device *drm;
@@ -832,12 +1202,22 @@ struct st7571_device *st7571_probe(struct device *dev,
 	if (!st7571->hwbuf)
 		return ERR_PTR(-ENOMEM);
 
+	memset(st7571->hwbuf, 0,  (st7571->nlines * st7571->ncols * st7571->bpp) / 8);
+
 	st7571->row = devm_kzalloc(st7571->dev,
 				   (st7571->ncols * st7571->bpp),
 				   GFP_KERNEL);
 	if (!st7571->row)
 		return ERR_PTR(-ENOMEM);
 
+	memset(st7571->row, 0, (st7571->ncols * st7571->bpp));
+
+	st7571->commandbuf = devm_kzalloc(st7571->dev,
+					  ST75156_COMMAND_BUFFER_LEN,
+							GFP_KERNEL);
+	if (!st7571->commandbuf)
+		return ERR_PTR(-ENOMEM);
+
 	ret = st7571_mode_config_init(st7571);
 	if (ret) {
 		dev_err(st7571->dev, "Failed to initialize mode config\n");
@@ -868,6 +1248,18 @@ struct st7571_device *st7571_probe(struct device *dev,
 		return ERR_PTR(ret);
 	}
 
+	if (st7571->device_type == DEVICE_TYPE_ST75156) {
+		/* ST75156 needs init function to be run earlier than ST7571
+		 * so we need to call it in the probe function instead of
+		 * the encoder_atomic_enable function.
+		 */
+		ret = st7571->pdata->init(st7571);
+		if (ret) {
+			dev_err(st7571->dev, "Failed to init display\n");
+			return ERR_PTR(ret);
+		}
+	}
+
 	drm_mode_config_reset(drm);
 
 	ret = drm_dev_register(drm, 0);
@@ -890,6 +1282,7 @@ EXPORT_SYMBOL_GPL(st7571_remove);
 const struct st7571_panel_data st7567_config = {
 	.init = st7567_lcd_init,
 	.parse_dt = st7567_parse_dt,
+	.fb_clear_screen = st7571_fb_clear_screen,
 	.constraints = {
 		.min_nlines = 1,
 		.max_nlines = 64,
@@ -903,6 +1296,7 @@ EXPORT_SYMBOL_NS_GPL(st7567_config, "DRM_ST7571");
 const struct st7571_panel_data st7571_config = {
 	.init = st7571_lcd_init,
 	.parse_dt = st7571_parse_dt,
+	.fb_clear_screen = st7571_fb_clear_screen,
 	.constraints = {
 		.min_nlines = 1,
 		.max_nlines = 128,
@@ -913,6 +1307,20 @@ const struct st7571_panel_data st7571_config = {
 };
 EXPORT_SYMBOL_NS_GPL(st7571_config, "DRM_ST7571");
 
+const struct st7571_panel_data st75156_config = {
+	.init = st75156_lcd_init,
+	.parse_dt = st75156_parse_dt,
+	.fb_clear_screen = st75156_fb_clear_screen,
+	.constraints = {
+		.min_nlines = 1,
+		.max_nlines = 64,
+		.min_ncols = 240,
+		.max_ncols = 240,
+		.support_grayscale = false,
+	},
+};
+EXPORT_SYMBOL_NS_GPL(st75156_config, "DRM_ST7571");
+
 MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@gmail.com>");
 MODULE_DESCRIPTION("DRM Driver for Sitronix ST7571 LCD controller");
 MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/sitronix/st7571.h b/drivers/gpu/drm/sitronix/st7571.h
index af264f2e2ea4..6e3e70f12c2d 100644
--- a/drivers/gpu/drm/sitronix/st7571.h
+++ b/drivers/gpu/drm/sitronix/st7571.h
@@ -17,11 +17,20 @@
 
 #include <linux/regmap.h>
 
+#define ST7571_COMMAND_MODE			(0x00)
+#define ST7571_DATA_MODE			(0x40)
+
 enum st7571_color_mode {
 	ST7571_COLOR_MODE_GRAY = 0,
 	ST7571_COLOR_MODE_BLACKWHITE = 1,
 };
 
+enum DeviceType {
+	DEVICE_TYPE_ST7567,
+	DEVICE_TYPE_ST7571,
+	DEVICE_TYPE_ST75156
+};
+
 struct st7571_device;
 
 struct st7571_panel_constraints {
@@ -35,6 +44,7 @@ struct st7571_panel_constraints {
 struct st7571_panel_data {
 	int (*init)(struct st7571_device *st7571);
 	int (*parse_dt)(struct st7571_device *st7571);
+	int (*fb_clear_screen)(struct st7571_device *st7571);
 	struct st7571_panel_constraints constraints;
 };
 
@@ -65,12 +75,15 @@ struct st7571_device {
 	const struct st7571_panel_data *pdata;
 	struct gpio_desc *reset;
 	struct regmap *regmap;
+	struct spi_device_data *spi_data;
+	struct backlight_device *backlight;
 
 	bool grayscale;
 	bool inverted;
 	u32 height_mm;
 	u32 width_mm;
 	u32 startline;
+	u32 startcol;
 	u32 nlines;
 	u32 ncols;
 	u32 bpp;
@@ -80,10 +93,17 @@ struct st7571_device {
 
 	/* Row of (transformed) pixels ready to be written to the display */
 	u8 *row;
+
+	/* Buffer containing current command and parameters */
+	u8 *commandbuf;
+
+	/* Type of device */
+	enum DeviceType device_type;
 };
 
 extern const struct st7571_panel_data st7567_config;
 extern const struct st7571_panel_data st7571_config;
+extern const struct st7571_panel_data st75156_config;
 
 struct st7571_device *st7571_probe(struct device *dev, struct regmap *regmap);
 void st7571_remove(struct st7571_device *st7571);

-- 
2.43.0

[PATCH 2/2] dt-bindings: display: sitronix,st75156: add example for SPI

[Lars Rask]

From: Lars Rask <lars.rask@axis.com>

Add example for using st75156 with SPI.

Signed-off-by: Lars Rask <lars.rask@axis.com>
---
 .../bindings/display/sitronix,st75156.yaml         | 67 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 68 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/sitronix,st75156.yaml b/Documentation/devicetree/bindings/display/sitronix,st75156.yaml
new file mode 100644
index 000000000000..a3897ddc5e38
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/sitronix,st75156.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/sitronix,st75156.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sitronix ST75156 Display Controller
+
+maintainers:
+  - Marcus Folkesson <marcus.folkesson@gmail.com>
+
+description:
+  Sitronix ST75156 is a monochrome dot matrix LCD panel used in SPI 4-wire mode.
+
+allOf:
+  - $ref: panel/panel-common.yaml#
+
+properties:
+  compatible:
+    const: sitronix,st75156
+
+  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>
+
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      display@0 {
+        compatible = "sitronix,st75156";
+        reg = <0>;
+        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>;
+        };
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 8014b9f8253e..9219916d8876 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8432,6 +8432,7 @@ F:	drivers/gpu/drm/sitronix/st7586.c
 DRM DRIVER FOR SITRONIX ST7571 PANELS
 M:	Marcus Folkesson <marcus.folkesson@gmail.com>
 S:	Maintained
+F:	Documentation/devicetree/bindings/display/sitronix,st75156.yaml
 F:	Documentation/devicetree/bindings/display/sitronix,st7567.yaml
 F:	Documentation/devicetree/bindings/display/sitronix,st7571.yaml
 F:	drivers/gpu/drm/sitronix/st7571-i2c.c

-- 
2.43.0

Re: [PATCH 1/2] drm/sitronix/st7571-spi: Add support for ST75156 controller

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] drm/sitronix: hwbuf OOB read and broken damage in st75156_transform_xy()
- [High] drm/sitronix: NULL deref and uninit regmap in st7571_spi_probe()
- [High] drm/sitronix: unchecked kmemdup() for databuf in regmap_spi_write()
- [High] drm/sitronix: devm_gpiod_get() swallows errors in st7571_spi_probe()
- [High] drm/sitronix: inverted reset GPIO polarity in st75156_reset()
- [Medium] drm/sitronix: missing BACKLIGHT_CLASS_DEVICE dependency for st7571
- [Medium] drm/sitronix: broken display and backlight PM for ST75156
- [Medium] drm/sitronix: st75156 ignores parsed DT settings and skips startcol check
--

commit c09138c5fe8d0e3240997fcbdd65bfabecbad95f
Author: Lars Rask <lars.rask@axis.com>

drm/sitronix/st7571-spi: Add support for ST75156 controller

This commit adds support for the Sitronix ST75156 monochrome display
controller to the st7571 DRM driver, including 4-line SPI D/C GPIO handling in
st7571-spi.c and ST75156 initialization and framebuffer update routines in
st7571.c.

> diff --git a/drivers/gpu/drm/sitronix/st7571-spi.c b/drivers/gpu/drm/sitronix/st7571-spi.c
> index 0206e9162f1cd..724c0eb5e7dab 100644
> --- a/drivers/gpu/drm/sitronix/st7571-spi.c
> +++ b/drivers/gpu/drm/sitronix/st7571-spi.c
> @@ -17,23 +17,133 @@ static const struct regmap_config st7571_spi_regmap_config = {

[ ... ]

> +static int regmap_spi_write(void *context, const void *data, size_t count)
> +{
> +	struct spi_device_data *spi_data = context;
> +	const u8 *buf = data;
> +	u8 mode;
> +	size_t num_data_bytes;
> +	const u8 *data_bytes;
> +	int write_result;
> +	const u8 *cmdbuf;
> +	const u8 *databuf;

[ ... ]

> +	if (mode == ST7571_COMMAND_MODE) {
> +		gpiod_set_value_cansleep(spi_data->dc_gpio, 0);
> +
> +		cmdbuf = kmemdup(&buf[1], 1, GFP_KERNEL);
> +		if (!cmdbuf)
> +			return -ENOMEM;
> +
> +		write_result = spi_write(spi_data->spi, cmdbuf, 1);
> +		kfree(cmdbuf);
> +
> +		if (write_result != 0)
> +			return write_result;
> +
> +		num_data_bytes = count - 2;
> +
> +		if (num_data_bytes > 0) {
> +			gpiod_set_value_cansleep(spi_data->dc_gpio, 1);
> +			data_bytes = &buf[2];
> +			databuf = kmemdup(data_bytes, num_data_bytes, GFP_KERNEL);
> +
> +			write_result = spi_write(spi_data->spi, databuf, num_data_bytes);
> +			kfree(databuf);
> +
> +			if (write_result != 0)
> +				return write_result;
> +		}
> +	} else if (mode == ST7571_DATA_MODE) {
> +		num_data_bytes = count - 1;
> +
> +		if (num_data_bytes > 0) {
> +			gpiod_set_value_cansleep(spi_data->dc_gpio, 1);
> +			data_bytes = &buf[1];
> +			databuf = kmemdup(data_bytes, num_data_bytes, GFP_KERNEL);
> +			write_result = spi_write(spi_data->spi, databuf, num_data_bytes);
> +			kfree(databuf);
> +
> +			if (write_result != 0)
> +				return write_result;
> +		}

[Severity: High]
What happens in regmap_spi_write() in drivers/gpu/drm/sitronix/st7571-spi.c if
kmemdup() returns NULL when allocating databuf?

In both the ST7571_COMMAND_MODE branch (when num_data_bytes > 0) and the
ST7571_DATA_MODE branch (when num_data_bytes > 0), databuf is passed directly
to spi_write(spi_data->spi, databuf, num_data_bytes) without a NULL check,
unlike cmdbuf above. If kmemdup() fails under memory pressure during a
multi-byte command write or framebuffer update over 4-line SPI, spi_write()
receives a NULL tx_buf with a non-zero length, which can cause a NULL pointer
dereference in the SPI controller driver or transmit dummy bytes instead of
returning -ENOMEM.

Should both databuf allocations check for !databuf and return -ENOMEM before
calling spi_write()?

> +
> +	} else {
> +		/* Unsupported mode */
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

[ ... ]

>  static int st7571_spi_probe(struct spi_device *spi)
>  {
>  	struct st7571_device *st7571;
>  	struct regmap *regmap;
> +	struct spi_device_data *spi_data;
> +
> +	spi_data = devm_kzalloc(&spi->dev, sizeof(*spi_data), GFP_KERNEL);
> +	if (IS_ERR(spi_data))
> +		return dev_err_probe(&spi->dev, PTR_ERR(regmap),
> +				     "Failed to initialize spi data\n");
>  
> -	regmap = devm_regmap_init_spi(spi, &st7571_spi_regmap_config);
> -	if (IS_ERR(regmap)) {
> +	spi_data->spi = spi;

[Severity: High]
In st7571_spi_probe() in drivers/gpu/drm/sitronix/st7571-spi.c, devm_kzalloc()
returns NULL on allocation failure rather than an error pointer, and regmap is
uninitialized at this point.

When devm_kzalloc() fails, IS_ERR(spi_data) evaluates to false and execution
continues to spi_data->spi = spi, causing a NULL pointer dereference. In
addition, passing PTR_ERR(regmap) to dev_err_probe() reads the uninitialized
regmap variable.

Should this check for !spi_data and return -ENOMEM instead?

> +	spi_data->dc_gpio = devm_gpiod_get(&spi->dev, "dc", GPIOD_OUT_LOW);
> +
> +	if (IS_ERR(spi_data->dc_gpio))
> +		/* init regmap for 3 line SPI */
> +		regmap = devm_regmap_init_spi(spi, &st7571_spi_regmap_config);
> +	else
> +		/* init regmap for 4 line SPI */
> +		regmap = devm_regmap_init(&spi->dev, &regmap_spi_dc_bus,
> +					  spi_data, &st7571_spi_regmap_config);

[Severity: High]
Could checking IS_ERR(spi_data->dc_gpio) after devm_gpiod_get() in
st7571_spi_probe() in drivers/gpu/drm/sitronix/st7571-spi.c break probe
deferral for 4-line SPI panels?

If a 4-line SPI panel specifies dc-gpios in Device Tree and the GPIO provider
is not ready yet, devm_gpiod_get() returns ERR_PTR(-EPROBE_DEFER). Because any
IS_ERR(spi_data->dc_gpio) result falls back to devm_regmap_init_spi(), the
driver silently ignores -EPROBE_DEFER (and any other GPIO error) and
configures the 4-line SPI panel as a 3-line SPI device without driving its D/C
pin.

Would it be better to use devm_gpiod_get_optional(&spi->dev, "dc",
GPIOD_OUT_LOW), propagate IS_ERR(spi_data->dc_gpio) via dev_err_probe(), and
select 3-line vs 4-line SPI based on whether spi_data->dc_gpio is NULL?

[ ... ]

> diff --git a/drivers/gpu/drm/sitronix/st7571.c b/drivers/gpu/drm/sitronix/st7571.c
> index 20954c33eca93..2dd52f2214d28 100644
> --- a/drivers/gpu/drm/sitronix/st7571.c
> +++ b/drivers/gpu/drm/sitronix/st7571.c

[ ... ]

> +static inline u8 st75156_transform_xy(struct st7571_device *st75156, int x, int y)
> +{
> +	int xrest = x % 8;
> +	u8 result = 0;
> +	u8 row_len = (st75156->ncols / 8) * st75156->bpp;
> +	int i;
> +	const char *p = st75156->hwbuf;
> +
> +	/*
> +	 * 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 (i = 0; i < 8; i++) {
> +		int row_idx = y + i;
> +		u8 byte = p[row_idx * row_len + x];
> +		/* LSB in input is left most pixel */
> +		u8 bit = (byte >> xrest) & 1;
> +		/* LSB in output is 7th row */
> +		result |= (bit << (7 - i));
> +	}
> +
> +	return result;
> +}
> +
>  static int st7571_set_position(struct st7571_device *st7571, int x, int y)
>  {
>  	u8 cmd_list[] = {
> @@ -138,6 +290,23 @@ static int st7571_set_position(struct st7571_device *st7571, int x, int y)
>  	return st7571_send_command_list(st7571, cmd_list, ARRAY_SIZE(cmd_list));
>  }
>  
> +static int st75156_set_position(struct st7571_device *st75156, int x, int y)
> +{
> +	u8 xs_param = st75156->startcol + x;
> +	u8 xe_param = st75156->startcol + st75156->ncols - 1;
> +	u8 ys_param = y / ST7571_PAGE_HEIGHT;
> +	u8 ye_param = ys_param;
> +
> +	u8 cmd_list[] = {
> +		ST75156_EXTENSION_COMMAND_1, 0x00,
> +		ST75156_COLUMN_ADDRESS_SETTING, 0x02, xs_param, xe_param,
> +		ST75156_PAGE_ADDRESS_SETTING, 0x02, ys_param, ye_param,
> +		ST75156_ENTER_WRITE_DDRAM_MODE, 0x00,
> +	};
> +
> +	return st75156_send_command_list(st75156, cmd_list, ARRAY_SIZE(cmd_list));
> +}

[ ... ]

> @@ -284,6 +468,37 @@ static int st7571_fb_update_rect_grayscale(struct drm_framebuffer *fb, struct dr
>  	return 0;
>  }
>  
> +static int st75156_fb_update_rect_monochrome(struct drm_framebuffer *fb, struct drm_rect *rect)
> +{
> +	struct st7571_device *st75156 = drm_to_st7571(fb->dev);
> +	struct device *dev = st75156->dev;
> +
> +	char *row = st75156->row;
> +	int x;
> +	int y;
> +	int num_bytes;
> +	int ret;
> +
> +	/* Align y to display page boundaries */
> +	rect->y1 = round_down(rect->y1, ST7571_PAGE_HEIGHT);
> +	rect->y2 = min_t(unsigned int, round_up(rect->y2, ST7571_PAGE_HEIGHT), st75156->nlines);
> +
> +	for (y = rect->y1; y < rect->y2; y += ST7571_PAGE_HEIGHT) {
> +		for (x = rect->x1; x < rect->x2; x++)
> +			row[x] = st75156_transform_xy(st75156, x, y);
> +
> +		st75156_set_position(st75156, rect->x1, y);
> +
> +		num_bytes = (rect->x2) - (rect->x1);
> +		ret = regmap_bulk_write(st75156->regmap, ST7571_DATA_MODE,
> +					row + rect->x1, num_bytes);
> +		if (ret < 0)
> +			dev_err(dev, "Failed to write data to display (error code: %i", ret);
> +	}
> +
> +	return 0;
> +}

[Severity: Critical]
Can st75156_transform_xy() in drivers/gpu/drm/sitronix/st7571.c read past the
end of st75156->hwbuf and read from the wrong offsets on partial damage
updates?

First, st75156_config allows nlines between 1 and 64, and st7571_probe()
allocates hwbuf for exactly nlines rows:

	st7571->hwbuf = devm_kzalloc(st7571->dev,
				     (st7571->nlines * st7571->ncols * st7571->bpp) / 8,
				     GFP_KERNEL);

In st75156_fb_update_rect_monochrome(), y advances by ST7571_PAGE_HEIGHT (8)
up to rect->y2 (clamped to st75156->nlines), and st75156_transform_xy()
unconditionally reads 8 rows at p[row_idx * row_len + x] for i = 0..7. When
st75156->nlines is not a multiple of 8, the final page iteration reads rows
st75156->nlines through round_up(st75156->nlines, 8) - 1, which is an
out-of-bounds heap read past st75156->hwbuf.

Second, in st7571_primary_plane_helper_atomic_update():

	drm_atomic_for_each_plane_damage(&iter, &damage) {
		st7571->pformat->prepare_buffer(st7571,
						&shadow_plane_state->data[0],
						fb, &damage,
						&shadow_plane_state->fmtcnv_state);

		st7571->pformat->update_rect(fb, &damage);
	}

st7571_prepare_buffer_monochrome() writes only the unaligned damage clip to
the beginning of st7571->hwbuf using dst_pitch =
DIV_ROUND_UP(drm_rect_width(rect), 8) before st75156_fb_update_rect_monochrome()
rounds rect->y1 and rect->y2 to multiples of 8. Then st75156_transform_xy()
indexes st75156->hwbuf using the full-screen stride row_len =
(st75156->ncols / 8) * st75156->bpp and absolute coordinates (x, y), reading
stale or uninitialized bytes from hwbuf whenever the damage clip is smaller
than the full screen or not aligned to page boundaries.

[ ... ]

> @@ -444,6 +670,13 @@ static void st7571_encoder_atomic_enable(struct drm_encoder *encoder,
>  	u8 command = ST7571_DISPLAY_ON;
>  	int ret;
>  
> +	/* ST75156 needs init function to be run earlier than ST7571
> +	 * so we need to call it in the probe function instead of
> +	 * the encoder_atomic_enable function.
> +	 */
> +	if (st7571->device_type == DEVICE_TYPE_ST75156)
> +		return;
> +
>  	ret = st7571->pdata->init(st7571);
>  	if (ret)
>  		return;
> @@ -458,6 +691,9 @@ static void st7571_encoder_atomic_disable(struct drm_encoder *encoder,
>  	struct st7571_device *st7571 = drm_to_st7571(drm);
>  	u8 command = ST7571_DISPLAY_OFF;
>  
> +	if (st7571->device_type == DEVICE_TYPE_ST75156)
> +		return;
> +
>  	st7571_send_command_list(st7571, &command, 1);
>  }

[Severity: Medium]
Why do st7571_encoder_atomic_enable() and st7571_encoder_atomic_disable() in
drivers/gpu/drm/sitronix/st7571.c return early for DEVICE_TYPE_ST75156 while
st7571_probe() runs st7571->pdata->init(st7571) before drm_dev_register()?

In st75156_lcd_init(), the driver sends ST75156_DISPLAY_ON and calls
backlight_enable(st75156->backlight) during st7571_probe(). Because both
atomic encoder callbacks bail out for DEVICE_TYPE_ST75156 and
backlight_disable() is never called anywhere in the driver:

1. The panel and backlight are turned on before drm_dev_register() and remain
   enabled if drm_dev_register() fails or when the driver is unbound.
2. Blanking, DPMS off, or disabling the CRTC/encoder never sends
   ST75156_DISPLAY_OFF or disables the backlight, and subsequent atomic
   enables cannot turn the display and backlight back on.

Could the panel power and backlight state be managed in the atomic enable and
disable callbacks instead?

[ ... ]

> @@ -710,6 +948,52 @@ static int st7571_parse_dt(struct st7571_device *st7571)
>  		return dev_err_probe(dev, PTR_ERR(st7571->reset),
>  				     "Failed to get reset gpio\n");
>  
> +	st7571->device_type = DEVICE_TYPE_ST7571;
> +
> +	return 0;
> +}
> +
> +static int st75156_parse_dt(struct st7571_device *st75156)
> +{

[ ... ]

> +	st75156->device_type = DEVICE_TYPE_ST75156;
> +
> +	st75156->backlight = devm_of_find_backlight(dev);
> +	if (IS_ERR(st75156->backlight))
> +		return dev_err_probe(dev, PTR_ERR(st75156->backlight),
> +			"Failed to get backlight from DT\n");
> +
>  	return 0;
>  }

[Severity: Medium]
Does DRM_ST7571 in drivers/gpu/drm/sitronix/Kconfig need to select
BACKLIGHT_CLASS_DEVICE now that st75156_parse_dt() and st75156_lcd_init() in
drivers/gpu/drm/sitronix/st7571.c call devm_of_find_backlight() and
backlight_enable()?

In include/linux/backlight.h, devm_of_find_backlight() is declared when
IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) is true. If CONFIG_DRM_ST7571=y and
CONFIG_BACKLIGHT_CLASS_DEVICE=m, linking the built-in st7571 driver will fail
with an undefined reference to devm_of_find_backlight.

>  
> @@ -720,6 +1004,14 @@ static void st7571_reset(struct st7571_device *st7571)
>  	gpiod_set_value_cansleep(st7571->reset, 0);
>  }
>  
> +static void st75156_reset(struct st7571_device *st7571)
> +{
> +	gpiod_set_value_cansleep(st7571->reset, 0);
> +	usleep_range(20, 10000);
> +	gpiod_set_value_cansleep(st7571->reset, 1);
> +	msleep(120);
> +}

[Severity: High]
Is the reset GPIO polarity inverted in st75156_reset() in
drivers/gpu/drm/sitronix/st7571.c?

st75156_parse_dt() requests the reset line with devm_gpiod_get(dev, "reset",
GPIOD_OUT_HIGH), and gpiod_set_value_cansleep() operates on logical line state
(1 = asserted/active, 0 = deasserted/inactive). Unlike st7571_reset() right
above, which sets 1 and then 0, st75156_reset() sets 0 and then leaves the
reset GPIO at logical 1. When reset-gpios is configured as GPIO_ACTIVE_LOW in
Device Tree, ending with gpiod_set_value_cansleep(st7571->reset, 1) drives the
physical pin low and leaves the ST75156 controller held in hardware reset.

Should st75156_reset() assert reset with 1 and deassert it with 0?

[ ... ]

> +static int st75156_lcd_init(struct st7571_device *st75156)
> +{
> +	int ret;

[ ... ]

> +	u8 commands[] = {
> +		ST75156_EXTENSION_COMMAND_2, 0x00,
> +		ST75156_SET_AUTO_READ_INSTRUCTION, 0x01, ST75156_DISABLE_AUTO_READ,
> +		ST75156_OTP_WR_RD_CONTROL, 0x01, 0x10,
> +		ST75156_SLEEP_MS, 0x01, 0x0a,
> +		ST75156_OTP_UPLOAD, 0x00,
> +		ST75156_SLEEP_MS, 0x01, 0x14,
> +		ST75156_OTP_CONTROL_OUT, 0x00,
> +		ST75156_EXTENSION_COMMAND_1, 0x00,
> +		ST75156_SLEEP_OUT, 0x00,
> +		ST75156_DISPLAY_OFF, 0x00,
> +		ST75156_SLEEP_MS, 0x01, 0x32,
> +		ST75156_POWER_CONTROL, 0x01, 0x0b,
> +		ST75156_SLEEP_MS, 0x01, 0x64,
> +		ST75156_SET_VOP, 0x02, 0x06, 0x03,
> +		ST75156_EXTENSION_COMMAND_2, 0x00,
> +		ST75156_ANALOG_CIRCUIT_SET, 0x03, 0x00, 0x01, 0x05,
> +		ST75156_BOOSTER_LEVEL_X10, 0x01, 0xfb,
> +		ST75156_EXTENSION_COMMAND_1, 0x00,
> +		ST75156_DISPLAY_MODE, 0x01, 0x10,
> +		ST75156_DISPLAY_CONTROL, 0x03, 0x00, 0x3f, 0x00,
> +		ST75156_DATA_SCAN_DIRECTION, 0x01, 0x02,
> +		ST75156_NORMAL_DISPLAY, 0x00,
> +		ST75156_EXTENSION_COMMAND_2, 0x00,
> +		ST75156_INTERAL_POWER_SUPPLY, 0x00,
> +		ST75156_EXTENSION_COMMAND_1, 0x00,
> +		ST75156_COLUMN_ADDRESS_SETTING, 0x02, 0x00, 0xef,
> +		ST75156_PAGE_ADDRESS_SETTING, 0x02, 0x00, 0x07,
> +		ST75156_DISPLAY_ON, 0x00,
> +	};

[Severity: Medium]
In drivers/gpu/drm/sitronix/st7571.c, st75156_parse_dt() parses
st75156->inverted, st75156->startline (from dt.vback_porch.typ),
st75156->startcol (from dt.hback_porch.typ), and st75156->nlines (from
dt.vactive.typ), but are these parameters wired up and validated for ST75156?

1. st75156_lcd_init() hardcodes ST75156_NORMAL_DISPLAY (0xa6),
   ST75156_DISPLAY_CONTROL (0x00, 0x3f, 0x00), ST75156_COLUMN_ADDRESS_SETTING
   (0x00, 0xef), and ST75156_PAGE_ADDRESS_SETTING (0x00, 0x07), ignoring
   st75156->inverted, st75156->startline, and st75156->nlines, and
   st75156->startline is never used in st75156_set_position() either.
2. st75156_set_position() computes:

	u8 xs_param = st75156->startcol + x;
	u8 xe_param = st75156->startcol + st75156->ncols - 1;

   However, st7571_validate_parameters() only checks startline + nlines >
   constraints->max_nlines and never checks startcol + ncols >
   constraints->max_ncols. Since st75156_config sets both min_ncols and
   max_ncols to 240, any non-zero startcol will exceed the 240-column
   controller limit (0xef) and overflow u8 when startcol >= 17.

Should st75156_lcd_init() and st75156_set_position() honor the parsed
Device Tree settings, and should st7571_validate_parameters() validate
startcol + ncols against constraints->max_ncols?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-add_st75156_support-v1-0-582525a39a39@axis.com?part=1

Re: [PATCH 2/2] dt-bindings: display: sitronix,st75156: add example for SPI

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] drm/sitronix: st7571-spi: missing kmemdup() NULL checks in regmap_spi_write()
- [High] drm/sitronix: inverted reset GPIO polarity in st75156_reset()
- [Medium] dt-bindings: display: sitronix,st75156: missing SPI and panel properties
- [Medium] dt-bindings: display: sitronix,st75156: example timings fail st7571_probe()
- [Medium] drm/sitronix: st75156_lcd_init() ignores DT inverted and startline
- [Medium] drm/sitronix: st7571: missing display power and backlight control for st75156
- [Low] drm/sitronix: missing BACKLIGHT_CLASS_DEVICE select for DRM_ST7571
--

commit 3557b66cb1cae50421d6b3c371688ed607f5a7dc
Author: Lars Rask <lars.rask@axis.com>

dt-bindings: display: sitronix,st75156: add example for SPI

This series adds support for the Sitronix st75156 display controller to the
st7571 driver and introduces the corresponding device tree binding schema
and spi example.

> diff --git a/Documentation/devicetree/bindings/display/sitronix,st75156.yaml b/Documentation/devicetree/bindings/display/sitronix,st75156.yaml
> new file mode 100644
> index 0000000000000..a3897ddc5e386
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/sitronix,st75156.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/sitronix,st75156.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sitronix ST75156 Display Controller
> +
> +maintainers:
> +  - Marcus Folkesson <marcus.folkesson@gmail.com>
> +
> +description:
> +  Sitronix ST75156 is a monochrome dot matrix LCD panel used in SPI 4-wire mode.
> +
> +allOf:
> +  - $ref: panel/panel-common.yaml#
> +
> +properties:
> +  compatible:
> +    const: sitronix,st75156
> +
> +  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

[Severity: Medium]
In Documentation/devicetree/bindings/display/sitronix,st75156.yaml, the
schema sets additionalProperties: false and only references
panel/panel-common.yaml#, without referencing
/schemas/spi/spi-peripheral-props.yaml# (with unevaluatedProperties: false)
or listing dc-gpios, backlight, and sitronix,inverted under properties.

In drivers/gpu/drm/sitronix/st7571-spi.c, st7571_spi_probe() requests the
data/command gpio to enable 4-wire spi mode:

	spi_data->dc_gpio = devm_gpiod_get(&spi->dev, "dc", GPIOD_OUT_LOW);

And in drivers/gpu/drm/sitronix/st7571.c, st75156_parse_dt() parses
sitronix,inverted and backlight from the device tree node:

	st75156->inverted = of_property_read_bool(np, "sitronix,inverted");
	...
	st75156->backlight = devm_of_find_backlight(dev);

Won't dtbs_check fail validation on any st75156 spi node that specifies
dc-gpios, spi-max-frequency, backlight, or sitronix,inverted, and won't a
device tree based on the example below (without dc-gpios) fall back to
3-wire spi mode in st7571_spi_probe() instead of 4-wire spi mode?

> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      display@0 {
> +        compatible = "sitronix,st75156";
> +        reg = <0>;
> +        reset-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>;
> +        width-mm = <37>;
> +        height-mm = <27>;
> +
> +        panel-timing {
> +          hactive = <128>;
> +          vactive = <96>;

[Severity: Medium]
When a device tree node uses the panel-timing values from the examples block
in Documentation/devicetree/bindings/display/sitronix,st75156.yaml,
st7571_probe() -> st75156_parse_dt() sets st75156->ncols to 128 and
st75156->nlines to 96.

Then st7571_probe() -> st7571_validate_parameters() checks those dimensions
against st75156_config in drivers/gpu/drm/sitronix/st7571.c:

	const struct st7571_panel_data st75156_config = {
		.init = st75156_lcd_init,
		.parse_dt = st75156_parse_dt,
		.fb_clear_screen = st75156_fb_clear_screen,
		.constraints = {
			.min_nlines = 1,
			.max_nlines = 64,
			.min_ncols = 240,
			.max_ncols = 240,
			.support_grayscale = false,
		},
	};

Since hactive = <128> and vactive = <96> are outside the allowed 240 columns
and 1..64 lines, won't st7571_probe() reject the example timings with
-EINVAL?

[ ... ]

> diff --git a/drivers/gpu/drm/sitronix/st7571-spi.c b/drivers/gpu/drm/sitronix/st7571-spi.c
> index 0206e9162f1cd..724c0eb5e7dab 100644
> --- a/drivers/gpu/drm/sitronix/st7571-spi.c
> +++ b/drivers/gpu/drm/sitronix/st7571-spi.c
> @@ -17,23 +17,133 @@ static const struct regmap_config st7571_spi_regmap_config = {

[ ... ]

> +	if (mode == ST7571_COMMAND_MODE) {
> +		gpiod_set_value_cansleep(spi_data->dc_gpio, 0);
> +
> +		cmdbuf = kmemdup(&buf[1], 1, GFP_KERNEL);
> +		if (!cmdbuf)
> +			return -ENOMEM;
> +
> +		write_result = spi_write(spi_data->spi, cmdbuf, 1);
> +		kfree(cmdbuf);
> +
> +		if (write_result != 0)
> +			return write_result;
> +
> +		num_data_bytes = count - 2;
> +
> +		if (num_data_bytes > 0) {
> +			gpiod_set_value_cansleep(spi_data->dc_gpio, 1);
> +			data_bytes = &buf[2];
> +			databuf = kmemdup(data_bytes, num_data_bytes, GFP_KERNEL);
> +
> +			write_result = spi_write(spi_data->spi, databuf, num_data_bytes);
> +			kfree(databuf);
> +
> +			if (write_result != 0)
> +				return write_result;
> +		}
> +	} else if (mode == ST7571_DATA_MODE) {
> +		num_data_bytes = count - 1;
> +
> +		if (num_data_bytes > 0) {
> +			gpiod_set_value_cansleep(spi_data->dc_gpio, 1);
> +			data_bytes = &buf[1];
> +			databuf = kmemdup(data_bytes, num_data_bytes, GFP_KERNEL);
> +			write_result = spi_write(spi_data->spi, databuf, num_data_bytes);
> +			kfree(databuf);

[Severity: High]
In drivers/gpu/drm/sitronix/st7571-spi.c, when regmap_spi_write() is called
from st75156_lcd_init() -> st75156_send_command_list() -> regmap_bulk_write()
or from st75156_fb_update_rect_monochrome() / st75156_fb_clear_screen() ->
regmap_bulk_write(), databuf is allocated with kmemdup() in both the
command-parameter branch and the data-mode branch when num_data_bytes > 0.

Unlike cmdbuf above, neither databuf allocation is checked for null before
passing databuf to spi_write(spi_data->spi, databuf, num_data_bytes).

If kmemdup() fails under memory pressure, spi_write() submits an
spi_transfer with tx_buf = NULL, rx_buf = NULL, and len = num_data_bytes > 0.
Can this silently drop command parameters or display data (when
spi_transfer_one_message() rejects the bufferless transfer and returns 0),
or cause a null pointer dereference in spi controller drivers whose
transfer_one_message() callback dereferences tx_buf when len > 0?

[ ... ]

> diff --git a/drivers/gpu/drm/sitronix/st7571.c b/drivers/gpu/drm/sitronix/st7571.c
> index 20954c33eca93..2dd52f2214d28 100644
> --- a/drivers/gpu/drm/sitronix/st7571.c
> +++ b/drivers/gpu/drm/sitronix/st7571.c

[ ... ]

> @@ -444,6 +670,13 @@ static void st7571_encoder_atomic_enable(struct drm_encoder *encoder,
>  	u8 command = ST7571_DISPLAY_ON;
>  	int ret;
>  
> +	/* ST75156 needs init function to be run earlier than ST7571
> +	 * so we need to call it in the probe function instead of
> +	 * the encoder_atomic_enable function.
> +	 */
> +	if (st7571->device_type == DEVICE_TYPE_ST75156)
> +		return;
> +
>  	ret = st7571->pdata->init(st7571);
>  	if (ret)
>  		return;
> @@ -458,6 +691,9 @@ static void st7571_encoder_atomic_disable(struct drm_encoder *encoder,
>  	struct st7571_device *st7571 = drm_to_st7571(drm);
>  	u8 command = ST7571_DISPLAY_OFF;
>  
> +	if (st7571->device_type == DEVICE_TYPE_ST75156)
> +		return;
> +
>  	st7571_send_command_list(st7571, &command, 1);
>  }

[Severity: Medium]
In drivers/gpu/drm/sitronix/st7571.c, st7571_probe() calls
st75156_lcd_init(), which turns on the display and enables the backlight via
backlight_enable(st75156->backlight).

However, both st7571_encoder_atomic_enable() and
st7571_encoder_atomic_disable() return early when st7571->device_type is
DEVICE_TYPE_ST75156, without sending ST75156_DISPLAY_ON or
ST75156_DISPLAY_OFF or toggling st7571->backlight. Likewise, st7571_remove()
only calls drm_dev_unplug(&st7571->drm) without disabling st7571->backlight:

	void st7571_remove(struct st7571_device *st7571)
	{
		drm_dev_unplug(&st7571->drm);
	}

How are display power and backlight state managed for st75156 during atomic
disable/enable cycles or when the driver is removed?

[ ... ]

> @@ -710,6 +948,52 @@ static int st7571_parse_dt(struct st7571_device *st7571)
>  		return dev_err_probe(dev, PTR_ERR(st7571->reset),
>  				     "Failed to get reset gpio\n");
>  
> +	st7571->device_type = DEVICE_TYPE_ST7571;
> +
> +	return 0;
> +}
> +
> +static int st75156_parse_dt(struct st7571_device *st75156)
> +{
> +	struct device *dev = st75156->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", &st75156->width_mm);
> +	of_property_read_u32(np, "height-mm", &st75156->height_mm);
> +	st75156->inverted = of_property_read_bool(np, "sitronix,inverted");
> +
> +	st75156->pformat = &st75156_monochrome;
> +	st75156->bpp = 1;
> +
> +	st75156->startline = dt.vback_porch.typ;
> +	st75156->startcol = dt.hback_porch.typ;
> +
> +	st75156->nlines = dt.vactive.typ;
> +	st75156->ncols = dt.hactive.typ;
> +
> +	st75156->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +
> +	if (IS_ERR(st75156->reset)) {
> +		return dev_err_probe(dev, PTR_ERR(st75156->reset),
> +				     "Failed to get reset gpio from DT\n");
> +	}
> +
> +	st75156->device_type = DEVICE_TYPE_ST75156;
> +
> +	st75156->backlight = devm_of_find_backlight(dev);
> +	if (IS_ERR(st75156->backlight))
> +		return dev_err_probe(dev, PTR_ERR(st75156->backlight),
> +			"Failed to get backlight from DT\n");
> +
>  	return 0;
>  }

[Severity: Low]
In drivers/gpu/drm/sitronix/st7571.c, st75156_parse_dt() calls
devm_of_find_backlight(dev). In include/linux/backlight.h,
devm_of_find_backlight() is declared as an external function under
#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE):

	#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
	struct backlight_device *devm_of_find_backlight(struct device *dev);
	#else
	static inline struct backlight_device *
	devm_of_find_backlight(struct device *dev)
	{
		return NULL;
	}
	#endif

Because config DRM_ST7571 in drivers/gpu/drm/sitronix/Kconfig does not
select BACKLIGHT_CLASS_DEVICE:

	config DRM_ST7571
		tristate "DRM support for Sitronix ST7567/ST7571 display panels"
		depends on DRM && MMU
		select DRM_CLIENT_SELECTION
		select DRM_GEM_SHMEM_HELPER
		select DRM_KMS_HELPER
		select VIDEOMODE_HELPERS

won't building with CONFIG_DRM_ST7571=y and CONFIG_BACKLIGHT_CLASS_DEVICE=m
fail to link vmlinux with an undefined reference to
devm_of_find_backlight()?

> @@ -720,6 +1004,14 @@ static void st7571_reset(struct st7571_device *st7571)
>  	gpiod_set_value_cansleep(st7571->reset, 0);
>  }
>  
> +static void st75156_reset(struct st7571_device *st7571)
> +{
> +	gpiod_set_value_cansleep(st7571->reset, 0);
> +	usleep_range(20, 10000);
> +	gpiod_set_value_cansleep(st7571->reset, 1);
> +	msleep(120);
> +}

[Severity: High]
When reset-gpios is configured as active-low in the device tree (as shown in
Documentation/devicetree/bindings/display/sitronix,st75156.yaml with
reset-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>;), gpiod_set_value_cansleep() uses
logical values where 1 asserts reset (physical low) and 0 deasserts reset
(physical high).

During st7571_spi_probe() -> st7571_probe() -> st75156_lcd_init() ->
st75156_reset() in drivers/gpu/drm/sitronix/st7571.c, st75156_reset() sets
st7571->reset to 0 and then leaves it at 1, which is the inverse of
st7571_reset() above it.

Won't this leave the st75156 controller held in reset after
st75156_reset() completes?

> @@ -799,8 +1091,86 @@ static int st7571_lcd_init(struct st7571_device *st7571)
>  	return st7571_send_command_list(st7571, commands, ARRAY_SIZE(commands));
>  }
>  
> -struct st7571_device *st7571_probe(struct device *dev,
> -				   struct regmap *regmap)
> +static int st75156_lcd_init(struct st7571_device *st75156)
> +{
> +	int ret;

[ ... ]

> +	u8 commands[] = {
> +		ST75156_EXTENSION_COMMAND_2, 0x00,
> +		ST75156_SET_AUTO_READ_INSTRUCTION, 0x01, ST75156_DISABLE_AUTO_READ,
> +		ST75156_OTP_WR_RD_CONTROL, 0x01, 0x10,
> +		ST75156_SLEEP_MS, 0x01, 0x0a,
> +		ST75156_OTP_UPLOAD, 0x00,
> +		ST75156_SLEEP_MS, 0x01, 0x14,
> +		ST75156_OTP_CONTROL_OUT, 0x00,
> +		ST75156_EXTENSION_COMMAND_1, 0x00,
> +		ST75156_SLEEP_OUT, 0x00,
> +		ST75156_DISPLAY_OFF, 0x00,
> +		ST75156_SLEEP_MS, 0x01, 0x32,
> +		ST75156_POWER_CONTROL, 0x01, 0x0b,
> +		ST75156_SLEEP_MS, 0x01, 0x64,
> +		ST75156_SET_VOP, 0x02, 0x06, 0x03,
> +		ST75156_EXTENSION_COMMAND_2, 0x00,
> +		ST75156_ANALOG_CIRCUIT_SET, 0x03, 0x00, 0x01, 0x05,
> +		ST75156_BOOSTER_LEVEL_X10, 0x01, 0xfb,
> +		ST75156_EXTENSION_COMMAND_1, 0x00,
> +		ST75156_DISPLAY_MODE, 0x01, 0x10,
> +		ST75156_DISPLAY_CONTROL, 0x03, 0x00, 0x3f, 0x00,
> +		ST75156_DATA_SCAN_DIRECTION, 0x01, 0x02,
> +		ST75156_NORMAL_DISPLAY, 0x00,
> +		ST75156_EXTENSION_COMMAND_2, 0x00,
> +		ST75156_INTERAL_POWER_SUPPLY, 0x00,
> +		ST75156_EXTENSION_COMMAND_1, 0x00,
> +		ST75156_COLUMN_ADDRESS_SETTING, 0x02, 0x00, 0xef,
> +		ST75156_PAGE_ADDRESS_SETTING, 0x02, 0x00, 0x07,
> +		ST75156_DISPLAY_ON, 0x00,
> +	};

[Severity: Medium]
In drivers/gpu/drm/sitronix/st7571.c, st75156_parse_dt() parses
st75156->inverted from the sitronix,inverted property and st75156->startline
from dt.vback_porch.typ.

However, unlike st7567_lcd_init() and st7571_lcd_init(), st75156_lcd_init()
hardcodes ST75156_DISPLAY_CONTROL and ST75156_NORMAL_DISPLAY in commands[],
and st75156_set_position() only applies st75156->startcol without using
st75156->startline:

	u8 xs_param = st75156->startcol + x;
	u8 xe_param = st75156->startcol + st75156->ncols - 1;
	u8 ys_param = y / ST7571_PAGE_HEIGHT;
	u8 ye_param = ys_param;

Should st75156_lcd_init() (or st75156_set_position()) apply
st75156->inverted and st75156->startline so that device tree nodes setting
sitronix,inverted or a non-zero vback-porch take effect?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-add_st75156_support-v1-0-582525a39a39@axis.com?part=2

Re: [PATCH 1/2] drm/sitronix/st7571-spi: Add support for ST75156 controller

[kernel test robot]

Hi Lars,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 8d3ae59288f1e7d58d76558a6ee96d533bc5019f]

url:    https://github.com/intel-lab-lkp/linux/commits/Lars-Rask/drm-sitronix-st7571-spi-Add-support-for-ST75156-controller/20261022-173105
base:   8d3ae59288f1e7d58d76558a6ee96d533bc5019f
patch link:    https://lore.kernel.org/r/20261022-add_st75156_support-v1-1-582525a39a39%40axis.com
patch subject: [PATCH 1/2] drm/sitronix/st7571-spi: Add support for ST75156 controller
config: i386-buildonly-randconfig-1001-20261023 (https://download.01.org/0day-ci/archive/20261023/202610231738.0gNssMIn-lkp@intel.com/config)
compiler: clang version 22.1.8 (https://github.com/llvm/llvm-project ca7933e47d3a3451d81e72ac174dcb5aa28b59d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20261023/202610231738.0gNssMIn-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202610231738.0gNssMIn-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/sitronix/st7571-spi.c:121:43: warning: variable 'regmap' is uninitialized when used here [-Wuninitialized]
     121 |                 return dev_err_probe(&spi->dev, PTR_ERR(regmap),
         |                                                         ^~~~~~
   drivers/gpu/drm/sitronix/st7571-spi.c:116:23: note: initialize the variable 'regmap' to silence this warning
     116 |         struct regmap *regmap;
         |                              ^
         |                               = NULL
   1 warning generated.


vim +/regmap +121 drivers/gpu/drm/sitronix/st7571-spi.c

   112	
   113	static int st7571_spi_probe(struct spi_device *spi)
   114	{
   115		struct st7571_device *st7571;
   116		struct regmap *regmap;
   117		struct spi_device_data *spi_data;
   118	
   119		spi_data = devm_kzalloc(&spi->dev, sizeof(*spi_data), GFP_KERNEL);
   120		if (IS_ERR(spi_data))
 > 121			return dev_err_probe(&spi->dev, PTR_ERR(regmap),
   122					     "Failed to initialize spi data\n");
   123	
   124		spi_data->spi = spi;
   125		spi_data->dc_gpio = devm_gpiod_get(&spi->dev, "dc", GPIOD_OUT_LOW);
   126	
   127		if (IS_ERR(spi_data->dc_gpio))
   128			/* init regmap for 3 line SPI */
   129			regmap = devm_regmap_init_spi(spi, &st7571_spi_regmap_config);
   130		else
   131			/* init regmap for 4 line SPI */
   132			regmap = devm_regmap_init(&spi->dev, &regmap_spi_dc_bus,
   133						  spi_data, &st7571_spi_regmap_config);
   134	
   135		if (IS_ERR(regmap))
   136			return dev_err_probe(&spi->dev, PTR_ERR(regmap),
   137					     "Failed to initialize regmap\n");
   138	
   139		st7571 = st7571_probe(&spi->dev, regmap);
   140		if (IS_ERR(st7571))
   141			return dev_err_probe(&spi->dev, PTR_ERR(st7571),
   142					     "Failed to initialize regmap\n");
   143	
   144		st7571->spi_data = spi_data;
   145		spi_set_drvdata(spi, st7571);
   146	
   147		return 0;
   148	}
   149	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] dt-bindings: display: sitronix,st75156: add example for SPI

[Krzysztof Kozlowski]

On Thu, Oct 22, 2026 at 05:31:06PM +0200, Lars Rask wrote:
> From: Lars Rask <lars.rask@axis.com>
> 
> Add example for using st75156 with SPI.

Your patch does not add example but entire binding. Please describe your
patches appropriately.

> 
> Signed-off-by: Lars Rask <lars.rask@axis.com>
> ---
>  .../bindings/display/sitronix,st75156.yaml         | 67 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 68 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/sitronix,st75156.yaml b/Documentation/devicetree/bindings/display/sitronix,st75156.yaml
> new file mode 100644
> index 000000000000..a3897ddc5e38
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/sitronix,st75156.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/sitronix,st75156.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sitronix ST75156 Display Controller
> +
> +maintainers:
> +  - Marcus Folkesson <marcus.folkesson@gmail.com>
> +
> +description:
> +  Sitronix ST75156 is a monochrome dot matrix LCD panel used in SPI 4-wire mode.
> +
> +allOf:
> +  - $ref: panel/panel-common.yaml#
> +
> +properties:
> +  compatible:
> +    const: sitronix,st75156
> +

Everything looks the same as sitronix,st7571, so why this cannot be
added there?

Best regards,
Krzysztof

Re: [PATCH 1/2] drm/sitronix/st7571-spi: Add support for ST75156 controller

[Krzysztof Kozlowski]

On Thu, Oct 22, 2026 at 05:31:05PM +0200, Lars Rask wrote:
> From: Lars Rask <lars.rask@axis.com>
> 
> Made additions to st7571 display driver to support st75156.
> 
> Signed-off-by: Lars Rask <lars.rask@axis.com>
> ---

Please organize the patch documenting the compatible (DT bindings)
before the patch using that compatible.
See also: https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46

>  drivers/gpu/drm/sitronix/st7571-spi.c | 118 +++++++++-
>  drivers/gpu/drm/sitronix/st7571.c     | 420 +++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/sitronix/st7571.h     |  20 ++
>  3 files changed, 549 insertions(+), 9 deletions(-)

...

> +
>  static int st7571_spi_probe(struct spi_device *spi)
>  {
>  	struct st7571_device *st7571;
>  	struct regmap *regmap;
> +	struct spi_device_data *spi_data;
> +
> +	spi_data = devm_kzalloc(&spi->dev, sizeof(*spi_data), GFP_KERNEL);
> +	if (IS_ERR(spi_data))
> +		return dev_err_probe(&spi->dev, PTR_ERR(regmap),
> +				     "Failed to initialize spi data\n");

I do not see how you initialize spi data here, so this is just incorrect
error message. Regardless, driver NEVER prints error messages for
allocation.


Best regards,
Krzysztof

Re: [PATCH 1/2] drm/sitronix/st7571-spi: Add support for ST75156 controller

[Krzysztof Kozlowski]

On 26/10/2026 08:23, Krzysztof Kozlowski wrote:
> On Thu, Oct 22, 2026 at 05:31:05PM +0200, Lars Rask wrote:
>> +
>>  static int st7571_spi_probe(struct spi_device *spi)
>>  {
>>  	struct st7571_device *st7571;
>>  	struct regmap *regmap;
>> +	struct spi_device_data *spi_data;
>> +
>> +	spi_data = devm_kzalloc(&spi->dev, sizeof(*spi_data), GFP_KERNEL);
>> +	if (IS_ERR(spi_data))
>> +		return dev_err_probe(&spi->dev, PTR_ERR(regmap),
>> +				     "Failed to initialize spi data\n");
> 
> I do not see how you initialize spi data here, so this is just incorrect
> error message. Regardless, driver NEVER prints error messages for
> allocation.
> 

... and you have there obvious build warning. Don't copy-paste code.

Best regards,
Krzysztof