Re: [RFC v2 01/11] iio: introduce iio backend device

[Olivier MOYSAN]

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.

BRs
Olivier

> - Nuno Sá