Message-ID: <75287793-a1c1-9b61-73aa-a2cf8d61dac2@foss.st.com>
Date:   Mon, 18 Sep 2023 17:52:28 +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/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.

>> 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.
>>
> 
> Please do so :).
> 
Here, we need to clarify some points related to DT first, I think.
I assume that API itself should not be too much a concern.

>>>
>>> 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.
>>
> 
> Sure. I think one backend might be used as frontend in another aggregate device,
> using the component API, but I'm 100% sure. So, yeah, something to keep in mind
> and test with some dummy setup.
> 
>>> 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.
> 
> My idea is to still make use of standard IIO attrs but with a more fine grained
> approach on the callback. Here is what I reasoned about in the other thread:
> 
> "There are some IIO attributes (like scale, frequency, etc) that might
> be implemented in the soft cores. I still didn't made my mind if I should just
> have a catch all read_raw() and write_raw() converter_ops or more fine
> tuned ops. Having the catch all reduces the number of ops but also makes
> it more easier to add stuff that ends up being not used anymore and then
> forgotten. There are also cases (eg: setting sampling frequency) where
> we might need to apply settings in both the frontend and the backend
> devices which means having the catch all write_raw() would be more
> awkward in these case. I'm a bit more inclined to the more specific ops."
> > - Nuno Sá
> 

Powered by blists - more mailing lists