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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZOdciyyM4/BYxXL9@smile.fi.intel.com>
Date:   Thu, 24 Aug 2023 16:35:07 +0300
From:   Andy Shevchenko <andriy.shevchenko@...el.com>
To:     Martin Zaťovič <m.zatovic1@...il.com>
Cc:     linux-kernel@...r.kernel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        gregkh@...uxfoundation.org, linus.walleij@...aro.org,
        quic_jhugo@...cinc.com, nipun.gupta@....com, tzimmermann@...e.de,
        ogabbay@...nel.org, mathieu.poirier@...aro.org, axboe@...nel.dk,
        damien.lemoal@...nsource.wdc.com, linux@...y.sk, arnd@...db.de,
        yangyicong@...ilicon.com, benjamin.tissoires@...hat.com,
        masahiroy@...nel.org, jacek.lawrynowicz@...ux.intel.com,
        geert+renesas@...der.be, devicetree@...r.kernel.org
Subject: Re: [PATCHv5 4/4] wiegand: add Wiegand GPIO bitbanged controller
 driver

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