Re: [PATCH v3 2/2] iio: accel: add ADXL367 driver

[Cosmin Tanislav]

On 12/28/21 22:58, Jonathan Cameron wrote:
> Hi Cosmin,
>
> Happy New year for a few day's time.
>
> > > ...
> > >    
> > > > +
> > > > +static bool adxl367_push_event(struct iio_dev *indio_dev, u8 status)
> > > > +{
> > > > +	unsigned int ev_dir;
> > > > +
> > > > +	if (FIELD_GET(ADXL367_STATUS_ACT_MASK, status))
> > > > +		ev_dir = IIO_EV_DIR_RISING;
> > > > +	else if (FIELD_GET(ADXL367_STATUS_INACT_MASK, status))
> > > > +		ev_dir = IIO_EV_DIR_FALLING;
> > > > +	else
> > > > +		return false;
> > > > +
> > > > +	iio_push_event(indio_dev,
> > > > +		       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > IIO_MOD_X_OR_Y_OR_Z,
> > > > +					  IIO_EV_TYPE_THRESH, ev_dir),
> > > This is unusual for event detection as it's a simple or of separately
> > > applied thresholds on X, Y and Z axes.  Given the effect of gravity that
> > > means you have to set the thresholds very wide.
> > >
> > > Also, I'd expect these to be magnitudes, not THRESH - no data sheet that
> > > I can find though so can't be sure.
> > >    
> >
> > Actually, the chip has a referenced, and an absolute mode. We use reference mode
> > in this driver, as configured in write_event_config.
> > The motion detection details are about the same as ADXL362 (page 14).
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADXL362.pdf
>
> Interesting.  We should figure out some way to make that clear to userspace
> given right now it has no way of knowing that and might set inappropriate limits
> without that information.

Any suggestions on how I should do this?

> It's kind of similar to some of the adaptive thresholds, just that it uses
> the value at a particular moment.
>
> Worth noting that for the adxl362 at least the maths is
> ABS(Acceleration - reference) > Threshold which is a magnitude not a threshold
> unless you want to represent it as a pair of thresholds (above and below) which
> gets fiddly as I assume there is only one control

Indeed. I didn't catch onto the difference between magnitude and
threshold. So, I should use IIO_EV_TYPE_MAG rather than
IIO_EV_TYPE_THRESH? Or IIO_EV_TYPE_MAG_ADAPTIVE? The ABI doesn't
describe these too well.

> > > > +		       iio_get_time_ns(indio_dev));
> > > > +
> > > > +	return true;
> > > > +}
>
> ...
>
> > > > +static int adxl367_write_event_config(struct iio_dev *indio_dev,
> > > > +				      const struct iio_chan_spec *chan,
> > > > +				      enum iio_event_type type,
> > > > +				      enum iio_event_direction dir,
> > > > +				      int state)
> > > > +{
> > > > +	struct adxl367_state *st = iio_priv(indio_dev);
> > > > +	enum adxl367_activity_type act;
> > > > +	int ret;
> > > > +
> > > > +	switch (dir) {
> > > > +	case IIO_EV_DIR_RISING:
> > > > +		act = ADXL367_ACTIVITY;
> > > > +		break;
> > > > +	case IIO_EV_DIR_FALLING:
> > > > +		act = ADXL367_INACTIVITY;
> > > > +		break;
> > > > +	default:
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	ret = iio_device_claim_direct_mode(indio_dev);
> > >
> > > It's unusual (though not unheard of) to have events that cannot be enabled
> > > at the same time as a fifo.  If that's true here, please add some comments
> > > to explain why.  Or is this just about the impact of having to disable
> > > the measurement to turn it on and the resulting interruption of data
> > > capture?
> > >
> > > If so that needs more thought as we have a situation where you can (I think)
> > > have events as long as you enable them before the fifo based capture is
> > > started,
> > > but cannot enable them after.
> > >    
> >
> > That is indeed the case. You mentioned in a previous patchset that various
> > attributes could toggle measurement mode while the FIFO capture was running,
> > so I checked all the possible places where that could happen and added claim
> > direct mode. Not too nice, but it's the nature of the chip...
>
> Hmm. I'm not sure what the right thing to do here is. Maybe we need a docs update
> to explicitly call out that this might happen for the event enables?  Calling
> it out for all devices is fine because all we are doing is saying userspace would
> ideally cope with this situation and make the decision to disable the buffered
> mode if it wants to enable events then reenable it afterwards if that is what
> is desired.

By docs you mean the ABI file?

> Jonathan