[PATCH v2 0/3] iio: adc: ad4695: add new regmap callbacks, timing improvements

[PATCH v2 0/3] iio: adc: ad4695: add new regmap callbacks, timing improvements

[Trevor Gamblin]

The AD4695 driver currently operates all SPI reads/writes at the speed
appropriate for register access, rather than the max rate for the bus.
Data reads should ideally operate at the latter speed, but making this
change universally makes it possible for data to be corrupted during use
and for unexpected behavior to occur on driver subsequent driver
binds/unbinds. To solve this, introduce custom regmap bus callbacks for
the driver that explicitly set a lower speed only for these operations.

The first patch in this series is a fix introduced after discovering the
corresponding issue during testing of the callbacks. This is a timing
fix that ensures the AD4695 datasheet's timing specs are met, as before
the busy signal would sometimes fail to toggle again following the end
of the conversion sequence. Adding an extra delay in the form of a blank
transfer before every CS deassert in ad4695_buffer_preenable() allows
this requirement to be met. The patch also makes similar changes in
ad4695_read_one_sample() (while also tidying that function somewhat) to
make sure that single reads are still functional with the regmap change.

The second patch is an improvement that increases the robustness of the
exit message in ad4695_exit_conversion_mode(), this time by adding a
delay before the actual exit command. This helps avoid the possibility
that the exit message will be read as data, causing corruption on some
buffered reads.

For additional context, see:
https://lore.kernel.org/linux-iio/20241028163907.00007e12@Huawei.com/

Suggested-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
Changes in v2:
- Link to v1: https://lore.kernel.org/r/20241111-tgamblin-ad4695_improvements-v1-0-698af4512635@baylibre.com

  [PATCH 1/3]
  - Fix bugs with transfer timings and sequencing in
    ad4695_buffer_preenable() and ad4695_read_one_sample() by adjusting
    xfer structs and simplifying
  - Add clearer comments explaining why the temperature channel is
    handled as-is with the advanced sequencer
  - Add David's Co-Developed-by tag

  [PATCH 2/3]
  - No change

  [PATCH 3/3]
  - Fix bug where ad4695_regmap_config was passed twice to
    devm_regmap_init() in ad4695_probe(), instead of passing
    ad4695_regmap_config once and ad4695_regmap16_config the second time

---
Trevor Gamblin (3):
      iio: adc: ad4695: fix buffered read, single sample timings
      iio: adc: ad4695: make ad4695_exit_conversion_mode() more robust
      iio: adc: ad4695: add custom regmap bus callbacks

 drivers/iio/adc/Kconfig  |   2 +-
 drivers/iio/adc/ad4695.c | 208 +++++++++++++++++++++++++++++++++++++----------
 2 files changed, 165 insertions(+), 45 deletions(-)
---
base-commit: 2d5404caa8c7bb5c4e0435f94b28834ae5456623
change-id: 20241111-tgamblin-ad4695_improvements-7a32a6268c26

Best regards,
-- 
Trevor Gamblin <tgamblin@baylibre.com>

[PATCH v2 1/3] iio: adc: ad4695: fix buffered read, single sample timings

[Trevor Gamblin]

Modify ad4695_buffer_preenable() by adding an extra SPI transfer after
each data read to help ensure that the timing requirement between the
last SCLK rising edge and the next CNV rising edge is met. This requires
a restructure of the buf_read_xfer array in ad4695_state. Also define
AD4695_T_SCK_CNV_DELAY_NS to use for each added transfer. Without this
change it is possible for the data to become corrupted on sequential
buffered reads due to the device not properly exiting conversion mode.

Similarly, make adjustments to ad4695_read_one_sample() so that timings
are respected, and clean up the function slightly in the process.

Fixes: 6cc7e4bf2e08 ("iio: adc: ad4695: implement triggered buffer")
Co-developed-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
 drivers/iio/adc/ad4695.c | 100 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 69 insertions(+), 31 deletions(-)

diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index 595ec4158e73..0146aed9069f 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -91,6 +91,7 @@
 #define AD4695_T_WAKEUP_SW_MS		3
 #define AD4695_T_REFBUF_MS		100
 #define AD4695_T_REGCONFIG_NS		20
+#define AD4695_T_SCK_CNV_DELAY_NS	80
 #define AD4695_REG_ACCESS_SCLK_HZ	(10 * MEGA)
 
 /* Max number of voltage input channels. */
@@ -132,8 +133,13 @@ struct ad4695_state {
 	unsigned int vref_mv;
 	/* Common mode input pin voltage. */
 	unsigned int com_mv;
-	/* 1 per voltage and temperature chan plus 1 xfer to trigger 1st CNV */
-	struct spi_transfer buf_read_xfer[AD4695_MAX_CHANNELS + 2];
+	/*
+	 * 2 per voltage and temperature chan plus 1 xfer to trigger 1st
+	 * CNV. Excluding the trigger xfer, every 2nd xfer only serves
+	 * to control CS and add a delay between the last SCLK and next
+	 * CNV rising edges.
+	 */
+	struct spi_transfer buf_read_xfer[AD4695_MAX_CHANNELS * 2 + 3];
 	struct spi_message buf_read_msg;
 	/* Raw conversion data received. */
 	u8 buf[ALIGN((AD4695_MAX_CHANNELS + 2) * AD4695_MAX_CHANNEL_SIZE,
@@ -423,7 +429,7 @@ static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
 	u8 temp_chan_bit = st->chip_info->num_voltage_inputs;
 	u32 bit, num_xfer, num_slots;
 	u32 temp_en = 0;
-	int ret;
+	int ret, rx_buf_offset = 0;
 
 	/*
 	 * We are using the advanced sequencer since it is the only way to read
@@ -449,11 +455,9 @@ static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
 	iio_for_each_active_channel(indio_dev, bit) {
 		xfer = &st->buf_read_xfer[num_xfer];
 		xfer->bits_per_word = 16;
-		xfer->rx_buf = &st->buf[(num_xfer - 1) * 2];
+		xfer->rx_buf = &st->buf[rx_buf_offset];
 		xfer->len = 2;
-		xfer->cs_change = 1;
-		xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
-		xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
+		rx_buf_offset += xfer->len;
 
 		if (bit == temp_chan_bit) {
 			temp_en = 1;
@@ -468,21 +472,44 @@ static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
 		}
 
 		num_xfer++;
+
+		/*
+		 * We need to add a blank xfer in data reads, to meet the timing
+		 * requirement of a minimum delay between the last SCLK rising
+		 * edge and the CS deassert.
+		 */
+		xfer = &st->buf_read_xfer[num_xfer];
+		xfer->delay.value = AD4695_T_SCK_CNV_DELAY_NS;
+		xfer->delay.unit = SPI_DELAY_UNIT_NSECS;
+		xfer->cs_change = 1;
+		xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
+		xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
+
+		num_xfer++;
 	}
 
 	/*
 	 * The advanced sequencer requires that at least 2 slots are enabled.
 	 * Since slot 0 is always used for other purposes, we need only 1
-	 * enabled voltage channel to meet this requirement. If the temperature
-	 * channel is the only enabled channel, we need to add one more slot
-	 * in the sequence but not read from it.
+	 * enabled voltage channel to meet this requirement.  If the temperature
+	 * channel is the only enabled channel, we need to add one more slot in
+	 * the sequence but not read from it. This is because the temperature
+	 * sensor is sampled at the end of the channel sequence in advanced
+	 * sequencer mode (see datasheet page 38).
+	 *
+	 * From the iio_for_each_active_channel() block above, we now have an
+	 * xfer with data followed by a blank xfer to allow us to meet the
+	 * timing spec, so move both of those up before adding an extra to
+	 * handle the temperature-only case.
 	 */
 	if (num_slots < 2) {
-		/* move last xfer so we can insert one more xfer before it */
-		st->buf_read_xfer[num_xfer] = *xfer;
+		/* Move last two xfers */
+		st->buf_read_xfer[num_xfer] = st->buf_read_xfer[num_xfer - 1];
+		st->buf_read_xfer[num_xfer - 1] = st->buf_read_xfer[num_xfer - 2];
 		num_xfer++;
 
-		/* modify 2nd to last xfer for extra slot */
+		/* Modify inserted xfer for extra slot. */
+		xfer = &st->buf_read_xfer[num_xfer - 3];
 		memset(xfer, 0, sizeof(*xfer));
 		xfer->cs_change = 1;
 		xfer->delay.value = st->chip_info->t_acq_ns;
@@ -499,6 +526,12 @@ static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
 			return ret;
 
 		num_slots++;
+
+		/*
+		 * We still want to point at the last xfer when finished, so
+		 * update the pointer.
+		 */
+		xfer = &st->buf_read_xfer[num_xfer - 1];
 	}
 
 	/*
@@ -583,8 +616,20 @@ static irqreturn_t ad4695_trigger_handler(int irq, void *p)
  */
 static int ad4695_read_one_sample(struct ad4695_state *st, unsigned int address)
 {
-	struct spi_transfer xfer[2] = { };
-	int ret, i = 0;
+	struct spi_transfer xfers[2] = {
+		{
+			.speed_hz = AD4695_REG_ACCESS_SCLK_HZ,
+			.bits_per_word = 16,
+			.tx_buf = &st->cnv_cmd,
+			.len = 2,
+		},
+		{
+			/* Required delay between last SCLK and CNV/CS */
+			.delay.value = AD4695_T_SCK_CNV_DELAY_NS,
+			.delay.unit = SPI_DELAY_UNIT_NSECS,
+		}
+	};
+	int ret;
 
 	ret = ad4695_set_single_cycle_mode(st, address);
 	if (ret)
@@ -592,29 +637,22 @@ static int ad4695_read_one_sample(struct ad4695_state *st, unsigned int address)
 
 	/*
 	 * Setting the first channel to the temperature channel isn't supported
-	 * in single-cycle mode, so we have to do an extra xfer to read the
-	 * temperature.
+	 * in single-cycle mode, so we have to do an extra conversion to read
+	 * the temperature.
 	 */
 	if (address == AD4695_CMD_TEMP_CHAN) {
-		/* We aren't reading, so we can make this a short xfer. */
-		st->cnv_cmd2 = AD4695_CMD_TEMP_CHAN << 3;
-		xfer[0].tx_buf = &st->cnv_cmd2;
-		xfer[0].len = 1;
-		xfer[0].cs_change = 1;
-		xfer[0].cs_change_delay.value = AD4695_T_CONVERT_NS;
-		xfer[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
-
-		i = 1;
+		st->cnv_cmd = AD4695_CMD_TEMP_CHAN << 11;
+
+		ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+		if (ret)
+			return ret;
 	}
 
 	/* Then read the result and exit conversion mode. */
 	st->cnv_cmd = AD4695_CMD_EXIT_CNV_MODE << 11;
-	xfer[i].bits_per_word = 16;
-	xfer[i].tx_buf = &st->cnv_cmd;
-	xfer[i].rx_buf = &st->raw_data;
-	xfer[i].len = 2;
+	xfers[0].rx_buf = &st->raw_data;
 
-	return spi_sync_transfer(st->spi, xfer, i + 1);
+	return spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
 }
 
 static int ad4695_read_raw(struct iio_dev *indio_dev,

-- 
2.39.5

Re: [PATCH v2 1/3] iio: adc: ad4695: fix buffered read, single sample timings

[Jonathan Cameron]

On Wed, 13 Nov 2024 15:52:58 -0500
Trevor Gamblin <tgamblin@baylibre.com> wrote:

> Modify ad4695_buffer_preenable() by adding an extra SPI transfer after
> each data read to help ensure that the timing requirement between the
> last SCLK rising edge and the next CNV rising edge is met. This requires
> a restructure of the buf_read_xfer array in ad4695_state. Also define
> AD4695_T_SCK_CNV_DELAY_NS to use for each added transfer. Without this
> change it is possible for the data to become corrupted on sequential
> buffered reads due to the device not properly exiting conversion mode.
> 
> Similarly, make adjustments to ad4695_read_one_sample() so that timings
> are respected, and clean up the function slightly in the process.
> 
> Fixes: 6cc7e4bf2e08 ("iio: adc: ad4695: implement triggered buffer")
> Co-developed-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
Applied to the fixes-togreg branch of iio.git. I'll rebase that
and send out a pull shortly after rc1.

The other two will have to wait for that to cycle around to be
in the upstream for my togreg branch later in the cycle.

Thanks,

Jonathan

>  drivers/iio/adc/ad4695.c | 100 ++++++++++++++++++++++++++++++++---------------
>  1 file changed, 69 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
> index 595ec4158e73..0146aed9069f 100644
> --- a/drivers/iio/adc/ad4695.c
> +++ b/drivers/iio/adc/ad4695.c
> @@ -91,6 +91,7 @@
>  #define AD4695_T_WAKEUP_SW_MS		3
>  #define AD4695_T_REFBUF_MS		100
>  #define AD4695_T_REGCONFIG_NS		20
> +#define AD4695_T_SCK_CNV_DELAY_NS	80
>  #define AD4695_REG_ACCESS_SCLK_HZ	(10 * MEGA)
>  
>  /* Max number of voltage input channels. */
> @@ -132,8 +133,13 @@ struct ad4695_state {
>  	unsigned int vref_mv;
>  	/* Common mode input pin voltage. */
>  	unsigned int com_mv;
> -	/* 1 per voltage and temperature chan plus 1 xfer to trigger 1st CNV */
> -	struct spi_transfer buf_read_xfer[AD4695_MAX_CHANNELS + 2];
> +	/*
> +	 * 2 per voltage and temperature chan plus 1 xfer to trigger 1st
> +	 * CNV. Excluding the trigger xfer, every 2nd xfer only serves
> +	 * to control CS and add a delay between the last SCLK and next
> +	 * CNV rising edges.
> +	 */
> +	struct spi_transfer buf_read_xfer[AD4695_MAX_CHANNELS * 2 + 3];
>  	struct spi_message buf_read_msg;
>  	/* Raw conversion data received. */
>  	u8 buf[ALIGN((AD4695_MAX_CHANNELS + 2) * AD4695_MAX_CHANNEL_SIZE,
> @@ -423,7 +429,7 @@ static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
>  	u8 temp_chan_bit = st->chip_info->num_voltage_inputs;
>  	u32 bit, num_xfer, num_slots;
>  	u32 temp_en = 0;
> -	int ret;
> +	int ret, rx_buf_offset = 0;
>  
>  	/*
>  	 * We are using the advanced sequencer since it is the only way to read
> @@ -449,11 +455,9 @@ static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
>  	iio_for_each_active_channel(indio_dev, bit) {
>  		xfer = &st->buf_read_xfer[num_xfer];
>  		xfer->bits_per_word = 16;
> -		xfer->rx_buf = &st->buf[(num_xfer - 1) * 2];
> +		xfer->rx_buf = &st->buf[rx_buf_offset];
>  		xfer->len = 2;
> -		xfer->cs_change = 1;
> -		xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
> -		xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
> +		rx_buf_offset += xfer->len;
>  
>  		if (bit == temp_chan_bit) {
>  			temp_en = 1;
> @@ -468,21 +472,44 @@ static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
>  		}
>  
>  		num_xfer++;
> +
> +		/*
> +		 * We need to add a blank xfer in data reads, to meet the timing
> +		 * requirement of a minimum delay between the last SCLK rising
> +		 * edge and the CS deassert.
> +		 */
> +		xfer = &st->buf_read_xfer[num_xfer];
> +		xfer->delay.value = AD4695_T_SCK_CNV_DELAY_NS;
> +		xfer->delay.unit = SPI_DELAY_UNIT_NSECS;
> +		xfer->cs_change = 1;
> +		xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
> +		xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> +		num_xfer++;
>  	}
>  
>  	/*
>  	 * The advanced sequencer requires that at least 2 slots are enabled.
>  	 * Since slot 0 is always used for other purposes, we need only 1
> -	 * enabled voltage channel to meet this requirement. If the temperature
> -	 * channel is the only enabled channel, we need to add one more slot
> -	 * in the sequence but not read from it.
> +	 * enabled voltage channel to meet this requirement.  If the temperature
> +	 * channel is the only enabled channel, we need to add one more slot in
> +	 * the sequence but not read from it. This is because the temperature
> +	 * sensor is sampled at the end of the channel sequence in advanced
> +	 * sequencer mode (see datasheet page 38).
> +	 *
> +	 * From the iio_for_each_active_channel() block above, we now have an
> +	 * xfer with data followed by a blank xfer to allow us to meet the
> +	 * timing spec, so move both of those up before adding an extra to
> +	 * handle the temperature-only case.
>  	 */
>  	if (num_slots < 2) {
> -		/* move last xfer so we can insert one more xfer before it */
> -		st->buf_read_xfer[num_xfer] = *xfer;
> +		/* Move last two xfers */
> +		st->buf_read_xfer[num_xfer] = st->buf_read_xfer[num_xfer - 1];
> +		st->buf_read_xfer[num_xfer - 1] = st->buf_read_xfer[num_xfer - 2];
>  		num_xfer++;
>  
> -		/* modify 2nd to last xfer for extra slot */
> +		/* Modify inserted xfer for extra slot. */
> +		xfer = &st->buf_read_xfer[num_xfer - 3];
>  		memset(xfer, 0, sizeof(*xfer));
>  		xfer->cs_change = 1;
>  		xfer->delay.value = st->chip_info->t_acq_ns;
> @@ -499,6 +526,12 @@ static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
>  			return ret;
>  
>  		num_slots++;
> +
> +		/*
> +		 * We still want to point at the last xfer when finished, so
> +		 * update the pointer.
> +		 */
> +		xfer = &st->buf_read_xfer[num_xfer - 1];
>  	}
>  
>  	/*
> @@ -583,8 +616,20 @@ static irqreturn_t ad4695_trigger_handler(int irq, void *p)
>   */
>  static int ad4695_read_one_sample(struct ad4695_state *st, unsigned int address)
>  {
> -	struct spi_transfer xfer[2] = { };
> -	int ret, i = 0;
> +	struct spi_transfer xfers[2] = {
> +		{
> +			.speed_hz = AD4695_REG_ACCESS_SCLK_HZ,
> +			.bits_per_word = 16,
> +			.tx_buf = &st->cnv_cmd,
> +			.len = 2,
> +		},
> +		{
> +			/* Required delay between last SCLK and CNV/CS */
> +			.delay.value = AD4695_T_SCK_CNV_DELAY_NS,
> +			.delay.unit = SPI_DELAY_UNIT_NSECS,
> +		}
> +	};
> +	int ret;
>  
>  	ret = ad4695_set_single_cycle_mode(st, address);
>  	if (ret)
> @@ -592,29 +637,22 @@ static int ad4695_read_one_sample(struct ad4695_state *st, unsigned int address)
>  
>  	/*
>  	 * Setting the first channel to the temperature channel isn't supported
> -	 * in single-cycle mode, so we have to do an extra xfer to read the
> -	 * temperature.
> +	 * in single-cycle mode, so we have to do an extra conversion to read
> +	 * the temperature.
>  	 */
>  	if (address == AD4695_CMD_TEMP_CHAN) {
> -		/* We aren't reading, so we can make this a short xfer. */
> -		st->cnv_cmd2 = AD4695_CMD_TEMP_CHAN << 3;
> -		xfer[0].tx_buf = &st->cnv_cmd2;
> -		xfer[0].len = 1;
> -		xfer[0].cs_change = 1;
> -		xfer[0].cs_change_delay.value = AD4695_T_CONVERT_NS;
> -		xfer[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
> -
> -		i = 1;
> +		st->cnv_cmd = AD4695_CMD_TEMP_CHAN << 11;
> +
> +		ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> +		if (ret)
> +			return ret;
>  	}
>  
>  	/* Then read the result and exit conversion mode. */
>  	st->cnv_cmd = AD4695_CMD_EXIT_CNV_MODE << 11;
> -	xfer[i].bits_per_word = 16;
> -	xfer[i].tx_buf = &st->cnv_cmd;
> -	xfer[i].rx_buf = &st->raw_data;
> -	xfer[i].len = 2;
> +	xfers[0].rx_buf = &st->raw_data;
>  
> -	return spi_sync_transfer(st->spi, xfer, i + 1);
> +	return spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
>  }
>  
>  static int ad4695_read_raw(struct iio_dev *indio_dev,
> 

Re: [PATCH v2 1/3] iio: adc: ad4695: fix buffered read, single sample timings

[Trevor Gamblin]

On 2024-11-24 07:52, Jonathan Cameron wrote:
> On Wed, 13 Nov 2024 15:52:58 -0500
> Trevor Gamblin <tgamblin@baylibre.com> wrote:
>
>> Modify ad4695_buffer_preenable() by adding an extra SPI transfer after
>> each data read to help ensure that the timing requirement between the
>> last SCLK rising edge and the next CNV rising edge is met. This requires
>> a restructure of the buf_read_xfer array in ad4695_state. Also define
>> AD4695_T_SCK_CNV_DELAY_NS to use for each added transfer. Without this
>> change it is possible for the data to become corrupted on sequential
>> buffered reads due to the device not properly exiting conversion mode.
>>
>> Similarly, make adjustments to ad4695_read_one_sample() so that timings
>> are respected, and clean up the function slightly in the process.
>>
>> Fixes: 6cc7e4bf2e08 ("iio: adc: ad4695: implement triggered buffer")
>> Co-developed-by: David Lechner <dlechner@baylibre.com>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
>> ---
> Applied to the fixes-togreg branch of iio.git. I'll rebase that
> and send out a pull shortly after rc1.
>
> The other two will have to wait for that to cycle around to be
> in the upstream for my togreg branch later in the cycle.
>
> Thanks,
>
> Jonathan
Thank you!

[PATCH v2 2/3] iio: adc: ad4695: make ad4695_exit_conversion_mode() more robust

[Trevor Gamblin]

Ensure that conversion mode is successfully exited when the command is
issued by adding an extra transfer beforehand, matching the minimum CNV
high and low times from the AD4695 datasheet. The AD4695 has a quirk
where the exit command only works during a conversion, so guarantee this
happens by triggering a conversion in ad4695_exit_conversion_mode().
Then make this even more robust by ensuring that the exit command is run
at AD4695_REG_ACCESS_SCLK_HZ rather than the bus maximum.

Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
 drivers/iio/adc/ad4695.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index 0146aed9069f..4bc800293e60 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -92,6 +92,8 @@
 #define AD4695_T_REFBUF_MS		100
 #define AD4695_T_REGCONFIG_NS		20
 #define AD4695_T_SCK_CNV_DELAY_NS	80
+#define AD4695_T_CNVL_NS		80
+#define AD4695_T_CNVH_NS		10
 #define AD4695_REG_ACCESS_SCLK_HZ	(10 * MEGA)
 
 /* Max number of voltage input channels. */
@@ -364,11 +366,31 @@ static int ad4695_enter_advanced_sequencer_mode(struct ad4695_state *st, u32 n)
  */
 static int ad4695_exit_conversion_mode(struct ad4695_state *st)
 {
-	struct spi_transfer xfer = {
-		.tx_buf = &st->cnv_cmd2,
-		.len = 1,
-		.delay.value = AD4695_T_REGCONFIG_NS,
-		.delay.unit = SPI_DELAY_UNIT_NSECS,
+	/*
+	 * An extra transfer is needed to trigger a conversion here so
+	 * that we can be 100% sure the command will be processed by the
+	 * ADC, rather than relying on it to be in the correct state
+	 * when this function is called (this chip has a quirk where the
+	 * command only works when reading a conversion, and if the
+	 * previous conversion was already read then it won't work). The
+	 * actual conversion command is then run at the slower
+	 * AD4695_REG_ACCESS_SCLK_HZ speed to guarantee this works.
+	 */
+	struct spi_transfer xfers[] = {
+		{
+			.delay.value = AD4695_T_CNVL_NS,
+			.delay.unit = SPI_DELAY_UNIT_NSECS,
+			.cs_change = 1,
+			.cs_change_delay.value = AD4695_T_CNVH_NS,
+			.cs_change_delay.unit = SPI_DELAY_UNIT_NSECS,
+		},
+		{
+			.speed_hz = AD4695_REG_ACCESS_SCLK_HZ,
+			.tx_buf = &st->cnv_cmd2,
+			.len = 1,
+			.delay.value = AD4695_T_REGCONFIG_NS,
+			.delay.unit = SPI_DELAY_UNIT_NSECS,
+		},
 	};
 
 	/*
@@ -377,7 +399,7 @@ static int ad4695_exit_conversion_mode(struct ad4695_state *st)
 	 */
 	st->cnv_cmd2 = AD4695_CMD_EXIT_CNV_MODE << 3;
 
-	return spi_sync_transfer(st->spi, &xfer, 1);
+	return spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
 }
 
 static int ad4695_set_ref_voltage(struct ad4695_state *st, int vref_mv)

-- 
2.39.5

Re: [PATCH v2 2/3] iio: adc: ad4695: make ad4695_exit_conversion_mode() more robust

[Jonathan Cameron]

On Wed, 13 Nov 2024 15:52:59 -0500
Trevor Gamblin <tgamblin@baylibre.com> wrote:

> Ensure that conversion mode is successfully exited when the command is
> issued by adding an extra transfer beforehand, matching the minimum CNV
> high and low times from the AD4695 datasheet. The AD4695 has a quirk
> where the exit command only works during a conversion, so guarantee this
> happens by triggering a conversion in ad4695_exit_conversion_mode().
> Then make this even more robust by ensuring that the exit command is run
> at AD4695_REG_ACCESS_SCLK_HZ rather than the bus maximum.
> 
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
Applied 2 and 3 to the testing branch of iio.git.
I'll be rebasing that on rc1 once available.

Thanks,

Jonathan

Re: [PATCH v2 2/3] iio: adc: ad4695: make ad4695_exit_conversion_mode() more robust

[Trevor Gamblin]

On 2025-01-18 12:12, Jonathan Cameron wrote:
> On Wed, 13 Nov 2024 15:52:59 -0500
> Trevor Gamblin <tgamblin@baylibre.com> wrote:
>
>> Ensure that conversion mode is successfully exited when the command is
>> issued by adding an extra transfer beforehand, matching the minimum CNV
>> high and low times from the AD4695 datasheet. The AD4695 has a quirk
>> where the exit command only works during a conversion, so guarantee this
>> happens by triggering a conversion in ad4695_exit_conversion_mode().
>> Then make this even more robust by ensuring that the exit command is run
>> at AD4695_REG_ACCESS_SCLK_HZ rather than the bus maximum.
>>
>> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> Applied 2 and 3 to the testing branch of iio.git.
> I'll be rebasing that on rc1 once available.
>
> Thanks,
>
> Jonathan
Thank you!

[PATCH v2 3/3] iio: adc: ad4695: add custom regmap bus callbacks

[Trevor Gamblin]

Add a custom implementation of regmap read/write callbacks using the SPI
bus. This allows them to be performed at a lower SCLK rate than data
reads. Previously, all SPI transfers were being performed at a lower
speed, but with this change sample data is read at the max bus speed
while the register reads/writes remain at the lower rate.

Also remove .can_multi_write from the AD4695 driver's regmap_configs, as
this isn't implemented or needed.

For some background context, see:

https://lore.kernel.org/linux-iio/20241028163907.00007e12@Huawei.com/

Suggested-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
 drivers/iio/adc/Kconfig  |  2 +-
 drivers/iio/adc/ad4695.c | 74 +++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 68 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 6c4e74420fd2..e0f9d01ce37d 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -51,9 +51,9 @@ config AD4130
 config AD4695
 	tristate "Analog Device AD4695 ADC Driver"
 	depends on SPI
-	select REGMAP_SPI
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
+	select REGMAP
 	help
 	  Say yes here to build support for Analog Devices AD4695 and similar
 	  analog to digital converters (ADC).
diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index 4bc800293e60..921d996fff79 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -150,6 +150,8 @@ struct ad4695_state {
 	/* Commands to send for single conversion. */
 	u16 cnv_cmd;
 	u8 cnv_cmd2;
+	/* Buffer for storing data from regmap bus reads/writes */
+	u8 regmap_bus_data[4];
 };
 
 static const struct regmap_range ad4695_regmap_rd_ranges[] = {
@@ -194,7 +196,6 @@ static const struct regmap_config ad4695_regmap_config = {
 	.max_register = AD4695_REG_AS_SLOT(127),
 	.rd_table = &ad4695_regmap_rd_table,
 	.wr_table = &ad4695_regmap_wr_table,
-	.can_multi_write = true,
 };
 
 static const struct regmap_range ad4695_regmap16_rd_ranges[] = {
@@ -226,7 +227,67 @@ static const struct regmap_config ad4695_regmap16_config = {
 	.max_register = AD4695_REG_GAIN_IN(15),
 	.rd_table = &ad4695_regmap16_rd_table,
 	.wr_table = &ad4695_regmap16_wr_table,
-	.can_multi_write = true,
+};
+
+static int ad4695_regmap_bus_reg_write(void *context, const void *data,
+				       size_t count)
+{
+	struct ad4695_state *st = context;
+	struct spi_transfer xfer = {
+			.speed_hz = AD4695_REG_ACCESS_SCLK_HZ,
+			.len = count,
+			.tx_buf = st->regmap_bus_data,
+	};
+
+	if (count > ARRAY_SIZE(st->regmap_bus_data))
+		return -EINVAL;
+
+	memcpy(st->regmap_bus_data, data, count);
+
+	return spi_sync_transfer(st->spi, &xfer, 1);
+}
+
+static int ad4695_regmap_bus_reg_read(void *context, const void *reg,
+				      size_t reg_size, void *val,
+				      size_t val_size)
+{
+	struct ad4695_state *st = context;
+	struct spi_transfer xfers[] = {
+		{
+			.speed_hz = AD4695_REG_ACCESS_SCLK_HZ,
+			.len = reg_size,
+			.tx_buf = &st->regmap_bus_data[0],
+		}, {
+			.speed_hz = AD4695_REG_ACCESS_SCLK_HZ,
+			.len = val_size,
+			.rx_buf = &st->regmap_bus_data[2],
+		},
+	};
+	int ret;
+
+	if (reg_size > 2)
+		return -EINVAL;
+
+	if (val_size > 2)
+		return -EINVAL;
+
+	memcpy(&st->regmap_bus_data[0], reg, reg_size);
+
+	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+	if (ret)
+		return ret;
+
+	memcpy(val, &st->regmap_bus_data[2], val_size);
+
+	return 0;
+}
+
+static const struct regmap_bus ad4695_regmap_bus = {
+	.write = ad4695_regmap_bus_reg_write,
+	.read = ad4695_regmap_bus_reg_read,
+	.read_flag_mask = 0x80,
+	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
+	.val_format_endian_default = REGMAP_ENDIAN_BIG,
 };
 
 static const struct iio_chan_spec ad4695_channel_template = {
@@ -1061,15 +1122,14 @@ static int ad4695_probe(struct spi_device *spi)
 	if (!st->chip_info)
 		return -EINVAL;
 
-	/* Registers cannot be read at the max allowable speed */
-	spi->max_speed_hz = AD4695_REG_ACCESS_SCLK_HZ;
-
-	st->regmap = devm_regmap_init_spi(spi, &ad4695_regmap_config);
+	st->regmap = devm_regmap_init(dev, &ad4695_regmap_bus, st,
+				      &ad4695_regmap_config);
 	if (IS_ERR(st->regmap))
 		return dev_err_probe(dev, PTR_ERR(st->regmap),
 				     "Failed to initialize regmap\n");
 
-	st->regmap16 = devm_regmap_init_spi(spi, &ad4695_regmap16_config);
+	st->regmap16 = devm_regmap_init(dev, &ad4695_regmap_bus, st,
+					&ad4695_regmap16_config);
 	if (IS_ERR(st->regmap16))
 		return dev_err_probe(dev, PTR_ERR(st->regmap16),
 				     "Failed to initialize regmap16\n");

-- 
2.39.5

Re: [PATCH v2 0/3] iio: adc: ad4695: add new regmap callbacks, timing improvements

[David Lechner]

On 11/13/24 2:52 PM, Trevor Gamblin wrote:
> The AD4695 driver currently operates all SPI reads/writes at the speed
> appropriate for register access, rather than the max rate for the bus.
> Data reads should ideally operate at the latter speed, but making this
> change universally makes it possible for data to be corrupted during use
> and for unexpected behavior to occur on driver subsequent driver
> binds/unbinds. To solve this, introduce custom regmap bus callbacks for
> the driver that explicitly set a lower speed only for these operations.
> 
> The first patch in this series is a fix introduced after discovering the
> corresponding issue during testing of the callbacks. This is a timing
> fix that ensures the AD4695 datasheet's timing specs are met, as before
> the busy signal would sometimes fail to toggle again following the end
> of the conversion sequence. Adding an extra delay in the form of a blank
> transfer before every CS deassert in ad4695_buffer_preenable() allows
> this requirement to be met. The patch also makes similar changes in
> ad4695_read_one_sample() (while also tidying that function somewhat) to
> make sure that single reads are still functional with the regmap change.
> 
> The second patch is an improvement that increases the robustness of the
> exit message in ad4695_exit_conversion_mode(), this time by adding a
> delay before the actual exit command. This helps avoid the possibility
> that the exit message will be read as data, causing corruption on some
> buffered reads.
> 
> For additional context, see:
> https://lore.kernel.org/linux-iio/20241028163907.00007e12@Huawei.com/
> 
> Suggested-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
Reviewed-by: David Lechner <dlechner@baylibre.com>
Tested-by: David Lechner <dlechner@baylibre.com>