Linux-IIO Archive on lore.kernel.org
 help / Atom feed
* [PATCH 1/2] iio: imu: st_lsm6dsx: only set available_scan_masks if using device fifo
@ 2019-05-07  8:02 Sean Nyekjaer
  2019-05-07  8:02 ` [PATCH 2/2] iio: imu: st_lsm6dsx: add iio trigger and buffer support Sean Nyekjaer
  0 siblings, 1 reply; 9+ messages in thread
From: Sean Nyekjaer @ 2019-05-07  8:02 UTC (permalink / raw)
  To: linux-iio, jic23; +Cc: Sean Nyekjaer, martin

We only need available scan_masks set if we are going to use the
onboard fifo. Unfortunately it requires the IRQ pins to be connected
to the SoC.
The next patch in this series add support for iio triggered buffer
for use with iio triggers.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index cf82c9049945..1ca69598678f 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1007,7 +1007,6 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
 
 	iio_dev->modes = INDIO_DIRECT_MODE;
 	iio_dev->dev.parent = hw->dev;
-	iio_dev->available_scan_masks = st_lsm6dsx_available_scan_masks;
 
 	sensor = iio_priv(iio_dev);
 	sensor->id = id;
@@ -1074,6 +1073,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
 		hw->iio_devs[i] = st_lsm6dsx_alloc_iiodev(hw, i, name);
 		if (!hw->iio_devs[i])
 			return -ENOMEM;
+
+		hw->iio_devs[i]->available_scan_masks =
+				 st_lsm6dsx_available_scan_masks;
 	}
 
 	err = st_lsm6dsx_init_device(hw);
-- 
2.21.0


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

* [PATCH 2/2] iio: imu: st_lsm6dsx: add iio trigger and buffer support
  2019-05-07  8:02 [PATCH 1/2] iio: imu: st_lsm6dsx: only set available_scan_masks if using device fifo Sean Nyekjaer
@ 2019-05-07  8:02 ` Sean Nyekjaer
  2019-05-11 11:37   ` Jonathan Cameron
  0 siblings, 1 reply; 9+ messages in thread
From: Sean Nyekjaer @ 2019-05-07  8:02 UTC (permalink / raw)
  To: linux-iio, jic23; +Cc: Sean Nyekjaer, martin

This adds support for using iio triggers, this is needed because
our hardware guys forgot to connect the irq pins from imu device
to the SoC.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 37 ++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 1ca69598678f..65ab853202de 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -40,8 +40,11 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/delay.h>
+#include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 #include <linux/pm.h>
 #include <linux/regmap.h>
 #include <linux/bitfield.h>
@@ -945,6 +948,30 @@ static int st_lsm6dsx_init_hw_timer(struct st_lsm6dsx_hw *hw)
 	return 0;
 }
 
+static irqreturn_t st_lsm6dsx_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct st_lsm6dsx_sensor *sensor = iio_priv(indio_dev);
+	u16 buffer[4 * sizeof(s64)/sizeof(u16)];
+	int tmp, bit;
+
+	memset(buffer, 0, sizeof(buffer));
+
+	for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength) {
+		st_lsm6dsx_read_oneshot(sensor,
+					indio_dev->channels[bit].address, &tmp);
+		buffer[bit] = tmp;
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
+					   iio_get_time_ns(indio_dev));
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
 static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
 {
 	u8 drdy_int_reg;
@@ -1093,6 +1120,16 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
 		err = st_lsm6dsx_fifo_setup(hw);
 		if (err < 0)
 			return err;
+	} else {
+		for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
+			err = devm_iio_triggered_buffer_setup(hw->dev,
+					hw->iio_devs[i], NULL,
+					st_lsm6dsx_trigger_handler, NULL);
+			if (err < 0) {
+				dev_err(hw->dev, "iio triggered buffer setup failed\n");
+				return err;
+			}
+		}
 	}
 
 	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
-- 
2.21.0


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

* Re: [PATCH 2/2] iio: imu: st_lsm6dsx: add iio trigger and buffer support
  2019-05-07  8:02 ` [PATCH 2/2] iio: imu: st_lsm6dsx: add iio trigger and buffer support Sean Nyekjaer
@ 2019-05-11 11:37   ` Jonathan Cameron
  2019-05-11 12:30     ` Lorenzo Bianconi
  2019-05-11 12:38     ` Sean Nyekjaer
  0 siblings, 2 replies; 9+ messages in thread
From: Jonathan Cameron @ 2019-05-11 11:37 UTC (permalink / raw)
  To: Sean Nyekjaer; +Cc: linux-iio, martin, Lorenzo Bianconi, Denis CIOCCA

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

> This adds support for using iio triggers, this is needed because
> our hardware guys forgot to connect the irq pins from imu device
> to the SoC.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
Hi Sean

A small question on the size of the buffer needed inline.  Otherwise looks
good to me.
> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 37 ++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 1ca69598678f..65ab853202de 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -40,8 +40,11 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/delay.h>
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  #include <linux/pm.h>
>  #include <linux/regmap.h>
>  #include <linux/bitfield.h>
> @@ -945,6 +948,30 @@ static int st_lsm6dsx_init_hw_timer(struct st_lsm6dsx_hw *hw)
>  	return 0;
>  }
>  
> +static irqreturn_t st_lsm6dsx_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct st_lsm6dsx_sensor *sensor = iio_priv(indio_dev);
> +	u16 buffer[4 * sizeof(s64)/sizeof(u16)];
The size may well be correct, but that particular way of expressing it doesn't make
it terribly clear.

It seems a bit large... You need the space for the timestamp, so it needs to be
padded to a u64, but that would still require your other channels to need
more than 2*sizeof(s64)/sizeof(u16) u16s which is 9 or more of them.

> +	int tmp, bit;
> +
> +	memset(buffer, 0, sizeof(buffer));
> +
> +	for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength) {
> +		st_lsm6dsx_read_oneshot(sensor,
> +					indio_dev->channels[bit].address, &tmp);
> +		buffer[bit] = tmp;
> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
> +					   iio_get_time_ns(indio_dev));
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
>  {
>  	u8 drdy_int_reg;
> @@ -1093,6 +1120,16 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
>  		err = st_lsm6dsx_fifo_setup(hw);
>  		if (err < 0)
>  			return err;
> +	} else {
> +		for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> +			err = devm_iio_triggered_buffer_setup(hw->dev,
> +					hw->iio_devs[i], NULL,
> +					st_lsm6dsx_trigger_handler, NULL);
> +			if (err < 0) {
> +				dev_err(hw->dev, "iio triggered buffer setup failed\n");
> +				return err;
> +			}
> +		}
>  	}
>  
>  	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {


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

* Re: [PATCH 2/2] iio: imu: st_lsm6dsx: add iio trigger and buffer support
  2019-05-11 11:37   ` Jonathan Cameron
@ 2019-05-11 12:30     ` Lorenzo Bianconi
  2019-05-11 12:54       ` Sean Nyekjaer
  2019-05-11 12:38     ` Sean Nyekjaer
  1 sibling, 1 reply; 9+ messages in thread
From: Lorenzo Bianconi @ 2019-05-11 12:30 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Sean Nyekjaer, linux-iio, martin, Lorenzo Bianconi, Denis CIOCCA

[-- Attachment #1: Type: text/plain, Size: 3969 bytes --]

> On Tue,  7 May 2019 10:02:25 +0200
> Sean Nyekjaer <sean@geanix.com> wrote:
> 
> > This adds support for using iio triggers, this is needed because
> > our hardware guys forgot to connect the irq pins from imu device
> > to the SoC.
> > 
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> Hi Sean
> 
> A small question on the size of the buffer needed inline.  Otherwise looks
> good to me.

Hi Sean,

this patch does not make sense to me since running st_lsm6dsx_read_oneshot
you need to wait to power up the device (and you will power it down at the
end). I guess you will not be able to read at a given ODR (e.g. 416Hz).
Moreover you can't read from the hw fifo without the irq line since
you need to read a full pattern from it in order to maintain the alignment.
From my point of view you have 2 possibility:
- poll the output registers from userspace (this is what you are actually
  doing from inside the kernel, what is the advantage of doing so?)
- fix the hw bug

Moreover if I read the patch correctly it has a NULL pointer dereference bug
since hw->iio_devs[i] can be NULL (e.g. if sensor hub is disabled)

Regards,
Lorenzo

> > ---
> >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 37 ++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> > 
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > index 1ca69598678f..65ab853202de 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > @@ -40,8 +40,11 @@
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/delay.h>
> > +#include <linux/iio/buffer.h>
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> >  #include <linux/pm.h>
> >  #include <linux/regmap.h>
> >  #include <linux/bitfield.h>
> > @@ -945,6 +948,30 @@ static int st_lsm6dsx_init_hw_timer(struct st_lsm6dsx_hw *hw)
> >  	return 0;
> >  }
> >  
> > +static irqreturn_t st_lsm6dsx_trigger_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct st_lsm6dsx_sensor *sensor = iio_priv(indio_dev);
> > +	u16 buffer[4 * sizeof(s64)/sizeof(u16)];
> The size may well be correct, but that particular way of expressing it doesn't make
> it terribly clear.
> 
> It seems a bit large... You need the space for the timestamp, so it needs to be
> padded to a u64, but that would still require your other channels to need
> more than 2*sizeof(s64)/sizeof(u16) u16s which is 9 or more of them.
> 
> > +	int tmp, bit;
> > +
> > +	memset(buffer, 0, sizeof(buffer));
> > +
> > +	for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength) {
> > +		st_lsm6dsx_read_oneshot(sensor,
> > +					indio_dev->channels[bit].address, &tmp);
> > +		buffer[bit] = tmp;
> > +	}
> > +
> > +	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
> > +					   iio_get_time_ns(indio_dev));
> > +
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
> >  {
> >  	u8 drdy_int_reg;
> > @@ -1093,6 +1120,16 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
> >  		err = st_lsm6dsx_fifo_setup(hw);
> >  		if (err < 0)
> >  			return err;
> > +	} else {
> > +		for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > +			err = devm_iio_triggered_buffer_setup(hw->dev,
> > +					hw->iio_devs[i], NULL,
> > +					st_lsm6dsx_trigger_handler, NULL);

I guess hw->iio_devs[i] can be NULL here

> > +			if (err < 0) {
> > +				dev_err(hw->dev, "iio triggered buffer setup failed\n");
> > +				return err;
> > +			}
> > +		}
> >  	}
> >  
> >  	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

* Re: [PATCH 2/2] iio: imu: st_lsm6dsx: add iio trigger and buffer support
  2019-05-11 11:37   ` Jonathan Cameron
  2019-05-11 12:30     ` Lorenzo Bianconi
@ 2019-05-11 12:38     ` Sean Nyekjaer
  1 sibling, 0 replies; 9+ messages in thread
From: Sean Nyekjaer @ 2019-05-11 12:38 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: linux-iio, martin, Lorenzo Bianconi, Denis CIOCCA



On 11/05/2019 13.37, Jonathan Cameron wrote:
> On Tue,  7 May 2019 10:02:25 +0200
> Sean Nyekjaer <sean@geanix.com> wrote:
> 
>> This adds support for using iio triggers, this is needed because
>> our hardware guys forgot to connect the irq pins from imu device
>> to the SoC.
>>
>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> Hi Sean
> 
> A small question on the size of the buffer needed inline.  Otherwise looks
> good to me.
>> ---
>>   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 37 ++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> index 1ca69598678f..65ab853202de 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> @@ -40,8 +40,11 @@
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>>   #include <linux/delay.h>
>> +#include <linux/iio/buffer.h>
>>   #include <linux/iio/iio.h>
>>   #include <linux/iio/sysfs.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>>   #include <linux/pm.h>
>>   #include <linux/regmap.h>
>>   #include <linux/bitfield.h>
>> @@ -945,6 +948,30 @@ static int st_lsm6dsx_init_hw_timer(struct st_lsm6dsx_hw *hw)
>>   	return 0;
>>   }
>>   
>> +static irqreturn_t st_lsm6dsx_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct st_lsm6dsx_sensor *sensor = iio_priv(indio_dev);
>> +	u16 buffer[4 * sizeof(s64)/sizeof(u16)];
> The size may well be correct, but that particular way of expressing it doesn't make
> it terribly clear.
> 
> It seems a bit large... You need the space for the timestamp, so it needs to be
> padded to a u64, but that would still require your other channels to need
> more than 2*sizeof(s64)/sizeof(u16) u16s which is 9 or more of them.
Right now we have u16[16], so that quite at bit large :-)

So maybe something like:
/* 3x s16 + 1x s16(padding) + 1x s64 timestamp */
u16 buffer[2 * sizeof(s64)/sizeof(u16)];
Would do it?

> 
>> +	int tmp, bit;
>> +
>> +	memset(buffer, 0, sizeof(buffer));
>> +
>> +	for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength) {
>> +		st_lsm6dsx_read_oneshot(sensor,
>> +					indio_dev->channels[bit].address, &tmp);
>> +		buffer[bit] = tmp;
>> +	}
As you pointed out in the
"[PATCH 1/2] iio: adc: ti-ads8688: save values correct in buffer"
When taking this approach it will not shift down.
>> +
>> +	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
>> +					   iio_get_time_ns(indio_dev));
>> +
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>>   static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
>>   {
>>   	u8 drdy_int_reg;
>> @@ -1093,6 +1120,16 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
>>   		err = st_lsm6dsx_fifo_setup(hw);
>>   		if (err < 0)
>>   			return err;
>> +	} else {
>> +		for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
>> +			err = devm_iio_triggered_buffer_setup(hw->dev,
>> +					hw->iio_devs[i], NULL,
>> +					st_lsm6dsx_trigger_handler, NULL);
>> +			if (err < 0) {
>> +				dev_err(hw->dev, "iio triggered buffer setup failed\n");
>> +				return err;
>> +			}
>> +		}
>>   	}
>>   
>>   	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> 

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

* Re: [PATCH 2/2] iio: imu: st_lsm6dsx: add iio trigger and buffer support
  2019-05-11 12:30     ` Lorenzo Bianconi
@ 2019-05-11 12:54       ` Sean Nyekjaer
  2019-05-11 17:00         ` Lorenzo Bianconi
  0 siblings, 1 reply; 9+ messages in thread
From: Sean Nyekjaer @ 2019-05-11 12:54 UTC (permalink / raw)
  To: Lorenzo Bianconi, Jonathan Cameron
  Cc: linux-iio, martin, Lorenzo Bianconi, Denis CIOCCA



On 11/05/2019 14.30, Lorenzo Bianconi wrote:
>> On Tue,  7 May 2019 10:02:25 +0200
>> Sean Nyekjaer <sean@geanix.com> wrote:
>>
>>> This adds support for using iio triggers, this is needed because
>>> our hardware guys forgot to connect the irq pins from imu device
>>> to the SoC.
>>>
>>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>> Hi Sean
>>
>> A small question on the size of the buffer needed inline.  Otherwise looks
>> good to me.
> 
> Hi Sean,
> 
> this patch does not make sense to me since running st_lsm6dsx_read_oneshot
> you need to wait to power up the device (and you will power it down at the
> end). I guess you will not be able to read at a given ODR (e.g. 416Hz).
> Moreover you can't read from the hw fifo without the irq line since
> you need to read a full pattern from it in order to maintain the alignment.
We are not using the hw fifo if the hw irq isn't present...
So if I understand correctly we could speed things up a bit, with 
leaving the sensor powered by implementing a new reading function and
calling st_lsm6dsx_sensor_set_enable when we enable the trigger?

>  From my point of view you have 2 possibility:
> - poll the output registers from userspace (this is what you are actually
>    doing from inside the kernel, what is the advantage of doing so?)
Yes this is exactly what I'm trying to accomplish here.
It would be nice for us to have the same hrtimer/trigger to sample this 
and our adc.

> - fix the hw bug
Not possible on +20K devices. ;-)

> 
> Moreover if I read the patch correctly it has a NULL pointer dereference bug
> since hw->iio_devs[i] can be NULL (e.g. if sensor hub is disabled)
Right above, there is:
if (!hw->iio_devs[i])
	return -ENOMEM;

Should be enough...
> 
> Regards,
> Lorenzo
> 

BR
/Sean

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

* Re: [PATCH 2/2] iio: imu: st_lsm6dsx: add iio trigger and buffer support
  2019-05-11 12:54       ` Sean Nyekjaer
@ 2019-05-11 17:00         ` Lorenzo Bianconi
  2019-05-16 11:46           ` Sean Nyekjaer
  2019-05-18  8:38           ` Jonathan Cameron
  0 siblings, 2 replies; 9+ messages in thread
From: Lorenzo Bianconi @ 2019-05-11 17:00 UTC (permalink / raw)
  To: Sean Nyekjaer
  Cc: Jonathan Cameron, linux-iio, martin, Lorenzo Bianconi, Denis CIOCCA

[-- Attachment #1: Type: text/plain, Size: 2736 bytes --]

> On 11/05/2019 14.30, Lorenzo Bianconi wrote:
> > > On Tue,  7 May 2019 10:02:25 +0200
> > > Sean Nyekjaer <sean@geanix.com> wrote:
> > > 
> > > > This adds support for using iio triggers, this is needed because
> > > > our hardware guys forgot to connect the irq pins from imu device
> > > > to the SoC.
> > > > 
> > > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > > Hi Sean
> > > 
> > > A small question on the size of the buffer needed inline.  Otherwise looks
> > > good to me.
> > 
> > Hi Sean,
> > 
> > this patch does not make sense to me since running st_lsm6dsx_read_oneshot
> > you need to wait to power up the device (and you will power it down at the
> > end). I guess you will not be able to read at a given ODR (e.g. 416Hz).
> > Moreover you can't read from the hw fifo without the irq line since
> > you need to read a full pattern from it in order to maintain the alignment.
> We are not using the hw fifo if the hw irq isn't present...
> So if I understand correctly we could speed things up a bit, with leaving
> the sensor powered by implementing a new reading function and
> calling st_lsm6dsx_sensor_set_enable when we enable the trigger?

I do not think so since in this way you will not know when the hw has updated
the output registers

> 
> >  From my point of view you have 2 possibility:
> > - poll the output registers from userspace (this is what you are actually
> >    doing from inside the kernel, what is the advantage of doing so?)
> Yes this is exactly what I'm trying to accomplish here.
> It would be nice for us to have the same hrtimer/trigger to sample this and
> our adc.

Since we need to wait the data to be ready there is no difference between reading
them polling the output registers in the sysfs and doing so

> 
> > - fix the hw bug
> Not possible on +20K devices. ;-)
> 
> > 
> > Moreover if I read the patch correctly it has a NULL pointer dereference bug
> > since hw->iio_devs[i] can be NULL (e.g. if sensor hub is disabled)
> Right above, there is:
> if (!hw->iio_devs[i])
> 	return -ENOMEM;
> 
> Should be enough...

Do you mean when we alloc hw->iio_devs[]? If so the for loop stops at
ST_LSM6DSX_ID_EXT0 while you are going through the complete list here
(ST_LSM6DSX_ID_MAX)...

> > 
> > Regards,
> > Lorenzo
> > 

What I think can be doable is to read data trough the hw fifo but use
the iio hr timer as trigger. We need to set the hr timer timeout according to
the pattern len and read the complete pattern. I am not 100% sure it will work
since read accesses and data generation are asynchronous (so there will be a
drift).
@Jonathan, Denis: can it work?

Regards,
Lorenzo


> 
> BR
> /Sean


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

* Re: [PATCH 2/2] iio: imu: st_lsm6dsx: add iio trigger and buffer support
  2019-05-11 17:00         ` Lorenzo Bianconi
@ 2019-05-16 11:46           ` Sean Nyekjaer
  2019-05-18  8:38           ` Jonathan Cameron
  1 sibling, 0 replies; 9+ messages in thread
From: Sean Nyekjaer @ 2019-05-16 11:46 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Jonathan Cameron, linux-iio, martin, Lorenzo Bianconi, Denis CIOCCA

Please ignore this patchset.
The IRQ is indeed connected to our SoC

/Sean

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

* Re: [PATCH 2/2] iio: imu: st_lsm6dsx: add iio trigger and buffer support
  2019-05-11 17:00         ` Lorenzo Bianconi
  2019-05-16 11:46           ` Sean Nyekjaer
@ 2019-05-18  8:38           ` Jonathan Cameron
  1 sibling, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2019-05-18  8:38 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Sean Nyekjaer, linux-iio, martin, Lorenzo Bianconi, Denis CIOCCA

On Sat, 11 May 2019 19:00:41 +0200
Lorenzo Bianconi <lorenzo@kernel.org> wrote:

> > On 11/05/2019 14.30, Lorenzo Bianconi wrote:  
> > > > On Tue,  7 May 2019 10:02:25 +0200
> > > > Sean Nyekjaer <sean@geanix.com> wrote:
> > > >   
> > > > > This adds support for using iio triggers, this is needed because
> > > > > our hardware guys forgot to connect the irq pins from imu device
> > > > > to the SoC.
> > > > > 
> > > > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>  
> > > > Hi Sean
> > > > 
> > > > A small question on the size of the buffer needed inline.  Otherwise looks
> > > > good to me.  
> > > 
> > > Hi Sean,
> > > 
> > > this patch does not make sense to me since running st_lsm6dsx_read_oneshot
> > > you need to wait to power up the device (and you will power it down at the
> > > end). I guess you will not be able to read at a given ODR (e.g. 416Hz).
> > > Moreover you can't read from the hw fifo without the irq line since
> > > you need to read a full pattern from it in order to maintain the alignment.  
> > We are not using the hw fifo if the hw irq isn't present...
> > So if I understand correctly we could speed things up a bit, with leaving
> > the sensor powered by implementing a new reading function and
> > calling st_lsm6dsx_sensor_set_enable when we enable the trigger?  
> 
> I do not think so since in this way you will not know when the hw has updated
> the output registers
> 
> >   
> > >  From my point of view you have 2 possibility:
> > > - poll the output registers from userspace (this is what you are actually
> > >    doing from inside the kernel, what is the advantage of doing so?)  
> > Yes this is exactly what I'm trying to accomplish here.
> > It would be nice for us to have the same hrtimer/trigger to sample this and
> > our adc.  
> 
> Since we need to wait the data to be ready there is no difference between reading
> them polling the output registers in the sysfs and doing so
> 
> >   
> > > - fix the hw bug  
> > Not possible on +20K devices. ;-)
> >   
> > > 
> > > Moreover if I read the patch correctly it has a NULL pointer dereference bug
> > > since hw->iio_devs[i] can be NULL (e.g. if sensor hub is disabled)  
> > Right above, there is:
> > if (!hw->iio_devs[i])
> > 	return -ENOMEM;
> > 
> > Should be enough...  
> 
> Do you mean when we alloc hw->iio_devs[]? If so the for loop stops at
> ST_LSM6DSX_ID_EXT0 while you are going through the complete list here
> (ST_LSM6DSX_ID_MAX)...
> 
> > > 
> > > Regards,
> > > Lorenzo
> > >   
> 
> What I think can be doable is to read data trough the hw fifo but use
> the iio hr timer as trigger. We need to set the hr timer timeout according to
> the pattern len and read the complete pattern. I am not 100% sure it will work
> since read accesses and data generation are asynchronous (so there will be a
> drift).
> @Jonathan, Denis: can it work?
Usual nasty trick to this is you read the fifo faster than you
in theory need to.  I wouldn't use the hr timer trigger for this though as it
adds apparent semantics that aren't true.  So spin a timer up inside the driver.

If you aren't using the fifo you can also do a higher efficiency check by
polling the status registers at twice the rate of of expected data generation.
When they are all set you read.  So basically you poll in the interrupt line
by reading the registers it reflects.  There are device variants out there
where there is an interrupt pin on the die but it doesn't reach the edge
of the package. In those cases we always have to use hacks like this.

Anyhow, seems no actual need for this. I just wanted to put some comments
in here for the future!

Jonathan


> 
> Regards,
> Lorenzo
> 
> 
> > 
> > BR
> > /Sean  
> 


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

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-07  8:02 [PATCH 1/2] iio: imu: st_lsm6dsx: only set available_scan_masks if using device fifo Sean Nyekjaer
2019-05-07  8:02 ` [PATCH 2/2] iio: imu: st_lsm6dsx: add iio trigger and buffer support Sean Nyekjaer
2019-05-11 11:37   ` Jonathan Cameron
2019-05-11 12:30     ` Lorenzo Bianconi
2019-05-11 12:54       ` Sean Nyekjaer
2019-05-11 17:00         ` Lorenzo Bianconi
2019-05-16 11:46           ` Sean Nyekjaer
2019-05-18  8:38           ` Jonathan Cameron
2019-05-11 12:38     ` 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