[PATCH v2 0/6] iio: adc: ad7124: add filter support

[PATCH v2 0/6] iio: adc: ad7124: add filter support

[David Lechner]

The AD7124 family of chips supports a number of different filter modes.
This series eventually gets around to adding support for selecting the
filter mode at runtime after first doing some cleanups to the existing
code.

The first 3 patches are just updating things to use newer APIs.

The 4th patch is addressing a shortcoming in the driver where the
sampling_frequency attributes were being limited to an integer value.

The 5th patch is the one that finally adds support for filter_type
and filter_type_available attributes.

And the last patch documents one new filter_type that was used that we
haven't seen before.

This applies on top of "[PATCH v4] iio: adc: ad7124: fix sample rate for
multi-channel use"

---
Changes in v2:
- Rebased on v4 depedendency.
- Added divisor local variable.
- Link to v1: https://lore.kernel.org/r/20250905-iio-adc-ad7124-add-filter-support-v1-0-aee3834be6a9@baylibre.com

---
David Lechner (6):
      iio: adc: ad7124: use clamp()
      iio: adc: ad7124: use read_avail() for scale_available
      iio: adc: ad7124: use guard(mutex) to simplify return paths
      iio: adc: ad7124: support fractional sampling_frequency
      iio: adc: ad7124: add filter support
      iio: ABI: document "sinc4+rej60" filter_type

 Documentation/ABI/testing/sysfs-bus-iio |   1 +
 drivers/iio/adc/ad7124.c                | 383 +++++++++++++++++++++++++-------
 2 files changed, 299 insertions(+), 85 deletions(-)
---
base-commit: b8902d55155cec7bd743dc1129e0b32e70b1751f
change-id: 20250725-iio-adc-ad7124-add-filter-support-d1c9e53f64b5
prerequisite-message-id: 20250910-iio-adc-ad7124-fix-samp-freq-for-multi-channel-v4-1-8ca624c6114c@baylibre.com
prerequisite-patch-id: 780ed21c461fc7629b4eab73bede62a02e771379

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

[PATCH v2 1/6] iio: adc: ad7124: use clamp()

[David Lechner]

Use clamp() instead of open-coding clamping.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 15d98b78ec2709e14c354a64f14e7deefc3bcb56..8f6ca33d0c902be4ada103a32f37855c82a5f2fc 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -15,6 +15,7 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/kfifo.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/property.h>
@@ -299,11 +300,7 @@ static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel
 	 * FS[10:0] can have a value from 1 to 2047
 	 */
 	factor = 32 * 4; /* N = 4 for default sinc4 filter. */
-	odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * factor);
-	if (odr_sel_bits < 1)
-		odr_sel_bits = 1;
-	else if (odr_sel_bits > 2047)
-		odr_sel_bits = 2047;
+	odr_sel_bits = clamp(DIV_ROUND_CLOSEST(fclk, odr * factor), 1, 2047);
 
 	if (odr_sel_bits != st->channels[channel].cfg.odr_sel_bits)
 		st->channels[channel].cfg.live = false;

-- 
2.43.0

[PATCH v2 2/6] iio: adc: ad7124: use read_avail() for scale_available

[David Lechner]

Replace custom attribute with the standard IIO read_avail() callback
to provide in_voltage_scale_available attribute.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 42 +++++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 8f6ca33d0c902be4ada103a32f37855c82a5f2fc..97d601b3387524fe411cc0afc736618e32759880 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -621,6 +621,33 @@ static const struct ad_sigma_delta_info ad7124_sigma_delta_info = {
 	.num_resetclks = 64,
 };
 
+static const int ad7124_voltage_scales[][2] = {
+	{ 0, 1164 },
+	{ 0, 2328 },
+	{ 0, 4656 },
+	{ 0, 9313 },
+	{ 0, 18626 },
+	{ 0, 37252 },
+	{ 0, 74505 },
+	{ 0, 149011 },
+	{ 0, 298023 },
+};
+
+static int ad7124_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length, long info)
+{
+	switch (info) {
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (const int *)ad7124_voltage_scales;
+		*type = IIO_VAL_INT_PLUS_NANO;
+		*length = ARRAY_SIZE(ad7124_voltage_scales) * 2;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int ad7124_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val, int *val2, long info)
@@ -775,18 +802,6 @@ static int ad7124_reg_access(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static IIO_CONST_ATTR(in_voltage_scale_available,
-	"0.000001164 0.000002328 0.000004656 0.000009313 0.000018626 0.000037252 0.000074505 0.000149011 0.000298023");
-
-static struct attribute *ad7124_attributes[] = {
-	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
-	NULL,
-};
-
-static const struct attribute_group ad7124_attrs_group = {
-	.attrs = ad7124_attributes,
-};
-
 static int ad7124_update_scan_mode(struct iio_dev *indio_dev,
 				   const unsigned long *scan_mask)
 {
@@ -816,12 +831,12 @@ static int ad7124_update_scan_mode(struct iio_dev *indio_dev,
 }
 
 static const struct iio_info ad7124_info = {
+	.read_avail = ad7124_read_avail,
 	.read_raw = ad7124_read_raw,
 	.write_raw = ad7124_write_raw,
 	.debugfs_reg_access = &ad7124_reg_access,
 	.validate_trigger = ad_sd_validate_trigger,
 	.update_scan_mode = ad7124_update_scan_mode,
-	.attrs = &ad7124_attrs_group,
 };
 
 /* Only called during probe, so dev_err_probe() can be used */
@@ -1011,6 +1026,7 @@ static const struct iio_chan_spec ad7124_channel_template = {
 		BIT(IIO_CHAN_INFO_OFFSET) |
 		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
 		BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE),
 	.scan_type = {
 		.sign = 'u',
 		.realbits = 24,

-- 
2.43.0

[PATCH v2 3/6] iio: adc: ad7124: use guard(mutex) to simplify return paths

[David Lechner]

Use guard(mutex) in a couple of functions to allow direct returns. This
simplifies the code a bit and will make later changes easier.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 97d601b3387524fe411cc0afc736618e32759880..3afc4dcc315e82ab822370ff8d21590086e0bc7e 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -739,24 +739,20 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
 {
 	struct ad7124_state *st = iio_priv(indio_dev);
 	unsigned int res, gain, full_scale, vref;
-	int ret = 0;
 
-	mutex_lock(&st->cfgs_lock);
+	guard(mutex)(&st->cfgs_lock);
 
 	switch (info) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		if (val2 != 0 || val == 0) {
-			ret = -EINVAL;
-			break;
-		}
+		if (val2 != 0 || val == 0)
+			return -EINVAL;
 
 		ad7124_set_channel_odr(st, chan->address, val);
-		break;
+
+		return 0;
 	case IIO_CHAN_INFO_SCALE:
-		if (val != 0) {
-			ret = -EINVAL;
-			break;
-		}
+		if (val != 0)
+			return -EINVAL;
 
 		if (st->channels[chan->address].cfg.bipolar)
 			full_scale = 1 << (chan->scan_type.realbits - 1);
@@ -772,13 +768,10 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
 			st->channels[chan->address].cfg.live = false;
 
 		st->channels[chan->address].cfg.pga_bits = res;
-		break;
+		return 0;
 	default:
-		ret = -EINVAL;
+		return -EINVAL;
 	}
-
-	mutex_unlock(&st->cfgs_lock);
-	return ret;
 }
 
 static int ad7124_reg_access(struct iio_dev *indio_dev,
@@ -810,7 +803,8 @@ static int ad7124_update_scan_mode(struct iio_dev *indio_dev,
 	int ret;
 	int i;
 
-	mutex_lock(&st->cfgs_lock);
+	guard(mutex)(&st->cfgs_lock);
+
 	for (i = 0; i < st->num_channels; i++) {
 		bit_set = test_bit(i, scan_mask);
 		if (bit_set)
@@ -818,15 +812,10 @@ static int ad7124_update_scan_mode(struct iio_dev *indio_dev,
 		else
 			ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i), AD7124_CHANNEL_ENABLE,
 						    0, 2);
-		if (ret < 0) {
-			mutex_unlock(&st->cfgs_lock);
-
+		if (ret < 0)
 			return ret;
-		}
 	}
 
-	mutex_unlock(&st->cfgs_lock);
-
 	return 0;
 }
 

-- 
2.43.0

[PATCH v2 4/6] iio: adc: ad7124: support fractional sampling_frequency

[David Lechner]

Modify the attribute read/write functions for sampling_frequency and
filter_low_pass_3db_frequency to return fractional values.

These ADCs support output data rates in the single digits, so being
able to specify fractional values is necessary to use all possible
sampling frequencies.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 65 +++++++++++++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 3afc4dcc315e82ab822370ff8d21590086e0bc7e..b644191319a5eb6ab1a8ba22df4520edbb34ee75 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -166,7 +166,6 @@ struct ad7124_channel_config {
 		bool buf_negative;
 		unsigned int vref_mv;
 		unsigned int pga_bits;
-		unsigned int odr;
 		unsigned int odr_sel_bits;
 		unsigned int filter_type;
 		unsigned int calibration_offset;
@@ -285,7 +284,17 @@ static u32 ad7124_get_fclk_hz(struct ad7124_state *st)
 	return fclk_hz;
 }
 
-static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel, unsigned int odr)
+static u32 ad7124_get_fadc_divisor(struct ad7124_state *st, unsigned int channel)
+{
+	/*
+	 * The output data rate (f_ADC) is f_CLK / divisor. We are returning
+	 * the divisor.
+	 */
+	return st->channels[channel].cfg.odr_sel_bits * 32 * 4;
+}
+
+static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel,
+				   unsigned int odr, unsigned int odr_micro)
 {
 	unsigned int fclk, factor, odr_sel_bits;
 
@@ -300,29 +309,28 @@ static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel
 	 * FS[10:0] can have a value from 1 to 2047
 	 */
 	factor = 32 * 4; /* N = 4 for default sinc4 filter. */
-	odr_sel_bits = clamp(DIV_ROUND_CLOSEST(fclk, odr * factor), 1, 2047);
+	odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * factor +
+					       odr_micro * factor / MICRO);
+	odr_sel_bits = clamp(odr_sel_bits, 1, 2047);
 
 	if (odr_sel_bits != st->channels[channel].cfg.odr_sel_bits)
 		st->channels[channel].cfg.live = false;
 
-	/* fADC = fCLK / (FS[10:0] x 32) */
-	st->channels[channel].cfg.odr = DIV_ROUND_CLOSEST(fclk, odr_sel_bits *
-								factor);
 	st->channels[channel].cfg.odr_sel_bits = odr_sel_bits;
 }
 
-static int ad7124_get_3db_filter_freq(struct ad7124_state *st,
-				      unsigned int channel)
+static int ad7124_get_3db_filter_factor(struct ad7124_state *st,
+					unsigned int channel)
 {
-	unsigned int fadc;
-
-	fadc = st->channels[channel].cfg.odr;
-
+	/*
+	 * 3dB point is the f_CLK rate times some factor. This functions returns
+	 * the factor times 1000.
+	 */
 	switch (st->channels[channel].cfg.filter_type) {
 	case AD7124_FILTER_FILTER_SINC3:
-		return DIV_ROUND_CLOSEST(fadc * 272, 1000);
+		return 272;
 	case AD7124_FILTER_FILTER_SINC4:
-		return DIV_ROUND_CLOSEST(fadc * 230, 1000);
+		return 230;
 	default:
 		return -EINVAL;
 	}
@@ -346,7 +354,6 @@ static struct ad7124_channel_config *ad7124_find_similar_live_cfg(struct ad7124_
 				     bool buf_negative;
 				     unsigned int vref_mv;
 				     unsigned int pga_bits;
-				     unsigned int odr;
 				     unsigned int odr_sel_bits;
 				     unsigned int filter_type;
 				     unsigned int calibration_offset;
@@ -363,7 +370,6 @@ static struct ad7124_channel_config *ad7124_find_similar_live_cfg(struct ad7124_
 		    cfg->buf_negative == cfg_aux->buf_negative &&
 		    cfg->vref_mv == cfg_aux->vref_mv &&
 		    cfg->pga_bits == cfg_aux->pga_bits &&
-		    cfg->odr == cfg_aux->odr &&
 		    cfg->odr_sel_bits == cfg_aux->odr_sel_bits &&
 		    cfg->filter_type == cfg_aux->filter_type &&
 		    cfg->calibration_offset == cfg_aux->calibration_offset &&
@@ -718,16 +724,23 @@ static int ad7124_read_raw(struct iio_dev *indio_dev,
 
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		mutex_lock(&st->cfgs_lock);
-		*val = st->channels[chan->address].cfg.odr;
+		*val = ad7124_get_fclk_hz(st);
+		*val2 = ad7124_get_fadc_divisor(st, chan->address);
 		mutex_unlock(&st->cfgs_lock);
 
-		return IIO_VAL_INT;
-	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
-		mutex_lock(&st->cfgs_lock);
-		*val = ad7124_get_3db_filter_freq(st, chan->scan_index);
-		mutex_unlock(&st->cfgs_lock);
+		return IIO_VAL_FRACTIONAL;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: {
+		guard(mutex)(&st->cfgs_lock);
 
-		return IIO_VAL_INT;
+		ret = ad7124_get_3db_filter_factor(st, chan->address);
+		if (ret < 0)
+			return ret;
+
+		/* 3dB point is the f_CLK rate times a fractional value */
+		*val = ret * ad7124_get_fclk_hz(st);
+		*val2 = MILLI * ad7124_get_fadc_divisor(st, chan->address);
+		return IIO_VAL_FRACTIONAL;
+	}
 	default:
 		return -EINVAL;
 	}
@@ -744,10 +757,10 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
 
 	switch (info) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		if (val2 != 0 || val == 0)
+		if (val2 < 0 || val < 0 || (val2 == 0 && val == 0))
 			return -EINVAL;
 
-		ad7124_set_channel_odr(st, chan->address, val);
+		ad7124_set_channel_odr(st, chan->address, val, val2);
 
 		return 0;
 	case IIO_CHAN_INFO_SCALE:
@@ -1296,7 +1309,7 @@ static int ad7124_setup(struct ad7124_state *st)
 		 * regardless of the selected power mode. Round it up to 10 and
 		 * set all channels to this default value.
 		 */
-		ad7124_set_channel_odr(st, i, 10);
+		ad7124_set_channel_odr(st, i, 10, 0);
 	}
 
 	ad7124_disable_all(&st->sd);

-- 
2.43.0

[PATCH v2 5/6] iio: adc: ad7124: add filter support

[David Lechner]

Add support to the ad7124 driver for selecting the filter type.

The filter type has an influence on the effective sampling frequency of
each channel. For sinc3+pf{1,2,3,4}, the sampling frequency is fixed.
For sinc{3,4} (without post filter), there is a factor of 3 or 4
depending on the filter type. For the extra +sinc1, there is an extra
averaging factor that depends on the power mode.

In order to select the closest sampling frequency for each filter type,
we keep a copy of the requested sampling frequency. This way, if the
user sets the sampling frequency first and then selects the filter type,
the sampling frequency will still be as close as possible to the
requested value.

Since we always either have the SINGLE_CYCLE bit set or have more than
one channel enabled, the sampling frequency is always using the
"zero-latency" calculation from the data sheet. This is only documented
for the basic sinc{3,4} filters, so the other filter types had to be
inferred and confirmed through testing.

Since the flat filter type list consists of multiple register fields,
the struct ad7124_channel_config::filter_type field is changed to the
enum ad7124_filter_type type to avoid nested switch statements in a
lot of places.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 262 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 230 insertions(+), 32 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index b644191319a5eb6ab1a8ba22df4520edbb34ee75..910b40393f77de84afc77d406c17c6e5051a02cd 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -3,6 +3,7 @@
  * AD7124 SPI ADC driver
  *
  * Copyright 2018 Analog Devices Inc.
+ * Copyright 2025 BayLibre, SAS
  */
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
@@ -97,6 +98,15 @@
 #define AD7124_FILTER_FILTER		GENMASK(23, 21)
 #define AD7124_FILTER_FILTER_SINC4		0
 #define AD7124_FILTER_FILTER_SINC3		2
+#define AD7124_FILTER_FILTER_SINC4_SINC1	4
+#define AD7124_FILTER_FILTER_SINC3_SINC1	5
+#define AD7124_FILTER_FILTER_SINC3_PF		7
+#define AD7124_FILTER_REJ60		BIT(20)
+#define AD7124_FILTER_POST_FILTER	GENMASK(19, 17)
+#define AD7124_FILTER_POST_FILTER_47dB		2
+#define AD7124_FILTER_POST_FILTER_62dB		3
+#define AD7124_FILTER_POST_FILTER_86dB		5
+#define AD7124_FILTER_POST_FILTER_92dB		6
 #define AD7124_FILTER_SINGLE_CYCLE	BIT(16)
 #define AD7124_FILTER_FS		GENMASK(10, 0)
 
@@ -151,9 +161,24 @@ struct ad7124_chip_info {
 	unsigned int num_inputs;
 };
 
+enum ad7124_filter_type {
+	AD7124_FILTER_TYPE_SINC3,
+	AD7124_FILTER_TYPE_SINC3_PF1,
+	AD7124_FILTER_TYPE_SINC3_PF2,
+	AD7124_FILTER_TYPE_SINC3_PF3,
+	AD7124_FILTER_TYPE_SINC3_PF4,
+	AD7124_FILTER_TYPE_SINC3_REJ60,
+	AD7124_FILTER_TYPE_SINC3_SINC1,
+	AD7124_FILTER_TYPE_SINC4,
+	AD7124_FILTER_TYPE_SINC4_REJ60,
+	AD7124_FILTER_TYPE_SINC4_SINC1,
+};
+
 struct ad7124_channel_config {
 	bool live;
 	unsigned int cfg_slot;
+	unsigned int requested_odr;
+	unsigned int requested_odr_micro;
 	/*
 	 * Following fields are used to compare for equality. If you
 	 * make adaptations in it, you most likely also have to adapt
@@ -167,7 +192,7 @@ struct ad7124_channel_config {
 		unsigned int vref_mv;
 		unsigned int pga_bits;
 		unsigned int odr_sel_bits;
-		unsigned int filter_type;
+		enum ad7124_filter_type filter_type;
 		unsigned int calibration_offset;
 		unsigned int calibration_gain;
 	);
@@ -284,21 +309,50 @@ static u32 ad7124_get_fclk_hz(struct ad7124_state *st)
 	return fclk_hz;
 }
 
+static u32 ad7124_get_fs_factor(struct ad7124_state *st, unsigned int channel)
+{
+	enum ad7124_power_mode power_mode =
+		FIELD_GET(AD7124_ADC_CONTROL_POWER_MODE, st->adc_control);
+	u32 avg = power_mode == AD7124_LOW_POWER ? 8 : 16;
+
+	/*
+	 * These are the "zero-latency" factors from the data sheet. For the
+	 * sinc1 filters, these aren't documented, but derived by taking the
+	 * single-channel formula from the sinc1 section of the data sheet and
+	 * multiplying that by the sinc3/4 factor from the corresponding zero-
+	 * latency sections.
+	 */
+	switch (st->channels[channel].cfg.filter_type) {
+	case AD7124_FILTER_TYPE_SINC4:
+	case AD7124_FILTER_TYPE_SINC4_REJ60:
+		return 4 * 32;
+	case AD7124_FILTER_TYPE_SINC4_SINC1:
+		return 4 * avg * 32;
+	case AD7124_FILTER_TYPE_SINC3_SINC1:
+		return 3 * avg * 32;
+	default:
+		return 3 * 32;
+	}
+}
+
 static u32 ad7124_get_fadc_divisor(struct ad7124_state *st, unsigned int channel)
 {
+	u32 factor = ad7124_get_fs_factor(st, channel);
+
 	/*
 	 * The output data rate (f_ADC) is f_CLK / divisor. We are returning
 	 * the divisor.
 	 */
-	return st->channels[channel].cfg.odr_sel_bits * 32 * 4;
+	return st->channels[channel].cfg.odr_sel_bits * factor;
 }
 
-static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel,
-				   unsigned int odr, unsigned int odr_micro)
+static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel)
 {
-	unsigned int fclk, factor, odr_sel_bits;
+	struct ad7124_channel_config *cfg = &st->channels[channel].cfg;
+	unsigned int fclk, factor, divisor, odr_sel_bits;
 
 	fclk = ad7124_get_fclk_hz(st);
+	factor = ad7124_get_fs_factor(st, channel);
 
 	/*
 	 * FS[10:0] = fCLK / (fADC x 32 * N) where:
@@ -308,10 +362,9 @@ static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel
 	 * FS[10:0] are the bits in the filter register
 	 * FS[10:0] can have a value from 1 to 2047
 	 */
-	factor = 32 * 4; /* N = 4 for default sinc4 filter. */
-	odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * factor +
-					       odr_micro * factor / MICRO);
-	odr_sel_bits = clamp(odr_sel_bits, 1, 2047);
+	divisor = cfg->requested_odr * factor +
+		  cfg->requested_odr_micro * factor / MICRO;
+	odr_sel_bits = clamp(DIV_ROUND_CLOSEST(fclk, divisor), 1, 2047);
 
 	if (odr_sel_bits != st->channels[channel].cfg.odr_sel_bits)
 		st->channels[channel].cfg.live = false;
@@ -322,15 +375,29 @@ static void ad7124_set_channel_odr(struct ad7124_state *st, unsigned int channel
 static int ad7124_get_3db_filter_factor(struct ad7124_state *st,
 					unsigned int channel)
 {
+	struct ad7124_channel_config *cfg = &st->channels[channel].cfg;
+
 	/*
 	 * 3dB point is the f_CLK rate times some factor. This functions returns
 	 * the factor times 1000.
 	 */
-	switch (st->channels[channel].cfg.filter_type) {
-	case AD7124_FILTER_FILTER_SINC3:
+	switch (cfg->filter_type) {
+	case AD7124_FILTER_TYPE_SINC3:
+	case AD7124_FILTER_TYPE_SINC3_REJ60:
+	case AD7124_FILTER_TYPE_SINC3_SINC1:
 		return 272;
-	case AD7124_FILTER_FILTER_SINC4:
+	case AD7124_FILTER_TYPE_SINC4:
+	case AD7124_FILTER_TYPE_SINC4_REJ60:
+	case AD7124_FILTER_TYPE_SINC4_SINC1:
 		return 230;
+	case AD7124_FILTER_TYPE_SINC3_PF1:
+		return 633;
+	case AD7124_FILTER_TYPE_SINC3_PF2:
+		return 605;
+	case AD7124_FILTER_TYPE_SINC3_PF3:
+		return 669;
+	case AD7124_FILTER_TYPE_SINC3_PF4:
+		return 759;
 	default:
 		return -EINVAL;
 	}
@@ -355,7 +422,7 @@ static struct ad7124_channel_config *ad7124_find_similar_live_cfg(struct ad7124_
 				     unsigned int vref_mv;
 				     unsigned int pga_bits;
 				     unsigned int odr_sel_bits;
-				     unsigned int filter_type;
+				     enum ad7124_filter_type filter_type;
 				     unsigned int calibration_offset;
 				     unsigned int calibration_gain;
 			     }));
@@ -422,8 +489,9 @@ static int ad7124_init_config_vref(struct ad7124_state *st, struct ad7124_channe
 static int ad7124_write_config(struct ad7124_state *st, struct ad7124_channel_config *cfg,
 			       unsigned int cfg_slot)
 {
-	unsigned int tmp;
-	unsigned int val;
+	unsigned int val, filter;
+	unsigned int rej60 = 0;
+	unsigned int post = 0;
 	int ret;
 
 	cfg->cfg_slot = cfg_slot;
@@ -446,6 +514,47 @@ static int ad7124_write_config(struct ad7124_state *st, struct ad7124_channel_co
 	if (ret < 0)
 		return ret;
 
+	switch (cfg->filter_type) {
+	case AD7124_FILTER_TYPE_SINC3:
+		filter = AD7124_FILTER_FILTER_SINC3;
+		break;
+	case AD7124_FILTER_TYPE_SINC3_PF1:
+		filter = AD7124_FILTER_FILTER_SINC3_PF;
+		post = AD7124_FILTER_POST_FILTER_47dB;
+		break;
+	case AD7124_FILTER_TYPE_SINC3_PF2:
+		filter = AD7124_FILTER_FILTER_SINC3_PF;
+		post = AD7124_FILTER_POST_FILTER_62dB;
+		break;
+	case AD7124_FILTER_TYPE_SINC3_PF3:
+		filter = AD7124_FILTER_FILTER_SINC3_PF;
+		post = AD7124_FILTER_POST_FILTER_86dB;
+		break;
+	case AD7124_FILTER_TYPE_SINC3_PF4:
+		filter = AD7124_FILTER_FILTER_SINC3_PF;
+		post = AD7124_FILTER_POST_FILTER_92dB;
+		break;
+	case AD7124_FILTER_TYPE_SINC3_REJ60:
+		filter = AD7124_FILTER_FILTER_SINC3;
+		rej60 = 1;
+		break;
+	case AD7124_FILTER_TYPE_SINC3_SINC1:
+		filter = AD7124_FILTER_FILTER_SINC3_SINC1;
+		break;
+	case AD7124_FILTER_TYPE_SINC4:
+		filter = AD7124_FILTER_FILTER_SINC4;
+		break;
+	case AD7124_FILTER_TYPE_SINC4_REJ60:
+		filter = AD7124_FILTER_FILTER_SINC4;
+		rej60 = 1;
+		break;
+	case AD7124_FILTER_TYPE_SINC4_SINC1:
+		filter = AD7124_FILTER_FILTER_SINC4_SINC1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	/*
 	 * NB: AD7124_FILTER_SINGLE_CYCLE is always set so that we get the same
 	 * sampling frequency even when only one channel is enabled in a
@@ -453,14 +562,12 @@ static int ad7124_write_config(struct ad7124_state *st, struct ad7124_channel_co
 	 * would be 1 and we would get a faster sampling frequency than what
 	 * was requested.
 	 */
-	tmp = FIELD_PREP(AD7124_FILTER_FILTER, cfg->filter_type) |
-		AD7124_FILTER_SINGLE_CYCLE |
-		FIELD_PREP(AD7124_FILTER_FS, cfg->odr_sel_bits);
-	return ad7124_spi_write_mask(st, AD7124_FILTER(cfg->cfg_slot),
-				     AD7124_FILTER_FILTER |
-				     AD7124_FILTER_SINGLE_CYCLE |
-				     AD7124_FILTER_FS,
-				     tmp, 3);
+	return ad_sd_write_reg(&st->sd, AD7124_FILTER(cfg->cfg_slot), 3,
+			       FIELD_PREP(AD7124_FILTER_FILTER, filter) |
+			       FIELD_PREP(AD7124_FILTER_REJ60, rej60) |
+			       FIELD_PREP(AD7124_FILTER_POST_FILTER, post) |
+			       AD7124_FILTER_SINGLE_CYCLE |
+			       FIELD_PREP(AD7124_FILTER_FS, cfg->odr_sel_bits));
 }
 
 static struct ad7124_channel_config *ad7124_pop_config(struct ad7124_state *st)
@@ -722,13 +829,47 @@ static int ad7124_read_raw(struct iio_dev *indio_dev,
 			return -EINVAL;
 		}
 
-	case IIO_CHAN_INFO_SAMP_FREQ:
-		mutex_lock(&st->cfgs_lock);
-		*val = ad7124_get_fclk_hz(st);
-		*val2 = ad7124_get_fadc_divisor(st, chan->address);
-		mutex_unlock(&st->cfgs_lock);
+	case IIO_CHAN_INFO_SAMP_FREQ: {
+		struct ad7124_channel_config *cfg = &st->channels[chan->address].cfg;
 
-		return IIO_VAL_FRACTIONAL;
+		guard(mutex)(&st->cfgs_lock);
+
+		switch (cfg->filter_type) {
+		case AD7124_FILTER_TYPE_SINC3:
+		case AD7124_FILTER_TYPE_SINC3_REJ60:
+		case AD7124_FILTER_TYPE_SINC3_SINC1:
+		case AD7124_FILTER_TYPE_SINC4:
+		case AD7124_FILTER_TYPE_SINC4_REJ60:
+		case AD7124_FILTER_TYPE_SINC4_SINC1:
+			*val = ad7124_get_fclk_hz(st);
+			*val2 = ad7124_get_fadc_divisor(st, chan->address);
+			return IIO_VAL_FRACTIONAL;
+		/*
+		 * Post filters force the chip to a fixed rate. These are the
+		 * single-channel rates from the data sheet divided by 3 for
+		 * the multi-channel case (data sheet doesn't explicitly state
+		 * this but confirmed through testing).
+		 */
+		case AD7124_FILTER_TYPE_SINC3_PF1:
+			*val = 300;
+			*val2 = 33;
+			return IIO_VAL_FRACTIONAL;
+		case AD7124_FILTER_TYPE_SINC3_PF2:
+			*val = 25;
+			*val2 = 3;
+			return IIO_VAL_FRACTIONAL;
+		case AD7124_FILTER_TYPE_SINC3_PF3:
+			*val = 20;
+			*val2 = 3;
+			return IIO_VAL_FRACTIONAL;
+		case AD7124_FILTER_TYPE_SINC3_PF4:
+			*val = 50;
+			*val2 = 9;
+			return IIO_VAL_FRACTIONAL;
+		default:
+			return -EINVAL;
+		}
+	}
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: {
 		guard(mutex)(&st->cfgs_lock);
 
@@ -751,6 +892,7 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
 			    int val, int val2, long info)
 {
 	struct ad7124_state *st = iio_priv(indio_dev);
+	struct ad7124_channel_config *cfg = &st->channels[chan->address].cfg;
 	unsigned int res, gain, full_scale, vref;
 
 	guard(mutex)(&st->cfgs_lock);
@@ -760,7 +902,9 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
 		if (val2 < 0 || val < 0 || (val2 == 0 && val == 0))
 			return -EINVAL;
 
-		ad7124_set_channel_odr(st, chan->address, val, val2);
+		cfg->requested_odr = val;
+		cfg->requested_odr_micro = val2;
+		ad7124_set_channel_odr(st, chan->address);
 
 		return 0;
 	case IIO_CHAN_INFO_SCALE:
@@ -1006,6 +1150,52 @@ static const struct iio_enum ad7124_syscalib_mode_enum = {
 	.get = ad7124_get_syscalib_mode
 };
 
+static const char * const ad7124_filter_types[] = {
+	[AD7124_FILTER_TYPE_SINC3] = "sinc3",
+	[AD7124_FILTER_TYPE_SINC3_PF1] = "sinc3+pf1",
+	[AD7124_FILTER_TYPE_SINC3_PF2] = "sinc3+pf2",
+	[AD7124_FILTER_TYPE_SINC3_PF3] = "sinc3+pf3",
+	[AD7124_FILTER_TYPE_SINC3_PF4] = "sinc3+pf4",
+	[AD7124_FILTER_TYPE_SINC3_REJ60] = "sinc3+rej60",
+	[AD7124_FILTER_TYPE_SINC3_SINC1] = "sinc3+sinc1",
+	[AD7124_FILTER_TYPE_SINC4] = "sinc4",
+	[AD7124_FILTER_TYPE_SINC4_REJ60] = "sinc4+rej60",
+	[AD7124_FILTER_TYPE_SINC4_SINC1] = "sinc4+sinc1",
+};
+
+static int ad7124_set_filter_type_attr(struct iio_dev *dev,
+				       const struct iio_chan_spec *chan,
+				       unsigned int value)
+{
+	struct ad7124_state *st = iio_priv(dev);
+	struct ad7124_channel_config *cfg = &st->channels[chan->address].cfg;
+
+	guard(mutex)(&st->cfgs_lock);
+
+	cfg->live = false;
+	cfg->filter_type = value;
+	ad7124_set_channel_odr(st, chan->address);
+
+	return 0;
+}
+
+static int ad7124_get_filter_type_attr(struct iio_dev *dev,
+				       const struct iio_chan_spec *chan)
+{
+	struct ad7124_state *st = iio_priv(dev);
+
+	guard(mutex)(&st->cfgs_lock);
+
+	return st->channels[chan->address].cfg.filter_type;
+}
+
+static const struct iio_enum ad7124_filter_type_enum = {
+	.items = ad7124_filter_types,
+	.num_items = ARRAY_SIZE(ad7124_filter_types),
+	.set = ad7124_set_filter_type_attr,
+	.get = ad7124_get_filter_type_attr,
+};
+
 static const struct iio_chan_spec_ext_info ad7124_calibsys_ext_info[] = {
 	{
 		.name = "sys_calibration",
@@ -1016,6 +1206,9 @@ static const struct iio_chan_spec_ext_info ad7124_calibsys_ext_info[] = {
 		 &ad7124_syscalib_mode_enum),
 	IIO_ENUM_AVAILABLE("sys_calibration_mode", IIO_SHARED_BY_TYPE,
 			   &ad7124_syscalib_mode_enum),
+	IIO_ENUM("filter_type", IIO_SEPARATE, &ad7124_filter_type_enum),
+	IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_TYPE,
+			   &ad7124_filter_type_enum),
 	{ }
 };
 
@@ -1299,17 +1492,22 @@ static int ad7124_setup(struct ad7124_state *st)
 	mutex_init(&st->cfgs_lock);
 	INIT_KFIFO(st->live_cfgs_fifo);
 	for (i = 0; i < st->num_channels; i++) {
+		struct ad7124_channel_config *cfg = &st->channels[i].cfg;
 
-		ret = ad7124_init_config_vref(st, &st->channels[i].cfg);
+		ret = ad7124_init_config_vref(st, cfg);
 		if (ret < 0)
 			return ret;
 
+		/* Default filter type on the ADC after reset. */
+		cfg->filter_type = AD7124_FILTER_TYPE_SINC4;
+
 		/*
 		 * 9.38 SPS is the minimum output data rate supported
 		 * regardless of the selected power mode. Round it up to 10 and
 		 * set all channels to this default value.
 		 */
-		ad7124_set_channel_odr(st, i, 10, 0);
+		cfg->requested_odr = 10;
+		ad7124_set_channel_odr(st, i);
 	}
 
 	ad7124_disable_all(&st->sd);

-- 
2.43.0

[PATCH v2 6/6] iio: ABI: document "sinc4+rej60" filter_type

[David Lechner]

Add a bullet point for "sinc4+rej60" filter_type that is used in the
ad7124 driver.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 2fb2cea4b19249743398b1ff0b538b03ced0340b..829ccfd902f2ca4e5ae38ca025eb3c4a9fe1389d 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -2292,6 +2292,7 @@ Description:
 		  1st conversion time.
 		* "sinc4+sinc1" - Sinc4 + averaging by 8. Low 1st conversion
 		  time.
+		* "sinc4+rej60" - Sinc4 + 60Hz rejection.
 		* "sinc5" - The digital sinc5 filter. Excellent noise
 		  performance
 		* "sinc5+avg" - Sinc5 + averaging by 4.

-- 
2.43.0

Re: [PATCH v2 3/6] iio: adc: ad7124: use guard(mutex) to simplify return paths

[Andy Shevchenko]

On Fri, Sep 12, 2025 at 12:42 AM David Lechner <dlechner@baylibre.com> wrote:
>
> Use guard(mutex) in a couple of functions to allow direct returns. This
> simplifies the code a bit and will make later changes easier.

From this and the patch it's unclear if cleanup.h was already there or
not. If not, this patch misses it, if yes, the commit message should
be different.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/6] iio: adc: ad7124: support fractional sampling_frequency

[Andy Shevchenko]

On Fri, Sep 12, 2025 at 12:42 AM David Lechner <dlechner@baylibre.com> wrote:
>
> Modify the attribute read/write functions for sampling_frequency and
> filter_low_pass_3db_frequency to return fractional values.
>
> These ADCs support output data rates in the single digits, so being
> able to specify fractional values is necessary to use all possible
> sampling frequencies.

...

>         factor = 32 * 4; /* N = 4 for default sinc4 filter. */
> -       odr_sel_bits = clamp(DIV_ROUND_CLOSEST(fclk, odr * factor), 1, 2047);
> +       odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * factor +
> +                                              odr_micro * factor / MICRO);

> +       odr_sel_bits = clamp(odr_sel_bits, 1, 2047);

I would rather see this clamp() call to be the part of
cfg.odr_sel_bits() assignment, otherwise the above line and this
operate on the semantically (slightly) different data. So, the first
line should use different variable name, or the second, like
odr_sel_bits_clamped.

>         if (odr_sel_bits != st->channels[channel].cfg.odr_sel_bits)
>                 st->channels[channel].cfg.live = false;
>
> -       /* fADC = fCLK / (FS[10:0] x 32) */
> -       st->channels[channel].cfg.odr = DIV_ROUND_CLOSEST(fclk, odr_sel_bits *
> -                                                               factor);
>         st->channels[channel].cfg.odr_sel_bits = odr_sel_bits;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/6] iio: adc: ad7124: add filter support

[Andy Shevchenko]

On Fri, Sep 12, 2025 at 12:43 AM David Lechner <dlechner@baylibre.com> wrote:
>
> Add support to the ad7124 driver for selecting the filter type.
>
> The filter type has an influence on the effective sampling frequency of
> each channel. For sinc3+pf{1,2,3,4}, the sampling frequency is fixed.
> For sinc{3,4} (without post filter), there is a factor of 3 or 4
> depending on the filter type. For the extra +sinc1, there is an extra
> averaging factor that depends on the power mode.
>
> In order to select the closest sampling frequency for each filter type,
> we keep a copy of the requested sampling frequency. This way, if the
> user sets the sampling frequency first and then selects the filter type,
> the sampling frequency will still be as close as possible to the
> requested value.
>
> Since we always either have the SINGLE_CYCLE bit set or have more than
> one channel enabled, the sampling frequency is always using the
> "zero-latency" calculation from the data sheet. This is only documented
> for the basic sinc{3,4} filters, so the other filter types had to be
> inferred and confirmed through testing.
>
> Since the flat filter type list consists of multiple register fields,
> the struct ad7124_channel_config::filter_type field is changed to the
> enum ad7124_filter_type type to avoid nested switch statements in a
> lot of places.

...

> -       factor = 32 * 4; /* N = 4 for default sinc4 filter. */
> -       odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * factor +
> -                                              odr_micro * factor / MICRO);
> -       odr_sel_bits = clamp(odr_sel_bits, 1, 2047);
> +       divisor = cfg->requested_odr * factor +
> +                 cfg->requested_odr_micro * factor / MICRO;
> +       odr_sel_bits = clamp(DIV_ROUND_CLOSEST(fclk, divisor), 1, 2047);

I have a déjà vu feeling here. Is this similar code to elsewhere?  Can
it be factored out to a helper?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/6] iio: adc: ad7124: add filter support

[Andy Shevchenko]

On Fri, Sep 12, 2025 at 12:42 AM David Lechner <dlechner@baylibre.com> wrote:
>
> The AD7124 family of chips supports a number of different filter modes.
> This series eventually gets around to adding support for selecting the
> filter mode at runtime after first doing some cleanups to the existing
> code.
>
> The first 3 patches are just updating things to use newer APIs.
>
> The 4th patch is addressing a shortcoming in the driver where the
> sampling_frequency attributes were being limited to an integer value.
>
> The 5th patch is the one that finally adds support for filter_type
> and filter_type_available attributes.
>
> And the last patch documents one new filter_type that was used that we
> haven't seen before.
>
> This applies on top of "[PATCH v4] iio: adc: ad7124: fix sample rate for
> multi-channel use"

For non-commented patches
Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/6] iio: adc: ad7124: use guard(mutex) to simplify return paths

[David Lechner]

On 9/11/25 11:39 PM, Andy Shevchenko wrote:
> On Fri, Sep 12, 2025 at 12:42 AM David Lechner <dlechner@baylibre.com> wrote:
>>
>> Use guard(mutex) in a couple of functions to allow direct returns. This
>> simplifies the code a bit and will make later changes easier.
> 
> From this and the patch it's unclear if cleanup.h was already there or
> not. If not, this patch misses it, if yes, the commit message should
> be different.
> 

cleanup.h is already there. I'm not sure what would need to be different
in the commit message though.

Re: [PATCH v2 4/6] iio: adc: ad7124: support fractional sampling_frequency

[David Lechner]

On 9/11/25 11:45 PM, Andy Shevchenko wrote:
> On Fri, Sep 12, 2025 at 12:42 AM David Lechner <dlechner@baylibre.com> wrote:
>>
>> Modify the attribute read/write functions for sampling_frequency and
>> filter_low_pass_3db_frequency to return fractional values.
>>
>> These ADCs support output data rates in the single digits, so being
>> able to specify fractional values is necessary to use all possible
>> sampling frequencies.
> 
> ...
> 
>>         factor = 32 * 4; /* N = 4 for default sinc4 filter. */
>> -       odr_sel_bits = clamp(DIV_ROUND_CLOSEST(fclk, odr * factor), 1, 2047);
>> +       odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * factor +
>> +                                              odr_micro * factor / MICRO);
> 
>> +       odr_sel_bits = clamp(odr_sel_bits, 1, 2047);
> 
> I would rather see this clamp() call to be the part of
> cfg.odr_sel_bits() assignment, otherwise the above line and this
> operate on the semantically (slightly) different data. So, the first
> line should use different variable name, or the second, like
> odr_sel_bits_clamped.

If we moved it, then we would unnecessarily clear the cfg.live bit
in cases where clamping changed the value.

In a later commit, this gets combined to a single assignment so
not much point in adding a 2nd variable temporarily.

> 
>>         if (odr_sel_bits != st->channels[channel].cfg.odr_sel_bits)
>>                 st->channels[channel].cfg.live = false;
>>
>> -       /* fADC = fCLK / (FS[10:0] x 32) */
>> -       st->channels[channel].cfg.odr = DIV_ROUND_CLOSEST(fclk, odr_sel_bits *
>> -                                                               factor);
>>         st->channels[channel].cfg.odr_sel_bits = odr_sel_bits;
> 
> 

Re: [PATCH v2 5/6] iio: adc: ad7124: add filter support

[David Lechner]

On 9/11/25 11:49 PM, Andy Shevchenko wrote:
> On Fri, Sep 12, 2025 at 12:43 AM David Lechner <dlechner@baylibre.com> wrote:
>>
>> Add support to the ad7124 driver for selecting the filter type.
>>
>> The filter type has an influence on the effective sampling frequency of
>> each channel. For sinc3+pf{1,2,3,4}, the sampling frequency is fixed.
>> For sinc{3,4} (without post filter), there is a factor of 3 or 4
>> depending on the filter type. For the extra +sinc1, there is an extra
>> averaging factor that depends on the power mode.
>>
>> In order to select the closest sampling frequency for each filter type,
>> we keep a copy of the requested sampling frequency. This way, if the
>> user sets the sampling frequency first and then selects the filter type,
>> the sampling frequency will still be as close as possible to the
>> requested value.
>>
>> Since we always either have the SINGLE_CYCLE bit set or have more than
>> one channel enabled, the sampling frequency is always using the
>> "zero-latency" calculation from the data sheet. This is only documented
>> for the basic sinc{3,4} filters, so the other filter types had to be
>> inferred and confirmed through testing.
>>
>> Since the flat filter type list consists of multiple register fields,
>> the struct ad7124_channel_config::filter_type field is changed to the
>> enum ad7124_filter_type type to avoid nested switch statements in a
>> lot of places.
> 
> ...
> 
>> -       factor = 32 * 4; /* N = 4 for default sinc4 filter. */
>> -       odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * factor +
>> -                                              odr_micro * factor / MICRO);
>> -       odr_sel_bits = clamp(odr_sel_bits, 1, 2047);
>> +       divisor = cfg->requested_odr * factor +
>> +                 cfg->requested_odr_micro * factor / MICRO;
>> +       odr_sel_bits = clamp(DIV_ROUND_CLOSEST(fclk, divisor), 1, 2047);
> 
> I have a déjà vu feeling here. Is this similar code to elsewhere?  Can
> it be factored out to a helper?
> 
> 

It is changing the same code from a previous commit, not duplicating
it. I guess I could have introduced the divisor variable in the
earlier commit and saved some churn.

Re: [PATCH v2 3/6] iio: adc: ad7124: use guard(mutex) to simplify return paths

[Andy Shevchenko]

On Fri, Sep 12, 2025 at 09:19:36AM -0500, David Lechner wrote:
> On 9/11/25 11:39 PM, Andy Shevchenko wrote:
> > On Fri, Sep 12, 2025 at 12:42 AM David Lechner <dlechner@baylibre.com> wrote:
> >>
> >> Use guard(mutex) in a couple of functions to allow direct returns. This
> >> simplifies the code a bit and will make later changes easier.
> > 
> > From this and the patch it's unclear if cleanup.h was already there or
> > not. If not, this patch misses it, if yes, the commit message should
> > be different.
> 
> cleanup.h is already there. I'm not sure what would need to be different
> in the commit message though.

I expect something like "finish converting the driver to use guard()()..."

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/6] iio: adc: ad7124: use guard(mutex) to simplify return paths

[David Lechner]

On 9/12/25 12:15 PM, Andy Shevchenko wrote:
> On Fri, Sep 12, 2025 at 09:19:36AM -0500, David Lechner wrote:
>> On 9/11/25 11:39 PM, Andy Shevchenko wrote:
>>> On Fri, Sep 12, 2025 at 12:42 AM David Lechner <dlechner@baylibre.com> wrote:
>>>>
>>>> Use guard(mutex) in a couple of functions to allow direct returns. This
>>>> simplifies the code a bit and will make later changes easier.
>>>
>>> From this and the patch it's unclear if cleanup.h was already there or
>>> not. If not, this patch misses it, if yes, the commit message should
>>> be different.
>>
>> cleanup.h is already there. I'm not sure what would need to be different
>> in the commit message though.
> 
> I expect something like "finish converting the driver to use guard()()..."
> 

cleanup.h was previously included for __free(), so the guard() stuff
is all new.

Re: [PATCH v2 3/6] iio: adc: ad7124: use guard(mutex) to simplify return paths

[Andy Shevchenko]

On Fri, Sep 12, 2025 at 12:41:08PM -0500, David Lechner wrote:
> On 9/12/25 12:15 PM, Andy Shevchenko wrote:
> > On Fri, Sep 12, 2025 at 09:19:36AM -0500, David Lechner wrote:
> >> On 9/11/25 11:39 PM, Andy Shevchenko wrote:
> >>> On Fri, Sep 12, 2025 at 12:42 AM David Lechner <dlechner@baylibre.com> wrote:
> >>>>
> >>>> Use guard(mutex) in a couple of functions to allow direct returns. This
> >>>> simplifies the code a bit and will make later changes easier.
> >>>
> >>> From this and the patch it's unclear if cleanup.h was already there or
> >>> not. If not, this patch misses it, if yes, the commit message should
> >>> be different.
> >>
> >> cleanup.h is already there. I'm not sure what would need to be different
> >> in the commit message though.
> > 
> > I expect something like "finish converting the driver to use guard()()..."
> 
> cleanup.h was previously included for __free(), so the guard() stuff
> is all new.

Okay, then something like "Cover the lock handling using guard()()..."
The point I'm trying to make is that "Use $FOO API/etc" without new header
being included either:
1) missing inclusion (proxying);
2) start using of a new API from the library/header that we already use for
another API, but without mentioning that.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/6] iio: adc: ad7124: use guard(mutex) to simplify return paths

[Jonathan Cameron]

On Fri, 12 Sep 2025 21:07:58 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Fri, Sep 12, 2025 at 12:41:08PM -0500, David Lechner wrote:
> > On 9/12/25 12:15 PM, Andy Shevchenko wrote:  
> > > On Fri, Sep 12, 2025 at 09:19:36AM -0500, David Lechner wrote:  
> > >> On 9/11/25 11:39 PM, Andy Shevchenko wrote:  
> > >>> On Fri, Sep 12, 2025 at 12:42 AM David Lechner <dlechner@baylibre.com> wrote:  
> > >>>>
> > >>>> Use guard(mutex) in a couple of functions to allow direct returns. This
> > >>>> simplifies the code a bit and will make later changes easier.  
> > >>>
> > >>> From this and the patch it's unclear if cleanup.h was already there or
> > >>> not. If not, this patch misses it, if yes, the commit message should
> > >>> be different.  
> > >>
> > >> cleanup.h is already there. I'm not sure what would need to be different
> > >> in the commit message though.  
> > > 
> > > I expect something like "finish converting the driver to use guard()()..."  
> > 
> > cleanup.h was previously included for __free(), so the guard() stuff
> > is all new.  
> 
> Okay, then something like "Cover the lock handling using guard()()..."
> The point I'm trying to make is that "Use $FOO API/etc" without new header
> being included either:
> 1) missing inclusion (proxying);
> 2) start using of a new API from the library/header that we already use for
> another API, but without mentioning that.
> 
I went with the far from subtle solution of adding a line to the commit log
that says

cleanup.h is already included for prior use of __free()

Seemed like that would be enough for Andy's request and so I added
his tag (as given to the cover letter).

Jonathan

Re: [PATCH v2 5/6] iio: adc: ad7124: add filter support

[Jonathan Cameron]

On Fri, 12 Sep 2025 09:27:47 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 9/11/25 11:49 PM, Andy Shevchenko wrote:
> > On Fri, Sep 12, 2025 at 12:43 AM David Lechner <dlechner@baylibre.com> wrote:  
> >>
> >> Add support to the ad7124 driver for selecting the filter type.
> >>
> >> The filter type has an influence on the effective sampling frequency of
> >> each channel. For sinc3+pf{1,2,3,4}, the sampling frequency is fixed.
> >> For sinc{3,4} (without post filter), there is a factor of 3 or 4
> >> depending on the filter type. For the extra +sinc1, there is an extra
> >> averaging factor that depends on the power mode.
> >>
> >> In order to select the closest sampling frequency for each filter type,
> >> we keep a copy of the requested sampling frequency. This way, if the
> >> user sets the sampling frequency first and then selects the filter type,
> >> the sampling frequency will still be as close as possible to the
> >> requested value.
> >>
> >> Since we always either have the SINGLE_CYCLE bit set or have more than
> >> one channel enabled, the sampling frequency is always using the
> >> "zero-latency" calculation from the data sheet. This is only documented
> >> for the basic sinc{3,4} filters, so the other filter types had to be
> >> inferred and confirmed through testing.
> >>
> >> Since the flat filter type list consists of multiple register fields,
> >> the struct ad7124_channel_config::filter_type field is changed to the
> >> enum ad7124_filter_type type to avoid nested switch statements in a
> >> lot of places.  
> > 
> > ...
> >   
> >> -       factor = 32 * 4; /* N = 4 for default sinc4 filter. */
> >> -       odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * factor +
> >> -                                              odr_micro * factor / MICRO);
> >> -       odr_sel_bits = clamp(odr_sel_bits, 1, 2047);
> >> +       divisor = cfg->requested_odr * factor +
> >> +                 cfg->requested_odr_micro * factor / MICRO;
> >> +       odr_sel_bits = clamp(DIV_ROUND_CLOSEST(fclk, divisor), 1, 2047);  
> > 
> > I have a déjà vu feeling here. Is this similar code to elsewhere?  Can
> > it be factored out to a helper?
> > 
> >   
> 
> It is changing the same code from a previous commit, not duplicating
> it. I guess I could have introduced the divisor variable in the
> earlier commit and saved some churn.
For this and the previous patch, to me it feels like we are letting
aiming for perfect patch break up be the enemy of a good result.
So I've applied them both but as I don't know if Andy will agree
not his RB.

Jonathan

Re: [PATCH v2 0/6] iio: adc: ad7124: add filter support

[Jonathan Cameron]

On Fri, 12 Sep 2025 07:50:39 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Sep 12, 2025 at 12:42 AM David Lechner <dlechner@baylibre.com> wrote:
> >
> > The AD7124 family of chips supports a number of different filter modes.
> > This series eventually gets around to adding support for selecting the
> > filter mode at runtime after first doing some cleanups to the existing
> > code.
> >
> > The first 3 patches are just updating things to use newer APIs.
> >
> > The 4th patch is addressing a shortcoming in the driver where the
> > sampling_frequency attributes were being limited to an integer value.
> >
> > The 5th patch is the one that finally adds support for filter_type
> > and filter_type_available attributes.
> >
> > And the last patch documents one new filter_type that was used that we
> > haven't seen before.
> >
> > This applies on top of "[PATCH v4] iio: adc: ad7124: fix sample rate for
> > multi-channel use"  
> 
> For non-commented patches
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> 

There was a bit of fuzz on patch 6 but resolution looks good to me.
So applied whole series to the togreg branch of iio.git which I'll briefly
push out as testing. 

Re: [PATCH v2 5/6] iio: adc: ad7124: add filter support

[Andy Shevchenko]

On Sat, Sep 13, 2025 at 02:42:00PM +0100, Jonathan Cameron wrote:
> On Fri, 12 Sep 2025 09:27:47 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> > On 9/11/25 11:49 PM, Andy Shevchenko wrote:
> > > On Fri, Sep 12, 2025 at 12:43 AM David Lechner <dlechner@baylibre.com> wrote:  

...

> > >> -       factor = 32 * 4; /* N = 4 for default sinc4 filter. */
> > >> -       odr_sel_bits = DIV_ROUND_CLOSEST(fclk, odr * factor +
> > >> -                                              odr_micro * factor / MICRO);
> > >> -       odr_sel_bits = clamp(odr_sel_bits, 1, 2047);
> > >> +       divisor = cfg->requested_odr * factor +
> > >> +                 cfg->requested_odr_micro * factor / MICRO;
> > >> +       odr_sel_bits = clamp(DIV_ROUND_CLOSEST(fclk, divisor), 1, 2047);  
> > > 
> > > I have a déjà vu feeling here. Is this similar code to elsewhere?  Can
> > > it be factored out to a helper?
> > 
> > It is changing the same code from a previous commit, not duplicating
> > it. I guess I could have introduced the divisor variable in the
> > earlier commit and saved some churn.
> For this and the previous patch, to me it feels like we are letting
> aiming for perfect patch break up be the enemy of a good result.
> So I've applied them both but as I don't know if Andy will agree
> not his RB.

Even if I not agree, this is not a big deal. We can amend it in the future if
even needed. So, thanks!

-- 
With Best Regards,
Andy Shevchenko