Re: [PATCH v2] iio: frequency: adf4350: replace loop with fls_long()

[David Laight <david.laight.linux@gmail.com>]

On Sat, 14 Mar 2026 13:20:06 -0400
Neel Bullywon <neelb2403@gmail.com> wrote:

> Address the TODO in adf4350_set_freq() by replacing the iterative
> power-of-2 shift loop with a constant-time bitwise calculation.
> 
> By comparing the highest set bits of the target constant and freq
> using fls_long(), we can calculate the required RF divider selection
> in a single step without relying on expensive 64-bit division.

Where is the 64bit division?
(apart from in v1)
Indeed where are the 64bit values at all.
If this code is used on 32bit it has to work with a 32bit long.
Which makes be think that the 'freq' variable should be u32 (or possibly u64
if frequencies above 4GHz are likely - which I doubt).

In any case this looks like initialisation code and the existing loop
has the advantage of being 'obviously correct' and small.

	David  


> 
> This ensures freq is properly shifted to meet or exceed the minimum
> VCO frequency.
> 
> Signed-off-by: Neel Bullywon <neelb2403@gmail.com>
> ---
> Changes in v2:
>   - Use fls_long() instead of order_base_2(DIV_ROUND_UP_ULL()) to avoid
>     unnecessary 64-bit division (Andy Shevchenko)
>   - Add correction check for mantissa edge case
>   - Adjust whitespace per review feedback
> 
>  drivers/iio/frequency/adf4350.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
> index ed1741165f55..2183deec179d 100644
> --- a/drivers/iio/frequency/adf4350.c
> +++ b/drivers/iio/frequency/adf4350.c
> @@ -17,6 +17,7 @@
>  #include <linux/err.h>
>  #include <linux/gcd.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/bitops.h>
>  #include <asm/div64.h>
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
> @@ -151,17 +152,14 @@ static int adf4350_set_freq(struct adf4350_state *st, unsigned long long freq)
>  
>  	st->r4_rf_div_sel = 0;
>  
> -	/*
> -	 * !\TODO: The below computation is making sure we get a power of 2
> -	 * shift (st->r4_rf_div_sel) so that freq becomes higher or equal to
> -	 * ADF4350_MIN_VCO_FREQ. This might be simplified with fls()/fls_long()
> -	 * and friends.
> -	 */
> -	while (freq < ADF4350_MIN_VCO_FREQ) {
> -		freq <<= 1;
> -		st->r4_rf_div_sel++;
> +	if (freq < ADF4350_MIN_VCO_FREQ) {
> +		st->r4_rf_div_sel = fls_long(ADF4350_MIN_VCO_FREQ - 1) - fls_long(freq);
> +		if ((freq << st->r4_rf_div_sel) < ADF4350_MIN_VCO_FREQ)
> +			st->r4_rf_div_sel++;
>  	}
>  
> +	freq <<= st->r4_rf_div_sel;
> +
>  	if (freq > ADF4350_MAX_FREQ_45_PRESC) {
>  		prescaler = ADF4350_REG1_PRESCALER;
>  		mdiv = 75;