[PATCH v1 4/6] video: panel: add Renesas R69328 MIPI DSI panel driver

[Simon Glass]

Hi Svyatoslav,

On Wed, 19 Apr 2023 at 13:17, Svyatoslav Ryhel <clamor95 at gmail.com> wrote:
>
> Driver adds support for panels with Renesas R69328 IC
>
> Currently supported compatible is:
> - jdi,dx12d100vm0eaa
>
> Tested-by: Andreas Westman Dorcsak <hedmoo at yahoo.com> # LG P880 T30
> Tested-by: Svyatoslav Ryhel <clamor95 at gmail.com> # LG P895 T30
> Signed-off-by: Svyatoslav Ryhel <clamor95 at gmail.com>
> ---
>  drivers/video/Kconfig          |   9 ++
>  drivers/video/Makefile         |   1 +
>  drivers/video/renesas-r69328.c | 232 +++++++++++++++++++++++++++++++++
>  3 files changed, 242 insertions(+)
>  create mode 100644 drivers/video/renesas-r69328.c

Reviewed-by: Simon Glass <sjg at chromium.org>

Please see below

[..]

> +static int renesas_r69328_enable_backlight(struct udevice *dev)
> +{
> +       struct renesas_r69328_priv *priv = dev_get_priv(dev);
> +       int ret;
> +
> +       ret = dm_gpio_set_value(&priv->enable_gpio, 1);
> +       if (ret) {
> +               printf("%s: error changing enable-gpios (%d)\n", __func__, ret);

If you use log_err() then the function name is should automatically if
CONFIG_LOGF_FUNC is enabled

> +               return ret;
> +       }
> +       mdelay(5);
> +
> +       ret = dm_gpio_set_value(&priv->reset_gpio, 0);
> +       if (ret) {
> +               printf("%s: error changing reset-gpios (%d)\n", __func__, ret);
> +               return ret;
> +       }
> +       mdelay(5);
> +
> +       ret = dm_gpio_set_value(&priv->reset_gpio, 1);
> +       if (ret) {
> +               printf("%s: error changing reset-gpios (%d)\n", __func__, ret);
> +               return ret;
> +       }
> +
> +       mdelay(5);

Please add a comment about the delays so people know how they were
chosen. E.g. it might be in the datasheet.

> +
> +       return 0;
> +}
> +
> +static int renesas_r69328_set_backlight(struct udevice *dev, int percent)
> +{
> +       struct renesas_r69328_priv *priv = dev_get_priv(dev);
> +       struct mipi_dsi_panel_plat *plat = dev_get_plat(dev);
> +       struct mipi_dsi_device *dsi = plat->device;
> +       int ret;
> +
> +       mipi_dsi_dcs_write_buffer(dsi, address_mode,
> +                                 sizeof(address_mode));
> +
> +       ret = mipi_dsi_dcs_set_pixel_format(dsi, MIPI_DCS_PIXEL_FMT_24BIT << 4);
> +       if (ret < 0) {
> +               printf("%s: failed to set pixel format: %d\n", __func__, ret);
> +               return ret;
> +       }
> +
> +       ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> +       if (ret < 0) {
> +               printf("%s: failed to exit sleep mode: %d\n", __func__, ret);
> +               return ret;
> +       }
> +
> +       mdelay(100);

Wow that is a long time. Please add a comment.

> +
> +       /* MACP Off */
> +       dsi_generic_write_seq(dsi, R69328_MACP, 0x04);
> +
> +       dsi_generic_write_seq(dsi, R69328_POWER_SET, 0x14,
> +                             0x1D, 0x21, 0x67, 0x11, 0x9A);
> +
> +       dsi_generic_write_seq(dsi, R69328_GAMMA_SET_A, 0x00,
> +                             0x1A, 0x20, 0x28, 0x25, 0x24,
> +                             0x26, 0x15, 0x13, 0x11, 0x18,
> +                             0x1E, 0x1C, 0x00, 0x00, 0x1A,
> +                             0x20, 0x28, 0x25, 0x24, 0x26,
> +                             0x15, 0x13, 0x11, 0x18, 0x1E,
> +                             0x1C, 0x00);

lower-case hex

> +       dsi_generic_write_seq(dsi, R69328_GAMMA_SET_B, 0x00,
> +                             0x1A, 0x20, 0x28, 0x25, 0x24,
> +                             0x26, 0x15, 0x13, 0x11, 0x18,
> +                             0x1E, 0x1C, 0x00, 0x00, 0x1A,
> +                             0x20, 0x28, 0x25, 0x24, 0x26,
> +                             0x15, 0x13, 0x11, 0x18, 0x1E,
> +                             0x1C, 0x00);
> +       dsi_generic_write_seq(dsi, R69328_GAMMA_SET_C, 0x00,
> +                             0x1A, 0x20, 0x28, 0x25, 0x24,
> +                             0x26, 0x15, 0x13, 0x11, 0x18,
> +                             0x1E, 0x1C, 0x00, 0x00, 0x1A,
> +                             0x20, 0x28, 0x25, 0x24, 0x26,
> +                             0x15, 0x13, 0x11, 0x18, 0x1E,
> +                             0x1C, 0x00);
> +
> +       /* MACP On */
> +       dsi_generic_write_seq(dsi, R69328_MACP, 0x03);
> +
> +       ret = mipi_dsi_dcs_set_display_on(dsi);
> +       if (ret < 0) {
> +               printf("%s: failed to set display on: %d\n", __func__, ret);
> +               return ret;
> +       }
> +
> +       mdelay(50);
> +
[..]

Regards,
Simon