Message-ID: <031e616b-b080-4cfc-9c99-00df46b4075b@foss.st.com>
Date:   Thu, 23 Nov 2023 18:36:45 +0100
From:   Olivier MOYSAN <olivier.moysan@...s.st.com>
To:     <nuno.sa@...log.com>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-iio@...r.kernel.org>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>
Subject: Re: [PATCH 00/12] iio: add new backend framework

Hi Nuno,

On 11/21/23 11:20, Nuno Sa via B4 Relay wrote:
> Hi all,
> 
> This is a Framework to handle complex IIO aggregate devices.
> 
> The typical architecture is to have one device as the frontend device which
> can be "linked" against one or multiple backend devices. All the IIO and
> userspace interface is expected to be registers/managed by the frontend
> device which will callback into the backends when needed (to get/set
> some configuration that it does not directly control).
> 
> The basic framework interface is pretty simple:
>   - Backends should register themselves with @devm_iio_backend_register()
>   - Frontend devices should get backends with @devm_iio_backend_get()
> 
> (typical provider - consumer stuff)
> 
> This is the result of the discussions in [1] and [2]. In short, both ADI
> and STM wanted some way to control/get configurations from a kind of
> IIO aggregate device. So discussions were made to have something that
> serves and can be used by everyone.
> 
> The main differences with the converter framework RFC [1]:
> 
> 1) Dropped the component framework. One can get more overview about
> the concerns on the references but the main reasons were:
>   * Relying on providing .remove() callbacks to be allowed to use device
>     managed functions. I was not even totally sure about the correctness
>     of it and in times where everyone tries to avoid that driver
>     callback, it could lead to some maintenance burden.
>   * Scalability issues. As mentioned in [2], to support backends defined
>     in FW child nodes was not so straightforward with the component
>     framework.
>   * Device links can already do some of the things that made me
>     try the component framework (eg: removing consumers on suppliers
>     unbind).
> 
> 2) Only support the minimal set of functionality to have the devices in
>     the same state as before using the backend framework. New features
>     will be added afterwards.
> 
> 3) Moved the API docs into the .c files.
> 
> 4) Moved the framework to the IIO top dir and renamed it to
>     industrialio-backend.c.
> 
> Also, as compared with the RFC in [2], I don't think there are that many
> similarities other than the filename. However, it should now be pretty
> straight for Olivier to build on top of it. Also to mention that I did
> grabbed patch 1 ("of: property: add device link support for
> io-backends") from that series and just did some minor changes:
> 

I did not already look at the framework patches in detail, but at first 
glance it looks fine.

I confirm that it has been quite straightforward to build on top of this 
framework, as it remains close to my first approach. I had only some 
small changes to do, to match the API, and to get it alive. This is great.

I see just one concern regarding ADC generic channel bindings support.
Here we have no devices associated to the channel sub-nodes. So, I 
cannot figure out we could use the devm_iio_backend_get() API, when 
looking for backend handle in channels sub-nodes. I have to think about it.

> 1) Renamed the property from "io-backend" to "io-backends".
> 2) No '#io-backend-cells' as it's not supported/needed by the framework
> (at least for now) .
> 
> Regarding the driver core patch
> ("driver: core: allow modifying device_links flags"), it is more like a
> RFC one. I'm not really sure if the current behavior isn't just
> expected/wanted. Since I could not really understand if it is or not
> (or why the different handling DL_FLAG_AUTOREMOVE_CONSUMER vs
> DL_FLAG_AUTOREMOVE_SUPPLIER), I'm sending out the patch.
> 
> Jonathan,
> 
> I also have some fixes and cleanups for the ad9467 driver. I added
> Fixes tags but I'm not sure if it's really worth it to backport them (given
> what we already discussed about these drivers). I'll leave that to you
> :).
> 
> I'm also not sure if I'm missing some tags (even though the series
> is frankly different from [2]).
> 
> Olivier,
> 
> If you want to be included as a Reviewer let me know and I'll happily do
> so in the next version.
> 

Yes, please add me as reviewer.
Thanks.
Olivier

> Also regarding the new IIO fw schemas. Should I send patches/PR to:
> 
> https://github.com/devicetree-org/dt-schema/
> 
> ? Or is there any other workflow for it?
> 
> [1]: https://lore.kernel.org/linux-iio/20230727150324.1157933-1-olivier.moysan@foss.st.com/
> [2]: https://lore.kernel.org/linux-iio/20230727150324.1157933-1-olivier.moysan@foss.st.com/
> 
> ---
> Nuno Sa (11):
>        driver: core: allow modifying device_links flags
>        iio: add the IIO backend framework
>        iio: adc: ad9467: fix reset gpio handling
>        iio: adc: ad9467: don't ignore error codes
>        iio: adc: ad9467: add mutex to struct ad9467_state
>        iio: adc: ad9467: fix scale setting
>        iio: adc: ad9467: use spi_get_device_match_data()
>        iio: adc: ad9467: use chip_info variables instead of array
>        iio: adc: ad9467: convert to backend framework
>        iio: adc: adi-axi-adc: convert to regmap
>        iio: adc: adi-axi-adc: move to backend framework
> 
> Olivier Moysan (1):
>        of: property: add device link support for io-backends
> 
>   MAINTAINERS                         |   7 +
>   drivers/base/core.c                 |  14 +-
>   drivers/iio/Kconfig                 |   5 +
>   drivers/iio/Makefile                |   1 +
>   drivers/iio/adc/Kconfig             |   3 +-
>   drivers/iio/adc/ad9467.c            | 382 +++++++++++++++++++++-----------
>   drivers/iio/adc/adi-axi-adc.c       | 429 +++++++-----------------------------
>   drivers/iio/industrialio-backend.c  | 302 +++++++++++++++++++++++++
>   drivers/of/property.c               |   2 +
>   include/linux/iio/adc/adi-axi-adc.h |   4 +
>   include/linux/iio/backend.h         |  58 +++++
>   11 files changed, 723 insertions(+), 484 deletions(-)
> 
> Thanks!
> - Nuno Sá
> 

Powered by blists - more mailing lists