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

[Svyatoslav Ryhel]

чт, 20 квіт. 2023 р. о 01:41 Simon Glass <sjg at chromium.org> пише:
>
> 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.
>

I would love to add reasonable arguments for these delays but unfortunately
I am not LG developer and there are no public datasheets available for either
of the panels. I have used the downstream kernel as a reference and built it
to a more modern look. In case someone can provide datasheets for this panel
or for a similar one with the same controller, this driver may be completed and
further extended for new panels. As for now it was extensively tested on p895
and p880 respectively and did not fail any time.

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