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