Re: [PATCH 4/5] dt-bindings: iio: Add sx9324 binding

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 4 Nov 2021 00:16:13 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> On Sat, Oct 30, 2021 at 10:20 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sat, 30 Oct 2021 04:18:26 -0700
> > Gwendal Grignou <gwendal@chromium.org> wrote:
> >  
> > > Similar to SX9310, add biddings to setup sx9324 hardware properties.
> > > SX9324 is a little different, introduce 4 phases to be configured in 2
> > > pairs over 3 antennas.
> > >
> > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>  
> > This has a high degree of black magic.
> >
> > I'm curious - is there a fairly heavy weight userspace library interpreting
> > the various readings?  If so I don't suppose it's available anywhere to look at?
> > If you can point to any public info on this part that would also be great.  
> Actually, user space does not care about the measurement once the
> sensor is set up correctly to match its antennas (gains, thresholds,
> hysteresis). They are only used during development and there is a
> little of black magic at that point.
> Then user spaces care about the events (far/close) generated by the
> sensor. See https://chromium.googlesource.com/chromiumos/platform2/+/main/power_manager/powerd/system/user_proximity_watcher.cc#47
> 

Fair enough.  I'll just remember this as 'black magic' :)

> >
> > Thanks,
> >
> > Jonathan
> >  
> > > ---
> > >  .../iio/proximity/semtech,sx9324.yaml         | 141 ++++++++++++++++++
> > >  1 file changed, 141 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> > > new file mode 100644
> > > index 0000000000000..fe9edf15c16d4
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> > > @@ -0,0 +1,141 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/proximity/semtech,sx9310.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Semtech's SX9324 capacitive proximity sensor
> > > +
> > > +maintainers:
> > > +  - Gwendal Grignou <gwendal@chromium.org>
> > > +  - Daniel Campello <campello@chromium.org>
> > > +
> > > +description: |
> > > +  Semtech's SX9324 proximity sensor.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - semtech,sx9324x
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    description:
> > > +      The sole interrupt generated by the device used to announce the
> > > +      preceding reading request has finished and that data is
> > > +      available or that a close/far proximity event has happened.  
> > No need to say sole given maxItems: 1
> > Perhaps...
> >
> >         Generated by device to announce preceding read request has finished
> >         and data is available or that a close/far proximity event has happened.
> >  
> Done.
> > > +    maxItems: 1
> > > +
> > > +  vdd-supply:
> > > +    description: Main power supply
> > > +
> > > +  svdd-supply:
> > > +    description: Host interface power supply
> > > +
> > > +  "#io-channel-cells":
> > > +    const: 1  
> >
> > Curious -  Do we have consumers of this?  
> I recopied them from sx9310. It looks standard for i2c devices.

Should only be the case for devices that have or are likely to have
in kernel IIO consumers.  Doesn't do any harm though and might be useful to
someone so fine to leave it.


> >  
> > > +
> > > +  semtech,ph0-pin:
> > > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > > +    description: |
> > > +      Indicates how each CS pin is used during phase 0.
> > > +      Each of the 3 pins have the following value -
> > > +      0 : unused (high impedance)
> > > +      1 : measured input
> > > +      2 : dynamic shield
> > > +      3 : grounded.
> > > +      For instance, CS0 measured, CS1 shield and CS2 ground is [1, 2, 3]
> > > +    items:
> > > +      enum: [ 0, 1, 2, 3 ]
> > > +    minItems: 3
> > > +    maxItems: 3
> > > +
> > > +  semtech,ph1-pin:
> > > +  semtech,ph2-pin:
> > > +  semtech,ph3-pin:
> > > +    Same as ph0-pin  
> >
> > I'm curious - why would you chose different combinations?  I guess because of
> > different wiring.  If that's the case, is there a documented 'right' choice
> > for a given wiring.   I'm just wondering if we are better off describing
> > what is connected to these pins and having the driver pick a valid control
> > sequence.  That might simplify things.  If not then fair enough.  
> I explained in Documentation/ABI/testing/sysfs-bus-iio-sx9324 in the
> previous patch.
> The BIOS would communicate how the input pins are wired.

So my reading here is that isn't quite true.  The BIOS is communicating
what state we should put the pins into which is a function of how they
are wired, but not a direct indication of that wiring.

From point of view of someone using this device, I'd really want
to be able to specify pin 1 to this part of the antenna etc and have
the driver work out the settings for each step in the sequence that would
work.

If we don't have that information, or it is complex to map to a sequence then
I can live with what you have here.

> >  
> > > +
> > > +  semtech,resolution01
> > > +    $ref: /schemas/types.yaml#definitions/uint32
> > > +    enum: [0, 1, 2, 3, 4, 5, 6, 7]
> > > +    description:
> > > +      Capacitance measurement resolution. For phase 0 and 1.
> > > +      Higher the number, higher the resolution.  
> >
> > Can we not give something more meaningful than high is higher?
> > I don't have the datasheet for this part and the 9330 that I'm
> > looking at is rather unhelpful on this.  I have no idea how anyone
> > using that datasheet would figure out what to set this to!  
> I don't have more info. It is tweaked during setup.

Ah well - magic tweak it is :)

> 
> >  
> > > +    default: 4
> > > +
> > > +  semtech,resolution23
> > > +    $ref: /schemas/types.yaml#definitions/uint32
> > > +    enum: [0, 1, 2, 3, 4, 5, 6, 7]
> > > +    description:
> > > +      Capacitance measurement resolution. For phase 2 and 3
> > > +    default: 4
> > > +
> > > +  semtech,startup-sensor:
> > > +    $ref: /schemas/types.yaml#definitions/uint32
> > > +    enum: [0, 1, 2, 3]
> > > +    default: 0
> > > +    description:
> > > +      Phase used for start-up proximity detection.
> > > +      It is used when we enable a phase to remove static offset and measure
> > > +      only capacitance changes introduced by the user.
> > > +
> > > +  semtech,proxraw-strength01:
> > > +    $ref: /schemas/types.yaml#definitions/uint32
> > > +    enum: [0, 1, 2, 3, 4, 5, 6, 7]
> > > +    default: 1
> > > +    description:
> > > +      PROXRAW filter strength for phase 0 and 1. A value of 0 represents off,i
> > > +      and other values represent 1-1/N.
> > > +
> > > +  semtech,proxraw-strength23:
> > > +    $ref: /schemas/types.yaml#definitions/uint32
> > > +    enum: [0, 1, 2, 3, 4, 5, 6, 7]
> > > +    default: 1
> > > +    description:
> > > +      PROXRAW filter strength for phase 2 and 3. A value of 0 represents off,i
> > > +      and other values represent 1-1/N.
> > > +
> > > +  semtech,avg-pos-strength:
> > > +    $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.
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - "#io-channel-cells"
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +    i2c {
> > > +      #address-cells = <1>;
> > > +      #size-cells = <0>;
> > > +      proximity@28 {
> > > +        compatible = "semtech,sx9310";
> > > +        reg = <0x28>;
> > > +        interrupt-parent = <&pio>;
> > > +        interrupts = <5 IRQ_TYPE_LEVEL_LOW 5>;
> > > +        vdd-supply = <&pp3300_a>;
> > > +        svdd-supply = <&pp1800_prox>;
> > > +        #io-channel-cells = <1>;
> > > +        semtech,ph0-pin = <1, 2, 3>;
> > > +        semtech,ph1-pin = <3, 2, 1>;
> > > +        semtech,ph2-pin = <1, 2, 3>;
> > > +        semtech,ph3-pin = <3, 2, 1>;
> > > +        semtech,resolution01 = 2;
> > > +        semtech,resolution23 = 2;
> > > +        semtech,startup-sensor = <1>;
> > > +        semtech,proxraw-strength01 = <2>;
> > > +        semtech,proxraw-strength23 = <2>;
> > > +        semtech,avg-pos-strength = <64>;
> > > +      };
> > > +    };  
> >