linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Revert "ARM: dts: rockchip: set PWM delay backlight settings for Minnie"
@ 2019-06-14 22:45 Matthias Kaehlcke
  2019-06-16 15:41 ` Pavel Machek
  2019-06-18  8:21 ` Enric Balletbo i Serra
  0 siblings, 2 replies; 8+ messages in thread
From: Matthias Kaehlcke @ 2019-06-14 22:45 UTC (permalink / raw)
  To: Heiko Stuebner, Rob Herring, Mark Rutland
  Cc: linux-arm-kernel, linux-rockchip, devicetree, linux-kernel,
	Enric Balletbo i Serra, Pavel Machek, Douglas Anderson,
	Matthias Kaehlcke

This reverts commit 288ceb85b505c19abe1895df068dda5ed20cf482.

According to the commit message the AUO B101EAN01 panel on minnie
requires a PWM delay of 200 ms, however this is not what the
datasheet says. The datasheet mentions a *max* delay of 200 ms
for T2 ("delay from LCDVDD to black video generation") and T3
("delay from LCDVDD to HPD high"), which aren't related to the
PWM. The backlight power sequence does not specify min/max
constraints for T15 (time from PWM on to BL enable) or T16
(time from BL disable to PWM off).

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Enric, if you think I misinterpreted the datasheet please holler!
---
 arch/arm/boot/dts/rk3288-veyron-minnie.dts | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/boot/dts/rk3288-veyron-minnie.dts b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
index 468a1818545d..28cbe07f96ec 100644
--- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts
+++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
@@ -86,8 +86,6 @@
 			240 241 242 243 244 245 246 247
 			248 249 250 251 252 253 254 255>;
 	power-supply = <&backlight_regulator>;
-	post-pwm-on-delay-ms = <200>;
-	pwm-off-delay-ms = <200>;
 };
 
 &emmc {
-- 
2.22.0.410.gd8fdbe21b5-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] Revert "ARM: dts: rockchip: set PWM delay backlight settings for Minnie"
  2019-06-14 22:45 [PATCH] Revert "ARM: dts: rockchip: set PWM delay backlight settings for Minnie" Matthias Kaehlcke
@ 2019-06-16 15:41 ` Pavel Machek
  2019-06-17 10:08   ` Enric Balletbo i Serra
  2019-06-17 16:16   ` Matthias Kaehlcke
  2019-06-18  8:21 ` Enric Balletbo i Serra
  1 sibling, 2 replies; 8+ messages in thread
From: Pavel Machek @ 2019-06-16 15:41 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Heiko Stuebner, Rob Herring, Mark Rutland, linux-arm-kernel,
	linux-rockchip, devicetree, linux-kernel, Enric Balletbo i Serra,
	Douglas Anderson

Hi!

> This reverts commit 288ceb85b505c19abe1895df068dda5ed20cf482.
> 
> According to the commit message the AUO B101EAN01 panel on minnie
> requires a PWM delay of 200 ms, however this is not what the
> datasheet says. The datasheet mentions a *max* delay of 200 ms
> for T2 ("delay from LCDVDD to black video generation") and T3
> ("delay from LCDVDD to HPD high"), which aren't related to the
> PWM. The backlight power sequence does not specify min/max
> constraints for T15 (time from PWM on to BL enable) or T16
> (time from BL disable to PWM off).
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Enric, if you think I misinterpreted the datasheet please holler!

Was this tested? Was previous patch tested?

Does patch being reverted actually break anything? If so, cc stable?

								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Revert "ARM: dts: rockchip: set PWM delay backlight settings for Minnie"
  2019-06-16 15:41 ` Pavel Machek
@ 2019-06-17 10:08   ` Enric Balletbo i Serra
  2019-06-17 16:30     ` Matthias Kaehlcke
  2019-06-17 16:16   ` Matthias Kaehlcke
  1 sibling, 1 reply; 8+ messages in thread
From: Enric Balletbo i Serra @ 2019-06-17 10:08 UTC (permalink / raw)
  To: Pavel Machek, Matthias Kaehlcke
  Cc: Heiko Stuebner, Rob Herring, Mark Rutland, linux-arm-kernel,
	linux-rockchip, devicetree, linux-kernel, Douglas Anderson

Hi,

On 16/6/19 17:41, Pavel Machek wrote:
> Hi!
> 
>> This reverts commit 288ceb85b505c19abe1895df068dda5ed20cf482.
>>
>> According to the commit message the AUO B101EAN01 panel on minnie
>> requires a PWM delay of 200 ms, however this is not what the
>> datasheet says. The datasheet mentions a *max* delay of 200 ms
>> for T2 ("delay from LCDVDD to black video generation") and T3
>> ("delay from LCDVDD to HPD high"), which aren't related to the
>> PWM. The backlight power sequence does not specify min/max
>> constraints for T15 (time from PWM on to BL enable) or T16
>> (time from BL disable to PWM off).
>>

Hmm, clearly we are not looking at the same datasheet, because in the one I have
I don't see any reference to T15/T16 or LCDVDD. And, I assume I am probably
wrong because you might have better access to the specific panel specs for minnie.

I looked at my archive and the datasheet I have is similar to this [1]. In page
21, Section 6.5 Power ON/OFF Sequence, there are two delays T3 and T4, it is
*min* time between the pwm signal and the bl_en and it is 200 ms. That's the
delay the patch was adding.

[1] http://www.yslcd.com.tw/docs/product/B101EAN01.1.pdf

>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>> Enric, if you think I misinterpreted the datasheet please holler!
> 
> Was this tested? Was previous patch tested?
> 

IIRC, It was tested measuring the backlight power on timing (although I am not
sure if I tested this on minnie or another board with better access to the pins)

> Does patch being reverted actually break anything? If so, cc stable?
> 
> 								Pavel
> 								
> 

Probably will not break anything, I don't remember the reverted patch as a fix
of any specific issue. IIRC it was more a fear to be out of specs but I'll not
be surprised if the datasheet lies and this delay is not needed at all.

Matthias, are you reverting this to solve any problem? Could you share your
datasheet?

Thanks,
~Enric

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Revert "ARM: dts: rockchip: set PWM delay backlight settings for Minnie"
  2019-06-16 15:41 ` Pavel Machek
  2019-06-17 10:08   ` Enric Balletbo i Serra
@ 2019-06-17 16:16   ` Matthias Kaehlcke
  2019-06-18 12:02     ` Pavel Machek
  1 sibling, 1 reply; 8+ messages in thread
From: Matthias Kaehlcke @ 2019-06-17 16:16 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Heiko Stuebner, Rob Herring, Mark Rutland, linux-arm-kernel,
	linux-rockchip, devicetree, linux-kernel, Enric Balletbo i Serra,
	Douglas Anderson

Hi Pavel,

On Sun, Jun 16, 2019 at 05:41:43PM +0200, Pavel Machek wrote:
> Hi!
> 
> > This reverts commit 288ceb85b505c19abe1895df068dda5ed20cf482.
> > 
> > According to the commit message the AUO B101EAN01 panel on minnie
> > requires a PWM delay of 200 ms, however this is not what the
> > datasheet says. The datasheet mentions a *max* delay of 200 ms
> > for T2 ("delay from LCDVDD to black video generation") and T3
> > ("delay from LCDVDD to HPD high"), which aren't related to the
> > PWM. The backlight power sequence does not specify min/max
> > constraints for T15 (time from PWM on to BL enable) or T16
> > (time from BL disable to PWM off).
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > Enric, if you think I misinterpreted the datasheet please holler!
> 
> Was this tested?

I performed limited manually testing.

minnie ships with the Chrome OS 3.14 downstream, which doesn't include
this delay, to my knowledge there are no open display related bugs for
minnie. One could argue that a the configuration without the delay was
widely field tested

> Does patch being reverted actually break anything?

To my knowledge it doesn't really break anything, however there is a
short user perceptible delay between switching on the LCD and
switching on the backlight. It's not the end of the world, but if it's
not actually needed better avoid it.

> If so, cc stable?

I guess this is an edge case, were you could go either way. I'm fine
with respinning and cc-ing stable.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Revert "ARM: dts: rockchip: set PWM delay backlight settings for Minnie"
  2019-06-17 10:08   ` Enric Balletbo i Serra
@ 2019-06-17 16:30     ` Matthias Kaehlcke
  0 siblings, 0 replies; 8+ messages in thread
From: Matthias Kaehlcke @ 2019-06-17 16:30 UTC (permalink / raw)
  To: Enric Balletbo i Serra
  Cc: Pavel Machek, Heiko Stuebner, Rob Herring, Mark Rutland,
	linux-arm-kernel, linux-rockchip, devicetree, linux-kernel,
	Douglas Anderson

Hi Enric,

On Mon, Jun 17, 2019 at 12:08:25PM +0200, Enric Balletbo i Serra wrote:
> Hi,
> 
> On 16/6/19 17:41, Pavel Machek wrote:
> > Hi!
> > 
> >> This reverts commit 288ceb85b505c19abe1895df068dda5ed20cf482.
> >>
> >> According to the commit message the AUO B101EAN01 panel on minnie
> >> requires a PWM delay of 200 ms, however this is not what the
> >> datasheet says. The datasheet mentions a *max* delay of 200 ms
> >> for T2 ("delay from LCDVDD to black video generation") and T3
> >> ("delay from LCDVDD to HPD high"), which aren't related to the
> >> PWM. The backlight power sequence does not specify min/max
> >> constraints for T15 (time from PWM on to BL enable) or T16
> >> (time from BL disable to PWM off).
> >>
> 
> Hmm, clearly we are not looking at the same datasheet, because in the one I have
> I don't see any reference to T15/T16 or LCDVDD. And, I assume I am probably
> wrong because you might have better access to the specific panel specs for minnie.
> 
> I looked at my archive and the datasheet I have is similar to this [1]. In page
> 21, Section 6.5 Power ON/OFF Sequence, there are two delays T3 and T4, it is
> *min* time between the pwm signal and the bl_en and it is 200 ms. That's the
> delay the patch was adding.
> 
> [1] http://www.yslcd.com.tw/docs/product/B101EAN01.1.pdf
> 
> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >> ---
> >> Enric, if you think I misinterpreted the datasheet please holler!
> > 
> > Was this tested? Was previous patch tested?
> > 
> 
> IIRC, It was tested measuring the backlight power on timing (although I am not
> sure if I tested this on minnie or another board with better access to the pins)
> 
> > Does patch being reverted actually break anything? If so, cc stable?
> > 
> > 								Pavel
> > 								
> > 
> 
> Probably will not break anything, I don't remember the reverted patch as a fix
> of any specific issue. IIRC it was more a fear to be out of specs but I'll not
> be surprised if the datasheet lies and this delay is not needed at all.

Indeed, we are looking at different datasheets. It turns out that
'B101EAN01' is an underspecification, minnie uses the 'B101EAN01.8'
(eDP interface), your datasheet describes the 'B101EAN01.1' (LVDS
interface).

> Matthias, are you reverting this to solve any problem?

With the patch there is a user perceptible delay between switching on
the LCD and the backlight. Not necessarily a big problem, but better
avoid it if the delay is not needed.

> Could you share your datasheet?

http://www.yslcd.com.tw/docs/product/B101EAN01.8.pdf (the server seems
to be down currently).

Thanks

Matthias

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Revert "ARM: dts: rockchip: set PWM delay backlight settings for Minnie"
  2019-06-14 22:45 [PATCH] Revert "ARM: dts: rockchip: set PWM delay backlight settings for Minnie" Matthias Kaehlcke
  2019-06-16 15:41 ` Pavel Machek
@ 2019-06-18  8:21 ` Enric Balletbo i Serra
  2019-06-18 18:34   ` Matthias Kaehlcke
  1 sibling, 1 reply; 8+ messages in thread
From: Enric Balletbo i Serra @ 2019-06-18  8:21 UTC (permalink / raw)
  To: Matthias Kaehlcke, Heiko Stuebner, Rob Herring, Mark Rutland
  Cc: linux-arm-kernel, linux-rockchip, devicetree, linux-kernel,
	Pavel Machek, Douglas Anderson

Hi Matthias,

On 15/6/19 0:45, Matthias Kaehlcke wrote:
> This reverts commit 288ceb85b505c19abe1895df068dda5ed20cf482.
> 
> According to the commit message the AUO B101EAN01 panel on minnie
> requires a PWM delay of 200 ms, however this is not what the
> datasheet says. The datasheet mentions a *max* delay of 200 ms
> for T2 ("delay from LCDVDD to black video generation") and T3
> ("delay from LCDVDD to HPD high"), which aren't related to the
> PWM. The backlight power sequence does not specify min/max
> constraints for T15 (time from PWM on to BL enable) or T16
> (time from BL disable to PWM off).
> 

Could you point from where the confusion comes from? I think will be helpful for
the record. B101EAN01.8 vs B101EAN01.1

> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

With the above added:

Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Thanks,
~ Enric

> ---
> Enric, if you think I misinterpreted the datasheet please holler!
> ---
>  arch/arm/boot/dts/rk3288-veyron-minnie.dts | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/rk3288-veyron-minnie.dts b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
> index 468a1818545d..28cbe07f96ec 100644
> --- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts
> +++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
> @@ -86,8 +86,6 @@
>  			240 241 242 243 244 245 246 247
>  			248 249 250 251 252 253 254 255>;
>  	power-supply = <&backlight_regulator>;
> -	post-pwm-on-delay-ms = <200>;
> -	pwm-off-delay-ms = <200>;
>  };
>  
>  &emmc {
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Revert "ARM: dts: rockchip: set PWM delay backlight settings for Minnie"
  2019-06-17 16:16   ` Matthias Kaehlcke
@ 2019-06-18 12:02     ` Pavel Machek
  0 siblings, 0 replies; 8+ messages in thread
From: Pavel Machek @ 2019-06-18 12:02 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Heiko Stuebner, Rob Herring, Mark Rutland, linux-arm-kernel,
	linux-rockchip, devicetree, linux-kernel, Enric Balletbo i Serra,
	Douglas Anderson

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

On Mon 2019-06-17 09:16:25, Matthias Kaehlcke wrote:
> Hi Pavel,
> 
> On Sun, Jun 16, 2019 at 05:41:43PM +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > This reverts commit 288ceb85b505c19abe1895df068dda5ed20cf482.
> > > 
> > > According to the commit message the AUO B101EAN01 panel on minnie
> > > requires a PWM delay of 200 ms, however this is not what the
> > > datasheet says. The datasheet mentions a *max* delay of 200 ms
> > > for T2 ("delay from LCDVDD to black video generation") and T3
> > > ("delay from LCDVDD to HPD high"), which aren't related to the
> > > PWM. The backlight power sequence does not specify min/max
> > > constraints for T15 (time from PWM on to BL enable) or T16
> > > (time from BL disable to PWM off).
> > > 
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > > Enric, if you think I misinterpreted the datasheet please holler!
> > 
> > Was this tested?
> 
> I performed limited manually testing.
> 
> minnie ships with the Chrome OS 3.14 downstream, which doesn't include
> this delay, to my knowledge there are no open display related bugs for
> minnie. One could argue that a the configuration without the delay was
> widely field tested
> 
> > Does patch being reverted actually break anything?
> 
> To my knowledge it doesn't really break anything, however there is a
> short user perceptible delay between switching on the LCD and
> switching on the backlight. It's not the end of the world, but if it's
> not actually needed better avoid it.
> 
> > If so, cc stable?
> 
> I guess this is an edge case, were you could go either way. I'm fine
> with respinning and cc-ing stable.

Ok, if it is just a small delay, stable probably does not need to be
involved.

Thanks,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Revert "ARM: dts: rockchip: set PWM delay backlight settings for Minnie"
  2019-06-18  8:21 ` Enric Balletbo i Serra
@ 2019-06-18 18:34   ` Matthias Kaehlcke
  0 siblings, 0 replies; 8+ messages in thread
From: Matthias Kaehlcke @ 2019-06-18 18:34 UTC (permalink / raw)
  To: Enric Balletbo i Serra
  Cc: Heiko Stuebner, Rob Herring, Mark Rutland, linux-arm-kernel,
	linux-rockchip, devicetree, linux-kernel, Pavel Machek,
	Douglas Anderson

Hi Enric,

On Tue, Jun 18, 2019 at 10:21:52AM +0200, Enric Balletbo i Serra wrote:
> Hi Matthias,
> 
> On 15/6/19 0:45, Matthias Kaehlcke wrote:
> > This reverts commit 288ceb85b505c19abe1895df068dda5ed20cf482.
> > 
> > According to the commit message the AUO B101EAN01 panel on minnie
> > requires a PWM delay of 200 ms, however this is not what the
> > datasheet says. The datasheet mentions a *max* delay of 200 ms
> > for T2 ("delay from LCDVDD to black video generation") and T3
> > ("delay from LCDVDD to HPD high"), which aren't related to the
> > PWM. The backlight power sequence does not specify min/max
> > constraints for T15 (time from PWM on to BL enable) or T16
> > (time from BL disable to PWM off).
> > 
> 
> Could you point from where the confusion comes from? I think will be helpful for
> the record. B101EAN01.8 vs B101EAN01.1

sounds good

> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> 
> With the above added:
> 
> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Thanks!

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-06-18 18:35 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-14 22:45 [PATCH] Revert "ARM: dts: rockchip: set PWM delay backlight settings for Minnie" Matthias Kaehlcke
2019-06-16 15:41 ` Pavel Machek
2019-06-17 10:08   ` Enric Balletbo i Serra
2019-06-17 16:30     ` Matthias Kaehlcke
2019-06-17 16:16   ` Matthias Kaehlcke
2019-06-18 12:02     ` Pavel Machek
2019-06-18  8:21 ` Enric Balletbo i Serra
2019-06-18 18:34   ` Matthias Kaehlcke

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).