Skip to contentSkip to conversation

AD7949 Fixes

PATCH Superseded Liam Beguin 2 participants 13 messages Jul 8, 2021 – Jul 9, 2021

Subsystems (3)
Device tree bindings IIO Analog Devices IIO drivers

v1 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Liam Beguin

author
#0 lore raw

cover AD7949 Fixes

While working on another series[1] I ran into issues where my SPI controller would fail to handle 14-bit and 16-bit SPI messages. This addresses that issue and adds support for selecting a different voltage reference source from the devicetree.

This is base on a series[2] that seems to not have made it all the way, and was tested on an ad7689.

[1] https://patchwork.kernel.org/project/linux-iio/list/?series=511545
[2] https://patchwork.kernel.org/project/linux-iio/list/?series=116971&state=%2A&archive=both

Thanks for your time,
Liam

Liam Beguin (4):
iio: adc: ad7949: define and use bitfield names
iio: adc: ad7949: fix spi messages on non 14-bit controllers
iio: adc: ad7949: add support for internal vref
dt-bindings: iio: adc: ad7949: add adi,reference-source

 .../bindings/iio/adc/adi,ad7949.yaml          |  22 ++
 drivers/iio/adc/ad7949.c                      | 191 +++++++++++++++---
 2 files changed, 181 insertions(+), 32 deletions(-)

base-commit: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78

Signature · 1 line
2.30.1.489.g328c10930387
author
#1 lore raw

PATCH 1/4 iio: adc: ad7949: define and use bitfield names

From: Liam Beguin <[email protected]>

Replace raw configuration register values by using FIELD_PREP and defines to improve readability.

Signed-off-by Liam Beguin <[email protected]>
---
 drivers/iio/adc/ad7949.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)
drivers/iio/adc/ad7949.c +31 -7
index 1b4b3203e428..93aacf4f680b 100644
@@ -12,12 +12,27 @@
 #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
 
+#define AD7949_CFG_BIT_CFG		BIT(13)
+#define AD7949_CFG_VAL_CFG_OVERWRITE		1
+#define AD7949_CFG_VAL_CFG_KEEP			0
+#define AD7949_CFG_BIT_INCC		GENMASK(12, 10)
+#define AD7949_CFG_VAL_INCC_UNIPOLAR_GND	7
+#define AD7949_CFG_VAL_INCC_UNIPOLAR_COMM	6
+#define AD7949_CFG_VAL_INCC_UNIPOLAR_DIFF	4
+#define AD7949_CFG_VAL_INCC_TEMP		3
+#define AD7949_CFG_VAL_INCC_BIPOLAR		2
+#define AD7949_CFG_VAL_INCC_BIPOLAR_DIFF	0
+#define AD7949_CFG_BIT_INX		GENMASK(9, 7)
+#define AD7949_CFG_BIT_BW		BIT(6)
+#define AD7949_CFG_VAL_BW_FULL			1
+#define AD7949_CFG_VAL_BW_QUARTER		0
+#define AD7949_CFG_BIT_REF		GENMASK(5, 3)
+#define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
+#define AD7949_CFG_BIT_RBN		BIT(0)
+
 enum {
 	ID_AD7949 = 0,
 	ID_AD7682,

@@ -109,8 +124,8 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	 */
 	for (i = 0; i < 2; i++) {
 		ret = ad7949_spi_write_cfg(ad7949_adc,
-					   channel << AD7949_OFFSET_CHANNEL_SEL,
-					   AD7949_MASK_CHANNEL_SEL);
+					   FIELD_PREP(AD7949_CFG_BIT_INX, channel),
+					   AD7949_CFG_BIT_INX);
 		if (ret)
 			return ret;
 		if (channel == ad7949_adc->current_channel)

@@ -214,10 +229,19 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 {
 	int ret;
 	int val;
+	u16 cfg;
 
-	/* 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);
+
+	cfg = FIELD_PREP(AD7949_CFG_BIT_CFG, AD7949_CFG_VAL_CFG_OVERWRITE) |
+		FIELD_PREP(AD7949_CFG_BIT_INCC, AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
+		FIELD_PREP(AD7949_CFG_BIT_INX, ad7949_adc->current_channel) |
+		FIELD_PREP(AD7949_CFG_BIT_BW, AD7949_CFG_VAL_BW_FULL) |
+		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_REF_EXT_BUF) |
+		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
+		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);
+
+	ret = ad7949_spi_write_cfg(ad7949_adc, cfg, AD7949_MASK_TOTAL);
 
 	/*
 	 * Do two dummy conversions to apply the first configuration setting.
Signature · 1 line
2.30.1.489.g328c10930387
author
#2 lore raw

PATCH 2/4 iio: adc: ad7949: fix spi messages on non 14-bit controllers

In reply to Liam Beguin

From: Liam Beguin <[email protected]>

This driver supports devices with 14-bit and 16-bit sample sizes. This is not always handled properly by spi controllers and can fail. To work around this limitation, pad samples to 16-bit and split the sample into two 8-bit messages in the event that only 8-bit messages are supported by the controller.

Signed-off-by Liam Beguin <[email protected]>
---
 drivers/iio/adc/ad7949.c | 67 ++++++++++++++++++++++++++++++++++------
 1 file changed, 58 insertions(+), 9 deletions(-)
drivers/iio/adc/ad7949.c +58 -9
index 93aacf4f680b..bbc6b56330a3 100644
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
+#include <linux/bitfield.h>
 
 #define AD7949_MASK_TOTAL		GENMASK(13, 0)
 #define AD7949_CFG_REG_SIZE_BITS	14

@@ -57,6 +58,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
  * @indio_dev: reference to iio structure
  * @spi: reference to spi structure
  * @resolution: resolution of the chip
+ * @bits_per_word: number of bits per SPI word
  * @cfg: copy of the configuration register
  * @current_channel: current channel in use
  * @buffer: buffer to send / receive data to / from device

@@ -67,28 +69,59 @@ struct ad7949_adc_chip {
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
 	u8 resolution;
+	u8 bits_per_word;
 	u16 cfg;
 	unsigned int current_channel;
-	u16 buffer ____cacheline_aligned;
+	union {
+		__be16 buffer;
+		u8 buf8[2];
+	} ____cacheline_aligned;
 };
 
+static void ad7949_set_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
+{
+	u32 adc_mask = SPI_BPW_MASK(ad7949_adc->resolution);
+	u32 bpw = adc_mask & ad7949_adc->spi->controller->bits_per_word_mask;
+
+	if (bpw == adc_mask)
+		ad7949_adc->bits_per_word = ad7949_adc->resolution;
+	else if (bpw == SPI_BPW_MASK(16))
+		ad7949_adc->bits_per_word = 16;
+	else
+		ad7949_adc->bits_per_word = 8;
+}
+
 static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
 				u16 mask)
 {
 	int ret;
-	int bits_per_word = ad7949_adc->resolution;
-	int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
 	struct spi_message msg;
 	struct spi_transfer tx[] = {
 		{
 			.tx_buf = &ad7949_adc->buffer,
 			.len = 2,
-			.bits_per_word = bits_per_word,
+			.bits_per_word = ad7949_adc->bits_per_word,
 		},
 	};
 
+	ad7949_adc->buffer = 0;
 	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
-	ad7949_adc->buffer = ad7949_adc->cfg << shift;
+
+	switch (ad7949_adc->bits_per_word) {
+	case 16:
+		ad7949_adc->buffer = ad7949_adc->cfg << 2;
+		break;
+	case 14:
+		ad7949_adc->buffer = ad7949_adc->cfg;
+		break;
+	case 8:
+		/* Pack 14-bit value into 2 bytes, MSB first */
+		ad7949_adc->buf8[0] = FIELD_GET(GENMASK(13, 6), ad7949_adc->cfg);
+		ad7949_adc->buf8[1] = FIELD_GET(GENMASK(5, 0), ad7949_adc->cfg);
+		ad7949_adc->buf8[1] = ad7949_adc->buf8[1] << 2;
+		break;
+	}
+
 	spi_message_init_with_transfers(&msg, tx, 1);
 	ret = spi_sync(ad7949_adc->spi, &msg);
 

@@ -105,14 +138,12 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 {
 	int ret;
 	int i;
-	int bits_per_word = ad7949_adc->resolution;
-	int mask = GENMASK(ad7949_adc->resolution - 1, 0);
 	struct spi_message msg;
 	struct spi_transfer tx[] = {
 		{
 			.rx_buf = &ad7949_adc->buffer,
 			.len = 2,
-			.bits_per_word = bits_per_word,
+			.bits_per_word = ad7949_adc->bits_per_word,
 		},
 	};
 

@@ -147,7 +178,24 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 
 	ad7949_adc->current_channel = channel;
 
-	*val = ad7949_adc->buffer & mask;
+	switch (ad7949_adc->bits_per_word) {
+	case 16:
+		*val = ad7949_adc->buffer;
+		/* Shift-out padding bits */
+		if (ad7949_adc->resolution == 14)
+			*val = *val >> 2;
+		break;
+	case 14:
+		*val = ad7949_adc->buffer & GENMASK(13, 0);
+		break;
+	case 8:
+		/* Convert byte array to u16, MSB first */
+		*val = (ad7949_adc->buf8[0] << 8) | ad7949_adc->buf8[1];
+		/* Shift-out padding bits */
+		if (ad7949_adc->resolution == 14)
+			*val = *val >> 2;
+		break;
+	}
 
 	return 0;
 }

@@ -280,6 +328,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
 	spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
 	indio_dev->num_channels = spec->num_channels;
 	ad7949_adc->resolution = spec->resolution;
+	ad7949_set_bits_per_word(ad7949_adc);
 
 	ad7949_adc->vref = devm_regulator_get(dev, "vref");
 	if (IS_ERR(ad7949_adc->vref)) {
Signature · 1 line
2.30.1.489.g328c10930387
maintainer
#3 lore raw

RE: [PATCH v1 2/4] iio: adc: ad7949: fix spi messages on non 14-bit controllers

Quoted 114 lines

-----Original Message-----
From: Liam Beguin <[email protected]>
Sent: Friday, July 9, 2021 1:56 AM
To: [email protected]; [email protected]; Hennerich, Michael <[email protected]>; [email protected]; charles- [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v1 2/4] iio: adc: ad7949: fix spi messages on non 14-bit controllers

[External]

From: Liam Beguin <[email protected]>

This driver supports devices with 14-bit and 16-bit sample sizes. This is not always handled properly by spi controllers and can fail. To work around this limitation, pad samples to 16-bit and split the sample into two 8-bit messages in the event that only 8-bit messages are supported by the controller.

Signed-off-by Liam Beguin <[email protected]>
---
 drivers/iio/adc/ad7949.c | 67
++++++++++++++++++++++++++++++++++------
 1 file changed, 58 insertions(+), 9 deletions(-)
drivers/iio/adc/ad7949.c +2 -0
index 93aacf4f680b..bbc6b56330a3 100644
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
+#include <linux/bitfield.h>
 
 #define AD7949_MASK_TOTAL		GENMASK(13, 0)
 #define AD7949_CFG_REG_SIZE_BITS	14

@@ -57,6 +58,7 @@ static const struct ad7949_adc_spec
 ad7949_adc_spec[] = {
  * @indio_dev: reference to iio structure
  * @spi: reference to spi structure
  * @resolution: resolution of the chip
+ * @bits_per_word: number of bits per SPI word
  * @cfg: copy of the configuration register
  * @current_channel: current channel in use

* @buffer: buffer to send / receive data to / from device

drivers/iio/adc/ad7949.c +31 -5
@@ -67,28 +69,59 @@ struct ad7949_adc_chip {
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
 	u8 resolution;
+	u8 bits_per_word;
 	u16 cfg;
 	unsigned int current_channel;
-	u16 buffer ____cacheline_aligned;
+	union {
+		__be16 buffer;
+		u8 buf8[2];
+	} ____cacheline_aligned;
 };
 
+static void ad7949_set_bits_per_word(struct ad7949_adc_chip
 *ad7949_adc)
+{
+	u32 adc_mask = SPI_BPW_MASK(ad7949_adc->resolution);
+	u32 bpw = adc_mask & ad7949_adc->spi->controller-
 >bits_per_word_mask;
+
+	if (bpw == adc_mask)
+		ad7949_adc->bits_per_word = ad7949_adc-
 >resolution;
+	else if (bpw == SPI_BPW_MASK(16))
+		ad7949_adc->bits_per_word = 16;
+	else
+		ad7949_adc->bits_per_word = 8;
+}
+
 static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc,
 u16 val,
 				u16 mask)
 {
 	int ret;
-	int bits_per_word = ad7949_adc->resolution;
-	int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
 	struct spi_message msg;
 	struct spi_transfer tx[] = {
 		{
 			.tx_buf = &ad7949_adc->buffer,
 			.len = 2,
-			.bits_per_word = bits_per_word,
+			.bits_per_word = ad7949_adc->bits_per_word,
 		},
 	};
 
+	ad7949_adc->buffer = 0;
 	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
-	ad7949_adc->buffer = ad7949_adc->cfg << shift;
+
+	switch (ad7949_adc->bits_per_word) {
+	case 16:
+		ad7949_adc->buffer = ad7949_adc->cfg << 2;
+		break;
+	case 14:
+		ad7949_adc->buffer = ad7949_adc->cfg;
+		break;
+	case 8:
+		/* Pack 14-bit value into 2 bytes, MSB first */
+		ad7949_adc->buf8[0] = FIELD_GET(GENMASK(13, 6),

ad7949_adc->cfg);

drivers/iio/adc/ad7949.c +1 -0
+		ad7949_adc->buf8[1] = FIELD_GET(GENMASK(5, 0),

ad7949_adc->cfg);

drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 123

Quoted 3 lines
drivers/iio/adc/ad7949.c +3 -0
+		ad7949_adc->buf8[1] = ad7949_adc->buf8[1] << 2;
+		break;
+	}

Honestly I didn't went through the driver but just a question... Are we sure that 'ad7949_adc->resolution' will have something valid (8, 14, 16)? A default statement is always a nice to have :).

Quoted 60 lines
drivers/iio/adc/ad7949.c +19 -4
 	spi_message_init_with_transfers(&msg, tx, 1);
 	ret = spi_sync(ad7949_adc->spi, &msg);

@@ -105,14 +138,12 @@ static int ad7949_spi_read_channel(struct
 ad7949_adc_chip *ad7949_adc, int *val,
 {
 	int ret;
 	int i;
-	int bits_per_word = ad7949_adc->resolution;
-	int mask = GENMASK(ad7949_adc->resolution - 1, 0);
 	struct spi_message msg;
 	struct spi_transfer tx[] = {
 		{
 			.rx_buf = &ad7949_adc->buffer,
 			.len = 2,
-			.bits_per_word = bits_per_word,
+			.bits_per_word = ad7949_adc->bits_per_word,
 		},
 	};

@@ -147,7 +178,24 @@ static int ad7949_spi_read_channel(struct
 ad7949_adc_chip *ad7949_adc, int *val,
 
 	ad7949_adc->current_channel = channel;
 
-	*val = ad7949_adc->buffer & mask;
+	switch (ad7949_adc->bits_per_word) {
+	case 16:
+		*val = ad7949_adc->buffer;
+		/* Shift-out padding bits */
+		if (ad7949_adc->resolution == 14)
+			*val = *val >> 2;
+		break;
+	case 14:
+		*val = ad7949_adc->buffer & GENMASK(13, 0);
+		break;
+	case 8:
+		/* Convert byte array to u16, MSB first */
+		*val = (ad7949_adc->buf8[0] << 8) | ad7949_adc-
 >buf8[1];
+		/* Shift-out padding bits */
+		if (ad7949_adc->resolution == 14)
+			*val = *val >> 2;
+		break;
+	}
drivers/iio/adc/ad7949.c +1 -0
 	return 0;
 }

@@ -280,6 +328,7 @@ static int ad7949_spi_probe(struct spi_device
 *spi)
 	spec = &ad7949_adc_spec[spi_get_device_id(spi)-
 >driver_data];
 	indio_dev->num_channels = spec->num_channels;
 	ad7949_adc->resolution = spec->resolution;
+	ad7949_set_bits_per_word(ad7949_adc);
drivers/iio/adc/ad7949.c +0 -1
 	ad7949_adc->vref = devm_regulator_get(dev, "vref");
 	if (IS_ERR(ad7949_adc->vref)) {
--

2.30.1.489.g328c10930387

author
#4 lore raw

RE: [PATCH v1 2/4] iio: adc: ad7949: fix spi messages on non 14-bit controllers

Quoted 114 lines · Nuno
Quoted 114 lines

-----Original Message-----
From: Liam Beguin <[email protected]>
Sent: Friday, July 9, 2021 1:56 AM
To: [email protected]; [email protected]; Hennerich, Michael <[email protected]>; [email protected]; charles- [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v1 2/4] iio: adc: ad7949: fix spi messages on non 14-bit controllers

[External]

From: Liam Beguin <[email protected]>

This driver supports devices with 14-bit and 16-bit sample sizes. This is not always handled properly by spi controllers and can fail. To work around this limitation, pad samples to 16-bit and split the sample into two 8-bit messages in the event that only 8-bit messages are supported by the controller.

Signed-off-by Liam Beguin <[email protected]>
---
 drivers/iio/adc/ad7949.c | 67
++++++++++++++++++++++++++++++++++------
 1 file changed, 58 insertions(+), 9 deletions(-)
drivers/iio/adc/ad7949.c +2 -0
index 93aacf4f680b..bbc6b56330a3 100644
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
+#include <linux/bitfield.h>
 
 #define AD7949_MASK_TOTAL		GENMASK(13, 0)
 #define AD7949_CFG_REG_SIZE_BITS	14

@@ -57,6 +58,7 @@ static const struct ad7949_adc_spec
 ad7949_adc_spec[] = {
  * @indio_dev: reference to iio structure
  * @spi: reference to spi structure
  * @resolution: resolution of the chip
+ * @bits_per_word: number of bits per SPI word
  * @cfg: copy of the configuration register
  * @current_channel: current channel in use

* @buffer: buffer to send / receive data to / from device

drivers/iio/adc/ad7949.c +31 -5
@@ -67,28 +69,59 @@ struct ad7949_adc_chip {
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
 	u8 resolution;
+	u8 bits_per_word;
 	u16 cfg;
 	unsigned int current_channel;
-	u16 buffer ____cacheline_aligned;
+	union {
+		__be16 buffer;
+		u8 buf8[2];
+	} ____cacheline_aligned;
 };
 
+static void ad7949_set_bits_per_word(struct ad7949_adc_chip
 *ad7949_adc)
+{
+	u32 adc_mask = SPI_BPW_MASK(ad7949_adc->resolution);
+	u32 bpw = adc_mask & ad7949_adc->spi->controller-
 >bits_per_word_mask;
+
+	if (bpw == adc_mask)
+		ad7949_adc->bits_per_word = ad7949_adc-
 >resolution;
+	else if (bpw == SPI_BPW_MASK(16))
+		ad7949_adc->bits_per_word = 16;
+	else
+		ad7949_adc->bits_per_word = 8;
+}
+
 static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc,
 u16 val,
 				u16 mask)
 {
 	int ret;
-	int bits_per_word = ad7949_adc->resolution;
-	int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
 	struct spi_message msg;
 	struct spi_transfer tx[] = {
 		{
 			.tx_buf = &ad7949_adc->buffer,
 			.len = 2,
-			.bits_per_word = bits_per_word,
+			.bits_per_word = ad7949_adc->bits_per_word,
 		},
 	};
 
+	ad7949_adc->buffer = 0;
 	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
-	ad7949_adc->buffer = ad7949_adc->cfg << shift;
+
+	switch (ad7949_adc->bits_per_word) {
+	case 16:
+		ad7949_adc->buffer = ad7949_adc->cfg << 2;
+		break;
+	case 14:
+		ad7949_adc->buffer = ad7949_adc->cfg;
+		break;
+	case 8:
+		/* Pack 14-bit value into 2 bytes, MSB first */
+		ad7949_adc->buf8[0] = FIELD_GET(GENMASK(13, 6),

ad7949_adc->cfg);

drivers/iio/adc/ad7949.c +1 -0
+		ad7949_adc->buf8[1] = FIELD_GET(GENMASK(5, 0),

ad7949_adc->cfg);

Quoted 9 lines

drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 123

Quoted 3 lines
drivers/iio/adc/ad7949.c +3 -0
+		ad7949_adc->buf8[1] = ad7949_adc->buf8[1] << 2;
+		break;
+	}

Honestly I didn't went through the driver but just a question... Are we sure that 'ad7949_adc->resolution' will have something valid (8, 14, 16)?
A default statement is always a nice to have :).

Thanks for pointing that out. You're right that based on this driver there's no guaranty that ad7949_adc->resolution will have a valid value. I'll add a default to 8-bit.

Thanks,
Liam

Quoted 60 lines
Quoted 60 lines
drivers/iio/adc/ad7949.c +19 -4
 	spi_message_init_with_transfers(&msg, tx, 1);
 	ret = spi_sync(ad7949_adc->spi, &msg);

@@ -105,14 +138,12 @@ static int ad7949_spi_read_channel(struct
 ad7949_adc_chip *ad7949_adc, int *val,
 {
 	int ret;
 	int i;
-	int bits_per_word = ad7949_adc->resolution;
-	int mask = GENMASK(ad7949_adc->resolution - 1, 0);
 	struct spi_message msg;
 	struct spi_transfer tx[] = {
 		{
 			.rx_buf = &ad7949_adc->buffer,
 			.len = 2,
-			.bits_per_word = bits_per_word,
+			.bits_per_word = ad7949_adc->bits_per_word,
 		},
 	};

@@ -147,7 +178,24 @@ static int ad7949_spi_read_channel(struct
 ad7949_adc_chip *ad7949_adc, int *val,
 
 	ad7949_adc->current_channel = channel;
 
-	*val = ad7949_adc->buffer & mask;
+	switch (ad7949_adc->bits_per_word) {
+	case 16:
+		*val = ad7949_adc->buffer;
+		/* Shift-out padding bits */
+		if (ad7949_adc->resolution == 14)
+			*val = *val >> 2;
+		break;
+	case 14:
+		*val = ad7949_adc->buffer & GENMASK(13, 0);
+		break;
+	case 8:
+		/* Convert byte array to u16, MSB first */
+		*val = (ad7949_adc->buf8[0] << 8) | ad7949_adc-
 >buf8[1];
+		/* Shift-out padding bits */
+		if (ad7949_adc->resolution == 14)
+			*val = *val >> 2;
+		break;
+	}
drivers/iio/adc/ad7949.c +1 -0
 	return 0;
 }

@@ -280,6 +328,7 @@ static int ad7949_spi_probe(struct spi_device
 *spi)
 	spec = &ad7949_adc_spec[spi_get_device_id(spi)-
 >driver_data];
 	indio_dev->num_channels = spec->num_channels;
 	ad7949_adc->resolution = spec->resolution;
+	ad7949_set_bits_per_word(ad7949_adc);
drivers/iio/adc/ad7949.c +0 -1
 	ad7949_adc->vref = devm_regulator_get(dev, "vref");
 	if (IS_ERR(ad7949_adc->vref)) {
--

2.30.1.489.g328c10930387

author
#5 lore raw

PATCH 3/4 iio: adc: ad7949: add support for internal vref

In reply to Liam Beguin

From: Liam Beguin <[email protected]>

Add support for selecting a custom reference voltage from the devicetree. If an external source is used, a vref regulator should be defined in the devicetree.

Signed-off-by Liam Beguin <[email protected]>
---
 drivers/iio/adc/ad7949.c | 84 +++++++++++++++++++++++++++++++++-------
 1 file changed, 69 insertions(+), 15 deletions(-)
drivers/iio/adc/ad7949.c +69 -15
index bbc6b56330a3..3c1293922d2e 100644
@@ -31,6 +31,7 @@
 #define AD7949_CFG_VAL_BW_FULL			1
 #define AD7949_CFG_VAL_BW_QUARTER		0
 #define AD7949_CFG_BIT_REF		GENMASK(5, 3)
+#define AD7949_CFG_VAL_REF_EXTERNAL		BIT(1)
 #define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
 #define AD7949_CFG_BIT_RBN		BIT(0)
 

@@ -40,6 +41,33 @@ enum {
 	ID_AD7689,
 };
 
+/**
+ * enum ad7949_ref - Reference selection
+ *
+ * AD7949_REF_INT_2500:     Internal reference and temperature sensor enabled.
+ *                          Vref=2.5V, buffered output
+ * AD7949_REF_INT_4096:     Internal reference and temperature sensor enabled.
+ *                          Vref=4.096V, buffered output
+ * AD7949_REF_EXT_TEMP:     Use external reference, temperature sensor enabled.
+ *                          Internal buffer disabled
+ * AD7949_REF_EXT_TEMP_BUF: Use external reference, internal buffer and
+ *                          temperature sensor enabled.
+ * AD7949_REF_RSRV_4:       Do not use
+ * AD7949_REF_RSRV_5:       Do not use
+ * AD7949_REF_EXT:          Use external reference, internal buffer and
+ *                          temperature sensor disabled.
+ * AD7949_REF_EXT_BUF:      Use external reference, internal buffer enabled.
+ *                          Internal reference and temperature sensor disabled.
+ */
+enum ad7949_ref {
+	AD7949_REF_INT_2500 = 0,
+	AD7949_REF_INT_4096,
+	AD7949_REF_EXT_TEMP,
+	AD7949_REF_EXT_TEMP_BUF,
+	AD7949_REF_EXT = 6,
+	AD7949_REF_EXT_BUF,
+};
+
 struct ad7949_adc_spec {
 	u8 num_channels;
 	u8 resolution;

@@ -55,6 +83,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
  * struct ad7949_adc_chip - AD ADC chip
  * @lock: protects write sequences
  * @vref: regulator generating Vref
+ * @refsel: reference selection
  * @indio_dev: reference to iio structure
  * @spi: reference to spi structure
  * @resolution: resolution of the chip

@@ -66,6 +95,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
 struct ad7949_adc_chip {
 	struct mutex lock;
 	struct regulator *vref;
+	enum ad7949_ref refsel;
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
 	u8 resolution;

@@ -241,12 +271,28 @@ 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;
+		switch (ad7949_adc->refsel) {
+		case AD7949_REF_INT_2500:
+			*val = 2500;
+			break;
+		case AD7949_REF_INT_4096:
+			*val = 4096;
+			break;
+		case AD7949_REF_EXT_TEMP:
+		case AD7949_REF_EXT_TEMP_BUF:
+		case AD7949_REF_EXT:
+		case AD7949_REF_EXT_BUF:
+			ret = regulator_get_voltage(ad7949_adc->vref);
+			if (ret < 0)
+				return ret;
+
+			/* convert value back to mV */
+			*val = ret / 1000;
+			break;
+		}
 
-		*val = ret / 5000;
-		return IIO_VAL_INT;
+		*val2 = (1 << ad7949_adc->resolution) - 1;
+		return IIO_VAL_FRACTIONAL;
 	}
 
 	return -EINVAL;

@@ -285,7 +331,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 		FIELD_PREP(AD7949_CFG_BIT_INCC, AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
 		FIELD_PREP(AD7949_CFG_BIT_INX, ad7949_adc->current_channel) |
 		FIELD_PREP(AD7949_CFG_BIT_BW, AD7949_CFG_VAL_BW_FULL) |
-		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_REF_EXT_BUF) |
+		FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_adc->refsel) |
 		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
 		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);
 

@@ -304,6 +350,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 static int ad7949_spi_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
+	struct device_node *np = dev->of_node;
 	const struct ad7949_adc_spec *spec;
 	struct ad7949_adc_chip *ad7949_adc;
 	struct iio_dev *indio_dev;

@@ -315,6 +362,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
 		return -ENOMEM;
 	}
 
+	indio_dev->dev.of_node = np;
 	indio_dev->info = &ad7949_spi_info;
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;

@@ -330,16 +378,22 @@ static int ad7949_spi_probe(struct spi_device *spi)
 	ad7949_adc->resolution = spec->resolution;
 	ad7949_set_bits_per_word(ad7949_adc);
 
-	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);
-	}
+	/* Set default devicetree parameters */
+	ad7949_adc->refsel = AD7949_REF_EXT_BUF;
+	of_property_read_u32(np, "adi,reference-select", &ad7949_adc->refsel);
 
-	ret = regulator_enable(ad7949_adc->vref);
-	if (ret < 0) {
-		dev_err(dev, "fail to enable regulator\n");
-		return ret;
+	if (ad7949_adc->refsel & AD7949_CFG_VAL_REF_EXTERNAL) {
+		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);
Signature · 1 line
2.30.1.489.g328c10930387
maintainer
#6 lore raw

RE: [PATCH v1 3/4] iio: adc: ad7949: add support for internal vref

Quoted 153 lines

From: Liam Beguin <[email protected]>
Sent: Friday, July 9, 2021 1:56 AM
To: [email protected]; [email protected]; Hennerich, Michael <[email protected]>; [email protected]; charles- [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v1 3/4] iio: adc: ad7949: add support for internal vref

[External]

From: Liam Beguin <[email protected]>

Add support for selecting a custom reference voltage from the devicetree. If an external source is used, a vref regulator should be defined in the devicetree.

Signed-off-by Liam Beguin <[email protected]>
---
 drivers/iio/adc/ad7949.c | 84
+++++++++++++++++++++++++++++++++-------
 1 file changed, 69 insertions(+), 15 deletions(-)
drivers/iio/adc/ad7949.c +5 -0
index bbc6b56330a3..3c1293922d2e 100644
@@ -31,6 +31,7 @@
 #define AD7949_CFG_VAL_BW_FULL			1
 #define AD7949_CFG_VAL_BW_QUARTER		0
 #define AD7949_CFG_BIT_REF		GENMASK(5, 3)
+#define AD7949_CFG_VAL_REF_EXTERNAL		BIT(1)
 #define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
 #define AD7949_CFG_BIT_RBN		BIT(0)
 

@@ -40,6 +41,33 @@ enum {
 	ID_AD7689,
 };
 
+/**
+ * enum ad7949_ref - Reference selection
+ *
+ * AD7949_REF_INT_2500:     Internal reference and temperature

sensor enabled.
+ * Vref=2.5V, buffered output
+ * AD7949_REF_INT_4096: Internal reference and temperature
sensor enabled.
+ * Vref=4.096V, buffered output
+ * AD7949_REF_EXT_TEMP: Use external reference, temperature
sensor enabled.
+ * Internal buffer disabled
+ * AD7949_REF_EXT_TEMP_BUF: Use external reference, internal buffer and

+ *                          temperature sensor enabled.
+ * AD7949_REF_RSRV_4:       Do not use
+ * AD7949_REF_RSRV_5:       Do not use
+ * AD7949_REF_EXT:          Use external reference, internal buffer and
+ *                          temperature sensor disabled.
+ * AD7949_REF_EXT_BUF:      Use external reference, internal buffer

enabled.
+ * Internal reference and temperature sensor disabled.
+ */

drivers/iio/adc/ad7949.c +10 -0
+enum ad7949_ref {
+	AD7949_REF_INT_2500 = 0,
+	AD7949_REF_INT_4096,
+	AD7949_REF_EXT_TEMP,
+	AD7949_REF_EXT_TEMP_BUF,
+	AD7949_REF_EXT = 6,
+	AD7949_REF_EXT_BUF,
+};
+
 struct ad7949_adc_spec {
 	u8 num_channels;
 	u8 resolution;

@@ -55,6 +83,7 @@ static const struct ad7949_adc_spec
 ad7949_adc_spec[] = {
  * struct ad7949_adc_chip - AD ADC chip
  * @lock: protects write sequences
  * @vref: regulator generating Vref
+ * @refsel: reference selection
  * @indio_dev: reference to iio structure
  * @spi: reference to spi structure

* @resolution: resolution of the chip

drivers/iio/adc/ad7949.c +1 -0
@@ -66,6 +95,7 @@ static const struct ad7949_adc_spec
 ad7949_adc_spec[] = {
 struct ad7949_adc_chip {
 	struct mutex lock;
 	struct regulator *vref;
+	enum ad7949_ref refsel;
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
drivers/iio/adc/ad7949.c +21 -5
 	u8 resolution;

@@ -241,12 +271,28 @@ 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;
+		switch (ad7949_adc->refsel) {
+		case AD7949_REF_INT_2500:
+			*val = 2500;
+			break;
+		case AD7949_REF_INT_4096:
+			*val = 4096;
+			break;
+		case AD7949_REF_EXT_TEMP:
+		case AD7949_REF_EXT_TEMP_BUF:
+		case AD7949_REF_EXT:
+		case AD7949_REF_EXT_BUF:
+			ret = regulator_get_voltage(ad7949_adc-
 >vref);
+			if (ret < 0)
+				return ret;
+
+			/* convert value back to mV */
+			*val = ret / 1000;
+			break;
+		}
 
-		*val = ret / 5000;
-		return IIO_VAL_INT;
+		*val2 = (1 << ad7949_adc->resolution) - 1;
+		return IIO_VAL_FRACTIONAL;
 	}
drivers/iio/adc/ad7949.c +0 -0
 	return -EINVAL;

@@ -285,7 +331,7 @@ static int ad7949_spi_init(struct
 ad7949_adc_chip *ad7949_adc)
 		FIELD_PREP(AD7949_CFG_BIT_INCC,

AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
FIELD_PREP(AD7949_CFG_BIT_INX, ad7949_adc-

Quoted 1 line

current_channel) |

FIELD_PREP(AD7949_CFG_BIT_BW,
AD7949_CFG_VAL_BW_FULL) |

drivers/iio/adc/ad7949.c +0 -1
-		FIELD_PREP(AD7949_CFG_BIT_REF,

AD7949_REF_EXT_BUF) |

drivers/iio/adc/ad7949.c +1 -0
+		FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_adc-
Quoted 1 line

refsel) |

drivers/iio/adc/ad7949.c +1 -0
 		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
 		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);

@@ -304,6 +350,7 @@ static int ad7949_spi_init(struct
 ad7949_adc_chip *ad7949_adc)
 static int ad7949_spi_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
+	struct device_node *np = dev->of_node;
 	const struct ad7949_adc_spec *spec;
 	struct ad7949_adc_chip *ad7949_adc;
drivers/iio/adc/ad7949.c +0 -0
 	struct iio_dev *indio_dev;

drivers/iio/adc/ad7949.c static int ad7949_spi_probe(struct spi_device · line 366

Quoted 5 lines
drivers/iio/adc/ad7949.c +1 -0
@@ -315,6 +362,7 @@ static int ad7949_spi_probe(struct spi_device
 *spi)
 		return -ENOMEM;
 	}
 
+	indio_dev->dev.of_node = np;

Why doing this? The IIO core will do it for us at register time.

Quoted 7 lines
drivers/iio/adc/ad7949.c +0 -0
 	indio_dev->info = &ad7949_spi_info;
 	indio_dev->name = spi_get_device_id(spi)->name;
drivers/iio/adc/ad7949.c +0 -0
 	indio_dev->modes = INDIO_DIRECT_MODE;

@@ -330,16 +378,22 @@ static int ad7949_spi_probe(struct spi_device
 *spi)
 	ad7949_adc->resolution = spec->resolution;
 	ad7949_set_bits_per_word(ad7949_adc);

drivers/iio/adc/ad7949.c static int ad7949_spi_probe(struct spi_device · line 385

Quoted 10 lines
drivers/iio/adc/ad7949.c +3 -5
 
-	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);
-	}
+	/* Set default devicetree parameters */
+	ad7949_adc->refsel = AD7949_REF_EXT_BUF;
+	of_property_read_u32(np, "adi,reference-select",
 &ad7949_adc->refsel);

In case the property is given, we should make sure we get a valid value and error out if not...

- Nuno Sá

Quoted 21 lines
drivers/iio/adc/ad7949.c +12 -4
-	ret = regulator_enable(ad7949_adc->vref);
-	if (ret < 0) {
-		dev_err(dev, "fail to enable regulator\n");
-		return ret;
+	if (ad7949_adc->refsel & AD7949_CFG_VAL_REF_EXTERNAL) {
+		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);
--
2.30.1.489.g328c10930387

author
#7 lore raw

RE: [PATCH v1 3/4] iio: adc: ad7949: add support for internal vref

Quoted 157 lines · Nuno
Quoted 157 lines

From: Liam Beguin <[email protected]>
Sent: Friday, July 9, 2021 1:56 AM
To: [email protected]; [email protected]; Hennerich, Michael <[email protected]>; [email protected]; charles- [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v1 3/4] iio: adc: ad7949: add support for internal vref

[External]

From: Liam Beguin <[email protected]>

Add support for selecting a custom reference voltage from the devicetree. If an external source is used, a vref regulator should be defined in the devicetree.

Signed-off-by Liam Beguin <[email protected]>
---
 drivers/iio/adc/ad7949.c | 84
+++++++++++++++++++++++++++++++++-------
 1 file changed, 69 insertions(+), 15 deletions(-)
drivers/iio/adc/ad7949.c +5 -0
index bbc6b56330a3..3c1293922d2e 100644
@@ -31,6 +31,7 @@
 #define AD7949_CFG_VAL_BW_FULL			1
 #define AD7949_CFG_VAL_BW_QUARTER		0
 #define AD7949_CFG_BIT_REF		GENMASK(5, 3)
+#define AD7949_CFG_VAL_REF_EXTERNAL		BIT(1)
 #define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
 #define AD7949_CFG_BIT_RBN		BIT(0)
 

@@ -40,6 +41,33 @@ enum {
 	ID_AD7689,
 };
 
+/**
+ * enum ad7949_ref - Reference selection
+ *
+ * AD7949_REF_INT_2500:     Internal reference and temperature

sensor enabled.
+ * Vref=2.5V, buffered output
+ * AD7949_REF_INT_4096: Internal reference and temperature
sensor enabled.
+ * Vref=4.096V, buffered output
+ * AD7949_REF_EXT_TEMP: Use external reference, temperature
sensor enabled.
+ * Internal buffer disabled
+ * AD7949_REF_EXT_TEMP_BUF: Use external reference, internal buffer and

+ *                          temperature sensor enabled.
+ * AD7949_REF_RSRV_4:       Do not use
+ * AD7949_REF_RSRV_5:       Do not use
+ * AD7949_REF_EXT:          Use external reference, internal buffer and
+ *                          temperature sensor disabled.
+ * AD7949_REF_EXT_BUF:      Use external reference, internal buffer

enabled.
+ * Internal reference and temperature sensor disabled.
+ */

drivers/iio/adc/ad7949.c +10 -0
+enum ad7949_ref {
+	AD7949_REF_INT_2500 = 0,
+	AD7949_REF_INT_4096,
+	AD7949_REF_EXT_TEMP,
+	AD7949_REF_EXT_TEMP_BUF,
+	AD7949_REF_EXT = 6,
+	AD7949_REF_EXT_BUF,
+};
+
 struct ad7949_adc_spec {
 	u8 num_channels;
 	u8 resolution;

@@ -55,6 +83,7 @@ static const struct ad7949_adc_spec
 ad7949_adc_spec[] = {
  * struct ad7949_adc_chip - AD ADC chip
  * @lock: protects write sequences
  * @vref: regulator generating Vref
+ * @refsel: reference selection
  * @indio_dev: reference to iio structure
  * @spi: reference to spi structure

* @resolution: resolution of the chip

drivers/iio/adc/ad7949.c +1 -0
@@ -66,6 +95,7 @@ static const struct ad7949_adc_spec
 ad7949_adc_spec[] = {
 struct ad7949_adc_chip {
 	struct mutex lock;
 	struct regulator *vref;
+	enum ad7949_ref refsel;
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
drivers/iio/adc/ad7949.c +21 -5
 	u8 resolution;

@@ -241,12 +271,28 @@ 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;
+		switch (ad7949_adc->refsel) {
+		case AD7949_REF_INT_2500:
+			*val = 2500;
+			break;
+		case AD7949_REF_INT_4096:
+			*val = 4096;
+			break;
+		case AD7949_REF_EXT_TEMP:
+		case AD7949_REF_EXT_TEMP_BUF:
+		case AD7949_REF_EXT:
+		case AD7949_REF_EXT_BUF:
+			ret = regulator_get_voltage(ad7949_adc-
 >vref);
+			if (ret < 0)
+				return ret;
+
+			/* convert value back to mV */
+			*val = ret / 1000;
+			break;
+		}
 
-		*val = ret / 5000;
-		return IIO_VAL_INT;
+		*val2 = (1 << ad7949_adc->resolution) - 1;
+		return IIO_VAL_FRACTIONAL;
 	}
drivers/iio/adc/ad7949.c +0 -0
 	return -EINVAL;

@@ -285,7 +331,7 @@ static int ad7949_spi_init(struct
 ad7949_adc_chip *ad7949_adc)
 		FIELD_PREP(AD7949_CFG_BIT_INCC,

AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
FIELD_PREP(AD7949_CFG_BIT_INX, ad7949_adc-

Quoted 1 line

current_channel) |

FIELD_PREP(AD7949_CFG_BIT_BW,
AD7949_CFG_VAL_BW_FULL) |

drivers/iio/adc/ad7949.c +0 -1
-		FIELD_PREP(AD7949_CFG_BIT_REF,

AD7949_REF_EXT_BUF) |

drivers/iio/adc/ad7949.c +1 -0
+		FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_adc-
Quoted 1 line

refsel) |

drivers/iio/adc/ad7949.c +1 -0
 		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
 		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);

@@ -304,6 +350,7 @@ static int ad7949_spi_init(struct
 ad7949_adc_chip *ad7949_adc)
 static int ad7949_spi_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
+	struct device_node *np = dev->of_node;
 	const struct ad7949_adc_spec *spec;
 	struct ad7949_adc_chip *ad7949_adc;
drivers/iio/adc/ad7949.c +0 -0
 	struct iio_dev *indio_dev;

@@ -315,6 +362,7 @@ static int ad7949_spi_probe(struct spi_device
 *spi)
 		return -ENOMEM;
 	}
Quoted 3 lines

drivers/iio/adc/ad7949.c static int ad7949_spi_probe(struct spi_device *spi) · line 365

Quoted 1 line
drivers/iio/adc/ad7949.c +1 -0
+	indio_dev->dev.of_node = np;

Why doing this? The IIO core will do it for us at register time.

Sorry about that, will drop.

Quoted 9 lines
Quoted 9 lines
drivers/iio/adc/ad7949.c +0 -0
 	indio_dev->info = &ad7949_spi_info;
 	indio_dev->name = spi_get_device_id(spi)->name;
drivers/iio/adc/ad7949.c +0 -0
 	indio_dev->modes = INDIO_DIRECT_MODE;

@@ -330,16 +378,22 @@ static int ad7949_spi_probe(struct spi_device
 *spi)
 	ad7949_adc->resolution = spec->resolution;
 	ad7949_set_bits_per_word(ad7949_adc);
Quoted 13 lines

drivers/iio/adc/ad7949.c static int ad7949_spi_probe(struct spi_device *spi)

Quoted 9 lines
drivers/iio/adc/ad7949.c +3 -5
-	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);
-	}
+	/* Set default devicetree parameters */
+	ad7949_adc->refsel = AD7949_REF_EXT_BUF;
+	of_property_read_u32(np, "adi,reference-select",
 &ad7949_adc->refsel);

In case the property is given, we should make sure we get a valid value and error out if not...

I thought it would be handled later on when processing the sample, but it's not really and it's better to fail early. I'll add that to v2.

Thanks,
Liam

Quoted 22 lines

- Nuno Sá

Quoted 21 lines
drivers/iio/adc/ad7949.c +12 -4
-	ret = regulator_enable(ad7949_adc->vref);
-	if (ret < 0) {
-		dev_err(dev, "fail to enable regulator\n");
-		return ret;
+	if (ad7949_adc->refsel & AD7949_CFG_VAL_REF_EXTERNAL) {
+		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);
--
2.30.1.489.g328c10930387

author
#8 lore raw

PATCH 4/4 dt-bindings: iio: adc: ad7949: add adi,reference-source

In reply to Liam Beguin

From: Liam Beguin <[email protected]>

Add bindings documentation for the adi,reference-source property. This property is required to properly configure the ADC sample request based on which reference source should be used for the calculation.

Signed-off-by Liam Beguin <[email protected]>
---
 .../bindings/iio/adc/adi,ad7949.yaml          | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +22 -0
index 9b56bd4d5510..3f4629281cc8 100644
@@ -35,6 +35,28 @@ properties:
   "#io-channel-cells":
     const: 1
 
+  adi,reference-select:
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - enum: [0, 1, 2, 3, 6, 7]
+
+    default: 7
+    description: |
+      Select the reference voltage source to use when converting samples.
+      Acceptable values are:
+      - 0: Internal reference and temperature sensor enabled.
+           Vref=2.5V, buffered output
+      - 1: Internal reference and temperature sensor enabled.
+           Vref=4.096V, buffered output
+      - 2: Use external reference, temperature sensor enabled.
+           Internal buffer disabled
+      - 3: Use external reference, internal buffer and temperature sensor
+           enabled.
+      - 6: Use external reference, internal buffer and temperature sensor
+           disabled.
+      - 7: Use external reference, internal buffer enabled.
+           Internal reference and temperature sensor disabled.
+
 required:
   - compatible
   - reg
Signature · 1 line
2.30.1.489.g328c10930387
maintainer
#9 lore raw

RE: [PATCH v1 4/4] dt-bindings: iio: adc: ad7949: add adi,reference-source

Quoted 57 lines

-----Original Message-----
From: Liam Beguin <[email protected]>
Sent: Friday, July 9, 2021 1:56 AM
To: [email protected]; [email protected]; Hennerich, Michael <[email protected]>; [email protected]; charles- [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v1 4/4] dt-bindings: iio: adc: ad7949: add adi,reference-source

[External]

From: Liam Beguin <[email protected]>

Add bindings documentation for the adi,reference-source property. This property is required to properly configure the ADC sample request based on which reference source should be used for the calculation.

Signed-off-by Liam Beguin <[email protected]>
---
 .../bindings/iio/adc/adi,ad7949.yaml          | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml index 9b56bd4d5510..3f4629281cc8 100644

Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +8 -0
@@ -35,6 +35,28 @@ properties:
   "#io-channel-cells":
     const: 1
 
+  adi,reference-select:
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - enum: [0, 1, 2, 3, 6, 7]
+
+    default: 7
+    description: |
+      Select the reference voltage source to use when converting

samples.

Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +8 -0
+      Acceptable values are:
+      - 0: Internal reference and temperature sensor enabled.
+           Vref=2.5V, buffered output
+      - 1: Internal reference and temperature sensor enabled.
+           Vref=4.096V, buffered output
+      - 2: Use external reference, temperature sensor enabled.
+           Internal buffer disabled
+      - 3: Use external reference, internal buffer and temperature

sensor

Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +2 -0
+           enabled.
+      - 6: Use external reference, internal buffer and temperature

sensor

Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml properties: · line 58

Quoted 3 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +3 -0
+           disabled.
+      - 7: Use external reference, internal buffer enabled.
+           Internal reference and temperature sensor disabled.

I think typically the description comes first. I also don't think you need the 'allOf'(not even sure if it will pass the binding check)... Just have '$ref' and 'enum' on the same level.

- Nuno Sá

Quoted 5 lines

required:
- compatible
- reg
--
2.30.1.489.g328c10930387

author
#10 lore raw

RE: [PATCH v1 4/4] dt-bindings: iio: adc: ad7949: add adi,reference-source

Hi Nuno,

Quoted 56 lines · Nuno
Quoted 56 lines

-----Original Message-----
From: Liam Beguin <[email protected]>
Sent: Friday, July 9, 2021 1:56 AM
To: [email protected]; [email protected]; Hennerich, Michael <[email protected]>; [email protected]; charles- [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v1 4/4] dt-bindings: iio: adc: ad7949: add adi,reference-source

[External]

From: Liam Beguin <[email protected]>

Add bindings documentation for the adi,reference-source property. This property is required to properly configure the ADC sample request based on which reference source should be used for the calculation.

Signed-off-by Liam Beguin <[email protected]>
---
 .../bindings/iio/adc/adi,ad7949.yaml          | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml index 9b56bd4d5510..3f4629281cc8 100644

Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +8 -0
@@ -35,6 +35,28 @@ properties:
   "#io-channel-cells":
     const: 1
 
+  adi,reference-select:
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - enum: [0, 1, 2, 3, 6, 7]
+
+    default: 7
+    description: |
+      Select the reference voltage source to use when converting

samples.

Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +8 -0
+      Acceptable values are:
+      - 0: Internal reference and temperature sensor enabled.
+           Vref=2.5V, buffered output
+      - 1: Internal reference and temperature sensor enabled.
+           Vref=4.096V, buffered output
+      - 2: Use external reference, temperature sensor enabled.
+           Internal buffer disabled
+      - 3: Use external reference, internal buffer and temperature

sensor

Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +2 -0
+           enabled.
+      - 6: Use external reference, internal buffer and temperature
Quoted 9 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml properties: · line 58

Quoted 4 lines

sensor

Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +3 -0
+           disabled.
+      - 7: Use external reference, internal buffer enabled.
+           Internal reference and temperature sensor disabled.

I think typically the description comes first. I also don't think you need the 'allOf'(not even sure if it will pass the binding check)... Just have '$ref' and 'enum' on the same level.

Understood, I can reorder the patches so that the bindings come first.

I thought I based that part on the example-schema.yaml, but looking at it again, it seems like you're right and the AllOf isn't required.

I did run the bindings check on this, but I'll fix it.

Thanks,
Liam

Quoted 7 lines

- Nuno Sá

Quoted 5 lines

required:
- compatible
- reg
--
2.30.1.489.g328c10930387

author
#11 lore raw

RE: [PATCH v1 4/4] dt-bindings: iio: adc: ad7949: add adi,reference-source

In reply to Sa, Nuno
Quoted 56 lines · Nuno
Quoted 56 lines

-----Original Message-----
From: Liam Beguin <[email protected]>
Sent: Friday, July 9, 2021 1:56 AM
To: [email protected]; [email protected]; Hennerich, Michael <[email protected]>; [email protected]; charles- [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v1 4/4] dt-bindings: iio: adc: ad7949: add adi,reference-source

[External]

From: Liam Beguin <[email protected]>

Add bindings documentation for the adi,reference-source property. This property is required to properly configure the ADC sample request based on which reference source should be used for the calculation.

Signed-off-by Liam Beguin <[email protected]>
---
 .../bindings/iio/adc/adi,ad7949.yaml          | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml index 9b56bd4d5510..3f4629281cc8 100644

Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +8 -0
@@ -35,6 +35,28 @@ properties:
   "#io-channel-cells":
     const: 1
 
+  adi,reference-select:
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - enum: [0, 1, 2, 3, 6, 7]
+
+    default: 7
+    description: |
+      Select the reference voltage source to use when converting

samples.

Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +8 -0
+      Acceptable values are:
+      - 0: Internal reference and temperature sensor enabled.
+           Vref=2.5V, buffered output
+      - 1: Internal reference and temperature sensor enabled.
+           Vref=4.096V, buffered output
+      - 2: Use external reference, temperature sensor enabled.
+           Internal buffer disabled
+      - 3: Use external reference, internal buffer and temperature

sensor

Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +2 -0
+           enabled.
+      - 6: Use external reference, internal buffer and temperature
Quoted 9 lines

Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml properties: · line 58

Quoted 4 lines

sensor

Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +3 -0
+           disabled.
+      - 7: Use external reference, internal buffer enabled.
+           Internal reference and temperature sensor disabled.

I think typically the description comes first. I also don't think you need the 'allOf'(not even sure if it will pass the binding check)... Just have '$ref' and 'enum' on the same level.

I realize I misread your comment on the order of the description. I'll move it at the top of the binding definition.

Liam

Quoted 7 lines

- Nuno Sá

Quoted 5 lines

required:
- compatible
- reg
--
2.30.1.489.g328c10930387

maintainer
#12 lore raw
In reply to Liam Beguin

Hi Liam,

The series looks good. Just some minor comments from my side...

Thanks!
- Nuno Sá

Quoted 47 lines

From: Liam Beguin <[email protected]>
Sent: Friday, July 9, 2021 1:56 AM
To: [email protected]; [email protected]; Hennerich, Michael <[email protected]>; [email protected]; charles- [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v1 0/4] AD7949 Fixes

[External]

While working on another series[1] I ran into issues where my SPI controller would fail to handle 14-bit and 16-bit SPI messages. This addresses that issue and adds support for selecting a different voltage reference source from the devicetree.

This is base on a series[2] that seems to not have made it all the way, and was tested on an ad7689.

[1]
https://urldefense.com/v3/__https://patchwork.kernel.org/project/li
nux-iio/list/?series=511545__;!!A3Ni8CS0y2Y!rF7O-WxC7HZM4hv_o- zYPtVCmZlDoASzPvamwwQkH9llVS8J-GpfQltvDBz7gQ$
[2]
https://urldefense.com/v3/__https://patchwork.kernel.org/project/li
nux- iio/list/?series=116971&state=*2A&archive=both__;JQ!!A3Ni8CS0y2Y! rF7O-WxC7HZM4hv_o-zYPtVCmZlDoASzPvamwwQkH9llVS8J-GpfQluZ- w3LYA$

Thanks for your time,
Liam

Liam Beguin (4):
iio: adc: ad7949: define and use bitfield names
iio: adc: ad7949: fix spi messages on non 14-bit controllers
iio: adc: ad7949: add support for internal vref
dt-bindings: iio: adc: ad7949: add adi,reference-source

 .../bindings/iio/adc/adi,ad7949.yaml          |  22 ++
 drivers/iio/adc/ad7949.c                      | 191 +++++++++++++++---
 2 files changed, 181 insertions(+), 32 deletions(-)

base-commit: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78 --
2.30.1.489.g328c10930387