[PATCH 0/4] Add support for ADMV8818

[PATCH 0/4] Add support for ADMV8818

[Antoniu Miclaus]

The ADMV8818-EP is a fully monolithic microwave integrated 
circuit (MMIC) that features a digitally selectable frequency of 
operation. The device features four independently controlled high-
pass filters (HPFs) and four independently controlled low-pass 
filters (LPFs) that span the 2 GHz to 18 GHz frequency range.

This patch series includes a proposal to add a subsection in the
IIO subsystem for filter drivers.

NOTE:
Currently depends on 64-bit architecture since the input
clock that server as RFIN should support values
in the range 2 GHz to 18 GHz.

We might need some scaling implementation in the clock
framework so that u64 types are supported when using 32-bit
architectures.

Antoniu Miclaus (4):
  iio: add filter subfolder
  iio:filter:admv8818: add support for ADMV8818
  dt-bindings:iio:filter: add admv8818 doc
  iio:filter:admv8818: Add sysfs ABI documentation

 .../ABI/testing/sysfs-bus-iio-filter-admv8818 |  60 ++
 .../bindings/iio/filter/adi,admv8818.yaml     |  78 ++
 drivers/iio/Kconfig                           |   1 +
 drivers/iio/Makefile                          |   1 +
 drivers/iio/filter/Kconfig                    |  18 +
 drivers/iio/filter/Makefile                   |   7 +
 drivers/iio/filter/admv8818.c                 | 819 ++++++++++++++++++
 7 files changed, 984 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818
 create mode 100644 Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
 create mode 100644 drivers/iio/filter/Kconfig
 create mode 100644 drivers/iio/filter/Makefile
 create mode 100644 drivers/iio/filter/admv8818.c

-- 
2.33.1

[PATCH 1/4] iio: add filter subfolder

[Antoniu Miclaus]

Add filter subfolder for IIO devices that handle filter functionality.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
 drivers/iio/Kconfig         | 1 +
 drivers/iio/Makefile        | 1 +
 drivers/iio/filter/Kconfig  | 8 ++++++++
 drivers/iio/filter/Makefile | 6 ++++++
 4 files changed, 16 insertions(+)
 create mode 100644 drivers/iio/filter/Kconfig
 create mode 100644 drivers/iio/filter/Makefile

diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 2334ad249b46..3a496a28bad4 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -77,6 +77,7 @@ source "drivers/iio/chemical/Kconfig"
 source "drivers/iio/common/Kconfig"
 source "drivers/iio/dac/Kconfig"
 source "drivers/iio/dummy/Kconfig"
+source "drivers/iio/filter/Kconfig"
 source "drivers/iio/frequency/Kconfig"
 source "drivers/iio/gyro/Kconfig"
 source "drivers/iio/health/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index 65e39bd4f934..97d2fbcf0950 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -24,6 +24,7 @@ obj-y += common/
 obj-y += dac/
 obj-y += dummy/
 obj-y += gyro/
+obj-y += filter/
 obj-y += frequency/
 obj-y += health/
 obj-y += humidity/
diff --git a/drivers/iio/filter/Kconfig b/drivers/iio/filter/Kconfig
new file mode 100644
index 000000000000..e268bba43852
--- /dev/null
+++ b/drivers/iio/filter/Kconfig
@@ -0,0 +1,8 @@
+#
+# Filter drivers
+#
+# When adding new entries keep the list in alphabetical order
+
+menu "Filters"
+
+endmenu
diff --git a/drivers/iio/filter/Makefile b/drivers/iio/filter/Makefile
new file mode 100644
index 000000000000..cc0892c01142
--- /dev/null
+++ b/drivers/iio/filter/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for industrial I/O Filter drivers
+#
+
+# When adding new entries keep the list in alphabetical order
-- 
2.33.1

[PATCH 2/4] iio:filter:admv8818: add support for ADMV8818

[Antoniu Miclaus]

The ADMV8818-EP is a fully monolithic microwave integrated
circuit (MMIC) that features a digitally selectable frequency of
operation. The device features four independently controlled high-
pass filters (HPFs) and four independently controlled low-pass
filters (LPFs) that span the 2 GHz to 18 GHz frequency range.

Datasheet:
https://www.analog.com/media/en/technical-documentation/data-sheets/admv8818-ep.pdf
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
 drivers/iio/filter/Kconfig    |  10 +
 drivers/iio/filter/Makefile   |   1 +
 drivers/iio/filter/admv8818.c | 819 ++++++++++++++++++++++++++++++++++
 3 files changed, 830 insertions(+)
 create mode 100644 drivers/iio/filter/admv8818.c

diff --git a/drivers/iio/filter/Kconfig b/drivers/iio/filter/Kconfig
index e268bba43852..3ae35817ad82 100644
--- a/drivers/iio/filter/Kconfig
+++ b/drivers/iio/filter/Kconfig
@@ -5,4 +5,14 @@
 
 menu "Filters"
 
+config ADMV8818
+	tristate "Analog Devices ADMV8818 High-Pass and Low-Pass Filter"
+	depends on SPI && COMMON_CLK && 64BIT
+	help
+	  Say yes here to build support for Analog Devices ADMV8818
+	  2 GHz to 18 GHz, Digitally Tunable, High-Pass and Low-Pass Filter.
+
+	  To compile this driver as a module, choose M here: the
+	  modiule will be called admv8818.
+
 endmenu
diff --git a/drivers/iio/filter/Makefile b/drivers/iio/filter/Makefile
index cc0892c01142..55e228c0dd20 100644
--- a/drivers/iio/filter/Makefile
+++ b/drivers/iio/filter/Makefile
@@ -4,3 +4,4 @@
 #
 
 # When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_ADMV8818) += admv8818.o
diff --git a/drivers/iio/filter/admv8818.c b/drivers/iio/filter/admv8818.c
new file mode 100644
index 000000000000..c31f4e53573e
--- /dev/null
+++ b/drivers/iio/filter/admv8818.c
@@ -0,0 +1,819 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ADMV8818 driver
+ *
+ * Copyright 2021 Analog Devices Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+#include <linux/iio/sysfs.h>
+#include <linux/units.h>
+
+/* ADMV8818 Register Map */
+#define ADMV8818_REG_SPI_CONFIG_A		0x0
+#define ADMV8818_REG_SPI_CONFIG_B		0x1
+#define ADMV8818_REG_CHIPTYPE			0x3
+#define ADMV8818_REG_PRODUCT_ID_L		0x4
+#define ADMV8818_REG_PRODUCT_ID_H		0x5
+#define ADMV8818_REG_FAST_LATCH_POINTER		0x10
+#define ADMV8818_REG_FAST_LATCH_STOP		0x11
+#define ADMV8818_REG_FAST_LATCH_START		0x12
+#define ADMV8818_REG_FAST_LATCH_DIRECTION	0x13
+#define ADMV8818_REG_FAST_LATCH_STATE		0x14
+#define ADMV8818_REG_WR0_SW			0x20
+#define ADMV8818_REG_WR0_FILTER			0x21
+#define ADMV8818_REG_WR1_SW			0x22
+#define ADMV8818_REG_WR1_FILTER			0x23
+#define ADMV8818_REG_WR2_SW			0x24
+#define ADMV8818_REG_WR2_FILTER			0x25
+#define ADMV8818_REG_WR3_SW			0x26
+#define ADMV8818_REG_WR3_FILTER			0x27
+#define ADMV8818_REG_WR4_SW			0x28
+#define ADMV8818_REG_WR4_FILTER			0x29
+#define ADMV8818_REG_LUT0_SW			0x100
+#define ADMV8818_REG_LUT0_FILTER		0x101
+#define ADMV8818_REG_LUT127_SW			0x1FE
+#define ADMV8818_REG_LUT127_FILTER		0x1FF
+
+/* ADMV8818_REG_SPI_CONFIG_A Map */
+#define ADMV8818_SOFTRESET_N_MSK		BIT(7)
+#define ADMV8818_LSB_FIRST_N_MSK		BIT(6)
+#define ADMV8818_ENDIAN_N_MSK			BIT(5)
+#define ADMV8818_SDOACTIVE_N_MSK		BIT(4)
+#define ADMV8818_SDOACTIVE_MSK			BIT(3)
+#define ADMV8818_ENDIAN_MSK			BIT(2)
+#define ADMV8818_LSBFIRST_MSK			BIT(1)
+#define ADMV8818_SOFTRESET_MSK			BIT(0)
+
+/* ADMV8818_REG_SPI_CONFIG_B Map */
+#define ADMV8818_SINGLE_INSTRUCTION_MSK		BIT(7)
+#define ADMV8818_CSB_STALL_MSK			BIT(6)
+#define ADMV8818_MASTER_SLAVE_RB_MSK		BIT(5)
+#define ADMV8818_MASTER_SLAVE_TRANSFER_MSK	BIT(0)
+
+/* ADMV8818_REG_WR0_SW Map */
+#define ADMV8818_SW_IN_SET_WR0_MSK		BIT(7)
+#define ADMV8818_SW_OUT_SET_WR0_MSK		BIT(6)
+#define ADMV8818_SW_IN_WR0_MSK			GENMASK(5, 3)
+#define ADMV8818_SW_OUT_WR0_MSK			GENMASK(2, 0)
+
+/* ADMV8818_REG_WR0_FILTER Map */
+#define ADMV8818_HPF_WR0_MSK			GENMASK(7, 4)
+#define ADMV8818_LPF_WR0_MSK			GENMASK(3, 0)
+
+enum {
+	ADMV8818_BW_FREQ,
+	ADMV8818_CENTER_FREQ
+};
+
+struct admv8818_state {
+	struct spi_device	*spi;
+	struct regmap		*regmap;
+	struct clk		*clkin;
+	struct notifier_block	nb;
+	/* Protect against concurrent accesses to the device and data content*/
+	struct mutex		lock;
+	unsigned int		filter_mode;
+	unsigned int		freq_scale;
+	u64			cf_hz;
+	u64			bw_hz;
+};
+
+static const unsigned long long freq_range_hpf[4][2] = {
+	{1750000000ULL, 3550000000ULL},
+	{3400000000ULL, 7250000000ULL},
+	{6600000000, 12000000000},
+	{12500000000, 19900000000}
+};
+
+static const unsigned long long freq_range_lpf[4][2] = {
+	{2050000000ULL, 3850000000ULL},
+	{3350000000ULL, 7250000000ULL},
+	{7000000000, 13000000000},
+	{12550000000, 18500000000}
+};
+
+static const struct regmap_config admv8818_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 8,
+	.read_flag_mask = 0x80,
+	.max_register = 0x1FF,
+};
+
+static const char * const admv8818_modes[] = {
+	[0] = "auto",
+	[1] = "bypass",
+	[2] = "manual"
+};
+
+static int __admv8818_hpf_select(struct admv8818_state *st, u64 freq)
+{
+	unsigned int hpf_step = 0, hpf_band = 0, i, j;
+	u64 freq_step;
+	int ret;
+
+	if (freq < freq_range_hpf[0][0])
+		goto hpf_write;
+
+	if (freq > freq_range_hpf[3][1]) {
+		hpf_step = 15;
+		hpf_band = 4;
+
+		goto hpf_write;
+	}
+
+	for (i = 0; i < 4; i++) {
+		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 = 1; j <= 16; j++) {
+				if (freq < (freq_range_hpf[i][0] + (freq_step * j))) {
+					hpf_step = j - 1;
+					break;
+				}
+			}
+			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 |
+				 ADMV8818_SW_IN_WR0_MSK,
+				 FIELD_PREP(ADMV8818_SW_IN_SET_WR0_MSK, 1) |
+				 FIELD_PREP(ADMV8818_SW_IN_WR0_MSK, hpf_band));
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(st->regmap, ADMV8818_REG_WR0_FILTER,
+				  ADMV8818_HPF_WR0_MSK,
+				  FIELD_PREP(ADMV8818_HPF_WR0_MSK, hpf_step));
+}
+
+static int admv8818_hpf_select(struct admv8818_state *st, u64 freq)
+{
+	int ret;
+
+	mutex_lock(&st->lock);
+	ret = __admv8818_hpf_select(st, freq);
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static int __admv8818_lpf_select(struct admv8818_state *st, u64 freq)
+{
+	unsigned int lpf_step = 0, lpf_band = 0, i, j;
+	u64 freq_step;
+	int ret;
+
+	if (freq > freq_range_lpf[3][1])
+		goto lpf_write;
+
+	if (freq < freq_range_lpf[0][0]) {
+		lpf_band = 1;
+
+		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;
+				}
+			}
+			break;
+		}
+	}
+
+lpf_write:
+	ret = regmap_update_bits(st->regmap, ADMV8818_REG_WR0_SW,
+				 ADMV8818_SW_OUT_SET_WR0_MSK |
+				 ADMV8818_SW_OUT_WR0_MSK,
+				 FIELD_PREP(ADMV8818_SW_OUT_SET_WR0_MSK, 1) |
+				 FIELD_PREP(ADMV8818_SW_OUT_WR0_MSK, lpf_band));
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(st->regmap, ADMV8818_REG_WR0_FILTER,
+				  ADMV8818_LPF_WR0_MSK,
+				  FIELD_PREP(ADMV8818_LPF_WR0_MSK, lpf_step));
+}
+
+static int admv8818_lpf_select(struct admv8818_state *st, u64 freq)
+{
+	int ret;
+
+	mutex_lock(&st->lock);
+	ret = __admv8818_lpf_select(st, freq);
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static int admv8818_filter_bypass(struct admv8818_state *st)
+{
+	int ret;
+
+	mutex_lock(&st->lock);
+
+	ret = regmap_update_bits(st->regmap, ADMV8818_REG_WR0_SW,
+				 ADMV8818_SW_IN_SET_WR0_MSK |
+				 ADMV8818_SW_IN_WR0_MSK |
+				 ADMV8818_SW_OUT_SET_WR0_MSK |
+				 ADMV8818_SW_OUT_WR0_MSK,
+				 FIELD_PREP(ADMV8818_SW_IN_SET_WR0_MSK, 1) |
+				 FIELD_PREP(ADMV8818_SW_IN_WR0_MSK, 0) |
+				 FIELD_PREP(ADMV8818_SW_OUT_SET_WR0_MSK, 1) |
+				 FIELD_PREP(ADMV8818_SW_OUT_WR0_MSK, 0));
+	if (ret)
+		goto exit;
+
+	ret = regmap_update_bits(st->regmap, ADMV8818_REG_WR0_FILTER,
+				 ADMV8818_HPF_WR0_MSK |
+				 ADMV8818_LPF_WR0_MSK,
+				 FIELD_PREP(ADMV8818_HPF_WR0_MSK, 0) |
+				 FIELD_PREP(ADMV8818_LPF_WR0_MSK, 0));
+
+exit:
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static int admv8818_rfin_band_select(struct admv8818_state *st)
+{
+	int ret;
+	u64 lpf, hpf;
+
+	st->cf_hz = clk_get_rate(st->clkin);
+
+	if (!st->bw_hz) {
+		lpf = st->cf_hz;
+		hpf = st->cf_hz;
+	} else {
+		lpf = st->cf_hz + div_u64(st->bw_hz, 2);
+		hpf = st->cf_hz - div_u64(st->bw_hz, 2);
+	}
+
+	mutex_lock(&st->lock);
+
+	ret = __admv8818_hpf_select(st, hpf);
+	if (ret)
+		goto exit;
+
+	ret = __admv8818_lpf_select(st, lpf);
+exit:
+	mutex_unlock(&st->lock);
+	return ret;
+}
+
+static int __admv8818_read_hpf_freq(struct admv8818_state *st, u64 *hpf_freq)
+{
+	unsigned int data, hpf_band, hpf_state;
+	int ret;
+
+	ret = regmap_read(st->regmap, ADMV8818_REG_WR0_SW, &data);
+	if (ret)
+		return ret;
+
+	hpf_band = FIELD_GET(ADMV8818_SW_IN_WR0_MSK, data);
+	if (!hpf_band) {
+		*hpf_freq = 0;
+		return ret;
+	}
+
+	ret = regmap_read(st->regmap, ADMV8818_REG_WR0_FILTER, &data);
+	if (ret)
+		return ret;
+
+	hpf_state = FIELD_GET(ADMV8818_HPF_WR0_MSK, data);
+
+	*hpf_freq = div_u64(freq_range_hpf[hpf_band - 1][1] - freq_range_hpf[hpf_band - 1][0], 15);
+	*hpf_freq = freq_range_hpf[hpf_band - 1][0] + (*hpf_freq * hpf_state);
+
+	return ret;
+}
+
+static int admv8818_read_hpf_freq(struct admv8818_state *st, u64 *hpf_freq)
+{
+	int ret;
+
+	mutex_lock(&st->lock);
+	ret = __admv8818_read_hpf_freq(st, hpf_freq);
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static int __admv8818_read_lpf_freq(struct admv8818_state *st, u64 *lpf_freq)
+{
+	unsigned int data, lpf_band, lpf_state;
+	int ret;
+
+	ret = regmap_read(st->regmap, ADMV8818_REG_WR0_SW, &data);
+	if (ret)
+		return ret;
+
+	lpf_band = FIELD_GET(ADMV8818_SW_OUT_WR0_MSK, data);
+	if (!lpf_band) {
+		*lpf_freq = 0;
+		return ret;
+	}
+
+	ret = regmap_read(st->regmap, ADMV8818_REG_WR0_FILTER, &data);
+	if (ret)
+		return ret;
+
+	lpf_state = FIELD_GET(ADMV8818_LPF_WR0_MSK, data);
+
+	*lpf_freq = div_u64(freq_range_lpf[lpf_band - 1][1] - freq_range_lpf[lpf_band - 1][0], 15);
+	*lpf_freq = freq_range_lpf[lpf_band - 1][0] + (*lpf_freq * lpf_state);
+
+	return ret;
+}
+
+static int admv8818_read_lpf_freq(struct admv8818_state *st, u64 *lpf_freq)
+{
+	int ret;
+
+	mutex_lock(&st->lock);
+	ret = __admv8818_read_lpf_freq(st, lpf_freq);
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static int admv8818_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int val, int val2, long info)
+{
+	struct admv8818_state *st = iio_priv(indio_dev);
+
+	u64 freq = (u64)val * st->freq_scale;
+
+	switch (info) {
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		return admv8818_lpf_select(st, freq);
+	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
+		return admv8818_hpf_select(st, freq);
+	case IIO_CHAN_INFO_SCALE:
+		st->freq_scale = val;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int admv8818_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long info)
+{
+	struct admv8818_state *st = iio_priv(indio_dev);
+	int ret;
+	u64 freq;
+
+	switch (info) {
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		ret = admv8818_read_lpf_freq(st, &freq);
+		if (ret)
+			return ret;
+
+		*val = div_u64(freq, st->freq_scale);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
+		ret = admv8818_read_hpf_freq(st, &freq);
+		if (ret)
+			return ret;
+
+		*val = div_u64(freq, st->freq_scale);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = st->freq_scale;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int admv8818_reg_access(struct iio_dev *indio_dev,
+			       unsigned int reg,
+			       unsigned int write_val,
+			       unsigned int *read_val)
+{
+	struct admv8818_state *st = iio_priv(indio_dev);
+
+	if (read_val)
+		return regmap_read(st->regmap, reg, read_val);
+	else
+		return regmap_write(st->regmap, reg, write_val);
+}
+
+static ssize_t admv8818_read(struct iio_dev *indio_dev,
+			     uintptr_t private,
+			     const struct iio_chan_spec *chan,
+			     char *buf)
+{
+	struct admv8818_state *st = iio_priv(indio_dev);
+	u64 val, lpf_freq, hpf_freq;
+	int ret;
+
+	mutex_lock(&st->lock);
+
+	ret = __admv8818_read_lpf_freq(st, &lpf_freq);
+	if (ret)
+		goto exit;
+
+	ret = __admv8818_read_hpf_freq(st, &hpf_freq);
+
+exit:
+	mutex_unlock(&st->lock);
+
+	if (ret)
+		return ret;
+
+	switch ((u32)private) {
+	case ADMV8818_BW_FREQ:
+		val = lpf_freq - hpf_freq;
+		break;
+	case ADMV8818_CENTER_FREQ:
+		val = hpf_freq + (lpf_freq - hpf_freq) / 2;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return sysfs_emit(buf, "%llu\n", val);
+}
+
+static ssize_t admv8818_write(struct iio_dev *indio_dev,
+			      uintptr_t private,
+			      const struct iio_chan_spec *chan,
+			      const char *buf, size_t len)
+{
+	struct admv8818_state *st = iio_priv(indio_dev);
+	unsigned long long freq, lpf_freq, hpf_freq;
+	int ret;
+
+	ret = kstrtoull(buf, 10, &freq);
+	if (ret)
+		return ret;
+
+	switch ((u32)private) {
+	case ADMV8818_BW_FREQ:
+		st->bw_hz = freq;
+		hpf_freq = st->cf_hz - div_u64(freq, 2);
+		lpf_freq = hpf_freq + freq;
+
+		break;
+	case ADMV8818_CENTER_FREQ:
+		st->cf_hz = freq;
+		hpf_freq = freq - div_u64(st->bw_hz, 2);
+		lpf_freq = hpf_freq + st->bw_hz;
+
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&st->lock);
+
+	ret = __admv8818_lpf_select(st, lpf_freq);
+	if (ret)
+		goto exit;
+
+	ret = __admv8818_hpf_select(st, hpf_freq);
+
+exit:
+	mutex_unlock(&st->lock);
+
+	return ret ? ret : len;
+}
+
+static int admv8818_get_mode(struct iio_dev *indio_dev,
+			     const struct iio_chan_spec *chan)
+{
+	struct admv8818_state *st = iio_priv(indio_dev);
+
+	return st->filter_mode;
+}
+
+static int admv8818_set_mode(struct iio_dev *indio_dev,
+			     const struct iio_chan_spec *chan,
+			     unsigned int mode)
+{
+	struct admv8818_state *st = iio_priv(indio_dev);
+	int ret = 0;
+
+	switch (mode) {
+	case 0:
+		if (st->filter_mode && st->clkin) {
+			ret = clk_prepare_enable(st->clkin);
+			if (ret)
+				return ret;
+
+			ret = clk_notifier_register(st->clkin, &st->nb);
+			if (ret)
+				return ret;
+		} else {
+			return ret;
+		}
+
+		break;
+	case 1:
+		if (st->filter_mode == 0 && st->clkin) {
+			clk_disable_unprepare(st->clkin);
+
+			ret = clk_notifier_unregister(st->clkin, &st->nb);
+			if (ret)
+				return ret;
+		}
+
+		ret = admv8818_filter_bypass(st);
+		if (ret)
+			return ret;
+
+		break;
+	case 2:
+		if (st->filter_mode == 0 && st->clkin) {
+			clk_disable_unprepare(st->clkin);
+
+			ret = clk_notifier_unregister(st->clkin, &st->nb);
+			if (ret)
+				return ret;
+		} else {
+			return ret;
+		}
+
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	st->filter_mode = mode;
+
+	return ret;
+}
+
+static const struct iio_info admv8818_info = {
+	.write_raw = admv8818_write_raw,
+	.read_raw = admv8818_read_raw,
+	.debugfs_reg_access = &admv8818_reg_access,
+};
+
+#define _ADMV8818_EXT_INFO(_name, _shared, _ident) { \
+		.name = _name, \
+		.read = admv8818_read, \
+		.write = admv8818_write, \
+		.private = _ident, \
+		.shared = _shared, \
+}
+
+static const struct iio_enum admv8818_mode_enum = {
+	.items = admv8818_modes,
+	.num_items = ARRAY_SIZE(admv8818_modes),
+	.get = admv8818_get_mode,
+	.set = admv8818_set_mode,
+};
+
+#define IIO_ENUM_AVAILABLE_SHARED(_name, _shared, _e) \
+{ \
+	.name = (_name "_available"), \
+	.shared = _shared, \
+	.read = iio_enum_available_read, \
+	.private = (uintptr_t)(_e), \
+}
+
+static const struct iio_chan_spec_ext_info admv8818_ext_info[] = {
+	_ADMV8818_EXT_INFO("filter_band_pass_bandwidth_3db_frequency", IIO_SEPARATE,
+			   ADMV8818_BW_FREQ),
+	_ADMV8818_EXT_INFO("filter_band_pass_center_frequency", IIO_SEPARATE,
+			   ADMV8818_CENTER_FREQ),
+	IIO_ENUM("mode", IIO_SEPARATE, &admv8818_mode_enum),
+	IIO_ENUM_AVAILABLE_SHARED("mode", IIO_SEPARATE, &admv8818_mode_enum),
+	{ },
+};
+
+#define ADMV8818_CHAN(_channel) {				\
+	.type = IIO_ALTVOLTAGE,					\
+	.output = 1,						\
+	.indexed = 1,						\
+	.channel = _channel,					\
+	.info_mask_separate =					\
+		BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
+		BIT(IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY) | \
+		BIT(IIO_CHAN_INFO_SCALE) \
+}
+
+#define ADMV8818_CHAN_BW_CF(_channel, _admv8818_ext_info) {	\
+	.type = IIO_ALTVOLTAGE,					\
+	.output = 1,						\
+	.indexed = 1,						\
+	.channel = _channel,					\
+	.ext_info = _admv8818_ext_info,				\
+}
+
+static const struct iio_chan_spec admv8818_channels[] = {
+	ADMV8818_CHAN(0),
+	ADMV8818_CHAN_BW_CF(0, admv8818_ext_info),
+};
+
+static int admv8818_freq_change(struct notifier_block *nb, unsigned long action, void *data)
+{
+	struct admv8818_state *st = container_of(nb, struct admv8818_state, nb);
+
+	if (action == POST_RATE_CHANGE)
+		return notifier_from_errno(admv8818_rfin_band_select(st));
+
+	return NOTIFY_OK;
+}
+
+static void admv8818_clk_notifier_unreg(void *data)
+{
+	struct admv8818_state *st = data;
+
+	if (st->filter_mode == 0)
+		clk_notifier_unregister(st->clkin, &st->nb);
+}
+
+static void admv8818_clk_disable(void *data)
+{
+	struct admv8818_state *st = data;
+
+	if (st->filter_mode == 0)
+		clk_disable_unprepare(st->clkin);
+}
+
+static int admv8818_init(struct admv8818_state *st)
+{
+	int ret;
+	struct spi_device *spi = st->spi;
+	unsigned int chip_id;
+
+	ret = regmap_update_bits(st->regmap, ADMV8818_REG_SPI_CONFIG_A,
+				 ADMV8818_SOFTRESET_N_MSK |
+				 ADMV8818_SOFTRESET_MSK,
+				 FIELD_PREP(ADMV8818_SOFTRESET_N_MSK, 1) |
+				 FIELD_PREP(ADMV8818_SOFTRESET_MSK, 1));
+	if (ret) {
+		dev_err(&spi->dev, "ADMV8818 Soft Reset failed.\n");
+		return ret;
+	}
+
+	ret = regmap_update_bits(st->regmap, ADMV8818_REG_SPI_CONFIG_A,
+				 ADMV8818_SDOACTIVE_N_MSK |
+				 ADMV8818_SDOACTIVE_MSK,
+				 FIELD_PREP(ADMV8818_SDOACTIVE_N_MSK, 1) |
+				 FIELD_PREP(ADMV8818_SDOACTIVE_MSK, 1));
+	if (ret) {
+		dev_err(&spi->dev, "ADMV8818 SDO Enable failed.\n");
+		return ret;
+	}
+
+	ret = regmap_read(st->regmap, ADMV8818_REG_CHIPTYPE, &chip_id);
+	if (ret) {
+		dev_err(&spi->dev, "ADMV8818 Chip ID read failed.\n");
+		return ret;
+	}
+
+	if (chip_id != 0x1) {
+		dev_err(&spi->dev, "ADMV8818 Invalid Chip ID.\n");
+		return -EINVAL;
+	}
+
+	ret = regmap_update_bits(st->regmap, ADMV8818_REG_SPI_CONFIG_B,
+				 ADMV8818_SINGLE_INSTRUCTION_MSK,
+				 FIELD_PREP(ADMV8818_SINGLE_INSTRUCTION_MSK, 1));
+	if (ret) {
+		dev_err(&spi->dev, "ADMV8818 Single Instruction failed.\n");
+		return ret;
+	}
+
+	st->freq_scale = HZ_PER_MHZ;
+
+	if (st->clkin)
+		return admv8818_rfin_band_select(st);
+	else
+		return 0;
+}
+
+static int admv8818_clk_setup(struct admv8818_state *st)
+{
+	struct spi_device *spi = st->spi;
+	int ret;
+
+	st->clkin = devm_clk_get_optional(&spi->dev, "rf_in");
+	if (IS_ERR(st->clkin))
+		return dev_err_probe(&spi->dev, PTR_ERR(st->clkin),
+				     "failed to get the input clock\n");
+	else if (!st->clkin)
+		return 0;
+
+	device_property_read_u64(&spi->dev, "adi,bw-hz", &st->bw_hz);
+
+	ret = clk_prepare_enable(st->clkin);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(&spi->dev, admv8818_clk_disable, st);
+	if (ret)
+		return ret;
+
+	st->nb.notifier_call = admv8818_freq_change;
+	ret = clk_notifier_register(st->clkin, &st->nb);
+	if (ret < 0)
+		return ret;
+
+	return devm_add_action_or_reset(&spi->dev, admv8818_clk_notifier_unreg, st);
+}
+
+static int admv8818_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct regmap *regmap;
+	struct admv8818_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	regmap = devm_regmap_init_spi(spi, &admv8818_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	st = iio_priv(indio_dev);
+	st->regmap = regmap;
+
+	indio_dev->info = &admv8818_info;
+	indio_dev->name = "admv8818";
+	indio_dev->channels = admv8818_channels;
+	indio_dev->num_channels = ARRAY_SIZE(admv8818_channels);
+
+	st->spi = spi;
+
+	ret = admv8818_clk_setup(st);
+	if (ret)
+		return ret;
+
+	mutex_init(&st->lock);
+
+	ret = admv8818_init(st);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id admv8818_id[] = {
+	{ "admv8818", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, admv8818_id);
+
+static const struct of_device_id admv8818_of_match[] = {
+	{ .compatible = "adi,admv8818" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, admv8818_of_match);
+
+static struct spi_driver admv8818_driver = {
+	.driver = {
+		.name = "admv8818",
+		.of_match_table = admv8818_of_match,
+	},
+	.probe = admv8818_probe,
+	.id_table = admv8818_id,
+};
+module_spi_driver(admv8818_driver);
+
+MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@analog.com");
+MODULE_DESCRIPTION("Analog Devices ADMV8818");
+MODULE_LICENSE("GPL v2");
-- 
2.33.1

[PATCH 3/4] dt-bindings:iio:filter: add admv8818 doc

[Antoniu Miclaus]

Add device tree bindings for the ADMV8818 Filter.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
 .../bindings/iio/filter/adi,admv8818.yaml     | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml

diff --git a/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml b/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
new file mode 100644
index 000000000000..d581e236dbdc
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/filter/adi,admv8818.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ADMV8818 Digitally Tunable, High-Pass and Low-Pass Filter
+
+maintainers:
+  - Antoniu Miclaus <antoniu.miclaus@analog.com>
+
+description: |
+    Fully monolithic microwave integrated circuit (MMIC) that
+    features a digitally selectable frequency of operation.
+    The device features four independently controlled high-pass
+    filters (HPFs) and four independently controlled low-pass filters
+    (LPFs) that span the 2 GHz to 18 GHz frequency range.
+
+    https://www.analog.com/en/products/admv8818.html
+
+properties:
+  compatible:
+    enum:
+      - adi,admv8818
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 10000000
+
+  clocks:
+    description:
+      Definition of the external clock.
+    minItems: 1
+
+  clock-names:
+    items:
+      - const: "rf_in"
+
+  clock-output-names:
+    maxItems: 1
+
+  adi,bw-hz:
+    description:
+      Allows the user to increase the Bandpass Filter (BPF) bandwidth
+      in Hz. Normally when invoked by the clk notifier, the driver
+      sets the HPF cutoff close below the frequency and the LPF cutoff
+      close above the frequency, and thus creating a BPF.
+    $ref: /schemas/types.yaml#/definitions/uint64
+
+  '#clock-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+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,bw-hz = /bits/ 64 <600000000>;
+      };
+    };
+...
+
-- 
2.33.1

[PATCH 4/4] iio:filter:admv8818: Add sysfs ABI documentation

[Antoniu Miclaus]

Add initial ABI documentation for admv8818 filter sysfs interfaces.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
 .../ABI/testing/sysfs-bus-iio-filter-admv8818 | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818 b/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818
new file mode 100644
index 000000000000..7fa5b0819055
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818
@@ -0,0 +1,60 @@
+What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_high_pass_3db_frequency
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		The cut-off frequency of the ADMV8818 high pass filter. The value is scaled using
+		the `out_altvoltageY_scale` attribute so that GHz frequencies are valid inputs,
+		The accepted range of values for the frequencies is between 1.75GHz and 19.9GHz.
+
+		The default value for the scale is 1000000, therefore MHz frequency values are
+		passed as input.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_low_pass_3db_frequency
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		The cut-off frequency of the ADMV8818 low pass filter. The value is scaled using
+		the `out_altvoltageY_scale` attribute so that GHz frequencies are valid inputs,
+		The accepted range of values for the frequencies is between 2.05GHz and 18.85GHz.
+
+		The default value for the scale is 1000000, therefore MHz frequency values are
+		passed as input.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_scale
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Scale high pass and lowpass filter frequency values to Hz.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_mode_available
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Reading this returns the valid values that can be written to the
+		on_altvoltage0_mode attribute:
+
+		- auto -> enable/register the clock rate notifier
+		- manual -> disable/unregister the clock rate notifier
+		- bypass -> bypass LPF/HPF and disable/unregister the clock rate notifier
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_mode
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute configures the filter mode.
+		Reading returns the actual mode.
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_band_pass_bandwidth_3db_frequency
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Store the band pass bandwidth frequency value applied.
+		Reading returns the bandwidth frequency scaled.
+
+
+What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_band_pass_center_frequency
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Store the band pass center frequency value applied.
+		Reading returns the center frequency scaled.
-- 
2.33.1

Re: [PATCH 3/4] dt-bindings:iio:filter: add admv8818 doc

[Rob Herring]

On Tue, 09 Nov 2021 14:31:26 +0200, Antoniu Miclaus wrote:
> Add device tree bindings for the ADMV8818 Filter.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
>  .../bindings/iio/filter/adi,admv8818.yaml     | 78 +++++++++++++++++++
>  1 file changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml: properties:adi,bw-hz: '$ref' should not be valid under {'const': '$ref'}
	hint: Standard unit suffix properties don't need a type $ref
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml: ignoring, error in schema: properties: adi,bw-hz
warning: no schema found in file: ./Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
Documentation/devicetree/bindings/iio/filter/adi,admv8818.example.dt.yaml:0:0: /example-0/spi/admv8818@0: failed to match any schema with compatible: ['adi,admv8818']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1552959

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

RE: [PATCH 3/4] dt-bindings:iio:filter: add admv8818 doc

[Miclaus, Antoniu]

--
Antoniu Miclăuş

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Wednesday, November 10, 2021 3:07 AM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: jic23@kernel.org; linux-iio@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; robh+dt@kernel.org
> Subject: Re: [PATCH 3/4] dt-bindings:iio:filter: add admv8818 doc
> 
> [External]
> 
> On Tue, 09 Nov 2021 14:31:26 +0200, Antoniu Miclaus wrote:
> > Add device tree bindings for the ADMV8818 Filter.
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > ---
> >  .../bindings/iio/filter/adi,admv8818.yaml     | 78 +++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> >
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m
> dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/linux-dt-
> review/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml:
> properties:adi,bw-hz: '$ref' should not be valid under {'const': '$ref'}
> 	hint: Standard unit suffix properties don't need a type $ref
> 	from schema $id:
> https://urldefense.com/v3/__http://devicetree.org/meta-
> schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!tVywX0fQFR2vYZq-
> YTIJiJ0AXF2WapK4hXuLoFCYYlg19vxsZrLtIe7gWW7NfqCnlwuk$
> /builds/robherring/linux-dt-
> review/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml:
> ignoring, error in schema: properties: adi,bw-hz
> warning: no schema found in file:
> ./Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> Documentation/devicetree/bindings/iio/filter/adi,admv8818.example.dt.ya
> ml:0:0: /example-0/spi/admv8818@0: failed to match any schema with
> compatible: ['adi,admv8818']
> 
> doc reference errors (make refcheckdocs):
> 
> See
> https://urldefense.com/v3/__https://patchwork.ozlabs.org/patch/1552959_
> _;!!A3Ni8CS0y2Y!tVywX0fQFR2vYZq-
> YTIJiJ0AXF2WapK4hXuLoFCYYlg19vxsZrLtIe7gWW7NfqkOdOfT$
> 
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit.

Thanks!
I will resubmit this patch in v2 after the driver is reviewed.

Regards,
Antoniu
 

Re: [PATCH 1/4] iio: add filter subfolder

[Jonathan Cameron]

On Tue, 9 Nov 2021 14:31:24 +0200
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> Add filter subfolder for IIO devices that handle filter functionality.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>

Hi Antoniu,

Are we likely to see many filter drivers?  If not we could classify them
as analog front ends and put them in the AFE directory?

If there are going to be lots then I'm fine with a new directory.

Jonathan

> ---
>  drivers/iio/Kconfig         | 1 +
>  drivers/iio/Makefile        | 1 +
>  drivers/iio/filter/Kconfig  | 8 ++++++++
>  drivers/iio/filter/Makefile | 6 ++++++
>  4 files changed, 16 insertions(+)
>  create mode 100644 drivers/iio/filter/Kconfig
>  create mode 100644 drivers/iio/filter/Makefile
> 
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 2334ad249b46..3a496a28bad4 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -77,6 +77,7 @@ source "drivers/iio/chemical/Kconfig"
>  source "drivers/iio/common/Kconfig"
>  source "drivers/iio/dac/Kconfig"
>  source "drivers/iio/dummy/Kconfig"
> +source "drivers/iio/filter/Kconfig"
>  source "drivers/iio/frequency/Kconfig"
>  source "drivers/iio/gyro/Kconfig"
>  source "drivers/iio/health/Kconfig"
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index 65e39bd4f934..97d2fbcf0950 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -24,6 +24,7 @@ obj-y += common/
>  obj-y += dac/
>  obj-y += dummy/
>  obj-y += gyro/
> +obj-y += filter/
>  obj-y += frequency/
>  obj-y += health/
>  obj-y += humidity/
> diff --git a/drivers/iio/filter/Kconfig b/drivers/iio/filter/Kconfig
> new file mode 100644
> index 000000000000..e268bba43852
> --- /dev/null
> +++ b/drivers/iio/filter/Kconfig
> @@ -0,0 +1,8 @@
> +#
> +# Filter drivers
> +#
> +# When adding new entries keep the list in alphabetical order
> +
> +menu "Filters"
> +
> +endmenu
> diff --git a/drivers/iio/filter/Makefile b/drivers/iio/filter/Makefile
> new file mode 100644
> index 000000000000..cc0892c01142
> --- /dev/null
> +++ b/drivers/iio/filter/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for industrial I/O Filter drivers
> +#
> +
> +# When adding new entries keep the list in alphabetical order

Re: [PATCH 1/4] iio: add filter subfolder

[Jonathan Cameron]

On Fri, 12 Nov 2021 17:36:21 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 9 Nov 2021 14:31:24 +0200
> Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> 
> > Add filter subfolder for IIO devices that handle filter functionality.
> > 
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>  
> 
> Hi Antoniu,
> 
> Are we likely to see many filter drivers?  If not we could classify them
> as analog front ends and put them in the AFE directory?

On actually reading what device was I'm fine with filters directory.
AFE is more appropriate if we are talking about a front end for something
we are going to feed to an ADC rather that filtering a microwave frequency signal.

Jonathan

> 
> If there are going to be lots then I'm fine with a new directory.
> 
> Jonathan
> 
> > ---
> >  drivers/iio/Kconfig         | 1 +
> >  drivers/iio/Makefile        | 1 +
> >  drivers/iio/filter/Kconfig  | 8 ++++++++
> >  drivers/iio/filter/Makefile | 6 ++++++
> >  4 files changed, 16 insertions(+)
> >  create mode 100644 drivers/iio/filter/Kconfig
> >  create mode 100644 drivers/iio/filter/Makefile
> > 
> > diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> > index 2334ad249b46..3a496a28bad4 100644
> > --- a/drivers/iio/Kconfig
> > +++ b/drivers/iio/Kconfig
> > @@ -77,6 +77,7 @@ source "drivers/iio/chemical/Kconfig"
> >  source "drivers/iio/common/Kconfig"
> >  source "drivers/iio/dac/Kconfig"
> >  source "drivers/iio/dummy/Kconfig"
> > +source "drivers/iio/filter/Kconfig"
> >  source "drivers/iio/frequency/Kconfig"
> >  source "drivers/iio/gyro/Kconfig"
> >  source "drivers/iio/health/Kconfig"
> > diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> > index 65e39bd4f934..97d2fbcf0950 100644
> > --- a/drivers/iio/Makefile
> > +++ b/drivers/iio/Makefile
> > @@ -24,6 +24,7 @@ obj-y += common/
> >  obj-y += dac/
> >  obj-y += dummy/
> >  obj-y += gyro/
> > +obj-y += filter/
> >  obj-y += frequency/
> >  obj-y += health/
> >  obj-y += humidity/
> > diff --git a/drivers/iio/filter/Kconfig b/drivers/iio/filter/Kconfig
> > new file mode 100644
> > index 000000000000..e268bba43852
> > --- /dev/null
> > +++ b/drivers/iio/filter/Kconfig
> > @@ -0,0 +1,8 @@
> > +#
> > +# Filter drivers
> > +#
> > +# When adding new entries keep the list in alphabetical order
> > +
> > +menu "Filters"
> > +
> > +endmenu
> > diff --git a/drivers/iio/filter/Makefile b/drivers/iio/filter/Makefile
> > new file mode 100644
> > index 000000000000..cc0892c01142
> > --- /dev/null
> > +++ b/drivers/iio/filter/Makefile
> > @@ -0,0 +1,6 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Makefile for industrial I/O Filter drivers
> > +#
> > +
> > +# When adding new entries keep the list in alphabetical order  
> 

Re: [PATCH 3/4] dt-bindings:iio:filter: add admv8818 doc

[Jonathan Cameron]

On Tue, 9 Nov 2021 14:31:26 +0200
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> Add device tree bindings for the ADMV8818 Filter.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
>  .../bindings/iio/filter/adi,admv8818.yaml     | 78 +++++++++++++++++++
>  1 file changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml b/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> new file mode 100644
> index 000000000000..d581e236dbdc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> @@ -0,0 +1,78 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/filter/adi,admv8818.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ADMV8818 Digitally Tunable, High-Pass and Low-Pass Filter
> +
> +maintainers:
> +  - Antoniu Miclaus <antoniu.miclaus@analog.com>
> +
> +description: |
> +    Fully monolithic microwave integrated circuit (MMIC) that
> +    features a digitally selectable frequency of operation.
> +    The device features four independently controlled high-pass
> +    filters (HPFs) and four independently controlled low-pass filters
> +    (LPFs) that span the 2 GHz to 18 GHz frequency range.
> +
> +    https://www.analog.com/en/products/admv8818.html
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,admv8818
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 10000000
> +
> +  clocks:
> +    description:
> +      Definition of the external clock.
> +    minItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: "rf_in"

Is this what we'd normally think of as a clock signal?  I'd not expect
a nice squarewave on that pin for example so this seems an odd way to
define it.

> +
> +  clock-output-names:
> +    maxItems: 1
> +
> +  adi,bw-hz:
> +    description:
> +      Allows the user to increase the Bandpass Filter (BPF) bandwidth
> +      in Hz. Normally when invoked by the clk notifier, the driver
> +      sets the HPF cutoff close below the frequency and the LPF cutoff
> +      close above the frequency, and thus creating a BPF.

I don't understand this item at all.  Why do we need a control to
basically change how the other filter parameters are expressed?

> +    $ref: /schemas/types.yaml#/definitions/uint64
> +
> +  '#clock-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +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,bw-hz = /bits/ 64 <600000000>;
> +      };
> +    };
> +...
> +

Re: [PATCH 4/4] iio:filter:admv8818: Add sysfs ABI documentation

[Jonathan Cameron]

On Tue, 9 Nov 2021 14:31:27 +0200
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> Add initial ABI documentation for admv8818 filter sysfs interfaces.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
>  .../ABI/testing/sysfs-bus-iio-filter-admv8818 | 60 +++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818 b/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818
> new file mode 100644
> index 000000000000..7fa5b0819055
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818
> @@ -0,0 +1,60 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_high_pass_3db_frequency
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		The cut-off frequency of the ADMV8818 high pass filter. The value is scaled using
> +		the `out_altvoltageY_scale` attribute so that GHz frequencies are valid inputs,
> +		The accepted range of values for the frequencies is between 1.75GHz and 19.9GHz.
> +
> +		The default value for the scale is 1000000, therefore MHz frequency values are
> +		passed as input.

I don't think this ABI really works unfortunately.  What we are talking here is a bunch of
selectable filters and one high pass + one low pass filter max can be enabled at a time.

So two options, we either have simply a single
out_altvoltage_filter_low_pass_3db_frequency
out_altvoltage_filter_high_pass_3db_frequency
Probably both with index 0 and index free channels are a silly idea given it's fine to just have
one with index 0.

or if there is sufficient reason to setup a selectable set of options then
we could look at indexed filters and a _symbol type selection which may seem
odd but generalises fairly well from Phase Shift Keying type symbol stuff we
have had before (though still in staging because no one has cleaned the drivers
up yet).


> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_low_pass_3db_frequency
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		The cut-off frequency of the ADMV8818 low pass filter. The value is scaled using
> +		the `out_altvoltageY_scale` attribute so that GHz frequencies are valid inputs,
> +		The accepted range of values for the frequencies is between 2.05GHz and 18.85GHz.
> +
> +		The default value for the scale is 1000000, therefore MHz frequency values are
> +		passed as input.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_scale
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Scale high pass and lowpass filter frequency values to Hz.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_mode_available
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Reading this returns the valid values that can be written to the
> +		on_altvoltage0_mode attribute:
> +
> +		- auto -> enable/register the clock rate notifier

Hmm I'm wondering about the usecases of this.

If this is being used with a clk device, then I think only the notifier option makes much
sense.  If it's not a clk that linux is aware of then manual makes more sense.

> +		- manual -> disable/unregister the clock rate notifier
> +		- bypass -> bypass LPF/HPF and disable/unregister the clock rate notifier

This should be separate enable for the two filters though I think we've use the value 0
to mean this in the past.  The bypasses look to be per filter anyway, so a single
mode is insufficiently flexible.

In the vast majority of cases, mode attributes are not used because they are always device
specific and hence generic code has no idea what to do with them.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_mode
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute configures the filter mode.
> +		Reading returns the actual mode.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_band_pass_bandwidth_3db_frequency
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Store the band pass bandwidth frequency value applied.
> +		Reading returns the bandwidth frequency scaled.

The device has no concept of bandpass that I can find so why are we introducing it?
Let the user set the two filters to achieve this result.  Userspace can do the maths for us :)

> +
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_band_pass_center_frequency
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Store the band pass center frequency value applied.
> +		Reading returns the center frequency scaled.

Re: [PATCH 4/4] iio:filter:admv8818: Add sysfs ABI documentation

[Jonathan Cameron]

On Fri, 12 Nov 2021 17:56:25 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 9 Nov 2021 14:31:27 +0200
> Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> 
> > Add initial ABI documentation for admv8818 filter sysfs interfaces.
> > 
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > ---
> >  .../ABI/testing/sysfs-bus-iio-filter-admv8818 | 60 +++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818 b/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818
> > new file mode 100644
> > index 000000000000..7fa5b0819055
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818
> > @@ -0,0 +1,60 @@
> > +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_high_pass_3db_frequency
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		The cut-off frequency of the ADMV8818 high pass filter. The value is scaled using
> > +		the `out_altvoltageY_scale` attribute so that GHz frequencies are valid inputs,
> > +		The accepted range of values for the frequencies is between 1.75GHz and 19.9GHz.
> > +
> > +		The default value for the scale is 1000000, therefore MHz frequency values are
> > +		passed as input.  
> 
> I don't think this ABI really works unfortunately.  What we are talking here is a bunch of
> selectable filters and one high pass + one low pass filter max can be enabled at a time.
> 
> So two options, we either have simply a single
> out_altvoltage_filter_low_pass_3db_frequency
> out_altvoltage_filter_high_pass_3db_frequency
> Probably both with index 0 and index free channels are a silly idea given it's fine to just have
> one with index 0.
> 
> or if there is sufficient reason to setup a selectable set of options then
> we could look at indexed filters and a _symbol type selection which may seem
> odd but generalises fairly well from Phase Shift Keying type symbol stuff we
> have had before (though still in staging because no one has cleaned the drivers
> up yet).

Ignore this comment.  You have already done what I'm suggesting and I didn't read you docs
closely enough. Sorry about that!

However, this is generic, move it to the main sysfs-bus-iio docs rather than in here.

Snag is that we have to provide the values in Hz as that's what the ABI already has defined.

If we have to define ways to deal with all the zeros as they don't fit in existing path then
we can add those to the core.

Jonathan

> 
> 
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_low_pass_3db_frequency
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		The cut-off frequency of the ADMV8818 low pass filter. The value is scaled using
> > +		the `out_altvoltageY_scale` attribute so that GHz frequencies are valid inputs,
> > +		The accepted range of values for the frequencies is between 2.05GHz and 18.85GHz.
> > +
> > +		The default value for the scale is 1000000, therefore MHz frequency values are
> > +		passed as input.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_scale
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Scale high pass and lowpass filter frequency values to Hz.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_mode_available
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Reading this returns the valid values that can be written to the
> > +		on_altvoltage0_mode attribute:
> > +
> > +		- auto -> enable/register the clock rate notifier  
> 
> Hmm I'm wondering about the usecases of this.
> 
> If this is being used with a clk device, then I think only the notifier option makes much
> sense.  If it's not a clk that linux is aware of then manual makes more sense.
> 
> > +		- manual -> disable/unregister the clock rate notifier
> > +		- bypass -> bypass LPF/HPF and disable/unregister the clock rate notifier  
> 
> This should be separate enable for the two filters though I think we've use the value 0
> to mean this in the past.  The bypasses look to be per filter anyway, so a single
> mode is insufficiently flexible.
> 
> In the vast majority of cases, mode attributes are not used because they are always device
> specific and hence generic code has no idea what to do with them.
> 
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_mode
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		This attribute configures the filter mode.
> > +		Reading returns the actual mode.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_band_pass_bandwidth_3db_frequency
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Store the band pass bandwidth frequency value applied.
> > +		Reading returns the bandwidth frequency scaled.  
> 
> The device has no concept of bandpass that I can find so why are we introducing it?
> Let the user set the two filters to achieve this result.  Userspace can do the maths for us :)
> 
> > +
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_band_pass_center_frequency
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Store the band pass center frequency value applied.
> > +		Reading returns the center frequency scaled.  
> 

Re: [PATCH 2/4] iio:filter:admv8818: add support for ADMV8818

[Jonathan Cameron]

On Tue, 9 Nov 2021 14:31:25 +0200
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> The ADMV8818-EP is a fully monolithic microwave integrated
> circuit (MMIC) that features a digitally selectable frequency of
> operation. The device features four independently controlled high-
> pass filters (HPFs) and four independently controlled low-pass
> filters (LPFs) that span the 2 GHz to 18 GHz frequency range.
> 
> Datasheet:
> https://www.analog.com/media/en/technical-documentation/data-sheets/admv8818-ep.pdf
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>

No significant comments in the actual driver from me.  Nice and clean, we just
need to figure out the ABI questions etc.

Thanks,

Jonathan

> ---
>  drivers/iio/filter/Kconfig    |  10 +
>  drivers/iio/filter/Makefile   |   1 +
>  drivers/iio/filter/admv8818.c | 819 ++++++++++++++++++++++++++++++++++
>  3 files changed, 830 insertions(+)
>  create mode 100644 drivers/iio/filter/admv8818.c
> 
> diff --git a/drivers/iio/filter/Kconfig b/drivers/iio/filter/Kconfig
> index e268bba43852..3ae35817ad82 100644
> --- a/drivers/iio/filter/Kconfig
> +++ b/drivers/iio/filter/Kconfig
> @@ -5,4 +5,14 @@
>  
>  menu "Filters"
>  
> +config ADMV8818
> +	tristate "Analog Devices ADMV8818 High-Pass and Low-Pass Filter"
> +	depends on SPI && COMMON_CLK && 64BIT
> +	help
> +	  Say yes here to build support for Analog Devices ADMV8818
> +	  2 GHz to 18 GHz, Digitally Tunable, High-Pass and Low-Pass Filter.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  modiule will be called admv8818.
> +
>  endmenu
> diff --git a/drivers/iio/filter/Makefile b/drivers/iio/filter/Makefile
> index cc0892c01142..55e228c0dd20 100644
> --- a/drivers/iio/filter/Makefile
> +++ b/drivers/iio/filter/Makefile
> @@ -4,3 +4,4 @@
>  #
>  
>  # When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_ADMV8818) += admv8818.o
> diff --git a/drivers/iio/filter/admv8818.c b/drivers/iio/filter/admv8818.c
> new file mode 100644
> index 000000000000..c31f4e53573e
> --- /dev/null
> +++ b/drivers/iio/filter/admv8818.c
> @@ -0,0 +1,819 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ADMV8818 driver
> + *
> + * Copyright 2021 Analog Devices Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/notifier.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/units.h>
> +
> +/* ADMV8818 Register Map */
> +#define ADMV8818_REG_SPI_CONFIG_A		0x0
> +#define ADMV8818_REG_SPI_CONFIG_B		0x1
> +#define ADMV8818_REG_CHIPTYPE			0x3
> +#define ADMV8818_REG_PRODUCT_ID_L		0x4
> +#define ADMV8818_REG_PRODUCT_ID_H		0x5
> +#define ADMV8818_REG_FAST_LATCH_POINTER		0x10
> +#define ADMV8818_REG_FAST_LATCH_STOP		0x11
> +#define ADMV8818_REG_FAST_LATCH_START		0x12
> +#define ADMV8818_REG_FAST_LATCH_DIRECTION	0x13
> +#define ADMV8818_REG_FAST_LATCH_STATE		0x14
> +#define ADMV8818_REG_WR0_SW			0x20
> +#define ADMV8818_REG_WR0_FILTER			0x21
> +#define ADMV8818_REG_WR1_SW			0x22
> +#define ADMV8818_REG_WR1_FILTER			0x23
> +#define ADMV8818_REG_WR2_SW			0x24
> +#define ADMV8818_REG_WR2_FILTER			0x25
> +#define ADMV8818_REG_WR3_SW			0x26
> +#define ADMV8818_REG_WR3_FILTER			0x27
> +#define ADMV8818_REG_WR4_SW			0x28
> +#define ADMV8818_REG_WR4_FILTER			0x29
> +#define ADMV8818_REG_LUT0_SW			0x100
> +#define ADMV8818_REG_LUT0_FILTER		0x101
> +#define ADMV8818_REG_LUT127_SW			0x1FE
> +#define ADMV8818_REG_LUT127_FILTER		0x1FF
> +
> +/* ADMV8818_REG_SPI_CONFIG_A Map */
> +#define ADMV8818_SOFTRESET_N_MSK		BIT(7)
> +#define ADMV8818_LSB_FIRST_N_MSK		BIT(6)
> +#define ADMV8818_ENDIAN_N_MSK			BIT(5)
> +#define ADMV8818_SDOACTIVE_N_MSK		BIT(4)
> +#define ADMV8818_SDOACTIVE_MSK			BIT(3)
> +#define ADMV8818_ENDIAN_MSK			BIT(2)
> +#define ADMV8818_LSBFIRST_MSK			BIT(1)
> +#define ADMV8818_SOFTRESET_MSK			BIT(0)
> +
> +/* ADMV8818_REG_SPI_CONFIG_B Map */
> +#define ADMV8818_SINGLE_INSTRUCTION_MSK		BIT(7)
> +#define ADMV8818_CSB_STALL_MSK			BIT(6)
> +#define ADMV8818_MASTER_SLAVE_RB_MSK		BIT(5)
> +#define ADMV8818_MASTER_SLAVE_TRANSFER_MSK	BIT(0)
> +
> +/* ADMV8818_REG_WR0_SW Map */
> +#define ADMV8818_SW_IN_SET_WR0_MSK		BIT(7)
> +#define ADMV8818_SW_OUT_SET_WR0_MSK		BIT(6)
> +#define ADMV8818_SW_IN_WR0_MSK			GENMASK(5, 3)
> +#define ADMV8818_SW_OUT_WR0_MSK			GENMASK(2, 0)
> +
> +/* ADMV8818_REG_WR0_FILTER Map */
> +#define ADMV8818_HPF_WR0_MSK			GENMASK(7, 4)
> +#define ADMV8818_LPF_WR0_MSK			GENMASK(3, 0)
> +
> +enum {
> +	ADMV8818_BW_FREQ,
> +	ADMV8818_CENTER_FREQ
> +};
> +
> +struct admv8818_state {
> +	struct spi_device	*spi;
> +	struct regmap		*regmap;
> +	struct clk		*clkin;
> +	struct notifier_block	nb;
> +	/* Protect against concurrent accesses to the device and data content*/
> +	struct mutex		lock;
> +	unsigned int		filter_mode;
> +	unsigned int		freq_scale;
> +	u64			cf_hz;
> +	u64			bw_hz;
> +};
> +
> +static const unsigned long long freq_range_hpf[4][2] = {
> +	{1750000000ULL, 3550000000ULL},
> +	{3400000000ULL, 7250000000ULL},
> +	{6600000000, 12000000000},
> +	{12500000000, 19900000000}
> +};
> +
> +static const unsigned long long freq_range_lpf[4][2] = {
> +	{2050000000ULL, 3850000000ULL},
> +	{3350000000ULL, 7250000000ULL},
> +	{7000000000, 13000000000},
> +	{12550000000, 18500000000}
> +};
> +
> +static const struct regmap_config admv8818_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.read_flag_mask = 0x80,
> +	.max_register = 0x1FF,
> +};
> +
> +static const char * const admv8818_modes[] = {
> +	[0] = "auto",
> +	[1] = "bypass",
> +	[2] = "manual"
> +};
> +
> +static int __admv8818_hpf_select(struct admv8818_state *st, u64 freq)
> +{
> +	unsigned int hpf_step = 0, hpf_band = 0, i, j;
> +	u64 freq_step;
> +	int ret;
> +
> +	if (freq < freq_range_hpf[0][0])
> +		goto hpf_write;
> +
> +	if (freq > freq_range_hpf[3][1]) {
> +		hpf_step = 15;
> +		hpf_band = 4;
> +
> +		goto hpf_write;
> +	}
> +
> +	for (i = 0; i < 4; i++) {
> +		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 = 1; j <= 16; j++) {
> +				if (freq < (freq_range_hpf[i][0] + (freq_step * j))) {
> +					hpf_step = j - 1;
> +					break;
> +				}
> +			}
> +			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 |
> +				 ADMV8818_SW_IN_WR0_MSK,
> +				 FIELD_PREP(ADMV8818_SW_IN_SET_WR0_MSK, 1) |
> +				 FIELD_PREP(ADMV8818_SW_IN_WR0_MSK, hpf_band));
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(st->regmap, ADMV8818_REG_WR0_FILTER,
> +				  ADMV8818_HPF_WR0_MSK,
> +				  FIELD_PREP(ADMV8818_HPF_WR0_MSK, hpf_step));
> +}
> +
> +static int admv8818_hpf_select(struct admv8818_state *st, u64 freq)
> +{
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +	ret = __admv8818_hpf_select(st, freq);
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static int __admv8818_lpf_select(struct admv8818_state *st, u64 freq)
> +{
> +	unsigned int lpf_step = 0, lpf_band = 0, i, j;
> +	u64 freq_step;
> +	int ret;
> +
> +	if (freq > freq_range_lpf[3][1])
> +		goto lpf_write;
> +
> +	if (freq < freq_range_lpf[0][0]) {
> +		lpf_band = 1;
> +
> +		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;
> +				}
> +			}
> +			break;
> +		}
> +	}
> +
> +lpf_write:
> +	ret = regmap_update_bits(st->regmap, ADMV8818_REG_WR0_SW,
> +				 ADMV8818_SW_OUT_SET_WR0_MSK |
> +				 ADMV8818_SW_OUT_WR0_MSK,
> +				 FIELD_PREP(ADMV8818_SW_OUT_SET_WR0_MSK, 1) |
> +				 FIELD_PREP(ADMV8818_SW_OUT_WR0_MSK, lpf_band));
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(st->regmap, ADMV8818_REG_WR0_FILTER,
> +				  ADMV8818_LPF_WR0_MSK,
> +				  FIELD_PREP(ADMV8818_LPF_WR0_MSK, lpf_step));
> +}
> +
> +static int admv8818_lpf_select(struct admv8818_state *st, u64 freq)
> +{
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +	ret = __admv8818_lpf_select(st, freq);
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static int admv8818_filter_bypass(struct admv8818_state *st)
> +{
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +
> +	ret = regmap_update_bits(st->regmap, ADMV8818_REG_WR0_SW,
> +				 ADMV8818_SW_IN_SET_WR0_MSK |
> +				 ADMV8818_SW_IN_WR0_MSK |
> +				 ADMV8818_SW_OUT_SET_WR0_MSK |
> +				 ADMV8818_SW_OUT_WR0_MSK,
> +				 FIELD_PREP(ADMV8818_SW_IN_SET_WR0_MSK, 1) |
> +				 FIELD_PREP(ADMV8818_SW_IN_WR0_MSK, 0) |
> +				 FIELD_PREP(ADMV8818_SW_OUT_SET_WR0_MSK, 1) |
> +				 FIELD_PREP(ADMV8818_SW_OUT_WR0_MSK, 0));
> +	if (ret)
> +		goto exit;
> +
> +	ret = regmap_update_bits(st->regmap, ADMV8818_REG_WR0_FILTER,
> +				 ADMV8818_HPF_WR0_MSK |
> +				 ADMV8818_LPF_WR0_MSK,
> +				 FIELD_PREP(ADMV8818_HPF_WR0_MSK, 0) |
> +				 FIELD_PREP(ADMV8818_LPF_WR0_MSK, 0));
> +
> +exit:
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +

> +
> +static int admv8818_set_mode(struct iio_dev *indio_dev,
> +			     const struct iio_chan_spec *chan,
> +			     unsigned int mode)
> +{
> +	struct admv8818_state *st = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	switch (mode) {
> +	case 0:

If you do end up going this way then use an enum.

> +		if (st->filter_mode && st->clkin) {
> +			ret = clk_prepare_enable(st->clkin);
> +			if (ret)
> +				return ret;
> +
> +			ret = clk_notifier_register(st->clkin, &st->nb);
> +			if (ret)
> +				return ret;
> +		} else {
> +			return ret;
> +		}
> +
> +		break;
> +	case 1:
> +		if (st->filter_mode == 0 && st->clkin) {
> +			clk_disable_unprepare(st->clkin);
> +
> +			ret = clk_notifier_unregister(st->clkin, &st->nb);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		ret = admv8818_filter_bypass(st);
> +		if (ret)
> +			return ret;
> +
> +		break;
> +	case 2:
> +		if (st->filter_mode == 0 && st->clkin) {
> +			clk_disable_unprepare(st->clkin);
> +
> +			ret = clk_notifier_unregister(st->clkin, &st->nb);
> +			if (ret)
> +				return ret;
> +		} else {
> +			return ret;
> +		}
> +
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	st->filter_mode = mode;
> +
> +	return ret;
> +}

RE: [PATCH 3/4] dt-bindings:iio:filter: add admv8818 doc

[Miclaus, Antoniu]

Hello Jonathan,

--
Antoniu Miclăuş

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Friday, November 12, 2021 7:46 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: robh+dt@kernel.org; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] dt-bindings:iio:filter: add admv8818 doc
> 
> [External]
> 
> On Tue, 9 Nov 2021 14:31:26 +0200
> Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> 
> > Add device tree bindings for the ADMV8818 Filter.
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > ---
> >  .../bindings/iio/filter/adi,admv8818.yaml     | 78 +++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> >
> > diff --git
> a/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> b/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> > new file mode 100644
> > index 000000000000..d581e236dbdc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> > @@ -0,0 +1,78 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> https://urldefense.com/v3/__http://devicetree.org/schemas/iio/filter/adi,a
> dmv8818.yaml*__;Iw!!A3Ni8CS0y2Y!qkKokhmcgS0YEIy3uC6OfOOF7Bq3yE_r
> Ny91yIkDRTXFe54x-cHq_BtsyzDOedLohB5D$
> > +$schema: https://urldefense.com/v3/__http://devicetree.org/meta-
> schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!qkKokhmcgS0YEIy3uC6OfOOF7Bq3
> yE_rNy91yIkDRTXFe54x-cHq_BtsyzDOeYdHtx0a$
> > +
> > +title: ADMV8818 Digitally Tunable, High-Pass and Low-Pass Filter
> > +
> > +maintainers:
> > +  - Antoniu Miclaus <antoniu.miclaus@analog.com>
> > +
> > +description: |
> > +    Fully monolithic microwave integrated circuit (MMIC) that
> > +    features a digitally selectable frequency of operation.
> > +    The device features four independently controlled high-pass
> > +    filters (HPFs) and four independently controlled low-pass filters
> > +    (LPFs) that span the 2 GHz to 18 GHz frequency range.
> > +
> > +    https://www.analog.com/en/products/admv8818.html
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,admv8818
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-max-frequency:
> > +    maximum: 10000000
> > +
> > +  clocks:
> > +    description:
> > +      Definition of the external clock.
> > +    minItems: 1
> > +
> > +  clock-names:
> > +    items:
> > +      - const: "rf_in"
> 
> Is this what we'd normally think of as a clock signal?  I'd not expect
> a nice squarewave on that pin for example so this seems an odd way to
> define it.
> 
The only actual use of this part, until now, was to filter the output of the following part:
https://www.analog.com/en/products/adf5610.html
This is the reason of using the clock framework in the driver. Moreover, the clock input is
optional inside the driver.
> > +
> > +  clock-output-names:
> > +    maxItems: 1
> > +
> > +  adi,bw-hz:
> > +    description:
> > +      Allows the user to increase the Bandpass Filter (BPF) bandwidth
> > +      in Hz. Normally when invoked by the clk notifier, the driver
> > +      sets the HPF cutoff close below the frequency and the LPF cutoff
> > +      close above the frequency, and thus creating a BPF.
> 
> I don't understand this item at all.  Why do we need a control to
> basically change how the other filter parameters are expressed?
> 

Indeed, this property was requested by the users of the application in which this part was involved.
Same goes for the filter modes and the bandwidth in the ABI documentation.

If you think these attributes/properties are way too custom, I can drop them.

Let me know your thoughts.
> > +    $ref: /schemas/types.yaml#/definitions/uint64
> > +
> > +  '#clock-cells':
> > +    const: 0
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +
> > +additionalProperties: false
> > +
> > +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,bw-hz = /bits/ 64 <600000000>;
> > +      };
> > +    };
> > +...
> > +

RE: [PATCH 4/4] iio:filter:admv8818: Add sysfs ABI documentation

[Miclaus, Antoniu]

Hello Jonathan,

--
Antoniu Miclăuş

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Friday, November 12, 2021 7:56 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> Cc: robh+dt@kernel.org; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 4/4] iio:filter:admv8818: Add sysfs ABI documentation
> 
> [External]
> 
> On Tue, 9 Nov 2021 14:31:27 +0200
> Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> 
> > Add initial ABI documentation for admv8818 filter sysfs interfaces.
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > ---
> >  .../ABI/testing/sysfs-bus-iio-filter-admv8818 | 60 +++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-filter-
> admv8818
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818
> b/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818
> > new file mode 100644
> > index 000000000000..7fa5b0819055
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818
> > @@ -0,0 +1,60 @@
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_high_pass_3
> db_frequency
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		The cut-off frequency of the ADMV8818 high pass filter. The
> value is scaled using
> > +		the `out_altvoltageY_scale` attribute so that GHz frequencies
> are valid inputs,
> > +		The accepted range of values for the frequencies is between
> 1.75GHz and 19.9GHz.
> > +
> > +		The default value for the scale is 1000000, therefore MHz
> frequency values are
> > +		passed as input.
> 
> I don't think this ABI really works unfortunately.  What we are talking here is
> a bunch of
> selectable filters and one high pass + one low pass filter max can be enabled
> at a time.
> 
> So two options, we either have simply a single
> out_altvoltage_filter_low_pass_3db_frequency
> out_altvoltage_filter_high_pass_3db_frequency
> Probably both with index 0 and index free channels are a silly idea given it's
> fine to just have
> one with index 0.
> 
> or if there is sufficient reason to setup a selectable set of options then
> we could look at indexed filters and a _symbol type selection which may
> seem
> odd but generalises fairly well from Phase Shift Keying type symbol stuff we
> have had before (though still in staging because no one has cleaned the
> drivers
> up yet).
> 
> 
> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_low_pass_3
> db_frequency
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		The cut-off frequency of the ADMV8818 low pass filter. The
> value is scaled using
> > +		the `out_altvoltageY_scale` attribute so that GHz frequencies
> are valid inputs,
> > +		The accepted range of values for the frequencies is between
> 2.05GHz and 18.85GHz.
> > +
> > +		The default value for the scale is 1000000, therefore MHz
> frequency values are
> > +		passed as input.
> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_scale
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Scale high pass and lowpass filter frequency values to Hz.
> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_mode_available
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Reading this returns the valid values that can be written to
> the
> > +		on_altvoltage0_mode attribute:
> > +
> > +		- auto -> enable/register the clock rate notifier
> 
> Hmm I'm wondering about the usecases of this.
> 
> If this is being used with a clk device, then I think only the notifier option
> makes much
> sense.  If it's not a clk that linux is aware of then manual makes more sense.
> 
> > +		- manual -> disable/unregister the clock rate notifier
> > +		- bypass -> bypass LPF/HPF and disable/unregister the clock
> rate notifier
> 
> This should be separate enable for the two filters though I think we've use
> the value 0
> to mean this in the past.  The bypasses look to be per filter anyway, so a
> single
> mode is insufficiently flexible.
> 
> In the vast majority of cases, mode attributes are not used because they are
> always device
> specific and hence generic code has no idea what to do with them.
> 

As I mentioned also in the dt-bindings comments, these attributes were added  
because they were requested by the users of the application in which this part
was involved.

If you think these attributes/properties are way too custom, I can drop them.

Same goes for the bandwidth attribute.

> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_mode
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		This attribute configures the filter mode.
> > +		Reading returns the actual mode.
> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_band_pass_
> bandwidth_3db_frequency
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Store the band pass bandwidth frequency value applied.
> > +		Reading returns the bandwidth frequency scaled.
> 
> The device has no concept of bandpass that I can find so why are we
> introducing it?
> Let the user set the two filters to achieve this result.  Userspace can do the
> maths for us :)
> 
> > +
> > +
> > +What:
> 	/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_band_pass_
> center_frequency
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Store the band pass center frequency value applied.
> > +		Reading returns the center frequency scaled.

Re: [PATCH 3/4] dt-bindings:iio:filter: add admv8818 doc

[Jonathan Cameron]

On Tue, 16 Nov 2021 14:43:14 +0000
"Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:

> Hello Jonathan,
> 
> --
> Antoniu Miclăuş
> 
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Friday, November 12, 2021 7:46 PM
> > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > Cc: robh+dt@kernel.org; linux-iio@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 3/4] dt-bindings:iio:filter: add admv8818 doc
> > 
> > [External]
> > 
> > On Tue, 9 Nov 2021 14:31:26 +0200
> > Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> >   
> > > Add device tree bindings for the ADMV8818 Filter.
> > >
> > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > ---
> > >  .../bindings/iio/filter/adi,admv8818.yaml     | 78 +++++++++++++++++++
> > >  1 file changed, 78 insertions(+)
> > >  create mode 100644  
> > Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml  
> > >
> > > diff --git  
> > a/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> > b/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml  
> > > new file mode 100644
> > > index 000000000000..d581e236dbdc
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> > > @@ -0,0 +1,78 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id:  
> > https://urldefense.com/v3/__http://devicetree.org/schemas/iio/filter/adi,a
> > dmv8818.yaml*__;Iw!!A3Ni8CS0y2Y!qkKokhmcgS0YEIy3uC6OfOOF7Bq3yE_r
> > Ny91yIkDRTXFe54x-cHq_BtsyzDOedLohB5D$  
> > > +$schema: https://urldefense.com/v3/__http://devicetree.org/meta-  
> > schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!qkKokhmcgS0YEIy3uC6OfOOF7Bq3
> > yE_rNy91yIkDRTXFe54x-cHq_BtsyzDOeYdHtx0a$  
> > > +
> > > +title: ADMV8818 Digitally Tunable, High-Pass and Low-Pass Filter
> > > +
> > > +maintainers:
> > > +  - Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > +
> > > +description: |
> > > +    Fully monolithic microwave integrated circuit (MMIC) that
> > > +    features a digitally selectable frequency of operation.
> > > +    The device features four independently controlled high-pass
> > > +    filters (HPFs) and four independently controlled low-pass filters
> > > +    (LPFs) that span the 2 GHz to 18 GHz frequency range.
> > > +
> > > +    https://www.analog.com/en/products/admv8818.html
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - adi,admv8818
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  spi-max-frequency:
> > > +    maximum: 10000000
> > > +
> > > +  clocks:
> > > +    description:
> > > +      Definition of the external clock.
> > > +    minItems: 1
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: "rf_in"  
> > 
> > Is this what we'd normally think of as a clock signal?  I'd not expect
> > a nice squarewave on that pin for example so this seems an odd way to
> > define it.
> >   
> The only actual use of this part, until now, was to filter the output of the following part:
> https://www.analog.com/en/products/adf5610.html
> This is the reason of using the clock framework in the driver. Moreover, the clock input is
> optional inside the driver.

OK, so in theory that part is generating a sinusoid. I guess the clk framework works for
handling such devices, even if it's not typically what people expect from a clk.

> > > +
> > > +  clock-output-names:
> > > +    maxItems: 1
> > > +
> > > +  adi,bw-hz:
> > > +    description:
> > > +      Allows the user to increase the Bandpass Filter (BPF) bandwidth
> > > +      in Hz. Normally when invoked by the clk notifier, the driver
> > > +      sets the HPF cutoff close below the frequency and the LPF cutoff
> > > +      close above the frequency, and thus creating a BPF.  
> > 
> > I don't understand this item at all.  Why do we need a control to
> > basically change how the other filter parameters are expressed?
> >   
> 
> Indeed, this property was requested by the users of the application in which this part was involved.
> Same goes for the filter modes and the bandwidth in the ABI documentation.
> 
> If you think these attributes/properties are way too custom, I can drop them.

It's interesting.  I guess the point here is people want a nice autonomous system to
keep the filter set appropriately for cleaning up a generated sine wave.

It could be argued that is a hardware related thing so makes sense in DT.

We are sort of 'emulating' a bandpass filter in the driver if we use this, but
I guess if that's the main use case then this is perhaps a reasonable decision.
> 
> Let me know your thoughts.
> > > +    $ref: /schemas/types.yaml#/definitions/uint64
> > > +
> > > +  '#clock-cells':
> > > +    const: 0
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - clock-names
> > > +
> > > +additionalProperties: false
> > > +
> > > +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,bw-hz = /bits/ 64 <600000000>;
> > > +      };
> > > +    };
> > > +...
> > > +  
> 

Re: [PATCH 4/4] iio:filter:admv8818: Add sysfs ABI documentation

[Jonathan Cameron]

On Tue, 16 Nov 2021 14:43:16 +0000
"Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:

> Hello Jonathan,
Hi Antoniu
> 
> --
> Antoniu Miclăuş
> 
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Friday, November 12, 2021 7:56 PM
> > To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
> > Cc: robh+dt@kernel.org; linux-iio@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 4/4] iio:filter:admv8818: Add sysfs ABI documentation
> > 
> > [External]
> > 
> > On Tue, 9 Nov 2021 14:31:27 +0200
> > Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
> >   
> > > Add initial ABI documentation for admv8818 filter sysfs interfaces.
> > >
> > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > > ---
> > >  .../ABI/testing/sysfs-bus-iio-filter-admv8818 | 60 +++++++++++++++++++
> > >  1 file changed, 60 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-filter-  
> > admv8818  
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818  
> > b/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818  
> > > new file mode 100644
> > > index 000000000000..7fa5b0819055
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-filter-admv8818
> > > @@ -0,0 +1,60 @@
> > > +What:  
> > 	/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_high_pass_3
> > db_frequency  
> > > +KernelVersion:
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		The cut-off frequency of the ADMV8818 high pass filter. The  
> > value is scaled using  
> > > +		the `out_altvoltageY_scale` attribute so that GHz frequencies  
> > are valid inputs,  
> > > +		The accepted range of values for the frequencies is between  
> > 1.75GHz and 19.9GHz.  
> > > +
> > > +		The default value for the scale is 1000000, therefore MHz  
> > frequency values are  
> > > +		passed as input.  
> > 
> > I don't think this ABI really works unfortunately.  What we are talking here is
> > a bunch of
> > selectable filters and one high pass + one low pass filter max can be enabled
> > at a time.
> > 
> > So two options, we either have simply a single
> > out_altvoltage_filter_low_pass_3db_frequency
> > out_altvoltage_filter_high_pass_3db_frequency
> > Probably both with index 0 and index free channels are a silly idea given it's
> > fine to just have
> > one with index 0.
> > 
> > or if there is sufficient reason to setup a selectable set of options then
> > we could look at indexed filters and a _symbol type selection which may
> > seem
> > odd but generalises fairly well from Phase Shift Keying type symbol stuff we
> > have had before (though still in staging because no one has cleaned the
> > drivers
> > up yet).
> > 
> >   
> > > +
> > > +What:  
> > 	/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_low_pass_3
> > db_frequency  
> > > +KernelVersion:
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		The cut-off frequency of the ADMV8818 low pass filter. The  
> > value is scaled using  
> > > +		the `out_altvoltageY_scale` attribute so that GHz frequencies  
> > are valid inputs,  
> > > +		The accepted range of values for the frequencies is between  
> > 2.05GHz and 18.85GHz.  
> > > +
> > > +		The default value for the scale is 1000000, therefore MHz  
> > frequency values are  
> > > +		passed as input.
> > > +
> > > +What:  
> > 	/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_scale  
> > > +KernelVersion:
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Scale high pass and lowpass filter frequency values to Hz.
> > > +
> > > +What:  
> > 	/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_mode_available  
> > > +KernelVersion:
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Reading this returns the valid values that can be written to  
> > the  
> > > +		on_altvoltage0_mode attribute:
> > > +
> > > +		- auto -> enable/register the clock rate notifier  

Probably want to separate this description from the 'how'

So "auto -> Adjust bandpass filter to track changes in input clock rate."
or something along those lines.

> > 
> > Hmm I'm wondering about the usecases of this.
> > 
> > If this is being used with a clk device, then I think only the notifier option
> > makes much
> > sense.  If it's not a clk that linux is aware of then manual makes more sense.
> >   
> > > +		- manual -> disable/unregister the clock rate notifier
> > > +		- bypass -> bypass LPF/HPF and disable/unregister the clock  
> > rate notifier
> > 
> > This should be separate enable for the two filters though I think we've use
> > the value 0
> > to mean this in the past.  The bypasses look to be per filter anyway, so a
> > single
> > mode is insufficiently flexible.
> > 
> > In the vast majority of cases, mode attributes are not used because they are
> > always device
> > specific and hence generic code has no idea what to do with them.
> >   
> 
> As I mentioned also in the dt-bindings comments, these attributes were added  
> because they were requested by the users of the application in which this part
> was involved.
> 
> If you think these attributes/properties are way too custom, I can drop them.
> 
> Same goes for the bandwidth attribute.

If that's the most common use case then it's fine to keep them in my view.

Bypass is disabling a particular filter though so we should express it like that.
out_altvotage0_low_pass_filter_en etc

Also I think a single 1/0 attribute called something like

out_altvoltage0_filter_auto_en

which, when set makes the control parameters read only.



> 
> > > +
> > > +What:  
> > 	/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_mode  
> > > +KernelVersion:
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		This attribute configures the filter mode.
> > > +		Reading returns the actual mode.
> > > +
> > > +What:  
> > 	/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_band_pass_
> > bandwidth_3db_frequency  
> > > +KernelVersion:
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Store the band pass bandwidth frequency value applied.
> > > +		Reading returns the bandwidth frequency scaled.  
> > 
> > The device has no concept of bandpass that I can find so why are we
> > introducing it?
> > Let the user set the two filters to achieve this result.  Userspace can do the
> > maths for us :)

Definitely expose the two filters separately. The auto path can control them
appropriately but if things have moved to userspace control then I think
exposing each filter is a better bet.  Given we should be able to disable them
independently it's more than possible a user will want just a low pass or just
a high pass filter depending on their application.

> >   
> > > +
> > > +
> > > +What:  
> > 	/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_filter_band_pass_
> > center_frequency  
> > > +KernelVersion:
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Store the band pass center frequency value applied.
> > > +		Reading returns the center frequency scaled.  
>