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

[Aldo Conte]

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