| Message ID | 20231220-iio-backend-v4-6-998e9148b692@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: add new backend framework | expand |
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);
On Thu, 2023-12-21 at 17:44 +0000, Jonathan Cameron wrote: > 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 :) > Alright... > > + > > 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. Yeah, typical stuff when it's a module and we don't want for it to go away. Even though we handle that case with pointer stuff. I'll comment it. > > > + 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? I guess having the links are not really mandatory for the whole thing to work (more like a nice to have). That's also how this is handled in another subsystems so I figured it would be fine. But since you are speaking about this... After you pointing me to Bartosz's talk and sawing it (as stuff like this is mentioned), I started to question this. The goal with the above comment is that if you remove the backend, all the consumers are automatically removed (unbound). Just not sure if that's what we always want (and we are already handling the situation where a backend goes away with -ENODEV) as some frontends could still be useful without the backend (I guess that could be plausible). I think for now we don't really have such usecases so the links can make sense (and we can figure something like optionally creating these links if we ever need too) but having your inputs on this will definitely be valuable. > > + > > + 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). Cool! This will surely be an user of that. > > > + 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. Alright. For now I'm not seeing any usecase where backends are optional. For the case we need the backward compatibility I'll do as you suggest. If we ever have a real 'optional' usecase we can easily add this one again. > > > > + > > +/** > > + * 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. ack > > > + * 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". Will do! Thanks for the inputs! - Nuno Sá
> > > + > > > + 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? > > I guess having the links are not really mandatory for the whole thing to work (more > like a nice to have). That's also how this is handled in another subsystems so I > figured it would be fine. > > But since you are speaking about this... After you pointing me to Bartosz's talk and > sawing it (as stuff like this is mentioned), I started to question this. The goal > with the above comment is that if you remove the backend, all the consumers are > automatically removed (unbound). Just not sure if that's what we always want (and we > are already handling the situation where a backend goes away with -ENODEV) as some > frontends could still be useful without the backend (I guess that could be > plausible). I think for now we don't really have such usecases so the links can make > sense (and we can figure something like optionally creating these links if we ever > need too) but having your inputs on this will definitely be valuable. I'm not keen on both trying to make everything tear down cleanly AND making sure it all works even if we don't. That just adds two code paths to test when either should be sufficient on its own. I don't really mind which. Bartosz's stuff is nice, but it may not be the right solution here. > > > > + > > > + pr_debug("%s: Found backend(%s) device\n", dev_name(dev), > > > + dev_name(back->dev)); > > > + return 0; > > > +} > > > + Jonathan
On Tue, 2023-12-26 at 15:59 +0000, Jonathan Cameron wrote: > > > > > + > > > > + 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? > > > > I guess having the links are not really mandatory for the whole thing to > > work (more > > like a nice to have). That's also how this is handled in another subsystems > > so I > > figured it would be fine. > > > > But since you are speaking about this... After you pointing me to Bartosz's > > talk and > > sawing it (as stuff like this is mentioned), I started to question this. The > > goal > > with the above comment is that if you remove the backend, all the consumers > > are > > automatically removed (unbound). Just not sure if that's what we always want > > (and we > > are already handling the situation where a backend goes away with -ENODEV) > > as some > > frontends could still be useful without the backend (I guess that could be > > plausible). I think for now we don't really have such usecases so the links > > can make > > sense (and we can figure something like optionally creating these links if > > we ever > > need too) but having your inputs on this will definitely be valuable. > > I'm not keen on both trying to make everything tear down cleanly AND making > sure > it all works even if we don't. That just adds two code paths to test when > either > should be sufficient on its own. I don't really mind which. Bartosz's stuff Agreed... > is nice, but it may not be the right solution here. There's pros and cons on both options... For the device links the cons I see is that it depends on patch 3 for it to work (or some other approach if the one in that patch is not good) - not really a real con though :). The biggest concern is (possible) future uses where we end up with cases where removing a backend is not really a "deal breaker". I could think of frontends that have multiple backends (one per data path) and removing one backend would not tear the whole thing down (we would just have one non functional data paths/port where the others are still ok). Olivier, for STM usecases, do we always need the backend? I mean, does it make sense to always remove/unbind the frontend in case the backend is unbound? Maybe some of your usecases already "forces" us with a decision. The biggest pro I see is code simplicity. If we can assume the frontend can never exist in case one of the backends is gone, we can: * get rid of the sync mutex; * get rid of the kref and bind the backend object lifetime to the backend device (using devm_kzalloc() instead of kzalloc + refcount. Basically, we would not need to care about syncing the backend existence with accessing it... To sum up, the device_links approach tends to be similar (not identical) to the previous approach using the component API. The biggest pro I see in Bartosz's stuff is flexibility. So it should just work in whatever future usecases we might have. I fear that going the device_links road we might end up needing this stuff anyways. Obviously, the biggest con is code complexity (not that bad though) as we always need to properly sync any backend callback. - Nuno Sá >
On Tue, 09 Jan 2024 13:15:54 +0100 Nuno Sá <noname.nuno@gmail.com> wrote: > On Tue, 2023-12-26 at 15:59 +0000, Jonathan Cameron wrote: > > > > > > > + > > > > > + 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? > > > > > > I guess having the links are not really mandatory for the whole thing to > > > work (more > > > like a nice to have). That's also how this is handled in another subsystems > > > so I > > > figured it would be fine. > > > > > > But since you are speaking about this... After you pointing me to Bartosz's > > > talk and > > > sawing it (as stuff like this is mentioned), I started to question this. The > > > goal > > > with the above comment is that if you remove the backend, all the consumers > > > are > > > automatically removed (unbound). Just not sure if that's what we always want > > > (and we > > > are already handling the situation where a backend goes away with -ENODEV) > > > as some > > > frontends could still be useful without the backend (I guess that could be > > > plausible). I think for now we don't really have such usecases so the links > > > can make > > > sense (and we can figure something like optionally creating these links if > > > we ever > > > need too) but having your inputs on this will definitely be valuable. > > > > I'm not keen on both trying to make everything tear down cleanly AND making > > sure > > it all works even if we don't. That just adds two code paths to test when > > either > > should be sufficient on its own. I don't really mind which. Bartosz's stuff > > Agreed... > > > is nice, but it may not be the right solution here. > > There's pros and cons on both options... > > For the device links the cons I see is that it depends on patch 3 for it to work > (or some other approach if the one in that patch is not good) - not really a > real con though :). The biggest concern is (possible) future uses where we end > up with cases where removing a backend is not really a "deal breaker". I could > think of frontends that have multiple backends (one per data path) and removing > one backend would not tear the whole thing down (we would just have one non > functional data paths/port where the others are still ok). I wouldn't waste time catering to such set ups. If the whole thing gets torn down because one element went away that should be fine. To do anything else I'd want to see a real world use case. > > Olivier, for STM usecases, do we always need the backend? I mean, does it make > sense to always remove/unbind the frontend in case the backend is unbound? > > Maybe some of your usecases already "forces" us with a decision. > > The biggest pro I see is code simplicity. If we can assume the frontend can > never exist in case one of the backends is gone, we can: > > * get rid of the sync mutex; > * get rid of the kref and bind the backend object lifetime to the backend > device (using devm_kzalloc() instead of kzalloc + refcount. > > Basically, we would not need to care about syncing the backend existence with > accessing it... > To sum up, the device_links approach tends to be similar (not identical) to the > previous approach using the component API. > > The biggest pro I see in Bartosz's stuff is flexibility. So it should just work > in whatever future usecases we might have. I fear that going the device_links > road we might end up needing this stuff anyways. I'm keen on it if it simplifies code or becomes the dominant paradigm for such things in the kernel (so becomes what people expect). That isn't true yet and I doubt it will be particularly soon. If things head that way we can revisit as it would enable things that currently we don't support - nothing should break. Jonathan > > Obviously, the biggest con is code complexity (not that bad though) as we always > need to properly sync any backend callback. > > - Nuno Sá > > >
On Wed, 2024-01-10 at 09:16 +0000, Jonathan Cameron wrote: > On Tue, 09 Jan 2024 13:15:54 +0100 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Tue, 2023-12-26 at 15:59 +0000, Jonathan Cameron wrote: > > > > > > > > > + > > > > > > + 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? > > > > > > > > I guess having the links are not really mandatory for the whole thing to > > > > work (more > > > > like a nice to have). That's also how this is handled in another > > > > subsystems > > > > so I > > > > figured it would be fine. > > > > > > > > But since you are speaking about this... After you pointing me to > > > > Bartosz's > > > > talk and > > > > sawing it (as stuff like this is mentioned), I started to question this. > > > > The > > > > goal > > > > with the above comment is that if you remove the backend, all the > > > > consumers > > > > are > > > > automatically removed (unbound). Just not sure if that's what we always > > > > want > > > > (and we > > > > are already handling the situation where a backend goes away with - > > > > ENODEV) > > > > as some > > > > frontends could still be useful without the backend (I guess that could > > > > be > > > > plausible). I think for now we don't really have such usecases so the > > > > links > > > > can make > > > > sense (and we can figure something like optionally creating these links > > > > if > > > > we ever > > > > need too) but having your inputs on this will definitely be valuable. > > > > > > I'm not keen on both trying to make everything tear down cleanly AND > > > making > > > sure > > > it all works even if we don't. That just adds two code paths to test when > > > either > > > should be sufficient on its own. I don't really mind which. Bartosz's > > > stuff > > > > Agreed... > > > > > is nice, but it may not be the right solution here. > > > > There's pros and cons on both options... > > > > For the device links the cons I see is that it depends on patch 3 for it to > > work > > (or some other approach if the one in that patch is not good) - not really a > > real con though :). The biggest concern is (possible) future uses where we > > end > > up with cases where removing a backend is not really a "deal breaker". I > > could > > think of frontends that have multiple backends (one per data path) and > > removing > > one backend would not tear the whole thing down (we would just have one non > > functional data paths/port where the others are still ok). > > I wouldn't waste time catering to such set ups. If the whole thing gets > torn down because one element went away that should be fine. > To do anything else I'd want to see a real world use case. Fair enough... > > > > > Olivier, for STM usecases, do we always need the backend? I mean, does it > > make > > sense to always remove/unbind the frontend in case the backend is unbound? > > > > Maybe some of your usecases already "forces" us with a decision. > > > > The biggest pro I see is code simplicity. If we can assume the frontend can > > never exist in case one of the backends is gone, we can: > > > > * get rid of the sync mutex; > > * get rid of the kref and bind the backend object lifetime to the backend > > device (using devm_kzalloc() instead of kzalloc + refcount. > > > > Basically, we would not need to care about syncing the backend existence > > with > > accessing it... > > To sum up, the device_links approach tends to be similar (not identical) to > > the > > previous approach using the component API. > > > > The biggest pro I see in Bartosz's stuff is flexibility. So it should just > > work > > in whatever future usecases we might have. I fear that going the > > device_links > > road we might end up needing this stuff anyways. > > I'm keen on it if it simplifies code or becomes the dominant paradigm for such > things in the kernel (so becomes what people expect). That isn't true yet > and I doubt it will be particularly soon. If things head that way we can > revisit as it would enable things that currently we don't support - nothing > should break. > Well, If I'm not missing anything, simpler code would be with device_links so I guess that's your preferred approach for now :). Also fine by me as this is an in kernel interface so we easily revisit it. I'll just wait a bit more (before sending v5) to see if Olivier has any foreseeable usecase where device_links would be an issue. - Nuno Sá
Hi Nuno, On 1/9/24 13:15, Nuno Sá wrote: > On Tue, 2023-12-26 at 15:59 +0000, Jonathan Cameron wrote: >> >>>>> + >>>>> + 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? >>> >>> I guess having the links are not really mandatory for the whole thing to >>> work (more >>> like a nice to have). That's also how this is handled in another subsystems >>> so I >>> figured it would be fine. >>> >>> But since you are speaking about this... After you pointing me to Bartosz's >>> talk and >>> sawing it (as stuff like this is mentioned), I started to question this. The >>> goal >>> with the above comment is that if you remove the backend, all the consumers >>> are >>> automatically removed (unbound). Just not sure if that's what we always want >>> (and we >>> are already handling the situation where a backend goes away with -ENODEV) >>> as some >>> frontends could still be useful without the backend (I guess that could be >>> plausible). I think for now we don't really have such usecases so the links >>> can make >>> sense (and we can figure something like optionally creating these links if >>> we ever >>> need too) but having your inputs on this will definitely be valuable. >> >> I'm not keen on both trying to make everything tear down cleanly AND making >> sure >> it all works even if we don't. That just adds two code paths to test when >> either >> should be sufficient on its own. I don't really mind which. Bartosz's stuff > > Agreed... > >> is nice, but it may not be the right solution here. > > There's pros and cons on both options... > > For the device links the cons I see is that it depends on patch 3 for it to work > (or some other approach if the one in that patch is not good) - not really a > real con though :). The biggest concern is (possible) future uses where we end > up with cases where removing a backend is not really a "deal breaker". I could > think of frontends that have multiple backends (one per data path) and removing > one backend would not tear the whole thing down (we would just have one non > functional data paths/port where the others are still ok). > > Olivier, for STM usecases, do we always need the backend? I mean, does it make > sense to always remove/unbind the frontend in case the backend is unbound? > In STM usecases we may have severals backends linked to a frontend. But I cannot find a usecase where it may be necessary to remove a backend while keeping the others alive. So from my side it is acceptable to tear everythings down if a backend if removed. Olivier > Maybe some of your usecases already "forces" us with a decision. > > The biggest pro I see is code simplicity. If we can assume the frontend can > never exist in case one of the backends is gone, we can: > > * get rid of the sync mutex; > * get rid of the kref and bind the backend object lifetime to the backend > device (using devm_kzalloc() instead of kzalloc + refcount. > > Basically, we would not need to care about syncing the backend existence with > accessing it... > To sum up, the device_links approach tends to be similar (not identical) to the > previous approach using the component API. > > The biggest pro I see in Bartosz's stuff is flexibility. So it should just work > in whatever future usecases we might have. I fear that going the device_links > road we might end up needing this stuff anyways. > > Obviously, the biggest con is code complexity (not that bad though) as we always > need to properly sync any backend callback. > > - Nuno Sá >>
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. + 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; +}; + +/* + * Helper struct for requesting buffers. Allows for multiple buffers per + * backend. + */ +struct iio_backend_buffer_pair { + struct iio_backend *back; + struct iio_buffer *buffer; +}; + +static LIST_HEAD(iio_back_list); +static DEFINE_MUTEX(iio_back_lock); + +/* + * Helper macros to properly call backend ops. The main point for these macros + * is to properly lock the backend mutex on every call plus checking if the + * backend device is still around (by looking at the *ops pointer). + */ + +#define iio_backend_check_op(back, op) ({ \ + struct iio_backend *____back = back; \ + int ____ret = 0; \ + \ + lockdep_assert_held(&____back->lock); \ + if (!____back->ops) { \ + pr_warn_once("Backend is no longer available\n"); \ + ____ret = -ENODEV; \ + } else if (!____back->ops->op) { \ + ____ret = -EOPNOTSUPP; \ + } \ + \ + ____ret; \ +}) + +#define iio_backend_op_call(back, op, args...) ({ \ + struct iio_backend *__back = back; \ + int __ret; \ + \ + guard(mutex)(&__back->lock); \ + __ret = iio_backend_check_op(__back, op); \ + if (!__ret) \ + __ret = __back->ops->op(__back, ##args); \ + \ + __ret; \ +}) + +#define iio_backend_ptr_op_call(back, op, args...) ({ \ + struct iio_backend *__back = back; \ + void *ptr_err; \ + int __ret; \ + \ + guard(mutex)(&__back->lock); \ + __ret = iio_backend_check_op(__back, op); \ + if (__ret) \ + ptr_err = ERR_PTR(__ret); \ + else \ + ptr_err = __back->ops->op(__back, ##args); \ + \ + ptr_err; \ +}) + +#define iio_backend_void_op_call(back, op, args...) { \ + struct iio_backend *__back = back; \ + int __ret; \ + \ + guard(mutex)(&__back->lock); \ + __ret = iio_backend_check_op(__back, op); \ + if (!__ret) \ + __back->ops->op(__back, ##args); \ +} + +/** + * iio_backend_chan_enable - Enable a backend channel. + * @back: Backend device. + * @chan: Channel number. + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan) +{ + return iio_backend_op_call(back, chan_enable, chan); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_chan_enable, IIO_BACKEND); + +/** + * iio_backend_chan_disable - Disable a backend channel. + * @back: Backend device. + * @chan: Channel number. + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_chan_disable(struct iio_backend *back, unsigned int chan) +{ + return iio_backend_op_call(back, chan_disable, chan); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_chan_disable, IIO_BACKEND); + +/** + * iio_backend_chan_enable - Enable the backend. + * @back: Backend device + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_enable(struct iio_backend *back) +{ + return iio_backend_op_call(back, enable); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_enable, IIO_BACKEND); + +/** + * iio_backend_disable - Disable the backend. + * @back: Backend device + */ +void iio_backend_disable(struct iio_backend *back) +{ + iio_backend_void_op_call(back, disable); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_disable, IIO_BACKEND); + +/** + * iio_backend_data_format_set - Configure the channel data format + * @back: Backend device + * @chan: Channel number. + * @data: Data format. + * + * Properly configure a channel with respect to the expected data format. A + * @struct iio_backend_data_fmt must be passed with the settings. + * + * RETURNS: + * 0 on success, negative error number on failure + */ +int iio_backend_data_format_set(struct iio_backend *back, unsigned int chan, + const struct iio_backend_data_fmt *data) +{ + if (!data || data->type >= IIO_BACKEND_DATA_TYPE_MAX) + return -EINVAL; + + return iio_backend_op_call(back, data_format_set, chan, data); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_data_format_set, IIO_BACKEND); + +static void iio_backend_free_buffer(void *arg) +{ + struct iio_backend_buffer_pair *pair = arg; + + iio_backend_void_op_call(pair->back, free_buffer, pair->buffer); +} + +/** + * devm_iio_backend_request_buffer - Request an IIO buffer from the backend. + * @dev: Device to bind the buffer lifetime. + * @back: Backend device. + * @indio_dev: IIO device. + * + * Request an IIO buffer from the backend. The type of the buffer (typically + * INDIO_BUFFER_HARDWARE) is up to the backend to decide. This is because, + * normally, the backend dictates what kind of buffering we can get. + * + * The backend .free_buffer() hooks is automatically called on @dev detach. + * + * RETURNS: + * 0 on success, negative error number on failure + */ +int devm_iio_backend_request_buffer(struct device *dev, + struct iio_backend *back, + struct iio_dev *indio_dev) +{ + struct iio_backend_buffer_pair *pair; + struct iio_buffer *buffer; + + buffer = iio_backend_ptr_op_call(back, request_buffer, indio_dev); + if (IS_ERR(buffer)) + return PTR_ERR(buffer); + + /* backend lock should not be need after getting the buffer */ + pair = devm_kzalloc(dev, sizeof(*pair), GFP_KERNEL); + if (!pair) + return -ENOMEM; + + /* weak reference should be all what we need */ + pair->back = back; + pair->buffer = buffer; + + return devm_add_action_or_reset(dev, iio_backend_free_buffer, pair); +} +EXPORT_SYMBOL_NS_GPL(devm_iio_backend_request_buffer, IIO_BACKEND); + +static void iio_backend_free(struct kref *ref) +{ + struct iio_backend *back = container_of(ref, struct iio_backend, ref); + + kfree(back); +} + +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)); + 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)); + + 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); + 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); + +/** + * 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. + * 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. + * + * 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); + +/** + * iio_backend_get_priv - Get driver private data + * @back: Backend device + */ +void *iio_backend_get_priv(const struct iio_backend *back) +{ + return back->priv; +} +EXPORT_SYMBOL_NS_GPL(iio_backend_get_priv, IIO_BACKEND); + +static void iio_backend_unregister(void *arg) +{ + struct iio_backend *back = arg; + + mutex_lock(&iio_back_lock); + list_del(&back->entry); + mutex_unlock(&iio_back_lock); + + mutex_lock(&back->lock); + back->ops = NULL; + mutex_unlock(&back->lock); + kref_put(&back->ref, iio_backend_free); +} + +/** + * devm_iio_backend_register - Register a new backend device + * @dev: Backend device being registered. + * @ops: Backend ops + * @priv: Device private data. + * + * @ops and @priv are both mandatory. Not providing them results in -EINVAL. + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int devm_iio_backend_register(struct device *dev, + const struct iio_backend_ops *ops, void *priv) +{ + struct iio_backend *back; + + if (!ops || !priv) { + pr_err("%s: No backend ops or private data given\n", + dev_name(dev)); + return -EINVAL; + } + + back = kzalloc(sizeof(*back), GFP_KERNEL); + if (!back) + return -ENOMEM; + + kref_init(&back->ref); + mutex_init(&back->lock); + back->ops = ops; + back->owner = dev->driver->owner; + back->dev = dev; + back->priv = priv; + mutex_lock(&iio_back_lock); + list_add(&back->entry, &iio_back_list); + mutex_unlock(&iio_back_lock); + + return devm_add_action_or_reset(dev, iio_backend_unregister, back); +} +EXPORT_SYMBOL_NS_GPL(devm_iio_backend_register, IIO_BACKEND); + +MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>"); +MODULE_DESCRIPTION("Framework to handle complex IIO aggregate devices"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h new file mode 100644 index 000000000000..239d4db08cee --- /dev/null +++ b/include/linux/iio/backend.h @@ -0,0 +1,75 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +#ifndef _IIO_BACKEND_H_ +#define _IIO_BACKEND_H_ + +#include <linux/types.h> + +struct fwnode_handle; +struct iio_backend; +struct device; +struct iio_dev; + +enum iio_backend_data_type { + IIO_BACKEND_TWOS_COMPLEMENT, + IIO_BACKEND_OFFSET_BINARY, + IIO_BACKEND_DATA_TYPE_MAX +}; + +/** + * struct iio_backend_data_fmt - Backend data format + * @type: Data type. + * @sign_extend: Bool to tell if the data is sign extended. + * @enable: Enable/Disable the data format module. If disabled, + * not formatting will happen. + */ +struct iio_backend_data_fmt { + enum iio_backend_data_type type; + bool sign_extend; + bool enable; +}; + +/** + * struct iio_backend_ops - operations structure for an iio_backend + * @enable: Enable backend. + * @disable: Disable backend. + * @chan_enable: Enable one channel. + * @chan_disable: Disable one channel. + * @data_format_set: Configure the data format for a specific channel. + * @request_buffer: Request an IIO buffer. + * @free_buffer: Free an IIO buffer. + **/ +struct iio_backend_ops { + int (*enable)(struct iio_backend *back); + void (*disable)(struct iio_backend *back); + int (*chan_enable)(struct iio_backend *back, unsigned int chan); + int (*chan_disable)(struct iio_backend *back, unsigned int chan); + int (*data_format_set)(struct iio_backend *back, unsigned int chan, + const struct iio_backend_data_fmt *data); + struct iio_buffer *(*request_buffer)(struct iio_backend *back, + struct iio_dev *indio_dev); + void (*free_buffer)(struct iio_backend *back, + struct iio_buffer *buffer); +}; + +int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan); +int iio_backend_chan_disable(struct iio_backend *back, unsigned int chan); +int iio_backend_enable(struct iio_backend *back); +void iio_backend_disable(struct iio_backend *back); +int iio_backend_data_format_set(struct iio_backend *back, unsigned int chan, + const struct iio_backend_data_fmt *data); +int devm_iio_backend_request_buffer(struct device *dev, + struct iio_backend *back, + struct iio_dev *indio_dev); + +void *iio_backend_get_priv(const struct iio_backend *conv); +struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name); +struct iio_backend *devm_iio_backend_get_optional(struct device *dev, + const char *name); +struct iio_backend * +devm_iio_backend_get_from_fwnode_lookup(struct device *dev, + struct fwnode_handle *fwnode); + +int devm_iio_backend_register(struct device *dev, + const struct iio_backend_ops *ops, void *priv); + +#endif
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> --- 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(+)