| Message ID | 20180107174405.35008-5-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Den 07.01.2018 18.44, skrev Noralf Trønnes: > Split out common poweron-reset functionality. > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- > drivers/gpu/drm/tinydrm/mi0283qt.c | 20 ++---------- > drivers/gpu/drm/tinydrm/mipi-dbi.c | 63 ++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/tinydrm/st7586.c | 9 ++---- > drivers/gpu/drm/tinydrm/st7735r.c | 8 ++--- > include/drm/tinydrm/mipi-dbi.h | 2 ++ > 5 files changed, 73 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c > index c69a4d958f24..2a78bcd35045 100644 > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c > @@ -49,31 +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 */ > - if (mipi_dbi_display_is_on(mipi)) > + if (ret > 0) > 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); > diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c > index ecc5c81a93ac..eea6803ff223 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,68 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi) > } > EXPORT_SYMBOL(mipi_dbi_display_is_on); > > +static int mipi_dbi_por_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; > + } I forgot about the case where we don't have hw reset: /* * 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); Noralf. > + > + 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_por_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_por_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 > @@ -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);
On 01/07/2018 11:44 AM, Noralf Trønnes wrote: > Split out common poweron-reset functionality. > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- > drivers/gpu/drm/tinydrm/mi0283qt.c | 20 ++---------- > drivers/gpu/drm/tinydrm/mipi-dbi.c | 63 ++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/tinydrm/st7586.c | 9 ++---- > drivers/gpu/drm/tinydrm/st7735r.c | 8 ++--- > include/drm/tinydrm/mipi-dbi.h | 2 ++ > 5 files changed, 73 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c > index c69a4d958f24..2a78bcd35045 100644 > --- a/drivers/gpu/drm/tinydrm/mi0283qt.c > +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c > @@ -49,31 +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 */ > - if (mipi_dbi_display_is_on(mipi)) > + if (ret > 0) > return 0; If I am reading the patch right, it looks like there are two if (ret > 0) return 0; in a row with nothing in between when this is applied. > > - 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); > diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c > index ecc5c81a93ac..eea6803ff223 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,68 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi) > } > EXPORT_SYMBOL(mipi_dbi_display_is_on); > > +static int mipi_dbi_por_conditional(struct mipi_dbi *mipi, bool cond) I know it is long, but it would be nice to spell out poweron_reset here instead of "por". > +{ > + 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); It seems unnecessary to do a soft reset after a hard reset, but I suppose some displays don't have a hard reset and the extra soft reset shouldn't hurt anything. > + if (ret) { > + DRM_DEV_ERROR(dev, "Failed to send reset command (%d)\n", ret); > + if (mipi->regulator) > + regulator_disable(mipi->regulator); > + return ret; > + } > + > + 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_por_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_por_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 > @@ -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); >
On 01/08/2018 07:38 PM, David Lechner wrote: > On 01/07/2018 11:44 AM, Noralf Trønnes wrote: >> Split out common poweron-reset functionality. >> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> --- >> drivers/gpu/drm/tinydrm/mi0283qt.c | 20 ++---------- >> drivers/gpu/drm/tinydrm/mipi-dbi.c | 63 ++++++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/tinydrm/st7586.c | 9 ++---- >> drivers/gpu/drm/tinydrm/st7735r.c | 8 ++--- >> include/drm/tinydrm/mipi-dbi.h | 2 ++ >> 5 files changed, 73 insertions(+), 29 deletions(-) >> >> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c >> index c69a4d958f24..2a78bcd35045 100644 >> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c >> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c >> @@ -49,31 +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 */ >> - if (mipi_dbi_display_is_on(mipi)) >> + if (ret > 0) >> return 0; > > If I am reading the patch right, it looks like there are two > > if (ret > 0) > return 0; > > in a row with nothing in between when this is applied. > I see now that I missed < vs. >. Probably better to say (ret == 1) instead of (ret > 0).
Den 09.01.2018 02.38, skrev David Lechner: > On 01/07/2018 11:44 AM, Noralf Trønnes wrote: >> Split out common poweron-reset functionality. >> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> --- >> drivers/gpu/drm/tinydrm/mi0283qt.c | 20 ++---------- >> drivers/gpu/drm/tinydrm/mipi-dbi.c | 63 >> ++++++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/tinydrm/st7586.c | 9 ++---- >> drivers/gpu/drm/tinydrm/st7735r.c | 8 ++--- >> include/drm/tinydrm/mipi-dbi.h | 2 ++ >> 5 files changed, 73 insertions(+), 29 deletions(-) >> >> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c >> b/drivers/gpu/drm/tinydrm/mi0283qt.c >> index c69a4d958f24..2a78bcd35045 100644 >> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c >> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c >> @@ -49,31 +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 */ >> - if (mipi_dbi_display_is_on(mipi)) >> + if (ret > 0) >> return 0; > > If I am reading the patch right, it looks like there are two > > if (ret > 0) > return 0; > > in a row with nothing in between when this is applied. > >> - 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); >> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c >> b/drivers/gpu/drm/tinydrm/mipi-dbi.c >> index ecc5c81a93ac..eea6803ff223 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,68 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi) >> } >> EXPORT_SYMBOL(mipi_dbi_display_is_on); >> +static int mipi_dbi_por_conditional(struct mipi_dbi *mipi, bool cond) > > I know it is long, but it would be nice to spell out poweron_reset here > instead of "por". > >> +{ >> + 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); > > It seems unnecessary to do a soft reset after a hard reset, but I > suppose some displays > don't have a hard reset and the extra soft reset shouldn't hurt anything. > Both ILI9341 and ST7735R (not ST7586S) lists soft reset as part of the Power Flow Chart, but it's not explicit about it being required or not: Power on sequence HW reset SW reset State is now Sleep in I looked in the MIPI DBI spec and there's nothing about requiring both hw _and_ soft reset. But I have seen hard and soft reset in many panel init's, so I think we keep this as the default. It has a 5ms penalty. I could shave that off in mipi_dbi_hw_reset(). It keeps reset low for 20ms, but the spec says it just has to be longer than 9us with noise spikes less than 20ns wide: - msleep(20); + usleep_range(20, 1000); Noralf. >> + if (ret) { >> + DRM_DEV_ERROR(dev, "Failed to send reset command (%d)\n", ret); >> + if (mipi->regulator) >> + regulator_disable(mipi->regulator); >> + return ret; >> + } >> + >> + 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_por_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_por_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 >> @@ -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); >> >
On 01/09/2018 09:01 AM, Noralf Trønnes wrote: > > Den 09.01.2018 02.38, skrev David Lechner: >> On 01/07/2018 11:44 AM, Noralf Trønnes wrote: >>> Split out common poweron-reset functionality. >>> >>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >>> --- >>> drivers/gpu/drm/tinydrm/mi0283qt.c | 20 ++---------- >>> drivers/gpu/drm/tinydrm/mipi-dbi.c | 63 ++++++++++++++++++++++++++++++++++++++ >>> drivers/gpu/drm/tinydrm/st7586.c | 9 ++---- >>> drivers/gpu/drm/tinydrm/st7735r.c | 8 ++--- >>> include/drm/tinydrm/mipi-dbi.h | 2 ++ >>> 5 files changed, 73 insertions(+), 29 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c >>> index c69a4d958f24..2a78bcd35045 100644 >>> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c >>> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c >>> @@ -49,31 +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 */ >>> - if (mipi_dbi_display_is_on(mipi)) >>> + if (ret > 0) >>> return 0; >> >> If I am reading the patch right, it looks like there are two >> >> if (ret > 0) >> return 0; >> >> in a row with nothing in between when this is applied. >> >>> - 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); >>> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c >>> index ecc5c81a93ac..eea6803ff223 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,68 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi) >>> } >>> EXPORT_SYMBOL(mipi_dbi_display_is_on); >>> +static int mipi_dbi_por_conditional(struct mipi_dbi *mipi, bool cond) >> >> I know it is long, but it would be nice to spell out poweron_reset here >> instead of "por". >> >>> +{ >>> + 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); >> >> It seems unnecessary to do a soft reset after a hard reset, but I suppose some displays >> don't have a hard reset and the extra soft reset shouldn't hurt anything. >> > > Both ILI9341 and ST7735R (not ST7586S) lists soft reset as part of the > Power Flow Chart, but it's not explicit about it being required or not: > > Power on sequence > HW reset > SW reset > State is now Sleep in > > I looked in the MIPI DBI spec and there's nothing about requiring both > hw _and_ soft reset. But I have seen hard and soft reset in many panel > init's, so I think we keep this as the default. It has a 5ms penalty. > I could shave that off in mipi_dbi_hw_reset(). It keeps reset low for > 20ms, but the spec says it just has to be longer than 9us with noise > spikes less than 20ns wide: > > - msleep(20); > + usleep_range(20, 1000); > Sounds good to me.
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c index c69a4d958f24..2a78bcd35045 100644 --- a/drivers/gpu/drm/tinydrm/mi0283qt.c +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c @@ -49,31 +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 */ - if (mipi_dbi_display_is_on(mipi)) + if (ret > 0) 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); diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c index ecc5c81a93ac..eea6803ff223 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,68 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *mipi) } EXPORT_SYMBOL(mipi_dbi_display_is_on); +static int mipi_dbi_por_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; + } + + 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_por_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_por_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 @@ -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);
Split out common poweron-reset functionality. Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- drivers/gpu/drm/tinydrm/mi0283qt.c | 20 ++---------- drivers/gpu/drm/tinydrm/mipi-dbi.c | 63 ++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/tinydrm/st7586.c | 9 ++---- drivers/gpu/drm/tinydrm/st7735r.c | 8 ++--- include/drm/tinydrm/mipi-dbi.h | 2 ++ 5 files changed, 73 insertions(+), 29 deletions(-)