[PATCH] Added possibility to use pwm_bl.c with percentage instead of levels

[PATCH] Added possibility to use pwm_bl.c with percentage instead of levels

[Johannes Pointner]

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

Hello,

I'm new working on the linux device drivers, so if I made something wrong
please point me into the right direction.

I'd like to use the pwm_bl driver for a sitara based terminal and for this
I would need the possibility to set the backlight within a percentage
range. The following patch should add this possibility to the pwm_bl
driver. The idea is to keep backward compatibility by moving the required
option brightness levels to optional. Therefore if there is no node with
brightness levels the percentage levels are used.

Signed-off-by: Johannes Pointner <johannes.pointner@gmail.com>
---
 .../bindings/video/backlight/
pwm-backlight.txt     | 11 +++---
 drivers/video/backlight/pwm_bl.c                   | 46
++++++++++++----------
 2 files changed, 31 insertions(+), 26 deletions(-)

diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 764db86..ea09669 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -3,16 +3,17 @@ pwm-backlight bindings
 Required properties:
   - compatible: "pwm-backlight"
   - pwms: OF device-tree PWM specification (see PWM binding[0])
+  - default-brightness-level: the default brightness level (in case of
defined levels:
+      index into the array defined by the "brightness-levels" property
otherwise the
+      percentage)
+  - power-supply: regulator for supply voltage
+
+Optional properties:
   - 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:
   - pwm-names: a list of names for the PWM devices specified in the
                "pwms" property (see PWM binding[0])
   - enable-gpios: contains a single GPIO specifier for the GPIO which
enables
diff --git a/drivers/video/backlight/pwm_bl.c
b/drivers/video/backlight/pwm_bl.c
index 38ca88b..f9f1f43 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -24,6 +24,8 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>

+#define MAX_PERCENT_BRIGHTNESS        100    /* max brightness if no
levels are used */
+
 struct pwm_bl_data {
     struct pwm_device    *pwm;
     struct device        *dev;
@@ -151,33 +153,35 @@ static int pwm_backlight_parse_dt(struct device *dev,

     /* determine the number of brightness levels */
     prop = of_find_property(node, "brightness-levels", &length);
-    if (!prop)
-        return -EINVAL;
+    if (prop) {
+        data->max_brightness = length / sizeof(u32);

-    data->max_brightness = length / sizeof(u32);
+        /* read brightness levels from DT property */
+        if (data->max_brightness > 0) {
+            size_t size = sizeof(*data->levels) * data->max_brightness;

-    /* read brightness levels from DT property */
-    if (data->max_brightness > 0) {
-        size_t size = sizeof(*data->levels) * data->max_brightness;
+            data->levels = devm_kzalloc(dev, size, GFP_KERNEL);
+            if (!data->levels)
+                return -ENOMEM;

-        data->levels = devm_kzalloc(dev, size, GFP_KERNEL);
-        if (!data->levels)
-            return -ENOMEM;
+            ret = of_property_read_u32_array(node, "brightness-levels",
+                            data->levels,
+                            data->max_brightness);
+            if (ret < 0)
+                return ret;

-        ret = of_property_read_u32_array(node, "brightness-levels",
-                         data->levels,
-                         data->max_brightness);
-        if (ret < 0)
-            return ret;
+            data->max_brightness--;
+        }
+    } else {
+        data->max_brightness = MAX_PERCENT_BRIGHTNESS;
+    }

-        ret = of_property_read_u32(node, "default-brightness-level",
-                       &value);
-        if (ret < 0)
-            return ret;
+    ret = of_property_read_u32(node, "default-brightness-level",
+                          &value);
+    if (ret < 0)
+        return ret;

-        data->dft_brightness = value;
-        data->max_brightness--;
-    }
+    data->dft_brightness = value;

     return 0;
 }

-- 
2.0.0

[-- Attachment #2: Type: text/html, Size: 6167 bytes --]

Re: [PATCH] Added possibility to use pwm_bl.c with percentage instead of levels

[Thierry Reding]

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

On Fri, Jun 27, 2014 at 07:32:20AM +0200, Johannes Pointner wrote:
> Hello,
> 
> I'm new working on the linux device drivers, so if I made something wrong
> please point me into the right direction.
> 
> I'd like to use the pwm_bl driver for a sitara based terminal and for this
> I would need the possibility to set the backlight within a percentage
> range. The following patch should add this possibility to the pwm_bl
> driver. The idea is to keep backward compatibility by moving the required
> option brightness levels to optional. Therefore if there is no node with
> brightness levels the percentage levels are used.
> 
> Signed-off-by: Johannes Pointner <johannes.pointner@gmail.com>
> ---
>  .../bindings/video/backlight/
> pwm-backlight.txt     | 11 +++---
>  drivers/video/backlight/pwm_bl.c                   | 46
> ++++++++++++----------
>  2 files changed, 31 insertions(+), 26 deletions(-)

Also adding the backlight maintainers on Cc.

This has been discussed a few times before. In fact the original device
tree binding had support for a continuous range of levels but that was
rejected during review. The reason was, as far as I remember, that the
number of levels and the corresponding duty cycle values is something
that's usually determined at system design time. Often backlights can't
properly light the whole surface of the panel at every level.

That said, there's always the possibility to fake this by adding a DT
property with a continuous range, such as this:

	brightness-levels = <0 1 2 ... 100>;

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] Added possibility to use pwm_bl.c with percentage instead of levels

[Johannes Pointner]

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

I know what you mean because I have here for example a panel that only
supports a duty cycle between 20 and 100 percent. But wouldn't I be a
possibility to add optional nodes with minimal/maximal brightness or
something like that?

Hannes


2014-06-27 8:12 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:

> On Fri, Jun 27, 2014 at 07:32:20AM +0200, Johannes Pointner wrote:
> > Hello,
> >
> > I'm new working on the linux device drivers, so if I made something wrong
> > please point me into the right direction.
> >
> > I'd like to use the pwm_bl driver for a sitara based terminal and for
> this
> > I would need the possibility to set the backlight within a percentage
> > range. The following patch should add this possibility to the pwm_bl
> > driver. The idea is to keep backward compatibility by moving the required
> > option brightness levels to optional. Therefore if there is no node with
> > brightness levels the percentage levels are used.
> >
> > Signed-off-by: Johannes Pointner <johannes.pointner@gmail.com>
> > ---
> >  .../bindings/video/backlight/
> > pwm-backlight.txt     | 11 +++---
> >  drivers/video/backlight/pwm_bl.c                   | 46
> > ++++++++++++----------
> >  2 files changed, 31 insertions(+), 26 deletions(-)
>
> Also adding the backlight maintainers on Cc.
>
> This has been discussed a few times before. In fact the original device
> tree binding had support for a continuous range of levels but that was
> rejected during review. The reason was, as far as I remember, that the
> number of levels and the corresponding duty cycle values is something
> that's usually determined at system design time. Often backlights can't
> properly light the whole surface of the panel at every level.
>
> That said, there's always the possibility to fake this by adding a DT
> property with a continuous range, such as this:
>
>         brightness-levels = <0 1 2 ... 100>;
>
> Thierry
>

[-- Attachment #2: Type: text/html, Size: 2551 bytes --]

Re: [PATCH] Added possibility to use pwm_bl.c with percentage instead of levels

[Thierry Reding]

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

On Fri, Jun 27, 2014 at 08:58:21AM +0200, Johannes Pointner wrote:
> I know what you mean because I have here for example a panel that only
> supports a duty cycle between 20 and 100 percent. But wouldn't I be a
> possibility to add optional nodes with minimal/maximal brightness or
> something like that?

We have something like that (lth_brightness) for non-DT and which was
supported in the first DT proposals. But again the equivalent using the
brightness-levels would be:

	brightness-levels = <0 20 21 ... 100>;

So the continuous range and low threshold are really just special cases
of what brightness-levels provides. Granted, it's somewhat tedious to
have to list ~100 values in a DT property like this, but I haven't
really heard any arguments why the range really needs to be that
fine-grained. Most of the devices that I've used have somewhere between
8 and 20 brightness levels. An exception to this are Android devices,
which do indeed seem to support truly continuous ranges.

Thierry

> 
> Hannes
> 
> 
> 2014-06-27 8:12 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> 
> > On Fri, Jun 27, 2014 at 07:32:20AM +0200, Johannes Pointner wrote:
> > > Hello,
> > >
> > > I'm new working on the linux device drivers, so if I made something wrong
> > > please point me into the right direction.
> > >
> > > I'd like to use the pwm_bl driver for a sitara based terminal and for
> > this
> > > I would need the possibility to set the backlight within a percentage
> > > range. The following patch should add this possibility to the pwm_bl
> > > driver. The idea is to keep backward compatibility by moving the required
> > > option brightness levels to optional. Therefore if there is no node with
> > > brightness levels the percentage levels are used.
> > >
> > > Signed-off-by: Johannes Pointner <johannes.pointner@gmail.com>
> > > ---
> > >  .../bindings/video/backlight/
> > > pwm-backlight.txt     | 11 +++---
> > >  drivers/video/backlight/pwm_bl.c                   | 46
> > > ++++++++++++----------
> > >  2 files changed, 31 insertions(+), 26 deletions(-)
> >
> > Also adding the backlight maintainers on Cc.
> >
> > This has been discussed a few times before. In fact the original device
> > tree binding had support for a continuous range of levels but that was
> > rejected during review. The reason was, as far as I remember, that the
> > number of levels and the corresponding duty cycle values is something
> > that's usually determined at system design time. Often backlights can't
> > properly light the whole surface of the panel at every level.
> >
> > That said, there's always the possibility to fake this by adding a DT
> > property with a continuous range, such as this:
> >
> >         brightness-levels = <0 1 2 ... 100>;
> >
> > Thierry
> >

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] Added possibility to use pwm_bl.c with percentage instead of levels

[Johannes Pointner]

2014-06-27 9:09 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
> On Fri, Jun 27, 2014 at 08:58:21AM +0200, Johannes Pointner wrote:
>> I know what you mean because I have here for example a panel that only
>> supports a duty cycle between 20 and 100 percent. But wouldn't I be a
>> possibility to add optional nodes with minimal/maximal brightness or
>> something like that?
>
> We have something like that (lth_brightness) for non-DT and which was
> supported in the first DT proposals. But again the equivalent using the
> brightness-levels would be:
>
>         brightness-levels = <0 20 21 ... 100>;

You are right this would be a possibility but I thought that
configuring such ranges is much more readable than really writing it
down.

>
> So the continuous range and low threshold are really just special cases
> of what brightness-levels provides. Granted, it's somewhat tedious to
> have to list ~100 values in a DT property like this, but I haven't
> really heard any arguments why the range really needs to be that
> fine-grained. Most of the devices that I've used have somewhere between
> 8 and 20 brightness levels. An exception to this are Android devices,
> which do indeed seem to support truly continuous ranges.

In my case the fine granularity is demanded by our customers ( they
are all used to desktop monitors where they can freely adjust the
brightness even they don't really need that) and I have to make that
somehow possible. But I understand your concerns.

Hannes

>
> Thierry
>
>>
>> Hannes
>>
>>
>> 2014-06-27 8:12 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>:
>>
>> > On Fri, Jun 27, 2014 at 07:32:20AM +0200, Johannes Pointner wrote:
>> > > Hello,
>> > >
>> > > I'm new working on the linux device drivers, so if I made something wrong
>> > > please point me into the right direction.
>> > >
>> > > I'd like to use the pwm_bl driver for a sitara based terminal and for
>> > this
>> > > I would need the possibility to set the backlight within a percentage
>> > > range. The following patch should add this possibility to the pwm_bl
>> > > driver. The idea is to keep backward compatibility by moving the required
>> > > option brightness levels to optional. Therefore if there is no node with
>> > > brightness levels the percentage levels are used.
>> > >
>> > > Signed-off-by: Johannes Pointner <johannes.pointner@gmail.com>
>> > > ---
>> > >  .../bindings/video/backlight/
>> > > pwm-backlight.txt     | 11 +++---
>> > >  drivers/video/backlight/pwm_bl.c                   | 46
>> > > ++++++++++++----------
>> > >  2 files changed, 31 insertions(+), 26 deletions(-)
>> >
>> > Also adding the backlight maintainers on Cc.
>> >
>> > This has been discussed a few times before. In fact the original device
>> > tree binding had support for a continuous range of levels but that was
>> > rejected during review. The reason was, as far as I remember, that the
>> > number of levels and the corresponding duty cycle values is something
>> > that's usually determined at system design time. Often backlights can't
>> > properly light the whole surface of the panel at every level.
>> >
>> > That said, there's always the possibility to fake this by adding a DT
>> > property with a continuous range, such as this:
>> >
>> >         brightness-levels = <0 1 2 ... 100>;
>> >
>> > Thierry
>> >