чт, 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.
> > +
> > + 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
