[PATCH v6 0/5] AD7949 Fixes

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

v6 drops support for per channel vref selection.
After switching the voltage reference, readings take a little while to
stabilize, invalidating consecutive readings.

This could've been addressed by adding more dummy cycles at the expense
of speed, but discussing the issue with colleagues more involved in
hardware design, it turns out these circuits are usually designed with a
single vref in mind.

[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 v5:
- rename defines: s/AD7949_CFG_BIT_/AD7949_CFG_MASK_/g
- rename AD7949_MASK_TOTAL to match other defines
- make vref selection global instead of per channel, and update
  dt-bindings
- reword commits 2/5, 3/5, and 4/5
- move bits_per_word configuration to struct spi_device, and switch to
  spi_{read,write}.

Changes since v4:
- fix premature deletion of define
- use separate be16 buffer for 8-bit transfers
- switch to devm_regulator_get_optional()
- fix vref setup
- apply Reviewed-by

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

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

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

Thanks for your time,
Liam

Liam Beguin (5):
  iio: adc: ad7949: define and use bitfield names
  iio: adc: ad7949: fix spi messages on non 14-bit controllers
  iio: adc: ad7949: add vref selection support
  dt-bindings: iio: adc: ad7949: update voltage reference bindings
  iio: adc: ad7949: use devm managed functions

 .../bindings/iio/adc/adi,ad7949.yaml          |  51 +++-
 drivers/iio/adc/ad7949.c                      | 258 +++++++++++++-----
 2 files changed, 232 insertions(+), 77 deletions(-)

Range-diff against v5:
1:  a5c211185661 ! 1:  a8d849189b6e iio: adc: ad7949: define and use bitfield names
    @@ drivers/iio/adc/ad7949.c
     +#include <linux/bitfield.h>
      
     -#define AD7949_MASK_CHANNEL_SEL		GENMASK(9, 7)
    - #define AD7949_MASK_TOTAL		GENMASK(13, 0)
    +-#define AD7949_MASK_TOTAL		GENMASK(13, 0)
     -#define AD7949_OFFSET_CHANNEL_SEL	7
     -#define AD7949_CFG_READ_BACK		0x1
    ++#define AD7949_CFG_MASK_TOTAL		GENMASK(13, 0)
      #define AD7949_CFG_REG_SIZE_BITS	14
      
     +/* CFG: Configuration Update */
    -+#define AD7949_CFG_BIT_OVERWRITE	BIT(13)
    ++#define AD7949_CFG_MASK_OVERWRITE	BIT(13)
     +
     +/* INCC: Input Channel Configuration */
    -+#define AD7949_CFG_BIT_INCC		GENMASK(12, 10)
    ++#define AD7949_CFG_MASK_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
    @@ drivers/iio/adc/ad7949.c
     +#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_MASK_INX		GENMASK(9, 7)
     +
     +/* BW: select bandwidth for low-pass filter. Full or Quarter */
    -+#define AD7949_CFG_BIT_BW_FULL			BIT(6)
    ++#define AD7949_CFG_MASK_BW_FULL		BIT(6)
     +
     +/* REF: reference/buffer selection */
    -+#define AD7949_CFG_BIT_REF		GENMASK(5, 3)
    ++#define AD7949_CFG_MASK_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_MASK_SEQ		GENMASK(2, 1)
     +
     +/* RB: Read back the CFG register */
    -+#define AD7949_CFG_BIT_RBN		BIT(0)
    ++#define AD7949_CFG_MASK_RBN		BIT(0)
     +
      enum {
      	ID_AD7949 = 0,
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_c
      		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);
    ++					   FIELD_PREP(AD7949_CFG_MASK_INX, channel),
    ++					   AD7949_CFG_MASK_INX);
      		if (ret)
      			return ret;
      		if (channel == ad7949_adc->current_channel)
    +@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_reg_access(struct iio_dev *indio_dev,
    + 	if (readval)
    + 		*readval = ad7949_adc->cfg;
    + 	else
    +-		ret = ad7949_spi_write_cfg(ad7949_adc,
    +-			writeval & AD7949_MASK_TOTAL, AD7949_MASK_TOTAL);
    ++		ret = ad7949_spi_write_cfg(ad7949_adc, writeval,
    ++					   AD7949_CFG_MASK_TOTAL);
    + 
    + 	return ret;
    + }
     @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
      {
      	int ret;
    @@ 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_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);
    ++	cfg = FIELD_PREP(AD7949_CFG_MASK_OVERWRITE, 1) |
    ++		FIELD_PREP(AD7949_CFG_MASK_INCC, AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
    ++		FIELD_PREP(AD7949_CFG_MASK_INX, ad7949_adc->current_channel) |
    ++		FIELD_PREP(AD7949_CFG_MASK_BW_FULL, 1) |
    ++		FIELD_PREP(AD7949_CFG_MASK_REF, AD7949_CFG_VAL_REF_EXT_BUF) |
    ++		FIELD_PREP(AD7949_CFG_MASK_SEQ, 0x0) |
    ++		FIELD_PREP(AD7949_CFG_MASK_RBN, 1);
     +
    -+	ret = ad7949_spi_write_cfg(ad7949_adc, cfg, AD7949_MASK_TOTAL);
    ++	ret = ad7949_spi_write_cfg(ad7949_adc, cfg, AD7949_CFG_MASK_TOTAL);
      
      	/*
      	 * Do two dummy conversions to apply the first configuration setting.
2:  df2f6df8f3d5 ! 2:  dfa817a7c51e iio: adc: ad7949: fix spi messages on non 14-bit controllers
    @@ Commit message
         iio: adc: ad7949: fix spi messages on non 14-bit controllers
     
         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.
    +    This implies different SPI transfer lengths which are not always handled
    +    properly by some SPI controllers.
    +
    +    To work around this limitation, define a big endian buffer used to split
    +    the buffer into two 8-bit messages in the event that the controller
    +    doesn't support 14-bit or 16-bit transfers.
    +    A separate buffer is introduced here to avoid performing operations on
    +    types of different endianness.
    +
    +    Since all transfers use the same bits_per_word value, move that logic to
    +    the probe function, and let transfers default to the value defined in
    +    the struct spi_device.
     
         Signed-off-by: Liam Beguin <lvb@xiphos.com>
     
    @@ drivers/iio/adc/ad7949.c
     @@
      #include <linux/bitfield.h>
      
    - #define AD7949_MASK_TOTAL		GENMASK(13, 0)
    + #define AD7949_CFG_MASK_TOTAL		GENMASK(13, 0)
     -#define AD7949_CFG_REG_SIZE_BITS	14
      
      /* CFG: Configuration Update */
    - #define AD7949_CFG_BIT_OVERWRITE	BIT(13)
    + #define AD7949_CFG_MASK_OVERWRITE	BIT(13)
     @@ drivers/iio/adc/ad7949.c: static const struct ad7949_adc_spec ad7949_adc_spec[] = {
    -  * @indio_dev: reference to iio structure
    -  * @spi: reference to spi structure
    -  * @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
    @@ drivers/iio/adc/ad7949.c: static const struct ad7949_adc_spec ad7949_adc_spec[]
      struct ad7949_adc_chip {
      	struct mutex lock;
     @@ drivers/iio/adc/ad7949.c: struct ad7949_adc_chip {
    - 	struct iio_dev *indio_dev;
    - 	struct spi_device *spi;
    - 	u8 resolution;
    -+	u8 bits_per_word;
      	u16 cfg;
      	unsigned int current_channel;
      	u16 buffer ____cacheline_aligned;
    @@ drivers/iio/adc/ad7949.c: struct ad7949_adc_chip {
      	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[] = {
    - 		{
    +-	struct spi_message msg;
    +-	struct spi_transfer tx[] = {
    +-		{
     -			.tx_buf = &ad7949_adc->buffer,
    - 			.len = 2,
    +-			.len = 2,
     -			.bits_per_word = bits_per_word,
    -+			.bits_per_word = ad7949_adc->bits_per_word,
    - 		},
    - 	};
    +-		},
    +-	};
      
      	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
     -	ad7949_adc->buffer = ad7949_adc->cfg << shift;
    +-	spi_message_init_with_transfers(&msg, tx, 1);
    +-	ret = spi_sync(ad7949_adc->spi, &msg);
     +
    -+	switch (ad7949_adc->bits_per_word) {
    ++	switch (ad7949_adc->spi->bits_per_word) {
     +	case 16:
    -+		tx[0].tx_buf = &ad7949_adc->buffer;
     +		ad7949_adc->buffer = ad7949_adc->cfg << 2;
    ++		ret = spi_write(ad7949_adc->spi, &ad7949_adc->buffer, 2);
     +		break;
     +	case 14:
    -+		tx[0].tx_buf = &ad7949_adc->buffer;
     +		ad7949_adc->buffer = ad7949_adc->cfg;
    ++		ret = spi_write(ad7949_adc->spi, &ad7949_adc->buffer, 2);
     +		break;
     +	case 8:
     +		/* Here, type is big endian as it must be sent in two transfers */
    -+		tx[0].tx_buf = &ad7949_adc->buf8b;
     +		ad7949_adc->buf8b = cpu_to_be16(ad7949_adc->cfg << 2);
    ++		ret = spi_write(ad7949_adc->spi, &ad7949_adc->buf8b, 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);
      
    + 	/*
    + 	 * This delay is to avoid a new request before the required time to
     @@ drivers/iio/adc/ad7949.c: 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[] = {
    - 		{
    +-	struct spi_message msg;
    +-	struct spi_transfer tx[] = {
    +-		{
     -			.rx_buf = &ad7949_adc->buffer,
    - 			.len = 2,
    +-			.len = 2,
     -			.bits_per_word = bits_per_word,
    -+			.bits_per_word = ad7949_adc->bits_per_word,
    - 		},
    - 	};
    +-		},
    +-	};
      
    -+	if (ad7949_adc->bits_per_word == 8)
    -+		tx[0].rx_buf = &ad7949_adc->buf8b;
    -+	else
    -+		tx[0].rx_buf = &ad7949_adc->buffer;
    -+
      	/*
      	 * 1: write CFG for sample N and read old data (sample N-2)
    - 	 * 2: if CFG was not changed since sample N-1 then we'll get good data
     @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
      	}
      
      	/* 3: write something and read actual data */
     -	ad7949_adc->buffer = 0;
    - 	spi_message_init_with_transfers(&msg, tx, 1);
    - 	ret = spi_sync(ad7949_adc->spi, &msg);
    +-	spi_message_init_with_transfers(&msg, tx, 1);
    +-	ret = spi_sync(ad7949_adc->spi, &msg);
    ++	if (ad7949_adc->spi->bits_per_word == 8)
    ++		ret = spi_read(ad7949_adc->spi, &ad7949_adc->buf8b, 2);
    ++	else
    ++		ret = spi_read(ad7949_adc->spi, &ad7949_adc->buffer, 2);
    ++
      	if (ret)
    + 		return ret;
    + 
     @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
      
      	ad7949_adc->current_channel = channel;
      
     -	*val = ad7949_adc->buffer & mask;
    -+	switch (ad7949_adc->bits_per_word) {
    ++	switch (ad7949_adc->spi->bits_per_word) {
     +	case 16:
     +		*val = ad7949_adc->buffer;
     +		/* Shift-out padding bits */
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_probe(struct spi_device *spi)
      
     +	/* Set SPI bits per word */
     +	if (spi_ctrl_mask & SPI_BPW_MASK(ad7949_adc->resolution)) {
    -+		ad7949_adc->bits_per_word = ad7949_adc->resolution;
    ++		spi->bits_per_word = ad7949_adc->resolution;
     +	} else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
    -+		ad7949_adc->bits_per_word = 16;
    ++		spi->bits_per_word = 16;
     +	} else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
    -+		ad7949_adc->bits_per_word = 8;
    ++		spi->bits_per_word = 8;
     +	} else {
     +		dev_err(dev, "unable to find common BPW with spi controller\n");
     +		return -EINVAL;
3:  8a33618a4f90 ! 3:  aae4f239db77 iio: adc: ad7949: add support for internal vref
    @@ Metadata
     Author: Liam Beguin <lvb@xiphos.com>
     
      ## Commit message ##
    -    iio: adc: ad7949: add support for internal vref
    +    iio: adc: ad7949: add vref selection support
     
    -    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.
    +    Add support for selecting the voltage reference from the devicetree.
    +
    +    This change is required to get valid readings with all three
    +    vref hardware configurations supported by the ADC.
    +
    +    For instance if the ADC isn't provided with an external reference,
    +    the sample request must specify an internal voltage reference to get a
    +    valid reading.
     
         Signed-off-by: Liam Beguin <lvb@xiphos.com>
     
    @@ drivers/iio/adc/ad7949.c
     @@
      
      /* REF: reference/buffer selection */
    - #define AD7949_CFG_BIT_REF		GENMASK(5, 3)
    + #define AD7949_CFG_MASK_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
    @@ drivers/iio/adc/ad7949.c
     +#define AD7949_CFG_VAL_REF_EXTERNAL		BIT(1)
      
      /* SEQ: channel sequencer. Allows for scanning channels */
    - #define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
    + #define AD7949_CFG_MASK_SEQ		GENMASK(2, 1)
     @@ drivers/iio/adc/ad7949.c: static const struct ad7949_adc_spec ad7949_adc_spec[] = {
    - 	[ID_AD7689] = { .num_channels = 8, .resolution = 16 },
    - };
    - 
    -+/**
    -+ * struct ad7949_channel - ADC Channel parameters
    +  * @vref: regulator generating Vref
    +  * @indio_dev: reference to iio structure
    +  * @spi: reference to spi structure
     + * @refsel: reference selection
    -+ */
    -+struct ad7949_channel {
    -+	u32 refsel;
    -+};
    -+
    - /**
    -  * struct ad7949_adc_chip - AD ADC chip
    -  * @lock: protects write sequences
    +  * @resolution: resolution of the chip
    +  * @cfg: copy of the configuration register
    +  * @current_channel: current channel in use
     @@ drivers/iio/adc/ad7949.c: struct ad7949_adc_chip {
      	struct regulator *vref;
      	struct iio_dev *indio_dev;
      	struct spi_device *spi;
    -+	struct ad7949_channel *channels;
    ++	u32 refsel;
      	u8 resolution;
    - 	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;
    - 	struct spi_message msg;
    -+	struct ad7949_channel *ad7949_chan = &ad7949_adc->channels[channel];
    - 	struct spi_transfer tx[] = {
    - 		{
    - 			.len = 2,
    -@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
    - 	 */
    - 	for (i = 0; i < 2; i++) {
    - 		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)
    -@@ drivers/iio/adc/ad7949.c: 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)
    + 	unsigned int current_channel;
     @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
      		return IIO_VAL_INT;
      
    @@ 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_adc->refsel) {
     +		case AD7949_CFG_VAL_REF_INT_2500:
     +			*val = 2500;
     +			break;
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_raw(struct iio_dev *indio_d
      
      	return -EINVAL;
     @@ drivers/iio/adc/ad7949.c: 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);
    + 		FIELD_PREP(AD7949_CFG_MASK_INCC, AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
    + 		FIELD_PREP(AD7949_CFG_MASK_INX, ad7949_adc->current_channel) |
    + 		FIELD_PREP(AD7949_CFG_MASK_BW_FULL, 1) |
    +-		FIELD_PREP(AD7949_CFG_MASK_REF, AD7949_CFG_VAL_REF_EXT_BUF) |
    ++		FIELD_PREP(AD7949_CFG_MASK_REF, ad7949_adc->refsel) |
    + 		FIELD_PREP(AD7949_CFG_MASK_SEQ, 0x0) |
    + 		FIELD_PREP(AD7949_CFG_MASK_RBN, 1);
      
     @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
      	return ret;
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_init(struct ad7949_adc_chip *ad7
      static int ad7949_spi_probe(struct spi_device *spi)
      {
      	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
    - 	struct device *dev = &spi->dev;
    +@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_probe(struct spi_device *spi)
      	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) {
     @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_probe(struct spi_device *spi)
      		return -EINVAL;
      	}
      
     -	ad7949_adc->vref = devm_regulator_get(dev, "vref");
    -+	/* Setup external voltage ref, buffered? */
    ++	/* Setup internal voltage reference */
    ++	tmp = 4096000;
    ++	ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);
    ++	if (ret < 0 && ret != -EINVAL) {
    ++		dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
    ++		return ret;
    ++	}
    ++
    ++	switch (tmp) {
    ++	case 2500000:
    ++		ad7949_adc->refsel = AD7949_CFG_VAL_REF_INT_2500;
    ++		break;
    ++	case 4096000:
    ++		ad7949_adc->refsel = AD7949_CFG_VAL_REF_INT_4096;
    ++		break;
    ++	default:
    ++		dev_err(dev, "unsupported internal voltage reference\n");
    ++		return -EINVAL;
    ++	}
    ++
    ++	/* Setup external voltage reference, buffered? */
     +	ad7949_adc->vref = devm_regulator_get_optional(dev, "vrefin");
      	if (IS_ERR(ad7949_adc->vref)) {
     -		dev_err(dev, "fail to request regulator\n");
    @@ drivers/iio/adc/ad7949.c: static int ad7949_spi_probe(struct spi_device *spi)
     +			ret = PTR_ERR(ad7949_adc->vref);
     +			if (ret != -ENODEV)
     +				return ret;
    -+			/* Internal then */
    -+			mode = AD7949_CFG_VAL_REF_INT_4096;
     +		} else {
    -+			mode = AD7949_CFG_VAL_REF_EXT_TEMP;
    ++			ad7949_adc->refsel = AD7949_CFG_VAL_REF_EXT_TEMP;
     +		}
     +	} else {
    -+		mode = AD7949_CFG_VAL_REF_EXT_TEMP_BUF;
    ++		ad7949_adc->refsel = 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) {
    ++	if (ad7949_adc->refsel & 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)
     +					       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);
4:  7612ff29db6b < -:  ------------ dt-bindings: iio: adc: ad7949: add per channel reference
-:  ------------ > 4:  b0f21366439b dt-bindings: iio: adc: ad7949: update voltage reference bindings
5:  74ee82caba57 = 5:  fb2347e306dd iio: adc: ad7949: use devm managed functions

base-commit: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
-- 
2.32.0.452.g940fe202adcb

[PATCH v6 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 | 55 ++++++++++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 1b4b3203e428..adc4487a7d56 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -11,13 +11,39 @@
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
+#include <linux/bitfield.h>
 
-#define AD7949_MASK_CHANNEL_SEL		GENMASK(9, 7)
-#define AD7949_MASK_TOTAL		GENMASK(13, 0)
-#define AD7949_OFFSET_CHANNEL_SEL	7
-#define AD7949_CFG_READ_BACK		0x1
+#define AD7949_CFG_MASK_TOTAL		GENMASK(13, 0)
 #define AD7949_CFG_REG_SIZE_BITS	14
 
+/* CFG: Configuration Update */
+#define AD7949_CFG_MASK_OVERWRITE	BIT(13)
+
+/* INCC: Input Channel Configuration */
+#define AD7949_CFG_MASK_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_MASK_INX		GENMASK(9, 7)
+
+/* BW: select bandwidth for low-pass filter. Full or Quarter */
+#define AD7949_CFG_MASK_BW_FULL		BIT(6)
+
+/* REF: reference/buffer selection */
+#define AD7949_CFG_MASK_REF		GENMASK(5, 3)
+#define AD7949_CFG_VAL_REF_EXT_BUF		7
+
+/* SEQ: channel sequencer. Allows for scanning channels */
+#define AD7949_CFG_MASK_SEQ		GENMASK(2, 1)
+
+/* RB: Read back the CFG register */
+#define AD7949_CFG_MASK_RBN		BIT(0)
+
 enum {
 	ID_AD7949 = 0,
 	ID_AD7682,
@@ -109,8 +135,8 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	 */
 	for (i = 0; i < 2; i++) {
 		ret = ad7949_spi_write_cfg(ad7949_adc,
-					   channel << AD7949_OFFSET_CHANNEL_SEL,
-					   AD7949_MASK_CHANNEL_SEL);
+					   FIELD_PREP(AD7949_CFG_MASK_INX, channel),
+					   AD7949_CFG_MASK_INX);
 		if (ret)
 			return ret;
 		if (channel == ad7949_adc->current_channel)
@@ -199,8 +225,8 @@ static int ad7949_spi_reg_access(struct iio_dev *indio_dev,
 	if (readval)
 		*readval = ad7949_adc->cfg;
 	else
-		ret = ad7949_spi_write_cfg(ad7949_adc,
-			writeval & AD7949_MASK_TOTAL, AD7949_MASK_TOTAL);
+		ret = ad7949_spi_write_cfg(ad7949_adc, writeval,
+					   AD7949_CFG_MASK_TOTAL);
 
 	return ret;
 }
@@ -214,10 +240,19 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 {
 	int ret;
 	int val;
+	u16 cfg;
 
-	/* Sequencer disabled, CFG readback disabled, IN0 as default channel */
 	ad7949_adc->current_channel = 0;
-	ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
+
+	cfg = FIELD_PREP(AD7949_CFG_MASK_OVERWRITE, 1) |
+		FIELD_PREP(AD7949_CFG_MASK_INCC, AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
+		FIELD_PREP(AD7949_CFG_MASK_INX, ad7949_adc->current_channel) |
+		FIELD_PREP(AD7949_CFG_MASK_BW_FULL, 1) |
+		FIELD_PREP(AD7949_CFG_MASK_REF, AD7949_CFG_VAL_REF_EXT_BUF) |
+		FIELD_PREP(AD7949_CFG_MASK_SEQ, 0x0) |
+		FIELD_PREP(AD7949_CFG_MASK_RBN, 1);
+
+	ret = ad7949_spi_write_cfg(ad7949_adc, cfg, AD7949_CFG_MASK_TOTAL);
 
 	/*
 	 * Do two dummy conversions to apply the first configuration setting.
-- 
2.32.0.452.g940fe202adcb

[PATCH v6 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 implies different SPI transfer lengths which are not always handled
properly by some SPI controllers.

To work around this limitation, define a big endian buffer used to split
the buffer into two 8-bit messages in the event that the controller
doesn't support 14-bit or 16-bit transfers.
A separate buffer is introduced here to avoid performing operations on
types of different endianness.

Since all transfers use the same bits_per_word value, move that logic to
the probe function, and let transfers default to the value defined in
the struct spi_device.

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

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index adc4487a7d56..a263d0fcec75 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -14,7 +14,6 @@
 #include <linux/bitfield.h>
 
 #define AD7949_CFG_MASK_TOTAL		GENMASK(13, 0)
-#define AD7949_CFG_REG_SIZE_BITS	14
 
 /* CFG: Configuration Update */
 #define AD7949_CFG_MASK_OVERWRITE	BIT(13)
@@ -71,6 +70,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
  * @cfg: copy of the configuration register
  * @current_channel: current channel in use
  * @buffer: buffer to send / receive data to / from device
+ * @buf8b: be16 buffer to exchange data with the device in 8-bit transfers
  */
 struct ad7949_adc_chip {
 	struct mutex lock;
@@ -81,27 +81,34 @@ struct ad7949_adc_chip {
 	u16 cfg;
 	unsigned int current_channel;
 	u16 buffer ____cacheline_aligned;
+	__be16 buf8b;
 };
 
 static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
 				u16 mask)
 {
 	int ret;
-	int bits_per_word = ad7949_adc->resolution;
-	int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
-	struct spi_message msg;
-	struct spi_transfer tx[] = {
-		{
-			.tx_buf = &ad7949_adc->buffer,
-			.len = 2,
-			.bits_per_word = bits_per_word,
-		},
-	};
 
 	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
-	ad7949_adc->buffer = ad7949_adc->cfg << shift;
-	spi_message_init_with_transfers(&msg, tx, 1);
-	ret = spi_sync(ad7949_adc->spi, &msg);
+
+	switch (ad7949_adc->spi->bits_per_word) {
+	case 16:
+		ad7949_adc->buffer = ad7949_adc->cfg << 2;
+		ret = spi_write(ad7949_adc->spi, &ad7949_adc->buffer, 2);
+		break;
+	case 14:
+		ad7949_adc->buffer = ad7949_adc->cfg;
+		ret = spi_write(ad7949_adc->spi, &ad7949_adc->buffer, 2);
+		break;
+	case 8:
+		/* Here, type is big endian as it must be sent in two transfers */
+		ad7949_adc->buf8b = cpu_to_be16(ad7949_adc->cfg << 2);
+		ret = spi_write(ad7949_adc->spi, &ad7949_adc->buf8b, 2);
+		break;
+	default:
+		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
+		return -EINVAL;
+	}
 
 	/*
 	 * This delay is to avoid a new request before the required time to
@@ -116,16 +123,6 @@ 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,
-		},
-	};
 
 	/*
 	 * 1: write CFG for sample N and read old data (sample N-2)
@@ -144,9 +141,11 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	}
 
 	/* 3: write something and read actual data */
-	ad7949_adc->buffer = 0;
-	spi_message_init_with_transfers(&msg, tx, 1);
-	ret = spi_sync(ad7949_adc->spi, &msg);
+	if (ad7949_adc->spi->bits_per_word == 8)
+		ret = spi_read(ad7949_adc->spi, &ad7949_adc->buf8b, 2);
+	else
+		ret = spi_read(ad7949_adc->spi, &ad7949_adc->buffer, 2);
+
 	if (ret)
 		return ret;
 
@@ -158,7 +157,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->spi->bits_per_word) {
+	case 16:
+		*val = ad7949_adc->buffer;
+		/* Shift-out padding bits */
+		*val >>= 16 - ad7949_adc->resolution;
+		break;
+	case 14:
+		*val = ad7949_adc->buffer & GENMASK(13, 0);
+		break;
+	case 8:
+		/* Here, type is big endian as data was sent in two transfers */
+		*val = be16_to_cpu(ad7949_adc->buf8b);
+		/* Shift-out padding bits */
+		*val >>= 16 - ad7949_adc->resolution;
+		break;
+	default:
+		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
+		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -266,6 +283,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 
 static int ad7949_spi_probe(struct spi_device *spi)
 {
+	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
 	struct device *dev = &spi->dev;
 	const struct ad7949_adc_spec *spec;
 	struct ad7949_adc_chip *ad7949_adc;
@@ -292,6 +310,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)) {
+		spi->bits_per_word = ad7949_adc->resolution;
+	} else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
+		spi->bits_per_word = 16;
+	} else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
+		spi->bits_per_word = 8;
+	} else {
+		dev_err(dev, "unable to find common BPW with spi controller\n");
+		return -EINVAL;
+	}
+
 	ad7949_adc->vref = devm_regulator_get(dev, "vref");
 	if (IS_ERR(ad7949_adc->vref)) {
 		dev_err(dev, "fail to request regulator\n");
-- 
2.32.0.452.g940fe202adcb

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

[Jonathan Cameron]

On Sun, 15 Aug 2021 17:33:06 -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 implies different SPI transfer lengths which are not always handled
> properly by some SPI controllers.
> 
> To work around this limitation, define a big endian buffer used to split
> the buffer into two 8-bit messages in the event that the controller
> doesn't support 14-bit or 16-bit transfers.
> A separate buffer is introduced here to avoid performing operations on
> types of different endianness.
> 
> Since all transfers use the same bits_per_word value, move that logic to
> the probe function, and let transfers default to the value defined in
> the struct spi_device.
> 
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
Hi Liam, 

I changed the title of this one to avoid it being picked up for automated
backporting.  I don't mind if you want to request it is backported explicitly
but it isn't a regression fix (as it never worked on such controllers) and
is non trivial.  As a result I want a backport to be a deliberate decision.
Now titled "iio: adc: ad7949: enable use with non 14/16-bit controllers"

This and previous applied to the togreg branch of iio.git and pushed out
as testing for 0-day to poke at it.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ad7949.c | 86 +++++++++++++++++++++++++++-------------
>  1 file changed, 58 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index adc4487a7d56..a263d0fcec75 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -14,7 +14,6 @@
>  #include <linux/bitfield.h>
>  
>  #define AD7949_CFG_MASK_TOTAL		GENMASK(13, 0)
> -#define AD7949_CFG_REG_SIZE_BITS	14
>  
>  /* CFG: Configuration Update */
>  #define AD7949_CFG_MASK_OVERWRITE	BIT(13)
> @@ -71,6 +70,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
>   * @cfg: copy of the configuration register
>   * @current_channel: current channel in use
>   * @buffer: buffer to send / receive data to / from device
> + * @buf8b: be16 buffer to exchange data with the device in 8-bit transfers
>   */
>  struct ad7949_adc_chip {
>  	struct mutex lock;
> @@ -81,27 +81,34 @@ struct ad7949_adc_chip {
>  	u16 cfg;
>  	unsigned int current_channel;
>  	u16 buffer ____cacheline_aligned;
> +	__be16 buf8b;
>  };
>  
>  static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
>  				u16 mask)
>  {
>  	int ret;
> -	int bits_per_word = ad7949_adc->resolution;
> -	int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
> -	struct spi_message msg;
> -	struct spi_transfer tx[] = {
> -		{
> -			.tx_buf = &ad7949_adc->buffer,
> -			.len = 2,
> -			.bits_per_word = bits_per_word,
> -		},
> -	};
>  
>  	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
> -	ad7949_adc->buffer = ad7949_adc->cfg << shift;
> -	spi_message_init_with_transfers(&msg, tx, 1);
> -	ret = spi_sync(ad7949_adc->spi, &msg);
> +
> +	switch (ad7949_adc->spi->bits_per_word) {
> +	case 16:
> +		ad7949_adc->buffer = ad7949_adc->cfg << 2;
> +		ret = spi_write(ad7949_adc->spi, &ad7949_adc->buffer, 2);
> +		break;
> +	case 14:
> +		ad7949_adc->buffer = ad7949_adc->cfg;
> +		ret = spi_write(ad7949_adc->spi, &ad7949_adc->buffer, 2);
> +		break;
> +	case 8:
> +		/* Here, type is big endian as it must be sent in two transfers */
> +		ad7949_adc->buf8b = cpu_to_be16(ad7949_adc->cfg << 2);
> +		ret = spi_write(ad7949_adc->spi, &ad7949_adc->buf8b, 2);
> +		break;
> +	default:
> +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> +		return -EINVAL;
> +	}
>  
>  	/*
>  	 * This delay is to avoid a new request before the required time to
> @@ -116,16 +123,6 @@ 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,
> -		},
> -	};
>  
>  	/*
>  	 * 1: write CFG for sample N and read old data (sample N-2)
> @@ -144,9 +141,11 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>  	}
>  
>  	/* 3: write something and read actual data */
> -	ad7949_adc->buffer = 0;
> -	spi_message_init_with_transfers(&msg, tx, 1);
> -	ret = spi_sync(ad7949_adc->spi, &msg);
> +	if (ad7949_adc->spi->bits_per_word == 8)
> +		ret = spi_read(ad7949_adc->spi, &ad7949_adc->buf8b, 2);
> +	else
> +		ret = spi_read(ad7949_adc->spi, &ad7949_adc->buffer, 2);
> +
>  	if (ret)
>  		return ret;
>  
> @@ -158,7 +157,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->spi->bits_per_word) {
> +	case 16:
> +		*val = ad7949_adc->buffer;
> +		/* Shift-out padding bits */
> +		*val >>= 16 - ad7949_adc->resolution;
> +		break;
> +	case 14:
> +		*val = ad7949_adc->buffer & GENMASK(13, 0);
> +		break;
> +	case 8:
> +		/* Here, type is big endian as data was sent in two transfers */
> +		*val = be16_to_cpu(ad7949_adc->buf8b);
> +		/* Shift-out padding bits */
> +		*val >>= 16 - ad7949_adc->resolution;
> +		break;
> +	default:
> +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> +		return -EINVAL;
> +	}
>  
>  	return 0;
>  }
> @@ -266,6 +283,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
>  
>  static int ad7949_spi_probe(struct spi_device *spi)
>  {
> +	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
>  	struct device *dev = &spi->dev;
>  	const struct ad7949_adc_spec *spec;
>  	struct ad7949_adc_chip *ad7949_adc;
> @@ -292,6 +310,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)) {
> +		spi->bits_per_word = ad7949_adc->resolution;
> +	} else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
> +		spi->bits_per_word = 16;
> +	} else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
> +		spi->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 v6 2/5] iio: adc: ad7949: fix spi messages on non 14-bit controllers

[Liam Beguin]

On Sun, Aug 29, 2021 at 03:33:34PM +0100, Jonathan Cameron wrote:
> On Sun, 15 Aug 2021 17:33:06 -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 implies different SPI transfer lengths which are not always handled
> > properly by some SPI controllers.
> > 
> > To work around this limitation, define a big endian buffer used to split
> > the buffer into two 8-bit messages in the event that the controller
> > doesn't support 14-bit or 16-bit transfers.
> > A separate buffer is introduced here to avoid performing operations on
> > types of different endianness.
> > 
> > Since all transfers use the same bits_per_word value, move that logic to
> > the probe function, and let transfers default to the value defined in
> > the struct spi_device.
> > 
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> Hi Liam, 
> 
Hi Jonathan,

> I changed the title of this one to avoid it being picked up for automated
> backporting.  I don't mind if you want to request it is backported explicitly
> but it isn't a regression fix (as it never worked on such controllers) and
> is non trivial.  As a result I want a backport to be a deliberate decision.
> Now titled "iio: adc: ad7949: enable use with non 14/16-bit controllers"
> 

That makes sense! I'll be more careful of that in the future.

Thanks,
Liam

> This and previous applied to the togreg branch of iio.git and pushed out
> as testing for 0-day to poke at it.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/adc/ad7949.c | 86 +++++++++++++++++++++++++++-------------
> >  1 file changed, 58 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > index adc4487a7d56..a263d0fcec75 100644
> > --- a/drivers/iio/adc/ad7949.c
> > +++ b/drivers/iio/adc/ad7949.c
> > @@ -14,7 +14,6 @@
> >  #include <linux/bitfield.h>
> >  
> >  #define AD7949_CFG_MASK_TOTAL		GENMASK(13, 0)
> > -#define AD7949_CFG_REG_SIZE_BITS	14
> >  
> >  /* CFG: Configuration Update */
> >  #define AD7949_CFG_MASK_OVERWRITE	BIT(13)
> > @@ -71,6 +70,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
> >   * @cfg: copy of the configuration register
> >   * @current_channel: current channel in use
> >   * @buffer: buffer to send / receive data to / from device
> > + * @buf8b: be16 buffer to exchange data with the device in 8-bit transfers
> >   */
> >  struct ad7949_adc_chip {
> >  	struct mutex lock;
> > @@ -81,27 +81,34 @@ struct ad7949_adc_chip {
> >  	u16 cfg;
> >  	unsigned int current_channel;
> >  	u16 buffer ____cacheline_aligned;
> > +	__be16 buf8b;
> >  };
> >  
> >  static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> >  				u16 mask)
> >  {
> >  	int ret;
> > -	int bits_per_word = ad7949_adc->resolution;
> > -	int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
> > -	struct spi_message msg;
> > -	struct spi_transfer tx[] = {
> > -		{
> > -			.tx_buf = &ad7949_adc->buffer,
> > -			.len = 2,
> > -			.bits_per_word = bits_per_word,
> > -		},
> > -	};
> >  
> >  	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
> > -	ad7949_adc->buffer = ad7949_adc->cfg << shift;
> > -	spi_message_init_with_transfers(&msg, tx, 1);
> > -	ret = spi_sync(ad7949_adc->spi, &msg);
> > +
> > +	switch (ad7949_adc->spi->bits_per_word) {
> > +	case 16:
> > +		ad7949_adc->buffer = ad7949_adc->cfg << 2;
> > +		ret = spi_write(ad7949_adc->spi, &ad7949_adc->buffer, 2);
> > +		break;
> > +	case 14:
> > +		ad7949_adc->buffer = ad7949_adc->cfg;
> > +		ret = spi_write(ad7949_adc->spi, &ad7949_adc->buffer, 2);
> > +		break;
> > +	case 8:
> > +		/* Here, type is big endian as it must be sent in two transfers */
> > +		ad7949_adc->buf8b = cpu_to_be16(ad7949_adc->cfg << 2);
> > +		ret = spi_write(ad7949_adc->spi, &ad7949_adc->buf8b, 2);
> > +		break;
> > +	default:
> > +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> > +		return -EINVAL;
> > +	}
> >  
> >  	/*
> >  	 * This delay is to avoid a new request before the required time to
> > @@ -116,16 +123,6 @@ 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,
> > -		},
> > -	};
> >  
> >  	/*
> >  	 * 1: write CFG for sample N and read old data (sample N-2)
> > @@ -144,9 +141,11 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> >  	}
> >  
> >  	/* 3: write something and read actual data */
> > -	ad7949_adc->buffer = 0;
> > -	spi_message_init_with_transfers(&msg, tx, 1);
> > -	ret = spi_sync(ad7949_adc->spi, &msg);
> > +	if (ad7949_adc->spi->bits_per_word == 8)
> > +		ret = spi_read(ad7949_adc->spi, &ad7949_adc->buf8b, 2);
> > +	else
> > +		ret = spi_read(ad7949_adc->spi, &ad7949_adc->buffer, 2);
> > +
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -158,7 +157,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->spi->bits_per_word) {
> > +	case 16:
> > +		*val = ad7949_adc->buffer;
> > +		/* Shift-out padding bits */
> > +		*val >>= 16 - ad7949_adc->resolution;
> > +		break;
> > +	case 14:
> > +		*val = ad7949_adc->buffer & GENMASK(13, 0);
> > +		break;
> > +	case 8:
> > +		/* Here, type is big endian as data was sent in two transfers */
> > +		*val = be16_to_cpu(ad7949_adc->buf8b);
> > +		/* Shift-out padding bits */
> > +		*val >>= 16 - ad7949_adc->resolution;
> > +		break;
> > +	default:
> > +		dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
> > +		return -EINVAL;
> > +	}
> >  
> >  	return 0;
> >  }
> > @@ -266,6 +283,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
> >  
> >  static int ad7949_spi_probe(struct spi_device *spi)
> >  {
> > +	u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
> >  	struct device *dev = &spi->dev;
> >  	const struct ad7949_adc_spec *spec;
> >  	struct ad7949_adc_chip *ad7949_adc;
> > @@ -292,6 +310,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)) {
> > +		spi->bits_per_word = ad7949_adc->resolution;
> > +	} else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
> > +		spi->bits_per_word = 16;
> > +	} else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
> > +		spi->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");
> 

[PATCH v6 3/5] iio: adc: ad7949: add vref selection support

[Liam Beguin]

From: Liam Beguin <lvb@xiphos.com>

Add support for selecting the voltage reference from the devicetree.

This change is required to get valid readings with all three
vref hardware configurations supported by the ADC.

For instance if the ADC isn't provided with an external reference,
the sample request must specify an internal voltage reference to get a
valid reading.

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

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index a263d0fcec75..5168d687687d 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -35,7 +35,11 @@
 
 /* REF: reference/buffer selection */
 #define AD7949_CFG_MASK_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_MASK_SEQ		GENMASK(2, 1)
@@ -66,6 +70,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {
  * @vref: regulator generating Vref
  * @indio_dev: reference to iio structure
  * @spi: reference to spi structure
+ * @refsel: reference selection
  * @resolution: resolution of the chip
  * @cfg: copy of the configuration register
  * @current_channel: current channel in use
@@ -77,6 +82,7 @@ struct ad7949_adc_chip {
 	struct regulator *vref;
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
+	u32 refsel;
 	u8 resolution;
 	u16 cfg;
 	unsigned int current_channel;
@@ -221,12 +227,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_adc->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;
@@ -265,7 +285,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 		FIELD_PREP(AD7949_CFG_MASK_INCC, AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
 		FIELD_PREP(AD7949_CFG_MASK_INX, ad7949_adc->current_channel) |
 		FIELD_PREP(AD7949_CFG_MASK_BW_FULL, 1) |
-		FIELD_PREP(AD7949_CFG_MASK_REF, AD7949_CFG_VAL_REF_EXT_BUF) |
+		FIELD_PREP(AD7949_CFG_MASK_REF, ad7949_adc->refsel) |
 		FIELD_PREP(AD7949_CFG_MASK_SEQ, 0x0) |
 		FIELD_PREP(AD7949_CFG_MASK_RBN, 1);
 
@@ -281,6 +301,11 @@ 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;
@@ -288,6 +313,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
 	const struct ad7949_adc_spec *spec;
 	struct ad7949_adc_chip *ad7949_adc;
 	struct iio_dev *indio_dev;
+	u32 tmp;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
@@ -322,16 +348,56 @@ static int ad7949_spi_probe(struct spi_device *spi)
 		return -EINVAL;
 	}
 
-	ad7949_adc->vref = devm_regulator_get(dev, "vref");
+	/* Setup internal voltage reference */
+	tmp = 4096000;
+	ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);
+	if (ret < 0 && ret != -EINVAL) {
+		dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
+		return ret;
+	}
+
+	switch (tmp) {
+	case 2500000:
+		ad7949_adc->refsel = AD7949_CFG_VAL_REF_INT_2500;
+		break;
+	case 4096000:
+		ad7949_adc->refsel = AD7949_CFG_VAL_REF_INT_4096;
+		break;
+	default:
+		dev_err(dev, "unsupported internal voltage reference\n");
+		return -EINVAL;
+	}
+
+	/* Setup external voltage reference, buffered? */
+	ad7949_adc->vref = devm_regulator_get_optional(dev, "vrefin");
 	if (IS_ERR(ad7949_adc->vref)) {
-		dev_err(dev, "fail to request regulator\n");
-		return PTR_ERR(ad7949_adc->vref);
+		ret = PTR_ERR(ad7949_adc->vref);
+		if (ret != -ENODEV)
+			return ret;
+		/* unbuffered? */
+		ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
+		if (IS_ERR(ad7949_adc->vref)) {
+			ret = PTR_ERR(ad7949_adc->vref);
+			if (ret != -ENODEV)
+				return ret;
+		} else {
+			ad7949_adc->refsel = AD7949_CFG_VAL_REF_EXT_TEMP;
+		}
+	} else {
+		ad7949_adc->refsel = 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 (ad7949_adc->refsel & 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;
 	}
 
 	mutex_init(&ad7949_adc->lock);
@@ -352,7 +418,6 @@ static int ad7949_spi_probe(struct spi_device *spi)
 
 err:
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
 
 	return ret;
 }
@@ -364,7 +429,6 @@ static int ad7949_spi_remove(struct spi_device *spi)
 
 	iio_device_unregister(indio_dev);
 	mutex_destroy(&ad7949_adc->lock);
-	regulator_disable(ad7949_adc->vref);
 
 	return 0;
 }
-- 
2.32.0.452.g940fe202adcb

Re: [PATCH v6 3/5] iio: adc: ad7949: add vref selection support

[Andy Shevchenko]

On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com> wrote:
>
> From: Liam Beguin <lvb@xiphos.com>
>
> Add support for selecting the voltage reference from the devicetree.
>
> This change is required to get valid readings with all three
> vref hardware configurations supported by the ADC.
>
> For instance if the ADC isn't provided with an external reference,
> the sample request must specify an internal voltage reference to get a
> valid reading.

...

> +       /* Setup internal voltage reference */
> +       tmp = 4096000;
> +       ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);

> +       if (ret < 0 && ret != -EINVAL) {

What does this check (second part) is supposed to mean?
The first part will make it mandatory, is it the goal?

> +               dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
> +               return ret;
> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 3/5] iio: adc: ad7949: add vref selection support

[Liam Beguin]

On Mon Aug 16, 2021 at 4:04 AM EDT, Andy Shevchenko wrote:
> On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com>
> wrote:
> >
> > From: Liam Beguin <lvb@xiphos.com>
> >
> > Add support for selecting the voltage reference from the devicetree.
> >
> > This change is required to get valid readings with all three
> > vref hardware configurations supported by the ADC.
> >
> > For instance if the ADC isn't provided with an external reference,
> > the sample request must specify an internal voltage reference to get a
> > valid reading.
>
> ...
>
> > +       /* Setup internal voltage reference */
> > +       tmp = 4096000;
> > +       ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);
>
> > +       if (ret < 0 && ret != -EINVAL) {

Hi Andy,

>
> What does this check (second part) is supposed to mean?
> The first part will make it mandatory, is it the goal?
>

device_property_read_u32() will return -EINVAL if the property isn't
found in the devicetree.

This checks for errors when the property is defined while keeping it
optional.

Liam

> > +               dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
> > +               return ret;
> > +       }
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v6 3/5] iio: adc: ad7949: add vref selection support

[Andy Shevchenko]

On Mon, Aug 16, 2021 at 3:39 PM Liam Beguin <liambeguin@gmail.com> wrote:
> On Mon Aug 16, 2021 at 4:04 AM EDT, Andy Shevchenko wrote:
> > On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com>
> > wrote:

...

> > > +       tmp = 4096000;
> > > +       ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);
> >
> > > +       if (ret < 0 && ret != -EINVAL) {
>
> Hi Andy,
>
> >
> > What does this check (second part) is supposed to mean?
> > The first part will make it mandatory, is it the goal?
> >
>
> device_property_read_u32() will return -EINVAL if the property isn't
> found in the devicetree.
>
> This checks for errors when the property is defined while keeping it
> optional.

Don't assign and don't check the error code of the API. As simply as that.

> > > +               dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
> > > +               return ret;
> > > +       }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 3/5] iio: adc: ad7949: add vref selection support

[Liam Beguin]

On Mon Aug 16, 2021 at 8:48 AM EDT, Andy Shevchenko wrote:
> On Mon, Aug 16, 2021 at 3:39 PM Liam Beguin <liambeguin@gmail.com>
> wrote:
> > On Mon Aug 16, 2021 at 4:04 AM EDT, Andy Shevchenko wrote:
> > > On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com>
> > > wrote:
>
> ...
>
> > > > +       tmp = 4096000;
> > > > +       ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);
> > >
> > > > +       if (ret < 0 && ret != -EINVAL) {
> >
> > Hi Andy,
> >
> > >
> > > What does this check (second part) is supposed to mean?
> > > The first part will make it mandatory, is it the goal?
> > >
> >
> > device_property_read_u32() will return -EINVAL if the property isn't
> > found in the devicetree.
> >
> > This checks for errors when the property is defined while keeping it
> > optional.
>
> Don't assign and don't check the error code of the API. As simply as
> that.

I'm not against getting rid of it, but I was asked to check for these
errors in earlier revisions of the patch.

Liam

>
> > > > +               dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
> > > > +               return ret;
> > > > +       }
>
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v6 3/5] iio: adc: ad7949: add vref selection support

[Andy Shevchenko]

On Mon, Aug 16, 2021 at 4:07 PM Liam Beguin <liambeguin@gmail.com> wrote:
> On Mon Aug 16, 2021 at 8:48 AM EDT, Andy Shevchenko wrote:
> > On Mon, Aug 16, 2021 at 3:39 PM Liam Beguin <liambeguin@gmail.com>
> > wrote:
> > > On Mon Aug 16, 2021 at 4:04 AM EDT, Andy Shevchenko wrote:
> > > > On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com>
> > > > wrote:

...

> > > > > +       tmp = 4096000;
> > > > > +       ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);
> > > >
> > > > > +       if (ret < 0 && ret != -EINVAL) {
> > >
> > > Hi Andy,
> > >
> > > >
> > > > What does this check (second part) is supposed to mean?
> > > > The first part will make it mandatory, is it the goal?
> > > >
> > >
> > > device_property_read_u32() will return -EINVAL if the property isn't
> > > found in the devicetree.
> > >
> > > This checks for errors when the property is defined while keeping it
> > > optional.
> >
> > Don't assign and don't check the error code of the API. As simply as
> > that.
>
> I'm not against getting rid of it, but I was asked to check for these
> errors in earlier revisions of the patch.

Okay, I leave it to you, guys, to decide, just note that the usual
pattern for optional stuff
a) either check for (!ret);
b) or ignore the returned value completely.

> > > > > +               dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
> > > > > +               return ret;
> > > > > +       }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 3/5] iio: adc: ad7949: add vref selection support

[Jonathan Cameron]

On Mon, 16 Aug 2021 16:12:58 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Aug 16, 2021 at 4:07 PM Liam Beguin <liambeguin@gmail.com> wrote:
> > On Mon Aug 16, 2021 at 8:48 AM EDT, Andy Shevchenko wrote:  
> > > On Mon, Aug 16, 2021 at 3:39 PM Liam Beguin <liambeguin@gmail.com>
> > > wrote:  
> > > > On Mon Aug 16, 2021 at 4:04 AM EDT, Andy Shevchenko wrote:  
> > > > > On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com>
> > > > > wrote:  
> 
> ...
> 
> > > > > > +       tmp = 4096000;
> > > > > > +       ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);  
> > > > >  
> > > > > > +       if (ret < 0 && ret != -EINVAL) {  
> > > >
> > > > Hi Andy,
> > > >  
> > > > >
> > > > > What does this check (second part) is supposed to mean?
> > > > > The first part will make it mandatory, is it the goal?
> > > > >  
> > > >
> > > > device_property_read_u32() will return -EINVAL if the property isn't
> > > > found in the devicetree.
> > > >
> > > > This checks for errors when the property is defined while keeping it
> > > > optional.  
> > >
> > > Don't assign and don't check the error code of the API. As simply as
> > > that.  
> >
> > I'm not against getting rid of it, but I was asked to check for these
> > errors in earlier revisions of the patch.  
> 
> Okay, I leave it to you, guys, to decide, just note that the usual
> pattern for optional stuff
> a) either check for (!ret);
> b) or ignore the returned value completely.

Hmm. My thinking (I suspect I asked for it to be checked, but can't remember :)
was that I'd really like to know if a device tree contains a property but that
property is invalid for some reason. The docs give a bunch of reasons beyond
the property not existing (which is unhelpfully described as just 'invalid parameters'). 

I guess that's a bit far fetched.  Let's drop the check as Andy suggests.

Dropped that check and applied to the togreg branch of iio.git, initially pushed out
as testing for 0-day to poke at it.  + we are about to enter merge window so I don't
want linux-next to pick it up just yet!

Jonathan

> 
> > > > > > +               dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
> > > > > > +               return ret;
> > > > > > +       }  
> 
> 

Re: [PATCH v6 3/5] iio: adc: ad7949: add vref selection support

[Liam Beguin]

On Sun, Aug 29, 2021 at 03:35:39PM +0100, Jonathan Cameron wrote:
> On Mon, 16 Aug 2021 16:12:58 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Mon, Aug 16, 2021 at 4:07 PM Liam Beguin <liambeguin@gmail.com> wrote:
> > > On Mon Aug 16, 2021 at 8:48 AM EDT, Andy Shevchenko wrote:  
> > > > On Mon, Aug 16, 2021 at 3:39 PM Liam Beguin <liambeguin@gmail.com>
> > > > wrote:  
> > > > > On Mon Aug 16, 2021 at 4:04 AM EDT, Andy Shevchenko wrote:  
> > > > > > On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com>
> > > > > > wrote:  
> > 
> > ...
> > 
> > > > > > > +       tmp = 4096000;
> > > > > > > +       ret = device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);  
> > > > > >  
> > > > > > > +       if (ret < 0 && ret != -EINVAL) {  
> > > > >
> > > > > Hi Andy,
> > > > >  
> > > > > >
> > > > > > What does this check (second part) is supposed to mean?
> > > > > > The first part will make it mandatory, is it the goal?
> > > > > >  
> > > > >
> > > > > device_property_read_u32() will return -EINVAL if the property isn't
> > > > > found in the devicetree.
> > > > >
> > > > > This checks for errors when the property is defined while keeping it
> > > > > optional.  
> > > >
> > > > Don't assign and don't check the error code of the API. As simply as
> > > > that.  
> > >
> > > I'm not against getting rid of it, but I was asked to check for these
> > > errors in earlier revisions of the patch.  
> > 
> > Okay, I leave it to you, guys, to decide, just note that the usual
> > pattern for optional stuff
> > a) either check for (!ret);
> > b) or ignore the returned value completely.
> 

Hi Jonathan,

> Hmm. My thinking (I suspect I asked for it to be checked, but can't remember :)
> was that I'd really like to know if a device tree contains a property but that
> property is invalid for some reason. The docs give a bunch of reasons beyond
> the property not existing (which is unhelpfully described as just 'invalid parameters'). 
> 
> I guess that's a bit far fetched.  Let's drop the check as Andy suggests.
> 

Understood, Thanks for making the change.

Liam

> Dropped that check and applied to the togreg branch of iio.git, initially pushed out
> as testing for 0-day to poke at it.  + we are about to enter merge window so I don't
> want linux-next to pick it up just yet!
> 
> Jonathan
> 
> > 
> > > > > > > +               dev_err(dev, "invalid value for adi,internal-ref-microvolt\n");
> > > > > > > +               return ret;
> > > > > > > +       }  
> > 
> > 
> 

[PATCH v6 4/5] dt-bindings: iio: adc: ad7949: update voltage reference bindings

[Liam Beguin]

From: Liam Beguin <lvb@xiphos.com>

Update bindings to describe support for buffered and unbuffered external
voltage references selection, and add adi,internal-ref-microvolt for
internal voltage reference selection.

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

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
index 9b56bd4d5510..0b10ed5f74ae 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml
@@ -26,19 +26,43 @@ properties:
   reg:
     maxItems: 1
 
+  vrefin-supply:
+    description:
+      Buffered ADC reference voltage supply.
+
   vref-supply:
     description:
-      ADC reference voltage supply
+      Unbuffered ADC reference voltage supply.
+
+  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
+
 
   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
 
 additionalProperties: false
 
@@ -49,9 +73,30 @@ examples:
         #size-cells = <0>;
 
         adc@0 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
             compatible = "adi,ad7949";
             reg = <0>;
             vref-supply = <&vdd_supply>;
         };
+
+        adc@1 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            compatible = "adi,ad7949";
+            reg = <1>;
+            vrefin-supply = <&vdd_supply>;
+        };
+
+        adc@2 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            compatible = "adi,ad7949";
+            reg = <2>;
+            adi,internal-ref-microvolt = <4096000>;
+        };
     };
 ...
-- 
2.32.0.452.g940fe202adcb

Re: [PATCH v6 4/5] dt-bindings: iio: adc: ad7949: update voltage reference bindings

[Rob Herring]

On Sun, 15 Aug 2021 17:33:08 -0400, Liam Beguin wrote:
> From: Liam Beguin <lvb@xiphos.com>
> 
> Update bindings to describe support for buffered and unbuffered external
> voltage references selection, and add adi,internal-ref-microvolt for
> internal voltage reference selection.
> 
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
>  .../bindings/iio/adc/adi,ad7949.yaml          | 51 +++++++++++++++++--
>  1 file changed, 48 insertions(+), 3 deletions(-)
> 

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

[PATCH v6 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 5168d687687d..5b8858719b53 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -405,34 +405,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" },
@@ -455,7 +437,6 @@ static struct spi_driver ad7949_spi_driver = {
 		.of_match_table	= ad7949_spi_of_id,
 	},
 	.probe	  = ad7949_spi_probe,
-	.remove   = ad7949_spi_remove,
 	.id_table = ad7949_spi_id,
 };
 module_spi_driver(ad7949_spi_driver);
-- 
2.32.0.452.g940fe202adcb

Re: [PATCH v6 0/5] AD7949 Fixes

[Andy Shevchenko]

On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com> wrote:
>
> 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.
>
> v6 drops support for per channel vref selection.
> After switching the voltage reference, readings take a little while to
> stabilize, invalidating consecutive readings.
>
> This could've been addressed by adding more dummy cycles at the expense
> of speed, but discussing the issue with colleagues more involved in
> hardware design, it turns out these circuits are usually designed with a
> single vref in mind.
>
> [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 v5:
> - rename defines: s/AD7949_CFG_BIT_/AD7949_CFG_MASK_/g
> - rename AD7949_MASK_TOTAL to match other defines

> - make vref selection global instead of per channel, and update
>   dt-bindings

Same as per v5: is it a hardware limitation?
It's unclear to me what happened here.

> - reword commits 2/5, 3/5, and 4/5
> - move bits_per_word configuration to struct spi_device, and switch to
>   spi_{read,write}.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 0/5] AD7949 Fixes

[Liam Beguin]

On Mon Aug 16, 2021 at 4:08 AM EDT, Andy Shevchenko wrote:
> On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com>
> wrote:
> >
> > 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.
> >
> > v6 drops support for per channel vref selection.
> > After switching the voltage reference, readings take a little while to
> > stabilize, invalidating consecutive readings.
> >
> > This could've been addressed by adding more dummy cycles at the expense
> > of speed, but discussing the issue with colleagues more involved in
> > hardware design, it turns out these circuits are usually designed with a
> > single vref in mind.
> >
> > [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 v5:
> > - rename defines: s/AD7949_CFG_BIT_/AD7949_CFG_MASK_/g
> > - rename AD7949_MASK_TOTAL to match other defines
>
> > - make vref selection global instead of per channel, and update
> >   dt-bindings

Hi Andy,

>
> Same as per v5: is it a hardware limitation?
> It's unclear to me what happened here.

I tried to provide more details in the last paragraph above.

After switching the voltage reference, readings take a little while to
stabilize invalidating consecutive readings.

One option was to add more dummy cycles, but in addition to making
things slower it was brought to my attention that this kind of circuit
is usually designed with a single vref in mind.

For those reasons and because I didn't have an explicit need for it, I
decided to drop that part.

Liam

>
> > - reword commits 2/5, 3/5, and 4/5
> > - move bits_per_word configuration to struct spi_device, and switch to
> >   spi_{read,write}.
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v6 0/5] AD7949 Fixes

[Jonathan Cameron]

On Mon, 16 Aug 2021 08:59:28 -0400
"Liam Beguin" <liambeguin@gmail.com> wrote:

> On Mon Aug 16, 2021 at 4:08 AM EDT, Andy Shevchenko wrote:
> > On Mon, Aug 16, 2021 at 12:35 AM Liam Beguin <liambeguin@gmail.com>
> > wrote:  
> > >
> > > 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.
> > >
> > > v6 drops support for per channel vref selection.
> > > After switching the voltage reference, readings take a little while to
> > > stabilize, invalidating consecutive readings.
> > >
> > > This could've been addressed by adding more dummy cycles at the expense
> > > of speed, but discussing the issue with colleagues more involved in
> > > hardware design, it turns out these circuits are usually designed with a
> > > single vref in mind.
> > >
> > > [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 v5:
> > > - rename defines: s/AD7949_CFG_BIT_/AD7949_CFG_MASK_/g
> > > - rename AD7949_MASK_TOTAL to match other defines  
> >  
> > > - make vref selection global instead of per channel, and update
> > >   dt-bindings  
> 
> Hi Andy,
> 
> >
> > Same as per v5: is it a hardware limitation?
> > It's unclear to me what happened here.  
> 
> I tried to provide more details in the last paragraph above.
> 
> After switching the voltage reference, readings take a little while to
> stabilize invalidating consecutive readings.
> 
> One option was to add more dummy cycles, but in addition to making
> things slower it was brought to my attention that this kind of circuit
> is usually designed with a single vref in mind.
> 
> For those reasons and because I didn't have an explicit need for it, I
> decided to drop that part.

It's not 'impossible' to add it back in later if someone needs it, but
until then this works for me.

Series applied with tweaks as described in individual patch replies.

Thanks,

Jonathan

> 
> Liam
> 
> >  
> > > - reword commits 2/5, 3/5, and 4/5
> > > - move bits_per_word configuration to struct spi_device, and switch to
> > >   spi_{read,write}.  
> >
> > --
> > With Best Regards,
> > Andy Shevchenko  
>