Re: [PATCH 2/2] drm/ssd130x: Add SSD1322_FAMILY and move the SSD1322 to it

[Javier Martinez Canillas]

Fabio Piparo <holofermes@xxxxxxxxx> writes:

Hello Fabio,

Thanks a lot for your patch and sorry for the delay with the review...

> The SSD1322 has been in the match tables since commit fdd591e00a9c
> ("drm/ssd130x: Add support for the SSD132x OLED controller family") as
> a member of the ssd132x family, which was brought up on an SSD1327. The
> two controllers share the 4-bit grayscale RAM layout and many opcodes,
> but the ssd132x paths cannot produce an image on an SSD1322:
>

Yes, I wrongly assumed that SSD1322, SSD1325 and SSD1327 were similar.
As you point out, I tested with SSD1327 because it was the only SSD132x
panel I had. I *think* that also checked the SSD1325 datasheet and saw
that were similar to SSD1327. But I did not check for SSD1322 indeed.

> - A command's parameters travel on the data path, as on the SSD1351,
>   so every multi-byte command in ssd132x_init() is read as a run of
>   single opcodes.
> - Some of the opcodes differ: 0xa4 is "entire display off" on the
>   SSD1322 and normal display mode is 0xa6, the multiplex ratio command
>   is 0xca, the contrast command 0xc1 and the precharge voltage command
>   0xbb.
> - A column address covers four Segments instead of two, the RAM only
>   accepts pixel data after a Write RAM command (0x5c), and with nibble
>   remap the leftmost pixel of a pair is the high nibble.
>

Thanks for this summary, it is very helpful to review your changes.

> Give the SSD1322 its own family, shaped like ssd135x: an init
> table, a Write RAM before the pixel data, a contrast command with its
> value in the data phase, and its own plane update, encoder enable and
> backlight callbacks.
>

I don't think that like this approach. When I conceived the per-family
split, I wanted to use the same "families" that Solomon chose for their
display controllers.

As far as I can tell, the part numbering depends on the pixel formats
that are supported by the devices. That is:

  * SSD130x: monochrome
  * SSD132x: 16 gray scale
  * SSD133x: 65k color
  * SSD135x: 262k color

So I would like the driver's family split to match the HW grouping.

Having a family for a single device would break this convention. Also,
your SSD1322_FAMILY reuses many of the ssd132x helpers and the ssd1322
ones you are adding are just a slight variation of the existing ones.

Instead, what about adding an .init and .set_contrast function pointers
to struct ssd130x_deviceinfo ? That way, you could for example use the
same ssd132x_encoder_atomic_enable() and just lookup the deviceinfo's
.init, instead of unconditionally call to the ssd132x_init() helper.

The same comment applies for the rest of your logic, I really think that
you could make extend the other ssd132x helpers to accommodate for the 
SSD1322 differences.

For example, having a device_info .nibble_hi_first, .segment_width and
.needs_write_ram fields for ssd132x_update_rect(), etc.

I believe doing it this way would make the driver more organized. But
maybe I'm missing subtleties, so I want to know your thoughts on this.

> The init sequence follows the Newhaven NHD-3.12-25664UCW2 datasheet,
> including its analog settings for the module (external VSL, precharge,
> VCOMH), with three changes: the display clock and the column offset come
> from the device tree, the contrast is the backlight brightness and is set
> after the sequence, and dual COM line mode, which the datasheet only
> allows up to 64 rows, is turned off for taller panels.
>
> Assisted-by: LLM

Please drop this, is not needed anymore since commit 1e035297ffda ("docs:
remove Assisted-by references").

> Signed-off-by: Fabio Piparo <holofermes@xxxxxxxxx>
> ---
>  drivers/gpu/drm/solomon/ssd130x-i2c.c |   3 +-
>  drivers/gpu/drm/solomon/ssd130x-spi.c |   6 +-
>  drivers/gpu/drm/solomon/ssd130x.c     | 277 +++++++++++++++++++++++++-
>  drivers/gpu/drm/solomon/ssd130x.h     |   4 +-
>  4 files changed, 281 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/solomon/ssd130x-i2c.c b/drivers/gpu/drm/solomon/ssd130x-i2c.c
> index 941a2eb44c5..5b85d000c57 100644
> --- a/drivers/gpu/drm/solomon/ssd130x-i2c.c
> +++ b/drivers/gpu/drm/solomon/ssd130x-i2c.c
> @@ -92,11 +92,12 @@ static const struct of_device_id ssd130x_of_match[] = {
>  		.compatible = "solomon,ssd1309fb-i2c",
>  		.data = &ssd130x_variants[SSD1309_ID],
>  	},
> -	/* ssd132x family */
> +	/* ssd1322 family */
>  	{
>  		.compatible = "solomon,ssd1322",
>  		.data = &ssd130x_variants[SSD1322_ID],
>  	},

This is what I mentioned, a family for a single device doesn't sound
correct to me.

>  
>  #define SSD132X_SEGMENT_WIDTH 2
> +#define SSD1322_SEGMENT_WIDTH 4
>

I think it's OK to keep this and then you could do:

+	[SSD1322_ID] = {
+		.default_width = 480,
+		.default_height = 128,
+		.default_dclk_div = 2,
+		.default_dclk_frq = 9,
+		.family_id = SSD132x_FAMILY,
+		.cmd_params_are_data = true,
+		.segment_width = SSD1322_SEGMENT_WIDTH,
+		.needs_write_ram = true,
+ 		.nibble_hi_first = true,
+		.init = ssd1322_init,
+		.set_contrast = ssd1322_set_contrast,
+
+	},

Please post a preparatory patch that sets the .init and .set_contrast
callbacks for all the other devices. It will be just set to the family
defaults currently used.

>  /* ssd13xx commands */
>  #define SSD13XX_CONTRAST			0x81
> @@ -182,6 +184,38 @@
>  #define SSD135X_SET_REMAP_COM_SPLIT		BIT(5)
>  #define SSD135X_SET_REMAP_65K			BIT(6)
>  
> +/* ssd1322 commands */

Just put it under /* ssd132x commands */, the constant defines
already are named SSD1322_* which implies is only for SSD1322.

[...]

> +static int ssd1322_update_rect(struct ssd130x_device *ssd130x,
> +			       struct drm_rect *rect, u8 *data_array,
> +			       unsigned int pitch)
> +{
> +	unsigned int col = ssd130x->col_offset + rect->x1 / SSD1322_SEGMENT_WIDTH;
> +	unsigned int columns = DIV_ROUND_UP(drm_rect_width(rect), SSD1322_SEGMENT_WIDTH);
> +	unsigned int rows = drm_rect_height(rect);
> +	struct drm_device *drm = &ssd130x->drm;
> +	int ret;
> +
> +	drm_WARN_ONCE(drm, rect->x1 % SSD1322_SEGMENT_WIDTH != 0,
> +		      "x must be aligned to screen segment\n");
> +
> +	ret = ssd132x_set_col_range(ssd130x, col, columns);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ssd132x_set_row_range(ssd130x, rect->y1, rows);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ssd130x_write_cmd(ssd130x, 1, SSD1322_WRITE_RAM);
> +	if (ret < 0)
> +		return ret;
> +

this for example would be something like the following:

          if (ssd130x->device_info->needs_write_ram) {
                  ret = ssd130x_write_cmd(ssd130x, 1, SSD1322_WRITE_RAM);
                  if (ret < 0)
                          return ret;
          }

and you could keep reuse the nibble pack logic from ssd132x_update_rect() here.
Just query deviceinfo->nibble_hi_first (or whatever you want to name it), and
use to decide whether to do (n2 & 0xf0) | (n1 >> 4) or (n1 & 0xf0) | (n2 >> 4).
  
> +static int ssd1322_fb_blit_rect(struct drm_framebuffer *fb,
> +				const struct iosys_map *vmap,
> +				struct drm_rect *rect, u8 *buf,
> +				u8 *data_array,
> +				struct drm_format_conv_state *fmtcnv_state)
> +{
> +	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
> +	unsigned int width, height, pitch;
> +	unsigned int i, j;
> +	u32 array_idx = 0;
> +	struct iosys_map dst;
> +
> +	/* Align x to display segment boundaries */
> +	rect->x1 = round_down(rect->x1, SSD1322_SEGMENT_WIDTH);
> +	rect->x2 = min_t(unsigned int, round_up(rect->x2, SSD1322_SEGMENT_WIDTH),

again deviceinfo->segment_width will be used to make ssd132x_fb_blit_rect()
usable for both ssd132x and ssd1322. 

> +			 ssd130x->width);
> +
> +	width = drm_rect_width(rect);
> +	height = drm_rect_height(rect);
> +	pitch = DIV_ROUND_UP(width, 2);
> +
> +	iosys_map_set_vaddr(&dst, buf);
> +	drm_fb_xrgb8888_to_gray8(&dst, &width, vmap, fb, rect, fmtcnv_state);
> +
> +	for (i = 0; i < height; i++) {
> +		/* Process pair of pixels and combine them into a single byte */
> +		for (j = 0; j < width; j += 2) {
> +			u8 n1 = buf[i * width + j];
> +			u8 n2 = j + 1 < width ? buf[i * width + j + 1] : 0;
> +
> +			data_array[array_idx++] = (n1 & 0xf0) | (n2 >> 4);
> +		}
> +	}
> +

And this nibble pack won't be needed anymore since ssd132x_update_rect()
will be parameterized by the deviceinfo data.

> +static void ssd1322_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 ssd130x_plane_state *ssd130x_plane_state = to_ssd130x_plane_state(plane_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;
> +
> +		ssd1322_fb_blit_rect(fb, &shadow_plane_state->data[0], &dst_clip,
> +				     ssd130x_plane_state->buffer,
> +				     ssd130x_crtc_state->data_array,
> +				     &shadow_plane_state->fmtcnv_state);

Note how the only difference with ssd132x_primary_plane_atomic_update() is
the custom _fb_blit_rect() but as mentioned ssd132x_fb_blit_rect() can be
reused. So you could just drop custom SSD1322 plane helpers.
  
> +static void ssd1322_primary_plane_atomic_disable(struct drm_plane *plane,
> +						 struct drm_atomic_commit *state)
> +{
> +	/*
> +	 * Keep this empty function to avoid calling
> +	 * atomic_update when disabling the plane.
> +	 */
> +}

The ssd132x_primary_plane_atomic_disable() clears the screen. So I'm not
sure why you needed this empty callback. Can you just reuse the ssd132x
one ? Ah, you need to add similar changes to ssd132x_clear_screen().

That is, use deviceinfo->segment_width, check device_info->needs_write_ram
to send the SSD1322_WRITE_RAM, etc.

> +	[SSD1322_FAMILY] = {
> +		DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> +		.atomic_check = ssd132x_primary_plane_atomic_check,
> +		.atomic_update = ssd1322_primary_plane_atomic_update,
> +		.atomic_disable = ssd1322_primary_plane_atomic_disable,
> +	},

And this can all go away since the default SSD132X_FAMILY plane helpers
will be used for SSD1322 as well.

>  };
>  
>  static const struct drm_plane_funcs ssd130x_primary_plane_funcs = {
> @@ -1825,6 +2047,10 @@ static const struct drm_crtc_helper_funcs ssd130x_crtc_helper_funcs[] = {
>  		.mode_valid = ssd130x_crtc_mode_valid,
>  		.atomic_check = ssd133x_crtc_atomic_check,
>  	},
> +	[SSD1322_FAMILY] = {
> +		.mode_valid = ssd130x_crtc_mode_valid,
> +		.atomic_check = ssd132x_crtc_atomic_check,
> +	},

and this can go away since are the same as used by the SSD132X_FAMILY.
  
> +static void ssd1322_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 = ssd1322_init(ssd130x);

only init is different, so the ssd132x + deviceinfo->init will be enough.

> @@ -1967,6 +2218,10 @@ static const struct drm_encoder_helper_funcs ssd130x_encoder_helper_funcs[] = {
>  		.atomic_enable = ssd135x_encoder_atomic_enable,
>  		.atomic_disable = ssd130x_encoder_atomic_disable,
>  	},
> +	[SSD1322_FAMILY] = {
> +		.atomic_enable = ssd1322_encoder_atomic_enable,
> +		.atomic_disable = ssd130x_encoder_atomic_disable,
> +	},

Same, it goes away.
  
> +static int ssd1322_update_bl(struct backlight_device *bdev)
> +{
> +	struct ssd130x_device *ssd130x = bl_get_data(bdev);
> +
> +	ssd130x->contrast = backlight_get_brightness(bdev);
> +
> +	return ssd1322_set_contrast(ssd130x, ssd130x->contrast);

ssd132x_update_bl() + deviceinfo->set_contrast instead.

> +	[SSD1322_FAMILY] = {
> +		.update_status	= ssd1322_update_bl,
> +	},

No need for this as well.

If you do all this rework, feel free to add my:

Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>

and I could just merge it as soon as you re-spin the series.

Thanks again for this work, it's great to know that more devices are
working with this driver!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat