[PATCH v2] pwm_backlight: Add device tree support for Low Threshold Brightness

[PATCH v2] pwm_backlight: Add device tree support for Low Threshold Brightness

[Philip, Avinash]

Some backlights perform poorly when driven by a PWM with a short
duty-cycle. For such devices, the low threshold can be used to specify a
lower bound for the duty-cycle and should be chosen to exclude the
problematic range.

This patch adds support for an optional low-threshold-brightness
property.

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
---
Changes since v1:
        - Updated commit message.
	- Changes to low-threshold-brightness.
	- Merged example section to original.

:100644 100644 1e4fc72... 5baebff... M	Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
:100644 100644 995f016... 29e6fe1... M	drivers/video/backlight/pwm_bl.c
 .../bindings/video/backlight/pwm-backlight.txt     |    4 ++++
 drivers/video/backlight/pwm_bl.c                   |    5 +++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..5baebff 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -14,6 +14,9 @@ Required properties:
 Optional properties:
   - pwm-names: a list of names for the PWM devices specified in the
                "pwms" property (see PWM binding[0])
+  - low-threshold-brightness: brightness threshold low level. Low threshold
+    brightness set to value so that backlight present on low end of
+    brightness.
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 
@@ -25,4 +28,5 @@ Example:
 
 		brightness-levels = <0 4 8 16 32 64 128 255>;
 		default-brightness-level = <6>;
+		low-threshold-brightness = <50>;
 	};
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 995f016..29e6fe1 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -143,6 +143,11 @@ static int pwm_backlight_parse_dt(struct device *dev,
 
 		data->dft_brightness = value;
 		data->max_brightness--;
+
+		ret = of_property_read_u32(node, "low-threshold-brightness",
+					   &value);
+		if (!ret)
+			data->lth_brightness = value;
 	}
 
 	/*
-- 
1.7.0.4

Re: [PATCH v2] pwm_backlight: Add device tree support for Low Threshold Brightness

[Stephen Warren]

On 09/20/2012 10:51 PM, Philip, Avinash wrote:
> Some backlights perform poorly when driven by a PWM with a short
> duty-cycle. For such devices, the low threshold can be used to specify a
> lower bound for the duty-cycle and should be chosen to exclude the
> problematic range.
> 
> This patch adds support for an optional low-threshold-brightness
> property.

> diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt

>  Optional properties:
>    - pwm-names: a list of names for the PWM devices specified in the
>                 "pwms" property (see PWM binding[0])
> +  - low-threshold-brightness: brightness threshold low level. Low threshold
> +    brightness set to value so that backlight present on low end of
> +    brightness.

For my education, why not just specify values above this value in the
brightness-levels array; how do those two interact? It seems like any
description of that is missing from the PWM documentation, and would be
good to have in the DT binding too.

RE: [PATCH v2] pwm_backlight: Add device tree support for Low Threshold Brightness

[Philip, Avinash]

Hi Stephen,

On Fri, Sep 21, 2012 at 10:46:45, Stephen Warren wrote:
> On 09/20/2012 10:51 PM, Philip, Avinash wrote:
> > Some backlights perform poorly when driven by a PWM with a short
> > duty-cycle. For such devices, the low threshold can be used to specify a
> > lower bound for the duty-cycle and should be chosen to exclude the
> > problematic range.
> > 
> > This patch adds support for an optional low-threshold-brightness
> > property.
> 
> > diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> 
> >  Optional properties:
> >    - pwm-names: a list of names for the PWM devices specified in the
> >                 "pwms" property (see PWM binding[0])
> > +  - low-threshold-brightness: brightness threshold low level. Low threshold
> > +    brightness set to value so that backlight present on low end of
> > +    brightness.
> 
> For my education, why not just specify values above this value in the
> brightness-levels array; how do those two interact?

Please find details from 
https://lkml.org/lkml/2012/7/18/284

Thanks
Avinash

> It seems like any
> description of that is missing from the PWM documentation, and would be
> good to have in the DT binding too.
> 
> 

Re: [PATCH v2] pwm_backlight: Add device tree support for Low Threshold Brightness

[Stephen Warren]

On 09/21/2012 12:03 AM, Philip, Avinash wrote:
> Hi Stephen,
> 
> On Fri, Sep 21, 2012 at 10:46:45, Stephen Warren wrote:
>> On 09/20/2012 10:51 PM, Philip, Avinash wrote:
>>> Some backlights perform poorly when driven by a PWM with a short
>>> duty-cycle. For such devices, the low threshold can be used to specify a
>>> lower bound for the duty-cycle and should be chosen to exclude the
>>> problematic range.
>>>
>>> This patch adds support for an optional low-threshold-brightness
>>> property.
>>
>>> diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
>>
>>>  Optional properties:
>>>    - pwm-names: a list of names for the PWM devices specified in the
>>>                 "pwms" property (see PWM binding[0])
>>> +  - low-threshold-brightness: brightness threshold low level. Low threshold
>>> +    brightness set to value so that backlight present on low end of
>>> +    brightness.
>>
>> For my education, why not just specify values above this value in the
>> brightness-levels array; how do those two interact?
> 
> Please find details from 
> https://lkml.org/lkml/2012/7/18/284

Hmm. That still doesn't really explain what this property does.

I'm going to guess that if this property is present, and values in the
brightness-levels property get scaled between the
low-threshold-brightness and 255 instead of being used directly. But
then, in the email you linked to, what does "But brightness-levels won't
be uniformly divided" mean? Why would doing the calculation at run-time
be any better than simply putting the correct values into
brightness-levels in the first place?

Either way, the DT binding should explain exactly what this value is
used for, and how it affects the interpretation of values in
brightness-levels.

RE: [PATCH v2] pwm_backlight: Add device tree support for Low Threshold Brightness

[Philip, Avinash]

On Fri, Sep 21, 2012 at 23:13:39, Stephen Warren wrote:
> On 09/21/2012 12:03 AM, Philip, Avinash wrote:
> > Hi Stephen,
> > 
> > On Fri, Sep 21, 2012 at 10:46:45, Stephen Warren wrote:
> >> On 09/20/2012 10:51 PM, Philip, Avinash wrote:
> >>> Some backlights perform poorly when driven by a PWM with a short
> >>> duty-cycle. For such devices, the low threshold can be used to specify a
> >>> lower bound for the duty-cycle and should be chosen to exclude the
> >>> problematic range.
> >>>
> >>> This patch adds support for an optional low-threshold-brightness
> >>> property.
> >>
> >>> diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> >>
> >>>  Optional properties:
> >>>    - pwm-names: a list of names for the PWM devices specified in the
> >>>                 "pwms" property (see PWM binding[0])
> >>> +  - low-threshold-brightness: brightness threshold low level. Low threshold
> >>> +    brightness set to value so that backlight present on low end of
> >>> +    brightness.
> >>
> >> For my education, why not just specify values above this value in the
> >> brightness-levels array; how do those two interact?
> > 
> > Please find details from 
> > https://lkml.org/lkml/2012/7/18/284
> 
> Hmm. That still doesn't really explain what this property does.
> 
> I'm going to guess that if this property is present, and values in the
> brightness-levels property get scaled between the
> low-threshold-brightness and 255 instead of being used directly.

This is correct.

> But then, in the email you linked to, what does "But brightness-levels won't
> be uniformly divided" mean?

For some panels, backlight would absent on low end of brightness due to low
percentage in duty_cycle. Consider following example where backlight absent
for brightness levels from 0 - 51.

pwms = <&pwm 0 50000>;
brightness-levels = <0 51 53 56 62 75 101 152 255>; 
default-brightness-level = <6>;

So in the example, brightness-levels are set to have values for backlight present.
Here levels are not uniformly divided.

But by providing,
brightness_threshold_level = <50>;

we can have a uniform division in brightness-levels as below

brightness-levels = <0 2 4 8 16 32 64 128 255>;


> Why would doing the calculation at run-time
> be any better than simply putting the correct values into
> brightness-levels in the first place?

I think you mean to provide the backlight-levels as in example
brightness-levels = <0 51 53 56 62 75 101 152 255>;

In that case, I still feel adding brightness_threshold_level would be a
better option.

> 
> Either way, the DT binding should explain exactly what this value is
> used for, and how it affects the interpretation of values in
> brightness-levels.

Is DT binding documentation a good place to explain this feature?
Initially Thierry suggested document option. So I left out.

Thanks
Avinash

> 

Re: [PATCH v2] pwm_backlight: Add device tree support for Low Threshold Brightness

[Stephen Warren]

On 09/24/2012 10:29 PM, Philip, Avinash wrote:
> On Fri, Sep 21, 2012 at 23:13:39, Stephen Warren wrote:
>> On 09/21/2012 12:03 AM, Philip, Avinash wrote:
>>> Hi Stephen,
>>>
>>> On Fri, Sep 21, 2012 at 10:46:45, Stephen Warren wrote:
>>>> On 09/20/2012 10:51 PM, Philip, Avinash wrote:
>>>>> Some backlights perform poorly when driven by a PWM with a short
>>>>> duty-cycle. For such devices, the low threshold can be used to specify a
>>>>> lower bound for the duty-cycle and should be chosen to exclude the
>>>>> problematic range.
>>>>>
>>>>> This patch adds support for an optional low-threshold-brightness
>>>>> property.
>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
>>>>
>>>>>  Optional properties:
>>>>>    - pwm-names: a list of names for the PWM devices specified in the
>>>>>                 "pwms" property (see PWM binding[0])
>>>>> +  - low-threshold-brightness: brightness threshold low level. Low threshold
>>>>> +    brightness set to value so that backlight present on low end of
>>>>> +    brightness.
>>>>
>>>> For my education, why not just specify values above this value in the
>>>> brightness-levels array; how do those two interact?
>>>
>>> Please find details from 
>>> https://lkml.org/lkml/2012/7/18/284
>>
>> Hmm. That still doesn't really explain what this property does.
>>
>> I'm going to guess that if this property is present, and values in the
>> brightness-levels property get scaled between the
>> low-threshold-brightness and 255 instead of being used directly.
> 
> This is correct.
> 
>> But then, in the email you linked to, what does "But brightness-levels won't
>> be uniformly divided" mean?
> 
> For some panels, backlight would absent on low end of brightness due to low
> percentage in duty_cycle. Consider following example where backlight absent
> for brightness levels from 0 - 51.
> 
> pwms = <&pwm 0 50000>;
> brightness-levels = <0 51 53 56 62 75 101 152 255>; 
> default-brightness-level = <6>;
> 
> So in the example, brightness-levels are set to have values for backlight present.
> Here levels are not uniformly divided.

So why not just change the values so they /are/ what you want? After
all, it's just data and you can put whatever values you want there. What
is preventing you from doing this?

Perhaps e.g.:

brightness-levels = <0 101 106 112 124 150 202 304 511>;
(just multiplying everything by N, for arbitrary N=2, to get extra
precision)

... plus whatever adjustments are required to make the data "uniformly
divided", which I can't do in the example here since I'd need to know
whatever non-linear equation characterizes the backlight's PWM % duty
cycle to perceived brightness mapping.

The only thing that could be preventing this is mathematical precision.
While all the PWM DT examples I've seen have brightness-levels range
from 0..255, I don't think there is any such actual limit; you could
range from say 0..1000000 if you wanted, right?

>> Either way, the DT binding should explain exactly what this value is
>> used for, and how it affects the interpretation of values in
>> brightness-levels.
> 
> Is DT binding documentation a good place to explain this feature?
> Initially Thierry suggested document option. So I left out.

The binding documents are supposed to be a standalone description of
what the data in DT does; given general no-Linux-specific domain
knowledge, the binding document should be detailed enough for someone to
understand how to fill in the DT. So, yes, I think the binding document
would be a great place to put such documentation.

RE: [PATCH v2] pwm_backlight: Add device tree support for Low Threshold Brightness

[Philip, Avinash]

On Tue, Sep 25, 2012 at 11:49:14, Stephen Warren wrote:
> On 09/24/2012 10:29 PM, Philip, Avinash wrote:
> > On Fri, Sep 21, 2012 at 23:13:39, Stephen Warren wrote:
> >> On 09/21/2012 12:03 AM, Philip, Avinash wrote:
> >>> Hi Stephen,
> >>>
> >>> On Fri, Sep 21, 2012 at 10:46:45, Stephen Warren wrote:
> >>>> On 09/20/2012 10:51 PM, Philip, Avinash wrote:
> >>>>> Some backlights perform poorly when driven by a PWM with a short
> >>>>> duty-cycle. For such devices, the low threshold can be used to specify a
> >>>>> lower bound for the duty-cycle and should be chosen to exclude the
> >>>>> problematic range.
> >>>>>
> >>>>> This patch adds support for an optional low-threshold-brightness
> >>>>> property.
> >>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> >>>>
> >>>>>  Optional properties:
> >>>>>    - pwm-names: a list of names for the PWM devices specified in the
> >>>>>                 "pwms" property (see PWM binding[0])
> >>>>> +  - low-threshold-brightness: brightness threshold low level. Low threshold
> >>>>> +    brightness set to value so that backlight present on low end of
> >>>>> +    brightness.
> >>>>
> >>>> For my education, why not just specify values above this value in the
> >>>> brightness-levels array; how do those two interact?
> >>>
> >>> Please find details from 
> >>> https://lkml.org/lkml/2012/7/18/284
> >>
> >> Hmm. That still doesn't really explain what this property does.
> >>
> >> I'm going to guess that if this property is present, and values in the
> >> brightness-levels property get scaled between the
> >> low-threshold-brightness and 255 instead of being used directly.
> > 
> > This is correct.
> > 
> >> But then, in the email you linked to, what does "But brightness-levels won't
> >> be uniformly divided" mean?
> > 
> > For some panels, backlight would absent on low end of brightness due to low
> > percentage in duty_cycle. Consider following example where backlight absent
> > for brightness levels from 0 - 51.
> > 
> > pwms = <&pwm 0 50000>;
> > brightness-levels = <0 51 53 56 62 75 101 152 255>; 
> > default-brightness-level = <6>;
> > 
> > So in the example, brightness-levels are set to have values for backlight present.
> > Here levels are not uniformly divided.
> 
> So why not just change the values so they /are/ what you want? After
> all, it's just data and you can put whatever values you want there. What
> is preventing you from doing this?

brightness_threshold_level was added to explore lth_brightness support already
present in non-DT case.
 
> 
> Perhaps e.g.:
> 
> brightness-levels = <0 101 106 112 124 150 202 304 511>;
> (just multiplying everything by N, for arbitrary N=2, to get extra
> precision)
> 
> ... plus whatever adjustments are required to make the data "uniformly
> divided", which I can't do in the example here since I'd need to know
> whatever non-linear equation characterizes the backlight's PWM % duty
> cycle to perceived brightness mapping.
> 
> The only thing that could be preventing this is mathematical precision.
> While all the PWM DT examples I've seen have brightness-levels range
> from 0..255, I don't think there is any such actual limit; you could
> range from say 0..1000000 if you wanted, right?

The observation is correct. There are no fixed levels, configure these values 
as required. This is a scale of division for brightness variation. A linear
division in brightness won't give much difference in high end of brightness-levels
scale. So adopting binary division in brightness-levels will allow better resolution
in brightness. 

> 
> >> Either way, the DT binding should explain exactly what this value is
> >> used for, and how it affects the interpretation of values in
> >> brightness-levels.
> > 
> > Is DT binding documentation a good place to explain this feature?
> > Initially Thierry suggested document option. So I left out.
> 
> The binding documents are supposed to be a standalone description of
> what the data in DT does; given general no-Linux-specific domain
> knowledge, the binding document should be detailed enough for someone to
> understand how to fill in the DT. So, yes, I think the binding document
> would be a great place to put such documentation.

I will add details.

Thanks
Avinash

> 

Re: [PATCH v2] pwm_backlight: Add device tree support for Low Threshold Brightness

[Stephen Warren]

On 09/25/2012 10:35 PM, Philip, Avinash wrote:
> On Tue, Sep 25, 2012 at 11:49:14, Stephen Warren wrote:
>> On 09/24/2012 10:29 PM, Philip, Avinash wrote:
>>> On Fri, Sep 21, 2012 at 23:13:39, Stephen Warren wrote:
>>>> On 09/21/2012 12:03 AM, Philip, Avinash wrote:
>>>>> Hi Stephen,
>>>>>
>>>>> On Fri, Sep 21, 2012 at 10:46:45, Stephen Warren wrote:
>>>>>> On 09/20/2012 10:51 PM, Philip, Avinash wrote:
>>>>>>> Some backlights perform poorly when driven by a PWM with a short
>>>>>>> duty-cycle. For such devices, the low threshold can be used to specify a
>>>>>>> lower bound for the duty-cycle and should be chosen to exclude the
>>>>>>> problematic range.
>>>>>>>
>>>>>>> This patch adds support for an optional low-threshold-brightness
>>>>>>> property.
>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
>>>>>>
>>>>>>>  Optional properties:
>>>>>>>    - pwm-names: a list of names for the PWM devices specified in the
>>>>>>>                 "pwms" property (see PWM binding[0])
>>>>>>> +  - low-threshold-brightness: brightness threshold low level. Low threshold
>>>>>>> +    brightness set to value so that backlight present on low end of
>>>>>>> +    brightness.
>>>>>>
>>>>>> For my education, why not just specify values above this value in the
>>>>>> brightness-levels array; how do those two interact?
>>>>>
>>>>> Please find details from 
>>>>> https://lkml.org/lkml/2012/7/18/284
>>>>
>>>> Hmm. That still doesn't really explain what this property does.
>>>>
>>>> I'm going to guess that if this property is present, and values in the
>>>> brightness-levels property get scaled between the
>>>> low-threshold-brightness and 255 instead of being used directly.
>>>
>>> This is correct.
>>>
>>>> But then, in the email you linked to, what does "But brightness-levels won't
>>>> be uniformly divided" mean?
>>>
>>> For some panels, backlight would absent on low end of brightness due to low
>>> percentage in duty_cycle. Consider following example where backlight absent
>>> for brightness levels from 0 - 51.
>>>
>>> pwms = <&pwm 0 50000>;
>>> brightness-levels = <0 51 53 56 62 75 101 152 255>; 
>>> default-brightness-level = <6>;
>>>
>>> So in the example, brightness-levels are set to have values for backlight present.
>>> Here levels are not uniformly divided.
>>
>> So why not just change the values so they /are/ what you want? After
>> all, it's just data and you can put whatever values you want there. What
>> is preventing you from doing this?
> 
> brightness_threshold_level was added to explore lth_brightness support already
> present in non-DT case.

I understand that. Given my discussion above, I would advocate removing
lth_brightness from the non-DT case rather than adding it to the DT
case, since it seems entirely pointless.

Re: [PATCH v2] pwm_backlight: Add device tree support for Low Threshold Brightness

[Thierry Reding]

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

On Wed, Sep 26, 2012 at 09:27:51AM -0600, Stephen Warren wrote:
> On 09/25/2012 10:35 PM, Philip, Avinash wrote:
> > On Tue, Sep 25, 2012 at 11:49:14, Stephen Warren wrote:
> >> On 09/24/2012 10:29 PM, Philip, Avinash wrote:
> >>> On Fri, Sep 21, 2012 at 23:13:39, Stephen Warren wrote:
> >>>> On 09/21/2012 12:03 AM, Philip, Avinash wrote:
> >>>>> Hi Stephen,
> >>>>>
> >>>>> On Fri, Sep 21, 2012 at 10:46:45, Stephen Warren wrote:
> >>>>>> On 09/20/2012 10:51 PM, Philip, Avinash wrote:
> >>>>>>> Some backlights perform poorly when driven by a PWM with a short
> >>>>>>> duty-cycle. For such devices, the low threshold can be used to specify a
> >>>>>>> lower bound for the duty-cycle and should be chosen to exclude the
> >>>>>>> problematic range.
> >>>>>>>
> >>>>>>> This patch adds support for an optional low-threshold-brightness
> >>>>>>> property.
> >>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> >>>>>>
> >>>>>>>  Optional properties:
> >>>>>>>    - pwm-names: a list of names for the PWM devices specified in the
> >>>>>>>                 "pwms" property (see PWM binding[0])
> >>>>>>> +  - low-threshold-brightness: brightness threshold low level. Low threshold
> >>>>>>> +    brightness set to value so that backlight present on low end of
> >>>>>>> +    brightness.
> >>>>>>
> >>>>>> For my education, why not just specify values above this value in the
> >>>>>> brightness-levels array; how do those two interact?
> >>>>>
> >>>>> Please find details from 
> >>>>> https://lkml.org/lkml/2012/7/18/284
> >>>>
> >>>> Hmm. That still doesn't really explain what this property does.
> >>>>
> >>>> I'm going to guess that if this property is present, and values in the
> >>>> brightness-levels property get scaled between the
> >>>> low-threshold-brightness and 255 instead of being used directly.
> >>>
> >>> This is correct.
> >>>
> >>>> But then, in the email you linked to, what does "But brightness-levels won't
> >>>> be uniformly divided" mean?
> >>>
> >>> For some panels, backlight would absent on low end of brightness due to low
> >>> percentage in duty_cycle. Consider following example where backlight absent
> >>> for brightness levels from 0 - 51.
> >>>
> >>> pwms = <&pwm 0 50000>;
> >>> brightness-levels = <0 51 53 56 62 75 101 152 255>; 
> >>> default-brightness-level = <6>;
> >>>
> >>> So in the example, brightness-levels are set to have values for backlight present.
> >>> Here levels are not uniformly divided.
> >>
> >> So why not just change the values so they /are/ what you want? After
> >> all, it's just data and you can put whatever values you want there. What
> >> is preventing you from doing this?
> > 
> > brightness_threshold_level was added to explore lth_brightness support already
> > present in non-DT case.
> 
> I understand that. Given my discussion above, I would advocate removing
> lth_brightness from the non-DT case rather than adding it to the DT
> case, since it seems entirely pointless.

It is still required for the case where brightness levels are not used.
So we can't drop it right away. I agree however that we should plan to
get rid of the max_brightness and lth_brightness eventually. Since the
DT bindings don't use it yet we should keep only the brightness levels.
Once all users have been converted we can rename max_brightness to
something like num_levels and remove lth_brightness. dft_brightness can
probably be renamed to default_level.

Thierry

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

RE: [PATCH v2] pwm_backlight: Add device tree support for Low Threshold Brightness

[Philip, Avinash]

On Wed, Sep 26, 2012 at 22:19:49, Thierry Reding wrote:
> On Wed, Sep 26, 2012 at 09:27:51AM -0600, Stephen Warren wrote:
> > On 09/25/2012 10:35 PM, Philip, Avinash wrote:
> > > On Tue, Sep 25, 2012 at 11:49:14, Stephen Warren wrote:
> > >> On 09/24/2012 10:29 PM, Philip, Avinash wrote:
> > >>> On Fri, Sep 21, 2012 at 23:13:39, Stephen Warren wrote:
> > >>>> On 09/21/2012 12:03 AM, Philip, Avinash wrote:
> > >>>>> Hi Stephen,
> > >>>>>
> > >>>>> On Fri, Sep 21, 2012 at 10:46:45, Stephen Warren wrote:
> > >>>>>> On 09/20/2012 10:51 PM, Philip, Avinash wrote:
> > >>>>>>> Some backlights perform poorly when driven by a PWM with a short
> > >>>>>>> duty-cycle. For such devices, the low threshold can be used to specify a
> > >>>>>>> lower bound for the duty-cycle and should be chosen to exclude the
> > >>>>>>> problematic range.
> > >>>>>>>
> > >>>>>>> This patch adds support for an optional low-threshold-brightness
> > >>>>>>> property.
> > >>>>>>
> > >>>>>>> diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> > >>>>>>
> > >>>>>>>  Optional properties:
> > >>>>>>>    - pwm-names: a list of names for the PWM devices specified in the
> > >>>>>>>                 "pwms" property (see PWM binding[0])
> > >>>>>>> +  - low-threshold-brightness: brightness threshold low level. Low threshold
> > >>>>>>> +    brightness set to value so that backlight present on low end of
> > >>>>>>> +    brightness.
> > >>>>>>
> > >>>>>> For my education, why not just specify values above this value in the
> > >>>>>> brightness-levels array; how do those two interact?
> > >>>>>
> > >>>>> Please find details from 
> > >>>>> https://lkml.org/lkml/2012/7/18/284
> > >>>>
> > >>>> Hmm. That still doesn't really explain what this property does.
> > >>>>
> > >>>> I'm going to guess that if this property is present, and values in the
> > >>>> brightness-levels property get scaled between the
> > >>>> low-threshold-brightness and 255 instead of being used directly.
> > >>>
> > >>> This is correct.
> > >>>
> > >>>> But then, in the email you linked to, what does "But brightness-levels won't
> > >>>> be uniformly divided" mean?
> > >>>
> > >>> For some panels, backlight would absent on low end of brightness due to low
> > >>> percentage in duty_cycle. Consider following example where backlight absent
> > >>> for brightness levels from 0 - 51.
> > >>>
> > >>> pwms = <&pwm 0 50000>;
> > >>> brightness-levels = <0 51 53 56 62 75 101 152 255>; 
> > >>> default-brightness-level = <6>;
> > >>>
> > >>> So in the example, brightness-levels are set to have values for backlight present.
> > >>> Here levels are not uniformly divided.
> > >>
> > >> So why not just change the values so they /are/ what you want? After
> > >> all, it's just data and you can put whatever values you want there. What
> > >> is preventing you from doing this?
> > > 
> > > brightness_threshold_level was added to explore lth_brightness support already
> > > present in non-DT case.
> > 
> > I understand that. Given my discussion above, I would advocate removing
> > lth_brightness from the non-DT case rather than adding it to the DT
> > case, since it seems entirely pointless.
> 
> It is still required for the case where brightness levels are not used.
> So we can't drop it right away. I agree however that we should plan to
> get rid of the max_brightness and lth_brightness eventually. Since the
> DT bindings don't use it yet we should keep only the brightness levels.
> Once all users have been converted we can rename max_brightness to
> something like num_levels and remove lth_brightness. dft_brightness can
> probably be renamed to default_level.

In non-DT case lth_brightness is required. 
But for DT we have options with/without lth_brightness support. 
In case if patch is dropped, user has to put proper brightness-levels 
(brightness-levels DT parameters should be specified considering the 
low-threshold values)

Meanwhile I had submitted another version yesterday (with more documentation)

[PATCH v3] pwm_backlight: Add device tree support for Low Threshold Brightness

https://lkml.org/lkml/2012/9/26/271

Thierry,

Can you please confirm the acceptance/rejection of the patch? This will help me
to submit the backlight DT blob for AM335x platform.

Thanks
Avinash

> 
> Thierry
> 

Re: [PATCH v2] pwm_backlight: Add device tree support for Low Threshold Brightness

[Thierry Reding]

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

On Thu, Sep 27, 2012 at 09:24:47AM +0000, Philip, Avinash wrote:
> On Wed, Sep 26, 2012 at 22:19:49, Thierry Reding wrote:
> > On Wed, Sep 26, 2012 at 09:27:51AM -0600, Stephen Warren wrote:
> > > On 09/25/2012 10:35 PM, Philip, Avinash wrote:
> > > > On Tue, Sep 25, 2012 at 11:49:14, Stephen Warren wrote:
> > > >> On 09/24/2012 10:29 PM, Philip, Avinash wrote:
> > > >>> On Fri, Sep 21, 2012 at 23:13:39, Stephen Warren wrote:
> > > >>>> On 09/21/2012 12:03 AM, Philip, Avinash wrote:
> > > >>>>> Hi Stephen,
> > > >>>>>
> > > >>>>> On Fri, Sep 21, 2012 at 10:46:45, Stephen Warren wrote:
> > > >>>>>> On 09/20/2012 10:51 PM, Philip, Avinash wrote:
> > > >>>>>>> Some backlights perform poorly when driven by a PWM with a short
> > > >>>>>>> duty-cycle. For such devices, the low threshold can be used to specify a
> > > >>>>>>> lower bound for the duty-cycle and should be chosen to exclude the
> > > >>>>>>> problematic range.
> > > >>>>>>>
> > > >>>>>>> This patch adds support for an optional low-threshold-brightness
> > > >>>>>>> property.
> > > >>>>>>
> > > >>>>>>> diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> > > >>>>>>
> > > >>>>>>>  Optional properties:
> > > >>>>>>>    - pwm-names: a list of names for the PWM devices specified in the
> > > >>>>>>>                 "pwms" property (see PWM binding[0])
> > > >>>>>>> +  - low-threshold-brightness: brightness threshold low level. Low threshold
> > > >>>>>>> +    brightness set to value so that backlight present on low end of
> > > >>>>>>> +    brightness.
> > > >>>>>>
> > > >>>>>> For my education, why not just specify values above this value in the
> > > >>>>>> brightness-levels array; how do those two interact?
> > > >>>>>
> > > >>>>> Please find details from 
> > > >>>>> https://lkml.org/lkml/2012/7/18/284
> > > >>>>
> > > >>>> Hmm. That still doesn't really explain what this property does.
> > > >>>>
> > > >>>> I'm going to guess that if this property is present, and values in the
> > > >>>> brightness-levels property get scaled between the
> > > >>>> low-threshold-brightness and 255 instead of being used directly.
> > > >>>
> > > >>> This is correct.
> > > >>>
> > > >>>> But then, in the email you linked to, what does "But brightness-levels won't
> > > >>>> be uniformly divided" mean?
> > > >>>
> > > >>> For some panels, backlight would absent on low end of brightness due to low
> > > >>> percentage in duty_cycle. Consider following example where backlight absent
> > > >>> for brightness levels from 0 - 51.
> > > >>>
> > > >>> pwms = <&pwm 0 50000>;
> > > >>> brightness-levels = <0 51 53 56 62 75 101 152 255>; 
> > > >>> default-brightness-level = <6>;
> > > >>>
> > > >>> So in the example, brightness-levels are set to have values for backlight present.
> > > >>> Here levels are not uniformly divided.
> > > >>
> > > >> So why not just change the values so they /are/ what you want? After
> > > >> all, it's just data and you can put whatever values you want there. What
> > > >> is preventing you from doing this?
> > > > 
> > > > brightness_threshold_level was added to explore lth_brightness support already
> > > > present in non-DT case.
> > > 
> > > I understand that. Given my discussion above, I would advocate removing
> > > lth_brightness from the non-DT case rather than adding it to the DT
> > > case, since it seems entirely pointless.
> > 
> > It is still required for the case where brightness levels are not used.
> > So we can't drop it right away. I agree however that we should plan to
> > get rid of the max_brightness and lth_brightness eventually. Since the
> > DT bindings don't use it yet we should keep only the brightness levels.
> > Once all users have been converted we can rename max_brightness to
> > something like num_levels and remove lth_brightness. dft_brightness can
> > probably be renamed to default_level.
> 
> In non-DT case lth_brightness is required. 
> But for DT we have options with/without lth_brightness support. 
> In case if patch is dropped, user has to put proper brightness-levels 
> (brightness-levels DT parameters should be specified considering the 
> low-threshold values)
> 
> Meanwhile I had submitted another version yesterday (with more documentation)
> 
> [PATCH v3] pwm_backlight: Add device tree support for Low Threshold Brightness
> 
> https://lkml.org/lkml/2012/9/26/271
> 
> Thierry,
> 
> Can you please confirm the acceptance/rejection of the patch? This will help me
> to submit the backlight DT blob for AM335x platform.

Let's go with the brightness levels approach. We have the upcoming power
sequence stuff that is needed to get rid of the platform data callbacks
and I'm afraid if we keep adding wildly. I think there are already too
many possible combinations right now and we should get them cleaned up
sooner rather than later.

Thierry

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