[PATCH v5 0/5] AD7949 Fixes

[PATCH v5 0/5] 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 v4:
- fix premature deletion of define
- use separate be16 buffer for 8-bit transfers
- switch to devm_regulator_get_optional()
- fix vref setup
- apply Reviewed-by

Changes since v3:
- use cpu_to_be16 and be16_to_cpu instead of manual conversion
- use pointers to channel structures instead of copies
- switch to generic device firmware property API
- use standard unit suffix names (mV to microvolt)
- switch to devm_iio_device_register() for additional cleanup

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 (5):
  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
  iio: adc: ad7949: use devm managed functions

 .../bindings/iio/adc/adi,ad7949.yaml          |  69 ++++-
 drivers/iio/adc/ad7949.c                      | 281 ++++++++++++++----
 2 files changed, 291 insertions(+), 59 deletions(-)

Range-diff against v4:
1:  8760b368f971 ! 1:  a5c211185661 iio: adc: ad7949: define and use bitfield names
    @@ drivers/iio/adc/ad7949.c
      #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_REG_SIZE_BITS	14
    + 
     +/* CFG: Configuration Update */
     +#define AD7949_CFG_BIT_OVERWRITE	BIT(13)
     +
    @@ drivers/iio/adc/ad7949.c
     +
     +/* 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++) {
2:  7b1484f2fc4c ! 2:  df2f6df8f3d5 iio: adc: ad7949: fix spi messages on non 14-bit controllers
    @@ Commit message
         Signed-off-by: Liam Beguin <lvb@xiphos.com>
     
      ## drivers/iio/adc/ad7949.c ##
    +@@
    + #include <linux/bitfield.h>
    + 
    + #define AD7949_MASK_TOTAL		GENMASK(13, 0)
    +-#define AD7949_CFG_REG_SIZE_BITS	14
    + 
    + /* CFG: Configuration Update */
    + #define AD7949_CFG_BIT_OVERWRITE	BIT(13)
     @@ 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: static const struct ad7949_adc_spec ad7949_adc_spec[]
       * @cfg: copy of the configuration register
       * @current_channel: current channel in use
       * @buffer: buffer to send / receive data to / from device
    ++ * @buf8b: be16 buffer to exchange data with the device in 8-bit transfers
    +  */
    + struct ad7949_adc_chip {
    + 	struct mutex lock;
     @@ drivers/iio/adc/ad7949.c: struct ad7949_adc_chip {
      	struct iio_dev *indio_dev;
      	struct spi_device *spi;
    @@ drivers/iio/adc/ad7949.c: struct ad7949_adc_chip {
      	u16 cfg;
      	unsigned int current_channel;
      	u16 buffer ____cacheline_aligned;
    -@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
    ++	__be16 buf8b;
    + };
    + 
    + static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
      				u16 mask)
      {
      	int ret;
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_write_cfg(struct ad7949_adc_chip
      	struct spi_message msg;
      	struct spi_transfer tx[] = {
      		{
    - 			.tx_buf = &ad7949_adc->buffer,
    +-			.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:
    ++		tx[0].tx_buf = &ad7949_adc->buffer;
     +		ad7949_adc->buffer = ad7949_adc->cfg << 2;
     +		break;
     +	case 14:
    ++		tx[0].tx_buf = &ad7949_adc->buffer;
     +		ad7949_adc->buffer = ad7949_adc->cfg;
     +		break;
     +	case 8:
     +		/* Here, type is big endian as it must be sent in two transfers */
    -+		ad7949_adc->buffer = (u16)cpu_to_be16(ad7949_adc->cfg << 2);
    ++		tx[0].tx_buf = &ad7949_adc->buf8b;
    ++		ad7949_adc->buf8b = cpu_to_be16(ad7949_adc->cfg << 2);
     +		break;
     +	default:
     +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_c
      	struct spi_message msg;
      	struct spi_transfer tx[] = {
      		{
    - 			.rx_buf = &ad7949_adc->buffer,
    +-			.rx_buf = &ad7949_adc->buffer,
      			.len = 2,
     -			.bits_per_word = bits_per_word,
     +			.bits_per_word = ad7949_adc->bits_per_word,
      		},
      	};
      
    ++	if (ad7949_adc->bits_per_word == 8)
    ++		tx[0].rx_buf = &ad7949_adc->buf8b;
    ++	else
    ++		tx[0].rx_buf = &ad7949_adc->buffer;
    ++
    + 	/*
    + 	 * 1: write CFG for sample N and read old data (sample N-2)
    + 	 * 2: if CFG was not changed since sample N-1 then we'll get good data
    +@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
    + 	}
    + 
    + 	/* 3: write something and read actual data */
    +-	ad7949_adc->buffer = 0;
    + 	spi_message_init_with_transfers(&msg, tx, 1);
    + 	ret = spi_sync(ad7949_adc->spi, &msg);
    + 	if (ret)
     @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
      
      	ad7949_adc->current_channel = channel;
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_c
     +		break;
     +	case 8:
     +		/* Here, type is big endian as data was sent in two transfers */
    -+		*val = be16_to_cpu(ad7949_adc->buffer);
    ++		*val = be16_to_cpu(ad7949_adc->buf8b);
     +		/* Shift-out padding bits */
     +		*val >>= 16 - ad7949_adc->resolution;
     +		break;
3:  41c4ab9c5e19 ! 3:  8a33618a4f90 iio: adc: ad7949: add support for internal vref
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_c
     +	struct ad7949_channel *ad7949_chan = &ad7949_adc->channels[channel];
      	struct spi_transfer tx[] = {
      		{
    - 			.rx_buf = &ad7949_adc->buffer,
    + 			.len = 2,
     @@ 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_probe(struct spi_device *spi)
      
     -	ad7949_adc->vref = devm_regulator_get(dev, "vref");
     +	/* Setup external voltage ref, buffered? */
    -+	ad7949_adc->vref = devm_regulator_get(dev, "vrefin");
    ++	ad7949_adc->vref = devm_regulator_get_optional(dev, "vrefin");
      	if (IS_ERR(ad7949_adc->vref)) {
     -		dev_err(dev, "fail to request regulator\n");
     -		return PTR_ERR(ad7949_adc->vref);
    ++		ret = PTR_ERR(ad7949_adc->vref);
    ++		if (ret != -ENODEV)
    ++			return ret;
     +		/* unbuffered? */
    -+		ad7949_adc->vref = devm_regulator_get(dev, "vref");
    ++		ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
     +		if (IS_ERR(ad7949_adc->vref)) {
    ++			ret = PTR_ERR(ad7949_adc->vref);
    ++			if (ret != -ENODEV)
    ++				return ret;
     +			/* Internal then */
     +			mode = AD7949_CFG_VAL_REF_INT_4096;
    ++		} else {
    ++			mode = AD7949_CFG_VAL_REF_EXT_TEMP;
     +		}
    -+		mode = AD7949_CFG_VAL_REF_EXT_TEMP;
    ++	} else {
    ++		mode = AD7949_CFG_VAL_REF_EXT_TEMP_BUF;
      	}
    -+	mode = AD7949_CFG_VAL_REF_EXT_TEMP_BUF;
      
     -	ret = regulator_enable(ad7949_adc->vref);
     -	if (ret < 0) {
4:  9cb48acbd05b ! 4:  7612ff29db6b dt-bindings: iio: adc: ad7949: add per channel reference
    @@ Commit message
         calculation.
     
         Signed-off-by: Liam Beguin <lvb@xiphos.com>
    +    Reviewed-by: Rob Herring <robh@kernel.org>
     
      ## Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml ##
     @@ Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml: properties:
5:  c48eb017058c = 5:  74ee82caba57 iio: adc: ad7949: use devm managed functions

base-commit: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
-- 
2.32.0.452.g940fe202adcb

[PATCH v5 1/5] 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 | 49 ++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 1b4b3203e428..937241ee2a22 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -11,13 +11,39 @@
 #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,
 	ID_AD7682,
@@ -109,8 +135,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 +240,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.32.0.452.g940fe202adcb

[PATCH v5 2/5] 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 | 75 +++++++++++++++++++++++++++++++++-------
 1 file changed, 63 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 937241ee2a22..3f94ae639a44 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -14,7 +14,6 @@
 #include <linux/bitfield.h>
 
 #define AD7949_MASK_TOTAL		GENMASK(13, 0)
-#define AD7949_CFG_REG_SIZE_BITS	14
 
 /* CFG: Configuration Update */
 #define AD7949_CFG_BIT_OVERWRITE	BIT(13)
@@ -68,9 +67,11 @@ 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
+ * @buf8b: be16 buffer to exchange data with the device in 8-bit transfers
  */
 struct ad7949_adc_chip {
 	struct mutex lock;
@@ -78,28 +79,46 @@ 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;
+	__be16 buf8b;
 };
 
 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->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
-	ad7949_adc->buffer = ad7949_adc->cfg << shift;
+
+	switch (ad7949_adc->bits_per_word) {
+	case 16:
+		tx[0].tx_buf = &ad7949_adc->buffer;
+		ad7949_adc->buffer = ad7949_adc->cfg << 2;
+		break;
+	case 14:
+		tx[0].tx_buf = &ad7949_adc->buffer;
+		ad7949_adc->buffer = ad7949_adc->cfg;
+		break;
+	case 8:
+		/* Here, type is big endian as it must be sent in two transfers */
+		tx[0].tx_buf = &ad7949_adc->buf8b;
+		ad7949_adc->buf8b = cpu_to_be16(ad7949_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);
 
@@ -116,17 +135,19 @@ 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,
 		},
 	};
 
+	if (ad7949_adc->bits_per_word == 8)
+		tx[0].rx_buf = &ad7949_adc->buf8b;
+	else
+		tx[0].rx_buf = &ad7949_adc->buffer;
+
 	/*
 	 * 1: write CFG for sample N and read old data (sample N-2)
 	 * 2: if CFG was not changed since sample N-1 then we'll get good data
@@ -144,7 +165,6 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	}
 
 	/* 3: write something and read actual data */
-	ad7949_adc->buffer = 0;
 	spi_message_init_with_transfers(&msg, tx, 1);
 	ret = spi_sync(ad7949_adc->spi, &msg);
 	if (ret)
@@ -158,7 +178,25 @@ 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:
+		/* Here, type is big endian as data was sent in two transfers */
+		*val = be16_to_cpu(ad7949_adc->buf8b);
+		/* 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;
 }
@@ -266,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;
@@ -292,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.32.0.452.g940fe202adcb

[PATCH v5 3/5] 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 | 140 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 122 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 3f94ae639a44..14a7c79d637e 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -35,7 +35,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)
@@ -60,6 +64,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;
@@ -136,6 +149,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	int ret;
 	int i;
 	struct spi_message msg;
+	struct ad7949_channel *ad7949_chan = &ad7949_adc->channels[channel];
 	struct spi_transfer tx[] = {
 		{
 			.len = 2,
@@ -156,8 +170,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 fwnode_handle *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,82 @@ 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_optional(dev, "vrefin");
 	if (IS_ERR(ad7949_adc->vref)) {
-		dev_err(dev, "fail to request regulator\n");
-		return PTR_ERR(ad7949_adc->vref);
+		ret = PTR_ERR(ad7949_adc->vref);
+		if (ret != -ENODEV)
+			return ret;
+		/* unbuffered? */
+		ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
+		if (IS_ERR(ad7949_adc->vref)) {
+			ret = PTR_ERR(ad7949_adc->vref);
+			if (ret != -ENODEV)
+				return ret;
+			/* Internal then */
+			mode = AD7949_CFG_VAL_REF_INT_4096;
+		} else {
+			mode = AD7949_CFG_VAL_REF_EXT_TEMP;
+		}
+	} else {
+		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_EXTERNAL) {
+		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 */
+	device_for_each_child_node(dev, child) {
+		ret = fwnode_property_read_u32(child, "reg", &i);
+		if (ret) {
+			dev_err(dev, "missing reg property in %pfw\n", child);
+			fwnode_handle_put(child);
+			return ret;
+		}
+
+		ad7949_chan = &ad7949_adc->channels[i];
+
+		ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
+		if (ret < 0 && ret != -EINVAL) {
+			dev_err(dev, "invalid internal reference in %pfw\n", child);
+			fwnode_handle_put(child);
+			return ret;
+		}
+
+		switch (tmp) {
+		case 2500000:
+			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_2500;
+			break;
+		case 4096000:
+			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_4096;
+			break;
+		default:
+			dev_err(dev, "unsupported internal voltage reference\n");
+			fwnode_handle_put(child);
+			return -EINVAL;
+		}
 	}
 
 	mutex_init(&ad7949_adc->lock);
@@ -373,7 +479,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 +490,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.32.0.452.g940fe202adcb

[PATCH v5 4/5] 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-microvolt 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/iio/adc/adi,ad7949.yaml          | 69 +++++++++++++++++--
 1 file changed, 65 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..893f72b8081e 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
@@ -26,19 +26,63 @@ 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-microvolt:
+        description: |
+          Internal reference voltage selection in microvolts.
+
+          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.
+
+        enum: [2500000, 4096000]
+        default: 4096000
+
+    required:
+      - reg
+
+    additionalProperties: false
 
 additionalProperties: false
 
@@ -49,9 +93,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-microvolt = <4096000>;
+                reg = <0>;
+            };
+
+            channel@1 {
+                adi,internal-ref-microvolt = <2500000>;
+                reg = <1>;
+            };
+
+            channel@2 {
+                reg = <2>;
+            };
         };
     };
 ...
-- 
2.32.0.452.g940fe202adcb

[PATCH v5 5/5] iio: adc: ad7949: use devm managed functions

[Liam Beguin]

From: Liam Beguin <lvb@xiphos.com>

Switch to devm_iio_device_register to finalize devm migration.
This removes the use for iio_device_unregister() and since
mutex_destroy() is not necessary here, remove it altogether.

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

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 14a7c79d637e..71561eb7898e 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -466,34 +466,16 @@ static int ad7949_spi_probe(struct spi_device *spi)
 	ret = ad7949_spi_init(ad7949_adc);
 	if (ret) {
 		dev_err(dev, "enable to init this device: %d\n", ret);
-		goto err;
+		return ret;
 	}
 
-	ret = iio_device_register(indio_dev);
-	if (ret) {
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
 		dev_err(dev, "fail to register iio device: %d\n", ret);
-		goto err;
-	}
-
-	return 0;
-
-err:
-	mutex_destroy(&ad7949_adc->lock);
 
 	return ret;
 }
 
-static int ad7949_spi_remove(struct spi_device *spi)
-{
-	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-	struct ad7949_adc_chip *ad7949_adc = iio_priv(indio_dev);
-
-	iio_device_unregister(indio_dev);
-	mutex_destroy(&ad7949_adc->lock);
-
-	return 0;
-}
-
 static const struct of_device_id ad7949_spi_of_id[] = {
 	{ .compatible = "adi,ad7949" },
 	{ .compatible = "adi,ad7682" },
@@ -516,7 +498,6 @@ static struct spi_driver ad7949_spi_driver = {
 		.of_match_table	= ad7949_spi_of_id,
 	},
 	.probe	  = ad7949_spi_probe,
-	.remove   = ad7949_spi_remove,
 	.id_table = ad7949_spi_id,
 };
 module_spi_driver(ad7949_spi_driver);
-- 
2.32.0.452.g940fe202adcb

Re: [PATCH v5 3/5] iio: adc: ad7949: add support for internal vref

[Jonathan Cameron]

On Sat,  7 Aug 2021 21:56:57 -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, I'm not sure the parsing of the child nodes will work if using
an external reference. See below.

Jonathan

> ---
>  drivers/iio/adc/ad7949.c | 140 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 122 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index 3f94ae639a44..14a7c79d637e 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -35,7 +35,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)
> @@ -60,6 +64,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;
> @@ -136,6 +149,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>  	int ret;
>  	int i;
>  	struct spi_message msg;
> +	struct ad7949_channel *ad7949_chan = &ad7949_adc->channels[channel];
>  	struct spi_transfer tx[] = {
>  		{
>  			.len = 2,
> @@ -156,8 +170,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 fwnode_handle *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,82 @@ 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_optional(dev, "vrefin");
>  	if (IS_ERR(ad7949_adc->vref)) {
> -		dev_err(dev, "fail to request regulator\n");
> -		return PTR_ERR(ad7949_adc->vref);
> +		ret = PTR_ERR(ad7949_adc->vref);
> +		if (ret != -ENODEV)
> +			return ret;
> +		/* unbuffered? */
> +		ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
> +		if (IS_ERR(ad7949_adc->vref)) {
> +			ret = PTR_ERR(ad7949_adc->vref);
> +			if (ret != -ENODEV)
> +				return ret;
> +			/* Internal then */
> +			mode = AD7949_CFG_VAL_REF_INT_4096;
> +		} else {
> +			mode = AD7949_CFG_VAL_REF_EXT_TEMP;
> +		}
> +	} else {
> +		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_EXTERNAL) {
> +		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 */
> +	device_for_each_child_node(dev, child) {
> +		ret = fwnode_property_read_u32(child, "reg", &i);
> +		if (ret) {
> +			dev_err(dev, "missing reg property in %pfw\n", child);
> +			fwnode_handle_put(child);
> +			return ret;
> +		}
> +
> +		ad7949_chan = &ad7949_adc->channels[i];
> +
> +		ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
> +		if (ret < 0 && ret != -EINVAL) {
> +			dev_err(dev, "invalid internal reference in %pfw\n", child);
> +			fwnode_handle_put(child);
> +			return ret;
> +		}

So if we are using external voltage, then we'd not expect this to exist. 
ret != -EINVAL should deal with that.  However, we then hit the following switch
and temp isn't set to an appropriate value so we get the error.

Am I missing something?

> +
> +		switch (tmp) {
> +		case 2500000:
> +			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_2500;
> +			break;
> +		case 4096000:
> +			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_4096;
> +			break;
> +		default:
> +			dev_err(dev, "unsupported internal voltage reference\n");
> +			fwnode_handle_put(child);
> +			return -EINVAL;
> +		}
>  	}
>  
>  	mutex_init(&ad7949_adc->lock);
> @@ -373,7 +479,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 +490,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;
>  }

Re: [PATCH v5 1/5] iio: adc: ad7949: define and use bitfield names

[Joe Perches]

On Sat, 2021-08-07 at 21:56 -0400, Liam Beguin wrote:
> Replace raw configuration register values by using FIELD_PREP and
> defines to improve readability.
[]
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
[]
+#define AD7949_CFG_BIT_INCC		GENMASK(12, 10)

I think the naming is a bit confusing as it appears as if
these bitfield ranges are single bits.

> +/* REF: reference/buffer selection */
> +#define AD7949_CFG_BIT_REF		GENMASK(5, 3)
[]
> +/* SEQ: channel sequencer. Allows for scanning channels */
> +#define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
> 

Re: [PATCH v5 3/5] iio: adc: ad7949: add support for internal vref

[Liam Beguin]

On Sun Aug 8, 2021 at 12:36 PM EDT, Jonathan Cameron wrote:
> On Sat, 7 Aug 2021 21:56:57 -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, I'm not sure the parsing of the child nodes will work if using
> an external reference. See below.
>
> Jonathan
>
> > ---
> >  drivers/iio/adc/ad7949.c | 140 ++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 122 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > index 3f94ae639a44..14a7c79d637e 100644
> > --- a/drivers/iio/adc/ad7949.c
> > +++ b/drivers/iio/adc/ad7949.c
> > @@ -35,7 +35,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)
> > @@ -60,6 +64,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;
> > @@ -136,6 +149,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> >  	int ret;
> >  	int i;
> >  	struct spi_message msg;
> > +	struct ad7949_channel *ad7949_chan = &ad7949_adc->channels[channel];
> >  	struct spi_transfer tx[] = {
> >  		{
> >  			.len = 2,
> > @@ -156,8 +170,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 fwnode_handle *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,82 @@ 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_optional(dev, "vrefin");
> >  	if (IS_ERR(ad7949_adc->vref)) {
> > -		dev_err(dev, "fail to request regulator\n");
> > -		return PTR_ERR(ad7949_adc->vref);
> > +		ret = PTR_ERR(ad7949_adc->vref);
> > +		if (ret != -ENODEV)
> > +			return ret;
> > +		/* unbuffered? */
> > +		ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
> > +		if (IS_ERR(ad7949_adc->vref)) {
> > +			ret = PTR_ERR(ad7949_adc->vref);
> > +			if (ret != -ENODEV)
> > +				return ret;
> > +			/* Internal then */
> > +			mode = AD7949_CFG_VAL_REF_INT_4096;
> > +		} else {
> > +			mode = AD7949_CFG_VAL_REF_EXT_TEMP;
> > +		}
> > +	} else {
> > +		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_EXTERNAL) {
> > +		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 */
> > +	device_for_each_child_node(dev, child) {
> > +		ret = fwnode_property_read_u32(child, "reg", &i);
> > +		if (ret) {
> > +			dev_err(dev, "missing reg property in %pfw\n", child);
> > +			fwnode_handle_put(child);
> > +			return ret;
> > +		}
> > +
> > +		ad7949_chan = &ad7949_adc->channels[i];
> > +
> > +		ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
> > +		if (ret < 0 && ret != -EINVAL) {
> > +			dev_err(dev, "invalid internal reference in %pfw\n", child);
> > +			fwnode_handle_put(child);
> > +			return ret;
> > +		}

Hi Jonathan,

>
> So if we are using external voltage, then we'd not expect this to exist.
> ret != -EINVAL should deal with that. However, we then hit the following
> switch
> and temp isn't set to an appropriate value so we get the error.
>
> Am I missing something?

You're right that using an external reference, will cause this to fail.
My apologies, I've done a poor job of testing the last two revisions of
this patch. I'll do better.

Since a regulator is also required when we're using an external source,
I'll add a check for that. Something like this:

	ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
	if (ret == -EINVAL && mode & AD7949_CFG_VAL_REF_EXTERNAL) {
		continue;
	} else if (ret < 0) {
		dev_err(dev, "invalid voltage reference in %pfw\n", child);
		fwnode_handle_put(child);
		return ret;
	}

Given that we can now have a different scale for each channel based on
the vref source, should BIT(IIO_CHAN_INFO_SCALE) be moved to
.info_mask_separate in the channel definition?

Thanks,
Liam

>
> > +
> > +		switch (tmp) {
> > +		case 2500000:
> > +			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_2500;
> > +			break;
> > +		case 4096000:
> > +			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_4096;
> > +			break;
> > +		default:
> > +			dev_err(dev, "unsupported internal voltage reference\n");
> > +			fwnode_handle_put(child);
> > +			return -EINVAL;
> > +		}
> >  	}
> >  
> >  	mutex_init(&ad7949_adc->lock);
> > @@ -373,7 +479,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 +490,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;
> >  }

Re: [PATCH v5 1/5] iio: adc: ad7949: define and use bitfield names

[Liam Beguin]

On Sun Aug 8, 2021 at 12:51 PM EDT, Joe Perches wrote:
> On Sat, 2021-08-07 at 21:56 -0400, Liam Beguin wrote:
> > Replace raw configuration register values by using FIELD_PREP and
> > defines to improve readability.
> []
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> []
> +#define AD7949_CFG_BIT_INCC GENMASK(12, 10)
>

Hi Joe,

> I think the naming is a bit confusing as it appears as if
> these bitfield ranges are single bits.

That makes sense.
Would AD7949_CFG_BITS_* be good enough?

Thanks,
Liam

>
> > +/* REF: reference/buffer selection */
> > +#define AD7949_CFG_BIT_REF		GENMASK(5, 3)
> []
> > +/* SEQ: channel sequencer. Allows for scanning channels */
> > +#define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
> > 

Re: [PATCH v5 3/5] iio: adc: ad7949: add support for internal vref

[Jonathan Cameron]

...
 > +
> > >  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 fwnode_handle *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,82 @@ 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_optional(dev, "vrefin");
> > >  	if (IS_ERR(ad7949_adc->vref)) {
> > > -		dev_err(dev, "fail to request regulator\n");
> > > -		return PTR_ERR(ad7949_adc->vref);
> > > +		ret = PTR_ERR(ad7949_adc->vref);
> > > +		if (ret != -ENODEV)
> > > +			return ret;
> > > +		/* unbuffered? */
> > > +		ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
> > > +		if (IS_ERR(ad7949_adc->vref)) {
> > > +			ret = PTR_ERR(ad7949_adc->vref);
> > > +			if (ret != -ENODEV)
> > > +				return ret;
> > > +			/* Internal then */
> > > +			mode = AD7949_CFG_VAL_REF_INT_4096;
> > > +		} else {
> > > +			mode = AD7949_CFG_VAL_REF_EXT_TEMP;
> > > +		}
> > > +	} else {
> > > +		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_EXTERNAL) {
> > > +		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 */
> > > +	device_for_each_child_node(dev, child) {
> > > +		ret = fwnode_property_read_u32(child, "reg", &i);
> > > +		if (ret) {
> > > +			dev_err(dev, "missing reg property in %pfw\n", child);
> > > +			fwnode_handle_put(child);
> > > +			return ret;
> > > +		}
> > > +
> > > +		ad7949_chan = &ad7949_adc->channels[i];
> > > +
> > > +		ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
> > > +		if (ret < 0 && ret != -EINVAL) {
> > > +			dev_err(dev, "invalid internal reference in %pfw\n", child);
> > > +			fwnode_handle_put(child);
> > > +			return ret;
> > > +		}  
> 
> Hi Jonathan,
> 
> >
> > So if we are using external voltage, then we'd not expect this to exist.
> > ret != -EINVAL should deal with that. However, we then hit the following
> > switch
> > and temp isn't set to an appropriate value so we get the error.
> >
> > Am I missing something?  
> 
> You're right that using an external reference, will cause this to fail.
> My apologies, I've done a poor job of testing the last two revisions of
> this patch. I'll do better.
> 
> Since a regulator is also required when we're using an external source,
> I'll add a check for that. Something like this:
> 
> 	ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
> 	if (ret == -EINVAL && mode & AD7949_CFG_VAL_REF_EXTERNAL) {
> 		continue;
> 	} else if (ret < 0) {
> 		dev_err(dev, "invalid voltage reference in %pfw\n", child);
> 		fwnode_handle_put(child);
> 		return ret;
> 	}

Makes sense.

> 
> Given that we can now have a different scale for each channel based on
> the vref source, should BIT(IIO_CHAN_INFO_SCALE) be moved to
> .info_mask_separate in the channel definition?
Yes, good point.

Hopefully no one will notice the ABI change *crosses fingers*.

Jonathan

> 
> Thanks,
> Liam
> 
> >  
> > > +
> > > +		switch (tmp) {
> > > +		case 2500000:
> > > +			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_2500;
> > > +			break;
> > > +		case 4096000:
> > > +			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_4096;
> > > +			break;
> > > +		default:
> > > +			dev_err(dev, "unsupported internal voltage reference\n");
> > > +			fwnode_handle_put(child);
> > > +			return -EINVAL;
> > > +		}
> > >  	}
> > >  
> > >  	mutex_init(&ad7949_adc->lock);
> > > @@ -373,7 +479,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 +490,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;
> > >  }  
> 

Re: [PATCH v5 1/5] iio: adc: ad7949: define and use bitfield names

[Joe Perches]

On Sun, 2021-08-08 at 18:46 -0400, Liam Beguin wrote:
> On Sun Aug 8, 2021 at 12:51 PM EDT, Joe Perches wrote:
> > On Sat, 2021-08-07 at 21:56 -0400, Liam Beguin wrote:
> > > Replace raw configuration register values by using FIELD_PREP and
> > > defines to improve readability.
> > []
> > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > []
> > +#define AD7949_CFG_BIT_INCC GENMASK(12, 10)
> > 
> 
> Hi Joe,
> 
> > I think the naming is a bit confusing as it appears as if
> > these bitfield ranges are single bits.
> 
> That makes sense.
> Would AD7949_CFG_BITS_* be good enough?

Sure, or add MASK or something else like AD7949_CFG_MASK_INCC.

It's pretty common to define _MASK and _SHIFT for these types
of uses.

For instance:
include/drm/drm_dp_helper.h-# define DP_DSC_MAJOR_MASK                  (0xf << 0)
include/drm/drm_dp_helper.h-# define DP_DSC_MINOR_MASK                  (0xf << 4)
include/drm/drm_dp_helper.h:# define DP_DSC_MAJOR_SHIFT                 0
include/drm/drm_dp_helper.h:# define DP_DSC_MINOR_SHIFT                 4

Re: [PATCH v5 3/5] iio: adc: ad7949: add support for internal vref

[Andy Shevchenko]

On Mon, Aug 9, 2021 at 1:50 AM Liam Beguin <liambeguin@gmail.com> wrote:
> On Sun Aug 8, 2021 at 12:36 PM EDT, Jonathan Cameron wrote:
> > On Sat, 7 Aug 2021 21:56:57 -0400
>         ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
>         if (ret == -EINVAL && mode & AD7949_CFG_VAL_REF_EXTERNAL) {
>                 continue;

>         } else if (ret < 0) {

Side note, redundant 'else'

>                 dev_err(dev, "invalid voltage reference in %pfw\n", child);
>                 fwnode_handle_put(child);
>                 return ret;
>         }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 3/5] iio: adc: ad7949: add support for internal vref

[Liam Beguin]

On Tue Aug 10, 2021 at 8:15 AM EDT, Andy Shevchenko wrote:
> On Mon, Aug 9, 2021 at 1:50 AM Liam Beguin <liambeguin@gmail.com> wrote:
> > On Sun Aug 8, 2021 at 12:36 PM EDT, Jonathan Cameron wrote:
> > > On Sat, 7 Aug 2021 21:56:57 -0400
> >         ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
> >         if (ret == -EINVAL && mode & AD7949_CFG_VAL_REF_EXTERNAL) {
> >                 continue;
>
> >         } else if (ret < 0) {
>

Hi Andy,

> Side note, redundant 'else'

Are you asking to add an 'else' statement?

because, unless I'm mistaken, in this case ret can have other negative values
that we want to catch with this 'else if'.

Thanks for your time,
Liam

>
> >                 dev_err(dev, "invalid voltage reference in %pfw\n", child);
> >                 fwnode_handle_put(child);
> >                 return ret;
> >         }
>
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v5 3/5] iio: adc: ad7949: add support for internal vref

[Andy Shevchenko]

On Tue, Aug 10, 2021 at 10:46 PM Liam Beguin <liambeguin@gmail.com> wrote:
> On Tue Aug 10, 2021 at 8:15 AM EDT, Andy Shevchenko wrote:
> > On Mon, Aug 9, 2021 at 1:50 AM Liam Beguin <liambeguin@gmail.com> wrote:
> > > On Sun Aug 8, 2021 at 12:36 PM EDT, Jonathan Cameron wrote:
> > > > On Sat, 7 Aug 2021 21:56:57 -0400
> > >         ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
> > >         if (ret == -EINVAL && mode & AD7949_CFG_VAL_REF_EXTERNAL) {
> > >                 continue;
> >
> > >         } else if (ret < 0) {

> > Side note, redundant 'else'
>
> Are you asking to add an 'else' statement?
>
> because, unless I'm mistaken, in this case ret can have other negative values
> that we want to catch with this 'else if'.

You lost me, I have no idea what "to add" and "other" mean here. No, I
asked to remove it. It's redundant.

> > >                 dev_err(dev, "invalid voltage reference in %pfw\n", child);
> > >                 fwnode_handle_put(child);
> > >                 return ret;
> > >         }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 3/5] iio: adc: ad7949: add support for internal vref

[Liam Beguin]

On Tue Aug 10, 2021 at 3:55 PM EDT, Andy Shevchenko wrote:
> On Tue, Aug 10, 2021 at 10:46 PM Liam Beguin <liambeguin@gmail.com>
> wrote:
> > On Tue Aug 10, 2021 at 8:15 AM EDT, Andy Shevchenko wrote:
> > > On Mon, Aug 9, 2021 at 1:50 AM Liam Beguin <liambeguin@gmail.com> wrote:
> > > > On Sun Aug 8, 2021 at 12:36 PM EDT, Jonathan Cameron wrote:
> > > > > On Sat, 7 Aug 2021 21:56:57 -0400
> > > >         ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
> > > >         if (ret == -EINVAL && mode & AD7949_CFG_VAL_REF_EXTERNAL) {
> > > >                 continue;
> > >
> > > >         } else if (ret < 0) {
>
> > > Side note, redundant 'else'
> >
> > Are you asking to add an 'else' statement?
> >
> > because, unless I'm mistaken, in this case ret can have other negative values
> > that we want to catch with this 'else if'.
>
> You lost me, I have no idea what "to add" and "other" mean here. No, I
> asked to remove it. It's redundant.
>

Oh, I see what you meant now. I'll fix it. Thanks!

Liam

> > > >                 dev_err(dev, "invalid voltage reference in %pfw\n", child);
> > > >                 fwnode_handle_put(child);
> > > >                 return ret;
> > > >         }
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v5 3/5] iio: adc: ad7949: add support for internal vref

[Liam Beguin]

On Mon Aug 9, 2021 at 3:42 PM EDT, Jonathan Cameron wrote:
> ...
> > +
> > > >  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 fwnode_handle *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,82 @@ 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_optional(dev, "vrefin");
> > > >  	if (IS_ERR(ad7949_adc->vref)) {
> > > > -		dev_err(dev, "fail to request regulator\n");
> > > > -		return PTR_ERR(ad7949_adc->vref);
> > > > +		ret = PTR_ERR(ad7949_adc->vref);
> > > > +		if (ret != -ENODEV)
> > > > +			return ret;
> > > > +		/* unbuffered? */
> > > > +		ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
> > > > +		if (IS_ERR(ad7949_adc->vref)) {
> > > > +			ret = PTR_ERR(ad7949_adc->vref);
> > > > +			if (ret != -ENODEV)
> > > > +				return ret;
> > > > +			/* Internal then */
> > > > +			mode = AD7949_CFG_VAL_REF_INT_4096;
> > > > +		} else {
> > > > +			mode = AD7949_CFG_VAL_REF_EXT_TEMP;
> > > > +		}
> > > > +	} else {
> > > > +		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_EXTERNAL) {
> > > > +		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 */
> > > > +	device_for_each_child_node(dev, child) {
> > > > +		ret = fwnode_property_read_u32(child, "reg", &i);
> > > > +		if (ret) {
> > > > +			dev_err(dev, "missing reg property in %pfw\n", child);
> > > > +			fwnode_handle_put(child);
> > > > +			return ret;
> > > > +		}
> > > > +
> > > > +		ad7949_chan = &ad7949_adc->channels[i];
> > > > +
> > > > +		ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
> > > > +		if (ret < 0 && ret != -EINVAL) {
> > > > +			dev_err(dev, "invalid internal reference in %pfw\n", child);
> > > > +			fwnode_handle_put(child);
> > > > +			return ret;
> > > > +		}  
> > 
> > Hi Jonathan,
> > 
> > >
> > > So if we are using external voltage, then we'd not expect this to exist.
> > > ret != -EINVAL should deal with that. However, we then hit the following
> > > switch
> > > and temp isn't set to an appropriate value so we get the error.
> > >
> > > Am I missing something?  
> > 
> > You're right that using an external reference, will cause this to fail.
> > My apologies, I've done a poor job of testing the last two revisions of
> > this patch. I'll do better.
> > 
> > Since a regulator is also required when we're using an external source,
> > I'll add a check for that. Something like this:
> > 
> > 	ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
> > 	if (ret == -EINVAL && mode & AD7949_CFG_VAL_REF_EXTERNAL) {
> > 		continue;
> > 	} else if (ret < 0) {
> > 		dev_err(dev, "invalid voltage reference in %pfw\n", child);
> > 		fwnode_handle_put(child);
> > 		return ret;
> > 	}
>
> Makes sense.

Hi Jonathan,

After getting access to another setup to run more tests, I noticed
that setting the reference per channel isn't really feasible.

It take a little while for the reference to be updated internally even
though the request is sent out properly, and reading channels in a
sequence returns bad values.

I'll resend this without the per-channel configuration and make
adi,internal-ref-microvolt a global parameter.

Sorry about that,
Liam

>
> > 
> > Given that we can now have a different scale for each channel based on
> > the vref source, should BIT(IIO_CHAN_INFO_SCALE) be moved to
> > .info_mask_separate in the channel definition?
> Yes, good point.
>
> Hopefully no one will notice the ABI change *crosses fingers*.
>
> Jonathan
>
> > 
> > Thanks,
> > Liam
> > 
> > >  
> > > > +
> > > > +		switch (tmp) {
> > > > +		case 2500000:
> > > > +			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_2500;
> > > > +			break;
> > > > +		case 4096000:
> > > > +			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_4096;
> > > > +			break;
> > > > +		default:
> > > > +			dev_err(dev, "unsupported internal voltage reference\n");
> > > > +			fwnode_handle_put(child);
> > > > +			return -EINVAL;
> > > > +		}
> > > >  	}
> > > >  
> > > >  	mutex_init(&ad7949_adc->lock);
> > > > @@ -373,7 +479,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 +490,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;
> > > >  }  
> > 

Re: [PATCH v5 3/5] iio: adc: ad7949: add support for internal vref

[Andy Shevchenko]

On Fri, Aug 13, 2021 at 11:51 PM Liam Beguin <liambeguin@gmail.com> wrote:
> On Mon Aug 9, 2021 at 3:42 PM EDT, Jonathan Cameron wrote:

...

> > > > > +       /* Read channel specific information form the devicetree */

from

> > > > > +       device_for_each_child_node(dev, child) {
> > > > > +               ret = fwnode_property_read_u32(child, "reg", &i);
> > > > > +               if (ret) {
> > > > > +                       dev_err(dev, "missing reg property in %pfw\n", child);
> > > > > +                       fwnode_handle_put(child);
> > > > > +                       return ret;
> > > > > +               }
> > > > > +
> > > > > +               ad7949_chan = &ad7949_adc->channels[i];
> > > > > +
> > > > > +               ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
> > > > > +               if (ret < 0 && ret != -EINVAL) {
> > > > > +                       dev_err(dev, "invalid internal reference in %pfw\n", child);
> > > > > +                       fwnode_handle_put(child);
> > > > > +                       return ret;
> > > > > +               }

> After getting access to another setup to run more tests, I noticed
> that setting the reference per channel isn't really feasible.

Is it a hardware limitation?

-- 
With Best Regards,
Andy Shevchenko