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

On 06/06/26 16:09, Jonathan Cameron wrote:
> On Sat, 6 Jun 2026 11:27:16 +0200
> Aldo Conte <[email protected]> wrote:
> 
>> On 04/06/26 11:23, Aldo Conte wrote:
>>> 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;
>>>>        }
>>>>    }
> 
> If this races with an update can we end up with too short a dynamic timeout?
> I.e. what happens if that time changes - is the current read cycle completed
> with old timing and it only affect the next one, or is it super simple and
> the affect is immediate - maybe changing some threshold on a counter that
> is used to trigger / stop the acquisition?
> 
> I would not expect us to either be able to rely on particular behaviour or
> find it documented anywhere.  So two options.
> 1) Lock around the retry loop
> 2) Set the retry max to the worse possible case if that's not insanely long.
>     Give the polling should exist early anyway it shouldn't make a practical
>     difference and is always long enough.   I think the max is about 7 seconds? That's
>     rather long to hold a lock for, but should never apply if real timing is a
>     microseconds.  This would be my preference as it's simple.
> 
> Only remaining thing to check is there is nothing in the datasheet to imply it
> is unsafe to change the timings during a capture - as if that were the case
> stronger locking would be needed.

Hi Jonathan,

Thanks for the analysis. I checked the TCS3472 datasheet
does not document what happens when ATIME, WTIME
or WLONG change mid-capture.

The worst case is ATIME=0x00, WTIME=0x00, WLONG=1, giving
614 ms (Max Integration Time) + 2.4 ms (RGBC Init) +
7.37 s (Max Wait Time) ~ 8 s.


For v4 I went with your option (2):

static int tcs3472_req_data(struct tcs3472_data *data)
{
	/*
	 * The worst-case cycle time is reached with ATIME=0x00, WTIME=0x00
	 * and WLONG=1. So: 614 ms (Max Integration Time) + 2.4 ms (RGBC Init) +
	 * 7.37 s (Max Wait Time) = ~ 8 s (Total Max cycle time).
	 * Use that as a polling upper bound; in normal operation the loop
	 * exits as soon as AVALID is set. So the total number of tries in 8
	 * seconds considering a polling period of 20 ms is 400.
	 */
	int tries = 400;
	int ret;

	while (tries--) {
...


400 iterations × 20 ms = 8 s upper bound.

Thanks,
Aldo