linux-kernel - Re: [RFCv3 PATCH 1/2] dt-bindings: bus: add Wiegand bus dt documentation

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 24 Oct 2022 17:12:19 -0400
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Martin Zaťovič <m.zatovic1@...il.com>,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        andersson@...nel.org, jeffrey.l.hugo@...il.com,
        Michael.Srba@...nam.cz, gregkh@...uxfoundation.org,
        elder@...aro.org, hemantk@...eaurora.org, mani@...nel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linus.walleij@...aro.org
Subject: Re: [RFCv3 PATCH 1/2] dt-bindings: bus: add Wiegand bus dt
 documentation

On 24/10/2022 12:26, Martin Zaťovič wrote:
> This patch documents the devicetree entry for a Wiegand bus.

Do not use "This commit/patch".
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> A Wiegand bus follows the Wiegand protocol. The bus claims two GPIO
> lines over which the communication is realized. It also introduces
> parameters to control the pulse durations and the length of a gap
> after finishing sending a frame.
> 
> Signed-off-by: Martin Zaťovič <m.zatovic1@...il.com>
> ---
>  .../devicetree/bindings/bus/wiegand.yaml      | 75 +++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/wiegand.yaml
> 
> diff --git a/Documentation/devicetree/bindings/bus/wiegand.yaml b/Documentation/devicetree/bindings/bus/wiegand.yaml
> new file mode 100644
> index 000000000000..cc8d3c46bcde
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/wiegand.yaml
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bus/wiegand.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Wiegand Bus
> +
> +maintainers:
> +  - Martin Zaťovič <m.zatovic1@...il.com>
> +
> +description: |
> +  Wiegand interface is a wiring standard popularized in the 1980s. To this day
> +  many card readers, fingerprint readers, sensors, etc. use Wiegand interface
> +  particularly for access control applications. It utilizes two wires to
> +  transmit the data - D0 and D1.
> +
> +  Both data lines are initially pulled up. To send a bit of value 1, the D1
> +  line is set low. Similarly to send a bit of value 0, the D0 line is set low.
> +
> +properties:
> +  $nodename:
> +    pattern: "^wiegand(@.*|-[0-9a-f])*$"
> +
> +  compatible:
> +    contains:
> +      const: wiegand

I think you are still making some shortcuts in design and this also
causes some misunderstanding.

If this is a bus, then there should be child device(s). But there is
none. If there is no device, how are you going to use this? Imagine now
a SPI controller without child device...

If this is a bus, then one controller implementing it might be using
GPIOs, but other one might have dedicated lines. Unless it's not
possible and all Wiegand implementations use GPIOs? It does not sound right.

> +
> +  data-hi-gpios:
> +    description: GPIO spec for data-hi line to use. This line is initially
> +      pulled up to high value. Wiegand write of a bit of value 1 results in
> +      this line being pulled down for pulse length duration.
> +    maxItems: 1
> +
> +  data-lo-gpios:
> +    description: GPIO spec for data-lo line to use. This line is initially
> +      pulled up to high value. Wiegand write of a bit of value 0 results in
> +      this line being pulled down for pulse length duration.
> +    maxItems: 1
> +
> +  pulse-len:
> +    description: length of the low pulse for both data-lo and data-hi lines.
> +    maxItems: 1
> +
> +  interval-len:
> +    description: length of a whole bit (both the pulse and the high phase) for
> +      both data-hi and data-lo lines in usecs; defaults to 2000us.
> +    maxItems: 1
> +
> +  frame-gap:
> +    description: length of the last bit of a frame (both the pulse and the high
> +      phase) in usec; defaults to 2000us.

For these properties you must use standard units.

https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

Best regards,
Krzysztof