Re: [PATCH v6 5/6] iio: imu: st_lsm6dsx: add motion report function and call from interrupt

From: Jonathan Cameron <jic23@kernel.org>
To: Sean Nyekjaer <sean@geanix.com>
Cc: linux-iio@vger.kernel.org, lorenzo.bianconi83@gmail.com,
	denis.ciocca@st.com, mario.tesi@st.com, armando.visconti@st.com,
	martin@geanix.com
Subject: Re: [PATCH v6 5/6] iio: imu: st_lsm6dsx: add motion report function and call from interrupt
Date: Sun, 15 Sep 2019 14:06:12 +0100	[thread overview]
Message-ID: <20190915140612.06791f31@archlinux> (raw)
In-Reply-To: <c912fbff-8277-cd99-515e-dd4239f8e220@geanix.com>

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