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

On 09/06/26 14:15, Andy Shevchenko wrote:
> On Tue, Jun 09, 2026 at 12:27:52PM +0200, Aldo Conte wrote:
> 
> ...
> 
>>>> +	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...
>> Could it be ok if i place a comment here like:
>>
>> 	/*
>> 	 * 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.
>