[PATCH 0/4] iio: light: opt4001: Fixes from code review

[PATCH 0/4] iio: light: opt4001: Fixes from code review

[Nikhil Gautam]

Hi,

This series fixes a number of issues found while reviewing the opt4001
driver. All of them date back to the initial submission of the driver
in commit 9a9608418292 ("iio: light: Add support for TI OPT4001 light
sensor"). I have not added Fixes: tags; happy to resend with them if
preferred.

Patch 1 fixes a read-modify-write in opt4001_power_down() that reads
the device ID register instead of the control register, corrupting the
CTRL configuration on the way to power down.

Patch 2 fixes an incompatible pointer type passed as the remainder
argument to div_u64_rem(), which takes a u32 * rather than an int *.

Patch 3 makes opt4001_write_raw() reject integration time writes with
a non-zero integer (seconds) part, which were previously accepted
silently based on the microseconds part alone.

Patch 4 fixes reversed GENMASK() arguments in the (currently unused)
fault count mask definition and adds the _MASK suffix for consistency
with the neighbouring defines.

Datasheet: https://www.ti.com/lit/ds/symlink/opt4001.pdf

Thanks,
Nikhil Gautam

Nikhil Gautam (4):
  iio: light: opt4001: Fix read-modify-write of wrong register in power
    down
  iio: light: opt4001: Fix incompatible pointer type passed to
    div_u64_rem()
  iio: light: opt4001: Reject integration times with a non-zero seconds
    part
  iio: light: opt4001: Fix reversed GENMASK() arguments in fault count
    mask

 drivers/iio/light/opt4001.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

-- 
2.39.5

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

[Nikhil Gautam]

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@gmail.com>
---
 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);
 	if (ret) {
 		dev_err(dev, "Failed to read configuration\n");
 		return ret;
-- 
2.39.5

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@gmail.com> 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@gmail.com>

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;

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

[Nikhil Gautam]

On 13-07-2026 06:12 am, Jonathan Cameron wrote:
> On Mon, 13 Jul 2026 01:54:48 +0530
> Nikhil Gautam <nikhilgtr@gmail.com> 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@gmail.com>
> 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.
Sure, will add Fixes tags.
>> ---
>>   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
>
Agreed, that is much cleaner. I will move the regmap_clear_bits() call
directly into the devm action and drop opt4001_power_down() as a single
fix patch in v2, with the Fixes tag added.

Thanks,
Nikhil

[PATCH v1 2/4] iio: light: opt4001: Fix incompatible pointer type passed to div_u64_rem()

[Nikhil Gautam]

div_u64_rem() takes a u32 * for the remainder but is passed val2,
which is an int *. Use a local u32 for the remainder and assign the
result to *val2.

Signed-off-by: Nikhil Gautam <nikhilgtr@gmail.com>
---
 drivers/iio/light/opt4001.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/opt4001.c b/drivers/iio/light/opt4001.c
index f2cf496cc243..bdb1eadbd450 100644
--- a/drivers/iio/light/opt4001.c
+++ b/drivers/iio/light/opt4001.c
@@ -173,6 +173,7 @@ static int opt4001_read_lux_value(struct iio_dev *indio_dev,
 	u8 crc;
 	u8 calc_crc;
 	u64 lux_raw;
+	u32 rem;
 	int ret;
 
 	ret = regmap_read(chip->regmap, OPT4001_LIGHT1_MSB, &light1);
@@ -199,8 +200,8 @@ static int opt4001_read_lux_value(struct iio_dev *indio_dev,
 
 	lux_raw = lux_raw << exp;
 	lux_raw = lux_raw * chip->chip_info->mul;
-	*val = div_u64_rem(lux_raw, chip->chip_info->div, val2);
-	*val2 = *val2 * 100;
+	*val = div_u64_rem(lux_raw, chip->chip_info->div, &rem);
+	*val2 = rem * 100;
 
 	return IIO_VAL_INT_PLUS_NANO;
 }
-- 
2.39.5

Re: [PATCH v1 2/4] iio: light: opt4001: Fix incompatible pointer type passed to div_u64_rem()

[Jonathan Cameron]

On Mon, 13 Jul 2026 01:54:49 +0530
Nikhil Gautam <nikhilgtr@gmail.com> wrote:

> div_u64_rem() takes a u32 * for the remainder but is passed val2,
> which is an int *. Use a local u32 for the remainder and assign the
> result to *val2.
State what affect (if any) that has. 
> 
> Signed-off-by: Nikhil Gautam <nikhilgtr@gmail.com>
Fix looks good, just add the tag for v2.

> ---
>  drivers/iio/light/opt4001.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/opt4001.c b/drivers/iio/light/opt4001.c
> index f2cf496cc243..bdb1eadbd450 100644
> --- a/drivers/iio/light/opt4001.c
> +++ b/drivers/iio/light/opt4001.c
> @@ -173,6 +173,7 @@ static int opt4001_read_lux_value(struct iio_dev *indio_dev,
>  	u8 crc;
>  	u8 calc_crc;
>  	u64 lux_raw;
> +	u32 rem;
>  	int ret;
>  
>  	ret = regmap_read(chip->regmap, OPT4001_LIGHT1_MSB, &light1);
> @@ -199,8 +200,8 @@ static int opt4001_read_lux_value(struct iio_dev *indio_dev,
>  
>  	lux_raw = lux_raw << exp;
>  	lux_raw = lux_raw * chip->chip_info->mul;
> -	*val = div_u64_rem(lux_raw, chip->chip_info->div, val2);
> -	*val2 = *val2 * 100;
> +	*val = div_u64_rem(lux_raw, chip->chip_info->div, &rem);
> +	*val2 = rem * 100;
>  
>  	return IIO_VAL_INT_PLUS_NANO;
>  }


L

Re: [PATCH v1 2/4] iio: light: opt4001: Fix incompatible pointer type passed to div_u64_rem()

[Nikhil Gautam]

On 13-07-2026 06:13 am, Jonathan Cameron wrote:
> On Mon, 13 Jul 2026 01:54:49 +0530
> Nikhil Gautam <nikhilgtr@gmail.com> wrote:
>
>> div_u64_rem() takes a u32 * for the remainder but is passed val2,
>> which is an int *. Use a local u32 for the remainder and assign the
>> result to *val2.
> State what affect (if any) that has.
There is no functional impact, int and u32 have the same size and
representation on all supported architectures, and the remainder is
always smaller than the divisor (10000000) so it fits in the positive
range of int.  I will state this in the commit message and add the tag 
in v2.
>> Signed-off-by: Nikhil Gautam <nikhilgtr@gmail.com>
> Fix looks good, just add the tag for v2.
Sure, will add Fixes tags to every patch.
>> ---
>>   drivers/iio/light/opt4001.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
...
>>   	return IIO_VAL_INT_PLUS_NANO;
>>   }
>>
>> L
Thanks for the quick review.

Regards,
Nikhil

[PATCH v1 3/4] iio: light: opt4001: Reject integration times with a non-zero seconds part

[Nikhil Gautam]

opt4001_write_raw() only looks at val2 when setting the integration
time, so a write such as 1.000600 is silently accepted as 600 us.
Return -EINVAL if val is non-zero.

Signed-off-by: Nikhil Gautam <nikhilgtr@gmail.com>
---
 drivers/iio/light/opt4001.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/light/opt4001.c b/drivers/iio/light/opt4001.c
index bdb1eadbd450..31a5397926eb 100644
--- a/drivers/iio/light/opt4001.c
+++ b/drivers/iio/light/opt4001.c
@@ -288,6 +288,8 @@ static int opt4001_write_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_INT_TIME:
+		if (val)
+			return -EINVAL;
 		int_time = opt4001_als_time_to_index(val2);
 		if (int_time < 0)
 			return int_time;
-- 
2.39.5

Re: [PATCH v1 3/4] iio: light: opt4001: Reject integration times with a non-zero seconds part

[Jonathan Cameron]

On Mon, 13 Jul 2026 01:54:50 +0530
Nikhil Gautam <nikhilgtr@gmail.com> wrote:

> opt4001_write_raw() only looks at val2 when setting the integration
> time, so a write such as 1.000600 is silently accepted as 600 us.
> Return -EINVAL if val is non-zero.
> 
> Signed-off-by: Nikhil Gautam <nikhilgtr@gmail.com>
Fixes tag definitely makes sense for this one.  Good spot.

> ---
>  drivers/iio/light/opt4001.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/light/opt4001.c b/drivers/iio/light/opt4001.c
> index bdb1eadbd450..31a5397926eb 100644
> --- a/drivers/iio/light/opt4001.c
> +++ b/drivers/iio/light/opt4001.c
> @@ -288,6 +288,8 @@ static int opt4001_write_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_INT_TIME:
> +		if (val)
> +			return -EINVAL;
Blank line here.

Otherwise lgtm.

>  		int_time = opt4001_als_time_to_index(val2);
>  		if (int_time < 0)
>  			return int_time;

[PATCH v1 4/4] iio: light: opt4001: Fix reversed GENMASK() arguments in fault count mask

[Nikhil Gautam]

GENMASK(h, l) requires h >= l, but OPT4001_CTRL_FAULT_COUNT is defined
as GENMASK(0, 1). The define is currently unused so there is no
functional impact, but fix it before anyone builds on it, and add the
_MASK suffix for consistency with the neighbouring definitions.

Signed-off-by: Nikhil Gautam <nikhilgtr@gmail.com>
---
 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 31a5397926eb..294532bcbc50 100644
--- a/drivers/iio/light/opt4001.c
+++ b/drivers/iio/light/opt4001.c
@@ -39,7 +39,7 @@
 #define OPT4001_CTRL_OPER_MODE_MASK      GENMASK(5, 4)
 #define OPT4001_CTRL_LATCH_MASK          GENMASK(3, 3)
 #define OPT4001_CTRL_INT_POL_MASK        GENMASK(2, 2)
-#define OPT4001_CTRL_FAULT_COUNT         GENMASK(0, 1)
+#define OPT4001_CTRL_FAULT_COUNT_MASK	 GENMASK(1, 0)
 
 /* OPT4001 constants */
 #define OPT4001_DEVICE_ID_VAL            0x121
-- 
2.39.5

Re: [PATCH v1 4/4] iio: light: opt4001: Fix reversed GENMASK() arguments in fault count mask

[Jonathan Cameron]

On Mon, 13 Jul 2026 01:54:51 +0530
Nikhil Gautam <nikhilgtr@gmail.com> wrote:

> GENMASK(h, l) requires h >= l, but OPT4001_CTRL_FAULT_COUNT is defined
> as GENMASK(0, 1). The define is currently unused so there is no
> functional impact, but fix it before anyone builds on it, and add the
> _MASK suffix for consistency with the neighbouring definitions.
> 
> Signed-off-by: Nikhil Gautam <nikhilgtr@gmail.com>
Another good spot.   I guess it is just about worth a fixes tag as
someone might add support that needs this then backport that missing this
fix otherwise.

Jonathan

> ---
>  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 31a5397926eb..294532bcbc50 100644
> --- a/drivers/iio/light/opt4001.c
> +++ b/drivers/iio/light/opt4001.c
> @@ -39,7 +39,7 @@
>  #define OPT4001_CTRL_OPER_MODE_MASK      GENMASK(5, 4)
>  #define OPT4001_CTRL_LATCH_MASK          GENMASK(3, 3)
>  #define OPT4001_CTRL_INT_POL_MASK        GENMASK(2, 2)
> -#define OPT4001_CTRL_FAULT_COUNT         GENMASK(0, 1)
> +#define OPT4001_CTRL_FAULT_COUNT_MASK	 GENMASK(1, 0)
>  
>  /* OPT4001 constants */
>  #define OPT4001_DEVICE_ID_VAL            0x121

Re: [PATCH v1 4/4] iio: light: opt4001: Fix reversed GENMASK() arguments in fault count mask

[Andy Shevchenko]

On Mon, Jul 13, 2026 at 01:54:51AM +0530, Nikhil Gautam wrote:
> GENMASK(h, l) requires h >= l, but OPT4001_CTRL_FAULT_COUNT is defined
> as GENMASK(0, 1). The define is currently unused so there is no
> functional impact, but fix it before anyone builds on it, and add the
> _MASK suffix for consistency with the neighbouring definitions.

...

>  #define OPT4001_CTRL_OPER_MODE_MASK      GENMASK(5, 4)
>  #define OPT4001_CTRL_LATCH_MASK          GENMASK(3, 3)
>  #define OPT4001_CTRL_INT_POL_MASK        GENMASK(2, 2)
> -#define OPT4001_CTRL_FAULT_COUNT         GENMASK(0, 1)
> +#define OPT4001_CTRL_FAULT_COUNT_MASK	 GENMASK(1, 0)

The original indentation uses spaces only, please keep it the same style. Yeah,
it's supposed to use tabs, but here we are (it might be the material for another
change in the future).

-- 
With Best Regards,
Andy Shevchenko