Re: [PATCHv4 4/4] wiegand: add Wiegand GPIO bitbanged controller driver

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, May 10, 2023 at 06:22:43PM +0200, Martin Zaťovič wrote:
> This controller formats the data to a Wiegand format and bit-bangs
> the message on devicetree defined GPIO lines.
> 
> Several attributes need to be defined in the devicetree in order
> for this driver to work, namely the data-hi-gpios, data-lo-gpios,
> pulse-len, frame-gap and interval-len. These attributes are
> documented in the devicetree bindings documentation files.
> 
> The driver creates a dev file for writing messages on the bus.
> It also creates a sysfs file to control the payload length of
> messages(in bits). If a message is shorter than the set payload
> length, it will be discarded. On the other hand, if a message is
> longer, the additional bits will be stripped off.

...

> +Date:		May 2023

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

...

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

...

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

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

...

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

ERANGE?

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

...

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

NIH fsleep()

...

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

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

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

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

	unsigned int i;
	bool value;

	if (bitlen == 0)
		return 0;

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

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

> +	return 0;
> +}

...

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

Maybe you can use devm_gpiod_get_array()?

> +}

...

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

> +	int status = 0;

Redundant assignment.

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

...

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

Instead of comment, make a self-explanatory definition?

...

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

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

> +	return status;

With above

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

	return 0;

> +}

...

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

Inner comma is not needed.

> +	{}
> +};

...

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

Leave trailing comma when it's not about termination.

> +	},
> +	.probe		= wiegand_gpio_probe

Ditto.

> +};

-- 
With Best Regards,
Andy Shevchenko