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

[Cosmin Tanislav <demonsingur@gmail.com>]

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
> 
>