[PATCH 0/4] Fixes for ad7949

[PATCH 0/4] Fixes for ad7949

[Andrea Merello]

This patch series fixes ad7949 driver incorrectly read data, simplify the
code, and enforces device timing constraints.

This has been tested on a UltraZed SOM + a custom carrier equipped with
several AD7689 A/Ds. Patches have been developed on a Xilinx upstream
kernel and then rebased on linux-next kernel.

Andrea Merello (4):
  iio: ad7949: kill pointless "readback"-handling code
  iio: ad7949: fix incorrect SPI xfer len
  iio: ad7949: fix SPI xfer delays
  iio: ad7949: fix channels mixups

 drivers/iio/adc/ad7949.c | 64 +++++++++++++---------------------------
 1 file changed, 21 insertions(+), 43 deletions(-)

--
2.17.1

[PATCH 1/4] iio: ad7949: kill pointless "readback"-handling code

[Andrea Merello]

The device could be configured to spit out also the configuration word
while reading the AD result value (in the same SPI xfer) - this is called
"readback" in the device datasheet.

The driver checks if readback is enabled and it eventually adjusts the SPI
xfer length and it applies proper shifts to still get the data, discarding
the configuration word.

The readback option is actually never enabled (the driver disables it), so
the said checks do not serve for any purpose.

Since enabling the readback option seems not to provide any advantage (the
driver entirely sets the configuration word without relying on any default
value), just kill the said, unused, code.

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 drivers/iio/adc/ad7949.c | 27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index ac0ffff6c5ae..518044c31a73 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -57,29 +57,11 @@ struct ad7949_adc_chip {
 	u32 buffer ____cacheline_aligned;
 };
 
-static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
-{
-	if (!(ad7949_adc->cfg & AD7949_CFG_READ_BACK))
-		return true;
-
-	return false;
-}
-
-static int ad7949_spi_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
-{
-	int ret = ad7949_adc->resolution;
-
-	if (ad7949_spi_cfg_is_read_back(ad7949_adc))
-		ret += AD7949_CFG_REG_SIZE_BITS;
-
-	return ret;
-}
-
 static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
 				u16 mask)
 {
 	int ret;
-	int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
+	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[] = {
@@ -107,7 +89,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 				   unsigned int channel)
 {
 	int ret;
-	int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
+	int bits_per_word = ad7949_adc->resolution;
 	int mask = GENMASK(ad7949_adc->resolution, 0);
 	struct spi_message msg;
 	struct spi_transfer tx[] = {
@@ -138,10 +120,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 
 	ad7949_adc->current_channel = channel;
 
-	if (ad7949_spi_cfg_is_read_back(ad7949_adc))
-		*val = (ad7949_adc->buffer >> AD7949_CFG_REG_SIZE_BITS) & mask;
-	else
-		*val = ad7949_adc->buffer & mask;
+	*val = ad7949_adc->buffer & mask;
 
 	return 0;
 }
-- 
2.17.1

[PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len

[Andrea Merello]

This driver supports 14-bits and 16-bits devices. All of them have a 14-bit
configuration registers. All SPI trasfers, for reading AD conversion
results and for writing the configuration register, fit in two bytes.

The driver always uses 4-bytes xfers which seems at least pointless (maybe
even harmful). This patch trims the SPI xfer len and the buffer size to
two bytes.

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 drivers/iio/adc/ad7949.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 518044c31a73..5c2b3446fa4a 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -54,7 +54,7 @@ struct ad7949_adc_chip {
 	u8 resolution;
 	u16 cfg;
 	unsigned int current_channel;
-	u32 buffer ____cacheline_aligned;
+	u16 buffer ____cacheline_aligned;
 };
 
 static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
@@ -67,7 +67,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
 	struct spi_transfer tx[] = {
 		{
 			.tx_buf = &ad7949_adc->buffer,
-			.len = 4,
+			.len = 2,
 			.bits_per_word = bits_per_word,
 		},
 	};
@@ -95,7 +95,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	struct spi_transfer tx[] = {
 		{
 			.rx_buf = &ad7949_adc->buffer,
-			.len = 4,
+			.len = 2,
 			.bits_per_word = bits_per_word,
 		},
 	};
-- 
2.17.1

[PATCH 3/4] iio: ad7949: fix SPI xfer delays

[Andrea Merello]

The driver calls udelay(2) after each SPI xfer. However, according to
the specifications, the SPI timing should be as follows:

1- The end of SPI xfer (CNV/CS rising edge) causes the device to initiate
   the conversion phase, which takes up to 2.2uS.

2- At the end of the conversion phase, the device starts the acquisition
   phase for the next conversion automatically (regardless to the state of
   CNV pin); the conversion phase should last at least 1.8 uS

The whole cycle timing is thus 4uS long. The SPI data is read during the
acquisition phase (RAC mode, no need to worry about "Tdata").

In order to be compliant wrt these timing specifications we should wait
4uS after each SPI xfer (that is conservative, because there is also the
SPI xfer duration itself - which at the maximum supported clock should be
about 320nS).

This patch enlarges the delay up to 4uS and it also removes the explicit
calls to udelay(), relying on spi_transfer->delay_usecs.

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 drivers/iio/adc/ad7949.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 5c2b3446fa4a..25d1e1b24257 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -69,6 +69,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
 			.tx_buf = &ad7949_adc->buffer,
 			.len = 2,
 			.bits_per_word = bits_per_word,
+			.delay_usecs = 4,
 		},
 	};
 
@@ -77,11 +78,6 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
 	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
-	 * send a new command to the device
-	 */
-	udelay(2);
 	return ret;
 }
 
@@ -97,6 +93,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 			.rx_buf = &ad7949_adc->buffer,
 			.len = 2,
 			.bits_per_word = bits_per_word,
+			.delay_usecs = 4,
 		},
 	};
 
@@ -112,12 +109,6 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	if (ret)
 		return ret;
 
-	/*
-	 * This delay is to avoid a new request before the required time to
-	 * send a new command to the device
-	 */
-	udelay(2);
-
 	ad7949_adc->current_channel = channel;
 
 	*val = ad7949_adc->buffer & mask;
-- 
2.17.1

[PATCH 4/4] iio: ad7949: fix channels mixups

[Andrea Merello]

Each time we need to read a sample the driver writes the CFG register
(setting the channel to be read in such register) and then it performs
another xfer to read the resulting value.

This does not work correctly because while writing the CFG register the
acquisition phase is ongoing using the _previous_ CFG settings. Then the
device performs the conversion during xfer delay on the voltage stored
duting the said acquisitaion phase. Finally the driver performs the read
(during the next acquisition phase, which is the one done with the right
settings) and it gets the last converted value, that is the wrong data.

In case the configuration is not actually changed, then we still get
correct data, but in case the configuration changes (and this happens e.g.
switching the MUX on another channel), we get wrong data (data from the
previously selected channel).

This patch fixes this by performing one more "dummy" transfer in order to
ending up in reading the data when it's really ready.

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 drivers/iio/adc/ad7949.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 25d1e1b24257..b1dbe2075ca9 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -85,6 +85,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 				   unsigned int channel)
 {
 	int ret;
+	int i;
 	int bits_per_word = ad7949_adc->resolution;
 	int mask = GENMASK(ad7949_adc->resolution, 0);
 	struct spi_message msg;
@@ -97,12 +98,19 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 		},
 	};
 
-	ret = ad7949_spi_write_cfg(ad7949_adc,
-				   channel << AD7949_OFFSET_CHANNEL_SEL,
-				   AD7949_MASK_CHANNEL_SEL);
-	if (ret)
-		return ret;
+	/*
+	 * 1: write CFG for sample 'n' and read garbage (sample n-2)
+	 * 2: write something and read garbage (sample n-1)
+	 */
+	for (i = 0; i < 2; i++) {
+		ret = ad7949_spi_write_cfg(ad7949_adc,
+					   channel << AD7949_OFFSET_CHANNEL_SEL,
+					   AD7949_MASK_CHANNEL_SEL);
+		if (ret)
+			return ret;
+	}
 
+	/* 3: write something and read data for sample 'n' */
 	ad7949_adc->buffer = 0;
 	spi_message_init_with_transfers(&msg, tx, 1);
 	ret = spi_sync(ad7949_adc->spi, &msg);
-- 
2.17.1

Re: [PATCH 1/4] iio: ad7949: kill pointless "readback"-handling code

[Ardelean, Alexandru]

On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:
> [External]
> 
> The device could be configured to spit out also the configuration word
> while reading the AD result value (in the same SPI xfer) - this is called
> "readback" in the device datasheet.
> 
> The driver checks if readback is enabled and it eventually adjusts the SPI
> xfer length and it applies proper shifts to still get the data, discarding
> the configuration word.
> 
> The readback option is actually never enabled (the driver disables it), so
> the said checks do not serve for any purpose.
> 
> Since enabling the readback option seems not to provide any advantage (the
> driver entirely sets the configuration word without relying on any default
> value), just kill the said, unused, code.

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

> 
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> ---
>  drivers/iio/adc/ad7949.c | 27 +++------------------------
>  1 file changed, 3 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index ac0ffff6c5ae..518044c31a73 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -57,29 +57,11 @@ struct ad7949_adc_chip {
>  	u32 buffer ____cacheline_aligned;
>  };
>  
> -static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
> -{
> -	if (!(ad7949_adc->cfg & AD7949_CFG_READ_BACK))
> -		return true;
> -
> -	return false;
> -}
> -
> -static int ad7949_spi_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
> -{
> -	int ret = ad7949_adc->resolution;
> -
> -	if (ad7949_spi_cfg_is_read_back(ad7949_adc))
> -		ret += AD7949_CFG_REG_SIZE_BITS;
> -
> -	return ret;
> -}
> -
>  static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
>  				u16 mask)
>  {
>  	int ret;
> -	int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
> +	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[] = {
> @@ -107,7 +89,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>  				   unsigned int channel)
>  {
>  	int ret;
> -	int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
> +	int bits_per_word = ad7949_adc->resolution;
>  	int mask = GENMASK(ad7949_adc->resolution, 0);
>  	struct spi_message msg;
>  	struct spi_transfer tx[] = {
> @@ -138,10 +120,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>  
>  	ad7949_adc->current_channel = channel;
>  
> -	if (ad7949_spi_cfg_is_read_back(ad7949_adc))
> -		*val = (ad7949_adc->buffer >> AD7949_CFG_REG_SIZE_BITS) & mask;
> -	else
> -		*val = ad7949_adc->buffer & mask;
> +	*val = ad7949_adc->buffer & mask;
>  
>  	return 0;
>  }

Re: [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len

[Ardelean, Alexandru]

On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:
> [External]
> 
> This driver supports 14-bits and 16-bits devices. All of them have a 14-bit
> configuration registers. All SPI trasfers, for reading AD conversion
> results and for writing the configuration register, fit in two bytes.
> 
> The driver always uses 4-bytes xfers which seems at least pointless (maybe
> even harmful). This patch trims the SPI xfer len and the buffer size to
> two bytes.
> 

The length reduction proposal is fine.

But, this patch raises a question about endianess.
I'm actually wondering here if we need to see about maybe using a __be16 vs u16.

I'm not that kernel-savy yet about some of these low-level things to be completely sure here.
So, I'd let someone else maybe handle it.

Thanks
Alex 

> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> ---
>  drivers/iio/adc/ad7949.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index 518044c31a73..5c2b3446fa4a 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -54,7 +54,7 @@ struct ad7949_adc_chip {
>  	u8 resolution;
>  	u16 cfg;
>  	unsigned int current_channel;
> -	u32 buffer ____cacheline_aligned;
> +	u16 buffer ____cacheline_aligned;
>  };
>  
>  static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> @@ -67,7 +67,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
>  	struct spi_transfer tx[] = {
>  		{
>  			.tx_buf = &ad7949_adc->buffer,
> -			.len = 4,
> +			.len = 2,
>  			.bits_per_word = bits_per_word,
>  		},
>  	};
> @@ -95,7 +95,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>  	struct spi_transfer tx[] = {
>  		{
>  			.rx_buf = &ad7949_adc->buffer,
> -			.len = 4,
> +			.len = 2,
>  			.bits_per_word = bits_per_word,
>  		},
>  	};

Re: [PATCH 3/4] iio: ad7949: fix SPI xfer delays

[Ardelean, Alexandru]

On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:
> [External]
> 
> The driver calls udelay(2) after each SPI xfer. However, according to
> the specifications, the SPI timing should be as follows:
> 
> 1- The end of SPI xfer (CNV/CS rising edge) causes the device to initiate
>    the conversion phase, which takes up to 2.2uS.

Yes, but there does not seem to be a minimum time for conversion.
( 2.2 uS is the max value )

This can be confusing a bit (I know).
If you do see issues with 2 uS, we can probably try 3 uS (+1 uS which is roughly half the max value).
Though, we are already gaining min 200 nS from the fact that the acquisition time is 1.8 uS and the delay is 2 uS.

But if there aren't any visible issues, I would leave 2 uS.
Increasing this delay can annoy people that would like to have some speed when reading samples.
I know 1-2 uS isn't much of a performance killer, but if there aren't reasons to change it, I wouldn't.

> 
> 2- At the end of the conversion phase, the device starts the acquisition
>    phase for the next conversion automatically (regardless to the state of
>    CNV pin); the conversion phase should last at least 1.8 uS
> 
> The whole cycle timing is thus 4uS long. The SPI data is read during the
> acquisition phase (RAC mode, no need to worry about "Tdata").
> 
> In order to be compliant wrt these timing specifications we should wait
> 4uS after each SPI xfer (that is conservative, because there is also the
> SPI xfer duration itself - which at the maximum supported clock should be
> about 320nS).
> 
> This patch enlarges the delay up to 4uS and it also removes the explicit
> calls to udelay(), relying on spi_transfer->delay_usecs.
> 

I like the switch from explicit udelay() to spi_transfer->delay_usecs.
The code looks cleaner.

> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> ---
>  drivers/iio/adc/ad7949.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index 5c2b3446fa4a..25d1e1b24257 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -69,6 +69,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
>  			.tx_buf = &ad7949_adc->buffer,
>  			.len = 2,
>  			.bits_per_word = bits_per_word,
> +			.delay_usecs = 4,
>  		},
>  	};
>  
> @@ -77,11 +78,6 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
>  	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
> -	 * send a new command to the device
> -	 */
> -	udelay(2);
>  	return ret;
>  }
>  
> @@ -97,6 +93,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>  			.rx_buf = &ad7949_adc->buffer,
>  			.len = 2,
>  			.bits_per_word = bits_per_word,
> +			.delay_usecs = 4,
>  		},
>  	};
>  
> @@ -112,12 +109,6 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>  	if (ret)
>  		return ret;
>  
> -	/*
> -	 * This delay is to avoid a new request before the required time to
> -	 * send a new command to the device
> -	 */
> -	udelay(2);
> -
>  	ad7949_adc->current_channel = channel;
>  
>  	*val = ad7949_adc->buffer & mask;

Re: [PATCH 4/4] iio: ad7949: fix channels mixups

[Ardelean, Alexandru]

On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:
> [External]
> 
> Each time we need to read a sample the driver writes the CFG register
> (setting the channel to be read in such register) and then it performs
> another xfer to read the resulting value.
> 
> This does not work correctly because while writing the CFG register the
> acquisition phase is ongoing using the _previous_ CFG settings. Then the
> device performs the conversion during xfer delay on the voltage stored
> duting the said acquisitaion phase. Finally the driver performs the read
> (during the next acquisition phase, which is the one done with the right
> settings) and it gets the last converted value, that is the wrong data.
> 
> In case the configuration is not actually changed, then we still get
> correct data, but in case the configuration changes (and this happens e.g.
> switching the MUX on another channel), we get wrong data (data from the
> previously selected channel).
> 
> This patch fixes this by performing one more "dummy" transfer in order to
> ending up in reading the data when it's really ready.

So, at power-up this chip seems to need 2 dummy reads to discard data.
Which seems to happen in ad7949_spi_init()

One thing that maybe could be optimized (for the driver), is in `ad7949_spi_read_channel()` to use the current-channel &
not do a SPI write to change the channel if it doesn't change.

Datasheets (in general) are not always obvious about describing chip behavior for SW (or for writing a driver), but I
would suspect that if you are reading garbage data, it could be that the channel has changed.
This is true for some other ADCs.
And requires testing for this one.

Added Charles-Antoine, since he wrote the driver.
Shoud have added him on the other patches as well, but I just remembered.

Thanks
Alex

> 
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> ---
>  drivers/iio/adc/ad7949.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index 25d1e1b24257..b1dbe2075ca9 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -85,6 +85,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>  				   unsigned int channel)
>  {
>  	int ret;
> +	int i;
>  	int bits_per_word = ad7949_adc->resolution;
>  	int mask = GENMASK(ad7949_adc->resolution, 0);
>  	struct spi_message msg;
> @@ -97,12 +98,19 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>  		},
>  	};
>  
> -	ret = ad7949_spi_write_cfg(ad7949_adc,
> -				   channel << AD7949_OFFSET_CHANNEL_SEL,
> -				   AD7949_MASK_CHANNEL_SEL);
> -	if (ret)
> -		return ret;
> +	/*
> +	 * 1: write CFG for sample 'n' and read garbage (sample n-2)
> +	 * 2: write something and read garbage (sample n-1)
> +	 */
> +	for (i = 0; i < 2; i++) {
> +		ret = ad7949_spi_write_cfg(ad7949_adc,
> +					   channel << AD7949_OFFSET_CHANNEL_SEL,
> +					   AD7949_MASK_CHANNEL_SEL);
> +		if (ret)
> +			return ret;
> +	}
>  
> +	/* 3: write something and read data for sample 'n' */
>  	ad7949_adc->buffer = 0;
>  	spi_message_init_with_transfers(&msg, tx, 1);
>  	ret = spi_sync(ad7949_adc->spi, &msg);

Re: [PATCH 0/4] Fixes for ad7949

[Ardelean, Alexandru]

On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:
> [External]
> 
> This patch series fixes ad7949 driver incorrectly read data, simplify the
> code, and enforces device timing constraints.
> 
> This has been tested on a UltraZed SOM + a custom carrier equipped with
> several AD7689 A/Ds. Patches have been developed on a Xilinx upstream
> kernel and then rebased on linux-next kernel.
> 

Hey,

Thanks for the patches.
Added Charles-Antoine to also take a look.
Apologies for not thinking of adding him sooner.

I typically try to review changes for ADI parts, but he wrote it, so he may have more input than I do.
Jonathan will likely also take a look.

If it's agreed, I would say to at least take the first patch ("iio: ad7949: kill pointless "readback"-handling code")
now and see about the rest.
The rest are a bit more open to discussion, so a v2 may happen.

Thanks
Alex 

> Andrea Merello (4):
>   iio: ad7949: kill pointless "readback"-handling code
>   iio: ad7949: fix incorrect SPI xfer len
>   iio: ad7949: fix SPI xfer delays
>   iio: ad7949: fix channels mixups
> 
>  drivers/iio/adc/ad7949.c | 64 +++++++++++++---------------------------
>  1 file changed, 21 insertions(+), 43 deletions(-)
> 
> --
> 2.17.1

Re: [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len

[Andrea Merello]

Il giorno ven 13 set 2019 alle ore 08:46 Ardelean, Alexandru
<alexandru.Ardelean@analog.com> ha scritto:
>
> On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:
> > [External]
> >
> > This driver supports 14-bits and 16-bits devices. All of them have a 14-bit
> > configuration registers. All SPI trasfers, for reading AD conversion
> > results and for writing the configuration register, fit in two bytes.
> >
> > The driver always uses 4-bytes xfers which seems at least pointless (maybe
> > even harmful). This patch trims the SPI xfer len and the buffer size to
> > two bytes.
> >
>
> The length reduction proposal is fine.
>
> But, this patch raises a question about endianess.
> I'm actually wondering here if we need to see about maybe using a __be16 vs u16.
>
> I'm not that kernel-savy yet about some of these low-level things to be completely sure here.
> So, I'd let someone else maybe handle it.

Good point.. It seems that indeed not much care has been taken about
endianess here.. Probably we need also some le16_to_cpu() and
firends..

Maybe another separate patch can be made to take care about endianess later on?

BTW Also, the  ____cacheline_aligned is a bit scaring :) I don't know
what is that for...

>
> Thanks
> Alex
>
> > Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> > ---
> >  drivers/iio/adc/ad7949.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > index 518044c31a73..5c2b3446fa4a 100644
> > --- a/drivers/iio/adc/ad7949.c
> > +++ b/drivers/iio/adc/ad7949.c
> > @@ -54,7 +54,7 @@ struct ad7949_adc_chip {
> >       u8 resolution;
> >       u16 cfg;
> >       unsigned int current_channel;
> > -     u32 buffer ____cacheline_aligned;
> > +     u16 buffer ____cacheline_aligned;
> >  };
> >
> >  static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> > @@ -67,7 +67,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> >       struct spi_transfer tx[] = {
> >               {
> >                       .tx_buf = &ad7949_adc->buffer,
> > -                     .len = 4,
> > +                     .len = 2,
> >                       .bits_per_word = bits_per_word,
> >               },
> >       };
> > @@ -95,7 +95,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> >       struct spi_transfer tx[] = {
> >               {
> >                       .rx_buf = &ad7949_adc->buffer,
> > -                     .len = 4,
> > +                     .len = 2,
> >                       .bits_per_word = bits_per_word,
> >               },
> >       };

Re: [PATCH 3/4] iio: ad7949: fix SPI xfer delays

[Andrea Merello]

Il giorno ven 13 set 2019 alle ore 09:00 Ardelean, Alexandru
<alexandru.Ardelean@analog.com> ha scritto:
>
> On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:
> > [External]
> >
> > The driver calls udelay(2) after each SPI xfer. However, according to
> > the specifications, the SPI timing should be as follows:
> >
> > 1- The end of SPI xfer (CNV/CS rising edge) causes the device to initiate
> >    the conversion phase, which takes up to 2.2uS.
>
> Yes, but there does not seem to be a minimum time for conversion.
> ( 2.2 uS is the max value )
>
> This can be confusing a bit (I know).
> If you do see issues with 2 uS, we can probably try 3 uS (+1 uS which is roughly half the max value).
> Though, we are already gaining min 200 nS from the fact that the acquisition time is 1.8 uS and the delay is 2 uS.
>
> But if there aren't any visible issues, I would leave 2 uS.
> Increasing this delay can annoy people that would like to have some speed when reading samples.

I admit that I got some hard time trying to fully understand the
timing specifications; so it's perfectly possible that I've got
something wrong here.. My interpretation was that the HW takes up to
2.2uS (thus it's a max value, as you said) and, since we are not using
the busy indication to check when the HW really finished, I stayed on
the safe side.

I've done this change while I was searching for the cause of some
reading issues that turned out to be actually fixed with the last
patch of the series, so I have no real evidence of  issues caused by
the 2uS delay. However, if I understood correctly the datasheet, the
effect of performing an early SPI xfer during the conversion phase -
specifically if it happens after tDATA and before EndOfConversion -
might be mild, so not it might be not obvious to notice (maybe affects
just LSBs) it with my basic testing. Quoting the datasheet: "The time
between tDATA and tCONV is a safe time when digital activity should
not occur, or sensitive bit decisions may be corrupted. "..

> I know 1-2 uS isn't much of a performance killer, but if there aren't reasons to change it, I wouldn't.

I guess you know the HW by far better than me, so if you say that 2uS
is OK then I will not insist anymore here :)

>
> >
> > 2- At the end of the conversion phase, the device starts the acquisition
> >    phase for the next conversion automatically (regardless to the state of
> >    CNV pin); the conversion phase should last at least 1.8 uS
> >
> > The whole cycle timing is thus 4uS long. The SPI data is read during the
> > acquisition phase (RAC mode, no need to worry about "Tdata").
> >
> > In order to be compliant wrt these timing specifications we should wait
> > 4uS after each SPI xfer (that is conservative, because there is also the
> > SPI xfer duration itself - which at the maximum supported clock should be
> > about 320nS).
> >
> > This patch enlarges the delay up to 4uS and it also removes the explicit
> > calls to udelay(), relying on spi_transfer->delay_usecs.
> >
>
> I like the switch from explicit udelay() to spi_transfer->delay_usecs.
> The code looks cleaner.
>
> > Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> > ---
> >  drivers/iio/adc/ad7949.c | 13 ++-----------
> >  1 file changed, 2 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > index 5c2b3446fa4a..25d1e1b24257 100644
> > --- a/drivers/iio/adc/ad7949.c
> > +++ b/drivers/iio/adc/ad7949.c
> > @@ -69,6 +69,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> >                       .tx_buf = &ad7949_adc->buffer,
> >                       .len = 2,
> >                       .bits_per_word = bits_per_word,
> > +                     .delay_usecs = 4,
> >               },
> >       };
> >
> > @@ -77,11 +78,6 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> >       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
> > -      * send a new command to the device
> > -      */
> > -     udelay(2);
> >       return ret;
> >  }
> >
> > @@ -97,6 +93,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> >                       .rx_buf = &ad7949_adc->buffer,
> >                       .len = 2,
> >                       .bits_per_word = bits_per_word,
> > +                     .delay_usecs = 4,
> >               },
> >       };
> >
> > @@ -112,12 +109,6 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> >       if (ret)
> >               return ret;
> >
> > -     /*
> > -      * This delay is to avoid a new request before the required time to
> > -      * send a new command to the device
> > -      */
> > -     udelay(2);
> > -
> >       ad7949_adc->current_channel = channel;
> >
> >       *val = ad7949_adc->buffer & mask;

Re: [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len

[Ardelean, Alexandru]

On Fri, 2019-09-13 at 09:56 +0200, Andrea Merello wrote:
> Il giorno ven 13 set 2019 alle ore 08:46 Ardelean, Alexandru
> <alexandru.Ardelean@analog.com> ha scritto:
> > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:
> > > [External]
> > > 
> > > This driver supports 14-bits and 16-bits devices. All of them have a 14-bit
> > > configuration registers. All SPI trasfers, for reading AD conversion
> > > results and for writing the configuration register, fit in two bytes.
> > > 
> > > The driver always uses 4-bytes xfers which seems at least pointless (maybe
> > > even harmful). This patch trims the SPI xfer len and the buffer size to
> > > two bytes.
> > > 
> > 
> > The length reduction proposal is fine.
> > 
> > But, this patch raises a question about endianess.
> > I'm actually wondering here if we need to see about maybe using a __be16 vs u16.
> > 
> > I'm not that kernel-savy yet about some of these low-level things to be completely sure here.
> > So, I'd let someone else maybe handle it.
> 
> Good point.. It seems that indeed not much care has been taken about
> endianess here.. Probably we need also some le16_to_cpu() and
> firends..
> 
> Maybe another separate patch can be made to take care about endianess later on?
> 

Sure. Another patch makes sense.
But, I am bit vague here about what to do.
I would copy examples from other drivers about how things were done.
There seem to be others that did this already somehow.


> BTW Also, the  ____cacheline_aligned is a bit scaring :) I don't know
> what is that for...

I'm still not completely clear on ___cacheline_aligned.
Mostly because I did not find any issues that needed it.
I just used it based on how others used it and all was fine.
From older notes, it looks helpful with DMA and some memory allocation stuff.
But this also depends on arch ; for some of them it's empty.

> 
> > Thanks
> > Alex
> > 
> > > Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> > > ---
> > >  drivers/iio/adc/ad7949.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > > index 518044c31a73..5c2b3446fa4a 100644
> > > --- a/drivers/iio/adc/ad7949.c
> > > +++ b/drivers/iio/adc/ad7949.c
> > > @@ -54,7 +54,7 @@ struct ad7949_adc_chip {
> > >       u8 resolution;
> > >       u16 cfg;
> > >       unsigned int current_channel;
> > > -     u32 buffer ____cacheline_aligned;
> > > +     u16 buffer ____cacheline_aligned;
> > >  };
> > > 
> > >  static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> > > @@ -67,7 +67,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> > >       struct spi_transfer tx[] = {
> > >               {
> > >                       .tx_buf = &ad7949_adc->buffer,
> > > -                     .len = 4,
> > > +                     .len = 2,
> > >                       .bits_per_word = bits_per_word,
> > >               },
> > >       };
> > > @@ -95,7 +95,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> > >       struct spi_transfer tx[] = {
> > >               {
> > >                       .rx_buf = &ad7949_adc->buffer,
> > > -                     .len = 4,
> > > +                     .len = 2,
> > >                       .bits_per_word = bits_per_word,
> > >               },
> > >       };

Re: [PATCH 4/4] iio: ad7949: fix channels mixups

[Andrea Merello]

Il giorno ven 13 set 2019 alle ore 09:19 Ardelean, Alexandru
<alexandru.Ardelean@analog.com> ha scritto:
>
> On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:
> > [External]
> >
> > Each time we need to read a sample the driver writes the CFG register
> > (setting the channel to be read in such register) and then it performs
> > another xfer to read the resulting value.
> >
> > This does not work correctly because while writing the CFG register the
> > acquisition phase is ongoing using the _previous_ CFG settings. Then the
> > device performs the conversion during xfer delay on the voltage stored
> > duting the said acquisitaion phase. Finally the driver performs the read
> > (during the next acquisition phase, which is the one done with the right
> > settings) and it gets the last converted value, that is the wrong data.
> >
> > In case the configuration is not actually changed, then we still get
> > correct data, but in case the configuration changes (and this happens e.g.
> > switching the MUX on another channel), we get wrong data (data from the
> > previously selected channel).
> >
> > This patch fixes this by performing one more "dummy" transfer in order to
> > ending up in reading the data when it's really ready.
>
> So, at power-up this chip seems to need 2 dummy reads to discard data.
> Which seems to happen in ad7949_spi_init()
>
> One thing that maybe could be optimized (for the driver), is in `ad7949_spi_read_channel()` to use the current-channel &
> not do a SPI write to change the channel if it doesn't change.
>
> Datasheets (in general) are not always obvious about describing chip behavior for SW (or for writing a driver), but I
> would suspect that if you are reading garbage data, it could be that the channel has changed.
> This is true for some other ADCs.
> And requires testing for this one.

Yes, it's exactly what I've seen here. If the channel does not change
then the AD is already in acquisition phase on the right channel (I
assume it's OK to keep it in such phase indefinitely), then we can
just trigger a new conversion (CNV low->high, that is a dummy xfer)
and then read the result in following xfer, as the driver already did.

I craft a V2 that performs the extra (3rd) spi xfer only if the
channel has to change.

> Added Charles-Antoine, since he wrote the driver.
> Shoud have added him on the other patches as well, but I just remembered.

I tried on my first answer, but apparently mails to his address bounce
back with a failure response..

> Thanks
> Alex
>
> >
> > Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> > ---
> >  drivers/iio/adc/ad7949.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > index 25d1e1b24257..b1dbe2075ca9 100644
> > --- a/drivers/iio/adc/ad7949.c
> > +++ b/drivers/iio/adc/ad7949.c
> > @@ -85,6 +85,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> >                                  unsigned int channel)
> >  {
> >       int ret;
> > +     int i;
> >       int bits_per_word = ad7949_adc->resolution;
> >       int mask = GENMASK(ad7949_adc->resolution, 0);
> >       struct spi_message msg;
> > @@ -97,12 +98,19 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> >               },
> >       };
> >
> > -     ret = ad7949_spi_write_cfg(ad7949_adc,
> > -                                channel << AD7949_OFFSET_CHANNEL_SEL,
> > -                                AD7949_MASK_CHANNEL_SEL);
> > -     if (ret)
> > -             return ret;
> > +     /*
> > +      * 1: write CFG for sample 'n' and read garbage (sample n-2)
> > +      * 2: write something and read garbage (sample n-1)
> > +      */
> > +     for (i = 0; i < 2; i++) {
> > +             ret = ad7949_spi_write_cfg(ad7949_adc,
> > +                                        channel << AD7949_OFFSET_CHANNEL_SEL,
> > +                                        AD7949_MASK_CHANNEL_SEL);
> > +             if (ret)
> > +                     return ret;
> > +     }
> >
> > +     /* 3: write something and read data for sample 'n' */
> >       ad7949_adc->buffer = 0;
> >       spi_message_init_with_transfers(&msg, tx, 1);
> >       ret = spi_sync(ad7949_adc->spi, &msg);

Re: [PATCH 3/4] iio: ad7949: fix SPI xfer delays

[Ardelean, Alexandru]

On Fri, 2019-09-13 at 10:23 +0200, Andrea Merello wrote:
> Il giorno ven 13 set 2019 alle ore 09:00 Ardelean, Alexandru
> <alexandru.Ardelean@analog.com> ha scritto:
> > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:
> > > [External]
> > > 
> > > The driver calls udelay(2) after each SPI xfer. However, according to
> > > the specifications, the SPI timing should be as follows:
> > > 
> > > 1- The end of SPI xfer (CNV/CS rising edge) causes the device to initiate
> > >    the conversion phase, which takes up to 2.2uS.
> > 
> > Yes, but there does not seem to be a minimum time for conversion.
> > ( 2.2 uS is the max value )
> > 
> > This can be confusing a bit (I know).
> > If you do see issues with 2 uS, we can probably try 3 uS (+1 uS which is roughly half the max value).
> > Though, we are already gaining min 200 nS from the fact that the acquisition time is 1.8 uS and the delay is 2 uS.
> > 
> > But if there aren't any visible issues, I would leave 2 uS.
> > Increasing this delay can annoy people that would like to have some speed when reading samples.
> 
> I admit that I got some hard time trying to fully understand the
> timing specifications; so it's perfectly possible that I've got
> something wrong here.. My interpretation was that the HW takes up to
> 2.2uS (thus it's a max value, as you said) and, since we are not using
> the busy indication to check when the HW really finished, I stayed on
> the safe side.
> 
> I've done this change while I was searching for the cause of some
> reading issues that turned out to be actually fixed with the last
> patch of the series, so I have no real evidence of  issues caused by
> the 2uS delay. However, if I understood correctly the datasheet, the
> effect of performing an early SPI xfer during the conversion phase -
> specifically if it happens after tDATA and before EndOfConversion -
> might be mild, so not it might be not obvious to notice (maybe affects
> just LSBs) it with my basic testing. Quoting the datasheet: "The time
> between tDATA and tCONV is a safe time when digital activity should
> not occur, or sensitive bit decisions may be corrupted. "..
> 
> > I know 1-2 uS isn't much of a performance killer, but if there aren't reasons to change it, I wouldn't.
> 
> I guess you know the HW by far better than me, so if you say that 2uS
> is OK then I will not insist anymore here :)

Well, yes & no.
To get the best possible answer, we would need to ask more directly to the business-unit that has released the part,
which is a bit disconnected from the group that writes & upstreams Linux drivers.
Depending on the effort required to go through this, or depending how important this item is, we can do that.

But, if testing suggests that 2 uS works, then it's also fine to use it.

> 
> > > 2- At the end of the conversion phase, the device starts the acquisition
> > >    phase for the next conversion automatically (regardless to the state of
> > >    CNV pin); the conversion phase should last at least 1.8 uS
> > > 
> > > The whole cycle timing is thus 4uS long. The SPI data is read during the
> > > acquisition phase (RAC mode, no need to worry about "Tdata").
> > > 
> > > In order to be compliant wrt these timing specifications we should wait
> > > 4uS after each SPI xfer (that is conservative, because there is also the
> > > SPI xfer duration itself - which at the maximum supported clock should be
> > > about 320nS).
> > > 
> > > This patch enlarges the delay up to 4uS and it also removes the explicit
> > > calls to udelay(), relying on spi_transfer->delay_usecs.
> > > 
> > 
> > I like the switch from explicit udelay() to spi_transfer->delay_usecs.
> > The code looks cleaner.
> > 
> > > Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> > > ---
> > >  drivers/iio/adc/ad7949.c | 13 ++-----------
> > >  1 file changed, 2 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > > index 5c2b3446fa4a..25d1e1b24257 100644
> > > --- a/drivers/iio/adc/ad7949.c
> > > +++ b/drivers/iio/adc/ad7949.c
> > > @@ -69,6 +69,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> > >                       .tx_buf = &ad7949_adc->buffer,
> > >                       .len = 2,
> > >                       .bits_per_word = bits_per_word,
> > > +                     .delay_usecs = 4,
> > >               },
> > >       };
> > > 
> > > @@ -77,11 +78,6 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> > >       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
> > > -      * send a new command to the device
> > > -      */
> > > -     udelay(2);
> > >       return ret;
> > >  }
> > > 
> > > @@ -97,6 +93,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> > >                       .rx_buf = &ad7949_adc->buffer,
> > >                       .len = 2,
> > >                       .bits_per_word = bits_per_word,
> > > +                     .delay_usecs = 4,
> > >               },
> > >       };
> > > 
> > > @@ -112,12 +109,6 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> > >       if (ret)
> > >               return ret;
> > > 
> > > -     /*
> > > -      * This delay is to avoid a new request before the required time to
> > > -      * send a new command to the device
> > > -      */
> > > -     udelay(2);
> > > -
> > >       ad7949_adc->current_channel = channel;
> > > 
> > >       *val = ad7949_adc->buffer & mask;

Re: [PATCH 4/4] iio: ad7949: fix channels mixups

[Couret Charles-Antoine]

Hi,

Le 13/09/2019 à 10:30, Andrea Merello a écrit :
> Il giorno ven 13 set 2019 alle ore 09:19 Ardelean, Alexandru
> <alexandru.Ardelean@analog.com> ha scritto:
>
>> So, at power-up this chip seems to need 2 dummy reads to discard data.
>> Which seems to happen in ad7949_spi_init()
>>
>> One thing that maybe could be optimized (for the driver), is in `ad7949_spi_read_channel()` to use the current-channel &
>> not do a SPI write to change the channel if it doesn't change.
>>
>> Datasheets (in general) are not always obvious about describing chip behavior for SW (or for writing a driver), but I
>> would suspect that if you are reading garbage data, it could be that the channel has changed.
>> This is true for some other ADCs.
>> And requires testing for this one.
> Yes, it's exactly what I've seen here. If the channel does not change
> then the AD is already in acquisition phase on the right channel (I
> assume it's OK to keep it in such phase indefinitely), then we can
> just trigger a new conversion (CNV low->high, that is a dummy xfer)
> and then read the result in following xfer, as the driver already did.
>
> I craft a V2 that performs the extra (3rd) spi xfer only if the
> channel has to change.

This design should be ok. I didn't implement in that way because not 
enough time to optimize the driver before release (I don't have access 
to the chip anymore) and for our workflow it was not relevant (we 
scanned all channels).


About your fix to read / write several times before reading the value 
after channel change seems not relevant. Did you try with the current 
implementation? Because when I developed the driver, I have always got 
the expected value for each channel with this design.


Just to be sure we are not adding useless steps.

>> Added Charles-Antoine, since he wrote the driver.
>> Shoud have added him on the other patches as well, but I just remembered.
> I tried on my first answer, but apparently mails to his address bounce
> back with a failure response..

But now it seems ok. Did you put the right email address?


Thank you for the copy.

Regards,

Charles-Antoine Couret

Re: [PATCH 4/4] iio: ad7949: fix channels mixups

[Andrea Merello]

Il giorno ven 13 set 2019 alle ore 13:30 Couret Charles-Antoine
<charles-antoine.couret@essensium.com> ha scritto:
>
> Hi,
>
> Le 13/09/2019 à 10:30, Andrea Merello a écrit :
> > Il giorno ven 13 set 2019 alle ore 09:19 Ardelean, Alexandru
> > <alexandru.Ardelean@analog.com> ha scritto:
> >
> >> So, at power-up this chip seems to need 2 dummy reads to discard data.
> >> Which seems to happen in ad7949_spi_init()
> >>
> >> One thing that maybe could be optimized (for the driver), is in `ad7949_spi_read_channel()` to use the current-channel &
> >> not do a SPI write to change the channel if it doesn't change.
> >>
> >> Datasheets (in general) are not always obvious about describing chip behavior for SW (or for writing a driver), but I
> >> would suspect that if you are reading garbage data, it could be that the channel has changed.
> >> This is true for some other ADCs.
> >> And requires testing for this one.
> > Yes, it's exactly what I've seen here. If the channel does not change
> > then the AD is already in acquisition phase on the right channel (I
> > assume it's OK to keep it in such phase indefinitely), then we can
> > just trigger a new conversion (CNV low->high, that is a dummy xfer)
> > and then read the result in following xfer, as the driver already did.
> >
> > I craft a V2 that performs the extra (3rd) spi xfer only if the
> > channel has to change.
>
> This design should be ok. I didn't implement in that way because not
> enough time to optimize the driver before release (I don't have access
> to the chip anymore) and for our workflow it was not relevant (we
> scanned all channels).
>
>
> About your fix to read / write several times before reading the value
> after channel change seems not relevant. Did you try with the current
> implementation? Because when I developed the driver, I have always got
> the expected value for each channel with this design.

Yes, I did. But I experienced the said problems. I don't have idea
about why it worked for you and it didn't for me..

By scanning the channels in circular fashion, with the current
implementation, I would expect to get values off-by-one (i.e. you read
ch ...,0,1,2,3,4,0,1,2,3,4,... and you get value for ch
...,4,0,1,2,3,4,0,1,2,3,...).

>
> Just to be sure we are not adding useless steps.
>
> >> Added Charles-Antoine, since he wrote the driver.
> >> Shoud have added him on the other patches as well, but I just remembered.
> > I tried on my first answer, but apparently mails to his address bounce
> > back with a failure response..
>
> But now it seems ok. Did you put the right email address?

Sorry, my fault. I've done a mistake in copy-and-paste.

>
> Thank you for the copy.
>
> Regards,
>
> Charles-Antoine Couret
>

Re: [PATCH 0/4] Fixes for ad7949

[Couret Charles-Antoine]

Le 13/09/2019 à 09:24, Ardelean, Alexandru a écrit :
> On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:
>> [External]
>>
>> This patch series fixes ad7949 driver incorrectly read data, simplify the
>> code, and enforces device timing constraints.
>>
>> This has been tested on a UltraZed SOM + a custom carrier equipped with
>> several AD7689 A/Ds. Patches have been developed on a Xilinx upstream
>> kernel and then rebased on linux-next kernel.
>>
> Thanks for the patches.
> Added Charles-Antoine to also take a look.
> Apologies for not thinking of adding him sooner.
>
> I typically try to review changes for ADI parts, but he wrote it, so he may have more input than I do.
> Jonathan will likely also take a look.
>
> If it's agreed, I would say to at least take the first patch ("iio: ad7949: kill pointless "readback"-handling code")
> now and see about the rest.
> The rest are a bit more open to discussion, so a v2 may happen.

Hi,

Don't worry. Due to the fact I don't have on my mail client access to 
the whole discussions, I'm making a complete answer there based on the 
archive of the mailing list. Sorry for that.


For the patch 1, I approve it too. This part of code is useless because 
the feature was removed. RIP my code. :D

For the patch 2, the cache information was added due to comment from 
Jonathan Cameron when I developed the driver. The comment was:

> Look very carefully at the requirements for a buffer being passed
> to spi_sync.  It needs to be DMA safe.  This one is not.  The usual
> way to do that easily is to put a cacheline aligned buffer in your
> ad7949_adc_chip structure.
>
> Lots of examples to copy, but it's also worth making sure you understand
> why this is necessary.

For the endianess thing, it shouldn't be required to make an explicit 
conversion into the driver. According to the spi.h documentation:

> * In-memory data values are always in native CPU byte order, translated
> * from the wire byte order (big-endian except with SPI_LSB_FIRST). So
> * for example when bits_per_word is sixteen, buffers are 2N bytes long
> * (@len = 2N) and hold N sixteen bit words in CPU byte order.
So from my point of view the SPI subsystem always converts to the right 
endianess. We don't have to take care about it.


For patch 3, I didn't use delay_usecs fiels due to the timings 
definition in the datasheet in "READ/WRITE SPANNING CONVERSION WITHOUT A 
BUSY INDICATOR" mode. During the delay, the chip select line must be 
released which is not the case when we use delay_usecs field. So I add 
the delay instruction after the write step to be compliant with these 
timings.


For patch 4, I explained a bit in the other thread. Maybe we have a 
difference of behaviour due to the choice of the timings "modes"?


BTW, from my point of view the datasheet is not totally clear about the 
timings and what is mandatory or not in the expected behaviour.

Regards,

Charles-Antoine Couret

Re: [PATCH 1/4] iio: ad7949: kill pointless "readback"-handling code

[Jonathan Cameron]

On Fri, 13 Sep 2019 06:37:17 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:
> > [External]
> > 
> > The device could be configured to spit out also the configuration word
> > while reading the AD result value (in the same SPI xfer) - this is called
> > "readback" in the device datasheet.
> > 
> > The driver checks if readback is enabled and it eventually adjusts the SPI
> > xfer length and it applies proper shifts to still get the data, discarding
> > the configuration word.
> > 
> > The readback option is actually never enabled (the driver disables it), so
> > the said checks do not serve for any purpose.
> > 
> > Since enabling the readback option seems not to provide any advantage (the
> > driver entirely sets the configuration word without relying on any default
> > value), just kill the said, unused, code.  
> 
> Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to poke at it.

Thanks,

Jonathan

> 
> > 
> > Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> > ---
> >  drivers/iio/adc/ad7949.c | 27 +++------------------------
> >  1 file changed, 3 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > index ac0ffff6c5ae..518044c31a73 100644
> > --- a/drivers/iio/adc/ad7949.c
> > +++ b/drivers/iio/adc/ad7949.c
> > @@ -57,29 +57,11 @@ struct ad7949_adc_chip {
> >  	u32 buffer ____cacheline_aligned;
> >  };
> >  
> > -static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
> > -{
> > -	if (!(ad7949_adc->cfg & AD7949_CFG_READ_BACK))
> > -		return true;
> > -
> > -	return false;
> > -}
> > -
> > -static int ad7949_spi_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
> > -{
> > -	int ret = ad7949_adc->resolution;
> > -
> > -	if (ad7949_spi_cfg_is_read_back(ad7949_adc))
> > -		ret += AD7949_CFG_REG_SIZE_BITS;
> > -
> > -	return ret;
> > -}
> > -
> >  static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> >  				u16 mask)
> >  {
> >  	int ret;
> > -	int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
> > +	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[] = {
> > @@ -107,7 +89,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> >  				   unsigned int channel)
> >  {
> >  	int ret;
> > -	int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
> > +	int bits_per_word = ad7949_adc->resolution;
> >  	int mask = GENMASK(ad7949_adc->resolution, 0);
> >  	struct spi_message msg;
> >  	struct spi_transfer tx[] = {
> > @@ -138,10 +120,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> >  
> >  	ad7949_adc->current_channel = channel;
> >  
> > -	if (ad7949_spi_cfg_is_read_back(ad7949_adc))
> > -		*val = (ad7949_adc->buffer >> AD7949_CFG_REG_SIZE_BITS) & mask;
> > -	else
> > -		*val = ad7949_adc->buffer & mask;
> > +	*val = ad7949_adc->buffer & mask;
> >  
> >  	return 0;
> >  }  

Re: [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len

[Jonathan Cameron]

On Fri, 13 Sep 2019 09:56:56 +0200
Andrea Merello <andrea.merello@gmail.com> wrote:

> Il giorno ven 13 set 2019 alle ore 08:46 Ardelean, Alexandru
> <alexandru.Ardelean@analog.com> ha scritto:
> >
> > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:  
> > > [External]
> > >
> > > This driver supports 14-bits and 16-bits devices. All of them have a 14-bit
> > > configuration registers. All SPI trasfers, for reading AD conversion
> > > results and for writing the configuration register, fit in two bytes.
> > >
> > > The driver always uses 4-bytes xfers which seems at least pointless (maybe
> > > even harmful). This patch trims the SPI xfer len and the buffer size to
> > > two bytes.
> > >  
> >
> > The length reduction proposal is fine.
> >
> > But, this patch raises a question about endianess.
> > I'm actually wondering here if we need to see about maybe using a __be16 vs u16.
> >
> > I'm not that kernel-savy yet about some of these low-level things to be completely sure here.
> > So, I'd let someone else maybe handle it.  
> 
> Good point.. It seems that indeed not much care has been taken about
> endianess here.. Probably we need also some le16_to_cpu() and
> firends..

More complexity here :)  So a lot of earlier SPI drivers didn't set bits_per_word,
the result of this is that a read had no way to know how to unwind the endian
nature of the data.  If you do a 4 byte read, is that 4x 1 byte, 2x 2 bytes or
1x 4 bytes.  Thus the SPI subsystem had no way of knowing how to convert from
wire order of big endian to cpu endianness.  This is particularly fun as it
is common to have variable length registers on SPI devices (be it described
on the datasheet as some registers have high and low byte addresses).

In drivers where this can be set to one consistent value, then the SPI subsystem
should do the work for us. Hence this one should be fine. ( I think :)

> 
> Maybe another separate patch can be made to take care about endianess later on?
> 
> BTW Also, the  ____cacheline_aligned is a bit scaring :) I don't know
> what is that for...
> 
> >
> > Thanks
> > Alex
> >  
> > > Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> > > ---
> > >  drivers/iio/adc/ad7949.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > > index 518044c31a73..5c2b3446fa4a 100644
> > > --- a/drivers/iio/adc/ad7949.c
> > > +++ b/drivers/iio/adc/ad7949.c
> > > @@ -54,7 +54,7 @@ struct ad7949_adc_chip {
> > >       u8 resolution;
> > >       u16 cfg;
> > >       unsigned int current_channel;
> > > -     u32 buffer ____cacheline_aligned;
> > > +     u16 buffer ____cacheline_aligned;
> > >  };
> > >
> > >  static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> > > @@ -67,7 +67,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> > >       struct spi_transfer tx[] = {
> > >               {
> > >                       .tx_buf = &ad7949_adc->buffer,
> > > -                     .len = 4,
> > > +                     .len = 2,
> > >                       .bits_per_word = bits_per_word,
> > >               },
> > >       };
> > > @@ -95,7 +95,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> > >       struct spi_transfer tx[] = {
> > >               {
> > >                       .rx_buf = &ad7949_adc->buffer,
> > > -                     .len = 4,
> > > +                     .len = 2,
> > >                       .bits_per_word = bits_per_word,
> > >               },
> > >       };  

Re: [PATCH 0/4] Fixes for ad7949

[Jonathan Cameron]

On Fri, 13 Sep 2019 16:00:29 +0200
Couret Charles-Antoine <charles-antoine.couret@essensium.com> wrote:

> Le 13/09/2019 à 09:24, Ardelean, Alexandru a écrit :
> > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:  
> >> [External]
> >>
> >> This patch series fixes ad7949 driver incorrectly read data, simplify the
> >> code, and enforces device timing constraints.
> >>
> >> This has been tested on a UltraZed SOM + a custom carrier equipped with
> >> several AD7689 A/Ds. Patches have been developed on a Xilinx upstream
> >> kernel and then rebased on linux-next kernel.
> >>  
> > Thanks for the patches.
> > Added Charles-Antoine to also take a look.
> > Apologies for not thinking of adding him sooner.
> >
> > I typically try to review changes for ADI parts, but he wrote it, so he may have more input than I do.
> > Jonathan will likely also take a look.
> >
> > If it's agreed, I would say to at least take the first patch ("iio: ad7949: kill pointless "readback"-handling code")
> > now and see about the rest.
> > The rest are a bit more open to discussion, so a v2 may happen.  
> 
> Hi,
> 
> Don't worry. Due to the fact I don't have on my mail client access to 
> the whole discussions, I'm making a complete answer there based on the 
> archive of the mailing list. Sorry for that.
> 
> 
> For the patch 1, I approve it too. This part of code is useless because 
> the feature was removed. RIP my code. :D
> 
> For the patch 2, the cache information was added due to comment from 
> Jonathan Cameron when I developed the driver. The comment was:
> 
> > Look very carefully at the requirements for a buffer being passed
> > to spi_sync.  It needs to be DMA safe.  This one is not.  The usual
> > way to do that easily is to put a cacheline aligned buffer in your
> > ad7949_adc_chip structure.

The short version of this is best illustrated with an example.
This only applies systems where the DMA engines are not coherent
(i.e. a change made by a DMA engine is not automatically updated to
 all other places a copy is held in caches in the system, we have to
 do it by hand).

We have a structure like
struct bob {
	int initial_data;
	u8 buffer[8];
	int magic_flags
};

When a DMA transfer is setup involving 'buffer', the DMA engine may take
up to a cacheline (typically 64 bytes) including buffer, make a copy of it
and assume that the only bit of hardware working in this cacheline is itself.
(Linux is 'guaranteeing' this when it tells the DMA engine to use this buffer.'.
Whilst that DMA is going on, a CPU can write something in magic flags.
That something might be important but unrelated to the DMA transfer going
on.

The DMA finishes, having put new data in the buffer element of the copy
of the cacheline local to . It's guaranteed to not change it's copy of the
cacheline (in this case containing the whole of bob).   However, it's version
of magic_flags is out of date so when we flush the caches at the end of the
non coherent DMA transfer (to force the CPU to read it from main memory and
get the new values in buffer), the value of magic_flags can be reset to the
version the DMA engine has.

So, upshot is to avoid any potential of such problems, DMA buffers 'must'
always be in a cacheline containing nothing that might be changed by
other activities.  This can mean it is safe to put both TX and RX buffers
in the same cacheline as we won't touch either during an SPI transfer.

> >
> > Lots of examples to copy, but it's also worth making sure you understand
> > why this is necessary.  
> 
> For the endianess thing, it shouldn't be required to make an explicit 
> conversion into the driver. According to the spi.h documentation:
> 
> > * In-memory data values are always in native CPU byte order, translated
> > * from the wire byte order (big-endian except with SPI_LSB_FIRST). So
> > * for example when bits_per_word is sixteen, buffers are 2N bytes long
> > * (@len = 2N) and hold N sixteen bit words in CPU byte order.  
> So from my point of view the SPI subsystem always converts to the right 
> endianess. We don't have to take care about it.

Correct, though as I commented on that patch, that's not always 'possible'
and not all drivers set the word length 'correctly'.

Wolfram's presentation on trying to implement DMA safety in I2C at ELCE2018
also touches on a lot of this.

Thanks,

Jonathan

> 
> 
> For patch 3, I didn't use delay_usecs fiels due to the timings 
> definition in the datasheet in "READ/WRITE SPANNING CONVERSION WITHOUT A 
> BUSY INDICATOR" mode. During the delay, the chip select line must be 
> released which is not the case when we use delay_usecs field. So I add 
> the delay instruction after the write step to be compliant with these 
> timings.
> 
> 
> For patch 4, I explained a bit in the other thread. Maybe we have a 
> difference of behaviour due to the choice of the timings "modes"?
> 
> 
> BTW, from my point of view the datasheet is not totally clear about the 
> timings and what is mandatory or not in the expected behaviour.
> 
> Regards,
> 
> Charles-Antoine Couret
> 

Re: [PATCH 0/4] Fixes for ad7949

[Andrea Merello]

Il giorno ven 13 set 2019 alle ore 16:00 Couret Charles-Antoine
<charles-antoine.couret@essensium.com> ha scritto:

>
> For patch 3, I didn't use delay_usecs fiels due to the timings
> definition in the datasheet in "READ/WRITE SPANNING CONVERSION WITHOUT A
> BUSY INDICATOR" mode. During the delay, the chip select line must be
> released which is not the case when we use delay_usecs field. So I add
> the delay instruction after the write step to be compliant with these
> timings.

Ok, you are right, thank you for pointing this out..

It looks like that, for my original intent of being strict about
acquisition time and conversion time (as per my original
interpretation), both delays (before and after) CS change would be
needed.. But since Alexandru pointed out that the single 2uS delay, as
per current implementation, should be OK for the A/D chip, then I'd
drop patch 3/4.

> For patch 4, I explained a bit in the other thread. Maybe we have a
> difference of behaviour due to the choice of the timings "modes"?

I don't know.. is it "RAC" the intended working mode? If you want I
can take some snapshot of scope images and share them..

>
> BTW, from my point of view the datasheet is not totally clear about the
> timings and what is mandatory or not in the expected behaviour.

I agree with you that datasheet is not so clear about timings..
However I think it makes it pretty clear that we need the extra
transfer in order to make sure that the read data is correct (except
if the CFG doesn't change).

IMO that's could be evinced by both diagrams (all modes I think) and
by the following sentence in "READING/WRITING AFTER CONVERSION, ANY
SPEED HOSTS" paragraph: "When reading/writing after conversion, or
during acquisition(n), conversion results are for the previous (n − 1)
conversion, and writing is for the (n + 1) acquisition".

I interpret this as follows:
- You write a new CFG in cycle n, you get (and discard) data for cycle
n-1, and the new CFG will be used for next _acquisition_.
- Once you release the CS (you finished to write) the conversion for
the _currently_ acquired data (old cfg) starts. Once the A/D finishes
the conversion, the acquisition starts with the new CFG
- You read data, and the A/D returns results from the last
_conversion_ that has been done, which is still related to the old
configuration. Once you release the CS (finish to read) the conversion
for the currently acquired data - which is now the one with the new
CFG - is started.
- Finally you read the result of the above conversion

So IMO we really need three cycles to read from a random channel


> Regards,
>
> Charles-Antoine Couret
>

Re: [PATCH 0/4] Fixes for ad7949

[Andrea Merello]

Il giorno dom 15 set 2019 alle ore 12:49 Jonathan Cameron
<jic23@kernel.org> ha scritto:
>
> On Fri, 13 Sep 2019 16:00:29 +0200
> Couret Charles-Antoine <charles-antoine.couret@essensium.com> wrote:
>
> > Le 13/09/2019 à 09:24, Ardelean, Alexandru a écrit :
> > > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:
> > >> [External]
> > >>
> > >> This patch series fixes ad7949 driver incorrectly read data, simplify the
> > >> code, and enforces device timing constraints.
> > >>
> > >> This has been tested on a UltraZed SOM + a custom carrier equipped with
> > >> several AD7689 A/Ds. Patches have been developed on a Xilinx upstream
> > >> kernel and then rebased on linux-next kernel.
> > >>
> > > Thanks for the patches.
> > > Added Charles-Antoine to also take a look.
> > > Apologies for not thinking of adding him sooner.
> > >
> > > I typically try to review changes for ADI parts, but he wrote it, so he may have more input than I do.
> > > Jonathan will likely also take a look.
> > >
> > > If it's agreed, I would say to at least take the first patch ("iio: ad7949: kill pointless "readback"-handling code")
> > > now and see about the rest.
> > > The rest are a bit more open to discussion, so a v2 may happen.
> >
> > Hi,
> >
> > Don't worry. Due to the fact I don't have on my mail client access to
> > the whole discussions, I'm making a complete answer there based on the
> > archive of the mailing list. Sorry for that.
> >
> >
> > For the patch 1, I approve it too. This part of code is useless because
> > the feature was removed. RIP my code. :D
> >
> > For the patch 2, the cache information was added due to comment from
> > Jonathan Cameron when I developed the driver. The comment was:
> >
> > > Look very carefully at the requirements for a buffer being passed
> > > to spi_sync.  It needs to be DMA safe.  This one is not.  The usual
> > > way to do that easily is to put a cacheline aligned buffer in your
> > > ad7949_adc_chip structure.
>
> The short version of this is best illustrated with an example.
> This only applies systems where the DMA engines are not coherent
> (i.e. a change made by a DMA engine is not automatically updated to
>  all other places a copy is held in caches in the system, we have to
>  do it by hand).
>
> We have a structure like
> struct bob {
>         int initial_data;
>         u8 buffer[8];
>         int magic_flags
> };
>
> When a DMA transfer is setup involving 'buffer', the DMA engine may take
> up to a cacheline (typically 64 bytes) including buffer, make a copy of it
> and assume that the only bit of hardware working in this cacheline is itself.
> (Linux is 'guaranteeing' this when it tells the DMA engine to use this buffer.'.
> Whilst that DMA is going on, a CPU can write something in magic flags.
> That something might be important but unrelated to the DMA transfer going
> on.
>
> The DMA finishes, having put new data in the buffer element of the copy
> of the cacheline local to . It's guaranteed to not change it's copy of the
> cacheline (in this case containing the whole of bob).   However, it's version
> of magic_flags is out of date so when we flush the caches at the end of the
> non coherent DMA transfer (to force the CPU to read it from main memory and
> get the new values in buffer), the value of magic_flags can be reset to the
> version the DMA engine has.
>
> So, upshot is to avoid any potential of such problems, DMA buffers 'must'
> always be in a cacheline containing nothing that might be changed by
> other activities.  This can mean it is safe to put both TX and RX buffers
> in the same cacheline as we won't touch either during an SPI transfer.
>
> > >
> > > Lots of examples to copy, but it's also worth making sure you understand
> > > why this is necessary.
> >
> > For the endianess thing, it shouldn't be required to make an explicit
> > conversion into the driver. According to the spi.h documentation:
> >
> > > * In-memory data values are always in native CPU byte order, translated
> > > * from the wire byte order (big-endian except with SPI_LSB_FIRST). So
> > > * for example when bits_per_word is sixteen, buffers are 2N bytes long
> > > * (@len = 2N) and hold N sixteen bit words in CPU byte order.
> > So from my point of view the SPI subsystem always converts to the right
> > endianess. We don't have to take care about it.
>
> Correct, though as I commented on that patch, that's not always 'possible'
> and not all drivers set the word length 'correctly'.

Thank you both for the explanations about DMA and SPI endianess :)

So indeed 2/4 seems OK to me, and it doesn't need any further
endianess-related fix.


> Wolfram's presentation on trying to implement DMA safety in I2C at ELCE2018
> also touches on a lot of this.
>
> Thanks,
>
> Jonathan
>
> >
> >
> > For patch 3, I didn't use delay_usecs fiels due to the timings
> > definition in the datasheet in "READ/WRITE SPANNING CONVERSION WITHOUT A
> > BUSY INDICATOR" mode. During the delay, the chip select line must be
> > released which is not the case when we use delay_usecs field. So I add
> > the delay instruction after the write step to be compliant with these
> > timings.
> >
> >
> > For patch 4, I explained a bit in the other thread. Maybe we have a
> > difference of behaviour due to the choice of the timings "modes"?
> >
> >
> > BTW, from my point of view the datasheet is not totally clear about the
> > timings and what is mandatory or not in the expected behaviour.
> >
> > Regards,
> >
> > Charles-Antoine Couret
> >
>

Re: [PATCH 0/4] Fixes for ad7949

[Ardelean, Alexandru]

On Mon, 2019-09-16 at 09:39 +0200, Andrea Merello wrote:
> Il giorno dom 15 set 2019 alle ore 12:49 Jonathan Cameron
> <jic23@kernel.org> ha scritto:
> > On Fri, 13 Sep 2019 16:00:29 +0200
> > Couret Charles-Antoine <charles-antoine.couret@essensium.com> wrote:
> > 
> > > Le 13/09/2019 à 09:24, Ardelean, Alexandru a écrit :
> > > > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:
> > > > > [External]
> > > > > 
> > > > > This patch series fixes ad7949 driver incorrectly read data, simplify the
> > > > > code, and enforces device timing constraints.
> > > > > 
> > > > > This has been tested on a UltraZed SOM + a custom carrier equipped with
> > > > > several AD7689 A/Ds. Patches have been developed on a Xilinx upstream
> > > > > kernel and then rebased on linux-next kernel.
> > > > > 
> > > > Thanks for the patches.
> > > > Added Charles-Antoine to also take a look.
> > > > Apologies for not thinking of adding him sooner.
> > > > 
> > > > I typically try to review changes for ADI parts, but he wrote it, so he may have more input than I do.
> > > > Jonathan will likely also take a look.
> > > > 
> > > > If it's agreed, I would say to at least take the first patch ("iio: ad7949: kill pointless "readback"-handling
> > > > code")
> > > > now and see about the rest.
> > > > The rest are a bit more open to discussion, so a v2 may happen.
> > > 
> > > Hi,
> > > 
> > > Don't worry. Due to the fact I don't have on my mail client access to
> > > the whole discussions, I'm making a complete answer there based on the
> > > archive of the mailing list. Sorry for that.
> > > 
> > > 
> > > For the patch 1, I approve it too. This part of code is useless because
> > > the feature was removed. RIP my code. :D
> > > 
> > > For the patch 2, the cache information was added due to comment from
> > > Jonathan Cameron when I developed the driver. The comment was:
> > > 
> > > > Look very carefully at the requirements for a buffer being passed
> > > > to spi_sync.  It needs to be DMA safe.  This one is not.  The usual
> > > > way to do that easily is to put a cacheline aligned buffer in your
> > > > ad7949_adc_chip structure.
> > 
> > The short version of this is best illustrated with an example.
> > This only applies systems where the DMA engines are not coherent
> > (i.e. a change made by a DMA engine is not automatically updated to
> >  all other places a copy is held in caches in the system, we have to
> >  do it by hand).
> > 
> > We have a structure like
> > struct bob {
> >         int initial_data;
> >         u8 buffer[8];
> >         int magic_flags
> > };
> > 
> > When a DMA transfer is setup involving 'buffer', the DMA engine may take
> > up to a cacheline (typically 64 bytes) including buffer, make a copy of it
> > and assume that the only bit of hardware working in this cacheline is itself.
> > (Linux is 'guaranteeing' this when it tells the DMA engine to use this buffer.'.
> > Whilst that DMA is going on, a CPU can write something in magic flags.
> > That something might be important but unrelated to the DMA transfer going
> > on.
> > 
> > The DMA finishes, having put new data in the buffer element of the copy
> > of the cacheline local to . It's guaranteed to not change it's copy of the
> > cacheline (in this case containing the whole of bob).   However, it's version
> > of magic_flags is out of date so when we flush the caches at the end of the
> > non coherent DMA transfer (to force the CPU to read it from main memory and
> > get the new values in buffer), the value of magic_flags can be reset to the
> > version the DMA engine has.
> > 
> > So, upshot is to avoid any potential of such problems, DMA buffers 'must'
> > always be in a cacheline containing nothing that might be changed by
> > other activities.  This can mean it is safe to put both TX and RX buffers
> > in the same cacheline as we won't touch either during an SPI transfer.
> > 
> > > > Lots of examples to copy, but it's also worth making sure you understand
> > > > why this is necessary.
> > > 
> > > For the endianess thing, it shouldn't be required to make an explicit
> > > conversion into the driver. According to the spi.h documentation:
> > > 
> > > > * In-memory data values are always in native CPU byte order, translated
> > > > * from the wire byte order (big-endian except with SPI_LSB_FIRST). So
> > > > * for example when bits_per_word is sixteen, buffers are 2N bytes long
> > > > * (@len = 2N) and hold N sixteen bit words in CPU byte order.
> > > So from my point of view the SPI subsystem always converts to the right
> > > endianess. We don't have to take care about it.
> > 
> > Correct, though as I commented on that patch, that's not always 'possible'
> > and not all drivers set the word length 'correctly'.
> 
> Thank you both for the explanations about DMA and SPI endianess :)
> 
> So indeed 2/4 seems OK to me, and it doesn't need any further
> endianess-related fix.

Yep.
With these explanations:

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

> 
> 
> > Wolfram's presentation on trying to implement DMA safety in I2C at ELCE2018
> > also touches on a lot of this.
> > 
> > Thanks,
> > 
> > Jonathan
> > 
> > > 
> > > For patch 3, I didn't use delay_usecs fiels due to the timings
> > > definition in the datasheet in "READ/WRITE SPANNING CONVERSION WITHOUT A
> > > BUSY INDICATOR" mode. During the delay, the chip select line must be
> > > released which is not the case when we use delay_usecs field. So I add
> > > the delay instruction after the write step to be compliant with these
> > > timings.
> > > 
> > > 
> > > For patch 4, I explained a bit in the other thread. Maybe we have a
> > > difference of behaviour due to the choice of the timings "modes"?
> > > 
> > > 
> > > BTW, from my point of view the datasheet is not totally clear about the
> > > timings and what is mandatory or not in the expected behaviour.
> > > 
> > > Regards,
> > > 
> > > Charles-Antoine Couret
> > > 

Re: [PATCH 0/4] Fixes for ad7949

[Ardelean, Alexandru]

On Mon, 2019-09-16 at 07:48 +0000, Ardelean, Alexandru wrote:
> [External]
> 
> On Mon, 2019-09-16 at 09:39 +0200, Andrea Merello wrote:
> > Il giorno dom 15 set 2019 alle ore 12:49 Jonathan Cameron
> > <jic23@kernel.org> ha scritto:
> > > On Fri, 13 Sep 2019 16:00:29 +0200
> > > Couret Charles-Antoine <charles-antoine.couret@essensium.com> wrote:
> > > 
> > > > Le 13/09/2019 à 09:24, Ardelean, Alexandru a écrit :
> > > > > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:
> > > > > > [External]
> > > > > > 
> > > > > > This patch series fixes ad7949 driver incorrectly read data, simplify the
> > > > > > code, and enforces device timing constraints.
> > > > > > 
> > > > > > This has been tested on a UltraZed SOM + a custom carrier equipped with
> > > > > > several AD7689 A/Ds. Patches have been developed on a Xilinx upstream
> > > > > > kernel and then rebased on linux-next kernel.
> > > > > > 
> > > > > Thanks for the patches.
> > > > > Added Charles-Antoine to also take a look.
> > > > > Apologies for not thinking of adding him sooner.
> > > > > 
> > > > > I typically try to review changes for ADI parts, but he wrote it, so he may have more input than I do.
> > > > > Jonathan will likely also take a look.
> > > > > 
> > > > > If it's agreed, I would say to at least take the first patch ("iio: ad7949: kill pointless "readback"-handling
> > > > > code")
> > > > > now and see about the rest.
> > > > > The rest are a bit more open to discussion, so a v2 may happen.
> > > > 
> > > > Hi,
> > > > 
> > > > Don't worry. Due to the fact I don't have on my mail client access to
> > > > the whole discussions, I'm making a complete answer there based on the
> > > > archive of the mailing list. Sorry for that.
> > > > 
> > > > 
> > > > For the patch 1, I approve it too. This part of code is useless because
> > > > the feature was removed. RIP my code. :D
> > > > 
> > > > For the patch 2, the cache information was added due to comment from
> > > > Jonathan Cameron when I developed the driver. The comment was:
> > > > 
> > > > > Look very carefully at the requirements for a buffer being passed
> > > > > to spi_sync.  It needs to be DMA safe.  This one is not.  The usual
> > > > > way to do that easily is to put a cacheline aligned buffer in your
> > > > > ad7949_adc_chip structure.
> > > 
> > > The short version of this is best illustrated with an example.
> > > This only applies systems where the DMA engines are not coherent
> > > (i.e. a change made by a DMA engine is not automatically updated to
> > >  all other places a copy is held in caches in the system, we have to
> > >  do it by hand).
> > > 
> > > We have a structure like
> > > struct bob {
> > >         int initial_data;
> > >         u8 buffer[8];
> > >         int magic_flags
> > > };
> > > 
> > > When a DMA transfer is setup involving 'buffer', the DMA engine may take
> > > up to a cacheline (typically 64 bytes) including buffer, make a copy of it
> > > and assume that the only bit of hardware working in this cacheline is itself.
> > > (Linux is 'guaranteeing' this when it tells the DMA engine to use this buffer.'.
> > > Whilst that DMA is going on, a CPU can write something in magic flags.
> > > That something might be important but unrelated to the DMA transfer going
> > > on.
> > > 
> > > The DMA finishes, having put new data in the buffer element of the copy
> > > of the cacheline local to . It's guaranteed to not change it's copy of the
> > > cacheline (in this case containing the whole of bob).   However, it's version
> > > of magic_flags is out of date so when we flush the caches at the end of the
> > > non coherent DMA transfer (to force the CPU to read it from main memory and
> > > get the new values in buffer), the value of magic_flags can be reset to the
> > > version the DMA engine has.
> > > 
> > > So, upshot is to avoid any potential of such problems, DMA buffers 'must'
> > > always be in a cacheline containing nothing that might be changed by
> > > other activities.  This can mean it is safe to put both TX and RX buffers
> > > in the same cacheline as we won't touch either during an SPI transfer.
> > > 
> > > > > Lots of examples to copy, but it's also worth making sure you understand
> > > > > why this is necessary.
> > > > 
> > > > For the endianess thing, it shouldn't be required to make an explicit
> > > > conversion into the driver. According to the spi.h documentation:
> > > > 
> > > > > * In-memory data values are always in native CPU byte order, translated
> > > > > * from the wire byte order (big-endian except with SPI_LSB_FIRST). So
> > > > > * for example when bits_per_word is sixteen, buffers are 2N bytes long
> > > > > * (@len = 2N) and hold N sixteen bit words in CPU byte order.
> > > > So from my point of view the SPI subsystem always converts to the right
> > > > endianess. We don't have to take care about it.
> > > 
> > > Correct, though as I commented on that patch, that's not always 'possible'
> > > and not all drivers set the word length 'correctly'.
> > 
> > Thank you both for the explanations about DMA and SPI endianess :)
> > 
> > So indeed 2/4 seems OK to me, and it doesn't need any further
> > endianess-related fix.
> 
> Yep.
> With these explanations:
> 
> Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 

this is for patch 2/4

> > 
> > > Wolfram's presentation on trying to implement DMA safety in I2C at ELCE2018
> > > also touches on a lot of this.
> > > 
> > > Thanks,
> > > 
> > > Jonathan
> > > 
> > > > For patch 3, I didn't use delay_usecs fiels due to the timings
> > > > definition in the datasheet in "READ/WRITE SPANNING CONVERSION WITHOUT A
> > > > BUSY INDICATOR" mode. During the delay, the chip select line must be
> > > > released which is not the case when we use delay_usecs field. So I add
> > > > the delay instruction after the write step to be compliant with these
> > > > timings.
> > > > 
> > > > 
> > > > For patch 4, I explained a bit in the other thread. Maybe we have a
> > > > difference of behaviour due to the choice of the timings "modes"?
> > > > 
> > > > 
> > > > BTW, from my point of view the datasheet is not totally clear about the
> > > > timings and what is mandatory or not in the expected behaviour.
> > > > 
> > > > Regards,
> > > > 
> > > > Charles-Antoine Couret
> > > > 

Re: [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len

[Ardelean, Alexandru]

On Sun, 2019-09-15 at 11:36 +0100, Jonathan Cameron wrote:
> On Fri, 13 Sep 2019 09:56:56 +0200
> Andrea Merello <andrea.merello@gmail.com> wrote:
> 
> > Il giorno ven 13 set 2019 alle ore 08:46 Ardelean, Alexandru
> > <alexandru.Ardelean@analog.com> ha scritto:
> > > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:  
> > > > [External]
> > > > 
> > > > This driver supports 14-bits and 16-bits devices. All of them have a 14-bit
> > > > configuration registers. All SPI trasfers, for reading AD conversion
> > > > results and for writing the configuration register, fit in two bytes.
> > > > 
> > > > The driver always uses 4-bytes xfers which seems at least pointless (maybe
> > > > even harmful). This patch trims the SPI xfer len and the buffer size to
> > > > two bytes.
> > > >  
> > > 
> > > The length reduction proposal is fine.
> > > 
> > > But, this patch raises a question about endianess.
> > > I'm actually wondering here if we need to see about maybe using a __be16 vs u16.
> > > 
> > > I'm not that kernel-savy yet about some of these low-level things to be completely sure here.
> > > So, I'd let someone else maybe handle it.  
> > 
> > Good point.. It seems that indeed not much care has been taken about
> > endianess here.. Probably we need also some le16_to_cpu() and
> > firends..
> 
> More complexity here :)  So a lot of earlier SPI drivers didn't set bits_per_word,
> the result of this is that a read had no way to know how to unwind the endian
> nature of the data.  If you do a 4 byte read, is that 4x 1 byte, 2x 2 bytes or
> 1x 4 bytes.  Thus the SPI subsystem had no way of knowing how to convert from
> wire order of big endian to cpu endianness.  This is particularly fun as it
> is common to have variable length registers on SPI devices (be it described
> on the datasheet as some registers have high and low byte addresses).
> 
> In drivers where this can be set to one consistent value, then the SPI subsystem
> should do the work for us. Hence this one should be fine. ( I think :)
> 

Based on other input:

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

> > Maybe another separate patch can be made to take care about endianess later on?
> > 
> > BTW Also, the  ____cacheline_aligned is a bit scaring :) I don't know
> > what is that for...
> > 
> > > Thanks
> > > Alex
> > >  
> > > > Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> > > > ---
> > > >  drivers/iio/adc/ad7949.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > > > index 518044c31a73..5c2b3446fa4a 100644
> > > > --- a/drivers/iio/adc/ad7949.c
> > > > +++ b/drivers/iio/adc/ad7949.c
> > > > @@ -54,7 +54,7 @@ struct ad7949_adc_chip {
> > > >       u8 resolution;
> > > >       u16 cfg;
> > > >       unsigned int current_channel;
> > > > -     u32 buffer ____cacheline_aligned;
> > > > +     u16 buffer ____cacheline_aligned;
> > > >  };
> > > > 
> > > >  static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> > > > @@ -67,7 +67,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> > > >       struct spi_transfer tx[] = {
> > > >               {
> > > >                       .tx_buf = &ad7949_adc->buffer,
> > > > -                     .len = 4,
> > > > +                     .len = 2,
> > > >                       .bits_per_word = bits_per_word,
> > > >               },
> > > >       };
> > > > @@ -95,7 +95,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> > > >       struct spi_transfer tx[] = {
> > > >               {
> > > >                       .rx_buf = &ad7949_adc->buffer,
> > > > -                     .len = 4,
> > > > +                     .len = 2,
> > > >                       .bits_per_word = bits_per_word,
> > > >               },
> > > >       };  

Re: [PATCH 4/4] iio: ad7949: fix channels mixups

[Andrea Merello]

Il giorno ven 13 set 2019 alle ore 13:30 Couret Charles-Antoine
<charles-antoine.couret@essensium.com> ha scritto:

> >> One thing that maybe could be optimized (for the driver), is in `ad7949_spi_read_channel()` to use the current-channel &
> >> not do a SPI write to change the channel if it doesn't change.
> >>
> >> Datasheets (in general) are not always obvious about describing chip behavior for SW (or for writing a driver), but I
> >> would suspect that if you are reading garbage data, it could be that the channel has changed.
> >> This is true for some other ADCs.
> >> And requires testing for this one.
> > Yes, it's exactly what I've seen here. If the channel does not change
> > then the AD is already in acquisition phase on the right channel (I
> > assume it's OK to keep it in such phase indefinitely), then we can
> > just trigger a new conversion (CNV low->high, that is a dummy xfer)
> > and then read the result in following xfer, as the driver already did.
> >
> > I craft a V2 that performs the extra (3rd) spi xfer only if the
> > channel has to change.
>
> This design should be ok. I didn't implement in that way because not
> enough time to optimize the driver before release (I don't have access
> to the chip anymore) and for our workflow it was not relevant (we
> scanned all channels).

I was in the process of doing this, but, thinking again about it, I'm
not completely sure it is really OK:
Should we guarantee that the value we return as outcome of a IIO read
request (i.e. we access in_voltage_raw) is sampled _after_ the user
read request?

For example, the user might setup some other HW for the measure, or
somehow wait for the right moment for doing the measure, and then
perform the read from IIO, thus it's probably not OK if we convert a
value sampled just before the IIO read request.

If we skip the configuration rewrite when the channel doesn't change -
as discussed above - then we actually _terminate_ the acquisition when
the IIO read is triggered, that is we are converting the value sampled
right before the IIO read.

If this is OK then I'll go on, otherwise I think that we should always
do the three cycles (so that triggering IIO read always waits also for
a new acquisition phase)

Re: [PATCH 4/4] iio: ad7949: fix channels mixups

[Jonathan Cameron]

On Fri, 20 Sep 2019 09:45:21 +0200
Andrea Merello <andrea.merello@gmail.com> wrote:

> Il giorno ven 13 set 2019 alle ore 13:30 Couret Charles-Antoine
> <charles-antoine.couret@essensium.com> ha scritto:
> 
> > >> One thing that maybe could be optimized (for the driver), is in `ad7949_spi_read_channel()` to use the current-channel &
> > >> not do a SPI write to change the channel if it doesn't change.
> > >>
> > >> Datasheets (in general) are not always obvious about describing chip behavior for SW (or for writing a driver), but I
> > >> would suspect that if you are reading garbage data, it could be that the channel has changed.
> > >> This is true for some other ADCs.
> > >> And requires testing for this one.  
> > > Yes, it's exactly what I've seen here. If the channel does not change
> > > then the AD is already in acquisition phase on the right channel (I
> > > assume it's OK to keep it in such phase indefinitely), then we can
> > > just trigger a new conversion (CNV low->high, that is a dummy xfer)
> > > and then read the result in following xfer, as the driver already did.
> > >
> > > I craft a V2 that performs the extra (3rd) spi xfer only if the
> > > channel has to change.  
> >
> > This design should be ok. I didn't implement in that way because not
> > enough time to optimize the driver before release (I don't have access
> > to the chip anymore) and for our workflow it was not relevant (we
> > scanned all channels).  
> 
> I was in the process of doing this, but, thinking again about it, I'm
> not completely sure it is really OK:
> Should we guarantee that the value we return as outcome of a IIO read
> request (i.e. we access in_voltage_raw) is sampled _after_ the user
> read request?
> 
> For example, the user might setup some other HW for the measure, or
> somehow wait for the right moment for doing the measure, and then
> perform the read from IIO, thus it's probably not OK if we convert a
> value sampled just before the IIO read request.

Absolutely.  MUX in front of the sensor is a fairly common thing to see.

> 
> If we skip the configuration rewrite when the channel doesn't change -
> as discussed above - then we actually _terminate_ the acquisition when
> the IIO read is triggered, that is we are converting the value sampled
> right before the IIO read.
> 
> If this is OK then I'll go on, otherwise I think that we should always
> do the three cycles (so that triggering IIO read always waits also for
> a new acquisition phase)
An excellent point.  I agree and suspect we may have this wrong in other
sensors.  The question gets more interesting if running in buffered mode
as we have had systems using a trigger generated by an external process.
I suppose in that case we just have to deal with the offset in the fifo
etc in userspace.

Maybe we should think about adding a note to be careful of that.  Not
really sure where we would note it though...

Thanks,

Jonathan

Re: [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len

[Jonathan Cameron]

On Mon, 16 Sep 2019 07:51:30 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sun, 2019-09-15 at 11:36 +0100, Jonathan Cameron wrote:
> > On Fri, 13 Sep 2019 09:56:56 +0200
> > Andrea Merello <andrea.merello@gmail.com> wrote:
> >   
> > > Il giorno ven 13 set 2019 alle ore 08:46 Ardelean, Alexandru
> > > <alexandru.Ardelean@analog.com> ha scritto:  
> > > > On Thu, 2019-09-12 at 16:43 +0200, Andrea Merello wrote:    
> > > > > [External]
> > > > > 
> > > > > This driver supports 14-bits and 16-bits devices. All of them have a 14-bit
> > > > > configuration registers. All SPI trasfers, for reading AD conversion
> > > > > results and for writing the configuration register, fit in two bytes.
> > > > > 
> > > > > The driver always uses 4-bytes xfers which seems at least pointless (maybe
> > > > > even harmful). This patch trims the SPI xfer len and the buffer size to
> > > > > two bytes.
> > > > >    
> > > > 
> > > > The length reduction proposal is fine.
> > > > 
> > > > But, this patch raises a question about endianess.
> > > > I'm actually wondering here if we need to see about maybe using a __be16 vs u16.
> > > > 
> > > > I'm not that kernel-savy yet about some of these low-level things to be completely sure here.
> > > > So, I'd let someone else maybe handle it.    
> > > 
> > > Good point.. It seems that indeed not much care has been taken about
> > > endianess here.. Probably we need also some le16_to_cpu() and
> > > firends..  
> > 
> > More complexity here :)  So a lot of earlier SPI drivers didn't set bits_per_word,
> > the result of this is that a read had no way to know how to unwind the endian
> > nature of the data.  If you do a 4 byte read, is that 4x 1 byte, 2x 2 bytes or
> > 1x 4 bytes.  Thus the SPI subsystem had no way of knowing how to convert from
> > wire order of big endian to cpu endianness.  This is particularly fun as it
> > is common to have variable length registers on SPI devices (be it described
> > on the datasheet as some registers have high and low byte addresses).
> > 
> > In drivers where this can be set to one consistent value, then the SPI subsystem
> > should do the work for us. Hence this one should be fine. ( I think :)
> >   
> 
> Based on other input:
> 
> Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

I've applied this to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Note, as we don't have a proven case in which it causes actual harm, I haven't
marked it for stable.

Thanks,

Jonathan

> 
> > > Maybe another separate patch can be made to take care about endianess later on?
> > > 
> > > BTW Also, the  ____cacheline_aligned is a bit scaring :) I don't know
> > > what is that for...
> > >   
> > > > Thanks
> > > > Alex
> > > >    
> > > > > Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> > > > > ---
> > > > >  drivers/iio/adc/ad7949.c | 6 +++---
> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > > > > index 518044c31a73..5c2b3446fa4a 100644
> > > > > --- a/drivers/iio/adc/ad7949.c
> > > > > +++ b/drivers/iio/adc/ad7949.c
> > > > > @@ -54,7 +54,7 @@ struct ad7949_adc_chip {
> > > > >       u8 resolution;
> > > > >       u16 cfg;
> > > > >       unsigned int current_channel;
> > > > > -     u32 buffer ____cacheline_aligned;
> > > > > +     u16 buffer ____cacheline_aligned;
> > > > >  };
> > > > > 
> > > > >  static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> > > > > @@ -67,7 +67,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> > > > >       struct spi_transfer tx[] = {
> > > > >               {
> > > > >                       .tx_buf = &ad7949_adc->buffer,
> > > > > -                     .len = 4,
> > > > > +                     .len = 2,
> > > > >                       .bits_per_word = bits_per_word,
> > > > >               },
> > > > >       };
> > > > > @@ -95,7 +95,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> > > > >       struct spi_transfer tx[] = {
> > > > >               {
> > > > >                       .rx_buf = &ad7949_adc->buffer,
> > > > > -                     .len = 4,
> > > > > +                     .len = 2,
> > > > >                       .bits_per_word = bits_per_word,
> > > > >               },
> > > > >       };    

Re: [PATCH 4/4] iio: ad7949: fix channels mixups

[Andrea Merello]

Il giorno sab 21 set 2019 alle ore 19:12 Jonathan Cameron
<jic23@kernel.org> ha scritto:

> >
> > If we skip the configuration rewrite when the channel doesn't change -
> > as discussed above - then we actually _terminate_ the acquisition when
> > the IIO read is triggered, that is we are converting the value sampled
> > right before the IIO read.
> >
> > If this is OK then I'll go on, otherwise I think that we should always
> > do the three cycles (so that triggering IIO read always waits also for
> > a new acquisition phase)

I had a discussion about this with a HW engineer, he said that it's
probably not necessary to perform a full extra cycle (i.e. SPI xfer +
udelay(2)), rather, since the HW is already in acquisition, it should
be enough to perform the udelay(2) to make sure the internal capacitor
settles (if we change channel of course we need also the SPI xfer to
update the CFG).

So indeed it seems to me that:
- if CFG (channel) changes: we need three full SPI xfer (actual SPI
xfer + delay(2))
- if CFG (channel) doesn't change: we need a delay(2) [*]- to
guarantee the user sees a value sampled after the IIO read, as
discussed - and two full SPI xfer (actual SPI xfer + delay(2))

.. Indeed I also wonder if it would be enough to cycle the CS, without
performing a full SPI xfer, in order to start the conversion.. But
given that this whole thing seems to me a bit complicated and unclear,
I would stick to the dummy cycle for now..

> An excellent point.  I agree and suspect we may have this wrong in other
> sensors.  The question gets more interesting if running in buffered mode
> as we have had systems using a trigger generated by an external process.
> I suppose in that case we just have to deal with the offset in the fifo
> etc in userspace.

Yes. I'm not familiar with IIO buffered mode, but for a streaming,
continuous, read mode I guess that the user would expect some latency
anyway (might be due to the ADC or the buffering mechanism itself or
whatever).

I didn't look into this buffered thing to see how it works, but maybe
we can skip the first udelay(2) [*] in the driver in case of buffered
access?

> Maybe we should think about adding a note to be careful of that.  Not
> really sure where we would note it though...

IMHO clarifying what the API guarantees and what it doesn't in IIO
DocBook could be helpful (I didn't see it, but I might have missed
it)..

So, we could state that a single shot read must return a value sampled
after the read has been shot, and that on the other hand, when using
buffered mode one should expect some latency.. But indeed, since you
said that we might have a number of IIO drivers that actually behave
in a different way, then I'm not sure that it's a good idea; maybe we
could just drop this requirement and allow (and document) that a
single shot IIO read could just _terminate_ the sampling and trigger
the conversion on the current sampled signal? (so also in this driver
we can just not care about this)..

> Thanks,
>
> Jonathan
>
>

Re: [PATCH 4/4] iio: ad7949: fix channels mixups

[Jonathan Cameron]

On Mon, 23 Sep 2019 10:21:49 +0200
Andrea Merello <andrea.merello@gmail.com> wrote:

> Il giorno sab 21 set 2019 alle ore 19:12 Jonathan Cameron
> <jic23@kernel.org> ha scritto:
> 
> > >
> > > If we skip the configuration rewrite when the channel doesn't change -
> > > as discussed above - then we actually _terminate_ the acquisition when
> > > the IIO read is triggered, that is we are converting the value sampled
> > > right before the IIO read.
> > >
> > > If this is OK then I'll go on, otherwise I think that we should always
> > > do the three cycles (so that triggering IIO read always waits also for
> > > a new acquisition phase)  
> 
> I had a discussion about this with a HW engineer, he said that it's
> probably not necessary to perform a full extra cycle (i.e. SPI xfer +
> udelay(2)), rather, since the HW is already in acquisition, it should
> be enough to perform the udelay(2) to make sure the internal capacitor
> settles (if we change channel of course we need also the SPI xfer to
> update the CFG).
> 
> So indeed it seems to me that:
> - if CFG (channel) changes: we need three full SPI xfer (actual SPI
> xfer + delay(2))
> - if CFG (channel) doesn't change: we need a delay(2) [*]- to
> guarantee the user sees a value sampled after the IIO read, as
> discussed - and two full SPI xfer (actual SPI xfer + delay(2))
> 
> .. Indeed I also wonder if it would be enough to cycle the CS, without
> performing a full SPI xfer, in order to start the conversion.. But
> given that this whole thing seems to me a bit complicated and unclear,
> I would stick to the dummy cycle for now..
> 
> > An excellent point.  I agree and suspect we may have this wrong in other
> > sensors.  The question gets more interesting if running in buffered mode
> > as we have had systems using a trigger generated by an external process.
> > I suppose in that case we just have to deal with the offset in the fifo
> > etc in userspace.  
> 
> Yes. I'm not familiar with IIO buffered mode, but for a streaming,
> continuous, read mode I guess that the user would expect some latency
> anyway (might be due to the ADC or the buffering mechanism itself or
> whatever).

There are a few ugly corners.
1) They'd normally expect the timestamp to be as closely aligned as
possible with the reading in a given scan.  Perhaps we can think about
how to offset that if we know we are actually looking at the one before
last...
2) There are tightly coupled setups where we are switching a mux in
front of the sensor and driving a trigger off that action.
                                                _________
 _________           |--------MUX CNTRL--------|         |
|         |        __|__        ---TRIGGER-----|         |
| INPUT 1 |-------|     |     __|__            |         |
|_________|       |     |    |     |           |   HOST  |
 _________        | MUX |----| ADC |-----------|         |
|         |       |     |    |_____|           |         |
| INPUT 2 |-------|_____|                      |_________|   
|_________| 

This gets represented as a single 'device' that is a consumer
of the ADC channel, and also provides the trigger and handles
the mux control.

Once you have stitched it all together the expectation is that
for each 'scan':
1. Set MUX
2. Allow short settling time
3. Trigger the ADC via gpio or SPI triggered read etc.
4. ADC result comes back.
5. Goto 1.

The full set of inputs are sampled to make up one 'scan' in
the buffer of the container driver.

Now in theory you could interleave the flows so that you know
the data is coming 2 scan's later, but there isn't currently
a way for the 'container' driver to know that is happening,
so the assumption it makes is that everything is 'direct'.

We could add some means of reporting an offset from trigger
to sample into fifo as that would allow such a container
driver to adjust it's mux settings to take that into account.

Note that out container driver also often has it's own
capture trigger coming in so it can get really fiddly as
we don't have any way of knowing if we are in a round robin
situation or just taking a single 'scan'. In single scan
case we want to drop the excess reads from the end, in round robin
we want to keep them as they are the start of the next scan.

> 
> I didn't look into this buffered thing to see how it works, but maybe
> we can skip the first udelay(2) [*] in the driver in case of buffered
> access?
> 
> > Maybe we should think about adding a note to be careful of that.  Not
> > really sure where we would note it though...  
> 
> IMHO clarifying what the API guarantees and what it doesn't in IIO
> DocBook could be helpful (I didn't see it, but I might have missed
> it)..
I agree we should clarify it, but I'm still not totally sure what the
preferred case is! Perhaps best plan is to put forward a patch to add
a description of one of the choices as we can push people to review
that closely as it may mean devices don't comply with the ABI.

There is a 3rd option which is to add the option for devices to describe
what they are doing via a new sysfs attribute.  That may get us
around causing potential breakage in drivers that are already doing it
the 'wrong' way.

> 
> So, we could state that a single shot read must return a value sampled
> after the read has been shot, and that on the other hand, when using
> buffered mode one should expect some latency.. But indeed, since you
> said that we might have a number of IIO drivers that actually behave
> in a different way, then I'm not sure that it's a good idea; maybe we
> could just drop this requirement and allow (and document) that a
> single shot IIO read could just _terminate_ the sampling and trigger
> the conversion on the current sampled signal? (so also in this driver
> we can just not care about this)..

I don't think we need to worry about the difference between a sample
window stopping vs one starting at the trigger.  Right now we don't
even say how long that window is, so a naive assumption would be that
we model it as instantaneous.  For single shot either model is
fine to my mind. We get the nearest practical signal to the point
of reading.  The sysfs interface is slow enough that any fine
grained control of timing is likely to be garbage anyway :)
The only exception would be really slow sensors such as light sensors
where we might be talking 100s of msecs.  In those I'd argue
we do want the capture to start only after we ask.  I think we are
fine on existing drivers for those.

Thanks,

Jonathan

> 
> > Thanks,
> >
> > Jonathan
> >
> >  

Re: [PATCH 4/4] iio: ad7949: fix channels mixups

[Jonathan Cameron]

On Fri, 18 Oct 2019 15:30:17 +0200
Andrea Merello <andrea.merello@gmail.com> wrote:

> Il giorno sab 5 ott 2019 alle ore 11:55 Jonathan Cameron <jic23@kernel.org>
> ha scritto:
> >
> > On Mon, 23 Sep 2019 10:21:49 +0200
> > Andrea Merello <andrea.merello@gmail.com> wrote:
> >  
> > > Il giorno sab 21 set 2019 alle ore 19:12 Jonathan Cameron
> > > <jic23@kernel.org> ha scritto:
> > >  
> > > > >
> > > > > If we skip the configuration rewrite when the channel doesn't  
> change -
> > > > > as discussed above - then we actually _terminate_ the acquisition  
> when
> > > > > the IIO read is triggered, that is we are converting the value  
> sampled
> > > > > right before the IIO read.
> > > > >
> > > > > If this is OK then I'll go on, otherwise I think that we should  
> always
> > > > > do the three cycles (so that triggering IIO read always waits also  
> for
> > > > > a new acquisition phase)  
> > >
> > > I had a discussion about this with a HW engineer, he said that it's
> > > probably not necessary to perform a full extra cycle (i.e. SPI xfer +
> > > udelay(2)), rather, since the HW is already in acquisition, it should
> > > be enough to perform the udelay(2) to make sure the internal capacitor
> > > settles (if we change channel of course we need also the SPI xfer to
> > > update the CFG).
> > >
> > > So indeed it seems to me that:
> > > - if CFG (channel) changes: we need three full SPI xfer (actual SPI
> > > xfer + delay(2))
> > > - if CFG (channel) doesn't change: we need a delay(2) [*]- to
> > > guarantee the user sees a value sampled after the IIO read, as
> > > discussed - and two full SPI xfer (actual SPI xfer + delay(2))
> > >
> > > .. Indeed I also wonder if it would be enough to cycle the CS, without
> > > performing a full SPI xfer, in order to start the conversion.. But
> > > given that this whole thing seems to me a bit complicated and unclear,
> > > I would stick to the dummy cycle for now..
> > >  
> > > > An excellent point.  I agree and suspect we may have this wrong in  
> other
> > > > sensors.  The question gets more interesting if running in buffered  
> mode
> > > > as we have had systems using a trigger generated by an external  
> process.
> > > > I suppose in that case we just have to deal with the offset in the  
> fifo
> > > > etc in userspace.  
> > >
> > > Yes. I'm not familiar with IIO buffered mode, but for a streaming,
> > > continuous, read mode I guess that the user would expect some latency
> > > anyway (might be due to the ADC or the buffering mechanism itself or
> > > whatever).  
> >
> > There are a few ugly corners.
> > 1) They'd normally expect the timestamp to be as closely aligned as
> > possible with the reading in a given scan.  Perhaps we can think about
> > how to offset that if we know we are actually looking at the one before
> > last...  
> 
> Maybe we could sample the timestamp whenever we start a conversion (end of
> SPI XFER), then we use this information for timestamping the value we'll
> read at the next SPI xfer (that is the outcome of the said conversion).
> Maybe we can also subtract, say, half of the acquisition time to each
> timestemp to better center it on the actual acquisition window..

I originally had a plan to 'accurately' describe time offsets so that
we could deal with the difference between devices that are self clocked
(the interrupt is after the samples are done) and devices that are
capture on demand.  It never got implemented though as it seems no one
ever cared :)

As for shifting the timestamp to reflect and earlier triggered capture
(so running a small fifo for the timestamps), that seems like a
sensible approach to keeping timestamp and sample together.

> 
> > 2) There are tightly coupled setups where we are switching a mux in
> > front of the sensor and driving a trigger off that action.
> >                                                 _________
> >  _________           |--------MUX CNTRL--------|         |
> > |         |        __|__        ---TRIGGER-----|         |
> > | INPUT 1 |-------|     |     __|__            |         |
> > |_________|       |     |    |     |           |   HOST  |
> >  _________        | MUX |----| ADC |-----------|         |
> > |         |       |     |    |_____|           |         |
> > | INPUT 2 |-------|_____|                      |_________|
> > |_________|
> >
> > This gets represented as a single 'device' that is a consumer
> > of the ADC channel, and also provides the trigger and handles
> > the mux control.
> >
> > Once you have stitched it all together the expectation is that
> > for each 'scan':
> > 1. Set MUX
> > 2. Allow short settling time  
> 
> Indeed my concern about begin vs end of acquisition window wrt switching
> time of the external mux could possibly be worked-around by just increasing
> settling time here. I also indeed agree wrt what you say below about sysfs
> [*].
> 
> So probably we don't need to worry about this at all in the chip driver.
> 
> > 3. Trigger the ADC via gpio or SPI triggered read etc.
> > 4. ADC result comes back.
> > 5. Goto 1.
> >
> > The full set of inputs are sampled to make up one 'scan' in
> > the buffer of the container driver.
> >
> > Now in theory you could interleave the flows so that you know
> > the data is coming 2 scan's later, but there isn't currently
> > a way for the 'container' driver to know that is happening,
> > so the assumption it makes is that everything is 'direct'.
> >
> > We could add some means of reporting an offset from trigger
> > to sample into fifo as that would allow such a container
> > driver to adjust it's mux settings to take that into account.
> >
> > Note that out container driver also often has it's own
> > capture trigger coming in so it can get really fiddly as
> > we don't have any way of knowing if we are in a round robin
> > situation or just taking a single 'scan'. In single scan
> > case we want to drop the excess reads from the end, in round robin
> > we want to keep them as they are the start of the next scan.  
> 
> Yes, that's what we want. But it seems we cannot easily distinguish the two
> cases.
> 
> The only thing I can think about to handle this is to take into account the
> samples ageing, and use excess samples for the next scan vs throwing them
> away depending on that. But there's the problem of choosing a threshold..
> 
> BTW Maybe if this is handled in the ADC driver then it may also adjust
> things so that all is transparent to upper layers, even getting rid of the
> offset.

It could be done in the ADC driver, but the cost would be that it would
have to run sufficient samples to ensure we are always 'fresh'.  That
is going to kill the sampling rate.   As things stand we have no way
of letting the ADC driver know that this is even desired.

> 
> > >
> > > I didn't look into this buffered thing to see how it works, but maybe
> > > we can skip the first udelay(2) [*] in the driver in case of buffered
> > > access?
> > >  
> > > > Maybe we should think about adding a note to be careful of that.  Not
> > > > really sure where we would note it though...  
> > >
> > > IMHO clarifying what the API guarantees and what it doesn't in IIO
> > > DocBook could be helpful (I didn't see it, but I might have missed
> > > it)..  
> > I agree we should clarify it, but I'm still not totally sure what the
> > preferred case is! Perhaps best plan is to put forward a patch to add
> > a description of one of the choices as we can push people to review
> > that closely as it may mean devices don't comply with the ABI.
> >
> > There is a 3rd option which is to add the option for devices to describe
> > what they are doing via a new sysfs attribute.  That may get us
> > around causing potential breakage in drivers that are already doing it
> > the 'wrong' way.  
> 
> This would be implicit if we make them reporting the offset: a zero offset
> means they behave in the simple, plain, way..
> 
> We might also want to be able to set it, so that if we do sparse single-shot
> scans we can ask the driver to provide data in the way we expect it.

Certainly a fiddly corner case.  If this had always been present we could
just have made it up to the consumer to deal with the offset.  If it wants
a single shot reading, it would just have to do N repeated readings to
flush out the ADC pipeline.   Too late for that though.  So we would have
to default to handling in the driver, and allow for it to be set to 
an offset for high sampling rate users who know how to handle it.
(new userspace).


> 
> > >
> > > So, we could state that a single shot read must return a value sampled
> > > after the read has been shot, and that on the other hand, when using
> > > buffered mode one should expect some latency.. But indeed, since you
> > > said that we might have a number of IIO drivers that actually behave
> > > in a different way, then I'm not sure that it's a good idea; maybe we
> > > could just drop this requirement and allow (and document) that a
> > > single shot IIO read could just _terminate_ the sampling and trigger
> > > the conversion on the current sampled signal? (so also in this driver
> > > we can just not care about this)..  
> >
> > I don't think we need to worry about the difference between a sample
> > window stopping vs one starting at the trigger.  Right now we don't
> > even say how long that window is, so a naive assumption would be that
> > we model it as instantaneous.  For single shot either model is
> > fine to my mind. We get the nearest practical signal to the point
> > of reading.  The sysfs interface is slow enough that any fine
> > grained control of timing is likely to be garbage anyway :)  
> 
> [*]
> 
> > The only exception would be really slow sensors such as light sensors
> > where we might be talking 100s of msecs.  In those I'd argue
> > we do want the capture to start only after we ask.  I think we are
> > fine on existing drivers for those.  
> 
> BTW Indeed the original patch tried to cover also another aspect of the
> thing: when I read e.g. in_voltage2_raw from sysfs, I guess I really want
> to be sure that I'm reading (rasonably fresh) data from channel number 2. I
> guess
> that if another process did read in_voltage3_raw before me, I still want to
> be sure I'm getting data from ch 2, not 3.
> 
> IMO the offset thing make sense only on scans performed by buffered reads,
> in which you configure a scan sequence and you are somehow interested in
> getting data from all those channels (note: the driver targeted by the patch
> does support only raw_read()).

Absolutely agree.  This stuff only applies to more complex drivers doing
buffered mode in which we have some assumptions of 'continuous' sampling.

> 
> Also, getting back to the former example, I don't know how the container
> driver accesses the ADC driver (buffered vs raw_read), but I bet that, as a
> consumer of one ADC channel, it really wants samples from the said channel
> (internal MUX), and not from another.

Uses buffered mode, which is why triggers are involved. However, if it had
the information, the consumer could basically pass through the delay to
userspace.  So if it gets the data 2 samples late, it would tell userspace that
it will provide it two samples late as well.
> 
> So making sure the internal MUX is adjusted at every raw_read() IMO seems
> still the right thing to do.. Still, I'm unsure if the whole thing of
> excess reads makes sense here, or if we can just optimize only buffered
> reads.

Definitely only optimize buffered reads.  The sysfs ones are slow anyway
so no need to be clever!  We just burn samples to get the right thing.

I'm now completely lost on where we got to with the actual change in this
driver.  Seem's Charles wasn't convinced yet (or lost track of the thread).
Perhaps a repost with the example that you gave of the sequence you were
seeing?

Definitely went down a rabbit hole here! :)

Jonathan
> 
> > Thanks,
> >
> > Jonathan
> >  
> > >  
> > > > Thanks,
> > > >
> > > > Jonathan
> > > >
> > > >  
> >  

Re: [PATCH 4/4] iio: ad7949: fix channels mixups

[Andrea Merello]

Il giorno mar 22 ott 2019 alle ore 10:56 Jonathan Cameron
<jic23@kernel.org> ha scritto:
>
> On Fri, 18 Oct 2019 15:30:17 +0200
> Andrea Merello <andrea.merello@gmail.com> wrote:
>
> > Il giorno sab 5 ott 2019 alle ore 11:55 Jonathan Cameron <jic23@kernel.org>
> > ha scritto:
> > >
> > > On Mon, 23 Sep 2019 10:21:49 +0200
> > > Andrea Merello <andrea.merello@gmail.com> wrote:
> > >
> > > > Il giorno sab 21 set 2019 alle ore 19:12 Jonathan Cameron
> > > > <jic23@kernel.org> ha scritto:
> > > >
> > > > > >
> > > > > > If we skip the configuration rewrite when the channel doesn't
> > change -
> > > > > > as discussed above - then we actually _terminate_ the acquisition
> > when
> > > > > > the IIO read is triggered, that is we are converting the value
> > sampled
> > > > > > right before the IIO read.
> > > > > >
> > > > > > If this is OK then I'll go on, otherwise I think that we should
> > always
> > > > > > do the three cycles (so that triggering IIO read always waits also
> > for
> > > > > > a new acquisition phase)
> > > >
> > > > I had a discussion about this with a HW engineer, he said that it's
> > > > probably not necessary to perform a full extra cycle (i.e. SPI xfer +
> > > > udelay(2)), rather, since the HW is already in acquisition, it should
> > > > be enough to perform the udelay(2) to make sure the internal capacitor
> > > > settles (if we change channel of course we need also the SPI xfer to
> > > > update the CFG).
> > > >
> > > > So indeed it seems to me that:
> > > > - if CFG (channel) changes: we need three full SPI xfer (actual SPI
> > > > xfer + delay(2))
> > > > - if CFG (channel) doesn't change: we need a delay(2) [*]- to
> > > > guarantee the user sees a value sampled after the IIO read, as
> > > > discussed - and two full SPI xfer (actual SPI xfer + delay(2))
> > > >
> > > > .. Indeed I also wonder if it would be enough to cycle the CS, without
> > > > performing a full SPI xfer, in order to start the conversion.. But
> > > > given that this whole thing seems to me a bit complicated and unclear,
> > > > I would stick to the dummy cycle for now..
> > > >
> > > > > An excellent point.  I agree and suspect we may have this wrong in
> > other
> > > > > sensors.  The question gets more interesting if running in buffered
> > mode
> > > > > as we have had systems using a trigger generated by an external
> > process.
> > > > > I suppose in that case we just have to deal with the offset in the
> > fifo
> > > > > etc in userspace.
> > > >
> > > > Yes. I'm not familiar with IIO buffered mode, but for a streaming,
> > > > continuous, read mode I guess that the user would expect some latency
> > > > anyway (might be due to the ADC or the buffering mechanism itself or
> > > > whatever).
> > >
> > > There are a few ugly corners.
> > > 1) They'd normally expect the timestamp to be as closely aligned as
> > > possible with the reading in a given scan.  Perhaps we can think about
> > > how to offset that if we know we are actually looking at the one before
> > > last...
> >
> > Maybe we could sample the timestamp whenever we start a conversion (end of
> > SPI XFER), then we use this information for timestamping the value we'll
> > read at the next SPI xfer (that is the outcome of the said conversion).
> > Maybe we can also subtract, say, half of the acquisition time to each
> > timestemp to better center it on the actual acquisition window..
>
> I originally had a plan to 'accurately' describe time offsets so that
> we could deal with the difference between devices that are self clocked
> (the interrupt is after the samples are done) and devices that are
> capture on demand.  It never got implemented though as it seems no one
> ever cared :)
>
> As for shifting the timestamp to reflect and earlier triggered capture
> (so running a small fifo for the timestamps), that seems like a
> sensible approach to keeping timestamp and sample together.
>
> >
> > > 2) There are tightly coupled setups where we are switching a mux in
> > > front of the sensor and driving a trigger off that action.
> > >                                                 _________
> > >  _________           |--------MUX CNTRL--------|         |
> > > |         |        __|__        ---TRIGGER-----|         |
> > > | INPUT 1 |-------|     |     __|__            |         |
> > > |_________|       |     |    |     |           |   HOST  |
> > >  _________        | MUX |----| ADC |-----------|         |
> > > |         |       |     |    |_____|           |         |
> > > | INPUT 2 |-------|_____|                      |_________|
> > > |_________|
> > >
> > > This gets represented as a single 'device' that is a consumer
> > > of the ADC channel, and also provides the trigger and handles
> > > the mux control.
> > >
> > > Once you have stitched it all together the expectation is that
> > > for each 'scan':
> > > 1. Set MUX
> > > 2. Allow short settling time
> >
> > Indeed my concern about begin vs end of acquisition window wrt switching
> > time of the external mux could possibly be worked-around by just increasing
> > settling time here. I also indeed agree wrt what you say below about sysfs
> > [*].
> >
> > So probably we don't need to worry about this at all in the chip driver.
> >
> > > 3. Trigger the ADC via gpio or SPI triggered read etc.
> > > 4. ADC result comes back.
> > > 5. Goto 1.
> > >
> > > The full set of inputs are sampled to make up one 'scan' in
> > > the buffer of the container driver.
> > >
> > > Now in theory you could interleave the flows so that you know
> > > the data is coming 2 scan's later, but there isn't currently
> > > a way for the 'container' driver to know that is happening,
> > > so the assumption it makes is that everything is 'direct'.
> > >
> > > We could add some means of reporting an offset from trigger
> > > to sample into fifo as that would allow such a container
> > > driver to adjust it's mux settings to take that into account.
> > >
> > > Note that out container driver also often has it's own
> > > capture trigger coming in so it can get really fiddly as
> > > we don't have any way of knowing if we are in a round robin
> > > situation or just taking a single 'scan'. In single scan
> > > case we want to drop the excess reads from the end, in round robin
> > > we want to keep them as they are the start of the next scan.
> >
> > Yes, that's what we want. But it seems we cannot easily distinguish the two
> > cases.
> >
> > The only thing I can think about to handle this is to take into account the
> > samples ageing, and use excess samples for the next scan vs throwing them
> > away depending on that. But there's the problem of choosing a threshold..
> >
> > BTW Maybe if this is handled in the ADC driver then it may also adjust
> > things so that all is transparent to upper layers, even getting rid of the
> > offset.
>
> It could be done in the ADC driver, but the cost would be that it would
> have to run sufficient samples to ensure we are always 'fresh'.  That
> is going to kill the sampling rate.   As things stand we have no way
> of letting the ADC driver know that this is even desired.
>
> >
> > > >
> > > > I didn't look into this buffered thing to see how it works, but maybe
> > > > we can skip the first udelay(2) [*] in the driver in case of buffered
> > > > access?
> > > >
> > > > > Maybe we should think about adding a note to be careful of that.  Not
> > > > > really sure where we would note it though...
> > > >
> > > > IMHO clarifying what the API guarantees and what it doesn't in IIO
> > > > DocBook could be helpful (I didn't see it, but I might have missed
> > > > it)..
> > > I agree we should clarify it, but I'm still not totally sure what the
> > > preferred case is! Perhaps best plan is to put forward a patch to add
> > > a description of one of the choices as we can push people to review
> > > that closely as it may mean devices don't comply with the ABI.
> > >
> > > There is a 3rd option which is to add the option for devices to describe
> > > what they are doing via a new sysfs attribute.  That may get us
> > > around causing potential breakage in drivers that are already doing it
> > > the 'wrong' way.
> >
> > This would be implicit if we make them reporting the offset: a zero offset
> > means they behave in the simple, plain, way..
> >
> > We might also want to be able to set it, so that if we do sparse single-shot
> > scans we can ask the driver to provide data in the way we expect it.
>
> Certainly a fiddly corner case.  If this had always been present we could
> just have made it up to the consumer to deal with the offset.  If it wants
> a single shot reading, it would just have to do N repeated readings to
> flush out the ADC pipeline.   Too late for that though.  So we would have
> to default to handling in the driver, and allow for it to be set to
> an offset for high sampling rate users who know how to handle it.
> (new userspace).
>
>
> >
> > > >
> > > > So, we could state that a single shot read must return a value sampled
> > > > after the read has been shot, and that on the other hand, when using
> > > > buffered mode one should expect some latency.. But indeed, since you
> > > > said that we might have a number of IIO drivers that actually behave
> > > > in a different way, then I'm not sure that it's a good idea; maybe we
> > > > could just drop this requirement and allow (and document) that a
> > > > single shot IIO read could just _terminate_ the sampling and trigger
> > > > the conversion on the current sampled signal? (so also in this driver
> > > > we can just not care about this)..
> > >
> > > I don't think we need to worry about the difference between a sample
> > > window stopping vs one starting at the trigger.  Right now we don't
> > > even say how long that window is, so a naive assumption would be that
> > > we model it as instantaneous.  For single shot either model is
> > > fine to my mind. We get the nearest practical signal to the point
> > > of reading.  The sysfs interface is slow enough that any fine
> > > grained control of timing is likely to be garbage anyway :)
> >
> > [*]
> >
> > > The only exception would be really slow sensors such as light sensors
> > > where we might be talking 100s of msecs.  In those I'd argue
> > > we do want the capture to start only after we ask.  I think we are
> > > fine on existing drivers for those.
> >
> > BTW Indeed the original patch tried to cover also another aspect of the
> > thing: when I read e.g. in_voltage2_raw from sysfs, I guess I really want
> > to be sure that I'm reading (rasonably fresh) data from channel number 2. I
> > guess
> > that if another process did read in_voltage3_raw before me, I still want to
> > be sure I'm getting data from ch 2, not 3.
> >
> > IMO the offset thing make sense only on scans performed by buffered reads,
> > in which you configure a scan sequence and you are somehow interested in
> > getting data from all those channels (note: the driver targeted by the patch
> > does support only raw_read()).
>
> Absolutely agree.  This stuff only applies to more complex drivers doing
> buffered mode in which we have some assumptions of 'continuous' sampling.
>
> >
> > Also, getting back to the former example, I don't know how the container
> > driver accesses the ADC driver (buffered vs raw_read), but I bet that, as a
> > consumer of one ADC channel, it really wants samples from the said channel
> > (internal MUX), and not from another.
>
> Uses buffered mode, which is why triggers are involved. However, if it had
> the information, the consumer could basically pass through the delay to
> userspace.  So if it gets the data 2 samples late, it would tell userspace that
> it will provide it two samples late as well.
> >
> > So making sure the internal MUX is adjusted at every raw_read() IMO seems
> > still the right thing to do.. Still, I'm unsure if the whole thing of
> > excess reads makes sense here, or if we can just optimize only buffered
> > reads.
>
> Definitely only optimize buffered reads.  The sysfs ones are slow anyway
> so no need to be clever!  We just burn samples to get the right thing.
>
> I'm now completely lost on where we got to with the actual change in this
> driver.  Seem's Charles wasn't convinced yet (or lost track of the thread).
> Perhaps a repost with the example that you gave of the sequence you were
> seeing?
>
> Definitely went down a rabbit hole here! :)

Yes, Indeed I also got a bit out of focus here :) let me recap a bit:

The original patch aimed to just fix the sysfs read (the only thing
this driver currently supports) so that the resulting value really
belongs to the requested channel. On my board this was not the case,
because to make the internal mux to switch I needed an extra cycle, so
the patch made this happening.

As per my understanding (please correct me if I got something wrong)
then the outcome of our discussions about this is that we don't have
to care about i.e. start of acquisition window wrt end of the
acquisition window; so, ensuring that we are converting from the right
channel, and the data is reasonably fresh (i.e. it is not the outcome
of an extra read performed in an undefined past) should be OK.

As you said, Charles reported that he didn't face this issue. As per
your suggestion, I will repost with the example, to see if Charles or
someone else can reproduce the bug.

Apart of this, I'd say that most of what we have discussed indeed
don't apply to the current driver yet.

> Jonathan
> >
> > > Thanks,
> > >
> > > Jonathan
> > >
> > > >
> > > > > Thanks,
> > > > >
> > > > > Jonathan
> > > > >
> > > > >
> > >
>

Re: [PATCH 4/4] iio: ad7949: fix channels mixups

[Jonathan Cameron]

On Mon, 4 Nov 2019 15:12:09 +0100
Andrea Merello <andrea.merello@gmail.com> wrote:

> Il giorno mar 22 ott 2019 alle ore 10:56 Jonathan Cameron
> <jic23@kernel.org> ha scritto:
> >
> > On Fri, 18 Oct 2019 15:30:17 +0200
> > Andrea Merello <andrea.merello@gmail.com> wrote:
> >  
> > > Il giorno sab 5 ott 2019 alle ore 11:55 Jonathan Cameron <jic23@kernel.org>
> > > ha scritto:  
> > > >
> > > > On Mon, 23 Sep 2019 10:21:49 +0200
> > > > Andrea Merello <andrea.merello@gmail.com> wrote:
> > > >  
> > > > > Il giorno sab 21 set 2019 alle ore 19:12 Jonathan Cameron
> > > > > <jic23@kernel.org> ha scritto:
> > > > >  
> > > > > > >
> > > > > > > If we skip the configuration rewrite when the channel doesn't  
> > > change -  
> > > > > > > as discussed above - then we actually _terminate_ the acquisition  
> > > when  
> > > > > > > the IIO read is triggered, that is we are converting the value  
> > > sampled  
> > > > > > > right before the IIO read.
> > > > > > >
> > > > > > > If this is OK then I'll go on, otherwise I think that we should  
> > > always  
> > > > > > > do the three cycles (so that triggering IIO read always waits also  
> > > for  
> > > > > > > a new acquisition phase)  
> > > > >
> > > > > I had a discussion about this with a HW engineer, he said that it's
> > > > > probably not necessary to perform a full extra cycle (i.e. SPI xfer +
> > > > > udelay(2)), rather, since the HW is already in acquisition, it should
> > > > > be enough to perform the udelay(2) to make sure the internal capacitor
> > > > > settles (if we change channel of course we need also the SPI xfer to
> > > > > update the CFG).
> > > > >
> > > > > So indeed it seems to me that:
> > > > > - if CFG (channel) changes: we need three full SPI xfer (actual SPI
> > > > > xfer + delay(2))
> > > > > - if CFG (channel) doesn't change: we need a delay(2) [*]- to
> > > > > guarantee the user sees a value sampled after the IIO read, as
> > > > > discussed - and two full SPI xfer (actual SPI xfer + delay(2))
> > > > >
> > > > > .. Indeed I also wonder if it would be enough to cycle the CS, without
> > > > > performing a full SPI xfer, in order to start the conversion.. But
> > > > > given that this whole thing seems to me a bit complicated and unclear,
> > > > > I would stick to the dummy cycle for now..
> > > > >  
> > > > > > An excellent point.  I agree and suspect we may have this wrong in  
> > > other  
> > > > > > sensors.  The question gets more interesting if running in buffered  
> > > mode  
> > > > > > as we have had systems using a trigger generated by an external  
> > > process.  
> > > > > > I suppose in that case we just have to deal with the offset in the  
> > > fifo  
> > > > > > etc in userspace.  
> > > > >
> > > > > Yes. I'm not familiar with IIO buffered mode, but for a streaming,
> > > > > continuous, read mode I guess that the user would expect some latency
> > > > > anyway (might be due to the ADC or the buffering mechanism itself or
> > > > > whatever).  
> > > >
> > > > There are a few ugly corners.
> > > > 1) They'd normally expect the timestamp to be as closely aligned as
> > > > possible with the reading in a given scan.  Perhaps we can think about
> > > > how to offset that if we know we are actually looking at the one before
> > > > last...  
> > >
> > > Maybe we could sample the timestamp whenever we start a conversion (end of
> > > SPI XFER), then we use this information for timestamping the value we'll
> > > read at the next SPI xfer (that is the outcome of the said conversion).
> > > Maybe we can also subtract, say, half of the acquisition time to each
> > > timestemp to better center it on the actual acquisition window..  
> >
> > I originally had a plan to 'accurately' describe time offsets so that
> > we could deal with the difference between devices that are self clocked
> > (the interrupt is after the samples are done) and devices that are
> > capture on demand.  It never got implemented though as it seems no one
> > ever cared :)
> >
> > As for shifting the timestamp to reflect and earlier triggered capture
> > (so running a small fifo for the timestamps), that seems like a
> > sensible approach to keeping timestamp and sample together.
> >  
> > >  
> > > > 2) There are tightly coupled setups where we are switching a mux in
> > > > front of the sensor and driving a trigger off that action.
> > > >                                                 _________
> > > >  _________           |--------MUX CNTRL--------|         |
> > > > |         |        __|__        ---TRIGGER-----|         |
> > > > | INPUT 1 |-------|     |     __|__            |         |
> > > > |_________|       |     |    |     |           |   HOST  |
> > > >  _________        | MUX |----| ADC |-----------|         |
> > > > |         |       |     |    |_____|           |         |
> > > > | INPUT 2 |-------|_____|                      |_________|
> > > > |_________|
> > > >
> > > > This gets represented as a single 'device' that is a consumer
> > > > of the ADC channel, and also provides the trigger and handles
> > > > the mux control.
> > > >
> > > > Once you have stitched it all together the expectation is that
> > > > for each 'scan':
> > > > 1. Set MUX
> > > > 2. Allow short settling time  
> > >
> > > Indeed my concern about begin vs end of acquisition window wrt switching
> > > time of the external mux could possibly be worked-around by just increasing
> > > settling time here. I also indeed agree wrt what you say below about sysfs
> > > [*].
> > >
> > > So probably we don't need to worry about this at all in the chip driver.
> > >  
> > > > 3. Trigger the ADC via gpio or SPI triggered read etc.
> > > > 4. ADC result comes back.
> > > > 5. Goto 1.
> > > >
> > > > The full set of inputs are sampled to make up one 'scan' in
> > > > the buffer of the container driver.
> > > >
> > > > Now in theory you could interleave the flows so that you know
> > > > the data is coming 2 scan's later, but there isn't currently
> > > > a way for the 'container' driver to know that is happening,
> > > > so the assumption it makes is that everything is 'direct'.
> > > >
> > > > We could add some means of reporting an offset from trigger
> > > > to sample into fifo as that would allow such a container
> > > > driver to adjust it's mux settings to take that into account.
> > > >
> > > > Note that out container driver also often has it's own
> > > > capture trigger coming in so it can get really fiddly as
> > > > we don't have any way of knowing if we are in a round robin
> > > > situation or just taking a single 'scan'. In single scan
> > > > case we want to drop the excess reads from the end, in round robin
> > > > we want to keep them as they are the start of the next scan.  
> > >
> > > Yes, that's what we want. But it seems we cannot easily distinguish the two
> > > cases.
> > >
> > > The only thing I can think about to handle this is to take into account the
> > > samples ageing, and use excess samples for the next scan vs throwing them
> > > away depending on that. But there's the problem of choosing a threshold..
> > >
> > > BTW Maybe if this is handled in the ADC driver then it may also adjust
> > > things so that all is transparent to upper layers, even getting rid of the
> > > offset.  
> >
> > It could be done in the ADC driver, but the cost would be that it would
> > have to run sufficient samples to ensure we are always 'fresh'.  That
> > is going to kill the sampling rate.   As things stand we have no way
> > of letting the ADC driver know that this is even desired.
> >  
> > >  
> > > > >
> > > > > I didn't look into this buffered thing to see how it works, but maybe
> > > > > we can skip the first udelay(2) [*] in the driver in case of buffered
> > > > > access?
> > > > >  
> > > > > > Maybe we should think about adding a note to be careful of that.  Not
> > > > > > really sure where we would note it though...  
> > > > >
> > > > > IMHO clarifying what the API guarantees and what it doesn't in IIO
> > > > > DocBook could be helpful (I didn't see it, but I might have missed
> > > > > it)..  
> > > > I agree we should clarify it, but I'm still not totally sure what the
> > > > preferred case is! Perhaps best plan is to put forward a patch to add
> > > > a description of one of the choices as we can push people to review
> > > > that closely as it may mean devices don't comply with the ABI.
> > > >
> > > > There is a 3rd option which is to add the option for devices to describe
> > > > what they are doing via a new sysfs attribute.  That may get us
> > > > around causing potential breakage in drivers that are already doing it
> > > > the 'wrong' way.  
> > >
> > > This would be implicit if we make them reporting the offset: a zero offset
> > > means they behave in the simple, plain, way..
> > >
> > > We might also want to be able to set it, so that if we do sparse single-shot
> > > scans we can ask the driver to provide data in the way we expect it.  
> >
> > Certainly a fiddly corner case.  If this had always been present we could
> > just have made it up to the consumer to deal with the offset.  If it wants
> > a single shot reading, it would just have to do N repeated readings to
> > flush out the ADC pipeline.   Too late for that though.  So we would have
> > to default to handling in the driver, and allow for it to be set to
> > an offset for high sampling rate users who know how to handle it.
> > (new userspace).
> >
> >  
> > >  
> > > > >
> > > > > So, we could state that a single shot read must return a value sampled
> > > > > after the read has been shot, and that on the other hand, when using
> > > > > buffered mode one should expect some latency.. But indeed, since you
> > > > > said that we might have a number of IIO drivers that actually behave
> > > > > in a different way, then I'm not sure that it's a good idea; maybe we
> > > > > could just drop this requirement and allow (and document) that a
> > > > > single shot IIO read could just _terminate_ the sampling and trigger
> > > > > the conversion on the current sampled signal? (so also in this driver
> > > > > we can just not care about this)..  
> > > >
> > > > I don't think we need to worry about the difference between a sample
> > > > window stopping vs one starting at the trigger.  Right now we don't
> > > > even say how long that window is, so a naive assumption would be that
> > > > we model it as instantaneous.  For single shot either model is
> > > > fine to my mind. We get the nearest practical signal to the point
> > > > of reading.  The sysfs interface is slow enough that any fine
> > > > grained control of timing is likely to be garbage anyway :)  
> > >
> > > [*]
> > >  
> > > > The only exception would be really slow sensors such as light sensors
> > > > where we might be talking 100s of msecs.  In those I'd argue
> > > > we do want the capture to start only after we ask.  I think we are
> > > > fine on existing drivers for those.  
> > >
> > > BTW Indeed the original patch tried to cover also another aspect of the
> > > thing: when I read e.g. in_voltage2_raw from sysfs, I guess I really want
> > > to be sure that I'm reading (rasonably fresh) data from channel number 2. I
> > > guess
> > > that if another process did read in_voltage3_raw before me, I still want to
> > > be sure I'm getting data from ch 2, not 3.
> > >
> > > IMO the offset thing make sense only on scans performed by buffered reads,
> > > in which you configure a scan sequence and you are somehow interested in
> > > getting data from all those channels (note: the driver targeted by the patch
> > > does support only raw_read()).  
> >
> > Absolutely agree.  This stuff only applies to more complex drivers doing
> > buffered mode in which we have some assumptions of 'continuous' sampling.
> >  
> > >
> > > Also, getting back to the former example, I don't know how the container
> > > driver accesses the ADC driver (buffered vs raw_read), but I bet that, as a
> > > consumer of one ADC channel, it really wants samples from the said channel
> > > (internal MUX), and not from another.  
> >
> > Uses buffered mode, which is why triggers are involved. However, if it had
> > the information, the consumer could basically pass through the delay to
> > userspace.  So if it gets the data 2 samples late, it would tell userspace that
> > it will provide it two samples late as well.  
> > >
> > > So making sure the internal MUX is adjusted at every raw_read() IMO seems
> > > still the right thing to do.. Still, I'm unsure if the whole thing of
> > > excess reads makes sense here, or if we can just optimize only buffered
> > > reads.  
> >
> > Definitely only optimize buffered reads.  The sysfs ones are slow anyway
> > so no need to be clever!  We just burn samples to get the right thing.
> >
> > I'm now completely lost on where we got to with the actual change in this
> > driver.  Seem's Charles wasn't convinced yet (or lost track of the thread).
> > Perhaps a repost with the example that you gave of the sequence you were
> > seeing?
> >
> > Definitely went down a rabbit hole here! :)  
> 
> Yes, Indeed I also got a bit out of focus here :) let me recap a bit:
> 
> The original patch aimed to just fix the sysfs read (the only thing
> this driver currently supports) so that the resulting value really
> belongs to the requested channel. On my board this was not the case,
> because to make the internal mux to switch I needed an extra cycle, so
> the patch made this happening.
> 
> As per my understanding (please correct me if I got something wrong)
> then the outcome of our discussions about this is that we don't have
> to care about i.e. start of acquisition window wrt end of the
> acquisition window; so, ensuring that we are converting from the right
> channel, and the data is reasonably fresh (i.e. it is not the outcome
> of an extra read performed in an undefined past) should be OK.
I don't think reasonably fresh is good enough.  It needs to occur
'after' the request is made to read it.   Thus userspace nasty controls
of an external mux could set the mux and then trigger the read, safe
in the assumption that if they leave the mux alone until they get
a value they are safe.

> 
> As you said, Charles reported that he didn't face this issue. As per
> your suggestion, I will repost with the example, to see if Charles or
> someone else can reproduce the bug.
That would be great.  Certainly curious that it's not repeating consistently.
> 
> Apart of this, I'd say that most of what we have discussed indeed
> don't apply to the current driver yet.
True enough ;)

Thanks,

Jonathan
> 
> > Jonathan  
> > >  
> > > > Thanks,
> > > >
> > > > Jonathan
> > > >  
> > > > >  
> > > > > > Thanks,
> > > > > >
> > > > > > Jonathan
> > > > > >
> > > > > >  
> > > >  
> >  

Re: [PATCH 4/4] iio: ad7949: fix channels mixups

[Couret Charles-Antoine]

Le 04/11/2019 à 15:12, Andrea Merello a écrit :
>> Il giorno mar 22 ott 2019 alle ore 10:56 Jonathan Cameron
>> <jic23@kernel.org> ha scritto:
>> As you said, Charles reported that he didn't face this issue. As per
>> your suggestion, I will repost with the example, to see if Charles or
>> someone else can reproduce the bug.
>>
>> Apart of this, I'd say that most of what we have discussed indeed
>> don't apply to the current driver yet.
> As you said, Charles reported that he didn't face this issue. As per
> your suggestion, I will repost with the example, to see if Charles or
> someone else can reproduce the bug.

Hi guys,

Sorry for the delay. Yes, when I developed the driver I was able to 
switch from any to any channel without issues with this driver. But from 
my understanding, there are different modes for this device and those 
modes can impact the timings to perform the sampling + getting the 
result. Maybe it can explain this situation?

Unfortunately I don't have access to this chip anymore so I can't 
reproduce it again. And obviously I can't confirm your changes. If on 
your side the change works fine, I don't see any reason to refuse it.


Regards,

Charles-Antoine Couret

[v2] iio: ad7949: fix channels mixups

[Andrea Merello]

Each time we need to read a sample (from the sysfs interface, since the
driver supports only it) the driver writes the configuration register
with the proper settings needed to perform the said read, then it runs
another xfer to actually read the resulting value. Most notably the
configuration register is updated to set the ADC internal MUX depending by
which channel the read targets.

Unfortunately this seems not enough to ensure correct operation because
the ADC works in a pipelined-like fashion and the new configuration isn't
applied in time.

The ADC alternates two phases: acquisition and conversion. During the
acquisition phase the ADC samples the analog signal in an internal
capacitor; in the conversion phase the ADC performs the actual analog to
digital conversion of the stored voltage. Note that of course the MUX
needs to be set to the proper channel when the acquisition phase is
performed.

Once the conversion phase has been completed, the device automatically
switches back to a new acquisition; on the other hand the device switches
from acquisition to conversion on the rising edge of SPI cs signal (that
is when the xfer finishes).

Only after both two phases have been completed (with the proper settings
already written in the configuration register since the beginning) it is
possible to read the outcome from SPI bus.

With the current driver implementation, we end up in the following
situation:

        _______  1st xfer ____________  2nd xfer ___________________
SPI cs..       \_________/            \_________/
SPI rd.. idle  |(val N-2)+    idle    | val N-1 +   idle ...
SPI wr.. idle  |  cfg N  +    idle    |   (X)   +   idle ...
------------------------ + -------------------- + ------------------
  AD  ..   acq  N-1      + cnv N-1 |  acq N     +  cnv N  | acq N+1

As shown in the diagram above, the value we read in the Nth read belongs
to configuration setting N-1.

In case the configuration is not changed (config[N] == config[N-1]), then
we still get correct data, but in case the configuration changes (i.e.
switching the MUX on another channel), we get wrong data (data from the
previously selected channel).

This patch fixes this by performing one more "dummy" transfer in order to
ending up in reading the data when it's really ready, as per the following
timing diagram.

        _______  1st xfer ____________  2nd xfer ___________  3rd xfer ___
SPI cs..       \_________/            \_________/           \_________/
SPI rd.. idle  |(val N-2)+    idle    |(val N-1)+    idle   |  val N  + ..
SPI wr.. idle  |  cfg N  +    idle    |   (X)   +    idle   |   (X)   + ..
------------------------ + -------------------- + ------------------- + --
  AD  ..   acq  N-1      + cnv N-1 |  acq N     +  cnv N  | acq N+1   | ..

NOTE: in the latter case (cfg changes), the acquisition phase for the
value to be read begins after the 1st xfer, that is after the read request
has been issued on sysfs. On the other hand, if the cfg doesn't change,
then we can refer to the fist diagram assuming N == (N - 1); the
acquisition phase _begins_ before the 1st xfer (potentially a lot of time
before the read has been issued via sysfs, but it _ends_ after the 1st
xfer, that is _after_ the read has started. This should guarantee a
reasonably fresh data, which value represents the voltage that the sampled
signal has after the read start or maybe just around it.

 Changes in V2
- Reword, with more detailed explanation
- Make the 3rd xfer conditional (only if prev cfg != new cfg)
- Clarify code comments

Cc: Couret Charles-Antoine <charles-antoine.couret@essensium.com>
Cc: Alexandru Ardelean <alexandru.Ardelean@analog.com>
Cc: Michael Hennerich <Michael.Hennerich@analog.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Stefan Popa <stefan.popa@analog.com>
Cc: Hartmut Knaack <knaack.h@gmx.de>
Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Cc: linux-iio@vger.kernel.org
Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 drivers/iio/adc/ad7949.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 5c2b3446fa4a..2c6f60edb7ce 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -89,6 +89,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 				   unsigned int channel)
 {
 	int ret;
+	int i;
 	int bits_per_word = ad7949_adc->resolution;
 	int mask = GENMASK(ad7949_adc->resolution, 0);
 	struct spi_message msg;
@@ -100,12 +101,23 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 		},
 	};
 
-	ret = ad7949_spi_write_cfg(ad7949_adc,
-				   channel << AD7949_OFFSET_CHANNEL_SEL,
-				   AD7949_MASK_CHANNEL_SEL);
-	if (ret)
-		return ret;
+	/*
+	 * 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
+	 *    at the next xfer, so we bail out now, otherwise we write something
+	 *    and we read garbage (sample N-1 configuration).
+	 */
+	for (i = 0; i < 2; i++) {
+		ret = ad7949_spi_write_cfg(ad7949_adc,
+					   channel << AD7949_OFFSET_CHANNEL_SEL,
+					   AD7949_MASK_CHANNEL_SEL);
+		if (ret)
+			return ret;
+		if (channel == ad7949_adc->current_channel)
+			break;
+	}
 
+	/* 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);
-- 
2.17.1

Re: [v2] iio: ad7949: fix channels mixups

[Couret Charles-Antoine]

Le 02/12/2019 à 15:13, Andrea Merello a écrit :
>   drivers/iio/adc/ad7949.c | 22 +++++++++++++++++-----
>   1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index 5c2b3446fa4a..2c6f60edb7ce 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -89,6 +89,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>   				   unsigned int channel)
>   {
>   	int ret;
> +	int i;
>   	int bits_per_word = ad7949_adc->resolution;
>   	int mask = GENMASK(ad7949_adc->resolution, 0);
>   	struct spi_message msg;
> @@ -100,12 +101,23 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>   		},
>   	};
>   
> -	ret = ad7949_spi_write_cfg(ad7949_adc,
> -				   channel << AD7949_OFFSET_CHANNEL_SEL,
> -				   AD7949_MASK_CHANNEL_SEL);
> -	if (ret)
> -		return ret;
> +	/*
> +	 * 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
> +	 *    at the next xfer, so we bail out now, otherwise we write something
> +	 *    and we read garbage (sample N-1 configuration).
> +	 */
> +	for (i = 0; i < 2; i++) {
> +		ret = ad7949_spi_write_cfg(ad7949_adc,
> +					   channel << AD7949_OFFSET_CHANNEL_SEL,
> +					   AD7949_MASK_CHANNEL_SEL);
> +		if (ret)
> +			return ret;
> +		if (channel == ad7949_adc->current_channel)
> +			break;
> +	}
>   
> +	/* 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);

Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com>

Regards,
Charles-Antoine Couret

Re: [v2] iio: ad7949: fix channels mixups

[Jonathan Cameron]

On Mon, 2 Dec 2019 16:36:19 +0100
Couret Charles-Antoine <charles-antoine.couret@essensium.com> wrote:

> Le 02/12/2019 à 15:13, Andrea Merello a écrit :
> >   drivers/iio/adc/ad7949.c | 22 +++++++++++++++++-----
> >   1 file changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > index 5c2b3446fa4a..2c6f60edb7ce 100644
> > --- a/drivers/iio/adc/ad7949.c
> > +++ b/drivers/iio/adc/ad7949.c
> > @@ -89,6 +89,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> >   				   unsigned int channel)
> >   {
> >   	int ret;
> > +	int i;
> >   	int bits_per_word = ad7949_adc->resolution;
> >   	int mask = GENMASK(ad7949_adc->resolution, 0);
> >   	struct spi_message msg;
> > @@ -100,12 +101,23 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> >   		},
> >   	};
> >   
> > -	ret = ad7949_spi_write_cfg(ad7949_adc,
> > -				   channel << AD7949_OFFSET_CHANNEL_SEL,
> > -				   AD7949_MASK_CHANNEL_SEL);
> > -	if (ret)
> > -		return ret;
> > +	/*
> > +	 * 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
> > +	 *    at the next xfer, so we bail out now, otherwise we write something
> > +	 *    and we read garbage (sample N-1 configuration).
> > +	 */
> > +	for (i = 0; i < 2; i++) {
> > +		ret = ad7949_spi_write_cfg(ad7949_adc,
> > +					   channel << AD7949_OFFSET_CHANNEL_SEL,
> > +					   AD7949_MASK_CHANNEL_SEL);
> > +		if (ret)
> > +			return ret;
> > +		if (channel == ad7949_adc->current_channel)
> > +			break;
> > +	}
> >   
> > +	/* 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);  
> 
> Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com>
Hi Charles-Antoine,

Why a signed-off-by as opposed to a reviewed-by or similar?

signed-off-by brings some very specific legal implications around Developer
Certificate of Origin, so is normally just the author plus people involved in
the path to upstream (maintainers).

> 
> Regards,
> Charles-Antoine Couret

Re: [v2] iio: ad7949: fix channels mixups

[Couret Charles-Antoine]

Le 04/12/2019 à 12:06, Jonathan Cameron a écrit :
> On Mon, 2 Dec 2019 16:36:19 +0100
> Couret Charles-Antoine <charles-antoine.couret@essensium.com> wrote:
>
>> Le 02/12/2019 à 15:13, Andrea Merello a écrit :
>>>    drivers/iio/adc/ad7949.c | 22 +++++++++++++++++-----
>>>    1 file changed, 17 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
>>> index 5c2b3446fa4a..2c6f60edb7ce 100644
>>> --- a/drivers/iio/adc/ad7949.c
>>> +++ b/drivers/iio/adc/ad7949.c
>>> @@ -89,6 +89,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>>>    				   unsigned int channel)
>>>    {
>>>    	int ret;
>>> +	int i;
>>>    	int bits_per_word = ad7949_adc->resolution;
>>>    	int mask = GENMASK(ad7949_adc->resolution, 0);
>>>    	struct spi_message msg;
>>> @@ -100,12 +101,23 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>>>    		},
>>>    	};
>>>    
>>> -	ret = ad7949_spi_write_cfg(ad7949_adc,
>>> -				   channel << AD7949_OFFSET_CHANNEL_SEL,
>>> -				   AD7949_MASK_CHANNEL_SEL);
>>> -	if (ret)
>>> -		return ret;
>>> +	/*
>>> +	 * 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
>>> +	 *    at the next xfer, so we bail out now, otherwise we write something
>>> +	 *    and we read garbage (sample N-1 configuration).
>>> +	 */
>>> +	for (i = 0; i < 2; i++) {
>>> +		ret = ad7949_spi_write_cfg(ad7949_adc,
>>> +					   channel << AD7949_OFFSET_CHANNEL_SEL,
>>> +					   AD7949_MASK_CHANNEL_SEL);
>>> +		if (ret)
>>> +			return ret;
>>> +		if (channel == ad7949_adc->current_channel)
>>> +			break;
>>> +	}
>>>    
>>> +	/* 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);
>> Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com>
> Hi Charles-Antoine,
>
> Why a signed-off-by as opposed to a reviewed-by or similar?
>
> signed-off-by brings some very specific legal implications around Developer
> Certificate of Origin, so is normally just the author plus people involved in
> the path to upstream (maintainers).

Ah sorry, I made the mistake, I wanted to add:

Reviewed-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com>

Regards,

Charles-Antoine COuret

Re: [v2] iio: ad7949: fix channels mixups

[Jonathan Cameron]

On Wed, 4 Dec 2019 12:13:03 +0100
Couret Charles-Antoine <charles-antoine.couret@essensium.com> wrote:

> Le 04/12/2019 à 12:06, Jonathan Cameron a écrit :
> > On Mon, 2 Dec 2019 16:36:19 +0100
> > Couret Charles-Antoine <charles-antoine.couret@essensium.com> wrote:
> >  
> >> Le 02/12/2019 à 15:13, Andrea Merello a écrit :  
> >>>    drivers/iio/adc/ad7949.c | 22 +++++++++++++++++-----
> >>>    1 file changed, 17 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> >>> index 5c2b3446fa4a..2c6f60edb7ce 100644
> >>> --- a/drivers/iio/adc/ad7949.c
> >>> +++ b/drivers/iio/adc/ad7949.c
> >>> @@ -89,6 +89,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> >>>    				   unsigned int channel)
> >>>    {
> >>>    	int ret;
> >>> +	int i;
> >>>    	int bits_per_word = ad7949_adc->resolution;
> >>>    	int mask = GENMASK(ad7949_adc->resolution, 0);
> >>>    	struct spi_message msg;
> >>> @@ -100,12 +101,23 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> >>>    		},
> >>>    	};
> >>>    
> >>> -	ret = ad7949_spi_write_cfg(ad7949_adc,
> >>> -				   channel << AD7949_OFFSET_CHANNEL_SEL,
> >>> -				   AD7949_MASK_CHANNEL_SEL);
> >>> -	if (ret)
> >>> -		return ret;
> >>> +	/*
> >>> +	 * 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
> >>> +	 *    at the next xfer, so we bail out now, otherwise we write something
> >>> +	 *    and we read garbage (sample N-1 configuration).
> >>> +	 */
> >>> +	for (i = 0; i < 2; i++) {
> >>> +		ret = ad7949_spi_write_cfg(ad7949_adc,
> >>> +					   channel << AD7949_OFFSET_CHANNEL_SEL,
> >>> +					   AD7949_MASK_CHANNEL_SEL);
> >>> +		if (ret)
> >>> +			return ret;
> >>> +		if (channel == ad7949_adc->current_channel)
> >>> +			break;
> >>> +	}
> >>>    
> >>> +	/* 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);  
> >> Signed-off-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com>  
> > Hi Charles-Antoine,
> >
> > Why a signed-off-by as opposed to a reviewed-by or similar?
> >
> > signed-off-by brings some very specific legal implications around Developer
> > Certificate of Origin, so is normally just the author plus people involved in
> > the path to upstream (maintainers).  
> 
> Ah sorry, I made the mistake, I wanted to add:
> 
> Reviewed-by: Charles-Antoine Couret <charles-antoine.couret@essensium.com>
Great thanks.

Applied to the fixes-togreg branch of iio.git.  I've also marked it for stable.

Thanks,

Jonathan

> 
> Regards,
> 
> Charles-Antoine COuret
>