On Wed, 2018-07-18 at 09:09 +0100, Lee Jones wrote:
> On Mon, 16 Jul 2018, Daniel Thompson wrote:
>
> > Currently, if the DT does not define num-interpolated-steps then
> > num_steps is undefined and the interpolation code will deploy
> > randomly.
> > Fix this.
> >
> > Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation
> > between
> > brightness-levels")
> > Reported-by: Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx>
> > Signed-off-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx>
>
> This line is confusing. Did you guys author this patch together?
Yes, I reported it and we came to a conclusion together.
> My guess is that this line should be dropped and the RB and TB tags
> should remain? If it was reviewed too, perhaps an AB too?
I'm OK either way and do not need any explicit authorship to be
expressed for me.
> > Tested-by: Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx>
> > ---
> > drivers/video/backlight/pwm_bl.c | 17 ++++++++---------
> > 1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/video/backlight/pwm_bl.c
> > b/drivers/video/backlight/pwm_bl.c
> > index 9ee4c1b735b2..e3c22b79fbcd 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -299,15 +299,14 @@ static int pwm_backlight_parse_dt(struct
> > device *dev,
> > * interpolation between each of the values of
> > brightness levels
> > * and creates a new pre-computed table.
> > */
> > - of_property_read_u32(node, "num-interpolated-
> > steps",
> > - &num_steps);
> > -
> > - /*
> > - * Make sure that there is at least two entries in
> > the
> > - * brightness-levels table, otherwise we can't
> > interpolate
> > - * between two points.
> > - */
> > - if (num_steps) {
> > + if ((of_property_read_u32(node, "num-interpolated-
> > steps",
> > + &num_steps) == 0) &&
> > num_steps) {
>
> This is pretty ugly, and isn't it suffering from over-bracketing? My
> suggestion would be to break out the invocation of
> of_property_read_u32() from the if and test only the result.
>
> of_property_read_u32(node, "num-interpolated-steps",
> &num_steps);
you mean:
ret = of_property_read_u32(node, "num-interpolated-
steps", &num_steps);
> if (!ret && num_steps) {
>
> I haven't checked the underling code, but is it even feasible for
> of_property_read_u32() to not succeed AND for num_steps to be set?
>
> If not, the check for !ret if superfluous and you can drop it.
No, then we are back to the initial issue of num_steps potentially not
being initialised. We really want both of_property_read_u32() to
succeed AND num_steps to actually be set.
> > + /*
> > + * Make sure that there is at least two
> > entries in the
>
> s/is/are/
>
> > + * brightness-levels table, otherwise we
> > can't
> > + * interpolate
>
> Why break the line here?
That's probably a remnant of going back and forth plus quoting on the
mailing list.
> > + * between two points.
> > + */
> > if (data->max_brightness < 2) {
> > dev_err(dev, "can't
> > interpolate\n");
> > return -EINVAL;
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
