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