[v5,4/7] iio: adc: ad7380: add support for pseudo-differential parts

Commit Message

Julien Stephan March 19, 2024, 10:11 a.m. UTC

From: David Lechner <[e-mail]>

Add support for AD7383, AD7384 pseudo-differential compatible parts.
Pseudo differential parts require common mode voltage supplies so add
the support for them and add the support of IIO_CHAN_INFO_OFFSET to
retrieve the offset

Signed-off-by: David Lechner <[e-mail]>
Signed-off-by: Julien Stephan <[e-mail]>
---
 drivers/iio/adc/ad7380.c | 98 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 85 insertions(+), 13 deletions(-)

Comments

Jonathan Cameron March 24, 2024, 1:01 p.m. UTC | #1

On Tue, 19 Mar 2024 11:11:25 +0100
Julien Stephan <[e-mail]> wrote:

> From: David Lechner <[e-mail]>
> 
> Add support for AD7383, AD7384 pseudo-differential compatible parts.
> Pseudo differential parts require common mode voltage supplies so add
> the support for them and add the support of IIO_CHAN_INFO_OFFSET to
> retrieve the offset
> 
> Signed-off-by: David Lechner <[e-mail]>
> Signed-off-by: Julien Stephan <[e-mail]>

Hi Julien,

A few aditional comments inline.  The one about
optional regulators may be something others disagree with.
Mark, perhaps you have time to comment.
Is this usage of devm_regulator_get_optional() to check a real regulator
is supplied (as we are going to get the voltage) sensible?  Feels wrong
given the regulator is the exact opposite of optional.

Jonathan

>  struct ad7380_state {
>  	const struct ad7380_chip_info *chip_info;
>  	struct spi_device *spi;
>  	struct regmap *regmap;
>  	unsigned int vref_mv;
> +	unsigned int vcm_mv[2];
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache lines.
> @@ -304,6 +333,11 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>  		*val2 = chan->scan_type.realbits;
>  
>  		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = st->vcm_mv[chan->channel] * (1 << chan->scan_type.realbits)
> +			/ st->vref_mv;

So this maths seems to be right to me, but it took me a while to figure it out.
Perhaps a comment would help along the lines of this is transforming

	(raw * scale) + vcm_mv
to
	(raw + vcm_mv / scale) * scale
as IIO ABI says offset is applied before scale.

> +
> +		return IIO_VAL_INT;
>  	}
>  
>  	return -EINVAL;
> @@ -350,7 +384,7 @@ static int ad7380_probe(struct spi_device *spi)
>  	struct iio_dev *indio_dev;
>  	struct ad7380_state *st;
>  	struct regulator *vref;
> -	int ret;
> +	int ret, i;
>  
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>  	if (!indio_dev)
> @@ -394,6 +428,40 @@ static int ad7380_probe(struct spi_device *spi)
>  		st->vref_mv = AD7380_INTERNAL_REF_MV;
>  	}
>  
> +	if (st->chip_info->num_vcm_supplies > ARRAY_SIZE(st->vcm_mv))
> +		return dev_err_probe(&spi->dev, -EINVAL,
> +				     "invalid number of VCM supplies\n");
> +
> +	/*
> +	 * pseudo-differential chips have common mode supplies for the negative
> +	 * input pin.
> +	 */
> +	for (i = 0; i < st->chip_info->num_vcm_supplies; i++) {
> +		struct regulator *vcm;
> +
> +		vcm = devm_regulator_get_optional(&spi->dev,

Why optional?

> +						  st->chip_info->vcm_supplies[i]);
> +		if (IS_ERR(vcm))

This will fail if it's not there, so I'm guessing you are using this to avoid
getting to the regulator_get_voltage?  If it's not present I'd rely on that
failing rather than the confusing handling here.

When the read of voltage wasn't in probe this would have resulted in a problem
much later than initial setup, now it is, we are just pushing it down a few lines.

Arguably we could have a devm_regulator_get_not_dummy()
that had same implementation to as get_optional() but whilst it's called that
I think it's confusing to use like this.

> +			return dev_err_probe(&spi->dev, PTR_ERR(vcm),
> +					     "Failed to get %s regulator\n",
> +					     st->chip_info->vcm_supplies[i]);
> +
> +		ret = regulator_enable(vcm);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_add_action_or_reset(&spi->dev,
> +					       ad7380_regulator_disable, vcm);
> +		if (ret)
> +			return ret;
> +
> +		ret = regulator_get_voltage(vcm);

I'd let this fail if we have a dummy regulator.

> +		if (ret < 0)
> +			return ret;
> +
> +		st->vcm_mv[i] = ret / 1000;
> +	}
> +

David Lechner March 25, 2024, 2:08 p.m. UTC | #2

On Sun, Mar 24, 2024 at 8:01 AM Jonathan Cameron <[e-mail]> wrote:
>
> On Tue, 19 Mar 2024 11:11:25 +0100
> Julien Stephan <[e-mail]> wrote:
>
> > From: David Lechner <[e-mail]>
> >
> > Add support for AD7383, AD7384 pseudo-differential compatible parts.
> > Pseudo differential parts require common mode voltage supplies so add
> > the support for them and add the support of IIO_CHAN_INFO_OFFSET to
> > retrieve the offset
> >
> > Signed-off-by: David Lechner <[e-mail]>
> > Signed-off-by: Julien Stephan <[e-mail]>
>
> Hi Julien,
>
> A few aditional comments inline.  The one about
> optional regulators may be something others disagree with.
> Mark, perhaps you have time to comment.
> Is this usage of devm_regulator_get_optional() to check a real regulator
> is supplied (as we are going to get the voltage) sensible?  Feels wrong
> given the regulator is the exact opposite of optional.
>
> Jonathan
>
> >  struct ad7380_state {
> >       const struct ad7380_chip_info *chip_info;
> >       struct spi_device *spi;
> >       struct regmap *regmap;
> >       unsigned int vref_mv;
> > +     unsigned int vcm_mv[2];
> >       /*
> >        * DMA (thus cache coherency maintenance) requires the
> >        * transfer buffers to live in their own cache lines.
> > @@ -304,6 +333,11 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
> >               *val2 = chan->scan_type.realbits;
> >
> >               return IIO_VAL_FRACTIONAL_LOG2;
> > +     case IIO_CHAN_INFO_OFFSET:
> > +             *val = st->vcm_mv[chan->channel] * (1 << chan->scan_type.realbits)
> > +                     / st->vref_mv;
>
> So this maths seems to be right to me, but it took me a while to figure it out.
> Perhaps a comment would help along the lines of this is transforming
>
>         (raw * scale) + vcm_mv
> to
>         (raw + vcm_mv / scale) * scale
> as IIO ABI says offset is applied before scale.
>
> > +
> > +             return IIO_VAL_INT;
> >       }
> >
> >       return -EINVAL;
> > @@ -350,7 +384,7 @@ static int ad7380_probe(struct spi_device *spi)
> >       struct iio_dev *indio_dev;
> >       struct ad7380_state *st;
> >       struct regulator *vref;
> > -     int ret;
> > +     int ret, i;
> >
> >       indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> >       if (!indio_dev)
> > @@ -394,6 +428,40 @@ static int ad7380_probe(struct spi_device *spi)
> >               st->vref_mv = AD7380_INTERNAL_REF_MV;
> >       }
> >
> > +     if (st->chip_info->num_vcm_supplies > ARRAY_SIZE(st->vcm_mv))
> > +             return dev_err_probe(&spi->dev, -EINVAL,
> > +                                  "invalid number of VCM supplies\n");
> > +
> > +     /*
> > +      * pseudo-differential chips have common mode supplies for the negative
> > +      * input pin.
> > +      */
> > +     for (i = 0; i < st->chip_info->num_vcm_supplies; i++) {
> > +             struct regulator *vcm;
> > +
> > +             vcm = devm_regulator_get_optional(&spi->dev,
>
> Why optional?
>
> > +                                               st->chip_info->vcm_supplies[i]);
> > +             if (IS_ERR(vcm))
>
> This will fail if it's not there, so I'm guessing you are using this to avoid
> getting to the regulator_get_voltage?  If it's not present I'd rely on that
> failing rather than the confusing handling here.
>
> When the read of voltage wasn't in probe this would have resulted in a problem
> much later than initial setup, now it is, we are just pushing it down a few lines.
>
> Arguably we could have a devm_regulator_get_not_dummy()
> that had same implementation to as get_optional() but whilst it's called that
> I think it's confusing to use like this.

Despite the misleading naming, I guess I am used to
devm_regulator_get_optional() by now having used it enough times.
Since it fails either way though, technically both ways seem fine so I
can't really argue for one over the other.

But given that this is a common pattern in many IIO drivers, maybe we
make a devm_regulator_get_enable_get_voltage()? This would return the
voltage on success or an error code. (If the regulator subsystem
doesn't want this maybe we could have
devm_iio_regulator_get_enable_get_voltage()).

If the dev_err_probe() calls were included in
devm_regulator_get_enable_get_voltage(), then the 10+ lines of code
here and in many other drivers to get the regulator, enable it, add
the reset action and get the voltage could be reduced to 3 lines.

>
> > +                     return dev_err_probe(&spi->dev, PTR_ERR(vcm),
> > +                                          "Failed to get %s regulator\n",
> > +                                          st->chip_info->vcm_supplies[i]);
> > +
> > +             ret = regulator_enable(vcm);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = devm_add_action_or_reset(&spi->dev,
> > +                                            ad7380_regulator_disable, vcm);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = regulator_get_voltage(vcm);
>
> I'd let this fail if we have a dummy regulator.
>
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             st->vcm_mv[i] = ret / 1000;
> > +     }
> > +

Jonathan Cameron March 25, 2024, 8:06 p.m. UTC | #3

> > > +     /*
> > > +      * pseudo-differential chips have common mode supplies for the negative
> > > +      * input pin.
> > > +      */
> > > +     for (i = 0; i < st->chip_info->num_vcm_supplies; i++) {
> > > +             struct regulator *vcm;
> > > +
> > > +             vcm = devm_regulator_get_optional(&spi->dev,  
> >
> > Why optional?
> >  
> > > +                                               st->chip_info->vcm_supplies[i]);
> > > +             if (IS_ERR(vcm))  
> >
> > This will fail if it's not there, so I'm guessing you are using this to avoid
> > getting to the regulator_get_voltage?  If it's not present I'd rely on that
> > failing rather than the confusing handling here.
> >
> > When the read of voltage wasn't in probe this would have resulted in a problem
> > much later than initial setup, now it is, we are just pushing it down a few lines.
> >
> > Arguably we could have a devm_regulator_get_not_dummy()
> > that had same implementation to as get_optional() but whilst it's called that
> > I think it's confusing to use like this.  
> 
> Despite the misleading naming, I guess I am used to
> devm_regulator_get_optional() by now having used it enough times.
> Since it fails either way though, technically both ways seem fine so I
> can't really argue for one over the other.
> 
> But given that this is a common pattern in many IIO drivers, maybe we
> make a devm_regulator_get_enable_get_voltage()? This would return the
> voltage on success or an error code. (If the regulator subsystem
> doesn't want this maybe we could have
> devm_iio_regulator_get_enable_get_voltage()).
> 
> If the dev_err_probe() calls were included in
> devm_regulator_get_enable_get_voltage(), then the 10+ lines of code
> here and in many other drivers to get the regulator, enable it, add
> the reset action and get the voltage could be reduced to 3 lines.

I like this proposal a lot. RFC, so it's visible outside the depths
of this thread?
Particularly good as it will keep the regulator opaque in the same
fashion as devm_regulator_get_enabled()

As you say, we have a 'lot' of instances of this (quick grep
suggests > 50 in IIO alone and smaller numbers elsewhere).

Jonathan


> 
> >  
> > > +                     return dev_err_probe(&spi->dev, PTR_ERR(vcm),
> > > +                                          "Failed to get %s regulator\n",
> > > +                                          st->chip_info->vcm_supplies[i]);
> > > +
> > > +             ret = regulator_enable(vcm);
> > > +             if (ret)
> > > +                     return ret;
> > > +
> > > +             ret = devm_add_action_or_reset(&spi->dev,
> > > +                                            ad7380_regulator_disable, vcm);
> > > +             if (ret)
> > > +                     return ret;
> > > +
> > > +             ret = regulator_get_voltage(vcm);  
> >
> > I'd let this fail if we have a dummy regulator.
> >  
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +
> > > +             st->vcm_mv[i] = ret / 1000;
> > > +     }
> > > +  
>

David Lechner March 25, 2024, 8:14 p.m. UTC | #4

On Mon, Mar 25, 2024 at 3:06 PM Jonathan Cameron <[e-mail]> wrote:
>
> >
> > But given that this is a common pattern in many IIO drivers, maybe we
> > make a devm_regulator_get_enable_get_voltage()? This would return the
> > voltage on success or an error code. (If the regulator subsystem
> > doesn't want this maybe we could have
> > devm_iio_regulator_get_enable_get_voltage()).
> >
> > If the dev_err_probe() calls were included in
> > devm_regulator_get_enable_get_voltage(), then the 10+ lines of code
> > here and in many other drivers to get the regulator, enable it, add
> > the reset action and get the voltage could be reduced to 3 lines.
>
> I like this proposal a lot. RFC, so it's visible outside the depths
> of this thread?

Yes, I can send an RFC separately so it doesn't hold up this patch/series.

> Particularly good as it will keep the regulator opaque in the same
> fashion as devm_regulator_get_enabled()
>
> As you say, we have a 'lot' of instances of this (quick grep
> suggests > 50 in IIO alone and smaller numbers elsewhere).
>

Patch

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index caf6deb3a8b1..996ca83feaed 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -7,6 +7,7 @@ 
  *
  * Datasheets of supported parts:
  * ad7380/1 : https://www.analog.com/media/en/technical-documentation/data-sheets/AD7380-7381.pdf
+ * ad7383/4 : https://www.analog.com/media/en/technical-documentation/data-sheets/ad7383-7384.pdf
  */
 
 #include <linux/bitfield.h>
@@ -68,16 +69,19 @@  struct ad7380_chip_info {
 	const char *name;
 	const struct iio_chan_spec *channels;
 	unsigned int num_channels;
+	const char * const *vcm_supplies;
+	unsigned int num_vcm_supplies;
 };
 
-#define AD7380_CHANNEL(index, bits) {				\
+#define AD7380_CHANNEL(index, bits, diff) {			\
 	.type = IIO_VOLTAGE,					\
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
+		((diff) ? 0 : BIT(IIO_CHAN_INFO_OFFSET)),	\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
 	.indexed = 1,						\
-	.differential = 1,					\
-	.channel = 2 * (index),					\
-	.channel2 = 2 * (index) + 1,				\
+	.differential = (diff),					\
+	.channel = (diff) ? (2 * (index)) : (index),		\
+	.channel2 = (diff) ? (2 * (index) + 1) : 0,		\
 	.scan_index = (index),					\
 	.scan_type = {						\
 		.sign = 's',					\
@@ -87,15 +91,23 @@  struct ad7380_chip_info {
 	},							\
 }
 
-#define DEFINE_AD7380_2_CHANNEL(name, bits)	\
-static const struct iio_chan_spec name[] = {	\
-	AD7380_CHANNEL(0, bits),		\
-	AD7380_CHANNEL(1, bits),		\
-	IIO_CHAN_SOFT_TIMESTAMP(2),		\
+#define DEFINE_AD7380_2_CHANNEL(name, bits, diff)	\
+static const struct iio_chan_spec name[] = {		\
+	AD7380_CHANNEL(0, bits, diff),			\
+	AD7380_CHANNEL(1, bits, diff),			\
+	IIO_CHAN_SOFT_TIMESTAMP(2),			\
 }
 
-DEFINE_AD7380_2_CHANNEL(ad7380_channels, 16);
-DEFINE_AD7380_2_CHANNEL(ad7381_channels, 14);
+/* fully differential */
+DEFINE_AD7380_2_CHANNEL(ad7380_channels, 16, 1);
+DEFINE_AD7380_2_CHANNEL(ad7381_channels, 14, 1);
+/* pseudo differential */
+DEFINE_AD7380_2_CHANNEL(ad7383_channels, 16, 0);
+DEFINE_AD7380_2_CHANNEL(ad7384_channels, 14, 0);
+
+static const char * const ad7380_2_channel_vcm_supplies[] = {
+	"aina", "ainb",
+};
 
 /* Since this is simultaneous sampling, we don't allow individual channels. */
 static const unsigned long ad7380_2_channel_scan_masks[] = {
@@ -115,11 +127,28 @@  static const struct ad7380_chip_info ad7381_chip_info = {
 	.num_channels = ARRAY_SIZE(ad7381_channels),
 };
 
+static const struct ad7380_chip_info ad7383_chip_info = {
+	.name = "ad7383",
+	.channels = ad7383_channels,
+	.num_channels = ARRAY_SIZE(ad7383_channels),
+	.vcm_supplies = ad7380_2_channel_vcm_supplies,
+	.num_vcm_supplies = ARRAY_SIZE(ad7380_2_channel_vcm_supplies),
+};
+
+static const struct ad7380_chip_info ad7384_chip_info = {
+	.name = "ad7384",
+	.channels = ad7384_channels,
+	.num_channels = ARRAY_SIZE(ad7384_channels),
+	.vcm_supplies = ad7380_2_channel_vcm_supplies,
+	.num_vcm_supplies = ARRAY_SIZE(ad7380_2_channel_vcm_supplies),
+};
+
 struct ad7380_state {
 	const struct ad7380_chip_info *chip_info;
 	struct spi_device *spi;
 	struct regmap *regmap;
 	unsigned int vref_mv;
+	unsigned int vcm_mv[2];
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
 	 * transfer buffers to live in their own cache lines.
@@ -304,6 +333,11 @@  static int ad7380_read_raw(struct iio_dev *indio_dev,
 		*val2 = chan->scan_type.realbits;
 
 		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = st->vcm_mv[chan->channel] * (1 << chan->scan_type.realbits)
+			/ st->vref_mv;
+
+		return IIO_VAL_INT;
 	}
 
 	return -EINVAL;
@@ -350,7 +384,7 @@  static int ad7380_probe(struct spi_device *spi)
 	struct iio_dev *indio_dev;
 	struct ad7380_state *st;
 	struct regulator *vref;
-	int ret;
+	int ret, i;
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
 	if (!indio_dev)
@@ -394,6 +428,40 @@  static int ad7380_probe(struct spi_device *spi)
 		st->vref_mv = AD7380_INTERNAL_REF_MV;
 	}
 
+	if (st->chip_info->num_vcm_supplies > ARRAY_SIZE(st->vcm_mv))
+		return dev_err_probe(&spi->dev, -EINVAL,
+				     "invalid number of VCM supplies\n");
+
+	/*
+	 * pseudo-differential chips have common mode supplies for the negative
+	 * input pin.
+	 */
+	for (i = 0; i < st->chip_info->num_vcm_supplies; i++) {
+		struct regulator *vcm;
+
+		vcm = devm_regulator_get_optional(&spi->dev,
+						  st->chip_info->vcm_supplies[i]);
+		if (IS_ERR(vcm))
+			return dev_err_probe(&spi->dev, PTR_ERR(vcm),
+					     "Failed to get %s regulator\n",
+					     st->chip_info->vcm_supplies[i]);
+
+		ret = regulator_enable(vcm);
+		if (ret)
+			return ret;
+
+		ret = devm_add_action_or_reset(&spi->dev,
+					       ad7380_regulator_disable, vcm);
+		if (ret)
+			return ret;
+
+		ret = regulator_get_voltage(vcm);
+		if (ret < 0)
+			return ret;
+
+		st->vcm_mv[i] = ret / 1000;
+	}
+
 	st->regmap = devm_regmap_init(&spi->dev, NULL, st, &ad7380_regmap_config);
 	if (IS_ERR(st->regmap))
 		return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
@@ -422,12 +490,16 @@  static int ad7380_probe(struct spi_device *spi)
 static const struct of_device_id ad7380_of_match_table[] = {
 	{ .compatible = "adi,ad7380", .data = &ad7380_chip_info },
 	{ .compatible = "adi,ad7381", .data = &ad7381_chip_info },
+	{ .compatible = "adi,ad7383", .data = &ad7383_chip_info },
+	{ .compatible = "adi,ad7384", .data = &ad7384_chip_info },
 	{ }
 };
 
 static const struct spi_device_id ad7380_id_table[] = {
 	{ "ad7380", (kernel_ulong_t)&ad7380_chip_info },
 	{ "ad7381", (kernel_ulong_t)&ad7381_chip_info },
+	{ "ad7383", (kernel_ulong_t)&ad7383_chip_info },
+	{ "ad7384", (kernel_ulong_t)&ad7384_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, ad7380_id_table);