On Mon, Jul 23, 2018 at 08:23:43AM +0100, Lee Jones wrote:
> On Thu, 19 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.
> >
> > Additionally fix a small grammar error that was identified and
> > tighten up return code checking of DT properties, both of which came
> > up during review of this patch.
> >
> > 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>
> > Tested-by: Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx>
> > ---
> >
> > Notes:
> > v2:
> > - Simplify SoB chain (with Marcel's permission)
> > - Separate complex if statement and make other similar calls use same
> > return code checking approach
> > - Tidy up comment formatting and fix pre-existing grammar error
> >
> > drivers/video/backlight/pwm_bl.c | 25 ++++++++++++-------------
> > 1 file changed, 12 insertions(+), 13 deletions(-)
>
> I'm hesitant to provide feedback on this, as I feel as though I've
> messed you around enough, however ... ;)
>
> > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > index 9ee4c1b735b2..f7799f62fea0 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -284,30 +284,29 @@ static int pwm_backlight_parse_dt(struct device *dev,
> > ret = of_property_read_u32_array(node, "brightness-levels",
> > data->levels,
> > data->max_brightness);
> > - if (ret < 0)
> > + if (!ret)
> > return ret;
> >
> > ret = of_property_read_u32(node, "default-brightness-level",
> > &value);
> > - if (ret < 0)
> > + if (!ret)
> > return ret;
>
> Just FYI (it didn't even make it to 'nit' status), this should really
> be done in a separate patch since it is unrelated to the rest of the
> patch.
Did wonder which way to go on this... I figured this close I'd accept
code either way so adopted fewest patches.
However I will split this out because I'm going to go back to the orignal
pre-v1 approach of just initializing the damn variable.
> > data->dft_brightness = value;
> >
> > /*
> > * This property is optional, if is set enables linear
> > - * interpolation between each of the values of brightness levels
> > - * and creates a new pre-computed table.
> > + * 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) {
> > + ret = of_property_read_u32(node, "num-interpolated-steps",
> > + &num_steps);
> > + if (!ret || num_steps) {
>
> Not sure if it's even possible for of_property_read_u32() to fail AND
> still populate num_steps, however this check makes it sound like that's
> okay. Is that correct?
>
> I can't help but think that this all 'just goes away' if you
> pre-initialise num_steps. I wouldn't let the "do not initialise too
> far away from the code using variable" affect this. However, if
> you're insistent, perhaps consider moving the declaration to just
> below:
>
> if (data->max_brightness > 0) {
>
> > + /*
> > + * Make sure that there are at least two entries in
> > + * the brightness-levels table, otherwise we can't
> > + * interpolate between two points.
> > + */
> > if (data->max_brightness < 2) {
> > dev_err(dev, "can't interpolate\n");
> > return -EINVAL;
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
