[PATCH v2 0/6] iio: adc: ad{4130,7124,7173}: A few fixes and ad7124 calibration

[PATCH v2 0/6] iio: adc: ad{4130,7124,7173}: A few fixes and ad7124 calibration

[Uwe Kleine-König]

Hello,

this v2 series is a rework of two series:

	https://lore.kernel.org/iio/20250212105322.10243-5-u.kleine-koenig@baylibre.com
	https://lore.kernel.org/iio/cover.1738258777.git.u.kleine-koenig@baylibre.com

; they overlap thematically and so I put them together in a single
series.

Changes since their (implicit) v1:

 - Use static_assert instead of BUILD_BUG, add more comments to the "Fix
   comparison" patches
 - Make ad7124 internal calibration actually compile
 - Sort the fix to the front of the series and add a Fixes: tag
 - Implement system calibration
 - More comments and commit log improvements

The "Fix comparison" patches trigger a checkpatch warning because the
struct members are considered wrongly indented. For me the indention
looks right, but I don't feel strong here and happily adapt if
requested.

Best regards
Uwe

Uwe Kleine-König (6):
  iio: adc: ad_sigma_delta: Disable channel after calibration
  iio: adc: ad4130: Fix comparison of channel setups
  iio: adc: ad7124: Fix comparison of channel configs
  iio: adc: ad7173: Fix comparison of channel configs
  iio: adc: ad7124: Implement internal calibration at probe time
  iio: adc: ad7124: Implement system calibration

 drivers/iio/adc/ad4130.c         |  41 ++++-
 drivers/iio/adc/ad7124.c         | 293 ++++++++++++++++++++++++++++---
 drivers/iio/adc/ad7173.c         |  25 ++-
 drivers/iio/adc/ad_sigma_delta.c |   1 +
 4 files changed, 331 insertions(+), 29 deletions(-)

base-commit: ac856912f210bcff6a1cf8cf9cb2f6a1dfe85798
-- 
2.47.1

[PATCH v2 1/6] iio: adc: ad_sigma_delta: Disable channel after calibration

[Uwe Kleine-König]

The function ad_sd_calibrate() enables the channel to calibrate at
function entry but doesn't disable it on exit. This is problematic
because if two (or more) channels are calibrated in a row, the second
calibration isn't executed as intended as the first (still enabled)
channel is recalibrated and after the first irq (i.e. when the
calibration of the first channel completed) the calibration is aborted.

This currently affects ad7173 only, as the other drivers using
ad_sd_calibrate() never have more than one channel enabled at a time.

To fix this, disable the calibrated channel after calibration.

Fixes: 031bdc8aee01 ("iio: adc: ad7173: add calibration support")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad_sigma_delta.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 10e635fc4fa4..fbe241b90f37 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -339,6 +339,7 @@ int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta,
 out:
 	sigma_delta->keep_cs_asserted = false;
 	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
+	ad_sigma_delta_disable_one(sigma_delta, channel);
 	sigma_delta->bus_locked = false;
 	spi_bus_unlock(sigma_delta->spi->controller);
 
-- 
2.47.1

[PATCH v2 2/6] iio: adc: ad4130: Fix comparison of channel setups

[Uwe Kleine-König]

Checking the binary representation of two structs (of the same type)
for equality doesn't have the same semantic as comparing all members for
equality. The former might find a difference where the latter doesn't in
the presence of padding or when ambiguous types like float or bool are
involved. (Floats typically have different representations for single
values, like -0.0 vs +0.0, or 0.5 * 2² vs 0.25 * 2³. The type bool has
at least 8 bits and the raw values 1 and 2 (probably) both evaluate to
true, but memcmp finds a difference.)

When searching for a channel that already has the configuration we need,
the comparison by member is the one that is needed.

Convert the comparison accordingly to compare the members one after
another. Also add a BUILD_BUG guard to (somewhat) ensure that when
struct ad4130_setup_info is expanded, the comparison is adapted, too.

Fixes: 62094060cf3a ("iio: adc: ad4130: add AD4130 driver")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad4130.c | 41 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad4130.c b/drivers/iio/adc/ad4130.c
index acc241cc0a7a..b4eb3065ae34 100644
--- a/drivers/iio/adc/ad4130.c
+++ b/drivers/iio/adc/ad4130.c
@@ -223,6 +223,10 @@ enum ad4130_pin_function {
 	AD4130_PIN_FN_VBIAS = BIT(3),
 };
 
+/*
+ * If you make adaptions in this struct, you most likely also have to adapt
+ * ad4130_setup_info_eq(), too.
+ */
 struct ad4130_setup_info {
 	unsigned int			iout0_val;
 	unsigned int			iout1_val;
@@ -591,6 +595,40 @@ static irqreturn_t ad4130_irq_handler(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
+static bool ad4130_setup_info_eq(struct ad4130_setup_info *a,
+				 struct ad4130_setup_info *b)
+{
+	/*
+	 * This is just to make sure that the comparison is adapted after
+	 * struct ad4130_setup_info was changed.
+	 */
+	static_assert(sizeof(*a) ==
+		      sizeof(struct {
+				     unsigned int iout0_val;
+				     unsigned int iout1_val;
+				     unsigned int burnout;
+				     unsigned int pga;
+				     unsigned int fs;
+				     u32 ref_sel;
+				     enum ad4130_filter_mode filter_mode;
+				     bool ref_bufp;
+				     bool ref_bufm;
+			     }));
+
+	if (a->iout0_val != b->iout0_val ||
+	    a->iout1_val != b->iout1_val ||
+	    a->burnout != b->burnout ||
+	    a->pga != b->pga ||
+	    a->fs != b->fs ||
+	    a->ref_sel != b->ref_sel ||
+	    a->filter_mode != b->filter_mode ||
+	    a->ref_bufp != b->ref_bufp ||
+	    a->ref_bufm != b->ref_bufm)
+		return false;
+
+	return true;
+}
+
 static int ad4130_find_slot(struct ad4130_state *st,
 			    struct ad4130_setup_info *target_setup_info,
 			    unsigned int *slot, bool *overwrite)
@@ -604,8 +642,7 @@ static int ad4130_find_slot(struct ad4130_state *st,
 		struct ad4130_slot_info *slot_info = &st->slots_info[i];
 
 		/* Immediately accept a matching setup info. */
-		if (!memcmp(target_setup_info, &slot_info->setup,
-			    sizeof(*target_setup_info))) {
+		if (ad4130_setup_info_eq(target_setup_info, &slot_info->setup)) {
 			*slot = i;
 			return 0;
 		}
-- 
2.47.1

Re: [PATCH v2 2/6] iio: adc: ad4130: Fix comparison of channel setups

[David Lechner]

On 2/18/25 12:31 PM, Uwe Kleine-König wrote:
> Checking the binary representation of two structs (of the same type)
> for equality doesn't have the same semantic as comparing all members for
> equality. The former might find a difference where the latter doesn't in
> the presence of padding or when ambiguous types like float or bool are
> involved. (Floats typically have different representations for single
> values, like -0.0 vs +0.0, or 0.5 * 2² vs 0.25 * 2³. The type bool has
> at least 8 bits and the raw values 1 and 2 (probably) both evaluate to
> true, but memcmp finds a difference.)
> 
> When searching for a channel that already has the configuration we need,
> the comparison by member is the one that is needed.
> 
> Convert the comparison accordingly to compare the members one after
> another. Also add a BUILD_BUG guard to (somewhat) ensure that when

Now it is static_assert instead of BUILD_BUG.

> struct ad4130_setup_info is expanded, the comparison is adapted, too.
> 
> Fixes: 62094060cf3a ("iio: adc: ad4130: add AD4130 driver")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---

Re: [PATCH v2 2/6] iio: adc: ad4130: Fix comparison of channel setups

[Andy Shevchenko]

On Tue, Feb 18, 2025 at 8:31 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
>
> Checking the binary representation of two structs (of the same type)
> for equality doesn't have the same semantic as comparing all members for
> equality. The former might find a difference where the latter doesn't in
> the presence of padding or when ambiguous types like float or bool are
> involved. (Floats typically have different representations for single
> values, like -0.0 vs +0.0, or 0.5 * 2² vs 0.25 * 2³. The type bool has
> at least 8 bits and the raw values 1 and 2 (probably) both evaluate to
> true, but memcmp finds a difference.)
>
> When searching for a channel that already has the configuration we need,
> the comparison by member is the one that is needed.
>
> Convert the comparison accordingly to compare the members one after
> another. Also add a BUILD_BUG guard to (somewhat) ensure that when
> struct ad4130_setup_info is expanded, the comparison is adapted, too.

...

> +/*
> + * If you make adaptions in this struct, you most likely also have to adapt

adaptations?


> + * ad4130_setup_info_eq(), too.
> + */

...

> +static bool ad4130_setup_info_eq(struct ad4130_setup_info *a,
> +                                struct ad4130_setup_info *b)
> +{
> +       /*
> +        * This is just to make sure that the comparison is adapted after
> +        * struct ad4130_setup_info was changed.
> +        */
> +       static_assert(sizeof(*a) ==
> +                     sizeof(struct {
> +                                    unsigned int iout0_val;
> +                                    unsigned int iout1_val;
> +                                    unsigned int burnout;
> +                                    unsigned int pga;
> +                                    unsigned int fs;
> +                                    u32 ref_sel;
> +                                    enum ad4130_filter_mode filter_mode;
> +                                    bool ref_bufp;
> +                                    bool ref_bufm;
> +                            }));

This can be moved out of the function, but in any case it should not
affect code generation I believe.

> +       if (a->iout0_val != b->iout0_val ||
> +           a->iout1_val != b->iout1_val ||
> +           a->burnout != b->burnout ||
> +           a->pga != b->pga ||
> +           a->fs != b->fs ||
> +           a->ref_sel != b->ref_sel ||
> +           a->filter_mode != b->filter_mode ||
> +           a->ref_bufp != b->ref_bufp ||
> +           a->ref_bufm != b->ref_bufm)
> +               return false;
> +
> +       return true;
> +}

...

>                 struct ad4130_slot_info *slot_info = &st->slots_info[i];
>
>                 /* Immediately accept a matching setup info. */
> -               if (!memcmp(target_setup_info, &slot_info->setup,
> -                           sizeof(*target_setup_info))) {
> +               if (ad4130_setup_info_eq(target_setup_info, &slot_info->setup)) {

I don't remember if we discussed the idea of using crc32 to compare
the data structures, like it's done for PLD data in USB Type-C cases.
https://elixir.bootlin.com/linux/v6.14-rc3/C/ident/pld_crc

>                         *slot = i;
>                         return 0;
>                 }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/6] iio: adc: ad4130: Fix comparison of channel setups

[Uwe Kleine-König]

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

On Wed, Feb 19, 2025 at 10:53:54AM +0200, Andy Shevchenko wrote:
> On Tue, Feb 18, 2025 at 8:31 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> >
> > Checking the binary representation of two structs (of the same type)
> > for equality doesn't have the same semantic as comparing all members for
> > equality. The former might find a difference where the latter doesn't in
> > the presence of padding or when ambiguous types like float or bool are
> > involved. (Floats typically have different representations for single
> > values, like -0.0 vs +0.0, or 0.5 * 2² vs 0.25 * 2³. The type bool has
> > at least 8 bits and the raw values 1 and 2 (probably) both evaluate to
> > true, but memcmp finds a difference.)
> >
> > When searching for a channel that already has the configuration we need,
> > the comparison by member is the one that is needed.
> >
> > Convert the comparison accordingly to compare the members one after
> > another. Also add a BUILD_BUG guard to (somewhat) ensure that when
> > struct ad4130_setup_info is expanded, the comparison is adapted, too.
> 
> ...
> 
> > +/*
> > + * If you make adaptions in this struct, you most likely also have to adapt
> 
> adaptations?

ack.

> > +static bool ad4130_setup_info_eq(struct ad4130_setup_info *a,
> > +                                struct ad4130_setup_info *b)
> > +{
> > +       /*
> > +        * This is just to make sure that the comparison is adapted after
> > +        * struct ad4130_setup_info was changed.
> > +        */
> > +       static_assert(sizeof(*a) ==
> > +                     sizeof(struct {
> > +                                    unsigned int iout0_val;
> > +                                    unsigned int iout1_val;
> > +                                    unsigned int burnout;
> > +                                    unsigned int pga;
> > +                                    unsigned int fs;
> > +                                    u32 ref_sel;
> > +                                    enum ad4130_filter_mode filter_mode;
> > +                                    bool ref_bufp;
> > +                                    bool ref_bufm;
> > +                            }));
> 
> This can be moved out of the function, but in any case it should not
> affect code generation I believe.

It can, but I like having it near its usage.
 
> > +       if (a->iout0_val != b->iout0_val ||
> > +           a->iout1_val != b->iout1_val ||
> > +           a->burnout != b->burnout ||
> > +           a->pga != b->pga ||
> > +           a->fs != b->fs ||
> > +           a->ref_sel != b->ref_sel ||
> > +           a->filter_mode != b->filter_mode ||
> > +           a->ref_bufp != b->ref_bufp ||
> > +           a->ref_bufm != b->ref_bufm)
> > +               return false;
> > +
> > +       return true;
> > +}
> 
> ...
> 
> >                 struct ad4130_slot_info *slot_info = &st->slots_info[i];
> >
> >                 /* Immediately accept a matching setup info. */
> > -               if (!memcmp(target_setup_info, &slot_info->setup,
> > -                           sizeof(*target_setup_info))) {
> > +               if (ad4130_setup_info_eq(target_setup_info, &slot_info->setup)) {
> 
> I don't remember if we discussed the idea of using crc32 to compare
> the data structures, like it's done for PLD data in USB Type-C cases.
> https://elixir.bootlin.com/linux/v6.14-rc3/C/ident/pld_crc

crc32 works on the binary representation again, right? That's what this
patch targets to remove.

Best regards
Uwe

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

[PATCH v2 3/6] iio: adc: ad7124: Fix comparison of channel configs

[Uwe Kleine-König]

Checking the binary representation of two structs (of the same type)
for equality doesn't have the same semantic as comparing all members for
equality. The former might find a difference where the latter doesn't in
the presence of padding or when ambiguous types like float or bool are
involved. (Floats typically have different representations for single
values, like -0.0 vs +0.0, or 0.5 * 2² vs 0.25 * 2³. The type bool has
at least 8 bits and the raw values 1 and 2 (probably) both evaluate to
true, but memcmp finds a difference.)

When searching for a channel that already has the configuration we need,
the comparison by member is the one that is needed.

Convert the comparison accordingly to compare the members one after
another. Also add a static_assert guard to (somewhat) ensure that when
struct ad7124_channel_config::config_props is expanded, the comparison
is adapted, too.

This issue is somewhat theoretic, but using memcmp() on a struct is a
bad pattern that is worth fixing.

Fixes: 7b8d045e497a ("iio: adc: ad7124: allow more than 8 channels")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 6bc418d38820..4f253c4831fa 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -151,7 +151,11 @@ struct ad7124_chip_info {
 struct ad7124_channel_config {
 	bool live;
 	unsigned int cfg_slot;
-	/* Following fields are used to compare equality. */
+	/*
+	 * Following fields are used to compare for equality. If you
+	 * make adaptions in it, you most likely also have to adapt
+	 * ad7124_find_similar_live_cfg(), too.
+	 */
 	struct_group(config_props,
 		enum ad7124_ref_sel refsel;
 		bool bipolar;
@@ -338,15 +342,38 @@ static struct ad7124_channel_config *ad7124_find_similar_live_cfg(struct ad7124_
 								  struct ad7124_channel_config *cfg)
 {
 	struct ad7124_channel_config *cfg_aux;
-	ptrdiff_t cmp_size;
 	int i;
 
-	cmp_size = sizeof_field(struct ad7124_channel_config, config_props);
+	/*
+	 * This is just to make sure that the comparison is adapted after
+	 * struct ad7124_channel_config was changed.
+	 */
+	static_assert(sizeof_field(struct ad7124_channel_config, config_props) ==
+		      sizeof(struct {
+				     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;
+			     }));
+
 	for (i = 0; i < st->num_channels; i++) {
 		cfg_aux = &st->channels[i].cfg;
 
 		if (cfg_aux->live &&
-		    !memcmp(&cfg->config_props, &cfg_aux->config_props, cmp_size))
+		    cfg->refsel == cfg_aux->refsel &&
+		    cfg->bipolar == cfg_aux->bipolar &&
+		    cfg->buf_positive == cfg_aux->buf_positive &&
+		    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)
 			return cfg_aux;
 	}
 
-- 
2.47.1

[PATCH v2 4/6] iio: adc: ad7173: Fix comparison of channel configs

[Uwe Kleine-König]

Checking the binary representation of two structs (of the same type)
for equality doesn't have the same semantic as comparing all members for
equality. The former might find a difference where the latter doesn't in
the presence of padding or when ambiguous types like float or bool are
involved. (Floats typically have different representations for single
values, like -0.0 vs +0.0, or 0.5 * 2² vs 0.25 * 2³. The type bool has
at least 8 bits and the raw values 1 and 2 (probably) both evaluate to
true, but memcmp finds a difference.)

When searching for a channel that already has the configuration we need,
the comparison by member is the one that is needed.

Convert the comparison accordingly to compare the members one after
another. Also add a static_assert guard to (somewhat) ensure that when
struct ad7173_channel_config::config_props is expanded, the comparison
is adapted, too.

This issue is somewhat theoretic, but using memcmp() on a struct is a
bad pattern that is worth fixing.

Fixes: 76a1e6a42802 ("iio: adc: ad7173: add AD7173 driver")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7173.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 54f9cc5a89f5..59cbb52f6f38 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -197,7 +197,11 @@ struct ad7173_channel_config {
 	u8 cfg_slot;
 	bool live;
 
-	/* Following fields are used to compare equality. */
+	/*
+	 * Following fields are used to compare equality. If you
+	 * make adaptions in it, you most likely also have to adapt
+	 * ad7173_find_live_config(), too.
+	 */
 	struct_group(config_props,
 		bool bipolar;
 		bool input_buf;
@@ -568,15 +572,28 @@ static struct ad7173_channel_config *
 ad7173_find_live_config(struct ad7173_state *st, struct ad7173_channel_config *cfg)
 {
 	struct ad7173_channel_config *cfg_aux;
-	ptrdiff_t cmp_size;
 	int i;
 
-	cmp_size = sizeof_field(struct ad7173_channel_config, config_props);
+	/*
+	 * This is just to make sure that the comparison is adapted after
+	 * struct ad7173_channel_config was changed.
+	 */
+	static_assert(sizeof_field(struct ad7173_channel_config, config_props) ==
+		      sizeof(struct {
+				     bool bipolar;
+				     bool input_buf;
+				     u8 odr;
+				     u8 ref_sel;
+			     }));
+
 	for (i = 0; i < st->num_channels; i++) {
 		cfg_aux = &st->channels[i].cfg;
 
 		if (cfg_aux->live &&
-		    !memcmp(&cfg->config_props, &cfg_aux->config_props, cmp_size))
+		    cfg->bipolar == cfg_aux->bipolar &&
+		    cfg->input_buf == cfg_aux->input_buf &&
+		    cfg->odr == cfg_aux->odr &&
+		    cfg->ref_sel == cfg_aux->ref_sel)
 			return cfg_aux;
 	}
 	return NULL;
-- 
2.47.1

[PATCH v2 5/6] iio: adc: ad7124: Implement internal calibration at probe time

[Uwe Kleine-König]

Use the calibration function provided by the ad_sigma_delta shim to
calibrate all channels at probe time.

For measurements with gain 1 (i.e. if CONFIG_x.PGA = 0) full-scale
calibrations are not supported and the reset default value of the GAIN
register is supposed to be used then.

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

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 4f253c4831fa..5c2e5a518af3 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -53,6 +53,11 @@
 #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_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)
@@ -166,6 +171,8 @@ struct ad7124_channel_config {
 		unsigned int odr;
 		unsigned int odr_sel_bits;
 		unsigned int filter_type;
+		unsigned int calibration_offset;
+		unsigned int calibration_gain;
 	);
 };
 
@@ -186,6 +193,12 @@ struct ad7124_state {
 	unsigned int num_channels;
 	struct mutex cfgs_lock; /* lock for configs access */
 	unsigned long cfg_slots_status; /* bitmap with slot status (1 means it is used) */
+
+	/*
+	 * Stores the power-on reset value for the GAIN(x) registers which are
+	 * needed for measurements at gain 1 (i.e. CONFIG(x).PGA == 0)
+	 */
+	unsigned int gain_default;
 	DECLARE_KFIFO(live_cfgs_fifo, struct ad7124_channel_config *, AD7124_MAX_CONFIGS);
 };
 
@@ -359,6 +372,8 @@ static struct ad7124_channel_config *ad7124_find_similar_live_cfg(struct ad7124_
 				     unsigned int odr;
 				     unsigned int odr_sel_bits;
 				     unsigned int filter_type;
+				     unsigned int calibration_offset;
+				     unsigned int calibration_gain;
 			     }));
 
 	for (i = 0; i < st->num_channels; i++) {
@@ -373,7 +388,9 @@ static struct ad7124_channel_config *ad7124_find_similar_live_cfg(struct ad7124_
 		    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->filter_type == cfg_aux->filter_type &&
+		    cfg->calibration_offset == cfg_aux->calibration_offset &&
+		    cfg->calibration_gain == cfg_aux->calibration_gain)
 			return cfg_aux;
 	}
 
@@ -429,6 +446,14 @@ static int ad7124_write_config(struct ad7124_state *st, struct ad7124_channel_co
 
 	cfg->cfg_slot = cfg_slot;
 
+	ret = ad_sd_write_reg(&st->sd, AD7124_OFFSET(cfg->cfg_slot), 3, cfg->calibration_offset);
+	if (ret)
+		return ret;
+
+	ret = ad_sd_write_reg(&st->sd, AD7124_GAIN(cfg->cfg_slot), 3, cfg->calibration_gain);
+	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);
@@ -835,13 +860,22 @@ static int ad7124_soft_reset(struct ad7124_state *st)
 			return dev_err_probe(dev, ret, "Error reading status register\n");
 
 		if (!(readval & AD7124_STATUS_POR_FLAG_MSK))
-			return 0;
+			break;
 
 		/* The AD7124 requires typically 2ms to power up and settle */
 		usleep_range(100, 2000);
 	} while (--timeout);
 
-	return dev_err_probe(dev, -EIO, "Soft reset failed\n");
+	if (readval & AD7124_STATUS_POR_FLAG_MSK)
+		return dev_err_probe(dev, -EIO, "Soft reset failed\n");
+
+	ret = ad_sd_read_reg(&st->sd, AD7124_GAIN(0), 3, &st->gain_default);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Error reading gain register\n");
+
+	dev_dbg(dev, "Reset value of GAIN register is 0x%x\n", st->gain_default);
+
+	return 0;
 }
 
 static int ad7124_check_chip_id(struct ad7124_state *st)
@@ -1054,6 +1088,81 @@ static int ad7124_setup(struct ad7124_state *st)
 	return ret;
 }
 
+static int ad7124_calibrate_all(struct ad7124_state *st, struct iio_dev *indio_dev)
+{
+	struct device *dev = &st->sd.spi->dev;
+	int ret, i;
+	unsigned int adc_control = st->adc_control;
+
+	/*
+	 * Calibration isn't supported at full power, so speed down a bit.
+	 * Setting .adc_control is enough here because the control register is
+	 * written as part of ad_sd_calibrate() -> ad_sigma_delta_set_mode().
+	 */
+	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);
+	}
+
+	for (i = 0; i < st->num_channels; i++) {
+
+		if (indio_dev->channels[i].type != IIO_VOLTAGE)
+			continue;
+
+		/*
+		 * For calibration the OFFSET register should hold its reset default
+		 * value. For the GAIN register there is no such requirement but
+		 * for gain 1 it should hold the reset default value, too. So to
+		 * simplify matters use the reset default value for both.
+		 */
+		st->channels[i].cfg.calibration_offset = 0x800000;
+		st->channels[i].cfg.calibration_gain = st->gain_default;
+
+		/*
+		 * Full-scale calibration isn't supported at gain 1, so skip in
+		 * that case. Note that untypically full-scale calibration has
+		 * to happen before zero-scale calibration. This only applies to
+		 * the internal calibration. For system calibration it's as
+		 * 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);
+			if (ret < 0)
+				goto out;
+
+			/*
+			 * read out the resulting value of GAIN
+			 * after full-scale calibration because the next
+			 * ad_sd_calibrate() call overwrites this via
+			 * ad_sigma_delta_set_channel() -> ad7124_set_channel()
+			 * ... -> ad7124_enable_channel().
+			 */
+			ret = ad_sd_read_reg(&st->sd, AD7124_GAIN(st->channels[i].cfg.cfg_slot), 3,
+					     &st->channels[i].cfg.calibration_gain);
+			if (ret < 0)
+				goto out;
+		}
+
+		ret = ad_sd_calibrate(&st->sd, AD7124_MODE_CAL_INT_ZERO, i);
+		if (ret < 0)
+			goto out;
+
+		ret = ad_sd_read_reg(&st->sd, AD7124_OFFSET(st->channels[i].cfg.cfg_slot), 3,
+				     &st->channels[i].cfg.calibration_offset);
+		if (ret < 0)
+			goto out;
+
+		dev_dbg(dev, "offset and gain for channel %d = 0x%x + 0x%x\n", i,
+			st->channels[i].cfg.calibration_offset,
+			st->channels[i].cfg.calibration_gain);
+	}
+
+out:
+	st->adc_control = adc_control;
+
+	return ret;
+}
+
 static void ad7124_reg_disable(void *r)
 {
 	regulator_disable(r);
@@ -1132,6 +1241,10 @@ static int ad7124_probe(struct spi_device *spi)
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to setup triggers\n");
 
+	ret = ad7124_calibrate_all(st, indio_dev);
+	if (ret)
+		return ret;
+
 	ret = devm_iio_device_register(&spi->dev, indio_dev);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to register iio device\n");
-- 
2.47.1

Re: [PATCH v2 5/6] iio: adc: ad7124: Implement internal calibration at probe time

[Jonathan Cameron]

On Tue, 18 Feb 2025 19:31:12 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Use the calibration function provided by the ad_sigma_delta shim to
> calibrate all channels at probe time.
> 
> For measurements with gain 1 (i.e. if CONFIG_x.PGA = 0) full-scale
> calibrations are not supported and the reset default value of the GAIN
> register is supposed to be used then.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
one passing comment inline. 

> +static int ad7124_calibrate_all(struct ad7124_state *st, struct iio_dev *indio_dev)
> +{
> +	struct device *dev = &st->sd.spi->dev;
> +	int ret, i;
> +	unsigned int adc_control = st->adc_control;
Maybe factor out from here...
> +
> +	/*
> +	 * Calibration isn't supported at full power, so speed down a bit.
> +	 * Setting .adc_control is enough here because the control register is
> +	 * written as part of ad_sd_calibrate() -> ad_sigma_delta_set_mode().
> +	 */
> +	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);
> +	}
> +
> +	for (i = 0; i < st->num_channels; i++) {
> +
> +		if (indio_dev->channels[i].type != IIO_VOLTAGE)
> +			continue;
> +
> +		/*
> +		 * For calibration the OFFSET register should hold its reset default
> +		 * value. For the GAIN register there is no such requirement but
> +		 * for gain 1 it should hold the reset default value, too. So to
> +		 * simplify matters use the reset default value for both.
> +		 */
> +		st->channels[i].cfg.calibration_offset = 0x800000;
> +		st->channels[i].cfg.calibration_gain = st->gain_default;
> +
> +		/*
> +		 * Full-scale calibration isn't supported at gain 1, so skip in
> +		 * that case. Note that untypically full-scale calibration has
> +		 * to happen before zero-scale calibration. This only applies to
> +		 * the internal calibration. For system calibration it's as
> +		 * 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);
> +			if (ret < 0)
> +				goto out;
> +
> +			/*
> +			 * read out the resulting value of GAIN
> +			 * after full-scale calibration because the next
> +			 * ad_sd_calibrate() call overwrites this via
> +			 * ad_sigma_delta_set_channel() -> ad7124_set_channel()
> +			 * ... -> ad7124_enable_channel().
> +			 */
> +			ret = ad_sd_read_reg(&st->sd, AD7124_GAIN(st->channels[i].cfg.cfg_slot), 3,
> +					     &st->channels[i].cfg.calibration_gain);
> +			if (ret < 0)
> +				goto out;
> +		}
> +
> +		ret = ad_sd_calibrate(&st->sd, AD7124_MODE_CAL_INT_ZERO, i);
> +		if (ret < 0)
> +			goto out;
> +
> +		ret = ad_sd_read_reg(&st->sd, AD7124_OFFSET(st->channels[i].cfg.cfg_slot), 3,
> +				     &st->channels[i].cfg.calibration_offset);
> +		if (ret < 0)
> +			goto out;
> +
> +		dev_dbg(dev, "offset and gain for channel %d = 0x%x + 0x%x\n", i,
> +			st->channels[i].cfg.calibration_offset,
> +			st->channels[i].cfg.calibration_gain);
> +	}

to here as a ad7124_do_calibrate_all() or something like that.
Then you can do direct returns and it becomes really obvious this function
is stashing and restoring the adc_control value.

I don't mind that much as the flow is fairly simple.

> +
> +out:
> +	st->adc_control = adc_control;
> +
> +	return ret;
> +}
> +
>  static void ad7124_reg_disable(void *r)
>  {
>  	regulator_disable(r);
> @@ -1132,6 +1241,10 @@ static int ad7124_probe(struct spi_device *spi)
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to setup triggers\n");
>  
> +	ret = ad7124_calibrate_all(st, indio_dev);
> +	if (ret)
> +		return ret;
> +
>  	ret = devm_iio_device_register(&spi->dev, indio_dev);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to register iio device\n");

[PATCH v2 6/6] iio: adc: ad7124: Implement system calibration

[Uwe Kleine-König]

Allow triggering both zero-scale and full-scale calibration via sysfs in
the same way as it's done for ad7173.

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

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 5c2e5a518af3..ad14503e9797 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -4,6 +4,9 @@
  *
  * Copyright 2018 Analog Devices Inc.
  */
+
+#define DEBUG
+
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/clk.h>
@@ -181,6 +184,7 @@ struct ad7124_channel {
 	struct ad7124_channel_config cfg;
 	unsigned int ain;
 	unsigned int slot;
+	u8 syscalib_mode;
 };
 
 struct ad7124_state {
@@ -202,23 +206,6 @@ struct ad7124_state {
 	DECLARE_KFIFO(live_cfgs_fifo, struct ad7124_channel_config *, AD7124_MAX_CONFIGS);
 };
 
-static const struct iio_chan_spec ad7124_channel_template = {
-	.type = IIO_VOLTAGE,
-	.indexed = 1,
-	.differential = 1,
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-		BIT(IIO_CHAN_INFO_SCALE) |
-		BIT(IIO_CHAN_INFO_OFFSET) |
-		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
-		BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
-	.scan_type = {
-		.sign = 'u',
-		.realbits = 24,
-		.storagebits = 32,
-		.endianness = IIO_BE,
-	},
-};
-
 static struct ad7124_chip_info ad7124_chip_info_tbl[] = {
 	[ID_AD7124_4] = {
 		.name = "ad7124-4",
@@ -903,6 +890,126 @@ static int ad7124_check_chip_id(struct ad7124_state *st)
 	return 0;
 }
 
+enum {
+	AD7124_SYSCALIB_ZERO_SCALE,
+	AD7124_SYSCALIB_FULL_SCALE,
+};
+
+static ssize_t ad7124_write_syscalib(struct iio_dev *indio_dev,
+				     uintptr_t private,
+				     const struct iio_chan_spec *chan,
+				     const char *buf, size_t len)
+{
+	struct ad7124_state *st = iio_priv(indio_dev);
+	struct ad7124_channel *ch = &st->channels[chan->channel];
+	struct device *dev = &st->sd.spi->dev;
+	bool sys_calib;
+	int ret, mode;
+
+	ret = kstrtobool(buf, &sys_calib);
+	if (ret)
+		return ret;
+
+	mode = ch->syscalib_mode;
+	if (sys_calib) {
+		if (mode == AD7124_SYSCALIB_ZERO_SCALE) {
+			ch->cfg.calibration_offset = 0x800000;
+
+			ret = ad_sd_calibrate(&st->sd, AD7124_MODE_CAL_SYS_ZERO,
+					      chan->address);
+			if (ret < 0)
+				return ret;
+
+			ret = ad_sd_read_reg(&st->sd, AD7124_OFFSET(ch->cfg.cfg_slot), 3,
+					     &ch->cfg.calibration_offset);
+			if (ret < 0)
+				return ret;
+
+			dev_dbg(dev, "offset for channel %d after zero-scale calibration: 0x%x\n",
+				chan->channel, ch->cfg.calibration_offset);
+		} else {
+			ch->cfg.calibration_gain = st->gain_default;
+
+			ret = ad_sd_calibrate(&st->sd, AD7124_MODE_CAL_SYS_FULL,
+					      chan->address);
+			if (ret < 0)
+				return ret;
+
+			ret = ad_sd_read_reg(&st->sd, AD7124_GAIN(ch->cfg.cfg_slot), 3,
+					     &ch->cfg.calibration_gain);
+			if (ret < 0)
+				return ret;
+
+			dev_dbg(dev, "gain for channel %d after full-scale calibration: 0x%x\n",
+				chan->channel, ch->cfg.calibration_gain);
+		}
+	}
+
+	return len;
+}
+
+static const char * const ad7124_syscalib_modes[] = {
+	[AD7124_SYSCALIB_ZERO_SCALE] = "zero_scale",
+	[AD7124_SYSCALIB_FULL_SCALE] = "full_scale",
+};
+
+static int ad7124_set_syscalib_mode(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    unsigned int mode)
+{
+	struct ad7124_state *st = iio_priv(indio_dev);
+
+	st->channels[chan->channel].syscalib_mode = mode;
+
+	return 0;
+}
+
+static int ad7124_get_syscalib_mode(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan)
+{
+	struct ad7124_state *st = iio_priv(indio_dev);
+
+	return st->channels[chan->channel].syscalib_mode;
+}
+
+static const struct iio_enum ad7124_syscalib_mode_enum = {
+	.items = ad7124_syscalib_modes,
+	.num_items = ARRAY_SIZE(ad7124_syscalib_modes),
+	.set = ad7124_set_syscalib_mode,
+	.get = ad7124_get_syscalib_mode
+};
+
+static const struct iio_chan_spec_ext_info ad7124_calibsys_ext_info[] = {
+	{
+		.name = "sys_calibration",
+		.write = ad7124_write_syscalib,
+		.shared = IIO_SEPARATE,
+	},
+	IIO_ENUM("sys_calibration_mode", IIO_SEPARATE,
+		 &ad7124_syscalib_mode_enum),
+	IIO_ENUM_AVAILABLE("sys_calibration_mode", IIO_SHARED_BY_TYPE,
+			   &ad7124_syscalib_mode_enum),
+	{ }
+};
+
+static const struct iio_chan_spec ad7124_channel_template = {
+	.type = IIO_VOLTAGE,
+	.indexed = 1,
+	.differential = 1,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+		BIT(IIO_CHAN_INFO_SCALE) |
+		BIT(IIO_CHAN_INFO_OFFSET) |
+		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+		BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
+	.scan_type = {
+		.sign = 'u',
+		.realbits = 24,
+		.storagebits = 32,
+		.endianness = IIO_BE,
+	},
+	.ext_info = ad7124_calibsys_ext_info,
+};
+
 /*
  * Input specifiers 8 - 15 are explicitly reserved for ad7124-4
  * while they are fine for ad7124-8. Values above 31 don't fit
-- 
2.47.1

Re: [PATCH v2 6/6] iio: adc: ad7124: Implement system calibration

[David Lechner]

On 2/18/25 12:31 PM, Uwe Kleine-König wrote:
> Allow triggering both zero-scale and full-scale calibration via sysfs in
> the same way as it's done for ad7173.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---

...

> +static ssize_t ad7124_write_syscalib(struct iio_dev *indio_dev,
> +				     uintptr_t private,
> +				     const struct iio_chan_spec *chan,
> +				     const char *buf, size_t len)
> +{
> +	struct ad7124_state *st = iio_priv(indio_dev);
> +	struct ad7124_channel *ch = &st->channels[chan->channel];
> +	struct device *dev = &st->sd.spi->dev;
> +	bool sys_calib;
> +	int ret, mode;
> +
> +	ret = kstrtobool(buf, &sys_calib);
> +	if (ret)
> +		return ret;
> +
> +	mode = ch->syscalib_mode;
> +	if (sys_calib) {

Could save some indent by inverting the if and doing early return.

> +		if (mode == AD7124_SYSCALIB_ZERO_SCALE) {

Probably should claim direct mode here to prevent calibration during
a buffered read or other operation (this seems to be missing from
other ad_sigma_delta driver calibrations functions as well).

> +			ch->cfg.calibration_offset = 0x800000;
> +
> +			ret = ad_sd_calibrate(&st->sd, AD7124_MODE_CAL_SYS_ZERO,
> +					      chan->address);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = ad_sd_read_reg(&st->sd, AD7124_OFFSET(ch->cfg.cfg_slot), 3,
> +					     &ch->cfg.calibration_offset);
> +			if (ret < 0)
> +				return ret;
> +
> +			dev_dbg(dev, "offset for channel %d after zero-scale calibration: 0x%x\n",
> +				chan->channel, ch->cfg.calibration_offset);
> +		} else {
> +			ch->cfg.calibration_gain = st->gain_default;
> +
> +			ret = ad_sd_calibrate(&st->sd, AD7124_MODE_CAL_SYS_FULL,
> +					      chan->address);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = ad_sd_read_reg(&st->sd, AD7124_GAIN(ch->cfg.cfg_slot), 3,
> +					     &ch->cfg.calibration_gain);
> +			if (ret < 0)
> +				return ret;
> +
> +			dev_dbg(dev, "gain for channel %d after full-scale calibration: 0x%x\n",
> +				chan->channel, ch->cfg.calibration_gain);
> +		}
> +	}
> +
> +	return len;
> +}
> +

Re: [PATCH v2 6/6] iio: adc: ad7124: Implement system calibration

[Uwe Kleine-König]

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

Hello,

On Tue, Feb 18, 2025 at 01:37:45PM -0600, David Lechner wrote:
> On 2/18/25 12:31 PM, Uwe Kleine-König wrote:
> ...
> > +	mode = ch->syscalib_mode;
> > +	if (sys_calib) {
> 
> Could save some indent by inverting the if and doing early return.

ack.

> > +		if (mode == AD7124_SYSCALIB_ZERO_SCALE) {
> 
> Probably should claim direct mode here to prevent calibration during
> a buffered read or other operation (this seems to be missing from
> other ad_sigma_delta driver calibrations functions as well).

I think the right path forward would be to claim direct mode in
ad_sd_calibrate(). A difficulty is that ad7192_write_raw() and
ad7793_write_raw() call this calibration functions while having claimed
direct mode. I wonder if it's expected that the driver automatically
recalibrates the channel(s) when the parameters change? In that case
we'd need also a variant of ad_sd_calibrate() that doesn't claim direct
mode. Anyhow, that looks like an additional thing that probably should
be unified?!

Best regards
Uwe

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

Re: [PATCH v2 6/6] iio: adc: ad7124: Implement system calibration

[Nuno Sá]

On Tue, 2025-02-18 at 19:31 +0100, Uwe Kleine-König wrote:
> Allow triggering both zero-scale and full-scale calibration via sysfs in
> the same way as it's done for ad7173.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  drivers/iio/adc/ad7124.c | 141 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 124 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 5c2e5a518af3..ad14503e9797 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -4,6 +4,9 @@
>   *
>   * Copyright 2018 Analog Devices Inc.
>   */
> +
> +#define DEBUG

Leftover :)

- Nuno Sá

> +
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
> @@ -181,6 +184,7 @@ struct ad7124_channel {
>  	struct ad7124_channel_config cfg;
>  	unsigned int ain;
>  	unsigned int slot;
> +	u8 syscalib_mode;
>  };
>  
>  struct ad7124_state {
> @@ -202,23 +206,6 @@ struct ad7124_state {
>  	DECLARE_KFIFO(live_cfgs_fifo, struct ad7124_channel_config *,
> AD7124_MAX_CONFIGS);
>  };
>  
> -static const struct iio_chan_spec ad7124_channel_template = {
> -	.type = IIO_VOLTAGE,
> -	.indexed = 1,
> -	.differential = 1,
> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -		BIT(IIO_CHAN_INFO_SCALE) |
> -		BIT(IIO_CHAN_INFO_OFFSET) |
> -		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> -		BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
> -	.scan_type = {
> -		.sign = 'u',
> -		.realbits = 24,
> -		.storagebits = 32,
> -		.endianness = IIO_BE,
> -	},
> -};
> -
>  static struct ad7124_chip_info ad7124_chip_info_tbl[] = {
>  	[ID_AD7124_4] = {
>  		.name = "ad7124-4",
> @@ -903,6 +890,126 @@ static int ad7124_check_chip_id(struct ad7124_state *st)
>  	return 0;
>  }
>  
> +enum {
> +	AD7124_SYSCALIB_ZERO_SCALE,
> +	AD7124_SYSCALIB_FULL_SCALE,
> +};
> +
> +static ssize_t ad7124_write_syscalib(struct iio_dev *indio_dev,
> +				     uintptr_t private,
> +				     const struct iio_chan_spec *chan,
> +				     const char *buf, size_t len)
> +{
> +	struct ad7124_state *st = iio_priv(indio_dev);
> +	struct ad7124_channel *ch = &st->channels[chan->channel];
> +	struct device *dev = &st->sd.spi->dev;
> +	bool sys_calib;
> +	int ret, mode;
> +
> +	ret = kstrtobool(buf, &sys_calib);
> +	if (ret)
> +		return ret;
> +
> +	mode = ch->syscalib_mode;
> +	if (sys_calib) {

FWIW, I agree with both David's comments. Moreover, if we do not claim the IIO
lock in other drivers/devices during calibration I think we should be doing
that. 


- Nuno Sá

Re: [PATCH v2 0/6] iio: adc: ad{4130,7124,7173}: A few fixes and ad7124 calibration

[Jonathan Cameron]

On Tue, 18 Feb 2025 19:31:07 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello,
> 
> this v2 series is a rework of two series:
> 
> 	https://lore.kernel.org/iio/20250212105322.10243-5-u.kleine-koenig@baylibre.com
> 	https://lore.kernel.org/iio/cover.1738258777.git.u.kleine-koenig@baylibre.com
> 
> ; they overlap thematically and so I put them together in a single
> series.

I considered tidying up the things David raised in patch 2 and picking
the first 4 patches up now, but that doesn't give room for others to apply tags
if they want to, so I'll wait for v3.


> 
> Changes since their (implicit) v1:
> 
>  - Use static_assert instead of BUILD_BUG, add more comments to the "Fix
>    comparison" patches
>  - Make ad7124 internal calibration actually compile
>  - Sort the fix to the front of the series and add a Fixes: tag
>  - Implement system calibration
>  - More comments and commit log improvements
> 
> The "Fix comparison" patches trigger a checkpatch warning because the
> struct members are considered wrongly indented. For me the indention
> looks right, but I don't feel strong here and happily adapt if
> requested.
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (6):
>   iio: adc: ad_sigma_delta: Disable channel after calibration
>   iio: adc: ad4130: Fix comparison of channel setups
>   iio: adc: ad7124: Fix comparison of channel configs
>   iio: adc: ad7173: Fix comparison of channel configs
>   iio: adc: ad7124: Implement internal calibration at probe time
>   iio: adc: ad7124: Implement system calibration
> 
>  drivers/iio/adc/ad4130.c         |  41 ++++-
>  drivers/iio/adc/ad7124.c         | 293 ++++++++++++++++++++++++++++---
>  drivers/iio/adc/ad7173.c         |  25 ++-
>  drivers/iio/adc/ad_sigma_delta.c |   1 +
>  4 files changed, 331 insertions(+), 29 deletions(-)
> 
> base-commit: ac856912f210bcff6a1cf8cf9cb2f6a1dfe85798