linux-kernel - Re: [PATCH] dt-bindings: iio: sx9310: Add various settings as DT properties

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <160108304513.310579.9483266115343530431@swboyd.mtv.corp.google.com>
Date:   Fri, 25 Sep 2020 18:17:25 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc:     Jonathan Cameron <jic23@...nel.org>, 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

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.

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

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] 

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.

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.