[PATCH v3 0/4] AD7949 Fixes

[PATCH v3 0/4] AD7949 Fixes

[Liam Beguin]

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.

V1 was 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

Changes since v2:
- add comments to ambiguous register names
- fix be16 definition of the adc buffer
- fix BPW logic
- rework vref support
  - support per channel vref selection
  - infer reference select value based on DT parameters
  - update dt-binding

Changes since v1:
- add default case in read/write size cases
- drop of_node change as the core already takes care of it
- check dt user input in probe
- move description at the top of dt-binding definition
- drop AllOf block in dt-binding

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 per channel reference

 .../bindings/iio/adc/adi,ad7949.yaml          |  71 ++++-
 drivers/iio/adc/ad7949.c                      | 243 +++++++++++++++---
 2 files changed, 280 insertions(+), 34 deletions(-)

Range-diff against v2:
1:  b8577e93229f ! 1:  8760b368f971 iio: adc: ad7949: define and use bitfield names
    @@ Commit message
     
      ## drivers/iio/adc/ad7949.c ##
     @@
    + #include <linux/module.h>
      #include <linux/regulator/consumer.h>
      #include <linux/spi/spi.h>
    ++#include <linux/bitfield.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_REG_SIZE_BITS	14
    ++
    ++/* CFG: Configuration Update */
    ++#define AD7949_CFG_BIT_OVERWRITE	BIT(13)
    ++
    ++/* INCC: Input Channel Configuration */
     +#define AD7949_CFG_BIT_INCC		GENMASK(12, 10)
     +#define AD7949_CFG_VAL_INCC_UNIPOLAR_GND	7
     +#define AD7949_CFG_VAL_INCC_UNIPOLAR_COMM	6
    @@ drivers/iio/adc/ad7949.c
     +#define AD7949_CFG_VAL_INCC_TEMP		3
     +#define AD7949_CFG_VAL_INCC_BIPOLAR		2
     +#define AD7949_CFG_VAL_INCC_BIPOLAR_DIFF	0
    ++
    ++/* INX: Input channel Selection in a binary fashion */
     +#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
    ++
    ++/* BW: select bandwidth for low-pass filter. Full or Quarter */
    ++#define AD7949_CFG_BIT_BW_FULL			BIT(6)
    ++
    ++/* REF: reference/buffer selection */
     +#define AD7949_CFG_BIT_REF		GENMASK(5, 3)
    ++#define AD7949_CFG_VAL_REF_EXT_BUF		7
    ++
    ++/* SEQ: channel sequencer. Allows for scanning channels */
     +#define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
    -+#define AD7949_CFG_BIT_RBN		BIT(0)
     +
    ++/* RB: Read back the CFG register */
    ++#define AD7949_CFG_BIT_RBN		BIT(0)
    + 
      enum {
      	ID_AD7949 = 0,
    - 	ID_AD7682,
     @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
      	 */
      	for (i = 0; i < 2; i++) {
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_init(struct ad7949_adc_chip *ad7
      	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) |
    ++	cfg = FIELD_PREP(AD7949_CFG_BIT_OVERWRITE, 1) |
     +		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_BW_FULL, 1) |
    ++		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_CFG_VAL_REF_EXT_BUF) |
     +		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
     +		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);
     +
2:  a8468391e3d0 ! 2:  a9243c834705 iio: adc: ad7949: fix spi messages on non 14-bit controllers
    @@ Commit message
     
      ## drivers/iio/adc/ad7949.c ##
     @@
    - #include <linux/module.h>
      #include <linux/regulator/consumer.h>
      #include <linux/spi/spi.h>
    -+#include <linux/bitfield.h>
    + #include <linux/bitfield.h>
    ++#include <asm/unaligned.h>
      
      #define AD7949_MASK_TOTAL		GENMASK(13, 0)
    - #define AD7949_CFG_REG_SIZE_BITS	14
    + 
     @@ drivers/iio/adc/ad7949.c: static const struct ad7949_adc_spec ad7949_adc_spec[] = {
       * @indio_dev: reference to iio structure
       * @spi: reference to spi structure
    @@ drivers/iio/adc/ad7949.c: struct ad7949_adc_chip {
     +	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 buffer ____cacheline_aligned;
    +@@ drivers/iio/adc/ad7949.c: 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;
    ++	u8 buf8[2];
      	struct spi_message msg;
      	struct spi_transfer tx[] = {
      		{
    @@ drivers/iio/adc/ad7949.c: struct ad7949_adc_chip {
     +		ad7949_adc->buffer = ad7949_adc->cfg;
     +		break;
     +	case 8:
    -+	default:
     +		/* 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;
    ++		buf8[0] = FIELD_GET(GENMASK(13, 6), ad7949_adc->cfg);
    ++		buf8[1] = FIELD_GET(GENMASK(5, 0), ad7949_adc->cfg) << 2;
    ++		memcpy(&ad7949_adc->buffer, buf8, 2);
     +		break;
    ++	default:
    ++		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
    ++		return -EINVAL;
     +	}
     +
      	spi_message_init_with_transfers(&msg, tx, 1);
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_c
      	int i;
     -	int bits_per_word = ad7949_adc->resolution;
     -	int mask = GENMASK(ad7949_adc->resolution - 1, 0);
    ++	u8 buf8[2];
      	struct spi_message msg;
      	struct spi_transfer tx[] = {
      		{
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_c
     +	case 16:
     +		*val = ad7949_adc->buffer;
     +		/* Shift-out padding bits */
    -+		if (ad7949_adc->resolution == 14)
    -+			*val = *val >> 2;
    ++		*val >>= 16 - ad7949_adc->resolution;
     +		break;
     +	case 14:
     +		*val = ad7949_adc->buffer & GENMASK(13, 0);
     +		break;
     +	case 8:
    -+	default:
    ++		memcpy(buf8, &ad7949_adc->buffer, 2);
     +		/* Convert byte array to u16, MSB first */
    -+		*val = (ad7949_adc->buf8[0] << 8) | ad7949_adc->buf8[1];
    ++		*val = get_unaligned_be16(buf8);
     +		/* Shift-out padding bits */
    -+		if (ad7949_adc->resolution == 14)
    -+			*val = *val >> 2;
    ++		*val >>= 16 - ad7949_adc->resolution;
     +		break;
    ++	default:
    ++		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
    ++		return -EINVAL;
     +	}
      
      	return 0;
      }
    +@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
    + 
    + static int ad7949_spi_probe(struct spi_device *spi)
    + {
    ++	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
    + 	struct device *dev = &spi->dev;
    + 	const struct ad7949_adc_spec *spec;
    + 	struct ad7949_adc_chip *ad7949_adc;
     @@ drivers/iio/adc/ad7949.c: 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);
      
    ++	/* Set SPI bits per word */
    ++	if (spi_ctrl_mask & SPI_BPW_MASK(ad7949_adc->resolution)) {
    ++		ad7949_adc->bits_per_word = ad7949_adc->resolution;
    ++	} else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
    ++		ad7949_adc->bits_per_word = 16;
    ++	} else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
    ++		ad7949_adc->bits_per_word = 8;
    ++	} else {
    ++		dev_err(dev, "unable to find common BPW with spi controller\n");
    ++		return -EINVAL;
    ++	}
    ++
      	ad7949_adc->vref = devm_regulator_get(dev, "vref");
      	if (IS_ERR(ad7949_adc->vref)) {
    + 		dev_err(dev, "fail to request regulator\n");
3:  4b0c11c9a748 < -:  ------------ iio: adc: ad7949: add support for internal vref
4:  a3b6a6ef15fd < -:  ------------ dt-bindings: iio: adc: ad7949: add adi,reference-source
-:  ------------ > 3:  bb25b7fcb0a3 iio: adc: ad7949: add support for internal vref
-:  ------------ > 4:  41e3ca4f0d52 dt-bindings: iio: adc: ad7949: add per channel reference

base-commit: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
-- 
2.30.1.489.g328c10930387

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

[Liam Beguin]

From: Liam Beguin <lvb@xiphos.com>

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

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 drivers/iio/adc/ad7949.c | 50 +++++++++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 1b4b3203e428..0b549b8bd7a9 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -11,12 +11,37 @@
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
+#include <linux/bitfield.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
+
+/* CFG: Configuration Update */
+#define AD7949_CFG_BIT_OVERWRITE	BIT(13)
+
+/* INCC: Input Channel Configuration */
+#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
+
+/* INX: Input channel Selection in a binary fashion */
+#define AD7949_CFG_BIT_INX		GENMASK(9, 7)
+
+/* BW: select bandwidth for low-pass filter. Full or Quarter */
+#define AD7949_CFG_BIT_BW_FULL			BIT(6)
+
+/* REF: reference/buffer selection */
+#define AD7949_CFG_BIT_REF		GENMASK(5, 3)
+#define AD7949_CFG_VAL_REF_EXT_BUF		7
+
+/* SEQ: channel sequencer. Allows for scanning channels */
+#define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
+
+/* RB: Read back the CFG register */
+#define AD7949_CFG_BIT_RBN		BIT(0)
 
 enum {
 	ID_AD7949 = 0,
@@ -109,8 +134,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 +239,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_OVERWRITE, 1) |
+		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_FULL, 1) |
+		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_CFG_VAL_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.
-- 
2.30.1.489.g328c10930387

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

[Liam Beguin]

From: Liam Beguin <lvb@xiphos.com>

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 <lvb@xiphos.com>
---
 drivers/iio/adc/ad7949.c | 68 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 60 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 0b549b8bd7a9..65f78751225b 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -12,6 +12,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
 #include <linux/bitfield.h>
+#include <asm/unaligned.h>
 
 #define AD7949_MASK_TOTAL		GENMASK(13, 0)
 
@@ -67,6 +68,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
@@ -77,6 +79,7 @@ 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;
@@ -86,19 +89,37 @@ 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;
+	u8 buf8[2];
 	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 */
+		buf8[0] = FIELD_GET(GENMASK(13, 6), ad7949_adc->cfg);
+		buf8[1] = FIELD_GET(GENMASK(5, 0), ad7949_adc->cfg) << 2;
+		memcpy(&ad7949_adc->buffer, buf8, 2);
+		break;
+	default:
+		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
+		return -EINVAL;
+	}
+
 	spi_message_init_with_transfers(&msg, tx, 1);
 	ret = spi_sync(ad7949_adc->spi, &msg);
 
@@ -115,14 +136,13 @@ 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);
+	u8 buf8[2];
 	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,
 		},
 	};
 
@@ -157,7 +177,26 @@ 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 */
+		*val >>= 16 - ad7949_adc->resolution;
+		break;
+	case 14:
+		*val = ad7949_adc->buffer & GENMASK(13, 0);
+		break;
+	case 8:
+		memcpy(buf8, &ad7949_adc->buffer, 2);
+		/* Convert byte array to u16, MSB first */
+		*val = get_unaligned_be16(buf8);
+		/* Shift-out padding bits */
+		*val >>= 16 - ad7949_adc->resolution;
+		break;
+	default:
+		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
+		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -265,6 +304,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 
 static int ad7949_spi_probe(struct spi_device *spi)
 {
+	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
 	struct device *dev = &spi->dev;
 	const struct ad7949_adc_spec *spec;
 	struct ad7949_adc_chip *ad7949_adc;
@@ -291,6 +331,18 @@ static int ad7949_spi_probe(struct spi_device *spi)
 	indio_dev->num_channels = spec->num_channels;
 	ad7949_adc->resolution = spec->resolution;
 
+	/* Set SPI bits per word */
+	if (spi_ctrl_mask & SPI_BPW_MASK(ad7949_adc->resolution)) {
+		ad7949_adc->bits_per_word = ad7949_adc->resolution;
+	} else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
+		ad7949_adc->bits_per_word = 16;
+	} else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
+		ad7949_adc->bits_per_word = 8;
+	} else {
+		dev_err(dev, "unable to find common BPW with spi controller\n");
+		return -EINVAL;
+	}
+
 	ad7949_adc->vref = devm_regulator_get(dev, "vref");
 	if (IS_ERR(ad7949_adc->vref)) {
 		dev_err(dev, "fail to request regulator\n");
-- 
2.30.1.489.g328c10930387

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

[Liam Beguin]

From: Liam Beguin <lvb@xiphos.com>

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 <lvb@xiphos.com>
---
 drivers/iio/adc/ad7949.c | 133 +++++++++++++++++++++++++++++++++------
 1 file changed, 115 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 65f78751225b..8b78da7ead36 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -36,7 +36,11 @@
 
 /* REF: reference/buffer selection */
 #define AD7949_CFG_BIT_REF		GENMASK(5, 3)
-#define AD7949_CFG_VAL_REF_EXT_BUF		7
+#define AD7949_CFG_VAL_REF_EXT_TEMP_BUF		3
+#define AD7949_CFG_VAL_REF_EXT_TEMP		2
+#define AD7949_CFG_VAL_REF_INT_4096		1
+#define AD7949_CFG_VAL_REF_INT_2500		0
+#define AD7949_CFG_VAL_REF_EXTERNAL		BIT(1)
 
 /* SEQ: channel sequencer. Allows for scanning channels */
 #define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
@@ -61,6 +65,14 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
 	[ID_AD7689] = { .num_channels = 8, .resolution = 16 },
 };
 
+/**
+ * struct ad7949_channel - ADC Channel parameters
+ * @refsel: reference selection
+ */
+struct ad7949_channel {
+	u32 refsel;
+};
+
 /**
  * struct ad7949_adc_chip - AD ADC chip
  * @lock: protects write sequences
@@ -78,6 +90,7 @@ struct ad7949_adc_chip {
 	struct regulator *vref;
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
+	struct ad7949_channel *channels;
 	u8 resolution;
 	u8 bits_per_word;
 	u16 cfg;
@@ -138,6 +151,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	int i;
 	u8 buf8[2];
 	struct spi_message msg;
+	struct ad7949_channel ad7949_chan = ad7949_adc->channels[channel];
 	struct spi_transfer tx[] = {
 		{
 			.rx_buf = &ad7949_adc->buffer,
@@ -154,8 +168,9 @@ 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,
-					   FIELD_PREP(AD7949_CFG_BIT_INX, channel),
-					   AD7949_CFG_BIT_INX);
+					   FIELD_PREP(AD7949_CFG_BIT_INX, channel) |
+					   FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_chan.refsel),
+					   AD7949_CFG_BIT_INX | AD7949_CFG_BIT_REF);
 		if (ret)
 			return ret;
 		if (channel == ad7949_adc->current_channel)
@@ -225,6 +240,7 @@ static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
 			   int *val, int *val2, long mask)
 {
 	struct ad7949_adc_chip *ad7949_adc = iio_priv(indio_dev);
+	struct ad7949_channel ad7949_chan = ad7949_adc->channels[chan->channel];
 	int ret;
 
 	if (!val)
@@ -242,12 +258,26 @@ 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_chan.refsel) {
+		case AD7949_CFG_VAL_REF_INT_2500:
+			*val = 2500;
+			break;
+		case AD7949_CFG_VAL_REF_INT_4096:
+			*val = 4096;
+			break;
+		case AD7949_CFG_VAL_REF_EXT_TEMP:
+		case AD7949_CFG_VAL_REF_EXT_TEMP_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;
@@ -286,7 +316,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_FULL, 1) |
-		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_CFG_VAL_REF_EXT_BUF) |
+		FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_adc->channels[0].refsel) |
 		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
 		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);
 
@@ -302,14 +332,24 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 	return ret;
 }
 
+static void ad7949_disable_reg(void *reg)
+{
+	regulator_disable(reg);
+}
+
 static int ad7949_spi_probe(struct spi_device *spi)
 {
 	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
 	struct device *dev = &spi->dev;
 	const struct ad7949_adc_spec *spec;
 	struct ad7949_adc_chip *ad7949_adc;
+	struct ad7949_channel *ad7949_chan;
+	struct device_node *child;
 	struct iio_dev *indio_dev;
+	int mode;
+	u32 tmp;
 	int ret;
+	int i;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
 	if (!indio_dev) {
@@ -343,16 +383,75 @@ static int ad7949_spi_probe(struct spi_device *spi)
 		return -EINVAL;
 	}
 
-	ad7949_adc->vref = devm_regulator_get(dev, "vref");
+	/* Setup external voltage ref, buffered? */
+	ad7949_adc->vref = devm_regulator_get(dev, "vrefin");
 	if (IS_ERR(ad7949_adc->vref)) {
-		dev_err(dev, "fail to request regulator\n");
-		return PTR_ERR(ad7949_adc->vref);
+		/* unbuffered? */
+		ad7949_adc->vref = devm_regulator_get(dev, "vref");
+		if (IS_ERR(ad7949_adc->vref)) {
+			/* Internal then */
+			mode = AD7949_CFG_VAL_REF_INT_4096;
+		}
+		mode = AD7949_CFG_VAL_REF_EXT_TEMP;
 	}
+	mode = AD7949_CFG_VAL_REF_EXT_TEMP_BUF;
 
-	ret = regulator_enable(ad7949_adc->vref);
-	if (ret < 0) {
-		dev_err(dev, "fail to enable regulator\n");
-		return ret;
+	if (mode > AD7949_CFG_VAL_REF_INT_4096) {
+		ret = regulator_enable(ad7949_adc->vref);
+		if (ret < 0) {
+			dev_err(dev, "fail to enable regulator\n");
+			return ret;
+		}
+
+		ret = devm_add_action_or_reset(dev, ad7949_disable_reg,
+					       ad7949_adc->vref);
+		if (ret)
+			return ret;
+	}
+
+	ad7949_adc->channels = devm_kcalloc(dev, spec->num_channels,
+					    sizeof(*ad7949_adc->channels),
+					    GFP_KERNEL);
+	if (!ad7949_adc->channels) {
+		dev_err(dev, "unable to allocate ADC channels\n");
+		return -ENOMEM;
+	}
+
+	/* Initialize all channel structures */
+	for (i = 0; i < spec->num_channels; i++) {
+		ad7949_adc->channels[i].refsel = mode;
+	}
+
+	/* Read channel specific information form the devicetree */
+	for_each_child_of_node(dev->of_node, child) {
+		ret = of_property_read_u32(child, "reg", &i);
+		if (ret) {
+			dev_err(dev, "missing reg property in child: %s\n",
+				child->full_name);
+			of_node_put(child);
+			return ret;
+		}
+
+		ad7949_chan = &ad7949_adc->channels[i];
+
+		ret = of_property_read_u32(child, "adi,internal-ref-mv", &tmp);
+		if (ret < 0 && ret != -EINVAL) {
+			of_node_put(child);
+			return ret;
+		}
+
+		switch (tmp) {
+		case 2500:
+			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_2500;
+			break;
+		case 4096:
+			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_4096;
+			break;
+		default:
+			dev_err(dev, "unsupported internal voltage reference\n");
+			of_node_put(child);
+			return -EINVAL;
+		}
 	}
 
 	mutex_init(&ad7949_adc->lock);
@@ -373,7 +472,6 @@ static int ad7949_spi_probe(struct spi_device *spi)
 
 err:
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
 
 	return ret;
 }
@@ -385,7 +483,6 @@ static int ad7949_spi_remove(struct spi_device *spi)
 
 	iio_device_unregister(indio_dev);
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
 
 	return 0;
 }
-- 
2.30.1.489.g328c10930387

[PATCH v3 4/4] dt-bindings: iio: adc: ad7949: add per channel reference

[Liam Beguin]

From: Liam Beguin <lvb@xiphos.com>

Add bindings documentation describing per channel reference voltage
selection.
This adds the adi,internal-ref-mv property, and child nodes for each
channel. This 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 <lvb@xiphos.com>
---
 .../bindings/iio/adc/adi,ad7949.yaml          | 71 +++++++++++++++++--
 1 file changed, 67 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
index 9b56bd4d5510..18abba3b0b4d 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
@@ -26,19 +26,65 @@ properties:
   reg:
     maxItems: 1
 
+  vrefin-supply:
+    description:
+      Buffered ADC reference voltage supply.
+
   vref-supply:
     description:
-      ADC reference voltage supply
+      Unbuffered ADC reference voltage supply.
 
   spi-max-frequency: true
 
-  "#io-channel-cells":
+  '#io-channel-cells':
     const: 1
 
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+
 required:
   - compatible
   - reg
-  - vref-supply
+
+patternProperties:
+  '^channel@([0-7])$':
+    type: object
+    description: |
+      Represents the external channels which are connected to the ADC.
+
+    properties:
+      reg:
+        description: |
+          The channel number.
+          Up to 4 channels, numbered from 0 to 3 for adi,ad7682.
+          Up to 8 channels, numbered from 0 to 7 for adi,ad7689 and adi,ad7949.
+        items:
+          minimum: 0
+          maximum: 7
+
+      adi,internal-ref-mv:
+        description: |
+          Internal reference voltage selection in millivolts.
+
+          If no internal reference is specified, the channel will default to the
+          external reference defined by vrefin-supply (or vref-supply).
+          vrefin-supply will take precedence over vref-supply if both are defined.
+
+          If no supplies are defined, the reference selection will default to
+          4096mV internal reference.
+
+        $ref: /schemas/types.yaml#/definitions/uint32
+        enum: [2500, 4096]
+        default: 4096
+
+    required:
+      - reg
+
+    additionalProperties: false
 
 additionalProperties: false
 
@@ -49,9 +95,26 @@ examples:
         #size-cells = <0>;
 
         adc@0 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
             compatible = "adi,ad7949";
             reg = <0>;
-            vref-supply = <&vdd_supply>;
+            vrefin-supply = <&vdd_supply>;
+
+            channel@0 {
+                adi,internal-ref-mv = <4096>;
+                reg = <0>;
+            };
+
+            channel@1 {
+                adi,internal-ref-mv = <2500>;
+                reg = <1>;
+            };
+
+            channel@2 {
+                reg = <2>;
+            };
         };
     };
 ...
-- 
2.30.1.489.g328c10930387

Re: [PATCH v3 4/4] dt-bindings: iio: adc: ad7949: add per channel reference

[Rob Herring]

On Tue, Jul 13, 2021 at 12:34:25AM -0400, Liam Beguin wrote:
> From: Liam Beguin <lvb@xiphos.com>
> 
> Add bindings documentation describing per channel reference voltage
> selection.
> This adds the adi,internal-ref-mv property, and child nodes for each
> channel. This 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 <lvb@xiphos.com>
> ---
>  .../bindings/iio/adc/adi,ad7949.yaml          | 71 +++++++++++++++++--
>  1 file changed, 67 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> index 9b56bd4d5510..18abba3b0b4d 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> @@ -26,19 +26,65 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  vrefin-supply:
> +    description:
> +      Buffered ADC reference voltage supply.
> +
>    vref-supply:
>      description:
> -      ADC reference voltage supply
> +      Unbuffered ADC reference voltage supply.
>  
>    spi-max-frequency: true
>  
> -  "#io-channel-cells":
> +  '#io-channel-cells':
>      const: 1
>  
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +

extra blank line.

>  required:
>    - compatible
>    - reg
> -  - vref-supply
> +
> +patternProperties:
> +  '^channel@([0-7])$':
> +    type: object
> +    description: |
> +      Represents the external channels which are connected to the ADC.
> +
> +    properties:
> +      reg:
> +        description: |
> +          The channel number.
> +          Up to 4 channels, numbered from 0 to 3 for adi,ad7682.
> +          Up to 8 channels, numbered from 0 to 7 for adi,ad7689 and adi,ad7949.
> +        items:
> +          minimum: 0
> +          maximum: 7
> +
> +      adi,internal-ref-mv:

Use standard unit suffix name. Then you can drop the type $ref.

> +        description: |
> +          Internal reference voltage selection in millivolts.
> +
> +          If no internal reference is specified, the channel will default to the
> +          external reference defined by vrefin-supply (or vref-supply).
> +          vrefin-supply will take precedence over vref-supply if both are defined.
> +
> +          If no supplies are defined, the reference selection will default to
> +          4096mV internal reference.
> +
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [2500, 4096]
> +        default: 4096
> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false
>  
>  additionalProperties: false
>  
> @@ -49,9 +95,26 @@ examples:
>          #size-cells = <0>;
>  
>          adc@0 {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
>              compatible = "adi,ad7949";
>              reg = <0>;
> -            vref-supply = <&vdd_supply>;
> +            vrefin-supply = <&vdd_supply>;
> +
> +            channel@0 {
> +                adi,internal-ref-mv = <4096>;
> +                reg = <0>;
> +            };
> +
> +            channel@1 {
> +                adi,internal-ref-mv = <2500>;
> +                reg = <1>;
> +            };
> +
> +            channel@2 {
> +                reg = <2>;
> +            };
>          };
>      };
>  ...
> -- 
> 2.30.1.489.g328c10930387
> 
> 

Re: [PATCH v3 1/4] iio: adc: ad7949: define and use bitfield names

[Jonathan Cameron]

On Tue, 13 Jul 2021 00:34:22 -0400
Liam Beguin <liambeguin@gmail.com> wrote:

> From: Liam Beguin <lvb@xiphos.com>
> 
> Replace raw configuration register values by using FIELD_PREP and
> defines to improve readability.
> 
> Signed-off-by: Liam Beguin <lvb@xiphos.com>

Nice. This looks good to me.  Will pick up when the rest of the
series is ready to go.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ad7949.c | 50 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 42 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index 1b4b3203e428..0b549b8bd7a9 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -11,12 +11,37 @@
>  #include <linux/module.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
> +#include <linux/bitfield.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
> +
> +/* CFG: Configuration Update */
> +#define AD7949_CFG_BIT_OVERWRITE	BIT(13)
> +
> +/* INCC: Input Channel Configuration */
> +#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
> +
> +/* INX: Input channel Selection in a binary fashion */
> +#define AD7949_CFG_BIT_INX		GENMASK(9, 7)
> +
> +/* BW: select bandwidth for low-pass filter. Full or Quarter */
> +#define AD7949_CFG_BIT_BW_FULL			BIT(6)
> +
> +/* REF: reference/buffer selection */
> +#define AD7949_CFG_BIT_REF		GENMASK(5, 3)
> +#define AD7949_CFG_VAL_REF_EXT_BUF		7
> +
> +/* SEQ: channel sequencer. Allows for scanning channels */
> +#define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
> +
> +/* RB: Read back the CFG register */
> +#define AD7949_CFG_BIT_RBN		BIT(0)
>  
>  enum {
>  	ID_AD7949 = 0,
> @@ -109,8 +134,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 +239,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_OVERWRITE, 1) |
> +		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_FULL, 1) |
> +		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_CFG_VAL_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.

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

[Jonathan Cameron]

On Tue, 13 Jul 2021 00:34:23 -0400
Liam Beguin <liambeguin@gmail.com> wrote:

> From: Liam Beguin <lvb@xiphos.com>
> 
> 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 <lvb@xiphos.com>

A small tidy up below that will make this even neater...

> ---
>  drivers/iio/adc/ad7949.c | 68 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 60 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index 0b549b8bd7a9..65f78751225b 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -12,6 +12,7 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
>  #include <linux/bitfield.h>
> +#include <asm/unaligned.h>
>  
>  #define AD7949_MASK_TOTAL		GENMASK(13, 0)
>  
> @@ -67,6 +68,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
> @@ -77,6 +79,7 @@ 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;
> @@ -86,19 +89,37 @@ 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;
> +	u8 buf8[2];
>  	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 */
> +		buf8[0] = FIELD_GET(GENMASK(13, 6), ad7949_adc->cfg);
> +		buf8[1] = FIELD_GET(GENMASK(5, 0), ad7949_adc->cfg) << 2;
> +		memcpy(&ad7949_adc->buffer, buf8, 2);

Can probably tidy this up given the form - all we actually need to do is ensure the
value ends up big endian.

		/* Type is only big endian for this case as must be done as two transfers */
		ad7949_adc->buffer = (u16)cpu_to_be16(ad7959_adc->cfg << 2);

> +		break;
> +	default:
> +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> +		return -EINVAL;
> +	}
> +
>  	spi_message_init_with_transfers(&msg, tx, 1);
>  	ret = spi_sync(ad7949_adc->spi, &msg);
>  
> @@ -115,14 +136,13 @@ 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);
> +	u8 buf8[2];

As below, not needed that I can see.

>  	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,
>  		},
>  	};
>  
> @@ -157,7 +177,26 @@ 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 */
> +		*val >>= 16 - ad7949_adc->resolution;
> +		break;
> +	case 14:
> +		*val = ad7949_adc->buffer & GENMASK(13, 0);
> +		break;
> +	case 8:
> +		memcpy(buf8, &ad7949_adc->buffer, 2);
> +		/* Convert byte array to u16, MSB first */
> +		*val = get_unaligned_be16(buf8);

Why the local copy first?

		*val = get_unaligned_be16(ad7949_adc->buffer);

should get what you want, it doesn't care about the type.
Even better, that buffer is aligned, so we can do
be16_to_cpu() safely I think...

> +		/* Shift-out padding bits */
> +		*val >>= 16 - ad7949_adc->resolution;
> +		break;
> +	default:
> +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> +		return -EINVAL;
> +	}
>  
>  	return 0;
>  }
> @@ -265,6 +304,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
>  
>  static int ad7949_spi_probe(struct spi_device *spi)
>  {
> +	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
>  	struct device *dev = &spi->dev;
>  	const struct ad7949_adc_spec *spec;
>  	struct ad7949_adc_chip *ad7949_adc;
> @@ -291,6 +331,18 @@ static int ad7949_spi_probe(struct spi_device *spi)
>  	indio_dev->num_channels = spec->num_channels;
>  	ad7949_adc->resolution = spec->resolution;
>  
> +	/* Set SPI bits per word */
> +	if (spi_ctrl_mask & SPI_BPW_MASK(ad7949_adc->resolution)) {
> +		ad7949_adc->bits_per_word = ad7949_adc->resolution;
> +	} else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
> +		ad7949_adc->bits_per_word = 16;
> +	} else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
> +		ad7949_adc->bits_per_word = 8;
> +	} else {
> +		dev_err(dev, "unable to find common BPW with spi controller\n");
> +		return -EINVAL;
> +	}
> +
>  	ad7949_adc->vref = devm_regulator_get(dev, "vref");
>  	if (IS_ERR(ad7949_adc->vref)) {
>  		dev_err(dev, "fail to request regulator\n");

Re: [PATCH v3 3/4] iio: adc: ad7949: add support for internal vref

[Jonathan Cameron]

On Tue, 13 Jul 2021 00:34:24 -0400
Liam Beguin <liambeguin@gmail.com> wrote:

> From: Liam Beguin <lvb@xiphos.com>
> 
> 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 <lvb@xiphos.com>

Hi Liam,

A few minor things inline.

Jonathan

> ---
>  drivers/iio/adc/ad7949.c | 133 +++++++++++++++++++++++++++++++++------
>  1 file changed, 115 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index 65f78751225b..8b78da7ead36 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -36,7 +36,11 @@
>  
>  /* REF: reference/buffer selection */
>  #define AD7949_CFG_BIT_REF		GENMASK(5, 3)
> -#define AD7949_CFG_VAL_REF_EXT_BUF		7
> +#define AD7949_CFG_VAL_REF_EXT_TEMP_BUF		3
> +#define AD7949_CFG_VAL_REF_EXT_TEMP		2
> +#define AD7949_CFG_VAL_REF_INT_4096		1
> +#define AD7949_CFG_VAL_REF_INT_2500		0
> +#define AD7949_CFG_VAL_REF_EXTERNAL		BIT(1)
>  
>  /* SEQ: channel sequencer. Allows for scanning channels */
>  #define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
> @@ -61,6 +65,14 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
>  	[ID_AD7689] = { .num_channels = 8, .resolution = 16 },
>  };
>  
> +/**
> + * struct ad7949_channel - ADC Channel parameters
> + * @refsel: reference selection
> + */
> +struct ad7949_channel {
> +	u32 refsel;
> +};
> +
>  /**
>   * struct ad7949_adc_chip - AD ADC chip
>   * @lock: protects write sequences
> @@ -78,6 +90,7 @@ struct ad7949_adc_chip {
>  	struct regulator *vref;
>  	struct iio_dev *indio_dev;
>  	struct spi_device *spi;
> +	struct ad7949_channel *channels;
>  	u8 resolution;
>  	u8 bits_per_word;
>  	u16 cfg;
> @@ -138,6 +151,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>  	int i;
>  	u8 buf8[2];
>  	struct spi_message msg;
> +	struct ad7949_channel ad7949_chan = ad7949_adc->channels[channel];

Better to use a pointer rather than copying the data.

>  	struct spi_transfer tx[] = {
>  		{
>  			.rx_buf = &ad7949_adc->buffer,
> @@ -154,8 +168,9 @@ 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,
> -					   FIELD_PREP(AD7949_CFG_BIT_INX, channel),
> -					   AD7949_CFG_BIT_INX);
> +					   FIELD_PREP(AD7949_CFG_BIT_INX, channel) |
> +					   FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_chan.refsel),
> +					   AD7949_CFG_BIT_INX | AD7949_CFG_BIT_REF);
>  		if (ret)
>  			return ret;
>  		if (channel == ad7949_adc->current_channel)
> @@ -225,6 +240,7 @@ static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
>  			   int *val, int *val2, long mask)
>  {
>  	struct ad7949_adc_chip *ad7949_adc = iio_priv(indio_dev);
> +	struct ad7949_channel ad7949_chan = ad7949_adc->channels[chan->channel];
>  	int ret;
>  
>  	if (!val)
> @@ -242,12 +258,26 @@ 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_chan.refsel) {
> +		case AD7949_CFG_VAL_REF_INT_2500:
> +			*val = 2500;
> +			break;
> +		case AD7949_CFG_VAL_REF_INT_4096:
> +			*val = 4096;
> +			break;
> +		case AD7949_CFG_VAL_REF_EXT_TEMP:
> +		case AD7949_CFG_VAL_REF_EXT_TEMP_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;
> @@ -286,7 +316,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_FULL, 1) |
> -		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_CFG_VAL_REF_EXT_BUF) |
> +		FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_adc->channels[0].refsel) |
>  		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
>  		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);
>  
> @@ -302,14 +332,24 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
>  	return ret;
>  }
>  
> +static void ad7949_disable_reg(void *reg)
> +{
> +	regulator_disable(reg);
> +}
> +
>  static int ad7949_spi_probe(struct spi_device *spi)
>  {
>  	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
>  	struct device *dev = &spi->dev;
>  	const struct ad7949_adc_spec *spec;
>  	struct ad7949_adc_chip *ad7949_adc;
> +	struct ad7949_channel *ad7949_chan;
> +	struct device_node *child;
>  	struct iio_dev *indio_dev;
> +	int mode;
> +	u32 tmp;
>  	int ret;
> +	int i;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
>  	if (!indio_dev) {
> @@ -343,16 +383,75 @@ static int ad7949_spi_probe(struct spi_device *spi)
>  		return -EINVAL;
>  	}
>  
> -	ad7949_adc->vref = devm_regulator_get(dev, "vref");
> +	/* Setup external voltage ref, buffered? */
> +	ad7949_adc->vref = devm_regulator_get(dev, "vrefin");
>  	if (IS_ERR(ad7949_adc->vref)) {
> -		dev_err(dev, "fail to request regulator\n");
> -		return PTR_ERR(ad7949_adc->vref);
> +		/* unbuffered? */
> +		ad7949_adc->vref = devm_regulator_get(dev, "vref");
> +		if (IS_ERR(ad7949_adc->vref)) {
> +			/* Internal then */
> +			mode = AD7949_CFG_VAL_REF_INT_4096;
> +		}
> +		mode = AD7949_CFG_VAL_REF_EXT_TEMP;
>  	}
> +	mode = AD7949_CFG_VAL_REF_EXT_TEMP_BUF;
>  
> -	ret = regulator_enable(ad7949_adc->vref);
> -	if (ret < 0) {
> -		dev_err(dev, "fail to enable regulator\n");
> -		return ret;
> +	if (mode > AD7949_CFG_VAL_REF_INT_4096) {

Range checks on enum values are always rather messy.  So
better to explicitly match the cases that matter.

> +		ret = regulator_enable(ad7949_adc->vref);
> +		if (ret < 0) {
> +			dev_err(dev, "fail to enable regulator\n");
> +			return ret;
> +		}
> +
> +		ret = devm_add_action_or_reset(dev, ad7949_disable_reg,
> +					       ad7949_adc->vref);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ad7949_adc->channels = devm_kcalloc(dev, spec->num_channels,
> +					    sizeof(*ad7949_adc->channels),
> +					    GFP_KERNEL);
> +	if (!ad7949_adc->channels) {
> +		dev_err(dev, "unable to allocate ADC channels\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* Initialize all channel structures */
> +	for (i = 0; i < spec->num_channels; i++) {
> +		ad7949_adc->channels[i].refsel = mode;

Kernel style is no brackets when only one item in a block.

> +	}
> +
> +	/* Read channel specific information form the devicetree */
> +	for_each_child_of_node(dev->of_node, child) {
> +		ret = of_property_read_u32(child, "reg", &i);
> +		if (ret) {
> +			dev_err(dev, "missing reg property in child: %s\n",
> +				child->full_name);
> +			of_node_put(child);
> +			return ret;
> +		}
> +
> +		ad7949_chan = &ad7949_adc->channels[i];
> +
> +		ret = of_property_read_u32(child, "adi,internal-ref-mv", &tmp);

Preference for generic device firmware properties not of_* specific ones.

I think there is nothing stopping this driver being probed from ACPI via PRP0001
before this change.  Note you'll also need fwnode_for_each_child_node() above.

> +		if (ret < 0 && ret != -EINVAL) {
> +			of_node_put(child);
> +			return ret;
> +		}
> +
> +		switch (tmp) {
> +		case 2500:
> +			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_2500;
> +			break;
> +		case 4096:
> +			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_4096;
> +			break;
> +		default:
> +			dev_err(dev, "unsupported internal voltage reference\n");
> +			of_node_put(child);
> +			return -EINVAL;
> +		}
>  	}
>  
>  	mutex_init(&ad7949_adc->lock);
> @@ -373,7 +472,6 @@ static int ad7949_spi_probe(struct spi_device *spi)
>  
>  err:
>  	mutex_destroy(&ad7949_adc->lock);
> -	regulator_disable(ad7949_adc->vref);
>  
>  	return ret;
>  }
> @@ -385,7 +483,6 @@ static int ad7949_spi_remove(struct spi_device *spi)
>  
>  	iio_device_unregister(indio_dev);
>  	mutex_destroy(&ad7949_adc->lock);
> -	regulator_disable(ad7949_adc->vref);

Given mutex_destroy is mostly pointless (and definitely is here) that
leaves us with just iio_device_register().   Hence if you could do
a follow up patch finishing conversion of this to entirely devm managed
that would be a nice addition to this series (or separate patch is fine as well).

>  
>  	return 0;
>  }

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

[Liam Beguin]

On Sat Jul 17, 2021 at 1:23 PM EDT, Jonathan Cameron wrote:
> On Tue, 13 Jul 2021 00:34:23 -0400
> Liam Beguin <liambeguin@gmail.com> wrote:
>
> > From: Liam Beguin <lvb@xiphos.com>
> > 
> > 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 <lvb@xiphos.com>
>
> A small tidy up below that will make this even neater...
>
> > ---
> >  drivers/iio/adc/ad7949.c | 68 +++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 60 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > index 0b549b8bd7a9..65f78751225b 100644
> > --- a/drivers/iio/adc/ad7949.c
> > +++ b/drivers/iio/adc/ad7949.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/spi/spi.h>
> >  #include <linux/bitfield.h>
> > +#include <asm/unaligned.h>
> >  
> >  #define AD7949_MASK_TOTAL		GENMASK(13, 0)
> >  
> > @@ -67,6 +68,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
> > @@ -77,6 +79,7 @@ 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;
> > @@ -86,19 +89,37 @@ 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;
> > +	u8 buf8[2];
> >  	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 */
> > +		buf8[0] = FIELD_GET(GENMASK(13, 6), ad7949_adc->cfg);
> > +		buf8[1] = FIELD_GET(GENMASK(5, 0), ad7949_adc->cfg) << 2;
> > +		memcpy(&ad7949_adc->buffer, buf8, 2);

Hi Jonathan,

Apologies for the delay on this.

>
> Can probably tidy this up given the form - all we actually need to do is
> ensure the
> value ends up big endian.
>
> /* Type is only big endian for this case as must be done as two
> transfers */
> ad7949_adc->buffer = (u16)cpu_to_be16(ad7959_adc->cfg << 2);

That's much nicer, I'll rework the patch using this.

Thanks,
Liam

>
> > +		break;
> > +	default:
> > +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	spi_message_init_with_transfers(&msg, tx, 1);
> >  	ret = spi_sync(ad7949_adc->spi, &msg);
> >  
> > @@ -115,14 +136,13 @@ 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);
> > +	u8 buf8[2];
>
> As below, not needed that I can see.
>
> >  	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,
> >  		},
> >  	};
> >  
> > @@ -157,7 +177,26 @@ 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 */
> > +		*val >>= 16 - ad7949_adc->resolution;
> > +		break;
> > +	case 14:
> > +		*val = ad7949_adc->buffer & GENMASK(13, 0);
> > +		break;
> > +	case 8:
> > +		memcpy(buf8, &ad7949_adc->buffer, 2);
> > +		/* Convert byte array to u16, MSB first */
> > +		*val = get_unaligned_be16(buf8);
>
> Why the local copy first?
>
> *val = get_unaligned_be16(ad7949_adc->buffer);
>
> should get what you want, it doesn't care about the type.
> Even better, that buffer is aligned, so we can do
> be16_to_cpu() safely I think...
>
> > +		/* Shift-out padding bits */
> > +		*val >>= 16 - ad7949_adc->resolution;
> > +		break;
> > +	default:
> > +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> > +		return -EINVAL;
> > +	}
> >  
> >  	return 0;
> >  }
> > @@ -265,6 +304,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
> >  
> >  static int ad7949_spi_probe(struct spi_device *spi)
> >  {
> > +	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
> >  	struct device *dev = &spi->dev;
> >  	const struct ad7949_adc_spec *spec;
> >  	struct ad7949_adc_chip *ad7949_adc;
> > @@ -291,6 +331,18 @@ static int ad7949_spi_probe(struct spi_device *spi)
> >  	indio_dev->num_channels = spec->num_channels;
> >  	ad7949_adc->resolution = spec->resolution;
> >  
> > +	/* Set SPI bits per word */
> > +	if (spi_ctrl_mask & SPI_BPW_MASK(ad7949_adc->resolution)) {
> > +		ad7949_adc->bits_per_word = ad7949_adc->resolution;
> > +	} else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
> > +		ad7949_adc->bits_per_word = 16;
> > +	} else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
> > +		ad7949_adc->bits_per_word = 8;
> > +	} else {
> > +		dev_err(dev, "unable to find common BPW with spi controller\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	ad7949_adc->vref = devm_regulator_get(dev, "vref");
> >  	if (IS_ERR(ad7949_adc->vref)) {
> >  		dev_err(dev, "fail to request regulator\n");

Re: [PATCH v3 3/4] iio: adc: ad7949: add support for internal vref

[Liam Beguin]

On Sat Jul 17, 2021 at 1:34 PM EDT, Jonathan Cameron wrote:
> On Tue, 13 Jul 2021 00:34:24 -0400
> Liam Beguin <liambeguin@gmail.com> wrote:
>
> > From: Liam Beguin <lvb@xiphos.com>
> > 
> > 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 <lvb@xiphos.com>
>
> Hi Liam,
>
> A few minor things inline.
>
> Jonathan
>
> > ---
> >  drivers/iio/adc/ad7949.c | 133 +++++++++++++++++++++++++++++++++------
> >  1 file changed, 115 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > index 65f78751225b..8b78da7ead36 100644
> > --- a/drivers/iio/adc/ad7949.c
> > +++ b/drivers/iio/adc/ad7949.c
> > @@ -36,7 +36,11 @@
> >  
> >  /* REF: reference/buffer selection */
> >  #define AD7949_CFG_BIT_REF		GENMASK(5, 3)
> > -#define AD7949_CFG_VAL_REF_EXT_BUF		7
> > +#define AD7949_CFG_VAL_REF_EXT_TEMP_BUF		3
> > +#define AD7949_CFG_VAL_REF_EXT_TEMP		2
> > +#define AD7949_CFG_VAL_REF_INT_4096		1
> > +#define AD7949_CFG_VAL_REF_INT_2500		0
> > +#define AD7949_CFG_VAL_REF_EXTERNAL		BIT(1)
> >  
> >  /* SEQ: channel sequencer. Allows for scanning channels */
> >  #define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
> > @@ -61,6 +65,14 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
> >  	[ID_AD7689] = { .num_channels = 8, .resolution = 16 },
> >  };
> >  
> > +/**
> > + * struct ad7949_channel - ADC Channel parameters
> > + * @refsel: reference selection
> > + */
> > +struct ad7949_channel {
> > +	u32 refsel;
> > +};
> > +
> >  /**
> >   * struct ad7949_adc_chip - AD ADC chip
> >   * @lock: protects write sequences
> > @@ -78,6 +90,7 @@ struct ad7949_adc_chip {
> >  	struct regulator *vref;
> >  	struct iio_dev *indio_dev;
> >  	struct spi_device *spi;
> > +	struct ad7949_channel *channels;
> >  	u8 resolution;
> >  	u8 bits_per_word;
> >  	u16 cfg;
> > @@ -138,6 +151,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> >  	int i;
> >  	u8 buf8[2];
> >  	struct spi_message msg;
> > +	struct ad7949_channel ad7949_chan = ad7949_adc->channels[channel];
>
> Better to use a pointer rather than copying the data.
>
> >  	struct spi_transfer tx[] = {
> >  		{
> >  			.rx_buf = &ad7949_adc->buffer,
> > @@ -154,8 +168,9 @@ 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,
> > -					   FIELD_PREP(AD7949_CFG_BIT_INX, channel),
> > -					   AD7949_CFG_BIT_INX);
> > +					   FIELD_PREP(AD7949_CFG_BIT_INX, channel) |
> > +					   FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_chan.refsel),
> > +					   AD7949_CFG_BIT_INX | AD7949_CFG_BIT_REF);
> >  		if (ret)
> >  			return ret;
> >  		if (channel == ad7949_adc->current_channel)
> > @@ -225,6 +240,7 @@ static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
> >  			   int *val, int *val2, long mask)
> >  {
> >  	struct ad7949_adc_chip *ad7949_adc = iio_priv(indio_dev);
> > +	struct ad7949_channel ad7949_chan = ad7949_adc->channels[chan->channel];
> >  	int ret;
> >  
> >  	if (!val)
> > @@ -242,12 +258,26 @@ 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_chan.refsel) {
> > +		case AD7949_CFG_VAL_REF_INT_2500:
> > +			*val = 2500;
> > +			break;
> > +		case AD7949_CFG_VAL_REF_INT_4096:
> > +			*val = 4096;
> > +			break;
> > +		case AD7949_CFG_VAL_REF_EXT_TEMP:
> > +		case AD7949_CFG_VAL_REF_EXT_TEMP_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;
> > @@ -286,7 +316,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_FULL, 1) |
> > -		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_CFG_VAL_REF_EXT_BUF) |
> > +		FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_adc->channels[0].refsel) |
> >  		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
> >  		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);
> >  
> > @@ -302,14 +332,24 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
> >  	return ret;
> >  }
> >  
> > +static void ad7949_disable_reg(void *reg)
> > +{
> > +	regulator_disable(reg);
> > +}
> > +
> >  static int ad7949_spi_probe(struct spi_device *spi)
> >  {
> >  	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
> >  	struct device *dev = &spi->dev;
> >  	const struct ad7949_adc_spec *spec;
> >  	struct ad7949_adc_chip *ad7949_adc;
> > +	struct ad7949_channel *ad7949_chan;
> > +	struct device_node *child;
> >  	struct iio_dev *indio_dev;
> > +	int mode;
> > +	u32 tmp;
> >  	int ret;
> > +	int i;
> >  
> >  	indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
> >  	if (!indio_dev) {
> > @@ -343,16 +383,75 @@ static int ad7949_spi_probe(struct spi_device *spi)
> >  		return -EINVAL;
> >  	}
> >  
> > -	ad7949_adc->vref = devm_regulator_get(dev, "vref");
> > +	/* Setup external voltage ref, buffered? */
> > +	ad7949_adc->vref = devm_regulator_get(dev, "vrefin");
> >  	if (IS_ERR(ad7949_adc->vref)) {
> > -		dev_err(dev, "fail to request regulator\n");
> > -		return PTR_ERR(ad7949_adc->vref);
> > +		/* unbuffered? */
> > +		ad7949_adc->vref = devm_regulator_get(dev, "vref");
> > +		if (IS_ERR(ad7949_adc->vref)) {
> > +			/* Internal then */
> > +			mode = AD7949_CFG_VAL_REF_INT_4096;
> > +		}
> > +		mode = AD7949_CFG_VAL_REF_EXT_TEMP;
> >  	}
> > +	mode = AD7949_CFG_VAL_REF_EXT_TEMP_BUF;
> >  
> > -	ret = regulator_enable(ad7949_adc->vref);
> > -	if (ret < 0) {
> > -		dev_err(dev, "fail to enable regulator\n");
> > -		return ret;
> > +	if (mode > AD7949_CFG_VAL_REF_INT_4096) {
>
> Range checks on enum values are always rather messy. So
> better to explicitly match the cases that matter.
>

Understood, I'll use AD7949_CFG_VAL_REF_EXTERNAL instead.

> > +		ret = regulator_enable(ad7949_adc->vref);
> > +		if (ret < 0) {
> > +			dev_err(dev, "fail to enable regulator\n");
> > +			return ret;
> > +		}
> > +
> > +		ret = devm_add_action_or_reset(dev, ad7949_disable_reg,
> > +					       ad7949_adc->vref);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	ad7949_adc->channels = devm_kcalloc(dev, spec->num_channels,
> > +					    sizeof(*ad7949_adc->channels),
> > +					    GFP_KERNEL);
> > +	if (!ad7949_adc->channels) {
> > +		dev_err(dev, "unable to allocate ADC channels\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	/* Initialize all channel structures */
> > +	for (i = 0; i < spec->num_channels; i++) {
> > +		ad7949_adc->channels[i].refsel = mode;
>
> Kernel style is no brackets when only one item in a block.
>
> > +	}
> > +
> > +	/* Read channel specific information form the devicetree */
> > +	for_each_child_of_node(dev->of_node, child) {
> > +		ret = of_property_read_u32(child, "reg", &i);
> > +		if (ret) {
> > +			dev_err(dev, "missing reg property in child: %s\n",
> > +				child->full_name);
> > +			of_node_put(child);
> > +			return ret;
> > +		}
> > +
> > +		ad7949_chan = &ad7949_adc->channels[i];
> > +
> > +		ret = of_property_read_u32(child, "adi,internal-ref-mv", &tmp);
>
> Preference for generic device firmware properties not of_* specific
> ones.
>
> I think there is nothing stopping this driver being probed from ACPI via
> PRP0001
> before this change. Note you'll also need fwnode_for_each_child_node()
> above.
>

will do.

> > +		if (ret < 0 && ret != -EINVAL) {
> > +			of_node_put(child);
> > +			return ret;
> > +		}
> > +
> > +		switch (tmp) {
> > +		case 2500:
> > +			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_2500;
> > +			break;
> > +		case 4096:
> > +			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_4096;
> > +			break;
> > +		default:
> > +			dev_err(dev, "unsupported internal voltage reference\n");
> > +			of_node_put(child);
> > +			return -EINVAL;
> > +		}
> >  	}
> >  
> >  	mutex_init(&ad7949_adc->lock);
> > @@ -373,7 +472,6 @@ static int ad7949_spi_probe(struct spi_device *spi)
> >  
> >  err:
> >  	mutex_destroy(&ad7949_adc->lock);
> > -	regulator_disable(ad7949_adc->vref);
> >  
> >  	return ret;
> >  }
> > @@ -385,7 +483,6 @@ static int ad7949_spi_remove(struct spi_device *spi)
> >  
> >  	iio_device_unregister(indio_dev);
> >  	mutex_destroy(&ad7949_adc->lock);
> > -	regulator_disable(ad7949_adc->vref);
>
> Given mutex_destroy is mostly pointless (and definitely is here) that
> leaves us with just iio_device_register(). Hence if you could do
> a follow up patch finishing conversion of this to entirely devm managed
> that would be a nice addition to this series (or separate patch is fine
> as well).
>

Sure, I'll add that cleanup in the next revision.

Thanks,
Liam

> >  
> >  	return 0;
> >  }