Re: [PATCH v4] iio: tcs3472: implement wait time and sampling frequency

[Andy Shevchenko]

On Sun, Jun 07, 2026 at 01:27:13PM +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().
> 
> Add a u8 enable_pre_suspend field to struct tcs3472_data:
> tcs3472_powerdown() snapshots data->enable into it, and
> tcs3472_resume() restores enable register content from the snapshot.
> This preserves the user's WEN choice across suspend/resume.
> 
> Bound tcs3472_req_data() polling to the worst-case cycle time
> (~8 seconds with ATIME=0x00, WTIME=0x00, WLONG=1).
> 
> Remove the "TODO: wait time" comment at the top of the file.

In general LGTM, but I have a few nit-picks/questions below.
In any case
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>

...

> +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;
> +
> +	atime_us = (256 - data->atime) * 2400;
> +	cycle_us = div64_u64(PSEC_PER_SEC,
> +			     (u64)val * USEC_PER_SEC + val2);

I'm a bit puzzled why cycle has "us" suffix. We divide seconds by seconds...

> +	/*
> +	 * 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;

> +

Unneeded blank line.

> +	if (wait_us < 2400) {
> +		if (data->enable & TCS3472_ENABLE_WEN) {
> +			u8 enable = data->enable & ~TCS3472_ENABLE_WEN;
> +
> +			ret = i2c_smbus_write_byte_data(data->client,
> +							TCS3472_ENABLE, enable);
> +			if (ret)
> +				return ret;
> +
> +			data->enable = enable;
> +		}
> +
> +		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)) {
> +		u8 enable = data->enable | TCS3472_ENABLE_WEN;
> +
> +		ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> +						enable);
> +		if (ret)
> +			return ret;
> +
> +		data->enable = enable;
> +	}
> +
> +	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);

Seems like wtime calculation is not used in the below conditional.

> +	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;
> +	}

Not sure, but we can move the

	wtime = clamp(wtime, 0, 255);

from the above to be here. It might also need a comment why wtime can be
negative and why we move that to 0.


> +	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;
> +}

...

> +			if (val2 != (256 - i) * 2400)

The same is used to calculate atime_us above. Can this be a helper
with a good name?

> +				continue;

-- 
With Best Regards,
Andy Shevchenko