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

[Thierry Reding <thierry.reding@avionic-design.de>]

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

On Wed, Aug 01, 2012 at 06:51:21AM +0000, Philip, Avinash wrote:
> On Mon, Jul 30, 2012 at 12:28:05, Thierry Reding wrote:
> > On Wed, Jul 25, 2012 at 05:54:02PM +0530, Philip, Avinash wrote:
> > > Low Threshold Brightness should be configured to have a linear relation
> > > in brightness scale. This patch adds device tree support for low
> > > threshold brightness as optional one for pwm_backlight.
> > 
> > I think this should be more explicit as to why this is required, perhaps
> > something like this:
> > 
> > 	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.
> 
> Ok I will correct it.
> 
> > 
> > Perhaps a similar explanation should be given somewhere else instead of
> > just the changelog. It took me some time to understand what exactly this
> > low threshold means and I think it'd make sense to write this kind of
> > information down somewhere. I'll see if I can make time to add a bit of
> > documentation somewhere below Documentation/backlight perhaps.
> > 
> > > Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> > > ---
> > > :100644 100644 1e4fc72... 5c54380... M	Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> > > :100644 100644 995f016... 4965408... M	drivers/video/backlight/pwm_bl.c
> > >  .../bindings/video/backlight/pwm-backlight.txt     |   21 ++++++++++++++++++++
> > >  drivers/video/backlight/pwm_bl.c                   |    5 ++++
> > >  2 files changed, 26 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..5c54380 100644
> > > --- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> > > +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
> > > @@ -14,6 +14,8 @@ 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. (get linear
> > > +		 scales in brightness in low end of brightness levels)
> > 
> > The convention is to use dashes, not underscores, in device tree
> > property names, so this should be "low-threshold-brightness". I'd also
> > omit the comment in parentheses because the DT binding document
> > shouldn't specify any particular use-case. However I think it'd make
> > sense to add some information about the number space of the low
> > threshold value.
> 
> Ok, I will correct it.
> 
> > 
> > Maybe we should also rethink how the low threshold is handled in cases
> > where the brightness levels are used. I'm not sure it makes sense to
> > specify the low threshold as a value relative to the range given by the
> > levels. Perhaps it is more meaningful to specify it as relative to the
> > period/duty-cycle.
> > 
> > >  
> > >  [0]: Documentation/devicetree/bindings/pwm/pwm.txt
> > >  
> > > @@ -26,3 +28,22 @@ Example:
> > >  		brightness-levels = <0 4 8 16 32 64 128 255>;
> > >  		default-brightness-level = <6>;
> > >  	};
> > > +
> > > +Example for brightness_threshold_level:
> > > +
> > > +	backlight {
> > > +		compatible	= "pwm-backlight";
> > > +		pwms = <&pwm 0 50000>;
> > > +
> > > +		brightness-levels = <0 4 8 16 32 64 128 255>;
> > > +		default-brightness-level = <6>;
> > > +		low_threshold_brightness = <50>;
> > > +	};
> > > +};
> > 
> > I think you can just merge the low-threshold-brightness with the
> > previous example.
> 
> Ok I will check and correct it.
> 
> > 
> > > +Note:
> > > +Low threshold support is required to have linear brightness scale from
> > > +0 to max. For some panels, backlight absent on low end of brightness
> > > +scale. So support for Low Threshold been required. So that the scale of
> > > +brightness changed from Low Threshold to Max in scales defined in
> > > +brightness-levels. In this example 20% maximum brightness scale should
> > > +be required to turn on panel backlight.
> > 
> > I think this kind of documentation doesn't belong in the device tree
> > binding. I'll work something like that into the proper documentation.
> > 
> > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > index 995f016..4965408 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;
> > >  	}
> > 
> > This obviously should also be low-threshold-brightness.
> 
> I will change to
> 
> ret = of_property_read_u32(node, " low-threshold-brightness ",
> 
> Thanks
> Avinash

I think you never sent an updated patch or maybe I missed it. But I
noticed that this patch has now ended up in Andrew's tree without the
comments being addressed. Can you please follow up?

Andrew: Can you remove this from your tree? As you can see it has been
discussed before. I've mentioned elsewhere that I volunteer to take over
maintenance of pwm-backlight so I'll carry this patch through the PWM
tree.

Thierry

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