LSM9DS1 testing with st_lsm6dsx driver

LSM9DS1 testing with st_lsm6dsx driver

[Bobby Jones]

Hello Lorenzo and Martin,

I'm testing the LSM9DS1 support recently added to the st_lsm6dsx
iio/imu driver and I'm encountering a device tree problem related to
interrupt config.

Here's the exception I'm seeing:
[    4.172529] irq 277: nobody cared (try booting with the "irqpoll" option)
[    4.179341] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
5.3.0-rc5-00322-g792b824-dirty #7
[    4.187359] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[    4.193920] [<c0112750>] (unwind_backtrace) from [<c010d018>]
(show_stack+0x10/0x14)
[    4.201690] [<c010d018>] (show_stack) from [<c0c2bfc8>]
(dump_stack+0xd8/0x10c)
[    4.209027] [<c0c2bfc8>] (dump_stack) from [<c01923fc>]
(__report_bad_irq+0x24/0xc0)
[    4.216793] [<c01923fc>] (__report_bad_irq) from [<c0192820>]
(note_interrupt+0x27c/0x2dc)
[    4.225078] [<c0192820>] (note_interrupt) from [<c018f174>]
(handle_irq_event_percpu+0x54/0x7c)
[    4.233793] [<c018f174>] (handle_irq_event_percpu) from
[<c018f1d4>] (handle_irq_event+0x38/0x5c)
[    4.242681] [<c018f1d4>] (handle_irq_event) from [<c0193664>]
(handle_level_irq+0xc8/0x154)
[    4.251051] [<c0193664>] (handle_level_irq) from [<c018df58>]
(generic_handle_irq+0x20/0x34)
[    4.259510] [<c018df58>] (generic_handle_irq) from [<c053c348>]
(mxc_gpio_irq_handler+0xc4/0xf8)
[    4.268313] [<c053c348>] (mxc_gpio_irq_handler) from [<c053c3e0>]
(mx3_gpio_irq_handler+0x64/0xb8)
[    4.277287] [<c053c3e0>] (mx3_gpio_irq_handler) from [<c018df58>]
(generic_handle_irq+0x20/0x34)
[    4.286089] [<c018df58>] (generic_handle_irq) from [<c018e550>]
(__handle_domain_irq+0x64/0xe0)
[    4.294810] [<c018e550>] (__handle_domain_irq) from [<c0529610>]
(gic_handle_irq+0x4c/0xa0)
[    4.303181] [<c0529610>] (gic_handle_irq) from [<c0101a70>]
(__irq_svc+0x70/0x98)
[    4.310675] Exception stack(0xc1301f10 to 0xc1301f58)
[    4.315744] 1f00:                                     00000001
00000006 00000000 c130c340
[    4.323937] 1f20: c1300000 c1308928 00000001 c1308960 00000000
c12b9db0 c1308908 00000000
[    4.332128] 1f40: 00000000 c1301f60 c0182010 c0109508 20000013 ffffffff
[    4.338762] [<c0101a70>] (__irq_svc) from [<c0109508>]
(arch_cpu_idle+0x20/0x3c)
[    4.346180] [<c0109508>] (arch_cpu_idle) from [<c015ed70>]
(do_idle+0x1bc/0x2bc)
[    4.353594] [<c015ed70>] (do_idle) from [<c015f204>]
(cpu_startup_entry+0x18/0x1c)
[    4.361183] [<c015f204>] (cpu_startup_entry) from [<c1200e68>]
(start_kernel+0x440/0x504)
[    4.369378] [<c1200e68>] (start_kernel) from [<00000000>] (0x0)
[    4.375309] handlers:
[    4.377645] [<62052c0d>] st_lsm6dsx_handler_irq threaded
[<f2004b92>] st_lsm6dsx_handler_thread
[    4.386484] Disabling IRQ #277

Here is the associated device tree node:
lsm9ds1_ag@6a {
    compatible = "st,lsm9ds1-imu";
    reg = <0x6a>;
    st,drdy-int-pin = <1>;
    pinctrl-names = "default";
    pinctrl-0 = <&pinctrl_acc_gyro>;
    interrupt-parent = <&gpio7>;
    interrupts = <13 IRQ_TYPE_LEVEL_LOW>;
};

Let me know if I can provide any more information.

Thanks,
Bobby Jones

Re: LSM9DS1 testing with st_lsm6dsx driver

[Lorenzo Bianconi]

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

> Hello Lorenzo and Martin,

Hi Bobby,

LSM9DS1 does not support hw FIFO for the moment. Are you trying to enable
buffered reading?
Could you please try if the following patch helps? (just compiled)

Regards,
Lorenzo

iio: imu: st_lsm6dsx: do not configure the fifo if not supported

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +-
 1 file changed, 1 insertion(+), 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 b65a6ca775e0..90a0e5ce44e5 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1572,7 +1572,7 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
 			return err;
 	}
 
-	if (hw->irq > 0) {
+	if (hw->irq > 0 && hw->settings->fifo_ops.update_fifo) {
 		err = st_lsm6dsx_fifo_setup(hw);
 		if (err < 0)
 			return err;
-- 
2.21.0


> 
> I'm testing the LSM9DS1 support recently added to the st_lsm6dsx
> iio/imu driver and I'm encountering a device tree problem related to
> interrupt config.
> 
> Here's the exception I'm seeing:
> [    4.172529] irq 277: nobody cared (try booting with the "irqpoll" option)
> [    4.179341] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
> 5.3.0-rc5-00322-g792b824-dirty #7
> [    4.187359] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> [    4.193920] [<c0112750>] (unwind_backtrace) from [<c010d018>]
> (show_stack+0x10/0x14)
> [    4.201690] [<c010d018>] (show_stack) from [<c0c2bfc8>]
> (dump_stack+0xd8/0x10c)
> [    4.209027] [<c0c2bfc8>] (dump_stack) from [<c01923fc>]
> (__report_bad_irq+0x24/0xc0)
> [    4.216793] [<c01923fc>] (__report_bad_irq) from [<c0192820>]
> (note_interrupt+0x27c/0x2dc)
> [    4.225078] [<c0192820>] (note_interrupt) from [<c018f174>]
> (handle_irq_event_percpu+0x54/0x7c)
> [    4.233793] [<c018f174>] (handle_irq_event_percpu) from
> [<c018f1d4>] (handle_irq_event+0x38/0x5c)
> [    4.242681] [<c018f1d4>] (handle_irq_event) from [<c0193664>]
> (handle_level_irq+0xc8/0x154)
> [    4.251051] [<c0193664>] (handle_level_irq) from [<c018df58>]
> (generic_handle_irq+0x20/0x34)
> [    4.259510] [<c018df58>] (generic_handle_irq) from [<c053c348>]
> (mxc_gpio_irq_handler+0xc4/0xf8)
> [    4.268313] [<c053c348>] (mxc_gpio_irq_handler) from [<c053c3e0>]
> (mx3_gpio_irq_handler+0x64/0xb8)
> [    4.277287] [<c053c3e0>] (mx3_gpio_irq_handler) from [<c018df58>]
> (generic_handle_irq+0x20/0x34)
> [    4.286089] [<c018df58>] (generic_handle_irq) from [<c018e550>]
> (__handle_domain_irq+0x64/0xe0)
> [    4.294810] [<c018e550>] (__handle_domain_irq) from [<c0529610>]
> (gic_handle_irq+0x4c/0xa0)
> [    4.303181] [<c0529610>] (gic_handle_irq) from [<c0101a70>]
> (__irq_svc+0x70/0x98)
> [    4.310675] Exception stack(0xc1301f10 to 0xc1301f58)
> [    4.315744] 1f00:                                     00000001
> 00000006 00000000 c130c340
> [    4.323937] 1f20: c1300000 c1308928 00000001 c1308960 00000000
> c12b9db0 c1308908 00000000
> [    4.332128] 1f40: 00000000 c1301f60 c0182010 c0109508 20000013 ffffffff
> [    4.338762] [<c0101a70>] (__irq_svc) from [<c0109508>]
> (arch_cpu_idle+0x20/0x3c)
> [    4.346180] [<c0109508>] (arch_cpu_idle) from [<c015ed70>]
> (do_idle+0x1bc/0x2bc)
> [    4.353594] [<c015ed70>] (do_idle) from [<c015f204>]
> (cpu_startup_entry+0x18/0x1c)
> [    4.361183] [<c015f204>] (cpu_startup_entry) from [<c1200e68>]
> (start_kernel+0x440/0x504)
> [    4.369378] [<c1200e68>] (start_kernel) from [<00000000>] (0x0)
> [    4.375309] handlers:
> [    4.377645] [<62052c0d>] st_lsm6dsx_handler_irq threaded
> [<f2004b92>] st_lsm6dsx_handler_thread
> [    4.386484] Disabling IRQ #277
> 
> Here is the associated device tree node:
> lsm9ds1_ag@6a {
>     compatible = "st,lsm9ds1-imu";
>     reg = <0x6a>;
>     st,drdy-int-pin = <1>;
>     pinctrl-names = "default";
>     pinctrl-0 = <&pinctrl_acc_gyro>;
>     interrupt-parent = <&gpio7>;
>     interrupts = <13 IRQ_TYPE_LEVEL_LOW>;
> };
> 
> Let me know if I can provide any more information.
> 
> Thanks,
> Bobby Jones

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

Re: LSM9DS1 testing with st_lsm6dsx driver

[Bobby Jones]

> LSM9DS1 does not support hw FIFO for the moment. Are you trying to enable
> buffered reading?

I am not manually interacting with the device at all when this
exception occurs. This happens during the driver probe.

> Could you please try if the following patch helps? (just compiled)

I no longer receive the exception with this patch and it makes sense, thanks.

For context I'm working with a board that has every data ready and
interrupt signal connected to the LSM9DS1. Could you clarify what the
proper usage of the "st,drdy-int-pin" would be in this case and
whether or not I need more than one interrupt called out in my device
tree node?
I'm not really understanding how they're currently being utilized for
this device in the driver.

Also, I know support for this device was added recently and the combo
device hardware FIFO is complex, but is support for this something
that's currently being worked on?

Thanks,
Bobby Jones

> Regards,
> Lorenzo
>
> iio: imu: st_lsm6dsx: do not configure the fifo if not supported
>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +-
>  1 file changed, 1 insertion(+), 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 b65a6ca775e0..90a0e5ce44e5 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -1572,7 +1572,7 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
>                         return err;
>         }
>
> -       if (hw->irq > 0) {
> +       if (hw->irq > 0 && hw->settings->fifo_ops.update_fifo) {
>                 err = st_lsm6dsx_fifo_setup(hw);
>                 if (err < 0)
>                         return err;
> --
> 2.21.0
>
>

Re: LSM9DS1 testing with st_lsm6dsx driver

[Lorenzo Bianconi]

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

> > LSM9DS1 does not support hw FIFO for the moment. Are you trying to enable
> > buffered reading?
> 
> I am not manually interacting with the device at all when this
> exception occurs. This happens during the driver probe.
> 
> > Could you please try if the following patch helps? (just compiled)
> 
> I no longer receive the exception with this patch and it makes sense, thanks.

Hi Bobby,

thx a lot for testing. Could you please try to drop the previous patch and
apply the following one? Does it fix the issue as well?

iio: imu: st_lsm6dsx: check read_fifo pointer in st_lsm6dsx_handler_thread

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index b0f3da1976e4..f4fd4842bd79 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -666,6 +666,9 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
 	struct st_lsm6dsx_hw *hw = private;
 	int count;
 
+	if (!hw->settings->fifo_ops.read_fifo)
+		return IRQ_NONE;
+
 	mutex_lock(&hw->fifo_lock);
 	count = hw->settings->fifo_ops.read_fifo(hw);
 	mutex_unlock(&hw->fifo_lock);
-- 
2.21.0

> 
> For context I'm working with a board that has every data ready and
> interrupt signal connected to the LSM9DS1. Could you clarify what the
> proper usage of the "st,drdy-int-pin" would be in this case and
> whether or not I need more than one interrupt called out in my device
> tree node?
> I'm not really understanding how they're currently being utilized for
> this device in the driver.

For the moment irq line in lsm9ds1 (acc/gyro) is not used at all,
so you can omit the "st,drdy-int-pin" property

> 
> Also, I know support for this device was added recently and the combo
> device hardware FIFO is complex, but is support for this something
> that's currently being worked on?

It is actually in my ToDo list but I have no this device at the moment, so
patches are welcome :)

Regards,
Lorenzo

> 
> Thanks,
> Bobby Jones
> 
> > Regards,
> > Lorenzo
> >
> > iio: imu: st_lsm6dsx: do not configure the fifo if not supported
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +-
> >  1 file changed, 1 insertion(+), 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 b65a6ca775e0..90a0e5ce44e5 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > @@ -1572,7 +1572,7 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
> >                         return err;
> >         }
> >
> > -       if (hw->irq > 0) {
> > +       if (hw->irq > 0 && hw->settings->fifo_ops.update_fifo) {
> >                 err = st_lsm6dsx_fifo_setup(hw);
> >                 if (err < 0)
> >                         return err;
> > --
> > 2.21.0
> >
> >

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

Re: LSM9DS1 testing with st_lsm6dsx driver

[Bobby Jones]

> > > LSM9DS1 does not support hw FIFO for the moment. Are you trying to enable
> > > buffered reading?
> >
> > I am not manually interacting with the device at all when this
> > exception occurs. This happens during the driver probe.
> >
> > > Could you please try if the following patch helps? (just compiled)
> >
> > I no longer receive the exception with this patch and it makes sense, thanks.
>
> Hi Bobby,
>
> thx a lot for testing. Could you please try to drop the previous patch and
> apply the following one? Does it fix the issue as well?

No problem, happy to help. I just tested, and unfortunately the issue
is still present with this patch.
I gave the datasheet and the hardware reference manual of the
connected CPU a closer look and suspected a problem with my pin
config. However even after various combinations of pull ups/downs and
IRQ_TYPE_LEVEL_LOW/IRQ_TYPE_LEVEL_HIGH the same exception occurs, so I
don't think that there's an issue with pin config stopping the
interrupt line from being deasserted.

>
> iio: imu: st_lsm6dsx: check read_fifo pointer in st_lsm6dsx_handler_thread
>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> index b0f3da1976e4..f4fd4842bd79 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> @@ -666,6 +666,9 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
>         struct st_lsm6dsx_hw *hw = private;
>         int count;
>
> +       if (!hw->settings->fifo_ops.read_fifo)
> +               return IRQ_NONE;
> +
>         mutex_lock(&hw->fifo_lock);
>         count = hw->settings->fifo_ops.read_fifo(hw);
>         mutex_unlock(&hw->fifo_lock);
> --
> 2.21.0
>
> >
> > For context I'm working with a board that has every data ready and
> > interrupt signal connected to the LSM9DS1. Could you clarify what the
> > proper usage of the "st,drdy-int-pin" would be in this case and
> > whether or not I need more than one interrupt called out in my device
> > tree node?
> > I'm not really understanding how they're currently being utilized for
> > this device in the driver.
>
> For the moment irq line in lsm9ds1 (acc/gyro) is not used at all,
> so you can omit the "st,drdy-int-pin" property
>
> >
> > Also, I know support for this device was added recently and the combo
> > device hardware FIFO is complex, but is support for this something
> > that's currently being worked on?
>
> It is actually in my ToDo list but I have no this device at the moment, so
> patches are welcome :)
>
> Regards,
> Lorenzo
>
> >
> > Thanks,
> > Bobby Jones
> >
> > > Regards,
> > > Lorenzo
> > >
> > > iio: imu: st_lsm6dsx: do not configure the fifo if not supported
> > >
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 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 b65a6ca775e0..90a0e5ce44e5 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > @@ -1572,7 +1572,7 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
> > >                         return err;
> > >         }
> > >
> > > -       if (hw->irq > 0) {
> > > +       if (hw->irq > 0 && hw->settings->fifo_ops.update_fifo) {
> > >                 err = st_lsm6dsx_fifo_setup(hw);
> > >                 if (err < 0)
> > >                         return err;
> > > --
> > > 2.21.0
> > >
> > >

Re: LSM9DS1 testing with st_lsm6dsx driver

[Lorenzo Bianconi]

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

> > > > LSM9DS1 does not support hw FIFO for the moment. Are you trying to enable
> > > > buffered reading?
> > >
> > > I am not manually interacting with the device at all when this
> > > exception occurs. This happens during the driver probe.
> > >
> > > > Could you please try if the following patch helps? (just compiled)
> > >
> > > I no longer receive the exception with this patch and it makes sense, thanks.
> >
> > Hi Bobby,
> >
> > thx a lot for testing. Could you please try to drop the previous patch and
> > apply the following one? Does it fix the issue as well?
> 
> No problem, happy to help. I just tested, and unfortunately the issue
> is still present with this patch.

re-looking at the code this patch will actually does not anything since in the
current implementation st_lsm6dsx_handler_irq will return IRQ_NONE and the
thread handler will not be run. Anyway since st_lsm6dsx_handler_irq will be
removed soon I will respin this patch ontop of Sean series.

> I gave the datasheet and the hardware reference manual of the
> connected CPU a closer look and suspected a problem with my pin
> config. However even after various combinations of pull ups/downs and
> IRQ_TYPE_LEVEL_LOW/IRQ_TYPE_LEVEL_HIGH the same exception occurs, so I
> don't think that there's an issue with pin config stopping the
> interrupt line from being deasserted.

are you able to monitor the line activity through an oscilloscope?
The issue is the irq line is never asserted and the kernel complains about
lot of interrupts not managed

Looking at the datasheet, LSM9DS1 does not support IRQ_TYPE_LEVEL_LOW.
ST_LSM6DSX_REG_HLACTIVE_ADDR and ST_LSM6DSX_REG_PP_OD_ADDR registers are
actually not defined for LSM9DS1 and I will move them in hw_settings map.

@Jonathan: do you prefer this patch to be ontop of Sean's series?

Regards,
Lorenzo

> 
> >
> > iio: imu: st_lsm6dsx: check read_fifo pointer in st_lsm6dsx_handler_thread
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > index b0f3da1976e4..f4fd4842bd79 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > @@ -666,6 +666,9 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> >         struct st_lsm6dsx_hw *hw = private;
> >         int count;
> >
> > +       if (!hw->settings->fifo_ops.read_fifo)
> > +               return IRQ_NONE;
> > +
> >         mutex_lock(&hw->fifo_lock);
> >         count = hw->settings->fifo_ops.read_fifo(hw);
> >         mutex_unlock(&hw->fifo_lock);
> > --
> > 2.21.0
> >
> > >
> > > For context I'm working with a board that has every data ready and
> > > interrupt signal connected to the LSM9DS1. Could you clarify what the
> > > proper usage of the "st,drdy-int-pin" would be in this case and
> > > whether or not I need more than one interrupt called out in my device
> > > tree node?
> > > I'm not really understanding how they're currently being utilized for
> > > this device in the driver.
> >
> > For the moment irq line in lsm9ds1 (acc/gyro) is not used at all,
> > so you can omit the "st,drdy-int-pin" property
> >
> > >
> > > Also, I know support for this device was added recently and the combo
> > > device hardware FIFO is complex, but is support for this something
> > > that's currently being worked on?
> >
> > It is actually in my ToDo list but I have no this device at the moment, so
> > patches are welcome :)
> >
> > Regards,
> > Lorenzo
> >
> > >
> > > Thanks,
> > > Bobby Jones
> > >
> > > > Regards,
> > > > Lorenzo
> > > >
> > > > iio: imu: st_lsm6dsx: do not configure the fifo if not supported
> > > >
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 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 b65a6ca775e0..90a0e5ce44e5 100644
> > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > @@ -1572,7 +1572,7 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
> > > >                         return err;
> > > >         }
> > > >
> > > > -       if (hw->irq > 0) {
> > > > +       if (hw->irq > 0 && hw->settings->fifo_ops.update_fifo) {
> > > >                 err = st_lsm6dsx_fifo_setup(hw);
> > > >                 if (err < 0)
> > > >                         return err;
> > > > --
> > > > 2.21.0
> > > >
> > > >

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

Re: LSM9DS1 testing with st_lsm6dsx driver

[Bobby Jones]

> > > > > LSM9DS1 does not support hw FIFO for the moment. Are you trying to enable
> > > > > buffered reading?
> > > >
> > > > I am not manually interacting with the device at all when this
> > > > exception occurs. This happens during the driver probe.
> > > >
> > > > > Could you please try if the following patch helps? (just compiled)
> > > >
> > > > I no longer receive the exception with this patch and it makes sense, thanks.
> > >
> > > Hi Bobby,
> > >
> > > thx a lot for testing. Could you please try to drop the previous patch and
> > > apply the following one? Does it fix the issue as well?
> >
> > No problem, happy to help. I just tested, and unfortunately the issue
> > is still present with this patch.
>
> re-looking at the code this patch will actually does not anything since in the
> current implementation st_lsm6dsx_handler_irq will return IRQ_NONE and the
> thread handler will not be run. Anyway since st_lsm6dsx_handler_irq will be
> removed soon I will respin this patch ontop of Sean series.
>
> > I gave the datasheet and the hardware reference manual of the
> > connected CPU a closer look and suspected a problem with my pin
> > config. However even after various combinations of pull ups/downs and
> > IRQ_TYPE_LEVEL_LOW/IRQ_TYPE_LEVEL_HIGH the same exception occurs, so I
> > don't think that there's an issue with pin config stopping the
> > interrupt line from being deasserted.
>
> are you able to monitor the line activity through an oscilloscope?
> The issue is the irq line is never asserted and the kernel complains about
> lot of interrupts not managed
>
> Looking at the datasheet, LSM9DS1 does not support IRQ_TYPE_LEVEL_LOW.
> ST_LSM6DSX_REG_HLACTIVE_ADDR and ST_LSM6DSX_REG_PP_OD_ADDR registers are
> actually not defined for LSM9DS1 and I will move them in hw_settings map.

The datasheet I have for the LSM9DS1 shows a HLACTIVE bit in the
CTRL_REG8 (0x22) register, I thought that indicated support for both
high and low IRQ types.
Either way, after applying your recent (Sep 22nd) patches to this
driver and changing my device tree node I have no kernel exception and
the iio device enumerates as normal. For anyone curious my device tree
node is now:

lsm9ds1_ag@6a {
  compatible = "st,lsm9ds1-imu";
  reg = <0x6a>;
  st,drdy-int-pin = <1>;
  pinctrl-names = "default";
  pinctrl-0 = <&pinctrl_acc_gyro>;
  interrupt-parent = <&gpio7>;
  interrupts = <13 IRQ_TYPE_EDGE_RISING>;
};

>
> @Jonathan: do you prefer this patch to be ontop of Sean's series?
>
> Regards,
> Lorenzo
>
> >
> > >
> > > iio: imu: st_lsm6dsx: check read_fifo pointer in st_lsm6dsx_handler_thread
> > >
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > index b0f3da1976e4..f4fd4842bd79 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > @@ -666,6 +666,9 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> > >         struct st_lsm6dsx_hw *hw = private;
> > >         int count;
> > >
> > > +       if (!hw->settings->fifo_ops.read_fifo)
> > > +               return IRQ_NONE;
> > > +
> > >         mutex_lock(&hw->fifo_lock);
> > >         count = hw->settings->fifo_ops.read_fifo(hw);
> > >         mutex_unlock(&hw->fifo_lock);
> > > --
> > > 2.21.0
> > >
> > > >
> > > > For context I'm working with a board that has every data ready and
> > > > interrupt signal connected to the LSM9DS1. Could you clarify what the
> > > > proper usage of the "st,drdy-int-pin" would be in this case and
> > > > whether or not I need more than one interrupt called out in my device
> > > > tree node?
> > > > I'm not really understanding how they're currently being utilized for
> > > > this device in the driver.
> > >
> > > For the moment irq line in lsm9ds1 (acc/gyro) is not used at all,
> > > so you can omit the "st,drdy-int-pin" property
> > >
> > > >
> > > > Also, I know support for this device was added recently and the combo
> > > > device hardware FIFO is complex, but is support for this something
> > > > that's currently being worked on?
> > >
> > > It is actually in my ToDo list but I have no this device at the moment, so
> > > patches are welcome :)
> > >
> > > Regards,
> > > Lorenzo
> > >
> > > >
> > > > Thanks,
> > > > Bobby Jones
> > > >
> > > > > Regards,
> > > > > Lorenzo
> > > > >
> > > > > iio: imu: st_lsm6dsx: do not configure the fifo if not supported
> > > > >
> > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > ---
> > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 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 b65a6ca775e0..90a0e5ce44e5 100644
> > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > @@ -1572,7 +1572,7 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
> > > > >                         return err;
> > > > >         }
> > > > >
> > > > > -       if (hw->irq > 0) {
> > > > > +       if (hw->irq > 0 && hw->settings->fifo_ops.update_fifo) {
> > > > >                 err = st_lsm6dsx_fifo_setup(hw);
> > > > >                 if (err < 0)
> > > > >                         return err;
> > > > > --
> > > > > 2.21.0
> > > > >
> > > > >

Re: LSM9DS1 testing with st_lsm6dsx driver

[Lorenzo Bianconi]

>
> > > > > > LSM9DS1 does not support hw FIFO for the moment. Are you trying to enable
> > > > > > buffered reading?
> > > > >
> > > > > I am not manually interacting with the device at all when this
> > > > > exception occurs. This happens during the driver probe.
> > > > >
> > > > > > Could you please try if the following patch helps? (just compiled)
> > > > >
> > > > > I no longer receive the exception with this patch and it makes sense, thanks.
> > > >
> > > > Hi Bobby,
> > > >
> > > > thx a lot for testing. Could you please try to drop the previous patch and
> > > > apply the following one? Does it fix the issue as well?
> > >
> > > No problem, happy to help. I just tested, and unfortunately the issue
> > > is still present with this patch.
> >
> > re-looking at the code this patch will actually does not anything since in the
> > current implementation st_lsm6dsx_handler_irq will return IRQ_NONE and the
> > thread handler will not be run. Anyway since st_lsm6dsx_handler_irq will be
> > removed soon I will respin this patch ontop of Sean series.
> >
> > > I gave the datasheet and the hardware reference manual of the
> > > connected CPU a closer look and suspected a problem with my pin
> > > config. However even after various combinations of pull ups/downs and
> > > IRQ_TYPE_LEVEL_LOW/IRQ_TYPE_LEVEL_HIGH the same exception occurs, so I
> > > don't think that there's an issue with pin config stopping the
> > > interrupt line from being deasserted.
> >
> > are you able to monitor the line activity through an oscilloscope?
> > The issue is the irq line is never asserted and the kernel complains about
> > lot of interrupts not managed
> >
> > Looking at the datasheet, LSM9DS1 does not support IRQ_TYPE_LEVEL_LOW.
> > ST_LSM6DSX_REG_HLACTIVE_ADDR and ST_LSM6DSX_REG_PP_OD_ADDR registers are
> > actually not defined for LSM9DS1 and I will move them in hw_settings map.
>
> The datasheet I have for the LSM9DS1 shows a HLACTIVE bit in the
> CTRL_REG8 (0x22) register, I thought that indicated support for both
> high and low IRQ types.

Yes, right. I will update the series. Looking at the datasheet even
the BDU register definition seems wrong. I will fix it as well.

> Either way, after applying your recent (Sep 22nd) patches to this
> driver and changing my device tree node I have no kernel exception and
> the iio device enumerates as normal. For anyone curious my device tree
> node is now:
>

What happen if you set the irq line active high? Does the issue occur?

Regards,
Lorenzo

> lsm9ds1_ag@6a {
>   compatible = "st,lsm9ds1-imu";
>   reg = <0x6a>;
>   st,drdy-int-pin = <1>;
>   pinctrl-names = "default";
>   pinctrl-0 = <&pinctrl_acc_gyro>;
>   interrupt-parent = <&gpio7>;
>   interrupts = <13 IRQ_TYPE_EDGE_RISING>;
> };
>
> >
> > @Jonathan: do you prefer this patch to be ontop of Sean's series?
> >
> > Regards,
> > Lorenzo
> >
> > >
> > > >
> > > > iio: imu: st_lsm6dsx: check read_fifo pointer in st_lsm6dsx_handler_thread
> > > >
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > index b0f3da1976e4..f4fd4842bd79 100644
> > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > @@ -666,6 +666,9 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> > > >         struct st_lsm6dsx_hw *hw = private;
> > > >         int count;
> > > >
> > > > +       if (!hw->settings->fifo_ops.read_fifo)
> > > > +               return IRQ_NONE;
> > > > +
> > > >         mutex_lock(&hw->fifo_lock);
> > > >         count = hw->settings->fifo_ops.read_fifo(hw);
> > > >         mutex_unlock(&hw->fifo_lock);
> > > > --
> > > > 2.21.0
> > > >
> > > > >
> > > > > For context I'm working with a board that has every data ready and
> > > > > interrupt signal connected to the LSM9DS1. Could you clarify what the
> > > > > proper usage of the "st,drdy-int-pin" would be in this case and
> > > > > whether or not I need more than one interrupt called out in my device
> > > > > tree node?
> > > > > I'm not really understanding how they're currently being utilized for
> > > > > this device in the driver.
> > > >
> > > > For the moment irq line in lsm9ds1 (acc/gyro) is not used at all,
> > > > so you can omit the "st,drdy-int-pin" property
> > > >
> > > > >
> > > > > Also, I know support for this device was added recently and the combo
> > > > > device hardware FIFO is complex, but is support for this something
> > > > > that's currently being worked on?
> > > >
> > > > It is actually in my ToDo list but I have no this device at the moment, so
> > > > patches are welcome :)
> > > >
> > > > Regards,
> > > > Lorenzo
> > > >
> > > > >
> > > > > Thanks,
> > > > > Bobby Jones
> > > > >
> > > > > > Regards,
> > > > > > Lorenzo
> > > > > >
> > > > > > iio: imu: st_lsm6dsx: do not configure the fifo if not supported
> > > > > >
> > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > ---
> > > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 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 b65a6ca775e0..90a0e5ce44e5 100644
> > > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > @@ -1572,7 +1572,7 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
> > > > > >                         return err;
> > > > > >         }
> > > > > >
> > > > > > -       if (hw->irq > 0) {
> > > > > > +       if (hw->irq > 0 && hw->settings->fifo_ops.update_fifo) {
> > > > > >                 err = st_lsm6dsx_fifo_setup(hw);
> > > > > >                 if (err < 0)
> > > > > >                         return err;
> > > > > > --
> > > > > > 2.21.0
> > > > > >
> > > > > >

Re: LSM9DS1 testing with st_lsm6dsx driver

[Bobby Jones]

> > > > > > > LSM9DS1 does not support hw FIFO for the moment. Are you trying to enable
> > > > > > > buffered reading?
> > > > > >
> > > > > > I am not manually interacting with the device at all when this
> > > > > > exception occurs. This happens during the driver probe.
> > > > > >
> > > > > > > Could you please try if the following patch helps? (just compiled)
> > > > > >
> > > > > > I no longer receive the exception with this patch and it makes sense, thanks.
> > > > >
> > > > > Hi Bobby,
> > > > >
> > > > > thx a lot for testing. Could you please try to drop the previous patch and
> > > > > apply the following one? Does it fix the issue as well?
> > > >
> > > > No problem, happy to help. I just tested, and unfortunately the issue
> > > > is still present with this patch.
> > >
> > > re-looking at the code this patch will actually does not anything since in the
> > > current implementation st_lsm6dsx_handler_irq will return IRQ_NONE and the
> > > thread handler will not be run. Anyway since st_lsm6dsx_handler_irq will be
> > > removed soon I will respin this patch ontop of Sean series.
> > >
> > > > I gave the datasheet and the hardware reference manual of the
> > > > connected CPU a closer look and suspected a problem with my pin
> > > > config. However even after various combinations of pull ups/downs and
> > > > IRQ_TYPE_LEVEL_LOW/IRQ_TYPE_LEVEL_HIGH the same exception occurs, so I
> > > > don't think that there's an issue with pin config stopping the
> > > > interrupt line from being deasserted.
> > >
> > > are you able to monitor the line activity through an oscilloscope?
> > > The issue is the irq line is never asserted and the kernel complains about
> > > lot of interrupts not managed
> > >
> > > Looking at the datasheet, LSM9DS1 does not support IRQ_TYPE_LEVEL_LOW.
> > > ST_LSM6DSX_REG_HLACTIVE_ADDR and ST_LSM6DSX_REG_PP_OD_ADDR registers are
> > > actually not defined for LSM9DS1 and I will move them in hw_settings map.
> >
> > The datasheet I have for the LSM9DS1 shows a HLACTIVE bit in the
> > CTRL_REG8 (0x22) register, I thought that indicated support for both
> > high and low IRQ types.
>
> Yes, right. I will update the series. Looking at the datasheet even
> the BDU register definition seems wrong. I will fix it as well.
>
> > Either way, after applying your recent (Sep 22nd) patches to this
> > driver and changing my device tree node I have no kernel exception and
> > the iio device enumerates as normal. For anyone curious my device tree
> > node is now:
> >
>
> What happen if you set the irq line active high? Does the issue occur?

No issue with IRQ_TYPE_LEVEL_HIGH either. Although all I'm doing at
this point to test is checking for the exception in the kernel prints
and reading the /sys/bus/iio/devices/iio:device*/in*raw of the device.

Thanks,
Bobby Jones

>
> Regards,
> Lorenzo
>
> > lsm9ds1_ag@6a {
> >   compatible = "st,lsm9ds1-imu";
> >   reg = <0x6a>;
> >   st,drdy-int-pin = <1>;
> >   pinctrl-names = "default";
> >   pinctrl-0 = <&pinctrl_acc_gyro>;
> >   interrupt-parent = <&gpio7>;
> >   interrupts = <13 IRQ_TYPE_EDGE_RISING>;
> > };
> >
> > >
> > > @Jonathan: do you prefer this patch to be ontop of Sean's series?
> > >
> > > Regards,
> > > Lorenzo
> > >
> > > >
> > > > >
> > > > > iio: imu: st_lsm6dsx: check read_fifo pointer in st_lsm6dsx_handler_thread
> > > > >
> > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > ---
> > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > index b0f3da1976e4..f4fd4842bd79 100644
> > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > @@ -666,6 +666,9 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> > > > >         struct st_lsm6dsx_hw *hw = private;
> > > > >         int count;
> > > > >
> > > > > +       if (!hw->settings->fifo_ops.read_fifo)
> > > > > +               return IRQ_NONE;
> > > > > +
> > > > >         mutex_lock(&hw->fifo_lock);
> > > > >         count = hw->settings->fifo_ops.read_fifo(hw);
> > > > >         mutex_unlock(&hw->fifo_lock);
> > > > > --
> > > > > 2.21.0
> > > > >
> > > > > >
> > > > > > For context I'm working with a board that has every data ready and
> > > > > > interrupt signal connected to the LSM9DS1. Could you clarify what the
> > > > > > proper usage of the "st,drdy-int-pin" would be in this case and
> > > > > > whether or not I need more than one interrupt called out in my device
> > > > > > tree node?
> > > > > > I'm not really understanding how they're currently being utilized for
> > > > > > this device in the driver.
> > > > >
> > > > > For the moment irq line in lsm9ds1 (acc/gyro) is not used at all,
> > > > > so you can omit the "st,drdy-int-pin" property
> > > > >
> > > > > >
> > > > > > Also, I know support for this device was added recently and the combo
> > > > > > device hardware FIFO is complex, but is support for this something
> > > > > > that's currently being worked on?
> > > > >
> > > > > It is actually in my ToDo list but I have no this device at the moment, so
> > > > > patches are welcome :)
> > > > >
> > > > > Regards,
> > > > > Lorenzo
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Bobby Jones
> > > > > >
> > > > > > > Regards,
> > > > > > > Lorenzo
> > > > > > >
> > > > > > > iio: imu: st_lsm6dsx: do not configure the fifo if not supported
> > > > > > >
> > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > ---
> > > > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 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 b65a6ca775e0..90a0e5ce44e5 100644
> > > > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > > @@ -1572,7 +1572,7 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
> > > > > > >                         return err;
> > > > > > >         }
> > > > > > >
> > > > > > > -       if (hw->irq > 0) {
> > > > > > > +       if (hw->irq > 0 && hw->settings->fifo_ops.update_fifo) {
> > > > > > >                 err = st_lsm6dsx_fifo_setup(hw);
> > > > > > >                 if (err < 0)
> > > > > > >                         return err;
> > > > > > > --
> > > > > > > 2.21.0
> > > > > > >
> > > > > > >

Re: LSM9DS1 testing with st_lsm6dsx driver

[Lorenzo Bianconi]

>
> > > > > > > > LSM9DS1 does not support hw FIFO for the moment. Are you trying to enable
> > > > > > > > buffered reading?
> > > > > > >
> > > > > > > I am not manually interacting with the device at all when this
> > > > > > > exception occurs. This happens during the driver probe.
> > > > > > >
> > > > > > > > Could you please try if the following patch helps? (just compiled)
> > > > > > >
> > > > > > > I no longer receive the exception with this patch and it makes sense, thanks.
> > > > > >
> > > > > > Hi Bobby,
> > > > > >
> > > > > > thx a lot for testing. Could you please try to drop the previous patch and
> > > > > > apply the following one? Does it fix the issue as well?
> > > > >
> > > > > No problem, happy to help. I just tested, and unfortunately the issue
> > > > > is still present with this patch.
> > > >
> > > > re-looking at the code this patch will actually does not anything since in the
> > > > current implementation st_lsm6dsx_handler_irq will return IRQ_NONE and the
> > > > thread handler will not be run. Anyway since st_lsm6dsx_handler_irq will be
> > > > removed soon I will respin this patch ontop of Sean series.
> > > >
> > > > > I gave the datasheet and the hardware reference manual of the
> > > > > connected CPU a closer look and suspected a problem with my pin
> > > > > config. However even after various combinations of pull ups/downs and
> > > > > IRQ_TYPE_LEVEL_LOW/IRQ_TYPE_LEVEL_HIGH the same exception occurs, so I
> > > > > don't think that there's an issue with pin config stopping the
> > > > > interrupt line from being deasserted.
> > > >
> > > > are you able to monitor the line activity through an oscilloscope?
> > > > The issue is the irq line is never asserted and the kernel complains about
> > > > lot of interrupts not managed
> > > >
> > > > Looking at the datasheet, LSM9DS1 does not support IRQ_TYPE_LEVEL_LOW.
> > > > ST_LSM6DSX_REG_HLACTIVE_ADDR and ST_LSM6DSX_REG_PP_OD_ADDR registers are
> > > > actually not defined for LSM9DS1 and I will move them in hw_settings map.
> > >
> > > The datasheet I have for the LSM9DS1 shows a HLACTIVE bit in the
> > > CTRL_REG8 (0x22) register, I thought that indicated support for both
> > > high and low IRQ types.
> >
> > Yes, right. I will update the series. Looking at the datasheet even
> > the BDU register definition seems wrong. I will fix it as well.
> >
> > > Either way, after applying your recent (Sep 22nd) patches to this
> > > driver and changing my device tree node I have no kernel exception and
> > > the iio device enumerates as normal. For anyone curious my device tree
> > > node is now:
> > >
> >
> > What happen if you set the irq line active high? Does the issue occur?
>
> No issue with IRQ_TYPE_LEVEL_HIGH either. Although all I'm doing at
> this point to test is checking for the exception in the kernel prints
> and reading the /sys/bus/iio/devices/iio:device*/in*raw of the device.

Ok, thx a lot for testing. Could you please try to understand which
patch of the series fix the issue?
In particular, could you please try to apply just '[PATCH 3/3] iio:
imu: st_lsm6dsx: add sanity check for read_fifo pointer' and double
check if the issue occurs?

Regards,
Lorenzo

>
> Thanks,
> Bobby Jones
>
> >
> > Regards,
> > Lorenzo
> >
> > > lsm9ds1_ag@6a {
> > >   compatible = "st,lsm9ds1-imu";
> > >   reg = <0x6a>;
> > >   st,drdy-int-pin = <1>;
> > >   pinctrl-names = "default";
> > >   pinctrl-0 = <&pinctrl_acc_gyro>;
> > >   interrupt-parent = <&gpio7>;
> > >   interrupts = <13 IRQ_TYPE_EDGE_RISING>;
> > > };
> > >
> > > >
> > > > @Jonathan: do you prefer this patch to be ontop of Sean's series?
> > > >
> > > > Regards,
> > > > Lorenzo
> > > >
> > > > >
> > > > > >
> > > > > > iio: imu: st_lsm6dsx: check read_fifo pointer in st_lsm6dsx_handler_thread
> > > > > >
> > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > ---
> > > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 3 +++
> > > > > >  1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > > index b0f3da1976e4..f4fd4842bd79 100644
> > > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > > @@ -666,6 +666,9 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> > > > > >         struct st_lsm6dsx_hw *hw = private;
> > > > > >         int count;
> > > > > >
> > > > > > +       if (!hw->settings->fifo_ops.read_fifo)
> > > > > > +               return IRQ_NONE;
> > > > > > +
> > > > > >         mutex_lock(&hw->fifo_lock);
> > > > > >         count = hw->settings->fifo_ops.read_fifo(hw);
> > > > > >         mutex_unlock(&hw->fifo_lock);
> > > > > > --
> > > > > > 2.21.0
> > > > > >
> > > > > > >
> > > > > > > For context I'm working with a board that has every data ready and
> > > > > > > interrupt signal connected to the LSM9DS1. Could you clarify what the
> > > > > > > proper usage of the "st,drdy-int-pin" would be in this case and
> > > > > > > whether or not I need more than one interrupt called out in my device
> > > > > > > tree node?
> > > > > > > I'm not really understanding how they're currently being utilized for
> > > > > > > this device in the driver.
> > > > > >
> > > > > > For the moment irq line in lsm9ds1 (acc/gyro) is not used at all,
> > > > > > so you can omit the "st,drdy-int-pin" property
> > > > > >
> > > > > > >
> > > > > > > Also, I know support for this device was added recently and the combo
> > > > > > > device hardware FIFO is complex, but is support for this something
> > > > > > > that's currently being worked on?
> > > > > >
> > > > > > It is actually in my ToDo list but I have no this device at the moment, so
> > > > > > patches are welcome :)
> > > > > >
> > > > > > Regards,
> > > > > > Lorenzo
> > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Bobby Jones
> > > > > > >
> > > > > > > > Regards,
> > > > > > > > Lorenzo
> > > > > > > >
> > > > > > > > iio: imu: st_lsm6dsx: do not configure the fifo if not supported
> > > > > > > >
> > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > > ---
> > > > > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +-
> > > > > > > >  1 file changed, 1 insertion(+), 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 b65a6ca775e0..90a0e5ce44e5 100644
> > > > > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > > > @@ -1572,7 +1572,7 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
> > > > > > > >                         return err;
> > > > > > > >         }
> > > > > > > >
> > > > > > > > -       if (hw->irq > 0) {
> > > > > > > > +       if (hw->irq > 0 && hw->settings->fifo_ops.update_fifo) {
> > > > > > > >                 err = st_lsm6dsx_fifo_setup(hw);
> > > > > > > >                 if (err < 0)
> > > > > > > >                         return err;
> > > > > > > > --
> > > > > > > > 2.21.0
> > > > > > > >
> > > > > > > >



-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep

Re: LSM9DS1 testing with st_lsm6dsx driver

[Bobby Jones]

> > > > > > > > > LSM9DS1 does not support hw FIFO for the moment. Are you trying to enable
> > > > > > > > > buffered reading?
> > > > > > > >
> > > > > > > > I am not manually interacting with the device at all when this
> > > > > > > > exception occurs. This happens during the driver probe.
> > > > > > > >
> > > > > > > > > Could you please try if the following patch helps? (just compiled)
> > > > > > > >
> > > > > > > > I no longer receive the exception with this patch and it makes sense, thanks.
> > > > > > >
> > > > > > > Hi Bobby,
> > > > > > >
> > > > > > > thx a lot for testing. Could you please try to drop the previous patch and
> > > > > > > apply the following one? Does it fix the issue as well?
> > > > > >
> > > > > > No problem, happy to help. I just tested, and unfortunately the issue
> > > > > > is still present with this patch.
> > > > >
> > > > > re-looking at the code this patch will actually does not anything since in the
> > > > > current implementation st_lsm6dsx_handler_irq will return IRQ_NONE and the
> > > > > thread handler will not be run. Anyway since st_lsm6dsx_handler_irq will be
> > > > > removed soon I will respin this patch ontop of Sean series.
> > > > >
> > > > > > I gave the datasheet and the hardware reference manual of the
> > > > > > connected CPU a closer look and suspected a problem with my pin
> > > > > > config. However even after various combinations of pull ups/downs and
> > > > > > IRQ_TYPE_LEVEL_LOW/IRQ_TYPE_LEVEL_HIGH the same exception occurs, so I
> > > > > > don't think that there's an issue with pin config stopping the
> > > > > > interrupt line from being deasserted.
> > > > >
> > > > > are you able to monitor the line activity through an oscilloscope?
> > > > > The issue is the irq line is never asserted and the kernel complains about
> > > > > lot of interrupts not managed
> > > > >
> > > > > Looking at the datasheet, LSM9DS1 does not support IRQ_TYPE_LEVEL_LOW.
> > > > > ST_LSM6DSX_REG_HLACTIVE_ADDR and ST_LSM6DSX_REG_PP_OD_ADDR registers are
> > > > > actually not defined for LSM9DS1 and I will move them in hw_settings map.
> > > >
> > > > The datasheet I have for the LSM9DS1 shows a HLACTIVE bit in the
> > > > CTRL_REG8 (0x22) register, I thought that indicated support for both
> > > > high and low IRQ types.
> > >
> > > Yes, right. I will update the series. Looking at the datasheet even
> > > the BDU register definition seems wrong. I will fix it as well.
> > >
> > > > Either way, after applying your recent (Sep 22nd) patches to this
> > > > driver and changing my device tree node I have no kernel exception and
> > > > the iio device enumerates as normal. For anyone curious my device tree
> > > > node is now:
> > > >
> > >
> > > What happen if you set the irq line active high? Does the issue occur?
> >
> > No issue with IRQ_TYPE_LEVEL_HIGH either. Although all I'm doing at
> > this point to test is checking for the exception in the kernel prints
> > and reading the /sys/bus/iio/devices/iio:device*/in*raw of the device.
>
> Ok, thx a lot for testing. Could you please try to understand which
> patch of the series fix the issue?
> In particular, could you please try to apply just '[PATCH 3/3] iio:
> imu: st_lsm6dsx: add sanity check for read_fifo pointer' and double
> check if the issue occurs?

No problem. It looks like that was the one, only having that patch
applied works fine.

Thanks,
Bobby Jones

>
> Regards,
> Lorenzo
>
> >
> > Thanks,
> > Bobby Jones
> >
> > >
> > > Regards,
> > > Lorenzo
> > >
> > > > lsm9ds1_ag@6a {
> > > >   compatible = "st,lsm9ds1-imu";
> > > >   reg = <0x6a>;
> > > >   st,drdy-int-pin = <1>;
> > > >   pinctrl-names = "default";
> > > >   pinctrl-0 = <&pinctrl_acc_gyro>;
> > > >   interrupt-parent = <&gpio7>;
> > > >   interrupts = <13 IRQ_TYPE_EDGE_RISING>;
> > > > };
> > > >
> > > > >
> > > > > @Jonathan: do you prefer this patch to be ontop of Sean's series?
> > > > >
> > > > > Regards,
> > > > > Lorenzo
> > > > >
> > > > > >
> > > > > > >
> > > > > > > iio: imu: st_lsm6dsx: check read_fifo pointer in st_lsm6dsx_handler_thread
> > > > > > >
> > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > ---
> > > > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 3 +++
> > > > > > >  1 file changed, 3 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > > > index b0f3da1976e4..f4fd4842bd79 100644
> > > > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > > > @@ -666,6 +666,9 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> > > > > > >         struct st_lsm6dsx_hw *hw = private;
> > > > > > >         int count;
> > > > > > >
> > > > > > > +       if (!hw->settings->fifo_ops.read_fifo)
> > > > > > > +               return IRQ_NONE;
> > > > > > > +
> > > > > > >         mutex_lock(&hw->fifo_lock);
> > > > > > >         count = hw->settings->fifo_ops.read_fifo(hw);
> > > > > > >         mutex_unlock(&hw->fifo_lock);
> > > > > > > --
> > > > > > > 2.21.0
> > > > > > >
> > > > > > > >
> > > > > > > > For context I'm working with a board that has every data ready and
> > > > > > > > interrupt signal connected to the LSM9DS1. Could you clarify what the
> > > > > > > > proper usage of the "st,drdy-int-pin" would be in this case and
> > > > > > > > whether or not I need more than one interrupt called out in my device
> > > > > > > > tree node?
> > > > > > > > I'm not really understanding how they're currently being utilized for
> > > > > > > > this device in the driver.
> > > > > > >
> > > > > > > For the moment irq line in lsm9ds1 (acc/gyro) is not used at all,
> > > > > > > so you can omit the "st,drdy-int-pin" property
> > > > > > >
> > > > > > > >
> > > > > > > > Also, I know support for this device was added recently and the combo
> > > > > > > > device hardware FIFO is complex, but is support for this something
> > > > > > > > that's currently being worked on?
> > > > > > >
> > > > > > > It is actually in my ToDo list but I have no this device at the moment, so
> > > > > > > patches are welcome :)
> > > > > > >
> > > > > > > Regards,
> > > > > > > Lorenzo
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Bobby Jones
> > > > > > > >
> > > > > > > > > Regards,
> > > > > > > > > Lorenzo
> > > > > > > > >
> > > > > > > > > iio: imu: st_lsm6dsx: do not configure the fifo if not supported
> > > > > > > > >
> > > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > > > ---
> > > > > > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +-
> > > > > > > > >  1 file changed, 1 insertion(+), 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 b65a6ca775e0..90a0e5ce44e5 100644
> > > > > > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > > > > @@ -1572,7 +1572,7 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
> > > > > > > > >                         return err;
> > > > > > > > >         }
> > > > > > > > >
> > > > > > > > > -       if (hw->irq > 0) {
> > > > > > > > > +       if (hw->irq > 0 && hw->settings->fifo_ops.update_fifo) {
> > > > > > > > >                 err = st_lsm6dsx_fifo_setup(hw);
> > > > > > > > >                 if (err < 0)
> > > > > > > > >                         return err;
> > > > > > > > > --
> > > > > > > > > 2.21.0
> > > > > > > > >
> > > > > > > > >

Re: LSM9DS1 testing with st_lsm6dsx driver

[Lorenzo Bianconi]

>
> > > > > > > > > > LSM9DS1 does not support hw FIFO for the moment. Are you trying to enable
> > > > > > > > > > buffered reading?
> > > > > > > > >
> > > > > > > > > I am not manually interacting with the device at all when this
> > > > > > > > > exception occurs. This happens during the driver probe.
> > > > > > > > >
> > > > > > > > > > Could you please try if the following patch helps? (just compiled)
> > > > > > > > >
> > > > > > > > > I no longer receive the exception with this patch and it makes sense, thanks.
> > > > > > > >
> > > > > > > > Hi Bobby,
> > > > > > > >
> > > > > > > > thx a lot for testing. Could you please try to drop the previous patch and
> > > > > > > > apply the following one? Does it fix the issue as well?
> > > > > > >
> > > > > > > No problem, happy to help. I just tested, and unfortunately the issue
> > > > > > > is still present with this patch.
> > > > > >
> > > > > > re-looking at the code this patch will actually does not anything since in the
> > > > > > current implementation st_lsm6dsx_handler_irq will return IRQ_NONE and the
> > > > > > thread handler will not be run. Anyway since st_lsm6dsx_handler_irq will be
> > > > > > removed soon I will respin this patch ontop of Sean series.
> > > > > >
> > > > > > > I gave the datasheet and the hardware reference manual of the
> > > > > > > connected CPU a closer look and suspected a problem with my pin
> > > > > > > config. However even after various combinations of pull ups/downs and
> > > > > > > IRQ_TYPE_LEVEL_LOW/IRQ_TYPE_LEVEL_HIGH the same exception occurs, so I
> > > > > > > don't think that there's an issue with pin config stopping the
> > > > > > > interrupt line from being deasserted.
> > > > > >
> > > > > > are you able to monitor the line activity through an oscilloscope?
> > > > > > The issue is the irq line is never asserted and the kernel complains about
> > > > > > lot of interrupts not managed
> > > > > >
> > > > > > Looking at the datasheet, LSM9DS1 does not support IRQ_TYPE_LEVEL_LOW.
> > > > > > ST_LSM6DSX_REG_HLACTIVE_ADDR and ST_LSM6DSX_REG_PP_OD_ADDR registers are
> > > > > > actually not defined for LSM9DS1 and I will move them in hw_settings map.
> > > > >
> > > > > The datasheet I have for the LSM9DS1 shows a HLACTIVE bit in the
> > > > > CTRL_REG8 (0x22) register, I thought that indicated support for both
> > > > > high and low IRQ types.
> > > >
> > > > Yes, right. I will update the series. Looking at the datasheet even
> > > > the BDU register definition seems wrong. I will fix it as well.
> > > >
> > > > > Either way, after applying your recent (Sep 22nd) patches to this
> > > > > driver and changing my device tree node I have no kernel exception and
> > > > > the iio device enumerates as normal. For anyone curious my device tree
> > > > > node is now:
> > > > >
> > > >
> > > > What happen if you set the irq line active high? Does the issue occur?
> > >
> > > No issue with IRQ_TYPE_LEVEL_HIGH either. Although all I'm doing at
> > > this point to test is checking for the exception in the kernel prints
> > > and reading the /sys/bus/iio/devices/iio:device*/in*raw of the device.
> >
> > Ok, thx a lot for testing. Could you please try to understand which
> > patch of the series fix the issue?
> > In particular, could you please try to apply just '[PATCH 3/3] iio:
> > imu: st_lsm6dsx: add sanity check for read_fifo pointer' and double
> > check if the issue occurs?
>
> No problem. It looks like that was the one, only having that patch
> applied works fine.

Ok, cool. Thx a lot for testing.

@Jonathan: how do you prefer to proceed? Push just this patch to
'togreg' branch and rebase the other fixes ontop of Sean's series or
merge all the fixes now? (they will conflict with Sean's series)

Regards,
Lorenzo

>
> Thanks,
> Bobby Jones
>
> >
> > Regards,
> > Lorenzo
> >
> > >
> > > Thanks,
> > > Bobby Jones
> > >
> > > >
> > > > Regards,
> > > > Lorenzo
> > > >
> > > > > lsm9ds1_ag@6a {
> > > > >   compatible = "st,lsm9ds1-imu";
> > > > >   reg = <0x6a>;
> > > > >   st,drdy-int-pin = <1>;
> > > > >   pinctrl-names = "default";
> > > > >   pinctrl-0 = <&pinctrl_acc_gyro>;
> > > > >   interrupt-parent = <&gpio7>;
> > > > >   interrupts = <13 IRQ_TYPE_EDGE_RISING>;
> > > > > };
> > > > >
> > > > > >
> > > > > > @Jonathan: do you prefer this patch to be ontop of Sean's series?
> > > > > >
> > > > > > Regards,
> > > > > > Lorenzo
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > iio: imu: st_lsm6dsx: check read_fifo pointer in st_lsm6dsx_handler_thread
> > > > > > > >
> > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > > ---
> > > > > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 3 +++
> > > > > > > >  1 file changed, 3 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > > > > index b0f3da1976e4..f4fd4842bd79 100644
> > > > > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > > > > @@ -666,6 +666,9 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> > > > > > > >         struct st_lsm6dsx_hw *hw = private;
> > > > > > > >         int count;
> > > > > > > >
> > > > > > > > +       if (!hw->settings->fifo_ops.read_fifo)
> > > > > > > > +               return IRQ_NONE;
> > > > > > > > +
> > > > > > > >         mutex_lock(&hw->fifo_lock);
> > > > > > > >         count = hw->settings->fifo_ops.read_fifo(hw);
> > > > > > > >         mutex_unlock(&hw->fifo_lock);
> > > > > > > > --
> > > > > > > > 2.21.0
> > > > > > > >
> > > > > > > > >
> > > > > > > > > For context I'm working with a board that has every data ready and
> > > > > > > > > interrupt signal connected to the LSM9DS1. Could you clarify what the
> > > > > > > > > proper usage of the "st,drdy-int-pin" would be in this case and
> > > > > > > > > whether or not I need more than one interrupt called out in my device
> > > > > > > > > tree node?
> > > > > > > > > I'm not really understanding how they're currently being utilized for
> > > > > > > > > this device in the driver.
> > > > > > > >
> > > > > > > > For the moment irq line in lsm9ds1 (acc/gyro) is not used at all,
> > > > > > > > so you can omit the "st,drdy-int-pin" property
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Also, I know support for this device was added recently and the combo
> > > > > > > > > device hardware FIFO is complex, but is support for this something
> > > > > > > > > that's currently being worked on?
> > > > > > > >
> > > > > > > > It is actually in my ToDo list but I have no this device at the moment, so
> > > > > > > > patches are welcome :)
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > > Lorenzo
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > > Bobby Jones
> > > > > > > > >
> > > > > > > > > > Regards,
> > > > > > > > > > Lorenzo
> > > > > > > > > >
> > > > > > > > > > iio: imu: st_lsm6dsx: do not configure the fifo if not supported
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +-
> > > > > > > > > >  1 file changed, 1 insertion(+), 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 b65a6ca775e0..90a0e5ce44e5 100644
> > > > > > > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > > > > > @@ -1572,7 +1572,7 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
> > > > > > > > > >                         return err;
> > > > > > > > > >         }
> > > > > > > > > >
> > > > > > > > > > -       if (hw->irq > 0) {
> > > > > > > > > > +       if (hw->irq > 0 && hw->settings->fifo_ops.update_fifo) {
> > > > > > > > > >                 err = st_lsm6dsx_fifo_setup(hw);
> > > > > > > > > >                 if (err < 0)
> > > > > > > > > >                         return err;
> > > > > > > > > > --
> > > > > > > > > > 2.21.0
> > > > > > > > > >
> > > > > > > > > >



-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep

Re: LSM9DS1 testing with st_lsm6dsx driver

[Jonathan Cameron]

On Tue, 24 Sep 2019 20:55:09 +0200
Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> wrote:

> >  
> > > > > > > > > > > LSM9DS1 does not support hw FIFO for the moment. Are you trying to enable
> > > > > > > > > > > buffered reading?  
> > > > > > > > > >
> > > > > > > > > > I am not manually interacting with the device at all when this
> > > > > > > > > > exception occurs. This happens during the driver probe.
> > > > > > > > > >  
> > > > > > > > > > > Could you please try if the following patch helps? (just compiled)  
> > > > > > > > > >
> > > > > > > > > > I no longer receive the exception with this patch and it makes sense, thanks.  
> > > > > > > > >
> > > > > > > > > Hi Bobby,
> > > > > > > > >
> > > > > > > > > thx a lot for testing. Could you please try to drop the previous patch and
> > > > > > > > > apply the following one? Does it fix the issue as well?  
> > > > > > > >
> > > > > > > > No problem, happy to help. I just tested, and unfortunately the issue
> > > > > > > > is still present with this patch.  
> > > > > > >
> > > > > > > re-looking at the code this patch will actually does not anything since in the
> > > > > > > current implementation st_lsm6dsx_handler_irq will return IRQ_NONE and the
> > > > > > > thread handler will not be run. Anyway since st_lsm6dsx_handler_irq will be
> > > > > > > removed soon I will respin this patch ontop of Sean series.
> > > > > > >  
> > > > > > > > I gave the datasheet and the hardware reference manual of the
> > > > > > > > connected CPU a closer look and suspected a problem with my pin
> > > > > > > > config. However even after various combinations of pull ups/downs and
> > > > > > > > IRQ_TYPE_LEVEL_LOW/IRQ_TYPE_LEVEL_HIGH the same exception occurs, so I
> > > > > > > > don't think that there's an issue with pin config stopping the
> > > > > > > > interrupt line from being deasserted.  
> > > > > > >
> > > > > > > are you able to monitor the line activity through an oscilloscope?
> > > > > > > The issue is the irq line is never asserted and the kernel complains about
> > > > > > > lot of interrupts not managed
> > > > > > >
> > > > > > > Looking at the datasheet, LSM9DS1 does not support IRQ_TYPE_LEVEL_LOW.
> > > > > > > ST_LSM6DSX_REG_HLACTIVE_ADDR and ST_LSM6DSX_REG_PP_OD_ADDR registers are
> > > > > > > actually not defined for LSM9DS1 and I will move them in hw_settings map.  
> > > > > >
> > > > > > The datasheet I have for the LSM9DS1 shows a HLACTIVE bit in the
> > > > > > CTRL_REG8 (0x22) register, I thought that indicated support for both
> > > > > > high and low IRQ types.  
> > > > >
> > > > > Yes, right. I will update the series. Looking at the datasheet even
> > > > > the BDU register definition seems wrong. I will fix it as well.
> > > > >  
> > > > > > Either way, after applying your recent (Sep 22nd) patches to this
> > > > > > driver and changing my device tree node I have no kernel exception and
> > > > > > the iio device enumerates as normal. For anyone curious my device tree
> > > > > > node is now:
> > > > > >  
> > > > >
> > > > > What happen if you set the irq line active high? Does the issue occur?  
> > > >
> > > > No issue with IRQ_TYPE_LEVEL_HIGH either. Although all I'm doing at
> > > > this point to test is checking for the exception in the kernel prints
> > > > and reading the /sys/bus/iio/devices/iio:device*/in*raw of the device.  
> > >
> > > Ok, thx a lot for testing. Could you please try to understand which
> > > patch of the series fix the issue?
> > > In particular, could you please try to apply just '[PATCH 3/3] iio:
> > > imu: st_lsm6dsx: add sanity check for read_fifo pointer' and double
> > > check if the issue occurs?  
> >
> > No problem. It looks like that was the one, only having that patch
> > applied works fine.  
> 
> Ok, cool. Thx a lot for testing.
> 
> @Jonathan: how do you prefer to proceed? Push just this patch to
> 'togreg' branch and rebase the other fixes ontop of Sean's series or
> merge all the fixes now? (they will conflict with Sean's series)
Assuming I don't find any issues in Sean's latest, I'll apply that
first.  Then we put this on top but also look at a backport fix
to get this into stable after the next merge window.

It's always tricky to deal with these cases when a driver is
in a state of fairly rapid change.

I'll take a proper look at patch 3 if that is the only one
'necessary'.  It looks small so we might just push the fix
along side Sean's patch and let everyone concerned know that
a merge conflict is expected by trivial.

Thanks,

Jonathan

> 
> Regards,
> Lorenzo
> 
> >
> > Thanks,
> > Bobby Jones
> >  
> > >
> > > Regards,
> > > Lorenzo
> > >  
> > > >
> > > > Thanks,
> > > > Bobby Jones
> > > >  
> > > > >
> > > > > Regards,
> > > > > Lorenzo
> > > > >  
> > > > > > lsm9ds1_ag@6a {
> > > > > >   compatible = "st,lsm9ds1-imu";
> > > > > >   reg = <0x6a>;
> > > > > >   st,drdy-int-pin = <1>;
> > > > > >   pinctrl-names = "default";
> > > > > >   pinctrl-0 = <&pinctrl_acc_gyro>;
> > > > > >   interrupt-parent = <&gpio7>;
> > > > > >   interrupts = <13 IRQ_TYPE_EDGE_RISING>;
> > > > > > };
> > > > > >  
> > > > > > >
> > > > > > > @Jonathan: do you prefer this patch to be ontop of Sean's series?
> > > > > > >
> > > > > > > Regards,
> > > > > > > Lorenzo
> > > > > > >  
> > > > > > > >  
> > > > > > > > >
> > > > > > > > > iio: imu: st_lsm6dsx: check read_fifo pointer in st_lsm6dsx_handler_thread
> > > > > > > > >
> > > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > > > ---
> > > > > > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 3 +++
> > > > > > > > >  1 file changed, 3 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > > > > > index b0f3da1976e4..f4fd4842bd79 100644
> > > > > > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > > > > > @@ -666,6 +666,9 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> > > > > > > > >         struct st_lsm6dsx_hw *hw = private;
> > > > > > > > >         int count;
> > > > > > > > >
> > > > > > > > > +       if (!hw->settings->fifo_ops.read_fifo)
> > > > > > > > > +               return IRQ_NONE;
> > > > > > > > > +
> > > > > > > > >         mutex_lock(&hw->fifo_lock);
> > > > > > > > >         count = hw->settings->fifo_ops.read_fifo(hw);
> > > > > > > > >         mutex_unlock(&hw->fifo_lock);
> > > > > > > > > --
> > > > > > > > > 2.21.0
> > > > > > > > >  
> > > > > > > > > >
> > > > > > > > > > For context I'm working with a board that has every data ready and
> > > > > > > > > > interrupt signal connected to the LSM9DS1. Could you clarify what the
> > > > > > > > > > proper usage of the "st,drdy-int-pin" would be in this case and
> > > > > > > > > > whether or not I need more than one interrupt called out in my device
> > > > > > > > > > tree node?
> > > > > > > > > > I'm not really understanding how they're currently being utilized for
> > > > > > > > > > this device in the driver.  
> > > > > > > > >
> > > > > > > > > For the moment irq line in lsm9ds1 (acc/gyro) is not used at all,
> > > > > > > > > so you can omit the "st,drdy-int-pin" property
> > > > > > > > >  
> > > > > > > > > >
> > > > > > > > > > Also, I know support for this device was added recently and the combo
> > > > > > > > > > device hardware FIFO is complex, but is support for this something
> > > > > > > > > > that's currently being worked on?  
> > > > > > > > >
> > > > > > > > > It is actually in my ToDo list but I have no this device at the moment, so
> > > > > > > > > patches are welcome :)
> > > > > > > > >
> > > > > > > > > Regards,
> > > > > > > > > Lorenzo
> > > > > > > > >  
> > > > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > > Bobby Jones
> > > > > > > > > >  
> > > > > > > > > > > Regards,
> > > > > > > > > > > Lorenzo
> > > > > > > > > > >
> > > > > > > > > > > iio: imu: st_lsm6dsx: do not configure the fifo if not supported
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +-
> > > > > > > > > > >  1 file changed, 1 insertion(+), 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 b65a6ca775e0..90a0e5ce44e5 100644
> > > > > > > > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > > > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > > > > > > @@ -1572,7 +1572,7 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
> > > > > > > > > > >                         return err;
> > > > > > > > > > >         }
> > > > > > > > > > >
> > > > > > > > > > > -       if (hw->irq > 0) {
> > > > > > > > > > > +       if (hw->irq > 0 && hw->settings->fifo_ops.update_fifo) {
> > > > > > > > > > >                 err = st_lsm6dsx_fifo_setup(hw);
> > > > > > > > > > >                 if (err < 0)
> > > > > > > > > > >                         return err;
> > > > > > > > > > > --
> > > > > > > > > > > 2.21.0
> > > > > > > > > > >
> > > > > > > > > > >  
> 
> 
> 

Re: LSM9DS1 testing with st_lsm6dsx driver

[Lorenzo Bianconi]

>
> > @Jonathan: how do you prefer to proceed? Push just this patch to
> > 'togreg' branch and rebase the other fixes ontop of Sean's series or
> > merge all the fixes now? (they will conflict with Sean's series)
> Assuming I don't find any issues in Sean's latest, I'll apply that
> first.  Then we put this on top but also look at a backport fix
> to get this into stable after the next merge window.

Ack, please hold on with this series since I want to post a new one
fixing missing bits for lsm9ds1.
I will rebase it ontop of Sean's series

Regards,
Lorenzo

>
> It's always tricky to deal with these cases when a driver is
> in a state of fairly rapid change.
>
> I'll take a proper look at patch 3 if that is the only one
> 'necessary'.  It looks small so we might just push the fix
> along side Sean's patch and let everyone concerned know that
> a merge conflict is expected by trivial.

Yes, patch 3/3 is the only one necessary to fix the issue.

>
> Thanks,
>
> Jonathan
>
> >
> > Regards,
> > Lorenzo
> >
> > >
> > > Thanks,
> > > Bobby Jones
> > >
> > > >
> > > > Regards,
> > > > Lorenzo
> > > >
> > > > >
> > > > > Thanks,
> > > > > Bobby Jones
> > > > >
> > > > > >
> > > > > > Regards,
> > > > > > Lorenzo
> > > > > >
> > > > > > > lsm9ds1_ag@6a {
> > > > > > >   compatible = "st,lsm9ds1-imu";
> > > > > > >   reg = <0x6a>;
> > > > > > >   st,drdy-int-pin = <1>;
> > > > > > >   pinctrl-names = "default";
> > > > > > >   pinctrl-0 = <&pinctrl_acc_gyro>;
> > > > > > >   interrupt-parent = <&gpio7>;
> > > > > > >   interrupts = <13 IRQ_TYPE_EDGE_RISING>;
> > > > > > > };
> > > > > > >
> > > > > > > >
> > > > > > > > @Jonathan: do you prefer this patch to be ontop of Sean's series?
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > > Lorenzo
> > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > iio: imu: st_lsm6dsx: check read_fifo pointer in st_lsm6dsx_handler_thread
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 3 +++
> > > > > > > > > >  1 file changed, 3 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > > > > > > index b0f3da1976e4..f4fd4842bd79 100644
> > > > > > > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > > > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > > > > > > > > @@ -666,6 +666,9 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> > > > > > > > > >         struct st_lsm6dsx_hw *hw = private;
> > > > > > > > > >         int count;
> > > > > > > > > >
> > > > > > > > > > +       if (!hw->settings->fifo_ops.read_fifo)
> > > > > > > > > > +               return IRQ_NONE;
> > > > > > > > > > +
> > > > > > > > > >         mutex_lock(&hw->fifo_lock);
> > > > > > > > > >         count = hw->settings->fifo_ops.read_fifo(hw);
> > > > > > > > > >         mutex_unlock(&hw->fifo_lock);
> > > > > > > > > > --
> > > > > > > > > > 2.21.0
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > For context I'm working with a board that has every data ready and
> > > > > > > > > > > interrupt signal connected to the LSM9DS1. Could you clarify what the
> > > > > > > > > > > proper usage of the "st,drdy-int-pin" would be in this case and
> > > > > > > > > > > whether or not I need more than one interrupt called out in my device
> > > > > > > > > > > tree node?
> > > > > > > > > > > I'm not really understanding how they're currently being utilized for
> > > > > > > > > > > this device in the driver.
> > > > > > > > > >
> > > > > > > > > > For the moment irq line in lsm9ds1 (acc/gyro) is not used at all,
> > > > > > > > > > so you can omit the "st,drdy-int-pin" property
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Also, I know support for this device was added recently and the combo
> > > > > > > > > > > device hardware FIFO is complex, but is support for this something
> > > > > > > > > > > that's currently being worked on?
> > > > > > > > > >
> > > > > > > > > > It is actually in my ToDo list but I have no this device at the moment, so
> > > > > > > > > > patches are welcome :)
> > > > > > > > > >
> > > > > > > > > > Regards,
> > > > > > > > > > Lorenzo
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Thanks,
> > > > > > > > > > > Bobby Jones
> > > > > > > > > > >
> > > > > > > > > > > > Regards,
> > > > > > > > > > > > Lorenzo
> > > > > > > > > > > >
> > > > > > > > > > > > iio: imu: st_lsm6dsx: do not configure the fifo if not supported
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > > > > > > ---
> > > > > > > > > > > >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +-
> > > > > > > > > > > >  1 file changed, 1 insertion(+), 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 b65a6ca775e0..90a0e5ce44e5 100644
> > > > > > > > > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > > > > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > > > > > > > @@ -1572,7 +1572,7 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
> > > > > > > > > > > >                         return err;
> > > > > > > > > > > >         }
> > > > > > > > > > > >
> > > > > > > > > > > > -       if (hw->irq > 0) {
> > > > > > > > > > > > +       if (hw->irq > 0 && hw->settings->fifo_ops.update_fifo) {
> > > > > > > > > > > >                 err = st_lsm6dsx_fifo_setup(hw);
> > > > > > > > > > > >                 if (err < 0)
> > > > > > > > > > > >                         return err;
> > > > > > > > > > > > --
> > > > > > > > > > > > 2.21.0
> > > > > > > > > > > >
> > > > > > > > > > > >
> >
> >
> >
>


-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep