[PATCH 0/3] iio: light: tcs3472: devm conversion, wait time, locking cleanup

[PATCH 0/3] iio: light: tcs3472: devm conversion, wait time, locking cleanup

[Aldo Conte]

This series of changes modernizes the tcs3472 driver by completing the
devm conversion and updating the locking style to use an automatic
guard (mutex). Furthermore, it implement control of the WAIT state,
resolving a TODO.

Patch1: Converts the driver to use device-managed resource allocation.
This removes the need for an explicit remove() callback, since cleanup
is now handled automatically. It also adds a new function called
tcs3472_powerdown_action() that powers down the chip when the driver is 
unloaded, removed or when probe fails after the chip has been enabled.

Patch2: adds support for the wait time resolving the old TODO comment.
The user can control the WTIME indirectly by writing to the
sampling_frequency attribute. Changing the sampling
frequency attribute results in a change to the wtime while preserving
the current integration time.
Similarly, if the user has previously set a sampling frequency and
then changes the integration time, the driver re-computes WTIME so the 
previous frequency is preserved. The patch also handles the deactivation
of wtime when the requested frequency is very high, as well as the
extension of wtime to its maximum values (by enabling the use of WLONG)
when the requested frequency is very low.

Patch3: converts the remaining mutex_lock()/mutex_unlock() pairs to
guard(mutex).

All patches have been tested on a Raspberry Pi 3B with a TCS3472
connected to I2C-1 at address 0x29. Sampling frequency and integration
time changes have been exercised checking the value of WTIME and
consequently WEN and WLONG. Raw RGBC reads, calibscale, integration_time
and threshold events continue to work as before.

Aldo Conte (3):
  iio: light: tcs3472: use devm for resource management
  iio: light: tcs3472: implement wait time and sampling frequency
  iio: light: tcs3472: Use guard(mutex)() family over manual locking

 drivers/iio/light/tcs3472.c | 287 ++++++++++++++++++++++++++----------
 1 file changed, 206 insertions(+), 81 deletions(-)

-- 
2.54.0

[PATCH 1/3] iio: light: tcs3472: use devm for resource management

[Aldo Conte]

Found by code inspection.

Convert the driver to use device-managed resource allocation:
- Add tcs3472_powerdown_action() and register it with
  devm_add_action_or_reset() to ensure the device is powered down on
  cleanup. Before this patch, the chip remained powered if probe
  failed after enabling it.
- Replace iio_triggered_buffer_setup() with
  devm_iio_triggered_buffer_setup().
- Replace request_threaded_irq() with devm_request_threaded_irq().
- Replace iio_device_register() with devm_iio_device_register().
- Remove tcs3472_remove() as all cleanup is now handled by devm.

Compiled with `make drivers/iio/light/tcs3472.o W=1`.

Tested on Raspberry Pi 3B with TCS3472 (Adafruit breakout):
Verified that RGBC channel reads return valid data and that ENABLE
register is cleared to 0x00 on device removal.

Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
 drivers/iio/light/tcs3472.c | 85 ++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 49 deletions(-)

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index 12429a3261b3..6d37a473c420 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -440,6 +440,28 @@ static const struct iio_info tcs3472_info = {
 	.attrs = &tcs3472_attribute_group,
 };
 
+static int tcs3472_powerdown(struct tcs3472_data *data)
+{
+	int ret;
+	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
+
+	mutex_lock(&data->lock);
+
+	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
+					data->enable & ~enable_mask);
+	if (!ret)
+		data->enable &= ~enable_mask;
+
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static void tcs3472_powerdown_action(void *data)
+{
+	tcs3472_powerdown(data);
+}
+
 static int tcs3472_probe(struct i2c_client *client)
 {
 	struct tcs3472_data *data;
@@ -510,61 +532,27 @@ static int tcs3472_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
-	ret = iio_triggered_buffer_setup(indio_dev, NULL,
-		tcs3472_trigger_handler, NULL);
+	ret = devm_add_action_or_reset(&client->dev,
+				       tcs3472_powerdown_action, data);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
+					      tcs3472_trigger_handler, NULL);
 	if (ret < 0)
 		return ret;
 
 	if (client->irq) {
-		ret = request_threaded_irq(client->irq, NULL,
-					   tcs3472_event_handler,
-					   IRQF_TRIGGER_FALLING | IRQF_SHARED |
-					   IRQF_ONESHOT,
-					   client->name, indio_dev);
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+						NULL, tcs3472_event_handler,
+						IRQF_TRIGGER_FALLING | IRQF_SHARED |
+						IRQF_ONESHOT,
+						client->name, indio_dev);
 		if (ret)
-			goto buffer_cleanup;
+			return ret;
 	}
 
-	ret = iio_device_register(indio_dev);
-	if (ret < 0)
-		goto free_irq;
-
-	return 0;
-
-free_irq:
-	if (client->irq)
-		free_irq(client->irq, indio_dev);
-buffer_cleanup:
-	iio_triggered_buffer_cleanup(indio_dev);
-	return ret;
-}
-
-static int tcs3472_powerdown(struct tcs3472_data *data)
-{
-	int ret;
-	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
-
-	mutex_lock(&data->lock);
-
-	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
-		data->enable & ~enable_mask);
-	if (!ret)
-		data->enable &= ~enable_mask;
-
-	mutex_unlock(&data->lock);
-
-	return ret;
-}
-
-static void tcs3472_remove(struct i2c_client *client)
-{
-	struct iio_dev *indio_dev = i2c_get_clientdata(client);
-
-	iio_device_unregister(indio_dev);
-	if (client->irq)
-		free_irq(client->irq, indio_dev);
-	iio_triggered_buffer_cleanup(indio_dev);
-	tcs3472_powerdown(iio_priv(indio_dev));
+	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
 static int tcs3472_suspend(struct device *dev)
@@ -608,7 +596,6 @@ static struct i2c_driver tcs3472_driver = {
 		.pm	= pm_sleep_ptr(&tcs3472_pm_ops),
 	},
 	.probe		= tcs3472_probe,
-	.remove		= tcs3472_remove,
 	.id_table	= tcs3472_id,
 };
 module_i2c_driver(tcs3472_driver);
-- 
2.54.0

Re: [PATCH 1/3] iio: light: tcs3472: use devm for resource management

[Andy Shevchenko]

On Wed, May 06, 2026 at 11:43:09AM +0200, Aldo Conte wrote:

> Found by code inspection.

It's unusual to start a commit message with this...
Move it to the end of the commit message (before tag block).

> Convert the driver to use device-managed resource allocation:
> - Add tcs3472_powerdown_action() and register it with
>   devm_add_action_or_reset() to ensure the device is powered down on
>   cleanup. Before this patch, the chip remained powered if probe
>   failed after enabling it.
> - Replace iio_triggered_buffer_setup() with
>   devm_iio_triggered_buffer_setup().
> - Replace request_threaded_irq() with devm_request_threaded_irq().
> - Replace iio_device_register() with devm_iio_device_register().
> - Remove tcs3472_remove() as all cleanup is now handled by devm.

> Compiled with `make drivers/iio/light/tcs3472.o W=1`.

Unneeded detail, move to the comments / cover letter.

> Tested on Raspberry Pi 3B with TCS3472 (Adafruit breakout):
> Verified that RGBC channel reads return valid data and that ENABLE
> register is cleared to 0x00 on device removal.

Ditto.

> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
> ---

FYI: Here is the location for the comments / changelog / et cetera.

>  drivers/iio/light/tcs3472.c | 85 ++++++++++++++++---------------------

...

> +static int tcs3472_powerdown(struct tcs3472_data *data)
> +{
> +	int ret;
> +	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
> +
> +	mutex_lock(&data->lock);
> +
> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> +					data->enable & ~enable_mask);
> +	if (!ret)
> +		data->enable &= ~enable_mask;

This patter is discouraged.

> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}

Make the guard()() patch to be first in the series. With that being done and
taking the above this becomes as simple as

static int tcs3472_powerdown(struct tcs3472_data *data)
{
	u8 value = data->enable & ~(TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON);
	int ret;

	guard(mutex)(&data->lock);

	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
	if (ret)
		return ret;

	data->enable = value;
	return 0;
}

...

> +	ret = devm_add_action_or_reset(&client->dev,

With temporary

	struct device *dev = &cliend->dev;

at the top of the function this and others become easier to read.

> +				       tcs3472_powerdown_action, data);
> +	if (ret)
> +		return ret;

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/3] iio: light: tcs3472: implement wait time and sampling frequency

[Aldo Conte]

The TCS3472 has a wait state controlled by the WEN bit in the ENABLE
register and the WAIT register, with ad additional WLONG bit in CONFIG
that if set multiplies the wait step by 12. The driver previously
defined TCS3472_WTIME but never used it leaving the TODO comment on
the top of the source file.

Implement sampling frequency control via IIO_CHAN_INFO_SAMP_FREQ:

- Reading sampling_frequancy computes the chip's actual cycle time as
  the sum of ATIME, RGBC initialization time (that is fixed) and the
  WAIT time that depends on WLONG and WEN bits. WEN is used to enable
  the chip to consider the WAIT state (without which it would proceed
  directly to the rgbc init state).

- Writing sampling_frequency programs WTIME accordingly with current
  ATIME in order to obtain a cycle period that approximates as close as
  possible the requested frequency.
   - If the requested frequency is too
     low (and so the cycle is too large) the WLONG bit is asserted.
   - If the requested frequency is too high to be reached with a
     non-zero wait time, WEN is disabled so that wtime_us becomes 0 and
     conversions run back-to-back at the maximum rate accordingly with
     ATIME.

- The user's last requested frequency is saved in the driver's
  private data in order to use it when a new integration time (ATIME)
  is requested: when ATIME changes, the wait time is recalculated to
  ensure that the previous requested frequency is ashered to as closely
  as possible.

- The cycle time computation respects the WEN and WLONG for the
  evaluation of wtime_us.

- Concurrent access to driver's private data into the
  tcs3472_set_sampling_freq function is protected by the guard(mutex).

Tested on a Raspberry Pi 3B with a TCS3472 breakout at I2C address
0x29, by writing values to `sampling_frequency` and veifying the
reported value via `cat sampling_frequency`, and checking that changing
`integration_time` preserves the previusly requested sampling frequency.

Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
 drivers/iio/light/tcs3472.c | 171 ++++++++++++++++++++++++++++++++++--
 1 file changed, 162 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index 6d37a473c420..de51eb61f42a 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -9,14 +9,14 @@
  * TCS34727)
  *
  * Datasheet: http://ams.com/eng/content/download/319364/1117183/file/TCS3472_Datasheet_EN_v2.pdf
- *
- * TODO: wait time
  */
 
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/delay.h>
 #include <linux/pm.h>
+#include <linux/math64.h>
+#include <linux/cleanup.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -51,9 +51,11 @@
 #define TCS3472_STATUS_AINT BIT(4)
 #define TCS3472_STATUS_AVALID BIT(0)
 #define TCS3472_ENABLE_AIEN BIT(4)
+#define TCS3472_ENABLE_WEN BIT(3)
 #define TCS3472_ENABLE_AEN BIT(1)
 #define TCS3472_ENABLE_PON BIT(0)
 #define TCS3472_CONTROL_AGAIN_MASK (BIT(0) | BIT(1))
+#define TCS3472_CONFIG_WLONG BIT(1)
 
 struct tcs3472_data {
 	struct i2c_client *client;
@@ -64,6 +66,10 @@ struct tcs3472_data {
 	u8 control;
 	u8 atime;
 	u8 apers;
+	u8 wtime;
+	bool wlong;
+	int target_freq_hz;
+	int target_freq_uhz;
 };
 
 static const struct iio_event_spec tcs3472_events[] = {
@@ -89,6 +95,7 @@ static const struct iio_event_spec tcs3472_events[] = {
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_CALIBSCALE) | \
 		BIT(IIO_CHAN_INFO_INT_TIME), \
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
 	.channel2 = IIO_MOD_LIGHT_##_color, \
 	.address = _addr, \
 	.scan_index = _si, \
@@ -112,6 +119,22 @@ static const struct iio_chan_spec tcs3472_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(4),
 };
 
+static unsigned int tcs3472_cycle_time_us(struct tcs3472_data *data)
+{
+	unsigned int atime_us = (256 - data->atime) * 2400;
+	unsigned int init_us = 2400;
+	unsigned int wtime_us;
+
+	if (!(data->enable & TCS3472_ENABLE_WEN))
+		wtime_us = 0;
+	else if (data->wlong)
+		wtime_us = (256 - data->wtime) * 28800;
+	else
+		wtime_us = (256 - data->wtime) * 2400;
+
+	return wtime_us + init_us + atime_us;
+}
+
 static int tcs3472_req_data(struct tcs3472_data *data)
 {
 	int tries = 50;
@@ -164,16 +187,118 @@ static int tcs3472_read_raw(struct iio_dev *indio_dev,
 		*val = 0;
 		*val2 = (256 - data->atime) * 2400;
 		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_SAMP_FREQ: {
+		unsigned int cycle_us = tcs3472_cycle_time_us(data);
+
+		*val = USEC_PER_SEC / cycle_us;
+		*val2 = (u64)(USEC_PER_SEC % cycle_us) * USEC_PER_SEC / cycle_us;
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
 	}
 	return -EINVAL;
 }
 
+static int tcs3472_set_sampling_freq(struct tcs3472_data *data,
+				     int val, int val2)
+{
+	unsigned int atime_us = (256 - data->atime) * 2400;
+	unsigned int init_us = 2400;
+	u64 cycle_us;
+	s64 wait_us;
+	int wtime;
+	bool wlong = false;
+	int ret;
+
+	if (val < 0 || (val == 0 && val2 <= 0))
+		return -EINVAL;
+
+	guard(mutex)(&data->lock);
+
+	cycle_us = div_u64((u64)USEC_PER_SEC * USEC_PER_SEC,
+			   (u64)val * USEC_PER_SEC + val2);
+
+	wait_us = (s64)cycle_us - init_us - atime_us;
+
+	/* Wait state is not needed.
+	 * Requested frequency is too high to be reached with any
+	 * non-zero wait time. Disable WEN so the chip runs at the
+	 * maximum rate allowed by ATIME alone.
+	 */
+	if (wait_us < 2400) {
+		if (data->enable & TCS3472_ENABLE_WEN) {
+			data->enable &= ~TCS3472_ENABLE_WEN;
+			ret = i2c_smbus_write_byte_data(data->client,
+							TCS3472_ENABLE,
+							data->enable);
+			if (ret < 0)
+				return ret;
+		}
+
+		data->target_freq_hz = val;
+		data->target_freq_uhz = val2;
+		return 0;
+	}
+
+	/*
+	 * Wait state is needed: make sure WEN is active before
+	 * programming WTIME (and possibly WLONG).
+	 */
+	if (!(data->enable & TCS3472_ENABLE_WEN)) {
+		data->enable |= TCS3472_ENABLE_WEN;
+		ret = i2c_smbus_write_byte_data(data->client,
+						TCS3472_ENABLE,
+						data->enable);
+		if (ret < 0)
+			return ret;
+	}
+
+	wtime = 256 - (int)DIV_ROUND_CLOSEST((unsigned long)wait_us, 2400);
+	if (wtime < 0) {
+		wlong = true;
+		wtime = 256 - (int)DIV_ROUND_CLOSEST((unsigned long)wait_us,
+						     28800);
+		if (wtime < 0)
+			wtime = 0;
+	}
+	if (wtime > 255)
+		wtime = 255;
+
+	if (wlong != data->wlong) {
+		ret = i2c_smbus_read_byte_data(data->client, TCS3472_CONFIG);
+		if (ret < 0)
+			return ret;
+
+		if (wlong)
+			ret |= TCS3472_CONFIG_WLONG;
+		else
+			ret &= ~TCS3472_CONFIG_WLONG;
+
+		ret = i2c_smbus_write_byte_data(data->client,
+						TCS3472_CONFIG, ret);
+		if (ret < 0)
+			return ret;
+
+		data->wlong = wlong;
+	}
+
+	ret = i2c_smbus_write_byte_data(data->client, TCS3472_WTIME, wtime);
+	if (ret < 0)
+		return ret;
+
+	data->wtime = wtime;
+	data->target_freq_hz = val;
+	data->target_freq_uhz = val2;
+
+	return 0;
+}
+
 static int tcs3472_write_raw(struct iio_dev *indio_dev,
 			       struct iio_chan_spec const *chan,
 			       int val, int val2, long mask)
 {
 	struct tcs3472_data *data = iio_priv(indio_dev);
 	int i;
+	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_CALIBSCALE:
@@ -195,13 +320,22 @@ static int tcs3472_write_raw(struct iio_dev *indio_dev,
 		for (i = 0; i < 256; i++) {
 			if (val2 == (256 - i) * 2400) {
 				data->atime = i;
-				return i2c_smbus_write_byte_data(
-					data->client, TCS3472_ATIME,
-					data->atime);
+				ret = i2c_smbus_write_byte_data(data->client,
+								TCS3472_ATIME,
+								data->atime);
+				if (ret < 0)
+					return ret;
+				/* since ATIME has changed, recalculate
+				 * WTIME to maintain sampling freq
+				 */
+				return tcs3472_set_sampling_freq(data,
+								 data->target_freq_hz,
+								 data->target_freq_uhz);
 			}
-
 		}
 		return -EINVAL;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return tcs3472_set_sampling_freq(data, val, val2);
 	}
 	return -EINVAL;
 }
@@ -443,7 +577,8 @@ static const struct iio_info tcs3472_info = {
 static int tcs3472_powerdown(struct tcs3472_data *data)
 {
 	int ret;
-	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
+	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON |
+			TCS3472_ENABLE_WEN;
 
 	mutex_lock(&data->lock);
 
@@ -466,6 +601,7 @@ static int tcs3472_probe(struct i2c_client *client)
 {
 	struct tcs3472_data *data;
 	struct iio_dev *indio_dev;
+	unsigned int cycle_us;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
@@ -504,6 +640,16 @@ static int tcs3472_probe(struct i2c_client *client)
 		return ret;
 	data->atime = ret;
 
+	ret = i2c_smbus_read_byte_data(data->client, TCS3472_WTIME);
+	if (ret < 0)
+		return ret;
+	data->wtime = ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, TCS3472_CONFIG);
+	if (ret < 0)
+		return ret;
+	data->wlong = ret & TCS3472_CONFIG_WLONG;
+
 	ret = i2c_smbus_read_word_data(data->client, TCS3472_AILT);
 	if (ret < 0)
 		return ret;
@@ -525,13 +671,19 @@ static int tcs3472_probe(struct i2c_client *client)
 		return ret;
 
 	/* enable device */
-	data->enable = ret | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN;
+	data->enable = ret | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN |
+			TCS3472_ENABLE_WEN;
 	data->enable &= ~TCS3472_ENABLE_AIEN;
 	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
 		data->enable);
 	if (ret < 0)
 		return ret;
 
+	cycle_us = tcs3472_cycle_time_us(data);
+	data->target_freq_hz = USEC_PER_SEC / cycle_us;
+	data->target_freq_uhz = (u64)(USEC_PER_SEC % cycle_us) *
+				USEC_PER_SEC / cycle_us;
+
 	ret = devm_add_action_or_reset(&client->dev,
 				       tcs3472_powerdown_action, data);
 	if (ret)
@@ -567,7 +719,8 @@ static int tcs3472_resume(struct device *dev)
 	struct tcs3472_data *data = iio_priv(i2c_get_clientdata(
 		to_i2c_client(dev)));
 	int ret;
-	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
+	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON |
+			TCS3472_ENABLE_WEN;
 
 	mutex_lock(&data->lock);
 
-- 
2.54.0

Re: [PATCH 2/3] iio: light: tcs3472: implement wait time and sampling frequency

[Andy Shevchenko]

On Wed, May 06, 2026 at 11:43:10AM +0200, Aldo Conte wrote:
> The TCS3472 has a wait state controlled by the WEN bit in the ENABLE
> register and the WAIT register, with ad additional WLONG bit in CONFIG
> that if set multiplies the wait step by 12. The driver previously
> defined TCS3472_WTIME but never used it leaving the TODO comment on
> the top of the source file.
> 
> Implement sampling frequency control via IIO_CHAN_INFO_SAMP_FREQ:
> 
> - Reading sampling_frequancy computes the chip's actual cycle time as
>   the sum of ATIME, RGBC initialization time (that is fixed) and the
>   WAIT time that depends on WLONG and WEN bits. WEN is used to enable
>   the chip to consider the WAIT state (without which it would proceed
>   directly to the rgbc init state).
> 
> - Writing sampling_frequency programs WTIME accordingly with current
>   ATIME in order to obtain a cycle period that approximates as close as
>   possible the requested frequency.
>    - If the requested frequency is too
>      low (and so the cycle is too large) the WLONG bit is asserted.
>    - If the requested frequency is too high to be reached with a
>      non-zero wait time, WEN is disabled so that wtime_us becomes 0 and
>      conversions run back-to-back at the maximum rate accordingly with
>      ATIME.
> 
> - The user's last requested frequency is saved in the driver's
>   private data in order to use it when a new integration time (ATIME)
>   is requested: when ATIME changes, the wait time is recalculated to
>   ensure that the previous requested frequency is ashered to as closely
>   as possible.
> 
> - The cycle time computation respects the WEN and WLONG for the
>   evaluation of wtime_us.
> 
> - Concurrent access to driver's private data into the
>   tcs3472_set_sampling_freq function is protected by the guard(mutex).

> Tested on a Raspberry Pi 3B with a TCS3472 breakout at I2C address
> 0x29, by writing values to `sampling_frequency` and veifying the
> reported value via `cat sampling_frequency`, and checking that changing
> `integration_time` preserves the previusly requested sampling frequency.

No noise in the commit message.

...

>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/delay.h>
>  #include <linux/pm.h>
> +#include <linux/math64.h>
> +#include <linux/cleanup.h>

Please, add another patch that sorts headers alphabetically first.

...

>  struct tcs3472_data {
>  	struct i2c_client *client;

>  	u8 control;
>  	u8 atime;
>  	u8 apers;
> +	u8 wtime;
> +	bool wlong;
> +	int target_freq_hz;
> +	int target_freq_uhz;
>  };

Does `pahole` agree with the proposed layout?

...

> +static unsigned int tcs3472_cycle_time_us(struct tcs3472_data *data)
> +{
> +	unsigned int atime_us = (256 - data->atime) * 2400;
> +	unsigned int init_us = 2400;
> +	unsigned int wtime_us;
> +
> +	if (!(data->enable & TCS3472_ENABLE_WEN))
> +		wtime_us = 0;
> +	else if (data->wlong)
> +		wtime_us = (256 - data->wtime) * 28800;
> +	else
> +		wtime_us = (256 - data->wtime) * 2400;

This is the same as atime_us.

> +	return wtime_us + init_us + atime_us;
> +}

...

> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		unsigned int cycle_us = tcs3472_cycle_time_us(data);
> +
> +		*val = USEC_PER_SEC / cycle_us;
> +		*val2 = (u64)(USEC_PER_SEC % cycle_us) * USEC_PER_SEC / cycle_us;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
>  	}
>  	return -EINVAL;

To avoid confusion add another patch to move this kind of standalone return:s
to the default case of the respective switch-cases.

...

> +static int tcs3472_set_sampling_freq(struct tcs3472_data *data,
> +				     int val, int val2)
> +{
> +	unsigned int atime_us = (256 - data->atime) * 2400;
> +	unsigned int init_us = 2400;
> +	u64 cycle_us;
> +	s64 wait_us;
> +	int wtime;
> +	bool wlong = false;
> +	int ret;
> +
> +	if (val < 0 || (val == 0 && val2 <= 0))
> +		return -EINVAL;

What if val > 0 && val2 < 0?

> +	guard(mutex)(&data->lock);

> +	cycle_us = div_u64((u64)USEC_PER_SEC * USEC_PER_SEC,

This is strange. One of the multiplier either should be MEGA / MICRO or this to
be PSEC_PER_SEC.

> +			   (u64)val * USEC_PER_SEC + val2);

include/linux/math64.h:116: * div_u64 - unsigned 64bit divide with 32bit divisor

> +	wait_us = (s64)cycle_us - init_us - atime_us;

So how wait_us can be negative. Can you add a comment explaining the cases when
it's possible and what we are going to do about this?

> +	/* Wait state is not needed.
> +	 * Requested frequency is too high to be reached with any
> +	 * non-zero wait time. Disable WEN so the chip runs at the
> +	 * maximum rate allowed by ATIME alone.
> +	 */

/*
 * Wrong multi-line comment style. Use this
 * example on how to do correctly.
 */

> +	if (wait_us < 2400) {
> +		if (data->enable & TCS3472_ENABLE_WEN) {
> +			data->enable &= ~TCS3472_ENABLE_WEN;
> +			ret = i2c_smbus_write_byte_data(data->client,
> +							TCS3472_ENABLE,
> +							data->enable);
> +			if (ret < 0)
> +				return ret;
> +		}
> +
> +		data->target_freq_hz = val;
> +		data->target_freq_uhz = val2;
> +		return 0;
> +	}
> +
> +	/*
> +	 * Wait state is needed: make sure WEN is active before
> +	 * programming WTIME (and possibly WLONG).
> +	 */
> +	if (!(data->enable & TCS3472_ENABLE_WEN)) {
> +		data->enable |= TCS3472_ENABLE_WEN;
> +		ret = i2c_smbus_write_byte_data(data->client,
> +						TCS3472_ENABLE,
> +						data->enable);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	wtime = 256 - (int)DIV_ROUND_CLOSEST((unsigned long)wait_us, 2400);

Too many castings.

> +	if (wtime < 0) {
> +		wlong = true;
> +		wtime = 256 - (int)DIV_ROUND_CLOSEST((unsigned long)wait_us,
> +						     28800);

Ditto.

> +		if (wtime < 0)
> +			wtime = 0;
> +	}
> +	if (wtime > 255)
> +		wtime = 255;

These two is reinvention of clamp() from minmax.h.

> +	if (wlong != data->wlong) {
> +		ret = i2c_smbus_read_byte_data(data->client, TCS3472_CONFIG);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (wlong)
> +			ret |= TCS3472_CONFIG_WLONG;
> +		else
> +			ret &= ~TCS3472_CONFIG_WLONG;
> +
> +		ret = i2c_smbus_write_byte_data(data->client,
> +						TCS3472_CONFIG, ret);

Use room up to 80 (inclusive) characters.
Also note the ret = foo(ret) is a bad style.

> +		if (ret < 0)
> +			return ret;
> +
> +		data->wlong = wlong;
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_WTIME, wtime);
> +	if (ret < 0)
> +		return ret;
> +
> +	data->wtime = wtime;
> +	data->target_freq_hz = val;
> +	data->target_freq_uhz = val2;
> +
> +	return 0;
> +}

...

> -	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
> +	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON |
> +			TCS3472_ENABLE_WEN;

It's used at least two times, define a new one that combines all three with
a meaningful name.

...

> +	data->target_freq_uhz = (u64)(USEC_PER_SEC % cycle_us) *
> +				USEC_PER_SEC / cycle_us;

This is wrong. It won't compile (on 32-bit architectures).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] iio: light: tcs3472: implement wait time and sampling frequency

[Jonathan Cameron]

> 
> > +static unsigned int tcs3472_cycle_time_us(struct tcs3472_data *data)
> > +{
> > +	unsigned int atime_us = (256 - data->atime) * 2400;
> > +	unsigned int init_us = 2400;
> > +	unsigned int wtime_us;
> > +
> > +	if (!(data->enable & TCS3472_ENABLE_WEN))
> > +		wtime_us = 0;
> > +	else if (data->wlong)
> > +		wtime_us = (256 - data->wtime) * 28800;
> > +	else
> > +		wtime_us = (256 - data->wtime) * 2400;  
> 
> This is the same as atime_us.
Similar form but not the same unless data->wtime == data->atime
Probably not worth defining anything common for that.

> 
> > +	return wtime_us + init_us + atime_us;

[PATCH 3/3] iio: light: tcs3472: Use guard(mutex)() family over manual locking

[Aldo Conte]

Convert tcs3472_read_event_config, tcs3472_write_event_config,
tcs3472_write_event, tcs3472_powerdown and tcs3472_resume to use
automatico cleanup with guard(mutex)() instead of the old manual
locking method.

Found by code inspection.

Tested on a Raspberry Pi 3B with a TCS34725 at 0x29 address.
The following fields were read and written without any issues:
sampling_frequency, integration_time, calibscale and the
threshold event interface.
The unload of the driver works cleanly.

Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
 drivers/iio/light/tcs3472.c | 39 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index de51eb61f42a..90552f47a373 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -393,7 +393,7 @@ static int tcs3472_write_event(struct iio_dev *indio_dev,
 	int period;
 	int i;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 	switch (info) {
 	case IIO_EV_INFO_VALUE:
 		switch (dir) {
@@ -404,18 +404,17 @@ static int tcs3472_write_event(struct iio_dev *indio_dev,
 			command = TCS3472_AILT;
 			break;
 		default:
-			ret = -EINVAL;
-			goto error;
+			return -EINVAL;
 		}
 		ret = i2c_smbus_write_word_data(data->client, command, val);
 		if (ret)
-			goto error;
+			return ret;
 
 		if (dir == IIO_EV_DIR_RISING)
 			data->high_thresh = val;
 		else
 			data->low_thresh = val;
-		break;
+		return 0;
 	case IIO_EV_INFO_PERIOD:
 		period = val * USEC_PER_SEC + val2;
 		for (i = 1; i < ARRAY_SIZE(tcs3472_intr_pers) - 1; i++) {
@@ -425,18 +424,13 @@ static int tcs3472_write_event(struct iio_dev *indio_dev,
 		}
 		ret = i2c_smbus_write_byte_data(data->client, TCS3472_PERS, i);
 		if (ret)
-			goto error;
+			return ret;
 
 		data->apers = i;
-		break;
+		return 0;
 	default:
-		ret = -EINVAL;
-		break;
+		return -EINVAL;
 	}
-error:
-	mutex_unlock(&data->lock);
-
-	return ret;
 }
 
 static int tcs3472_read_event_config(struct iio_dev *indio_dev,
@@ -444,13 +438,9 @@ static int tcs3472_read_event_config(struct iio_dev *indio_dev,
 	enum iio_event_direction dir)
 {
 	struct tcs3472_data *data = iio_priv(indio_dev);
-	int ret;
 
-	mutex_lock(&data->lock);
-	ret = !!(data->enable & TCS3472_ENABLE_AIEN);
-	mutex_unlock(&data->lock);
-
-	return ret;
+	guard(mutex)(&data->lock);
+	return !!(data->enable & TCS3472_ENABLE_AIEN);
 }
 
 static int tcs3472_write_event_config(struct iio_dev *indio_dev,
@@ -461,7 +451,7 @@ static int tcs3472_write_event_config(struct iio_dev *indio_dev,
 	int ret = 0;
 	u8 enable_old;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 
 	enable_old = data->enable;
 
@@ -476,7 +466,6 @@ static int tcs3472_write_event_config(struct iio_dev *indio_dev,
 		if (ret)
 			data->enable = enable_old;
 	}
-	mutex_unlock(&data->lock);
 
 	return ret;
 }
@@ -580,15 +569,13 @@ static int tcs3472_powerdown(struct tcs3472_data *data)
 	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON |
 			TCS3472_ENABLE_WEN;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 
 	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
 					data->enable & ~enable_mask);
 	if (!ret)
 		data->enable &= ~enable_mask;
 
-	mutex_unlock(&data->lock);
-
 	return ret;
 }
 
@@ -722,15 +709,13 @@ static int tcs3472_resume(struct device *dev)
 	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON |
 			TCS3472_ENABLE_WEN;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 
 	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
 		data->enable | enable_mask);
 	if (!ret)
 		data->enable |= enable_mask;
 
-	mutex_unlock(&data->lock);
-
 	return ret;
 }
 
-- 
2.54.0

Re: [PATCH 3/3] iio: light: tcs3472: Use guard(mutex)() family over manual locking

[Andy Shevchenko]

On Wed, May 06, 2026 at 11:43:11AM +0200, Aldo Conte wrote:
> Convert tcs3472_read_event_config, tcs3472_write_event_config,
> tcs3472_write_event, tcs3472_powerdown and tcs3472_resume to use
> automatico cleanup with guard(mutex)() instead of the old manual
> locking method.

> Found by code inspection.

Huh?! Is it a bug? What is the inspection assumed here?

> Tested on a Raspberry Pi 3B with a TCS34725 at 0x29 address.
> The following fields were read and written without any issues:
> sampling_frequency, integration_time, calibscale and the
> threshold event interface.
> The unload of the driver works cleanly.

The comments should be in the cover letter and not in each of the commit
message, we do not want that noise here!

> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>

So, do not do ping-pong programming: when the code introduced in the series is
immediately being changed by another patch in the same series. As explained
this patch should be first in the series to solve that.

...

>  {
>  	struct tcs3472_data *data = iio_priv(indio_dev);
> -	int ret;
>  
> -	mutex_lock(&data->lock);
> -	ret = !!(data->enable & TCS3472_ENABLE_AIEN);
> -	mutex_unlock(&data->lock);
> -
> -	return ret;
> +	guard(mutex)(&data->lock);

Leave a blank line here.

> +	return !!(data->enable & TCS3472_ENABLE_AIEN);
>  }

...

> static int tcs3472_resume(struct device *dev)

>  	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON |
>  			TCS3472_ENABLE_WEN;

Taking into account the comment against patch 1, add a new patch to make this
follow the patter suggested there.

> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>  
>  	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>  		data->enable | enable_mask);
>  	if (!ret)
>  		data->enable |= enable_mask;
>  
> -	mutex_unlock(&data->lock);
> -
>  	return ret;
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] iio: light: tcs3472: devm conversion, wait time, locking cleanup

[Andy Shevchenko]

On Wed, May 06, 2026 at 11:43:08AM +0200, Aldo Conte wrote:
> This series of changes modernizes the tcs3472 driver by completing the
> devm conversion and updating the locking style to use an automatic
> guard (mutex). Furthermore, it implement control of the WAIT state,
> resolving a TODO.
> 
> Patch1: Converts the driver to use device-managed resource allocation.
> This removes the need for an explicit remove() callback, since cleanup
> is now handled automatically. It also adds a new function called
> tcs3472_powerdown_action() that powers down the chip when the driver is 
> unloaded, removed or when probe fails after the chip has been enabled.
> 
> Patch2: adds support for the wait time resolving the old TODO comment.
> The user can control the WTIME indirectly by writing to the
> sampling_frequency attribute. Changing the sampling
> frequency attribute results in a change to the wtime while preserving
> the current integration time.
> Similarly, if the user has previously set a sampling frequency and
> then changes the integration time, the driver re-computes WTIME so the 
> previous frequency is preserved. The patch also handles the deactivation
> of wtime when the requested frequency is very high, as well as the
> extension of wtime to its maximum values (by enabling the use of WLONG)
> when the requested frequency is very low.
> 
> Patch3: converts the remaining mutex_lock()/mutex_unlock() pairs to
> guard(mutex).
> 
> All patches have been tested on a Raspberry Pi 3B with a TCS3472
> connected to I2C-1 at address 0x29. Sampling frequency and integration
> time changes have been exercised checking the value of WTIME and
> consequently WEN and WLONG. Raw RGBC reads, calibscale, integration_time
> and threshold events continue to work as before.

Thanks for contribution, unfortunately this series needs much more work
and more preparatory patches as well. I have reviewed the current series.
Please, follow and address (or comment why it can't be done as suggested).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] iio: light: tcs3472: devm conversion, wait time, locking cleanup

[Aldo Conte]

On 5/6/26 12:21, Andy Shevchenko wrote:
> On Wed, May 06, 2026 at 11:43:08AM +0200, Aldo Conte wrote:
>> This series of changes modernizes the tcs3472 driver by completing the
>> devm conversion and updating the locking style to use an automatic
>> guard (mutex). Furthermore, it implement control of the WAIT state,
>> resolving a TODO.
>>
>> Patch1: Converts the driver to use device-managed resource allocation.
>> This removes the need for an explicit remove() callback, since cleanup
>> is now handled automatically. It also adds a new function called
>> tcs3472_powerdown_action() that powers down the chip when the driver is
>> unloaded, removed or when probe fails after the chip has been enabled.
>>
>> Patch2: adds support for the wait time resolving the old TODO comment.
>> The user can control the WTIME indirectly by writing to the
>> sampling_frequency attribute. Changing the sampling
>> frequency attribute results in a change to the wtime while preserving
>> the current integration time.
>> Similarly, if the user has previously set a sampling frequency and
>> then changes the integration time, the driver re-computes WTIME so the
>> previous frequency is preserved. The patch also handles the deactivation
>> of wtime when the requested frequency is very high, as well as the
>> extension of wtime to its maximum values (by enabling the use of WLONG)
>> when the requested frequency is very low.
>>
>> Patch3: converts the remaining mutex_lock()/mutex_unlock() pairs to
>> guard(mutex).
>>
>> All patches have been tested on a Raspberry Pi 3B with a TCS3472
>> connected to I2C-1 at address 0x29. Sampling frequency and integration
>> time changes have been exercised checking the value of WTIME and
>> consequently WEN and WLONG. Raw RGBC reads, calibscale, integration_time
>> and threshold events continue to work as before.
> 
> Thanks for contribution, unfortunately this series needs much more work
> and more preparatory patches as well. I have reviewed the current series.
> Please, follow and address (or comment why it can't be done as suggested).
> 

Thank you for your review.
These mistakes are undoubtedly due to my still limited experience. I 
will start working on them right away.

Best regards,
Aldo Conte

Re: [PATCH 0/3] iio: light: tcs3472: devm conversion, wait time, locking cleanup

[Aldo Conte]

On 5/6/26 12:21, Andy Shevchenko wrote:
> On Wed, May 06, 2026 at 11:43:08AM +0200, Aldo Conte wrote:
>> This series of changes modernizes the tcs3472 driver by completing the
>> devm conversion and updating the locking style to use an automatic
>> guard (mutex). Furthermore, it implement control of the WAIT state,
>> resolving a TODO.
>>
>> Patch1: Converts the driver to use device-managed resource allocation.
>> This removes the need for an explicit remove() callback, since cleanup
>> is now handled automatically. It also adds a new function called
>> tcs3472_powerdown_action() that powers down the chip when the driver is
>> unloaded, removed or when probe fails after the chip has been enabled.
>>
>> Patch2: adds support for the wait time resolving the old TODO comment.
>> The user can control the WTIME indirectly by writing to the
>> sampling_frequency attribute. Changing the sampling
>> frequency attribute results in a change to the wtime while preserving
>> the current integration time.
>> Similarly, if the user has previously set a sampling frequency and
>> then changes the integration time, the driver re-computes WTIME so the
>> previous frequency is preserved. The patch also handles the deactivation
>> of wtime when the requested frequency is very high, as well as the
>> extension of wtime to its maximum values (by enabling the use of WLONG)
>> when the requested frequency is very low.
>>
>> Patch3: converts the remaining mutex_lock()/mutex_unlock() pairs to
>> guard(mutex).
>>
>> All patches have been tested on a Raspberry Pi 3B with a TCS3472
>> connected to I2C-1 at address 0x29. Sampling frequency and integration
>> time changes have been exercised checking the value of WTIME and
>> consequently WEN and WLONG. Raw RGBC reads, calibscale, integration_time
>> and threshold events continue to work as before.
> 
> Thanks for contribution, unfortunately this series needs much more work
> and more preparatory patches as well. I have reviewed the current series.
> Please, follow and address (or comment why it can't be done as suggested).
> 
Hi Andy,

Thanks a lot for the detailed review. I'll address all the points in v2.

Regarding the series structure, I need to clarify the correct order:
1) Guard (mutex) conversion
2) Header sorting
3) Devm conversion (with the new TCS3472_Powerdown using Guard (mutex) 
directly)
4) Wait time implementation
Would this be OK?

Aldo

Re: [PATCH 0/3] iio: light: tcs3472: devm conversion, wait time, locking cleanup

[Andy Shevchenko]

On Wed, May 06, 2026 at 05:35:42PM +0200, Aldo Conte wrote:
> On 5/6/26 12:21, Andy Shevchenko wrote:
> > On Wed, May 06, 2026 at 11:43:08AM +0200, Aldo Conte wrote:

...

> Thanks a lot for the detailed review. I'll address all the points in v2.
> 
> Regarding the series structure, I need to clarify the correct order:
> 1) Guard (mutex) conversion

Since this most likely wants a new cleanup.h to be added, the 2) Header sorting
should go before even this one.

> 2) Header sorting
> 3) Devm conversion (with the new TCS3472_Powerdown using Guard (mutex)
> directly)
> 4) Wait time implementation
> Would this be OK?

Otherwise sounds good, but I can't be 100% sure until I see a v2.

-- 
With Best Regards,
Andy Shevchenko