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

[Jonathan Cameron]

On Thu, 4 Jun 2026 10:10:02 +0200
Aldo Conte <aldocontelk@xxxxxxxxx> wrote:

> On 22/05/26 14:34, Aldo Conte wrote:
> > @@ -196,15 +359,29 @@ static int tcs3472_write_raw(struct iio_dev *indio_dev,
> >   		if (val != 0)
> >   			return -EINVAL;
> >   		for (i = 0; i < 256; i++) {
> > -			if (val2 == (256 - i) * 2400) {
> > -				data->atime = i;
> > -				return i2c_smbus_write_byte_data(
> > -					data->client, TCS3472_ATIME,
> > -					data->atime);
> > -			}
> > -
> > +			if (val2 != (256 - i) * 2400)
> > +				continue;
> > +
> > +			data->atime = i;
> > +			ret = i2c_smbus_write_byte_data(data->client,
> > +							TCS3472_ATIME,
> > +							data->atime);
> > +			if (ret)
> > +				return ret;  
> 
> Hi Jonathan,
> 
> Two questions on the INT_TIME case in tcs3472_write_raw() before
> I send v4.
> 
> - The compute/write/commit pattern issue you raised elsewhere
> also applies here. I plan to swap the order: write first with i
> 
> return i2c_smbus_write_byte_data( data->client, TCS3472_ATIME, i);
> 
> then update data->atime only on success. OK?

Yes. That's conventional way to do it as ensures the cache reflects
the most likely state (fails that don't write are probably slightly
more likely than fails that do).



> 
> - Sashiko flagged a race ( 
> https://sashiko.dev/#/patchset/20260522123420.45495-1-aldocontelk%40gmail.com ) :
> target_freq_hz/uhz are read without
> the lock and then passed to tcs3472_set_sampling_freq() which
> takes the lock internally. Two options:
> 
> 1)  Take the lock briefly in write_raw() to write ATIME, update
>      data->atime, and snapshot target_freq_hz/uhz. Then release the
>      lock and call tcs3472_set_sampling_freq() (which takes the lock
>      again internally). Minimal change, but ATIME and WTIME updates
>      are not atomic with each other.
> 
> 2)  Split tcs3472_set_sampling_freq() into a public wrapper that
>      takes the lock and a __tcs3472_set_sampling_freq() helper with
>      the lock already held. Then in write_raw() take the lock once,
>      write ATIME, and call the helper. This keeps ATIME and WTIME
>      updates atomic.
> 
> In these cases, what is used to do?

2 is cleanest solution so do that.


> Thanks,
> Aldo
> 
> > +
> > +			/*
> > +			 * ATIME just changed, so the cycle time changed too.
> > +			 * Re-run the sampling frequency logic to recompute
> > +			 * WTIME and preserve the user's last requested
> > +			 * frequency.
> > +			 */
> > +			return tcs3472_set_sampling_freq(data,
> > +							 data->target_freq_hz,
> > +							 data->target_freq_uhz);
> >   		}
> >   		return -EINVAL;
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		return tcs3472_set_sampling_freq(data, val, val2);
> >   	default:
> >   		return -EINVAL;
> >   	}  
> 
>