[v3,1/5] iio: ad7949: Fix dummy read cycle placement
diff mbox series

Message ID 1557759185-167857-1-git-send-email-adam.michaelis@rockwellcollins.com
State New
Headers show
Series
  • [v3,1/5] iio: ad7949: Fix dummy read cycle placement
Related show

Commit Message

Adam Michaelis May 13, 2019, 2:53 p.m. UTC
The AD7949 requires two conversion cycles following the first
configuration change, and one extra cycle following any other
configuration change (including changing the analog channel being
sampled). Therefore, adding a dummy read cycle when config is changed
and removing the extra cycle at initial configuration (the first dummy
cycle is now performed as part of applying the configuration change).

Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
	V2:
	- Add some defines to reduce use of magic numbers.
	V3:
	- Switch back to using a u32 data buffer.
	- Add-back the second dummy cycle on initialization.
	- Move to first patch in series.
---
 drivers/iio/adc/ad7949.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

Comments

Alexandru Ardelean May 23, 2019, 11:47 a.m. UTC | #1
On Mon, May 13, 2019 at 7:18 PM Adam Michaelis
<adam.michaelis@rockwellcollins.com> wrote:
>
> The AD7949 requires two conversion cycles following the first
> configuration change, and one extra cycle following any other
> configuration change (including changing the analog channel being
> sampled). Therefore, adding a dummy read cycle when config is changed
> and removing the extra cycle at initial configuration (the first dummy
> cycle is now performed as part of applying the configuration change).
>

CC-ing my work email.
And Stefan as well.

We'll have to try this driver & changes internally a bit.
Hopefully we have a compatible device around the office.
Since it wasn't written by us, we're also unsure about things just by
looking at the code.

> Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> ---
>         V2:
>         - Add some defines to reduce use of magic numbers.
>         V3:
>         - Switch back to using a u32 data buffer.
>         - Add-back the second dummy cycle on initialization.
>         - Move to first patch in series.
> ---
>  drivers/iio/adc/ad7949.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index ac0ffff6c5ae..c7fe27aa2519 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -100,6 +100,23 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
>          * send a new command to the device
>          */
>         udelay(2);
> +
> +       /*
> +        * Perform extra read cycle to allow configuration, acquisition,
> +        * and conversion sequences to complete for new configuration.
> +        */
> +       ad7949_adc->buffer = 0;
> +
> +       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);
> +

Is this needed if the channel doesn't change ?
If it isn't, maybe add a check in ad7949_spi_read_channel() to skip
the call to ad7949_spi_write_cfg().

We should also take performance into account when doing SPI
transactions to the device, and if we can skip some of them, all the
better.
This change, introduces a performance penalty by doing this extra read + udelay.

>         return ret;
>  }
>
> @@ -229,11 +246,10 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
>         ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
>
>         /*
> -        * Do two dummy conversions to apply the first configuration setting.
> +        * Do a dummy conversion to apply the first configuration setting.
>          * Required only after the start up of the device.
>          */
>         ad7949_spi_read_channel(ad7949_adc, &val, ad7949_adc->current_channel);
> -       ad7949_spi_read_channel(ad7949_adc, &val, ad7949_adc->current_channel);

The datasheet mentions that 2 dummy conversions are needed on power-up / init.

The way, this was done here was a bit easier to follow (or
straightforward) with the datasheet.

This isn't to say that this is bad, but if we need to do an extra SPI
read (and skip the SPI write part), then I would just add an SPI read
function, instead of moving it completely into ad7949_spi_write_cfg(),
which would then compact things in ad7949_spi_read_channel().

>
>         return ret;
>  }
> --
> 1.9.1
>
Couret Charles-Antoine May 24, 2019, 11:49 a.m. UTC | #2
Le 23/05/2019 à 13:47, Alexandru Ardelean a écrit :
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
>> index ac0ffff6c5ae..c7fe27aa2519 100644
>> --- a/drivers/iio/adc/ad7949.c
>> +++ b/drivers/iio/adc/ad7949.c
>> @@ -100,6 +100,23 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
>>           * send a new command to the device
>>           */
>>          udelay(2);
>> +
>> +       /*
>> +        * Perform extra read cycle to allow configuration, acquisition,
>> +        * and conversion sequences to complete for new configuration.
>> +        */
>> +       ad7949_adc->buffer = 0;
>> +
>> +       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);
>> +
> Is this needed if the channel doesn't change ?
> If it isn't, maybe add a check in ad7949_spi_read_channel() to skip
> the call to ad7949_spi_write_cfg().
>
> We should also take performance into account when doing SPI
> transactions to the device, and if we can skip some of them, all the
> better.
> This change, introduces a performance penalty by doing this extra read + udelay.

I wrote the initial driver and you're right. It is not required in the 
situation where the channel is not changed.

In our product the channel changed every time so we didn't think about 
this kind of optimisations. It is relevant to implement them.

>>          return ret;
>>   }
>>
>> @@ -229,11 +246,10 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
>>          ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
>>
>>          /*
>> -        * Do two dummy conversions to apply the first configuration setting.
>> +        * Do a dummy conversion to apply the first configuration setting.
>>           * Required only after the start up of the device.
>>           */
>>          ad7949_spi_read_channel(ad7949_adc, &val, ad7949_adc->current_channel);
>> -       ad7949_spi_read_channel(ad7949_adc, &val, ad7949_adc->current_channel);
> The datasheet mentions that 2 dummy conversions are needed on power-up / init.
>
> The way, this was done here was a bit easier to follow (or
> straightforward) with the datasheet.
>
> This isn't to say that this is bad, but if we need to do an extra SPI
> read (and skip the SPI write part), then I would just add an SPI read
> function, instead of moving it completely into ad7949_spi_write_cfg(),
> which would then compact things in ad7949_spi_read_channel().

+1. It is more readable from my point of view to have two explicit 
commands for this step.

I agree with your solution.


Regards,

Charles-Antoine Couret

Patch
diff mbox series

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index ac0ffff6c5ae..c7fe27aa2519 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -100,6 +100,23 @@  static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
 	 * send a new command to the device
 	 */
 	udelay(2);
+
+	/*
+	 * Perform extra read cycle to allow configuration, acquisition,
+	 * and conversion sequences to complete for new configuration.
+	 */
+	ad7949_adc->buffer = 0;
+
+	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;
 }
 
@@ -229,11 +246,10 @@  static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 	ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
 
 	/*
-	 * Do two dummy conversions to apply the first configuration setting.
+	 * Do a dummy conversion to apply the first configuration setting.
 	 * Required only after the start up of the device.
 	 */
 	ad7949_spi_read_channel(ad7949_adc, &val, ad7949_adc->current_channel);
-	ad7949_spi_read_channel(ad7949_adc, &val, ad7949_adc->current_channel);
 
 	return ret;
 }