[PATCH v3 0/8] iio: tcs3472: cleanups, devm conversion and wait time

[PATCH v3 0/8] iio: tcs3472: cleanups, devm conversion and wait time

[Aldo Conte]

This is v3 of the TCS3472 series, addressing the review comments from
Andy Shevchenko, Jonathan Cameron and Joshua Crofts on v2.

The series has been reorganized into 8 patches as
requested by the reviewers: cleanup/bug-fix patches separated from
refactors, dev_info() conversion split out of the devm patch, and a
new precursor bug fix for powerdown on probe failure.

Patch 1: powers down the chip if probe fails after the chip has been
         enabled. Pre-existing bug, found by code inspection.
         v3:
         - New patch in v3, split out as a precursor bug fix.

Patch 2: sorts the #include block alphabetically.
         v3:
         - No changes since v2.

Patch 3: converts the remaining mutex_lock/unlock pairs to
         guard(mutex)(), with small cleanups that guard() enables
         (early returns on I2C error, !! -> ? 1 : 0).
         v3:
         - In write_event_config, powerdown, resume: early return on
           I2C error.
         - In read_event_config: !! -> ? 1 : 0.
         - Removed stray blank line in trigger_handler.
         - Subject shortened (no function list).

Patch 4: uses ! instead of explicit NULL check in probe.
         v3:
         - New patch (suggested by Joshua).

Patch 5: 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.
         v3:
         - Powerdown bug fix moved to patch 1.
         - Powerdown/resume RMW refactor moved to guard patch.
         - TCS3472_ENABLE_RUN define moved to wait time patch.
         - dev_info() conversion split into patch 6.
         - devm_mutex_init() used for the lock (suggested by Joshua).

Patch 6: uses the local 'struct device *dev' for the remaining
         dev_info() calls in probe.
         v3:
         - New patch, split out from the devm conversion.

Patch 7: moves the trailing return -EINVAL in tcs3472_read_raw() and
         tcs3472_write_raw() into explicit default: cases.
         v3:
         - No code changes since v2.
         - Carried over Reviewed-by tags from Andy and Joshua.

Patch 8: implements wait time support exposed through
         sampling_frequency, closing a long-standing TODO. WTIME is
         recomputed when integration_time changes to preserve the
         requested frequency. WEN is disabled when the frequency is
         too high; WLONG is enabled when the period exceeds WTIME
         range.
         v3:
         - TCS3472_ENABLE_RUN: new macro style (name+tab+backslash).
         - cycle_us: div64_u64() and (u64)val cast for 32-bit safety.
         - wait_us: kept s64 with comment explaining the range.
         - 'if (ret)' instead of 'if (ret < 0)' after i2c writes.
         - New helper tcs3472_cycle_to_freq() deduplicates val/val2.
         - !! -> ? 1 : 0 in probe (WLONG extraction).
         - Fix in tcs3472_resume: preserve WEN from data->enable
           instead of forcing TCS3472_ENABLE_RUN, so a user-set
           sampling_frequency that disabled WEN is honored.

Testing:

All eight 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 2 (sort headers):
  - Driver loads cleanly; RGBC channel reads return reasonable values.
  - No warnings in dmesg.

Patch 3 (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 4 (! instead of NULL check):
  - Driver loads cleanly; behavior identical to previous patch.

Patch 5 (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 6 (use 'dev' for dev_info):
  - Driver loads cleanly; "TCS34721/34725 found" or "TCS34723/34727
    found" appears in dmesg as before.

Patch 7 (default case):
  - All read_raw and write_raw cases handle attributes identically
    to the previous patch.
  - Invalid inputs still rejected with -EINVAL through the explicit
    default: case.

Patch 8 (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.
  - 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.024 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.
  - Suspend/resume cycle preserves WEN state: after disabling WEN
    via high sampling_frequency, suspend then resume leaves ENABLE
    at 0x03 (WEN not forced back on).
  - After rmmod, ENABLE reads back as 0x00.

Aldo Conte (8):
  iio: tcs3472: power down chip on probe failure
  iio: tcs3472: sort headers alphabetically
  iio: tcs3472: convert remaining locking to guard(mutex)
  iio: tcs3472: use ! instead of explicit NULL check
  iio: tcs3472: use devm for resource management
  iio: tcs3472: use local struct device * for remaining cases
  iio: tcs3472: move standalone return to default case
  iio: tcs3472: implement wait time and sampling frequency

 drivers/iio/light/tcs3472.c | 416 ++++++++++++++++++++++++++----------
 1 file changed, 303 insertions(+), 113 deletions(-)

-- 
2.54.0

[PATCH v3 1/8] iio: tcs3472: power down chip on probe failure

[Aldo Conte]

If tcs3472_probe() fails after enabling the chip (by writing PON | AEN
to the ENABLE register), the error paths return without powering down
the device.

Add an 'error_powerdown' label at the end of the cleanup chain that
calls tcs3472_powerdown() to power down the chip. The existing label
cascade is rerouted to fall through to the new label.

Move tcs3472_powerdown() above tcs3472_probe() so the probe can call
it without a forward declaration.

Found by code inspection while reviewing the probe error paths in
preparation for the devm_ conversion.

Fixes: eb869ade30a6 ("iio: Add tcs3472 color light sensor driver")
Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
v3:
- New patch: extracted bug fix from devm conversion.

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

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index 12429a3261b3..849ca7885d71 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -440,6 +440,23 @@ 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 int tcs3472_probe(struct i2c_client *client)
 {
 	struct tcs3472_data *data;
@@ -513,7 +530,7 @@ static int tcs3472_probe(struct i2c_client *client)
 	ret = iio_triggered_buffer_setup(indio_dev, NULL,
 		tcs3472_trigger_handler, NULL);
 	if (ret < 0)
-		return ret;
+		goto error_powerdown;
 
 	if (client->irq) {
 		ret = request_threaded_irq(client->irq, NULL,
@@ -536,23 +553,8 @@ static int tcs3472_probe(struct i2c_client *client)
 		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);
-
+error_powerdown:
+	tcs3472_powerdown(data);
 	return ret;
 }
 
-- 
2.54.0

[PATCH v3 2/8] iio: 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>
Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
v3:
- No changes.

 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 849ca7885d71..1803bb882804 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 v3 3/8] iio: tcs3472: convert remaining locking to guard(mutex)

[Aldo Conte]

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.

While the conversion is in progress, take the opportunity to make a
few small cleanups that guard() enables:

  - In tcs3472_read_event_config(), replace '!!(...)' with
    '(...) ? 1 : 0' for readability.

  - In tcs3472_write_event_config(), tcs3472_powerdown() and
    tcs3472_resume() use an early return on the I2C
    write failure path.

No functional change.

Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
v3:
- Drop Suggested-by: Andy (inappropriate tag).
- In read_event_config(): !! -> ? 1 : 0.
- In write_event_config(): early return on I2C error.
- In powerdown() and resume(): early return on I2C error.
- Remove stray blank line in trigger_handler().

 drivers/iio/light/tcs3472.c | 76 ++++++++++++++++---------------------
 1 file changed, 32 insertions(+), 44 deletions(-)

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index 1803bb882804..b31714040a02 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) ? 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;
 
@@ -339,12 +326,13 @@ static int tcs3472_write_event_config(struct iio_dev *indio_dev,
 	if (enable_old != data->enable) {
 		ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
 						data->enable);
-		if (ret)
+		if (ret) {
 			data->enable = enable_old;
+			return ret;
+		}
 	}
-	mutex_unlock(&data->lock);
 
-	return ret;
+	return 0;
 }
 
 static irqreturn_t tcs3472_event_handler(int irq, void *priv)
@@ -445,16 +433,16 @@ 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;
+	if (ret)
+		return ret;
 
-	mutex_unlock(&data->lock);
+	data->enable &= ~enable_mask;
 
-	return ret;
+	return 0;
 }
 
 static int tcs3472_probe(struct i2c_client *client)
@@ -583,16 +571,16 @@ 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;
 
-	mutex_unlock(&data->lock);
+	data->enable |= enable_mask;
 
-	return ret;
+	return 0;
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(tcs3472_pm_ops, tcs3472_suspend,
-- 
2.54.0

[PATCH v3 4/8] iio: tcs3472: use ! instead of explicit NULL check

[Aldo Conte]

Replace 'if (indio_dev == NULL)' with 'if (!indio_dev)' in
tcs3472_probe() to follow the preferred kernel style.

No functional change.

Suggested-by: Joshua Crofts <joshua.crofts1@gmail.com>
Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
v3:
- New patch.

 drivers/iio/light/tcs3472.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index b31714040a02..d7b5805108f6 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -452,7 +452,7 @@ static int tcs3472_probe(struct i2c_client *client)
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
-	if (indio_dev == NULL)
+	if (!indio_dev)
 		return -ENOMEM;
 
 	data = iio_priv(indio_dev);
-- 
2.54.0

[PATCH v3 5/8] iio: 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.
- 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().
- Replace mutex_init() with devm_mutex_init().
- Remove tcs3472_remove() as all cleanup is now handled by devm.

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

Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
v3:
- Drop Suggested-by: Andy (inappropriate tag).
- Drop powerdown bug fix (now patch 1).
- Drop powerdown/resume RMW refactor (now in guard patch).
- Drop TCS3472_ENABLE_RUN define (now in wait time patch).
- Drop dev_info() conversion (now in separate patch).
- Add devm_mutex_init() for the lock (suggested by Joshua).

 drivers/iio/light/tcs3472.c | 63 +++++++++++++++----------------------
 1 file changed, 26 insertions(+), 37 deletions(-)

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index d7b5805108f6..4966abf83c18 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -17,6 +17,7 @@
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/pm.h>
 
 #include <linux/iio/buffer.h>
@@ -445,20 +446,28 @@ static int tcs3472_powerdown(struct tcs3472_data *data)
 	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)
 		return -ENOMEM;
 
 	data = iio_priv(indio_dev);
 	i2c_set_clientdata(client, indio_dev);
 	data->client = client;
-	mutex_init(&data->lock);
+	ret = devm_mutex_init(dev, &data->lock);
+	if (ret)
+		return ret;
 
 	indio_dev->info = &tcs3472_info;
 	indio_dev->name = TCS3472_DRV_NAME;
@@ -515,46 +524,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(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)
-		goto error_powerdown;
+		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);
-error_powerdown:
-	tcs3472_powerdown(data);
-	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)
@@ -598,7 +588,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 v3 6/8] iio: tcs3472: use local struct device * for remaining cases

[Aldo Conte]

Use the local 'struct device *dev' variable introduced for the devm
calls also for the dev_info() calls in tcs3472_probe(), to keep the
probe function consistent.

No functional change.

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

 drivers/iio/light/tcs3472.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index 4966abf83c18..f74e6633db45 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -480,9 +480,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;
 
-- 
2.54.0

[PATCH v3 7/8] iio: 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>
Reviewed-by: Andy Shevchenko <andy@kernel.org>
Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
v3:
- No changes.

 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 f74e6633db45..1f597ca93697 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -166,8 +166,9 @@ static int tcs3472_read_raw(struct iio_dev *indio_dev,
 		*val = 0;
 		*val2 = (256 - data->atime) * 2400;
 		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
 	}
-	return -EINVAL;
 }
 
 static int tcs3472_write_raw(struct iio_dev *indio_dev,
@@ -204,8 +205,9 @@ static int tcs3472_write_raw(struct iio_dev *indio_dev,
 
 		}
 		return -EINVAL;
+	default:
+		return -EINVAL;
 	}
-	return -EINVAL;
 }
 
 /*
-- 
2.54.0

[PATCH v3 8/8] iio: 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().

In tcs3472_resume(), only PON and AEN are re-asserted while WEN is
preserved from data->enable.

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

Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
---
v3:
- Drop Suggested-by: Andy (inappropriate tag).
- TCS3472_ENABLE_RUN: new style.
- cycle_us: use div64_u64().
- cycle_us: cast val to u64 for 32-bit safety.
- wait_us: keep s64; added comment explaining the range bounds.
- Use 'if (ret)' instead of 'if (ret < 0)' after i2c writes.
- New helper tcs3472_cycle_to_freq() deduplicates val/val2 logic.
- !! -> ? 1 : 0 in probe.
- Fix tcs3472_resume(): preserve WEN from data->enable instead of
  forcing TCS3472_ENABLE_RUN.

 drivers/iio/light/tcs3472.c | 245 +++++++++++++++++++++++++++++++++---
 1 file changed, 227 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index 1f597ca93697..81549d7eba35 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -9,8 +9,6 @@
  * TCS34727)
  *
  * Datasheet: http://ams.com/eng/content/download/319364/1117183/file/TCS3472_Datasheet_EN_v2.pdf
- *
- * TODO: wait time
  */
 
 #include <linux/cleanup.h>
@@ -53,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[] = {
@@ -91,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, \
@@ -114,6 +121,47 @@ static const struct iio_chan_spec tcs3472_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(4),
 };
 
+/*
+ * The chip's cycle time is the sum of three components:
+ *   - ATIME: the programmable RGBC integration time.
+ *   - The fixed RGBC initialization time (2400 us).
+ *   - WTIME: the wait time, used only if WEN is set. If WLONG is active,
+ *     the wait step is multiplied by 12 (2400 us -> 28800 us).
+ */
+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 atime_us + init_us + wtime_us;
+}
+
+/*
+ * Convert a cycle time in microseconds to a frequency in Hz and microhertz.
+ *
+ * Given cycle_us = T (the cycle period in microseconds), the corresponding
+ * frequency is:
+ *   f = 1e6 / T  [Hz]
+ *
+ * The result is split into the IIO_VAL_INT_PLUS_MICRO format:
+ *   val  = floor(1e6 / T)                [Hz]
+ *   val2 = (1e6 mod T) * 1e6 / T         [microhertz]
+ */
+static void tcs3472_cycle_to_freq(unsigned 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);
+}
+
 static int tcs3472_req_data(struct tcs3472_data *data)
 {
 	int tries = 50;
@@ -166,16 +214,131 @@ 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);
+
+		tcs3472_cycle_to_freq(cycle_us, val, val2);
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
 	default:
 		return -EINVAL;
 	}
 }
 
+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 = div64_u64(PSEC_PER_SEC,
+			     (u64)val * USEC_PER_SEC + val2);
+
+	/*
+	 * wait_us can be negative when the requested frequency is too high
+	 * to be reached, or very large when the requested frequency is
+	 * close to zero. Use s64 to cover the full range:
+	 *
+	 *   cycle_us = PSEC_PER_SEC / (val * USEC_PER_SEC + val2)
+	 *
+	 * The divisor of the formula above reaches its maximum when
+	 * val = val2 = INT_MAX:
+	 *  INT_MAX * USEC_PER_SEC + INT_MAX = ~2.15e18
+	 * so cycle_us_min = floor(1e12 / 2.15e18) = 0.
+	 *
+	 * The divisor reaches its minimum (1) when val = 0 and val2 = 1,
+	 * so cycle_us_max = 1e12 / 1 = 1e12.
+	 *
+	 * Therefore:
+	 *   wait_us_min = 0 - 2400 - 612000 = -616800
+	 *   wait_us_max = 1e12 - 2400 - 2400 = 999999995200
+	 *
+	 * Both fit comfortably in s64.
+	 */
+	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)
+				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)
+			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)
+			return ret;
+
+		data->wlong = wlong;
+	}
+
+	ret = i2c_smbus_write_byte_data(data->client, TCS3472_WTIME, wtime);
+	if (ret)
+		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 ret;
 	int i;
 
 	switch (mask) {
@@ -196,15 +359,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)
+				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);
 	default:
 		return -EINVAL;
 	}
@@ -434,16 +611,15 @@ 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;
 
 	guard(mutex)(&data->lock);
 
 	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
-					data->enable & ~enable_mask);
+					data->enable & ~TCS3472_ENABLE_RUN);
 	if (ret)
 		return ret;
 
-	data->enable &= ~enable_mask;
+	data->enable &= ~TCS3472_ENABLE_RUN;
 
 	return 0;
 }
@@ -458,6 +634,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));
@@ -498,6 +675,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) ? 1 : 0;
+
 	ret = i2c_smbus_read_word_data(data->client, TCS3472_AILT);
 	if (ret < 0)
 		return ret;
@@ -518,14 +705,29 @@ static int tcs3472_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
-	/* enable device */
-	data->enable = ret | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN;
+	/*
+	 * Enable the chip in its full running state, including WEN. The
+	 * actual wait time is controlled by the WTIME and WLONG registers,
+	 * which retain their power-on defaults until userspace writes to
+	 * sampling_frequency.
+	 */
+	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);
+	tcs3472_cycle_to_freq(cycle_us, &data->target_freq_hz,
+			      &data->target_freq_uhz);
+
 	ret = devm_add_action_or_reset(dev, tcs3472_powerdown_action, data);
 	if (ret)
 		return ret;
@@ -561,16 +763,23 @@ 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;
 
 	guard(mutex)(&data->lock);
 
-	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
-		data->enable | enable_mask);
+	/*
+	 * On resume, only PON and AEN need to be re-asserted. WEN must not
+	 * be forced on: its desired state was set by the user via
+	 * sampling_frequency and is already stored in data->enable, so we
+	 * read it from there to preserve the user's choice.
+	 */
+	value = data->enable | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN;
+
+	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
 	if (ret)
 		return ret;
 
-	data->enable |= enable_mask;
+	data->enable = value;
 
 	return 0;
 }
-- 
2.54.0

Re: [PATCH v3 3/8] iio: tcs3472: convert remaining locking to guard(mutex)

[Jonathan Cameron]

>  
>  static irqreturn_t tcs3472_event_handler(int irq, void *priv)
> @@ -445,16 +433,16 @@ 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;
> +	if (ret)
> +		return ret;
>  
> -	mutex_unlock(&data->lock);
> +	data->enable &= ~enable_mask;

Hi Aldo,

I'm going to comment on a similar (but worse) case in review of patch 8.
If you do rework that to avoid falsely setting cached state that we can't
rely on being right, you could switch to using a local variable here.

	u8 enable = data->enable & ~(TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON);

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

	data->enable = enable;

	return 0;

(similar for other cases).  Given it's unrelated to what you are doing here
that doesn't stop me picking up this patch as is.

I'm working through the series, but so far 1-3 applied to the testing branch
of iio.git.

Thanks,

Jonathan


> -	return ret;
> +	return 0;
>  }
>  
>  static int tcs3472_probe(struct i2c_client *client)
> @@ -583,16 +571,16 @@ 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;
>  
> -	mutex_unlock(&data->lock);
> +	data->enable |= enable_mask;
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static DEFINE_SIMPLE_DEV_PM_OPS(tcs3472_pm_ops, tcs3472_suspend,

Re: [PATCH v3 4/8] iio: tcs3472: use ! instead of explicit NULL check

[Jonathan Cameron]

On Fri, 22 May 2026 14:34:15 +0200
Aldo Conte <aldocontelk@gmail.com> wrote:

> Replace 'if (indio_dev == NULL)' with 'if (!indio_dev)' in
> tcs3472_probe() to follow the preferred kernel style.
> 
> No functional change.
> 
> Suggested-by: Joshua Crofts <joshua.crofts1@gmail.com>
> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
Applied.
Thanks,
J

Re: [PATCH v3 5/8] iio: tcs3472: use devm for resource management

[Jonathan Cameron]

On Fri, 22 May 2026 14:34:16 +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.
> - 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().
> - Replace mutex_init() with devm_mutex_init().
> - Remove tcs3472_remove() as all cleanup is now handled by devm.
> 
> Use a local 'dev = &client->dev' in tcs3472_probe() to keep the devm
> calls compact.
> 
> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
Applied to the testing branch of iio.git.

Thanks,

Jonathan

Re: [PATCH v3 6/8] iio: tcs3472: use local struct device * for remaining cases

[Jonathan Cameron]

On Fri, 22 May 2026 14:34:17 +0200
Aldo Conte <aldocontelk@gmail.com> wrote:

> Use the local 'struct device *dev' variable introduced for the devm
> calls also for the dev_info() calls in tcs3472_probe(), to keep the
> probe function consistent.
> 
> No functional change.
> 
> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
Hmm.  My first instinct was to say drop these prints - because 'obviously'
the name attribute provides this information and doesn't fill up the kernel
log.  Except (sigh) it doesn't.  The driver reports "tcs3472" for all parts
supported.   Fun question of whether we could get away with fixing that.
I think on balance doing so is too risky wrt to possible regression reports
so we are stuck.

That brings us back around to this change and argues in favour of keeping
the prints - so I guess we do that.

Applied to the testing branch of iio.git.

Thanks,

Jonathan

> ---
> v3:
> - New patch
> 
>  drivers/iio/light/tcs3472.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
> index 4966abf83c18..f74e6633db45 100644
> --- a/drivers/iio/light/tcs3472.c
> +++ b/drivers/iio/light/tcs3472.c
> @@ -480,9 +480,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;
>  

Re: [PATCH v3 8/8] iio: tcs3472: implement wait time and sampling frequency

[Jonathan Cameron]

On Fri, 22 May 2026 14:34:19 +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.
> 
>   - 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().
> 
> In tcs3472_resume(), only PON and AEN are re-asserted while WEN is
> preserved from data->enable.
> 
> Remove the "TODO: wait time" comment at the top of the file.
Hi Aldo,

Please look at Sashiko's comments on this particular patch
(the others are about an issue that has nothing to do with your series)

https://sashiko.dev/#/patchset/20260522123420.45495-1-aldocontelk%40gmail.com

The suggestion is that you need to make the timeout in 
tcs3472_req_data() longer given with these settings the capture may
take a lot longer than previously.

There is some other general good practice stuff in there. For example:
> +			data->enable &= ~TCS3472_ENABLE_WEN;
> +			ret = i2c_smbus_write_byte_data(data->client,
> +							TCS3472_ENABLE,
> +							data->enable);

if you were to use a local variable to do the masking on enable
	
	enable = data->enable & ~TCS3472_ENABLE_WEN;
	ret = i2c_smbus_write_byte_data(data->client,..., enable);
	if (ret)
		return ret;

	data->enable = enable;

You'd ensure that 'to best effort' the cached value is not updated
if the write fails.  The approx (possibly correct) arguement is that
any failure in i2c_smbus_write_byte_data() is more likely to have
left the original state alone than successfully written the new one.
Or more generally the principle that in ideal world any call that fails
has no side effects.

Anyhow, take a look at all the feedback.  If you disagree with any of
it feel free to add a note to the change log of the next version.


Ah. I've just noticed that as part of this you did make the change
I suggested in patch 3. Oops - I should have checked.  It is fine
in here.

I didn't crop so that you'd have full context visible. A few specific
cases and comments inline.

Note I've picked up patches 1-7 so it's just this one that needs
revisions for v4.

thanks,

Jonathan


> 
> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
> ---
> v3:
> - Drop Suggested-by: Andy (inappropriate tag).
> - TCS3472_ENABLE_RUN: new style.
> - cycle_us: use div64_u64().
> - cycle_us: cast val to u64 for 32-bit safety.
> - wait_us: keep s64; added comment explaining the range bounds.
> - Use 'if (ret)' instead of 'if (ret < 0)' after i2c writes.
> - New helper tcs3472_cycle_to_freq() deduplicates val/val2 logic.
> - !! -> ? 1 : 0 in probe.
> - Fix tcs3472_resume(): preserve WEN from data->enable instead of
>   forcing TCS3472_ENABLE_RUN.
> 
>  drivers/iio/light/tcs3472.c | 245 +++++++++++++++++++++++++++++++++---
>  1 file changed, 227 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
> index 1f597ca93697..81549d7eba35 100644
> --- a/drivers/iio/light/tcs3472.c
> +++ b/drivers/iio/light/tcs3472.c
> @@ -9,8 +9,6 @@
>   * TCS34727)
>   *
>   * Datasheet: http://ams.com/eng/content/download/319364/1117183/file/TCS3472_Datasheet_EN_v2.pdf
> - *
> - * TODO: wait time
>   */
>  
>  #include <linux/cleanup.h>
> @@ -53,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[] = {
> @@ -91,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, \
> @@ -114,6 +121,47 @@ static const struct iio_chan_spec tcs3472_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(4),
>  };
>  
> +/*
> + * The chip's cycle time is the sum of three components:
> + *   - ATIME: the programmable RGBC integration time.
> + *   - The fixed RGBC initialization time (2400 us).
> + *   - WTIME: the wait time, used only if WEN is set. If WLONG is active,
> + *     the wait step is multiplied by 12 (2400 us -> 28800 us).
> + */
> +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 atime_us + init_us + wtime_us;
> +}
> +
> +/*
> + * Convert a cycle time in microseconds to a frequency in Hz and microhertz.
> + *
> + * Given cycle_us = T (the cycle period in microseconds), the corresponding
> + * frequency is:
> + *   f = 1e6 / T  [Hz]
> + *
> + * The result is split into the IIO_VAL_INT_PLUS_MICRO format:
> + *   val  = floor(1e6 / T)                [Hz]
> + *   val2 = (1e6 mod T) * 1e6 / T         [microhertz]
> + */
> +static void tcs3472_cycle_to_freq(unsigned 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);
> +}
> +
>  static int tcs3472_req_data(struct tcs3472_data *data)
>  {
>  	int tries = 50;
> @@ -166,16 +214,131 @@ 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);
> +
> +		tcs3472_cycle_to_freq(cycle_us, val, val2);
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
>  	default:
>  		return -EINVAL;
>  	}
>  }
>  
> +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 = div64_u64(PSEC_PER_SEC,
> +			     (u64)val * USEC_PER_SEC + val2);
> +
> +	/*
> +	 * wait_us can be negative when the requested frequency is too high
> +	 * to be reached, or very large when the requested frequency is
> +	 * close to zero. Use s64 to cover the full range:
> +	 *
> +	 *   cycle_us = PSEC_PER_SEC / (val * USEC_PER_SEC + val2)
> +	 *
> +	 * The divisor of the formula above reaches its maximum when
> +	 * val = val2 = INT_MAX:
> +	 *  INT_MAX * USEC_PER_SEC + INT_MAX = ~2.15e18
> +	 * so cycle_us_min = floor(1e12 / 2.15e18) = 0.
> +	 *
> +	 * The divisor reaches its minimum (1) when val = 0 and val2 = 1,
> +	 * so cycle_us_max = 1e12 / 1 = 1e12.
> +	 *
> +	 * Therefore:
> +	 *   wait_us_min = 0 - 2400 - 612000 = -616800
> +	 *   wait_us_max = 1e12 - 2400 - 2400 = 999999995200
> +	 *
> +	 * Both fit comfortably in s64.
> +	 */
> +	wait_us = (s64)cycle_us - init_us - atime_us;
> +
> +	if (wait_us < 2400) {
> +		if (data->enable & TCS3472_ENABLE_WEN) {
> +			data->enable &= ~TCS3472_ENABLE_WEN;
As below.
> +			ret = i2c_smbus_write_byte_data(data->client,
> +							TCS3472_ENABLE,
> +							data->enable);
> +			if (ret)
> +				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;

Similar to below. Can we only update data->enable if the write succeeds.

> +		ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> +						data->enable);
> +		if (ret)
> +			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)
> +			return ret;
> +
> +		data->wlong = wlong;
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_WTIME, wtime);
> +	if (ret)
> +		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 ret;
>  	int i;
>  
>  	switch (mask) {
> @@ -196,15 +359,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)
> +				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);
>  	default:
>  		return -EINVAL;
>  	}
> @@ -434,16 +611,15 @@ 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;
>  
>  	guard(mutex)(&data->lock);
>  
>  	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> -					data->enable & ~enable_mask);
> +					data->enable & ~TCS3472_ENABLE_RUN);
>  	if (ret)
>  		return ret;
>  
> -	data->enable &= ~enable_mask;
> +	data->enable &= ~TCS3472_ENABLE_RUN;
This would be more consistent and easier to read if you have
u8 value;

	value = data->enable & ~TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON
	value &= ~TCS3472_ENABLE_RUN;

	ret = i2c_...
	if (ret)
		return ret;

	data->enable = value;

	return 0;

Or something along those lines. No need for separate enable_mask local
variable.

	>  
>  	return 0;
>  }
> @@ -458,6 +634,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));
> @@ -498,6 +675,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) ? 1 : 0;
> +
>  	ret = i2c_smbus_read_word_data(data->client, TCS3472_AILT);
>  	if (ret < 0)
>  		return ret;
> @@ -518,14 +705,29 @@ static int tcs3472_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;
>  
> -	/* enable device */
> -	data->enable = ret | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN;
> +	/*
> +	 * Enable the chip in its full running state, including WEN. The
> +	 * actual wait time is controlled by the WTIME and WLONG registers,
> +	 * which retain their power-on defaults until userspace writes to
> +	 * sampling_frequency.
> +	 */
> +	data->enable = ret | TCS3472_ENABLE_RUN;
>  	data->enable &= ~TCS3472_ENABLE_AIEN;

Here is a case that should look more like the one that follows.
Use a local variable to build what is written and update the cache
only if the write succeeds.

>  	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);
> +	tcs3472_cycle_to_freq(cycle_us, &data->target_freq_hz,
> +			      &data->target_freq_uhz);
> +
>  	ret = devm_add_action_or_reset(dev, tcs3472_powerdown_action, data);
>  	if (ret)
>  		return ret;
> @@ -561,16 +763,23 @@ 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;
>  
>  	guard(mutex)(&data->lock);
>  
> -	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> -		data->enable | enable_mask);
> +	/*
> +	 * On resume, only PON and AEN need to be re-asserted. WEN must not
> +	 * be forced on: its desired state was set by the user via
> +	 * sampling_frequency and is already stored in data->enable, so we
> +	 * read it from there to preserve the user's choice.
> +	 */
> +	value = data->enable | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
>  	if (ret)
>  		return ret;
>  
> -	data->enable |= enable_mask;
> +	data->enable = value;
>  
>  	return 0;
>  }

Re: [PATCH v3 8/8] iio: tcs3472: implement wait time and sampling frequency

[Aldo Conte]

On 26/05/26 15:11, Jonathan Cameron wrote:
> On Fri, 22 May 2026 14:34:19 +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.
>>
>>    - 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().
>>
>> In tcs3472_resume(), only PON and AEN are re-asserted while WEN is
>> preserved from data->enable.
>>
>> Remove the "TODO: wait time" comment at the top of the file.
> Hi Aldo,
> 
> Please look at Sashiko's comments on this particular patch
> (the others are about an issue that has nothing to do with your series)
> 
> https://sashiko.dev/#/patchset/20260522123420.45495-1-aldocontelk%40gmail.com
> 
> The suggestion is that you need to make the timeout in
> tcs3472_req_data() longer given with these settings the capture may
> take a lot longer than previously.
> 
> There is some other general good practice stuff in there. For example:
>> +			data->enable &= ~TCS3472_ENABLE_WEN;
>> +			ret = i2c_smbus_write_byte_data(data->client,
>> +							TCS3472_ENABLE,
>> +							data->enable);
> 
> if you were to use a local variable to do the masking on enable
> 	
> 	enable = data->enable & ~TCS3472_ENABLE_WEN;
> 	ret = i2c_smbus_write_byte_data(data->client,..., enable);
> 	if (ret)
> 		return ret;
> 
> 	data->enable = enable;
> 
> You'd ensure that 'to best effort' the cached value is not updated
> if the write fails.  The approx (possibly correct) arguement is that
> any failure in i2c_smbus_write_byte_data() is more likely to have
> left the original state alone than successfully written the new one.
> Or more generally the principle that in ideal world any call that fails
> has no side effects.
> 
> Anyhow, take a look at all the feedback.  If you disagree with any of
> it feel free to add a note to the change log of the next version.
> 
> 
> Ah. I've just noticed that as part of this you did make the change
> I suggested in patch 3. Oops - I should have checked.  It is fine
> in here.
> 
> I didn't crop so that you'd have full context visible. A few specific
> cases and comments inline.
> 
> Note I've picked up patches 1-7 so it's just this one that needs
> revisions for v4.
> 
> thanks,
> 
> Jonathan
> 
> 
>>
>> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
>> ---
>> v3:
>> - Drop Suggested-by: Andy (inappropriate tag).
>> - TCS3472_ENABLE_RUN: new style.
>> - cycle_us: use div64_u64().
>> - cycle_us: cast val to u64 for 32-bit safety.
>> - wait_us: keep s64; added comment explaining the range bounds.
>> - Use 'if (ret)' instead of 'if (ret < 0)' after i2c writes.
>> - New helper tcs3472_cycle_to_freq() deduplicates val/val2 logic.
>> - !! -> ? 1 : 0 in probe.
>> - Fix tcs3472_resume(): preserve WEN from data->enable instead of
>>    forcing TCS3472_ENABLE_RUN.
>>
>>   drivers/iio/light/tcs3472.c | 245 +++++++++++++++++++++++++++++++++---
>>   1 file changed, 227 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
>> index 1f597ca93697..81549d7eba35 100644
>> --- a/drivers/iio/light/tcs3472.c
>> +++ b/drivers/iio/light/tcs3472.c
>> @@ -9,8 +9,6 @@
>>    * TCS34727)
>>    *
>>    * Datasheet: http://ams.com/eng/content/download/319364/1117183/file/TCS3472_Datasheet_EN_v2.pdf
>> - *
>> - * TODO: wait time
>>    */
>>   
>>   #include <linux/cleanup.h>
>> @@ -53,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[] = {
>> @@ -91,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, \
>> @@ -114,6 +121,47 @@ static const struct iio_chan_spec tcs3472_channels[] = {
>>   	IIO_CHAN_SOFT_TIMESTAMP(4),
>>   };
>>   
>> +/*
>> + * The chip's cycle time is the sum of three components:
>> + *   - ATIME: the programmable RGBC integration time.
>> + *   - The fixed RGBC initialization time (2400 us).
>> + *   - WTIME: the wait time, used only if WEN is set. If WLONG is active,
>> + *     the wait step is multiplied by 12 (2400 us -> 28800 us).
>> + */
>> +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 atime_us + init_us + wtime_us;
>> +}
>> +
>> +/*
>> + * Convert a cycle time in microseconds to a frequency in Hz and microhertz.
>> + *
>> + * Given cycle_us = T (the cycle period in microseconds), the corresponding
>> + * frequency is:
>> + *   f = 1e6 / T  [Hz]
>> + *
>> + * The result is split into the IIO_VAL_INT_PLUS_MICRO format:
>> + *   val  = floor(1e6 / T)                [Hz]
>> + *   val2 = (1e6 mod T) * 1e6 / T         [microhertz]
>> + */
>> +static void tcs3472_cycle_to_freq(unsigned 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);
>> +}
>> +
>>   static int tcs3472_req_data(struct tcs3472_data *data)
>>   {
>>   	int tries = 50;
>> @@ -166,16 +214,131 @@ 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);
>> +
>> +		tcs3472_cycle_to_freq(cycle_us, val, val2);
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +	}
>>   	default:
>>   		return -EINVAL;
>>   	}
>>   }
>>   
>> +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 = div64_u64(PSEC_PER_SEC,
>> +			     (u64)val * USEC_PER_SEC + val2);
>> +
>> +	/*
>> +	 * wait_us can be negative when the requested frequency is too high
>> +	 * to be reached, or very large when the requested frequency is
>> +	 * close to zero. Use s64 to cover the full range:
>> +	 *
>> +	 *   cycle_us = PSEC_PER_SEC / (val * USEC_PER_SEC + val2)
>> +	 *
>> +	 * The divisor of the formula above reaches its maximum when
>> +	 * val = val2 = INT_MAX:
>> +	 *  INT_MAX * USEC_PER_SEC + INT_MAX = ~2.15e18
>> +	 * so cycle_us_min = floor(1e12 / 2.15e18) = 0.
>> +	 *
>> +	 * The divisor reaches its minimum (1) when val = 0 and val2 = 1,
>> +	 * so cycle_us_max = 1e12 / 1 = 1e12.
>> +	 *
>> +	 * Therefore:
>> +	 *   wait_us_min = 0 - 2400 - 612000 = -616800
>> +	 *   wait_us_max = 1e12 - 2400 - 2400 = 999999995200
>> +	 *
>> +	 * Both fit comfortably in s64.
>> +	 */
>> +	wait_us = (s64)cycle_us - init_us - atime_us;
>> +
>> +	if (wait_us < 2400) {
>> +		if (data->enable & TCS3472_ENABLE_WEN) {
>> +			data->enable &= ~TCS3472_ENABLE_WEN;
> As below.
>> +			ret = i2c_smbus_write_byte_data(data->client,
>> +							TCS3472_ENABLE,
>> +							data->enable);
>> +			if (ret)
>> +				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;
> 
> Similar to below. Can we only update data->enable if the write succeeds.
> 
>> +		ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>> +						data->enable);
>> +		if (ret)
>> +			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)
>> +			return ret;
>> +
>> +		data->wlong = wlong;
>> +	}
>> +
>> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_WTIME, wtime);
>> +	if (ret)
>> +		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 ret;
>>   	int i;
>>   
>>   	switch (mask) {
>> @@ -196,15 +359,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)
>> +				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);
>>   	default:
>>   		return -EINVAL;
>>   	}
>> @@ -434,16 +611,15 @@ 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;
>>   
>>   	guard(mutex)(&data->lock);
>>   
>>   	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>> -					data->enable & ~enable_mask);
>> +					data->enable & ~TCS3472_ENABLE_RUN);
>>   	if (ret)
>>   		return ret;
>>   
>> -	data->enable &= ~enable_mask;
>> +	data->enable &= ~TCS3472_ENABLE_RUN;
> This would be more consistent and easier to read if you have
> u8 value;
> 
> 	value = data->enable & ~TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON
> 	value &= ~TCS3472_ENABLE_RUN;
> 
> 	ret = i2c_...
> 	if (ret)
> 		return ret;
> 
> 	data->enable = value;
> 
> 	return 0;
> 
> Or something along those lines. No need for separate enable_mask local
> variable.
> 
> 	>
>>   	return 0;
>>   }
>> @@ -458,6 +634,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));
>> @@ -498,6 +675,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) ? 1 : 0;
>> +
>>   	ret = i2c_smbus_read_word_data(data->client, TCS3472_AILT);
>>   	if (ret < 0)
>>   		return ret;
>> @@ -518,14 +705,29 @@ static int tcs3472_probe(struct i2c_client *client)
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	/* enable device */
>> -	data->enable = ret | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN;
>> +	/*
>> +	 * Enable the chip in its full running state, including WEN. The
>> +	 * actual wait time is controlled by the WTIME and WLONG registers,
>> +	 * which retain their power-on defaults until userspace writes to
>> +	 * sampling_frequency.
>> +	 */
>> +	data->enable = ret | TCS3472_ENABLE_RUN;
>>   	data->enable &= ~TCS3472_ENABLE_AIEN;
> 
> Here is a case that should look more like the one that follows.
> Use a local variable to build what is written and update the cache
> only if the write succeeds.
> 
>>   	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);
>> +	tcs3472_cycle_to_freq(cycle_us, &data->target_freq_hz,
>> +			      &data->target_freq_uhz);
>> +
>>   	ret = devm_add_action_or_reset(dev, tcs3472_powerdown_action, data);
>>   	if (ret)
>>   		return ret;
>> @@ -561,16 +763,23 @@ 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;
>>   
>>   	guard(mutex)(&data->lock);
>>   
>> -	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>> -		data->enable | enable_mask);
>> +	/*
>> +	 * On resume, only PON and AEN need to be re-asserted. WEN must not
>> +	 * be forced on: its desired state was set by the user via
>> +	 * sampling_frequency and is already stored in data->enable, so we
>> +	 * read it from there to preserve the user's choice.
>> +	 */
>> +	value = data->enable | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN;
>> +
>> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
>>   	if (ret)
>>   		return ret;
>>   
>> -	data->enable |= enable_mask;
>> +	data->enable = value;
>>   
>>   	return 0;
>>   }
> 

Hi Jonathan,

Thanks for the review. I'd like to ask a design
question about Sashiko's finding on the cached data->enable.

Sashiko pointed out:

 > @@ -434,16 +611,15 @@ 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;
 >
 >  	guard(mutex)(&data->lock);
 >
 >  	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
 > -					data->enable & ~enable_mask);
 > +					data->enable & ~TCS3472_ENABLE_RUN);
 >  	if (ret)
 >  		return ret;
 >
 > -	data->enable &= ~enable_mask;
 > +	data->enable &= ~TCS3472_ENABLE_RUN;
Does this permanently lose the WEN configuration after a suspend/resume cycle?
Because TCS3472_ENABLE_WEN is part of the TCS3472_ENABLE_RUN mask, this clears
the WEN bit in the cached data->enable. Upon resume, tcs3472_resume() attempts
to restore the configuration by reading this cache:
     value = data->enable | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN;
But since the WEN bit was permanently cleared in the cache during suspend, the
device always resumes with WEN disabled, discarding the user's sampling
frequency configuration.




I see two possible fixes:

1.  In tcs3472_powerdown(), clear only PON and AEN from data->enable
     (keeping WEN as the "user's last desired state"). This is minimal
     but introduces a divergence between data->enable and the actual
     hardware ENABLE register, which breaks the simple "cache of the
     HW register" semantic.
2.  Add a separate field (e.g. bool target_wen) that holds the user's
     desired WEN state, set in tcs3472_set_sampling_freq() and read
     back in tcs3472_resume(). data->enable then strictly remains a
     pure cache of the HW register.


With solution2, the functions would behave as follows:

   - tcs3472_set_sampling_freq(): when activating wait state, sets
     target_wen = true; when disabling it (frequency too high), sets
     target_wen = false. data->enable is then updated to match what
     is written to the chip.

   - tcs3472_cycle_time_us(): reads target_wen instead of
     (data->enable & WEN) to decide whether to include wtime_us in
     the cycle. This avoids reporting a stale wait time during a
     suspended state.

   - tcs3472_powerdown(): unchanged in semantics clears PON, AEN
     and WEN from both the chip and data->enable. target_wen is left
     untouched, since it represents the user's wish, not the HW
     state.

   - tcs3472_resume(): rebuilds the ENABLE value from
     PON | AEN | (target_wen ? WEN : 0), writes it to the chip, then
     updates data->enable to match.

   - tcs3472_probe(): initializes target_wen from the chip's current
     WEN bit (read at probe time).

Solution 2 feels cleaner to me, but it adds a field and a bit of
synchronization between target_wen and (data->enable & WEN). Do you
have another approach in mind?

Thanks,
Aldo

Re: [PATCH v3 8/8] iio: tcs3472: implement wait time and sampling frequency

[Jonathan Cameron]

> 
> Hi Jonathan,
Hi Aldo,

Please crop away anything you aren't responding to to help us
focus on the important bits.

> 
> Thanks for the review. I'd like to ask a design
> question about Sashiko's finding on the cached data->enable.
> 
> Sashiko pointed out:
> 
>  > @@ -434,16 +611,15 @@ 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;
>  >
>  >  	guard(mutex)(&data->lock);
>  >
>  >  	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>  > -					data->enable & ~enable_mask);
>  > +					data->enable & ~TCS3472_ENABLE_RUN);
>  >  	if (ret)
>  >  		return ret;
>  >
>  > -	data->enable &= ~enable_mask;
>  > +	data->enable &= ~TCS3472_ENABLE_RUN;  
> Does this permanently lose the WEN configuration after a suspend/resume cycle?
> Because TCS3472_ENABLE_WEN is part of the TCS3472_ENABLE_RUN mask, this clears
> the WEN bit in the cached data->enable. Upon resume, tcs3472_resume() attempts
> to restore the configuration by reading this cache:
>      value = data->enable | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN;
> But since the WEN bit was permanently cleared in the cache during suspend, the
> device always resumes with WEN disabled, discarding the user's sampling
> frequency configuration.
> 
> 
> 
> 
> I see two possible fixes:
> 
> 1.  In tcs3472_powerdown(), clear only PON and AEN from data->enable
>      (keeping WEN as the "user's last desired state"). This is minimal
>      but introduces a divergence between data->enable and the actual
>      hardware ENABLE register, which breaks the simple "cache of the
>      HW register" semantic.

Suspend / resume is one place where it's common to not cache the 'current'
value rather than the value we expect after coming back via resume().
If you do go this way that field should not be updated at all in suspend
because we will need it's full value in resume.

Sometimes people have a second cache for just this case though. Maybe
that works here. It would I think be the whole register rather than
just the flag you have in option 2.


> 2.  Add a separate field (e.g. bool target_wen) that holds the user's
>      desired WEN state, set in tcs3472_set_sampling_freq() and read
>      back in tcs3472_resume(). data->enable then strictly remains a
>      pure cache of the HW register.
> 
> 
> With solution2, the functions would behave as follows:
> 
>    - tcs3472_set_sampling_freq(): when activating wait state, sets
>      target_wen = true; when disabling it (frequency too high), sets
>      target_wen = false. data->enable is then updated to match what
>      is written to the chip.
> 
>    - tcs3472_cycle_time_us(): reads target_wen instead of
>      (data->enable & WEN) to decide whether to include wtime_us in
>      the cycle. This avoids reporting a stale wait time during a
>      suspended state.
> 
>    - tcs3472_powerdown(): unchanged in semantics clears PON, AEN
>      and WEN from both the chip and data->enable. target_wen is left
>      untouched, since it represents the user's wish, not the HW
>      state.
> 
>    - tcs3472_resume(): rebuilds the ENABLE value from
>      PON | AEN | (target_wen ? WEN : 0), writes it to the chip, then
>      updates data->enable to match.
> 
>    - tcs3472_probe(): initializes target_wen from the chip's current
>      WEN bit (read at probe time).
> 
> Solution 2 feels cleaner to me, but it adds a field and a bit of
> synchronization between target_wen and (data->enable & WEN). Do you
> have another approach in mind?

As long as it ends up easy to follow I'm not that fussed what solution
you use.  Both of these will work.  It does slightly feel like caching
the value we want on resume for the whole register might be the simplest
path but I haven't tried coding it up so may be missing something.

thanks,

Jonathan

> 
> Thanks,
> Aldo
> 
> 
> 
> 

Re: [PATCH v3 8/8] iio: tcs3472: implement wait time and sampling frequency

[Aldo Conte]

On 22/05/26 14:34, Aldo Conte wrote:
> @@ -196,15 +359,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)
> +				return ret;

Hi Jonathan,

Two questions on the INT_TIME case in tcs3472_write_raw() before
I send v4.

- The compute/write/commit pattern issue you raised elsewhere
also applies here. I plan to swap the order: write first with i

return i2c_smbus_write_byte_data( data->client, TCS3472_ATIME, i);

then update data->atime only on success. OK?

- Sashiko flagged a race ( 
https://sashiko.dev/#/patchset/20260522123420.45495-1-aldocontelk%40gmail.com ) :
target_freq_hz/uhz are read without
the lock and then passed to tcs3472_set_sampling_freq() which
takes the lock internally. Two options:

1)  Take the lock briefly in write_raw() to write ATIME, update
     data->atime, and snapshot target_freq_hz/uhz. Then release the
     lock and call tcs3472_set_sampling_freq() (which takes the lock
     again internally). Minimal change, but ATIME and WTIME updates
     are not atomic with each other.

2)  Split tcs3472_set_sampling_freq() into a public wrapper that
     takes the lock and a __tcs3472_set_sampling_freq() helper with
     the lock already held. Then in write_raw() take the lock once,
     write ATIME, and call the helper. This keeps ATIME and WTIME
     updates atomic.

In these cases, what is used to do?

Thanks,
Aldo

> +
> +			/*
> +			 * 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);
>   	default:
>   		return -EINVAL;
>   	}

Re: [PATCH v3 4/8] iio: tcs3472: use ! instead of explicit NULL check

[Andy Shevchenko]

On Fri, May 22, 2026 at 02:34:15PM +0200, Aldo Conte wrote:
> Replace 'if (indio_dev == NULL)' with 'if (!indio_dev)' in
> tcs3472_probe() to follow the preferred kernel style.

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

Joshua, even if it's not written in the documentation (or unclear) the idea is
that whoever formally suggested the change has to acknowledge it (Acked-by is
enough, but full review is superior, of course).

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/8] iio: tcs3472: use ! instead of explicit NULL check

[Joshua Crofts]

On Thu, 4 Jun 2026 at 10:31, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Fri, May 22, 2026 at 02:34:15PM +0200, Aldo Conte wrote:
> > Replace 'if (indio_dev == NULL)' with 'if (!indio_dev)' in
> > tcs3472_probe() to follow the preferred kernel style.
>
> > Suggested-by: Joshua Crofts <joshua.crofts1@gmail.com>
>
> Joshua, even if it's not written in the documentation (or unclear) the idea is
> that whoever formally suggested the change has to acknowledge it (Acked-by is
> enough, but full review is superior, of course).

I see, thanks for the heads up. Not sure if Jonathan will add my tag
at this point,
nevertheless

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

-- 
Kind regards

CJD

Re: [PATCH v3 8/8] iio: tcs3472: implement wait time and sampling frequency

[Aldo Conte]

On 22/05/26 14:34, Aldo Conte wrote:

>   static int tcs3472_req_data(struct tcs3472_data *data)
>   {
>   	int tries = 50;
> @@ -166,16 +214,131 @@ 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);
> +
> +		tcs3472_cycle_to_freq(cycle_us, val, val2);
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
>   	default:
>   		return -EINVAL;
>   	}
>   }Hi Jonathan,

One more thing on v4 before I send it.

For Sashiko's "tries too short" finding, I made the timeout dynamic
based on the actual cycle time:

	cycle_us = tcs3472_cycle_time_us(data);
	timeout_ms = max(1000U, (cycle_us * 2) / USEC_PER_MSEC);
	tries = DIV_ROUND_UP(timeout_ms, 20);

2 * cycle_us gives some safety margin, and the 1-second floor keeps
the original behavior on default configurations.

This solves the timeout issue but introduces a new concern:
tcs3472_cycle_time_us() reads data->{enable,wlong,wtime,atime}
without holding the lock, both here and in the SAMP_FREQ case of
read_raw() (also flagged by Sashiko).

For the SAMP_FREQ case I'll just add guard(mutex)(&data->lock)
before the call:

	case IIO_CHAN_INFO_SAMP_FREQ: {
		unsigned int cycle_us;

		guard(mutex)(&data->lock);
		cycle_us = tcs3472_cycle_time_us(data);
		tcs3472_cycle_to_freq(cycle_us, val, val2);
		return IIO_VAL_INT_PLUS_MICRO;
	}

For tcs3472_req_data() I plan to use scoped_guard() only around the
cycle_us computation:

	scoped_guard(mutex, &data->lock)
		cycle_us = tcs3472_cycle_time_us(data);

	timeout_ms = max(1000U, (cycle_us * 2) / USEC_PER_MSEC);
	tries = DIV_ROUND_UP(timeout_ms, 20);

	while (tries--) {
		...
	}

Could this be ok?

Thanks,
Aldo

Re: [PATCH v3 4/8] iio: tcs3472: use ! instead of explicit NULL check

[Jonathan Cameron]

On Thu, 4 Jun 2026 10:38:18 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:

> On Thu, 4 Jun 2026 at 10:31, Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> >
> > On Fri, May 22, 2026 at 02:34:15PM +0200, Aldo Conte wrote:  
> > > Replace 'if (indio_dev == NULL)' with 'if (!indio_dev)' in
> > > tcs3472_probe() to follow the preferred kernel style.  
> >  
> > > Suggested-by: Joshua Crofts <joshua.crofts1@gmail.com>  
> >
> > Joshua, even if it's not written in the documentation (or unclear) the idea is
> > that whoever formally suggested the change has to acknowledge it (Acked-by is
> > enough, but full review is superior, of course).  
> 
> I see, thanks for the heads up. Not sure if Jonathan will add my tag
> at this point,
> nevertheless
> 
> Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
> 

This is burried under a merge so indeed unlikely I'll add tags.
Still good to have a record on the list for anyone who looks back.

I rushed things a bit because today is more or less the cut off for
anything making this cycle.

Thanks,

Jonathan

Re: [PATCH v3 8/8] iio: tcs3472: implement wait time and sampling frequency

[Jonathan Cameron]

On Thu, 4 Jun 2026 10:10:02 +0200
Aldo Conte <aldocontelk@gmail.com> wrote:

> On 22/05/26 14:34, Aldo Conte wrote:
> > @@ -196,15 +359,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)
> > +				return ret;  
> 
> Hi Jonathan,
> 
> Two questions on the INT_TIME case in tcs3472_write_raw() before
> I send v4.
> 
> - The compute/write/commit pattern issue you raised elsewhere
> also applies here. I plan to swap the order: write first with i
> 
> return i2c_smbus_write_byte_data( data->client, TCS3472_ATIME, i);
> 
> then update data->atime only on success. OK?

Yes. That's conventional way to do it as ensures the cache reflects
the most likely state (fails that don't write are probably slightly
more likely than fails that do).



> 
> - Sashiko flagged a race ( 
> https://sashiko.dev/#/patchset/20260522123420.45495-1-aldocontelk%40gmail.com ) :
> target_freq_hz/uhz are read without
> the lock and then passed to tcs3472_set_sampling_freq() which
> takes the lock internally. Two options:
> 
> 1)  Take the lock briefly in write_raw() to write ATIME, update
>      data->atime, and snapshot target_freq_hz/uhz. Then release the
>      lock and call tcs3472_set_sampling_freq() (which takes the lock
>      again internally). Minimal change, but ATIME and WTIME updates
>      are not atomic with each other.
> 
> 2)  Split tcs3472_set_sampling_freq() into a public wrapper that
>      takes the lock and a __tcs3472_set_sampling_freq() helper with
>      the lock already held. Then in write_raw() take the lock once,
>      write ATIME, and call the helper. This keeps ATIME and WTIME
>      updates atomic.
> 
> In these cases, what is used to do?

2 is cleanest solution so do that.


> Thanks,
> Aldo
> 
> > +
> > +			/*
> > +			 * 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);
> >   	default:
> >   		return -EINVAL;
> >   	}  
> 
> 

Re: [PATCH v3 8/8] iio: tcs3472: implement wait time and sampling frequency

[Aldo Conte]

On 04/06/26 11:23, Aldo Conte wrote:
> On 22/05/26 14:34, Aldo Conte wrote:
> 
>>   static int tcs3472_req_data(struct tcs3472_data *data)
>>   {
>>       int tries = 50;
>> @@ -166,16 +214,131 @@ 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);
>> +
>> +        tcs3472_cycle_to_freq(cycle_us, val, val2);
>> +        return IIO_VAL_INT_PLUS_MICRO;
>> +    }
>>       default:
>>           return -EINVAL;
>>       }
>>   }Hi Jonathan,
> 
> One more thing on v4 before I send it.
> 
> For Sashiko's "tries too short" finding, I made the timeout dynamic
> based on the actual cycle time:
> 
>      cycle_us = tcs3472_cycle_time_us(data);
>      timeout_ms = max(1000U, (cycle_us * 2) / USEC_PER_MSEC);
>      tries = DIV_ROUND_UP(timeout_ms, 20);
> 
> 2 * cycle_us gives some safety margin, and the 1-second floor keeps
> the original behavior on default configurations.
> 
> This solves the timeout issue but introduces a new concern:
> tcs3472_cycle_time_us() reads data->{enable,wlong,wtime,atime}
> without holding the lock, both here and in the SAMP_FREQ case of
> read_raw() (also flagged by Sashiko).
> 
> For the SAMP_FREQ case I'll just add guard(mutex)(&data->lock)
> before the call:
> 
>      case IIO_CHAN_INFO_SAMP_FREQ: {
>          unsigned int cycle_us;
> 
>          guard(mutex)(&data->lock);
>          cycle_us = tcs3472_cycle_time_us(data);
>          tcs3472_cycle_to_freq(cycle_us, val, val2);
>          return IIO_VAL_INT_PLUS_MICRO;
>      }
> 
> For tcs3472_req_data() I plan to use scoped_guard() only around the
> cycle_us computation:
> 
>      scoped_guard(mutex, &data->lock)
>          cycle_us = tcs3472_cycle_time_us(data);
> 
>      timeout_ms = max(1000U, (cycle_us * 2) / USEC_PER_MSEC);
>      tries = DIV_ROUND_UP(timeout_ms, 20);
> 
>      while (tries--) {
>          ...
>      }
> 
> Could this be ok?
> 
> Thanks,
> Aldo
Hi Jonathan,
do you have any updates on this idea?

I have already implemented it and it seems to work.

If you agree, I am ready to submit the v4 of the patch.

thanks,
-- Aldo

Re: [PATCH v3 8/8] iio: tcs3472: implement wait time and sampling frequency

[Jonathan Cameron]

On Sat, 6 Jun 2026 11:27:16 +0200
Aldo Conte <aldocontelk@gmail.com> wrote:

> On 04/06/26 11:23, Aldo Conte wrote:
> > On 22/05/26 14:34, Aldo Conte wrote:
> >   
> >>   static int tcs3472_req_data(struct tcs3472_data *data)
> >>   {
> >>       int tries = 50;
> >> @@ -166,16 +214,131 @@ 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);
> >> +
> >> +        tcs3472_cycle_to_freq(cycle_us, val, val2);
> >> +        return IIO_VAL_INT_PLUS_MICRO;
> >> +    }
> >>       default:
> >>           return -EINVAL;
> >>       }
> >>   }Hi Jonathan,  
> > 
> > One more thing on v4 before I send it.
> > 
> > For Sashiko's "tries too short" finding, I made the timeout dynamic
> > based on the actual cycle time:
> > 
> >      cycle_us = tcs3472_cycle_time_us(data);
> >      timeout_ms = max(1000U, (cycle_us * 2) / USEC_PER_MSEC);
> >      tries = DIV_ROUND_UP(timeout_ms, 20);
> > 
> > 2 * cycle_us gives some safety margin, and the 1-second floor keeps
> > the original behavior on default configurations.
> > 
> > This solves the timeout issue but introduces a new concern:
> > tcs3472_cycle_time_us() reads data->{enable,wlong,wtime,atime}
> > without holding the lock, both here and in the SAMP_FREQ case of
> > read_raw() (also flagged by Sashiko).
> > 
> > For the SAMP_FREQ case I'll just add guard(mutex)(&data->lock)
> > before the call:
> > 
> >      case IIO_CHAN_INFO_SAMP_FREQ: {
> >          unsigned int cycle_us;
> > 
> >          guard(mutex)(&data->lock);
> >          cycle_us = tcs3472_cycle_time_us(data);
> >          tcs3472_cycle_to_freq(cycle_us, val, val2);
> >          return IIO_VAL_INT_PLUS_MICRO;
> >      }
> > 
> > For tcs3472_req_data() I plan to use scoped_guard() only around the
> > cycle_us computation:
> > 
> >      scoped_guard(mutex, &data->lock)
> >          cycle_us = tcs3472_cycle_time_us(data);
> > 
> >      timeout_ms = max(1000U, (cycle_us * 2) / USEC_PER_MSEC);
> >      tries = DIV_ROUND_UP(timeout_ms, 20);
> > 
> >      while (tries--) {
> >          ...
> >      }
> > 
> > Could this be ok?

If this races with an update can we end up with too short a dynamic timeout?
I.e. what happens if that time changes - is the current read cycle completed
with old timing and it only affect the next one, or is it super simple and
the affect is immediate - maybe changing some threshold on a counter that
is used to trigger / stop the acquisition?

I would not expect us to either be able to rely on particular behaviour or
find it documented anywhere.  So two options.
1) Lock around the retry loop
2) Set the retry max to the worse possible case if that's not insanely long.
   Give the polling should exist early anyway it shouldn't make a practical
   difference and is always long enough.   I think the max is about 7 seconds? That's
   rather long to hold a lock for, but should never apply if real timing is a
   microseconds.  This would be my preference as it's simple.

Only remaining thing to check is there is nothing in the datasheet to imply it
is unsafe to change the timings during a capture - as if that were the case
stronger locking would be needed.

> > 
> > Thanks,
> > Aldo  
> Hi Jonathan,
> do you have any updates on this idea?
> 
> I have already implemented it and it seems to work.
> 
> If you agree, I am ready to submit the v4 of the patch.
> 
> thanks,
> -- Aldo

Re: [PATCH v3 8/8] iio: tcs3472: implement wait time and sampling frequency

[Aldo Conte]

On 06/06/26 16:09, Jonathan Cameron wrote:
> On Sat, 6 Jun 2026 11:27:16 +0200
> Aldo Conte <aldocontelk@gmail.com> wrote:
> 
>> On 04/06/26 11:23, Aldo Conte wrote:
>>> On 22/05/26 14:34, Aldo Conte wrote:
>>>    
>>>>    static int tcs3472_req_data(struct tcs3472_data *data)
>>>>    {
>>>>        int tries = 50;
>>>> @@ -166,16 +214,131 @@ 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);
>>>> +
>>>> +        tcs3472_cycle_to_freq(cycle_us, val, val2);
>>>> +        return IIO_VAL_INT_PLUS_MICRO;
>>>> +    }
>>>>        default:
>>>>            return -EINVAL;
>>>>        }
>>>>    }
> 
> If this races with an update can we end up with too short a dynamic timeout?
> I.e. what happens if that time changes - is the current read cycle completed
> with old timing and it only affect the next one, or is it super simple and
> the affect is immediate - maybe changing some threshold on a counter that
> is used to trigger / stop the acquisition?
> 
> I would not expect us to either be able to rely on particular behaviour or
> find it documented anywhere.  So two options.
> 1) Lock around the retry loop
> 2) Set the retry max to the worse possible case if that's not insanely long.
>     Give the polling should exist early anyway it shouldn't make a practical
>     difference and is always long enough.   I think the max is about 7 seconds? That's
>     rather long to hold a lock for, but should never apply if real timing is a
>     microseconds.  This would be my preference as it's simple.
> 
> Only remaining thing to check is there is nothing in the datasheet to imply it
> is unsafe to change the timings during a capture - as if that were the case
> stronger locking would be needed.

Hi Jonathan,

Thanks for the analysis. I checked the TCS3472 datasheet
does not document what happens when ATIME, WTIME
or WLONG change mid-capture.

The worst case is ATIME=0x00, WTIME=0x00, WLONG=1, giving
614 ms (Max Integration Time) + 2.4 ms (RGBC Init) +
7.37 s (Max Wait Time) ~ 8 s.


For v4 I went with your option (2):

static int tcs3472_req_data(struct tcs3472_data *data)
{
	/*
	 * The worst-case cycle time is reached with ATIME=0x00, WTIME=0x00
	 * and WLONG=1. So: 614 ms (Max Integration Time) + 2.4 ms (RGBC Init) +
	 * 7.37 s (Max Wait Time) = ~ 8 s (Total Max cycle time).
	 * Use that as a polling upper bound; in normal operation the loop
	 * exits as soon as AVALID is set. So the total number of tries in 8
	 * seconds considering a polling period of 20 ms is 400.
	 */
	int tries = 400;
	int ret;

	while (tries--) {
...


400 iterations × 20 ms = 8 s upper bound.

Thanks,
Aldo