Hi Svyatoslav, On Thu, 20 Apr 2023 at 17:53, Svyatoslav Ryhel <clamo...@gmail.com> wrote: > > чт, 20 квіт. 2023 р. о 01:41 Simon Glass <s...@chromium.org> пише: > > > > Hi Svyatoslav, > > > > On Wed, 19 Apr 2023 at 13:17, Svyatoslav Ryhel <clamo...@gmail.com> wrote: > > > > > > Driver adds support for panels with Renesas R69328 IC > > > > > > Currently supported compatible is: > > > - jdi,dx12d100vm0eaa > > > > > > Tested-by: Andreas Westman Dorcsak <hed...@yahoo.com> # LG P880 T30 > > > Tested-by: Svyatoslav Ryhel <clamo...@gmail.com> # LG P895 T30 > > > Signed-off-by: Svyatoslav Ryhel <clamo...@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 <s...@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