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

[Noralf Trønnes]

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.

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

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