Re: [PATCH v3 0/8] iio: add new backend framework

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

Hi Nuno,

On 12/15/23 16:18, Nuno Sá wrote:
> On Thu, 2023-12-14 at 11:03 -0600, Rob Herring wrote:
>> On Thu, Dec 14, 2023 at 05:05:20PM +0100, Nuno Sá wrote:
>>> On Thu, 2023-12-14 at 08:16 -0600, Rob Herring wrote:
>>>> On Wed, Dec 13, 2023 at 04:02:31PM +0100, Nuno Sa wrote:
>>>>> v1:
>>>>>   
>>>>> https://lore.kernel.org/linux-iio/20231204144925.4fe9922f@jic23-huawei/T/#m222f517
>>>>> 5273b81dbfe40b7f0daffcdc67d6cb8ff
>>>>>
>>>>> v2:
>>>>>   
>>>>> https://lore.kernel.org/r/20231208-dev-iio-backend-v2-0-5450951895e1@analog.co
>>>>> m
>>>>>
>>>>> Changes in v3:
>>>>> - Patch 1:
>>>>>   * Use proposed generic schema [1]. Also make it a required property;
>>>>>   * Improved the commit message.
>>>>> - Patch 2:
>>>>>   * Improved commit message.
>>>>> - Patch 4:
>>>>>   * Namespace all IIO DMAENGINE buffer exports;
>>>>>   * Removed unrelated new line removal change.
>>>>> - Patch 5:
>>>>>   * Namespace all IIO backend exports.
>>>>> - Patch 6:
>>>>>   * Set backend.h in alphabetical order;
>>>>>   * Import IIO backend namespace.
>>>>> - Patch 7:
>>>>>   * Don't depend on OF in kbuild anymore;
>>>>>   * Import IIO backend namespace.
>>>>>
>>>>> For the bindings patches, I tried not to enter into much details about
>>>>> the IIO framework as I think specifics of the implementation don't care
>>>>> from the bindings perspective. Hopefully the commit messages are good
>>>>> enough.
>>>>>
>>>>> I'm also aware that patch 1 is not backward compatible but we are
>>>>> anyways doing it on the driver side (and on the driver the property is
>>>>> indeed required). Anyways, just let me know if making the property
>>>>> required is not acceptable (I'm fairly confident no one was using the
>>>>> upstream version of the driver and so validating devicetrees for it).
>>>>>
>>>>> Keeping the block diagram in v3's cover so we don't have to follow links
>>>>> to check the one of the typicals setups.
>>>>>
>>>>>                                             ----------------------------------
>>>>> ----
>>>>> -----------------
>>>>>   ------------------                        | -----------         ------------
>>>>>        -------  FPGA |
>>>>>   |     ADC        |------------------------| | AXI ADC |---------| DMA CORE
>>>>> |----
>>>>> --| RAM |       |
>>>>>   | (Frontend/IIO) | Serial Data (eg: LVDS) | |(backend)|---------|
>>>>> |----
>>>>> --|     |       |
>>>>>   |                |------------------------| -----------         ------------
>>>>>        -------       |
>>>>>   ------------------                        ----------------------------------
>>>>> ----
>>>>> -----------------
>>>>
>>>> Why doesn't axi-adc just have an io-channels property to adc? It's the
>>>> opposite direction for the link, but it seems more logical to me that
>>>> axi-adc depends on adc rather than the other way around.
>>>>
>>>
>>> We are not interested on a single channel but on the complete device. >From the
>>> axi-
>>> adc point of view, there's not much we could do with the adc channel. I mean,
>>> maybe
>>> we could still do something but it would be far from ideal (see below).
>>
>> Will this hold up for everyone? Could there be a backend device that
>> handles multiple ADCs? IOW, do you need #io-backend-cells? It's somewhat
>> better if we add that up front rather than later and have to treat
>> missing as 0 cells. It is also the only way to generically identify the
>> providers (well, there's also 'foo-controller' properties, but we've
>> gone away from those).
>>
> 
> For the axi-adc backend, it's very unlikely. The way the core connects to the
> converters is through a serial data interface (LVDS, CMOS or JESD in ADI usecases).
> This interface is not really a bus so it's kind of a 1:1 connection. Now, in more
> complicated devices (like highly integrated RF transceivers) what we have is that we
> have multiple of these cores (one per RX/TX port) connected to the frontend. So,
> effectively 1 frontend could have multiple backends. So, yes, your first "doubts" are
> not that "crazy" as this is also not the "typical" provider - consumer relationship.
> However, for all of what I've said in the previous email, even in these cases,
> thinking in these cores as the provider, makes things much more easier to handle.
> 
> However, the above is just ADI usecases. In theory, yes, it can be very possible for
> another backend other than axi-adc to have multiple frontends connected so I guess we
> can make #io-backend-cells already available in the schema.
> 
> For the axi-adc bindings this would be 'const: 0', right?
> 
>>
>>> The opposite direction is exactly what we had (look at patch 2) just with another
>>> custom property. The problem with that is that we would then need a bidirectional
>>> link (we would need to callback into the provider and the provider would need to
>>> access the consumer) between consumers and providers and that would be far from
>>> optimal. The bidirectional link would exist because if we want to support
>>> fundamental
>>> things like LVDS/CMOS interface tuning we need to set/get settings from the axi-
>>> adc
>>> core. And as Jonathan suggested, the real data is captured or sent on the
>>> converters
>>> (ADC or DACs) and that is why we have the IIO device/interface in there and why
>>> we
>>> call them "frontends". In ADI usecases, backends are these FPGA cores providing
>>> "services" to the "real" high speed converters. To put it in another way, the
>>> real
>>> converter is the one who knows how to use the axi-adc core and not the other way
>>> around. That's also one of the reasons why it would be way more difficult to
>>> handle
>>> things with the opposite link. That's how we have done it so far and the mess we
>>> have
>>> out of tree is massive :sweat_smile: We ended up doing raw writes and reads on
>>> the
>>> axi-adc MMIO registers from the converter driver just because we had to configure
>>> or
>>> get something from the axi-adc device but the link was backwards.
>>
>> The direction (for the binding) doesn't really matter. It doesn't
>> dictate the direction in the OS. In the ad9467 driver, you can search
>> the DT for 'adi,adc-dev' and find the node which matches your node's
>> phandle. It's not exactly efficient, but you are doing it once. It would
>> also prevent the DT ABI break you are introducing.
>>
> 
> Hmm, I think I see your idea. So you mean something like
> devm_iio_backend_get_optional() and if not present, then we would look for nodes
> having the 'adi,adc-dev' property and look for the one pointing at us... Then, we
> would need another API in the backend to look for registered backends matching that
> fwnode. Right?
> 
> I mean, I guess this could work but we would already have to start a fresh framework
> with API's that are not really meant to be used anymore other than the ad9467 driver
> (not devm_iio_backend_get_optional() because sooner or later I think we'll need that
> one). We are already breaking ABI in the driver and I'm still fairly confident no one
> is really using the upstream driver because it's lacking support for devices and
> important features (not even in ADI fork we're using it).
> 
> Anyways, if you still insist on having something like this (and feel more comfortable
> in not breaking DT ABI), I can see how this would look like in the next version...
> 
>>
>>>> And if there's another consumer in the chain, then a node could
>>>> certainly be both an io-channels consumer and producer.
>>>>
>>>
>>> This should also be possible with this architecture. A node can be both a backend
>>> (provider) and a consumer and we have an out of tree design that fits this (that
>>> I
>>> surely want to upstream after the foundations are done).
>>>
>>>> The architecture of the drivers seems odd to me. It looks similar to
>>>> making a phy driver handle all the state and protocol with the host
>>>> controller being a backend.
>>>
>>> In this case, it's not really a controller. It's more like an extension of the
>>> device
>>> because we need a way to handle the high sample rates this ADCs can do. Then we
>>> can
>>> also do test tones with the backend which is useful for interface tuning (as
>>> mentioned above).
>>>
>>> To give you a bit more context, I'm adding the generic property because we will
>>> have
>>> more users for it (from ADI - the next should be the axi-dac core) but STM is
>>> also
>>> interested in this (likely the next user).
>>>
>>> Hope this makes it a bit more clear...
>>
>> Yes, thanks.
>>
>> I generally ask for 2 users on new common bindings. I've accepted too
>> many only to have the 2nd user come in a month later and need additions.
>> An ack on the binding from the STM folks would be nice here. And
>> Jonathan too.
>>
> 
> Olivier, could we get an ack on the bindings patch? Do you also have any idea about
> how long it would take for you to send patches so we have another user of the schema?
> 
> On my side, it might very well take a month or so (given we have holidays nearby) as
> the axi-dac core is more complex than the axi-adc. Bah it might take less than a
> month to have the first version of it posted in the lists but I can't make any
> promises.
> 

Sorry for late answer.
Regarding bindings patch, I assume you refer to [1].
After adding the #io-backend-cells property in v5 you can add my
Ack-by Olivier Moysan <olivier.moysan@foss.st.com>

I will prepare a serie based on these bindings. Hopefully it should be 
possible this month.

BRs
Olivier

[1] https://github.com/devicetree-org/dt-schema/pull/120


> - Nuno Sá
>