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

[Aldo Conte]

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?

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

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