by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 10/12] iio: adc: ad9467: convert to backend framework

2023-11-23 17:37:33

by Olivier MOYSAN

[permalink] [raw]

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/[email protected]/
> [2]: https://lore.kernel.org/linux-iio/[email protected]/
>
> ---
> 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á
>

2023-11-24 09:16:18

by Nuno Sá

[permalink] [raw]

Subject: Re: [PATCH 00/12] iio: add new backend framework

On Thu, 2023-11-23 at 18:36 +0100, Olivier MOYSAN wrote:
> 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.
>

Yeah, I'm keeping the series small (as Jonathan asked in the RFC) and just with basic
stuff needed to get the ad9647 driver in the exact same state as before the
framework. So yes, it's the same deal as with the component approach. You'll need to
add support for it. But, in this case, I believe it should be as straight as:

-/**
- * devm_iio_backend_get - Get a backend device
- * @dev: Device where to look for the backend.
- * @name: Backend name.
- *
- * Get's the backend associated with @dev.
- *
- * RETURNS:
- * A backend pointer, negative error pointer otherwise.
- */
-struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
+struct iio_backend *devm_fwnode_iio_backend_get(struct device *dev,
+ const struct fwnode_handle *fwnode,
+ const char *name)
{
- struct fwnode_handle *fwnode;
+ struct fwnode_handle *back_fwnode;
struct iio_backend *back;
int index = 0, ret;

@@ -195,20 +187,20 @@ struct iio_backend *devm_iio_backend_get(struct device *dev,
const char *name)
return ERR_PTR(index);
}

- fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index);
- if (IS_ERR(fwnode)) {
+ back_fwnode = fwnode_find_reference(fwnode, "io-backends", index);
+ if (IS_ERR(back_fwnode)) {
dev_err(dev, "Cannot get Firmware reference\n");
- return ERR_CAST(fwnode);
+ return ERR_CAST(back_fwnode);
}

guard(mutex)(&iio_back_lock);
list_for_each_entry(back, &iio_back_list, entry) {
struct device_link *link;

- if (!device_match_fwnode(back->dev, fwnode))
+ if (!device_match_fwnode(back->dev, back_fwnode))
continue;

- fwnode_handle_put(fwnode);
+ fwnode_handle_put(back_fwnode);
kref_get(&back->ref);
if (!try_module_get(back->owner)) {
dev_err(dev, "Cannot get module reference\n");
@@ -229,9 +221,25 @@ struct iio_backend *devm_iio_backend_get(struct device *dev,
const char *name)
return back;
}

- fwnode_handle_put(fwnode);
+ fwnode_handle_put(back_fwnode);
return ERR_PTR(-EPROBE_DEFER);
}
+EXPORT_SYMBOL_GPL(devm_fwnode_iio_backend_get);
+
+/**
+ * devm_iio_backend_get - Get a backend device
+ * @dev: Device where to look for the backend.
+ * @name: Backend name.
+ *
+ * Get's the backend associated with @dev.
+ *
+ * RETURNS:
+ * A backend pointer, negative error pointer otherwise.
+ */
+struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
+{
+ return devm_fwnode_iio_backend_get(dev, dev_fwnode(dev), name);
+}
EXPORT_SYMBOL_GPL(devm_iio_backend_get);

/**

Completely untested (not even compiled :)). Anyways, the goal is to just have the
minimum accepted and you can then send the needed patches for subnode lookups.

> > 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

Will do.

- Nuno Sá
>

2023-11-25 07:44:37

by kernel test robot

[permalink] [raw]

On Fri, 2023-12-01 at 09:41 +0100, Nuno Sá wrote:
> On Thu, 2023-11-30 at 17:54 -0600, David Lechner wrote:
> > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > <[email protected]> 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)
> > >
> >
> > The "typical provider - consumer stuff" seems pretty straight forward
> > for finding and connecting two different devices, but the definition
> > of what is a frontend and what is a backend seems a bit nebulous. It
> > would be nice to seem some example devicetree to be able to get a
> > better picture of how this will be used in practices (links to the the
> > hardware docs for those examples would be nice too).
> >
> > In addition to the backend ops given in this series, what are some
> > other expected ops that could be added in the future? Do we need some
> > kind of spec to say "I need a backend with feature X and feature Y" or
> > "I need a backend with compatible string" rather than just "I need a
> > generic backend"?

To also reply to this one, I also have somewhere a comment about it (I think in the
code itself). For now, I'm thinking in this to serve all kinds of IIO devices in a
generic way which means, yes, the ops structure can grow badly. I'm not so sure if
that will actually happen in practise but if it does we can always play some OOP
games and leave the backend with the generic stuff (like .enable()/.disable() and so
on) and extend it (for example: having a converters thing that would have ops more
suitable for ADCs/DACs). Anyways, for now I think that would be overcomplicating
things. It's in kernel interfaces so we can always change things.

- Nuno Sá

2023-12-01 09:17:39

by Nuno Sá

[permalink] [raw]

On Wed, 06 Dec 2023 13:05:53 +0100
Nuno Sá <[email protected]> wrote:

> On Mon, 2023-12-04 at 15:38 +0000, Jonathan Cameron wrote:
> > On Tue, 21 Nov 2023 11:20:16 +0100
> > Nuno Sa via B4 Relay <[email protected]> wrote:
> >
> > > From: Nuno Sa <[email protected]>
> > >
> > > 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).
> >
> > As this is first place backend / frontend terminology used (I think), make
> > sure to give an example so people understand what sorts of IP / devices thes
> > might be.
> >
> > >
> > > 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()
> > >
> > > Signed-off-by: Nuno Sa <[email protected]>
> >
> > Looks good to me in general. I'll need to have a really close read though
> > before we merge this as there may be sticky corners! (hopefully not)
> >
> >
> > ...
> >
> > > +static LIST_HEAD(iio_back_list);
> > > +static DEFINE_MUTEX(iio_back_lock);
> > > +
> > > +/*
> > > + * Helper macros to properly call backend ops. The main point for these macros
> > > + * is to properly lock the backend mutex on every call plus checking if the
> > > + * backend device is still around (by looking at the *ops pointer).
> > If just checking if it is around rather thank looking for a bug, then
> > I'd suggest a lighter choice than WARN_ON_x
> >
>
> Arguably, in here, removing a backend is the user doing something seriously wrong so
> I see the splat with good eyes :D.
>
> That said, I'm fine in turning this into a pr_warn_once()...
>
> > Btw, there were some interesting discussions on lifetimes and consumer / provider
> > models at plumbers. I think https://www.youtube.com/watch?v=bHaMMnIH6AM will be
> > the video. Suggested the approach of not refcounting but instead allowing for
> > a deliberate removal of access similar to your check on ops here (and the one
> > we do in core IIO for similar purposes). Sounded interesting but I've not
> > explored what it would really mean to switch to that model yet.
>
> Yes, interesting talk indeed. I have been following this issue for some time now.
> That's why I tried to be careful in the backend stuff (so we don't explode if a
> backend is gone) even though is a much more simpler approach. But the talk mentions
> three solutions and we kind of have both option C (the pointer stuff) and option A
> (consumer removed on provicer unbind)
> in here. option A is being given through device links with the AUTO_REMOVE_CONSUMER
> flag.
>
> And the talk actually left me thinking on that (as it's discussed in there. In our
> simpler case (ADI ones), it does make sense to remove the consumer if the provider is
> not there. But if we think in more advanced usecases (or maybe already in the STM
> usecase) where we have a backend per data path. Does it make sense to completely
> "kill" the consumer if we remove one of the data paths? Starting to think it
> doesn't...

There is a reasonably argument that partial tear down isn't a common case. So
may not be worth worrying about.

J
>
> - Nuno Sá
>