Message-ID: <9cf91ed4-8d57-6d84-1767-708a296803b9@foss.st.com>
Date:   Tue, 5 Sep 2023 12:06:09 +0200
From:   Olivier MOYSAN <olivier.moysan@...s.st.com>
To:     Nuno Sá <noname.nuno@...il.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 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. 
So I need to take time to look at existing examples.
I think I need also to try a template implementation of dfsdm use case 
based on your API, to figure out how it could work.

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

The cascaded use case by itself is not a real requirement for dfsdm use 
case. The idea here was to think about future possible needs, and to 
ensure that the solution is scalable enough. So, it is not a strong 
requirement, but we probably need to keep it in mind.

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

Yes, It would be nice to have read_raw(), as this allows to stick to 
existing IIO API for standard IIO attributes. But I guess this should 
not be a problem.

> 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...
> 
> - Nuno Sá
>>>
> 

Olivier

Powered by blists - more mailing lists