[PATCH] drm/panel: simple: ensure Sharp lq123p1jx31 isn't turned off too soon

[PATCH] drm/panel: simple: ensure Sharp lq123p1jx31 isn't turned off too soon

[Douglas Anderson]

The Sharp lq123p1jx31 has a requirement that the VDD is on for at
least 300 ms before being turned off.  At the moment nothing anywhere
in the kernel is ensuring this.

The simplest way to ensure this is to add a "disable" timing of 150
ms.  With this the we know that there will be at least 300 ms between
the regulator/gpio being turned on and being turned off.
Specifically, here's what happens after this change:

* prepare: enable regulator, delay 110 ms (10 for regulator, then 100)
* enable: delay 50 ms
* disable: delay 150 ms, then disable regulator
* unprepare: delay 550 ms (50 for regulator, 500 to off=>on too quick)

As you can see, even giving the regulator 10 ms (the max the panel
spec allows) for ramping up we are sure that the regulator was on for
300 ms now.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/gpu/drm/panel/panel-simple.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 89eb0422821c..dbaadfa6b823 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1622,6 +1622,7 @@ static const struct panel_desc sharp_lq123p1jx31 = {
 	.delay = {
 		.prepare = 110,
 		.enable = 50,
+		.disable = 150,
 		.unprepare = 550,
 	},
 };
-- 
2.11.0.483.g087da7b7c-goog

Re: [PATCH] drm/panel: simple: ensure Sharp lq123p1jx31 isn't turned off too soon

[Doug Anderson]

Hi

On Thu, Feb 2, 2017 at 3:38 PM, Douglas Anderson <dianders@chromium.org> wrote:
> The Sharp lq123p1jx31 has a requirement that the VDD is on for at
> least 300 ms before being turned off.  At the moment nothing anywhere
> in the kernel is ensuring this.
>
> The simplest way to ensure this is to add a "disable" timing of 150
> ms.  With this the we know that there will be at least 300 ms between
> the regulator/gpio being turned on and being turned off.
> Specifically, here's what happens after this change:
>
> * prepare: enable regulator, delay 110 ms (10 for regulator, then 100)
> * enable: delay 50 ms
> * disable: delay 150 ms, then disable regulator
> * unprepare: delay 550 ms (50 for regulator, 500 to off=>on too quick)
>
> As you can see, even giving the regulator 10 ms (the max the panel
> spec allows) for ramping up we are sure that the regulator was on for
> 300 ms now.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 1 +
>  1 file changed, 1 insertion(+)

NAKing my own patch.

While this patch _does_ indeed enforce a requirement from the
datasheet and also empirically fixes the problem I was seeing, I'm
nearly certain that the reason it works has more to do with luck than
anything else.

After a little more digging, I'm becoming more certain that we need to
manage the backlight state for this panel and sequence it properly
with the enable signals.  Assuming this is correct, I will likely post
another patch adding a custom panel driver.

-Doug

Re: [PATCH] drm/panel: simple: ensure Sharp lq123p1jx31 isn't turned off too soon

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On Thu, Feb 02, 2017 at 03:38:53PM -0800, Douglas Anderson wrote:
> The Sharp lq123p1jx31 has a requirement that the VDD is on for at
> least 300 ms before being turned off.  At the moment nothing anywhere
> in the kernel is ensuring this.

Looks to me like .delay.prepare would be a better fit. That enables the
regulator and setting it to 300 ms would make sure that the regulator
will always get enabled for at least 300 ms.

Anyway, your commit message says nothing about why this is important.
What are the side-effects if the above isn't true? Does the display hang
in some way?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm/panel: simple: ensure Sharp lq123p1jx31 isn't turned off too soon

[Doug Anderson]

Hi,

On Mon, Feb 6, 2017 at 4:03 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Thu, Feb 02, 2017 at 03:38:53PM -0800, Douglas Anderson wrote:
>> The Sharp lq123p1jx31 has a requirement that the VDD is on for at
>> least 300 ms before being turned off.  At the moment nothing anywhere
>> in the kernel is ensuring this.
>
> Looks to me like .delay.prepare would be a better fit. That enables the
> regulator and setting it to 300 ms would make sure that the regulator
> will always get enabled for at least 300 ms.

This would also ensure that the 300 ms wasn't violated, right.


> Anyway, your commit message says nothing about why this is important.
> What are the side-effects if the above isn't true? Does the display hang
> in some way?

See my own response where I NAKed my own patch because I realized that
it wasn't fixing the root cause of the problem I was seeing (it just
happened to push timing around).  Basically the problem was that the
backlight was getting turned on while the panel was off and the panel
didn't like that.  This appears to already be addressed with upstream
patches, so picking those into my tree fixed the problem.

It might be nice long term to address some of these "the panel needs
to be on for at least 300 ms" or "the panel needs to be off for at
least 500 ms" in a way that doesn't require a hardcoded delay.

I suppose it could be done today with a custom panel driver, so I
guess it depends on how common those types of requirements are in the
spec and how important they are to actually follow.

-Doug