Messages in this thread | | | Date | Thu, 22 Jun 2023 16:39:43 +0300 | From | Andy Shevchenko <> | 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
| |