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@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.
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
>
> Thierry
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
