| Message ID | 1556813672-49861-5-git-send-email-adam.michaelis@rockwellcollins.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/6] iio: ad7949: Support internal Vref | expand |
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; > }
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 }, }; /** @@ -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; }; 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); + + tmp_cfg = ((val & mask) | (ad7949_adc->cfg & ~mask)) & + AD7949_CFG_MASK_TOTAL; + + if (tmp_cfg != ad7949_adc->cfg) { + 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); + ad7949_adc->buffer[1] = (ad7949_adc->cfg & 0x007f) << 2; + + tx.tx_buf = ad7949_adc->buffer; + tx.len = ad7949_message_len(ad7949_adc); + + 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); + } - /* - * 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); 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-bit value in 16-bit buffer */ + *val = *val >> 2; + } return 0; }
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. 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> --- V2: Add some defines to reduce use of magic numbers. --- drivers/iio/adc/ad7949.c | 106 +++++++++++++++++++++++++++-------------------- 1 file changed, 60 insertions(+), 46 deletions(-)