On Thu, Aug 24, 2023 at 01:10:13PM +0200, Martin Zaťovič wrote: > The Wiegand protocol serves as a standardized interface protocol, extensively > employed within electronic access control systems, to facilitate data exchange > between credentials, readers, and door controllers. Its inception can be > attributed to the widespread adoption of Wiegand card readers, leveraging the > Wiegand effect - a physical phenomenon wherein a Wiegand wire (or card) > generates small yet distinct magnetic fields. A Wiegand reader detects the > magnetic pulses emitted by the internal wires of the card. > > Three wires are central to the Wiegand protocol: a common ground wire and two > distinct data wires designated as DATA0 and DATA1. During periods of inactivity, > both DATA0 and DATA1 lines remain pulled up. For transmitting a '0,' the DATA0 > line is pulled down while DATA1 remains pulled up; conversely, transmitting > a '1' causes DATA1 to be pulled down while DATA0 remains pulled up. Notably, > this protocol ensures that the two lines never simultaneously experience a low > state. > > Timing characteristics within the Wiegand protocol lack a uniform > standardization, introducing variability between devices. Generally, pulse > durations hover between 50 to 100 microseconds, while inter-pulse gaps span 20 > to 100 milliseconds. There is no stop bit or similar delimiter to signal the > conclusion of a message. Instead, the receiver either counts the bits within the > message or enforces a timeout, often set at around ten times the inter-pulse gap > duration. > > The 26-Bit Wiegand Interface Standard, or 26-Bit Wiegand Format outlines the > message format most commonly used among Wiegand devices. This format allocates > the initial and terminal bits for parity. The subsequent eight bits following > the initial parity bit are reserved for the Facility Code designed for distinct > location identification. The remaining bits house the unique ID number. As the > technology evolved, new Wiegand formats emerged, including 36-bit and 37-bit > formats. It was also common practice for manufacturers to engineer devices > compatible with proprietary Wiegand formats tailored to their specifications. > > The Wiegand bus driver handles Wiegand controller and Wiegand device > managemement and driver matching. The bus driver defines the structures for > Wiegand controllers and Wiegand devices. Wiegand controller structure contains > all attributes that define the communication such as the payload_len for > configuring the size of a single Wiegand message in bits, thus choosing a > format. Each Wiegand controller should be associated with one Wiegand device, > as Wiegand is typically a point-to-point bus. ... > +#include <linux/bitops.h> Should be bitmap.h > +#include <linux/cdev.h> > +#include <linux/container_of.h> > +#include <linux/device.h> > +#include <linux/dmaengine.h> > +#include <linux/dma-mapping.h> > +#include <linux/module.h> mutex.h ? > +#include <linux/of_device.h> > +#include <linux/property.h> > +#include <linux/slab.h> > +#include <linux/types.h> > +#include <linux/wiegand.h> ... > +static ssize_t wiegand_fwrite(struct file *filp, char __user const *buf, size_t len, > + loff_t *offset) > +{ > + int ret; > + struct wiegand_controller *ctlr = filp->private_data; > + > + mutex_lock(&ctlr->file_lock); > + > + if (!buf || len == 0) Ouch?! To avoid this, you can use cleanup.h and guard your functions with a lock, moreover in this case the check is not even needed to be performed under the lock. > + return -EINVAL; > + > + ret = wiegand_get_user_data(ctlr, buf, len); > + if (ret < 0) > + return ret; > + > + ctlr->transfer_message(ctlr); > + > + mutex_unlock(&ctlr->file_lock); > + return len; > +} ... > +static int wiegand_fopen(struct inode *ino, struct file *filp) > +{ > + int ret; > + struct wiegand_controller *ctlr = container_of(filp->f_op, struct wiegand_controller, fops); > + > + filp->private_data = ctlr; > + > + mutex_lock(&ctlr->file_lock); Do you care about the call being interrupted by a signal? Ditto for other system calls in the framework. > + if ((filp->f_flags & O_ACCMODE) == O_RDONLY || (filp->f_flags & O_ACCMODE) == O_RDWR) { > + dev_err(ctlr->miscdev.this_device, "device is write only\n"); > + ret = -EIO; > + goto out_unlock; > + } > + > + mutex_unlock(&ctlr->file_lock); > + return 0; > + > +out_unlock: > + mutex_unlock(&ctlr->file_lock); > + return ret; > +} ... > + size_t ctlr_size = ALIGN(sizeof(*ctlr), dma_get_cache_alignment()); Yeah, yet another user for a new macro in overflow.h (not yet there, though). > + size_t total_size; > + > + if (check_add_overflow(size, ctlr_size, &total_size)) > + return NULL; > + > + ctlr = kzalloc(total_size, GFP_KERNEL); > + if (!ctlr) > + return NULL; ... > +struct wiegand_controller *devm_wiegand_alloc_controller(struct device *dev, unsigned int size, > + bool secondary) > +{ > + struct wiegand_controller *ctlr; > + > + ctlr = wiegand_alloc_controller(dev, size, secondary); > + if (ctlr) > + ctlr->devm_allocated = true; > + else > + return NULL; if (!ctrl) return NULL; or maybe return ERR_PTR(-ENOMEM); > + if (devm_add_action_or_reset(dev, wiegand_controller_put, ctlr)) > + return NULL; ret = ... if (ret) return ERR_PTR(ret); ? > + > + return ctlr; ... > +/** > + * register_wiegand_device - allocates and registers a new Wiegand device > + * > + * @ctlr: controller structure to attach device to > + * @node: firmware node for the device Run scripts/kernel-doc -v -none -Wall ... against this file and fix all warnings. > + */ > + struct fwnode_handle *node) Usually we call it fwnode. ... > + device_set_node(&wiegand->dev, node); device_set_node(&wiegand->dev, fwnode_handle_get(fwnode)); > +out_node_put: > + fwnode_handle_put(node); Hmm... Shouldn't we do this at the ->release()? > + put_device(&wiegand->dev); > + wiegand_cleanup(wiegand); > + return ERR_PTR(ret); > +} ... > + struct fwnode_handle *node; fwnode here and everywhere else. ... > + fwnode_for_each_available_child_node(ctlr->dev.fwnode, node) { device_for_each_child_node() > + wiegand = register_wiegand_device(ctlr, node); > + if (IS_ERR(wiegand)) > + dev_warn(&ctlr->dev, "failed to create wiegand device for %pfwf\n", node); > + } ... > + struct device *ctlr_dev = &ctlr->dev; Just name it "dev". You can use similar approach in another functions, like the above, to make them look nicer. ... > + ret = device_property_read_u32(ctlr_dev, "pulse-len-us", &pulse_len); > + if (!ret && pulse_len > 0) > + timing->pulse_len = pulse_len; > + else > + timing->pulse_len = WIEGAND_DEFAULT_PULSE_LEN; What about /* Use default if property is absent or can't be read */ pulse_len = WIEGAND_DEFAULT_PULSE_LEN; device_property_read_u32(dev, "pulse-len-us", &pulse_len); timing->pulse_len = pulse_len; Wrong values should be caught up by DT schema linter, no? If the parameters are 0 it's not your problem, although you can warn. I'm not sure about this, so your current variant is okay. Ditto for the rest. ... > +static int __unregister(struct device *dev, void *null) It is recommended to use namespace even if it's static function. ... > +void wiegand_unregister_controller(void *ptr) > +{ Why the parameter is not properly typed? Yes for devm you probably need a wrapper static devm_..._unregister_...(void *ctrl) { wiegand_unregister_controller(ctrl) } For the exported _unregister, taking into account the possible optional support for this, you may need to check the parameter to be valid. if (IS_ERR_OR_NULL(...)) // _OR_NULL probably is not needed as per above return; > +} > + status = misc_register(&ctlr->miscdev); > + if (status) { > + dev_err(&ctlr->dev, "couldn't register misc device\n"); > + goto out_free_ida_name; > + } > + mutex_init(&ctlr->file_lock); Shouldn't it be needed to be initialized before device itself? ... > + status = device_add(&ctlr->dev); > + if (status < 0) Don't we need to call something like device_del() or put_device() at this point? OK, read doc for device_add() it clarifies this. > + goto out_free_ida_name_misc; ... > + status = __wiegand_add_device(wiegand); > + if (!status) { > + ctlr->device_count++; > + wiegand->id = wiegand->controller->device_count; > + } > + > + return status; if (status) return status; ... return 0; ... > +void wiegand_unregister_device(struct wiegand_device *wiegand) > +{ > + if (!wiegand) IS_ERR_OR_NULL() or alike (see above) > + return; > + > + fwnode_handle_put(wiegand->dev.fwnode); No, do not dereference fwnode in struct device, always use proper API and/or dev_fwnode() accessor. > + device_del(&wiegand->dev); > + wiegand_cleanup(wiegand); > + put_device(&wiegand->dev); > +} ... > +static int wiegand_match_device(struct device *dev, struct device_driver *drv) > +{ > + struct wiegand_device *wiegand_dev = to_wiegand_device(dev); > + > + if (of_driver_match_device(dev, drv)) > + return 1; I would add ACPI support as well as if (acpi_driver_match_device(dev, drv)) return 1; > + return strcmp(wiegand_dev->modalias, drv->name) == 0; > +} ... > +static int __init wiegand_init(void) > +{ > + int ret; > + > + ret = bus_register(&wiegand_bus_type); > + if (ret < 0) > + pr_err("Wiegand bus registration failed: %d\n", ret); pr_fmt() may help to have unified prefix for all messages printed with help of pr_*() macros. > + return ret; > +} ... > +#define WIEGAND_MAX_PAYLEN_BYTES 256 I don't see you use BYTES, but rather BITS. Can you define _BITS instead? ... > +struct wiegand_device; + Blank line. > +/** > + * struct wiegand_timing - Wiegand timings in microseconds Perhaps timings ? > + * @pulse_len: length of the low pulse > + * @interval_len: length of a whole bit (both the pulse and the high phase) > + * @frame_gap: length of the last bit of a frame (both the pulse and the high phase) > + */ ... > + struct wiegand_timing timing; Perhaps timings ? ... > +#endif /* H_LINUX_INCLUDE_LINUX_WIEGAND_H */ Leading H is something unusual? -- With Best Regards, Andy Shevchenko