Message-ID: <efc6590f-be79-3cb5-3264-f11a4e0c372c@foss.st.com>
Date:   Thu, 28 Sep 2023 18:30:17 +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/28/23 09:15, Nuno Sá wrote:
> Hi Olivier,
> 
> On Tue, 2023-09-26 at 18:44 +0200, Olivier MOYSAN wrote:
>> Hi Nuno,
>>
>> On 9/25/23 08:48, Nuno Sá wrote:
>>> Hi Olivier,
>>>
>>> On Fri, 2023-09-22 at 10:53 +0200, Nuno Sá wrote:
>>>> Hi Olivier,
>>>>
>>>> Sorry for the delay...
>>>>
>>>> On Mon, 2023-09-18 at 17:52 +0200, Olivier MOYSAN wrote:
>>>>> 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.
>>>>>
>>>>
>>>> I might be missing something but there should be no limitation in the component
>>>> stuff for this. Note your frontend/backend devices are just normal device tree
>>>> nodes (meaning that they can have all the properties they want as a normal
>>>> node)
>>>> and then in the correspondent drivers you handle all the properties. For now,
>>>> the only FW properties supported in the framework I sent are 'converters' and
>>>> 'converter-name' which will be used to "create" the aggregate device. This
>>>> pretty much means that the complete thing should only come up when all the
>>>> devices you set in DT probe.
>>>>
>>>> Of course we can move more properties into the framework if we start to see
>>>> some
>>>> generic ones that are almost always present...
>>>>
>>>> One thing that Jonathan already mentioned is that the component API works in a
>>>> away that you can have either 1->1 or 1->N (frontends->backends). So, if you
>>>> have setups where you have more than one frontend (basically M->N) we need to
>>>> make sure it still works. In theory (in the component API), I think you can
>>>> have
>>>> one backend associated with more than one frontend so we should be able to
>>>> still
>>>> get the M->N topology. Of course the "communications link" is always between
>>>> frontend -> backend.
>>>>
>>>> I'll see if I send the devicetree over the weekend (don't have it in my current
>>>> machine)
>>>>
>>>>
>>>
>>> Here it goes the 2 nodes of interest in my testing...
>>>
>>> adc_ad9467: ad9467@0 {
>>>           compatible = "adi,ad9467";
>>>           reg = <0>;
>>>
>>>           dmas = <&rx_dma 0>;
>>>           dma-names = "rx";
>>>
>>>           spi-max-frequency = <10000000>;
>>>           adi,spi-3wire-enable;
>>>
>>>           clocks = <&clk_ad9517 3>;
>>>           clock-names = "adc-clk";
>>>
>>>           converters = <&cf_ad9467_core_0>;
>>> };
>>>
>>> cf_ad9467_core_0: cf-ad9467-core-lpc@...00000 {
>>>           compatible = "adi,axi-adc-10.0.a";
>>>           reg = <0x44A00000 0x10000>;
>>>
>>>           clocks = <&clkc 16>;
>>> };
>>>
>>> Naturally, converter-names only makes sense when you have more than one backend.
>>> But
>>> see that in 'cf_ad9467_core_0', you are free to place a regulator (as I have a
>>> clock)
>>> as long as you handle it in the backend driver.
>>>
>>> - Nuno Sá
>>
>> Thanks for the example. This helped me prototyping a dfsdm driver based
>> on the converter framework. Regarding device tree and driver update this
>> looks fine. I could integrate the API smartly in my frontend (dfsdm) and
>> backend (sd modulator).
>>
>> My prototype executes up to probe. I have noticed however that init
>> (backend & frontend) ops are not called in my implementation. I can see
>> that init ops are called from bind ops. component_bind_all() calls
> 
> Note that you need to call converter_frontend_add() from your frontend device (stm32-
> dfsdm-adc) probe function and converter_add() from your backend's probes. And
> ideally, this is the only thing you do at probe. Then, once all the elements are
> probed, the complete aggregate device is initialised and the .bind()/.init() function
> should be called.
> 
> And I want to reinforce the above, in the component API, things will only come up
> when all the pieces (all the converters you specified in DT) are probed. The same is
> true if one of the elements is unbound from it's driver - all the other elements in
> the aggregate device will be torn down and converter_frontend_unbind() will be
> called. This means it's an all or nothing solution... Let me know if this does not
> work for you.
> 
>> converter bind ops, but component_bind_all() is called from converter
>> bind ops. So, I don't understand how initialization can proceed with
>> these circular calls. Maybe I missed something here.
>>
> 
> This one I'm not following... component_bind_all() should be called from
> converter_frontend_bind() and this will call all converter_bind() you have (depends
> on how many backends you have). After all backends are initialized, .frontend_init()
> is called. In there, if you need (most likely you do) an handle to a converter you
> then need to call converter_get(). So, component_bind_all() should not be called from
> converter bind ops but from frontend_component_ops which are the
> component_master_ops. If this is not happening, then we have an issue :)
> 

A quick update to my previous feedback:
As you mentioned it in the converter fw serie, 
component_compare_fwnode() and component_release_fwnode() patch is not 
included. By default I used the component_release_of() and 
component_compare_of() from the component API. This was not the best 
idea. With a correct compare function the init callbacks are actually 
called. So, no real issue here :-)

Olivier

>> The change in the DT has an impact (But moderated) on legacy. Breaking
>> the legacy was unavoidable anyway.
>>
>> DFSDM legacy binding (with two channels)
>>         dfsdm_pdm1: filter@1 {
>>           compatible = "st,stm32-dfsdm-adc";
>>           st,adc-channels = <2 3>;
>>           st,adc-channel-types = "SPI_R", "SPI_R";
>>           ...
>>           io-channels = <&sd_adc2 &sd_adc3>;
>>         };
>>
>> DFSDM binding with converter fw
>>       dfsdm_pdm1: filter@1 {
>>           compatible = "st,stm32-dfsdm-adc";
>>           st,adc-channels = <2 3>;
>>           st,adc-channel-types = "SPI_R", "SPI_R";
>>           ...
>>           converters = <&sd_adc2 &sd_adc3>;
>>         };
>>
>> I have also the aim to change DFSDM bindings to use IIO generic channels
>> bindings (bindings/iio/adc/adc.yaml).
>>
>> Ideally the DFSDM bindings should looks like this:
>>         dfsdm_pdm1: filter@1 {
>>           compatible = "st,stm32-dfsdm-adc";
>>           channel@2 {
>>                   reg = <2>;
>>                   st,adc-channel-types = "SPI_R";
>>                   ...
>>                   converters = <&sd_adc2>;
>>           };
>>           channel@3 {
>>                   reg = <3>;
>>                   st,adc-channel-types = "SPI_R";
>>                   ...
>>                   converters = <&sd_adc3>;
>>           };
>>         };
>>
>> But it seems that current framework converter API cannot support this
>> topology.
>>
> 
> Indeed this won't work and I honestly it never crossed my mind ehehe,
> 
>> As a fallback solution the following binding may be adopted
>>         dfsdm_pdm1: filter@1 {
>>           compatible = "st,stm32-dfsdm-adc";
>>           channel@2 {
>>                   reg = <2>;
>>                   st,adc-channel-types = "SPI_R";
>>                   ...
>>           };
>>           channel@3 {
>>                   reg = <3>;
>>                   st,adc-channel-types = "SPI_R";
>>                   ...
>>           };
>>           converters = <&sd_adc2 &sd_adc3>;
>>
>> In this case the frontend driver needs a mean to map backend and
>> channels. It's not the smartest solution, yet. Especially since the use
>> of generic channel is quite common.
> 
> Yeah, I'm also not a fan of that... To support the above topology and from the top of
> my head we could either:
> 
> 1) Somehow split converter_frontend_add() to give more control to the caller to call
> converter_frontend_add_matches() and in this case have another API that accepts a
> fwnode.
> 
> 2) Just extend converter_frontend_add_matches() so that we also look into child nodes
> for 'converters'
> 
> Then, on the get() side, we would need something like converters_get_from_fwnode() to
> get each handle. I would likely prefer to go with 2) because 1) already implies some
> FW parsing during probe that I would like to avoid.
> 
> Anyways the above is already showing that maybe going with the component API for
> something more generic might be a stretch and harder to scale for everyone needs.
> With an OF lookup, the above topology would be easier to accomplish (though we would
> always need a converters_get_from_fwnode() kind of function).
> 
> When you say:
> 
> "In this case the frontend driver needs a mean to map backend and channels."
> 
> Could 'converter-names' be used for the above? Or would the above be trivial with an
> OF lookup?
> 
>> Perhaps the converter_frontend_add() API needs to be extended to support
>> generic channel configuration. Maybe the IIO core should provide the
>> related helpers as well. (As far as I know this does not exists).
>> So, still opened questions ..
>>
> 
> No sure what do you mean by the above?
> 
>> That said, I feel confident that the converter framework is a good
>> option for the DFSDM use case.
>>
> 
> Yeah, I'm also confident that we can get something that suits both our usecases
> either with OF or component. I must say that I'm tempted to send a version of this
> with an OF lookup just so we get a look on how it would look like and compare against
> the component API.
> 
> - Nuno Sá
>>>>

Powered by blists - more mailing lists