Date:   Wed, 1 Mar 2023 18:23:54 +0200
From:   Andy Shevchenko <andriy.shevchenko@...el.com>
To:     Martin Zaťovič <m.zatovic1@...il.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 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.

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