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
