| Message ID | 930560a0d0ca7af597d4ad901422bc9ba3fc6a79.1742394806.git.marcelo.schmitt@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad4000: Add SPI offload support | expand |
On 3/19/25 9:57 AM, Marcelo Schmitt wrote: > When SPI `bits_per_word` is not set, SPI transfers default 8-bit word size > and ADC data gets stored in big-endian format in memory. Because of that, > the IIO driver requests ADC data to be rearranged from BE to CPU > endianness. However, with `bits_per_word` set to the number of ADC > precision bits, transfers use larger word sizes that get stored in > 'in-memory wordsizes' and can be read in CPU endianness. > > Use proper `bits_per_word` size for SPI transfers thus saving the driver > from requesting endianness conversions. With that, shifting the buffer > data is also no longer needed. This change has no impact on IIO device > functionality. > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > --- This is a breaking change. Some SPI controllers, like RPi can only do 8-bit transfers, so this driver would stop working on those platforms. Also, if anyone made software already that depended on the big-endian ordering without checking the scan_type attribute, it would break that software. I would leave this as-is (drop this patch) and just make it: .endianness = _offl ? IIO_CPU : IIO_BE, in the next patch.
On 03/20, David Lechner wrote: > On 3/19/25 9:57 AM, Marcelo Schmitt wrote: > > When SPI `bits_per_word` is not set, SPI transfers default 8-bit word size > > and ADC data gets stored in big-endian format in memory. Because of that, > > the IIO driver requests ADC data to be rearranged from BE to CPU > > endianness. However, with `bits_per_word` set to the number of ADC > > precision bits, transfers use larger word sizes that get stored in > > 'in-memory wordsizes' and can be read in CPU endianness. > > > > Use proper `bits_per_word` size for SPI transfers thus saving the driver > > from requesting endianness conversions. With that, shifting the buffer > > data is also no longer needed. This change has no impact on IIO device > > functionality. > > > > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > > --- > > This is a breaking change. Some SPI controllers, like RPi can only do 8-bit > transfers, so this driver would stop working on those platforms. Also, if > anyone made software already that depended on the big-endian ordering without > checking the scan_type attribute, it would break that software. Hmm, if user space software doesn't check deviceX/scan_elements/in_<channel_type>_type, nor deviceX/bufferY/in_<channel_type>Y_type, then I'd say that's already somewhat broken since at least buffer scan_element type attributes are documented https://web.git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/ABI/testing/sysfs-bus-iio?h=testing#n1470. Anyway, this patch it indeed broke RPi support so I'll revert buffer endianness and SPI transfer bits_per_word configurations the way it was in v1. Adding possible failure due to controller not supporting specific bits_per_word size to my list of peculiarities related to that field. > > I would leave this as-is (drop this patch) and just make it: > > .endianness = _offl ? IIO_CPU : IIO_BE, > > in the next patch.
diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c index 4fe8dee48da9..19bc021e1b5d 100644 --- a/drivers/iio/adc/ad4000.c +++ b/drivers/iio/adc/ad4000.c @@ -50,8 +50,7 @@ .sign = _sign, \ .realbits = _real_bits, \ .storagebits = _storage_bits, \ - .shift = _storage_bits - _real_bits, \ - .endianness = IIO_BE, \ + .endianness = IIO_CPU, \ }, \ } @@ -79,8 +78,7 @@ .sign = _sign, \ .realbits = _real_bits, \ .storagebits = _storage_bits, \ - .shift = _storage_bits - _real_bits, \ - .endianness = IIO_BE, \ + .endianness = IIO_CPU, \ }, \ } @@ -411,8 +409,8 @@ struct ad4000_state { */ struct { union { - __be16 sample_buf16; - __be32 sample_buf32; + u16 sample_buf16; + u32 sample_buf32; } data; aligned_s64 timestamp; } scan __aligned(IIO_DMA_MINALIGN); @@ -516,11 +514,9 @@ static int ad4000_single_conversion(struct iio_dev *indio_dev, return ret; if (chan->scan_type.storagebits > 16) - sample = be32_to_cpu(st->scan.data.sample_buf32); + sample = st->scan.data.sample_buf32; else - sample = be16_to_cpu(st->scan.data.sample_buf16); - - sample >>= chan->scan_type.shift; + sample = st->scan.data.sample_buf16; if (chan->scan_type.sign == 's') *val = sign_extend32(sample, chan->scan_type.realbits - 1); @@ -689,6 +685,7 @@ static int ad4000_prepare_3wire_mode_message(struct ad4000_state *st, xfers[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS; xfers[1].rx_buf = &st->scan.data; + xfers[1].bits_per_word = chan->scan_type.realbits; xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits); xfers[1].delay.value = st->time_spec->t_quiet2_ns; xfers[1].delay.unit = SPI_DELAY_UNIT_NSECS; @@ -719,6 +716,7 @@ static int ad4000_prepare_4wire_mode_message(struct ad4000_state *st, xfers[0].delay.unit = SPI_DELAY_UNIT_NSECS; xfers[1].rx_buf = &st->scan.data; + xfers[1].bits_per_word = chan->scan_type.realbits; xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits); spi_message_init_with_transfers(&st->msg, st->xfers, 2);
When SPI `bits_per_word` is not set, SPI transfers default 8-bit word size and ADC data gets stored in big-endian format in memory. Because of that, the IIO driver requests ADC data to be rearranged from BE to CPU endianness. However, with `bits_per_word` set to the number of ADC precision bits, transfers use larger word sizes that get stored in 'in-memory wordsizes' and can be read in CPU endianness. Use proper `bits_per_word` size for SPI transfers thus saving the driver from requesting endianness conversions. With that, shifting the buffer data is also no longer needed. This change has no impact on IIO device functionality. Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> --- drivers/iio/adc/ad4000.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)