[PATCH v1 0/4] AD7949 Fixes

[PATCH v1 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.

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

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

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 adi,reference-source

 .../bindings/iio/adc/adi,ad7949.yaml          |  21 ++
 drivers/iio/adc/ad7949.c                      | 201 +++++++++++++++---
 2 files changed, 192 insertions(+), 30 deletions(-)

Range-diff against v1:
-:  ------------ > 1:  b8577e93229f iio: adc: ad7949: define and use bitfield names
1:  86bab3bedcf8 ! 2:  a8468391e3d0 iio: adc: ad7949: fix spi messages on non 14-bit controllers
    @@ 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);
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_c
     +		*val = ad7949_adc->buffer & GENMASK(13, 0);
     +		break;
     +	case 8:
    ++	default:
     +		/* Convert byte array to u16, MSB first */
     +		*val = (ad7949_adc->buf8[0] << 8) | ad7949_adc->buf8[1];
     +		/* Shift-out padding bits */
2:  5f4dbdd51e1f ! 3:  4b0c11c9a748 iio: adc: ad7949: add support for internal vref
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_init(struct ad7949_adc_chip *ad7
      	const struct ad7949_adc_spec *spec;
      	struct ad7949_adc_chip *ad7949_adc;
      	struct iio_dev *indio_dev;
    -@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_probe(struct spi_device *spi)
    - 		return -ENOMEM;
    - 	}
    - 
    -+	indio_dev->dev.of_node = np;
    - 	indio_dev->info = &ad7949_spi_info;
    - 	indio_dev->name = spi_get_device_id(spi)->name;
    - 	indio_dev->modes = INDIO_DIRECT_MODE;
     @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_probe(struct spi_device *spi)
      	ad7949_adc->resolution = spec->resolution;
      	ad7949_set_bits_per_word(ad7949_adc);
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_probe(struct spi_device *spi)
     -	if (IS_ERR(ad7949_adc->vref)) {
     -		dev_err(dev, "fail to request regulator\n");
     -		return PTR_ERR(ad7949_adc->vref);
    --	}
     +	/* Set default devicetree parameters */
     +	ad7949_adc->refsel = AD7949_REF_EXT_BUF;
     +	of_property_read_u32(np, "adi,reference-select", &ad7949_adc->refsel);
    ++	switch (ad7949_adc->refsel) {
    ++	case AD7949_REF_INT_2500:
    ++	case AD7949_REF_INT_4096:
    ++	case AD7949_REF_EXT_TEMP:
    ++	case AD7949_REF_EXT_TEMP_BUF:
    ++	case AD7949_REF_EXT:
    ++	case AD7949_REF_EXT_BUF:
    ++		break;
    ++	default:
    ++		dev_err(dev, "invalid adi,reference-select value (%d)\n",
    ++			ad7949_adc->refsel);
    ++		return -EINVAL;
    + 	}
      
     -	ret = regulator_enable(ad7949_adc->vref);
     -	if (ret < 0) {
3:  99367ba6e5f9 ! 4:  a3b6a6ef15fd dt-bindings: iio: adc: ad7949: add adi,reference-source
    @@ Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml: properties:
          const: 1
      
     +  adi,reference-select:
    -+    allOf:
    -+      - $ref: /schemas/types.yaml#/definitions/uint32
    -+      - enum: [0, 1, 2, 3, 6, 7]
    -+
    -+    default: 7
     +    description: |
     +      Select the reference voltage source to use when converting samples.
     +      Acceptable values are:
    @@ Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml: properties:
     +           disabled.
     +      - 7: Use external reference, internal buffer enabled.
     +           Internal reference and temperature sensor disabled.
    ++
    ++    $ref: /schemas/types.yaml#/definitions/uint32
    ++    enum: [0, 1, 2, 3, 6, 7]
    ++    default: 7
     +
      required:
        - compatible

base-commit: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
-- 
2.30.1.489.g328c10930387

[PATCH v2 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 | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 1b4b3203e428..93aacf4f680b 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -12,12 +12,27 @@
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
 
-#define AD7949_MASK_CHANNEL_SEL		GENMASK(9, 7)
 #define AD7949_MASK_TOTAL		GENMASK(13, 0)
-#define AD7949_OFFSET_CHANNEL_SEL	7
-#define AD7949_CFG_READ_BACK		0x1
 #define AD7949_CFG_REG_SIZE_BITS	14
 
+#define AD7949_CFG_BIT_CFG		BIT(13)
+#define AD7949_CFG_VAL_CFG_OVERWRITE		1
+#define AD7949_CFG_VAL_CFG_KEEP			0
+#define AD7949_CFG_BIT_INCC		GENMASK(12, 10)
+#define AD7949_CFG_VAL_INCC_UNIPOLAR_GND	7
+#define AD7949_CFG_VAL_INCC_UNIPOLAR_COMM	6
+#define AD7949_CFG_VAL_INCC_UNIPOLAR_DIFF	4
+#define AD7949_CFG_VAL_INCC_TEMP		3
+#define AD7949_CFG_VAL_INCC_BIPOLAR		2
+#define AD7949_CFG_VAL_INCC_BIPOLAR_DIFF	0
+#define AD7949_CFG_BIT_INX		GENMASK(9, 7)
+#define AD7949_CFG_BIT_BW		BIT(6)
+#define AD7949_CFG_VAL_BW_FULL			1
+#define AD7949_CFG_VAL_BW_QUARTER		0
+#define AD7949_CFG_BIT_REF		GENMASK(5, 3)
+#define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
+#define AD7949_CFG_BIT_RBN		BIT(0)
+
 enum {
 	ID_AD7949 = 0,
 	ID_AD7682,
@@ -109,8 +124,8 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	 */
 	for (i = 0; i < 2; i++) {
 		ret = ad7949_spi_write_cfg(ad7949_adc,
-					   channel << AD7949_OFFSET_CHANNEL_SEL,
-					   AD7949_MASK_CHANNEL_SEL);
+					   FIELD_PREP(AD7949_CFG_BIT_INX, channel),
+					   AD7949_CFG_BIT_INX);
 		if (ret)
 			return ret;
 		if (channel == ad7949_adc->current_channel)
@@ -214,10 +229,19 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 {
 	int ret;
 	int val;
+	u16 cfg;
 
-	/* Sequencer disabled, CFG readback disabled, IN0 as default channel */
 	ad7949_adc->current_channel = 0;
-	ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
+
+	cfg = FIELD_PREP(AD7949_CFG_BIT_CFG, AD7949_CFG_VAL_CFG_OVERWRITE) |
+		FIELD_PREP(AD7949_CFG_BIT_INCC, AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
+		FIELD_PREP(AD7949_CFG_BIT_INX, ad7949_adc->current_channel) |
+		FIELD_PREP(AD7949_CFG_BIT_BW, AD7949_CFG_VAL_BW_FULL) |
+		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_REF_EXT_BUF) |
+		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
+		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);
+
+	ret = ad7949_spi_write_cfg(ad7949_adc, cfg, AD7949_MASK_TOTAL);
 
 	/*
 	 * Do two dummy conversions to apply the first configuration setting.
-- 
2.30.1.489.g328c10930387

[PATCH v2 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 | 69 ++++++++++++++++++++++++++++++++++------
 1 file changed, 60 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 93aacf4f680b..770112ac820f 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
+#include <linux/bitfield.h>
 
 #define AD7949_MASK_TOTAL		GENMASK(13, 0)
 #define AD7949_CFG_REG_SIZE_BITS	14
@@ -57,6 +58,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
  * @indio_dev: reference to iio structure
  * @spi: reference to spi structure
  * @resolution: resolution of the chip
+ * @bits_per_word: number of bits per SPI word
  * @cfg: copy of the configuration register
  * @current_channel: current channel in use
  * @buffer: buffer to send / receive data to / from device
@@ -67,28 +69,60 @@ struct ad7949_adc_chip {
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
 	u8 resolution;
+	u8 bits_per_word;
 	u16 cfg;
 	unsigned int current_channel;
-	u16 buffer ____cacheline_aligned;
+	union {
+		__be16 buffer;
+		u8 buf8[2];
+	} ____cacheline_aligned;
 };
 
+static void ad7949_set_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
+{
+	u32 adc_mask = SPI_BPW_MASK(ad7949_adc->resolution);
+	u32 bpw = adc_mask & ad7949_adc->spi->controller->bits_per_word_mask;
+
+	if (bpw == adc_mask)
+		ad7949_adc->bits_per_word = ad7949_adc->resolution;
+	else if (bpw == SPI_BPW_MASK(16))
+		ad7949_adc->bits_per_word = 16;
+	else
+		ad7949_adc->bits_per_word = 8;
+}
+
 static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
 				u16 mask)
 {
 	int ret;
-	int bits_per_word = ad7949_adc->resolution;
-	int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
 	struct spi_message msg;
 	struct spi_transfer tx[] = {
 		{
 			.tx_buf = &ad7949_adc->buffer,
 			.len = 2,
-			.bits_per_word = bits_per_word,
+			.bits_per_word = ad7949_adc->bits_per_word,
 		},
 	};
 
+	ad7949_adc->buffer = 0;
 	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
-	ad7949_adc->buffer = ad7949_adc->cfg << shift;
+
+	switch (ad7949_adc->bits_per_word) {
+	case 16:
+		ad7949_adc->buffer = ad7949_adc->cfg << 2;
+		break;
+	case 14:
+		ad7949_adc->buffer = ad7949_adc->cfg;
+		break;
+	case 8:
+	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;
+		break;
+	}
+
 	spi_message_init_with_transfers(&msg, tx, 1);
 	ret = spi_sync(ad7949_adc->spi, &msg);
 
@@ -105,14 +139,12 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 {
 	int ret;
 	int i;
-	int bits_per_word = ad7949_adc->resolution;
-	int mask = GENMASK(ad7949_adc->resolution - 1, 0);
 	struct spi_message msg;
 	struct spi_transfer tx[] = {
 		{
 			.rx_buf = &ad7949_adc->buffer,
 			.len = 2,
-			.bits_per_word = bits_per_word,
+			.bits_per_word = ad7949_adc->bits_per_word,
 		},
 	};
 
@@ -147,7 +179,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 */
+		if (ad7949_adc->resolution == 14)
+			*val = *val >> 2;
+		break;
+	case 14:
+		*val = ad7949_adc->buffer & GENMASK(13, 0);
+		break;
+	case 8:
+	default:
+		/* Convert byte array to u16, MSB first */
+		*val = (ad7949_adc->buf8[0] << 8) | ad7949_adc->buf8[1];
+		/* Shift-out padding bits */
+		if (ad7949_adc->resolution == 14)
+			*val = *val >> 2;
+		break;
+	}
 
 	return 0;
 }
@@ -280,6 +330,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
 	spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
 	indio_dev->num_channels = spec->num_channels;
 	ad7949_adc->resolution = spec->resolution;
+	ad7949_set_bits_per_word(ad7949_adc);
 
 	ad7949_adc->vref = devm_regulator_get(dev, "vref");
 	if (IS_ERR(ad7949_adc->vref)) {
-- 
2.30.1.489.g328c10930387

[PATCH v2 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 | 94 ++++++++++++++++++++++++++++++++++------
 1 file changed, 80 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 770112ac820f..a20fd81a0830 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -31,6 +31,7 @@
 #define AD7949_CFG_VAL_BW_FULL			1
 #define AD7949_CFG_VAL_BW_QUARTER		0
 #define AD7949_CFG_BIT_REF		GENMASK(5, 3)
+#define AD7949_CFG_VAL_REF_EXTERNAL		BIT(1)
 #define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
 #define AD7949_CFG_BIT_RBN		BIT(0)
 
@@ -40,6 +41,33 @@ enum {
 	ID_AD7689,
 };
 
+/**
+ * enum ad7949_ref - Reference selection
+ *
+ * AD7949_REF_INT_2500:     Internal reference and temperature sensor enabled.
+ *                          Vref=2.5V, buffered output
+ * AD7949_REF_INT_4096:     Internal reference and temperature sensor enabled.
+ *                          Vref=4.096V, buffered output
+ * AD7949_REF_EXT_TEMP:     Use external reference, temperature sensor enabled.
+ *                          Internal buffer disabled
+ * AD7949_REF_EXT_TEMP_BUF: Use external reference, internal buffer and
+ *                          temperature sensor enabled.
+ * AD7949_REF_RSRV_4:       Do not use
+ * AD7949_REF_RSRV_5:       Do not use
+ * AD7949_REF_EXT:          Use external reference, internal buffer and
+ *                          temperature sensor disabled.
+ * AD7949_REF_EXT_BUF:      Use external reference, internal buffer enabled.
+ *                          Internal reference and temperature sensor disabled.
+ */
+enum ad7949_ref {
+	AD7949_REF_INT_2500 = 0,
+	AD7949_REF_INT_4096,
+	AD7949_REF_EXT_TEMP,
+	AD7949_REF_EXT_TEMP_BUF,
+	AD7949_REF_EXT = 6,
+	AD7949_REF_EXT_BUF,
+};
+
 struct ad7949_adc_spec {
 	u8 num_channels;
 	u8 resolution;
@@ -55,6 +83,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
  * struct ad7949_adc_chip - AD ADC chip
  * @lock: protects write sequences
  * @vref: regulator generating Vref
+ * @refsel: reference selection
  * @indio_dev: reference to iio structure
  * @spi: reference to spi structure
  * @resolution: resolution of the chip
@@ -66,6 +95,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
 struct ad7949_adc_chip {
 	struct mutex lock;
 	struct regulator *vref;
+	enum ad7949_ref refsel;
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
 	u8 resolution;
@@ -243,12 +273,28 @@ static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_SCALE:
-		ret = regulator_get_voltage(ad7949_adc->vref);
-		if (ret < 0)
-			return ret;
+		switch (ad7949_adc->refsel) {
+		case AD7949_REF_INT_2500:
+			*val = 2500;
+			break;
+		case AD7949_REF_INT_4096:
+			*val = 4096;
+			break;
+		case AD7949_REF_EXT_TEMP:
+		case AD7949_REF_EXT_TEMP_BUF:
+		case AD7949_REF_EXT:
+		case AD7949_REF_EXT_BUF:
+			ret = regulator_get_voltage(ad7949_adc->vref);
+			if (ret < 0)
+				return ret;
+
+			/* convert value back to mV */
+			*val = ret / 1000;
+			break;
+		}
 
-		*val = ret / 5000;
-		return IIO_VAL_INT;
+		*val2 = (1 << ad7949_adc->resolution) - 1;
+		return IIO_VAL_FRACTIONAL;
 	}
 
 	return -EINVAL;
@@ -287,7 +333,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 		FIELD_PREP(AD7949_CFG_BIT_INCC, AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
 		FIELD_PREP(AD7949_CFG_BIT_INX, ad7949_adc->current_channel) |
 		FIELD_PREP(AD7949_CFG_BIT_BW, AD7949_CFG_VAL_BW_FULL) |
-		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_REF_EXT_BUF) |
+		FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_adc->refsel) |
 		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
 		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);
 
@@ -306,6 +352,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 static int ad7949_spi_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
+	struct device_node *np = dev->of_node;
 	const struct ad7949_adc_spec *spec;
 	struct ad7949_adc_chip *ad7949_adc;
 	struct iio_dev *indio_dev;
@@ -332,16 +379,35 @@ static int ad7949_spi_probe(struct spi_device *spi)
 	ad7949_adc->resolution = spec->resolution;
 	ad7949_set_bits_per_word(ad7949_adc);
 
-	ad7949_adc->vref = devm_regulator_get(dev, "vref");
-	if (IS_ERR(ad7949_adc->vref)) {
-		dev_err(dev, "fail to request regulator\n");
-		return PTR_ERR(ad7949_adc->vref);
+	/* Set default devicetree parameters */
+	ad7949_adc->refsel = AD7949_REF_EXT_BUF;
+	of_property_read_u32(np, "adi,reference-select", &ad7949_adc->refsel);
+	switch (ad7949_adc->refsel) {
+	case AD7949_REF_INT_2500:
+	case AD7949_REF_INT_4096:
+	case AD7949_REF_EXT_TEMP:
+	case AD7949_REF_EXT_TEMP_BUF:
+	case AD7949_REF_EXT:
+	case AD7949_REF_EXT_BUF:
+		break;
+	default:
+		dev_err(dev, "invalid adi,reference-select value (%d)\n",
+			ad7949_adc->refsel);
+		return -EINVAL;
 	}
 
-	ret = regulator_enable(ad7949_adc->vref);
-	if (ret < 0) {
-		dev_err(dev, "fail to enable regulator\n");
-		return ret;
+	if (ad7949_adc->refsel & AD7949_CFG_VAL_REF_EXTERNAL) {
+		ad7949_adc->vref = devm_regulator_get(dev, "vref");
+		if (IS_ERR(ad7949_adc->vref)) {
+			dev_err(dev, "fail to request regulator\n");
+			return PTR_ERR(ad7949_adc->vref);
+		}
+
+		ret = regulator_enable(ad7949_adc->vref);
+		if (ret < 0) {
+			dev_err(dev, "fail to enable regulator\n");
+			return ret;
+		}
 	}
 
 	mutex_init(&ad7949_adc->lock);
-- 
2.30.1.489.g328c10930387

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

[Liam Beguin]

From: Liam Beguin <lvb@xiphos.com>

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

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 .../bindings/iio/adc/adi,ad7949.yaml          | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
index 9b56bd4d5510..eae3121cad01 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
@@ -35,6 +35,27 @@ properties:
   "#io-channel-cells":
     const: 1
 
+  adi,reference-select:
+    description: |
+      Select the reference voltage source to use when converting samples.
+      Acceptable values are:
+      - 0: Internal reference and temperature sensor enabled.
+           Vref=2.5V, buffered output
+      - 1: Internal reference and temperature sensor enabled.
+           Vref=4.096V, buffered output
+      - 2: Use external reference, temperature sensor enabled.
+           Internal buffer disabled
+      - 3: Use external reference, internal buffer and temperature sensor
+           enabled.
+      - 6: Use external reference, internal buffer and temperature sensor
+           disabled.
+      - 7: Use external reference, internal buffer enabled.
+           Internal reference and temperature sensor disabled.
+
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 6, 7]
+    default: 7
+
 required:
   - compatible
   - reg
-- 
2.30.1.489.g328c10930387

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

[Jonathan Cameron]

On Fri,  9 Jul 2021 11:58:53 -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>

Ideally fixes should come before any refactors / cleanups like this one.
That reduces the burden if people want to backport them.

In this particular case I'm guessing no one ran into the issues the
following patches deal with so we can just take these in the order
you have here.

Otherwise, good cleanup.  A few minor comments inline, mostly as a result
of some less than ideal name choices on the datasheet.

> ---
>  drivers/iio/adc/ad7949.c | 38 +++++++++++++++++++++++++++++++-------
>  1 file changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index 1b4b3203e428..93aacf4f680b 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -12,12 +12,27 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
>  
> -#define AD7949_MASK_CHANNEL_SEL		GENMASK(9, 7)
>  #define AD7949_MASK_TOTAL		GENMASK(13, 0)
> -#define AD7949_OFFSET_CHANNEL_SEL	7
> -#define AD7949_CFG_READ_BACK		0x1
>  #define AD7949_CFG_REG_SIZE_BITS	14
>  
> +#define AD7949_CFG_BIT_CFG		BIT(13)

Even though that's the name on the datasheet it is silly!

I would have just one define called
AD7949_CFG_VAL_OVERWRITE BIT(13)

It's common to do that for single flags where
FIELD_PREP(AD7949_CFG_VAL_OVERWRITE, 1) for example has an
obvious meaning for the 1.

> +#define AD7949_CFG_VAL_CFG_OVERWRITE		1
> +#define AD7949_CFG_VAL_CFG_KEEP			0
> +#define AD7949_CFG_BIT_INCC		GENMASK(12, 10)
> +#define AD7949_CFG_VAL_INCC_UNIPOLAR_GND	7
> +#define AD7949_CFG_VAL_INCC_UNIPOLAR_COMM	6
> +#define AD7949_CFG_VAL_INCC_UNIPOLAR_DIFF	4
> +#define AD7949_CFG_VAL_INCC_TEMP		3
> +#define AD7949_CFG_VAL_INCC_BIPOLAR		2
> +#define AD7949_CFG_VAL_INCC_BIPOLAR_DIFF	0
> +#define AD7949_CFG_BIT_INX		GENMASK(9, 7)

This is rather non obvious abbreviation. _INx would be clearer
perhaps, but then we'd get someone fixing the camel case...
Given it would be good to match the datasheet, keep the name
but add a comment to say this is the input channel select.

> +#define AD7949_CFG_BIT_BW		BIT(6)

As above, I'd suggest just defining AD7949_CFG_VAL_BW_FULL BIT(6)
then it's either full or not depending on a 0 or 1 write.

> +#define AD7949_CFG_VAL_BW_FULL			1
> +#define AD7949_CFG_VAL_BW_QUARTER		0
> +#define AD7949_CFG_BIT_REF		GENMASK(5, 3)
> +#define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
> +#define AD7949_CFG_BIT_RBN		BIT(0)
> +
>  enum {
>  	ID_AD7949 = 0,
>  	ID_AD7682,
> @@ -109,8 +124,8 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>  	 */
>  	for (i = 0; i < 2; i++) {
>  		ret = ad7949_spi_write_cfg(ad7949_adc,
> -					   channel << AD7949_OFFSET_CHANNEL_SEL,
> -					   AD7949_MASK_CHANNEL_SEL);
> +					   FIELD_PREP(AD7949_CFG_BIT_INX, channel),
> +					   AD7949_CFG_BIT_INX);
>  		if (ret)
>  			return ret;
>  		if (channel == ad7949_adc->current_channel)
> @@ -214,10 +229,19 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
>  {
>  	int ret;
>  	int val;
> +	u16 cfg;
>  
> -	/* Sequencer disabled, CFG readback disabled, IN0 as default channel */
>  	ad7949_adc->current_channel = 0;
> -	ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
> +
> +	cfg = FIELD_PREP(AD7949_CFG_BIT_CFG, AD7949_CFG_VAL_CFG_OVERWRITE) |
> +		FIELD_PREP(AD7949_CFG_BIT_INCC, AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
> +		FIELD_PREP(AD7949_CFG_BIT_INX, ad7949_adc->current_channel) |
> +		FIELD_PREP(AD7949_CFG_BIT_BW, AD7949_CFG_VAL_BW_FULL) |
> +		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_REF_EXT_BUF) |
> +		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
> +		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);
> +
> +	ret = ad7949_spi_write_cfg(ad7949_adc, cfg, AD7949_MASK_TOTAL);
>  
>  	/*
>  	 * Do two dummy conversions to apply the first configuration setting.

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

[Jonathan Cameron]

On Fri,  9 Jul 2021 11:58:54 -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.

Hmm. I assumed spi controllers would all support 16 bits, but a quick
grep confirms otherwise...  ah well!

> 
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
>  drivers/iio/adc/ad7949.c | 69 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 60 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index 93aacf4f680b..770112ac820f 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -11,6 +11,7 @@
>  #include <linux/module.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
> +#include <linux/bitfield.h>
>  
>  #define AD7949_MASK_TOTAL		GENMASK(13, 0)
>  #define AD7949_CFG_REG_SIZE_BITS	14
> @@ -57,6 +58,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
>   * @indio_dev: reference to iio structure
>   * @spi: reference to spi structure
>   * @resolution: resolution of the chip
> + * @bits_per_word: number of bits per SPI word
>   * @cfg: copy of the configuration register
>   * @current_channel: current channel in use
>   * @buffer: buffer to send / receive data to / from device
> @@ -67,28 +69,60 @@ struct ad7949_adc_chip {
>  	struct iio_dev *indio_dev;
>  	struct spi_device *spi;
>  	u8 resolution;
> +	u8 bits_per_word;
>  	u16 cfg;
>  	unsigned int current_channel;
> -	u16 buffer ____cacheline_aligned;
> +	union {
> +		__be16 buffer;
> +		u8 buf8[2];
> +	} ____cacheline_aligned;

This union made me a little nervous given issues with them under c spec, but
seems that you only use buffer or buf8, there is no mixing. I think I'd
rather you didn't have the union though as it makes it look like there might be.

Given we are doing cacheline spacing anyway for dma safety it take no more
real space to just have

__be16 buffer ____cacheline_aligned;
buf8[2];



>  };
>  
> +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)

if (bpw) is sufficient I think.

> +		ad7949_adc->bits_per_word = ad7949_adc->resolution;
> +	else if (bpw == SPI_BPW_MASK(16))

I'm missing something here...  If the resolution is 14, but the SPI controller
supports 8 and 16 I'd expect us to hit here .  adc_mask will not have the 16bit
bit set though so bpw == 0..

else if (ad7949_adc->spi->controller_bits_per_word_mask & SPI_BPW_MASK(16))

should do the right thing.

> +		ad7949_adc->bits_per_word = 16;
> +	else
> +		ad7949_adc->bits_per_word = 8;

I'd like a comment here on why this is the right default.  Took me a while
to think through it.

> +}
> +
>  static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
>  				u16 mask)
>  {
>  	int ret;
> -	int bits_per_word = ad7949_adc->resolution;
> -	int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
>  	struct spi_message msg;
>  	struct spi_transfer tx[] = {
>  		{
>  			.tx_buf = &ad7949_adc->buffer,
>  			.len = 2,
> -			.bits_per_word = bits_per_word,
> +			.bits_per_word = ad7949_adc->bits_per_word,
>  		},
>  	};
>  
> +	ad7949_adc->buffer = 0;
>  	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
> -	ad7949_adc->buffer = ad7949_adc->cfg << shift;
> +
> +	switch (ad7949_adc->bits_per_word) {
> +	case 16:
> +		ad7949_adc->buffer = ad7949_adc->cfg << 2;
> +		break;
> +	case 14:
> +		ad7949_adc->buffer = ad7949_adc->cfg;
> +		break;
> +	case 8:
> +	default:

Default here should be an error. Imagine a 12 bit device is added.  Then
the above set_bits_per_word() might return either 12 or 8.  If it returns
12 then this is not the right thing to do. I'd argue we want to know about
that rather than papering over it.  Possibly we want to not let anything other
than 14 bits through in set_bits_per_word() so we print an error early...

> +		/* Pack 14-bit value into 2 bytes, MSB first */
> +		ad7949_adc->buf8[0] = FIELD_GET(GENMASK(13, 6), ad7949_adc->cfg);
> +		ad7949_adc->buf8[1] = FIELD_GET(GENMASK(5, 0), ad7949_adc->cfg);
> +		ad7949_adc->buf8[1] = ad7949_adc->buf8[1] << 2;
> +		break;
> +	}
> +
>  	spi_message_init_with_transfers(&msg, tx, 1);
>  	ret = spi_sync(ad7949_adc->spi, &msg);
>  
> @@ -105,14 +139,12 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>  {
>  	int ret;
>  	int i;
> -	int bits_per_word = ad7949_adc->resolution;
> -	int mask = GENMASK(ad7949_adc->resolution - 1, 0);
>  	struct spi_message msg;
>  	struct spi_transfer tx[] = {
>  		{
>  			.rx_buf = &ad7949_adc->buffer,
>  			.len = 2,
> -			.bits_per_word = bits_per_word,
> +			.bits_per_word = ad7949_adc->bits_per_word,
>  		},
>  	};
>  
> @@ -147,7 +179,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;

buffer is __be16 and *val is an int so this isn't valid.

Now curious bit is I'm assuming original code worked... Given big endian
platforms are rare, that makes me suspect something odd is going on.
 
> +		/* Shift-out padding bits */
> +		if (ad7949_adc->resolution == 14)
> +			*val = *val >> 2;

		*val >>= 16 - ad7949_adc->resolution;

No one likes conditionals if we don't need them!


> +		break;
> +	case 14:
> +		*val = ad7949_adc->buffer & GENMASK(13, 0);
> +		break;
> +	case 8:
> +	default:
> +		/* Convert byte array to u16, MSB first */
> +		*val = (ad7949_adc->buf8[0] << 8) | ad7949_adc->buf8[1];

Use unaligned_get_be16()

> +		/* Shift-out padding bits */
> +		if (ad7949_adc->resolution == 14)
> +			*val = *val >> 2;

As above make this unconditional.

> +		break;
> +	}
>  
>  	return 0;
>  }
> @@ -280,6 +330,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
>  	spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
>  	indio_dev->num_channels = spec->num_channels;
>  	ad7949_adc->resolution = spec->resolution;
> +	ad7949_set_bits_per_word(ad7949_adc);
>  
>  	ad7949_adc->vref = devm_regulator_get(dev, "vref");
>  	if (IS_ERR(ad7949_adc->vref)) {

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

[Jonathan Cameron]

On Fri,  9 Jul 2021 11:58:56 -0400
Liam Beguin <liambeguin@gmail.com> wrote:

> From: Liam Beguin <lvb@xiphos.com>
> 
> Add bindings documentation for the adi,reference-source property.
> This property is required to properly configure the ADC sample request
> based on which reference source should be used for the calculation.

Should this be per channel? That will effect some of what I say below...

> 
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
>  .../bindings/iio/adc/adi,ad7949.yaml          | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> index 9b56bd4d5510..eae3121cad01 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> @@ -35,6 +35,27 @@ properties:
>    "#io-channel-cells":
>      const: 1
>  
> +  adi,reference-select:

This is one field in the register, but it's not one thing, so lets break it up
in DT.  We should do this both to make for more readable dts files and to
enforce the requirements on regulators...

> +    description: |
> +      Select the reference voltage source to use when converting samples.
> +      Acceptable values are:
> +      - 0: Internal reference and temperature sensor enabled.
> +           Vref=2.5V, buffered output
> +      - 1: Internal reference and temperature sensor enabled.
> +           Vref=4.096V, buffered output
> +      - 2: Use external reference, temperature sensor enabled.
> +           Internal buffer disabled
> +      - 3: Use external reference, internal buffer and temperature sensor
> +           enabled.
> +      - 6: Use external reference, internal buffer and temperature sensor
> +           disabled.
> +      - 7: Use external reference, internal buffer enabled.
> +           Internal reference and temperature sensor disabled.

So question 1 is whether to use an external or internal reference.
Normally we'd make the coarse decision of whether to use an external reference
by whether there is a regulator provided.  That won't work so well if we make
this per channel.

Question 2, assuming internal reference, what voltage?  Those should take
an actual voltage (probably in mV and match against an enum of the two possible values).
Binding should check to make sure this isn't specified as well as saying we
are using an external refernce.

Question 3, assuming external reference, is temperature sensor enabled?
- actually dumb question, but why would anyone not want this enabled?  Maybe turn it
off in runtime pm, but in general if you've fitted a chip with a temperature sensor
you at least sometimes want to measure temperature!  So my gut feeling is don't
allow this to be controlled (effectively drop cases 6 and 7 above as being
unlikely to be of interest to anyone)

Question 4, Is the internal buffer enabled when using and external reference.
This one is interesting.   We could just expose it in general, but I wonder
if we can do something that reflects how it is used.  From the various figures in
the datasheet this seems to be coupled to whether the external reference is on
pin REF_IN or pin REF.  If that's the case can we have two optional regs only
one of which should be supplied?  However, this gets more fiddly because
the default right now is vref-supply actually being connected to the vrefin connection.
That's annoying as it stops us using the obvious naming...
Hence I think we can have
vref-supply (actually connected to vrefin) and vref-unbuffered-supply



> +
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2, 3, 6, 7]
> +    default: 7
> +
>  required:
>    - compatible
>    - reg

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

[Jonathan Cameron]

On Fri,  9 Jul 2021 11:58:55 -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>
> ---
>  drivers/iio/adc/ad7949.c | 94 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 80 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index 770112ac820f..a20fd81a0830 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -31,6 +31,7 @@
>  #define AD7949_CFG_VAL_BW_FULL			1
>  #define AD7949_CFG_VAL_BW_QUARTER		0
>  #define AD7949_CFG_BIT_REF		GENMASK(5, 3)
> +#define AD7949_CFG_VAL_REF_EXTERNAL		BIT(1)
>  #define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
>  #define AD7949_CFG_BIT_RBN		BIT(0)
>  
> @@ -40,6 +41,33 @@ enum {
>  	ID_AD7689,
>  };
>  
> +/**
> + * enum ad7949_ref - Reference selection
> + *
> + * AD7949_REF_INT_2500:     Internal reference and temperature sensor enabled.
> + *                          Vref=2.5V, buffered output
> + * AD7949_REF_INT_4096:     Internal reference and temperature sensor enabled.
> + *                          Vref=4.096V, buffered output
> + * AD7949_REF_EXT_TEMP:     Use external reference, temperature sensor enabled.
> + *                          Internal buffer disabled
> + * AD7949_REF_EXT_TEMP_BUF: Use external reference, internal buffer and
> + *                          temperature sensor enabled.
> + * AD7949_REF_RSRV_4:       Do not use
> + * AD7949_REF_RSRV_5:       Do not use
> + * AD7949_REF_EXT:          Use external reference, internal buffer and
> + *                          temperature sensor disabled.
> + * AD7949_REF_EXT_BUF:      Use external reference, internal buffer enabled.
> + *                          Internal reference and temperature sensor disabled.
> + */
> +enum ad7949_ref {
> +	AD7949_REF_INT_2500 = 0,

For case where you have gaps like this, an ENUM is probably not appropriate.
I'd just used defines.

> +	AD7949_REF_INT_4096,
> +	AD7949_REF_EXT_TEMP,
> +	AD7949_REF_EXT_TEMP_BUF,
> +	AD7949_REF_EXT = 6,
> +	AD7949_REF_EXT_BUF,
> +};
> +
>  struct ad7949_adc_spec {
>  	u8 num_channels;
>  	u8 resolution;
> @@ -55,6 +83,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
>   * struct ad7949_adc_chip - AD ADC chip
>   * @lock: protects write sequences
>   * @vref: regulator generating Vref
> + * @refsel: reference selection
>   * @indio_dev: reference to iio structure
>   * @spi: reference to spi structure
>   * @resolution: resolution of the chip
> @@ -66,6 +95,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
>  struct ad7949_adc_chip {
>  	struct mutex lock;
>  	struct regulator *vref;
> +	enum ad7949_ref refsel;
>  	struct iio_dev *indio_dev;
>  	struct spi_device *spi;
>  	u8 resolution;
> @@ -243,12 +273,28 @@ static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT;
>  
>  	case IIO_CHAN_INFO_SCALE:
> -		ret = regulator_get_voltage(ad7949_adc->vref);
> -		if (ret < 0)
> -			return ret;
> +		switch (ad7949_adc->refsel) {
> +		case AD7949_REF_INT_2500:
> +			*val = 2500;
> +			break;
> +		case AD7949_REF_INT_4096:
> +			*val = 4096;
> +			break;
> +		case AD7949_REF_EXT_TEMP:
> +		case AD7949_REF_EXT_TEMP_BUF:
> +		case AD7949_REF_EXT:
> +		case AD7949_REF_EXT_BUF:
> +			ret = regulator_get_voltage(ad7949_adc->vref);
> +			if (ret < 0)
> +				return ret;
> +
> +			/* convert value back to mV */
> +			*val = ret / 1000;
> +			break;
> +		}
>  
> -		*val = ret / 5000;
> -		return IIO_VAL_INT;
> +		*val2 = (1 << ad7949_adc->resolution) - 1;
> +		return IIO_VAL_FRACTIONAL;
>  	}
>  
>  	return -EINVAL;
> @@ -287,7 +333,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
>  		FIELD_PREP(AD7949_CFG_BIT_INCC, AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
>  		FIELD_PREP(AD7949_CFG_BIT_INX, ad7949_adc->current_channel) |
>  		FIELD_PREP(AD7949_CFG_BIT_BW, AD7949_CFG_VAL_BW_FULL) |
> -		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_REF_EXT_BUF) |
> +		FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_adc->refsel) |
>  		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
>  		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);
>  
> @@ -306,6 +352,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
>  static int ad7949_spi_probe(struct spi_device *spi)
>  {
>  	struct device *dev = &spi->dev;
> +	struct device_node *np = dev->of_node;
>  	const struct ad7949_adc_spec *spec;
>  	struct ad7949_adc_chip *ad7949_adc;
>  	struct iio_dev *indio_dev;
> @@ -332,16 +379,35 @@ static int ad7949_spi_probe(struct spi_device *spi)
>  	ad7949_adc->resolution = spec->resolution;
>  	ad7949_set_bits_per_word(ad7949_adc);
>  
> -	ad7949_adc->vref = devm_regulator_get(dev, "vref");
> -	if (IS_ERR(ad7949_adc->vref)) {
> -		dev_err(dev, "fail to request regulator\n");
> -		return PTR_ERR(ad7949_adc->vref);
> +	/* Set default devicetree parameters */
> +	ad7949_adc->refsel = AD7949_REF_EXT_BUF;
> +	of_property_read_u32(np, "adi,reference-select", &ad7949_adc->refsel);

Check for errors.  Not being present is fine, but other errors may not be.


> +	switch (ad7949_adc->refsel) {
> +	case AD7949_REF_INT_2500:
> +	case AD7949_REF_INT_4096:
> +	case AD7949_REF_EXT_TEMP:
> +	case AD7949_REF_EXT_TEMP_BUF:
> +	case AD7949_REF_EXT:
> +	case AD7949_REF_EXT_BUF:
> +		break;
> +	default:
> +		dev_err(dev, "invalid adi,reference-select value (%d)\n",
> +			ad7949_adc->refsel);
> +		return -EINVAL;
>  	}
>  
> -	ret = regulator_enable(ad7949_adc->vref);
> -	if (ret < 0) {
> -		dev_err(dev, "fail to enable regulator\n");
> -		return ret;
> +	if (ad7949_adc->refsel & AD7949_CFG_VAL_REF_EXTERNAL) {
> +		ad7949_adc->vref = devm_regulator_get(dev, "vref");
> +		if (IS_ERR(ad7949_adc->vref)) {
> +			dev_err(dev, "fail to request regulator\n");
> +			return PTR_ERR(ad7949_adc->vref);
> +		}
> +
> +		ret = regulator_enable(ad7949_adc->vref);

Shouldn't be calling regulator_disable unless you enabled it...

> +		if (ret < 0) {
> +			dev_err(dev, "fail to enable regulator\n");
> +			return ret;
> +		}
>  	}
>  
>  	mutex_init(&ad7949_adc->lock);

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

[Liam Beguin]

On Sat Jul 10, 2021 at 12:08 PM EDT, Jonathan Cameron wrote:
> On Fri, 9 Jul 2021 11:58:53 -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>

Hi Jonathan,

>
> Ideally fixes should come before any refactors / cleanups like this one.
> That reduces the burden if people want to backport them.
>
> In this particular case I'm guessing no one ran into the issues the
> following patches deal with so we can just take these in the order
> you have here.
>

Understood, I will follow that guideline next time.

> Otherwise, good cleanup. A few minor comments inline, mostly as a result
> of some less than ideal name choices on the datasheet.
>
> > ---
> >  drivers/iio/adc/ad7949.c | 38 +++++++++++++++++++++++++++++++-------
> >  1 file changed, 31 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > index 1b4b3203e428..93aacf4f680b 100644
> > --- a/drivers/iio/adc/ad7949.c
> > +++ b/drivers/iio/adc/ad7949.c
> > @@ -12,12 +12,27 @@
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/spi/spi.h>
> >  
> > -#define AD7949_MASK_CHANNEL_SEL		GENMASK(9, 7)
> >  #define AD7949_MASK_TOTAL		GENMASK(13, 0)
> > -#define AD7949_OFFSET_CHANNEL_SEL	7
> > -#define AD7949_CFG_READ_BACK		0x1
> >  #define AD7949_CFG_REG_SIZE_BITS	14
> >  
> > +#define AD7949_CFG_BIT_CFG		BIT(13)
>
> Even though that's the name on the datasheet it is silly!

Agreed, datasheet register and bitfield names aren't always great :-/

>
> I would have just one define called
> AD7949_CFG_VAL_OVERWRITE BIT(13)
>
> It's common to do that for single flags where
> FIELD_PREP(AD7949_CFG_VAL_OVERWRITE, 1) for example has an
> obvious meaning for the 1.
>

Sounds good, I'll fix these with your recommendation.

> > +#define AD7949_CFG_VAL_CFG_OVERWRITE		1
> > +#define AD7949_CFG_VAL_CFG_KEEP			0
> > +#define AD7949_CFG_BIT_INCC		GENMASK(12, 10)
> > +#define AD7949_CFG_VAL_INCC_UNIPOLAR_GND	7
> > +#define AD7949_CFG_VAL_INCC_UNIPOLAR_COMM	6
> > +#define AD7949_CFG_VAL_INCC_UNIPOLAR_DIFF	4
> > +#define AD7949_CFG_VAL_INCC_TEMP		3
> > +#define AD7949_CFG_VAL_INCC_BIPOLAR		2
> > +#define AD7949_CFG_VAL_INCC_BIPOLAR_DIFF	0
> > +#define AD7949_CFG_BIT_INX		GENMASK(9, 7)
>
> This is rather non obvious abbreviation. _INx would be clearer
> perhaps, but then we'd get someone fixing the camel case...
> Given it would be good to match the datasheet, keep the name
> but add a comment to say this is the input channel select.
>

I agree! While I'm at it, I might as well add comments for INCC and
others that aren't so abvious.

Thanks,
Liam

> > +#define AD7949_CFG_BIT_BW		BIT(6)
>
> As above, I'd suggest just defining AD7949_CFG_VAL_BW_FULL BIT(6)
> then it's either full or not depending on a 0 or 1 write.
>
> > +#define AD7949_CFG_VAL_BW_FULL			1
> > +#define AD7949_CFG_VAL_BW_QUARTER		0
> > +#define AD7949_CFG_BIT_REF		GENMASK(5, 3)
> > +#define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
> > +#define AD7949_CFG_BIT_RBN		BIT(0)
> > +
> >  enum {
> >  	ID_AD7949 = 0,
> >  	ID_AD7682,
> > @@ -109,8 +124,8 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> >  	 */
> >  	for (i = 0; i < 2; i++) {
> >  		ret = ad7949_spi_write_cfg(ad7949_adc,
> > -					   channel << AD7949_OFFSET_CHANNEL_SEL,
> > -					   AD7949_MASK_CHANNEL_SEL);
> > +					   FIELD_PREP(AD7949_CFG_BIT_INX, channel),
> > +					   AD7949_CFG_BIT_INX);
> >  		if (ret)
> >  			return ret;
> >  		if (channel == ad7949_adc->current_channel)
> > @@ -214,10 +229,19 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
> >  {
> >  	int ret;
> >  	int val;
> > +	u16 cfg;
> >  
> > -	/* Sequencer disabled, CFG readback disabled, IN0 as default channel */
> >  	ad7949_adc->current_channel = 0;
> > -	ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
> > +
> > +	cfg = FIELD_PREP(AD7949_CFG_BIT_CFG, AD7949_CFG_VAL_CFG_OVERWRITE) |
> > +		FIELD_PREP(AD7949_CFG_BIT_INCC, AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
> > +		FIELD_PREP(AD7949_CFG_BIT_INX, ad7949_adc->current_channel) |
> > +		FIELD_PREP(AD7949_CFG_BIT_BW, AD7949_CFG_VAL_BW_FULL) |
> > +		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_REF_EXT_BUF) |
> > +		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
> > +		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);
> > +
> > +	ret = ad7949_spi_write_cfg(ad7949_adc, cfg, AD7949_MASK_TOTAL);
> >  
> >  	/*
> >  	 * Do two dummy conversions to apply the first configuration setting.

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

[Liam Beguin]

On Sat Jul 10, 2021 at 12:39 PM EDT, Jonathan Cameron wrote:
> On Fri, 9 Jul 2021 11:58:54 -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.
>
> Hmm. I assumed spi controllers would all support 16 bits, but a quick
> grep confirms otherwise... ah well!
>
> > 
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > ---
> >  drivers/iio/adc/ad7949.c | 69 ++++++++++++++++++++++++++++++++++------
> >  1 file changed, 60 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > index 93aacf4f680b..770112ac820f 100644
> > --- a/drivers/iio/adc/ad7949.c
> > +++ b/drivers/iio/adc/ad7949.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/module.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/spi/spi.h>
> > +#include <linux/bitfield.h>
> >  
> >  #define AD7949_MASK_TOTAL		GENMASK(13, 0)
> >  #define AD7949_CFG_REG_SIZE_BITS	14
> > @@ -57,6 +58,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
> >   * @indio_dev: reference to iio structure
> >   * @spi: reference to spi structure
> >   * @resolution: resolution of the chip
> > + * @bits_per_word: number of bits per SPI word
> >   * @cfg: copy of the configuration register
> >   * @current_channel: current channel in use
> >   * @buffer: buffer to send / receive data to / from device
> > @@ -67,28 +69,60 @@ struct ad7949_adc_chip {
> >  	struct iio_dev *indio_dev;
> >  	struct spi_device *spi;
> >  	u8 resolution;
> > +	u8 bits_per_word;
> >  	u16 cfg;
> >  	unsigned int current_channel;
> > -	u16 buffer ____cacheline_aligned;
> > +	union {
> > +		__be16 buffer;
> > +		u8 buf8[2];
> > +	} ____cacheline_aligned;

Hi Jonathan,

>
> This union made me a little nervous given issues with them under c spec,
> but
> seems that you only use buffer or buf8, there is no mixing. I think I'd
> rather you didn't have the union though as it makes it look like there
> might be.

I don't particularly like unions either..
If I'm not mistaken, the only place where the union is useful is
in the spi_transfer struct definitions. I'll use a local buf8 there
instead.

>
> Given we are doing cacheline spacing anyway for dma safety it take no
> more
> real space to just have
>
> __be16 buffer ____cacheline_aligned;
> buf8[2];
>
>
>
> >  };
> >  
> > +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)
>
> if (bpw) is sufficient I think.
>
> > +		ad7949_adc->bits_per_word = ad7949_adc->resolution;
> > +	else if (bpw == SPI_BPW_MASK(16))
>
> I'm missing something here... If the resolution is 14, but the SPI
> controller
> supports 8 and 16 I'd expect us to hit here . adc_mask will not have the
> 16bit
> bit set though so bpw == 0..
>
> else if (ad7949_adc->spi->controller_bits_per_word_mask &
> SPI_BPW_MASK(16))
>
> should do the right thing.
>
> > +		ad7949_adc->bits_per_word = 16;
> > +	else
> > +		ad7949_adc->bits_per_word = 8;
>
> I'd like a comment here on why this is the right default. Took me a
> while
> to think through it.
>

I'll rework this function based on your comments and fail early on
unsupported cases.

> > +}
> > +
> >  static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> >  				u16 mask)
> >  {
> >  	int ret;
> > -	int bits_per_word = ad7949_adc->resolution;
> > -	int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
> >  	struct spi_message msg;
> >  	struct spi_transfer tx[] = {
> >  		{
> >  			.tx_buf = &ad7949_adc->buffer,
> >  			.len = 2,
> > -			.bits_per_word = bits_per_word,
> > +			.bits_per_word = ad7949_adc->bits_per_word,
> >  		},
> >  	};
> >  
> > +	ad7949_adc->buffer = 0;
> >  	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
> > -	ad7949_adc->buffer = ad7949_adc->cfg << shift;
> > +
> > +	switch (ad7949_adc->bits_per_word) {
> > +	case 16:
> > +		ad7949_adc->buffer = ad7949_adc->cfg << 2;
> > +		break;
> > +	case 14:
> > +		ad7949_adc->buffer = ad7949_adc->cfg;
> > +		break;
> > +	case 8:
> > +	default:
>
> Default here should be an error. Imagine a 12 bit device is added. Then
> the above set_bits_per_word() might return either 12 or 8. If it returns
> 12 then this is not the right thing to do. I'd argue we want to know
> about
> that rather than papering over it. Possibly we want to not let anything
> other
> than 14 bits through in set_bits_per_word() so we print an error
> early...
>

Sorry about that, I added that default case maybe a little too quickly.
I'll fix that.

> > +		/* Pack 14-bit value into 2 bytes, MSB first */
> > +		ad7949_adc->buf8[0] = FIELD_GET(GENMASK(13, 6), ad7949_adc->cfg);
> > +		ad7949_adc->buf8[1] = FIELD_GET(GENMASK(5, 0), ad7949_adc->cfg);
> > +		ad7949_adc->buf8[1] = ad7949_adc->buf8[1] << 2;
> > +		break;
> > +	}
> > +
> >  	spi_message_init_with_transfers(&msg, tx, 1);
> >  	ret = spi_sync(ad7949_adc->spi, &msg);
> >  
> > @@ -105,14 +139,12 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> >  {
> >  	int ret;
> >  	int i;
> > -	int bits_per_word = ad7949_adc->resolution;
> > -	int mask = GENMASK(ad7949_adc->resolution - 1, 0);
> >  	struct spi_message msg;
> >  	struct spi_transfer tx[] = {
> >  		{
> >  			.rx_buf = &ad7949_adc->buffer,
> >  			.len = 2,
> > -			.bits_per_word = bits_per_word,
> > +			.bits_per_word = ad7949_adc->bits_per_word,
> >  		},
> >  	};
> >  
> > @@ -147,7 +179,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;
>
> buffer is __be16 and *val is an int so this isn't valid.
>
> Now curious bit is I'm assuming original code worked... Given big endian
> platforms are rare, that makes me suspect something odd is going on.
>  

Hum... that's probably my fault. I changed the type of buffer from u16
to _be16.
I'll add calls to be16_to_cpu() around this and the 14 bit case.

> > +		/* Shift-out padding bits */
> > +		if (ad7949_adc->resolution == 14)
> > +			*val = *val >> 2;
>
> *val >>= 16 - ad7949_adc->resolution;
>
> No one likes conditionals if we don't need them!

Will do.

>
>
> > +		break;
> > +	case 14:
> > +		*val = ad7949_adc->buffer & GENMASK(13, 0);
> > +		break;
> > +	case 8:
> > +	default:
> > +		/* Convert byte array to u16, MSB first */
> > +		*val = (ad7949_adc->buf8[0] << 8) | ad7949_adc->buf8[1];
>
> Use unaligned_get_be16()
>
> > +		/* Shift-out padding bits */
> > +		if (ad7949_adc->resolution == 14)
> > +			*val = *val >> 2;
>
> As above make this unconditional.
>

Thanks,
Liam

> > +		break;
> > +	}
> >  
> >  	return 0;
> >  }
> > @@ -280,6 +330,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
> >  	spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
> >  	indio_dev->num_channels = spec->num_channels;
> >  	ad7949_adc->resolution = spec->resolution;
> > +	ad7949_set_bits_per_word(ad7949_adc);
> >  
> >  	ad7949_adc->vref = devm_regulator_get(dev, "vref");
> >  	if (IS_ERR(ad7949_adc->vref)) {

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

[Jonathan Cameron]

On Sun, 11 Jul 2021 15:58:27 -0400
"Liam Beguin" <liambeguin@gmail.com> wrote:

> On Sat Jul 10, 2021 at 12:39 PM EDT, Jonathan Cameron wrote:
> > On Fri, 9 Jul 2021 11:58:54 -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.  
> >
> > Hmm. I assumed spi controllers would all support 16 bits, but a quick
> > grep confirms otherwise... ah well!
> >  
> > > 
> > > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > > ---
> > >  drivers/iio/adc/ad7949.c | 69 ++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 60 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > > index 93aacf4f680b..770112ac820f 100644
> > > --- a/drivers/iio/adc/ad7949.c
> > > +++ b/drivers/iio/adc/ad7949.c
> > > @@ -11,6 +11,7 @@
> > >  #include <linux/module.h>
> > >  #include <linux/regulator/consumer.h>
> > >  #include <linux/spi/spi.h>
> > > +#include <linux/bitfield.h>
> > >  
> > >  #define AD7949_MASK_TOTAL		GENMASK(13, 0)
> > >  #define AD7949_CFG_REG_SIZE_BITS	14
> > > @@ -57,6 +58,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
> > >   * @indio_dev: reference to iio structure
> > >   * @spi: reference to spi structure
> > >   * @resolution: resolution of the chip
> > > + * @bits_per_word: number of bits per SPI word
> > >   * @cfg: copy of the configuration register
> > >   * @current_channel: current channel in use
> > >   * @buffer: buffer to send / receive data to / from device
> > > @@ -67,28 +69,60 @@ struct ad7949_adc_chip {
> > >  	struct iio_dev *indio_dev;
> > >  	struct spi_device *spi;
> > >  	u8 resolution;
> > > +	u8 bits_per_word;
> > >  	u16 cfg;
> > >  	unsigned int current_channel;
> > > -	u16 buffer ____cacheline_aligned;
> > > +	union {
> > > +		__be16 buffer;
> > > +		u8 buf8[2];
> > > +	} ____cacheline_aligned;  
> 
> Hi Jonathan,
> 
> >
> > This union made me a little nervous given issues with them under c spec,
> > but
> > seems that you only use buffer or buf8, there is no mixing. I think I'd
> > rather you didn't have the union though as it makes it look like there
> > might be.  
> 
> I don't particularly like unions either..
> If I'm not mistaken, the only place where the union is useful is
> in the spi_transfer struct definitions. I'll use a local buf8 there
> instead.

Make sure it's either DMA safe, or that we can be sure it doesn't need
to be for some reason.

> 
> >
> > Given we are doing cacheline spacing anyway for dma safety it take no
> > more
> > real space to just have
> >
> > __be16 buffer ____cacheline_aligned;
> > buf8[2];
> >
> >
> >  
> > >  };
> > >  
> > > +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)  
> >
> > if (bpw) is sufficient I think.
> >  
> > > +		ad7949_adc->bits_per_word = ad7949_adc->resolution;
> > > +	else if (bpw == SPI_BPW_MASK(16))  
> >
> > I'm missing something here... If the resolution is 14, but the SPI
> > controller
> > supports 8 and 16 I'd expect us to hit here . adc_mask will not have the
> > 16bit
> > bit set though so bpw == 0..
> >
> > else if (ad7949_adc->spi->controller_bits_per_word_mask &
> > SPI_BPW_MASK(16))
> >
> > should do the right thing.
> >  
> > > +		ad7949_adc->bits_per_word = 16;
> > > +	else
> > > +		ad7949_adc->bits_per_word = 8;  
> >
> > I'd like a comment here on why this is the right default. Took me a
> > while
> > to think through it.
> >  
> 
> I'll rework this function based on your comments and fail early on
> unsupported cases.
> 
> > > +}
> > > +
> > >  static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> > >  				u16 mask)
> > >  {
> > >  	int ret;
> > > -	int bits_per_word = ad7949_adc->resolution;
> > > -	int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
> > >  	struct spi_message msg;
> > >  	struct spi_transfer tx[] = {
> > >  		{
> > >  			.tx_buf = &ad7949_adc->buffer,
> > >  			.len = 2,
> > > -			.bits_per_word = bits_per_word,
> > > +			.bits_per_word = ad7949_adc->bits_per_word,
> > >  		},
> > >  	};
> > >  
> > > +	ad7949_adc->buffer = 0;
> > >  	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
> > > -	ad7949_adc->buffer = ad7949_adc->cfg << shift;
> > > +
> > > +	switch (ad7949_adc->bits_per_word) {
> > > +	case 16:
> > > +		ad7949_adc->buffer = ad7949_adc->cfg << 2;
> > > +		break;
> > > +	case 14:
> > > +		ad7949_adc->buffer = ad7949_adc->cfg;
> > > +		break;
> > > +	case 8:
> > > +	default:  
> >
> > Default here should be an error. Imagine a 12 bit device is added. Then
> > the above set_bits_per_word() might return either 12 or 8. If it returns
> > 12 then this is not the right thing to do. I'd argue we want to know
> > about
> > that rather than papering over it. Possibly we want to not let anything
> > other
> > than 14 bits through in set_bits_per_word() so we print an error
> > early...
> >  
> 
> Sorry about that, I added that default case maybe a little too quickly.
> I'll fix that.
> 
> > > +		/* Pack 14-bit value into 2 bytes, MSB first */
> > > +		ad7949_adc->buf8[0] = FIELD_GET(GENMASK(13, 6), ad7949_adc->cfg);
> > > +		ad7949_adc->buf8[1] = FIELD_GET(GENMASK(5, 0), ad7949_adc->cfg);
> > > +		ad7949_adc->buf8[1] = ad7949_adc->buf8[1] << 2;
> > > +		break;
> > > +	}
> > > +
> > >  	spi_message_init_with_transfers(&msg, tx, 1);
> > >  	ret = spi_sync(ad7949_adc->spi, &msg);
> > >  
> > > @@ -105,14 +139,12 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> > >  {
> > >  	int ret;
> > >  	int i;
> > > -	int bits_per_word = ad7949_adc->resolution;
> > > -	int mask = GENMASK(ad7949_adc->resolution - 1, 0);
> > >  	struct spi_message msg;
> > >  	struct spi_transfer tx[] = {
> > >  		{
> > >  			.rx_buf = &ad7949_adc->buffer,
> > >  			.len = 2,
> > > -			.bits_per_word = bits_per_word,
> > > +			.bits_per_word = ad7949_adc->bits_per_word,
> > >  		},
> > >  	};
> > >  
> > > @@ -147,7 +179,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;  
> >
> > buffer is __be16 and *val is an int so this isn't valid.
> >
> > Now curious bit is I'm assuming original code worked... Given big endian
> > platforms are rare, that makes me suspect something odd is going on.
> >    
> 
> Hum... that's probably my fault. I changed the type of buffer from u16
> to _be16.
> I'll add calls to be16_to_cpu() around this and the 14 bit case.

On most platforms that's going to reverse the bytes. So if this
worked before without it, then it will now be broken.
That makes me wonder if it was either broken previously, or the
only user was a big endian host, or more likely the byte reverse
isn't needed, meaning we have a wrong assumption here somewhere.

I might be missing another option though!

Jonathan

> 
> > > +		/* Shift-out padding bits */
> > > +		if (ad7949_adc->resolution == 14)
> > > +			*val = *val >> 2;  
> >
> > *val >>= 16 - ad7949_adc->resolution;
> >
> > No one likes conditionals if we don't need them!  
> 
> Will do.
> 
> >
> >  
> > > +		break;
> > > +	case 14:
> > > +		*val = ad7949_adc->buffer & GENMASK(13, 0);
> > > +		break;
> > > +	case 8:
> > > +	default:
> > > +		/* Convert byte array to u16, MSB first */
> > > +		*val = (ad7949_adc->buf8[0] << 8) | ad7949_adc->buf8[1];  
> >
> > Use unaligned_get_be16()
> >  
> > > +		/* Shift-out padding bits */
> > > +		if (ad7949_adc->resolution == 14)
> > > +			*val = *val >> 2;  
> >
> > As above make this unconditional.
> >  
> 
> Thanks,
> Liam
> 
> > > +		break;
> > > +	}
> > >  
> > >  	return 0;
> > >  }
> > > @@ -280,6 +330,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
> > >  	spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
> > >  	indio_dev->num_channels = spec->num_channels;
> > >  	ad7949_adc->resolution = spec->resolution;
> > > +	ad7949_set_bits_per_word(ad7949_adc);
> > >  
> > >  	ad7949_adc->vref = devm_regulator_get(dev, "vref");
> > >  	if (IS_ERR(ad7949_adc->vref)) {  
> 

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

[Liam Beguin]

Hi Jonathan,

On Sat Jul 10, 2021 at 1:00 PM EDT, Jonathan Cameron wrote:
> On Fri, 9 Jul 2021 11:58:56 -0400
> Liam Beguin <liambeguin@gmail.com> wrote:
>
> > From: Liam Beguin <lvb@xiphos.com>
> > 
> > Add bindings documentation for the adi,reference-source property.
> > This property is required to properly configure the ADC sample request
> > based on which reference source should be used for the calculation.
>
> Should this be per channel? That will effect some of what I say below...
>

We could make it per channel. Ideally, I'd also like to add support for
differential channels, so might as well add per channel configurations
now.

> > 
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > ---
> >  .../bindings/iio/adc/adi,ad7949.yaml          | 21 +++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> > index 9b56bd4d5510..eae3121cad01 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> > @@ -35,6 +35,27 @@ properties:
> >    "#io-channel-cells":
> >      const: 1
> >  
> > +  adi,reference-select:
>
> This is one field in the register, but it's not one thing, so lets break
> it up
> in DT. We should do this both to make for more readable dts files and to
> enforce the requirements on regulators...
>
> > +    description: |
> > +      Select the reference voltage source to use when converting samples.
> > +      Acceptable values are:
> > +      - 0: Internal reference and temperature sensor enabled.
> > +           Vref=2.5V, buffered output
> > +      - 1: Internal reference and temperature sensor enabled.
> > +           Vref=4.096V, buffered output
> > +      - 2: Use external reference, temperature sensor enabled.
> > +           Internal buffer disabled
> > +      - 3: Use external reference, internal buffer and temperature sensor
> > +           enabled.
> > +      - 6: Use external reference, internal buffer and temperature sensor
> > +           disabled.
> > +      - 7: Use external reference, internal buffer enabled.
> > +           Internal reference and temperature sensor disabled.
>
> So question 1 is whether to use an external or internal reference.
> Normally we'd make the coarse decision of whether to use an external
> reference
> by whether there is a regulator provided. That won't work so well if we
> make
> this per channel.
>
> Question 2, assuming internal reference, what voltage? Those should take
> an actual voltage (probably in mV and match against an enum of the two
> possible values).
> Binding should check to make sure this isn't specified as well as saying
> we
> are using an external refernce.
>
> Question 3, assuming external reference, is temperature sensor enabled?
> - actually dumb question, but why would anyone not want this enabled?
> Maybe turn it
> off in runtime pm, but in general if you've fitted a chip with a
> temperature sensor
> you at least sometimes want to measure temperature! So my gut feeling is
> don't
> allow this to be controlled (effectively drop cases 6 and 7 above as
> being
> unlikely to be of interest to anyone)
>

I like your suggestion of breaking this down so far, it would look
something like this:

	ad7949: adc@0 {
		compatible = "adi,ad7949";
		reg = <0>;

		vref-supply = <&vdd_supply>;

		channel@0 {
			adi,internal-ref-mv = <2500>;
			reg = <0>;
		};

		channel@1 {
			reg = <1>;
			/*
			 * defaults to vref-supply if defined or error
			 * out
			 */
		};
	};

> Question 4, Is the internal buffer enabled when using and external
> reference.
> This one is interesting. We could just expose it in general, but I
> wonder
> if we can do something that reflects how it is used. From the various
> figures in
> the datasheet this seems to be coupled to whether the external reference
> is on
> pin REF_IN or pin REF. If that's the case can we have two optional regs
> only
> one of which should be supplied? However, this gets more fiddly because
> the default right now is vref-supply actually being connected to the
> vrefin connection.
> That's annoying as it stops us using the obvious naming...
> Hence I think we can have
> vref-supply (actually connected to vrefin) and vref-unbuffered-supply
>

I really like the idea of using the same names as the datasheet
(vref-supply and vrefin-supply), to infer the buffered state,
but it's annoying (and confusing) that it's setup the other way
right now.

I wonder what happens if the reference is connected to refin and we're
configured as unbuffered (and the other way around).
I looked around and I might be able to test it on one setup I have where
the external reference is connected to REF.

If it's not a breaking change, would it be okay with you to follow the
datasheet naming?

Liam

>
>
> > +
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1, 2, 3, 6, 7]
> > +    default: 7
> > +
> >  required:
> >    - compatible
> >    - reg

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

[Jonathan Cameron]

On Mon, 12 Jul 2021 13:05:23 -0400
"Liam Beguin" <liambeguin@gmail.com> wrote:

> Hi Jonathan,
> 
> On Sat Jul 10, 2021 at 1:00 PM EDT, Jonathan Cameron wrote:
> > On Fri, 9 Jul 2021 11:58:56 -0400
> > Liam Beguin <liambeguin@gmail.com> wrote:
> >  
> > > From: Liam Beguin <lvb@xiphos.com>
> > > 
> > > Add bindings documentation for the adi,reference-source property.
> > > This property is required to properly configure the ADC sample request
> > > based on which reference source should be used for the calculation.  
> >
> > Should this be per channel? That will effect some of what I say below...
> >  
> 
> We could make it per channel. Ideally, I'd also like to add support for
> differential channels, so might as well add per channel configurations
> now.
> 
> > > 
> > > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > > ---
> > >  .../bindings/iio/adc/adi,ad7949.yaml          | 21 +++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> > > index 9b56bd4d5510..eae3121cad01 100644
> > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
> > > @@ -35,6 +35,27 @@ properties:
> > >    "#io-channel-cells":
> > >      const: 1
> > >  
> > > +  adi,reference-select:  
> >
> > This is one field in the register, but it's not one thing, so lets break
> > it up
> > in DT. We should do this both to make for more readable dts files and to
> > enforce the requirements on regulators...
> >  
> > > +    description: |
> > > +      Select the reference voltage source to use when converting samples.
> > > +      Acceptable values are:
> > > +      - 0: Internal reference and temperature sensor enabled.
> > > +           Vref=2.5V, buffered output
> > > +      - 1: Internal reference and temperature sensor enabled.
> > > +           Vref=4.096V, buffered output
> > > +      - 2: Use external reference, temperature sensor enabled.
> > > +           Internal buffer disabled
> > > +      - 3: Use external reference, internal buffer and temperature sensor
> > > +           enabled.
> > > +      - 6: Use external reference, internal buffer and temperature sensor
> > > +           disabled.
> > > +      - 7: Use external reference, internal buffer enabled.
> > > +           Internal reference and temperature sensor disabled.  
> >
> > So question 1 is whether to use an external or internal reference.
> > Normally we'd make the coarse decision of whether to use an external
> > reference
> > by whether there is a regulator provided. That won't work so well if we
> > make
> > this per channel.
> >
> > Question 2, assuming internal reference, what voltage? Those should take
> > an actual voltage (probably in mV and match against an enum of the two
> > possible values).
> > Binding should check to make sure this isn't specified as well as saying
> > we
> > are using an external refernce.
> >
> > Question 3, assuming external reference, is temperature sensor enabled?
> > - actually dumb question, but why would anyone not want this enabled?
> > Maybe turn it
> > off in runtime pm, but in general if you've fitted a chip with a
> > temperature sensor
> > you at least sometimes want to measure temperature! So my gut feeling is
> > don't
> > allow this to be controlled (effectively drop cases 6 and 7 above as
> > being
> > unlikely to be of interest to anyone)
> >  
> 
> I like your suggestion of breaking this down so far, it would look
> something like this:
> 
> 	ad7949: adc@0 {
> 		compatible = "adi,ad7949";
> 		reg = <0>;
> 
> 		vref-supply = <&vdd_supply>;
> 
> 		channel@0 {
> 			adi,internal-ref-mv = <2500>;
> 			reg = <0>;
> 		};
> 
> 		channel@1 {
> 			reg = <1>;
> 			/*
> 			 * defaults to vref-supply if defined or error
> 			 * out
> 			 */
> 		};
> 	};
> 
> > Question 4, Is the internal buffer enabled when using and external
> > reference.
> > This one is interesting. We could just expose it in general, but I
> > wonder
> > if we can do something that reflects how it is used. From the various
> > figures in
> > the datasheet this seems to be coupled to whether the external reference
> > is on
> > pin REF_IN or pin REF. If that's the case can we have two optional regs
> > only
> > one of which should be supplied? However, this gets more fiddly because
> > the default right now is vref-supply actually being connected to the
> > vrefin connection.
> > That's annoying as it stops us using the obvious naming...
> > Hence I think we can have
> > vref-supply (actually connected to vrefin) and vref-unbuffered-supply
> >  
> 
> I really like the idea of using the same names as the datasheet
> (vref-supply and vrefin-supply), to infer the buffered state,
> but it's annoying (and confusing) that it's setup the other way
> right now.
> 
> I wonder what happens if the reference is connected to refin and we're
> configured as unbuffered (and the other way around).
> I looked around and I might be able to test it on one setup I have where
> the external reference is connected to REF.
> 
> If it's not a breaking change, would it be okay with you to follow the
> datasheet naming?

Absolutely. If we can get away with fixing that it would be great.

Jonathan

> 
> Liam
> 
> >
> >  
> > > +
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [0, 1, 2, 3, 6, 7]
> > > +    default: 7
> > > +
> > >  required:
> > >    - compatible
> > >    - reg