Re: [RFCv3 PATCH 2/2] bus: add bit banged Wiegand bus driver

[Krzysztof Kozlowski]

On 24/10/2022 12:26, Martin Zaťovič wrote:
> This patch adds a bit banged Wiegand bus driver. The communication
> is realized over two GPIO lines defined in the devicetree.
> The parameters specific for the bus such as the pulse duration
> are also configured in the devicetree, while format and payload
> length of messages are configured via sysfs files. The driver
> currently supports 4 formats - 26, 36 and 37-bit and a custom
> length format. The 26, 36 and 37-bit formats automatically
> append the checksum as specified in the Wiegand protocol.
> It is up to the user to append or discard the checksum bits
> using the custom format. A user can use this driver to write
> directly to the bus using a device file. Drivers for devices
> communicating via Wiegand can utilize the API functions to
> write on the bus or set a message format.
> 
> Signed-off-by: Martin Zaťovič <m.zatovic1@xxxxxxxxx>
> ---
>  Documentation/ABI/testing/sysfs-bus-wiegand |  20 +
>  MAINTAINERS                                 |   8 +
>  drivers/bus/Kconfig                         |   7 +
>  drivers/bus/Makefile                        |   1 +
>  drivers/bus/wiegand.c                       | 509 ++++++++++++++++++++
>  include/linux/wiegand.h                     |  58 +++
>  6 files changed, 603 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-wiegand
>  create mode 100644 drivers/bus/wiegand.c
>  create mode 100644 include/linux/wiegand.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-wiegand b/Documentation/ABI/testing/sysfs-bus-wiegand
> new file mode 100644
> index 000000000000..1f989e360d53
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-wiegand
> @@ -0,0 +1,20 @@
> +What:		/sys/devices/platform/.../wiegand-attributes/format
> +Date:		October 2022
> +Contact:	Martin Zaťovič <m.zatovic1@xxxxxxxxx>
> +Description:    Sets a format of Wiegand communication. Currently supported
> +        formats are 26, 36 and 37 bits. These formats automatically add
> +        checksums to the first and last bits of a message. To set a custom
> +        format, 0 needs to be written here. Custom format writes an amount of
> +        bits defined by payload_len(see below) and it does not add checksums so
> +        the user is responsible for that.
> +Users:		any user space application which wants to communicate using Wiegand
> +
> +What:		/sys/devices/platform/.../wiegand-attributes/payload_len
> +Date:		October 2022
> +Contact:	Martin Zaťovič <m.zatovic1@xxxxxxxxx>
> +Description:    Depends on format attribute - using one of the standard formats
> +        the payload_len attribute file becomes read-only and it contains the
> +        number of bits of a message without the checksum bits(e.g. 24 for
> +        26-bit format). Using a custom format makes this file writable for
> +        configuring the Wiegand message length in bits.
> +Users:		any user space application which wants to communicate using Wiegand
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ca063a504026..30aadc8e664d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22138,6 +22138,14 @@ L:	linux-rtc@xxxxxxxxxxxxxxx
>  S:	Maintained
>  F:	drivers/rtc/rtc-sd3078.c
> 
> +WIEGAND BUS DRIVER
> +M:	Martin Zaťovič <m.zatovic1@xxxxxxxxx>
> +S:	Maintained
> +F:	Documentation/ABI/testing/sysfs-bus-wiegand
> +F:	Documentation/devicetree/bindings/bus/wiegand.yaml
> +F:	drivers/bus/wiegand.c
> +F:	include/linux/wiegand.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..1b7dd3dcad72 100644
> --- a/drivers/bus/Kconfig
> +++ b/drivers/bus/Kconfig
> @@ -241,6 +241,13 @@ config VEXPRESS_CONFIG
>  	  Platform configuration infrastructure for the ARM Ltd.
>  	  Versatile Express.
> 
> +config WIEGAND
> +      tristate "Wiegand bus via GPIOs"
> +      depends on OF_GPIO
> +      help
> +        Wiegand Protocol is a low level 2-wire serial protocol. This
> +        enables the support of the bus.
> +
>  config DA8XX_MSTPRI
>  	bool "TI da8xx master peripheral priority driver"
>  	depends on ARCH_DAVINCI_DA8XX
> diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
> index d90eed189a65..ed64d1fde1b7 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)		+= wiegand.o
> 
>  obj-$(CONFIG_DA8XX_MSTPRI)	+= da8xx-mstpri.o
> 
> diff --git a/drivers/bus/wiegand.c b/drivers/bus/wiegand.c
> new file mode 100644
> index 000000000000..7de42e9f41eb
> --- /dev/null
> +++ b/drivers/bus/wiegand.c
> @@ -0,0 +1,509 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Wiegand write-only bus driver
> + *
> + * This driver implements a GPIOs bit-banged bus following the Wiegand
> + * protocol. It is used to communicate with various peripherals such as
> + * card readers or fingerprint sensors.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/poll.h>
> +#include <linux/miscdevice.h>
> +#include <linux/of.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/wiegand.h>
> +
> +struct wiegand_device *wiegand_glob;
> +struct wiegand_instance *wiegand_instance_glob;

No globals. How does it work with two Wiegand devices?

> +
> +struct wiegand_device {
> +	struct device *dev;
> +	struct miscdevice *misc_dev;
> +	struct mutex mutex;
> +	struct gpio_desc *gpio_data_hi;
> +	struct gpio_desc *gpio_data_lo;
> +	struct wiegand_setup setup;
> +	u8 data[WIEGAND_MAX_PAYLEN_BYTES];
> +};
> +
> +struct wiegand_instance {
> +	struct wiegand_device *dev;
> +	unsigned long flags;
> +};
> +
> +static const struct wiegand_setup WIEGAND_SETUP = {
> +	.pulse_len	= 50,
> +	.interval_len	= 2000,
> +	.frame_gap	= 2000,
> +	.format		= WIEGAND_V26,
> +	.payload_len	= 24,
> +};
> +
> +int wiegand_calc_parity8(u8 v)
> +{
> +	v = (v >> 4) ^ (v & ((1 << 4)-1));
> +	v = (v >> 2) ^ (v & ((1 << 2)-1));
> +	v = (v >> 1) ^ (v & ((1 << 1)-1));
> +	return v;
> +}
> +
> +void wiegand_add_parity_to_data(unsigned char *tmp, u8 *data,
> +						enum wiegand_format fmt)
> +{
> +	switch (fmt) {
> +	case WIEGAND_V26:
> +		data[0] = (tmp[0] >> 1) | (wiegand_calc_parity8(
> +						tmp[0] ^ (tmp[1] & 0xf0)) << 7);
> +		data[1] = (tmp[0] << 7) | (tmp[1] >> 1);
> +		data[2] = (tmp[1] << 7) | (tmp[2] >> 1);
> +		data[3] = (tmp[2] << 7) | (!wiegand_calc_parity8(
> +						(tmp[1] & 0x0f) ^ tmp[2]) << 6);
> +		break;
> +	case WIEGAND_V36:
> +		tmp[4] &= 0xc0;
> +
> +		data[0] = (tmp[0] >> 1) | (wiegand_calc_parity8(
> +				tmp[0] ^ tmp[1] ^ (tmp[2] & 0x80)) << 7);
> +		data[1] = (tmp[0] << 7) | (tmp[1] >> 1);
> +		data[2] = (tmp[1] << 7) | (tmp[2] >> 1);
> +		data[3] = (tmp[2] << 7) | (tmp[3] >> 1);
> +		data[4] = (tmp[3] << 7) | (tmp[4] >> 1) |
> +			(!wiegand_calc_parity8(
> +				(tmp[2] & 0x7f) ^ tmp[3] ^ tmp[4]) << 4);
> +		break;
> +	case WIEGAND_V37:
> +		tmp[4] &= 0xe0;
> +
> +		data[0] = (tmp[0] >> 1) | (wiegand_calc_parity8(
> +				tmp[0] ^ tmp[1] ^ (tmp[2] & 0xc0)) << 7);
> +		data[1] = (tmp[0] << 7) | (tmp[1] >> 1);
> +		data[2] = (tmp[1] << 7) | (tmp[2] >> 1);
> +		data[3] = (tmp[2] << 7) | (tmp[3] >> 1);
> +		data[4] = (tmp[3] << 7) | (tmp[4] >> 1) |
> +				(!wiegand_calc_parity8(
> +				(tmp[2] & 0x7f) ^ tmp[3] ^ tmp[4]) << 3);
> +		break;
> +	default:
> +		WARN_ON(fmt != WIEGAND_V37 &&
> +			fmt != WIEGAND_V36 &&
> +			fmt != WIEGAND_V26);
> +	}
> +}
> +
> +/*
> + * 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 ar the same time.
> + */
> +void wiegand_send_bit(struct wiegand_device *wiegand,
> +				bool value, bool last)
> +{
> +	struct wiegand_setup *setup = &wiegand->setup;
> +	struct gpio_desc *gpio = value ? wiegand->gpio_data_hi
> +				: wiegand->gpio_data_lo;
> +
> +	gpiod_set_value_cansleep(gpio, 0);
> +	udelay(setup->pulse_len);
> +	gpiod_set_value_cansleep(gpio, 1);
> +
> +	if (last)
> +		udelay(setup->frame_gap - setup->pulse_len);
> +	else
> +		udelay(setup->interval_len - setup->pulse_len);
> +}
> +
> +/* This function is used for custom format */
> +static int wiegand_write_by_bits(struct wiegand_device *wiegand,
> +				size_t len)
> +{
> +	size_t i, bitlen;
> +	bool bit_value, is_last_bit;
> +	struct wiegand_setup *setup = &wiegand->setup;
> +
> +	bitlen = setup->format ? setup->payload_len + 2 : setup->payload_len;
> +
> +	for (i = 0; i < bitlen; i++) {
> +		bit_value = ((wiegand->data[i / 8] >> (7 - (i % 8)))
> +									& 0x01);
> +		is_last_bit = (i + 1) == bitlen;
> +
> +		wiegand_send_bit(wiegand, (bool)bit_value,
> +				is_last_bit);
> +	}
> +
> +	return 0;
> +}
> +
> +static unsigned int wiegand_calc_bytes(unsigned int bits)
> +{
> +	if (bits % 8 != 0)
> +		return (bits / 8) + 1;
> +	else
> +		return bits / 8;
> +}
> +
> +static unsigned int wiegand_get_payload_size(
> +						unsigned long payload_len_bits,
> +						enum wiegand_format fmt)

Thisi does not look like wrapping in Linux coding style.

> +{
> +	unsigned int ret;
> +
> +	if (fmt == WIEGAND_CUSTOM)
> +		ret = wiegand_calc_bytes(payload_len_bits);
> +	else
> +		/* add 2 for parity bits */
> +		ret = wiegand_calc_bytes(payload_len_bits + 2);
> +
> +	return ret;
> +}
> +
> +static ssize_t wiegand_get_user_data(
> +				struct wiegand_device *wiegand,
> +				char __user const *buf, size_t len)
> +{
> +	size_t rc;
> +	size_t num_copy;
> +	unsigned char tmp[WIEGAND_MAX_PAYLEN_BYTES];
> +	struct wiegand_setup *setup = &wiegand->setup;
> +
> +	num_copy = wiegand_get_payload_size(setup->payload_len,
> +								setup->format);
> +
> +	if (setup->format == WIEGAND_CUSTOM) {
> +		rc = copy_from_user(&wiegand->data[0], buf, num_copy);
> +		if (rc < 0)
> +			return rc;
> +	} else {
> +		rc = copy_from_user(tmp, buf, num_copy);
> +		if (rc < 0)
> +			return rc;
> +		wiegand_add_parity_to_data(tmp, wiegand->data,
> +								setup->format);
> +	}
> +	return num_copy;
> +}
> +
> +static int wiegand_release(struct inode *ino, struct file *filp)
> +{
> +	struct wiegand_instance *info = filp->private_data;
> +	struct wiegand_device *wiegand = info->dev;
> +
> +	kfree(wiegand);
> +	kfree(info);

Double free. Test unbinding your driver - you will see it.

> +
> +	return 0;
> +}
> +
> +ssize_t wiegand_write(struct wiegand_device *wiegand, const char *buf, size_t len)
> +{
> +	int rc;
> +
> +	strcpy(wiegand->data, buf);
> +
> +	if (len * 8 < wiegand->setup.payload_len)
> +		return -EBADMSG;
> +	if (buf == NULL || len == 0)
> +		return -EINVAL;
> +
> +	rc = wiegand_write_by_bits(wiegand, len);
> +
> +	return len;
> +}
> +EXPORT_SYMBOL_GPL(wiegand_write);
> +
> +static ssize_t wiegand_write_from_file(struct file *filp,
> +		char __user const *buf, size_t len, loff_t *offset)
> +{
> +	struct wiegand_instance *info = filp->private_data;
> +	struct wiegand_device *wiegand = info->dev;
> +	int rc;
> +
> +	if (len * 8 < wiegand->setup.payload_len)
> +		return -EBADMSG;
> +	if (buf == NULL || len == 0)
> +		return -EINVAL;
> +
> +	wiegand_get_user_data(wiegand, buf, len);
> +	rc = wiegand_write_by_bits(wiegand, len);
> +
> +	return len;
> +}
> +
> +static int wiegand_open(struct inode *ino, struct file *filp)
> +{
> +	struct wiegand_device *wiegand;
> +	struct wiegand_instance *info;
> +	int rc;
> +
> +	wiegand = wiegand_glob;
> +
> +	mutex_lock(&wiegand->mutex);
> +
> +	if ((filp->f_flags & O_ACCMODE) == O_RDONLY ||
> +		(filp->f_flags & O_ACCMODE) == O_RDWR) {
> +		dev_err(wiegand->dev, "Device is write only\n");
> +		rc = -EIO;
> +		goto err;
> +	}
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info) {
> +		rc = -ENOMEM;
> +		goto err;
> +	}
> +
> +	gpiod_direction_output(wiegand->gpio_data_hi, 1);
> +	gpiod_direction_output(wiegand->gpio_data_lo, 1);
> +
> +	info->dev = wiegand;
> +	info->flags = filp->f_flags;
> +	wiegand_instance_glob = info;
> +
> +	mutex_unlock(&wiegand->mutex);
> +
> +	filp->private_data = info;
> +
> +	return 0;
> +err:
> +	mutex_unlock(&wiegand->mutex);
> +
> +	return rc;
> +}
> +
> +const struct file_operations wiegand_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= wiegand_open,
> +	.release	= wiegand_release,
> +	.write		= wiegand_write_from_file,
> +};
> +
> +static struct miscdevice wiegand_misc_device = {
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.name = "wiegand-bus",
> +	.fops = &wiegand_fops,
> +};
> +
> +static ssize_t store_uint(unsigned int *val, const char *buf,
> +				size_t size, unsigned long max)
> +{
> +	int ret;
> +	unsigned long new;
> +
> +	ret = kstrtoul(buf, 0, &new);
> +	if (ret)
> +		return ret;
> +	if (max != 0 && new > max)
> +		return -EINVAL;
> +
> +	*val = new;
> +	return size;
> +}
> +
> +ssize_t format_show(struct device *dev, struct device_attribute *attr,
> +		char *buf)
> +{
> +	struct wiegand_device *device = dev_get_drvdata(dev);
> +
> +	if (!device)
> +		return -ENODEV;
> +
> +	return sysfs_emit(buf, "%u\n", device->setup.format);
> +}
> +
> +/*
> + * To set a particular format, the number of bits the driver is supposed to
> + * transmit is written to the format sysfs file. For standard formats, the
> + * allowed inputs are 26, 36 and 37. To set a custom format, 0 is passed.
> + */
> +ssize_t format_store(struct device *dev, struct device_attribute *attr,
> +		const char *buf, size_t count)
> +{
> +	int ret;
> +	unsigned long new;
> +	enum wiegand_format *val;
> +	struct wiegand_device *device = dev_get_drvdata(dev);
> +
> +	if (!device)
> +		return -ENODEV;
> +
> +	val = &(device->setup.format);
> +
> +	ret = kstrtoul(buf, 0, &new);
> +	if (ret)
> +		return ret;
> +
> +	switch (new) {
> +	case 0:
> +		*val = WIEGAND_CUSTOM;
> +		break;
> +	case 26:
> +		*val = WIEGAND_V26;
> +		device->setup.payload_len = WIEGAND_V26_PAYLEN;
> +		break;
> +	case 36:
> +		*val = WIEGAND_V36;
> +		device->setup.payload_len = WIEGAND_V36_PAYLEN;
> +		break;
> +	case 37:
> +		*val = WIEGAND_V37;
> +		device->setup.payload_len = WIEGAND_V37_PAYLEN;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return count;
> +}
> +
> +void wiegand_set_format(struct wiegand_device *wiegand, enum wiegand_format fmt)
> +{
> +	wiegand->setup.format = fmt;
> +}
> +EXPORT_SYMBOL_GPL(wiegand_set_format);
> +
> +/*
> + * Using a custom format, the payload_len sysfs file configures the size of
> + * messages payload in bits. When one of the standard formats is used, this
> + * file is read-only and contains the size of the message in bits without the
> + * parity bits.
> + */
> +ssize_t payload_len_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct wiegand_device *device = dev_get_drvdata(dev);
> +
> +	if (!device)
> +		return -ENODEV;
> +
> +	return sysfs_emit(buf, "%u\n", device->setup.pulse_len);
> +}
> +
> +ssize_t payload_len_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct wiegand_device *device = dev_get_drvdata(dev);
> +
> +	if (!device)
> +		return -ENODEV;
> +
> +	/* standard formats use fixed payload size */
> +	if (device->setup.format != WIEGAND_CUSTOM)
> +		return -EPERM;
> +
> +	return store_uint(&(device->setup.payload_len), buf, count,
> +					WIEGAND_MAX_PAYLEN_BYTES * 8);
> +}
> +
> +void wiegand_set_payload_len(struct wiegand_device *wiegand, u32 paylen)
> +{
> +	wiegand->setup.payload_len = paylen;
> +}
> +EXPORT_SYMBOL_GPL(wiegand_set_payload_len);
> +
> +DEVICE_ATTR_RW(format);
> +DEVICE_ATTR_RW(payload_len);
> +
> +static struct attribute *wiegand_attrs[] = {
> +	&dev_attr_format.attr,
> +	&dev_attr_payload_len.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group wiegand_group = {
> +	.name = "wiegand-attributes",
> +	.attrs = wiegand_attrs,
> +};
> +
> +static int wiegand_dev_probe(struct platform_device *device)
> +{
> +	int rc;
> +	struct wiegand_device *wiegand;
> +
> +	wiegand = devm_kzalloc(&device->dev, sizeof(*wiegand), GFP_KERNEL);
> +	if (!wiegand)
> +		return -ENOMEM;
> +
> +	wiegand->dev = &device->dev;
> +	wiegand->misc_dev = &wiegand_misc_device;
> +
> +	wiegand_glob = wiegand;
> +
> +	memcpy(&wiegand->setup, &WIEGAND_SETUP,
> +			sizeof(struct wiegand_setup));

Mixed up indentation. In other places as well.


> +
> +	/* Get GPIO lines using device tree bindings. */

Comment is actually confusing... It will work on non OF platforms as well.

> +	wiegand->gpio_data_lo = devm_gpiod_get(wiegand->dev,
> +			"data-lo", GPIOD_OUT_HIGH);
> +	if (IS_ERR(wiegand->gpio_data_lo)) {
> +		dev_err(wiegand->dev,
> +			"Failed to get data-lo pin.\n");
> +		return PTR_ERR(wiegand->gpio_data_lo);
> +	}
> +	wiegand->gpio_data_hi = devm_gpiod_get(wiegand->dev,
> +			"data-hi", GPIOD_OUT_HIGH);
> +	if (IS_ERR(wiegand->gpio_data_hi)) {
> +		dev_err(wiegand->dev,
> +			"Failed to get data-hi pin.\n");
> +		return PTR_ERR(wiegand->gpio_data_hi);
> +	}
> +
> +	of_property_read_u32(wiegand->dev->of_node, "pulse-len",
> +					&wiegand->setup.pulse_len);

Use device_property*

> +	of_property_read_u32(wiegand->dev->of_node, "interval-len", &wiegand->setup.interval_len);
> +	of_property_read_u32(wiegand->dev->of_node, "frame-gap", &wiegand->setup.frame_gap);
> +
> +	rc = sysfs_create_group(&wiegand->dev->kobj, &wiegand_group);
> +	if (rc < 0) {
> +		dev_err(wiegand->dev, "Couldn't register sysfs group\n");

return dev_err_probe
In other palces as well

> +		return rc;
> +	}
> +
> +	mutex_init(&wiegand->mutex);
> +
> +	rc = misc_register(&wiegand_misc_device);
> +	dev_info(wiegand->dev, "miscdevice prob registered, rc=%d\n", rc);

No need to print probe successes for regular drivers (and you created a
regular driver, not a bus).

> +	wiegand->misc_dev->parent = wiegand->dev;
> +
> +	dev_set_drvdata(&device->dev, wiegand);
> +
> +	return 0;
> +}
> +
> +static int wiegand_dev_remove(struct platform_device *device)
> +{
> +	struct wiegand_device *wiegand = dev_get_drvdata(&device->dev);
> +
> +	sysfs_remove_group(&wiegand->dev->kobj, &wiegand_group);
> +
> +	misc_deregister(&wiegand_misc_device);
> +	kfree(wiegand_instance_glob);
> +	kfree(wiegand);

Double free. Please test your exit and error paths.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id wiegand_dt_idtable[] = {
> +	{ .compatible = "wiegand", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, wiegand_dt_idtable);
> +
> +static struct platform_driver wiegand_driver = {
> +	.probe		= wiegand_dev_probe,
> +	.remove		= wiegand_dev_remove,
> +	.driver		= {
> +		.name		= "wiegand",
> +		.of_match_table = wiegand_dt_idtable,

This is a driver, not a bus... Either you create a bus or a device
driver. What is here is a device driver called bus...

Best regards,
Krzysztof