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. 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@xxxxxx> > --- > :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. 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. > +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. Thierry
Attachment:
pgpz_dyw8qXYk.pgp
Description: PGP signature