Re: [PATCH v4] iio: tcs3472: implement wait time and sampling frequency
From: Aldo Conte
Date: Tue Jun 09 2026 - 09:40:13 EST
On 09/06/26 14:15, Andy Shevchenko wrote:
On Tue, Jun 09, 2026 at 12:27:52PM +0200, Aldo Conte wrote:
...
Could it be ok if i place a comment here like:+ 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...
/*
* cycle_us = 1 / freq, expressed in microseconds.
* Numerator: 1 [s] = PSEC_PER_SEC [ps]
* Denominator: freq [Hz] * USEC_PER_SEC + val2 [µHz] = freq in [µHz]
* Result: ps / µHz = µs
*/
Yes, and replacing the USEC_PER_SEC by MICROHZ_PER_HZ.
ok!
...
/*
* If the requested wait is so long that even WLONG cannot
* cover it, wtime may still be negative. Saturate to 0,
* which is the largest possible wait (256 * 28.8 ms = 7.37 s).
*/
This and other comments make total sense, with them it's much easier to read
the code, thanks!
to you!
...
Moreover, I would like to take this opportunity to address the comments
raised on Sashiko:
https://sashiko.dev/#/patchset/20260607112713.299968-1-aldocontelk%40gmail.com
I propose the following changes.
For the read and write event functions:
The read path would become:
case IIO_EV_INFO_PERIOD:
period = tcs3472_cycle_time_us(data) *
tcs3472_intr_pers[data->apers];
*val = period / USEC_PER_SEC;
*val2 = period % USEC_PER_SEC;
Again, use HZ-based multiplier, see above.
Here i do not understand why MICROHZ_PER_HZ is better.
In the previous case, we were indeed referring to the denominator of the calculation, which was actually a frequency, so MICROHZ_PER_HZ is indeed correct.
But in this case, we are calculating the integer and fractional parts of a period, so I think this would be fine.
Tell me where I’m going wrong.
return IIO_VAL_INT_PLUS_MICRO;
write becomes:
case IIO_EV_INFO_PERIOD:{
unsigned int cycle_us;
period = val * USEC_PER_SEC + val2;
And again, use HZ-based multiplier, see above.
as above...
cycle_us = tcs3472_cycle_time_us(data);
for (i = 1; i < ARRAY_SIZE(tcs3472_intr_pers) - 1; i++) {
if (period <= cycle_us * tcs3472_intr_pers[i])
break;
}
do {} while () seems better choice here (and do it in reverse order?).
Could you suggest the do {} while {} form, which seems better? Because I think it is already comprehensible.
ret = i2c_smbus_write_byte_data(data->client, TCS3472_PERS, i);
if (ret)
return ret;
data->apers = i;
return 0;
}
Regarding the oscillator tolerance, I suggest using:
tries = 500
Currently, it is set to 400, based on an 8-second interval divided into 20
ms steps. Considering a 20% margin, the total duration becomes approximately
9.8 seconds, which corresponds to about 480 steps. Therefore, setting it to
500 appears to be a reasonable and safe trade-off.
Agree.