diff mbox

[v4,2/5] dt-bindings: pwm-backlight: add PWM delay proprieties.

Message ID 20170721104813.5389-2-enric.balletbo@collabora.com
State Superseded
Headers show

Commit Message

Enric Balletbo i Serra July 21, 2017, 10:48 a.m. UTC
Hardware needs a delay between setting an initial (non-zero) PWM and
enabling the backlight using GPIO. The post-pwm-on-delay-ms specifies
this delay in milli seconds. Hardware also needs a delay between disabing
the backlight using GPIO and setting PWM value to 0. The pwm-off-delay-ms
is this delay in milli seconds.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
Based on the original Huang Lin <hl@rock-chips.com> work.

Changes since v3:
 - Replace us for ms.
 - Add Acked-by: Pavel Machek <pavel@ucw.cz>
Changes since v2:
 - Use separate properties (Rob Herring)
Changes since v1:
 - As suggested by Daniel Thompson
   - Do not assume power-on delay and power-off delay will be the same

 Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Daniel Thompson July 24, 2017, 3:21 p.m. UTC | #1
On 21/07/17 11:48, Enric Balletbo i Serra wrote:
> Hardware needs a delay between setting an initial (non-zero) PWM and
> enabling the backlight using GPIO. The post-pwm-on-delay-ms specifies
> this delay in milli seconds. Hardware also needs a delay between disabing
> the backlight using GPIO and setting PWM value to 0. The pwm-off-delay-ms
> is this delay in milli seconds.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> ---
> Based on the original Huang Lin <hl@rock-chips.com> work.
> 
> Changes since v3:
>   - Replace us for ms.
>   - Add Acked-by: Pavel Machek <pavel@ucw.cz>
> Changes since v2:
>   - Use separate properties (Rob Herring)
> Changes since v1:
>   - As suggested by Daniel Thompson
>     - Do not assume power-on delay and power-off delay will be the same
> 
>   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..3108109 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> @@ -17,6 +17,10 @@ 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])
> +  - post-pwm-on-delay-ms: Delay in ms between setting an initial (non-zero) PWM
> +                          and enabling the backlight using GPIO.
> +  - pwm-off-delay-ms: Delay in ms between disabling the backlight using GPIO
> +                      and setting PWM value to 0.

Whilst it is strictly true that the delay you added to the driver is 
currently between disabling the backlight and setting PWM value to 0 I 
don't think the action should be described at this level of detail in 
the DT bindings.

The semantic action that is being performed is "stopping the PWM". This 
is currently implemented by setting the duty cycle to 0 and then calling 
disable but that could change (especially so since the current behavior 
looks asymmetric versus the enable sequence).


Daniel.


>   
>   [0]: Documentation/devicetree/bindings/pwm/pwm.txt
>   [1]: Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -32,4 +36,6 @@ Example:
>   
>   		power-supply = <&vdd_bl_reg>;
>   		enable-gpios = <&gpio 58 0>;
> +		post-pwm-on-delay-ms = <10>;
> +		pwm-off-delay-ms = <10>;
>   	};
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Machek July 24, 2017, 7:19 p.m. UTC | #2
On Mon 2017-07-24 16:21:44, Daniel Thompson wrote:
> On 21/07/17 11:48, Enric Balletbo i Serra wrote:
> >Hardware needs a delay between setting an initial (non-zero) PWM and
> >enabling the backlight using GPIO. The post-pwm-on-delay-ms specifies
> >this delay in milli seconds. Hardware also needs a delay between disabing
> >the backlight using GPIO and setting PWM value to 0. The pwm-off-delay-ms
> >is this delay in milli seconds.
> >
> >Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >Acked-by: Pavel Machek <pavel@ucw.cz>
> >---
> >Based on the original Huang Lin <hl@rock-chips.com> work.
> >
> >Changes since v3:
> >  - Replace us for ms.
> >  - Add Acked-by: Pavel Machek <pavel@ucw.cz>
> >Changes since v2:
> >  - Use separate properties (Rob Herring)
> >Changes since v1:
> >  - As suggested by Daniel Thompson
> >    - Do not assume power-on delay and power-off delay will be the same
> >
> >  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..3108109 100644
> >--- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> >+++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> >@@ -17,6 +17,10 @@ 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])
> >+  - post-pwm-on-delay-ms: Delay in ms between setting an initial (non-zero) PWM
> >+                          and enabling the backlight using GPIO.
> >+  - pwm-off-delay-ms: Delay in ms between disabling the backlight using GPIO
> >+                      and setting PWM value to 0.
> 
> Whilst it is strictly true that the delay you added to the driver is
> currently between disabling the backlight and setting PWM value to 0 I don't
> think the action should be described at this level of detail in the DT
> bindings.
> 
> The semantic action that is being performed is "stopping the PWM". This is
> currently implemented by setting the duty cycle to 0 and then calling
> disable but that could change (especially so since the current behavior
> looks asymmetric versus the enable sequence).

Well, datasheet say the delay is required between given actions, so
yes, I'd say that belongs in the device tree at this level of detail.

									Pavel
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
index 764db86..3108109 100644
--- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
@@ -17,6 +17,10 @@  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])
+  - post-pwm-on-delay-ms: Delay in ms between setting an initial (non-zero) PWM
+                          and enabling the backlight using GPIO.
+  - pwm-off-delay-ms: Delay in ms between disabling the backlight using GPIO
+                      and setting PWM value to 0.
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 [1]: Documentation/devicetree/bindings/gpio/gpio.txt
@@ -32,4 +36,6 @@  Example:
 
 		power-supply = <&vdd_bl_reg>;
 		enable-gpios = <&gpio 58 0>;
+		post-pwm-on-delay-ms = <10>;
+		pwm-off-delay-ms = <10>;
 	};