Message-ID: <20230903114658.24eed6bd@jic23-huawei>
Date:   Sun, 3 Sep 2023 11:46:58 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Nuno Sá <noname.nuno@...il.com>
Cc:     Olivier MOYSAN <olivier.moysan@...s.st.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        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

On Fri, 01 Sep 2023 10:01:19 +0200
Nuno Sá <noname.nuno@...il.com> 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:
> 
> 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.
> 
> 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.
> 
> Alternatively, another (maybe crazy) idea would be to have this framework have the
> really generic stuff (like lookup + generic ops) and build my iio-converter on top of
> it (extending it). You know, some OO fun :). Maybe not worth the trouble though.
> 
> Let's if Jonathan has some suggestions on how to proceed...

The two of you are definitely the most familiar with the code and the restrictions
around it, so ideally I'd like you go figure out the path forwards and tell me :)

To me this is a non core extension of IIO so as long as we end up with something
maintainable that solves some (all?) of the ordering and dependency issues I'll
be happy.  I'd rather not have two solutions of course if there is not a good
reason why they have to be different.  If we do end up with two solutions I
want clear documentation for the restrictions of each so that we hopefully
don't end up with a 3rd solution down the line.

Jonathan

> 
> - Nuno Sá
> > >   
> 

Powered by blists - more mailing lists