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

[Aldo Conte]

> > +static int __tcs3472_set_sampling_freq(struct tcs3472_data *data,
> > +				       int val, int val2)
> > +{
> > +	unsigned int atime_us;
> > +	unsigned int init_us = 2400;
> > +	u64 cycle_us;
> > +	s64 wait_us;
> > +	int wtime;
> > +	bool wlong = false;
> > +	u8 config;
> > +	int ret;
> > +
> > +	if (val < 0 || val2 < 0 || (val == 0 && val2 == 0))
> > +		return -EINVAL;
> > +
> > +	atime_us = (256 - data->atime) * 2400;
> > +	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
	 */

> > +
>
> Unneeded blank line.

ok i will fix it.
> > +	/*
> > +	 * Wait state is needed: make sure WEN is active before programming
> > +	 * WTIME (and possibly WLONG).
> > +	 */
> > +	if (!(data->enable & TCS3472_ENABLE_WEN)) {
> > +		u8 enable = data->enable | TCS3472_ENABLE_WEN;
> > +
> > +		ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> > +						enable);
> > +		if (ret)
> > +			return ret;
> > +
> > +		data->enable = enable;
> > +	}
> > +
> > +	wtime = 256 - DIV_ROUND_CLOSEST_ULL(wait_us, 2400);
> > +	if (wtime < 0) {
ok here i can write a comment:
		/*
		 * If wait_us is too high (so the requested frequency is too
		 * low), the resulting wait exceeds what WTIME can represent
		 * (max 614 ms without WLONG). Enable WLONG, whose step is 12x
		 * longer (28.8 ms instead of 2.4 ms), and recompute.
		 */

> > +		wlong = true;
> > +		wtime = 256 - DIV_ROUND_CLOSEST_ULL(wait_us, 28800);
> > +	}
> > +	wtime = clamp(wtime, 0, 255);
>
> Seems like wtime calculation is not used in the below conditional.
>
> > +	if (wlong != data->wlong) {
> > +		ret = i2c_smbus_read_byte_data(data->client, TCS3472_CONFIG);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		config = ret;
> > +		if (wlong)
> > +			config |= TCS3472_CONFIG_WLONG;
> > +		else
> > +			config &= ~TCS3472_CONFIG_WLONG;
> > +
> > +		ret = i2c_smbus_write_byte_data(data->client, TCS3472_CONFIG,
> > +						config);
> > +		if (ret)
> > +			return ret;
> > +
> > +		data->wlong = wlong;
> > +	}
>
> Not sure, but we can move the
>
> 	wtime = clamp(wtime, 0, 255);
>
> from the above to be here. It might also need a comment why wtime can be
> negative and why we move that to 0.

Ok for me to move clamp here. I would write another comment here to justify the 
clamp

	/*
	 * 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).
	 */
	wtime = clamp(wtime, 0, 255);
	ret = i2c_smbus_write_byte_data(data->client, TCS3472_WTIME, wtime);
	if (ret)
		return ret;

> > +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_WTIME, wtime);
> > +	if (ret)
> > +		return ret;
> > +
> > +	data->wtime = wtime;
> > +	data->target_freq_hz = val;
> > +	data->target_freq_uhz = val2;
> > +
> > +	return 0;
> > +}
>
> ...
>
> > +			if (val2 != (256 - i) * 2400)
>
> The same is used to calculate atime_us above. Can this be a helper
> with a good name?

ok i could use
#define TCS3472_ATIME_TO_US(atime) (((256) - (atime)) * 2400)


> > +				continue;


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;
		return IIO_VAL_INT_PLUS_MICRO;

write becomes:

case IIO_EV_INFO_PERIOD:{
		unsigned int cycle_us;

		period = val * USEC_PER_SEC + val2;
		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;
		}
		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.