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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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

...

> const struct ssd130x_deviceinfo ssd130x_variants[] = {

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

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

...

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

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

...

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

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

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

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

...

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

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

Can be

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

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

...

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

> +	const u8 cmds[] = {

Why not static?

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

> +		0,

No trailing comma for the terminator entry.

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

Any reference to the datasheet?

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

...

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

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

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

		int ret;

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

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

...

> static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs[]

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

As per another similar case.

>  };

...

> static const struct drm_encoder_helper_funcs ssd130x_encoder_helper_funcs[] = {

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

Ditto.

...


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

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

>  };

...

>  enum ssd130x_variants {

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

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

>  };

-- 
With Best Regards,
Andy Shevchenko