[PATCH 4/5] drm/tinydrm/mi0283qt: Let the display pipe handle power

[Noralf Trønnes]

Den 06.01.2018 19.10, skrev David Lechner:
> On 01/06/2018 06:45 AM, Noralf Trønnes wrote:
>>
>> Den 05.01.2018 19.59, skrev David Lechner:
>>> On 01/05/2018 10:55 AM, Noralf Trønnes wrote:
>>>> It's better to leave power handling and controller init to the
>>>> modesetting machinery using the simple pipe .enable and .disable
>>>> callbacks.
>>>>
>>>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>>>> ---
>>>>   drivers/gpu/drm/tinydrm/mi0283qt.c | 51 
>>>> ++++++++------------------------------
>>>>   drivers/gpu/drm/tinydrm/mipi-dbi.c | 14 ++++++-----
>>>>   2 files changed, 19 insertions(+), 46 deletions(-)
>>>
>>> <snip>
>>>
>>>> @@ -316,8 +315,8 @@ static void mipi_dbi_blank(struct mipi_dbi *mipi)
>>>>    * mipi_dbi_pipe_disable - MIPI DBI pipe disable helper
>>>>    * @pipe: Display pipe
>>>>    *
>>>> - * This function disables backlight if present or if not the
>>>> - * display memory is blanked. Drivers can use this as their
>>>> + * This function disables backlight if present, if not the display 
>>>> memory is
>>>> + * blanked. The regulator is disabled if in use. Drivers can use 
>>>> this as their
>>>>    * &drm_simple_display_pipe_funcs->disable callback.
>>>>    */
>>>>   void mipi_dbi_pipe_disable(struct drm_simple_display_pipe *pipe)
>>>> @@ -333,6 +332,9 @@ void mipi_dbi_pipe_disable(struct 
>>>> drm_simple_display_pipe *pipe)
>>>>           tinydrm_disable_backlight(mipi->backlight);
>>>>       else
>>>>           mipi_dbi_blank(mipi);
>>>> +
>>>> +    if (mipi->regulator)
>>>> +        regulator_disable(mipi->regulator);
>>>>   }
>>>>   EXPORT_SYMBOL(mipi_dbi_pipe_disable);
>>>
>>> If a display physically has a backlight, but it is not controllable 
>>> (i.e.
>>> mipi->backlight == NULL) and you disable the regulator, would that 
>>> not cause
>>> the display to be all white instead of blanked?
>>>
>>
>> Yes, if the regulator doesn't control the backlight. But on these simple
>> displays I assume that a regulator would control the whole display
>> including the backlight. If we get a display with a separate backlight
>> regulator, I think it's better to treat that as the exception rather 
>> than
>> the other way around.
>
> Makes sense. All of the displays I have that have a controllable 
> backlight don't
> have a regulator for the display itself, so they won't be affected 
> anyway.
>
>>
>>> Also, even if this is OK, it seems like you should call 
>>> regulator_enable()
>>> in mipi_dbi_pipe_enable() to keep things balanced in the helper 
>>> functions.
>>>
>>
>> Yes, I wasn't entirely happy with this. The regulator has to be enabled
>> before the controller is initialized, so it can't happen in this 
>> _enable helper.
>> I thought about having mipi_dbi_pipe_enable_begin/end functions, but I'm
>> not sure I like that either. I prefer something more descriptive.
>>
>> Maybe we should drop the pipe enable helper, since it's not very likely
>> that anyone can use it directly as an .enable function.
>>
>> What do you think about this approach:
>
> It looks OK to me. Although I am wondering about the conditional 
> reset. If the
> display is already on, how do we know it is configured correctly?
>

The MIPI DCS spec says that the reset value of get_power_mode 0Ah is 08h.
That's 0b00001000 which differs from what we call ON: 0bX00111XX.

If mipi->read_commands isn't NULL and the MISO pin isn't connected we will
read all 1's or all 0's (unless it's floating), which isn't a problem
since it will return false in that case.

The only way to get ON if the driver didn't do it, is if the bootloader
did it. In that case we trust the bootloader.

This speeds up subsequent .enable calls, since we can avoid the msleeps.


     MIPI_DCS_GET_POWER_MODE        = 0x0A,

#define DCS_POWER_MODE_DISPLAY            BIT(2)
#define DCS_POWER_MODE_DISPLAY_NORMAL_MODE    BIT(3)
#define DCS_POWER_MODE_SLEEP_MODE        BIT(4)
#define DCS_POWER_MODE_PARTIAL_MODE        BIT(5)
#define DCS_POWER_MODE_IDLE_MODE        BIT(6)
#define DCS_POWER_MODE_RESERVED_MASK        (BIT(0) | BIT(1) | BIT(7))

bool mipi_dbi_display_is_on(struct mipi_dbi *mipi)
{
     u8 val;

     if (mipi_dbi_command_read(mipi, MIPI_DCS_GET_POWER_MODE, &val))
         return false;

     val &= ~DCS_POWER_MODE_RESERVED_MASK;

     if (val != (DCS_POWER_MODE_DISPLAY |
         DCS_POWER_MODE_DISPLAY_NORMAL_MODE | DCS_POWER_MODE_SLEEP_MODE))
         return false;

     DRM_DEBUG_DRIVER("Display is ON\n");

     return true;
}

Noralf.

>>
>> /**
>>   * 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)
>> {
>>      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 (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);
>>          regulator_disable(mipi->regulator);
>>          return ret;
>>      }
>>
>>      return 0;
>> }
>> EXPORT_SYMBOL(mipi_dbi_poweron_conditional_reset);
>>
>> /**
>>   * mipi_dbi_enable_flush - MIPI DBI enable helper
>>   * @mipi: MIPI DBI structure
>>   *
>>   * This function sets &mipi_dbi->enabled, flushes the whole 
>> framebuffer and
>>   * enables the backlight. Drivers can use this in their
>>   * &drm_simple_display_pipe_funcs->enable callback.
>>   */
>> void mipi_dbi_enable_flush(struct mipi_dbi *mipi)
>> {
>>      struct drm_framebuffer *fb = mipi->tdev.pipe->plane.fb;
>>
>>      mipi->enabled = true;
>>      if (fb)
>>          fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
>>
>>      tinydrm_enable_backlight(mipi->backlight);
>> }
>> EXPORT_SYMBOL(mipi_dbi_enable_flush);
>>
>>
>> static void mi0283qt_enable(struct drm_simple_display_pipe *pipe,
>>                  struct drm_crtc_state *crtc_state)
>> {
>>      struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
>>      struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
>>      struct device *dev = tdev->drm->dev;
>>
>>      ret = mipi_dbi_poweron_conditional_reset(mipi);
>>      if (ret < 0)
>>          return;
>>      if (ret > 0)
>>          goto out_enable;
>>
>>      /* initialize controller */
>>
>> out_enable:
>>      mipi_dbi_enable_flush(mipi);
>> }
>>
>> Noralf.
>>
>