[PATCH] iio: frequency: adf4350: replace loop with order_base_2()

[PATCH] iio: frequency: adf4350: replace loop with order_base_2()

[Neel Bullywon]

Address the TODO in adf4350_set_freq() by replacing the iterative
power-of-2 shift loop with the standard order_base_2() macro.

By utilizing DIV_ROUND_UP_ULL(), we
can calculate the required RF divider selection in a single step.

This ensures freq is properly shifted to meet or exceed the minimum
VCO frequency.

Signed-off-by: Neel Bullywon <neelb2403@gmail.com>
---
 drivers/iio/frequency/adf4350.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
index ed1741165f55..e07be6d5e67e 100644
--- a/drivers/iio/frequency/adf4350.c
+++ b/drivers/iio/frequency/adf4350.c
@@ -17,6 +17,8 @@
 #include <linux/err.h>
 #include <linux/gcd.h>
 #include <linux/gpio/consumer.h>
+#include <linux/log2.h>
+#include <linux/math64.h>
 #include <asm/div64.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
@@ -149,18 +151,12 @@ static int adf4350_set_freq(struct adf4350_state *st, unsigned long long freq)
 	if (freq > ADF4350_MAX_OUT_FREQ || freq < st->min_out_freq)
 		return -EINVAL;
 
-	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.
+	 * Calculate the required RF divider selection (power of 2 shift)
+	 * to ensure the VCO frequency is >= ADF4350_MIN_VCO_FREQ.
 	 */
-	while (freq < ADF4350_MIN_VCO_FREQ) {
-		freq <<= 1;
-		st->r4_rf_div_sel++;
-	}
+	st->r4_rf_div_sel = order_base_2(DIV_ROUND_UP_ULL(ADF4350_MIN_VCO_FREQ, freq));
+	freq <<= st->r4_rf_div_sel;
 
 	if (freq > ADF4350_MAX_FREQ_45_PRESC) {
 		prescaler = ADF4350_REG1_PRESCALER;
-- 
2.44.0

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

[Andy Shevchenko]

On Tue, Mar 10, 2026 at 10:01:15PM -0400, Neel Bullywon wrote:
> Address the TODO in adf4350_set_freq() by replacing the iterative
> power-of-2 shift loop with the standard order_base_2() macro.
> 
> By utilizing DIV_ROUND_UP_ULL(), we
> can calculate the required RF divider selection in a single step.
> 
> This ensures freq is properly shifted to meet or exceed the minimum
> VCO frequency.

Thanks! My comments below.

...

> -	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.
> +	 * Calculate the required RF divider selection (power of 2 shift)
> +	 * to ensure the VCO frequency is >= ADF4350_MIN_VCO_FREQ.
>  	 */
> -	while (freq < ADF4350_MIN_VCO_FREQ) {
> -		freq <<= 1;
> -		st->r4_rf_div_sel++;
> -	}
> +	st->r4_rf_div_sel = order_base_2(DIV_ROUND_UP_ULL(ADF4350_MIN_VCO_FREQ, freq));

We don't need 64-bit division even with your approach as values are 32-bit for
this case (yeah, I know that they have 64-bit types).

What you need is to do as suggested by comment: use fls_long() against the
constant and freq to see the difference in value, if there is none the while
will become just a check and a single shift.

Or consider it as the opposite problem: "How much the given constant is now
bigger than freq?" With that it might be more obvious solution.

I.o.w. think about this a bit more.

> +	freq <<= st->r4_rf_div_sel;
>  

I would move this blank line to be before freq shift, this will associate it
with the check below.

>  	if (freq > ADF4350_MAX_FREQ_45_PRESC) {
>  		prescaler = ADF4350_REG1_PRESCALER;

-- 
With Best Regards,
Andy Shevchenko

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

[Neel Bullywon]

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

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

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

[Andy Shevchenko]

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

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

[Jonathan Cameron]

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

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

[David Laight]

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;

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

[Andy Shevchenko]

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

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

[Nuno Sá]

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.

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

[David Laight]

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.