Message-ID: <20200926165049.4c314816@archlinux>
Date:   Sat, 26 Sep 2020 16:50:49 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
        Daniel Campello <campello@...omium.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        Douglas Anderson <dianders@...omium.org>,
        Gwendal Grignou <gwendal@...omium.org>,
        Evan Green <evgreen@...omium.org>
Subject: Re: [PATCH] dt-bindings: iio: sx9310: Add various settings as DT
 properties

On Fri, 25 Sep 2020 18:17:25 -0700
Stephen Boyd <swboyd@...omium.org> wrote:

> Sorry this thread is deep! Good news is I have moved the proximity
> thresholds, hysteresis, hardware gain, and debounce to userspace. Now
> just to figure out this filter strength.

:) They get a lot deeper sometimes!

> 
> Quoting Jonathan Cameron (2020-09-09 04:15:50)
> > On Tue, 8 Sep 2020 23:18:43 -0700
> > Stephen Boyd <swboyd@...omium.org> wrote:
> >   
> > > Quoting Jonathan Cameron (2020-09-06 07:02:47)  
> >   
> > >   
> > > >     
> > > > > +
> > > > > +  semtech,proxraw-strength:
> > > > > +    allOf:
> > > > > +      - $ref: /schemas/types.yaml#definitions/uint32
> > > > > +      - enum: [0, 2, 4, 8]
> > > > > +    default: 2
> > > > > +    description:
> > > > > +      PROXRAW filter strength. A value of 0 represents off, and other values
> > > > > +      represent 1-1/N.    
> > > > 
> > > > Having looked at the datasheet I have little or now idea of what this filter
> > > > actually is.  However, what is the argument for it being in DT rather than
> > > > exposing a userspace control of some type.    
> > > 
> > > I only see this equation in the datasheet
> > > 
> > > F(PROXRAW ; PROXUSEFUL[n-1] ; RAWFILT) = (1 - RAWFILT).PROXRAW + RAWFILT.PROXUSEFUL[n-1] 
> > > 
> > > and it's talking about updating PROXUSEFUL. "PROXUSEFUL update consists
> > > of filtering PROXRAW upfront to remove its high frequencies components".
> > > So presumably this filter is used to make proxraw into proxuseful so
> > > that it is a meaningful number. Is this a new knob in userspace?  
> > 
> > It might fit with the various filter definitions, but there is so little info
> > it is hard to map it across.   Perhaps DT is the best we can do here even
> > though it would ideally be controlled from userspace.
> >   
> 
> Ok I read the datasheet a couple more times :)
> 
> This sensor seems to have multiple levels of processing on the signal.
> First the raw signal is there as PROXRAW. That gets turned into
> PROXUSEFUL with this calculation:
> 
>  F(PROXRAW ; PROXUSEFUL[n-1] ; RAWFILT) = (1 - RAWFILT) * PROXRAW + RAWFILT * PROXUSEFUL[n-1]
> 
> This semtech,proxraw-strength property is trying to set that RAWFILT
> variable to something like 2, 4, or 8. Or 0 for "off". Is that in terms
> of 3db?

Don't think so.  Looks like a very short IIR filter.
https://zipcpu.com/dsp/2017/08/19/simple-filter.html looks like it
has a good basic explanation.  To get the 3db point you'd need to
plot a graph like Fig 2 on there and read off the point 

http://www.dspguide.com/ch19/2.htm provides some relevant maths for
working out the value of RAW_FILT (which is probably 1/2, 1/4, 1/8?)
if we wanted a low pass filter with a particular cut off.
If f_c is in samples and I haven't made a silly error...

1/RAWFILT = e^(-2pi*f_c)

We want fc for each value of raw filt.

ln(1/RAWFILT) = -2pi*f_c
fc = -1/(2pi) * ln(1/RAWFILT)

Next problem is that IIO filter 3db point is in Hz, not samples.
So if we have fixed sampling rate we'll need to take that into
account.  The filter 3db point will need to change with that
sampling rate.

> A bigger question, does the useful value need to be a different
> channel so it can be configured from userspace? We don't get an
> interrupt when this changes but at least the value can be read out of
> the hardware from what I can tell.

Yes, it will have to be a separate value if we are exposing both
PROXRAW and PROXUSEFUL. 

> 
> The PROXUSEFUL value is turned into PROXAVG. There is a positive filter
> strength and a negative filter strength that is used to filter the
> PROXAVG value. I need to set the positive filter strength to be
> different than the default. That's what I'm trying to do with
> semtech,avg-pos-strength. It factors into this equation for PROXUSEFUL:
> 
> if (PROXUSEFUL - PROXAVG[n-1] >= 0)
>   F(PROXUSEFUL ; PROXAVG[n-1] ; AVGPOSFILT) = (1 - AVGPOSFILT) * PROXUSEFUL + AVGPOSFILT * PROXAVG[n-1] 
> else
>   F(PROXUSEFUL ; PROXAVG[n-1] ; AVGNEGFILT) = (1 - AVGNEGFILT) * PROXUSEFUL + AVGNEGFILT * PROXAVG[n-1] 

Gah. Whoever designed this was doing some nasty adhoc DSP hacks.
That is a second low pass IIR filter with a different parameter.

It's been 20 years since I learnt this stuff, so I'm going to sigh at this
point and stop doing the maths (z transform fun..)

It's probably a more complex IIR filter but beyond that...

> 
> so depending on how the historical average value is going we filter
> differently. Again, is this in 3db? This register has a setting of
> "infinite" which I guess is used to make the above equation come out to
> be just PROXAVG[n - 1]? Otherwise 0 is "off" which seems to make the
> above equation boil down to:
> 
>   PROXAVG = PROXUSEFUL
> 
> when you do substitution.
> 
> I agree it looks like some sort of filter, so maybe I need to introduce
> some proximity.*filter ABI? I don't know the units though.

A control with no strict definition becomes a unitless wiggle dial.
Not actually possible to set other than by guessing and see if it works.

> 
> To complete the story, the PROXAVG value gets compared to a threshold
> AVGTHRESH (settable in a register) and that can be debounced with
> another register setting (AVGDEB). That results in PROXUSEFUL which goes
> into this PROXDIFF equation:
> 
>  PROXDIFF = (PROXUSEFUL - PROXAVG) >> 4
> 
> The PROXDIFF value is compared to the proximity threshold register
> setting (PROXTHRESH, i.e. bits 3:7 in register RegProxCtrl8/9) plus or
> minus the hysteresis (RegProxCtrl10 bits 5:4) and then debounced
> (RegProxCtrl10 bits 3:2 (for close) and 1:0 (for far)).
> 
> if (PROXDIFF > PROXTHRESH + HYST)
>   // close event, i.e. DIR_FALLING
>   PROXSTAT = debounce() ? 1 : 0;
> else if (PROXDIFF < PROXTHRESH - HYST)
>   // far event, i.e. DIR_RISING
>   PROXSTAT = debounce() ? 0 : 1;
> 
> If that all passes then PROXSTAT is set to 1 for the close condition and
> 0 for the far condition. An irq is raised and eventually this driver
> will signal a new event indicating rising or falling.
> 
> I see that the driver implements sx9310_read_prox_data() as a read on
> the PROXDIFF value. That looks good for reading the processed signal for
> a channel after all that raw/avg/useful debouncing and filtering.

So, the controls IIO exposes rely on 3dB being enough to give the user some
sort of reasonable control over what filtering is done.  Sometimes we
get a part that does a bunch of filters like this one doesn't come
with plots of the filter responses...  In those cases it is almost impossible
to present anything to a user that they can figure out. 

So maybe best plan is just to give up and put them as controls in DT that
basically say see datasheet?

Horrible, but alternative is to do the maths and see if you can come
up with a control that has real meaning to a user.

Jonathan

Powered by blists - more mailing lists