Re: [PATCH v3 8/8] iio: tcs3472: implement wait time and sampling frequency

On 22/05/26 14:34, Aldo Conte wrote:

>   static int tcs3472_req_data(struct tcs3472_data *data)
>   {
>   	int tries = 50;
> @@ -166,16 +214,131 @@ static int tcs3472_read_raw(struct iio_dev *indio_dev,
>   		*val = 0;
>   		*val2 = (256 - data->atime) * 2400;
>   		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		unsigned int cycle_us = tcs3472_cycle_time_us(data);
> +
> +		tcs3472_cycle_to_freq(cycle_us, val, val2);
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
>   	default:
>   		return -EINVAL;
>   	}
>   }Hi Jonathan,

One more thing on v4 before I send it.

For Sashiko's "tries too short" finding, I made the timeout dynamic
based on the actual cycle time:

	cycle_us = tcs3472_cycle_time_us(data);
	timeout_ms = max(1000U, (cycle_us * 2) / USEC_PER_MSEC);
	tries = DIV_ROUND_UP(timeout_ms, 20);

2 * cycle_us gives some safety margin, and the 1-second floor keeps
the original behavior on default configurations.

This solves the timeout issue but introduces a new concern:
tcs3472_cycle_time_us() reads data->{enable,wlong,wtime,atime}
without holding the lock, both here and in the SAMP_FREQ case of
read_raw() (also flagged by Sashiko).

For the SAMP_FREQ case I'll just add guard(mutex)(&data->lock)
before the call:

	case IIO_CHAN_INFO_SAMP_FREQ: {
		unsigned int cycle_us;

		guard(mutex)(&data->lock);
		cycle_us = tcs3472_cycle_time_us(data);
		tcs3472_cycle_to_freq(cycle_us, val, val2);
		return IIO_VAL_INT_PLUS_MICRO;
	}

For tcs3472_req_data() I plan to use scoped_guard() only around the
cycle_us computation:

	scoped_guard(mutex, &data->lock)
		cycle_us = tcs3472_cycle_time_us(data);

	timeout_ms = max(1000U, (cycle_us * 2) / USEC_PER_MSEC);
	tries = DIV_ROUND_UP(timeout_ms, 20);

	while (tries--) {
		...
	}

Could this be ok?

Thanks,
Aldo