[PATCH v2 1/6] iio: ad7949: Support internal Vref

[PATCH v2 1/6] iio: ad7949: Support internal Vref

[Adam Michaelis]

Adding configurable (via device tree) options to select either an external
reference voltage (default, original implementation) or one of the two
internal reference voltages provided by the AD7949 part family.

Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
	V2: Add some defines to reduce use of magic numbers.
---
 drivers/iio/adc/ad7949.c | 135 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 108 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index ac0ffff6c5ae..afc1361af5fb 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -11,12 +11,27 @@
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
-
-#define AD7949_MASK_CHANNEL_SEL		GENMASK(9, 7)
-#define AD7949_MASK_TOTAL		GENMASK(13, 0)
-#define AD7949_OFFSET_CHANNEL_SEL	7
-#define AD7949_CFG_READ_BACK		0x1
-#define AD7949_CFG_REG_SIZE_BITS	14
+#include <linux/of.h>
+
+#define AD7949_CFG_REG_SIZE_BITS           14
+#define AD7949_CFG_MASK_TOTAL              GENMASK(13, 0)
+#define AD7949_CFG_OVERWRITE_SHIFT         13
+#define AD7949_CFG_CHAN_CFG_UNIPOLAR_GND   0x7
+#define AD7949_CFG_CHAN_CFG_MASK           GENMASK(12, 10)
+#define AD7949_CFG_CHAN_CFG_SHIFT          10
+#define AD7949_CFG_CHAN_SEL_MASK           GENMASK(9, 7)
+#define AD7949_CFG_CHAN_SEL_SHIFT          7
+#define AD7949_CFG_BW_FULL                 1
+#define AD7949_CFG_BW_SHIFT                6
+#define AD7949_CFG_BW_MASK                 GENMASK(6, 6)
+#define AD7949_CFG_REF_SEL_MASK            GENMASK(5, 3)
+#define AD7949_CFG_REF_SEL_SHIFT           3
+#define AD7949_CFG_SEQ_DISABLED            0x0
+#define AD7949_CFG_SEQ_SHIFT               1
+#define AD7949_CFG_SEQ_MASK                GENMASK(2, 1)
+#define AD7949_CFG_READBACK_EN             0
+#define AD7949_CFG_READBACK_DIS            1
+#define AD7949_CFG_READBACK_MASK           GENMASK(0, 0)
 
 enum {
 	ID_AD7949 = 0,
@@ -24,6 +39,18 @@ enum {
 	ID_AD7689,
 };
 
+enum ad7949_ref_sel {
+	AD7949_REF_2V5 = 0, /* 2.5V internal ref + temp sensor */
+	AD7949_REF_4V0, /* 4.096V internal ref + temp sensor */
+	AD7949_REF_EXT_TEMP, /* External ref + temp sensor, no buffer */
+	AD7949_REF_EXT_TEMP_BUF, /* External ref + temp sensor + buffer */
+	AD7949_REF_RSRV_4,
+	AD7949_REF_RSRV_5,
+	AD7949_REF_EXT, /* External ref, no temp, no buffer */
+	AD7949_REF_EXT_BUF, /* External ref + buffer, no temp */
+	AD7949_REF_MAX,
+};
+
 struct ad7949_adc_spec {
 	u8 num_channels;
 	u8 resolution;
@@ -41,6 +68,7 @@ struct ad7949_adc_spec {
  * @vref: regulator generating Vref
  * @iio_dev: reference to iio structure
  * @spi: reference to spi structure
+ * @ref_sel: selected reference voltage source
  * @resolution: resolution of the chip
  * @cfg: copy of the configuration register
  * @current_channel: current channel in use
@@ -51,6 +79,7 @@ struct ad7949_adc_chip {
 	struct regulator *vref;
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
+	enum ad7949_ref_sel ref_sel;
 	u8 resolution;
 	u16 cfg;
 	unsigned int current_channel;
@@ -59,7 +88,7 @@ struct ad7949_adc_chip {
 
 static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
 {
-	if (!(ad7949_adc->cfg & AD7949_CFG_READ_BACK))
+	if (!(ad7949_adc->cfg & AD7949_CFG_READBACK_MASK))
 		return true;
 
 	return false;
@@ -91,7 +120,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
 	};
 
 	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
-	ad7949_adc->buffer = ad7949_adc->cfg << shift;
+	ad7949_adc->buffer = (ad7949_adc->cfg & AD7949_CFG_MASK_TOTAL) << shift;
 	spi_message_init_with_transfers(&msg, tx, 1);
 	ret = spi_sync(ad7949_adc->spi, &msg);
 
@@ -119,8 +148,8 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	};
 
 	ret = ad7949_spi_write_cfg(ad7949_adc,
-				   channel << AD7949_OFFSET_CHANNEL_SEL,
-				   AD7949_MASK_CHANNEL_SEL);
+				   channel << AD7949_CFG_CHAN_SEL_SHIFT,
+				   AD7949_CFG_CHAN_SEL_MASK);
 	if (ret)
 		return ret;
 
@@ -187,11 +216,20 @@ static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_SCALE:
-		ret = regulator_get_voltage(ad7949_adc->vref);
-		if (ret < 0)
-			return ret;
+		if (ad7949_adc->vref) {
+			ret = regulator_get_voltage(ad7949_adc->vref);
+			if (ret < 0)
+				return ret;
+
+			*val = ret / 5000;
+		} else if (ad7949_adc->ref_sel == AD7949_REF_2V5) {
+			*val = 2500;
+		} else if (ad7949_adc->ref_sel == AD7949_REF_4V0) {
+			*val = 4096;
+		} else {
+			return -EINVAL;
+		}
 
-		*val = ret / 5000;
 		return IIO_VAL_INT;
 	}
 
@@ -209,7 +247,7 @@ static int ad7949_spi_reg_access(struct iio_dev *indio_dev,
 		*readval = ad7949_adc->cfg;
 	else
 		ret = ad7949_spi_write_cfg(ad7949_adc,
-			writeval & AD7949_MASK_TOTAL, AD7949_MASK_TOTAL);
+			writeval & AD7949_CFG_MASK_TOTAL, AD7949_CFG_MASK_TOTAL);
 
 	return ret;
 }
@@ -223,10 +261,33 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 {
 	int ret;
 	int val;
+	u16 adc_config = 0;
 
-	/* Sequencer disabled, CFG readback disabled, IN0 as default channel */
 	ad7949_adc->current_channel = 0;
-	ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
+	ad7949_adc->cfg = 0;
+
+	/*
+	 * 14-bit configuration register (pg 12 of AD7949 Datasheet):
+	 * | 13  | 12 - 10 | 9 - 7 | 6  | 5 -3 | 2 - 1 | 0  |
+	 * | CFG | INCC    | INx   | BW | REF  | SEQ   | RB |
+	 */
+	adc_config = 1 << AD7949_CFG_OVERWRITE_SHIFT;
+	adc_config |=
+		(AD7949_CFG_CHAN_CFG_UNIPOLAR_GND << AD7949_CFG_CHAN_CFG_SHIFT)
+			& AD7949_CFG_CHAN_CFG_MASK;
+	adc_config |= (ad7949_adc->current_channel << AD7949_CFG_CHAN_SEL_SHIFT)
+			& AD7949_CFG_CHAN_SEL_MASK;
+	adc_config |= (AD7949_CFG_BW_FULL << AD7949_CFG_BW_SHIFT) &
+			AD7949_CFG_BW_MASK;
+	adc_config |= (ad7949_adc->ref_sel << AD7949_CFG_REF_SEL_SHIFT) &
+			AD7949_CFG_REF_SEL_MASK;
+	adc_config |= (AD7949_CFG_SEQ_DISABLED << AD7949_CFG_SEQ_SHIFT) &
+			AD7949_CFG_SEQ_MASK;
+	adc_config |= AD7949_CFG_READBACK_DIS;
+
+	ret = ad7949_spi_write_cfg(ad7949_adc,
+			adc_config,
+			AD7949_CFG_MASK_TOTAL);
 
 	/*
 	 * Do two dummy conversions to apply the first configuration setting.
@@ -245,6 +306,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
 	struct ad7949_adc_chip *ad7949_adc;
 	struct iio_dev *indio_dev;
 	int ret;
+	u32 temp;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
 	if (!indio_dev) {
@@ -263,21 +325,38 @@ static int ad7949_spi_probe(struct spi_device *spi)
 	ad7949_adc = iio_priv(indio_dev);
 	ad7949_adc->indio_dev = indio_dev;
 	ad7949_adc->spi = spi;
+	ad7949_adc->vref = NULL;
 
 	spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
 	indio_dev->num_channels = spec->num_channels;
 	ad7949_adc->resolution = spec->resolution;
 
-	ad7949_adc->vref = devm_regulator_get(dev, "vref");
-	if (IS_ERR(ad7949_adc->vref)) {
-		dev_err(dev, "fail to request regulator\n");
-		return PTR_ERR(ad7949_adc->vref);
+	ret = of_property_read_u32(ad7949_adc->indio_dev->dev.of_node,
+			"adi,reference-select",
+			&temp);
+	if ((ret == 0) && (temp < AD7949_REF_MAX) &&
+			(temp != AD7949_REF_RSRV_4) &&
+			(temp != AD7949_REF_RSRV_5)) {
+		ad7949_adc->ref_sel = (enum ad7949_ref_sel)temp;
+	} else {
+		ad7949_adc->ref_sel = AD7949_REF_EXT_BUF;
+		dev_warn(dev, "using external Vref by default\n");
 	}
 
-	ret = regulator_enable(ad7949_adc->vref);
-	if (ret < 0) {
-		dev_err(dev, "fail to enable regulator\n");
-		return ret;
+	/* Check whether using external Vref */
+	if ((ad7949_adc->ref_sel != AD7949_REF_2V5) &&
+			(ad7949_adc->ref_sel != AD7949_REF_4V0)) {
+		ad7949_adc->vref = devm_regulator_get(dev, "vref");
+		if (IS_ERR(ad7949_adc->vref)) {
+			dev_err(dev, "fail to request regulator\n");
+			return PTR_ERR(ad7949_adc->vref);
+		}
+
+		ret = regulator_enable(ad7949_adc->vref);
+		if (ret < 0) {
+			dev_err(dev, "fail to enable regulator\n");
+			return ret;
+		}
 	}
 
 	mutex_init(&ad7949_adc->lock);
@@ -298,7 +377,8 @@ static int ad7949_spi_probe(struct spi_device *spi)
 
 err:
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
+	if (ad7949_adc->vref)
+		regulator_disable(ad7949_adc->vref);
 
 	return ret;
 }
@@ -310,7 +390,8 @@ static int ad7949_spi_remove(struct spi_device *spi)
 
 	iio_device_unregister(indio_dev);
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
+	if (ad7949_adc->vref)
+		regulator_disable(ad7949_adc->vref);
 
 	return 0;
 }
-- 
1.9.1

[PATCH v2 2/6] dt-bindings: iio: ad7949: Add adi,reference-select

[Adam Michaelis]

Adding optional parameter to AD7949 to specify the source for the
reference voltage signal. Default value is maintaned with option '6' to
match previous version of driver.

Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
	V2: Add some defines to reduce use of magic numbers.
---
 .../devicetree/bindings/iio/adc/ad7949.txt         | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/ad7949.txt b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
index c7f5057356b1..14ee9a2cb2a5 100644
--- a/Documentation/devicetree/bindings/iio/adc/ad7949.txt
+++ b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
@@ -6,11 +6,29 @@ Required properties:
 	* "adi,ad7682"
 	* "adi,ad7689"
  - reg: spi chip select number for the device
- - vref-supply: The regulator supply for ADC reference voltage
 
-Example:
+Optional properties:
+ - adi,reference-select: Select the reference voltage source to use
+ when converting the input voltages. Valid values are:
+   0: Internal 2.5V reference; temperature sensor enabled
+   1: Internal 4.096V reference; temperature sensor enabled
+   2: External reference, temperature sensor enabled, no buffer
+   3: External reference, temperature sensor enabled, buffer enabled
+   6: External reference, temperature sensor disabled, no buffer
+   7: External reference, temperature sensor disabled, buffer enabled
+ - vref-supply: The regulator supply for ADC reference voltage. Required
+ if external reference selected by 'adi,reference-select'.
+
+Examples:
 adc@0 {
 	compatible = "adi,ad7949";
 	reg = <0>;
+	adi,reference-select = <2>;
 	vref-supply = <&vdd_supply>;
 };
+
+adc@0 {
+	compatible = "adi,ad7949";
+	reg = <0>;
+	adi,reference-select = <0>;
+};
-- 
1.9.1

[PATCH v2 3/6] iio: ad7949: Support configuration read-back

[Adam Michaelis]

Adds device tree parameter to set the configuration read-back bit
in the configuration register to tell the AD7949 to include the value of
the configuration register at the time the current sample was acquired
when reading from the part.

Further work must be done to make read-back information available to
consumer.

Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
	V2: Add some defines to reduce use of magic numbers.
---
 drivers/iio/adc/ad7949.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index afc1361af5fb..7820e1097787 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -69,6 +69,7 @@ struct ad7949_adc_spec {
  * @iio_dev: reference to iio structure
  * @spi: reference to spi structure
  * @ref_sel: selected reference voltage source
+ * @cfg_readback: whether reads will include configuration data
  * @resolution: resolution of the chip
  * @cfg: copy of the configuration register
  * @current_channel: current channel in use
@@ -80,6 +81,7 @@ struct ad7949_adc_chip {
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
 	enum ad7949_ref_sel ref_sel;
+	bool cfg_readback;
 	u8 resolution;
 	u16 cfg;
 	unsigned int current_channel;
@@ -283,7 +285,11 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 			AD7949_CFG_REF_SEL_MASK;
 	adc_config |= (AD7949_CFG_SEQ_DISABLED << AD7949_CFG_SEQ_SHIFT) &
 			AD7949_CFG_SEQ_MASK;
-	adc_config |= AD7949_CFG_READBACK_DIS;
+
+	if (ad7949_adc->cfg_readback)
+		adc_config |= AD7949_CFG_READBACK_EN;
+	else
+		adc_config |= AD7949_CFG_READBACK_DIS;
 
 	ret = ad7949_spi_write_cfg(ad7949_adc,
 			adc_config,
@@ -331,6 +337,10 @@ static int ad7949_spi_probe(struct spi_device *spi)
 	indio_dev->num_channels = spec->num_channels;
 	ad7949_adc->resolution = spec->resolution;
 
+	ad7949_adc->cfg_readback = of_property_read_bool(
+			ad7949_adc->indio_dev->dev.of_node,
+			"adi,cfg-readback");
+
 	ret = of_property_read_u32(ad7949_adc->indio_dev->dev.of_node,
 			"adi,reference-select",
 			&temp);
-- 
1.9.1

Re: [PATCH v2 3/6] iio: ad7949: Support configuration read-back

[Jonathan Cameron]

On Tue, 7 May 2019 14:53:32 -0500
Adam Michaelis <adam.michaelis@collins.com> wrote:

> On Sun, May 5, 2019 at 9:42 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu,  2 May 2019 11:14:29 -0500
> > Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:
> >  
> > > Adds device tree parameter to set the configuration read-back bit
> > > in the configuration register to tell the AD7949 to include the value of
> > > the configuration register at the time the current sample was acquired
> > > when reading from the part.
> > >
> > > Further work must be done to make read-back information available to
> > > consumer.  
> >
> > This needs some explanation of why it is useful at all. I'm certainly unclear
> > on why it would be useful to configure this at boot time.
> >
> > Code looks fine.
> >
> > Jonathan
> >  
> The configuration read-back feature is being maintained from the
> original version of this driver. Before adding the device tree entry,
> there was no way to change this setting other than debugfs raw access
> to the SPI interface, and there is still no access to the returned
> configuration data should the feature be enabled. I would be willing
> to remove the feature altogether, but wanted to tread softly on
> existing features.
Ah. Makes sense.  My gut feeling is to drop it.

Anyone at Analog have a view on this?

Thanks,

Jonathan

> 
> Adam
> > >
> > > Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> > > ---
> > >       V2: Add some defines to reduce use of magic numbers.
> > > ---
> > >  drivers/iio/adc/ad7949.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > > index afc1361af5fb..7820e1097787 100644
> > > --- a/drivers/iio/adc/ad7949.c
> > > +++ b/drivers/iio/adc/ad7949.c
> > > @@ -69,6 +69,7 @@ struct ad7949_adc_spec {
> > >   * @iio_dev: reference to iio structure
> > >   * @spi: reference to spi structure
> > >   * @ref_sel: selected reference voltage source
> > > + * @cfg_readback: whether reads will include configuration data
> > >   * @resolution: resolution of the chip
> > >   * @cfg: copy of the configuration register
> > >   * @current_channel: current channel in use
> > > @@ -80,6 +81,7 @@ struct ad7949_adc_chip {
> > >       struct iio_dev *indio_dev;
> > >       struct spi_device *spi;
> > >       enum ad7949_ref_sel ref_sel;
> > > +     bool cfg_readback;
> > >       u8 resolution;
> > >       u16 cfg;
> > >       unsigned int current_channel;
> > > @@ -283,7 +285,11 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
> > >                       AD7949_CFG_REF_SEL_MASK;
> > >       adc_config |= (AD7949_CFG_SEQ_DISABLED << AD7949_CFG_SEQ_SHIFT) &
> > >                       AD7949_CFG_SEQ_MASK;
> > > -     adc_config |= AD7949_CFG_READBACK_DIS;
> > > +
> > > +     if (ad7949_adc->cfg_readback)
> > > +             adc_config |= AD7949_CFG_READBACK_EN;
> > > +     else
> > > +             adc_config |= AD7949_CFG_READBACK_DIS;
> > >
> > >       ret = ad7949_spi_write_cfg(ad7949_adc,
> > >                       adc_config,
> > > @@ -331,6 +337,10 @@ static int ad7949_spi_probe(struct spi_device *spi)
> > >       indio_dev->num_channels = spec->num_channels;
> > >       ad7949_adc->resolution = spec->resolution;
> > >
> > > +     ad7949_adc->cfg_readback = of_property_read_bool(
> > > +                     ad7949_adc->indio_dev->dev.of_node,
> > > +                     "adi,cfg-readback");
> > > +
> > >       ret = of_property_read_u32(ad7949_adc->indio_dev->dev.of_node,
> > >                       "adi,reference-select",
> > >                       &temp);  
> >  
> 
> --

Re: [PATCH v2 3/6] iio: ad7949: Support configuration read-back

[Popa, Stefan Serban]

On Sb, 2019-05-11 at 11:31 +0100, Jonathan Cameron wrote:
> [External]
> 
> 
> On Tue, 7 May 2019 14:53:32 -0500
> Adam Michaelis <adam.michaelis@collins.com> wrote:
> 
> > 
> > On Sun, May 5, 2019 at 9:42 AM Jonathan Cameron <jic23@kernel.org>
> > wrote:
> > > 
> > > 
> > > On Thu,  2 May 2019 11:14:29 -0500
> > > Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:
> > > 
> > > > 
> > > > Adds device tree parameter to set the configuration read-back bit
> > > > in the configuration register to tell the AD7949 to include the
> > > > value of
> > > > the configuration register at the time the current sample was
> > > > acquired
> > > > when reading from the part.
> > > > 
> > > > Further work must be done to make read-back information available
> > > > to
> > > > consumer.
> > > This needs some explanation of why it is useful at all. I'm certainly
> > > unclear
> > > on why it would be useful to configure this at boot time.
> > > 
> > > Code looks fine.
> > > 
> > > Jonathan
> > > 
> > The configuration read-back feature is being maintained from the
> > original version of this driver. Before adding the device tree entry,
> > there was no way to change this setting other than debugfs raw access
> > to the SPI interface, and there is still no access to the returned
> > configuration data should the feature be enabled. I would be willing
> > to remove the feature altogether, but wanted to tread softly on
> > existing features.
> Ah. Makes sense.  My gut feeling is to drop it.
> 
> Anyone at Analog have a view on this?
> 
> Thanks,
> 
> Jonathan
> 

Hi, 

It is not obvious to me why this feature would be useful. I would not add
it.

Regards,
-Stefan

> > 
> > 
> > Adam
> > > 
> > > > 
> > > > 
> > > > Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> > > > ---
> > > >       V2: Add some defines to reduce use of magic numbers.
> > > > ---
> > > >  drivers/iio/adc/ad7949.c | 12 +++++++++++-
> > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > > > index afc1361af5fb..7820e1097787 100644
> > > > --- a/drivers/iio/adc/ad7949.c
> > > > +++ b/drivers/iio/adc/ad7949.c
> > > > @@ -69,6 +69,7 @@ struct ad7949_adc_spec {
> > > >   * @iio_dev: reference to iio structure
> > > >   * @spi: reference to spi structure
> > > >   * @ref_sel: selected reference voltage source
> > > > + * @cfg_readback: whether reads will include configuration data
> > > >   * @resolution: resolution of the chip
> > > >   * @cfg: copy of the configuration register
> > > >   * @current_channel: current channel in use
> > > > @@ -80,6 +81,7 @@ struct ad7949_adc_chip {
> > > >       struct iio_dev *indio_dev;
> > > >       struct spi_device *spi;
> > > >       enum ad7949_ref_sel ref_sel;
> > > > +     bool cfg_readback;
> > > >       u8 resolution;
> > > >       u16 cfg;
> > > >       unsigned int current_channel;
> > > > @@ -283,7 +285,11 @@ static int ad7949_spi_init(struct
> > > > ad7949_adc_chip *ad7949_adc)
> > > >                       AD7949_CFG_REF_SEL_MASK;
> > > >       adc_config |= (AD7949_CFG_SEQ_DISABLED <<
> > > > AD7949_CFG_SEQ_SHIFT) &
> > > >                       AD7949_CFG_SEQ_MASK;
> > > > -     adc_config |= AD7949_CFG_READBACK_DIS;
> > > > +
> > > > +     if (ad7949_adc->cfg_readback)
> > > > +             adc_config |= AD7949_CFG_READBACK_EN;
> > > > +     else
> > > > +             adc_config |= AD7949_CFG_READBACK_DIS;
> > > > 
> > > >       ret = ad7949_spi_write_cfg(ad7949_adc,
> > > >                       adc_config,
> > > > @@ -331,6 +337,10 @@ static int ad7949_spi_probe(struct spi_device
> > > > *spi)
> > > >       indio_dev->num_channels = spec->num_channels;
> > > >       ad7949_adc->resolution = spec->resolution;
> > > > 
> > > > +     ad7949_adc->cfg_readback = of_property_read_bool(
> > > > +                     ad7949_adc->indio_dev->dev.of_node,
> > > > +                     "adi,cfg-readback");
> > > > +
> > > >       ret = of_property_read_u32(ad7949_adc->indio_dev-
> > > > >dev.of_node,
> > > >                       "adi,reference-select",
> > > >                       &temp);
> > --

Re: [External] Re: [PATCH v2 3/6] iio: ad7949: Support configuration read-back

[Adam Michaelis]

On Mon, May 13, 2019 at 5:04 AM Popa, Stefan Serban
<StefanSerban.Popa@analog.com> wrote:
>
> On Sb, 2019-05-11 at 11:31 +0100, Jonathan Cameron wrote:
> > [External]
> >
> >
> > On Tue, 7 May 2019 14:53:32 -0500
> > Adam Michaelis <adam.michaelis@collins.com> wrote:
> >
> > >
> > > On Sun, May 5, 2019 at 9:42 AM Jonathan Cameron <jic23@kernel.org>
> > > wrote:
> > > >
> > > >
> > > > On Thu,  2 May 2019 11:14:29 -0500
> > > > Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:
> > > >
> > > > >
> > > > > Adds device tree parameter to set the configuration read-back bit
> > > > > in the configuration register to tell the AD7949 to include the
> > > > > value of
> > > > > the configuration register at the time the current sample was
> > > > > acquired
> > > > > when reading from the part.
> > > > >
> > > > > Further work must be done to make read-back information available
> > > > > to
> > > > > consumer.
> > > > This needs some explanation of why it is useful at all. I'm certainly
> > > > unclear
> > > > on why it would be useful to configure this at boot time.
> > > >
> > > > Code looks fine.
> > > >
> > > > Jonathan
> > > >
> > > The configuration read-back feature is being maintained from the
> > > original version of this driver. Before adding the device tree entry,
> > > there was no way to change this setting other than debugfs raw access
> > > to the SPI interface, and there is still no access to the returned
> > > configuration data should the feature be enabled. I would be willing
> > > to remove the feature altogether, but wanted to tread softly on
> > > existing features.
> > Ah. Makes sense.  My gut feeling is to drop it.
> >
> > Anyone at Analog have a view on this?
> >
> > Thanks,
> >
> > Jonathan
> >
>
> Hi,
>
> It is not obvious to me why this feature would be useful. I would not add
> it.
>
> Regards,
> -Stefan
>

I have removed this feature in V3 of the patch series.

Thank you for the feedback,
Adam

> > >
> > >
> > > Adam
> > > >
> > > > >
> > > > >
> > > > > Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> > > > > ---
> > > > >       V2: Add some defines to reduce use of magic numbers.
> > > > > ---
> > > > >  drivers/iio/adc/ad7949.c | 12 +++++++++++-
> > > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > > > > index afc1361af5fb..7820e1097787 100644
> > > > > --- a/drivers/iio/adc/ad7949.c
> > > > > +++ b/drivers/iio/adc/ad7949.c
> > > > > @@ -69,6 +69,7 @@ struct ad7949_adc_spec {
> > > > >   * @iio_dev: reference to iio structure
> > > > >   * @spi: reference to spi structure
> > > > >   * @ref_sel: selected reference voltage source
> > > > > + * @cfg_readback: whether reads will include configuration data
> > > > >   * @resolution: resolution of the chip
> > > > >   * @cfg: copy of the configuration register
> > > > >   * @current_channel: current channel in use
> > > > > @@ -80,6 +81,7 @@ struct ad7949_adc_chip {
> > > > >       struct iio_dev *indio_dev;
> > > > >       struct spi_device *spi;
> > > > >       enum ad7949_ref_sel ref_sel;
> > > > > +     bool cfg_readback;
> > > > >       u8 resolution;
> > > > >       u16 cfg;
> > > > >       unsigned int current_channel;
> > > > > @@ -283,7 +285,11 @@ static int ad7949_spi_init(struct
> > > > > ad7949_adc_chip *ad7949_adc)
> > > > >                       AD7949_CFG_REF_SEL_MASK;
> > > > >       adc_config |= (AD7949_CFG_SEQ_DISABLED <<
> > > > > AD7949_CFG_SEQ_SHIFT) &
> > > > >                       AD7949_CFG_SEQ_MASK;
> > > > > -     adc_config |= AD7949_CFG_READBACK_DIS;
> > > > > +
> > > > > +     if (ad7949_adc->cfg_readback)
> > > > > +             adc_config |= AD7949_CFG_READBACK_EN;
> > > > > +     else
> > > > > +             adc_config |= AD7949_CFG_READBACK_DIS;
> > > > >
> > > > >       ret = ad7949_spi_write_cfg(ad7949_adc,
> > > > >                       adc_config,
> > > > > @@ -331,6 +337,10 @@ static int ad7949_spi_probe(struct spi_device
> > > > > *spi)
> > > > >       indio_dev->num_channels = spec->num_channels;
> > > > >       ad7949_adc->resolution = spec->resolution;
> > > > >
> > > > > +     ad7949_adc->cfg_readback = of_property_read_bool(
> > > > > +                     ad7949_adc->indio_dev->dev.of_node,
> > > > > +                     "adi,cfg-readback");
> > > > > +
> > > > >       ret = of_property_read_u32(ad7949_adc->indio_dev-
> > > > > >dev.of_node,
> > > > >                       "adi,reference-select",
> > > > >                       &temp);
> > > --

Re: [PATCH v2 3/6] iio: ad7949: Support configuration read-back

[Couret Charles-Antoine]

Le 07/05/2019 à 21:53, Adam Michaelis a écrit :
> On Sun, May 5, 2019 at 9:42 AM Jonathan Cameron <jic23@kernel.org> wrote:
>> On Thu,  2 May 2019 11:14:29 -0500
>> Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:
>>
>>> Adds device tree parameter to set the configuration read-back bit
>>> in the configuration register to tell the AD7949 to include the value of
>>> the configuration register at the time the current sample was acquired
>>> when reading from the part.
>>>
>>> Further work must be done to make read-back information available to
>>> consumer.
>> This needs some explanation of why it is useful at all. I'm certainly unclear
>> on why it would be useful to configure this at boot time.
>>
>> Code looks fine.
>>
>> Jonathan
>>
> The configuration read-back feature is being maintained from the
> original version of this driver. Before adding the device tree entry,
> there was no way to change this setting other than debugfs raw access
> to the SPI interface, and there is still no access to the returned
> configuration data should the feature be enabled. I would be willing
> to remove the feature altogether, but wanted to tread softly on
> existing features.

Hi,

I added this feature for debug purpose but it is not used in our case 
anymore because the driver and the device are working as expected.

But maybe we can use it the check if the config is correctly applied? I 
don't know, it is probably useless to keep this feature here.

Regards,

Charles-Antoine Couret

[PATCH v2 4/6] dt-bindings: iio: ad7949: Add cfg-readback option

[Adam Michaelis]

Adding optional parameter to the ad7949 to indicate that the part should
be configured to include the configuration register value used when
acquiring the current sample value.

Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
	V2: Add some defines to reduce use of magic numbers.
---
 Documentation/devicetree/bindings/iio/adc/ad7949.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/ad7949.txt b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
index 14ee9a2cb2a5..cd3572828243 100644
--- a/Documentation/devicetree/bindings/iio/adc/ad7949.txt
+++ b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
@@ -18,6 +18,8 @@ Optional properties:
    7: External reference, temperature sensor disabled, buffer enabled
  - vref-supply: The regulator supply for ADC reference voltage. Required
  if external reference selected by 'adi,reference-select'.
+ - adi,cfg-readback: If defined, set bit in configuration register to
+ read-back the configuration used for the current sample value.
 
 Examples:
 adc@0 {
@@ -32,3 +34,10 @@ adc@0 {
 	reg = <0>;
 	adi,reference-select = <0>;
 };
+
+adc@0 {
+	compatible = "adi,ad7949";
+	reg = <0>;
+	adi,reference-select = <1>;
+	adi,cfg-readback;
+};
-- 
1.9.1

[PATCH v2 5/6] iio: ad7949: Fix SPI interfacing for 14-bit messages

[Adam Michaelis]

The AD7949 (but not the other two models supported by this driver) uses
samples 14 bits wide. When attempting to communicate through certain SPI
controllers that require power-of-2 word widths, this fails. Adding
logic to pack the 14-bit messages into the most-significant bits of a
16-bit message for communicating over byte-based SPI bus.

Only able to test with AD7949 part, but should support the 16-bit
samples of the AD7682 and AD7689, as well.

Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
	V2: Add some defines to reduce use of magic numbers.
---
 drivers/iio/adc/ad7949.c | 106 +++++++++++++++++++++++++++--------------------
 1 file changed, 60 insertions(+), 46 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 7820e1097787..cba152151908 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -33,6 +33,11 @@
 #define AD7949_CFG_READBACK_DIS            1
 #define AD7949_CFG_READBACK_MASK           GENMASK(0, 0)
 
+#define AD7949_BUFFER_LEN 4
+
+#define AD7949_HI_RESOLUTION 16
+#define AD7949_LO_RESOLUTION 14
+
 enum {
 	ID_AD7949 = 0,
 	ID_AD7682,
@@ -57,9 +62,9 @@ struct ad7949_adc_spec {
 };
 
 static const struct ad7949_adc_spec ad7949_adc_spec[] = {
-	[ID_AD7949] = { .num_channels = 8, .resolution = 14 },
-	[ID_AD7682] = { .num_channels = 4, .resolution = 16 },
-	[ID_AD7689] = { .num_channels = 8, .resolution = 16 },
+	[ID_AD7949] = { .num_channels = 8, .resolution = AD7949_LO_RESOLUTION },
+	[ID_AD7682] = { .num_channels = 4, .resolution = AD7949_HI_RESOLUTION },
+	[ID_AD7689] = { .num_channels = 8, .resolution = AD7949_HI_RESOLUTION },
 };
 
 /**
@@ -85,7 +90,7 @@ struct ad7949_adc_chip {
 	u8 resolution;
 	u16 cfg;
 	unsigned int current_channel;
-	u32 buffer ____cacheline_aligned;
+	u8 buffer[AD7949_BUFFER_LEN] ____cacheline_aligned;
 };
 
 static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
@@ -96,41 +101,51 @@ static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
 	return false;
 }
 
-static int ad7949_spi_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
+static int ad7949_message_len(struct ad7949_adc_chip *ad7949_adc)
 {
-	int ret = ad7949_adc->resolution;
+	int tx_len = 2;
 
 	if (ad7949_spi_cfg_is_read_back(ad7949_adc))
-		ret += AD7949_CFG_REG_SIZE_BITS;
+		tx_len += 2;
 
-	return ret;
+	return tx_len;
 }
 
 static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
 				u16 mask)
 {
-	int ret;
-	int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
-	int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
+	int ret = 0;
 	struct spi_message msg;
-	struct spi_transfer tx[] = {
-		{
-			.tx_buf = &ad7949_adc->buffer,
-			.len = 4,
-			.bits_per_word = bits_per_word,
-		},
-	};
-
-	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
-	ad7949_adc->buffer = (ad7949_adc->cfg & AD7949_CFG_MASK_TOTAL) << shift;
-	spi_message_init_with_transfers(&msg, tx, 1);
-	ret = spi_sync(ad7949_adc->spi, &msg);
+	struct spi_transfer tx;
+	u16 tmp_cfg = 0;
+
+	(void)memset(&tx, 0, sizeof(tx));
+	(void)memset(ad7949_adc->buffer, 0, AD7949_BUFFER_LEN);
+
+	tmp_cfg = ((val & mask) | (ad7949_adc->cfg & ~mask)) &
+		AD7949_CFG_MASK_TOTAL;
+
+	if (tmp_cfg != ad7949_adc->cfg) {
+		ad7949_adc->cfg = tmp_cfg;
+
+		/* Pack 14-bit value into 2 bytes, MSB first */
+		ad7949_adc->buffer[0] = ((ad7949_adc->cfg & 0x7f00) >> 6) |
+			((ad7949_adc->cfg & 0xc0) >> 6);
+		ad7949_adc->buffer[1] = (ad7949_adc->cfg & 0x007f) << 2;
+
+		tx.tx_buf = ad7949_adc->buffer;
+		tx.len = ad7949_message_len(ad7949_adc);
+
+		spi_message_init_with_transfers(&msg, &tx, 1);
+		ret = spi_sync(ad7949_adc->spi, &msg);
+
+		/*
+		 * This delay is to avoid a new request before the required
+		 * time to send a new command to the device
+		 */
+		udelay(2);
+	}
 
-	/*
-	 * This delay is to avoid a new request before the required time to
-	 * send a new command to the device
-	 */
-	udelay(2);
 	return ret;
 }
 
@@ -138,16 +153,10 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 				   unsigned int channel)
 {
 	int ret;
-	int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
-	int mask = GENMASK(ad7949_adc->resolution, 0);
 	struct spi_message msg;
-	struct spi_transfer tx[] = {
-		{
-			.rx_buf = &ad7949_adc->buffer,
-			.len = 4,
-			.bits_per_word = bits_per_word,
-		},
-	};
+	struct spi_transfer tx;
+
+	ad7949_adc->current_channel = channel;
 
 	ret = ad7949_spi_write_cfg(ad7949_adc,
 				   channel << AD7949_CFG_CHAN_SEL_SHIFT,
@@ -155,24 +164,29 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	if (ret)
 		return ret;
 
-	ad7949_adc->buffer = 0;
-	spi_message_init_with_transfers(&msg, tx, 1);
+	(void)memset(&tx, 0, sizeof(tx));
+	(void)memset(ad7949_adc->buffer, 0, AD7949_BUFFER_LEN);
+
+	tx.rx_buf = ad7949_adc->buffer;
+	tx.len = ad7949_message_len(ad7949_adc);
+
+	spi_message_init_with_transfers(&msg, &tx, 1);
 	ret = spi_sync(ad7949_adc->spi, &msg);
 	if (ret)
 		return ret;
 
 	/*
-	 * This delay is to avoid a new request before the required time to
-	 * send a new command to the device
+	 * This delay is to avoid a new request before the required time
+	 * to send a new command to the device.
 	 */
 	udelay(2);
 
-	ad7949_adc->current_channel = channel;
+	*val = (ad7949_adc->buffer[0] << 8) | ad7949_adc->buffer[1];
 
-	if (ad7949_spi_cfg_is_read_back(ad7949_adc))
-		*val = (ad7949_adc->buffer >> AD7949_CFG_REG_SIZE_BITS) & mask;
-	else
-		*val = ad7949_adc->buffer & mask;
+	if (ad7949_adc->resolution == AD7949_LO_RESOLUTION) {
+		/* 14-bit value in 16-bit buffer */
+		*val = *val >> 2;
+	}
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH v2 5/6] iio: ad7949: Fix SPI interfacing for 14-bit messages

[Jonathan Cameron]

On Thu,  2 May 2019 11:14:31 -0500
Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:

> The AD7949 (but not the other two models supported by this driver) uses
> samples 14 bits wide. When attempting to communicate through certain SPI
> controllers that require power-of-2 word widths, this fails. Adding
> logic to pack the 14-bit messages into the most-significant bits of a
> 16-bit message for communicating over byte-based SPI bus.

So this does penalise controllers that do support 14 bits. Can we query
that and perhaps look at only padding if necessary?

> 
> Only able to test with AD7949 part, but should support the 16-bit
> samples of the AD7682 and AD7689, as well.
> 
> Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
This has ended up more complex that I expected.  I 'think' the
result is just to shift the data up two bits if needed to pad?

There are some endian issues introduced as well by this patch.

Jonathan


> ---
> 	V2: Add some defines to reduce use of magic numbers.
> ---
>  drivers/iio/adc/ad7949.c | 106 +++++++++++++++++++++++++++--------------------
>  1 file changed, 60 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index 7820e1097787..cba152151908 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -33,6 +33,11 @@
>  #define AD7949_CFG_READBACK_DIS            1
>  #define AD7949_CFG_READBACK_MASK           GENMASK(0, 0)
>  
> +#define AD7949_BUFFER_LEN 4
> +
> +#define AD7949_HI_RESOLUTION 16
> +#define AD7949_LO_RESOLUTION 14
> +
>  enum {
>  	ID_AD7949 = 0,
>  	ID_AD7682,
> @@ -57,9 +62,9 @@ struct ad7949_adc_spec {
>  };
>  
>  static const struct ad7949_adc_spec ad7949_adc_spec[] = {
> -	[ID_AD7949] = { .num_channels = 8, .resolution = 14 },
> -	[ID_AD7682] = { .num_channels = 4, .resolution = 16 },
> -	[ID_AD7689] = { .num_channels = 8, .resolution = 16 },
> +	[ID_AD7949] = { .num_channels = 8, .resolution = AD7949_LO_RESOLUTION },
> +	[ID_AD7682] = { .num_channels = 4, .resolution = AD7949_HI_RESOLUTION },
> +	[ID_AD7689] = { .num_channels = 8, .resolution = AD7949_HI_RESOLUTION },
This obscures a 'non magic' number. It is better to just leave it as how
many bits it actually is.

>  };
>  
>  /**
> @@ -85,7 +90,7 @@ struct ad7949_adc_chip {
>  	u8 resolution;
>  	u16 cfg;
>  	unsigned int current_channel;
> -	u32 buffer ____cacheline_aligned;
> +	u8 buffer[AD7949_BUFFER_LEN] ____cacheline_aligned;
Why this change?  Ah, not using bits_per_word any more..

>  };
>  
>  static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
> @@ -96,41 +101,51 @@ static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
>  	return false;
>  }
>  
> -static int ad7949_spi_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
> +static int ad7949_message_len(struct ad7949_adc_chip *ad7949_adc)
>  {
> -	int ret = ad7949_adc->resolution;
> +	int tx_len = 2;
>  
>  	if (ad7949_spi_cfg_is_read_back(ad7949_adc))
> -		ret += AD7949_CFG_REG_SIZE_BITS;
> +		tx_len += 2;
>  
> -	return ret;
> +	return tx_len;
>  }
>  
>  static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
>  				u16 mask)
>  {
> -	int ret;
> -	int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
> -	int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
> +	int ret = 0;
>  	struct spi_message msg;
> -	struct spi_transfer tx[] = {
> -		{
> -			.tx_buf = &ad7949_adc->buffer,
> -			.len = 4,
> -			.bits_per_word = bits_per_word,
> -		},
> -	};
> -
> -	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
> -	ad7949_adc->buffer = (ad7949_adc->cfg & AD7949_CFG_MASK_TOTAL) << shift;
> -	spi_message_init_with_transfers(&msg, tx, 1);
> -	ret = spi_sync(ad7949_adc->spi, &msg);
> +	struct spi_transfer tx;
> +	u16 tmp_cfg = 0;
> +
> +	(void)memset(&tx, 0, sizeof(tx));
> +	(void)memset(ad7949_adc->buffer, 0, AD7949_BUFFER_LEN);
The void casts do nothing useful.

memset(ad7949_adc->spi, sizeof(ad7949_adc->spi)) is better.

however, I'm not clear why you can't use a similar c99 assignment to that
the driver previously did.
> +
> +	tmp_cfg = ((val & mask) | (ad7949_adc->cfg & ~mask)) &
> +		AD7949_CFG_MASK_TOTAL;
> +
> +	if (tmp_cfg != ad7949_adc->cfg) {
Flip the logic here and return early if it hasn't changed.

> +		ad7949_adc->cfg = tmp_cfg;
> +
> +		/* Pack 14-bit value into 2 bytes, MSB first */
> +		ad7949_adc->buffer[0] = ((ad7949_adc->cfg & 0x7f00) >> 6) |
> +			((ad7949_adc->cfg & 0xc0) >> 6);
So this:
takes bits 13..8 and shifts them down to 7..2.
takes bits 7..6 and shifts them down to 1..0

Unless I'm missing something that's just 13..6 shifted down to 7..0
(ad7949_adc->cfg & GENMASK(13, 6)) >> 6 or something like that.

> +		ad7949_adc->buffer[1] = (ad7949_adc->cfg & 0x007f) << 2;

Use GENMASK here as well.

This looks like a shift up by 2 followed by a endian swap. Can we make
that explicit as it'll be more readable...

I'm a little worried that we have dropped the endian swap that was
effectively occuring due to the larger bits_per_word for a version
that always does it, whether or not we are on a big endian platform
or a little endian one.

From the spi.h header

 * In-memory data values are always in native CPU byte order, translated
 * from the wire byte order (big-endian except with SPI_LSB_FIRST).  So
 * for example when bits_per_word is sixteen, buffers are 2N bytes long
 * (@len = 2N) and hold N sixteen bit words in CPU byte order.
 *

As you have it the bits_per_word is 8 and so there is no inbuilt assumption
of ordering and you need you need to swap as you are on a little endian
platform.

> +
> +		tx.tx_buf = ad7949_adc->buffer;
> +		tx.len = ad7949_message_len(ad7949_adc);
> +
> +		spi_message_init_with_transfers(&msg, &tx, 1);

spi_write?

> +		ret = spi_sync(ad7949_adc->spi, &msg);
> +
> +		/*
> +		 * This delay is to avoid a new request before the required
> +		 * time to send a new command to the device
> +		 */
> +		udelay(2);
> +	}
>  
> -	/*
> -	 * This delay is to avoid a new request before the required time to
> -	 * send a new command to the device
> -	 */
> -	udelay(2);
>  	return ret;
>  }
>  
> @@ -138,16 +153,10 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>  				   unsigned int channel)
>  {
>  	int ret;
> -	int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
> -	int mask = GENMASK(ad7949_adc->resolution, 0);
>  	struct spi_message msg;
> -	struct spi_transfer tx[] = {
> -		{
> -			.rx_buf = &ad7949_adc->buffer,
> -			.len = 4,
> -			.bits_per_word = bits_per_word,
> -		},
> -	};
> +	struct spi_transfer tx;
> +
> +	ad7949_adc->current_channel = channel;
>  
>  	ret = ad7949_spi_write_cfg(ad7949_adc,
>  				   channel << AD7949_CFG_CHAN_SEL_SHIFT,
> @@ -155,24 +164,29 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>  	if (ret)
>  		return ret;
>  
> -	ad7949_adc->buffer = 0;
> -	spi_message_init_with_transfers(&msg, tx, 1);
> +	(void)memset(&tx, 0, sizeof(tx));

> +	(void)memset(ad7949_adc->buffer, 0, AD7949_BUFFER_LEN);
> +
> +	tx.rx_buf = ad7949_adc->buffer;
> +	tx.len = ad7949_message_len(ad7949_adc);
> +
> +	spi_message_init_with_transfers(&msg, &tx, 1);
spi_read?

>  	ret = spi_sync(ad7949_adc->spi, &msg);
>  	if (ret)
>  		return ret;
>  
>  	/*
> -	 * This delay is to avoid a new request before the required time to
> -	 * send a new command to the device
> +	 * This delay is to avoid a new request before the required time
> +	 * to send a new command to the device.
>  	 */
>  	udelay(2);
>  
> -	ad7949_adc->current_channel = channel;
> +	*val = (ad7949_adc->buffer[0] << 8) | ad7949_adc->buffer[1];
>  
> -	if (ad7949_spi_cfg_is_read_back(ad7949_adc))
> -		*val = (ad7949_adc->buffer >> AD7949_CFG_REG_SIZE_BITS) & mask;
> -	else
> -		*val = ad7949_adc->buffer & mask;
> +	if (ad7949_adc->resolution == AD7949_LO_RESOLUTION) {

14, much more readable as obvious what is happening.

> +		/* 14-bit value in 16-bit buffer */
> +		*val = *val >> 2;
> +	}
>  
>  	return 0;
>  }

[PATCH v2 6/6] iio: ad7949: Fix dummy read cycle placement

[Adam Michaelis]

The AD7949 requires two conversion cycles following any configuration
change (including changing the analog channel being sampled). Therefore,
adding a dummy read cycle when config is changed and removing the extra
cycles at initial configuration.

Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
	V2: Add some defines to reduce use of magic numbers.
---
 drivers/iio/adc/ad7949.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index cba152151908..5a6fe522c931 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -144,6 +144,25 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
 		 * time to send a new command to the device
 		 */
 		udelay(2);
+
+		/*
+		 * Perform extra read cycle to allow configuration, acquisition,
+		 * and conversion sequences to complete for new configuration.
+		 */
+		(void)memset(&tx, 0, sizeof(tx));
+		(void)memset(ad7949_adc->buffer, 0, AD7949_BUFFER_LEN);
+
+		tx.rx_buf = ad7949_adc->buffer;
+		tx.len = ad7949_message_len(ad7949_adc);
+
+		spi_message_init_with_transfers(&msg, &tx, 1);
+		ret = spi_sync(ad7949_adc->spi, &msg);
+
+		/*
+		 * This delay is to avoid a new request before the required time
+		 * to send a new command to the device.
+		 */
+		udelay(2);
 	}
 
 	return ret;
@@ -276,7 +295,6 @@ static int ad7949_spi_reg_access(struct iio_dev *indio_dev,
 static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 {
 	int ret;
-	int val;
 	u16 adc_config = 0;
 
 	ad7949_adc->current_channel = 0;
@@ -309,13 +327,6 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 			adc_config,
 			AD7949_CFG_MASK_TOTAL);
 
-	/*
-	 * Do two dummy conversions to apply the first configuration setting.
-	 * Required only after the start up of the device.
-	 */
-	ad7949_spi_read_channel(ad7949_adc, &val, ad7949_adc->current_channel);
-	ad7949_spi_read_channel(ad7949_adc, &val, ad7949_adc->current_channel);
-
 	return ret;
 }
 
-- 
1.9.1

Re: [PATCH v2 1/6] iio: ad7949: Support internal Vref

[Jonathan Cameron]

On Thu,  2 May 2019 11:14:27 -0500
Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:

> Adding configurable (via device tree) options to select either an external
> reference voltage (default, original implementation) or one of the two
> internal reference voltages provided by the AD7949 part family.
> 
> Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
Hi Adam,

Clearly the comments I made on the DT patch will follow through to changes
in here.

A few minor comments below.

Jonathan
> ---
> 	V2: Add some defines to reduce use of magic numbers.
> ---
>  drivers/iio/adc/ad7949.c | 135 +++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 108 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index ac0ffff6c5ae..afc1361af5fb 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -11,12 +11,27 @@
>  #include <linux/module.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
> -
> -#define AD7949_MASK_CHANNEL_SEL		GENMASK(9, 7)
> -#define AD7949_MASK_TOTAL		GENMASK(13, 0)
> -#define AD7949_OFFSET_CHANNEL_SEL	7
> -#define AD7949_CFG_READ_BACK		0x1
> -#define AD7949_CFG_REG_SIZE_BITS	14
> +#include <linux/of.h>
> +
> +#define AD7949_CFG_REG_SIZE_BITS           14
> +#define AD7949_CFG_MASK_TOTAL              GENMASK(13, 0)
> +#define AD7949_CFG_OVERWRITE_SHIFT         13
> +#define AD7949_CFG_CHAN_CFG_UNIPOLAR_GND   0x7
> +#define AD7949_CFG_CHAN_CFG_MASK           GENMASK(12, 10)
> +#define AD7949_CFG_CHAN_CFG_SHIFT          10
> +#define AD7949_CFG_CHAN_SEL_MASK           GENMASK(9, 7)
> +#define AD7949_CFG_CHAN_SEL_SHIFT          7
> +#define AD7949_CFG_BW_FULL                 1
> +#define AD7949_CFG_BW_SHIFT                6
> +#define AD7949_CFG_BW_MASK                 GENMASK(6, 6)
> +#define AD7949_CFG_REF_SEL_MASK            GENMASK(5, 3)
> +#define AD7949_CFG_REF_SEL_SHIFT           3
> +#define AD7949_CFG_SEQ_DISABLED            0x0
> +#define AD7949_CFG_SEQ_SHIFT               1
> +#define AD7949_CFG_SEQ_MASK                GENMASK(2, 1)
Where possible use the FIELD_PREP and FIELD_GET macros
to avoid needing to define both a _SHIFT and a _MASK.

IIRC the mask needs to be a compile time constant, but I think that
is always true with these.

> +#define AD7949_CFG_READBACK_EN             0
> +#define AD7949_CFG_READBACK_DIS            1
> +#define AD7949_CFG_READBACK_MASK           GENMASK(0, 0)
>  
>  enum {
>  	ID_AD7949 = 0,
> @@ -24,6 +39,18 @@ enum {
>  	ID_AD7689,
>  };
>  
> +enum ad7949_ref_sel {
> +	AD7949_REF_2V5 = 0, /* 2.5V internal ref + temp sensor */
> +	AD7949_REF_4V0, /* 4.096V internal ref + temp sensor */
> +	AD7949_REF_EXT_TEMP, /* External ref + temp sensor, no buffer */
> +	AD7949_REF_EXT_TEMP_BUF, /* External ref + temp sensor + buffer */
> +	AD7949_REF_RSRV_4,
> +	AD7949_REF_RSRV_5,
> +	AD7949_REF_EXT, /* External ref, no temp, no buffer */
> +	AD7949_REF_EXT_BUF, /* External ref + buffer, no temp */
> +	AD7949_REF_MAX,
> +};
> +
>  struct ad7949_adc_spec {
>  	u8 num_channels;
>  	u8 resolution;
> @@ -41,6 +68,7 @@ struct ad7949_adc_spec {
>   * @vref: regulator generating Vref
>   * @iio_dev: reference to iio structure
>   * @spi: reference to spi structure
> + * @ref_sel: selected reference voltage source
>   * @resolution: resolution of the chip
>   * @cfg: copy of the configuration register
>   * @current_channel: current channel in use
> @@ -51,6 +79,7 @@ struct ad7949_adc_chip {
>  	struct regulator *vref;
>  	struct iio_dev *indio_dev;
>  	struct spi_device *spi;
> +	enum ad7949_ref_sel ref_sel;
>  	u8 resolution;
>  	u16 cfg;
>  	unsigned int current_channel;
> @@ -59,7 +88,7 @@ struct ad7949_adc_chip {
>  
>  static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
>  {
> -	if (!(ad7949_adc->cfg & AD7949_CFG_READ_BACK))
> +	if (!(ad7949_adc->cfg & AD7949_CFG_READBACK_MASK))
>  		return true;
>  
>  	return false;
> @@ -91,7 +120,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
>  	};
>  
>  	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
> -	ad7949_adc->buffer = ad7949_adc->cfg << shift;
> +	ad7949_adc->buffer = (ad7949_adc->cfg & AD7949_CFG_MASK_TOTAL) << shift;
>  	spi_message_init_with_transfers(&msg, tx, 1);
>  	ret = spi_sync(ad7949_adc->spi, &msg);
>  
> @@ -119,8 +148,8 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>  	};
>  
>  	ret = ad7949_spi_write_cfg(ad7949_adc,
> -				   channel << AD7949_OFFSET_CHANNEL_SEL,
> -				   AD7949_MASK_CHANNEL_SEL);
> +				   channel << AD7949_CFG_CHAN_SEL_SHIFT,
> +				   AD7949_CFG_CHAN_SEL_MASK);
>  	if (ret)
>  		return ret;
>  
> @@ -187,11 +216,20 @@ static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT;
>  
>  	case IIO_CHAN_INFO_SCALE:
> -		ret = regulator_get_voltage(ad7949_adc->vref);
> -		if (ret < 0)
> -			return ret;
> +		if (ad7949_adc->vref) {
> +			ret = regulator_get_voltage(ad7949_adc->vref);
> +			if (ret < 0)
> +				return ret;
> +
> +			*val = ret / 5000;
> +		} else if (ad7949_adc->ref_sel == AD7949_REF_2V5) {
> +			*val = 2500;
> +		} else if (ad7949_adc->ref_sel == AD7949_REF_4V0) {
> +			*val = 4096;
> +		} else {
> +			return -EINVAL;
> +		}
>  
> -		*val = ret / 5000;
>  		return IIO_VAL_INT;
>  	}
>  
> @@ -209,7 +247,7 @@ static int ad7949_spi_reg_access(struct iio_dev *indio_dev,
>  		*readval = ad7949_adc->cfg;
>  	else
>  		ret = ad7949_spi_write_cfg(ad7949_adc,
> -			writeval & AD7949_MASK_TOTAL, AD7949_MASK_TOTAL);
> +			writeval & AD7949_CFG_MASK_TOTAL, AD7949_CFG_MASK_TOTAL);
>  
>  	return ret;
>  }
> @@ -223,10 +261,33 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
>  {
>  	int ret;
>  	int val;
> +	u16 adc_config = 0;
>  
> -	/* Sequencer disabled, CFG readback disabled, IN0 as default channel */
>  	ad7949_adc->current_channel = 0;
> -	ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
> +	ad7949_adc->cfg = 0;
> +
> +	/*
> +	 * 14-bit configuration register (pg 12 of AD7949 Datasheet):
> +	 * | 13  | 12 - 10 | 9 - 7 | 6  | 5 -3 | 2 - 1 | 0  |
> +	 * | CFG | INCC    | INx   | BW | REF  | SEQ   | RB |
> +	 */
Given this is now all opaque (as it should be) the comment isn't really
very useful any more so I'd drop it.

> +	adc_config = 1 << AD7949_CFG_OVERWRITE_SHIFT;
> +	adc_config |=
> +		(AD7949_CFG_CHAN_CFG_UNIPOLAR_GND << AD7949_CFG_CHAN_CFG_SHIFT)
> +			& AD7949_CFG_CHAN_CFG_MASK;
> +	adc_config |= (ad7949_adc->current_channel << AD7949_CFG_CHAN_SEL_SHIFT)
> +			& AD7949_CFG_CHAN_SEL_MASK;
> +	adc_config |= (AD7949_CFG_BW_FULL << AD7949_CFG_BW_SHIFT) &
> +			AD7949_CFG_BW_MASK;
> +	adc_config |= (ad7949_adc->ref_sel << AD7949_CFG_REF_SEL_SHIFT) &
> +			AD7949_CFG_REF_SEL_MASK;
> +	adc_config |= (AD7949_CFG_SEQ_DISABLED << AD7949_CFG_SEQ_SHIFT) &
> +			AD7949_CFG_SEQ_MASK;

Perhaps could used FIELD_PREP to do this in a cleaner fashion.

> +	adc_config |= AD7949_CFG_READBACK_DIS;
> +
> +	ret = ad7949_spi_write_cfg(ad7949_adc,
> +			adc_config,
> +			AD7949_CFG_MASK_TOTAL);
>  
>  	/*
>  	 * Do two dummy conversions to apply the first configuration setting.
> @@ -245,6 +306,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
>  	struct ad7949_adc_chip *ad7949_adc;
>  	struct iio_dev *indio_dev;
>  	int ret;
> +	u32 temp;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
>  	if (!indio_dev) {
> @@ -263,21 +325,38 @@ static int ad7949_spi_probe(struct spi_device *spi)
>  	ad7949_adc = iio_priv(indio_dev);
>  	ad7949_adc->indio_dev = indio_dev;
>  	ad7949_adc->spi = spi;
> +	ad7949_adc->vref = NULL;
>  
>  	spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
>  	indio_dev->num_channels = spec->num_channels;
>  	ad7949_adc->resolution = spec->resolution;
>  
> -	ad7949_adc->vref = devm_regulator_get(dev, "vref");
> -	if (IS_ERR(ad7949_adc->vref)) {
> -		dev_err(dev, "fail to request regulator\n");
> -		return PTR_ERR(ad7949_adc->vref);
> +	ret = of_property_read_u32(ad7949_adc->indio_dev->dev.of_node,
> +			"adi,reference-select",
> +			&temp);
> +	if ((ret == 0) && (temp < AD7949_REF_MAX) &&
> +			(temp != AD7949_REF_RSRV_4) &&
> +			(temp != AD7949_REF_RSRV_5)) {
> +		ad7949_adc->ref_sel = (enum ad7949_ref_sel)temp;
> +	} else {
> +		ad7949_adc->ref_sel = AD7949_REF_EXT_BUF;
> +		dev_warn(dev, "using external Vref by default\n");
>  	}
>  
> -	ret = regulator_enable(ad7949_adc->vref);
> -	if (ret < 0) {
> -		dev_err(dev, "fail to enable regulator\n");
> -		return ret;
> +	/* Check whether using external Vref */
> +	if ((ad7949_adc->ref_sel != AD7949_REF_2V5) &&
> +			(ad7949_adc->ref_sel != AD7949_REF_4V0)) {
> +		ad7949_adc->vref = devm_regulator_get(dev, "vref");
> +		if (IS_ERR(ad7949_adc->vref)) {
> +			dev_err(dev, "fail to request regulator\n");
> +			return PTR_ERR(ad7949_adc->vref);
> +		}
> +
> +		ret = regulator_enable(ad7949_adc->vref);
> +		if (ret < 0) {
> +			dev_err(dev, "fail to enable regulator\n");
> +			return ret;
> +		}
>  	}
>  
>  	mutex_init(&ad7949_adc->lock);
> @@ -298,7 +377,8 @@ static int ad7949_spi_probe(struct spi_device *spi)
>  
>  err:
>  	mutex_destroy(&ad7949_adc->lock);
> -	regulator_disable(ad7949_adc->vref);
> +	if (ad7949_adc->vref)
> +		regulator_disable(ad7949_adc->vref);
>  
>  	return ret;
>  }
> @@ -310,7 +390,8 @@ static int ad7949_spi_remove(struct spi_device *spi)
>  
>  	iio_device_unregister(indio_dev);
>  	mutex_destroy(&ad7949_adc->lock);
> -	regulator_disable(ad7949_adc->vref);
> +	if (ad7949_adc->vref)
> +		regulator_disable(ad7949_adc->vref);
>  
>  	return 0;
>  }