| 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
| ||
|
Date: Thu, 22 Jun 2023 16:39:43 +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, beanhuo@...ron.com,
nipun.gupta@....com, linus.walleij@...aro.org, mwen@...lia.com,
bvanassche@....org, arnd@...db.de, ogabbay@...nel.org,
linux@...y.sk, jacek.lawrynowicz@...ux.intel.com,
geert+renesas@...der.be, benjamin.tissoires@...hat.com,
masahiroy@...nel.org, yangyicong@...ilicon.com,
devicetree@...r.kernel.org
Subject: Re: [PATCHv4 4/4] wiegand: add Wiegand GPIO bitbanged controller
driver
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
Powered by blists - more mailing lists