On Wed, 18 Jul 2018, Marcel Ziswiler wrote:

On Wed, 2018-07-18 at 11:12 +0100, Daniel Thompson wrote:

quoted

On Wed, Jul 18, 2018 at 10:53:35AM +0100, Lee Jones wrote:

quoted

On Wed, 18 Jul 2018, Marcel Ziswiler wrote:

quoted

On Wed, 2018-07-18 at 09:09 +0100, Lee Jones wrote:

quoted

On Mon, 16 Jul 2018, Daniel Thompson wrote:

quoted

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 <redacted>
Signed-off-by: Daniel Thompson <redacted>
Signed-off-by: Marcel Ziswiler <redacted>

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).

quoted

quoted

quoted

quoted

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.

quoted

quoted

quoted

Tested-by: Marcel Ziswiler <redacted>
---
 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);

quoted

		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.

quoted

quoted

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.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog