Re: [PATCH v3 26/27] staging: iio: resolver: ad2s1210: implement fault events

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 2 Oct 2023 11:58:17 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On Sat, Sep 30, 2023 at 11:00 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 29 Sep 2023 12:23:31 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >  
> > > From: David Lechner <david@lechnology.com>
> > >
> > > From: David Lechner <dlechner@baylibre.com>
> > >
> > > When reading the position and velocity on the AD2S1210, there is also a
> > > 3rd byte following the two data bytes that contains the fault flag bits.
> > > This patch adds support for reading this byte and generating events when
> > > faults occur.
> > >
> > > The faults are mapped to various channels and event types in order to
> > > have a unique event for each fault.
> > >
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>  
> >
> > Use of x and y modifiers is a little odd.  What was your reasoning?
> > Was it just that there was a X_OR_Y modifier?  If so, don't use that!
> > It seemed like a good idea at the time, but it's not nice to deal with
> > and requires a channel with that modifier to hang the controls off
> > + make sure userspace expects that event code.  
> 
> 
> Regarding the point about "requires a channel with that modifier to
> hang the controls off...". Although that comment was about modifiers,
> does it also apply in general.
> 
> There are several fault events that don't have any configurable
> parameters, namely _sine/cosine inputs clipping_ and _velocity exceeds
> max tracking rate_. So there won't be any attributes that contain the
> event specification for those (e.g. no `events/in_angl0_*`
> attributes). It sounds like this would be a problem as well?

It's fine to have a channel that doesn't have controls or the ability
to be read.

We do have history of doing what you have here (a couple
of accelerometers do it) but it's esoteric and rather hard for userspace
to comprehend so I'd rather not introduce it for other types of devices.

I think we should go with the most flexible option of allowing
events to trigger when they 'may be true' to incorporate this case.
Unfortunately I can't see another option that would scale to all the
random combinations of events that might occur.  There are all sorts
of extensions we could make to the event descriptions, but only at the
cost of breaking backwards compatibility and simplicity.

SWith hindsight the whole IIO_MOD_X_OR_Y_OR_Z mess was a design error :(
We can teach userspace code about that quirk for accelerations where
the one that would be hard to handle is the AND case used for
freefall detectors (you detect that the signal magnitude is near 0 for
all axes).  I can't think of another option for that one other than
the weird modifier (unlike this case)

> 
> Should we consider a IIO_EV_INFO_LABEL so that we can have some sort
> of attribute (namely `events/<dir>_<channel spec>_label`) so that
> userspace can enumerate expected events for non-configurable events?

Probably needs something similar to channel labelling, so a separate
callback given we don't handle strings, but sure something like this
would be useful and provide 'hints' along the lines of what the
datasheet calls a particular event.  Not however for what event is sent
as such info should be apparent from the event naming.

Jonathan