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

[David Lechner <david@lechnology.com>]

On 01/10/2018 12:59 PM, 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 | 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.

> -	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.

> @@ -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@lechnology.com>

Also, the whole series:

Tested-by: David Lechner <david@lechnology.com>

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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel