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

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

[Uwe Kleine-König]

Hello,

here comes another fix for the ad7124: The getter function for the
filter_low_pass_3db_frequency sysfs property used wrong factors to
calculate the f_{3dB}.

The first patch is a cleanup I implemented before I noticed the issue. I
didn't switch their ordering because I was lazy. If I continue to
discover issues in the ad7124 driver at that rate, swapping for this one
fix doesn't really matter :-)

Note the setter function is still broken. And it's worse enough that I
don't know how to fix it at all. The relevant part of the function looks
as follows:

	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_FILTER_FILTER_SINC3;
		new_odr = sinc4_3db_odr;
	} else {
		new_filter = AD7124_FILTER_FILTER_SINC4;
		new_odr = sinc3_3db_odr;
	}

The issues I'm aware of in this function are:

 - the sinc3 factor should be 0.272 not 0.262 (which is fixed for the
   getter in patch #2)
 - for freq > 1 the if condition is always true
 - 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.)

Also it's unclear to me why sinc4_3db_odr > sinc3_3db_odr is the test to
decide between the two branches. Maybe something like

	if (abs(sinc4_3db_odr - current_odr) < abs(sinc3_3db_odr - current_odr))
		use_sinc4()
	else
		use_sinc3()

would make more sense.

I intend to add a filter_type property to the driver next. When this is
implemented setting the filter_low_pass_3db_frequency shouldn't be
needed any more and we can either keep the function as is (and
discourage its use) or just drop it.

Best regards
Uwe

Uwe Kleine-König (2):
  iio: adc: ad7124: Make register naming consistent
  iio: adc: ad7124: Fix 3dB filter frequency reading

 drivers/iio/adc/ad7124.c | 176 +++++++++++++++++++--------------------
 1 file changed, 85 insertions(+), 91 deletions(-)


base-commit: 8dbeb413806f9f810d97d25284f585b201aa3bdc
-- 
2.47.1

[PATCH 1/2] 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 | 174 +++++++++++++++++++--------------------
 1 file changed, 84 insertions(+), 90 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 3ea81a98e455..67d49e6184f7 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 * 230, 1000);
-	case AD7124_SINC4_FILTER:
+	case AD7124_FILTER_FILTER_SINC4:
 		return DIV_ROUND_CLOSEST(fadc * 262, 1000);
 	default:
 		return -EINVAL;
@@ -321,10 +311,10 @@ static void ad7124_set_3db_filter_freq(struct ad7124_state *st, unsigned int cha
 	sinc3_3db_odr = DIV_ROUND_CLOSEST(freq * 1000, 262);
 
 	if (sinc4_3db_odr > sinc3_3db_odr) {
-		new_filter = AD7124_SINC3_FILTER;
+		new_filter = AD7124_FILTER_FILTER_SINC3;
 		new_odr = sinc4_3db_odr;
 	} else {
-		new_filter = AD7124_SINC4_FILTER;
+		new_filter = AD7124_FILTER_FILTER_SINC4;
 		new_odr = sinc3_3db_odr;
 	}
 
@@ -413,8 +403,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);
@@ -438,18 +427,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);
 }
 
@@ -514,7 +505,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)
@@ -564,8 +556,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)
@@ -580,7 +574,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);
 }
 
@@ -802,7 +796,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);
@@ -843,14 +837,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);
@@ -872,8 +866,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,
@@ -901,7 +895,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;
@@ -916,7 +910,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;
@@ -1031,7 +1025,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,
@@ -1096,8 +1090,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");
@@ -1123,8 +1117,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,
 			},
@@ -1175,11 +1169,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);
@@ -1233,7 +1227,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;
 
@@ -1250,7 +1244,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;
 
@@ -1279,9 +1273,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

[PATCH 2/2] 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 67d49e6184f7..a3cb47ca3901 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -291,9 +291,9 @@ static int ad7124_get_3db_filter_freq(struct ad7124_state *st,
 
 	switch (st->channels[channel].cfg.filter_type) {
 	case AD7124_FILTER_FILTER_SINC3:
-		return DIV_ROUND_CLOSEST(fadc * 230, 1000);
-	case AD7124_FILTER_FILTER_SINC4:
 		return DIV_ROUND_CLOSEST(fadc * 262, 1000);
+	case AD7124_FILTER_FILTER_SINC4:
+		return DIV_ROUND_CLOSEST(fadc * 230, 1000);
 	default:
 		return -EINVAL;
 	}
-- 
2.47.1

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

[Nuno Sá]

On Wed, 2025-03-12 at 19:38 +0100, 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).
> 
> 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 67d49e6184f7..a3cb47ca3901 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -291,9 +291,9 @@ static int ad7124_get_3db_filter_freq(struct ad7124_state *st,
>  
>  	switch (st->channels[channel].cfg.filter_type) {
>  	case AD7124_FILTER_FILTER_SINC3:
> -		return DIV_ROUND_CLOSEST(fadc * 230, 1000);
> -	case AD7124_FILTER_FILTER_SINC4:
>  		return DIV_ROUND_CLOSEST(fadc * 262, 1000);

I wonder if we shouldn't fix the sinc3 factor as well? Or at the very least mention
in the commit message why we're not doing it now. Otherwise it's confusing and raises
questions to state the proper factor in the commit and then look at this diff.

- Nuno Sá

> +	case AD7124_FILTER_FILTER_SINC4:
> +		return DIV_ROUND_CLOSEST(fadc * 230, 1000);
>  	default:
>  		return -EINVAL;
>  	}

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

[Uwe Kleine-König]

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

On Fri, Mar 14, 2025 at 08:01:13AM +0000, Nuno Sá wrote:
> On Wed, 2025-03-12 at 19:38 +0100, 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).
> > 
> > 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 67d49e6184f7..a3cb47ca3901 100644
> > --- a/drivers/iio/adc/ad7124.c
> > +++ b/drivers/iio/adc/ad7124.c
> > @@ -291,9 +291,9 @@ static int ad7124_get_3db_filter_freq(struct ad7124_state *st,
> >  
> >  	switch (st->channels[channel].cfg.filter_type) {
> >  	case AD7124_FILTER_FILTER_SINC3:
> > -		return DIV_ROUND_CLOSEST(fadc * 230, 1000);
> > -	case AD7124_FILTER_FILTER_SINC4:
> >  		return DIV_ROUND_CLOSEST(fadc * 262, 1000);
> 
> I wonder if we shouldn't fix the sinc3 factor as well? Or at the very least mention
> in the commit message why we're not doing it now. Otherwise it's confusing and raises
> questions to state the proper factor in the commit and then look at this diff.

Huh, I intended to fix that, but it seems I sent out an older version of
my patch :-\. Will send a v2. Thanks for catching that.

Best regards
Uwe

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

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

[Nuno Sá]

On Wed, 2025-03-12 at 19:38 +0100, Uwe Kleine-König wrote:
> Hello,
> 
> here comes another fix for the ad7124: The getter function for the
> filter_low_pass_3db_frequency sysfs property used wrong factors to
> calculate the f_{3dB}.
> 
> The first patch is a cleanup I implemented before I noticed the issue. I
> didn't switch their ordering because I was lazy. If I continue to
> discover issues in the ad7124 driver at that rate, swapping for this one
> fix doesn't really matter :-)
> 
> Note the setter function is still broken. And it's worse enough that I
> don't know how to fix it at all. The relevant part of the function looks
> as follows:
> 
> 	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_FILTER_FILTER_SINC3;
> 		new_odr = sinc4_3db_odr;
> 	} else {
> 		new_filter = AD7124_FILTER_FILTER_SINC4;
> 		new_odr = sinc3_3db_odr;
> 	}
> 
> The issues I'm aware of in this function are:
> 
>  - the sinc3 factor should be 0.272 not 0.262 (which is fixed for the
>    getter in patch #2)
>  - for freq > 1 the if condition is always true
>  - 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.)
> 

Ouch...

> Also it's unclear to me why sinc4_3db_odr > sinc3_3db_odr is the test to
> decide between the two branches. Maybe something like
> 
> 	if (abs(sinc4_3db_odr - current_odr) < abs(sinc3_3db_odr - current_odr))
> 		use_sinc4()
> 	else
> 		use_sinc3()
> 
> would make more sense.
> 

I think the below is indeed the proper solution. I know that removing an attr like
tis breaks ABI but since this interface is fairly broken anyways maybe it makes it
more acceptable. Anyways, just a minor comment for the series. With it:

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

> I intend to add a filter_type property to the driver next. When this is
> implemented setting the filter_low_pass_3db_frequency shouldn't be
> needed any more and we can either keep the function as is (and
> discourage its use) or just drop it.
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (2):
>   iio: adc: ad7124: Make register naming consistent
>   iio: adc: ad7124: Fix 3dB filter frequency reading
> 
>  drivers/iio/adc/ad7124.c | 176 +++++++++++++++++++--------------------
>  1 file changed, 85 insertions(+), 91 deletions(-)
> 
> 
> base-commit: 8dbeb413806f9f810d97d25284f585b201aa3bdc

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

[Jonathan Cameron]

On Wed, 12 Mar 2025 19:38:36 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello,
> 
> here comes another fix for the ad7124: The getter function for the
> filter_low_pass_3db_frequency sysfs property used wrong factors to
> calculate the f_{3dB}.
> 
> The first patch is a cleanup I implemented before I noticed the issue. I
> didn't switch their ordering because I was lazy. If I continue to
> discover issues in the ad7124 driver at that rate, swapping for this one
> fix doesn't really matter :-)

Hmm. Even so, please swap if not too ahrd.

> 
> Note the setter function is still broken. And it's worse enough that I
> don't know how to fix it at all. The relevant part of the function looks
> as follows:
> 
> 	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_FILTER_FILTER_SINC3;
> 		new_odr = sinc4_3db_odr;
> 	} else {
> 		new_filter = AD7124_FILTER_FILTER_SINC4;
> 		new_odr = sinc3_3db_odr;
> 	}
> 
> The issues I'm aware of in this function are:
> 
>  - the sinc3 factor should be 0.272 not 0.262 (which is fixed for the
>    getter in patch #2)
>  - for freq > 1 the if condition is always true
>  - 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.)
> 
> Also it's unclear to me why sinc4_3db_odr > sinc3_3db_odr is the test to
> decide between the two branches. Maybe something like
> 
> 	if (abs(sinc4_3db_odr - current_odr) < abs(sinc3_3db_odr - current_odr))
> 		use_sinc4()
> 	else
> 		use_sinc3()
> 
> would make more sense.
> 
> I intend to add a filter_type property to the driver next. When this is
> implemented setting the filter_low_pass_3db_frequency shouldn't be
> needed any more and we can either keep the function as is (and
> discourage its use) or just drop it.

If it is currently broken I'm less fussed about dropping the ABI than I would be
for even correct but useless ABI.  This falls into the category of unlikely
anyone will noticed as they should have noticed it wasn't working if
they were using it!

Jonathan

> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (2):
>   iio: adc: ad7124: Make register naming consistent
>   iio: adc: ad7124: Fix 3dB filter frequency reading
> 
>  drivers/iio/adc/ad7124.c | 176 +++++++++++++++++++--------------------
>  1 file changed, 85 insertions(+), 91 deletions(-)
> 
> 
> base-commit: 8dbeb413806f9f810d97d25284f585b201aa3bdc