| Message ID | 20180719161923.21510-1-daniel.thompson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@toradex.com> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > --- > > 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. > 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;
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@toradex.com> > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > --- > > > > 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. > > > 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? No, it's bogus. Looks like when I broke the if statement into two clauses I ended up flipping the && to an ||. > 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
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@toradex.com> > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > --- > > > > 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
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; 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) { + /* + * 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;
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@toradex.com> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> --- 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(-) -- 2.17.1