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

[Simon Glass]

Hi Svyatoslav,

On Thu, 20 Apr 2023 at 17:53, Svyatoslav Ryhel <clamor95 at gmail.com> wrote:
>
> чт, 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.

A comment like that in the source code would go a long way to help
someone looking at these delays later.

Regards,
Simon