Re: [PATCH v1 1/4] iio: light: opt4001: Fix read-modify-write of wrong register in power down

[Jonathan Cameron]

On Mon, 13 Jul 2026 01:54:48 +0530
Nikhil Gautam <nikhilgtr@xxxxxxxxx> wrote:

> opt4001_power_down() intends to clear the operating mode bits in the
> CTRL register but reads OPT4001_DEVICE_ID instead of OPT4001_CTRL.
> The device ID value with the mode bits masked out is then written to
> CTRL, corrupting the conversion time and fault count configuration.
> 
> Read the CTRL register instead so only the operating mode bits are
> cleared and the rest of the configuration is preserved.
> 
> Signed-off-by: Nikhil Gautam <nikhilgtr@xxxxxxxxx>

Fixes tag please (you asked in the cover letter and yes it is appropriate
even though these go all the way back).

Looks correct but I think we can take this opportunity to simplify
the code whilst fixing the issue.

> ---
>  drivers/iio/light/opt4001.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/opt4001.c b/drivers/iio/light/opt4001.c
> index ba4eb82d9bc2..f2cf496cc243 100644
> --- a/drivers/iio/light/opt4001.c
> +++ b/drivers/iio/light/opt4001.c
> @@ -228,7 +228,7 @@ static int opt4001_power_down(struct opt4001_chip *chip)
>  	int ret;
>  	unsigned int reg;
>  
> -	ret = regmap_read(chip->regmap, OPT4001_DEVICE_ID, &reg);
> +	ret = regmap_read(chip->regmap, OPT4001_CTRL, &reg);

Let us jump directly to a regmap_clear_bits()

It will fix this issue and generally improve the code readabilty.

Given this is only called in one place and the code will be nearly a one liner
after that change, lets also just  move it directly into opt3001_chip_power_off_action().

I my opinion all that is just about fine in a single fix patch, rather than a
separate fix + code cleanup to follow, but if you would rather split it up into
fix first then refactor I don't mind.  I may well decide to mere this whole series
for the next merge window anyway just to ensure it all makes it reasonably quickly
(at expensive of the fixes making it in a little faster and possibly delaying any
cleanup).

Thanks,

Jonathan


>  	if (ret) {
>  		dev_err(dev, "Failed to read configuration\n");
>  		return ret;