Message-ID: <160090354388.310579.2465697259775252128@swboyd.mtv.corp.google.com>
Date:   Wed, 23 Sep 2020 16:25:43 -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

Quoting Jonathan Cameron (2020-09-09 04:15:50)
> On Tue, 8 Sep 2020 23:18:43 -0700
> Stephen Boyd <swboyd@...omium.org> wrote:
> > >   
> > > > +
> > > > +  semtech,cs0-gain-factor:
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#definitions/uint32
> > > > +      - enum: [1, 2, 4, 8]
> > > > +    default: 1
> > > > +    description:
> > > > +      Gain factor for CS0 (and combined if any) sensor.  
> > > 
> > > Why is this something that should be in DT as opposed to via
> > > a userspace control?  We have hardwaregain for this purpose (I think)  
> > 
> > Thanks I'm not aware of hardwaregain. That looks like it should work.

One weird thing I notice is that the channels can share some settings
like this hardware gain factor. Is there support for a subset of
channels sharing the same gain? I see there is mask_separate and
shared_by_all, type, etc. but I don't see how I can make the setting
apply to some subset of channels. I guess just do proper locking and
make sure when one is changed it changes the other channel too?

> > 
> > >   
> > >   
> > > > +
> > > > +  semtech,compensate-common:
> > > > +    description: Any sensor triggers compensation of all channels.
> > > > +    type: boolean  
> > > 
> > > Compensation for what?  
> > 
> > This is for RegProxCtrl6 bit 6 AVGCOMPMETHOD. 
> > 
> >       Defines the average compensation method:
> > 
> >       0: Individual. Each sensor triggers only its own compensation
> >       1: Common. Any sensor triggers compensation of all channels. 
> > 
> > I believe this is for the offset compensation.
> 
> I wonder if anyone will actually care which choice we make on that?
> Perhaps just pick one and don't make it controllable?
> 
> Reading that it sounds like a control that is there because it was easy
> to do in hardware rather than necessarily making any sense from
> a usecase point of view.  Do we have any info on how it is used?

Fair enough. I will try to contact Semtech to understand how it is used.
This bit is different between two products I'm aware of but for all I
know it doesn't actually matter. Given that we detect proximity of each
channel individually it feels like we should leave this as 0. I'll try
that out and see how it works.

> 
> > 
> > >   
> > > > +
> > > > +  semtech,avg-pos-strength:
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#definitions/uint32
> > > > +      - enum: [0, 16, 64, 128, 256, 512, 1024, 4294967295]
> > > > +    default: 16
> > > > +    description:
> > > > +      Average positive filter strength. A value of 0 represents off and
> > > > +      UINT_MAX (4294967295) represents infinite. Other values
> > > > +      represent 1-1/N.  
> > > 
> > > I'm not sure about using UINT_MAX to represent infinity. Rob any thoughts on
> > > this?
> > > 
> > > Again, why does it make sense to have the filter controls in DT?  
> > 
> > Is there an IIO property for this? Seems OK to move it to userspace.
> 
> I'm not sure enough of what it means, but we have filter controls in
> terms of 3db point and oversampling. If you can figure out a match to
> those or something that seems more generic than the above to propose
> as new ABI that would be great.

Ok let me see what I can do.

> > 
> > >   
> > > > +
> > > > +  semtech,cs1-prox-threshold:
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#definitions/uint32
> > > > +      - enum: [2, 4, 6, 8, 12, 16, 20, 24, 28, 32, 40,
> > > > +               48, 56, 64, 72, 80, 88, 96, 112, 128, 144,
> > > > +               160, 192, 224, 256, 320, 384, 512, 640,
> > > > +               768, 1024, 1536]
> > > > +    default: 12
> > > > +    description:
> > > > +      Proximity detection threshold for CS1 sensor.
> > > > +
> > > > +  semtech,cs2-prox-threshold:
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#definitions/uint32
> > > > +      - enum: [2, 4, 6, 8, 12, 16, 20, 24, 28, 32, 40,
> > > > +               48, 56, 64, 72, 80, 88, 96, 112, 128, 144,
> > > > +               160, 192, 224, 256, 320, 384, 512, 640,
> > > > +               768, 1024, 1536]
> > > > +    default: 12
> > > > +    description:
> > > > +      Proximity detection threshold for CS2 sensor.
> > > > +
> > > > +  semtech,cs0-body-threshold:
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#definitions/uint32
> > > > +      - enum: [0, 300, 600, 900, 1200, 1500, 1800, 30000]
> > > > +    default: 1800
> > > > +    description:
> > > > +      Body detection threshold for CS0 (and combined if any) sensor.  
> > > 
> > > As before, why DT plus child nodes  
> > 
> > How should I differentiate body vs. proximity thresholds in userspace?
> > Or should I make it /sys/.../events/in_proximity0_thresh_falling_value
> > vs. /sys/.../events/in_proximity0_thresh_rising_value?
> 
> I'm not sure what they actually are. A problem with IIO that may be relevant
> is that we have never supported multiple events of the same type for a channel.
> Unfortunately that is hard to change now as we'd have to redefine the event
> codes.
> 
> It feels like these are potentially a bit smarter however. If they are we
> could handle them as a different event type.  Or potentially an event
> on a different channel (arguably they are some result of some sort of
> processing of more than a simple single value).  There is a patent but I've
> not read it in detail.

Hmm.. ok. It looks like the driver doesn't enable the "smart sensor"
mode where the body threshold would matter. If I'm reading the datasheet
properly, the only threshold we need to configure is the proximity one.
If/when the driver supports the body threshold we can look into adding
the second threshold. So, let's punt this one? I'll implement a
threshold for the proximity in userspace via the IIO_EV_TYPE_THRESH.

> 
> > 
> > >   
> > > > +
> > > > +  semtech,cs1-body-threshold:
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#definitions/uint32
> > > > +      - enum: [0, 300, 600, 900, 1200, 1500, 1800, 30000]
> > > > +    default: 12
> > > > +    description:
> > > > +      Body detection threshold for CS1 sensor.
> > > > +
> > > > +  semtech,cs2-body-threshold:
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#definitions/uint32
> > > > +      - enum: [0, 300, 600, 900, 1200, 1500, 1800, 30000]
> > > > +    default: 12
> > > > +    description:
> > > > +      Body detection threshold for CS2 sensor.
> > > > +
> > > > +  semtech,hysteresis:
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#definitions/uint32
> > > > +      - enum: [0, 6, 12, 25]
> > > > +    default: 0
> > > > +    description:
> > > > +      The percentage of hysteresis +/- applied to proximity/body samples.  
> > > 
> > > Is this hysteresis on an event?  If so we have defined ABI to control that
> > > from userspace, though as an absolute value rather than a precentage so some
> > > magic will be needed.  Hysteresis is usually defined only the 'not event'
> > > direction rather than +/-  
> > 
> > Is this IIO_EV_INFO_HYSTERESIS? It looks like it is applied to the
> > threshold by shifting it right by 4, 3, or 2. I think the +/- is
> > actually dependent on the RegProxCtrl10 bit 6 FARCOND value, so maybe
> > that isn't a problem. We could make another value like hysteresis shift
> > or hysteresis percentage?
> 
> I'd rather avoid extra ABI if we can make it work as it stands, even if it
> is a little involved to do so.  Extra ABI just means we end up with more
> incompatible userspace code over time.

Alright. I will attempt to make this work with the
IIO_EV_INFO_HYSTERESIS knob.

> 
> > 
> > >   
> > > > +
> > > > +  semtech,close-debounce-samples:
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#definitions/uint32
> > > > +      - enum: [0, 2, 4, 8]
> > > > +    default: 0
> > > > +    description:
> > > > +      The number of close samples debounced for proximity/body thresholds.  
> > > 
> > > This feels like something that has more to do with the object motion than
> > > the sensor setup, so perhaps should be controlled from userspace?  
> > 
> > Sure. Is there an IIO sample property? Or I should make a custom
> > knob for this?
> 
> It's kind of close to in_proximity0_thresh_period and that may be how they
> have implemented it.
> 
> That control specifies a number of samples for which a condition should be true
> before it is reported.

Sounds good. I can do that. It looks like the driver reports close/far
via an event and these debounce values are the same for me so I can
write both fields (close and far) with the same thresh_period value from
userspace. If they need to be different between the two then this can be
reevaluated?

> 
> > 
> > >   
> > > > +
> > > > +  semtech,far-debounce-samples:
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#definitions/uint32
> > > > +      - enum: [0, 2, 4, 8]
> > > > +    default: 0
> > > > +    description:
> > > > +      The number of far samples debounced for proximity/body thresholds.
> > > > +
> > > >  required:
> > > >    - compatible

Powered by blists - more mailing lists