| Message ID | 20190912144310.7458-4-andrea.merello@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for ad7949 | expand |
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;
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;
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;
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;
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(-)