* [PATCHv4 1/4] dt-bindings: wiegand: add Wiegand controller common properties
2023-05-10 16:22 [PATCHv4 0/4] Wiegand bus driver and GPIO bitbanged Wiegand Martin Zaťovič
@ 2023-05-10 16:22 ` Martin Zaťovič
2023-05-10 16:22 ` [PATCHv4 2/4] wiegand: add Wiegand bus driver Martin Zaťovič
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Martin Zaťovič @ 2023-05-10 16:22 UTC (permalink / raw)
To: linux-kernel
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh, beanhuo,
nipun.gupta, linus.walleij, mwen, bvanassche, arnd, ogabbay,
linux, jacek.lawrynowicz, geert+renesas, benjamin.tissoires,
masahiroy, yangyicong, devicetree, Martin Zaťovič,
Rob Herring
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 7e0b87d5aa2e..55602888f084 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22697,6 +22697,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.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCHv4 2/4] wiegand: add Wiegand bus driver
2023-05-10 16:22 [PATCHv4 0/4] Wiegand bus driver and GPIO bitbanged Wiegand Martin Zaťovič
2023-05-10 16:22 ` [PATCHv4 1/4] dt-bindings: wiegand: add Wiegand controller common properties Martin Zaťovič
@ 2023-05-10 16:22 ` Martin Zaťovič
2023-06-22 13:20 ` Andy Shevchenko
2023-05-10 16:22 ` [PATCHv4 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand controller Martin Zaťovič
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Martin Zaťovič @ 2023-05-10 16:22 UTC (permalink / raw)
To: linux-kernel
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh, beanhuo,
nipun.gupta, linus.walleij, mwen, bvanassche, arnd, ogabbay,
linux, jacek.lawrynowicz, geert+renesas, benjamin.tissoires,
masahiroy, yangyicong, devicetree, 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.
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 | 20 ++
drivers/wiegand/Makefile | 1 +
drivers/wiegand/wiegand.c | 609 ++++++++++++++++++++++++++++++++++++++
include/linux/wiegand.h | 144 +++++++++
7 files changed, 779 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 55602888f084..7b7e546572e8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22701,6 +22701,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..f53f0d83195b 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..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..f82091d251d0
--- /dev/null
+++ b/drivers/wiegand/wiegand.c
@@ -0,0 +1,609 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/cdev.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/wiegand.h>
+
+static 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];
+};
+
+DEFINE_IDA(wiegand_controller_ida);
+
+static inline void wiegand_device_put(struct wiegand_device *wiegand);
+static inline struct wiegand_device *to_wiegand_device(struct device *dev);
+
+static int wiegand_fopen(struct inode *ino, struct file *filp);
+static int wiegand_frelease(struct inode *ino, struct file *filp);
+static ssize_t wiegand_fwrite(struct file *filp, char __user const *buf, size_t len,
+ loff_t *offset);
+
+/**
+ * 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, unsigned int size,
+ bool secondary)
+{
+ 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->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_GPL(wiegand_alloc_controller);
+
+static void devm_wiegand_release_controller(struct device *dev, void *ctlr)
+{
+ wiegand_controller_put(*(struct wiegand_controller **)ctlr);
+}
+
+/**
+ * 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, **ptr;
+
+ ptr = devres_alloc(devm_wiegand_release_controller, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return NULL;
+
+ ctlr = wiegand_alloc_controller(dev, size, secondary);
+ 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;
+}
+
+/**
+ * register_wiegand_device - allocates and registers a new Wiegand device
+ * @ctlr: controller structure to attach device to
+ * @nc: devicetree node for the device
+ */
+static struct wiegand_device *register_wiegand_device(struct wiegand_controller *ctlr,
+ struct fwnode_handle *fwnode)
+{
+ struct wiegand_device *wiegand;
+ int ret;
+
+ wiegand = wiegand_alloc_device(ctlr);
+ if (!wiegand) {
+ dev_err(&ctlr->dev, "wiegad_device alloc error for %pOF\n", fwnode);
+ ret = -ENOMEM;
+ goto err_out;
+ }
+
+ fwnode_handle_get(fwnode);
+ wiegand->dev.fwnode = fwnode;
+
+ ret = wiegand_add_device(wiegand);
+ if (ret) {
+ dev_err(&ctlr->dev, "wiegand_device register error %pOF\n", fwnode);
+ goto err_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_node_put:
+ fwnode_handle_put(fwnode);
+err_out:
+ wiegand_device_put(wiegand);
+ return ERR_PTR(ret);
+}
+
+static void register_wiegand_devices(struct wiegand_controller *ctlr)
+{
+ struct wiegand_device *wiegand;
+ struct fwnode_handle *fwnode;
+
+ if (!ctlr->dev.fwnode)
+ return;
+
+ fwnode_for_each_available_child_node(ctlr->dev.fwnode, fwnode) {
+ wiegand = register_wiegand_device(ctlr, fwnode);
+ if (IS_ERR(wiegand))
+ dev_warn(&ctlr->dev, "failed to create wiegand device for %pOF\n", fwnode);
+ }
+}
+
+static void wiegand_controller_parse_property(struct device *dev, const char *prop_name,
+ u32 *cur_val_p, u32 def_val, bool use_def)
+{
+ int ret;
+
+ ret = device_property_read_u32(dev, prop_name, cur_val_p);
+ if ((ret && use_def) || *cur_val_p == 0)
+ *cur_val_p = def_val;
+
+ dev_dbg(dev, "%s: %u\n", prop_name, *cur_val_p);
+}
+
+#define USE_DEFAULT_VAL 1
+
+static void wiegand_controller_parse_properties(struct wiegand_controller *ctlr)
+{
+ wiegand_controller_parse_property(&ctlr->dev, "pulse-len-us", &ctlr->pulse_len,
+ 50, USE_DEFAULT_VAL);
+ wiegand_controller_parse_property(&ctlr->dev, "interval-len-us", &ctlr->interval_len,
+ 2000, USE_DEFAULT_VAL);
+ wiegand_controller_parse_property(&ctlr->dev, "frame-gap-us", &ctlr->frame_gap,
+ 2000, USE_DEFAULT_VAL);
+}
+
+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_GPL(wiegand_unregister_controller);
+
+/**
+ * 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)
+{
+ struct device *dev = ctlr->dev.parent;
+ int status, id;
+
+ if (!dev)
+ return -ENODEV;
+
+ status = wiegand_controller_check_ops(ctlr);
+ if (status)
+ return status;
+
+ id = ida_alloc(&wiegand_controller_ida, GFP_KERNEL);
+ if (WARN(id < 0, "couldn't get an id\n"))
+ return id;
+ ctlr->bus_num = id;
+
+ wiegand_controller_parse_properties(ctlr);
+
+ dev_set_name(&ctlr->dev, "wiegand%u", ctlr->bus_num);
+ ctlr->miscdev.name = kasprintf(GFP_KERNEL, "wiegand1");
+ if (!ctlr->miscdev.name)
+ return -ENOMEM;
+
+ 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");
+ return status;
+ }
+
+ mutex_init(&ctlr->file_lock);
+
+ status = device_add(&ctlr->dev);
+ if (status < 0)
+ goto free_bus_id;
+
+ ctlr->device_count = 0;
+ ctlr->miscdev.parent = &ctlr->dev;
+ register_wiegand_devices(ctlr);
+
+ return status;
+
+free_bus_id:
+ ida_free(&wiegand_controller_ida, ctlr->bus_num);
+ misc_deregister(&ctlr->miscdev);
+ kfree(ctlr->miscdev.name);
+ return status;
+}
+EXPORT_SYMBOL_GPL(wiegand_register_controller);
+
+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_GPL(devm_wiegand_register_controller);
+
+/* Device section */
+
+static inline struct wiegand_device *to_wiegand_device(struct device *dev)
+{
+ return dev ? container_of(dev, struct wiegand_device, dev) : NULL;
+}
+
+struct wiegand_controller *wiegand_device_get_controller(struct wiegand_device *dev)
+{
+ return dev->controller;
+}
+EXPORT_SYMBOL_GPL(wiegand_device_get_controller);
+
+static inline void wiegand_device_put(struct wiegand_device *wiegand)
+{
+ if (wiegand)
+ put_device(&wiegand->dev);
+
+ if (wiegand->controller->cleanup)
+ wiegand->controller->cleanup(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_GPL(wiegand_alloc_device);
+
+static void wiegand_cleanup(struct wiegand_device *wiegand)
+{
+ if (wiegand->controller->cleanup)
+ wiegand->controller->cleanup(wiegand);
+}
+
+static int wiegand_dev_set_name(struct wiegand_device *wiegand, u8 id)
+{
+ int ret = dev_set_name(&wiegand->dev, "%s.%u", dev_name(&wiegand->controller->dev), id);
+ return ret;
+}
+
+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 = 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.fwnode)
+ fwnode_handle_put(wiegand->dev.fwnode);
+
+ fwnode_remove_software_node(wiegand->dev.fwnode);
+ 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, 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_GPL(wiegand_send_message);
+
+static ssize_t wiegand_get_user_data(struct wiegand_controller *ctlr, char __user const *buf,
+ size_t len)
+{
+ int i;
+ char data_buffer[WIEGAND_MAX_PAYLEN_BYTES];
+
+ if (len > WIEGAND_MAX_PAYLEN_BYTES)
+ return -EBADMSG;
+
+ if (copy_from_user(&data_buffer[0], buf, WIEGAND_MAX_PAYLEN_BYTES))
+ return -EFAULT;
+
+ for (i = 0; i < len; i++)
+ bitmap_set_value8(ctlr->data_bitmap, data_buffer[i], i * 8);
+
+ 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;
+ u32 msg_length = ctlr->payload_len;
+
+ if (!buf || len == 0 || DIV_ROUND_UP(msg_length, 8) > len)
+ return -EINVAL;
+
+ ret = wiegand_get_user_data(ctlr, buf, len);
+ if (ret < 0)
+ return ret;
+
+ ctlr->transfer_message(ctlr);
+
+ 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 err;
+ }
+
+ mutex_unlock(&ctlr->file_lock);
+
+ return 0;
+err:
+ mutex_unlock(&ctlr->file_lock);
+ mutex_destroy(&ctlr->file_lock);
+ return ret;
+}
+
+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);
+
+ if (wdrv->probe)
+ return wdrv->probe(wiegand);
+
+ return 0;
+}
+
+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;
+
+ 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);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void __exit wiegand_exit(void)
+{
+ ida_destroy(&wiegand_controller_ida);
+ bus_unregister(&wiegand_bus_type);
+}
+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..92d46114a4a8
--- /dev/null
+++ b/include/linux/wiegand.h
@@ -0,0 +1,144 @@
+/* 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/miscdevice.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+
+#define WIEGAND_NAME_SIZE 32
+#define WIEGAND_MAX_PAYLEN_BYTES 256
+
+extern struct bus_type wiegand_type;
+
+struct wiegand_device;
+
+/**
+ * struct wiegand_controller - Wiegand primary or secondary 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
+ * @secondary - Whether the controller is a secondary(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 miscdevice miscdev;
+
+ struct file_operations fops;
+ struct mutex file_lock;
+
+ unsigned int bus_num;
+
+ bool secondary;
+ bool devm_allocated;
+
+ u32 pulse_len;
+ u32 interval_len;
+ u32 frame_gap;
+ 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);
+};
+
+/* Wiegand controller section */
+
+#define wiegand_primary wiegand_controller
+extern struct wiegand_controller *wiegand_alloc_controller(struct device *host, unsigned int size,
+ bool secondary);
+
+extern 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);
+}
+
+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_primary(_ctlr) wiegand_register_controller(_ctlr)
+#define devm_wiegand_register_primary(_dev, _ctlr)devm_wiegand_register_controller(_dev, _ctlr)
+extern void wiegand_unregister_controller(void *ctlr);
+#define wiegand_unregister_primary(_ctlr) wiegand_unregister_controller(_ctlr)
+extern 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);
+}
+
+/* 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 struct wiegand_controller *wiegand_device_get_controller(struct wiegand_device *dev);
+
+extern int wiegand_send_message(struct wiegand_device *wiegand, unsigned long *msg_bmp, u8 bitlen);
+
+/* Wiegand driver section */
+
+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);
+}
+
+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.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCHv4 2/4] wiegand: add Wiegand bus driver
2023-05-10 16:22 ` [PATCHv4 2/4] wiegand: add Wiegand bus driver Martin Zaťovič
@ 2023-06-22 13:20 ` Andy Shevchenko
0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2023-06-22 13:20 UTC (permalink / raw)
To: Martin Zaťovič
Cc: linux-kernel, robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh,
beanhuo, nipun.gupta, linus.walleij, mwen, bvanassche, arnd,
ogabbay, linux, jacek.lawrynowicz, geert+renesas,
benjamin.tissoires, masahiroy, yangyicong, devicetree
On Wed, May 10, 2023 at 06:22:41PM +0200, Martin Zaťovič wrote:
This needs much more work. See my comments below.
> 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.
Any chance to have a Datasheet: (or Link:) tag to point to the protocol
specifications?
...
> +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.
&You have an indentation issues in all lines above, except the first one.
Should be
config ...
<TAB>tristate
<TAB>help
<TAB><sp><sp>help text
<TAB> — tabulation
<sp> — space
...
> +#include <linux/cdev.h>
+ 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>
+ types.h
> +#include <linux/wiegand.h>
...
> +/**
> + * 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];
> +};
> +DEFINE_IDA(wiegand_controller_ida);
Why not static?
> +static inline void wiegand_device_put(struct wiegand_device *wiegand);
> +static inline struct wiegand_device *to_wiegand_device(struct device *dev);
> +
> +static int wiegand_fopen(struct inode *ino, struct file *filp);
> +static int wiegand_frelease(struct inode *ino, struct file *filp);
> +static ssize_t wiegand_fwrite(struct file *filp, char __user const *buf, size_t len,
> + loff_t *offset);
Why do you need forward declarations?
...
> +struct wiegand_controller *wiegand_alloc_controller(struct device *dev, unsigned int size,
> + bool secondary)
> +{
> + struct wiegand_controller *ctlr;
> + size_t ctlr_size = ALIGN(sizeof(*ctlr), dma_get_cache_alignment());
> + if (!dev)
> + return NULL;
Hmm... Why this check is needed?
> + ctlr = kzalloc(size + ctlr_size, GFP_KERNEL);
Perhaps you need to use one of the macros from overflow.h.
> + 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_GPL(wiegand_alloc_controller);
Can we have it namespaced from day 1, please?
...
> +struct wiegand_controller *devm_wiegand_alloc_controller(struct device *dev, unsigned int size,
> + bool secondary)
> +{
> + struct wiegand_controller *ctlr, **ptr;
> +
> + ptr = devres_alloc(devm_wiegand_release_controller, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return NULL;
> +
> + ctlr = wiegand_alloc_controller(dev, size, secondary);
> + if (ctlr) {
> + ctlr->devm_allocated = true;
> + *ptr = ctlr;
> + devres_add(dev, ptr);
> + } else {
> + devres_free(ptr);
> + }
Please, use devm_add_action_or_reset().
> + return ctlr;
> +}
> +/**
> + * register_wiegand_device - allocates and registers a new Wiegand device
> + * @ctlr: controller structure to attach device to
> + * @nc: devicetree node for the device
Have you run kernel doc validator?
And it's not device tree only anymore, use word "firmware" instead.
> + */
...
> + wiegand = wiegand_alloc_device(ctlr);
> + if (!wiegand) {
> + dev_err(&ctlr->dev, "wiegad_device alloc error for %pOF\n", fwnode);
We don't print an error messages for -ENOMEM.
> + ret = -ENOMEM;
> + goto err_out;
Are you sure we need to put the device in this case? When it will be not NULL here?
> + }
> + fwnode_handle_get(fwnode);
> + wiegand->dev.fwnode = fwnode;
Not sure why you need a bumped reference, but in any case the problematic
is the second line. Please, avoid dereferencing fwnode in struct device.
Use respective APIs, here is device_set_node().
> + ret = wiegand_add_device(wiegand);
> + if (ret) {
> + dev_err(&ctlr->dev, "wiegand_device register error %pOF\n", fwnode);
> + goto err_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_node_put:
> + fwnode_handle_put(fwnode);
> +err_out:
> + wiegand_device_put(wiegand);
> + return ERR_PTR(ret);
...
> +static void register_wiegand_devices(struct wiegand_controller *ctlr)
> +{
> + struct wiegand_device *wiegand;
> + struct fwnode_handle *fwnode;
> + if (!ctlr->dev.fwnode)
> + return;
This is a dup, which is implied already by the below for_each call.
> + fwnode_for_each_available_child_node(ctlr->dev.fwnode, fwnode) {
> + wiegand = register_wiegand_device(ctlr, fwnode);
> + if (IS_ERR(wiegand))
> + dev_warn(&ctlr->dev, "failed to create wiegand device for %pOF\n", fwnode);
You are using wrong specifier for fwnode. PLease, fix it everywhere.
> + }
> +}
...
> +static void wiegand_controller_parse_property(struct device *dev, const char *prop_name,
> + u32 *cur_val_p, u32 def_val, bool use_def)
> +{
> + int ret;
> +
> + ret = device_property_read_u32(dev, prop_name, cur_val_p);
> + if ((ret && use_def) || *cur_val_p == 0)
> + *cur_val_p = def_val;
> +
> + dev_dbg(dev, "%s: %u\n", prop_name, *cur_val_p);
Why do we need this message? What for?
> +}
> +#define USE_DEFAULT_VAL 1
Redundant. hence redundant second parameter to the above call.
...
> +static void wiegand_controller_parse_properties(struct wiegand_controller *ctlr)
> +{
> + wiegand_controller_parse_property(&ctlr->dev, "pulse-len-us", &ctlr->pulse_len,
> + 50, USE_DEFAULT_VAL);
> + wiegand_controller_parse_property(&ctlr->dev, "interval-len-us", &ctlr->interval_len,
> + 2000, USE_DEFAULT_VAL);
> + wiegand_controller_parse_property(&ctlr->dev, "frame-gap-us", &ctlr->frame_gap,
> + 2000, USE_DEFAULT_VAL);
Default make more sense from group parsing, like I²C core does for timings.
> +}
...
> +int wiegand_register_controller(struct wiegand_controller *ctlr)
> +{
> + struct device *dev = ctlr->dev.parent;
> + int status, id;
> + if (!dev)
> + return -ENODEV;
Why?
> + status = wiegand_controller_check_ops(ctlr);
> + if (status)
> + return status;
> +
> + id = ida_alloc(&wiegand_controller_ida, GFP_KERNEL);
> + if (WARN(id < 0, "couldn't get an id\n"))
Is it really needs to be WARN()?
> + return id;
> + ctlr->bus_num = id;
> + dev_set_name(&ctlr->dev, "wiegand%u", ctlr->bus_num);
No check?
> + ctlr->miscdev.name = kasprintf(GFP_KERNEL, "wiegand1");
> + if (!ctlr->miscdev.name)
> + return -ENOMEM;
...
> +free_bus_id:
out_free_bus_id:
> + ida_free(&wiegand_controller_ida, ctlr->bus_num);
> + misc_deregister(&ctlr->miscdev);
> + kfree(ctlr->miscdev.name);
> + return status;
> +}
...
> +static inline void wiegand_device_put(struct wiegand_device *wiegand)
And where is the corresponding get()?
> +{
> + if (wiegand)
> + put_device(&wiegand->dev);
> + if (wiegand->controller->cleanup)
> + wiegand->controller->cleanup(wiegand);
Dup of wiegand_cleanup().
> +}
...
> +static int wiegand_dev_set_name(struct wiegand_device *wiegand, u8 id)
> +{
> + int ret = dev_set_name(&wiegand->dev, "%s.%u", dev_name(&wiegand->controller->dev), id);
> + return ret;
return dev_set_name(...); ?
> +}
...
> +int wiegand_setup(struct wiegand_device *wiegand)
> +{
> + int status = 0;
Redundant assignment, see below.
> + if (wiegand->controller->setup) {
> + status = wiegand->controller->setup(wiegand);
> + if (status) {
> + dev_err(&wiegand->controller->dev, "failed to setup device: %d\n", status);
In all such cases you may make the code neater with
struct device *ctrl_dev = &wiegand->controller->dev;
Also consider
struct ... *ctrl = wiegand->controller;
struct device *ctrl_dev = &controller->dev;
> + return status;
> + }
> + }
> + return status;
Here it's always 0, right?
return 0;
> +}
...
> +void wiegand_unregister_device(struct wiegand_device *wiegand)
> +{
> + if (!wiegand)
> + return;
> + if (wiegand->dev.fwnode)
Yet another dup conditional, implied by fwnode API.
> + fwnode_handle_put(wiegand->dev.fwnode);
> + fwnode_remove_software_node(wiegand->dev.fwnode);
Where this has come from? Leftover from some copy'n'paste?
> + device_del(&wiegand->dev);
> + wiegand_cleanup(wiegand);
> + put_device(&wiegand->dev);
> +}
...
> +static ssize_t wiegand_get_user_data(struct wiegand_controller *ctlr, char __user const *buf,
> + size_t len)
> +{
> + int i;
> + char data_buffer[WIEGAND_MAX_PAYLEN_BYTES];
> +
> + if (len > WIEGAND_MAX_PAYLEN_BYTES)
> + return -EBADMSG;
> +
> + if (copy_from_user(&data_buffer[0], buf, WIEGAND_MAX_PAYLEN_BYTES))
&data_buffer[0] --> data_buffer
> + return -EFAULT;
> +
> + for (i = 0; i < len; i++)
> + bitmap_set_value8(ctlr->data_bitmap, data_buffer[i], i * 8);
> +
> + return len;
> +}
I'm wondering why you can't use bitmap_parse_user() with the respective format?
Or even bitmap_parselist_user() (if you feel like this would be better, I feel
like not really, but who knows).
...
> +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;
> + u32 msg_length = ctlr->payload_len;
> +
> + if (!buf || len == 0 || DIV_ROUND_UP(msg_length, 8) > len)
BITS_TO_BYTES()
> + return -EINVAL;
> +
> + ret = wiegand_get_user_data(ctlr, buf, len);
> + if (ret < 0)
> + return ret;
> +
> + ctlr->transfer_message(ctlr);
> +
> + 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 err;
> + }
> +
> + mutex_unlock(&ctlr->file_lock);
> +
> + return 0;
> +err:
err_unlock:
> + mutex_unlock(&ctlr->file_lock);
> + mutex_destroy(&ctlr->file_lock);
Huh?!
> + return ret;
> +}
...
> + return (strcmp(wiegand_dev->modalias, drv->name) == 0);
Outer parentheses are redundant.
...
> +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);
> + if (wdrv->probe)
Shouldn't this be a mandatory callback? Or i.o.w. can you elaborate the use
case with probe == NULL?
> + return wdrv->probe(wiegand);
> +
> + return 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);
> + return ret;
> + }
> +
> + return 0;
return ret;
> +}
...
> +postcore_initcall_sync(wiegand_init);
Move it closer to the callback itself.
...
> +#ifndef H_LINUX_INCLUDE_LINUX_WIEGAND_H
> +#define H_LINUX_INCLUDE_LINUX_WIEGAND_H
container_of.h
> +#include <linux/device.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mod_devicetable.h>
Not sure it's in use.
> +#include <linux/mutex.h>
types.h
> +#define WIEGAND_NAME_SIZE 32
> +#define WIEGAND_MAX_PAYLEN_BYTES 256
> +extern struct bus_type wiegand_type;
So, extern or static? Please, be consistent.
...
> + struct file_operations fops;
Missing header for this data type.
...
> +#define wiegand_primary_get_devdata(_ctlr) wiegand_controller_get_devdata(_ctlr)
> +#define wiegand_primary_set_devdata(_ctlr, _data) wiegand_controller_set_devdata(_ctlr, _data)
Why not _data --> data?
...
> +extern void wiegand_unregister_device(struct wiegand_device *wiegand);
> +extern struct wiegand_controller *wiegand_device_get_controller(struct wiegand_device *dev);
> +
> +extern int wiegand_send_message(struct wiegand_device *wiegand, unsigned long *msg_bmp, u8 bitlen);
...and so on...
Drop extern from the function declarations.
...
> +/* Wiegand driver section */
Single space is enough.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCHv4 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand controller
2023-05-10 16:22 [PATCHv4 0/4] Wiegand bus driver and GPIO bitbanged Wiegand Martin Zaťovič
2023-05-10 16:22 ` [PATCHv4 1/4] dt-bindings: wiegand: add Wiegand controller common properties Martin Zaťovič
2023-05-10 16:22 ` [PATCHv4 2/4] wiegand: add Wiegand bus driver Martin Zaťovič
@ 2023-05-10 16:22 ` Martin Zaťovič
2023-05-10 16:33 ` Conor Dooley
2023-05-11 5:30 ` Krzysztof Kozlowski
2023-05-10 16:22 ` [PATCHv4 4/4] wiegand: add Wiegand GPIO bitbanged controller driver Martin Zaťovič
2023-06-22 12:36 ` [PATCHv4 0/4] Wiegand bus driver and GPIO bitbanged Wiegand Andy Shevchenko
4 siblings, 2 replies; 11+ messages in thread
From: Martin Zaťovič @ 2023-05-10 16:22 UTC (permalink / raw)
To: linux-kernel
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh, beanhuo,
nipun.gupta, linus.walleij, mwen, bvanassche, arnd, ogabbay,
linux, jacek.lawrynowicz, geert+renesas, benjamin.tissoires,
masahiroy, yangyicong, devicetree, 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).
Acked-by: Linus Walleij <linus.walleij@linaro.org>
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..df28929f6dae
--- /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 7b7e546572e8..915cb36e5b2f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22704,6 +22704,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.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCHv4 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand controller
2023-05-10 16:22 ` [PATCHv4 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand controller Martin Zaťovič
@ 2023-05-10 16:33 ` Conor Dooley
2023-05-11 5:30 ` Krzysztof Kozlowski
1 sibling, 0 replies; 11+ messages in thread
From: Conor Dooley @ 2023-05-10 16:33 UTC (permalink / raw)
To: Martin Zaťovič
Cc: linux-kernel, robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh,
beanhuo, nipun.gupta, linus.walleij, mwen, bvanassche, arnd,
ogabbay, linux, jacek.lawrynowicz, geert+renesas,
benjamin.tissoires, masahiroy, yangyicong, devicetree
[-- Attachment #1: Type: text/plain, Size: 2584 bytes --]
On Wed, May 10, 2023 at 06:22:42PM +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 - D0(low data line) and D1(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 | 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..df28929f6dae
> --- /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
From the previous versions, drop "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 {
Also as previously pointed out dt_binding_check doesn't like this:
Warning (unit_address_vs_reg): /example-0/wiegand@f00: node has a unit name, but no reg or ranges property
Cheers,
Conor.
> + 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 */
> + };
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCHv4 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand controller
2023-05-10 16:22 ` [PATCHv4 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand controller Martin Zaťovič
2023-05-10 16:33 ` Conor Dooley
@ 2023-05-11 5:30 ` Krzysztof Kozlowski
1 sibling, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2023-05-11 5:30 UTC (permalink / raw)
To: Martin Zaťovič
Cc: arnd, linus.walleij, jacek.lawrynowicz, linux-kernel, nipun.gupta,
conor+dt, geert+renesas, krzysztof.kozlowski+dt, beanhuo,
bvanassche, ogabbay, linux, mwen, robh+dt, yangyicong,
benjamin.tissoires, devicetree, gregkh, masahiroy
On Wed, 10 May 2023 18:22:42 +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 - D0(low data line) and D1(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 | 51 +++++++++++++++++++
> MAINTAINERS | 5 ++
> 2 files changed, 56 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.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/wiegand/wiegand-controller.yaml
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
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/wiegand/wiegand-gpio.example.dtb: wiegand@f00: False schema does not allow {'compatible': ['wiegand-gpio'], 'pulse-len-us': [[50]], 'interval-len-us': [[2000]], 'frame-gap-us': [[2000]], 'data-lo-gpios': [[4294967295, 6, 6]], 'data-hi-gpios': [[4294967295, 7, 6]], '$nodename': ['wiegand@f00']}
From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/wiegand/wiegand-gpio.example.dtb: wiegand@f00: Unevaluated properties are not allowed ('frame-gap-us', 'interval-len-us', 'pulse-len-us' were unexpected)
From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml
See https://patchwork.ozlabs.org/patch/1779628
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCHv4 4/4] wiegand: add Wiegand GPIO bitbanged controller driver
2023-05-10 16:22 [PATCHv4 0/4] Wiegand bus driver and GPIO bitbanged Wiegand Martin Zaťovič
` (2 preceding siblings ...)
2023-05-10 16:22 ` [PATCHv4 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand controller Martin Zaťovič
@ 2023-05-10 16:22 ` Martin Zaťovič
2023-05-10 20:16 ` kernel test robot
2023-06-22 13:39 ` Andy Shevchenko
2023-06-22 12:36 ` [PATCHv4 0/4] Wiegand bus driver and GPIO bitbanged Wiegand Andy Shevchenko
4 siblings, 2 replies; 11+ messages in thread
From: Martin Zaťovič @ 2023-05-10 16:22 UTC (permalink / raw)
To: linux-kernel
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh, beanhuo,
nipun.gupta, linus.walleij, mwen, bvanassche, arnd, ogabbay,
linux, jacek.lawrynowicz, geert+renesas, benjamin.tissoires,
masahiroy, yangyicong, devicetree, 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.
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 | 8 +
drivers/wiegand/Makefile | 1 +
drivers/wiegand/wiegand-gpio.c | 189 ++++++++++++++++++
5 files changed, 209 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..c3981972dbc8
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-wiegand-gpio
@@ -0,0 +1,9 @@
+What: /sys/devices/platform/.../wiegand-gpio-attributes/payload_len
+Date: May 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 915cb36e5b2f..e828a5ec8162 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22707,7 +22707,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..a46421f515b1
--- /dev/null
+++ b/drivers/wiegand/wiegand-gpio.c
@@ -0,0 +1,189 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/poll.h>
+#include <linux/slab.h>
+#include <linux/wiegand.h>
+
+#define UP_TO_100_USEC_DEVIATION 1
+#define MORE_THAN_100_USEC_DEVIATION 3
+
+struct wiegand_gpio {
+ struct device *dev;
+ struct wiegand_controller *ctlr;
+ struct gpio_desc *data1_gpio;
+ struct gpio_desc *data0_gpio;
+};
+
+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 -EINVAL;
+
+ *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);
+
+/*
+ * 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;
+ 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->data1_gpio : wiegand_gpio->data0_gpio;
+
+ 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;
+
+ if (sleep_len < 10)
+ udelay(sleep_len);
+ else if (sleep_len < 100)
+ usleep_range(sleep_len - UP_TO_100_USEC_DEVIATION,
+ sleep_len + UP_TO_100_USEC_DEVIATION);
+ else
+ usleep_range(sleep_len - MORE_THAN_100_USEC_DEVIATION,
+ sleep_len + MORE_THAN_100_USEC_DEVIATION);
+}
+
+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 = test_bit(i, wiegand_gpio->ctlr->data_bitmap);
+ is_last_bit = (i + 1) == bitlen;
+ wiegand_gpio_send_bit(wiegand_gpio, bit_value, is_last_bit);
+ }
+
+ 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->data0_gpio = devm_gpiod_get(dev, "data-lo", GPIOD_OUT_HIGH);
+ if (IS_ERR(wiegand_gpio->data0_gpio))
+ return PTR_ERR(wiegand_gpio->data0_gpio);
+
+ wiegand_gpio->data1_gpio = devm_gpiod_get(dev, "data-hi", GPIOD_OUT_HIGH);
+ return PTR_ERR_OR_ZERO(wiegand_gpio->data1_gpio);
+}
+
+static int wiegand_gpio_probe(struct platform_device *device)
+{
+ int status = 0;
+ struct wiegand_controller *primary;
+ struct wiegand_gpio *wiegand_gpio;
+ struct device *dev = &device->dev;
+
+ primary = wiegand_alloc_controller(dev, sizeof(*wiegand_gpio), false);
+ if (!primary)
+ return -ENOMEM;
+
+ primary->transfer_message = &wiegand_gpio_transfer_message;
+ primary->payload_len = 26; // set standard 26-bit format
+
+ 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(wiegand_gpio->dev, status, "failed at requesting GPIOs\n");
+
+ status = gpiod_direction_output(wiegand_gpio->data1_gpio, 1);
+ if (status)
+ return dev_err_probe(wiegand_gpio->dev, status, "failed to set GPIOs direction\n");
+
+ status = gpiod_direction_output(wiegand_gpio->data0_gpio, 1);
+ if (status)
+ return dev_err_probe(wiegand_gpio->dev, status, "failed to set GPIOs direction\n");
+
+ status = wiegand_register_controller(primary);
+ if (status)
+ dev_err_probe(wiegand_gpio->dev, status, "failed to register primary\n");
+ return status;
+}
+
+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_LICENSE("GPL");
+MODULE_DESCRIPTION("Wiegand write-only driver realized by GPIOs");
+MODULE_AUTHOR("Martin Zaťovič <m.zatovic1@gmail.com>");
--
2.40.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCHv4 4/4] wiegand: add Wiegand GPIO bitbanged controller driver
2023-05-10 16:22 ` [PATCHv4 4/4] wiegand: add Wiegand GPIO bitbanged controller driver Martin Zaťovič
@ 2023-05-10 20:16 ` kernel test robot
2023-06-22 13:39 ` Andy Shevchenko
1 sibling, 0 replies; 11+ messages in thread
From: kernel test robot @ 2023-05-10 20:16 UTC (permalink / raw)
To: Martin Zaťovič, linux-kernel
Cc: oe-kbuild-all, robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh,
beanhuo, nipun.gupta, linus.walleij, mwen, bvanassche, arnd,
ogabbay, linux, jacek.lawrynowicz, geert+renesas,
benjamin.tissoires, masahiroy, yangyicong, devicetree,
Martin Zaťovič
Hi Martin,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.4-rc1 next-20230510]
[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/20230511-002708
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230510162243.95820-5-m.zatovic1%40gmail.com
patch subject: [PATCHv4 4/4] wiegand: add Wiegand GPIO bitbanged controller driver
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230511/202305110450.jjNwIYfp-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/3eb47f0de6aecc78d72c144b36ccd97f22d908c5
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/20230511-002708
git checkout 3eb47f0de6aecc78d72c144b36ccd97f22d908c5
# 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>
| Link: https://lore.kernel.org/oe-kbuild-all/202305110450.jjNwIYfp-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/wiegand/wiegand-gpio.c:70:6: warning: no previous prototype for 'wiegand_gpio_send_bit' [-Wmissing-prototypes]
70 | void wiegand_gpio_send_bit(struct wiegand_gpio *wiegand_gpio, bool value, bool last)
| ^~~~~~~~~~~~~~~~~~~~~
>> drivers/wiegand/wiegand-gpio.c:111:5: warning: no previous prototype for 'wiegand_gpio_transfer_message' [-Wmissing-prototypes]
111 | int wiegand_gpio_transfer_message(struct wiegand_controller *ctlr)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/wiegand_gpio_send_bit +70 drivers/wiegand/wiegand-gpio.c
63
64 /*
65 * To send a bit of value 1 following the wiegand protocol, one must set
66 * the wiegand_data_hi to low for the duration of pulse. Similarly to send
67 * a bit of value 0, the wiegand_data_lo is set to low for pulse duration.
68 * This way the two lines are never low at the same time.
69 */
> 70 void wiegand_gpio_send_bit(struct wiegand_gpio *wiegand_gpio, bool value, bool last)
71 {
72 u32 sleep_len;
73 u32 pulse_len = wiegand_gpio->ctlr->pulse_len;
74 u32 interval_len = wiegand_gpio->ctlr->interval_len;
75 u32 frame_gap = wiegand_gpio->ctlr->frame_gap;
76 struct gpio_desc *gpio = value ? wiegand_gpio->data1_gpio : wiegand_gpio->data0_gpio;
77
78 gpiod_set_value_cansleep(gpio, 0);
79 udelay(pulse_len);
80 gpiod_set_value_cansleep(gpio, 1);
81
82 if (last)
83 sleep_len = frame_gap - pulse_len;
84 else
85 sleep_len = interval_len - pulse_len;
86
87 if (sleep_len < 10)
88 udelay(sleep_len);
89 else if (sleep_len < 100)
90 usleep_range(sleep_len - UP_TO_100_USEC_DEVIATION,
91 sleep_len + UP_TO_100_USEC_DEVIATION);
92 else
93 usleep_range(sleep_len - MORE_THAN_100_USEC_DEVIATION,
94 sleep_len + MORE_THAN_100_USEC_DEVIATION);
95 }
96
97 static int wiegand_gpio_write_by_bits(struct wiegand_gpio *wiegand_gpio, u16 bitlen)
98 {
99 size_t i;
100 bool bit_value, is_last_bit;
101
102 for (i = 0; i < bitlen; i++) {
103 bit_value = test_bit(i, wiegand_gpio->ctlr->data_bitmap);
104 is_last_bit = (i + 1) == bitlen;
105 wiegand_gpio_send_bit(wiegand_gpio, bit_value, is_last_bit);
106 }
107
108 return 0;
109 }
110
> 111 int wiegand_gpio_transfer_message(struct wiegand_controller *ctlr)
112 {
113 struct wiegand_gpio *wiegand_gpio = wiegand_primary_get_devdata(ctlr);
114 u8 msg_bitlen = ctlr->payload_len;
115
116 wiegand_gpio_write_by_bits(wiegand_gpio, msg_bitlen);
117
118 return 0;
119 }
120
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCHv4 4/4] wiegand: add Wiegand GPIO bitbanged controller driver
2023-05-10 16:22 ` [PATCHv4 4/4] wiegand: add Wiegand GPIO bitbanged controller driver Martin Zaťovič
2023-05-10 20:16 ` kernel test robot
@ 2023-06-22 13:39 ` Andy Shevchenko
1 sibling, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2023-06-22 13:39 UTC (permalink / raw)
To: Martin Zaťovič
Cc: linux-kernel, robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh,
beanhuo, nipun.gupta, linus.walleij, mwen, bvanassche, arnd,
ogabbay, linux, jacek.lawrynowicz, geert+renesas,
benjamin.tissoires, masahiroy, yangyicong, devicetree
On Wed, May 10, 2023 at 06:22:43PM +0200, Martin Zaťovič wrote:
> 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.
...
> +Date: May 2023
Taking into account the estimated release date I think this should be changed
to Aug 2023.
...
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/poll.h>
> +#include <linux/slab.h>
> +#include <linux/wiegand.h>
...
> +#define UP_TO_100_USEC_DEVIATION 1
> +#define MORE_THAN_100_USEC_DEVIATION 3
These require some comments. And maybe better naming (depending on the content
of those comments).
...
> +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 -EINVAL;
ERANGE?
> + *val = new;
> + return size;
> +}
...
> + if (sleep_len < 10)
> + udelay(sleep_len);
> + else if (sleep_len < 100)
> + usleep_range(sleep_len - UP_TO_100_USEC_DEVIATION,
> + sleep_len + UP_TO_100_USEC_DEVIATION);
> + else
> + usleep_range(sleep_len - MORE_THAN_100_USEC_DEVIATION,
> + sleep_len + MORE_THAN_100_USEC_DEVIATION);
NIH fsleep()
...
> +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 = test_bit(i, wiegand_gpio->ctlr->data_bitmap);
> + is_last_bit = (i + 1) == bitlen;
This is idempotent from for-loop, so...
> + wiegand_gpio_send_bit(wiegand_gpio, bit_value, is_last_bit);
> + }
unsigned int i;
bool value;
if (bitlen == 0)
return 0;
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;
> +}
...
> +static int wiegand_gpio_request(struct device *dev, struct wiegand_gpio *wiegand_gpio)
> +{
> + wiegand_gpio->data0_gpio = devm_gpiod_get(dev, "data-lo", GPIOD_OUT_HIGH);
> + if (IS_ERR(wiegand_gpio->data0_gpio))
> + return PTR_ERR(wiegand_gpio->data0_gpio);
> +
> + wiegand_gpio->data1_gpio = devm_gpiod_get(dev, "data-hi", GPIOD_OUT_HIGH);
> + return PTR_ERR_OR_ZERO(wiegand_gpio->data1_gpio);
Maybe you can use devm_gpiod_get_array()?
> +}
...
> +static int wiegand_gpio_probe(struct platform_device *device)
> +{
> + int status = 0;
Redundant assignment.
> + struct wiegand_controller *primary;
> + struct wiegand_gpio *wiegand_gpio;
> + struct device *dev = &device->dev;
...
> + primary->payload_len = 26; // set standard 26-bit format
Instead of comment, make a self-explanatory definition?
...
> + status = wiegand_register_controller(primary);
> + if (status)
> + dev_err_probe(wiegand_gpio->dev, status, "failed to register primary\n");
Why out of a sudden it uses this device and not real one?
> + return status;
With above
return 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", },
Inner comma is not needed.
> + {}
> +};
...
> +static struct platform_driver wiegand_gpio_driver = {
> + .driver = {
> + .name = "wiegand-gpio",
> + .of_match_table = wiegand_gpio_dt_idtable,
> + .dev_groups = wiegand_gpio_groups
Leave trailing comma when it's not about termination.
> + },
> + .probe = wiegand_gpio_probe
Ditto.
> +};
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv4 0/4] Wiegand bus driver and GPIO bitbanged Wiegand
2023-05-10 16:22 [PATCHv4 0/4] Wiegand bus driver and GPIO bitbanged Wiegand Martin Zaťovič
` (3 preceding siblings ...)
2023-05-10 16:22 ` [PATCHv4 4/4] wiegand: add Wiegand GPIO bitbanged controller driver Martin Zaťovič
@ 2023-06-22 12:36 ` Andy Shevchenko
4 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2023-06-22 12:36 UTC (permalink / raw)
To: Martin Zaťovič
Cc: linux-kernel, robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh,
beanhuo, nipun.gupta, linus.walleij, mwen, bvanassche, arnd,
ogabbay, linux, jacek.lawrynowicz, geert+renesas,
benjamin.tissoires, masahiroy, yangyicong, devicetree
On Wed, May 10, 2023 at 06:22:39PM +0200, Martin Zaťovič wrote:
> Hello,
>
> thank you for the feedback regarding the previous version of this patch series.
> I have tried to follow all of the advice I got and fix all the pointed issues.
> One of the main issues was the usage of of API for device registration. This
> has now been fixed to use fwnode API, however I was not yet able to get rid of
> the of_device include, since it is required for of_driver_match_device. Please
> let me know if this is correct.
Since it is a bus, I think we need that.
> CHANGELOG:
>
> wiegand.c:
> - changed ID allocation API from IDR to IDA, since the references associated to
> the IDs are not needed
> - removed the board_lock mutex, because it was only guarding the allocacion
> and freeing of IDs, which is already supported by IDA API
> - restructured the file, so that most functions are close to their caller, or
> defined them at the top for better code readability
> - in the function devm_wiegand_register_controller, the devres management of
> the pointer to wiegand_controller structure has been replaced with
> devm_add_action_or_reset function. It was intended to do the same with
> devm_wiegand_alloc_controller, however, the kernel kept panicing, despite the
> call order of the unregister and release functions being proper(same as with
> devres managed pointer). Please let me know if this is an absolute must, if so
> I will look into it further.
What panic? Can you elaborate?
> - moved the miscdevice from wiegand-gpio driver to be a part of the bus
> driver. Now every controller is associated a /dev file. The file operation
> functions were simply moved and renamed and the miscdevice structure was moved
> to be a part of wiegand_controller structure
> - since now every controller has a miscdevice assosciated, the data_buffer was
> also moved to be a part of the controller structure, and it was made a bitmap
> - used fwnode API for device registration instead of of API
> - removed warnings when driver fails to get wiegand properties, instead
> implemented mechanism for setting a default value similar I2C
> - removed the driver matching code in register driver, as
> of_driver_match_device does that already
> - made wiegand_device and opaque pointer
> - changed the terminology to primary and secondary
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 11+ messages in thread