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