Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Liam Beguin <liambeguin@gmail.com>
To:     liambeguin@gmail.com, lars@metafoo.de,
        Michael.Hennerich@analog.com, jic23@kernel.org,
        charles-antoine.couret@essensium.com, Nuno.Sa@analog.com
Cc:     linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
        devicetree@vger.kernel.org, robh+dt@kernel.org
Subject: [PATCH v3 0/4] AD7949 Fixes
Date:   Tue, 13 Jul 2021 00:34:21 -0400
Message-Id: <20210713043425.3321230-1-liambeguin@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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

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

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

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

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

Thanks for your time,
Liam

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

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

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

base-commit: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
-- 
2.30.1.489.g328c10930387