[v2] backlight: pwm_bl: Fix uninitialized variable

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

--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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