Add support for AD4000 series of ADCs

The behavior of an SPI controller data output line (SDO or MOSI or COPI (Controller Output Peripheral Input) for disambiguation) is not specified when the controller is not clocking out data on SCLK edges. However, there exist SPI peripherals that require specific COPI line state when data is not being clocked out of the controller.
Add SPI mode bit to allow pheripherals to request explicit COPI idle behavior when needed.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 drivers/spi/spi.c            | 6 ++++++
 include/uapi/linux/spi/spi.h | 3 ++-

 2 files changed, 8 insertions(+), 1 deletion(-)

drivers/spi/spi.c +0 -0

index 289feccca376..6072b6e93bef 100644
@@ -3921,6 +3921,12 @@ int spi_setup(struct spi_device *spi)

 		(SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL |
 		 SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL)))

  return -EINVAL;

drivers/spi/spi.c +2 -0

+	/* Check against conflicting MOSI idle configuration */
+	if ((spi->mode & SPI_MOSI_IDLE_LOW) && (spi->mode &

SPI_MOSI_IDLE_HIGH)) {

drivers/spi/spi.c +2 -0

+		dev_warn(&spi->dev,
+			 "setup: erratic MOSI idle configuration. Set to idle

low\n");

Implement MOSI idle low and MOSI idle high to better support peripherals that request specific MOSI behavior.

Signed-off-by Marcelo Schmitt <[email protected]>

---

Implement MOSI idle low and MOSI idle high to better support peripherals that request specific MOSI behavior.

Signed-off-by Marcelo Schmitt <[email protected]>

---

 drivers/spi/spi-gpio.c | 12 +++++++++++-

 1 file changed, 11 insertions(+), 1 deletion(-)

drivers/spi/spi-gpio.c +0 -0

index 909cce109bba..d3b8c99f0cb4 100644
@@ -236,6 +236,14 @@ static void spi_gpio_chipselect(struct spi_device *spi,
 int is_active)
  	}
  }

drivers/spi/spi-gpio.c +8 -0

+static void spi_gpio_set_mosi_idle(struct spi_device *spi)
+{
+	struct spi_gpio *spi_gpio = spi_to_spi_gpio(spi);
+
+	gpiod_set_value_cansleep(spi_gpio->mosi,
+				 !!(spi->mode & SPI_MOSI_IDLE_HIGH));
+}
+

 static int spi_gpio_setup(struct spi_device *spi) {
  struct gpio_desc *cs;

drivers/spi/spi-gpio.c +0 -0

@@ -411,7 +419,8 @@ static int spi_gpio_probe(struct platform_device *pdev)

  host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32); host->mode_bits = SPI_3WIRE | SPI_3WIRE_HIZ | SPI_CPHA | SPI_CPOL |

drivers/spi/spi-gpio.c +2 -1

-			    SPI_CS_HIGH | SPI_LSB_FIRST;
+			  SPI_CS_HIGH | SPI_LSB_FIRST | SPI_MOSI_IDLE_LOW |
+			  SPI_MOSI_IDLE_HIGH;

  if (!spi_gpio->mosi) {
  /* HW configuration without MOSI pin *

drivers/spi/spi-gpio.c +1 -0

@@ -436,6 +445,7 @@ static int spi_gpio_probe(struct platform_device *pdev)
  	host->flags |= SPI_CONTROLLER_GPIO_SS;
  	bb->chipselect = spi_gpio_chipselect;
  	bb->set_line_direction = spi_gpio_set_direction;
+	bb->set_mosi_idle = spi_gpio_set_mosi_idle;

  if (host->flags & SPI_CONTROLLER_NO_TX) { bb->txrx_word[SPI_MODE_0] = spi_gpio_spec_txrx_word_mode0;

Some SPI peripherals may require strict MOSI line state when the controller is not clocking out data.
Implement support for MOSI idle state configuration (low or high) by setting the data output line level on controller setup and after transfers. Bitbang operations now call controller specific set_mosi_idle() call back to set MOSI to its idle state.
The MOSI line is kept at its idle state if no tx buffer is provided.

Signed-off-by Marcelo Schmitt <[email protected]>

---

 drivers/spi/spi-bitbang.c       | 24 ++++++++++++++++++++++++ include/linux/spi/spi_bitbang.h |  1 +

 2 files changed, 25 insertions(+)

drivers/spi/spi-bitbang.c +0 -0

index ca5cc67555c5..3dee085d3570 100644
@@ -63,21 +63,28 @@ static unsigned bitbang_txrx_8(

  unsigned flags
 )
 {

drivers/spi/spi-bitbang.c +1 -0

+	struct spi_bitbang	*bitbang;

  unsigned bits = t->bits_per_word;
  unsigned count = t->len;
  const u8 *tx = t->tx_buf;
  u8 *rx = t->rx_buf;

drivers/spi/spi-bitbang.c +1 -0

+	bitbang = spi_controller_get_devdata(spi->controller);

  while (likely(count > 0)) {
  u8 word = 0;

  if (tx)
  word = *tx++;

  word = txrx_word(spi, ns, word, bits, flags); if (rx)
  *rx++ = word;
  count -= 1;
  }

drivers/spi/spi-bitbang.c +3 -0

+	if (bitbang->set_mosi_idle)
+		bitbang->set_mosi_idle(spi);
+

  return t->len - count;
 }

drivers/spi/spi-bitbang.c +0 -0

@@ -92,21 +99,28 @@ static unsigned bitbang_txrx_16(

  unsigned flags
 )
 {

drivers/spi/spi-bitbang.c +1 -0

+	struct spi_bitbang	*bitbang;

  unsigned bits = t->bits_per_word;
  unsigned count = t->len;
  const u16 *tx = t->tx_buf;
  u16 *rx = t->rx_buf;

drivers/spi/spi-bitbang.c +1 -0

+	bitbang = spi_controller_get_devdata(spi->controller);

  while (likely(count > 1)) {
  u16 word = 0;

  if (tx)
  word = *tx++;

This patch series extends the SPI bitbang, gpio, and spi-engine controllers to support configurable MOSI line idle state.
It then introduces the ad4000 driver which makes use of the MOSI idle configuration to properly support AD4000 series of ADCs.

Hi Marcelo,

Quoted 4 lines · Marcelo Schmitt

This patch series extends the SPI bitbang, gpio, and spi-engine controllers to support configurable MOSI line idle state.
It then introduces the ad4000 driver which makes use of the MOSI idle configuration to properly support AD4000 series of ADCs.

Not sure what happened but I'm not seeing the patch for ad4000...

- Nuno Sá

drivers/spi/spi.c int spi_setup(struct spi_device *spi) · line 3929

Quoted 8 lines · Marcelo Schmitt

(quoted diff) +2 -0

+	/* Check against conflicting MOSI idle configuration */
+	if ((spi->mode & SPI_MOSI_IDLE_LOW) && (spi->mode &

SPI_MOSI_IDLE_HIGH)) {

(quoted diff) +2 -0

+		dev_warn(&spi->dev,
+			 "setup: erratic MOSI idle configuration. Set to idle

low\n");

drivers/spi/spi.c +2 -0

+		spi->mode &= ~SPI_MOSI_IDLE_HIGH;
+	}

Should we assume such a thing? IOW, should this be treated as a warning or a real error? I would assume this should be a configuration error and return - EINVAL but...

The behavior of an SPI controller data output line (SDO or MOSI or COPI (Controller Output Peripheral Input) for disambiguation) is not specified when the controller is not clocking out data on SCLK edges. However, there exist SPI peripherals that require specific COPI line state when data is not being clocked out of the controller.

#define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave mosi line low when idle */
+#define SPI_MOSI_IDLE_HIGH _BITUL(18) /* leave mosi line high when idle */

Add support for AD4000 series of low noise, low power, high speed, successive aproximation register (SAR) ADCs.

Signed-off-by Marcelo Schmitt <[email protected]>

---

drivers/iio/adc/ad4000.c new file +58 -0

+
+static const struct ad4000_chip_info ad4000_chips[] = {
+	[ID_AD4000] = {
+		.dev_name = "ad4000",
+		.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
+	},
+	[ID_AD4001] = {
+		.dev_name = "ad4001",
+		.chan_spec = AD4000_DIFF_CHANNEL('s', 16),
+	},
+	[ID_AD4002] = {
+		.dev_name = "ad4002",
+		.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
+	},
+	[ID_AD4003] = {
+		.dev_name = "ad4003",
+		.chan_spec = AD4000_DIFF_CHANNEL('s', 18),
+	},
+	[ID_AD4004] = {
+		.dev_name = "ad4004",
+		.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
+	},
+	[ID_AD4005] = {
+		.dev_name = "ad4005",
+		.chan_spec = AD4000_DIFF_CHANNEL('s', 16),
+	},
+	[ID_AD4006] = {
+		.dev_name = "ad4006",
+		.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
+	},
+	[ID_AD4007] = {
+		.dev_name = "ad4007",
+		.chan_spec = AD4000_DIFF_CHANNEL('s', 18),
+	},
+	[ID_AD4008] = {
+		.dev_name = "ad4008",
+		.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
+	},
+	[ID_AD4010] = {
+		.dev_name = "ad4010",
+		.chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
+	},
+	[ID_AD4011] = {
+		.dev_name = "ad4011",
+		.chan_spec = AD4000_DIFF_CHANNEL('s', 18),
+	},
+	[ID_AD4020] = {
+		.dev_name = "ad4020",
+		.chan_spec = AD4000_DIFF_CHANNEL('s', 20),
+	},
+	[ID_AD4021] = {
+		.dev_name = "ad4021",
+		.chan_spec = AD4000_DIFF_CHANNEL('s', 20),
+	},
+	[ID_AD4022] = {
+		.dev_name = "ad4022",
+		.chan_spec = AD4000_DIFF_CHANNEL('s', 20),
+	},

drivers/iio/adc/ad4000.c new file +32 -0

+struct ad4000_state {
+	struct spi_device *spi;
+	struct gpio_desc *cnv_gpio;
+	struct spi_transfer xfers[2];
+	struct spi_message msg;
+	int vref;
+	enum ad4000_spi_mode spi_mode;
+	bool span_comp;
+	bool turbo_mode;
+	int gain_milli;
+	int scale_tbl[2][2];
+
+	/*
+	 * DMA (thus cache coherency maintenance) requires the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	struct {
+		union {
+			__be16 sample_buf16;
+			__be32 sample_buf32;
+		} data;
+		s64 timestamp __aligned(8);
+	} scan __aligned(IIO_DMA_MINALIGN);
+	__be16 tx_buf;
+	__be16 rx_buf;
+};
+
+static void ad4000_fill_scale_tbl(struct ad4000_state *st, int scale_bits,
+				  const struct ad4000_chip_info *chip)
+{
+	int diff = chip->chan_spec.differential;
+	int val, val2, tmp0, tmp1;

drivers/iio/adc/ad4000.c new file +15 -0

+	/* Store scale for when span compression is disabled */
+	st->scale_tbl[0][0] = tmp0; /* Integer part */
+	st->scale_tbl[0][1] = abs(tmp1); /* Fractional part */
+	/* Store scale for when span compression is enabled */
+	st->scale_tbl[1][0] = tmp0;
+	/* The integer part is always zero so don't bother to divide it. */
+	if (diff)
+		st->scale_tbl[1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 4, 5);
+	else
+		st->scale_tbl[1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 9, 10);
+}
+
+static int ad4000_write_reg(struct ad4000_state *st, uint8_t val)
+{
+	st->tx_buf = cpu_to_be16(AD4000_WRITE_COMMAND << BITS_PER_BYTE |

val);

drivers/iio/adc/ad4000.c new file +31 -0

+}
+
+static int ad4000_read_reg(struct ad4000_state *st, unsigned int *val)
+{
+	struct spi_transfer t[] = {
+		{
+			.tx_buf = &st->tx_buf,
+			.rx_buf = &st->rx_buf,
+			.len = 2,
+		},
+	};
+	int ret;
+
+	st->tx_buf = cpu_to_be16(AD4000_READ_COMMAND << BITS_PER_BYTE);
+	ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
+	if (ret < 0)
+		return ret;
+
+	*val = be16_to_cpu(st->rx_buf);
+
+	return ret;
+}
+
+static void ad4000_unoptimize_msg(void *msg)
+{
+	spi_unoptimize_message(msg);
+}
+
+/*
+ * This executes a data sample transfer for when the device connections are
+ * in "3-wire" mode, selected by setting the adi,spi-mode device tree

property
+ * to "single". In this connection mode, the ADC SDI pin is connected to MOSI or
+ * to VIO and ADC CNV pin is connected either to a SPI controller CS or to a GPIO.
+ * AD4000 series of devices initiate conversions on the rising edge of CNV pin.
+ *
+ * If the CNV pin is connected to an SPI controller CS line (which is by default
+ * active low), the ADC readings would have a latency (delay) of one read.
+ * Moreover, since we also do ADC sampling for filling the buffer on triggered
+ * buffer mode, the timestamps of buffer readings would be disarranged.
+ * To prevent the read latency and reduce the time discrepancy between the
+ * sample read request and the time of actual sampling by the ADC, do a
+ * preparatory transfer to pulse the CS/CNV line.
+ */

(quoted diff) +2 -0

+static int ad4000_prepare_3wire_mode_message(struct ad4000_state *st,
+					     const struct iio_chan_spec

*chan)

drivers/iio/adc/ad4000.c new file +29 -0

+{
+	unsigned int cnv_pulse_time = st->turbo_mode ? AD4000_TQUIET1_NS
+						     : AD4000_TCONV_NS;
+	struct spi_transfer *xfers = st->xfers;
+	int ret;
+
+	xfers[0].cs_change = 1;
+	xfers[0].cs_change_delay.value = cnv_pulse_time;
+	xfers[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
+
+	xfers[1].rx_buf = &st->scan.data;
+	xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
+	xfers[1].delay.value = AD4000_TQUIET2_NS;
+	xfers[1].delay.unit = SPI_DELAY_UNIT_NSECS;
+
+	spi_message_init_with_transfers(&st->msg, st->xfers, 2);
+
+	ret = spi_optimize_message(st->spi, &st->msg);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(&st->spi->dev, ad4000_unoptimize_msg,
+					&st->msg);
+}
+
+/*
+ * This executes a data sample transfer for when the device connections are
+ * in "4-wire" mode, selected when the adi,spi-mode device tree
+ * property is absent or empty. In this connection mode, the controller CS

pin
+ * is connected to ADC SDI pin and a GPIO is connected to ADC CNV pin.
+ * The GPIO connected to ADC CNV pin is set outside of the SPI transfer.
+ */

(quoted diff) +2 -0

+static int ad4000_prepare_4wire_mode_message(struct ad4000_state *st,
+					     const struct iio_chan_spec

*chan)

drivers/iio/adc/ad4000.c new file +8 -0

+{
+	unsigned int cnv_to_sdi_time = st->turbo_mode ? AD4000_TQUIET1_NS
+						      : AD4000_TCONV_NS;
+	struct spi_transfer *xfers = st->xfers;
+	int ret;
+
+	/*
+	 * Dummy transfer to cause enough delay between CNV going high and

SDI

drivers/iio/adc/ad4000.c new file +25 -0

+	 * going low.
+	 */
+	xfers[0].cs_off = 1;
+	xfers[0].delay.value = cnv_to_sdi_time;
+	xfers[0].delay.unit = SPI_DELAY_UNIT_NSECS;
+
+	xfers[1].rx_buf = &st->scan.data;
+	xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
+
+	spi_message_init_with_transfers(&st->msg, st->xfers, 2);
+
+	ret = spi_optimize_message(st->spi, &st->msg);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(&st->spi->dev, ad4000_unoptimize_msg,
+					&st->msg);
+}
+
+static int ad4000_convert_and_acquire(struct ad4000_state *st)
+{
+	int ret;
+
+	/*
+	 * In 4-wire mode, the CNV line is held high for the entire

conversion

(quoted diff) +1 -0

+	 * and acquisition process. In other modes st->cnv_gpio is NULL and

is

drivers/iio/adc/ad4000.c new file +8 -0

+	ret = spi_sync(st->spi, &st->msg);
+	gpiod_set_value_cansleep(st->cnv_gpio, 0);
+
+	return ret;
+}
+
+static int ad4000_single_conversion(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan, int

*val)

drivers/iio/adc/ad4000.c new file +34 -0

+	if (chan->scan_type.storagebits > 16)
+		sample = be32_to_cpu(st->scan.data.sample_buf32);
+	else
+		sample = be16_to_cpu(st->scan.data.sample_buf16);
+
+	switch (chan->scan_type.realbits) {
+	case 16:
+		break;
+	case 18:
+		sample = FIELD_GET(AD4000_18BIT_MSK, sample);
+		break;
+	case 20:
+		sample = FIELD_GET(AD4000_20BIT_MSK, sample);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (chan->scan_type.sign == 's')
+		*val = sign_extend32(sample, chan->scan_type.realbits - 1);
+
+	return IIO_VAL_INT;
+}
+
+static int ad4000_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long info)
+{
+	struct ad4000_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+			return ad4000_single_conversion(indio_dev, chan,

val);

drivers/iio/adc/ad4000.c new file +36 -0

+		unreachable();
+	case IIO_CHAN_INFO_SCALE:
+		*val = st->scale_tbl[st->span_comp][0];
+		*val2 = st->scale_tbl[st->span_comp][1];
+		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = 0;
+		if (st->span_comp)
+			*val = mult_frac(st->vref / 1000, 1, 10);
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4000_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long info)
+{
+	struct ad4000_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (int *)st->scale_tbl;
+		*length = 2 * 2;
+		*type = IIO_VAL_INT_PLUS_NANO;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
+				    struct iio_chan_spec const *chan, long

mask)

drivers/iio/adc/ad4000.c new file +11 -0

+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_NANO;
+	default:
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
+}
+
+static int ad4000_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int val, int

val2,

drivers/iio/adc/ad4000.c new file +5 -0

+			    long mask)
+{
+	struct ad4000_state *st = iio_priv(indio_dev);
+	unsigned int reg_val;
+	bool span_comp_en;

(quoted diff) +2 -0

+			reg_val &= ~AD4000_CFG_SPAN_COMP;
+			reg_val |= FIELD_PREP(AD4000_CFG_SPAN_COMP,

span_comp_en);

drivers/iio/adc/ad4000.c new file +33 -0

+
+			ret = ad4000_write_reg(st, reg_val);
+			if (ret < 0)
+				return ret;
+
+			st->span_comp = span_comp_en;
+			return 0;
+		}
+		unreachable();
+	default:
+		return -EINVAL;
+	}
+}
+
+static irqreturn_t ad4000_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ad4000_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = ad4000_convert_and_acquire(st);
+	if (ret < 0)
+		goto err_out;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &st->scan,
+					   iio_get_time_ns(indio_dev));
+
+err_out:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+

drivers/iio/adc/ad4000.c new file +23 -0

+
+	return ret;
+}
+
+static const struct iio_info ad4000_info = {
+	.read_raw = &ad4000_read_raw,
+	.read_avail = &ad4000_read_avail,
+	.write_raw = &ad4000_write_raw,
+	.write_raw_get_fmt = &ad4000_write_raw_get_fmt,
+	.debugfs_reg_access = &ad4000_reg_access,
+
+};
+
+static int ad4000_config(struct ad4000_state *st)
+{
+	unsigned int reg_val;
+
+	if (device_property_present(&st->spi->dev, "adi,high-z-input"))
+		reg_val |= FIELD_PREP(AD4000_CFG_HIGHZ, 1);
+
+	/*
+	 * The ADC SDI pin might be connected to controller CS line in which
+	 * case the write might fail. This, however, does not prevent the

device

drivers/iio/adc/ad4000.c new file +30 -0

+	return ad4000_write_reg(st, reg_val);
+}
+
+static void ad4000_regulator_disable(void *reg)
+{
+	regulator_disable(reg);
+}
+
+static int ad4000_probe(struct spi_device *spi)
+{
+	const struct ad4000_chip_info *chip;
+	struct regulator *vref_reg;
+	struct iio_dev *indio_dev;
+	struct ad4000_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	chip = spi_get_device_match_data(spi);
+	if (!chip)
+		return -EINVAL;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+
+	ret = devm_regulator_get_enable(&spi->dev, "vdd");
+	if (ret)
+		return dev_err_probe(&spi->dev, ret, "Failed to enable VDD

supply\n");

drivers/iio/adc/ad4000.c new file +4 -0

+
+	ret = devm_regulator_get_enable(&spi->dev, "vio");
+	if (ret)
+		return dev_err_probe(&spi->dev, ret, "Failed to enable VIO

supply\n");

drivers/iio/adc/ad4000.c new file +7 -0

+
+	ret = regulator_enable(vref_reg);
+	if (ret < 0)
+		return dev_err_probe(&spi->dev, ret,
+				     "Failed to enable voltage regulator\n");
+
+	ret = devm_add_action_or_reset(&spi->dev, ad4000_regulator_disable,

vref_reg);

drivers/iio/adc/ad4000.c new file +3 -0

+	if (ret)
+		return dev_err_probe(&spi->dev, ret,
+				     "Failed to add regulator disable

action\n");

drivers/iio/adc/ad4000.c new file +4 -0

+
+	st->vref = regulator_get_voltage(vref_reg);
+	if (st->vref < 0)
+		return dev_err_probe(&spi->dev, st->vref, "Failed to get

vref\n");
+

(quoted diff) +1 -0

+	st->cnv_gpio = devm_gpiod_get_optional(&spi->dev, "cnv",

GPIOD_OUT_HIGH);

drivers/iio/adc/ad4000.c new file +5 -0

+	if (IS_ERR(st->cnv_gpio))
+		return dev_err_probe(&spi->dev, PTR_ERR(st->cnv_gpio),
+				     "Failed to get CNV GPIO");
+
+	ret = device_property_match_property_string(&spi->dev, "adi,spi-

mode",

drivers/iio/adc/ad4000.c new file +2 -0

+						    ad4000_spi_modes,
+						   

ARRAY_SIZE(ad4000_spi_modes));

drivers/iio/adc/ad4000.c new file +6 -0

+	/* Default to 4-wire mode if adi,spi-mode property is not present */
+	if (ret == -EINVAL)
+		st->spi_mode = AD4000_SPI_MODE_DEFAULT;
+	else if (ret < 0)
+		return dev_err_probe(&spi->dev, ret,
+				     "getting adi,spi-mode property

failed\n");

drivers/iio/adc/ad4000.c new file +6 -0

+	else
+		st->spi_mode = ret;
+
+	switch (st->spi_mode) {
+	case AD4000_SPI_MODE_DEFAULT:
+		ret = ad4000_prepare_4wire_mode_message(st, &chip-

Quoted 1 line

chan_spec);

drivers/iio/adc/ad4000.c new file +6 -0

+		if (ret)
+			return ret;
+
+		break;
+	case AD4000_SPI_MODE_SINGLE:
+		ret = ad4000_prepare_3wire_mode_message(st, &chip-

Quoted 1 line

chan_spec);

drivers/iio/adc/ad4000.c new file +11 -0

+		if (ret)
+			return ret;
+
+		/*
+		 * In "3-wire mode", the ADC SDI line must be kept high when
+		 * data is not being clocked out of the controller.
+		 * Request the SPI controller to make MOSI idle high.
+		 */
+		spi->mode = SPI_MODE_0 | SPI_MOSI_IDLE_HIGH;
+		if (spi_setup(spi))
+			dev_warn(&st->spi->dev, "SPI controller setup

failed\n");

Quoted 5 lines · Marcelo Schmitt

The behavior of an SPI controller data output line (SDO or MOSI or COPI (Controller Output Peripheral Input) for disambiguation) is not specified when the controller is not clocking out data on SCLK edges. However, there exist SPI peripherals that require specific COPI line state when data is not being clocked out of the controller.

This is an optimisation for accelerating devices that need a specific value, really if these devices need a value they should send it.

Quoted 2 lines

#define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave mosi line low when idle */
+#define SPI_MOSI_IDLE_HIGH _BITUL(18) /* leave mosi line high when idle */

Realistically we'll have a large set of drivers that are expecting the line to be held low so I'm not sure we need that option. I would also expect to have an implementation of these options in the core which supplies buffers with the relevant data for use with controllers that don't have the feature (similar to how _MUST_TX and _MUST_RX are done). Even without that we'd need feature detection so that drivers that try to use this aren't just buggy when used with a controller that doesn't implement it, but once you're detecting you may as well just make things work.

Implement MOSI idle low and MOSI idle high to better support peripherals that request specific MOSI behavior.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 drivers/spi/spi-axi-spi-engine.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/spi/spi-axi-spi-engine.c +5 -0

index 0aa31d745734..549f03069d0e 100644
@@ -41,6 +41,7 @@
 #define SPI_ENGINE_CONFIG_CPHA			BIT(0)
 #define SPI_ENGINE_CONFIG_CPOL			BIT(1)
 #define SPI_ENGINE_CONFIG_3WIRE			BIT(2)
+#define SPI_ENGINE_CONFIG_SDO_IDLE		BIT(3)
 
 #define SPI_ENGINE_INST_TRANSFER		0x0
 #define SPI_ENGINE_INST_ASSERT			0x1

@@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct spi_device *spi)
 		config |= SPI_ENGINE_CONFIG_CPHA;
 	if (spi->mode & SPI_3WIRE)
 		config |= SPI_ENGINE_CONFIG_3WIRE;
+	if (spi->mode & SPI_MOSI_IDLE_HIGH)
+		config |= SPI_ENGINE_CONFIG_SDO_IDLE;
+	if (spi->mode & SPI_MOSI_IDLE_LOW)
+		config &= ~SPI_ENGINE_CONFIG_SDO_IDLE;
 
 	return config;
 }

Quoted 7 lines · Mark Brown

Quoted 5 lines · Marcelo Schmitt

The behavior of an SPI controller data output line (SDO or MOSI or COPI (Controller Output Peripheral Input) for disambiguation) is not specified when the controller is not clocking out data on SCLK edges. However, there exist SPI peripherals that require specific COPI line state when data is not being clocked out of the controller.

I think this description is missing a key detail that the tx data line needs to be high just before and also during the CS assertion at the start of each message.

And it would be helpful to have this more detailed description in the source code somewhere and not just in the commit message.

Quoted 4 lines

Even without that we'd need feature detection so that drivers that try to use this aren't just buggy when used with a controller that doesn't implement it, but once you're detecting you may as well just make things work.

I could see something like this working for controllers that leave the tx data line in the state of the last bit of a write transfer. I.e. do a write transfer of 0xff (using the smallest number of bits per word supported by the controller) with CS not asserted, then assert CS, then do the rest of actual the transfers requested by the peripheral.

But it doesn't seem like it would work for controllers that always return the tx data line to a low state after a write since this would mean that the data line would still be low during the CS assertion which is what we need to prevent.

Add device tree documentation for AD4000 series of ADC devices.

Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf

Suggested-by David Lechner <[email protected]>

Signed-off-by Marcelo Schmitt <[email protected]>

---
Even though didn't pick all suggestions to the dt-bindings, I did pick most them so kept David's Suggested-by tag.

 .../bindings/iio/adc/adi,ad4000.yaml          | 207 ++++++++++++++++++
 MAINTAINERS                                   |   7 +
 2 files changed, 214 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml

Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml new file +33 -0

new file mode 100644
index 000000000000..7470d386906b
@@ -0,0 +1,207 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad4000.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD4000 and similar Analog to Digital Converters
+
+maintainers:
+  - Marcelo Schmitt <[email protected]>
+
+description: |
+  Analog Devices AD4000 family of Analog to Digital Converters with SPI support.
+  Specifications can be found at:
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - adi,ad4000
+      - adi,ad4001
+      - adi,ad4002
+      - adi,ad4003
+      - adi,ad4004
+      - adi,ad4005

Quoted 40 lines · Marcelo Schmitt

Implement MOSI idle low and MOSI idle high to better support peripherals that request specific MOSI behavior.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 drivers/spi/spi-axi-spi-engine.c | 8 +++++++-

 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/spi/spi-axi-spi- renamed from drivers/spi/spi-axi-spi-engine.c +0 -0

engine.c
index 0aa31d745734..549f03069d0e 100644

drivers/spi/spi-axi-spi-engine.c +1 -0

@@ -41,6 +41,7 @@
  #define SPI_ENGINE_CONFIG_CPHA			BIT(0)
  #define SPI_ENGINE_CONFIG_CPOL			BIT(1)
  #define SPI_ENGINE_CONFIG_3WIRE			BIT(2)
+#define SPI_ENGINE_CONFIG_SDO_IDLE		BIT(3)

 #define SPI_ENGINE_INST_TRANSFER 0x0
 #define SPI_ENGINE_INST_ASSERT 0x1

drivers/spi/spi-axi-spi-engine.c +4 -0

@@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct
 spi_device *spi)
  		config |= SPI_ENGINE_CONFIG_CPHA;
  	if (spi->mode & SPI_3WIRE)
  		config |= SPI_ENGINE_CONFIG_3WIRE;
+	if (spi->mode & SPI_MOSI_IDLE_HIGH)
+		config |= SPI_ENGINE_CONFIG_SDO_IDLE;
+	if (spi->mode & SPI_MOSI_IDLE_LOW)
+		config &= ~SPI_ENGINE_CONFIG_SDO_IDLE;

  return config;
 }

drivers/spi/spi-axi-spi-engine.c +0 -0

@@ -645,7 +650,8 @@ static int spi_engine_probe(struct platform_device
 *pdev)
  		return ret;

  host->dev.of_node = pdev->dev.of_node;

drivers/spi/spi-axi-spi-engine.c +1 -1

-	host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE;
+	host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE |

Quoted 5 lines

SPI_MOSI_IDLE_LOW

drivers/spi/spi-axi-spi-engine.c +1 -0

+			  | SPI_MOSI_IDLE_HIGH;

  host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32); host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2; host->transfer_one_message = spi_engine_transfer_one_message;

I think we need a version check instead of setting the flags unconditionally here since older versions of the AXI SPI Engine won't support this feature.

Quoted 41 lines · David Lechner

Quoted 40 lines · Marcelo Schmitt

Implement MOSI idle low and MOSI idle high to better support peripherals that request specific MOSI behavior.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 drivers/spi/spi-axi-spi-engine.c | 8 +++++++-

 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/spi/spi-axi-spi- renamed from drivers/spi/spi-axi-spi-engine.c +0 -0

engine.c
index 0aa31d745734..549f03069d0e 100644

drivers/spi/spi-axi-spi-engine.c +1 -0

@@ -41,6 +41,7 @@
  #define SPI_ENGINE_CONFIG_CPHA			BIT(0)
  #define SPI_ENGINE_CONFIG_CPOL			BIT(1)
  #define SPI_ENGINE_CONFIG_3WIRE			BIT(2)
+#define SPI_ENGINE_CONFIG_SDO_IDLE		BIT(3)

 #define SPI_ENGINE_INST_TRANSFER 0x0
 #define SPI_ENGINE_INST_ASSERT 0x1

drivers/spi/spi-axi-spi-engine.c +4 -0

@@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct
 spi_device *spi)
  		config |= SPI_ENGINE_CONFIG_CPHA;
  	if (spi->mode & SPI_3WIRE)
  		config |= SPI_ENGINE_CONFIG_3WIRE;
+	if (spi->mode & SPI_MOSI_IDLE_HIGH)
+		config |= SPI_ENGINE_CONFIG_SDO_IDLE;
+	if (spi->mode & SPI_MOSI_IDLE_LOW)
+		config &= ~SPI_ENGINE_CONFIG_SDO_IDLE;

  return config;
 }

drivers/spi/spi-axi-spi-engine.c +0 -0

@@ -645,7 +650,8 @@ static int spi_engine_probe(struct platform_device
 *pdev)
  		return ret;

  host->dev.of_node = pdev->dev.of_node;

drivers/spi/spi-axi-spi-engine.c +1 -1

-	host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE;
+	host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE |

Quoted 8 lines

Quoted 5 lines

SPI_MOSI_IDLE_LOW

drivers/spi/spi-axi-spi-engine.c +1 -0

+			  | SPI_MOSI_IDLE_HIGH;

  host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32); host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2; host->transfer_one_message = spi_engine_transfer_one_message;

I think we need a version check instead of setting the flags unconditionally here since older versions of the AXI SPI Engine won't support this feature.

Oh, was not aware of that... Then, we definitely need to do that. Marcelo, only add my r-b tag with the version change in place.

- Nuno Sá

Quoted 5 lines · Marcelo Schmitt

The behavior of an SPI controller data output line (SDO or MOSI or COPI (Controller Output Peripheral Input) for disambiguation) is not specified when the controller is not clocking out data on SCLK edges. However, there exist SPI peripherals that require specific COPI line state when data is not being clocked out of the controller.

This is an optimisation for accelerating devices that need a specific value, really if these devices need a value they should send it.

Quoted 3 lines

#define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave mosi line low when idle */
+#define SPI_MOSI_IDLE_HIGH _BITUL(18) /* leave mosi line high when idle */

Realistically we'll have a large set of drivers that are expecting the line to be held low so I'm not sure we need that option. I would also

expect to have an implementation of these options in the core which supplies buffers with the relevant data for use with controllers that don't have the feature (similar to how _MUST_TX and _MUST_RX are done). Even without that we'd need feature detection so that drivers that try to use this aren't just buggy when used with a controller that doesn't implement it, but once you're detecting you may as well just make things work.

Quoted 14 lines · Nuno Sá

drivers/spi/spi.c int spi_setup(struct spi_device *spi) · line 3929

Quoted 8 lines · Marcelo Schmitt

(quoted diff) +2 -0

+	/* Check against conflicting MOSI idle configuration */
+	if ((spi->mode & SPI_MOSI_IDLE_LOW) && (spi->mode &

SPI_MOSI_IDLE_HIGH)) {

(quoted diff) +2 -0

+		dev_warn(&spi->dev,
+			 "setup: erratic MOSI idle configuration. Set to idle

low\n");

drivers/spi/spi.c +2 -0

+		spi->mode &= ~SPI_MOSI_IDLE_HIGH;
+	}

Should we assume such a thing? IOW, should this be treated as a warning or a real error? I would assume this should be a configuration error and return - EINVAL but...

Right, and the error message isn't very clear.

Quoted 51 lines · Nuno Sá

Quoted 49 lines · David Lechner

Quoted 42 lines · Marcelo Schmitt

Implement MOSI idle low and MOSI idle high to better support peripherals that request specific MOSI behavior.

Signed-off-by Marcelo Schmitt <[email protected]>

---
 drivers/spi/spi-axi-spi-engine.c | 8 +++++++-

 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/spi/spi-axi-spi- renamed from drivers/spi/spi-axi-spi-engine.c +0 -0

engine.c
index 0aa31d745734..549f03069d0e 100644

drivers/spi/spi-axi-spi-engine.c +1 -0

@@ -41,6 +41,7 @@
  #define SPI_ENGINE_CONFIG_CPHA			BIT(0)
  #define SPI_ENGINE_CONFIG_CPOL			BIT(1)
  #define SPI_ENGINE_CONFIG_3WIRE			BIT(2)
+#define SPI_ENGINE_CONFIG_SDO_IDLE		BIT(3)

 #define SPI_ENGINE_INST_TRANSFER 0x0
 #define SPI_ENGINE_INST_ASSERT 0x1

drivers/spi/spi-axi-spi-engine.c +4 -0

@@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct
 spi_device *spi)
  		config |= SPI_ENGINE_CONFIG_CPHA;
  	if (spi->mode & SPI_3WIRE)
  		config |= SPI_ENGINE_CONFIG_3WIRE;
+	if (spi->mode & SPI_MOSI_IDLE_HIGH)
+		config |= SPI_ENGINE_CONFIG_SDO_IDLE;
+	if (spi->mode & SPI_MOSI_IDLE_LOW)
+		config &= ~SPI_ENGINE_CONFIG_SDO_IDLE;

  return config;
 }

drivers/spi/spi-axi-spi-engine.c +0 -0

@@ -645,7 +650,8 @@ static int spi_engine_probe(struct platform_device
 *pdev)
  		return ret;

  host->dev.of_node = pdev->dev.of_node;

drivers/spi/spi-axi-spi-engine.c +1 -1

-	host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE;
+	host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE |

SPI_MOSI_IDLE_LOW

drivers/spi/spi-axi-spi-engine.c +1 -0

+			  | SPI_MOSI_IDLE_HIGH;

Quoted 3 lines

  host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32); host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2; host->transfer_one_message = spi_engine_transfer_one_message;

I think we need a version check instead of setting the flags unconditionally here since older versions of the AXI SPI Engine won't support this feature.

Quoted 4 lines

Oh, was not aware of that... Then, we definitely need to do that. Marcelo, only add my r-b tag with the version change in place.

- Nuno Sá

Actually, looking at [1], it looks like this could be a compile-time flag when the HDL is built. If it stays that way, then we would need a way to read that flag from a register instead of using the version.

[1]: https://github.com/analogdevicesinc/hdl/pull/1320#issuecomment-2145744521

Quoted 12 lines · David Lechner

Quoted 7 lines · Mark Brown

Quoted 5 lines · Marcelo Schmitt

The behavior of an SPI controller data output line (SDO or MOSI or COPI (Controller Output Peripheral Input) for disambiguation) is not specified when the controller is not clocking out data on SCLK edges. However, there exist SPI peripherals that require specific COPI line state when data is not being clocked out of the controller.

I think this description is missing a key detail that the tx data line needs to be high just before and also during the CS assertion at the start of each message.

Quoted 2 lines

And it would be helpful to have this more detailed description in the source code somewhere and not just in the commit message.

Yes, there's no way anyone could infer this from any aspect of the series. I think the properties also need a clearer name since someone might want the accelerator functionality at some point.

Quoted 12 lines

Quoted 5 lines

Even without that we'd need feature detection so that drivers that try to use this aren't just buggy when used with a controller that doesn't implement it, but once you're detecting you may as well just make things work.

I could see something like this working for controllers that leave the tx data line in the state of the last bit of a write transfer. I.e. do a write transfer of 0xff (using the smallest number of bits per word supported by the controller) with CS not asserted, then assert CS, then do the rest of actual the transfers requested by the peripheral.

Quoted 4 lines

But it doesn't seem like it would work for controllers that always return the tx data line to a low state after a write since this would mean that the data line would still be low during the CS assertion which is what we need to prevent.

With the additional requirement it's not emulatable, but we'd still need the checks in the core.

When is a driver version check needed?
Yes, older versions of SPI-Engine won't support this, but the patch set should cause no regression. Even if loading the current ad4000 driver with older SPI-Engine HDL and driver, the ADC driver would get a warn (or error?) and do what's possible without MOSI idle feature (probably only be able to do reg access) or fail probing.

As far as I searched, the definitions for SPI protocol usually don't specify any behavior for the MOSI line when the controller is not clocking out data. So, I think SPI controllers that are not capable of implementing any type of MOSI idle configuration are anyway compliant to what is usual SPI. For those that can implement such feature, I thought peripherals could request it by setting SPI mode bits.

But yeah, it's not that evident what this patch set is all about and why this is wanted so I made a wiki page to explain the reasoning for this set.
https://wiki.analog.com/software/linux/docs/spi/spi_copi_idle?rev=1717699755
Hopefully the figures with timing diagrams and transfer captures there will provide quicker understanding of this rather than I try to explain it with only text.

If you still think we need feature detection for MOSI idle capability just let me know, I'll implement what be needed.

So, if I understand correctly, it would be desirable to also have flags and descriptions for the MOSI idle configuration capabilities in include/linux/spi/spi.h.

Does the following definitions look good?
#define SPI_CONTROLLER_MOSI_IDLE_LOW BIT(8)
#define SPI_CONTROLLER_MOSI_IDLE_HIGH BIT(9)

Maybe also document the MOSI idle configuration feature in spi-summary.rst?

Quoted 5 lines · Mark Brown

Quoted 3 lines

Should we assume such a thing? IOW, should this be treated as a warning or a real error? I would assume this should be a configuration error and return - EINVAL but...

Right, and the error message isn't very clear.

Yeah, the message is not all that clear. I'll think of something better. I'm biased towards having this as a warning because I don't see this as a feature of usual SPI protocol but not really sure about either ...

Quoted 11 lines · Marcelo Schmitt

Add device tree documentation for AD4000 series of ADC devices.

Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf

Suggested-by David Lechner <[email protected]>

A suggested-by on a binding? That's unusual...

Quoted 57 lines

Signed-off-by Marcelo Schmitt <[email protected]>

---
Even though didn't pick all suggestions to the dt-bindings, I did pick most them so kept David's Suggested-by tag.

 .../bindings/iio/adc/adi,ad4000.yaml          | 207 ++++++++++++++++++
 MAINTAINERS                                   |   7 +
 2 files changed, 214 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml

Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml new file +41 -0

new file mode 100644
index 000000000000..7470d386906b
@@ -0,0 +1,207 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad4000.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD4000 and similar Analog to Digital Converters
+
+maintainers:
+  - Marcelo Schmitt <[email protected]>
+
+description: |
+  Analog Devices AD4000 family of Analog to Digital Converters with SPI support.
+  Specifications can be found at:
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - adi,ad4000
+      - adi,ad4001
+      - adi,ad4002
+      - adi,ad4003
+      - adi,ad4004
+      - adi,ad4005
+      - adi,ad4006
+      - adi,ad4007
+      - adi,ad4008
+      - adi,ad4010
+      - adi,ad4011
+      - adi,ad4020
+      - adi,ad4021
+      - adi,ad4022

Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml line 43

Quoted 2 lines

Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml new file +2 -0

+      - adi,adaq4001
+      - adi,adaq4003

Are all these actually incompatible? I'd like a note in the commit message as to why that's the case. A quick look at the driver showed that the differences in the driver between the ad402{0,1,2} are limited to the "dev_name". Same went for some other devices, like the ad40{02,06,10}.

Thanks,
Conor.

On Thu, 2024-06-06 at 18:31 -0300, Marcelo Schmitt wrote:

...

Quoted 10 lines

When is a driver version check needed?
Yes, older versions of SPI-Engine won't support this, but the patch set should cause no regression. Even if loading the current ad4000 driver with older SPI-Engine HDL and driver, the ADC driver would get a warn (or error?) and do what's possible without MOSI idle feature (probably only be able to do reg access) or fail probing.

Maybe I'm missing something but with the patchset we unconditionally set SPI_MOSI_IDLE_HIGH. So if we load an hdl which does not support it things will apparently be ok but it won't actually work, right? If I'm right we should have

a bit in a RO config_register telling us that the feature is being supported or not. That way we only set the mode bit if we do support it...

- Nuno Sá

Quoted 22 lines · Conor Dooley

Quoted 10 lines · Marcelo Schmitt

Add device tree documentation for AD4000 series of ADC devices.

Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml new file +9 -0

+properties:
+  compatible:
+    enum:
+      - adi,ad4000
+      - adi,ad4001
+      - adi,ad4002
+      - adi,ad4003
+      - adi,ad4004
+      - adi,ad4005

Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml line 43

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml new file +10 -0

+      - adi,ad4006
+      - adi,ad4007
+      - adi,ad4008
+      - adi,ad4010
+      - adi,ad4011
+      - adi,ad4020
+      - adi,ad4021
+      - adi,ad4022
+      - adi,adaq4001
+      - adi,adaq4003

Quoted 5 lines

Are all these actually incompatible? I'd like a note in the commit message as to why that's the case. A quick look at the driver showed that the differences in the driver between the ad402{0,1,2} are limited to the "dev_name". Same went for some other devices, like the ad40{02,06,10}.

Yes, that's correct. Some chips only vary by name and max sample rate which boils down to only having a different dev_name in the driver. Can those have grouped compatible strings?
dt_binding_check fails if curly brackets are used.
properties:
compatible:
enum:
- adi,ad402{0,1,2}

The groups of similar chips are:
AD4020/AD4021/AD4022
AD4003/AD4007/AD4011
AD4002/AD4006/AD4010
AD4001/AD4005
AD4000/AD4004/AD4008

Quoted 6 lines · Marcelo Schmitt

As far as I searched, the definitions for SPI protocol usually don't specify any behavior for the MOSI line when the controller is not clocking out data. So, I think SPI controllers that are not capable of implementing any type of MOSI idle configuration are anyway compliant to what is usual SPI. For those that can implement such feature, I thought peripherals could request it by setting SPI mode bits.

The issue here is the one that Richard highlighted with it not being clear exactly what the intended behaviour is.

Quoted 6 lines

But yeah, it's not that evident what this patch set is all about and why this is wanted so I made a wiki page to explain the reasoning for this set.
https://wiki.analog.com/software/linux/docs/spi/spi_copi_idle?rev=1717699755
Hopefully the figures with timing diagrams and transfer captures there will provide quicker understanding of this rather than I try to explain it with only text.

It needs to be apparent to someone looking at the kernel what the code is intended to do.

Quoted 3 lines

If you still think we need feature detection for MOSI idle capability just let me know, I'll implement what be needed.

If the devices actually require this mode then we can't just randomly ignore them when they request it.

Quoted 15 lines · Nuno Sá

On Thu, 2024-06-06 at 18:31 -0300, Marcelo Schmitt wrote:

...

Quoted 10 lines

When is a driver version check needed?
Yes, older versions of SPI-Engine won't support this, but the patch set should cause no regression. Even if loading the current ad4000 driver with older SPI-Engine HDL and driver, the ADC driver would get a warn (or error?) and do what's possible without MOSI idle feature (probably only be able to do reg access) or fail probing.

Quoted 3 lines

Maybe I'm missing something but with the patchset we unconditionally set SPI_MOSI_IDLE_HIGH. So if we load an hdl which does not support it things will apparently be ok but it won't actually work, right? If I'm right we should have

Yes, that's correct.

Quoted 2 lines

a bit in a RO config_register telling us that the feature is being supported or not. That way we only set the mode bit if we do support it...

Ok, understood. Will do it for v4.

Thanks,
Marcelo

Quoted 4 lines

- Nuno Sá

Quoted 9 lines

drivers/iio/adc/ad4000.c new file +7 -0

+
+static int ad4000_convert_and_acquire(struct ad4000_state *st)
+{
+	int ret;
+
+	/*
+	 * In 4-wire mode, the CNV line is held high for the entire

conversion

(quoted diff) +1 -0

+	 * and acquisition process. In other modes st->cnv_gpio is NULL and

drivers/iio/adc/ad4000.c line 363

Quoted 4 lines

is

drivers/iio/adc/ad4000.c new file +3 -0

+	 * ignored (CS is wired to CNV in those cases).
+	 */
+	gpiod_set_value_cansleep(st->cnv_gpio, 1);  

Not sure it's a good practise to assume internal details as you're going for GPIO. I would prefer to have an explicit check for st->cnv_gpio being NULL or not.

Quoted 20 lines

drivers/iio/adc/ad4000.c new file +16 -0

+	ret = spi_sync(st->spi, &st->msg);
+	gpiod_set_value_cansleep(st->cnv_gpio, 0);
+
+	return ret;
+}
+
 
+static int ad4000_config(struct ad4000_state *st)
+{
+	unsigned int reg_val;
+
+	if (device_property_present(&st->spi->dev, "adi,high-z-input"))
+		reg_val |= FIELD_PREP(AD4000_CFG_HIGHZ, 1);
+
+	/*
+	 * The ADC SDI pin might be connected to controller CS line in which
+	 * case the write might fail. This, however, does not prevent the

device

drivers/iio/adc/ad4000.c new file +1 -0

+	 * from functioning even though in a configuration other than the

drivers/iio/adc/ad4000.c line 545

Quoted 2 lines

drivers/iio/adc/ad4000.c new file +2 -0

+	 * requested one.
+	 */  

This raises the question if there's any way to describe that through DT (if not doing it already)? So that, if SDI is connected to CS we don't even call this? Other question that comes to mind is that in case SDI is connected to CS, will all writes fail? Because if that's the case we other writes (like scale) that won't work and we should take care of that...

drivers/iio/adc/ad4000.c line 548

Quoted 4 lines

drivers/iio/adc/ad4000.c new file +3 -0

+	return ad4000_write_reg(st, reg_val);
+}
+

Quoted 4 lines

Quoted 4 lines

(quoted diff) +2 -0

+	/*
+	 * In 4-wire mode, the CNV line is held high for the entire

conversion

(quoted diff) +1 -0

+	 * and acquisition process. In other modes st->cnv_gpio is NULL and

Quoted 8 lines

drivers/iio/adc/ad4000.c line 363

Quoted 4 lines

is

drivers/iio/adc/ad4000.c new file +3 -0

+	 * ignored (CS is wired to CNV in those cases).
+	 */
+	gpiod_set_value_cansleep(st->cnv_gpio, 1);  

Not sure it's a good practise to assume internal details as you're going for GPIO. I would prefer to have an explicit check for st->cnv_gpio being NULL or not.

Hmm. I had it in my head that this was documented behaviour, but I can't find such in the docs, so agreed checking it makes sense.

I would be very surprised if this ever changed as it's one of the things that makes optional gpios easy to work with but who knows!

Sun, Jun 09, 2024 at 10:23:54AM +0100, Jonathan Cameron kirjoitti:

...

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

(quoted diff) +2 -0

+	/*
+	 * In 4-wire mode, the CNV line is held high for the entire

conversion

(quoted diff) +1 -0

+	 * and acquisition process. In other modes st->cnv_gpio is NULL and

Quoted 8 lines

drivers/iio/adc/ad4000.c line 363

Quoted 4 lines

is

drivers/iio/adc/ad4000.c new file +3 -0

+	 * ignored (CS is wired to CNV in those cases).
+	 */
+	gpiod_set_value_cansleep(st->cnv_gpio, 1);    

Not sure it's a good practise to assume internal details as you're going for GPIO. I would prefer to have an explicit check for st->cnv_gpio being NULL or not.

Quoted 5 lines

Hmm. I had it in my head that this was documented behaviour, but I can't find such in the docs, so agreed checking it makes sense.

I would be very surprised if this ever changed as it's one of the things that makes optional gpios easy to work with but who knows!

Not Linus and not Bart, but we have tons of drivers that call GPIO APIs unconditionally as long as they want optional GPIO.

What I see here is the comment that should be rewritten to say something like

"if GPIO is defined blablabla, otherwise blablabla."

I.o.w. do not mention that implementation detail (being NULL, i.e. optional).