Re: [PATCH v1] drivers:adp8870_bl: check the return value of adp8870_write

[Daniel Thompson <daniel.thompson@linaro.org>]

On Wed, Sep 21, 2022 at 02:50:49PM -0700, Li Zhong wrote:
> Subject: [PATCH v1] drivers:adp8870_bl: check the return value of
> adp8870_write
                     ^^^^^^^^^^^^^^^^^^^^

Should be backlight: adp8870_bl.

> Check and propagate the return value of adp8870_write() when it fails,
> which is possible when SMBus writing byte fails.

This looks like a sensible change, however...

When writing patches like this please review the whole file for similar
concerns and fix all instances of the same issue. In this case there is another
unchecked call to adp8870_write() in
adp8870_bl_ambient_light_zone_store() (this function also contains
other unchecked calls).

Note that the unchecked use in adp8870_led_work() because there is no
way to propogate the error from this function (and adp8870_write()
already logged the error).

It would also be good to review and fix adp8860_bl.c at the same time
since these drivers are very similar (ideally the identical code in
these drivers should be factored out).


> Signed-off-by: Li Zhong <floridsleeves@gmail.com>
> ---
>  drivers/video/backlight/adp8870_bl.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/backlight/adp8870_bl.c b/drivers/video/backlight/adp8870_bl.c
> index 8b5213a39527..0eb4ae2ff592 100644
> --- a/drivers/video/backlight/adp8870_bl.c
> +++ b/drivers/video/backlight/adp8870_bl.c
> @@ -567,9 +567,13 @@ static ssize_t adp8870_store(struct device *dev, const char *buf,
>  		return ret;
>
>  	mutex_lock(&data->lock);
> -	adp8870_write(data->client, reg, val);
> +	ret = adp8870_write(data->client, reg, val);
>  	mutex_unlock(&data->lock);
>
> +	if (ret) {
> +		return ret;
> +	}

No need for braces here.

> +
>  	return count;
>  }
>

Thanks


Daniel.