Messages in this thread | | | Date | Tue, 11 Jan 2022 00:43:25 +0200 | Subject | Re: [PATCH v3 2/2] iio: accel: add ADXL367 driver | From | 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 > >
| |