> > > + return FXLS8962AF_SDCD_CONFIG1_X_OT_EN & data->enable_event;
> >
> > I would force these to be 1/0. The IIO core doesn't sanitise these values currently
> > (it's something we should perhaps think about, though I don't fancy auditing all the
> > users to check no one is doing anything strange through this interface).
> >
>
> So something like?
> return !!(FXLS8962AF_SDCD_CONFIG1_X_OT_EN & data->enable_event);
yup. That does the job.
>
> > > + case IIO_MOD_Y:
> > > + return FXLS8962AF_SDCD_CONFIG1_Y_OT_EN & data->enable_event;
> > > + case IIO_MOD_Z:
> > > + return FXLS8962AF_SDCD_CONFIG1_Z_OT_EN & data->enable_event;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
>
> > > + return 0;
> > > +
> > > + /* Enable events */
> > > + ret = regmap_write(data->regmap, FXLS8962AF_SDCD_CONFIG1, enable_event | 0x80);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /*
> > > + * Enable update of SDCD_REF_X/Y/Z values with the current decimated and
> > > + * trimmed X/Y/Z acceleration input data. This allows for acceleration
> > > + * slope detection with Data(n) to Data(n–1) always used as the input
> > > + * to the window comparator.
> >
> > This sounds like a ROC detector... If true the current description is wrong as you
> > need to use the IIO_EV_TYPE_ROC for that sort of event. Ideally also add the
> > absolute magnitude version, or at least clearly document that it is not currently
> > supported by the driver.
>
> What is a ROC event? Right now it works like the event in ism330dlc...
Rate of Change.
Hmm. We don't get these very often so I couldn't remember what the docs said wrt
to units. Seems it is rather vague which isn't great.
See Documentation/ABI/testing/sysfs-bus-iio and grep for roc_
Mind you this is getting close to the adaptive thresholds where pretty much anything
goes :)
>
> Where do you suggest the comment should be? Beside the event creation
> structs?
That would work. Just somewhere anyone wondering why it doesn't do what they want
would find it.
>
> >
> > The datasheet isn't exactly friendly for working this stuff out. *sigh*
>
> No it's not the best :/
>
> >
> > Whilst here, also looks like there is also a convenient vector magnitude option. Would be
> > good to support that.
>
> So support:
> - absolute magnitude
This one isn't quite true as I read it. Not sure how you would use the positive and negatives differently
though...
> - convenient vector magnitude
> - slope detection?
Could also (I think) do the in window interrupt as a falling magnitude.
Those tend to be used as freefall detectors which is always fun to test :)
>
> >
> > Also break that c up into the two parts that it contains (enable and REF_UPDM value) with suitable
> > definitions for the various options of REF_UPDM.
>
> Will do :)
>
> >
> >
> > > + */
> > > + ret = regmap_write(data->regmap, FXLS8962AF_SDCD_CONFIG2, enable_event ? 0xc0 : 0x00);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = fxls8962af_event_setup(data, state);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + data->enable_event = enable_event;
> > > +
> > > + if (data->enable_event) {
> > > + fxls8962af_active(data);
> > > + ret = fxls8962af_power_on(data);
> > > + } else {
> > > + if (!iio_buffer_enabled(indio_dev))
> >
> > This can (I think) race with buffer enabling. To avoid that, whilst doing this check
> > do a claim_direct_mode() and release it after turning the power off. That will
> > ensure consistency. There is a nasty corner case though if another function is
> > doing a claim_direct_mode() say due to a concurrent userspace access. In that case
> > this one will fail, but the buffer is not enabled and hence we won't power down.
> >
> > Hmm. Looking at what this power_off() is doing and the fact it's using runtime_pm,
> > you should be able to rely on the runtime_pm reference counting and not need this check.
> > In fact the presence of the power_on above without an iio_buffer_enabled() check
> > suggests to me this is currently unbalanced and you can end up with the power stuck
> > on permanently.
>
> Will try to add this and verify the functionality.
>
> >
> > The fact we even have separate config and value callbacks is a historical bit of mess
> > it would be nice to clean up one day (but is quite a way down the todo list!)
> >
> > > + ret = fxls8962af_power_off(data);
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static const struct iio_event_spec fxls8962af_event = {
> > > + .type = IIO_EV_TYPE_THRESH,
> > > + .dir = IIO_EV_DIR_EITHER,
> > > + .mask_separate = BIT(IIO_EV_INFO_VALUE) | BIT(IIO_EV_INFO_ENABLE),
> >
> > I'm missing something here. The thresholds seem to be independent...
>
> Sorry, I do not understand this. The event can be enabled per axis', but
> the threshold is common for all axis'.
this was mostly me figuring out that there were separate upper and lower thresholds.
So the enable is DIR_EITHER but the values are not.
>
> >
> > > +};
> > > +
> > > #define FXLS8962AF_CHANNEL(axis, reg, idx) { \
> > > .type = IIO_ACCEL, \
> > > .address = reg, \
> > > @@ -481,6 +690,8 @@ static int fxls8962af_set_watermark(struct iio_dev *indio_dev, unsigned val)
> > > .shift = 4, \
> > > .endianness = IIO_BE, \
> > > }, \
> > > + .event_spec = &fxls8962af_event, \
> > > + .num_event_specs = 1, \
> > > }
> > >
>
> [...]
>
> > > +static int fxls8962af_event_interrupt(struct iio_dev *indio_dev)
> > > +{
> > > + struct fxls8962af_data *data = iio_priv(indio_dev);
> > > + s64 ts = iio_get_time_ns(indio_dev);
> > > + unsigned int reg;
> > > + int ret;
> > > +
> > > + ret = regmap_read(data->regmap, FXLS8962AF_SDCD_INT_SRC1, ®);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (reg & FXLS8962AF_SDCD_INT_SRC1_X_OT)
> > > + iio_push_event(indio_dev,
> > > + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_X,
> > > + IIO_EV_TYPE_THRESH,
> > > + IIO_EV_DIR_EITHER),
> >
> > I was a bit surprised to see DIR_EITHER here so went looking at the datasheet.
> > Seems to me that we have X_OT_POL that lets us know which threshold was passed.
> > Can we use that here to give a more specific event?
> >
>
> Yes will add.
>
> Thank you for the review and comments Jonathan.
You are welcome.
J
>
> Br,
> /Sean
