| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200906150247.3aaef3a3@archlinux>
Date: Sun, 6 Sep 2020 15:02:47 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Stephen Boyd <swboyd@...omium.org>
Cc: linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
Daniel Campello <campello@...omium.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald-Stadler <pmeerw@...erw.net>,
Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
Douglas Anderson <dianders@...omium.org>,
Gwendal Grignou <gwendal@...omium.org>,
Evan Green <evgreen@...omium.org>
Subject: Re: [PATCH] dt-bindings: iio: sx9310: Add various settings as DT
properties
On Thu, 3 Sep 2020 15:18:28 -0700
Stephen Boyd <swboyd@...omium.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@...omium.org>
> Cc: Lars-Peter Clausen <lars@...afoo.de>
> Cc: Peter Meerwald-Stadler <pmeerw@...erw.net>
> Cc: Rob Herring <robh+dt@...nel.org>
> Cc: <devicetree@...r.kernel.org>
> Cc: Douglas Anderson <dianders@...omium.org>
> Cc: Gwendal Grignou <gwendal@...omium.org>
> Cc: Evan Green <evgreen@...omium.org>
> Signed-off-by: Stephen Boyd <swboyd@...omium.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
Powered by blists - more mailing lists