Re: [PATCH v2 4/5] iio: light: tcs3472: implement wait time and sampling frequency

On 13/05/26 13:17, Andy Shevchenko wrote:
> On Wed, May 13, 2026 at 12:32:14AM +0200, Aldo Conte 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() and tcs3472_resume().
>>
>> Remove the "TODO: wait time" comment at the top of the file.
> 
>> Suggested-by: Andy Shevchenko <[email protected]>
> 
> Inappropriate tag.
> You must not put tags on your own, if in doubt, ask first!
> 
> ...
> 
>> +#define TCS3472_ENABLE_RUN (TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON | \
>> +			    TCS3472_ENABLE_WEN)
> 
> Better style is
> 
> #define TCS3472_ENABLE_RUN						\
> 	(TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON | TCS3472_ENABLE_WEN)
> 
> ...
> 
>> +	cycle_us = div_u64((u64)PSEC_PER_SEC,
>> +			   (u64)val * USEC_PER_SEC + val2);
> 
> First of all, it's one line. Second, the divisor for this function is 32-bit.
> And at last the castings are not needed. I think I already told these...

In theory, val and val2 could have a value of INT_MAX. Therefore, the 
calculation INT_MAX * USEC_PER_SEC + INT_MAX results in a value of 
2,147,483,861,748,364,700, which can only be stored in 64 bits. This is, 
of course, an extremely rare case in which userspace writes an 
impossible frequency that is simply too high.
Therefore, div64_u64() should be used instead of div_u64().
Therefore, the following code would be required:

cycle_us = div64_u64(PSEC_PER_SEC, val * USEC_PER_SEC + val2);



> ...
> 
>> +	wtime = 256 - DIV_ROUND_CLOSEST_ULL(wait_us, 2400);
>> +	if (wtime < 0) {
>> +		wlong = true;
>> +		wtime = 256 - DIV_ROUND_CLOSEST_ULL(wait_us, 28800);
>> +	}
> 
> Why 64-bit divisions? Do you expect the wait_us be outside INT_MIN/INT_MAX range?
> This will need a comment and/or dropping the 64-bit arithmetics.

cycle_us_MIN is reached when (u64)val * USEC_PER_SEC + val2 is MAX and 
so is INT_MAX * USEC_PER_SEC + INT_MAX. Assuming this, the division 
would be 10⁶ / (INT_MAX * USEC_PER_SEC + INT_MAX) = 0.000465661, so 0.

Therefore, cycle_us_MIN would equal 0 and cycle_us_MAX would equal 
PSEC_PER_SEC/1, i.e. 10⁶; consequently, wait_us_MIN would equal −614400:

wait_us_min = 0−2400−612000= −614400

and wait_us_MAX would equal 999999995200:

wait_us_MAX = 1000000000000 −2400−2400 = 999999995200

This means that wait_us needs to be s64 if i am not wrong.
> 
>> +	wtime = clamp(wtime, 0, 255);
> 
> ...
> 
>> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_WTIME, wtime);
>> +	if (ret < 0)
> 
> What's the meaning of positive returned values? I think this function never
> does that. If I'm right, drop ' < 0' parts in all similar cases.
The i2c_smbus_write_byte_data() function returns a value of 0 on success 
and a value less than 0 in the event of an error. I do not understand 
this comment. Could you please explain it to me in more detail?

> 
>> +		return ret;
> 
> ...
> 
>> +		*val = USEC_PER_SEC / cycle_us;
>> +		*val2 = div_u64((u64)(USEC_PER_SEC % cycle_us) * USEC_PER_SEC,
>> +				cycle_us);
> 
> Is this even correct? We take modulo of cycle_us to get under the MICRO range,
> then multiply to MICRO (seconds) and divide by full cycle_us. I'm lost here.


> 
> ...
> 
>> +	cycle_us = tcs3472_cycle_time_us(data);
>> +	data->target_freq_hz = USEC_PER_SEC / cycle_us;
>> +	data->target_freq_uhz = div_u64((u64)(USEC_PER_SEC % cycle_us) *
>> +					USEC_PER_SEC, cycle_us);
> 
> Okay, this might help with the above... Can you deduplicate this division to
> a helper with a comment that explains the calculations behind?
> 
Something like this could be ok?

/*
  * Convert cycle time in microseconds to frequency in Hz and microhertz.
  *
  * Given:
  *   cycle_us = T, i.e. 1 cycle lasts T microseconds
  *
  * The frequency is:
  *   f = 10^6 / T    [Hz]
  *
  * We split it as:
  *   val = floor(10^6 / T)
  *   val2 = (10^6 mod T) * 10^6 / T   [microhertz]
  *
  * I.E.:
  *   val = 10^6 / T          (integer division)
  *   val2 = (10^6 % T) * 10^6 / T   (fraction scaled to microhertz)
  */
static void
us_cycle_to_freq_hz(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);
}


thank you,

Aldo