Message-ID: <c25d404fa19c03cafc6dfe70b964369c7c773849.camel@gmail.com>
Date:   Mon, 25 Sep 2023 08:48:23 +0200
From:   Nuno Sá <noname.nuno@...il.com>
To:     Olivier MOYSAN <olivier.moysan@...s.st.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>
Cc:     linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
        Fabrice GASNIER <fabrice.gasnier@...com>
Subject: Re: [RFC v2 01/11] iio: introduce iio backend device

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@...s.st.com>
> > > > > > > > ---
> > > > > > > >     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@...00000 {
        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á
> > 

Powered by blists - more mailing lists