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

[Jonathan Cameron]

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;
> >>       }
> >>   }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?

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.

> > 
> > Thanks,
> > Aldo  
> Hi Jonathan,
> do you have any updates on this idea?
> 
> I have already implemented it and it seems to work.
> 
> If you agree, I am ready to submit the v4 of the patch.
> 
> thanks,
> -- Aldo