Re: [RFC PATCH 1/3] iio: addac: add new converter framework

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

Hi Nuno,

On 11/14/23 10:03, Nuno Sá wrote:
> On Mon, 2023-11-13 at 18:20 +0100, Olivier MOYSAN wrote:
> 
> Ho Olivier,
> 
>> Hi Nuno, Jonathan,
>>
>> On 9/4/23 17:31, Jonathan Cameron wrote:
>>> On Mon, 04 Sep 2023 16:14:17 +0200
>>> Nuno Sá <noname.nuno@gmail.com> wrote:
>>>
>>>> On Sun, 2023-09-03 at 11:56 +0100, Jonathan Cameron wrote:
>>>>> On Thu, 31 Aug 2023 11:32:54 +0200
>>>>> Nuno Sá <noname.nuno@gmail.com> wrote:
>>>>>     
>>>>>> On Wed, 2023-08-30 at 18:02 +0100, Jonathan Cameron wrote:
>>>>>>> On Fri, 4 Aug 2023 16:53:39 +0200
>>>>>>> Nuno Sa <nuno.sa@analog.com> wrote:
>>>>>>>       
>>>>>>>> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
>>>>>>>
>>>>>>> Hi Nuno,
>>>>>>>       
>>>>>>
>>>>>> Hi Jonathan,
>>>>>>
>>>>>> Thanks for the initial review...
>>>>>>     
>>>>>>>
>>>>>>> One general comment is that you could have stripped this back a fair
>>>>>>> bit
>>>>>>> for ease of understanding.  At this stage we don't care about things
>>>>>>> like debug or control of test patterns.  Bring those in as extra
>>>>>>> patches.
>>>>>>>       
>>>>>>
>>>>>> Agreed... As I mentioned (I think) in the cover, I made the RFC bigger
>>>>>> than
>>>>>> needed to
>>>>>> kind of showcase how we can properly configure the hdl core to support
>>>>>> things
>>>>>> (interface calibration) that were very hard to do with the current
>>>>>> implementation.
>>>>>> I'll make sure to add the minimum needed API to accommodate what we
>>>>>> have
>>>>>> right now.
>>>>>>     
>>>>>>> I haven't fully gotten my head around the ordering constraints on
>>>>>>> removal.
>>>>>>> Are there other users of the component framework that have similar
>>>>>>> problems?
>>>>>>>       
>>>>>>
>>>>>> My understanding on the component API is that one should do all the
>>>>>> tear
>>>>>> down in the
>>>>>> .unbind() callback. As usual, I can see some drivers not really doing
>>>>>> that.
>>>>>>     
>>>>>>> Also, I don't yet understand how a multiple front end, single
>>>>>>> backend
>>>>>>> setup
>>>>>>> would work.  Or indeed single front end, multiple backend...  Maybe
>>>>>>> we
>>>>>>> don't
>>>>>>> need those cases, but if we want this to be useful beyond adi-axi we
>>>>>>> probably at least want an outline of how they work.
>>>>>>>       
>>>>>>
>>>>>> Indeed we can have multiple (and we have it out of tree) backends on
>>>>>> one
>>>>>> frontend.
>>>>>> Think on an ADC/DAC with fairly complex data path with more than one
>>>>>> channel/interface (CMOS, LVDS, etc). Typically, in those case, each of
>>>>>> the
>>>>>> interface
>>>>>> will be connected to an instance of the hdl core (the backend).
>>>>>
>>>>> That might work out for your case, but not the stm32 one where I think
>>>>> we can
>>>>> end
>>>>> up with interleaved data from two front ends in the same buffer...
>>>>>     
>>>>
>>>> Not sure I'm following this one. But wouldn't that be something specific
>>>> for
>>>> each system (through devicetree)? I haven't tried but I think the same
>>>> backend
>>>> could be used in different frontend devices (using the component API).
>>>> That is
>>>> not really a usecase for me but definitely something that could be
>>>> supported (if
>>>> we need to start doing things like keep enable/disable counters and so on)
>>>> if it
>>>> is a usecase for stm32.
>>>
>>> If we are going to support both usecases, we just need to figure out what
>>> composite
>>> devices with N-M backend - frontend look like and make sure that doesn't
>>> cause problems.  I'd expect the separation between backend instances might
>>> reflect data storage on capture but then again that might end up like the
>>> many
>>> IIO devices for many buffers mess we had before the multiple buffer support
>>> was added.
>>>
>>
>> The stm32 dfsdm interleaved use case is not a problem as it is possible
>> to associate several backends to a frontend.
>> I did some experiments based on converter framework, and did not
>> identified blocking points regarding dfsdm use cases.
>>
>> Some limitations where discussed in [1], about generic bindings support.
>> The preferred solution was to extend converter_frontend_add_matches() to
>> parse also child nodes. I have added converters_get_from_fwnode() API
>> and adapted converter_frontend_add_matches() to test this approach.
>> With this changes and an additional api to support channel attributes
>> read, the framework fulfills all the needs for dfsdm.
>>
>> So, I feel comfortable to drop my previous "backend framework" proposal,
>> and move to the current proposal.
>>
>> If we go further in converter framework adaption, I will push these updates.
>>
> 
> I hope you didn't had too much trouble with those patches. The reason I'm saying
> this is because, after some thought, I'm strongly considering in moving to
> normal OF/ACPI lookup. 3 mains reasons for it:
> 

No problem, this was an opportunity to discover the component framework. 
The converter API is quite simple to use once you understand the basics 
of component concepts. It does the job for stm dfsdm, but I agree that 
for the long-term, a component-based solution is probably less scalable.

> 1) That "hack/rule" for a driver to provide a .remove() callback (in order to
> devm_*) is really non obvious and might even be prune to subtle bugs (that I'm
> not seeing now :)). But my main argument is that it can become hard to maintain
> it (depending on how much people starts to use the framework).
> 
> 2) From the discussion we had about the limitations you pointed in your link, I
> started to realize that it might get harder to scale the framework. Yes, we
> found a fairly easy way of doing it but still took more code to do it when
> compared to a typical lookup.
> 
> 3) This is the most important together with 1). You mentioned something like
> cascaded designs and I just found an usercase in ADI out of tree drivers. We
> have a design where we have something like:
> 
>                     ------------------------------------------
>                     |		FPGA			    |
> --------------     |  -------------    -------------------  |
> |DAC Frontend| ->  |  |DAC Backend| -> |DAC Interpolation|  |
> --------------     |  -------------    -------------------  |
>                     |					    |
>                     ------------------------------------------
> 
> In the above design we kind of have a cascaded thing where the DAC backend is
> both a frontend and a backend and the Intrerpolation stuff just serves as
> backend to the DAC core. So, ideally the DAC frontend should not have to know a
> thing about the interpolation... I realized that having this with the component
> framework is far from straight because we would need two components/aggregate
> devices to accomplish this (DAC Front + DAC Back) and (DAC Back + DAC Interp)
> and I think we would need some extra care regarding one of the components going
> away (not sure though). One way to make it simple would be to not "respect" the
> HW configuration and just have one aggregate device with 1 Frontend + 2 Backends
> and so the frontend would need to "know" about the interpolation core. Again, I
> think that with OF/ACPI this setup with be fairly straight to get and "respect".
> 
> Anyways, all the above makes me feel that component might not be the best choice
> (even though I was eager to use it :))
> 
> I'll also get to work on this again and I might just use an industrialio-
> backend.c file in the base dir as you had in your RFC. From a quick look on your
> series, I'm no sure how much code I will reuse but we can see later if a Co-
> authored-by tag makes sense or not.
> 

The APIs in my RFC were kept minimalistic to assess the proposal's 
relevance. As you pointed out, there is room for improvement, 
particularly with regards to object releasing. If you are on the way to 
rework your framework, feel free to re-use some parts of this RFC.
At the end, what really matters is to converge towards a solution :)

Regards
Olivier

> Let me know if there's something you don't agree or if there's any concern on
> your side.
> 
> - Nuno Sá
>>>>