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@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.