| Message ID | 20191202141336.4507-1-andrea.merello@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iio: ad7949: fix channels mixups | expand |
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
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
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
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 >
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);
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(-)