mbox series

[PATCHv4,0/4] Wiegand bus driver and GPIO bitbanged Wiegand

Message ID 20230510162243.95820-1-m.zatovic1@gmail.com
Headers show
Series Wiegand bus driver and GPIO bitbanged Wiegand | expand

Message

Martin Zaťovič May 10, 2023, 4:22 p.m. UTC
Hello,

thank you for the feedback regarding the previous version of this patch series.
I have tried to follow all of the advice I got and fix all the pointed issues.
One of the main issues was the usage of of API for device registration. This
has now been fixed to use fwnode API, however I was not yet able to get rid of
the of_device include, since it is required for of_driver_match_device. Please
let me know if this is correct.

CHANGELOG:

wiegand.c:
- changed ID allocation API from IDR to IDA, since the references associated to
the IDs are not needed
- removed the board_lock mutex, because it was only guarding the allocacion
and freeing of IDs, which is already supported by IDA API
- restructured the file, so that most functions are close to their caller, or
defined them at the top for better code readability
- in the function devm_wiegand_register_controller, the devres management of
the pointer to wiegand_controller structure has been replaced with
devm_add_action_or_reset function. It was intended to do the same with
devm_wiegand_alloc_controller, however, the kernel kept panicing, despite the
call order of the unregister and release functions being proper(same as with
devres managed pointer). Please let me know if this is an absolute must, if so
I will look into it further.
- moved the miscdevice from wiegand-gpio driver to be a part of the bus
driver. Now every controller is associated a /dev file. The file operation
functions were simply moved and renamed and the miscdevice structure was moved
to be a part of wiegand_controller structure
- since now every controller has a miscdevice assosciated, the data_buffer was
also moved to be a part of the controller structure, and it was made a bitmap
- used fwnode API for device registration instead of of API
- removed warnings when driver fails to get wiegand properties, instead
implemented mechanism for setting a default value similar I2C
- removed the driver matching code in register driver, as
of_driver_match_device does that already
- made wiegand_device and opaque pointer
- changed the terminology to primary and secondary

wiegand.h
- reordered the wiegand_driver structure so that miscdevice is first

wiegand-gpio.c
- removed of.h include
- changed udelay to usleep_range for longer wait times in wiegand_gpio_send_bit
- moved wiegand_gpio_groups to dev groups of the struct driver
- style changes(no commas for terminator entries, removed redundant blank
lines etc.)
- changed the names of gpio_descs from gpio_data_hi and gpio_data_lo to
data1_gpio and data0_gpio

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

 .../ABI/testing/sysfs-driver-wiegand-gpio     |   9 +
 .../bindings/wiegand/wiegand-controller.yaml  |  39 ++
 .../bindings/wiegand/wiegand-gpio.yaml        |  51 ++
 MAINTAINERS                                   |  14 +
 drivers/Kconfig                               |   2 +
 drivers/Makefile                              |   1 +
 drivers/wiegand/Kconfig                       |  28 +
 drivers/wiegand/Makefile                      |   2 +
 drivers/wiegand/wiegand-gpio.c                | 189 ++++++
 drivers/wiegand/wiegand.c                     | 609 ++++++++++++++++++
 include/linux/wiegand.h                       | 144 +++++
 11 files changed, 1088 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-wiegand-gpio
 create mode 100644 Documentation/devicetree/bindings/wiegand/wiegand-controller.yaml
 create mode 100644 Documentation/devicetree/bindings/wiegand/wiegand-gpio.yaml
 create mode 100644 drivers/wiegand/Kconfig
 create mode 100644 drivers/wiegand/Makefile
 create mode 100644 drivers/wiegand/wiegand-gpio.c
 create mode 100644 drivers/wiegand/wiegand.c
 create mode 100644 include/linux/wiegand.h

Comments

Andy Shevchenko June 22, 2023, 12:36 p.m. UTC | #1
On Wed, May 10, 2023 at 06:22:39PM +0200, Martin Zaťovič wrote:
> Hello,
> 
> thank you for the feedback regarding the previous version of this patch series.
> I have tried to follow all of the advice I got and fix all the pointed issues.
> One of the main issues was the usage of of API for device registration. This
> has now been fixed to use fwnode API, however I was not yet able to get rid of
> the of_device include, since it is required for of_driver_match_device. Please
> let me know if this is correct.

Since it is a bus, I think we need that.

> CHANGELOG:
> 
> wiegand.c:
> - changed ID allocation API from IDR to IDA, since the references associated to
> the IDs are not needed
> - removed the board_lock mutex, because it was only guarding the allocacion
> and freeing of IDs, which is already supported by IDA API
> - restructured the file, so that most functions are close to their caller, or
> defined them at the top for better code readability

> - in the function devm_wiegand_register_controller, the devres management of
> the pointer to wiegand_controller structure has been replaced with
> devm_add_action_or_reset function. It was intended to do the same with
> devm_wiegand_alloc_controller, however, the kernel kept panicing, despite the
> call order of the unregister and release functions being proper(same as with
> devres managed pointer). Please let me know if this is an absolute must, if so
> I will look into it further.

What panic? Can you elaborate?

> - moved the miscdevice from wiegand-gpio driver to be a part of the bus
> driver. Now every controller is associated a /dev file. The file operation
> functions were simply moved and renamed and the miscdevice structure was moved
> to be a part of wiegand_controller structure
> - since now every controller has a miscdevice assosciated, the data_buffer was
> also moved to be a part of the controller structure, and it was made a bitmap
> - used fwnode API for device registration instead of of API
> - removed warnings when driver fails to get wiegand properties, instead
> implemented mechanism for setting a default value similar I2C
> - removed the driver matching code in register driver, as
> of_driver_match_device does that already
> - made wiegand_device and opaque pointer
> - changed the terminology to primary and secondary
Andy Shevchenko June 22, 2023, 1:39 p.m. UTC | #2
On Wed, May 10, 2023 at 06:22:43PM +0200, Martin Zaťovič wrote:
> This controller formats the data to a Wiegand format and bit-bangs
> the message on devicetree defined GPIO lines.
> 
> Several attributes need to be defined in the devicetree in order
> for this driver to work, namely the data-hi-gpios, data-lo-gpios,
> pulse-len, frame-gap and interval-len. These attributes are
> documented in the devicetree bindings documentation files.
> 
> The driver creates a dev file for writing messages on the bus.
> It also creates a sysfs file to control the payload length of
> messages(in bits). If a message is shorter than the set payload
> length, it will be discarded. On the other hand, if a message is
> longer, the additional bits will be stripped off.

...

> +Date:		May 2023

Taking into account the estimated release date I think this should be changed
to Aug 2023.

...

> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/poll.h>
> +#include <linux/slab.h>
> +#include <linux/wiegand.h>

...

> +#define UP_TO_100_USEC_DEVIATION 1
> +#define MORE_THAN_100_USEC_DEVIATION 3

These require some comments. And maybe better naming (depending on the content
of those comments).

...

> +static ssize_t store_ulong(u32 *val, const char *buf, size_t size, unsigned long max)
> +{
> +	int rc;
> +	u32 new;
> +
> +	rc = kstrtou32(buf, 0, &new);
> +	if (rc)
> +		return rc;
> +
> +	if (new > max)
> +		return -EINVAL;

ERANGE?

> +	*val = new;
> +	return size;
> +}

...

> +	if (sleep_len < 10)
> +		udelay(sleep_len);
> +	else if (sleep_len < 100)
> +		usleep_range(sleep_len - UP_TO_100_USEC_DEVIATION,
> +			     sleep_len + UP_TO_100_USEC_DEVIATION);
> +	else
> +		usleep_range(sleep_len - MORE_THAN_100_USEC_DEVIATION,
> +			     sleep_len + MORE_THAN_100_USEC_DEVIATION);

NIH fsleep()

...

> +static int wiegand_gpio_write_by_bits(struct wiegand_gpio *wiegand_gpio, u16 bitlen)
> +{
> +	size_t i;
> +	bool bit_value, is_last_bit;
> +
> +	for (i = 0; i < bitlen; i++) {
> +		bit_value = test_bit(i, wiegand_gpio->ctlr->data_bitmap);

> +		is_last_bit = (i + 1) == bitlen;

This is idempotent from for-loop, so...

> +		wiegand_gpio_send_bit(wiegand_gpio, bit_value, is_last_bit);
> +	}

	unsigned int i;
	bool value;

	if (bitlen == 0)
		return 0;

	for (i = 0; i < bitlen - 1; i++) {
		value = test_bit(i, wiegand_gpio->ctlr->data_bitmap);
		wiegand_gpio_send_bit(wiegand_gpio, value, false);
	}

	value = test_bit(bitlen - 1, wiegand_gpio->ctlr->data_bitmap);
	wiegand_gpio_send_bit(wiegand_gpio, value, true);

> +	return 0;
> +}

...

> +static int wiegand_gpio_request(struct device *dev, struct wiegand_gpio *wiegand_gpio)
> +{
> +	wiegand_gpio->data0_gpio = devm_gpiod_get(dev, "data-lo", GPIOD_OUT_HIGH);
> +	if (IS_ERR(wiegand_gpio->data0_gpio))
> +		return PTR_ERR(wiegand_gpio->data0_gpio);
> +
> +	wiegand_gpio->data1_gpio = devm_gpiod_get(dev, "data-hi", GPIOD_OUT_HIGH);
> +	return PTR_ERR_OR_ZERO(wiegand_gpio->data1_gpio);

Maybe you can use devm_gpiod_get_array()?

> +}

...

> +static int wiegand_gpio_probe(struct platform_device *device)
> +{

> +	int status = 0;

Redundant assignment.

> +	struct wiegand_controller *primary;
> +	struct wiegand_gpio *wiegand_gpio;
> +	struct device *dev = &device->dev;

...

> +	primary->payload_len = 26; // set standard 26-bit format

Instead of comment, make a self-explanatory definition?

...

> +	status = wiegand_register_controller(primary);
> +	if (status)
> +		dev_err_probe(wiegand_gpio->dev, status, "failed to register primary\n");

Why out of a sudden it uses this device and not real one?

> +	return status;

With above

		return dev_err_probe(dev, status, "failed to register primary\n");

	return 0;

> +}

...

> +static const struct of_device_id wiegand_gpio_dt_idtable[] = {
> +	{ .compatible = "wiegand-gpio", },

Inner comma is not needed.

> +	{}
> +};

...

> +static struct platform_driver wiegand_gpio_driver = {
> +	.driver = {
> +		.name	= "wiegand-gpio",
> +		.of_match_table = wiegand_gpio_dt_idtable,
> +		.dev_groups = wiegand_gpio_groups

Leave trailing comma when it's not about termination.

> +	},
> +	.probe		= wiegand_gpio_probe

Ditto.

> +};