[PATCHv5 0/4] Wiegand bus driver and GPIO bitbanged controller

[PATCHv5 0/4] Wiegand bus driver and GPIO bitbanged controller

[Martin Zaťovič]

Hello,

I have fixed all the pointed out issues and I present the next version
of Wiegand bus driver and Wiegand GPIO bitbanged controller. The most
noticable change is the change of the input data format, which is now
ASCII hex string. Please let me know, if you see any issues.

I have also included a brief information about Wiegand in the commit
message of the commit adding the bus driver, since there is no official
datasheet that I could link.

CHANGELOG:
wiegand.c
- kernel symbols are now part of WIEGAND namespace
- added includes for conatiner_of.h and types.h
- made IDA static
- removed redundant null checks and error messages
- added overflow check for memory allocation size
- used devm_add_action_or_reset() for controller allocation
- fixed comments according to kernel doc validator
- avoided dereferencing fwnode in struct device by using device_set_node()
- fixed fwnode format specifier
- removed the overly complicated system for getting timings and replaced it
  with a clear and easily understandable one
- unified the goto label names
- removed static keyword from struct bus_type
- used bitmap_parse_user() function to parse the user data, which also means
  that the input format has changed to ASCII hex string
- removed input length checks as they are not valid anymore using the new input
  format

wiegand.h
- removed unnecesary mod_devicetable.h include and added container_of.h and
  types.h includes
- added fs.h header for struct file_operations
- removed underscores in some variables names within macros

wiegand-gpio.c
- fixed some error codes
- used fsleep() in the wiegand_gpio_send_bit() function
- edited the loop in wiegand_gpio_write_by_bits() so that it doesnt check for
  the last bit in each iteration
- used the devm_gpiod_get_array() for acquiring the two gpio lines
- imported WIEGAND namespace

wiegand-gpio.yaml
- dropped devicetree bindings

Martin Zaťovič (4):
  dt-bindings: wiegand: add Wiegand controller common properties
  wiegand: add Wiegand bus driver
  dt-bindings: wiegand: add GPIO bitbanged Wiegand controller
  wiegand: add Wiegand GPIO bitbanged controller driver

 .../ABI/testing/sysfs-driver-wiegand-gpio     |   9 +
 .../bindings/wiegand/wiegand-controller.yaml  |  39 ++
 .../bindings/wiegand/wiegand-gpio.yaml        |  46 ++
 MAINTAINERS                                   |  14 +
 drivers/Kconfig                               |   2 +
 drivers/Makefile                              |   1 +
 drivers/wiegand/Kconfig                       |  36 ++
 drivers/wiegand/Makefile                      |   2 +
 drivers/wiegand/wiegand-gpio.c                | 192 ++++++
 drivers/wiegand/wiegand.c                     | 590 ++++++++++++++++++
 include/linux/wiegand.h                       | 148 +++++
 11 files changed, 1079 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-wiegand-gpio
 create mode 100644 Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
 create mode 100644 Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml
 create mode 100644 drivers/wiegand/Kconfig
 create mode 100644 drivers/wiegand/Makefile
 create mode 100644 drivers/wiegand/wiegand-gpio.c
 create mode 100644 drivers/wiegand/wiegand.c
 create mode 100644 include/linux/wiegand.h

-- 
2.40.1

[PATCHv5 1/4] dt-bindings: wiegand: add Wiegand controller common properties

[Martin Zaťovič]

Wiegand bus is defined by a Wiegand controller node. This node
can contain one or more device nodes for devices attached to
the controller(it is advised to only connect one device as Wiegand
is a point-to-point bus).

Wiegand controller needs to specify several attributes such as
the pulse length in order to function properly. These attributes
are documented here.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Martin Zaťovič <m.zatovic1@gmail.com>
---
 .../bindings/wiegand/wiegand-controller.yaml  | 39 +++++++++++++++++++
 MAINTAINERS                                   |  5 +++
 2 files changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml

diff --git a/Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml b/Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
new file mode 100644
index 000000000000..8f36287e4fed
--- /dev/null
+++ b/Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/wiegand/wiegand-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Wiegand Generic Controller Common Properties
+
+maintainers:
+  - Martin Zaťovič <m.zatovic1@gmail.com>
+
+description:
+  Wiegand busses can be described with a node for the Wiegand controller device
+  and a set of child nodes for each SPI slave on the bus.
+
+properties:
+  $nodename:
+    pattern: "^wiegand(@.*|-[0-9a-f])?$"
+
+  pulse-len-us:
+    description:
+      Length of the low pulse in microseconds.
+
+  interval-len-us:
+    description:
+      Length of a whole bit (both the pulse and the high phase) in microseconds.
+
+  frame-gap-us:
+    description:
+      Length of the last bit of a frame (both the pulse and the high phase) in
+      microseconds.
+
+required:
+  - compatible
+  - pulse-len-us
+  - interval-len-us
+  - frame-gap-us
+
+additionalProperties: true
diff --git a/MAINTAINERS b/MAINTAINERS
index d590ce31aa72..75462d3746ca 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22922,6 +22922,11 @@ L:	linux-input@vger.kernel.org
 S:	Maintained
 F:	drivers/hid/hid-wiimote*
 
+WIEGAND BUS DRIVER
+M:	Martin Zaťovič <m.zatovic1@gmail.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
+
 WILOCITY WIL6210 WIRELESS DRIVER
 L:	linux-wireless@vger.kernel.org
 S:	Orphan
-- 
2.40.1

Re: [PATCHv5 1/4] dt-bindings: wiegand: add Wiegand controller common properties

[Rob Herring]

On Thu, Aug 24, 2023 at 01:10:12PM +0200, Martin Zaťovič wrote:
> Wiegand bus is defined by a Wiegand controller node. This node
> can contain one or more device nodes for devices attached to
> the controller(it is advised to only connect one device as Wiegand
> is a point-to-point bus).

How would multiple devices work? Seems like you'd need some sort of mux 
which would be another node. If there's not really any need, then I'd 
just say it's only a single device. Either way the binding needs to 
define this.

> 
> Wiegand controller needs to specify several attributes such as
> the pulse length in order to function properly. These attributes
> are documented here.
> 
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Martin Zaťovič <m.zatovic1@gmail.com>
> ---
>  .../bindings/wiegand/wiegand-controller.yaml  | 39 +++++++++++++++++++
>  MAINTAINERS                                   |  5 +++
>  2 files changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
> 
> diff --git a/Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml b/Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
> new file mode 100644
> index 000000000000..8f36287e4fed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
> @@ -0,0 +1,39 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/wiegand/wiegand-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Wiegand Generic Controller Common Properties
> +
> +maintainers:
> +  - Martin Zaťovič <m.zatovic1@gmail.com>
> +
> +description:
> +  Wiegand busses can be described with a node for the Wiegand controller device
> +  and a set of child nodes for each SPI slave on the bus.

Some more detail on what Wiegand is would be useful. Link to spec? What 
it is used for?

> +
> +properties:
> +  $nodename:
> +    pattern: "^wiegand(@.*|-[0-9a-f])?$"

The '-[0-9a-f]' suffix should be decimal rather than hex.

> +
> +  pulse-len-us:
> +    description:
> +      Length of the low pulse in microseconds.
> +
> +  interval-len-us:
> +    description:
> +      Length of a whole bit (both the pulse and the high phase) in microseconds.
> +
> +  frame-gap-us:
> +    description:
> +      Length of the last bit of a frame (both the pulse and the high phase) in
> +      microseconds.

If you have multiple devices, you need to define the child node format. 
Specifically, you need addresses for multiple devices. So you need to 
define the unit-address format, #address-cells and #size-cells values, 
and any constraints on 'reg' such as max address and/or number of entries.

Rob

Re: [PATCHv5 1/4] dt-bindings: wiegand: add Wiegand controller common properties

[Martin Zaťovič]

On Thu, Aug 24, 2023 at 08:37:01AM -0500, Rob Herring wrote:
> On Thu, Aug 24, 2023 at 01:10:12PM +0200, Martin Zaťovič wrote:
> > Wiegand bus is defined by a Wiegand controller node. This node
> > can contain one or more device nodes for devices attached to
> > the controller(it is advised to only connect one device as Wiegand
> > is a point-to-point bus).
> 
> How would multiple devices work? Seems like you'd need some sort of mux 
> which would be another node. If there's not really any need, then I'd 
> just say it's only a single device. Either way the binding needs to 
> define this.
> 
> > 
> > Wiegand controller needs to specify several attributes such as
> > the pulse length in order to function properly. These attributes
> > are documented here.
> > 
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Martin Zaťovič <m.zatovic1@gmail.com>
> > ---
> >  .../bindings/wiegand/wiegand-controller.yaml  | 39 +++++++++++++++++++
> >  MAINTAINERS                                   |  5 +++
> >  2 files changed, 44 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml b/Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
> > new file mode 100644
> > index 000000000000..8f36287e4fed
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
> > @@ -0,0 +1,39 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/wiegand/wiegand-controller.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Wiegand Generic Controller Common Properties
> > +
> > +maintainers:
> > +  - Martin Zaťovič <m.zatovic1@gmail.com>
> > +
> > +description:
> > +  Wiegand busses can be described with a node for the Wiegand controller device
> > +  and a set of child nodes for each SPI slave on the bus.
> 
> Some more detail on what Wiegand is would be useful. Link to spec? What 
> it is used for?

There is no official Wiegand datasheet out there. I can include the summary of
some proprietary documents about Wiegand, like the commit message of the second
commit in the series.

> 
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: "^wiegand(@.*|-[0-9a-f])?$"
> 
> The '-[0-9a-f]' suffix should be decimal rather than hex.
> 
> > +
> > +  pulse-len-us:
> > +    description:
> > +      Length of the low pulse in microseconds.
> > +
> > +  interval-len-us:
> > +    description:
> > +      Length of a whole bit (both the pulse and the high phase) in microseconds.
> > +
> > +  frame-gap-us:
> > +    description:
> > +      Length of the last bit of a frame (both the pulse and the high phase) in
> > +      microseconds.
> 
> If you have multiple devices, you need to define the child node format. 
> Specifically, you need addresses for multiple devices. So you need to 
> define the unit-address format, #address-cells and #size-cells values, 
> and any constraints on 'reg' such as max address and/or number of entries.

I see.. the bus should really only allow one device to be connected.
That means, there is no need to define anything else in this file, right?
I will explicitely mention it in the Description and I will add the

unevaluatedPropertied:
  type: object

to the wiegand-gpio.yaml file so that adding a child node is allowed.

> 
> Rob

With regards,
Martin Zaťovič

[PATCHv5 2/4] wiegand: add Wiegand bus driver

[Martin Zaťovič]

The Wiegand protocol serves as a standardized interface protocol, extensively
employed within electronic access control systems, to facilitate data exchange
between credentials, readers, and door controllers. Its inception can be
attributed to the widespread adoption of Wiegand card readers, leveraging the
Wiegand effect - a physical phenomenon wherein a Wiegand wire (or card)
generates small yet distinct magnetic fields. A Wiegand reader detects the
magnetic pulses emitted by the internal wires of the card.

Three wires are central to the Wiegand protocol: a common ground wire and two
distinct data wires designated as DATA0 and DATA1. During periods of inactivity,
both DATA0 and DATA1 lines remain pulled up. For transmitting a '0,' the DATA0
line is pulled down while DATA1 remains pulled up; conversely, transmitting
a '1' causes DATA1 to be pulled down while DATA0 remains pulled up. Notably,
this protocol ensures that the two lines never simultaneously experience a low
state.

Timing characteristics within the Wiegand protocol lack a uniform
standardization, introducing variability between devices. Generally, pulse
durations hover between 50 to 100 microseconds, while inter-pulse gaps span 20
to 100 milliseconds. There is no stop bit or similar delimiter to signal the
conclusion of a message. Instead, the receiver either counts the bits within the
message or enforces a timeout, often set at around ten times the inter-pulse gap
duration.

The 26-Bit Wiegand Interface Standard, or 26-Bit Wiegand Format outlines the
message format most commonly used among Wiegand devices. This format allocates
the initial and terminal bits for parity. The subsequent eight bits following
the initial parity bit are reserved for the Facility Code designed for distinct
location identification. The remaining bits house the unique ID number. As the
technology evolved, new Wiegand formats emerged, including 36-bit and 37-bit
formats. It was also common practice for manufacturers to engineer devices
compatible with proprietary Wiegand formats tailored to their specifications.

The Wiegand bus driver handles Wiegand controller and Wiegand device
managemement and driver matching. The bus driver defines the structures for
Wiegand controllers and Wiegand devices. Wiegand controller structure contains
all attributes that define the communication such as the payload_len for
configuring the size of a single Wiegand message in bits, thus choosing a
format. Each Wiegand controller should be associated with one Wiegand device,
as Wiegand is typically a point-to-point bus.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Martin Zaťovič <m.zatovic1@gmail.com>
---
 MAINTAINERS               |   2 +
 drivers/Kconfig           |   2 +
 drivers/Makefile          |   1 +
 drivers/wiegand/Kconfig   |  25 ++
 drivers/wiegand/Makefile  |   1 +
 drivers/wiegand/wiegand.c | 590 ++++++++++++++++++++++++++++++++++++++
 include/linux/wiegand.h   | 148 ++++++++++
 7 files changed, 769 insertions(+)
 create mode 100644 drivers/wiegand/Kconfig
 create mode 100644 drivers/wiegand/Makefile
 create mode 100644 drivers/wiegand/wiegand.c
 create mode 100644 include/linux/wiegand.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 75462d3746ca..0afcc88a38d2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22926,6 +22926,8 @@ WIEGAND BUS DRIVER
 M:	Martin Zaťovič <m.zatovic1@gmail.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
+F:	drivers/wiegand/wiegand.c
+F:	include/linux/wiegand.h
 
 WILOCITY WIL6210 WIRELESS DRIVER
 L:	linux-wireless@vger.kernel.org
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 514ae6b24cb2..6609dfd6635f 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -145,6 +145,8 @@ source "drivers/vdpa/Kconfig"
 
 source "drivers/vhost/Kconfig"
 
+source "drivers/wiegand/Kconfig"
+
 source "drivers/hv/Kconfig"
 
 source "drivers/xen/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 7241d80a7b29..898ec50cfcc4 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -150,6 +150,7 @@ obj-$(CONFIG_VHOST_RING)	+= vhost/
 obj-$(CONFIG_VHOST_IOTLB)	+= vhost/
 obj-$(CONFIG_VHOST)		+= vhost/
 obj-$(CONFIG_VLYNQ)		+= vlynq/
+obj-$(CONFIG_WIEGAND)		+= wiegand/
 obj-$(CONFIG_GREYBUS)		+= greybus/
 obj-$(CONFIG_COMEDI)		+= comedi/
 obj-$(CONFIG_STAGING)		+= staging/
diff --git a/drivers/wiegand/Kconfig b/drivers/wiegand/Kconfig
new file mode 100644
index 000000000000..09ac10217ede
--- /dev/null
+++ b/drivers/wiegand/Kconfig
@@ -0,0 +1,25 @@
+config WIEGAND
+	tristate "Wiegand Bus driver"
+	help
+	  The "Wiegand Interface" stands as an asynchronous low-lever protocol
+	  or a wiring standard. Its primary application is within point-to-point
+	  communication scenarios, especially between a credential and a reader
+	  as well as between card reader and a door controller. Wiegand's data
+	  throughput tends to be modest relatively to other protocols and it
+	  depends upon the selected pulse duration and intervals between pulses.
+
+	  Despite its relatively advanced age, Wiegand remains extensively
+	  employed in access control systems, particularly for linking card
+	  swipe mechanisms. These mechanisms harness the Wiegand effect as a
+	  means of transferring data from a card to a reader.
+
+	  Wiegand uses two wires to transmit the data - DATA0 and DATA1. Both
+	  lines are initially pulled up. When a bit of value 0 is being
+	  transmitted, the DATA0 line is pulled down. Similarly, when a bit of
+	  value 1 is being transmitted, the DATA1 line is pulled down.
+
+	  If you want Wiegand support, you should say Y here and also to the
+	  specific controller driver(s) below.
+
+	  This Wiegand support can also be built as a module. If so, the module
+	  will be called wiegand.
diff --git a/drivers/wiegand/Makefile b/drivers/wiegand/Makefile
new file mode 100644
index 000000000000..d17ecb722c6e
--- /dev/null
+++ b/drivers/wiegand/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_WIEGAND)			+= wiegand.o
diff --git a/drivers/wiegand/wiegand.c b/drivers/wiegand/wiegand.c
new file mode 100644
index 000000000000..7cdffdd47ea3
--- /dev/null
+++ b/drivers/wiegand/wiegand.c
@@ -0,0 +1,590 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/bitops.h>
+#include <linux/cdev.h>
+#include <linux/container_of.h>
+#include <linux/device.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/wiegand.h>
+
+#define WIEGAND_DEFAULT_PULSE_LEN 50
+#define WIEGAND_DEFAULT_INTERVAL_LEN 2000
+#define WIEGAND_DEFAULT_FRAME_GAP 2000
+
+struct bus_type wiegand_bus_type;
+
+/**
+ * struct wiegand_device - Wiegand listener device
+ *
+ * @dev: drivers structure of the device
+ * @id: unique device id
+ * @controller: Wiegand controller associated with the device
+ * @modalias: Name of the driver to use with this device, or its alias.
+ */
+struct wiegand_device {
+	struct device dev;
+	u8 id;
+	struct wiegand_controller *controller;
+	char modalias[WIEGAND_NAME_SIZE];
+};
+
+static DEFINE_IDA(wiegand_controller_ida);
+
+static ssize_t wiegand_get_user_data(struct wiegand_controller *ctlr, char __user const *buf,
+					  size_t len)
+{
+	int ret;
+
+	ret = bitmap_parse_user(buf, len, ctlr->data_bitmap, WIEGAND_MAX_PAYLEN_BYTES * 8);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static int wiegand_frelease(struct inode *ino, struct file *filp)
+{
+	struct wiegand_controller *ctlr = filp->private_data;
+
+	mutex_destroy(&ctlr->file_lock);
+	return 0;
+}
+
+static ssize_t wiegand_fwrite(struct file *filp, char __user const *buf, size_t len,
+				loff_t *offset)
+{
+	int ret;
+	struct wiegand_controller *ctlr = filp->private_data;
+
+	mutex_lock(&ctlr->file_lock);
+
+	if (!buf || len == 0)
+		return -EINVAL;
+
+	ret = wiegand_get_user_data(ctlr, buf, len);
+	if (ret < 0)
+		return ret;
+
+	ctlr->transfer_message(ctlr);
+
+	mutex_unlock(&ctlr->file_lock);
+	return len;
+}
+
+static int wiegand_fopen(struct inode *ino, struct file *filp)
+{
+	int ret;
+	struct wiegand_controller *ctlr = container_of(filp->f_op, struct wiegand_controller, fops);
+
+	filp->private_data = ctlr;
+
+	mutex_lock(&ctlr->file_lock);
+
+	if ((filp->f_flags & O_ACCMODE) == O_RDONLY || (filp->f_flags & O_ACCMODE) == O_RDWR) {
+		dev_err(ctlr->miscdev.this_device, "device is write only\n");
+		ret = -EIO;
+		goto out_unlock;
+	}
+
+	mutex_unlock(&ctlr->file_lock);
+	return 0;
+
+out_unlock:
+	mutex_unlock(&ctlr->file_lock);
+	return ret;
+}
+
+static inline struct wiegand_device *to_wiegand_device(struct device *dev)
+{
+	return dev ? container_of(dev, struct wiegand_device, dev) : NULL;
+}
+
+static void wiegand_cleanup(struct wiegand_device *wiegand)
+{
+	if (wiegand->controller->cleanup)
+		wiegand->controller->cleanup(wiegand);
+}
+
+/**
+ * wiegand_controller_release - called after the final refererence decrement
+ *
+ * @dev: the controller device
+ */
+static void wiegand_controller_release(struct device *dev)
+{
+	struct wiegand_controller *ctlr;
+
+	ctlr = container_of(dev, struct wiegand_controller, dev);
+	kfree(ctlr);
+}
+
+/**
+ * wiegand_alloc_controller - allocate a new Wiegand controller
+ *
+ * @dev: the controller device
+ * @size: size of the private data to be allocated for the caller
+ * @secondary: true if the controller is a secondary controller(reads data)
+ *
+ * This function is only by Wiegand controller drivers to allocate a new Wiegand controller
+ * structure before registering it using wiegand_register_controller().
+ */
+struct wiegand_controller *wiegand_alloc_controller(struct device *dev, size_t size,
+							bool secondary)
+{
+	struct wiegand_controller *ctlr;
+	size_t ctlr_size = ALIGN(sizeof(*ctlr), dma_get_cache_alignment());
+	size_t total_size;
+
+	if (check_add_overflow(size, ctlr_size, &total_size))
+		return NULL;
+
+	ctlr = kzalloc(total_size, GFP_KERNEL);
+	if (!ctlr)
+		return NULL;
+
+	device_initialize(&ctlr->dev);
+
+	ctlr->bus_num = -1;
+	ctlr->secondary = secondary;
+	ctlr->dev.parent = dev;
+	ctlr->dev.release = wiegand_controller_release;
+
+	wiegand_controller_set_devdata(ctlr, (void *)ctlr + ctlr_size);
+
+	return ctlr;
+}
+EXPORT_SYMBOL_NS_GPL(wiegand_alloc_controller, WIEGAND);
+
+/**
+ * devm_wiegand_alloc_controller - device managed allocation of a new Wiegand controller
+ * @dev: physical device of Wiegand controller
+ * @size: size of the private data to be allocated for the caller
+ * @secondary: true if the controller is a secondary controller(reads data)
+ *
+ * Device managed version of wiegand_alloc_controller(). The Wiegand controller is automatically
+ * freed on driver detach.
+ */
+struct wiegand_controller *devm_wiegand_alloc_controller(struct device *dev, unsigned int size,
+							bool secondary)
+{
+	struct wiegand_controller *ctlr;
+
+	ctlr = wiegand_alloc_controller(dev, size, secondary);
+	if (ctlr)
+		ctlr->devm_allocated = true;
+	else
+		return NULL;
+
+	if (devm_add_action_or_reset(dev, wiegand_controller_put, ctlr))
+		return NULL;
+
+	return ctlr;
+}
+EXPORT_SYMBOL_NS_GPL(devm_wiegand_alloc_controller, WIEGAND);
+
+static int wiegand_controller_check_ops(struct wiegand_controller *ctlr)
+{
+	if (!ctlr->transfer_message)
+		return -EINVAL;
+	return 0;
+}
+
+/**
+ * register_wiegand_device - allocates and registers a new Wiegand device
+ *
+ * @ctlr: controller structure to attach device to
+ * @node: firmware node for the device
+ */
+static struct wiegand_device *register_wiegand_device(struct wiegand_controller *ctlr,
+							struct fwnode_handle *node)
+{
+	struct wiegand_device *wiegand;
+	int ret;
+
+	wiegand = wiegand_alloc_device(ctlr);
+	if (!wiegand)
+		return NULL;
+
+	fwnode_handle_get(node);
+	device_set_node(&wiegand->dev, node);
+
+	ret = wiegand_add_device(wiegand);
+	if (ret) {
+		dev_err(&ctlr->dev, "wiegand_device register error %pfwf\n", node);
+		goto out_node_put;
+	}
+
+	/* check if more devices are connected to the bus */
+	if (ctlr->device_count > 1)
+		dev_warn(&ctlr->dev, "Wiegand is a point-to-point bus, it is advised to only connect one device per Wiegand bus. The devices may not communicate using the same pulse length, format or else.\n");
+
+	return wiegand;
+
+out_node_put:
+	fwnode_handle_put(node);
+	put_device(&wiegand->dev);
+	wiegand_cleanup(wiegand);
+	return ERR_PTR(ret);
+}
+
+static void register_wiegand_devices(struct wiegand_controller *ctlr)
+{
+	struct wiegand_device *wiegand;
+	struct fwnode_handle *node;
+
+	fwnode_for_each_available_child_node(ctlr->dev.fwnode, node) {
+		wiegand = register_wiegand_device(ctlr, node);
+		if (IS_ERR(wiegand))
+			dev_warn(&ctlr->dev, "failed to create wiegand device for %pfwf\n", node);
+	}
+}
+
+/**
+ * wiegand_controller_get_timings - gets timings using a firmware node
+ *
+ * @ctlr: pointer to a wiegand_controller structure
+ *
+ * Tries to get timing data from a firmware node, if they are not defined, then default values are
+ * set.
+ */
+static void wiegand_controller_get_timings(struct wiegand_controller *ctlr)
+{
+	int ret;
+	u32 pulse_len, interval_len, frame_gap;
+	struct wiegand_timing *timing = &ctlr->timing;
+	struct device *ctlr_dev = &ctlr->dev;
+
+	ret = device_property_read_u32(ctlr_dev, "pulse-len-us", &pulse_len);
+	if (!ret && pulse_len > 0)
+		timing->pulse_len = pulse_len;
+	else
+		timing->pulse_len = WIEGAND_DEFAULT_PULSE_LEN;
+
+	ret = device_property_read_u32(ctlr_dev, "interval-len-us", &interval_len);
+	if (!ret && interval_len > 0)
+		timing->interval_len = interval_len;
+	else
+		timing->interval_len = WIEGAND_DEFAULT_INTERVAL_LEN;
+
+	ret = device_property_read_u32(ctlr_dev, "frame-gap-us", &frame_gap);
+	if (!ret && frame_gap > 0)
+		timing->frame_gap = frame_gap;
+	else
+		timing->frame_gap = WIEGAND_DEFAULT_FRAME_GAP;
+}
+
+static int __unregister(struct device *dev, void *null)
+{
+	wiegand_unregister_device(to_wiegand_device(dev));
+	return 0;
+}
+
+/**
+ * wiegand_unregister_controller - unregisters controller structure within Wiegand bus
+ *
+ * @ptr: pointer to a wiegand_controller structure
+ *
+ * Frees all resources allocated by the wiegand_register_controller() function.
+ * If the controller was registered using devm_wiegand_alloc_controller() then
+ * this function is called automatically on driver detach.Otherwise the function needs
+ * to be called manually. If controller is not devm managed, then the reference to the
+ * controller structure is put.
+ */
+void wiegand_unregister_controller(void *ptr)
+{
+	struct wiegand_controller *ctlr = ptr;
+	unsigned int id = ctlr->bus_num;
+
+	device_for_each_child(&ctlr->dev, NULL, __unregister);
+	ida_free(&wiegand_controller_ida, id);
+	device_del(&ctlr->dev);
+
+	kfree(ctlr->miscdev.name);
+	misc_deregister(&ctlr->miscdev);
+
+	if (!ctlr->devm_allocated)
+		put_device(&ctlr->dev);
+}
+EXPORT_SYMBOL_NS_GPL(wiegand_unregister_controller, WIEGAND);
+
+/**
+ * wiegand_register_controller - registers controller structure within bus
+ * @ctlr: controller structure to register
+ *
+ * Function checks that the message transfer functions is defined for passed controller structure,
+ * gets its attributes and finally adds the controller device and registers the controller on the
+ * bus.
+ */
+int wiegand_register_controller(struct wiegand_controller *ctlr)
+{
+	int status, id;
+
+	status = wiegand_controller_check_ops(ctlr);
+	if (status)
+		return status;
+
+	id = ida_alloc(&wiegand_controller_ida, GFP_KERNEL);
+	if (id < 0)
+		return id;
+	ctlr->bus_num = id;
+
+	wiegand_controller_get_timings(ctlr);
+
+	status = dev_set_name(&ctlr->dev, "wiegand%u", ctlr->bus_num);
+	if (status < 0)
+		goto out_free_ida;
+
+	ctlr->miscdev.name = kasprintf(GFP_KERNEL, "wiegand1");
+	if (!ctlr->miscdev.name) {
+		status = -ENOMEM;
+		goto out_free_ida;
+	}
+
+	ctlr->fops.owner = THIS_MODULE;
+	ctlr->fops.open = wiegand_fopen;
+	ctlr->fops.release = wiegand_frelease;
+	ctlr->fops.write = wiegand_fwrite;
+	ctlr->miscdev.fops = &ctlr->fops;
+	ctlr->miscdev.minor = MISC_DYNAMIC_MINOR;
+
+	status = misc_register(&ctlr->miscdev);
+	if (status) {
+		dev_err(&ctlr->dev, "couldn't register misc device\n");
+		goto out_free_ida_name;
+	}
+
+	mutex_init(&ctlr->file_lock);
+
+	status = device_add(&ctlr->dev);
+	if (status < 0)
+		goto out_free_ida_name_misc;
+
+	ctlr->device_count = 0;
+	ctlr->miscdev.parent = &ctlr->dev;
+	register_wiegand_devices(ctlr);
+
+	return status;
+
+out_free_ida_name_misc:
+	misc_deregister(&ctlr->miscdev);
+
+out_free_ida_name:
+	kfree(ctlr->miscdev.name);
+
+out_free_ida:
+	ida_free(&wiegand_controller_ida, ctlr->bus_num);
+	return status;
+}
+EXPORT_SYMBOL_NS_GPL(wiegand_register_controller, WIEGAND);
+
+int devm_wiegand_register_controller(struct device *dev, struct wiegand_controller *ctlr)
+{
+	int ret;
+
+	ret = wiegand_register_controller(ctlr);
+	if (ret < 0)
+		return ret;
+
+	return devm_add_action_or_reset(dev, wiegand_unregister_controller, ctlr);
+}
+EXPORT_SYMBOL_NS_GPL(devm_wiegand_register_controller, WIEGAND);
+
+struct wiegand_controller *wiegand_device_get_controller(struct wiegand_device *dev)
+{
+	return dev->controller;
+}
+EXPORT_SYMBOL_NS_GPL(wiegand_device_get_controller, WIEGAND);
+
+/**
+ * wiegand_dev_release - called after the final reference count decrement
+ * @dev: device to release
+ */
+static void wiegand_dev_release(struct device *dev)
+{
+	struct wiegand_device *wiegand = to_wiegand_device(dev);
+
+	wiegand_controller_put(wiegand->controller);
+	kfree(wiegand);
+}
+
+struct wiegand_device *wiegand_alloc_device(struct wiegand_controller *ctlr)
+{
+	struct wiegand_device *wiegand;
+
+	wiegand = kzalloc(sizeof(*wiegand), GFP_KERNEL);
+	if (!wiegand) {
+		wiegand_controller_put(ctlr);
+		return NULL;
+	}
+
+	wiegand->controller = ctlr;
+	wiegand->dev.parent = &ctlr->dev;
+	wiegand->dev.bus = &wiegand_bus_type;
+	wiegand->dev.release = wiegand_dev_release;
+
+	device_initialize(&wiegand->dev);
+	return wiegand;
+}
+EXPORT_SYMBOL_NS_GPL(wiegand_alloc_device, WIEGAND);
+
+static int wiegand_dev_set_name(struct wiegand_device *wiegand, u8 id)
+{
+	return dev_set_name(&wiegand->dev, "%s.%u", dev_name(&wiegand->controller->dev), id);
+}
+
+static int __wiegand_add_device(struct wiegand_device *wiegand)
+{
+	struct wiegand_controller *ctlr = wiegand->controller;
+	struct device *dev = ctlr->dev.parent;
+	int status;
+
+	status = wiegand_setup(wiegand);
+	if (status < 0) {
+		dev_err(dev, "can't setup %s, status %d\n",
+			dev_name(&wiegand->dev), status);
+		return status;
+	}
+
+	status = device_add(&wiegand->dev);
+	if (status < 0) {
+		dev_err(dev, "can't add %s, status %d\n", dev_name(&wiegand->dev), status);
+		wiegand_cleanup(wiegand);
+	} else {
+		dev_dbg(dev, "registered child %s\n", dev_name(&wiegand->dev));
+	}
+
+	return status;
+}
+
+int wiegand_add_device(struct wiegand_device *wiegand)
+{
+	struct wiegand_controller *ctlr = wiegand->controller;
+	int status;
+
+	status = wiegand_dev_set_name(wiegand, ctlr->device_count);
+	if (status)
+		return status;
+
+	status = __wiegand_add_device(wiegand);
+	if (!status) {
+		ctlr->device_count++;
+		wiegand->id = wiegand->controller->device_count;
+	}
+
+	return status;
+}
+
+int wiegand_setup(struct wiegand_device *wiegand)
+{
+	int status;
+	struct device *ctlr_dev = &wiegand->controller->dev;
+	struct wiegand_controller *ctlr = wiegand->controller;
+
+	if (ctlr->setup) {
+		status = ctlr->setup(wiegand);
+		if (status) {
+			dev_err(ctlr_dev, "failed to setup device: %d\n", status);
+			return status;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(wiegand_setup, WIEGAND);
+
+void wiegand_unregister_device(struct wiegand_device *wiegand)
+{
+	if (!wiegand)
+		return;
+
+	fwnode_handle_put(wiegand->dev.fwnode);
+	device_del(&wiegand->dev);
+	wiegand_cleanup(wiegand);
+	put_device(&wiegand->dev);
+}
+EXPORT_SYMBOL_NS_GPL(wiegand_unregister_device, WIEGAND);
+
+int wiegand_send_message(struct wiegand_device *wiegand, unsigned long *msg_bmp, u8 bitlen)
+{
+	struct wiegand_primary *primary = wiegand->controller;
+
+	if (msg_bmp == NULL)
+		return -EINVAL;
+
+	if (primary->transfer_message)
+		primary->transfer_message(primary);
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(wiegand_send_message, WIEGAND);
+
+static int wiegand_match_device(struct device *dev, struct device_driver *drv)
+{
+	struct wiegand_device *wiegand_dev = to_wiegand_device(dev);
+
+	if (of_driver_match_device(dev, drv))
+		return 1;
+
+	return strcmp(wiegand_dev->modalias, drv->name) == 0;
+}
+
+static int wiegand_probe(struct device *dev)
+{
+	struct wiegand_device *wiegand = to_wiegand_device(dev);
+	const struct wiegand_driver *wdrv = to_wiegand_driver(dev->driver);
+
+	return wdrv->probe(wiegand);
+}
+
+static void wiegand_remove(struct device *dev)
+{
+	const struct wiegand_driver *wdrv = to_wiegand_driver(dev->driver);
+
+	if (wdrv->remove)
+		wdrv->remove(to_wiegand_device(dev));
+}
+
+struct bus_type wiegand_bus_type = {
+	.name		= "wiegand",
+	.match		= wiegand_match_device,
+	.probe		= wiegand_probe,
+	.remove		= wiegand_remove,
+};
+EXPORT_SYMBOL_NS_GPL(wiegand_bus_type, WIEGAND);
+
+int __wiegand_register_driver(struct module *owner, struct wiegand_driver *wdrv)
+{
+	wdrv->driver.owner = owner;
+	wdrv->driver.bus = &wiegand_bus_type;
+
+	return driver_register(&wdrv->driver);
+}
+EXPORT_SYMBOL_NS_GPL(__wiegand_register_driver, WIEGAND);
+
+static int __init wiegand_init(void)
+{
+	int ret;
+
+	ret = bus_register(&wiegand_bus_type);
+	if (ret < 0)
+		pr_err("Wiegand bus registration failed: %d\n", ret);
+
+	return ret;
+}
+postcore_initcall_sync(wiegand_init);
+
+static void __exit wiegand_exit(void)
+{
+	ida_destroy(&wiegand_controller_ida);
+	bus_unregister(&wiegand_bus_type);
+}
+module_exit(wiegand_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Wiegand bus driver");
+MODULE_AUTHOR("Martin Zaťovič <m.zatovic1@gmail.com>");
diff --git a/include/linux/wiegand.h b/include/linux/wiegand.h
new file mode 100644
index 000000000000..52d49f4efdd4
--- /dev/null
+++ b/include/linux/wiegand.h
@@ -0,0 +1,148 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef H_LINUX_INCLUDE_LINUX_WIEGAND_H
+#define H_LINUX_INCLUDE_LINUX_WIEGAND_H
+
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/mutex.h>
+#include <linux/types.h>
+
+#define WIEGAND_NAME_SIZE 32
+#define WIEGAND_MAX_PAYLEN_BYTES 256
+
+extern struct bus_type wiegand_type;
+
+struct wiegand_device;
+/**
+ * struct wiegand_timing - Wiegand timings in microseconds
+ * @pulse_len: length of the low pulse
+ * @interval_len: length of a whole bit (both the pulse and the high phase)
+ * @frame_gap: length of the last bit of a frame (both the pulse and the high phase)
+ */
+struct wiegand_timing {
+	u32 pulse_len;
+	u32 interval_len;
+	u32 frame_gap;
+};
+
+/**
+ * struct wiegand_controller - Wiegand primary or secondary interface
+ * @dev: Device interface of the controller
+ * @miscdev: Misc device associated with controller
+ * @fops: File operations
+ * @file_lock: File mutex
+ * @bus_num: Board-specific identifier for Wiegand controller
+ * @secondary: Whether the controller is a secondary(receives data)
+ * @devm_allocated: Whether the allocation of this struct is devres-managed
+ * @timing: Stores timing parameters for Wiegand communication
+ * @payload_len: Number of bits in a Wiegand message
+ * @device_count: Counter of devices connected to the same Wiegand bus(controller)
+ * @data_bitmap: Data buffer
+ * @transfer_message: Send a message on the bus
+ * @setup: Setup a device
+ * @cleanup: Cleanup after a device
+ */
+struct wiegand_controller {
+	struct device dev;
+	struct miscdevice miscdev;
+
+	struct file_operations fops;
+	struct mutex file_lock;
+
+	unsigned int bus_num;
+
+	bool secondary;
+	bool devm_allocated;
+
+	struct wiegand_timing timing;
+	u32 payload_len;
+
+	u8 device_count;
+
+	DECLARE_BITMAP(data_bitmap, WIEGAND_MAX_PAYLEN_BYTES * 8);
+
+	int (*transfer_message)(struct wiegand_controller *ctlr);
+	int (*setup)(struct wiegand_device *wiegand);
+	void (*cleanup)(struct wiegand_device *wiegand);
+};
+
+struct wiegand_driver {
+	struct device_driver driver;
+	const struct wiegand_device_id *id_table;
+	int (*probe)(struct wiegand_device *wiegand);
+	void (*remove)(struct wiegand_device *wiegand);
+};
+
+#define wiegand_primary wiegand_controller
+struct wiegand_controller *wiegand_alloc_controller(struct device *host, size_t size,
+							bool secondary);
+
+struct wiegand_controller *devm_wiegand_alloc_controller(struct device *dev,
+								unsigned int size, bool secondary);
+static inline struct wiegand_controller *devm_wiegand_alloc_primary(struct device *dev,
+								unsigned int size)
+{
+	return devm_wiegand_alloc_controller(dev, size, false);
+}
+
+int wiegand_register_controller(struct wiegand_controller *ctlr);
+int devm_wiegand_register_controller(struct device *dev, struct wiegand_controller *ctlr);
+#define wiegand_register_primary(_ctlr) wiegand_register_controller(_ctlr)
+#define devm_wiegand_register_primary(_dev, _ctlr)devm_wiegand_register_controller(_dev, _ctlr)
+void wiegand_unregister_controller(void *ctlr);
+#define wiegand_unregister_primary(_ctlr) wiegand_unregister_controller(_ctlr)
+struct wiegand_primary *wiegand_busnum_to_primary(u16 bus_num);
+
+static inline void *wiegand_controller_get_devdata(struct wiegand_controller *ctlr)
+{
+	return dev_get_drvdata(&ctlr->dev);
+}
+
+static inline void wiegand_controller_set_devdata(struct wiegand_controller *ctlr, void *data)
+{
+	dev_set_drvdata(&ctlr->dev, data);
+}
+
+#define wiegand_primary_get_devdata(_ctlr) wiegand_controller_get_devdata(_ctlr)
+#define wiegand_primary_set_devdata(_ctlr, data) wiegand_controller_set_devdata(_ctlr, data)
+
+static inline struct wiegand_controller *wiegand_controller_get(struct wiegand_controller *ctlr)
+{
+	if (!ctlr || !get_device(&ctlr->dev))
+		return NULL;
+	return ctlr;
+}
+
+static inline void wiegand_controller_put(void *ptr)
+{
+	struct wiegand_controller *ctlr = ptr;
+
+	if (ctlr)
+		put_device(&ctlr->dev);
+}
+
+struct wiegand_device *wiegand_alloc_device(struct wiegand_controller *ctlr);
+int wiegand_add_device(struct wiegand_device *wiegand);
+int wiegand_setup(struct wiegand_device *wiegand);
+void wiegand_unregister_device(struct wiegand_device *wiegand);
+struct wiegand_controller *wiegand_device_get_controller(struct wiegand_device *dev);
+
+int wiegand_send_message(struct wiegand_device *wiegand, unsigned long *msg_bmp, u8 bitlen);
+
+int __wiegand_register_driver(struct module *owner, struct wiegand_driver *wdrv);
+#define wiegand_register_driver(driver) __wiegand_register_driver(THIS_MODULE, driver)
+
+static inline void wiegand_unregister_driver(struct wiegand_driver *wdrv)
+{
+	if (wdrv)
+		driver_unregister(&wdrv->driver);
+}
+
+static inline struct wiegand_driver *to_wiegand_driver(struct device_driver *drv)
+{
+	return drv ? container_of(drv, struct wiegand_driver, driver) : NULL;
+}
+
+#endif	/* H_LINUX_INCLUDE_LINUX_WIEGAND_H */
-- 
2.40.1

Re: [PATCHv5 2/4] wiegand: add Wiegand bus driver

[Greg KH]

On Thu, Aug 24, 2023 at 01:10:13PM +0200, Martin Zaťovič wrote:
> +static inline void wiegand_controller_put(void *ptr)

Why is this a void *?  It should be "struct wiegand_controller *"

Please never use void * if at all possible.

> +static inline void wiegand_unregister_driver(struct wiegand_driver *wdrv)
> +{
> +	if (wdrv)

How could this ever be true?

> +		driver_unregister(&wdrv->driver);
> +}
> +
> +static inline struct wiegand_driver *to_wiegand_driver(struct device_driver *drv)
> +{
> +	return drv ? container_of(drv, struct wiegand_driver, driver) : NULL;

How could drv be NULL?

And you should make this a const *, right?

thanks,

greg k-h

Re: [PATCHv5 2/4] wiegand: add Wiegand bus driver

[Martin Zaťovič]

On Thu, Aug 24, 2023 at 01:40:35PM +0200, Greg KH wrote:
> On Thu, Aug 24, 2023 at 01:10:13PM +0200, Martin Zaťovič wrote:
> > +static inline void wiegand_controller_put(void *ptr)
> 
> Why is this a void *?  It should be "struct wiegand_controller *"

It is a void* because it is being passed to devm_add_action_or_reset()
and this function only accepts a pointer to a function that accepts a void*
parameter. I am not sure if there is a way around it.

> 
> Please never use void * if at all possible.
> 
> > +static inline void wiegand_unregister_driver(struct wiegand_driver *wdrv)
> > +{
> > +	if (wdrv)
> 
> How could this ever be true?
> 
> > +		driver_unregister(&wdrv->driver);
> > +}
> > +
> > +static inline struct wiegand_driver *to_wiegand_driver(struct device_driver *drv)
> > +{
> > +	return drv ? container_of(drv, struct wiegand_driver, driver) : NULL;
> 
> How could drv be NULL?
> 
> And you should make this a const *, right?

Will fix the other issues. Thank you!

> 
> thanks,
> 
> greg k-h

Re: [PATCHv5 2/4] wiegand: add Wiegand bus driver

[Greg KH]

On Thu, Aug 24, 2023 at 02:53:13PM +0200, Martin Zaťovič wrote:
> On Thu, Aug 24, 2023 at 01:40:35PM +0200, Greg KH wrote:
> > On Thu, Aug 24, 2023 at 01:10:13PM +0200, Martin Zaťovič wrote:
> > > +static inline void wiegand_controller_put(void *ptr)
> > 
> > Why is this a void *?  It should be "struct wiegand_controller *"
> 
> It is a void* because it is being passed to devm_add_action_or_reset()
> and this function only accepts a pointer to a function that accepts a void*
> parameter. I am not sure if there is a way around it.

Why is this passed to that function?  This feels odd, I'll look at it
again when you send your next version, but this is not a normal "put"
type of call at all to have.

And inline functions can't be passed as parameters (well they can, but
then they are no longer an inline function...)

thanks,

greg k-h

Re: [PATCHv5 2/4] wiegand: add Wiegand bus driver

[Andy Shevchenko]

On Thu, Aug 24, 2023 at 02:53:13PM +0200, Martin Zaťovič wrote:
> On Thu, Aug 24, 2023 at 01:40:35PM +0200, Greg KH wrote:
> > On Thu, Aug 24, 2023 at 01:10:13PM +0200, Martin Zaťovič wrote:

...

> > > +static inline void wiegand_controller_put(void *ptr)
> > 
> > Why is this a void *?  It should be "struct wiegand_controller *"
> 
> It is a void* because it is being passed to devm_add_action_or_reset()
> and this function only accepts a pointer to a function that accepts a void*
> parameter. I am not sure if there is a way around it.

I just replied with a deeper review with the answer to this.

> > Please never use void * if at all possible.

Correct. We better to have a type checking when possible. Esp. for the exported
functions.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCHv5 2/4] wiegand: add Wiegand bus driver

[Andy Shevchenko]

On Thu, Aug 24, 2023 at 01:10:13PM +0200, Martin Zaťovič wrote:
> The Wiegand protocol serves as a standardized interface protocol, extensively
> employed within electronic access control systems, to facilitate data exchange
> between credentials, readers, and door controllers. Its inception can be
> attributed to the widespread adoption of Wiegand card readers, leveraging the
> Wiegand effect - a physical phenomenon wherein a Wiegand wire (or card)
> generates small yet distinct magnetic fields. A Wiegand reader detects the
> magnetic pulses emitted by the internal wires of the card.
> 
> Three wires are central to the Wiegand protocol: a common ground wire and two
> distinct data wires designated as DATA0 and DATA1. During periods of inactivity,
> both DATA0 and DATA1 lines remain pulled up. For transmitting a '0,' the DATA0
> line is pulled down while DATA1 remains pulled up; conversely, transmitting
> a '1' causes DATA1 to be pulled down while DATA0 remains pulled up. Notably,
> this protocol ensures that the two lines never simultaneously experience a low
> state.
> 
> Timing characteristics within the Wiegand protocol lack a uniform
> standardization, introducing variability between devices. Generally, pulse
> durations hover between 50 to 100 microseconds, while inter-pulse gaps span 20
> to 100 milliseconds. There is no stop bit or similar delimiter to signal the
> conclusion of a message. Instead, the receiver either counts the bits within the
> message or enforces a timeout, often set at around ten times the inter-pulse gap
> duration.
> 
> The 26-Bit Wiegand Interface Standard, or 26-Bit Wiegand Format outlines the
> message format most commonly used among Wiegand devices. This format allocates
> the initial and terminal bits for parity. The subsequent eight bits following
> the initial parity bit are reserved for the Facility Code designed for distinct
> location identification. The remaining bits house the unique ID number. As the
> technology evolved, new Wiegand formats emerged, including 36-bit and 37-bit
> formats. It was also common practice for manufacturers to engineer devices
> compatible with proprietary Wiegand formats tailored to their specifications.
> 
> The Wiegand bus driver handles Wiegand controller and Wiegand device
> managemement and driver matching. The bus driver defines the structures for
> Wiegand controllers and Wiegand devices. Wiegand controller structure contains
> all attributes that define the communication such as the payload_len for
> configuring the size of a single Wiegand message in bits, thus choosing a
> format. Each Wiegand controller should be associated with one Wiegand device,
> as Wiegand is typically a point-to-point bus.

...

> +#include <linux/bitops.h>

Should be bitmap.h

> +#include <linux/cdev.h>
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/module.h>

mutex.h ?

> +#include <linux/of_device.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/wiegand.h>

...

> +static ssize_t wiegand_fwrite(struct file *filp, char __user const *buf, size_t len,
> +				loff_t *offset)
> +{
> +	int ret;
> +	struct wiegand_controller *ctlr = filp->private_data;
> +
> +	mutex_lock(&ctlr->file_lock);
> +
> +	if (!buf || len == 0)

Ouch?!

To avoid this, you can use cleanup.h and guard your functions with a lock,
moreover in this case the check is not even needed to be performed under
the lock.

> +		return -EINVAL;
> +
> +	ret = wiegand_get_user_data(ctlr, buf, len);
> +	if (ret < 0)
> +		return ret;
> +
> +	ctlr->transfer_message(ctlr);
> +
> +	mutex_unlock(&ctlr->file_lock);
> +	return len;
> +}

...

> +static int wiegand_fopen(struct inode *ino, struct file *filp)
> +{
> +	int ret;
> +	struct wiegand_controller *ctlr = container_of(filp->f_op, struct wiegand_controller, fops);
> +
> +	filp->private_data = ctlr;
> +
> +	mutex_lock(&ctlr->file_lock);

Do you care about the call being interrupted by a signal?
Ditto for other system calls in the framework.

> +	if ((filp->f_flags & O_ACCMODE) == O_RDONLY || (filp->f_flags & O_ACCMODE) == O_RDWR) {
> +		dev_err(ctlr->miscdev.this_device, "device is write only\n");
> +		ret = -EIO;
> +		goto out_unlock;
> +	}
> +
> +	mutex_unlock(&ctlr->file_lock);
> +	return 0;
> +
> +out_unlock:
> +	mutex_unlock(&ctlr->file_lock);
> +	return ret;
> +}

...

> +	size_t ctlr_size = ALIGN(sizeof(*ctlr), dma_get_cache_alignment());

Yeah, yet another user for a new macro in overflow.h (not yet there, though).

> +	size_t total_size;
> +
> +	if (check_add_overflow(size, ctlr_size, &total_size))
> +		return NULL;
> +
> +	ctlr = kzalloc(total_size, GFP_KERNEL);
> +	if (!ctlr)
> +		return NULL;

...

> +struct wiegand_controller *devm_wiegand_alloc_controller(struct device *dev, unsigned int size,
> +							bool secondary)
> +{
> +	struct wiegand_controller *ctlr;
> +
> +	ctlr = wiegand_alloc_controller(dev, size, secondary);
> +	if (ctlr)
> +		ctlr->devm_allocated = true;
> +	else
> +		return NULL;

	if (!ctrl)
		return NULL;

or maybe
		return ERR_PTR(-ENOMEM);


> +	if (devm_add_action_or_reset(dev, wiegand_controller_put, ctlr))
> +		return NULL;

	ret = ...
	if (ret)
		return ERR_PTR(ret);

?

> +
> +	return ctlr;

...

> +/**
> + * register_wiegand_device - allocates and registers a new Wiegand device
> + *
> + * @ctlr: controller structure to attach device to
> + * @node: firmware node for the device

Run

	scripts/kernel-doc -v -none -Wall ...

against this file and fix all warnings.

> + */

> +							struct fwnode_handle *node)

Usually we call it fwnode.

...

> +	device_set_node(&wiegand->dev, node);

	device_set_node(&wiegand->dev, fwnode_handle_get(fwnode));

> +out_node_put:

> +	fwnode_handle_put(node);

Hmm... Shouldn't we do this at the ->release()?

> +	put_device(&wiegand->dev);
> +	wiegand_cleanup(wiegand);
> +	return ERR_PTR(ret);
> +}

...

> +	struct fwnode_handle *node;

fwnode

here and everywhere else.

...

> +	fwnode_for_each_available_child_node(ctlr->dev.fwnode, node) {

	device_for_each_child_node()

> +		wiegand = register_wiegand_device(ctlr, node);
> +		if (IS_ERR(wiegand))
> +			dev_warn(&ctlr->dev, "failed to create wiegand device for %pfwf\n", node);
> +	}

...

> +	struct device *ctlr_dev = &ctlr->dev;

Just name it "dev". You can use similar approach in another functions,
like the above, to make them look nicer.

...

> +	ret = device_property_read_u32(ctlr_dev, "pulse-len-us", &pulse_len);
> +	if (!ret && pulse_len > 0)
> +		timing->pulse_len = pulse_len;
> +	else
> +		timing->pulse_len = WIEGAND_DEFAULT_PULSE_LEN;

What about

	/* Use default if property is absent or can't be read */
	pulse_len = WIEGAND_DEFAULT_PULSE_LEN;
	device_property_read_u32(dev, "pulse-len-us", &pulse_len);
	timing->pulse_len = pulse_len;

Wrong values should be caught up by DT schema linter, no? If the parameters are
0 it's not your problem, although you can warn.

I'm not sure about this, so your current variant is okay.

Ditto for the rest.

...

> +static int __unregister(struct device *dev, void *null)

It is recommended to use namespace even if it's static function.

...

> +void wiegand_unregister_controller(void *ptr)
> +{

Why the parameter is not properly typed?
Yes for devm you probably need a wrapper

static devm_..._unregister_...(void *ctrl)
{
	wiegand_unregister_controller(ctrl)
}


For the exported _unregister, taking into account the possible optional support
for this, you may need to check the parameter to be valid.

	if (IS_ERR_OR_NULL(...)) // _OR_NULL probably is not needed as per above
		return;

> +}

> +	status = misc_register(&ctlr->miscdev);
> +	if (status) {
> +		dev_err(&ctlr->dev, "couldn't register misc device\n");
> +		goto out_free_ida_name;
> +	}

> +	mutex_init(&ctlr->file_lock);

Shouldn't it be needed to be initialized before device itself?

...

> +	status = device_add(&ctlr->dev);
> +	if (status < 0)

Don't we need to call something like device_del() or put_device() at this point?
OK, read doc for device_add() it clarifies this.

> +		goto out_free_ida_name_misc;

...

> +	status = __wiegand_add_device(wiegand);
> +	if (!status) {
> +		ctlr->device_count++;
> +		wiegand->id = wiegand->controller->device_count;
> +	}
> +
> +	return status;

	if (status)
		return status;
	...
	return 0;

...

> +void wiegand_unregister_device(struct wiegand_device *wiegand)
> +{
> +	if (!wiegand)

	IS_ERR_OR_NULL() or alike (see above)

> +		return;
> +
> +	fwnode_handle_put(wiegand->dev.fwnode);

No, do not dereference fwnode in struct device, always use proper API
and/or dev_fwnode() accessor.

> +	device_del(&wiegand->dev);
> +	wiegand_cleanup(wiegand);
> +	put_device(&wiegand->dev);
> +}

...

> +static int wiegand_match_device(struct device *dev, struct device_driver *drv)
> +{
> +	struct wiegand_device *wiegand_dev = to_wiegand_device(dev);
> +
> +	if (of_driver_match_device(dev, drv))
> +		return 1;

I would add ACPI support as well as

	if (acpi_driver_match_device(dev, drv))
		return 1;

> +	return strcmp(wiegand_dev->modalias, drv->name) == 0;
> +}

...

> +static int __init wiegand_init(void)
> +{
> +	int ret;
> +
> +	ret = bus_register(&wiegand_bus_type);
> +	if (ret < 0)
> +		pr_err("Wiegand bus registration failed: %d\n", ret);

pr_fmt() may help to have unified prefix for all messages printed with help of
pr_*() macros.

> +	return ret;
> +}

...

> +#define WIEGAND_MAX_PAYLEN_BYTES 256

I don't see you use BYTES, but rather BITS. Can you define _BITS instead?

...

> +struct wiegand_device;

+ Blank line.

> +/**
> + * struct wiegand_timing - Wiegand timings in microseconds

Perhaps timings ?

> + * @pulse_len: length of the low pulse
> + * @interval_len: length of a whole bit (both the pulse and the high phase)
> + * @frame_gap: length of the last bit of a frame (both the pulse and the high phase)
> + */

...

> +	struct wiegand_timing timing;

Perhaps timings ?

...

> +#endif	/* H_LINUX_INCLUDE_LINUX_WIEGAND_H */

Leading H is something unusual?

-- 
With Best Regards,
Andy Shevchenko

[PATCHv5 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand controller

[Martin Zaťovič]

GPIO bitbanged Wiegand controller requires definitions of GPIO lines to be
used on top of the common Wiegand properties. Wiegand utilizes two such
lines - DATA0(low data line) and DATA1(high data line).

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Martin Zaťovič <m.zatovic1@gmail.com>
---
 .../bindings/wiegand/wiegand-gpio.yaml        | 46 +++++++++++++++++++
 MAINTAINERS                                   |  5 ++
 2 files changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml

diff --git a/Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml b/Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml
new file mode 100644
index 000000000000..cf2cb938de02
--- /dev/null
+++ b/Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/wiegand/wiegand-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GPIO bitbanged Wiegand interface properties
+
+maintainers:
+  - Martin Zaťovič <m.zatovic1@gmail.com>
+
+description:
+  This represents the GPIO lines used for bit-banged Wiegand on dedicated GPIO
+  lines.
+
+allOf:
+  - $ref: /schemas/wiegand/wiegand-controller.yaml#
+
+properties:
+  compatible:
+    const: wiegand-gpio
+
+  data-gpios:
+    description: GPIOs used as Wiegand data lines, DATA0 and DATA1 respectivelly.
+    maxItems: 2
+
+required:
+  - compatible
+  - data-gpios
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    wiegand@f00 {
+        compatible = "wiegand-gpio";
+        pulse-len-us = <50>;
+        interval-len-us = <2000>;
+        frame-gap-us = <2000>;
+        data-gpios = <&gpio2 6 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>,
+                     <&gpio2 7 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
+
+        /* devices */
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 0afcc88a38d2..fb158e74ca4b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22929,6 +22929,11 @@ F:	Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
 F:	drivers/wiegand/wiegand.c
 F:	include/linux/wiegand.h
 
+WIEGAND GPIO BITBANG DRIVER
+M:	Martin Zaťovič <m.zatovic1@gmail.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml
+
 WILOCITY WIL6210 WIRELESS DRIVER
 L:	linux-wireless@vger.kernel.org
 S:	Orphan
-- 
2.40.1

Re: [PATCHv5 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand controller

[Rob Herring]

On Thu, 24 Aug 2023 13:10:14 +0200, Martin Zaťovič wrote:
> GPIO bitbanged Wiegand controller requires definitions of GPIO lines to be
> used on top of the common Wiegand properties. Wiegand utilizes two such
> lines - DATA0(low data line) and DATA1(high data line).
> 
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Martin Zaťovič <m.zatovic1@gmail.com>
> ---
>  .../bindings/wiegand/wiegand-gpio.yaml        | 46 +++++++++++++++++++
>  MAINTAINERS                                   |  5 ++
>  2 files changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/wiegand/wiegand-gpio.example.dts:20.21-29.11: Warning (unit_address_vs_reg): /example-0/wiegand@f00: node has a unit name, but no reg or ranges property

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230824111015.57765-4-m.zatovic1@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCHv5 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand controller

[Rob Herring]

On Thu, Aug 24, 2023 at 01:10:14PM +0200, Martin Zaťovič wrote:
> GPIO bitbanged Wiegand controller requires definitions of GPIO lines to be
> used on top of the common Wiegand properties. Wiegand utilizes two such
> lines - DATA0(low data line) and DATA1(high data line).
> 
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Martin Zaťovič <m.zatovic1@gmail.com>
> ---
>  .../bindings/wiegand/wiegand-gpio.yaml        | 46 +++++++++++++++++++
>  MAINTAINERS                                   |  5 ++
>  2 files changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml b/Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml
> new file mode 100644
> index 000000000000..cf2cb938de02
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/wiegand/wiegand-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GPIO bitbanged Wiegand interface properties
> +
> +maintainers:
> +  - Martin Zaťovič <m.zatovic1@gmail.com>
> +
> +description:
> +  This represents the GPIO lines used for bit-banged Wiegand on dedicated GPIO
> +  lines.
> +
> +allOf:
> +  - $ref: /schemas/wiegand/wiegand-controller.yaml#
> +
> +properties:
> +  compatible:
> +    const: wiegand-gpio
> +
> +  data-gpios:
> +    description: GPIOs used as Wiegand data lines, DATA0 and DATA1 respectivelly.
> +    maxItems: 2
> +
> +required:
> +  - compatible
> +  - data-gpios
> +
> +unevaluatedProperties: false

You'll find this fails if you add an actual child node. Assuming you go 
with only 1 child node allowed, then you need:

unevaluatedProperties:
  type: object

(There's not any way to say only 1 child, but multiple would have 
constraints in the common binding as I explained.)

Rob

[PATCHv5 4/4] wiegand: add Wiegand GPIO bitbanged controller driver

[Martin Zaťovič]

This controller formats the data to a Wiegand format and bit-bangs
the message on devicetree defined GPIO lines.

The driver creates a dev file for writing messages on the bus.
It also creates a sysfs file to control the payload length of
messages(in bits). If a message is shorter than the set payload
length, it will be discarded. On the other hand, if a message is
longer, the additional bits will be stripped off.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Martin Zaťovič <m.zatovic1@gmail.com>
---
 .../ABI/testing/sysfs-driver-wiegand-gpio     |   9 +
 MAINTAINERS                                   |   2 +
 drivers/wiegand/Kconfig                       |  11 +
 drivers/wiegand/Makefile                      |   1 +
 drivers/wiegand/wiegand-gpio.c                | 192 ++++++++++++++++++
 5 files changed, 215 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-wiegand-gpio
 create mode 100644 drivers/wiegand/wiegand-gpio.c

diff --git a/Documentation/ABI/testing/sysfs-driver-wiegand-gpio b/Documentation/ABI/testing/sysfs-driver-wiegand-gpio
new file mode 100644
index 000000000000..6bf6c2b93bf7
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-wiegand-gpio
@@ -0,0 +1,9 @@
+What:		/sys/devices/platform/.../wiegand-gpio-attributes/payload_len
+Date:		August 2023
+Contact:	Martin Zaťovič <m.zatovic1@gmail.com>
+Description:
+		Read/set the payload length of messages sent by Wiegand GPIO
+		bit-banging controller in bits. The default value is 26, as
+		that is the most widely-used message length of Wiegand messages.
+		Controller will only send messages of at least the set length
+		and it will strip off bits of longer messages.
diff --git a/MAINTAINERS b/MAINTAINERS
index fb158e74ca4b..221ffa766ed0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22932,7 +22932,9 @@ F:	include/linux/wiegand.h
 WIEGAND GPIO BITBANG DRIVER
 M:	Martin Zaťovič <m.zatovic1@gmail.com>
 S:	Maintained
+F:	Documentation/ABI/testing/sysfs-driver-wiegand-gpio
 F:	Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml
+F:	drivers/wiegand/wiegand-gpio.c
 
 WILOCITY WIL6210 WIRELESS DRIVER
 L:	linux-wireless@vger.kernel.org
diff --git a/drivers/wiegand/Kconfig b/drivers/wiegand/Kconfig
index 09ac10217ede..17fd367a9004 100644
--- a/drivers/wiegand/Kconfig
+++ b/drivers/wiegand/Kconfig
@@ -23,3 +23,14 @@ config WIEGAND
 
 	  This Wiegand support can also be built as a module. If so, the module
 	  will be called wiegand.
+
+config WIEGAND_GPIO
+	tristate "GPIO-based wiegand master (write only)"
+	depends on WIEGAND
+	help
+	  This GPIO bitbanging Wiegand controller uses the libgpiod library to
+	  utilize GPIO lines for sending Wiegand data. Userspace may access the
+	  Wiegand GPIO interface via a dev file.
+
+	  This support is also available as a module. If so, the module will be
+	  called wiegand-gpio.
diff --git a/drivers/wiegand/Makefile b/drivers/wiegand/Makefile
index d17ecb722c6e..ddf697e21088 100644
--- a/drivers/wiegand/Makefile
+++ b/drivers/wiegand/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_WIEGAND)			+= wiegand.o
+obj-$(CONFIG_WIEGAND_GPIO)		+= wiegand-gpio.o
diff --git a/drivers/wiegand/wiegand-gpio.c b/drivers/wiegand/wiegand-gpio.c
new file mode 100644
index 000000000000..448bfefaa7d9
--- /dev/null
+++ b/drivers/wiegand/wiegand-gpio.c
@@ -0,0 +1,192 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/poll.h>
+#include <linux/slab.h>
+#include <linux/wiegand.h>
+
+#define WIEGAND_DEFAULT_PAYLOAD_LEN 26
+
+struct wiegand_gpio {
+	struct device *dev;
+	struct wiegand_controller *ctlr;
+	struct gpio_descs *gpios;
+};
+
+static ssize_t store_ulong(u32 *val, const char *buf, size_t size, unsigned long max)
+{
+	int rc;
+	u32 new;
+
+	rc = kstrtou32(buf, 0, &new);
+	if (rc)
+		return rc;
+
+	if (new > max)
+		return -ERANGE;
+
+	*val = new;
+	return size;
+}
+
+/*
+ * Attribute file for setting payload length of Wiegand messages.
+ */
+static ssize_t payload_len_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct wiegand_gpio *wiegand_gpio = dev_get_drvdata(dev);
+	struct wiegand_controller *ctlr = wiegand_gpio->ctlr;
+
+	return sysfs_emit(buf, "%u\n", ctlr->payload_len);
+}
+
+static ssize_t payload_len_store(struct device *dev, struct device_attribute *attr, const char *buf,
+			size_t count)
+{
+	struct wiegand_gpio *wiegand_gpio = dev_get_drvdata(dev);
+	struct wiegand_controller *ctlr = wiegand_gpio->ctlr;
+
+	return store_ulong(&(ctlr->payload_len), buf, count, WIEGAND_MAX_PAYLEN_BYTES * 8);
+}
+static DEVICE_ATTR_RW(payload_len);
+
+static struct attribute *wiegand_gpio_attrs[] = {
+	&dev_attr_payload_len.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(wiegand_gpio);
+
+/**
+ * wiegand_gpio_send_bit - Sends a single bit
+ *
+ * @wiegand_gpio: Instance of the Wiegand
+ * @value: Bit value to send
+ * @last: Indicates last bit of a message
+ *
+ * To send a bit of value 1 following the wiegand protocol, one must set
+ * the wiegand_data_hi to low for the duration of pulse. Similarly to send
+ * a bit of value 0, the wiegand_data_lo is set to low for pulse duration.
+ * This way the two lines are never low at the same time.
+ */
+void wiegand_gpio_send_bit(struct wiegand_gpio *wiegand_gpio, bool value, bool last)
+{
+	u32 sleep_len;
+	struct wiegand_timing *timing = &wiegand_gpio->ctlr->timing;
+	u32 pulse_len = timing->pulse_len;
+	u32 interval_len = timing->interval_len;
+	u32 frame_gap = timing->frame_gap;
+	struct gpio_desc *gpio = value ? wiegand_gpio->gpios->desc[1] :
+						wiegand_gpio->gpios->desc[0];
+
+	gpiod_set_value_cansleep(gpio, 0);
+	udelay(pulse_len);
+	gpiod_set_value_cansleep(gpio, 1);
+
+	if (last)
+		sleep_len = frame_gap - pulse_len;
+	else
+		sleep_len = interval_len - pulse_len;
+
+	fsleep(sleep_len);
+}
+
+static int wiegand_gpio_write_by_bits(struct wiegand_gpio *wiegand_gpio, u16 bitlen)
+{
+	unsigned int i;
+	bool value;
+
+	for (i = 0; i < bitlen - 1; i++) {
+		value = test_bit(i, wiegand_gpio->ctlr->data_bitmap);
+		wiegand_gpio_send_bit(wiegand_gpio, value, false);
+	}
+
+	value = test_bit(bitlen - 1, wiegand_gpio->ctlr->data_bitmap);
+	wiegand_gpio_send_bit(wiegand_gpio, value, true);
+
+	return 0;
+}
+
+int wiegand_gpio_transfer_message(struct wiegand_controller *ctlr)
+{
+	struct wiegand_gpio *wiegand_gpio = wiegand_primary_get_devdata(ctlr);
+	u8 msg_bitlen = ctlr->payload_len;
+
+	wiegand_gpio_write_by_bits(wiegand_gpio, msg_bitlen);
+
+	return 0;
+}
+
+static int wiegand_gpio_request(struct device *dev, struct wiegand_gpio *wiegand_gpio)
+{
+	wiegand_gpio->gpios = devm_gpiod_get_array(dev, "data", GPIOD_OUT_HIGH);
+
+	if (IS_ERR(wiegand_gpio->gpios))
+		dev_err(dev, "unable to get gpios\n");
+
+	return PTR_ERR_OR_ZERO(wiegand_gpio->gpios);
+}
+
+static int wiegand_gpio_probe(struct platform_device *device)
+{
+	int status;
+	struct wiegand_controller *primary;
+	struct wiegand_gpio *wiegand_gpio;
+	struct device *dev = &device->dev;
+
+	primary = devm_wiegand_alloc_controller(dev, sizeof(*wiegand_gpio), false);
+	if (!primary)
+		return -ENOMEM;
+
+	primary->transfer_message = &wiegand_gpio_transfer_message;
+	primary->payload_len = WIEGAND_DEFAULT_PAYLOAD_LEN;
+
+	wiegand_gpio = wiegand_primary_get_devdata(primary);
+	wiegand_gpio->ctlr = primary;
+
+	platform_set_drvdata(device, wiegand_gpio);
+
+	primary->bus_num = device->id;
+	wiegand_gpio->dev = dev;
+
+	status = wiegand_gpio_request(dev, wiegand_gpio);
+	if (status)
+		return dev_err_probe(dev, status, "failed at requesting GPIOs\n");
+
+	status = gpiod_direction_output(wiegand_gpio->gpios->desc[0], 1);
+	if (status)
+		return dev_err_probe(dev, status, "failed to set GPIOs direction\n");
+
+	status = gpiod_direction_output(wiegand_gpio->gpios->desc[1], 1);
+	if (status)
+		return dev_err_probe(dev, status, "failed to set GPIOs direction\n");
+
+	status = devm_wiegand_register_controller(dev, primary);
+	if (status)
+		dev_err_probe(dev, status, "failed to register primary\n");
+	return 0;
+}
+
+static const struct of_device_id wiegand_gpio_dt_idtable[] = {
+	{ .compatible = "wiegand-gpio", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, wiegand_gpio_dt_idtable);
+
+static struct platform_driver wiegand_gpio_driver = {
+	.driver = {
+		.name	= "wiegand-gpio",
+		.of_match_table = wiegand_gpio_dt_idtable,
+		.dev_groups = wiegand_gpio_groups,
+	},
+	.probe		= wiegand_gpio_probe,
+};
+module_platform_driver(wiegand_gpio_driver);
+
+MODULE_IMPORT_NS(WIEGAND);
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Wiegand write-only driver realized by GPIOs");
+MODULE_AUTHOR("Martin Zaťovič <m.zatovic1@gmail.com>");
-- 
2.40.1

Re: [PATCHv5 4/4] wiegand: add Wiegand GPIO bitbanged controller driver

[Andy Shevchenko]

On Thu, Aug 24, 2023 at 01:10:15PM +0200, Martin Zaťovič wrote:
> This controller formats the data to a Wiegand format and bit-bangs
> the message on devicetree defined GPIO lines.
> 
> The driver creates a dev file for writing messages on the bus.
> It also creates a sysfs file to control the payload length of
> messages(in bits). If a message is shorter than the set payload
> length, it will be discarded. On the other hand, if a message is
> longer, the additional bits will be stripped off.

...

> +Date:		August 2023

Unrealistic. Use https://hansen.beer/~dave/phb/ to define Date and
KernelVersion fields.

...

> +#define WIEGAND_DEFAULT_PAYLOAD_LEN 26

_LEN --> _BITS

...

> +	return store_ulong(&(ctlr->payload_len), buf, count, WIEGAND_MAX_PAYLEN_BYTES * 8);

Yeah, again use _BITS directly.

...

> +/**
> + * wiegand_gpio_send_bit - Sends a single bit

Do not forget to validate this with scripts/kernel-doc.

> + * @wiegand_gpio: Instance of the Wiegand
> + * @value: Bit value to send
> + * @last: Indicates last bit of a message
> + *
> + * To send a bit of value 1 following the wiegand protocol, one must set
> + * the wiegand_data_hi to low for the duration of pulse. Similarly to send
> + * a bit of value 0, the wiegand_data_lo is set to low for pulse duration.
> + * This way the two lines are never low at the same time.
> + */

...

> +	struct gpio_desc *gpio = value ? wiegand_gpio->gpios->desc[1] :
> +						wiegand_gpio->gpios->desc[0];

Make it separate line.

	gpio = value ? wiegand_gpio->gpios->desc[1] : wiegand_gpio->gpios->desc[0];

...

> +	wiegand_gpio->gpios = devm_gpiod_get_array(dev, "data", GPIOD_OUT_HIGH);

> +

Redundant blank line.

> +	if (IS_ERR(wiegand_gpio->gpios))
> +		dev_err(dev, "unable to get gpios\n");
> +
> +	return PTR_ERR_OR_ZERO(wiegand_gpio->gpios);

Oh.

	wiegand_gpio->gpios = devm_gpiod_get_array(dev, "data", GPIOD_OUT_HIGH);
	ret = PTR_...();
	if (ret)
		dev_err();

	return ret;

...

> +static int wiegand_gpio_probe(struct platform_device *device)

	device --> pdev

...

> +static const struct of_device_id wiegand_gpio_dt_idtable[] = {
> +	{ .compatible = "wiegand-gpio", },

Inner comma is not needed.

> +	{}
> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCHv5 4/4] wiegand: add Wiegand GPIO bitbanged controller driver

[Andy Shevchenko]

On Thu, Aug 24, 2023 at 04:35:08PM +0300, Andy Shevchenko wrote:
> On Thu, Aug 24, 2023 at 01:10:15PM +0200, Martin Zaťovič wrote:

...

> > +Date:		August 2023
> 
> Unrealistic. Use https://hansen.beer/~dave/phb/ to define Date and
> KernelVersion fields.

Hint: should be for v6.7 at least.

-- 
With Best Regards,
Andy Shevchenko