Re: [PATCH v3 1/2] dt-bindings:iio:proximity: Add hx9023s binding

[Jonathan Cameron <jic23@kernel.org>]

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,.*":