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

[Andy Shevchenko]

On Thu, Aug 24, 2023 at 01:10:15PM +0200, Martin Zaťovič wrote:
> This controller formats the data to a Wiegand format and bit-bangs
> the message on devicetree defined GPIO lines.
> 
> 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:		August 2023

Unrealistic. Use https://hansen.beer/~dave/phb/ to define Date and
KernelVersion fields.

...

> +#define WIEGAND_DEFAULT_PAYLOAD_LEN 26

_LEN --> _BITS

...

> +	return store_ulong(&(ctlr->payload_len), buf, count, WIEGAND_MAX_PAYLEN_BYTES * 8);

Yeah, again use _BITS directly.

...

> +/**
> + * wiegand_gpio_send_bit - Sends a single bit

Do not forget to validate this with scripts/kernel-doc.

> + * @wiegand_gpio: Instance of the Wiegand
> + * @value: Bit value to send
> + * @last: Indicates last bit of a message
> + *
> + * 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.
> + */

...

> +	struct gpio_desc *gpio = value ? wiegand_gpio->gpios->desc[1] :
> +						wiegand_gpio->gpios->desc[0];

Make it separate line.

	gpio = value ? wiegand_gpio->gpios->desc[1] : wiegand_gpio->gpios->desc[0];

...

> +	wiegand_gpio->gpios = devm_gpiod_get_array(dev, "data", GPIOD_OUT_HIGH);

> +

Redundant blank line.

> +	if (IS_ERR(wiegand_gpio->gpios))
> +		dev_err(dev, "unable to get gpios\n");
> +
> +	return PTR_ERR_OR_ZERO(wiegand_gpio->gpios);

Oh.

	wiegand_gpio->gpios = devm_gpiod_get_array(dev, "data", GPIOD_OUT_HIGH);
	ret = PTR_...();
	if (ret)
		dev_err();

	return ret;

...

> +static int wiegand_gpio_probe(struct platform_device *device)

	device --> pdev

...

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

Inner comma is not needed.

> +	{}
> +};

-- 
With Best Regards,
Andy Shevchenko