Amit Barzilai <[email protected]> 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
