* [RFC PATCH 1/3] iio: addac: add new converter framework
2023-08-04 14:53 [RFC PATCH 0/3] Add converter framework Nuno Sa
@ 2023-08-04 14:53 ` Nuno Sa
2023-08-30 17:02 ` Jonathan Cameron
2023-08-04 14:53 ` [RFC PATCH 2/3] iio: adc: ad9647: add based on " Nuno Sa
` (2 subsequent siblings)
3 siblings, 1 reply; 19+ messages in thread
From: Nuno Sa @ 2023-08-04 14:53 UTC (permalink / raw)
To: linux-iio; +Cc: Jonathan Cameron
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/addac/converter.c | 547 ++++++++++++++++++++++++++++
include/linux/iio/addac/converter.h | 485 ++++++++++++++++++++++++
2 files changed, 1032 insertions(+)
create mode 100644 drivers/iio/addac/converter.c
create mode 100644 include/linux/iio/addac/converter.h
diff --git a/drivers/iio/addac/converter.c b/drivers/iio/addac/converter.c
new file mode 100644
index 000000000000..31ac704255ad
--- /dev/null
+++ b/drivers/iio/addac/converter.c
@@ -0,0 +1,547 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Framework to handle complex IIO aggregate devices
+ *
+ * A note on some of the design expectations with regards to lifetimes and
+ * devices bringup/removal.
+ *
+ * The Framework is using, under the wood, the component API which makes it to
+ * easy treat a bunch of devices as one aggregate device. This means that the
+ * complete thing is only brought to live when all the deviced are probed. To do
+ * this, two callbacks are used that should in fact completely replace .probe()
+ * and .remove(). The formers should only be used the minimum needed. Ideally,
+ * only to call the functions to add and remove frontend annd backend devices.
+ *
+ * It is advised for frontend and backend drivers to use their .remove()
+ * callbacks (to use devres API during the frontend and backends initialization).
+ * See the comment in @converter_frontend_bind().
+ *
+ * It is also assumed that converter objects cannot be accessed once one of the
+ * devices of the aggregate device is removed (effectively bringing the all the
+ * devices down). Based on that assumption, these objects are not refcount which
+ * means accessing them will likely fail miserably.
+ *
+ * Copyright (C) 2023 Analog Devices Inc.
+ */
+
+#define dev_fmt(fmt) "Converter - " fmt
+
+#include <linux/component.h>
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/iio/addc/converter.h>
+#include <linux/iio/iio.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+struct converter_backend {
+ struct list_head entry;
+ struct device *dev;
+ const struct converter_ops *ops;
+ const char *name;
+ void *drvdata;
+
+ struct regmap *regmap;
+ unsigned int cached_reg_addr;
+};
+
+struct converter_frontend {
+ struct list_head list;
+ const struct frontend_ops *ops;
+ struct device *dev;
+};
+
+static ssize_t converter_debugfs_read_reg(struct file *file,
+ char __user *userbuf,
+ size_t count, loff_t *ppos)
+{
+ struct converter_backend *conv = file->private_data;
+ unsigned int val = 0;
+ char read_buf[20];
+ int ret, len;
+
+ ret = regmap_read(conv->regmap, conv->cached_reg_addr, &val);
+ if (ret) {
+ dev_err(conv->dev, "%s: read failed\n", __func__);
+ return ret;
+ }
+
+ len = scnprintf(read_buf, sizeof(read_buf), "0x%X\n", val);
+
+ return simple_read_from_buffer(userbuf, count, ppos, read_buf, len);
+}
+
+static ssize_t converter_debugfs_write_reg(struct file *file,
+ const char __user *userbuf,
+ size_t count, loff_t *ppos)
+{
+ struct converter_backend *conv = file->private_data;
+ unsigned int val;
+ char buf[80];
+ ssize_t rc;
+ int ret;
+
+ rc = simple_write_to_buffer(buf, sizeof(buf), ppos, userbuf, count);
+ if (rc < 0)
+ return rc;
+
+ ret = sscanf(buf, "%i %i", &conv->cached_reg_addr, &val);
+
+ switch (ret) {
+ case 1:
+ break;
+ case 2:
+ ret = regmap_write(conv->regmap, conv->cached_reg_addr, val);
+ if (ret) {
+ dev_err(conv->dev, "%s: write failed\n", __func__);
+ return ret;
+ }
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return count;
+}
+
+static const struct file_operations converter_debugfs_reg_fops = {
+ .open = simple_open,
+ .read = converter_debugfs_read_reg,
+ .write = converter_debugfs_write_reg,
+};
+
+static void __converter_add_direct_reg_access(struct converter_backend *conv,
+ struct iio_dev *indio_dev)
+{
+ struct dentry *d = iio_get_debugfs_dentry(indio_dev);
+ const char *name = conv->name;
+ char file_name[64];
+
+ if (!conv->regmap)
+ return;
+ if (!d)
+ return;
+
+ if (!conv->name)
+ name = "converter";
+
+ snprintf(file_name, sizeof(file_name), "%s_direct_reg_access", name);
+
+ debugfs_create_file(file_name, 0644, d, conv,
+ &converter_debugfs_reg_fops);
+}
+
+void converter_add_direct_reg_access(struct converter_backend *conv,
+ struct iio_dev *indio_dev)
+{
+ if (IS_ENABLED(CONFIG_DEBUG_FS))
+ __converter_add_direct_reg_access(conv, indio_dev);
+}
+EXPORT_SYMBOL_NS_GPL(converter_add_direct_reg_access, IIO_CONVERTER);
+
+static int converter_bind(struct device *dev, struct device *aggregate,
+ void *data)
+{
+ struct converter_frontend *frontend = dev_get_drvdata(aggregate);
+ struct converter_backend *conv = dev_get_drvdata(dev);
+ int ret;
+
+ ret = conv->ops->backend_init(conv, dev);
+ if (ret)
+ return ret;
+
+ list_add_tail(&conv->entry, &frontend->list);
+
+ return 0;
+}
+
+static void converter_unbind(struct device *dev, struct device *aggregate,
+ void *data)
+{
+ struct converter_backend *conv = dev_get_drvdata(dev);
+
+ if (conv->ops->backend_close)
+ conv->ops->backend_close(conv);
+
+ /* after this point the converter should not be used anymore */
+ converter_set_drvdata(conv, NULL);
+}
+
+static const struct component_ops converter_component_ops = {
+ .bind = converter_bind,
+ .unbind = converter_unbind,
+};
+
+static int converter_frontend_bind(struct device *dev)
+{
+ struct converter_frontend *frontend = dev_get_drvdata(dev);
+ int ret;
+
+ ret = component_bind_all(dev, NULL);
+ if (ret)
+ return ret;
+ /*
+ * We open a new group so that we can control when resources are
+ * released and still use device managed (devm_) calls. The expectations
+ * are that on probe, backend resources are allocated first followed by
+ * the frontend resources (where registering the IIO device must happen)
+ * Naturally we want the reverse order on the unbind path and that would
+ * not be possible without opening our own devres group.
+
+ * Note that the component API also opens it's own devres group when
+ * calling the .bind() callbacks for both the aggregate device
+ * (our frontend) and each of the components (our backends). On the
+ * unbind path, the aggregate .unbind() function is called
+ * (@converter_frontend_unbind()) which should be responsible to tear
+ * down all the components (effectively releasing all the resources
+ * allocated on each component devres group) and only then the aggregate
+ * devres group is released. Hence, the order we want to maintain for
+ * releasing resources would not be satisfied because backend resources
+ * would be freed first. With our own group, we can control when
+ * releasing the resources and we do it before @component_unbind_all().
+ *
+ * This also relies that internally the component API is releasing each
+ * of the component's devres group. That is likely not to change, but
+ * maybe we should not trust it and also open our own groups for backend
+ * devices?!
+ *
+ * Another very important thing to keep in mind is that this is only
+ * valid if frontend and backend driver's are implementing their
+ * .remove() callback to call @converter_frontend_del() and
+ * @converter_backend_del(). Calling those functions from
+ * devm_add_action* and use devm APIs in .frontend_init() and
+ * .backend_init() is not going to work. Not perfect but still better
+ * than having to tear everything down in .frontend_close() and
+ * .backend_close()
+ */
+ if (!devres_open_group(dev, frontend, GFP_KERNEL))
+ return -ENOMEM;
+
+ ret = frontend->ops->frontend_init(frontend, dev);
+ if (ret) {
+ devres_release_group(dev, frontend);
+ return ret;
+ }
+
+ devres_close_group(dev, NULL);
+ return 0;
+}
+
+static void converter_frontend_unbind(struct device *dev)
+{
+ struct converter_frontend *frontend = dev_get_drvdata(dev);
+
+ if (frontend->ops->frontend_close)
+ frontend->ops->frontend_close(frontend);
+
+ devres_release_group(dev, frontend);
+ component_unbind_all(dev, NULL);
+ list_del_init(&frontend->list);
+}
+
+static const struct component_master_ops frontend_component_ops = {
+ .bind = converter_frontend_bind,
+ .unbind = converter_frontend_unbind,
+};
+
+struct converter_backend *converter_get(const struct converter_frontend *frontend,
+ const char *name)
+{
+ struct converter_backend *iter, *conv = NULL;
+ struct device *dev = frontend->dev;
+ struct fwnode_handle *fwnode;
+ int index = 0;
+
+ if (list_empty(&frontend->list)) {
+ dev_err(dev, "Backend list is empty...\n");
+ return ERR_PTR(-ENODEV);
+ }
+
+ /* if no name given, we assume only one converter_backend exists */
+ if (!name)
+ return list_first_entry(&frontend->list,
+ struct converter_backend, entry);
+
+ index = device_property_match_string(frontend->dev, "converter-names",
+ name);
+ if (index < 0)
+ return ERR_PTR(index);
+
+ fwnode = fwnode_find_reference(dev_fwnode(dev), "converters", index);
+ if (IS_ERR(fwnode))
+ return ERR_CAST(fwnode);
+
+ list_for_each_entry(iter, &frontend->list, entry) {
+ if (device_match_fwnode(iter->dev, fwnode)) {
+ conv = iter;
+ break;
+ }
+ }
+
+ fwnode_handle_put(fwnode);
+
+ if (!conv) {
+ dev_err(dev, "Converter (%s) not found in the list\n", name);
+ return ERR_PTR(-ENODEV);
+ }
+
+ /* See if we can add device_property_string_read_index() */
+ conv->name = kstrdup_const(name, GFP_KERNEL);
+ if (!conv->name)
+ return ERR_PTR(-ENOMEM);
+
+ return conv;
+}
+EXPORT_SYMBOL_NS_GPL(converter_get, IIO_CONVERTER);
+
+static int converter_frontend_add_matches(struct converter_frontend *frontend,
+ struct component_match **match)
+{
+ struct device *dev = frontend->dev;
+ struct fwnode_handle *fwnode;
+ int index = 0;
+
+ do {
+ fwnode = fwnode_find_reference(dev_fwnode(dev), "converters",
+ index);
+ if (IS_ERR(fwnode))
+ break;
+
+ component_match_add_release(dev, match,
+ component_release_fwnode,
+ component_compare_fwnode, fwnode);
+ index++;
+ } while (true);
+
+ /* no devices?! */
+ if (!index) {
+ dev_err(dev, "No converters. Make sure the \"converters\" property is given!\n");
+ return -ENODEV;
+ }
+
+ if (PTR_ERR(fwnode) != -ENOENT)
+ return PTR_ERR(fwnode);
+
+ return 0;
+}
+
+int converter_test_pattern_set(struct converter_backend *conv,
+ unsigned int chan,
+ enum converter_test_pattern pattern)
+{
+ if (pattern >= CONVERTER_TEST_PATTERN_MAX)
+ return -EINVAL;
+ if (!conv->ops->test_pattern_set)
+ return -ENOTSUPP;
+
+ return conv->ops->test_pattern_set(conv, chan, pattern);
+}
+EXPORT_SYMBOL_NS_GPL(converter_test_pattern_set, IIO_CONVERTER);
+
+int converter_chan_status_get(struct converter_backend *conv,
+ unsigned int chan,
+ struct converter_chan_status *status)
+{
+ if (!conv->ops->chan_status)
+ return -ENOTSUPP;
+
+ return conv->ops->chan_status(conv, chan, status);
+}
+EXPORT_SYMBOL_NS_GPL(converter_chan_status_get, IIO_CONVERTER);
+
+int converter_iodelay_set(struct converter_backend *conv,
+ unsigned int num_lanes, unsigned int delay)
+{
+ if (!num_lanes)
+ return -EINVAL;
+ if (!conv->ops->iodelay_set)
+ return -ENOTSUPP;
+
+ return conv->ops->iodelay_set(conv, num_lanes, delay);
+}
+EXPORT_SYMBOL_NS_GPL(converter_iodelay_set, IIO_CONVERTER);
+
+int converter_data_format_set(struct converter_backend *conv,
+ unsigned int chan,
+ const struct converter_data_fmt *data)
+{
+ if (data->type >= CONVERTER_DATA_TYPE_MAX)
+ return -EINVAL;
+ if (!conv->ops->data_format_set)
+ return -ENOTSUPP;
+
+ return conv->ops->data_format_set(conv, chan, data);
+}
+EXPORT_SYMBOL_NS_GPL(converter_data_format_set, IIO_CONVERTER);
+
+int converter_sample_edge_select(struct converter_backend *conv,
+ enum converter_edge edge)
+{
+ if (edge >= CONVERTER_EDGE_MAX)
+ return -EINVAL;
+ if (conv->ops->sample_edge_select)
+ return -ENOTSUPP;
+
+ return conv->ops->sample_edge_select(conv, edge);
+}
+EXPORT_SYMBOL_NS_GPL(converter_sample_edge_select, IIO_CONVERTER);
+
+int converter_chan_enable(struct converter_backend *conv, unsigned int chan)
+{
+ if (!conv->ops->chan_enable)
+ return -ENOTSUPP;
+
+ return conv->ops->chan_enable(conv, chan);
+}
+EXPORT_SYMBOL_NS_GPL(converter_chan_enable, IIO_CONVERTER);
+
+int converter_chan_disable(struct converter_backend *conv, unsigned int chan)
+{
+ if (!conv->ops->disable)
+ return -ENOTSUPP;
+
+ return conv->ops->chan_disable(conv, chan);
+}
+EXPORT_SYMBOL_NS_GPL(converter_chan_disable, IIO_CONVERTER);
+
+int converter_enable(struct converter_backend *conv)
+{
+ if (!conv->ops->enable)
+ return -ENOTSUPP;
+
+ return conv->ops->enable(conv);
+}
+EXPORT_SYMBOL_NS_GPL(converter_enable, IIO_CONVERTER);
+
+void converter_disable(struct converter_backend *conv)
+{
+ if (!conv->ops->disable)
+ return;
+
+ conv->ops->disable(conv);
+}
+EXPORT_SYMBOL_NS_GPL(converter_disable, IIO_CONVERTER);
+
+int __converter_test_pattern_xlate(unsigned int pattern,
+ const struct converter_test_pattern_xlate *xlate,
+ int n_matches)
+{
+ unsigned int p = n_matches;
+
+ while (p--) {
+ if (pattern == xlate[p].pattern)
+ return xlate[p].reg_val;
+ }
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL_NS_GPL(__converter_test_pattern_xlate, IIO_CONVERTER);
+
+void converter_set_regmap(struct converter_backend *conv,
+ struct regmap *regmap)
+{
+ conv->regmap = regmap;
+}
+EXPORT_SYMBOL_NS_GPL(converter_set_regmap, IIO_CONVERTER);
+
+void converter_set_drvdata(struct converter_backend *conv, void *drvdata)
+{
+ conv->drvdata = drvdata;
+}
+EXPORT_SYMBOL_NS_GPL(converter_set_drvdata, IIO_CONVERTER);
+
+void *converter_get_drvdata(const struct converter_backend *conv)
+{
+ WARN_ON(!conv->drvdata);
+ return conv->drvdata;
+}
+EXPORT_SYMBOL_NS_GPL(converter_get_drvdata, IIO_CONVERTER);
+
+void converter_del(struct device *dev)
+{
+ component_del(dev, &converter_component_ops);
+}
+EXPORT_SYMBOL_NS_GPL(converter_del, IIO_CONVERTER);
+
+static void converter_free(void *conv)
+{
+ struct converter_backend *__conv = conv;
+
+ if (__conv->name)
+ kfree_const(__conv->name);
+
+ kfree(__conv);
+}
+
+int converter_add(struct device *dev, const struct converter_ops *ops)
+{
+ struct converter_backend *conv;
+ int ret;
+
+ if (!ops || !ops->backend_init)
+ return -EINVAL;
+
+ conv = kzalloc(sizeof(*conv), GFP_KERNEL);
+ if (!conv)
+ return -ENOMEM;
+
+ /*
+ * The expectation is that everything goes up and down in
+ * .converter_bind() and .converter_unbind() respectively. Hence, it's
+ * not expected for converter objects to be accessed after unbind(). As
+ * soon as that does not stand anymore, we need to
+ * drop devm_add_action_or_reset() and properly refcount the objects.
+ */
+ ret = devm_add_action_or_reset(dev, converter_free, conv);
+ if (ret)
+ return ret;
+
+ conv->ops = ops;
+ dev_set_drvdata(dev, conv);
+ conv->dev = dev;
+
+ return component_add(dev, &converter_component_ops);
+}
+EXPORT_SYMBOL_NS_GPL(converter_add, IIO_CONVERTER);
+
+void converter_frontend_del(struct device *dev)
+{
+ component_master_del(dev, &frontend_component_ops);
+}
+EXPORT_SYMBOL_NS_GPL(converter_frontend_del, IIO_CONVERTER);
+
+int converter_frontend_add(struct device *dev, const struct frontend_ops *ops)
+{
+ struct converter_frontend *frontend;
+ struct component_match *match;
+ int ret;
+
+ if (!ops || !ops->frontend_init) {
+ dev_err(dev, "Mandatory ops missing\n");
+ return -EINVAL;
+ }
+
+ frontend = devm_kzalloc(dev, sizeof(*frontend), GFP_KERNEL);
+ if (!frontend)
+ return -ENOMEM;
+
+ frontend->ops = ops;
+ frontend->dev = dev;
+ INIT_LIST_HEAD(&frontend->list);
+ dev_set_drvdata(dev, frontend);
+
+ ret = converter_frontend_add_matches(frontend, &match);
+ if (ret)
+ return ret;
+
+ return component_master_add_with_match(dev, &frontend_component_ops,
+ match);
+}
+EXPORT_SYMBOL_NS_GPL(converter_frontend_add, IIO_CONVERTER);
+
+MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>");
+MODULE_DESCRIPTION("Framework to handle complex IIO aggregate devices");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/iio/addac/converter.h b/include/linux/iio/addac/converter.h
new file mode 100644
index 000000000000..09d9d491b2b8
--- /dev/null
+++ b/include/linux/iio/addac/converter.h
@@ -0,0 +1,485 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _CONVERTER_H
+#define _CONVERTER_H
+
+struct converter_frontend;
+struct converter_backend;
+struct iio_dev;
+struct device;
+struct regmap;
+
+enum converter_test_pattern {
+ CONVERTER_PRBS_7,
+ CONVERTER_PRBS_15,
+ CONVERTER_PRBS_23,
+ CONVERTER_PRBS_31,
+ CONVERTER_RAMP_NIBBLE,
+ CONVERTER_RAMP_16,
+ /* vendor specific from 32 */
+ CONVERTER_ADI_PRBS_9A = 32,
+ CONVERTER_ADI_PRBS_23A,
+ CONVERTER_ADI_PRBS_X,
+ CONVERTER_TEST_PATTERN_MAX
+};
+
+enum converter_data_type {
+ CONVERTER_TWOS_COMPLEMENT,
+ CONVERTER_OFFSET_BINARY,
+ CONVERTER_DATA_TYPE_MAX
+};
+
+enum converter_edge {
+ CONVERTER_RISING_EDGE_SAMPLE,
+ CONVERTER_FALLING_EDGE_SAMPLE,
+ CONVERTER_EDGE_MAX
+};
+
+struct converter_chan_status {
+ bool errors;
+};
+
+/**
+ * struct converter_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 converter_data_fmt {
+ enum converter_data_type type;
+ bool sign_extend;
+ bool enable;
+};
+
+/**
+ * struct converter_test_pattern_xlate - Helper struct for test pattern handling
+ * @pattern: Pattern to configure.
+ * @reg_val: Register value for the pattern to configure.
+ */
+struct converter_test_pattern_xlate {
+ enum converter_test_pattern pattern;
+ unsigned int reg_val;
+};
+
+/**
+ * struct converter_ops - Backend supported operations
+ * @backend_init: Mandatory function to initialize the backend device. It
+ * should be a replacement for .probe() where the latest
+ * should only have to care about doing @converter_add().
+ * @backend_close: Optional function to tear down the device.
+ * @enable: Enable the backend device.
+ * @disable: Disable the backend device.
+ * @data_format_set: Configure the data format for a specific channel.
+ * @chan_enable: Enable one channel.
+ * @chan_disable: Disable one channel.
+ * @iodelay_set: Controls the IO delay for all the lanes at the interface
+ * (where data is actually transferred between frontend and
+ backend) level.
+ * @test_pattern_set: Set's a test pattern to be transmitted/received by the
+ * backend. Typically useful for debug or interface
+ * purposes calibration.
+ */
+struct converter_ops {
+ int (*backend_init)(struct converter_backend *conv, struct device *dev);
+ void (*backend_close)(struct converter_backend *conv);
+ int (*enable)(struct converter_backend *conv);
+ void (*disable)(struct converter_backend *conv);
+ int (*data_format_set)(struct converter_backend *conv,
+ unsigned int chan,
+ const struct converter_data_fmt *data);
+ int (*chan_enable)(struct converter_backend *conv, unsigned int chan);
+ int (*chan_disable)(struct converter_backend *conv, unsigned int chan);
+ int (*iodelay_set)(struct converter_backend *conv,
+ unsigned int num_lanes, unsigned int delay);
+ int (*test_pattern_set)(struct converter_backend *conv,
+ unsigned int chan,
+ enum converter_test_pattern pattern);
+ int (*chan_status)(struct converter_backend *conv, unsigned int chan,
+ struct converter_chan_status *status);
+ int (*sample_edge_select)(struct converter_backend *conv,
+ enum converter_edge edge);
+};
+
+/**
+ * struct frontend_ops - Frontend supported operations
+ * @frontend_init: Mandatory function to initialize the frontend device. It
+ * should be a replacement for .probe() where the latest
+ * should only have to care about doing @frontend_add().
+ * @frontend_close: Optional function to tear down the device.
+ */
+struct frontend_ops {
+ int (*frontend_init)(struct converter_frontend *frontend,
+ struct device *dev);
+ void (*frontend_close)(struct converter_frontend *frontend);
+};
+
+/**
+ * converter_test_pattern_xlate() - Helper macro for translatting test patterns
+ * @pattern: Pattern to translate.
+ * @xlate: List of @struct converter_test_pattern_xlate pairs.
+ *
+ * Simple helper to match a supported pattern and get the register value. Should
+ * only be called by backend devices. Automatically computes the number of
+ * @xlate entries.
+ */
+#define converter_test_pattern_xlate(pattern, xlate) \
+ __converter_test_pattern_xlate(pattern, xlate, ARRAY_SIZE(xlate));
+
+#if IS_ENABLED(CONFIG_IIO_CONVERTER)
+
+/**
+ * converter_get_drvdata - Get driver private data
+ * @conv: Converter device.
+ */
+void *converter_get_drvdata(const struct converter_backend *conv);
+
+/**
+ * converter_set_drvdata - Set driver private data
+ * @conv: Converter device.
+ * @drvdata: Driver private data.
+ */
+void converter_set_drvdata(struct converter_backend *conv, void *drvdata);
+
+/**
+ * converter_set_regmap - Add a regmap object to a converter
+ * @conv: Converter device.
+ * @regmap: Regmap object.
+ */
+void converter_set_regmap(struct converter_backend *conv,
+ struct regmap *regmap);
+
+/**
+ * __converter_test_pattern_xlate - Helper macro for translatting test patterns
+ * @pattern: Pattern to translate.
+ * @xlate: List of @struct converter_test_pattern_xlate pairs.
+ * @n_matches: Number of entries in @xlate.
+ *
+ * Simple helper to match a supported pattern and get the register value. Should
+ * only be called by backend devices.
+ */
+int __converter_test_pattern_xlate(unsigned int pattern,
+ const struct converter_test_pattern_xlate *xlate,
+ int n_matches);
+
+/**
+ *
+ */
+int converter_add(struct device *dev, const struct converter_ops *ops);
+
+/**
+ * converter_del - Remove the converter device
+ * @dev: device to remove from the aggregate
+ *
+ * Removes the converter from the aggregate device. This tears down the frontend
+ * and all the converters.
+ *
+ * Ideally, this should be called from the backend driver .remove() callback.
+ * This means that all the converters (and the frontend) will be tear down before
+ * running any specific devres cleanup (at the driver core level). What this all
+ * means is that we can use devm_ apis in @backend_init() and being sure those
+ * resources will be released after the backend resources and before any devm_*
+ * used in @probe(). If that is not the case, one should likely not use any
+ * devm_ API in @backend_init(). That means .backend_close() should be
+ * provided to do all the necessary cleanups.
+ */
+void converter_del(struct device *dev);
+
+/**
+ * converter_enable - Enable the device
+ * @conv: Converter device.
+ *
+ * Enables the backend device.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int converter_enable(struct converter_backend *conv);
+
+/**
+ * converter_disable - Disable the device
+ * @conv: Converter device.
+ *
+ * Disables the backend device.
+ */
+void converter_disable(struct converter_backend *conv);
+
+/**
+ * converter_test_pattern_set - Set a test pattern
+ * @conv: Converter device.
+ * @chan: Channel number.
+ * @pattern: Pattern to set.
+ *
+ * Set's a test pattern to be transmitted/received by the backend. Typically
+ * useful for debug or interface calibration purposes. A backend driver can
+ * call the @converter_test_pattern_xlate() helper to validate the pattern
+ * (given an array of @struct converter_test_pattern_xlate).
+ *
+ * Note that some patterns might be frontend specific. I.e, as far as the
+ * backend is concerned the pattern is valid (from a register point of view) but
+ * the actual support for the pattern is not implemented in the device for this
+ * specific frontend. It's up to the frontend to ask for a proper pattern
+ * (as it should know better).
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int converter_test_pattern_set(struct converter_backend *conv,
+ unsigned int chan,
+ enum converter_test_pattern pattern);
+
+int converter_chan_status_get(struct converter_backend *conv,
+ unsigned int chan,
+ struct converter_chan_status *status);
+
+/**
+ * converter_data_format_set - Configure the data format
+ * @conv: Converter device.
+ * @chan: Channel number.
+ * @data: Data format.
+ *
+ * Properly configure a channel with respect to the expected data format. A
+ * @struct converter_data_fmt must be passed with the settings.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int converter_data_format_set(struct converter_backend *conv,
+ unsigned int chan,
+ const struct converter_data_fmt *data);
+
+int converter_sample_edge_select(struct converter_backend *conv,
+ enum converter_edge edge);
+
+static inline int
+converter_sample_on_falling_edge(struct converter_backend *conv)
+{
+ return converter_sample_edge_select(conv, CONVERTER_RISING_EDGE_SAMPLE);
+}
+
+static inline int
+converter_sample_on_rising_edge(struct converter_backend *conv)
+{
+ return converter_sample_edge_select(conv, CONVERTER_FALLING_EDGE_SAMPLE);
+}
+
+/**
+ * converter_chan_enable - Enable a backend channel
+ * @conv: Converter device.
+ * @chan: Channel number.
+ *
+ * Enables a channel on the backend device.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int converter_chan_enable(struct converter_backend *conv, unsigned int chan);
+
+/**
+ * converter_chan_disable - Disable a backend channel
+ * @conv: Converter device.
+ * @chan: Channel number.
+ *
+ * Disables a channel on the backend device.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int converter_chan_disable(struct converter_backend *conv, unsigned int chan);
+
+/**
+ * converter_iodelay_set - Set's the backend data interface IO delay
+ * @conv: Converter device.
+ * @num_lanes: Number of lanes in the data interface.
+ * @delay: Delay to set.
+ *
+ * Controls the IO delay for all the lanes at the data interface (where data is
+ * actually transferred between frontend and backend) level.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int converter_iodelay_set(struct converter_backend *conv,
+ unsigned int num_lanes, unsigned int delay);
+
+/**
+ * converter_frontend_del - Remove the frontend device
+ * @dev: Device to remove from the aggregate
+ *
+ * Removes the frontend from the aggregate device. This tears down the frontend
+ * and all the converters.
+ *
+ * Ideally, this should be called from the frontend driver .remove() callback.
+ * This means that all the converters (and the frontend) will be tear down
+ * before running any specific devres cleanup (at the driver core level). What
+ * this all means is that we can use devm_ apis in .frontend_init() and being
+ * sure those resources will be released after the backend resources and before
+ * any devm_* used in .probe(). If that is not the case, one should likely not
+ * use any devm_ API in .frontend_init(). That means .frontend_close() should be
+ * provided to do all the necessary cleanups.
+ */
+void converter_frontend_del(struct device *dev);
+
+/**
+ * converter_frontend_add - Allocate and add a frontend device
+ * @dev: Device to allocate frontend for.
+ * @ops: Frontend callbacks.
+ *
+ * This allocates the frontend device and looks for all converters needed
+ * so that, when they are available, all of the devices in the aggregate can be
+ * initialized.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int converter_frontend_add(struct device *dev, const struct frontend_ops *ops);
+
+/**
+ * converter_get - Get a converter object
+ * @frontend: Frontend device.
+ * @name: Converter name.
+ *
+ * Get's a pointer to a converter device. If name is NULL, then it is assumed
+ * that only one backend device is bond with the frontend and the first element
+ * in the list is retrieved. Should only be called from the .frontend_init()
+ * callback.
+ *
+ * RETURNS:
+ * A converter pointer, negative error pointer otherwise.
+ */
+struct converter_backend *__must_check
+converter_get(const struct converter_frontend *frontend, const char *name);
+
+/**
+ * converter_add_direct_reg_access - Add debugfs direct register access
+ * @conv: Coverter device
+ * @indio_dev: IIO device
+ *
+ * This is analogous to the typical IIO direct register access in debugfs. The
+ * extra converter file will be added in the same debugs dir as @indio_dev.
+ * Moreover, if @conv->name is NULL, the file will be called
+ * converter_direct_reg_access. Otherwise, will be
+ * @conv->name_converter_direct_reg_access.
+ */
+void converter_add_direct_reg_access(struct converter_backend *conv,
+ struct iio_dev *indio_dev);
+
+#else
+
+static inline void *converter_get_drvdata(const struct converter_backend *conv)
+{
+ WARN_ONCE(1, "converter API is disabled");
+ return NULL;
+}
+
+static inline void converter_set_drvdata(struct converter_backend *conv,
+ void *drvdata)
+{
+ WARN_ONCE(1, "converter API is disabled");
+}
+
+static inline void converter_set_regmap(struct converter_backend *conv,
+ struct regmap *regmap)
+{
+ WARN_ONCE(1, "converter API is disabled");
+}
+
+static inline int
+__converter_test_pattern_xlate(unsigned int pattern,
+ const struct converter_test_pattern_xlate *xlate,
+ int n_matches)
+{
+ WARN_ONCE(1, "converter API is disabled");
+ return -ENOTSUPP;
+}
+
+static inline struct converter_backend *__must_check
+converter_get(const struct converter_frontend *frontend, const char *name)
+{
+ WARN_ONCE(1, "converter API is disabled");
+ return ERR_PTR(-ENOTSUPP);
+}
+
+static inline int converter_add(struct device *dev,
+ const struct converter_ops *ops)
+{
+ WARN_ONCE(1, "converter API is disabled");
+ return -ENOTSUPP;
+}
+
+static inline void converter_del(struct device *dev)
+{
+ WARN_ONCE(1, "converter API is disabled");
+}
+
+static inline int converter_enable(struct converter_backend *conv)
+{
+ WARN_ONCE(1, "converter API is disabled");
+ return -ENOTSUPP;
+}
+
+static inline void converter_disable(struct converter_backend *conv)
+{
+ WARN_ONCE(1, "converter API is disabled");
+}
+
+static inline int
+converter_test_pattern_set(struct converter_backend *conv,
+ unsigned int chan,
+ enum converter_test_pattern pattern)
+{
+ WARN_ONCE(1, "converter API is disabled");
+ return -ENOTSUPP;
+}
+
+static inline int
+converter_data_format_set(struct converter_backend *conv,
+ unsigned int chan,
+ const struct converter_data_fmt *data)
+{
+ WARN_ONCE(1, "converter API is disabled");
+ return -ENOTSUPP;
+}
+
+static inline int converter_chan_enable(struct converter_backend *conv,
+ unsigned int chan)
+{
+ WARN_ONCE(1, "converter API is disabled");
+ return -ENOTSUPP;
+}
+
+static inline int converter_chan_disable(struct converter_backend *conv,
+ unsigned int chan)
+{
+ WARN_ONCE(1, "converter API is disabled");
+ return -ENOTSUPP;
+}
+
+static inline int converter_iodelay_set(struct converter_backend *conv,
+ unsigned int num_lanes,
+ unsigned int val)
+{
+ WARN_ONCE(1, "converter API is disabled");
+ return -ENOTSUPP;
+}
+
+static inline void
+converter_add_direct_reg_access(struct converter_backend *conv,
+ struct iio_dev *indio_dev)
+{
+ WARN_ONCE(1, "converter API is disabled");
+}
+
+static inline int converter_frontend_add(struct device *dev,
+ const struct frontend_ops *ops)
+{
+ WARN_ONCE(1, "converter API is disabled");
+ return -ENOTSUPP;
+}
+
+static inline void converter_frontend_del(struct device *dev)
+{
+ WARN_ONCE(1, "converter API is disabled");
+}
+
+#endif
+#endif
--
2.41.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [RFC PATCH 1/3] iio: addac: add new converter framework
2023-08-04 14:53 ` [RFC PATCH 1/3] iio: addac: add new " Nuno Sa
@ 2023-08-30 17:02 ` Jonathan Cameron
2023-08-31 9:32 ` Nuno Sá
0 siblings, 1 reply; 19+ messages in thread
From: Jonathan Cameron @ 2023-08-30 17:02 UTC (permalink / raw)
To: Nuno Sa; +Cc: linux-iio, Jonathan Cameron
On Fri, 4 Aug 2023 16:53:39 +0200
Nuno Sa <nuno.sa@analog.com> wrote:
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
Hi Nuno,
One general comment is that you could have stripped this back a fair bit
for ease of understanding. At this stage we don't care about things
like debug or control of test patterns. Bring those in as extra patches.
I haven't fully gotten my head around the ordering constraints on removal.
Are there other users of the component framework that have similar problems?
Also, I don't yet understand how a multiple front end, single backend setup
would work. Or indeed single front end, multiple backend... Maybe we don't
need those cases, but if we want this to be useful beyond adi-axi we
probably at least want an outline of how they work.
Jonathan
> ---
> drivers/iio/addac/converter.c | 547 ++++++++++++++++++++++++++++
> include/linux/iio/addac/converter.h | 485 ++++++++++++++++++++++++
> 2 files changed, 1032 insertions(+)
> create mode 100644 drivers/iio/addac/converter.c
> create mode 100644 include/linux/iio/addac/converter.h
>
> diff --git a/drivers/iio/addac/converter.c b/drivers/iio/addac/converter.c
> new file mode 100644
> index 000000000000..31ac704255ad
> --- /dev/null
> +++ b/drivers/iio/addac/converter.c
> @@ -0,0 +1,547 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Framework to handle complex IIO aggregate devices
> + *
> + * A note on some of the design expectations with regards to lifetimes and
> + * devices bringup/removal.
> + *
> + * The Framework is using, under the wood, the component API which makes it to
> + * easy treat a bunch of devices as one aggregate device. This means that the
> + * complete thing is only brought to live when all the deviced are probed. To do
devices
> + * this, two callbacks are used that should in fact completely replace .probe()
> + * and .remove(). The formers should only be used the minimum needed. Ideally,
I don't follow the sentence in the middle of the line above.
> + * only to call the functions to add and remove frontend annd backend devices.
Spell check...
> + *
> + * It is advised for frontend and backend drivers to use their .remove()
I'd not 'advise' things. I'd say the 'use' them.
> + * callbacks (to use devres API during the frontend and backends initialization).
> + * See the comment in @converter_frontend_bind().
> + *
> + * It is also assumed that converter objects cannot be accessed once one of the
> + * devices of the aggregate device is removed (effectively bringing the all the
bringing all the devices down
> + * devices down). Based on that assumption, these objects are not refcount which
recounted
> + * means accessing them will likely fail miserably.
I hope that doesn't mean there will be no protection. I don't mind if nothing works
but breaking the kernel isn't an option.
> + *
> + * Copyright (C) 2023 Analog Devices Inc.
> + */
> +
> +#define dev_fmt(fmt) "Converter - " fmt
> +
> +#include <linux/component.h>
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/iio/addc/converter.h>
> +#include <linux/iio/iio.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +struct converter_backend {
> + struct list_head entry;
> + struct device *dev;
> + const struct converter_ops *ops;
> + const char *name;
> + void *drvdata;
> +
> + struct regmap *regmap;
> + unsigned int cached_reg_addr;
> +};
> +
> +struct converter_frontend {
> + struct list_head list;
> + const struct frontend_ops *ops;
> + struct device *dev;
> +};
> +static int converter_bind(struct device *dev, struct device *aggregate,
> + void *data)
> +{
> + struct converter_frontend *frontend = dev_get_drvdata(aggregate);
> + struct converter_backend *conv = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = conv->ops->backend_init(conv, dev);
> + if (ret)
> + return ret;
> +
> + list_add_tail(&conv->entry, &frontend->list);
> +
> + return 0;
> +}
> +
> +static void converter_unbind(struct device *dev, struct device *aggregate,
> + void *data)
> +{
> + struct converter_backend *conv = dev_get_drvdata(dev);
> +
> + if (conv->ops->backend_close)
> + conv->ops->backend_close(conv);
> +
> + /* after this point the converter should not be used anymore */
> + converter_set_drvdata(conv, NULL);
> +}
> +
> +static const struct component_ops converter_component_ops = {
> + .bind = converter_bind,
> + .unbind = converter_unbind,
> +};
> +
> +static int converter_frontend_bind(struct device *dev)
> +{
> + struct converter_frontend *frontend = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = component_bind_all(dev, NULL);
> + if (ret)
> + return ret;
> + /*
> + * We open a new group so that we can control when resources are
> + * released and still use device managed (devm_) calls. The expectations
> + * are that on probe, backend resources are allocated first followed by
> + * the frontend resources (where registering the IIO device must happen)
> + * Naturally we want the reverse order on the unbind path and that would
> + * not be possible without opening our own devres group.
> +
> + * Note that the component API also opens it's own devres group when
> + * calling the .bind() callbacks for both the aggregate device
> + * (our frontend) and each of the components (our backends). On the
> + * unbind path, the aggregate .unbind() function is called
> + * (@converter_frontend_unbind()) which should be responsible to tear
> + * down all the components (effectively releasing all the resources
> + * allocated on each component devres group) and only then the aggregate
> + * devres group is released. Hence, the order we want to maintain for
> + * releasing resources would not be satisfied because backend resources
> + * would be freed first. With our own group, we can control when
> + * releasing the resources and we do it before @component_unbind_all().
> + *
> + * This also relies that internally the component API is releasing each
> + * of the component's devres group. That is likely not to change, but
> + * maybe we should not trust it and also open our own groups for backend
> + * devices?!
> + *
> + * Another very important thing to keep in mind is that this is only
> + * valid if frontend and backend driver's are implementing their
> + * .remove() callback to call @converter_frontend_del() and
> + * @converter_backend_del(). Calling those functions from
> + * devm_add_action* and use devm APIs in .frontend_init() and
> + * .backend_init() is not going to work. Not perfect but still better
> + * than having to tear everything down in .frontend_close() and
> + * .backend_close()
That last bit is nasty and will be non obvious to driver authors.
I wonder if we can come up with some means to make it hard to do.
> + */
> + if (!devres_open_group(dev, frontend, GFP_KERNEL))
> + return -ENOMEM;
> +
> + ret = frontend->ops->frontend_init(frontend, dev);
> + if (ret) {
> + devres_release_group(dev, frontend);
> + return ret;
> + }
> +
> + devres_close_group(dev, NULL);
> + return 0;
> +}
> +
> +static void converter_frontend_unbind(struct device *dev)
> +{
> + struct converter_frontend *frontend = dev_get_drvdata(dev);
> +
> + if (frontend->ops->frontend_close)
> + frontend->ops->frontend_close(frontend);
> +
> + devres_release_group(dev, frontend);
> + component_unbind_all(dev, NULL);
> + list_del_init(&frontend->list);
> +}
> +
> +static const struct component_master_ops frontend_component_ops = {
> + .bind = converter_frontend_bind,
> + .unbind = converter_frontend_unbind,
> +};
> diff --git a/include/linux/iio/addac/converter.h b/include/linux/iio/addac/converter.h
> new file mode 100644
> index 000000000000..09d9d491b2b8
> --- /dev/null
> +++ b/include/linux/iio/addac/converter.h
> @@ -0,0 +1,485 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef _CONVERTER_H
> +#define _CONVERTER_H
> +
> +struct converter_frontend;
> +struct converter_backend;
> +struct iio_dev;
> +struct device;
> +struct regmap;
> +
> +enum converter_test_pattern {
> + CONVERTER_PRBS_7,
> + CONVERTER_PRBS_15,
> + CONVERTER_PRBS_23,
> + CONVERTER_PRBS_31,
> + CONVERTER_RAMP_NIBBLE,
> + CONVERTER_RAMP_16,
> + /* vendor specific from 32 */
> + CONVERTER_ADI_PRBS_9A = 32,
> + CONVERTER_ADI_PRBS_23A,
> + CONVERTER_ADI_PRBS_X,
> + CONVERTER_TEST_PATTERN_MAX
> +};
> +
> +enum converter_data_type {
> + CONVERTER_TWOS_COMPLEMENT,
> + CONVERTER_OFFSET_BINARY,
> + CONVERTER_DATA_TYPE_MAX
> +};
> +
> +enum converter_edge {
> + CONVERTER_RISING_EDGE_SAMPLE,
> + CONVERTER_FALLING_EDGE_SAMPLE,
> + CONVERTER_EDGE_MAX
> +};
> +
> +struct converter_chan_status {
> + bool errors;
> +};
> +
> +/**
> + * struct converter_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 converter_data_fmt {
> + enum converter_data_type type;
> + bool sign_extend;
> + bool enable;
> +};
> +
> +/**
> + * struct converter_test_pattern_xlate - Helper struct for test pattern handling
> + * @pattern: Pattern to configure.
> + * @reg_val: Register value for the pattern to configure.
> + */
> +struct converter_test_pattern_xlate {
> + enum converter_test_pattern pattern;
> + unsigned int reg_val;
> +};
> +
> +/**
> + * struct converter_ops - Backend supported operations
> + * @backend_init: Mandatory function to initialize the backend device. It
> + * should be a replacement for .probe() where the latest
just say .probe() again as 'the latest' is a fiddly bit of English
> + * should only have to care about doing @converter_add().
> + * @backend_close: Optional function to tear down the device.
> + * @enable: Enable the backend device.
> + * @disable: Disable the backend device.
> + * @data_format_set: Configure the data format for a specific channel.
> + * @chan_enable: Enable one channel.
> + * @chan_disable: Disable one channel.
> + * @iodelay_set: Controls the IO delay for all the lanes at the interface
> + * (where data is actually transferred between frontend and
> + backend) level.
> + * @test_pattern_set: Set's a test pattern to be transmitted/received by the
> + * backend. Typically useful for debug or interface
> + * purposes calibration.
> + */
> +struct converter_ops {
> + int (*backend_init)(struct converter_backend *conv, struct device *dev);
> + void (*backend_close)(struct converter_backend *conv);
> + int (*enable)(struct converter_backend *conv);
> + void (*disable)(struct converter_backend *conv);
> + int (*data_format_set)(struct converter_backend *conv,
> + unsigned int chan,
> + const struct converter_data_fmt *data);
> + int (*chan_enable)(struct converter_backend *conv, unsigned int chan);
> + int (*chan_disable)(struct converter_backend *conv, unsigned int chan);
> + int (*iodelay_set)(struct converter_backend *conv,
> + unsigned int num_lanes, unsigned int delay);
> + int (*test_pattern_set)(struct converter_backend *conv,
> + unsigned int chan,
> + enum converter_test_pattern pattern);
> + int (*chan_status)(struct converter_backend *conv, unsigned int chan,
> + struct converter_chan_status *status);
> + int (*sample_edge_select)(struct converter_backend *conv,
> + enum converter_edge edge);
> +};
> +
> +/**
> + * struct frontend_ops - Frontend supported operations
> + * @frontend_init: Mandatory function to initialize the frontend device. It
> + * should be a replacement for .probe() where the latest
As above.
> + * should only have to care about doing @frontend_add().
> + * @frontend_close: Optional function to tear down the device.
> + */
> +struct frontend_ops {
> + int (*frontend_init)(struct converter_frontend *frontend,
> + struct device *dev);
> + void (*frontend_close)(struct converter_frontend *frontend);
> +};
> +
> +/**
> + * converter_test_pattern_xlate() - Helper macro for translatting test patterns
> + * @pattern: Pattern to translate.
> + * @xlate: List of @struct converter_test_pattern_xlate pairs.
> + *
> + * Simple helper to match a supported pattern and get the register value. Should
> + * only be called by backend devices. Automatically computes the number of
> + * @xlate entries.
> + */
> +#define converter_test_pattern_xlate(pattern, xlate) \
> + __converter_test_pattern_xlate(pattern, xlate, ARRAY_SIZE(xlate));
> +
> +#if IS_ENABLED(CONFIG_IIO_CONVERTER)
Why? I'd expect any driver that uses this framework to be useless without
it, so we shouldn't need protections. Handle that with Kconfig select / depends
> +
> +/**
> + * converter_get_drvdata - Get driver private data
> + * @conv: Converter device.
Comments should be next to implementation (I got this wrong in original IIO
code and still haven't pushed them all down). One reason is that people often
change the internals without realizing there is a comment on them in a header
that also needs updating. Much harder to do that if it's right in front of
you in the c file.
> + */
> +void *converter_get_drvdata(const struct converter_backend *conv);
> +
> +/**
> + * converter_set_drvdata - Set driver private data
> + * @conv: Converter device.
> + * @drvdata: Driver private data.
> + */
> +void converter_set_drvdata(struct converter_backend *conv, void *drvdata);
> +
> +/**
> + * converter_set_regmap - Add a regmap object to a converter
> + * @conv: Converter device.
> + * @regmap: Regmap object.
> + */
> +void converter_set_regmap(struct converter_backend *conv,
> + struct regmap *regmap);
> +
> +/**
> + * __converter_test_pattern_xlate - Helper macro for translatting test patterns
> + * @pattern: Pattern to translate.
> + * @xlate: List of @struct converter_test_pattern_xlate pairs.
> + * @n_matches: Number of entries in @xlate.
> + *
> + * Simple helper to match a supported pattern and get the register value. Should
> + * only be called by backend devices.
> + */
> +int __converter_test_pattern_xlate(unsigned int pattern,
> + const struct converter_test_pattern_xlate *xlate,
> + int n_matches);
> +
> +/**
> + *
> + */
> +int converter_add(struct device *dev, const struct converter_ops *ops);
> +
> +/**
> + * converter_del - Remove the converter device
> + * @dev: device to remove from the aggregate
> + *
> + * Removes the converter from the aggregate device. This tears down the frontend
> + * and all the converters.
> + *
> + * Ideally, this should be called from the backend driver .remove() callback.
> + * This means that all the converters (and the frontend) will be tear down before
> + * running any specific devres cleanup (at the driver core level). What this all
> + * means is that we can use devm_ apis in @backend_init() and being sure those
> + * resources will be released after the backend resources and before any devm_*
> + * used in @probe(). If that is not the case, one should likely not use any
> + * devm_ API in @backend_init(). That means .backend_close() should be
> + * provided to do all the necessary cleanups.
> + */
> +void converter_del(struct device *dev);
> +
> +/**
> + * converter_enable - Enable the device
> + * @conv: Converter device.
> + *
> + * Enables the backend device.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int converter_enable(struct converter_backend *conv);
> +
> +/**
> + * converter_disable - Disable the device
> + * @conv: Converter device.
> + *
> + * Disables the backend device.
> + */
> +void converter_disable(struct converter_backend *conv);
> +
> +/**
> + * converter_test_pattern_set - Set a test pattern
> + * @conv: Converter device.
> + * @chan: Channel number.
> + * @pattern: Pattern to set.
> + *
> + * Set's a test pattern to be transmitted/received by the backend. Typically
> + * useful for debug or interface calibration purposes. A backend driver can
> + * call the @converter_test_pattern_xlate() helper to validate the pattern
> + * (given an array of @struct converter_test_pattern_xlate).
> + *
> + * Note that some patterns might be frontend specific. I.e, as far as the
> + * backend is concerned the pattern is valid (from a register point of view) but
> + * the actual support for the pattern is not implemented in the device for this
> + * specific frontend. It's up to the frontend to ask for a proper pattern
> + * (as it should know better).
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int converter_test_pattern_set(struct converter_backend *conv,
> + unsigned int chan,
> + enum converter_test_pattern pattern);
> +
> +int converter_chan_status_get(struct converter_backend *conv,
> + unsigned int chan,
> + struct converter_chan_status *status);
> +
> +/**
> + * converter_data_format_set - Configure the data format
> + * @conv: Converter device.
> + * @chan: Channel number.
> + * @data: Data format.
> + *
> + * Properly configure a channel with respect to the expected data format. A
Configure a channel ...
We won't do it improperly ;)
> + * @struct converter_data_fmt must be passed with the settings.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int converter_data_format_set(struct converter_backend *conv,
> + unsigned int chan,
> + const struct converter_data_fmt *data);
> +
> +int converter_sample_edge_select(struct converter_backend *conv,
> + enum converter_edge edge);
> +
> +static inline int
> +converter_sample_on_falling_edge(struct converter_backend *conv)
> +{
> + return converter_sample_edge_select(conv, CONVERTER_RISING_EDGE_SAMPLE);
> +}
> +
> +static inline int
> +converter_sample_on_rising_edge(struct converter_backend *conv)
> +{
> + return converter_sample_edge_select(conv, CONVERTER_FALLING_EDGE_SAMPLE);
> +}
> +
> +/**
> + * converter_chan_enable - Enable a backend channel
> + * @conv: Converter device.
> + * @chan: Channel number.
> + *
> + * Enables a channel on the backend device.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int converter_chan_enable(struct converter_backend *conv, unsigned int chan);
> +
> +/**
> + * converter_chan_disable - Disable a backend channel
> + * @conv: Converter device.
> + * @chan: Channel number.
> + *
> + * Disables a channel on the backend device.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int converter_chan_disable(struct converter_backend *conv, unsigned int chan);
> +
> +/**
> + * converter_iodelay_set - Set's the backend data interface IO delay
> + * @conv: Converter device.
> + * @num_lanes: Number of lanes in the data interface.
> + * @delay: Delay to set.
> + *
> + * Controls the IO delay for all the lanes at the data interface (where data is
> + * actually transferred between frontend and backend) level.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int converter_iodelay_set(struct converter_backend *conv,
> + unsigned int num_lanes, unsigned int delay);
> +
> +/**
> + * converter_frontend_del - Remove the frontend device
> + * @dev: Device to remove from the aggregate
> + *
> + * Removes the frontend from the aggregate device. This tears down the frontend
> + * and all the converters.
> + *
> + * Ideally, this should be called from the frontend driver .remove() callback.
> + * This means that all the converters (and the frontend) will be tear down
torn down
> + * before running any specific devres cleanup (at the driver core level). What
> + * this all means is that we can use devm_ apis in .frontend_init() and being
> + * sure those resources will be released after the backend resources and before
> + * any devm_* used in .probe(). If that is not the case, one should likely not
> + * use any devm_ API in .frontend_init(). That means .frontend_close() should be
> + * provided to do all the necessary cleanups.
You can force a driver remove to tear down another driver binding first though it all
gets fiddly. Take a look at how device_release_driver() is used. May well not
help you here though - I've not thought it through properly.
> + */
> +void converter_frontend_del(struct device *dev);
> +
> +/**
> + * converter_frontend_add - Allocate and add a frontend device
> + * @dev: Device to allocate frontend for.
> + * @ops: Frontend callbacks.
> + *
> + * This allocates the frontend device and looks for all converters needed
> + * so that, when they are available, all of the devices in the aggregate can be
> + * initialized.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int converter_frontend_add(struct device *dev, const struct frontend_ops *ops);
> +
> +/**
> + * converter_get - Get a converter object
> + * @frontend: Frontend device.
> + * @name: Converter name.
> + *
> + * Get's a pointer to a converter device. If name is NULL, then it is assumed
> + * that only one backend device is bond with the frontend and the first element
> + * in the list is retrieved. Should only be called from the .frontend_init()
> + * callback.
> + *
> + * RETURNS:
> + * A converter pointer, negative error pointer otherwise.
> + */
> +struct converter_backend *__must_check
> +converter_get(const struct converter_frontend *frontend, const char *name);
> +
> +/**
> + * converter_add_direct_reg_access - Add debugfs direct register access
> + * @conv: Coverter device
> + * @indio_dev: IIO device
> + *
> + * This is analogous to the typical IIO direct register access in debugfs. The
> + * extra converter file will be added in the same debugs dir as @indio_dev.
> + * Moreover, if @conv->name is NULL, the file will be called
> + * converter_direct_reg_access. Otherwise, will be
> + * @conv->name_converter_direct_reg_access.
> + */
> +void converter_add_direct_reg_access(struct converter_backend *conv,
> + struct iio_dev *indio_dev);
> +
...
/
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC PATCH 1/3] iio: addac: add new converter framework
2023-08-30 17:02 ` Jonathan Cameron
@ 2023-08-31 9:32 ` Nuno Sá
2023-09-03 10:56 ` Jonathan Cameron
0 siblings, 1 reply; 19+ messages in thread
From: Nuno Sá @ 2023-08-31 9:32 UTC (permalink / raw)
To: Jonathan Cameron, Nuno Sa; +Cc: linux-iio, Jonathan Cameron
On Wed, 2023-08-30 at 18:02 +0100, Jonathan Cameron wrote:
> On Fri, 4 Aug 2023 16:53:39 +0200
> Nuno Sa <nuno.sa@analog.com> wrote:
>
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
>
> Hi Nuno,
>
Hi Jonathan,
Thanks for the initial review...
>
> One general comment is that you could have stripped this back a fair bit
> for ease of understanding. At this stage we don't care about things
> like debug or control of test patterns. Bring those in as extra patches.
>
Agreed... As I mentioned (I think) in the cover, I made the RFC bigger than needed to
kind of showcase how we can properly configure the hdl core to support things
(interface calibration) that were very hard to do with the current implementation.
I'll make sure to add the minimum needed API to accommodate what we have right now.
> I haven't fully gotten my head around the ordering constraints on removal.
> Are there other users of the component framework that have similar problems?
>
My understanding on the component API is that one should do all the tear down in the
.unbind() callback. As usual, I can see some drivers not really doing that.
> Also, I don't yet understand how a multiple front end, single backend setup
> would work. Or indeed single front end, multiple backend... Maybe we don't
> need those cases, but if we want this to be useful beyond adi-axi we
> probably at least want an outline of how they work.
>
Indeed we can have multiple (and we have it out of tree) backends on one frontend.
Think on an ADC/DAC with fairly complex data path with more than one
channel/interface (CMOS, LVDS, etc). Typically, in those case, each of the interface
will be connected to an instance of the hdl core (the backend).
> Jonathan
>
> > ---
> > drivers/iio/addac/converter.c | 547 ++++++++++++++++++++++++++++
> > include/linux/iio/addac/converter.h | 485 ++++++++++++++++++++++++
> > 2 files changed, 1032 insertions(+)
> > create mode 100644 drivers/iio/addac/converter.c
> > create mode 100644 include/linux/iio/addac/converter.h
> >
> > diff --git a/drivers/iio/addac/converter.c b/drivers/iio/addac/converter.c
> > new file mode 100644
> > index 000000000000..31ac704255ad
> > --- /dev/null
> > +++ b/drivers/iio/addac/converter.c
> > @@ -0,0 +1,547 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Framework to handle complex IIO aggregate devices
> > + *
> > + * A note on some of the design expectations with regards to lifetimes and
> > + * devices bringup/removal.
> > + *
> > + * The Framework is using, under the wood, the component API which makes it to
> > + * easy treat a bunch of devices as one aggregate device. This means that the
> > + * complete thing is only brought to live when all the deviced are probed. To do
>
> devices
>
> > + * this, two callbacks are used that should in fact completely replace .probe()
> > + * and .remove(). The formers should only be used the minimum needed. Ideally,
>
> I don't follow the sentence in the middle of the line above.
>
> > + * only to call the functions to add and remove frontend annd backend devices.
> Spell check...
>
> > + *
> > + * It is advised for frontend and backend drivers to use their .remove()
>
> I'd not 'advise' things. I'd say the 'use' them.
>
> > + * callbacks (to use devres API during the frontend and backends
> > initialization).
> > + * See the comment in @converter_frontend_bind().
> > + *
> > + * It is also assumed that converter objects cannot be accessed once one of the
> > + * devices of the aggregate device is removed (effectively bringing the all the
>
> bringing all the devices down
>
> > + * devices down). Based on that assumption, these objects are not refcount which
>
> recounted
>
> > + * means accessing them will likely fail miserably.
>
> I hope that doesn't mean there will be no protection. I don't mind if nothing
> works
> but breaking the kernel isn't an option.
>
Hmm, well, you'll have a use after free... But one will have to be creative to use
one of these objects after releasing the device from the driver (on the unbind path).
And here we don't have any interaction with chardevs, etc which might keep references
to devices even after unbind.
The only place where I can see someone doing it wrong is from a frontend driver if
for some reason (that I cannot think of now) we need to keep references/use 'struct
converter' after .frontend_close() is called. In that case and if the backend driver
was the one being removed/unbind, the converter object will effectively be freed (as
it was allocated with devres) and we are left with a possible use after free. But
that would be a very strange usecase to be missed in review (I hope :)).
We can always refcount the converters (not sure if we need to do it for frontend
devices). Sure, drivers can still screw up but at least in that case, the framework
is not to blame :).
> > + *
> > + * Copyright (C) 2023 Analog Devices Inc.
> > + */
> > +
> > +#define dev_fmt(fmt) "Converter - " fmt
> > +
> > +#include <linux/component.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/iio/addc/converter.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/list.h>
> > +#include <linux/module.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +
> > +struct converter_backend {
> > + struct list_head entry;
> > + struct device *dev;
> > + const struct converter_ops *ops;
> > + const char *name;
> > + void *drvdata;
> > +
> > + struct regmap *regmap;
> > + unsigned int cached_reg_addr;
> > +};
> > +
> > +struct converter_frontend {
> > + struct list_head list;
> > + const struct frontend_ops *ops;
> > + struct device *dev;
> > +};
>
>
>
> > +static int converter_bind(struct device *dev, struct device *aggregate,
> > + void *data)
> > +{
> > + struct converter_frontend *frontend = dev_get_drvdata(aggregate);
> > + struct converter_backend *conv = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = conv->ops->backend_init(conv, dev);
> > + if (ret)
> > + return ret;
> > +
> > + list_add_tail(&conv->entry, &frontend->list);
> > +
> > + return 0;
> > +}
> > +
> > +static void converter_unbind(struct device *dev, struct device *aggregate,
> > + void *data)
> > +{
> > + struct converter_backend *conv = dev_get_drvdata(dev);
> > +
> > + if (conv->ops->backend_close)
> > + conv->ops->backend_close(conv);
> > +
> > + /* after this point the converter should not be used anymore */
> > + converter_set_drvdata(conv, NULL);
> > +}
> > +
> > +static const struct component_ops converter_component_ops = {
> > + .bind = converter_bind,
> > + .unbind = converter_unbind,
> > +};
> > +
> > +static int converter_frontend_bind(struct device *dev)
> > +{
> > + struct converter_frontend *frontend = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = component_bind_all(dev, NULL);
> > + if (ret)
> > + return ret;
> > + /*
> > + * We open a new group so that we can control when resources are
> > + * released and still use device managed (devm_) calls. The expectations
> > + * are that on probe, backend resources are allocated first followed by
> > + * the frontend resources (where registering the IIO device must happen)
> > + * Naturally we want the reverse order on the unbind path and that would
> > + * not be possible without opening our own devres group.
> > +
> > + * Note that the component API also opens it's own devres group when
> > + * calling the .bind() callbacks for both the aggregate device
> > + * (our frontend) and each of the components (our backends). On the
> > + * unbind path, the aggregate .unbind() function is called
> > + * (@converter_frontend_unbind()) which should be responsible to tear
> > + * down all the components (effectively releasing all the resources
> > + * allocated on each component devres group) and only then the aggregate
> > + * devres group is released. Hence, the order we want to maintain for
> > + * releasing resources would not be satisfied because backend resources
> > + * would be freed first. With our own group, we can control when
> > + * releasing the resources and we do it before @component_unbind_all().
> > + *
> > + * This also relies that internally the component API is releasing each
> > + * of the component's devres group. That is likely not to change, but
> > + * maybe we should not trust it and also open our own groups for backend
> > + * devices?!
> > + *
> > + * Another very important thing to keep in mind is that this is only
> > + * valid if frontend and backend driver's are implementing their
> > + * .remove() callback to call @converter_frontend_del() and
> > + * @converter_backend_del(). Calling those functions from
> > + * devm_add_action* and use devm APIs in .frontend_init() and
> > + * .backend_init() is not going to work. Not perfect but still better
> > + * than having to tear everything down in .frontend_close() and
> > + * .backend_close()
>
> That last bit is nasty and will be non obvious to driver authors.
>
> I wonder if we can come up with some means to make it hard to do.
>
Yeah, I agree. The alternative is to always bring everything down in
.frontend_close() and .backend_close(). But that can also be prone to subtle bugs
because it's easy to mess up the ordering when not using devres.
So, at this point, I cannot really think on a perfect solution rather than keeping
some rules like (assuming we keep the logic we have now):
* Using devres on frontend|backend_init() only when .remove() is provided on the
driver.
* No mixes of devres and .frontend|backend_close()
But yeah, would be nice if we could come up with something to make it more obvious to
driver authors.
We might be able to detect that converter_backend_del() and converter_frontend_del()
are under devres while no .frontend|backend_close() is being given. I guess that
could be a valid indicator of likely misusage.
Or even better (but I'm not sure it's doable with the current devres API), detecting
that converter_backend_del() or converter_frontend_del() are under devres while more
resources are also allocated in our specific opened groups. That would always be a
problem (I think) because the only way for the _del() functions to be under devres is
if someone added them (from .probe) with devm_add_action() which means that tearing
down the aggregate will happen after some resources (which were allocated in the
_init() function) are already freed (as even with new groups, devres will remove
things on the reverse order). And that would defenitely be problematic. And, in fact,
is the whole reason why I have the .del() functions on .remove() (so, tearing down
the aggregate device is the first thing to happen and resources are freed in the
reverse order they were allocated).
Other thought would be some generic helper macros to use in these type of drivers so
a .remove() callback is always added to remove the components.
Anyways, even the above might be tricky enough to not include it. I'll give it some
thought.
> > + */
> > + if (!devres_open_group(dev, frontend, GFP_KERNEL))
> > + return -ENOMEM;
> > +
> > + ret = frontend->ops->frontend_init(frontend, dev);
> > + if (ret) {
> > + devres_release_group(dev, frontend);
> > + return ret;
> > + }
> > +
> > + devres_close_group(dev, NULL);
> > + return 0;
> > +}
> > +
> > +static void converter_frontend_unbind(struct device *dev)
> > +{
> > + struct converter_frontend *frontend = dev_get_drvdata(dev);
> > +
> > + if (frontend->ops->frontend_close)
> > + frontend->ops->frontend_close(frontend);
> > +
> > + devres_release_group(dev, frontend);
> > + component_unbind_all(dev, NULL);
> > + list_del_init(&frontend->list);
> > +}
> > +
> > +static const struct component_master_ops frontend_component_ops = {
> > + .bind = converter_frontend_bind,
> > + .unbind = converter_frontend_unbind,
> > +};
>
>
> > diff --git a/include/linux/iio/addac/converter.h
> > b/include/linux/iio/addac/converter.h
> > new file mode 100644
> > index 000000000000..09d9d491b2b8
> > --- /dev/null
> > +++ b/include/linux/iio/addac/converter.h
> > @@ -0,0 +1,485 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef _CONVERTER_H
> > +#define _CONVERTER_H
> > +
> > +struct converter_frontend;
> > +struct converter_backend;
> > +struct iio_dev;
> > +struct device;
> > +struct regmap;
> > +
> > +enum converter_test_pattern {
> > + CONVERTER_PRBS_7,
> > + CONVERTER_PRBS_15,
> > + CONVERTER_PRBS_23,
> > + CONVERTER_PRBS_31,
> > + CONVERTER_RAMP_NIBBLE,
> > + CONVERTER_RAMP_16,
> > + /* vendor specific from 32 */
> > + CONVERTER_ADI_PRBS_9A = 32,
> > + CONVERTER_ADI_PRBS_23A,
> > + CONVERTER_ADI_PRBS_X,
> > + CONVERTER_TEST_PATTERN_MAX
> > +};
> > +
> > +enum converter_data_type {
> > + CONVERTER_TWOS_COMPLEMENT,
> > + CONVERTER_OFFSET_BINARY,
> > + CONVERTER_DATA_TYPE_MAX
> > +};
> > +
> > +enum converter_edge {
> > + CONVERTER_RISING_EDGE_SAMPLE,
> > + CONVERTER_FALLING_EDGE_SAMPLE,
> > + CONVERTER_EDGE_MAX
> > +};
> > +
> > +struct converter_chan_status {
> > + bool errors;
> > +};
> > +
> > +/**
> > + * struct converter_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 converter_data_fmt {
> > + enum converter_data_type type;
> > + bool sign_extend;
> > + bool enable;
> > +};
> > +
> > +/**
> > + * struct converter_test_pattern_xlate - Helper struct for test pattern handling
> > + * @pattern: Pattern to configure.
> > + * @reg_val: Register value for the pattern to configure.
> > + */
> > +struct converter_test_pattern_xlate {
> > + enum converter_test_pattern pattern;
> > + unsigned int reg_val;
> > +};
> > +
> > +/**
> > + * struct converter_ops - Backend supported operations
> > + * @backend_init: Mandatory function to initialize the backend device. It
> > + * should be a replacement for .probe() where the latest
>
> just say .probe() again as 'the latest' is a fiddly bit of English
>
> > + * should only have to care about doing @converter_add().
> > + * @backend_close: Optional function to tear down the device.
> > + * @enable: Enable the backend device.
> > + * @disable: Disable the backend device.
> > + * @data_format_set: Configure the data format for a specific channel.
> > + * @chan_enable: Enable one channel.
> > + * @chan_disable: Disable one channel.
> > + * @iodelay_set: Controls the IO delay for all the lanes at the interface
> > + * (where data is actually transferred between frontend and
> > + backend) level.
> > + * @test_pattern_set: Set's a test pattern to be transmitted/received by the
> > + * backend. Typically useful for debug or interface
> > + * purposes calibration.
> > + */
> > +struct converter_ops {
> > + int (*backend_init)(struct converter_backend *conv, struct device *dev);
> > + void (*backend_close)(struct converter_backend *conv);
> > + int (*enable)(struct converter_backend *conv);
> > + void (*disable)(struct converter_backend *conv);
> > + int (*data_format_set)(struct converter_backend *conv,
> > + unsigned int chan,
> > + const struct converter_data_fmt *data);
> > + int (*chan_enable)(struct converter_backend *conv, unsigned int chan);
> > + int (*chan_disable)(struct converter_backend *conv, unsigned int chan);
> > + int (*iodelay_set)(struct converter_backend *conv,
> > + unsigned int num_lanes, unsigned int delay);
> > + int (*test_pattern_set)(struct converter_backend *conv,
> > + unsigned int chan,
> > + enum converter_test_pattern pattern);
> > + int (*chan_status)(struct converter_backend *conv, unsigned int chan,
> > + struct converter_chan_status *status);
> > + int (*sample_edge_select)(struct converter_backend *conv,
> > + enum converter_edge edge);
> > +};
> > +
> > +/**
> > + * struct frontend_ops - Frontend supported operations
> > + * @frontend_init: Mandatory function to initialize the frontend device. It
> > + * should be a replacement for .probe() where the latest
>
> As above.
>
>
> > + * should only have to care about doing @frontend_add().
> > + * @frontend_close: Optional function to tear down the device.
> > + */
> > +struct frontend_ops {
> > + int (*frontend_init)(struct converter_frontend *frontend,
> > + struct device *dev);
> > + void (*frontend_close)(struct converter_frontend *frontend);
> > +};
> > +
> > +/**
> > + * converter_test_pattern_xlate() - Helper macro for translatting test patterns
> > + * @pattern: Pattern to translate.
> > + * @xlate: List of @struct converter_test_pattern_xlate pairs.
> > + *
> > + * Simple helper to match a supported pattern and get the register value. Should
> > + * only be called by backend devices. Automatically computes the number of
> > + * @xlate entries.
> > + */
> > +#define converter_test_pattern_xlate(pattern, xlate) \
> > + __converter_test_pattern_xlate(pattern, xlate, ARRAY_SIZE(xlate));
> > +
> > +#if IS_ENABLED(CONFIG_IIO_CONVERTER)
>
> Why? I'd expect any driver that uses this framework to be useless without
> it, so we shouldn't need protections. Handle that with Kconfig select / depends
>
Well, we do have cases of frontends that might be used in standalone mode (I mean,
with no backend device) or with the backend connected. But alright, I will keep
things simple for now and let's take care if such case ever get's upstream (hopefully
they eventual do :)).
> > +
> > +/**
> > + * converter_get_drvdata - Get driver private data
> > + * @conv: Converter device.
>
> Comments should be next to implementation (I got this wrong in original IIO
> code and still haven't pushed them all down). One reason is that people often
> change the internals without realizing there is a comment on them in a header
> that also needs updating. Much harder to do that if it's right in front of
> you in the c file.
>
Ohh I didn't realised that but makes sense...
> > + */
> > +void *converter_get_drvdata(const struct converter_backend *conv);
> > +
> > +/**
> > + * converter_set_drvdata - Set driver private data
> > + * @conv: Converter device.
> > + * @drvdata: Driver private data.
> > + */
> > +void converter_set_drvdata(struct converter_backend *conv, void *drvdata);
> > +
> > +/**
> > + * converter_set_regmap - Add a regmap object to a converter
> > + * @conv: Converter device.
> > + * @regmap: Regmap object.
> > + */
> > +void converter_set_regmap(struct converter_backend *conv,
> > + struct regmap *regmap);
> > +
> > +/**
> > + * __converter_test_pattern_xlate - Helper macro for translatting test patterns
> > + * @pattern: Pattern to translate.
> > + * @xlate: List of @struct converter_test_pattern_xlate pairs.
> > + * @n_matches: Number of entries in @xlate.
> > + *
> > + * Simple helper to match a supported pattern and get the register value. Should
> > + * only be called by backend devices.
> > + */
> > +int __converter_test_pattern_xlate(unsigned int pattern,
> > + const struct converter_test_pattern_xlate
> > *xlate,
> > + int n_matches);
> > +
> > +/**
> > + *
> > + */
> > +int converter_add(struct device *dev, const struct converter_ops *ops);
> > +
> > +/**
> > + * converter_del - Remove the converter device
> > + * @dev: device to remove from the aggregate
> > + *
> > + * Removes the converter from the aggregate device. This tears down the frontend
> > + * and all the converters.
> > + *
> > + * Ideally, this should be called from the backend driver .remove() callback.
> > + * This means that all the converters (and the frontend) will be tear down
> > before
> > + * running any specific devres cleanup (at the driver core level). What this all
> > + * means is that we can use devm_ apis in @backend_init() and being sure those
> > + * resources will be released after the backend resources and before any devm_*
> > + * used in @probe(). If that is not the case, one should likely not use any
> > + * devm_ API in @backend_init(). That means .backend_close() should be
> > + * provided to do all the necessary cleanups.
> > + */
> > +void converter_del(struct device *dev);
> > +
> > +/**
> > + * converter_enable - Enable the device
> > + * @conv: Converter device.
> > + *
> > + * Enables the backend device.
> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> > + */
> > +int converter_enable(struct converter_backend *conv);
> > +
> > +/**
> > + * converter_disable - Disable the device
> > + * @conv: Converter device.
> > + *
> > + * Disables the backend device.
> > + */
> > +void converter_disable(struct converter_backend *conv);
> > +
> > +/**
> > + * converter_test_pattern_set - Set a test pattern
> > + * @conv: Converter device.
> > + * @chan: Channel number.
> > + * @pattern: Pattern to set.
> > + *
> > + * Set's a test pattern to be transmitted/received by the backend. Typically
> > + * useful for debug or interface calibration purposes. A backend driver can
> > + * call the @converter_test_pattern_xlate() helper to validate the pattern
> > + * (given an array of @struct converter_test_pattern_xlate).
> > + *
> > + * Note that some patterns might be frontend specific. I.e, as far as the
> > + * backend is concerned the pattern is valid (from a register point of view) but
> > + * the actual support for the pattern is not implemented in the device for this
> > + * specific frontend. It's up to the frontend to ask for a proper pattern
> > + * (as it should know better).
> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> > + */
> > +int converter_test_pattern_set(struct converter_backend *conv,
> > + unsigned int chan,
> > + enum converter_test_pattern pattern);
> > +
> > +int converter_chan_status_get(struct converter_backend *conv,
> > + unsigned int chan,
> > + struct converter_chan_status *status);
> > +
> > +/**
> > + * converter_data_format_set - Configure the data format
> > + * @conv: Converter device.
> > + * @chan: Channel number.
> > + * @data: Data format.
> > + *
> > + * Properly configure a channel with respect to the expected data format. A
> Configure a channel ...
>
> We won't do it improperly ;)
>
> > + * @struct converter_data_fmt must be passed with the settings.
> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> > + */
> > +int converter_data_format_set(struct converter_backend *conv,
> > + unsigned int chan,
> > + const struct converter_data_fmt *data);
> > +
> > +int converter_sample_edge_select(struct converter_backend *conv,
> > + enum converter_edge edge);
> > +
> > +static inline int
> > +converter_sample_on_falling_edge(struct converter_backend *conv)
> > +{
> > + return converter_sample_edge_select(conv, CONVERTER_RISING_EDGE_SAMPLE);
> > +}
> > +
> > +static inline int
> > +converter_sample_on_rising_edge(struct converter_backend *conv)
> > +{
> > + return converter_sample_edge_select(conv, CONVERTER_FALLING_EDGE_SAMPLE);
> > +}
> > +
> > +/**
> > + * converter_chan_enable - Enable a backend channel
> > + * @conv: Converter device.
> > + * @chan: Channel number.
> > + *
> > + * Enables a channel on the backend device.
> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> > + */
> > +int converter_chan_enable(struct converter_backend *conv, unsigned int chan);
> > +
> > +/**
> > + * converter_chan_disable - Disable a backend channel
> > + * @conv: Converter device.
> > + * @chan: Channel number.
> > + *
> > + * Disables a channel on the backend device.
> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> > + */
> > +int converter_chan_disable(struct converter_backend *conv, unsigned int chan);
> > +
> > +/**
> > + * converter_iodelay_set - Set's the backend data interface IO delay
> > + * @conv: Converter device.
> > + * @num_lanes: Number of lanes in the data interface.
> > + * @delay: Delay to set.
> > + *
> > + * Controls the IO delay for all the lanes at the data interface (where data is
> > + * actually transferred between frontend and backend) level.
> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> > + */
> > +int converter_iodelay_set(struct converter_backend *conv,
> > + unsigned int num_lanes, unsigned int delay);
> > +
> > +/**
> > + * converter_frontend_del - Remove the frontend device
> > + * @dev: Device to remove from the aggregate
> > + *
> > + * Removes the frontend from the aggregate device. This tears down the frontend
> > + * and all the converters.
> > + *
> > + * Ideally, this should be called from the frontend driver .remove() callback.
> > + * This means that all the converters (and the frontend) will be tear down
> torn down
> > + * before running any specific devres cleanup (at the driver core level). What
> > + * this all means is that we can use devm_ apis in .frontend_init() and being
> > + * sure those resources will be released after the backend resources and before
> > + * any devm_* used in .probe(). If that is not the case, one should likely not
> > + * use any devm_ API in .frontend_init(). That means .frontend_close() should be
> > + * provided to do all the necessary cleanups.
>
> You can force a driver remove to tear down another driver binding first though it
> all
> gets fiddly. Take a look at how device_release_driver() is used. May well not
> help you here though - I've not thought it through properly.
>
Yeah, I know but I don't think it would help us here or even be correct and I think
the problem would be the same. I mean, we would still need to call
converter_frontend_del() or converter_backend_del() from the respective .remove()
callbacks.
BTW, thanks for all the English fixes and help :)
- Nuno Sá
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC PATCH 1/3] iio: addac: add new converter framework
2023-08-31 9:32 ` Nuno Sá
@ 2023-09-03 10:56 ` Jonathan Cameron
2023-09-04 14:14 ` Nuno Sá
0 siblings, 1 reply; 19+ messages in thread
From: Jonathan Cameron @ 2023-09-03 10:56 UTC (permalink / raw)
To: Nuno Sá; +Cc: Jonathan Cameron, Nuno Sa, linux-iio
On Thu, 31 Aug 2023 11:32:54 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Wed, 2023-08-30 at 18:02 +0100, Jonathan Cameron wrote:
> > On Fri, 4 Aug 2023 16:53:39 +0200
> > Nuno Sa <nuno.sa@analog.com> wrote:
> >
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> >
> > Hi Nuno,
> >
>
> Hi Jonathan,
>
> Thanks for the initial review...
>
> >
> > One general comment is that you could have stripped this back a fair bit
> > for ease of understanding. At this stage we don't care about things
> > like debug or control of test patterns. Bring those in as extra patches.
> >
>
> Agreed... As I mentioned (I think) in the cover, I made the RFC bigger than needed to
> kind of showcase how we can properly configure the hdl core to support things
> (interface calibration) that were very hard to do with the current implementation.
> I'll make sure to add the minimum needed API to accommodate what we have right now.
>
> > I haven't fully gotten my head around the ordering constraints on removal.
> > Are there other users of the component framework that have similar problems?
> >
>
> My understanding on the component API is that one should do all the tear down in the
> .unbind() callback. As usual, I can see some drivers not really doing that.
>
> > Also, I don't yet understand how a multiple front end, single backend setup
> > would work. Or indeed single front end, multiple backend... Maybe we don't
> > need those cases, but if we want this to be useful beyond adi-axi we
> > probably at least want an outline of how they work.
> >
>
> Indeed we can have multiple (and we have it out of tree) backends on one frontend.
> Think on an ADC/DAC with fairly complex data path with more than one
> channel/interface (CMOS, LVDS, etc). Typically, in those case, each of the interface
> will be connected to an instance of the hdl core (the backend).
That might work out for your case, but not the stm32 one where I think we can end
up with interleaved data from two front ends in the same buffer...
>
> > Jonathan
> >
> > > ---
> > > drivers/iio/addac/converter.c | 547 ++++++++++++++++++++++++++++
> > > include/linux/iio/addac/converter.h | 485 ++++++++++++++++++++++++
> > > 2 files changed, 1032 insertions(+)
> > > create mode 100644 drivers/iio/addac/converter.c
> > > create mode 100644 include/linux/iio/addac/converter.h
> > >
> > > diff --git a/drivers/iio/addac/converter.c b/drivers/iio/addac/converter.c
> > > new file mode 100644
> > > index 000000000000..31ac704255ad
> > > --- /dev/null
> > > +++ b/drivers/iio/addac/converter.c
> > > @@ -0,0 +1,547 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Framework to handle complex IIO aggregate devices
> > > + *
> > > + * A note on some of the design expectations with regards to lifetimes and
> > > + * devices bringup/removal.
> > > + *
> > > + * The Framework is using, under the wood, the component API which makes it to
> > > + * easy treat a bunch of devices as one aggregate device. This means that the
> > > + * complete thing is only brought to live when all the deviced are probed. To do
> >
> > devices
> >
> > > + * this, two callbacks are used that should in fact completely replace .probe()
> > > + * and .remove(). The formers should only be used the minimum needed. Ideally,
> >
> > I don't follow the sentence in the middle of the line above.
> >
> > > + * only to call the functions to add and remove frontend annd backend devices.
> > Spell check...
> >
> > > + *
> > > + * It is advised for frontend and backend drivers to use their .remove()
> >
> > I'd not 'advise' things. I'd say the 'use' them.
> >
> > > + * callbacks (to use devres API during the frontend and backends
> > > initialization).
> > > + * See the comment in @converter_frontend_bind().
> > > + *
> > > + * It is also assumed that converter objects cannot be accessed once one of the
> > > + * devices of the aggregate device is removed (effectively bringing the all the
> >
> > bringing all the devices down
> >
> > > + * devices down). Based on that assumption, these objects are not refcount which
> >
> > recounted
> >
> > > + * means accessing them will likely fail miserably.
> >
> > I hope that doesn't mean there will be no protection. I don't mind if nothing
> > works
> > but breaking the kernel isn't an option.
> >
>
> Hmm, well, you'll have a use after free... But one will have to be creative to use
> one of these objects after releasing the device from the driver (on the unbind path).
> And here we don't have any interaction with chardevs, etc which might keep references
> to devices even after unbind.
>
> The only place where I can see someone doing it wrong is from a frontend driver if
> for some reason (that I cannot think of now) we need to keep references/use 'struct
> converter' after .frontend_close() is called. In that case and if the backend driver
> was the one being removed/unbind, the converter object will effectively be freed (as
> it was allocated with devres) and we are left with a possible use after free. But
> that would be a very strange usecase to be missed in review (I hope :)).
>
> We can always refcount the converters (not sure if we need to do it for frontend
> devices). Sure, drivers can still screw up but at least in that case, the framework
> is not to blame :).
If the rules are clearly stated (with some reasoning) I don't think we need
to care about saying what happens if you break them. People will always shoot
themselves in the foot, but as long as it is reasonably fiddly to do that's
fine by me :)
...
> > > +static int converter_frontend_bind(struct device *dev)
> > > +{
> > > + struct converter_frontend *frontend = dev_get_drvdata(dev);
> > > + int ret;
> > > +
> > > + ret = component_bind_all(dev, NULL);
> > > + if (ret)
> > > + return ret;
> > > + /*
> > > + * We open a new group so that we can control when resources are
> > > + * released and still use device managed (devm_) calls. The expectations
> > > + * are that on probe, backend resources are allocated first followed by
> > > + * the frontend resources (where registering the IIO device must happen)
> > > + * Naturally we want the reverse order on the unbind path and that would
> > > + * not be possible without opening our own devres group.
> > > +
> > > + * Note that the component API also opens it's own devres group when
> > > + * calling the .bind() callbacks for both the aggregate device
> > > + * (our frontend) and each of the components (our backends). On the
> > > + * unbind path, the aggregate .unbind() function is called
> > > + * (@converter_frontend_unbind()) which should be responsible to tear
> > > + * down all the components (effectively releasing all the resources
> > > + * allocated on each component devres group) and only then the aggregate
> > > + * devres group is released. Hence, the order we want to maintain for
> > > + * releasing resources would not be satisfied because backend resources
> > > + * would be freed first. With our own group, we can control when
> > > + * releasing the resources and we do it before @component_unbind_all().
> > > + *
> > > + * This also relies that internally the component API is releasing each
> > > + * of the component's devres group. That is likely not to change, but
> > > + * maybe we should not trust it and also open our own groups for backend
> > > + * devices?!
> > > + *
> > > + * Another very important thing to keep in mind is that this is only
> > > + * valid if frontend and backend driver's are implementing their
> > > + * .remove() callback to call @converter_frontend_del() and
> > > + * @converter_backend_del(). Calling those functions from
> > > + * devm_add_action* and use devm APIs in .frontend_init() and
> > > + * .backend_init() is not going to work. Not perfect but still better
> > > + * than having to tear everything down in .frontend_close() and
> > > + * .backend_close()
> >
> > That last bit is nasty and will be non obvious to driver authors.
> >
> > I wonder if we can come up with some means to make it hard to do.
> >
>
> Yeah, I agree. The alternative is to always bring everything down in
> .frontend_close() and .backend_close(). But that can also be prone to subtle bugs
> because it's easy to mess up the ordering when not using devres.
>
> So, at this point, I cannot really think on a perfect solution rather than keeping
> some rules like (assuming we keep the logic we have now):
>
> * Using devres on frontend|backend_init() only when .remove() is provided on the
> driver.
> * No mixes of devres and .frontend|backend_close()
>
> But yeah, would be nice if we could come up with something to make it more obvious to
> driver authors.
>
> We might be able to detect that converter_backend_del() and converter_frontend_del()
> are under devres while no .frontend|backend_close() is being given. I guess that
> could be a valid indicator of likely misusage.
>
> Or even better (but I'm not sure it's doable with the current devres API), detecting
> that converter_backend_del() or converter_frontend_del() are under devres while more
> resources are also allocated in our specific opened groups. That would always be a
> problem (I think) because the only way for the _del() functions to be under devres is
> if someone added them (from .probe) with devm_add_action() which means that tearing
> down the aggregate will happen after some resources (which were allocated in the
> _init() function) are already freed (as even with new groups, devres will remove
> things on the reverse order). And that would defenitely be problematic. And, in fact,
> is the whole reason why I have the .del() functions on .remove() (so, tearing down
> the aggregate device is the first thing to happen and resources are freed in the
> reverse order they were allocated).
>
I couldn't work out how to do anything easily and would need some experiments.
Maybe some 'hidden' devres callbacks and a state flag somewhere. If we register
that very late we can perhaps detect that we entered devres cleanup before calling
expected manual cleanup. I'm thinking have the setup path register a flag checking
callback and the cleanup path set a flag (devres now safe). Then we can at least
make it scream if we end up doing things in wrong way.
> Other thought would be some generic helper macros to use in these type of drivers so
> a .remove() callback is always added to remove the components.
I wondered if that could work but it's an ugly macro because needs to deal with
different bus types.
>
> Anyways, even the above might be tricky enough to not include it. I'll give it some
> thought.
Agreed - seems fiddly but maybe there is a neat trick to it.
Or indeed another subsystem already doing something.
> > > +#define converter_test_pattern_xlate(pattern, xlate) \
> > > + __converter_test_pattern_xlate(pattern, xlate, ARRAY_SIZE(xlate));
> > > +
> > > +#if IS_ENABLED(CONFIG_IIO_CONVERTER)
> >
> > Why? I'd expect any driver that uses this framework to be useless without
> > it, so we shouldn't need protections. Handle that with Kconfig select / depends
> >
>
> Well, we do have cases of frontends that might be used in standalone mode (I mean,
> with no backend device) or with the backend connected. But alright, I will keep
> things simple for now and let's take care if such case ever get's upstream (hopefully
> they eventual do :)).
Yeah. Introduce the stubs when you need them ;)
...
> > > +
> > > +/**
> > > + * converter_frontend_del - Remove the frontend device
> > > + * @dev: Device to remove from the aggregate
> > > + *
> > > + * Removes the frontend from the aggregate device. This tears down the frontend
> > > + * and all the converters.
> > > + *
> > > + * Ideally, this should be called from the frontend driver .remove() callback.
> > > + * This means that all the converters (and the frontend) will be tear down
> > torn down
> > > + * before running any specific devres cleanup (at the driver core level). What
> > > + * this all means is that we can use devm_ apis in .frontend_init() and being
> > > + * sure those resources will be released after the backend resources and before
> > > + * any devm_* used in .probe(). If that is not the case, one should likely not
> > > + * use any devm_ API in .frontend_init(). That means .frontend_close() should be
> > > + * provided to do all the necessary cleanups.
> >
> > You can force a driver remove to tear down another driver binding first though it
> > all
> > gets fiddly. Take a look at how device_release_driver() is used. May well not
> > help you here though - I've not thought it through properly.
> >
>
> Yeah, I know but I don't think it would help us here or even be correct and I think
> the problem would be the same. I mean, we would still need to call
> converter_frontend_del() or converter_backend_del() from the respective .remove()
> callbacks.
>
Agreed. I'm also not sure it helps and low on time at the moment to experiment around this.
Jonathan
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC PATCH 1/3] iio: addac: add new converter framework
2023-09-03 10:56 ` Jonathan Cameron
@ 2023-09-04 14:14 ` Nuno Sá
2023-09-04 15:31 ` Jonathan Cameron
0 siblings, 1 reply; 19+ messages in thread
From: Nuno Sá @ 2023-09-04 14:14 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Jonathan Cameron, Nuno Sa, linux-iio
On Sun, 2023-09-03 at 11:56 +0100, Jonathan Cameron wrote:
> On Thu, 31 Aug 2023 11:32:54 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
>
> > On Wed, 2023-08-30 at 18:02 +0100, Jonathan Cameron wrote:
> > > On Fri, 4 Aug 2023 16:53:39 +0200
> > > Nuno Sa <nuno.sa@analog.com> wrote:
> > >
> > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > >
> > > Hi Nuno,
> > >
> >
> > Hi Jonathan,
> >
> > Thanks for the initial review...
> >
> > >
> > > One general comment is that you could have stripped this back a fair bit
> > > for ease of understanding. At this stage we don't care about things
> > > like debug or control of test patterns. Bring those in as extra patches.
> > >
> >
> > Agreed... As I mentioned (I think) in the cover, I made the RFC bigger than
> > needed to
> > kind of showcase how we can properly configure the hdl core to support
> > things
> > (interface calibration) that were very hard to do with the current
> > implementation.
> > I'll make sure to add the minimum needed API to accommodate what we have
> > right now.
> >
> > > I haven't fully gotten my head around the ordering constraints on removal.
> > > Are there other users of the component framework that have similar
> > > problems?
> > >
> >
> > My understanding on the component API is that one should do all the tear
> > down in the
> > .unbind() callback. As usual, I can see some drivers not really doing that.
> >
> > > Also, I don't yet understand how a multiple front end, single backend
> > > setup
> > > would work. Or indeed single front end, multiple backend... Maybe we
> > > don't
> > > need those cases, but if we want this to be useful beyond adi-axi we
> > > probably at least want an outline of how they work.
> > >
> >
> > Indeed we can have multiple (and we have it out of tree) backends on one
> > frontend.
> > Think on an ADC/DAC with fairly complex data path with more than one
> > channel/interface (CMOS, LVDS, etc). Typically, in those case, each of the
> > interface
> > will be connected to an instance of the hdl core (the backend).
>
> That might work out for your case, but not the stm32 one where I think we can
> end
> up with interleaved data from two front ends in the same buffer...
>
Not sure I'm following this one. But wouldn't that be something specific for
each system (through devicetree)? I haven't tried but I think the same backend
could be used in different frontend devices (using the component API). That is
not really a usecase for me but definitely something that could be supported (if
we need to start doing things like keep enable/disable counters and so on) if it
is a usecase for stm32.
> >
> > > Jonathan
> > >
> > > > ---
> > > > drivers/iio/addac/converter.c | 547 ++++++++++++++++++++++++++++
> > > > include/linux/iio/addac/converter.h | 485 ++++++++++++++++++++++++
> > > > 2 files changed, 1032 insertions(+)
> > > > create mode 100644 drivers/iio/addac/converter.c
> > > > create mode 100644 include/linux/iio/addac/converter.h
> > > >
> > > > diff --git a/drivers/iio/addac/converter.c
> > > > b/drivers/iio/addac/converter.c
> > > > new file mode 100644
> > > > index 000000000000..31ac704255ad
> > > > --- /dev/null
> > > > +++ b/drivers/iio/addac/converter.c
> > > > @@ -0,0 +1,547 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * Framework to handle complex IIO aggregate devices
> > > > + *
> > > > + * A note on some of the design expectations with regards to lifetimes
> > > > and
> > > > + * devices bringup/removal.
> > > > + *
> > > > + * The Framework is using, under the wood, the component API which
> > > > makes it to
> > > > + * easy treat a bunch of devices as one aggregate device. This means
> > > > that the
> > > > + * complete thing is only brought to live when all the deviced are
> > > > probed. To do
> > >
> > > devices
> > >
> > > > + * this, two callbacks are used that should in fact completely replace
> > > > .probe()
> > > > + * and .remove(). The formers should only be used the minimum needed.
> > > > Ideally,
> > >
> > > I don't follow the sentence in the middle of the line above.
> > >
> > > > + * only to call the functions to add and remove frontend annd backend
> > > > devices.
> > > Spell check...
> > >
> > > > + *
> > > > + * It is advised for frontend and backend drivers to use their
> > > > .remove()
> > >
> > > I'd not 'advise' things. I'd say the 'use' them.
> > >
> > > > + * callbacks (to use devres API during the frontend and backends
> > > > initialization).
> > > > + * See the comment in @converter_frontend_bind().
> > > > + *
> > > > + * It is also assumed that converter objects cannot be accessed once
> > > > one of the
> > > > + * devices of the aggregate device is removed (effectively bringing the
> > > > all the
> > >
> > > bringing all the devices down
> > >
> > > > + * devices down). Based on that assumption, these objects are not
> > > > refcount which
> > >
> > > recounted
> > >
> > > > + * means accessing them will likely fail miserably.
> > >
> > > I hope that doesn't mean there will be no protection. I don't mind if
> > > nothing
> > > works
> > > but breaking the kernel isn't an option.
> > >
> >
> > Hmm, well, you'll have a use after free... But one will have to be creative
> > to use
> > one of these objects after releasing the device from the driver (on the
> > unbind path).
> > And here we don't have any interaction with chardevs, etc which might keep
> > references
> > to devices even after unbind.
> >
> > The only place where I can see someone doing it wrong is from a frontend
> > driver if
> > for some reason (that I cannot think of now) we need to keep references/use
> > 'struct
> > converter' after .frontend_close() is called. In that case and if the
> > backend driver
> > was the one being removed/unbind, the converter object will effectively be
> > freed (as
> > it was allocated with devres) and we are left with a possible use after
> > free. But
> > that would be a very strange usecase to be missed in review (I hope :)).
> >
> > We can always refcount the converters (not sure if we need to do it for
> > frontend
> > devices). Sure, drivers can still screw up but at least in that case, the
> > framework
> > is not to blame :).
>
> If the rules are clearly stated (with some reasoning) I don't think we need
> to care about saying what happens if you break them. People will always shoot
> themselves in the foot, but as long as it is reasonably fiddly to do that's
> fine by me :)
>
> ...
>
> > > > +static int converter_frontend_bind(struct device *dev)
> > > > +{
> > > > + struct converter_frontend *frontend = dev_get_drvdata(dev);
> > > > + int ret;
> > > > +
> > > > + ret = component_bind_all(dev, NULL);
> > > > + if (ret)
> > > > + return ret;
> > > > + /*
> > > > + * We open a new group so that we can control when resources are
> > > > + * released and still use device managed (devm_) calls. The
> > > > expectations
> > > > + * are that on probe, backend resources are allocated first
> > > > followed by
> > > > + * the frontend resources (where registering the IIO device must
> > > > happen)
> > > > + * Naturally we want the reverse order on the unbind path and
> > > > that would
> > > > + * not be possible without opening our own devres group.
> > > > +
> > > > + * Note that the component API also opens it's own devres group
> > > > when
> > > > + * calling the .bind() callbacks for both the aggregate device
> > > > + * (our frontend) and each of the components (our backends). On
> > > > the
> > > > + * unbind path, the aggregate .unbind() function is called
> > > > + * (@converter_frontend_unbind()) which should be responsible to
> > > > tear
> > > > + * down all the components (effectively releasing all the
> > > > resources
> > > > + * allocated on each component devres group) and only then the
> > > > aggregate
> > > > + * devres group is released. Hence, the order we want to
> > > > maintain for
> > > > + * releasing resources would not be satisfied because backend
> > > > resources
> > > > + * would be freed first. With our own group, we can control when
> > > > + * releasing the resources and we do it before
> > > > @component_unbind_all().
> > > > + *
> > > > + * This also relies that internally the component API is
> > > > releasing each
> > > > + * of the component's devres group. That is likely not to
> > > > change, but
> > > > + * maybe we should not trust it and also open our own groups for
> > > > backend
> > > > + * devices?!
> > > > + *
> > > > + * Another very important thing to keep in mind is that this is
> > > > only
> > > > + * valid if frontend and backend driver's are implementing their
> > > > + * .remove() callback to call @converter_frontend_del() and
> > > > + * @converter_backend_del(). Calling those functions from
> > > > + * devm_add_action* and use devm APIs in .frontend_init() and
> > > > + * .backend_init() is not going to work. Not perfect but still
> > > > better
> > > > + * than having to tear everything down in .frontend_close() and
> > > > + * .backend_close()
> > >
> > > That last bit is nasty and will be non obvious to driver authors.
> > >
> > > I wonder if we can come up with some means to make it hard to do.
> > >
> >
> > Yeah, I agree. The alternative is to always bring everything down in
> > .frontend_close() and .backend_close(). But that can also be prone to subtle
> > bugs
> > because it's easy to mess up the ordering when not using devres.
> >
> > So, at this point, I cannot really think on a perfect solution rather than
> > keeping
> > some rules like (assuming we keep the logic we have now):
> >
> > * Using devres on frontend|backend_init() only when .remove() is provided on
> > the
> > driver.
> > * No mixes of devres and .frontend|backend_close()
> >
> > But yeah, would be nice if we could come up with something to make it more
> > obvious to
> > driver authors.
>
> >
> > We might be able to detect that converter_backend_del() and
> > converter_frontend_del()
> > are under devres while no .frontend|backend_close() is being given. I guess
> > that
> > could be a valid indicator of likely misusage.
> >
> > Or even better (but I'm not sure it's doable with the current devres API),
> > detecting
> > that converter_backend_del() or converter_frontend_del() are under devres
> > while more
> > resources are also allocated in our specific opened groups. That would
> > always be a
> > problem (I think) because the only way for the _del() functions to be under
> > devres is
> > if someone added them (from .probe) with devm_add_action() which means that
> > tearing
> > down the aggregate will happen after some resources (which were allocated in
> > the
> > _init() function) are already freed (as even with new groups, devres will
> > remove
> > things on the reverse order). And that would defenitely be problematic. And,
> > in fact,
> > is the whole reason why I have the .del() functions on .remove() (so,
> > tearing down
> > the aggregate device is the first thing to happen and resources are freed in
> > the
> > reverse order they were allocated).
> >
>
This would actually be very messy and hard to do properly. Concurrency between
the aggregate probing (at the component level) and unbinding (at driver core
level) would be very tricky if doable at all. On top that, we do have
devres_find() but no way to tell if a devres group has resources or not. It
would be easy to add one new API but likely not worth it just for this usecase.
I also thought about an helper macro to wrap every devm_ call but your below
suggestion is way better and transparent to users.
> I couldn't work out how to do anything easily and would need some experiments.
> Maybe some 'hidden' devres callbacks and a state flag somewhere. If we
> register
> that very late we can perhaps detect that we entered devres cleanup before
> calling
> expected manual cleanup. I'm thinking have the setup path register a flag
> checking
> callback and the cleanup path set a flag (devres now safe). Then we can at
> least
> make it scream if we end up doing things in wrong way.
>
Hmm, that might actually be a good idea and something to try. It likely means
having a mutex (I was happy not to have one for now :) but likely inevitable
somewhere down the road) for the flag but it might work. At least we'll be able
to dump a WARN or something if we suspect something is wrong.
> > Other thought would be some generic helper macros to use in these type of
> > drivers so
> > a .remove() callback is always added to remove the components.
> I wondered if that could work but it's an ugly macro because needs to deal
> with
> different bus types.
>
>
We could have a macro per bus type. I'm not really seeing anything different
than platform, spi and i2c. But even like this, it could easily start to get
ugly because of accepted parameters (and any deviation would again mean a
different version per bus type). Another thing that crossed my mind was wrappers
to module_spi_driver() and friends. Maybe that could work. Anyways, as you said
in one of your replies, this is in kernel interface that we can easily
add/change. So, I will defer this to a later point in time.
- Nuno Sá
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC PATCH 1/3] iio: addac: add new converter framework
2023-09-04 14:14 ` Nuno Sá
@ 2023-09-04 15:31 ` Jonathan Cameron
2023-11-13 17:20 ` Olivier MOYSAN
0 siblings, 1 reply; 19+ messages in thread
From: Jonathan Cameron @ 2023-09-04 15:31 UTC (permalink / raw)
To: Nuno Sá; +Cc: Jonathan Cameron, Nuno Sa, linux-iio
On Mon, 04 Sep 2023 16:14:17 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Sun, 2023-09-03 at 11:56 +0100, Jonathan Cameron wrote:
> > On Thu, 31 Aug 2023 11:32:54 +0200
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >
> > > On Wed, 2023-08-30 at 18:02 +0100, Jonathan Cameron wrote:
> > > > On Fri, 4 Aug 2023 16:53:39 +0200
> > > > Nuno Sa <nuno.sa@analog.com> wrote:
> > > >
> > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > >
> > > > Hi Nuno,
> > > >
> > >
> > > Hi Jonathan,
> > >
> > > Thanks for the initial review...
> > >
> > > >
> > > > One general comment is that you could have stripped this back a fair bit
> > > > for ease of understanding. At this stage we don't care about things
> > > > like debug or control of test patterns. Bring those in as extra patches.
> > > >
> > >
> > > Agreed... As I mentioned (I think) in the cover, I made the RFC bigger than
> > > needed to
> > > kind of showcase how we can properly configure the hdl core to support
> > > things
> > > (interface calibration) that were very hard to do with the current
> > > implementation.
> > > I'll make sure to add the minimum needed API to accommodate what we have
> > > right now.
> > >
> > > > I haven't fully gotten my head around the ordering constraints on removal.
> > > > Are there other users of the component framework that have similar
> > > > problems?
> > > >
> > >
> > > My understanding on the component API is that one should do all the tear
> > > down in the
> > > .unbind() callback. As usual, I can see some drivers not really doing that.
> > >
> > > > Also, I don't yet understand how a multiple front end, single backend
> > > > setup
> > > > would work. Or indeed single front end, multiple backend... Maybe we
> > > > don't
> > > > need those cases, but if we want this to be useful beyond adi-axi we
> > > > probably at least want an outline of how they work.
> > > >
> > >
> > > Indeed we can have multiple (and we have it out of tree) backends on one
> > > frontend.
> > > Think on an ADC/DAC with fairly complex data path with more than one
> > > channel/interface (CMOS, LVDS, etc). Typically, in those case, each of the
> > > interface
> > > will be connected to an instance of the hdl core (the backend).
> >
> > That might work out for your case, but not the stm32 one where I think we can
> > end
> > up with interleaved data from two front ends in the same buffer...
> >
>
> Not sure I'm following this one. But wouldn't that be something specific for
> each system (through devicetree)? I haven't tried but I think the same backend
> could be used in different frontend devices (using the component API). That is
> not really a usecase for me but definitely something that could be supported (if
> we need to start doing things like keep enable/disable counters and so on) if it
> is a usecase for stm32.
If we are going to support both usecases, we just need to figure out what composite
devices with N-M backend - frontend look like and make sure that doesn't
cause problems. I'd expect the separation between backend instances might
reflect data storage on capture but then again that might end up like the many
IIO devices for many buffers mess we had before the multiple buffer support
was added.
Might need enable /disable counters as you say - I'm not quite sure without
trying it!
> > > > > + * than having to tear everything down in .frontend_close() and
> > > > > + * .backend_close()
> > > >
> > > > That last bit is nasty and will be non obvious to driver authors.
> > > >
> > > > I wonder if we can come up with some means to make it hard to do.
> > > >
> > >
> > > Yeah, I agree. The alternative is to always bring everything down in
> > > .frontend_close() and .backend_close(). But that can also be prone to subtle
> > > bugs
> > > because it's easy to mess up the ordering when not using devres.
> > >
> > > So, at this point, I cannot really think on a perfect solution rather than
> > > keeping
> > > some rules like (assuming we keep the logic we have now):
> > >
> > > * Using devres on frontend|backend_init() only when .remove() is provided on
> > > the
> > > driver.
> > > * No mixes of devres and .frontend|backend_close()
> > >
> > > But yeah, would be nice if we could come up with something to make it more
> > > obvious to
> > > driver authors.
> >
> > >
> > > We might be able to detect that converter_backend_del() and
> > > converter_frontend_del()
> > > are under devres while no .frontend|backend_close() is being given. I guess
> > > that
> > > could be a valid indicator of likely misusage.
> > >
> > > Or even better (but I'm not sure it's doable with the current devres API),
> > > detecting
> > > that converter_backend_del() or converter_frontend_del() are under devres
> > > while more
> > > resources are also allocated in our specific opened groups. That would
> > > always be a
> > > problem (I think) because the only way for the _del() functions to be under
> > > devres is
> > > if someone added them (from .probe) with devm_add_action() which means that
> > > tearing
> > > down the aggregate will happen after some resources (which were allocated in
> > > the
> > > _init() function) are already freed (as even with new groups, devres will
> > > remove
> > > things on the reverse order). And that would defenitely be problematic. And,
> > > in fact,
> > > is the whole reason why I have the .del() functions on .remove() (so,
> > > tearing down
> > > the aggregate device is the first thing to happen and resources are freed in
> > > the
> > > reverse order they were allocated).
> > >
> >
>
> This would actually be very messy and hard to do properly. Concurrency between
> the aggregate probing (at the component level) and unbinding (at driver core
> level) would be very tricky if doable at all. On top that, we do have
> devres_find() but no way to tell if a devres group has resources or not. It
> would be easy to add one new API but likely not worth it just for this usecase.
>
> I also thought about an helper macro to wrap every devm_ call but your below
> suggestion is way better and transparent to users.
>
> > I couldn't work out how to do anything easily and would need some experiments.
> > Maybe some 'hidden' devres callbacks and a state flag somewhere. If we
> > register
> > that very late we can perhaps detect that we entered devres cleanup before
> > calling
> > expected manual cleanup. I'm thinking have the setup path register a flag
> > checking
> > callback and the cleanup path set a flag (devres now safe). Then we can at
> > least
> > make it scream if we end up doing things in wrong way.
> >
>
> Hmm, that might actually be a good idea and something to try. It likely means
> having a mutex (I was happy not to have one for now :) but likely inevitable
> somewhere down the road) for the flag but it might work. At least we'll be able
> to dump a WARN or something if we suspect something is wrong.
Careful ordering an atomic might work.
>
> > > Other thought would be some generic helper macros to use in these type of
> > > drivers so
> > > a .remove() callback is always added to remove the components.
> > I wondered if that could work but it's an ugly macro because needs to deal
> > with
> > different bus types.
> >
> >
>
> We could have a macro per bus type. I'm not really seeing anything different
> than platform, spi and i2c. But even like this, it could easily start to get
> ugly because of accepted parameters (and any deviation would again mean a
> different version per bus type). Another thing that crossed my mind was wrappers
> to module_spi_driver() and friends. Maybe that could work. Anyways, as you said
> in one of your replies, this is in kernel interface that we can easily
> add/change. So, I will defer this to a later point in time.
Makes sense
J
>
> - Nuno Sá
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 1/3] iio: addac: add new converter framework
2023-09-04 15:31 ` Jonathan Cameron
@ 2023-11-13 17:20 ` Olivier MOYSAN
2023-11-14 9:03 ` Nuno Sá
0 siblings, 1 reply; 19+ messages in thread
From: Olivier MOYSAN @ 2023-11-13 17:20 UTC (permalink / raw)
To: Jonathan Cameron, Nuno Sá; +Cc: Jonathan Cameron, Nuno Sa, linux-iio
Hi Nuno, Jonathan,
On 9/4/23 17:31, Jonathan Cameron wrote:
> On Mon, 04 Sep 2023 16:14:17 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
>
>> On Sun, 2023-09-03 at 11:56 +0100, Jonathan Cameron wrote:
>>> On Thu, 31 Aug 2023 11:32:54 +0200
>>> Nuno Sá <noname.nuno@gmail.com> wrote:
>>>
>>>> On Wed, 2023-08-30 at 18:02 +0100, Jonathan Cameron wrote:
>>>>> On Fri, 4 Aug 2023 16:53:39 +0200
>>>>> Nuno Sa <nuno.sa@analog.com> wrote:
>>>>>
>>>>>> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
>>>>>
>>>>> Hi Nuno,
>>>>>
>>>>
>>>> Hi Jonathan,
>>>>
>>>> Thanks for the initial review...
>>>>
>>>>>
>>>>> One general comment is that you could have stripped this back a fair bit
>>>>> for ease of understanding. At this stage we don't care about things
>>>>> like debug or control of test patterns. Bring those in as extra patches.
>>>>>
>>>>
>>>> Agreed... As I mentioned (I think) in the cover, I made the RFC bigger than
>>>> needed to
>>>> kind of showcase how we can properly configure the hdl core to support
>>>> things
>>>> (interface calibration) that were very hard to do with the current
>>>> implementation.
>>>> I'll make sure to add the minimum needed API to accommodate what we have
>>>> right now.
>>>>
>>>>> I haven't fully gotten my head around the ordering constraints on removal.
>>>>> Are there other users of the component framework that have similar
>>>>> problems?
>>>>>
>>>>
>>>> My understanding on the component API is that one should do all the tear
>>>> down in the
>>>> .unbind() callback. As usual, I can see some drivers not really doing that.
>>>>
>>>>> Also, I don't yet understand how a multiple front end, single backend
>>>>> setup
>>>>> would work. Or indeed single front end, multiple backend... Maybe we
>>>>> don't
>>>>> need those cases, but if we want this to be useful beyond adi-axi we
>>>>> probably at least want an outline of how they work.
>>>>>
>>>>
>>>> Indeed we can have multiple (and we have it out of tree) backends on one
>>>> frontend.
>>>> Think on an ADC/DAC with fairly complex data path with more than one
>>>> channel/interface (CMOS, LVDS, etc). Typically, in those case, each of the
>>>> interface
>>>> will be connected to an instance of the hdl core (the backend).
>>>
>>> That might work out for your case, but not the stm32 one where I think we can
>>> end
>>> up with interleaved data from two front ends in the same buffer...
>>>
>>
>> Not sure I'm following this one. But wouldn't that be something specific for
>> each system (through devicetree)? I haven't tried but I think the same backend
>> could be used in different frontend devices (using the component API). That is
>> not really a usecase for me but definitely something that could be supported (if
>> we need to start doing things like keep enable/disable counters and so on) if it
>> is a usecase for stm32.
>
> If we are going to support both usecases, we just need to figure out what composite
> devices with N-M backend - frontend look like and make sure that doesn't
> cause problems. I'd expect the separation between backend instances might
> reflect data storage on capture but then again that might end up like the many
> IIO devices for many buffers mess we had before the multiple buffer support
> was added.
>
The stm32 dfsdm interleaved use case is not a problem as it is possible
to associate several backends to a frontend.
I did some experiments based on converter framework, and did not
identified blocking points regarding dfsdm use cases.
Some limitations where discussed in [1], about generic bindings support.
The preferred solution was to extend converter_frontend_add_matches() to
parse also child nodes. I have added converters_get_from_fwnode() API
and adapted converter_frontend_add_matches() to test this approach.
With this changes and an additional api to support channel attributes
read, the framework fulfills all the needs for dfsdm.
So, I feel comfortable to drop my previous "backend framework" proposal,
and move to the current proposal.
If we go further in converter framework adaption, I will push these updates.
[1]:
https://lore.kernel.org/lkml/60e913e81ce67192ed0449fe7b718434fd360d97.camel@gmail.com/
BRs
Olivier
> Might need enable /disable counters as you say - I'm not quite sure without
> trying it!
>>>>>> + * than having to tear everything down in .frontend_close() and
>>>>>> + * .backend_close()
>>>>>
>>>>> That last bit is nasty and will be non obvious to driver authors.
>>>>>
>>>>> I wonder if we can come up with some means to make it hard to do.
>>>>>
>>>>
>>>> Yeah, I agree. The alternative is to always bring everything down in
>>>> .frontend_close() and .backend_close(). But that can also be prone to subtle
>>>> bugs
>>>> because it's easy to mess up the ordering when not using devres.
>>>>
>>>> So, at this point, I cannot really think on a perfect solution rather than
>>>> keeping
>>>> some rules like (assuming we keep the logic we have now):
>>>>
>>>> * Using devres on frontend|backend_init() only when .remove() is provided on
>>>> the
>>>> driver.
>>>> * No mixes of devres and .frontend|backend_close()
>>>>
>>>> But yeah, would be nice if we could come up with something to make it more
>>>> obvious to
>>>> driver authors.
>>>
>>>>
>>>> We might be able to detect that converter_backend_del() and
>>>> converter_frontend_del()
>>>> are under devres while no .frontend|backend_close() is being given. I guess
>>>> that
>>>> could be a valid indicator of likely misusage.
>>>>
>>>> Or even better (but I'm not sure it's doable with the current devres API),
>>>> detecting
>>>> that converter_backend_del() or converter_frontend_del() are under devres
>>>> while more
>>>> resources are also allocated in our specific opened groups. That would
>>>> always be a
>>>> problem (I think) because the only way for the _del() functions to be under
>>>> devres is
>>>> if someone added them (from .probe) with devm_add_action() which means that
>>>> tearing
>>>> down the aggregate will happen after some resources (which were allocated in
>>>> the
>>>> _init() function) are already freed (as even with new groups, devres will
>>>> remove
>>>> things on the reverse order). And that would defenitely be problematic. And,
>>>> in fact,
>>>> is the whole reason why I have the .del() functions on .remove() (so,
>>>> tearing down
>>>> the aggregate device is the first thing to happen and resources are freed in
>>>> the
>>>> reverse order they were allocated).
>>>>
>>>
>>
>> This would actually be very messy and hard to do properly. Concurrency between
>> the aggregate probing (at the component level) and unbinding (at driver core
>> level) would be very tricky if doable at all. On top that, we do have
>> devres_find() but no way to tell if a devres group has resources or not. It
>> would be easy to add one new API but likely not worth it just for this usecase.
>>
>> I also thought about an helper macro to wrap every devm_ call but your below
>> suggestion is way better and transparent to users.
>>
>>> I couldn't work out how to do anything easily and would need some experiments.
>>> Maybe some 'hidden' devres callbacks and a state flag somewhere. If we
>>> register
>>> that very late we can perhaps detect that we entered devres cleanup before
>>> calling
>>> expected manual cleanup. I'm thinking have the setup path register a flag
>>> checking
>>> callback and the cleanup path set a flag (devres now safe). Then we can at
>>> least
>>> make it scream if we end up doing things in wrong way.
>>>
>>
>> Hmm, that might actually be a good idea and something to try. It likely means
>> having a mutex (I was happy not to have one for now :) but likely inevitable
>> somewhere down the road) for the flag but it might work. At least we'll be able
>> to dump a WARN or something if we suspect something is wrong.
>
> Careful ordering an atomic might work.
>
>>
>>>> Other thought would be some generic helper macros to use in these type of
>>>> drivers so
>>>> a .remove() callback is always added to remove the components.
>>> I wondered if that could work but it's an ugly macro because needs to deal
>>> with
>>> different bus types.
>>>
>>>
>>
>> We could have a macro per bus type. I'm not really seeing anything different
>> than platform, spi and i2c. But even like this, it could easily start to get
>> ugly because of accepted parameters (and any deviation would again mean a
>> different version per bus type). Another thing that crossed my mind was wrappers
>> to module_spi_driver() and friends. Maybe that could work. Anyways, as you said
>> in one of your replies, this is in kernel interface that we can easily
>> add/change. So, I will defer this to a later point in time.
>
> Makes sense
>
> J
>>
>> - Nuno Sá
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 1/3] iio: addac: add new converter framework
2023-11-13 17:20 ` Olivier MOYSAN
@ 2023-11-14 9:03 ` Nuno Sá
2023-11-16 15:42 ` Olivier MOYSAN
0 siblings, 1 reply; 19+ messages in thread
From: Nuno Sá @ 2023-11-14 9:03 UTC (permalink / raw)
To: Olivier MOYSAN, Jonathan Cameron; +Cc: Jonathan Cameron, Nuno Sa, linux-iio
On Mon, 2023-11-13 at 18:20 +0100, Olivier MOYSAN wrote:
Ho Olivier,
> Hi Nuno, Jonathan,
>
> On 9/4/23 17:31, Jonathan Cameron wrote:
> > On Mon, 04 Sep 2023 16:14:17 +0200
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >
> > > On Sun, 2023-09-03 at 11:56 +0100, Jonathan Cameron wrote:
> > > > On Thu, 31 Aug 2023 11:32:54 +0200
> > > > Nuno Sá <noname.nuno@gmail.com> wrote:
> > > >
> > > > > On Wed, 2023-08-30 at 18:02 +0100, Jonathan Cameron wrote:
> > > > > > On Fri, 4 Aug 2023 16:53:39 +0200
> > > > > > Nuno Sa <nuno.sa@analog.com> wrote:
> > > > > >
> > > > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > > > >
> > > > > > Hi Nuno,
> > > > > >
> > > > >
> > > > > Hi Jonathan,
> > > > >
> > > > > Thanks for the initial review...
> > > > >
> > > > > >
> > > > > > One general comment is that you could have stripped this back a fair
> > > > > > bit
> > > > > > for ease of understanding. At this stage we don't care about things
> > > > > > like debug or control of test patterns. Bring those in as extra
> > > > > > patches.
> > > > > >
> > > > >
> > > > > Agreed... As I mentioned (I think) in the cover, I made the RFC bigger
> > > > > than
> > > > > needed to
> > > > > kind of showcase how we can properly configure the hdl core to support
> > > > > things
> > > > > (interface calibration) that were very hard to do with the current
> > > > > implementation.
> > > > > I'll make sure to add the minimum needed API to accommodate what we
> > > > > have
> > > > > right now.
> > > > >
> > > > > > I haven't fully gotten my head around the ordering constraints on
> > > > > > removal.
> > > > > > Are there other users of the component framework that have similar
> > > > > > problems?
> > > > > >
> > > > >
> > > > > My understanding on the component API is that one should do all the
> > > > > tear
> > > > > down in the
> > > > > .unbind() callback. As usual, I can see some drivers not really doing
> > > > > that.
> > > > >
> > > > > > Also, I don't yet understand how a multiple front end, single
> > > > > > backend
> > > > > > setup
> > > > > > would work. Or indeed single front end, multiple backend... Maybe
> > > > > > we
> > > > > > don't
> > > > > > need those cases, but if we want this to be useful beyond adi-axi we
> > > > > > probably at least want an outline of how they work.
> > > > > >
> > > > >
> > > > > Indeed we can have multiple (and we have it out of tree) backends on
> > > > > one
> > > > > frontend.
> > > > > Think on an ADC/DAC with fairly complex data path with more than one
> > > > > channel/interface (CMOS, LVDS, etc). Typically, in those case, each of
> > > > > the
> > > > > interface
> > > > > will be connected to an instance of the hdl core (the backend).
> > > >
> > > > That might work out for your case, but not the stm32 one where I think
> > > > we can
> > > > end
> > > > up with interleaved data from two front ends in the same buffer...
> > > >
> > >
> > > Not sure I'm following this one. But wouldn't that be something specific
> > > for
> > > each system (through devicetree)? I haven't tried but I think the same
> > > backend
> > > could be used in different frontend devices (using the component API).
> > > That is
> > > not really a usecase for me but definitely something that could be
> > > supported (if
> > > we need to start doing things like keep enable/disable counters and so on)
> > > if it
> > > is a usecase for stm32.
> >
> > If we are going to support both usecases, we just need to figure out what
> > composite
> > devices with N-M backend - frontend look like and make sure that doesn't
> > cause problems. I'd expect the separation between backend instances might
> > reflect data storage on capture but then again that might end up like the
> > many
> > IIO devices for many buffers mess we had before the multiple buffer support
> > was added.
> >
>
> The stm32 dfsdm interleaved use case is not a problem as it is possible
> to associate several backends to a frontend.
> I did some experiments based on converter framework, and did not
> identified blocking points regarding dfsdm use cases.
>
> Some limitations where discussed in [1], about generic bindings support.
> The preferred solution was to extend converter_frontend_add_matches() to
> parse also child nodes. I have added converters_get_from_fwnode() API
> and adapted converter_frontend_add_matches() to test this approach.
> With this changes and an additional api to support channel attributes
> read, the framework fulfills all the needs for dfsdm.
>
> So, I feel comfortable to drop my previous "backend framework" proposal,
> and move to the current proposal.
>
> If we go further in converter framework adaption, I will push these updates.
>
I hope you didn't had too much trouble with those patches. The reason I'm saying
this is because, after some thought, I'm strongly considering in moving to
normal OF/ACPI lookup. 3 mains reasons for it:
1) That "hack/rule" for a driver to provide a .remove() callback (in order to
devm_*) is really non obvious and might even be prune to subtle bugs (that I'm
not seeing now :)). But my main argument is that it can become hard to maintain
it (depending on how much people starts to use the framework).
2) From the discussion we had about the limitations you pointed in your link, I
started to realize that it might get harder to scale the framework. Yes, we
found a fairly easy way of doing it but still took more code to do it when
compared to a typical lookup.
3) This is the most important together with 1). You mentioned something like
cascaded designs and I just found an usercase in ADI out of tree drivers. We
have a design where we have something like:
------------------------------------------
| FPGA |
-------------- | ------------- ------------------- |
|DAC Frontend| -> | |DAC Backend| -> |DAC Interpolation| |
-------------- | ------------- ------------------- |
| |
------------------------------------------
In the above design we kind of have a cascaded thing where the DAC backend is
both a frontend and a backend and the Intrerpolation stuff just serves as
backend to the DAC core. So, ideally the DAC frontend should not have to know a
thing about the interpolation... I realized that having this with the component
framework is far from straight because we would need two components/aggregate
devices to accomplish this (DAC Front + DAC Back) and (DAC Back + DAC Interp)
and I think we would need some extra care regarding one of the components going
away (not sure though). One way to make it simple would be to not "respect" the
HW configuration and just have one aggregate device with 1 Frontend + 2 Backends
and so the frontend would need to "know" about the interpolation core. Again, I
think that with OF/ACPI this setup with be fairly straight to get and "respect".
Anyways, all the above makes me feel that component might not be the best choice
(even though I was eager to use it :))
I'll also get to work on this again and I might just use an industrialio-
backend.c file in the base dir as you had in your RFC. From a quick look on your
series, I'm no sure how much code I will reuse but we can see later if a Co-
authored-by tag makes sense or not.
Let me know if there's something you don't agree or if there's any concern on
your side.
- Nuno Sá
> > >
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC PATCH 1/3] iio: addac: add new converter framework
2023-11-14 9:03 ` Nuno Sá
@ 2023-11-16 15:42 ` Olivier MOYSAN
0 siblings, 0 replies; 19+ messages in thread
From: Olivier MOYSAN @ 2023-11-16 15:42 UTC (permalink / raw)
To: Nuno Sá, Jonathan Cameron; +Cc: Jonathan Cameron, Nuno Sa, linux-iio
Hi Nuno,
On 11/14/23 10:03, Nuno Sá wrote:
> On Mon, 2023-11-13 at 18:20 +0100, Olivier MOYSAN wrote:
>
> Ho Olivier,
>
>> Hi Nuno, Jonathan,
>>
>> On 9/4/23 17:31, Jonathan Cameron wrote:
>>> On Mon, 04 Sep 2023 16:14:17 +0200
>>> Nuno Sá <noname.nuno@gmail.com> wrote:
>>>
>>>> On Sun, 2023-09-03 at 11:56 +0100, Jonathan Cameron wrote:
>>>>> On Thu, 31 Aug 2023 11:32:54 +0200
>>>>> Nuno Sá <noname.nuno@gmail.com> wrote:
>>>>>
>>>>>> On Wed, 2023-08-30 at 18:02 +0100, Jonathan Cameron wrote:
>>>>>>> On Fri, 4 Aug 2023 16:53:39 +0200
>>>>>>> Nuno Sa <nuno.sa@analog.com> wrote:
>>>>>>>
>>>>>>>> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
>>>>>>>
>>>>>>> Hi Nuno,
>>>>>>>
>>>>>>
>>>>>> Hi Jonathan,
>>>>>>
>>>>>> Thanks for the initial review...
>>>>>>
>>>>>>>
>>>>>>> One general comment is that you could have stripped this back a fair
>>>>>>> bit
>>>>>>> for ease of understanding. At this stage we don't care about things
>>>>>>> like debug or control of test patterns. Bring those in as extra
>>>>>>> patches.
>>>>>>>
>>>>>>
>>>>>> Agreed... As I mentioned (I think) in the cover, I made the RFC bigger
>>>>>> than
>>>>>> needed to
>>>>>> kind of showcase how we can properly configure the hdl core to support
>>>>>> things
>>>>>> (interface calibration) that were very hard to do with the current
>>>>>> implementation.
>>>>>> I'll make sure to add the minimum needed API to accommodate what we
>>>>>> have
>>>>>> right now.
>>>>>>
>>>>>>> I haven't fully gotten my head around the ordering constraints on
>>>>>>> removal.
>>>>>>> Are there other users of the component framework that have similar
>>>>>>> problems?
>>>>>>>
>>>>>>
>>>>>> My understanding on the component API is that one should do all the
>>>>>> tear
>>>>>> down in the
>>>>>> .unbind() callback. As usual, I can see some drivers not really doing
>>>>>> that.
>>>>>>
>>>>>>> Also, I don't yet understand how a multiple front end, single
>>>>>>> backend
>>>>>>> setup
>>>>>>> would work. Or indeed single front end, multiple backend... Maybe
>>>>>>> we
>>>>>>> don't
>>>>>>> need those cases, but if we want this to be useful beyond adi-axi we
>>>>>>> probably at least want an outline of how they work.
>>>>>>>
>>>>>>
>>>>>> Indeed we can have multiple (and we have it out of tree) backends on
>>>>>> one
>>>>>> frontend.
>>>>>> Think on an ADC/DAC with fairly complex data path with more than one
>>>>>> channel/interface (CMOS, LVDS, etc). Typically, in those case, each of
>>>>>> the
>>>>>> interface
>>>>>> will be connected to an instance of the hdl core (the backend).
>>>>>
>>>>> That might work out for your case, but not the stm32 one where I think
>>>>> we can
>>>>> end
>>>>> up with interleaved data from two front ends in the same buffer...
>>>>>
>>>>
>>>> Not sure I'm following this one. But wouldn't that be something specific
>>>> for
>>>> each system (through devicetree)? I haven't tried but I think the same
>>>> backend
>>>> could be used in different frontend devices (using the component API).
>>>> That is
>>>> not really a usecase for me but definitely something that could be
>>>> supported (if
>>>> we need to start doing things like keep enable/disable counters and so on)
>>>> if it
>>>> is a usecase for stm32.
>>>
>>> If we are going to support both usecases, we just need to figure out what
>>> composite
>>> devices with N-M backend - frontend look like and make sure that doesn't
>>> cause problems. I'd expect the separation between backend instances might
>>> reflect data storage on capture but then again that might end up like the
>>> many
>>> IIO devices for many buffers mess we had before the multiple buffer support
>>> was added.
>>>
>>
>> The stm32 dfsdm interleaved use case is not a problem as it is possible
>> to associate several backends to a frontend.
>> I did some experiments based on converter framework, and did not
>> identified blocking points regarding dfsdm use cases.
>>
>> Some limitations where discussed in [1], about generic bindings support.
>> The preferred solution was to extend converter_frontend_add_matches() to
>> parse also child nodes. I have added converters_get_from_fwnode() API
>> and adapted converter_frontend_add_matches() to test this approach.
>> With this changes and an additional api to support channel attributes
>> read, the framework fulfills all the needs for dfsdm.
>>
>> So, I feel comfortable to drop my previous "backend framework" proposal,
>> and move to the current proposal.
>>
>> If we go further in converter framework adaption, I will push these updates.
>>
>
> I hope you didn't had too much trouble with those patches. The reason I'm saying
> this is because, after some thought, I'm strongly considering in moving to
> normal OF/ACPI lookup. 3 mains reasons for it:
>
No problem, this was an opportunity to discover the component framework.
The converter API is quite simple to use once you understand the basics
of component concepts. It does the job for stm dfsdm, but I agree that
for the long-term, a component-based solution is probably less scalable.
> 1) That "hack/rule" for a driver to provide a .remove() callback (in order to
> devm_*) is really non obvious and might even be prune to subtle bugs (that I'm
> not seeing now :)). But my main argument is that it can become hard to maintain
> it (depending on how much people starts to use the framework).
>
> 2) From the discussion we had about the limitations you pointed in your link, I
> started to realize that it might get harder to scale the framework. Yes, we
> found a fairly easy way of doing it but still took more code to do it when
> compared to a typical lookup.
>
> 3) This is the most important together with 1). You mentioned something like
> cascaded designs and I just found an usercase in ADI out of tree drivers. We
> have a design where we have something like:
>
> ------------------------------------------
> | FPGA |
> -------------- | ------------- ------------------- |
> |DAC Frontend| -> | |DAC Backend| -> |DAC Interpolation| |
> -------------- | ------------- ------------------- |
> | |
> ------------------------------------------
>
> In the above design we kind of have a cascaded thing where the DAC backend is
> both a frontend and a backend and the Intrerpolation stuff just serves as
> backend to the DAC core. So, ideally the DAC frontend should not have to know a
> thing about the interpolation... I realized that having this with the component
> framework is far from straight because we would need two components/aggregate
> devices to accomplish this (DAC Front + DAC Back) and (DAC Back + DAC Interp)
> and I think we would need some extra care regarding one of the components going
> away (not sure though). One way to make it simple would be to not "respect" the
> HW configuration and just have one aggregate device with 1 Frontend + 2 Backends
> and so the frontend would need to "know" about the interpolation core. Again, I
> think that with OF/ACPI this setup with be fairly straight to get and "respect".
>
> Anyways, all the above makes me feel that component might not be the best choice
> (even though I was eager to use it :))
>
> I'll also get to work on this again and I might just use an industrialio-
> backend.c file in the base dir as you had in your RFC. From a quick look on your
> series, I'm no sure how much code I will reuse but we can see later if a Co-
> authored-by tag makes sense or not.
>
The APIs in my RFC were kept minimalistic to assess the proposal's
relevance. As you pointed out, there is room for improvement,
particularly with regards to object releasing. If you are on the way to
rework your framework, feel free to re-use some parts of this RFC.
At the end, what really matters is to converge towards a solution :)
Regards
Olivier
> Let me know if there's something you don't agree or if there's any concern on
> your side.
>
> - Nuno Sá
>>>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC PATCH 2/3] iio: adc: ad9647: add based on converter framework
2023-08-04 14:53 [RFC PATCH 0/3] Add converter framework Nuno Sa
2023-08-04 14:53 ` [RFC PATCH 1/3] iio: addac: add new " Nuno Sa
@ 2023-08-04 14:53 ` Nuno Sa
2023-08-30 17:13 ` Jonathan Cameron
2023-08-04 14:53 ` [RFC PATCH 3/3] iio: adc: adi-axi-adc: add based on new " Nuno Sa
2023-08-30 16:29 ` [RFC PATCH 0/3] Add " Jonathan Cameron
3 siblings, 1 reply; 19+ messages in thread
From: Nuno Sa @ 2023-08-04 14:53 UTC (permalink / raw)
To: linux-iio; +Cc: Jonathan Cameron
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/adc/ad9467_new.c | 830 +++++++++++++++++++++++++++++++++++
1 file changed, 830 insertions(+)
create mode 100644 drivers/iio/adc/ad9467_new.c
diff --git a/drivers/iio/adc/ad9467_new.c b/drivers/iio/adc/ad9467_new.c
new file mode 100644
index 000000000000..ccdd3a893beb
--- /dev/null
+++ b/drivers/iio/adc/ad9467_new.c
@@ -0,0 +1,830 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices AD9467 SPI ADC driver
+ *
+ * Copyright 2012-2023 Analog Devices Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+
+#include <linux/iio/addc/converter.h>
+#include <linux/iio/buffer-dmaengine.h>
+#include <linux/iio/iio.h>
+
+/*
+ * ADI High-Speed ADC common spi interface registers
+ * See Application-Note AN-877:
+ * https://www.analog.com/media/en/technical-documentation/application-notes/AN-877.pdf
+ */
+
+#define AN877_ADC_REG_CHIP_ID 0x01
+#define AN877_ADC_REG_CHAN_INDEX 0x05
+#define AN877_ADC_REG_TEST_IO 0x0D
+#define AN877_ADC_REG_OUTPUT_MODE 0x14
+#define AN877_ADC_REG_OUTPUT_PHASE 0x16
+#define AN877_ADC_REG_OUTPUT_DELAY 0x17
+#define AN877_ADC_REG_VREF 0x18
+#define AN877_ADC_REG_TRANSFER 0xFF
+
+/* AN877_ADC_REG_TRANSFER */
+#define AN877_ADC_TRANSFER_SYNC 0x1
+
+/* AN877_ADC_REG_OUTPUT_MODE */
+#define AN877_ADC_OUTPUT_MODE_OFFSET_BINARY 0x0
+#define AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT 0x1
+
+/* AN877_ADC_REG_OUTPUT_PHASE */
+#define AN877_ADC_OUTPUT_EVEN_ODD_MODE_EN 0x20
+#define AN877_ADC_INVERT_DCO_CLK 0x80
+
+/* AN877_ADC_REG_TEST_IO */
+#define AN877_ADC_TESTMODE_OFF 0x0
+#define AN877_ADC_TESTMODE_PN23_SEQ 0x5
+#define AN877_ADC_TESTMODE_PN9_SEQ 0x6
+
+#define AD9647_MAX_TEST_POINTS 32
+/*
+ * Analog Devices AD9265 16-Bit, 125/105/80 MSPS ADC
+ */
+
+#define CHIPID_AD9265 0x64
+#define AD9265_DEF_OUTPUT_MODE 0x40
+#define AD9265_REG_VREF_MASK 0xC0
+
+/*
+ * Analog Devices AD9434 12-Bit, 370/500 MSPS ADC
+ */
+
+#define CHIPID_AD9434 0x6A
+#define AD9434_DEF_OUTPUT_MODE 0x00
+#define AD9434_REG_VREF_MASK 0xC0
+
+/*
+ * Analog Devices AD9467 16-Bit, 200/250 MSPS ADC
+ */
+
+#define CHIPID_AD9467 0x50
+#define AD9467_DEF_OUTPUT_MODE 0x08
+#define AD9467_REG_VREF_MASK 0x0F
+
+struct ad9467_chip_info {
+ const char *name;
+ const struct iio_chan_spec *channels;
+ const unsigned int (*scale_table)[2];
+ unsigned int id;
+ int num_scales;
+ unsigned long max_rate;
+ unsigned int default_output_mode;
+ unsigned int vref_mask;
+ unsigned int num_channels;
+ unsigned int num_lanes;
+ bool has_dco;
+};
+
+struct ad9467_state {
+ const struct ad9467_chip_info *info;
+ struct converter_backend *conv;
+ struct spi_device *spi;
+ struct clk *clk;
+ unsigned int output_mode;
+ unsigned long adc_clk;
+};
+
+/*
+ * Infer about moving to regmap (looks pretty straight)...
+ * Moreover we need to make this DMA safe
+ */
+static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
+{
+ unsigned char tbuf[2], rbuf[1];
+ int ret;
+
+ tbuf[0] = 0x80 | (reg >> 8);
+ tbuf[1] = reg & 0xFF;
+
+ ret = spi_write_then_read(spi,
+ tbuf, ARRAY_SIZE(tbuf),
+ rbuf, ARRAY_SIZE(rbuf));
+
+ if (ret < 0)
+ return ret;
+
+ return rbuf[0];
+}
+
+static int ad9467_spi_write(struct spi_device *spi, unsigned int reg,
+ unsigned int val)
+{
+ unsigned char buf[3];
+
+ buf[0] = reg >> 8;
+ buf[1] = reg & 0xFF;
+ buf[2] = val;
+
+ return spi_write(spi, buf, ARRAY_SIZE(buf));
+}
+
+static void __ad9467_get_scale(struct ad9467_state *st, int index,
+ unsigned int *val, unsigned int *val2)
+{
+ const struct iio_chan_spec *chan = &st->info->channels[0];
+ unsigned int tmp;
+
+ tmp = (st->info->scale_table[index][0] * 1000000ULL) >> chan->scan_type.realbits;
+ *val = tmp / 1000000;
+ *val2 = tmp % 1000000;
+}
+
+/* needs to check for ret codes */
+static int ad9467_get_scale(struct ad9467_state *st, int *val, int *val2)
+{
+ unsigned int i, vref_val;
+
+ vref_val = ad9467_spi_read(st->spi, AN877_ADC_REG_VREF);
+
+ vref_val &= st->info->vref_mask;
+
+ for (i = 0; i < st->info->num_scales; i++) {
+ if (vref_val == st->info->scale_table[i][1])
+ break;
+ }
+
+ if (i == st->info->num_scales)
+ return -ERANGE;
+
+ __ad9467_get_scale(st, i, val, val2);
+
+ return IIO_VAL_INT_PLUS_MICRO;
+}
+
+/* Needs mutex and check for ret codes */
+static int ad9467_set_scale(struct ad9467_state *st, int val, int val2)
+{
+ unsigned int scale_val[2];
+ unsigned int i;
+
+ if (val != 0)
+ return -EINVAL;
+
+ 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;
+
+ ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
+ st->info->scale_table[i][1]);
+ ad9467_spi_write(st->spi, AN877_ADC_REG_TRANSFER,
+ AN877_ADC_TRANSFER_SYNC);
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static int ad9467_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct ad9467_state *st = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ return ad9467_get_scale(st, val, val2);
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *val = clk_get_rate(st->clk);
+
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ad9647_calibrate_prepare(const struct ad9467_state *st)
+{
+ int ret;
+
+ ret = ad9467_spi_write(st->spi, AN877_ADC_REG_TEST_IO,
+ AN877_ADC_TESTMODE_PN9_SEQ);
+ if (ret)
+ return ret;
+
+ ret = ad9467_spi_write(st->spi, AN877_ADC_REG_TRANSFER,
+ AN877_ADC_TRANSFER_SYNC);
+ if (ret)
+ return ret;
+
+ ret = converter_test_pattern_set(st->conv, 0, CONVERTER_ADI_PRBS_9A);
+ if (ret)
+ return ret;
+
+ return converter_chan_enable(st->conv, 0);
+}
+
+static int ad9647_calibrate_stop(const struct ad9467_state *st)
+{
+ int ret;
+
+ ret = ad9467_spi_write(st->spi, AN877_ADC_REG_TEST_IO,
+ AN877_ADC_TESTMODE_OFF);
+ if (ret)
+ return ret;
+
+ ret = ad9467_spi_write(st->spi, AN877_ADC_REG_TRANSFER,
+ AN877_ADC_TRANSFER_SYNC);
+ if (ret)
+ return ret;
+
+ return converter_chan_disable(st->conv, 0);
+}
+
+static int ad9467_calibrate_apply(const struct ad9467_state *st,
+ unsigned int val)
+{
+ if (st->info->has_dco) {
+ int ret;
+
+ ret = ad9467_spi_write(st->spi, AN877_ADC_REG_OUTPUT_DELAY,
+ val);
+ if (ret)
+ return ret;
+
+ return ad9467_spi_write(st->spi, AN877_ADC_REG_TRANSFER,
+ AN877_ADC_TRANSFER_SYNC);
+ }
+
+ return converter_iodelay_set(st->conv, st->info->num_lanes, val);
+}
+
+static int ad9467_calibrate_status_check(const struct ad9467_state *st)
+{
+ struct converter_chan_status status = {0};
+ int ret;
+
+ ret = converter_chan_status_get(st->conv, 0, &status);
+ if (ret)
+ return ret;
+
+ if (status.errors)
+ return 1;
+
+ return 0;
+}
+
+static void ad9467_dump_table(const unsigned char *err_field,
+ unsigned int size, unsigned int val)
+{
+ unsigned int cnt;
+
+ for (cnt = 0; cnt < size; cnt++) {
+ if (cnt == val) {
+ pr_debug("|");
+ continue;
+ }
+
+ pr_debug("%c", err_field[cnt] ? '-' : 'o');
+ if (cnt == size / 2)
+ pr_debug("\n");
+ }
+}
+
+static int ad9467_find_optimal_point(const unsigned char *err_field,
+ unsigned int size)
+{
+ unsigned int val, cnt = 0, max_cnt = 0, max_start = 0;
+ int start = -1;
+
+ for (val = 0; val < size; val++) {
+ if (!err_field[val]) {
+ if (start == -1)
+ start = val;
+ cnt++;
+ } else {
+ if (cnt > max_cnt) {
+ max_cnt = cnt;
+ max_start = start;
+ }
+
+ start = -1;
+ cnt = 0;
+ }
+ }
+
+ if (cnt > max_cnt) {
+ max_cnt = cnt;
+ max_start = start;
+ }
+
+ if (!max_cnt)
+ return -EIO;
+
+ val = max_start + max_cnt / 2;
+ ad9467_dump_table(err_field, size, val);
+
+ return val;
+}
+
+static int ad9467_do_calibrate(const struct ad9467_state *st)
+{
+ unsigned char err_field[AD9647_MAX_TEST_POINTS * 2] = {0};
+ unsigned int max_val = AD9647_MAX_TEST_POINTS, val;
+ bool inv_range = false;
+ int ret;
+
+ ret = ad9647_calibrate_prepare(st);
+ if (ret)
+ return ret;
+retune:
+ if (st->info->has_dco) {
+ unsigned int phase = AN877_ADC_OUTPUT_EVEN_ODD_MODE_EN;
+
+ if (inv_range)
+ phase |= AN877_ADC_INVERT_DCO_CLK;
+
+ ret = ad9467_spi_write(st->spi, AN877_ADC_REG_OUTPUT_PHASE,
+ phase);
+ if (ret)
+ return ret;
+ } else {
+ if (inv_range)
+ ret = converter_sample_on_falling_edge(st->conv);
+ else
+ ret = converter_sample_on_rising_edge(st->conv);
+
+ if (ret)
+ return ret;
+ }
+
+ for (val = 0; val < max_val; val++) {
+ ret = ad9467_calibrate_apply(st, val);
+ if (ret)
+ return ret;
+
+ ret = ad9467_calibrate_status_check(st);
+ if (ret < 0)
+ return ret;
+
+ err_field[val + inv_range * max_val] = ret;
+ }
+
+ if (!inv_range) {
+ inv_range = true;
+ goto retune;
+ }
+
+ val = ad9467_find_optimal_point(err_field, sizeof(err_field));
+ if (val < 0)
+ return val;
+
+ if (val < max_val) {
+ if (st->info->has_dco)
+ ret = ad9467_spi_write(st->spi,
+ AN877_ADC_REG_OUTPUT_PHASE,
+ AN877_ADC_OUTPUT_EVEN_ODD_MODE_EN);
+ else
+ ret = converter_sample_on_rising_edge(st->conv);
+ } else {
+ val -= max_val + 1;
+ /*
+ * inv_range = true is the last test to run. Hence, there's no
+ * need to re-do any configuration
+ */
+ inv_range = false;
+ }
+
+ if (st->info->has_dco)
+ dev_dbg(&st->spi->dev,
+ " %s DCO 0x%X CLK %lu Hz\n", inv_range ? "INVERT" : "",
+ val, st->adc_clk);
+ else
+ dev_dbg(&st->spi->dev,
+ " %s IDELAY 0x%x\n", inv_range ? "INVERT" : "", val);
+
+ ret = ad9647_calibrate_stop(st);
+ if (ret)
+ return ret;
+
+ /* finally apply the optimal value */
+ return ad9467_calibrate_apply(st, val);
+}
+
+static int ad9467_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct ad9467_state *st = iio_priv(indio_dev);
+ long r_clk;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ return ad9467_set_scale(st, val, val2);
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+
+ r_clk = clk_round_rate(st->clk, val);
+ if (r_clk < 0 || r_clk > st->info->max_rate) {
+ dev_warn(&st->spi->dev,
+ "Error setting ADC sample rate %ld", r_clk);
+ iio_device_release_direct_mode(indio_dev);
+ return -EINVAL;
+ }
+
+ if (st->adc_clk == r_clk) {
+ iio_device_release_direct_mode(indio_dev);
+ return 0;
+ }
+
+ ret = clk_set_rate(st->clk, r_clk);
+ if (ret) {
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
+ }
+
+ st->adc_clk = r_clk;
+ ret = ad9467_do_calibrate(st);
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ad9467_read_available(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long mask)
+{
+ struct ad9467_state *st = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ *vals = (const int *)st->info->scale_table;
+ *type = IIO_VAL_INT_PLUS_MICRO;
+ /* Values are stored in a 2D matrix */
+ *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 = converter_chan_enable(st->conv, c);
+ else
+ ret = converter_chan_disable(st->conv, c);
+
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int ad9467_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+ unsigned int writeval, unsigned int *readval)
+{
+ struct ad9467_state *st = iio_priv(indio_dev);
+ struct spi_device *spi = st->spi;
+ int ret;
+
+ if (!readval) {
+ ret = ad9467_spi_write(spi, reg, writeval);
+ if (ret)
+ return ret;
+
+ return ad9467_spi_write(spi, AN877_ADC_REG_TRANSFER,
+ AN877_ADC_TRANSFER_SYNC);
+ }
+
+ ret = ad9467_spi_read(spi, reg);
+ if (ret < 0)
+ return ret;
+
+ *readval = ret;
+
+ return 0;
+}
+
+/* missing available scales... */
+#define AD9467_CHAN(_chan, _si, _bits, _sign) \
+{ \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .channel = _chan, \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+ .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE), \
+ .scan_index = _si, \
+ .scan_type = { \
+ .sign = _sign, \
+ .realbits = _bits, \
+ .storagebits = 16, \
+ }, \
+}
+
+static const struct iio_chan_spec ad9434_channels[] = {
+ AD9467_CHAN(0, 0, 12, 'S'),
+};
+
+static const struct iio_chan_spec ad9467_channels[] = {
+ AD9467_CHAN(0, 0, 16, 'S'),
+};
+
+static const unsigned int ad9265_scale_table[][2] = {
+ {1250, 0x00}, {1500, 0x40}, {1750, 0x80}, {2000, 0xC0},
+};
+
+static const unsigned int ad9434_scale_table[][2] = {
+ {1600, 0x1C}, {1580, 0x1D}, {1550, 0x1E}, {1520, 0x1F}, {1500, 0x00},
+ {1470, 0x01}, {1440, 0x02}, {1420, 0x03}, {1390, 0x04}, {1360, 0x05},
+ {1340, 0x06}, {1310, 0x07}, {1280, 0x08}, {1260, 0x09}, {1230, 0x0A},
+ {1200, 0x0B}, {1180, 0x0C},
+};
+
+static const unsigned int ad9467_scale_table[][2] = {
+ {2000, 0}, {2100, 6}, {2200, 7},
+ {2300, 8}, {2400, 9}, {2500, 10},
+};
+
+static const struct ad9467_chip_info ad9467_chip_tbl = {
+ .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,
+ .num_lanes = 8,
+};
+
+static const struct ad9467_chip_info ad9265_chip_tbl = {
+ .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,
+ .has_dco = true,
+};
+
+static const struct ad9467_chip_info ad9434_chip_tbl = {
+ .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,
+ .num_lanes = 6,
+};
+
+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_available,
+};
+
+static int ad9467_reset(struct device *dev)
+{
+ struct gpio_desc *gpio;
+
+ gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(gpio))
+ return PTR_ERR(gpio);
+ if (!gpio)
+ return 0;
+
+ fsleep(1);
+ gpiod_set_value_cansleep(gpio, 0);
+ fsleep(10);
+
+ return 0;
+}
+
+/*
+ * Also candidate for a generic helper...
+ *
+ * This is something that I don't like much because, hardwarewise, the dma is
+ * connected to the backend device so it would make sense for the dma
+ * properties to be in the platform device rather than the frontend. However,
+ * detaching the IIO DMA buffer like that from the place where the IIO
+ * device is handled would feel equally odd and, while doable, it would
+ * require some hacking and new converter ops to make sure that resources
+ * lifetime feel right (so also export the non devm_ @iio_dmaengine_buffer_alloc()).
+ */
+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_outputmode_set(struct spi_device *spi, unsigned int mode)
+{
+ int ret;
+
+ ret = ad9467_spi_write(spi, AN877_ADC_REG_OUTPUT_MODE, mode);
+ if (ret < 0)
+ return ret;
+
+ return ad9467_spi_write(spi, AN877_ADC_REG_TRANSFER,
+ AN877_ADC_TRANSFER_SYNC);
+}
+
+static int ad9467_channels_setup(const struct ad9467_state *st, bool test_mode)
+{
+ struct converter_data_fmt data;
+ unsigned int c, mode;
+ int ret;
+
+ if (test_mode) {
+ data.enable = false;
+ mode = st->info->default_output_mode;
+ } else {
+ mode = st->info->default_output_mode |
+ AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT;
+ data.type = CONVERTER_TWOS_COMPLEMENT;
+ data.sign_extend = true;
+ data.enable = true;
+ }
+
+ ret = ad9467_outputmode_set(st->spi, mode);
+ if (ret)
+ return ret;
+
+ for (c = 0; c < st->info->num_channels; c++) {
+ ret = converter_data_format_set(st->conv, c, &data);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int ad9467_calibrate(const struct ad9467_state *st)
+{
+ int ret;
+
+ ret = ad9467_channels_setup(st, true);
+ if (ret)
+ return ret;
+
+ ret = ad9467_do_calibrate(st);
+ if (ret)
+ return ret;
+
+ return ad9467_channels_setup(st, false);
+}
+
+static int ad9467_init(struct converter_frontend *frontend, struct device *dev)
+{
+ struct spi_device *spi = to_spi_device(dev);
+ struct iio_dev *indio_dev;
+ struct ad9467_state *st;
+ unsigned int id;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ st = iio_priv(indio_dev);
+ st->spi = spi;
+
+ st->info = spi_get_device_match_data(spi);
+ if (!st->info)
+ return -EINVAL;
+
+ st->conv = converter_get(frontend, NULL);
+ if (IS_ERR(st->conv))
+ return PTR_ERR(st->conv);
+
+ st->clk = devm_clk_get_enabled(dev, "adc-clk");
+ if (IS_ERR(st->clk))
+ return PTR_ERR(st->clk);
+
+ st->adc_clk = clk_get_rate(st->clk);
+
+ ret = ad9467_reset(dev);
+ if (ret)
+ return ret;
+
+ id = ad9467_spi_read(spi, AN877_ADC_REG_CHIP_ID);
+ if (id != st->info->id) {
+ dev_err(dev, "Mismatch CHIP_ID, got 0x%X, expected 0x%X\n",
+ id, st->info->id);
+ return -ENODEV;
+ }
+
+ 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;
+
+ ret = converter_enable(st->conv);
+ if (ret)
+ return ret;
+
+ ret = ad9467_calibrate(st);
+ if (ret)
+ return ret;
+ ret = devm_iio_device_register(dev, indio_dev);
+ if (ret)
+ return ret;
+
+ converter_add_direct_reg_access(st->conv, indio_dev);
+
+ return 0;
+}
+
+static const struct frontend_ops ad9467_ops = {
+ .frontend_init = ad9467_init,
+};
+
+static int ad9467_probe(struct spi_device *spi)
+{
+ return converter_frontend_add(&spi->dev, &ad9467_ops);
+}
+
+/*
+ * It actually matters to remove the frontend in the .remove() hook. This means
+ * that all the converters (and the frontend) will be tear down before running
+ * any specific devres cleanup (at the driver core level). What this all means is
+ * that we can use devm_ apis in .frontend_init() and being sure those resources
+ * will be released after the backend resources and before any devm_* used
+ * in .probe().
+ */
+static void ad9467_remove(struct spi_device *spi)
+{
+ converter_del(&spi->dev);
+}
+
+static const struct of_device_id ad9467_of_match[] = {
+ { .compatible = "adi,ad9265", .data = &ad9265_chip_tbl, },
+ { .compatible = "adi,ad9434", .data = &ad9434_chip_tbl, },
+ { .compatible = "adi,ad9467-new", .data = &ad9467_chip_tbl, },
+ {}
+};
+MODULE_DEVICE_TABLE(of, ad9467_of_match);
+
+static const struct spi_device_id ad9467_ids[] = {
+ { "ad9265", (kernel_ulong_t)&ad9265_chip_tbl },
+ { "ad9434", (kernel_ulong_t)&ad9434_chip_tbl },
+ { "ad9467-new", (kernel_ulong_t)&ad9467_chip_tbl },
+ {}
+};
+MODULE_DEVICE_TABLE(spi, ad9467_ids);
+
+static struct spi_driver ad9467_driver = {
+ .driver = {
+ .name = "ad9467",
+ .of_match_table = ad9467_of_match,
+ },
+ .probe = ad9467_probe,
+ .remove = ad9467_remove,
+ .id_table = ad9467_ids,
+};
+module_spi_driver(ad9467_driver);
+
+MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD9467 ADC driver");
+MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS(IIO_CONVERTER);
--
2.41.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [RFC PATCH 3/3] iio: adc: adi-axi-adc: add based on new converter framework
2023-08-04 14:53 [RFC PATCH 0/3] Add converter framework Nuno Sa
2023-08-04 14:53 ` [RFC PATCH 1/3] iio: addac: add new " Nuno Sa
2023-08-04 14:53 ` [RFC PATCH 2/3] iio: adc: ad9647: add based on " Nuno Sa
@ 2023-08-04 14:53 ` Nuno Sa
2023-08-30 17:15 ` Jonathan Cameron
2023-08-30 16:29 ` [RFC PATCH 0/3] Add " Jonathan Cameron
3 siblings, 1 reply; 19+ messages in thread
From: Nuno Sa @ 2023-08-04 14:53 UTC (permalink / raw)
To: linux-iio; +Cc: Jonathan Cameron
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
drivers/iio/adc/adi-axi-adc-new.c | 405 ++++++++++++++++++++++++++++++
1 file changed, 405 insertions(+)
create mode 100644 drivers/iio/adc/adi-axi-adc-new.c
diff --git a/drivers/iio/adc/adi-axi-adc-new.c b/drivers/iio/adc/adi-axi-adc-new.c
new file mode 100644
index 000000000000..7ee24f765d07
--- /dev/null
+++ b/drivers/iio/adc/adi-axi-adc-new.c
@@ -0,0 +1,405 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices Generic AXI ADC IP core
+ * Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
+ *
+ * Copyright 2012-2023 Analog Devices Inc.
+ */
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/limits.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#include <linux/fpga/adi-axi-common.h>
+#include <linux/iio/addc/converter.h>
+
+/*
+ * Register definitions:
+ * https://wiki.analog.com/resources/fpga/docs/axi_adc_ip#register_map
+ */
+
+/* ADC systhesis configuration*/
+#define AXI_ADC_REG_CONFIG 0x000c
+#define AXI_ADC_DATAFORMAT_DISABLE_MASK BIT(2)
+
+/* ADC controls */
+#define AXI_ADC_REG_RSTN 0x0040
+#define AXI_ADC_RSTN_RESET_MASK GENMASK(1, 0)
+#define AXI_ADC_RSTN_MMCM_RSTN BIT(1)
+#define AXI_ADC_RSTN_RSTN BIT(0)
+
+#define AXI_ADC_REG_CTRL 0x0044
+#define AXI_ADC_CTRL_DDR_EDGESEL_MASK BIT(1)
+
+/* ADC Channel controls */
+#define AXI_ADC_REG_CHAN_CTRL(c) (0x0400 + (c) * 0x40)
+#define AXI_ADC_CHAN_CTRL_FMT_MASK GENMASK(6, 4)
+#define AXI_ADC_CHAN_CTRL_FMT_EN BIT(0)
+#define AXI_ADC_CHAN_CTRL_FMT_BIN_OFF BIT(1)
+#define AXI_ADC_CHAN_CTRL_FMT_SIGEXT BIT(2)
+#define AXI_ADC_CHAN_CTRL_EN_MASK BIT(0)
+
+#define AXI_ADC_REG_CHAN_STATUS(c) (0x0404 + (c) * 0x40)
+#define AXI_ADC_CHAN_STAT_PN_MASK GENMASK(2, 1)
+
+#define AXI_ADC_REG_CHAN_CTRL_3(c) (0x0418 + (c) * 0x40)
+#define AXI_ADC_CHAN_PN_SEL_MASK GENMASK(19, 16)
+
+/* IO Delays */
+#define AXI_ADC_REG_DELAY(l) (0x0800 + (l) * 0x4)
+#define AXI_ADC_DELAY_CTRL_MASK GENMASK(4, 0)
+
+enum {
+ AXI_ADC_PN9A,
+ AXI_ADC_PN23A,
+ AXI_ADC_PN7 = 0x4,
+ AXI_ADC_PN15,
+ AXI_ADC_PN23,
+ AXI_ADC_PN31,
+ AXI_ADC_PNX = 0x9,
+ AXI_ADC_RAMP_NIBBLE,
+ AXI_ADC_RAMP_16,
+};
+
+struct axi_adc_state {
+ struct regmap *regmap;
+ /* Protect against concurrent access to the device registers */
+ struct mutex lock;
+ struct device *dev;
+ u32 capabilities;
+};
+
+static int axi_adc_iodelay_set(struct converter_backend *conv,
+ unsigned int num_lanes, unsigned int delay)
+{
+ struct axi_adc_state *st = converter_get_drvdata(conv);
+ unsigned int l;
+ u32 val;
+ int ret;
+
+ if (delay > FIELD_MAX(AXI_ADC_DELAY_CTRL_MASK))
+ return -EINVAL;
+
+ mutex_lock(&st->lock);
+ for (l = 0; l < num_lanes; l++) {
+ ret = regmap_update_bits(st->regmap, AXI_ADC_REG_DELAY(l),
+ AXI_ADC_DELAY_CTRL_MASK, delay);
+ if (ret)
+ break;
+ /*
+ * If a readback is ~0, that means there are issues with the
+ * delay_clk
+ */
+ ret = regmap_read(st->regmap, AXI_ADC_REG_DELAY(l), &val);
+ if (val == U32_MAX) {
+ ret = -EIO;
+ break;
+ }
+ }
+ mutex_unlock(&st->lock);
+
+ return ret;
+}
+
+static int axi_adc_ddr_edge_set(struct converter_backend *conv,
+ enum converter_edge edge)
+{
+ struct axi_adc_state *st = converter_get_drvdata(conv);
+
+ if (edge == CONVERTER_RISING_EDGE_SAMPLE)
+ return regmap_clear_bits(st->regmap, AXI_ADC_REG_CTRL,
+ AXI_ADC_CTRL_DDR_EDGESEL_MASK);
+ if (edge == CONVERTER_FALLING_EDGE_SAMPLE)
+ return regmap_set_bits(st->regmap, AXI_ADC_REG_CTRL,
+ AXI_ADC_CTRL_DDR_EDGESEL_MASK);
+
+ return -EINVAL;
+}
+
+static const struct converter_test_pattern_xlate axi_adc_test_pattern[] = {
+ {CONVERTER_PRBS_7, AXI_ADC_PN7},
+ {CONVERTER_PRBS_15, AXI_ADC_PN15},
+ {CONVERTER_PRBS_15, AXI_ADC_PN15},
+ {CONVERTER_PRBS_23, AXI_ADC_PN23},
+ {CONVERTER_PRBS_31, AXI_ADC_PN31},
+ {CONVERTER_ADI_PRBS_9A, AXI_ADC_PN9A},
+ {CONVERTER_ADI_PRBS_23A, AXI_ADC_PN23A},
+ {CONVERTER_ADI_PRBS_X, AXI_ADC_PNX},
+ {CONVERTER_RAMP_NIBBLE, AXI_ADC_RAMP_NIBBLE},
+ {CONVERTER_RAMP_16, AXI_ADC_RAMP_16},
+};
+
+static int axi_adc_test_pattern_set(struct converter_backend *conv,
+ unsigned int chan,
+ enum converter_test_pattern pattern)
+{
+ struct axi_adc_state *st = converter_get_drvdata(conv);
+ u32 val;
+
+ val = converter_test_pattern_xlate(pattern, axi_adc_test_pattern);
+ if (val < 0)
+ return val;
+
+ return regmap_update_bits(st->regmap, AXI_ADC_REG_CHAN_CTRL_3(chan),
+ AXI_ADC_CHAN_PN_SEL_MASK,
+ FIELD_PREP(AXI_ADC_CHAN_PN_SEL_MASK, val));
+}
+
+static int axi_adc_chan_status_get(struct converter_backend *conv,
+ unsigned int chan,
+ struct converter_chan_status *status)
+{
+ struct axi_adc_state *st = converter_get_drvdata(conv);
+ int ret;
+ u32 val;
+
+ mutex_lock(&st->lock);
+
+ /* reset test bits by setting them */
+ ret = regmap_set_bits(st->regmap, AXI_ADC_REG_CHAN_STATUS(chan),
+ AXI_ADC_CHAN_STAT_PN_MASK);
+ if (ret)
+ goto out_unlock;
+
+ fsleep(5000);
+
+ ret = regmap_read(st->regmap, AXI_ADC_REG_CHAN_STATUS(chan), &val);
+ if (ret)
+ goto out_unlock;
+
+ mutex_unlock(&st->lock);
+
+ if (AXI_ADC_CHAN_STAT_PN_MASK & val)
+ status->errors = true;
+
+ return 0;
+
+out_unlock:
+ mutex_unlock(&st->lock);
+ return ret;
+}
+
+static int axi_adc_chan_enable(struct converter_backend *conv,
+ unsigned int chan)
+{
+ struct axi_adc_state *st = converter_get_drvdata(conv);
+
+ return regmap_set_bits(st->regmap, AXI_ADC_REG_CHAN_CTRL(chan),
+ AXI_ADC_CHAN_CTRL_EN_MASK);
+}
+
+static int axi_adc_chan_disable(struct converter_backend *conv,
+ unsigned int chan)
+{
+ struct axi_adc_state *st = converter_get_drvdata(conv);
+
+ return regmap_clear_bits(st->regmap, AXI_ADC_REG_CHAN_CTRL(chan),
+ AXI_ADC_CHAN_CTRL_EN_MASK);
+}
+
+static int axi_adc_data_format_set(struct converter_backend *conv,
+ unsigned int chan,
+ const struct converter_data_fmt *data)
+{
+ struct axi_adc_state *st = converter_get_drvdata(conv);
+ u32 val = 0;
+
+ if (FIELD_GET(AXI_ADC_DATAFORMAT_DISABLE_MASK, st->capabilities))
+ /* data format not available */
+ return -ENOTSUPP;
+
+ if (!data->enable)
+ return regmap_clear_bits(st->regmap,
+ AXI_ADC_REG_CHAN_CTRL(chan),
+ AXI_ADC_CHAN_CTRL_FMT_MASK);
+
+ val = FIELD_PREP(AXI_ADC_CHAN_CTRL_FMT_MASK, AXI_ADC_CHAN_CTRL_FMT_EN);
+ if (data->sign_extend)
+ val |= FIELD_PREP(AXI_ADC_CHAN_CTRL_FMT_MASK,
+ AXI_ADC_CHAN_CTRL_FMT_SIGEXT);
+
+ if (data->type == CONVERTER_OFFSET_BINARY)
+ val |= FIELD_PREP(AXI_ADC_CHAN_CTRL_FMT_MASK,
+ AXI_ADC_CHAN_CTRL_FMT_BIN_OFF);
+
+ return regmap_update_bits(st->regmap, AXI_ADC_REG_CHAN_CTRL(chan),
+ AXI_ADC_CHAN_CTRL_FMT_MASK, val);
+}
+
+static void __axi_adc_disable(const struct axi_adc_state *st)
+{
+ regmap_clear_bits(st->regmap, AXI_ADC_REG_RSTN,
+ AXI_ADC_RSTN_RESET_MASK);
+}
+
+static int __axi_adc_enable(const struct axi_adc_state *st)
+{
+ return regmap_set_bits(st->regmap, AXI_ADC_REG_RSTN,
+ AXI_ADC_RSTN_RESET_MASK);
+}
+
+static int axi_adc_enable(struct converter_backend *conv)
+{
+ return __axi_adc_enable(converter_get_drvdata(conv));
+}
+
+static void axi_adc_disable(struct converter_backend *conv)
+{
+ __axi_adc_disable(converter_get_drvdata(conv));
+}
+
+static int axi_adc_reset(struct axi_adc_state *st)
+{
+ int ret;
+
+ __axi_adc_disable(st);
+ fsleep(10);
+ ret = __axi_adc_enable(st);
+ if (ret)
+ return ret;
+
+ fsleep(10);
+ return 0;
+}
+
+static const struct regmap_config axi_adc_regmap_config = {
+ .val_bits = 32,
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .max_register = 0x0800,
+};
+
+static int axi_adc_generic_init(struct converter_backend *conv,
+ struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ unsigned int ver, *expected_ver, ret;
+ struct axi_adc_state *st;
+ void __iomem *base;
+ struct clk *clk;
+
+ st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
+ if (!st)
+ return -ENOMEM;
+
+ st->dev = dev;
+
+ expected_ver = (unsigned int *)device_get_match_data(dev);
+ if (!expected_ver)
+ return -ENODEV;
+
+ base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ st->regmap = devm_regmap_init_mmio(dev, base, &axi_adc_regmap_config);
+ if (IS_ERR(st->regmap))
+ return PTR_ERR(st->regmap);
+
+ converter_set_drvdata(conv, st);
+ converter_set_regmap(conv, st->regmap);
+
+ clk = devm_clk_get_enabled(&pdev->dev, NULL);
+ if (IS_ERR(clk)) {
+ dev_err(&pdev->dev, "clk_get failed with %ld\n", PTR_ERR(clk));
+ return PTR_ERR(clk);
+ }
+
+ ret = axi_adc_reset(st);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(st->regmap, ADI_AXI_REG_VERSION, &ver);
+ if (ret)
+ return ret;
+
+ 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(*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;
+ }
+
+ /* fetch synthesis capabilities */
+ ret = regmap_read(st->regmap, AXI_ADC_REG_CONFIG, &st->capabilities);
+ if (ret)
+ return ret;
+
+ dev_dbg(&pdev->dev, "AXI ADC IP core (%d.%.2d.%c) up\n",
+ ADI_AXI_PCORE_VER_MAJOR(ver),
+ ADI_AXI_PCORE_VER_MINOR(ver),
+ ADI_AXI_PCORE_VER_PATCH(ver));
+
+ /* up to the frontend to explicitly enable us */
+ __axi_adc_disable(st);
+ mutex_init(&st->lock);
+ return 0;
+}
+
+static const struct converter_ops adi_axi_adc_generic = {
+ .backend_init = axi_adc_generic_init,
+ .enable = axi_adc_enable,
+ .disable = axi_adc_disable,
+ .data_format_set = axi_adc_data_format_set,
+ .test_pattern_set = axi_adc_test_pattern_set,
+ .chan_enable = axi_adc_chan_enable,
+ .chan_disable = axi_adc_chan_disable,
+ .iodelay_set = axi_adc_iodelay_set,
+ .sample_edge_select = axi_adc_ddr_edge_set,
+ .chan_status = axi_adc_chan_status_get,
+};
+
+static int axi_adc_probe(struct platform_device *pdev)
+{
+ return converter_add(&pdev->dev, &adi_axi_adc_generic);
+}
+
+/*
+ * It actually matters to remove the converter in the .remove() hook. This means
+ * that the all the converters (an the frontend) will be tear down before running
+ * any specific devres cleanup (at the driver core level). What this all means is
+ * that we can use devm_ apis in .backend_init() and being sure those resources
+ * will be released before the frontend resources and before any devm_* used
+ * in .probe().
+ */
+static int axi_adc_remove(struct platform_device *pdev)
+{
+ converter_del(&pdev->dev);
+ return 0;
+}
+
+static unsigned int axi_adc_10_0_a = ADI_AXI_PCORE_VER(10, 0, 'a');
+
+/* Match table for of_platform binding */
+static const struct of_device_id axi_adc_of_match[] = {
+ { .compatible = "adi,axi-adc-10.0.a-new", .data = &axi_adc_10_0_a },
+ { /* end of list */ }
+};
+MODULE_DEVICE_TABLE(of, axi_adc_of_match);
+
+static struct platform_driver axi_adc_driver = {
+ .driver = {
+ .name = "axi-adc",
+ .of_match_table = axi_adc_of_match,
+ },
+ .probe = axi_adc_probe,
+ .remove = axi_adc_remove,
+};
+module_platform_driver(axi_adc_driver);
+
+MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>");
+MODULE_DESCRIPTION("Analog Devices Generic AXI ADC IP core driver");
+MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS(IIO_CONVERTER);
--
2.41.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [RFC PATCH 0/3] Add converter framework
2023-08-04 14:53 [RFC PATCH 0/3] Add converter framework Nuno Sa
` (2 preceding siblings ...)
2023-08-04 14:53 ` [RFC PATCH 3/3] iio: adc: adi-axi-adc: add based on new " Nuno Sa
@ 2023-08-30 16:29 ` Jonathan Cameron
2023-08-30 16:30 ` Jonathan Cameron
2023-08-31 8:20 ` Nuno Sá
3 siblings, 2 replies; 19+ messages in thread
From: Jonathan Cameron @ 2023-08-30 16:29 UTC (permalink / raw)
To: Nuno Sa; +Cc: linux-iio, Jonathan Cameron
On Fri, 4 Aug 2023 16:53:38 +0200
Nuno Sa <nuno.sa@analog.com> wrote:
> This is the initial RFC following the discussion in [1]. I'm aware this is
> by no means ready for inclusion and it's not even compilable since in
> the RFC I did not included the patch to add component_compare_fwnode()
> and component_release_fwnode().
Whilst I haven't read this through yet, I suspect Olivier will be able to
offer some insight on some of this and likewise you may be able to
point out pitfalls etc in his series (I see you did some review already :)
https://lore.kernel.org/linux-iio/20230727150324.1157933-1-olivier.moysan@foss.st.com/
Both are about multiple interacting components of an overall datapath.
Whether there is commonality isn't yet clear to me.
>
> The goal is to have a first feel on the
> direction of the framework so that if I need to drastically change it,
> better do it now. The RFC also brings the ad9647 and the axi_adc core to
> the same functionality we have now upstream with one extra fundamental
> feature that is calibrating the digital interface. This would be very
> difficult to do with the current design. Note that I don't expect any
> review on those drivers (rather than things related to the framework).
>
> I also want to bring up a couple of things that I've
> been thinking that I'm yet not sure about (so some feedback might make
> mind in one direction or another).
>
> 1) Im yet not sure if I should have different compatibles in the
> axi-adc-core driver. Note this soft core is a generic core and for every
> design (where the frontend device changes or has subtle changes like
> different number of data paths) there are subtle changes. So, the number
> of channels might be different, the available test patterns might be
> different, some ops might be available for some designs but not for
> others, etc...
I don't suppose there is any chance Analog can make at least some of this
discoverable from the hardware? Capability registers etc in the long
run. Can't fix what is already out there.
> With a different compatible we could fine tune
> those differences (with a chip_info like structure) and pass some const
> converter_config to the framework that would allow it to do more safety
> checks and potentially reduce the number of converter_ops.
> OTOH, starting to add all of these compatibles might become messy in the
> long run and will likely mean that we'll always have to change both
> drivers in order to support a new frontend. And the frontend devices
> should really be the ones having all the "knowledge" to configure the
> soft core even if it means more converter_ops (though devicetree might
> help as some features are really HW dependent). I more inclined to just
> leave things as-is in the RFC.
I'm fine with putting this stuff in DT where possible.
>
> 2) There are some IIO attributes (like scale, frequency, etc) that might
> be implemented in the soft cores. I still didn't made my mind if I should just
> have a catch all read_raw() and write_raw() converter_ops or more fine
> tuned ops. Having the catch all reduces the number of ops but also makes
> it more easier to add stuff that ends up being not used anymore and then
> forgotten. There are also cases (eg: setting sampling frequency) where
> we might need to apply settings in both the frontend and the backend
> devices which means having the catch all write_raw() would be more
> awkward in these case. I'm a bit more inclined to the more specific ops.
It's the kernel - we can always change the internal API later as long as we
don't touch the user space part. Go with your gut feeling today and
if it changes this sort of refactor usually isn't that bad.
>
> 3) I also placed this in addac as this is mostly used in high speed DACs
> and ADCs but maybe we should just have it in the top level directory
> just in case this is started to be used in different type of devices?
Easy to change later so right now I don't care where it is.
>
> 4) Some function and data names are also starting to become very big so
> if there are no objections I will move all to use conv instead of full
> converter. Or maybe something a bit more generic (converter is a bit specific
> I know)?
Abrv. fine as long as consistenty used.
>
> I would love to hear some ideas about the above...
>
> Anyways, I should also mention that the only visible ABI breakage is in
> the IIO device name. Before it was named "adi-axi-adc" and now it's
> "ad9647" which is what makes sense actually. With the current approach
> we would not be able to actually distinguish between designs.
Given that will probably only result in support calls to ADI I'm fine with
that breakage. :)
>
> So my plan for the actual series will be to just add the framework and migrate
> the current drivers to it with the same functionality as they have now (not
> sure if it will be viable to migrate the drivers in a way each commit is
> functional - unless we convert both drivers in one commit).
Make sure they build. It's fine to end up with some non functional stubs
during such a migration.
> After that
> point, I will start adding all the missing features (and devices) to the
> ad9467 driver. To note that I also plan to include the axi-dac driver in
> the first series and that will require IIO DMA output buffer support
> so we might need to cherry-pick those patches from Paul's DMABUF series.
As mentioned in reply to that, I'm fine with you carrying Paul's miniseries
in your patch set to make this all easy to manage.
Jonathan
>
> Thanks!
> - Nuno Sá
>
> [1]: https://lore.kernel.org/linux-iio/dac3967805d7ddbd4653ead6d50e614844e0b70b.camel@gmail.com/
>
> Nuno Sa (3):
> iio: addac: add new converter framework
> iio: adc: ad9647: add based on converter framework
> iio: adc: adi-axi-adc: add based on new converter framework
>
> drivers/iio/adc/ad9467_new.c | 830 ++++++++++++++++++++++++++++
> drivers/iio/adc/adi-axi-adc-new.c | 405 ++++++++++++++
> drivers/iio/addac/converter.c | 547 ++++++++++++++++++
> include/linux/iio/addac/converter.h | 485 ++++++++++++++++
> 4 files changed, 2267 insertions(+)
> create mode 100644 drivers/iio/adc/ad9467_new.c
> create mode 100644 drivers/iio/adc/adi-axi-adc-new.c
> create mode 100644 drivers/iio/addac/converter.c
> create mode 100644 include/linux/iio/addac/converter.h
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC PATCH 0/3] Add converter framework
2023-08-30 16:29 ` [RFC PATCH 0/3] Add " Jonathan Cameron
@ 2023-08-30 16:30 ` Jonathan Cameron
2023-08-31 8:20 ` Nuno Sá
1 sibling, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2023-08-30 16:30 UTC (permalink / raw)
To: Nuno Sa; +Cc: linux-iio, Jonathan Cameron, Olivier Moysan
On Wed, 30 Aug 2023 17:29:03 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> On Fri, 4 Aug 2023 16:53:38 +0200
> Nuno Sa <nuno.sa@analog.com> wrote:
>
> > This is the initial RFC following the discussion in [1]. I'm aware this is
> > by no means ready for inclusion and it's not even compilable since in
> > the RFC I did not included the patch to add component_compare_fwnode()
> > and component_release_fwnode().
>
> Whilst I haven't read this through yet, I suspect Olivier will be able to
> offer some insight on some of this and likewise you may be able to
> point out pitfalls etc in his series (I see you did some review already :)
>
> https://lore.kernel.org/linux-iio/20230727150324.1157933-1-olivier.moysan@foss.st.com/
>
> Both are about multiple interacting components of an overall datapath.
> Whether there is commonality isn't yet clear to me.
Works better if I actually remember to CC Olivier.
+CC Olivier!
>
> >
> > The goal is to have a first feel on the
> > direction of the framework so that if I need to drastically change it,
> > better do it now. The RFC also brings the ad9647 and the axi_adc core to
> > the same functionality we have now upstream with one extra fundamental
> > feature that is calibrating the digital interface. This would be very
> > difficult to do with the current design. Note that I don't expect any
> > review on those drivers (rather than things related to the framework).
> >
> > I also want to bring up a couple of things that I've
> > been thinking that I'm yet not sure about (so some feedback might make
> > mind in one direction or another).
> >
> > 1) Im yet not sure if I should have different compatibles in the
> > axi-adc-core driver. Note this soft core is a generic core and for every
> > design (where the frontend device changes or has subtle changes like
> > different number of data paths) there are subtle changes. So, the number
> > of channels might be different, the available test patterns might be
> > different, some ops might be available for some designs but not for
> > others, etc...
>
> I don't suppose there is any chance Analog can make at least some of this
> discoverable from the hardware? Capability registers etc in the long
> run. Can't fix what is already out there.
>
> > With a different compatible we could fine tune
> > those differences (with a chip_info like structure) and pass some const
> > converter_config to the framework that would allow it to do more safety
> > checks and potentially reduce the number of converter_ops.
> > OTOH, starting to add all of these compatibles might become messy in the
> > long run and will likely mean that we'll always have to change both
> > drivers in order to support a new frontend. And the frontend devices
> > should really be the ones having all the "knowledge" to configure the
> > soft core even if it means more converter_ops (though devicetree might
> > help as some features are really HW dependent). I more inclined to just
> > leave things as-is in the RFC.
>
> I'm fine with putting this stuff in DT where possible.
>
> >
> > 2) There are some IIO attributes (like scale, frequency, etc) that might
> > be implemented in the soft cores. I still didn't made my mind if I should just
> > have a catch all read_raw() and write_raw() converter_ops or more fine
> > tuned ops. Having the catch all reduces the number of ops but also makes
> > it more easier to add stuff that ends up being not used anymore and then
> > forgotten. There are also cases (eg: setting sampling frequency) where
> > we might need to apply settings in both the frontend and the backend
> > devices which means having the catch all write_raw() would be more
> > awkward in these case. I'm a bit more inclined to the more specific ops.
>
> It's the kernel - we can always change the internal API later as long as we
> don't touch the user space part. Go with your gut feeling today and
> if it changes this sort of refactor usually isn't that bad.
>
> >
> > 3) I also placed this in addac as this is mostly used in high speed DACs
> > and ADCs but maybe we should just have it in the top level directory
> > just in case this is started to be used in different type of devices?
>
> Easy to change later so right now I don't care where it is.
>
> >
> > 4) Some function and data names are also starting to become very big so
> > if there are no objections I will move all to use conv instead of full
> > converter. Or maybe something a bit more generic (converter is a bit specific
> > I know)?
>
> Abrv. fine as long as consistenty used.
>
> >
> > I would love to hear some ideas about the above...
> >
> > Anyways, I should also mention that the only visible ABI breakage is in
> > the IIO device name. Before it was named "adi-axi-adc" and now it's
> > "ad9647" which is what makes sense actually. With the current approach
> > we would not be able to actually distinguish between designs.
>
> Given that will probably only result in support calls to ADI I'm fine with
> that breakage. :)
>
> >
> > So my plan for the actual series will be to just add the framework and migrate
> > the current drivers to it with the same functionality as they have now (not
> > sure if it will be viable to migrate the drivers in a way each commit is
> > functional - unless we convert both drivers in one commit).
> Make sure they build. It's fine to end up with some non functional stubs
> during such a migration.
>
> > After that
> > point, I will start adding all the missing features (and devices) to the
> > ad9467 driver. To note that I also plan to include the axi-dac driver in
> > the first series and that will require IIO DMA output buffer support
> > so we might need to cherry-pick those patches from Paul's DMABUF series.
> As mentioned in reply to that, I'm fine with you carrying Paul's miniseries
> in your patch set to make this all easy to manage.
>
> Jonathan
>
> >
> > Thanks!
> > - Nuno Sá
> >
> > [1]: https://lore.kernel.org/linux-iio/dac3967805d7ddbd4653ead6d50e614844e0b70b.camel@gmail.com/
> >
> > Nuno Sa (3):
> > iio: addac: add new converter framework
> > iio: adc: ad9647: add based on converter framework
> > iio: adc: adi-axi-adc: add based on new converter framework
> >
> > drivers/iio/adc/ad9467_new.c | 830 ++++++++++++++++++++++++++++
> > drivers/iio/adc/adi-axi-adc-new.c | 405 ++++++++++++++
> > drivers/iio/addac/converter.c | 547 ++++++++++++++++++
> > include/linux/iio/addac/converter.h | 485 ++++++++++++++++
> > 4 files changed, 2267 insertions(+)
> > create mode 100644 drivers/iio/adc/ad9467_new.c
> > create mode 100644 drivers/iio/adc/adi-axi-adc-new.c
> > create mode 100644 drivers/iio/addac/converter.c
> > create mode 100644 include/linux/iio/addac/converter.h
> >
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 0/3] Add converter framework
2023-08-30 16:29 ` [RFC PATCH 0/3] Add " Jonathan Cameron
2023-08-30 16:30 ` Jonathan Cameron
@ 2023-08-31 8:20 ` Nuno Sá
2023-08-31 9:28 ` Jonathan Cameron
1 sibling, 1 reply; 19+ messages in thread
From: Nuno Sá @ 2023-08-31 8:20 UTC (permalink / raw)
To: Jonathan Cameron, Nuno Sa; +Cc: linux-iio, Jonathan Cameron
On Wed, 2023-08-30 at 17:29 +0100, Jonathan Cameron wrote:
> On Fri, 4 Aug 2023 16:53:38 +0200
> Nuno Sa <nuno.sa@analog.com> wrote:
>
> > This is the initial RFC following the discussion in [1]. I'm aware this is
> > by no means ready for inclusion and it's not even compilable since in
> > the RFC I did not included the patch to add component_compare_fwnode()
> > and component_release_fwnode().
>
> Whilst I haven't read this through yet, I suspect Olivier will be able to
> offer some insight on some of this and likewise you may be able to
> point out pitfalls etc in his series (I see you did some review already :)
>
> https://lore.kernel.org/linux-iio/20230727150324.1157933-1-olivier.moysan@foss.st.com/
>
> Both are about multiple interacting components of an overall datapath.
> Whether there is commonality isn't yet clear to me.
>
I made a very general comment in that series but I need to look better at it. Not
sure if we can merge them together but let's see...
> >
> > The goal is to have a first feel on the
> > direction of the framework so that if I need to drastically change it,
> > better do it now. The RFC also brings the ad9647 and the axi_adc core to
> > the same functionality we have now upstream with one extra fundamental
> > feature that is calibrating the digital interface. This would be very
> > difficult to do with the current design. Note that I don't expect any
> > review on those drivers (rather than things related to the framework).
> >
> > I also want to bring up a couple of things that I've
> > been thinking that I'm yet not sure about (so some feedback might make
> > mind in one direction or another).
> >
> > 1) Im yet not sure if I should have different compatibles in the
> > axi-adc-core driver. Note this soft core is a generic core and for every
> > design (where the frontend device changes or has subtle changes like
> > different number of data paths) there are subtle changes. So, the number
> > of channels might be different, the available test patterns might be
> > different, some ops might be available for some designs but not for
> > others, etc...
>
> I don't suppose there is any chance Analog can make at least some of this
> discoverable from the hardware? Capability registers etc in the long
> run. Can't fix what is already out there.
>
Well, it is a soft core so my naive assumption is that it's doable if some HDL guy is
willing to implement it. But yes, it might get supported only for new designs.
> > With a different compatible we could fine tune
> > those differences (with a chip_info like structure) and pass some const
> > converter_config to the framework that would allow it to do more safety
> > checks and potentially reduce the number of converter_ops.
> > OTOH, starting to add all of these compatibles might become messy in the
> > long run and will likely mean that we'll always have to change both
> > drivers in order to support a new frontend. And the frontend devices
> > should really be the ones having all the "knowledge" to configure the
> > soft core even if it means more converter_ops (though devicetree might
> > help as some features are really HW dependent). I more inclined to just
> > leave things as-is in the RFC.
>
> I'm fine with putting this stuff in DT where possible.
>
> >
> > 2) There are some IIO attributes (like scale, frequency, etc) that might
> > be implemented in the soft cores. I still didn't made my mind if I should just
> > have a catch all read_raw() and write_raw() converter_ops or more fine
> > tuned ops. Having the catch all reduces the number of ops but also makes
> > it more easier to add stuff that ends up being not used anymore and then
> > forgotten. There are also cases (eg: setting sampling frequency) where
> > we might need to apply settings in both the frontend and the backend
> > devices which means having the catch all write_raw() would be more
> > awkward in these case. I'm a bit more inclined to the more specific ops.
>
> It's the kernel - we can always change the internal API later as long as we
> don't touch the user space part. Go with your gut feeling today and
> if it changes this sort of refactor usually isn't that bad.
>
Agreed...
>
- Nuno Sá
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 0/3] Add converter framework
2023-08-31 8:20 ` Nuno Sá
@ 2023-08-31 9:28 ` Jonathan Cameron
2023-08-31 10:58 ` Nuno Sá
0 siblings, 1 reply; 19+ messages in thread
From: Jonathan Cameron @ 2023-08-31 9:28 UTC (permalink / raw)
To: Nuno Sá; +Cc: Nuno Sa, linux-iio, Jonathan Cameron
On Thu, 31 Aug 2023 10:20:20 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Wed, 2023-08-30 at 17:29 +0100, Jonathan Cameron wrote:
> > On Fri, 4 Aug 2023 16:53:38 +0200
> > Nuno Sa <nuno.sa@analog.com> wrote:
> >
> > > This is the initial RFC following the discussion in [1]. I'm aware this is
> > > by no means ready for inclusion and it's not even compilable since in
> > > the RFC I did not included the patch to add component_compare_fwnode()
> > > and component_release_fwnode().
> >
> > Whilst I haven't read this through yet, I suspect Olivier will be able to
> > offer some insight on some of this and likewise you may be able to
> > point out pitfalls etc in his series (I see you did some review already :)
> >
> > https://lore.kernel.org/linux-iio/20230727150324.1157933-1-olivier.moysan@foss.st.com/
> >
> > Both are about multiple interacting components of an overall datapath.
> > Whether there is commonality isn't yet clear to me.
> >
>
> I made a very general comment in that series but I need to look better at it. Not
> sure if we can merge them together but let's see...
Great. I wasn't sure either! If nothing else more cross review is always
good even if we decide we need two frameworks.
My one takeaway from looking at this is I need to understand the component
framework better and do some messing around with simple cases before I'll
be confident on how this works. Maybe we can get some input from developers
of that framework on future versions?
Jonathan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 0/3] Add converter framework
2023-08-31 9:28 ` Jonathan Cameron
@ 2023-08-31 10:58 ` Nuno Sá
0 siblings, 0 replies; 19+ messages in thread
From: Nuno Sá @ 2023-08-31 10:58 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Nuno Sa, linux-iio, Jonathan Cameron
On Thu, 2023-08-31 at 10:28 +0100, Jonathan Cameron wrote:
> On Thu, 31 Aug 2023 10:20:20 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
>
> > On Wed, 2023-08-30 at 17:29 +0100, Jonathan Cameron wrote:
> > > On Fri, 4 Aug 2023 16:53:38 +0200
> > > Nuno Sa <nuno.sa@analog.com> wrote:
> > >
> > > > This is the initial RFC following the discussion in [1]. I'm aware this is
> > > > by no means ready for inclusion and it's not even compilable since in
> > > > the RFC I did not included the patch to add component_compare_fwnode()
> > > > and component_release_fwnode().
> > >
> > > Whilst I haven't read this through yet, I suspect Olivier will be able to
> > > offer some insight on some of this and likewise you may be able to
> > > point out pitfalls etc in his series (I see you did some review already :)
> > >
> > > https://lore.kernel.org/linux-iio/20230727150324.1157933-1-olivier.moysan@foss.st.com/
> > >
> > > Both are about multiple interacting components of an overall datapath.
> > > Whether there is commonality isn't yet clear to me.
> > >
> >
> > I made a very general comment in that series but I need to look better at it. Not
> > sure if we can merge them together but let's see...
>
> Great. I wasn't sure either! If nothing else more cross review is always
> good even if we decide we need two frameworks.
>
Indeed...
> My one takeaway from looking at this is I need to understand the component
> framework better and do some messing around with simple cases before I'll
Fair enough... It's a fairly straight framework and I like the idea of having the
whole thing (multiple devices) coming up/down together since it simplifies some
things. But I don't feel to strong about it so in the end, if we decide to go with
typical OF/FW lookup, also fine with me.
> be confident on how this works. Maybe we can get some input from developers
> of that framework on future versions?
>
Sure, they might have some thoughts on the hacks I'm doing to use devres. But I fear
they'll be a bit biased for what they "enforce" in the component framework (my idea
was to build on top of it).
- Nuno Sá
^ permalink raw reply [flat|nested] 19+ messages in thread