Message-ID: <20231005153736.2603dbbf@jic23-huawei>
Date:   Thu, 5 Oct 2023 15:37:36 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     David Lechner <dlechner@...libre.com>
Cc:     linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-staging@...ts.linux.dev,
        David Lechner <david@...hnology.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Nuno Sá <nuno.sa@...log.com>,
        Axel Haslam <ahaslam@...libre.com>,
        Philip Molloy <pmolloy@...libre.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 22/27] staging: iio: resolver: ad2s1210: convert LOS
 threshold to event attr

On Mon, 2 Oct 2023 11:09:11 -0500
David Lechner <dlechner@...libre.com> wrote:

> On Sat, Sep 30, 2023 at 10:42 AM Jonathan Cameron <jic23@...nel.org> wrote:
> >
> > On Fri, 29 Sep 2023 12:23:27 -0500
> > David Lechner <dlechner@...libre.com> wrote:
> >  
> > > From: David Lechner <david@...hnology.com>
> > >
> > > From: David Lechner <dlechner@...libre.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.)

Hmm. That's certainly a bit more complex than I expected.
Relying on the brief description led me astray.

It's related to the differences in the measured and  as if
theta == phi and A1 == A2 (ideal) then it will be A1.

I can see it's relevant to DOS, but not LOS.  The description of LOS
seems to overlap a number of different things unfortunately.



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

Probably best to introduce that monitor signal though we'll have
to be a bit vague about what it is which has the side effect that
anyone trying to understand what on earth these faults are is going
to be confused (having read the datasheet section a couple of times
I'm not 100% sure...)

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

Tricky indeed.  Though I guess we only hit the clipping case after
LOS or DOS fires or if their thresholds are set too wide (is that
even possible?).  So it is useful to report it as we are already in
error? Or can we combine the cases by treating it as a cap on the
threshold controls for LOS and DOS?

Even when they aren't just there for error reporting, designers
seem to always come up with new create signals to use for event
detection and sometimes it's a real struggle to map them to
something general.

Jonathan

Powered by blists - more mailing lists