[PATCH v2 1/4] dt-bindings: pwm-backlight: add pwm-delay-us property

[PATCH v2 1/4] dt-bindings: pwm-backlight: add pwm-delay-us property

[Enric Balletbo i Serra]

From: huang lin <hl@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@rock-chips.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@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.
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 [1]: Documentation/devicetree/bindings/gpio/gpio.txt
@@ -32,4 +37,5 @@ Example:
 
 		power-supply = <&vdd_bl_reg>;
 		enable-gpios = <&gpio 58 0>;
+		pwm-delay-us = <10000 10000>;
 	};
-- 
2.9.3

[PATCH v2 2/4] pwm-backlight: add support for pwm-delay-us property

[Enric Balletbo i Serra]

From: huang lin <hl@rock-chips.com>

Some panels (i.e. N116BGE-L41), in their power sequence specifications,
request a delay between set the PWM signal and enable the backlight and
between clear the PWM signal and disable the backlight. Add support for
the new pwm-delay-us property to meet the timing.

Note that this patch inverts current sequence. Before this patch the
enable signal was set before the PWM signal and vice-versa on power off.

I assumed that this sequence was wrong, at least it is on different panel
datasheets that I checked, so I inverted the sequence to follow:

  On power on, set the PWM signal, wait, and set the LED_EN signal.
  On power off, clear the LED_EN signal, wait, and stop the PWM signal.

Signed-off-by: huang lin <hl@rock-chips.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Changes since v1:
 - As suggested by Daniel Thompson
   - Do not assume power-on delay and power-off delay will be the same
 - Move the check of dt property to the parse dt function.

v1: https://lkml.org/lkml/2017/6/28/219

 drivers/video/backlight/pwm_bl.c | 24 ++++++++++++++++++++----
 include/linux/pwm_backlight.h    |  1 +
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 002f1ce..0f5470e 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -10,6 +10,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio.h>
 #include <linux/module.h>
@@ -35,6 +36,7 @@ struct pwm_bl_data {
 	struct gpio_desc	*enable_gpio;
 	unsigned int		scale;
 	bool			legacy;
+	unsigned int		pwm_delay[2];
 	int			(*notify)(struct device *,
 					  int brightness);
 	void			(*notify_after)(struct device *,
@@ -54,10 +56,14 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
 	if (err < 0)
 		dev_err(pb->dev, "failed to enable power supply\n");
 
+	pwm_enable(pb->pwm);
+
+	if (pb->pwm_delay[0])
+		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] + 2000);
+
 	if (pb->enable_gpio)
 		gpiod_set_value_cansleep(pb->enable_gpio, 1);
 
-	pwm_enable(pb->pwm);
 	pb->enabled = true;
 }
 
@@ -66,12 +72,15 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
 	if (!pb->enabled)
 		return;
 
-	pwm_config(pb->pwm, 0, pb->period);
-	pwm_disable(pb->pwm);
-
 	if (pb->enable_gpio)
 		gpiod_set_value_cansleep(pb->enable_gpio, 0);
 
+	if (pb->pwm_delay[1])
+		usleep_range(pb->pwm_delay[1], pb->pwm_delay[1] + 2000);
+
+	pwm_config(pb->pwm, 0, pb->period);
+	pwm_disable(pb->pwm);
+
 	regulator_disable(pb->power_supply);
 	pb->enabled = false;
 }
@@ -174,6 +183,12 @@ static int pwm_backlight_parse_dt(struct device *dev,
 		data->max_brightness--;
 	}
 
+	/* read pwm to enable pre/post delays from DT property */
+	ret = of_property_read_u32_array(node, "pwm-delay-us", data->pwm_delay,
+					 ARRAY_SIZE(data->pwm_delay));
+	if (ret < 0)
+		return ret;
+
 	data->enable_gpio = -EINVAL;
 	return 0;
 }
@@ -272,6 +287,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	pb->exit = data->exit;
 	pb->dev = &pdev->dev;
 	pb->enabled = false;
+	memcpy(pb->pwm_delay, data->pwm_delay, sizeof(pb->pwm_delay));
 
 	pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
 						  GPIOD_ASIS);
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index efdd922..bf37131 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -13,6 +13,7 @@ struct platform_pwm_backlight_data {
 	unsigned int lth_brightness;
 	unsigned int pwm_period_ns;
 	unsigned int *levels;
+	unsigned int pwm_delay[2];
 	/* TODO remove once all users are switched to gpiod_* API */
 	int enable_gpio;
 	int (*init)(struct device *dev);
-- 
2.9.3

Re: [PATCH v2 2/4] pwm-backlight: add support for pwm-delay-us property

[Thierry Reding]

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

On Fri, Jun 30, 2017 at 01:21:07PM +0200, Enric Balletbo i Serra wrote:
> From: huang lin <hl@rock-chips.com>
> 
> Some panels (i.e. N116BGE-L41), in their power sequence specifications,
> request a delay between set the PWM signal and enable the backlight and
> between clear the PWM signal and disable the backlight. Add support for
> the new pwm-delay-us property to meet the timing.
> 
> Note that this patch inverts current sequence. Before this patch the
> enable signal was set before the PWM signal and vice-versa on power off.
> 
> I assumed that this sequence was wrong, at least it is on different panel
> datasheets that I checked, so I inverted the sequence to follow:
> 
>   On power on, set the PWM signal, wait, and set the LED_EN signal.
>   On power off, clear the LED_EN signal, wait, and stop the PWM signal.

I think this should be two separate patches to make it easier to revert
the inverted sequence should it prove to regress on other panels.

Two more comments below.

> Signed-off-by: huang lin <hl@rock-chips.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> Changes since v1:
>  - As suggested by Daniel Thompson
>    - Do not assume power-on delay and power-off delay will be the same
>  - Move the check of dt property to the parse dt function.
> 
> v1: https://lkml.org/lkml/2017/6/28/219
> 
>  drivers/video/backlight/pwm_bl.c | 24 ++++++++++++++++++++----
>  include/linux/pwm_backlight.h    |  1 +
>  2 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 002f1ce..0f5470e 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -10,6 +10,7 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/gpio.h>
>  #include <linux/module.h>
> @@ -35,6 +36,7 @@ struct pwm_bl_data {
>  	struct gpio_desc	*enable_gpio;
>  	unsigned int		scale;
>  	bool			legacy;
> +	unsigned int		pwm_delay[2];
>  	int			(*notify)(struct device *,
>  					  int brightness);
>  	void			(*notify_after)(struct device *,
> @@ -54,10 +56,14 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
>  	if (err < 0)
>  		dev_err(pb->dev, "failed to enable power supply\n");
>  
> +	pwm_enable(pb->pwm);
> +
> +	if (pb->pwm_delay[0])
> +		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] + 2000);

2000 us is kind of arbitrary. What if pwm_delay[0] is on the order of 20
us? Making the delay 2 ms longer (in the worst case) seems somewhat
excessive. Why not something like:

	usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] * 2);

?

> +
>  	if (pb->enable_gpio)
>  		gpiod_set_value_cansleep(pb->enable_gpio, 1);
>  
> -	pwm_enable(pb->pwm);
>  	pb->enabled = true;
>  }
>  
> @@ -66,12 +72,15 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
>  	if (!pb->enabled)
>  		return;
>  
> -	pwm_config(pb->pwm, 0, pb->period);
> -	pwm_disable(pb->pwm);
> -
>  	if (pb->enable_gpio)
>  		gpiod_set_value_cansleep(pb->enable_gpio, 0);
>  
> +	if (pb->pwm_delay[1])
> +		usleep_range(pb->pwm_delay[1], pb->pwm_delay[1] + 2000);
> +
> +	pwm_config(pb->pwm, 0, pb->period);
> +	pwm_disable(pb->pwm);
> +
>  	regulator_disable(pb->power_supply);
>  	pb->enabled = false;
>  }
> @@ -174,6 +183,12 @@ static int pwm_backlight_parse_dt(struct device *dev,
>  		data->max_brightness--;
>  	}
>  
> +	/* read pwm to enable pre/post delays from DT property */

This comment is confusing. This isn't reading anything from the PWM.

Thierry

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

Re: [PATCH v2 2/4] pwm-backlight: add support for pwm-delay-us property

[Pavel Machek]

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

On Thu 2017-07-06 10:01:32, Thierry Reding wrote:
> On Fri, Jun 30, 2017 at 01:21:07PM +0200, Enric Balletbo i Serra wrote:
> > From: huang lin <hl@rock-chips.com>
> > 
> > Some panels (i.e. N116BGE-L41), in their power sequence specifications,
> > request a delay between set the PWM signal and enable the backlight and
> > between clear the PWM signal and disable the backlight. Add support for
> > the new pwm-delay-us property to meet the timing.
> > 
> > Note that this patch inverts current sequence. Before this patch the
> > enable signal was set before the PWM signal and vice-versa on power off.
> > 
> > I assumed that this sequence was wrong, at least it is on different panel
> > datasheets that I checked, so I inverted the sequence to follow:
> > 
> >   On power on, set the PWM signal, wait, and set the LED_EN signal.
> >   On power off, clear the LED_EN signal, wait, and stop the PWM signal.
> 
> I think this should be two separate patches to make it easier to revert
> the inverted sequence should it prove to regress on other panels.

Don't make this overly complex. This is trivial. No need to split it
into more patches.


> Two more comments below.
> 
> > Signed-off-by: huang lin <hl@rock-chips.com>
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > ---
> > Changes since v1:
> >  - As suggested by Daniel Thompson
> >    - Do not assume power-on delay and power-off delay will be the same
> >  - Move the check of dt property to the parse dt function.
> > 
> > v1: https://lkml.org/lkml/2017/6/28/219
> > 
> >  drivers/video/backlight/pwm_bl.c | 24 ++++++++++++++++++++----
> >  include/linux/pwm_backlight.h    |  1 +
> >  2 files changed, 21 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > index 002f1ce..0f5470e 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -10,6 +10,7 @@
> >   * published by the Free Software Foundation.
> >   */
> >  
> > +#include <linux/delay.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/gpio.h>
> >  #include <linux/module.h>
> > @@ -35,6 +36,7 @@ struct pwm_bl_data {
> >  	struct gpio_desc	*enable_gpio;
> >  	unsigned int		scale;
> >  	bool			legacy;
> > +	unsigned int		pwm_delay[2];
> >  	int			(*notify)(struct device *,
> >  					  int brightness);
> >  	void			(*notify_after)(struct device *,
> > @@ -54,10 +56,14 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
> >  	if (err < 0)
> >  		dev_err(pb->dev, "failed to enable power supply\n");
> >  
> > +	pwm_enable(pb->pwm);
> > +
> > +	if (pb->pwm_delay[0])
> > +		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] + 2000);
> 
> 2000 us is kind of arbitrary. What if pwm_delay[0] is on the order of 20
> us? Making the delay 2 ms longer (in the worst case) seems somewhat
> excessive. Why not something like:
> 
> 	usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] * 2);
> 
> ?
> 
> > +
> >  	if (pb->enable_gpio)
> >  		gpiod_set_value_cansleep(pb->enable_gpio, 1);
> >  
> > -	pwm_enable(pb->pwm);
> >  	pb->enabled = true;
> >  }
> >  
> > @@ -66,12 +72,15 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
> >  	if (!pb->enabled)
> >  		return;
> >  
> > -	pwm_config(pb->pwm, 0, pb->period);
> > -	pwm_disable(pb->pwm);
> > -
> >  	if (pb->enable_gpio)
> >  		gpiod_set_value_cansleep(pb->enable_gpio, 0);
> >  
> > +	if (pb->pwm_delay[1])
> > +		usleep_range(pb->pwm_delay[1], pb->pwm_delay[1] + 2000);
> > +
> > +	pwm_config(pb->pwm, 0, pb->period);
> > +	pwm_disable(pb->pwm);
> > +
> >  	regulator_disable(pb->power_supply);
> >  	pb->enabled = false;
> >  }
> > @@ -174,6 +183,12 @@ static int pwm_backlight_parse_dt(struct device *dev,
> >  		data->max_brightness--;
> >  	}
> >  
> > +	/* read pwm to enable pre/post delays from DT property */
> 
> This comment is confusing. This isn't reading anything from the PWM.
> 
> Thierry



-- 
(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 --]

Re: [PATCH v2 2/4] pwm-backlight: add support for pwm-delay-us property

[Daniel Thompson]

On 06/07/17 10:12, Pavel Machek wrote:
> On Thu 2017-07-06 10:01:32, Thierry Reding wrote:
>> On Fri, Jun 30, 2017 at 01:21:07PM +0200, Enric Balletbo i Serra wrote:
>>> From: huang lin <hl@rock-chips.com>
>>>
>>> Some panels (i.e. N116BGE-L41), in their power sequence specifications,
>>> request a delay between set the PWM signal and enable the backlight and
>>> between clear the PWM signal and disable the backlight. Add support for
>>> the new pwm-delay-us property to meet the timing.
>>>
>>> Note that this patch inverts current sequence. Before this patch the
>>> enable signal was set before the PWM signal and vice-versa on power off.
>>>
>>> I assumed that this sequence was wrong, at least it is on different panel
>>> datasheets that I checked, so I inverted the sequence to follow:
>>>
>>>    On power on, set the PWM signal, wait, and set the LED_EN signal.
>>>    On power off, clear the LED_EN signal, wait, and stop the PWM signal.
>>
>> I think this should be two separate patches to make it easier to revert
>> the inverted sequence should it prove to regress on other panels.
> 
> Don't make this overly complex. This is trivial. No need to split it
> into more patches.

Agree. IMHO getting the code that reads the (optional) new parameter 
correct is the best way to manage risk of regression since in most cases 
the delay will be skipped anyway.


Daniel.


> 
> 
>> Two more comments below.
>>
>>> Signed-off-by: huang lin <hl@rock-chips.com>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>> ---
>>> Changes since v1:
>>>   - As suggested by Daniel Thompson
>>>     - Do not assume power-on delay and power-off delay will be the same
>>>   - Move the check of dt property to the parse dt function.
>>>
>>> v1: https://lkml.org/lkml/2017/6/28/219
>>>
>>>   drivers/video/backlight/pwm_bl.c | 24 ++++++++++++++++++++----
>>>   include/linux/pwm_backlight.h    |  1 +
>>>   2 files changed, 21 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
>>> index 002f1ce..0f5470e 100644
>>> --- a/drivers/video/backlight/pwm_bl.c
>>> +++ b/drivers/video/backlight/pwm_bl.c
>>> @@ -10,6 +10,7 @@
>>>    * published by the Free Software Foundation.
>>>    */
>>>   
>>> +#include <linux/delay.h>
>>>   #include <linux/gpio/consumer.h>
>>>   #include <linux/gpio.h>
>>>   #include <linux/module.h>
>>> @@ -35,6 +36,7 @@ struct pwm_bl_data {
>>>   	struct gpio_desc	*enable_gpio;
>>>   	unsigned int		scale;
>>>   	bool			legacy;
>>> +	unsigned int		pwm_delay[2];
>>>   	int			(*notify)(struct device *,
>>>   					  int brightness);
>>>   	void			(*notify_after)(struct device *,
>>> @@ -54,10 +56,14 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
>>>   	if (err < 0)
>>>   		dev_err(pb->dev, "failed to enable power supply\n");
>>>   
>>> +	pwm_enable(pb->pwm);
>>> +
>>> +	if (pb->pwm_delay[0])
>>> +		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] + 2000);
>>
>> 2000 us is kind of arbitrary. What if pwm_delay[0] is on the order of 20
>> us? Making the delay 2 ms longer (in the worst case) seems somewhat
>> excessive. Why not something like:
>>
>> 	usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] * 2);
>>
>> ?
>>
>>> +
>>>   	if (pb->enable_gpio)
>>>   		gpiod_set_value_cansleep(pb->enable_gpio, 1);
>>>   
>>> -	pwm_enable(pb->pwm);
>>>   	pb->enabled = true;
>>>   }
>>>   
>>> @@ -66,12 +72,15 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
>>>   	if (!pb->enabled)
>>>   		return;
>>>   
>>> -	pwm_config(pb->pwm, 0, pb->period);
>>> -	pwm_disable(pb->pwm);
>>> -
>>>   	if (pb->enable_gpio)
>>>   		gpiod_set_value_cansleep(pb->enable_gpio, 0);
>>>   
>>> +	if (pb->pwm_delay[1])
>>> +		usleep_range(pb->pwm_delay[1], pb->pwm_delay[1] + 2000);
>>> +
>>> +	pwm_config(pb->pwm, 0, pb->period);
>>> +	pwm_disable(pb->pwm);
>>> +
>>>   	regulator_disable(pb->power_supply);
>>>   	pb->enabled = false;
>>>   }
>>> @@ -174,6 +183,12 @@ static int pwm_backlight_parse_dt(struct device *dev,
>>>   		data->max_brightness--;
>>>   	}
>>>   
>>> +	/* read pwm to enable pre/post delays from DT property */
>>
>> This comment is confusing. This isn't reading anything from the PWM.
>>
>> Thierry
> 
> 
> 

Re: [PATCH v2 2/4] pwm-backlight: add support for pwm-delay-us property

[Thierry Reding]

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

On Thu, Jul 06, 2017 at 10:17:18AM +0100, Daniel Thompson wrote:
> On 06/07/17 10:12, Pavel Machek wrote:
> > On Thu 2017-07-06 10:01:32, Thierry Reding wrote:
> > > On Fri, Jun 30, 2017 at 01:21:07PM +0200, Enric Balletbo i Serra wrote:
> > > > From: huang lin <hl@rock-chips.com>
> > > > 
> > > > Some panels (i.e. N116BGE-L41), in their power sequence specifications,
> > > > request a delay between set the PWM signal and enable the backlight and
> > > > between clear the PWM signal and disable the backlight. Add support for
> > > > the new pwm-delay-us property to meet the timing.
> > > > 
> > > > Note that this patch inverts current sequence. Before this patch the
> > > > enable signal was set before the PWM signal and vice-versa on power off.
> > > > 
> > > > I assumed that this sequence was wrong, at least it is on different panel
> > > > datasheets that I checked, so I inverted the sequence to follow:
> > > > 
> > > >    On power on, set the PWM signal, wait, and set the LED_EN signal.
> > > >    On power off, clear the LED_EN signal, wait, and stop the PWM signal.
> > > 
> > > I think this should be two separate patches to make it easier to revert
> > > the inverted sequence should it prove to regress on other panels.
> > 
> > Don't make this overly complex. This is trivial. No need to split it
> > into more patches.
> 
> Agree. IMHO getting the code that reads the (optional) new parameter correct
> is the best way to manage risk of regression since in most cases the delay
> will be skipped anyway.

The potential regression that I'm referring to would be caused by
inversing the sequence (GPIO enable -> PWM enable). That's completely
unrelated to the delays introduced by this patch. Many boards use this
driver and they've been running with the old sequence for many years.
Granted, it's fairly unlikely to regress, but it's still a possibility.

Given that both changes are logically separate, I think separate patches
are totally appropriate. I also don't think that this would overly
complicate things.

Thierry

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

Re: [PATCH v2 2/4] pwm-backlight: add support for pwm-delay-us property

[Pavel Machek]

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

Hi!

On Thu 2017-07-06 11:24:48, Thierry Reding wrote:
> On Thu, Jul 06, 2017 at 10:17:18AM +0100, Daniel Thompson wrote:
> > On 06/07/17 10:12, Pavel Machek wrote:
> > > On Thu 2017-07-06 10:01:32, Thierry Reding wrote:
> > > > On Fri, Jun 30, 2017 at 01:21:07PM +0200, Enric Balletbo i Serra wrote:
> > > > > From: huang lin <hl@rock-chips.com>
> > > > > 
> > > > > Some panels (i.e. N116BGE-L41), in their power sequence specifications,
> > > > > request a delay between set the PWM signal and enable the backlight and
> > > > > between clear the PWM signal and disable the backlight. Add support for
> > > > > the new pwm-delay-us property to meet the timing.
> > > > > 
> > > > > Note that this patch inverts current sequence. Before this patch the
> > > > > enable signal was set before the PWM signal and vice-versa on power off.
> > > > > 
> > > > > I assumed that this sequence was wrong, at least it is on different panel
> > > > > datasheets that I checked, so I inverted the sequence to follow:
> > > > > 
> > > > >    On power on, set the PWM signal, wait, and set the LED_EN signal.
> > > > >    On power off, clear the LED_EN signal, wait, and stop the PWM signal.
> > > > 
> > > > I think this should be two separate patches to make it easier to revert
> > > > the inverted sequence should it prove to regress on other panels.
> > > 
> > > Don't make this overly complex. This is trivial. No need to split it
> > > into more patches.
> > 
> > Agree. IMHO getting the code that reads the (optional) new parameter correct
> > is the best way to manage risk of regression since in most cases the delay
> > will be skipped anyway.
> 
> The potential regression that I'm referring to would be caused by
> inversing the sequence (GPIO enable -> PWM enable). That's completely
> unrelated to the delays introduced by this patch. Many boards use this
> driver and they've been running with the old sequence for many years.
> Granted, it's fairly unlikely to regress, but it's still a
> possibility.
> 
> Given that both changes are logically separate, I think separate patches
> are totally appropriate. I also don't think that this would overly
> complicate things.

Ah, yes, you are right; should be two patches.

Best regards,
									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 --]

Re: [PATCH v2 2/4] pwm-backlight: add support for pwm-delay-us property

[Daniel Thompson]

On 06/07/17 10:24, Thierry Reding wrote:
> On Thu, Jul 06, 2017 at 10:17:18AM +0100, Daniel Thompson wrote:
>> On 06/07/17 10:12, Pavel Machek wrote:
>>> On Thu 2017-07-06 10:01:32, Thierry Reding wrote:
>>>> On Fri, Jun 30, 2017 at 01:21:07PM +0200, Enric Balletbo i Serra wrote:
>>>>> From: huang lin <hl@rock-chips.com>
>>>>>
>>>>> Some panels (i.e. N116BGE-L41), in their power sequence specifications,
>>>>> request a delay between set the PWM signal and enable the backlight and
>>>>> between clear the PWM signal and disable the backlight. Add support for
>>>>> the new pwm-delay-us property to meet the timing.
>>>>>
>>>>> Note that this patch inverts current sequence. Before this patch the
>>>>> enable signal was set before the PWM signal and vice-versa on power off.
>>>>>
>>>>> I assumed that this sequence was wrong, at least it is on different panel
>>>>> datasheets that I checked, so I inverted the sequence to follow:
>>>>>
>>>>>     On power on, set the PWM signal, wait, and set the LED_EN signal.
>>>>>     On power off, clear the LED_EN signal, wait, and stop the PWM signal.
>>>>
>>>> I think this should be two separate patches to make it easier to revert
>>>> the inverted sequence should it prove to regress on other panels.
>>>
>>> Don't make this overly complex. This is trivial. No need to split it
>>> into more patches.
>>
>> Agree. IMHO getting the code that reads the (optional) new parameter correct
>> is the best way to manage risk of regression since in most cases the delay
>> will be skipped anyway.
> 
> The potential regression that I'm referring to would be caused by
> inversing the sequence (GPIO enable -> PWM enable). That's completely
> unrelated to the delays introduced by this patch. Many boards use this
> driver and they've been running with the old sequence for many years.
> Granted, it's fairly unlikely to regress, but it's still a possibility.
> 
> Given that both changes are logically separate, I think separate patches
> are totally appropriate. I also don't think that this would overly
> complicate things.

... and you are right on both counts!

Thanks for the detailed reply.


Daniel.

Re: [PATCH v2 2/4] pwm-backlight: add support for pwm-delay-us property

[Thierry Reding]

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

On Fri, Jun 30, 2017 at 01:21:07PM +0200, Enric Balletbo i Serra wrote:
> From: huang lin <hl@rock-chips.com>
> 
> Some panels (i.e. N116BGE-L41), in their power sequence specifications,
> request a delay between set the PWM signal and enable the backlight and
> between clear the PWM signal and disable the backlight. Add support for
> the new pwm-delay-us property to meet the timing.
> 
> Note that this patch inverts current sequence. Before this patch the
> enable signal was set before the PWM signal and vice-versa on power off.
> 
> I assumed that this sequence was wrong, at least it is on different panel
> datasheets that I checked, so I inverted the sequence to follow:
> 
>   On power on, set the PWM signal, wait, and set the LED_EN signal.
>   On power off, clear the LED_EN signal, wait, and stop the PWM signal.
> 
> Signed-off-by: huang lin <hl@rock-chips.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> Changes since v1:
>  - As suggested by Daniel Thompson
>    - Do not assume power-on delay and power-off delay will be the same
>  - Move the check of dt property to the parse dt function.
> 
> v1: https://lkml.org/lkml/2017/6/28/219
> 
>  drivers/video/backlight/pwm_bl.c | 24 ++++++++++++++++++++----
>  include/linux/pwm_backlight.h    |  1 +
>  2 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 002f1ce..0f5470e 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -10,6 +10,7 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/gpio.h>
>  #include <linux/module.h>
> @@ -35,6 +36,7 @@ struct pwm_bl_data {
>  	struct gpio_desc	*enable_gpio;
>  	unsigned int		scale;
>  	bool			legacy;
> +	unsigned int		pwm_delay[2];
>  	int			(*notify)(struct device *,
>  					  int brightness);
>  	void			(*notify_after)(struct device *,
> @@ -54,10 +56,14 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
>  	if (err < 0)
>  		dev_err(pb->dev, "failed to enable power supply\n");
>  
> +	pwm_enable(pb->pwm);
> +
> +	if (pb->pwm_delay[0])
> +		usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] + 2000);
> +
>  	if (pb->enable_gpio)
>  		gpiod_set_value_cansleep(pb->enable_gpio, 1);
>  
> -	pwm_enable(pb->pwm);
>  	pb->enabled = true;
>  }
>  
> @@ -66,12 +72,15 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
>  	if (!pb->enabled)
>  		return;
>  
> -	pwm_config(pb->pwm, 0, pb->period);
> -	pwm_disable(pb->pwm);
> -
>  	if (pb->enable_gpio)
>  		gpiod_set_value_cansleep(pb->enable_gpio, 0);
>  
> +	if (pb->pwm_delay[1])
> +		usleep_range(pb->pwm_delay[1], pb->pwm_delay[1] + 2000);
> +
> +	pwm_config(pb->pwm, 0, pb->period);
> +	pwm_disable(pb->pwm);
> +
>  	regulator_disable(pb->power_supply);
>  	pb->enabled = false;
>  }
> @@ -174,6 +183,12 @@ static int pwm_backlight_parse_dt(struct device *dev,
>  		data->max_brightness--;
>  	}
>  
> +	/* read pwm to enable pre/post delays from DT property */
> +	ret = of_property_read_u32_array(node, "pwm-delay-us", data->pwm_delay,
> +					 ARRAY_SIZE(data->pwm_delay));
> +	if (ret < 0)
> +		return ret;

Also I think you need to make sure you have a fallback in place in case
that this fails, otherwise the property is no longer optional.

Ignoring -EINVAL should do the trick since data->pwm_delay should be
zeroed out by the memset() earlier in this function.

Thierry

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

Re: [PATCH v2 2/4] pwm-backlight: add support for pwm-delay-us property

[Enric Balletbo Serra]

Hi Thierry,

Many thanks for your comments, I'll send a v3.

2017-07-06 10:13 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> On Fri, Jun 30, 2017 at 01:21:07PM +0200, Enric Balletbo i Serra wrote:
>> From: huang lin <hl@rock-chips.com>
>>
>> Some panels (i.e. N116BGE-L41), in their power sequence specifications,
>> request a delay between set the PWM signal and enable the backlight and
>> between clear the PWM signal and disable the backlight. Add support for
>> the new pwm-delay-us property to meet the timing.
>>
>> Note that this patch inverts current sequence. Before this patch the
>> enable signal was set before the PWM signal and vice-versa on power off.
>>
>> I assumed that this sequence was wrong, at least it is on different panel
>> datasheets that I checked, so I inverted the sequence to follow:
>>
>>   On power on, set the PWM signal, wait, and set the LED_EN signal.
>>   On power off, clear the LED_EN signal, wait, and stop the PWM signal.
>>
>> Signed-off-by: huang lin <hl@rock-chips.com>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>> Changes since v1:
>>  - As suggested by Daniel Thompson
>>    - Do not assume power-on delay and power-off delay will be the same
>>  - Move the check of dt property to the parse dt function.
>>
>> v1: https://lkml.org/lkml/2017/6/28/219
>>
>>  drivers/video/backlight/pwm_bl.c | 24 ++++++++++++++++++++----
>>  include/linux/pwm_backlight.h    |  1 +
>>  2 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
>> index 002f1ce..0f5470e 100644
>> --- a/drivers/video/backlight/pwm_bl.c
>> +++ b/drivers/video/backlight/pwm_bl.c
>> @@ -10,6 +10,7 @@
>>   * published by the Free Software Foundation.
>>   */
>>
>> +#include <linux/delay.h>
>>  #include <linux/gpio/consumer.h>
>>  #include <linux/gpio.h>
>>  #include <linux/module.h>
>> @@ -35,6 +36,7 @@ struct pwm_bl_data {
>>       struct gpio_desc        *enable_gpio;
>>       unsigned int            scale;
>>       bool                    legacy;
>> +     unsigned int            pwm_delay[2];
>>       int                     (*notify)(struct device *,
>>                                         int brightness);
>>       void                    (*notify_after)(struct device *,
>> @@ -54,10 +56,14 @@ static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
>>       if (err < 0)
>>               dev_err(pb->dev, "failed to enable power supply\n");
>>
>> +     pwm_enable(pb->pwm);
>> +
>> +     if (pb->pwm_delay[0])
>> +             usleep_range(pb->pwm_delay[0], pb->pwm_delay[0] + 2000);
>> +
>>       if (pb->enable_gpio)
>>               gpiod_set_value_cansleep(pb->enable_gpio, 1);
>>
>> -     pwm_enable(pb->pwm);
>>       pb->enabled = true;
>>  }
>>
>> @@ -66,12 +72,15 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
>>       if (!pb->enabled)
>>               return;
>>
>> -     pwm_config(pb->pwm, 0, pb->period);
>> -     pwm_disable(pb->pwm);
>> -
>>       if (pb->enable_gpio)
>>               gpiod_set_value_cansleep(pb->enable_gpio, 0);
>>
>> +     if (pb->pwm_delay[1])
>> +             usleep_range(pb->pwm_delay[1], pb->pwm_delay[1] + 2000);
>> +
>> +     pwm_config(pb->pwm, 0, pb->period);
>> +     pwm_disable(pb->pwm);
>> +
>>       regulator_disable(pb->power_supply);
>>       pb->enabled = false;
>>  }
>> @@ -174,6 +183,12 @@ static int pwm_backlight_parse_dt(struct device *dev,
>>               data->max_brightness--;
>>       }
>>
>> +     /* read pwm to enable pre/post delays from DT property */
>> +     ret = of_property_read_u32_array(node, "pwm-delay-us", data->pwm_delay,
>> +                                      ARRAY_SIZE(data->pwm_delay));
>> +     if (ret < 0)
>> +             return ret;
>
> Also I think you need to make sure you have a fallback in place in case
> that this fails, otherwise the property is no longer optional.
>
> Ignoring -EINVAL should do the trick since data->pwm_delay should be
> zeroed out by the memset() earlier in this function.
>

Yep, you have reason. Thanks.


> Thierry

[PATCH v2 3/4] ARM: dts: rockchip: set pre/post pwm-delay-us property.

[Enric Balletbo i Serra]

For veyron the binding should provide both pwm timings, the delay between
you enable the PWM and set the enable signal, and the delay between you
disable the PWM signal and clear the enable signal. Update the binding
accordingly, in this case the panels connected to the veyron boards have
a symetric power sequence, hence the same value is used.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Changes since v1:
 - Add this new patch to fix current binding on veyron.

 arch/arm/boot/dts/rk3288-veyron-chromebook.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/rk3288-veyron-chromebook.dtsi b/arch/arm/boot/dts/rk3288-veyron-chromebook.dtsi
index d752a31..c0e8ce2 100644
--- a/arch/arm/boot/dts/rk3288-veyron-chromebook.dtsi
+++ b/arch/arm/boot/dts/rk3288-veyron-chromebook.dtsi
@@ -96,7 +96,7 @@
 		pinctrl-names = "default";
 		pinctrl-0 = <&bl_en>;
 		pwms = <&pwm0 0 1000000 0>;
-		pwm-delay-us = <10000>;
+		pwm-delay-us = <10000 10000>;
 	};
 
 	gpio-charger {
-- 
2.9.3

[PATCH v2 4/4] ARM: dts: rockchip: add pwm-delay-us backlight setting.

[Enric Balletbo i Serra]

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 pwm-delay-us 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?

Thanks,
 Enric

Changes since v1:
 - Add this new patch as minnie has differents timings.

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

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

Re: [PATCH v2 1/4] dt-bindings: pwm-backlight: add pwm-delay-us property

[Pavel Machek]

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

On Fri 2017-06-30 13:21:06, Enric Balletbo i Serra wrote:
> From: huang lin <hl@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@rock-chips.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@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.

"Hardware needs a delay between setting an initial (non-zero) PWM and
enabling the backlight using GPIO. The 1st cell specifies this delay
in micro seconds. Hardware also needs a delay between disabing the
backlight using GPIO and setting PWM value to 0. The 2nd cell is this
delay in micro seconds."

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 --]

Re: [PATCH v2 1/4] dt-bindings: pwm-backlight: add pwm-delay-us property

[Rob Herring]

On Fri, Jun 30, 2017 at 6:21 AM, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> From: huang lin <hl@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@rock-chips.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@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.

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.

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

Re: [PATCH v2 1/4] dt-bindings: pwm-backlight: add pwm-delay-us property

[Enric Balletbo Serra]

Hi Rob,

2017-07-06 19:07 GMT+02:00 Rob Herring <robh+dt@kernel.org>:
> On Fri, Jun 30, 2017 at 6:21 AM, Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
>> From: huang lin <hl@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@rock-chips.com>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@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

Re: [PATCH v2 1/4] dt-bindings: pwm-backlight: add pwm-delay-us property

[Enric Balletbo Serra]

Rob,

2017-07-06 20:23 GMT+02:00 Enric Balletbo Serra <eballetbo@gmail.com>:
> Hi Rob,
>
> 2017-07-06 19:07 GMT+02:00 Rob Herring <robh+dt@kernel.org>:
>> On Fri, Jun 30, 2017 at 6:21 AM, Enric Balletbo i Serra
>> <enric.balletbo@collabora.com> wrote:
>>> From: huang lin <hl@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@rock-chips.com>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@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.
>>

A second proposal, what do you think?

  - post-pwm-on-delay-us: Delay in us after setting an initial (non-zero) PWM
                          and enabling the backlight using GPIO.
  - pwm-off-delay-us: Delay in us after disabling the backlight using a GPIO
                      and setting PWM value to 0.

Thanks,
Enric

>
> 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

Re: [PATCH v2 1/4] dt-bindings: pwm-backlight: add pwm-delay-us property

[Pavel Machek]

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

On Thu 2017-07-13 09:22:15, Enric Balletbo Serra wrote:
> Rob,
> 
> 2017-07-06 20:23 GMT+02:00 Enric Balletbo Serra <eballetbo@gmail.com>:
> > Hi Rob,
> >
> > 2017-07-06 19:07 GMT+02:00 Rob Herring <robh+dt@kernel.org>:
> >> On Fri, Jun 30, 2017 at 6:21 AM, Enric Balletbo i Serra
> >> <enric.balletbo@collabora.com> wrote:
> >>> From: huang lin <hl@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@rock-chips.com>
> >>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@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.
> >>
> 
> A second proposal, what do you think?
> 
>   - post-pwm-on-delay-us: Delay in us after setting an initial (non-zero) PWM
>                           and enabling the backlight using GPIO.

This says "PWM on", "enable GPIO", "delay". Which is not what you
want.

-- 
(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 --]

Re: [PATCH v2 1/4] dt-bindings: pwm-backlight: add pwm-delay-us property

[Enric Balletbo Serra]

2017-07-13 9:39 GMT+02:00 Pavel Machek <pavel@ucw.cz>:
> On Thu 2017-07-13 09:22:15, Enric Balletbo Serra wrote:
>> Rob,
>>
>> 2017-07-06 20:23 GMT+02:00 Enric Balletbo Serra <eballetbo@gmail.com>:
>> > Hi Rob,
>> >
>> > 2017-07-06 19:07 GMT+02:00 Rob Herring <robh+dt@kernel.org>:
>> >> On Fri, Jun 30, 2017 at 6:21 AM, Enric Balletbo i Serra
>> >> <enric.balletbo@collabora.com> wrote:
>> >>> From: huang lin <hl@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@rock-chips.com>
>> >>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@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.
>> >>
>>
>> A second proposal, what do you think?
>>
>>   - post-pwm-on-delay-us: Delay in us after setting an initial (non-zero) PWM
>>                           and enabling the backlight using GPIO.
>
> This says "PWM on", "enable GPIO", "delay". Which is not what you
> want.
>

Ok, seems I need to improve a bit more my English skills. :)

after -> between ?

Then, if I understand correctly, this (found in another binding that I
used as reference)

- post-power-on-delay-ms : Delay in ms after powering the card and
        de-asserting the reset-gpios (if any)

means,

Power the card, de-asserting reset, delay ?

Regards,
 Enric.

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