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

[Aldo Conte]

On 06/06/26 16:09, Jonathan Cameron wrote:
> On Sat, 6 Jun 2026 11:27:16 +0200
> Aldo Conte <aldocontelk@xxxxxxxxx> 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