Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] iio: adc: ti-ads8688: save values correct in buffer
@ 2019-05-07  8:23 Sean Nyekjaer
  2019-05-07  8:23 ` [PATCH 2/2] iio: adc: ti-ads8688: fix timestamp is not updated " Sean Nyekjaer
  2019-05-11 11:44 ` [PATCH 1/2] iio: adc: ti-ads8688: save values correct " Jonathan Cameron
  0 siblings, 2 replies; 5+ messages in thread
From: Sean Nyekjaer @ 2019-05-07  8:23 UTC (permalink / raw)
  To: linux-iio, jic23; +Cc: 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/2] iio: adc: ti-ads8688: fix timestamp is not updated in buffer
  2019-05-07  8:23 [PATCH 1/2] iio: adc: ti-ads8688: save values correct in buffer Sean Nyekjaer
@ 2019-05-07  8:23 ` " Sean Nyekjaer
  2019-05-11 11:50   ` Jonathan Cameron
  2019-05-11 11:44 ` [PATCH 1/2] iio: adc: ti-ads8688: save values correct " Jonathan Cameron
  1 sibling, 1 reply; 5+ messages in thread
From: Sean Nyekjaer @ 2019-05-07  8:23 UTC (permalink / raw)
  To: linux-iio, jic23; +Cc: 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] iio: adc: ti-ads8688: save values correct in buffer
  2019-05-07  8:23 [PATCH 1/2] iio: adc: ti-ads8688: save values correct in buffer Sean Nyekjaer
  2019-05-07  8:23 ` [PATCH 2/2] iio: adc: ti-ads8688: fix timestamp is not updated " Sean Nyekjaer
@ 2019-05-11 11:44 ` " Jonathan Cameron
  2019-05-11 12:03   ` Sean Nyekjaer
  1 sibling, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2019-05-11 11:44 UTC (permalink / raw)
  To: Sean Nyekjaer; +Cc: linux-iio

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,


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] iio: adc: ti-ads8688: fix timestamp is not updated in buffer
  2019-05-07  8:23 ` [PATCH 2/2] iio: adc: ti-ads8688: fix timestamp is not updated " Sean Nyekjaer
@ 2019-05-11 11:50   ` Jonathan Cameron
  0 siblings, 0 replies; 5+ messages in thread
From: Jonathan Cameron @ 2019-05-11 11:50 UTC (permalink / raw)
  To: Sean Nyekjaer; +Cc: linux-iio

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);
>  


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] iio: adc: ti-ads8688: save values correct in buffer
  2019-05-11 11:44 ` [PATCH 1/2] iio: adc: ti-ads8688: save values correct " Jonathan Cameron
@ 2019-05-11 12:03   ` Sean Nyekjaer
  0 siblings, 0 replies; 5+ messages in thread
From: Sean Nyekjaer @ 2019-05-11 12:03 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: linux-iio



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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-07  8:23 [PATCH 1/2] iio: adc: ti-ads8688: save values correct in buffer Sean Nyekjaer
2019-05-07  8:23 ` [PATCH 2/2] iio: adc: ti-ads8688: fix timestamp is not updated " Sean Nyekjaer
2019-05-11 11:50   ` Jonathan Cameron
2019-05-11 11:44 ` [PATCH 1/2] iio: adc: ti-ads8688: save values correct " Jonathan Cameron
2019-05-11 12:03   ` Sean Nyekjaer

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org linux-iio@archiver.kernel.org
	public-inbox-index linux-iio


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/ public-inbox