On 28/07/2022 11:17, Martin Zaťovič wrote:
> Wiegand is a communication protocol that is still widely used
> especially for access control applications. It utilizes two wires to
> transmit data - D0 and D1, the generic names of which are data-lo and
> data-hi.
>
> Both data lines are initially pulled up. To send a bit of value 1, the
> D1 line is set low. Similarly to send a bit of value 0, the D0 line is
> set low. Standard Wiegand formats include 26, 36 and 37 bit and they
> reserve the first and last bits for parity. The first(MSB) parity bit
> is set to 1 if the parity of the first half of the payload is odd. The
> last(LSB) parity bit is set to 1 if the parity of the second half of
> the payload even.
>
> The driver currently supports the 3 standard formats - 26, 36 and 37
> bits. When one of these formats is used, it automatically calculates
> the values of parity bits and appends them to the messages. It also
> offers to set a custom format. Using a custom format, the user is
> responsible for setting the parity bits. The driver offers setting of
> the following sysfs attributes:
>
> pulse_len - length of the low pulse in usec; defaults to 50us
> interval_len - length of a whole bit(pulse_len + high phase)
> in usec; defaults to 50us
> frame_gap - length of the last bit of a frame(pulse_len +
> high phase); defaults to interval_len
> format - valid values are 0 for custom, 26, 36, 37
> custom_payload_len - can be set when using a custom format(0);
> 0 means all bits of a message will be sent
>
> Signed-off-by: Martin Zaťovič <m.zatovic1@xxxxxxxxx>
> ---
> The driver was tested on NXP Verdin iMX8MP Plus.
>
> I would like to kindly ask a few questions:
> 1. Is debug printing of the data being transmitted a valid thing to do?
> Wiegand could potentially be used to transmit sensitive data which
> might get exposed by the debug mode.
> 2. The part of the code, where sysfs files are being created does not
> currently contain freeing of the ones already created on an error. Is
> it better to use goto jumps to free them and exit, or let the driver
> run without some of the attribute files?
>
> If you have any suggestions to make this patch better, please let me
> know, I am eager to learn. I am very much new to this field, so any
> feedback will be aprreciated.
> ---
> MAINTAINERS | 6 +
> drivers/bus/Kconfig | 10 +
> drivers/bus/Makefile | 1 +
> drivers/bus/wiegand-gpio.c | 661 +++++++++++++++++++++++++++++++++++++
> drivers/bus/wiegand-gpio.h | 54 +++
> 5 files changed, 732 insertions(+)
> create mode 100644 drivers/bus/wiegand-gpio.c
> create mode 100644 drivers/bus/wiegand-gpio.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 64379c699903..9a519530e44e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21586,6 +21586,12 @@ L: linux-rtc@xxxxxxxxxxxxxxx
> S: Maintained
> F: drivers/rtc/rtc-sd3078.c
>
> +WIEGAND WRITE-ONLY GPIO DRIVER
> +M: Martin Zaťovič <m.zatovic1@xxxxxxxxx>
> +S: Maintained
> +F: drivers/bus/wiegand-gpio.c
> +F: drivers/bus/wiegand-gpio.h
> +
> WIIMOTE HID DRIVER
> M: David Rheinsberg <david.rheinsberg@xxxxxxxxx>
> L: linux-input@xxxxxxxxxxxxxxx
> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
> index 7bfe998f3514..f7c7d3b24710 100644
> --- a/drivers/bus/Kconfig
> +++ b/drivers/bus/Kconfig
> @@ -231,6 +231,16 @@ config UNIPHIER_SYSTEM_BUS
> Support for UniPhier System Bus, a simple external bus. This is
> needed to use on-board devices connected to UniPhier SoCs.
>
> +config WIEGAND_GPIO
> + tristate "GPIO-based wiegand master (write only)"
> + depends on OF_GPIO
> + help
> + Say y here to enable a driver which uses GPIO pins to send
> + Wiegand data. Say m to build it as module. Say n to disable
> + building. The driver utilizes two data lines that need to be
> + defined as outputs in the device tree - wiegand-data-hi and
> + wiegand-data-lo.
> +
> config VEXPRESS_CONFIG
> tristate "Versatile Express configuration bus"
> default y if ARCH_VEXPRESS
> diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
> index d90eed189a65..cc21530a441f 100644
> --- a/drivers/bus/Makefile
> +++ b/drivers/bus/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_TI_SYSC) += ti-sysc.o
> obj-$(CONFIG_TS_NBUS) += ts-nbus.o
> obj-$(CONFIG_UNIPHIER_SYSTEM_BUS) += uniphier-system-bus.o
> obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o
> +obj-$(CONFIG_WIEGAND_GPIO) += wiegand-gpio.o
>
This is confusing. You put it in bus, but you just added a single
driver, not a bus driver, right?
If it is not a bus driver, it goes to respective subsystem.
> +DEVICE_ATTR_RW(pulse_len);
> +DEVICE_ATTR_RW(interval_len);
> +DEVICE_ATTR_RW(frame_gap);
> +DEVICE_ATTR_RW(format);
> +DEVICE_ATTR_RW(payload_len);
> +
> +static int wiegand_gpio_dev_probe(struct platform_device *pdev)
> +{
> + int rc;
> + struct wiegand_gpio_device *wiegand_gpio;
> + struct wiegand_gpio_platform_data *pdata = pdev->dev.platform_data;
> +
> + if (!pdata) {
> + if (IS_ERR(pdata))
> + return PTR_ERR(pdata);
Why? How is this going to bind? Who provides pdata? Who provides error
pdata?!?
> + }
> +
> + wiegand_gpio = kzalloc(sizeof(struct wiegand_gpio_device), GFP_KERNEL);
devm, sizeof(*wiegand_gpio)
> + if (!wiegand_gpio)
> + return -ENOMEM;
> +
> + wiegand_gpio->dev = &pdev->dev;
> +
> + /* Initialize character device */
> + cdev_init(&wiegand_gpio->cdev, &wiegand_gpio_fops);
> + wiegand_gpio->cdev.owner = THIS_MODULE;
> +
> + rc = cdev_add(&wiegand_gpio->cdev, MKDEV(MAJOR(base_devno),
> + pdev->id == -1 ? 0 : pdev->id), 1);
> + if (rc < 0) {
> + dev_err(&pdev->dev, "Failed to allocate cdev: %d\n", rc);
> + kfree(wiegand_gpio);
> + return rc;
> + }
> +
> + wiegand_gpio->dev->devt = wiegand_gpio->cdev.dev;
> + mutex_init(&wiegand_gpio->mutex);
> +
> + /* Get GPIO lines using device tree bindings. */
> + wiegand_gpio->gpio_data_lo = devm_gpiod_get(wiegand_gpio->dev,
> + "wiegand-data-lo", GPIOD_OUT_HIGH);
> + if (IS_ERR(wiegand_gpio->gpio_data_lo)) {
> + dev_info(wiegand_gpio->dev,
> + "Failed to get wiegand-data-lo pin.\n");
> + return PTR_ERR(wiegand_gpio->gpio_data_lo);
No, return dev_err_probe().
> + }
> + wiegand_gpio->gpio_data_hi = devm_gpiod_get(wiegand_gpio->dev,
> + "wiegand-data-hi", GPIOD_OUT_HIGH);
> + if (IS_ERR(wiegand_gpio->gpio_data_hi)) {
> + dev_info(wiegand_gpio->dev,
> + "Failed to get wiegand-data-hi pin.\n");
> + return PTR_ERR(wiegand_gpio->gpio_data_hi);
return dev_err_probe()
> + }
> +
> + memcpy(&wiegand_gpio->setup, &WIEGAND_SETUP,
> + sizeof(struct wiegand_setup));
> +
> + platform_set_drvdata(pdev, wiegand_gpio);
> +
> + dev_info(&pdev->dev, "devno=%d:%d\n",
> + MAJOR(wiegand_gpio->dev->devt),
> + MINOR(wiegand_gpio->dev->devt));
> +
> + rc = device_create_file(wiegand_gpio->dev, &dev_attr_pulse_len);
> + rc |= device_create_file(wiegand_gpio->dev, &dev_attr_interval_len);
> + rc |= device_create_file(wiegand_gpio->dev, &dev_attr_frame_gap);
> + rc |= device_create_file(wiegand_gpio->dev, &dev_attr_format);
> + rc |= device_create_file(wiegand_gpio->dev,
> + &dev_attr_payload_len);
> + if (rc != 0)
> + dev_warn(&pdev->dev,
> + "Failed to register attribute files(%d)\n", rc);
> +
> + return 0;
> +}
> +
> +static int wiegand_gpio_dev_remove(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct wiegand_gpio_device *wiegand_gpio = platform_get_drvdata(pdev);
> +
> + device_remove_file(dev, &dev_attr_pulse_len);
> + device_remove_file(dev, &dev_attr_interval_len);
> + device_remove_file(dev, &dev_attr_frame_gap);
> + device_remove_file(dev, &dev_attr_format);
> + device_remove_file(dev, &dev_attr_payload_len);
> + cdev_del(&wiegand_gpio->cdev);
> + kfree(wiegand_gpio);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id wiegand_gpio_dt_idtable[] = {
> + { .compatible = "wiegand-gpio" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, wiegand_gpio_dt_idtable);
> +#endif
> +
> +static struct platform_driver wiegand_gpio_driver = {
> + .probe = wiegand_gpio_dev_probe,
> + .remove = wiegand_gpio_dev_remove,
> + .driver = {
> + .owner = THIS_MODULE,
This is not needed. Do you see it in any driver?
> + .name = "wiegand-gpio",
> + .of_match_table = of_match_ptr(wiegand_gpio_dt_idtable),
> + }
> +};
> +MODULE_ALIAS("platform:wiegand-gpio");
> +
Best regards,
Krzysztof
