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@toradex.com Signed-off-by: Daniel Thompson daniel.thompson@linaro.org Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com Tested-by: Marcel Ziswiler marcel.ziswiler@toradex.com --- 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) { + /* + * Make sure that there is 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; -- 2.17.1
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@toradex.com Signed-off-by: Daniel Thompson daniel.thompson@linaro.org Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
This line is confusing. Did you guys author this patch 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?
Tested-by: Marcel Ziswiler marcel.ziswiler@toradex.com
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); 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.
/** 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?
* between two points.*/ if (data->max_brightness < 2) { dev_err(dev, "can't interpolate\n"); return -EINVAL;
On Wed, 18 Jul 2018, 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@toradex.com Signed-off-by: Daniel Thompson daniel.thompson@linaro.org Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
This line is confusing. Did you guys author this patch 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?
Tested-by: Marcel Ziswiler marcel.ziswiler@toradex.com
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); if (!ret && num_steps) {
Whoops! I was playing around with the 80-char limit and forgot to revert. The lines should read:
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.
/** Make sure that there is at least two entries in thes/is/are/
* brightness-levels table, otherwise we can't* interpolateWhy break the line here?
* between two points.*/ if (data->max_brightness < 2) { dev_err(dev, "can't interpolate\n"); return -EINVAL;
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@toradex.com Signed-off-by: Daniel Thompson daniel.thompson@linaro.org Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
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@toradex.com
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 inthe
* brightness-levels table, otherwise we can'tinterpolate
* 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 twoentries in the
s/is/are/
* brightness-levels table, otherwise wecan't
* interpolateWhy 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'tinterpolate\n"); return -EINVAL;
On Wed, 18 Jul 2018, Marcel Ziswiler wrote:
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@toradex.com Signed-off-by: Daniel Thompson daniel.thompson@linaro.org Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
This line is confusing. Did you guys author this patch together?
Yes, I reported it and we came to a conclusion together.
It sounds like you need to have all of the tags (except this one). :)
Reported-by: for reporting the issue Suggested-by: for suggesting a resolution Acked-by: for reviewing it Tested-by: for testing it
Signed-off-by usually means you either wrote a significant amount of the diffstat or you were part of the submission path.
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.
In this instance I suggest keeping Reported-by and Tested-by.
Tested-by: Marcel Ziswiler marcel.ziswiler@toradex.com
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 inthe
* brightness-levels table, otherwise we can'tinterpolate
* 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.
I also think num_steps should be pre-initialised.
Then it will only be set if of_property_read_u32() succeeds.
On Wed, Jul 18, 2018 at 10:53:35AM +0100, Lee Jones wrote:
On Wed, 18 Jul 2018, Marcel Ziswiler wrote:
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@toradex.com Signed-off-by: Daniel Thompson daniel.thompson@linaro.org Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
This line is confusing. Did you guys author this patch together?
Yes, I reported it and we came to a conclusion together.
It sounds like you need to have all of the tags (except this one). :)
Reported-by: for reporting the issue Suggested-by: for suggesting a resolution Acked-by: for reviewing it Tested-by: for testing it
Signed-off-by usually means you either wrote a significant amount of the diffstat or you were part of the submission path.
He did [I don't object to but wouldn't have used the extra brackets you brought up ;-) ].
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.
In this instance I suggest keeping Reported-by and Tested-by.
Tested-by: Marcel Ziswiler marcel.ziswiler@toradex.com
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 inthe
* brightness-levels table, otherwise we can'tinterpolate
* 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.
I also think num_steps should be pre-initialised.
Then it will only be set if of_property_read_u32() succeeds.
-- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Wed, 2018-07-18 at 11:12 +0100, Daniel Thompson wrote:
On Wed, Jul 18, 2018 at 10:53:35AM +0100, Lee Jones wrote:
On Wed, 18 Jul 2018, Marcel Ziswiler wrote:
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@toradex.com Signed-off-by: Daniel Thompson daniel.thompson@linaro.org Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
This line is confusing. Did you guys author this patch together?
Yes, I reported it and we came to a conclusion together.
It sounds like you need to have all of the tags (except this one). :)
Reported-by: for reporting the issue Suggested-by: for suggesting a resolution Acked-by: for reviewing it Tested-by: for testing it
Signed-off-by usually means you either wrote a significant amount of the diffstat or you were part of the submission path.
He did [I don't object to but wouldn't have used the extra brackets you brought up ;-) ].
Yes, I take all the blame for the extra brackets. Regardless of having a masters in CS or not I still prefer too many then too few of them (;- p).
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.
In this instance I suggest keeping Reported-by and Tested-by.
Tested-by: Marcel Ziswiler marcel.ziswiler@toradex.com
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 twoentries in the
* brightness-levels table, otherwise wecan'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.
I also think num_steps should be pre-initialised.
Yes, I guess it definitely does not hurt.
Then it will only be set if of_property_read_u32() succeeds.
Yes, but we still need to check for both, the function not failing and num_steps to actually be non zero.
-- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Wed, 18 Jul 2018, Marcel Ziswiler wrote:
On Wed, 2018-07-18 at 11:12 +0100, Daniel Thompson wrote:
On Wed, Jul 18, 2018 at 10:53:35AM +0100, Lee Jones wrote:
On Wed, 18 Jul 2018, Marcel Ziswiler wrote:
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@toradex.com Signed-off-by: Daniel Thompson daniel.thompson@linaro.org Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
This line is confusing. Did you guys author this patch together?
Yes, I reported it and we came to a conclusion together.
It sounds like you need to have all of the tags (except this one). :)
Reported-by: for reporting the issue Suggested-by: for suggesting a resolution Acked-by: for reviewing it Tested-by: for testing it
Signed-off-by usually means you either wrote a significant amount of the diffstat or you were part of the submission path.
He did [I don't object to but wouldn't have used the extra brackets you brought up ;-) ].
Yes, I take all the blame for the extra brackets. Regardless of having a masters in CS or not I still prefer too many then too few of them (;- p).
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.
In this instance I suggest keeping Reported-by and Tested-by.
Tested-by: Marcel Ziswiler marcel.ziswiler@toradex.com
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 twoentries in the
* brightness-levels table, otherwise wecan'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.
I also think num_steps should be pre-initialised.
Yes, I guess it definitely does not hurt.
Then it will only be set if of_property_read_u32() succeeds.
Yes, but we still need to check for both, the function not failing and num_steps to actually be non zero.
Why? You don't do anything differently if it fails.
On Wed, 2018-07-18 at 14:08 +0100, Lee Jones wrote:
On Wed, 18 Jul 2018, Marcel Ziswiler wrote:
On Wed, 2018-07-18 at 11:12 +0100, Daniel Thompson wrote:
On Wed, Jul 18, 2018 at 10:53:35AM +0100, Lee Jones wrote:
On Wed, 18 Jul 2018, Marcel Ziswiler wrote:
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@toradex.com > > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.or > g> > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.c > om>
This line is confusing. Did you guys author this patch together?
Yes, I reported it and we came to a conclusion together.
It sounds like you need to have all of the tags (except this one). :)
Reported-by: for reporting the issue Suggested-by: for suggesting a resolution Acked-by: for reviewing it Tested-by: for testing it
Signed-off-by usually means you either wrote a significant amount of the diffstat or you were part of the submission path.
He did [I don't object to but wouldn't have used the extra brackets you brought up ;-) ].
Yes, I take all the blame for the extra brackets. Regardless of having a masters in CS or not I still prefer too many then too few of them (;- p).
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.
In this instance I suggest keeping Reported-by and Tested-by.
> Tested-by: Marcel Ziswiler marcel.ziswiler@toradex.com > --- > 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.
I also think num_steps should be pre-initialised.
Yes, I guess it definitely does not hurt.
Then it will only be set if of_property_read_u32() succeeds.
Yes, but we still need to check for both, the function not failing and num_steps to actually be non zero.
Why? You don't do anything differently if it fails.
Well, maybe we should but given this being an optional property nobody cared.
On Wed, Jul 18, 2018 at 02:08:53PM +0100, Lee Jones wrote:
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.
I also think num_steps should be pre-initialised.
Yes, I guess it definitely does not hurt.
Then it will only be set if of_property_read_u32() succeeds.
Yes, but we still need to check for both, the function not failing and num_steps to actually be non zero.
Why? You don't do anything differently if it fails.
Only if you initialize num_steps...
We should either initialize to zero and not worry about the return code[1] or we check the return code and not worry about initialization[2]. I don't think both are worthwhile.
Whilst initialization can fix this specific instance we generally avoid overusing it since it messes up static analysis and, in this instance, distance from declaration to use is >25 lines, hence current patchset.
Daniel.
[1] https://lkml.org/lkml/2018/7/16/399 [2] https://lkml.org/lkml/2018/7/16/1042
Or...
We check the return code and leave number
num_steps is uninitialized and stack allocated so it only has a valid value if of_property_read_u32() succeeds.
We can (and I originally did) fix the bug by initializing num_steps to 0 but its quite some distance between declaration and use so I accepted Marcel's counter proposal to check the return code instead.
Daniel.
On Wed, 18 Jul 2018, Daniel Thompson wrote:
On Wed, Jul 18, 2018 at 02:08:53PM +0100, Lee Jones wrote:
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.
I also think num_steps should be pre-initialised.
Yes, I guess it definitely does not hurt.
Then it will only be set if of_property_read_u32() succeeds.
Yes, but we still need to check for both, the function not failing and num_steps to actually be non zero.
Why? You don't do anything differently if it fails.
Only if you initialize num_steps...
We should either initialize to zero and not worry about the return code[1] or we check the return code and not worry about initialization[2]. I don't think both are worthwhile.
Whilst initialization can fix this specific instance we generally avoid overusing it since it messes up static analysis and, in this instance, distance from declaration to use is >25 lines, hence current patchset.
Daniel.
[1] https://lkml.org/lkml/2018/7/16/399 [2] https://lkml.org/lkml/2018/7/16/1042
Or...
We check the return code and leave number
num_steps is uninitialized and stack allocated so it only has a valid value if of_property_read_u32() succeeds.
We can (and I originally did) fix the bug by initializing num_steps to 0 but its quite some distance between declaration and use so I accepted Marcel's counter proposal to check the return code instead.
Only checking the return value of of_property_read_u32() is also suitable.
On Wed, Jul 18, 2018 at 04:55:44PM +0100, Lee Jones wrote:
On Wed, 18 Jul 2018, Daniel Thompson wrote:
On Wed, Jul 18, 2018 at 02:08:53PM +0100, Lee Jones wrote:
> 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.
I also think num_steps should be pre-initialised.
Yes, I guess it definitely does not hurt.
Then it will only be set if of_property_read_u32() succeeds.
Yes, but we still need to check for both, the function not failing and num_steps to actually be non zero.
Why? You don't do anything differently if it fails.
Only if you initialize num_steps...
We should either initialize to zero and not worry about the return code[1] or we check the return code and not worry about initialization[2]. I don't think both are worthwhile.
Whilst initialization can fix this specific instance we generally avoid overusing it since it messes up static analysis and, in this instance, distance from declaration to use is >25 lines, hence current patchset.
Daniel.
[1] https://lkml.org/lkml/2018/7/16/399 [2] https://lkml.org/lkml/2018/7/16/1042
Or...
We check the return code and leave number
num_steps is uninitialized and stack allocated so it only has a valid value if of_property_read_u32() succeeds.
We can (and I originally did) fix the bug by initializing num_steps to 0 but its quite some distance between declaration and use so I accepted Marcel's counter proposal to check the return code instead.
Only checking the return value of of_property_read_u32() is also suitable.
I did think about that case... I concluded that it isn't wrong for a DT to set to this property to 0 (effectively meaning "no interpolated steps please").
If we take the branch when num_steps is zero we get a bunch of pointless housekeeping that amounts to no more than an extremely elaborate malloc/memcpy/free.
Daniel.
On Wed, 18 Jul 2018, Daniel Thompson wrote:
On Wed, Jul 18, 2018 at 04:55:44PM +0100, Lee Jones wrote:
On Wed, 18 Jul 2018, Daniel Thompson wrote:
On Wed, Jul 18, 2018 at 02:08:53PM +0100, Lee Jones wrote:
> > 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. > > I also think num_steps should be pre-initialised.
Yes, I guess it definitely does not hurt.
> Then it will only be set if of_property_read_u32() succeeds.
Yes, but we still need to check for both, the function not failing and num_steps to actually be non zero.
Why? You don't do anything differently if it fails.
Only if you initialize num_steps...
We should either initialize to zero and not worry about the return code[1] or we check the return code and not worry about initialization[2]. I don't think both are worthwhile.
Whilst initialization can fix this specific instance we generally avoid overusing it since it messes up static analysis and, in this instance, distance from declaration to use is >25 lines, hence current patchset.
Daniel.
[1] https://lkml.org/lkml/2018/7/16/399 [2] https://lkml.org/lkml/2018/7/16/1042
Or...
We check the return code and leave number
num_steps is uninitialized and stack allocated so it only has a valid value if of_property_read_u32() succeeds.
We can (and I originally did) fix the bug by initializing num_steps to 0 but its quite some distance between declaration and use so I accepted Marcel's counter proposal to check the return code instead.
Only checking the return value of of_property_read_u32() is also suitable.
I did think about that case... I concluded that it isn't wrong for a DT to set to this property to 0 (effectively meaning "no interpolated steps please").
If we take the branch when num_steps is zero we get a bunch of pointless housekeeping that amounts to no more than an extremely elaborate malloc/memcpy/free.
Yet in the latest patch, you do it anyway? Or have I misread it?
On Wed, Jul 18, 2018 at 09:09:13AM +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@toradex.com Signed-off-by: Daniel Thompson daniel.thompson@linaro.org Signed-off-by: Marcel Ziswiler marcel.ziswiler@toradex.com
This line is confusing. Did you guys author this patch together?
Yes (although the manipulations were fairly mechanical).
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?
Tested-by: Marcel Ziswiler marcel.ziswiler@toradex.com
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); 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.
/** Make sure that there is at least two entries in thes/is/are/
* brightness-levels table, otherwise we can't* interpolateWhy break the line here?
* 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
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(-)
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; -- 2.17.1
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
Currently, if the DT does not define num-interpolated-steps then num_steps is undefined and the interpolation code will deploy randomly. Fix with a simple initialize to zero.
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: v3: - Switch to the simplest fix and zero the uninitialized variable. git grep indicates that ~25% of calls to of_property_read_u32() use this pattern (pre-initialize optional properties with sane values and ignore the return code).
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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 9ee4c1b735b2..bdfcc0a71db1 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct device *dev, struct device_node *node = dev->of_node; unsigned int num_levels = 0; unsigned int levels_count; - unsigned int num_steps; + unsigned int num_steps = 0; struct property *prop; unsigned int *table; int length; -- 2.17.1
Hi,
On Tue, Jul 24, 2018 at 12:12 AM, Daniel Thompson daniel.thompson@linaro.org wrote:
Currently, if the DT does not define num-interpolated-steps then num_steps is undefined and the interpolation code will deploy randomly. Fix with a simple initialize to zero.
Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation between brightness-levels")
I think it may be incorrect to word-wrap the Fixes line. A quick grep through the source code looks like others agree.
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: v3: - Switch to the simplest fix and zero the uninitialized variable. git grep indicates that ~25% of calls to of_property_read_u32() use this pattern (pre-initialize optional properties with sane values and ignore the return code).
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 errordrivers/video/backlight/pwm_bl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Other than the nit on the commit message, this simple fix seems sane.
Reviewed-by: Douglas Anderson dianders@chromium.org
On Tue, 24 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 with a simple initialize to zero.
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: v3: - Switch to the simplest fix and zero the uninitialized variable. git grep indicates that ~25% of calls to of_property_read_u32() use this pattern (pre-initialize optional properties with sane values and ignore the return code).
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 errordrivers/video/backlight/pwm_bl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Once the 'Fixes:' line has been repaired:
Acked-by: Lee Jones lee.jones@linaro.org
Currently, if the DT does not define num-interpolated-steps then num_steps is undefined and the interpolation code will deploy randomly. Fix with a simple initialize to zero.
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 Reviewed-by: Douglas Anderson dianders@chromium.org Acked-by: Lee Jones lee.jones@linaro.org ---
Notes: v4: - Remove line break from Fixes: and update the *-by:s
v3: - Switch to the simplest fix and zero the uninitialized variable. git grep indicates that ~25% of calls to of_property_read_u32() use this pattern (pre-initialize optional properties with sane values and ignore the return code).
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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 9ee4c1b735b2..bdfcc0a71db1 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct device *dev, struct device_node *node = dev->of_node; unsigned int num_levels = 0; unsigned int levels_count; - unsigned int num_steps; + unsigned int num_steps = 0; struct property *prop; unsigned int *table; int length; -- 2.17.1
On Wed, 25 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 with a simple initialize to zero.
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 Reviewed-by: Douglas Anderson dianders@chromium.org Acked-by: Lee Jones lee.jones@linaro.org
Notes: v4: - Remove line break from Fixes: and update the *-by:s
v3: - Switch to the simplest fix and zero the uninitialized variable. git grep indicates that ~25% of calls to of_property_read_u32() use this pattern (pre-initialize optional properties with sane values and ignore the return code). 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 errordrivers/video/backlight/pwm_bl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Applied, thanks.
dri-devel@lists.freedesktop.org
-
Daniel Thompson -
Doug Anderson -
Lee Jones -
Marcel Ziswiler