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

[Andy Shevchenko]

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.

...

> 	/*
> 	 * 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!

...

> 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.

> 		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.

> 		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?).

> 		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.

-- 
With Best Regards,
Andy Shevchenko