[PATCH v4 0/5] AD7949 Fixes

[PATCH v4 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 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                      | 261 ++++++++++++++----
 2 files changed, 274 insertions(+), 56 deletions(-)

Range-diff against v3:
-:  ------------ > 1:  8760b368f971 iio: adc: ad7949: define and use bitfield names
1:  a9243c834705 ! 2:  7b1484f2fc4c 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/regulator/consumer.h>
    - #include <linux/spi/spi.h>
    - #include <linux/bitfield.h>
    -+#include <asm/unaligned.h>
    - 
    - #define AD7949_MASK_TOTAL		GENMASK(13, 0)
    - 
     @@ 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 int ad7949_spi_write_cfg(struct ad7949_adc_chip
      	int ret;
     -	int bits_per_word = ad7949_adc->resolution;
     -	int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
    -+	u8 buf8[2];
      	struct spi_message msg;
      	struct spi_transfer tx[] = {
      		{
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_write_cfg(struct ad7949_adc_chip
     +		ad7949_adc->buffer = ad7949_adc->cfg;
     +		break;
     +	case 8:
    -+		/* Pack 14-bit value into 2 bytes, MSB first */
    -+		buf8[0] = FIELD_GET(GENMASK(13, 6), ad7949_adc->cfg);
    -+		buf8[1] = FIELD_GET(GENMASK(5, 0), ad7949_adc->cfg) << 2;
    -+		memcpy(&ad7949_adc->buffer, buf8, 2);
    ++		/* Here, type is big endian as it must be sent in two transfers */
    ++		ad7949_adc->buffer = (u16)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
      	int i;
     -	int bits_per_word = ad7949_adc->resolution;
     -	int mask = GENMASK(ad7949_adc->resolution - 1, 0);
    -+	u8 buf8[2];
      	struct spi_message msg;
      	struct spi_transfer tx[] = {
      		{
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_c
     +		*val = ad7949_adc->buffer & GENMASK(13, 0);
     +		break;
     +	case 8:
    -+		memcpy(buf8, &ad7949_adc->buffer, 2);
    -+		/* Convert byte array to u16, MSB first */
    -+		*val = get_unaligned_be16(buf8);
    ++		/* Here, type is big endian as data was sent in two transfers */
    ++		*val = be16_to_cpu(ad7949_adc->buffer);
     +		/* Shift-out padding bits */
     +		*val >>= 16 - ad7949_adc->resolution;
     +		break;
2:  bb25b7fcb0a3 ! 3:  41c4ab9c5e19 iio: adc: ad7949: add support for internal vref
    @@ drivers/iio/adc/ad7949.c: struct ad7949_adc_chip {
      	u8 bits_per_word;
      	u16 cfg;
     @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
    + 	int ret;
      	int i;
    - 	u8 buf8[2];
      	struct spi_message msg;
    -+	struct ad7949_channel ad7949_chan = ad7949_adc->channels[channel];
    ++	struct ad7949_channel *ad7949_chan = &ad7949_adc->channels[channel];
      	struct spi_transfer tx[] = {
      		{
      			.rx_buf = &ad7949_adc->buffer,
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_c
     -					   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),
    ++					   FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_chan->refsel),
     +					   AD7949_CFG_BIT_INX | AD7949_CFG_BIT_REF);
      		if (ret)
      			return ret;
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_raw(struct iio_dev *indio_d
      			   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];
    ++	struct ad7949_channel *ad7949_chan = &ad7949_adc->channels[chan->channel];
      	int ret;
      
      	if (!val)
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_raw(struct iio_dev *indio_d
     -		ret = regulator_get_voltage(ad7949_adc->vref);
     -		if (ret < 0)
     -			return ret;
    -+		switch (ad7949_chan.refsel) {
    ++		switch (ad7949_chan->refsel) {
     +		case AD7949_CFG_VAL_REF_INT_2500:
     +			*val = 2500;
     +			break;
    @@ 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 ad7949_channel *ad7949_chan;
    -+	struct device_node *child;
    ++	struct fwnode_handle *child;
      	struct iio_dev *indio_dev;
     +	int mode;
     +	u32 tmp;
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_probe(struct spi_device *spi)
     -	if (ret < 0) {
     -		dev_err(dev, "fail to enable regulator\n");
     -		return ret;
    -+	if (mode > AD7949_CFG_VAL_REF_INT_4096) {
    ++	if (mode & AD7949_CFG_VAL_REF_EXTERNAL) {
     +		ret = regulator_enable(ad7949_adc->vref);
     +		if (ret < 0) {
     +			dev_err(dev, "fail to enable regulator\n");
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_probe(struct spi_device *spi)
     +	}
     +
     +	/* Initialize all channel structures */
    -+	for (i = 0; i < spec->num_channels; i++) {
    ++	for (i = 0; i < spec->num_channels; i++)
     +		ad7949_adc->channels[i].refsel = mode;
    -+	}
     +
     +	/* Read channel specific information form the devicetree */
    -+	for_each_child_of_node(dev->of_node, child) {
    -+		ret = of_property_read_u32(child, "reg", &i);
    ++	device_for_each_child_node(dev, child) {
    ++		ret = fwnode_property_read_u32(child, "reg", &i);
     +		if (ret) {
    -+			dev_err(dev, "missing reg property in child: %s\n",
    -+				child->full_name);
    -+			of_node_put(child);
    ++			dev_err(dev, "missing reg property in %pfw\n", child);
    ++			fwnode_handle_put(child);
     +			return ret;
     +		}
     +
     +		ad7949_chan = &ad7949_adc->channels[i];
     +
    -+		ret = of_property_read_u32(child, "adi,internal-ref-mv", &tmp);
    ++		ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
     +		if (ret < 0 && ret != -EINVAL) {
    -+			of_node_put(child);
    ++			dev_err(dev, "invalid internal reference in %pfw\n", child);
    ++			fwnode_handle_put(child);
     +			return ret;
     +		}
     +
     +		switch (tmp) {
    -+		case 2500:
    ++		case 2500000:
     +			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_2500;
     +			break;
    -+		case 4096:
    ++		case 4096000:
     +			ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_4096;
     +			break;
     +		default:
     +			dev_err(dev, "unsupported internal voltage reference\n");
    -+			of_node_put(child);
    ++			fwnode_handle_put(child);
     +			return -EINVAL;
     +		}
      	}
3:  41e3ca4f0d52 ! 4:  9cb48acbd05b dt-bindings: iio: adc: ad7949: add per channel reference
    @@ Commit message
     
         Add bindings documentation describing per channel reference voltage
         selection.
    -    This adds the adi,internal-ref-mv property, and child nodes for each
    -    channel. This is required to properly configure the ADC sample request
    -    based on which reference source should be used for the calculation.
    +    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>
     
    @@ Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml: properties:
     +
     +  '#size-cells':
     +    const: 0
    -+
     +
      required:
        - compatible
    @@ Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml: properties:
     +          minimum: 0
     +          maximum: 7
     +
    -+      adi,internal-ref-mv:
    ++      adi,internal-ref-microvolt:
     +        description: |
    -+          Internal reference voltage selection in millivolts.
    ++          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).
    @@ Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml: properties:
     +          If no supplies are defined, the reference selection will default to
     +          4096mV internal reference.
     +
    -+        $ref: /schemas/types.yaml#/definitions/uint32
    -+        enum: [2500, 4096]
    -+        default: 4096
    ++        enum: [2500000, 4096000]
    ++        default: 4096000
     +
     +    required:
     +      - reg
    @@ Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml: examples:
     +            vrefin-supply = <&vdd_supply>;
     +
     +            channel@0 {
    -+                adi,internal-ref-mv = <4096>;
    ++                adi,internal-ref-microvolt = <4096000>;
     +                reg = <0>;
     +            };
     +
     +            channel@1 {
    -+                adi,internal-ref-mv = <2500>;
    ++                adi,internal-ref-microvolt = <2500000>;
     +                reg = <1>;
     +            };
     +
-:  ------------ > 5:  c48eb017058c iio: adc: ad7949: use devm managed functions

base-commit: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
-- 
2.30.1.489.g328c10930387

[PATCH v4 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 | 50 +++++++++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 1b4b3203e428..0b549b8bd7a9 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -11,12 +11,37 @@
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
+#include <linux/bitfield.h>
 
-#define AD7949_MASK_CHANNEL_SEL		GENMASK(9, 7)
 #define AD7949_MASK_TOTAL		GENMASK(13, 0)
-#define AD7949_OFFSET_CHANNEL_SEL	7
-#define AD7949_CFG_READ_BACK		0x1
-#define AD7949_CFG_REG_SIZE_BITS	14
+
+/* CFG: Configuration Update */
+#define AD7949_CFG_BIT_OVERWRITE	BIT(13)
+
+/* INCC: Input Channel Configuration */
+#define AD7949_CFG_BIT_INCC		GENMASK(12, 10)
+#define AD7949_CFG_VAL_INCC_UNIPOLAR_GND	7
+#define AD7949_CFG_VAL_INCC_UNIPOLAR_COMM	6
+#define AD7949_CFG_VAL_INCC_UNIPOLAR_DIFF	4
+#define AD7949_CFG_VAL_INCC_TEMP		3
+#define AD7949_CFG_VAL_INCC_BIPOLAR		2
+#define AD7949_CFG_VAL_INCC_BIPOLAR_DIFF	0
+
+/* INX: Input channel Selection in a binary fashion */
+#define AD7949_CFG_BIT_INX		GENMASK(9, 7)
+
+/* BW: select bandwidth for low-pass filter. Full or Quarter */
+#define AD7949_CFG_BIT_BW_FULL			BIT(6)
+
+/* REF: reference/buffer selection */
+#define AD7949_CFG_BIT_REF		GENMASK(5, 3)
+#define AD7949_CFG_VAL_REF_EXT_BUF		7
+
+/* SEQ: channel sequencer. Allows for scanning channels */
+#define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
+
+/* RB: Read back the CFG register */
+#define AD7949_CFG_BIT_RBN		BIT(0)
 
 enum {
 	ID_AD7949 = 0,
@@ -109,8 +134,8 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	 */
 	for (i = 0; i < 2; i++) {
 		ret = ad7949_spi_write_cfg(ad7949_adc,
-					   channel << AD7949_OFFSET_CHANNEL_SEL,
-					   AD7949_MASK_CHANNEL_SEL);
+					   FIELD_PREP(AD7949_CFG_BIT_INX, channel),
+					   AD7949_CFG_BIT_INX);
 		if (ret)
 			return ret;
 		if (channel == ad7949_adc->current_channel)
@@ -214,10 +239,19 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 {
 	int ret;
 	int val;
+	u16 cfg;
 
-	/* Sequencer disabled, CFG readback disabled, IN0 as default channel */
 	ad7949_adc->current_channel = 0;
-	ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
+
+	cfg = FIELD_PREP(AD7949_CFG_BIT_OVERWRITE, 1) |
+		FIELD_PREP(AD7949_CFG_BIT_INCC, AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
+		FIELD_PREP(AD7949_CFG_BIT_INX, ad7949_adc->current_channel) |
+		FIELD_PREP(AD7949_CFG_BIT_BW_FULL, 1) |
+		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_CFG_VAL_REF_EXT_BUF) |
+		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
+		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);
+
+	ret = ad7949_spi_write_cfg(ad7949_adc, cfg, AD7949_MASK_TOTAL);
 
 	/*
 	 * Do two dummy conversions to apply the first configuration setting.
-- 
2.30.1.489.g328c10930387

[PATCH v4 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 | 62 ++++++++++++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 0b549b8bd7a9..f1702c54c8be 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -67,6 +67,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
  * @indio_dev: reference to iio structure
  * @spi: reference to spi structure
  * @resolution: resolution of the chip
+ * @bits_per_word: number of bits per SPI word
  * @cfg: copy of the configuration register
  * @current_channel: current channel in use
  * @buffer: buffer to send / receive data to / from device
@@ -77,6 +78,7 @@ struct ad7949_adc_chip {
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
 	u8 resolution;
+	u8 bits_per_word;
 	u16 cfg;
 	unsigned int current_channel;
 	u16 buffer ____cacheline_aligned;
@@ -86,19 +88,34 @@ 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:
+		/* Here, type is big endian as it must be sent in two transfers */
+		ad7949_adc->buffer = (u16)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);
 
@@ -115,14 +132,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,
 		},
 	};
 
@@ -157,7 +172,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->buffer);
+		/* Shift-out padding bits */
+		*val >>= 16 - ad7949_adc->resolution;
+		break;
+	default:
+		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
+		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -265,6 +298,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 
 static int ad7949_spi_probe(struct spi_device *spi)
 {
+	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
 	struct device *dev = &spi->dev;
 	const struct ad7949_adc_spec *spec;
 	struct ad7949_adc_chip *ad7949_adc;
@@ -291,6 +325,18 @@ static int ad7949_spi_probe(struct spi_device *spi)
 	indio_dev->num_channels = spec->num_channels;
 	ad7949_adc->resolution = spec->resolution;
 
+	/* Set SPI bits per word */
+	if (spi_ctrl_mask & SPI_BPW_MASK(ad7949_adc->resolution)) {
+		ad7949_adc->bits_per_word = ad7949_adc->resolution;
+	} else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
+		ad7949_adc->bits_per_word = 16;
+	} else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
+		ad7949_adc->bits_per_word = 8;
+	} else {
+		dev_err(dev, "unable to find common BPW with spi controller\n");
+		return -EINVAL;
+	}
+
 	ad7949_adc->vref = devm_regulator_get(dev, "vref");
 	if (IS_ERR(ad7949_adc->vref)) {
 		dev_err(dev, "fail to request regulator\n");
-- 
2.30.1.489.g328c10930387

[PATCH v4 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 | 132 +++++++++++++++++++++++++++++++++------
 1 file changed, 114 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index f1702c54c8be..eaea8f5e87d8 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
@@ -77,6 +89,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;
@@ -133,6 +146,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[] = {
 		{
 			.rx_buf = &ad7949_adc->buffer,
@@ -149,8 +163,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)
@@ -219,6 +234,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)
@@ -236,12 +252,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;
@@ -280,7 +310,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);
 
@@ -296,14 +326,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) {
@@ -337,16 +377,74 @@ static int ad7949_spi_probe(struct spi_device *spi)
 		return -EINVAL;
 	}
 
-	ad7949_adc->vref = devm_regulator_get(dev, "vref");
+	/* Setup external voltage ref, buffered? */
+	ad7949_adc->vref = devm_regulator_get(dev, "vrefin");
 	if (IS_ERR(ad7949_adc->vref)) {
-		dev_err(dev, "fail to request regulator\n");
-		return PTR_ERR(ad7949_adc->vref);
+		/* unbuffered? */
+		ad7949_adc->vref = devm_regulator_get(dev, "vref");
+		if (IS_ERR(ad7949_adc->vref)) {
+			/* Internal then */
+			mode = AD7949_CFG_VAL_REF_INT_4096;
+		}
+		mode = AD7949_CFG_VAL_REF_EXT_TEMP;
 	}
+	mode = AD7949_CFG_VAL_REF_EXT_TEMP_BUF;
 
-	ret = regulator_enable(ad7949_adc->vref);
-	if (ret < 0) {
-		dev_err(dev, "fail to enable regulator\n");
-		return ret;
+	if (mode & AD7949_CFG_VAL_REF_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);
@@ -367,7 +465,6 @@ static int ad7949_spi_probe(struct spi_device *spi)
 
 err:
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
 
 	return ret;
 }
@@ -379,7 +476,6 @@ static int ad7949_spi_remove(struct spi_device *spi)
 
 	iio_device_unregister(indio_dev);
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
 
 	return 0;
 }
-- 
2.30.1.489.g328c10930387

[PATCH v4 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>
---
 .../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.30.1.489.g328c10930387

[PATCH v4 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 eaea8f5e87d8..29a87a12e551 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -452,34 +452,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" },
@@ -502,7 +484,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.30.1.489.g328c10930387

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

[Jonathan Cameron]

On Tue, 27 Jul 2021 19:29:03 -0400
Liam Beguin <liambeguin@gmail.com> wrote:

> From: Liam Beguin <lvb@xiphos.com>
> 
> This driver supports devices with 14-bit and 16-bit sample sizes.
> This is not always handled properly by spi controllers and can fail. To
> work around this limitation, pad samples to 16-bit and split the sample
> into two 8-bit messages in the event that only 8-bit messages are
> supported by the controller.
> 
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
>  drivers/iio/adc/ad7949.c | 62 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 54 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index 0b549b8bd7a9..f1702c54c8be 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -67,6 +67,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
>   * @indio_dev: reference to iio structure
>   * @spi: reference to spi structure
>   * @resolution: resolution of the chip
> + * @bits_per_word: number of bits per SPI word
>   * @cfg: copy of the configuration register
>   * @current_channel: current channel in use
>   * @buffer: buffer to send / receive data to / from device
> @@ -77,6 +78,7 @@ struct ad7949_adc_chip {
>  	struct iio_dev *indio_dev;
>  	struct spi_device *spi;
>  	u8 resolution;
> +	u8 bits_per_word;
>  	u16 cfg;
>  	unsigned int current_channel;
>  	u16 buffer ____cacheline_aligned;
> @@ -86,19 +88,34 @@ 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;

The define for this was removed in patch 1.  I'll fix that up whilst applying by
keeping it until this patch.  Please check build passes on intermediate points
during a patch series as otherwise we may break bisectability and that's really
annoying if you are bisecting!

Jonathan

>  	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:
> +		/* Here, type is big endian as it must be sent in two transfers */
> +		ad7949_adc->buffer = (u16)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);
>  
> @@ -115,14 +132,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,
>  		},
>  	};
>  
> @@ -157,7 +172,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->buffer);
> +		/* Shift-out padding bits */
> +		*val >>= 16 - ad7949_adc->resolution;
> +		break;
> +	default:
> +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> +		return -EINVAL;
> +	}
>  
>  	return 0;
>  }
> @@ -265,6 +298,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
>  
>  static int ad7949_spi_probe(struct spi_device *spi)
>  {
> +	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
>  	struct device *dev = &spi->dev;
>  	const struct ad7949_adc_spec *spec;
>  	struct ad7949_adc_chip *ad7949_adc;
> @@ -291,6 +325,18 @@ static int ad7949_spi_probe(struct spi_device *spi)
>  	indio_dev->num_channels = spec->num_channels;
>  	ad7949_adc->resolution = spec->resolution;
>  
> +	/* Set SPI bits per word */
> +	if (spi_ctrl_mask & SPI_BPW_MASK(ad7949_adc->resolution)) {
> +		ad7949_adc->bits_per_word = ad7949_adc->resolution;
> +	} else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
> +		ad7949_adc->bits_per_word = 16;
> +	} else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
> +		ad7949_adc->bits_per_word = 8;
> +	} else {
> +		dev_err(dev, "unable to find common BPW with spi controller\n");
> +		return -EINVAL;
> +	}
> +
>  	ad7949_adc->vref = devm_regulator_get(dev, "vref");
>  	if (IS_ERR(ad7949_adc->vref)) {
>  		dev_err(dev, "fail to request regulator\n");

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

[Jonathan Cameron]

On Sat, 31 Jul 2021 15:29:21 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 27 Jul 2021 19:29:03 -0400
> Liam Beguin <liambeguin@gmail.com> wrote:
> 
> > From: Liam Beguin <lvb@xiphos.com>
> > 
> > This driver supports devices with 14-bit and 16-bit sample sizes.
> > This is not always handled properly by spi controllers and can fail. To
> > work around this limitation, pad samples to 16-bit and split the sample
> > into two 8-bit messages in the event that only 8-bit messages are
> > supported by the controller.
> > 
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > ---
> >  drivers/iio/adc/ad7949.c | 62 ++++++++++++++++++++++++++++++++++------
> >  1 file changed, 54 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > index 0b549b8bd7a9..f1702c54c8be 100644
> > --- a/drivers/iio/adc/ad7949.c
> > +++ b/drivers/iio/adc/ad7949.c
> > @@ -67,6 +67,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
> >   * @indio_dev: reference to iio structure
> >   * @spi: reference to spi structure
> >   * @resolution: resolution of the chip
> > + * @bits_per_word: number of bits per SPI word
> >   * @cfg: copy of the configuration register
> >   * @current_channel: current channel in use
> >   * @buffer: buffer to send / receive data to / from device
> > @@ -77,6 +78,7 @@ struct ad7949_adc_chip {
> >  	struct iio_dev *indio_dev;
> >  	struct spi_device *spi;
> >  	u8 resolution;
> > +	u8 bits_per_word;
> >  	u16 cfg;
> >  	unsigned int current_channel;
> >  	u16 buffer ____cacheline_aligned;
> > @@ -86,19 +88,34 @@ 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;  
> 
> The define for this was removed in patch 1.  I'll fix that up whilst applying by
> keeping it until this patch.  Please check build passes on intermediate points
> during a patch series as otherwise we may break bisectability and that's really
> annoying if you are bisecting!
> 
> Jonathan
> 
> >  	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:
> > +		/* Here, type is big endian as it must be sent in two transfers */
> > +		ad7949_adc->buffer = (u16)cpu_to_be16(ad7949_adc->cfg << 2);

Gah, I wasn't thinking clearly when I suggested this.  Sparse warns on the
endian conversion

One option is to resort to ignoring the fact we know it's aligned and
use the put_unaligned_be16() and get_unaligned_be16 calls which sparse seems to be
happy with.  Alternative would be to just have a be16 buffer after the existing
one in the iio_priv structure. Then you will have to change the various users
of iio_priv()->buffer to point to the new value if we are doing 8 bit transfers.

Whilst more invasive, this second option is the one I'd suggest.
Note that there will be no need to add an __cacheline_aligned marking to this
new element because it will be in a cachline that is only used for DMA simply being
after the other buffer element which is force to start on a new cacheline.

Jonathan
 
> > +		break;
> > +	default:
> > +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	spi_message_init_with_transfers(&msg, tx, 1);
> >  	ret = spi_sync(ad7949_adc->spi, &msg);
> >  
> > @@ -115,14 +132,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,
> >  		},
> >  	};
> >  
> > @@ -157,7 +172,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->buffer);
> > +		/* Shift-out padding bits */
> > +		*val >>= 16 - ad7949_adc->resolution;
> > +		break;
> > +	default:
> > +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> > +		return -EINVAL;
> > +	}
> >  
> >  	return 0;
> >  }
> > @@ -265,6 +298,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
> >  
> >  static int ad7949_spi_probe(struct spi_device *spi)
> >  {
> > +	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
> >  	struct device *dev = &spi->dev;
> >  	const struct ad7949_adc_spec *spec;
> >  	struct ad7949_adc_chip *ad7949_adc;
> > @@ -291,6 +325,18 @@ static int ad7949_spi_probe(struct spi_device *spi)
> >  	indio_dev->num_channels = spec->num_channels;
> >  	ad7949_adc->resolution = spec->resolution;
> >  
> > +	/* Set SPI bits per word */
> > +	if (spi_ctrl_mask & SPI_BPW_MASK(ad7949_adc->resolution)) {
> > +		ad7949_adc->bits_per_word = ad7949_adc->resolution;
> > +	} else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
> > +		ad7949_adc->bits_per_word = 16;
> > +	} else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
> > +		ad7949_adc->bits_per_word = 8;
> > +	} else {
> > +		dev_err(dev, "unable to find common BPW with spi controller\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	ad7949_adc->vref = devm_regulator_get(dev, "vref");
> >  	if (IS_ERR(ad7949_adc->vref)) {
> >  		dev_err(dev, "fail to request regulator\n");  
> 

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

[Jonathan Cameron]

On Tue, 27 Jul 2021 19:29:04 -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>

I didn't do a great job reviewing earlier versions of this series. Sorry about
that. As a result some comments inline.

Jonathan

> ---
>  drivers/iio/adc/ad7949.c | 132 +++++++++++++++++++++++++++++++++------
>  1 file changed, 114 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index f1702c54c8be..eaea8f5e87d8 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
> @@ -77,6 +89,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;
> @@ -133,6 +146,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[] = {
>  		{
>  			.rx_buf = &ad7949_adc->buffer,
> @@ -149,8 +163,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)
> @@ -219,6 +234,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)
> @@ -236,12 +252,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;
> @@ -280,7 +310,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);
>  
> @@ -296,14 +326,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) {
> @@ -337,16 +377,74 @@ static int ad7949_spi_probe(struct spi_device *spi)
>  		return -EINVAL;
>  	}
>  
> -	ad7949_adc->vref = devm_regulator_get(dev, "vref");
> +	/* Setup external voltage ref, buffered? */
> +	ad7949_adc->vref = devm_regulator_get(dev, "vrefin");

Needs to be the optional form as otherwise we may get a stub regulator
and not fail here as we should.

>  	if (IS_ERR(ad7949_adc->vref)) {

Need to check the error code for -ENODEV (IIRC) which is the only error
to indicate it is not present.  If we get -EDEFER for example then we want
to fail the probe here as we know we should have the regulator later, once its
own driver has loaded.

> -		dev_err(dev, "fail to request regulator\n");
> -		return PTR_ERR(ad7949_adc->vref);
> +		/* unbuffered? */
> +		ad7949_adc->vref = devm_regulator_get(dev, "vref");

devm_regulator_get_optional() again to avoid stub regulators.

> +		if (IS_ERR(ad7949_adc->vref)) {

As above, need to check specifically for the code that reflects it not
being specified rather than any error.  Only if it is not specified
do we want to be ignore the error and use the internal regulator.

> +			/* Internal then */
> +			mode = AD7949_CFG_VAL_REF_INT_4096;
> +		}
> +		mode = AD7949_CFG_VAL_REF_EXT_TEMP;
>  	}
> +	mode = AD7949_CFG_VAL_REF_EXT_TEMP_BUF;
>  
> -	ret = regulator_enable(ad7949_adc->vref);
> -	if (ret < 0) {
> -		dev_err(dev, "fail to enable regulator\n");
> -		return ret;
> +	if (mode & AD7949_CFG_VAL_REF_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);
> @@ -367,7 +465,6 @@ static int ad7949_spi_probe(struct spi_device *spi)
>  
>  err:
>  	mutex_destroy(&ad7949_adc->lock);
> -	regulator_disable(ad7949_adc->vref);
>  
>  	return ret;
>  }
> @@ -379,7 +476,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 v4 4/5] dt-bindings: iio: adc: ad7949: add per channel reference

[Jonathan Cameron]

On Tue, 27 Jul 2021 19:29:05 -0400
Liam Beguin <liambeguin@gmail.com> wrote:

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

I'm fine with this, but as it's a bit unusual, definitely want to give a
little more time for Rob and others to take a look.

Jonathan
> ---
>  .../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>;
> +            };
>          };
>      };
>  ...

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

[Liam Beguin]

On Sat Jul 31, 2021 at 10:29 AM EDT, Jonathan Cameron wrote:
> On Tue, 27 Jul 2021 19:29:03 -0400
> Liam Beguin <liambeguin@gmail.com> wrote:
>
> > From: Liam Beguin <lvb@xiphos.com>
> > 
> > This driver supports devices with 14-bit and 16-bit sample sizes.
> > This is not always handled properly by spi controllers and can fail. To
> > work around this limitation, pad samples to 16-bit and split the sample
> > into two 8-bit messages in the event that only 8-bit messages are
> > supported by the controller.
> > 
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > ---
> >  drivers/iio/adc/ad7949.c | 62 ++++++++++++++++++++++++++++++++++------
> >  1 file changed, 54 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > index 0b549b8bd7a9..f1702c54c8be 100644
> > --- a/drivers/iio/adc/ad7949.c
> > +++ b/drivers/iio/adc/ad7949.c
> > @@ -67,6 +67,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
> >   * @indio_dev: reference to iio structure
> >   * @spi: reference to spi structure
> >   * @resolution: resolution of the chip
> > + * @bits_per_word: number of bits per SPI word
> >   * @cfg: copy of the configuration register
> >   * @current_channel: current channel in use
> >   * @buffer: buffer to send / receive data to / from device
> > @@ -77,6 +78,7 @@ struct ad7949_adc_chip {
> >  	struct iio_dev *indio_dev;
> >  	struct spi_device *spi;
> >  	u8 resolution;
> > +	u8 bits_per_word;
> >  	u16 cfg;
> >  	unsigned int current_channel;
> >  	u16 buffer ____cacheline_aligned;
> > @@ -86,19 +88,34 @@ 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;

Hi Jonathan,

>
> The define for this was removed in patch 1. I'll fix that up whilst
> applying by
> keeping it until this patch. Please check build passes on intermediate
> points
> during a patch series as otherwise we may break bisectability and that's
> really
> annoying if you are bisecting!

Apologies for that. I'll make sure to add at least a `git rebase -x` to
my checks.
I'll fix it before sending the next version of this series.

Thanks,
Liam

>
> Jonathan
>
> >  	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:
> > +		/* Here, type is big endian as it must be sent in two transfers */
> > +		ad7949_adc->buffer = (u16)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);
> >  
> > @@ -115,14 +132,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,
> >  		},
> >  	};
> >  
> > @@ -157,7 +172,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->buffer);
> > +		/* Shift-out padding bits */
> > +		*val >>= 16 - ad7949_adc->resolution;
> > +		break;
> > +	default:
> > +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> > +		return -EINVAL;
> > +	}
> >  
> >  	return 0;
> >  }
> > @@ -265,6 +298,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
> >  
> >  static int ad7949_spi_probe(struct spi_device *spi)
> >  {
> > +	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
> >  	struct device *dev = &spi->dev;
> >  	const struct ad7949_adc_spec *spec;
> >  	struct ad7949_adc_chip *ad7949_adc;
> > @@ -291,6 +325,18 @@ static int ad7949_spi_probe(struct spi_device *spi)
> >  	indio_dev->num_channels = spec->num_channels;
> >  	ad7949_adc->resolution = spec->resolution;
> >  
> > +	/* Set SPI bits per word */
> > +	if (spi_ctrl_mask & SPI_BPW_MASK(ad7949_adc->resolution)) {
> > +		ad7949_adc->bits_per_word = ad7949_adc->resolution;
> > +	} else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
> > +		ad7949_adc->bits_per_word = 16;
> > +	} else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
> > +		ad7949_adc->bits_per_word = 8;
> > +	} else {
> > +		dev_err(dev, "unable to find common BPW with spi controller\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	ad7949_adc->vref = devm_regulator_get(dev, "vref");
> >  	if (IS_ERR(ad7949_adc->vref)) {
> >  		dev_err(dev, "fail to request regulator\n");

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

[Liam Beguin]

On Sat Jul 31, 2021 at 10:52 AM EDT, Jonathan Cameron wrote:
> On Sat, 31 Jul 2021 15:29:21 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
>
> > On Tue, 27 Jul 2021 19:29:03 -0400
> > Liam Beguin <liambeguin@gmail.com> wrote:
> > 
> > > From: Liam Beguin <lvb@xiphos.com>
> > > 
> > > This driver supports devices with 14-bit and 16-bit sample sizes.
> > > This is not always handled properly by spi controllers and can fail. To
> > > work around this limitation, pad samples to 16-bit and split the sample
> > > into two 8-bit messages in the event that only 8-bit messages are
> > > supported by the controller.
> > > 
> > > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > > ---
> > >  drivers/iio/adc/ad7949.c | 62 ++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 54 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > > index 0b549b8bd7a9..f1702c54c8be 100644
> > > --- a/drivers/iio/adc/ad7949.c
> > > +++ b/drivers/iio/adc/ad7949.c
> > > @@ -67,6 +67,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
> > >   * @indio_dev: reference to iio structure
> > >   * @spi: reference to spi structure
> > >   * @resolution: resolution of the chip
> > > + * @bits_per_word: number of bits per SPI word
> > >   * @cfg: copy of the configuration register
> > >   * @current_channel: current channel in use
> > >   * @buffer: buffer to send / receive data to / from device
> > > @@ -77,6 +78,7 @@ struct ad7949_adc_chip {
> > >  	struct iio_dev *indio_dev;
> > >  	struct spi_device *spi;
> > >  	u8 resolution;
> > > +	u8 bits_per_word;
> > >  	u16 cfg;
> > >  	unsigned int current_channel;
> > >  	u16 buffer ____cacheline_aligned;
> > > @@ -86,19 +88,34 @@ 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;  
> > 
> > The define for this was removed in patch 1.  I'll fix that up whilst applying by
> > keeping it until this patch.  Please check build passes on intermediate points
> > during a patch series as otherwise we may break bisectability and that's really
> > annoying if you are bisecting!
> > 
> > Jonathan
> > 
> > >  	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:
> > > +		/* Here, type is big endian as it must be sent in two transfers */
> > > +		ad7949_adc->buffer = (u16)cpu_to_be16(ad7949_adc->cfg << 2);
>
> Gah, I wasn't thinking clearly when I suggested this. Sparse warns on
> the
> endian conversion
>
> One option is to resort to ignoring the fact we know it's aligned and
> use the put_unaligned_be16() and get_unaligned_be16 calls which sparse
> seems to be
> happy with. Alternative would be to just have a be16 buffer after the
> existing
> one in the iio_priv structure. Then you will have to change the various
> users
> of iio_priv()->buffer to point to the new value if we are doing 8 bit
> transfers.
>
> Whilst more invasive, this second option is the one I'd suggest.

Understood, I'll go with your suggestion.

Out of curiosity, other that being more explicit, is there another
we'd rather not use {get,put}_unaligned_be16()?

> Note that there will be no need to add an __cacheline_aligned marking to
> this
> new element because it will be in a cachline that is only used for DMA
> simply being
> after the other buffer element which is force to start on a new
> cacheline.

Noted, Thanks for taking the time to explaining this.

Liam

>
> Jonathan
>  
> > > +		break;
> > > +	default:
> > > +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > >  	spi_message_init_with_transfers(&msg, tx, 1);
> > >  	ret = spi_sync(ad7949_adc->spi, &msg);
> > >  
> > > @@ -115,14 +132,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,
> > >  		},
> > >  	};
> > >  
> > > @@ -157,7 +172,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->buffer);
> > > +		/* Shift-out padding bits */
> > > +		*val >>= 16 - ad7949_adc->resolution;
> > > +		break;
> > > +	default:
> > > +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> > > +		return -EINVAL;
> > > +	}
> > >  
> > >  	return 0;
> > >  }
> > > @@ -265,6 +298,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
> > >  
> > >  static int ad7949_spi_probe(struct spi_device *spi)
> > >  {
> > > +	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
> > >  	struct device *dev = &spi->dev;
> > >  	const struct ad7949_adc_spec *spec;
> > >  	struct ad7949_adc_chip *ad7949_adc;
> > > @@ -291,6 +325,18 @@ static int ad7949_spi_probe(struct spi_device *spi)
> > >  	indio_dev->num_channels = spec->num_channels;
> > >  	ad7949_adc->resolution = spec->resolution;
> > >  
> > > +	/* Set SPI bits per word */
> > > +	if (spi_ctrl_mask & SPI_BPW_MASK(ad7949_adc->resolution)) {
> > > +		ad7949_adc->bits_per_word = ad7949_adc->resolution;
> > > +	} else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
> > > +		ad7949_adc->bits_per_word = 16;
> > > +	} else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
> > > +		ad7949_adc->bits_per_word = 8;
> > > +	} else {
> > > +		dev_err(dev, "unable to find common BPW with spi controller\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > >  	ad7949_adc->vref = devm_regulator_get(dev, "vref");
> > >  	if (IS_ERR(ad7949_adc->vref)) {
> > >  		dev_err(dev, "fail to request regulator\n");  
> > 

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

[Jonathan Cameron]

On Sun, 01 Aug 2021 19:01:10 -0400
"Liam Beguin" <liambeguin@gmail.com> wrote:

> On Sat Jul 31, 2021 at 10:52 AM EDT, Jonathan Cameron wrote:
> > On Sat, 31 Jul 2021 15:29:21 +0100
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >  
> > > On Tue, 27 Jul 2021 19:29:03 -0400
> > > Liam Beguin <liambeguin@gmail.com> wrote:
> > >   
> > > > From: Liam Beguin <lvb@xiphos.com>
> > > > 
> > > > This driver supports devices with 14-bit and 16-bit sample sizes.
> > > > This is not always handled properly by spi controllers and can fail. To
> > > > work around this limitation, pad samples to 16-bit and split the sample
> > > > into two 8-bit messages in the event that only 8-bit messages are
> > > > supported by the controller.
> > > > 
> > > > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > > > ---
> > > >  drivers/iio/adc/ad7949.c | 62 ++++++++++++++++++++++++++++++++++------
> > > >  1 file changed, 54 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > > > index 0b549b8bd7a9..f1702c54c8be 100644
> > > > --- a/drivers/iio/adc/ad7949.c
> > > > +++ b/drivers/iio/adc/ad7949.c
> > > > @@ -67,6 +67,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
> > > >   * @indio_dev: reference to iio structure
> > > >   * @spi: reference to spi structure
> > > >   * @resolution: resolution of the chip
> > > > + * @bits_per_word: number of bits per SPI word
> > > >   * @cfg: copy of the configuration register
> > > >   * @current_channel: current channel in use
> > > >   * @buffer: buffer to send / receive data to / from device
> > > > @@ -77,6 +78,7 @@ struct ad7949_adc_chip {
> > > >  	struct iio_dev *indio_dev;
> > > >  	struct spi_device *spi;
> > > >  	u8 resolution;
> > > > +	u8 bits_per_word;
> > > >  	u16 cfg;
> > > >  	unsigned int current_channel;
> > > >  	u16 buffer ____cacheline_aligned;
> > > > @@ -86,19 +88,34 @@ 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;    
> > > 
> > > The define for this was removed in patch 1.  I'll fix that up whilst applying by
> > > keeping it until this patch.  Please check build passes on intermediate points
> > > during a patch series as otherwise we may break bisectability and that's really
> > > annoying if you are bisecting!
> > > 
> > > Jonathan
> > >   
> > > >  	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:
> > > > +		/* Here, type is big endian as it must be sent in two transfers */
> > > > +		ad7949_adc->buffer = (u16)cpu_to_be16(ad7949_adc->cfg << 2);  
> >
> > Gah, I wasn't thinking clearly when I suggested this. Sparse warns on
> > the
> > endian conversion
> >
> > One option is to resort to ignoring the fact we know it's aligned and
> > use the put_unaligned_be16() and get_unaligned_be16 calls which sparse
> > seems to be
> > happy with. Alternative would be to just have a be16 buffer after the
> > existing
> > one in the iio_priv structure. Then you will have to change the various
> > users
> > of iio_priv()->buffer to point to the new value if we are doing 8 bit
> > transfers.
> >
> > Whilst more invasive, this second option is the one I'd suggest.  
> 
> Understood, I'll go with your suggestion.
> 
> Out of curiosity, other that being more explicit, is there another
> we'd rather not use {get,put}_unaligned_be16()?

We know it is aligned (as u16) so on a big endian platform that happens
to not handle unaligned accesses we will now be doing work which wouldn't
be needed if there was a get_aligned_be16() that was more relaxed about
types than cpu_to_be16() etc.

So basically it looks odd and we will will be hiding that we are smashing
data of potentially different ordering into the same structure field.

> 
> > Note that there will be no need to add an __cacheline_aligned marking to
> > this
> > new element because it will be in a cachline that is only used for DMA
> > simply being
> > after the other buffer element which is force to start on a new
> > cacheline.  
> 
> Noted, Thanks for taking the time to explaining this.
> 
> Liam
> 
> >
> > Jonathan
> >    
> > > > +		break;
> > > > +	default:
> > > > +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > >  	spi_message_init_with_transfers(&msg, tx, 1);
> > > >  	ret = spi_sync(ad7949_adc->spi, &msg);
> > > >  
> > > > @@ -115,14 +132,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,
> > > >  		},
> > > >  	};
> > > >  
> > > > @@ -157,7 +172,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->buffer);
> > > > +		/* Shift-out padding bits */
> > > > +		*val >>= 16 - ad7949_adc->resolution;
> > > > +		break;
> > > > +	default:
> > > > +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > >  
> > > >  	return 0;
> > > >  }
> > > > @@ -265,6 +298,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
> > > >  
> > > >  static int ad7949_spi_probe(struct spi_device *spi)
> > > >  {
> > > > +	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
> > > >  	struct device *dev = &spi->dev;
> > > >  	const struct ad7949_adc_spec *spec;
> > > >  	struct ad7949_adc_chip *ad7949_adc;
> > > > @@ -291,6 +325,18 @@ static int ad7949_spi_probe(struct spi_device *spi)
> > > >  	indio_dev->num_channels = spec->num_channels;
> > > >  	ad7949_adc->resolution = spec->resolution;
> > > >  
> > > > +	/* Set SPI bits per word */
> > > > +	if (spi_ctrl_mask & SPI_BPW_MASK(ad7949_adc->resolution)) {
> > > > +		ad7949_adc->bits_per_word = ad7949_adc->resolution;
> > > > +	} else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
> > > > +		ad7949_adc->bits_per_word = 16;
> > > > +	} else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
> > > > +		ad7949_adc->bits_per_word = 8;
> > > > +	} else {
> > > > +		dev_err(dev, "unable to find common BPW with spi controller\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > >  	ad7949_adc->vref = devm_regulator_get(dev, "vref");
> > > >  	if (IS_ERR(ad7949_adc->vref)) {
> > > >  		dev_err(dev, "fail to request regulator\n");    
> > >   
> 

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

[Liam Beguin]

On Mon Aug 2, 2021 at 6:20 AM EDT, Jonathan Cameron wrote:
> On Sun, 01 Aug 2021 19:01:10 -0400
> "Liam Beguin" <liambeguin@gmail.com> wrote:
>
> > On Sat Jul 31, 2021 at 10:52 AM EDT, Jonathan Cameron wrote:
> > > On Sat, 31 Jul 2021 15:29:21 +0100
> > > Jonathan Cameron <jic23@kernel.org> wrote:
> > >  
> > > > On Tue, 27 Jul 2021 19:29:03 -0400
> > > > Liam Beguin <liambeguin@gmail.com> wrote:
> > > >   
> > > > > From: Liam Beguin <lvb@xiphos.com>
> > > > > 
> > > > > This driver supports devices with 14-bit and 16-bit sample sizes.
> > > > > This is not always handled properly by spi controllers and can fail. To
> > > > > work around this limitation, pad samples to 16-bit and split the sample
> > > > > into two 8-bit messages in the event that only 8-bit messages are
> > > > > supported by the controller.
> > > > > 
> > > > > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > > > > ---
> > > > >  drivers/iio/adc/ad7949.c | 62 ++++++++++++++++++++++++++++++++++------
> > > > >  1 file changed, 54 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > > > > index 0b549b8bd7a9..f1702c54c8be 100644
> > > > > --- a/drivers/iio/adc/ad7949.c
> > > > > +++ b/drivers/iio/adc/ad7949.c
> > > > > @@ -67,6 +67,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
> > > > >   * @indio_dev: reference to iio structure
> > > > >   * @spi: reference to spi structure
> > > > >   * @resolution: resolution of the chip
> > > > > + * @bits_per_word: number of bits per SPI word
> > > > >   * @cfg: copy of the configuration register
> > > > >   * @current_channel: current channel in use
> > > > >   * @buffer: buffer to send / receive data to / from device
> > > > > @@ -77,6 +78,7 @@ struct ad7949_adc_chip {
> > > > >  	struct iio_dev *indio_dev;
> > > > >  	struct spi_device *spi;
> > > > >  	u8 resolution;
> > > > > +	u8 bits_per_word;
> > > > >  	u16 cfg;
> > > > >  	unsigned int current_channel;
> > > > >  	u16 buffer ____cacheline_aligned;
> > > > > @@ -86,19 +88,34 @@ 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;    
> > > > 
> > > > The define for this was removed in patch 1.  I'll fix that up whilst applying by
> > > > keeping it until this patch.  Please check build passes on intermediate points
> > > > during a patch series as otherwise we may break bisectability and that's really
> > > > annoying if you are bisecting!
> > > > 
> > > > Jonathan
> > > >   
> > > > >  	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:
> > > > > +		/* Here, type is big endian as it must be sent in two transfers */
> > > > > +		ad7949_adc->buffer = (u16)cpu_to_be16(ad7949_adc->cfg << 2);  
> > >
> > > Gah, I wasn't thinking clearly when I suggested this. Sparse warns on
> > > the
> > > endian conversion
> > >
> > > One option is to resort to ignoring the fact we know it's aligned and
> > > use the put_unaligned_be16() and get_unaligned_be16 calls which sparse
> > > seems to be
> > > happy with. Alternative would be to just have a be16 buffer after the
> > > existing
> > > one in the iio_priv structure. Then you will have to change the various
> > > users
> > > of iio_priv()->buffer to point to the new value if we are doing 8 bit
> > > transfers.
> > >
> > > Whilst more invasive, this second option is the one I'd suggest.  
> > 
> > Understood, I'll go with your suggestion.
> > 
> > Out of curiosity, other that being more explicit, is there another
> > we'd rather not use {get,put}_unaligned_be16()?
>
> We know it is aligned (as u16) so on a big endian platform that happens
> to not handle unaligned accesses we will now be doing work which
> wouldn't
> be needed if there was a get_aligned_be16() that was more relaxed about
> types than cpu_to_be16() etc.
>
> So basically it looks odd and we will will be hiding that we are
> smashing
> data of potentially different ordering into the same structure field.

Got it! Thanks for the explanation.

Liam

>
> > 
> > > Note that there will be no need to add an __cacheline_aligned marking to
> > > this
> > > new element because it will be in a cachline that is only used for DMA
> > > simply being
> > > after the other buffer element which is force to start on a new
> > > cacheline.  
> > 
> > Noted, Thanks for taking the time to explaining this.
> > 
> > Liam
> > 
> > >
> > > Jonathan
> > >    
> > > > > +		break;
> > > > > +	default:
> > > > > +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > >  	spi_message_init_with_transfers(&msg, tx, 1);
> > > > >  	ret = spi_sync(ad7949_adc->spi, &msg);
> > > > >  
> > > > > @@ -115,14 +132,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,
> > > > >  		},
> > > > >  	};
> > > > >  
> > > > > @@ -157,7 +172,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->buffer);
> > > > > +		/* Shift-out padding bits */
> > > > > +		*val >>= 16 - ad7949_adc->resolution;
> > > > > +		break;
> > > > > +	default:
> > > > > +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > >  
> > > > >  	return 0;
> > > > >  }
> > > > > @@ -265,6 +298,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
> > > > >  
> > > > >  static int ad7949_spi_probe(struct spi_device *spi)
> > > > >  {
> > > > > +	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
> > > > >  	struct device *dev = &spi->dev;
> > > > >  	const struct ad7949_adc_spec *spec;
> > > > >  	struct ad7949_adc_chip *ad7949_adc;
> > > > > @@ -291,6 +325,18 @@ static int ad7949_spi_probe(struct spi_device *spi)
> > > > >  	indio_dev->num_channels = spec->num_channels;
> > > > >  	ad7949_adc->resolution = spec->resolution;
> > > > >  
> > > > > +	/* Set SPI bits per word */
> > > > > +	if (spi_ctrl_mask & SPI_BPW_MASK(ad7949_adc->resolution)) {
> > > > > +		ad7949_adc->bits_per_word = ad7949_adc->resolution;
> > > > > +	} else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
> > > > > +		ad7949_adc->bits_per_word = 16;
> > > > > +	} else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
> > > > > +		ad7949_adc->bits_per_word = 8;
> > > > > +	} else {
> > > > > +		dev_err(dev, "unable to find common BPW with spi controller\n");
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > >  	ad7949_adc->vref = devm_regulator_get(dev, "vref");
> > > > >  	if (IS_ERR(ad7949_adc->vref)) {
> > > > >  		dev_err(dev, "fail to request regulator\n");    
> > > >   
> > 

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

[Rob Herring]

On Tue, 27 Jul 2021 19:29:05 -0400, Liam Beguin wrote:
> From: Liam Beguin <lvb@xiphos.com>
> 
> Add bindings documentation describing per channel reference voltage
> selection.
> This adds the adi,internal-ref-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>
> ---
>  .../bindings/iio/adc/adi,ad7949.yaml          | 69 +++++++++++++++++--
>  1 file changed, 65 insertions(+), 4 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>