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

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

[Aldo Conte]

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

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

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

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

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

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

Testing:

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

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

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

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

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

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

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

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

-- 
2.54.0

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

[Aldo Conte]

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

No functional change.

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

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

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

[Aldo Conte]

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

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

No functional change.

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

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

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

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

[Aldo Conte]

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

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

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

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

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

diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index 4fd4fd74d0d6..7a6dc8360326 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -427,13 +427,38 @@ static const struct iio_info tcs3472_info = {
 	.attrs = &tcs3472_attribute_group,
 };
 
+static int tcs3472_powerdown(struct tcs3472_data *data)
+{
+	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
+	u8 value;
+	int ret;
+
+	guard(mutex)(&data->lock);
+
+	value = data->enable & ~enable_mask;
+
+	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
+	if (ret)
+		return ret;
+
+	data->enable = value;
+
+	return 0;
+}
+
+static void tcs3472_powerdown_action(void *data)
+{
+	tcs3472_powerdown(data);
+}
+
 static int tcs3472_probe(struct i2c_client *client)
 {
+	struct device *dev = &client->dev;
 	struct tcs3472_data *data;
 	struct iio_dev *indio_dev;
 	int ret;
 
-	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (indio_dev == NULL)
 		return -ENOMEM;
 
@@ -453,9 +478,9 @@ static int tcs3472_probe(struct i2c_client *client)
 		return ret;
 
 	if (ret == 0x44)
-		dev_info(&client->dev, "TCS34721/34725 found\n");
+		dev_info(dev, "TCS34721/34725 found\n");
 	else if (ret == 0x4d)
-		dev_info(&client->dev, "TCS34723/34727 found\n");
+		dev_info(dev, "TCS34723/34727 found\n");
 	else
 		return -ENODEV;
 
@@ -497,59 +522,26 @@ static int tcs3472_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
-	ret = iio_triggered_buffer_setup(indio_dev, NULL,
-		tcs3472_trigger_handler, NULL);
+	ret = devm_add_action_or_reset(dev, tcs3472_powerdown_action, data);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+					      tcs3472_trigger_handler, NULL);
 	if (ret < 0)
 		return ret;
 
 	if (client->irq) {
-		ret = request_threaded_irq(client->irq, NULL,
-					   tcs3472_event_handler,
-					   IRQF_TRIGGER_FALLING | IRQF_SHARED |
-					   IRQF_ONESHOT,
-					   client->name, indio_dev);
+		ret = devm_request_threaded_irq(dev, client->irq, NULL,
+						tcs3472_event_handler,
+						IRQF_TRIGGER_FALLING | IRQF_SHARED |
+						IRQF_ONESHOT,
+						client->name, indio_dev);
 		if (ret)
-			goto buffer_cleanup;
+			return ret;
 	}
 
-	ret = iio_device_register(indio_dev);
-	if (ret < 0)
-		goto free_irq;
-
-	return 0;
-
-free_irq:
-	if (client->irq)
-		free_irq(client->irq, indio_dev);
-buffer_cleanup:
-	iio_triggered_buffer_cleanup(indio_dev);
-	return ret;
-}
-
-static int tcs3472_powerdown(struct tcs3472_data *data)
-{
-	int ret;
-	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
-
-	guard(mutex)(&data->lock);
-
-	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
-		data->enable & ~enable_mask);
-	if (!ret)
-		data->enable &= ~enable_mask;
-
-	return ret;
-}
-
-static void tcs3472_remove(struct i2c_client *client)
-{
-	struct iio_dev *indio_dev = i2c_get_clientdata(client);
-
-	iio_device_unregister(indio_dev);
-	if (client->irq)
-		free_irq(client->irq, indio_dev);
-	iio_triggered_buffer_cleanup(indio_dev);
-	tcs3472_powerdown(iio_priv(indio_dev));
+	return devm_iio_device_register(dev, indio_dev);
 }
 
 static int tcs3472_suspend(struct device *dev)
@@ -563,17 +555,21 @@ static int tcs3472_resume(struct device *dev)
 {
 	struct tcs3472_data *data = iio_priv(i2c_get_clientdata(
 		to_i2c_client(dev)));
-	int ret;
 	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
+	u8 value;
+	int ret;
 
 	guard(mutex)(&data->lock);
 
-	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
-		data->enable | enable_mask);
-	if (!ret)
-		data->enable |= enable_mask;
+	value = data->enable | enable_mask;
 
-	return ret;
+	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
+	if (ret)
+		return ret;
+
+	data->enable = value;
+
+	return 0;
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(tcs3472_pm_ops, tcs3472_suspend,
@@ -591,7 +587,6 @@ static struct i2c_driver tcs3472_driver = {
 		.pm	= pm_sleep_ptr(&tcs3472_pm_ops),
 	},
 	.probe		= tcs3472_probe,
-	.remove		= tcs3472_remove,
 	.id_table	= tcs3472_id,
 };
 module_i2c_driver(tcs3472_driver);
-- 
2.54.0

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

[Aldo Conte]

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

Implement control of the wait time through IIO_CHAN_INFO_SAMP_FREQ:

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

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

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

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

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

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

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

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

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

[Aldo Conte]

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

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

No functional change.

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

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

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

[Joshua Crofts]

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

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

Othewise, this looks like a solid cleanup.

-- 
Kind regards

CJD

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

[Joshua Crofts]

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

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

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

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

-- 
Kind regards

CJD

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

[Joshua Crofts]

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

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

-- 
Kind regards

CJD

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

[Joshua Crofts]

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

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

-- 
Kind regards

CJD

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

[Andy Shevchenko]

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

...

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

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

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

...

Code wise LGTM.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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

...

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

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

Giving the above context, this should be rather as

       } scan = { };
       int ret;

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

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, May 13, 2026 at 12:32:13AM +0200, Aldo Conte wrote:
> Convert the driver to use device-managed resource allocation:
> - Add tcs3472_powerdown_action() and register it with
>   devm_add_action_or_reset() to ensure the device is powered down on
>   cleanup. Before this patch, the chip remained powered if probe
>   failed after enabling it.
> - Replace iio_triggered_buffer_setup() with
>   devm_iio_triggered_buffer_setup().
> - Replace request_threaded_irq() with devm_request_threaded_irq().
> - Replace iio_device_register() with devm_iio_device_register().
> - Remove tcs3472_remove() as all cleanup is now handled by devm.
> 
> Rewrite the read-modify-write pattern in tcs3472_powerdown() and
> tcs3472_resume() so the new register value is computed first, written
> to the chip, and committed to data->enable only on success.
> 
> Use a local 'dev = &client->dev' in tcs3472_probe() to keep the devm
> calls compact.

...

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

In appropriate tag again.

...

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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

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

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

...

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

Better style is

#define TCS3472_ENABLE_RUN						\
	(TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON | TCS3472_ENABLE_WEN)

...

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

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

...

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

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

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

...

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

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

> +		return ret;

...

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

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

...

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[Aldo Conte]

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

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

My plan is to split v2 into 7 patches:

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

Two things I am not sure about:

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

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


Aldo

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

[Andy Shevchenko]

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

...

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

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

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

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

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

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


-- 
With Best Regards,
Andy Shevchenko