Date:   Tue, 4 Apr 2023 15:13:34 +0200
From:   Martin Zaťovič <m.zatovic1@...il.com>
To:     Andy Shevchenko <andriy.shevchenko@...el.com>
Cc:     linux-kernel@...r.kernel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, gregkh@...uxfoundation.org,
        airlied@...hat.com, dipenp@...dia.com, treding@...dia.com,
        mwen@...lia.com, fmdefrancesco@...il.com, arnd@...db.de,
        bvanassche@....org, ogabbay@...nel.org, axboe@...nel.dk,
        mathieu.poirier@...aro.org, linux@...y.sk, masahiroy@...nel.org,
        yangyicong@...ilicon.com, dan.j.williams@...el.com,
        jacek.lawrynowicz@...ux.intel.com, benjamin.tissoires@...hat.com,
        devicetree@...r.kernel.org, furong.zhou@...ux.intel.com,
        linus.walleij@...aro.org
Subject: Re: [PATCHv3 2/4] wiegand: add Wiegand bus driver

On Wed, Mar 01, 2023 at 06:23:54PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 01, 2023 at 03:28:33PM +0100, Martin Zaťovič wrote:
> > 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.
> 
> ...
> 
> > +#include <linux/device.h>
> > +#include <linux/dmaengine.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +#include <linux/wiegand.h>
> > +
> > +static struct bus_type wiegand_bus_type;
> 
> > +static DEFINE_IDR(wiegand_controller_idr);
> 
> Why not IDA or even xarray?
> 
> ...
> 
> > +static DEFINE_MUTEX(board_lock);
> 
> Or locks need a good description for what they are.
> 
> ...
> 
> > +static void devm_wiegand_release_controller(struct device *dev, void *ctlr)
> > +{
> > +	wiegand_controller_put(*(struct wiegand_controller **)ctlr);
> > +}
> 
> This is not used in the following function, so can be moved closer to its user.
> 
> ...
> 
> > +struct wiegand_controller *devm_wiegand_alloc_controller(struct device *dev, unsigned int size,
> > +							bool slave)
> > +{
> > +	struct wiegand_controller **ptr, *ctlr;
> > +
> > +	ptr = devres_alloc(devm_wiegand_release_controller, sizeof(*ptr), GFP_KERNEL);
> > +	if (!ptr)
> > +		return NULL;
> > +
> > +	ctlr = wiegand_alloc_controller(dev, size, slave);
> > +	if (ctlr) {
> > +		ctlr->devm_allocated = true;
> > +		*ptr = ctlr;
> > +		devres_add(dev, ptr);
> > +	} else {
> > +		devres_free(ptr);
> > +	}
> > +
> > +	return ctlr;
> 
> Can this utilize devm_add_action_or_reset()?
> 
> > +}
> 
> ...
> 
> > +/**
> > + * of_register_wiegand_device - allocates and registers a new Wiegand device based on devicetree
> 
> NAK for OF only code. New, esp. bus, code must be agnostic. We have all means
> for that.

In one of the previous versions of this patch series, there was also the possibility to instantiate
the device from another driver. I have been told, that this is not the way to go anymore, unless
there is a very specific reason for that. I did not find such reason, so I have removed this suport.

The only other device instantiating method I could think of is ACPI, however I believe it would be
just dead code no one would use, as the Wiegand interface is not normally used on non-embedded
devices. This is the point Evgeny Boger argued for as well. Is there any other device instantiating
method I am not aware of, that would be suitable for this bus?
 
> register_wiegand_device() or similar name.
> 
> > + * node
> > + * @ctlr: controller structure to attach device to
> > + * @nc: devicetree node for the device
> > + */
> > +static struct wiegand_device *of_register_wiegand_device(struct wiegand_controller *ctlr,
> 
> Ditto.
> 
> > +							struct device_node *nc)
> 
> struct fwnode_handle *fwnode
> 
> > +{
> > +	struct wiegand_device *wiegand;
> > +	int rc;
> > +
> > +	wiegand = wiegand_alloc_device(ctlr);
> > +	if (!wiegand) {
> > +		dev_err(&ctlr->dev, "wiegad_device alloc error for %pOF\n", nc);
> > +		rc = -ENOMEM;
> > +		goto err_out;
> > +	}
> 
> > +	of_node_get(nc);
> > +	wiegand->dev.of_node = nc;
> > +	wiegand->dev.fwnode = of_fwnode_handle(nc);
> 
> 	device_set_node(&wiegand->dev, fwnode_handle_get(fwnode));
> 
> > +	rc = wiegand_add_device(wiegand);
> > +	if (rc) {
> > +		dev_err(&ctlr->dev, "wiegand_device register error %pOF\n", nc);
> > +		goto err_of_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_of_node_put:
> > +	of_node_put(nc);
> > +err_out:
> > +	wiegand_dev_put(wiegand);
> > +	return ERR_PTR(rc);
> > +}
> > +
> > +/**
> > + * of_register_wiegand_devices - creates a wiegand device for all children of a controller
> > + * devicetree node
> > + * @ctlr: controller structure to check
> > + */
> > +static void of_register_wiegand_devices(struct wiegand_controller *ctlr)
> > +{
> > +	struct wiegand_device *wiegand;
> > +	struct device_node *nc;
> > +
> > +	if (!ctlr->dev.of_node)
> > +		return;
> > +
> > +	for_each_available_child_of_node(ctlr->dev.of_node, nc) {
> > +		if (of_node_test_and_set_flag(nc, OF_POPULATED))
> > +			continue;
> > +		wiegand = of_register_wiegand_device(ctlr, nc);
> > +		if (IS_ERR(wiegand)) {
> > +			dev_warn(&ctlr->dev, "Failed to create wiegand device for %pOF\n", nc);
> > +			of_node_clear_flag(nc, OF_POPULATED);
> > +		}
> > +	}
> 
> No way. Use agnostic approach. See above for some suggestions.
> 
> > +}
> 
> ...
> 
> > +	if (!dev)
> > +		return -ENODEV;
> 
> When is it true and why is it a problem?
> 
> > +	if (ctlr->dev.of_node) {
> > +		id = of_alias_get_id(ctlr->dev.of_node, "wiegand");
> 
> Why? What does this bring to us and why it's so important?
> 
> > +		if (id > 0) {
> > +			ctlr->bus_num = id;
> > +			mutex_lock(&board_lock);
> > +			id = idr_alloc(&wiegand_controller_idr, ctlr, ctlr->bus_num,
> > +					ctlr->bus_num + 1, GFP_KERNEL);
> > +			mutex_unlock(&board_lock);
> > +			if (WARN(id < 0, "couldn't get idr"))
> > +				return id == -ENOSPC ? -EBUSY : id;
> 
> Why rewriting error code?
> 
> > +		}
> > +		device_property_read_u32(&ctlr->dev, "pulse-len-us", &ctlr->pulse_len);
> > +		device_property_read_u32(&ctlr->dev, "interval-len-us", &ctlr->interval_len);
> > +		device_property_read_u32(&ctlr->dev, "frame-gap-us", &ctlr->frame_gap);
> > +	}
> > +	if (ctlr->bus_num < 0) {
> > +		first_dynamic = of_alias_get_highest_id("wiegand");
> > +		if (first_dynamic < 0)
> > +			first_dynamic = 0;
> > +		else
> > +			first_dynamic++;
> > +
> > +		mutex_lock(&board_lock);
> > +		id = idr_alloc(&wiegand_controller_idr, ctlr, first_dynamic,
> > +								0, GFP_KERNEL);
> > +		mutex_unlock(&board_lock);
> > +		if (WARN(id < 0, "couldn't get idr\n"))
> > +			return id;
> > +		ctlr->bus_num = id;
> > +	}
> > +
> > +	if (ctlr->pulse_len == 0) {
> > +		dev_warn(&ctlr->dev, "pulse_len is not initialized, setting the default value 50us\n");
> > +		ctlr->pulse_len = 50;
> > +	}
> > +	if (ctlr->interval_len == 0) {
> > +		dev_warn(&ctlr->dev, "interval_len is not initialized, setting the default value 2000us\n");
> > +		ctlr->interval_len = 2000;
> > +	}
> > +	if (ctlr->frame_gap == 0) {
> > +		dev_warn(&ctlr->dev, "frame_gap is not initialized, setting the default value 2000us\n");
> > +		ctlr->frame_gap = 2000;
> > +	}
> 
> Why warnings? Can't it survive without them? (See, for example, how I²C
> controllers get timings).
> 
> > +	dev_set_name(&ctlr->dev, "wiegand%u", ctlr->bus_num);
> > +	ctlr->device_count = 0;
> > +
> > +	status = device_add(&ctlr->dev);
> > +	if (status < 0)
> > +		goto free_bus_id;
> > +
> > +	of_register_wiegand_devices(ctlr);
> > +
> > +	return status;
> > +
> > +free_bus_id:
> > +	mutex_lock(&board_lock);
> > +	idr_remove(&wiegand_controller_idr, ctlr->bus_num);
> > +	mutex_unlock(&board_lock);
> > +	return status;
> > +}
> 
> ...
> 
> > +int devm_wiegand_register_controller(struct device *dev, struct wiegand_controller *ctlr)
> > +{
> > +	struct wiegand_controller **ptr;
> > +	int ret;
> > +
> > +	ptr = devres_alloc(devm_wiegand_unregister, sizeof(*ptr), GFP_KERNEL);
> > +	if (!ptr)
> > +		return -ENOMEM;
> > +
> > +	ret = wiegand_register_controller(ctlr);
> > +	if (!ret) {
> > +		*ptr = ctlr;
> > +		devres_add(dev, ptr);
> > +	} else {
> > +		devres_free(ptr);
> > +	}
> 
> devm_add_action_or_reset() ?
> 
> > +	return ret;
> > +}
> 
> ...
> 
> > +struct wiegand_device *wiegand_alloc_device(struct wiegand_controller *ctlr)
> > +{
> > +	struct wiegand_device *wiegand;
> > +
> > +	if (!wiegand_controller_get(ctlr))
> > +		return NULL;
> 
> Is it important to be called before pure memory allocation? The reference
> counting on the existing resource is less failure prone, right?
> 
> > +	wiegand = kzalloc(sizeof(*wiegand), GFP_KERNEL);
> > +	if (!wiegand) {
> > +		wiegand_controller_put(ctlr);
> > +		return NULL;
> > +	}
> > +
> > +	wiegand->controller = ctlr;
> > +	wiegand->dev.parent = &ctlr->dev;
> > +	wiegand->dev.bus = &wiegand_bus_type;
> > +	wiegand->dev.release = wieganddev_release;
> > +
> > +	device_initialize(&wiegand->dev);
> > +	return wiegand;
> > +}
> 
> ...
> 
> > +static void wiegand_dev_set_name(struct wiegand_device *wiegand, u8 id)
> > +{
> > +	dev_set_name(&wiegand->dev, "%s.%u", dev_name(&wiegand->controller->dev), id);
> 
> Why error is ignored?
> 
> > +}
> 
> ...
> 
> > +	if (wiegand->dev.of_node) {
> > +		of_node_clear_flag(wiegand->dev.of_node, OF_POPULATED);
> > +		of_node_put(wiegand->dev.of_node);
> > +	}
> 
> fwnode APIs, please.
> 
> ...
> 
> > +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);
> > +	int ret = 0;
> > +
> > +	if (wdrv->probe)
> > +		ret = wdrv->probe(wiegand);
> > +
> > +	return ret;
> 
> Hmm... this is just
> 
> 	if (wdrv->probe)
> 		return wdrv->probe(wiegand);
> 
> 	return 0;
> 
> > +}
> 
> ...
> 
> > +	if (wdrv->driver.of_match_table) {
> > +		const struct of_device_id *of_id;
> > +
> > +		for (of_id = wdrv->driver.of_match_table; of_id->compatible[0];
> > +			of_id++) {
> > +			const char *of_name;
> > +
> > +			/* remove vendor prefix */
> > +			of_name = strnchr(of_id->compatible,
> > +						sizeof(of_id->compatible), ',');
> > +			if (of_name)
> > +				of_name++;
> > +			else
> > +				of_name = of_id->compatible;
> > +
> > +			if (wdrv->driver.name) {
> > +				if (strcmp(wdrv->driver.name, of_name) == 0)
> > +					continue;
> > +			}
> > +
> > +			pr_warn("Wiegand driver %s has no device_id for %s\n",
> > +				wdrv->driver.name, of_id->compatible);
> > +		}
> > +	}
> 
> This looks like very much of a repetition of the existing code somewhere.
> 
> ...
> 
> > +/**
> > + * 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];
> > +};
> 
> Wondering if this can be made an opaque pointer instead.
> 
> ...
> 
> > +/**
> > + * struct wiegand_controller - Wiegand master or slave interface
> > + * @dev - Device interface of the controller
> > + * @list - Link with the global wiegand_controller list
> > + * @bus_num - Board-specific identifier for Wiegand controller
> > + * @pulse_len: length of the low pulse in usec; defaults to 50us
> > + * @interval_len: length of a whole bit (both the pulse and the high phase) in usec;
> > + * defaults to 2000us
> > + * @frame_gap: length of the last bit of a frame (both the pulse and the high phase) in usec;
> > + * defaults to interval_len
> > + * device_count - Counter of devices connected to the same Wiegand bus(controller).
> > + * devm_allocated - Whether the allocation of this struct is devres-managed
> > + * slave - Whether the controller is a slave(receives data).
> > + * transfer_message - Send a message on the bus.
> > + * setup - Setup a device.
> > + * cleanup - Cleanup after a device.
> 
> At some point you lost the grip on @ key button.
> 
> > + */
> 
> ...
> 
> > +struct wiegand_driver {
> > +	const struct wiegand_device_id *id_table;
> > +	int (*probe)(struct wiegand_device *wiegand);
> > +	void (*remove)(struct wiegand_device *wiegand);
> 
> > +	struct device_driver driver;
> 
> Making it first may save a few assembly instructions in pointer arithmetics.
> Check if I'm right with bloat-o-meter (you will need a user of this data type,
> of course).
> 
> > +};
> 
> ...
> 
> > +		driver_unregister(&wdrv->driver);
> 
> Yep, here and...
> 
> > +	return drv ? container_of(drv, struct wiegand_driver, driver) : NULL;
> 
> ...here you will save code bytes.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Powered by blists - more mailing lists