[PATCH v2 0/4] drm/ssd130x: Add support for the Solomon SSD1351 OLED controller

[PATCH v2 0/4] drm/ssd130x: Add support for the Solomon SSD1351 OLED controller

[Amit Barzilai]

This series adds support for the Solomon SSD1351, a 128x128 65k-color
RGB OLED controller, to the ssd130x DRM driver, and removes the legacy
fbtft fb_ssd1351 driver it supersedes.

v1 [1] was a self-contained ssd1351.c driver. Following Javier's review,
the SSD1351 is instead folded into ssd130x as a new color family, which
also brings 65k color to the existing SSD1331. The work is split as:

  - Patch 1 adds the device tree binding. It was previously posted
    standalone as a v2 [2]; it is folded into this series here, as Conor
    asked, so the binding lands together with the driver and the fbtft
    removal.

  - Patch 2 changes the SSD133X family to drive RGB565 instead of
    RGB332, via a per-variant flag in deviceinfo. The SSD1331 is the
    only current member and gains 65k color from this.

  - Patch 3 adds the SSD1351 as a new SSD135X_FAMILY, reusing the
    SSD133X plane/CRTC and blit/clear helpers. The only data-path
    difference is the explicit Write RAM command (0x5c) the SSD1351
    needs before pixel data; it also gets its own init sequence and a
    longer post-reset settle delay.

  - Patch 4 removes the now-redundant staging fbtft fb_ssd1351 driver.

Testing:

  - The SSD1351 (patches 1 and 3) is tested on hardware.
  - The SSD1331 RGB565 change (patch 2) is compile-tested only; I do not
    currently have a working SSD1331 panel. Javier has kindly offered to
    test it on his SSD1331.

Dependency:

  The SSD1351 reuses ssd133x_update_rect(), which programs the column
  and row *end* address as a relative offset rather than an absolute
  coordinate. This breaks partial updates that do not start at (0,0). A
  separate fix is posted at [3]; until it lands, the SSD1351 shows the
  same partial-redraw artifacts. This series applies independently of
  that fix, but the two are best merged together.

Based on drm-misc-next.

[1] standalone v1 driver:
    https://lore.kernel.org/dri-devel/20260615181253.97551-1-amit.barzilai22@gmail.com
[2] standalone v2 binding:
    https://lore.kernel.org/dri-devel/20260615175620.88828-1-amit.barzilai22@gmail.com
[3] ssd132x/ssd133x update_rect end-address fix:
    https://lore.kernel.org/dri-devel/20260622122604.32500-1-amit.barzilai22@gmail.com

---

Changes since v1:
- Fold the SSD1351 into ssd130x as a new SSD135X family instead of a
  standalone ssd1351.c driver (per Javier).
- Add RGB565 to the SSD133X family, so the SSD1331 also gains 65k color.
- Drop native 256k color (no matching DRM fourcc) and the 0/180
  rotation support, to keep the series focused; both can return later.
- Binding: drop solomon,width / solomon,height (deducible from the
  compatible) and the rotation property (no consumer), per Krzysztof;
  use dt-bindings/gpio/gpio.h flag defines in the example.
- Remove the staging fbtft fb_ssd1351 driver in the same series (per
  Conor).

Amit Barzilai (4):
  dt-bindings: display: Add Solomon SSD1351 OLED controller
  drm/ssd130x: Add RGB565 support to SSD133X family
  drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support
  staging: fbtft: remove fb_ssd1351 driver

 .../bindings/display/solomon,ssd1351.yaml     |  42 +++
 drivers/gpu/drm/solomon/ssd130x-spi.c         |   7 +
 drivers/gpu/drm/solomon/ssd130x.c             | 269 +++++++++++++++---
 drivers/gpu/drm/solomon/ssd130x.h             |  12 +-
 drivers/staging/fbtft/Kconfig                 |   5 -
 drivers/staging/fbtft/Makefile                |   1 -
 drivers/staging/fbtft/fb_ssd1351.c            | 240 ----------------
 7 files changed, 283 insertions(+), 293 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/solomon,ssd1351.yaml
 delete mode 100644 drivers/staging/fbtft/fb_ssd1351.c

-- 
2.54.0

[PATCH v2 1/4] dt-bindings: display: Add Solomon SSD1351 OLED controller

[Amit Barzilai]

Add a device tree binding for the Solomon SSD1351, a 128x128 65k-color
RGB OLED display controller driven over a 4-wire SPI bus. The binding
builds on the shared solomon,ssd-common.yaml properties already used by
the other Solomon display controllers.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Amit Barzilai <amit.barzilai22@gmail.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 .../bindings/display/solomon,ssd1351.yaml     | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/solomon,ssd1351.yaml

diff --git a/Documentation/devicetree/bindings/display/solomon,ssd1351.yaml b/Documentation/devicetree/bindings/display/solomon,ssd1351.yaml
new file mode 100644
index 000000000000..80850c2ab5b3
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/solomon,ssd1351.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/solomon,ssd1351.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Solomon SSD1351 OLED Display Controller
+
+maintainers:
+  - Amit Barzilai <amit.barzilai22@gmail.com>
+  - Javier Martinez Canillas <javierm@redhat.com>
+
+allOf:
+  - $ref: solomon,ssd-common.yaml#
+
+properties:
+  compatible:
+    enum:
+      - solomon,ssd1351
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        oled@0 {
+            compatible = "solomon,ssd1351";
+            reg = <0x0>;
+            reset-gpios = <&gpio2 7 GPIO_ACTIVE_LOW>;
+            dc-gpios = <&gpio2 8 GPIO_ACTIVE_HIGH>;
+            spi-max-frequency = <10000000>;
+        };
+    };
-- 
2.54.0

[PATCH v2 2/4] drm/ssd130x: Add RGB565 support to SSD133X family

[Amit Barzilai]

SSD133X screens were getting 8bpp (RGB332) instead of the 16bpp
(RGB565) that they support. This change adds a boolean to the
deviceinfo struct selecting whether the variant is driven at
DRM_FORMAT_RGB565.

Changed SSD133X to now utilize 65k color (RGB565).

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Amit Barzilai <amit.barzilai22@gmail.com>
---
 drivers/gpu/drm/solomon/ssd130x.c | 55 +++++++++++++++++++++++++------
 drivers/gpu/drm/solomon/ssd130x.h |  7 ++++
 2 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 04da4f2f7d08..2b0a8218f529 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -140,6 +140,12 @@
 #define SSD133X_SET_PRECHARGE_VOLTAGE		0xbb
 #define SSD133X_SET_VCOMH_VOLTAGE		0xbe
 
+/* ssd133x remap byte (data of SSD13XX_SET_SEG_REMAP) */
+#define SSD133X_SET_REMAP_COM_SPLIT		BIT(5)
+#define SSD133X_SET_REMAP_COLOR_DEPTH_MASK	GENMASK(7, 6)
+#define SSD133X_COLOR_DEPTH_256			0x0
+#define SSD133X_COLOR_DEPTH_65K			0x1
+
 #define MAX_CONTRAST 255
 
 const struct ssd130x_deviceinfo ssd130x_variants[] = {
@@ -206,6 +212,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
 	[SSD1331_ID] = {
 		.default_width = 96,
 		.default_height = 64,
+		.format_rgb565 = 1,
 		.family_id = SSD133X_FAMILY,
 	}
 };
@@ -584,6 +591,10 @@ static int ssd132x_init(struct ssd130x_device *ssd130x)
 
 static int ssd133x_init(struct ssd130x_device *ssd130x)
 {
+	u8 remap = SSD133X_SET_REMAP_COM_SPLIT |
+		   FIELD_PREP(SSD133X_SET_REMAP_COLOR_DEPTH_MASK,
+			      ssd130x->device_info->format_rgb565 ?
+			      SSD133X_COLOR_DEPTH_65K : SSD133X_COLOR_DEPTH_256);
 	const u8 cmds[] = {
 		2, SSD133X_CONTRAST_A, 0x91,
 		2, SSD133X_CONTRAST_B, 0x50,
@@ -595,9 +606,9 @@ static int ssd133x_init(struct ssd130x_device *ssd130x)
 		 * Horizontal Address Increment
 		 * Normal order SA,SB,SC (e.g. RGB)
 		 * COM Split Odd Even
-		 * 256 color format
+		 * 256 or 65k color format, depending on the variant
 		 */
-		2, SSD13XX_SET_SEG_REMAP, 0x20,
+		2, SSD13XX_SET_SEG_REMAP, remap,
 		2, SSD133X_SET_DISPLAY_START, 0x00,
 		2, SSD133X_SET_DISPLAY_OFFSET, 0x00,
 		1, SSD133X_SET_DISPLAY_NORMAL,
@@ -794,14 +805,20 @@ static int ssd133x_update_rect(struct ssd130x_device *ssd130x,
 	 * COM0 to COM[N - 1] are the rows and SEG0 to SEG[M - 1] are
 	 * the columns.
 	 *
-	 * Each Segment has a 8-bit pixel and each Common output has a
-	 * row of pixels. When using the (default) horizontal address
-	 * increment mode, each byte of data sent to the controller has
-	 * a Segment (e.g: SEG0).
+	 * Each Segment holds one pixel and each Common output has a row
+	 * of pixels. A pixel is 8 bits (one byte) in the 256 color
+	 * (RGB332) format or 16 bits (two bytes) in the 65k color
+	 * (RGB565) format. When using the (default) horizontal address
+	 * increment mode, the pixel data is sent Segment by Segment
+	 * (e.g: SEG0 first).
 	 *
 	 * When using the 256 color depth format, each pixel contains 3
 	 * sub-pixels for color A, B and C. These have 3 bit, 3 bit and
 	 * 2 bits respectively.
+	 *
+	 * When using the 65k color depth format, each pixel contains 3
+	 * sub-pixels for color A, B and C. These have 5 bit, 6 bit and
+	 * 5 bits respectively.
 	 */
 
 	/* Set column start and end */
@@ -872,9 +889,24 @@ static void ssd132x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
 	ssd130x_write_data(ssd130x, data_array, columns * height);
 }
 
+/*
+ * The SSD133X family can drive the panel in either RGB332 (1 byte per pixel)
+ * or RGB565 (2 bytes per pixel). The format is a per-variant policy choice
+ * selected through ssd130x_deviceinfo::format_rgb565, not a capability probe.
+ * Centralize the choice here so that the buffer sizing (allocation, clear and
+ * blit pitch) can never disagree.
+ */
+static const struct drm_format_info *ssd133x_format_info(struct ssd130x_device *ssd130x)
+{
+	if (ssd130x->device_info->format_rgb565)
+		return drm_format_info(DRM_FORMAT_RGB565);
+
+	return drm_format_info(DRM_FORMAT_RGB332);
+}
+
 static void ssd133x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
 {
-	const struct drm_format_info *fi = drm_format_info(DRM_FORMAT_RGB332);
+	const struct drm_format_info *fi = ssd133x_format_info(ssd130x);
 	unsigned int pitch;
 
 	if (!fi)
@@ -945,7 +977,7 @@ static int ssd133x_fb_blit_rect(struct drm_framebuffer *fb,
 				struct drm_format_conv_state *fmtcnv_state)
 {
 	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
-	const struct drm_format_info *fi = drm_format_info(DRM_FORMAT_RGB332);
+	const struct drm_format_info *fi = ssd133x_format_info(ssd130x);
 	unsigned int dst_pitch;
 	struct iosys_map dst;
 	int ret = 0;
@@ -956,7 +988,10 @@ static int ssd133x_fb_blit_rect(struct drm_framebuffer *fb,
 	dst_pitch = drm_format_info_min_pitch(fi, 0, drm_rect_width(rect));
 
 	iosys_map_set_vaddr(&dst, data_array);
-	drm_fb_xrgb8888_to_rgb332(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);
+	if (ssd130x->device_info->format_rgb565)
+		drm_fb_xrgb8888_to_rgb565be(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);
+	else
+		drm_fb_xrgb8888_to_rgb332(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);
 
 	ssd133x_update_rect(ssd130x, rect, data_array, dst_pitch);
 
@@ -1414,7 +1449,7 @@ static int ssd133x_crtc_atomic_check(struct drm_crtc *crtc,
 	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
 	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 	struct ssd130x_crtc_state *ssd130x_state = to_ssd130x_crtc_state(crtc_state);
-	const struct drm_format_info *fi = drm_format_info(DRM_FORMAT_RGB332);
+	const struct drm_format_info *fi = ssd133x_format_info(ssd130x);
 	unsigned int pitch;
 	int ret;
 
diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h
index a4554018bb2a..b0b487c06e04 100644
--- a/drivers/gpu/drm/solomon/ssd130x.h
+++ b/drivers/gpu/drm/solomon/ssd130x.h
@@ -54,6 +54,13 @@ struct ssd130x_deviceinfo {
 	bool need_pwm;
 	bool need_chargepump;
 	bool page_mode_only;
+	/*
+	 * Per-variant output format selector for the SSD133X data path. The
+	 * hardware can drive the panel in RGB332 (1 byte/pixel) or RGB565
+	 * (2 bytes/pixel); this is a policy choice per variant, not a
+	 * capability probe. When set, the variant is driven at RGB565.
+	 */
+	bool format_rgb565;
 
 	enum ssd130x_family_ids family_id;
 };
-- 
2.54.0

[PATCH v2 3/4] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support

[Amit Barzilai]

The Solomon SSD1351 is a 128x128 RGB color OLED controller. It shares
the SSD133X data path: a column/row addressing window followed by a bulk
RGB565 pixel write. Add it as a new SSD135X_FAMILY rather than a separate
driver, reusing the SSD133X plane, CRTC and blit/clear helpers.

The only data-path difference is that the SSD1351 requires an explicit
Write RAM command (0x5c) after the address window is programmed, before
pixel data is accepted, whereas the SSD133X enters data mode implicitly.
This is emitted from a shared ssd133x_write_pixels() helper so both the
damage-update and clear-screen paths cover it.

The SSD1351 also needs its own init sequence (ssd135x_init), dispatched
via ssd135x_encoder_atomic_enable, and a longer post-reset settle delay.
The re-map byte is fixed at 0 degrees, 65k color, COM split, BGR
sub-pixel order; rotation is not supported.

The SSD1351 is SPI-only, so only the SPI transport match tables gain an
entry; no new config symbol is needed.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Amit Barzilai <amit.barzilai22@gmail.com>
---
 drivers/gpu/drm/solomon/ssd130x-spi.c |   7 +
 drivers/gpu/drm/solomon/ssd130x.c     | 214 +++++++++++++++++++++-----
 drivers/gpu/drm/solomon/ssd130x.h     |   5 +-
 3 files changed, 189 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x-spi.c b/drivers/gpu/drm/solomon/ssd130x-spi.c
index b52f5fd592a1..6e0dd6e5a88d 100644
--- a/drivers/gpu/drm/solomon/ssd130x-spi.c
+++ b/drivers/gpu/drm/solomon/ssd130x-spi.c
@@ -146,6 +146,11 @@ static const struct of_device_id ssd130x_of_match[] = {
 		.compatible = "solomon,ssd1331",
 		.data = &ssd130x_variants[SSD1331_ID],
 	},
+	/* ssd135x family */
+	{
+		.compatible = "solomon,ssd1351",
+		.data = &ssd130x_variants[SSD1351_ID],
+	},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, ssd130x_of_match);
@@ -171,6 +176,8 @@ static const struct spi_device_id ssd130x_spi_id[] = {
 	{ "ssd1327", SSD1327_ID },
 	/* ssd133x family */
 	{ "ssd1331", SSD1331_ID },
+	/* ssd135x family */
+	{ "ssd1351", SSD1351_ID },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(spi, ssd130x_spi_id);
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 2b0a8218f529..e5a9428f91b8 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -146,6 +146,33 @@
 #define SSD133X_COLOR_DEPTH_256			0x0
 #define SSD133X_COLOR_DEPTH_65K			0x1
 
+/* ssd135x commands */
+#define SSD135X_SET_COL_RANGE			0x15
+#define SSD135X_WRITE_RAM			0x5c
+#define SSD135X_SET_ROW_RANGE			0x75
+#define SSD135X_SET_DISPLAY_START		0xa1
+#define SSD135X_SET_DISPLAY_OFFSET		0xa2
+#define SSD135X_SET_DISPLAY_NORMAL		0xa6
+#define SSD135X_SET_FUNCTION			0xab
+#define SSD135X_SET_PHASE_LENGTH		0xb1
+#define SSD135X_SET_CLOCK_FREQ			0xb3
+#define SSD135X_SET_VSL				0xb4
+#define SSD135X_SET_GPIO			0xb5
+#define SSD135X_SET_PRECHARGE2			0xb6
+#define SSD135X_SET_PRECHARGE			0xbb
+#define SSD135X_SET_VCOMH_VOLTAGE		0xbe
+#define SSD135X_SET_CONTRAST			0xc1
+#define SSD135X_SET_CONTRAST_MASTER		0xc7
+#define SSD135X_SET_MUX_RATIO			0xca
+#define SSD135X_SET_COMMAND_LOCK		0xfd
+
+/* ssd135x remap byte (data of SSD13XX_SET_SEG_REMAP) */
+#define SSD135X_SET_REMAP_COLUMN		BIT(1)
+#define SSD135X_SET_REMAP_COLOR_BGR		BIT(2)
+#define SSD135X_SET_REMAP_COM_SCAN		BIT(4)
+#define SSD135X_SET_REMAP_COM_SPLIT		BIT(5)
+#define SSD135X_SET_REMAP_65K			BIT(6)
+
 #define MAX_CONTRAST 255
 
 const struct ssd130x_deviceinfo ssd130x_variants[] = {
@@ -214,6 +241,13 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
 		.default_height = 64,
 		.format_rgb565 = 1,
 		.family_id = SSD133X_FAMILY,
+	},
+	/* ssd135x family */
+	[SSD1351_ID] = {
+		.default_width = 128,
+		.default_height = 128,
+		.format_rgb565 = 1,
+		.family_id = SSD135X_FAMILY,
 	}
 };
 EXPORT_SYMBOL_NS_GPL(ssd130x_variants, "DRM_SSD130X");
@@ -248,47 +282,16 @@ static inline struct ssd130x_device *drm_to_ssd130x(struct drm_device *drm)
 /*
  * Helper to write data (SSD13XX_DATA) to the device.
  */
-static int ssd130x_write_data(struct ssd130x_device *ssd130x, u8 *values, int count)
+static int ssd130x_write_data(struct ssd130x_device *ssd130x, const u8 *values, int count)
 {
 	return regmap_bulk_write(ssd130x->regmap, SSD13XX_DATA, values, count);
 }
 
-/*
- * Helper to write command (SSD13XX_COMMAND). The fist variadic argument
- * is the command to write and the following are the command options.
- *
- * Note that the ssd13xx protocol requires each command and option to be
- * written as a SSD13XX_COMMAND device register value. That is why a call
- * to regmap_write(..., SSD13XX_COMMAND, ...) is done for each argument.
- */
-static int ssd130x_write_cmd(struct ssd130x_device *ssd130x, int count,
-			     /* u8 cmd, u8 option, ... */...)
-{
-	va_list ap;
-	u8 value;
-	int ret;
-
-	va_start(ap, count);
-
-	do {
-		value = va_arg(ap, int);
-		ret = regmap_write(ssd130x->regmap, SSD13XX_COMMAND, value);
-		if (ret)
-			goto out_end;
-	} while (--count);
-
-out_end:
-	va_end(ap);
-
-	return ret;
-}
-
 /*
  * Write a command byte sequence from a buffer.
  *
- * Like ssd130x_write_cmd() but takes a pre-built byte array instead of
- * variadic arguments, handy when the command is already in an array or
- * when the caller wants to use sizeof() for the length.
+ * The first byte is the command opcode and the following bytes are its
+ * options/parameters.
  */
 static int ssd130x_write_cmds(struct ssd130x_device *ssd130x, const u8 *cmd,
 			      size_t len)
@@ -296,6 +299,22 @@ static int ssd130x_write_cmds(struct ssd130x_device *ssd130x, const u8 *cmd,
 	unsigned int i;
 	int ret;
 
+	/*
+	 * The SSD135X family latches command parameters with D/C# HIGH (i.e.
+	 * clocked in as data), unlike the other families where the opcode and
+	 * all of its parameters are sent as commands (D/C# LOW). Send the
+	 * opcode as a command and any following parameter bytes as data.
+	 */
+	if (ssd130x->device_info->family_id == SSD135X_FAMILY) {
+		if (len == 0)
+			return 0;
+		ret = regmap_write(ssd130x->regmap, SSD13XX_COMMAND, cmd[0]);
+		if (ret || len == 1)
+			return ret;
+
+		return ssd130x_write_data(ssd130x, cmd + 1, len - 1);
+	}
+
 	for (i = 0; i < len; i++) {
 		ret = regmap_write(ssd130x->regmap, SSD13XX_COMMAND, cmd[i]);
 		if (ret)
@@ -305,6 +324,28 @@ static int ssd130x_write_cmds(struct ssd130x_device *ssd130x, const u8 *cmd,
 	return 0;
 }
 
+/*
+ * Variadic wrapper around ssd130x_write_cmds(). The first variadic argument is
+ * the command opcode and the following ones are its options/parameters.
+ */
+static int ssd130x_write_cmd(struct ssd130x_device *ssd130x, int count,
+			     /* u8 cmd, u8 option, ... */...)
+{
+	u8 buf[8];
+	va_list ap;
+	int i;
+
+	if (count > ARRAY_SIZE(buf))
+		return -EINVAL;
+
+	va_start(ap, count);
+	for (i = 0; i < count; i++)
+		buf[i] = va_arg(ap, int);
+	va_end(ap);
+
+	return ssd130x_write_cmds(ssd130x, buf, count);
+}
+
 /*
  * Run a packed command sequence.  The format is a flat byte array where each
  * entry starts with a length byte followed by that many command bytes.  A
@@ -628,6 +669,49 @@ static int ssd133x_init(struct ssd130x_device *ssd130x)
 	return ssd130x_run_cmd_seq(ssd130x, cmds);
 }
 
+static int ssd135x_init(struct ssd130x_device *ssd130x)
+{
+	/*
+	 * Horizontal address increment, COM split, reversed COM scan direction,
+	 * BGR sub-pixel order and 65k (RGB565) color depth. Rotation is not
+	 * supported, so the remap byte is fixed.
+	 */
+	u8 remap = SSD135X_SET_REMAP_65K | SSD135X_SET_REMAP_COM_SPLIT |
+		   SSD135X_SET_REMAP_COLOR_BGR | SSD135X_SET_REMAP_COM_SCAN;
+	const u8 cmds[] = {
+		2, SSD135X_SET_COMMAND_LOCK, 0x12,
+		2, SSD135X_SET_COMMAND_LOCK, 0xb1,
+		1, SSD13XX_DISPLAY_OFF,
+		2, SSD135X_SET_CLOCK_FREQ, 0xf1,
+		2, SSD135X_SET_MUX_RATIO, ssd130x->height - 1,
+		3, SSD135X_SET_COL_RANGE, 0x00, ssd130x->width - 1,
+		3, SSD135X_SET_ROW_RANGE, 0x00, ssd130x->height - 1,
+		2, SSD135X_SET_DISPLAY_START, 0x00,
+		2, SSD135X_SET_DISPLAY_OFFSET, 0x00,
+		2, SSD135X_SET_GPIO, 0x00,
+		2, SSD135X_SET_FUNCTION, 0x01,
+		2, SSD135X_SET_PHASE_LENGTH, 0x32,
+		4, SSD135X_SET_VSL, 0xa0, 0xb5, 0x55,
+		2, SSD135X_SET_PRECHARGE, 0x17,
+		2, SSD135X_SET_VCOMH_VOLTAGE, 0x05,
+		4, SSD135X_SET_CONTRAST, 0xc8, 0x80, 0xc8,
+		2, SSD135X_SET_CONTRAST_MASTER, 0x0f,
+		2, SSD135X_SET_PRECHARGE2, 0x01,
+		1, SSD135X_SET_DISPLAY_NORMAL,
+		2, SSD13XX_SET_SEG_REMAP, remap,
+		0,
+	};
+
+	/*
+	 * ssd130x_power_on() issues a short reset pulse, but the SSD1351 is not
+	 * ready to accept commands immediately afterwards. Give the controller
+	 * time to settle before sending the init sequence.
+	 */
+	msleep(120);
+
+	return ssd130x_run_cmd_seq(ssd130x, cmds);
+}
+
 static int ssd130x_update_rect(struct ssd130x_device *ssd130x,
 			       struct drm_rect *rect, u8 *buf,
 			       u8 *data_array)
@@ -790,6 +874,25 @@ static int ssd132x_update_rect(struct ssd130x_device *ssd130x,
 	return ret;
 }
 
+/*
+ * Write a run of pixel data to the controller's display RAM. The SSD135X
+ * family requires an explicit Write RAM command once the address window has
+ * been set, before any pixel data is accepted; the SSD133X family enters data
+ * mode implicitly after the column/row range is programmed.
+ */
+static int ssd133x_write_pixels(struct ssd130x_device *ssd130x,
+				u8 *data_array, unsigned int count)
+{
+	if (ssd130x->device_info->family_id == SSD135X_FAMILY) {
+		int ret = ssd130x_write_cmd(ssd130x, 1, SSD135X_WRITE_RAM);
+
+		if (ret < 0)
+			return ret;
+	}
+
+	return ssd130x_write_data(ssd130x, data_array, count);
+}
+
 static int ssd133x_update_rect(struct ssd130x_device *ssd130x,
 			       struct drm_rect *rect, u8 *data_array,
 			       unsigned int pitch)
@@ -832,7 +935,7 @@ static int ssd133x_update_rect(struct ssd130x_device *ssd130x,
 		return ret;
 
 	/* Write out update in one go since horizontal addressing mode is used */
-	ret = ssd130x_write_data(ssd130x, data_array, pitch * rows);
+	ret = ssd133x_write_pixels(ssd130x, data_array, pitch * rows);
 
 	return ret;
 }
@@ -917,7 +1020,7 @@ static void ssd133x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
 	memset(data_array, 0, pitch * ssd130x->height);
 
 	/* Write out update in one go since horizontal addressing mode is used */
-	ssd130x_write_data(ssd130x, data_array, pitch * ssd130x->height);
+	ssd133x_write_pixels(ssd130x, data_array, pitch * ssd130x->height);
 }
 
 static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb,
@@ -1380,6 +1483,12 @@ static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs[]
 		.atomic_check = ssd133x_primary_plane_atomic_check,
 		.atomic_update = ssd133x_primary_plane_atomic_update,
 		.atomic_disable = ssd133x_primary_plane_atomic_disable,
+	},
+	[SSD135X_FAMILY] = {
+		DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+		.atomic_check = ssd133x_primary_plane_atomic_check,
+		.atomic_update = ssd133x_primary_plane_atomic_update,
+		.atomic_disable = ssd133x_primary_plane_atomic_disable,
 	}
 };
 
@@ -1534,6 +1643,10 @@ static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs[] = {
 		.mode_valid = ssd130x_crtc_mode_valid,
 		.atomic_check = ssd133x_crtc_atomic_check,
 	},
+	[SSD135X_FAMILY] = {
+		.mode_valid = ssd130x_crtc_mode_valid,
+		.atomic_check = ssd133x_crtc_atomic_check,
+	},
 };
 
 static const struct drm_crtc_funcs ssd130x_crtc_funcs = {
@@ -1621,6 +1734,31 @@ static void ssd133x_encoder_atomic_enable(struct drm_encoder *encoder,
 	ssd130x_power_off(ssd130x);
 }
 
+static void ssd135x_encoder_atomic_enable(struct drm_encoder *encoder,
+					  struct drm_atomic_commit *state)
+{
+	struct drm_device *drm = encoder->dev;
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
+	int ret;
+
+	ret = ssd130x_power_on(ssd130x);
+	if (ret)
+		return;
+
+	ret = ssd135x_init(ssd130x);
+	if (ret)
+		goto power_off;
+
+	ssd130x_write_cmd(ssd130x, 1, SSD13XX_DISPLAY_ON);
+
+	backlight_enable(ssd130x->bl_dev);
+
+	return;
+
+power_off:
+	ssd130x_power_off(ssd130x);
+}
+
 static void ssd130x_encoder_atomic_disable(struct drm_encoder *encoder,
 					   struct drm_atomic_commit *state)
 {
@@ -1646,6 +1784,10 @@ static const struct drm_encoder_helper_funcs ssd130x_encoder_helper_funcs[] = {
 	[SSD133X_FAMILY] = {
 		.atomic_enable = ssd133x_encoder_atomic_enable,
 		.atomic_disable = ssd130x_encoder_atomic_disable,
+	},
+	[SSD135X_FAMILY] = {
+		.atomic_enable = ssd135x_encoder_atomic_enable,
+		.atomic_disable = ssd130x_encoder_atomic_disable,
 	}
 };
 
diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h
index b0b487c06e04..da89d4455270 100644
--- a/drivers/gpu/drm/solomon/ssd130x.h
+++ b/drivers/gpu/drm/solomon/ssd130x.h
@@ -26,7 +26,8 @@
 enum ssd130x_family_ids {
 	SSD130X_FAMILY,
 	SSD132X_FAMILY,
-	SSD133X_FAMILY
+	SSD133X_FAMILY,
+	SSD135X_FAMILY
 };
 
 enum ssd130x_variants {
@@ -42,6 +43,8 @@ enum ssd130x_variants {
 	SSD1327_ID,
 	/* ssd133x family */
 	SSD1331_ID,
+	/* ssd135x family */
+	SSD1351_ID,
 	NR_SSD130X_VARIANTS
 };
 
-- 
2.54.0

[PATCH v2 4/4] staging: fbtft: remove fb_ssd1351 driver

[Amit Barzilai]

The SSD1351 support was added to the ssd130x DRM driver. To avoid
confusion and irrelevant updates, the staging fb_ssd1351 driver is
removed.

Signed-off-by: Amit Barzilai <amit.barzilai22@gmail.com>
---
 drivers/staging/fbtft/Kconfig      |   5 -
 drivers/staging/fbtft/Makefile     |   1 -
 drivers/staging/fbtft/fb_ssd1351.c | 240 -----------------------------
 3 files changed, 246 deletions(-)
 delete mode 100644 drivers/staging/fbtft/fb_ssd1351.c

diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
index 92943564cb91..c8ea1a0f3fb0 100644
--- a/drivers/staging/fbtft/Kconfig
+++ b/drivers/staging/fbtft/Kconfig
@@ -133,11 +133,6 @@ config FB_TFT_SSD1331
 	help
 	  Framebuffer support for SSD1331
 
-config FB_TFT_SSD1351
-	tristate "FB driver for the SSD1351 LCD Controller"
-	help
-	  Framebuffer support for SSD1351
-
 config FB_TFT_ST7735R
 	tristate "FB driver for the ST7735R LCD Controller"
 	help
diff --git a/drivers/staging/fbtft/Makefile b/drivers/staging/fbtft/Makefile
index e9cdf0f0a7da..c230bf008fc7 100644
--- a/drivers/staging/fbtft/Makefile
+++ b/drivers/staging/fbtft/Makefile
@@ -28,7 +28,6 @@ obj-$(CONFIG_FB_TFT_SSD1305)     += fb_ssd1305.o
 obj-$(CONFIG_FB_TFT_SSD1306)     += fb_ssd1306.o
 obj-$(CONFIG_FB_TFT_SSD1305)     += fb_ssd1325.o
 obj-$(CONFIG_FB_TFT_SSD1331)     += fb_ssd1331.o
-obj-$(CONFIG_FB_TFT_SSD1351)     += fb_ssd1351.o
 obj-$(CONFIG_FB_TFT_ST7735R)     += fb_st7735r.o
 obj-$(CONFIG_FB_TFT_ST7789V)     += fb_st7789v.o
 obj-$(CONFIG_FB_TFT_TINYLCD)     += fb_tinylcd.o
diff --git a/drivers/staging/fbtft/fb_ssd1351.c b/drivers/staging/fbtft/fb_ssd1351.c
deleted file mode 100644
index 6736b09b2f45..000000000000
--- a/drivers/staging/fbtft/fb_ssd1351.c
+++ /dev/null
@@ -1,240 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-
-#include <linux/backlight.h>
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/spi/spi.h>
-#include <linux/delay.h>
-#include <linux/string_choices.h>
-
-#include "fbtft.h"
-
-#define DRVNAME		"fb_ssd1351"
-#define WIDTH		128
-#define HEIGHT		128
-#define GAMMA_NUM	1
-#define GAMMA_LEN	63
-#define DEFAULT_GAMMA	"0 2 2 2 2 2 2 2 " \
-			"2 2 2 2 2 2 2 2 " \
-			"2 2 2 2 2 2 2 2 " \
-			"2 2 2 2 2 2 2 2 " \
-			"2 2 2 2 2 2 2 2 " \
-			"2 2 2 2 2 2 2 2 " \
-			"2 2 2 2 2 2 2 2 " \
-			"2 2 2 2 2 2 2" \
-
-static void register_onboard_backlight(struct fbtft_par *par);
-
-static int init_display(struct fbtft_par *par)
-{
-	if (par->pdata &&
-	    par->pdata->display.backlight == FBTFT_ONBOARD_BACKLIGHT) {
-		/* module uses onboard GPIO for panel power */
-		par->fbtftops.register_backlight = register_onboard_backlight;
-	}
-
-	par->fbtftops.reset(par);
-
-	write_reg(par, 0xfd, 0x12); /* Command Lock */
-	write_reg(par, 0xfd, 0xb1); /* Command Lock */
-	write_reg(par, 0xae); /* Display Off */
-	write_reg(par, 0xb3, 0xf1); /* Front Clock Div */
-	write_reg(par, 0xca, 0x7f); /* Set Mux Ratio */
-	write_reg(par, 0x15, 0x00, 0x7f); /* Set Column Address */
-	write_reg(par, 0x75, 0x00, 0x7f); /* Set Row Address */
-	write_reg(par, 0xa1, 0x00); /* Set Display Start Line */
-	write_reg(par, 0xa2, 0x00); /* Set Display Offset */
-	write_reg(par, 0xb5, 0x00); /* Set GPIO */
-	write_reg(par, 0xab, 0x01); /* Set Function Selection */
-	write_reg(par, 0xb1, 0x32); /* Set Phase Length */
-	write_reg(par, 0xb4, 0xa0, 0xb5, 0x55); /* Set Segment Low Voltage */
-	write_reg(par, 0xbb, 0x17); /* Set Precharge Voltage */
-	write_reg(par, 0xbe, 0x05); /* Set VComH Voltage */
-	write_reg(par, 0xc1, 0xc8, 0x80, 0xc8); /* Set Contrast */
-	write_reg(par, 0xc7, 0x0f); /* Set Master Contrast */
-	write_reg(par, 0xb6, 0x01); /* Set Second Precharge Period */
-	write_reg(par, 0xa6); /* Set Display Mode Reset */
-	write_reg(par, 0xaf); /* Set Sleep Mode Display On */
-
-	return 0;
-}
-
-static void set_addr_win(struct fbtft_par *par, int xs, int ys, int xe, int ye)
-{
-	write_reg(par, 0x15, xs, xe);
-	write_reg(par, 0x75, ys, ye);
-	write_reg(par, 0x5c);
-}
-
-static int set_var(struct fbtft_par *par)
-{
-	unsigned int remap;
-
-	if (par->fbtftops.init_display != init_display) {
-		/* don't risk messing up register A0h */
-		return 0;
-	}
-
-	remap = 0x60 | (par->bgr << 2); /* Set Colour Depth */
-
-	switch (par->info->var.rotate) {
-	case 0:
-		write_reg(par, 0xA0, remap | 0x00 | BIT(4));
-		break;
-	case 270:
-		write_reg(par, 0xA0, remap | 0x03 | BIT(4));
-		break;
-	case 180:
-		write_reg(par, 0xA0, remap | 0x02);
-		break;
-	case 90:
-		write_reg(par, 0xA0, remap | 0x01);
-		break;
-	}
-
-	return 0;
-}
-
-/*
- * Grayscale Lookup Table
- * GS1 - GS63
- * The driver Gamma curve contains the relative values between the entries
- * in the Lookup table.
- *
- * From datasheet:
- * 8.8 Gray Scale Decoder
- *
- *	there are total 180 Gamma Settings (Setting 0 to Setting 180)
- *	available for the Gray Scale table.
- *
- *	The gray scale is defined in incremental way, with reference
- *	to the length of previous table entry:
- *		Setting of GS1 has to be >= 0
- *		Setting of GS2 has to be > Setting of GS1 +1
- *		Setting of GS3 has to be > Setting of GS2 +1
- *		:
- *		Setting of GS63 has to be > Setting of GS62 +1
- *
- */
-static int set_gamma(struct fbtft_par *par, u32 *curves)
-{
-	unsigned long tmp[GAMMA_NUM * GAMMA_LEN];
-	int i, acc = 0;
-
-	for (i = 0; i < 63; i++) {
-		if (i > 0 && curves[i] < 2) {
-			dev_err(par->info->device,
-				"Illegal value in Grayscale Lookup Table at index %d : %d. Must be greater than 1\n",
-				i, curves[i]);
-			return -EINVAL;
-		}
-		acc += curves[i];
-		tmp[i] = acc;
-		if (acc > 180) {
-			dev_err(par->info->device,
-				"Illegal value(s) in Grayscale Lookup Table. At index=%d : %d, the accumulated value has exceeded 180\n",
-				i, acc);
-			return -EINVAL;
-		}
-	}
-
-	write_reg(par, 0xB8,
-		  tmp[0],  tmp[1],  tmp[2],  tmp[3],
-		  tmp[4],  tmp[5],  tmp[6],  tmp[7],
-		  tmp[8],  tmp[9],  tmp[10], tmp[11],
-		  tmp[12], tmp[13], tmp[14], tmp[15],
-		  tmp[16], tmp[17], tmp[18], tmp[19],
-		  tmp[20], tmp[21], tmp[22], tmp[23],
-		  tmp[24], tmp[25], tmp[26], tmp[27],
-		  tmp[28], tmp[29], tmp[30], tmp[31],
-		  tmp[32], tmp[33], tmp[34], tmp[35],
-		  tmp[36], tmp[37], tmp[38], tmp[39],
-		  tmp[40], tmp[41], tmp[42], tmp[43],
-		  tmp[44], tmp[45], tmp[46], tmp[47],
-		  tmp[48], tmp[49], tmp[50], tmp[51],
-		  tmp[52], tmp[53], tmp[54], tmp[55],
-		  tmp[56], tmp[57], tmp[58], tmp[59],
-		  tmp[60], tmp[61], tmp[62]);
-
-	return 0;
-}
-
-static int blank(struct fbtft_par *par, bool on)
-{
-	fbtft_par_dbg(DEBUG_BLANK, par, "(%s=%s)\n",
-		      __func__, str_true_false(on));
-	if (on)
-		write_reg(par, 0xAE);
-	else
-		write_reg(par, 0xAF);
-	return 0;
-}
-
-static struct fbtft_display display = {
-	.regwidth = 8,
-	.width = WIDTH,
-	.height = HEIGHT,
-	.gamma_num = GAMMA_NUM,
-	.gamma_len = GAMMA_LEN,
-	.gamma = DEFAULT_GAMMA,
-	.fbtftops = {
-		.init_display = init_display,
-		.set_addr_win = set_addr_win,
-		.set_var = set_var,
-		.set_gamma = set_gamma,
-		.blank = blank,
-	},
-};
-
-static int update_onboard_backlight(struct backlight_device *bd)
-{
-	struct fbtft_par *par = bl_get_data(bd);
-	bool on;
-
-	fbtft_par_dbg(DEBUG_BACKLIGHT, par, "%s: power=%d\n", __func__, bd->props.power);
-
-	on = !backlight_is_blank(bd);
-	/* Onboard backlight connected to GPIO0 on SSD1351, GPIO1 unused */
-	write_reg(par, 0xB5, on ? 0x03 : 0x02);
-
-	return 0;
-}
-
-static const struct backlight_ops bl_ops = {
-	.update_status = update_onboard_backlight,
-};
-
-static void register_onboard_backlight(struct fbtft_par *par)
-{
-	struct backlight_device *bd;
-	struct backlight_properties bl_props = { 0, };
-
-	bl_props.type = BACKLIGHT_RAW;
-	bl_props.power = BACKLIGHT_POWER_OFF;
-
-	bd = backlight_device_register(dev_driver_string(par->info->device),
-				       par->info->device, par, &bl_ops,
-				       &bl_props);
-	if (IS_ERR(bd)) {
-		dev_err(par->info->device,
-			"cannot register backlight device (%ld)\n",
-			PTR_ERR(bd));
-		return;
-	}
-	par->info->bl_dev = bd;
-
-	if (!par->fbtftops.unregister_backlight)
-		par->fbtftops.unregister_backlight = fbtft_unregister_backlight;
-}
-
-FBTFT_REGISTER_DRIVER(DRVNAME, "solomon,ssd1351", &display);
-
-MODULE_ALIAS("spi:" DRVNAME);
-MODULE_ALIAS("platform:" DRVNAME);
-MODULE_ALIAS("spi:ssd1351");
-MODULE_ALIAS("platform:ssd1351");
-
-MODULE_DESCRIPTION("SSD1351 OLED Driver");
-MODULE_AUTHOR("James Davies");
-MODULE_LICENSE("GPL");
-- 
2.54.0

Re: [PATCH v2 4/4] staging: fbtft: remove fb_ssd1351 driver

[Maxime Ripard]

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

Hi,

On Mon, Jun 22, 2026 at 06:25:06PM +0300, Amit Barzilai wrote:
> The SSD1351 support was added to the ssd130x DRM driver. To avoid
> confusion and irrelevant updates, the staging fb_ssd1351 driver is
> removed.
> 
> Signed-off-by: Amit Barzilai <amit.barzilai22@gmail.com>

fbtft uses a different dt binding iirc, and has a different uABI, so we
should probably keep it around.

Maxime

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

Re: [PATCH v2 4/4] staging: fbtft: remove fb_ssd1351 driver

[Andy Shevchenko]

On Mon, Jun 22, 2026 at 06:25:06PM +0300, Amit Barzilai wrote:
> The SSD1351 support was added to the ssd130x DRM driver. To avoid
> confusion and irrelevant updates, the staging fb_ssd1351 driver is
> removed.

NAK, the fbtft has two drivers in one (SPI + parallel), plus as Maxime said,
it has its own binding.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/4] dt-bindings: display: Add Solomon SSD1351 OLED controller

[Krzysztof Kozlowski]

On Mon, Jun 22, 2026 at 06:25:03PM +0300, Amit Barzilai wrote:
> Add a device tree binding for the Solomon SSD1351, a 128x128 65k-color
> RGB OLED display controller driven over a 4-wire SPI bus. The binding
> builds on the shared solomon,ssd-common.yaml properties already used by
> the other Solomon display controllers.
> 
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Amit Barzilai <amit.barzilai22@gmail.com>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>  .../bindings/display/solomon,ssd1351.yaml     | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/solomon,ssd1351.yaml

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

Re: [PATCH v2 4/4] staging: fbtft: remove fb_ssd1351 driver

[Javier Martinez Canillas]

Andy Shevchenko <andriy.shevchenko@intel.com> writes:

> On Mon, Jun 22, 2026 at 06:25:06PM +0300, Amit Barzilai wrote:
>> The SSD1351 support was added to the ssd130x DRM driver. To avoid
>> confusion and irrelevant updates, the staging fb_ssd1351 driver is
>> removed.
>
> NAK, the fbtft has two drivers in one (SPI + parallel), plus as Maxime said,
> it has its own binding.
>

Yes, I agree that we don't want to delete the fbtft driver.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 2/4] drm/ssd130x: Add RGB565 support to SSD133X family

[Andy Shevchenko]

On Mon, Jun 22, 2026 at 06:25:04PM +0300, Amit Barzilai wrote:
> SSD133X screens were getting 8bpp (RGB332) instead of the 16bpp
> (RGB565) that they support. This change adds a boolean to the
> deviceinfo struct selecting whether the variant is driven at
> DRM_FORMAT_RGB565.
> 
> Changed SSD133X to now utilize 65k color (RGB565).

...

> -	 * Each Segment has a 8-bit pixel and each Common output has a
> -	 * row of pixels. When using the (default) horizontal address
> -	 * increment mode, each byte of data sent to the controller has
> -	 * a Segment (e.g: SEG0).
> +	 * Each Segment holds one pixel and each Common output has a row
> +	 * of pixels. A pixel is 8 bits (one byte) in the 256 color
> +	 * (RGB332) format or 16 bits (two bytes) in the 65k color
> +	 * (RGB565) format. When using the (default) horizontal address
> +	 * increment mode, the pixel data is sent Segment by Segment
> +	 * (e.g: SEG0 first).
>  	 *
>  	 * When using the 256 color depth format, each pixel contains 3
>  	 * sub-pixels for color A, B and C. These have 3 bit, 3 bit and
>  	 * 2 bits respectively.

Something wrong with the plural. There is a difference between "3-bit" and
"3 bits", but "3 bit" is odd.

> +	 *
> +	 * When using the 65k color depth format, each pixel contains 3
> +	 * sub-pixels for color A, B and C. These have 5 bit, 6 bit and
> +	 * 5 bits respectively.

Same mistake is repeated here.

...

> +	/*
> +	 * Per-variant output format selector for the SSD133X data path. The
> +	 * hardware can drive the panel in RGB332 (1 byte/pixel) or RGB565
> +	 * (2 bytes/pixel); this is a policy choice per variant, not a

In other comments it was spelled fully, be consistent "1 byte per pixel",
"2 bytes per pixel".

> +	 * capability probe. When set, the variant is driven at RGB565.
> +	 */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support

[Markus Elfring]

…
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -146,6 +146,33 @@
>  #define SSD133X_COLOR_DEPTH_256			0x0
>  #define SSD133X_COLOR_DEPTH_65K			0x1
>  
> +/* ssd135x commands */
> +#define SSD135X_SET_COL_RANGE			0x15
> +#define SSD135X_WRITE_RAM			0x5c
> +#define SSD135X_SET_ROW_RANGE			0x75
…

How do you think about to use an enumeration for such data?
https://en.wikipedia.org/wiki/Enumerated_type#C_and_syntactically_similar_languages

Regards,
Markus

Re: [PATCH v2 3/4] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support

[Andy Shevchenko]

On Mon, Jun 22, 2026 at 06:25:05PM +0300, Amit Barzilai wrote:
> The Solomon SSD1351 is a 128x128 RGB color OLED controller. It shares
> the SSD133X data path: a column/row addressing window followed by a bulk
> RGB565 pixel write. Add it as a new SSD135X_FAMILY rather than a separate
> driver, reusing the SSD133X plane, CRTC and blit/clear helpers.
> 
> The only data-path difference is that the SSD1351 requires an explicit
> Write RAM command (0x5c) after the address window is programmed, before
> pixel data is accepted, whereas the SSD133X enters data mode implicitly.
> This is emitted from a shared ssd133x_write_pixels() helper so both the
> damage-update and clear-screen paths cover it.
> 
> The SSD1351 also needs its own init sequence (ssd135x_init), dispatched
> via ssd135x_encoder_atomic_enable, and a longer post-reset settle delay.
> The re-map byte is fixed at 0 degrees, 65k color, COM split, BGR
> sub-pixel order; rotation is not supported.
> 
> The SSD1351 is SPI-only, so only the SPI transport match tables gain an
> entry; no new config symbol is needed.

...

> const struct ssd130x_deviceinfo ssd130x_variants[] = {

>  		.default_height = 64,
>  		.format_rgb565 = 1,
>  		.family_id = SSD133X_FAMILY,
> +	},
> +	/* ssd135x family */
> +	[SSD1351_ID] = {
> +		.default_width = 128,
> +		.default_height = 128,
> +		.format_rgb565 = 1,
> +		.family_id = SSD135X_FAMILY,
>  	}

While it's not a problem _in this case_, the rule of thumb is always to have a
trailing comma for non-terminator entry.

...

>  /*
>   * Helper to write data (SSD13XX_DATA) to the device.
>   */
> -static int ssd130x_write_data(struct ssd130x_device *ssd130x, u8 *values, int count)
> +static int ssd130x_write_data(struct ssd130x_device *ssd130x, const u8 *values, int count)
>  {
>  	return regmap_bulk_write(ssd130x->regmap, SSD13XX_DATA, values, count);
>  }

Stray change. If needed, either explain in the commit message or create
a separate patch (depending on the dependencies).

...

>  	unsigned int i;
>  	int ret;
>  
> +	/*
> +	 * The SSD135X family latches command parameters with D/C# HIGH (i.e.
> +	 * clocked in as data), unlike the other families where the opcode and
> +	 * all of its parameters are sent as commands (D/C# LOW). Send the
> +	 * opcode as a command and any following parameter bytes as data.
> +	 */
> +	if (ssd130x->device_info->family_id == SSD135X_FAMILY) {
> +		if (len == 0)
> +			return 0;
> +		ret = regmap_write(ssd130x->regmap, SSD13XX_COMMAND, cmd[0]);
> +		if (ret || len == 1)
> +			return ret;
> +
> +		return ssd130x_write_data(ssd130x, cmd + 1, len - 1);
> +	}

>  	for (i = 0; i < len; i++) {

This loop seems for the len, so it will be the same for both devices as far as
I can see the context. I can't find this piece in the original driver, perhaps
it's some dependency?

>  		ret = regmap_write(ssd130x->regmap, SSD13XX_COMMAND, cmd[i]);
>  		if (ret)

...

> +/*
> + * Variadic wrapper around ssd130x_write_cmds(). The first variadic argument is
> + * the command opcode and the following ones are its options/parameters.
> + */
> +static int ssd130x_write_cmd(struct ssd130x_device *ssd130x, int count,
> +			     /* u8 cmd, u8 option, ... */...)
> +{
> +	u8 buf[8];
> +	va_list ap;
> +	int i;
> +
> +	if (count > ARRAY_SIZE(buf))
> +		return -EINVAL;
> +
> +	va_start(ap, count);

> +	for (i = 0; i < count; i++)

Can be

	for (int i = 0; i < count; i++)

> +		buf[i] = va_arg(ap, int);
> +	va_end(ap);
> +
> +	return ssd130x_write_cmds(ssd130x, buf, count);
> +}

...

> +static int ssd135x_init(struct ssd130x_device *ssd130x)
> +{
> +	/*
> +	 * Horizontal address increment, COM split, reversed COM scan direction,
> +	 * BGR sub-pixel order and 65k (RGB565) color depth. Rotation is not
> +	 * supported, so the remap byte is fixed.
> +	 */
> +	u8 remap = SSD135X_SET_REMAP_65K | SSD135X_SET_REMAP_COM_SPLIT |
> +		   SSD135X_SET_REMAP_COLOR_BGR | SSD135X_SET_REMAP_COM_SCAN;

> +	const u8 cmds[] = {

Why not static?

> +		2, SSD135X_SET_COMMAND_LOCK, 0x12,
> +		2, SSD135X_SET_COMMAND_LOCK, 0xb1,
> +		1, SSD13XX_DISPLAY_OFF,
> +		2, SSD135X_SET_CLOCK_FREQ, 0xf1,
> +		2, SSD135X_SET_MUX_RATIO, ssd130x->height - 1,
> +		3, SSD135X_SET_COL_RANGE, 0x00, ssd130x->width - 1,
> +		3, SSD135X_SET_ROW_RANGE, 0x00, ssd130x->height - 1,
> +		2, SSD135X_SET_DISPLAY_START, 0x00,
> +		2, SSD135X_SET_DISPLAY_OFFSET, 0x00,
> +		2, SSD135X_SET_GPIO, 0x00,
> +		2, SSD135X_SET_FUNCTION, 0x01,
> +		2, SSD135X_SET_PHASE_LENGTH, 0x32,
> +		4, SSD135X_SET_VSL, 0xa0, 0xb5, 0x55,
> +		2, SSD135X_SET_PRECHARGE, 0x17,
> +		2, SSD135X_SET_VCOMH_VOLTAGE, 0x05,
> +		4, SSD135X_SET_CONTRAST, 0xc8, 0x80, 0xc8,
> +		2, SSD135X_SET_CONTRAST_MASTER, 0x0f,
> +		2, SSD135X_SET_PRECHARGE2, 0x01,
> +		1, SSD135X_SET_DISPLAY_NORMAL,
> +		2, SSD13XX_SET_SEG_REMAP, remap,

> +		0,

No trailing comma for the terminator entry.

> +	};
> +
> +	/*
> +	 * ssd130x_power_on() issues a short reset pulse, but the SSD1351 is not
> +	 * ready to accept commands immediately afterwards. Give the controller
> +	 * time to settle before sending the init sequence.
> +	 */

Any reference to the datasheet?

> +	msleep(120);
> +
> +	return ssd130x_run_cmd_seq(ssd130x, cmds);
> +}

...

> +/*
> + * Write a run of pixel data to the controller's display RAM. The SSD135X
> + * family requires an explicit Write RAM command once the address window has
> + * been set, before any pixel data is accepted; the SSD133X family enters data
> + * mode implicitly after the column/row range is programmed.
> + */
> +static int ssd133x_write_pixels(struct ssd130x_device *ssd130x,
> +				u8 *data_array, unsigned int count)
> +{
> +	if (ssd130x->device_info->family_id == SSD135X_FAMILY) {

> +		int ret = ssd130x_write_cmd(ssd130x, 1, SSD135X_WRITE_RAM);
> +
> +		if (ret < 0)
> +			return ret;

This style is discouraged as it's harder to maintain. Better to split
assignment and definition

		int ret;

		ret = ssd130x_write_cmd(ssd130x, 1, SSD135X_WRITE_RAM);
		if (ret < 0)
			return ret;

> +	}
> +
> +	return ssd130x_write_data(ssd130x, data_array, count);
> +}

...

> static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs[]

>  		.atomic_check = ssd133x_primary_plane_atomic_check,
>  		.atomic_update = ssd133x_primary_plane_atomic_update,
>  		.atomic_disable = ssd133x_primary_plane_atomic_disable,
> +	},
> +	[SSD135X_FAMILY] = {
> +		DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> +		.atomic_check = ssd133x_primary_plane_atomic_check,
> +		.atomic_update = ssd133x_primary_plane_atomic_update,
> +		.atomic_disable = ssd133x_primary_plane_atomic_disable,
>  	}

As per another similar case.

>  };

...

> static const struct drm_encoder_helper_funcs ssd130x_encoder_helper_funcs[] = {

>  	[SSD133X_FAMILY] = {
>  		.atomic_enable = ssd133x_encoder_atomic_enable,
>  		.atomic_disable = ssd130x_encoder_atomic_disable,
> +	},
> +	[SSD135X_FAMILY] = {
> +		.atomic_enable = ssd135x_encoder_atomic_enable,
> +		.atomic_disable = ssd130x_encoder_atomic_disable,
>  	}
>  };

Ditto.

...


> diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h
> index b0b487c06e04..da89d4455270 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.h
> +++ b/drivers/gpu/drm/solomon/ssd130x.h
> @@ -26,7 +26,8 @@
>  enum ssd130x_family_ids {
>  	SSD130X_FAMILY,
>  	SSD132X_FAMILY,
> -	SSD133X_FAMILY
> +	SSD133X_FAMILY,
> +	SSD135X_FAMILY

Ditto, and this is exactly the whole point why non-terminator entries should
have a trailing comma.

>  };

...

>  enum ssd130x_variants {

>  	SSD1327_ID,
>  	/* ssd133x family */
>  	SSD1331_ID,
> +	/* ssd135x family */
> +	SSD1351_ID,
>  	NR_SSD130X_VARIANTS

See the difference? Here is terminator, which is clear. The above cases are
not.

>  };

-- 
With Best Regards,
Andy Shevchenko