Re: [PATCH v3 4/7] drm/tinydrm/mipi-dbi: Add poweron-reset functions

[Noralf Trønnes <noralf@xxxxxxxxxxx>]

Den 11.01.2018 23.22, skrev David Lechner:
> On 01/10/2018 12:59 PM, Noralf Trønnes wrote:
> > Split out common poweron-reset functionality.
> >
> > Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/tinydrm/mi0283qt.c | 22 ++----------
> >   drivers/gpu/drm/tinydrm/mipi-dbi.c | 73 
> > ++++++++++++++++++++++++++++++++++++++
> >   drivers/gpu/drm/tinydrm/st7586.c   |  9 ++---
> >   drivers/gpu/drm/tinydrm/st7735r.c  |  8 ++---
> >   include/drm/tinydrm/mipi-dbi.h     |  2 ++
> >   5 files changed, 83 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c 
> > b/drivers/gpu/drm/tinydrm/mi0283qt.c
> > index c69a4d958f24..1617405faed4 100644
> > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> > @@ -49,33 +49,17 @@
> >     static int mi0283qt_init(struct mipi_dbi *mipi)
> >   {
> > -    struct tinydrm_device *tdev = &mipi->tinydrm;
> > -    struct device *dev = tdev->drm->dev;
> >       u8 addr_mode;
> >       int ret;
> >         DRM_DEBUG_KMS("\n");
> >   -    ret = regulator_enable(mipi->regulator);
> > -    if (ret) {
> > -        DRM_DEV_ERROR(dev, "Failed to enable regulator %d\n", ret);
> > +    ret = mipi_dbi_poweron_conditional_reset(mipi);
> > +    if (ret < 0)
> >           return ret;
> > -    }
> > -
> > -    /* Avoid flicker by skipping setup if the bootloader has done it */
>
> It could be helpful to keep this comment.

It's not valid anymore since we're not doing this at probe time.

> > -    if (mipi_dbi_display_is_on(mipi))
> > +    if (ret == 1)
> >           return 0;
> >   -    mipi_dbi_hw_reset(mipi);
> > -    ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
> > -    if (ret) {
> > -        DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
> > -        regulator_disable(mipi->regulator);
> > -        return ret;
> > -    }
> > -
> > -    msleep(20);
> > -
> >       mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
> >         mipi_dbi_command(mipi, ILI9341_PWCTRLB, 0x00, 0x83, 0x30);
> > diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c 
> > b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> > index 1c8ef0c4d6d4..3e879d605ed3 100644
> > --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> > +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> > @@ -463,6 +463,7 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi)
> >         val &= ~DCS_POWER_MODE_RESERVED_MASK;
> >   +    /* The poweron/reset value is 08h 
> > DCS_POWER_MODE_DISPLAY_NORMAL_MODE */
> >       if (val != (DCS_POWER_MODE_DISPLAY |
> >           DCS_POWER_MODE_DISPLAY_NORMAL_MODE | 
> > DCS_POWER_MODE_SLEEP_MODE))
> >           return false;
> > @@ -473,6 +474,78 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi)
> >   }
> >   EXPORT_SYMBOL(mipi_dbi_display_is_on);
> >   +static int mipi_dbi_poweron_reset_conditional(struct mipi_dbi 
> > *mipi, bool cond)
> > +{
> > +    struct device *dev = mipi->tinydrm.drm->dev;
> > +    int ret;
> > +
> > +    if (mipi->regulator) {
> > +        ret = regulator_enable(mipi->regulator);
> > +        if (ret) {
> > +            DRM_DEV_ERROR(dev, "Failed to enable regulator (%d)\n", 
> > ret);
> > +            return ret;
> > +        }
> > +    }
> > +
> > +    if (cond && mipi_dbi_display_is_on(mipi))
> > +        return 1;
> > +
> > +    mipi_dbi_hw_reset(mipi);
> > +    ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
> > +    if (ret) {
> > +        DRM_DEV_ERROR(dev, "Failed to send reset command (%d)\n", ret);
> > +        if (mipi->regulator)
> > +            regulator_disable(mipi->regulator);
> > +        return ret;
> > +    }
> > +
> > +    /*
> > +     * If we did a hw reset, we know the controller is in Sleep mode 
> > and
> > +     * per MIPI DSC spec should wait 5ms after soft reset. If we 
> > didn't,
> > +     * we assume worst case and wait 120ms.
> > +     */
> > +    if (mipi->reset)
> > +        usleep_range(5000, 20000);
> > +    else
> > +        msleep(120);
> > +
> > +    return 0;
> > +}
> > +
> > +/**
> > + * mipi_dbi_poweron_reset - MIPI DBI poweron and reset
> > + * @mipi: MIPI DBI structure
> > + *
> > + * This function enables the regulator if used and does a hardware 
> > and software
> > + * reset.
> > + *
> > + * Returns:
> > + * Zero on success, or a negative error code.
> > + */
> > +int mipi_dbi_poweron_reset(struct mipi_dbi *mipi)
> > +{
> > +    return mipi_dbi_poweron_reset_conditional(mipi, false);
> > +}
> > +EXPORT_SYMBOL(mipi_dbi_poweron_reset);
> > +
> > +/**
> > + * mipi_dbi_poweron_conditional_reset - MIPI DBI poweron and 
> > conditional reset
> > + * @mipi: MIPI DBI structure
> > + *
> > + * This function enables the regulator if used and if the display is 
> > off, it
> > + * does a hardware and software reset. If mipi_dbi_display_is_on() 
> > determines
> > + * that the display is on, no reset is performed.
> > + *
> > + * Returns:
> > + * Zero if the controller was reset, 1 if the display was already 
> > on, or a
> > + * negative error code.
> > + */
> > +int mipi_dbi_poweron_conditional_reset(struct mipi_dbi *mipi)
> > +{
> > +    return mipi_dbi_poweron_reset_conditional(mipi, true);
> > +}
> > +EXPORT_SYMBOL(mipi_dbi_poweron_conditional_reset);
> > +
> >   #if IS_ENABLED(CONFIG_SPI)
> >     /**
> > diff --git a/drivers/gpu/drm/tinydrm/st7586.c 
> > b/drivers/gpu/drm/tinydrm/st7586.c
> > index 9fd4423c8e70..a6396ef9cc4a 100644
> > --- a/drivers/gpu/drm/tinydrm/st7586.c
> > +++ b/drivers/gpu/drm/tinydrm/st7586.c
> > @@ -179,19 +179,16 @@ static void st7586_pipe_enable(struct 
> > drm_simple_display_pipe *pipe,
> >   {
> >       struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
> >       struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
> > -    struct device *dev = tdev->drm->dev;
> >       int ret;
> >       u8 addr_mode;
> >         DRM_DEBUG_KMS("\n");
> >   -    mipi_dbi_hw_reset(mipi);
> > -    ret = mipi_dbi_command(mipi, ST7586_AUTO_READ_CTRL, 0x9f);
> > -    if (ret) {
> > -        DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
> > +    ret = mipi_dbi_poweron_reset(mipi);
> > +    if (ret)
> >           return;
> > -    }
> >   +    mipi_dbi_command(mipi, ST7586_AUTO_READ_CTRL, 0x9f);
> >       mipi_dbi_command(mipi, ST7586_OTP_RW_CTRL, 0x00);
> >         msleep(10);
> > diff --git a/drivers/gpu/drm/tinydrm/st7735r.c 
> > b/drivers/gpu/drm/tinydrm/st7735r.c
> > index 1f38e15da676..650257ad0193 100644
> > --- a/drivers/gpu/drm/tinydrm/st7735r.c
> > +++ b/drivers/gpu/drm/tinydrm/st7735r.c
>
> Also need:
>
> @@ -40,7 +40,6 @@ static void jd_t18003_t01_pipe_enable(struct 
> drm_simple_display_pipe *pipe,
>  {
>      struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
>      struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
> -    struct device *dev = tdev->drm->dev;
>      int ret;
>      u8 addr_mode;
>
> to prevent compiler warning.

Oops, hadn't enabled the driver...

> > @@ -46,13 +46,9 @@ static void jd_t18003_t01_pipe_enable(struct 
> > drm_simple_display_pipe *pipe,
> >         DRM_DEBUG_KMS("\n");
> >   -    mipi_dbi_hw_reset(mipi);
> > -
> > -    ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
> > -    if (ret) {
> > -        DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
> > +    ret = mipi_dbi_poweron_reset(mipi);
> > +    if (ret)
> >           return;
> > -    }
> >         msleep(150);
> >   diff --git a/include/drm/tinydrm/mipi-dbi.h 
> > b/include/drm/tinydrm/mipi-dbi.h
> > index 6441d9a9161a..795a4a2205bb 100644
> > --- a/include/drm/tinydrm/mipi-dbi.h
> > +++ b/include/drm/tinydrm/mipi-dbi.h
> > @@ -73,6 +73,8 @@ void mipi_dbi_pipe_enable(struct 
> > drm_simple_display_pipe *pipe,
> >   void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe);
> >   void mipi_dbi_hw_reset(struct mipi_dbi *mipi);
> >   bool mipi_dbi_display_is_on(struct mipi_dbi *mipi);
> > +int mipi_dbi_poweron_reset(struct mipi_dbi *mipi);
> > +int mipi_dbi_poweron_conditional_reset(struct mipi_dbi *mipi);
> >   u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len);
> >     int mipi_dbi_command_read(struct mipi_dbi *mipi, u8 cmd, u8 *val);
>
> With the compiler warning fixed:
>
> Reviewed-by: David Lechner <david@xxxxxxxxxxxxxx>
>
> Also, the whole series:
>
> Tested-by: David Lechner <david@xxxxxxxxxxxxxx>
>
> using st7735r which has a hardware reset and backlight, but no regulator.
> And it is write-only, so cannot detect that the display is already on.

Thanks,

Noralf.

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel