Re: [PATCH v2 5/6] iio: ad7949: Fix SPI interfacing for 14-bit messages

[Jonathan Cameron <jic23@kernel.org>]

On Thu,  2 May 2019 11:14:31 -0500
Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:

> The AD7949 (but not the other two models supported by this driver) uses
> samples 14 bits wide. When attempting to communicate through certain SPI
> controllers that require power-of-2 word widths, this fails. Adding
> logic to pack the 14-bit messages into the most-significant bits of a
> 16-bit message for communicating over byte-based SPI bus.

So this does penalise controllers that do support 14 bits. Can we query
that and perhaps look at only padding if necessary?

> 
> Only able to test with AD7949 part, but should support the 16-bit
> samples of the AD7682 and AD7689, as well.
> 
> Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
This has ended up more complex that I expected.  I 'think' the
result is just to shift the data up two bits if needed to pad?

There are some endian issues introduced as well by this patch.

Jonathan


> ---
> 	V2: Add some defines to reduce use of magic numbers.
> ---
>  drivers/iio/adc/ad7949.c | 106 +++++++++++++++++++++++++++--------------------
>  1 file changed, 60 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index 7820e1097787..cba152151908 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -33,6 +33,11 @@
>  #define AD7949_CFG_READBACK_DIS            1
>  #define AD7949_CFG_READBACK_MASK           GENMASK(0, 0)
>  
> +#define AD7949_BUFFER_LEN 4
> +
> +#define AD7949_HI_RESOLUTION 16
> +#define AD7949_LO_RESOLUTION 14
> +
>  enum {
>  	ID_AD7949 = 0,
>  	ID_AD7682,
> @@ -57,9 +62,9 @@ struct ad7949_adc_spec {
>  };
>  
>  static const struct ad7949_adc_spec ad7949_adc_spec[] = {
> -	[ID_AD7949] = { .num_channels = 8, .resolution = 14 },
> -	[ID_AD7682] = { .num_channels = 4, .resolution = 16 },
> -	[ID_AD7689] = { .num_channels = 8, .resolution = 16 },
> +	[ID_AD7949] = { .num_channels = 8, .resolution = AD7949_LO_RESOLUTION },
> +	[ID_AD7682] = { .num_channels = 4, .resolution = AD7949_HI_RESOLUTION },
> +	[ID_AD7689] = { .num_channels = 8, .resolution = AD7949_HI_RESOLUTION },
This obscures a 'non magic' number. It is better to just leave it as how
many bits it actually is.

>  };
>  
>  /**
> @@ -85,7 +90,7 @@ struct ad7949_adc_chip {
>  	u8 resolution;
>  	u16 cfg;
>  	unsigned int current_channel;
> -	u32 buffer ____cacheline_aligned;
> +	u8 buffer[AD7949_BUFFER_LEN] ____cacheline_aligned;
Why this change?  Ah, not using bits_per_word any more..

>  };
>  
>  static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
> @@ -96,41 +101,51 @@ static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
>  	return false;
>  }
>  
> -static int ad7949_spi_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
> +static int ad7949_message_len(struct ad7949_adc_chip *ad7949_adc)
>  {
> -	int ret = ad7949_adc->resolution;
> +	int tx_len = 2;
>  
>  	if (ad7949_spi_cfg_is_read_back(ad7949_adc))
> -		ret += AD7949_CFG_REG_SIZE_BITS;
> +		tx_len += 2;
>  
> -	return ret;
> +	return tx_len;
>  }
>  
>  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 shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
> +	int ret = 0;
>  	struct spi_message msg;
> -	struct spi_transfer tx[] = {
> -		{
> -			.tx_buf = &ad7949_adc->buffer,
> -			.len = 4,
> -			.bits_per_word = bits_per_word,
> -		},
> -	};
> -
> -	ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
> -	ad7949_adc->buffer = (ad7949_adc->cfg & AD7949_CFG_MASK_TOTAL) << shift;
> -	spi_message_init_with_transfers(&msg, tx, 1);
> -	ret = spi_sync(ad7949_adc->spi, &msg);
> +	struct spi_transfer tx;
> +	u16 tmp_cfg = 0;
> +
> +	(void)memset(&tx, 0, sizeof(tx));
> +	(void)memset(ad7949_adc->buffer, 0, AD7949_BUFFER_LEN);
The void casts do nothing useful.

memset(ad7949_adc->spi, sizeof(ad7949_adc->spi)) is better.

however, I'm not clear why you can't use a similar c99 assignment to that
the driver previously did.
> +
> +	tmp_cfg = ((val & mask) | (ad7949_adc->cfg & ~mask)) &
> +		AD7949_CFG_MASK_TOTAL;
> +
> +	if (tmp_cfg != ad7949_adc->cfg) {
Flip the logic here and return early if it hasn't changed.

> +		ad7949_adc->cfg = tmp_cfg;
> +
> +		/* Pack 14-bit value into 2 bytes, MSB first */
> +		ad7949_adc->buffer[0] = ((ad7949_adc->cfg & 0x7f00) >> 6) |
> +			((ad7949_adc->cfg & 0xc0) >> 6);
So this:
takes bits 13..8 and shifts them down to 7..2.
takes bits 7..6 and shifts them down to 1..0

Unless I'm missing something that's just 13..6 shifted down to 7..0
(ad7949_adc->cfg & GENMASK(13, 6)) >> 6 or something like that.

> +		ad7949_adc->buffer[1] = (ad7949_adc->cfg & 0x007f) << 2;

Use GENMASK here as well.

This looks like a shift up by 2 followed by a endian swap. Can we make
that explicit as it'll be more readable...

I'm a little worried that we have dropped the endian swap that was
effectively occuring due to the larger bits_per_word for a version
that always does it, whether or not we are on a big endian platform
or a little endian one.

From the spi.h header

 * 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.
 *

As you have it the bits_per_word is 8 and so there is no inbuilt assumption
of ordering and you need you need to swap as you are on a little endian
platform.

> +
> +		tx.tx_buf = ad7949_adc->buffer;
> +		tx.len = ad7949_message_len(ad7949_adc);
> +
> +		spi_message_init_with_transfers(&msg, &tx, 1);

spi_write?

> +		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);
> +	}
>  
> -	/*
> -	 * This delay is to avoid a new request before the required time to
> -	 * send a new command to the device
> -	 */
> -	udelay(2);
>  	return ret;
>  }
>  
> @@ -138,16 +153,10 @@ 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 mask = GENMASK(ad7949_adc->resolution, 0);
>  	struct spi_message msg;
> -	struct spi_transfer tx[] = {
> -		{
> -			.rx_buf = &ad7949_adc->buffer,
> -			.len = 4,
> -			.bits_per_word = bits_per_word,
> -		},
> -	};
> +	struct spi_transfer tx;
> +
> +	ad7949_adc->current_channel = channel;
>  
>  	ret = ad7949_spi_write_cfg(ad7949_adc,
>  				   channel << AD7949_CFG_CHAN_SEL_SHIFT,
> @@ -155,24 +164,29 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
>  	if (ret)
>  		return ret;
>  
> -	ad7949_adc->buffer = 0;
> -	spi_message_init_with_transfers(&msg, tx, 1);
> +	(void)memset(&tx, 0, sizeof(tx));

> +	(void)memset(ad7949_adc->buffer, 0, AD7949_BUFFER_LEN);
> +
> +	tx.rx_buf = ad7949_adc->buffer;
> +	tx.len = ad7949_message_len(ad7949_adc);
> +
> +	spi_message_init_with_transfers(&msg, &tx, 1);
spi_read?

>  	ret = spi_sync(ad7949_adc->spi, &msg);
>  	if (ret)
>  		return ret;
>  
>  	/*
> -	 * This delay is to avoid a new request before the required time to
> -	 * send a new command to the device
> +	 * 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[0] << 8) | ad7949_adc->buffer[1];
>  
> -	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;
> +	if (ad7949_adc->resolution == AD7949_LO_RESOLUTION) {

14, much more readable as obvious what is happening.

> +		/* 14-bit value in 16-bit buffer */
> +		*val = *val >> 2;
> +	}
>  
>  	return 0;
>  }