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

Andy Shevchenko
2026-05-06T10:20:03Z

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

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

No noise in the commit message.

...

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

Please, add another patch that sorts headers alphabetically first.

...

> struct tcs3472_data {
> struct i2c_client *client;

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

Does `pahole` agree with the proposed layout?

...

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

This is the same as atime_us.

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

...

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

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

...

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

What if val > 0 && val2 < 0?

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

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

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

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

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

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

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

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

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

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

Too many castings.

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

Ditto.

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

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

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

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

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

...

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

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

...

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

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

--
With Best Regards,
Andy Shevchenko