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

[David Lechner <david@lechnology.com>]

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