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

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

[Sean Nyekjaer]

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

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,
-- 
2.21.0

[PATCH 2/2] iio: adc: ti-ads8688: fix timestamp is not updated in buffer

[Sean Nyekjaer]

When using the hrtimer iio trigger timestamp isn't updated.
If we use iio_get_time_ns it is updated correctly.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti-ads8688.c b/drivers/iio/adc/ti-ads8688.c
index d9c354dbd7e4..304cad3dddc6 100644
--- a/drivers/iio/adc/ti-ads8688.c
+++ b/drivers/iio/adc/ti-ads8688.c
@@ -396,7 +396,7 @@ static irqreturn_t ads8688_trigger_handler(int irq, void *p)
 	}
 
 	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
-			pf->timestamp);
+			iio_get_time_ns(indio_dev));
 
 	iio_trigger_notify_done(indio_dev->trig);
 
-- 
2.21.0

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

[Jonathan Cameron]

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,

Re: [PATCH 2/2] iio: adc: ti-ads8688: fix timestamp is not updated in buffer

[Jonathan Cameron]

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

> When using the hrtimer iio trigger timestamp isn't updated.
> If we use iio_get_time_ns it is updated correctly.
> 
> Fixes: 2a86487786b5c ("iio: adc: ti-ads8688: add trigger and buffer support")
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
Ah. I assumed this was the normal, with interrupts it'll update, with hrtimer
it won't - but turns out that in this driver it isn't updated in any path as
the top half that sets pf->timestamp isn't being registered.

Not registering that is correct for a device that doesn't have a dataready interrupt
as we want the timestamp as near to the actual read time as possible.

Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ti-ads8688.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ti-ads8688.c b/drivers/iio/adc/ti-ads8688.c
> index d9c354dbd7e4..304cad3dddc6 100644
> --- a/drivers/iio/adc/ti-ads8688.c
> +++ b/drivers/iio/adc/ti-ads8688.c
> @@ -396,7 +396,7 @@ static irqreturn_t ads8688_trigger_handler(int irq, void *p)
>  	}
>  
>  	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
> -			pf->timestamp);
> +			iio_get_time_ns(indio_dev));
>  
>  	iio_trigger_notify_done(indio_dev->trig);
>  

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

[Sean Nyekjaer]

On 11/05/2019 13.44, Jonathan Cameron wrote:
> 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
> 
Thanks for clarifying :-)

/Sean