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

[PATCH v4 0/6] 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:

  - Patch 1 adds the device tree binding.

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

  - Patches 3 to 5 are preparatory cleanups requested on v3 [1]: constify
    the ssd130x_write_data() 'values' parameter, convert
    ssd130x_spi_id[] to C99 initializers, and reimplement
    ssd130x_write_cmd() as a variadic wrapper around
    ssd130x_write_cmds() so a single loop remains.

  - Patch 6 adds the SSD1351 as a new SSD135X_FAMILY. It gets its own
    primary plane update/disable, encoder enable and backlight
    callbacks; only the callbacks with no family-specific logic
    (ssd133x_primary_plane_atomic_check(), ssd133x_crtc_atomic_check()
    and ssd130x_encoder_atomic_disable()) are reused as is.

Testing:

  - Patches 1, 3, 4, 5 and 6 are tested on an SSD1351.
  - The SSD1331 RGB565 change (patch 2) was kindly tested by Javier on
    his SSD1331.

Based on drm-misc-next, as requested on v3, now that the ssd132x/ssd133x
column and row end address fixes have landed there.

Thanks to Javier, Andy, Krzysztof and Fabio for the reviews.

[1] v3 of this series:
    https://lore.kernel.org/dri-devel/20260704080925.75113-1-amit.barzilai22@gmail.com
[2] Command parameter path discussion:
    https://lore.kernel.org/dri-devel/20260811122603.30773-1-amit.barzilai22@gmail.com

---

Changes since v3:
- Rebase on drm-misc-next (per Javier). The v3 dependency on the
  ssd133x update_rect end-address fix is gone, it is upstream now.
- Split the ssd130x_write_data() constification into its own
  preparatory patch (per Javier), patch 3.
- Split the ssd130x_write_cmd()/ssd130x_write_cmds() unification into
  its own preparatory patch (per Javier), patch 5. It is a pure
  refactor: the bytes sent and the bus transactions used to send them
  are unchanged for every chip on both transports.
- New patch 4: convert ssd130x_spi_id[] to C99 initializers (per Andy).
- Patch 6: give the SSD135X family its own .atomic_update,
  .atomic_disable, encoder .atomic_enable and backlight callbacks
  instead of branching on the family inside the ssd133x ones, and drop
  the shared ssd133x_write_pixels() helper (per Javier).
- Patch 6: the SSD1351 command parameter path is now driven by a
  cmd_params_are_data flag in struct ssd130x_deviceinfo rather than a
  hardcoded family_id check (per Javier). It stays in ssd130x_write_cmds()
  instead of moving to ssd130x-spi.c as originally agreed; see [2] for
  the reasoning. In short, SSD13XX_COMMAND is the I2C control byte 0x80,
  whose Co=1 bit promises exactly one payload byte, so routing command
  buffers through regmap_raw_write() would have changed I2C command
  framing for every existing chip. Keeping the split in the core also
  avoids duplicating it in ssd130x-i2c.c if an I2C part ever needs it.
  Fabio reports the SSD1322 needs the same flag and plans to send
  SSD1322 support on top of this series.
- Patch 6: the SSD135X family now registers a backlight device like the
  other families, using ssd135x_set_contrast(). v3 skipped backlight
  registration for the family because SSD13XX_CONTRAST (0x81) does not
  exist on the SSD1351; the per-family backlight rework that landed
  upstream in the meantime made a per-family callback the natural fit.
  The SSD1351 takes all three color channels as parameters of a single
  command (0xc1), which ssd135x_set_contrast() handles.
- Patch 6: drop the post-reset delay comment in ssd130x_reset() (per
  Javier), it documented a mistake rather than the code.
- Collect Reviewed-by tags on patches 1, 2 and 3.

Amit Barzilai (6):
  dt-bindings: display: Add Solomon SSD1351 OLED controller
  drm/ssd130x: Change SSD133X color format to RGB565 from RGB332
  drm/ssd130x: Constify ssd130x_write_data() 'values' parameter
  drm/ssd130x: Replace positional ssd130x_spi_id[] initialization with
    C99
  drm/ssd130x: Implement ssd130x_write_cmd() on top of
    ssd130x_write_cmds()
  drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support

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


base-commit: 09b47186a4164f3aaa3591313f80794443117342
-- 
2.55.0

[PATCH v4 1/6] 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>
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.55.0

[PATCH v4 2/6] drm/ssd130x: Change SSD133X color format to RGB565 from RGB332

[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>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.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 0b0fc6fe3df2..1110aa63d066 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -146,6 +146,11 @@
 #define SSD133X_DEFAULT_CONTRAST_B		0x50
 #define SSD133X_DEFAULT_CONTRAST_C		0x7d
 
+/* 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[] = {
@@ -618,17 +623,17 @@ static int ssd133x_set_contrast(struct ssd130x_device *ssd130x, u32 brightness)
 static int ssd133x_init(struct ssd130x_device *ssd130x)
 {
 	int ret;
+	/*
+	 * 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_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,
@@ -829,14 +834,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 */
@@ -909,7 +920,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)
@@ -978,7 +989,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;
 
@@ -988,7 +999,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);
 
@@ -1446,7 +1457,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.55.0

Re: [PATCH v4 2/6] drm/ssd130x: Change SSD133X color format to RGB565 from RGB332

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing or undersized memory reservations for format conversion state in atomic check leading to atomic context violation.
- [High] Display hardware state desynchronization causing incomplete screen clears.
- [High] Uninitialized kernel memory disclosure to display panel due to silent conversion failure.
--

--- Patch [2]: [PATCH v4 2/6] drm/ssd130x: Change SSD133X color format to RGB565 from RGB332 ---
commit 8cc727648aa06ffb4256eb0ad7ba43c6e84be6d7
Author: Amit Barzilai <amit.barzilai22@gmail.com>

drm/ssd130x: Change SSD133X color format to RGB565 from RGB332

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

> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 0b0fc6fe3df23..1110aa63d0666 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c

[ ... ]

> @@ -909,7 +920,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;

[Severity: High]
This isn't a bug introduced by this patch, but does ssd133x_clear_screen() (and 
ssd132x_clear_screen()) clear the entire screen properly? 

Since the hardware update window (COL_RANGE and ROW_RANGE registers) is 
not reset before writing the full-screen zero payload, won't the hardware 
just wrap the zeroed payload within whatever stale damage rectangle was 
last configured, leaving the rest of the panel uncleared?
 
[ ... ]

> @@ -978,7 +989,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;
>  
> @@ -988,7 +999,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);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to unconditionally write the
data_array buffer to the hardware here?

In ssd133x_crtc_atomic_check(), data_array is allocated with kmalloc_array() 
which does not zero-initialize the memory. If drm_fb_xrgb8888_to_rgb565be()
fails and silently aborts, won't ssd133x_update_rect() then push 
uninitialized kernel heap memory directly to the physical display?

[ ... ]

> @@ -1446,7 +1457,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;

[Severity: High]
This isn't a bug introduced by this patch, but does this driver correctly manage memory 
reservations for format conversion to avoid atomic context violations?

Looking at ssd133x_primary_plane_atomic_check(), it completely omits a
call to drm_format_conv_state_reserve(). If the format conversion needs
intermediate buffers during the atomic update, it will dynamically provision
them using kmalloc(..., GFP_KERNEL) since the reservation is missing.

Similarly, ssd130x_primary_plane_atomic_check() and
ssd132x_primary_plane_atomic_check() seem to reserve undersized buffers 
using the destination pitch rather than the source/conversion line size 
requirement:

    buf = drm_format_conv_state_reserve(&shadow_plane_state->fmtcnv_state,
                                        pitch, GFP_KERNEL);

Won't these missing or undersized reservations cause sleeping functions to
be invoked during the non-blocking commit phase, violating atomic context
invariants?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818080626.30430-1-amit.barzilai22@gmail.com?part=2

Re: [PATCH v4 2/6] drm/ssd130x: Change SSD133X color format to RGB565 from RGB332

[Andy Shevchenko]

On Tue, Aug 18, 2026 at 11:06:22AM +0300, Amit Barzilai wrote:
> SSD133X screens were driven at 8bpp RGB332 despite supporting 16bpp RGB565.
> Switch the SSD133X data path to RGB565.

> +	/*
> +	 * 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_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,

Strictly speaking this is now initialisation of const object by non-const value
(yes, since it's a simple integer  there is nothing in between to modify it,
 but still...).

>  		2, SSD133X_SET_DISPLAY_START, 0x00,
>  		2, SSD133X_SET_DISPLAY_OFFSET, 0x00,
>  		1, SSD133X_SET_DISPLAY_NORMAL,

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 3/6] drm/ssd130x: Constify ssd130x_write_data() 'values' parameter

[Amit Barzilai]

At no point does ssd130x_write_data() change the bytes it receives via
the 'values' parameter. Constify the parameter so const-qualified
buffers pass through without casting away const.

Signed-off-by: Amit Barzilai <amit.barzilai22@gmail.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/solomon/ssd130x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 1110aa63d066..a229d7a87da0 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -252,7 +252,7 @@ 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);
 }
-- 
2.55.0

Re: [PATCH v4 3/6] drm/ssd130x: Constify ssd130x_write_data() 'values' parameter

[Andy Shevchenko]

On Tue, Aug 18, 2026 at 11:06:23AM +0300, Amit Barzilai wrote:
> At no point does ssd130x_write_data() change the bytes it receives via
> the 'values' parameter. Constify the parameter so const-qualified
> buffers pass through without casting away const.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 4/6] drm/ssd130x: Replace positional ssd130x_spi_id[] initialization with C99

[Amit Barzilai]

ssd130x_spi_id[] is initialized with positional initializers, which
easily create easy-to-miss bugs when changing the members of the target
struct (struct spi_device_id in this example).

Change this to C99 initializers to guarantee each member is initialized
with the correct value.

Signed-off-by: Amit Barzilai <amit.barzilai22@gmail.com>
---
 drivers/gpu/drm/solomon/ssd130x-spi.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x-spi.c b/drivers/gpu/drm/solomon/ssd130x-spi.c
index b52f5fd592a1..be50e5aa2c87 100644
--- a/drivers/gpu/drm/solomon/ssd130x-spi.c
+++ b/drivers/gpu/drm/solomon/ssd130x-spi.c
@@ -160,17 +160,17 @@ MODULE_DEVICE_TABLE(of, ssd130x_of_match);
  */
 static const struct spi_device_id ssd130x_spi_id[] = {
 	/* ssd130x family */
-	{ "sh1106",  SH1106_ID },
-	{ "ssd1305", SSD1305_ID },
-	{ "ssd1306", SSD1306_ID },
-	{ "ssd1307", SSD1307_ID },
-	{ "ssd1309", SSD1309_ID },
+	{ .name = "sh1106", .driver_data = SH1106_ID },
+	{ .name = "ssd1305", .driver_data = SSD1305_ID },
+	{ .name = "ssd1306", .driver_data = SSD1306_ID },
+	{ .name = "ssd1307", .driver_data = SSD1307_ID },
+	{ .name = "ssd1309", .driver_data = SSD1309_ID },
 	/* ssd132x family */
-	{ "ssd1322", SSD1322_ID },
-	{ "ssd1325", SSD1325_ID },
-	{ "ssd1327", SSD1327_ID },
+	{ .name = "ssd1322", .driver_data = SSD1322_ID },
+	{ .name = "ssd1325", .driver_data = SSD1325_ID },
+	{ .name = "ssd1327", .driver_data = SSD1327_ID },
 	/* ssd133x family */
-	{ "ssd1331", SSD1331_ID },
+	{ .name = "ssd1331", .driver_data = SSD1331_ID },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(spi, ssd130x_spi_id);
-- 
2.55.0

Re: [PATCH v4 4/6] drm/ssd130x: Replace positional ssd130x_spi_id[] initialization with C99

[Andy Shevchenko]

On Tue, Aug 18, 2026 at 11:06:24AM +0300, Amit Barzilai wrote:
> ssd130x_spi_id[] is initialized with positional initializers, which
> easily create easy-to-miss bugs when changing the members of the target
> struct (struct spi_device_id in this example).
> 
> Change this to C99 initializers to guarantee each member is initialized
> with the correct value.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

...

>  static const struct spi_device_id ssd130x_spi_id[] = {
>  	/* ssd130x family */
> +	{ .name = "sh1106", .driver_data = SH1106_ID },

Hmm... Is the comment correct for sh1106?

>  	/* ssd132x family */
>  	/* ssd133x family */

Overall, what do these comment bring us? Are those families have something in
common? Because the driver data is unique for each device, so I assume it's not
that compatible inside even the same families.

TL;DR: Perhaps simply drop those comments for once?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 5/6] drm/ssd130x: Implement ssd130x_write_cmd() on top of ssd130x_write_cmds()

[Amit Barzilai]

ssd130x_write_cmd() and ssd130x_write_cmds() each carried their own
regmap_write() loop over SSD13XX_COMMAND, differing only in how the caller
supplies the bytes.

Turn ssd130x_write_cmd() into a thin variadic wrapper that collects its
arguments into a small stack buffer and defers to ssd130x_write_cmds(), and
move the protocol comment onto the latter, which is where the loop now
lives.

No functional change: the bytes sent and the bus transactions used to send
them are identical for every chip on both the I2C and SPI transports.

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

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index a229d7a87da0..61187a630a0d 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -258,41 +258,12 @@ static int ssd130x_write_data(struct ssd130x_device *ssd130x, const u8 *values,
 }
 
 /*
- * Helper to write command (SSD13XX_COMMAND). The fist variadic argument
- * is the command to write and the following are the command options.
+ * Helper to write a command (SSD13XX_COMMAND) from a buffer. The first byte
+ * is the command opcode and the following ones are its parameters.
  *
- * 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.
+ * Note that the ssd13xx protocol requires the opcode and each parameter to
+ * be written as a SSD13XX_COMMAND device register value. That is why a call
+ * to regmap_write(..., SSD13XX_COMMAND, ...) is done for each byte.
  */
 static int ssd130x_write_cmds(struct ssd130x_device *ssd130x, const u8 *cmd,
 			      size_t len)
@@ -309,6 +280,32 @@ 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 are its parameters.
+ *
+ * The arguments are gathered into a fixed size buffer, so at most 8 bytes
+ * can be sent per call, i.e. an opcode and seven parameters. That covers
+ * every command this driver sends through it. Commands taking more parameters,
+ * such as the grey scale tables, must use ssd130x_write_cmds() instead.
+ */
+static int ssd130x_write_cmd(struct ssd130x_device *ssd130x, int count,
+			     /* u8 cmd, u8 param, ... */...)
+{
+	u8 buf[8];
+	va_list ap;
+
+	if (drm_WARN_ON(&ssd130x->drm, 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
-- 
2.55.0

Re: [PATCH v4 5/6] drm/ssd130x: Implement ssd130x_write_cmd() on top of ssd130x_write_cmds()

[Andy Shevchenko]

On Tue, Aug 18, 2026 at 11:06:25AM +0300, Amit Barzilai wrote:
> ssd130x_write_cmd() and ssd130x_write_cmds() each carried their own
> regmap_write() loop over SSD13XX_COMMAND, differing only in how the caller
> supplies the bytes.
> 
> Turn ssd130x_write_cmd() into a thin variadic wrapper that collects its
> arguments into a small stack buffer and defers to ssd130x_write_cmds(), and
> move the protocol comment onto the latter, which is where the loop now
> lives.
> 
> No functional change: the bytes sent and the bus transactions used to send
> them are identical for every chip on both the I2C and SPI transports.

...

> +static int ssd130x_write_cmd(struct ssd130x_device *ssd130x, int count,
> +			     /* u8 cmd, u8 param, ... */...)
> +{
> +	u8 buf[8];
> +	va_list ap;
> +
> +	if (drm_WARN_ON(&ssd130x->drm, count > ARRAY_SIZE(buf)))

For byte arrays sizeof() is sufficient.

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

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 6/6] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support

[Amit Barzilai]

The Solomon SSD1351 is a 128x128 RGB color OLED controller. It shares the
SSD133X pixel layout: one 65k color (RGB565) pixel per Segment, written as
a bulk transfer once a column/row addressing window has been programmed.
Add it as a new SSD135X_FAMILY rather than as a separate driver, so that
the ssd130x plane, CRTC and encoder infrastructure is reused.

Give the family its own primary plane update and disable, encoder enable
and backlight callbacks instead of teaching the ssd133x ones about a second
family. Only the callbacks that carry no family specific logic are reused
as is: ssd133x_primary_plane_atomic_check(), ssd133x_crtc_atomic_check()
and ssd130x_encoder_atomic_disable().

The data path differs from the ssd133x family in one respect. The SSD1351
only starts accepting pixel data after an explicit Write RAM command
(0x5c), while the SSD133X enters data mode as soon as the address window
has been programmed. Emit it from ssd135x_update_rect(), which both the
damage update and the clear screen paths go through.

SSD1351 differs from previous controllers in the command protocol. While
the opcode is still sent on the command path, the parameters are sent on
the data path. Introduce the cmd_params_are_data flag to struct
ssd130x_deviceinfo and let ssd130x_write_cmds() split the buffer in
accordance to the device specifications.

The SSD1351 also needs its own init sequence (ssd135x_init). The remap
byte is fixed at horizontal address increment, COM split, reversed COM
scan direction, BGR sub-pixel order and 65k color depth; rotation is not
supported.

Contrast is calibrated per color channel as for the ssd133x family, but
the three channels are parameters of a single command (0xc1) instead of
one command per channel. Add ssd135x_set_contrast() for that and use it
from both the init and the backlight update paths.

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-5
Signed-off-by: Amit Barzilai <amit.barzilai22@gmail.com>
---
 drivers/gpu/drm/solomon/ssd130x-spi.c |   7 +
 drivers/gpu/drm/solomon/ssd130x.c     | 308 +++++++++++++++++++++++++-
 drivers/gpu/drm/solomon/ssd130x.h     |  10 +-
 3 files changed, 318 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x-spi.c b/drivers/gpu/drm/solomon/ssd130x-spi.c
index be50e5aa2c87..c41064aa8221 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[] = {
 	{ .name = "ssd1327", .driver_data = SSD1327_ID },
 	/* ssd133x family */
 	{ .name = "ssd1331", .driver_data = SSD1331_ID },
+	/* ssd135x family */
+	{ .name = "ssd1351", .driver_data = 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 61187a630a0d..af1f8e21f159 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -151,6 +151,37 @@
 #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_VOLTAGE		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 A/B/C channel contrast at full brightness (white balance) */
+#define SSD135X_DEFAULT_CONTRAST_A		0xc8
+#define SSD135X_DEFAULT_CONTRAST_B		0x80
+#define SSD135X_DEFAULT_CONTRAST_C		0xc8
+
+/* ssd135x remap byte (data of SSD13XX_SET_SEG_REMAP) */
+#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[] = {
@@ -218,7 +249,14 @@ 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,
+		.cmd_params_are_data = true,
+	},
 };
 EXPORT_SYMBOL_NS_GPL(ssd130x_variants, "DRM_SSD130X");
 
@@ -261,9 +299,10 @@ static int ssd130x_write_data(struct ssd130x_device *ssd130x, const u8 *values,
  * Helper to write a command (SSD13XX_COMMAND) from a buffer. The first byte
  * is the command opcode and the following ones are its parameters.
  *
- * Note that the ssd13xx protocol requires the opcode and each parameter to
- * be written as a SSD13XX_COMMAND device register value. That is why a call
- * to regmap_write(..., SSD13XX_COMMAND, ...) is done for each byte.
+ * By default every byte is written as a SSD13XX_COMMAND register value, hence
+ * the regmap_write() per byte. Controllers that set cmd_params_are_data expect
+ * the parameters on the data path instead, so only the opcode is written as
+ * SSD13XX_COMMAND and the rest as SSD13XX_DATA.
  */
 static int ssd130x_write_cmds(struct ssd130x_device *ssd130x, const u8 *cmd,
 			      size_t len)
@@ -271,6 +310,17 @@ static int ssd130x_write_cmds(struct ssd130x_device *ssd130x, const u8 *cmd,
 	unsigned int i;
 	int ret;
 
+	if (ssd130x->device_info->cmd_params_are_data) {
+		if (!len)
+			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)
@@ -617,6 +667,20 @@ static int ssd133x_set_contrast(struct ssd130x_device *ssd130x, u32 brightness)
 	return ssd130x_run_cmd_seq(ssd130x, cmds);
 }
 
+/*
+ * Same white balance calibration as for the ssd133x family, except that the
+ * ssd135x controllers take the three channels as parameters of a single
+ * command instead of one command per channel.
+ */
+static int ssd135x_set_contrast(struct ssd130x_device *ssd130x, u32 brightness)
+{
+	u8 a = ssd130x_scale_contrast(SSD135X_DEFAULT_CONTRAST_A, brightness);
+	u8 b = ssd130x_scale_contrast(SSD135X_DEFAULT_CONTRAST_B, brightness);
+	u8 c = ssd130x_scale_contrast(SSD135X_DEFAULT_CONTRAST_C, brightness);
+
+	return ssd130x_write_cmd(ssd130x, 4, SSD135X_SET_CONTRAST, a, b, c);
+}
+
 static int ssd133x_init(struct ssd130x_device *ssd130x)
 {
 	int ret;
@@ -654,6 +718,47 @@ 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;
+	int ret;
+	const u8 cmds[] = {
+		/* Unlock the controller, then the extended command set */
+		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_VOLTAGE, 0x17,
+		2, SSD135X_SET_VCOMH_VOLTAGE, 0x05,
+		2, SSD135X_SET_CONTRAST_MASTER, 0x0f,
+		2, SSD135X_SET_PRECHARGE2, 0x01,
+		1, SSD135X_SET_DISPLAY_NORMAL,
+		2, SSD13XX_SET_SEG_REMAP, remap,
+		0,
+	};
+
+	ret = ssd130x_run_cmd_seq(ssd130x, cmds);
+	if (ret < 0)
+		return ret;
+
+	return ssd135x_set_contrast(ssd130x, ssd130x->contrast);
+}
+
 static int ssd130x_update_rect(struct ssd130x_device *ssd130x,
 			       struct drm_rect *rect, u8 *buf,
 			       u8 *data_array)
@@ -863,6 +968,44 @@ static int ssd133x_update_rect(struct ssd130x_device *ssd130x,
 	return ret;
 }
 
+static int ssd135x_update_rect(struct ssd130x_device *ssd130x,
+			       struct drm_rect *rect, u8 *data_array,
+			       unsigned int pitch)
+{
+	unsigned int x = rect->x1;
+	unsigned int y = rect->y1;
+	unsigned int columns = drm_rect_width(rect);
+	unsigned int rows = drm_rect_height(rect);
+	int ret;
+
+	/*
+	 * The pixel layout is the same as for the ssd133x family: one 65k
+	 * color (RGB565) pixel per Segment, sent Segment by Segment when the
+	 * (default) horizontal address increment mode is used.
+	 *
+	 * But unlike the ssd133x family, which starts accepting pixel data as
+	 * soon as the address window has been programmed, the ssd135x family
+	 * needs an explicit Write RAM command before the data is written.
+	 */
+
+	/* Set column start and end */
+	ret = ssd130x_write_cmd(ssd130x, 3, SSD135X_SET_COL_RANGE, x, x + columns - 1);
+	if (ret < 0)
+		return ret;
+
+	/* Set row start and end */
+	ret = ssd130x_write_cmd(ssd130x, 3, SSD135X_SET_ROW_RANGE, y, y + rows - 1);
+	if (ret < 0)
+		return ret;
+
+	ret = ssd130x_write_cmd(ssd130x, 1, SSD135X_WRITE_RAM);
+	if (ret < 0)
+		return ret;
+
+	/* Write out update in one go since horizontal addressing mode is used */
+	return ssd130x_write_data(ssd130x, data_array, pitch * rows);
+}
+
 static void ssd130x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
 {
 	unsigned int pages = DIV_ROUND_UP(ssd130x->height, SSD130X_PAGE_HEIGHT);
@@ -931,6 +1074,22 @@ static void ssd133x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
 	ssd130x_write_data(ssd130x, data_array, pitch * ssd130x->height);
 }
 
+static void ssd135x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
+{
+	struct drm_rect screen = DRM_RECT_INIT(0, 0, ssd130x->width, ssd130x->height);
+	const struct drm_format_info *fi = drm_format_info(DRM_FORMAT_RGB565);
+	unsigned int pitch;
+
+	if (!fi)
+		return;
+
+	pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
+
+	memset(data_array, 0, pitch * ssd130x->height);
+
+	ssd135x_update_rect(ssd130x, &screen, data_array, pitch);
+}
+
 static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb,
 				const struct iosys_map *vmap,
 				struct drm_rect *rect,
@@ -1003,6 +1162,29 @@ static int ssd133x_fb_blit_rect(struct drm_framebuffer *fb,
 	return 0;
 }
 
+static int ssd135x_fb_blit_rect(struct drm_framebuffer *fb,
+				const struct iosys_map *vmap,
+				struct drm_rect *rect, u8 *data_array,
+				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_RGB565);
+	unsigned int dst_pitch;
+	struct iosys_map dst;
+
+	if (!fi)
+		return -EINVAL;
+
+	dst_pitch = drm_format_info_min_pitch(fi, 0, drm_rect_width(rect));
+
+	iosys_map_set_vaddr(&dst, data_array);
+	drm_fb_xrgb8888_to_rgb565be(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);
+
+	ssd135x_update_rect(ssd130x, rect, data_array, dst_pitch);
+
+	return 0;
+}
+
 static int ssd130x_primary_plane_atomic_check(struct drm_plane *plane,
 					      struct drm_atomic_commit *state)
 {
@@ -1245,6 +1427,45 @@ static void ssd133x_primary_plane_atomic_update(struct drm_plane *plane,
 	drm_dev_exit(idx);
 }
 
+static void ssd135x_primary_plane_atomic_update(struct drm_plane *plane,
+						struct drm_atomic_commit *state)
+{
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
+	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
+	struct ssd130x_crtc_state *ssd130x_crtc_state =  to_ssd130x_crtc_state(crtc_state);
+	struct drm_framebuffer *fb = plane_state->fb;
+	struct drm_atomic_helper_damage_iter iter;
+	struct drm_device *drm = plane->dev;
+	struct drm_rect dst_clip;
+	struct drm_rect damage;
+	int idx;
+
+	if (!drm_dev_enter(drm, &idx))
+		return;
+
+	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE))
+		goto out_drm_dev_exit;
+
+	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
+	drm_atomic_for_each_plane_damage(&iter, &damage) {
+		dst_clip = plane_state->dst;
+
+		if (!drm_rect_intersect(&dst_clip, &damage))
+			continue;
+
+		ssd135x_fb_blit_rect(fb, &shadow_plane_state->data[0], &dst_clip,
+				     ssd130x_crtc_state->data_array,
+				     &shadow_plane_state->fmtcnv_state);
+	}
+
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
+
+out_drm_dev_exit:
+	drm_dev_exit(idx);
+}
+
 static void ssd130x_primary_plane_atomic_disable(struct drm_plane *plane,
 						 struct drm_atomic_commit *state)
 {
@@ -1317,6 +1538,30 @@ static void ssd133x_primary_plane_atomic_disable(struct drm_plane *plane,
 	drm_dev_exit(idx);
 }
 
+static void ssd135x_primary_plane_atomic_disable(struct drm_plane *plane,
+						 struct drm_atomic_commit *state)
+{
+	struct drm_device *drm = plane->dev;
+	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_crtc_state *crtc_state;
+	struct ssd130x_crtc_state *ssd130x_crtc_state;
+	int idx;
+
+	if (!plane_state->crtc)
+		return;
+
+	crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
+	ssd130x_crtc_state = to_ssd130x_crtc_state(crtc_state);
+
+	if (!drm_dev_enter(drm, &idx))
+		return;
+
+	ssd135x_clear_screen(ssd130x, ssd130x_crtc_state->data_array);
+
+	drm_dev_exit(idx);
+}
+
 /* Called during init to allocate the plane's atomic state. */
 static void ssd130x_primary_plane_reset(struct drm_plane *plane)
 {
@@ -1385,7 +1630,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 = ssd135x_primary_plane_atomic_update,
+		.atomic_disable = ssd135x_primary_plane_atomic_disable,
+	},
 };
 
 static const struct drm_plane_funcs ssd130x_primary_plane_funcs = {
@@ -1539,6 +1790,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 = {
@@ -1626,6 +1881,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)
 {
@@ -1651,7 +1931,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 = {
@@ -1728,6 +2012,15 @@ static int ssd133x_update_bl(struct backlight_device *bdev)
 	return ssd133x_set_contrast(ssd130x, ssd130x->contrast);
 }
 
+static int ssd135x_update_bl(struct backlight_device *bdev)
+{
+	struct ssd130x_device *ssd130x = bl_get_data(bdev);
+
+	ssd130x->contrast = backlight_get_brightness(bdev);
+
+	return ssd135x_set_contrast(ssd130x, ssd130x->contrast);
+}
+
 static const struct backlight_ops ssd130xfb_bl_ops[] = {
 	[SSD130X_FAMILY] = {
 		.update_status	= ssd130x_update_bl,
@@ -1738,6 +2031,9 @@ static const struct backlight_ops ssd130xfb_bl_ops[] = {
 	[SSD133X_FAMILY] = {
 		.update_status	= ssd133x_update_bl,
 	},
+	[SSD135X_FAMILY] = {
+		.update_status	= ssd135x_update_bl,
+	},
 };
 
 static void ssd130x_parse_properties(struct ssd130x_device *ssd130x)
diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h
index a4554018bb2a..fd81262f2ceb 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
 };
 
@@ -54,6 +57,11 @@ struct ssd130x_deviceinfo {
 	bool need_pwm;
 	bool need_chargepump;
 	bool page_mode_only;
+	/*
+	 * Whether the controller takes a command's parameters on the data path
+	 * instead of as further command bytes.
+	 */
+	bool cmd_params_are_data;
 
 	enum ssd130x_family_ids family_id;
 };
-- 
2.55.0

Re: [PATCH v4 6/6] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] `ssd133x_primary_plane_atomic_check` fails to call `drm_format_conv_state_reserve()`, forcing the subsequent atomic update phase to perform dynamic memory allocation with `GFP_KERNEL` in an atomic context.
- [High] Sleeping functions (e.g., mutexes, slow bus I/O) are called from Plane and Encoder atomic callbacks, violating DRM atomic context requirements.
- [High] Concurrent execution of sysfs backlight updates and DRM plane updates causes command stream corruption.
--

--- Patch [6]: [PATCH v4 6/6] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support ---
commit 0d606a373153c3406138083714dbc3321e2a839d
Author: Amit Barzilai <amit.barzilai22@gmail.com>

drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support

This commit introduces support for the Solomon SSD1351 RGB OLED controller
by adding a new SSD135X_FAMILY. It provides family-specific callbacks for
plane updates, disable paths, encoder enable paths, and backlight
management while accommodating the SSD1351's unique init sequence and
Write RAM command protocol.

> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 61187a630a0d5..af1f8e21f159a 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c

[ ... ]

> @@ -863,6 +968,44 @@ static int ssd133x_update_rect(struct ssd130x_device *ssd130x,
>  	return ret;
>  }
>  
> +static int ssd135x_update_rect(struct ssd130x_device *ssd130x,
> +			       struct drm_rect *rect, u8 *data_array,
> +			       unsigned int pitch)
> +{
> +	unsigned int x = rect->x1;
> +	unsigned int y = rect->y1;
> +	unsigned int columns = drm_rect_width(rect);
> +	unsigned int rows = drm_rect_height(rect);
> +	int ret;
> +
> +	/*
> +	 * The pixel layout is the same as for the ssd133x family: one 65k
> +	 * color (RGB565) pixel per Segment, sent Segment by Segment when the
> +	 * (default) horizontal address increment mode is used.
> +	 *
> +	 * But unlike the ssd133x family, which starts accepting pixel data as
> +	 * soon as the address window has been programmed, the ssd135x family
> +	 * needs an explicit Write RAM command before the data is written.
> +	 */
> +
> +	/* Set column start and end */
> +	ret = ssd130x_write_cmd(ssd130x, 3, SSD135X_SET_COL_RANGE, x, x + columns - 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set row start and end */
> +	ret = ssd130x_write_cmd(ssd130x, 3, SSD135X_SET_ROW_RANGE, y, y + rows - 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ssd130x_write_cmd(ssd130x, 1, SSD135X_WRITE_RAM);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Write out update in one go since horizontal addressing mode is used */
> +	return ssd130x_write_data(ssd130x, data_array, pitch * rows);
> +}

[Severity: High]
This is a pre-existing issue, but is this command sequence vulnerable to
concurrent access?

In ssd135x_update_rect(), a multi-step hardware programming sequence is
sent (column range, row range, Write RAM) followed by a bulk pixel data
transfer. There appears to be no driver-level lock synchronizing this
sequence.

If an unprivileged user concurrently writes to the sysfs backlight node, it
triggers ssd135x_update_bl(), sending a contrast command over the same bus.
If the contrast command interleaves after SSD135X_WRITE_RAM but before the
pixel data, could the OLED controller abort Write RAM mode and corrupt the
display output?

[ ... ]

> @@ -1245,6 +1427,45 @@ static void ssd133x_primary_plane_atomic_update(struct drm_plane *plane,
>  	drm_dev_exit(idx);
>  }
>  
> +static void ssd135x_primary_plane_atomic_update(struct drm_plane *plane,
> +						struct drm_atomic_commit *state)
> +{
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
> +	struct ssd130x_crtc_state *ssd130x_crtc_state =  to_ssd130x_crtc_state(crtc_state);
> +	struct drm_framebuffer *fb = plane_state->fb;
> +	struct drm_atomic_helper_damage_iter iter;
> +	struct drm_device *drm = plane->dev;
> +	struct drm_rect dst_clip;
> +	struct drm_rect damage;
> +	int idx;
> +
> +	if (!drm_dev_enter(drm, &idx))
> +		return;
> +
> +	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE))
> +		goto out_drm_dev_exit;

[Severity: High]
This is a pre-existing issue, but does this code sleep in an atomic
context?

The DRM subsystem guidelines specify that plane atomic update callbacks
execute in an atomic context where sleeping is forbidden. However,
ssd135x_primary_plane_atomic_update() calls drm_gem_fb_begin_cpu_access(),
which acquires a sleeping dma_resv ww_mutex lock.

Can this lead to kernel warnings or system hangs during display updates?

> +
> +	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
> +	drm_atomic_for_each_plane_damage(&iter, &damage) {
> +		dst_clip = plane_state->dst;
> +
> +		if (!drm_rect_intersect(&dst_clip, &damage))
> +			continue;
> +
> +		ssd135x_fb_blit_rect(fb, &shadow_plane_state->data[0], &dst_clip,
> +				     ssd130x_crtc_state->data_array,
> +				     &shadow_plane_state->fmtcnv_state);
> +	}

[Severity: High]
This is a pre-existing issue, but does this format conversion trigger a
GFP_KERNEL allocation in an atomic context?

Since ssd133x_primary_plane_atomic_check() (which is reused for this family)
does not call drm_format_conv_state_reserve() to pre-allocate memory, the
format conversion state is unallocated. When ssd135x_fb_blit_rect() is called
during the atomic update phase:

ssd135x_fb_blit_rect()
  drm_fb_xrgb8888_to_rgb565be()
    drm_fb_xfrm()

The underlying drm_fb_xfrm() function sees an unallocated state buffer and
dynamically allocates it using the GFP_KERNEL flag.

Could this cause sleeping in an atomic context and trigger kernel panics?

[ ... ]

> @@ -1626,6 +1881,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);

[Severity: High]
This is a pre-existing issue, but does this power-on sequence sleep during
an atomic enable callback?

Similar to the plane update callback, encoder atomic enable callbacks
execute in an atomic context. However, ssd135x_encoder_atomic_enable()
calls ssd130x_power_on(), which takes a mutex via regulator_enable(). It
also calls backlight_enable() which takes the backlight update_lock mutex,
and performs slow bus writes via ssd135x_init().

Are these sleeping operations safe here, or could they cause system
instability?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818080626.30430-1-amit.barzilai22@gmail.com?part=6

Re: [PATCH v4 6/6] drm/ssd130x: Add SSD135X_FAMILY and SSD1351 support

[Andy Shevchenko]

On Tue, Aug 18, 2026 at 11:06:26AM +0300, Amit Barzilai wrote:
> The Solomon SSD1351 is a 128x128 RGB color OLED controller. It shares the
> SSD133X pixel layout: one 65k color (RGB565) pixel per Segment, written as
> a bulk transfer once a column/row addressing window has been programmed.
> Add it as a new SSD135X_FAMILY rather than as a separate driver, so that
> the ssd130x plane, CRTC and encoder infrastructure is reused.
> 
> Give the family its own primary plane update and disable, encoder enable
> and backlight callbacks instead of teaching the ssd133x ones about a second
> family. Only the callbacks that carry no family specific logic are reused
> as is: ssd133x_primary_plane_atomic_check(), ssd133x_crtc_atomic_check()
> and ssd130x_encoder_atomic_disable().
> 
> The data path differs from the ssd133x family in one respect. The SSD1351
> only starts accepting pixel data after an explicit Write RAM command
> (0x5c), while the SSD133X enters data mode as soon as the address window
> has been programmed. Emit it from ssd135x_update_rect(), which both the
> damage update and the clear screen paths go through.
> 
> SSD1351 differs from previous controllers in the command protocol. While
> the opcode is still sent on the command path, the parameters are sent on
> the data path. Introduce the cmd_params_are_data flag to struct
> ssd130x_deviceinfo and let ssd130x_write_cmds() split the buffer in
> accordance to the device specifications.
> 
> The SSD1351 also needs its own init sequence (ssd135x_init). The remap
> byte is fixed at horizontal address increment, COM split, reversed COM
> scan direction, BGR sub-pixel order and 65k color depth; rotation is not
> supported.
> 
> Contrast is calibrated per color channel as for the ssd133x family, but
> the three channels are parameters of a single command (0xc1) instead of
> one command per channel. Add ssd135x_set_contrast() for that and use it
> from both the init and the backlight update paths.
> 
> The SSD1351 is SPI-only, so only the SPI transport match tables gain an
> entry; no new config symbol is needed.

...

>  static int ssd130x_write_cmds(struct ssd130x_device *ssd130x, const u8 *cmd,
>  			      size_t len)

>  	unsigned int i;
>  	int ret;
>  
> +	if (ssd130x->device_info->cmd_params_are_data) {
> +		if (!len)
> +			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)

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

Same comment about const.

> +	int ret;

Why not placing it after cmds?

> +	const u8 cmds[] = {
> +		/* Unlock the controller, then the extended command set */
> +		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_VOLTAGE, 0x17,
> +		2, SSD135X_SET_VCOMH_VOLTAGE, 0x05,
> +		2, SSD135X_SET_CONTRAST_MASTER, 0x0f,
> +		2, SSD135X_SET_PRECHARGE2, 0x01,
> +		1, SSD135X_SET_DISPLAY_NORMAL,
> +		2, SSD13XX_SET_SEG_REMAP, remap,
> +		0,
> +	};
> +
> +	ret = ssd130x_run_cmd_seq(ssd130x, cmds);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ssd135x_set_contrast(ssd130x, ssd130x->contrast);
> +}

...

> +static void ssd135x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
> +{
> +	struct drm_rect screen = DRM_RECT_INIT(0, 0, ssd130x->width, ssd130x->height);
> +	const struct drm_format_info *fi = drm_format_info(DRM_FORMAT_RGB565);
> +	unsigned int pitch;

> +	if (!fi)
> +		return;

It's less maintainable than

	const struct drm_format_info *fi;
	unsigned int pitch;

	fi = drm_format_info(DRM_FORMAT_RGB565);
	if (!fi)
		return;


> +	pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
> +
> +	memset(data_array, 0, pitch * ssd130x->height);
> +
> +	ssd135x_update_rect(ssd130x, &screen, data_array, pitch);
> +}

...

> +static int ssd135x_fb_blit_rect(struct drm_framebuffer *fb,
> +				const struct iosys_map *vmap,
> +				struct drm_rect *rect, u8 *data_array,
> +				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_RGB565);
> +	unsigned int dst_pitch;
> +	struct iosys_map dst;
> +
> +	if (!fi)
> +		return -EINVAL;

Ditto. The problem is that the current style is tempting for subtle mistakes
such as defining more variables that may use fi in between.

> +	dst_pitch = drm_format_info_min_pitch(fi, 0, drm_rect_width(rect));
> +
> +	iosys_map_set_vaddr(&dst, data_array);
> +	drm_fb_xrgb8888_to_rgb565be(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);
> +
> +	ssd135x_update_rect(ssd130x, rect, data_array, dst_pitch);
> +
> +	return 0;
> +}

...

> +static void ssd135x_primary_plane_atomic_disable(struct drm_plane *plane,
> +						 struct drm_atomic_commit *state)
> +{
> +	struct drm_device *drm = plane->dev;
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(drm);
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_crtc_state *crtc_state;
> +	struct ssd130x_crtc_state *ssd130x_crtc_state;
> +	int idx;
> +
> +	if (!plane_state->crtc)
> +		return;

In the similar way here.

> +	crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
> +	ssd130x_crtc_state = to_ssd130x_crtc_state(crtc_state);
> +
> +	if (!drm_dev_enter(drm, &idx))
> +		return;
> +
> +	ssd135x_clear_screen(ssd130x, ssd130x_crtc_state->data_array);
> +
> +	drm_dev_exit(idx);
> +}

-- 
With Best Regards,
Andy Shevchenko