Re: [RFC v2 01/11] iio: introduce iio backend device
From: Olivier MOYSAN
Date: Mon Sep 18 2023 - 12:32:36 EST
Hi Nuno
On 9/11/23 11:39, Nuno Sá wrote:
On Tue, 2023-09-05 at 12:06 +0200, Olivier MOYSAN wrote:
Hi Nuno,
On 9/1/23 10:01, Nuno Sá wrote:
Hi Olivier,
On Thu, 2023-08-31 at 18:14 +0200, Olivier MOYSAN wrote:
Hi Nuno,
On 7/28/23 10:42, Nuno Sá wrote:
Hi Olivier,
On Thu, 2023-07-27 at 17:03 +0200, Olivier Moysan wrote:
Add a new device type in IIO framework.
This backend device does not compute channel attributes and does not
expose
them through sysfs, as done typically in iio-rescale frontend device.
Instead, it allows to report information applying to channel
attributes through callbacks. These backend devices can be cascaded
to represent chained components.
An IIO device configured as a consumer of a backend device can compute
the channel attributes of the whole chain.
Signed-off-by: Olivier Moysan <olivier.moysan@xxxxxxxxxxx>
---
drivers/iio/Makefile | 1 +
drivers/iio/industrialio-backend.c | 107
+++++++++++++++++++++++++++++
include/linux/iio/backend.h | 56 +++++++++++++++
3 files changed, 164 insertions(+)
create mode 100644 drivers/iio/industrialio-backend.c
create mode 100644 include/linux/iio/backend.h
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index 9622347a1c1b..9b59c6ab1738 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -5,6 +5,7 @@
obj-$(CONFIG_IIO) += industrialio.o
industrialio-y := industrialio-core.o industrialio-event.o inkern.o
+industrialio-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
diff --git a/drivers/iio/industrialio-backend.c
b/drivers/iio/industrialio-
backend.c
new file mode 100644
index 000000000000..7d0625889873
--- /dev/null
+++ b/drivers/iio/industrialio-backend.c
@@ -0,0 +1,107 @@
+// SPDX-License-Identifier: GPL-2.0
+/* The industrial I/O core, backend handling functions
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/property.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/backend.h>
+
+static DEFINE_IDA(iio_backend_ida);
+
+#define to_iio_backend(_device) container_of((_device), struct
iio_backend,
dev)
+
+static void iio_backend_release(struct device *device)
+{
+ struct iio_backend *backend = to_iio_backend(device);
+
+ kfree(backend->name);
+ kfree(backend);
+}
+
+static const struct device_type iio_backend_type = {
+ .release = iio_backend_release,
+ .name = "iio_backend_device",
+};
+
+struct iio_backend *iio_backend_alloc(struct device *parent)
+{
+ struct iio_backend *backend;
+
+ backend = devm_kzalloc(parent, sizeof(*backend), GFP_KERNEL);
No error checking.
I guess a lot of cleanings are still missing but the important thing I
wanted to
notice is that the above pattern is not ok.
Your 'struct iio_backend *backend'' embeds a 'stuct device' which is a
refcounted object. Nevertheless, you're binding the lifetime of your
object to
the parent device and that is wrong. The reason is that as soon as your
parent
device get's released or just unbinded from it's driver, all the devres
stuff
(including your 'struct iio_backend' object) will be released
independentof
your 'struct device' refcount value...
So, you might argue this won't ever be an issue in here but the pattern
is still
wrong. There are some talks about this, the last one was given at the
latest
EOSS:
https://www.youtube.com/watch?v=HCiJL7djGw8&list=PLbzoR-pLrL6pY8a8zSKRC6-AihFrruOkq&index=27&ab_channel=TheLinuxFoundation
This is a good point. Thanks for pointing it out. Sure, there are still
many things to improve.
I have seen the comment from Jonathan on your "Add converter framework"
serie. I had a quick look at the serie. It seems that we share the need
to aggregate some IIO devices. But I need to read it more carefully to
check if we can find some convergences here.
Yeah, In my case, the backend devices are typically FPGA soft cores and the
aggregate
device might connect to multiple of these backends. That was one of the
reason why I
used the component API where the aggregate device is only configured when
all the
devices are probed. Similarly, when one of them is unbind, the whole thing
should be
torn down. Also, in my case, the frontend device needs to do a lot of setup
on the
backend device so the whole thing works (so I do have/need a lot more .ops).
Anyways, it does not matter much what the backend device is and from a first
glance
and looking at the .ops you have, it seems that this could easily be
supported in the
framework I'm adding. The only things I'm seeing are:
Thanks for your feedback. Yes, my feeling is that the API I need for the
dfsdm use case, can be covered by the API you propose. I'm not familiar
with component API however, as I discovered it in your serie. It is not
clear for me how this affects device tree description of the hardware.
Your aggregate device (that we can think of as a frontend device needs to
properly reference all the backends it needs - in your case I guess it's just
one device). The dts properties I have for now are 'converters' and 'converter-
names'. But one thing that starts to become clear to me is that I should
probably change the name for the framework. Maybe industrialio-aggregate.c if we
keep the component API (and so the same frontend + backend naming) or just
industrialio-backend.c (as you have now) if we go with a typical OF lookup.
In my case I have a digital filter peripheral (frontend) linked to
several sigma delta converters (backends). So, here 'converters'
property may be relevant as well. But I agree that a more generic name
seems better for the long term.
My backend devices need to get a regulator phandle from the device tree.
It seems that the component API does not offer services allowing to
retrieve DT properties for the sub-devices. Tell me if I'm wrong, but I
think this constraint require to change converter framework to a typical
OF lookup.
Could you please share the structure of your DT for your ad9476 based
example ? This will help me identify the gaps regarding my need.
So I need to take time to look at existing examples.
I think I need also to try a template implementation of dfsdm use case
based on your API, to figure out how it could work.
Please do so :).
Here, we need to clarify some points related to DT first, I think.
I assume that API itself should not be too much a concern.
1) You would need to use the component API if it's ok. Also not sure if the
cascaded
usecase you mention would work with that API.
The cascaded use case by itself is not a real requirement for dfsdm use
case. The idea here was to think about future possible needs, and to
ensure that the solution is scalable enough. So, it is not a strong
requirement, but we probably need to keep it in mind.
Sure. I think one backend might be used as frontend in another aggregate device,
using the component API, but I'm 100% sure. So, yeah, something to keep in mind
and test with some dummy setup.
2) We would need to add the .read_raw() op. If you look at my RFC, I already
have
some comments/concerns about having an option like that (see there).
Having said that, none of the above are blockers as 1), I can ditch the
component API
in favour of typical FW/OF lookup (even though the component API makes some
things
easier to handle) and 2), adding a .read_raw() op is not a blocker for me.
Yes, It would be nice to have read_raw(), as this allows to stick to
existing IIO API for standard IIO attributes. But I guess this should
not be a problem.
My idea is to still make use of standard IIO attrs but with a more fine grained
approach on the callback. Here is what I reasoned about in the other thread:
"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."
> - Nuno Sá