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

[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

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

[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

[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

[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

[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

[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

[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

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