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

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