| Message ID | SN7PR12MB8101FD306CBFC84DDF3D2466A4F22@SN7PR12MB8101.namprd12.prod.outlook.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio-proximity-hx9023s-Add-TYHX-HX9023S-sensor-driver | expand |
On 29/05/2024 06:57, Yasin Lee wrote: > From: Yasin Lee <yasin.lee.x@gmail.com> > > A capacitive proximity sensor > Nothing improved. You ignored all previous comments. Best regards, Krzysztof
On Wed, 29 May 2024 12:57:48 +0800 Yasin Lee <yasin.lee.x@outlook.com> wrote: > From: Yasin Lee <yasin.lee.x@gmail.com> > > A capacitive proximity sensor > > Signed-off-by: Yasin Lee <yasin.lee.x@gmail.com> Hi Yasin, Some comments inline. A lot of what you have here sounds like it should be under userspace control, not in the dt-binding. Also, look at how we handled optional channels for ADCs and copy that approach here. Jonathan > --- > .../bindings/iio/proximity/tyhx,hx9023s.yaml | 106 ++++++++++++++++++ > .../devicetree/bindings/vendor-prefixes.yaml | 2 + > 2 files changed, 108 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/proximity/tyhx,hx9023s.yaml > > diff --git a/Documentation/devicetree/bindings/iio/proximity/tyhx,hx9023s.yaml b/Documentation/devicetree/bindings/iio/proximity/tyhx,hx9023s.yaml > new file mode 100644 > index 000000000000..ba4d7343bb30 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/proximity/tyhx,hx9023s.yaml > @@ -0,0 +1,106 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/proximity/tyhx,hx9023s.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: TYHX HX9023S capacitive proximity sensor > + > +maintainers: > + - Yasin Lee <yasin.lee.x@gmail.com> > + > +description: | > + TYHX HX9023S proximity sensor > + > +allOf: > + - $ref: /schemas/iio/iio.yaml# > + > +properties: > + compatible: > + const: tyhx,hx9023s > + > + reg: > + maxItems: 1 > + > + interrupts: > + description: | > + Generated by device to announce preceding read request has finished > + and data is available or that a close/far proximity event has happened. > + maxItems: 1 > + > + vdd-supply: > + description: Main power supply vdd-supply: true is enough. vdd is pretty much always used for the main power supply so the docs are are redundant. > + > + accuracy: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: Accuracy level of the sensor No idea what this means! > + > + channel-used-flag: > + description: Bit flag indicating which channels are used > + $ref: /schemas/types.yaml#/definitions/uint32 As below - subnodes not a bunch of arrays. If the subnode is there it should be used. > + > + odr: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: Set ODR for all channenls. I assume output data rate? That's something for userspace not the DT binding. > + > + integration-sample: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: Set Integration number and Sample number for each channenl. Ok. This one might possibly be hardware related as it depends on the wiring and physical elements of the board. If so give a good description on how this should be set. > + > + osr: > + $ref: /schemas/types.yaml#/definitions/uint32-array > + description: Set number of OSR for each channenl. Expand the acronym. This sounds like oversampling ratio which would be odd to see alongside an average control. Hence more detail needed and consider if it should be controlled by the dt binding. > + > + avg: > + $ref: /schemas/types.yaml#/definitions/uint32-array > + description: Set number of AVG for each channenl. This sounds like oversampling? If so that is normally a userspace problem not a binding thing. Is it related to the physical wiring? If not don't put it in the binding. > + > + lp-alpha: > + $ref: /schemas/types.yaml#/definitions/uint32-array > + description: Set lp-alpha for each channenl. Spell check. Also this needs more description. > + > + cs-position: > + $ref: /schemas/types.yaml#/definitions/uint32-array > + description: | > + Position of the CS pins. > + Indicates the corresponding bit for each CS pin in the register. I've no idea what this. Add more description. Normally CS is chip select and there is only one of those. Also this not general dt binding material so vendor prefix this stuff. > + > + channel-positive: Use per channel nodes. There are examples in for example adc/adc.yaml on how to do this. Not having a child node is a lot easier to follow than magic values to say something isn't there. From what is here these look like differential channels. Use the adc style binding for that. > + $ref: /schemas/types.yaml#/definitions/uint32-array > + description: Positive channel assignments. Use 255 for not connected > + > + channel-negative: > + $ref: /schemas/types.yaml#/definitions/uint32-array > + description: | > + Negative channel assignments. Use 255 for not connected > + > +required: > + - compatible > + - reg > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + hx9023s@2a { As was pointed out in earlier patch review - generic node names only. > + compatible = "tyhx,hx9023s"; > + reg = <0x2a>; > + interrupt-parent = <&pio>; > + interrupts = <16 IRQ_TYPE_EDGE_FALLING>; > + vdd-supply = <&pp1800_prox>; > + accuracy = <16>; > + channel-used-flag = <0x1F>; > + odr = <0x17>; > + integration-sample = <0x0065>; > + osr = <0x4 0x4 0x4 0x0 0x0>; > + avg = <0x3 0x3 0x3 0x0 0x0>; > + lp-alpha = <0x8 0x8 0x8 0x8 0x2>; > + cs-position = <0 2 4 6 8>; > + channel-positive = <0 1 2 3 4>; > + channel-negative = <255 255 255 255 255>; > + }; > + }; > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml > index b97d298b3eb6..e2224eea9ab9 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml > +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml > @@ -1507,6 +1507,8 @@ patternProperties: > description: Turing Machines, Inc. > "^tyan,.*": > description: Tyan Computer Corporation > + "^tyhx,.*": > + description: NanjingTianyihexin Electronics Ltd. Convention is separate patch for vendor prefix editions. Makes it easier for people to cherrypick them for a different device driver binding. > "^u-blox,.*": > description: u-blox > "^u-boot,.*":
diff --git a/Documentation/devicetree/bindings/iio/proximity/tyhx,hx9023s.yaml b/Documentation/devicetree/bindings/iio/proximity/tyhx,hx9023s.yaml new file mode 100644 index 000000000000..ba4d7343bb30 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/proximity/tyhx,hx9023s.yaml @@ -0,0 +1,106 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/proximity/tyhx,hx9023s.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: TYHX HX9023S capacitive proximity sensor + +maintainers: + - Yasin Lee <yasin.lee.x@gmail.com> + +description: | + TYHX HX9023S proximity sensor + +allOf: + - $ref: /schemas/iio/iio.yaml# + +properties: + compatible: + const: tyhx,hx9023s + + reg: + maxItems: 1 + + interrupts: + description: | + Generated by device to announce preceding read request has finished + and data is available or that a close/far proximity event has happened. + maxItems: 1 + + vdd-supply: + description: Main power supply + + accuracy: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Accuracy level of the sensor + + channel-used-flag: + description: Bit flag indicating which channels are used + $ref: /schemas/types.yaml#/definitions/uint32 + + odr: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Set ODR for all channenls. + + integration-sample: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Set Integration number and Sample number for each channenl. + + osr: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: Set number of OSR for each channenl. + + avg: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: Set number of AVG for each channenl. + + lp-alpha: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: Set lp-alpha for each channenl. + + cs-position: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: | + Position of the CS pins. + Indicates the corresponding bit for each CS pin in the register. + + channel-positive: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: Positive channel assignments. Use 255 for not connected + + channel-negative: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: | + Negative channel assignments. Use 255 for not connected + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + hx9023s@2a { + compatible = "tyhx,hx9023s"; + reg = <0x2a>; + interrupt-parent = <&pio>; + interrupts = <16 IRQ_TYPE_EDGE_FALLING>; + vdd-supply = <&pp1800_prox>; + accuracy = <16>; + channel-used-flag = <0x1F>; + odr = <0x17>; + integration-sample = <0x0065>; + osr = <0x4 0x4 0x4 0x0 0x0>; + avg = <0x3 0x3 0x3 0x0 0x0>; + lp-alpha = <0x8 0x8 0x8 0x8 0x2>; + cs-position = <0 2 4 6 8>; + channel-positive = <0 1 2 3 4>; + channel-negative = <255 255 255 255 255>; + }; + }; diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml index b97d298b3eb6..e2224eea9ab9 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml @@ -1507,6 +1507,8 @@ patternProperties: description: Turing Machines, Inc. "^tyan,.*": description: Tyan Computer Corporation + "^tyhx,.*": + description: NanjingTianyihexin Electronics Ltd. "^u-blox,.*": description: u-blox "^u-boot,.*":