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

[Javier Martinez Canillas <javierm@redhat.com>]

Amit Barzilai <amit.barzilai22@gmail.com> writes:

Hello Amit,

Thanks for the patch. Unfortunately I think that there is still some work
to be done for this patch to land. Inline comments below.

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

[...]

>   */
> -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);
>  }

>

This change is correct but need to be split in a separate preparatory
patch. When you do that, feel free to add my Reviewed-by tag.

> -/*
> - * Helper to write command (SSD13XX_COMMAND). The fist variadic argument
> - * is the command to write and the following are the command options.
> - *
> - * 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;
> -}
> -

Please split these ssd130x_write_cmds() and ssd1330x_write_cmd() refactoring
as separate preparatory patches. This patch is changing too many things at
once, it is better to have each logical changes as a separate patch. This
makes reviewer much easier.

>  /*
>   * 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.
> + * The first byte is the command opcode and the following bytes are its
> + * options/parameters.
>   */
>  static int ssd130x_write_cmds(struct ssd130x_device *ssd130x, const u8 *cmd,
>  			      size_t len)
> @@ -294,6 +296,22 @@ static int ssd130x_write_cmds(struct ssd130x_device *ssd130x, const u8 *cmd,
>  	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);
> +	}
> +

I don't like that this logic is in the ssd130x core part of the driver. This
is supposed to be transport agonistic and should not be aware of the D/C#
behaviour that is specific to the SPI transport.

Can we move this to the ssd130x-spi driver? For example, something like the
following might work:

1. Make ssd130x_write_cmds() to just be a static inline wrapper that calls
   to regmap_raw_write(ssd130x->regmap, SSD13XX_COMMAND, cmd, len).

2. Make ssd130x_write_cmd() be a variadic wrapper around ssd130x_write_cmds().

3. Add your logic to ssd130x_spi_write() instead of ssd130x_write_cmds(), that
   way it stays in the correct layer rather than having a leaking abstraction.

Also, instead of checking for info->family_id == SSD135X_FAMILY, we could add
a dc_high_params member (or whatever name is more suitable) to the struct
ssd130x_spi_transport Then other families that might use the same can just
reuse this option instead of checking for specific families.

[...]

>  /*
>   * 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
> @@ -415,6 +454,12 @@ static void ssd130x_reset(struct ssd130x_device *ssd130x)
>  	udelay(4);
>  	gpiod_set_value_cansleep(ssd130x->reset, 0);
>  	udelay(4);
> +	/*
> +	 * No long post-reset delay: the controller datasheets only specify
> +	 * microsecond-scale reset timing. The lengthy delay seen in some other
> +	 * drivers comes from fbtft's generic reset helper and targets slow
> +	 * parallel-bus GPIOs, which do not apply here.
> +	 */
>  }
>

I don't understand the point of this comment. You are not changing the logic
of this function nor I see how this comment is useful.

[...]
  
> +/*
> + * 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;
> +
> +		ret = ssd130x_write_cmd(ssd130x, 1, SSD135X_WRITE_RAM);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return ssd130x_write_data(ssd130x, data_array, count);
> +}
> +
>  static int ssd133x_update_rect(struct ssd130x_device *ssd130x,
>  			       struct drm_rect *rect, u8 *data_array,
>  			       unsigned int pitch)
> @@ -826,7 +927,7 @@ static int ssd133x_update_rect(struct ssd130x_device *ssd130x,
>  		return ret;
>  
>  	/* Write out update in one go since horizontal addressing mode is used */
> -	ret = ssd130x_write_data(ssd130x, data_array, pitch * rows);
> +	ret = ssd133x_write_pixels(ssd130x, data_array, pitch * rows);
>  
>  	return ret;
>  }
> @@ -896,7 +997,7 @@ static void ssd133x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
>  	memset(data_array, 0, pitch * ssd130x->height);
>  
>  	/* Write out update in one go since horizontal addressing mode is used */
> -	ssd130x_write_data(ssd130x, data_array, pitch * ssd130x->height);
> +	ssd133x_write_pixels(ssd130x, data_array, pitch * ssd130x->height);
>  }
>  

Instead of ssd133x_write_pixels() and having per family logic in the primary
place update path, I prefer to have a little bit more of code duplication
and add an .atomic_update, .atomic_disable, etc for the SSD1351.

Yes, I know that your current approach reduces code duplication but also
makes it harder to change the primary plane update path for a family without
affecting another one. This is particularly important in my opinion, given
that contributors usually just test on their display family when posting
the patches. So I prefer to have per family callbacks if possible.

[...]

>  static const struct drm_encoder_funcs ssd130x_encoder_funcs = {
> @@ -1897,15 +2037,25 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> -	bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
> -					    &ssd130xfb_bl_ops, NULL);
> -	if (IS_ERR(bl))
> -		return ERR_PTR(dev_err_probe(dev, PTR_ERR(bl),
> -					     "Unable to register backlight device\n"));
> +	/*
> +	 * The backlight update path drives contrast through the
> +	 * SSD13XX_CONTRAST command (0x81), which the SSD135X family does not
> +	 * implement; the brightness byte would be interpreted as a command
> +	 * opcode instead. Do not register a backlight device for this family
> +	 * until the backlight path is family-aware.
> +	 */
> +	if (ssd130x->device_info->family_id != SSD135X_FAMILY) {
> +		bl = devm_backlight_device_register(dev, dev_name(dev), dev,
> +						    ssd130x, &ssd130xfb_bl_ops,
> +						    NULL);
> +		if (IS_ERR(bl))
> +			return ERR_PTR(dev_err_probe(dev, PTR_ERR(bl),
> +						     "Unable to register backlight device\n"));
>
> -	bl->props.brightness = ssd130x->contrast;
> -	bl->props.max_brightness = MAX_CONTRAST;
> -	ssd130x->bl_dev = bl;
> +		bl->props.brightness = ssd130x->contrast;
> +		bl->props.max_brightness = MAX_CONTRAST;
> +		ssd130x->bl_dev = bl;
> +	}

Again I wonder if is better to have a device_info member to decide whether to
register the backlight, instead of harcoding the check for a specific family.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat