[PATCH 1/2] dt-bindings: pwm-backlight: Add pwm-delay-us property

[PATCH 1/2] 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>
---
 Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
index 764db86..f75b08f 100644
--- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
@@ -17,6 +17,9 @@ 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.
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 [1]: Documentation/devicetree/bindings/gpio/gpio.txt
@@ -32,4 +35,5 @@ Example:
 
 		power-supply = <&vdd_bl_reg>;
 		enable-gpios = <&gpio 58 0>;
+		pwm-delay-us = <10000>;
 	};
-- 
2.9.3

[PATCH 1/2] 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>
---
 Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
index 764db86..f75b08f 100644
--- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
@@ -17,6 +17,9 @@ 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.
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 [1]: Documentation/devicetree/bindings/gpio/gpio.txt
@@ -32,4 +35,5 @@ Example:
 
 		power-supply = <&vdd_bl_reg>;
 		enable-gpios = <&gpio 58 0>;
+		pwm-delay-us = <10000>;
 	};
-- 
2.9.3

[PATCH 2/2] pwm-backlight: Add support for pwm-delay-us property

[Enric Balletbo i Serra]

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

Some panel backlight (i.e. N116BGE-L41), in their power sequence
specifications, request a delay between PWM signal and the backlight
enable signal, so use the pwm-delay-us property to meet the timing.

Signed-off-by: huang lin <hl@rock-chips.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
 drivers/video/backlight/pwm_bl.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 002f1ce..c5c4ef0 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>
@@ -30,6 +31,7 @@ struct pwm_bl_data {
 	unsigned int		period;
 	unsigned int		lth_brightness;
 	unsigned int		*levels;
+	unsigned int		pwm_delay;
 	bool			enabled;
 	struct regulator	*power_supply;
 	struct gpio_desc	*enable_gpio;
@@ -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)
+		usleep_range(pb->pwm_delay, pb->pwm_delay + 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)
+		usleep_range(pb->pwm_delay, pb->pwm_delay + 2000);
+
+	pwm_config(pb->pwm, 0, pb->period);
+	pwm_disable(pb->pwm);
+
 	regulator_disable(pb->power_supply);
 	pb->enabled = false;
 }
@@ -273,6 +282,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	pb->dev = &pdev->dev;
 	pb->enabled = false;
 
+	of_property_read_u32(pdev->dev.of_node, "pwm-delay-us", &pb->pwm_delay);
 	pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
 						  GPIOD_ASIS);
 	if (IS_ERR(pb->enable_gpio)) {
-- 
2.9.3

[PATCH 2/2] pwm-backlight: Add support for pwm-delay-us property

[Enric Balletbo i Serra]

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

Some panel backlight (i.e. N116BGE-L41), in their power sequence
specifications, request a delay between PWM signal and the backlight
enable signal, so use the pwm-delay-us property to meet the timing.

Signed-off-by: huang lin <hl@rock-chips.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
 drivers/video/backlight/pwm_bl.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 002f1ce..c5c4ef0 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>
@@ -30,6 +31,7 @@ struct pwm_bl_data {
 	unsigned int		period;
 	unsigned int		lth_brightness;
 	unsigned int		*levels;
+	unsigned int		pwm_delay;
 	bool			enabled;
 	struct regulator	*power_supply;
 	struct gpio_desc	*enable_gpio;
@@ -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)
+		usleep_range(pb->pwm_delay, pb->pwm_delay + 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)
+		usleep_range(pb->pwm_delay, pb->pwm_delay + 2000);
+
+	pwm_config(pb->pwm, 0, pb->period);
+	pwm_disable(pb->pwm);
+
 	regulator_disable(pb->power_supply);
 	pb->enabled = false;
 }
@@ -273,6 +282,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	pb->dev = &pdev->dev;
 	pb->enabled = false;
 
+	of_property_read_u32(pdev->dev.of_node, "pwm-delay-us", &pb->pwm_delay);
 	pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
 						  GPIOD_ASIS);
 	if (IS_ERR(pb->enable_gpio)) {
-- 
2.9.3

Re: [PATCH 1/2] dt-bindings: pwm-backlight: Add pwm-delay-us property

[Daniel Thompson]

On 28/06/17 12:16, 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>
> ---
>   Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> index 764db86..f75b08f 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> @@ -17,6 +17,9 @@ 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.

If is safe to assume power-on delay and power-off delay will be the same?

I've only took a quick look but several backlight controllers support 
asymetric power-on/off sequences...


Daniel.


>   
>   [0]: Documentation/devicetree/bindings/pwm/pwm.txt
>   [1]: Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -32,4 +35,5 @@ Example:
>   
>   		power-supply = <&vdd_bl_reg>;
>   		enable-gpios = <&gpio 58 0>;
> +		pwm-delay-us = <10000>;
>   	};
> 

Re: [PATCH 1/2] dt-bindings: pwm-backlight: Add pwm-delay-us property

[Daniel Thompson]

On 28/06/17 12:16, 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>
> ---
>   Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> index 764db86..f75b08f 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> @@ -17,6 +17,9 @@ 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.

If is safe to assume power-on delay and power-off delay will be the same?

I've only took a quick look but several backlight controllers support 
asymetric power-on/off sequences...


Daniel.


>   
>   [0]: Documentation/devicetree/bindings/pwm/pwm.txt
>   [1]: Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -32,4 +35,5 @@ Example:
>   
>   		power-supply = <&vdd_bl_reg>;
>   		enable-gpios = <&gpio 58 0>;
> +		pwm-delay-us = <10000>;
>   	};
> 

Re: [PATCH 1/2] dt-bindings: pwm-backlight: Add pwm-delay-us property

[Pavel Machek]

On Wed 2017-06-28 13:16:31, 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>
> ---
>  Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> index 764db86..f75b08f 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> @@ -17,6 +17,9 @@ 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.
>

I understood it as "you set PWM and it takes a while for display to
light up"
but that's not correct. Changelog from second patch makes it
clear. Please
clarify it here, too.

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

Re: [PATCH 1/2] dt-bindings: pwm-backlight: Add pwm-delay-us property

[Pavel Machek]

On Wed 2017-06-28 13:16:31, 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>
> ---
>  Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> index 764db86..f75b08f 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> @@ -17,6 +17,9 @@ 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.
>

I understood it as "you set PWM and it takes a while for display to
light up"
but that's not correct. Changelog from second patch makes it
clear. Please
clarify it here, too.

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

Re: [PATCH 1/2] dt-bindings: pwm-backlight: Add pwm-delay-us property

[Enric Balletbo Serra]

Hi Daniel, Pavel,

2017-06-28 15:30 GMT+02:00 Pavel Machek <pavel@ucw.cz>:
> On Wed 2017-06-28 13:16:31, 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>
>> ---
>>  Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>> index 764db86..f75b08f 100644
>> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>> @@ -17,6 +17,9 @@ 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.
>>
>
>> If is safe to assume power-on delay and power-off delay will be the same?
>>
>> I've only took a quick look but several backlight controllers support asymetric power-on/off sequences..

Daniel, right the ones I checked are symmetric but asymmetric timings
are possible I guess, so I think now that specify the two delays is
more accurate, maybe the binding should be something like this?

  pwm-delay-us = <delay_before_on  delay_after_off>;

> I understood it as "you set PWM and it takes a while for display to
> light up"
> but that's not correct. Changelog from second patch makes it
> clear. Please
> clarify it here, too.

Pavel, oh, your dude is interesting ...

That's the idea, the sequence is:
  Power on, you set the PWM signal, wait a bit and set the LED_EN signal.
  Power off, you clear the LED_EN signal, wait a bit and stop the PWM signal.

Note that the patch inverts the sequence, before this patch first you
set LED_EN signal and then the PWM signal

I assumed that the sequence was wrong but maybe I'm mistaken and there
are some panels that follow the original sequence. On the few panels I
checked the power on/off sequence is how I described above, i.e. see
[1] p. 17, the sequence is first PWM and then LED_EN. I'll take a look
at other panel datasheets, or if you know one, could you provide me
the datasheet?

Thanks,

[1] http://www.jxlcd.com/Upload/PicFiles/N116BGE-L41.pdf

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

Re: [PATCH 1/2] dt-bindings: pwm-backlight: Add pwm-delay-us property

[Enric Balletbo Serra]

Hi Daniel, Pavel,

2017-06-28 15:30 GMT+02:00 Pavel Machek <pavel@ucw.cz>:
> On Wed 2017-06-28 13:16:31, 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>
>> ---
>>  Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>> index 764db86..f75b08f 100644
>> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>> @@ -17,6 +17,9 @@ 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.
>>
>
>> If is safe to assume power-on delay and power-off delay will be the same?
>>
>> I've only took a quick look but several backlight controllers support asymetric power-on/off sequences..

Daniel, right the ones I checked are symmetric but asymmetric timings
are possible I guess, so I think now that specify the two delays is
more accurate, maybe the binding should be something like this?

  pwm-delay-us = <delay_before_on  delay_after_off>;

> I understood it as "you set PWM and it takes a while for display to
> light up"
> but that's not correct. Changelog from second patch makes it
> clear. Please
> clarify it here, too.

Pavel, oh, your dude is interesting ...

That's the idea, the sequence is:
  Power on, you set the PWM signal, wait a bit and set the LED_EN signal.
  Power off, you clear the LED_EN signal, wait a bit and stop the PWM signal.

Note that the patch inverts the sequence, before this patch first you
set LED_EN signal and then the PWM signal

I assumed that the sequence was wrong but maybe I'm mistaken and there
are some panels that follow the original sequence. On the few panels I
checked the power on/off sequence is how I described above, i.e. see
[1] p. 17, the sequence is first PWM and then LED_EN. I'll take a look
at other panel datasheets, or if you know one, could you provide me
the datasheet?

Thanks,

[1] http://www.jxlcd.com/Upload/PicFiles/N116BGE-L41.pdf

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

Re: [PATCH 1/2] dt-bindings: pwm-backlight: Add pwm-delay-us property

[Daniel Thompson]

On 28/06/17 15:15, Enric Balletbo Serra wrote:
> Hi Daniel, Pavel,
> 
> 2017-06-28 15:30 GMT+02:00 Pavel Machek <pavel@ucw.cz>:
>> On Wed 2017-06-28 13:16:31, 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>
>>> ---
>>>   Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>>> index 764db86..f75b08f 100644
>>> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>>> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>>> @@ -17,6 +17,9 @@ 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.
>>>
>>
>>> If is safe to assume power-on delay and power-off delay will be the same?
>>>
>>> I've only took a quick look but several backlight controllers support asymetric power-on/off sequences..
> 
> Daniel, right the ones I checked are symmetric but asymmetric timings
> are possible I guess, so I think now that specify the two delays is
> more accurate, maybe the binding should be something like this?
> 
>    pwm-delay-us = <delay_before_on  delay_after_off>;

I think so. Like you I can't actually point at any asymmetric power 
sequence diagrams but there are a controller devices with power 
sequencing registers that support asymmetry. I'm inclined to take that 
as a hint...


>> I understood it as "you set PWM and it takes a while for display to
>> light up"
>> but that's not correct. Changelog from second patch makes it
>> clear. Please
>> clarify it here, too.
> 
> Pavel, oh, your dude is interesting ...
> 
> That's the idea, the sequence is:
>    Power on, you set the PWM signal, wait a bit and set the LED_EN signal.
>    Power off, you clear the LED_EN signal, wait a bit and stop the PWM signal.
> 
> Note that the patch inverts the sequence, before this patch first you
> set LED_EN signal and then the PWM signal
> 
> I assumed that the sequence was wrong but maybe I'm mistaken and there
> are some panels that follow the original sequence. On the few panels I
> checked the power on/off sequence is how I described above, i.e. see
> [1] p. 17, the sequence is first PWM and then LED_EN. I'll take a look
> at other panel datasheets, or if you know one, could you provide me
> the datasheet?
> 
> Thanks,
> 
> [1] http://www.jxlcd.com/Upload/PicFiles/N116BGE-L41.pdf
> 
>>
>>                                                                  Pavel
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/2] dt-bindings: pwm-backlight: Add pwm-delay-us property

[Daniel Thompson]

On 28/06/17 15:15, Enric Balletbo Serra wrote:
> Hi Daniel, Pavel,
> 
> 2017-06-28 15:30 GMT+02:00 Pavel Machek <pavel@ucw.cz>:
>> On Wed 2017-06-28 13:16:31, 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>
>>> ---
>>>   Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>>> index 764db86..f75b08f 100644
>>> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>>> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>>> @@ -17,6 +17,9 @@ 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.
>>>
>>
>>> If is safe to assume power-on delay and power-off delay will be the same?
>>>
>>> I've only took a quick look but several backlight controllers support asymetric power-on/off sequences..
> 
> Daniel, right the ones I checked are symmetric but asymmetric timings
> are possible I guess, so I think now that specify the two delays is
> more accurate, maybe the binding should be something like this?
> 
>    pwm-delay-us = <delay_before_on  delay_after_off>;

I think so. Like you I can't actually point at any asymmetric power 
sequence diagrams but there are a controller devices with power 
sequencing registers that support asymmetry. I'm inclined to take that 
as a hint...


>> I understood it as "you set PWM and it takes a while for display to
>> light up"
>> but that's not correct. Changelog from second patch makes it
>> clear. Please
>> clarify it here, too.
> 
> Pavel, oh, your dude is interesting ...
> 
> That's the idea, the sequence is:
>    Power on, you set the PWM signal, wait a bit and set the LED_EN signal.
>    Power off, you clear the LED_EN signal, wait a bit and stop the PWM signal.
> 
> Note that the patch inverts the sequence, before this patch first you
> set LED_EN signal and then the PWM signal
> 
> I assumed that the sequence was wrong but maybe I'm mistaken and there
> are some panels that follow the original sequence. On the few panels I
> checked the power on/off sequence is how I described above, i.e. see
> [1] p. 17, the sequence is first PWM and then LED_EN. I'll take a look
> at other panel datasheets, or if you know one, could you provide me
> the datasheet?
> 
> Thanks,
> 
> [1] http://www.jxlcd.com/Upload/PicFiles/N116BGE-L41.pdf
> 
>>
>>                                                                  Pavel
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH 1/2] dt-bindings: pwm-backlight: Add 'max-brightness' property

[Matthias Kaehlcke]

Add an optional 'max-brightness' property, which is used to specify
the number of brightness levels (max-brightness + 1) when the node
has no 'brightness-levels' table.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 .../devicetree/bindings/leds/backlight/pwm-backlight.txt       | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
index 64fa2fbd98c9..98f4ba626054 100644
--- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
@@ -27,6 +27,9 @@ Optional properties:
                             resolution pwm duty cycle can be used without
                             having to list out every possible value in the
                             brightness-level array.
+  - max-brightness: Maximum brightness value. Used to specify the number of
+                    brightness levels (max-brightness + 1) when the node
+                    has no 'brightness-levels' table.
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 [1]: Documentation/devicetree/bindings/gpio/gpio.txt
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

[PATCH 1/2] dt-bindings: pwm-backlight: Add 'max-brightness' property

[Matthias Kaehlcke]

Add an optional 'max-brightness' property, which is used to specify
the number of brightness levels (max-brightness + 1) when the node
has no 'brightness-levels' table.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 .../devicetree/bindings/leds/backlight/pwm-backlight.txt       | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
index 64fa2fbd98c9..98f4ba626054 100644
--- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
@@ -27,6 +27,9 @@ Optional properties:
                             resolution pwm duty cycle can be used without
                             having to list out every possible value in the
                             brightness-level array.
+  - max-brightness: Maximum brightness value. Used to specify the number of
+                    brightness levels (max-brightness + 1) when the node
+                    has no 'brightness-levels' table.
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 [1]: Documentation/devicetree/bindings/gpio/gpio.txt
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

[PATCH 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree

[Matthias Kaehlcke]

Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED
linearly to human eye") uses pwm_period / hweight32(pwm_period) as
as heuristic to determine the number of brightness levels when the DT
doesn't provide a brightness level table. This heuristic is broken
and can result in excessively large brightness tables.

Instead of using the heuristic try to retrieve the number of
brightness levels from the device tree (property 'max-brightness'
+ 1). If the value is not specified use a default of 256 levels.

Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye")
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/video/backlight/pwm_bl.c | 59 ++++++++++++--------------------
 1 file changed, 21 insertions(+), 38 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index fb45f866b923..2913cbe9cfcb 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -194,38 +194,19 @@ int pwm_backlight_brightness_default(struct device *dev,
 				     struct platform_pwm_backlight_data *data,
 				     unsigned int period)
 {
-	unsigned int counter = 0;
-	unsigned int i, n;
+	unsigned int i;
+	unsigned int nlevels = data->max_brightness + 1;
 	u64 retval;
 
-	/*
-	 * Count the number of bits needed to represent the period number. The
-	 * number of bits is used to calculate the number of levels used for the
-	 * brightness-levels table, the purpose of this calculation is have a
-	 * pre-computed table with enough levels to get linear brightness
-	 * perception. The period is divided by the number of bits so for a
-	 * 8-bit PWM we have 255 / 8 = 32 brightness levels or for a 16-bit PWM
-	 * we have 65535 / 16 = 4096 brightness levels.
-	 *
-	 * Note that this method is based on empirical testing on different
-	 * devices with PWM of 8 and 16 bits of resolution.
-	 */
-	n = period;
-	while (n) {
-		counter += n % 2;
-		n >>= 1;
-	}
-
-	data->max_brightness = DIV_ROUND_UP(period, counter);
-	data->levels = devm_kcalloc(dev, data->max_brightness,
+	data->levels = devm_kcalloc(dev, nlevels,
 				    sizeof(*data->levels), GFP_KERNEL);
 	if (!data->levels)
 		return -ENOMEM;
 
 	/* Fill the table using the cie1931 algorithm */
-	for (i = 0; i < data->max_brightness; i++) {
+	for (i = 0; i < nlevels; i++) {
 		retval = cie1931((i * PWM_LUMINANCE_SCALE) /
-				 data->max_brightness, PWM_LUMINANCE_SCALE) *
+				 nlevels, PWM_LUMINANCE_SCALE) *
 				 period;
 		retval = DIV_ROUND_CLOSEST_ULL(retval, PWM_LUMINANCE_SCALE);
 		if (retval > UINT_MAX)
@@ -233,8 +214,7 @@ int pwm_backlight_brightness_default(struct device *dev,
 		data->levels[i] = (unsigned int)retval;
 	}
 
-	data->dft_brightness = data->max_brightness / 2;
-	data->max_brightness--;
+	data->dft_brightness = nlevels / 2;
 
 	return 0;
 }
@@ -272,8 +252,13 @@ static int pwm_backlight_parse_dt(struct device *dev,
 	 * set a default table of brightness levels will be used.
 	 */
 	prop = of_find_property(node, "brightness-levels", &length);
-	if (!prop)
+	if (!prop) {
+		if (of_property_read_u32(node, "max-brightness",
+					 &data->max_brightness))
+			data->max_brightness = 255;
+
 		return 0;
+	}
 
 	data->max_brightness = length / sizeof(u32);
 
@@ -565,13 +550,10 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 
 			pb->levels = data->levels;
 		}
-	} else if (!data->max_brightness) {
+	} else if (node) {
 		/*
-		 * If no brightness levels are provided and max_brightness is
-		 * not set, use the default brightness table. For the DT case,
-		 * max_brightness is set to 0 when brightness levels is not
-		 * specified. For the non-DT case, max_brightness is usually
-		 * set to some value.
+		 * If no brightness levels are provided use the default
+		 * brightness table.
 		 */
 
 		/* Get the PWM period (in nanoseconds) */
@@ -591,12 +573,13 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 
 			pb->levels = data->levels;
 		}
-	} else {
-		/*
-		 * That only happens for the non-DT case, where platform data
-		 * sets the max_brightness value.
-		 */
+	} else if (data->max_brightness) {
+		/* non-DT case, max_brightness value set in platform data. */
 		pb->scale = data->max_brightness;
+	} else {
+		dev_err(&pdev->dev, "max brightness is not specified\n");
+		ret = -EINVAL;
+		goto err_alloc;
 	}
 
 	pb->lth_brightness = data->lth_brightness * (state.period / pb->scale);
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

[PATCH 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree

[Matthias Kaehlcke]

Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED
linearly to human eye") uses pwm_period / hweight32(pwm_period) as
as heuristic to determine the number of brightness levels when the DT
doesn't provide a brightness level table. This heuristic is broken
and can result in excessively large brightness tables.

Instead of using the heuristic try to retrieve the number of
brightness levels from the device tree (property 'max-brightness'
+ 1). If the value is not specified use a default of 256 levels.

Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye")
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/video/backlight/pwm_bl.c | 59 ++++++++++++--------------------
 1 file changed, 21 insertions(+), 38 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index fb45f866b923..2913cbe9cfcb 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -194,38 +194,19 @@ int pwm_backlight_brightness_default(struct device *dev,
 				     struct platform_pwm_backlight_data *data,
 				     unsigned int period)
 {
-	unsigned int counter = 0;
-	unsigned int i, n;
+	unsigned int i;
+	unsigned int nlevels = data->max_brightness + 1;
 	u64 retval;
 
-	/*
-	 * Count the number of bits needed to represent the period number. The
-	 * number of bits is used to calculate the number of levels used for the
-	 * brightness-levels table, the purpose of this calculation is have a
-	 * pre-computed table with enough levels to get linear brightness
-	 * perception. The period is divided by the number of bits so for a
-	 * 8-bit PWM we have 255 / 8 = 32 brightness levels or for a 16-bit PWM
-	 * we have 65535 / 16 = 4096 brightness levels.
-	 *
-	 * Note that this method is based on empirical testing on different
-	 * devices with PWM of 8 and 16 bits of resolution.
-	 */
-	n = period;
-	while (n) {
-		counter += n % 2;
-		n >>= 1;
-	}
-
-	data->max_brightness = DIV_ROUND_UP(period, counter);
-	data->levels = devm_kcalloc(dev, data->max_brightness,
+	data->levels = devm_kcalloc(dev, nlevels,
 				    sizeof(*data->levels), GFP_KERNEL);
 	if (!data->levels)
 		return -ENOMEM;
 
 	/* Fill the table using the cie1931 algorithm */
-	for (i = 0; i < data->max_brightness; i++) {
+	for (i = 0; i < nlevels; i++) {
 		retval = cie1931((i * PWM_LUMINANCE_SCALE) /
-				 data->max_brightness, PWM_LUMINANCE_SCALE) *
+				 nlevels, PWM_LUMINANCE_SCALE) *
 				 period;
 		retval = DIV_ROUND_CLOSEST_ULL(retval, PWM_LUMINANCE_SCALE);
 		if (retval > UINT_MAX)
@@ -233,8 +214,7 @@ int pwm_backlight_brightness_default(struct device *dev,
 		data->levels[i] = (unsigned int)retval;
 	}
 
-	data->dft_brightness = data->max_brightness / 2;
-	data->max_brightness--;
+	data->dft_brightness = nlevels / 2;
 
 	return 0;
 }
@@ -272,8 +252,13 @@ static int pwm_backlight_parse_dt(struct device *dev,
 	 * set a default table of brightness levels will be used.
 	 */
 	prop = of_find_property(node, "brightness-levels", &length);
-	if (!prop)
+	if (!prop) {
+		if (of_property_read_u32(node, "max-brightness",
+					 &data->max_brightness))
+			data->max_brightness = 255;
+
 		return 0;
+	}
 
 	data->max_brightness = length / sizeof(u32);
 
@@ -565,13 +550,10 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 
 			pb->levels = data->levels;
 		}
-	} else if (!data->max_brightness) {
+	} else if (node) {
 		/*
-		 * If no brightness levels are provided and max_brightness is
-		 * not set, use the default brightness table. For the DT case,
-		 * max_brightness is set to 0 when brightness levels is not
-		 * specified. For the non-DT case, max_brightness is usually
-		 * set to some value.
+		 * If no brightness levels are provided use the default
+		 * brightness table.
 		 */
 
 		/* Get the PWM period (in nanoseconds) */
@@ -591,12 +573,13 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 
 			pb->levels = data->levels;
 		}
-	} else {
-		/*
-		 * That only happens for the non-DT case, where platform data
-		 * sets the max_brightness value.
-		 */
+	} else if (data->max_brightness) {
+		/* non-DT case, max_brightness value set in platform data. */
 		pb->scale = data->max_brightness;
+	} else {
+		dev_err(&pdev->dev, "max brightness is not specified\n");
+		ret = -EINVAL;
+		goto err_alloc;
 	}
 
 	pb->lth_brightness = data->lth_brightness * (state.period / pb->scale);
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

Re: [PATCH 1/2] dt-bindings: pwm-backlight: Add 'max-brightness' property

[Pavel Machek]

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

On Mon 2019-06-10 16:37:38, Matthias Kaehlcke wrote:
> Add an optional 'max-brightness' property, which is used to specify
> the number of brightness levels (max-brightness + 1) when the node
> has no 'brightness-levels' table.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(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 1/2] dt-bindings: pwm-backlight: Add 'max-brightness' property

[Pavel Machek]

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

On Mon 2019-06-10 16:37:38, Matthias Kaehlcke wrote:
> Add an optional 'max-brightness' property, which is used to specify
> the number of brightness levels (max-brightness + 1) when the node
> has no 'brightness-levels' table.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(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 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1106 bytes --]

On Mon 2019-06-10 16:37:39, Matthias Kaehlcke wrote:
> Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED
> linearly to human eye") uses pwm_period / hweight32(pwm_period) as
> as heuristic to determine the number of brightness levels when the DT
> doesn't provide a brightness level table. This heuristic is broken
> and can result in excessively large brightness tables.
> 
> Instead of using the heuristic try to retrieve the number of
> brightness levels from the device tree (property 'max-brightness'
> + 1). If the value is not specified use a default of 256 levels.
> 
> Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye")

I don't think this one is suitable for stable. I'm pretty sure the
heuristics works well for many boards, and you just replaced it with
another heuristics ("256").

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

Acked-by: Pavel Machek <pavel@ucw.cz>

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

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree

[Pavel Machek]

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

On Mon 2019-06-10 16:37:39, Matthias Kaehlcke wrote:
> Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED
> linearly to human eye") uses pwm_period / hweight32(pwm_period) as
> as heuristic to determine the number of brightness levels when the DT
> doesn't provide a brightness level table. This heuristic is broken
> and can result in excessively large brightness tables.
> 
> Instead of using the heuristic try to retrieve the number of
> brightness levels from the device tree (property 'max-brightness'
> + 1). If the value is not specified use a default of 256 levels.
> 
> Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye")

I don't think this one is suitable for stable. I'm pretty sure the
heuristics works well for many boards, and you just replaced it with
another heuristics ("256").

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

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(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 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree

[Pavel Machek]

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

On Mon 2019-06-10 16:37:39, Matthias Kaehlcke wrote:
> Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED
> linearly to human eye") uses pwm_period / hweight32(pwm_period) as
> as heuristic to determine the number of brightness levels when the DT
> doesn't provide a brightness level table. This heuristic is broken
> and can result in excessively large brightness tables.
> 
> Instead of using the heuristic try to retrieve the number of
> brightness levels from the device tree (property 'max-brightness'
> + 1). If the value is not specified use a default of 256 levels.
> 
> Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye")

I don't think this one is suitable for stable. I'm pretty sure the
heuristics works well for many boards, and you just replaced it with
another heuristics ("256").

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

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(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 1/2] dt-bindings: pwm-backlight: Add 'max-brightness' property

[Thierry Reding]

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

On Mon, Jun 10, 2019 at 04:37:38PM -0700, Matthias Kaehlcke wrote:
> Add an optional 'max-brightness' property, which is used to specify
> the number of brightness levels (max-brightness + 1) when the node
> has no 'brightness-levels' table.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  .../devicetree/bindings/leds/backlight/pwm-backlight.txt       | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> index 64fa2fbd98c9..98f4ba626054 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> @@ -27,6 +27,9 @@ Optional properties:
>                              resolution pwm duty cycle can be used without
>                              having to list out every possible value in the
>                              brightness-level array.
> +  - max-brightness: Maximum brightness value. Used to specify the number of
> +                    brightness levels (max-brightness + 1) when the node
> +                    has no 'brightness-levels' table.

Back at the time when these bindings were defined we specifically didn't
add this because it was deemed impractical. That is, no real hardware is
actually capable of achieving useful results with a simplified
description like this.

Besides, we already have the num-interpolated-steps property which
should allow you to achieve the same thing:

	brightness-levels = <0 255>;
	default-brightness-level = <1>;
	num-interpolated-steps = <255>;

Though given the original discussion that we had around how backlight
hardware behaves, that doesn't seem like a good choice.

Thierry

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

Re: [PATCH 1/2] dt-bindings: pwm-backlight: Add 'max-brightness' property

[Thierry Reding]

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

On Mon, Jun 10, 2019 at 04:37:38PM -0700, Matthias Kaehlcke wrote:
> Add an optional 'max-brightness' property, which is used to specify
> the number of brightness levels (max-brightness + 1) when the node
> has no 'brightness-levels' table.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  .../devicetree/bindings/leds/backlight/pwm-backlight.txt       | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> index 64fa2fbd98c9..98f4ba626054 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> @@ -27,6 +27,9 @@ Optional properties:
>                              resolution pwm duty cycle can be used without
>                              having to list out every possible value in the
>                              brightness-level array.
> +  - max-brightness: Maximum brightness value. Used to specify the number of
> +                    brightness levels (max-brightness + 1) when the node
> +                    has no 'brightness-levels' table.

Back at the time when these bindings were defined we specifically didn't
add this because it was deemed impractical. That is, no real hardware is
actually capable of achieving useful results with a simplified
description like this.

Besides, we already have the num-interpolated-steps property which
should allow you to achieve the same thing:

	brightness-levels = <0 255>;
	default-brightness-level = <1>;
	num-interpolated-steps = <255>;

Though given the original discussion that we had around how backlight
hardware behaves, that doesn't seem like a good choice.

Thierry

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

Re: [PATCH 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree

[Daniel Thompson]

On Mon, Jun 10, 2019 at 04:37:39PM -0700, Matthias Kaehlcke wrote:
> Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED
> linearly to human eye") uses pwm_period / hweight32(pwm_period) as
> as heuristic to determine the number of brightness levels when the DT
> doesn't provide a brightness level table. This heuristic is broken
> and can result in excessively large brightness tables.
> 
> Instead of using the heuristic try to retrieve the number of
> brightness levels from the device tree (property 'max-brightness'
> + 1). If the value is not specified use a default of 256 levels.

I'll look at the code tomorrow but why 256?

To me it feels simultaneously too big for a simple 8-bit PWM and too
small for animated backlight effects.

I certainly agree that an override could be useful but I'm not clear why
deriving a default based on the period is bogus (and the description is
merely concerned about uselessly big tables).

/*
 * Once we have 4096 levels there's little point going much higher...
 * neither interactive sliders nor animation benefits from having
 * more values in the table.
 */
max_brightness = min(DIV_ROUND_UP(period, ffs(period), 4096);


Daniel.

> 
> Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye")
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/video/backlight/pwm_bl.c | 59 ++++++++++++--------------------
>  1 file changed, 21 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index fb45f866b923..2913cbe9cfcb 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -194,38 +194,19 @@ int pwm_backlight_brightness_default(struct device *dev,
>  				     struct platform_pwm_backlight_data *data,
>  				     unsigned int period)
>  {
> -	unsigned int counter = 0;
> -	unsigned int i, n;
> +	unsigned int i;
> +	unsigned int nlevels = data->max_brightness + 1;
>  	u64 retval;
>  
> -	/*
> -	 * Count the number of bits needed to represent the period number. The
> -	 * number of bits is used to calculate the number of levels used for the
> -	 * brightness-levels table, the purpose of this calculation is have a
> -	 * pre-computed table with enough levels to get linear brightness
> -	 * perception. The period is divided by the number of bits so for a
> -	 * 8-bit PWM we have 255 / 8 = 32 brightness levels or for a 16-bit PWM
> -	 * we have 65535 / 16 = 4096 brightness levels.
> -	 *
> -	 * Note that this method is based on empirical testing on different
> -	 * devices with PWM of 8 and 16 bits of resolution.
> -	 */
> -	n = period;
> -	while (n) {
> -		counter += n % 2;
> -		n >>= 1;
> -	}
> -
> -	data->max_brightness = DIV_ROUND_UP(period, counter);
> -	data->levels = devm_kcalloc(dev, data->max_brightness,
> +	data->levels = devm_kcalloc(dev, nlevels,
>  				    sizeof(*data->levels), GFP_KERNEL);
>  	if (!data->levels)
>  		return -ENOMEM;
>  
>  	/* Fill the table using the cie1931 algorithm */
> -	for (i = 0; i < data->max_brightness; i++) {
> +	for (i = 0; i < nlevels; i++) {
>  		retval = cie1931((i * PWM_LUMINANCE_SCALE) /
> -				 data->max_brightness, PWM_LUMINANCE_SCALE) *
> +				 nlevels, PWM_LUMINANCE_SCALE) *
>  				 period;
>  		retval = DIV_ROUND_CLOSEST_ULL(retval, PWM_LUMINANCE_SCALE);
>  		if (retval > UINT_MAX)
> @@ -233,8 +214,7 @@ int pwm_backlight_brightness_default(struct device *dev,
>  		data->levels[i] = (unsigned int)retval;
>  	}
>  
> -	data->dft_brightness = data->max_brightness / 2;
> -	data->max_brightness--;
> +	data->dft_brightness = nlevels / 2;
>  
>  	return 0;
>  }
> @@ -272,8 +252,13 @@ static int pwm_backlight_parse_dt(struct device *dev,
>  	 * set a default table of brightness levels will be used.
>  	 */
>  	prop = of_find_property(node, "brightness-levels", &length);
> -	if (!prop)
> +	if (!prop) {
> +		if (of_property_read_u32(node, "max-brightness",
> +					 &data->max_brightness))
> +			data->max_brightness = 255;
> +
>  		return 0;
> +	}
>  
>  	data->max_brightness = length / sizeof(u32);
>  
> @@ -565,13 +550,10 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  
>  			pb->levels = data->levels;
>  		}
> -	} else if (!data->max_brightness) {
> +	} else if (node) {
>  		/*
> -		 * If no brightness levels are provided and max_brightness is
> -		 * not set, use the default brightness table. For the DT case,
> -		 * max_brightness is set to 0 when brightness levels is not
> -		 * specified. For the non-DT case, max_brightness is usually
> -		 * set to some value.
> +		 * If no brightness levels are provided use the default
> +		 * brightness table.
>  		 */
>  
>  		/* Get the PWM period (in nanoseconds) */
> @@ -591,12 +573,13 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  
>  			pb->levels = data->levels;
>  		}
> -	} else {
> -		/*
> -		 * That only happens for the non-DT case, where platform data
> -		 * sets the max_brightness value.
> -		 */
> +	} else if (data->max_brightness) {
> +		/* non-DT case, max_brightness value set in platform data. */
>  		pb->scale = data->max_brightness;
> +	} else {
> +		dev_err(&pdev->dev, "max brightness is not specified\n");
> +		ret = -EINVAL;
> +		goto err_alloc;
>  	}
>  
>  	pb->lth_brightness = data->lth_brightness * (state.period / pb->scale);
> -- 
> 2.22.0.rc2.383.gf4fbbf30c2-goog
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree

[Daniel Thompson]

On Mon, Jun 10, 2019 at 04:37:39PM -0700, Matthias Kaehlcke wrote:
> Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED
> linearly to human eye") uses pwm_period / hweight32(pwm_period) as
> as heuristic to determine the number of brightness levels when the DT
> doesn't provide a brightness level table. This heuristic is broken
> and can result in excessively large brightness tables.
> 
> Instead of using the heuristic try to retrieve the number of
> brightness levels from the device tree (property 'max-brightness'
> + 1). If the value is not specified use a default of 256 levels.

I'll look at the code tomorrow but why 256?

To me it feels simultaneously too big for a simple 8-bit PWM and too
small for animated backlight effects.

I certainly agree that an override could be useful but I'm not clear why
deriving a default based on the period is bogus (and the description is
merely concerned about uselessly big tables).

/*
 * Once we have 4096 levels there's little point going much higher...
 * neither interactive sliders nor animation benefits from having
 * more values in the table.
 */
max_brightness = min(DIV_ROUND_UP(period, ffs(period), 4096);


Daniel.

> 
> Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye")
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/video/backlight/pwm_bl.c | 59 ++++++++++++--------------------
>  1 file changed, 21 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index fb45f866b923..2913cbe9cfcb 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -194,38 +194,19 @@ int pwm_backlight_brightness_default(struct device *dev,
>  				     struct platform_pwm_backlight_data *data,
>  				     unsigned int period)
>  {
> -	unsigned int counter = 0;
> -	unsigned int i, n;
> +	unsigned int i;
> +	unsigned int nlevels = data->max_brightness + 1;
>  	u64 retval;
>  
> -	/*
> -	 * Count the number of bits needed to represent the period number. The
> -	 * number of bits is used to calculate the number of levels used for the
> -	 * brightness-levels table, the purpose of this calculation is have a
> -	 * pre-computed table with enough levels to get linear brightness
> -	 * perception. The period is divided by the number of bits so for a
> -	 * 8-bit PWM we have 255 / 8 = 32 brightness levels or for a 16-bit PWM
> -	 * we have 65535 / 16 = 4096 brightness levels.
> -	 *
> -	 * Note that this method is based on empirical testing on different
> -	 * devices with PWM of 8 and 16 bits of resolution.
> -	 */
> -	n = period;
> -	while (n) {
> -		counter += n % 2;
> -		n >>= 1;
> -	}
> -
> -	data->max_brightness = DIV_ROUND_UP(period, counter);
> -	data->levels = devm_kcalloc(dev, data->max_brightness,
> +	data->levels = devm_kcalloc(dev, nlevels,
>  				    sizeof(*data->levels), GFP_KERNEL);
>  	if (!data->levels)
>  		return -ENOMEM;
>  
>  	/* Fill the table using the cie1931 algorithm */
> -	for (i = 0; i < data->max_brightness; i++) {
> +	for (i = 0; i < nlevels; i++) {
>  		retval = cie1931((i * PWM_LUMINANCE_SCALE) /
> -				 data->max_brightness, PWM_LUMINANCE_SCALE) *
> +				 nlevels, PWM_LUMINANCE_SCALE) *
>  				 period;
>  		retval = DIV_ROUND_CLOSEST_ULL(retval, PWM_LUMINANCE_SCALE);
>  		if (retval > UINT_MAX)
> @@ -233,8 +214,7 @@ int pwm_backlight_brightness_default(struct device *dev,
>  		data->levels[i] = (unsigned int)retval;
>  	}
>  
> -	data->dft_brightness = data->max_brightness / 2;
> -	data->max_brightness--;
> +	data->dft_brightness = nlevels / 2;
>  
>  	return 0;
>  }
> @@ -272,8 +252,13 @@ static int pwm_backlight_parse_dt(struct device *dev,
>  	 * set a default table of brightness levels will be used.
>  	 */
>  	prop = of_find_property(node, "brightness-levels", &length);
> -	if (!prop)
> +	if (!prop) {
> +		if (of_property_read_u32(node, "max-brightness",
> +					 &data->max_brightness))
> +			data->max_brightness = 255;
> +
>  		return 0;
> +	}
>  
>  	data->max_brightness = length / sizeof(u32);
>  
> @@ -565,13 +550,10 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  
>  			pb->levels = data->levels;
>  		}
> -	} else if (!data->max_brightness) {
> +	} else if (node) {
>  		/*
> -		 * If no brightness levels are provided and max_brightness is
> -		 * not set, use the default brightness table. For the DT case,
> -		 * max_brightness is set to 0 when brightness levels is not
> -		 * specified. For the non-DT case, max_brightness is usually
> -		 * set to some value.
> +		 * If no brightness levels are provided use the default
> +		 * brightness table.
>  		 */
>  
>  		/* Get the PWM period (in nanoseconds) */
> @@ -591,12 +573,13 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  
>  			pb->levels = data->levels;
>  		}
> -	} else {
> -		/*
> -		 * That only happens for the non-DT case, where platform data
> -		 * sets the max_brightness value.
> -		 */
> +	} else if (data->max_brightness) {
> +		/* non-DT case, max_brightness value set in platform data. */
>  		pb->scale = data->max_brightness;
> +	} else {
> +		dev_err(&pdev->dev, "max brightness is not specified\n");
> +		ret = -EINVAL;
> +		goto err_alloc;
>  	}
>  
>  	pb->lth_brightness = data->lth_brightness * (state.period / pb->scale);
> -- 
> 2.22.0.rc2.383.gf4fbbf30c2-goog
> 

Re: [PATCH 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree

[Daniel Thompson]

On Mon, Jun 10, 2019 at 04:37:39PM -0700, Matthias Kaehlcke wrote:
> Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED
> linearly to human eye") uses pwm_period / hweight32(pwm_period) as
> as heuristic to determine the number of brightness levels when the DT
> doesn't provide a brightness level table. This heuristic is broken
> and can result in excessively large brightness tables.
> 
> Instead of using the heuristic try to retrieve the number of
> brightness levels from the device tree (property 'max-brightness'
> + 1). If the value is not specified use a default of 256 levels.

I'll look at the code tomorrow but why 256?

To me it feels simultaneously too big for a simple 8-bit PWM and too
small for animated backlight effects.

I certainly agree that an override could be useful but I'm not clear why
deriving a default based on the period is bogus (and the description is
merely concerned about uselessly big tables).

/*
 * Once we have 4096 levels there's little point going much higher...
 * neither interactive sliders nor animation benefits from having
 * more values in the table.
 */
max_brightness = min(DIV_ROUND_UP(period, ffs(period), 4096);


Daniel.

> 
> Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye")
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/video/backlight/pwm_bl.c | 59 ++++++++++++--------------------
>  1 file changed, 21 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index fb45f866b923..2913cbe9cfcb 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -194,38 +194,19 @@ int pwm_backlight_brightness_default(struct device *dev,
>  				     struct platform_pwm_backlight_data *data,
>  				     unsigned int period)
>  {
> -	unsigned int counter = 0;
> -	unsigned int i, n;
> +	unsigned int i;
> +	unsigned int nlevels = data->max_brightness + 1;
>  	u64 retval;
>  
> -	/*
> -	 * Count the number of bits needed to represent the period number. The
> -	 * number of bits is used to calculate the number of levels used for the
> -	 * brightness-levels table, the purpose of this calculation is have a
> -	 * pre-computed table with enough levels to get linear brightness
> -	 * perception. The period is divided by the number of bits so for a
> -	 * 8-bit PWM we have 255 / 8 = 32 brightness levels or for a 16-bit PWM
> -	 * we have 65535 / 16 = 4096 brightness levels.
> -	 *
> -	 * Note that this method is based on empirical testing on different
> -	 * devices with PWM of 8 and 16 bits of resolution.
> -	 */
> -	n = period;
> -	while (n) {
> -		counter += n % 2;
> -		n >>= 1;
> -	}
> -
> -	data->max_brightness = DIV_ROUND_UP(period, counter);
> -	data->levels = devm_kcalloc(dev, data->max_brightness,
> +	data->levels = devm_kcalloc(dev, nlevels,
>  				    sizeof(*data->levels), GFP_KERNEL);
>  	if (!data->levels)
>  		return -ENOMEM;
>  
>  	/* Fill the table using the cie1931 algorithm */
> -	for (i = 0; i < data->max_brightness; i++) {
> +	for (i = 0; i < nlevels; i++) {
>  		retval = cie1931((i * PWM_LUMINANCE_SCALE) /
> -				 data->max_brightness, PWM_LUMINANCE_SCALE) *
> +				 nlevels, PWM_LUMINANCE_SCALE) *
>  				 period;
>  		retval = DIV_ROUND_CLOSEST_ULL(retval, PWM_LUMINANCE_SCALE);
>  		if (retval > UINT_MAX)
> @@ -233,8 +214,7 @@ int pwm_backlight_brightness_default(struct device *dev,
>  		data->levels[i] = (unsigned int)retval;
>  	}
>  
> -	data->dft_brightness = data->max_brightness / 2;
> -	data->max_brightness--;
> +	data->dft_brightness = nlevels / 2;
>  
>  	return 0;
>  }
> @@ -272,8 +252,13 @@ static int pwm_backlight_parse_dt(struct device *dev,
>  	 * set a default table of brightness levels will be used.
>  	 */
>  	prop = of_find_property(node, "brightness-levels", &length);
> -	if (!prop)
> +	if (!prop) {
> +		if (of_property_read_u32(node, "max-brightness",
> +					 &data->max_brightness))
> +			data->max_brightness = 255;
> +
>  		return 0;
> +	}
>  
>  	data->max_brightness = length / sizeof(u32);
>  
> @@ -565,13 +550,10 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  
>  			pb->levels = data->levels;
>  		}
> -	} else if (!data->max_brightness) {
> +	} else if (node) {
>  		/*
> -		 * If no brightness levels are provided and max_brightness is
> -		 * not set, use the default brightness table. For the DT case,
> -		 * max_brightness is set to 0 when brightness levels is not
> -		 * specified. For the non-DT case, max_brightness is usually
> -		 * set to some value.
> +		 * If no brightness levels are provided use the default
> +		 * brightness table.
>  		 */
>  
>  		/* Get the PWM period (in nanoseconds) */
> @@ -591,12 +573,13 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  
>  			pb->levels = data->levels;
>  		}
> -	} else {
> -		/*
> -		 * That only happens for the non-DT case, where platform data
> -		 * sets the max_brightness value.
> -		 */
> +	} else if (data->max_brightness) {
> +		/* non-DT case, max_brightness value set in platform data. */
>  		pb->scale = data->max_brightness;
> +	} else {
> +		dev_err(&pdev->dev, "max brightness is not specified\n");
> +		ret = -EINVAL;
> +		goto err_alloc;
>  	}
>  
>  	pb->lth_brightness = data->lth_brightness * (state.period / pb->scale);
> -- 
> 2.22.0.rc2.383.gf4fbbf30c2-goog
> 

Re: [PATCH 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree

[Matthias Kaehlcke]

Hi Daniel,

On Tue, Jun 11, 2019 at 04:33:14PM +0100, Daniel Thompson wrote:
> On Mon, Jun 10, 2019 at 04:37:39PM -0700, Matthias Kaehlcke wrote:
> > Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED
> > linearly to human eye") uses pwm_period / hweight32(pwm_period) as
> > as heuristic to determine the number of brightness levels when the DT
> > doesn't provide a brightness level table. This heuristic is broken
> > and can result in excessively large brightness tables.
> > 
> > Instead of using the heuristic try to retrieve the number of
> > brightness levels from the device tree (property 'max-brightness'
> > + 1). If the value is not specified use a default of 256 levels.
> 
> I'll look at the code tomorrow but why 256?
> 
> To me it feels simultaneously too big for a simple 8-bit PWM and too
> small for animated backlight effects.

I agree there is no one-size-fits-it-all default, 256 seemed like a
possible compromise.

> I certainly agree that an override could be useful but I'm not clear why
> deriving a default based on the period is bogus (and the description is
> merely concerned about uselessly big tables).

Maybe it's not necessarily bogus, but the current heuristic that
counts the number of set bits (hweight()) in the period certainly is.

IIUC the period provides a clue about the PWM resolution, because it
would be hard/impossible to accomodate the high resolution in shorter
periods.

> /*
>  * Once we have 4096 levels there's little point going much higher...
>  * neither interactive sliders nor animation benefits from having
>  * more values in the table.
>  */
> max_brightness = min(DIV_ROUND_UP(period, ffs(period), 4096);

I was also considering something along these lines, but wasn't sure
if there is indeed a relation between the period and the PWM
resolution. I take your suggestion as a confirmation :)

Re: [PATCH 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree

[Matthias Kaehlcke]

Hi Daniel,

On Tue, Jun 11, 2019 at 04:33:14PM +0100, Daniel Thompson wrote:
> On Mon, Jun 10, 2019 at 04:37:39PM -0700, Matthias Kaehlcke wrote:
> > Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED
> > linearly to human eye") uses pwm_period / hweight32(pwm_period) as
> > as heuristic to determine the number of brightness levels when the DT
> > doesn't provide a brightness level table. This heuristic is broken
> > and can result in excessively large brightness tables.
> > 
> > Instead of using the heuristic try to retrieve the number of
> > brightness levels from the device tree (property 'max-brightness'
> > + 1). If the value is not specified use a default of 256 levels.
> 
> I'll look at the code tomorrow but why 256?
> 
> To me it feels simultaneously too big for a simple 8-bit PWM and too
> small for animated backlight effects.

I agree there is no one-size-fits-it-all default, 256 seemed like a
possible compromise.

> I certainly agree that an override could be useful but I'm not clear why
> deriving a default based on the period is bogus (and the description is
> merely concerned about uselessly big tables).

Maybe it's not necessarily bogus, but the current heuristic that
counts the number of set bits (hweight()) in the period certainly is.

IIUC the period provides a clue about the PWM resolution, because it
would be hard/impossible to accomodate the high resolution in shorter
periods.

> /*
>  * Once we have 4096 levels there's little point going much higher...
>  * neither interactive sliders nor animation benefits from having
>  * more values in the table.
>  */
> max_brightness = min(DIV_ROUND_UP(period, ffs(period), 4096);

I was also considering something along these lines, but wasn't sure
if there is indeed a relation between the period and the PWM
resolution. I take your suggestion as a confirmation :)

Re: [PATCH 1/2] dt-bindings: pwm-backlight: Add 'max-brightness' property

[Jacek Anaszewski]

Hi Matthias,

On 6/11/19 1:37 AM, Matthias Kaehlcke wrote:
> Add an optional 'max-brightness' property, which is used to specify
> the number of brightness levels (max-brightness + 1) when the node
> has no 'brightness-levels' table.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>   .../devicetree/bindings/leds/backlight/pwm-backlight.txt       | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> index 64fa2fbd98c9..98f4ba626054 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> @@ -27,6 +27,9 @@ Optional properties:
>                               resolution pwm duty cycle can be used without
>                               having to list out every possible value in the
>                               brightness-level array.
> +  - max-brightness: Maximum brightness value. Used to specify the number of
> +                    brightness levels (max-brightness + 1) when the node
> +                    has no 'brightness-levels' table.

In the LED subsystem we have led-max-microamp property which seems to
better describe hardware capabilities. It says just: this is the current
level the LED can withstand. max-brightness does not implicitly convey
this kind of information.

Why the need for the property at all? If for the reasons other than
hardware capabilities than it should be more likely handled
by userspace.

>   [0]: Documentation/devicetree/bindings/pwm/pwm.txt
>   [1]: Documentation/devicetree/bindings/gpio/gpio.txt
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/2] dt-bindings: pwm-backlight: Add 'max-brightness' property

[Jacek Anaszewski]

Hi Matthias,

On 6/11/19 1:37 AM, Matthias Kaehlcke wrote:
> Add an optional 'max-brightness' property, which is used to specify
> the number of brightness levels (max-brightness + 1) when the node
> has no 'brightness-levels' table.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>   .../devicetree/bindings/leds/backlight/pwm-backlight.txt       | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> index 64fa2fbd98c9..98f4ba626054 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> @@ -27,6 +27,9 @@ Optional properties:
>                               resolution pwm duty cycle can be used without
>                               having to list out every possible value in the
>                               brightness-level array.
> +  - max-brightness: Maximum brightness value. Used to specify the number of
> +                    brightness levels (max-brightness + 1) when the node
> +                    has no 'brightness-levels' table.

In the LED subsystem we have led-max-microamp property which seems to
better describe hardware capabilities. It says just: this is the current
level the LED can withstand. max-brightness does not implicitly convey
this kind of information.

Why the need for the property at all? If for the reasons other than
hardware capabilities than it should be more likely handled
by userspace.

>   [0]: Documentation/devicetree/bindings/pwm/pwm.txt
>   [1]: Documentation/devicetree/bindings/gpio/gpio.txt
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/2] dt-bindings: pwm-backlight: Add 'max-brightness' property

[Matthias Kaehlcke]

Hi Thierry,

On Tue, Jun 11, 2019 at 12:28:51PM +0200, Thierry Reding wrote:
> On Mon, Jun 10, 2019 at 04:37:38PM -0700, Matthias Kaehlcke wrote:
> > Add an optional 'max-brightness' property, which is used to specify
> > the number of brightness levels (max-brightness + 1) when the node
> > has no 'brightness-levels' table.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >  .../devicetree/bindings/leds/backlight/pwm-backlight.txt       | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> > index 64fa2fbd98c9..98f4ba626054 100644
> > --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> > +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> > @@ -27,6 +27,9 @@ Optional properties:
> >                              resolution pwm duty cycle can be used without
> >                              having to list out every possible value in the
> >                              brightness-level array.
> > +  - max-brightness: Maximum brightness value. Used to specify the number of
> > +                    brightness levels (max-brightness + 1) when the node
> > +                    has no 'brightness-levels' table.
> 
> Back at the time when these bindings were defined we specifically didn't
> add this because it was deemed impractical. That is, no real hardware is
> actually capable of achieving useful results with a simplified
> description like this.
> 
> Besides, we already have the num-interpolated-steps property which
> should allow you to achieve the same thing:
> 
> 	brightness-levels = <0 255>;
> 	default-brightness-level = <1>;
> 	num-interpolated-steps = <255>;

It doesn't achieve the same. With this configuration the device would
have a table with 256 linearly increasing values, the intended use of
the property is to provide the number of brightness levels to be used
by the CIE 1931 algorithm to compute a brightness scale that is
perceived as linear by the human eye. We could possibly treat a
'brightness-levels' table with only two levels as special case and
get the number of levels from it.

In any case from the discussion on "backlight: pwm_bl: Get number of
brightness levels for CIE 1931 from the device tree" it might not be
necessary to specify the number of levels in the DT.

> Though given the original discussion that we had around how backlight
> hardware behaves, that doesn't seem like a good choice.

Re: [PATCH 1/2] dt-bindings: pwm-backlight: Add 'max-brightness' property

[Matthias Kaehlcke]

Hi Thierry,

On Tue, Jun 11, 2019 at 12:28:51PM +0200, Thierry Reding wrote:
> On Mon, Jun 10, 2019 at 04:37:38PM -0700, Matthias Kaehlcke wrote:
> > Add an optional 'max-brightness' property, which is used to specify
> > the number of brightness levels (max-brightness + 1) when the node
> > has no 'brightness-levels' table.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >  .../devicetree/bindings/leds/backlight/pwm-backlight.txt       | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> > index 64fa2fbd98c9..98f4ba626054 100644
> > --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> > +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> > @@ -27,6 +27,9 @@ Optional properties:
> >                              resolution pwm duty cycle can be used without
> >                              having to list out every possible value in the
> >                              brightness-level array.
> > +  - max-brightness: Maximum brightness value. Used to specify the number of
> > +                    brightness levels (max-brightness + 1) when the node
> > +                    has no 'brightness-levels' table.
> 
> Back at the time when these bindings were defined we specifically didn't
> add this because it was deemed impractical. That is, no real hardware is
> actually capable of achieving useful results with a simplified
> description like this.
> 
> Besides, we already have the num-interpolated-steps property which
> should allow you to achieve the same thing:
> 
> 	brightness-levels = <0 255>;
> 	default-brightness-level = <1>;
> 	num-interpolated-steps = <255>;

It doesn't achieve the same. With this configuration the device would
have a table with 256 linearly increasing values, the intended use of
the property is to provide the number of brightness levels to be used
by the CIE 1931 algorithm to compute a brightness scale that is
perceived as linear by the human eye. We could possibly treat a
'brightness-levels' table with only two levels as special case and
get the number of levels from it.

In any case from the discussion on "backlight: pwm_bl: Get number of
brightness levels for CIE 1931 from the device tree" it might not be
necessary to specify the number of levels in the DT.

> Though given the original discussion that we had around how backlight
> hardware behaves, that doesn't seem like a good choice.

Re: [PATCH 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree

[Matthias Kaehlcke]

Hi Pavel,

On Tue, Jun 11, 2019 at 12:18:43PM +0200, Pavel Machek wrote:
> On Mon 2019-06-10 16:37:39, Matthias Kaehlcke wrote:
> > Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED
> > linearly to human eye") uses pwm_period / hweight32(pwm_period) as
> > as heuristic to determine the number of brightness levels when the DT
> > doesn't provide a brightness level table. This heuristic is broken
> > and can result in excessively large brightness tables.
> > 
> > Instead of using the heuristic try to retrieve the number of
> > brightness levels from the device tree (property 'max-brightness'
> > + 1). If the value is not specified use a default of 256 levels.
> > 
> > Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye")
> 
> I don't think this one is suitable for stable. I'm pretty sure the
> heuristics works well for many boards, and you just replaced it with
> another heuristics ("256").

whether the patch is suitable for stable/upstream is certainly
debatable, in any case I'd argue the current heuristic is bogus and
works by accident or at a cost:

nlevels = period / hweight(period)

w/ period = 131071 ns  (0x1FFFF)

  nlevels = 131071 / 17 = 7710

w/ period = 131072 ns (0x20000)

  nlevels = 131072 / 1 = 131072

and some PWMs use significantly higher periods like 1 ms or 10 ms.

Re: [PATCH 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree

[Matthias Kaehlcke]

Hi Pavel,

On Tue, Jun 11, 2019 at 12:18:43PM +0200, Pavel Machek wrote:
> On Mon 2019-06-10 16:37:39, Matthias Kaehlcke wrote:
> > Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED
> > linearly to human eye") uses pwm_period / hweight32(pwm_period) as
> > as heuristic to determine the number of brightness levels when the DT
> > doesn't provide a brightness level table. This heuristic is broken
> > and can result in excessively large brightness tables.
> > 
> > Instead of using the heuristic try to retrieve the number of
> > brightness levels from the device tree (property 'max-brightness'
> > + 1). If the value is not specified use a default of 256 levels.
> > 
> > Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye")
> 
> I don't think this one is suitable for stable. I'm pretty sure the
> heuristics works well for many boards, and you just replaced it with
> another heuristics ("256").

whether the patch is suitable for stable/upstream is certainly
debatable, in any case I'd argue the current heuristic is bogus and
works by accident or at a cost:

nlevels = period / hweight(period)

w/ period = 131071 ns  (0x1FFFF)

  nlevels = 131071 / 17 = 7710

w/ period = 131072 ns (0x20000)

  nlevels = 131072 / 1 = 131072

and some PWMs use significantly higher periods like 1 ms or 10 ms.

Re: [PATCH 1/2] dt-bindings: pwm-backlight: Add 'max-brightness' property

[Matthias Kaehlcke]

Hi Jacek,

On Tue, Jun 11, 2019 at 10:02:23PM +0200, Jacek Anaszewski wrote:
> Hi Matthias,
> 
> On 6/11/19 1:37 AM, Matthias Kaehlcke wrote:
> > Add an optional 'max-brightness' property, which is used to specify
> > the number of brightness levels (max-brightness + 1) when the node
> > has no 'brightness-levels' table.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >   .../devicetree/bindings/leds/backlight/pwm-backlight.txt       | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> > index 64fa2fbd98c9..98f4ba626054 100644
> > --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> > +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> > @@ -27,6 +27,9 @@ Optional properties:
> >                               resolution pwm duty cycle can be used without
> >                               having to list out every possible value in the
> >                               brightness-level array.
> > +  - max-brightness: Maximum brightness value. Used to specify the number of
> > +                    brightness levels (max-brightness + 1) when the node
> > +                    has no 'brightness-levels' table.
> 
> In the LED subsystem we have led-max-microamp property which seems to
> better describe hardware capabilities. It says just: this is the current
> level the LED can withstand. max-brightness does not implicitly convey
> this kind of information.
> 
> Why the need for the property at all? If for the reasons other than
> hardware capabilities than it should be more likely handled
> by userspace.

The driver needs to know how many brightness levels to expose to
userspace. It currently uses a heuristic for that which is broken:

https://elixir.bootlin.com/linux/v5.1.9/source/drivers/video/backlight/pwm_bl.c#L234
https://lore.kernel.org/patchwork/patch/1086777/#1282610

In any case it seems the discussion is going into the direction of
fixing the heuristic (apparently using the period as an indicator of
the PWM resolution has more merit than I was initially aware of), if
that moves forward the property wouldn't be needed.

Re: [PATCH 1/2] dt-bindings: pwm-backlight: Add 'max-brightness' property

[Matthias Kaehlcke]

Hi Jacek,

On Tue, Jun 11, 2019 at 10:02:23PM +0200, Jacek Anaszewski wrote:
> Hi Matthias,
> 
> On 6/11/19 1:37 AM, Matthias Kaehlcke wrote:
> > Add an optional 'max-brightness' property, which is used to specify
> > the number of brightness levels (max-brightness + 1) when the node
> > has no 'brightness-levels' table.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >   .../devicetree/bindings/leds/backlight/pwm-backlight.txt       | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> > index 64fa2fbd98c9..98f4ba626054 100644
> > --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> > +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> > @@ -27,6 +27,9 @@ Optional properties:
> >                               resolution pwm duty cycle can be used without
> >                               having to list out every possible value in the
> >                               brightness-level array.
> > +  - max-brightness: Maximum brightness value. Used to specify the number of
> > +                    brightness levels (max-brightness + 1) when the node
> > +                    has no 'brightness-levels' table.
> 
> In the LED subsystem we have led-max-microamp property which seems to
> better describe hardware capabilities. It says just: this is the current
> level the LED can withstand. max-brightness does not implicitly convey
> this kind of information.
> 
> Why the need for the property at all? If for the reasons other than
> hardware capabilities than it should be more likely handled
> by userspace.

The driver needs to know how many brightness levels to expose to
userspace. It currently uses a heuristic for that which is broken:

https://elixir.bootlin.com/linux/v5.1.9/source/drivers/video/backlight/pwm_bl.c#L234
https://lore.kernel.org/patchwork/patch/1086777/#1282610

In any case it seems the discussion is going into the direction of
fixing the heuristic (apparently using the period as an indicator of
the PWM resolution has more merit than I was initially aware of), if
that moves forward the property wouldn't be needed.