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

[Aldo Conte <aldocontelk@gmail.com>]

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