Hi Rob, 2017-07-06 19:07 GMT+02:00 Rob Herring <robh+dt at kernel.org>: > On Fri, Jun 30, 2017 at 6:21 AM, Enric Balletbo i Serra > <enric.balletbo at collabora.com> wrote: >> From: huang lin <hl at rock-chips.com> >> >> Add a pwm-delay-us property to specify the delay between setting an >> initial (non-zero) PWM value and enabling the backlight, and also the >> delay between disabling the backlight and setting PWM value to 0. >> >> Signed-off-by: huang lin <hl at rock-chips.com> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo at collabora.com> >> --- >> Changes since v1: >> - As suggested by Daniel Thompson >> - Do not assume power-on delay and power-off delay will be the same >> >> v1: https://lkml.org/lkml/2017/6/28/219 >> >> Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >> index 764db86..49b037e 100644 >> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt >> @@ -17,6 +17,11 @@ Optional properties: >> "pwms" property (see PWM binding[0]) >> - enable-gpios: contains a single GPIO specifier for the GPIO which enables >> and disables the backlight (see GPIO binding[1]) >> + - pwm-delay-us: delay between setting an initial (non-zero) PWM value and >> + enabling the backlight, and also the delay between disabling >> + the backlight and setting PWM value to 0. >> + The 1st cell is the pre-delay in micro seconds. >> + The 2nd cell is the post-delay in micro seconds. > > pre and post imply a time before and after a certain event, but these > are for 2 different events. These are more like an enable/on delay and > disable/off delay which probably should be separate properties. What > happens when we need the opposite sequence or a different sequence? > Maybe some panel requires the PWM to be 0 until some time after > enabling. > Maybe, Only I can say that the panels I checked always first enable the PWM and then set the ENABLE signal, but of course I didn't check all the panels. Would be more acceptable have enable-delay-us and disable-delay-us proprieties? > I don't understand why you even need a post delay. The PWM can be set > to 0 while enabled, right? So if the PWM is set to 0 while enabled and > then disable the backlight, then there's no delay. Plus this doesn't > make much sense to me electrically either. The PWM duty cycle is going > to be completely async to the enable line change. The PWM state could > go from 1 to 0 at any point in time relative to the enable line > change. > To be honest I'm also not sure why is required the post delay, some panels specify a 0 but others specifies a minimum value between you off the panel and disable the PWM. The only reason I added the post delay is because the different datasheets specifies it, I don't have a use case that the post delay is used to fix something. Thanks, Enric > These issues are the problem with generic bindings. Adding 1 property > is no big deal, but then what happens with the next variation. These > timing constraints need to be able to be implied by the panel's > compatible. > > Rob
