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@xxxxxxxxxx>

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