Re: [PATCH v2 4/5] iio: light: tcs3472: implement wait time and sampling frequency
From: Aldo Conte
Date: Fri May 15 2026 - 12:12:17 EST
On 13/05/26 13:17, Andy Shevchenko wrote:
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...
In theory, val and val2 could have a value of INT_MAX. Therefore, the calculation INT_MAX * USEC_PER_SEC + INT_MAX results in a value of 2,147,483,861,748,364,700, which can only be stored in 64 bits. This is, of course, an extremely rare case in which userspace writes an impossible frequency that is simply too high.
Therefore, div64_u64() should be used instead of div_u64().
Therefore, the following code would be required:
cycle_us = div64_u64(PSEC_PER_SEC, val * USEC_PER_SEC + val2);
...
+ 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.
cycle_us_MIN is reached when (u64)val * USEC_PER_SEC + val2 is MAX and so is INT_MAX * USEC_PER_SEC + INT_MAX. Assuming this, the division would be 10⁶ / (INT_MAX * USEC_PER_SEC + INT_MAX) = 0.000465661, so 0.
Therefore, cycle_us_MIN would equal 0 and cycle_us_MAX would equal PSEC_PER_SEC/1, i.e. 10⁶; consequently, wait_us_MIN would equal −614400:
wait_us_min = 0−2400−612000= −614400
and wait_us_MAX would equal 999999995200:
wait_us_MAX = 1000000000000 −2400−2400 = 999999995200
This means that wait_us needs to be s64 if i am not wrong.
The i2c_smbus_write_byte_data() function returns a value of 0 on success and a value less than 0 in the event of an error. I do not understand this comment. Could you please explain it to me in more detail?
+ 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.
Something like this could be ok?
...
+ 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?
/*
* Convert cycle time in microseconds to frequency in Hz and microhertz.
*
* Given:
* cycle_us = T, i.e. 1 cycle lasts T microseconds
*
* The frequency is:
* f = 10^6 / T [Hz]
*
* We split it as:
* val = floor(10^6 / T)
* val2 = (10^6 mod T) * 10^6 / T [microhertz]
*
* I.E.:
* val = 10^6 / T (integer division)
* val2 = (10^6 % T) * 10^6 / T (fraction scaled to microhertz)
*/
static void
us_cycle_to_freq_hz(int cycle_us, int *val, int *val2)
{
*val = USEC_PER_SEC / cycle_us;
*val2 = div_u64((u64)(USEC_PER_SEC % cycle_us) * USEC_PER_SEC,
cycle_us);
}
thank you,
Aldo