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@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;
-- 
2.44.0

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;

On Mon, Mar 16, 2026 at 01:51:51PM +0000, David Laight wrote:
> 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).

I don't know about _this_ device, but before looking into datasheet I wouldn't
put a low probability on the frequencies higher than 4.3GHz. We have (or going
to have) devices that work with up to 26GHz frequencies in this folder.

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

-- 
With Best Regards,
Andy Shevchenko

On Mon, 2026-03-16 at 16:07 +0200, Andy Shevchenko wrote:
> On Mon, Mar 16, 2026 at 01:51:51PM +0000, David Laight wrote:
> > 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).
> 
> I don't know about _this_ device, but before looking into datasheet I wouldn't
> put a low probability on the frequencies higher than 4.3GHz. We have (or going
> to have) devices that work with up to 26GHz frequencies in this folder.

Yes, it goes up to 4.4GHz.

- Nuno Sá

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

On Mon, 23 Mar 2026 11:08:15 +0000
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2026-03-16 at 16:07 +0200, Andy Shevchenko wrote:
> > On Mon, Mar 16, 2026 at 01:51:51PM +0000, David Laight wrote:  
> > > 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).  
> > 
> > I don't know about _this_ device, but before looking into datasheet I wouldn't
> > put a low probability on the frequencies higher than 4.3GHz. We have (or going
> > to have) devices that work with up to 26GHz frequencies in this folder.  
> 
> Yes, it goes up to 4.4GHz.

In which case all the 'long' need to be u64.

	David

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

On Sat, Mar 14, 2026 at 01:20:06PM -0400, Neel Bullywon 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.

...

>  	st->r4_rf_div_sel = 0;

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

Yeah, unfortunately it looks like too complicated in comparison with the
original code. Have you checked binary output? My gut feelings that this
gives also a lot of code in addition to.

Little optimisation can be like:

	if (freq < ADF4350_MIN_VCO_FREQ) {
		st->r4_rf_div_sel = fls_long(ADF4350_MIN_VCO_FREQ - 1) - fls_long(freq);
		freq <<= st->r4_rf_div_sel;
		if (freq < ADF4350_MIN_VCO_FREQ) {
			st->r4_rf_div_sel++;
			freq <<= 1;
		}
	}

but it is still too much.

...

Maybe we simply need to replace TODO with a NOTE explaining that if better algo
is found we can replace.

-- 
With Best Regards,
Andy Shevchenko

On Mon, 16 Mar 2026 14:40:39 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Sat, Mar 14, 2026 at 01:20:06PM -0400, Neel Bullywon 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.  
> 
> ...
> 
> >  	st->r4_rf_div_sel = 0;  
> 
> > -	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;  
> 
> Yeah, unfortunately it looks like too complicated in comparison with the
> original code. Have you checked binary output? My gut feelings that this
> gives also a lot of code in addition to.
> 
> Little optimisation can be like:
> 
> 	if (freq < ADF4350_MIN_VCO_FREQ) {
> 		st->r4_rf_div_sel = fls_long(ADF4350_MIN_VCO_FREQ - 1) - fls_long(freq);
> 		freq <<= st->r4_rf_div_sel;
> 		if (freq < ADF4350_MIN_VCO_FREQ) {
> 			st->r4_rf_div_sel++;
> 			freq <<= 1;
> 		}
> 	}
> 
> but it is still too much.
> 
> ...
> 
> Maybe we simply need to replace TODO with a NOTE explaining that if better algo
> is found we can replace.
> 
This comment update makes sense to me as will make people look for previous
attempts before sending a new one!

Neel, you ran into tricky challenge - thanks for giving it a go!

Jonathan

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.
> 
> This ensures freq is properly shifted to meet or exceed the minimum
> VCO frequency.
> 
> Signed-off-by: Neel Bullywon <neelb2403@gmail.com>
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;