| Message ID | 20190610233739.29477-1-mka@chromium.org |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/2] dt-bindings: pwm-backlight: Add 'max-brightness' property | expand |
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>
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
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 >
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.
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.
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
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(+)