[PATCH 00/12] iio: add new backend framework

[PATCH 00/12] iio: add new backend framework

[Nuno Sa via B4 Relay]

Hi all,

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()

(typical provider - consumer stuff)

This is the result of the discussions in [1] and [2]. In short, both ADI
and STM wanted some way to control/get configurations from a kind of
IIO aggregate device. So discussions were made to have something that
serves and can be used by everyone.

The main differences with the converter framework RFC [1]:

1) Dropped the component framework. One can get more overview about
the concerns on the references but the main reasons were: 
 * Relying on providing .remove() callbacks to be allowed to use device
   managed functions. I was not even totally sure about the correctness
   of it and in times where everyone tries to avoid that driver
   callback, it could lead to some maintenance burden.
 * Scalability issues. As mentioned in [2], to support backends defined
   in FW child nodes was not so straightforward with the component
   framework.
 * Device links can already do some of the things that made me
   try the component framework (eg: removing consumers on suppliers
   unbind).

2) Only support the minimal set of functionality to have the devices in
   the same state as before using the backend framework. New features
   will be added afterwards. 

3) Moved the API docs into the .c files.

4) Moved the framework to the IIO top dir and renamed it to
   industrialio-backend.c.

Also, as compared with the RFC in [2], I don't think there are that many
similarities other than the filename. However, it should now be pretty
straight for Olivier to build on top of it. Also to mention that I did
grabbed patch 1 ("of: property: add device link support for
io-backends") from that series and just did some minor changes:

1) Renamed the property from "io-backend" to "io-backends".
2) No '#io-backend-cells' as it's not supported/needed by the framework
(at least for now) .

Regarding the driver core patch
("driver: core: allow modifying device_links flags"), it is more like a
RFC one. I'm not really sure if the current behavior isn't just
expected/wanted. Since I could not really understand if it is or not
(or why the different handling DL_FLAG_AUTOREMOVE_CONSUMER vs
DL_FLAG_AUTOREMOVE_SUPPLIER), I'm sending out the patch.

Jonathan,

I also have some fixes and cleanups for the ad9467 driver. I added
Fixes tags but I'm not sure if it's really worth it to backport them (given
what we already discussed about these drivers). I'll leave that to you
:).

I'm also not sure if I'm missing some tags (even though the series
is frankly different from [2]). 

Olivier,

If you want to be included as a Reviewer let me know and I'll happily do
so in the next version.

Also regarding the new IIO fw schemas. Should I send patches/PR to:

https://github.com/devicetree-org/dt-schema/

? Or is there any other workflow for it?

[1]: https://lore.kernel.org/linux-iio/20230727150324.1157933-1-olivier.moysan@foss.st.com/
[2]: https://lore.kernel.org/linux-iio/20230727150324.1157933-1-olivier.moysan@foss.st.com/

---
Nuno Sa (11):
      driver: core: allow modifying device_links flags
      iio: add the IIO backend framework
      iio: adc: ad9467: fix reset gpio handling
      iio: adc: ad9467: don't ignore error codes
      iio: adc: ad9467: add mutex to struct ad9467_state
      iio: adc: ad9467: fix scale setting
      iio: adc: ad9467: use spi_get_device_match_data()
      iio: adc: ad9467: use chip_info variables instead of array
      iio: adc: ad9467: convert to backend framework
      iio: adc: adi-axi-adc: convert to regmap
      iio: adc: adi-axi-adc: move to backend framework

Olivier Moysan (1):
      of: property: add device link support for io-backends

 MAINTAINERS                         |   7 +
 drivers/base/core.c                 |  14 +-
 drivers/iio/Kconfig                 |   5 +
 drivers/iio/Makefile                |   1 +
 drivers/iio/adc/Kconfig             |   3 +-
 drivers/iio/adc/ad9467.c            | 382 +++++++++++++++++++++-----------
 drivers/iio/adc/adi-axi-adc.c       | 429 +++++++-----------------------------
 drivers/iio/industrialio-backend.c  | 302 +++++++++++++++++++++++++
 drivers/of/property.c               |   2 +
 include/linux/iio/adc/adi-axi-adc.h |   4 +
 include/linux/iio/backend.h         |  58 +++++
 11 files changed, 723 insertions(+), 484 deletions(-)

Thanks!
- Nuno Sá

[PATCH 01/12] driver: core: allow modifying device_links flags

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

If a device_link is previously created (eg: via
fw_devlink_create_devlink()) before the supplier + consumer are both
present and bound to their respective drivers, there's no way to set
DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
DL_FLAG_AUTOREMOVE_SUPPLIER is done.

While at it, make sure that we are never left with
DL_FLAG_AUTOPROBE_CONSUMER set together with one of
DL_FLAG_AUTOREMOVE_CONSUMER or DL_FLAG_AUTOREMOVE_SUPPLIER.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/base/core.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4d8b315c48a1..b6aac55c361d 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -806,11 +806,15 @@ struct device_link *device_link_add(struct device *consumer,
 		 * update the existing link to stay around longer.
 		 */
 		if (flags & DL_FLAG_AUTOREMOVE_SUPPLIER) {
-			if (link->flags & DL_FLAG_AUTOREMOVE_CONSUMER) {
-				link->flags &= ~DL_FLAG_AUTOREMOVE_CONSUMER;
-				link->flags |= DL_FLAG_AUTOREMOVE_SUPPLIER;
-			}
-		} else if (!(flags & DL_FLAG_AUTOREMOVE_CONSUMER)) {
+			link->flags &= ~DL_FLAG_AUTOREMOVE_CONSUMER;
+			link->flags &= ~DL_FLAG_AUTOPROBE_CONSUMER;
+			link->flags |= DL_FLAG_AUTOREMOVE_SUPPLIER;
+
+		} else if (flags & DL_FLAG_AUTOREMOVE_CONSUMER) {
+			link->flags &= ~DL_FLAG_AUTOREMOVE_SUPPLIER;
+			link->flags &= ~DL_FLAG_AUTOPROBE_CONSUMER;
+			link->flags |= DL_FLAG_AUTOREMOVE_CONSUMER;
+		} else {
 			link->flags &= ~(DL_FLAG_AUTOREMOVE_CONSUMER |
 					 DL_FLAG_AUTOREMOVE_SUPPLIER);
 		}

-- 
2.42.1

[PATCH 02/12] of: property: add device link support for io-backends

[Nuno Sa via B4 Relay]

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(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index cf8dacf3e3b8..40a157daf08f 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1244,6 +1244,7 @@ DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells")
 DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
 DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
 DEFINE_SIMPLE_PROP(io_channels, "io-channel", "#io-channel-cells")
+DEFINE_SIMPLE_PROP(io_backends, "io-backends", NULL)
 DEFINE_SIMPLE_PROP(interrupt_parent, "interrupt-parent", NULL)
 DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
 DEFINE_SIMPLE_PROP(power_domains, "power-domains", "#power-domain-cells")
@@ -1333,6 +1334,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_iommu_maps, .optional = true, },
 	{ .parse_prop = parse_mboxes, },
 	{ .parse_prop = parse_io_channels, },
+	{ .parse_prop = parse_io_backends, },
 	{ .parse_prop = parse_interrupt_parent, },
 	{ .parse_prop = parse_dmas, .optional = true, },
 	{ .parse_prop = parse_power_domains, },

-- 
2.42.1

[PATCH 03/12] iio: add the IIO backend framework

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

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                        |   7 +
 drivers/iio/Kconfig                |   5 +
 drivers/iio/Makefile               |   1 +
 drivers/iio/industrialio-backend.c | 302 +++++++++++++++++++++++++++++++++++++
 include/linux/iio/backend.h        |  58 +++++++
 5 files changed, 373 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c885d784dcb1..c20e288f23f0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10163,6 +10163,13 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	drivers/media/rc/iguanair.c
 
+IIO BACKEND FRAMEWORK
+M:	Nuno Sa <nuno.sa@analog.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..b11fcb2195c5
--- /dev/null
+++ b/drivers/iio/industrialio-backend.c
@@ -0,0 +1,302 @@
+// 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/list.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 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_op_call(back, op, args...) ({ \
+	struct iio_backend *__back = back; \
+	int __ret; \
+			\
+	guard(mutex)(&__back->lock); \
+	if (WARN_ON_ONCE(!__back->ops)) \
+		__ret = -ENODEV; \
+	else if (!__back->ops->op) \
+		__ret = -EOPNOTSUPP; \
+	else \
+		__ret = __back->ops->op(__back, ##args); \
+	\
+	__ret; \
+})
+
+#define iio_backend_void_op_call(back, op, args...) { \
+	struct iio_backend *__back = back; \
+						\
+	guard(mutex)(&__back->lock); \
+	WARN_ON_ONCE(!__back->ops); \
+		\
+	if (__back->ops && __back->ops->op) \
+		__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_GPL(iio_backend_chan_enable);
+
+/**
+ * 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_GPL(iio_backend_chan_disable);
+
+/**
+ * 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_GPL(iio_backend_enable);
+
+/**
+ * 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_GPL(iio_backend_disable);
+
+/**
+ * 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_GPL(iio_backend_data_format_set);
+
+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);
+}
+
+/**
+ * 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)) {
+		dev_err(dev, "Cannot get Firmware reference\n");
+		return ERR_CAST(fwnode);
+	}
+
+	guard(mutex)(&iio_back_lock);
+	list_for_each_entry(back, &iio_back_list, entry) {
+		struct device_link *link;
+
+		if (!device_match_fwnode(back->dev, fwnode))
+			continue;
+
+		fwnode_handle_put(fwnode);
+		kref_get(&back->ref);
+		if (!try_module_get(back->owner)) {
+			dev_err(dev, "Cannot get module reference\n");
+			return ERR_PTR(-ENODEV);
+		}
+
+		ret = devm_add_action_or_reset(dev, iio_backend_release, back);
+		if (ret)
+			return ERR_PTR(ret);
+
+		link = device_link_add(dev, back->dev,
+				       DL_FLAG_AUTOREMOVE_CONSUMER);
+		if (!link)
+			dev_warn(dev, "Could not link to supplier(%s)\n",
+				 dev_name(back->dev));
+
+		dev_dbg(dev, "Found backend(%s) device\n", dev_name(back->dev));
+		return back;
+	}
+
+	fwnode_handle_put(fwnode);
+	return ERR_PTR(-EPROBE_DEFER);
+}
+EXPORT_SYMBOL_GPL(devm_iio_backend_get);
+
+/**
+ * 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_GPL(iio_backend_get_priv);
+
+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) {
+		dev_err(dev, "No backend ops or private data given\n");
+		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_GPL(devm_iio_backend_register);
+
+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..8ae6549b8246
--- /dev/null
+++ b/include/linux/iio/backend.h
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef _IIO_BACKEND_H_
+#define _IIO_BACKEND_H_
+
+#include <linux/types.h>
+
+struct iio_backend;
+struct device;
+
+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.
+ **/
+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 *conv, unsigned int chan,
+			       const struct iio_backend_data_fmt *data);
+};
+
+int iio_backend_chan_disable(struct iio_backend *back, unsigned int chan);
+int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan);
+void iio_backend_disable(struct iio_backend *back);
+int iio_backend_enable(struct iio_backend *back);
+int iio_backend_data_format_set(struct iio_backend *back, unsigned int chan,
+				const struct iio_backend_data_fmt *data);
+
+void *iio_backend_get_priv(const struct iio_backend *conv);
+struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name);
+int devm_iio_backend_register(struct device *dev,
+			      const struct iio_backend_ops *ops, void *priv);
+
+#endif

-- 
2.42.1

Re: [PATCH 03/12] iio: add the IIO backend framework

[Jonathan Cameron]

On Tue, 21 Nov 2023 11:20:16 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:

> From: Nuno Sa <nuno.sa@analog.com>
> 
> 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).

As this is first place backend / frontend terminology used (I think), make
sure to give an example so people understand what sorts of IP / devices thes
might be.

> 
> 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>

Looks good to me in general.  I'll need to have a really close read though
before we merge this as there may be sticky corners! (hopefully not)


...

> +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).
If just checking if it is around rather thank looking for a bug, then
I'd suggest a lighter choice than WARN_ON_x 

Btw, there were some interesting discussions on lifetimes and consumer / provider
models at plumbers. I think https://www.youtube.com/watch?v=bHaMMnIH6AM will be
the video.   Suggested the approach of not refcounting but instead allowing for
a deliberate removal of access similar to your check on ops here (and the one
we do in core IIO for similar purposes).  Sounded interesting but I've not
explored what it would really mean to switch to that model yet.

> + */
> +#define iio_backend_op_call(back, op, args...) ({ \
> +	struct iio_backend *__back = back; \
> +	int __ret; \
> +			\
> +	guard(mutex)(&__back->lock); \
> +	if (WARN_ON_ONCE(!__back->ops)) \
> +		__ret = -ENODEV; \
> +	else if (!__back->ops->op) \
> +		__ret = -EOPNOTSUPP; \
> +	else \
> +		__ret = __back->ops->op(__back, ##args); \
> +	\
> +	__ret; \
> +})

Re: [PATCH 03/12] iio: add the IIO backend framework

[Nuno Sá]

On Mon, 2023-12-04 at 15:38 +0000, Jonathan Cameron wrote:
> On Tue, 21 Nov 2023 11:20:16 +0100
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > 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).
> 
> As this is first place backend / frontend terminology used (I think), make
> sure to give an example so people understand what sorts of IP / devices thes
> might be.
> 
> > 
> > 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>
> 
> Looks good to me in general.  I'll need to have a really close read though
> before we merge this as there may be sticky corners! (hopefully not)
> 
> 
> ...
> 
> > +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).
> If just checking if it is around rather thank looking for a bug, then
> I'd suggest a lighter choice than WARN_ON_x 
> 

Arguably, in here, removing a backend is the user doing something seriously wrong so
I see the splat with good eyes :D.

That said, I'm fine in turning this into a pr_warn_once()...

> Btw, there were some interesting discussions on lifetimes and consumer / provider
> models at plumbers. I think https://www.youtube.com/watch?v=bHaMMnIH6AM will be
> the video.   Suggested the approach of not refcounting but instead allowing for
> a deliberate removal of access similar to your check on ops here (and the one
> we do in core IIO for similar purposes).  Sounded interesting but I've not
> explored what it would really mean to switch to that model yet.

Yes, interesting talk indeed. I have been following this issue for some time now.
That's why I tried to be careful in the backend stuff (so we don't explode if a
backend is gone) even though is a much more simpler approach. But the talk mentions
three solutions and we kind of have both option C (the pointer stuff) and option A
(consumer removed on provicer unbind)
in here. option A is being given through device links with the AUTO_REMOVE_CONSUMER
flag.

And the talk actually left me thinking on that (as it's discussed in there. In our
simpler case (ADI ones), it does make sense to remove the consumer if the provider is
not there. But if we think in more advanced usecases (or maybe already in the STM
usecase) where we have a backend per data path. Does it make sense to completely
"kill" the consumer if we remove one of the data paths? Starting to think it
doesn't...

- Nuno Sá

Re: [PATCH 03/12] iio: add the IIO backend framework

[Jonathan Cameron]

On Wed, 06 Dec 2023 13:05:53 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2023-12-04 at 15:38 +0000, Jonathan Cameron wrote:
> > On Tue, 21 Nov 2023 11:20:16 +0100
> > Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> >   
> > > From: Nuno Sa <nuno.sa@analog.com>
> > > 
> > > 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).  
> > 
> > As this is first place backend / frontend terminology used (I think), make
> > sure to give an example so people understand what sorts of IP / devices thes
> > might be.
> >   
> > > 
> > > 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>  
> > 
> > Looks good to me in general.  I'll need to have a really close read though
> > before we merge this as there may be sticky corners! (hopefully not)
> > 
> > 
> > ...
> >   
> > > +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).  
> > If just checking if it is around rather thank looking for a bug, then
> > I'd suggest a lighter choice than WARN_ON_x 
> >   
> 
> Arguably, in here, removing a backend is the user doing something seriously wrong so
> I see the splat with good eyes :D.
> 
> That said, I'm fine in turning this into a pr_warn_once()...
> 
> > Btw, there were some interesting discussions on lifetimes and consumer / provider
> > models at plumbers. I think https://www.youtube.com/watch?v=bHaMMnIH6AM will be
> > the video.   Suggested the approach of not refcounting but instead allowing for
> > a deliberate removal of access similar to your check on ops here (and the one
> > we do in core IIO for similar purposes).  Sounded interesting but I've not
> > explored what it would really mean to switch to that model yet.  
> 
> Yes, interesting talk indeed. I have been following this issue for some time now.
> That's why I tried to be careful in the backend stuff (so we don't explode if a
> backend is gone) even though is a much more simpler approach. But the talk mentions
> three solutions and we kind of have both option C (the pointer stuff) and option A
> (consumer removed on provicer unbind)
> in here. option A is being given through device links with the AUTO_REMOVE_CONSUMER
> flag.
> 
> And the talk actually left me thinking on that (as it's discussed in there. In our
> simpler case (ADI ones), it does make sense to remove the consumer if the provider is
> not there. But if we think in more advanced usecases (or maybe already in the STM
> usecase) where we have a backend per data path. Does it make sense to completely
> "kill" the consumer if we remove one of the data paths? Starting to think it
> doesn't...

There is a reasonably argument that partial tear down isn't a common case. So
may not be worth worrying about.

J
> 
> - Nuno Sá
> 

[PATCH 04/12] iio: adc: ad9467: fix reset gpio handling

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

The reset gpio was being requested with GPIOD_OUT_LOW which means, not
asserted. Then it was being asserted but never de-asserted which means
the devices was left in reset. Fix it by de-asserting the gpio.

While at it, moved the handling to it's own function and dropped
'reset_gpio' from the 'struct ad9467_state' as we only need it during
probe. On top of that, refactored things so that we now request the gpio
asserted (i.e in reset) and then de-assert it.

Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/ad9467.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 39eccc28debe..368ea57be117 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -121,7 +121,6 @@ struct ad9467_state {
 	unsigned int			output_mode;
 
 	struct gpio_desc		*pwrdown_gpio;
-	struct gpio_desc		*reset_gpio;
 };
 
 static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
@@ -378,6 +377,23 @@ static int ad9467_preenable_setup(struct adi_axi_adc_conv *conv)
 	return ad9467_outputmode_set(st->spi, st->output_mode);
 }
 
+static int ad9467_reset(struct device *dev)
+{
+	struct gpio_desc *gpio;
+
+	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(gpio))
+		return PTR_ERR(gpio);
+	if (!gpio)
+		return 0;
+
+	fsleep(1);
+	gpiod_direction_output(gpio, 0);
+	fsleep(10);
+
+	return 0;
+}
+
 static int ad9467_probe(struct spi_device *spi)
 {
 	const struct ad9467_chip_info *info;
@@ -408,18 +424,9 @@ static int ad9467_probe(struct spi_device *spi)
 	if (IS_ERR(st->pwrdown_gpio))
 		return PTR_ERR(st->pwrdown_gpio);
 
-	st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
-						 GPIOD_OUT_LOW);
-	if (IS_ERR(st->reset_gpio))
-		return PTR_ERR(st->reset_gpio);
-
-	if (st->reset_gpio) {
-		udelay(1);
-		ret = gpiod_direction_output(st->reset_gpio, 1);
-		if (ret)
-			return ret;
-		mdelay(10);
-	}
+	ret = ad9467_reset(&spi->dev);
+	if (ret)
+		return ret;
 
 	conv->chip_info = &info->axi_adc_info;
 

-- 
2.42.1

Re: [PATCH 04/12] iio: adc: ad9467: fix reset gpio handling

[David Lechner]

On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
<devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> From: Nuno Sa <nuno.sa@analog.com>
>
> The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> asserted. Then it was being asserted but never de-asserted which means
> the devices was left in reset. Fix it by de-asserting the gpio.

It could be helpful to update the devicetree bindings to state the
expected active-high or active-low setting for this gpio so it is
clear which state means asserted.

>
> While at it, moved the handling to it's own function and dropped
> 'reset_gpio' from the 'struct ad9467_state' as we only need it during
> probe. On top of that, refactored things so that we now request the gpio
> asserted (i.e in reset) and then de-assert it.
>
> Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/adc/ad9467.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index 39eccc28debe..368ea57be117 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c
> @@ -121,7 +121,6 @@ struct ad9467_state {
>         unsigned int                    output_mode;
>
>         struct gpio_desc                *pwrdown_gpio;
> -       struct gpio_desc                *reset_gpio;
>  };
>
>  static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> @@ -378,6 +377,23 @@ static int ad9467_preenable_setup(struct adi_axi_adc_conv *conv)
>         return ad9467_outputmode_set(st->spi, st->output_mode);
>  }
>
> +static int ad9467_reset(struct device *dev)
> +{
> +       struct gpio_desc *gpio;
> +
> +       gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +       if (IS_ERR(gpio))
> +               return PTR_ERR(gpio);
> +       if (!gpio)
> +               return 0;

can be done in one test instead of 2:

if (IS_ERR_OR_NULL(gpio))
        return PTR_ERR_OR_ZERO(gpio);

> +
> +       fsleep(1);
> +       gpiod_direction_output(gpio, 0);
> +       fsleep(10);

Previous version was 10 milliseconds instead of 10 microseconds. Was
this change intentional? If yes, it should be mentioned it in the
commit message.

> +
> +       return 0;
> +}
> +
>  static int ad9467_probe(struct spi_device *spi)
>  {
>         const struct ad9467_chip_info *info;
> @@ -408,18 +424,9 @@ static int ad9467_probe(struct spi_device *spi)
>         if (IS_ERR(st->pwrdown_gpio))
>                 return PTR_ERR(st->pwrdown_gpio);
>
> -       st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
> -                                                GPIOD_OUT_LOW);
> -       if (IS_ERR(st->reset_gpio))
> -               return PTR_ERR(st->reset_gpio);
> -
> -       if (st->reset_gpio) {
> -               udelay(1);
> -               ret = gpiod_direction_output(st->reset_gpio, 1);
> -               if (ret)
> -                       return ret;
> -               mdelay(10);
> -       }
> +       ret = ad9467_reset(&spi->dev);
> +       if (ret)
> +               return ret;
>
>         conv->chip_info = &info->axi_adc_info;
>
>
> --
> 2.42.1
>
>

Re: [PATCH 04/12] iio: adc: ad9467: fix reset gpio handling

[Nuno Sá]

On Thu, 2023-11-30 at 15:41 -0600, David Lechner wrote:
> On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> > asserted. Then it was being asserted but never de-asserted which means
> > the devices was left in reset. Fix it by de-asserting the gpio.
> 
> It could be helpful to update the devicetree bindings to state the
> expected active-high or active-low setting for this gpio so it is
> clear which state means asserted.
> 

You could state that the chip is active low but I don't see that change that
important for now. Not sure if this is clear and maybe that's why your comment.
GPIOD_OUT_HIGH has nothing to do with active high or low. It just means, "get me the
pin in the asserted state".

> > While at it, moved the handling to it's own function and dropped
> > 'reset_gpio' from the 'struct ad9467_state' as we only need it during
> > probe. On top of that, refactored things so that we now request the gpio
> > asserted (i.e in reset) and then de-assert it.
> > 
> > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/adc/ad9467.c | 33 ++++++++++++++++++++-------------
> >  1 file changed, 20 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > index 39eccc28debe..368ea57be117 100644
> > --- a/drivers/iio/adc/ad9467.c
> > +++ b/drivers/iio/adc/ad9467.c
> > @@ -121,7 +121,6 @@ struct ad9467_state {
> >         unsigned int                    output_mode;
> > 
> >         struct gpio_desc                *pwrdown_gpio;
> > -       struct gpio_desc                *reset_gpio;
> >  };
> > 
> >  static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> > @@ -378,6 +377,23 @@ static int ad9467_preenable_setup(struct adi_axi_adc_conv
> > *conv)
> >         return ad9467_outputmode_set(st->spi, st->output_mode);
> >  }
> > 
> > +static int ad9467_reset(struct device *dev)
> > +{
> > +       struct gpio_desc *gpio;
> > +
> > +       gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +       if (IS_ERR(gpio))
> > +               return PTR_ERR(gpio);
> > +       if (!gpio)
> > +               return 0;
> 
> can be done in one test instead of 2:
> 
> if (IS_ERR_OR_NULL(gpio))
>         return PTR_ERR_OR_ZERO(gpio);
> 

Yep, better that way...

> > +
> > +       fsleep(1);
> > +       gpiod_direction_output(gpio, 0);
> > +       fsleep(10);
> 
> Previous version was 10 milliseconds instead of 10 microseconds. Was
> this change intentional? If yes, it should be mentioned it in the
> commit message.

Oh, good catch! Copy past thing with even realizing the differences in the arguments
:face_palm:

- Nuno Sá


> 

Re: [PATCH 04/12] iio: adc: ad9467: fix reset gpio handling

[David Lechner]

On Fri, Dec 1, 2023 at 2:47 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Thu, 2023-11-30 at 15:41 -0600, David Lechner wrote:
> > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > >
> > > From: Nuno Sa <nuno.sa@analog.com>
> > >
> > > The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> > > asserted. Then it was being asserted but never de-asserted which means
> > > the devices was left in reset. Fix it by de-asserting the gpio.
> >
> > It could be helpful to update the devicetree bindings to state the
> > expected active-high or active-low setting for this gpio so it is
> > clear which state means asserted.
> >
>
> You could state that the chip is active low but I don't see that change that
> important for now. Not sure if this is clear and maybe that's why your comment.
> GPIOD_OUT_HIGH has nothing to do with active high or low. It just means, "get me the
> pin in the asserted state".
>

I would assume that this bug happened in the first place because
someone forgot GPIOD_OUT_LOW in the devicetree when they were
developing the driver. So this is why I suggested that updating the
devicetree binding docs so that future users are less likely to make
the same mistake. Currently, the bindings don't even have reset-gpios
in the examples.

Re: [PATCH 04/12] iio: adc: ad9467: fix reset gpio handling

[Nuno Sá]

On Fri, 2023-12-01 at 11:01 -0600, David Lechner wrote:
> On Fri, Dec 1, 2023 at 2:47 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > On Thu, 2023-11-30 at 15:41 -0600, David Lechner wrote:
> > > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > > 
> > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > 
> > > > The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> > > > asserted. Then it was being asserted but never de-asserted which means
> > > > the devices was left in reset. Fix it by de-asserting the gpio.
> > > 
> > > It could be helpful to update the devicetree bindings to state the
> > > expected active-high or active-low setting for this gpio so it is
> > > clear which state means asserted.
> > > 
> > 
> > You could state that the chip is active low but I don't see that change that
> > important for now. Not sure if this is clear and maybe that's why your comment.
> > GPIOD_OUT_HIGH has nothing to do with active high or low. It just means, "get me
> > the
> > pin in the asserted state".
> > 
> 
> I would assume that this bug happened in the first place because
> someone forgot GPIOD_OUT_LOW in the devicetree when they were
> developing the driver. So this is why I suggested that updating the
> devicetree binding docs so that future users are less likely to make
> the same mistake. Currently, the bindings don't even have reset-gpios
> in the examples.

Hmm, I think you're missing the point... The bug has nothing to do with devicetree.
This is what was happening:

1) We were calling devm_gpiod_get_optional() with GPIOD_OUT_LOW. What this means is
that you get an output gpio deasserted. Hence the device is out of reset. And here is
the important part... what you have in dts does not matter. If you have active low,
it means the pin level will be 1. If you have high, the pin level is 0. And this is
all handled by gpiolib for you.

2) Then, we called gpiod_direction_output(..., 1), which means set the direction out
(which is actually not needed since it was already done when getting the pin) and
assert the pin. Hence, reset the device. And we were never de-asserting the pin so
the device would be left in reset.

- Nuno Sá

Re: [PATCH 04/12] iio: adc: ad9467: fix reset gpio handling

[Jonathan Cameron]

On Sat, 02 Dec 2023 09:36:47 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Fri, 2023-12-01 at 11:01 -0600, David Lechner wrote:
> > On Fri, Dec 1, 2023 at 2:47 AM Nuno Sá <noname.nuno@gmail.com> wrote:  
> > > 
> > > On Thu, 2023-11-30 at 15:41 -0600, David Lechner wrote:  
> > > > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:  
> > > > > 
> > > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > > 
> > > > > The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> > > > > asserted. Then it was being asserted but never de-asserted which means
> > > > > the devices was left in reset. Fix it by de-asserting the gpio.  
> > > > 
> > > > It could be helpful to update the devicetree bindings to state the
> > > > expected active-high or active-low setting for this gpio so it is
> > > > clear which state means asserted.
> > > >   
> > > 
> > > You could state that the chip is active low but I don't see that change that
> > > important for now. Not sure if this is clear and maybe that's why your comment.
> > > GPIOD_OUT_HIGH has nothing to do with active high or low. It just means, "get me
> > > the
> > > pin in the asserted state".
> > >   
> > 
> > I would assume that this bug happened in the first place because
> > someone forgot GPIOD_OUT_LOW in the devicetree when they were
> > developing the driver. So this is why I suggested that updating the
> > devicetree binding docs so that future users are less likely to make
> > the same mistake. Currently, the bindings don't even have reset-gpios
> > in the examples.  
> 
> Hmm, I think you're missing the point... The bug has nothing to do with devicetree.
> This is what was happening:
> 
> 1) We were calling devm_gpiod_get_optional() with GPIOD_OUT_LOW. What this means is
> that you get an output gpio deasserted. Hence the device is out of reset. And here is
> the important part... what you have in dts does not matter. If you have active low,
> it means the pin level will be 1. If you have high, the pin level is 0. And this is
> all handled by gpiolib for you. 
> 
> 2) Then, we called gpiod_direction_output(..., 1), which means set the direction out
> (which is actually not needed since it was already done when getting the pin) and
> assert the pin. Hence, reset the device. And we were never de-asserting the pin so
> the device would be left in reset.

Functionally I believe David is correct.   Flipping the DT would 'fix' this.
It's all down to a nreset vs reset pin description.

In this case I guess it's defined a a 'not reset' on the datasheet which is what
is causing the confusion.  It's not uncommon for people to refer to a reset when
they mean a "not reset" with assumptions on polarity to match.

Jonathan



> 
> - Nuno Sá

Re: [PATCH 04/12] iio: adc: ad9467: fix reset gpio handling

[Nuno Sá]

On Mon, 2023-12-04 at 15:15 +0000, Jonathan Cameron wrote:
> On Sat, 02 Dec 2023 09:36:47 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Fri, 2023-12-01 at 11:01 -0600, David Lechner wrote:
> > > On Fri, Dec 1, 2023 at 2:47 AM Nuno Sá <noname.nuno@gmail.com> wrote:  
> > > > 
> > > > On Thu, 2023-11-30 at 15:41 -0600, David Lechner wrote:  
> > > > > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:  
> > > > > > 
> > > > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > > > 
> > > > > > The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> > > > > > asserted. Then it was being asserted but never de-asserted which means
> > > > > > the devices was left in reset. Fix it by de-asserting the gpio.  
> > > > > 
> > > > > It could be helpful to update the devicetree bindings to state the
> > > > > expected active-high or active-low setting for this gpio so it is
> > > > > clear which state means asserted.
> > > > >   
> > > > 
> > > > You could state that the chip is active low but I don't see that change that
> > > > important for now. Not sure if this is clear and maybe that's why your
> > > > comment.
> > > > GPIOD_OUT_HIGH has nothing to do with active high or low. It just means, "get
> > > > me
> > > > the
> > > > pin in the asserted state".
> > > >   
> > > 
> > > I would assume that this bug happened in the first place because
> > > someone forgot GPIOD_OUT_LOW in the devicetree when they were
> > > developing the driver. So this is why I suggested that updating the
> > > devicetree binding docs so that future users are less likely to make
> > > the same mistake. Currently, the bindings don't even have reset-gpios
> > > in the examples.  
> > 
> > Hmm, I think you're missing the point... The bug has nothing to do with
> > devicetree.
> > This is what was happening:
> > 
> > 1) We were calling devm_gpiod_get_optional() with GPIOD_OUT_LOW. What this means
> > is
> > that you get an output gpio deasserted. Hence the device is out of reset. And
> > here is
> > the important part... what you have in dts does not matter. If you have active
> > low,
> > it means the pin level will be 1. If you have high, the pin level is 0. And this
> > is
> > all handled by gpiolib for you. 
> > 
> > 2) Then, we called gpiod_direction_output(..., 1), which means set the direction
> > out
> > (which is actually not needed since it was already done when getting the pin) and
> > assert the pin. Hence, reset the device. And we were never de-asserting the pin
> > so
> > the device would be left in reset.
> 
> Functionally I believe David is correct.   Flipping the DT would 'fix' this.
> It's all down to a nreset vs reset pin description.
> 

Ahh I see. Well would not really be a fix :)

- Nuno Sá

[PATCH 05/12] iio: adc: ad9467: don't ignore error codes

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

Make sure functions that return errors are not ignored.

Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/ad9467.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 368ea57be117..04474dbfa631 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
@@ -160,11 +161,12 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
 	struct spi_device *spi = st->spi;
 	int ret;
 
-	if (readval == NULL) {
+	if (!readval) {
 		ret = ad9467_spi_write(spi, reg, writeval);
-		ad9467_spi_write(spi, AN877_ADC_REG_TRANSFER,
-				 AN877_ADC_TRANSFER_SYNC);
-		return ret;
+		if (ret)
+			return ret;
+		return ad9467_spi_write(spi, AN877_ADC_REG_TRANSFER,
+					AN877_ADC_TRANSFER_SYNC);
 	}
 
 	ret = ad9467_spi_read(spi, reg);
@@ -274,6 +276,8 @@ static int ad9467_get_scale(struct adi_axi_adc_conv *conv, int *val, int *val2)
 	unsigned int i, vref_val;
 
 	vref_val = ad9467_spi_read(st->spi, AN877_ADC_REG_VREF);
+	if (vref_val < 0)
+		return vref_val;
 
 	vref_val &= info1->vref_mask;
 
@@ -296,6 +300,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
 	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
 	unsigned int scale_val[2];
 	unsigned int i;
+	int ret;
 
 	if (val != 0)
 		return -EINVAL;
@@ -305,11 +310,13 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
 		if (scale_val[0] != val || scale_val[1] != val2)
 			continue;
 
-		ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
-				 info->scale_table[i][1]);
-		ad9467_spi_write(st->spi, AN877_ADC_REG_TRANSFER,
-				 AN877_ADC_TRANSFER_SYNC);
-		return 0;
+		ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
+				       info->scale_table[i][1]);
+		if (ret < 0)
+			return ret;
+
+		return ad9467_spi_write(st->spi, AN877_ADC_REG_TRANSFER,
+					AN877_ADC_TRANSFER_SYNC);
 	}
 
 	return -EINVAL;

-- 
2.42.1

Re: [PATCH 05/12] iio: adc: ad9467: don't ignore error codes

[David Lechner]

On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
<devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> From: Nuno Sa <nuno.sa@analog.com>
>
> Make sure functions that return errors are not ignored.
>
> Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/adc/ad9467.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index 368ea57be117..04474dbfa631 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c
> @@ -6,6 +6,7 @@
>   */
>
>  #include <linux/module.h>
> +#include <linux/mutex.h>

This looks like an unrelated change (should probably be in a separate commit).

>  #include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/slab.h>

Re: [PATCH 05/12] iio: adc: ad9467: don't ignore error codes

[Nuno Sá]

On Thu, 2023-11-30 at 15:44 -0600, David Lechner wrote:
> On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > Make sure functions that return errors are not ignored.
> > 
> > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/adc/ad9467.c | 25 ++++++++++++++++---------
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > index 368ea57be117..04474dbfa631 100644
> > --- a/drivers/iio/adc/ad9467.c
> > +++ b/drivers/iio/adc/ad9467.c
> > @@ -6,6 +6,7 @@
> >   */
> > 
> >  #include <linux/module.h>
> > +#include <linux/mutex.h>
> 
> This looks like an unrelated change (should probably be in a separate commit).

Indeed it is... 

> 
> >  #include <linux/device.h>
> >  #include <linux/kernel.h>
> >  #include <linux/slab.h>

Re: [PATCH 05/12] iio: adc: ad9467: don't ignore error codes

[Jonathan Cameron]

On Tue, 21 Nov 2023 11:20:18 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:

> From: Nuno Sa <nuno.sa@analog.com>
> 
> Make sure functions that return errors are not ignored.
> 
> Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/adc/ad9467.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index 368ea57be117..04474dbfa631 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/module.h>
> +#include <linux/mutex.h>
David noted this one...

>  #include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/slab.h>
> @@ -160,11 +161,12 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
>  	struct spi_device *spi = st->spi;
>  	int ret;
>  
> -	if (readval == NULL) {
> +	if (!readval) {

Nothing wrong with tidying this up if the !readval syntax is more common
in the driver, but it doesn't have anything to do with the fix, so not in this
patch.

>  		ret = ad9467_spi_write(spi, reg, writeval);
> -		ad9467_spi_write(spi, AN877_ADC_REG_TRANSFER,
> -				 AN877_ADC_TRANSFER_SYNC);
> -		return ret;
> +		if (ret)
> +			return ret;
> +		return ad9467_spi_write(spi, AN877_ADC_REG_TRANSFER,
> +					AN877_ADC_TRANSFER_SYNC);
>  	}
>  
>  	ret = ad9467_spi_read(spi, reg);
> @@ -274,6 +276,8 @@ static int ad9467_get_scale(struct adi_axi_adc_conv *conv, int *val, int *val2)
>  	unsigned int i, vref_val;
unsigned and you check it for < 0 ..

>  
>  	vref_val = ad9467_spi_read(st->spi, AN877_ADC_REG_VREF);
> +	if (vref_val < 0)
> +		return vref_val;

int ret = ...

	vref_val = ret & info1->vref_mask; 
if not an error.


>  
>  	vref_val &= info1->vref_mask;
>  
> @@ -296,6 +300,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
>  	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
>  	unsigned int scale_val[2];
>  	unsigned int i;
> +	int ret;
>  
>  	if (val != 0)
>  		return -EINVAL;
> @@ -305,11 +310,13 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
>  		if (scale_val[0] != val || scale_val[1] != val2)
>  			continue;
>  
> -		ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
> -				 info->scale_table[i][1]);
> -		ad9467_spi_write(st->spi, AN877_ADC_REG_TRANSFER,
> -				 AN877_ADC_TRANSFER_SYNC);
> -		return 0;
> +		ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
> +				       info->scale_table[i][1]);
> +		if (ret < 0)
> +			return ret;
> +
> +		return ad9467_spi_write(st->spi, AN877_ADC_REG_TRANSFER,
> +					AN877_ADC_TRANSFER_SYNC);
>  	}
>  
>  	return -EINVAL;
> 

[PATCH 06/12] iio: adc: ad9467: add mutex to struct ad9467_state

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
are done which means we need to properly protect the whole operation so
we are sure we will be in a sane state if two concurrent calls occur.

Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/ad9467.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 04474dbfa631..91821dee03b7 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -4,7 +4,7 @@
  *
  * Copyright 2012-2020 Analog Devices Inc.
  */
-
+#include <linux/cleanup.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/device.h>
@@ -122,6 +122,8 @@ struct ad9467_state {
 	unsigned int			output_mode;
 
 	struct gpio_desc		*pwrdown_gpio;
+	/* protect against concurrent accesses to the device */
+	struct mutex			lock;
 };
 
 static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
@@ -162,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
 	int ret;
 
 	if (!readval) {
+		guard(mutex)(&st->lock);
 		ret = ad9467_spi_write(spi, reg, writeval);
 		if (ret)
 			return ret;
@@ -310,6 +313,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
 		if (scale_val[0] != val || scale_val[1] != val2)
 			continue;
 
+		guard(mutex)(&st->lock);
 		ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
 				       info->scale_table[i][1]);
 		if (ret < 0)

-- 
2.42.1

Re: [PATCH 06/12] iio: adc: ad9467: add mutex to struct ad9467_state

[David Lechner]

On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
<devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> From: Nuno Sa <nuno.sa@analog.com>
>
> When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
> are done which means we need to properly protect the whole operation so
> we are sure we will be in a sane state if two concurrent calls occur.
>
> Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/adc/ad9467.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index 04474dbfa631..91821dee03b7 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c
> @@ -4,7 +4,7 @@
>   *
>   * Copyright 2012-2020 Analog Devices Inc.
>   */
> -
> +#include <linux/cleanup.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>

Ah, the case of the misplaced header from the previous patch is solved. :-)

>  #include <linux/device.h>
> @@ -122,6 +122,8 @@ struct ad9467_state {
>         unsigned int                    output_mode;
>
>         struct gpio_desc                *pwrdown_gpio;
> +       /* protect against concurrent accesses to the device */
> +       struct mutex                    lock;
>  };
>
>  static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> @@ -162,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
>         int ret;
>
>         if (!readval) {
> +               guard(mutex)(&st->lock);
>                 ret = ad9467_spi_write(spi, reg, writeval);
>                 if (ret)
>                         return ret;
> @@ -310,6 +313,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
>                 if (scale_val[0] != val || scale_val[1] != val2)
>                         continue;
>
> +               guard(mutex)(&st->lock);
>                 ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
>                                        info->scale_table[i][1]);
>                 if (ret < 0)
>
> --
> 2.42.1
>
>

Alternately, this could probably be solved with spi_bus_lock/unlock
and spi_sync_locked rather than introducing a new mutex.

Re: [PATCH 06/12] iio: adc: ad9467: add mutex to struct ad9467_state

[Nuno Sá]

On Thu, 2023-11-30 at 15:50 -0600, David Lechner wrote:
> On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
> > are done which means we need to properly protect the whole operation so
> > we are sure we will be in a sane state if two concurrent calls occur.
> > 
> > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/adc/ad9467.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > index 04474dbfa631..91821dee03b7 100644
> > --- a/drivers/iio/adc/ad9467.c
> > +++ b/drivers/iio/adc/ad9467.c
> > @@ -4,7 +4,7 @@
> >   *
> >   * Copyright 2012-2020 Analog Devices Inc.
> >   */
> > -
> > +#include <linux/cleanup.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> 
> Ah, the case of the misplaced header from the previous patch is solved. :-)
> 

Yeps, it needs to be in this patch :)

> >  #include <linux/device.h>
> > @@ -122,6 +122,8 @@ struct ad9467_state {
> >         unsigned int                    output_mode;
> > 
> >         struct gpio_desc                *pwrdown_gpio;
> > +       /* protect against concurrent accesses to the device */
> > +       struct mutex                    lock;
> >  };
> > 
> >  static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> > @@ -162,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv,
> > unsigned int reg,
> >         int ret;
> > 
> >         if (!readval) {
> > +               guard(mutex)(&st->lock);
> >                 ret = ad9467_spi_write(spi, reg, writeval);
> >                 if (ret)
> >                         return ret;
> > @@ -310,6 +313,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv,
> > int val, int val2)
> >                 if (scale_val[0] != val || scale_val[1] != val2)
> >                         continue;
> > 
> > +               guard(mutex)(&st->lock);
> >                 ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
> >                                        info->scale_table[i][1]);
> >                 if (ret < 0)
> > 
> > --
> > 2.42.1
> > 
> > 
> 
> Alternately, this could probably be solved with spi_bus_lock/unlock
> and spi_sync_locked rather than introducing a new mutex.

Hmm, typically you just have your own lock. No reason to lock the spi bus. And I also
have some plans to eventually change this to regmap :)

- Nuno Sá

Re: [PATCH 06/12] iio: adc: ad9467: add mutex to struct ad9467_state

[Jonathan Cameron]

On Fri, 01 Dec 2023 09:49:38 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Thu, 2023-11-30 at 15:50 -0600, David Lechner wrote:
> > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > <devnull+nuno.sa.analog.com@kernel.org> wrote:  
> > > 
> > > From: Nuno Sa <nuno.sa@analog.com>
> > > 
> > > When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
> > > are done which means we need to properly protect the whole operation so
> > > we are sure we will be in a sane state if two concurrent calls occur.
> > > 
> > > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > ---
> > >  drivers/iio/adc/ad9467.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > > index 04474dbfa631..91821dee03b7 100644
> > > --- a/drivers/iio/adc/ad9467.c
> > > +++ b/drivers/iio/adc/ad9467.c
> > > @@ -4,7 +4,7 @@
> > >   *
> > >   * Copyright 2012-2020 Analog Devices Inc.
> > >   */
> > > -
> > > +#include <linux/cleanup.h>
> > >  #include <linux/module.h>
> > >  #include <linux/mutex.h>  
> > 
> > Ah, the case of the misplaced header from the previous patch is solved. :-)
> >   
> 
> Yeps, it needs to be in this patch :)
> 
> > >  #include <linux/device.h>
> > > @@ -122,6 +122,8 @@ struct ad9467_state {
> > >         unsigned int                    output_mode;
> > > 
> > >         struct gpio_desc                *pwrdown_gpio;
> > > +       /* protect against concurrent accesses to the device */
> > > +       struct mutex                    lock;
> > >  };
> > > 
> > >  static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> > > @@ -162,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv,
> > > unsigned int reg,
> > >         int ret;
> > > 
> > >         if (!readval) {
> > > +               guard(mutex)(&st->lock);
> > >                 ret = ad9467_spi_write(spi, reg, writeval);
> > >                 if (ret)
> > >                         return ret;
> > > @@ -310,6 +313,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv,
> > > int val, int val2)
> > >                 if (scale_val[0] != val || scale_val[1] != val2)
> > >                         continue;
> > > 
> > > +               guard(mutex)(&st->lock);
> > >                 ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
> > >                                        info->scale_table[i][1]);
> > >                 if (ret < 0)
> > > 
> > > --
> > > 2.42.1
> > > 
> > >   
> > 
> > Alternately, this could probably be solved with spi_bus_lock/unlock
> > and spi_sync_locked rather than introducing a new mutex.  
> 
> Hmm, typically you just have your own lock. No reason to lock the spi bus. And I also
> have some plans to eventually change this to regmap :)

Bus lock typically implies that we can't let other users grab the bus in between
for reasons like the chip select needing to be held. I'm not keen on it being
used if the locking is just needed for a specific driver to deal with its
associated device and driver state.

Jonathan

> 
> - Nuno Sá
> 

Re: [PATCH 06/12] iio: adc: ad9467: add mutex to struct ad9467_state

[Jonathan Cameron]

On Tue, 21 Nov 2023 11:20:19 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:

> From: Nuno Sa <nuno.sa@analog.com>
> 
> When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
> are done which means we need to properly protect the whole operation so
> we are sure we will be in a sane state if two concurrent calls occur.
> 
> Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/adc/ad9467.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index 04474dbfa631..91821dee03b7 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c
> @@ -4,7 +4,7 @@
>   *
>   * Copyright 2012-2020 Analog Devices Inc.
>   */
> -
> +#include <linux/cleanup.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/device.h>
> @@ -122,6 +122,8 @@ struct ad9467_state {
>  	unsigned int			output_mode;
>  
>  	struct gpio_desc		*pwrdown_gpio;
> +	/* protect against concurrent accesses to the device */
Not very specific.  Concurrent access usually fine at granularity of
individual read/write as the bus locks protect it.  What state
is actually being protected?  A shared buffer or some state that we
need to ensure remains consistent between driver and device?

> +	struct mutex			lock;
>  };
>  
>  static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> @@ -162,6 +164,7 @@ static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
>  	int ret;
>  
>  	if (!readval) {
> +		guard(mutex)(&st->lock);
>  		ret = ad9467_spi_write(spi, reg, writeval);
>  		if (ret)
>  			return ret;
> @@ -310,6 +313,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
>  		if (scale_val[0] != val || scale_val[1] != val2)
>  			continue;
>  
> +		guard(mutex)(&st->lock);
>  		ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
>  				       info->scale_table[i][1]);
>  		if (ret < 0)
> 

Re: [PATCH 06/12] iio: adc: ad9467: add mutex to struct ad9467_state

[Nuno Sá]

On Mon, 2023-12-04 at 15:23 +0000, Jonathan Cameron wrote:
> On Tue, 21 Nov 2023 11:20:19 +0100
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
> > are done which means we need to properly protect the whole operation so
> > we are sure we will be in a sane state if two concurrent calls occur.
> > 
> > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/adc/ad9467.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > index 04474dbfa631..91821dee03b7 100644
> > --- a/drivers/iio/adc/ad9467.c
> > +++ b/drivers/iio/adc/ad9467.c
> > @@ -4,7 +4,7 @@
> >   *
> >   * Copyright 2012-2020 Analog Devices Inc.
> >   */
> > -
> > +#include <linux/cleanup.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> >  #include <linux/device.h>
> > @@ -122,6 +122,8 @@ struct ad9467_state {
> >         unsigned int                    output_mode;
> >  
> >         struct gpio_desc                *pwrdown_gpio;
> > +       /* protect against concurrent accesses to the device */
> Not very specific.  Concurrent access usually fine at granularity of
> individual read/write as the bus locks protect it.  What state
> is actually being protected?  A shared buffer or some state that we
> need to ensure remains consistent between driver and device?

At this point not any buffer/data... Just making sure things remain consistent
(typical case when you have multiple reads/writes to the device). That's why a tried
to emphasize "accesses to the device". Maybe I should make it explicit I'm speaking
about multiple reads/writes.

- Nuno Sá
> 

Re: [PATCH 06/12] iio: adc: ad9467: add mutex to struct ad9467_state

[Jonathan Cameron]

On Mon, 04 Dec 2023 17:10:01 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2023-12-04 at 15:23 +0000, Jonathan Cameron wrote:
> > On Tue, 21 Nov 2023 11:20:19 +0100
> > Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> >   
> > > From: Nuno Sa <nuno.sa@analog.com>
> > > 
> > > When calling ad9467_set_scale(), multiple calls to ad9467_spi_write()
> > > are done which means we need to properly protect the whole operation so
> > > we are sure we will be in a sane state if two concurrent calls occur.
> > > 
> > > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > ---
> > >  drivers/iio/adc/ad9467.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > > index 04474dbfa631..91821dee03b7 100644
> > > --- a/drivers/iio/adc/ad9467.c
> > > +++ b/drivers/iio/adc/ad9467.c
> > > @@ -4,7 +4,7 @@
> > >   *
> > >   * Copyright 2012-2020 Analog Devices Inc.
> > >   */
> > > -
> > > +#include <linux/cleanup.h>
> > >  #include <linux/module.h>
> > >  #include <linux/mutex.h>
> > >  #include <linux/device.h>
> > > @@ -122,6 +122,8 @@ struct ad9467_state {
> > >         unsigned int                    output_mode;
> > >  
> > >         struct gpio_desc                *pwrdown_gpio;
> > > +       /* protect against concurrent accesses to the device */  
> > Not very specific.  Concurrent access usually fine at granularity of
> > individual read/write as the bus locks protect it.  What state
> > is actually being protected?  A shared buffer or some state that we
> > need to ensure remains consistent between driver and device?  
> 
> At this point not any buffer/data... Just making sure things remain consistent
> (typical case when you have multiple reads/writes to the device). That's why a tried
> to emphasize "accesses to the device". Maybe I should make it explicit I'm speaking
> about multiple reads/writes.

Talk about the data or state rather than the access to it.
Something like
'ensure consistent state obtained on multiple related accesses.'
Or if it's RMW then say that.
> 
> - Nuno Sá
> >   
> 

[PATCH 07/12] iio: adc: ad9467: fix scale setting

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

When reading in_voltage_scale we can get something like:

root@analog:/sys/bus/iio/devices/iio:device2# cat in_voltage_scale
0.038146

However, when reading the available options:

root@analog:/sys/bus/iio/devices/iio:device2# cat
in_voltage_scale_available
2000.000000 2100.000006 2200.000007 2300.000008 2400.000009 2500.000010

which does not make sense. Moreover, when trying to set a new scale we
get an error because there's no call to __ad9467_get_scale() to give us
values as given when reading in_voltage_scale. Fix it by computing the
available scales during probe and properly pass the list when
.read_available() is called.

While at it, change to use .read_available() from iio_info. Also note
that to properly fix this, adi-axi-adc.c has to be changed accordingly.

Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/ad9467.c            | 47 +++++++++++++++++++++++
 drivers/iio/adc/adi-axi-adc.c       | 74 ++++++++-----------------------------
 include/linux/iio/adc/adi-axi-adc.h |  4 ++
 3 files changed, 66 insertions(+), 59 deletions(-)

diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 91821dee03b7..df4c21248226 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -120,6 +120,7 @@ struct ad9467_state {
 	struct spi_device		*spi;
 	struct clk			*clk;
 	unsigned int			output_mode;
+	unsigned int                    (*scales)[2];
 
 	struct gpio_desc		*pwrdown_gpio;
 	/* protect against concurrent accesses to the device */
@@ -216,6 +217,7 @@ static void __ad9467_get_scale(struct adi_axi_adc_conv *conv, int index,
 	.channel = _chan,						\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
 		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE), \
 	.scan_index = _si,						\
 	.scan_type = {							\
 		.sign = _sign,						\
@@ -369,6 +371,26 @@ static int ad9467_write_raw(struct adi_axi_adc_conv *conv,
 	}
 }
 
+static int ad9467_read_avail(struct adi_axi_adc_conv *conv,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long mask)
+{
+	const struct adi_axi_adc_chip_info *info = conv->chip_info;
+	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (const int *)st->scales;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		/* Values are stored in a 2D matrix */
+		*length = info->num_scales * 2;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int ad9467_outputmode_set(struct spi_device *spi, unsigned int mode)
 {
 	int ret;
@@ -381,6 +403,26 @@ static int ad9467_outputmode_set(struct spi_device *spi, unsigned int mode)
 				AN877_ADC_TRANSFER_SYNC);
 }
 
+static int ad9467_scale_fill(struct adi_axi_adc_conv *conv)
+{
+	const struct adi_axi_adc_chip_info *info = conv->chip_info;
+	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	unsigned int i, val1, val2;
+
+	st->scales = devm_kcalloc(&st->spi->dev, ARRAY_SIZE(*st->scales),
+				  info->num_scales, GFP_KERNEL);
+	if (!st->scales)
+		return -ENOMEM;
+
+	for (i = 0; i < info->num_scales * 2; i++) {
+		__ad9467_get_scale(conv, i, &val1, &val2);
+		st->scales[i][0] = val1;
+		st->scales[i][1] = val2;
+	}
+
+	return 0;
+}
+
 static int ad9467_preenable_setup(struct adi_axi_adc_conv *conv)
 {
 	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
@@ -441,6 +483,10 @@ static int ad9467_probe(struct spi_device *spi)
 
 	conv->chip_info = &info->axi_adc_info;
 
+	ret = ad9467_scale_fill(conv);
+	if (ret)
+		return ret;
+
 	id = ad9467_spi_read(spi, AN877_ADC_REG_CHIP_ID);
 	if (id != conv->chip_info->id) {
 		dev_err(&spi->dev, "Mismatch CHIP_ID, got 0x%X, expected 0x%X\n",
@@ -451,6 +497,7 @@ static int ad9467_probe(struct spi_device *spi)
 	conv->reg_access = ad9467_reg_access;
 	conv->write_raw = ad9467_write_raw;
 	conv->read_raw = ad9467_read_raw;
+	conv->read_avail = ad9467_read_avail;
 	conv->preenable_setup = ad9467_preenable_setup;
 
 	st->output_mode = info->default_output_mode |
diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index aff0532a974a..ae83ada7f9f2 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -144,6 +144,20 @@ static int adi_axi_adc_write_raw(struct iio_dev *indio_dev,
 	return conv->write_raw(conv, chan, val, val2, mask);
 }
 
+static int adi_axi_adc_read_avail(struct iio_dev *indio_dev,
+				  struct iio_chan_spec const *chan,
+				  const int **vals, int *type, int *length,
+				  long mask)
+{
+	struct adi_axi_adc_state *st = iio_priv(indio_dev);
+	struct adi_axi_adc_conv *conv = &st->client->conv;
+
+	if (!conv->read_avail)
+		return -EOPNOTSUPP;
+
+	return conv->read_avail(conv, chan, vals, type, length, mask);
+}
+
 static int adi_axi_adc_update_scan_mode(struct iio_dev *indio_dev,
 					const unsigned long *scan_mask)
 {
@@ -228,69 +242,11 @@ struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
 }
 EXPORT_SYMBOL_NS_GPL(devm_adi_axi_adc_conv_register, IIO_ADI_AXI);
 
-static ssize_t in_voltage_scale_available_show(struct device *dev,
-					       struct device_attribute *attr,
-					       char *buf)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct adi_axi_adc_state *st = iio_priv(indio_dev);
-	struct adi_axi_adc_conv *conv = &st->client->conv;
-	size_t len = 0;
-	int i;
-
-	for (i = 0; i < conv->chip_info->num_scales; i++) {
-		const unsigned int *s = conv->chip_info->scale_table[i];
-
-		len += scnprintf(buf + len, PAGE_SIZE - len,
-				 "%u.%06u ", s[0], s[1]);
-	}
-	buf[len - 1] = '\n';
-
-	return len;
-}
-
-static IIO_DEVICE_ATTR_RO(in_voltage_scale_available, 0);
-
-enum {
-	ADI_AXI_ATTR_SCALE_AVAIL,
-};
-
-#define ADI_AXI_ATTR(_en_, _file_)			\
-	[ADI_AXI_ATTR_##_en_] = &iio_dev_attr_##_file_.dev_attr.attr
-
-static struct attribute *adi_axi_adc_attributes[] = {
-	ADI_AXI_ATTR(SCALE_AVAIL, in_voltage_scale_available),
-	NULL
-};
-
-static umode_t axi_adc_attr_is_visible(struct kobject *kobj,
-				       struct attribute *attr, int n)
-{
-	struct device *dev = kobj_to_dev(kobj);
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct adi_axi_adc_state *st = iio_priv(indio_dev);
-	struct adi_axi_adc_conv *conv = &st->client->conv;
-
-	switch (n) {
-	case ADI_AXI_ATTR_SCALE_AVAIL:
-		if (!conv->chip_info->num_scales)
-			return 0;
-		return attr->mode;
-	default:
-		return attr->mode;
-	}
-}
-
-static const struct attribute_group adi_axi_adc_attribute_group = {
-	.attrs = adi_axi_adc_attributes,
-	.is_visible = axi_adc_attr_is_visible,
-};
-
 static const struct iio_info adi_axi_adc_info = {
 	.read_raw = &adi_axi_adc_read_raw,
 	.write_raw = &adi_axi_adc_write_raw,
-	.attrs = &adi_axi_adc_attribute_group,
 	.update_scan_mode = &adi_axi_adc_update_scan_mode,
+	.read_avail = &adi_axi_adc_read_avail,
 };
 
 static const struct adi_axi_adc_core_info adi_axi_adc_10_0_a_info = {
diff --git a/include/linux/iio/adc/adi-axi-adc.h b/include/linux/iio/adc/adi-axi-adc.h
index 52620e5b8052..b7904992d561 100644
--- a/include/linux/iio/adc/adi-axi-adc.h
+++ b/include/linux/iio/adc/adi-axi-adc.h
@@ -41,6 +41,7 @@ struct adi_axi_adc_chip_info {
  * @reg_access		IIO debugfs_reg_access hook for the client ADC
  * @read_raw		IIO read_raw hook for the client ADC
  * @write_raw		IIO write_raw hook for the client ADC
+ * @read_avail		IIO read_avail hook for the client ADC
  */
 struct adi_axi_adc_conv {
 	const struct adi_axi_adc_chip_info		*chip_info;
@@ -54,6 +55,9 @@ struct adi_axi_adc_conv {
 	int (*write_raw)(struct adi_axi_adc_conv *conv,
 			 struct iio_chan_spec const *chan,
 			 int val, int val2, long mask);
+	int (*read_avail)(struct adi_axi_adc_conv *conv,
+			  struct iio_chan_spec const *chan,
+			  const int **val, int *type, int *length, long mask);
 };
 
 struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,

-- 
2.42.1

[PATCH 08/12] iio: adc: ad9467: use spi_get_device_match_data()

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

Make use of spi_get_device_match_data() to simplify things.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/ad9467.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index df4c21248226..f0342c8942ee 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -455,9 +455,7 @@ static int ad9467_probe(struct spi_device *spi)
 	unsigned int id;
 	int ret;
 
-	info = of_device_get_match_data(&spi->dev);
-	if (!info)
-		info = (void *)spi_get_device_id(spi)->driver_data;
+	info = spi_get_device_match_data(spi);
 	if (!info)
 		return -ENODEV;
 

-- 
2.42.1

[PATCH 09/12] iio: adc: ad9467: use chip_info variables instead of array

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

Instead of having an array and keeping IDs for each entry of the array,
just have a chip_info struct per device.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/ad9467.c | 89 +++++++++++++++++++++++-------------------------
 1 file changed, 43 insertions(+), 46 deletions(-)

diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index f0342c8942ee..5db5690ccee8 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -101,12 +101,6 @@
 #define AD9467_DEF_OUTPUT_MODE		0x08
 #define AD9467_REG_VREF_MASK		0x0F
 
-enum {
-	ID_AD9265,
-	ID_AD9434,
-	ID_AD9467,
-};
-
 struct ad9467_chip_info {
 	struct adi_axi_adc_chip_info	axi_adc_info;
 	unsigned int			default_output_mode;
@@ -234,43 +228,46 @@ static const struct iio_chan_spec ad9467_channels[] = {
 	AD9467_CHAN(0, 0, 16, 'S'),
 };
 
-static const struct ad9467_chip_info ad9467_chip_tbl[] = {
-	[ID_AD9265] = {
-		.axi_adc_info = {
-			.id = CHIPID_AD9265,
-			.max_rate = 125000000UL,
-			.scale_table = ad9265_scale_table,
-			.num_scales = ARRAY_SIZE(ad9265_scale_table),
-			.channels = ad9467_channels,
-			.num_channels = ARRAY_SIZE(ad9467_channels),
-		},
-		.default_output_mode = AD9265_DEF_OUTPUT_MODE,
-		.vref_mask = AD9265_REG_VREF_MASK,
+static const struct ad9467_chip_info ad9467_chip_tbl = {
+	.axi_adc_info = {
+		.name = "ad9467",
+		.id = CHIPID_AD9467,
+		.max_rate = 250000000UL,
+		.scale_table = ad9467_scale_table,
+		.num_scales = ARRAY_SIZE(ad9467_scale_table),
+		.channels = ad9467_channels,
+		.num_channels = ARRAY_SIZE(ad9467_channels),
 	},
-	[ID_AD9434] = {
-		.axi_adc_info = {
-			.id = CHIPID_AD9434,
-			.max_rate = 500000000UL,
-			.scale_table = ad9434_scale_table,
-			.num_scales = ARRAY_SIZE(ad9434_scale_table),
-			.channels = ad9434_channels,
-			.num_channels = ARRAY_SIZE(ad9434_channels),
-		},
-		.default_output_mode = AD9434_DEF_OUTPUT_MODE,
-		.vref_mask = AD9434_REG_VREF_MASK,
+	.default_output_mode = AD9467_DEF_OUTPUT_MODE,
+	.vref_mask = AD9467_REG_VREF_MASK,
+};
+
+static const struct ad9467_chip_info ad9434_chip_tbl = {
+	.axi_adc_info = {
+		.name = "ad9434",
+		.id = CHIPID_AD9434,
+		.max_rate = 500000000UL,
+		.scale_table = ad9434_scale_table,
+		.num_scales = ARRAY_SIZE(ad9434_scale_table),
+		.channels = ad9434_channels,
+		.num_channels = ARRAY_SIZE(ad9434_channels),
 	},
-	[ID_AD9467] = {
-		.axi_adc_info = {
-			.id = CHIPID_AD9467,
-			.max_rate = 250000000UL,
-			.scale_table = ad9467_scale_table,
-			.num_scales = ARRAY_SIZE(ad9467_scale_table),
-			.channels = ad9467_channels,
-			.num_channels = ARRAY_SIZE(ad9467_channels),
-		},
-		.default_output_mode = AD9467_DEF_OUTPUT_MODE,
-		.vref_mask = AD9467_REG_VREF_MASK,
+	.default_output_mode = AD9434_DEF_OUTPUT_MODE,
+	.vref_mask = AD9434_REG_VREF_MASK,
+};
+
+static const struct ad9467_chip_info ad9265_chip_tbl = {
+	.axi_adc_info = {
+		.name = "ad9265",
+		.id = CHIPID_AD9265,
+		.max_rate = 125000000UL,
+		.scale_table = ad9265_scale_table,
+		.num_scales = ARRAY_SIZE(ad9265_scale_table),
+		.channels = ad9467_channels,
+		.num_channels = ARRAY_SIZE(ad9467_channels),
 	},
+	.default_output_mode = AD9265_DEF_OUTPUT_MODE,
+	.vref_mask = AD9265_REG_VREF_MASK,
 };
 
 static int ad9467_get_scale(struct adi_axi_adc_conv *conv, int *val, int *val2)
@@ -505,17 +502,17 @@ static int ad9467_probe(struct spi_device *spi)
 }
 
 static const struct of_device_id ad9467_of_match[] = {
-	{ .compatible = "adi,ad9265", .data = &ad9467_chip_tbl[ID_AD9265], },
-	{ .compatible = "adi,ad9434", .data = &ad9467_chip_tbl[ID_AD9434], },
-	{ .compatible = "adi,ad9467", .data = &ad9467_chip_tbl[ID_AD9467], },
+	{ .compatible = "adi,ad9265", .data = &ad9265_chip_tbl, },
+	{ .compatible = "adi,ad9434", .data = &ad9434_chip_tbl, },
+	{ .compatible = "adi,ad9467", .data = &ad9467_chip_tbl, },
 	{}
 };
 MODULE_DEVICE_TABLE(of, ad9467_of_match);
 
 static const struct spi_device_id ad9467_ids[] = {
-	{ "ad9265", (kernel_ulong_t)&ad9467_chip_tbl[ID_AD9265] },
-	{ "ad9434", (kernel_ulong_t)&ad9467_chip_tbl[ID_AD9434] },
-	{ "ad9467", (kernel_ulong_t)&ad9467_chip_tbl[ID_AD9467] },
+	{ "ad9265", (kernel_ulong_t)&ad9265_chip_tbl },
+	{ "ad9434", (kernel_ulong_t)&ad9434_chip_tbl },
+	{ "ad9467", (kernel_ulong_t)&ad9467_chip_tbl },
 	{}
 };
 MODULE_DEVICE_TABLE(spi, ad9467_ids);

-- 
2.42.1

Re: [PATCH 09/12] iio: adc: ad9467: use chip_info variables instead of array

[Jonathan Cameron]

On Tue, 21 Nov 2023 11:20:22 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:

> From: Nuno Sa <nuno.sa@analog.com>
> 
> Instead of having an array and keeping IDs for each entry of the array,
> just have a chip_info struct per device.
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
Hi Nuno,

Probably makes sense to break out the various tidy up an fixes for ad9467 as a
precursor series and just refer to it from the main one (which will probably take
longer to settle).  Hopefully we can get these out the way and remove the distraction
from the stuff you really care about!

Jonathan

Re: [PATCH 09/12] iio: adc: ad9467: use chip_info variables instead of array

[Nuno Sá]

On Mon, 2023-12-04 at 15:25 +0000, Jonathan Cameron wrote:
> On Tue, 21 Nov 2023 11:20:22 +0100
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > Instead of having an array and keeping IDs for each entry of the array,
> > just have a chip_info struct per device.
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> Hi Nuno,
> 
> Probably makes sense to break out the various tidy up an fixes for ad9467 as a
> precursor series and just refer to it from the main one (which will probably take
> longer to settle).  Hopefully we can get these out the way and remove the
> distraction
> from the stuff you really care about!
> 
> Jonathan

Alright... will do that! 

- Nuno Sá

[PATCH 10/12] iio: adc: ad9467: convert to backend framework

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

Convert the driver to use the new IIO backend framework. The device
functionality is expected to be the same (meaning no added or removed
features).

Also note this patch effectively breaks ABI and that's needed so we can
properly support this device and add needed features making use of the
new IIO framework.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/Kconfig  |   2 +-
 drivers/iio/adc/ad9467.c | 256 +++++++++++++++++++++++++++++------------------
 2 files changed, 157 insertions(+), 101 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 1e2b7a2c67c6..af56df63beff 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -275,7 +275,7 @@ config AD799X
 config AD9467
 	tristate "Analog Devices AD9467 High Speed ADC driver"
 	depends on SPI
-	depends on ADI_AXI_ADC
+	select IIO_BACKEND
 	help
 	  Say yes here to build support for Analog Devices:
 	  * AD9467 16-Bit, 200 MSPS/250 MSPS Analog-to-Digital Converter
diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 5db5690ccee8..8b0402e73ace 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -16,13 +16,13 @@
 #include <linux/gpio/consumer.h>
 #include <linux/of.h>
 
-
+#include <linux/iio/buffer-dmaengine.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
 #include <linux/clk.h>
 
-#include <linux/iio/adc/adi-axi-adc.h>
+#include <linux/iio/backend.h>
 
 /*
  * ADI High-Speed ADC common spi interface registers
@@ -102,15 +102,20 @@
 #define AD9467_REG_VREF_MASK		0x0F
 
 struct ad9467_chip_info {
-	struct adi_axi_adc_chip_info	axi_adc_info;
-	unsigned int			default_output_mode;
-	unsigned int			vref_mask;
+	const char		*name;
+	unsigned int		id;
+	const struct		iio_chan_spec *channels;
+	unsigned int		num_channels;
+	const unsigned int	(*scale_table)[2];
+	int			num_scales;
+	unsigned long		max_rate;
+	unsigned int		default_output_mode;
+	unsigned int		vref_mask;
 };
 
-#define to_ad9467_chip_info(_info)	\
-	container_of(_info, struct ad9467_chip_info, axi_adc_info)
-
 struct ad9467_state {
+	const struct ad9467_chip_info	*info;
+	struct iio_backend		*back;
 	struct spi_device		*spi;
 	struct clk			*clk;
 	unsigned int			output_mode;
@@ -151,10 +156,10 @@ static int ad9467_spi_write(struct spi_device *spi, unsigned int reg,
 	return spi_write(spi, buf, ARRAY_SIZE(buf));
 }
 
-static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
+static int ad9467_reg_access(struct iio_dev *indio_dev, unsigned int reg,
 			     unsigned int writeval, unsigned int *readval)
 {
-	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	struct ad9467_state *st = iio_priv(indio_dev);
 	struct spi_device *spi = st->spi;
 	int ret;
 
@@ -191,14 +196,13 @@ static const unsigned int ad9467_scale_table[][2] = {
 	{2300, 8}, {2400, 9}, {2500, 10},
 };
 
-static void __ad9467_get_scale(struct adi_axi_adc_conv *conv, int index,
+static void __ad9467_get_scale(struct ad9467_state *st, int index,
 			       unsigned int *val, unsigned int *val2)
 {
-	const struct adi_axi_adc_chip_info *info = conv->chip_info;
-	const struct iio_chan_spec *chan = &info->channels[0];
+	const struct iio_chan_spec *chan = &st->info->channels[0];
 	unsigned int tmp;
 
-	tmp = (info->scale_table[index][0] * 1000000ULL) >>
+	tmp = (st->info->scale_table[index][0] * 1000000ULL) >>
 			chan->scan_type.realbits;
 	*val = tmp / 1000000;
 	*val2 = tmp % 1000000;
@@ -229,77 +233,66 @@ static const struct iio_chan_spec ad9467_channels[] = {
 };
 
 static const struct ad9467_chip_info ad9467_chip_tbl = {
-	.axi_adc_info = {
-		.name = "ad9467",
-		.id = CHIPID_AD9467,
-		.max_rate = 250000000UL,
-		.scale_table = ad9467_scale_table,
-		.num_scales = ARRAY_SIZE(ad9467_scale_table),
-		.channels = ad9467_channels,
-		.num_channels = ARRAY_SIZE(ad9467_channels),
-	},
+	.name = "ad9467",
+	.id = CHIPID_AD9467,
+	.max_rate = 250000000UL,
+	.scale_table = ad9467_scale_table,
+	.num_scales = ARRAY_SIZE(ad9467_scale_table),
+	.channels = ad9467_channels,
+	.num_channels = ARRAY_SIZE(ad9467_channels),
 	.default_output_mode = AD9467_DEF_OUTPUT_MODE,
 	.vref_mask = AD9467_REG_VREF_MASK,
 };
 
 static const struct ad9467_chip_info ad9434_chip_tbl = {
-	.axi_adc_info = {
-		.name = "ad9434",
-		.id = CHIPID_AD9434,
-		.max_rate = 500000000UL,
-		.scale_table = ad9434_scale_table,
-		.num_scales = ARRAY_SIZE(ad9434_scale_table),
-		.channels = ad9434_channels,
-		.num_channels = ARRAY_SIZE(ad9434_channels),
-	},
+	.name = "ad9434",
+	.id = CHIPID_AD9434,
+	.max_rate = 500000000UL,
+	.scale_table = ad9434_scale_table,
+	.num_scales = ARRAY_SIZE(ad9434_scale_table),
+	.channels = ad9434_channels,
+	.num_channels = ARRAY_SIZE(ad9434_channels),
 	.default_output_mode = AD9434_DEF_OUTPUT_MODE,
 	.vref_mask = AD9434_REG_VREF_MASK,
 };
 
 static const struct ad9467_chip_info ad9265_chip_tbl = {
-	.axi_adc_info = {
-		.name = "ad9265",
-		.id = CHIPID_AD9265,
-		.max_rate = 125000000UL,
-		.scale_table = ad9265_scale_table,
-		.num_scales = ARRAY_SIZE(ad9265_scale_table),
-		.channels = ad9467_channels,
-		.num_channels = ARRAY_SIZE(ad9467_channels),
-	},
+	.name = "ad9265",
+	.id = CHIPID_AD9265,
+	.max_rate = 125000000UL,
+	.scale_table = ad9265_scale_table,
+	.num_scales = ARRAY_SIZE(ad9265_scale_table),
+	.channels = ad9467_channels,
+	.num_channels = ARRAY_SIZE(ad9467_channels),
 	.default_output_mode = AD9265_DEF_OUTPUT_MODE,
 	.vref_mask = AD9265_REG_VREF_MASK,
 };
 
-static int ad9467_get_scale(struct adi_axi_adc_conv *conv, int *val, int *val2)
+static int ad9467_get_scale(struct ad9467_state *st, int *val, int *val2)
 {
-	const struct adi_axi_adc_chip_info *info = conv->chip_info;
-	const struct ad9467_chip_info *info1 = to_ad9467_chip_info(info);
-	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
 	unsigned int i, vref_val;
 
 	vref_val = ad9467_spi_read(st->spi, AN877_ADC_REG_VREF);
 	if (vref_val < 0)
 		return vref_val;
 
-	vref_val &= info1->vref_mask;
+	vref_val &= st->info->vref_mask;
 
-	for (i = 0; i < info->num_scales; i++) {
-		if (vref_val == info->scale_table[i][1])
+	for (i = 0; i < st->info->num_scales; i++) {
+		if (vref_val == st->info->scale_table[i][1])
 			break;
 	}
 
-	if (i == info->num_scales)
+	if (i == st->info->num_scales)
 		return -ERANGE;
 
-	__ad9467_get_scale(conv, i, val, val2);
+	__ad9467_get_scale(st, i, val, val2);
 
 	return IIO_VAL_INT_PLUS_MICRO;
 }
 
-static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
+static int ad9467_set_scale(struct ad9467_state *st, int val, int val2)
 {
-	const struct adi_axi_adc_chip_info *info = conv->chip_info;
-	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
 	unsigned int scale_val[2];
 	unsigned int i;
 	int ret;
@@ -307,14 +300,14 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
 	if (val != 0)
 		return -EINVAL;
 
-	for (i = 0; i < info->num_scales; i++) {
-		__ad9467_get_scale(conv, i, &scale_val[0], &scale_val[1]);
+	for (i = 0; i < st->info->num_scales; i++) {
+		__ad9467_get_scale(st, i, &scale_val[0], &scale_val[1]);
 		if (scale_val[0] != val || scale_val[1] != val2)
 			continue;
 
 		guard(mutex)(&st->lock);
 		ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
-				       info->scale_table[i][1]);
+				       st->info->scale_table[i][1]);
 		if (ret < 0)
 			return ret;
 
@@ -325,15 +318,15 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
 	return -EINVAL;
 }
 
-static int ad9467_read_raw(struct adi_axi_adc_conv *conv,
+static int ad9467_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val, int *val2, long m)
 {
-	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	struct ad9467_state *st = iio_priv(indio_dev);
 
 	switch (m) {
 	case IIO_CHAN_INFO_SCALE:
-		return ad9467_get_scale(conv, val, val2);
+		return ad9467_get_scale(st, val, val2);
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		*val = clk_get_rate(st->clk);
 
@@ -343,20 +336,19 @@ static int ad9467_read_raw(struct adi_axi_adc_conv *conv,
 	}
 }
 
-static int ad9467_write_raw(struct adi_axi_adc_conv *conv,
+static int ad9467_write_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int val, int val2, long mask)
 {
-	const struct adi_axi_adc_chip_info *info = conv->chip_info;
-	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	struct ad9467_state *st = iio_priv(indio_dev);
 	long r_clk;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
-		return ad9467_set_scale(conv, val, val2);
+		return ad9467_set_scale(st, val, val2);
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		r_clk = clk_round_rate(st->clk, val);
-		if (r_clk < 0 || r_clk > info->max_rate) {
+		if (r_clk < 0 || r_clk > st->info->max_rate) {
 			dev_warn(&st->spi->dev,
 				 "Error setting ADC sample rate %ld", r_clk);
 			return -EINVAL;
@@ -368,26 +360,53 @@ static int ad9467_write_raw(struct adi_axi_adc_conv *conv,
 	}
 }
 
-static int ad9467_read_avail(struct adi_axi_adc_conv *conv,
+static int ad9467_read_avail(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     const int **vals, int *type, int *length,
 			     long mask)
 {
-	const struct adi_axi_adc_chip_info *info = conv->chip_info;
-	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	struct ad9467_state *st = iio_priv(indio_dev);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
 		*vals = (const int *)st->scales;
 		*type = IIO_VAL_INT_PLUS_MICRO;
 		/* Values are stored in a 2D matrix */
-		*length = info->num_scales * 2;
+		*length = st->info->num_scales * 2;
 		return IIO_AVAIL_LIST;
 	default:
 		return -EINVAL;
 	}
 }
 
+static int ad9467_update_scan_mode(struct iio_dev *indio_dev,
+				   const unsigned long *scan_mask)
+{
+	struct ad9467_state *st = iio_priv(indio_dev);
+	unsigned int c;
+	int ret;
+
+	for (c = 0; c < st->info->num_channels; c++) {
+		if (test_bit(c, scan_mask))
+			ret = iio_backend_chan_enable(st->back, c);
+		else
+			ret = iio_backend_chan_disable(st->back, c);
+
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct iio_info ad9467_info = {
+	.read_raw = ad9467_read_raw,
+	.write_raw = ad9467_write_raw,
+	.update_scan_mode = ad9467_update_scan_mode,
+	.debugfs_reg_access = ad9467_reg_access,
+	.read_avail = ad9467_read_avail,
+};
+
 static int ad9467_outputmode_set(struct spi_device *spi, unsigned int mode)
 {
 	int ret;
@@ -400,19 +419,17 @@ static int ad9467_outputmode_set(struct spi_device *spi, unsigned int mode)
 				AN877_ADC_TRANSFER_SYNC);
 }
 
-static int ad9467_scale_fill(struct adi_axi_adc_conv *conv)
+static int ad9467_scale_fill(struct ad9467_state *st)
 {
-	const struct adi_axi_adc_chip_info *info = conv->chip_info;
-	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
 	unsigned int i, val1, val2;
 
 	st->scales = devm_kcalloc(&st->spi->dev, ARRAY_SIZE(*st->scales),
-				  info->num_scales, GFP_KERNEL);
+				  st->info->num_scales, GFP_KERNEL);
 	if (!st->scales)
 		return -ENOMEM;
 
-	for (i = 0; i < info->num_scales * 2; i++) {
-		__ad9467_get_scale(conv, i, &val1, &val2);
+	for (i = 0; i < st->info->num_scales * 2; i++) {
+		__ad9467_get_scale(st, i, &val1, &val2);
 		st->scales[i][0] = val1;
 		st->scales[i][1] = val2;
 	}
@@ -420,11 +437,27 @@ static int ad9467_scale_fill(struct adi_axi_adc_conv *conv)
 	return 0;
 }
 
-static int ad9467_preenable_setup(struct adi_axi_adc_conv *conv)
+static int ad9467_setup(struct ad9467_state *st)
 {
-	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	struct iio_backend_data_fmt data = {
+		.sign_extend = true,
+		.enable = true,
+	};
+	unsigned int c, mode;
+	int ret;
 
-	return ad9467_outputmode_set(st->spi, st->output_mode);
+	mode = st->info->default_output_mode | AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT;
+	ret = ad9467_outputmode_set(st->spi, mode);
+	if (ret)
+		return ret;
+
+	for (c = 0; c < st->info->num_channels; c++) {
+		ret = iio_backend_data_format_set(st->back, c, &data);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 static int ad9467_reset(struct device *dev)
@@ -444,25 +477,38 @@ static int ad9467_reset(struct device *dev)
 	return 0;
 }
 
+static int ad9467_buffer_get(struct iio_dev *indio_dev)
+{
+	struct device *dev = indio_dev->dev.parent;
+	const char *dma_name;
+
+	if (!device_property_present(dev, "dmas"))
+		return 0;
+
+	if (device_property_read_string(dev, "dma-names", &dma_name))
+		dma_name = "rx";
+
+	return devm_iio_dmaengine_buffer_setup(dev, indio_dev, dma_name);
+}
+
 static int ad9467_probe(struct spi_device *spi)
 {
-	const struct ad9467_chip_info *info;
-	struct adi_axi_adc_conv *conv;
+	struct iio_dev *indio_dev;
 	struct ad9467_state *st;
 	unsigned int id;
 	int ret;
 
-	info = spi_get_device_match_data(spi);
-	if (!info)
-		return -ENODEV;
-
-	conv = devm_adi_axi_adc_conv_register(&spi->dev, sizeof(*st));
-	if (IS_ERR(conv))
-		return PTR_ERR(conv);
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
 
-	st = adi_axi_adc_conv_priv(conv);
+	st = iio_priv(indio_dev);
 	st->spi = spi;
 
+	st->info = spi_get_device_match_data(spi);
+	if (!st->info)
+		return -ENODEV;
+
 	st->clk = devm_clk_get_enabled(&spi->dev, "adc-clk");
 	if (IS_ERR(st->clk))
 		return PTR_ERR(st->clk);
@@ -476,29 +522,39 @@ static int ad9467_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	conv->chip_info = &info->axi_adc_info;
-
-	ret = ad9467_scale_fill(conv);
+	ret = ad9467_scale_fill(st);
 	if (ret)
 		return ret;
 
 	id = ad9467_spi_read(spi, AN877_ADC_REG_CHIP_ID);
-	if (id != conv->chip_info->id) {
+	if (id != st->info->id) {
 		dev_err(&spi->dev, "Mismatch CHIP_ID, got 0x%X, expected 0x%X\n",
-			id, conv->chip_info->id);
+			id, st->info->id);
 		return -ENODEV;
 	}
 
-	conv->reg_access = ad9467_reg_access;
-	conv->write_raw = ad9467_write_raw;
-	conv->read_raw = ad9467_read_raw;
-	conv->read_avail = ad9467_read_avail;
-	conv->preenable_setup = ad9467_preenable_setup;
+	indio_dev->name = st->info->name;
+	indio_dev->channels = st->info->channels;
+	indio_dev->num_channels = st->info->num_channels;
+	indio_dev->info = &ad9467_info;
+
+	ret = ad9467_buffer_get(indio_dev);
+	if (ret)
+		return ret;
 
-	st->output_mode = info->default_output_mode |
-			  AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT;
+	st->back = devm_iio_backend_get(&spi->dev, NULL);
+	if (IS_ERR(st->back))
+		return PTR_ERR(st->back);
 
-	return 0;
+	ret = iio_backend_enable(st->back);
+	if (ret)
+		return ret;
+
+	ret = ad9467_setup(st);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
 }
 
 static const struct of_device_id ad9467_of_match[] = {

-- 
2.42.1

Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework

[kernel test robot]

Hi Nuno,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus robh/for-next linus/master v6.7-rc2 next-20231121]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nuno-Sa-via-B4-Relay/driver-core-allow-modifying-device_links-flags/20231121-182010
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20231121-dev-iio-backend-v1-10-6a3d542eba35%40analog.com
patch subject: [PATCH 10/12] iio: adc: ad9467: convert to backend framework
config: powerpc-randconfig-r071-20231122 (https://download.01.org/0day-ci/archive/20231122/202311220807.NUS4r7ML-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231122/202311220807.NUS4r7ML-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311220807.NUS4r7ML-lkp@intel.com/

smatch warnings:
drivers/iio/adc/ad9467.c:276 ad9467_get_scale() warn: unsigned 'vref_val' is never less than zero.

vim +/vref_val +276 drivers/iio/adc/ad9467.c

ad67971202381c Michael Hennerich  2020-03-24  270  
a78b758afbce92 Nuno Sa            2023-11-21  271  static int ad9467_get_scale(struct ad9467_state *st, int *val, int *val2)
ad67971202381c Michael Hennerich  2020-03-24  272  {
337dbb6ec1acc2 Alexandru Ardelean 2020-09-24  273  	unsigned int i, vref_val;
ad67971202381c Michael Hennerich  2020-03-24  274  
ad67971202381c Michael Hennerich  2020-03-24  275  	vref_val = ad9467_spi_read(st->spi, AN877_ADC_REG_VREF);
d1e957a3e7676f Nuno Sa            2023-11-21 @276  	if (vref_val < 0)
d1e957a3e7676f Nuno Sa            2023-11-21  277  		return vref_val;
ad67971202381c Michael Hennerich  2020-03-24  278  
a78b758afbce92 Nuno Sa            2023-11-21  279  	vref_val &= st->info->vref_mask;
ad67971202381c Michael Hennerich  2020-03-24  280  
a78b758afbce92 Nuno Sa            2023-11-21  281  	for (i = 0; i < st->info->num_scales; i++) {
a78b758afbce92 Nuno Sa            2023-11-21  282  		if (vref_val == st->info->scale_table[i][1])
ad67971202381c Michael Hennerich  2020-03-24  283  			break;
ad67971202381c Michael Hennerich  2020-03-24  284  	}
ad67971202381c Michael Hennerich  2020-03-24  285  
a78b758afbce92 Nuno Sa            2023-11-21  286  	if (i == st->info->num_scales)
ad67971202381c Michael Hennerich  2020-03-24  287  		return -ERANGE;
ad67971202381c Michael Hennerich  2020-03-24  288  
a78b758afbce92 Nuno Sa            2023-11-21  289  	__ad9467_get_scale(st, i, val, val2);
ad67971202381c Michael Hennerich  2020-03-24  290  
ad67971202381c Michael Hennerich  2020-03-24  291  	return IIO_VAL_INT_PLUS_MICRO;
ad67971202381c Michael Hennerich  2020-03-24  292  }
ad67971202381c Michael Hennerich  2020-03-24  293  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework

[David Lechner]

On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
<devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> From: Nuno Sa <nuno.sa@analog.com>
>
> Convert the driver to use the new IIO backend framework. The device
> functionality is expected to be the same (meaning no added or removed
> features).

Missing a devicetree bindings patch before this one?

>
> Also note this patch effectively breaks ABI and that's needed so we can
> properly support this device and add needed features making use of the
> new IIO framework.

Can you be more specific about what is actually breaking?

>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/adc/Kconfig  |   2 +-
>  drivers/iio/adc/ad9467.c | 256 +++++++++++++++++++++++++++++------------------
>  2 files changed, 157 insertions(+), 101 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 1e2b7a2c67c6..af56df63beff 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -275,7 +275,7 @@ config AD799X
>  config AD9467
>         tristate "Analog Devices AD9467 High Speed ADC driver"
>         depends on SPI
> -       depends on ADI_AXI_ADC
> +       select IIO_BACKEND
>         help
>           Say yes here to build support for Analog Devices:
>           * AD9467 16-Bit, 200 MSPS/250 MSPS Analog-to-Digital Converter
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index 5db5690ccee8..8b0402e73ace 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c

<snip>

> +static int ad9467_buffer_get(struct iio_dev *indio_dev)

perhaps a more descriptive name: ad9467_buffer_setup_optional?

> +{
> +       struct device *dev = indio_dev->dev.parent;
> +       const char *dma_name;
> +
> +       if (!device_property_present(dev, "dmas"))
> +               return 0;
> +
> +       if (device_property_read_string(dev, "dma-names", &dma_name))
> +               dma_name = "rx";
> +
> +       return devm_iio_dmaengine_buffer_setup(dev, indio_dev, dma_name);

The device tree bindings for "adi,ad9467" don't include dma properties
(nor should they). Perhaps the DMA lookup should be a callback to the
backend? Or something similar to the SPI Engine offload that we are
working on?

> +}
> +
>  static int ad9467_probe(struct spi_device *spi)
>  {
> -       const struct ad9467_chip_info *info;
> -       struct adi_axi_adc_conv *conv;
> +       struct iio_dev *indio_dev;
>         struct ad9467_state *st;
>         unsigned int id;
>         int ret;
>
> -       info = spi_get_device_match_data(spi);
> -       if (!info)
> -               return -ENODEV;
> -
> -       conv = devm_adi_axi_adc_conv_register(&spi->dev, sizeof(*st));
> -       if (IS_ERR(conv))
> -               return PTR_ERR(conv);
> +       indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +       if (!indio_dev)
> +               return -ENOMEM;
>
> -       st = adi_axi_adc_conv_priv(conv);
> +       st = iio_priv(indio_dev);
>         st->spi = spi;
>
> +       st->info = spi_get_device_match_data(spi);
> +       if (!st->info)
> +               return -ENODEV;
> +
>         st->clk = devm_clk_get_enabled(&spi->dev, "adc-clk");
>         if (IS_ERR(st->clk))
>                 return PTR_ERR(st->clk);
> @@ -476,29 +522,39 @@ static int ad9467_probe(struct spi_device *spi)
>         if (ret)
>                 return ret;
>
> -       conv->chip_info = &info->axi_adc_info;
> -
> -       ret = ad9467_scale_fill(conv);
> +       ret = ad9467_scale_fill(st);
>         if (ret)
>                 return ret;
>
>         id = ad9467_spi_read(spi, AN877_ADC_REG_CHIP_ID);
> -       if (id != conv->chip_info->id) {
> +       if (id != st->info->id) {
>                 dev_err(&spi->dev, "Mismatch CHIP_ID, got 0x%X, expected 0x%X\n",
> -                       id, conv->chip_info->id);
> +                       id, st->info->id);
>                 return -ENODEV;
>         }
>
> -       conv->reg_access = ad9467_reg_access;
> -       conv->write_raw = ad9467_write_raw;
> -       conv->read_raw = ad9467_read_raw;
> -       conv->read_avail = ad9467_read_avail;
> -       conv->preenable_setup = ad9467_preenable_setup;
> +       indio_dev->name = st->info->name;
> +       indio_dev->channels = st->info->channels;
> +       indio_dev->num_channels = st->info->num_channels;
> +       indio_dev->info = &ad9467_info;
> +
> +       ret = ad9467_buffer_get(indio_dev);
> +       if (ret)
> +               return ret;
>
> -       st->output_mode = info->default_output_mode |
> -                         AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT;
> +       st->back = devm_iio_backend_get(&spi->dev, NULL);

Based on the descriptions given of IIO frontend and backend, I was
expecting this driver to be the backend since SPI is only used to
configure the chip while the adi-axi-adc driver is the one determining
the scan data format, providing the DMA (INDIO_BUFFER_HARDWARE), etc.

Also, from a devicetree "describe the hardware" mindset, it doesn't
seem like this chip (AD9467) should dictate a specific backend. I know
it doesn't make sense practlically for this chip to not use DMA given
the high sample rate, but why should the devicetree for this chip
require it when there is nothing intrensic about this chip itself
related to DMA?

> +       if (IS_ERR(st->back))
> +               return PTR_ERR(st->back);
>
> -       return 0;
> +       ret = iio_backend_enable(st->back);
> +       if (ret)
> +               return ret;
> +
> +       ret = ad9467_setup(st);
> +       if (ret)
> +               return ret;
> +
> +       return devm_iio_device_register(&spi->dev, indio_dev);
>  }
>
>  static const struct of_device_id ad9467_of_match[] = {
>
> --
> 2.42.1
>
>

Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework

[David Lechner]

On Thu, Nov 30, 2023 at 5:30 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay

<snip>

> > +       st->back = devm_iio_backend_get(&spi->dev, NULL);
>
> Based on the descriptions given of IIO frontend and backend, I was
> expecting this driver to be the backend since SPI is only used to
> configure the chip while the adi-axi-adc driver is the one determining
> the scan data format, providing the DMA (INDIO_BUFFER_HARDWARE), etc.
>
> Also, from a devicetree "describe the hardware" mindset, it doesn't
> seem like this chip (AD9467) should dictate a specific backend. I know
> it doesn't make sense practlically for this chip to not use DMA given
> the high sample rate, but why should the devicetree for this chip
> require it when there is nothing intrensic about this chip itself
> related to DMA?
>

Afterthought:

Put another way, it seems like it would be much easier to say "I, the
arbitrary frontend that actually handles the data from the LVDS
outputs, need a backend that provides a SPI connection to an AD9467
chip and takes care of turning on power supplies" than it is to say
"I, the AD9467 chip frontend, need an arbitrary backend that handles
reading data from the LVDS outputs in a very specific manner that is
determined by the driver, not the hardware".

Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework

[Nuno Sá]

On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > Convert the driver to use the new IIO backend framework. The device
> > functionality is expected to be the same (meaning no added or removed
> > features).
> 
> Missing a devicetree bindings patch before this one?
> 
> > 
> > Also note this patch effectively breaks ABI and that's needed so we can
> > properly support this device and add needed features making use of the
> > new IIO framework.
> 
> Can you be more specific about what is actually breaking?
> 
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/adc/Kconfig  |   2 +-
> >  drivers/iio/adc/ad9467.c | 256 +++++++++++++++++++++++++++++------------------
> >  2 files changed, 157 insertions(+), 101 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 1e2b7a2c67c6..af56df63beff 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -275,7 +275,7 @@ config AD799X
> >  config AD9467
> >         tristate "Analog Devices AD9467 High Speed ADC driver"
> >         depends on SPI
> > -       depends on ADI_AXI_ADC
> > +       select IIO_BACKEND
> >         help
> >           Say yes here to build support for Analog Devices:
> >           * AD9467 16-Bit, 200 MSPS/250 MSPS Analog-to-Digital Converter
> > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > index 5db5690ccee8..8b0402e73ace 100644
> > --- a/drivers/iio/adc/ad9467.c
> > +++ b/drivers/iio/adc/ad9467.c
> 
> <snip>
> 
> > +static int ad9467_buffer_get(struct iio_dev *indio_dev)
> 
> perhaps a more descriptive name: ad9467_buffer_setup_optional?
> 

Hmm, no strong feeling. So yeah, can do as you suggest. Even though, now that I'm
thinking, I'm not so sure if this is just some legacy thing we had in ADI tree. I
wonder if it actually makes sense for a device like with no buffering support?!
 
> > +{
> > +       struct device *dev = indio_dev->dev.parent;
> > +       const char *dma_name;
> > +
> > +       if (!device_property_present(dev, "dmas"))
> > +               return 0;
> > +
> > +       if (device_property_read_string(dev, "dma-names", &dma_name))
> > +               dma_name = "rx";
> > +
> > +       return devm_iio_dmaengine_buffer_setup(dev, indio_dev, dma_name);
> 
> The device tree bindings for "adi,ad9467" don't include dma properties
> (nor should they). Perhaps the DMA lookup should be a callback to the
> backend? Or something similar to the SPI Engine offload that we are
> working on?
> 

Oh yes, I need to update the bindings. In the link I sent you we can see my thoughts
on this. In theory, hardwarewise, it would actually make sense for the DMA to be on
the backend device because that's where the connection is in HW. However, since we
want to have the IIO interface in the frontend, it would be hard to do that without
hacking devm_iio_dmaengine_buffer_setup(). I mean, lifetime wise it would be far from
wise to have the DMA buffer associated to a completely different device than the IIO
parent device. I mean, one way could just be export iio_dmaengine_buffer_free() and
iio_dmaengine_buffer_alloc() so we can actually control the lifetime of the buffer
from the frontend device. If Jonathan is fine with this, I'm on board for it....

- Nuno Sá
> 

Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework

[David Lechner]

On Fri, Dec 1, 2023 at 3:08 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > >
> > > From: Nuno Sa <nuno.sa@analog.com>
> > >
> > > Convert the driver to use the new IIO backend framework. The device
> > > functionality is expected to be the same (meaning no added or removed
> > > features).
> >
> > Missing a devicetree bindings patch before this one?
> >
> > >
> > > Also note this patch effectively breaks ABI and that's needed so we can
> > > properly support this device and add needed features making use of the
> > > new IIO framework.
> >
> > Can you be more specific about what is actually breaking?
> >
> > >
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > ---
> > >  drivers/iio/adc/Kconfig  |   2 +-
> > >  drivers/iio/adc/ad9467.c | 256 +++++++++++++++++++++++++++++------------------
> > >  2 files changed, 157 insertions(+), 101 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > index 1e2b7a2c67c6..af56df63beff 100644
> > > --- a/drivers/iio/adc/Kconfig
> > > +++ b/drivers/iio/adc/Kconfig
> > > @@ -275,7 +275,7 @@ config AD799X
> > >  config AD9467
> > >         tristate "Analog Devices AD9467 High Speed ADC driver"
> > >         depends on SPI
> > > -       depends on ADI_AXI_ADC
> > > +       select IIO_BACKEND
> > >         help
> > >           Say yes here to build support for Analog Devices:
> > >           * AD9467 16-Bit, 200 MSPS/250 MSPS Analog-to-Digital Converter
> > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > > index 5db5690ccee8..8b0402e73ace 100644
> > > --- a/drivers/iio/adc/ad9467.c
> > > +++ b/drivers/iio/adc/ad9467.c
> >
> > <snip>
> >
> > > +static int ad9467_buffer_get(struct iio_dev *indio_dev)
> >
> > perhaps a more descriptive name: ad9467_buffer_setup_optional?
> >
>
> Hmm, no strong feeling. So yeah, can do as you suggest. Even though, now that I'm
> thinking, I'm not so sure if this is just some legacy thing we had in ADI tree. I
> wonder if it actually makes sense for a device like with no buffering support?!
>
> > > +{
> > > +       struct device *dev = indio_dev->dev.parent;
> > > +       const char *dma_name;
> > > +
> > > +       if (!device_property_present(dev, "dmas"))
> > > +               return 0;
> > > +
> > > +       if (device_property_read_string(dev, "dma-names", &dma_name))
> > > +               dma_name = "rx";
> > > +
> > > +       return devm_iio_dmaengine_buffer_setup(dev, indio_dev, dma_name);
> >
> > The device tree bindings for "adi,ad9467" don't include dma properties
> > (nor should they). Perhaps the DMA lookup should be a callback to the
> > backend? Or something similar to the SPI Engine offload that we are
> > working on?
> >
>
> Oh yes, I need to update the bindings. In the link I sent you we can see my thoughts
> on this. In theory, hardwarewise, it would actually make sense for the DMA to be on
> the backend device because that's where the connection is in HW. However, since we
> want to have the IIO interface in the frontend, it would be hard to do that without
> hacking devm_iio_dmaengine_buffer_setup(). I mean, lifetime wise it would be far from
> wise to have the DMA buffer associated to a completely different device than the IIO
> parent device. I mean, one way could just be export iio_dmaengine_buffer_free() and
> iio_dmaengine_buffer_alloc() so we can actually control the lifetime of the buffer
> from the frontend device. If Jonathan is fine with this, I'm on board for it....
>
> - Nuno Sá
> >
>

I was planning on exporting iio_dmaengine_buffer_alloc() [1] for SPI
Engine offload support, so I hope that is the right way to go. ;-)

[1]: https://github.com/analogdevicesinc/linux/pull/2341/commits/71048ff83a63e9d0a5ddb9ffa331871edd6bd2a5

Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework

[Nuno Sá]

On Fri, 2023-12-01 at 11:44 -0600, David Lechner wrote:
> On Fri, Dec 1, 2023 at 3:08 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> > > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > > 
> > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > 
> > > > Convert the driver to use the new IIO backend framework. The device
> > > > functionality is expected to be the same (meaning no added or removed
> > > > features).
> > > 
> > > Missing a devicetree bindings patch before this one?
> > > 
> > > > 
> > > > Also note this patch effectively breaks ABI and that's needed so we can
> > > > properly support this device and add needed features making use of the
> > > > new IIO framework.
> > > 
> > > Can you be more specific about what is actually breaking?
> > > 
> > > > 
> > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > ---
> > > >  drivers/iio/adc/Kconfig  |   2 +-
> > > >  drivers/iio/adc/ad9467.c | 256 +++++++++++++++++++++++++++++----------------
> > > > --
> > > >  2 files changed, 157 insertions(+), 101 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > > index 1e2b7a2c67c6..af56df63beff 100644
> > > > --- a/drivers/iio/adc/Kconfig
> > > > +++ b/drivers/iio/adc/Kconfig
> > > > @@ -275,7 +275,7 @@ config AD799X
> > > >  config AD9467
> > > >         tristate "Analog Devices AD9467 High Speed ADC driver"
> > > >         depends on SPI
> > > > -       depends on ADI_AXI_ADC
> > > > +       select IIO_BACKEND
> > > >         help
> > > >           Say yes here to build support for Analog Devices:
> > > >           * AD9467 16-Bit, 200 MSPS/250 MSPS Analog-to-Digital Converter
> > > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > > > index 5db5690ccee8..8b0402e73ace 100644
> > > > --- a/drivers/iio/adc/ad9467.c
> > > > +++ b/drivers/iio/adc/ad9467.c
> > > 
> > > <snip>
> > > 
> > > > +static int ad9467_buffer_get(struct iio_dev *indio_dev)
> > > 
> > > perhaps a more descriptive name: ad9467_buffer_setup_optional?
> > > 
> > 
> > Hmm, no strong feeling. So yeah, can do as you suggest. Even though, now that I'm
> > thinking, I'm not so sure if this is just some legacy thing we had in ADI tree. I
> > wonder if it actually makes sense for a device like with no buffering support?!
> > 
> > > > +{
> > > > +       struct device *dev = indio_dev->dev.parent;
> > > > +       const char *dma_name;
> > > > +
> > > > +       if (!device_property_present(dev, "dmas"))
> > > > +               return 0;
> > > > +
> > > > +       if (device_property_read_string(dev, "dma-names", &dma_name))
> > > > +               dma_name = "rx";
> > > > +
> > > > +       return devm_iio_dmaengine_buffer_setup(dev, indio_dev, dma_name);
> > > 
> > > The device tree bindings for "adi,ad9467" don't include dma properties
> > > (nor should they). Perhaps the DMA lookup should be a callback to the
> > > backend? Or something similar to the SPI Engine offload that we are
> > > working on?
> > > 
> > 
> > Oh yes, I need to update the bindings. In the link I sent you we can see my
> > thoughts
> > on this. In theory, hardwarewise, it would actually make sense for the DMA to be
> > on
> > the backend device because that's where the connection is in HW. However, since
> > we
> > want to have the IIO interface in the frontend, it would be hard to do that
> > without
> > hacking devm_iio_dmaengine_buffer_setup(). I mean, lifetime wise it would be far
> > from
> > wise to have the DMA buffer associated to a completely different device than the
> > IIO
> > parent device. I mean, one way could just be export iio_dmaengine_buffer_free()
> > and
> > iio_dmaengine_buffer_alloc() so we can actually control the lifetime of the
> > buffer
> > from the frontend device. If Jonathan is fine with this, I'm on board for it....
> > 
> > - Nuno Sá
> > > 
> > 
> 
> I was planning on exporting iio_dmaengine_buffer_alloc() [1] for SPI
> Engine offload support, so I hope that is the right way to go. ;-)
> 
> [1]:
> https://github.com/analogdevicesinc/linux/pull/2341/commits/71048ff83a63e9d0a5ddb9ffa331871edd6bd2a5

I don't really want to extend much on this since this is still out of tree code so
I'm not sure we should be discussing it much in here. But there a couple of concerns
already I'm seeing:

* AFAIU, you export the function so you can use it in your pwm trigger. And you don't
want to attach the buffer to a device. That looks very questionable. If you don't
attach to a device, how do you have the userspace interface working on that buffer?
How can you fetch samples from it? Also hiding the buffer allocation in pure trigger
device is at the very least questionable. But the point is, in the end of the day,
the buffer should belong to a device.

* Your PWM trigger seems to be highly focused on the spi_engine offload feature. You
should go for a generic pwm trigger which is something that was already discussed on
the list and AFAIR, completely acceptable. That said, not so sure how much will we
win (in terms of code simplification) by having devices under the spi_engine
controller using a pwm trigger. But conceptually it is correct and we do have a mode
for hardware triggered buffers.

- Nuno Sá

Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework

[Nuno Sá]

On Sat, 2023-12-02 at 09:46 +0100, Nuno Sá wrote:
> On Fri, 2023-12-01 at 11:44 -0600, David Lechner wrote:
> > On Fri, Dec 1, 2023 at 3:08 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > 
> > > On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> > > > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > > > 
> > > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > > 
> > > > > Convert the driver to use the new IIO backend framework. The device
> > > > > functionality is expected to be the same (meaning no added or removed
> > > > > features).
> > > > 
> > > > Missing a devicetree bindings patch before this one?
> > > > 
> > > > > 
> > > > > Also note this patch effectively breaks ABI and that's needed so we can
> > > > > properly support this device and add needed features making use of the
> > > > > new IIO framework.
> > > > 
> > > > Can you be more specific about what is actually breaking?
> > > > 
> > > > > 
> > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > > ---
> > > > >  drivers/iio/adc/Kconfig  |   2 +-
> > > > >  drivers/iio/adc/ad9467.c | 256 +++++++++++++++++++++++++++++--------------
> > > > > --
> > > > > --
> > > > >  2 files changed, 157 insertions(+), 101 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > > > index 1e2b7a2c67c6..af56df63beff 100644
> > > > > --- a/drivers/iio/adc/Kconfig
> > > > > +++ b/drivers/iio/adc/Kconfig
> > > > > @@ -275,7 +275,7 @@ config AD799X
> > > > >  config AD9467
> > > > >         tristate "Analog Devices AD9467 High Speed ADC driver"
> > > > >         depends on SPI
> > > > > -       depends on ADI_AXI_ADC
> > > > > +       select IIO_BACKEND
> > > > >         help
> > > > >           Say yes here to build support for Analog Devices:
> > > > >           * AD9467 16-Bit, 200 MSPS/250 MSPS Analog-to-Digital Converter
> > > > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > > > > index 5db5690ccee8..8b0402e73ace 100644
> > > > > --- a/drivers/iio/adc/ad9467.c
> > > > > +++ b/drivers/iio/adc/ad9467.c
> > > > 
> > > > <snip>
> > > > 
> > > > > +static int ad9467_buffer_get(struct iio_dev *indio_dev)
> > > > 
> > > > perhaps a more descriptive name: ad9467_buffer_setup_optional?
> > > > 
> > > 
> > > Hmm, no strong feeling. So yeah, can do as you suggest. Even though, now that
> > > I'm
> > > thinking, I'm not so sure if this is just some legacy thing we had in ADI tree.
> > > I
> > > wonder if it actually makes sense for a device like with no buffering support?!
> > > 
> > > > > +{
> > > > > +       struct device *dev = indio_dev->dev.parent;
> > > > > +       const char *dma_name;
> > > > > +
> > > > > +       if (!device_property_present(dev, "dmas"))
> > > > > +               return 0;
> > > > > +
> > > > > +       if (device_property_read_string(dev, "dma-names", &dma_name))
> > > > > +               dma_name = "rx";
> > > > > +
> > > > > +       return devm_iio_dmaengine_buffer_setup(dev, indio_dev, dma_name);
> > > > 
> > > > The device tree bindings for "adi,ad9467" don't include dma properties
> > > > (nor should they). Perhaps the DMA lookup should be a callback to the
> > > > backend? Or something similar to the SPI Engine offload that we are
> > > > working on?
> > > > 
> > > 
> > > Oh yes, I need to update the bindings. In the link I sent you we can see my
> > > thoughts
> > > on this. In theory, hardwarewise, it would actually make sense for the DMA to
> > > be
> > > on
> > > the backend device because that's where the connection is in HW. However, since
> > > we
> > > want to have the IIO interface in the frontend, it would be hard to do that
> > > without
> > > hacking devm_iio_dmaengine_buffer_setup(). I mean, lifetime wise it would be
> > > far
> > > from
> > > wise to have the DMA buffer associated to a completely different device than
> > > the
> > > IIO
> > > parent device. I mean, one way could just be export iio_dmaengine_buffer_free()
> > > and
> > > iio_dmaengine_buffer_alloc() so we can actually control the lifetime of the
> > > buffer
> > > from the frontend device. If Jonathan is fine with this, I'm on board for
> > > it....
> > > 
> > > - Nuno Sá
> > > > 
> > > 
> > 
> > I was planning on exporting iio_dmaengine_buffer_alloc() [1] for SPI
> > Engine offload support, so I hope that is the right way to go. ;-)
> > 
> > [1]:
> > https://github.com/analogdevicesinc/linux/pull/2341/commits/71048ff83a63e9d0a5ddb9ffa331871edd6bd2a5
> 
> I don't really want to extend much on this since this is still out of tree code so
> I'm not sure we should be discussing it much in here. But there a couple of
> concerns
> already I'm seeing:
> 
> * AFAIU, you export the function so you can use it in your pwm trigger. And you
> don't
> want to attach the buffer to a device. That looks very questionable. If you don't
> attach to a device, how do you have the userspace interface working on that buffer?
> How can you fetch samples from it? Also hiding the buffer allocation in pure
> trigger
> device is at the very least questionable. But the point is, in the end of the day,
> the buffer should belong to a device.
> 
> * Your PWM trigger seems to be highly focused on the spi_engine offload feature.
> You

OTOH, it also seems there are some stm focused triggers. So maybe we can also have
something more oriented to spi_engine...

- Nuno Sá

Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework

[Jonathan Cameron]

On Fri, 01 Dec 2023 10:08:27 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > <devnull+nuno.sa.analog.com@kernel.org> wrote:  
> > > 
> > > From: Nuno Sa <nuno.sa@analog.com>
> > > 
> > > Convert the driver to use the new IIO backend framework. The device
> > > functionality is expected to be the same (meaning no added or removed
> > > features).  
> > 
> > Missing a devicetree bindings patch before this one?
> >   
> > > 
> > > Also note this patch effectively breaks ABI and that's needed so we can
> > > properly support this device and add needed features making use of the
> > > new IIO framework.  
> > 
> > Can you be more specific about what is actually breaking?
> >   
> > > 
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > ---
> > >  drivers/iio/adc/Kconfig  |   2 +-
> > >  drivers/iio/adc/ad9467.c | 256 +++++++++++++++++++++++++++++------------------
> > >  2 files changed, 157 insertions(+), 101 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > index 1e2b7a2c67c6..af56df63beff 100644
> > > --- a/drivers/iio/adc/Kconfig
> > > +++ b/drivers/iio/adc/Kconfig
> > > @@ -275,7 +275,7 @@ config AD799X
> > >  config AD9467
> > >         tristate "Analog Devices AD9467 High Speed ADC driver"
> > >         depends on SPI
> > > -       depends on ADI_AXI_ADC
> > > +       select IIO_BACKEND
> > >         help
> > >           Say yes here to build support for Analog Devices:
> > >           * AD9467 16-Bit, 200 MSPS/250 MSPS Analog-to-Digital Converter
> > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > > index 5db5690ccee8..8b0402e73ace 100644
> > > --- a/drivers/iio/adc/ad9467.c
> > > +++ b/drivers/iio/adc/ad9467.c  
> > 
> > <snip>
> >   
> > > +static int ad9467_buffer_get(struct iio_dev *indio_dev)  
> > 
> > perhaps a more descriptive name: ad9467_buffer_setup_optional?
> >   
> 
> Hmm, no strong feeling. So yeah, can do as you suggest. Even though, now that I'm
> thinking, I'm not so sure if this is just some legacy thing we had in ADI tree. I
> wonder if it actually makes sense for a device like with no buffering support?!
>  
> > > +{
> > > +       struct device *dev = indio_dev->dev.parent;
> > > +       const char *dma_name;
> > > +
> > > +       if (!device_property_present(dev, "dmas"))
> > > +               return 0;
> > > +
> > > +       if (device_property_read_string(dev, "dma-names", &dma_name))
> > > +               dma_name = "rx";
> > > +
> > > +       return devm_iio_dmaengine_buffer_setup(dev, indio_dev, dma_name);  
> > 
> > The device tree bindings for "adi,ad9467" don't include dma properties
> > (nor should they). Perhaps the DMA lookup should be a callback to the
> > backend? Or something similar to the SPI Engine offload that we are
> > working on?
> >   
> 
> Oh yes, I need to update the bindings. In the link I sent you we can see my thoughts
> on this. In theory, hardwarewise, it would actually make sense for the DMA to be on
> the backend device because that's where the connection is in HW. However, since we
> want to have the IIO interface in the frontend, it would be hard to do that without
> hacking devm_iio_dmaengine_buffer_setup(). I mean, lifetime wise it would be far from
> wise to have the DMA buffer associated to a completely different device than the IIO
> parent device. I mean, one way could just be export iio_dmaengine_buffer_free() and
> iio_dmaengine_buffer_alloc() so we can actually control the lifetime of the buffer
> from the frontend device. If Jonathan is fine with this, I'm on board for it....

It is going to be fiddly but I'd kind of expect the front end to be using a more
abstracted interface to tell the backend to start grabbing data.  Maybe that ends
up being so slim it's just these interfaces and it's not sensible to wrap it though.

The argument for the IIO device (you control) being associated with the analog/digital front end is that
is what happens with a consumer interface today. We think of the analog components (consuming device)
using the sevice of the ADC it is consuming to measure their state.

You can flip the whole thing and make the data grabbing engine the IIO device but
I guess that's just not how we did it and I'm sure I had many good reasons at the time
(one of them being that it needed to work for other cases such as a touchscreen driver
consuming the IIO device)  I that case it's the touchscreen driver that is responsible
for data scaling etc not the ADC.  

In my head, data acquisition is a service - what is interesting is what is being measured
and so I put the IIO device as near that as possible.

Jonathan

Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework

[Nuno Sá]

On Mon, 2023-12-04 at 15:48 +0000, Jonathan Cameron wrote:
> On Fri, 01 Dec 2023 10:08:27 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> > > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > > <devnull+nuno.sa.analog.com@kernel.org> wrote:  
> > > > 
> > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > 
> > > > Convert the driver to use the new IIO backend framework. The device
> > > > functionality is expected to be the same (meaning no added or removed
> > > > features).  
> > > 
> > > Missing a devicetree bindings patch before this one?
> > >   
> > > > 
> > > > Also note this patch effectively breaks ABI and that's needed so we can
> > > > properly support this device and add needed features making use of the
> > > > new IIO framework.  
> > > 
> > > Can you be more specific about what is actually breaking?
> > >   
> > > > 
> > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > ---
> > > >  drivers/iio/adc/Kconfig  |   2 +-
> > > >  drivers/iio/adc/ad9467.c | 256 +++++++++++++++++++++++++++++----------------
> > > > --
> > > >  2 files changed, 157 insertions(+), 101 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > > index 1e2b7a2c67c6..af56df63beff 100644
> > > > --- a/drivers/iio/adc/Kconfig
> > > > +++ b/drivers/iio/adc/Kconfig
> > > > @@ -275,7 +275,7 @@ config AD799X
> > > >  config AD9467
> > > >         tristate "Analog Devices AD9467 High Speed ADC driver"
> > > >         depends on SPI
> > > > -       depends on ADI_AXI_ADC
> > > > +       select IIO_BACKEND
> > > >         help
> > > >           Say yes here to build support for Analog Devices:
> > > >           * AD9467 16-Bit, 200 MSPS/250 MSPS Analog-to-Digital Converter
> > > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > > > index 5db5690ccee8..8b0402e73ace 100644
> > > > --- a/drivers/iio/adc/ad9467.c
> > > > +++ b/drivers/iio/adc/ad9467.c  
> > > 
> > > <snip>
> > >   
> > > > +static int ad9467_buffer_get(struct iio_dev *indio_dev)  
> > > 
> > > perhaps a more descriptive name: ad9467_buffer_setup_optional?
> > >   
> > 
> > Hmm, no strong feeling. So yeah, can do as you suggest. Even though, now that I'm
> > thinking, I'm not so sure if this is just some legacy thing we had in ADI tree. I
> > wonder if it actually makes sense for a device like with no buffering support?!
> >  
> > > > +{
> > > > +       struct device *dev = indio_dev->dev.parent;
> > > > +       const char *dma_name;
> > > > +
> > > > +       if (!device_property_present(dev, "dmas"))
> > > > +               return 0;
> > > > +
> > > > +       if (device_property_read_string(dev, "dma-names", &dma_name))
> > > > +               dma_name = "rx";
> > > > +
> > > > +       return devm_iio_dmaengine_buffer_setup(dev, indio_dev, dma_name);  
> > > 
> > > The device tree bindings for "adi,ad9467" don't include dma properties
> > > (nor should they). Perhaps the DMA lookup should be a callback to the
> > > backend? Or something similar to the SPI Engine offload that we are
> > > working on?
> > >   
> > 
> > Oh yes, I need to update the bindings. In the link I sent you we can see my
> > thoughts
> > on this. In theory, hardwarewise, it would actually make sense for the DMA to be
> > on
> > the backend device because that's where the connection is in HW. However, since
> > we
> > want to have the IIO interface in the frontend, it would be hard to do that
> > without
> > hacking devm_iio_dmaengine_buffer_setup(). I mean, lifetime wise it would be far
> > from
> > wise to have the DMA buffer associated to a completely different device than the
> > IIO
> > parent device. I mean, one way could just be export iio_dmaengine_buffer_free()
> > and
> > iio_dmaengine_buffer_alloc() so we can actually control the lifetime of the
> > buffer
> > from the frontend device. If Jonathan is fine with this, I'm on board for it....
> 
> It is going to be fiddly but I'd kind of expect the front end to be using a more
> abstracted interface to tell the backend to start grabbing data.  Maybe that ends
> up being so slim it's just these interfaces and it's not sensible to wrap it
> though.
> 

Likely I'm missing your point but the discussion here is where the DMA buffer should
be allocated. In theory, in the backend (at least on ADI usecases - it's the proper
representation of the HW) but as we have the iio device in the frontend, it's more
appropriate to have the buffer there. Or at least to have a way to control the buffer
lifetime from there...

On the our usecases, it's not like we tell the backend to grab data, we just use the
normal .update_scan_mode() to enable/disable the channels in the backend so when we
enable the buffer (and the frontend starts receiving and sending data via the serial
interface) the data paths are enabaled/disabled accordingly. Bah, yeah, in a way is
another wording for "grab" or "grab not" :)

- Nuno Sá

Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework

[Jonathan Cameron]

On Mon, 04 Dec 2023 17:23:12 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2023-12-04 at 15:48 +0000, Jonathan Cameron wrote:
> > On Fri, 01 Dec 2023 10:08:27 +0100
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >   
> > > On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:  
> > > > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:    
> > > > > 
> > > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > > 
> > > > > Convert the driver to use the new IIO backend framework. The device
> > > > > functionality is expected to be the same (meaning no added or removed
> > > > > features).    
> > > > 
> > > > Missing a devicetree bindings patch before this one?
> > > >     
> > > > > 
> > > > > Also note this patch effectively breaks ABI and that's needed so we can
> > > > > properly support this device and add needed features making use of the
> > > > > new IIO framework.    
> > > > 
> > > > Can you be more specific about what is actually breaking?
> > > >     
> > > > > 
> > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > > ---
> > > > >  drivers/iio/adc/Kconfig  |   2 +-
> > > > >  drivers/iio/adc/ad9467.c | 256 +++++++++++++++++++++++++++++----------------
> > > > > --
> > > > >  2 files changed, 157 insertions(+), 101 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > > > index 1e2b7a2c67c6..af56df63beff 100644
> > > > > --- a/drivers/iio/adc/Kconfig
> > > > > +++ b/drivers/iio/adc/Kconfig
> > > > > @@ -275,7 +275,7 @@ config AD799X
> > > > >  config AD9467
> > > > >         tristate "Analog Devices AD9467 High Speed ADC driver"
> > > > >         depends on SPI
> > > > > -       depends on ADI_AXI_ADC
> > > > > +       select IIO_BACKEND
> > > > >         help
> > > > >           Say yes here to build support for Analog Devices:
> > > > >           * AD9467 16-Bit, 200 MSPS/250 MSPS Analog-to-Digital Converter
> > > > > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > > > > index 5db5690ccee8..8b0402e73ace 100644
> > > > > --- a/drivers/iio/adc/ad9467.c
> > > > > +++ b/drivers/iio/adc/ad9467.c    
> > > > 
> > > > <snip>
> > > >     
> > > > > +static int ad9467_buffer_get(struct iio_dev *indio_dev)    
> > > > 
> > > > perhaps a more descriptive name: ad9467_buffer_setup_optional?
> > > >     
> > > 
> > > Hmm, no strong feeling. So yeah, can do as you suggest. Even though, now that I'm
> > > thinking, I'm not so sure if this is just some legacy thing we had in ADI tree. I
> > > wonder if it actually makes sense for a device like with no buffering support?!
> > >    
> > > > > +{
> > > > > +       struct device *dev = indio_dev->dev.parent;
> > > > > +       const char *dma_name;
> > > > > +
> > > > > +       if (!device_property_present(dev, "dmas"))
> > > > > +               return 0;
> > > > > +
> > > > > +       if (device_property_read_string(dev, "dma-names", &dma_name))
> > > > > +               dma_name = "rx";
> > > > > +
> > > > > +       return devm_iio_dmaengine_buffer_setup(dev, indio_dev, dma_name);    
> > > > 
> > > > The device tree bindings for "adi,ad9467" don't include dma properties
> > > > (nor should they). Perhaps the DMA lookup should be a callback to the
> > > > backend? Or something similar to the SPI Engine offload that we are
> > > > working on?
> > > >     
> > > 
> > > Oh yes, I need to update the bindings. In the link I sent you we can see my
> > > thoughts
> > > on this. In theory, hardwarewise, it would actually make sense for the DMA to be
> > > on
> > > the backend device because that's where the connection is in HW. However, since
> > > we
> > > want to have the IIO interface in the frontend, it would be hard to do that
> > > without
> > > hacking devm_iio_dmaengine_buffer_setup(). I mean, lifetime wise it would be far
> > > from
> > > wise to have the DMA buffer associated to a completely different device than the
> > > IIO
> > > parent device. I mean, one way could just be export iio_dmaengine_buffer_free()
> > > and
> > > iio_dmaengine_buffer_alloc() so we can actually control the lifetime of the
> > > buffer
> > > from the frontend device. If Jonathan is fine with this, I'm on board for it....  
> > 
> > It is going to be fiddly but I'd kind of expect the front end to be using a more
> > abstracted interface to tell the backend to start grabbing data.  Maybe that ends
> > up being so slim it's just these interfaces and it's not sensible to wrap it
> > though.
> >   
> 
> Likely I'm missing your point but the discussion here is where the DMA buffer should
> be allocated. In theory, in the backend (at least on ADI usecases - it's the proper
> representation of the HW) but as we have the iio device in the frontend, it's more
> appropriate to have the buffer there. Or at least to have a way to control the buffer
> lifetime from there...

My instinct was put it in the backened and proxy / interfaces as necessary but (based
on my vague recollection of how this works) at times these DMA buffers are handed off
to userspace which is a front end problem rather than the hardware.  So I guess it's
a question of who logically creates them?  Then as  you say provide the controls for
the other part to mess with their lifetimes or at least ensure the stick around whilst
it expects them to.

> 
> On the our usecases, it's not like we tell the backend to grab data, we just use the
> normal .update_scan_mode() to enable/disable the channels in the backend so when we
> enable the buffer (and the frontend starts receiving and sending data via the serial
> interface) the data paths are enabaled/disabled accordingly. Bah, yeah, in a way is
> another wording for "grab" or "grab not" :)

Yup. It's not as easily separated as simple always on analog only front ends. Someone
drives the clock ultimately and that could be either end in theory at least.

What fun.

J
> 
> - Nuno Sá

Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework

[Nuno Sá]

On Thu, 2023-11-30 at 17:30 -0600, David Lechner wrote:
> On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > Convert the driver to use the new IIO backend framework. The device
> > functionality is expected to be the same (meaning no added or removed
> > features).
> 
> Missing a devicetree bindings patch before this one?
> 
> > 
> > Also note this patch effectively breaks ABI and that's needed so we can
> > properly support this device and add needed features making use of the
> > new IIO framework.
> 
> Can you be more specific about what is actually breaking?

Device name for example changed. And it might be some other subtle breakage but that
was kind of agreed that would an ADI problem. I'm also fairly confident no one is
actually using the upstream version of the driver because it lacks some devices and
important features (like interface tuning).

- Nuno Sá

[PATCH 11/12] iio: adc: adi-axi-adc: convert to regmap

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

Use MMIO regmap interface. It makes things easier for manipulating bits.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/adi-axi-adc.c | 85 ++++++++++++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 33 deletions(-)

diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index ae83ada7f9f2..c247ff1541d2 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -14,6 +14,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 
 #include <linux/iio/iio.h>
@@ -62,7 +63,7 @@ struct adi_axi_adc_state {
 	struct mutex				lock;
 
 	struct adi_axi_adc_client		*client;
-	void __iomem				*regs;
+	struct regmap				*regmap;
 };
 
 struct adi_axi_adc_client {
@@ -90,19 +91,6 @@ void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
 }
 EXPORT_SYMBOL_NS_GPL(adi_axi_adc_conv_priv, IIO_ADI_AXI);
 
-static void adi_axi_adc_write(struct adi_axi_adc_state *st,
-			      unsigned int reg,
-			      unsigned int val)
-{
-	iowrite32(val, st->regs + reg);
-}
-
-static unsigned int adi_axi_adc_read(struct adi_axi_adc_state *st,
-				     unsigned int reg)
-{
-	return ioread32(st->regs + reg);
-}
-
 static int adi_axi_adc_config_dma_buffer(struct device *dev,
 					 struct iio_dev *indio_dev)
 {
@@ -163,17 +151,20 @@ static int adi_axi_adc_update_scan_mode(struct iio_dev *indio_dev,
 {
 	struct adi_axi_adc_state *st = iio_priv(indio_dev);
 	struct adi_axi_adc_conv *conv = &st->client->conv;
-	unsigned int i, ctrl;
+	unsigned int i;
+	int ret;
 
 	for (i = 0; i < conv->chip_info->num_channels; i++) {
-		ctrl = adi_axi_adc_read(st, ADI_AXI_REG_CHAN_CTRL(i));
-
 		if (test_bit(i, scan_mask))
-			ctrl |= ADI_AXI_REG_CHAN_CTRL_ENABLE;
+			ret = regmap_set_bits(st->regmap,
+					      ADI_AXI_REG_CHAN_CTRL(i),
+					      ADI_AXI_REG_CHAN_CTRL_ENABLE);
 		else
-			ctrl &= ~ADI_AXI_REG_CHAN_CTRL_ENABLE;
-
-		adi_axi_adc_write(st, ADI_AXI_REG_CHAN_CTRL(i), ctrl);
+			ret = regmap_clear_bits(st->regmap,
+						ADI_AXI_REG_CHAN_CTRL(i),
+						ADI_AXI_REG_CHAN_CTRL_ENABLE);
+		if (ret)
+			return ret;
 	}
 
 	return 0;
@@ -310,21 +301,32 @@ static int adi_axi_adc_setup_channels(struct device *dev,
 	}
 
 	for (i = 0; i < conv->chip_info->num_channels; i++) {
-		adi_axi_adc_write(st, ADI_AXI_REG_CHAN_CTRL(i),
-				  ADI_AXI_REG_CHAN_CTRL_DEFAULTS);
+		ret = regmap_write(st->regmap, ADI_AXI_REG_CHAN_CTRL(i),
+				   ADI_AXI_REG_CHAN_CTRL_DEFAULTS);
+		if (ret)
+			return ret;
 	}
 
 	return 0;
 }
 
-static void axi_adc_reset(struct adi_axi_adc_state *st)
+static int axi_adc_reset(struct adi_axi_adc_state *st)
 {
-	adi_axi_adc_write(st, ADI_AXI_REG_RSTN, 0);
+	int ret;
+
+	ret = regmap_write(st->regmap, ADI_AXI_REG_RSTN, 0);
+	if (ret)
+		return ret;
+
 	mdelay(10);
-	adi_axi_adc_write(st, ADI_AXI_REG_RSTN, ADI_AXI_REG_RSTN_MMCM_RSTN);
+	ret = regmap_write(st->regmap, ADI_AXI_REG_RSTN,
+			   ADI_AXI_REG_RSTN_MMCM_RSTN);
+	if (ret)
+		return ret;
+
 	mdelay(10);
-	adi_axi_adc_write(st, ADI_AXI_REG_RSTN,
-			  ADI_AXI_REG_RSTN_RSTN | ADI_AXI_REG_RSTN_MMCM_RSTN);
+	return regmap_write(st->regmap, ADI_AXI_REG_RSTN,
+			    ADI_AXI_REG_RSTN_RSTN | ADI_AXI_REG_RSTN_MMCM_RSTN);
 }
 
 static void adi_axi_adc_cleanup(void *data)
@@ -335,12 +337,20 @@ static void adi_axi_adc_cleanup(void *data)
 	module_put(cl->dev->driver->owner);
 }
 
+static const struct regmap_config axi_adc_regmap_config = {
+	.val_bits = 32,
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.max_register = 0x0800,
+};
+
 static int adi_axi_adc_probe(struct platform_device *pdev)
 {
 	struct adi_axi_adc_conv *conv;
 	struct iio_dev *indio_dev;
 	struct adi_axi_adc_client *cl;
 	struct adi_axi_adc_state *st;
+	void __iomem *base;
 	unsigned int ver;
 	int ret;
 
@@ -361,15 +371,24 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
 	cl->state = st;
 	mutex_init(&st->lock);
 
-	st->regs = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(st->regs))
-		return PTR_ERR(st->regs);
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	st->regmap = devm_regmap_init_mmio(&pdev->dev, base,
+					   &axi_adc_regmap_config);
+	if (IS_ERR(st->regmap))
+		return PTR_ERR(st->regmap);
 
 	conv = &st->client->conv;
 
-	axi_adc_reset(st);
+	ret = axi_adc_reset(st);
+	if (ret)
+		return ret;
 
-	ver = adi_axi_adc_read(st, ADI_AXI_REG_VERSION);
+	ret = regmap_read(st->regmap, ADI_AXI_REG_VERSION, &ver);
+	if (ret)
+		return ret;
 
 	if (cl->info->version > ver) {
 		dev_err(&pdev->dev,

-- 
2.42.1

Re: [PATCH 11/12] iio: adc: adi-axi-adc: convert to regmap

[Jonathan Cameron]

On Tue, 21 Nov 2023 11:20:24 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:

> From: Nuno Sa <nuno.sa@analog.com>
> 
> Use MMIO regmap interface. It makes things easier for manipulating bits.
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
Perhaps put this in the precursor set as well. Looks fine to me and will just
be noise in the main discussion.

Jonathan

> ---
>  drivers/iio/adc/adi-axi-adc.c | 85 ++++++++++++++++++++++++++-----------------
>  1 file changed, 52 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> index ae83ada7f9f2..c247ff1541d2 100644
> --- a/drivers/iio/adc/adi-axi-adc.c
> +++ b/drivers/iio/adc/adi-axi-adc.c
> @@ -14,6 +14,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
> +#include <linux/regmap.h>
>  #include <linux/slab.h>
>  
>  #include <linux/iio/iio.h>
> @@ -62,7 +63,7 @@ struct adi_axi_adc_state {
>  	struct mutex				lock;
>  
>  	struct adi_axi_adc_client		*client;
> -	void __iomem				*regs;
> +	struct regmap				*regmap;
>  };
>  
>  struct adi_axi_adc_client {
> @@ -90,19 +91,6 @@ void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
>  }
>  EXPORT_SYMBOL_NS_GPL(adi_axi_adc_conv_priv, IIO_ADI_AXI);
>  
> -static void adi_axi_adc_write(struct adi_axi_adc_state *st,
> -			      unsigned int reg,
> -			      unsigned int val)
> -{
> -	iowrite32(val, st->regs + reg);
> -}
> -
> -static unsigned int adi_axi_adc_read(struct adi_axi_adc_state *st,
> -				     unsigned int reg)
> -{
> -	return ioread32(st->regs + reg);
> -}
> -
>  static int adi_axi_adc_config_dma_buffer(struct device *dev,
>  					 struct iio_dev *indio_dev)
>  {
> @@ -163,17 +151,20 @@ static int adi_axi_adc_update_scan_mode(struct iio_dev *indio_dev,
>  {
>  	struct adi_axi_adc_state *st = iio_priv(indio_dev);
>  	struct adi_axi_adc_conv *conv = &st->client->conv;
> -	unsigned int i, ctrl;
> +	unsigned int i;
> +	int ret;
>  
>  	for (i = 0; i < conv->chip_info->num_channels; i++) {
> -		ctrl = adi_axi_adc_read(st, ADI_AXI_REG_CHAN_CTRL(i));
> -
>  		if (test_bit(i, scan_mask))
> -			ctrl |= ADI_AXI_REG_CHAN_CTRL_ENABLE;
> +			ret = regmap_set_bits(st->regmap,
> +					      ADI_AXI_REG_CHAN_CTRL(i),
> +					      ADI_AXI_REG_CHAN_CTRL_ENABLE);
>  		else
> -			ctrl &= ~ADI_AXI_REG_CHAN_CTRL_ENABLE;
> -
> -		adi_axi_adc_write(st, ADI_AXI_REG_CHAN_CTRL(i), ctrl);
> +			ret = regmap_clear_bits(st->regmap,
> +						ADI_AXI_REG_CHAN_CTRL(i),
> +						ADI_AXI_REG_CHAN_CTRL_ENABLE);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	return 0;
> @@ -310,21 +301,32 @@ static int adi_axi_adc_setup_channels(struct device *dev,
>  	}
>  
>  	for (i = 0; i < conv->chip_info->num_channels; i++) {
> -		adi_axi_adc_write(st, ADI_AXI_REG_CHAN_CTRL(i),
> -				  ADI_AXI_REG_CHAN_CTRL_DEFAULTS);
> +		ret = regmap_write(st->regmap, ADI_AXI_REG_CHAN_CTRL(i),
> +				   ADI_AXI_REG_CHAN_CTRL_DEFAULTS);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	return 0;
>  }
>  
> -static void axi_adc_reset(struct adi_axi_adc_state *st)
> +static int axi_adc_reset(struct adi_axi_adc_state *st)
>  {
> -	adi_axi_adc_write(st, ADI_AXI_REG_RSTN, 0);
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, ADI_AXI_REG_RSTN, 0);
> +	if (ret)
> +		return ret;
> +
>  	mdelay(10);
> -	adi_axi_adc_write(st, ADI_AXI_REG_RSTN, ADI_AXI_REG_RSTN_MMCM_RSTN);
> +	ret = regmap_write(st->regmap, ADI_AXI_REG_RSTN,
> +			   ADI_AXI_REG_RSTN_MMCM_RSTN);
> +	if (ret)
> +		return ret;
> +
>  	mdelay(10);
> -	adi_axi_adc_write(st, ADI_AXI_REG_RSTN,
> -			  ADI_AXI_REG_RSTN_RSTN | ADI_AXI_REG_RSTN_MMCM_RSTN);
> +	return regmap_write(st->regmap, ADI_AXI_REG_RSTN,
> +			    ADI_AXI_REG_RSTN_RSTN | ADI_AXI_REG_RSTN_MMCM_RSTN);
>  }
>  
>  static void adi_axi_adc_cleanup(void *data)
> @@ -335,12 +337,20 @@ static void adi_axi_adc_cleanup(void *data)
>  	module_put(cl->dev->driver->owner);
>  }
>  
> +static const struct regmap_config axi_adc_regmap_config = {
> +	.val_bits = 32,
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = 0x0800,
> +};
> +
>  static int adi_axi_adc_probe(struct platform_device *pdev)
>  {
>  	struct adi_axi_adc_conv *conv;
>  	struct iio_dev *indio_dev;
>  	struct adi_axi_adc_client *cl;
>  	struct adi_axi_adc_state *st;
> +	void __iomem *base;
>  	unsigned int ver;
>  	int ret;
>  
> @@ -361,15 +371,24 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
>  	cl->state = st;
>  	mutex_init(&st->lock);
>  
> -	st->regs = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(st->regs))
> -		return PTR_ERR(st->regs);
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	st->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> +					   &axi_adc_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return PTR_ERR(st->regmap);
>  
>  	conv = &st->client->conv;
>  
> -	axi_adc_reset(st);
> +	ret = axi_adc_reset(st);
> +	if (ret)
> +		return ret;
>  
> -	ver = adi_axi_adc_read(st, ADI_AXI_REG_VERSION);
> +	ret = regmap_read(st->regmap, ADI_AXI_REG_VERSION, &ver);
> +	if (ret)
> +		return ret;
>  
>  	if (cl->info->version > ver) {
>  		dev_err(&pdev->dev,
> 

Re: [PATCH 11/12] iio: adc: adi-axi-adc: convert to regmap

[Nuno Sá]

On Mon, 2023-12-04 at 15:51 +0000, Jonathan Cameron wrote:
> On Tue, 21 Nov 2023 11:20:24 +0100
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > Use MMIO regmap interface. It makes things easier for manipulating bits.
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> Perhaps put this in the precursor set as well. Looks fine to me and will just
> be noise in the main discussion.
> 

will do...


- Nuno Sá

[PATCH 12/12] iio: adc: adi-axi-adc: move to backend framework

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

Move to the IIO backend framework. Devices supported by adi-axi-adc now
register themselves as backend devices.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/Kconfig       |   1 +
 drivers/iio/adc/adi-axi-adc.c | 364 ++++++++----------------------------------
 2 files changed, 65 insertions(+), 300 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index af56df63beff..cc42a3399c63 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -292,6 +292,7 @@ config ADI_AXI_ADC
 	select IIO_BUFFER
 	select IIO_BUFFER_HW_CONSUMER
 	select IIO_BUFFER_DMAENGINE
+	select IIO_BACKEND
 	depends on HAS_IOMEM
 	depends on OF
 	help
diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index c247ff1541d2..b2ab2c119efa 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -17,13 +17,9 @@
 #include <linux/regmap.h>
 #include <linux/slab.h>
 
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
-#include <linux/iio/buffer.h>
-#include <linux/iio/buffer-dmaengine.h>
-
 #include <linux/fpga/adi-axi-common.h>
-#include <linux/iio/adc/adi-axi-adc.h>
+#include <linux/iio/backend.h>
+
 
 /*
  * Register definitions:
@@ -44,6 +40,7 @@
 #define   ADI_AXI_REG_CHAN_CTRL_PN_SEL_OWR	BIT(10)
 #define   ADI_AXI_REG_CHAN_CTRL_IQCOR_EN	BIT(9)
 #define   ADI_AXI_REG_CHAN_CTRL_DCFILT_EN	BIT(8)
+#define   ADI_AXI_REG_CHAN_CTRL_FMT_MASK	GENMASK(6, 4)
 #define   ADI_AXI_REG_CHAN_CTRL_FMT_SIGNEXT	BIT(6)
 #define   ADI_AXI_REG_CHAN_CTRL_FMT_TYPE	BIT(5)
 #define   ADI_AXI_REG_CHAN_CTRL_FMT_EN		BIT(4)
@@ -55,286 +52,67 @@
 	 ADI_AXI_REG_CHAN_CTRL_FMT_EN |		\
 	 ADI_AXI_REG_CHAN_CTRL_ENABLE)
 
-struct adi_axi_adc_core_info {
-	unsigned int				version;
-};
-
 struct adi_axi_adc_state {
-	struct mutex				lock;
-
-	struct adi_axi_adc_client		*client;
 	struct regmap				*regmap;
 };
 
-struct adi_axi_adc_client {
-	struct list_head			entry;
-	struct adi_axi_adc_conv			conv;
-	struct adi_axi_adc_state		*state;
-	struct device				*dev;
-	const struct adi_axi_adc_core_info	*info;
-};
-
-static LIST_HEAD(registered_clients);
-static DEFINE_MUTEX(registered_clients_lock);
-
-static struct adi_axi_adc_client *conv_to_client(struct adi_axi_adc_conv *conv)
-{
-	return container_of(conv, struct adi_axi_adc_client, conv);
-}
-
-void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
-{
-	struct adi_axi_adc_client *cl = conv_to_client(conv);
-
-	return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client),
-				  IIO_DMA_MINALIGN);
-}
-EXPORT_SYMBOL_NS_GPL(adi_axi_adc_conv_priv, IIO_ADI_AXI);
-
-static int adi_axi_adc_config_dma_buffer(struct device *dev,
-					 struct iio_dev *indio_dev)
-{
-	const char *dma_name;
-
-	if (!device_property_present(dev, "dmas"))
-		return 0;
-
-	if (device_property_read_string(dev, "dma-names", &dma_name))
-		dma_name = "rx";
-
-	return devm_iio_dmaengine_buffer_setup(indio_dev->dev.parent,
-					       indio_dev, dma_name);
-}
-
-static int adi_axi_adc_read_raw(struct iio_dev *indio_dev,
-				struct iio_chan_spec const *chan,
-				int *val, int *val2, long mask)
-{
-	struct adi_axi_adc_state *st = iio_priv(indio_dev);
-	struct adi_axi_adc_conv *conv = &st->client->conv;
-
-	if (!conv->read_raw)
-		return -EOPNOTSUPP;
-
-	return conv->read_raw(conv, chan, val, val2, mask);
-}
-
-static int adi_axi_adc_write_raw(struct iio_dev *indio_dev,
-				 struct iio_chan_spec const *chan,
-				 int val, int val2, long mask)
-{
-	struct adi_axi_adc_state *st = iio_priv(indio_dev);
-	struct adi_axi_adc_conv *conv = &st->client->conv;
-
-	if (!conv->write_raw)
-		return -EOPNOTSUPP;
-
-	return conv->write_raw(conv, chan, val, val2, mask);
-}
-
-static int adi_axi_adc_read_avail(struct iio_dev *indio_dev,
-				  struct iio_chan_spec const *chan,
-				  const int **vals, int *type, int *length,
-				  long mask)
-{
-	struct adi_axi_adc_state *st = iio_priv(indio_dev);
-	struct adi_axi_adc_conv *conv = &st->client->conv;
-
-	if (!conv->read_avail)
-		return -EOPNOTSUPP;
-
-	return conv->read_avail(conv, chan, vals, type, length, mask);
-}
-
-static int adi_axi_adc_update_scan_mode(struct iio_dev *indio_dev,
-					const unsigned long *scan_mask)
-{
-	struct adi_axi_adc_state *st = iio_priv(indio_dev);
-	struct adi_axi_adc_conv *conv = &st->client->conv;
-	unsigned int i;
-	int ret;
-
-	for (i = 0; i < conv->chip_info->num_channels; i++) {
-		if (test_bit(i, scan_mask))
-			ret = regmap_set_bits(st->regmap,
-					      ADI_AXI_REG_CHAN_CTRL(i),
-					      ADI_AXI_REG_CHAN_CTRL_ENABLE);
-		else
-			ret = regmap_clear_bits(st->regmap,
-						ADI_AXI_REG_CHAN_CTRL(i),
-						ADI_AXI_REG_CHAN_CTRL_ENABLE);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-
-static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device *dev,
-							  size_t sizeof_priv)
-{
-	struct adi_axi_adc_client *cl;
-	size_t alloc_size;
-
-	alloc_size = ALIGN(sizeof(struct adi_axi_adc_client), IIO_DMA_MINALIGN);
-	if (sizeof_priv)
-		alloc_size += ALIGN(sizeof_priv, IIO_DMA_MINALIGN);
-
-	cl = kzalloc(alloc_size, GFP_KERNEL);
-	if (!cl)
-		return ERR_PTR(-ENOMEM);
-
-	mutex_lock(&registered_clients_lock);
-
-	cl->dev = get_device(dev);
-
-	list_add_tail(&cl->entry, &registered_clients);
-
-	mutex_unlock(&registered_clients_lock);
-
-	return &cl->conv;
-}
-
-static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
-{
-	struct adi_axi_adc_client *cl = conv_to_client(conv);
-
-	mutex_lock(&registered_clients_lock);
-
-	list_del(&cl->entry);
-	put_device(cl->dev);
-
-	mutex_unlock(&registered_clients_lock);
-
-	kfree(cl);
-}
-
-static void devm_adi_axi_adc_conv_release(void *conv)
-{
-	adi_axi_adc_conv_unregister(conv);
-}
-
-struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
-							size_t sizeof_priv)
+static int axi_adc_enable(struct iio_backend *back)
 {
-	struct adi_axi_adc_conv *conv;
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
 	int ret;
 
-	conv = adi_axi_adc_conv_register(dev, sizeof_priv);
-	if (IS_ERR(conv))
-		return conv;
-
-	ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release,
-				       conv);
+	 ret = regmap_set_bits(st->regmap, ADI_AXI_REG_RSTN,
+			       ADI_AXI_REG_RSTN_MMCM_RSTN);
 	if (ret)
-		return ERR_PTR(ret);
+		return ret;
 
-	return conv;
+	fsleep(10);
+	return regmap_set_bits(st->regmap, ADI_AXI_REG_RSTN,
+			       ADI_AXI_REG_RSTN_RSTN | ADI_AXI_REG_RSTN_MMCM_RSTN);
 }
-EXPORT_SYMBOL_NS_GPL(devm_adi_axi_adc_conv_register, IIO_ADI_AXI);
 
-static const struct iio_info adi_axi_adc_info = {
-	.read_raw = &adi_axi_adc_read_raw,
-	.write_raw = &adi_axi_adc_write_raw,
-	.update_scan_mode = &adi_axi_adc_update_scan_mode,
-	.read_avail = &adi_axi_adc_read_avail,
-};
-
-static const struct adi_axi_adc_core_info adi_axi_adc_10_0_a_info = {
-	.version = ADI_AXI_PCORE_VER(10, 0, 'a'),
-};
-
-static struct adi_axi_adc_client *adi_axi_adc_attach_client(struct device *dev)
+static void axi_adc_disable(struct iio_backend *back)
 {
-	const struct adi_axi_adc_core_info *info;
-	struct adi_axi_adc_client *cl;
-	struct device_node *cln;
-
-	info = of_device_get_match_data(dev);
-	if (!info)
-		return ERR_PTR(-ENODEV);
-
-	cln = of_parse_phandle(dev->of_node, "adi,adc-dev", 0);
-	if (!cln) {
-		dev_err(dev, "No 'adi,adc-dev' node defined\n");
-		return ERR_PTR(-ENODEV);
-	}
-
-	mutex_lock(&registered_clients_lock);
-
-	list_for_each_entry(cl, &registered_clients, entry) {
-		if (!cl->dev)
-			continue;
-
-		if (cl->dev->of_node != cln)
-			continue;
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
 
-		if (!try_module_get(cl->dev->driver->owner)) {
-			mutex_unlock(&registered_clients_lock);
-			of_node_put(cln);
-			return ERR_PTR(-ENODEV);
-		}
-
-		get_device(cl->dev);
-		cl->info = info;
-		mutex_unlock(&registered_clients_lock);
-		of_node_put(cln);
-		return cl;
-	}
-
-	mutex_unlock(&registered_clients_lock);
-	of_node_put(cln);
-
-	return ERR_PTR(-EPROBE_DEFER);
+	regmap_write(st->regmap, ADI_AXI_REG_RSTN, 0);
 }
 
-static int adi_axi_adc_setup_channels(struct device *dev,
-				      struct adi_axi_adc_state *st)
+static int axi_adc_data_format_set(struct iio_backend *back, unsigned int chan,
+				   const struct iio_backend_data_fmt *data)
 {
-	struct adi_axi_adc_conv *conv = &st->client->conv;
-	int i, ret;
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
+	u32 val;
 
-	if (conv->preenable_setup) {
-		ret = conv->preenable_setup(conv);
-		if (ret)
-			return ret;
-	}
+	if (!data->enable)
+		return regmap_clear_bits(st->regmap,
+					 ADI_AXI_REG_CHAN_CTRL(chan),
+					 ADI_AXI_REG_CHAN_CTRL_FMT_EN);
 
-	for (i = 0; i < conv->chip_info->num_channels; i++) {
-		ret = regmap_write(st->regmap, ADI_AXI_REG_CHAN_CTRL(i),
-				   ADI_AXI_REG_CHAN_CTRL_DEFAULTS);
-		if (ret)
-			return ret;
-	}
+	val = FIELD_PREP(ADI_AXI_REG_CHAN_CTRL_FMT_EN, true);
+	if (data->sign_extend)
+		val |= FIELD_PREP(ADI_AXI_REG_CHAN_CTRL_FMT_SIGNEXT, true);
+	if (data->type == IIO_BACKEND_OFFSET_BINARY)
+		val |= FIELD_PREP(ADI_AXI_REG_CHAN_CTRL_FMT_TYPE, true);
 
-	return 0;
+	return regmap_update_bits(st->regmap, ADI_AXI_REG_CHAN_CTRL(chan),
+				  ADI_AXI_REG_CHAN_CTRL_FMT_MASK, val);
 }
 
-static int axi_adc_reset(struct adi_axi_adc_state *st)
+static int axi_adc_chan_enable(struct iio_backend *back, unsigned int chan)
 {
-	int ret;
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
 
-	ret = regmap_write(st->regmap, ADI_AXI_REG_RSTN, 0);
-	if (ret)
-		return ret;
-
-	mdelay(10);
-	ret = regmap_write(st->regmap, ADI_AXI_REG_RSTN,
-			   ADI_AXI_REG_RSTN_MMCM_RSTN);
-	if (ret)
-		return ret;
-
-	mdelay(10);
-	return regmap_write(st->regmap, ADI_AXI_REG_RSTN,
-			    ADI_AXI_REG_RSTN_RSTN | ADI_AXI_REG_RSTN_MMCM_RSTN);
+	return regmap_set_bits(st->regmap, ADI_AXI_REG_CHAN_CTRL(chan),
+			       ADI_AXI_REG_CHAN_CTRL_ENABLE);
 }
 
-static void adi_axi_adc_cleanup(void *data)
+static int axi_adc_chan_disable(struct iio_backend *back, unsigned int chan)
 {
-	struct adi_axi_adc_client *cl = data;
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
 
-	put_device(cl->dev);
-	module_put(cl->dev->driver->owner);
+	return regmap_clear_bits(st->regmap, ADI_AXI_REG_CHAN_CTRL(chan),
+				 ADI_AXI_REG_CHAN_CTRL_ENABLE);
 }
 
 static const struct regmap_config axi_adc_regmap_config = {
@@ -344,33 +122,25 @@ static const struct regmap_config axi_adc_regmap_config = {
 	.max_register = 0x0800,
 };
 
+static const struct iio_backend_ops adi_axi_adc_generic = {
+	.enable = axi_adc_enable,
+	.disable = axi_adc_disable,
+	.data_format_set = axi_adc_data_format_set,
+	.chan_enable = axi_adc_chan_enable,
+	.chan_disable = axi_adc_chan_disable,
+};
+
 static int adi_axi_adc_probe(struct platform_device *pdev)
 {
-	struct adi_axi_adc_conv *conv;
-	struct iio_dev *indio_dev;
-	struct adi_axi_adc_client *cl;
 	struct adi_axi_adc_state *st;
 	void __iomem *base;
-	unsigned int ver;
+	unsigned int ver, *expected_ver;
 	int ret;
 
-	cl = adi_axi_adc_attach_client(&pdev->dev);
-	if (IS_ERR(cl))
-		return PTR_ERR(cl);
-
-	ret = devm_add_action_or_reset(&pdev->dev, adi_axi_adc_cleanup, cl);
-	if (ret)
-		return ret;
-
-	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*st));
-	if (indio_dev == NULL)
+	st = devm_kzalloc(&pdev->dev, sizeof(*st), GFP_KERNEL);
+	if (!st)
 		return -ENOMEM;
 
-	st = iio_priv(indio_dev);
-	st->client = cl;
-	cl->state = st;
-	mutex_init(&st->lock);
-
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
@@ -380,9 +150,15 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
 	if (IS_ERR(st->regmap))
 		return PTR_ERR(st->regmap);
 
-	conv = &st->client->conv;
+	expected_ver = (unsigned int *)device_get_match_data(&pdev->dev);
+	if (!expected_ver)
+		return -ENODEV;
 
-	ret = axi_adc_reset(st);
+	/*
+	 * Force disable the core. Up to the frontend to enable us. And we can
+	 * still read/write registers...
+	 */
+	ret = regmap_write(st->regmap, ADI_AXI_REG_RSTN, 0);
 	if (ret)
 		return ret;
 
@@ -390,37 +166,23 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	if (cl->info->version > ver) {
+	if (*expected_ver > ver) {
 		dev_err(&pdev->dev,
 			"IP core version is too old. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n",
-			ADI_AXI_PCORE_VER_MAJOR(cl->info->version),
-			ADI_AXI_PCORE_VER_MINOR(cl->info->version),
-			ADI_AXI_PCORE_VER_PATCH(cl->info->version),
+			ADI_AXI_PCORE_VER_MAJOR(*expected_ver),
+			ADI_AXI_PCORE_VER_MINOR(*expected_ver),
+			ADI_AXI_PCORE_VER_PATCH(*expected_ver),
 			ADI_AXI_PCORE_VER_MAJOR(ver),
 			ADI_AXI_PCORE_VER_MINOR(ver),
 			ADI_AXI_PCORE_VER_PATCH(ver));
 		return -ENODEV;
 	}
 
-	indio_dev->info = &adi_axi_adc_info;
-	indio_dev->name = "adi-axi-adc";
-	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->num_channels = conv->chip_info->num_channels;
-	indio_dev->channels = conv->chip_info->channels;
-
-	ret = adi_axi_adc_config_dma_buffer(&pdev->dev, indio_dev);
+	ret = devm_iio_backend_register(&pdev->dev, &adi_axi_adc_generic, st);
 	if (ret)
 		return ret;
 
-	ret = adi_axi_adc_setup_channels(&pdev->dev, st);
-	if (ret)
-		return ret;
-
-	ret = devm_iio_device_register(&pdev->dev, indio_dev);
-	if (ret)
-		return ret;
-
-	dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%c) probed\n",
+	dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%d) probed\n",
 		 ADI_AXI_PCORE_VER_MAJOR(ver),
 		 ADI_AXI_PCORE_VER_MINOR(ver),
 		 ADI_AXI_PCORE_VER_PATCH(ver));
@@ -428,6 +190,8 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static unsigned int adi_axi_adc_10_0_a_info = ADI_AXI_PCORE_VER(10, 0, 'a');
+
 /* Match table for of_platform binding */
 static const struct of_device_id adi_axi_adc_of_match[] = {
 	{ .compatible = "adi,axi-adc-10.0.a", .data = &adi_axi_adc_10_0_a_info },

-- 
2.42.1

Re: [PATCH 12/12] iio: adc: adi-axi-adc: move to backend framework

[kernel test robot]

Hi Nuno,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus robh/for-next linus/master v6.7-rc2 next-20231121]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nuno-Sa-via-B4-Relay/driver-core-allow-modifying-device_links-flags/20231121-182010
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20231121-dev-iio-backend-v1-12-6a3d542eba35%40analog.com
patch subject: [PATCH 12/12] iio: adc: adi-axi-adc: move to backend framework
config: i386-randconfig-141-20231122 (https://download.01.org/0day-ci/archive/20231122/202311220748.T1FnZwoy-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231122/202311220748.T1FnZwoy-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311220748.T1FnZwoy-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/industrialio-backend.c:123: warning: expecting prototype for iio_backend_chan_enable(). Prototype was for iio_backend_enable() instead
>> drivers/iio/industrialio-backend.c:242: warning: Function parameter or member 'back' not described in 'iio_backend_get_priv'
>> drivers/iio/industrialio-backend.c:274: warning: Function parameter or member 'dev' not described in 'devm_iio_backend_register'
>> drivers/iio/industrialio-backend.c:274: warning: Function parameter or member 'ops' not described in 'devm_iio_backend_register'
>> drivers/iio/industrialio-backend.c:274: warning: Function parameter or member 'priv' not described in 'devm_iio_backend_register'


vim +242 drivers/iio/industrialio-backend.c

67915cd5ae2cc11 Nuno Sa 2023-11-21  114  
67915cd5ae2cc11 Nuno Sa 2023-11-21  115  /**
67915cd5ae2cc11 Nuno Sa 2023-11-21  116   * iio_backend_chan_enable - Enable the backend.
67915cd5ae2cc11 Nuno Sa 2023-11-21  117   * @back:	Backend device
67915cd5ae2cc11 Nuno Sa 2023-11-21  118   *
67915cd5ae2cc11 Nuno Sa 2023-11-21  119   * RETURNS:
67915cd5ae2cc11 Nuno Sa 2023-11-21  120   * 0 on success, negative error number on failure.
67915cd5ae2cc11 Nuno Sa 2023-11-21  121   */
67915cd5ae2cc11 Nuno Sa 2023-11-21  122  int iio_backend_enable(struct iio_backend *back)
67915cd5ae2cc11 Nuno Sa 2023-11-21 @123  {
67915cd5ae2cc11 Nuno Sa 2023-11-21  124  	return iio_backend_op_call(back, enable);
67915cd5ae2cc11 Nuno Sa 2023-11-21  125  }
67915cd5ae2cc11 Nuno Sa 2023-11-21  126  EXPORT_SYMBOL_GPL(iio_backend_enable);
67915cd5ae2cc11 Nuno Sa 2023-11-21  127  
67915cd5ae2cc11 Nuno Sa 2023-11-21  128  /**
67915cd5ae2cc11 Nuno Sa 2023-11-21  129   * iio_backend_disable - Disable the backend.
67915cd5ae2cc11 Nuno Sa 2023-11-21  130   * @back:	Backend device
67915cd5ae2cc11 Nuno Sa 2023-11-21  131   */
67915cd5ae2cc11 Nuno Sa 2023-11-21  132  void iio_backend_disable(struct iio_backend *back)
67915cd5ae2cc11 Nuno Sa 2023-11-21  133  {
67915cd5ae2cc11 Nuno Sa 2023-11-21  134  	iio_backend_void_op_call(back, disable);
67915cd5ae2cc11 Nuno Sa 2023-11-21  135  }
67915cd5ae2cc11 Nuno Sa 2023-11-21  136  EXPORT_SYMBOL_GPL(iio_backend_disable);
67915cd5ae2cc11 Nuno Sa 2023-11-21  137  
67915cd5ae2cc11 Nuno Sa 2023-11-21  138  /**
67915cd5ae2cc11 Nuno Sa 2023-11-21  139   * iio_backend_data_format_set - Configure the channel data format
67915cd5ae2cc11 Nuno Sa 2023-11-21  140   * @back:	Backend device
67915cd5ae2cc11 Nuno Sa 2023-11-21  141   * @chan:	Channel number.
67915cd5ae2cc11 Nuno Sa 2023-11-21  142   * @data:	Data format.
67915cd5ae2cc11 Nuno Sa 2023-11-21  143   *
67915cd5ae2cc11 Nuno Sa 2023-11-21  144   * Properly configure a channel with respect to the expected data format. A
67915cd5ae2cc11 Nuno Sa 2023-11-21  145   * @struct iio_backend_data_fmt must be passed with the settings.
67915cd5ae2cc11 Nuno Sa 2023-11-21  146   *
67915cd5ae2cc11 Nuno Sa 2023-11-21  147   * RETURNS:
67915cd5ae2cc11 Nuno Sa 2023-11-21  148   * 0 on success, negative error number on failure
67915cd5ae2cc11 Nuno Sa 2023-11-21  149   */
67915cd5ae2cc11 Nuno Sa 2023-11-21  150  int iio_backend_data_format_set(struct iio_backend *back, unsigned int chan,
67915cd5ae2cc11 Nuno Sa 2023-11-21  151  				const struct iio_backend_data_fmt *data)
67915cd5ae2cc11 Nuno Sa 2023-11-21  152  {
67915cd5ae2cc11 Nuno Sa 2023-11-21  153  	if (!data || data->type >= IIO_BACKEND_DATA_TYPE_MAX)
67915cd5ae2cc11 Nuno Sa 2023-11-21  154  		return -EINVAL;
67915cd5ae2cc11 Nuno Sa 2023-11-21  155  
67915cd5ae2cc11 Nuno Sa 2023-11-21  156  	return iio_backend_op_call(back, data_format_set, chan, data);
67915cd5ae2cc11 Nuno Sa 2023-11-21  157  }
67915cd5ae2cc11 Nuno Sa 2023-11-21  158  EXPORT_SYMBOL_GPL(iio_backend_data_format_set);
67915cd5ae2cc11 Nuno Sa 2023-11-21  159  
67915cd5ae2cc11 Nuno Sa 2023-11-21  160  static void iio_backend_free(struct kref *ref)
67915cd5ae2cc11 Nuno Sa 2023-11-21  161  {
67915cd5ae2cc11 Nuno Sa 2023-11-21  162  	struct iio_backend *back = container_of(ref, struct iio_backend, ref);
67915cd5ae2cc11 Nuno Sa 2023-11-21  163  
67915cd5ae2cc11 Nuno Sa 2023-11-21  164  	kfree(back);
67915cd5ae2cc11 Nuno Sa 2023-11-21  165  }
67915cd5ae2cc11 Nuno Sa 2023-11-21  166  
67915cd5ae2cc11 Nuno Sa 2023-11-21  167  static void iio_backend_release(void *arg)
67915cd5ae2cc11 Nuno Sa 2023-11-21  168  {
67915cd5ae2cc11 Nuno Sa 2023-11-21  169  	struct iio_backend *back = arg;
67915cd5ae2cc11 Nuno Sa 2023-11-21  170  
67915cd5ae2cc11 Nuno Sa 2023-11-21  171  	module_put(back->owner);
67915cd5ae2cc11 Nuno Sa 2023-11-21  172  	kref_put(&back->ref, iio_backend_free);
67915cd5ae2cc11 Nuno Sa 2023-11-21  173  }
67915cd5ae2cc11 Nuno Sa 2023-11-21  174  
67915cd5ae2cc11 Nuno Sa 2023-11-21  175  /**
67915cd5ae2cc11 Nuno Sa 2023-11-21  176   * devm_iio_backend_get - Get a backend device
67915cd5ae2cc11 Nuno Sa 2023-11-21  177   * @dev:	Device where to look for the backend.
67915cd5ae2cc11 Nuno Sa 2023-11-21  178   * @name:	Backend name.
67915cd5ae2cc11 Nuno Sa 2023-11-21  179   *
67915cd5ae2cc11 Nuno Sa 2023-11-21  180   * Get's the backend associated with @dev.
67915cd5ae2cc11 Nuno Sa 2023-11-21  181   *
67915cd5ae2cc11 Nuno Sa 2023-11-21  182   * RETURNS:
67915cd5ae2cc11 Nuno Sa 2023-11-21  183   * A backend pointer, negative error pointer otherwise.
67915cd5ae2cc11 Nuno Sa 2023-11-21  184   */
67915cd5ae2cc11 Nuno Sa 2023-11-21  185  struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
67915cd5ae2cc11 Nuno Sa 2023-11-21  186  {
67915cd5ae2cc11 Nuno Sa 2023-11-21  187  	struct fwnode_handle *fwnode;
67915cd5ae2cc11 Nuno Sa 2023-11-21  188  	struct iio_backend *back;
67915cd5ae2cc11 Nuno Sa 2023-11-21  189  	int index = 0, ret;
67915cd5ae2cc11 Nuno Sa 2023-11-21  190  
67915cd5ae2cc11 Nuno Sa 2023-11-21  191  	if (name) {
67915cd5ae2cc11 Nuno Sa 2023-11-21  192  		index = device_property_match_string(dev, "io-backends-names",
67915cd5ae2cc11 Nuno Sa 2023-11-21  193  						     name);
67915cd5ae2cc11 Nuno Sa 2023-11-21  194  		if (index < 0)
67915cd5ae2cc11 Nuno Sa 2023-11-21  195  			return ERR_PTR(index);
67915cd5ae2cc11 Nuno Sa 2023-11-21  196  	}
67915cd5ae2cc11 Nuno Sa 2023-11-21  197  
67915cd5ae2cc11 Nuno Sa 2023-11-21  198  	fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index);
67915cd5ae2cc11 Nuno Sa 2023-11-21  199  	if (IS_ERR(fwnode)) {
67915cd5ae2cc11 Nuno Sa 2023-11-21  200  		dev_err(dev, "Cannot get Firmware reference\n");
67915cd5ae2cc11 Nuno Sa 2023-11-21  201  		return ERR_CAST(fwnode);
67915cd5ae2cc11 Nuno Sa 2023-11-21  202  	}
67915cd5ae2cc11 Nuno Sa 2023-11-21  203  
67915cd5ae2cc11 Nuno Sa 2023-11-21  204  	guard(mutex)(&iio_back_lock);
67915cd5ae2cc11 Nuno Sa 2023-11-21  205  	list_for_each_entry(back, &iio_back_list, entry) {
67915cd5ae2cc11 Nuno Sa 2023-11-21  206  		struct device_link *link;
67915cd5ae2cc11 Nuno Sa 2023-11-21  207  
67915cd5ae2cc11 Nuno Sa 2023-11-21  208  		if (!device_match_fwnode(back->dev, fwnode))
67915cd5ae2cc11 Nuno Sa 2023-11-21  209  			continue;
67915cd5ae2cc11 Nuno Sa 2023-11-21  210  
67915cd5ae2cc11 Nuno Sa 2023-11-21  211  		fwnode_handle_put(fwnode);
67915cd5ae2cc11 Nuno Sa 2023-11-21  212  		kref_get(&back->ref);
67915cd5ae2cc11 Nuno Sa 2023-11-21  213  		if (!try_module_get(back->owner)) {
67915cd5ae2cc11 Nuno Sa 2023-11-21  214  			dev_err(dev, "Cannot get module reference\n");
67915cd5ae2cc11 Nuno Sa 2023-11-21  215  			return ERR_PTR(-ENODEV);
67915cd5ae2cc11 Nuno Sa 2023-11-21  216  		}
67915cd5ae2cc11 Nuno Sa 2023-11-21  217  
67915cd5ae2cc11 Nuno Sa 2023-11-21  218  		ret = devm_add_action_or_reset(dev, iio_backend_release, back);
67915cd5ae2cc11 Nuno Sa 2023-11-21  219  		if (ret)
67915cd5ae2cc11 Nuno Sa 2023-11-21  220  			return ERR_PTR(ret);
67915cd5ae2cc11 Nuno Sa 2023-11-21  221  
67915cd5ae2cc11 Nuno Sa 2023-11-21  222  		link = device_link_add(dev, back->dev,
67915cd5ae2cc11 Nuno Sa 2023-11-21  223  				       DL_FLAG_AUTOREMOVE_CONSUMER);
67915cd5ae2cc11 Nuno Sa 2023-11-21  224  		if (!link)
67915cd5ae2cc11 Nuno Sa 2023-11-21  225  			dev_warn(dev, "Could not link to supplier(%s)\n",
67915cd5ae2cc11 Nuno Sa 2023-11-21  226  				 dev_name(back->dev));
67915cd5ae2cc11 Nuno Sa 2023-11-21  227  
67915cd5ae2cc11 Nuno Sa 2023-11-21  228  		dev_dbg(dev, "Found backend(%s) device\n", dev_name(back->dev));
67915cd5ae2cc11 Nuno Sa 2023-11-21  229  		return back;
67915cd5ae2cc11 Nuno Sa 2023-11-21  230  	}
67915cd5ae2cc11 Nuno Sa 2023-11-21  231  
67915cd5ae2cc11 Nuno Sa 2023-11-21  232  	fwnode_handle_put(fwnode);
67915cd5ae2cc11 Nuno Sa 2023-11-21  233  	return ERR_PTR(-EPROBE_DEFER);
67915cd5ae2cc11 Nuno Sa 2023-11-21  234  }
67915cd5ae2cc11 Nuno Sa 2023-11-21  235  EXPORT_SYMBOL_GPL(devm_iio_backend_get);
67915cd5ae2cc11 Nuno Sa 2023-11-21  236  
67915cd5ae2cc11 Nuno Sa 2023-11-21  237  /**
67915cd5ae2cc11 Nuno Sa 2023-11-21  238   * iio_backend_get_priv - Get driver private data
67915cd5ae2cc11 Nuno Sa 2023-11-21  239   * @back	Backend device
67915cd5ae2cc11 Nuno Sa 2023-11-21  240   */
67915cd5ae2cc11 Nuno Sa 2023-11-21  241  void *iio_backend_get_priv(const struct iio_backend *back)
67915cd5ae2cc11 Nuno Sa 2023-11-21 @242  {
67915cd5ae2cc11 Nuno Sa 2023-11-21  243  	return back->priv;
67915cd5ae2cc11 Nuno Sa 2023-11-21  244  }
67915cd5ae2cc11 Nuno Sa 2023-11-21  245  EXPORT_SYMBOL_GPL(iio_backend_get_priv);
67915cd5ae2cc11 Nuno Sa 2023-11-21  246  
67915cd5ae2cc11 Nuno Sa 2023-11-21  247  static void iio_backend_unregister(void *arg)
67915cd5ae2cc11 Nuno Sa 2023-11-21  248  {
67915cd5ae2cc11 Nuno Sa 2023-11-21  249  	struct iio_backend *back = arg;
67915cd5ae2cc11 Nuno Sa 2023-11-21  250  
67915cd5ae2cc11 Nuno Sa 2023-11-21  251  	mutex_lock(&iio_back_lock);
67915cd5ae2cc11 Nuno Sa 2023-11-21  252  	list_del(&back->entry);
67915cd5ae2cc11 Nuno Sa 2023-11-21  253  	mutex_unlock(&iio_back_lock);
67915cd5ae2cc11 Nuno Sa 2023-11-21  254  
67915cd5ae2cc11 Nuno Sa 2023-11-21  255  	mutex_lock(&back->lock);
67915cd5ae2cc11 Nuno Sa 2023-11-21  256  	back->ops = NULL;
67915cd5ae2cc11 Nuno Sa 2023-11-21  257  	mutex_unlock(&back->lock);
67915cd5ae2cc11 Nuno Sa 2023-11-21  258  	kref_put(&back->ref, iio_backend_free);
67915cd5ae2cc11 Nuno Sa 2023-11-21  259  }
67915cd5ae2cc11 Nuno Sa 2023-11-21  260  
67915cd5ae2cc11 Nuno Sa 2023-11-21  261  /**
67915cd5ae2cc11 Nuno Sa 2023-11-21  262   * devm_iio_backend_register - Register a new backend device
67915cd5ae2cc11 Nuno Sa 2023-11-21  263   * @dev		Backend device being registered.
67915cd5ae2cc11 Nuno Sa 2023-11-21  264   * @ops		Backend ops
67915cd5ae2cc11 Nuno Sa 2023-11-21  265   * @priv	Device private data.
67915cd5ae2cc11 Nuno Sa 2023-11-21  266   *
67915cd5ae2cc11 Nuno Sa 2023-11-21  267   * @ops and @priv are both mandatory. Not providing them results in -EINVAL.
67915cd5ae2cc11 Nuno Sa 2023-11-21  268   *
67915cd5ae2cc11 Nuno Sa 2023-11-21  269   * RETURNS:
67915cd5ae2cc11 Nuno Sa 2023-11-21  270   * 0 on success, negative error number on failure.
67915cd5ae2cc11 Nuno Sa 2023-11-21  271   */
67915cd5ae2cc11 Nuno Sa 2023-11-21  272  int devm_iio_backend_register(struct device *dev,
67915cd5ae2cc11 Nuno Sa 2023-11-21  273  			      const struct iio_backend_ops *ops, void *priv)
67915cd5ae2cc11 Nuno Sa 2023-11-21 @274  {
67915cd5ae2cc11 Nuno Sa 2023-11-21  275  	struct iio_backend *back;
67915cd5ae2cc11 Nuno Sa 2023-11-21  276  
67915cd5ae2cc11 Nuno Sa 2023-11-21  277  	if (!ops || !priv) {
67915cd5ae2cc11 Nuno Sa 2023-11-21  278  		dev_err(dev, "No backend ops or private data given\n");
67915cd5ae2cc11 Nuno Sa 2023-11-21  279  		return -EINVAL;
67915cd5ae2cc11 Nuno Sa 2023-11-21  280  	}
67915cd5ae2cc11 Nuno Sa 2023-11-21  281  
67915cd5ae2cc11 Nuno Sa 2023-11-21  282  	back = kzalloc(sizeof(*back), GFP_KERNEL);
67915cd5ae2cc11 Nuno Sa 2023-11-21  283  	if (!back)
67915cd5ae2cc11 Nuno Sa 2023-11-21  284  		return -ENOMEM;
67915cd5ae2cc11 Nuno Sa 2023-11-21  285  
67915cd5ae2cc11 Nuno Sa 2023-11-21  286  	kref_init(&back->ref);
67915cd5ae2cc11 Nuno Sa 2023-11-21  287  	mutex_init(&back->lock);
67915cd5ae2cc11 Nuno Sa 2023-11-21  288  	back->ops = ops;
67915cd5ae2cc11 Nuno Sa 2023-11-21  289  	back->owner = dev->driver->owner;
67915cd5ae2cc11 Nuno Sa 2023-11-21  290  	back->dev = dev;
67915cd5ae2cc11 Nuno Sa 2023-11-21  291  	back->priv = priv;
67915cd5ae2cc11 Nuno Sa 2023-11-21  292  	mutex_lock(&iio_back_lock);
67915cd5ae2cc11 Nuno Sa 2023-11-21  293  	list_add(&back->entry, &iio_back_list);
67915cd5ae2cc11 Nuno Sa 2023-11-21  294  	mutex_unlock(&iio_back_lock);
67915cd5ae2cc11 Nuno Sa 2023-11-21  295  
67915cd5ae2cc11 Nuno Sa 2023-11-21  296  	return devm_add_action_or_reset(dev, iio_backend_unregister, back);
67915cd5ae2cc11 Nuno Sa 2023-11-21  297  }
67915cd5ae2cc11 Nuno Sa 2023-11-21  298  EXPORT_SYMBOL_GPL(devm_iio_backend_register);
67915cd5ae2cc11 Nuno Sa 2023-11-21  299  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 12/12] iio: adc: adi-axi-adc: move to backend framework

[kernel test robot]

Hi Nuno,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus robh/for-next linus/master v6.7-rc2 next-20231124]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nuno-Sa-via-B4-Relay/driver-core-allow-modifying-device_links-flags/20231121-182010
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20231121-dev-iio-backend-v1-12-6a3d542eba35%40analog.com
patch subject: [PATCH 12/12] iio: adc: adi-axi-adc: move to backend framework
config: arm-randconfig-r081-20231123 (https://download.01.org/0day-ci/archive/20231125/202311251430.QfOfg5Ws-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231125/202311251430.QfOfg5Ws-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311251430.QfOfg5Ws-lkp@intel.com/

smatch warnings:
drivers/iio/adc/adi-axi-adc.c:64 axi_adc_enable() warn: inconsistent indenting

vim +64 drivers/iio/adc/adi-axi-adc.c

    58	
    59	static int axi_adc_enable(struct iio_backend *back)
    60	{
    61		struct adi_axi_adc_state *st = iio_backend_get_priv(back);
    62		int ret;
    63	
  > 64		 ret = regmap_set_bits(st->regmap, ADI_AXI_REG_RSTN,
    65				       ADI_AXI_REG_RSTN_MMCM_RSTN);
    66		if (ret)
    67			return ret;
    68	
    69		fsleep(10);
    70		return regmap_set_bits(st->regmap, ADI_AXI_REG_RSTN,
    71				       ADI_AXI_REG_RSTN_RSTN | ADI_AXI_REG_RSTN_MMCM_RSTN);
    72	}
    73	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 12/12] iio: adc: adi-axi-adc: move to backend framework

[David Lechner]

On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
<devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> From: Nuno Sa <nuno.sa@analog.com>
>
> Move to the IIO backend framework. Devices supported by adi-axi-adc now
> register themselves as backend devices.
>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/adc/Kconfig       |   1 +
>  drivers/iio/adc/adi-axi-adc.c | 364 ++++++++----------------------------------
>  2 files changed, 65 insertions(+), 300 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index af56df63beff..cc42a3399c63 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -292,6 +292,7 @@ config ADI_AXI_ADC
>         select IIO_BUFFER
>         select IIO_BUFFER_HW_CONSUMER
>         select IIO_BUFFER_DMAENGINE
> +       select IIO_BACKEND
>         depends on HAS_IOMEM
>         depends on OF
>         help
> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> index c247ff1541d2..b2ab2c119efa 100644
> --- a/drivers/iio/adc/adi-axi-adc.c
> +++ b/drivers/iio/adc/adi-axi-adc.c

<snip>

> @@ -390,37 +166,23 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
>         if (ret)
>                 return ret;
>
> -       if (cl->info->version > ver) {
> +       if (*expected_ver > ver) {
>                 dev_err(&pdev->dev,
>                         "IP core version is too old. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n",
> -                       ADI_AXI_PCORE_VER_MAJOR(cl->info->version),
> -                       ADI_AXI_PCORE_VER_MINOR(cl->info->version),
> -                       ADI_AXI_PCORE_VER_PATCH(cl->info->version),
> +                       ADI_AXI_PCORE_VER_MAJOR(*expected_ver),
> +                       ADI_AXI_PCORE_VER_MINOR(*expected_ver),
> +                       ADI_AXI_PCORE_VER_PATCH(*expected_ver),
>                         ADI_AXI_PCORE_VER_MAJOR(ver),
>                         ADI_AXI_PCORE_VER_MINOR(ver),
>                         ADI_AXI_PCORE_VER_PATCH(ver));
>                 return -ENODEV;
>         }
>
> -       indio_dev->info = &adi_axi_adc_info;
> -       indio_dev->name = "adi-axi-adc";
> -       indio_dev->modes = INDIO_DIRECT_MODE;
> -       indio_dev->num_channels = conv->chip_info->num_channels;
> -       indio_dev->channels = conv->chip_info->channels;
> -
> -       ret = adi_axi_adc_config_dma_buffer(&pdev->dev, indio_dev);
> +       ret = devm_iio_backend_register(&pdev->dev, &adi_axi_adc_generic, st);
>         if (ret)
>                 return ret;
>
> -       ret = adi_axi_adc_setup_channels(&pdev->dev, st);
> -       if (ret)
> -               return ret;
> -
> -       ret = devm_iio_device_register(&pdev->dev, indio_dev);
> -       if (ret)
> -               return ret;
> -
> -       dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%c) probed\n",
> +       dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%d) probed\n",

Was this format change intentional? There are other places above where
%c is still used.

>                  ADI_AXI_PCORE_VER_MAJOR(ver),
>                  ADI_AXI_PCORE_VER_MINOR(ver),
>                  ADI_AXI_PCORE_VER_PATCH(ver));
> @@ -428,6 +190,8 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
>         return 0;
>  }
>
> +static unsigned int adi_axi_adc_10_0_a_info = ADI_AXI_PCORE_VER(10, 0, 'a');
> +
>  /* Match table for of_platform binding */
>  static const struct of_device_id adi_axi_adc_of_match[] = {
>         { .compatible = "adi,axi-adc-10.0.a", .data = &adi_axi_adc_10_0_a_info },
>
> --
> 2.42.1
>
>

Re: [PATCH 12/12] iio: adc: adi-axi-adc: move to backend framework

[Nuno Sá]

On Thu, 2023-11-30 at 17:33 -0600, David Lechner wrote:
> On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > Move to the IIO backend framework. Devices supported by adi-axi-adc now
> > register themselves as backend devices.
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/iio/adc/Kconfig       |   1 +
> >  drivers/iio/adc/adi-axi-adc.c | 364 ++++++++----------------------------------
> >  2 files changed, 65 insertions(+), 300 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index af56df63beff..cc42a3399c63 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -292,6 +292,7 @@ config ADI_AXI_ADC
> >         select IIO_BUFFER
> >         select IIO_BUFFER_HW_CONSUMER
> >         select IIO_BUFFER_DMAENGINE
> > +       select IIO_BACKEND
> >         depends on HAS_IOMEM
> >         depends on OF
> >         help
> > diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> > index c247ff1541d2..b2ab2c119efa 100644
> > --- a/drivers/iio/adc/adi-axi-adc.c
> > +++ b/drivers/iio/adc/adi-axi-adc.c
> 
> <snip>
> 
> > @@ -390,37 +166,23 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
> >         if (ret)
> >                 return ret;
> > 
> > -       if (cl->info->version > ver) {
> > +       if (*expected_ver > ver) {
> >                 dev_err(&pdev->dev,
> >                         "IP core version is too old. Expected %d.%.2d.%c,
> > Reported %d.%.2d.%c\n",
> > -                       ADI_AXI_PCORE_VER_MAJOR(cl->info->version),
> > -                       ADI_AXI_PCORE_VER_MINOR(cl->info->version),
> > -                       ADI_AXI_PCORE_VER_PATCH(cl->info->version),
> > +                       ADI_AXI_PCORE_VER_MAJOR(*expected_ver),
> > +                       ADI_AXI_PCORE_VER_MINOR(*expected_ver),
> > +                       ADI_AXI_PCORE_VER_PATCH(*expected_ver),
> >                         ADI_AXI_PCORE_VER_MAJOR(ver),
> >                         ADI_AXI_PCORE_VER_MINOR(ver),
> >                         ADI_AXI_PCORE_VER_PATCH(ver));
> >                 return -ENODEV;
> >         }
> > 
> > -       indio_dev->info = &adi_axi_adc_info;
> > -       indio_dev->name = "adi-axi-adc";
> > -       indio_dev->modes = INDIO_DIRECT_MODE;
> > -       indio_dev->num_channels = conv->chip_info->num_channels;
> > -       indio_dev->channels = conv->chip_info->channels;
> > -
> > -       ret = adi_axi_adc_config_dma_buffer(&pdev->dev, indio_dev);
> > +       ret = devm_iio_backend_register(&pdev->dev, &adi_axi_adc_generic, st);
> >         if (ret)
> >                 return ret;
> > 
> > -       ret = adi_axi_adc_setup_channels(&pdev->dev, st);
> > -       if (ret)
> > -               return ret;
> > -
> > -       ret = devm_iio_device_register(&pdev->dev, indio_dev);
> > -       if (ret)
> > -               return ret;
> > -
> > -       dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%c) probed\n",
> > +       dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%d) probed\n",
> 
> Was this format change intentional? There are other places above where
> %c is still used.
> 

Yes, the output was weird with %c. I guess something changed... Hmm need to look at
the other places.

- Nuno Sá
> 

Re: [PATCH 00/12] iio: add new backend framework

[Olivier MOYSAN]

Hi Nuno,

On 11/21/23 11:20, Nuno Sa via B4 Relay wrote:
> Hi all,
> 
> 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()
> 
> (typical provider - consumer stuff)
> 
> This is the result of the discussions in [1] and [2]. In short, both ADI
> and STM wanted some way to control/get configurations from a kind of
> IIO aggregate device. So discussions were made to have something that
> serves and can be used by everyone.
> 
> The main differences with the converter framework RFC [1]:
> 
> 1) Dropped the component framework. One can get more overview about
> the concerns on the references but the main reasons were:
>   * Relying on providing .remove() callbacks to be allowed to use device
>     managed functions. I was not even totally sure about the correctness
>     of it and in times where everyone tries to avoid that driver
>     callback, it could lead to some maintenance burden.
>   * Scalability issues. As mentioned in [2], to support backends defined
>     in FW child nodes was not so straightforward with the component
>     framework.
>   * Device links can already do some of the things that made me
>     try the component framework (eg: removing consumers on suppliers
>     unbind).
> 
> 2) Only support the minimal set of functionality to have the devices in
>     the same state as before using the backend framework. New features
>     will be added afterwards.
> 
> 3) Moved the API docs into the .c files.
> 
> 4) Moved the framework to the IIO top dir and renamed it to
>     industrialio-backend.c.
> 
> Also, as compared with the RFC in [2], I don't think there are that many
> similarities other than the filename. However, it should now be pretty
> straight for Olivier to build on top of it. Also to mention that I did
> grabbed patch 1 ("of: property: add device link support for
> io-backends") from that series and just did some minor changes:
> 

I did not already look at the framework patches in detail, but at first 
glance it looks fine.

I confirm that it has been quite straightforward to build on top of this 
framework, as it remains close to my first approach. I had only some 
small changes to do, to match the API, and to get it alive. This is great.

I see just one concern regarding ADC generic channel bindings support.
Here we have no devices associated to the channel sub-nodes. So, I 
cannot figure out we could use the devm_iio_backend_get() API, when 
looking for backend handle in channels sub-nodes. I have to think about it.

> 1) Renamed the property from "io-backend" to "io-backends".
> 2) No '#io-backend-cells' as it's not supported/needed by the framework
> (at least for now) .
> 
> Regarding the driver core patch
> ("driver: core: allow modifying device_links flags"), it is more like a
> RFC one. I'm not really sure if the current behavior isn't just
> expected/wanted. Since I could not really understand if it is or not
> (or why the different handling DL_FLAG_AUTOREMOVE_CONSUMER vs
> DL_FLAG_AUTOREMOVE_SUPPLIER), I'm sending out the patch.
> 
> Jonathan,
> 
> I also have some fixes and cleanups for the ad9467 driver. I added
> Fixes tags but I'm not sure if it's really worth it to backport them (given
> what we already discussed about these drivers). I'll leave that to you
> :).
> 
> I'm also not sure if I'm missing some tags (even though the series
> is frankly different from [2]).
> 
> Olivier,
> 
> If you want to be included as a Reviewer let me know and I'll happily do
> so in the next version.
> 

Yes, please add me as reviewer.
Thanks.
Olivier

> Also regarding the new IIO fw schemas. Should I send patches/PR to:
> 
> https://github.com/devicetree-org/dt-schema/
> 
> ? Or is there any other workflow for it?
> 
> [1]: https://lore.kernel.org/linux-iio/20230727150324.1157933-1-olivier.moysan@foss.st.com/
> [2]: https://lore.kernel.org/linux-iio/20230727150324.1157933-1-olivier.moysan@foss.st.com/
> 
> ---
> Nuno Sa (11):
>        driver: core: allow modifying device_links flags
>        iio: add the IIO backend framework
>        iio: adc: ad9467: fix reset gpio handling
>        iio: adc: ad9467: don't ignore error codes
>        iio: adc: ad9467: add mutex to struct ad9467_state
>        iio: adc: ad9467: fix scale setting
>        iio: adc: ad9467: use spi_get_device_match_data()
>        iio: adc: ad9467: use chip_info variables instead of array
>        iio: adc: ad9467: convert to backend framework
>        iio: adc: adi-axi-adc: convert to regmap
>        iio: adc: adi-axi-adc: move to backend framework
> 
> Olivier Moysan (1):
>        of: property: add device link support for io-backends
> 
>   MAINTAINERS                         |   7 +
>   drivers/base/core.c                 |  14 +-
>   drivers/iio/Kconfig                 |   5 +
>   drivers/iio/Makefile                |   1 +
>   drivers/iio/adc/Kconfig             |   3 +-
>   drivers/iio/adc/ad9467.c            | 382 +++++++++++++++++++++-----------
>   drivers/iio/adc/adi-axi-adc.c       | 429 +++++++-----------------------------
>   drivers/iio/industrialio-backend.c  | 302 +++++++++++++++++++++++++
>   drivers/of/property.c               |   2 +
>   include/linux/iio/adc/adi-axi-adc.h |   4 +
>   include/linux/iio/backend.h         |  58 +++++
>   11 files changed, 723 insertions(+), 484 deletions(-)
> 
> Thanks!
> - Nuno Sá
> 

Re: [PATCH 00/12] iio: add new backend framework

[Nuno Sá]

On Thu, 2023-11-23 at 18:36 +0100, Olivier MOYSAN wrote:
> Hi Nuno,
> 
> On 11/21/23 11:20, Nuno Sa via B4 Relay wrote:
> > Hi all,
> > 
> > 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()
> > 
> > (typical provider - consumer stuff)
> > 
> > This is the result of the discussions in [1] and [2]. In short, both ADI
> > and STM wanted some way to control/get configurations from a kind of
> > IIO aggregate device. So discussions were made to have something that
> > serves and can be used by everyone.
> > 
> > The main differences with the converter framework RFC [1]:
> > 
> > 1) Dropped the component framework. One can get more overview about
> > the concerns on the references but the main reasons were:
> >   * Relying on providing .remove() callbacks to be allowed to use device
> >     managed functions. I was not even totally sure about the correctness
> >     of it and in times where everyone tries to avoid that driver
> >     callback, it could lead to some maintenance burden.
> >   * Scalability issues. As mentioned in [2], to support backends defined
> >     in FW child nodes was not so straightforward with the component
> >     framework.
> >   * Device links can already do some of the things that made me
> >     try the component framework (eg: removing consumers on suppliers
> >     unbind).
> > 
> > 2) Only support the minimal set of functionality to have the devices in
> >     the same state as before using the backend framework. New features
> >     will be added afterwards.
> > 
> > 3) Moved the API docs into the .c files.
> > 
> > 4) Moved the framework to the IIO top dir and renamed it to
> >     industrialio-backend.c.
> > 
> > Also, as compared with the RFC in [2], I don't think there are that many
> > similarities other than the filename. However, it should now be pretty
> > straight for Olivier to build on top of it. Also to mention that I did
> > grabbed patch 1 ("of: property: add device link support for
> > io-backends") from that series and just did some minor changes:
> > 
> 
> I did not already look at the framework patches in detail, but at first 
> glance it looks fine.
> 
> I confirm that it has been quite straightforward to build on top of this 
> framework, as it remains close to my first approach. I had only some 
> small changes to do, to match the API, and to get it alive. This is great.
> 
> I see just one concern regarding ADC generic channel bindings support.
> Here we have no devices associated to the channel sub-nodes. So, I 
> cannot figure out we could use the devm_iio_backend_get() API, when 
> looking for backend handle in channels sub-nodes. I have to think about it.
> 

Yeah, I'm keeping the series small (as Jonathan asked in the RFC) and just with basic
stuff needed to get the ad9647 driver in the exact same state as before the
framework. So yes, it's the same deal as with the component approach. You'll need to
add support for it. But, in this case, I believe it should be as straight as:

-/**
- * 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 iio_backend *devm_fwnode_iio_backend_get(struct device *dev,
+                                               const struct fwnode_handle *fwnode,
+                                               const char *name)
 {
-       struct fwnode_handle *fwnode;
+       struct fwnode_handle *back_fwnode;
        struct iio_backend *back;
        int index = 0, ret;

@@ -195,20 +187,20 @@ struct iio_backend *devm_iio_backend_get(struct device *dev,
const char *name)
                        return ERR_PTR(index);
        }

-       fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index);
-       if (IS_ERR(fwnode)) {
+       back_fwnode = fwnode_find_reference(fwnode, "io-backends", index);
+       if (IS_ERR(back_fwnode)) {
                dev_err(dev, "Cannot get Firmware reference\n");
-               return ERR_CAST(fwnode);
+               return ERR_CAST(back_fwnode);
        }

        guard(mutex)(&iio_back_lock);
        list_for_each_entry(back, &iio_back_list, entry) {
                struct device_link *link;

-               if (!device_match_fwnode(back->dev, fwnode))
+               if (!device_match_fwnode(back->dev, back_fwnode))
                        continue;

-               fwnode_handle_put(fwnode);
+               fwnode_handle_put(back_fwnode);
                kref_get(&back->ref);
                if (!try_module_get(back->owner)) {
                        dev_err(dev, "Cannot get module reference\n");
@@ -229,9 +221,25 @@ struct iio_backend *devm_iio_backend_get(struct device *dev,
const char *name)
                return back;
        }

-       fwnode_handle_put(fwnode);
+       fwnode_handle_put(back_fwnode);
        return ERR_PTR(-EPROBE_DEFER);
 }
+EXPORT_SYMBOL_GPL(devm_fwnode_iio_backend_get);
+
+/**
+ * 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)
+{
+       return devm_fwnode_iio_backend_get(dev, dev_fwnode(dev), name);
+}
 EXPORT_SYMBOL_GPL(devm_iio_backend_get);

 /**

Completely untested (not even compiled :)). Anyways, the goal is to just have the
minimum accepted and you can then send the needed patches for subnode lookups.


> > 1) Renamed the property from "io-backend" to "io-backends".
> > 2) No '#io-backend-cells' as it's not supported/needed by the framework
> > (at least for now) .
> > 
> > Regarding the driver core patch
> > ("driver: core: allow modifying device_links flags"), it is more like a
> > RFC one. I'm not really sure if the current behavior isn't just
> > expected/wanted. Since I could not really understand if it is or not
> > (or why the different handling DL_FLAG_AUTOREMOVE_CONSUMER vs
> > DL_FLAG_AUTOREMOVE_SUPPLIER), I'm sending out the patch.
> > 
> > Jonathan,
> > 
> > I also have some fixes and cleanups for the ad9467 driver. I added
> > Fixes tags but I'm not sure if it's really worth it to backport them (given
> > what we already discussed about these drivers). I'll leave that to you
> > :).
> > 
> > I'm also not sure if I'm missing some tags (even though the series
> > is frankly different from [2]).
> > 
> > Olivier,
> > 
> > If you want to be included as a Reviewer let me know and I'll happily do
> > so in the next version.
> > 
> 
> Yes, please add me as reviewer.
> Thanks.
> Olivier

Will do.

- Nuno Sá
> 

Re: [PATCH 00/12] iio: add new backend framework

[David Lechner]

On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
<devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> Hi all,
>
> 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()
>
> (typical provider - consumer stuff)
>

The "typical provider - consumer stuff" seems pretty straight forward
for finding and connecting two different devices, but the definition
of what is a frontend and what is a backend seems a bit nebulous. It
would be nice to seem some example devicetree to be able to get a
better picture of how this will be used in practices (links to the the
hardware docs for those examples would be nice too).

In addition to the backend ops given in this series, what are some
other expected ops that could be added in the future? Do we need some
kind of spec to say "I need a backend with feature X and feature Y" or
"I need a backend with compatible string" rather than just "I need a
generic backend"?

Re: [PATCH 00/12] iio: add new backend framework

[Nuno Sá]

On Thu, 2023-11-30 at 17:54 -0600, David Lechner wrote:
> On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > 
> > Hi all,
> > 
> > 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()
> > 
> > (typical provider - consumer stuff)
> > 
> 
> The "typical provider - consumer stuff" seems pretty straight forward
> for finding and connecting two different devices, but the definition
> of what is a frontend and what is a backend seems a bit nebulous. It
> would be nice to seem some example devicetree to be able to get a
> better picture of how this will be used in practices (links to the the
> hardware docs for those examples would be nice too).
> 
> In addition to the backend ops given in this series, what are some
> other expected ops that could be added in the future? Do we need some
> kind of spec to say "I need a backend with feature X and feature Y" or
> "I need a backend with compatible string" rather than just "I need a
> generic backend"?

Bah, I know saw that I messed up in the cover letter. This was the link [1] I wanted
to add. In there, you'll see the RFC patch which already has lots of info. It also
has a link to a previous discussion  had with Jonathan about this kind off
arrangement.

You already more or less figured the frontend + backend story in one of your replies.
So, yes the data is received or transmitted (if we think DAC/DDS) by the actual
converter which makes sense to be seen as the frontend. This connects to another
device capable of handling the high sample rate (typically a core in a FPGA) of these
converters. But this is ADI usecase, note that STM also wants to have something like
this in order to link devices. Ah, and one of the things that Jonathan wants to see
is that only the frontend device exposes the IIO interface to userspace.




[1]: https://lore.kernel.org/linux-iio/20230804145342.1600136-1-nuno.sa@analog.com/

- Nuno Sá

Re: [PATCH 00/12] iio: add new backend framework

[Nuno Sá]

On Fri, 2023-12-01 at 09:41 +0100, Nuno Sá wrote:
> On Thu, 2023-11-30 at 17:54 -0600, David Lechner wrote:
> > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > 
> > > Hi all,
> > > 
> > > 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()
> > > 
> > > (typical provider - consumer stuff)
> > > 
> > 
> > The "typical provider - consumer stuff" seems pretty straight forward
> > for finding and connecting two different devices, but the definition
> > of what is a frontend and what is a backend seems a bit nebulous. It
> > would be nice to seem some example devicetree to be able to get a
> > better picture of how this will be used in practices (links to the the
> > hardware docs for those examples would be nice too).
> > 
> > In addition to the backend ops given in this series, what are some
> > other expected ops that could be added in the future? Do we need some
> > kind of spec to say "I need a backend with feature X and feature Y" or
> > "I need a backend with compatible string" rather than just "I need a
> > generic backend"?

To also reply to this one, I also have somewhere a comment about it (I think in the
code itself). For now, I'm thinking in this to serve all kinds of IIO devices in a
generic way which means, yes, the ops structure can grow badly. I'm not so sure if
that will actually happen in practise but if it does we can always play some OOP
games and leave the backend with the generic stuff (like .enable()/.disable() and so
on) and extend it (for example: having a converters thing that would have ops more
suitable for ADCs/DACs). Anyways, for now I think that would be overcomplicating
things. It's in kernel interfaces so we can always change things.


- Nuno Sá

Re: [PATCH 00/12] iio: add new backend framework

[David Lechner]

On Thu, Nov 30, 2023 at 5:54 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> <devnull+nuno.sa.analog.com@kernel.org> wrote:
> >
> > Hi all,
> >
> > 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()
> >
> > (typical provider - consumer stuff)
> >
>
> The "typical provider - consumer stuff" seems pretty straight forward
> for finding and connecting two different devices, but the definition
> of what is a frontend and what is a backend seems a bit nebulous. It
> would be nice to seem some example devicetree to be able to get a
> better picture of how this will be used in practices (links to the the
> hardware docs for those examples would be nice too).
>

Fulfilling my own request here...

Since AD9467 is being use as the example first user of the IIO offload framework
I did a deep dive into how it is actually being used. It looks like this:

----------------------------------------------------------------------------
Hardware
----------------------------------------------------------------------------

AD9467
------

High-speed ADC that uses SPI for configuration and LVDS for data capture.

https://www.analog.com/media/en/technical-documentation/data-sheets/AD9467.pdf

Pins:

    Supplies:
    - AVDD1 (1.8V analog supply)
    - AVDD2 (3.3V analog supply)
    - AVDD3 (1.8V analog supply)
    - SPIVDD (1.8V or 3.3V SPI supply)
    - DRVDD (1.8V digital output driver supply)
    - XVREF (optional reference voltage)

    SPI:
    - CSB
    - SDIO
    - SCLK

    Analog input:
    - VIN+/VIN-

    Clock input:
    - CLK+/CLK-

    Digital output:
    - Dn-/Dn+ (parallel digital output)
    - DCO+/DCO- (data clock output)
    - OR+/OR- (out of range output)


ADI AXI ADC
-----------

FPGA IP core for interfacing an ADC device with a high speed serial (JESD204B)
or source synchronous parallel interface (LVDS/CMOS).

https://wiki.analog.com/resources/fpga/docs/axi_adc_ip

Interfaces:

    LVDS:
    - rx_clk_in_[p|n]   clock input
    - rx_data_in_[p|n]  parallel data input
    - rx_frame_in_[p|n] frame input signal (optional/device specific)
    - rx_or_in_[p|n]    over range input (optional/device specific)

    Write FIFO:
    - see link for details, this consists of multiple signals that connect to a
      "sink module"

    MMIO:
    - memory mapped registers (detailed in link)


"sink module"
-------------

FPGA IP core for piping a data stream to DMA.

https://wiki.analog.com/resources/fpga/docs/util_var_fifo

Interfaces:

    Data Input:
    - data_in           Data to be stored
    - data_in_valid     Valid for the input data

    Data Output:
    - data_out          Data forwarded to the DMA
    - data_out_valid    Valid for the output data

    There are additional interfaces but they probably don't concern devicetree
    since software doesn't interact with them (AFAIK).


Schematic
---------

+----------------------------+    +---------------+    +-------------------+
| SOC/FPGA                   |    | ADC           |    | Signal generator  |
|                            |    |               |    |                   |
|  +--------------------+    |    |       VIN+/- xxxxxxxx VOUT+/-          |
|  | SPI                |    |    |               |    |                   |
|  |               SDIO ------------ SDIO         |    +-------------------+
|  |               SCLK ------------ SCLK         |
|  |                 CS ------------ CSB          |    +-------------------+
|  |                    |    |    |               |    | External clock    |
|  +--------------------+    |    |               |    |                   |
|  | AXI ADC            |    |    |      CLK+/- xxxxxxxxx CLKOUT+/-        |
|  |                    |    |    |               |    |                   |
|  |  rx_data_in_[p|n] xxxxxxxxxxxxx Dn+/-        |    +-------------------+
|  |   rx_clk_in_[p|n] xxxxxxxxxxxxx DCO+/DCO-    |
|  |    rx_or_in_[p|n] xxxxxxxxxxxxx OR+/-        |    +-------------------+
|  |                    |    |    |               |    | Power supplies    |
|  |        Write FIFO ===#  |    |               |    |                   |
|  +--------------------+ H  |    |        AVDD1 --------- 1.8V            |
|  | "sink module"      | H  |    |        AVDD2 --------- 2.2V            |
|  |                    | H  |    |        AVDD3 --------- 1.8V            |
|  |        Data Input ===#  |    |       SPIVDD --------- 3.3V            |
|  |                    |    |    |        DRVDD --------- 1.8V            |
|  |       Data Output ===#  |    |        XVREF --------- 1.2V            |
|  +--------------------+ H  |    |               |    |                   |
|  | DMA controller     | H  |    +---------------+    +-------------------+
|  |                    | H  |
|  |         channel 0 ===#  |
|  |                    |    |
|  +--------------------+    |
|                            |
+----------------------------+

----------------------------------------------------------------------------
Devicetree
----------------------------------------------------------------------------

Current situation:

&soc: {
    spi {
        ...

        spi_adc: adc@0 {
            compatible = "adi,ad9467";
            reg = <0>;
            clocks = <&adc_clk>;
            clock-names = "adc-clk";
        };
    };

    fpga {
        ...

        /* IIO device is associated with this node. */
        axi-adc@44a00000 {
            compatible = "adi,axi-adc-10.0.a";
            reg = <0x44a00000 0x10000>;
            /* Not sure dmas belong here since it is a property of the
             * "sink module" which is separate from AXI ADC module. */
            dmas = <&rx_dma 0>;
            dma-names = "rx";

            adi,adc-dev = <&spi_adc>;
        };
    };
};

---

Proposed IIO backend framework (inferred from v1 patches):

&soc: {
    spi {
        ...

        /* IIO device is associated with this node. */
        adc@0 {
            compatible = "adi,ad9467";
            reg = <0>;
            clocks = <&adc_clk>;
            clock-names = "adc-clk";
            /* As discussed already, this isn't really the right place for
             * dmas either. */
            dmas = <&rx_dma 0>;
            dma-names = "rx";

            io-backends = <&axi_adc>;
        };
    };

    fpga {
        ...

        axi_adc: axi-adc@44a00000 {
            compatible = "adi,axi-adc-10.0.a";
            reg = <0x44a00000 0x10000>;
        };
    };
};

---

Composite device?

/* IIO device is associated with this node. */
adc {
    compatible = "adi,ad9467";

    io-backends = <&adc_spi_backend>, <&adc_lvds_backend>;

    clocks = <&adc_clk>;
    clock-names = "adc-clk";

    xvref-supply = <&ref_voltage>;
    avdd1-supply = <&regulator_1_8V>;
    avdd2-supply = <&regulator_3_3V>;
    avdd3-supply = <&regulator_1_8V>;
};

&soc: {
    spi {
        ...

        /* This node describes only the SPI aspects of the ADC chip */
        adc_spi_backend: adc@0 {
            compatible = "adi,ad9467-spi-io-backend";
            reg = <0>;

            spi-3wire;
            spi-max-frequency = <25000000>;

            spivdd-supply = <&regulator_1_8V>;
        };
    };

    fpga {
        ...

        /* This node is an LVDS bus, analogous to a SPI bus or I2C bus */
        axi-adc@44a00000 {
            compatible = "adi,axi-adc-10.0.a";
            reg = <0x44a00000 0x10000>;

            ...

            /* This node describes all sink modules that are connected to
             * the AXI ADC controller through the FPGA fabric. */
            sink-modules {
                ...

                /* This node describes the FIFO to DMA sink module used by
                 * our ADC. */
                adc_dma: module@0 {
                    compatible = "adi,dma-fifo-1.0.a";
                    reg = <0>;

                    dmas = <&rx_dma 0>;
                    dma-names = "rx";
                };
            };

            /* Then we describe what is connected to each channel of the
             * controller (reg == channel number). */

            /* This node describes only the digital output (LVDS) aspects of
            * the ADC chip */
            adc_lvds_backend: adc@0 {
                compatible = "adi,ad9467-lvds-io-backend";
                reg = <0>;

                drvdd-supply = <&regulator_1_8V>;

                /* This is a LVDS bus peripheral property that can only be used
                 * with peripheral nodes that are children of a compatible =
                 * "adi,axi-adc-10.0.a" node. This works exactly like the
                 * controller-specific SPI bus peripheral properties, e.g.
                 * like samsung,spi-peripheral-props.yaml. */
                adi,sink-modules = <&adc_dma>;
            }
        };
    };
};


----------------------------------------------------------------------------
Discussion
----------------------------------------------------------------------------

After reviewing the existing device tree bindings, it appears the current
adi,ad9467.yaml is incomplete. It lacks the supplies and even though the
example shows that it is a child of a spi node, it is missing a reference to
/schemas/spi/spi-peripheral-props.yaml# and spi properties like spi-3wire
and spi-max-frequency. It also misses a description of what is connected to
the digital output, but that I think that is the main thing we are trying to
solve here - if it belongs there or somewhere else.

Having read more about the AXI ADC IP block, it seems to me like it should just
be considered an LVDS bus controller with generic bindings similar to how we
have SPI and I2C buses.

Following that line of thought, if the compatible = "adi,axi-adc-10.0.a" node
is a bus node, then logically, the ADC device node should be a child node of
that LVDS bus node. But since the ADC is also a SPI device it also needs to
be a child node of the SPI bus node. But is can't be a child of two different
nodes, so where does it go?

This is where the IIO backend framework can come in. We can create a "composite"
device as seen in the example dts above. This is just a platform device (in
Linux-speak) and it composes the two "io-backend" devices from the the two
busses to create a logical iio/adc device.

To solve the mystery of "where does the dmas property belong?", I have taken
a page out of the work I am preparing the AXI SPI Engine offload support [1].
(This hasn't been submitted to a mailing list yet because I'm still working
on the driver, but the bindings are finished and I ran the idea by Rob on IRC
a while back who suggested doing it this way, so maybe it works here too?)

[1]: https://github.com/analogdevicesinc/linux/pull/2341/commits/57bb1998371f61f4144689136aba5dd6d16a2a66

Since the "sink module" is really a separate IP block from the AXI ADC, it gets
its own node and compatible string. Since these "sink modules" are connected to
the AXI ADC, they get listed as child nodes, but we group them together under a
single sink-modules node to separate them from the LVDS peripherals nodes. Then
they get associated with a peripheral with the adi,sink-modules property (also
see comment on that property in the example above).

My "composite" device example evolved quite a bit as I was writing this but I'm
pretty happy with where it ended up. I think adding child nodes to the axi-adc
node answers Nuno's concerns about how to keep a generic axi-adc binding while
accounting for the fact that it changes slightly depending on what it is
attached to. And having a separate platform device solves my questions about
the ambiguity of which should be the front end, the spi node or the axi-adc
node. It turns out, perhaps the answer is neither.

Re: [PATCH 00/12] iio: add new backend framework

[Nuno Sá]

On Fri, 2023-12-01 at 21:53 -0600, David Lechner wrote:
> On Thu, Nov 30, 2023 at 5:54 PM David Lechner <dlechner@baylibre.com> wrote:
> > 
> > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > 
> > > Hi all,
> > > 
> > > 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()
> > > 
> > > (typical provider - consumer stuff)
> > > 
> > 
> > The "typical provider - consumer stuff" seems pretty straight forward
> > for finding and connecting two different devices, but the definition
> > of what is a frontend and what is a backend seems a bit nebulous. It
> > would be nice to seem some example devicetree to be able to get a
> > better picture of how this will be used in practices (links to the the
> > hardware docs for those examples would be nice too).
> > 
> 
> Fulfilling my own request here...
> 
> Since AD9467 is being use as the example first user of the IIO offload framework
> I did a deep dive into how it is actually being used. It looks like this:
> 

This is not an offload framework... I think somehow you're connecting this to the
spi_engine offload and these are two completely different things. Maybe they can
intersect at some point but as of now, I don't see any benefit in doing it. The goal
of this patchseries is to have a simple and generic framework so we can connect IIO
devices (frontends) to a backend device having kind of an IIO aggregate device so to
say. Moreover, we just want to have the ad9467 driver in the same state it was before
to keep things simple. I'm already fixing some things but I don't want extend that
too much as the primary goal is to have the framework in. Cleanups can come
afterwards.

That said, is fine to have this kind of discussion but I honestly think you're over
engineering the whole thing. Maybe you're already too ahead of me :), but where we
stand right now, I don't see any reason for anything so complicated as the below.
Also note this should be simple and generic. As I already said, this is not supposed
to be an ADI only thing and STM also wants to make use of this infrastructure. But
see below some of my comments on why I think it's too much...
 
> ----------------------------------------------------------------------------
> Hardware
> ----------------------------------------------------------------------------
> 
> AD9467
> ------
> 
> High-speed ADC that uses SPI for configuration and LVDS for data capture.
> 
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD9467.pdf
> 
> Pins:
> 
>     Supplies:
>     - AVDD1 (1.8V analog supply)
>     - AVDD2 (3.3V analog supply)
>     - AVDD3 (1.8V analog supply)
>     - SPIVDD (1.8V or 3.3V SPI supply)
>     - DRVDD (1.8V digital output driver supply)
>     - XVREF (optional reference voltage)
> 
>     SPI:
>     - CSB
>     - SDIO
>     - SCLK
> 
>     Analog input:
>     - VIN+/VIN-
> 
>     Clock input:
>     - CLK+/CLK-
> 
>     Digital output:
>     - Dn-/Dn+ (parallel digital output)
>     - DCO+/DCO- (data clock output)
>     - OR+/OR- (out of range output)
> 
> 
> ADI AXI ADC
> -----------
> 
> FPGA IP core for interfacing an ADC device with a high speed serial (JESD204B)
> or source synchronous parallel interface (LVDS/CMOS).
> 
> https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
> 
> Interfaces:
> 
>     LVDS:
>     - rx_clk_in_[p|n]   clock input
>     - rx_data_in_[p|n]  parallel data input
>     - rx_frame_in_[p|n] frame input signal (optional/device specific)
>     - rx_or_in_[p|n]    over range input (optional/device specific)
> 
>     Write FIFO:
>     - see link for details, this consists of multiple signals that connect to a
>       "sink module"
> 
>     MMIO:
>     - memory mapped registers (detailed in link)
> 
> 
> "sink module"
> -------------
> 
> FPGA IP core for piping a data stream to DMA.
> 
> https://wiki.analog.com/resources/fpga/docs/util_var_fifo
> 
> Interfaces:
> 
>     Data Input:
>     - data_in           Data to be stored
>     - data_in_valid     Valid for the input data
> 
>     Data Output:
>     - data_out          Data forwarded to the DMA
>     - data_out_valid    Valid for the output data
> 
>     There are additional interfaces but they probably don't concern devicetree
>     since software doesn't interact with them (AFAIK).
> 
> 
> Schematic
> ---------
> 
> +----------------------------+    +---------------+    +-------------------+
> > SOC/FPGA                   |    | ADC           |    | Signal generator  |
> >                            |    |               |    |                   |
> >  +--------------------+    |    |       VIN+/- xxxxxxxx VOUT+/-          |
> >  | SPI                |    |    |               |    |                   |
> >  |               SDIO ------------ SDIO         |    +-------------------+
> >  |               SCLK ------------ SCLK         |
> >  |                 CS ------------ CSB          |    +-------------------+
> >  |                    |    |    |               |    | External clock    |
> >  +--------------------+    |    |               |    |                   |
> >  | AXI ADC            |    |    |      CLK+/- xxxxxxxxx CLKOUT+/-        |
> >  |                    |    |    |               |    |                   |
> >  |  rx_data_in_[p|n] xxxxxxxxxxxxx Dn+/-        |    +-------------------+
> >  |   rx_clk_in_[p|n] xxxxxxxxxxxxx DCO+/DCO-    |
> >  |    rx_or_in_[p|n] xxxxxxxxxxxxx OR+/-        |    +-------------------+
> >  |                    |    |    |               |    | Power supplies    |
> >  |        Write FIFO ===#  |    |               |    |                   |
> >  +--------------------+ H  |    |        AVDD1 --------- 1.8V            |
> >  | "sink module"      | H  |    |        AVDD2 --------- 2.2V            |
> >  |                    | H  |    |        AVDD3 --------- 1.8V            |
> >  |        Data Input ===#  |    |       SPIVDD --------- 3.3V            |
> >  |                    |    |    |        DRVDD --------- 1.8V            |
> >  |       Data Output ===#  |    |        XVREF --------- 1.2V            |
> >  +--------------------+ H  |    |               |    |                   |
> >  | DMA controller     | H  |    +---------------+    +-------------------+
> >  |                    | H  |
> >  |         channel 0 ===#  |
> >  |                    |    |
> >  +--------------------+    |
> >                            |
> +----------------------------+
> 
> ----------------------------------------------------------------------------
> Devicetree
> ----------------------------------------------------------------------------
> 
> Current situation:
> 
> &soc: {
>     spi {
>         ...
> 
>         spi_adc: adc@0 {
>             compatible = "adi,ad9467";
>             reg = <0>;
>             clocks = <&adc_clk>;
>             clock-names = "adc-clk";
>         };
>     };
> 
>     fpga {
>         ...
> 
>         /* IIO device is associated with this node. */
>         axi-adc@44a00000 {
>             compatible = "adi,axi-adc-10.0.a";
>             reg = <0x44a00000 0x10000>;
>             /* Not sure dmas belong here since it is a property of the
>              * "sink module" which is separate from AXI ADC module. */
>             dmas = <&rx_dma 0>;
>             dma-names = "rx";
> 
>             adi,adc-dev = <&spi_adc>;
>         };
>     };
> };
> 
> ---
> 
> Proposed IIO backend framework (inferred from v1 patches):
> 
> &soc: {
>     spi {
>         ...
> 
>         /* IIO device is associated with this node. */
>         adc@0 {
>             compatible = "adi,ad9467";
>             reg = <0>;
>             clocks = <&adc_clk>;
>             clock-names = "adc-clk";
>             /* As discussed already, this isn't really the right place for
>              * dmas either. */
>             dmas = <&rx_dma 0>;
>             dma-names = "rx";

Not ideally but if they stay here is also not a big deal/problem...
> 
>             io-backends = <&axi_adc>;
>         };
>     };
> 
>     fpga {
>         ...
> 
>         axi_adc: axi-adc@44a00000 {
>             compatible = "adi,axi-adc-10.0.a";
>             reg = <0x44a00000 0x10000>;
>         };
>     };
> };
> 
> ---
> 
> Composite device?
> 
> /* IIO device is associated with this node. */
> adc {
>     compatible = "adi,ad9467";
> 
>     io-backends = <&adc_spi_backend>, <&adc_lvds_backend>;
> 
>     clocks = <&adc_clk>;
>     clock-names = "adc-clk";
> 
>     xvref-supply = <&ref_voltage>;
>     avdd1-supply = <&regulator_1_8V>;
>     avdd2-supply = <&regulator_3_3V>;
>     avdd3-supply = <&regulator_1_8V>;
> };
> 
> &soc: {
>     spi {
>         ...
> 
>         /* This node describes only the SPI aspects of the ADC chip */
>         adc_spi_backend: adc@0 {
>             compatible = "adi,ad9467-spi-io-backend";
>             reg = <0>;
> 
>             spi-3wire;
>             spi-max-frequency = <25000000>;
> 
>             spivdd-supply = <&regulator_1_8V>;
>         };
>     };
> 
>     fpga {
>         ...
> 
>         /* This node is an LVDS bus, analogous to a SPI bus or I2C bus */
>         axi-adc@44a00000 {
>             compatible = "adi,axi-adc-10.0.a";
>             reg = <0x44a00000 0x10000>;
> 
>             ...
> 
>             /* This node describes all sink modules that are connected to
>              * the AXI ADC controller through the FPGA fabric. */
>             sink-modules {
>                 ...
> 
>                 /* This node describes the FIFO to DMA sink module used by
>                  * our ADC. */
>                 adc_dma: module@0 {
>                     compatible = "adi,dma-fifo-1.0.a";

Why a different compatible?
> 

>                     reg = <0>;
> 
>                     dmas = <&rx_dma 0>;
>                     dma-names = "rx";
>                 };
>             };
> 
>             /* Then we describe what is connected to each channel of the
>              * controller (reg == channel number). */

I don't think we need to describe that in here. We might get to a point where we
might need some clever properties in the backend device but also note that the
frontend device should also know what to do with the device. Most of the knowledge
should be there (of course we should not have properties in the frontend that don't
belong to that device).

> 
>             /* This node describes only the digital output (LVDS) aspects of
>             * the ADC chip */
>             adc_lvds_backend: adc@0 {
>                 compatible = "adi,ad9467-lvds-io-backend";

This is just adding complexity... why different compatibles? 

>                 reg = <0>;
> 
>                 drvdd-supply = <&regulator_1_8V>;
> 
>                 /* This is a LVDS bus peripheral property that can only be used
>                  * with peripheral nodes that are children of a compatible =
>                  * "adi,axi-adc-10.0.a" node. This works exactly like the
>                  * controller-specific SPI bus peripheral properties, e.g.
>                  * like samsung,spi-peripheral-props.yaml. */
>                 adi,sink-modules = <&adc_dma>;
>             }
>         };
>     };
> };
> 
> 
> ----------------------------------------------------------------------------
> Discussion
> ----------------------------------------------------------------------------
> 
> After reviewing the existing device tree bindings, it appears the current
> adi,ad9467.yaml is incomplete. It lacks the supplies and even though the
> example shows that it is a child of a spi node, it is missing a reference to
> /schemas/spi/spi-peripheral-props.yaml# and spi properties like spi-3wire
> and spi-max-frequency. It also misses a description of what is connected to
> the digital output, but that I think that is the main thing we are trying to
> solve here - if it belongs there or somewhere else.

As stated, bindings can be improved/cleaned afterwards...

> 
> Having read more about the AXI ADC IP block, it seems to me like it should just
> be considered an LVDS bus controller with generic bindings similar to how we
> have SPI and I2C buses.

No... That core can handle LVDS interfaces, yes. But it can also do CMOS and JESD. So
you can see already that having a bus for it is completely over-kill. And those cores
have a 1:1 connection. It's just a serial interface to transfer data at high-speed
rates so I'm not really seeing how we would benefit from having a bus for it. Of
course if the day comes where this gets somehow used outside of converters and IIO
devices, we could think in making it a generic bus. As of now, I'm honestly not
seeing it.

And to add a bit more on the 1:1 stuff, what sometimes happens for highly complex
devices (like RF transceivers) with complex RX/TX data paths where each path as a
serial data interface (LVDS/CMOS/JESD) is that you have an instance of the
AXI_ADC/AXI_DAC cores per data path. So, effectively you end up with one frontend
(the transceiver) with multiple backends which also fits nicely in the current
proposal. Multiple frontends for one backend is something that I'm not aware (at
least ADI wise) but it might be a thing for other usecases. Also doable, but then you
already need to add support for it (like adding counters for enable/disable states
and things like that).

Since I'm talking usecases, one that we have at ADI that is upstreamable is a
cascaded one where the AXI_DAC core can serve as backend and frontend at the same
time. That might happen in cases the DAC core has an interpolation core behind it.
So, some fun ahead of us :) (should also be fairly straight with what we have).

One thing that at some point I might ask our hdl guys is to have an ID register so we
can identify for which frontend the backend was built. This could be useful to pass
come config to the core so we could do some safety checks. A nice thing but also not
a super duper crucial one...

> 
> Following that line of thought, if the compatible = "adi,axi-adc-10.0.a" node
> is a bus node, then logically, the ADC device node should be a child node of
> that LVDS bus node. But since the ADC is also a SPI device it also needs to
> be a child node of the SPI bus node. But is can't be a child of two different
> nodes, so where does it go?
> 

I think the above "kills" this line of thought :).

> This is where the IIO backend framework can come in. We can create a "composite"
> device as seen in the example dts above. This is just a platform device (in
> Linux-speak) and it composes the two "io-backend" devices from the the two
> busses to create a logical iio/adc device.
> 
> To solve the mystery of "where does the dmas property belong?", I have taken
> a page out of the work I am preparing the AXI SPI Engine offload support [1].
> (This hasn't been submitted to a mailing list yet because I'm still working
> on the driver, but the bindings are finished and I ran the idea by Rob on IRC
> a while back who suggested doing it this way, so maybe it works here too?)
> 
> [1]:
> https://github.com/analogdevicesinc/linux/pull/2341/commits/57bb1998371f61f4144689136aba5dd6d16a2a66
> 
> Since the "sink module" is really a separate IP block from the AXI ADC, it gets
> its own node and compatible string. Since these "sink modules" are connected to
> the AXI ADC, they get listed as child nodes, but we group them together under a
> single sink-modules node to separate them from the LVDS peripherals nodes. Then
> they get associated with a peripheral with the adi,sink-modules property (also
> see comment on that property in the example above).
> 

I never really looked at that IP but if it is something sitting behind the axi_adc
might also be a candidate for a cascaded configuration. Anyways, as I said, it's fine
to discuss it but that is all stuff that will only go in once we have users for it.
I'm also not aware if we do have an usecase for it or if it's something we control at
all from linux (might be a pure hw thing).

> My "composite" device example evolved quite a bit as I was writing this but I'm
> pretty happy with where it ended up. I think adding child nodes to the axi-adc
> node answers Nuno's concerns about how to keep a generic axi-adc binding while
> accounting for the fact that it changes slightly depending on what it is
> attached to. And having a separate platform device solves my questions about
> the ambiguity of which should be the front end, the spi node or the axi-adc
> node. It turns out, perhaps the answer is neither.

So, in conclusion, let's make this simple for now. When we have enough users and we
are aware of the real complexity of the whole thing we can see how to proceed. It
might even be you're right from the beginning and we end up in something like this
(but at least for the bus stuff I'm very sceptical). Now, if we add too much
complexity right from the beginning, it might be way harder to change/scale it down
the road.

Anyways, thanks for your feedback and for some bugs issues you already found in the
series!

- Nuno Sá

Re: [PATCH 00/12] iio: add new backend framework

[David Lechner]

On Sat, Dec 2, 2023 at 3:37 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Fri, 2023-12-01 at 21:53 -0600, David Lechner wrote:
> > On Thu, Nov 30, 2023 at 5:54 PM David Lechner <dlechner@baylibre.com> wrote:
> > >
> > > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > >
> > > > Hi all,
> > > >
> > > > 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()
> > > >
> > > > (typical provider - consumer stuff)
> > > >
> > >
> > > The "typical provider - consumer stuff" seems pretty straight forward
> > > for finding and connecting two different devices, but the definition
> > > of what is a frontend and what is a backend seems a bit nebulous. It
> > > would be nice to seem some example devicetree to be able to get a
> > > better picture of how this will be used in practices (links to the the
> > > hardware docs for those examples would be nice too).
> > >
> >
> > Fulfilling my own request here...
> >
> > Since AD9467 is being use as the example first user of the IIO offload framework
> > I did a deep dive into how it is actually being used. It looks like this:
> >
>
> This is not an offload framework... I think somehow you're connecting this to the
> spi_engine offload and these are two completely different things. Maybe they can
> intersect at some point but as of now, I don't see any benefit in doing it. The goal
> of this patchseries is to have a simple and generic framework so we can connect IIO
> devices (frontends) to a backend device having kind of an IIO aggregate device so to
> say. Moreover, we just want to have the ad9467 driver in the same state it was before
> to keep things simple. I'm already fixing some things but I don't want extend that
> too much as the primary goal is to have the framework in. Cleanups can come
> afterwards.
>
> That said, is fine to have this kind of discussion but I honestly think you're over
> engineering the whole thing. Maybe you're already too ahead of me :), but where we
> stand right now, I don't see any reason for anything so complicated as the below.
> Also note this should be simple and generic. As I already said, this is not supposed
> to be an ADI only thing and STM also wants to make use of this infrastructure. But
> see below some of my comments on why I think it's too much...

This is a very fair point. I do have a tendency to overthink things. :-)

At the very least, being able to see the schematic of how it all fits
together filled in the holes of my understanding and now everything
you are doing in this series makes sense to me. And I totally agree
with keeping it simpler is better.

Re: [PATCH 00/12] iio: add new backend framework

[Jonathan Cameron]

On Sat, 2 Dec 2023 10:16:52 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On Sat, Dec 2, 2023 at 3:37 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> >
> > On Fri, 2023-12-01 at 21:53 -0600, David Lechner wrote:  
> > > On Thu, Nov 30, 2023 at 5:54 PM David Lechner <dlechner@baylibre.com> wrote:  
> > > >
> > > > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:  
> > > > >
> > > > > Hi all,
> > > > >
> > > > > 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()
> > > > >
> > > > > (typical provider - consumer stuff)
> > > > >  
> > > >
> > > > The "typical provider - consumer stuff" seems pretty straight forward
> > > > for finding and connecting two different devices, but the definition
> > > > of what is a frontend and what is a backend seems a bit nebulous. It
> > > > would be nice to seem some example devicetree to be able to get a
> > > > better picture of how this will be used in practices (links to the the
> > > > hardware docs for those examples would be nice too).
> > > >  
> > >
> > > Fulfilling my own request here...
> > >
> > > Since AD9467 is being use as the example first user of the IIO offload framework
> > > I did a deep dive into how it is actually being used. It looks like this:
> > >  
> >
> > This is not an offload framework... I think somehow you're connecting this to the
> > spi_engine offload and these are two completely different things. Maybe they can
> > intersect at some point but as of now, I don't see any benefit in doing it. The goal
> > of this patchseries is to have a simple and generic framework so we can connect IIO
> > devices (frontends) to a backend device having kind of an IIO aggregate device so to
> > say. Moreover, we just want to have the ad9467 driver in the same state it was before
> > to keep things simple. I'm already fixing some things but I don't want extend that
> > too much as the primary goal is to have the framework in. Cleanups can come
> > afterwards.
> >
> > That said, is fine to have this kind of discussion but I honestly think you're over
> > engineering the whole thing. Maybe you're already too ahead of me :), but where we
> > stand right now, I don't see any reason for anything so complicated as the below.
> > Also note this should be simple and generic. As I already said, this is not supposed
> > to be an ADI only thing and STM also wants to make use of this infrastructure. But
> > see below some of my comments on why I think it's too much...  
> 
> This is a very fair point. I do have a tendency to overthink things. :-)
> 
> At the very least, being able to see the schematic of how it all fits
> together filled in the holes of my understanding and now everything
> you are doing in this series makes sense to me. And I totally agree
> with keeping it simpler is better.

Interesting discussion. One key thing it has highlighted for me is that
even the simpler proposal that Nuno has put forward deserves some
more documentation!  Preferably with some asci art - though maybe not as complex
as David's pretty picture.  I keep forgetting which is the backend and which
is the front end for this discussion which isn't helping me get my head
around it.

Jonathan