[PATCH 0/4] ad7124 fixes and improvements

[PATCH 0/4] ad7124 fixes and improvements

[Dumitru Ceclan]

This patch series adds fixes and improvements in the ad7124 driver.

Fixes:
- properly compare config values
- add a delay after reset to allow chip initialization

Improvements:
- reduce the number of SPI transfers
- ensure that after probe the ADC is in idle mode
  and not continuos conversion mode

Another thing that could be considered is improving the config pop
behavior as kfifo_get() will return the least recently added config
instead of the least recently *used* one.

This could be an issue when multiple channels are using the same "old"
config and the LRU considers that one as the least recently used.

If this is considered a valid issue, I can add another patch for it.

Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
Dumitru Ceclan (4):
      iio: adc: ad7124: fix chip ID mismatch
      iio: adc: ad7124: fix config comparison
      iio: adc: ad7124: reduce the number of SPI transfers
      iio: adc: ad7124: set initial ADC mode to idle

 drivers/iio/adc/ad7124.c | 61 ++++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 30 deletions(-)
---
base-commit: 380afccc2a55e8015adae4266e8beff96ab620be
change-id: 20240731-ad7124-fix-37aec7fe0b6b

Best regards,
-- 
Dumitru Ceclan <dumitru.ceclan@analog.com>

[PATCH 1/4] iio: adc: ad7124: fix chip ID mismatch

[Dumitru Ceclan]

The ad7124_soft_reset() function has the assumption that the chip will
assert the "power-on reset" bit in the STATUS register after a software
reset without any delay. The POR bit =0 is used to check if the chip
initialization is done.

A chip ID mismatch probe error appears intermittently when the probe
continues too soon and the ID register does not contain the expected
value.

Fix by adding a 200us delay after the software reset command is issued.

Fixes: b3af341bbd96 ("iio: adc: Add ad7124 support")
Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7124.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index e7b1d517d3de..54d4c5597696 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -762,6 +762,7 @@ static int ad7124_soft_reset(struct ad7124_state *st)
 	if (ret < 0)
 		return ret;
 
+	fsleep(200);
 	timeout = 100;
 	do {
 		ret = ad_sd_read_reg(&st->sd, AD7124_STATUS, 1, &readval);

-- 
2.43.0

[PATCH 2/4] iio: adc: ad7124: fix config comparison

[Dumitru Ceclan]

The ad7124_find_similar_live_cfg() computes the compare size by
substracting the address of the cfg struct from the address of the live
field. Because the live field is the first field in the struct, the
result is 0.

Also, the memcmp() call is made from the start of the cfg struct, which
includes the live and cfg_slot fields, which are not relevant for the
comparison.

Fix by grouping the relevant fields with struct_group() and use the
size of the group to compute the compare size; make the memcmp() call
from the address of the group.

Fixes: 7b8d045e497a ("iio: adc: ad7124: allow more than 8 channels")
Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7124.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 54d4c5597696..bd323c6bd756 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -147,15 +147,18 @@ struct ad7124_chip_info {
 struct ad7124_channel_config {
 	bool live;
 	unsigned int cfg_slot;
-	enum ad7124_ref_sel refsel;
-	bool bipolar;
-	bool buf_positive;
-	bool buf_negative;
-	unsigned int vref_mv;
-	unsigned int pga_bits;
-	unsigned int odr;
-	unsigned int odr_sel_bits;
-	unsigned int filter_type;
+	/* Following fields are used to compare equality. */
+	struct_group(config_props,
+		enum ad7124_ref_sel refsel;
+		bool bipolar;
+		bool buf_positive;
+		bool buf_negative;
+		unsigned int vref_mv;
+		unsigned int pga_bits;
+		unsigned int odr;
+		unsigned int odr_sel_bits;
+		unsigned int filter_type;
+	);
 };
 
 struct ad7124_channel {
@@ -334,11 +337,12 @@ static struct ad7124_channel_config *ad7124_find_similar_live_cfg(struct ad7124_
 	ptrdiff_t cmp_size;
 	int i;
 
-	cmp_size = (u8 *)&cfg->live - (u8 *)cfg;
+	cmp_size = sizeof_field(struct ad7124_channel_config, config_props);
 	for (i = 0; i < st->num_channels; i++) {
 		cfg_aux = &st->channels[i].cfg;
 
-		if (cfg_aux->live && !memcmp(cfg, cfg_aux, cmp_size))
+		if (cfg_aux->live &&
+		    !memcmp(&cfg->config_props, &cfg_aux->config_props, cmp_size))
 			return cfg_aux;
 	}
 

-- 
2.43.0

[PATCH 3/4] iio: adc: ad7124: reduce the number of SPI transfers

[Dumitru Ceclan]

The ad7124_init_config_vref() function writes the AD7124_ADC_CONTROL
register for each channel that is configured to use the internal
reference.

The ad7124_write_config()function performs 7 SPI transfers for
configuring 2 registers: config_x and filter_x.

Reduce the number of SPI transfers:
-during the probe by only setting the st->adc_control value in
 ad7124_init_config_vref() and writing to the device only at the end of
 ad7124_setup().
-in ad7124_write_config() by grouping writes to the same register.

Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7124.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index bd323c6bd756..4d63cd5c9d04 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -382,8 +382,7 @@ static int ad7124_init_config_vref(struct ad7124_state *st, struct ad7124_channe
 		cfg->vref_mv = 2500;
 		st->adc_control &= ~AD7124_ADC_CTRL_REF_EN_MSK;
 		st->adc_control |= AD7124_ADC_CTRL_REF_EN(1);
-		return ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL,
-				      2, st->adc_control);
+		return 0;
 	default:
 		dev_err(&st->sd.spi->dev, "Invalid reference %d\n", refsel);
 		return -EINVAL;
@@ -401,24 +400,17 @@ static int ad7124_write_config(struct ad7124_state *st, struct ad7124_channel_co
 
 	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);
-	ret = ad_sd_write_reg(&st->sd, AD7124_CONFIG(cfg->cfg_slot), 2, val);
-	if (ret < 0)
-		return ret;
+	      AD7124_CONFIG_IN_BUFF(tmp) | AD7124_CONFIG_PGA(cfg->pga_bits);
 
-	tmp = AD7124_FILTER_TYPE_SEL(cfg->filter_type);
-	ret = ad7124_spi_write_mask(st, AD7124_FILTER(cfg->cfg_slot), AD7124_FILTER_TYPE_MSK,
-				    tmp, 3);
-	if (ret < 0)
-		return ret;
-
-	ret = ad7124_spi_write_mask(st, AD7124_FILTER(cfg->cfg_slot), AD7124_FILTER_FS_MSK,
-				    AD7124_FILTER_FS(cfg->odr_sel_bits), 3);
+	ret = ad_sd_write_reg(&st->sd, AD7124_CONFIG(cfg->cfg_slot), 2, val);
 	if (ret < 0)
 		return ret;
 
-	return ad7124_spi_write_mask(st, AD7124_CONFIG(cfg->cfg_slot), AD7124_CONFIG_PGA_MSK,
-				     AD7124_CONFIG_PGA(cfg->pga_bits), 2);
+	tmp = AD7124_FILTER_TYPE_SEL(cfg->filter_type) |
+	      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,
+				     tmp, 3);
 }
 
 static struct ad7124_channel_config *ad7124_pop_config(struct ad7124_state *st)
@@ -906,9 +898,6 @@ 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);
-	ret = ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
-	if (ret < 0)
-		return ret;
 
 	mutex_init(&st->cfgs_lock);
 	INIT_KFIFO(st->live_cfgs_fifo);
@@ -926,6 +915,10 @@ static int ad7124_setup(struct ad7124_state *st)
 		ad7124_set_channel_odr(st, i, 10);
 	}
 
+	ret = ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
+	if (ret < 0)
+		return ret;
+
 	return ret;
 }
 

-- 
2.43.0

[PATCH 4/4] iio: adc: ad7124: set initial ADC mode to idle

[Dumitru Ceclan]

During setup the st->adc_control is 0, which corresponds to a continuous
conversion mode. The reset value for channel 1 is to enable it. The
combined effect of these two is that the ADC will start conversions for
channel 1 without them being read.
This is not neccessarily a problem, but it is an unexpected behavior.

Set the ADC state to idle during setup to avoid this.

Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7124.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 4d63cd5c9d04..47abefd0fe5f 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -899,6 +899,9 @@ static int ad7124_setup(struct ad7124_state *st)
 	st->adc_control &= ~AD7124_ADC_CTRL_PWR_MSK;
 	st->adc_control |= AD7124_ADC_CTRL_PWR(power_mode);
 
+	st->adc_control &= ~AD7124_ADC_CTRL_MODE_MSK;
+	st->adc_control |= AD7124_ADC_CTRL_MODE(AD_SD_MODE_IDLE);
+
 	mutex_init(&st->cfgs_lock);
 	INIT_KFIFO(st->live_cfgs_fifo);
 	for (i = 0; i < st->num_channels; i++) {

-- 
2.43.0

Re: [PATCH 0/4] ad7124 fixes and improvements

[Nuno Sá]

On Wed, 2024-07-31 at 15:37 +0300, Dumitru Ceclan wrote:
> This patch series adds fixes and improvements in the ad7124 driver.
> 
> Fixes:
> - properly compare config values
> - add a delay after reset to allow chip initialization
> 
> Improvements:
> - reduce the number of SPI transfers
> - ensure that after probe the ADC is in idle mode
>   and not continuos conversion mode
> 
> Another thing that could be considered is improving the config pop
> behavior as kfifo_get() will return the least recently added config
> instead of the least recently *used* one.
> 
> This could be an issue when multiple channels are using the same "old"
> config and the LRU considers that one as the least recently used.
> 
> If this is considered a valid issue, I can add another patch for it.
> 
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---

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

> Dumitru Ceclan (4):
>       iio: adc: ad7124: fix chip ID mismatch
>       iio: adc: ad7124: fix config comparison
>       iio: adc: ad7124: reduce the number of SPI transfers
>       iio: adc: ad7124: set initial ADC mode to idle
> 
>  drivers/iio/adc/ad7124.c | 61 ++++++++++++++++++++++++-----------------------
> -
>  1 file changed, 31 insertions(+), 30 deletions(-)
> ---
> base-commit: 380afccc2a55e8015adae4266e8beff96ab620be
> change-id: 20240731-ad7124-fix-37aec7fe0b6b
> 
> Best regards,

Re: [PATCH 1/4] iio: adc: ad7124: fix chip ID mismatch

[Jonathan Cameron]

On Wed, 31 Jul 2024 15:37:22 +0300
Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:

> The ad7124_soft_reset() function has the assumption that the chip will
> assert the "power-on reset" bit in the STATUS register after a software
> reset without any delay. The POR bit =0 is used to check if the chip
> initialization is done.
> 
> A chip ID mismatch probe error appears intermittently when the probe
> continues too soon and the ID register does not contain the expected
> value.
> 
> Fix by adding a 200us delay after the software reset command is issued.
> 
> Fixes: b3af341bbd96 ("iio: adc: Add ad7124 support")
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

J
> ---
>  drivers/iio/adc/ad7124.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index e7b1d517d3de..54d4c5597696 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -762,6 +762,7 @@ static int ad7124_soft_reset(struct ad7124_state *st)
>  	if (ret < 0)
>  		return ret;
>  
> +	fsleep(200);
>  	timeout = 100;
>  	do {
>  		ret = ad_sd_read_reg(&st->sd, AD7124_STATUS, 1, &readval);
> 

Re: [PATCH 2/4] iio: adc: ad7124: fix config comparison

[Jonathan Cameron]

On Wed, 31 Jul 2024 15:37:23 +0300
Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:

> The ad7124_find_similar_live_cfg() computes the compare size by
> substracting the address of the cfg struct from the address of the live
> field. Because the live field is the first field in the struct, the
> result is 0.
> 
> Also, the memcmp() call is made from the start of the cfg struct, which
> includes the live and cfg_slot fields, which are not relevant for the
> comparison.
> 
> Fix by grouping the relevant fields with struct_group() and use the
> size of the group to compute the compare size; make the memcmp() call
> from the address of the group.
> 
> Fixes: 7b8d045e497a ("iio: adc: ad7124: allow more than 8 channels")
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
Applied to the fixes-togreg branch of iio.git and marked for stable.

Re: [PATCH 3/4] iio: adc: ad7124: reduce the number of SPI transfers

[Jonathan Cameron]

On Wed, 31 Jul 2024 15:37:24 +0300
Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:

> The ad7124_init_config_vref() function writes the AD7124_ADC_CONTROL
> register for each channel that is configured to use the internal
> reference.
> 
> The ad7124_write_config()function performs 7 SPI transfers for
> configuring 2 registers: config_x and filter_x.
> 
> Reduce the number of SPI transfers:
> -during the probe by only setting the st->adc_control value in
>  ad7124_init_config_vref() and writing to the device only at the end of
>  ad7124_setup().
> -in ad7124_write_config() by grouping writes to the same register.
> 
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
There doesn't seem to be any overlap between code touched by 1 and 2 (the fixes)
and 3 and 4 (non fixes), so I've picked 3 and 4 up now via the togreg branch of
iio.git which will be pushed out as testing for 0-day to look at.

Thanks,

Jonathan