[PATCHv2 0/4] Wiegand bus driver and GPIO bit-banged controller

[PATCHv2 0/4] Wiegand bus driver and GPIO bit-banged controller

[Martin Zaťovič]

Hello,

Thank you for your feedback on the last version.

I came to a realization, that it is for the best to let message
formats and checksum calculation be handled by device drivers.
Support of these options was removed from the bus driver.

The GPIO bitbanging controller driver contained two attribute
files - format and payload_len. The format file is obviously not
needed anymore, however I have decided to keep the payload_len
file. It seems to me to be the best way to communicate this
information to the controller driver. This information needs
to be provided especially in order for the /dev file to work
properly(as the driver has no idea where the message ends). If
there is a better way to approach this problem, please let me
know.

Device drivers will not face the same problem, as the length of
a message is passed with every call of the transfer_message()
function.

CHANGELOG since v1:
- added dt-bindings for wiegand_gpio driver
- dt_binding_check now passes
- added help texts to Kconfig files
- removed controller list from bus driver
- removed the option to add a device from another driver (along
  with wiegand_baord_info structure)
- moved the bus driver to drivers/wiegand/
- removed all explicit castings, used specific getters instead
- fixed indentation
- removed fromat attribute from controller structure
- removed format implementation from wiegand_gpio driver

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

 .../ABI/testing/sysfs-driver-wiegand-gpio     |   9 +
 .../bindings/wiegand/wiegand-controller.yaml  |  50 ++
 .../bindings/wiegand/wiegand-gpio.yaml        |  51 ++
 MAINTAINERS                                   |  14 +
 drivers/Kconfig                               |   2 +
 drivers/Makefile                              |   1 +
 drivers/wiegand/Kconfig                       |  28 +
 drivers/wiegand/Makefile                      |   2 +
 drivers/wiegand/wiegand-gpio.c                | 324 +++++++++++
 drivers/wiegand/wiegand.c                     | 543 ++++++++++++++++++
 include/linux/wiegand.h                       | 177 ++++++
 11 files changed, 1201 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.39.1

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

[Martin Zaťovič]

Weigand 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.

Signed-off-by: Martin Zaťovič <m.zatovic1@gmail.com>
---
 .../bindings/wiegand/wiegand-controller.yaml  | 50 +++++++++++++++++++
 MAINTAINERS                                   |  5 ++
 2 files changed, 55 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..fed90e01e56f
--- /dev/null
+++ b/Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
@@ -0,0 +1,50 @@
+# 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č <martin.zatovic@tbs-biometrics.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
+
+examples:
+  - |
+    wiegand@f00 {
+        compatible = "wiegand-foo";
+        pulse-len-us = <50>;
+        interval-len-us = <2000>;
+        frame-gap-us = <2000>;
+
+        /* devices */
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 7f86d02cb427..db9624d93af0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22428,6 +22428,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.39.1

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

[Rob Herring]

On Thu, Feb 02, 2023 at 03:33:02PM +0100, Martin Zaťovič wrote:
> Weigand 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.
> 
> Signed-off-by: Martin Zaťovič <m.zatovic1@gmail.com>
> ---
>  .../bindings/wiegand/wiegand-controller.yaml  | 50 +++++++++++++++++++
>  MAINTAINERS                                   |  5 ++
>  2 files changed, 55 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..fed90e01e56f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
> @@ -0,0 +1,50 @@
> +# 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č <martin.zatovic@tbs-biometrics.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
> +
> +examples:
> +  - |
> +    wiegand@f00 {
> +        compatible = "wiegand-foo";

You've got a real example in the bitbanged schema, just drop the fake 
one here. You can put about anything in here because it is not getting 
validated.

> +        pulse-len-us = <50>;
> +        interval-len-us = <2000>;
> +        frame-gap-us = <2000>;
> +
> +        /* devices */
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7f86d02cb427..db9624d93af0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22428,6 +22428,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.39.1
> 

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

[Martin Zaťovič]

Add a bus driver for Wiegand protocol. The 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 represents a master and contains
attributes such as the payload_len for configuring the size
of a single Wiegand message in bits. It also stores the
controller attributes defined in the devicetree.

Each Wiegand controller should be associated with one Wiegand
device, as Wiegand is typically a point-to-point bus.

Signed-off-by: Martin Zaťovič <m.zatovic1@gmail.com>
---
 MAINTAINERS               |   2 +
 drivers/Kconfig           |   2 +
 drivers/Makefile          |   1 +
 drivers/wiegand/Kconfig   |  20 ++
 drivers/wiegand/Makefile  |   1 +
 drivers/wiegand/wiegand.c | 543 ++++++++++++++++++++++++++++++++++++++
 include/linux/wiegand.h   | 177 +++++++++++++
 7 files changed, 746 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 db9624d93af0..8119d12dac41 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22432,6 +22432,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 968bd0a6fd78..bedc5a9fecba 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -67,6 +67,8 @@ source "drivers/spi/Kconfig"
 
 source "drivers/spmi/Kconfig"
 
+source "drivers/wiegand/Kconfig"
+
 source "drivers/hsi/Kconfig"
 
 source "drivers/pps/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index bdf1c66141c9..c5b613e2045a 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -146,6 +146,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..fc99575bc3cc
--- /dev/null
+++ b/drivers/wiegand/Kconfig
@@ -0,0 +1,20 @@
+config WIEGAND
+        tristate "Wiegand Bus driver"
+        help
+	  The "Wiegand Interface" is an asynchronous low-level protocol
+	  or wiring standard. It is typically used for point-to-point
+	  communication. The data length of Wiegand messages is not defined,
+	  so the devices usually default to 26, 36 or 37 bits per message.
+	  The throughput of Wiegand depends on the selected pulse length and
+	  the intervals between pulses, in comparison to other busses it
+	  is generally rather slow.
+
+	  Despite its higher age, Wiegand remains widely used in access
+	  control systems to connect a card swipe mechanism. Such mechanisms
+	  utilize the Wiegand effect to transfer data from the card to
+	  the reader.
+
+	  Wiegand uses two wires to transmit the data D0 and D1. Both lines
+	  are initially pulled up. When a bit of value 0 is being transmitted,
+	  the D0 line is pulled down. Similarly, when a bit of value 1 is being
+	  transmitted, the D1 line is pulled down.
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..1147195fc256
--- /dev/null
+++ b/drivers/wiegand/wiegand.c
@@ -0,0 +1,543 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/slab.h>
+#include <linux/wiegand.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/property.h>
+
+static struct bus_type wiegand_bus_type;
+static DEFINE_IDR(wiegand_controller_idr);
+
+static void devm_wiegand_release_controller(struct device *dev, void *ctlr)
+{
+	wiegand_controller_put(*(struct wiegand_controller **)ctlr);
+}
+
+static void wiegand_controller_release(struct device *dev)
+{
+	struct wiegand_controller *ctlr;
+
+	ctlr = container_of(dev, struct wiegand_controller, dev);
+	kfree(ctlr);
+}
+
+static struct class wiegand_controller_class = {
+	.name = "wiegand_master",
+	.owner = THIS_MODULE,
+	.dev_release = wiegand_controller_release,
+};
+
+static DEFINE_MUTEX(board_lock);
+
+struct wiegand_controller *__wiegand_alloc_controller(struct device *dev,
+						unsigned int size, bool slave)
+{
+	struct wiegand_controller *ctlr;
+	size_t ctlr_size = ALIGN(sizeof(*ctlr), dma_get_cache_alignment());
+
+	if (!dev)
+		return NULL;
+
+	ctlr = kzalloc(size + ctlr_size, GFP_KERNEL);
+	if (!ctlr)
+		return NULL;
+
+	device_initialize(&ctlr->dev);
+	ctlr->bus_num = -1;
+	ctlr->slave = slave;
+	ctlr->dev.class = &wiegand_controller_class;
+	ctlr->dev.parent = dev;
+	wiegand_controller_set_devdata(ctlr, (void *)ctlr + ctlr_size);
+
+	return ctlr;
+}
+EXPORT_SYMBOL_GPL(__wiegand_alloc_controller);
+
+struct wiegand_controller *__devm_wiegand_alloc_controller(struct device *dev,
+							unsigned int size,
+							bool slave)
+{
+	struct wiegand_controller **ptr, *ctlr;
+
+	ptr = devres_alloc(devm_wiegand_release_controller, sizeof(*ptr),
+								GFP_KERNEL);
+	if (!ptr)
+		return NULL;
+
+	ctlr = __wiegand_alloc_controller(dev, size, slave);
+	if (ctlr) {
+		ctlr->devm_allocated = true;
+		*ptr = ctlr;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return ctlr;
+}
+EXPORT_SYMBOL_GPL(__devm_wiegand_alloc_controller);
+
+static int wiegand_controller_check_ops(struct wiegand_controller *ctlr)
+{
+	if (!ctlr->transfer_message)
+		return -EINVAL;
+	return 0;
+}
+
+static struct wiegand_device *of_register_wiegand_device(
+						struct wiegand_controller *ctlr,
+						struct device_node *nc)
+{
+	struct wiegand_device *wiegand;
+	int rc;
+
+	wiegand = wiegand_alloc_device(ctlr);
+	if (!wiegand) {
+		dev_err(&ctlr->dev, "wiegad_device alloc error for %pOF\n", nc);
+		rc = -ENOMEM;
+		goto err_out;
+	}
+
+	rc = of_modalias_node(nc, wiegand->modalias, sizeof(wiegand->modalias));
+	if (rc < 0) {
+		dev_err(&ctlr->dev, "cannot find modalias for %pOF\n", nc);
+		goto err_out;
+	}
+
+	of_node_get(nc);
+	wiegand->dev.of_node = nc;
+	wiegand->dev.fwnode = of_fwnode_handle(nc);
+
+	rc = wiegand_add_device(wiegand);
+	if (rc) {
+		dev_err(&ctlr->dev, "wiegand_device register error %pOF\n", nc);
+		goto err_of_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;
+
+err_of_node_put:
+	of_node_put(nc);
+err_out:
+	wiegand_dev_put(wiegand);
+	return ERR_PTR(rc);
+}
+
+static void of_register_wiegand_devices(struct wiegand_controller *ctlr)
+{
+	struct wiegand_device *wiegand;
+	struct device_node *nc;
+
+	if (!ctlr->dev.of_node)
+		return;
+
+	for_each_available_child_of_node(ctlr->dev.of_node, nc) {
+		if (of_node_test_and_set_flag(nc, OF_POPULATED))
+			continue;
+		wiegand = of_register_wiegand_device(ctlr, nc);
+		if (IS_ERR(wiegand)) {
+			dev_warn(&ctlr->dev,
+				 "Failed to create wiegand device for %pOF\n",
+								nc);
+			of_node_clear_flag(nc, OF_POPULATED);
+		}
+	}
+}
+
+/*
+ * Controllers that do not have a devicetree entry need to initialize the
+ * following struct wiegand_controller attributes: pulse_len, interval_len and
+ * frame_gap.
+ */
+int wiegand_register_controller(struct wiegand_controller *ctlr)
+{
+	struct device *dev = ctlr->dev.parent;
+	int status, id, first_dynamic;
+
+	if (!dev)
+		return -ENODEV;
+
+	status = wiegand_controller_check_ops(ctlr);
+	if (status)
+		return status;
+
+	if (ctlr->dev.of_node) {
+		id = of_alias_get_id(ctlr->dev.of_node, "wiegand");
+		if (id > 0) {
+			ctlr->bus_num = id;
+			mutex_lock(&board_lock);
+			id = idr_alloc(&wiegand_controller_idr, ctlr,
+							ctlr->bus_num,
+							ctlr->bus_num + 1,
+							GFP_KERNEL);
+			mutex_unlock(&board_lock);
+			if (WARN(id < 0, "couldn't get idr"))
+				return id == -ENOSPC ? -EBUSY : id;
+		}
+		device_property_read_u32(&ctlr->dev, "pulse-len-us",
+							&ctlr->pulse_len);
+		device_property_read_u32(&ctlr->dev, "interval-len-us",
+							&ctlr->interval_len);
+		device_property_read_u32(&ctlr->dev, "frame-gap-us",
+							&ctlr->frame_gap);
+	}
+	if (ctlr->bus_num < 0) {
+		first_dynamic = of_alias_get_highest_id("wiegand");
+		if (first_dynamic < 0)
+			first_dynamic = 0;
+		else
+			first_dynamic++;
+
+		mutex_lock(&board_lock);
+		id = idr_alloc(&wiegand_controller_idr, ctlr, first_dynamic,
+								0, GFP_KERNEL);
+		mutex_unlock(&board_lock);
+		if (WARN(id < 0, "couldn't get idr\n"))
+			return id;
+		ctlr->bus_num = id;
+	}
+
+	if (ctlr->pulse_len == 0)
+		dev_warn(&ctlr->dev, "pulse_len is not initialized\n");
+	if (ctlr->interval_len == 0)
+		dev_warn(&ctlr->dev, "interval_len is not initialized\n");
+	if (ctlr->frame_gap == 0)
+		dev_warn(&ctlr->dev, "frame_gap is not initialized\n");
+
+	dev_set_name(&ctlr->dev, "wiegand%u", ctlr->bus_num);
+	ctlr->device_count = 0;
+
+	status = device_add(&ctlr->dev);
+	if (status < 0)
+		goto free_bus_id;
+
+	of_register_wiegand_devices(ctlr);
+
+	return status;
+
+free_bus_id:
+	mutex_lock(&board_lock);
+	idr_remove(&wiegand_controller_idr, ctlr->bus_num);
+	mutex_unlock(&board_lock);
+	return status;
+}
+
+static int __unregister(struct device *dev, void *null)
+{
+	wiegand_unregister_device(to_wiegand_device(dev));
+	return 0;
+}
+
+void wiegand_unregister_controller(struct wiegand_controller *ctlr)
+{
+	struct wiegand_controller *found;
+	int id = ctlr->bus_num;
+
+	device_for_each_child(&ctlr->dev, NULL, __unregister);
+	found = idr_find(&wiegand_controller_idr, id);
+	device_del(&ctlr->dev);
+
+	mutex_lock(&board_lock);
+	if (found == ctlr)
+		idr_remove(&wiegand_controller_idr, id);
+	mutex_unlock(&board_lock);
+
+	if (!ctlr->devm_allocated)
+		put_device(&ctlr->dev);
+}
+EXPORT_SYMBOL_GPL(wiegand_unregister_controller);
+
+static void devm_wiegand_unregister(struct device *dev, void *res)
+{
+	wiegand_unregister_controller(*(struct wiegand_controller **)res);
+}
+
+int devm_wiegand_register_controller(struct device *dev,
+				     struct wiegand_controller *ctlr)
+{
+	struct wiegand_controller **ptr;
+	int ret;
+
+	ptr = devres_alloc(devm_wiegand_unregister, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	ret = wiegand_register_controller(ctlr);
+	if (!ret) {
+		*ptr = ctlr;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_wiegand_register_controller);
+
+static int __wiegand_master_match(struct device *dev, const void *data)
+{
+	struct wiegand_master *master;
+	const u16 *bus_num = data;
+
+	master = container_of(dev, struct wiegand_master, dev);
+	return master->bus_num == *bus_num;
+}
+
+struct wiegand_master *wiegand_busnum_to_master(u16 bus_num)
+{
+	struct device *dev;
+	struct wiegand_master *master = NULL;
+
+	dev = class_find_device(&wiegand_controller_class, NULL, &bus_num,
+				__wiegand_master_match);
+	if (dev)
+		master = container_of(dev, struct wiegand_master, dev);
+
+	return master;
+}
+EXPORT_SYMBOL_GPL(wiegand_busnum_to_master);
+
+/* Device section */
+
+static void wieganddev_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;
+
+	if (!wiegand_controller_get(ctlr))
+		return NULL;
+
+	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 = wieganddev_release;
+
+	device_initialize(&wiegand->dev);
+	return wiegand;
+}
+EXPORT_SYMBOL_GPL(wiegand_alloc_device);
+
+static void wiegand_cleanup(struct wiegand_device *wiegand)
+{
+	if (wiegand->controller->cleanup)
+		wiegand->controller->cleanup(wiegand);
+}
+
+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;
+}
+
+static void wiegand_dev_set_name(struct wiegand_device *wiegand, u8 id)
+{
+	dev_set_name(&wiegand->dev, "%s.%u",
+			dev_name(&wiegand->controller->dev), id);
+}
+
+int wiegand_add_device(struct wiegand_device *wiegand)
+{
+	struct wiegand_controller *ctlr = wiegand->controller;
+	int status;
+
+	wiegand_dev_set_name(wiegand, ctlr->device_count);
+
+	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 = 0;
+
+	if (wiegand->controller->setup) {
+		status = wiegand->controller->setup(wiegand);
+		if (status) {
+			dev_err(&wiegand->controller->dev,
+						"Failed to setup device: %d\n",
+									status);
+			return status;
+		}
+	}
+
+	return status;
+}
+EXPORT_SYMBOL_GPL(wiegand_setup);
+
+void wiegand_unregister_device(struct wiegand_device *wiegand)
+{
+	if (!wiegand)
+		return;
+
+	if (wiegand->dev.of_node) {
+		of_node_clear_flag(wiegand->dev.of_node, OF_POPULATED);
+		of_node_put(wiegand->dev.of_node);
+	}
+	device_del(&wiegand->dev);
+	wiegand_cleanup(wiegand);
+	put_device(&wiegand->dev);
+}
+EXPORT_SYMBOL_GPL(wiegand_unregister_device);
+
+int wiegand_send_message(struct wiegand_device *wiegand, u8 *message, u8 bitlen)
+{
+	struct wiegand_master *master = wiegand->controller;
+
+	if (message == NULL || message == 0)
+		return -EINVAL;
+
+	if (master->transfer_message)
+		master->transfer_message(wiegand, message, bitlen);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(wiegand_send_message);
+
+static int wiegand_match_device(struct device *dev, struct device_driver *drv)
+{
+	const struct wiegand_device *wiegand = to_wiegand_device(dev);
+
+	if (of_driver_match_device(dev, drv))
+		return 1;
+
+	return strcmp(wiegand->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);
+	int ret = 0;
+
+	if (wdrv->probe)
+		ret = wdrv->probe(wiegand);
+
+	return ret;
+}
+
+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));
+}
+
+static struct bus_type wiegand_bus_type = {
+	.name		= "wiegand",
+	.match		= wiegand_match_device,
+	.probe		= wiegand_probe,
+	.remove		= wiegand_remove,
+};
+
+int __wiegand_register_driver(struct module *owner, struct wiegand_driver *wdrv)
+{
+	wdrv->driver.owner = owner;
+	wdrv->driver.bus = &wiegand_bus_type;
+
+	if (wdrv->driver.of_match_table) {
+		const struct of_device_id *of_id;
+
+		for (of_id = wdrv->driver.of_match_table; of_id->compatible[0];
+		     of_id++) {
+			const char *of_name;
+
+			/* remove vendor prefix */
+			of_name = strnchr(of_id->compatible,
+					  sizeof(of_id->compatible), ',');
+			if (of_name)
+				of_name++;
+			else
+				of_name = of_id->compatible;
+
+			if (strcmp(wdrv->driver.name, of_name) == 0)
+				continue;
+
+			pr_warn("Wiegand driver %s has no device_id for %s\n",
+				wdrv->driver.name, of_id->compatible);
+		}
+	}
+
+	return driver_register(&wdrv->driver);
+}
+EXPORT_SYMBOL_GPL(__wiegand_register_driver);
+
+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);
+		goto err0;
+	}
+
+	ret = class_register(&wiegand_controller_class);
+	if (ret < 0)
+		goto err1;
+
+	return 0;
+
+err1:
+	bus_unregister(&wiegand_bus_type);
+err0:
+	return ret;
+}
+
+static void __exit wiegand_exit(void)
+{
+	bus_unregister(&wiegand_bus_type);
+	class_unregister(&wiegand_controller_class);
+}
+postcore_initcall_sync(wiegand_init);
+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..dc733af464c4
--- /dev/null
+++ b/include/linux/wiegand.h
@@ -0,0 +1,177 @@
+/* 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/mutex.h>
+#include <linux/mod_devicetable.h>
+
+#define WIEGAND_NAME_SIZE 32
+
+extern struct bus_type wiegand_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];
+};
+
+/**
+ * struct wiegand_controller - Wiegand master or slave interface
+ * @dev - Device interface of the controller
+ * @list - Link with the global wiegand_controller list
+ * @bus_num - Board-specific identifier for Wiegand controller
+ * @pulse_len: length of the low pulse in usec; defaults to 50us
+ * @interval_len: length of a whole bit (both the pulse and the high phase) in
+ *	usec; defaults to 2000us
+ * @frame_gap: length of the last bit of a frame (both the pulse and the high
+ *	phase) in usec; defaults to interval_len
+ * device_count - Counter of devices connected to the same Wiegand
+ *	bus(controller).
+ * devm_allocated - Whether the allocation of this struct is devres-managed
+ * slave - Whether the controller is a slave(receives data).
+ * transfer_message - Send a message on the bus.
+ * setup - Setup a device.
+ * cleanup - Cleanup after a device.
+ */
+struct wiegand_controller {
+	struct device dev;
+	struct list_head list;
+
+	s16 bus_num;
+
+	u32 pulse_len;
+	u32 interval_len;
+	u32 frame_gap;
+
+	u32 payload_len;
+
+	u8 device_count;
+
+	bool devm_allocated;
+	bool slave;
+
+	int (*transfer_message)(struct wiegand_device *dev, u8 *message,
+								u8 bitlen);
+
+	int (*setup)(struct wiegand_device *wiegand);
+	void (*cleanup)(struct wiegand_device *wiegand);
+};
+
+struct wiegand_driver {
+	const struct wiegand_device_id *id_table;
+	int (*probe)(struct wiegand_device *wiegand);
+	void (*remove)(struct wiegand_device *wiegand);
+	struct device_driver driver;
+};
+
+/* Wiegand controller section */
+
+#define wiegand_master wiegand_controller
+extern struct wiegand_controller *__wiegand_alloc_controller(
+							struct device *host,
+							unsigned int size,
+							bool slave);
+
+struct wiegand_controller *__devm_wiegand_alloc_controller(struct device *dev,
+						   unsigned int size,
+						   bool slave);
+struct wiegand_controller *__wiegand_alloc_controller(struct device *dev,
+							unsigned int size,
+							bool slave);
+static inline struct wiegand_controller *devm_wiegand_alloc_master(
+							struct device *dev,
+							unsigned int size)
+{
+	return __devm_wiegand_alloc_controller(dev, size, false);
+}
+extern int wiegand_register_controller(struct wiegand_controller *ctlr);
+extern int devm_wiegand_register_controller(struct device *dev,
+					struct wiegand_controller *ctlr);
+#define wiegand_register_master(_ctlr) wiegand_register_controller(_ctlr)
+#define devm_wiegand_register_master(_dev, _ctlr) \
+	devm_wiegand_register_controller(_dev, _ctlr)
+extern void wiegand_unregister_controller(struct wiegand_controller *ctlr);
+#define wiegand_unregister_master(_ctlr) wiegand_unregister_controller(_ctlr)
+extern struct wiegand_master *wiegand_busnum_to_master(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_master_get_devdata(_ctlr) \
+	wiegand_controller_get_devdata(_ctlr)
+#define wiegand_master_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(struct wiegand_controller *ctlr)
+{
+	if (ctlr)
+		put_device(&ctlr->dev);
+}
+
+/* Wiegand device section */
+
+extern struct wiegand_device *wiegand_alloc_device(
+					struct wiegand_controller *ctlr);
+extern int wiegand_add_device(struct wiegand_device *wiegand);
+extern int wiegand_setup(struct wiegand_device *wiegand);
+extern void wiegand_unregister_device(struct wiegand_device *wiegand);
+
+extern int wiegand_send_message(struct wiegand_device *wiegand, u8 *message,
+								u8 bitlen);
+
+static inline struct wiegand_device *to_wiegand_device(struct device *dev)
+{
+	return dev ? container_of(dev, struct wiegand_device, dev) : NULL;
+}
+static inline void wiegand_dev_put(struct wiegand_device *wiegand)
+{
+	if (wiegand)
+		put_device(&wiegand->dev);
+}
+
+/* Wiegand driver section  */
+
+static inline struct wiegand_driver *to_wiegand_driver(struct device_driver *drv)
+{
+	return drv ? container_of(drv, struct wiegand_driver, driver) : NULL;
+}
+extern 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);
+}
+
+#endif	/* H_LINUX_INCLUDE_LINUX_WIEGAND_H */
-- 
2.39.1

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

[Greg KH]

On Thu, Feb 02, 2023 at 03:33:03PM +0100, Martin Zaťovič wrote:
> Add a bus driver for Wiegand protocol. The 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 represents a master and contains
> attributes such as the payload_len for configuring the size
> of a single Wiegand message in bits. It also stores the
> controller attributes defined in the devicetree.
> 
> Each Wiegand controller should be associated with one Wiegand
> device, as Wiegand is typically a point-to-point bus.
> 
> Signed-off-by: Martin Zaťovič <m.zatovic1@gmail.com>

Looking better, some comments below:

> --- /dev/null
> +++ b/drivers/wiegand/Kconfig
> @@ -0,0 +1,20 @@
> +config WIEGAND
> +        tristate "Wiegand Bus driver"
> +        help
> +	  The "Wiegand Interface" is an asynchronous low-level protocol

Mix of tabs and spaces, please use tabs for the other lines as well.

> --- /dev/null
> +++ b/drivers/wiegand/wiegand.c
> @@ -0,0 +1,543 @@
> +// SPDX-License-Identifier: GPL-2.0-only

No copyright line?  It's not required, but usually a nice idea.

> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/slab.h>
> +#include <linux/wiegand.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +#include <linux/property.h>
> +
> +static struct bus_type wiegand_bus_type;
> +static DEFINE_IDR(wiegand_controller_idr);
> +
> +static void devm_wiegand_release_controller(struct device *dev, void *ctlr)
> +{
> +	wiegand_controller_put(*(struct wiegand_controller **)ctlr);
> +}
> +
> +static void wiegand_controller_release(struct device *dev)
> +{
> +	struct wiegand_controller *ctlr;
> +
> +	ctlr = container_of(dev, struct wiegand_controller, dev);
> +	kfree(ctlr);
> +}
> +
> +static struct class wiegand_controller_class = {
> +	.name = "wiegand_master",
> +	.owner = THIS_MODULE,
> +	.dev_release = wiegand_controller_release,
> +};

You have a class and a bus.  Why?  What is the class for?  What's the
difference between devices on the bus and devices in the class?  Usually
it is either one or the other, not both.


> +
> +static DEFINE_MUTEX(board_lock);
> +
> +struct wiegand_controller *__wiegand_alloc_controller(struct device *dev,
> +						unsigned int size, bool slave)
> +{
> +	struct wiegand_controller *ctlr;
> +	size_t ctlr_size = ALIGN(sizeof(*ctlr), dma_get_cache_alignment());
> +
> +	if (!dev)
> +		return NULL;
> +
> +	ctlr = kzalloc(size + ctlr_size, GFP_KERNEL);
> +	if (!ctlr)
> +		return NULL;
> +
> +	device_initialize(&ctlr->dev);
> +	ctlr->bus_num = -1;
> +	ctlr->slave = slave;
> +	ctlr->dev.class = &wiegand_controller_class;
> +	ctlr->dev.parent = dev;
> +	wiegand_controller_set_devdata(ctlr, (void *)ctlr + ctlr_size);
> +
> +	return ctlr;
> +}
> +EXPORT_SYMBOL_GPL(__wiegand_alloc_controller);

Why are you exporting functions with "__" as a prefix?  You do that in a
few places here, that's not normal.

> +/*
> + * Controllers that do not have a devicetree entry need to initialize the
> + * following struct wiegand_controller attributes: pulse_len, interval_len and
> + * frame_gap.
> + */
> +int wiegand_register_controller(struct wiegand_controller *ctlr)

Use real kerneldoc comment style for your public functions?

> +{
> +	struct device *dev = ctlr->dev.parent;
> +	int status, id, first_dynamic;
> +
> +	if (!dev)
> +		return -ENODEV;
> +
> +	status = wiegand_controller_check_ops(ctlr);
> +	if (status)
> +		return status;
> +
> +	if (ctlr->dev.of_node) {
> +		id = of_alias_get_id(ctlr->dev.of_node, "wiegand");
> +		if (id > 0) {
> +			ctlr->bus_num = id;
> +			mutex_lock(&board_lock);
> +			id = idr_alloc(&wiegand_controller_idr, ctlr,
> +							ctlr->bus_num,
> +							ctlr->bus_num + 1,
> +							GFP_KERNEL);
> +			mutex_unlock(&board_lock);
> +			if (WARN(id < 0, "couldn't get idr"))
> +				return id == -ENOSPC ? -EBUSY : id;
> +		}
> +		device_property_read_u32(&ctlr->dev, "pulse-len-us",
> +							&ctlr->pulse_len);

You have an odd "continued line" style throughouut the .c and .h files.
It doesn't need to look like this, you have 100 columns to play with,
this can all be on one line.

> +		device_property_read_u32(&ctlr->dev, "interval-len-us",
> +							&ctlr->interval_len);
> +		device_property_read_u32(&ctlr->dev, "frame-gap-us",
> +							&ctlr->frame_gap);
> +	}
> +	if (ctlr->bus_num < 0) {
> +		first_dynamic = of_alias_get_highest_id("wiegand");
> +		if (first_dynamic < 0)
> +			first_dynamic = 0;
> +		else
> +			first_dynamic++;
> +
> +		mutex_lock(&board_lock);
> +		id = idr_alloc(&wiegand_controller_idr, ctlr, first_dynamic,
> +								0, GFP_KERNEL);

But when you can't put it on one line, indendent the next line to line
up with the "(" character.  So these 2 lines should be:

		id = idr_alloc(&wiegand_controller_idr, ctlr, first_dynamic,
			       0, GFP_KERNEL);

But again, you have 100 columns, this all could have been on oneline.

> +		mutex_unlock(&board_lock);
> +		if (WARN(id < 0, "couldn't get idr\n"))
> +			return id;
> +		ctlr->bus_num = id;
> +	}
> +
> +	if (ctlr->pulse_len == 0)
> +		dev_warn(&ctlr->dev, "pulse_len is not initialized\n");
> +	if (ctlr->interval_len == 0)
> +		dev_warn(&ctlr->dev, "interval_len is not initialized\n");
> +	if (ctlr->frame_gap == 0)
> +		dev_warn(&ctlr->dev, "frame_gap is not initialized\n");

You warn, but then do nothing about it?  What are these messages for?

> --- /dev/null
> +++ b/include/linux/wiegand.h
> @@ -0,0 +1,177 @@
> +/* 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/mutex.h>
> +#include <linux/mod_devicetable.h>
> +
> +#define WIEGAND_NAME_SIZE 32
> +
> +extern struct bus_type wiegand_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];

Why is the modalias in the device structure?  That should be able to be
computed by the id or something like that, not a fixed string in here.


> +};
> +
> +/**
> + * struct wiegand_controller - Wiegand master or slave interface
> + * @dev - Device interface of the controller
> + * @list - Link with the global wiegand_controller list
> + * @bus_num - Board-specific identifier for Wiegand controller
> + * @pulse_len: length of the low pulse in usec; defaults to 50us
> + * @interval_len: length of a whole bit (both the pulse and the high phase) in
> + *	usec; defaults to 2000us
> + * @frame_gap: length of the last bit of a frame (both the pulse and the high
> + *	phase) in usec; defaults to interval_len
> + * device_count - Counter of devices connected to the same Wiegand
> + *	bus(controller).
> + * devm_allocated - Whether the allocation of this struct is devres-managed
> + * slave - Whether the controller is a slave(receives data).
> + * transfer_message - Send a message on the bus.
> + * setup - Setup a device.
> + * cleanup - Cleanup after a device.
> + */
> +struct wiegand_controller {
> +	struct device dev;
> +	struct list_head list;

Why do you need a separate list for this, why not use the list in the
device structure that you already have?


> +
> +	s16 bus_num;
> +
> +	u32 pulse_len;
> +	u32 interval_len;
> +	u32 frame_gap;
> +
> +	u32 payload_len;
> +
> +	u8 device_count;
> +
> +	bool devm_allocated;
> +	bool slave;
> +
> +	int (*transfer_message)(struct wiegand_device *dev, u8 *message,
> +								u8 bitlen);

One line please.

> +
> +	int (*setup)(struct wiegand_device *wiegand);
> +	void (*cleanup)(struct wiegand_device *wiegand);

Try using the tool 'pahole' and see how much wasted space is in this
structure and then move things around a bit.  No need for holes in the
structure for no good reason.

> +};
> +
> +struct wiegand_driver {
> +	const struct wiegand_device_id *id_table;
> +	int (*probe)(struct wiegand_device *wiegand);
> +	void (*remove)(struct wiegand_device *wiegand);
> +	struct device_driver driver;
> +};
> +
> +/* Wiegand controller section */
> +
> +#define wiegand_master wiegand_controller
> +extern struct wiegand_controller *__wiegand_alloc_controller(
> +							struct device *host,
> +							unsigned int size,
> +							bool slave);

No need for "extern".

> +
> +struct wiegand_controller *__devm_wiegand_alloc_controller(struct device *dev,
> +						   unsigned int size,
> +						   bool slave);
> +struct wiegand_controller *__wiegand_alloc_controller(struct device *dev,
> +							unsigned int size,
> +							bool slave);
> +static inline struct wiegand_controller *devm_wiegand_alloc_master(
> +							struct device *dev,
> +							unsigned int size)
> +{
> +	return __devm_wiegand_alloc_controller(dev, size, false);
> +}
> +extern int wiegand_register_controller(struct wiegand_controller *ctlr);
> +extern int devm_wiegand_register_controller(struct device *dev,
> +					struct wiegand_controller *ctlr);
> +#define wiegand_register_master(_ctlr) wiegand_register_controller(_ctlr)
> +#define devm_wiegand_register_master(_dev, _ctlr) \
> +	devm_wiegand_register_controller(_dev, _ctlr)
> +extern void wiegand_unregister_controller(struct wiegand_controller *ctlr);
> +#define wiegand_unregister_master(_ctlr) wiegand_unregister_controller(_ctlr)
> +extern struct wiegand_master *wiegand_busnum_to_master(u16 bus_num);
> +
> +static inline void *wiegand_controller_get_devdata(
> +						struct wiegand_controller *ctlr)

Odd alignment, all on one line please.

thanks,

greg k-h

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

[Zhou Furong]

>> +
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/wiegand.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/dmaengine.h>
>> +#include <linux/property.h>
>> +

please order headers

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

[Greg KH]

On Mon, Feb 06, 2023 at 05:49:44PM +0800, Zhou Furong wrote:
> 
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/wiegand.h>
> > > +#include <linux/dma-mapping.h>
> > > +#include <linux/dmaengine.h>
> > > +#include <linux/property.h>
> > > +
> 
> please order headers

Why?  What order?  For what gain?

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

[Zhou Furong]

On 2023/2/6 18:26, Greg KH wrote:
> On Mon, Feb 06, 2023 at 05:49:44PM +0800, Zhou Furong wrote:
>>
>>>> +
>>>> +#include <linux/device.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/wiegand.h>
>>>> +#include <linux/dma-mapping.h>
>>>> +#include <linux/dmaengine.h>
>>>> +#include <linux/property.h>
>>>> +
>>
>> please order headers
> 
> Why?  What order?  For what gain >

If all header file ordered in alphabet, it will be easy to find if a 
header file has been included or not when header file list is long.

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

[Greg KH]

On Tue, Feb 07, 2023 at 08:36:47AM +0800, Zhou Furong wrote:
> 
> 
> On 2023/2/6 18:26, Greg KH wrote:
> > On Mon, Feb 06, 2023 at 05:49:44PM +0800, Zhou Furong wrote:
> > > 
> > > > > +
> > > > > +#include <linux/device.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/of.h>
> > > > > +#include <linux/of_device.h>
> > > > > +#include <linux/slab.h>
> > > > > +#include <linux/wiegand.h>
> > > > > +#include <linux/dma-mapping.h>
> > > > > +#include <linux/dmaengine.h>
> > > > > +#include <linux/property.h>
> > > > > +
> > > 
> > > please order headers
> > 
> > Why?  What order?  For what gain >
> 
> If all header file ordered in alphabet, it will be easy to find if a header
> file has been included or not when header file list is long.

That's what search in your editor is for :)

This is not a real problem with this code, sorry.

thanks,

greg k-h

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

[Krzysztof Kozlowski]

On 07/02/2023 07:08, Greg KH wrote:
> On Tue, Feb 07, 2023 at 08:36:47AM +0800, Zhou Furong wrote:
>>
>>
>> On 2023/2/6 18:26, Greg KH wrote:
>>> On Mon, Feb 06, 2023 at 05:49:44PM +0800, Zhou Furong wrote:
>>>>
>>>>>> +
>>>>>> +#include <linux/device.h>
>>>>>> +#include <linux/module.h>
>>>>>> +#include <linux/of.h>
>>>>>> +#include <linux/of_device.h>
>>>>>> +#include <linux/slab.h>
>>>>>> +#include <linux/wiegand.h>
>>>>>> +#include <linux/dma-mapping.h>
>>>>>> +#include <linux/dmaengine.h>
>>>>>> +#include <linux/property.h>
>>>>>> +
>>>>
>>>> please order headers
>>>
>>> Why?  What order?  For what gain >
>>
>> If all header file ordered in alphabet, it will be easy to find if a header
>> file has been included or not when header file list is long.
> 
> That's what search in your editor is for :)
> 
> This is not a real problem with this code, sorry.

I would say the only argument is reducing conflicts for simultaneous
edits, mostly when adding new headers. If everyone add at the end, you
get conflicts which could not happen if entries were ordered.

Another thing is that actual order allows easier to spot duplicates or
unneeded headers by looking. At least to me it's easier to read.

Best regards,
Krzysztof

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

[kernel test robot]

Hi Martin,

I love your patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.2-rc6 next-20230203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Martin-Za-ovi/dt-bindings-wiegand-add-Wiegand-controller-common-properties/20230202-223510
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230202143305.21789-3-m.zatovic1%40gmail.com
patch subject: [PATCHv2 2/4] wiegand: add Wiegand bus driver
config: ia64-randconfig-s042-20230204 (https://download.01.org/0day-ci/archive/20230204/202302040653.thgxkOi8-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/5dc4c223e5bb967973f6fbcbea5d45ee1f95db97
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Martin-Za-ovi/dt-bindings-wiegand-add-Wiegand-controller-common-properties/20230202-223510
        git checkout 5dc4c223e5bb967973f6fbcbea5d45ee1f95db97
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=ia64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/wiegand/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/wiegand/wiegand.c: In function 'of_register_wiegand_device':
>> drivers/wiegand/wiegand.c:106:14: error: implicit declaration of function 'of_modalias_node'; did you mean 'of_match_node'? [-Werror=implicit-function-declaration]
     106 |         rc = of_modalias_node(nc, wiegand->modalias, sizeof(wiegand->modalias));
         |              ^~~~~~~~~~~~~~~~
         |              of_match_node
   cc1: some warnings being treated as errors


vim +106 drivers/wiegand/wiegand.c

    91	
    92	static struct wiegand_device *of_register_wiegand_device(
    93							struct wiegand_controller *ctlr,
    94							struct device_node *nc)
    95	{
    96		struct wiegand_device *wiegand;
    97		int rc;
    98	
    99		wiegand = wiegand_alloc_device(ctlr);
   100		if (!wiegand) {
   101			dev_err(&ctlr->dev, "wiegad_device alloc error for %pOF\n", nc);
   102			rc = -ENOMEM;
   103			goto err_out;
   104		}
   105	
 > 106		rc = of_modalias_node(nc, wiegand->modalias, sizeof(wiegand->modalias));
   107		if (rc < 0) {
   108			dev_err(&ctlr->dev, "cannot find modalias for %pOF\n", nc);
   109			goto err_out;
   110		}
   111	
   112		of_node_get(nc);
   113		wiegand->dev.of_node = nc;
   114		wiegand->dev.fwnode = of_fwnode_handle(nc);
   115	
   116		rc = wiegand_add_device(wiegand);
   117		if (rc) {
   118			dev_err(&ctlr->dev, "wiegand_device register error %pOF\n", nc);
   119			goto err_of_node_put;
   120		}
   121	
   122		/* check if more devices are connected to the bus */
   123		if (ctlr->device_count > 1)
   124			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");
   125	
   126		return wiegand;
   127	
   128	err_of_node_put:
   129		of_node_put(nc);
   130	err_out:
   131		wiegand_dev_put(wiegand);
   132		return ERR_PTR(rc);
   133	}
   134	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

[PATCHv2 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand documentation

[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 - D0(low data line) and D1(high data line).

Signed-off-by: Martin Zaťovič <m.zatovic1@gmail.com>
---
 .../bindings/wiegand/wiegand-gpio.yaml        | 51 +++++++++++++++++++
 MAINTAINERS                                   |  5 ++
 2 files changed, 56 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..3af0b7e04b3a
--- /dev/null
+++ b/Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml
@@ -0,0 +1,51 @@
+# 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 devicetree bindings
+
+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-hi-gpios:
+    description: GPIO used as Wiegands data-hi line.
+    maxItems: 1
+
+  data-lo-gpios:
+    description: GPIO used as Wiegands data-lo line.
+    maxItems: 1
+
+required:
+  - compatible
+  - data-hi-gpios
+  - data-lo-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-lo-gpios = <&gpio2 6 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
+        data-hi-gpios = <&gpio2 7 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
+
+        /* devices */
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 8119d12dac41..54d61d95a1ba 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22435,6 +22435,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.39.1

Re: [PATCHv2 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand documentation

[Krzysztof Kozlowski]

On 02/02/2023 15:33, 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 - D0(low data line) and D1(high data line).

Subject: drop second/last, redundant "bindings". The "documentation"
prefix is already stating that these are bindings.

You already got almost same comment with your v1.

> 
> Signed-off-by: Martin Zaťovič <m.zatovic1@gmail.com>
> ---
>  .../bindings/wiegand/wiegand-gpio.yaml        | 51 +++++++++++++++++++
>  MAINTAINERS                                   |  5 ++
>  2 files changed, 56 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..3af0b7e04b3a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml
> @@ -0,0 +1,51 @@
> +# 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 devicetree bindings

Drop "devicetree bindings"

You already got almost same comment with your v1.

> +
> +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#"

Drop quotes

> +
> +properties:
> +  compatible:
> +    const: wiegand-gpio
> +
> +  data-hi-gpios:
> +    description: GPIO used as Wiegands data-hi line.
> +    maxItems: 1
> +
> +  data-lo-gpios:
> +    description: GPIO used as Wiegands data-lo line.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - data-hi-gpios
> +  - data-lo-gpios
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    wiegand@f00 {

Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

Best regards,
Krzysztof

[PATCHv2 4/4] wiegand: add Wiegand GPIO bit-banged controller driver

[Martin Zaťovič]

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

Several attributes need to be defined in the devicetree in order
for this driver to work, namely the data-hi-gpios, data-lo-gpios,
pulse-len, frame-gap and interval-len. These attributes are
documented in the devicetree bindings documentation files.

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.

Signed-off-by: Martin Zaťovič <m.zatovic1@gmail.com>
---
 .../ABI/testing/sysfs-driver-wiegand-gpio     |   9 +
 MAINTAINERS                                   |   2 +
 drivers/wiegand/Kconfig                       |   8 +
 drivers/wiegand/Makefile                      |   1 +
 drivers/wiegand/wiegand-gpio.c                | 324 ++++++++++++++++++
 5 files changed, 344 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..be2246880a83
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-wiegand-gpio
@@ -0,0 +1,9 @@
+What:		/sys/devices/platform/.../wiegand-gpio-attributes/payload_len
+Date:		January 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 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 54d61d95a1ba..1c6f242aa250 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22438,7 +22438,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 fc99575bc3cc..9a8f705d4646 100644
--- a/drivers/wiegand/Kconfig
+++ b/drivers/wiegand/Kconfig
@@ -18,3 +18,11 @@ config WIEGAND
 	  are initially pulled up. When a bit of value 0 is being transmitted,
 	  the D0 line is pulled down. Similarly, when a bit of value 1 is being
 	  transmitted, the D1 line is pulled down.
+
+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 entry.
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..fc4e7e0e988a
--- /dev/null
+++ b/drivers/wiegand/wiegand-gpio.c
@@ -0,0 +1,324 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/poll.h>
+#include <linux/miscdevice.h>
+#include <linux/of.h>
+#include <linux/gpio/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/wiegand.h>
+
+#define WIEGAND_MAX_PAYLEN_BYTES 256
+
+struct wiegand_gpio {
+	struct device *dev;
+	struct wiegand_controller *ctlr;
+	struct miscdevice misc_dev;
+	struct mutex mutex;
+	struct gpio_desc *gpio_data_hi;
+	struct gpio_desc *gpio_data_lo;
+	struct file_operations fops;
+	u8 data[WIEGAND_MAX_PAYLEN_BYTES];
+};
+
+struct wiegand_gpio_instance {
+	struct wiegand_gpio *dev;
+	unsigned long flags;
+};
+
+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 (max != 0 && new > max)
+		return -EINVAL;
+
+	*val = new;
+	return size;
+}
+
+/*
+ * Attribute file for setting payload length of Wiegand messages.
+ */
+ssize_t payload_len_show(struct device *dev, struct device_attribute *attr,
+								 char *buf)
+{
+	struct wiegand_gpio *wiegand_gpio = (struct wiegand_gpio *)
+							dev->driver_data;
+	struct wiegand_controller *ctlr = wiegand_gpio->ctlr;
+
+	return sysfs_emit(buf, "%u\n", ctlr->payload_len);
+}
+
+ssize_t payload_len_store(struct device *dev, struct device_attribute *attr,
+						const char *buf, size_t count)
+{
+	struct wiegand_gpio *wiegand_gpio = (struct wiegand_gpio *)
+							dev->driver_data;
+	struct wiegand_controller *ctlr = wiegand_gpio->ctlr;
+
+	return store_ulong(&(ctlr->payload_len), buf, count,
+						WIEGAND_MAX_PAYLEN_BYTES * 8);
+}
+DEVICE_ATTR_RW(payload_len);
+
+/*
+ * 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 pulse_len = wiegand_gpio->ctlr->pulse_len;
+	u32 interval_len = wiegand_gpio->ctlr->interval_len;
+	u32 frame_gap = wiegand_gpio->ctlr->frame_gap;
+	struct gpio_desc *gpio = value ? wiegand_gpio->gpio_data_hi
+					: wiegand_gpio->gpio_data_lo;
+
+	gpiod_set_value_cansleep(gpio, 0);
+	udelay(pulse_len);
+	gpiod_set_value_cansleep(gpio, 1);
+
+	if (last)
+		udelay(frame_gap - pulse_len);
+	else
+		udelay(interval_len - pulse_len);
+}
+
+/* This function is used for writing from file in dev directory */
+static int wiegand_gpio_write_by_bits(struct wiegand_gpio *wiegand_gpio,
+								u16 bitlen)
+{
+	size_t i;
+	bool bit_value, is_last_bit;
+
+	for (i = 0; i < bitlen; i++) {
+		bit_value = ((wiegand_gpio->data[i / 8] >> (7 - (i % 8)))
+									& 0x01);
+		is_last_bit = (i + 1) == bitlen;
+		wiegand_gpio_send_bit(wiegand_gpio, bit_value, is_last_bit);
+	}
+
+	return 0;
+}
+
+static ssize_t wiegand_gpio_get_user_data(struct wiegand_gpio *wiegand_gpio,
+					char __user const *buf, size_t len)
+{
+	size_t rc;
+
+	if (len > WIEGAND_MAX_PAYLEN_BYTES)
+		return -EBADMSG;
+
+	rc = copy_from_user(&wiegand_gpio->data[0], buf,
+						WIEGAND_MAX_PAYLEN_BYTES);
+	if (rc < 0)
+		return rc;
+
+	return len;
+}
+
+static int wiegand_gpio_frelease(struct inode *ino, struct file *filp)
+{
+	struct wiegand_gpio_instance *info = filp->private_data;
+	struct wiegand_gpio *wiegand_gpio = info->dev;
+
+	mutex_lock(&wiegand_gpio->mutex);
+	info->flags = 0;
+	mutex_unlock(&wiegand_gpio->mutex);
+
+	kfree(info);
+
+	return 0;
+}
+
+static ssize_t wiegand_gpio_fwrite(struct file *filp, char __user const *buf,
+						size_t len, loff_t *offset)
+{
+	struct wiegand_gpio_instance *info = filp->private_data;
+	struct wiegand_gpio *wiegand_gpio = info->dev;
+	u32 msg_length = wiegand_gpio->ctlr->payload_len;
+	int rc;
+
+	if (buf == NULL || len == 0 || len * 8 < msg_length)
+		return -EINVAL;
+
+	rc = wiegand_gpio_get_user_data(wiegand_gpio, buf, len);
+	if (rc < 0)
+		return rc;
+
+	wiegand_gpio_write_by_bits(wiegand_gpio, msg_length);
+
+	return len;
+}
+
+static int wiegand_gpio_fopen(struct inode *ino, struct file *filp)
+{
+	int rc;
+	struct wiegand_gpio_instance *info;
+	struct wiegand_gpio *wiegand_gpio = container_of(filp->f_op,
+							 struct wiegand_gpio,
+							 fops);
+	mutex_lock(&wiegand_gpio->mutex);
+	if ((filp->f_flags & O_ACCMODE) == O_RDONLY ||
+				(filp->f_flags & O_ACCMODE) == O_RDWR) {
+		dev_err(wiegand_gpio->dev, "Device is write only\n");
+		rc = -EIO;
+		goto err;
+	}
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info) {
+		rc = -ENOMEM;
+		goto err;
+	}
+
+	info->dev = wiegand_gpio;
+	info->flags = filp->f_flags;
+	mutex_unlock(&wiegand_gpio->mutex);
+
+	filp->private_data = info;
+
+	return 0;
+err:
+	mutex_unlock(&wiegand_gpio->mutex);
+	return rc;
+}
+
+/* This function is used by device drivers */
+int wiegand_gpio_transfer_message(struct wiegand_device *dev, u8 *message,
+								u8 msg_bitlen)
+{
+	struct wiegand_controller *ctlr = dev->controller;
+	struct wiegand_gpio *wiegand_gpio = wiegand_master_get_devdata(ctlr);
+	u8 msg_bytelength = (msg_bitlen % 8) ?
+				(msg_bitlen / 8) + 1 : (msg_bitlen / 8);
+
+	memcpy(wiegand_gpio->data, message, msg_bytelength);
+	wiegand_gpio_write_by_bits(wiegand_gpio, msg_bitlen);
+
+	return 0;
+}
+
+static int wiegand_gpio_request(struct device *dev,
+					struct wiegand_gpio *wiegand_gpio)
+{
+	/* Get GPIO lines using device tree bindings. */
+	wiegand_gpio->gpio_data_lo = devm_gpiod_get(dev, "data-lo",
+						    GPIOD_OUT_HIGH);
+	if (IS_ERR(wiegand_gpio->gpio_data_lo))
+		return PTR_ERR(wiegand_gpio->gpio_data_lo);
+
+	wiegand_gpio->gpio_data_hi = devm_gpiod_get(dev, "data-hi",
+						    GPIOD_OUT_HIGH);
+	return PTR_ERR_OR_ZERO(wiegand_gpio->gpio_data_hi);
+}
+
+static int wiegand_gpio_probe(struct platform_device *device)
+{
+	int status;
+	struct wiegand_controller *master;
+	struct wiegand_gpio *wiegand_gpio;
+	struct device *dev = &device->dev;
+
+	master = devm_wiegand_alloc_master(dev, sizeof(*wiegand_gpio));
+	if (!master)
+		return -ENOMEM;
+
+	if (dev->of_node)
+		master->dev.of_node = device->dev.of_node;
+
+	if (status)
+		return status;
+
+	master->transfer_message = &wiegand_gpio_transfer_message;
+	master->payload_len = 26; /* set standard 26-bit format */
+
+	wiegand_gpio = wiegand_master_get_devdata(master);
+	wiegand_gpio->ctlr = master;
+	wiegand_gpio->fops.owner = THIS_MODULE;
+	wiegand_gpio->fops.open = wiegand_gpio_fopen;
+	wiegand_gpio->fops.release = wiegand_gpio_frelease;
+	wiegand_gpio->fops.write = wiegand_gpio_fwrite;
+
+	platform_set_drvdata(device, wiegand_gpio);
+
+	master->bus_num = device->id;
+	wiegand_gpio->dev = dev;
+
+	mutex_init(&wiegand_gpio->mutex);
+
+	status = wiegand_gpio_request(dev, wiegand_gpio);
+	if (status) {
+		dev_err(wiegand_gpio->dev, "failed at requesting GPIOs\n");
+		return status;
+	}
+
+	status = gpiod_direction_output(wiegand_gpio->gpio_data_hi, 1);
+	status |= gpiod_direction_output(wiegand_gpio->gpio_data_lo, 1);
+	if (status) {
+		dev_err(wiegand_gpio->dev, "failed to set GPIOs direction\n");
+		return status;
+	}
+
+	status = devm_wiegand_register_master(dev, master);
+	if (status) {
+		dev_err(wiegand_gpio->dev, "failed to register master\n");
+		return status;
+	}
+
+	wiegand_gpio->misc_dev.name = kasprintf(GFP_KERNEL, "wiegand-gpio%u",
+							master->bus_num);
+	wiegand_gpio->misc_dev.minor = MISC_DYNAMIC_MINOR;
+	wiegand_gpio->misc_dev.fops = &wiegand_gpio->fops;
+
+	status = misc_register(&wiegand_gpio->misc_dev);
+	if (status) {
+		dev_err(wiegand_gpio->dev, "couldn't register misc device\n");
+		return status;
+	}
+	wiegand_gpio->misc_dev.parent = wiegand_gpio->dev;
+
+	device_create_file(dev, &dev_attr_payload_len);
+
+	return status;
+}
+
+static int wiegand_gpio_remove(struct platform_device *device)
+{
+	struct wiegand_gpio *wiegand_gpio = platform_get_drvdata(device);
+
+	misc_deregister(&wiegand_gpio->misc_dev);
+
+	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,
+	},
+	.probe		= wiegand_gpio_probe,
+	.remove		= wiegand_gpio_remove,
+};
+
+module_platform_driver(wiegand_gpio_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Wiegand write-only driver realized by GPIOs");
+MODULE_AUTHOR("Martin Zaťovič <m.zatovic1@gmail.com>");
-- 
2.39.1

Re: [PATCHv2 4/4] wiegand: add Wiegand GPIO bit-banged controller driver

[kernel test robot]

Hi Martin,

I love your patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.2-rc6 next-20230202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Martin-Za-ovi/dt-bindings-wiegand-add-Wiegand-controller-common-properties/20230202-223510
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230202143305.21789-5-m.zatovic1%40gmail.com
patch subject: [PATCHv2 4/4] wiegand: add Wiegand GPIO bit-banged controller driver
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230203/202302030403.qE5eN2x8-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/d2c4bf0953d186dba735c1f95b5c25ff5523872c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Martin-Za-ovi/dt-bindings-wiegand-add-Wiegand-controller-common-properties/20230202-223510
        git checkout d2c4bf0953d186dba735c1f95b5c25ff5523872c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/wiegand/wiegand-gpio.c:51:9: warning: no previous prototype for 'payload_len_show' [-Wmissing-prototypes]
      51 | ssize_t payload_len_show(struct device *dev, struct device_attribute *attr,
         |         ^~~~~~~~~~~~~~~~
>> drivers/wiegand/wiegand-gpio.c:61:9: warning: no previous prototype for 'payload_len_store' [-Wmissing-prototypes]
      61 | ssize_t payload_len_store(struct device *dev, struct device_attribute *attr,
         |         ^~~~~~~~~~~~~~~~~
>> drivers/wiegand/wiegand-gpio.c:79:6: warning: no previous prototype for 'wiegand_gpio_send_bit' [-Wmissing-prototypes]
      79 | void wiegand_gpio_send_bit(struct wiegand_gpio *wiegand_gpio, bool value,
         |      ^~~~~~~~~~~~~~~~~~~~~
>> drivers/wiegand/wiegand-gpio.c:198:5: warning: no previous prototype for 'wiegand_gpio_transfer_message' [-Wmissing-prototypes]
     198 | int wiegand_gpio_transfer_message(struct wiegand_device *dev, u8 *message,
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/payload_len_show +51 drivers/wiegand/wiegand-gpio.c

    47	
    48	/*
    49	 * Attribute file for setting payload length of Wiegand messages.
    50	 */
  > 51	ssize_t payload_len_show(struct device *dev, struct device_attribute *attr,
    52									 char *buf)
    53	{
    54		struct wiegand_gpio *wiegand_gpio = (struct wiegand_gpio *)
    55								dev->driver_data;
    56		struct wiegand_controller *ctlr = wiegand_gpio->ctlr;
    57	
    58		return sysfs_emit(buf, "%u\n", ctlr->payload_len);
    59	}
    60	
  > 61	ssize_t payload_len_store(struct device *dev, struct device_attribute *attr,
    62							const char *buf, size_t count)
    63	{
    64		struct wiegand_gpio *wiegand_gpio = (struct wiegand_gpio *)
    65								dev->driver_data;
    66		struct wiegand_controller *ctlr = wiegand_gpio->ctlr;
    67	
    68		return store_ulong(&(ctlr->payload_len), buf, count,
    69							WIEGAND_MAX_PAYLEN_BYTES * 8);
    70	}
    71	DEVICE_ATTR_RW(payload_len);
    72	
    73	/*
    74	 * To send a bit of value 1 following the wiegand protocol, one must set
    75	 * the wiegand_data_hi to low for the duration of pulse. Similarly to send
    76	 * a bit of value 0, the wiegand_data_lo is set to low for pulse duration.
    77	 * This way the two lines are never low at the same time.
    78	 */
  > 79	void wiegand_gpio_send_bit(struct wiegand_gpio *wiegand_gpio, bool value,
    80									bool last)
    81	{
    82		u32 pulse_len = wiegand_gpio->ctlr->pulse_len;
    83		u32 interval_len = wiegand_gpio->ctlr->interval_len;
    84		u32 frame_gap = wiegand_gpio->ctlr->frame_gap;
    85		struct gpio_desc *gpio = value ? wiegand_gpio->gpio_data_hi
    86						: wiegand_gpio->gpio_data_lo;
    87	
    88		gpiod_set_value_cansleep(gpio, 0);
    89		udelay(pulse_len);
    90		gpiod_set_value_cansleep(gpio, 1);
    91	
    92		if (last)
    93			udelay(frame_gap - pulse_len);
    94		else
    95			udelay(interval_len - pulse_len);
    96	}
    97	
    98	/* This function is used for writing from file in dev directory */
    99	static int wiegand_gpio_write_by_bits(struct wiegand_gpio *wiegand_gpio,
   100									u16 bitlen)
   101	{
   102		size_t i;
   103		bool bit_value, is_last_bit;
   104	
   105		for (i = 0; i < bitlen; i++) {
   106			bit_value = ((wiegand_gpio->data[i / 8] >> (7 - (i % 8)))
   107										& 0x01);
   108			is_last_bit = (i + 1) == bitlen;
   109			wiegand_gpio_send_bit(wiegand_gpio, bit_value, is_last_bit);
   110		}
   111	
   112		return 0;
   113	}
   114	
   115	static ssize_t wiegand_gpio_get_user_data(struct wiegand_gpio *wiegand_gpio,
   116						char __user const *buf, size_t len)
   117	{
   118		size_t rc;
   119	
   120		if (len > WIEGAND_MAX_PAYLEN_BYTES)
   121			return -EBADMSG;
   122	
   123		rc = copy_from_user(&wiegand_gpio->data[0], buf,
   124							WIEGAND_MAX_PAYLEN_BYTES);
   125		if (rc < 0)
   126			return rc;
   127	
   128		return len;
   129	}
   130	
   131	static int wiegand_gpio_frelease(struct inode *ino, struct file *filp)
   132	{
   133		struct wiegand_gpio_instance *info = filp->private_data;
   134		struct wiegand_gpio *wiegand_gpio = info->dev;
   135	
   136		mutex_lock(&wiegand_gpio->mutex);
   137		info->flags = 0;
   138		mutex_unlock(&wiegand_gpio->mutex);
   139	
   140		kfree(info);
   141	
   142		return 0;
   143	}
   144	
   145	static ssize_t wiegand_gpio_fwrite(struct file *filp, char __user const *buf,
   146							size_t len, loff_t *offset)
   147	{
   148		struct wiegand_gpio_instance *info = filp->private_data;
   149		struct wiegand_gpio *wiegand_gpio = info->dev;
   150		u32 msg_length = wiegand_gpio->ctlr->payload_len;
   151		int rc;
   152	
   153		if (buf == NULL || len == 0 || len * 8 < msg_length)
   154			return -EINVAL;
   155	
   156		rc = wiegand_gpio_get_user_data(wiegand_gpio, buf, len);
   157		if (rc < 0)
   158			return rc;
   159	
   160		wiegand_gpio_write_by_bits(wiegand_gpio, msg_length);
   161	
   162		return len;
   163	}
   164	
   165	static int wiegand_gpio_fopen(struct inode *ino, struct file *filp)
   166	{
   167		int rc;
   168		struct wiegand_gpio_instance *info;
   169		struct wiegand_gpio *wiegand_gpio = container_of(filp->f_op,
   170								 struct wiegand_gpio,
   171								 fops);
   172		mutex_lock(&wiegand_gpio->mutex);
   173		if ((filp->f_flags & O_ACCMODE) == O_RDONLY ||
   174					(filp->f_flags & O_ACCMODE) == O_RDWR) {
   175			dev_err(wiegand_gpio->dev, "Device is write only\n");
   176			rc = -EIO;
   177			goto err;
   178		}
   179		info = kzalloc(sizeof(*info), GFP_KERNEL);
   180		if (!info) {
   181			rc = -ENOMEM;
   182			goto err;
   183		}
   184	
   185		info->dev = wiegand_gpio;
   186		info->flags = filp->f_flags;
   187		mutex_unlock(&wiegand_gpio->mutex);
   188	
   189		filp->private_data = info;
   190	
   191		return 0;
   192	err:
   193		mutex_unlock(&wiegand_gpio->mutex);
   194		return rc;
   195	}
   196	
   197	/* This function is used by device drivers */
 > 198	int wiegand_gpio_transfer_message(struct wiegand_device *dev, u8 *message,
   199									u8 msg_bitlen)
   200	{
   201		struct wiegand_controller *ctlr = dev->controller;
   202		struct wiegand_gpio *wiegand_gpio = wiegand_master_get_devdata(ctlr);
   203		u8 msg_bytelength = (msg_bitlen % 8) ?
   204					(msg_bitlen / 8) + 1 : (msg_bitlen / 8);
   205	
   206		memcpy(wiegand_gpio->data, message, msg_bytelength);
   207		wiegand_gpio_write_by_bits(wiegand_gpio, msg_bitlen);
   208	
   209		return 0;
   210	}
   211	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCHv2 4/4] wiegand: add Wiegand GPIO bit-banged controller driver

[Greg KH]

On Thu, Feb 02, 2023 at 03:33:05PM +0100, Martin Zaťovič wrote:
> +/*
> + * Attribute file for setting payload length of Wiegand messages.
> + */
> +ssize_t payload_len_show(struct device *dev, struct device_attribute *attr,
> +								 char *buf)
> +{
> +	struct wiegand_gpio *wiegand_gpio = (struct wiegand_gpio *)
> +							dev->driver_data;

No need to cast.


> +
> +	device_create_file(dev, &dev_attr_payload_len);

No, just add an attribute group pointer to your driver and the driver
core will add/remove the sysfs attributes automatically.  No need to do
it manually at all.

thanks,

greg k-h

Re: [PATCHv2 0/4] Wiegand bus driver and GPIO bit-banged controller

[Linus Walleij]

Hi Martin,

the whole patch set looks very sensible to me, I see there is still some
DT binding things to fix (it is a somewhat moving target, so expected)
but the whole design looks very sound to me, so from a GPIO point
of view (which is a minor thing though):
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij