On Wed, 20 Dec 2023 16:34:09 +0100 Nuno Sa <nuno.sa@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx > S: Maintained > F: drivers/media/rc/iguanair.c > > +IIO BACKEND FRAMEWORK > +M: Nuno Sa <nuno.sa@xxxxxxxxxx> > +R: Olivier Moysan <olivier.moysan@xxxxxxxxxxx> > +L: linux-iio@xxxxxxxxxxxxxxx > +S: Maintained > +F: drivers/iio/industrialio-backend.c > +F: include/linux/iio/backend.h > + > IIO DIGITAL POTENTIOMETER DAC > M: Peter Rosin <peda@xxxxxxxxxx> > L: linux-iio@xxxxxxxxxxxxxxx > 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);