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

[Olivier MOYSAN <olivier.moysan@foss.st.com>]

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@foss.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.

BRs
Olivier

> - Nuno Sá
> 
>