Re: [PATCH v3 22/27] staging: iio: resolver: ad2s1210: convert LOS threshold to event attr

[David Lechner <dlechner@baylibre.com>]

On Sat, Sep 30, 2023 at 10:42 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 29 Sep 2023 12:23:27 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
> > From: David Lechner <david@lechnology.com>
> >
> > From: David Lechner <dlechner@baylibre.com>
> >
> > The AD2S1210 has a programmable threshold for the loss of signal (LOS)
> > fault. This fault is triggered when either the sine or cosine input
> > falls below the threshold voltage.
> >
> > This patch converts the custom device LOS threshold attribute to an
> > event falling edge threshold attribute on a new monitor signal channel.
> > The monitor signal is an internal signal that combines the amplitudes
> > of the sine and cosine inputs as well as the current angle and position
> > output. This signal is used to detect faults in the input signals.
>
> Hmm. Looking forwards, I'm less sure that we should be shoving all these
> error conditions onto one channel. Fundamentally we have
> sine and cosine inputs. I think we should treat those as separate channels
> and include a third differential channel between them.

At first, I did consider a differential channel as you suggested in
v2. However, the datasheet is quite clear that the LOS and DOS faults
(and only those faults) come from a signal it calls the "monitor
signal". This signal is defined as:

    Monitor = A1 * sin(theta)  * sin(phi) + A2 * cos(theta) * cos(phi)

where A1 * sin(theta) is the the sine input, A2 * cos(theta) is the
cosine input and phi is the position output. So mathematically
speaking, there is no signal that is the difference between the two
inputs. (See "Theory of Operation" section in the datasheet.)

But if we want to hide these internal details and don't care about a
strict definition of "differential", then what is suggested below
seems fine.

>
> So this one becomes a double event (you need to signal it on both
> cosine and sine channels).  The DOS overange is similar.
> The DOS mismatch is a threshold on the differential channel giving
>
> events/in_altvoltage0_thresh_falling_value
> events/in_altvoltage1_thresh_falling_value (these match)
> events/in_altvoltage0_thresh_rising_value
> events/in_altvoltage1_thresh_rising_value (matches previous which is fine)
> events/in_altvoltage1-altvoltage0_mag_rising_value
>
> Does that work here?  Avoids smashing different types of signals together.
> We could even do the LOT as differential between two angle channels
> (tracking one and measured one) but meh that's getting complex.>
> Note this will rely on channel labels to make the above make any sense at all.

I think this could be OK - I think what matters most is having some
documentation that maps the faults and registers on the chip to the
iio names. Where would the sine/cosine clipping fault fit in though? I
got a bit too creative and used X_OR_Y to differentiate it (see
discussion in "staging: iio: resolver: ad2s1210: implement fault
events"). Strictly speaking, it should probably be a type: threshold,
direction: either event on both the sine and cosine input channels
(another double event) since it occurs if either of the signal exceeds
the power or ground rail voltage. But we already have threshold rising
and threshold falling on these channels with a different meaning. I
guess it could call it magnitude instead of a threshold?