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

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

[Stephen Boyd]

We need to set various bits in the hardware registers for this device to
operate properly depending on how it is installed. Add a handful of DT
properties to configure these things.

Cc: Daniel Campello <campello@chromium.org>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: <devicetree@vger.kernel.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Gwendal Grignou <gwendal@chromium.org>
Cc: Evan Green <evgreen@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

I haven't written any code to handle these properties yet. I'd rather do
that once the binding patch is reviewed. Patch based on iio.git testing
branch.

 .../iio/proximity/semtech,sx9310.yaml         | 182 ++++++++++++++++++
 1 file changed, 182 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
index 5739074d3592..e74b81483c14 100644
--- a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
+++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
@@ -40,6 +40,169 @@ properties:
   "#io-channel-cells":
     const: 1
 
+  semtech,cs0-ground:
+    description: Indicates the CS0 sensor is connected to ground.
+    type: boolean
+
+  semtech,combined-sensors:
+    allOf:
+      - $ref: /schemas/types.yaml#definitions/uint32
+      - enum: [0, 1, 2, 3]
+    default: 0
+    description:
+      Which sensors are combined. 0 for CS3, 1 for CS0+CS1, 2 for CS1+CS2,
+      and 3 for all sensors.
+
+  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.
+
+  semtech,cs1-gain-factor:
+    allOf:
+      - $ref: /schemas/types.yaml#definitions/uint32
+      - enum: [1, 2, 4, 8]
+    default: 1
+    description:
+      Gain factor for CS1 sensor.
+
+  semtech,cs2-gain-factor:
+    allOf:
+      - $ref: /schemas/types.yaml#definitions/uint32
+      - enum: [1, 2, 4, 8]
+    default: 1
+    description:
+      Gain factor for CS2 sensor.
+
+  semtech,resolution:
+    description:
+      Capacitance measure resolution.
+    enum:
+      - coarsest
+      - very-coarse
+      - coarse
+      - medium-coarse
+      - medium
+      - fine
+      - very-fine
+      - finest
+
+  semtech,startup-sensor:
+    allOf:
+      - $ref: /schemas/types.yaml#definitions/uint32
+      - enum: [0, 1, 2, 3]
+    default: 0
+    description:
+      Sensor used for start-up proximity detection. The combined
+      sensor is represented by 3.
+
+  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.
+
+  semtech,compensate-common:
+    description: Any sensor triggers compensation of all channels.
+    type: boolean
+
+  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.
+
+  semtech,cs0-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 CS0 (and combined if any) sensor.
+
+  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.
+
+  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.
+
+  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.
+
+  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
   - reg
@@ -61,5 +224,24 @@ examples:
         vdd-supply = <&pp3300_a>;
         svdd-supply = <&pp1800_prox>;
         #io-channel-cells = <1>;
+        semtech,cs0-ground;
+        semtech,combined-sensors = <0>;
+        semtech,cs0-gain-factor = <8>;
+        semtech,cs1-gain-factor = <8>;
+        semtech,cs2-gain-factor = <8>;
+        semtech,resolution = "fine";
+        semtech,startup-sensor = <1>;
+        semtech,proxraw-strength = <2>;
+        semtech,compensate-common;
+        semtech,avg-pos-strength = <64>;
+        semtech,cs0-prox-threshold = <96>;
+        semtech,cs1-prox-threshold = <112>;
+        semtech,cs2-prox-threshold = <96>;
+        semtech,cs0-body-threshold = <300>;
+        semtech,cs1-body-threshold = <300>;
+        semtech,cs2-body-threshold = <300>;
+        semtech,hysteresis = <0>;
+        semtech,close-debounce-samples = <2>;
+        semtech,far-debounce-samples = <2>;
       };
     };

base-commit: 1bebdcb928eba880f3a119bacb8149216206958a
-- 
Sent by a computer, using git, on the internet

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

[Jonathan Cameron]

On Thu,  3 Sep 2020 15:18:28 -0700
Stephen Boyd <swboyd@chromium.org> wrote:

> We need to set various bits in the hardware registers for this device to
> operate properly depending on how it is installed. Add a handful of DT
> properties to configure these things.
> 
> Cc: Daniel Campello <campello@chromium.org>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: <devicetree@vger.kernel.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Cc: Evan Green <evgreen@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> 
> I haven't written any code to handle these properties yet. I'd rather do
> that once the binding patch is reviewed. Patch based on iio.git testing
> branch.
Makes sense to do docs first for this.  Quite a bit feels like it isn't
a feature of the device configuration, but rather of the usecase.  That
stuff should probably be done with a userspace interface, but you may
be able to argue me around on some of them! 

Jonathan

> 
>  .../iio/proximity/semtech,sx9310.yaml         | 182 ++++++++++++++++++
>  1 file changed, 182 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> index 5739074d3592..e74b81483c14 100644
> --- a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> +++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> @@ -40,6 +40,169 @@ properties:
>    "#io-channel-cells":
>      const: 1
>  
> +  semtech,cs0-ground:
> +    description: Indicates the CS0 sensor is connected to ground.
> +    type: boolean

This one is probably fine. I can't think of a similar interface we need
to match, but maybe Rob or someone else will have a suggestion.

> +
> +  semtech,combined-sensors:
> +    allOf:
> +      - $ref: /schemas/types.yaml#definitions/uint32
> +      - enum: [0, 1, 2, 3]
> +    default: 0
> +    description:
> +      Which sensors are combined. 0 for CS3, 1 for CS0+CS1, 2 for CS1+CS2,
> +      and 3 for all sensors.

Make it clear in this description what 'combined' means.
Also, I think this would be better as a set of values with an anyOf match to say
<3>
<0>, <1> 
<1>, <2> 
<1>, <2>, <3>

Fine to insist they are in numeric order.

> +
> +  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)

Also we mostly use child nodes to allow us to specify characteristics
of individual channels.

> +
> +  semtech,cs1-gain-factor:
> +    allOf:
> +      - $ref: /schemas/types.yaml#definitions/uint32
> +      - enum: [1, 2, 4, 8]
> +    default: 1
> +    description:
> +      Gain factor for CS1 sensor.
> +
> +  semtech,cs2-gain-factor:
> +    allOf:
> +      - $ref: /schemas/types.yaml#definitions/uint32
> +      - enum: [1, 2, 4, 8]
> +    default: 1
> +    description:
> +      Gain factor for CS2 sensor.
> +
> +  semtech,resolution:
> +    description:
> +      Capacitance measure resolution.
> +    enum:
> +      - coarsest
> +      - very-coarse
> +      - coarse
> +      - medium-coarse
> +      - medium
> +      - fine
> +      - very-fine
> +      - finest
I'd normally be very against cases like this where we have something that
feels like it should have a clear definition rather than a random wordy scale
but these are all the information I can find in the datasheet.

I would suggest adding a specific reference to the datasheet for this one.

> +
> +  semtech,startup-sensor:
> +    allOf:
> +      - $ref: /schemas/types.yaml#definitions/uint32
> +      - enum: [0, 1, 2, 3]
> +    default: 0
> +    description:
> +      Sensor used for start-up proximity detection. The combined
> +      sensor is represented by 3.

This feels like it should be a userspace control rather than in DT?

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

> +
> +  semtech,compensate-common:
> +    description: Any sensor triggers compensation of all channels.
> +    type: boolean

Compensation for what?

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


> +
> +  semtech,cs0-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 CS0 (and combined if any) sensor.

That is definitely a userspace thing. Why would you put it in DT?
Also same comment as above for channels as child nodes

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

> +
> +  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 +/-

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

> +
> +  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
>    - reg
> @@ -61,5 +224,24 @@ examples:
>          vdd-supply = <&pp3300_a>;
>          svdd-supply = <&pp1800_prox>;
>          #io-channel-cells = <1>;
> +        semtech,cs0-ground;
> +        semtech,combined-sensors = <0>;
> +        semtech,cs0-gain-factor = <8>;
> +        semtech,cs1-gain-factor = <8>;
> +        semtech,cs2-gain-factor = <8>;
> +        semtech,resolution = "fine";
> +        semtech,startup-sensor = <1>;
> +        semtech,proxraw-strength = <2>;
> +        semtech,compensate-common;
> +        semtech,avg-pos-strength = <64>;
> +        semtech,cs0-prox-threshold = <96>;
> +        semtech,cs1-prox-threshold = <112>;
> +        semtech,cs2-prox-threshold = <96>;
> +        semtech,cs0-body-threshold = <300>;
> +        semtech,cs1-body-threshold = <300>;
> +        semtech,cs2-body-threshold = <300>;
> +        semtech,hysteresis = <0>;
> +        semtech,close-debounce-samples = <2>;
> +        semtech,far-debounce-samples = <2>;
>        };
>      };
> 
> base-commit: 1bebdcb928eba880f3a119bacb8149216206958a

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

[Stephen Boyd]

Quoting Jonathan Cameron (2020-09-06 07:02:47)
> On Thu,  3 Sep 2020 15:18:28 -0700
> Stephen Boyd <swboyd@chromium.org> wrote:
> 
> > We need to set various bits in the hardware registers for this device to
> > operate properly depending on how it is installed. Add a handful of DT
> > properties to configure these things.
> > 
> > Cc: Daniel Campello <campello@chromium.org>
> > Cc: Lars-Peter Clausen <lars@metafoo.de>
> > Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: <devicetree@vger.kernel.org>
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Gwendal Grignou <gwendal@chromium.org>
> > Cc: Evan Green <evgreen@chromium.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> > 
> > I haven't written any code to handle these properties yet. I'd rather do
> > that once the binding patch is reviewed. Patch based on iio.git testing
> > branch.
> Makes sense to do docs first for this.  Quite a bit feels like it isn't
> a feature of the device configuration, but rather of the usecase.  That
> stuff should probably be done with a userspace interface, but you may
> be able to argue me around on some of them! 
> 
> 

Thanks for reviewing! I'm not well read on IIO so please bear with my
ignorance.

> > 
> >  .../iio/proximity/semtech,sx9310.yaml         | 182 ++++++++++++++++++
> >  1 file changed, 182 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> > index 5739074d3592..e74b81483c14 100644
> > --- a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> > +++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> > @@ -40,6 +40,169 @@ properties:
> >    "#io-channel-cells":
> >      const: 1
> >  
> > +  semtech,cs0-ground:
> > +    description: Indicates the CS0 sensor is connected to ground.
> > +    type: boolean
> 
> This one is probably fine. I can't think of a similar interface we need
> to match, but maybe Rob or someone else will have a suggestion.

Ok.

> 
> > +
> > +  semtech,combined-sensors:
> > +    allOf:
> > +      - $ref: /schemas/types.yaml#definitions/uint32
> > +      - enum: [0, 1, 2, 3]
> > +    default: 0
> > +    description:
> > +      Which sensors are combined. 0 for CS3, 1 for CS0+CS1, 2 for CS1+CS2,
> > +      and 3 for all sensors.
> 
> Make it clear in this description what 'combined' means.
> Also, I think this would be better as a set of values with an anyOf match to say
> <3>
> <0>, <1> 
> <1>, <2> 
> <1>, <2>, <3>
> 
> Fine to insist they are in numeric order.

Ok, sure. I can make it a list of sensor numbers.

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

> 
> Also we mostly use child nodes to allow us to specify characteristics
> of individual channels.

Got it.

> 
> > +
> > +  semtech,cs1-gain-factor:
> > +    allOf:
> > +      - $ref: /schemas/types.yaml#definitions/uint32
> > +      - enum: [1, 2, 4, 8]
> > +    default: 1
> > +    description:
> > +      Gain factor for CS1 sensor.
> > +
> > +  semtech,cs2-gain-factor:
> > +    allOf:
> > +      - $ref: /schemas/types.yaml#definitions/uint32
> > +      - enum: [1, 2, 4, 8]
> > +    default: 1
> > +    description:
> > +      Gain factor for CS2 sensor.
> > +
> > +  semtech,resolution:
> > +    description:
> > +      Capacitance measure resolution.
> > +    enum:
> > +      - coarsest
> > +      - very-coarse
> > +      - coarse
> > +      - medium-coarse
> > +      - medium
> > +      - fine
> > +      - very-fine
> > +      - finest
> I'd normally be very against cases like this where we have something that
> feels like it should have a clear definition rather than a random wordy scale
> but these are all the information I can find in the datasheet.
> 
> I would suggest adding a specific reference to the datasheet for this one.

Are you saying description should say:

Capacitance measure resolution. Refer to datasheet for more details.

?

> 
> > +
> > +  semtech,startup-sensor:
> > +    allOf:
> > +      - $ref: /schemas/types.yaml#definitions/uint32
> > +      - enum: [0, 1, 2, 3]
> > +    default: 0
> > +    description:
> > +      Sensor used for start-up proximity detection. The combined
> > +      sensor is represented by 3.
> 
> This feels like it should be a userspace control rather than in DT?

I believe this is used during initial compensation, so it needs to be
set before sx9310_init_compensation() runs at probe time. Probably can't
be moved to userspace.

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

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

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

> 
> 
> > +
> > +  semtech,cs0-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 CS0 (and combined if any) sensor.
> 
> That is definitely a userspace thing. Why would you put it in DT?
> Also same comment as above for channels as child nodes

Alright. Presumably this is IIO_EV_TYPE_THRESH?

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

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

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

> 
> > +
> > +  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
> >    - reg

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

[Jonathan Cameron]

On Tue, 8 Sep 2020 23:18:43 -0700
Stephen Boyd <swboyd@chromium.org> wrote:

> Quoting Jonathan Cameron (2020-09-06 07:02:47)
> > On Thu,  3 Sep 2020 15:18:28 -0700
> > Stephen Boyd <swboyd@chromium.org> wrote:
> >   
> > > We need to set various bits in the hardware registers for this device to
> > > operate properly depending on how it is installed. Add a handful of DT
> > > properties to configure these things.
> > > 
> > > Cc: Daniel Campello <campello@chromium.org>
> > > Cc: Lars-Peter Clausen <lars@metafoo.de>
> > > Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: <devicetree@vger.kernel.org>
> > > Cc: Douglas Anderson <dianders@chromium.org>
> > > Cc: Gwendal Grignou <gwendal@chromium.org>
> > > Cc: Evan Green <evgreen@chromium.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > ---
> > > 
> > > I haven't written any code to handle these properties yet. I'd rather do
> > > that once the binding patch is reviewed. Patch based on iio.git testing
> > > branch.  
> > Makes sense to do docs first for this.  Quite a bit feels like it isn't
> > a feature of the device configuration, but rather of the usecase.  That
> > stuff should probably be done with a userspace interface, but you may
> > be able to argue me around on some of them! 
> > 
> >   
> 
> Thanks for reviewing! I'm not well read on IIO so please bear with my
> ignorance.
> 
No problem!

> > > 
> > >  .../iio/proximity/semtech,sx9310.yaml         | 182 ++++++++++++++++++
> > >  1 file changed, 182 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> > > index 5739074d3592..e74b81483c14 100644
> > > --- a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> > > +++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> > > @@ -40,6 +40,169 @@ properties:
> > >    "#io-channel-cells":
> > >      const: 1
> > >  
> > > +  semtech,cs0-ground:
> > > +    description: Indicates the CS0 sensor is connected to ground.
> > > +    type: boolean  
> > 
> > This one is probably fine. I can't think of a similar interface we need
> > to match, but maybe Rob or someone else will have a suggestion.  
> 
> Ok.
> 
> >   
> > > +
> > > +  semtech,combined-sensors:
> > > +    allOf:
> > > +      - $ref: /schemas/types.yaml#definitions/uint32
> > > +      - enum: [0, 1, 2, 3]
> > > +    default: 0
> > > +    description:
> > > +      Which sensors are combined. 0 for CS3, 1 for CS0+CS1, 2 for CS1+CS2,
> > > +      and 3 for all sensors.  
> > 
> > Make it clear in this description what 'combined' means.
> > Also, I think this would be better as a set of values with an anyOf match to say
> > <3>
> > <0>, <1> 
> > <1>, <2> 
> > <1>, <2>, <3>
> > 
> > Fine to insist they are in numeric order.  
> 
> Ok, sure. I can make it a list of sensor numbers.
> 
> >   
> > > +
> > > +  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.
> 
> > 
> > Also we mostly use child nodes to allow us to specify characteristics
> > of individual channels.  
> 
> Got it.
> 
> >   
> > > +
> > > +  semtech,cs1-gain-factor:
> > > +    allOf:
> > > +      - $ref: /schemas/types.yaml#definitions/uint32
> > > +      - enum: [1, 2, 4, 8]
> > > +    default: 1
> > > +    description:
> > > +      Gain factor for CS1 sensor.
> > > +
> > > +  semtech,cs2-gain-factor:
> > > +    allOf:
> > > +      - $ref: /schemas/types.yaml#definitions/uint32
> > > +      - enum: [1, 2, 4, 8]
> > > +    default: 1
> > > +    description:
> > > +      Gain factor for CS2 sensor.
> > > +
> > > +  semtech,resolution:
> > > +    description:
> > > +      Capacitance measure resolution.
> > > +    enum:
> > > +      - coarsest
> > > +      - very-coarse
> > > +      - coarse
> > > +      - medium-coarse
> > > +      - medium
> > > +      - fine
> > > +      - very-fine
> > > +      - finest  
> > I'd normally be very against cases like this where we have something that
> > feels like it should have a clear definition rather than a random wordy scale
> > but these are all the information I can find in the datasheet.
> > 
> > I would suggest adding a specific reference to the datasheet for this one.  
> 
> Are you saying description should say:
> 
> Capacitance measure resolution. Refer to datasheet for more details.

Yes, basically we are saying we can't provide documentation here that is
sufficiently detailed.  (Mostly because the datasheet doesn't really do so
either) Hence, we aren't going to try.  Normally I hate this sort of
poor local documentation but I can't see what else we can do here given
all we have is those names.

> 
> ?
> 
> >   
> > > +
> > > +  semtech,startup-sensor:
> > > +    allOf:
> > > +      - $ref: /schemas/types.yaml#definitions/uint32
> > > +      - enum: [0, 1, 2, 3]
> > > +    default: 0
> > > +    description:
> > > +      Sensor used for start-up proximity detection. The combined
> > > +      sensor is represented by 3.  
> > 
> > This feels like it should be a userspace control rather than in DT?  
> 
> I believe this is used during initial compensation, so it needs to be
> set before sx9310_init_compensation() runs at probe time. Probably can't
> be moved to userspace.

Add that detail to the description here.

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

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

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

> 
> > 
> >   
> > > +
> > > +  semtech,cs0-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 CS0 (and combined if any) sensor.  
> > 
> > That is definitely a userspace thing. Why would you put it in DT?
> > Also same comment as above for channels as child nodes  
> 
> Alright. Presumably this is IIO_EV_TYPE_THRESH?

Yes.

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

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

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

> 
> >   
> > > +
> > > +  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
> > >    - reg  

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

[Rob Herring]

On Thu, Sep 03, 2020 at 03:18:28PM -0700, Stephen Boyd wrote:
> We need to set various bits in the hardware registers for this device to
> operate properly depending on how it is installed. Add a handful of DT
> properties to configure these things.
> 
> Cc: Daniel Campello <campello@chromium.org>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: <devicetree@vger.kernel.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Cc: Evan Green <evgreen@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> 
> I haven't written any code to handle these properties yet. I'd rather do
> that once the binding patch is reviewed. Patch based on iio.git testing
> branch.
> 
>  .../iio/proximity/semtech,sx9310.yaml         | 182 ++++++++++++++++++
>  1 file changed, 182 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> index 5739074d3592..e74b81483c14 100644
> --- a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> +++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> @@ -40,6 +40,169 @@ properties:
>    "#io-channel-cells":
>      const: 1
>  
> +  semtech,cs0-ground:
> +    description: Indicates the CS0 sensor is connected to ground.
> +    type: boolean
> +
> +  semtech,combined-sensors:
> +    allOf:
> +      - $ref: /schemas/types.yaml#definitions/uint32
> +      - enum: [0, 1, 2, 3]
> +    default: 0
> +    description:
> +      Which sensors are combined. 0 for CS3, 1 for CS0+CS1, 2 for CS1+CS2,
> +      and 3 for all sensors.
> +
> +  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.
> +
> +  semtech,cs1-gain-factor:
> +    allOf:
> +      - $ref: /schemas/types.yaml#definitions/uint32
> +      - enum: [1, 2, 4, 8]

Now that everyone is trained on 'allOf', you can drop it. json-schema 
draft8 changed this behavior.

Rob

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

[Stephen Boyd]

Quoting Rob Herring (2020-09-14 14:00:47)
> On Thu, Sep 03, 2020 at 03:18:28PM -0700, Stephen Boyd wrote:
> > +  semtech,cs1-gain-factor:
> > +    allOf:
> > +      - $ref: /schemas/types.yaml#definitions/uint32
> > +      - enum: [1, 2, 4, 8]
> 
> Now that everyone is trained on 'allOf', you can drop it. json-schema 
> draft8 changed this behavior.
> 

Ok. Do I need $ref: still or that is implicit now?

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

[Stephen Boyd]

Quoting Jonathan Cameron (2020-09-09 04:15:50)
> On Tue, 8 Sep 2020 23:18:43 -0700
> Stephen Boyd <swboyd@chromium.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

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

[Stephen Boyd]

Quoting Stephen Boyd (2020-09-23 16:25:43)
> > > > > +
> > > > > +  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?
> 

Or I can assign thresh_period to falling and rising corresponding to
close/far debounce. Seems that the direction is the same, but that can
be split apart and each direction gets a different sysfs file?

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

[Jonathan Cameron]

On Wed, 23 Sep 2020 16:25:43 -0700
Stephen Boyd <swboyd@chromium.org> wrote:

> Quoting Jonathan Cameron (2020-09-09 04:15:50)
> > On Tue, 8 Sep 2020 23:18:43 -0700
> > Stephen Boyd <swboyd@chromium.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?

Exactly.  It's impossible to describe ever combination of shared
attributes so we just do the big versions.  For everything else the
ABI relies on the fact that any change to a value can in theory impact
any other attribute in the same IIO device.

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

Great.

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

Great. That makes life easier.

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

Not totally sure I followed that, but sounds fine.
If close and far are different events, you can have a separate
controls.

Jonathan


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

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

[Jonathan Cameron]

On Thu, 24 Sep 2020 00:57:13 -0700
Stephen Boyd <swboyd@chromium.org> wrote:

> Quoting Stephen Boyd (2020-09-23 16:25:43)
> > > > > > +
> > > > > > +  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?
> >   
> 
> Or I can assign thresh_period to falling and rising corresponding to
> close/far debounce. Seems that the direction is the same, but that can
> be split apart and each direction gets a different sysfs file?
Ah I should have read on.  Yes,
thresh_falling_period and thresh_rising_period is fine.

Jonathan

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

[Stephen Boyd]

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

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

[Jonathan Cameron]

On Fri, 25 Sep 2020 18:17:25 -0700
Stephen Boyd <swboyd@chromium.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@chromium.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