Messages in this thread | | | Date | Thu, 22 Jun 2023 16:20:29 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCHv4 2/4] wiegand: add Wiegand bus driver |
| |
On Wed, May 10, 2023 at 06:22:41PM +0200, Martin Zaťovič wrote:
This needs much more work. See my comments below.
> Add a bus driver for Wiegand protocol. The 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 represents a master and contains > attributes such as the payload_len for configuring the size > of a single Wiegand message in bits. It also stores the > controller attributes defined in the devicetree. > > Each Wiegand controller should be associated with one Wiegand > device, as Wiegand is typically a point-to-point bus.
Any chance to have a Datasheet: (or Link:) tag to point to the protocol specifications?
...
> +config WIEGAND > + tristate "Wiegand Bus driver" > + help > + The "Wiegand Interface" is an asynchronous low-level protocol > + or wiring standard. It is typically used for point-to-point > + communication. The data length of Wiegand messages is not defined, > + so the devices usually default to 26, 36 or 37 bits per message. > + The throughput of Wiegand depends on the selected pulse length and > + the intervals between pulses, in comparison to other busses it > + is generally rather slow. > + > + Despite its higher age, Wiegand remains widely used in access > + control systems to connect a card swipe mechanism. Such mechanisms > + utilize the Wiegand effect to transfer data from the card to > + the reader. > + > + Wiegand uses two wires to transmit the data D0 and D1. Both lines > + are initially pulled up. When a bit of value 0 is being transmitted, > + the D0 line is pulled down. Similarly, when a bit of value 1 is being > + transmitted, the D1 line is pulled down.
&You have an indentation issues in all lines above, except the first one.
Should be
config ... <TAB>tristate <TAB>help <TAB><sp><sp>help text
<TAB> — tabulation <sp> — space
...
> +#include <linux/cdev.h>
+ container_of.h
> +#include <linux/device.h> > +#include <linux/dmaengine.h> > +#include <linux/dma-mapping.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/property.h> > +#include <linux/slab.h>
+ types.h
> +#include <linux/wiegand.h>
...
> +/** > + * struct wiegand_device - Wiegand listener device > + * @dev - drivers structure of the device > + * @id - unique device id > + * @controller - Wiegand controller associated with the device > + * @modalias - Name of the driver to use with this device, or its alias. > + */ > +struct wiegand_device { > + struct device dev; > + u8 id; > + struct wiegand_controller *controller; > + char modalias[WIEGAND_NAME_SIZE]; > +};
> +DEFINE_IDA(wiegand_controller_ida);
Why not static?
> +static inline void wiegand_device_put(struct wiegand_device *wiegand); > +static inline struct wiegand_device *to_wiegand_device(struct device *dev); > + > +static int wiegand_fopen(struct inode *ino, struct file *filp); > +static int wiegand_frelease(struct inode *ino, struct file *filp); > +static ssize_t wiegand_fwrite(struct file *filp, char __user const *buf, size_t len, > + loff_t *offset);
Why do you need forward declarations?
...
> +struct wiegand_controller *wiegand_alloc_controller(struct device *dev, unsigned int size, > + bool secondary) > +{ > + struct wiegand_controller *ctlr; > + size_t ctlr_size = ALIGN(sizeof(*ctlr), dma_get_cache_alignment());
> + if (!dev) > + return NULL;
Hmm... Why this check is needed?
> + ctlr = kzalloc(size + ctlr_size, GFP_KERNEL);
Perhaps you need to use one of the macros from overflow.h.
> + if (!ctlr) > + return NULL; > + > + device_initialize(&ctlr->dev); > + > + ctlr->bus_num = -1; > + ctlr->secondary = secondary; > + ctlr->dev.parent = dev; > + ctlr->dev.release = wiegand_controller_release; > + > + wiegand_controller_set_devdata(ctlr, (void *)ctlr + ctlr_size); > + > + return ctlr; > +}
...
> +EXPORT_SYMBOL_GPL(wiegand_alloc_controller);
Can we have it namespaced from day 1, please?
...
> +struct wiegand_controller *devm_wiegand_alloc_controller(struct device *dev, unsigned int size, > + bool secondary) > +{ > + struct wiegand_controller *ctlr, **ptr; > + > + ptr = devres_alloc(devm_wiegand_release_controller, sizeof(*ptr), GFP_KERNEL); > + if (!ptr) > + return NULL; > + > + ctlr = wiegand_alloc_controller(dev, size, secondary); > + if (ctlr) { > + ctlr->devm_allocated = true; > + *ptr = ctlr; > + devres_add(dev, ptr); > + } else { > + devres_free(ptr); > + }
Please, use devm_add_action_or_reset().
> + return ctlr; > +}
> +/** > + * register_wiegand_device - allocates and registers a new Wiegand device > + * @ctlr: controller structure to attach device to > + * @nc: devicetree node for the device
Have you run kernel doc validator? And it's not device tree only anymore, use word "firmware" instead.
> + */
...
> + wiegand = wiegand_alloc_device(ctlr); > + if (!wiegand) {
> + dev_err(&ctlr->dev, "wiegad_device alloc error for %pOF\n", fwnode);
We don't print an error messages for -ENOMEM.
> + ret = -ENOMEM; > + goto err_out;
Are you sure we need to put the device in this case? When it will be not NULL here?
> + }
> + fwnode_handle_get(fwnode); > + wiegand->dev.fwnode = fwnode;
Not sure why you need a bumped reference, but in any case the problematic is the second line. Please, avoid dereferencing fwnode in struct device. Use respective APIs, here is device_set_node().
> + ret = wiegand_add_device(wiegand); > + if (ret) { > + dev_err(&ctlr->dev, "wiegand_device register error %pOF\n", fwnode); > + goto err_node_put; > + } > + > + /* check if more devices are connected to the bus */ > + if (ctlr->device_count > 1) > + dev_warn(&ctlr->dev, "Wiegand is a point-to-point bus, it is advised to only connect one device per Wiegand bus. The devices may not communicate using the same pulse length, format or else.\n"); > + > + return wiegand; > + > +err_node_put: > + fwnode_handle_put(fwnode); > +err_out: > + wiegand_device_put(wiegand); > + return ERR_PTR(ret);
...
> +static void register_wiegand_devices(struct wiegand_controller *ctlr) > +{ > + struct wiegand_device *wiegand; > + struct fwnode_handle *fwnode;
> + if (!ctlr->dev.fwnode) > + return;
This is a dup, which is implied already by the below for_each call.
> + fwnode_for_each_available_child_node(ctlr->dev.fwnode, fwnode) { > + wiegand = register_wiegand_device(ctlr, fwnode); > + if (IS_ERR(wiegand)) > + dev_warn(&ctlr->dev, "failed to create wiegand device for %pOF\n", fwnode);
You are using wrong specifier for fwnode. PLease, fix it everywhere.
> + } > +}
...
> +static void wiegand_controller_parse_property(struct device *dev, const char *prop_name, > + u32 *cur_val_p, u32 def_val, bool use_def) > +{ > + int ret; > + > + ret = device_property_read_u32(dev, prop_name, cur_val_p); > + if ((ret && use_def) || *cur_val_p == 0) > + *cur_val_p = def_val; > + > + dev_dbg(dev, "%s: %u\n", prop_name, *cur_val_p);
Why do we need this message? What for?
> +}
> +#define USE_DEFAULT_VAL 1
Redundant. hence redundant second parameter to the above call.
...
> +static void wiegand_controller_parse_properties(struct wiegand_controller *ctlr) > +{ > + wiegand_controller_parse_property(&ctlr->dev, "pulse-len-us", &ctlr->pulse_len, > + 50, USE_DEFAULT_VAL); > + wiegand_controller_parse_property(&ctlr->dev, "interval-len-us", &ctlr->interval_len, > + 2000, USE_DEFAULT_VAL); > + wiegand_controller_parse_property(&ctlr->dev, "frame-gap-us", &ctlr->frame_gap, > + 2000, USE_DEFAULT_VAL);
Default make more sense from group parsing, like I²C core does for timings.
> +}
...
> +int wiegand_register_controller(struct wiegand_controller *ctlr) > +{ > + struct device *dev = ctlr->dev.parent; > + int status, id;
> + if (!dev) > + return -ENODEV;
Why?
> + status = wiegand_controller_check_ops(ctlr); > + if (status) > + return status; > + > + id = ida_alloc(&wiegand_controller_ida, GFP_KERNEL); > + if (WARN(id < 0, "couldn't get an id\n"))
Is it really needs to be WARN()?
> + return id; > + ctlr->bus_num = id;
> + dev_set_name(&ctlr->dev, "wiegand%u", ctlr->bus_num);
No check?
> + ctlr->miscdev.name = kasprintf(GFP_KERNEL, "wiegand1"); > + if (!ctlr->miscdev.name) > + return -ENOMEM;
...
> +free_bus_id:
out_free_bus_id:
> + ida_free(&wiegand_controller_ida, ctlr->bus_num); > + misc_deregister(&ctlr->miscdev); > + kfree(ctlr->miscdev.name); > + return status; > +}
...
> +static inline void wiegand_device_put(struct wiegand_device *wiegand)
And where is the corresponding get()?
> +{ > + if (wiegand) > + put_device(&wiegand->dev);
> + if (wiegand->controller->cleanup) > + wiegand->controller->cleanup(wiegand);
Dup of wiegand_cleanup().
> +}
...
> +static int wiegand_dev_set_name(struct wiegand_device *wiegand, u8 id) > +{ > + int ret = dev_set_name(&wiegand->dev, "%s.%u", dev_name(&wiegand->controller->dev), id); > + return ret;
return dev_set_name(...); ?
> +}
...
> +int wiegand_setup(struct wiegand_device *wiegand) > +{ > + int status = 0;
Redundant assignment, see below.
> + if (wiegand->controller->setup) { > + status = wiegand->controller->setup(wiegand); > + if (status) { > + dev_err(&wiegand->controller->dev, "failed to setup device: %d\n", status);
In all such cases you may make the code neater with
struct device *ctrl_dev = &wiegand->controller->dev;
Also consider
struct ... *ctrl = wiegand->controller; struct device *ctrl_dev = &controller->dev;
> + return status; > + } > + }
> + return status;
Here it's always 0, right?
return 0;
> +}
...
> +void wiegand_unregister_device(struct wiegand_device *wiegand) > +{ > + if (!wiegand) > + return;
> + if (wiegand->dev.fwnode)
Yet another dup conditional, implied by fwnode API.
> + fwnode_handle_put(wiegand->dev.fwnode);
> + fwnode_remove_software_node(wiegand->dev.fwnode);
Where this has come from? Leftover from some copy'n'paste?
> + device_del(&wiegand->dev); > + wiegand_cleanup(wiegand); > + put_device(&wiegand->dev); > +}
...
> +static ssize_t wiegand_get_user_data(struct wiegand_controller *ctlr, char __user const *buf, > + size_t len) > +{ > + int i; > + char data_buffer[WIEGAND_MAX_PAYLEN_BYTES]; > + > + if (len > WIEGAND_MAX_PAYLEN_BYTES) > + return -EBADMSG; > + > + if (copy_from_user(&data_buffer[0], buf, WIEGAND_MAX_PAYLEN_BYTES))
&data_buffer[0] --> data_buffer
> + return -EFAULT; > + > + for (i = 0; i < len; i++) > + bitmap_set_value8(ctlr->data_bitmap, data_buffer[i], i * 8); > + > + return len; > +}
I'm wondering why you can't use bitmap_parse_user() with the respective format? Or even bitmap_parselist_user() (if you feel like this would be better, I feel like not really, but who knows).
...
> +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; > + u32 msg_length = ctlr->payload_len; > + > + if (!buf || len == 0 || DIV_ROUND_UP(msg_length, 8) > len)
BITS_TO_BYTES()
> + return -EINVAL; > + > + ret = wiegand_get_user_data(ctlr, buf, len); > + if (ret < 0) > + return ret; > + > + ctlr->transfer_message(ctlr); > + > + 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); > + > + 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 err; > + } > + > + mutex_unlock(&ctlr->file_lock); > + > + return 0; > +err:
err_unlock:
> + mutex_unlock(&ctlr->file_lock);
> + mutex_destroy(&ctlr->file_lock);
Huh?!
> + return ret; > +}
...
> + return (strcmp(wiegand_dev->modalias, drv->name) == 0);
Outer parentheses are redundant.
...
> +static int wiegand_probe(struct device *dev) > +{ > + struct wiegand_device *wiegand = to_wiegand_device(dev); > + const struct wiegand_driver *wdrv = to_wiegand_driver(dev->driver);
> + if (wdrv->probe)
Shouldn't this be a mandatory callback? Or i.o.w. can you elaborate the use case with probe == NULL?
> + return wdrv->probe(wiegand); > + > + return 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);
> + return ret; > + } > + > + return 0;
return ret;
> +}
...
> +postcore_initcall_sync(wiegand_init);
Move it closer to the callback itself.
...
> +#ifndef H_LINUX_INCLUDE_LINUX_WIEGAND_H > +#define H_LINUX_INCLUDE_LINUX_WIEGAND_H
container_of.h
> +#include <linux/device.h> > +#include <linux/miscdevice.h>
> +#include <linux/mod_devicetable.h>
Not sure it's in use.
> +#include <linux/mutex.h>
types.h
> +#define WIEGAND_NAME_SIZE 32 > +#define WIEGAND_MAX_PAYLEN_BYTES 256
> +extern struct bus_type wiegand_type;
So, extern or static? Please, be consistent.
...
> + struct file_operations fops;
Missing header for this data type.
...
> +#define wiegand_primary_get_devdata(_ctlr) wiegand_controller_get_devdata(_ctlr) > +#define wiegand_primary_set_devdata(_ctlr, _data) wiegand_controller_set_devdata(_ctlr, _data)
Why not _data --> data?
...
> +extern void wiegand_unregister_device(struct wiegand_device *wiegand); > +extern struct wiegand_controller *wiegand_device_get_controller(struct wiegand_device *dev); > + > +extern int wiegand_send_message(struct wiegand_device *wiegand, unsigned long *msg_bmp, u8 bitlen);
...and so on...
Drop extern from the function declarations.
...
> +/* Wiegand driver section */
Single space is enough.
-- With Best Regards, Andy Shevchenko
| |