Re: [PATCH v4 6/8] iio: add the IIO backend framework

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 20 Dec 2023 16:34:09 +0100
Nuno Sa <nuno.sa@analog.com> wrote:

> This is a Framework to handle complex IIO aggregate devices.
> 
> The typical architecture is to have one device as the frontend device which
> can be "linked" against one or multiple backend devices. All the IIO and
> userspace interface is expected to be registers/managed by the frontend
> device which will callback into the backends when needed (to get/set
> some configuration that it does not directly control).
> 
> The basic framework interface is pretty simple:
>  - Backends should register themselves with @devm_iio_backend_register()
>  - Frontend devices should get backends with @devm_iio_backend_get()
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>

A few minor comments, but seems good to me otherwise.

Jonathan

> ---
>  MAINTAINERS                        |   8 +
>  drivers/iio/Kconfig                |   5 +
>  drivers/iio/Makefile               |   1 +
>  drivers/iio/industrialio-backend.c | 456 +++++++++++++++++++++++++++++++++++++
>  include/linux/iio/backend.h        |  75 ++++++
>  5 files changed, 545 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3029841e92a8..df5f5b988926 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10334,6 +10334,14 @@ L:	linux-media@vger.kernel.org
>  S:	Maintained
>  F:	drivers/media/rc/iguanair.c
>  
> +IIO BACKEND FRAMEWORK
> +M:	Nuno Sa <nuno.sa@analog.com>
> +R:	Olivier Moysan <olivier.moysan@foss.st.com>
> +L:	linux-iio@vger.kernel.org
> +S:	Maintained
> +F:	drivers/iio/industrialio-backend.c
> +F:	include/linux/iio/backend.h
> +
>  IIO DIGITAL POTENTIOMETER DAC
>  M:	Peter Rosin <peda@axentia.se>
>  L:	linux-iio@vger.kernel.org
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 52eb46ef84c1..451671112f73 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -71,6 +71,11 @@ config IIO_TRIGGERED_EVENT
>  	help
>  	  Provides helper functions for setting up triggered events.
>  
> +config IIO_BACKEND
> +	tristate
> +	help
> +	  Framework to handle complex IIO aggregate devices.

Add some more description here. Not sure the current text will help
anyone understand it :)

> +
>  source "drivers/iio/accel/Kconfig"
>  source "drivers/iio/adc/Kconfig"
>  source "drivers/iio/addac/Kconfig"
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index 9622347a1c1b..0ba0e1521ba4 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_IIO_GTS_HELPER) += industrialio-gts-helper.o
>  obj-$(CONFIG_IIO_SW_DEVICE) += industrialio-sw-device.o
>  obj-$(CONFIG_IIO_SW_TRIGGER) += industrialio-sw-trigger.o
>  obj-$(CONFIG_IIO_TRIGGERED_EVENT) += industrialio-triggered-event.o
> +obj-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
>  
>  obj-y += accel/
>  obj-y += adc/
> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
> new file mode 100644
> index 000000000000..75a0a66003e1
> --- /dev/null
> +++ b/drivers/iio/industrialio-backend.c
> @@ -0,0 +1,456 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Framework to handle complex IIO aggregate devices.
> + *
> + * The typical architecture is to have one device as the frontend device which
> + * can be "linked" against one or multiple backend devices. All the IIO and
> + * userspace interface is expected to be registers/managed by the frontend
> + * device which will callback into the backends when needed (to get/set some
> + * configuration that it does not directly control).
> + *
> + * The framework interface is pretty simple:
> + *   - Backends should register themselves with @devm_iio_backend_register()
> + *   - Frontend devices should get backends with @devm_iio_backend_get()
> + *
> + * Also to note that the primary target for this framework are converters like
> + * ADC/DACs so @iio_backend_ops will have some operations typical of converter
> + * devices. On top of that, this is "generic" for all IIO which means any kind
> + * of device can make use of the framework. That said, If the @iio_backend_ops
> + * struct begins to grow out of control, we can always refactor things so that
> + * the industrialio-backend.c is only left with the really generic stuff. Then,
> + * we can build on top of it depending on the needs.
> + *
> + * Copyright (C) 2023 Analog Devices Inc.
> + */
> +#define pr_fmt(fmt) "iio-backend: " fmt
> +
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/list.h>
> +#include <linux/lockdep.h>
> +#include <linux/kref.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +
> +#include <linux/iio/backend.h>
> +
> +struct iio_backend {
> +	struct list_head entry;
> +	const struct iio_backend_ops *ops;
> +	struct device *dev;
> +	struct module *owner;
> +	void *priv;
> +	/*
> +	 * mutex used to synchronize backend callback access with concurrent
> +	 * calls to @iio_backend_unregister. The lock makes sure a device is
> +	 * not unregistered while a callback is being run.
> +	 */
> +	struct mutex lock;
> +	struct kref ref;
> +};
> +

...

> +
> +static void iio_backend_release(void *arg)
> +{
> +	struct iio_backend *back = arg;
> +
> +	module_put(back->owner);
> +	kref_put(&back->ref, iio_backend_free);
> +}
> +
> +static int __devm_iio_backend_get(struct device *dev, struct iio_backend *back)
> +{
> +	struct device_link *link;
> +	int ret;
> +
> +	kref_get(&back->ref);
> +	if (!try_module_get(back->owner)) {
> +		pr_err("%s: Cannot get module reference\n", dev_name(dev));

Why do you need the reference?  Good to add a comment on that here.

> +		return -ENODEV;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, iio_backend_release, back);
> +	if (ret)
> +		return ret;
> +
> +	link = device_link_add(dev, back->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> +	if (!link)
> +		pr_warn("%s: Could not link to supplier(%s)\n", dev_name(dev),
> +			dev_name(back->dev));

Why is that not an error and we try to carry on?

> +
> +	pr_debug("%s: Found backend(%s) device\n", dev_name(dev),
> +		 dev_name(back->dev));
> +	return 0;
> +}
> +
> +/**
> + * devm_iio_backend_get - Get a backend device
> + * @dev:	Device where to look for the backend.
> + * @name:	Backend name.
> + *
> + * Get's the backend associated with @dev.
> + *
> + * RETURNS:
> + * A backend pointer, negative error pointer otherwise.
> + */
> +struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
> +{
> +	struct fwnode_handle *fwnode;
> +	struct iio_backend *back;
> +	int index = 0, ret;
> +
> +	if (name) {
> +		index = device_property_match_string(dev, "io-backends-names",
> +						     name);
> +		if (index < 0)
> +			return ERR_PTR(index);
> +	}
> +
> +	fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index);
> +	if (IS_ERR(fwnode)) {
> +		/* not an error if optional */
> +		pr_debug("%s: Cannot get Firmware reference\n", dev_name(dev));
> +		return ERR_CAST(fwnode);
> +	}
> +
> +	guard(mutex)(&iio_back_lock);
> +	list_for_each_entry(back, &iio_back_list, entry) {
> +		if (!device_match_fwnode(back->dev, fwnode))
> +			continue;
> +
> +		fwnode_handle_put(fwnode);
> +		ret = __devm_iio_backend_get(dev, back);
> +		if (ret)
> +			return ERR_PTR(ret);
> +
> +		return back;
> +	}
> +
> +	fwnode_handle_put(fwnode);

FYI. I have a series doing auto cleanup of fwnode_handles in progress.
Should get some time over the weekend to snd that out.  Aim is to avoid need
to dance around manually freeing them (similar to the DT __free(device_node)
series I posted the other day).

> +	return ERR_PTR(-EPROBE_DEFER);
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_iio_backend_get, IIO_BACKEND);
> +
> +/**
> + * devm_iio_backend_get_optional - Get optional backend device
> + * @dev:	Device where to look for the backend.
> + * @name:	Backend name.
> + *
> + * Same as @devm_iio_backend_get() but return NULL if backend not found.
> + *
> + * RETURNS:
> + * A backend pointer, negative error pointer otherwise or NULL if not found.
> + */
> +struct iio_backend *devm_iio_backend_get_optional(struct device *dev,
> +						  const char *name)
> +{
> +	struct iio_backend *back;
> +
> +	back = devm_iio_backend_get(dev, name);
> +	if (IS_ERR(back) && PTR_ERR(back) == -ENOENT)
> +		return NULL;
> +
> +	return back;
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_iio_backend_get_optional, IIO_BACKEND);

I'm not convinced the optional variant is worth while.  Could just choose
a particular return value to mean that e.g. ERR_PTR(-ENOENT) and document
it for the normal get.  Then have special handling in the drivers where
you need backwards compatibility with a previous approach.

I'd rather pay the complexity price in a couple of drivers than have
to explain backends aren't typically optional for years to come.


> +
> +/**
> + * devm_iio_backend_get_from_fwnode_lookup
> + * @dev:	Device where to bind the backend lifetime.
> + * @fwnode:	Firmware node of the backend device.
> + *
> + * It directly looks the backend device list for a device matching @fwnode.

I would word this:
Search the backend list for a device matchign &fwnode.

> + * This API should not be used and it's only present for preventing the first
> + * user of this framework to break it's DT ABI.

You could stick a __ in front of the name to hopefully scare people away :)
+ highlight something odd is going on to reviewers seeing this called in
some future driver.
Also I can we might convert other drivers that are doing similar things
(dfsdm for example) and maybe this will be useful
so __devm_iio_backend_get_from_fwnode_lookup() and
"preventing breakage of old DT bindings".

> + *
> + * RETURNS:
> + * A backend pointer, negative error pointer otherwise.
> + */
> +struct iio_backend *
> +devm_iio_backend_get_from_fwnode_lookup(struct device *dev,
> +					struct fwnode_handle *fwnode)
> +{
> +	struct iio_backend *back;
> +	int ret;
> +
> +	guard(mutex)(&iio_back_lock);
> +	list_for_each_entry(back, &iio_back_list, entry) {
> +		if (!device_match_fwnode(back->dev, fwnode))
> +			continue;
> +
> +		ret = __devm_iio_backend_get(dev, back);
> +		if (ret)
> +			return ERR_PTR(ret);
> +
> +		return back;
> +	}
> +
> +	return ERR_PTR(-EPROBE_DEFER);
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_iio_backend_get_from_fwnode_lookup, IIO_BACKEND);