lkml.org 
[lkml]   [2022]   [Jan]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3 2/2] iio: accel: add ADXL367 driver
From


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

\
 
 \ /
  Last update: 2022-01-10 23:44    [W:0.185 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site