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

On Fri, May 15, 2026 at 05:57:11PM +0200, Aldo Conte wrote:
> On 13/05/26 13:17, Andy Shevchenko wrote:
> > On Wed, May 13, 2026 at 12:32:14AM +0200, Aldo Conte wrote:

...

> > > +	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().

Yes, exactly my point!

> Therefore, the following code would be required:
> 
> cycle_us = div64_u64(PSEC_PER_SEC, val * USEC_PER_SEC + val2);

You will need casting for val because of 32-bit compilation which will
overflow otherwise.

...

> > > +	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.

Please, add a respective comment.

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

As you pointed out there is no meaning of positive returned value as there
should not be a such in the first place. Hence, why ' < 0'? This already being
explained just after the Q in the same paragraph above :-)

> > > +		return ret;

...

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

Looks like yes.

-- 
With Best Regards,
Andy Shevchenko