[PATCH v3 0/8] iio: light: si1133: driver cleanup

[PATCH v3 0/8] iio: light: si1133: driver cleanup

[Joshua Crofts via B4 Relay]

This series deals with the cleanup and modernization of the Silicon
Labs SI1133 UV/Ambient light sensor.

What seemed like a simple checkpatch cleanup turned out to be more
complex, therefore I've ended up doing a patch series instead.

Changes include:
 - adding missing headers
 - removing unused macros
 - adding guard(mutex)() support
 - code style fixes
 - race condition fixes reported by Sashiko

Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
Changes in v3:
- PATCH 6: add local variable for timeout
- PATCH 7 & 8: fix potential race conditions as pointed out by Sashiko
- Link to v2: https://lore.kernel.org/r/20260428-si1133-checkup-v2-0-70ad14bfefe2@gmail.com

Changes in v2:
- Change v2 to patch series
- PATCH 2: remove unused macros
- PATCH 3: add missing include headers
- PATCH 4: group generic <linux/*> headers
- PATCH 5: add guard(mutex)() support
- Link to v1: https://lore.kernel.org/r/20260427-si1133-checkup-v1-1-20f5f11eba6c@gmail.com

---
Joshua Crofts (8):
      iio: light: si1133: remove unused macros
      iio: light: si1133: prefer complex macros enclosed in parenthesis
      iio: light: si1133: add missing include headers
      iio: light: si1133: group generic <linux/*> headers
      iio: light: si1133: use guard(mutex)() macro
      iio: light: si1133: add local variable for timeout
      iio: light: si1133: reset counter to prevent race condition
      iio: light: si1133: prevent race condition on timeout

 drivers/iio/light/si1133.c | 73 ++++++++++++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 28 deletions(-)
---
base-commit: d86db1905add39f905cf9f04252804b359914ed6
change-id: 20260427-si1133-checkup-abcc5280adf3

Best regards,
-- 
Joshua Crofts <joshua.crofts1@gmail.com>

[PATCH v3 1/8] iio: light: si1133: remove unused macros

[Joshua Crofts via B4 Relay]

From: Joshua Crofts <joshua.crofts1@gmail.com>

Remove unused macros to improve readability.

No functional change.

Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/light/si1133.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/iio/light/si1133.c b/drivers/iio/light/si1133.c
index 44fa152dbd24c26c97cc778cbe641d92ecd56afa..73b0ce21e017cd4ab6c6b280229f417763866502 100644
--- a/drivers/iio/light/si1133.c
+++ b/drivers/iio/light/si1133.c
@@ -86,13 +86,9 @@
 #define SI1133_CMD_MINSLEEP_US_LOW	5000
 #define SI1133_CMD_MINSLEEP_US_HIGH	7500
 #define SI1133_CMD_TIMEOUT_MS		25
-#define SI1133_CMD_LUX_TIMEOUT_MS	5000
-#define SI1133_CMD_TIMEOUT_US		SI1133_CMD_TIMEOUT_MS * 1000
 
 #define SI1133_REG_HOSTOUT(x)		(x) + 0x13
 
-#define SI1133_MEASUREMENT_FREQUENCY 1250
-
 #define SI1133_X_ORDER_MASK            0x0070
 #define SI1133_Y_ORDER_MASK            0x0007
 #define si1133_get_x_order(m)          ((m) & SI1133_X_ORDER_MASK) >> 4

-- 
2.47.3

Re: [PATCH v3 1/8] iio: light: si1133: remove unused macros

[Andy Shevchenko]

On Wed, Apr 29, 2026 at 05:04:49PM +0200, Joshua Crofts via B4 Relay wrote:

> Remove unused macros to improve readability.

Perhaps needs elaboration that these macros a purely Linux ones and has no
(direct) relation to the HW.

> No functional change.

Suggested-by? (I am fine without that tag, up to you.)


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/8] iio: light: si1133: remove unused macros

[Joshua Crofts]

On Wed, 29 Apr 2026 at 21:14, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Apr 29, 2026 at 05:04:49PM +0200, Joshua Crofts via B4 Relay wrote:
>
> > Remove unused macros to improve readability.
>
> Perhaps needs elaboration that these macros a purely Linux ones and has no
> (direct) relation to the HW.

Sure, will add a more detailed description.

>
> Suggested-by? (I am fine without that tag, up to you.)

Credit where credit is due :)

-- 
Kind regards

CJD

[PATCH v3 2/8] iio: light: si1133: prefer complex macros enclosed in parenthesis

[Joshua Crofts via B4 Relay]

From: Joshua Crofts <joshua.crofts1@gmail.com>

Enclose complex macros in parenthesis per checkpatch.pl error to
improve code style.

Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/light/si1133.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/light/si1133.c b/drivers/iio/light/si1133.c
index 73b0ce21e017cd4ab6c6b280229f417763866502..136436c0379aa732a2c93d40b08734ae4fc2a45f 100644
--- a/drivers/iio/light/si1133.c
+++ b/drivers/iio/light/si1133.c
@@ -50,23 +50,23 @@
 #define SI1133_MAX_CMD_CTR		0xF
 
 #define SI1133_PARAM_REG_CHAN_LIST	0x01
-#define SI1133_PARAM_REG_ADCCONFIG(x)	((x) * 4) + 2
-#define SI1133_PARAM_REG_ADCSENS(x)	((x) * 4) + 3
-#define SI1133_PARAM_REG_ADCPOST(x)	((x) * 4) + 4
+#define SI1133_PARAM_REG_ADCCONFIG(x)	(((x) * 4) + 2)
+#define SI1133_PARAM_REG_ADCSENS(x)	(((x) * 4) + 3)
+#define SI1133_PARAM_REG_ADCPOST(x)	(((x) * 4) + 4)
 
 #define SI1133_ADCMUX_MASK 0x1F
 
-#define SI1133_ADCCONFIG_DECIM_RATE(x)	(x) << 5
+#define SI1133_ADCCONFIG_DECIM_RATE(x)	((x) << 5)
 
 #define SI1133_ADCSENS_SCALE_MASK 0x70
 #define SI1133_ADCSENS_SCALE_SHIFT 4
 #define SI1133_ADCSENS_HSIG_MASK BIT(7)
 #define SI1133_ADCSENS_HSIG_SHIFT 7
 #define SI1133_ADCSENS_HW_GAIN_MASK 0xF
-#define SI1133_ADCSENS_NB_MEAS(x)	fls(x) << SI1133_ADCSENS_SCALE_SHIFT
+#define SI1133_ADCSENS_NB_MEAS(x)	(fls(x) << SI1133_ADCSENS_SCALE_SHIFT)
 
 #define SI1133_ADCPOST_24BIT_EN BIT(6)
-#define SI1133_ADCPOST_POSTSHIFT_BITQTY(x) (x & GENMASK(2, 0)) << 3
+#define SI1133_ADCPOST_POSTSHIFT_BITQTY(x) (((x) & GENMASK(2, 0)) << 3)
 
 #define SI1133_PARAM_ADCMUX_SMALL_IR	0x0
 #define SI1133_PARAM_ADCMUX_MED_IR	0x1
@@ -87,11 +87,11 @@
 #define SI1133_CMD_MINSLEEP_US_HIGH	7500
 #define SI1133_CMD_TIMEOUT_MS		25
 
-#define SI1133_REG_HOSTOUT(x)		(x) + 0x13
+#define SI1133_REG_HOSTOUT(x)		((x) + 0x13)
 
 #define SI1133_X_ORDER_MASK            0x0070
 #define SI1133_Y_ORDER_MASK            0x0007
-#define si1133_get_x_order(m)          ((m) & SI1133_X_ORDER_MASK) >> 4
+#define si1133_get_x_order(m)          (((m) & SI1133_X_ORDER_MASK) >> 4)
 #define si1133_get_y_order(m)          ((m) & SI1133_Y_ORDER_MASK)
 
 #define SI1133_LUX_ADC_MASK		0xE

-- 
2.47.3

[PATCH v3 3/8] iio: light: si1133: add missing include headers

[Joshua Crofts via B4 Relay]

From: Joshua Crofts <joshua.crofts1@gmail.com>

Add missing include headers to prevent compilation relying on transient
dependencies (array_size.h, bitops.h, completion.h, dev_printk.h, err.h,
irqreturn.h, jiffies.h, math.h, mutex.h, types.h).

Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/light/si1133.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/iio/light/si1133.c b/drivers/iio/light/si1133.c
index 136436c0379aa732a2c93d40b08734ae4fc2a45f..dabb1aae05535313e77441b7d066bce931de79f9 100644
--- a/drivers/iio/light/si1133.c
+++ b/drivers/iio/light/si1133.c
@@ -6,11 +6,21 @@
  * Copyright 2018 Maxime Roussin-Belanger <maxime.roussinbelanger@gmail.com>
  */
 
+#include <linux/array_size.h>
+#include <linux/bitops.h>
+#include <linux/completion.h>
 #include <linux/delay.h>
+#include <linux/dev_printk.h>
+#include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
+#include <linux/irqreturn.h>
+#include <linux/jiffies.h>
+#include <linux/math.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/regmap.h>
+#include <linux/types.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>

-- 
2.47.3

Re: [PATCH v3 3/8] iio: light: si1133: add missing include headers

[Andy Shevchenko]

On Wed, Apr 29, 2026 at 05:04:51PM +0200, Joshua Crofts via B4 Relay wrote:

> Add missing include headers to prevent compilation relying on transient
> dependencies (array_size.h, bitops.h, completion.h, dev_printk.h, err.h,
> irqreturn.h, jiffies.h, math.h, mutex.h, types.h).

...

> +#include <linux/array_size.h>
> +#include <linux/bitops.h>
> +#include <linux/completion.h>
>  #include <linux/delay.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>

> +#include <linux/irqreturn.h>

Assumed by interrupt.h

> +#include <linux/jiffies.h>
> +#include <linux/math.h>

Also mod_devicetable.h
*Yes, most of the SPI/I²C drivers do not do that, I know.

>  #include <linux/module.h>

> +#include <linux/mutex.h>
>  #include <linux/regmap.h>
> +#include <linux/types.h>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/8] iio: light: si1133: add missing include headers

[Joshua Crofts]

On Wed, 29 Apr 2026 at 21:16, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
> > +#include <linux/irqreturn.h>
>
> Assumed by interrupt.h

Will do.

> Also mod_devicetable.h
> *Yes, most of the SPI/I涎 drivers do not do that, I know.

How did I miss mod_devicetable.h after adding it in several IWYU
patches?

-- 
Kind regards

CJD

[PATCH v3 4/8] iio: light: si1133: group generic <linux/*> headers

[Joshua Crofts via B4 Relay]

From: Joshua Crofts <joshua.crofts1@gmail.com>

Group generic <linux/*> include headers to improve code style.

No functional change.

Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/light/si1133.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/si1133.c b/drivers/iio/light/si1133.c
index dabb1aae05535313e77441b7d066bce931de79f9..f46485304877a89eff3616c62d5d2a4459bd0c5f 100644
--- a/drivers/iio/light/si1133.c
+++ b/drivers/iio/light/si1133.c
@@ -21,14 +21,12 @@
 #include <linux/mutex.h>
 #include <linux/regmap.h>
 #include <linux/types.h>
+#include <linux/unaligned.h>
+#include <linux/util_macros.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
-#include <linux/util_macros.h>
-
-#include <linux/unaligned.h>
-
 #define SI1133_REG_PART_ID		0x00
 #define SI1133_REG_REV_ID		0x01
 #define SI1133_REG_MFR_ID		0x02

-- 
2.47.3

[PATCH v3 5/8] iio: light: si1133: use guard(mutex)() macro

[Joshua Crofts via B4 Relay]

From: Joshua Crofts <joshua.crofts1@gmail.com>

Remove mutex_lock()/mutex_unlock() and goto instances and add
guard(mutex)() macro to modernize driver and improve mutex handling.

Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/light/si1133.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/light/si1133.c b/drivers/iio/light/si1133.c
index f46485304877a89eff3616c62d5d2a4459bd0c5f..939189d8c18a85ad64d8c05dfb83b5989f524791 100644
--- a/drivers/iio/light/si1133.c
+++ b/drivers/iio/light/si1133.c
@@ -8,6 +8,7 @@
 
 #include <linux/array_size.h>
 #include <linux/bitops.h>
+#include <linux/cleanup.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/dev_printk.h>
@@ -395,7 +396,7 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
 	int err;
 	int expected_seq;
 
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
 	expected_seq = (data->rsp_seq + 1) & SI1133_MAX_CMD_CTR;
 
@@ -406,19 +407,17 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
 	if (err) {
 		dev_warn(dev, "Failed to write command 0x%02x, ret=%d\n", cmd,
 			 err);
-		goto out;
+		return err;
 	}
 
 	if (cmd == SI1133_CMD_FORCE) {
 		/* wait for irq */
 		if (!wait_for_completion_timeout(&data->completion,
-			msecs_to_jiffies(SI1133_COMPLETION_TIMEOUT_MS))) {
-			err = -ETIMEDOUT;
-			goto out;
-		}
+			msecs_to_jiffies(SI1133_COMPLETION_TIMEOUT_MS)))
+			return -ETIMEDOUT;
 		err = regmap_read(data->regmap, SI1133_REG_RESPONSE0, &resp);
 		if (err)
-			goto out;
+			return err;
 	} else {
 		err = regmap_read_poll_timeout(data->regmap,
 					       SI1133_REG_RESPONSE0, resp,
@@ -431,7 +430,7 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
 			dev_warn(dev,
 				 "Failed to read command 0x%02x, ret=%d\n",
 				 cmd, err);
-			goto out;
+			return err;
 		}
 	}
 
@@ -442,9 +441,6 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
 		data->rsp_seq = expected_seq;
 	}
 
-out:
-	mutex_unlock(&data->mutex);
-
 	return err;
 }
 

-- 
2.47.3

Re: [PATCH v3 5/8] iio: light: si1133: use guard(mutex)() macro

[Andy Shevchenko]

On Wed, Apr 29, 2026 at 05:04:53PM +0200, Joshua Crofts via B4 Relay wrote:

> Remove mutex_lock()/mutex_unlock() and goto instances and add
> guard(mutex)() macro to modernize driver and improve mutex handling.

...

>  		if (!wait_for_completion_timeout(&data->completion,
> +			msecs_to_jiffies(SI1133_COMPLETION_TIMEOUT_MS)))
> +			return -ETIMEDOUT;

I believe Jonathan gave you a green light for loner line and properly indented.

		if (!wait_for_completion_timeout(&data->completion,
						 msecs_to_jiffies(SI1133_COMPLETION_TIMEOUT_MS)))

It fits 100 and hence doesn't trigger a warning in checkpatch (in default mode).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 5/8] iio: light: si1133: use guard(mutex)() macro

[Joshua Crofts]

On Wed, 29 Apr 2026 at 21:20, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Apr 29, 2026 at 05:04:53PM +0200, Joshua Crofts via B4 Relay wrote:
>
> > Remove mutex_lock()/mutex_unlock() and goto instances and add
> > guard(mutex)() macro to modernize driver and improve mutex handling.
>
> ...
>
> >               if (!wait_for_completion_timeout(&data->completion,
> > +                     msecs_to_jiffies(SI1133_COMPLETION_TIMEOUT_MS)))
> > +                     return -ETIMEDOUT;
>
> I believe Jonathan gave you a green light for loner line and properly indented.
>
>                 if (!wait_for_completion_timeout(&data->completion,
>                                                  msecs_to_jiffies(SI1133_COMPLETION_TIMEOUT_MS)))
>
> It fits 100 and hence doesn't trigger a warning in checkpatch (in default mode).

Yeah, it's more of a problem of me not putting patch 6 before this one.


-- 
Kind regards

CJD

[PATCH v3 6/8] iio: light: si1133: add local variable for timeout

[Joshua Crofts via B4 Relay]

From: Joshua Crofts <joshua.crofts1@gmail.com>

Add local variable for timeout to improve readability.

No functional change.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/light/si1133.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/si1133.c b/drivers/iio/light/si1133.c
index 939189d8c18a85ad64d8c05dfb83b5989f524791..471692057b0f16c82946507bf7d23a0004f80bf0 100644
--- a/drivers/iio/light/si1133.c
+++ b/drivers/iio/light/si1133.c
@@ -391,6 +391,7 @@ static int si1133_cmd_reset_counter(struct si1133_data *data)
 
 static int si1133_command(struct si1133_data *data, u8 cmd)
 {
+	unsigned long timeout = msecs_to_jiffies(SI1133_COMPLETION_TIMEOUT_MS);
 	struct device *dev = &data->client->dev;
 	u32 resp;
 	int err;
@@ -412,8 +413,7 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
 
 	if (cmd == SI1133_CMD_FORCE) {
 		/* wait for irq */
-		if (!wait_for_completion_timeout(&data->completion,
-			msecs_to_jiffies(SI1133_COMPLETION_TIMEOUT_MS)))
+		if (!wait_for_completion_timeout(&data->completion, timeout))
 			return -ETIMEDOUT;
 		err = regmap_read(data->regmap, SI1133_REG_RESPONSE0, &resp);
 		if (err)

-- 
2.47.3

Re: [PATCH v3 6/8] iio: light: si1133: add local variable for timeout

[Andy Shevchenko]

On Wed, Apr 29, 2026 at 05:04:54PM +0200, Joshua Crofts via B4 Relay wrote:

> Add local variable for timeout to improve readability.
> 
> No functional change.

Ah, with this it doesn't make much sense to indent properly but it makes me
think that this patch should go before that one.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 7/8] iio: light: si1133: reset counter to prevent race condition

[Joshua Crofts via B4 Relay]

From: Joshua Crofts <joshua.crofts1@gmail.com>

Sashiko reported a potential race condition happening when the driver
returns an errno after a timeout in the si1133_command() function. The
premature exit causes the hardware and software counters to become out
of sync by not updating data->rsp_seq, therefore the internal hardware
counter keeps incrementing.

Fix this by adding a call to si1133_cmd_reset_counter() before returing
from timeout.

Closes: https://sashiko.dev/#/message/20260428-si1133-checkup-v2-5-70ad14bfefe2%40gmail.com
Assisted-by: gemini:gemini-3.1-pro-preview
Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/light/si1133.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/iio/light/si1133.c b/drivers/iio/light/si1133.c
index 471692057b0f16c82946507bf7d23a0004f80bf0..842a59bc68c20b3206e0c826f86c913f6c66bd7c 100644
--- a/drivers/iio/light/si1133.c
+++ b/drivers/iio/light/si1133.c
@@ -430,6 +430,12 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
 			dev_warn(dev,
 				 "Failed to read command 0x%02x, ret=%d\n",
 				 cmd, err);
+			/*
+			 * reset counter on err to prevent sofware and hardware
+			 * counters being out of sync
+			 */
+			si1133_cmd_reset_counter(data);
+
 			return err;
 		}
 	}

-- 
2.47.3

Re: [PATCH v3 7/8] iio: light: si1133: reset counter to prevent race condition

[Andy Shevchenko]

On Wed, Apr 29, 2026 at 05:04:55PM +0200, Joshua Crofts via B4 Relay wrote:

> Sashiko reported a potential race condition happening when the driver
> returns an errno after a timeout in the si1133_command() function. The
> premature exit causes the hardware and software counters to become out
> of sync by not updating data->rsp_seq, therefore the internal hardware
> counter keeps incrementing.
> 
> Fix this by adding a call to si1133_cmd_reset_counter() before returing
> from timeout.
> 
> Closes: https://sashiko.dev/#/message/20260428-si1133-checkup-v2-5-70ad14bfefe2%40gmail.com
> Assisted-by: gemini:gemini-3.1-pro-preview

Reported-by: sashiko-bot@... // there is an email mentioned in some thread in
			     // linux-iio mailing list.

,,,

> +			/*
> +			 * reset counter on err to prevent sofware and hardware
> +			 * counters being out of sync
> +			 */

/*
 * Respect English grammar and punctuation. This is
 * the style for multi-line comments. You can consider
 * this example.
 */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 7/8] iio: light: si1133: reset counter to prevent race condition

[Joshua Crofts]

On Wed, 29 Apr 2026 at 21:24, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
> > +                     /*
> > +                      * reset counter on err to prevent sofware and hardware
> > +                      * counters being out of sync
> > +                      */
>
> /*
>  * Respect English grammar and punctuation. This is
>  * the style for multi-line comments. You can consider
>  * this example.
>  */

See patch 8.

-- 
Kind regards

CJD

[PATCH v3 8/8] iio: light: si1133: prevent race condition on timeout

[Joshua Crofts via B4 Relay]

From: Joshua Crofts <joshua.crofts1@gmail.com>

Sashiko reported a bug where the si1133_command exits on timeout
without halting the sensor or masking the interrupt. If the sensor
completes the command later, any subsequent command to the sensor
will cause the IRQ handler to complete immediately, returning stale
data to the driver all while the command hasn't finished yet, shifting
all potential reads in the future.

Fix this by masking the IRQ if wait_for_completion_timeout() fails.
When initiating a new command, do a dummy read of the IRQ_STATUS
register and turn the IRQ back on.

Closes: https://sashiko.dev/#/message/20260428-si1133-checkup-v2-5-70ad14bfefe2%40gmail.com
Assisted-by: gemini:gemini-3.1-pro-preview
Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/light/si1133.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/si1133.c b/drivers/iio/light/si1133.c
index 842a59bc68c20b3206e0c826f86c913f6c66bd7c..91c90f2873ae053eb7cbc8c9c79ce6d91a1ec9dc 100644
--- a/drivers/iio/light/si1133.c
+++ b/drivers/iio/light/si1133.c
@@ -401,8 +401,14 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
 
 	expected_seq = (data->rsp_seq + 1) & SI1133_MAX_CMD_CTR;
 
-	if (cmd == SI1133_CMD_FORCE)
+	if (cmd == SI1133_CMD_FORCE) {
+		/* Flush pending IRQs from a previous timeout. */
+		regmap_read(data->regmap, SI1133_REG_IRQ_STATUS, &resp);
+		regmap_write(data->regmap, SI1133_REG_IRQ_ENABLE,
+			     SI1133_IRQ_CHANNEL_ENABLE);
+
 		reinit_completion(&data->completion);
+	}
 
 	err = regmap_write(data->regmap, SI1133_REG_COMMAND, cmd);
 	if (err) {
@@ -413,8 +419,13 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
 
 	if (cmd == SI1133_CMD_FORCE) {
 		/* wait for irq */
-		if (!wait_for_completion_timeout(&data->completion, timeout))
+		if (!wait_for_completion_timeout(&data->completion, timeout)) {
+			/* Mask the IRQ to prevent delayed interrupt waking up
+			 * any subsequent command.
+			 */
+			regmap_write(data->regmap, SI1133_REG_IRQ_ENABLE, 0);
 			return -ETIMEDOUT;
+		}
 		err = regmap_read(data->regmap, SI1133_REG_RESPONSE0, &resp);
 		if (err)
 			return err;
@@ -431,8 +442,8 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
 				 "Failed to read command 0x%02x, ret=%d\n",
 				 cmd, err);
 			/*
-			 * reset counter on err to prevent sofware and hardware
-			 * counters being out of sync
+			 * Reset counter on err to prevent sofware and hardware
+			 * counters being out of sync.
 			 */
 			si1133_cmd_reset_counter(data);
 

-- 
2.47.3

Re: [PATCH v3 8/8] iio: light: si1133: prevent race condition on timeout

[Andy Shevchenko]

On Wed, Apr 29, 2026 at 05:04:56PM +0200, Joshua Crofts via B4 Relay wrote:

> Sashiko reported a bug where the si1133_command exits on timeout
> without halting the sensor or masking the interrupt. If the sensor
> completes the command later, any subsequent command to the sensor
> will cause the IRQ handler to complete immediately, returning stale
> data to the driver all while the command hasn't finished yet, shifting
> all potential reads in the future.
> 
> Fix this by masking the IRQ if wait_for_completion_timeout() fails.
> When initiating a new command, do a dummy read of the IRQ_STATUS
> register and turn the IRQ back on.

> Closes: https://sashiko.dev/#/message/20260428-si1133-checkup-v2-5-70ad14bfefe2%40gmail.com
> Assisted-by: gemini:gemini-3.1-pro-preview
> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>

Reported-by?

Also with this seems the Fixes tag is appropriate (and in the other patch).
Also note, fixes should go first in the series.

...

> -		if (!wait_for_completion_timeout(&data->completion, timeout))
> +		if (!wait_for_completion_timeout(&data->completion, timeout)) {
> +			/* Mask the IRQ to prevent delayed interrupt waking up
> +			 * any subsequent command.
> +			 */
> +			regmap_write(data->regmap, SI1133_REG_IRQ_ENABLE, 0);
>  			return -ETIMEDOUT;
> +		}

Seems like you dropped {} and reinstantiated them. It's not good style. Just
make sure that the first patch that does something leaves it in the form that
next patches won't have too many + or - lines that basically revert previous
subchanges. I call this style ping-pong and highly discourage from using it.

...

>  			/*

> -			 * reset counter on err to prevent sofware and hardware
> -			 * counters being out of sync
> +			 * Reset counter on err to prevent sofware and hardware
> +			 * counters being out of sync.
>  			 */

Ha-ha, doesn't belong to this change (see also previous reply).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 8/8] iio: light: si1133: prevent race condition on timeout

[Joshua Crofts]

On Wed, 29 Apr 2026 at 21:27, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
> Reported-by?
>
> Also with this seems the Fixes tag is appropriate (and in the other patch).
> Also note, fixes should go first in the series.

Yeah, makes sense.

> ...
>
> > -             if (!wait_for_completion_timeout(&data->completion, timeout))
> > +             if (!wait_for_completion_timeout(&data->completion, timeout)) {
> > +                     /* Mask the IRQ to prevent delayed interrupt waking up
> > +                      * any subsequent command.
> > +                      */
> > +                     regmap_write(data->regmap, SI1133_REG_IRQ_ENABLE, 0);
> >                       return -ETIMEDOUT;
> > +             }
>
> Seems like you dropped {} and reinstantiated them. It's not good style. Just
> make sure that the first patch that does something leaves it in the form that
> next patches won't have too many + or - lines that basically revert previous
> subchanges. I call this style ping-pong and highly discourage from using it.

Agreed, I seem to have missed this.

>
> >                       /*
>
> > -                      * reset counter on err to prevent sofware and hardware
> > -                      * counters being out of sync
> > +                      * Reset counter on err to prevent sofware and hardware
> > +                      * counters being out of sync.
> >                        */
>
> Ha-ha, doesn't belong to this change (see also previous reply).

I noticed this immediately after sending the series :((( I must've edited
it while rebasing...

-- 
Kind regards

CJD

Re: [PATCH v3 0/8] iio: light: si1133: driver cleanup

[Andy Shevchenko]

On Wed, Apr 29, 2026 at 05:04:48PM +0200, Joshua Crofts via B4 Relay wrote:
> This series deals with the cleanup and modernization of the Silicon
> Labs SI1133 UV/Ambient light sensor.
> 
> What seemed like a simple checkpatch cleanup turned out to be more
> complex, therefore I've ended up doing a patch series instead.

In general LGTM, but there are several nit-picks that needs to be addressed,
I hope the v4 will be final.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/8] iio: light: si1133: driver cleanup

[Joshua Crofts]

On Wed, 29 Apr 2026 at 21:28, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Apr 29, 2026 at 05:04:48PM +0200, Joshua Crofts via B4 Relay wrote:
> > This series deals with the cleanup and modernization of the Silicon
> > Labs SI1133 UV/Ambient light sensor.
> >
> > What seemed like a simple checkpatch cleanup turned out to be more
> > complex, therefore I've ended up doing a patch series instead.
>
> In general LGTM, but there are several nit-picks that needs to be addressed,
> I hope the v4 will be final.

Not really nit-picks, the majority of your comments stem from me ordering
my patches incorrectly, it would shorten review time.

-- 
Kind regards

CJD