| Message ID | 20231220-iio-backend-v4-0-998e9148b692@analog.com |
|---|---|
| Headers | show |
| Series | iio: add new backend framework | expand |
On Wed, 20 Dec 2023 10:56:24 -0600 Rob Herring <robh@kernel.org> wrote: > On Wed, 20 Dec 2023 16:34:04 +0100, Nuno Sa wrote: > > The ad9467 will make use of the new IIO backend framework which is a > > provider - consumer interface where IIO backends provide services to > > consumers. As such, and being this device a consumer, add the new > > generic io-backend property to the bindings. > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > --- > > Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml: io-backends: missing type definition For reference this is expected as if this goes ahead that will be a dtschema addition. > > doc reference errors (make refcheckdocs): > > See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231220-iio-backend-v4-1-998e9148b692@analog.com > > The base for the series is generally the latest rc1. A different dependency > should be noted in *this* patch. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit after running the above command yourself. Note > that DT_SCHEMA_FILES can be set to your schema file to speed up checking > your schema. However, it must be unset to test all examples with your schema. >
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 Wed, 20 Dec 2023 16:34:04 +0100, Nuno Sa wrote: > The ad9467 will make use of the new IIO backend framework which is a > provider - consumer interface where IIO backends provide services to > consumers. As such, and being this device a consumer, add the new > generic io-backend property to the bindings. > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > --- > Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > Reviewed-by: Rob Herring <robh@kernel.org>
On Wed, 20 Dec 2023 16:34:07 +0100, Nuno Sa wrote: > From: Olivier Moysan <olivier.moysan@foss.st.com> > > Add support for creating device links out of more DT properties. > > Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com> > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > --- > drivers/of/property.c | 2 ++ > 1 file changed, 2 insertions(+) > Acked-by: Rob Herring <robh@kernel.org>
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
v1: https://lore.kernel.org/linux-iio/20231204144925.4fe9922f@jic23-huawei/T/#m222f5175273b81dbfe40b7f0daffcdc67d6cb8ff v2: https://lore.kernel.org/r/20231208-dev-iio-backend-v2-0-5450951895e1@analog.com v3: https://lore.kernel.org/linux-iio/20231213-dev-iio-backend-v3-0-bb9f12a5c6dc@analog.com/ Changes in v4: - Patch 1: * Don't make io-backends mandatory. - Patch 5: * Add helper __devm_iio_backend_get(); * Add new devm_iio_backend_get_optional(); * Add new devm_iio_backend_get_from_fwnode_lookup(); * Convert all dev_ logs to plain pr_ logs so we actually use pr_fmt. - Patch 6: * Implement the fallback backend lookput in case devm_iio_backend_get_optional() fails; So the main difference in this series is to still handle 'adi,adc-dev' in case 'io-backends' is not given. Hence we don't need to make it mandatory and break dt ABI for the ad9467 bindings. Rob, it would be nice if you can take a quick a look on it and let me know if there's a more efficient way of doing things. Rob also already expressed that he is ok with the dt ABI breakage as long as it is properly justified in the commit message. Jonathan, we should see if we keep things as in this version or rollback. Personally I'm not that "happy" with that devm_iio_backend_get_from_fwnode_lookup() given the fact that is not really supposed to be used anymore. OTOH, it's small and simple... So, let me know your preference and I'll go with that :). This is likely the last version of this series for this year. If things go well, I plan to start working on the second user of the framework right in the beginning of the next year... Keeping the block diagram in v3's cover so we don't have to follow links to check one of the typical setups. ------------------------------------------------------- ------------------ | ----------- ------------ ------- FPGA | | ADC |------------------------| | AXI ADC |---------| DMA CORE |------| RAM | | | (Frontend/IIO) | Serial Data (eg: LVDS) | |(backend)|---------| |------| | | | |------------------------| ----------- ------------ ------- | ------------------ ------------------------------------------------------- --- Nuno Sa (7): dt-bindings: adc: ad9467: add new io-backend property dt-bindings: adc: axi-adc: deprecate 'adi,adc-dev' driver: core: allow modifying device_links flags iio: buffer-dmaengine: export buffer alloc and free functions iio: add the IIO backend framework iio: adc: ad9467: convert to backend framework iio: adc: adi-axi-adc: move to backend framework Olivier Moysan (1): of: property: add device link support for io-backends .../devicetree/bindings/iio/adc/adi,ad9467.yaml | 4 + .../devicetree/bindings/iio/adc/adi,axi-adc.yaml | 4 +- MAINTAINERS | 8 + drivers/base/core.c | 14 +- drivers/iio/Kconfig | 5 + drivers/iio/Makefile | 1 + drivers/iio/adc/Kconfig | 4 +- drivers/iio/adc/ad9467.c | 282 ++++++++----- drivers/iio/adc/adi-axi-adc.c | 381 +++++------------ drivers/iio/buffer/industrialio-buffer-dmaengine.c | 8 +- drivers/iio/industrialio-backend.c | 456 +++++++++++++++++++++ drivers/of/property.c | 2 + include/linux/iio/adc/adi-axi-adc.h | 68 --- include/linux/iio/backend.h | 75 ++++ include/linux/iio/buffer-dmaengine.h | 3 + 15 files changed, 845 insertions(+), 470 deletions(-) --- base-commit: 2dfef50589aef3b9a2fa2190ae95b328fb664f89 change-id: 20231219-iio-backend-a3dc1a6a7a58 -- Thanks! - Nuno Sá