Skip to contentSkip to conversation

iio: filter: admv8818: fix range calculation

PATCH In review Sam Winchenbach 4 participants 15 messages Feb 25, 2025 – Mar 8, 2025

Subsystems (4)
Device tree bindings IIO Analog Devices IIO drivers Analog devices inc ADMV8818 driver
lore
Share & tools
lore

Sam Winchenbach

author
#0 lore raw

PATCH v4 1/2 iio: filter: admv8818: fix range calculation

Corrects the upper range of LPF Band 4 from 18.5 GHz to 18.85 GHz per the ADMV8818 datasheet

Search for the minimum error while ensuring that the LPF corner frequency is greater than the target, and the HPF corner frequency is lower than the target

This fixes issues where the range calculations were suboptimal.

Add two new DTS properties to set the margin between the input frequency and the calculated corner frequency

Below is a generated table of the differences between the old algorithm and the new. This is a sweep from 0 to 20 GHz in 10 MHz steps. === HPF ===
freq = 1750 MHz, 3db: bypass => 1750 MHz
freq = 3400 MHz, 3db: 3310 => 3400 MHz
freq = 3410 MHz, 3db: 3310 => 3400 MHz
freq = 3420 MHz, 3db: 3310 => 3400 MHz
freq = 3660 MHz, 3db: 3550 => 3656 MHz
freq = 6600 MHz, 3db: 6479 => 6600 MHz
freq = 6610 MHz, 3db: 6479 => 6600 MHz
freq = 6620 MHz, 3db: 6479 => 6600 MHz
freq = 6630 MHz, 3db: 6479 => 6600 MHz
freq = 6640 MHz, 3db: 6479 => 6600 MHz
freq = 6650 MHz, 3db: 6479 => 6600 MHz
freq = 6660 MHz, 3db: 6479 => 6600 MHz
freq = 6670 MHz, 3db: 6479 => 6600 MHz
freq = 6680 MHz, 3db: 6479 => 6600 MHz
freq = 6690 MHz, 3db: 6479 => 6600 MHz
freq = 6700 MHz, 3db: 6479 => 6600 MHz
freq = 6710 MHz, 3db: 6479 => 6600 MHz
freq = 6720 MHz, 3db: 6479 => 6600 MHz
freq = 6730 MHz, 3db: 6479 => 6600 MHz
freq = 6960 MHz, 3db: 6736 => 6960 MHz
freq = 6970 MHz, 3db: 6736 => 6960 MHz
freq = 6980 MHz, 3db: 6736 => 6960 MHz
freq = 6990 MHz, 3db: 6736 => 6960 MHz
freq = 7320 MHz, 3db: 7249 => 7320 MHz
freq = 7330 MHz, 3db: 7249 => 7320 MHz
freq = 7340 MHz, 3db: 7249 => 7320 MHz
freq = 7350 MHz, 3db: 7249 => 7320 MHz
freq = 7360 MHz, 3db: 7249 => 7320 MHz
freq = 7370 MHz, 3db: 7249 => 7320 MHz
freq = 7380 MHz, 3db: 7249 => 7320 MHz
freq = 7390 MHz, 3db: 7249 => 7320 MHz
freq = 7400 MHz, 3db: 7249 => 7320 MHz
freq = 7410 MHz, 3db: 7249 => 7320 MHz
freq = 7420 MHz, 3db: 7249 => 7320 MHz
freq = 7430 MHz, 3db: 7249 => 7320 MHz
freq = 7440 MHz, 3db: 7249 => 7320 MHz
freq = 7450 MHz, 3db: 7249 => 7320 MHz
freq = 7460 MHz, 3db: 7249 => 7320 MHz
freq = 7470 MHz, 3db: 7249 => 7320 MHz
freq = 7480 MHz, 3db: 7249 => 7320 MHz
freq = 7490 MHz, 3db: 7249 => 7320 MHz
freq = 7500 MHz, 3db: 7249 => 7320 MHz
freq = 12500 MHz, 3db: 12000 => 12500 MHz

=== LPF ===
freq = 2050 MHz, 3db: bypass => 2050 MHz freq = 2170 MHz, 3db: 2290 => 2170 MHz freq = 2290 MHz, 3db: 2410 => 2290 MHz freq = 2410 MHz, 3db: 2530 => 2410 MHz freq = 2530 MHz, 3db: 2650 => 2530 MHz freq = 2650 MHz, 3db: 2770 => 2650 MHz freq = 2770 MHz, 3db: 2890 => 2770 MHz freq = 2890 MHz, 3db: 3010 => 2890 MHz freq = 3010 MHz, 3db: 3130 => 3010 MHz freq = 3130 MHz, 3db: 3250 => 3130 MHz freq = 3250 MHz, 3db: 3370 => 3250 MHz freq = 3260 MHz, 3db: 3370 => 3350 MHz freq = 3270 MHz, 3db: 3370 => 3350 MHz freq = 3280 MHz, 3db: 3370 => 3350 MHz freq = 3290 MHz, 3db: 3370 => 3350 MHz freq = 3300 MHz, 3db: 3370 => 3350 MHz freq = 3310 MHz, 3db: 3370 => 3350 MHz freq = 3320 MHz, 3db: 3370 => 3350 MHz freq = 3330 MHz, 3db: 3370 => 3350 MHz freq = 3340 MHz, 3db: 3370 => 3350 MHz freq = 3350 MHz, 3db: 3370 => 3350 MHz freq = 3370 MHz, 3db: 3490 => 3370 MHz freq = 3490 MHz, 3db: 3610 => 3490 MHz freq = 3610 MHz, 3db: 3730 => 3610 MHz freq = 3730 MHz, 3db: 3850 => 3730 MHz freq = 3850 MHz, 3db: 3870 => 3850 MHz freq = 3870 MHz, 3db: 4130 => 3870 MHz freq = 4130 MHz, 3db: 4390 => 4130 MHz freq = 4390 MHz, 3db: 4650 => 4390 MHz freq = 4650 MHz, 3db: 4910 => 4650 MHz freq = 4910 MHz, 3db: 5170 => 4910 MHz freq = 5170 MHz, 3db: 5430 => 5170 MHz freq = 5430 MHz, 3db: 5690 => 5430 MHz freq = 5690 MHz, 3db: 5950 => 5690 MHz freq = 5950 MHz, 3db: 6210 => 5950 MHz freq = 6210 MHz, 3db: 6470 => 6210 MHz freq = 6470 MHz, 3db: 6730 => 6470 MHz freq = 6730 MHz, 3db: 6990 => 6730 MHz freq = 6990 MHz, 3db: 7250 => 6990 MHz freq = 7000 MHz, 3db: 7250 => 7000 MHz freq = 7250 MHz, 3db: 7400 => 7250 MHz freq = 7400 MHz, 3db: 7800 => 7400 MHz freq = 7800 MHz, 3db: 8200 => 7800 MHz freq = 8200 MHz, 3db: 8600 => 8200 MHz freq = 8600 MHz, 3db: 9000 => 8600 MHz freq = 9000 MHz, 3db: 9400 => 9000 MHz freq = 9400 MHz, 3db: 9800 => 9400 MHz freq = 9800 MHz, 3db: 10200 => 9800 MHz freq = 10200 MHz, 3db: 10600 => 10200 MHz freq = 10600 MHz, 3db: 11000 => 10600 MHz freq = 11000 MHz, 3db: 11400 => 11000 MHz freq = 11400 MHz, 3db: 11800 => 11400 MHz freq = 11800 MHz, 3db: 12200 => 11800 MHz freq = 12200 MHz, 3db: 12600 => 12200 MHz freq = 12210 MHz, 3db: 12600 => 12550 MHz freq = 12220 MHz, 3db: 12600 => 12550 MHz freq = 12230 MHz, 3db: 12600 => 12550 MHz freq = 12240 MHz, 3db: 12600 => 12550 MHz freq = 12250 MHz, 3db: 12600 => 12550 MHz freq = 12260 MHz, 3db: 12600 => 12550 MHz freq = 12270 MHz, 3db: 12600 => 12550 MHz freq = 12280 MHz, 3db: 12600 => 12550 MHz freq = 12290 MHz, 3db: 12600 => 12550 MHz freq = 12300 MHz, 3db: 12600 => 12550 MHz freq = 12310 MHz, 3db: 12600 => 12550 MHz freq = 12320 MHz, 3db: 12600 => 12550 MHz freq = 12330 MHz, 3db: 12600 => 12550 MHz freq = 12340 MHz, 3db: 12600 => 12550 MHz freq = 12350 MHz, 3db: 12600 => 12550 MHz freq = 12360 MHz, 3db: 12600 => 12550 MHz freq = 12370 MHz, 3db: 12600 => 12550 MHz freq = 12380 MHz, 3db: 12600 => 12550 MHz freq = 12390 MHz, 3db: 12600 => 12550 MHz freq = 12400 MHz, 3db: 12600 => 12550 MHz freq = 12410 MHz, 3db: 12600 => 12550 MHz freq = 12420 MHz, 3db: 12600 => 12550 MHz freq = 12430 MHz, 3db: 12600 => 12550 MHz freq = 12440 MHz, 3db: 12600 => 12550 MHz freq = 12450 MHz, 3db: 12600 => 12550 MHz freq = 12460 MHz, 3db: 12600 => 12550 MHz freq = 12470 MHz, 3db: 12600 => 12550 MHz freq = 12480 MHz, 3db: 12600 => 12550 MHz freq = 12490 MHz, 3db: 12600 => 12550 MHz freq = 12500 MHz, 3db: 12600 => 12550 MHz freq = 12510 MHz, 3db: 12600 => 12550 MHz freq = 12520 MHz, 3db: 12600 => 12550 MHz freq = 12530 MHz, 3db: 12600 => 12550 MHz freq = 12540 MHz, 3db: 12600 => 12550 MHz freq = 12550 MHz, 3db: 12600 => 12550 MHz freq = 12600 MHz, 3db: 13000 => 12600 MHz freq = 12610 MHz, 3db: 13000 => 12970 MHz freq = 12620 MHz, 3db: 13000 => 12970 MHz freq = 12630 MHz, 3db: 13000 => 12970 MHz freq = 12640 MHz, 3db: 13000 => 12970 MHz freq = 12650 MHz, 3db: 13000 => 12970 MHz freq = 12660 MHz, 3db: 13000 => 12970 MHz freq = 12670 MHz, 3db: 13000 => 12970 MHz freq = 12680 MHz, 3db: 13000 => 12970 MHz freq = 12690 MHz, 3db: 13000 => 12970 MHz freq = 12700 MHz, 3db: 13000 => 12970 MHz freq = 12710 MHz, 3db: 13000 => 12970 MHz freq = 12720 MHz, 3db: 13000 => 12970 MHz freq = 12730 MHz, 3db: 13000 => 12970 MHz freq = 12740 MHz, 3db: 13000 => 12970 MHz freq = 12750 MHz, 3db: 13000 => 12970 MHz freq = 12760 MHz, 3db: 13000 => 12970 MHz freq = 12770 MHz, 3db: 13000 => 12970 MHz freq = 12780 MHz, 3db: 13000 => 12970 MHz freq = 12790 MHz, 3db: 13000 => 12970 MHz freq = 12800 MHz, 3db: 13000 => 12970 MHz freq = 12810 MHz, 3db: 13000 => 12970 MHz freq = 12820 MHz, 3db: 13000 => 12970 MHz freq = 12830 MHz, 3db: 13000 => 12970 MHz freq = 12840 MHz, 3db: 13000 => 12970 MHz freq = 12850 MHz, 3db: 13000 => 12970 MHz freq = 12860 MHz, 3db: 13000 => 12970 MHz freq = 12870 MHz, 3db: 13000 => 12970 MHz freq = 12880 MHz, 3db: 13000 => 12970 MHz freq = 12890 MHz, 3db: 13000 => 12970 MHz freq = 12900 MHz, 3db: 13000 => 12970 MHz freq = 12910 MHz, 3db: 13000 => 12970 MHz freq = 12920 MHz, 3db: 13000 => 12970 MHz freq = 12930 MHz, 3db: 13000 => 12970 MHz freq = 12940 MHz, 3db: 13000 => 12970 MHz freq = 12950 MHz, 3db: 13000 => 12970 MHz freq = 12960 MHz, 3db: 13000 => 12970 MHz freq = 12970 MHz, 3db: 13000 => 12970 MHz freq = 13000 MHz, 3db: 13390 => 13000 MHz freq = 13390 MHz, 3db: 13810 => 13390 MHz freq = 13810 MHz, 3db: 14230 => 13810 MHz freq = 14230 MHz, 3db: 14650 => 14230 MHz freq = 14650 MHz, 3db: 15070 => 14650 MHz freq = 15070 MHz, 3db: 15490 => 15070 MHz freq = 15490 MHz, 3db: 15910 => 15490 MHz freq = 15910 MHz, 3db: 16330 => 15910 MHz freq = 16330 MHz, 3db: 16750 => 16330 MHz freq = 16750 MHz, 3db: 17170 => 16750 MHz freq = 17170 MHz, 3db: 17590 => 17170 MHz freq = 17590 MHz, 3db: 18010 => 17590 MHz freq = 18010 MHz, 3db: 18430 => 18010 MHz freq = 18430 MHz, 3db: 18850 => 18430 MHz freq = 18850 MHz, 3db: bypass => 18850 MHz

Fixes f34fe888ad05 ("iio:filter:admv8818: add support for ADMV8818")
Signed-off-by Sam Winchenbach <[email protected]>

---
V1 -> V2: Cleaned up the wording of the commit message V2 -> V3: Add DTS properties to control corner frequency margins

---
 drivers/iio/filter/admv8818.c | 136 ++++++++++++++++++++++++++--------
 1 file changed, 105 insertions(+), 31 deletions(-)
drivers/iio/filter/admv8818.c +105 -31
index 848baa6e3bbf..a446d8d421ae 100644
@@ -90,6 +90,8 @@ struct admv8818_state {
 	struct mutex		lock;
 	unsigned int		filter_mode;
 	u64			cf_hz;
+	u64			lpf_margin_hz;
+	u64			hpf_margin_hz;
 };
 
 static const unsigned long long freq_range_hpf[4][2] = {

@@ -103,7 +105,7 @@ static const unsigned long long freq_range_lpf[4][2] = {
 	{2050000000ULL, 3850000000ULL},
 	{3350000000ULL, 7250000000ULL},
 	{7000000000, 13000000000},
-	{12550000000, 18500000000}
+	{12550000000, 18850000000}
 };
 
 static const struct regmap_config admv8818_regmap_config = {

@@ -122,43 +124,59 @@ static const char * const admv8818_modes[] = {
 static int __admv8818_hpf_select(struct admv8818_state *st, u64 freq)
 {
 	unsigned int hpf_step = 0, hpf_band = 0, i, j;
+	u64 freq_error;
+	u64 min_freq_error;
+	u64 freq_corner;
 	u64 freq_step;
 	int ret;
 
 	if (freq < freq_range_hpf[0][0])
 		goto hpf_write;
 
-	if (freq > freq_range_hpf[3][1]) {
+	if (freq >= freq_range_hpf[3][1]) {
 		hpf_step = 15;
 		hpf_band = 4;
 
 		goto hpf_write;
 	}
 
+	/* Close HPF frequency gap between 12 and 12.5 GHz */
+	if (freq >= 12000 * HZ_PER_MHZ && freq < 12500 * HZ_PER_MHZ) {
+		hpf_step = 15;
+		hpf_band = 3;
+
+		goto hpf_write;
+	}
+
+	min_freq_error = U64_MAX;
 	for (i = 0; i < 4; i++) {
+		/* This (and therefore all other ranges) have a corner
+		 * frequency higher than the target frequency.
+		 */
+		if (freq_range_hpf[i][0] > freq)
+			break;
+
 		freq_step = div_u64((freq_range_hpf[i][1] -
 			freq_range_hpf[i][0]), 15);
 
-		if (freq > freq_range_hpf[i][0] &&
-		    (freq < freq_range_hpf[i][1] + freq_step)) {
-			hpf_band = i + 1;
+		for (j = 0; j <= 15; j++) {
+			freq_corner = freq_range_hpf[i][0] + (freq_step * j);
+
+			/* This (and therefore all other steps) have a corner
+			 * frequency higher than the target frequency.
+			 */
+			if (freq_corner > freq)
+				break;
 
-			for (j = 1; j <= 16; j++) {
-				if (freq < (freq_range_hpf[i][0] + (freq_step * j))) {
-					hpf_step = j - 1;
-					break;
-				}
+			freq_error = freq - freq_corner;
+			if (freq_error < min_freq_error) {
+				min_freq_error = freq_error;
+				hpf_step = j;
+				hpf_band = i + 1;
 			}
-			break;
 		}
 	}
 
-	/* Close HPF frequency gap between 12 and 12.5 GHz */
-	if (freq >= 12000 * HZ_PER_MHZ && freq <= 12500 * HZ_PER_MHZ) {
-		hpf_band = 3;
-		hpf_step = 15;
-	}
-
 hpf_write:
 	ret = regmap_update_bits(st->regmap, ADMV8818_REG_WR0_SW,
 				 ADMV8818_SW_IN_SET_WR0_MSK |

@@ -186,7 +204,11 @@ static int admv8818_hpf_select(struct admv8818_state *st, u64 freq)
 
 static int __admv8818_lpf_select(struct admv8818_state *st, u64 freq)
 {
-	unsigned int lpf_step = 0, lpf_band = 0, i, j;
+	int i, j;
+	unsigned int lpf_step = 0, lpf_band = 0;
+	u64 freq_error;
+	u64 min_freq_error;
+	u64 freq_corner;
 	u64 freq_step;
 	int ret;
 

@@ -199,18 +221,34 @@ static int __admv8818_lpf_select(struct admv8818_state *st, u64 freq)
 		goto lpf_write;
 	}
 
-	for (i = 0; i < 4; i++) {
-		if (freq > freq_range_lpf[i][0] && freq < freq_range_lpf[i][1]) {
-			lpf_band = i + 1;
-			freq_step = div_u64((freq_range_lpf[i][1] - freq_range_lpf[i][0]), 15);
-
-			for (j = 0; j <= 15; j++) {
-				if (freq < (freq_range_lpf[i][0] + (freq_step * j))) {
-					lpf_step = j;
-					break;
-				}
-			}
+	min_freq_error = U64_MAX;
+	for (i = 3; i >= 0; --i) {
+		/* At this point the highest corner frequency of
+		 * all remaining ranges is below the target.
+		 * LPF corner should be >= the target.
+		 */
+		if (freq > freq_range_lpf[i][1])
 			break;
+
+		freq_step = div_u64((freq_range_lpf[i][1] - freq_range_lpf[i][0]), 15);
+
+		for (j = 15; j >= 0; --j) {
+
+			freq_corner = freq_range_lpf[i][0] + j*freq_step;
+
+			/* At this point all other steps in range will
+			 * place the corner frequency below the target
+			 * LPF corner should >= the target.
+			 */
+			if (freq > freq_corner)
+				break;
+
+			freq_error = freq_corner - freq;
+			if (freq_error < min_freq_error) {
+				min_freq_error = freq_error;
+				lpf_step = j;
+				lpf_band = i + 1;
+			}
 		}
 	}
 

@@ -242,16 +280,28 @@ static int admv8818_lpf_select(struct admv8818_state *st, u64 freq)
 static int admv8818_rfin_band_select(struct admv8818_state *st)
 {
 	int ret;
+	u64 hpf_corner_target, lpf_corner_target;
 
 	st->cf_hz = clk_get_rate(st->clkin);
 
+	// Check for underflow
+	if (st->cf_hz > st->hpf_margin_hz)
+		hpf_corner_target = st->cf_hz - st->hpf_margin_hz;
+	else
+		hpf_corner_target = 0;
+
+	// Check for overflow
+	lpf_corner_target = st->cf_hz + st->lpf_margin_hz;
+	if (lpf_corner_target < st->cf_hz)
+		lpf_corner_target = U64_MAX;
+
 	mutex_lock(&st->lock);
 
-	ret = __admv8818_hpf_select(st, st->cf_hz);
+	ret = __admv8818_hpf_select(st, hpf_corner_target);
 	if (ret)
 		goto exit;
 
-	ret = __admv8818_lpf_select(st, st->cf_hz);
+	ret = __admv8818_lpf_select(st, lfp_corner_target);
 exit:
 	mutex_unlock(&st->lock);
 	return ret;

@@ -647,6 +697,26 @@ static int admv8818_clk_setup(struct admv8818_state *st)
 	return devm_add_action_or_reset(&spi->dev, admv8818_clk_notifier_unreg, st);
 }
 
+static int admv8818_read_properties(struct admv8818_state *st)
+{
+	struct spi_device *spi = st->spi;
+	int ret;
+
+	ret = device_property_read_u64(&spi->dev, "adi,lpf-margin-hz", &st->lpf_margin_hz);
+	if (ret == -EINVAL)
+		st->lpf_margin_hz = 0;
+	else if (ret < 0)
+		return ret;
+
+	ret = device_property_read_u64(&spi->dev, "adi,hpf-margin-hz", &st->hpf_margin_hz);
+	if (ret == -EINVAL)
+		st->hpf_margin_hz = 0;
+	else if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static int admv8818_probe(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev;

@@ -678,6 +748,10 @@ static int admv8818_probe(struct spi_device *spi)
 
 	mutex_init(&st->lock);
 
+	ret = admv8818_read_properties(st);
+	if (ret)
+		return ret;
+
 	ret = admv8818_init(st);
 	if (ret)
 		return ret;
Signature · 1 line
2.48.1
author
#1 lore raw

PATCH v4 2/2 dt-bindings: iio: filter: Add lpf/hpf freq margins

Adds two properties to add a margin when automatically finding the corner frequencies.

Signed-off-by Sam Winchenbach <[email protected]>
---
 .../bindings/iio/filter/adi,admv8818.yaml          | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml +14 -0
index b77e855bd594..2acdbd8d84cb 100644
@@ -44,6 +44,18 @@ properties:
   '#clock-cells':
     const: 0
 
+  adi,lpf-margin-hz:
+    description:
+      Sets minimum low-pass corner frequency to the frequency of rf_in plus
+      this value when in auto mode.
+    default: 0
+
+  adi,hpf-margin-hz:
+    description:
+      Sets maximum high-pass corner frequency to the frequency of rf_in minus
+      this value when in auto mode.
+    default: 0
+
 required:
   - compatible
   - reg

@@ -61,6 +73,8 @@ examples:
         spi-max-frequency = <10000000>;
         clocks = <&admv8818_rfin>;
         clock-names = "rf_in";
+        adi,lpf-margin-hz = /bits/ 64 <30000000>;
+        adi,hpf-margin-hz = /bits/ 64 <30000000>;
       };
     };
 ...
Signature · 1 line
2.48.1
#2 lore raw

Re: [PATCH v4 2/2] dt-bindings: iio: filter: Add lpf/hpf freq margins

Quoted 7 lines · Sam Winchenbach

Adds two properties to add a margin when automatically finding the corner frequencies.

Signed-off-by Sam Winchenbach <[email protected]>
---
 .../bindings/iio/filter/adi,admv8818.yaml          | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Bindings are before users (see DT submitting patches), so this should be re-ordered.

Quoted 9 lines
Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml +0 -0
index b77e855bd594..2acdbd8d84cb 100644
@@ -44,6 +44,18 @@ properties:
   '#clock-cells':
     const: 0

Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml properties: · line 56

Quoted 10 lines
Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml +10 -0
+  adi,lpf-margin-hz:
+    description:
+      Sets minimum low-pass corner frequency to the frequency of rf_in plus
+      this value when in auto mode.
+    default: 0
+
+  adi,hpf-margin-hz:
+    description:
+      Sets maximum high-pass corner frequency to the frequency of rf_in minus
+      this value when in auto mode.

IIUC, these are two bounds - lower and upper - in relation to something else (like rf_in frequency)? If so, make it an array (naming to be discuss, I assume you know better what's that):

adi,filter-margin-hz:
items:
- description: low-pass corner frequency to the freq.....
minimum: xxxx?
maximum: xxxx?
default: 0
- description: high-pass ....
minimum: xxxx?
maximum: xxxx?
default: 0

Best regards,
Krzysztof

author
#3 lore raw

Re: [PATCH v4 2/2] dt-bindings: iio: filter: Add lpf/hpf freq margins

Quoted 21 lines · Krzysztof Kozlowski
Quoted 7 lines · Sam Winchenbach

Adds two properties to add a margin when automatically finding the corner frequencies.

Signed-off-by Sam Winchenbach <[email protected]>
---
 .../bindings/iio/filter/adi,admv8818.yaml          | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Bindings are before users (see DT submitting patches), so this should be re-ordered.

Quoted 9 lines
Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml +0 -0
index b77e855bd594..2acdbd8d84cb 100644
@@ -44,6 +44,18 @@ properties:
   '#clock-cells':
     const: 0
Quoted 14 lines

Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml properties: · line 56

Quoted 10 lines
Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml +10 -0
+  adi,lpf-margin-hz:
+    description:
+      Sets minimum low-pass corner frequency to the frequency of rf_in plus
+      this value when in auto mode.
+    default: 0
+
+  adi,hpf-margin-hz:
+    description:
+      Sets maximum high-pass corner frequency to the frequency of rf_in minus
+      this value when in auto mode.

IIUC, these are two bounds - lower and upper - in relation to something else (like rf_in frequency)? If so, make it an array (naming to be discuss, I assume you know better what's that):

It is true that these are both related to rf_in but both the low and high pass filters can operate independently. Logically, IMO, it makes more sense to have them as separate controls but I am happy to put them into an array if that is the idiomatic approach to situations like this. That said, I am having a difficult time getting dt_binding_check to pass when I have an array of uint64.

When listing two items, as in your example below, I get the following:
adi,admv8818.example.dtb: admv8818@0: adi,filter-margins-hz: [[0, 30000000], [0, 30000000]] is too long

I have tried specifying the scheme for each item, setting minItems/maxItems.

Any advice on this would be appreciated.
Thanks.

Quoted 15 lines

adi,filter-margin-hz:
items:
- description: low-pass corner frequency to the freq.....
minimum: xxxx?
maximum: xxxx?
default: 0
- description: high-pass ....
minimum: xxxx?
maximum: xxxx?
default: 0

Best regards,
Krzysztof

#4 lore raw

Re: [PATCH v4 2/2] dt-bindings: iio: filter: Add lpf/hpf freq margins

Quoted 37 lines · Sam Winchenbach
Quoted 21 lines · Krzysztof Kozlowski
Quoted 7 lines · Sam Winchenbach

Adds two properties to add a margin when automatically finding the corner frequencies.

Signed-off-by Sam Winchenbach <[email protected]>
---
 .../bindings/iio/filter/adi,admv8818.yaml          | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Bindings are before users (see DT submitting patches), so this should be re-ordered.

Quoted 9 lines
Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml +0 -0
index b77e855bd594..2acdbd8d84cb 100644
@@ -44,6 +44,18 @@ properties:
   '#clock-cells':
     const: 0
Quoted 14 lines

Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml properties: · line 56

Quoted 10 lines
Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml +10 -0
+  adi,lpf-margin-hz:
+    description:
+      Sets minimum low-pass corner frequency to the frequency of rf_in plus
+      this value when in auto mode.
+    default: 0
+
+  adi,hpf-margin-hz:
+    description:
+      Sets maximum high-pass corner frequency to the frequency of rf_in minus
+      this value when in auto mode.

IIUC, these are two bounds - lower and upper - in relation to something else (like rf_in frequency)? If so, make it an array (naming to be discuss, I assume you know better what's that):

Quoted 2 lines

It is true that these are both related to rf_in but both the low and high pass filters can operate independently. Logically, IMO, it makes more sense to have

You mean you can set only low or high pass and keep other as default? But what is the default then - something from reset value or "0" means disabled?

Quoted 6 lines

them as separate controls but I am happy to put them into an array if that is the idiomatic approach to situations like this. That said, I am having a difficult time getting dt_binding_check to pass when I have an array of uint64.

When listing two items, as in your example below, I get the following:
adi,admv8818.example.dtb: admv8818@0: adi,filter-margins-hz: [[0, 30000000], [0, 30000000]] is too long

Tricky to say without seeing your code. Magic crystal ball had malfunction today.

Best regards,
Krzysztof

author
#5 lore raw

Re: [PATCH v4 2/2] dt-bindings: iio: filter: Add lpf/hpf freq margins

Quoted 38 lines · Krzysztof Kozlowski
Quoted 37 lines · Sam Winchenbach
Quoted 35 lines · Krzysztof Kozlowski
Quoted 7 lines · Sam Winchenbach

Adds two properties to add a margin when automatically finding the corner frequencies.

Signed-off-by Sam Winchenbach <[email protected]>
---
 .../bindings/iio/filter/adi,admv8818.yaml          | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Bindings are before users (see DT submitting patches), so this should be re-ordered.

Quoted 9 lines
Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml +0 -0
index b77e855bd594..2acdbd8d84cb 100644
@@ -44,6 +44,18 @@ properties:
   '#clock-cells':
     const: 0

Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml properties: · line 56

Quoted 10 lines
Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml +10 -0
+  adi,lpf-margin-hz:
+    description:
+      Sets minimum low-pass corner frequency to the frequency of rf_in plus
+      this value when in auto mode.
+    default: 0
+
+  adi,hpf-margin-hz:
+    description:
+      Sets maximum high-pass corner frequency to the frequency of rf_in minus
+      this value when in auto mode.

IIUC, these are two bounds - lower and upper - in relation to something else (like rf_in frequency)? If so, make it an array (naming to be discuss, I assume you know better what's that):

Quoted 7 lines
Quoted 2 lines

It is true that these are both related to rf_in but both the low and high pass filters can operate independently. Logically, IMO, it makes more sense to have

You mean you can set only low or high pass and keep other as default? But what is the default then - something from reset value or "0" means disabled?

This value isn't setting the corner frequency of the filter, but the minimum distance the corner must be from the fundamental frequency. So, for example, if rf_in is 3.35 GHz and you set lpf-margin-hz to 0 then the corner frequency will be set to 3.35 GHz because that is an exact value supported by the device.

If lpf-margin-hz is set to 30 MHz (for example), then corner frequency would be at least 3.35 GHz + 30 MHz = 3.38 GHz. 3.49 GHz is the closest corner frequency without going below 3.38 GHz that is supported by the device, so that is what will be selected.

This prevents the situation where your fundamental frequency falls on, or close to, a corner frequency which could result in 3dB (half power) loss in your signal.

This is all completely indepent of the high-pass filter.

Quoted 10 lines
Quoted 7 lines

them as separate controls but I am happy to put them into an array if that is the idiomatic approach to situations like this. That said, I am having a difficult time getting dt_binding_check to pass when I have an array of uint64.

When listing two items, as in your example below, I get the following:
adi,admv8818.example.dtb: admv8818@0: adi,filter-margins-hz: [[0, 30000000], [0, 30000000]] is too long

Tricky to say without seeing your code. Magic crystal ball had malfunction today.

This is the property:

adi,filter-margins-hz:
items:
- description: |
The minimum distance, in Hz, between rf_in and the low-pass corner frequency when the device is used in "auto" mode. If the sum of rf_in and this value is greater than 18.85 GHz then the low-pass filter will be put into bypass mode, otherwise the closest corner frequency that is greater than or equal to the sum of rf_in plus this value will be used.
minimum: 0
maximum: 0xFFFFFFFFFFFFFFFF
default: 0
- description: |
The minimum distance, in Hz, between rf_in and the high-pass corner frequency when the device is used in "auto" mode. If the difference between rf_in and this value is less than 1.75 GHz then the high-pass filter will be put into bypass mode, otherwise the closest corner frequency that is less than or equal to the difference of rf_in and this value will be used.
minimum: 0
maximum: 0xFFFFFFFFFFFFFFFF
default: 0

And this is the example:

examples:
- |

    spi {
      #address-cells = <1>;
      #size-cells = <0>;
      admv8818@0 {
        compatible = "adi,admv8818";
        reg = <0>;
        spi-max-frequency = <10000000>;
        clocks = <&admv8818_rfin>;
        clock-names = "rf_in";
        adi,filter-margins-hz = /bits/ 64 <30000000 30000000>;
      };
    };

...

Thank you for taking the time to go through this,
-Sam

Quoted 3 lines

Best regards,
Krzysztof

#6 lore raw

Re: [PATCH v4 2/2] dt-bindings: iio: filter: Add lpf/hpf freq margins

Quoted 27 lines · Sam Winchenbach
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines

Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml properties: · line 56

Quoted 4 lines
Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml +4 -0
+  adi,hpf-margin-hz:
+    description:
+      Sets maximum high-pass corner frequency to the frequency of rf_in minus
+      this value when in auto mode.

IIUC, these are two bounds - lower and upper - in relation to something else (like rf_in frequency)? If so, make it an array (naming to be discuss, I assume you know better what's that):

Quoted 7 lines
Quoted 2 lines

It is true that these are both related to rf_in but both the low and high pass filters can operate independently. Logically, IMO, it makes more sense to have

You mean you can set only low or high pass and keep other as default? But what is the default then - something from reset value or "0" means disabled?

This value isn't setting the corner frequency of the filter, but the minimum distance the corner must be from the fundamental frequency. So, for example, if rf_in is 3.35 GHz and you set lpf-margin-hz to 0 then the corner frequency will be set to 3.35 GHz because that is an exact value supported by the device.

If lpf-margin-hz is set to 30 MHz (for example), then corner frequency would be at least 3.35 GHz + 30 MHz = 3.38 GHz. 3.49 GHz is the closest corner frequency without going below 3.38 GHz that is supported by the device, so that is what will be selected.

Quoted 5 lines

This prevents the situation where your fundamental frequency falls on, or close to, a corner frequency which could result in 3dB (half power) loss in your signal.

This is all completely indepent of the high-pass filter.

Description is confusing a bit, because it suggests the value sets the corner frequency. It explicitly says this - "sets ... corner frequency" and such meaning for properties we usually associate with the property doing this. Here however corner frequency will be always set to rf_in and you just adjust the value.

Quoted 41 lines
Quoted 11 lines
Quoted 8 lines

them as separate controls but I am happy to put them into an array if that is the idiomatic approach to situations like this. That said, I am having a difficult time getting dt_binding_check to pass when I have an array of uint64.

When listing two items, as in your example below, I get the following:
adi,admv8818.example.dtb: admv8818@0: adi,filter-margins-hz: [[0, 30000000], [0, 30000000]] is too long

Tricky to say without seeing your code. Magic crystal ball had malfunction today.

This is the property:

adi,filter-margins-hz:
items:
- description: |
The minimum distance, in Hz, between rf_in and the low-pass corner frequency when the device is used in "auto" mode. If the sum of rf_in and this value is greater than 18.85 GHz then the low-pass filter will be put into bypass mode, otherwise the closest corner frequency that is greater than or equal to the sum of rf_in plus this value will be used.
minimum: 0
maximum: 0xFFFFFFFFFFFFFFFF
default: 0
- description: |
The minimum distance, in Hz, between rf_in and the high-pass corner frequency when the device is used in "auto" mode. If the difference between rf_in and this value is less than 1.75 GHz then the high-pass filter will be put into bypass mode, otherwise the closest corner frequency that is less than or equal to the difference of rf_in and this value will be used.
minimum: 0
maximum: 0xFFFFFFFFFFFFFFFF
default: 0

And this is the example:

examples:
- |

Quoted 10 lines
    spi {
      #address-cells = <1>;
      #size-cells = <0>;
      admv8818@0 {
        compatible = "adi,admv8818";
        reg = <0>;
        spi-max-frequency = <10000000>;
        clocks = <&admv8818_rfin>;
        clock-names = "rf_in";
        adi,filter-margins-hz = /bits/ 64 <30000000 30000000>;

foo-hz is in 32-bit, so basically you have here 4 32-bit numbers which indeed reported by dtschema - property is too long. Drop 64-bit here.

Device allows multiple LPF/HPF values to be stored in LUT tables and it actually has four independent filters. Shouldn't these be included here? Maybe not LUT tables, but the configuration for all filters?

Best regards,
Krzysztof

author
#7 lore raw

Re: [PATCH v4 2/2] dt-bindings: iio: filter: Add lpf/hpf freq margins

Quoted 32 lines · Krzysztof Kozlowski
Quoted 31 lines · Sam Winchenbach
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines

Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml properties: · line 56

Quoted 4 lines
Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml +4 -0
+  adi,hpf-margin-hz:
+    description:
+      Sets maximum high-pass corner frequency to the frequency of rf_in minus
+      this value when in auto mode.

IIUC, these are two bounds - lower and upper - in relation to something else (like rf_in frequency)? If so, make it an array (naming to be discuss, I assume you know better what's that):

Quoted 7 lines
Quoted 2 lines

It is true that these are both related to rf_in but both the low and high pass filters can operate independently. Logically, IMO, it makes more sense to have

You mean you can set only low or high pass and keep other as default? But what is the default then - something from reset value or "0" means disabled?

This value isn't setting the corner frequency of the filter, but the minimum distance the corner must be from the fundamental frequency. So, for example, if rf_in is 3.35 GHz and you set lpf-margin-hz to 0 then the corner frequency will be set to 3.35 GHz because that is an exact value supported by the device.

If lpf-margin-hz is set to 30 MHz (for example), then corner frequency would be at least 3.35 GHz + 30 MHz = 3.38 GHz. 3.49 GHz is the closest corner frequency without going below 3.38 GHz that is supported by the device, so that is what will be selected.

This prevents the situation where your fundamental frequency falls on, or close to, a corner frequency which could result in 3dB (half power) loss in your signal.

Quoted 8 lines
Quoted 1 line

This is all completely indepent of the high-pass filter.

Description is confusing a bit, because it suggests the value sets the corner frequency. It explicitly says this - "sets ... corner frequency" and such meaning for properties we usually associate with the property doing this. Here however corner frequency will be always set to rf_in and you just adjust the value.

How about: "Sets the minimum distance (in Hz) between the fundamental frequency of rf_in and the corner frequency of the high-pass, input filter when operatred in 'auto' mode. The selected high-pass corner frequency will be less than, or equal to, rf_in - hpf-margin-hz. If not setting is found that satisfies this relationship the filter will be put into 'bypass'."

Perhaps that is a bit more clear on the intention of this parameter?

Quoted 53 lines
Quoted 41 lines
Quoted 11 lines
Quoted 8 lines

them as separate controls but I am happy to put them into an array if that is the idiomatic approach to situations like this. That said, I am having a difficult time getting dt_binding_check to pass when I have an array of uint64.

When listing two items, as in your example below, I get the following:
adi,admv8818.example.dtb: admv8818@0: adi,filter-margins-hz: [[0, 30000000], [0, 30000000]] is too long

Tricky to say without seeing your code. Magic crystal ball had malfunction today.

This is the property:

adi,filter-margins-hz:
items:
- description: |
The minimum distance, in Hz, between rf_in and the low-pass corner frequency when the device is used in "auto" mode. If the sum of rf_in and this value is greater than 18.85 GHz then the low-pass filter will be put into bypass mode, otherwise the closest corner frequency that is greater than or equal to the sum of rf_in plus this value will be used.
minimum: 0
maximum: 0xFFFFFFFFFFFFFFFF
default: 0
- description: |
The minimum distance, in Hz, between rf_in and the high-pass corner frequency when the device is used in "auto" mode. If the difference between rf_in and this value is less than 1.75 GHz then the high-pass filter will be put into bypass mode, otherwise the closest corner frequency that is less than or equal to the difference of rf_in and this value will be used.
minimum: 0
maximum: 0xFFFFFFFFFFFFFFFF
default: 0

And this is the example:

examples:
- |

Quoted 10 lines
    spi {
      #address-cells = <1>;
      #size-cells = <0>;
      admv8818@0 {
        compatible = "adi,admv8818";
        reg = <0>;
        spi-max-frequency = <10000000>;
        clocks = <&admv8818_rfin>;
        clock-names = "rf_in";
        adi,filter-margins-hz = /bits/ 64 <30000000 30000000>;
Quoted 3 lines

foo-hz is in 32-bit, so basically you have here 4 32-bit numbers which indeed reported by dtschema - property is too long. Drop 64-bit here.

I was hoping to keep this 64 bits seeing this is a 18 GHz+ filter. I suppose I could change this to MHz and just lose a bit of resolution. Does that sound like a better approach?

Quoted 4 lines

Device allows multiple LPF/HPF values to be stored in LUT tables and it actually has four independent filters. Shouldn't these be included here? Maybe not LUT tables, but the configuration for all filters?

There are two filters, the input (high-pass) filter, and the output (low-pass) filter. Each filter has four banks, each with a different range of frequencies. Only one bank can be selected at a time. Each bank has 16 different possible cutoff/corner frequencies. That is a total of 64 distinct values for each of the two filters.

The issue with setting the corner frequency directly is that in certain applications (such as software defined radios) the fundamental frequency is adjustable, necessitating that the corner frequencies of the filter are adjusted accordingly. When the filter is in "auto" mode it is notified via the clock system of frequency changes, so using this information it should be possible to select new corner frequencies if you know the minimum distance between your fundamental frequency and the corner.

It is possible there is either not enough call for this feature, or it goes against the designs of the maintainters. If that is the case we should decline this patch and we will maintain it in our fork of the kernel.

Thanks,
-Sam

Quoted 2 lines

Best regards,
Krzysztof

#8 lore raw

Re: [PATCH v4 2/2] dt-bindings: iio: filter: Add lpf/hpf freq margins

Quoted 13 lines · Sam Winchenbach
Quoted 4 lines
Quoted 4 lines

This prevents the situation where your fundamental frequency falls on, or close to, a corner frequency which could result in 3dB (half power) loss in your signal.

Quoted 8 lines
Quoted 1 line

This is all completely indepent of the high-pass filter.

Description is confusing a bit, because it suggests the value sets the corner frequency. It explicitly says this - "sets ... corner frequency" and such meaning for properties we usually associate with the property doing this. Here however corner frequency will be always set to rf_in and you just adjust the value.

Quoted 7 lines

How about: "Sets the minimum distance (in Hz) between the fundamental frequency of rf_in and the corner frequency of the high-pass, input filter when operatred in 'auto' mode. The selected high-pass corner frequency will be less than, or equal to, rf_in - hpf-margin-hz. If not setting is found that satisfies this relationship the filter will be put into 'bypass'."

Perhaps that is a bit more clear on the intention of this parameter?

Yes

Quoted 54 lines
Quoted 54 lines
Quoted 52 lines
Quoted 7 lines
Quoted 7 lines

them as separate controls but I am happy to put them into an array if that is the idiomatic approach to situations like this. That said, I am having a difficult time getting dt_binding_check to pass when I have an array of uint64.

Quoted 5 lines
Quoted 2 lines

When listing two items, as in your example below, I get the following:
adi,admv8818.example.dtb: admv8818@0: adi,filter-margins-hz: [[0, 30000000], [0, 30000000]] is too long

Tricky to say without seeing your code. Magic crystal ball had malfunction today.

This is the property:

adi,filter-margins-hz:
items:
- description: |
The minimum distance, in Hz, between rf_in and the low-pass corner frequency when the device is used in "auto" mode. If the sum of rf_in and this value is greater than 18.85 GHz then the low-pass filter will be put into bypass mode, otherwise the closest corner frequency that is greater than or equal to the sum of rf_in plus this value will be used.
minimum: 0
maximum: 0xFFFFFFFFFFFFFFFF
default: 0
- description: |
The minimum distance, in Hz, between rf_in and the high-pass corner frequency when the device is used in "auto" mode. If the difference between rf_in and this value is less than 1.75 GHz then the high-pass filter will be put into bypass mode, otherwise the closest corner frequency that is less than or equal to the difference of rf_in and this value will be used.
minimum: 0
maximum: 0xFFFFFFFFFFFFFFFF
default: 0

And this is the example:

examples:
- |

    spi {
      #address-cells = <1>;
      #size-cells = <0>;
      admv8818@0 {
        compatible = "adi,admv8818";
        reg = <0>;
        spi-max-frequency = <10000000>;
        clocks = <&admv8818_rfin>;
        clock-names = "rf_in";
        adi,filter-margins-hz = /bits/ 64 <30000000 30000000>;
Quoted 7 lines
Quoted 3 lines

foo-hz is in 32-bit, so basically you have here 4 32-bit numbers which indeed reported by dtschema - property is too long. Drop 64-bit here.

I was hoping to keep this 64 bits seeing this is a 18 GHz+ filter. I suppose I could change this to MHz and just lose a bit of resolution. Does that sound like a better approach?

Does the hardware accept Hz resolution? How many bits do you have in the registers per each value?

Anyway, the value was 32-bit even in your original patch and your DTS example was not correct.

Quoted 11 lines
Quoted 5 lines

Device allows multiple LPF/HPF values to be stored in LUT tables and it actually has four independent filters. Shouldn't these be included here? Maybe not LUT tables, but the configuration for all filters?

There are two filters, the input (high-pass) filter, and the output (low-pass) filter. Each filter has four banks, each with a different range of frequencies. Only one bank can be selected at a time. Each bank has 16 different possible cutoff/corner frequencies. That is a total of 64 distinct values for each of the two filters.

Hm, datasheet says:
"four independently controlled high-
pass filters (HPFs) and four independently controlled low-pass filters (LPFs)"

so four each, not one each, but I guess they wanted to say banks as only one filter/bank can be active in given time?

Quoted 8 lines

The issue with setting the corner frequency directly is that in certain applications (such as software defined radios) the fundamental frequency is adjustable, necessitating that the corner frequencies of the filter are adjusted accordingly. When the filter is in "auto" mode it is notified via the clock system of frequency changes, so using this information it should be possible to select new corner frequencies if you know the minimum distance between your fundamental frequency and the corner.

I am not advocating to set the corner frequency directly, but just pointing that your current binding seems incomplete.

Best regards,
Krzysztof

author
#9 lore raw

Re: [PATCH v4 2/2] dt-bindings: iio: filter: Add lpf/hpf freq margins

Quoted 20 lines · Krzysztof Kozlowski
Quoted 19 lines · Sam Winchenbach
Quoted 4 lines
Quoted 4 lines

This prevents the situation where your fundamental frequency falls on, or close to, a corner frequency which could result in 3dB (half power) loss in your signal.

Quoted 8 lines
Quoted 1 line

This is all completely indepent of the high-pass filter.

Description is confusing a bit, because it suggests the value sets the corner frequency. It explicitly says this - "sets ... corner frequency" and such meaning for properties we usually associate with the property doing this. Here however corner frequency will be always set to rf_in and you just adjust the value.

How about: "Sets the minimum distance (in Hz) between the fundamental frequency of rf_in and the corner frequency of the high-pass, input filter when operatred in 'auto' mode. The selected high-pass corner frequency will be less than, or equal to, rf_in - hpf-margin-hz. If not setting is found that satisfies this relationship the filter will be put into 'bypass'."

Quoted 3 lines
Quoted 1 line

Perhaps that is a bit more clear on the intention of this parameter?

Yes

I will update with this wording.

Quoted 59 lines
Quoted 59 lines
Quoted 58 lines
Quoted 43 lines
Quoted 8 lines
Quoted 8 lines

them as separate controls but I am happy to put them into an array if that is the idiomatic approach to situations like this. That said, I am having a difficult time getting dt_binding_check to pass when I have an array of uint64.

Quoted 5 lines
Quoted 2 lines

When listing two items, as in your example below, I get the following:
adi,admv8818.example.dtb: admv8818@0: adi,filter-margins-hz: [[0, 30000000], [0, 30000000]] is too long

Tricky to say without seeing your code. Magic crystal ball had malfunction today.

This is the property:

adi,filter-margins-hz:
items:
- description: |
The minimum distance, in Hz, between rf_in and the low-pass corner frequency when the device is used in "auto" mode. If the sum of rf_in and this value is greater than 18.85 GHz then the low-pass filter will be put into bypass mode, otherwise the closest corner frequency that is greater than or equal to the sum of rf_in plus this value will be used.
minimum: 0
maximum: 0xFFFFFFFFFFFFFFFF
default: 0
- description: |
The minimum distance, in Hz, between rf_in and the high-pass corner frequency when the device is used in "auto" mode. If the difference between rf_in and this value is less than 1.75 GHz then the high-pass filter will be put into bypass mode, otherwise the closest corner frequency that is less than or equal to the difference of rf_in and this value will be used.
minimum: 0
maximum: 0xFFFFFFFFFFFFFFFF
default: 0

And this is the example:

examples:
- |

Quoted 10 lines
    spi {
      #address-cells = <1>;
      #size-cells = <0>;
      admv8818@0 {
        compatible = "adi,admv8818";
        reg = <0>;
        spi-max-frequency = <10000000>;
        clocks = <&admv8818_rfin>;
        clock-names = "rf_in";
        adi,filter-margins-hz = /bits/ 64 <30000000 30000000>;

foo-hz is in 32-bit, so basically you have here 4 32-bit numbers which indeed reported by dtschema - property is too long. Drop 64-bit here.

Quoted 7 lines
Quoted 3 lines

I was hoping to keep this 64 bits seeing this is a 18 GHz+ filter. I suppose I could change this to MHz and just lose a bit of resolution. Does that sound like a better approach?

Does the hardware accept Hz resolution? How many bits do you have in the registers per each value?

This isn't necessarily abour the hardware accepts, more about the tolerance of the application. A user could, for example, require that the corner frequency is at least 500 kHz from the fundamental, or the user could require the corner be at least 6 GHz from the corner. A lot of it depends on the bandwidth of the design. Changing this to MHz would be likely be fine. The average user would likely specify 10's if not 100's of MHz in this field.

Quoted 3 lines

Anyway, the value was 32-bit even in your original patch and your DTS example was not correct.

Was it incorrect because I chose a value less than 2^32? I believe this expands to 64 bits:
+ adi,lpf-margin-hz = /bits/ 64 <30000000>;
+ adi,hpf-margin-hz = /bits/ 64 <30000000>;

Is it more appropriate to specify it like this?

        adi,lpf-margin-hz = <0 0x1c9c380>;
Quoted 13 lines
Quoted 7 lines
Quoted 6 lines

Device allows multiple LPF/HPF values to be stored in LUT tables and it actually has four independent filters. Shouldn't these be included here? Maybe not LUT tables, but the configuration for all filters?

Quoted 5 lines

There are two filters, the input (high-pass) filter, and the output (low-pass) filter. Each filter has four banks, each with a different range of frequencies. Only one bank can be selected at a time. Each bank has 16 different possible cutoff/corner frequencies. That is a total of 64 distinct values for each of the two filters.

Quoted 8 lines

Hm, datasheet says:
"four independently controlled high-
pass filters (HPFs) and four independently controlled low-pass filters (LPFs)"

so four each, not one each, but I guess they wanted to say banks as only one filter/bank can be active in given time?

Correct. On the first page you can see that each of the 4 filters is selected by a multiplexer/demultiplexer - the 5 options are: bypass (pass the signal) band 1, band 2, band 3, band 4. You can see this on p. 30, register 0x20.

Register 0x21 selects which of corner is used on the selected band.

Quoted 9 lines
Quoted 8 lines

The issue with setting the corner frequency directly is that in certain applications (such as software defined radios) the fundamental frequency is adjustable, necessitating that the corner frequencies of the filter are adjusted accordingly. When the filter is in "auto" mode it is notified via the clock system of frequency changes, so using this information it should be possible to select new corner frequencies if you know the minimum distance between your fundamental frequency and the corner.

Quoted 3 lines

I am not advocating to set the corner frequency directly, but just pointing that your current binding seems incomplete.

I see. Let's see if we can come to an agreement on this then. I think this would be a useful feature for others.

Thanks,
-Sam

Quoted 3 lines

Best regards,
Krzysztof

build bot
#10 lore raw
In reply to Sam Winchenbach
kernel test robot noticed the following build errors:

Hi Sam,

kernel test robot noticed the following build errors:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linus/master v6.14-rc4 next-20250227] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sam-Winchenbach/dt-bindings-iio-filter-Add-lpf-hpf-freq-margins/20250225-215003
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20250225134612.577022-1-sam.winchenbach%40framepointer.org
patch subject: [PATCH v4 1/2] iio: filter: admv8818: fix range calculation
config: um-allmodconfig (https://download.01.org/0day-ci/archive/20250228/[email protected]/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project 204dcafec0ecf0db81d420d2de57b02ada6b09ec)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250228/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

In file included from drivers/iio/filter/admv8818.c:17:
In file included from include/linux/regmap.h:20:
In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:14:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:549:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]

     549 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:567:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     567 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/iio/filter/admv8818.c:17:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:14:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:585:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/iio/filter/admv8818.c:17:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:14:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:601:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     601 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:616:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     616 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:631:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     631 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:724:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     724 |         readsb(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:737:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     737 |         readsw(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:750:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     750 |         readsl(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:764:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     764 |         writesb(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:778:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     778 |         writesw(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:792:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     792 |         writesl(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
>> drivers/iio/filter/admv8818.c:304:34: error: use of undeclared identifier 'lfp_corner_target'; did you mean 'lpf_corner_target'?
     304 |         ret = __admv8818_lpf_select(st, lfp_corner_target);
         |                                         ^~~~~~~~~~~~~~~~~
         |                                         lpf_corner_target
   drivers/iio/filter/admv8818.c:283:25: note: 'lpf_corner_target' declared here
     283 |         u64 hpf_corner_target, lpf_corner_target;
         |                                ^

12 warnings and 1 error generated.

vim +304 drivers/iio/filter/admv8818.c

279
280 static int admv8818_rfin_band_select(struct admv8818_state *st)
281 {
282 int ret;
283 u64 hpf_corner_target, lpf_corner_target;
284
285 st->cf_hz = clk_get_rate(st->clkin);
286
287 // Check for underflow
288 if (st->cf_hz > st->hpf_margin_hz)
289 hpf_corner_target = st->cf_hz - st->hpf_margin_hz;
290 else
291 hpf_corner_target = 0;
292
293 // Check for overflow
294 lpf_corner_target = st->cf_hz + st->lpf_margin_hz;
295 if (lpf_corner_target < st->cf_hz)
296 lpf_corner_target = U64_MAX;
297
298 mutex_lock(&st->lock);
299
300 ret = __admv8818_hpf_select(st, hpf_corner_target);
301 if (ret)
302 goto exit;
303

 > 304		ret = __admv8818_lpf_select(st, lfp_corner_target);

305 exit:
306 mutex_unlock(&st->lock);
307 return ret;
308 }
309

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
author
#11 lore raw
Quoted 4 lines · kernel test robot

Hi Sam,

kernel test robot noticed the following build errors:

--- snip ---

Quoted 8 lines
Quoted 1 line
Quoted 1 line

drivers/iio/filter/admv8818.c:304:34: error: use of undeclared identifier 'lfp_corner_target'; did you mean 'lpf_corner_target'?

     304 |         ret = __admv8818_lpf_select(st, lfp_corner_target);
         |                                         ^~~~~~~~~~~~~~~~~
         |                                         lpf_corner_target
   drivers/iio/filter/admv8818.c:283:25: note: 'lpf_corner_target' declared here
     283 |         u64 hpf_corner_target, lpf_corner_target;
         |                                ^

12 warnings and 1 error generated.

Well this is embarassing. I must have have had a fat-finger accident when inspecting the result of applying the patch to torvalds/master.

I will fix this in v5 once we come to a concensus on the DTS changes.

--- snip ---

Quoted 11 lines
   303
Quoted 1 line

304 ret = __admv8818_lpf_select(st, lfp_corner_target);

305 exit:

   306		mutex_unlock(&st->lock);
   307		return ret;
   308	}
   309
Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
maintainer
#12 lore raw
In reply to Sam Winchenbach

On Tue, 25 Feb 2025 08:46:11 -0500
Sam Winchenbach <[email protected]> wrote:

Hi Sam,

Various comments inline.

Jonathan

Quoted 2 lines

Corrects the upper range of LPF Band 4 from 18.5 GHz to 18.85 GHz per the ADMV8818 datasheet

This feels like a first fix...

Quoted 6 lines

Search for the minimum error while ensuring that the LPF corner frequency is greater than the target, and the HPF corner frequency is lower than the target

This fixes issues where the range calculations were suboptimal.

This feels like a 2nd one. Maybe two patches appropriate.

Quoted 3 lines

Add two new DTS properties to set the margin between the input frequency and the calculated corner frequency

And this feels like a feature. So 3rd patch that we don't necessarily backport. For earlier stages we just use the default values that you have in the binding.

Quoted 3 lines

Below is a generated table of the differences between the old algorithm and the new. This is a sweep from 0 to 20 GHz in 10 MHz steps.

So, these are just the entries where the 3db point changes? All the others are same?

Quoted 212 lines

=== HPF ===
freq = 1750 MHz, 3db: bypass => 1750 MHz freq = 3400 MHz, 3db: 3310 => 3400 MHz freq = 3410 MHz, 3db: 3310 => 3400 MHz freq = 3420 MHz, 3db: 3310 => 3400 MHz freq = 3660 MHz, 3db: 3550 => 3656 MHz freq = 6600 MHz, 3db: 6479 => 6600 MHz freq = 6610 MHz, 3db: 6479 => 6600 MHz freq = 6620 MHz, 3db: 6479 => 6600 MHz freq = 6630 MHz, 3db: 6479 => 6600 MHz freq = 6640 MHz, 3db: 6479 => 6600 MHz freq = 6650 MHz, 3db: 6479 => 6600 MHz freq = 6660 MHz, 3db: 6479 => 6600 MHz freq = 6670 MHz, 3db: 6479 => 6600 MHz freq = 6680 MHz, 3db: 6479 => 6600 MHz freq = 6690 MHz, 3db: 6479 => 6600 MHz freq = 6700 MHz, 3db: 6479 => 6600 MHz freq = 6710 MHz, 3db: 6479 => 6600 MHz freq = 6720 MHz, 3db: 6479 => 6600 MHz freq = 6730 MHz, 3db: 6479 => 6600 MHz freq = 6960 MHz, 3db: 6736 => 6960 MHz freq = 6970 MHz, 3db: 6736 => 6960 MHz freq = 6980 MHz, 3db: 6736 => 6960 MHz freq = 6990 MHz, 3db: 6736 => 6960 MHz freq = 7320 MHz, 3db: 7249 => 7320 MHz freq = 7330 MHz, 3db: 7249 => 7320 MHz freq = 7340 MHz, 3db: 7249 => 7320 MHz freq = 7350 MHz, 3db: 7249 => 7320 MHz freq = 7360 MHz, 3db: 7249 => 7320 MHz freq = 7370 MHz, 3db: 7249 => 7320 MHz freq = 7380 MHz, 3db: 7249 => 7320 MHz freq = 7390 MHz, 3db: 7249 => 7320 MHz freq = 7400 MHz, 3db: 7249 => 7320 MHz freq = 7410 MHz, 3db: 7249 => 7320 MHz freq = 7420 MHz, 3db: 7249 => 7320 MHz freq = 7430 MHz, 3db: 7249 => 7320 MHz freq = 7440 MHz, 3db: 7249 => 7320 MHz freq = 7450 MHz, 3db: 7249 => 7320 MHz freq = 7460 MHz, 3db: 7249 => 7320 MHz freq = 7470 MHz, 3db: 7249 => 7320 MHz freq = 7480 MHz, 3db: 7249 => 7320 MHz freq = 7490 MHz, 3db: 7249 => 7320 MHz freq = 7500 MHz, 3db: 7249 => 7320 MHz freq = 12500 MHz, 3db: 12000 => 12500 MHz

=== LPF ===
freq = 2050 MHz, 3db: bypass => 2050 MHz freq = 2170 MHz, 3db: 2290 => 2170 MHz freq = 2290 MHz, 3db: 2410 => 2290 MHz freq = 2410 MHz, 3db: 2530 => 2410 MHz freq = 2530 MHz, 3db: 2650 => 2530 MHz freq = 2650 MHz, 3db: 2770 => 2650 MHz freq = 2770 MHz, 3db: 2890 => 2770 MHz freq = 2890 MHz, 3db: 3010 => 2890 MHz freq = 3010 MHz, 3db: 3130 => 3010 MHz freq = 3130 MHz, 3db: 3250 => 3130 MHz freq = 3250 MHz, 3db: 3370 => 3250 MHz freq = 3260 MHz, 3db: 3370 => 3350 MHz freq = 3270 MHz, 3db: 3370 => 3350 MHz freq = 3280 MHz, 3db: 3370 => 3350 MHz freq = 3290 MHz, 3db: 3370 => 3350 MHz freq = 3300 MHz, 3db: 3370 => 3350 MHz freq = 3310 MHz, 3db: 3370 => 3350 MHz freq = 3320 MHz, 3db: 3370 => 3350 MHz freq = 3330 MHz, 3db: 3370 => 3350 MHz freq = 3340 MHz, 3db: 3370 => 3350 MHz freq = 3350 MHz, 3db: 3370 => 3350 MHz freq = 3370 MHz, 3db: 3490 => 3370 MHz freq = 3490 MHz, 3db: 3610 => 3490 MHz freq = 3610 MHz, 3db: 3730 => 3610 MHz freq = 3730 MHz, 3db: 3850 => 3730 MHz freq = 3850 MHz, 3db: 3870 => 3850 MHz freq = 3870 MHz, 3db: 4130 => 3870 MHz freq = 4130 MHz, 3db: 4390 => 4130 MHz freq = 4390 MHz, 3db: 4650 => 4390 MHz freq = 4650 MHz, 3db: 4910 => 4650 MHz freq = 4910 MHz, 3db: 5170 => 4910 MHz freq = 5170 MHz, 3db: 5430 => 5170 MHz freq = 5430 MHz, 3db: 5690 => 5430 MHz freq = 5690 MHz, 3db: 5950 => 5690 MHz freq = 5950 MHz, 3db: 6210 => 5950 MHz freq = 6210 MHz, 3db: 6470 => 6210 MHz freq = 6470 MHz, 3db: 6730 => 6470 MHz freq = 6730 MHz, 3db: 6990 => 6730 MHz freq = 6990 MHz, 3db: 7250 => 6990 MHz freq = 7000 MHz, 3db: 7250 => 7000 MHz freq = 7250 MHz, 3db: 7400 => 7250 MHz freq = 7400 MHz, 3db: 7800 => 7400 MHz freq = 7800 MHz, 3db: 8200 => 7800 MHz freq = 8200 MHz, 3db: 8600 => 8200 MHz freq = 8600 MHz, 3db: 9000 => 8600 MHz freq = 9000 MHz, 3db: 9400 => 9000 MHz freq = 9400 MHz, 3db: 9800 => 9400 MHz freq = 9800 MHz, 3db: 10200 => 9800 MHz freq = 10200 MHz, 3db: 10600 => 10200 MHz freq = 10600 MHz, 3db: 11000 => 10600 MHz freq = 11000 MHz, 3db: 11400 => 11000 MHz freq = 11400 MHz, 3db: 11800 => 11400 MHz freq = 11800 MHz, 3db: 12200 => 11800 MHz freq = 12200 MHz, 3db: 12600 => 12200 MHz freq = 12210 MHz, 3db: 12600 => 12550 MHz freq = 12220 MHz, 3db: 12600 => 12550 MHz freq = 12230 MHz, 3db: 12600 => 12550 MHz freq = 12240 MHz, 3db: 12600 => 12550 MHz freq = 12250 MHz, 3db: 12600 => 12550 MHz freq = 12260 MHz, 3db: 12600 => 12550 MHz freq = 12270 MHz, 3db: 12600 => 12550 MHz freq = 12280 MHz, 3db: 12600 => 12550 MHz freq = 12290 MHz, 3db: 12600 => 12550 MHz freq = 12300 MHz, 3db: 12600 => 12550 MHz freq = 12310 MHz, 3db: 12600 => 12550 MHz freq = 12320 MHz, 3db: 12600 => 12550 MHz freq = 12330 MHz, 3db: 12600 => 12550 MHz freq = 12340 MHz, 3db: 12600 => 12550 MHz freq = 12350 MHz, 3db: 12600 => 12550 MHz freq = 12360 MHz, 3db: 12600 => 12550 MHz freq = 12370 MHz, 3db: 12600 => 12550 MHz freq = 12380 MHz, 3db: 12600 => 12550 MHz freq = 12390 MHz, 3db: 12600 => 12550 MHz freq = 12400 MHz, 3db: 12600 => 12550 MHz freq = 12410 MHz, 3db: 12600 => 12550 MHz freq = 12420 MHz, 3db: 12600 => 12550 MHz freq = 12430 MHz, 3db: 12600 => 12550 MHz freq = 12440 MHz, 3db: 12600 => 12550 MHz freq = 12450 MHz, 3db: 12600 => 12550 MHz freq = 12460 MHz, 3db: 12600 => 12550 MHz freq = 12470 MHz, 3db: 12600 => 12550 MHz freq = 12480 MHz, 3db: 12600 => 12550 MHz freq = 12490 MHz, 3db: 12600 => 12550 MHz freq = 12500 MHz, 3db: 12600 => 12550 MHz freq = 12510 MHz, 3db: 12600 => 12550 MHz freq = 12520 MHz, 3db: 12600 => 12550 MHz freq = 12530 MHz, 3db: 12600 => 12550 MHz freq = 12540 MHz, 3db: 12600 => 12550 MHz freq = 12550 MHz, 3db: 12600 => 12550 MHz freq = 12600 MHz, 3db: 13000 => 12600 MHz freq = 12610 MHz, 3db: 13000 => 12970 MHz freq = 12620 MHz, 3db: 13000 => 12970 MHz freq = 12630 MHz, 3db: 13000 => 12970 MHz freq = 12640 MHz, 3db: 13000 => 12970 MHz freq = 12650 MHz, 3db: 13000 => 12970 MHz freq = 12660 MHz, 3db: 13000 => 12970 MHz freq = 12670 MHz, 3db: 13000 => 12970 MHz freq = 12680 MHz, 3db: 13000 => 12970 MHz freq = 12690 MHz, 3db: 13000 => 12970 MHz freq = 12700 MHz, 3db: 13000 => 12970 MHz freq = 12710 MHz, 3db: 13000 => 12970 MHz freq = 12720 MHz, 3db: 13000 => 12970 MHz freq = 12730 MHz, 3db: 13000 => 12970 MHz freq = 12740 MHz, 3db: 13000 => 12970 MHz freq = 12750 MHz, 3db: 13000 => 12970 MHz freq = 12760 MHz, 3db: 13000 => 12970 MHz freq = 12770 MHz, 3db: 13000 => 12970 MHz freq = 12780 MHz, 3db: 13000 => 12970 MHz freq = 12790 MHz, 3db: 13000 => 12970 MHz freq = 12800 MHz, 3db: 13000 => 12970 MHz freq = 12810 MHz, 3db: 13000 => 12970 MHz freq = 12820 MHz, 3db: 13000 => 12970 MHz freq = 12830 MHz, 3db: 13000 => 12970 MHz freq = 12840 MHz, 3db: 13000 => 12970 MHz freq = 12850 MHz, 3db: 13000 => 12970 MHz freq = 12860 MHz, 3db: 13000 => 12970 MHz freq = 12870 MHz, 3db: 13000 => 12970 MHz freq = 12880 MHz, 3db: 13000 => 12970 MHz freq = 12890 MHz, 3db: 13000 => 12970 MHz freq = 12900 MHz, 3db: 13000 => 12970 MHz freq = 12910 MHz, 3db: 13000 => 12970 MHz freq = 12920 MHz, 3db: 13000 => 12970 MHz freq = 12930 MHz, 3db: 13000 => 12970 MHz freq = 12940 MHz, 3db: 13000 => 12970 MHz freq = 12950 MHz, 3db: 13000 => 12970 MHz freq = 12960 MHz, 3db: 13000 => 12970 MHz freq = 12970 MHz, 3db: 13000 => 12970 MHz freq = 13000 MHz, 3db: 13390 => 13000 MHz freq = 13390 MHz, 3db: 13810 => 13390 MHz freq = 13810 MHz, 3db: 14230 => 13810 MHz freq = 14230 MHz, 3db: 14650 => 14230 MHz freq = 14650 MHz, 3db: 15070 => 14650 MHz freq = 15070 MHz, 3db: 15490 => 15070 MHz freq = 15490 MHz, 3db: 15910 => 15490 MHz freq = 15910 MHz, 3db: 16330 => 15910 MHz freq = 16330 MHz, 3db: 16750 => 16330 MHz freq = 16750 MHz, 3db: 17170 => 16750 MHz freq = 17170 MHz, 3db: 17590 => 17170 MHz freq = 17590 MHz, 3db: 18010 => 17590 MHz freq = 18010 MHz, 3db: 18430 => 18010 MHz freq = 18430 MHz, 3db: 18850 => 18430 MHz freq = 18850 MHz, 3db: bypass => 18850 MHz

Fixes f34fe888ad05 ("iio:filter:admv8818: add support for ADMV8818")
Signed-off-by Sam Winchenbach <[email protected]>

---
V1 -> V2: Cleaned up the wording of the commit message V2 -> V3: Add DTS properties to control corner frequency margins

---
 drivers/iio/filter/admv8818.c | 136 ++++++++++++++++++++++++++--------
 1 file changed, 105 insertions(+), 31 deletions(-)
drivers/iio/filter/admv8818.c +2 -0
index 848baa6e3bbf..a446d8d421ae 100644
@@ -90,6 +90,8 @@ struct admv8818_state {
 	struct mutex		lock;
 	unsigned int		filter_mode;
 	u64			cf_hz;
+	u64			lpf_margin_hz;
+	u64			hpf_margin_hz;
 };
 
 static const unsigned long long freq_range_hpf[4][2] = {

drivers/iio/filter/admv8818.c static const unsigned long long freq_range_lpf[4][2] = { · line 108

Quoted 5 lines
drivers/iio/filter/admv8818.c +1 -1
@@ -103,7 +105,7 @@ static const unsigned long long freq_range_lpf[4][2] = {
 	{2050000000ULL, 3850000000ULL},
 	{3350000000ULL, 7250000000ULL},
 	{7000000000, 13000000000},
-	{12550000000, 18500000000}
+	{12550000000, 18850000000}

As above. This seems to be the first fix and should stand on it's own.

Quoted 24 lines
drivers/iio/filter/admv8818.c +4 -1
 };
 
 static const struct regmap_config admv8818_regmap_config = {

@@ -122,43 +124,59 @@ static const char * const admv8818_modes[] = {
 static int __admv8818_hpf_select(struct admv8818_state *st, u64 freq)
 {
 	unsigned int hpf_step = 0, hpf_band = 0, i, j;
+	u64 freq_error;
+	u64 min_freq_error;
+	u64 freq_corner;
 	u64 freq_step;
 	int ret;
 
 	if (freq < freq_range_hpf[0][0])
 		goto hpf_write;
 
-	if (freq > freq_range_hpf[3][1]) {
+	if (freq >= freq_range_hpf[3][1]) {
 		hpf_step = 15;
 		hpf_band = 4;
 
 		goto hpf_write;
 	}

drivers/iio/filter/admv8818.c static const char * const admv8818_modes[] = { · line 152

Quoted 10 lines
drivers/iio/filter/admv8818.c +9 -0
+	/* Close HPF frequency gap between 12 and 12.5 GHz */
+	if (freq >= 12000 * HZ_PER_MHZ && freq < 12500 * HZ_PER_MHZ) {
+		hpf_step = 15;
+		hpf_band = 3;
+
+		goto hpf_write;
+	}
+
+	min_freq_error = U64_MAX;
 	for (i = 0; i < 4; i++) {

Can we get that 4 from an array size rather than hard coding here?

drivers/iio/filter/admv8818.c static const char * const admv8818_modes[] = { · line 153

Quoted 1 line
drivers/iio/filter/admv8818.c +1 -0
+		/* This (and therefore all other ranges) have a corner

Multiline comment in IIO (and most of kernel for that matter) is /*
* This...

drivers/iio/filter/admv8818.c static const char * const admv8818_modes[] = { · line 162

Quoted 12 lines
drivers/iio/filter/admv8818.c +6 -3
+		 * frequency higher than the target frequency.
+		 */
+		if (freq_range_hpf[i][0] > freq)
+			break;
+
 		freq_step = div_u64((freq_range_hpf[i][1] -
 			freq_range_hpf[i][0]), 15);
 
-		if (freq > freq_range_hpf[i][0] &&
-		    (freq < freq_range_hpf[i][1] + freq_step)) {
-			hpf_band = i + 1;
+		for (j = 0; j <= 15; j++) {

Similarly, where does the 15 come from? It's kind of in the old code but given you are changing this good to make that clearer in some fashion.

drivers/iio/filter/admv8818.c static const char * const admv8818_modes[] = { · line 163

Quoted 1 line
drivers/iio/filter/admv8818.c +1 -0
+			freq_corner = freq_range_hpf[i][0] + (freq_step * j);

No need for brackets around the two multiplied term.

For all these comments check for other instances. I'm just pointing out one.

Quoted 32 lines
drivers/iio/filter/admv8818.c +11 -12
+
+			/* This (and therefore all other steps) have a corner
+			 * frequency higher than the target frequency.
+			 */
+			if (freq_corner > freq)
+				break;
 
-			for (j = 1; j <= 16; j++) {
-				if (freq < (freq_range_hpf[i][0] + (freq_step * j))) {
-					hpf_step = j - 1;
-					break;
-				}
+			freq_error = freq - freq_corner;
+			if (freq_error < min_freq_error) {
+				min_freq_error = freq_error;
+				hpf_step = j;
+				hpf_band = i + 1;
 			}
-			break;
 		}
 	}
 
-	/* Close HPF frequency gap between 12 and 12.5 GHz */
-	if (freq >= 12000 * HZ_PER_MHZ && freq <= 12500 * HZ_PER_MHZ) {
-		hpf_band = 3;
-		hpf_step = 15;
-	}
-
 hpf_write:
 	ret = regmap_update_bits(st->regmap, ADMV8818_REG_WR0_SW,
 				 ADMV8818_SW_IN_SET_WR0_MSK |

drivers/iio/filter/admv8818.c static int admv8818_hpf_select(struct admv8818_state *st, u64 freq) · line 207

Quoted 5 lines
drivers/iio/filter/admv8818.c +1 -1
@@ -186,7 +204,11 @@ static int admv8818_hpf_select(struct admv8818_state *st, u64 freq)
 
 static int __admv8818_lpf_select(struct admv8818_state *st, u64 freq)
 {
-	unsigned int lpf_step = 0, lpf_band = 0, i, j;
+	int i, j;

Might as well combine with declaration of ret below.

drivers/iio/filter/admv8818.c static int admv8818_hpf_select(struct admv8818_state *st, u64 freq) · line 211

Quoted 4 lines
drivers/iio/filter/admv8818.c +4 -0
+	unsigned int lpf_step = 0, lpf_band = 0;
+	u64 freq_error;
+	u64 min_freq_error;
+	u64 freq_corner;

Good to combine a few of these related u64 as single line declaration..

Quoted 10 lines
drivers/iio/filter/admv8818.c +0 -3
 	u64 freq_step;
 	int ret;
 

@@ -199,18 +221,34 @@ static int __admv8818_lpf_select(struct admv8818_state *st, u64 freq)
 		goto lpf_write;
 	}
 
-	for (i = 0; i < 4; i++) {
-		if (freq > freq_range_lpf[i][0] && freq < freq_range_lpf[i][1]) {
-			lpf_band = i + 1;

drivers/iio/filter/admv8818.c static int __admv8818_lpf_select(struct admv8818_state *st, u64 freq) · line 225

Quoted 10 lines
drivers/iio/filter/admv8818.c +2 -8
-			freq_step = div_u64((freq_range_lpf[i][1] - freq_range_lpf[i][0]), 15);
-
-			for (j = 0; j <= 15; j++) {
-				if (freq < (freq_range_lpf[i][0] + (freq_step * j))) {
-					lpf_step = j;
-					break;
-				}
-			}
+	min_freq_error = U64_MAX;
+	for (i = 3; i >= 0; --i) {

As above. If that 3 comes from an array size, please make that clear.

Quoted 30 lines
drivers/iio/filter/admv8818.c +25 -0
+		/* At this point the highest corner frequency of
+		 * all remaining ranges is below the target.
+		 * LPF corner should be >= the target.
+		 */
+		if (freq > freq_range_lpf[i][1])
 			break;
+
+		freq_step = div_u64((freq_range_lpf[i][1] - freq_range_lpf[i][0]), 15);
+
+		for (j = 15; j >= 0; --j) {
+
+			freq_corner = freq_range_lpf[i][0] + j*freq_step;
+
+			/* At this point all other steps in range will
+			 * place the corner frequency below the target
+			 * LPF corner should >= the target.
+			 */
+			if (freq > freq_corner)
+				break;
+
+			freq_error = freq_corner - freq;
+			if (freq_error < min_freq_error) {
+				min_freq_error = freq_error;
+				lpf_step = j;
+				lpf_band = i + 1;
+			}
 		}
 	}

drivers/iio/filter/admv8818.c static int admv8818_lpf_select(struct admv8818_state *st, u64 freq) · line 287

Quoted 8 lines
drivers/iio/filter/admv8818.c +2 -0
@@ -242,16 +280,28 @@ static int admv8818_lpf_select(struct admv8818_state *st, u64 freq)
 static int admv8818_rfin_band_select(struct admv8818_state *st)
 {
 	int ret;
+	u64 hpf_corner_target, lpf_corner_target;
 
 	st->cf_hz = clk_get_rate(st->clkin);
 
+	// Check for underflow

No C++ style comments in IIO code. This is just a consistency thing rather than really matter. We have lots of code that predates those being at all acceptable in the kernel and a mixture of the two styles is messy!

Quoted 26 lines
drivers/iio/filter/admv8818.c +12 -2
+	if (st->cf_hz > st->hpf_margin_hz)
+		hpf_corner_target = st->cf_hz - st->hpf_margin_hz;
+	else
+		hpf_corner_target = 0;
+
+	// Check for overflow
+	lpf_corner_target = st->cf_hz + st->lpf_margin_hz;
+	if (lpf_corner_target < st->cf_hz)
+		lpf_corner_target = U64_MAX;
+
 	mutex_lock(&st->lock);
 
-	ret = __admv8818_hpf_select(st, st->cf_hz);
+	ret = __admv8818_hpf_select(st, hpf_corner_target);
 	if (ret)
 		goto exit;
 
-	ret = __admv8818_lpf_select(st, st->cf_hz);
+	ret = __admv8818_lpf_select(st, lfp_corner_target);
 exit:
 	mutex_unlock(&st->lock);
 	return ret;

@@ -647,6 +697,26 @@ static int admv8818_clk_setup(struct admv8818_state *st)
 	return devm_add_action_or_reset(&spi->dev, admv8818_clk_notifier_unreg, st);
 }

drivers/iio/filter/admv8818.c static int admv8818_clk_setup(struct admv8818_state *st) · line 709

Quoted 10 lines
drivers/iio/filter/admv8818.c +10 -0
+static int admv8818_read_properties(struct admv8818_state *st)
+{
+	struct spi_device *spi = st->spi;
+	int ret;
+
+	ret = device_property_read_u64(&spi->dev, "adi,lpf-margin-hz", &st->lpf_margin_hz);
+	if (ret == -EINVAL)
+		st->lpf_margin_hz = 0;
+	else if (ret < 0)
+		return ret;

Often for properties with defaults we don't worry too much about checking for errors other than 'not there'. So I'd be fine with this being the simpler.

	st->lpf_margin_hz = 0;
	device_property_read_u64(...)

and no explicit error checking.

If you really want to retain the protection against wrong formats etc, then fair enough.

Quoted 14 lines
drivers/iio/filter/admv8818.c +10 -0
+
+	ret = device_property_read_u64(&spi->dev, "adi,hpf-margin-hz", &st->hpf_margin_hz);
+	if (ret == -EINVAL)
+		st->hpf_margin_hz = 0;
+	else if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static int admv8818_probe(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev;

drivers/iio/filter/admv8818.c static int admv8818_probe(struct spi_device *spi) · line 751

Quoted 4 lines
drivers/iio/filter/admv8818.c +1 -0
@@ -678,6 +748,10 @@ static int admv8818_probe(struct spi_device *spi)
 
 	mutex_init(&st->lock);
 
+	ret = admv8818_read_properties(st);

I haven't checked but if this is first place that you use property.h then need that included.

Quoted 6 lines
drivers/iio/filter/admv8818.c +3 -0
+	if (ret)
+		return ret;
+
 	ret = admv8818_init(st);
 	if (ret)
 		return ret;
author
#13 lore raw
Quoted 5 lines · Jonathan Cameron

On Tue, 25 Feb 2025 08:46:11 -0500
Sam Winchenbach <[email protected]> wrote:

Hi Sam,

Quoted 8 lines

Various comments inline.

Jonathan

Quoted 2 lines

Corrects the upper range of LPF Band 4 from 18.5 GHz to 18.85 GHz per the ADMV8818 datasheet

This feels like a first fix...

Agreed. This should be broken out. For discussion let's call this PATCH_1.

Quoted 8 lines
Quoted 6 lines

Search for the minimum error while ensuring that the LPF corner frequency is greater than the target, and the HPF corner frequency is lower than the target

This fixes issues where the range calculations were suboptimal.

This feels like a 2nd one. Maybe two patches appropriate.

Agreed. For discussion let's call this PATCH_2.

Quoted 7 lines
Quoted 3 lines

Add two new DTS properties to set the margin between the input frequency and the calculated corner frequency

And this feels like a feature. So 3rd patch that we don't necessarily backport. For earlier stages we just use the default values that you have in the binding.

Hmm. This is interesting. I propose that PATCH_1 is a fix, and both PATCH_2 and this DTS change are treated as a feature. The reason I am suggesting this is that PATCH_2 changes the behavior of the corner frequency and if we backport that then some users may find that their devices no longer function as they used to. Another way of saying this is that PATCH_2 really should include the DTS changes for those users that depended on the old corner calculation algorithm. Does this sound reasonable?

Quoted 7 lines
Quoted 3 lines

Below is a generated table of the differences between the old algorithm and the new. This is a sweep from 0 to 20 GHz in 10 MHz steps.

So, these are just the entries where the 3db point changes? All the others are same?

With a 10 MHz resolution, yes. It was an attempt to show that if the user selected a corner frequency that in many cases there was a better option. The code, as it exists, uses the same algortihm for finding the corner frequency when in either manual or auto mode - There are two problems with this approach:
1. If you are using manual mode, you can't select a specific corner frequency without subtracting 1 from your target frequency. This highlights problem number 2.
2. If you are in automatic mode and your fundamental frequency is 1 Hz below a corner - that corner will be selected. This will efectively put the corner frequency/3db point at the fundamental frequency. This will cut your signal power in half.

Perhaps there is a better way to show this? Conveying this without images is challenging.

Here is an example of where both algorithms agree (1 Hz resolution):
freq = 1750000001 Hz, 3db: 1750000000 (old algorithm) => 1750000000 (new algorithm) Hz

Note that if the user is in auto mode then the corner frequency will be put almost exactly on their fundamental frequency. The same will happen with the new algorithm, but the user has the ability to select a minimum margin using DTS.

Quoted 210 lines
Quoted 210 lines

=== HPF ===
freq = 1750 MHz, 3db: bypass => 1750 MHz freq = 3400 MHz, 3db: 3310 => 3400 MHz freq = 3410 MHz, 3db: 3310 => 3400 MHz freq = 3420 MHz, 3db: 3310 => 3400 MHz freq = 3660 MHz, 3db: 3550 => 3656 MHz freq = 6600 MHz, 3db: 6479 => 6600 MHz freq = 6610 MHz, 3db: 6479 => 6600 MHz freq = 6620 MHz, 3db: 6479 => 6600 MHz freq = 6630 MHz, 3db: 6479 => 6600 MHz freq = 6640 MHz, 3db: 6479 => 6600 MHz freq = 6650 MHz, 3db: 6479 => 6600 MHz freq = 6660 MHz, 3db: 6479 => 6600 MHz freq = 6670 MHz, 3db: 6479 => 6600 MHz freq = 6680 MHz, 3db: 6479 => 6600 MHz freq = 6690 MHz, 3db: 6479 => 6600 MHz freq = 6700 MHz, 3db: 6479 => 6600 MHz freq = 6710 MHz, 3db: 6479 => 6600 MHz freq = 6720 MHz, 3db: 6479 => 6600 MHz freq = 6730 MHz, 3db: 6479 => 6600 MHz freq = 6960 MHz, 3db: 6736 => 6960 MHz freq = 6970 MHz, 3db: 6736 => 6960 MHz freq = 6980 MHz, 3db: 6736 => 6960 MHz freq = 6990 MHz, 3db: 6736 => 6960 MHz freq = 7320 MHz, 3db: 7249 => 7320 MHz freq = 7330 MHz, 3db: 7249 => 7320 MHz freq = 7340 MHz, 3db: 7249 => 7320 MHz freq = 7350 MHz, 3db: 7249 => 7320 MHz freq = 7360 MHz, 3db: 7249 => 7320 MHz freq = 7370 MHz, 3db: 7249 => 7320 MHz freq = 7380 MHz, 3db: 7249 => 7320 MHz freq = 7390 MHz, 3db: 7249 => 7320 MHz freq = 7400 MHz, 3db: 7249 => 7320 MHz freq = 7410 MHz, 3db: 7249 => 7320 MHz freq = 7420 MHz, 3db: 7249 => 7320 MHz freq = 7430 MHz, 3db: 7249 => 7320 MHz freq = 7440 MHz, 3db: 7249 => 7320 MHz freq = 7450 MHz, 3db: 7249 => 7320 MHz freq = 7460 MHz, 3db: 7249 => 7320 MHz freq = 7470 MHz, 3db: 7249 => 7320 MHz freq = 7480 MHz, 3db: 7249 => 7320 MHz freq = 7490 MHz, 3db: 7249 => 7320 MHz freq = 7500 MHz, 3db: 7249 => 7320 MHz freq = 12500 MHz, 3db: 12000 => 12500 MHz

=== LPF ===
freq = 2050 MHz, 3db: bypass => 2050 MHz freq = 2170 MHz, 3db: 2290 => 2170 MHz freq = 2290 MHz, 3db: 2410 => 2290 MHz freq = 2410 MHz, 3db: 2530 => 2410 MHz freq = 2530 MHz, 3db: 2650 => 2530 MHz freq = 2650 MHz, 3db: 2770 => 2650 MHz freq = 2770 MHz, 3db: 2890 => 2770 MHz freq = 2890 MHz, 3db: 3010 => 2890 MHz freq = 3010 MHz, 3db: 3130 => 3010 MHz freq = 3130 MHz, 3db: 3250 => 3130 MHz freq = 3250 MHz, 3db: 3370 => 3250 MHz freq = 3260 MHz, 3db: 3370 => 3350 MHz freq = 3270 MHz, 3db: 3370 => 3350 MHz freq = 3280 MHz, 3db: 3370 => 3350 MHz freq = 3290 MHz, 3db: 3370 => 3350 MHz freq = 3300 MHz, 3db: 3370 => 3350 MHz freq = 3310 MHz, 3db: 3370 => 3350 MHz freq = 3320 MHz, 3db: 3370 => 3350 MHz freq = 3330 MHz, 3db: 3370 => 3350 MHz freq = 3340 MHz, 3db: 3370 => 3350 MHz freq = 3350 MHz, 3db: 3370 => 3350 MHz freq = 3370 MHz, 3db: 3490 => 3370 MHz freq = 3490 MHz, 3db: 3610 => 3490 MHz freq = 3610 MHz, 3db: 3730 => 3610 MHz freq = 3730 MHz, 3db: 3850 => 3730 MHz freq = 3850 MHz, 3db: 3870 => 3850 MHz freq = 3870 MHz, 3db: 4130 => 3870 MHz freq = 4130 MHz, 3db: 4390 => 4130 MHz freq = 4390 MHz, 3db: 4650 => 4390 MHz freq = 4650 MHz, 3db: 4910 => 4650 MHz freq = 4910 MHz, 3db: 5170 => 4910 MHz freq = 5170 MHz, 3db: 5430 => 5170 MHz freq = 5430 MHz, 3db: 5690 => 5430 MHz freq = 5690 MHz, 3db: 5950 => 5690 MHz freq = 5950 MHz, 3db: 6210 => 5950 MHz freq = 6210 MHz, 3db: 6470 => 6210 MHz freq = 6470 MHz, 3db: 6730 => 6470 MHz freq = 6730 MHz, 3db: 6990 => 6730 MHz freq = 6990 MHz, 3db: 7250 => 6990 MHz freq = 7000 MHz, 3db: 7250 => 7000 MHz freq = 7250 MHz, 3db: 7400 => 7250 MHz freq = 7400 MHz, 3db: 7800 => 7400 MHz freq = 7800 MHz, 3db: 8200 => 7800 MHz freq = 8200 MHz, 3db: 8600 => 8200 MHz freq = 8600 MHz, 3db: 9000 => 8600 MHz freq = 9000 MHz, 3db: 9400 => 9000 MHz freq = 9400 MHz, 3db: 9800 => 9400 MHz freq = 9800 MHz, 3db: 10200 => 9800 MHz freq = 10200 MHz, 3db: 10600 => 10200 MHz freq = 10600 MHz, 3db: 11000 => 10600 MHz freq = 11000 MHz, 3db: 11400 => 11000 MHz freq = 11400 MHz, 3db: 11800 => 11400 MHz freq = 11800 MHz, 3db: 12200 => 11800 MHz freq = 12200 MHz, 3db: 12600 => 12200 MHz freq = 12210 MHz, 3db: 12600 => 12550 MHz freq = 12220 MHz, 3db: 12600 => 12550 MHz freq = 12230 MHz, 3db: 12600 => 12550 MHz freq = 12240 MHz, 3db: 12600 => 12550 MHz freq = 12250 MHz, 3db: 12600 => 12550 MHz freq = 12260 MHz, 3db: 12600 => 12550 MHz freq = 12270 MHz, 3db: 12600 => 12550 MHz freq = 12280 MHz, 3db: 12600 => 12550 MHz freq = 12290 MHz, 3db: 12600 => 12550 MHz freq = 12300 MHz, 3db: 12600 => 12550 MHz freq = 12310 MHz, 3db: 12600 => 12550 MHz freq = 12320 MHz, 3db: 12600 => 12550 MHz freq = 12330 MHz, 3db: 12600 => 12550 MHz freq = 12340 MHz, 3db: 12600 => 12550 MHz freq = 12350 MHz, 3db: 12600 => 12550 MHz freq = 12360 MHz, 3db: 12600 => 12550 MHz freq = 12370 MHz, 3db: 12600 => 12550 MHz freq = 12380 MHz, 3db: 12600 => 12550 MHz freq = 12390 MHz, 3db: 12600 => 12550 MHz freq = 12400 MHz, 3db: 12600 => 12550 MHz freq = 12410 MHz, 3db: 12600 => 12550 MHz freq = 12420 MHz, 3db: 12600 => 12550 MHz freq = 12430 MHz, 3db: 12600 => 12550 MHz freq = 12440 MHz, 3db: 12600 => 12550 MHz freq = 12450 MHz, 3db: 12600 => 12550 MHz freq = 12460 MHz, 3db: 12600 => 12550 MHz freq = 12470 MHz, 3db: 12600 => 12550 MHz freq = 12480 MHz, 3db: 12600 => 12550 MHz freq = 12490 MHz, 3db: 12600 => 12550 MHz freq = 12500 MHz, 3db: 12600 => 12550 MHz freq = 12510 MHz, 3db: 12600 => 12550 MHz freq = 12520 MHz, 3db: 12600 => 12550 MHz freq = 12530 MHz, 3db: 12600 => 12550 MHz freq = 12540 MHz, 3db: 12600 => 12550 MHz freq = 12550 MHz, 3db: 12600 => 12550 MHz freq = 12600 MHz, 3db: 13000 => 12600 MHz freq = 12610 MHz, 3db: 13000 => 12970 MHz freq = 12620 MHz, 3db: 13000 => 12970 MHz freq = 12630 MHz, 3db: 13000 => 12970 MHz freq = 12640 MHz, 3db: 13000 => 12970 MHz freq = 12650 MHz, 3db: 13000 => 12970 MHz freq = 12660 MHz, 3db: 13000 => 12970 MHz freq = 12670 MHz, 3db: 13000 => 12970 MHz freq = 12680 MHz, 3db: 13000 => 12970 MHz freq = 12690 MHz, 3db: 13000 => 12970 MHz freq = 12700 MHz, 3db: 13000 => 12970 MHz freq = 12710 MHz, 3db: 13000 => 12970 MHz freq = 12720 MHz, 3db: 13000 => 12970 MHz freq = 12730 MHz, 3db: 13000 => 12970 MHz freq = 12740 MHz, 3db: 13000 => 12970 MHz freq = 12750 MHz, 3db: 13000 => 12970 MHz freq = 12760 MHz, 3db: 13000 => 12970 MHz freq = 12770 MHz, 3db: 13000 => 12970 MHz freq = 12780 MHz, 3db: 13000 => 12970 MHz freq = 12790 MHz, 3db: 13000 => 12970 MHz freq = 12800 MHz, 3db: 13000 => 12970 MHz freq = 12810 MHz, 3db: 13000 => 12970 MHz freq = 12820 MHz, 3db: 13000 => 12970 MHz freq = 12830 MHz, 3db: 13000 => 12970 MHz freq = 12840 MHz, 3db: 13000 => 12970 MHz freq = 12850 MHz, 3db: 13000 => 12970 MHz freq = 12860 MHz, 3db: 13000 => 12970 MHz freq = 12870 MHz, 3db: 13000 => 12970 MHz freq = 12880 MHz, 3db: 13000 => 12970 MHz freq = 12890 MHz, 3db: 13000 => 12970 MHz freq = 12900 MHz, 3db: 13000 => 12970 MHz freq = 12910 MHz, 3db: 13000 => 12970 MHz freq = 12920 MHz, 3db: 13000 => 12970 MHz freq = 12930 MHz, 3db: 13000 => 12970 MHz freq = 12940 MHz, 3db: 13000 => 12970 MHz freq = 12950 MHz, 3db: 13000 => 12970 MHz freq = 12960 MHz, 3db: 13000 => 12970 MHz freq = 12970 MHz, 3db: 13000 => 12970 MHz freq = 13000 MHz, 3db: 13390 => 13000 MHz freq = 13390 MHz, 3db: 13810 => 13390 MHz freq = 13810 MHz, 3db: 14230 => 13810 MHz freq = 14230 MHz, 3db: 14650 => 14230 MHz freq = 14650 MHz, 3db: 15070 => 14650 MHz freq = 15070 MHz, 3db: 15490 => 15070 MHz freq = 15490 MHz, 3db: 15910 => 15490 MHz freq = 15910 MHz, 3db: 16330 => 15910 MHz freq = 16330 MHz, 3db: 16750 => 16330 MHz freq = 16750 MHz, 3db: 17170 => 16750 MHz freq = 17170 MHz, 3db: 17590 => 17170 MHz freq = 17590 MHz, 3db: 18010 => 17590 MHz freq = 18010 MHz, 3db: 18430 => 18010 MHz freq = 18430 MHz, 3db: 18850 => 18430 MHz freq = 18850 MHz, 3db: bypass => 18850 MHz

Fixes f34fe888ad05 ("iio:filter:admv8818: add support for ADMV8818")
Signed-off-by Sam Winchenbach <[email protected]>

---
V1 -> V2: Cleaned up the wording of the commit message V2 -> V3: Add DTS properties to control corner frequency margins

---
 drivers/iio/filter/admv8818.c | 136 ++++++++++++++++++++++++++--------
 1 file changed, 105 insertions(+), 31 deletions(-)
drivers/iio/filter/admv8818.c +2 -0
index 848baa6e3bbf..a446d8d421ae 100644
@@ -90,6 +90,8 @@ struct admv8818_state {
 	struct mutex		lock;
 	unsigned int		filter_mode;
 	u64			cf_hz;
+	u64			lpf_margin_hz;
+	u64			hpf_margin_hz;
 };
Quoted 10 lines

drivers/iio/filter/admv8818.c static const unsigned long long freq_range_lpf[4][2] = { · line 108

Quoted 7 lines
drivers/iio/filter/admv8818.c +1 -1
 static const unsigned long long freq_range_hpf[4][2] = {

@@ -103,7 +105,7 @@ static const unsigned long long freq_range_lpf[4][2] = {
 	{2050000000ULL, 3850000000ULL},
 	{3350000000ULL, 7250000000ULL},
 	{7000000000, 13000000000},
-	{12550000000, 18500000000}
+	{12550000000, 18850000000}

As above. This seems to be the first fix and should stand on it's own.

Good point, I will include this change in v5.

Quoted 24 lines
Quoted 24 lines
drivers/iio/filter/admv8818.c +4 -1
 };
 
 static const struct regmap_config admv8818_regmap_config = {

@@ -122,43 +124,59 @@ static const char * const admv8818_modes[] = {
 static int __admv8818_hpf_select(struct admv8818_state *st, u64 freq)
 {
 	unsigned int hpf_step = 0, hpf_band = 0, i, j;
+	u64 freq_error;
+	u64 min_freq_error;
+	u64 freq_corner;
 	u64 freq_step;
 	int ret;
 
 	if (freq < freq_range_hpf[0][0])
 		goto hpf_write;
 
-	if (freq > freq_range_hpf[3][1]) {
+	if (freq >= freq_range_hpf[3][1]) {
 		hpf_step = 15;
 		hpf_band = 4;
 
 		goto hpf_write;
 	}
Quoted 13 lines

drivers/iio/filter/admv8818.c static const char * const admv8818_modes[] = { · line 152

Quoted 10 lines
drivers/iio/filter/admv8818.c +9 -0
+	/* Close HPF frequency gap between 12 and 12.5 GHz */
+	if (freq >= 12000 * HZ_PER_MHZ && freq < 12500 * HZ_PER_MHZ) {
+		hpf_step = 15;
+		hpf_band = 3;
+
+		goto hpf_write;
+	}
+
+	min_freq_error = U64_MAX;
 	for (i = 0; i < 4; i++) {

Can we get that 4 from an array size rather than hard coding here?

Sure, noted for v5.

Quoted 6 lines

drivers/iio/filter/admv8818.c static const char * const admv8818_modes[] = { · line 153

Quoted 1 line
drivers/iio/filter/admv8818.c +1 -0
+		/* This (and therefore all other ranges) have a corner

Multiline comment in IIO (and most of kernel for that matter) is /*
* This...

Shoot, I figured check_patch would have caught that. Noted for v5

Quoted 8 lines
Quoted 8 lines
drivers/iio/filter/admv8818.c +5 -0
+		 * frequency higher than the target frequency.
+		 */
+		if (freq_range_hpf[i][0] > freq)
+			break;
+
 		freq_step = div_u64((freq_range_hpf[i][1] -
 			freq_range_hpf[i][0]), 15);
Quoted 8 lines

drivers/iio/filter/admv8818.c static const char * const admv8818_modes[] = { · line 162

Quoted 4 lines
drivers/iio/filter/admv8818.c +1 -3
-		if (freq > freq_range_hpf[i][0] &&
-		    (freq < freq_range_hpf[i][1] + freq_step)) {
-			hpf_band = i + 1;
+		for (j = 0; j <= 15; j++) {

Similarly, where does the 15 come from? It's kind of in the old code but given you are changing this good to make that clearer in some fashion.

Each filter band has 16 possible corners (0-15). I will add a define in v5.

Quoted 5 lines

drivers/iio/filter/admv8818.c static const char * const admv8818_modes[] = { · line 163

Quoted 2 lines
drivers/iio/filter/admv8818.c +1 -0
+			freq_corner = freq_range_hpf[i][0] + (freq_step * j);

No need for brackets around the two multiplied term.

For all these comments check for other instances. I'm just pointing out one.

Noted.

Quoted 33 lines
Quoted 33 lines
drivers/iio/filter/admv8818.c +11 -12
+
+			/* This (and therefore all other steps) have a corner
+			 * frequency higher than the target frequency.
+			 */
+			if (freq_corner > freq)
+				break;
 
-			for (j = 1; j <= 16; j++) {
-				if (freq < (freq_range_hpf[i][0] + (freq_step * j))) {
-					hpf_step = j - 1;
-					break;
-				}
+			freq_error = freq - freq_corner;
+			if (freq_error < min_freq_error) {
+				min_freq_error = freq_error;
+				hpf_step = j;
+				hpf_band = i + 1;
 			}
-			break;
 		}
 	}
 
-	/* Close HPF frequency gap between 12 and 12.5 GHz */
-	if (freq >= 12000 * HZ_PER_MHZ && freq <= 12500 * HZ_PER_MHZ) {
-		hpf_band = 3;
-		hpf_step = 15;
-	}
-
 hpf_write:
 	ret = regmap_update_bits(st->regmap, ADMV8818_REG_WR0_SW,
 				 ADMV8818_SW_IN_SET_WR0_MSK |

@@ -186,7 +204,11 @@ static int admv8818_hpf_select(struct admv8818_state *st, u64 freq)
Quoted 7 lines

drivers/iio/filter/admv8818.c static int admv8818_hpf_select(struct admv8818_state *st, u64 freq) · line 207

Quoted 4 lines
drivers/iio/filter/admv8818.c +1 -1
 static int __admv8818_lpf_select(struct admv8818_state *st, u64 freq)
 {
-	unsigned int lpf_step = 0, lpf_band = 0, i, j;
+	int i, j;

Might as well combine with declaration of ret below.

Noted.

Quoted 6 lines

drivers/iio/filter/admv8818.c static int admv8818_hpf_select(struct admv8818_state *st, u64 freq) · line 211

Quoted 4 lines
drivers/iio/filter/admv8818.c +4 -0
+	unsigned int lpf_step = 0, lpf_band = 0;
+	u64 freq_error;
+	u64 min_freq_error;
+	u64 freq_corner;

Good to combine a few of these related u64 as single line declaration..

Noted.

Quoted 7 lines
Quoted 7 lines
drivers/iio/filter/admv8818.c +0 -0
 	u64 freq_step;
 	int ret;
 

@@ -199,18 +221,34 @@ static int __admv8818_lpf_select(struct admv8818_state *st, u64 freq)
 		goto lpf_write;
 	}
Quoted 16 lines

drivers/iio/filter/admv8818.c static int __admv8818_lpf_select(struct admv8818_state *st, u64 freq) · line 225

Quoted 13 lines
drivers/iio/filter/admv8818.c +2 -11
-	for (i = 0; i < 4; i++) {
-		if (freq > freq_range_lpf[i][0] && freq < freq_range_lpf[i][1]) {
-			lpf_band = i + 1;
-			freq_step = div_u64((freq_range_lpf[i][1] - freq_range_lpf[i][0]), 15);
-
-			for (j = 0; j <= 15; j++) {
-				if (freq < (freq_range_lpf[i][0] + (freq_step * j))) {
-					lpf_step = j;
-					break;
-				}
-			}
+	min_freq_error = U64_MAX;
+	for (i = 3; i >= 0; --i) {

As above. If that 3 comes from an array size, please make that clear.

Noted.

Quoted 35 lines
Quoted 35 lines
drivers/iio/filter/admv8818.c +26 -0
+		/* At this point the highest corner frequency of
+		 * all remaining ranges is below the target.
+		 * LPF corner should be >= the target.
+		 */
+		if (freq > freq_range_lpf[i][1])
 			break;
+
+		freq_step = div_u64((freq_range_lpf[i][1] - freq_range_lpf[i][0]), 15);
+
+		for (j = 15; j >= 0; --j) {
+
+			freq_corner = freq_range_lpf[i][0] + j*freq_step;
+
+			/* At this point all other steps in range will
+			 * place the corner frequency below the target
+			 * LPF corner should >= the target.
+			 */
+			if (freq > freq_corner)
+				break;
+
+			freq_error = freq_corner - freq;
+			if (freq_error < min_freq_error) {
+				min_freq_error = freq_error;
+				lpf_step = j;
+				lpf_band = i + 1;
+			}
 		}
 	}
 

@@ -242,16 +280,28 @@ static int admv8818_lpf_select(struct admv8818_state *st, u64 freq)
 static int admv8818_rfin_band_select(struct admv8818_state *st)
 {
 	int ret;
+	u64 hpf_corner_target, lpf_corner_target;
Quoted 8 lines

drivers/iio/filter/admv8818.c static int admv8818_lpf_select(struct admv8818_state *st, u64 freq) · line 287

Quoted 3 lines
drivers/iio/filter/admv8818.c +1 -0
 	st->cf_hz = clk_get_rate(st->clkin);
 
+	// Check for underflow

No C++ style comments in IIO code. This is just a consistency thing rather than really matter. We have lots of code that predates those being at all acceptable in the kernel and a mixture of the two styles is messy!

Bugger. check_patch failed me again :)
Noted. I will go through and address all comments to make sure they fit the style.

Quoted 39 lines
Quoted 26 lines
drivers/iio/filter/admv8818.c +12 -2
+	if (st->cf_hz > st->hpf_margin_hz)
+		hpf_corner_target = st->cf_hz - st->hpf_margin_hz;
+	else
+		hpf_corner_target = 0;
+
+	// Check for overflow
+	lpf_corner_target = st->cf_hz + st->lpf_margin_hz;
+	if (lpf_corner_target < st->cf_hz)
+		lpf_corner_target = U64_MAX;
+
 	mutex_lock(&st->lock);
 
-	ret = __admv8818_hpf_select(st, st->cf_hz);
+	ret = __admv8818_hpf_select(st, hpf_corner_target);
 	if (ret)
 		goto exit;
 
-	ret = __admv8818_lpf_select(st, st->cf_hz);
+	ret = __admv8818_lpf_select(st, lfp_corner_target);
 exit:
 	mutex_unlock(&st->lock);
 	return ret;

@@ -647,6 +697,26 @@ static int admv8818_clk_setup(struct admv8818_state *st)
 	return devm_add_action_or_reset(&spi->dev, admv8818_clk_notifier_unreg, st);
 }

drivers/iio/filter/admv8818.c static int admv8818_clk_setup(struct admv8818_state *st) · line 709

Quoted 10 lines
drivers/iio/filter/admv8818.c +10 -0
+static int admv8818_read_properties(struct admv8818_state *st)
+{
+	struct spi_device *spi = st->spi;
+	int ret;
+
+	ret = device_property_read_u64(&spi->dev, "adi,lpf-margin-hz", &st->lpf_margin_hz);
+	if (ret == -EINVAL)
+		st->lpf_margin_hz = 0;
+	else if (ret < 0)
+		return ret;

Often for properties with defaults we don't worry too much about checking for errors other than 'not there'. So I'd be fine with this being the simpler.

Quoted 6 lines
	st->lpf_margin_hz = 0;
	device_property_read_u64(...)

and no explicit error checking.

If you really want to retain the protection against wrong formats etc, then fair enough.

My only concern that the user will have no feedback that his or her filter settings are not being used which could lead to subtle, hard to track down frequency responses. Would it be more appropriate here to print a warning instead of returning an error?

Quoted 28 lines
Quoted 14 lines
drivers/iio/filter/admv8818.c +10 -0
+
+	ret = device_property_read_u64(&spi->dev, "adi,hpf-margin-hz", &st->hpf_margin_hz);
+	if (ret == -EINVAL)
+		st->hpf_margin_hz = 0;
+	else if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static int admv8818_probe(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev;

drivers/iio/filter/admv8818.c static int admv8818_probe(struct spi_device *spi) · line 751

Quoted 4 lines
drivers/iio/filter/admv8818.c +1 -0
@@ -678,6 +748,10 @@ static int admv8818_probe(struct spi_device *spi)
 
 	mutex_init(&st->lock);
 
+	ret = admv8818_read_properties(st);

I haven't checked but if this is first place that you use property.h then need that included.

Quoted 6 lines
drivers/iio/filter/admv8818.c +3 -0
+	if (ret)
+		return ret;
+
 	ret = admv8818_init(st);
 	if (ret)
 		return ret;
maintainer
#14 lore raw
Quoted 29 lines · On Wed, 5 Mar 2025 07:58:23 -0500 Sam Winchenbach…
Quoted 5 lines · Jonathan Cameron

On Tue, 25 Feb 2025 08:46:11 -0500
Sam Winchenbach <[email protected]> wrote:

Hi Sam,

Quoted 8 lines

Various comments inline.

Jonathan

Quoted 2 lines

Corrects the upper range of LPF Band 4 from 18.5 GHz to 18.85 GHz per the ADMV8818 datasheet

This feels like a first fix...

Agreed. This should be broken out. For discussion let's call this PATCH_1.

Quoted 8 lines
Quoted 6 lines

Search for the minimum error while ensuring that the LPF corner frequency is greater than the target, and the HPF corner frequency is lower than the target

This fixes issues where the range calculations were suboptimal.

This feels like a 2nd one. Maybe two patches appropriate.

Agreed. For discussion let's call this PATCH_2.

Quoted 1 line
Quoted 1 line
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

Add two new DTS properties to set the margin between the input frequency and the calculated corner frequency

And this feels like a feature. So 3rd patch that we don't necessarily backport. For earlier stages we just use the default values that you have in the binding.

Hmm. This is interesting. I propose that PATCH_1 is a fix, and both PATCH_2 and this DTS change are treated as a feature. The reason I am suggesting this is that PATCH_2 changes the behavior of the corner frequency and if we backport that then some users may find that their devices no longer function as they used to. Another way of saying this is that PATCH_2 really should include the DTS changes for those users that depended on the old corner calculation algorithm. Does this sound reasonable?

Sure. I'm fine going with your judgement on what is a fix and what isn't.

Jonathan

Quoted 9 lines
Quoted 8 lines
Quoted 4 lines

Below is a generated table of the differences between the old algorithm and the new. This is a sweep from 0 to 20 GHz in 10 MHz steps.

So, these are just the entries where the 3db point changes? All the others are same?

Quoted 5 lines

With a 10 MHz resolution, yes. It was an attempt to show that if the user selected a corner frequency that in many cases there was a better option. The code, as it exists, uses the same algortihm for finding the corner frequency when in either manual or auto mode - There are two problems with this approach:
1. If you are using manual mode, you can't select a specific corner frequency without subtracting 1 from your target frequency. This highlights problem number 2.
2. If you are in automatic mode and your fundamental frequency is 1 Hz below a corner - that corner will be selected. This will efectively put the corner frequency/3db point at the fundamental frequency. This will cut your signal power in half.

Perhaps there is a better way to show this? Conveying this without images is challenging.

Agreed. Ascii art is always an option but obviously limited.

Quoted 8 lines

Here is an example of where both algorithms agree (1 Hz resolution):
freq = 1750000001 Hz, 3db: 1750000000 (old algorithm) => 1750000000 (new algorithm) Hz

Note that if the user is in auto mode then the corner frequency will be put almost exactly on their fundamental frequency. The same will happen with the new algorithm, but the user has the ability to select a minimum margin using DTS.

Quoted 6 lines
Quoted 4 lines

Multiline comment in IIO (and most of kernel for that matter) is /*
* This...

Shoot, I figured check_patch would have caught that. Noted for v5

Unfortunately it's not a consistent requirement across the kernel. networking still likes the style you used I think.

Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
drivers/iio/filter/admv8818.c +1 -0
@@ -242,16 +280,28 @@ static int admv8818_lpf_select(struct admv8818_state *st, u64 freq)
 static int admv8818_rfin_band_select(struct admv8818_state *st)
 {
 	int ret;
+	u64 hpf_corner_target, lpf_corner_target;
Quoted 8 lines

drivers/iio/filter/admv8818.c static int admv8818_lpf_select(struct admv8818_state *st, u64 freq) · line 287

Quoted 3 lines
drivers/iio/filter/admv8818.c +1 -0
 	st->cf_hz = clk_get_rate(st->clkin);
 
+	// Check for underflow

No C++ style comments in IIO code. This is just a consistency thing rather than really matter. We have lots of code that predates those being at all acceptable in the kernel and a mixture of the two styles is messy!

Quoted 2 lines

Bugger. check_patch failed me again :)
Noted. I will go through and address all comments to make sure they fit the style.

Likewise, not a hard rule in all of the kernel.

Quoted 11 lines
Quoted 9 lines

drivers/iio/filter/admv8818.c static int admv8818_lpf_select(struct admv8818_state *st, u64 freq) · line 288

Quoted 2 lines
drivers/iio/filter/admv8818.c +1 -0
+	if (st->cf_hz > st->hpf_margin_hz)

	st->lpf_margin_hz = 0;
	device_property_read_u64(...)

and no explicit error checking.

If you really want to retain the protection against wrong formats etc, then fair enough.

My only concern that the user will have no feedback that his or her filter settings are not being used which could lead to subtle, hard to track down frequency responses. Would it be more appropriate here to print a warning instead of returning an error?

Just stick to an error. I'd hope they test enough to know their DT is broken even without the error messages but i guess maybe not.

Jonathan