[v6,5/6] iio: imu: st_lsm6dsx: add motion report function and call from interrupt
diff mbox series

Message ID 20190909112846.55280-5-sean@geanix.com
State New
Headers show
Series
  • [v6,1/6] iio: imu: st_lsm6dsx: move interrupt thread to core
Related show

Commit Message

Sean Nyekjaer Sept. 9, 2019, 11:28 a.m. UTC
Report iio motion events to iio subsystem

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
Changes since v4:
 * Updated bitmask as pr Jonathans comments

Changes since v5:
 * None

 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  5 ++
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 70 ++++++++++++++++++++
 2 files changed, 75 insertions(+)

Comments

Sean Nyekjaer Sept. 9, 2019, 11:51 a.m. UTC | #1
On 09/09/2019 13.28, Sean Nyekjaer wrote:
> Report iio motion events to iio subsystem
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> Changes since v4:
>   * Updated bitmask as pr Jonathans comments
> 
> Changes since v5:
>   * None
> 
>   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  5 ++
>   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 70 ++++++++++++++++++++
>   2 files changed, 75 insertions(+)
> 
[...]
>   
> +void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data)
> +{
> +	s64 timestamp = iio_get_time_ns(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
> +
> +	if (data & hw->settings->event_settings.wakeup_src_z_mask)
> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> +						  0,
> +						  IIO_MOD_Z,
> +						  IIO_EV_TYPE_THRESH,
> +						  IIO_EV_DIR_EITHER),
> +						  timestamp);
> +
> +	if (data & hw->settings->event_settings.wakeup_src_x_mask)
> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> +						  0,
> +						  IIO_MOD_Y,
> +						  IIO_EV_TYPE_THRESH,
> +						  IIO_EV_DIR_EITHER),
> +						  timestamp);
> +
> +	if (data & hw->settings->event_settings.wakeup_src_x_mask)
> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> +						  0,
> +						  IIO_MOD_X,
> +						  IIO_EV_TYPE_THRESH,
> +						  IIO_EV_DIR_EITHER),
> +						  timestamp);
> +}
> +

I was looking at this again, and if the user enables events for channel 
x, we continue to report events for y, z.
Is it okay or is it better to filter them out?

/Sean
Lorenzo Bianconi Sept. 9, 2019, 12:05 p.m. UTC | #2
> Report iio motion events to iio subsystem
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> Changes since v4:
>  * Updated bitmask as pr Jonathans comments
> 
> Changes since v5:
>  * None
> 
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  5 ++
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 70 ++++++++++++++++++++
>  2 files changed, 75 insertions(+)
> 

[...]

>  static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
>  {
>  	return IRQ_WAKE_THREAD;
> @@ -1668,6 +1726,18 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
>  {
>  	struct st_lsm6dsx_hw *hw = private;
>  	int count;
> +	int data, err;
> +
> +	if (hw->enable_event) {


Maybe I understood the issue between the buffered reading and event generation.
I guess it is a race here between when the device is generating the interrupt
and when you set enable_event. I think there are two solutions:
1- trivial one: always read wakeup_src_reg
2- set hw->enable_event as first instruction in st_lsm6dsx_write_event_config()
and roll back in case of error.

Could you please try that changes and double check if you are still able to
trigger the issue?

Regards,
Lorenzo

> +		err = regmap_read(hw->regmap,
> +				  hw->settings->event_settings.wakeup_src_reg,
> +				  &data);
> +		if (err < 0)
> +			return IRQ_NONE;
> +
> +		if (data & hw->settings->event_settings.wakeup_src_status_mask)
> +			st_lsm6dsx_report_motion_event(hw, data);
> +	}
>  
>  	mutex_lock(&hw->fifo_lock);
>  	count = hw->settings->fifo_ops.read_fifo(hw);
> -- 
> 2.23.0
>
Sean Nyekjaer Sept. 9, 2019, 12:41 p.m. UTC | #3
On 09/09/2019 14.05, Lorenzo Bianconi wrote:
>>   static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
>>   {
>>   	return IRQ_WAKE_THREAD;
>> @@ -1668,6 +1726,18 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
>>   {
>>   	struct st_lsm6dsx_hw *hw = private;
>>   	int count;
>> +	int data, err;
>> +
>> +	if (hw->enable_event) {
> 
> Maybe I understood the issue between the buffered reading and event generation.
> I guess it is a race here between when the device is generating the interrupt
> and when you set enable_event. I think there are two solutions:
> 1- trivial one: always read wakeup_src_reg
> 2- set hw->enable_event as first instruction in st_lsm6dsx_write_event_config()
> and roll back in case of error.
> 
> Could you please try that changes and double check if you are still able to
> trigger the issue?
> 

1. Without the last [PATCH v6 6/6] and this diff:

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c 
b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index bd5c7c65a519..b303459e0d31 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1728,7 +1728,6 @@ static irqreturn_t st_lsm6dsx_handler_thread(int 
irq, void *private)
         int count;
         int data, err;

-       if (hw->enable_event) {
                 err = regmap_read(hw->regmap,
 
hw->settings->event_settings.wakeup_src_reg,
                                   &data);
@@ -1737,7 +1736,6 @@ static irqreturn_t st_lsm6dsx_handler_thread(int 
irq, void *private)

                 if (data & 
hw->settings->event_settings.wakeup_src_status_mask)
                         st_lsm6dsx_report_motion_event(hw, data);
-       }

         mutex_lock(&hw->fifo_lock);
         count = hw->settings->fifo_ops.read_fifo(hw);

$ cd /sys/bus/iio/devices/iio:device2
$ echo 1 > events/in_accel_x_thresh_either_en
$ echo 1 > events/in_accel_x_thresh_either_value
$ echo 1 > scan_elements/in_accel_x_en
$ echo 1 > buffer/enable

FIFO interrupts ticking in... until I trigger the first event. :-(
The event is reported correctly. The interrupt pin is staying high.
The result is the same if I enable the FIFO first.
I don't think we have a race in the driver around this, to me it looks 
like something in the ism330 device should be cleared.
Could the device go into sleep or power down mode?

2. Seems like an okay idea, do you want this in v7?

/Sean
Lorenzo Bianconi Sept. 10, 2019, 7:12 a.m. UTC | #4
> > Maybe I understood the issue between the buffered reading and event generation.
> > I guess it is a race here between when the device is generating the interrupt
> > and when you set enable_event. I think there are two solutions:
> > 1- trivial one: always read wakeup_src_reg
> > 2- set hw->enable_event as first instruction in st_lsm6dsx_write_event_config()
> > and roll back in case of error.
> > 
> > Could you please try that changes and double check if you are still able to
> > trigger the issue?
> > 

[...]

> $ cd /sys/bus/iio/devices/iio:device2
> $ echo 1 > events/in_accel_x_thresh_either_en
> $ echo 1 > events/in_accel_x_thresh_either_value
> $ echo 1 > scan_elements/in_accel_x_en
> $ echo 1 > buffer/enable
> 
> FIFO interrupts ticking in... until I trigger the first event. :-(
> The event is reported correctly. The interrupt pin is staying high.
> The result is the same if I enable the FIFO first.
> I don't think we have a race in the driver around this, to me it looks like
> something in the ism330 device should be cleared.
> Could the device go into sleep or power down mode?

probably a silly question..are you tracing the interrupt line with an
oscilloscope or a logical analyser? If you dump interrupt counters in
/proc/interrupts will you see an interrupt storm for the selected irq
pin?

Regards,
Lorenzo

> 
> 2. Seems like an okay idea, do you want this in v7?
> 
> /Sean
Sean Nyekjaer Sept. 10, 2019, 7:17 a.m. UTC | #5
On 10/09/2019 09.12, Lorenzo Bianconi wrote:
> probably a silly question..are you tracing the interrupt line with an
> oscilloscope or a logical analyser? If you dump interrupt counters in
> /proc/interrupts will you see an interrupt storm for the selected irq
> pin?
> 

Not a silly question ;-)

An Oscilloscope :-)
The interrupt counter is stopping.
If I switch to IRQ_TYPE_LEVEL_HIGH, (to test if additional readings of 
the event and FIFO registers would help. It results in interrupt storm 
and the line continues to stay high.

/Sean
Lorenzo Bianconi Sept. 10, 2019, 7:26 a.m. UTC | #6
> 
> 
> On 10/09/2019 09.12, Lorenzo Bianconi wrote:
> > probably a silly question..are you tracing the interrupt line with an
> > oscilloscope or a logical analyser? If you dump interrupt counters in
> > /proc/interrupts will you see an interrupt storm for the selected irq
> > pin?
> > 
> 
> Not a silly question ;-)
> 
> An Oscilloscope :-)

ack, thx

Could you please try to carry out the following test?
1- set the FIFO watermark to a high level (e.g. 128)
   $echo 256 > /sys/bus/iio/devices/iio:device{x}/buffer/length
   $echo 128 > /sys/bus/iio/devices/iio:device{x}/buffer/watermark
2- set a low acc odr (e.g 13Hz)
   $echo 13 > /sys/bus/iio/devices/iio:device{x}/sampling_frequency
3- start reading from the accel and generate a wake-upp event

Is still happen? Are you able to decode bus transaction? (register addresses,
data read, ..)

> The interrupt counter is stopping.
> If I switch to IRQ_TYPE_LEVEL_HIGH, (to test if additional readings of the
> event and FIFO registers would help. It results in interrupt storm and the
> line continues to stay high.
> 

@Denis, @Mario, @Armando: any thoughts about this?

Regards,
Lorenzo

> /Sean
Lorenzo Bianconi Sept. 10, 2019, 12:27 p.m. UTC | #7
> > > probably a silly question..are you tracing the interrupt line with an
> > > oscilloscope or a logical analyser? If you dump interrupt counters in
> > > /proc/interrupts will you see an interrupt storm for the selected irq
> > > pin?
> > > 
> > 
> > Not a silly question ;-)
> > 
> > An Oscilloscope :-)
> 
> ack, thx
> 
> Could you please try to carry out the following test?
> 1- set the FIFO watermark to a high level (e.g. 128)
>    $echo 256 > /sys/bus/iio/devices/iio:device{x}/buffer/length
>    $echo 128 > /sys/bus/iio/devices/iio:device{x}/buffer/watermark
> 2- set a low acc odr (e.g 13Hz)
>    $echo 13 > /sys/bus/iio/devices/iio:device{x}/sampling_frequency
> 3- start reading from the accel and generate a wake-upp event
> 
> Is still happen? Are you able to decode bus transaction? (register addresses,
> data read, ..)
> 
> > The interrupt counter is stopping.
> > If I switch to IRQ_TYPE_LEVEL_HIGH, (to test if additional readings of the
> > event and FIFO registers would help. It results in interrupt storm and the
> > line continues to stay high.
> > 

Could you please try to enable the LIR (Latched interrupt - BIT(0) in 0x58)?
Please remember that on ISM330DLC the interrupt will be automatically cleared
reading the wake up src register after a time slice equals to 1/ODR so the it
will be set for longer time if you run the device at low ODR

Regards,
Lorenzo

> 
> @Denis, @Mario, @Armando: any thoughts about this?
> 
> Regards,
> Lorenzo
> 
> > /Sean
Sean Nyekjaer Sept. 11, 2019, 12:41 p.m. UTC | #8
On 10/09/2019 14.27, Lorenzo Bianconi wrote:
>> Could you please try to carry out the following test?
>> 1- set the FIFO watermark to a high level (e.g. 128)
>>     $echo 256 > /sys/bus/iio/devices/iio:device{x}/buffer/length
>>     $echo 128 > /sys/bus/iio/devices/iio:device{x}/buffer/watermark
>> 2- set a low acc odr (e.g 13Hz)
>>     $echo 13 > /sys/bus/iio/devices/iio:device{x}/sampling_frequency
>> 3- start reading from the accel and generate a wake-upp event
>>
>> Is still happen? Are you able to decode bus transaction? (register addresses,
>> data read, ..)
>>

Do you still want this tested?

[...]

> Could you please try to enable the LIR (Latched interrupt - BIT(0) in 0x58)?
> Please remember that on ISM330DLC the interrupt will be automatically cleared
> reading the wake up src register after a time slice equals to 1/ODR so the it
> will be set for longer time if you run the device at low ODR


"iio: imu: st_lsm6dsx: enable LIR for sensor events"
"iio: imu: st_lsm6dsx: enable clear on read for latched interrupts"
Does allow us to get events and buffered reads simultaneously.

I will drop PATCH 6/6.

/Sean
Lorenzo Bianconi Sept. 11, 2019, 12:59 p.m. UTC | #9
On Sep 11, Sean Nyekjaer wrote:
> 
> On 10/09/2019 14.27, Lorenzo Bianconi wrote:
> > > Could you please try to carry out the following test?
> > > 1- set the FIFO watermark to a high level (e.g. 128)
> > >     $echo 256 > /sys/bus/iio/devices/iio:device{x}/buffer/length
> > >     $echo 128 > /sys/bus/iio/devices/iio:device{x}/buffer/watermark
> > > 2- set a low acc odr (e.g 13Hz)
> > >     $echo 13 > /sys/bus/iio/devices/iio:device{x}/sampling_frequency
> > > 3- start reading from the accel and generate a wake-upp event
> > > 
> > > Is still happen? Are you able to decode bus transaction? (register addresses,
> > > data read, ..)
> > > 
> 
> Do you still want this tested?

no, not necessary

> 
> [...]
> 
> > Could you please try to enable the LIR (Latched interrupt - BIT(0) in 0x58)?
> > Please remember that on ISM330DLC the interrupt will be automatically cleared
> > reading the wake up src register after a time slice equals to 1/ODR so the it
> > will be set for longer time if you run the device at low ODR
> 
> 
> "iio: imu: st_lsm6dsx: enable LIR for sensor events"
> "iio: imu: st_lsm6dsx: enable clear on read for latched interrupts"
> Does allow us to get events and buffered reads simultaneously.
> 

cool :)

Regards,
Lorenzo

> I will drop PATCH 6/6.
> 
> /Sean
Jonathan Cameron Sept. 15, 2019, 12:20 p.m. UTC | #10
On Mon, 9 Sep 2019 13:51:13 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> On 09/09/2019 13.28, Sean Nyekjaer wrote:
> > Report iio motion events to iio subsystem
> > 
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > ---
> > Changes since v4:
> >   * Updated bitmask as pr Jonathans comments
> > 
> > Changes since v5:
> >   * None
> > 
> >   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  5 ++
> >   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 70 ++++++++++++++++++++
> >   2 files changed, 75 insertions(+)
> >   
> [...]
> >   
> > +void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data)
> > +{
> > +	s64 timestamp = iio_get_time_ns(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
> > +
> > +	if (data & hw->settings->event_settings.wakeup_src_z_mask)
> > +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> > +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> > +						  0,
> > +						  IIO_MOD_Z,
> > +						  IIO_EV_TYPE_THRESH,
> > +						  IIO_EV_DIR_EITHER),
> > +						  timestamp);
> > +
> > +	if (data & hw->settings->event_settings.wakeup_src_x_mask)
> > +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> > +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> > +						  0,
> > +						  IIO_MOD_Y,
> > +						  IIO_EV_TYPE_THRESH,
> > +						  IIO_EV_DIR_EITHER),
> > +						  timestamp);
> > +
> > +	if (data & hw->settings->event_settings.wakeup_src_x_mask)
> > +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> > +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> > +						  0,
> > +						  IIO_MOD_X,
> > +						  IIO_EV_TYPE_THRESH,
> > +						  IIO_EV_DIR_EITHER),
> > +						  timestamp);
> > +}
> > +  
> 
> I was looking at this again, and if the user enables events for channel 
> x, we continue to report events for y, z.
> Is it okay or is it better to filter them out?
Better to filter them out.  It'll be a bit of a surprise for userspace
otherwise.

Thanks,

Jonathan

> 
> /Sean
Sean Nyekjaer Sept. 15, 2019, 12:24 p.m. UTC | #11
On 15/09/2019 14.20, Jonathan Cameron wrote:
> On Mon, 9 Sep 2019 13:51:13 +0200
> Sean Nyekjaer <sean@geanix.com> wrote:
> 
>> On 09/09/2019 13.28, Sean Nyekjaer wrote:
>>> Report iio motion events to iio subsystem
>>>
>>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>>> ---
>>> Changes since v4:
>>>    * Updated bitmask as pr Jonathans comments
>>>
>>> Changes since v5:
>>>    * None
>>>
>>>    drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  5 ++
>>>    drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 70 ++++++++++++++++++++
>>>    2 files changed, 75 insertions(+)
>>>    
>> [...]
>>>    
>>> +void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data)
>>> +{
>>> +	s64 timestamp = iio_get_time_ns(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
>>> +
>>> +	if (data & hw->settings->event_settings.wakeup_src_z_mask)
>>> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
>>> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
>>> +						  0,
>>> +						  IIO_MOD_Z,
>>> +						  IIO_EV_TYPE_THRESH,
>>> +						  IIO_EV_DIR_EITHER),
>>> +						  timestamp);
>>> +
>>> +	if (data & hw->settings->event_settings.wakeup_src_x_mask)
>>> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
>>> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
>>> +						  0,
>>> +						  IIO_MOD_Y,
>>> +						  IIO_EV_TYPE_THRESH,
>>> +						  IIO_EV_DIR_EITHER),
>>> +						  timestamp);
>>> +
>>> +	if (data & hw->settings->event_settings.wakeup_src_x_mask)
>>> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
>>> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
>>> +						  0,
>>> +						  IIO_MOD_X,
>>> +						  IIO_EV_TYPE_THRESH,
>>> +						  IIO_EV_DIR_EITHER),
>>> +						  timestamp);
>>> +}
>>> +
>>
>> I was looking at this again, and if the user enables events for channel
>> x, we continue to report events for y, z.
>> Is it okay or is it better to filter them out?
> Better to filter them out.  It'll be a bit of a surprise for userspace
> otherwise.
> 
> Thanks,
> 
> Jonathan
> 
Okay, but keep in mind that we can't distinguish which channel we're 
waking up to. So even if some channel is disabled, we still wake up on 
it ...

/Sean
Jonathan Cameron Sept. 15, 2019, 1:06 p.m. UTC | #12
On Sun, 15 Sep 2019 14:24:57 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> On 15/09/2019 14.20, Jonathan Cameron wrote:
> > On Mon, 9 Sep 2019 13:51:13 +0200
> > Sean Nyekjaer <sean@geanix.com> wrote:
> >   
> >> On 09/09/2019 13.28, Sean Nyekjaer wrote:  
> >>> Report iio motion events to iio subsystem
> >>>
> >>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> >>> ---
> >>> Changes since v4:
> >>>    * Updated bitmask as pr Jonathans comments
> >>>
> >>> Changes since v5:
> >>>    * None
> >>>
> >>>    drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  5 ++
> >>>    drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 70 ++++++++++++++++++++
> >>>    2 files changed, 75 insertions(+)
> >>>      
> >> [...]  
> >>>    
> >>> +void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data)
> >>> +{
> >>> +	s64 timestamp = iio_get_time_ns(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
> >>> +
> >>> +	if (data & hw->settings->event_settings.wakeup_src_z_mask)
> >>> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> >>> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> >>> +						  0,
> >>> +						  IIO_MOD_Z,
> >>> +						  IIO_EV_TYPE_THRESH,
> >>> +						  IIO_EV_DIR_EITHER),
> >>> +						  timestamp);
> >>> +
> >>> +	if (data & hw->settings->event_settings.wakeup_src_x_mask)
> >>> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> >>> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> >>> +						  0,
> >>> +						  IIO_MOD_Y,
> >>> +						  IIO_EV_TYPE_THRESH,
> >>> +						  IIO_EV_DIR_EITHER),
> >>> +						  timestamp);
> >>> +
> >>> +	if (data & hw->settings->event_settings.wakeup_src_x_mask)
> >>> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> >>> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> >>> +						  0,
> >>> +						  IIO_MOD_X,
> >>> +						  IIO_EV_TYPE_THRESH,
> >>> +						  IIO_EV_DIR_EITHER),
> >>> +						  timestamp);
> >>> +}
> >>> +  
> >>
> >> I was looking at this again, and if the user enables events for channel
> >> x, we continue to report events for y, z.
> >> Is it okay or is it better to filter them out?  
> > Better to filter them out.  It'll be a bit of a surprise for userspace
> > otherwise.
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> Okay, but keep in mind that we can't distinguish which channel we're 
> waking up to. So even if some channel is disabled, we still wake up on 
> it ...

Hmm. Alternative is to not filter it, but make sure that the enable
and disable interfaces treat all of them as one control.  In theory
I think the ABI would allow for events/in_accel_thresh_both_en in a similar
fashion to the shared_by_type of the info elements for the channel, but
given I'm not sure anything implements it, it's likely to be a hole
in userspace code.  We do have devices that do
in_accel_thresh_both_en when the events are rising and falling separately
but they two can't be enabled independently.  This is kind of similar
to that but on the channel rather than the direction.

Jonathan

> 
> /Sean

Patch
diff mbox series

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index d04473861fba..015b837f366f 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -186,6 +186,11 @@  struct st_lsm6dsx_shub_settings {
 struct st_lsm6dsx_event_settings {
 	struct st_lsm6dsx_reg enable_reg;
 	struct st_lsm6dsx_reg wakeup_reg;
+	u8 wakeup_src_reg;
+	u8 wakeup_src_status_mask;
+	u8 wakeup_src_z_mask;
+	u8 wakeup_src_y_mask;
+	u8 wakeup_src_x_mask;
 };
 
 enum st_lsm6dsx_ext_sensor_id {
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 5b8bcd9cc4d2..bd5c7c65a519 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -48,6 +48,7 @@ 
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/delay.h>
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/interrupt.h>
@@ -283,6 +284,11 @@  static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x5B,
 				.mask = GENMASK(5, 0),
 			},
+			.wakeup_src_reg = 0x1b,
+			.wakeup_src_status_mask = BIT(3),
+			.wakeup_src_z_mask = BIT(0),
+			.wakeup_src_y_mask = BIT(1),
+			.wakeup_src_x_mask = BIT(2),
 		},
 	},
 	{
@@ -404,6 +410,11 @@  static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x5B,
 				.mask = GENMASK(5, 0),
 			},
+			.wakeup_src_reg = 0x1b,
+			.wakeup_src_status_mask = BIT(3),
+			.wakeup_src_z_mask = BIT(0),
+			.wakeup_src_y_mask = BIT(1),
+			.wakeup_src_x_mask = BIT(2),
 		},
 	},
 	{
@@ -538,6 +549,11 @@  static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x5B,
 				.mask = GENMASK(5, 0),
 			},
+			.wakeup_src_reg = 0x1b,
+			.wakeup_src_status_mask = BIT(3),
+			.wakeup_src_z_mask = BIT(0),
+			.wakeup_src_y_mask = BIT(1),
+			.wakeup_src_x_mask = BIT(2),
 		},
 	},
 	{
@@ -788,6 +804,11 @@  static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x5B,
 				.mask = GENMASK(5, 0),
 			},
+			.wakeup_src_reg = 0x1b,
+			.wakeup_src_status_mask = BIT(3),
+			.wakeup_src_z_mask = BIT(0),
+			.wakeup_src_y_mask = BIT(1),
+			.wakeup_src_x_mask = BIT(2),
 		},
 	},
 	{
@@ -934,6 +955,11 @@  static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x5B,
 				.mask = GENMASK(5, 0),
 			},
+			.wakeup_src_reg = 0x1b,
+			.wakeup_src_status_mask = BIT(3),
+			.wakeup_src_z_mask = BIT(0),
+			.wakeup_src_y_mask = BIT(1),
+			.wakeup_src_x_mask = BIT(2),
 		}
 	},
 };
@@ -1659,6 +1685,38 @@  static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
 	return iio_dev;
 }
 
+void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data)
+{
+	s64 timestamp = iio_get_time_ns(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
+
+	if (data & hw->settings->event_settings.wakeup_src_z_mask)
+		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
+			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
+						  0,
+						  IIO_MOD_Z,
+						  IIO_EV_TYPE_THRESH,
+						  IIO_EV_DIR_EITHER),
+						  timestamp);
+
+	if (data & hw->settings->event_settings.wakeup_src_x_mask)
+		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
+			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
+						  0,
+						  IIO_MOD_Y,
+						  IIO_EV_TYPE_THRESH,
+						  IIO_EV_DIR_EITHER),
+						  timestamp);
+
+	if (data & hw->settings->event_settings.wakeup_src_x_mask)
+		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
+			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
+						  0,
+						  IIO_MOD_X,
+						  IIO_EV_TYPE_THRESH,
+						  IIO_EV_DIR_EITHER),
+						  timestamp);
+}
+
 static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
 {
 	return IRQ_WAKE_THREAD;
@@ -1668,6 +1726,18 @@  static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
 {
 	struct st_lsm6dsx_hw *hw = private;
 	int count;
+	int data, err;
+
+	if (hw->enable_event) {
+		err = regmap_read(hw->regmap,
+				  hw->settings->event_settings.wakeup_src_reg,
+				  &data);
+		if (err < 0)
+			return IRQ_NONE;
+
+		if (data & hw->settings->event_settings.wakeup_src_status_mask)
+			st_lsm6dsx_report_motion_event(hw, data);
+	}
 
 	mutex_lock(&hw->fifo_lock);
 	count = hw->settings->fifo_ops.read_fifo(hw);