* [PATCHv3 1/4] dt-bindings: wiegand: add Wiegand controller common properties
2023-03-01 14:28 [PATCHv3 0/4] Wiegand bus driver and GPIO bitbanged controller Martin Zaťovič
@ 2023-03-01 14:28 ` Martin Zaťovič
2023-03-03 19:32 ` Rob Herring
2023-03-01 14:28 ` [PATCHv3 2/4] wiegand: add Wiegand bus driver Martin Zaťovič
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Martin Zaťovič @ 2023-03-01 14:28 UTC (permalink / raw)
To: linux-kernel
Cc: robh+dt, krzysztof.kozlowski+dt, gregkh, airlied, dipenp, treding,
mwen, fmdefrancesco, arnd, bvanassche, ogabbay, axboe,
mathieu.poirier, linux, masahiroy, yangyicong, dan.j.williams,
jacek.lawrynowicz, benjamin.tissoires, devicetree, furong.zhou,
andriy.shevchenko, linus.walleij, Martin Zaťovič
Wiegand bus is defined by a Wiegand controller node. This node
can contain one or more device nodes for devices attached to
the controller(it is advised to only connect one device as Wiegand
is a point-to-point bus).
Wiegand controller needs to specify several attributes such as
the pulse length in order to function properly. These attributes
are documented here.
Acked-by: Linus Walleij <linus.walleij@linaro.org>
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..df985cb3045a
--- /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č <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
diff --git a/MAINTAINERS b/MAINTAINERS
index b0db911207ba..1f6f6d236f0c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22512,6 +22512,11 @@ L: linux-rtc@vger.kernel.org
S: Maintained
F: drivers/rtc/rtc-sd3078.c
+WIEGAND BUS DRIVER
+M: Martin Zaťovič <m.zatovic1@gmail.com>
+S: Maintained
+F: Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
+
WIIMOTE HID DRIVER
M: David Rheinsberg <david.rheinsberg@gmail.com>
L: linux-input@vger.kernel.org
--
2.39.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCHv3 1/4] dt-bindings: wiegand: add Wiegand controller common properties
2023-03-01 14:28 ` [PATCHv3 1/4] dt-bindings: wiegand: add Wiegand controller common properties Martin Zaťovič
@ 2023-03-03 19:32 ` Rob Herring
0 siblings, 0 replies; 19+ messages in thread
From: Rob Herring @ 2023-03-03 19:32 UTC (permalink / raw)
To: Martin Zaťovič
Cc: linux-kernel, krzysztof.kozlowski+dt, gregkh, airlied, dipenp,
treding, mwen, fmdefrancesco, arnd, bvanassche, ogabbay, axboe,
mathieu.poirier, linux, masahiroy, yangyicong, dan.j.williams,
jacek.lawrynowicz, benjamin.tissoires, devicetree, furong.zhou,
andriy.shevchenko, linus.walleij
On Wed, Mar 01, 2023 at 03:28:32PM +0100, Martin Zaťovič wrote:
> Wiegand bus is defined by a Wiegand controller node. This node
> can contain one or more device nodes for devices attached to
> the controller(it is advised to only connect one device as Wiegand
> is a point-to-point bus).
>
> 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>
> 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..df985cb3045a
> --- /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č <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: |
Don't need '|' here and elsewhere.
With that fixed,
Reviewed-by: Rob Herring <robh@kernel.org>
> + 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 b0db911207ba..1f6f6d236f0c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22512,6 +22512,11 @@ L: linux-rtc@vger.kernel.org
> S: Maintained
> F: drivers/rtc/rtc-sd3078.c
>
> +WIEGAND BUS DRIVER
> +M: Martin Zaťovič <m.zatovic1@gmail.com>
> +S: Maintained
> +F: Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
> +
> WIIMOTE HID DRIVER
> M: David Rheinsberg <david.rheinsberg@gmail.com>
> L: linux-input@vger.kernel.org
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCHv3 2/4] wiegand: add Wiegand bus driver
2023-03-01 14:28 [PATCHv3 0/4] Wiegand bus driver and GPIO bitbanged controller Martin Zaťovič
2023-03-01 14:28 ` [PATCHv3 1/4] dt-bindings: wiegand: add Wiegand controller common properties Martin Zaťovič
@ 2023-03-01 14:28 ` Martin Zaťovič
2023-03-01 16:23 ` Andy Shevchenko
2023-03-01 22:53 ` kernel test robot
2023-03-01 14:28 ` [PATCHv3 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand controller Martin Zaťovič
` (3 subsequent siblings)
5 siblings, 2 replies; 19+ messages in thread
From: Martin Zaťovič @ 2023-03-01 14:28 UTC (permalink / raw)
To: linux-kernel
Cc: robh+dt, krzysztof.kozlowski+dt, gregkh, airlied, dipenp, treding,
mwen, fmdefrancesco, arnd, bvanassche, ogabbay, axboe,
mathieu.poirier, linux, masahiroy, yangyicong, dan.j.williams,
jacek.lawrynowicz, benjamin.tissoires, devicetree, furong.zhou,
andriy.shevchenko, linus.walleij, 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 | 500 ++++++++++++++++++++++++++++++++++++++
include/linux/wiegand.h | 155 ++++++++++++
7 files changed, 681 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 1f6f6d236f0c..23a67b32f095 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22516,6 +22516,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
WIIMOTE HID DRIVER
M: David Rheinsberg <david.rheinsberg@gmail.com>
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 20b118dca999..ef96e937eacc 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..d6b63250e80b
--- /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..ebec2e3e4cd6
--- /dev/null
+++ b/drivers/wiegand/wiegand.c
@@ -0,0 +1,500 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/device.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/of.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;
+static DEFINE_IDR(wiegand_controller_idr);
+static DEFINE_MUTEX(board_lock);
+
+static void devm_wiegand_release_controller(struct device *dev, void *ctlr)
+{
+ wiegand_controller_put(*(struct wiegand_controller **)ctlr);
+}
+
+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.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);
+
+/**
+ * wiegand_controller_check_ops - checks whether message transfer function was defined for a
+ * controller
+ * @ctlr: controller structure to check
+ */
+static int wiegand_controller_check_ops(struct wiegand_controller *ctlr)
+{
+ if (!ctlr->transfer_message)
+ return -EINVAL;
+ return 0;
+}
+
+/**
+ * of_register_wiegand_device - allocates and registers a new Wiegand device based on devicetree
+ * node
+ * @ctlr: controller structure to attach device to
+ * @nc: devicetree node for the device
+ */
+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;
+ }
+
+ 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);
+}
+
+/**
+ * of_register_wiegand_devices - creates a wiegand device for all children of a controller
+ * devicetree node
+ * @ctlr: controller structure to check
+ */
+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);
+ }
+ }
+}
+
+/**
+ * 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 the devicetree defined attributes and checks whether they have all been initialized 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, 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, setting the default value 50us\n");
+ ctlr->pulse_len = 50;
+ }
+ if (ctlr->interval_len == 0) {
+ dev_warn(&ctlr->dev, "interval_len is not initialized, setting the default value 2000us\n");
+ ctlr->interval_len = 2000;
+ }
+ if (ctlr->frame_gap == 0) {
+ dev_warn(&ctlr->dev, "frame_gap is not initialized, setting the default value 2000us\n");
+ ctlr->frame_gap = 2000;
+ }
+
+ 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);
+
+/* 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)
+{
+ if (of_driver_match_device(dev, drv))
+ return 1;
+
+ return 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 int wiegand_remove(struct device *dev)
+{
+ const struct wiegand_driver *wdrv = to_wiegand_driver(dev->driver);
+
+ if (wdrv->remove)
+ wdrv->remove(to_wiegand_device(dev));
+
+ return 0;
+}
+
+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 (wdrv->driver.name) {
+ 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);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void __exit wiegand_exit(void)
+{
+ 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..9dc683d91b3d
--- /dev/null
+++ b/include/linux/wiegand.h
@@ -0,0 +1,155 @@
+/* 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;
+
+ s16 bus_num;
+
+ bool slave;
+ bool devm_allocated;
+
+ u32 pulse_len;
+ u32 interval_len;
+ u32 frame_gap;
+ u32 payload_len;
+ u8 device_count;
+
+ 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
+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);
+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 */
+
+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.39.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCHv3 2/4] wiegand: add Wiegand bus driver
2023-03-01 14:28 ` [PATCHv3 2/4] wiegand: add Wiegand bus driver Martin Zaťovič
@ 2023-03-01 16:23 ` Andy Shevchenko
2023-03-01 16:48 ` Andy Shevchenko
2023-04-04 13:13 ` Martin Zaťovič
2023-03-01 22:53 ` kernel test robot
1 sibling, 2 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-03-01 16:23 UTC (permalink / raw)
To: Martin Zaťovič
Cc: linux-kernel, robh+dt, krzysztof.kozlowski+dt, gregkh, airlied,
dipenp, treding, mwen, fmdefrancesco, arnd, bvanassche, ogabbay,
axboe, mathieu.poirier, linux, masahiroy, yangyicong,
dan.j.williams, jacek.lawrynowicz, benjamin.tissoires, devicetree,
furong.zhou, linus.walleij
On Wed, Mar 01, 2023 at 03:28:33PM +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.
...
> +#include <linux/device.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/module.h>
> +#include <linux/of.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;
> +static DEFINE_IDR(wiegand_controller_idr);
Why not IDA or even xarray?
...
> +static DEFINE_MUTEX(board_lock);
Or locks need a good description for what they are.
...
> +static void devm_wiegand_release_controller(struct device *dev, void *ctlr)
> +{
> + wiegand_controller_put(*(struct wiegand_controller **)ctlr);
> +}
This is not used in the following function, so can be moved closer to its user.
...
> +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;
Can this utilize devm_add_action_or_reset()?
> +}
...
> +/**
> + * of_register_wiegand_device - allocates and registers a new Wiegand device based on devicetree
NAK for OF only code. New, esp. bus, code must be agnostic. We have all means
for that.
register_wiegand_device() or similar name.
> + * node
> + * @ctlr: controller structure to attach device to
> + * @nc: devicetree node for the device
> + */
> +static struct wiegand_device *of_register_wiegand_device(struct wiegand_controller *ctlr,
Ditto.
> + struct device_node *nc)
struct fwnode_handle *fwnode
> +{
> + 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;
> + }
> + of_node_get(nc);
> + wiegand->dev.of_node = nc;
> + wiegand->dev.fwnode = of_fwnode_handle(nc);
device_set_node(&wiegand->dev, fwnode_handle_get(fwnode));
> + 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);
> +}
> +
> +/**
> + * of_register_wiegand_devices - creates a wiegand device for all children of a controller
> + * devicetree node
> + * @ctlr: controller structure to check
> + */
> +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);
> + }
> + }
No way. Use agnostic approach. See above for some suggestions.
> +}
...
> + if (!dev)
> + return -ENODEV;
When is it true and why is it a problem?
> + if (ctlr->dev.of_node) {
> + id = of_alias_get_id(ctlr->dev.of_node, "wiegand");
Why? What does this bring to us and why it's so important?
> + 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;
Why rewriting error code?
> + }
> + 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, setting the default value 50us\n");
> + ctlr->pulse_len = 50;
> + }
> + if (ctlr->interval_len == 0) {
> + dev_warn(&ctlr->dev, "interval_len is not initialized, setting the default value 2000us\n");
> + ctlr->interval_len = 2000;
> + }
> + if (ctlr->frame_gap == 0) {
> + dev_warn(&ctlr->dev, "frame_gap is not initialized, setting the default value 2000us\n");
> + ctlr->frame_gap = 2000;
> + }
Why warnings? Can't it survive without them? (See, for example, how I²C
controllers get timings).
> + 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;
> +}
...
> +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);
> + }
devm_add_action_or_reset() ?
> + return ret;
> +}
...
> +struct wiegand_device *wiegand_alloc_device(struct wiegand_controller *ctlr)
> +{
> + struct wiegand_device *wiegand;
> +
> + if (!wiegand_controller_get(ctlr))
> + return NULL;
Is it important to be called before pure memory allocation? The reference
counting on the existing resource is less failure prone, right?
> + 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;
> +}
...
> +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);
Why error is ignored?
> +}
...
> + if (wiegand->dev.of_node) {
> + of_node_clear_flag(wiegand->dev.of_node, OF_POPULATED);
> + of_node_put(wiegand->dev.of_node);
> + }
fwnode APIs, please.
...
> +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;
Hmm... this is just
if (wdrv->probe)
return wdrv->probe(wiegand);
return 0;
> +}
...
> + 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 (wdrv->driver.name) {
> + 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);
> + }
> + }
This looks like very much of a repetition of the existing code somewhere.
...
> +/**
> + * 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];
> +};
Wondering if this can be made an opaque pointer instead.
...
> +/**
> + * 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.
At some point you lost the grip on @ key button.
> + */
...
> +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;
Making it first may save a few assembly instructions in pointer arithmetics.
Check if I'm right with bloat-o-meter (you will need a user of this data type,
of course).
> +};
...
> + driver_unregister(&wdrv->driver);
Yep, here and...
> + return drv ? container_of(drv, struct wiegand_driver, driver) : NULL;
...here you will save code bytes.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCHv3 2/4] wiegand: add Wiegand bus driver
2023-03-01 16:23 ` Andy Shevchenko
@ 2023-03-01 16:48 ` Andy Shevchenko
2023-04-04 13:13 ` Martin Zaťovič
1 sibling, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-03-01 16:48 UTC (permalink / raw)
To: Martin Zaťovič
Cc: linux-kernel, robh+dt, krzysztof.kozlowski+dt, gregkh, airlied,
dipenp, treding, mwen, fmdefrancesco, arnd, bvanassche, ogabbay,
axboe, mathieu.poirier, linux, masahiroy, yangyicong,
dan.j.williams, jacek.lawrynowicz, benjamin.tissoires, devicetree,
furong.zhou, linus.walleij
On Wed, Mar 01, 2023 at 06:23:54PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 01, 2023 at 03:28:33PM +0100, Martin Zaťovič wrote:
...
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
Forgot to add that these are probably optional. Or at least OF specific code
should leave in a separate file with a good explanation why we even have it
to begin with.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv3 2/4] wiegand: add Wiegand bus driver
2023-03-01 16:23 ` Andy Shevchenko
2023-03-01 16:48 ` Andy Shevchenko
@ 2023-04-04 13:13 ` Martin Zaťovič
2023-04-04 20:30 ` Linus Walleij
1 sibling, 1 reply; 19+ messages in thread
From: Martin Zaťovič @ 2023-04-04 13:13 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-kernel, robh+dt, krzysztof.kozlowski+dt, gregkh, airlied,
dipenp, treding, mwen, fmdefrancesco, arnd, bvanassche, ogabbay,
axboe, mathieu.poirier, linux, masahiroy, yangyicong,
dan.j.williams, jacek.lawrynowicz, benjamin.tissoires, devicetree,
furong.zhou, linus.walleij
On Wed, Mar 01, 2023 at 06:23:54PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 01, 2023 at 03:28:33PM +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.
>
> ...
>
> > +#include <linux/device.h>
> > +#include <linux/dmaengine.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/module.h>
> > +#include <linux/of.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;
>
> > +static DEFINE_IDR(wiegand_controller_idr);
>
> Why not IDA or even xarray?
>
> ...
>
> > +static DEFINE_MUTEX(board_lock);
>
> Or locks need a good description for what they are.
>
> ...
>
> > +static void devm_wiegand_release_controller(struct device *dev, void *ctlr)
> > +{
> > + wiegand_controller_put(*(struct wiegand_controller **)ctlr);
> > +}
>
> This is not used in the following function, so can be moved closer to its user.
>
> ...
>
> > +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;
>
> Can this utilize devm_add_action_or_reset()?
>
> > +}
>
> ...
>
> > +/**
> > + * of_register_wiegand_device - allocates and registers a new Wiegand device based on devicetree
>
> NAK for OF only code. New, esp. bus, code must be agnostic. We have all means
> for that.
In one of the previous versions of this patch series, there was also the possibility to instantiate
the device from another driver. I have been told, that this is not the way to go anymore, unless
there is a very specific reason for that. I did not find such reason, so I have removed this suport.
The only other device instantiating method I could think of is ACPI, however I believe it would be
just dead code no one would use, as the Wiegand interface is not normally used on non-embedded
devices. This is the point Evgeny Boger argued for as well. Is there any other device instantiating
method I am not aware of, that would be suitable for this bus?
> register_wiegand_device() or similar name.
>
> > + * node
> > + * @ctlr: controller structure to attach device to
> > + * @nc: devicetree node for the device
> > + */
> > +static struct wiegand_device *of_register_wiegand_device(struct wiegand_controller *ctlr,
>
> Ditto.
>
> > + struct device_node *nc)
>
> struct fwnode_handle *fwnode
>
> > +{
> > + 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;
> > + }
>
> > + of_node_get(nc);
> > + wiegand->dev.of_node = nc;
> > + wiegand->dev.fwnode = of_fwnode_handle(nc);
>
> device_set_node(&wiegand->dev, fwnode_handle_get(fwnode));
>
> > + 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);
> > +}
> > +
> > +/**
> > + * of_register_wiegand_devices - creates a wiegand device for all children of a controller
> > + * devicetree node
> > + * @ctlr: controller structure to check
> > + */
> > +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);
> > + }
> > + }
>
> No way. Use agnostic approach. See above for some suggestions.
>
> > +}
>
> ...
>
> > + if (!dev)
> > + return -ENODEV;
>
> When is it true and why is it a problem?
>
> > + if (ctlr->dev.of_node) {
> > + id = of_alias_get_id(ctlr->dev.of_node, "wiegand");
>
> Why? What does this bring to us and why it's so important?
>
> > + 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;
>
> Why rewriting error code?
>
> > + }
> > + 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, setting the default value 50us\n");
> > + ctlr->pulse_len = 50;
> > + }
> > + if (ctlr->interval_len == 0) {
> > + dev_warn(&ctlr->dev, "interval_len is not initialized, setting the default value 2000us\n");
> > + ctlr->interval_len = 2000;
> > + }
> > + if (ctlr->frame_gap == 0) {
> > + dev_warn(&ctlr->dev, "frame_gap is not initialized, setting the default value 2000us\n");
> > + ctlr->frame_gap = 2000;
> > + }
>
> Why warnings? Can't it survive without them? (See, for example, how I²C
> controllers get timings).
>
> > + 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;
> > +}
>
> ...
>
> > +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);
> > + }
>
> devm_add_action_or_reset() ?
>
> > + return ret;
> > +}
>
> ...
>
> > +struct wiegand_device *wiegand_alloc_device(struct wiegand_controller *ctlr)
> > +{
> > + struct wiegand_device *wiegand;
> > +
> > + if (!wiegand_controller_get(ctlr))
> > + return NULL;
>
> Is it important to be called before pure memory allocation? The reference
> counting on the existing resource is less failure prone, right?
>
> > + 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;
> > +}
>
> ...
>
> > +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);
>
> Why error is ignored?
>
> > +}
>
> ...
>
> > + if (wiegand->dev.of_node) {
> > + of_node_clear_flag(wiegand->dev.of_node, OF_POPULATED);
> > + of_node_put(wiegand->dev.of_node);
> > + }
>
> fwnode APIs, please.
>
> ...
>
> > +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;
>
> Hmm... this is just
>
> if (wdrv->probe)
> return wdrv->probe(wiegand);
>
> return 0;
>
> > +}
>
> ...
>
> > + 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 (wdrv->driver.name) {
> > + 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);
> > + }
> > + }
>
> This looks like very much of a repetition of the existing code somewhere.
>
> ...
>
> > +/**
> > + * 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];
> > +};
>
> Wondering if this can be made an opaque pointer instead.
>
> ...
>
> > +/**
> > + * 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.
>
> At some point you lost the grip on @ key button.
>
> > + */
>
> ...
>
> > +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;
>
> Making it first may save a few assembly instructions in pointer arithmetics.
> Check if I'm right with bloat-o-meter (you will need a user of this data type,
> of course).
>
> > +};
>
> ...
>
> > + driver_unregister(&wdrv->driver);
>
> Yep, here and...
>
> > + return drv ? container_of(drv, struct wiegand_driver, driver) : NULL;
>
> ...here you will save code bytes.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCHv3 2/4] wiegand: add Wiegand bus driver
2023-04-04 13:13 ` Martin Zaťovič
@ 2023-04-04 20:30 ` Linus Walleij
2023-04-05 8:39 ` Andy Shevchenko
0 siblings, 1 reply; 19+ messages in thread
From: Linus Walleij @ 2023-04-04 20:30 UTC (permalink / raw)
To: Martin Zaťovič
Cc: Andy Shevchenko, linux-kernel, robh+dt, krzysztof.kozlowski+dt,
gregkh, airlied, dipenp, treding, mwen, fmdefrancesco, arnd,
bvanassche, ogabbay, axboe, mathieu.poirier, linux, masahiroy,
yangyicong, dan.j.williams, jacek.lawrynowicz, benjamin.tissoires,
devicetree, furong.zhou
On Tue, Apr 4, 2023 at 3:13 PM Martin Zaťovič <m.zatovic1@gmail.com> wrote:
> On Wed, Mar 01, 2023 at 06:23:54PM +0200, Andy Shevchenko wrote:
> > > +/**
> > > + * of_register_wiegand_device - allocates and registers a new Wiegand device based on devicetree
> >
> > NAK for OF only code. New, esp. bus, code must be agnostic. We have all means
> > for that.
>
> In one of the previous versions of this patch series, there was also the possibility to instantiate
> the device from another driver. I have been told, that this is not the way to go anymore, unless
> there is a very specific reason for that. I did not find such reason, so I have removed this suport.
I don't know for sure but I think Andy simply means that you should take a look
in include/linux/property.h and replace every function named of_* with
the corresponding fwnode_* or device_* function from that file, and it
should all just magically work the same, but abstracted away from device
tree. It's not much more than a search/replace and rename operation
(unless there is a "hole" in the fwnode implementations... I hope not.)
In the end you can keep just <linux/property.h> and drop <linux/of.h>
and <linux/of_device.h> if this works.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv3 2/4] wiegand: add Wiegand bus driver
2023-04-04 20:30 ` Linus Walleij
@ 2023-04-05 8:39 ` Andy Shevchenko
0 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-04-05 8:39 UTC (permalink / raw)
To: Linus Walleij
Cc: Martin Zaťovič, linux-kernel, robh+dt,
krzysztof.kozlowski+dt, gregkh, airlied, dipenp, treding, mwen,
fmdefrancesco, arnd, bvanassche, ogabbay, axboe, mathieu.poirier,
linux, masahiroy, yangyicong, dan.j.williams, jacek.lawrynowicz,
benjamin.tissoires, devicetree, furong.zhou
On Tue, Apr 04, 2023 at 10:30:59PM +0200, Linus Walleij wrote:
> On Tue, Apr 4, 2023 at 3:13 PM Martin Zaťovič <m.zatovic1@gmail.com> wrote:
> > On Wed, Mar 01, 2023 at 06:23:54PM +0200, Andy Shevchenko wrote:
...
> > > > +/**
> > > > + * of_register_wiegand_device - allocates and registers a new Wiegand device based on devicetree
> > >
> > > NAK for OF only code. New, esp. bus, code must be agnostic. We have all means
> > > for that.
> >
> > In one of the previous versions of this patch series, there was also the possibility to instantiate
> > the device from another driver. I have been told, that this is not the way to go anymore, unless
> > there is a very specific reason for that. I did not find such reason, so I have removed this suport.
>
> I don't know for sure but I think Andy simply means that you should take a look
> in include/linux/property.h and replace every function named of_* with
> the corresponding fwnode_* or device_* function from that file, and it
> should all just magically work the same, but abstracted away from device
> tree. It's not much more than a search/replace and rename operation
> (unless there is a "hole" in the fwnode implementations... I hope not.)
>
> In the end you can keep just <linux/property.h> and drop <linux/of.h>
> and <linux/of_device.h> if this works.
Yes. And the argument for embedded devices it's exactly why the non-OF
interface is needed as we want to have a possibility to connect devices on
ACPI (or another, if any) based platform without changing the code.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv3 2/4] wiegand: add Wiegand bus driver
2023-03-01 14:28 ` [PATCHv3 2/4] wiegand: add Wiegand bus driver Martin Zaťovič
2023-03-01 16:23 ` Andy Shevchenko
@ 2023-03-01 22:53 ` kernel test robot
[not found] ` <CAPGNi97oc+tSrt-NF4KZhcEyUfZTo4PT0ms8zYSFVEyzOBq4ZA@mail.gmail.com>
1 sibling, 1 reply; 19+ messages in thread
From: kernel test robot @ 2023-03-01 22:53 UTC (permalink / raw)
To: Martin Zaťovič, linux-kernel
Cc: oe-kbuild-all, robh+dt, krzysztof.kozlowski+dt, gregkh, airlied,
dipenp, treding, mwen, fmdefrancesco, arnd, bvanassche, ogabbay,
axboe, mathieu.poirier, linux, masahiroy, yangyicong,
dan.j.williams, jacek.lawrynowicz, benjamin.tissoires, devicetree,
furong.zhou, andriy.shevchenko, linus.walleij,
Martin Zaťovič
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 next-20230301]
[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/20230301-223030
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230301142835.19614-3-m.zatovic1%40gmail.com
patch subject: [PATCHv3 2/4] wiegand: add Wiegand bus driver
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20230302/202303020615.0F00suDa-lkp@intel.com/config)
compiler: sh4-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/c62b833f42989e355d82cd20b7803e0228e33792
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/20230301-223030
git checkout c62b833f42989e355d82cd20b7803e0228e33792
# 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=sh olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh SHELL=/bin/bash drivers/wiegand/
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/202303020615.0F00suDa-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/wiegand/wiegand.c:441:27: error: initialization of 'void (*)(struct device *)' from incompatible pointer type 'int (*)(struct device *)' [-Werror=incompatible-pointer-types]
441 | .remove = wiegand_remove,
| ^~~~~~~~~~~~~~
drivers/wiegand/wiegand.c:441:27: note: (near initialization for 'wiegand_bus_type.remove')
cc1: some warnings being treated as errors
vim +441 drivers/wiegand/wiegand.c
436
437 static struct bus_type wiegand_bus_type = {
438 .name = "wiegand",
439 .match = wiegand_match_device,
440 .probe = wiegand_probe,
> 441 .remove = wiegand_remove,
442 };
443
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCHv3 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand controller
2023-03-01 14:28 [PATCHv3 0/4] Wiegand bus driver and GPIO bitbanged controller Martin Zaťovič
2023-03-01 14:28 ` [PATCHv3 1/4] dt-bindings: wiegand: add Wiegand controller common properties Martin Zaťovič
2023-03-01 14:28 ` [PATCHv3 2/4] wiegand: add Wiegand bus driver Martin Zaťovič
@ 2023-03-01 14:28 ` Martin Zaťovič
2023-03-12 10:34 ` Evgeny Boger
2023-03-01 14:28 ` [PATCHv3 4/4] wiegand: add Wiegand GPIO bitbanged controller driver Martin Zaťovič
` (2 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Martin Zaťovič @ 2023-03-01 14:28 UTC (permalink / raw)
To: linux-kernel
Cc: robh+dt, krzysztof.kozlowski+dt, gregkh, airlied, dipenp, treding,
mwen, fmdefrancesco, arnd, bvanassche, ogabbay, axboe,
mathieu.poirier, linux, masahiroy, yangyicong, dan.j.williams,
jacek.lawrynowicz, benjamin.tissoires, devicetree, furong.zhou,
andriy.shevchenko, linus.walleij, 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 23a67b32f095..91e573466d6b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22519,6 +22519,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
+
WIIMOTE HID DRIVER
M: David Rheinsberg <david.rheinsberg@gmail.com>
L: linux-input@vger.kernel.org
--
2.39.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCHv3 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand controller
2023-03-01 14:28 ` [PATCHv3 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand controller Martin Zaťovič
@ 2023-03-12 10:34 ` Evgeny Boger
0 siblings, 0 replies; 19+ messages in thread
From: Evgeny Boger @ 2023-03-12 10:34 UTC (permalink / raw)
To: Martin Zaťovič, linux-kernel
Cc: robh+dt, krzysztof.kozlowski+dt, gregkh, airlied, dipenp, treding,
mwen, fmdefrancesco, arnd, bvanassche, ogabbay, axboe,
mathieu.poirier, linux, masahiroy, yangyicong, dan.j.williams,
jacek.lawrynowicz, benjamin.tissoires, devicetree, furong.zhou,
andriy.shevchenko, linus.walleij
On 3/1/23 17:28, 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
> +
> +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
In my experience, the data lines are usually labeled D0/D1, sometimes
DATA0/DATA1. Data high/data low marking is very rare.
> +
> +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 23a67b32f095..91e573466d6b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22519,6 +22519,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
> +
> WIIMOTE HID DRIVER
> M: David Rheinsberg <david.rheinsberg@gmail.com>
> L: linux-input@vger.kernel.org
--
Kind regards,
Evgeny Boger
CTO @ Wiren Board
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCHv3 4/4] wiegand: add Wiegand GPIO bitbanged controller driver
2023-03-01 14:28 [PATCHv3 0/4] Wiegand bus driver and GPIO bitbanged controller Martin Zaťovič
` (2 preceding siblings ...)
2023-03-01 14:28 ` [PATCHv3 3/4] dt-bindings: wiegand: add GPIO bitbanged Wiegand controller Martin Zaťovič
@ 2023-03-01 14:28 ` Martin Zaťovič
2023-03-01 16:43 ` Andy Shevchenko
` (2 more replies)
2023-03-01 16:43 ` [PATCHv3 0/4] Wiegand bus driver and GPIO bitbanged controller Andy Shevchenko
2023-03-12 10:19 ` Evgeny Boger
5 siblings, 3 replies; 19+ messages in thread
From: Martin Zaťovič @ 2023-03-01 14:28 UTC (permalink / raw)
To: linux-kernel
Cc: robh+dt, krzysztof.kozlowski+dt, gregkh, airlied, dipenp, treding,
mwen, fmdefrancesco, arnd, bvanassche, ogabbay, axboe,
mathieu.poirier, linux, masahiroy, yangyicong, dan.j.williams,
jacek.lawrynowicz, benjamin.tissoires, devicetree, furong.zhou,
andriy.shevchenko, linus.walleij, 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 | 316 ++++++++++++++++++
5 files changed, 336 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 91e573466d6b..eeeb343ee91c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22522,7 +22522,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
WIIMOTE HID DRIVER
M: David Rheinsberg <david.rheinsberg@gmail.com>
diff --git a/drivers/wiegand/Kconfig b/drivers/wiegand/Kconfig
index d6b63250e80b..d3a6c773c767 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..e67a30a1c5ae
--- /dev/null
+++ b/drivers/wiegand/wiegand-gpio.c
@@ -0,0 +1,316 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/poll.h>
+#include <linux/platform_device.h>
+#include <linux/slab.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 = dev_get_drvdata(dev);
+ 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 = dev_get_drvdata(dev);
+ 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);
+
+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 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);
+ dev->groups = wiegand_gpio_groups;
+
+ 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.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCHv3 4/4] wiegand: add Wiegand GPIO bitbanged controller driver
2023-03-01 14:28 ` [PATCHv3 4/4] wiegand: add Wiegand GPIO bitbanged controller driver Martin Zaťovič
@ 2023-03-01 16:43 ` Andy Shevchenko
2023-03-02 2:11 ` kernel test robot
2023-03-12 10:32 ` Evgeny Boger
2 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-03-01 16:43 UTC (permalink / raw)
To: Martin Zaťovič
Cc: linux-kernel, robh+dt, krzysztof.kozlowski+dt, gregkh, airlied,
dipenp, treding, mwen, fmdefrancesco, arnd, bvanassche, ogabbay,
axboe, mathieu.poirier, linux, masahiroy, yangyicong,
dan.j.williams, jacek.lawrynowicz, benjamin.tissoires, devicetree,
furong.zhou, linus.walleij
On Wed, Mar 01, 2023 at 03:28:35PM +0100, 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: January 2023
Blast from the past?
...
> +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.
What will be the name of the module if M?
...
> +#include <linux/of.h>
No way.
...
> +struct wiegand_gpio {
> + struct device *dev;
> + struct wiegand_controller *ctlr;
> + struct miscdevice misc_dev;
Make it first, same idea as per previous patch comments.
> + 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];
Have you considered DMA alignment? Is it a problem or not here?
> +};
...
> +static ssize_t store_ulong(u32 *val, const char *buf, size_t size, unsigned long max)
> +{
> + int rc;
> + u32 new;
> + if (max != 0 && new > max)
First part of the conditional is redundant. When you have such a user, you may
add the restriction back.
> + return -EINVAL;
> +
> + *val = new;
> + return size;
> +}
...
> +static struct attribute *wiegand_gpio_attrs[] = {
> + &dev_attr_payload_len.attr,
> + NULL,
No comma for the terminator entry.
> +};
> +
Redundant blank line.
> +ATTRIBUTE_GROUPS(wiegand_gpio);
...
> +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 is quite dangerous. You may end up with CPU 100% load for a long time
without any way out. What is the range and why udelay() can't be replaced
with usleep_range() for longer waits?
> +}
...
> +/* 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);
Ah, your buffer should probably be a bitmap.
Also consider bitmap_get_value8().
> + 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;
This is wrong. Homework: read the documentation and existing code to see why
and how to fix.
> + return len;
> +}
...
> +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)
!buf
DIV_ROUND_UP(msg_length / 8) > len
less overflow prone.
> + 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);
Can it be interrupted by a signal?
> + 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;
> +}
...
> + u8 msg_bytelength = (msg_bitlen % 8) ? (msg_bitlen / 8) + 1 : (msg_bitlen / 8);
DIV_ROUND_UP() (you will need math.h)
...
> + if (dev->of_node)
> + master->dev.of_node = device->dev.of_node;
No.
...
> + if (status)
> + return status;
What's this and why is it here?
I'm afraid you haven't compiled this code... :-(
...
> + master->transfer_message = &wiegand_gpio_transfer_message;
> + master->payload_len = 26; /* set standard 26-bit format */
Can you replace master with some of the suggested words?
Or is this a terminology from the specification of the bus?
...
> + status = wiegand_gpio_request(dev, wiegand_gpio);
> + if (status) {
> + dev_err(wiegand_gpio->dev, "failed at requesting GPIOs\n");
> + return status;
return dev_error_probe();
Ditto for the rest.
> + }
...
> + status = gpiod_direction_output(wiegand_gpio->gpio_data_hi, 1);
> + status |= gpiod_direction_output(wiegand_gpio->gpio_data_lo, 1);
Huh?!
...
> + wiegand_gpio->misc_dev.name = kasprintf(GFP_KERNEL, "wiegand-gpio%u", master->bus_num);
No checks?
...
> + dev->groups = wiegand_gpio_groups;
Feels like this can be moved to dev_groups member of the struct driver.
> +
> + return status;
> +}
...
> +static const struct of_device_id wiegand_gpio_dt_idtable[] = {
> + { .compatible = "wiegand-gpio", },
> + {},
No comma for the terminator entry.
> +};
...
> +
Redundant blank line.
> +module_platform_driver(wiegand_gpio_driver);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCHv3 4/4] wiegand: add Wiegand GPIO bitbanged controller driver
2023-03-01 14:28 ` [PATCHv3 4/4] wiegand: add Wiegand GPIO bitbanged controller driver Martin Zaťovič
2023-03-01 16:43 ` Andy Shevchenko
@ 2023-03-02 2:11 ` kernel test robot
2023-03-12 10:32 ` Evgeny Boger
2 siblings, 0 replies; 19+ messages in thread
From: kernel test robot @ 2023-03-02 2:11 UTC (permalink / raw)
To: Martin Zaťovič, linux-kernel
Cc: oe-kbuild-all, robh+dt, krzysztof.kozlowski+dt, gregkh, airlied,
dipenp, treding, mwen, fmdefrancesco, arnd, bvanassche, ogabbay,
axboe, mathieu.poirier, linux, masahiroy, yangyicong,
dan.j.williams, jacek.lawrynowicz, benjamin.tissoires, devicetree,
furong.zhou, andriy.shevchenko, linus.walleij,
Martin Zaťovič
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 next-20230301]
[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/20230301-223030
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230301142835.19614-5-m.zatovic1%40gmail.com
patch subject: [PATCHv3 4/4] wiegand: add Wiegand GPIO bitbanged controller driver
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20230302/202303020937.pxOPT5nt-lkp@intel.com/config)
compiler: sh4-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/641c36b9878a19ea4977f0e14df22c7475b423df
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/20230301-223030
git checkout 641c36b9878a19ea4977f0e14df22c7475b423df
# 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=sh olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh 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/202303020937.pxOPT5nt-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/wiegand/wiegand-gpio.c:50:9: warning: no previous prototype for 'payload_len_show' [-Wmissing-prototypes]
50 | ssize_t payload_len_show(struct device *dev, struct device_attribute *attr, char *buf)
| ^~~~~~~~~~~~~~~~
>> drivers/wiegand/wiegand-gpio.c:58:9: warning: no previous prototype for 'payload_len_store' [-Wmissing-prototypes]
58 | ssize_t payload_len_store(struct device *dev, struct device_attribute *attr, const char *buf,
| ^~~~~~~~~~~~~~~~~
>> drivers/wiegand/wiegand-gpio.c:81:6: warning: no previous prototype for 'wiegand_gpio_send_bit' [-Wmissing-prototypes]
81 | void wiegand_gpio_send_bit(struct wiegand_gpio *wiegand_gpio, bool value, bool last)
| ^~~~~~~~~~~~~~~~~~~~~
>> drivers/wiegand/wiegand-gpio.c:195:5: warning: no previous prototype for 'wiegand_gpio_transfer_message' [-Wmissing-prototypes]
195 | int wiegand_gpio_transfer_message(struct wiegand_device *dev, u8 *message, u8 msg_bitlen)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/payload_len_show +50 drivers/wiegand/wiegand-gpio.c
46
47 /*
48 * Attribute file for setting payload length of Wiegand messages.
49 */
> 50 ssize_t payload_len_show(struct device *dev, struct device_attribute *attr, char *buf)
51 {
52 struct wiegand_gpio *wiegand_gpio = dev_get_drvdata(dev);
53 struct wiegand_controller *ctlr = wiegand_gpio->ctlr;
54
55 return sysfs_emit(buf, "%u\n", ctlr->payload_len);
56 }
57
> 58 ssize_t payload_len_store(struct device *dev, struct device_attribute *attr, const char *buf,
59 size_t count)
60 {
61 struct wiegand_gpio *wiegand_gpio = dev_get_drvdata(dev);
62 struct wiegand_controller *ctlr = wiegand_gpio->ctlr;
63
64 return store_ulong(&(ctlr->payload_len), buf, count, WIEGAND_MAX_PAYLEN_BYTES * 8);
65 }
66 DEVICE_ATTR_RW(payload_len);
67
68 static struct attribute *wiegand_gpio_attrs[] = {
69 &dev_attr_payload_len.attr,
70 NULL,
71 };
72
73 ATTRIBUTE_GROUPS(wiegand_gpio);
74
75 /*
76 * To send a bit of value 1 following the wiegand protocol, one must set
77 * the wiegand_data_hi to low for the duration of pulse. Similarly to send
78 * a bit of value 0, the wiegand_data_lo is set to low for pulse duration.
79 * This way the two lines are never low at the same time.
80 */
> 81 void wiegand_gpio_send_bit(struct wiegand_gpio *wiegand_gpio, bool value, bool last)
82 {
83 u32 pulse_len = wiegand_gpio->ctlr->pulse_len;
84 u32 interval_len = wiegand_gpio->ctlr->interval_len;
85 u32 frame_gap = wiegand_gpio->ctlr->frame_gap;
86 struct gpio_desc *gpio = value ? wiegand_gpio->gpio_data_hi : 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, u16 bitlen)
100 {
101 size_t i;
102 bool bit_value, is_last_bit;
103
104 for (i = 0; i < bitlen; i++) {
105 bit_value = ((wiegand_gpio->data[i / 8] >> (7 - (i % 8))) & 0x01);
106 is_last_bit = (i + 1) == bitlen;
107 wiegand_gpio_send_bit(wiegand_gpio, bit_value, is_last_bit);
108 }
109
110 return 0;
111 }
112
113 static ssize_t wiegand_gpio_get_user_data(struct wiegand_gpio *wiegand_gpio, char __user const *buf,
114 size_t len)
115 {
116 size_t rc;
117
118 if (len > WIEGAND_MAX_PAYLEN_BYTES)
119 return -EBADMSG;
120
121 rc = copy_from_user(&wiegand_gpio->data[0], buf, WIEGAND_MAX_PAYLEN_BYTES);
122 if (rc < 0)
123 return rc;
124
125 return len;
126 }
127
128 static int wiegand_gpio_frelease(struct inode *ino, struct file *filp)
129 {
130 struct wiegand_gpio_instance *info = filp->private_data;
131 struct wiegand_gpio *wiegand_gpio = info->dev;
132
133 mutex_lock(&wiegand_gpio->mutex);
134 info->flags = 0;
135 mutex_unlock(&wiegand_gpio->mutex);
136
137 kfree(info);
138
139 return 0;
140 }
141
142 static ssize_t wiegand_gpio_fwrite(struct file *filp, char __user const *buf, size_t len,
143 loff_t *offset)
144 {
145 struct wiegand_gpio_instance *info = filp->private_data;
146 struct wiegand_gpio *wiegand_gpio = info->dev;
147 u32 msg_length = wiegand_gpio->ctlr->payload_len;
148 int rc;
149
150 if (buf == NULL || len == 0 || len * 8 < msg_length)
151 return -EINVAL;
152
153 rc = wiegand_gpio_get_user_data(wiegand_gpio, buf, len);
154 if (rc < 0)
155 return rc;
156
157 wiegand_gpio_write_by_bits(wiegand_gpio, msg_length);
158
159 return len;
160 }
161
162 static int wiegand_gpio_fopen(struct inode *ino, struct file *filp)
163 {
164 int rc;
165 struct wiegand_gpio_instance *info;
166 struct wiegand_gpio *wiegand_gpio = container_of(filp->f_op, struct wiegand_gpio, fops);
167
168 mutex_lock(&wiegand_gpio->mutex);
169
170 if ((filp->f_flags & O_ACCMODE) == O_RDONLY || (filp->f_flags & O_ACCMODE) == O_RDWR) {
171 dev_err(wiegand_gpio->dev, "Device is write only\n");
172 rc = -EIO;
173 goto err;
174 }
175
176 info = kzalloc(sizeof(*info), GFP_KERNEL);
177 if (!info) {
178 rc = -ENOMEM;
179 goto err;
180 }
181
182 info->dev = wiegand_gpio;
183 info->flags = filp->f_flags;
184 mutex_unlock(&wiegand_gpio->mutex);
185
186 filp->private_data = info;
187
188 return 0;
189 err:
190 mutex_unlock(&wiegand_gpio->mutex);
191 return rc;
192 }
193
194 /* This function is used by device drivers */
> 195 int wiegand_gpio_transfer_message(struct wiegand_device *dev, u8 *message, u8 msg_bitlen)
196 {
197 struct wiegand_controller *ctlr = dev->controller;
198 struct wiegand_gpio *wiegand_gpio = wiegand_master_get_devdata(ctlr);
199 u8 msg_bytelength = (msg_bitlen % 8) ? (msg_bitlen / 8) + 1 : (msg_bitlen / 8);
200
201 memcpy(wiegand_gpio->data, message, msg_bytelength);
202 wiegand_gpio_write_by_bits(wiegand_gpio, msg_bitlen);
203
204 return 0;
205 }
206
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCHv3 4/4] wiegand: add Wiegand GPIO bitbanged controller driver
2023-03-01 14:28 ` [PATCHv3 4/4] wiegand: add Wiegand GPIO bitbanged controller driver Martin Zaťovič
2023-03-01 16:43 ` Andy Shevchenko
2023-03-02 2:11 ` kernel test robot
@ 2023-03-12 10:32 ` Evgeny Boger
2 siblings, 0 replies; 19+ messages in thread
From: Evgeny Boger @ 2023-03-12 10:32 UTC (permalink / raw)
To: Martin Zaťovič, linux-kernel
Cc: robh+dt, krzysztof.kozlowski+dt, gregkh, airlied, dipenp, treding,
mwen, fmdefrancesco, arnd, bvanassche, ogabbay, axboe,
mathieu.poirier, linux, masahiroy, yangyicong, dan.j.williams,
jacek.lawrynowicz, benjamin.tissoires, devicetree, furong.zhou,
andriy.shevchenko, linus.walleij
On 3/1/23 17:28, 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.
>
> 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 | 316 ++++++++++++++++++
> 5 files changed, 336 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 91e573466d6b..eeeb343ee91c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22522,7 +22522,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
>
> WIIMOTE HID DRIVER
> M: David Rheinsberg <david.rheinsberg@gmail.com>
> diff --git a/drivers/wiegand/Kconfig b/drivers/wiegand/Kconfig
> index d6b63250e80b..d3a6c773c767 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..e67a30a1c5ae
> --- /dev/null
> +++ b/drivers/wiegand/wiegand-gpio.c
> @@ -0,0 +1,316 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/poll.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.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];
maybe use bitmap for this?
> +};
> +
> +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 = dev_get_drvdata(dev);
> + 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 = dev_get_drvdata(dev);
> + 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);
> +
> +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 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);
As far as I understand, Wiegand GPIO bit-banged driver is just one of
the many possible driver implementations. I can imagine having dedicated
hardware to send Wiegand messages. So it seems a little strange to have
chardev API implemented in the bit-banged driver and not in the core, so
it's common for all possible driver implementations.
> + dev->groups = wiegand_gpio_groups;
> +
> + 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>");
--
Kind regards,
Evgeny Boger
CTO @ Wiren Board
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCHv3 0/4] Wiegand bus driver and GPIO bitbanged controller
2023-03-01 14:28 [PATCHv3 0/4] Wiegand bus driver and GPIO bitbanged controller Martin Zaťovič
` (3 preceding siblings ...)
2023-03-01 14:28 ` [PATCHv3 4/4] wiegand: add Wiegand GPIO bitbanged controller driver Martin Zaťovič
@ 2023-03-01 16:43 ` Andy Shevchenko
2023-03-12 10:19 ` Evgeny Boger
5 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-03-01 16:43 UTC (permalink / raw)
To: Martin Zaťovič
Cc: linux-kernel, robh+dt, krzysztof.kozlowski+dt, gregkh, airlied,
dipenp, treding, mwen, fmdefrancesco, arnd, bvanassche, ogabbay,
axboe, mathieu.poirier, linux, masahiroy, yangyicong,
dan.j.williams, jacek.lawrynowicz, benjamin.tissoires, devicetree,
furong.zhou, linus.walleij
On Wed, Mar 01, 2023 at 03:28:31PM +0100, Martin Zaťovič wrote:
> Hello,
>
> thank you for the feedback regarding last patch. I have decided against
> using a copyright line despite the suggestion.
Thank you for an update. More work is needed.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCHv3 0/4] Wiegand bus driver and GPIO bitbanged controller
2023-03-01 14:28 [PATCHv3 0/4] Wiegand bus driver and GPIO bitbanged controller Martin Zaťovič
` (4 preceding siblings ...)
2023-03-01 16:43 ` [PATCHv3 0/4] Wiegand bus driver and GPIO bitbanged controller Andy Shevchenko
@ 2023-03-12 10:19 ` Evgeny Boger
5 siblings, 0 replies; 19+ messages in thread
From: Evgeny Boger @ 2023-03-12 10:19 UTC (permalink / raw)
To: m.zatovic1
Cc: airlied, andriy.shevchenko, arnd, axboe, benjamin.tissoires,
bvanassche, dan.j.williams, devicetree, dipenp, fmdefrancesco,
furong.zhou, gregkh, jacek.lawrynowicz, krzysztof.kozlowski+dt,
linus.walleij, linux-kernel, linux, masahiroy, mathieu.poirier,
mwen, ogabbay, robh+dt, treding, yangyicong
Hi Martin,
Thank you for you work! I'm currently working on Wiegand *receiver*
kernel driver, and hopefully we can make both sending and receiving
Wiegand implementation in kernel.
I tried to read all the discussion for the previous series, but still
don't quite understand why do we need the infrastructure to be that
complex. I mean Wiegand is point-to-point connection, so why do we even
need bus/controller/device abstractions at all? There will be always
just the single device per controller, right?
--
Kind regards,
Evgeny Boger
CTO @ Wiren Board
^ permalink raw reply [flat|nested] 19+ messages in thread