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

[PATCH v3 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").

Patch 1 fixes the power down path, which read the device ID register
instead of the control register in a read-modify-write, 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(). No functional impact.

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.

Changes in v3:
- Patch 4: replaced tabs by spaces as suggested by Andy

Changes in v2:
- Add Fixes: tags to all patches
- Patch 1: use regmap_clear_bits() directly in the opt4001_chip_off_action
  and drop opt4001_power_down() (Jonathan)
- Patch 2: document that there is no functional impact (Jonathan)
- Patch 3: add blank line after the early return (Jonathan)
- Patch 4: fix whitespace alignment of the new define

Regards,
Nikhil

Nikhil Gautam (4):
  iio: light: opt4001: Fix power down clearing bits of the wrong
    register
  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 | 37 +++++++++++--------------------------
 1 file changed, 11 insertions(+), 26 deletions(-)

-- 
2.39.5

[PATCH v3 1/4] iio: light: opt4001: Fix power down clearing bits of the wrong register

[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, so
the value written back to CTRL contains device ID bits rather than the
current configuration.

Fix and simplify this by using regmap_clear_bits() on the CTRL register
directly in the devm action, and drop opt4001_power_down() which has no
other users.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Fixes: 9a9608418292 ("iio: light: Add support for TI OPT4001 light sensor")
Signed-off-by: Nikhil Gautam <nikhilgtr@gmail.com>
---
 drivers/iio/light/opt4001.c | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/light/opt4001.c b/drivers/iio/light/opt4001.c
index ba4eb82d9bc2..66563000d081 100644
--- a/drivers/iio/light/opt4001.c
+++ b/drivers/iio/light/opt4001.c
@@ -222,33 +222,14 @@ static int opt4001_set_conf(struct opt4001_chip *chip)
 	return ret;
 }
 
-static int opt4001_power_down(struct opt4001_chip *chip)
-{
-	struct device *dev = &chip->client->dev;
-	int ret;
-	unsigned int reg;
-
-	ret = regmap_read(chip->regmap, OPT4001_DEVICE_ID, &reg);
-	if (ret) {
-		dev_err(dev, "Failed to read configuration\n");
-		return ret;
-	}
-
-	/* MODE_OFF is 0x0 so just set bits to 0 */
-	reg &= ~OPT4001_CTRL_OPER_MODE_MASK;
-
-	ret = regmap_write(chip->regmap, OPT4001_CTRL, reg);
-	if (ret)
-		dev_err(dev, "Failed to set configuration to power down\n");
-
-	return ret;
-}
-
 static void opt4001_chip_off_action(void *data)
 {
 	struct opt4001_chip *chip = data;
+	int ret;
 
-	opt4001_power_down(chip);
+	ret = regmap_clear_bits(chip->regmap, OPT4001_CTRL, OPT4001_CTRL_OPER_MODE_MASK);
+	if (ret)
+		dev_err(&chip->client->dev, "Failed to power down\n");
 }
 
 static const struct iio_chan_spec opt4001_channels[] = {
-- 
2.39.5

[PATCH v3 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 *. There is no functional impact as int and u32 have the same
size and representation on all supported architectures and the remainder
is always smaller than the divisor, so it fits in the positive range of
int. Fix the type mismatch by using a local u32 for the remainder and
assigning the result to *val2.

Fixes: 9a9608418292 ("iio: light: Add support for TI OPT4001 light sensor")
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 66563000d081..a0f174888384 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

[PATCH v3 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.

Fixes: 9a9608418292 ("iio: light: Add support for TI OPT4001 light sensor")
Signed-off-by: Nikhil Gautam <nikhilgtr@gmail.com>
---
 drivers/iio/light/opt4001.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/light/opt4001.c b/drivers/iio/light/opt4001.c
index a0f174888384..bb4be6384e4c 100644
--- a/drivers/iio/light/opt4001.c
+++ b/drivers/iio/light/opt4001.c
@@ -269,6 +269,9 @@ 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

[PATCH v3 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.

Fixes: 9a9608418292 ("iio: light: Add support for TI OPT4001 light sensor")
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 bb4be6384e4c..731eb8d7326a 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 v3 0/4] iio: light: opt4001: Fixes from code review

[Andy Shevchenko]

On Tue, Jul 14, 2026 at 05:01:30PM +0530, Nikhil Gautam wrote:
> 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").
> 
> Patch 1 fixes the power down path, which read the device ID register
> instead of the control register in a read-modify-write, 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(). No functional impact.
> 
> 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.

LGTM now, FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

-- 
With Best Regards,
Andy Shevchenko