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

[Jonathan Cameron]

On Sat, 14 Mar 2026 13:20:06 -0400
Neel Bullywon <neelb2403@xxxxxxxxx> 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.
> 
> This ensures freq is properly shifted to meet or exceed the minimum
> VCO frequency.
> 
> Signed-off-by: Neel Bullywon <neelb2403@xxxxxxxxx>
Hi Neel,

I'll wait for Andy to comment on the new implementation.
In meantime a process comment.  Don't send a new version
in reply to old one. It tends to end up with deeply nested
messy email threads. I'm not entirely sure where people doing this
comes from. Maybe there is some part of the kernel where this style
is requested?  Or it's coming from other projects.

If you want to associate a new patch with an old one, put a link
to lore after the change log.

No need to resend this time though!


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