* [PATCH 1/4] iio: ad7949: kill pointless "readback"-handling code
2019-09-12 14:43 [PATCH 0/4] Fixes for ad7949 Andrea Merello
@ 2019-09-12 14:43 ` Andrea Merello
2019-09-13 6:37 ` Ardelean, Alexandru
2019-09-12 14:43 ` [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len Andrea Merello
` (3 subsequent siblings)
4 siblings, 1 reply; 40+ messages in thread
From: Andrea Merello @ 2019-09-12 14:43 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw
Cc: linux-iio, 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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH 1/4] iio: ad7949: kill pointless "readback"-handling code
2019-09-12 14:43 ` [PATCH 1/4] iio: ad7949: kill pointless "readback"-handling code Andrea Merello
@ 2019-09-13 6:37 ` Ardelean, Alexandru
2019-09-15 10:26 ` Jonathan Cameron
0 siblings, 1 reply; 40+ messages in thread
From: Ardelean, Alexandru @ 2019-09-13 6:37 UTC (permalink / raw)
To: andrea.merello, Hennerich, Michael, lars, jic23, pmeerw, knaack.h
Cc: linux-iio
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;
> }
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 1/4] iio: ad7949: kill pointless "readback"-handling code
2019-09-13 6:37 ` Ardelean, Alexandru
@ 2019-09-15 10:26 ` Jonathan Cameron
0 siblings, 0 replies; 40+ messages in thread
From: Jonathan Cameron @ 2019-09-15 10:26 UTC (permalink / raw)
To: Ardelean, Alexandru
Cc: andrea.merello, Hennerich, Michael, lars, pmeerw, knaack.h, linux-iio
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;
> > }
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len
2019-09-12 14:43 [PATCH 0/4] Fixes for ad7949 Andrea Merello
2019-09-12 14:43 ` [PATCH 1/4] iio: ad7949: kill pointless "readback"-handling code Andrea Merello
@ 2019-09-12 14:43 ` Andrea Merello
2019-09-13 6:46 ` Ardelean, Alexandru
2019-09-12 14:43 ` [PATCH 3/4] iio: ad7949: fix SPI xfer delays Andrea Merello
` (2 subsequent siblings)
4 siblings, 1 reply; 40+ messages in thread
From: Andrea Merello @ 2019-09-12 14:43 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw
Cc: linux-iio, 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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len
2019-09-12 14:43 ` [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len Andrea Merello
@ 2019-09-13 6:46 ` Ardelean, Alexandru
2019-09-13 7:56 ` Andrea Merello
0 siblings, 1 reply; 40+ messages in thread
From: Ardelean, Alexandru @ 2019-09-13 6:46 UTC (permalink / raw)
To: andrea.merello, Hennerich, Michael, lars, jic23, pmeerw, knaack.h
Cc: linux-iio
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,
> },
> };
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len
2019-09-13 6:46 ` Ardelean, Alexandru
@ 2019-09-13 7:56 ` Andrea Merello
2019-09-13 8:28 ` Ardelean, Alexandru
2019-09-15 10:36 ` Jonathan Cameron
0 siblings, 2 replies; 40+ messages in thread
From: Andrea Merello @ 2019-09-13 7:56 UTC (permalink / raw)
To: Ardelean, Alexandru
Cc: Hennerich, Michael, lars, jic23, pmeerw, knaack.h, linux-iio,
antoine.couret
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,
> > },
> > };
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len
2019-09-13 7:56 ` Andrea Merello
@ 2019-09-13 8:28 ` Ardelean, Alexandru
2019-09-15 10:36 ` Jonathan Cameron
1 sibling, 0 replies; 40+ messages in thread
From: Ardelean, Alexandru @ 2019-09-13 8:28 UTC (permalink / raw)
To: andrea.merello
Cc: antoine.couret, Hennerich, Michael, jic23, lars, pmeerw,
linux-iio, knaack.h
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,
> > > },
> > > };
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len
2019-09-13 7:56 ` Andrea Merello
2019-09-13 8:28 ` Ardelean, Alexandru
@ 2019-09-15 10:36 ` Jonathan Cameron
2019-09-16 7:51 ` Ardelean, Alexandru
1 sibling, 1 reply; 40+ messages in thread
From: Jonathan Cameron @ 2019-09-15 10:36 UTC (permalink / raw)
To: Andrea Merello
Cc: Ardelean, Alexandru, Hennerich, Michael, lars, pmeerw, knaack.h,
linux-iio, antoine.couret, Couret Charles-Antoine
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,
> > > },
> > > };
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len
2019-09-15 10:36 ` Jonathan Cameron
@ 2019-09-16 7:51 ` Ardelean, Alexandru
2019-09-21 17:16 ` Jonathan Cameron
0 siblings, 1 reply; 40+ messages in thread
From: Ardelean, Alexandru @ 2019-09-16 7:51 UTC (permalink / raw)
To: jic23, andrea.merello
Cc: antoine.couret, Hennerich, Michael, charles-antoine.couret, lars,
pmeerw, linux-iio, knaack.h
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,
> > > > },
> > > > };
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len
2019-09-16 7:51 ` Ardelean, Alexandru
@ 2019-09-21 17:16 ` Jonathan Cameron
0 siblings, 0 replies; 40+ messages in thread
From: Jonathan Cameron @ 2019-09-21 17:16 UTC (permalink / raw)
To: Ardelean, Alexandru
Cc: andrea.merello, antoine.couret, Hennerich, Michael,
charles-antoine.couret, lars, pmeerw, linux-iio, knaack.h
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,
> > > > > },
> > > > > };
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 3/4] iio: ad7949: fix SPI xfer delays
2019-09-12 14:43 [PATCH 0/4] Fixes for ad7949 Andrea Merello
2019-09-12 14:43 ` [PATCH 1/4] iio: ad7949: kill pointless "readback"-handling code Andrea Merello
2019-09-12 14:43 ` [PATCH 2/4] iio: ad7949: fix incorrect SPI xfer len Andrea Merello
@ 2019-09-12 14:43 ` Andrea Merello
2019-09-13 6:59 ` Ardelean, Alexandru
2019-09-12 14:43 ` [PATCH 4/4] iio: ad7949: fix channels mixups Andrea Merello
2019-09-13 7:24 ` [PATCH 0/4] Fixes for ad7949 Ardelean, Alexandru
4 siblings, 1 reply; 40+ messages in thread
From: Andrea Merello @ 2019-09-12 14:43 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw
Cc: linux-iio, 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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH 3/4] iio: ad7949: fix SPI xfer delays
2019-09-12 14:43 ` [PATCH 3/4] iio: ad7949: fix SPI xfer delays Andrea Merello
@ 2019-09-13 6:59 ` Ardelean, Alexandru
2019-09-13 8:23 ` Andrea Merello
0 siblings, 1 reply; 40+ messages in thread
From: Ardelean, Alexandru @ 2019-09-13 6:59 UTC (permalink / raw)
To: andrea.merello, Hennerich, Michael, lars, jic23, pmeerw, knaack.h
Cc: linux-iio
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;
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 3/4] iio: ad7949: fix SPI xfer delays
2019-09-13 6:59 ` Ardelean, Alexandru
@ 2019-09-13 8:23 ` Andrea Merello
2019-09-13 8:43 ` Ardelean, Alexandru
0 siblings, 1 reply; 40+ messages in thread
From: Andrea Merello @ 2019-09-13 8:23 UTC (permalink / raw)
To: Ardelean, Alexandru
Cc: Hennerich, Michael, lars, jic23, pmeerw, knaack.h, linux-iio
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;
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 3/4] iio: ad7949: fix SPI xfer delays
2019-09-13 8:23 ` Andrea Merello
@ 2019-09-13 8:43 ` Ardelean, Alexandru
0 siblings, 0 replies; 40+ messages in thread
From: Ardelean, Alexandru @ 2019-09-13 8:43 UTC (permalink / raw)
To: andrea.merello
Cc: Hennerich, Michael, jic23, lars, pmeerw, linux-iio, knaack.h
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;
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 4/4] iio: ad7949: fix channels mixups
2019-09-12 14:43 [PATCH 0/4] Fixes for ad7949 Andrea Merello
` (2 preceding siblings ...)
2019-09-12 14:43 ` [PATCH 3/4] iio: ad7949: fix SPI xfer delays Andrea Merello
@ 2019-09-12 14:43 ` Andrea Merello
2019-09-13 7:19 ` Ardelean, Alexandru
2019-09-13 7:24 ` [PATCH 0/4] Fixes for ad7949 Ardelean, Alexandru
4 siblings, 1 reply; 40+ messages in thread
From: Andrea Merello @ 2019-09-12 14:43 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw
Cc: linux-iio, 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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH 4/4] iio: ad7949: fix channels mixups
2019-09-12 14:43 ` [PATCH 4/4] iio: ad7949: fix channels mixups Andrea Merello
@ 2019-09-13 7:19 ` Ardelean, Alexandru
2019-09-13 8:30 ` Andrea Merello
0 siblings, 1 reply; 40+ messages in thread
From: Ardelean, Alexandru @ 2019-09-13 7:19 UTC (permalink / raw)
To: charles-antoine.couret, andrea.merello, Hennerich, Michael, lars,
jic23, pmeerw, knaack.h
Cc: linux-iio
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);
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 4/4] iio: ad7949: fix channels mixups
2019-09-13 7:19 ` Ardelean, Alexandru
@ 2019-09-13 8:30 ` Andrea Merello
2019-09-13 11:30 ` Couret Charles-Antoine
0 siblings, 1 reply; 40+ messages in thread
From: Andrea Merello @ 2019-09-13 8:30 UTC (permalink / raw)
To: Ardelean, Alexandru
Cc: charles-antoine.couret, Hennerich, Michael, lars, jic23, pmeerw,
knaack.h, linux-iio
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);
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 4/4] iio: ad7949: fix channels mixups
2019-09-13 8:30 ` Andrea Merello
@ 2019-09-13 11:30 ` Couret Charles-Antoine
2019-09-13 11:40 ` Andrea Merello
2019-09-20 7:45 ` Andrea Merello
0 siblings, 2 replies; 40+ messages in thread
From: Couret Charles-Antoine @ 2019-09-13 11:30 UTC (permalink / raw)
To: andrea.merello, Ardelean, Alexandru
Cc: Hennerich, Michael, lars, jic23, pmeerw, knaack.h, linux-iio
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 4/4] iio: ad7949: fix channels mixups
2019-09-13 11:30 ` Couret Charles-Antoine
@ 2019-09-13 11:40 ` Andrea Merello
2019-09-20 7:45 ` Andrea Merello
1 sibling, 0 replies; 40+ messages in thread
From: Andrea Merello @ 2019-09-13 11:40 UTC (permalink / raw)
To: Couret Charles-Antoine
Cc: Ardelean, Alexandru, Hennerich, Michael, lars, jic23, pmeerw,
knaack.h, linux-iio
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
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 4/4] iio: ad7949: fix channels mixups
2019-09-13 11:30 ` Couret Charles-Antoine
2019-09-13 11:40 ` Andrea Merello
@ 2019-09-20 7:45 ` Andrea Merello
2019-09-21 17:12 ` Jonathan Cameron
1 sibling, 1 reply; 40+ messages in thread
From: Andrea Merello @ 2019-09-20 7:45 UTC (permalink / raw)
To: Couret Charles-Antoine
Cc: Ardelean, Alexandru, Hennerich, Michael, lars, jic23, pmeerw,
knaack.h, linux-iio
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)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 4/4] iio: ad7949: fix channels mixups
2019-09-20 7:45 ` Andrea Merello
@ 2019-09-21 17:12 ` Jonathan Cameron
2019-09-23 8:21 ` Andrea Merello
0 siblings, 1 reply; 40+ messages in thread
From: Jonathan Cameron @ 2019-09-21 17:12 UTC (permalink / raw)
To: Andrea Merello
Cc: Couret Charles-Antoine, Ardelean, Alexandru, Hennerich, Michael,
lars, pmeerw, knaack.h, linux-iio
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 4/4] iio: ad7949: fix channels mixups
2019-09-21 17:12 ` Jonathan Cameron
@ 2019-09-23 8:21 ` Andrea Merello
2019-10-05 9:55 ` Jonathan Cameron
0 siblings, 1 reply; 40+ messages in thread
From: Andrea Merello @ 2019-09-23 8:21 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Couret Charles-Antoine, Ardelean, Alexandru, Hennerich, Michael,
lars, pmeerw, knaack.h, linux-iio
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
>
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 4/4] iio: ad7949: fix channels mixups
2019-09-23 8:21 ` Andrea Merello
@ 2019-10-05 9:55 ` Jonathan Cameron
[not found] ` <CAN8YU5PRO5Y5EeEj2SZGm5XfuKSB1rtS7nKdu6wWxXYDOfexqw@mail.gmail.com>
0 siblings, 1 reply; 40+ messages in thread
From: Jonathan Cameron @ 2019-10-05 9:55 UTC (permalink / raw)
To: Andrea Merello
Cc: Couret Charles-Antoine, Ardelean, Alexandru, Hennerich, Michael,
lars, pmeerw, knaack.h, linux-iio
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
> >
> >
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/4] Fixes for ad7949
2019-09-12 14:43 [PATCH 0/4] Fixes for ad7949 Andrea Merello
` (3 preceding siblings ...)
2019-09-12 14:43 ` [PATCH 4/4] iio: ad7949: fix channels mixups Andrea Merello
@ 2019-09-13 7:24 ` Ardelean, Alexandru
2019-09-13 14:00 ` Couret Charles-Antoine
4 siblings, 1 reply; 40+ messages in thread
From: Ardelean, Alexandru @ 2019-09-13 7:24 UTC (permalink / raw)
To: charles-antoine.couret, andrea.merello, Hennerich, Michael, lars,
jic23, pmeerw, knaack.h
Cc: linux-iio
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/4] Fixes for ad7949
2019-09-13 7:24 ` [PATCH 0/4] Fixes for ad7949 Ardelean, Alexandru
@ 2019-09-13 14:00 ` Couret Charles-Antoine
2019-09-15 10:49 ` Jonathan Cameron
2019-09-16 7:34 ` Andrea Merello
0 siblings, 2 replies; 40+ messages in thread
From: Couret Charles-Antoine @ 2019-09-13 14:00 UTC (permalink / raw)
To: Ardelean, Alexandru, andrea.merello, Hennerich, Michael, lars,
jic23, pmeerw, knaack.h
Cc: linux-iio
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
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/4] Fixes for ad7949
2019-09-13 14:00 ` Couret Charles-Antoine
@ 2019-09-15 10:49 ` Jonathan Cameron
2019-09-16 7:39 ` Andrea Merello
2019-09-16 7:34 ` Andrea Merello
1 sibling, 1 reply; 40+ messages in thread
From: Jonathan Cameron @ 2019-09-15 10:49 UTC (permalink / raw)
To: Couret Charles-Antoine
Cc: Ardelean, Alexandru, andrea.merello, Hennerich, Michael, lars,
pmeerw, knaack.h, linux-iio
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
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/4] Fixes for ad7949
2019-09-15 10:49 ` Jonathan Cameron
@ 2019-09-16 7:39 ` Andrea Merello
2019-09-16 7:48 ` Ardelean, Alexandru
0 siblings, 1 reply; 40+ messages in thread
From: Andrea Merello @ 2019-09-16 7:39 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Couret Charles-Antoine, Ardelean, Alexandru, Hennerich, Michael,
lars, pmeerw, knaack.h, linux-iio
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
> >
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/4] Fixes for ad7949
2019-09-16 7:39 ` Andrea Merello
@ 2019-09-16 7:48 ` Ardelean, Alexandru
2019-09-16 7:50 ` Ardelean, Alexandru
0 siblings, 1 reply; 40+ messages in thread
From: Ardelean, Alexandru @ 2019-09-16 7:48 UTC (permalink / raw)
To: andrea.merello, jic23
Cc: charles-antoine.couret, Hennerich, Michael, lars, pmeerw,
linux-iio, knaack.h
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
> > >
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/4] Fixes for ad7949
2019-09-16 7:48 ` Ardelean, Alexandru
@ 2019-09-16 7:50 ` Ardelean, Alexandru
0 siblings, 0 replies; 40+ messages in thread
From: Ardelean, Alexandru @ 2019-09-16 7:50 UTC (permalink / raw)
To: andrea.merello, jic23
Cc: charles-antoine.couret, Hennerich, Michael, lars, pmeerw,
linux-iio, knaack.h
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
> > > >
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/4] Fixes for ad7949
2019-09-13 14:00 ` Couret Charles-Antoine
2019-09-15 10:49 ` Jonathan Cameron
@ 2019-09-16 7:34 ` Andrea Merello
1 sibling, 0 replies; 40+ messages in thread
From: Andrea Merello @ 2019-09-16 7:34 UTC (permalink / raw)
To: Couret Charles-Antoine
Cc: Ardelean, Alexandru, Hennerich, Michael, lars, jic23, pmeerw,
knaack.h, linux-iio
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
>
^ permalink raw reply [flat|nested] 40+ messages in thread