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

[Svyatoslav Ryhel]

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

Ok, I will add some comments on the next iteration if such will be needed.

> Regards,
> Simon