linux-kernel - Re: [PATCH 1/2] dt-bindings: hwmon: Add D3-323-AA

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <wy7nyg3cztixe5y5rg4kbsbbly32h547hwumwwvrfme4fdgsj5@znfpypleebrb>
Date: Fri, 13 Dec 2024 12:06:27 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Hermes Zhang <Hermes.Zhang@...s.com>
Cc: jdelvare@...e.com, linux@...ck-us.net, robh@...nel.org, 
	krzk+dt@...nel.org, conor+dt@...nel.org, kernel@...s.com, 
	linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] dt-bindings: hwmon: Add D3-323-AA

On Thu, Dec 12, 2024 at 12:24:09PM +0800, Hermes Zhang wrote:
> Add Devicetree binding documentation for Nicera D3-323-AA Pyroelectric
> IR sensor.
> 
> Signed-off-by: Hermes Zhang <Hermes.Zhang@...s.com>
> ---
>  .../bindings/hwmon/nicera,d3-323-aa.yaml      | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/nicera,d3-323-aa.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/nicera,d3-323-aa.yaml b/Documentation/devicetree/bindings/hwmon/nicera,d3-323-aa.yaml
> new file mode 100644
> index 000000000000..31690e630b5f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/nicera,d3-323-aa.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/nicera,d3-323-aa.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nicera D3-323-AA Pyroelectric IR sensor
> +
> +maintainers:
> +  - Hermes Zhang <Hermes.Zhang@...s.com>
> +
> +description: |
> +  Nicera D3-323-AA Pyroelectric IR sensor
> +
> +  datasheet:
> +    https://www.nicera.co.jp/wordpress/wp-content/uploads/2022/01/D3-323-AA_e.pdf
> +
> +properties:
> +  compatible:
> +    const: nicera,d3-323-aa

Undocumented vendor prefix.

It does not look like you tested the bindings, at least after quick
look. Please run 'make dt_binding_check' (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint. Don't rely on
distro packages for dtschema and be sure you are using the latest
released dtschema.

It does not look like you tested the DTS against bindings. Please run
'make dtbs_check W=1' (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
Maybe you need to update your dtschema and yamllint. Don't rely on
distro packages for dtschema and be sure you are using the latest
released dtschema.

> +
> +  reset-gpios:
> +    description: The GPIO pin connected to the reset pin on the sensor
> +    maxItems: 1
> +
> +  clk-gpios:
> +    description: The GPIO pin connected to the clk pin on the sensor

What is a clk pin? Usually clock pins are clocks, not GPIOs. This needs
explanation

> +    maxItems: 1
> +
> +  si-gpios:
> +    description: The GPIO pin connected to the si pin on the sensor

In all description please drop redundant pieces. It cannot be anything
else than GPIO pin and cannot be connected to something else than the
sensor.

So basically the only useful information you wrote above - after
dropping obvious and redundant pieces - is "si".

Write something useful, what is "si"? This applies also to "clk".
reset-gpios can be without description, because it is obvious - cannot
be anything else than reset pin. OTOH, you could say if pin is active
low or high.

> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reset-gpios
> +  - clk-gpios
> +  - si-gpios
> +

Just one blank line.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    sensor {
> +        compatible = "nicera,d3-323-aa";
> +        reset-gpios = <&gpio4 12 0>;

Include the header and use standard defines for GPIO flags.

Best regards,
Krzysztof