From: David Lechner <dlechner@baylibre.com>
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 <dlechner@baylibre.com>
Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
drivers/iio/adc/ad7380.c | 98 +++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 85 insertions(+), 13 deletions(-)
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);
--
2.44.0On Tue, 19 Mar 2024 11:11:25 +0100
Julien Stephan <jstephan@baylibre.com> wrote:
> From: David Lechner <dlechner@baylibre.com>
>
> 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 <dlechner@baylibre.com>
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
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;
> + }
> +
On Sun, Mar 24, 2024 at 8:01 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 19 Mar 2024 11:11:25 +0100
> Julien Stephan <jstephan@baylibre.com> wrote:
>
> > From: David Lechner <dlechner@baylibre.com>
> >
> > 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 <dlechner@baylibre.com>
> > Signed-off-by: Julien Stephan <jstephan@baylibre.com>
>
> 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;
> > + }
> > +
> > > + /*
> > > + * 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;
> > > + }
> > > +
>
On Mon, Mar 25, 2024 at 3:06 PM Jonathan Cameron <jic23@kernel.org> 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). >
© 2016 - 2025 Red Hat, Inc.