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