mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v3 0/3] drm/ssd130x: Add support for the Solomon SSD1351 OLED controller
@ 2026-07-04  8:09 Amit Barzilai
  2026-07-04  8:09 ` [PATCH v3 1/3] dt-bindings: display: Add " Amit Barzilai
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Amit Barzilai @ 2026-07-04  8:09 UTC (permalink / raw)
  To: Javier Martinez Canillas, Andy Shevchenko, dri-devel
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, devicetree, linux-kernel, Amit Barzilai

This series adds support for the Solomon SSD1351, a 128x128 65k-color
RGB OLED controller, to the ssd130x DRM driver:

  - Patch 1 adds the device tree binding.

  - Patch 2 switches the SSD133X family from RGB332 to RGB565, bringing
    65k color to the SSD1331.

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

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 [2]; 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.

Backlight:

  While adding the SSD1351 I noticed that the shared backlight path
  (ssd130x_update_bl()) is only correct for the SSD130X and SSD132X
  families, where 0x81 is the contrast command. On the SSD133X, 0x81 is
  "Set Contrast for Color A", so brightness changes shift the color
  balance rather than dim the panel. On the SSD1351, 0x81 is not
  implemented at all and the brightness byte itself would be executed
  as a command opcode (e.g. 0xae is Display OFF). This series therefore
  does not register a backlight device for the SSD135X family. I plan a
  follow-up making the backlight path family-aware (scaling
  0x81/0x82/0x83 together for SSD133X, 0xc1 contrast A/B/C for
  SSD135X), which would also fix the existing SSD1331 behavior. Happy
  to reorder if you would prefer that rework to land first.

Based on drm-misc-next. Note that the series depends on
ssd130x_run_cmd_seq() (commit 208211646fb3 ("drm/solomon: add
ssd130x_run_cmd_seq() for batch command execution")), which is in
drm-misc-next but not yet in mainline.

Thanks to Javier, Andy and Krzysztof for the reviews.

[1] v2 of this series:
    https://lore.kernel.org/dri-devel/20260622152506.78627-1-amit.barzilai22@gmail.com
[2] ssd132x/ssd133x update_rect end-address fix:
    https://lore.kernel.org/dri-devel/20260622122604.32500-1-amit.barzilai22@gmail.com

---

Changes since v2 [1]:
- Drop patch 4 (staging fbtft fb_ssd1351 removal); it was agreed that
  the fbtft driver stays.
- Patch 2: remove RGB332 support entirely and drive the SSD133X family
  at RGB565 unconditionally, instead of adding a per-variant format
  flag to the deviceinfo struct (per Javier). Retitled accordingly.
- Patch 3: drop the 120 ms post-reset msleep(); it was a generic fbtft
  value, not an SSD1351 datasheet figure. Retested on hardware without
  it. A comment noting this is left in ssd130x_reset() (per Andy).
- Patch 3: explain the ssd130x_write_data() constification in the
  commit message (per Andy).
- Add trailing commas to non-terminator array/enum entries and drop
  the comma from the init-sequence terminator (per Andy).
- Declare the loop variable inside the for statement in
  ssd130x_write_cmd() (per Andy).
- Split declaration and assignment of ret in ssd133x_write_pixels()
  (per Andy).
- Fix comment wording/plurals ("3, 3 and 2 bits") (per Andy).
- Patch 3: skip backlight device registration for the SSD135X family;
  the shared backlight path would execute the brightness byte as a
  command opcode on the SSD1351. To be addressed by a follow-up making
  the backlight path family-aware (see the Backlight section above).
- Collect Reviewed-by tags on patch 1.

Amit Barzilai (3):
  dt-bindings: display: Add Solomon SSD1351 OLED controller
  drm/ssd130x: Change SSD133X color format to RGB565 from RGB332
  drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support

 .../bindings/display/solomon,ssd1351.yaml     |  42 +++
 drivers/gpu/drm/solomon/ssd130x-spi.c         |   7 +
 drivers/gpu/drm/solomon/ssd130x.c             | 289 ++++++++++++++----
 drivers/gpu/drm/solomon/ssd130x.h             |   5 +-
 4 files changed, 278 insertions(+), 65 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/solomon,ssd1351.yaml

--
2.54.0

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3 1/3] dt-bindings: display: Add Solomon SSD1351 OLED controller
  2026-07-04  8:09 [PATCH v3 0/3] drm/ssd130x: Add support for the Solomon SSD1351 OLED controller Amit Barzilai
@ 2026-07-04  8:09 ` Amit Barzilai
  2026-07-04  8:09 ` [PATCH v3 2/3] drm/ssd130x: Change SSD133X color format to RGB565 from RGB332 Amit Barzilai
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Amit Barzilai @ 2026-07-04  8:09 UTC (permalink / raw)
  To: Javier Martinez Canillas, Andy Shevchenko, dri-devel
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, devicetree, linux-kernel, Amit Barzilai,
	Krzysztof Kozlowski

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>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3 2/3] drm/ssd130x: Change SSD133X color format to RGB565 from RGB332
  2026-07-04  8:09 [PATCH v3 0/3] drm/ssd130x: Add support for the Solomon SSD1351 OLED controller Amit Barzilai
  2026-07-04  8:09 ` [PATCH v3 1/3] dt-bindings: display: Add " Amit Barzilai
@ 2026-07-04  8:09 ` Amit Barzilai
  2026-07-16  8:36   ` Javier Martinez Canillas
  2026-07-04  8:09 ` [PATCH v3 3/3] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support Amit Barzilai
  2026-07-06 10:46 ` [PATCH v3 0/3] drm/ssd130x: Add support for the Solomon SSD1351 OLED controller Javier Martinez Canillas
  3 siblings, 1 reply; 8+ messages in thread
From: Amit Barzilai @ 2026-07-04  8:09 UTC (permalink / raw)
  To: Javier Martinez Canillas, Andy Shevchenko, dri-devel
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, devicetree, linux-kernel, Amit Barzilai

SSD133X screens were driven at 8bpp RGB332 despite supporting 16bpp RGB565.
Switch the SSD133X data path to RGB565.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Amit Barzilai <amit.barzilai22@gmail.com>
---
 drivers/gpu/drm/solomon/ssd130x.c | 45 +++++++++++++++++++------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 04da4f2f7d08..3f09977d227b 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -140,6 +140,11 @@
 #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_65K			0x1
+
 #define MAX_CONTRAST 255
 
 const struct ssd130x_deviceinfo ssd130x_variants[] = {
@@ -584,6 +589,12 @@ static int ssd132x_init(struct ssd130x_device *ssd130x)
 
 static int ssd133x_init(struct ssd130x_device *ssd130x)
 {
+	/*
+	 * Horizontal address increment, normal SA,SB,SC (e.g. RGB) sub-pixel
+	 * order, COM split odd even and 65k (RGB565) color depth.
+	 */
+	u8 remap = SSD133X_SET_REMAP_COM_SPLIT |
+		   FIELD_PREP(SSD133X_SET_REMAP_COLOR_DEPTH_MASK, SSD133X_COLOR_DEPTH_65K);
 	const u8 cmds[] = {
 		2, SSD133X_CONTRAST_A, 0x91,
 		2, SSD133X_CONTRAST_B, 0x50,
@@ -591,13 +602,7 @@ static int ssd133x_init(struct ssd130x_device *ssd130x)
 		2, SSD133X_SET_MASTER_CURRENT, 0x06,
 		3, SSD133X_SET_COL_RANGE, 0x00, ssd130x->width - 1,
 		3, SSD133X_SET_ROW_RANGE, 0x00, ssd130x->height - 1,
-		/*
-		 * Horizontal Address Increment
-		 * Normal order SA,SB,SC (e.g. RGB)
-		 * COM Split Odd Even
-		 * 256 color format
-		 */
-		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 +799,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.
+	 * sub-pixels for color A, B and C. These have 3, 3 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, 6 and 5 bits
+	 * respectively.
 	 */
 
 	/* Set column start and end */
@@ -874,7 +885,7 @@ static void ssd132x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
 
 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 = drm_format_info(DRM_FORMAT_RGB565);
 	unsigned int pitch;
 
 	if (!fi)
@@ -945,7 +956,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 = drm_format_info(DRM_FORMAT_RGB565);
 	unsigned int dst_pitch;
 	struct iosys_map dst;
 	int ret = 0;
@@ -956,7 +967,7 @@ 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);
+	drm_fb_xrgb8888_to_rgb565be(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);
 
 	ssd133x_update_rect(ssd130x, rect, data_array, dst_pitch);
 
@@ -1414,7 +1425,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 = drm_format_info(DRM_FORMAT_RGB565);
 	unsigned int pitch;
 	int ret;
 
-- 
2.54.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3 3/3] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support
  2026-07-04  8:09 [PATCH v3 0/3] drm/ssd130x: Add support for the Solomon SSD1351 OLED controller Amit Barzilai
  2026-07-04  8:09 ` [PATCH v3 1/3] dt-bindings: display: Add " Amit Barzilai
  2026-07-04  8:09 ` [PATCH v3 2/3] drm/ssd130x: Change SSD133X color format to RGB565 from RGB332 Amit Barzilai
@ 2026-07-04  8:09 ` Amit Barzilai
  2026-07-16  9:35   ` Andy Shevchenko
  2026-07-16 10:23   ` Javier Martinez Canillas
  2026-07-06 10:46 ` [PATCH v3 0/3] drm/ssd130x: Add support for the Solomon SSD1351 OLED controller Javier Martinez Canillas
  3 siblings, 2 replies; 8+ messages in thread
From: Amit Barzilai @ 2026-07-04  8:09 UTC (permalink / raw)
  To: Javier Martinez Canillas, Andy Shevchenko, dri-devel
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, devicetree, linux-kernel, 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. The re-map byte is fixed at 0 degrees,
65k color, COM split, BGR sub-pixel order; rotation is not supported.

The SSD1351 doesn't implement the SSD13XX_CONTRAST command (0x81) and
expects different opcodes for contrast and brightness management. The
current ssd130x_update_bl() function wouldn't work and the brightness
byte would be executed as a command opcode if run on the SSD1351 panel.
Until the backlight path is family-aware, skip backlight device
registration for the SSD135X family.

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

While adding the SSD135X command path, ssd130x_write_cmds() forwards its
'const u8 *cmd' buffer to ssd130x_write_data(), which previously took a
non-const 'u8 *'. Constify the ssd130x_write_data() 'values' parameter
so the const-qualified buffer passes through without casting away const.

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     | 244 +++++++++++++++++++++-----
 drivers/gpu/drm/solomon/ssd130x.h     |   5 +-
 3 files changed, 208 insertions(+), 48 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 3f09977d227b..f7f0a74a297e 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -145,6 +145,33 @@
 #define SSD133X_SET_REMAP_COLOR_DEPTH_MASK	GENMASK(7, 6)
 #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[] = {
@@ -212,7 +239,13 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
 		.default_width = 96,
 		.default_height = 64,
 		.family_id = SSD133X_FAMILY,
-	}
+	},
+	/* ssd135x family */
+	[SSD1351_ID] = {
+		.default_width = 128,
+		.default_height = 128,
+		.family_id = SSD135X_FAMILY,
+	},
 };
 EXPORT_SYMBOL_NS_GPL(ssd130x_variants, "DRM_SSD130X");
 
@@ -246,47 +279,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)
@@ -294,6 +296,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)
@@ -303,6 +321,27 @@ 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;
+
+	if (count > ARRAY_SIZE(buf))
+		return -EINVAL;
+
+	va_start(ap, count);
+	for (int 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
@@ -415,6 +454,12 @@ static void ssd130x_reset(struct ssd130x_device *ssd130x)
 	udelay(4);
 	gpiod_set_value_cansleep(ssd130x->reset, 0);
 	udelay(4);
+	/*
+	 * No long post-reset delay: the controller datasheets only specify
+	 * microsecond-scale reset timing. The lengthy delay seen in some other
+	 * drivers comes from fbtft's generic reset helper and targets slow
+	 * parallel-bus GPIOs, which do not apply here.
+	 */
 }
 
 static int ssd130x_power_on(struct ssd130x_device *ssd130x)
@@ -622,6 +667,42 @@ 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
+	};
+
+	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)
@@ -784,6 +865,26 @@ 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;
+
+		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)
@@ -826,7 +927,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;
 }
@@ -896,7 +997,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,
@@ -1356,7 +1457,13 @@ 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,
+	},
 };
 
 static const struct drm_plane_funcs ssd130x_primary_plane_funcs = {
@@ -1510,6 +1617,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 = {
@@ -1597,6 +1708,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)
 {
@@ -1622,7 +1758,11 @@ 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,
+	},
 };
 
 static const struct drm_encoder_funcs ssd130x_encoder_funcs = {
@@ -1897,15 +2037,25 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
 	if (ret)
 		return ERR_PTR(ret);
 
-	bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
-					    &ssd130xfb_bl_ops, NULL);
-	if (IS_ERR(bl))
-		return ERR_PTR(dev_err_probe(dev, PTR_ERR(bl),
-					     "Unable to register backlight device\n"));
+	/*
+	 * The backlight update path drives contrast through the
+	 * SSD13XX_CONTRAST command (0x81), which the SSD135X family does not
+	 * implement; the brightness byte would be interpreted as a command
+	 * opcode instead. Do not register a backlight device for this family
+	 * until the backlight path is family-aware.
+	 */
+	if (ssd130x->device_info->family_id != SSD135X_FAMILY) {
+		bl = devm_backlight_device_register(dev, dev_name(dev), dev,
+						    ssd130x, &ssd130xfb_bl_ops,
+						    NULL);
+		if (IS_ERR(bl))
+			return ERR_PTR(dev_err_probe(dev, PTR_ERR(bl),
+						     "Unable to register backlight device\n"));
 
-	bl->props.brightness = ssd130x->contrast;
-	bl->props.max_brightness = MAX_CONTRAST;
-	ssd130x->bl_dev = bl;
+		bl->props.brightness = ssd130x->contrast;
+		bl->props.max_brightness = MAX_CONTRAST;
+		ssd130x->bl_dev = bl;
+	}
 
 	ret = ssd130x_init_modeset(ssd130x);
 	if (ret)
diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h
index a4554018bb2a..350472cc5a12 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 0/3] drm/ssd130x: Add support for the Solomon SSD1351 OLED controller
  2026-07-04  8:09 [PATCH v3 0/3] drm/ssd130x: Add support for the Solomon SSD1351 OLED controller Amit Barzilai
                   ` (2 preceding siblings ...)
  2026-07-04  8:09 ` [PATCH v3 3/3] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support Amit Barzilai
@ 2026-07-06 10:46 ` Javier Martinez Canillas
  3 siblings, 0 replies; 8+ messages in thread
From: Javier Martinez Canillas @ 2026-07-06 10:46 UTC (permalink / raw)
  To: Amit Barzilai, Andy Shevchenko, dri-devel
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, devicetree, linux-kernel, Amit Barzilai

Amit Barzilai <amit.barzilai22@gmail.com> writes:

Hello Amit,

> This series adds support for the Solomon SSD1351, a 128x128 65k-color
> RGB OLED controller, to the ssd130x DRM driver:
>
>   - Patch 1 adds the device tree binding.
>
>   - Patch 2 switches the SSD133X family from RGB332 to RGB565, bringing
>     65k color to the SSD1331.
>
>   - 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.
>

Great, this approach looks correct to me now. I'll review this series but
likely will do it in a few days.

> 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 [2]; 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.

I just pushed this series to drm-misc-next, please rebase on top of that
when posting a new version.

>
> Backlight:
>
>   While adding the SSD1351 I noticed that the shared backlight path
>   (ssd130x_update_bl()) is only correct for the SSD130X and SSD132X
>   families, where 0x81 is the contrast command. On the SSD133X, 0x81 is
>   "Set Contrast for Color A", so brightness changes shift the color
>   balance rather than dim the panel. On the SSD1351, 0x81 is not
>   implemented at all and the brightness byte itself would be executed
>   as a command opcode (e.g. 0xae is Display OFF). This series therefore
>   does not register a backlight device for the SSD135X family. I plan a
>   follow-up making the backlight path family-aware (scaling
>   0x81/0x82/0x83 together for SSD133X, 0xc1 contrast A/B/C for
>   SSD135X), which would also fix the existing SSD1331 behavior. Happy
>   to reorder if you would prefer that rework to land first.
>

Oh, interesting. I think your call is the right one here and this
backlight logic fix / rework can be done as a follow-up series.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/3] drm/ssd130x: Change SSD133X color format to RGB565 from RGB332
  2026-07-04  8:09 ` [PATCH v3 2/3] drm/ssd130x: Change SSD133X color format to RGB565 from RGB332 Amit Barzilai
@ 2026-07-16  8:36   ` Javier Martinez Canillas
  0 siblings, 0 replies; 8+ messages in thread
From: Javier Martinez Canillas @ 2026-07-16  8:36 UTC (permalink / raw)
  To: Amit Barzilai, Andy Shevchenko, dri-devel
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, devicetree, linux-kernel, Amit Barzilai

Amit Barzilai <amit.barzilai22@gmail.com> writes:

Hello Amit,

> SSD133X screens were driven at 8bpp RGB332 despite supporting 16bpp RGB565.
> Switch the SSD133X data path to RGB565.
>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Amit Barzilai <amit.barzilai22@gmail.com>
> ---

Patch looks good to me now. Thanks for the respin.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Could you please rebase on top of latest drm-misc-next, now that your
column/row fixes landed? Then I should be able to apply this patchset.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 3/3] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support
  2026-07-04  8:09 ` [PATCH v3 3/3] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support Amit Barzilai
@ 2026-07-16  9:35   ` Andy Shevchenko
  2026-07-16 10:23   ` Javier Martinez Canillas
  1 sibling, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2026-07-16  9:35 UTC (permalink / raw)
  To: Amit Barzilai
  Cc: Javier Martinez Canillas, dri-devel, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, devicetree,
	linux-kernel

On Sat, Jul 04, 2026 at 11:09:25AM +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. The re-map byte is fixed at 0 degrees,
> 65k color, COM split, BGR sub-pixel order; rotation is not supported.
> 
> The SSD1351 doesn't implement the SSD13XX_CONTRAST command (0x81) and
> expects different opcodes for contrast and brightness management. The
> current ssd130x_update_bl() function wouldn't work and the brightness
> byte would be executed as a command opcode if run on the SSD1351 panel.
> Until the backlight path is family-aware, skip backlight device
> registration for the SSD135X family.
> 
> The SSD1351 is SPI-only, so only the SPI transport match tables gain an
> entry; no new config symbol is needed.
> 
> While adding the SSD135X command path, ssd130x_write_cmds() forwards its
> 'const u8 *cmd' buffer to ssd130x_write_data(), which previously took a
> non-const 'u8 *'. Constify the ssd130x_write_data() 'values' parameter
> so the const-qualified buffer passes through without casting away const.

...

>  	{ "ssd1327", SSD1327_ID },
>  	/* ssd133x family */
>  	{ "ssd1331", SSD1331_ID },
> +	/* ssd135x family */
> +	{ "ssd1351", SSD1351_ID },
>  	{ /* sentinel */ }

To avoid conflicts it's better to convert this ID table to use C99 initialisers
(so Uwe will be happy).

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 3/3] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support
  2026-07-04  8:09 ` [PATCH v3 3/3] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support Amit Barzilai
  2026-07-16  9:35   ` Andy Shevchenko
@ 2026-07-16 10:23   ` Javier Martinez Canillas
  1 sibling, 0 replies; 8+ messages in thread
From: Javier Martinez Canillas @ 2026-07-16 10:23 UTC (permalink / raw)
  To: Amit Barzilai, Andy Shevchenko, dri-devel
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, devicetree, linux-kernel, Amit Barzilai

Amit Barzilai <amit.barzilai22@gmail.com> writes:

Hello Amit,

Thanks for the patch. Unfortunately I think that there is still some work
to be done for this patch to land. Inline comments below.

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

[...]

>   */
> -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);
>  }

>

This change is correct but need to be split in a separate preparatory
patch. When you do that, feel free to add my Reviewed-by tag.

> -/*
> - * 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;
> -}
> -

Please split these ssd130x_write_cmds() and ssd1330x_write_cmd() refactoring
as separate preparatory patches. This patch is changing too many things at
once, it is better to have each logical changes as a separate patch. This
makes reviewer much easier.

>  /*
>   * 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)
> @@ -294,6 +296,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);
> +	}
> +

I don't like that this logic is in the ssd130x core part of the driver. This
is supposed to be transport agonistic and should not be aware of the D/C#
behaviour that is specific to the SPI transport.

Can we move this to the ssd130x-spi driver? For example, something like the
following might work:

1. Make ssd130x_write_cmds() to just be a static inline wrapper that calls
   to regmap_raw_write(ssd130x->regmap, SSD13XX_COMMAND, cmd, len).

2. Make ssd130x_write_cmd() be a variadic wrapper around ssd130x_write_cmds().

3. Add your logic to ssd130x_spi_write() instead of ssd130x_write_cmds(), that
   way it stays in the correct layer rather than having a leaking abstraction.

Also, instead of checking for info->family_id == SSD135X_FAMILY, we could add
a dc_high_params member (or whatever name is more suitable) to the struct
ssd130x_spi_transport Then other families that might use the same can just
reuse this option instead of checking for specific families.

[...]

>  /*
>   * 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
> @@ -415,6 +454,12 @@ static void ssd130x_reset(struct ssd130x_device *ssd130x)
>  	udelay(4);
>  	gpiod_set_value_cansleep(ssd130x->reset, 0);
>  	udelay(4);
> +	/*
> +	 * No long post-reset delay: the controller datasheets only specify
> +	 * microsecond-scale reset timing. The lengthy delay seen in some other
> +	 * drivers comes from fbtft's generic reset helper and targets slow
> +	 * parallel-bus GPIOs, which do not apply here.
> +	 */
>  }
>

I don't understand the point of this comment. You are not changing the logic
of this function nor I see how this comment is useful.

[...]
  
> +/*
> + * 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;
> +
> +		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)
> @@ -826,7 +927,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;
>  }
> @@ -896,7 +997,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);
>  }
>  

Instead of ssd133x_write_pixels() and having per family logic in the primary
place update path, I prefer to have a little bit more of code duplication
and add an .atomic_update, .atomic_disable, etc for the SSD1351.

Yes, I know that your current approach reduces code duplication but also
makes it harder to change the primary plane update path for a family without
affecting another one. This is particularly important in my opinion, given
that contributors usually just test on their display family when posting
the patches. So I prefer to have per family callbacks if possible.

[...]

>  static const struct drm_encoder_funcs ssd130x_encoder_funcs = {
> @@ -1897,15 +2037,25 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> -	bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
> -					    &ssd130xfb_bl_ops, NULL);
> -	if (IS_ERR(bl))
> -		return ERR_PTR(dev_err_probe(dev, PTR_ERR(bl),
> -					     "Unable to register backlight device\n"));
> +	/*
> +	 * The backlight update path drives contrast through the
> +	 * SSD13XX_CONTRAST command (0x81), which the SSD135X family does not
> +	 * implement; the brightness byte would be interpreted as a command
> +	 * opcode instead. Do not register a backlight device for this family
> +	 * until the backlight path is family-aware.
> +	 */
> +	if (ssd130x->device_info->family_id != SSD135X_FAMILY) {
> +		bl = devm_backlight_device_register(dev, dev_name(dev), dev,
> +						    ssd130x, &ssd130xfb_bl_ops,
> +						    NULL);
> +		if (IS_ERR(bl))
> +			return ERR_PTR(dev_err_probe(dev, PTR_ERR(bl),
> +						     "Unable to register backlight device\n"));
>
> -	bl->props.brightness = ssd130x->contrast;
> -	bl->props.max_brightness = MAX_CONTRAST;
> -	ssd130x->bl_dev = bl;
> +		bl->props.brightness = ssd130x->contrast;
> +		bl->props.max_brightness = MAX_CONTRAST;
> +		ssd130x->bl_dev = bl;
> +	}

Again I wonder if is better to have a device_info member to decide whether to
register the backlight, instead of harcoding the check for a specific family.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-07-16 10:23 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-07-04  8:09 [PATCH v3 0/3] drm/ssd130x: Add support for the Solomon SSD1351 OLED controller Amit Barzilai
2026-07-04  8:09 ` [PATCH v3 1/3] dt-bindings: display: Add " Amit Barzilai
2026-07-04  8:09 ` [PATCH v3 2/3] drm/ssd130x: Change SSD133X color format to RGB565 from RGB332 Amit Barzilai
2026-07-16  8:36   ` Javier Martinez Canillas
2026-07-04  8:09 ` [PATCH v3 3/3] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support Amit Barzilai
2026-07-16  9:35   ` Andy Shevchenko
2026-07-16 10:23   ` Javier Martinez Canillas
2026-07-06 10:46 ` [PATCH v3 0/3] drm/ssd130x: Add support for the Solomon SSD1351 OLED controller Javier Martinez Canillas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox