[PATCH v2 0/5] devm conversion, wait time, locking cleanup

[PATCH v2 0/5] devm conversion, wait time, locking cleanup

[Aldo Conte]

This is v2 of the TCS3472 series, addressing the review comments from
Andy Shevchenko on v1. The series modernizes the tcs3472 driver by
converting it to use devm for resource management, switching to
guard(mutex) for locking, implementing wait time support exposed
through sampling_frequency (which was a long-standing TODO).

Patch 1: sorts the #include block alphabetically.
         v2:
         - New in v2 (suggested by Andy).

Patch 2: converts the remaining mutex_lock/unlock pairs to
         guard(mutex).
         v2: (suggested by Andy).
         - Moved earlier in the series; dropped "Found by code inspection".

Patch 3: converts the driver to devm for resource allocation, drops
         the explicit remove() callback, and adds 
         tcs3472_powerdown_action()
         to power down the chip on cleanup or probe failure.
         v2: (suggested by Andy).
         - Read-modify-write in powerdown/resume rewritten as 
           "compute, write, commit on success".
         - Use local 'struct device *dev = &client->dev' in probe.

Patch 4: implements wait time support, closing a long-standing TODO.
         The user controls WTIME indirectly via sampling_frequency.
         WTIME is recomputed when integration_time changes, preserving 
         the requested frequency.
         WEN is disabled when the frequency is too high; WLONG is
         enabled when the period exceeds the WTIME range.
         v2: (suggested by Andy).
         - validation tightened (rejects val > 0 with val2 < 0).
         - target_freq_uhz uses div_u64() for 32-bit portability.
         - TCS3472_ENABLE_RUN introduced here with AEN | PON | WEN,
           no longer redefined between patches.

Patch 5: moves the trailing return -EINVAL in tcs3472_read_raw() and
         tcs3472_write_raw() into explicit default: cases.
         v2:
         - New in v2 (suggested by Andy).

Testing:

All five patches were tested individually on a Raspberry Pi 3B with
an Adafruit TCS3472 breakout connected to I2C-1 at address 0x29.
Each patch was checked out and built separately to ensure the driver
remains functional in every intermediate state of the series.

Patch 1 (sort headers):
  - Driver loads cleanly; RGBC channel reads return reasonable values.
  - No warnings in dmesg.

Patch 2 (guard(mutex)):
  - Threshold value writes (RISING=1000, FALLING=200) read back
    correctly; AIHT/AILT registers on the chip mirror the values.
  - Threshold enable toggles the AIEN bit in ENABLE
    (sysfs '1' -> 0x13, sysfs '0' -> 0x03).
  - rmmod completes with no lock leak in dmesg.

Patch 3 (devm conversion):
  - Probe completes with ENABLE = 0x03 (PON|AEN, WEN not yet
    introduced).
  - RGBC reads return sensible values (e.g. clear=525, red=269,
    green=151, blue=97).
  - calibscale, integration_time, threshold value/period, and
    threshold enable all work as before.
  - After echo 0x29 to delete_device, ENABLE reads back as 0x00,
    confirming that devm_add_action_or_reset() invokes
    tcs3472_powerdown() on cleanup.
  - rmmod completes with no warning.

Patch 4 (wait time / sampling_frequency):
  - Initial state after probe: ATIME=0x00, WTIME=0xff, CONFIG=0x00,
    ENABLE=0x0b (PON|AEN|WEN), sampling_frequency=1.614987 Hz.
  - Write 1 Hz: WTIME=0x60, CONFIG=0x00, ENABLE=0x0b,
    sampling_frequency reads back 0.999200 Hz.
  - Write 5 Hz (too high for any wait time): WEN cleared,
    ENABLE=0x03, sampling_frequency reads back 1.621271 Hz (cycle
    with ATIME alone).
  - Write 0.5 Hz (period exceeds WTIME range): WLONG enabled,
    CONFIG=0x02, WTIME=0xd0, WEN re-enabled, ENABLE=0x0b,
    sampling_frequency reads back 0.500200 Hz.
  - After setting 0.5 Hz, changing integration_time to 0.024000 s:
    WTIME automatically recomputed to 0xbb, sampling_frequency reads
    back 0.496622 Hz (closest achievable to 0.5 Hz at the new ATIME).
  - Invalid inputs (-1.0, 0.0) rejected with -EINVAL; chip state
    unchanged.
  - After rmmod, ENABLE reads back as 0x00 (PON, AEN and WEN all
    cleared by powerdown via devm).

Patch 5 (default case):
  - All read_raw and write_raw cases (raw RGBC, calibscale,
    integration_time, sampling_frequency) handle attributes
    identically to the previous patch.
  - Invalid inputs still rejected with -EINVAL through the explicit
    default: case.
  - rmmod completes cleanly with ENABLE = 0x00

Aldo Conte (5):
  iio: light: tcs3472: sort headers alphabetically
  iio: light: tcs3472: convert remaining locking to guard(mutex)
  iio: light: tcs3472: use devm for resource management
  iio: light: tcs3472: implement wait time and sampling frequency
  iio: light: tcs3472: move standalone return to default case

 drivers/iio/light/tcs3472.c | 357 +++++++++++++++++++++++++-----------
 1 file changed, 249 insertions(+), 108 deletions(-)

-- 
2.54.0

[PATCH v2 1/5] iio: light: tcs3472: sort headers alphabetically

[Aldo Conte]

Sort the #include directives in alphabetical order in preparation for
adding new headers in upcoming patches.

No functional change.

Suggested-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
 drivers/iio/light/tcs3472.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index 12429a3261b3..9b0f64cd9c50 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -13,16 +13,16 @@
  * TODO: wait time
  */
 
-#include <linux/module.h>
-#include <linux/i2c.h>
 #include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
 #include <linux/pm.h>
 
+#include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-#include <linux/iio/events.h>
 #include <linux/iio/trigger_consumer.h>
-#include <linux/iio/buffer.h>
 #include <linux/iio/triggered_buffer.h>
 
 #define TCS3472_DRV_NAME "tcs3472"
-- 
2.54.0

[PATCH v2 2/5] iio: light: tcs3472: convert remaining locking to guard(mutex)

[Aldo Conte]

Convert the locking in tcs3472_read_event(), tcs3472_write_event(),
tcs3472_read_event_config(), tcs3472_write_event_config(),
tcs3472_powerdown() and tcs3472_resume() to use guard(mutex)
instead of explicit mutex_lock()/mutex_unlock() pairs.

This avoids manual unlock calls on each return path, drops the goto
in tcs3472_write_event(), and removes 'ret' variables only needed to
return after the unlock.

No functional change.

Suggested-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
v2:
(Suggested by Andy)
- Moved earlier in the series to avoid introducing manual locking in the devm
  patch that would then be converted here.
- Dropped "found by code inspection" (this is a cleanup, not a bug)
- Test details moved to cover letter

 drivers/iio/light/tcs3472.c | 59 +++++++++++++------------------------
 1 file changed, 21 insertions(+), 38 deletions(-)

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index 9b0f64cd9c50..4fd4fd74d0d6 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -13,6 +13,7 @@
  * TODO: wait time
  */
 
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
@@ -220,32 +221,24 @@ static int tcs3472_read_event(struct iio_dev *indio_dev,
 	int *val2)
 {
 	struct tcs3472_data *data = iio_priv(indio_dev);
-	int ret;
 	unsigned int period;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 
 	switch (info) {
 	case IIO_EV_INFO_VALUE:
 		*val = (dir == IIO_EV_DIR_RISING) ?
 			data->high_thresh : data->low_thresh;
-		ret = IIO_VAL_INT;
-		break;
+		return IIO_VAL_INT;
 	case IIO_EV_INFO_PERIOD:
 		period = (256 - data->atime) * 2400 *
 			tcs3472_intr_pers[data->apers];
 		*val = period / USEC_PER_SEC;
 		*val2 = period % USEC_PER_SEC;
-		ret = IIO_VAL_INT_PLUS_MICRO;
-		break;
+		return IIO_VAL_INT_PLUS_MICRO;
 	default:
-		ret = -EINVAL;
-		break;
+		return -EINVAL;
 	}
-
-	mutex_unlock(&data->lock);
-
-	return ret;
 }
 
 static int tcs3472_write_event(struct iio_dev *indio_dev,
@@ -259,7 +252,8 @@ 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) {
@@ -270,18 +264,18 @@ 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++) {
@@ -291,18 +285,14 @@ 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,
@@ -310,13 +300,10 @@ 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);
+	guard(mutex)(&data->lock);
 
-	return ret;
+	return !!(data->enable & TCS3472_ENABLE_AIEN);
 }
 
 static int tcs3472_write_event_config(struct iio_dev *indio_dev,
@@ -327,7 +314,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;
 
@@ -342,7 +329,6 @@ static int tcs3472_write_event_config(struct iio_dev *indio_dev,
 		if (ret)
 			data->enable = enable_old;
 	}
-	mutex_unlock(&data->lock);
 
 	return ret;
 }
@@ -379,6 +365,7 @@ static irqreturn_t tcs3472_trigger_handler(int irq, void *p)
 	} scan = { };
 
 	int ret = tcs3472_req_data(data);
+
 	if (ret < 0)
 		goto done;
 
@@ -544,15 +531,13 @@ static int tcs3472_powerdown(struct tcs3472_data *data)
 	int ret;
 	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
 
-	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;
 }
 
@@ -581,15 +566,13 @@ static int tcs3472_resume(struct device *dev)
 	int ret;
 	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
 
-	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

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

[Aldo Conte]

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.

Rewrite the read-modify-write pattern in tcs3472_powerdown() and
tcs3472_resume() so the new register value is computed first, written
to the chip, and committed to data->enable only on success.

Use a local 'dev = &client->dev' in tcs3472_probe() to keep the devm
calls compact.

Suggested-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
v2:
(Suggested by Andy)
- Rewrote read-modify-write in tcs3472_powerdown() and tcs3472_resume()
- Use local 'struct device *dev = &client->dev' in probe.
- Dropped "Compiled with W=1" and test details from the commit message.

 drivers/iio/light/tcs3472.c | 107 +++++++++++++++++-------------------
 1 file changed, 51 insertions(+), 56 deletions(-)

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index 4fd4fd74d0d6..7a6dc8360326 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -427,13 +427,38 @@ static const struct iio_info tcs3472_info = {
 	.attrs = &tcs3472_attribute_group,
 };
 
+static int tcs3472_powerdown(struct tcs3472_data *data)
+{
+	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
+	u8 value;
+	int ret;
+
+	guard(mutex)(&data->lock);
+
+	value = data->enable & ~enable_mask;
+
+	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
+	if (ret)
+		return ret;
+
+	data->enable = value;
+
+	return 0;
+}
+
+static void tcs3472_powerdown_action(void *data)
+{
+	tcs3472_powerdown(data);
+}
+
 static int tcs3472_probe(struct i2c_client *client)
 {
+	struct device *dev = &client->dev;
 	struct tcs3472_data *data;
 	struct iio_dev *indio_dev;
 	int ret;
 
-	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (indio_dev == NULL)
 		return -ENOMEM;
 
@@ -453,9 +478,9 @@ static int tcs3472_probe(struct i2c_client *client)
 		return ret;
 
 	if (ret == 0x44)
-		dev_info(&client->dev, "TCS34721/34725 found\n");
+		dev_info(dev, "TCS34721/34725 found\n");
 	else if (ret == 0x4d)
-		dev_info(&client->dev, "TCS34723/34727 found\n");
+		dev_info(dev, "TCS34723/34727 found\n");
 	else
 		return -ENODEV;
 
@@ -497,59 +522,26 @@ 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(dev, tcs3472_powerdown_action, data);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_triggered_buffer_setup(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(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;
-
-	guard(mutex)(&data->lock);
-
-	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
-		data->enable & ~enable_mask);
-	if (!ret)
-		data->enable &= ~enable_mask;
-
-	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(dev, indio_dev);
 }
 
 static int tcs3472_suspend(struct device *dev)
@@ -563,17 +555,21 @@ 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 value;
+	int ret;
 
 	guard(mutex)(&data->lock);
 
-	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
-		data->enable | enable_mask);
-	if (!ret)
-		data->enable |= enable_mask;
+	value = data->enable | enable_mask;
 
-	return ret;
+	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
+	if (ret)
+		return ret;
+
+	data->enable = value;
+
+	return 0;
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(tcs3472_pm_ops, tcs3472_suspend,
@@ -591,7 +587,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

[PATCH v2 4/5] 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 an 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 control of the wait time through IIO_CHAN_INFO_SAMP_FREQ:

  - Reading sampling_frequency returns the chip's current cycle time,
    computed as the sum of ATIME, the fixed RGBC initialization time
    and the wait time (which depends on WEN and WLONG).

  - Writing sampling_frequency programs WTIME so that the resulting
    cycle period approximates the requested frequency. If the
    requested frequency cannot be reached with any
    non-zero wait time, WEN is disabled and the chip runs
    back-to-back conversions at the maximum rate allowed by ATIME.
    If the requested period exceeds the maximum WTIME range, WLONG
    is enabled to extend the wait step from 2.4 ms to 28.8 ms.

  - The user's last requested frequency is stored in the driver's
    private data so that subsequent changes to integration_time
    recompute WTIME and preserve the requested sampling rate as
    closely as possible.

Add TCS3472_ENABLE_WEN, TCS3472_ENABLE_RUN and TCS3472_CONFIG_WLONG
bit definitions. TCS3472_ENABLE_RUN bundles the bits
(AEN | PON | WEN) that are simultaneously set when the chip is in
running state and cleared during powerdown, and is used by
tcs3472_probe(), tcs3472_powerdown() and tcs3472_resume().

Remove the "TODO: wait time" comment at the top of the file.

Suggested-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
v2:
( Suggested-by Andy )
- Validation also rejects val > 0 with val2 < 0
- cycle_us uses PSEC_PER_SEC (was USEC_PER_SEC * USEC_PER_SEC),
- clamp() replaces manual WTIME bounds checks.
- DIV_ROUND_CLOSEST_ULL() allows to avoid all the castings.
- CONFIG read-modify-write uses a separate 'config' variable instead of the
  ret = foo(ret).
- Multi-line comments now follow kernel style.
- Added comment explaining why wait_us can be negative.
- target_freq_uhz uses div_u64() for 32-bit portability.
- TCS3472_ENABLE_RUN introduced here with AEN | PON | WEN.
- Test details moved to the cover letter.

 drivers/iio/light/tcs3472.c | 189 +++++++++++++++++++++++++++++++++---
 1 file changed, 175 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index 7a6dc8360326..f8f70399a4dc 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -9,13 +9,12 @@
  * TCS34727)
  *
  * Datasheet: http://ams.com/eng/content/download/319364/1117183/file/TCS3472_Datasheet_EN_v2.pdf
- *
- * TODO: wait time
  */
 
 #include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/math64.h>
 #include <linux/module.h>
 #include <linux/pm.h>
 
@@ -52,19 +51,27 @@
 #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_ENABLE_RUN (TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON | \
+			    TCS3472_ENABLE_WEN)
 #define TCS3472_CONTROL_AGAIN_MASK (BIT(0) | BIT(1))
+#define TCS3472_CONFIG_WLONG BIT(1)
 
 struct tcs3472_data {
 	struct i2c_client *client;
 	struct mutex lock;
+	int target_freq_hz;
+	int target_freq_uhz;
 	u16 low_thresh;
 	u16 high_thresh;
 	u8 enable;
 	u8 control;
 	u8 atime;
 	u8 apers;
+	u8 wtime;
+	bool wlong;
 };
 
 static const struct iio_event_spec tcs3472_events[] = {
@@ -90,6 +97,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, \
@@ -113,6 +121,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;
@@ -135,6 +159,100 @@ static int tcs3472_req_data(struct tcs3472_data *data)
 	return 0;
 }
 
+static int tcs3472_set_sampling_freq(struct tcs3472_data *data,
+				     int val, int val2)
+{
+	unsigned int atime_us;
+	unsigned int init_us = 2400;
+	u64 cycle_us;
+	s64 wait_us;
+	int wtime;
+	bool wlong = false;
+	u8 config;
+	int ret;
+
+	if (val < 0 || val2 < 0 || (val == 0 && val2 == 0))
+		return -EINVAL;
+
+	guard(mutex)(&data->lock);
+
+	atime_us = (256 - data->atime) * 2400;
+	cycle_us = div_u64((u64)PSEC_PER_SEC,
+			   (u64)val * USEC_PER_SEC + val2);
+
+	/*
+	 * wait_us can be negative or smaller than the minimum wait step
+	 * (2400 us) when the requested frequency is too high to be reached.
+	 * In that case, disable WEN so that the chip can perform back-to-back
+	 * conversions at the maximum rate permitted by the current ATIME.
+	 */
+	wait_us = (s64)cycle_us - init_us - atime_us;
+
+	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 - DIV_ROUND_CLOSEST_ULL(wait_us, 2400);
+	if (wtime < 0) {
+		wlong = true;
+		wtime = 256 - DIV_ROUND_CLOSEST_ULL(wait_us, 28800);
+	}
+	wtime = clamp(wtime, 0, 255);
+
+	if (wlong != data->wlong) {
+		ret = i2c_smbus_read_byte_data(data->client, TCS3472_CONFIG);
+		if (ret < 0)
+			return ret;
+
+		config = ret;
+		if (wlong)
+			config |= TCS3472_CONFIG_WLONG;
+		else
+			config &= ~TCS3472_CONFIG_WLONG;
+
+		ret = i2c_smbus_write_byte_data(data->client, TCS3472_CONFIG,
+						config);
+		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_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val, int *val2, long mask)
@@ -165,6 +283,14 @@ 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 = div_u64((u64)(USEC_PER_SEC % cycle_us) * USEC_PER_SEC,
+				cycle_us);
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
 	}
 	return -EINVAL;
 }
@@ -175,6 +301,7 @@ static int tcs3472_write_raw(struct iio_dev *indio_dev,
 {
 	struct tcs3472_data *data = iio_priv(indio_dev);
 	int i;
+	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_CALIBSCALE:
@@ -194,15 +321,29 @@ static int tcs3472_write_raw(struct iio_dev *indio_dev,
 		if (val != 0)
 			return -EINVAL;
 		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);
-			}
-
+			if (val2 != (256 - i) * 2400)
+				continue;
+
+			data->atime = i;
+			ret = i2c_smbus_write_byte_data(data->client,
+							TCS3472_ATIME,
+							data->atime);
+			if (ret < 0)
+				return ret;
+
+			/*
+			 * ATIME just changed, so the cycle time changed too.
+			 * Re-run the sampling frequency logic to recompute
+			 * WTIME and preserve the user's last requested
+			 * frequency.
+			 */
+			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;
 }
@@ -429,13 +570,12 @@ static const struct iio_info tcs3472_info = {
 
 static int tcs3472_powerdown(struct tcs3472_data *data)
 {
-	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
 	u8 value;
 	int ret;
 
 	guard(mutex)(&data->lock);
 
-	value = data->enable & ~enable_mask;
+	value = data->enable & ~TCS3472_ENABLE_RUN;
 
 	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
 	if (ret)
@@ -456,6 +596,7 @@ static int tcs3472_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct tcs3472_data *data;
 	struct iio_dev *indio_dev;
+	unsigned int cycle_us;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
@@ -494,6 +635,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;
@@ -515,13 +666,24 @@ 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_RUN;
 	data->enable &= ~TCS3472_ENABLE_AIEN;
 	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
 		data->enable);
 	if (ret < 0)
 		return ret;
 
+	/*
+	 * Initialize target frequency from the chip's current state so
+	 * that subsequent integration_time changes via IIO_CHAN_INFO_INT_TIME
+	 * can preserve a meaningful sampling rate, even before userspace
+	 * writes sampling_frequency for the first time.
+	 */
+	cycle_us = tcs3472_cycle_time_us(data);
+	data->target_freq_hz = USEC_PER_SEC / cycle_us;
+	data->target_freq_uhz = div_u64((u64)(USEC_PER_SEC % cycle_us) *
+					USEC_PER_SEC, cycle_us);
+
 	ret = devm_add_action_or_reset(dev, tcs3472_powerdown_action, data);
 	if (ret)
 		return ret;
@@ -555,13 +717,12 @@ static int tcs3472_resume(struct device *dev)
 {
 	struct tcs3472_data *data = iio_priv(i2c_get_clientdata(
 		to_i2c_client(dev)));
-	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
 	u8 value;
 	int ret;
 
 	guard(mutex)(&data->lock);
 
-	value = data->enable | enable_mask;
+	value = data->enable | TCS3472_ENABLE_RUN;
 
 	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
 	if (ret)
-- 
2.54.0

[PATCH v2 5/5] iio: light: tcs3472: move standalone return to default case

[Aldo Conte]

Move the trailing 'return -EINVAL' statements at the end of
tcs3472_read_raw() and tcs3472_write_raw() into explicit default:
cases inside the respective switch statements.

This removes the need for a separate return statement after the switch.

No functional change.

Suggested-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
 drivers/iio/light/tcs3472.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index f8f70399a4dc..4ba2def244b7 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -291,8 +291,9 @@ static int tcs3472_read_raw(struct iio_dev *indio_dev,
 				cycle_us);
 		return IIO_VAL_INT_PLUS_MICRO;
 	}
+	default:
+		return -EINVAL;
 	}
-	return -EINVAL;
 }
 
 static int tcs3472_write_raw(struct iio_dev *indio_dev,
@@ -344,8 +345,9 @@ static int tcs3472_write_raw(struct iio_dev *indio_dev,
 		return -EINVAL;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		return tcs3472_set_sampling_freq(data, val, val2);
+	default:
+		return -EINVAL;
 	}
-	return -EINVAL;
 }
 
 /*
-- 
2.54.0

Re: [PATCH v2 2/5] iio: light: tcs3472: convert remaining locking to guard(mutex)

[Joshua Crofts]

On Wed, 13 May 2026 at 00:39, Aldo Conte <aldocontelk@gmail.com> wrote:
> @@ -379,6 +365,7 @@ static irqreturn_t tcs3472_trigger_handler(int irq, void *p)
>         } scan = { };
>
>         int ret = tcs3472_req_data(data);
> +

Unnecessary space here, function calls and return value checks should
be grouped together.

Othewise, this looks like a solid cleanup.

-- 
Kind regards

CJD

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

[Joshua Crofts]

On Wed, 13 May 2026 at 00:40, Aldo Conte <aldocontelk@gmail.com> wrote:
>  static int tcs3472_probe(struct i2c_client *client)
>  {
> +       struct device *dev = &client->dev;
>         struct tcs3472_data *data;
>         struct iio_dev *indio_dev;
>         int ret;
>
> -       indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +       indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>         if (indio_dev == NULL)

Not introduced by this patch, but `if (!indio_dev)` is better if you're checking
for NULL (checkpatch.pl would probably mention it during a run).

Since you're moving the driver to use devm_* functions, might I suggest
moving mutex_init() to devm_mutex_init() in the probe() function? Something
along the lines of the following:

ret = devm_mutex_int(dev, &data->lock);
if (ret)
         return ret;

-- 
Kind regards

CJD

Re: [PATCH v2 1/5] iio: light: tcs3472: sort headers alphabetically

[Joshua Crofts]

On Wed, 13 May 2026 at 00:39, Aldo Conte <aldocontelk@gmail.com> wrote:
>
> Sort the #include directives in alphabetical order in preparation for
> adding new headers in upcoming patches.
>
> No functional change.
>
> Suggested-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
> ---
>  drivers/iio/light/tcs3472.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
> index 12429a3261b3..9b0f64cd9c50 100644
> --- a/drivers/iio/light/tcs3472.c
> +++ b/drivers/iio/light/tcs3472.c
> @@ -13,16 +13,16 @@
>   * TODO: wait time
>   */
>
> -#include <linux/module.h>
> -#include <linux/i2c.h>
>  #include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
>  #include <linux/pm.h>
>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> -#include <linux/iio/events.h>
>  #include <linux/iio/trigger_consumer.h>
> -#include <linux/iio/buffer.h>
>  #include <linux/iio/triggered_buffer.h>
>
>  #define TCS3472_DRV_NAME "tcs3472"
> --

Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>

-- 
Kind regards

CJD

Re: [PATCH v2 5/5] iio: light: tcs3472: move standalone return to default case

[Joshua Crofts]

On Wed, 13 May 2026 at 00:40, Aldo Conte <aldocontelk@gmail.com> wrote:
>
> Move the trailing 'return -EINVAL' statements at the end of
> tcs3472_read_raw() and tcs3472_write_raw() into explicit default:
> cases inside the respective switch statements.
>
> This removes the need for a separate return statement after the switch.
>
> No functional change.
>
> Suggested-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>

Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>

-- 
Kind regards

CJD

Re: [PATCH v2 2/5] iio: light: tcs3472: convert remaining locking to guard(mutex)

[Andy Shevchenko]

On Wed, May 13, 2026 at 12:32:12AM +0200, Aldo Conte wrote:
> Convert the locking in tcs3472_read_event(), tcs3472_write_event(),
> tcs3472_read_event_config(), tcs3472_write_event_config(),
> tcs3472_powerdown() and tcs3472_resume() to use guard(mutex)
> instead of explicit mutex_lock()/mutex_unlock() pairs.
> 
> This avoids manual unlock calls on each return path, drops the goto
> in tcs3472_write_event(), and removes 'ret' variables only needed to
> return after the unlock.
> 
> No functional change.

...

> Suggested-by: Andy Shevchenko <andy@kernel.org>

Not appropriate tag, I suggested only changes (as you pointed out below) to
your idea and initial contribution.

> ---
> v2:
> (Suggested by Andy)
> - Moved earlier in the series to avoid introducing manual locking in the devm
>   patch that would then be converted here.
> - Dropped "found by code inspection" (this is a cleanup, not a bug)
> - Test details moved to cover letter

...

Code wise LGTM.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/5] iio: light: tcs3472: convert remaining locking to guard(mutex)

[Andy Shevchenko]

On Wed, May 13, 2026 at 09:47:06AM +0200, Joshua Crofts wrote:
> On Wed, 13 May 2026 at 00:39, Aldo Conte <aldocontelk@gmail.com> wrote:

...

> >         } scan = { };
> >
> >         int ret = tcs3472_req_data(data);
> > +
> 
> Unnecessary space here, function calls and return value checks should
> be grouped together.

Ah, good catch! We actually discourage the assignment and definition like this
(when the result is going to be validated). It's harder to maintain and prone to
subtle bugs.

Giving the above context, this should be rather as

       } scan = { };
       int ret;

       ret = tcs3472_req_data(data);
       if (ret ...)
           ...

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, May 13, 2026 at 12:32:13AM +0200, Aldo Conte wrote:
> 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.
> 
> Rewrite the read-modify-write pattern in tcs3472_powerdown() and
> tcs3472_resume() so the new register value is computed first, written
> to the chip, and committed to data->enable only on success.
> 
> Use a local 'dev = &client->dev' in tcs3472_probe() to keep the devm
> calls compact.

...

> Suggested-by: Andy Shevchenko <andy@kernel.org>

In appropriate tag again.

...

> -		dev_info(&client->dev, "TCS34723/34727 found\n");
> +		dev_info(dev, "TCS34723/34727 found\n");

This is not part of devm conversion. You should not do two or more things in a
single patch — split!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/5] iio: light: tcs3472: implement wait time and sampling frequency

[Andy Shevchenko]

On Wed, May 13, 2026 at 12:32:14AM +0200, Aldo Conte wrote:
> The TCS3472 has a wait state controlled by the WEN bit in the ENABLE
> register and the WAIT register, with an 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 control of the wait time through IIO_CHAN_INFO_SAMP_FREQ:
> 
>   - Reading sampling_frequency returns the chip's current cycle time,
>     computed as the sum of ATIME, the fixed RGBC initialization time
>     and the wait time (which depends on WEN and WLONG).
> 
>   - Writing sampling_frequency programs WTIME so that the resulting
>     cycle period approximates the requested frequency. If the
>     requested frequency cannot be reached with any
>     non-zero wait time, WEN is disabled and the chip runs
>     back-to-back conversions at the maximum rate allowed by ATIME.
>     If the requested period exceeds the maximum WTIME range, WLONG
>     is enabled to extend the wait step from 2.4 ms to 28.8 ms.
> 
>   - The user's last requested frequency is stored in the driver's
>     private data so that subsequent changes to integration_time
>     recompute WTIME and preserve the requested sampling rate as
>     closely as possible.
> 
> Add TCS3472_ENABLE_WEN, TCS3472_ENABLE_RUN and TCS3472_CONFIG_WLONG
> bit definitions. TCS3472_ENABLE_RUN bundles the bits
> (AEN | PON | WEN) that are simultaneously set when the chip is in
> running state and cleared during powerdown, and is used by
> tcs3472_probe(), tcs3472_powerdown() and tcs3472_resume().
> 
> Remove the "TODO: wait time" comment at the top of the file.

> Suggested-by: Andy Shevchenko <andy@kernel.org>

Inappropriate tag.
You must not put tags on your own, if in doubt, ask first!

...

> +#define TCS3472_ENABLE_RUN (TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON | \
> +			    TCS3472_ENABLE_WEN)

Better style is

#define TCS3472_ENABLE_RUN						\
	(TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON | TCS3472_ENABLE_WEN)

...

> +	cycle_us = div_u64((u64)PSEC_PER_SEC,
> +			   (u64)val * USEC_PER_SEC + val2);

First of all, it's one line. Second, the divisor for this function is 32-bit.
And at last the castings are not needed. I think I already told these...

...

> +	wtime = 256 - DIV_ROUND_CLOSEST_ULL(wait_us, 2400);
> +	if (wtime < 0) {
> +		wlong = true;
> +		wtime = 256 - DIV_ROUND_CLOSEST_ULL(wait_us, 28800);
> +	}

Why 64-bit divisions? Do you expect the wait_us be outside INT_MIN/INT_MAX range?
This will need a comment and/or dropping the 64-bit arithmetics.

> +	wtime = clamp(wtime, 0, 255);

...

> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_WTIME, wtime);
> +	if (ret < 0)

What's the meaning of positive returned values? I think this function never
does that. If I'm right, drop ' < 0' parts in all similar cases.

> +		return ret;

...

> +		*val = USEC_PER_SEC / cycle_us;
> +		*val2 = div_u64((u64)(USEC_PER_SEC % cycle_us) * USEC_PER_SEC,
> +				cycle_us);

Is this even correct? We take modulo of cycle_us to get under the MICRO range,
then multiply to MICRO (seconds) and divide by full cycle_us. I'm lost here.

...

> +	cycle_us = tcs3472_cycle_time_us(data);
> +	data->target_freq_hz = USEC_PER_SEC / cycle_us;
> +	data->target_freq_uhz = div_u64((u64)(USEC_PER_SEC % cycle_us) *
> +					USEC_PER_SEC, cycle_us);

Okay, this might help with the above... Can you deduplicate this division to
a helper with a comment that explains the calculations behind?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iio: light: tcs3472: move standalone return to default case

[Andy Shevchenko]

On Wed, May 13, 2026 at 12:32:15AM +0200, Aldo Conte wrote:
> Move the trailing 'return -EINVAL' statements at the end of
> tcs3472_read_raw() and tcs3472_write_raw() into explicit default:
> cases inside the respective switch statements.
> 
> This removes the need for a separate return statement after the switch.
> 
> No functional change.

Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/5] iio: light: tcs3472: move standalone return to default case

[Aldo Conte]

On 5/13/26 13:23, Andy Shevchenko wrote:
> On Wed, May 13, 2026 at 12:32:15AM +0200, Aldo Conte wrote:
>> Move the trailing 'return -EINVAL' statements at the end of
>> tcs3472_read_raw() and tcs3472_write_raw() into explicit default:
>> cases inside the respective switch statements.
>>
>> This removes the need for a separate return statement after the switch.
>>
>> No functional change.
> 
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> 
Hi Andy, Joshua,

Thanks for the review. Before I send v3, I want to make sure I split
the series the way you want.

My plan is to split v2 into 7 patches:

   1. sort headers (same as v2)
   2. convert locking to guard(mutex) (drop Suggested-by, clean up
      trigger_handler)
   3. replace == NULL with ! (add Suggested-by: Joshua Crofts)
   4. use devm for resource management (drop Suggested-by, remove the
      dev_info change, add devm_mutex_init)
   5. use 'dev' for dev_info() calls (new, taken out of patch 4)
   6. implement wait time and sampling_frequency (drop Suggested-by,
      fix macro style, simplify cycle_us and all the other revisions)
   7. move standalone return to default case (same as v2)

Two things I am not sure about:

   - Should patch 3 ("replace == NULL with !") really be its own patch,
     or do you prefer it inside patch 4, since that line is right next
     to devm_iio_device_alloc?

   - Is patch 5 (dev_info refactor) in the right place after the devm
     patch?


Aldo

Re: [PATCH v2 5/5] iio: light: tcs3472: move standalone return to default case

[Andy Shevchenko]

On Wed, May 13, 2026 at 06:12:25PM +0200, Aldo Conte wrote:
> On 5/13/26 13:23, Andy Shevchenko wrote:
> > On Wed, May 13, 2026 at 12:32:15AM +0200, Aldo Conte wrote:

...

> Thanks for the review. Before I send v3, I want to make sure I split
> the series the way you want.
> 
> My plan is to split v2 into 7 patches:
> 
>   1. sort headers (same as v2)
>   2. convert locking to guard(mutex) (drop Suggested-by, clean up
>      trigger_handler)
>   3. replace == NULL with ! (add Suggested-by: Joshua Crofts)
>   4. use devm for resource management (drop Suggested-by, remove the
>      dev_info change, add devm_mutex_init)
>   5. use 'dev' for dev_info() calls (new, taken out of patch 4)
>   6. implement wait time and sampling_frequency (drop Suggested-by,
>      fix macro style, simplify cycle_us and all the other revisions)
>   7. move standalone return to default case (same as v2)

(Also collect tags for the patches that are not [drastically] changed, like #7
 here.)

> Two things I am not sure about:
> 
>   - Should patch 3 ("replace == NULL with !") really be its own patch,
>     or do you prefer it inside patch 4, since that line is right next
>     to devm_iio_device_alloc?

Jonathan is okay to combine in some cases, but I'm not sure if this is small
enough. Ask him?

>   - Is patch 5 (dev_info refactor) in the right place after the devm
>     patch?

Basically sort patches by severity, with this in mind seems you suggested
the right order.


-- 
With Best Regards,
Andy Shevchenko

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

[Aldo Conte]

On 5/13/26 00:32, Aldo Conte wrote:
> 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.
> 
> Rewrite the read-modify-write pattern in tcs3472_powerdown() and
> tcs3472_resume() so the new register value is computed first, written
> to the chip, and committed to data->enable only on success.
> 
> Use a local 'dev = &client->dev' in tcs3472_probe() to keep the devm
> calls compact.
> 
> Suggested-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
> ---
> v2:
> (Suggested by Andy)
> - Rewrote read-modify-write in tcs3472_powerdown() and tcs3472_resume()
> - Use local 'struct device *dev = &client->dev' in probe.
> - Dropped "Compiled with W=1" and test details from the commit message.
> 
>   drivers/iio/light/tcs3472.c | 107 +++++++++++++++++-------------------
>   1 file changed, 51 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
> index 4fd4fd74d0d6..7a6dc8360326 100644
> --- a/drivers/iio/light/tcs3472.c
> +++ b/drivers/iio/light/tcs3472.c
> @@ -427,13 +427,38 @@ static const struct iio_info tcs3472_info = {
>   	.attrs = &tcs3472_attribute_group,
>   };
>   
> +static int tcs3472_powerdown(struct tcs3472_data *data)
> +{
> +	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
> +	u8 value;
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
> +
> +	value = data->enable & ~enable_mask;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
> +	if (ret)
> +		return ret;
> +
> +	data->enable = value;
> +
> +	return 0;
> +}
> +
> +static void tcs3472_powerdown_action(void *data)
> +{
> +	tcs3472_powerdown(data);
> +}
> +
>   static int tcs3472_probe(struct i2c_client *client)
>   {
> +	struct device *dev = &client->dev;
>   	struct tcs3472_data *data;
>   	struct iio_dev *indio_dev;
>   	int ret;
>   
> -	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>   	if (indio_dev == NULL)
>   		return -ENOMEM;
> 

Hi Jonathan,

In v3 I have a small style cleanup: replacing 'if (indio_dev == NULL)'
with 'if (!indio_dev)' in tcs3472_probe(). This was suggested by
Joshua Crofts in his v2 review.

What do you prefer it is a separate patch
or folded into the devm conversion patch (which already modifies the
context next to that line).

Either way works for me. Which do you prefer?

Thanks,
Aldo

...

Re: [PATCH v2 0/5] devm conversion, wait time, locking cleanup

[Jonathan Cameron]

On Wed, 13 May 2026 00:32:10 +0200
Aldo Conte <aldocontelk@gmail.com> wrote:

General comment. Cover letters provide the titles for email thread,
patchwork and sashiko entries.  Make sure they have same prefix as
patches.
iio: tcs3472: ...

Re: [PATCH v2 2/5] iio: light: tcs3472: convert remaining locking to guard(mutex)

[Jonathan Cameron]

On Wed, 13 May 2026 00:32:12 +0200
Aldo Conte <aldocontelk@gmail.com> wrote:

> Convert the locking in tcs3472_read_event(), tcs3472_write_event(),
> tcs3472_read_event_config(), tcs3472_write_event_config(),
> tcs3472_powerdown() and tcs3472_resume() to use guard(mutex)
> instead of explicit mutex_lock()/mutex_unlock() pairs.
I'd skip the list.  If anyone cares which functions are changed then
they can read the code!

Convert several functions to use guard(mutex)()

> 
> This avoids manual unlock calls on each return path, drops the goto
> in tcs3472_write_event(), and removes 'ret' variables only needed to
> return after the unlock.
> 
> No functional change.
> 
> Suggested-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
A couple of places where the use of guard() enables some nice cleanup.
Given it's the guard() that let's us do it I consider it part of the same
change.

Also one stray change in here that has nothing to do with the rest.

> ---
> v2:
> (Suggested by Andy)
> - Moved earlier in the series to avoid introducing manual locking in the devm
>   patch that would then be converted here.
> - Dropped "found by code inspection" (this is a cleanup, not a bug)
> - Test details moved to cover letter
> 
>  drivers/iio/light/tcs3472.c | 59 +++++++++++++------------------------
>  1 file changed, 21 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
> index 9b0f64cd9c50..4fd4fd74d0d6 100644
> --- a/drivers/iio/light/tcs3472.c
> +++ b/drivers/iio/light/tcs3472.c

>  
>  static int tcs3472_read_event_config(struct iio_dev *indio_dev,
> @@ -310,13 +300,10 @@ 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);
> +	guard(mutex)(&data->lock);
>  
> -	return ret;
> +	return !!(data->enable & TCS3472_ENABLE_AIEN);

Hmm. Kind of unrelated, but given the code is changing can we make this a FIELD_GET()
Linus got quite grumpy a few years ago about readability of !!.
Alternative would ? 1 : 0;

>  }
>  
>  static int tcs3472_write_event_config(struct iio_dev *indio_dev,
> @@ -327,7 +314,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;
>  
> @@ -342,7 +329,6 @@ static int tcs3472_write_event_config(struct iio_dev *indio_dev,
>  		if (ret)
>  			data->enable = enable_old;
		if (ret) {
			data->enable = enable_old;
			return ret;
		}
>  	}
> -	mutex_unlock(&data->lock);
>  
>  	return ret;

	return 0;

This one is a case of reducing how far we have to look to see what happens
in each code path. Fine in this patch as it's the guard() enabling it.

>  }
> @@ -379,6 +365,7 @@ static irqreturn_t tcs3472_trigger_handler(int irq, void *p)
>  	} scan = { };
>  
>  	int ret = tcs3472_req_data(data);
> +
Unrelated change so doesn't belong in a patch doing guard() stuff.

>  	if (ret < 0)
>  		goto done;
>  
> @@ -544,15 +531,13 @@ static int tcs3472_powerdown(struct tcs3472_data *data)
>  	int ret;
>  	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
>  
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>  
>  	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>  		data->enable & ~enable_mask);
>  	if (!ret)

As below.

>  		data->enable &= ~enable_mask;
>  
> -	mutex_unlock(&data->lock);
> -
>  	return ret;
>  }
>  
> @@ -581,15 +566,13 @@ static int tcs3472_resume(struct device *dev)
>  	int ret;
>  	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
>  
> -	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;
	
	if (ret)
		return ret;

	data->enable |= enable_mask;

	return 0;

One of the nicest things about cleanup.h magic is avoids us needing
to do handling of good paths 'out of line'.  Thus we can switch to
more ideomatic code that is easier to read. That can be done as
part of this change as it's the guard() stuff that made it possible.

Jonathan

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

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

[Jonathan Cameron]

On Wed, 13 May 2026 22:29:55 +0200
Aldo Conte <aldocontelk@gmail.com> wrote:

> On 5/13/26 00:32, Aldo Conte wrote:
> > 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.
> > 
> > Rewrite the read-modify-write pattern in tcs3472_powerdown() and
> > tcs3472_resume() so the new register value is computed first, written
> > to the chip, and committed to data->enable only on success.
> > 
> > Use a local 'dev = &client->dev' in tcs3472_probe() to keep the devm
> > calls compact.
> > 
> > Suggested-by: Andy Shevchenko <andy@kernel.org>
> > Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
> > ---
> > v2:
> > (Suggested by Andy)
> > - Rewrote read-modify-write in tcs3472_powerdown() and tcs3472_resume()
> > - Use local 'struct device *dev = &client->dev' in probe.
> > - Dropped "Compiled with W=1" and test details from the commit message.
> > 
> >   drivers/iio/light/tcs3472.c | 107 +++++++++++++++++-------------------
> >   1 file changed, 51 insertions(+), 56 deletions(-)
> > 
> > diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
> > index 4fd4fd74d0d6..7a6dc8360326 100644
> > --- a/drivers/iio/light/tcs3472.c
> > +++ b/drivers/iio/light/tcs3472.c
> > @@ -427,13 +427,38 @@ static const struct iio_info tcs3472_info = {
> >   	.attrs = &tcs3472_attribute_group,
> >   };
> >   
> > +static int tcs3472_powerdown(struct tcs3472_data *data)
> > +{
> > +	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
> > +	u8 value;
> > +	int ret;
> > +
> > +	guard(mutex)(&data->lock);
> > +
> > +	value = data->enable & ~enable_mask;
> > +
> > +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
> > +	if (ret)
> > +		return ret;
> > +
> > +	data->enable = value;
> > +
> > +	return 0;
> > +}
> > +
> > +static void tcs3472_powerdown_action(void *data)
> > +{
> > +	tcs3472_powerdown(data);
> > +}
> > +
> >   static int tcs3472_probe(struct i2c_client *client)
> >   {
> > +	struct device *dev = &client->dev;
> >   	struct tcs3472_data *data;
> >   	struct iio_dev *indio_dev;
> >   	int ret;
> >   
> > -	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> >   	if (indio_dev == NULL)
> >   		return -ENOMEM;
> >   
> 
> Hi Jonathan,
> 
> In v3 I have a small style cleanup: replacing 'if (indio_dev == NULL)'
> with 'if (!indio_dev)' in tcs3472_probe(). This was suggested by
> Joshua Crofts in his v2 review.
> 
> What do you prefer it is a separate patch
> or folded into the devm conversion patch (which already modifies the
> context next to that line).
> 
> Either way works for me. Which do you prefer?

Separate patch.  However, don't rush. This version wants to sit on list
for a least a few more days!  For something as complex at this set
I'd suggest at least a week between first few versions.  Maybe
accelerate if you have a bunch of tags and there is just a little thing
needed.

Jonathan

> 
> Thanks,
> Aldo
> 
> ...
> 
> 

Re: [PATCH v2 4/5] iio: light: tcs3472: implement wait time and sampling frequency

[Aldo Conte]

On 13/05/26 13:17, Andy Shevchenko wrote:
> On Wed, May 13, 2026 at 12:32:14AM +0200, Aldo Conte wrote:
>> The TCS3472 has a wait state controlled by the WEN bit in the ENABLE
>> register and the WAIT register, with an 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 control of the wait time through IIO_CHAN_INFO_SAMP_FREQ:
>>
>>    - Reading sampling_frequency returns the chip's current cycle time,
>>      computed as the sum of ATIME, the fixed RGBC initialization time
>>      and the wait time (which depends on WEN and WLONG).
>>
>>    - Writing sampling_frequency programs WTIME so that the resulting
>>      cycle period approximates the requested frequency. If the
>>      requested frequency cannot be reached with any
>>      non-zero wait time, WEN is disabled and the chip runs
>>      back-to-back conversions at the maximum rate allowed by ATIME.
>>      If the requested period exceeds the maximum WTIME range, WLONG
>>      is enabled to extend the wait step from 2.4 ms to 28.8 ms.
>>
>>    - The user's last requested frequency is stored in the driver's
>>      private data so that subsequent changes to integration_time
>>      recompute WTIME and preserve the requested sampling rate as
>>      closely as possible.
>>
>> Add TCS3472_ENABLE_WEN, TCS3472_ENABLE_RUN and TCS3472_CONFIG_WLONG
>> bit definitions. TCS3472_ENABLE_RUN bundles the bits
>> (AEN | PON | WEN) that are simultaneously set when the chip is in
>> running state and cleared during powerdown, and is used by
>> tcs3472_probe(), tcs3472_powerdown() and tcs3472_resume().
>>
>> Remove the "TODO: wait time" comment at the top of the file.
> 
>> Suggested-by: Andy Shevchenko <andy@kernel.org>
> 
> Inappropriate tag.
> You must not put tags on your own, if in doubt, ask first!
> 
> ...
> 
>> +#define TCS3472_ENABLE_RUN (TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON | \
>> +			    TCS3472_ENABLE_WEN)
> 
> Better style is
> 
> #define TCS3472_ENABLE_RUN						\
> 	(TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON | TCS3472_ENABLE_WEN)
> 
> ...
> 
>> +	cycle_us = div_u64((u64)PSEC_PER_SEC,
>> +			   (u64)val * USEC_PER_SEC + val2);
> 
> First of all, it's one line. Second, the divisor for this function is 32-bit.
> And at last the castings are not needed. I think I already told these...

In theory, val and val2 could have a value of INT_MAX. Therefore, the 
calculation INT_MAX * USEC_PER_SEC + INT_MAX results in a value of 
2,147,483,861,748,364,700, which can only be stored in 64 bits. This is, 
of course, an extremely rare case in which userspace writes an 
impossible frequency that is simply too high.
Therefore, div64_u64() should be used instead of div_u64().
Therefore, the following code would be required:

cycle_us = div64_u64(PSEC_PER_SEC, val * USEC_PER_SEC + val2);



> ...
> 
>> +	wtime = 256 - DIV_ROUND_CLOSEST_ULL(wait_us, 2400);
>> +	if (wtime < 0) {
>> +		wlong = true;
>> +		wtime = 256 - DIV_ROUND_CLOSEST_ULL(wait_us, 28800);
>> +	}
> 
> Why 64-bit divisions? Do you expect the wait_us be outside INT_MIN/INT_MAX range?
> This will need a comment and/or dropping the 64-bit arithmetics.

cycle_us_MIN is reached when (u64)val * USEC_PER_SEC + val2 is MAX and 
so is INT_MAX * USEC_PER_SEC + INT_MAX. Assuming this, the division 
would be 10⁶ / (INT_MAX * USEC_PER_SEC + INT_MAX) = 0.000465661, so 0.

Therefore, cycle_us_MIN would equal 0 and cycle_us_MAX would equal 
PSEC_PER_SEC/1, i.e. 10⁶; consequently, wait_us_MIN would equal −614400:

wait_us_min = 0−2400−612000= −614400

and wait_us_MAX would equal 999999995200:

wait_us_MAX = 1000000000000 −2400−2400 = 999999995200

This means that wait_us needs to be s64 if i am not wrong.
> 
>> +	wtime = clamp(wtime, 0, 255);
> 
> ...
> 
>> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_WTIME, wtime);
>> +	if (ret < 0)
> 
> What's the meaning of positive returned values? I think this function never
> does that. If I'm right, drop ' < 0' parts in all similar cases.
The i2c_smbus_write_byte_data() function returns a value of 0 on success 
and a value less than 0 in the event of an error. I do not understand 
this comment. Could you please explain it to me in more detail?

> 
>> +		return ret;
> 
> ...
> 
>> +		*val = USEC_PER_SEC / cycle_us;
>> +		*val2 = div_u64((u64)(USEC_PER_SEC % cycle_us) * USEC_PER_SEC,
>> +				cycle_us);
> 
> Is this even correct? We take modulo of cycle_us to get under the MICRO range,
> then multiply to MICRO (seconds) and divide by full cycle_us. I'm lost here.


> 
> ...
> 
>> +	cycle_us = tcs3472_cycle_time_us(data);
>> +	data->target_freq_hz = USEC_PER_SEC / cycle_us;
>> +	data->target_freq_uhz = div_u64((u64)(USEC_PER_SEC % cycle_us) *
>> +					USEC_PER_SEC, cycle_us);
> 
> Okay, this might help with the above... Can you deduplicate this division to
> a helper with a comment that explains the calculations behind?
> 
Something like this could be ok?

/*
  * Convert cycle time in microseconds to frequency in Hz and microhertz.
  *
  * Given:
  *   cycle_us = T, i.e. 1 cycle lasts T microseconds
  *
  * The frequency is:
  *   f = 10^6 / T    [Hz]
  *
  * We split it as:
  *   val = floor(10^6 / T)
  *   val2 = (10^6 mod T) * 10^6 / T   [microhertz]
  *
  * I.E.:
  *   val = 10^6 / T          (integer division)
  *   val2 = (10^6 % T) * 10^6 / T   (fraction scaled to microhertz)
  */
static void
us_cycle_to_freq_hz(int cycle_us, int *val, int *val2)
{
         *val = USEC_PER_SEC / cycle_us;
         *val2 = div_u64((u64)(USEC_PER_SEC % cycle_us) * USEC_PER_SEC,
                         cycle_us);
}


thank you,

Aldo

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

[Jonathan Cameron]

On Wed, 13 May 2026 00:32:13 +0200
Aldo Conte <aldocontelk@gmail.com> wrote:

> 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.
Hi Aldo,

This is a precursor. Do it without devm first (using effectively
same call but under and error label.  Then devm conversion will be a
no-op patch  - which is much easier to review.


> - 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.
> 
> Rewrite the read-modify-write pattern in tcs3472_powerdown() and
> tcs3472_resume() so the new register value is computed first, written
> to the chip, and committed to data->enable only on success.

This belongs in the guard() patch that enables that change.

> 
> Use a local 'dev = &client->dev' in tcs3472_probe() to keep the devm
> calls compact.

All the comments are not about the end result but rather about
how to break this up. 
1. Start with a fix for the failure to power down.  May not be one
   we bother backporting - but nice to be able to (move that ahead
   of the guard patch - note that will involve moving the powerdown()
   function without modifying it at that point. Will make later patches
   easier to read as a nice side effect.
2. Move the guard() related stuff to earlier patch - refactors as a direct
   result of using guard() belong there not in a patch doing anything else.
3. The devm conversion - should be easier to see it's like for like.
4. Remaining uses of struct device *dev in probe()

So effectively 3 patches + some stuff in earlier one.

Getting used to making series as readable as possible for reviewers
is a skill that takes some work!  It is easy to go too far the other
way as well so you may well get some review comments in future saying
to combine patches :(

Jonathan

> 
> Suggested-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
> ---
> v2:
> (Suggested by Andy)
> - Rewrote read-modify-write in tcs3472_powerdown() and tcs3472_resume()
> - Use local 'struct device *dev = &client->dev' in probe.
> - Dropped "Compiled with W=1" and test details from the commit message.
> 
>  drivers/iio/light/tcs3472.c | 107 +++++++++++++++++-------------------
>  1 file changed, 51 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
> index 4fd4fd74d0d6..7a6dc8360326 100644
> --- a/drivers/iio/light/tcs3472.c
> +++ b/drivers/iio/light/tcs3472.c
> @@ -427,13 +427,38 @@ static const struct iio_info tcs3472_info = {
>  	.attrs = &tcs3472_attribute_group,
>  };
>  
> +static int tcs3472_powerdown(struct tcs3472_data *data)
> +{
> +	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
> +	u8 value;
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
Whilst the move needs to be here, the refactor should be in
the previous patch (as guard() related).  Obviously bit tideous
to touch the code then move it but I think it will be easier to review.

> +
> +	value = data->enable & ~enable_mask;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
> +	if (ret)
> +		return ret;
> +
> +	data->enable = value;
> +
> +	return 0;
> +}
> +
> +static void tcs3472_powerdown_action(void *data)
> +{
> +	tcs3472_powerdown(data);
> +}
> +
>  static int tcs3472_probe(struct i2c_client *client)
>  {
> +	struct device *dev = &client->dev;
>  	struct tcs3472_data *data;
>  	struct iio_dev *indio_dev;
>  	int ret;
>  
> -	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
As below. I'd keep the switch to dev for a separate patch.
>  	if (indio_dev == NULL)
>  		return -ENOMEM;
>  
> @@ -453,9 +478,9 @@ static int tcs3472_probe(struct i2c_client *client)
>  		return ret;
>  
>  	if (ret == 0x44)
> -		dev_info(&client->dev, "TCS34721/34725 found\n");
> +		dev_info(dev, "TCS34721/34725 found\n");

Whilst it's good to put the dev in for the calls that are otherwise
changing it would be better to not touch anything that isn't.  Leave
those for a follow on patch 

.... : Use locale struct device * for remaining cases
or something like that.
Aim is to have this patch (and devm patches are hard to review) do
the minimum relevant stuff.

>  	else if (ret == 0x4d)
> -		dev_info(&client->dev, "TCS34723/34727 found\n");
> +		dev_info(dev, "TCS34723/34727 found\n");
>  	else
>  		return -ENODEV;
>  
> @@ -497,59 +522,26 @@ 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(dev, tcs3472_powerdown_action, data);
Do prior to this change set am I right in thinking the device simply wasn't
powered down on error?  If so I would fix that as a precursor patch (with an extra
label and direct call of tsc3472_powerdown())  Then do the devm conversion
on top of that.

> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_triggered_buffer_setup(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(dev, client->irq, NULL,
> +						tcs3472_event_handler,
> +						IRQF_TRIGGER_FALLING | IRQF_SHARED |
> +						IRQF_ONESHOT,
I'd break this as
						IRQF_TRIGGER_FALLING |
						IRQF_SHARED | IRQF_ONESHOT,
or
						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;
> -
> -	guard(mutex)(&data->lock);
> -
> -	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> -		data->enable & ~enable_mask);
> -	if (!ret)
> -		data->enable &= ~enable_mask;
> -
> -	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(dev, indio_dev);
>  }
>  
>  static int tcs3472_suspend(struct device *dev)
> @@ -563,17 +555,21 @@ 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 value;
> +	int ret;
>  
>  	guard(mutex)(&data->lock);
>  
> -	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> -		data->enable | enable_mask);
> -	if (!ret)
> -		data->enable |= enable_mask;
> +	value = data->enable | enable_mask;
>  
> -	return ret;
> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
> +	if (ret)
> +		return ret;
> +
> +	data->enable = value;

I think this change belongs in the previous patch - it's not related to devm
whereas it is related to the guard() change.

> +
> +	return 0;
>  }

Re: [PATCH v2 4/5] iio: light: tcs3472: implement wait time and sampling frequency

[Jonathan Cameron]

On Wed, 13 May 2026 00:32:14 +0200
Aldo Conte <aldocontelk@gmail.com> wrote:

> The TCS3472 has a wait state controlled by the WEN bit in the ENABLE
> register and the WAIT register, with an 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 control of the wait time through IIO_CHAN_INFO_SAMP_FREQ:
> 
>   - Reading sampling_frequency returns the chip's current cycle time,
>     computed as the sum of ATIME, the fixed RGBC initialization time
>     and the wait time (which depends on WEN and WLONG).
> 
>   - Writing sampling_frequency programs WTIME so that the resulting
>     cycle period approximates the requested frequency. If the
>     requested frequency cannot be reached with any
>     non-zero wait time, WEN is disabled and the chip runs
>     back-to-back conversions at the maximum rate allowed by ATIME.
>     If the requested period exceeds the maximum WTIME range, WLONG
>     is enabled to extend the wait step from 2.4 ms to 28.8 ms.

Tidy up wrap of this paragraph. Can go to 75 chars.

> 
>   - The user's last requested frequency is stored in the driver's
>     private data so that subsequent changes to integration_time
>     recompute WTIME and preserve the requested sampling rate as
>     closely as possible.
> 
> Add TCS3472_ENABLE_WEN, TCS3472_ENABLE_RUN and TCS3472_CONFIG_WLONG
> bit definitions. TCS3472_ENABLE_RUN bundles the bits
> (AEN | PON | WEN) that are simultaneously set when the chip is in
> running state and cleared during powerdown, and is used by
> tcs3472_probe(), tcs3472_powerdown() and tcs3472_resume().
> 
> Remove the "TODO: wait time" comment at the top of the file.
> 
> Suggested-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
A few comments inline though many may overlap with what Andy raised.

> ---
> v2:
> ( Suggested-by Andy )
> - Validation also rejects val > 0 with val2 < 0
> - cycle_us uses PSEC_PER_SEC (was USEC_PER_SEC * USEC_PER_SEC),
> - clamp() replaces manual WTIME bounds checks.
> - DIV_ROUND_CLOSEST_ULL() allows to avoid all the castings.
> - CONFIG read-modify-write uses a separate 'config' variable instead of the
>   ret = foo(ret).
> - Multi-line comments now follow kernel style.
> - Added comment explaining why wait_us can be negative.
> - target_freq_uhz uses div_u64() for 32-bit portability.
> - TCS3472_ENABLE_RUN introduced here with AEN | PON | WEN.
> - Test details moved to the cover letter.
> 
>  drivers/iio/light/tcs3472.c | 189 +++++++++++++++++++++++++++++++++---
>  1 file changed, 175 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
> index 7a6dc8360326..f8f70399a4dc 100644
> --- a/drivers/iio/light/tcs3472.c
> +++ b/drivers/iio/light/tcs3472.c

> @@ -135,6 +159,100 @@ static int tcs3472_req_data(struct tcs3472_data *data)
>  	return 0;
>  }
>  
> +static int tcs3472_set_sampling_freq(struct tcs3472_data *data,
> +				     int val, int val2)
> +{
> +	unsigned int atime_us;
> +	unsigned int init_us = 2400;
> +	u64 cycle_us;
> +	s64 wait_us;
> +	int wtime;
> +	bool wlong = false;
> +	u8 config;
> +	int ret;
> +
> +	if (val < 0 || val2 < 0 || (val == 0 && val2 == 0))
> +		return -EINVAL;
> +
> +	guard(mutex)(&data->lock);
> +
> +	atime_us = (256 - data->atime) * 2400;
> +	cycle_us = div_u64((u64)PSEC_PER_SEC,

Sashiko points out truncation on these. as the divisor is 32 bit
Probably div64_u64()
https://sashiko.dev/#/patchset/20260512223215.25596-1-aldocontelk%40gmail.com

Check for other instances of this.
> +			   (u64)val * USEC_PER_SEC + val2);
> +
> +	/*
> +	 * wait_us can be negative or smaller than the minimum wait step
> +	 * (2400 us) when the requested frequency is too high to be reached.
> +	 * In that case, disable WEN so that the chip can perform back-to-back
> +	 * conversions at the maximum rate permitted by the current ATIME.
> +	 */
> +	wait_us = (s64)cycle_us - init_us - atime_us;
> +
> +	if (wait_us < 2400) {
> +		if (data->enable & TCS3472_ENABLE_WEN) {
> +			data->enable &= ~TCS3472_ENABLE_WEN;
In resume you unconditionally use ENABLE_RUN

Seems broken if you have previously passed through this path

> +			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).

Wrap to 80 when no strong reason to do differently.

> +	 */
> +	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 - DIV_ROUND_CLOSEST_ULL(wait_us, 2400);
> +	if (wtime < 0) {
> +		wlong = true;
> +		wtime = 256 - DIV_ROUND_CLOSEST_ULL(wait_us, 28800);
> +	}
> +	wtime = clamp(wtime, 0, 255);
> +
> +	if (wlong != data->wlong) {
All this dancing to avoid writes would be better replaced with
regmap + regcache but that's a much bigger job.

> +		ret = i2c_smbus_read_byte_data(data->client, TCS3472_CONFIG);
> +		if (ret < 0)
> +			return ret;
> +
> +		config = ret;
> +		if (wlong)
> +			config |= TCS3472_CONFIG_WLONG;
> +		else
> +			config &= ~TCS3472_CONFIG_WLONG;
> +
> +		ret = i2c_smbus_write_byte_data(data->client, TCS3472_CONFIG,
> +						config);
> +		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_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val, int *val2, long mask)
> @@ -165,6 +283,14 @@ 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 = div_u64((u64)(USEC_PER_SEC % cycle_us) * USEC_PER_SEC,
> +				cycle_us);
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
>  	}
>  	return -EINVAL;
>  }
> @@ -175,6 +301,7 @@ static int tcs3472_write_raw(struct iio_dev *indio_dev,
>  {
>  	struct tcs3472_data *data = iio_priv(indio_dev);
>  	int i;
> +	int ret;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_CALIBSCALE:
> @@ -194,15 +321,29 @@ static int tcs3472_write_raw(struct iio_dev *indio_dev,
>  		if (val != 0)
>  			return -EINVAL;
>  		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);
> -			}
> -
> +			if (val2 != (256 - i) * 2400)
> +				continue;
> +
> +			data->atime = i;
> +			ret = i2c_smbus_write_byte_data(data->client,
> +							TCS3472_ATIME,
> +							data->atime);
> +			if (ret < 0)
> +				return ret;
> +
> +			/*
> +			 * ATIME just changed, so the cycle time changed too.
> +			 * Re-run the sampling frequency logic to recompute
> +			 * WTIME and preserve the user's last requested
> +			 * frequency.
> +			 */
> +			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;
>  }
> @@ -429,13 +570,12 @@ static const struct iio_info tcs3472_info = {
>  
>  static int tcs3472_powerdown(struct tcs3472_data *data)
>  {
> -	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
>  	u8 value;
>  	int ret;
>  
>  	guard(mutex)(&data->lock);
>  
> -	value = data->enable & ~enable_mask;
> +	value = data->enable & ~TCS3472_ENABLE_RUN;
>  
>  	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
>  	if (ret)
> @@ -456,6 +596,7 @@ static int tcs3472_probe(struct i2c_client *client)
>  	struct device *dev = &client->dev;
>  	struct tcs3472_data *data;
>  	struct iio_dev *indio_dev;
> +	unsigned int cycle_us;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> @@ -494,6 +635,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);

!! is kind of frowned upon though it is common in older kernel code.
Linus once got quite grumpy about it being tricky to read ;)

? 1 : 0; probably given driver isn't using FIELD_GET()/_PREP()


> +
>  	ret = i2c_smbus_read_word_data(data->client, TCS3472_AILT);
>  	if (ret < 0)
>  		return ret;
> @@ -515,13 +666,24 @@ 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_RUN;
>  	data->enable &= ~TCS3472_ENABLE_AIEN;
>  	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>  		data->enable);
>  	if (ret < 0)
>  		return ret;
>  
> +	/*
> +	 * Initialize target frequency from the chip's current state so
> +	 * that subsequent integration_time changes via IIO_CHAN_INFO_INT_TIME
> +	 * can preserve a meaningful sampling rate, even before userspace
> +	 * writes sampling_frequency for the first time.
Slightly odd wrap. Go to 80 chars for comments.

	 * Initialize target frequency from the chip's current state so that
	 * subsequent integration_time changes via IIO_CHAN_INFO_INT_TIME can
	 * preserve a meaningful sampling rate, even before userspace writes
	 * sampling_frequency for the first time.

It's one of those things where it doesn't really matter what the standard is
as long as we keep to it as visual consistency is always good for readability.
> +	 */
> +	cycle_us = tcs3472_cycle_time_us(data);
> +	data->target_freq_hz = USEC_PER_SEC / cycle_us;
> +	data->target_freq_uhz = div_u64((u64)(USEC_PER_SEC % cycle_us) *
> +					USEC_PER_SEC, cycle_us);

I see Andy already queried the maths here. I was about to do the same
as I'm failing to follow it.

> +
>  	ret = devm_add_action_or_reset(dev, tcs3472_powerdown_action, data);
>  	if (ret)
>  		return ret;

Re: [PATCH v2 5/5] iio: light: tcs3472: move standalone return to default case

[Jonathan Cameron]

On Wed, 13 May 2026 20:58:40 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Wed, May 13, 2026 at 06:12:25PM +0200, Aldo Conte wrote:
> > On 5/13/26 13:23, Andy Shevchenko wrote:  
> > > On Wed, May 13, 2026 at 12:32:15AM +0200, Aldo Conte wrote:  
> 
> ...
> 
> > Thanks for the review. Before I send v3, I want to make sure I split
> > the series the way you want.
> > 
> > My plan is to split v2 into 7 patches:
> > 
> >   1. sort headers (same as v2)
> >   2. convert locking to guard(mutex) (drop Suggested-by, clean up
> >      trigger_handler)
> >   3. replace == NULL with ! (add Suggested-by: Joshua Crofts)
> >   4. use devm for resource management (drop Suggested-by, remove the
> >      dev_info change, add devm_mutex_init)
I made a few suggestions in reply to that patch on stuff to move out
of it etc.

> >   5. use 'dev' for dev_info() calls (new, taken out of patch 4)
> >   6. implement wait time and sampling_frequency (drop Suggested-by,
> >      fix macro style, simplify cycle_us and all the other revisions)
> >   7. move standalone return to default case (same as v2)  
> 
> (Also collect tags for the patches that are not [drastically] changed, like #7
>  here.)
> 
> > Two things I am not sure about:
> > 
> >   - Should patch 3 ("replace == NULL with !") really be its own patch,
> >     or do you prefer it inside patch 4, since that line is right next
> >     to devm_iio_device_alloc?  
> 
> Jonathan is okay to combine in some cases, but I'm not sure if this is small
> enough. Ask him?

Slight preference for a separate patch - or just skip it.  There are lots
more things that could be tidied up in this driver I think. You don't have
to do them all!

> 
> >   - Is patch 5 (dev_info refactor) in the right place after the devm
> >     patch?  
> 
> Basically sort patches by severity, with this in mind seems you suggested
> the right order.

I would add for this that, to reduce churn it's fine to add the
struct device *dev = ... bit and use it in new devm calls. Just do the
remainder in the follow on commit.

> 
>