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

Amit Barzilai [off-list ref] writes:

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

[...]

Can len even be 0? If that's the case then I guess that makes more sense
to check and bail out early regardless if cmd_params_are_data is true ?

For the !cmd_params_are_data case, the for loop will be a no-op anyways
but still I think is cleaner to check as the first thing in this function.

+		ret = regmap_write(ssd130x->regmap, SSD13XX_COMMAND, cmd[0]);
+		if (ret || len == 1)
+			return ret;
+

The len == 1 case is for commands that do not have parameters right? I
think that adding some comments explaining this to make it clear why
there is an early return.

I'm happy with the implementation now, thanks a lot for bearing with
me and your patience iterating over this series.

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat