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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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