[RESEND PATCH v3 0/4] backlight: pwm_bl: support linear interpolation and brightness to human eye

[RESEND PATCH v3 0/4] backlight: pwm_bl: support linear interpolation and brightness to human eye

[Enric Balletbo i Serra]

Dear all,

This series is a third patchset (resend )integrating the requested
changes.

The first and second patch what tries to solve is the problem of
granularity for high resolution PWMs. The idea is simple interpolate
between 2 brightness values so we can have a high PWM duty cycle (a
16 bits PWM is up to 65535 possible steps) without having to list
out every possible value in the dts. I think that this patch is
required to not break backward compatibility, to be more flexible and
also extend the functionality to be able to use high resolution PWM
with enough steps to have a good UI experience in userspace.

The thirth and fourth patch is a bit more ambicious, the idea is let
decide the driver the brightness-levels required in function of the PWM
resolution. To do this create a brightness-levels table filled with the
CIE 1931 algorithm values to convert brightness to PWM duty cycle.

More detailed info is available in the commit message of every patch.

Both functionalities were tested on a Samsung Chromebook Plus (that has
a 16 bits PWM) and a SL50 device (with a 8 bits PWM)

Waiting for your feedback.

Best regards

Enric Balletbo i Serra (4):
  backlight: pwm_bl: linear interpolation between brightness-levels
  dt-bindings: pwm-backlight: add a num-interpolation-steps property.
  backlight: pwm_bl: compute brightness of LED linearly to human eye.
  dt-bindings: pwm-backlight: move brightness-levels to optional.

 .../bindings/leds/backlight/pwm-backlight.txt      |  34 ++-
 drivers/video/backlight/pwm_bl.c                   | 232 +++++++++++++++++++--
 2 files changed, 246 insertions(+), 20 deletions(-)

-- 
2.16.3

[RESEND PATCH v3 1/4] backlight: pwm_bl: linear interpolation between brightness-levels

[Enric Balletbo i Serra]

Setting num-interpolated-steps in the dts will allow you to have linear
interpolation between values of brightness-levels. This way a high
resolution pwm duty cycle can be used without having to list out every
possible value in the dts. This system also allows for gamma corrected
values.

The most simple example is interpolate between two brightness values a
number of steps, this can be done setting the following in the dts:

  brightness-levels = <0 65535>;
  num-interpolated-steps = <1024>;
  default-brightness-level = <512>;

This will create a brightness-level table with the following values:

  <0 63 126 189 252 315 378 441 ... 64260 64323 64386 64449 65535>

Another use case can be describe a gamma corrected curve, as we have
better sensitivity at low luminance than high luminance we probably
want have smaller steps for low brightness levels values and bigger
steps for high brightness levels values. This can be achieved with
the following in the dts:

  brightness-levels = <0 4096 65535>;
  num-interpolated-steps = <1024>;
  default-brightness-level = <512>;

This will create a brightness-levels table with the following values:

  <0 4 8 12 16 20 ... 4096 4156 4216 4276 ... 65535>

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 drivers/video/backlight/pwm_bl.c | 83 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 8e3f1245f5c5..f0a108ab570a 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -147,7 +147,11 @@ static int pwm_backlight_parse_dt(struct device *dev,
 				  struct platform_pwm_backlight_data *data)
 {
 	struct device_node *node = dev->of_node;
+	unsigned int num_levels = 0;
+	unsigned int levels_count;
+	unsigned int num_steps;
 	struct property *prop;
+	unsigned int *table;
 	int length;
 	u32 value;
 	int ret;
@@ -167,6 +171,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
 	/* read brightness levels from DT property */
 	if (data->max_brightness > 0) {
 		size_t size = sizeof(*data->levels) * data->max_brightness;
+		unsigned int i, j, n = 0;
 
 		data->levels = devm_kzalloc(dev, size, GFP_KERNEL);
 		if (!data->levels)
@@ -184,6 +189,84 @@ static int pwm_backlight_parse_dt(struct device *dev,
 			return ret;
 
 		data->dft_brightness = value;
+
+		/*
+		 * This property is optional, if is set enables linear
+		 * interpolation between each of the values of brightness levels
+		 * and creates a new pre-computed table.
+		 */
+		of_property_read_u32(node, "num-interpolated-steps",
+				     &num_steps);
+
+		/*
+		 * Make sure that there is at least two entries in the
+		 * brightness-levels table, otherwise we can't interpolate
+		 * between two points.
+		 */
+		if (num_steps) {
+			if (data->max_brightness < 2) {
+				dev_err(dev, "can't interpolate\n");
+				return -EINVAL;
+			}
+
+			/*
+			 * Recalculate the number of brightness levels, now
+			 * taking in consideration the number of interpolated
+			 * steps between two levels.
+			 */
+			for (i = 0; i < data->max_brightness - 1; i++) {
+				if ((data->levels[i + 1] - data->levels[i]) /
+				   num_steps)
+					num_levels += num_steps;
+				else
+					num_levels++;
+			}
+			num_levels++;
+			dev_dbg(dev, "new number of brightness levels: %d\n",
+				num_levels);
+
+			/*
+			 * Create a new table of brightness levels with all the
+			 * interpolated steps.
+			 */
+			size = sizeof(*table) * num_levels;
+			table = devm_kzalloc(dev, size, GFP_KERNEL);
+			if (!table)
+				return -ENOMEM;
+
+			/* Fill the interpolated table. */
+			levels_count = 0;
+			for (i = 0; i < data->max_brightness - 1; i++) {
+				value = data->levels[i];
+				n = (data->levels[i + 1] - value) / num_steps;
+				if (n > 0) {
+					for (j = 0; j < num_steps; j++) {
+						table[levels_count] = value;
+						value += n;
+						levels_count++;
+					}
+				} else {
+					table[levels_count] = data->levels[i];
+					levels_count++;
+				}
+			}
+			table[levels_count] = data->levels[i];
+
+			/*
+			 * As we use interpolation lets remove current
+			 * brightness levels table and replace for the
+			 * new interpolated table.
+			 */
+			devm_kfree(dev, data->levels);
+			data->levels = table;
+
+			/*
+			 * Reassign max_brightness value to the new total number
+			 * of brightness levels.
+			 */
+			data->max_brightness = num_levels;
+		}
+
 		data->max_brightness--;
 	}
 
-- 
2.16.3

REGRESSION: [RESEND PATCH v3 1/4] backlight: pwm_bl: linear interpolation between brightness-levels

[Marcel Ziswiler]

On Mon, 2018-04-09 at 10:33 +0200, Enric Balletbo i Serra wrote:
> Setting num-interpolated-steps in the dts will allow you to have
> linear
> interpolation between values of brightness-levels. This way a high
> resolution pwm duty cycle can be used without having to list out
> every
> possible value in the dts. This system also allows for gamma
> corrected
> values.
> 
> The most simple example is interpolate between two brightness values
> a
> number of steps, this can be done setting the following in the dts:
> 
>   brightness-levels = <0 65535>;
>   num-interpolated-steps = <1024>;
>   default-brightness-level = <512>;
> 
> This will create a brightness-level table with the following values:
> 
>   <0 63 126 189 252 315 378 441 ... 64260 64323 64386 64449 65535>
> 
> Another use case can be describe a gamma corrected curve, as we have
> better sensitivity at low luminance than high luminance we probably
> want have smaller steps for low brightness levels values and bigger
> steps for high brightness levels values. This can be achieved with
> the following in the dts:
> 
>   brightness-levels = <0 4096 65535>;
>   num-interpolated-steps = <1024>;
>   default-brightness-level = <512>;
> 
> This will create a brightness-levels table with the following values:
> 
>   <0 4 8 12 16 20 ... 4096 4156 4216 4276 ... 65535>
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
>  drivers/video/backlight/pwm_bl.c | 83
> ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c
> b/drivers/video/backlight/pwm_bl.c
> index 8e3f1245f5c5..f0a108ab570a 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -147,7 +147,11 @@ static int pwm_backlight_parse_dt(struct device
> *dev,
>  				  struct platform_pwm_backlight_data
> *data)
>  {
>  	struct device_node *node = dev->of_node;
> +	unsigned int num_levels = 0;
> +	unsigned int levels_count;
> +	unsigned int num_steps;
>  	struct property *prop;
> +	unsigned int *table;
>  	int length;
>  	u32 value;
>  	int ret;
> @@ -167,6 +171,7 @@ static int pwm_backlight_parse_dt(struct device
> *dev,
>  	/* read brightness levels from DT property */
>  	if (data->max_brightness > 0) {
>  		size_t size = sizeof(*data->levels) * data-
> >max_brightness;
> +		unsigned int i, j, n = 0;
>  
>  		data->levels = devm_kzalloc(dev, size, GFP_KERNEL);
>  		if (!data->levels)
> @@ -184,6 +189,84 @@ static int pwm_backlight_parse_dt(struct device
> *dev,
>  			return ret;
>  
>  		data->dft_brightness = value;
> +
> +		/*
> +		 * This property is optional, if is set enables
> linear
> +		 * interpolation between each of the values of
> brightness levels
> +		 * and creates a new pre-computed table.
> +		 */
> +		of_property_read_u32(node, "num-interpolated-steps",
> +				     &num_steps);
> +
> +		/*
> +		 * Make sure that there is at least two entries in
> the
> +		 * brightness-levels table, otherwise we can't
> interpolate
> +		 * between two points.
> +		 */
> +		if (num_steps) {
> +			if (data->max_brightness < 2) {
> +				dev_err(dev, "can't interpolate\n");
> +				return -EINVAL;
> +			}
> +
> +			/*
> +			 * Recalculate the number of brightness
> levels, now
> +			 * taking in consideration the number of
> interpolated
> +			 * steps between two levels.
> +			 */
> +			for (i = 0; i < data->max_brightness - 1;
> i++) {
> +				if ((data->levels[i + 1] - data-
> >levels[i]) /
> +				   num_steps)
> +					num_levels += num_steps;
> +				else
> +					num_levels++;
> +			}
> +			num_levels++;
> +			dev_dbg(dev, "new number of brightness
> levels: %d\n",
> +				num_levels);
> +
> +			/*
> +			 * Create a new table of brightness levels
> with all the
> +			 * interpolated steps.
> +			 */
> +			size = sizeof(*table) * num_levels;
> +			table = devm_kzalloc(dev, size, GFP_KERNEL);
> +			if (!table)
> +				return -ENOMEM;
> +
> +			/* Fill the interpolated table. */
> +			levels_count = 0;
> +			for (i = 0; i < data->max_brightness - 1;
> i++) {
> +				value = data->levels[i];
> +				n = (data->levels[i + 1] - value) /
> num_steps;
> +				if (n > 0) {
> +					for (j = 0; j < num_steps;
> j++) {
> +						table[levels_count]
> = value;
> +						value += n;
> +						levels_count++;
> +					}
> +				} else {
> +					table[levels_count] = data-
> >levels[i];
> +					levels_count++;
> +				}
> +			}
> +			table[levels_count] = data->levels[i];
> +
> +			/*
> +			 * As we use interpolation lets remove
> current
> +			 * brightness levels table and replace for
> the
> +			 * new interpolated table.
> +			 */
> +			devm_kfree(dev, data->levels);
> +			data->levels = table;
> +
> +			/*
> +			 * Reassign max_brightness value to the new
> total number
> +			 * of brightness levels.
> +			 */
> +			data->max_brightness = num_levels;
> +		}
> +
>  		data->max_brightness--;
>  	}

Unfortunately, bisection has shown this to break graphics on at least
Apalis T30  [1] and Colibri T30 [2] running today's (resp. yesterday's)
-next:

[    3.302618] ------------[ cut here ]------------
[    3.302664] WARNING: CPU: 2 PID: 1 at
/run/media/zim/Build/Sources/linux-next.git/mm/slab_common.c:1031
kmalloc_slab+0x98/0xa0
[    3.302701] Modules linked in:
[    3.302732] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 4.18.0-rc4-
next-20180713-dirty #50
[    3.302763] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[    3.302820] [<c011248c>] (unwind_backtrace) from [<c010ce70>]
(show_stack+0x10/0x14)
[    3.302865] [<c010ce70>] (show_stack) from [<c0a00a74>]
(dump_stack+0x8c/0xa0)
[    3.302905] [<c0a00a74>] (dump_stack) from [<c0123cb0>]
(__warn+0xe0/0xf8)
[    3.302937] [<c0123cb0>] (__warn) from [<c0123de0>]
(warn_slowpath_null+0x40/0x48)
[    3.302971] [<c0123de0>] (warn_slowpath_null) from [<c0231b74>]
(kmalloc_slab+0x98/0xa0)
[    3.303014] [<c0231b74>] (kmalloc_slab) from [<c0258ac0>]
(__kmalloc_track_caller+0x18/0x214)   
[    3.303060] [<c0258ac0>] (__kmalloc_track_caller) from [<c0571264>]
(devm_kmalloc+0x24/0x6c)
[    3.303108] [<c0571264>] (devm_kmalloc) from [<c0477c4c>]
(pwm_backlight_probe+0x4b4/0x9d8)
[    3.303147] [<c0477c4c>] (pwm_backlight_probe) from [<c056f8d0>]
(platform_drv_probe+0x48/0x98) 
[    3.303185] [<c056f8d0>] (platform_drv_probe) from [<c056d9e8>]
(really_probe+0x1d0/0x2bc)
[    3.303219] [<c056d9e8>] (really_probe) from [<c056dc38>]
(driver_probe_device+0x60/0x160)
[    3.303252] [<c056dc38>] (driver_probe_device) from [<c056de08>]
(__driver_attach+0xd0/0xd4)
[    3.303297] [<c056de08>] (__driver_attach) from [<c056bd34>]
(bus_for_each_dev+0x74/0xb4)
[    3.303337] [<c056bd34>] (bus_for_each_dev) from [<c056ce94>]
(bus_add_driver+0x18c/0x210)
[    3.303373] [<c056ce94>] (bus_add_driver) from [<c056ea3c>]
(driver_register+0x7c/0x114)
[    3.303412] [<c056ea3c>] (driver_register) from [<c0102f24>]
(do_one_initcall+0x54/0x278)
[    3.303463] [<c0102f24>] (do_one_initcall) from [<c0e01110>]
(kernel_init_freeable+0x2c0/0x354) 
[    3.303512] [<c0e01110>] (kernel_init_freeable) from [<c0a1715c>]
(kernel_init+0x8/0x10c)
[    3.303551] [<c0a1715c>] (kernel_init) from [<c01010e8>]
(ret_from_fork+0x14/0x2c)
[    3.303579] Exception stack(0xf68a7fb0 to 0xf68a7ff8)
[    3.303602] 7fa0:                                     00000000
00000000 00000000 00000000
[    3.303635] 7fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    3.303666] 7fe0: 00000000 00000000 00000000 00000000 00000013
00000000
[    3.303695] ---[ end trace 8ab8d599271271a0 ]---
[    3.303721] pwm-backlight backlight: failed to find platform data
[    3.303765] pwm-backlight: probe of backlight failed with error -12

Just reverting this single patch makes it work fine as expected again.
Further investigation pending but maybe some of you guys know what is
going on here.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
/tree/arch/arm/boot/dts/tegra30-apalis-eval.dts?h=next-20180713
[2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
/tree/arch/arm/boot/dts/tegra30-colibri-eval-v3.dts?h=next-20180713

Re: REGRESSION: [RESEND PATCH v3 1/4] backlight: pwm_bl: linear interpolation between brightness-levels

[Daniel Thompson]

On Sat, Jul 14, 2018 at 03:08:17PM +0000, Marcel Ziswiler wrote:
> On Mon, 2018-04-09 at 10:33 +0200, Enric Balletbo i Serra wrote:
> > diff --git a/drivers/video/backlight/pwm_bl.c
> > b/drivers/video/backlight/pwm_bl.c
> > index 8e3f1245f5c5..f0a108ab570a 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -147,7 +147,11 @@ static int pwm_backlight_parse_dt(struct device
> > *dev,
> >  				  struct platform_pwm_backlight_data
> > *data)
> >  {
> >  	struct device_node *node = dev->of_node;
> > +	unsigned int num_levels = 0;
> > +	unsigned int levels_count;
> > +	unsigned int num_steps;

num_steps is not initialized...


> >  	struct property *prop;
> > +	unsigned int *table;
> >  	int length;
> >  	u32 value;
> >  	int ret;
> > @@ -167,6 +171,7 @@ static int pwm_backlight_parse_dt(struct device
> > *dev,
> >  	/* read brightness levels from DT property */
> >  	if (data->max_brightness > 0) {
> >  		size_t size = sizeof(*data->levels) * data-
> > >max_brightness;
> > +		unsigned int i, j, n = 0;
> >  
> >  		data->levels = devm_kzalloc(dev, size, GFP_KERNEL);
> >  		if (!data->levels)
> > @@ -184,6 +189,84 @@ static int pwm_backlight_parse_dt(struct device
> > *dev,
> >  			return ret;
> >  
> >  		data->dft_brightness = value;
> > +
> > +		/*
> > +		 * This property is optional, if is set enables
> > linear
> > +		 * interpolation between each of the values of
> > brightness levels
> > +		 * and creates a new pre-computed table.
> > +		 */
> > +		of_property_read_u32(node, "num-interpolated-steps",
> > +				     &num_steps);

... this is not guaranteed to initialized num_steps ...

> > +
> > +		/*
> > +		 * Make sure that there is at least two entries in
> > the
> > +		 * brightness-levels table, otherwise we can't
> > interpolate
> > +		 * between two points.
> > +		 */
> > +		if (num_steps) {

... and we make a decision on it here.

Marcel: Can you try the following quick fix? It's untested on my side
        but very simple...

>From 6fa2fbeb017086147ac61981107a95cb8ae7b4e7 Mon Sep 17 00:00:00 2001
From: Daniel Thompson <daniel.thompson@linaro.org>
Date: Sun, 15 Jul 2018 08:49:05 +0100
Subject: [PATCH] backlight: pwm_bl: Fix uninitialized variable

Currently, if the DT does not define num-interpolated-steps then
num_steps is undefined meaning the interpolation code will deploy
randomly. Fix this.

Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation between
brightness-levels")
Reported-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 drivers/video/backlight/pwm_bl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 9ee4c1b735b2..bdfcc0a71db1 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
 	struct device_node *node = dev->of_node;
 	unsigned int num_levels = 0;
 	unsigned int levels_count;
-	unsigned int num_steps;
+	unsigned int num_steps = 0;
 	struct property *prop;
 	unsigned int *table;
 	int length;
-- 
2.17.1

Re: REGRESSION: [RESEND PATCH v3 1/4] backlight: pwm_bl: linear interpolation between brightness-levels

[Marcel Ziswiler]

On Sun, 2018-07-15 at 08:57 +0100, Daniel Thompson wrote:
> On Sat, Jul 14, 2018 at 03:08:17PM +0000, Marcel Ziswiler wrote:
> > On Mon, 2018-04-09 at 10:33 +0200, Enric Balletbo i Serra wrote:
> > > diff --git a/drivers/video/backlight/pwm_bl.c
> > > b/drivers/video/backlight/pwm_bl.c
> > > index 8e3f1245f5c5..f0a108ab570a 100644
> > > --- a/drivers/video/backlight/pwm_bl.c
> > > +++ b/drivers/video/backlight/pwm_bl.c
> > > @@ -147,7 +147,11 @@ static int pwm_backlight_parse_dt(struct
> > > device
> > > *dev,
> > >  				  struct
> > > platform_pwm_backlight_data
> > > *data)
> > >  {
> > >  	struct device_node *node = dev->of_node;
> > > +	unsigned int num_levels = 0;
> > > +	unsigned int levels_count;
> > > +	unsigned int num_steps;
> 
> num_steps is not initialized...
> 
> 
> > >  	struct property *prop;
> > > +	unsigned int *table;
> > >  	int length;
> > >  	u32 value;
> > >  	int ret;
> > > @@ -167,6 +171,7 @@ static int pwm_backlight_parse_dt(struct
> > > device
> > > *dev,
> > >  	/* read brightness levels from DT property */
> > >  	if (data->max_brightness > 0) {
> > >  		size_t size = sizeof(*data->levels) * data-
> > > > max_brightness;
> > > 
> > > +		unsigned int i, j, n = 0;
> > >  
> > >  		data->levels = devm_kzalloc(dev, size,
> > > GFP_KERNEL);
> > >  		if (!data->levels)
> > > @@ -184,6 +189,84 @@ static int pwm_backlight_parse_dt(struct
> > > device
> > > *dev,
> > >  			return ret;
> > >  
> > >  		data->dft_brightness = value;
> > > +
> > > +		/*
> > > +		 * This property is optional, if is set enables
> > > linear
> > > +		 * interpolation between each of the values of
> > > brightness levels
> > > +		 * and creates a new pre-computed table.
> > > +		 */
> > > +		of_property_read_u32(node, "num-interpolated-
> > > steps",
> > > +				     &num_steps);
> 
> ... this is not guaranteed to initialized num_steps ...

Yes, as it only does so if returning zero. I do further propose to
check its return value as well. Isn't that what return values are used
for?

Quoting from include/linux/of.h:

Search for a property in a device node and read 32-bit value(s) from
it. Returns 0 on success, -EINVAL if the property does not exist,
-ENODATA if property does not have a value, and -EOVERFLOW if the
property data isn't large enough.

> > > +
> > > +		/*
> > > +		 * Make sure that there is at least two entries
> > > in
> > > the
> > > +		 * brightness-levels table, otherwise we can't
> > > interpolate
> > > +		 * between two points.
> > > +		 */
> > > +		if (num_steps) {
> 
> ... and we make a decision on it here.
> 
> Marcel: Can you try the following quick fix? It's untested on my side
>         but very simple...
> 
> From 6fa2fbeb017086147ac61981107a95cb8ae7b4e7 Mon Sep 17 00:00:00
> 2001
> From: Daniel Thompson <daniel.thompson@linaro.org>
> Date: Sun, 15 Jul 2018 08:49:05 +0100
> Subject: [PATCH] backlight: pwm_bl: Fix uninitialized variable
> 
> Currently, if the DT does not define num-interpolated-steps then
> num_steps is undefined meaning the interpolation code will deploy
> randomly. Fix this.
> 
> Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation between
> brightness-levels")
> Reported-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
>  drivers/video/backlight/pwm_bl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c
> b/drivers/video/backlight/pwm_bl.c
> index 9ee4c1b735b2..bdfcc0a71db1 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct device
> *dev,
>  	struct device_node *node = dev->of_node;
>  	unsigned int num_levels = 0;
>  	unsigned int levels_count;
> -	unsigned int num_steps;
> +	unsigned int num_steps = 0;
>  	struct property *prop;
>  	unsigned int *table;
>  	int length;
> --
> 2.17.1

From dbb31d00c9f2873affedbceae917c9d7fce5f832 Mon Sep 17 00:00:00 2001
Message-Id: <dbb31d00c9f2873affedbceae917c9d7fce5f832.1531664663.git.ma
rcel.ziswiler@toradex.com>
From: Daniel Thompson <daniel.thompson@linaro.org>
Date: Sun, 15 Jul 2018 08:49:05 +0100
Subject: [PATCH] backlight: pwm_bl: Fix uninitialized variable

Currently, if the DT does not define num-interpolated-steps then
num_steps is undefined meaning the interpolation code will deploy
randomly. Fix this.

Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation between
brightness-levels")
Reported-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
---
 drivers/video/backlight/pwm_bl.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c
b/drivers/video/backlight/pwm_bl.c
index 9ee4c1b735b2..e884d589378d 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct device
*dev,
 	struct device_node *node = dev->of_node;
 	unsigned int num_levels = 0;
 	unsigned int levels_count;
-	unsigned int num_steps;
+	unsigned int num_steps = 0;
 	struct property *prop;
 	unsigned int *table;
 	int length;
@@ -299,15 +299,13 @@ static int pwm_backlight_parse_dt(struct device
*dev,
 		 * interpolation between each of the values of
brightness levels
 		 * and creates a new pre-computed table.
 		 */
-		of_property_read_u32(node, "num-interpolated-steps",
-				     &num_steps);
-
-		/*
-		 * Make sure that there is at least two entries in the
-		 * brightness-levels table, otherwise we can't
interpolate
-		 * between two points.
-		 */
-		if (num_steps) {
+		if ((of_property_read_u32(node, "num-interpolated-
steps",
+					  &num_steps) == 0) &&
(num_steps)) {
+			/*
+			 * Make sure that there is at least two
entries in the
+			 * brightness-levels table, otherwise we can't
interpolate
+			 * between two points.
+			 */
 			if (data->max_brightness < 2) {
 				dev_err(dev, "can't interpolate\n");
 				return -EINVAL;
-- 
2.14.4

Re: REGRESSION: [RESEND PATCH v3 1/4] backlight: pwm_bl: linear interpolation between brightness-levels

[Daniel Thompson]

On Sun, Jul 15, 2018 at 02:26:44PM +0000, Marcel Ziswiler wrote:
> On Sun, 2018-07-15 at 08:57 +0100, Daniel Thompson wrote:
> > On Sat, Jul 14, 2018 at 03:08:17PM +0000, Marcel Ziswiler wrote:
> > > On Mon, 2018-04-09 at 10:33 +0200, Enric Balletbo i Serra wrote:
> > > > diff --git a/drivers/video/backlight/pwm_bl.c
> > > > b/drivers/video/backlight/pwm_bl.c
> > > > index 8e3f1245f5c5..f0a108ab570a 100644
> > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > @@ -147,7 +147,11 @@ static int pwm_backlight_parse_dt(struct
> > > > device
> > > > *dev,
> > > >  				  struct
> > > > platform_pwm_backlight_data
> > > > *data)
> > > >  {
> > > >  	struct device_node *node = dev->of_node;
> > > > +	unsigned int num_levels = 0;
> > > > +	unsigned int levels_count;
> > > > +	unsigned int num_steps;
> > 
> > num_steps is not initialized...
> > 
> > 
> > > >  	struct property *prop;
> > > > +	unsigned int *table;
> > > >  	int length;
> > > >  	u32 value;
> > > >  	int ret;
> > > > @@ -167,6 +171,7 @@ static int pwm_backlight_parse_dt(struct
> > > > device
> > > > *dev,
> > > >  	/* read brightness levels from DT property */
> > > >  	if (data->max_brightness > 0) {
> > > >  		size_t size = sizeof(*data->levels) * data-
> > > > > max_brightness;
> > > > 
> > > > +		unsigned int i, j, n = 0;
> > > >  
> > > >  		data->levels = devm_kzalloc(dev, size,
> > > > GFP_KERNEL);
> > > >  		if (!data->levels)
> > > > @@ -184,6 +189,84 @@ static int pwm_backlight_parse_dt(struct
> > > > device
> > > > *dev,
> > > >  			return ret;
> > > >  
> > > >  		data->dft_brightness = value;
> > > > +
> > > > +		/*
> > > > +		 * This property is optional, if is set enables
> > > > linear
> > > > +		 * interpolation between each of the values of
> > > > brightness levels
> > > > +		 * and creates a new pre-computed table.
> > > > +		 */
> > > > +		of_property_read_u32(node, "num-interpolated-
> > > > steps",
> > > > +				     &num_steps);
> > 
> > ... this is not guaranteed to initialized num_steps ...
> 
> Yes, as it only does so if returning zero. I do further propose to
> check its return value as well. Isn't that what return values are used
> for?

I don't much mind either way.

I originally wrote the patch you shared below but then decided to ask you
to test and, since I didn't compile test before doing so, I opted for
something more immune to silly mistakes.


> Quoting from include/linux/of.h:
> 
> Search for a property in a device node and read 32-bit value(s) from
> it. Returns 0 on success, -EINVAL if the property does not exist,
> -ENODATA if property does not have a value, and -EOVERFLOW if the
> property data isn't large enough.
> 
> > > > +
> > > > +		/*
> > > > +		 * Make sure that there is at least two entries
> > > > in
> > > > the
> > > > +		 * brightness-levels table, otherwise we can't
> > > > interpolate
> > > > +		 * between two points.
> > > > +		 */
> > > > +		if (num_steps) {
> > 
> > ... and we make a decision on it here.
> > 
> > Marcel: Can you try the following quick fix? It's untested on my side
> >         but very simple...
> > 
> > From 6fa2fbeb017086147ac61981107a95cb8ae7b4e7 Mon Sep 17 00:00:00
> > 2001
> > From: Daniel Thompson <daniel.thompson@linaro.org>
> > Date: Sun, 15 Jul 2018 08:49:05 +0100
> > Subject: [PATCH] backlight: pwm_bl: Fix uninitialized variable
> > 
> > Currently, if the DT does not define num-interpolated-steps then
> > num_steps is undefined meaning the interpolation code will deploy
> > randomly. Fix this.
> > 
> > Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation between
> > brightness-levels")
> > Reported-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > ---
> >  drivers/video/backlight/pwm_bl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/video/backlight/pwm_bl.c
> > b/drivers/video/backlight/pwm_bl.c
> > index 9ee4c1b735b2..bdfcc0a71db1 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct device
> > *dev,
> >  	struct device_node *node = dev->of_node;
> >  	unsigned int num_levels = 0;
> >  	unsigned int levels_count;
> > -	unsigned int num_steps;
> > +	unsigned int num_steps = 0;
> >  	struct property *prop;
> >  	unsigned int *table;
> >  	int length;
> > --
> > 2.17.1
> 
> From dbb31d00c9f2873affedbceae917c9d7fce5f832 Mon Sep 17 00:00:00 2001
> Message-Id: <dbb31d00c9f2873affedbceae917c9d7fce5f832.1531664663.git.ma
> rcel.ziswiler@toradex.com>
> From: Daniel Thompson <daniel.thompson@linaro.org>
> Date: Sun, 15 Jul 2018 08:49:05 +0100
> Subject: [PATCH] backlight: pwm_bl: Fix uninitialized variable
> 
> Currently, if the DT does not define num-interpolated-steps then
> num_steps is undefined meaning the interpolation code will deploy
> randomly. Fix this.
> 
> Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation between
> brightness-levels")
> Reported-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

Is it Tested-by: too? It would be good to confirm I was right about the
cause of the problem.


> ---
>  drivers/video/backlight/pwm_bl.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c
> b/drivers/video/backlight/pwm_bl.c
> index 9ee4c1b735b2..e884d589378d 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct device
> *dev,
>  	struct device_node *node = dev->of_node;
>  	unsigned int num_levels = 0;
>  	unsigned int levels_count;
> -	unsigned int num_steps;
> +	unsigned int num_steps = 0;

This can go. If we check the return code them this variable is no longer
used uninitialized [I'm OK to make the change though... since you've
kept my name at the top ;-) ].


Daniel.

>  	struct property *prop;
>  	unsigned int *table;
>  	int length;
> @@ -299,15 +299,13 @@ static int pwm_backlight_parse_dt(struct device
> *dev,
>  		 * interpolation between each of the values of
> brightness levels
>  		 * and creates a new pre-computed table.
>  		 */
> -		of_property_read_u32(node, "num-interpolated-steps",
> -				     &num_steps);
> -
> -		/*
> -		 * Make sure that there is at least two entries in the
> -		 * brightness-levels table, otherwise we can't
> interpolate
> -		 * between two points.
> -		 */
> -		if (num_steps) {
> +		if ((of_property_read_u32(node, "num-interpolated-
> steps",
> +					  &num_steps) == 0) &&
> (num_steps)) {
> +			/*
> +			 * Make sure that there is at least two
> entries in the
> +			 * brightness-levels table, otherwise we can't
> interpolate
> +			 * between two points.
> +			 */
>  			if (data->max_brightness < 2) {
>  				dev_err(dev, "can't interpolate\n");
>  				return -EINVAL;
> -- 
> 2.14.4

Re: REGRESSION: [RESEND PATCH v3 1/4] backlight: pwm_bl: linear interpolation between brightness-levels

[Marcel Ziswiler]

On Mon, 2018-07-16 at 10:42 +0100, Daniel Thompson wrote:
> On Sun, Jul 15, 2018 at 02:26:44PM +0000, Marcel Ziswiler wrote:
> > On Sun, 2018-07-15 at 08:57 +0100, Daniel Thompson wrote:
> > > On Sat, Jul 14, 2018 at 03:08:17PM +0000, Marcel Ziswiler wrote:
> > > > On Mon, 2018-04-09 at 10:33 +0200, Enric Balletbo i Serra
> > > > wrote:
> > > > > diff --git a/drivers/video/backlight/pwm_bl.c
> > > > > b/drivers/video/backlight/pwm_bl.c
> > > > > index 8e3f1245f5c5..f0a108ab570a 100644
> > > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > > @@ -147,7 +147,11 @@ static int pwm_backlight_parse_dt(struct
> > > > > device
> > > > > *dev,
> > > > >  				  struct
> > > > > platform_pwm_backlight_data
> > > > > *data)
> > > > >  {
> > > > >  	struct device_node *node = dev->of_node;
> > > > > +	unsigned int num_levels = 0;
> > > > > +	unsigned int levels_count;
> > > > > +	unsigned int num_steps;
> > > 
> > > num_steps is not initialized...
> > > 
> > > 
> > > > >  	struct property *prop;
> > > > > +	unsigned int *table;
> > > > >  	int length;
> > > > >  	u32 value;
> > > > >  	int ret;
> > > > > @@ -167,6 +171,7 @@ static int pwm_backlight_parse_dt(struct
> > > > > device
> > > > > *dev,
> > > > >  	/* read brightness levels from DT property */
> > > > >  	if (data->max_brightness > 0) {
> > > > >  		size_t size = sizeof(*data->levels) * data-
> > > > > > max_brightness;
> > > > > 
> > > > > +		unsigned int i, j, n = 0;
> > > > >  
> > > > >  		data->levels = devm_kzalloc(dev, size,
> > > > > GFP_KERNEL);
> > > > >  		if (!data->levels)
> > > > > @@ -184,6 +189,84 @@ static int pwm_backlight_parse_dt(struct
> > > > > device
> > > > > *dev,
> > > > >  			return ret;
> > > > >  
> > > > >  		data->dft_brightness = value;
> > > > > +
> > > > > +		/*
> > > > > +		 * This property is optional, if is set
> > > > > enables
> > > > > linear
> > > > > +		 * interpolation between each of the values
> > > > > of
> > > > > brightness levels
> > > > > +		 * and creates a new pre-computed table.
> > > > > +		 */
> > > > > +		of_property_read_u32(node, "num-
> > > > > interpolated-
> > > > > steps",
> > > > > +				     &num_steps);
> > > 
> > > ... this is not guaranteed to initialized num_steps ...
> > 
> > Yes, as it only does so if returning zero. I do further propose to
> > check its return value as well. Isn't that what return values are
> > used
> > for?
> 
> I don't much mind either way.
> 
> I originally wrote the patch you shared below but then decided to ask
> you
> to test and, since I didn't compile test before doing so, I opted for
> something more immune to silly mistakes.
> 
> 
> > Quoting from include/linux/of.h:
> > 
> > Search for a property in a device node and read 32-bit value(s)
> > from
> > it. Returns 0 on success, -EINVAL if the property does not exist,
> > -ENODATA if property does not have a value, and -EOVERFLOW if the
> > property data isn't large enough.
> > 
> > > > > +
> > > > > +		/*
> > > > > +		 * Make sure that there is at least two
> > > > > entries
> > > > > in
> > > > > the
> > > > > +		 * brightness-levels table, otherwise we
> > > > > can't
> > > > > interpolate
> > > > > +		 * between two points.
> > > > > +		 */
> > > > > +		if (num_steps) {
> > > 
> > > ... and we make a decision on it here.
> > > 
> > > Marcel: Can you try the following quick fix? It's untested on my
> > > side
> > >         but very simple...
> > > 
> > > From 6fa2fbeb017086147ac61981107a95cb8ae7b4e7 Mon Sep 17 00:00:00
> > > 2001
> > > From: Daniel Thompson <daniel.thompson@linaro.org>
> > > Date: Sun, 15 Jul 2018 08:49:05 +0100
> > > Subject: [PATCH] backlight: pwm_bl: Fix uninitialized variable
> > > 
> > > Currently, if the DT does not define num-interpolated-steps then
> > > num_steps is undefined meaning the interpolation code will deploy
> > > randomly. Fix this.
> > > 
> > > Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation
> > > between
> > > brightness-levels")
> > > Reported-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > > ---
> > >  drivers/video/backlight/pwm_bl.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/video/backlight/pwm_bl.c
> > > b/drivers/video/backlight/pwm_bl.c
> > > index 9ee4c1b735b2..bdfcc0a71db1 100644
> > > --- a/drivers/video/backlight/pwm_bl.c
> > > +++ b/drivers/video/backlight/pwm_bl.c
> > > @@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct
> > > device
> > > *dev,
> > >  	struct device_node *node = dev->of_node;
> > >  	unsigned int num_levels = 0;
> > >  	unsigned int levels_count;
> > > -	unsigned int num_steps;
> > > +	unsigned int num_steps = 0;
> > >  	struct property *prop;
> > >  	unsigned int *table;
> > >  	int length;
> > > --
> > > 2.17.1
> > 
> > From dbb31d00c9f2873affedbceae917c9d7fce5f832 Mon Sep 17 00:00:00
> > 2001
> > Message-Id:
> > <dbb31d00c9f2873affedbceae917c9d7fce5f832.1531664663.git.ma
> > rcel.ziswiler@toradex.com>
> > From: Daniel Thompson <daniel.thompson@linaro.org>
> > Date: Sun, 15 Jul 2018 08:49:05 +0100
> > Subject: [PATCH] backlight: pwm_bl: Fix uninitialized variable
> > 
> > Currently, if the DT does not define num-interpolated-steps then
> > num_steps is undefined meaning the interpolation code will deploy
> > randomly. Fix this.
> > 
> > Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation
> > between
> > brightness-levels")
> > Reported-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> Is it Tested-by: too? It would be good to confirm I was right about
> the
> cause of the problem.

Yes and I confirm you were right.

> > ---
> >  drivers/video/backlight/pwm_bl.c | 18 ++++++++----------
> >  1 file changed, 8 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/video/backlight/pwm_bl.c
> > b/drivers/video/backlight/pwm_bl.c
> > index 9ee4c1b735b2..e884d589378d 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct device
> > *dev,
> >  	struct device_node *node = dev->of_node;
> >  	unsigned int num_levels = 0;
> >  	unsigned int levels_count;
> > -	unsigned int num_steps;
> > +	unsigned int num_steps = 0;
> 
> This can go. If we check the return code them this variable is no
> longer
> used uninitialized [I'm OK to make the change though... since you've
> kept my name at the top ;-) ].

Yes, I confirm that this works for me. Are you gona send a proper patch
out or should I do it?

> Daniel.
> 
> >  	struct property *prop;
> >  	unsigned int *table;
> >  	int length;
> > @@ -299,15 +299,13 @@ static int pwm_backlight_parse_dt(struct
> > device
> > *dev,
> >  		 * interpolation between each of the values of
> > brightness levels
> >  		 * and creates a new pre-computed table.
> >  		 */
> > -		of_property_read_u32(node, "num-interpolated-
> > steps",
> > -				     &num_steps);
> > -
> > -		/*
> > -		 * Make sure that there is at least two entries in
> > the
> > -		 * brightness-levels table, otherwise we can't
> > interpolate
> > -		 * between two points.
> > -		 */
> > -		if (num_steps) {
> > +		if ((of_property_read_u32(node, "num-interpolated-
> > steps",
> > +					  &num_steps) == 0) &&
> > (num_steps)) {
> > +			/*
> > +			 * Make sure that there is at least two
> > entries in the
> > +			 * brightness-levels table, otherwise we
> > can't
> > interpolate
> > +			 * between two points.
> > +			 */
> >  			if (data->max_brightness < 2) {
> >  				dev_err(dev, "can't
> > interpolate\n");
> >  				return -EINVAL;
> > -- 
> > 2.14.4

Re: REGRESSION: [RESEND PATCH v3 1/4] backlight: pwm_bl: linear interpolation between brightness-levels

[Daniel Thompson]

On Mon, Jul 16, 2018 at 11:57:29AM +0000, Marcel Ziswiler wrote:
> On Mon, 2018-07-16 at 10:42 +0100, Daniel Thompson wrote:
> > > 
> > > From dbb31d00c9f2873affedbceae917c9d7fce5f832 Mon Sep 17 00:00:00
> > > 2001
> > > Message-Id:
> > > <dbb31d00c9f2873affedbceae917c9d7fce5f832.1531664663.git.ma
> > > rcel.ziswiler@toradex.com>
> > > From: Daniel Thompson <daniel.thompson@linaro.org>
> > > Date: Sun, 15 Jul 2018 08:49:05 +0100
> > > Subject: [PATCH] backlight: pwm_bl: Fix uninitialized variable
> > > 
> > > Currently, if the DT does not define num-interpolated-steps then
> > > num_steps is undefined meaning the interpolation code will deploy
> > > randomly. Fix this.
> > > 
> > > Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation
> > > between
> > > brightness-levels")
> > > Reported-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > 
> > Is it Tested-by: too? It would be good to confirm I was right about
> > the
> > cause of the problem.
> 
> Yes and I confirm you were right.

Thanks.


> > >  drivers/video/backlight/pwm_bl.c | 18 ++++++++----------
> > >  1 file changed, 8 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/video/backlight/pwm_bl.c
> > > b/drivers/video/backlight/pwm_bl.c
> > > index 9ee4c1b735b2..e884d589378d 100644
> > > --- a/drivers/video/backlight/pwm_bl.c
> > > +++ b/drivers/video/backlight/pwm_bl.c
> > > @@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct device
> > > *dev,
> > >  	struct device_node *node = dev->of_node;
> > >  	unsigned int num_levels = 0;
> > >  	unsigned int levels_count;
> > > -	unsigned int num_steps;
> > > +	unsigned int num_steps = 0;
> > 
> > This can go. If we check the return code them this variable is no
> > longer
> > used uninitialized [I'm OK to make the change though... since you've
> > kept my name at the top ;-) ].
> 
> Yes, I confirm that this works for me. Are you gona send a proper patch
> out or should I do it?

I can send it out.


Daniel.

Re: REGRESSION: [RESEND PATCH v3 1/4] backlight: pwm_bl: linear interpolation between brightness-levels

[Marcel Ziswiler]

On Mon, 2018-07-16 at 14:51 +0100, Daniel Thompson wrote:
> On Mon, Jul 16, 2018 at 11:57:29AM +0000, Marcel Ziswiler wrote:
> > On Mon, 2018-07-16 at 10:42 +0100, Daniel Thompson wrote:
> > > > 
> > > > From dbb31d00c9f2873affedbceae917c9d7fce5f832 Mon Sep 17
> > > > 00:00:00
> > > > 2001
> > > > Message-Id:
> > > > <dbb31d00c9f2873affedbceae917c9d7fce5f832.1531664663.git.ma
> > > > rcel.ziswiler@toradex.com>
> > > > From: Daniel Thompson <daniel.thompson@linaro.org>
> > > > Date: Sun, 15 Jul 2018 08:49:05 +0100
> > > > Subject: [PATCH] backlight: pwm_bl: Fix uninitialized variable
> > > > 
> > > > Currently, if the DT does not define num-interpolated-steps
> > > > then
> > > > num_steps is undefined meaning the interpolation code will
> > > > deploy
> > > > randomly. Fix this.
> > > > 
> > > > Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation
> > > > between
> > > > brightness-levels")
> > > > Reported-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > 
> > > Is it Tested-by: too? It would be good to confirm I was right
> > > about
> > > the
> > > cause of the problem.
> > 
> > Yes and I confirm you were right.
> 
> Thanks.
> 
> 
> > > >  drivers/video/backlight/pwm_bl.c | 18 ++++++++----------
> > > >  1 file changed, 8 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/video/backlight/pwm_bl.c
> > > > b/drivers/video/backlight/pwm_bl.c
> > > > index 9ee4c1b735b2..e884d589378d 100644
> > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > @@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct
> > > > device
> > > > *dev,
> > > >  	struct device_node *node = dev->of_node;
> > > >  	unsigned int num_levels = 0;
> > > >  	unsigned int levels_count;
> > > > -	unsigned int num_steps;
> > > > +	unsigned int num_steps = 0;
> > > 
> > > This can go. If we check the return code them this variable is no
> > > longer
> > > used uninitialized [I'm OK to make the change though... since
> > > you've
> > > kept my name at the top ;-) ].
> > 
> > Yes, I confirm that this works for me. Are you gona send a proper
> > patch
> > out or should I do it?
> 
> I can send it out.
> 
> 
> Daniel.

Thank you.

Cheers

Marcel

[RESEND PATCH v3 2/4] dt-bindings: pwm-backlight: add a num-interpolation-steps property.

[Enric Balletbo i Serra]

The num-interpolated-steps property specifies the number of
interpolated steps between each value of brightness-level table. This is
useful for high resolution PWMs to not have to list out every possible
value in the brightness-level array.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/leds/backlight/pwm-backlight.txt         | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
index 310810906613..ce9b5746b774 100644
--- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
@@ -21,6 +21,11 @@ Optional properties:
                           and enabling the backlight using GPIO.
   - pwm-off-delay-ms: Delay in ms between disabling the backlight using GPIO
                       and setting PWM value to 0.
+  - num-interpolated-steps: Number of interpolated steps between each value
+                            of brightness-levels table. This way a high
+                            resolution pwm duty cycle can be used without
+                            having to list out every possible value in the
+                            brightness-level array.
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 [1]: Documentation/devicetree/bindings/gpio/gpio.txt
@@ -39,3 +44,17 @@ Example:
 		post-pwm-on-delay-ms = <10>;
 		pwm-off-delay-ms = <10>;
 	};
+
+Example using num-interpolation-steps:
+
+	backlight {
+		compatible = "pwm-backlight";
+		pwms = <&pwm 0 5000000>;
+
+		brightness-levels = <0 2048 4096 8192 16384 65535>;
+		num-interpolated-steps = <2048>;
+		default-brightness-level = <4096>;
+
+		power-supply = <&vdd_bl_reg>;
+		enable-gpios = <&gpio 58 0>;
+	};
-- 
2.16.3

[RESEND PATCH v3 3/4] backlight: pwm_bl: compute brightness of LED linearly to human eye.

[Enric Balletbo i Serra]

When you want to change the brightness using a PWM signal, one thing you
need to consider is how human perceive the brightness. Human perceive
the brightness change non-linearly, we have better sensitivity at low
luminance than high luminance, so to achieve perceived linear dimming,
the brightness must be matches to the way our eyes behave. The CIE 1931
lightness formula is what actually describes how we perceive light.

This patch computes a default table with the brightness levels filled
with the numbers provided by the CIE 1931 algorithm, the number of the
brightness levels is calculated based on the PWM resolution.

The calculation of the table using the CIE 1931 algorithm is enabled by
default when you do not define the 'brightness-levels' propriety in your
device tree.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 drivers/video/backlight/pwm_bl.c | 149 +++++++++++++++++++++++++++++++++++----
 1 file changed, 136 insertions(+), 13 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index f0a108ab570a..d047d875f251 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -143,6 +143,107 @@ static const struct backlight_ops pwm_backlight_ops = {
 };
 
 #ifdef CONFIG_OF
+#define PWM_LUMINANCE_SCALE	10000 /* luminance scale */
+
+/* An integer based power function */
+static u64 int_pow(u64 base, int exp)
+{
+	u64 result = 1;
+
+	while (exp) {
+		if (exp & 1)
+			result *= base;
+		exp >>= 1;
+		base *= base;
+	}
+
+	return result;
+}
+
+/*
+ * CIE lightness to PWM conversion.
+ *
+ * The CIE 1931 lightness formula is what actually describes how we perceive
+ * light:
+ *          Y = (L* / 902.3)           if L* ≤ 0.08856
+ *          Y = ((L* + 16) / 116)^3    if L* > 0.08856
+ *
+ * Where Y is the luminance, the amount of light coming out of the screen, and
+ * is a number between 0.0 and 1.0; and L* is the lightness, how bright a human
+ * perceives the screen to be, and is a number between 0 and 100.
+ *
+ * The following function does the fixed point maths needed to implement the
+ * above formula.
+ */
+static u64 cie1931(unsigned int lightness, unsigned int scale)
+{
+	u64 retval;
+
+	lightness *= 100;
+	if (lightness <= (8 * scale)) {
+		retval = DIV_ROUND_CLOSEST_ULL(lightness * 10, 9023);
+	} else {
+		retval = int_pow((lightness + (16 * scale)) / 116, 3);
+		retval = DIV_ROUND_CLOSEST_ULL(retval, (scale * scale));
+	}
+
+	return retval;
+}
+
+/*
+ * Create a default correction table for PWM values to create linear brightness
+ * for LED based backlights using the CIE1931 algorithm.
+ */
+static
+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;
+	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,
+				    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++) {
+		retval = cie1931((i * PWM_LUMINANCE_SCALE) /
+				 data->max_brightness, PWM_LUMINANCE_SCALE) *
+				 period;
+		retval = DIV_ROUND_CLOSEST_ULL(retval, PWM_LUMINANCE_SCALE);
+		if (retval > UINT_MAX)
+			return -EINVAL;
+		data->levels[i] = (unsigned int)retval;
+	}
+
+	data->dft_brightness = data->max_brightness / 2;
+	data->max_brightness--;
+
+	return 0;
+}
+
 static int pwm_backlight_parse_dt(struct device *dev,
 				  struct platform_pwm_backlight_data *data)
 {
@@ -161,10 +262,13 @@ static int pwm_backlight_parse_dt(struct device *dev,
 
 	memset(data, 0, sizeof(*data));
 
-	/* determine the number of brightness levels */
+	/*
+	 * Determine the number of brightness levels, if this property is not
+	 * set a default table of brightness levels will be used.
+	 */
 	prop = of_find_property(node, "brightness-levels", &length);
 	if (!prop)
-		return -EINVAL;
+		return 0;
 
 	data->max_brightness = length / sizeof(u32);
 
@@ -294,6 +398,14 @@ static int pwm_backlight_parse_dt(struct device *dev,
 {
 	return -ENODEV;
 }
+
+static
+int pwm_backlight_brightness_default(struct device *dev,
+				     struct platform_pwm_backlight_data *data,
+				     unsigned int period)
+{
+	return -ENODEV;
+}
 #endif
 
 static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb)
@@ -334,7 +446,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	struct backlight_device *bl;
 	struct device_node *node = pdev->dev.of_node;
 	struct pwm_bl_data *pb;
+	struct pwm_state state;
 	struct pwm_args pargs;
+	unsigned int i;
 	int ret;
 
 	if (!data) {
@@ -359,17 +473,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		goto err_alloc;
 	}
 
-	if (data->levels) {
-		unsigned int i;
-
-		for (i = 0; i <= data->max_brightness; i++)
-			if (data->levels[i] > pb->scale)
-				pb->scale = data->levels[i];
-
-		pb->levels = data->levels;
-	} else
-		pb->scale = data->max_brightness;
-
 	pb->notify = data->notify;
 	pb->notify_after = data->notify_after;
 	pb->check_fb = data->check_fb;
@@ -436,6 +539,26 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 
 	dev_dbg(&pdev->dev, "got pwm for backlight\n");
 
+	if (!data->levels) {
+		/* Get the PWM period (in nanoseconds) */
+		pwm_get_state(pb->pwm, &state);
+
+		ret = pwm_backlight_brightness_default(&pdev->dev, data,
+						       state.period);
+		if (ret < 0) {
+			dev_err(&pdev->dev,
+				"failed to setup default brightness table\n");
+			goto err_alloc;
+		}
+	}
+
+	for (i = 0; i <= data->max_brightness; i++) {
+		if (data->levels[i] > pb->scale)
+			pb->scale = data->levels[i];
+
+		pb->levels = data->levels;
+	}
+
 	/*
 	 * FIXME: pwm_apply_args() should be removed when switching to
 	 * the atomic PWM API.
-- 
2.16.3

[RESEND PATCH v3 4/4] dt-bindings: pwm-backlight: move brightness-levels to optional.

[Enric Balletbo i Serra]

The patch 'backlight: pwm_bl: compute brightness of LED linearly to
human eye' introduced a default brightness-levels table that is used
when brightness-levels is not available in the dts. So move
brightness-levels and default-brightness-level to be optional.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 .../devicetree/bindings/leds/backlight/pwm-backlight.txt  | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
index ce9b5746b774..64fa2fbd98c9 100644
--- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
@@ -3,13 +3,6 @@ pwm-backlight bindings
 Required properties:
   - compatible: "pwm-backlight"
   - pwms: OF device-tree PWM specification (see PWM binding[0])
-  - brightness-levels: Array of distinct brightness levels. Typically these
-      are in the range from 0 to 255, but any range starting at 0 will do.
-      The actual brightness level (PWM duty cycle) will be interpolated
-      from these values. 0 means a 0% duty cycle (darkest/off), while the
-      last value in the array represents a 100% duty cycle (brightest).
-  - default-brightness-level: the default brightness level (index into the
-      array defined by the "brightness-levels" property)
   - power-supply: regulator for supply voltage
 
 Optional properties:
@@ -21,6 +14,14 @@ Optional properties:
                           and enabling the backlight using GPIO.
   - pwm-off-delay-ms: Delay in ms between disabling the backlight using GPIO
                       and setting PWM value to 0.
+  - brightness-levels: Array of distinct brightness levels. Typically these
+                       are in the range from 0 to 255, but any range starting at
+                       0 will do. The actual brightness level (PWM duty cycle)
+                       will be interpolated from these values. 0 means a 0% duty
+                       cycle (darkest/off), while the last value in the array
+                       represents a 100% duty cycle (brightest).
+  - default-brightness-level: The default brightness level (index into the
+                              array defined by the "brightness-levels" property).
   - num-interpolated-steps: Number of interpolated steps between each value
                             of brightness-levels table. This way a high
                             resolution pwm duty cycle can be used without
-- 
2.16.3

Re: [RESEND PATCH v3 0/4] backlight: pwm_bl: support linear interpolation and brightness to human eye

[Lee Jones]

On Mon, 09 Apr 2018, Enric Balletbo i Serra wrote:

> Dear all,
> 
> This series is a third patchset (resend )integrating the requested
> changes.
> 
> The first and second patch what tries to solve is the problem of
> granularity for high resolution PWMs. The idea is simple interpolate
> between 2 brightness values so we can have a high PWM duty cycle (a
> 16 bits PWM is up to 65535 possible steps) without having to list
> out every possible value in the dts. I think that this patch is
> required to not break backward compatibility, to be more flexible and
> also extend the functionality to be able to use high resolution PWM
> with enough steps to have a good UI experience in userspace.
> 
> The thirth and fourth patch is a bit more ambicious, the idea is let
> decide the driver the brightness-levels required in function of the PWM
> resolution. To do this create a brightness-levels table filled with the
> CIE 1931 algorithm values to convert brightness to PWM duty cycle.
> 
> More detailed info is available in the commit message of every patch.
> 
> Both functionalities were tested on a Samsung Chromebook Plus (that has
> a 16 bits PWM) and a SL50 device (with a 8 bits PWM)
> 
> Waiting for your feedback.
> 
> Best regards
> 
> Enric Balletbo i Serra (4):
>   backlight: pwm_bl: linear interpolation between brightness-levels
>   dt-bindings: pwm-backlight: add a num-interpolation-steps property.
>   backlight: pwm_bl: compute brightness of LED linearly to human eye.
>   dt-bindings: pwm-backlight: move brightness-levels to optional.
> 
>  .../bindings/leds/backlight/pwm-backlight.txt      |  34 ++-
>  drivers/video/backlight/pwm_bl.c                   | 232 +++++++++++++++++++--
>  2 files changed, 246 insertions(+), 20 deletions(-)

All applied for v4.18, thanks.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog