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

[Aldo Conte <aldocontelk@gmail.com>]

On 26/05/26 15:11, Jonathan Cameron wrote:
> On Fri, 22 May 2026 14:34:19 +0200
> Aldo Conte <aldocontelk@gmail.com> wrote:
> 
>> The TCS3472 has a wait state controlled by the WEN bit in the ENABLE
>> register and the WAIT register, with an additional WLONG bit in CONFIG
>> that if set multiplies the wait step by 12. The driver previously
>> defined TCS3472_WTIME but never used it leaving the TODO comment on
>> the top of the source file.
>>
>> Implement control of the wait time through IIO_CHAN_INFO_SAMP_FREQ:
>>
>>    - Reading sampling_frequency returns the chip's current cycle time,
>>      computed as the sum of ATIME, the fixed RGBC initialization time
>>      and the wait time (which depends on WEN and WLONG).
>>
>>    - Writing sampling_frequency programs WTIME so that the resulting
>>      cycle period approximates the requested frequency. If the
>>      requested frequency cannot be reached with any
>>      non-zero wait time, WEN is disabled and the chip runs
>>      back-to-back conversions at the maximum rate allowed by ATIME.
>>      If the requested period exceeds the maximum WTIME range, WLONG
>>      is enabled to extend the wait step from 2.4 ms to 28.8 ms.
>>
>>    - The user's last requested frequency is stored in the driver's
>>      private data so that subsequent changes to integration_time
>>      recompute WTIME and preserve the requested sampling rate as
>>      closely as possible.
>>
>> Add TCS3472_ENABLE_WEN, TCS3472_ENABLE_RUN and TCS3472_CONFIG_WLONG
>> bit definitions. TCS3472_ENABLE_RUN bundles the bits
>> (AEN | PON | WEN) that are simultaneously set when the chip is in
>> running state and cleared during powerdown, and is used by
>> tcs3472_probe(), tcs3472_powerdown().
>>
>> In tcs3472_resume(), only PON and AEN are re-asserted while WEN is
>> preserved from data->enable.
>>
>> Remove the "TODO: wait time" comment at the top of the file.
> Hi Aldo,
> 
> Please look at Sashiko's comments on this particular patch
> (the others are about an issue that has nothing to do with your series)
> 
> https://sashiko.dev/#/patchset/20260522123420.45495-1-aldocontelk%40gmail.com
> 
> The suggestion is that you need to make the timeout in
> tcs3472_req_data() longer given with these settings the capture may
> take a lot longer than previously.
> 
> There is some other general good practice stuff in there. For example:
>> +			data->enable &= ~TCS3472_ENABLE_WEN;
>> +			ret = i2c_smbus_write_byte_data(data->client,
>> +							TCS3472_ENABLE,
>> +							data->enable);
> 
> if you were to use a local variable to do the masking on enable
> 	
> 	enable = data->enable & ~TCS3472_ENABLE_WEN;
> 	ret = i2c_smbus_write_byte_data(data->client,..., enable);
> 	if (ret)
> 		return ret;
> 
> 	data->enable = enable;
> 
> You'd ensure that 'to best effort' the cached value is not updated
> if the write fails.  The approx (possibly correct) arguement is that
> any failure in i2c_smbus_write_byte_data() is more likely to have
> left the original state alone than successfully written the new one.
> Or more generally the principle that in ideal world any call that fails
> has no side effects.
> 
> Anyhow, take a look at all the feedback.  If you disagree with any of
> it feel free to add a note to the change log of the next version.
> 
> 
> Ah. I've just noticed that as part of this you did make the change
> I suggested in patch 3. Oops - I should have checked.  It is fine
> in here.
> 
> I didn't crop so that you'd have full context visible. A few specific
> cases and comments inline.
> 
> Note I've picked up patches 1-7 so it's just this one that needs
> revisions for v4.
> 
> thanks,
> 
> Jonathan
> 
> 
>>
>> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
>> ---
>> v3:
>> - Drop Suggested-by: Andy (inappropriate tag).
>> - TCS3472_ENABLE_RUN: new style.
>> - cycle_us: use div64_u64().
>> - cycle_us: cast val to u64 for 32-bit safety.
>> - wait_us: keep s64; added comment explaining the range bounds.
>> - Use 'if (ret)' instead of 'if (ret < 0)' after i2c writes.
>> - New helper tcs3472_cycle_to_freq() deduplicates val/val2 logic.
>> - !! -> ? 1 : 0 in probe.
>> - Fix tcs3472_resume(): preserve WEN from data->enable instead of
>>    forcing TCS3472_ENABLE_RUN.
>>
>>   drivers/iio/light/tcs3472.c | 245 +++++++++++++++++++++++++++++++++---
>>   1 file changed, 227 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
>> index 1f597ca93697..81549d7eba35 100644
>> --- a/drivers/iio/light/tcs3472.c
>> +++ b/drivers/iio/light/tcs3472.c
>> @@ -9,8 +9,6 @@
>>    * TCS34727)
>>    *
>>    * Datasheet: http://ams.com/eng/content/download/319364/1117183/file/TCS3472_Datasheet_EN_v2.pdf
>> - *
>> - * TODO: wait time
>>    */
>>   
>>   #include <linux/cleanup.h>
>> @@ -53,19 +51,27 @@
>>   #define TCS3472_STATUS_AINT BIT(4)
>>   #define TCS3472_STATUS_AVALID BIT(0)
>>   #define TCS3472_ENABLE_AIEN BIT(4)
>> +#define TCS3472_ENABLE_WEN BIT(3)
>>   #define TCS3472_ENABLE_AEN BIT(1)
>>   #define TCS3472_ENABLE_PON BIT(0)
>> +#define TCS3472_ENABLE_RUN						\
>> +	(TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON | TCS3472_ENABLE_WEN)
>>   #define TCS3472_CONTROL_AGAIN_MASK (BIT(0) | BIT(1))
>> +#define TCS3472_CONFIG_WLONG BIT(1)
>>   
>>   struct tcs3472_data {
>>   	struct i2c_client *client;
>>   	struct mutex lock;
>> +	int target_freq_hz;
>> +	int target_freq_uhz;
>>   	u16 low_thresh;
>>   	u16 high_thresh;
>>   	u8 enable;
>>   	u8 control;
>>   	u8 atime;
>>   	u8 apers;
>> +	u8 wtime;
>> +	bool wlong;
>>   };
>>   
>>   static const struct iio_event_spec tcs3472_events[] = {
>> @@ -91,6 +97,7 @@ static const struct iio_event_spec tcs3472_events[] = {
>>   	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>>   	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_CALIBSCALE) | \
>>   		BIT(IIO_CHAN_INFO_INT_TIME), \
>> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>>   	.channel2 = IIO_MOD_LIGHT_##_color, \
>>   	.address = _addr, \
>>   	.scan_index = _si, \
>> @@ -114,6 +121,47 @@ static const struct iio_chan_spec tcs3472_channels[] = {
>>   	IIO_CHAN_SOFT_TIMESTAMP(4),
>>   };
>>   
>> +/*
>> + * The chip's cycle time is the sum of three components:
>> + *   - ATIME: the programmable RGBC integration time.
>> + *   - The fixed RGBC initialization time (2400 us).
>> + *   - WTIME: the wait time, used only if WEN is set. If WLONG is active,
>> + *     the wait step is multiplied by 12 (2400 us -> 28800 us).
>> + */
>> +static unsigned int tcs3472_cycle_time_us(struct tcs3472_data *data)
>> +{
>> +	unsigned int atime_us = (256 - data->atime) * 2400;
>> +	unsigned int init_us = 2400;
>> +	unsigned int wtime_us;
>> +
>> +	if (!(data->enable & TCS3472_ENABLE_WEN))
>> +		wtime_us = 0;
>> +	else if (data->wlong)
>> +		wtime_us = (256 - data->wtime) * 28800;
>> +	else
>> +		wtime_us = (256 - data->wtime) * 2400;
>> +
>> +	return atime_us + init_us + wtime_us;
>> +}
>> +
>> +/*
>> + * Convert a cycle time in microseconds to a frequency in Hz and microhertz.
>> + *
>> + * Given cycle_us = T (the cycle period in microseconds), the corresponding
>> + * frequency is:
>> + *   f = 1e6 / T  [Hz]
>> + *
>> + * The result is split into the IIO_VAL_INT_PLUS_MICRO format:
>> + *   val  = floor(1e6 / T)                [Hz]
>> + *   val2 = (1e6 mod T) * 1e6 / T         [microhertz]
>> + */
>> +static void tcs3472_cycle_to_freq(unsigned int cycle_us, int *val, int *val2)
>> +{
>> +	*val = USEC_PER_SEC / cycle_us;
>> +	*val2 = div_u64((u64)(USEC_PER_SEC % cycle_us) * USEC_PER_SEC,
>> +			cycle_us);
>> +}
>> +
>>   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;
>>   	}
>>   }
>>   
>> +static int tcs3472_set_sampling_freq(struct tcs3472_data *data,
>> +				     int val, int val2)
>> +{
>> +	unsigned int atime_us;
>> +	unsigned int init_us = 2400;
>> +	u64 cycle_us;
>> +	s64 wait_us;
>> +	int wtime;
>> +	bool wlong = false;
>> +	u8 config;
>> +	int ret;
>> +
>> +	if (val < 0 || val2 < 0 || (val == 0 && val2 == 0))
>> +		return -EINVAL;
>> +
>> +	guard(mutex)(&data->lock);
>> +
>> +	atime_us = (256 - data->atime) * 2400;
>> +	cycle_us = div64_u64(PSEC_PER_SEC,
>> +			     (u64)val * USEC_PER_SEC + val2);
>> +
>> +	/*
>> +	 * wait_us can be negative when the requested frequency is too high
>> +	 * to be reached, or very large when the requested frequency is
>> +	 * close to zero. Use s64 to cover the full range:
>> +	 *
>> +	 *   cycle_us = PSEC_PER_SEC / (val * USEC_PER_SEC + val2)
>> +	 *
>> +	 * The divisor of the formula above reaches its maximum when
>> +	 * val = val2 = INT_MAX:
>> +	 *  INT_MAX * USEC_PER_SEC + INT_MAX = ~2.15e18
>> +	 * so cycle_us_min = floor(1e12 / 2.15e18) = 0.
>> +	 *
>> +	 * The divisor reaches its minimum (1) when val = 0 and val2 = 1,
>> +	 * so cycle_us_max = 1e12 / 1 = 1e12.
>> +	 *
>> +	 * Therefore:
>> +	 *   wait_us_min = 0 - 2400 - 612000 = -616800
>> +	 *   wait_us_max = 1e12 - 2400 - 2400 = 999999995200
>> +	 *
>> +	 * Both fit comfortably in s64.
>> +	 */
>> +	wait_us = (s64)cycle_us - init_us - atime_us;
>> +
>> +	if (wait_us < 2400) {
>> +		if (data->enable & TCS3472_ENABLE_WEN) {
>> +			data->enable &= ~TCS3472_ENABLE_WEN;
> As below.
>> +			ret = i2c_smbus_write_byte_data(data->client,
>> +							TCS3472_ENABLE,
>> +							data->enable);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +
>> +		data->target_freq_hz = val;
>> +		data->target_freq_uhz = val2;
>> +		return 0;
>> +	}
>> +
>> +	/*
>> +	 * Wait state is needed: make sure WEN is active before programming
>> +	 * WTIME (and possibly WLONG).
>> +	 */
>> +	if (!(data->enable & TCS3472_ENABLE_WEN)) {
>> +		data->enable |= TCS3472_ENABLE_WEN;
> 
> Similar to below. Can we only update data->enable if the write succeeds.
> 
>> +		ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>> +						data->enable);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	wtime = 256 - DIV_ROUND_CLOSEST_ULL(wait_us, 2400);
>> +	if (wtime < 0) {
>> +		wlong = true;
>> +		wtime = 256 - DIV_ROUND_CLOSEST_ULL(wait_us, 28800);
>> +	}
>> +	wtime = clamp(wtime, 0, 255);
>> +
>> +	if (wlong != data->wlong) {
>> +		ret = i2c_smbus_read_byte_data(data->client, TCS3472_CONFIG);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		config = ret;
>> +		if (wlong)
>> +			config |= TCS3472_CONFIG_WLONG;
>> +		else
>> +			config &= ~TCS3472_CONFIG_WLONG;
>> +
>> +		ret = i2c_smbus_write_byte_data(data->client, TCS3472_CONFIG,
>> +						config);
>> +		if (ret)
>> +			return ret;
>> +
>> +		data->wlong = wlong;
>> +	}
>> +
>> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_WTIME, wtime);
>> +	if (ret)
>> +		return ret;
>> +
>> +	data->wtime = wtime;
>> +	data->target_freq_hz = val;
>> +	data->target_freq_uhz = val2;
>> +
>> +	return 0;
>> +}
>> +
>>   static int tcs3472_write_raw(struct iio_dev *indio_dev,
>>   			       struct iio_chan_spec const *chan,
>>   			       int val, int val2, long mask)
>>   {
>>   	struct tcs3472_data *data = iio_priv(indio_dev);
>> +	int ret;
>>   	int i;
>>   
>>   	switch (mask) {
>> @@ -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;
>> +
>> +			/*
>> +			 * 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;
>>   	}
>> @@ -434,16 +611,15 @@ static const struct iio_info tcs3472_info = {
>>   static int tcs3472_powerdown(struct tcs3472_data *data)
>>   {
>>   	int ret;
>> -	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
>>   
>>   	guard(mutex)(&data->lock);
>>   
>>   	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>> -					data->enable & ~enable_mask);
>> +					data->enable & ~TCS3472_ENABLE_RUN);
>>   	if (ret)
>>   		return ret;
>>   
>> -	data->enable &= ~enable_mask;
>> +	data->enable &= ~TCS3472_ENABLE_RUN;
> This would be more consistent and easier to read if you have
> u8 value;
> 
> 	value = data->enable & ~TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON
> 	value &= ~TCS3472_ENABLE_RUN;
> 
> 	ret = i2c_...
> 	if (ret)
> 		return ret;
> 
> 	data->enable = value;
> 
> 	return 0;
> 
> Or something along those lines. No need for separate enable_mask local
> variable.
> 
> 	>
>>   	return 0;
>>   }
>> @@ -458,6 +634,7 @@ static int tcs3472_probe(struct i2c_client *client)
>>   	struct device *dev = &client->dev;
>>   	struct tcs3472_data *data;
>>   	struct iio_dev *indio_dev;
>> +	unsigned int cycle_us;
>>   	int ret;
>>   
>>   	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>> @@ -498,6 +675,16 @@ static int tcs3472_probe(struct i2c_client *client)
>>   		return ret;
>>   	data->atime = ret;
>>   
>> +	ret = i2c_smbus_read_byte_data(data->client, TCS3472_WTIME);
>> +	if (ret < 0)
>> +		return ret;
>> +	data->wtime = ret;
>> +
>> +	ret = i2c_smbus_read_byte_data(data->client, TCS3472_CONFIG);
>> +	if (ret < 0)
>> +		return ret;
>> +	data->wlong = (ret & TCS3472_CONFIG_WLONG) ? 1 : 0;
>> +
>>   	ret = i2c_smbus_read_word_data(data->client, TCS3472_AILT);
>>   	if (ret < 0)
>>   		return ret;
>> @@ -518,14 +705,29 @@ static int tcs3472_probe(struct i2c_client *client)
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	/* enable device */
>> -	data->enable = ret | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN;
>> +	/*
>> +	 * Enable the chip in its full running state, including WEN. The
>> +	 * actual wait time is controlled by the WTIME and WLONG registers,
>> +	 * which retain their power-on defaults until userspace writes to
>> +	 * sampling_frequency.
>> +	 */
>> +	data->enable = ret | TCS3472_ENABLE_RUN;
>>   	data->enable &= ~TCS3472_ENABLE_AIEN;
> 
> Here is a case that should look more like the one that follows.
> Use a local variable to build what is written and update the cache
> only if the write succeeds.
> 
>>   	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>>   		data->enable);
>>   	if (ret < 0)
>>   		return ret;
>>   
>> +	/*
>> +	 * Initialize target frequency from the chip's current state so that
>> +	 * subsequent integration_time changes via IIO_CHAN_INFO_INT_TIME can
>> +	 * preserve a meaningful sampling rate, even before userspace writes
>> +	 * sampling_frequency for the first time.
>> +	 */
>> +	cycle_us = tcs3472_cycle_time_us(data);
>> +	tcs3472_cycle_to_freq(cycle_us, &data->target_freq_hz,
>> +			      &data->target_freq_uhz);
>> +
>>   	ret = devm_add_action_or_reset(dev, tcs3472_powerdown_action, data);
>>   	if (ret)
>>   		return ret;
>> @@ -561,16 +763,23 @@ static int tcs3472_resume(struct device *dev)
>>   	struct tcs3472_data *data = iio_priv(i2c_get_clientdata(
>>   		to_i2c_client(dev)));
>>   	int ret;
>> -	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
>> +	u8 value;
>>   
>>   	guard(mutex)(&data->lock);
>>   
>> -	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>> -		data->enable | enable_mask);
>> +	/*
>> +	 * On resume, only PON and AEN need to be re-asserted. WEN must not
>> +	 * be forced on: its desired state was set by the user via
>> +	 * sampling_frequency and is already stored in data->enable, so we
>> +	 * read it from there to preserve the user's choice.
>> +	 */
>> +	value = data->enable | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN;
>> +
>> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
>>   	if (ret)
>>   		return ret;
>>   
>> -	data->enable |= enable_mask;
>> +	data->enable = value;
>>   
>>   	return 0;
>>   }
> 

Hi Jonathan,

Thanks for the review. I'd like to ask a design
question about Sashiko's finding on the cached data->enable.

Sashiko pointed out:

 > @@ -434,16 +611,15 @@ static const struct iio_info tcs3472_info = {
 >  static int tcs3472_powerdown(struct tcs3472_data *data)
 >  {
 >  	int ret;
 > -	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
 >
 >  	guard(mutex)(&data->lock);
 >
 >  	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
 > -					data->enable & ~enable_mask);
 > +					data->enable & ~TCS3472_ENABLE_RUN);
 >  	if (ret)
 >  		return ret;
 >
 > -	data->enable &= ~enable_mask;
 > +	data->enable &= ~TCS3472_ENABLE_RUN;
Does this permanently lose the WEN configuration after a suspend/resume cycle?
Because TCS3472_ENABLE_WEN is part of the TCS3472_ENABLE_RUN mask, this clears
the WEN bit in the cached data->enable. Upon resume, tcs3472_resume() attempts
to restore the configuration by reading this cache:
     value = data->enable | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN;
But since the WEN bit was permanently cleared in the cache during suspend, the
device always resumes with WEN disabled, discarding the user's sampling
frequency configuration.




I see two possible fixes:

1.  In tcs3472_powerdown(), clear only PON and AEN from data->enable
     (keeping WEN as the "user's last desired state"). This is minimal
     but introduces a divergence between data->enable and the actual
     hardware ENABLE register, which breaks the simple "cache of the
     HW register" semantic.
2.  Add a separate field (e.g. bool target_wen) that holds the user's
     desired WEN state, set in tcs3472_set_sampling_freq() and read
     back in tcs3472_resume(). data->enable then strictly remains a
     pure cache of the HW register.


With solution2, the functions would behave as follows:

   - tcs3472_set_sampling_freq(): when activating wait state, sets
     target_wen = true; when disabling it (frequency too high), sets
     target_wen = false. data->enable is then updated to match what
     is written to the chip.

   - tcs3472_cycle_time_us(): reads target_wen instead of
     (data->enable & WEN) to decide whether to include wtime_us in
     the cycle. This avoids reporting a stale wait time during a
     suspended state.

   - tcs3472_powerdown(): unchanged in semantics clears PON, AEN
     and WEN from both the chip and data->enable. target_wen is left
     untouched, since it represents the user's wish, not the HW
     state.

   - tcs3472_resume(): rebuilds the ENABLE value from
     PON | AEN | (target_wen ? WEN : 0), writes it to the chip, then
     updates data->enable to match.

   - tcs3472_probe(): initializes target_wen from the chip's current
     WEN bit (read at probe time).

Solution 2 feels cleaner to me, but it adds a field and a bit of
synchronization between target_wen and (data->enable & WEN). Do you
have another approach in mind?

Thanks,
Aldo