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

[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?

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