Re: [RFC v2 01/11] iio: introduce iio backend device
From: Olivier MOYSAN
Date: Tue Sep 26 2023 - 12:45:12 EST
Hi Nuno,
On 9/25/23 08:48, Nuno Sá wrote:
Hi Olivier,
On Fri, 2023-09-22 at 10:53 +0200, Nuno Sá wrote:
Hi Olivier,
Sorry for the delay...
On Mon, 2023-09-18 at 17:52 +0200, Olivier MOYSAN wrote:
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.
I might be missing something but there should be no limitation in the component
stuff for this. Note your frontend/backend devices are just normal device tree
nodes (meaning that they can have all the properties they want as a normal node)
and then in the correspondent drivers you handle all the properties. For now,
the only FW properties supported in the framework I sent are 'converters' and
'converter-name' which will be used to "create" the aggregate device. This
pretty much means that the complete thing should only come up when all the
devices you set in DT probe.
Of course we can move more properties into the framework if we start to see some
generic ones that are almost always present...
One thing that Jonathan already mentioned is that the component API works in a
away that you can have either 1->1 or 1->N (frontends->backends). So, if you
have setups where you have more than one frontend (basically M->N) we need to
make sure it still works. In theory (in the component API), I think you can have
one backend associated with more than one frontend so we should be able to still
get the M->N topology. Of course the "communications link" is always between
frontend -> backend.
I'll see if I send the devicetree over the weekend (don't have it in my current
machine)
Here it goes the 2 nodes of interest in my testing...
adc_ad9467: ad9467@0 {
compatible = "adi,ad9467";
reg = <0>;
dmas = <&rx_dma 0>;
dma-names = "rx";
spi-max-frequency = <10000000>;
adi,spi-3wire-enable;
clocks = <&clk_ad9517 3>;
clock-names = "adc-clk";
converters = <&cf_ad9467_core_0>;
};
cf_ad9467_core_0: cf-ad9467-core-lpc@44a00000 {
compatible = "adi,axi-adc-10.0.a";
reg = <0x44A00000 0x10000>;
clocks = <&clkc 16>;
};
Naturally, converter-names only makes sense when you have more than one backend. But
see that in 'cf_ad9467_core_0', you are free to place a regulator (as I have a clock)
as long as you handle it in the backend driver.
- Nuno Sá
Thanks for the example. This helped me prototyping a dfsdm driver based
on the converter framework. Regarding device tree and driver update this
looks fine. I could integrate the API smartly in my frontend (dfsdm) and
backend (sd modulator).
My prototype executes up to probe. I have noticed however that init
(backend & frontend) ops are not called in my implementation. I can see
that init ops are called from bind ops. component_bind_all() calls
converter bind ops, but component_bind_all() is called from converter
bind ops. So, I don't understand how initialization can proceed with
these circular calls. Maybe I missed something here.
The change in the DT has an impact (But moderated) on legacy. Breaking
the legacy was unavoidable anyway.
DFSDM legacy binding (with two channels)
dfsdm_pdm1: filter@1 {
compatible = "st,stm32-dfsdm-adc";
st,adc-channels = <2 3>;
st,adc-channel-types = "SPI_R", "SPI_R";
...
io-channels = <&sd_adc2 &sd_adc3>;
};
DFSDM binding with converter fw
dfsdm_pdm1: filter@1 {
compatible = "st,stm32-dfsdm-adc";
st,adc-channels = <2 3>;
st,adc-channel-types = "SPI_R", "SPI_R";
...
converters = <&sd_adc2 &sd_adc3>;
};
I have also the aim to change DFSDM bindings to use IIO generic channels
bindings (bindings/iio/adc/adc.yaml).
Ideally the DFSDM bindings should looks like this:
dfsdm_pdm1: filter@1 {
compatible = "st,stm32-dfsdm-adc";
channel@2 {
reg = <2>;
st,adc-channel-types = "SPI_R";
...
converters = <&sd_adc2>;
};
channel@3 {
reg = <3>;
st,adc-channel-types = "SPI_R";
...
converters = <&sd_adc3>;
};
};
But it seems that current framework converter API cannot support this
topology.
As a fallback solution the following binding may be adopted
dfsdm_pdm1: filter@1 {
compatible = "st,stm32-dfsdm-adc";
channel@2 {
reg = <2>;
st,adc-channel-types = "SPI_R";
...
};
channel@3 {
reg = <3>;
st,adc-channel-types = "SPI_R";
...
};
converters = <&sd_adc2 &sd_adc3>;
In this case the frontend driver needs a mean to map backend and
channels. It's not the smartest solution, yet. Especially since the use
of generic channel is quite common.
Perhaps the converter_frontend_add() API needs to be extended to support
generic channel configuration. Maybe the IIO core should provide the
related helpers as well. (As far as I know this does not exists).
So, still opened questions ..
That said, I feel confident that the converter framework is a good
option for the DFSDM use case.
BRs
Olivier