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

[Jonathan Cameron <jic23@kernel.org>]

On Fri,  7 Jun 2024 19:41:37 +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 improvements but seems you missed some of the feedback on v3.

See inline.

Jonathan

> ---
>  .../bindings/iio/proximity/tyhx,hx9023s.yaml  | 103 ++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
>  2 files changed, 105 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..50bf2849d823
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/tyhx,hx9023s.yaml
> @@ -0,0 +1,103 @@
> +# 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

A device like this needs at least one power supply.  Make sure to document
all such supplies and make the ones that are required for functionality part of
your required properties.  Note that you should do this even if on your
board they are always turned on.

> +
> +  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:
> +    true
  vdd-supply: true

on single line is commonly done for these.

> +
> +  channel-in-use:
> +    description: |
> +      Bit flag indicating which channels are used,
> +      depends on the hardware circuit design.
> +    $ref: /schemas/types.yaml#/definitions/uint32

Presence of the channel nodes below should make this clear
without a separate element.


> +
> +patternProperties:
> +  "^channel@[0-9]+$":
> +    type: object
> +    properties:
> +      reg:
> +        description: Channel register address
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +      channel-positive:
> +        description: Positive channel assignments
> +        $ref: /schemas/types.yaml#/definitions/uint32

That size seems implausible.  What are the limits. What does
255 mean?

In review of previous version I pointed you at the differential
channel bindings for ADCs.  If they cannot be applied here
explain why in your patch description.

> +      channel-negative:
> +        description: Negative channel assignments
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +    required:
> +      - reg
> +      - channel-positive
> +      - channel-negative
> +
> +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>;
> +        channel-in-use = <0x1F>;
> +        channel@0 {
> +          reg = <0>;
> +          channel-positive = <0>;
> +          channel-negative = <255>;
> +        };
> +        channel@1 {
> +          reg = <1>;
> +          channel-positive = <1>;
> +          channel-negative = <255>;
> +        };
> +        channel@2 {
> +          reg = <2>;
> +          channel-positive = <2>;
> +          channel-negative = <255>;
> +        };
> +        channel@3 {
> +          reg = <3>;
> +          channel-positive = <3>;
> +          channel-negative = <255>;
> +        };
> +        channel@4 {
> +          reg = <4>;
> +          channel-positive = <4>;
> +          channel-negative = <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.

Use a separate patch for the new vendor prefix.  Makes it easier for people to cherrypick that
if they are backporting some other tyhx dt binding.

>    "^u-blox,.*":
>      description: u-blox
>    "^u-boot,.*":