[PATCH v2 0/3] iio: adc: ad7124: Fix 3dB filter frequency reading

[PATCH v2 0/3] iio: adc: ad7124: Fix 3dB filter frequency reading

[Uwe Kleine-König]

Hello,

(implicit) v1 of this patch set is available at
https://lore.kernel.org/linux-iio/cover.1741801853.git.u.kleine-koenig@baylibre.com
.

Changes since then:

 - Reorder patches to have the cleanup ("Make register naming
   consistent") last
 - Drop write support for the filter_low_pass_3db_frequency property
   which is completely broken.
 - trivially rebase to todays iio/togreg

I wonder if there is a way to remove the writable permission of the
filter_low_pass_3db_frequency sysfs file instead of erroring out when a
value is written. Hints welcome.

Best regards
Uwe

Uwe Kleine-König (3):
  iio: adc: ad7124: Fix 3dB filter frequency reading
  iio: adc: ad7124: Remove ability to write
    filter_low_pass_3db_frequency
  iio: adc: ad7124: Make register naming consistent

 drivers/iio/adc/ad7124.c | 208 ++++++++++++++++-----------------------
 1 file changed, 84 insertions(+), 124 deletions(-)


base-commit: 8dbeb413806f9f810d97d25284f585b201aa3bdc
-- 
2.47.1

[PATCH v2 1/3] iio: adc: ad7124: Fix 3dB filter frequency reading

[Uwe Kleine-König]

The sinc4 filter has a factor 0.23 between Output Data Rate and f_{3dB}
and for sinc3 the factor is 0.272 according to the data sheets for
ad7124-4 (Rev. E.) and ad7124-8 (Rev. F).

Fixes: cef2760954cf ("iio: adc: ad7124: add 3db filter")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 3ea81a98e455..7d5d84a07cae 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -301,9 +301,9 @@ static int ad7124_get_3db_filter_freq(struct ad7124_state *st,
 
 	switch (st->channels[channel].cfg.filter_type) {
 	case AD7124_SINC3_FILTER:
-		return DIV_ROUND_CLOSEST(fadc * 230, 1000);
+		return DIV_ROUND_CLOSEST(fadc * 272, 1000);
 	case AD7124_SINC4_FILTER:
-		return DIV_ROUND_CLOSEST(fadc * 262, 1000);
+		return DIV_ROUND_CLOSEST(fadc * 230, 1000);
 	default:
 		return -EINVAL;
 	}
-- 
2.47.1

[PATCH v2 2/3] iio: adc: ad7124: Remove ability to write filter_low_pass_3db_frequency

[Uwe Kleine-König]

There are several issues with the function that implements writing to
the filter_low_pass_3db_frequency property:

 - The sinc3 factor should be 0.272 not 0.262 (this is fixed for the
   reading side in the previous patch).
 - For freq > 1 the if condition is always true so the sinc4 filter is
   hardly ever chosen.
 - In the nearly always taken if branch the filter is set to sinc3, but
   the frequency is set for sinc4. (And vice versa in the else branch.)

This is broken enough to justify the claim that there isn't any serious
user. Also it it counter-intuitive that setting the 3db frequency
modifies the sample frequency and the filter type.

So drop the ability to write that property.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 36 +-----------------------------------
 1 file changed, 1 insertion(+), 35 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 7d5d84a07cae..662a3eb2f90e 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -309,32 +309,6 @@ static int ad7124_get_3db_filter_freq(struct ad7124_state *st,
 	}
 }
 
-static void ad7124_set_3db_filter_freq(struct ad7124_state *st, unsigned int channel,
-				       unsigned int freq)
-{
-	unsigned int sinc4_3db_odr;
-	unsigned int sinc3_3db_odr;
-	unsigned int new_filter;
-	unsigned int new_odr;
-
-	sinc4_3db_odr = DIV_ROUND_CLOSEST(freq * 1000, 230);
-	sinc3_3db_odr = DIV_ROUND_CLOSEST(freq * 1000, 262);
-
-	if (sinc4_3db_odr > sinc3_3db_odr) {
-		new_filter = AD7124_SINC3_FILTER;
-		new_odr = sinc4_3db_odr;
-	} else {
-		new_filter = AD7124_SINC4_FILTER;
-		new_odr = sinc3_3db_odr;
-	}
-
-	if (new_odr != st->channels[channel].cfg.odr)
-		st->channels[channel].cfg.live = false;
-
-	st->channels[channel].cfg.filter_type = new_filter;
-	st->channels[channel].cfg.odr = new_odr;
-}
-
 static struct ad7124_channel_config *ad7124_find_similar_live_cfg(struct ad7124_state *st,
 								  struct ad7124_channel_config *cfg)
 {
@@ -739,16 +713,8 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
 
 		st->channels[chan->address].cfg.pga_bits = res;
 		break;
-	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
-		if (val2 != 0) {
-			ret = -EINVAL;
-			break;
-		}
-
-		ad7124_set_3db_filter_freq(st, chan->address, val);
-		break;
 	default:
-		ret =  -EINVAL;
+		ret = -EINVAL;
 	}
 
 	mutex_unlock(&st->cfgs_lock);
-- 
2.47.1

[PATCH v2 3/3] iio: adc: ad7124: Make register naming consistent

[Uwe Kleine-König]

Cleanup definition of register related constants:

 - Use the register and field names exactly as documented in the data
   sheet.
 - Consistently use <regname>_<bitfield> to name a register's bitfield.
 - Drop _MSK definitions and implicit FIELD_PREP calls.
 - Consistent indentation.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 170 +++++++++++++++++++--------------------
 1 file changed, 82 insertions(+), 88 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 662a3eb2f90e..92596f15e797 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -32,7 +32,7 @@
 #define AD7124_IO_CONTROL_2		0x04
 #define AD7124_ID			0x05
 #define AD7124_ERROR			0x06
-#define AD7124_ERROR_EN		0x07
+#define AD7124_ERROR_EN			0x07
 #define AD7124_MCLK_COUNT		0x08
 #define AD7124_CHANNEL(x)		(0x09 + (x))
 #define AD7124_CONFIG(x)		(0x19 + (x))
@@ -41,68 +41,58 @@
 #define AD7124_GAIN(x)			(0x31 + (x))
 
 /* AD7124_STATUS */
-#define AD7124_STATUS_POR_FLAG_MSK	BIT(4)
+#define AD7124_STATUS_POR_FLAG			BIT(4)
 
 /* AD7124_ADC_CONTROL */
-#define AD7124_ADC_STATUS_EN_MSK	BIT(10)
-#define AD7124_ADC_STATUS_EN(x)		FIELD_PREP(AD7124_ADC_STATUS_EN_MSK, x)
-#define AD7124_ADC_CTRL_REF_EN_MSK	BIT(8)
-#define AD7124_ADC_CTRL_REF_EN(x)	FIELD_PREP(AD7124_ADC_CTRL_REF_EN_MSK, x)
-#define AD7124_ADC_CTRL_PWR_MSK	GENMASK(7, 6)
-#define AD7124_ADC_CTRL_PWR(x)		FIELD_PREP(AD7124_ADC_CTRL_PWR_MSK, x)
-#define AD7124_ADC_CTRL_MODE_MSK	GENMASK(5, 2)
-#define AD7124_ADC_CTRL_MODE(x)	FIELD_PREP(AD7124_ADC_CTRL_MODE_MSK, x)
+#define AD7124_ADC_CONTROL_MODE			GENMASK(5, 2)
+#define AD7124_ADC_CONTROL_MODE_CONTINUOUS		0
+#define AD7124_ADC_CONTROL_MODE_SINGLE			1
+#define AD7124_ADC_CONTROL_MODE_STANDBY			2
+#define AD7124_ADC_CONTROL_MODE_POWERDOWN		3
+#define AD7124_ADC_CONTROL_MODE_IDLE			4
+#define AD7124_ADC_CONTROL_MODE_INT_OFFSET_CALIB	5 /* Internal Zero-Scale Calibration */
+#define AD7124_ADC_CONTROL_MODE_INT_GAIN_CALIB		6 /* Internal Full-Scale Calibration */
+#define AD7124_ADC_CONTROL_MODE_SYS_OFFSET_CALIB	7 /* System Zero-Scale Calibration */
+#define AD7124_ADC_CONTROL_MODE_SYS_GAIN_CALIB		8 /* System Full-Scale Calibration */
+#define AD7124_ADC_CONTROL_POWER_MODE		GENMASK(7, 6)
+#define AD7124_ADC_CONTROL_POWER_MODE_LOW		0
+#define AD7124_ADC_CONTROL_POWER_MODE_MID		1
+#define AD7124_ADC_CONTROL_POWER_MODE_FULL		2
+#define AD7124_ADC_CONTROL_REF_EN		BIT(8)
+#define AD7124_ADC_CONTROL_DATA_STATUS		BIT(10)
 
-#define AD7124_MODE_CAL_INT_ZERO	0x5 /* Internal Zero-Scale Calibration */
-#define AD7124_MODE_CAL_INT_FULL	0x6 /* Internal Full-Scale Calibration */
-#define AD7124_MODE_CAL_SYS_ZERO	0x7 /* System Zero-Scale Calibration */
-#define AD7124_MODE_CAL_SYS_FULL	0x8 /* System Full-Scale Calibration */
-
-/* AD7124 ID */
-#define AD7124_DEVICE_ID_MSK		GENMASK(7, 4)
-#define AD7124_DEVICE_ID_GET(x)		FIELD_GET(AD7124_DEVICE_ID_MSK, x)
-#define AD7124_SILICON_REV_MSK		GENMASK(3, 0)
-#define AD7124_SILICON_REV_GET(x)	FIELD_GET(AD7124_SILICON_REV_MSK, x)
-
-#define CHIPID_AD7124_4			0x0
-#define CHIPID_AD7124_8			0x1
+/* AD7124_ID */
+#define AD7124_ID_SILICON_REVISION		GENMASK(3, 0)
+#define AD7124_ID_DEVICE_ID			GENMASK(7, 4)
+#define AD7124_ID_DEVICE_ID_AD7124_4			0x0
+#define AD7124_ID_DEVICE_ID_AD7124_8			0x1
 
 /* AD7124_CHANNEL_X */
-#define AD7124_CHANNEL_EN_MSK		BIT(15)
-#define AD7124_CHANNEL_EN(x)		FIELD_PREP(AD7124_CHANNEL_EN_MSK, x)
-#define AD7124_CHANNEL_SETUP_MSK	GENMASK(14, 12)
-#define AD7124_CHANNEL_SETUP(x)	FIELD_PREP(AD7124_CHANNEL_SETUP_MSK, x)
-#define AD7124_CHANNEL_AINP_MSK	GENMASK(9, 5)
-#define AD7124_CHANNEL_AINP(x)		FIELD_PREP(AD7124_CHANNEL_AINP_MSK, x)
-#define AD7124_CHANNEL_AINM_MSK	GENMASK(4, 0)
-#define AD7124_CHANNEL_AINM(x)		FIELD_PREP(AD7124_CHANNEL_AINM_MSK, x)
+#define AD7124_CHANNEL_ENABLE		BIT(15)
+#define AD7124_CHANNEL_SETUP		GENMASK(14, 12)
+#define AD7124_CHANNEL_AINP		GENMASK(9, 5)
+#define AD7124_CHANNEL_AINM		GENMASK(4, 0)
+#define AD7124_CHANNEL_AINx_TEMPSENSOR		16
+#define AD7124_CHANNEL_AINx_AVSS		17
 
 /* AD7124_CONFIG_X */
-#define AD7124_CONFIG_BIPOLAR_MSK	BIT(11)
-#define AD7124_CONFIG_BIPOLAR(x)	FIELD_PREP(AD7124_CONFIG_BIPOLAR_MSK, x)
-#define AD7124_CONFIG_REF_SEL_MSK	GENMASK(4, 3)
-#define AD7124_CONFIG_REF_SEL(x)	FIELD_PREP(AD7124_CONFIG_REF_SEL_MSK, x)
-#define AD7124_CONFIG_PGA_MSK		GENMASK(2, 0)
-#define AD7124_CONFIG_PGA(x)		FIELD_PREP(AD7124_CONFIG_PGA_MSK, x)
-#define AD7124_CONFIG_IN_BUFF_MSK	GENMASK(6, 5)
-#define AD7124_CONFIG_IN_BUFF(x)	FIELD_PREP(AD7124_CONFIG_IN_BUFF_MSK, x)
+#define AD7124_CONFIG_BIPOLAR		BIT(11)
+#define AD7124_CONFIG_IN_BUFF		GENMASK(6, 5)
+#define AD7124_CONFIG_AIN_BUFP		BIT(6)
+#define AD7124_CONFIG_AIN_BUFM		BIT(5)
+#define AD7124_CONFIG_REF_SEL		GENMASK(4, 3)
+#define AD7124_CONFIG_PGA		GENMASK(2, 0)
 
 /* AD7124_FILTER_X */
-#define AD7124_FILTER_FS_MSK		GENMASK(10, 0)
-#define AD7124_FILTER_FS(x)		FIELD_PREP(AD7124_FILTER_FS_MSK, x)
-#define AD7124_FILTER_TYPE_MSK		GENMASK(23, 21)
-#define AD7124_FILTER_TYPE_SEL(x)	FIELD_PREP(AD7124_FILTER_TYPE_MSK, x)
+#define AD7124_FILTER_FS		GENMASK(10, 0)
+#define AD7124_FILTER_FILTER		GENMASK(23, 21)
+#define AD7124_FILTER_FILTER_SINC4		0
+#define AD7124_FILTER_FILTER_SINC3		2
 
-#define AD7124_SINC3_FILTER 2
-#define AD7124_SINC4_FILTER 0
-
-#define AD7124_CONF_ADDR_OFFSET	20
 #define AD7124_MAX_CONFIGS	8
 #define AD7124_MAX_CHANNELS	16
 
 /* AD7124 input sources */
-#define AD7124_INPUT_TEMPSENSOR	16
-#define AD7124_INPUT_AVSS	17
 
 enum ad7124_ids {
 	ID_AD7124_4,
@@ -206,12 +196,12 @@ struct ad7124_state {
 static struct ad7124_chip_info ad7124_chip_info_tbl[] = {
 	[ID_AD7124_4] = {
 		.name = "ad7124-4",
-		.chip_id = CHIPID_AD7124_4,
+		.chip_id = AD7124_ID_DEVICE_ID_AD7124_4,
 		.num_inputs = 8,
 	},
 	[ID_AD7124_8] = {
 		.name = "ad7124-8",
-		.chip_id = CHIPID_AD7124_8,
+		.chip_id = AD7124_ID_DEVICE_ID_AD7124_8,
 		.num_inputs = 16,
 	},
 };
@@ -260,8 +250,8 @@ static int ad7124_set_mode(struct ad_sigma_delta *sd,
 {
 	struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
 
-	st->adc_control &= ~AD7124_ADC_CTRL_MODE_MSK;
-	st->adc_control |= AD7124_ADC_CTRL_MODE(mode);
+	st->adc_control &= ~AD7124_ADC_CONTROL_MODE;
+	st->adc_control |= FIELD_PREP(AD7124_ADC_CONTROL_MODE, mode);
 
 	return ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
 }
@@ -300,9 +290,9 @@ static int ad7124_get_3db_filter_freq(struct ad7124_state *st,
 	fadc = st->channels[channel].cfg.odr;
 
 	switch (st->channels[channel].cfg.filter_type) {
-	case AD7124_SINC3_FILTER:
+	case AD7124_FILTER_FILTER_SINC3:
 		return DIV_ROUND_CLOSEST(fadc * 272, 1000);
-	case AD7124_SINC4_FILTER:
+	case AD7124_FILTER_FILTER_SINC4:
 		return DIV_ROUND_CLOSEST(fadc * 230, 1000);
 	default:
 		return -EINVAL;
@@ -387,8 +377,7 @@ static int ad7124_init_config_vref(struct ad7124_state *st, struct ad7124_channe
 		return 0;
 	case AD7124_INT_REF:
 		cfg->vref_mv = 2500;
-		st->adc_control &= ~AD7124_ADC_CTRL_REF_EN_MSK;
-		st->adc_control |= AD7124_ADC_CTRL_REF_EN(1);
+		st->adc_control |= AD7124_ADC_CONTROL_REF_EN;
 		return 0;
 	default:
 		return dev_err_probe(dev, -EINVAL, "Invalid reference %d\n", refsel);
@@ -412,18 +401,20 @@ static int ad7124_write_config(struct ad7124_state *st, struct ad7124_channel_co
 	if (ret)
 		return ret;
 
-	tmp = (cfg->buf_positive << 1) + cfg->buf_negative;
-	val = AD7124_CONFIG_BIPOLAR(cfg->bipolar) | AD7124_CONFIG_REF_SEL(cfg->refsel) |
-	      AD7124_CONFIG_IN_BUFF(tmp) | AD7124_CONFIG_PGA(cfg->pga_bits);
+	val = FIELD_PREP(AD7124_CONFIG_BIPOLAR, cfg->bipolar) |
+		FIELD_PREP(AD7124_CONFIG_REF_SEL, cfg->refsel) |
+		(cfg->buf_positive ? AD7124_CONFIG_AIN_BUFP : 0) |
+		(cfg->buf_negative ? AD7124_CONFIG_AIN_BUFM : 0) |
+		FIELD_PREP(AD7124_CONFIG_PGA, cfg->pga_bits);
 
 	ret = ad_sd_write_reg(&st->sd, AD7124_CONFIG(cfg->cfg_slot), 2, val);
 	if (ret < 0)
 		return ret;
 
-	tmp = AD7124_FILTER_TYPE_SEL(cfg->filter_type) |
-	      AD7124_FILTER_FS(cfg->odr_sel_bits);
+	tmp = FIELD_PREP(AD7124_FILTER_FILTER, cfg->filter_type) |
+		FIELD_PREP(AD7124_FILTER_FS, cfg->odr_sel_bits);
 	return ad7124_spi_write_mask(st, AD7124_FILTER(cfg->cfg_slot),
-				     AD7124_FILTER_TYPE_MSK | AD7124_FILTER_FS_MSK,
+				     AD7124_FILTER_FILTER | AD7124_FILTER_FS,
 				     tmp, 3);
 }
 
@@ -488,7 +479,8 @@ static int ad7124_enable_channel(struct ad7124_state *st, struct ad7124_channel
 {
 	ch->cfg.live = true;
 	return ad_sd_write_reg(&st->sd, AD7124_CHANNEL(ch->nr), 2, ch->ain |
-			      AD7124_CHANNEL_SETUP(ch->cfg.cfg_slot) | AD7124_CHANNEL_EN(1));
+			       FIELD_PREP(AD7124_CHANNEL_SETUP, ch->cfg.cfg_slot) |
+			       AD7124_CHANNEL_ENABLE);
 }
 
 static int ad7124_prepare_read(struct ad7124_state *st, int address)
@@ -538,8 +530,10 @@ static int ad7124_append_status(struct ad_sigma_delta *sd, bool append)
 	unsigned int adc_control = st->adc_control;
 	int ret;
 
-	adc_control &= ~AD7124_ADC_STATUS_EN_MSK;
-	adc_control |= AD7124_ADC_STATUS_EN(append);
+	if (append)
+		adc_control |= AD7124_ADC_CONTROL_DATA_STATUS;
+	else
+		adc_control &= ~AD7124_ADC_CONTROL_DATA_STATUS;
 
 	ret = ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, adc_control);
 	if (ret < 0)
@@ -554,7 +548,7 @@ static int ad7124_disable_one(struct ad_sigma_delta *sd, unsigned int chan)
 {
 	struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
 
-	/* The relevant thing here is that AD7124_CHANNEL_EN_MSK is cleared. */
+	/* The relevant thing here is that AD7124_CHANNEL_ENABLE is cleared. */
 	return ad_sd_write_reg(&st->sd, AD7124_CHANNEL(chan), 2, 0);
 }
 
@@ -768,7 +762,7 @@ static int ad7124_update_scan_mode(struct iio_dev *indio_dev,
 		if (bit_set)
 			ret = __ad7124_set_channel(&st->sd, i);
 		else
-			ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i), AD7124_CHANNEL_EN_MSK,
+			ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i), AD7124_CHANNEL_ENABLE,
 						    0, 2);
 		if (ret < 0) {
 			mutex_unlock(&st->cfgs_lock);
@@ -809,14 +803,14 @@ static int ad7124_soft_reset(struct ad7124_state *st)
 		if (ret < 0)
 			return dev_err_probe(dev, ret, "Error reading status register\n");
 
-		if (!(readval & AD7124_STATUS_POR_FLAG_MSK))
+		if (!(readval & AD7124_STATUS_POR_FLAG))
 			break;
 
 		/* The AD7124 requires typically 2ms to power up and settle */
 		usleep_range(100, 2000);
 	} while (--timeout);
 
-	if (readval & AD7124_STATUS_POR_FLAG_MSK)
+	if (readval & AD7124_STATUS_POR_FLAG)
 		return dev_err_probe(dev, -EIO, "Soft reset failed\n");
 
 	ret = ad_sd_read_reg(&st->sd, AD7124_GAIN(0), 3, &st->gain_default);
@@ -838,8 +832,8 @@ static int ad7124_check_chip_id(struct ad7124_state *st)
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failure to read ID register\n");
 
-	chip_id = AD7124_DEVICE_ID_GET(readval);
-	silicon_rev = AD7124_SILICON_REV_GET(readval);
+	chip_id = FIELD_GET(AD7124_ID_DEVICE_ID, readval);
+	silicon_rev = FIELD_GET(AD7124_ID_SILICON_REVISION, readval);
 
 	if (chip_id != st->chip_info->chip_id)
 		return dev_err_probe(dev, -ENODEV,
@@ -867,7 +861,7 @@ static int ad7124_syscalib_locked(struct ad7124_state *st, const struct iio_chan
 	if (ch->syscalib_mode == AD7124_SYSCALIB_ZERO_SCALE) {
 		ch->cfg.calibration_offset = 0x800000;
 
-		ret = ad_sd_calibrate(&st->sd, AD7124_MODE_CAL_SYS_ZERO,
+		ret = ad_sd_calibrate(&st->sd, AD7124_ADC_CONTROL_MODE_SYS_OFFSET_CALIB,
 				      chan->address);
 		if (ret < 0)
 			return ret;
@@ -882,7 +876,7 @@ static int ad7124_syscalib_locked(struct ad7124_state *st, const struct iio_chan
 	} else {
 		ch->cfg.calibration_gain = st->gain_default;
 
-		ret = ad_sd_calibrate(&st->sd, AD7124_MODE_CAL_SYS_FULL,
+		ret = ad_sd_calibrate(&st->sd, AD7124_ADC_CONTROL_MODE_SYS_GAIN_CALIB,
 				      chan->address);
 		if (ret < 0)
 			return ret;
@@ -997,7 +991,7 @@ static bool ad7124_valid_input_select(unsigned int ain, const struct ad7124_chip
 	if (ain >= info->num_inputs && ain < 16)
 		return false;
 
-	return ain <= FIELD_MAX(AD7124_CHANNEL_AINM_MSK);
+	return ain <= FIELD_MAX(AD7124_CHANNEL_AINM);
 }
 
 static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
@@ -1062,8 +1056,8 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 					     "diff-channels property of %pfwP contains invalid data\n", child);
 
 		st->channels[channel].nr = channel;
-		st->channels[channel].ain = AD7124_CHANNEL_AINP(ain[0]) |
-						  AD7124_CHANNEL_AINM(ain[1]);
+		st->channels[channel].ain = FIELD_PREP(AD7124_CHANNEL_AINP, ain[0]) |
+			FIELD_PREP(AD7124_CHANNEL_AINM, ain[1]);
 
 		cfg = &st->channels[channel].cfg;
 		cfg->bipolar = fwnode_property_read_bool(child, "bipolar");
@@ -1089,8 +1083,8 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 	if (num_channels < AD7124_MAX_CHANNELS) {
 		st->channels[num_channels] = (struct ad7124_channel) {
 			.nr = num_channels,
-			.ain = AD7124_CHANNEL_AINP(AD7124_INPUT_TEMPSENSOR) |
-				AD7124_CHANNEL_AINM(AD7124_INPUT_AVSS),
+			.ain = FIELD_PREP(AD7124_CHANNEL_AINP, AD7124_CHANNEL_AINx_TEMPSENSOR) |
+				FIELD_PREP(AD7124_CHANNEL_AINM, AD7124_CHANNEL_AINx_AVSS),
 			.cfg = {
 				.bipolar = true,
 			},
@@ -1141,11 +1135,11 @@ static int ad7124_setup(struct ad7124_state *st)
 	}
 
 	/* Set the power mode */
-	st->adc_control &= ~AD7124_ADC_CTRL_PWR_MSK;
-	st->adc_control |= AD7124_ADC_CTRL_PWR(power_mode);
+	st->adc_control &= ~AD7124_ADC_CONTROL_POWER_MODE;
+	st->adc_control |= FIELD_PREP(AD7124_ADC_CONTROL_POWER_MODE, power_mode);
 
-	st->adc_control &= ~AD7124_ADC_CTRL_MODE_MSK;
-	st->adc_control |= AD7124_ADC_CTRL_MODE(AD_SD_MODE_IDLE);
+	st->adc_control &= ~AD7124_ADC_CONTROL_MODE;
+	st->adc_control |= FIELD_PREP(AD7124_ADC_CONTROL_MODE, AD_SD_MODE_IDLE);
 
 	mutex_init(&st->cfgs_lock);
 	INIT_KFIFO(st->live_cfgs_fifo);
@@ -1199,7 +1193,7 @@ static int __ad7124_calibrate_all(struct ad7124_state *st, struct iio_dev *indio
 		 * usual: first zero-scale then full-scale calibration.
 		 */
 		if (st->channels[i].cfg.pga_bits > 0) {
-			ret = ad_sd_calibrate(&st->sd, AD7124_MODE_CAL_INT_FULL, i);
+			ret = ad_sd_calibrate(&st->sd, AD7124_ADC_CONTROL_MODE_INT_GAIN_CALIB, i);
 			if (ret < 0)
 				return ret;
 
@@ -1216,7 +1210,7 @@ static int __ad7124_calibrate_all(struct ad7124_state *st, struct iio_dev *indio
 				return ret;
 		}
 
-		ret = ad_sd_calibrate(&st->sd, AD7124_MODE_CAL_INT_ZERO, i);
+		ret = ad_sd_calibrate(&st->sd, AD7124_ADC_CONTROL_MODE_INT_OFFSET_CALIB, i);
 		if (ret < 0)
 			return ret;
 
@@ -1245,9 +1239,9 @@ static int ad7124_calibrate_all(struct ad7124_state *st, struct iio_dev *indio_d
 	 * The resulting calibration is then also valid for high-speed, so just
 	 * restore adc_control afterwards.
 	 */
-	if (FIELD_GET(AD7124_ADC_CTRL_PWR_MSK, adc_control) >= AD7124_FULL_POWER) {
-		st->adc_control &= ~AD7124_ADC_CTRL_PWR_MSK;
-		st->adc_control |= AD7124_ADC_CTRL_PWR(AD7124_MID_POWER);
+	if (FIELD_GET(AD7124_ADC_CONTROL_POWER_MODE, adc_control) >= AD7124_FULL_POWER) {
+		st->adc_control &= ~AD7124_ADC_CONTROL_POWER_MODE;
+		st->adc_control |= FIELD_PREP(AD7124_ADC_CONTROL_POWER_MODE, AD7124_MID_POWER);
 	}
 
 	ret = __ad7124_calibrate_all(st, indio_dev);
-- 
2.47.1

Re: [PATCH v2 0/3] iio: adc: ad7124: Fix 3dB filter frequency reading

[Jonathan Cameron]

On Mon, 17 Mar 2025 12:52:46 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello,
> 
> (implicit) v1 of this patch set is available at
> https://lore.kernel.org/linux-iio/cover.1741801853.git.u.kleine-koenig@baylibre.com
> .
> 
> Changes since then:
> 
>  - Reorder patches to have the cleanup ("Make register naming
>    consistent") last
>  - Drop write support for the filter_low_pass_3db_frequency property
>    which is completely broken.
>  - trivially rebase to todays iio/togreg
> 
> I wonder if there is a way to remove the writable permission of the
> filter_low_pass_3db_frequency sysfs file instead of erroring out when a
> value is written. Hints welcome.

Unfortunately not. With a lot of hindsight that is a flaw in the way
we generate sysfs attributes. IIRC when hwmon added similar they
avoided that trap.  To retrofit it onto IIO now we'd have to have
some form of complex permissions query or duplicate all the masks
to allow r and w separately.

Jonathan


> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (3):
>   iio: adc: ad7124: Fix 3dB filter frequency reading
>   iio: adc: ad7124: Remove ability to write
>     filter_low_pass_3db_frequency
>   iio: adc: ad7124: Make register naming consistent
> 
>  drivers/iio/adc/ad7124.c | 208 ++++++++++++++++-----------------------
>  1 file changed, 84 insertions(+), 124 deletions(-)
> 
> 
> base-commit: 8dbeb413806f9f810d97d25284f585b201aa3bdc

Re: [PATCH v2 0/3] iio: adc: ad7124: Fix 3dB filter frequency reading

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

Hello Jonathan,

On Mon, Mar 17, 2025 at 07:00:31PM +0000, Jonathan Cameron wrote:
> On Mon, 17 Mar 2025 12:52:46 +0100
> Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> 
> > Hello,
> > 
> > (implicit) v1 of this patch set is available at
> > https://lore.kernel.org/linux-iio/cover.1741801853.git.u.kleine-koenig@baylibre.com
> > .
> > 
> > Changes since then:
> > 
> >  - Reorder patches to have the cleanup ("Make register naming
> >    consistent") last
> >  - Drop write support for the filter_low_pass_3db_frequency property
> >    which is completely broken.
> >  - trivially rebase to todays iio/togreg
> > 
> > I wonder if there is a way to remove the writable permission of the
> > filter_low_pass_3db_frequency sysfs file instead of erroring out when a
> > value is written. Hints welcome.
> 
> Unfortunately not. With a lot of hindsight that is a flaw in the way
> we generate sysfs attributes. IIRC when hwmon added similar they
> avoided that trap.  To retrofit it onto IIO now we'd have to have
> some form of complex permissions query or duplicate all the masks
> to allow r and w separately.

OK, fine for me, so I didn't miss anything :-)

I have another patch in my queue for ad7124. For that it would be great
to know if you intend to apply this patch set. If yes, I continue to
build on top of this stack.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] iio: adc: ad7124: Fix 3dB filter frequency reading

[Marcelo Schmitt]

Hello,

On 03/17, Uwe Kleine-König wrote:
> The sinc4 filter has a factor 0.23 between Output Data Rate and f_{3dB}
> and for sinc3 the factor is 0.272 according to the data sheets for
> ad7124-4 (Rev. E.) and ad7124-8 (Rev. F).

Potentially dumb question but, how do we get to these factors between ODR and
3dB frequency?
Looking at Table 8, Table 18, Table 28, and 
dividing values from Output Data Rate (SPS) column by respective
values from f3dB (Hz) column gives me 4.3478.
If the zero latency mode SPS values are used as numerator, the result is 1.0869
for most pairs of SPS and f3dB.

> 
> Fixes: cef2760954cf ("iio: adc: ad7124: add 3db filter")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  drivers/iio/adc/ad7124.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 3ea81a98e455..7d5d84a07cae 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -301,9 +301,9 @@ static int ad7124_get_3db_filter_freq(struct ad7124_state *st,
>  
>  	switch (st->channels[channel].cfg.filter_type) {
>  	case AD7124_SINC3_FILTER:
> -		return DIV_ROUND_CLOSEST(fadc * 230, 1000);
> +		return DIV_ROUND_CLOSEST(fadc * 272, 1000);
>  	case AD7124_SINC4_FILTER:
> -		return DIV_ROUND_CLOSEST(fadc * 262, 1000);
> +		return DIV_ROUND_CLOSEST(fadc * 230, 1000);
>  	default:
>  		return -EINVAL;
>  	}
> -- 
> 2.47.1
> 
> 

Re: [PATCH v2 2/3] iio: adc: ad7124: Remove ability to write filter_low_pass_3db_frequency

[Marcelo Schmitt]

On 03/17, Uwe Kleine-König wrote:
> There are several issues with the function that implements writing to
> the filter_low_pass_3db_frequency property:
> 
>  - The sinc3 factor should be 0.272 not 0.262 (this is fixed for the
>    reading side in the previous patch).
>  - For freq > 1 the if condition is always true so the sinc4 filter is
>    hardly ever chosen.
>  - In the nearly always taken if branch the filter is set to sinc3, but
>    the frequency is set for sinc4. (And vice versa in the else branch.)
> 
> This is broken enough to justify the claim that there isn't any serious
> user. Also it it counter-intuitive that setting the 3db frequency
> modifies the sample frequency and the filter type.

Not from engineering background but, as a Linux developer, I agree changing 3dB
frequency to set the ODR (sampling frequency) is counter-intuitive and uncommon
among other IIO sigma-delta ADC drivers.

> 
> So drop the ability to write that property.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

Re: [PATCH v2 2/3] iio: adc: ad7124: Remove ability to write filter_low_pass_3db_frequency

[Jonathan Cameron]

On Wed, 26 Mar 2025 15:35:04 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> On 03/17, Uwe Kleine-König wrote:
> > There are several issues with the function that implements writing to
> > the filter_low_pass_3db_frequency property:
> > 
> >  - The sinc3 factor should be 0.272 not 0.262 (this is fixed for the
> >    reading side in the previous patch).
> >  - For freq > 1 the if condition is always true so the sinc4 filter is
> >    hardly ever chosen.
> >  - In the nearly always taken if branch the filter is set to sinc3, but
> >    the frequency is set for sinc4. (And vice versa in the else branch.)
> > 
> > This is broken enough to justify the claim that there isn't any serious
> > user. Also it it counter-intuitive that setting the 3db frequency
> > modifies the sample frequency and the filter type.  
> 
> Not from engineering background but, as a Linux developer, I agree changing 3dB
> frequency to set the ODR (sampling frequency) is counter-intuitive and uncommon
> among other IIO sigma-delta ADC drivers.
First, ABI wise, any parameter is allowed to change any other - so we have
no firm rules on this.

Changing the filter type isn't completely silly as a side effect of changing
3db point as when we are selecting between fixed filters that may be precisely
what the user is trying to do.

Changing the sampling frequency is harder to argue.

Given it's horribly broken anyway I guess no one ever touched it and we are
fine to clean things up.

Jonathan

> 
> > 
> > So drop the ability to write that property.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>  
> 
> Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

Re: [PATCH v2 0/3] iio: adc: ad7124: Fix 3dB filter frequency reading

[Jonathan Cameron]

On Mon, 24 Mar 2025 10:44:52 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello Jonathan,
> 
> On Mon, Mar 17, 2025 at 07:00:31PM +0000, Jonathan Cameron wrote:
> > On Mon, 17 Mar 2025 12:52:46 +0100
> > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> >   
> > > Hello,
> > > 
> > > (implicit) v1 of this patch set is available at
> > > https://lore.kernel.org/linux-iio/cover.1741801853.git.u.kleine-koenig@baylibre.com
> > > .
> > > 
> > > Changes since then:
> > > 
> > >  - Reorder patches to have the cleanup ("Make register naming
> > >    consistent") last
> > >  - Drop write support for the filter_low_pass_3db_frequency property
> > >    which is completely broken.
> > >  - trivially rebase to todays iio/togreg
> > > 
> > > I wonder if there is a way to remove the writable permission of the
> > > filter_low_pass_3db_frequency sysfs file instead of erroring out when a
> > > value is written. Hints welcome.  
> > 
> > Unfortunately not. With a lot of hindsight that is a flaw in the way
> > we generate sysfs attributes. IIRC when hwmon added similar they
> > avoided that trap.  To retrofit it onto IIO now we'd have to have
> > some form of complex permissions query or duplicate all the masks
> > to allow r and w separately.  
> 
> OK, fine for me, so I didn't miss anything :-)
> 
> I have another patch in my queue for ad7124. For that it would be great
> to know if you intend to apply this patch set. If yes, I continue to
> build on top of this stack.

Sorry for slow reply.  Bunch of travel (and being a tourist on either end
of the work bit ;)

Subject to the question on patch 1 from Marcelo I'm fine with this.

I'll probably take all 3 for next merge window though as patch 1 is
a minor fix I think in something that people probably haven't really 
been using and splitting it makes for a messy cycle.

Jonathan



> 
> Best regards
> Uwe

Re: [PATCH v2 1/3] iio: adc: ad7124: Fix 3dB filter frequency reading

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1713 bytes --]

Hello,

On Wed, Mar 26, 2025 at 03:20:24PM -0300, Marcelo Schmitt wrote:
> Hello,
> 
> On 03/17, Uwe Kleine-König wrote:
> > The sinc4 filter has a factor 0.23 between Output Data Rate and f_{3dB}
> > and for sinc3 the factor is 0.272 according to the data sheets for
> > ad7124-4 (Rev. E.) and ad7124-8 (Rev. F).
> 
> Potentially dumb question but, how do we get to these factors between ODR and
> 3dB frequency?
> Looking at Table 8, Table 18, Table 28, and 
> dividing values from Output Data Rate (SPS) column by respective
> values from f3dB (Hz) column gives me 4.3478.

Using the datasheet for AD7124-4 Rev. E in Table 8 we have for example:

	ODR = 19200 SPS
	f_{3dB} = 4416 Hz

So it's either multiplying with 0.23 (as does my patch) or dividing by
4.3478260869565215 (as you found).

But having said that, a definitive formula would be nice instead of
guessing that there is a linear correlation between the columns and
determining the factor yourself. Note that in Table 10 the f_{3dB} value
corresponding to ODR = 15 SPS should be 4.08 Hz. The 5.44 Hz specified
there would be the right value for ODR = 20 SPS which is a value that
occurs in Tables 8 and 9, but not 10. :-\

> > @@ -301,9 +301,9 @@ static int ad7124_get_3db_filter_freq(struct ad7124_state *st,
> >  
> >  	switch (st->channels[channel].cfg.filter_type) {
> >  	case AD7124_SINC3_FILTER:
> > -		return DIV_ROUND_CLOSEST(fadc * 230, 1000);
> > +		return DIV_ROUND_CLOSEST(fadc * 272, 1000);
> >  	case AD7124_SINC4_FILTER:
> > -		return DIV_ROUND_CLOSEST(fadc * 262, 1000);
> > +		return DIV_ROUND_CLOSEST(fadc * 230, 1000);
> >  	default:
> >  		return -EINVAL;
> >  	}

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] iio: adc: ad7124: Fix 3dB filter frequency reading

[Marcelo Schmitt]

On 03/31, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Mar 26, 2025 at 03:20:24PM -0300, Marcelo Schmitt wrote:
> > Hello,
> > 
> > On 03/17, Uwe Kleine-König wrote:
> > > The sinc4 filter has a factor 0.23 between Output Data Rate and f_{3dB}
> > > and for sinc3 the factor is 0.272 according to the data sheets for
> > > ad7124-4 (Rev. E.) and ad7124-8 (Rev. F).
> > 
> > Potentially dumb question but, how do we get to these factors between ODR and
> > 3dB frequency?
> > Looking at Table 8, Table 18, Table 28, and 
> > dividing values from Output Data Rate (SPS) column by respective
> > values from f3dB (Hz) column gives me 4.3478.
> 
> Using the datasheet for AD7124-4 Rev. E in Table 8 we have for example:
> 
> 	ODR = 19200 SPS
> 	f_{3dB} = 4416 Hz
> 
> So it's either multiplying with 0.23 (as does my patch) or dividing by
> 4.3478260869565215 (as you found).

Got it. Thanks for clarifying that out.

> 
> But having said that, a definitive formula would be nice instead of
> guessing that there is a linear correlation between the columns and
> determining the factor yourself. Note that in Table 10 the f_{3dB} value
> corresponding to ODR = 15 SPS should be 4.08 Hz. The 5.44 Hz specified
> there would be the right value for ODR = 20 SPS which is a value that
> occurs in Tables 8 and 9, but not 10. :-\

I see. Yeah, I feel like datasheets are similar to device drivers in the sense
that they tend to get better over time. datasheets for newer parts often have
more information and better explanations compared to old ones. Anyways,

Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

Thanks,
Marcelo

Re: [PATCH v2 0/3] iio: adc: ad7124: Fix 3dB filter frequency reading

[Jonathan Cameron]

On Mon, 31 Mar 2025 11:02:14 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 24 Mar 2025 10:44:52 +0100
> Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> 
> > Hello Jonathan,
> > 
> > On Mon, Mar 17, 2025 at 07:00:31PM +0000, Jonathan Cameron wrote:  
> > > On Mon, 17 Mar 2025 12:52:46 +0100
> > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> > >     
> > > > Hello,
> > > > 
> > > > (implicit) v1 of this patch set is available at
> > > > https://lore.kernel.org/linux-iio/cover.1741801853.git.u.kleine-koenig@baylibre.com
> > > > .
> > > > 
> > > > Changes since then:
> > > > 
> > > >  - Reorder patches to have the cleanup ("Make register naming
> > > >    consistent") last
> > > >  - Drop write support for the filter_low_pass_3db_frequency property
> > > >    which is completely broken.
> > > >  - trivially rebase to todays iio/togreg
> > > > 
> > > > I wonder if there is a way to remove the writable permission of the
> > > > filter_low_pass_3db_frequency sysfs file instead of erroring out when a
> > > > value is written. Hints welcome.    
> > > 
> > > Unfortunately not. With a lot of hindsight that is a flaw in the way
> > > we generate sysfs attributes. IIRC when hwmon added similar they
> > > avoided that trap.  To retrofit it onto IIO now we'd have to have
> > > some form of complex permissions query or duplicate all the masks
> > > to allow r and w separately.    
> > 
> > OK, fine for me, so I didn't miss anything :-)
> > 
> > I have another patch in my queue for ad7124. For that it would be great
> > to know if you intend to apply this patch set. If yes, I continue to
> > build on top of this stack.  
> 
> Sorry for slow reply.  Bunch of travel (and being a tourist on either end
> of the work bit ;)
> 
> Subject to the question on patch 1 from Marcelo I'm fine with this.
> 
> I'll probably take all 3 for next merge window though as patch 1 is
> a minor fix I think in something that people probably haven't really 
> been using and splitting it makes for a messy cycle.
> 
Not sure how I managed it, but I had patch 2 already on my tree. Anyhow
backed that out and applied the whole series to the testing branch of iio.git.

Thanks,

Jonathan

> Jonathan
> 
> 
> 
> > 
> > Best regards
> > Uwe  
> 
>