2022-10-24 18:08:01

by Martin Zaťovič

[permalink] [raw]
Subject: [RFCv3 PATCH 0/2] Wiegand bus driver

Thank you for all your suggestions and points.

I have realized that I have approached implementing the Wiegand bus
driver wrong. I have used the words 'GPIO controller', which made it
seem like there was a GPIO controller for which another driver would
be needed. What I meant to implement is a simple bit-banged Wiegand
bus driver.

There is no GPIO controller, instead the two GPIO lines are
controlled by the bus. They are utilized to send messages following
the Wiegand protocol and the bus driver should implement such
bit-banging. Now only a single devicetree entry is needed instead
of two as in the previous version - thanks to Krzysztof for
pointing it out.

The driver was based on similar drivers in the drivers/bus directory.
There is also an abstract API in include/linux/wiegand.h that
contains functions to write to the bus or set a particular format.
This API is meant to be used in drivers of devices communicating
using Wiegand protocol.

The user can write on the bus using a device file or change
the format and message payload length via sysfs files.

The driver was tested on NXP Verdin iMX8MP Plus.

With regards,
Martin Zaťovič

v1->v2:
Split the driver into a bus driver a gpio driver and an abstract API.
Fix issues pointed out in RFC

v2->v3:
Discard the bus driver. Implement new bus driver based on the gpio
driver(the gpio driver was actually implementing the functionality
of the bus).
Fix make dt_bindings_check errors.
Use devicetree properties for pulse_len, interval_len and frame_gap
attributes.

Martin Zaťovič (2):
dt-bindings: bus: add Wiegand bus dt documentation
bus: add bit banged Wiegand bus driver

Documentation/ABI/testing/sysfs-bus-wiegand | 20 +
.../devicetree/bindings/bus/wiegand.yaml | 75 +++
MAINTAINERS | 8 +
drivers/bus/Kconfig | 7 +
drivers/bus/Makefile | 1 +
drivers/bus/wiegand.c | 509 ++++++++++++++++++
include/linux/wiegand.h | 58 ++
7 files changed, 678 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-wiegand
create mode 100644 Documentation/devicetree/bindings/bus/wiegand.yaml
create mode 100644 drivers/bus/wiegand.c
create mode 100644 include/linux/wiegand.h

--
2.37.3


2022-10-24 21:08:10

by Martin Zaťovič

[permalink] [raw]
Subject: [RFCv3 PATCH 1/2] dt-bindings: bus: add Wiegand bus dt documentation

This patch documents the devicetree entry for a Wiegand bus.
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č <[email protected]>
---
.../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
+%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č <[email protected]>
+
+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
+
+ 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.
+ maxItems: 1
+
+required:
+ - compatible
+ - data-hi-gpios
+ - data-lo-gpios
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ wiegand {
+ compatible = "wiegand";
+ data-hi-gpios = <&gpio 7 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
+ data-lo-gpios = <&gpio 6 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
+ pulse-len = <100>;
+ interval-len = <2500>;
+ frame-gap = <4000>;
+ };
+
+...
--
2.37.3

2022-10-24 23:14:00

by Krzysztof Kozlowski

[permalink] [raw]
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č <[email protected]>
> ---
> .../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č <[email protected]>
> +
> +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