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@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
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:
pgp05KyGEQog6.pgp
Description: PGP signature
