2023-11-21 10:19:28

by Nuno Sa via B4 Relay

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

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/[email protected]/
[2]: https://lore.kernel.org/linux-iio/[email protected]/

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


2023-11-21 10:19:30

by Nuno Sa via B4 Relay

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

From: Nuno Sa <[email protected]>

Make sure functions that return errors are not ignored.

Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
Signed-off-by: Nuno Sa <[email protected]>
---
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

2023-11-21 10:19:33

by Nuno Sa via B4 Relay

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

From: Nuno Sa <[email protected]>

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 <[email protected]>
---
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: [email protected]
S: Maintained
F: drivers/media/rc/iguanair.c

+IIO BACKEND FRAMEWORK
+M: Nuno Sa <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/iio/industrialio-backend.c
+F: include/linux/iio/backend.h
+
IIO DIGITAL POTENTIOMETER DAC
M: Peter Rosin <[email protected]>
L: [email protected]
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 <[email protected]>");
+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

2023-11-21 10:19:37

by Nuno Sa via B4 Relay

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

From: Nuno Sa <[email protected]>

Make use of spi_get_device_match_data() to simplify things.

Signed-off-by: Nuno Sa <[email protected]>
---
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

2023-11-21 10:19:48

by Nuno Sa via B4 Relay

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

From: Nuno Sa <[email protected]>

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

Signed-off-by: Nuno Sa <[email protected]>
---
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

2023-11-21 10:19:51

by Nuno Sa via B4 Relay

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

From: Nuno Sa <[email protected]>

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

Signed-off-by: Nuno Sa <[email protected]>
---
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

2023-11-21 10:19:53

by Nuno Sa via B4 Relay

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

From: Nuno Sa <[email protected]>

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 <[email protected]>
---
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

2023-11-21 23:28:56

by kernel test robot

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

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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

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

2023-11-22 00:55:21

by kernel test robot

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

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/[email protected]/config)
compiler: powerpc-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231122/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

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

2023-11-23 17:37:33

by Olivier MOYSAN

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

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/[email protected]/
> [2]: https://lore.kernel.org/linux-iio/[email protected]/
>
> ---
> 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á
>

2023-11-24 09:16:18

by Nuno Sá

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

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

2023-11-25 07:44:37

by kernel test robot

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

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/[email protected]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231125/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

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

2023-11-30 21:44:41

by David Lechner

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

On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
<[email protected]> wrote:
>
> From: Nuno Sa <[email protected]>
>
> Make sure functions that return errors are not ignored.
>
> Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> Signed-off-by: Nuno Sa <[email protected]>
> ---
> 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>

2023-11-30 23:31:47

by David Lechner

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

On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
<[email protected]> wrote:
>
> From: Nuno Sa <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>
>

2023-11-30 23:34:27

by David Lechner

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

On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
<[email protected]> wrote:
>
> From: Nuno Sa <[email protected]>
>
> Move to the IIO backend framework. Devices supported by adi-axi-adc now
> register themselves as backend devices.
>
> Signed-off-by: Nuno Sa <[email protected]>
> ---
> 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
>
>

2023-11-30 23:55:00

by David Lechner

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

On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
<[email protected]> 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"?

2023-12-01 00:12:45

by David Lechner

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

On Thu, Nov 30, 2023 at 5:30 PM David Lechner <[email protected]> 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".

2023-12-01 08:42:13

by Nuno Sá

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

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
> <[email protected]> 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/[email protected]/

- Nuno Sá

2023-12-01 08:48:05

by Nuno Sá

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

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
> <[email protected]> wrote:
> >
> > From: Nuno Sa <[email protected]>
> >
> > Make sure functions that return errors are not ignored.
> >
> > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > Signed-off-by: Nuno Sa <[email protected]>
> > ---
> >  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>

2023-12-01 08:50:54

by Nuno Sá

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

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
> <[email protected]> wrote:
> >
> > From: Nuno Sa <[email protected]>
> >
> > Move to the IIO backend framework. Devices supported by adi-axi-adc now
> > register themselves as backend devices.
> >
> > Signed-off-by: Nuno Sa <[email protected]>
> > ---
> >  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á
>

2023-12-01 09:08:47

by Nuno Sá

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

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
> <[email protected]> wrote:
> >
> > From: Nuno Sa <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> >  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á
>

2023-12-01 09:14:31

by Nuno Sá

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

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
> > <[email protected]> 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á

2023-12-01 09:17:39

by Nuno Sá

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

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
> <[email protected]> wrote:
> >
> > From: Nuno Sa <[email protected]>
> >
> > 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á

2023-12-01 17:45:42

by David Lechner

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

On Fri, Dec 1, 2023 at 3:08 AM Nuno Sá <[email protected]> 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
> > <[email protected]> wrote:
> > >
> > > From: Nuno Sa <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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

2023-12-02 03:54:08

by David Lechner

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

On Thu, Nov 30, 2023 at 5:54 PM David Lechner <[email protected]> wrote:
>
> On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> <[email protected]> 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.

2023-12-02 08:51:09

by Nuno Sá

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

On Fri, 2023-12-01 at 11:44 -0600, David Lechner wrote:
> On Fri, Dec 1, 2023 at 3:08 AM Nuno Sá <[email protected]> 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
> > > <[email protected]> wrote:
> > > >
> > > > From: Nuno Sa <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > >  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á

2023-12-02 09:37:57

by Nuno Sá

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

On Fri, 2023-12-01 at 21:53 -0600, David Lechner wrote:
> On Thu, Nov 30, 2023 at 5:54 PM David Lechner <[email protected]> wrote:
> >
> > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > <[email protected]> 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á

2023-12-02 16:17:19

by David Lechner

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

On Sat, Dec 2, 2023 at 3:37 AM Nuno Sá <[email protected]> wrote:
>
> On Fri, 2023-12-01 at 21:53 -0600, David Lechner wrote:
> > On Thu, Nov 30, 2023 at 5:54 PM David Lechner <[email protected]> wrote:
> > >
> > > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > > <[email protected]> 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.

2023-12-04 08:56:58

by Nuno Sá

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

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á <[email protected]> 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
> > > > <[email protected]> wrote:
> > > > >
> > > > > From: Nuno Sa <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > >  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á

2023-12-04 14:49:54

by Jonathan Cameron

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

On Sat, 2 Dec 2023 10:16:52 -0600
David Lechner <[email protected]> wrote:

> On Sat, Dec 2, 2023 at 3:37 AM Nuno Sá <[email protected]> wrote:
> >
> > On Fri, 2023-12-01 at 21:53 -0600, David Lechner wrote:
> > > On Thu, Nov 30, 2023 at 5:54 PM David Lechner <[email protected]> wrote:
> > > >
> > > > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > > > <[email protected]> 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



2023-12-04 15:20:12

by Jonathan Cameron

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

On Tue, 21 Nov 2023 11:20:18 +0100
Nuno Sa via B4 Relay <[email protected]> wrote:

> From: Nuno Sa <[email protected]>
>
> Make sure functions that return errors are not ignored.
>
> Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> Signed-off-by: Nuno Sa <[email protected]>
> ---
> 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;
>

2023-12-04 15:39:18

by Jonathan Cameron

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

On Tue, 21 Nov 2023 11:20:16 +0100
Nuno Sa via B4 Relay <[email protected]> wrote:

> From: Nuno Sa <[email protected]>
>
> 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 <[email protected]>

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; \
> +})

2023-12-04 15:48:31

by Jonathan Cameron

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

On Fri, 01 Dec 2023 10:08:27 +0100
Nuno Sá <[email protected]> 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
> > <[email protected]> wrote:
> > >
> > > From: Nuno Sa <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > >  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

2023-12-04 15:52:21

by Jonathan Cameron

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

On Tue, 21 Nov 2023 11:20:24 +0100
Nuno Sa via B4 Relay <[email protected]> wrote:

> From: Nuno Sa <[email protected]>
>
> Use MMIO regmap interface. It makes things easier for manipulating bits.
>
> Signed-off-by: Nuno Sa <[email protected]>
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,
>

2023-12-04 16:16:25

by Nuno Sá

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

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 <[email protected]> wrote:
>
> > From: Nuno Sa <[email protected]>
> >
> > Use MMIO regmap interface. It makes things easier for manipulating bits.
> >
> > Signed-off-by: Nuno Sa <[email protected]>
> 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á

2023-12-04 16:23:31

by Nuno Sá

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

On Mon, 2023-12-04 at 15:48 +0000, Jonathan Cameron wrote:
> On Fri, 01 Dec 2023 10:08:27 +0100
> Nuno Sá <[email protected]> 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
> > > <[email protected]> wrote: 
> > > >
> > > > From: Nuno Sa <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > >  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á

2023-12-04 16:57:33

by Jonathan Cameron

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

On Mon, 04 Dec 2023 17:23:12 +0100
Nuno Sá <[email protected]> wrote:

> On Mon, 2023-12-04 at 15:48 +0000, Jonathan Cameron wrote:
> > On Fri, 01 Dec 2023 10:08:27 +0100
> > Nuno Sá <[email protected]> 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
> > > > <[email protected]> wrote: 
> > > > >
> > > > > From: Nuno Sa <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > >  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á

2023-12-06 12:06:12

by Nuno Sá

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

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 <[email protected]> wrote:
>
> > From: Nuno Sa <[email protected]>
> >
> > 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 <[email protected]>
>
> 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á

2023-12-06 17:15:50

by Jonathan Cameron

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

On Wed, 06 Dec 2023 13:05:53 +0100
Nuno Sá <[email protected]> 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 <[email protected]> wrote:
> >
> > > From: Nuno Sa <[email protected]>
> > >
> > > 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 <[email protected]>
> >
> > 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á
>