diff mbox

[v3,5/5] ARM: dts: rockchip: set PWM delay backlight settings for Minnie

Message ID 20170717212811.25374-5-enric.balletbo@collabora.com (mailing list archive)
State New, archived
Headers show

Commit Message

Enric Balletbo i Serra July 17, 2017, 9:28 p.m. UTC
The minnie devices comes with an AUO B101EAN01 panel which is different
from default veyron devices, thus the power on/off timing sequence is
slightly different. The datasheet specifies a pwm delay of 200 ms, so
update the PMW delay proprieties accordingly.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Heiko,
 I'm not able to test this patch in a minnie device because I don't have
one, so could you do a quick try, please?

Changes since v2:
 - Use new names for proprieties.
Changes since v1:
 - Add this new patch as minnie has differents timings

 arch/arm/boot/dts/rk3288-veyron-minnie.dts | 2 ++
 1 file changed, 2 insertions(+)

Comments

Pavel Machek July 20, 2017, 8:10 a.m. UTC | #1
On Mon 2017-07-17 23:28:11, Enric Balletbo i Serra wrote:
> The minnie devices comes with an AUO B101EAN01 panel which is different
> from default veyron devices, thus the power on/off timing sequence is
> slightly different. The datasheet specifies a pwm delay of 200 ms, so
> update the PMW delay proprieties accordingly.

Wait a wait a moment!

> --- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts
> +++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
> @@ -123,6 +123,8 @@
>  			240 241 242 243 244 245 246 247
>  			248 249 250 251 252 253 254 255>;
>  	power-supply = <&backlight_regulator>;
> +	post-pwm-on-delay-us = <20000>;

-us = <20 000>;

This is not 200 msec.

Plus, it is quite anti-social to do udelay(200 000).

Plus, it is very anti-socifal to use udelay_range(200msec,
400msec).

Whoever told you udelay_range is good thing to use -- it is not, and
it is certainly not worth making user wait 200msec more!

									Pavel
Enric Balletbo Serra July 20, 2017, 10:03 a.m. UTC | #2
Pavel,

2017-07-20 10:10 GMT+02:00 Pavel Machek <pavel@ucw.cz>:
> On Mon 2017-07-17 23:28:11, Enric Balletbo i Serra wrote:
>> The minnie devices comes with an AUO B101EAN01 panel which is different
>> from default veyron devices, thus the power on/off timing sequence is
>> slightly different. The datasheet specifies a pwm delay of 200 ms, so
>> update the PMW delay proprieties accordingly.
>
> Wait a wait a moment!
>
>> --- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts
>> +++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
>> @@ -123,6 +123,8 @@
>>                       240 241 242 243 244 245 246 247
>>                       248 249 250 251 252 253 254 255>;
>>       power-supply = <&backlight_regulator>;
>> +     post-pwm-on-delay-us = <20000>;
>
> -us = <20 000>;
>
> This is not 200 msec.
>

Oops, good catch.

> Plus, it is quite anti-social to do udelay(200 000).
>
> Plus, it is very anti-socifal to use udelay_range(200msec,
> 400msec).
>
> Whoever told you udelay_range is good thing to use -- it is not, and
> it is certainly not worth making user wait 200msec more!
>

Checked again some datasheets and seems that or not require a delay or
the delays are 10ms+, so according to [1] I'll use msleep instead.

[1] https://www.kernel.org/doc/Documentation/timers/timers-howto.txt

Thanks,
 Enric

>                                                                         Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Pavel Machek July 20, 2017, 10:13 a.m. UTC | #3
Hi!

> 2017-07-20 10:10 GMT+02:00 Pavel Machek <pavel@ucw.cz>:
> > On Mon 2017-07-17 23:28:11, Enric Balletbo i Serra wrote:
> >> The minnie devices comes with an AUO B101EAN01 panel which is different
> >> from default veyron devices, thus the power on/off timing sequence is
> >> slightly different. The datasheet specifies a pwm delay of 200 ms, so
> >> update the PMW delay proprieties accordingly.
> >
> > Wait a wait a moment!
> >
> >> --- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts
> >> +++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
> >> @@ -123,6 +123,8 @@
> >>                       240 241 242 243 244 245 246 247
> >>                       248 249 250 251 252 253 254 255>;
> >>       power-supply = <&backlight_regulator>;
> >> +     post-pwm-on-delay-us = <20000>;
> >
> > -us = <20 000>;
> >
> > This is not 200 msec.
> >
> 
> Oops, good catch.
> 
> > Plus, it is quite anti-social to do udelay(200 000).
> >
> > Plus, it is very anti-socifal to use udelay_range(200msec,
> > 400msec).
> >
> > Whoever told you udelay_range is good thing to use -- it is not, and
> > it is certainly not worth making user wait 200msec more!
> >
> 
> Checked again some datasheets and seems that or not require a delay or
> the delays are 10ms+, so according to [1] I'll use msleep instead.
> 
> [1] https://www.kernel.org/doc/Documentation/timers/timers-howto.txt

Ok, msleep makes sense.

But in such case, you probably want to specify delays in the
miliseconds in the device tree... or at least carefully round the
values up.

Best regards,
									Pavel
diff mbox

Patch

diff --git a/arch/arm/boot/dts/rk3288-veyron-minnie.dts b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
index 544de60..a5b7387 100644
--- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts
+++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts
@@ -123,6 +123,8 @@ 
 			240 241 242 243 244 245 246 247
 			248 249 250 251 252 253 254 255>;
 	power-supply = <&backlight_regulator>;
+	post-pwm-on-delay-us = <20000>;
+	pwm-off-delay-us = <20000>;
 };
 
 &emmc {