| Message ID | 20230510162243.95820-1-m.zatovic1@gmail.com |
|---|---|
| Headers | show |
| Series | Wiegand bus driver and GPIO bitbanged Wiegand | expand |
On Wed, May 10, 2023 at 06:22:39PM +0200, Martin Zaťovič wrote: > Hello, > > thank you for the feedback regarding the previous version of this patch series. > I have tried to follow all of the advice I got and fix all the pointed issues. > One of the main issues was the usage of of API for device registration. This > has now been fixed to use fwnode API, however I was not yet able to get rid of > the of_device include, since it is required for of_driver_match_device. Please > let me know if this is correct. Since it is a bus, I think we need that. > CHANGELOG: > > wiegand.c: > - changed ID allocation API from IDR to IDA, since the references associated to > the IDs are not needed > - removed the board_lock mutex, because it was only guarding the allocacion > and freeing of IDs, which is already supported by IDA API > - restructured the file, so that most functions are close to their caller, or > defined them at the top for better code readability > - in the function devm_wiegand_register_controller, the devres management of > the pointer to wiegand_controller structure has been replaced with > devm_add_action_or_reset function. It was intended to do the same with > devm_wiegand_alloc_controller, however, the kernel kept panicing, despite the > call order of the unregister and release functions being proper(same as with > devres managed pointer). Please let me know if this is an absolute must, if so > I will look into it further. What panic? Can you elaborate? > - moved the miscdevice from wiegand-gpio driver to be a part of the bus > driver. Now every controller is associated a /dev file. The file operation > functions were simply moved and renamed and the miscdevice structure was moved > to be a part of wiegand_controller structure > - since now every controller has a miscdevice assosciated, the data_buffer was > also moved to be a part of the controller structure, and it was made a bitmap > - used fwnode API for device registration instead of of API > - removed warnings when driver fails to get wiegand properties, instead > implemented mechanism for setting a default value similar I2C > - removed the driver matching code in register driver, as > of_driver_match_device does that already > - made wiegand_device and opaque pointer > - changed the terminology to primary and secondary
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. > +};