Re: [PATCH 1/2] iio: adc: ti-ads8688: save values correct in buffer

[Jonathan Cameron <jic23@kernel.org>]

On Tue,  7 May 2019 10:23:03 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> Fill the read values in the buffer so we comply
> with the given index values.
Why?

They are not supposed to always remain in the same place. If some channels
are turned off then they will shift 'down'.  That has to happen in general
to allow us to do more efficient packing when they happen to fit into a
smaller power of 2 size.
So if channels 0-3 (8 bits each) are enabled and timestamp, it should be

0 1 2 3 X X X X Timestamp, 0 1 2 3 X X X X Timestamp..

If no timestamp it should fall back to the packing
0 1 2 3, 0 1 2 3

If only channels 0, 1 and 3 we should get

0 1 3 X, 0 1 3 X (padding to 32 bits)

For only 0 and 3
0 3, 0 3, 0 3

For only 0
0, 0, 0, 0, 0, 0 

Jonathan

> 
> Fixes: 2a86487786b5c ("iio: adc: ti-ads8688: add trigger and buffer support")
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  drivers/iio/adc/ti-ads8688.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads8688.c b/drivers/iio/adc/ti-ads8688.c
> index f9461070a74a..d9c354dbd7e4 100644
> --- a/drivers/iio/adc/ti-ads8688.c
> +++ b/drivers/iio/adc/ti-ads8688.c
> @@ -387,13 +387,12 @@ static irqreturn_t ads8688_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	u16 buffer[ADS8688_MAX_CHANNELS + sizeof(s64)/sizeof(u16)];
> -	int i, j = 0;
> +	int bit;
>  
> -	for (i = 0; i < indio_dev->masklength; i++) {
> -		if (!test_bit(i, indio_dev->active_scan_mask))
> -			continue;
> -		buffer[j] = ads8688_read(indio_dev, i);
> -		j++;
> +	memset(buffer, 0, sizeof(buffer));
> +
> +	for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength) {
> +		buffer[bit] = ads8688_read(indio_dev, bit);
>  	}
>  
>  	iio_push_to_buffers_with_timestamp(indio_dev, buffer,