[PATCH v7 0/9] iio: add new backend framework

[PATCH v7 0/9] iio: add new backend framework

[Nuno Sa via B4 Relay]

v1:
 https://lore.kernel.org/linux-iio/20231204144925.4fe9922f@jic23-huawei/T/#m222f5175273b81dbfe40b7f0daffcdc67d6cb8ff

v2:
 https://lore.kernel.org/r/20231208-dev-iio-backend-v2-0-5450951895e1@analog.com

v3:
 https://lore.kernel.org/linux-iio/20231213-dev-iio-backend-v3-0-bb9f12a5c6dc@analog.com/

v4:
 https://lore.kernel.org/r/20231220-iio-backend-v4-0-998e9148b692@analog.com

v5:
 https://lore.kernel.org/r/20240112-iio-backend-v5-0-bdecad041ab4@analog.com

v6:
 https://lore.kernel.org/r/20240119-iio-backend-v6-0-189536c35a05@analog.com

Changes in v7:
 - Patch 1
  * New patch.
 - Patch 3
  * Added Rob's tag.
 - Patch 5
  * Renamed '#io-backends-cells' -> '#io-backend-cells'.
 - Patch 8
  * Added a statement in the commit message about the ABI breakage;
  * Used local info variables in a couple of forgotten places.

Rob, I was thinking in sending a different patch for the io-channels fix
but then I changed my mind and included it in this series as it is then easier
to apply the io-backends patch on top of it. If you prefer for me to
send that patch alone, I'll do so.

Keeping the block diagram  so we don't have to follow links
to check one of the typical setups.

                                           -------------------------------------------------------
 ------------------                        | -----------         ------------      -------  FPGA |
 |     ADC        |------------------------| | AXI ADC |---------| DMA CORE |------| RAM |       |
 | (Frontend/IIO) | Serial Data (eg: LVDS) | |(backend)|---------|          |------|     |       |
 |                |------------------------| -----------         ------------      -------       |
 ------------------                        -------------------------------------------------------

---
Nuno Sa (8):
      of: property: fix typo in io-channels
      dt-bindings: adc: ad9467: add new io-backend property
      dt-bindings: adc: axi-adc: update bindings for backend framework
      driver: core: allow modifying device_links flags
      iio: buffer-dmaengine: export buffer alloc and free functions
      iio: add the IIO backend framework
      iio: adc: ad9467: convert to backend framework
      iio: adc: adi-axi-adc: move to backend framework

Olivier Moysan (1):
      of: property: add device link support for io-backends

 .../devicetree/bindings/iio/adc/adi,ad9467.yaml    |   4 +
 .../devicetree/bindings/iio/adc/adi,axi-adc.yaml   |   8 +-
 MAINTAINERS                                        |   8 +
 drivers/base/core.c                                |  14 +-
 drivers/iio/Kconfig                                |   9 +
 drivers/iio/Makefile                               |   1 +
 drivers/iio/adc/Kconfig                            |   4 +-
 drivers/iio/adc/ad9467.c                           | 268 +++++++++-----
 drivers/iio/adc/adi-axi-adc.c                      | 383 +++++--------------
 drivers/iio/buffer/industrialio-buffer-dmaengine.c |   8 +-
 drivers/iio/industrialio-backend.c                 | 412 +++++++++++++++++++++
 drivers/of/property.c                              |   4 +-
 include/linux/iio/adc/adi-axi-adc.h                |  68 ----
 include/linux/iio/backend.h                        |  72 ++++
 include/linux/iio/buffer-dmaengine.h               |   3 +
 15 files changed, 805 insertions(+), 461 deletions(-)
---
base-commit: 824adc9cc314be065db726e30fae876c87660775
change-id: 20231219-iio-backend-a3dc1a6a7a58
--

Thanks!
- Nuno Sá

[PATCH v7 1/9] of: property: fix typo in io-channels

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

The property is io-channels and not io-channel. This was effectively
preventing the devlink creation.

Fixes: 8e12257dead7 ("of: property: Add device link support for iommus, mboxes and io-channels")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/of/property.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 641a40cf5cf3..bbf0dee2fb9c 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1244,7 +1244,7 @@ DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")
 DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells")
 DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
 DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
-DEFINE_SIMPLE_PROP(io_channels, "io-channel", "#io-channel-cells")
+DEFINE_SIMPLE_PROP(io_channels, "io-channels", "#io-channel-cells")
 DEFINE_SIMPLE_PROP(interrupt_parent, "interrupt-parent", NULL)
 DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
 DEFINE_SIMPLE_PROP(power_domains, "power-domains", "#power-domain-cells")

-- 
2.43.0

Re: [PATCH v7 1/9] of: property: fix typo in io-channels

[Saravana Kannan]

On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
<devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> From: Nuno Sa <nuno.sa@analog.com>
>
> The property is io-channels and not io-channel. This was effectively
> preventing the devlink creation.

Thank you!

Reviewed-by: Saravana Kannan <saravanak@google.com>

>
> Fixes: 8e12257dead7 ("of: property: Add device link support for iommus, mboxes and io-channels")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/of/property.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 641a40cf5cf3..bbf0dee2fb9c 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1244,7 +1244,7 @@ DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")
>  DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells")
>  DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
>  DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
> -DEFINE_SIMPLE_PROP(io_channels, "io-channel", "#io-channel-cells")
> +DEFINE_SIMPLE_PROP(io_channels, "io-channels", "#io-channel-cells")
>  DEFINE_SIMPLE_PROP(interrupt_parent, "interrupt-parent", NULL)
>  DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
>  DEFINE_SIMPLE_PROP(power_domains, "power-domains", "#power-domain-cells")
>
> --
> 2.43.0
>

Re: [PATCH v7 1/9] of: property: fix typo in io-channels

[Jonathan Cameron]

On Wed, 24 Jan 2024 19:14:35 -0800
Saravana Kannan <saravanak@google.com> wrote:

> On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> <devnull+nuno.sa.analog.com@kernel.org> wrote:
> >
> > From: Nuno Sa <nuno.sa@analog.com>
> >
> > The property is io-channels and not io-channel. This was effectively
> > preventing the devlink creation.  

I'm a bit lost on the devlink side of things. Is this something with
effects on existing drivers before this patch set, or is it only causing
real problems with the changes in here?

i.e. Should we mark it for stable and rush it in as a fix, or can it wait
for the rest of the set?

Jonathan

> 
> Thank you!
> 
> Reviewed-by: Saravana Kannan <saravanak@google.com>

> 
> >
> > Fixes: 8e12257dead7 ("of: property: Add device link support for iommus, mboxes and io-channels")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/of/property.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 641a40cf5cf3..bbf0dee2fb9c 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1244,7 +1244,7 @@ DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")
> >  DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells")
> >  DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
> >  DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
> > -DEFINE_SIMPLE_PROP(io_channels, "io-channel", "#io-channel-cells")
> > +DEFINE_SIMPLE_PROP(io_channels, "io-channels", "#io-channel-cells")
> >  DEFINE_SIMPLE_PROP(interrupt_parent, "interrupt-parent", NULL)
> >  DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
> >  DEFINE_SIMPLE_PROP(power_domains, "power-domains", "#power-domain-cells")
> >
> > --
> > 2.43.0
> >  

Re: [PATCH v7 1/9] of: property: fix typo in io-channels

[Jonathan Cameron]

On Sat, 27 Jan 2024 15:07:26 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed, 24 Jan 2024 19:14:35 -0800
> Saravana Kannan <saravanak@google.com> wrote:
> 
> > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > <devnull+nuno.sa.analog.com@kernel.org> wrote:  
> > >
> > > From: Nuno Sa <nuno.sa@analog.com>
> > >
> > > The property is io-channels and not io-channel. This was effectively
> > > preventing the devlink creation.    
> 
> I'm a bit lost on the devlink side of things. Is this something with
> effects on existing drivers before this patch set, or is it only causing
> real problems with the changes in here?
> 
> i.e. Should we mark it for stable and rush it in as a fix, or can it wait
> for the rest of the set?
> 
I'm also fine with this going via DT tree if that makes more sense.
If that happens.

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> Jonathan
> 
> > 
> > Thank you!
> > 
> > Reviewed-by: Saravana Kannan <saravanak@google.com>  
> 
> >   
> > >
> > > Fixes: 8e12257dead7 ("of: property: Add device link support for iommus, mboxes and io-channels")
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > > ---
> > >  drivers/of/property.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index 641a40cf5cf3..bbf0dee2fb9c 100644
> > > --- a/drivers/of/property.c
> > > +++ b/drivers/of/property.c
> > > @@ -1244,7 +1244,7 @@ DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")
> > >  DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells")
> > >  DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
> > >  DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
> > > -DEFINE_SIMPLE_PROP(io_channels, "io-channel", "#io-channel-cells")
> > > +DEFINE_SIMPLE_PROP(io_channels, "io-channels", "#io-channel-cells")
> > >  DEFINE_SIMPLE_PROP(interrupt_parent, "interrupt-parent", NULL)
> > >  DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
> > >  DEFINE_SIMPLE_PROP(power_domains, "power-domains", "#power-domain-cells")
> > >
> > > --
> > > 2.43.0
> > >    
> 
> 

Re: [PATCH v7 1/9] of: property: fix typo in io-channels

[Nuno Sá]

On Sat, 2024-01-27 at 15:07 +0000, Jonathan Cameron wrote:
> On Wed, 24 Jan 2024 19:14:35 -0800
> Saravana Kannan <saravanak@google.com> wrote:
> 
> > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > 
> > > From: Nuno Sa <nuno.sa@analog.com>
> > > 
> > > The property is io-channels and not io-channel. This was effectively
> > > preventing the devlink creation.  
> 
> I'm a bit lost on the devlink side of things. Is this something with
> effects on existing drivers before this patch set, or is it only causing
> real problems with the changes in here?
> 
> i.e. Should we mark it for stable and rush it in as a fix, or can it wait
> for the rest of the set?
> 

I guess you already figured this out? Anyways, this is a real fix that was
affecting any consumer using io-channels (so nothing to do with the current
series). Since the there was a typo, no fw_links were being created.

I stated in the cover why I included this. Just to make the addition of io-
backends later on easier. But if Rob prefers (or you), I can just send this
standalone patch to the devicetree list.

- Nuno Sá
> 
> 

Re: [PATCH v7 1/9] of: property: fix typo in io-channels

[Saravana Kannan]

On Mon, Jan 29, 2024 at 12:15 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Sat, 2024-01-27 at 15:07 +0000, Jonathan Cameron wrote:
> > On Wed, 24 Jan 2024 19:14:35 -0800
> > Saravana Kannan <saravanak@google.com> wrote:
> >
> > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > >
> > > > From: Nuno Sa <nuno.sa@analog.com>
> > > >
> > > > The property is io-channels and not io-channel. This was effectively
> > > > preventing the devlink creation.
> >
> > I'm a bit lost on the devlink side of things. Is this something with
> > effects on existing drivers before this patch set, or is it only causing
> > real problems with the changes in here?
> >
> > i.e. Should we mark it for stable and rush it in as a fix, or can it wait
> > for the rest of the set?
> >
>
> I guess you already figured this out? Anyways, this is a real fix that was
> affecting any consumer using io-channels (so nothing to do with the current
> series). Since the there was a typo, no fw_links were being created.
>
> I stated in the cover why I included this. Just to make the addition of io-
> backends later on easier. But if Rob prefers (or you), I can just send this
> standalone patch to the devicetree list.

I think you should send this to Rob directly as a DT fix. I already
gave you a Reviewed-by too. So it should land quickly.

This is a real bug that we want to fix as soon as we can. And your
series might take a bit longer to land.

-Saravana

Re: [PATCH v7 1/9] of: property: fix typo in io-channels

[Nuno Sá]

On Mon, 2024-01-29 at 14:33 -0800, Saravana Kannan wrote:
> On Mon, Jan 29, 2024 at 12:15 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > On Sat, 2024-01-27 at 15:07 +0000, Jonathan Cameron wrote:
> > > On Wed, 24 Jan 2024 19:14:35 -0800
> > > Saravana Kannan <saravanak@google.com> wrote:
> > > 
> > > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > > > 
> > > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > > 
> > > > > The property is io-channels and not io-channel. This was effectively
> > > > > preventing the devlink creation.
> > > 
> > > I'm a bit lost on the devlink side of things. Is this something with
> > > effects on existing drivers before this patch set, or is it only causing
> > > real problems with the changes in here?
> > > 
> > > i.e. Should we mark it for stable and rush it in as a fix, or can it wait
> > > for the rest of the set?
> > > 
> > 
> > I guess you already figured this out? Anyways, this is a real fix that was
> > affecting any consumer using io-channels (so nothing to do with the current
> > series). Since the there was a typo, no fw_links were being created.
> > 
> > I stated in the cover why I included this. Just to make the addition of io-
> > backends later on easier. But if Rob prefers (or you), I can just send this
> > standalone patch to the devicetree list.
> 
> I think you should send this to Rob directly as a DT fix. I already
> gave you a Reviewed-by too. So it should land quickly.
> 
> This is a real bug that we want to fix as soon as we can. And your
> series might take a bit longer to land.
> 
> -Saravana

Alright... will do that

- Nuno Sá

Re: [PATCH v7 1/9] of: property: fix typo in io-channels

[Rob Herring]

On Tue, Jan 30, 2024 at 11:32:00AM +0100, Nuno Sá wrote:
> On Mon, 2024-01-29 at 14:33 -0800, Saravana Kannan wrote:
> > On Mon, Jan 29, 2024 at 12:15 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > 
> > > On Sat, 2024-01-27 at 15:07 +0000, Jonathan Cameron wrote:
> > > > On Wed, 24 Jan 2024 19:14:35 -0800
> > > > Saravana Kannan <saravanak@google.com> wrote:
> > > > 
> > > > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > > > > 
> > > > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > > > 
> > > > > > The property is io-channels and not io-channel. This was effectively
> > > > > > preventing the devlink creation.
> > > > 
> > > > I'm a bit lost on the devlink side of things. Is this something with
> > > > effects on existing drivers before this patch set, or is it only causing
> > > > real problems with the changes in here?
> > > > 
> > > > i.e. Should we mark it for stable and rush it in as a fix, or can it wait
> > > > for the rest of the set?
> > > > 
> > > 
> > > I guess you already figured this out? Anyways, this is a real fix that was
> > > affecting any consumer using io-channels (so nothing to do with the current
> > > series). Since the there was a typo, no fw_links were being created.
> > > 
> > > I stated in the cover why I included this. Just to make the addition of io-
> > > backends later on easier. But if Rob prefers (or you), I can just send this
> > > standalone patch to the devicetree list.
> > 
> > I think you should send this to Rob directly as a DT fix. I already
> > gave you a Reviewed-by too. So it should land quickly.
> > 
> > This is a real bug that we want to fix as soon as we can. And your
> > series might take a bit longer to land.
> > 
> > -Saravana
> 
> Alright... will do that

No need to resend. I'll apply this one.

Rob

Re: [PATCH v7 1/9] of: property: fix typo in io-channels

[Rob Herring]

On Tue, 23 Jan 2024 16:14:22 +0100, Nuno Sa wrote:
> The property is io-channels and not io-channel. This was effectively
> preventing the devlink creation.
> 
> Fixes: 8e12257dead7 ("of: property: Add device link support for iommus, mboxes and io-channels")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/of/property.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Applied, thanks!

Re: [PATCH v7 1/9] of: property: fix typo in io-channels

[Nuno Sá]

On Tue, 2024-01-30 at 14:54 -0600, Rob Herring wrote:
> 
> On Tue, 23 Jan 2024 16:14:22 +0100, Nuno Sa wrote:
> > The property is io-channels and not io-channel. This was effectively
> > preventing the devlink creation.
> > 
> > Fixes: 8e12257dead7 ("of: property: Add device link support for iommus,
> > mboxes and io-channels")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  drivers/of/property.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> 
> Applied, thanks!
> 

Thanks Rob!

- Nuno Sá

[PATCH v7 2/9] dt-bindings: adc: ad9467: add new io-backend property

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

The ad9467 will make use of the new IIO backend framework which is a
provider - consumer interface where IIO backends provide services to
consumers. As such, and being this device a consumer,  add the new
generic io-backend property to the bindings.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml
index 7aa748d6b7a0..eecd5fbab695 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad9467.yaml
@@ -44,6 +44,9 @@ properties:
       Pin that controls the powerdown mode of the device.
     maxItems: 1
 
+  io-backends:
+    maxItems: 1
+
   reset-gpios:
     description:
       Reset pin for the device.
@@ -68,6 +71,7 @@ examples:
             reg = <0>;
             clocks = <&adc_clk>;
             clock-names = "adc-clk";
+            io-backends = <&iio_backend>;
         };
     };
 ...

-- 
2.43.0

[PATCH v7 3/9] dt-bindings: adc: axi-adc: update bindings for backend framework

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

'adi,adc-dev' is now deprecated and must not be used anymore. Hence,
also remove it from being required.

The reason why it's being deprecated is because the axi-adc CORE is now
an IIO service provider hardware (IIO backends) for consumers to make use
of. Before, the logic with 'adi,adc-dev' was the opposite (it was kind
of consumer referencing other nodes/devices) and that proved to be wrong
and to not scale.

Now, IIO consumers of this hardware are expected to reference it using the
io-backends property. Hence, the new '#io-backend-cells' is being added
so the device is easily identified as a provider.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml b/Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml
index 9996dd93f84b..add10b22dcac 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml
@@ -39,12 +39,15 @@ properties:
     $ref: /schemas/types.yaml#/definitions/phandle
     description:
       A reference to a the actual ADC to which this FPGA ADC interfaces to.
+    deprecated: true
+
+  '#io-backends-cells'
+    const: 0
 
 required:
   - compatible
   - dmas
   - reg
-  - adi,adc-dev
 
 additionalProperties: false
 
@@ -55,7 +58,6 @@ examples:
         reg = <0x44a00000 0x10000>;
         dmas = <&rx_dma 0>;
         dma-names = "rx";
-
-        adi,adc-dev = <&spi_adc>;
+        #io-backends-cells = <0>;
     };
 ...

-- 
2.43.0

Re: [PATCH v7 3/9] dt-bindings: adc: axi-adc: update bindings for backend framework

[Rob Herring]

On Tue, 23 Jan 2024 16:14:24 +0100, Nuno Sa wrote:
> 'adi,adc-dev' is now deprecated and must not be used anymore. Hence,
> also remove it from being required.
> 
> The reason why it's being deprecated is because the axi-adc CORE is now
> an IIO service provider hardware (IIO backends) for consumers to make use
> of. Before, the logic with 'adi,adc-dev' was the opposite (it was kind
> of consumer referencing other nodes/devices) and that proved to be wrong
> and to not scale.
> 
> Now, IIO consumers of this hardware are expected to reference it using the
> io-backends property. Hence, the new '#io-backend-cells' is being added
> so the device is easily identified as a provider.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml:45:5: [error] syntax error: could not find expected ':' (syntax)

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml:45:5: could not find expected ':'
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml: ignoring, error parsing file
make[2]: *** Deleting file 'Documentation/devicetree/bindings/iio/adc/adi,axi-adc.example.dts'
Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml:45:5: could not find expected ':'
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/iio/adc/adi,axi-adc.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1428: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240123-iio-backend-v7-3-1bff236b8693@analog.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

[PATCH v7 4/9] driver: core: allow modifying device_links flags

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

If a device_link is previously created (eg: via
fw_devlink_create_devlink()) before the supplier + consumer are both
present and bound to their respective drivers, there's no way to set
DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
DL_FLAG_AUTOREMOVE_SUPPLIER is done.

While at it, make sure that we are never left with
DL_FLAG_AUTOPROBE_CONSUMER set together with one of
DL_FLAG_AUTOREMOVE_CONSUMER or DL_FLAG_AUTOREMOVE_SUPPLIER.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/base/core.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 14d46af40f9a..ee8a46df28e1 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -807,11 +807,15 @@ struct device_link *device_link_add(struct device *consumer,
 		 * update the existing link to stay around longer.
 		 */
 		if (flags & DL_FLAG_AUTOREMOVE_SUPPLIER) {
-			if (link->flags & DL_FLAG_AUTOREMOVE_CONSUMER) {
-				link->flags &= ~DL_FLAG_AUTOREMOVE_CONSUMER;
-				link->flags |= DL_FLAG_AUTOREMOVE_SUPPLIER;
-			}
-		} else if (!(flags & DL_FLAG_AUTOREMOVE_CONSUMER)) {
+			link->flags &= ~DL_FLAG_AUTOREMOVE_CONSUMER;
+			link->flags &= ~DL_FLAG_AUTOPROBE_CONSUMER;
+			link->flags |= DL_FLAG_AUTOREMOVE_SUPPLIER;
+
+		} else if (flags & DL_FLAG_AUTOREMOVE_CONSUMER) {
+			link->flags &= ~DL_FLAG_AUTOREMOVE_SUPPLIER;
+			link->flags &= ~DL_FLAG_AUTOPROBE_CONSUMER;
+			link->flags |= DL_FLAG_AUTOREMOVE_CONSUMER;
+		} else {
 			link->flags &= ~(DL_FLAG_AUTOREMOVE_CONSUMER |
 					 DL_FLAG_AUTOREMOVE_SUPPLIER);
 		}

-- 
2.43.0

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Saravana Kannan]

On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
<devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> From: Nuno Sa <nuno.sa@analog.com>
>
> If a device_link is previously created (eg: via
> fw_devlink_create_devlink()) before the supplier + consumer are both
> present and bound to their respective drivers, there's no way to set
> DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
> for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> DL_FLAG_AUTOREMOVE_SUPPLIER is done.

Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
Especially if fw_devlink already created the link? You are effectively
trying to delete the link fw_devlink created if any of your devices
unbind.

> While at it, make sure that we are never left with
> DL_FLAG_AUTOPROBE_CONSUMER set together with one of
> DL_FLAG_AUTOREMOVE_CONSUMER or DL_FLAG_AUTOREMOVE_SUPPLIER.

fw_devlink sets AUTOPROBE_CONSUMER. So, are you trying to clear it? Why?

I almost want to NACK this, but I'll hear more before I do.

-Saravana

>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/base/core.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 14d46af40f9a..ee8a46df28e1 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -807,11 +807,15 @@ struct device_link *device_link_add(struct device *consumer,
>                  * update the existing link to stay around longer.
>                  */
>                 if (flags & DL_FLAG_AUTOREMOVE_SUPPLIER) {
> -                       if (link->flags & DL_FLAG_AUTOREMOVE_CONSUMER) {
> -                               link->flags &= ~DL_FLAG_AUTOREMOVE_CONSUMER;
> -                               link->flags |= DL_FLAG_AUTOREMOVE_SUPPLIER;
> -                       }
> -               } else if (!(flags & DL_FLAG_AUTOREMOVE_CONSUMER)) {
> +                       link->flags &= ~DL_FLAG_AUTOREMOVE_CONSUMER;
> +                       link->flags &= ~DL_FLAG_AUTOPROBE_CONSUMER;
> +                       link->flags |= DL_FLAG_AUTOREMOVE_SUPPLIER;
> +
> +               } else if (flags & DL_FLAG_AUTOREMOVE_CONSUMER) {
> +                       link->flags &= ~DL_FLAG_AUTOREMOVE_SUPPLIER;
> +                       link->flags &= ~DL_FLAG_AUTOPROBE_CONSUMER;
> +                       link->flags |= DL_FLAG_AUTOREMOVE_CONSUMER;
> +               } else {
>                         link->flags &= ~(DL_FLAG_AUTOREMOVE_CONSUMER |
>                                          DL_FLAG_AUTOREMOVE_SUPPLIER);
>                 }
>
> --
> 2.43.0
>

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Nuno Sá]

Hi Saravana,

Thanks for your feedback,

On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:
> On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > If a device_link is previously created (eg: via
> > fw_devlink_create_devlink()) before the supplier + consumer are both
> > present and bound to their respective drivers, there's no way to set
> > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
> > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > DL_FLAG_AUTOREMOVE_SUPPLIER is done.
> 
> Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> Especially if fw_devlink already created the link? You are effectively
> trying to delete the link fw_devlink created if any of your devices
> unbind.
> 

Well, this is still useful in the modules case as the link will be relaxed after
all devices are initialized and that will already clear AUTOPROBE_CONSUMER
AFAIU. But, more importantly, if I'm not missing anything, in [1], fw_devlinks
will be dropped after the consumer + supplier are bound which means I definitely
want to create a link between my consumer and supplier. 

FWIW, I was misunderstanding things since I thought DL_FLAG_AUTOREMOVE_CONSUMER
was needed to make sure the consumer is unbound before the supplier. But for
that I think I can even pass 0 in the flags as I only need the link to be
MANAGED. Still, I think having DL_FLAG_AUTOREMOVE_CONSUMER makes sense.

Also note that there are more places in the kernel with
DL_FLAG_AUTOREMOVE_CONSUMER and that flag is likely being ignored in case the
link already exists.

I'm also clearing DL_FLAG_AUTOPROBE_CONSUMER as from the first check in
device_link_add(() check I realize that we can't/shouldn't have it together with
one of AUTOREMOVE_CONSUMER | AUTOREMOVE_SUPPLIER, right? At this point,
AUTOPROBE_CONSUMER is also likely not that useful anymore as both supplier and
consumer are up and I guess that's the typical case for subsystems/drivers to
call device_link_add().

And since I have your attention, it would be nice if you could look in another
sensible patch [2] that I've resended 3 times already. You're not in CC but I
see you've done quite some work in dev_links so... Completely unrelated I know
:)

[1]: https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L1292
[2]: https://lore.kernel.org/all/20231213-fix-device-links-overlays-v1-1-f091b213777c@analog.com/#t

- Nuno Sá
> 

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Nuno Sá]

On Thu, 2024-01-25 at 09:14 +0100, Nuno Sá wrote:
> 
> Hi Saravana,
> 
> Thanks for your feedback,
> 
> On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:
> > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > 
> > > From: Nuno Sa <nuno.sa@analog.com>
> > > 
> > > If a device_link is previously created (eg: via
> > > fw_devlink_create_devlink()) before the supplier + consumer are both
> > > present and bound to their respective drivers, there's no way to set
> > > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
> > > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > > DL_FLAG_AUTOREMOVE_SUPPLIER is done.
> > 
> > Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> > Especially if fw_devlink already created the link? You are effectively
> > trying to delete the link fw_devlink created if any of your devices
> > unbind.
> > 
> 
> Well, this is still useful in the modules case as the link will be relaxed
> after
> all devices are initialized and that will already clear AUTOPROBE_CONSUMER
> AFAIU. But, more importantly, if I'm not missing anything, in [1], fw_devlinks
> will be dropped after the consumer + supplier are bound which means I
> definitely
> want to create a link between my consumer and supplier. 
> 

Ok, so to add a bit more on this, there are two cases:

1) Both sup and con are modules and after boot up, the link is relaxed and thus
turned into a sync_state_only link. That means the link will be deleted anyways
and AUTOPROBE_CONSUMER is already cleared by the time we try to change the link.

2) The built-in case where the link is kept as created by fw_devlink and this
patch effectively clears AUTOPROBE_CONSUMER.

Given the above, not sure what's the best option. I can think of 4:

1) Drop this patch and leave things as they are. DL_FLAG_AUTOREMOVE_CONSUMER is
pretty much ignored in my call but it will turn the link in a MANAGED one and
clear SYNC_STATE_ONLY. I could very well just pass 0 in the flags as
DL_FLAG_AUTOREMOVE_CONSUMER is always ignored;

2) Rework this patch so we can still change an existing link to accept
DL_FLAG_AUTOREMOVE_CONSUMER (in the modules case for example).

However, instead of clearing AUTOPROBE_CONSUMER, I would add some checks so if
flags have one of DL_FLAG_AUTOREMOVE_SUPPLIER or DL_FLAG_AUTOREMOVE_CONSUMER and
AUTOPROBE_CONSUMER is already set, we ignore them. In fact, right now, I think
one could pass DL_FLAG_AUTOREMOVE_SUPPLIER and link->flags ends ups with
AUTOREMOVE_SUPPLIER | AUTOPROBE_CONSUMER which in theory is not allowed...

3) Keep it as-is... This one is likely a NACK as I'm getting the feeling that
clearing stuff that might have been created by fw_devlinks is probably a no-go.

Let me know your thoughts...

- Nuno Sá

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Rafael J. Wysocki]

On Thu, Jan 25, 2024 at 4:31 PM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Thu, 2024-01-25 at 09:14 +0100, Nuno Sá wrote:
> >
> > Hi Saravana,
> >
> > Thanks for your feedback,
> >
> > On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:
> > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > >
> > > > From: Nuno Sa <nuno.sa@analog.com>
> > > >
> > > > If a device_link is previously created (eg: via
> > > > fw_devlink_create_devlink()) before the supplier + consumer are both
> > > > present and bound to their respective drivers, there's no way to set
> > > > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > > > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
> > > > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > > > DL_FLAG_AUTOREMOVE_SUPPLIER is done.
> > >
> > > Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> > > Especially if fw_devlink already created the link? You are effectively
> > > trying to delete the link fw_devlink created if any of your devices
> > > unbind.
> > >
> >
> > Well, this is still useful in the modules case as the link will be relaxed
> > after
> > all devices are initialized and that will already clear AUTOPROBE_CONSUMER
> > AFAIU. But, more importantly, if I'm not missing anything, in [1], fw_devlinks
> > will be dropped after the consumer + supplier are bound which means I
> > definitely
> > want to create a link between my consumer and supplier.
> >
>
> Ok, so to add a bit more on this, there are two cases:
>
> 1) Both sup and con are modules and after boot up, the link is relaxed and thus
> turned into a sync_state_only link. That means the link will be deleted anyways
> and AUTOPROBE_CONSUMER is already cleared by the time we try to change the link.
>
> 2) The built-in case where the link is kept as created by fw_devlink and this
> patch effectively clears AUTOPROBE_CONSUMER.
>
> Given the above, not sure what's the best option. I can think of 4:
>
> 1) Drop this patch and leave things as they are. DL_FLAG_AUTOREMOVE_CONSUMER is
> pretty much ignored in my call but it will turn the link in a MANAGED one and
> clear SYNC_STATE_ONLY. I could very well just pass 0 in the flags as
> DL_FLAG_AUTOREMOVE_CONSUMER is always ignored;
>
> 2) Rework this patch so we can still change an existing link to accept
> DL_FLAG_AUTOREMOVE_CONSUMER (in the modules case for example).
>
> However, instead of clearing AUTOPROBE_CONSUMER, I would add some checks so if
> flags have one of DL_FLAG_AUTOREMOVE_SUPPLIER or DL_FLAG_AUTOREMOVE_CONSUMER and
> AUTOPROBE_CONSUMER is already set, we ignore them. In fact, right now, I think
> one could pass DL_FLAG_AUTOREMOVE_SUPPLIER and link->flags ends ups with
> AUTOREMOVE_SUPPLIER | AUTOPROBE_CONSUMER which in theory is not allowed...

No, because DL_FLAG_AUTOREMOVE_SUPPLIER is only added to the link
flags if DL_FLAG_AUTOREMOVE_CONSUMER is already set in there and the
former replaces the latter.

Now, DL_FLAG_AUTOREMOVE_CONSUMER cannot be set in the link flags if
AUTOPROBE_CONSUMER is set in there.

> 3) Keep it as-is... This one is likely a NACK as I'm getting the feeling that
> clearing stuff that might have been created by fw_devlinks is probably a no-go.
>
> Let me know your thoughts...

If the original creator of the link didn't indicate either
DL_FLAG_AUTOREMOVE_CONSUMER, or DL_FLAG_AUTOREMOVE_SUPPLIER, they are
expected to need the link to stay around until it is explicitly
deleted.

Therefore adding any of these flags for an existing link where they
both are unset would be a mistake, because it would effectively cause
the link to live shorter than expected by the original creator and
that might lead to correctness issues.

Thanks!

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Nuno Sá]

On Thu, 2024-01-25 at 17:57 +0100, Rafael J. Wysocki wrote:
> On Thu, Jan 25, 2024 at 4:31 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > On Thu, 2024-01-25 at 09:14 +0100, Nuno Sá wrote:
> > > 
> > > Hi Saravana,
> > > 
> > > Thanks for your feedback,
> > > 
> > > On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:
> > > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > > > 
> > > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > > 
> > > > > If a device_link is previously created (eg: via
> > > > > fw_devlink_create_devlink()) before the supplier + consumer are both
> > > > > present and bound to their respective drivers, there's no way to set
> > > > > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > > > > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
> > > > > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > > > > DL_FLAG_AUTOREMOVE_SUPPLIER is done.
> > > > 
> > > > Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> > > > Especially if fw_devlink already created the link? You are effectively
> > > > trying to delete the link fw_devlink created if any of your devices
> > > > unbind.
> > > > 
> > > 
> > > Well, this is still useful in the modules case as the link will be relaxed
> > > after
> > > all devices are initialized and that will already clear AUTOPROBE_CONSUMER
> > > AFAIU. But, more importantly, if I'm not missing anything, in [1],
> > > fw_devlinks
> > > will be dropped after the consumer + supplier are bound which means I
> > > definitely
> > > want to create a link between my consumer and supplier.
> > > 
> > 
> > Ok, so to add a bit more on this, there are two cases:
> > 
> > 1) Both sup and con are modules and after boot up, the link is relaxed and
> > thus
> > turned into a sync_state_only link. That means the link will be deleted
> > anyways
> > and AUTOPROBE_CONSUMER is already cleared by the time we try to change the
> > link.
> > 
> > 2) The built-in case where the link is kept as created by fw_devlink and
> > this
> > patch effectively clears AUTOPROBE_CONSUMER.
> > 
> > Given the above, not sure what's the best option. I can think of 4:
> > 
> > 1) Drop this patch and leave things as they are. DL_FLAG_AUTOREMOVE_CONSUMER
> > is
> > pretty much ignored in my call but it will turn the link in a MANAGED one
> > and
> > clear SYNC_STATE_ONLY. I could very well just pass 0 in the flags as
> > DL_FLAG_AUTOREMOVE_CONSUMER is always ignored;
> > 
> > 2) Rework this patch so we can still change an existing link to accept
> > DL_FLAG_AUTOREMOVE_CONSUMER (in the modules case for example).
> > 
> > However, instead of clearing AUTOPROBE_CONSUMER, I would add some checks so
> > if
> > flags have one of DL_FLAG_AUTOREMOVE_SUPPLIER or DL_FLAG_AUTOREMOVE_CONSUMER
> > and
> > AUTOPROBE_CONSUMER is already set, we ignore them. In fact, right now, I
> > think
> > one could pass DL_FLAG_AUTOREMOVE_SUPPLIER and link->flags ends ups with
> > AUTOREMOVE_SUPPLIER | AUTOPROBE_CONSUMER which in theory is not allowed...
> 
> No, because DL_FLAG_AUTOREMOVE_SUPPLIER is only added to the link
> flags if DL_FLAG_AUTOREMOVE_CONSUMER is already set in there and the
> former replaces the latter.
> 

Oh yes, I missed that extra if() against the DL_FLAG_AUTOREMOVE_CONSUMER flag...

> Now, DL_FLAG_AUTOREMOVE_CONSUMER cannot be set in the link flags if
> AUTOPROBE_CONSUMER is set in there.
> 
> > 3) Keep it as-is... This one is likely a NACK as I'm getting the feeling
> > that
> > clearing stuff that might have been created by fw_devlinks is probably a no-
> > go.
> > 
> > Let me know your thoughts...
> 
> If the original creator of the link didn't indicate either
> DL_FLAG_AUTOREMOVE_CONSUMER, or DL_FLAG_AUTOREMOVE_SUPPLIER, they are
> expected to need the link to stay around until it is explicitly
> deleted.
> 
> Therefore adding any of these flags for an existing link where they
> both are unset would be a mistake, because it would effectively cause
> the link to live shorter than expected by the original creator and
> that might lead to correctness issues.
> 
> Thanks!

Thanks Rafael, your last two paragraphs make it really clear what's the
reasoning and why this patch is wrong.

Jonathan, if nothing else comes that I need a re-spin, can you drop this patch
when applying?

I think we can keep the DL_FLAG_AUTOREMOVE_CONSUMER in the device_link_add()
call as it will be ignored if fw_devlinks already created the link but might be
important if the kernel command line fw_devlink is set to 'off'.

Or maybe, as Saravan mentioned in his reply we can just pass DL_FLAG_MANAGED as
having the link around is useful in case we re-probe so we don't need to call
the consumer probe function (just to EPROBE_DEFER) without the supplier being
already there...

- Nuno Sá

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Nuno Sá]

On Fri, 2024-01-26 at 09:04 +0100, Nuno Sá wrote:
> On Thu, 2024-01-25 at 17:57 +0100, Rafael J. Wysocki wrote:
> > On Thu, Jan 25, 2024 at 4:31 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > 
> > > On Thu, 2024-01-25 at 09:14 +0100, Nuno Sá wrote:
> > > > 
> > > > Hi Saravana,
> > > > 
> > > > Thanks for your feedback,
> > > > 
> > > > On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:
> > > > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > > > > 
> > > > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > > > 
> > > > > > If a device_link is previously created (eg: via
> > > > > > fw_devlink_create_devlink()) before the supplier + consumer are both
> > > > > > present and bound to their respective drivers, there's no way to set
> > > > > > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > > > > > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
> > > > > > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > > > > > DL_FLAG_AUTOREMOVE_SUPPLIER is done.
> > > > > 
> > > > > Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> > > > > Especially if fw_devlink already created the link? You are effectively
> > > > > trying to delete the link fw_devlink created if any of your devices
> > > > > unbind.
> > > > > 
> > > > 
> > > > Well, this is still useful in the modules case as the link will be
> > > > relaxed
> > > > after
> > > > all devices are initialized and that will already clear
> > > > AUTOPROBE_CONSUMER
> > > > AFAIU. But, more importantly, if I'm not missing anything, in [1],
> > > > fw_devlinks
> > > > will be dropped after the consumer + supplier are bound which means I
> > > > definitely
> > > > want to create a link between my consumer and supplier.
> > > > 
> > > 
> > > Ok, so to add a bit more on this, there are two cases:
> > > 
> > > 1) Both sup and con are modules and after boot up, the link is relaxed and
> > > thus
> > > turned into a sync_state_only link. That means the link will be deleted
> > > anyways
> > > and AUTOPROBE_CONSUMER is already cleared by the time we try to change the
> > > link.
> > > 
> > > 2) The built-in case where the link is kept as created by fw_devlink and
> > > this
> > > patch effectively clears AUTOPROBE_CONSUMER.
> > > 
> > > Given the above, not sure what's the best option. I can think of 4:
> > > 
> > > 1) Drop this patch and leave things as they are.
> > > DL_FLAG_AUTOREMOVE_CONSUMER
> > > is
> > > pretty much ignored in my call but it will turn the link in a MANAGED one
> > > and
> > > clear SYNC_STATE_ONLY. I could very well just pass 0 in the flags as
> > > DL_FLAG_AUTOREMOVE_CONSUMER is always ignored;
> > > 
> > > 2) Rework this patch so we can still change an existing link to accept
> > > DL_FLAG_AUTOREMOVE_CONSUMER (in the modules case for example).
> > > 
> > > However, instead of clearing AUTOPROBE_CONSUMER, I would add some checks
> > > so
> > > if
> > > flags have one of DL_FLAG_AUTOREMOVE_SUPPLIER or
> > > DL_FLAG_AUTOREMOVE_CONSUMER
> > > and
> > > AUTOPROBE_CONSUMER is already set, we ignore them. In fact, right now, I
> > > think
> > > one could pass DL_FLAG_AUTOREMOVE_SUPPLIER and link->flags ends ups with
> > > AUTOREMOVE_SUPPLIER | AUTOPROBE_CONSUMER which in theory is not allowed...
> > 
> > No, because DL_FLAG_AUTOREMOVE_SUPPLIER is only added to the link
> > flags if DL_FLAG_AUTOREMOVE_CONSUMER is already set in there and the
> > former replaces the latter.
> > 
> 
> Oh yes, I missed that extra if() against the DL_FLAG_AUTOREMOVE_CONSUMER
> flag...
> 
> > Now, DL_FLAG_AUTOREMOVE_CONSUMER cannot be set in the link flags if
> > AUTOPROBE_CONSUMER is set in there.
> > 
> > > 3) Keep it as-is... This one is likely a NACK as I'm getting the feeling
> > > that
> > > clearing stuff that might have been created by fw_devlinks is probably a
> > > no-
> > > go.
> > > 
> > > Let me know your thoughts...
> > 
> > If the original creator of the link didn't indicate either
> > DL_FLAG_AUTOREMOVE_CONSUMER, or DL_FLAG_AUTOREMOVE_SUPPLIER, they are
> > expected to need the link to stay around until it is explicitly
> > deleted.
> > 
> > Therefore adding any of these flags for an existing link where they
> > both are unset would be a mistake, because it would effectively cause
> > the link to live shorter than expected by the original creator and
> > that might lead to correctness issues.
> > 
> > Thanks!
> 
> Thanks Rafael, your last two paragraphs make it really clear what's the
> reasoning and why this patch is wrong.
> 
> Jonathan, if nothing else comes that I need a re-spin, can you drop this patch
> when applying?
> 
> I think we can keep the DL_FLAG_AUTOREMOVE_CONSUMER in the device_link_add()
> call as it will be ignored if fw_devlinks already created the link but might
> be
> important if the kernel command line fw_devlink is set to 'off'.
> 
> Or maybe, as Saravan mentioned in his reply we can just pass DL_FLAG_MANAGED
> as

Forget about this as I just realized DL_FLAG_MANAGED is not a proper flag we can
pass...

- Nuno Sá

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Jonathan Cameron]

On Fri, 26 Jan 2024 15:26:08 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Fri, 2024-01-26 at 09:04 +0100, Nuno Sá wrote:
> > On Thu, 2024-01-25 at 17:57 +0100, Rafael J. Wysocki wrote:  
> > > On Thu, Jan 25, 2024 at 4:31 PM Nuno Sá <noname.nuno@gmail.com> wrote:  
> > > > 
> > > > On Thu, 2024-01-25 at 09:14 +0100, Nuno Sá wrote:  
> > > > > 
> > > > > Hi Saravana,
> > > > > 
> > > > > Thanks for your feedback,
> > > > > 
> > > > > On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:  
> > > > > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:  
> > > > > > > 
> > > > > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > > > > 
> > > > > > > If a device_link is previously created (eg: via
> > > > > > > fw_devlink_create_devlink()) before the supplier + consumer are both
> > > > > > > present and bound to their respective drivers, there's no way to set
> > > > > > > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > > > > > > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
> > > > > > > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > > > > > > DL_FLAG_AUTOREMOVE_SUPPLIER is done.  
> > > > > > 
> > > > > > Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> > > > > > Especially if fw_devlink already created the link? You are effectively
> > > > > > trying to delete the link fw_devlink created if any of your devices
> > > > > > unbind.
> > > > > >   
> > > > > 
> > > > > Well, this is still useful in the modules case as the link will be
> > > > > relaxed
> > > > > after
> > > > > all devices are initialized and that will already clear
> > > > > AUTOPROBE_CONSUMER
> > > > > AFAIU. But, more importantly, if I'm not missing anything, in [1],
> > > > > fw_devlinks
> > > > > will be dropped after the consumer + supplier are bound which means I
> > > > > definitely
> > > > > want to create a link between my consumer and supplier.
> > > > >   
> > > > 
> > > > Ok, so to add a bit more on this, there are two cases:
> > > > 
> > > > 1) Both sup and con are modules and after boot up, the link is relaxed and
> > > > thus
> > > > turned into a sync_state_only link. That means the link will be deleted
> > > > anyways
> > > > and AUTOPROBE_CONSUMER is already cleared by the time we try to change the
> > > > link.
> > > > 
> > > > 2) The built-in case where the link is kept as created by fw_devlink and
> > > > this
> > > > patch effectively clears AUTOPROBE_CONSUMER.
> > > > 
> > > > Given the above, not sure what's the best option. I can think of 4:
> > > > 
> > > > 1) Drop this patch and leave things as they are.
> > > > DL_FLAG_AUTOREMOVE_CONSUMER
> > > > is
> > > > pretty much ignored in my call but it will turn the link in a MANAGED one
> > > > and
> > > > clear SYNC_STATE_ONLY. I could very well just pass 0 in the flags as
> > > > DL_FLAG_AUTOREMOVE_CONSUMER is always ignored;
> > > > 
> > > > 2) Rework this patch so we can still change an existing link to accept
> > > > DL_FLAG_AUTOREMOVE_CONSUMER (in the modules case for example).
> > > > 
> > > > However, instead of clearing AUTOPROBE_CONSUMER, I would add some checks
> > > > so
> > > > if
> > > > flags have one of DL_FLAG_AUTOREMOVE_SUPPLIER or
> > > > DL_FLAG_AUTOREMOVE_CONSUMER
> > > > and
> > > > AUTOPROBE_CONSUMER is already set, we ignore them. In fact, right now, I
> > > > think
> > > > one could pass DL_FLAG_AUTOREMOVE_SUPPLIER and link->flags ends ups with
> > > > AUTOREMOVE_SUPPLIER | AUTOPROBE_CONSUMER which in theory is not allowed...  
> > > 
> > > No, because DL_FLAG_AUTOREMOVE_SUPPLIER is only added to the link
> > > flags if DL_FLAG_AUTOREMOVE_CONSUMER is already set in there and the
> > > former replaces the latter.
> > >   
> > 
> > Oh yes, I missed that extra if() against the DL_FLAG_AUTOREMOVE_CONSUMER
> > flag...
> >   
> > > Now, DL_FLAG_AUTOREMOVE_CONSUMER cannot be set in the link flags if
> > > AUTOPROBE_CONSUMER is set in there.
> > >   
> > > > 3) Keep it as-is... This one is likely a NACK as I'm getting the feeling
> > > > that
> > > > clearing stuff that might have been created by fw_devlinks is probably a
> > > > no-
> > > > go.
> > > > 
> > > > Let me know your thoughts...  
> > > 
> > > If the original creator of the link didn't indicate either
> > > DL_FLAG_AUTOREMOVE_CONSUMER, or DL_FLAG_AUTOREMOVE_SUPPLIER, they are
> > > expected to need the link to stay around until it is explicitly
> > > deleted.
> > > 
> > > Therefore adding any of these flags for an existing link where they
> > > both are unset would be a mistake, because it would effectively cause
> > > the link to live shorter than expected by the original creator and
> > > that might lead to correctness issues.
> > > 
> > > Thanks!  
> > 
> > Thanks Rafael, your last two paragraphs make it really clear what's the
> > reasoning and why this patch is wrong.
> > 
> > Jonathan, if nothing else comes that I need a re-spin, can you drop this patch
> > when applying?
> > 
> > I think we can keep the DL_FLAG_AUTOREMOVE_CONSUMER in the device_link_add()
> > call as it will be ignored if fw_devlinks already created the link but might
> > be
> > important if the kernel command line fw_devlink is set to 'off'.
> > 
> > Or maybe, as Saravan mentioned in his reply we can just pass DL_FLAG_MANAGED
> > as  
> 
> Forget about this as I just realized DL_FLAG_MANAGED is not a proper flag we can
> pass...
> 
> - Nuno Sá
> 

Discussion has gotten too complex - so even if no changes, send a v8 dropping
the patch (assuming that's the end conclusion!)

Jonathan

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Nuno Sá]

On Sat, 2024-01-27 at 15:15 +0000, Jonathan Cameron wrote:
> On Fri, 26 Jan 2024 15:26:08 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Fri, 2024-01-26 at 09:04 +0100, Nuno Sá wrote:
> > > On Thu, 2024-01-25 at 17:57 +0100, Rafael J. Wysocki wrote:  
> > > > On Thu, Jan 25, 2024 at 4:31 PM Nuno Sá <noname.nuno@gmail.com> wrote:  
> > > > > 
> > > > > On Thu, 2024-01-25 at 09:14 +0100, Nuno Sá wrote:  
> > > > > > 
> > > > > > Hi Saravana,
> > > > > > 
> > > > > > Thanks for your feedback,
> > > > > > 
> > > > > > On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:  
> > > > > > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > > > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:  
> > > > > > > > 
> > > > > > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > > > > > 
> > > > > > > > If a device_link is previously created (eg: via
> > > > > > > > fw_devlink_create_devlink()) before the supplier + consumer are
> > > > > > > > both
> > > > > > > > present and bound to their respective drivers, there's no way to
> > > > > > > > set
> > > > > > > > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > > > > > > > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to
> > > > > > > > allow
> > > > > > > > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > > > > > > > DL_FLAG_AUTOREMOVE_SUPPLIER is done.  
> > > > > > > 
> > > > > > > Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> > > > > > > Especially if fw_devlink already created the link? You are
> > > > > > > effectively
> > > > > > > trying to delete the link fw_devlink created if any of your
> > > > > > > devices
> > > > > > > unbind.
> > > > > > >   
> > > > > > 
> > > > > > Well, this is still useful in the modules case as the link will be
> > > > > > relaxed
> > > > > > after
> > > > > > all devices are initialized and that will already clear
> > > > > > AUTOPROBE_CONSUMER
> > > > > > AFAIU. But, more importantly, if I'm not missing anything, in [1],
> > > > > > fw_devlinks
> > > > > > will be dropped after the consumer + supplier are bound which means
> > > > > > I
> > > > > > definitely
> > > > > > want to create a link between my consumer and supplier.
> > > > > >   
> > > > > 
> > > > > Ok, so to add a bit more on this, there are two cases:
> > > > > 
> > > > > 1) Both sup and con are modules and after boot up, the link is relaxed
> > > > > and
> > > > > thus
> > > > > turned into a sync_state_only link. That means the link will be
> > > > > deleted
> > > > > anyways
> > > > > and AUTOPROBE_CONSUMER is already cleared by the time we try to change
> > > > > the
> > > > > link.
> > > > > 
> > > > > 2) The built-in case where the link is kept as created by fw_devlink
> > > > > and
> > > > > this
> > > > > patch effectively clears AUTOPROBE_CONSUMER.
> > > > > 
> > > > > Given the above, not sure what's the best option. I can think of 4:
> > > > > 
> > > > > 1) Drop this patch and leave things as they are.
> > > > > DL_FLAG_AUTOREMOVE_CONSUMER
> > > > > is
> > > > > pretty much ignored in my call but it will turn the link in a MANAGED
> > > > > one
> > > > > and
> > > > > clear SYNC_STATE_ONLY. I could very well just pass 0 in the flags as
> > > > > DL_FLAG_AUTOREMOVE_CONSUMER is always ignored;
> > > > > 
> > > > > 2) Rework this patch so we can still change an existing link to accept
> > > > > DL_FLAG_AUTOREMOVE_CONSUMER (in the modules case for example).
> > > > > 
> > > > > However, instead of clearing AUTOPROBE_CONSUMER, I would add some
> > > > > checks
> > > > > so
> > > > > if
> > > > > flags have one of DL_FLAG_AUTOREMOVE_SUPPLIER or
> > > > > DL_FLAG_AUTOREMOVE_CONSUMER
> > > > > and
> > > > > AUTOPROBE_CONSUMER is already set, we ignore them. In fact, right now,
> > > > > I
> > > > > think
> > > > > one could pass DL_FLAG_AUTOREMOVE_SUPPLIER and link->flags ends ups
> > > > > with
> > > > > AUTOREMOVE_SUPPLIER | AUTOPROBE_CONSUMER which in theory is not
> > > > > allowed...  
> > > > 
> > > > No, because DL_FLAG_AUTOREMOVE_SUPPLIER is only added to the link
> > > > flags if DL_FLAG_AUTOREMOVE_CONSUMER is already set in there and the
> > > > former replaces the latter.
> > > >   
> > > 
> > > Oh yes, I missed that extra if() against the DL_FLAG_AUTOREMOVE_CONSUMER
> > > flag...
> > >   
> > > > Now, DL_FLAG_AUTOREMOVE_CONSUMER cannot be set in the link flags if
> > > > AUTOPROBE_CONSUMER is set in there.
> > > >   
> > > > > 3) Keep it as-is... This one is likely a NACK as I'm getting the
> > > > > feeling
> > > > > that
> > > > > clearing stuff that might have been created by fw_devlinks is probably
> > > > > a
> > > > > no-
> > > > > go.
> > > > > 
> > > > > Let me know your thoughts...  
> > > > 
> > > > If the original creator of the link didn't indicate either
> > > > DL_FLAG_AUTOREMOVE_CONSUMER, or DL_FLAG_AUTOREMOVE_SUPPLIER, they are
> > > > expected to need the link to stay around until it is explicitly
> > > > deleted.
> > > > 
> > > > Therefore adding any of these flags for an existing link where they
> > > > both are unset would be a mistake, because it would effectively cause
> > > > the link to live shorter than expected by the original creator and
> > > > that might lead to correctness issues.
> > > > 
> > > > Thanks!  
> > > 
> > > Thanks Rafael, your last two paragraphs make it really clear what's the
> > > reasoning and why this patch is wrong.
> > > 
> > > Jonathan, if nothing else comes that I need a re-spin, can you drop this
> > > patch
> > > when applying?
> > > 
> > > I think we can keep the DL_FLAG_AUTOREMOVE_CONSUMER in the
> > > device_link_add()
> > > call as it will be ignored if fw_devlinks already created the link but
> > > might
> > > be
> > > important if the kernel command line fw_devlink is set to 'off'.
> > > 
> > > Or maybe, as Saravan mentioned in his reply we can just pass
> > > DL_FLAG_MANAGED
> > > as  
> > 
> > Forget about this as I just realized DL_FLAG_MANAGED is not a proper flag we
> > can
> > pass...
> > 
> > - Nuno Sá
> > 
> 
> Discussion has gotten too complex - so even if no changes, send a v8 dropping
> the patch (assuming that's the end conclusion!)
> 

Dropping the patch is pretty much decided is the right thing to do. The only
thing I'm still thinking is that if I should use AUTOPROBE_CONSUMER (as
fw_devlinks) instead when creating the link. With that flag, any IIO consumer of
the IIO backend will be automatically probed as soon as the backend is probed.
It also has the advantage of keeping the link around (vs AUREMOVE_CONSUMER which
deletes the link when the IIO consumer is gone) so in the re-bind case we can
avoid useless EPROBE_DEFERs. 

It's a nitpicky thing in the end and not really that important. Moreover because
I expect that in 99% of the usecases, fw_devlinks will already create our link
so the flags we pass in our call don't really matter. Note that our explicit
call is still important (as I explained to Saravan in another email) as we based
the design with the assumption that the consumer can never be around without the
backend. And in the case we have modules, we can have the links created by
fw_devlinks removed unless we explicitly call device_link_add() (and that would
mean our previous assumptions are no longer valid).


- Nuno Sá

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Saravana Kannan]

On Mon, Jan 29, 2024 at 12:26 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Sat, 2024-01-27 at 15:15 +0000, Jonathan Cameron wrote:
> > On Fri, 26 Jan 2024 15:26:08 +0100
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >
> > > On Fri, 2024-01-26 at 09:04 +0100, Nuno Sá wrote:
> > > > On Thu, 2024-01-25 at 17:57 +0100, Rafael J. Wysocki wrote:
> > > > > On Thu, Jan 25, 2024 at 4:31 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > > > >
> > > > > > On Thu, 2024-01-25 at 09:14 +0100, Nuno Sá wrote:
> > > > > > >
> > > > > > > Hi Saravana,
> > > > > > >
> > > > > > > Thanks for your feedback,
> > > > > > >
> > > > > > > On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:
> > > > > > > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > > > > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > > > > > > >
> > > > > > > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > > > > > >
> > > > > > > > > If a device_link is previously created (eg: via
> > > > > > > > > fw_devlink_create_devlink()) before the supplier + consumer are
> > > > > > > > > both
> > > > > > > > > present and bound to their respective drivers, there's no way to
> > > > > > > > > set
> > > > > > > > > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > > > > > > > > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to
> > > > > > > > > allow
> > > > > > > > > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > > > > > > > > DL_FLAG_AUTOREMOVE_SUPPLIER is done.
> > > > > > > >
> > > > > > > > Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> > > > > > > > Especially if fw_devlink already created the link? You are
> > > > > > > > effectively
> > > > > > > > trying to delete the link fw_devlink created if any of your
> > > > > > > > devices
> > > > > > > > unbind.
> > > > > > > >
> > > > > > >
> > > > > > > Well, this is still useful in the modules case as the link will be
> > > > > > > relaxed
> > > > > > > after
> > > > > > > all devices are initialized and that will already clear
> > > > > > > AUTOPROBE_CONSUMER
> > > > > > > AFAIU. But, more importantly, if I'm not missing anything, in [1],
> > > > > > > fw_devlinks
> > > > > > > will be dropped after the consumer + supplier are bound which means
> > > > > > > I
> > > > > > > definitely
> > > > > > > want to create a link between my consumer and supplier.
> > > > > > >
> > > > > >
> > > > > > Ok, so to add a bit more on this, there are two cases:
> > > > > >
> > > > > > 1) Both sup and con are modules and after boot up, the link is relaxed
> > > > > > and
> > > > > > thus
> > > > > > turned into a sync_state_only link. That means the link will be
> > > > > > deleted
> > > > > > anyways
> > > > > > and AUTOPROBE_CONSUMER is already cleared by the time we try to change
> > > > > > the
> > > > > > link.
> > > > > >
> > > > > > 2) The built-in case where the link is kept as created by fw_devlink
> > > > > > and
> > > > > > this
> > > > > > patch effectively clears AUTOPROBE_CONSUMER.
> > > > > >
> > > > > > Given the above, not sure what's the best option. I can think of 4:
> > > > > >
> > > > > > 1) Drop this patch and leave things as they are.
> > > > > > DL_FLAG_AUTOREMOVE_CONSUMER
> > > > > > is
> > > > > > pretty much ignored in my call but it will turn the link in a MANAGED
> > > > > > one
> > > > > > and
> > > > > > clear SYNC_STATE_ONLY. I could very well just pass 0 in the flags as
> > > > > > DL_FLAG_AUTOREMOVE_CONSUMER is always ignored;
> > > > > >
> > > > > > 2) Rework this patch so we can still change an existing link to accept
> > > > > > DL_FLAG_AUTOREMOVE_CONSUMER (in the modules case for example).
> > > > > >
> > > > > > However, instead of clearing AUTOPROBE_CONSUMER, I would add some
> > > > > > checks
> > > > > > so
> > > > > > if
> > > > > > flags have one of DL_FLAG_AUTOREMOVE_SUPPLIER or
> > > > > > DL_FLAG_AUTOREMOVE_CONSUMER
> > > > > > and
> > > > > > AUTOPROBE_CONSUMER is already set, we ignore them. In fact, right now,
> > > > > > I
> > > > > > think
> > > > > > one could pass DL_FLAG_AUTOREMOVE_SUPPLIER and link->flags ends ups
> > > > > > with
> > > > > > AUTOREMOVE_SUPPLIER | AUTOPROBE_CONSUMER which in theory is not
> > > > > > allowed...
> > > > >
> > > > > No, because DL_FLAG_AUTOREMOVE_SUPPLIER is only added to the link
> > > > > flags if DL_FLAG_AUTOREMOVE_CONSUMER is already set in there and the
> > > > > former replaces the latter.
> > > > >
> > > >
> > > > Oh yes, I missed that extra if() against the DL_FLAG_AUTOREMOVE_CONSUMER
> > > > flag...
> > > >
> > > > > Now, DL_FLAG_AUTOREMOVE_CONSUMER cannot be set in the link flags if
> > > > > AUTOPROBE_CONSUMER is set in there.
> > > > >
> > > > > > 3) Keep it as-is... This one is likely a NACK as I'm getting the
> > > > > > feeling
> > > > > > that
> > > > > > clearing stuff that might have been created by fw_devlinks is probably
> > > > > > a
> > > > > > no-
> > > > > > go.
> > > > > >
> > > > > > Let me know your thoughts...
> > > > >
> > > > > If the original creator of the link didn't indicate either
> > > > > DL_FLAG_AUTOREMOVE_CONSUMER, or DL_FLAG_AUTOREMOVE_SUPPLIER, they are
> > > > > expected to need the link to stay around until it is explicitly
> > > > > deleted.
> > > > >
> > > > > Therefore adding any of these flags for an existing link where they
> > > > > both are unset would be a mistake, because it would effectively cause
> > > > > the link to live shorter than expected by the original creator and
> > > > > that might lead to correctness issues.
> > > > >
> > > > > Thanks!
> > > >
> > > > Thanks Rafael, your last two paragraphs make it really clear what's the
> > > > reasoning and why this patch is wrong.
> > > >
> > > > Jonathan, if nothing else comes that I need a re-spin, can you drop this
> > > > patch
> > > > when applying?
> > > >
> > > > I think we can keep the DL_FLAG_AUTOREMOVE_CONSUMER in the
> > > > device_link_add()
> > > > call as it will be ignored if fw_devlinks already created the link but
> > > > might
> > > > be
> > > > important if the kernel command line fw_devlink is set to 'off'.
> > > >
> > > > Or maybe, as Saravan mentioned in his reply we can just pass
> > > > DL_FLAG_MANAGED
> > > > as
> > >
> > > Forget about this as I just realized DL_FLAG_MANAGED is not a proper flag we
> > > can
> > > pass...
> > >
> > > - Nuno Sá
> > >
> >
> > Discussion has gotten too complex - so even if no changes, send a v8 dropping
> > the patch (assuming that's the end conclusion!)
> >
>
> Dropping the patch is pretty much decided is the right thing to do. The only
> thing I'm still thinking is that if I should use AUTOPROBE_CONSUMER (as
> fw_devlinks) instead when creating the link. With that flag, any IIO consumer of
> the IIO backend will be automatically probed as soon as the backend is probed.
> It also has the advantage of keeping the link around (vs AUREMOVE_CONSUMER which
> deletes the link when the IIO consumer is gone) so in the re-bind case we can
> avoid useless EPROBE_DEFERs.
>
> It's a nitpicky thing in the end and not really that important. Moreover because
> I expect that in 99% of the usecases, fw_devlinks will already create our link
> so the flags we pass in our call don't really matter. Note that our explicit
> call is still important (as I explained to Saravan in another email) as we based
> the design with the assumption that the consumer can never be around without the
> backend. And in the case we have modules, we can have the links created by
> fw_devlinks removed unless we explicitly call device_link_add() (and that would
> mean our previous assumptions are no longer valid).

I saw your reasoning, but technically there are still gaps in the
forced unbinding of consumers. If the consumer doesn't have a bus or
doesn't have an explicit driver, it won't be force unbound. But this
is all generic issues that need to be resolved at a driver core level.
I'd really prefer drivers/frameworks not duplicating it all over.

How about just checking for fw_devlink=on or better and not probe your
supplier if it's not set? Or not allow unbinding your supplier if
fw_devlink=on or better isn't there?

-Saravana

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Nuno Sá]

On Mon, 2024-01-29 at 14:31 -0800, Saravana Kannan wrote:
> On Mon, Jan 29, 2024 at 12:26 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > On Sat, 2024-01-27 at 15:15 +0000, Jonathan Cameron wrote:
> > > On Fri, 26 Jan 2024 15:26:08 +0100
> > > Nuno Sá <noname.nuno@gmail.com> wrote:
> > > 
> > > > On Fri, 2024-01-26 at 09:04 +0100, Nuno Sá wrote:
> > > > > On Thu, 2024-01-25 at 17:57 +0100, Rafael J. Wysocki wrote:
> > > > > > On Thu, Jan 25, 2024 at 4:31 PM Nuno Sá <noname.nuno@gmail.com>
> > > > > > wrote:
> > > > > > > 
> > > > > > > On Thu, 2024-01-25 at 09:14 +0100, Nuno Sá wrote:
> > > > > > > > 
> > > > > > > > Hi Saravana,
> > > > > > > > 
> > > > > > > > Thanks for your feedback,
> > > > > > > > 
> > > > > > > > On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:
> > > > > > > > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > > > > > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > > > > > > > > 
> > > > > > > > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > > > > > > > 
> > > > > > > > > > If a device_link is previously created (eg: via
> > > > > > > > > > fw_devlink_create_devlink()) before the supplier + consumer
> > > > > > > > > > are
> > > > > > > > > > both
> > > > > > > > > > present and bound to their respective drivers, there's no
> > > > > > > > > > way to
> > > > > > > > > > set
> > > > > > > > > > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > > > > > > > > > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks
> > > > > > > > > > to
> > > > > > > > > > allow
> > > > > > > > > > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > > > > > > > > > DL_FLAG_AUTOREMOVE_SUPPLIER is done.
> > > > > > > > > 
> > > > > > > > > Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> > > > > > > > > Especially if fw_devlink already created the link? You are
> > > > > > > > > effectively
> > > > > > > > > trying to delete the link fw_devlink created if any of your
> > > > > > > > > devices
> > > > > > > > > unbind.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > Well, this is still useful in the modules case as the link will
> > > > > > > > be
> > > > > > > > relaxed
> > > > > > > > after
> > > > > > > > all devices are initialized and that will already clear
> > > > > > > > AUTOPROBE_CONSUMER
> > > > > > > > AFAIU. But, more importantly, if I'm not missing anything, in
> > > > > > > > [1],
> > > > > > > > fw_devlinks
> > > > > > > > will be dropped after the consumer + supplier are bound which
> > > > > > > > means
> > > > > > > > I
> > > > > > > > definitely
> > > > > > > > want to create a link between my consumer and supplier.
> > > > > > > > 
> > > > > > > 
> > > > > > > Ok, so to add a bit more on this, there are two cases:
> > > > > > > 
> > > > > > > 1) Both sup and con are modules and after boot up, the link is
> > > > > > > relaxed
> > > > > > > and
> > > > > > > thus
> > > > > > > turned into a sync_state_only link. That means the link will be
> > > > > > > deleted
> > > > > > > anyways
> > > > > > > and AUTOPROBE_CONSUMER is already cleared by the time we try to
> > > > > > > change
> > > > > > > the
> > > > > > > link.
> > > > > > > 
> > > > > > > 2) The built-in case where the link is kept as created by
> > > > > > > fw_devlink
> > > > > > > and
> > > > > > > this
> > > > > > > patch effectively clears AUTOPROBE_CONSUMER.
> > > > > > > 
> > > > > > > Given the above, not sure what's the best option. I can think of
> > > > > > > 4:
> > > > > > > 
> > > > > > > 1) Drop this patch and leave things as they are.
> > > > > > > DL_FLAG_AUTOREMOVE_CONSUMER
> > > > > > > is
> > > > > > > pretty much ignored in my call but it will turn the link in a
> > > > > > > MANAGED
> > > > > > > one
> > > > > > > and
> > > > > > > clear SYNC_STATE_ONLY. I could very well just pass 0 in the flags
> > > > > > > as
> > > > > > > DL_FLAG_AUTOREMOVE_CONSUMER is always ignored;
> > > > > > > 
> > > > > > > 2) Rework this patch so we can still change an existing link to
> > > > > > > accept
> > > > > > > DL_FLAG_AUTOREMOVE_CONSUMER (in the modules case for example).
> > > > > > > 
> > > > > > > However, instead of clearing AUTOPROBE_CONSUMER, I would add some
> > > > > > > checks
> > > > > > > so
> > > > > > > if
> > > > > > > flags have one of DL_FLAG_AUTOREMOVE_SUPPLIER or
> > > > > > > DL_FLAG_AUTOREMOVE_CONSUMER
> > > > > > > and
> > > > > > > AUTOPROBE_CONSUMER is already set, we ignore them. In fact, right
> > > > > > > now,
> > > > > > > I
> > > > > > > think
> > > > > > > one could pass DL_FLAG_AUTOREMOVE_SUPPLIER and link->flags ends
> > > > > > > ups
> > > > > > > with
> > > > > > > AUTOREMOVE_SUPPLIER | AUTOPROBE_CONSUMER which in theory is not
> > > > > > > allowed...
> > > > > > 
> > > > > > No, because DL_FLAG_AUTOREMOVE_SUPPLIER is only added to the link
> > > > > > flags if DL_FLAG_AUTOREMOVE_CONSUMER is already set in there and the
> > > > > > former replaces the latter.
> > > > > > 
> > > > > 
> > > > > Oh yes, I missed that extra if() against the
> > > > > DL_FLAG_AUTOREMOVE_CONSUMER
> > > > > flag...
> > > > > 
> > > > > > Now, DL_FLAG_AUTOREMOVE_CONSUMER cannot be set in the link flags if
> > > > > > AUTOPROBE_CONSUMER is set in there.
> > > > > > 
> > > > > > > 3) Keep it as-is... This one is likely a NACK as I'm getting the
> > > > > > > feeling
> > > > > > > that
> > > > > > > clearing stuff that might have been created by fw_devlinks is
> > > > > > > probably
> > > > > > > a
> > > > > > > no-
> > > > > > > go.
> > > > > > > 
> > > > > > > Let me know your thoughts...
> > > > > > 
> > > > > > If the original creator of the link didn't indicate either
> > > > > > DL_FLAG_AUTOREMOVE_CONSUMER, or DL_FLAG_AUTOREMOVE_SUPPLIER, they
> > > > > > are
> > > > > > expected to need the link to stay around until it is explicitly
> > > > > > deleted.
> > > > > > 
> > > > > > Therefore adding any of these flags for an existing link where they
> > > > > > both are unset would be a mistake, because it would effectively
> > > > > > cause
> > > > > > the link to live shorter than expected by the original creator and
> > > > > > that might lead to correctness issues.
> > > > > > 
> > > > > > Thanks!
> > > > > 
> > > > > Thanks Rafael, your last two paragraphs make it really clear what's
> > > > > the
> > > > > reasoning and why this patch is wrong.
> > > > > 
> > > > > Jonathan, if nothing else comes that I need a re-spin, can you drop
> > > > > this
> > > > > patch
> > > > > when applying?
> > > > > 
> > > > > I think we can keep the DL_FLAG_AUTOREMOVE_CONSUMER in the
> > > > > device_link_add()
> > > > > call as it will be ignored if fw_devlinks already created the link but
> > > > > might
> > > > > be
> > > > > important if the kernel command line fw_devlink is set to 'off'.
> > > > > 
> > > > > Or maybe, as Saravan mentioned in his reply we can just pass
> > > > > DL_FLAG_MANAGED
> > > > > as
> > > > 
> > > > Forget about this as I just realized DL_FLAG_MANAGED is not a proper
> > > > flag we
> > > > can
> > > > pass...
> > > > 
> > > > - Nuno Sá
> > > > 
> > > 
> > > Discussion has gotten too complex - so even if no changes, send a v8
> > > dropping
> > > the patch (assuming that's the end conclusion!)
> > > 
> > 
> > Dropping the patch is pretty much decided is the right thing to do. The only
> > thing I'm still thinking is that if I should use AUTOPROBE_CONSUMER (as
> > fw_devlinks) instead when creating the link. With that flag, any IIO
> > consumer of
> > the IIO backend will be automatically probed as soon as the backend is
> > probed.
> > It also has the advantage of keeping the link around (vs AUREMOVE_CONSUMER
> > which
> > deletes the link when the IIO consumer is gone) so in the re-bind case we
> > can
> > avoid useless EPROBE_DEFERs.
> > 
> > It's a nitpicky thing in the end and not really that important. Moreover
> > because
> > I expect that in 99% of the usecases, fw_devlinks will already create our
> > link
> > so the flags we pass in our call don't really matter. Note that our explicit
> > call is still important (as I explained to Saravan in another email) as we
> > based
> > the design with the assumption that the consumer can never be around without
> > the
> > backend. And in the case we have modules, we can have the links created by
> > fw_devlinks removed unless we explicitly call device_link_add() (and that
> > would
> > mean our previous assumptions are no longer valid).
> 
> I saw your reasoning, but technically there are still gaps in the
> forced unbinding of consumers. If the consumer doesn't have a bus or
> doesn't have an explicit driver, it won't be force unbound. But this

It will never be the case for us. An IIO frontend (the consumer in here) will
always be on a bus (typically spi or i2c) and have a driver. In fact, the IIO
ABI should be registered in this device.

> is all generic issues that need to be resolved at a driver core level.
> I'd really prefer drivers/frameworks not duplicating it all over.
> 
> How about just checking for fw_devlink=on or better and not probe your
> supplier if it's not set? Or not allow unbinding your supplier if
> fw_devlink=on or better isn't there?
> 

The problem with that is that we still want our IIO converter to work
even if fw_devlink is off (but if having the links is ever an issue - which
shouldn't be - then I should not be using the links already). but most
importantly, we would also need to put similar constrains and check the deferred
timeout parameter otherwise we could not rely on the links in the modules case.

I see your concern about drivers/frameworks doing unnecessary calls but, at
least, in here we do have a reason to rely on it and the simplification code it
gives us, really pays off. You mention we also need some fixes in the core so
maybe when we are in a better state I can drop the explicit call.

Also thinking in your suggestion, what I could do is not allow the IIO backend
to be registered in case fw_links are off or permissive (and hence the supplier
should never probe). But then, we would also need to care about the module case
and I'm not seeing this checks being better than the explicit call, honestly.

To sum it up, I would be fine with the constrain for the built-in case but we
definitely want things to work when compiled as modules. And the checks in there
would be odd (or telling users that they need to add that command line
parameter)

- Nuno Sá

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Saravana Kannan]

On Thu, Jan 25, 2024 at 7:31 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Thu, 2024-01-25 at 09:14 +0100, Nuno Sá wrote:
> >
> > Hi Saravana,
> >
> > Thanks for your feedback,
> >
> > On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:
> > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > >
> > > > From: Nuno Sa <nuno.sa@analog.com>
> > > >
> > > > If a device_link is previously created (eg: via
> > > > fw_devlink_create_devlink()) before the supplier + consumer are both
> > > > present and bound to their respective drivers, there's no way to set
> > > > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > > > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
> > > > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > > > DL_FLAG_AUTOREMOVE_SUPPLIER is done.
> > >
> > > Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> > > Especially if fw_devlink already created the link? You are effectively
> > > trying to delete the link fw_devlink created if any of your devices
> > > unbind.
> > >
> >
> > Well, this is still useful in the modules case as the link will be relaxed
> > after
> > all devices are initialized and that will already clear AUTOPROBE_CONSUMER
> > AFAIU. But, more importantly, if I'm not missing anything, in [1], fw_devlinks
> > will be dropped after the consumer + supplier are bound which means I
> > definitely
> > want to create a link between my consumer and supplier.
> >
>
> Ok, so to add a bit more on this, there are two cases:
>
> 1) Both sup and con are modules and after boot up, the link is relaxed and thus
> turned into a sync_state_only link. That means the link will be deleted anyways
> and AUTOPROBE_CONSUMER is already cleared by the time we try to change the link.
>
> 2) The built-in case where the link is kept as created by fw_devlink and this
> patch effectively clears AUTOPROBE_CONSUMER.

I still don't see a good reason for you to set those flags. And if you
see my other reply, I'm not sure you even need to make changes. Just
use the existing command line arg.

> Given the above, not sure what's the best option. I can think of 4:
>
> 1) Drop this patch and leave things as they are. DL_FLAG_AUTOREMOVE_CONSUMER is
> pretty much ignored in my call but it will turn the link in a MANAGED one and
> clear SYNC_STATE_ONLY. I could very well just pass 0 in the flags as
> DL_FLAG_AUTOREMOVE_CONSUMER is always ignored;
>
> 2) Rework this patch so we can still change an existing link to accept
> DL_FLAG_AUTOREMOVE_CONSUMER (in the modules case for example).
>
> However, instead of clearing AUTOPROBE_CONSUMER, I would add some checks so if
> flags have one of DL_FLAG_AUTOREMOVE_SUPPLIER or DL_FLAG_AUTOREMOVE_CONSUMER and
> AUTOPROBE_CONSUMER is already set, we ignore them. In fact, right now, I think
> one could pass DL_FLAG_AUTOREMOVE_SUPPLIER and link->flags ends ups with
> AUTOREMOVE_SUPPLIER | AUTOPROBE_CONSUMER which in theory is not allowed...

This is all way too complicated and I still see no good reason to use
those flags in whatever case you have in mind.

And Rafael explained why your changes don't make sense. Once a link is
created, any AUTOREMOVE flags should be set.

> 3) Keep it as-is... This one is likely a NACK as I'm getting the feeling that
> clearing stuff that might have been created by fw_devlinks is probably a no-go.
>
> Let me know your thoughts...

Basically drop this patch. I don't see a point in this.

-Saravana

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Nuno Sá]

On Thu, 2024-01-25 at 16:57 -0800, Saravana Kannan wrote:
> On Thu, Jan 25, 2024 at 7:31 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > On Thu, 2024-01-25 at 09:14 +0100, Nuno Sá wrote:
> > > 
> > > Hi Saravana,
> > > 
> > > Thanks for your feedback,
> > > 
> > > On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:
> > > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > > > 
> > > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > > 
> > > > > If a device_link is previously created (eg: via
> > > > > fw_devlink_create_devlink()) before the supplier + consumer are both
> > > > > present and bound to their respective drivers, there's no way to set
> > > > > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > > > > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
> > > > > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > > > > DL_FLAG_AUTOREMOVE_SUPPLIER is done.
> > > > 
> > > > Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> > > > Especially if fw_devlink already created the link? You are effectively
> > > > trying to delete the link fw_devlink created if any of your devices
> > > > unbind.
> > > > 
> > > 
> > > Well, this is still useful in the modules case as the link will be relaxed
> > > after
> > > all devices are initialized and that will already clear AUTOPROBE_CONSUMER
> > > AFAIU. But, more importantly, if I'm not missing anything, in [1],
> > > fw_devlinks
> > > will be dropped after the consumer + supplier are bound which means I
> > > definitely
> > > want to create a link between my consumer and supplier.
> > > 
> > 
> > Ok, so to add a bit more on this, there are two cases:
> > 
> > 1) Both sup and con are modules and after boot up, the link is relaxed and
> > thus
> > turned into a sync_state_only link. That means the link will be deleted
> > anyways
> > and AUTOPROBE_CONSUMER is already cleared by the time we try to change the
> > link.
> > 
> > 2) The built-in case where the link is kept as created by fw_devlink and
> > this
> > patch effectively clears AUTOPROBE_CONSUMER.
> 
> I still don't see a good reason for you to set those flags. And if you
> see my other reply, I'm not sure you even need to make changes. Just
> use the existing command line arg.
> 
> > Given the above, not sure what's the best option. I can think of 4:
> > 
> > 1) Drop this patch and leave things as they are. DL_FLAG_AUTOREMOVE_CONSUMER
> > is
> > pretty much ignored in my call but it will turn the link in a MANAGED one
> > and
> > clear SYNC_STATE_ONLY. I could very well just pass 0 in the flags as
> > DL_FLAG_AUTOREMOVE_CONSUMER is always ignored;
> > 
> > 2) Rework this patch so we can still change an existing link to accept
> > DL_FLAG_AUTOREMOVE_CONSUMER (in the modules case for example).
> > 
> > However, instead of clearing AUTOPROBE_CONSUMER, I would add some checks so
> > if
> > flags have one of DL_FLAG_AUTOREMOVE_SUPPLIER or DL_FLAG_AUTOREMOVE_CONSUMER
> > and
> > AUTOPROBE_CONSUMER is already set, we ignore them. In fact, right now, I
> > think
> > one could pass DL_FLAG_AUTOREMOVE_SUPPLIER and link->flags ends ups with
> > AUTOREMOVE_SUPPLIER | AUTOPROBE_CONSUMER which in theory is not allowed...
> 
> This is all way too complicated and I still see no good reason to use
> those flags in whatever case you have in mind.
> 
> And Rafael explained why your changes don't make sense. Once a link is
> created, any AUTOREMOVE flags should be set.

Yeah, Rafael reply made it clear...

- Nuno Sá

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Saravana Kannan]

On Thu, Jan 25, 2024 at 12:11 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
>
> Hi Saravana,
>
> Thanks for your feedback,
>
> On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:
> > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > >
> > > From: Nuno Sa <nuno.sa@analog.com>
> > >
> > > If a device_link is previously created (eg: via
> > > fw_devlink_create_devlink()) before the supplier + consumer are both
> > > present and bound to their respective drivers, there's no way to set
> > > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
> > > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > > DL_FLAG_AUTOREMOVE_SUPPLIER is done.
> >
> > Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> > Especially if fw_devlink already created the link? You are effectively
> > trying to delete the link fw_devlink created if any of your devices
> > unbind.
> >
>
> Well, this is still useful in the modules case as the link will be relaxed after
> all devices are initialized and that will already clear AUTOPROBE_CONSUMER
> AFAIU. But, more importantly, if I'm not missing anything, in [1], fw_devlinks
> will be dropped after the consumer + supplier are bound which means I definitely
> want to create a link between my consumer and supplier.
>
> FWIW, I was misunderstanding things since I thought DL_FLAG_AUTOREMOVE_CONSUMER
> was needed to make sure the consumer is unbound before the supplier. But for
> that I think I can even pass 0 in the flags as I only need the link to be
> MANAGED. Still, I think having DL_FLAG_AUTOREMOVE_CONSUMER makes sense.

As you noticed, your understanding of DL_FLAG_AUTOREMOVE_CONSUMER is
not correct. There's almost never a good reason to drop a device link.
Even when a device/driver are unbound, we still want future probe
attempts to make use of the dependency info and block a device from
probing if the supplier hasn't probed.

If you don't want the links created by fw_devlink to be relaxed, I
think you should instead set the kernel command line param so that the
kernel doesn't time out and give up on enforcing dependencies.
deferred_probe_timeout=-1

Then you don't have to worry about creating device links.

> Also note that there are more places in the kernel with
> DL_FLAG_AUTOREMOVE_CONSUMER and that flag is likely being ignored in case the
> link already exists.
>
> I'm also clearing DL_FLAG_AUTOPROBE_CONSUMER as from the first check in
> device_link_add(() check I realize that we can't/shouldn't have it together with
> one of AUTOREMOVE_CONSUMER | AUTOREMOVE_SUPPLIER, right? At this point,
> AUTOPROBE_CONSUMER is also likely not that useful anymore as both supplier and
> consumer are up and I guess that's the typical case for subsystems/drivers to
> call device_link_add().
>
> And since I have your attention, it would be nice if you could look in another
> sensible patch [2] that I've resended 3 times already. You're not in CC but I
> see you've done quite some work in dev_links so... Completely unrelated I know
> :)

Regarding [2], I'll try.

-Saravana

>
> [1]: https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L1292
> [2]: https://lore.kernel.org/all/20231213-fix-device-links-overlays-v1-1-f091b213777c@analog.com/#t
>
> - Nuno Sá
> >
>

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Nuno Sá]

On Thu, 2024-01-25 at 16:50 -0800, Saravana Kannan wrote:
> On Thu, Jan 25, 2024 at 12:11 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > 
> > Hi Saravana,
> > 
> > Thanks for your feedback,
> > 
> > On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:
> > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > > 
> > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > 
> > > > If a device_link is previously created (eg: via
> > > > fw_devlink_create_devlink()) before the supplier + consumer are both
> > > > present and bound to their respective drivers, there's no way to set
> > > > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > > > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
> > > > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > > > DL_FLAG_AUTOREMOVE_SUPPLIER is done.
> > > 
> > > Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> > > Especially if fw_devlink already created the link? You are effectively
> > > trying to delete the link fw_devlink created if any of your devices
> > > unbind.
> > > 
> > 
> > Well, this is still useful in the modules case as the link will be relaxed
> > after
> > all devices are initialized and that will already clear AUTOPROBE_CONSUMER
> > AFAIU. But, more importantly, if I'm not missing anything, in [1],
> > fw_devlinks
> > will be dropped after the consumer + supplier are bound which means I
> > definitely
> > want to create a link between my consumer and supplier.
> > 
> > FWIW, I was misunderstanding things since I thought
> > DL_FLAG_AUTOREMOVE_CONSUMER
> > was needed to make sure the consumer is unbound before the supplier. But for
> > that I think I can even pass 0 in the flags as I only need the link to be
> > MANAGED. Still, I think having DL_FLAG_AUTOREMOVE_CONSUMER makes sense.
> 
> As you noticed, your understanding of DL_FLAG_AUTOREMOVE_CONSUMER is
> not correct. There's almost never a good reason to drop a device link.
> Even when a device/driver are unbound, we still want future probe
> attempts to make use of the dependency info and block a device from
> probing if the supplier hasn't probed.
> 

Yeah that makes sense and is making me thinking that I should change my call (in
patch 7 to use the MANAGED flag instead of AUTOREMOVE_CONSUMER). Sure,
AUTOREMOVE_CONSUMER won't matter most cases but if someone disables fw_devlinks
then it matters.

> If you don't want the links created by fw_devlink to be relaxed, I
> think you should instead set the kernel command line param so that the
> kernel doesn't time out and give up on enforcing dependencies.
> deferred_probe_timeout=-1

Good to know... Nope, I don't care much about them being relaxed as I will still
call device_link_add() when the consumer probes and finds the supplier. The only
downside from relaxing is "loosing" AUTOPROBE_CONSUMER but that is not a big
deal.

> 
> Then you don't have to worry about creating device links.
> 
> > Also note that there are more places in the kernel with
> > DL_FLAG_AUTOREMOVE_CONSUMER and that flag is likely being ignored in case
> > the
> > link already exists.
> > 
> > I'm also clearing DL_FLAG_AUTOPROBE_CONSUMER as from the first check in
> > device_link_add(() check I realize that we can't/shouldn't have it together
> > with
> > one of AUTOREMOVE_CONSUMER | AUTOREMOVE_SUPPLIER, right? At this point,
> > AUTOPROBE_CONSUMER is also likely not that useful anymore as both supplier
> > and
> > consumer are up and I guess that's the typical case for subsystems/drivers
> > to
> > call device_link_add().
> > 
> > And since I have your attention, it would be nice if you could look in
> > another
> > sensible patch [2] that I've resended 3 times already. You're not in CC but
> > I
> > see you've done quite some work in dev_links so... Completely unrelated I
> > know
> > :)
> 
> Regarding [2], I'll try.
> 

Thanks! I think it's a valid bug with devlinks and overlays but it's sensible
stuff (so the RFC) so it would be nice to have some review and recommendations
how to solve it... I would definitely like to have it fixed as I see more and
more people (ab)using overlays.

- Nuno Sá

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Nuno Sá]

On Fri, 2024-01-26 at 09:13 +0100, Nuno Sá wrote:
> On Thu, 2024-01-25 at 16:50 -0800, Saravana Kannan wrote:
> > On Thu, Jan 25, 2024 at 12:11 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > 
> > > 
> > > Hi Saravana,
> > > 
> > > Thanks for your feedback,
> > > 
> > > On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:
> > > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > > > 
> > > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > > 
> > > > > If a device_link is previously created (eg: via
> > > > > fw_devlink_create_devlink()) before the supplier + consumer are both
> > > > > present and bound to their respective drivers, there's no way to set
> > > > > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > > > > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
> > > > > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > > > > DL_FLAG_AUTOREMOVE_SUPPLIER is done.
> > > > 
> > > > Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> > > > Especially if fw_devlink already created the link? You are effectively
> > > > trying to delete the link fw_devlink created if any of your devices
> > > > unbind.
> > > > 
> > > 
> > > Well, this is still useful in the modules case as the link will be relaxed
> > > after
> > > all devices are initialized and that will already clear AUTOPROBE_CONSUMER
> > > AFAIU. But, more importantly, if I'm not missing anything, in [1],
> > > fw_devlinks
> > > will be dropped after the consumer + supplier are bound which means I
> > > definitely
> > > want to create a link between my consumer and supplier.
> > > 
> > > FWIW, I was misunderstanding things since I thought
> > > DL_FLAG_AUTOREMOVE_CONSUMER
> > > was needed to make sure the consumer is unbound before the supplier. But
> > > for
> > > that I think I can even pass 0 in the flags as I only need the link to be
> > > MANAGED. Still, I think having DL_FLAG_AUTOREMOVE_CONSUMER makes sense.
> > 
> > As you noticed, your understanding of DL_FLAG_AUTOREMOVE_CONSUMER is
> > not correct. There's almost never a good reason to drop a device link.
> > Even when a device/driver are unbound, we still want future probe
> > attempts to make use of the dependency info and block a device from
> > probing if the supplier hasn't probed.
> > 
> 
> Yeah that makes sense and is making me thinking that I should change my call
> (in
> patch 7 to use the MANAGED flag instead of AUTOREMOVE_CONSUMER). Sure,
> AUTOREMOVE_CONSUMER won't matter most cases but if someone disables
> fw_devlinks
> then it matters.
> 

Yeah, just realized MANAGED is not a valid flag one can pass to
device_link_add() :)

- Nuno Sá
> 

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Saravana Kannan]

On Fri, Jan 26, 2024 at 6:24 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Fri, 2024-01-26 at 09:13 +0100, Nuno Sá wrote:
> > On Thu, 2024-01-25 at 16:50 -0800, Saravana Kannan wrote:
> > > On Thu, Jan 25, 2024 at 12:11 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > >
> > > >
> > > > Hi Saravana,
> > > >
> > > > Thanks for your feedback,
> > > >
> > > > On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:
> > > > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > > > >
> > > > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > > >
> > > > > > If a device_link is previously created (eg: via
> > > > > > fw_devlink_create_devlink()) before the supplier + consumer are both
> > > > > > present and bound to their respective drivers, there's no way to set
> > > > > > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > > > > > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
> > > > > > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > > > > > DL_FLAG_AUTOREMOVE_SUPPLIER is done.
> > > > >
> > > > > Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> > > > > Especially if fw_devlink already created the link? You are effectively
> > > > > trying to delete the link fw_devlink created if any of your devices
> > > > > unbind.
> > > > >
> > > >
> > > > Well, this is still useful in the modules case as the link will be relaxed
> > > > after
> > > > all devices are initialized and that will already clear AUTOPROBE_CONSUMER
> > > > AFAIU. But, more importantly, if I'm not missing anything, in [1],
> > > > fw_devlinks
> > > > will be dropped after the consumer + supplier are bound which means I
> > > > definitely
> > > > want to create a link between my consumer and supplier.
> > > >
> > > > FWIW, I was misunderstanding things since I thought
> > > > DL_FLAG_AUTOREMOVE_CONSUMER
> > > > was needed to make sure the consumer is unbound before the supplier. But
> > > > for
> > > > that I think I can even pass 0 in the flags as I only need the link to be
> > > > MANAGED. Still, I think having DL_FLAG_AUTOREMOVE_CONSUMER makes sense.
> > >
> > > As you noticed, your understanding of DL_FLAG_AUTOREMOVE_CONSUMER is
> > > not correct. There's almost never a good reason to drop a device link.
> > > Even when a device/driver are unbound, we still want future probe
> > > attempts to make use of the dependency info and block a device from
> > > probing if the supplier hasn't probed.
> > >
> >
> > Yeah that makes sense and is making me thinking that I should change my call
> > (in
> > patch 7 to use the MANAGED flag instead of AUTOREMOVE_CONSUMER). Sure,
> > AUTOREMOVE_CONSUMER won't matter most cases but if someone disables
> > fw_devlinks
> > then it matters.

I don't fully understand the patch series, but what exactly are you
gaining by adding device links explicitly. I'd rather that every
driver didn't explicitly create a device link. That's just a lot of
useless code in most cases and we shouldn't have useless code lying
around. If someone does fw_devlink=off and you don't create a device
link explicitly, what's the worst that's going to happen? Useless
deferred probes? fw_devlink is really only there as a debug tool at
this point -- I don't think you need to worry about people setting it
to off/permissive.

> >
>
> Yeah, just realized MANAGED is not a valid flag one can pass to
> device_link_add() :)

If you don't pass the STATELESS flag, a link is assumed to be MANAGED.
So, you can still create managed device links.

-Saravana

Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Nuno Sá]

On Fri, 2024-01-26 at 10:09 -0800, Saravana Kannan wrote:
> On Fri, Jan 26, 2024 at 6:24 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > 
> > On Fri, 2024-01-26 at 09:13 +0100, Nuno Sá wrote:
> > > On Thu, 2024-01-25 at 16:50 -0800, Saravana Kannan wrote:
> > > > On Thu, Jan 25, 2024 at 12:11 AM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > > > 
> > > > > 
> > > > > Hi Saravana,
> > > > > 
> > > > > Thanks for your feedback,
> > > > > 
> > > > > On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:
> > > > > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > > > > <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > > > > > > 
> > > > > > > From: Nuno Sa <nuno.sa@analog.com>
> > > > > > > 
> > > > > > > If a device_link is previously created (eg: via
> > > > > > > fw_devlink_create_devlink()) before the supplier + consumer are both
> > > > > > > present and bound to their respective drivers, there's no way to set
> > > > > > > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > > > > > > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
> > > > > > > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > > > > > > DL_FLAG_AUTOREMOVE_SUPPLIER is done.
> > > > > > 
> > > > > > Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> > > > > > Especially if fw_devlink already created the link? You are effectively
> > > > > > trying to delete the link fw_devlink created if any of your devices
> > > > > > unbind.
> > > > > > 
> > > > > 
> > > > > Well, this is still useful in the modules case as the link will be relaxed
> > > > > after
> > > > > all devices are initialized and that will already clear AUTOPROBE_CONSUMER
> > > > > AFAIU. But, more importantly, if I'm not missing anything, in [1],
> > > > > fw_devlinks
> > > > > will be dropped after the consumer + supplier are bound which means I
> > > > > definitely
> > > > > want to create a link between my consumer and supplier.
> > > > > 
> > > > > FWIW, I was misunderstanding things since I thought
> > > > > DL_FLAG_AUTOREMOVE_CONSUMER
> > > > > was needed to make sure the consumer is unbound before the supplier. But
> > > > > for
> > > > > that I think I can even pass 0 in the flags as I only need the link to be
> > > > > MANAGED. Still, I think having DL_FLAG_AUTOREMOVE_CONSUMER makes sense.
> > > > 
> > > > As you noticed, your understanding of DL_FLAG_AUTOREMOVE_CONSUMER is
> > > > not correct. There's almost never a good reason to drop a device link.
> > > > Even when a device/driver are unbound, we still want future probe
> > > > attempts to make use of the dependency info and block a device from
> > > > probing if the supplier hasn't probed.
> > > > 
> > > 
> > > Yeah that makes sense and is making me thinking that I should change my call
> > > (in
> > > patch 7 to use the MANAGED flag instead of AUTOREMOVE_CONSUMER). Sure,
> > > AUTOREMOVE_CONSUMER won't matter most cases but if someone disables
> > > fw_devlinks
> > > then it matters.
> 
> I don't fully understand the patch series, but what exactly are you
> gaining by adding device links explicitly. I'd rather that every
> driver didn't explicitly create a device link. That's just a lot of
> useless code in most cases and we shouldn't have useless code lying
> around. If someone does fw_devlink=off and you don't create a device
> link explicitly, what's the worst that's going to happen? Useless
> deferred probes? fw_devlink is really only there as a debug tool at
> this point -- I don't think you need to worry about people setting it
> to off/permissive.
> 

There's (at least) a reasoning behind the explicit use of the links. We want to
ensure the creation of a MANAGED link so that we can be assured (i think) that the
consumer device will never be around if the supplier unbinds causing those typical
issues of a supplier going away and consumers trying to access it's code. Now, in the
HW arrangement we're trying to support there's no such thing as optional suppliers.
If the IIO backend is going away, there's no good reason for an IIO frontend to stay
around. And using the guarantee provided by the links made the code way simpler.

Note that in the first versions of the series I was also adding code (together with
dev_links) to make sure we would return error in case the consumer tried to access a
supplier callback and the supplier is no longer around. That meant a mutex for
syncing callbacks plus checking a pointer and having a kref for the backend object.

Jonathan (rightfully) complained that I was adding two ways of guaranteeing the same
thing. Thus, as we don't really have optional suppliers, we went with the explicit
links creation as it makes the code much simpler. If you have any interest, look at
[1] (and let me know if there's any wrong assumption). 

> > > 
> > 
> > Yeah, just realized MANAGED is not a valid flag one can pass to
> > device_link_add() :)
> 
> If you don't pass the STATELESS flag, a link is assumed to be MANAGED.
> So, you can still create managed device links.
> 

Yes, I realized that... Maybe even passing no flag would do the trick.

[1]: https://lore.kernel.org/linux-iio/8085910199d4b653edb61c51fc80a503ee50131d.camel@gmail.com/

- Nuno Sá

[PATCH v7 5/9] of: property: add device link support for io-backends

[Nuno Sa via B4 Relay]

From: Olivier Moysan <olivier.moysan@foss.st.com>

Add support for creating device links out of more DT properties.

Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/of/property.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index bbf0dee2fb9c..44203e42f0e2 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1245,6 +1245,7 @@ DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells")
 DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
 DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
 DEFINE_SIMPLE_PROP(io_channels, "io-channels", "#io-channel-cells")
+DEFINE_SIMPLE_PROP(io_backends, "io-backends", "#io-backend-cells")
 DEFINE_SIMPLE_PROP(interrupt_parent, "interrupt-parent", NULL)
 DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
 DEFINE_SIMPLE_PROP(power_domains, "power-domains", "#power-domain-cells")
@@ -1335,6 +1336,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_iommu_maps, .optional = true, },
 	{ .parse_prop = parse_mboxes, },
 	{ .parse_prop = parse_io_channels, },
+	{ .parse_prop = parse_io_backends, },
 	{ .parse_prop = parse_interrupt_parent, },
 	{ .parse_prop = parse_dmas, .optional = true, },
 	{ .parse_prop = parse_power_domains, },

-- 
2.43.0

[PATCH v7 6/9] iio: buffer-dmaengine: export buffer alloc and free functions

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

Export iio_dmaengine_buffer_free() and iio_dmaengine_buffer_alloc().
This is in preparation of introducing IIO backends support. This will
allow us to allocate a buffer and control it's lifetime from a device
different from the one holding the DMA firmware properties. Effectively,
in this case the struct device holding the firmware information about
the DMA channels is not the same as iio_dev->dev.parent (typical case).

While at it, namespace the buffer-dmaengine exports and update the
current user of these buffers.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/adi-axi-adc.c                      | 1 +
 drivers/iio/buffer/industrialio-buffer-dmaengine.c | 8 +++++---
 include/linux/iio/buffer-dmaengine.h               | 3 +++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index c247ff1541d2..0f21d1d98b9f 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -447,3 +447,4 @@ module_platform_driver(adi_axi_adc_driver);
 MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
 MODULE_DESCRIPTION("Analog Devices Generic AXI ADC IP core driver");
 MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS(IIO_DMAENGINE_BUFFER);
diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
index 45fe7d0d42ee..a18c1da292af 100644
--- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
+++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
@@ -159,7 +159,7 @@ static const struct iio_dev_attr *iio_dmaengine_buffer_attrs[] = {
  * Once done using the buffer iio_dmaengine_buffer_free() should be used to
  * release it.
  */
-static struct iio_buffer *iio_dmaengine_buffer_alloc(struct device *dev,
+struct iio_buffer *iio_dmaengine_buffer_alloc(struct device *dev,
 	const char *channel)
 {
 	struct dmaengine_buffer *dmaengine_buffer;
@@ -210,6 +210,7 @@ static struct iio_buffer *iio_dmaengine_buffer_alloc(struct device *dev,
 	kfree(dmaengine_buffer);
 	return ERR_PTR(ret);
 }
+EXPORT_SYMBOL_NS_GPL(iio_dmaengine_buffer_alloc, IIO_DMAENGINE_BUFFER);
 
 /**
  * iio_dmaengine_buffer_free() - Free dmaengine buffer
@@ -217,7 +218,7 @@ static struct iio_buffer *iio_dmaengine_buffer_alloc(struct device *dev,
  *
  * Frees a buffer previously allocated with iio_dmaengine_buffer_alloc().
  */
-static void iio_dmaengine_buffer_free(struct iio_buffer *buffer)
+void iio_dmaengine_buffer_free(struct iio_buffer *buffer)
 {
 	struct dmaengine_buffer *dmaengine_buffer =
 		iio_buffer_to_dmaengine_buffer(buffer);
@@ -227,6 +228,7 @@ static void iio_dmaengine_buffer_free(struct iio_buffer *buffer)
 
 	iio_buffer_put(buffer);
 }
+EXPORT_SYMBOL_NS_GPL(iio_dmaengine_buffer_free, IIO_DMAENGINE_BUFFER);
 
 static void __devm_iio_dmaengine_buffer_free(void *buffer)
 {
@@ -287,7 +289,7 @@ int devm_iio_dmaengine_buffer_setup(struct device *dev,
 
 	return iio_device_attach_buffer(indio_dev, buffer);
 }
-EXPORT_SYMBOL_GPL(devm_iio_dmaengine_buffer_setup);
+EXPORT_SYMBOL_NS_GPL(devm_iio_dmaengine_buffer_setup, IIO_DMAENGINE_BUFFER);
 
 MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
 MODULE_DESCRIPTION("DMA buffer for the IIO framework");
diff --git a/include/linux/iio/buffer-dmaengine.h b/include/linux/iio/buffer-dmaengine.h
index 5c355be89814..cbb8ba957fad 100644
--- a/include/linux/iio/buffer-dmaengine.h
+++ b/include/linux/iio/buffer-dmaengine.h
@@ -10,6 +10,9 @@
 struct iio_dev;
 struct device;
 
+struct iio_buffer *iio_dmaengine_buffer_alloc(struct device *dev,
+					      const char *channel);
+void iio_dmaengine_buffer_free(struct iio_buffer *buffer);
 int devm_iio_dmaengine_buffer_setup(struct device *dev,
 				    struct iio_dev *indio_dev,
 				    const char *channel);

-- 
2.43.0

[PATCH v7 7/9] iio: add the IIO backend framework

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

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

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 MAINTAINERS                        |   8 +
 drivers/iio/Kconfig                |   9 +
 drivers/iio/Makefile               |   1 +
 drivers/iio/industrialio-backend.c | 412 +++++++++++++++++++++++++++++++++++++
 include/linux/iio/backend.h        |  72 +++++++
 5 files changed, 502 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0d015d85e30c..5f4a101fe508 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10393,6 +10393,14 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	drivers/media/rc/iguanair.c
 
+IIO BACKEND FRAMEWORK
+M:	Nuno Sa <nuno.sa@analog.com>
+R:	Olivier Moysan <olivier.moysan@foss.st.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	drivers/iio/industrialio-backend.c
+F:	include/linux/iio/backend.h
+
 IIO DIGITAL POTENTIOMETER DAC
 M:	Peter Rosin <peda@axentia.se>
 L:	linux-iio@vger.kernel.org
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 52eb46ef84c1..9c351ffc7bed 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -71,6 +71,15 @@ config IIO_TRIGGERED_EVENT
 	help
 	  Provides helper functions for setting up triggered events.
 
+config IIO_BACKEND
+	tristate
+	help
+	  Framework to handle complex IIO aggregate devices. The typical
+	  architecture that can make use of this framework is to have one
+	  device as the frontend device which can be "linked" against one or
+	  multiple backend devices. The framework then makes it easy to get
+	  and control such backend devices.
+
 source "drivers/iio/accel/Kconfig"
 source "drivers/iio/adc/Kconfig"
 source "drivers/iio/addac/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index 9622347a1c1b..0ba0e1521ba4 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_IIO_GTS_HELPER) += industrialio-gts-helper.o
 obj-$(CONFIG_IIO_SW_DEVICE) += industrialio-sw-device.o
 obj-$(CONFIG_IIO_SW_TRIGGER) += industrialio-sw-trigger.o
 obj-$(CONFIG_IIO_TRIGGERED_EVENT) += industrialio-triggered-event.o
+obj-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
 
 obj-y += accel/
 obj-y += adc/
diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
new file mode 100644
index 000000000000..2cc0b4bba0a0
--- /dev/null
+++ b/drivers/iio/industrialio-backend.c
@@ -0,0 +1,412 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * 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 framework interface is pretty simple:
+ *   - Backends should register themselves with @devm_iio_backend_register()
+ *   - Frontend devices should get backends with @devm_iio_backend_get()
+ *
+ * Also to note that the primary target for this framework are converters like
+ * ADC/DACs so @iio_backend_ops will have some operations typical of converter
+ * devices. On top of that, this is "generic" for all IIO which means any kind
+ * of device can make use of the framework. That said, If the @iio_backend_ops
+ * struct begins to grow out of control, we can always refactor things so that
+ * the industrialio-backend.c is only left with the really generic stuff. Then,
+ * we can build on top of it depending on the needs.
+ *
+ * Copyright (C) 2023 Analog Devices Inc.
+ */
+#define pr_fmt(fmt) "iio-backend: " fmt
+
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+
+#include <linux/iio/backend.h>
+
+struct iio_backend {
+	struct list_head entry;
+	const struct iio_backend_ops *ops;
+	struct device *dev;
+	struct module *owner;
+	void *priv;
+};
+
+/*
+ * Helper struct for requesting buffers. This ensures that we have all data
+ * that we need to free the buffer in a device managed action.
+ */
+struct iio_backend_buffer_pair {
+	struct iio_backend *back;
+	struct iio_buffer *buffer;
+};
+
+static LIST_HEAD(iio_back_list);
+static DEFINE_MUTEX(iio_back_lock);
+
+/*
+ * Helper macros to call backend ops. Makes sure the option is supported
+ */
+#define iio_backend_check_op(back, op) ({ \
+	struct iio_backend *____back = back;				\
+	int ____ret = 0;						\
+									\
+	if (!____back->ops->op)						\
+		____ret = -EOPNOTSUPP;					\
+									\
+	____ret;							\
+})
+
+#define iio_backend_op_call(back, op, args...) ({		\
+	struct iio_backend *__back = back;			\
+	int __ret;						\
+								\
+	__ret = iio_backend_check_op(__back, op);		\
+	if (!__ret)						\
+		__ret = __back->ops->op(__back, ##args);	\
+								\
+	__ret;							\
+})
+
+#define iio_backend_ptr_op_call(back, op, args...) ({		\
+	struct iio_backend *__back = back;			\
+	void *ptr_err;						\
+	int __ret;						\
+								\
+	__ret = iio_backend_check_op(__back, op);		\
+	if (__ret)						\
+		ptr_err = ERR_PTR(__ret);			\
+	else							\
+		ptr_err = __back->ops->op(__back, ##args);	\
+								\
+	ptr_err;						\
+})
+
+#define iio_backend_void_op_call(back, op, args...) {		\
+	struct iio_backend *__back = back;			\
+	int __ret;						\
+								\
+	__ret = iio_backend_check_op(__back, op);		\
+	if (!__ret)						\
+		__back->ops->op(__back, ##args);		\
+}
+
+/**
+ * iio_backend_chan_enable - Enable a backend channel
+ * @back:	Backend device
+ * @chan:	Channel number
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan)
+{
+	return iio_backend_op_call(back, chan_enable, chan);
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_chan_enable, IIO_BACKEND);
+
+/**
+ * iio_backend_chan_disable - Disable a backend channel
+ * @back:	Backend device
+ * @chan:	Channel number
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int iio_backend_chan_disable(struct iio_backend *back, unsigned int chan)
+{
+	return iio_backend_op_call(back, chan_disable, chan);
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_chan_disable, IIO_BACKEND);
+
+static void __iio_backend_disable(void *back)
+{
+	iio_backend_void_op_call(back, disable);
+}
+
+/**
+ * devm_iio_backend_enable - Device managed backend enable
+ * @dev:	Consumer device for the backend
+ * @back:	Backend device
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int devm_iio_backend_enable(struct device *dev, struct iio_backend *back)
+{
+	int ret;
+
+	ret = iio_backend_op_call(back, enable);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, __iio_backend_disable, back);
+}
+EXPORT_SYMBOL_NS_GPL(devm_iio_backend_enable, IIO_BACKEND);
+
+/**
+ * iio_backend_data_format_set - Configure the channel data format
+ * @back:	Backend device
+ * @chan:	Channel number
+ * @data:	Data format
+ *
+ * Properly configure a channel with respect to the expected data format. A
+ * @struct iio_backend_data_fmt must be passed with the settings.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int iio_backend_data_format_set(struct iio_backend *back, unsigned int chan,
+				const struct iio_backend_data_fmt *data)
+{
+	if (!data || data->type >= IIO_BACKEND_DATA_TYPE_MAX)
+		return -EINVAL;
+
+	return iio_backend_op_call(back, data_format_set, chan, data);
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_data_format_set, IIO_BACKEND);
+
+static void iio_backend_free_buffer(void *arg)
+{
+	struct iio_backend_buffer_pair *pair = arg;
+
+	iio_backend_void_op_call(pair->back, free_buffer, pair->buffer);
+}
+
+/**
+ * devm_iio_backend_request_buffer - Device managed buffer request
+ * @dev:	Consumer device for the backend
+ * @back:	Backend device
+ * @indio_dev:	IIO device
+ *
+ * Request an IIO buffer from the backend. The type of the buffer (typically
+ * INDIO_BUFFER_HARDWARE) is up to the backend to decide. This is because,
+ * normally, the backend dictates what kind of buffering we can get.
+ *
+ * The backend .free_buffer() hooks is automatically called on @dev detach.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int devm_iio_backend_request_buffer(struct device *dev,
+				    struct iio_backend *back,
+				    struct iio_dev *indio_dev)
+{
+	struct iio_backend_buffer_pair *pair;
+	struct iio_buffer *buffer;
+
+	buffer = iio_backend_ptr_op_call(back, request_buffer, indio_dev);
+	if (IS_ERR(buffer))
+		return PTR_ERR(buffer);
+
+	pair = devm_kzalloc(dev, sizeof(*pair), GFP_KERNEL);
+	if (!pair)
+		return -ENOMEM;
+
+	/* weak reference should be all what we need */
+	pair->back = back;
+	pair->buffer = buffer;
+
+	return devm_add_action_or_reset(dev, iio_backend_free_buffer, pair);
+}
+EXPORT_SYMBOL_NS_GPL(devm_iio_backend_request_buffer, IIO_BACKEND);
+
+static void iio_backend_release(void *arg)
+{
+	struct iio_backend *back = arg;
+
+	module_put(back->owner);
+}
+
+static int __devm_iio_backend_get(struct device *dev, struct iio_backend *back)
+{
+	struct device_link *link;
+	int ret;
+
+	/*
+	 * Make sure the provider cannot be unloaded before the consumer module.
+	 * Note that device_links would still guarantee that nothing is
+	 * accessible (and breaks) but this makes it explicit that the consumer
+	 * module must be also unloaded.
+	 */
+	if (!try_module_get(back->owner)) {
+		pr_err("%s: Cannot get module reference\n", dev_name(dev));
+		return -ENODEV;
+	}
+
+	ret = devm_add_action_or_reset(dev, iio_backend_release, back);
+	if (ret)
+		return ret;
+
+	link = device_link_add(dev, back->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
+	if (!link) {
+		pr_err("%s: Could not link to supplier(%s)\n", dev_name(dev),
+		       dev_name(back->dev));
+		return -EINVAL;
+	}
+
+	pr_debug("%s: Found backend(%s) device\n", dev_name(dev),
+		 dev_name(back->dev));
+
+	return 0;
+}
+
+/**
+ * devm_iio_backend_get - Device managed backend device get
+ * @dev:	Consumer device 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 fwnode_handle *fwnode;
+	struct iio_backend *back;
+	int index = 0, ret;
+
+	if (name) {
+		index = device_property_match_string(dev, "io-backends-names",
+						     name);
+		if (index < 0)
+			return ERR_PTR(index);
+	}
+
+	fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index);
+	if (IS_ERR(fwnode)) {
+		/* not an error if optional */
+		pr_debug("%s: Cannot get Firmware reference\n", dev_name(dev));
+		return ERR_CAST(fwnode);
+	}
+
+	guard(mutex)(&iio_back_lock);
+	list_for_each_entry(back, &iio_back_list, entry) {
+		if (!device_match_fwnode(back->dev, fwnode))
+			continue;
+
+		fwnode_handle_put(fwnode);
+		ret = __devm_iio_backend_get(dev, back);
+		if (ret)
+			return ERR_PTR(ret);
+
+		return back;
+	}
+
+	fwnode_handle_put(fwnode);
+	return ERR_PTR(-EPROBE_DEFER);
+}
+EXPORT_SYMBOL_NS_GPL(devm_iio_backend_get, IIO_BACKEND);
+
+/**
+ * __devm_iio_backend_get_from_fwnode_lookup - Device managed fwnode backend device get
+ * @dev:	Consumer device for the backend
+ * @fwnode:	Firmware node of the backend device
+ *
+ * Search the backend list for a device matching @fwnode.
+ * This API should not be used and it's only present for preventing the first
+ * user of this framework to break it's DT ABI.
+ *
+ * RETURNS:
+ * A backend pointer, negative error pointer otherwise.
+ */
+struct iio_backend *
+__devm_iio_backend_get_from_fwnode_lookup(struct device *dev,
+					  struct fwnode_handle *fwnode)
+{
+	struct iio_backend *back;
+	int ret;
+
+	guard(mutex)(&iio_back_lock);
+	list_for_each_entry(back, &iio_back_list, entry) {
+		if (!device_match_fwnode(back->dev, fwnode))
+			continue;
+
+		ret = __devm_iio_backend_get(dev, back);
+		if (ret)
+			return ERR_PTR(ret);
+
+		return back;
+	}
+
+	return ERR_PTR(-EPROBE_DEFER);
+}
+EXPORT_SYMBOL_NS_GPL(__devm_iio_backend_get_from_fwnode_lookup, IIO_BACKEND);
+
+/**
+ * iio_backend_get_priv - Get driver private data
+ * @back:	Backend device
+ */
+void *iio_backend_get_priv(const struct iio_backend *back)
+{
+	return back->priv;
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_get_priv, IIO_BACKEND);
+
+static void iio_backend_unregister(void *arg)
+{
+	struct iio_backend *back = arg;
+
+	mutex_lock(&iio_back_lock);
+	list_del(&back->entry);
+	mutex_unlock(&iio_back_lock);
+}
+
+/**
+ * devm_iio_backend_register - Device managed backend device register
+ * @dev:	Backend device being registered
+ * @ops:	Backend ops
+ * @priv:	Device private data
+ *
+ * @ops is mandatory. Not providing it results in -EINVAL.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int devm_iio_backend_register(struct device *dev,
+			      const struct iio_backend_ops *ops, void *priv)
+{
+	struct iio_backend *back;
+
+	if (!ops) {
+		pr_err("%s: No backend ops given\n", dev_name(dev));
+		return -EINVAL;
+	}
+
+	/*
+	 * Through device_links, we guarantee that a frontend device cannot be
+	 * bound/exist if the backend driver is not around. Hence, we can bind
+	 * the backend object lifetime with the device being passed since
+	 * removing it will tear the frontend/consumer down.
+	 */
+	back = devm_kzalloc(dev, sizeof(*back), GFP_KERNEL);
+	if (!back)
+		return -ENOMEM;
+
+	back->ops = ops;
+	back->owner = dev->driver->owner;
+	back->dev = dev;
+	back->priv = priv;
+	mutex_lock(&iio_back_lock);
+	list_add(&back->entry, &iio_back_list);
+	mutex_unlock(&iio_back_lock);
+
+	return devm_add_action_or_reset(dev, iio_backend_unregister, back);
+}
+EXPORT_SYMBOL_NS_GPL(devm_iio_backend_register, IIO_BACKEND);
+
+MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>");
+MODULE_DESCRIPTION("Framework to handle complex IIO aggregate devices");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
new file mode 100644
index 000000000000..a6d79381866e
--- /dev/null
+++ b/include/linux/iio/backend.h
@@ -0,0 +1,72 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef _IIO_BACKEND_H_
+#define _IIO_BACKEND_H_
+
+#include <linux/types.h>
+
+struct fwnode_handle;
+struct iio_backend;
+struct device;
+struct iio_dev;
+
+enum iio_backend_data_type {
+	IIO_BACKEND_TWOS_COMPLEMENT,
+	IIO_BACKEND_OFFSET_BINARY,
+	IIO_BACKEND_DATA_TYPE_MAX
+};
+
+/**
+ * struct iio_backend_data_fmt - Backend data format
+ * @type:		Data type.
+ * @sign_extend:	Bool to tell if the data is sign extended.
+ * @enable:		Enable/Disable the data format module. If disabled,
+ *			not formatting will happen.
+ */
+struct iio_backend_data_fmt {
+	enum iio_backend_data_type type;
+	bool sign_extend;
+	bool enable;
+};
+
+/**
+ * struct iio_backend_ops - operations structure for an iio_backend
+ * @enable:		Enable backend.
+ * @disable:		Disable backend.
+ * @chan_enable:	Enable one channel.
+ * @chan_disable:	Disable one channel.
+ * @data_format_set:	Configure the data format for a specific channel.
+ * @request_buffer:	Request an IIO buffer.
+ * @free_buffer:	Free an IIO buffer.
+ **/
+struct iio_backend_ops {
+	int (*enable)(struct iio_backend *back);
+	void (*disable)(struct iio_backend *back);
+	int (*chan_enable)(struct iio_backend *back, unsigned int chan);
+	int (*chan_disable)(struct iio_backend *back, unsigned int chan);
+	int (*data_format_set)(struct iio_backend *back, unsigned int chan,
+			       const struct iio_backend_data_fmt *data);
+	struct iio_buffer *(*request_buffer)(struct iio_backend *back,
+					     struct iio_dev *indio_dev);
+	void (*free_buffer)(struct iio_backend *back,
+			    struct iio_buffer *buffer);
+};
+
+int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan);
+int iio_backend_chan_disable(struct iio_backend *back, unsigned int chan);
+int devm_iio_backend_enable(struct device *dev, struct iio_backend *back);
+int iio_backend_data_format_set(struct iio_backend *back, unsigned int chan,
+				const struct iio_backend_data_fmt *data);
+int devm_iio_backend_request_buffer(struct device *dev,
+				    struct iio_backend *back,
+				    struct iio_dev *indio_dev);
+
+void *iio_backend_get_priv(const struct iio_backend *conv);
+struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name);
+struct iio_backend *
+__devm_iio_backend_get_from_fwnode_lookup(struct device *dev,
+					  struct fwnode_handle *fwnode);
+
+int devm_iio_backend_register(struct device *dev,
+			      const struct iio_backend_ops *ops, void *priv);
+
+#endif

-- 
2.43.0

[PATCH v7 8/9] iio: adc: ad9467: convert to backend framework

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

Convert the driver to use the new IIO backend framework. The device
functionality is expected to be the same (meaning no added or removed
features).

Also note this patch effectively breaks ABI and that's needed so we can
properly support this device and add needed features making use of the
new IIO framework.

Giving the lack of features (and devices supported) in the ad9467 driver
compared with the ADI out of tree version, we don't expect any user of
the upstream driver so no one should notice the ABI breakage. However,
if someone is affected by this, ADI will happily support in transitioning
to the backend framework.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/Kconfig  |   2 +-
 drivers/iio/adc/ad9467.c | 268 +++++++++++++++++++++++++++++++----------------
 2 files changed, 179 insertions(+), 91 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 59ae1d17b50d..61736dc21d2a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -307,7 +307,7 @@ config AD799X
 config AD9467
 	tristate "Analog Devices AD9467 High Speed ADC driver"
 	depends on SPI
-	depends on ADI_AXI_ADC
+	select IIO_BACKEND
 	help
 	  Say yes here to build support for Analog Devices:
 	  * AD9467 16-Bit, 200 MSPS/250 MSPS Analog-to-Digital Converter
diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 6581fce4ba95..7072e7e59200 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -17,13 +17,12 @@
 #include <linux/of.h>
 
 
+#include <linux/iio/backend.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
 #include <linux/clk.h>
 
-#include <linux/iio/adc/adi-axi-adc.h>
-
 /*
  * ADI High-Speed ADC common spi interface registers
  * See Application-Note AN-877:
@@ -102,15 +101,20 @@
 #define AD9467_REG_VREF_MASK		0x0F
 
 struct ad9467_chip_info {
-	struct adi_axi_adc_chip_info	axi_adc_info;
-	unsigned int			default_output_mode;
-	unsigned int			vref_mask;
+	const char		*name;
+	unsigned int		id;
+	const struct		iio_chan_spec *channels;
+	unsigned int		num_channels;
+	const unsigned int	(*scale_table)[2];
+	int			num_scales;
+	unsigned long		max_rate;
+	unsigned int		default_output_mode;
+	unsigned int		vref_mask;
 };
 
-#define to_ad9467_chip_info(_info)	\
-	container_of(_info, struct ad9467_chip_info, axi_adc_info)
-
 struct ad9467_state {
+	const struct ad9467_chip_info	*info;
+	struct iio_backend		*back;
 	struct spi_device		*spi;
 	struct clk			*clk;
 	unsigned int			output_mode;
@@ -151,10 +155,10 @@ static int ad9467_spi_write(struct spi_device *spi, unsigned int reg,
 	return spi_write(spi, buf, ARRAY_SIZE(buf));
 }
 
-static int ad9467_reg_access(struct adi_axi_adc_conv *conv, unsigned int reg,
+static int ad9467_reg_access(struct iio_dev *indio_dev, unsigned int reg,
 			     unsigned int writeval, unsigned int *readval)
 {
-	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	struct ad9467_state *st = iio_priv(indio_dev);
 	struct spi_device *spi = st->spi;
 	int ret;
 
@@ -191,10 +195,10 @@ static const unsigned int ad9467_scale_table[][2] = {
 	{2300, 8}, {2400, 9}, {2500, 10},
 };
 
-static void __ad9467_get_scale(struct adi_axi_adc_conv *conv, int index,
+static void __ad9467_get_scale(struct ad9467_state *st, int index,
 			       unsigned int *val, unsigned int *val2)
 {
-	const struct adi_axi_adc_chip_info *info = conv->chip_info;
+	const struct ad9467_chip_info *info = st->info;
 	const struct iio_chan_spec *chan = &info->channels[0];
 	unsigned int tmp;
 
@@ -229,52 +233,44 @@ static const struct iio_chan_spec ad9467_channels[] = {
 };
 
 static const struct ad9467_chip_info ad9467_chip_tbl = {
-	.axi_adc_info = {
-		.name = "ad9467",
-		.id = CHIPID_AD9467,
-		.max_rate = 250000000UL,
-		.scale_table = ad9467_scale_table,
-		.num_scales = ARRAY_SIZE(ad9467_scale_table),
-		.channels = ad9467_channels,
-		.num_channels = ARRAY_SIZE(ad9467_channels),
-	},
+	.name = "ad9467",
+	.id = CHIPID_AD9467,
+	.max_rate = 250000000UL,
+	.scale_table = ad9467_scale_table,
+	.num_scales = ARRAY_SIZE(ad9467_scale_table),
+	.channels = ad9467_channels,
+	.num_channels = ARRAY_SIZE(ad9467_channels),
 	.default_output_mode = AD9467_DEF_OUTPUT_MODE,
 	.vref_mask = AD9467_REG_VREF_MASK,
 };
 
 static const struct ad9467_chip_info ad9434_chip_tbl = {
-	.axi_adc_info = {
-		.name = "ad9434",
-		.id = CHIPID_AD9434,
-		.max_rate = 500000000UL,
-		.scale_table = ad9434_scale_table,
-		.num_scales = ARRAY_SIZE(ad9434_scale_table),
-		.channels = ad9434_channels,
-		.num_channels = ARRAY_SIZE(ad9434_channels),
-	},
+	.name = "ad9434",
+	.id = CHIPID_AD9434,
+	.max_rate = 500000000UL,
+	.scale_table = ad9434_scale_table,
+	.num_scales = ARRAY_SIZE(ad9434_scale_table),
+	.channels = ad9434_channels,
+	.num_channels = ARRAY_SIZE(ad9434_channels),
 	.default_output_mode = AD9434_DEF_OUTPUT_MODE,
 	.vref_mask = AD9434_REG_VREF_MASK,
 };
 
 static const struct ad9467_chip_info ad9265_chip_tbl = {
-	.axi_adc_info = {
-		.name = "ad9265",
-		.id = CHIPID_AD9265,
-		.max_rate = 125000000UL,
-		.scale_table = ad9265_scale_table,
-		.num_scales = ARRAY_SIZE(ad9265_scale_table),
-		.channels = ad9467_channels,
-		.num_channels = ARRAY_SIZE(ad9467_channels),
-	},
+	.name = "ad9265",
+	.id = CHIPID_AD9265,
+	.max_rate = 125000000UL,
+	.scale_table = ad9265_scale_table,
+	.num_scales = ARRAY_SIZE(ad9265_scale_table),
+	.channels = ad9467_channels,
+	.num_channels = ARRAY_SIZE(ad9467_channels),
 	.default_output_mode = AD9265_DEF_OUTPUT_MODE,
 	.vref_mask = AD9265_REG_VREF_MASK,
 };
 
-static int ad9467_get_scale(struct adi_axi_adc_conv *conv, int *val, int *val2)
+static int ad9467_get_scale(struct ad9467_state *st, int *val, int *val2)
 {
-	const struct adi_axi_adc_chip_info *info = conv->chip_info;
-	const struct ad9467_chip_info *info1 = to_ad9467_chip_info(info);
-	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	const struct ad9467_chip_info *info = st->info;
 	unsigned int i, vref_val;
 	int ret;
 
@@ -282,7 +278,7 @@ static int ad9467_get_scale(struct adi_axi_adc_conv *conv, int *val, int *val2)
 	if (ret < 0)
 		return ret;
 
-	vref_val = ret & info1->vref_mask;
+	vref_val = ret & info->vref_mask;
 
 	for (i = 0; i < info->num_scales; i++) {
 		if (vref_val == info->scale_table[i][1])
@@ -292,15 +288,14 @@ static int ad9467_get_scale(struct adi_axi_adc_conv *conv, int *val, int *val2)
 	if (i == info->num_scales)
 		return -ERANGE;
 
-	__ad9467_get_scale(conv, i, val, val2);
+	__ad9467_get_scale(st, i, val, val2);
 
 	return IIO_VAL_INT_PLUS_MICRO;
 }
 
-static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
+static int ad9467_set_scale(struct ad9467_state *st, int val, int val2)
 {
-	const struct adi_axi_adc_chip_info *info = conv->chip_info;
-	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	const struct ad9467_chip_info *info = st->info;
 	unsigned int scale_val[2];
 	unsigned int i;
 	int ret;
@@ -309,7 +304,7 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
 		return -EINVAL;
 
 	for (i = 0; i < info->num_scales; i++) {
-		__ad9467_get_scale(conv, i, &scale_val[0], &scale_val[1]);
+		__ad9467_get_scale(st, i, &scale_val[0], &scale_val[1]);
 		if (scale_val[0] != val || scale_val[1] != val2)
 			continue;
 
@@ -326,15 +321,15 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
 	return -EINVAL;
 }
 
-static int ad9467_read_raw(struct adi_axi_adc_conv *conv,
+static int ad9467_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val, int *val2, long m)
 {
-	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	struct ad9467_state *st = iio_priv(indio_dev);
 
 	switch (m) {
 	case IIO_CHAN_INFO_SCALE:
-		return ad9467_get_scale(conv, val, val2);
+		return ad9467_get_scale(st, val, val2);
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		*val = clk_get_rate(st->clk);
 
@@ -344,17 +339,17 @@ static int ad9467_read_raw(struct adi_axi_adc_conv *conv,
 	}
 }
 
-static int ad9467_write_raw(struct adi_axi_adc_conv *conv,
+static int ad9467_write_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int val, int val2, long mask)
 {
-	const struct adi_axi_adc_chip_info *info = conv->chip_info;
-	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	struct ad9467_state *st = iio_priv(indio_dev);
+	const struct ad9467_chip_info *info = st->info;
 	long r_clk;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
-		return ad9467_set_scale(conv, val, val2);
+		return ad9467_set_scale(st, val, val2);
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		r_clk = clk_round_rate(st->clk, val);
 		if (r_clk < 0 || r_clk > info->max_rate) {
@@ -369,13 +364,13 @@ static int ad9467_write_raw(struct adi_axi_adc_conv *conv,
 	}
 }
 
-static int ad9467_read_avail(struct adi_axi_adc_conv *conv,
+static int ad9467_read_avail(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     const int **vals, int *type, int *length,
 			     long mask)
 {
-	const struct adi_axi_adc_chip_info *info = conv->chip_info;
-	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	struct ad9467_state *st = iio_priv(indio_dev);
+	const struct ad9467_chip_info *info = st->info;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
@@ -389,6 +384,34 @@ static int ad9467_read_avail(struct adi_axi_adc_conv *conv,
 	}
 }
 
+static int ad9467_update_scan_mode(struct iio_dev *indio_dev,
+				   const unsigned long *scan_mask)
+{
+	struct ad9467_state *st = iio_priv(indio_dev);
+	unsigned int c;
+	int ret;
+
+	for (c = 0; c < st->info->num_channels; c++) {
+		if (test_bit(c, scan_mask))
+			ret = iio_backend_chan_enable(st->back, c);
+		else
+			ret = iio_backend_chan_disable(st->back, c);
+
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct iio_info ad9467_info = {
+	.read_raw = ad9467_read_raw,
+	.write_raw = ad9467_write_raw,
+	.update_scan_mode = ad9467_update_scan_mode,
+	.debugfs_reg_access = ad9467_reg_access,
+	.read_avail = ad9467_read_avail,
+};
+
 static int ad9467_outputmode_set(struct spi_device *spi, unsigned int mode)
 {
 	int ret;
@@ -401,10 +424,9 @@ static int ad9467_outputmode_set(struct spi_device *spi, unsigned int mode)
 				AN877_ADC_TRANSFER_SYNC);
 }
 
-static int ad9467_scale_fill(struct adi_axi_adc_conv *conv)
+static int ad9467_scale_fill(struct ad9467_state *st)
 {
-	const struct adi_axi_adc_chip_info *info = conv->chip_info;
-	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	const struct ad9467_chip_info *info = st->info;
 	unsigned int i, val1, val2;
 
 	st->scales = devm_kmalloc_array(&st->spi->dev, info->num_scales,
@@ -413,7 +435,7 @@ static int ad9467_scale_fill(struct adi_axi_adc_conv *conv)
 		return -ENOMEM;
 
 	for (i = 0; i < info->num_scales; i++) {
-		__ad9467_get_scale(conv, i, &val1, &val2);
+		__ad9467_get_scale(st, i, &val1, &val2);
 		st->scales[i][0] = val1;
 		st->scales[i][1] = val2;
 	}
@@ -421,11 +443,27 @@ static int ad9467_scale_fill(struct adi_axi_adc_conv *conv)
 	return 0;
 }
 
-static int ad9467_preenable_setup(struct adi_axi_adc_conv *conv)
+static int ad9467_setup(struct ad9467_state *st)
 {
-	struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
+	struct iio_backend_data_fmt data = {
+		.sign_extend = true,
+		.enable = true,
+	};
+	unsigned int c, mode;
+	int ret;
+
+	mode = st->info->default_output_mode | AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT;
+	ret = ad9467_outputmode_set(st->spi, mode);
+	if (ret)
+		return ret;
 
-	return ad9467_outputmode_set(st->spi, st->output_mode);
+	for (c = 0; c < st->info->num_channels; c++) {
+		ret = iio_backend_data_format_set(st->back, c, &data);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 static int ad9467_reset(struct device *dev)
@@ -443,25 +481,65 @@ static int ad9467_reset(struct device *dev)
 	return 0;
 }
 
+static int ad9467_iio_backend_get(struct ad9467_state *st)
+{
+	struct device *dev = &st->spi->dev;
+	struct device_node *__back;
+
+	st->back = devm_iio_backend_get(&st->spi->dev, NULL);
+	/* If not found, don't error out as we might have legacy DT property */
+	if (!IS_ERR(st->back))
+		return 0;
+	if (PTR_ERR(st->back) != -ENOENT)
+		return PTR_ERR(st->back);
+
+	/*
+	 * if we don't get the backend using the normal API's, use the legacy
+	 * 'adi,adc-dev' property. So we get all nodes with that property, and
+	 * look for the one pointing at us. Then we directly lookup that fwnode
+	 * on the backend list of registered devices. This is done so we don't
+	 * make io-backends mandatory which would break DT ABI.
+	 */
+	for_each_node_with_property(__back, "adi,adc-dev") {
+		struct device_node *__me;
+
+		__me = of_parse_phandle(__back, "adi,adc-dev", 0);
+		if (!__me)
+			continue;
+
+		if (!device_match_of_node(dev, __me)) {
+			of_node_put(__me);
+			continue;
+		}
+
+		of_node_put(__me);
+		st->back = __devm_iio_backend_get_from_fwnode_lookup(dev,
+								     of_fwnode_handle(__back));
+		of_node_put(__back);
+		return PTR_ERR_OR_ZERO(st->back);
+	}
+
+	return -ENODEV;
+}
+
 static int ad9467_probe(struct spi_device *spi)
 {
-	const struct ad9467_chip_info *info;
-	struct adi_axi_adc_conv *conv;
+	struct iio_dev *indio_dev;
 	struct ad9467_state *st;
 	unsigned int id;
 	int ret;
 
-	info = spi_get_device_match_data(spi);
-	if (!info)
-		return -ENODEV;
-
-	conv = devm_adi_axi_adc_conv_register(&spi->dev, sizeof(*st));
-	if (IS_ERR(conv))
-		return PTR_ERR(conv);
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
 
-	st = adi_axi_adc_conv_priv(conv);
+	st = iio_priv(indio_dev);
 	st->spi = spi;
 
+	st->info = spi_get_device_match_data(spi);
+	if (!st->info)
+		return -ENODEV;
+
 	st->clk = devm_clk_get_enabled(&spi->dev, "adc-clk");
 	if (IS_ERR(st->clk))
 		return PTR_ERR(st->clk);
@@ -475,29 +553,39 @@ static int ad9467_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	conv->chip_info = &info->axi_adc_info;
-
-	ret = ad9467_scale_fill(conv);
+	ret = ad9467_scale_fill(st);
 	if (ret)
 		return ret;
 
 	id = ad9467_spi_read(spi, AN877_ADC_REG_CHIP_ID);
-	if (id != conv->chip_info->id) {
+	if (id != st->info->id) {
 		dev_err(&spi->dev, "Mismatch CHIP_ID, got 0x%X, expected 0x%X\n",
-			id, conv->chip_info->id);
+			id, st->info->id);
 		return -ENODEV;
 	}
 
-	conv->reg_access = ad9467_reg_access;
-	conv->write_raw = ad9467_write_raw;
-	conv->read_raw = ad9467_read_raw;
-	conv->read_avail = ad9467_read_avail;
-	conv->preenable_setup = ad9467_preenable_setup;
+	indio_dev->name = st->info->name;
+	indio_dev->channels = st->info->channels;
+	indio_dev->num_channels = st->info->num_channels;
+	indio_dev->info = &ad9467_info;
 
-	st->output_mode = info->default_output_mode |
-			  AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT;
+	ret = ad9467_iio_backend_get(st);
+	if (ret)
+		return ret;
 
-	return 0;
+	ret = devm_iio_backend_request_buffer(&spi->dev, st->back, indio_dev);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_backend_enable(&spi->dev, st->back);
+	if (ret)
+		return ret;
+
+	ret = ad9467_setup(st);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
 }
 
 static const struct of_device_id ad9467_of_match[] = {
@@ -529,4 +617,4 @@ module_spi_driver(ad9467_driver);
 MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
 MODULE_DESCRIPTION("Analog Devices AD9467 ADC driver");
 MODULE_LICENSE("GPL v2");
-MODULE_IMPORT_NS(IIO_ADI_AXI);
+MODULE_IMPORT_NS(IIO_BACKEND);

-- 
2.43.0

[PATCH v7 9/9] iio: adc: adi-axi-adc: move to backend framework

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

Move to the IIO backend framework. Devices supported by adi-axi-adc now
register themselves as backend devices.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/Kconfig             |   2 +-
 drivers/iio/adc/adi-axi-adc.c       | 382 +++++++++---------------------------
 include/linux/iio/adc/adi-axi-adc.h |  68 -------
 3 files changed, 94 insertions(+), 358 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 61736dc21d2a..977832d797e1 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -325,7 +325,7 @@ config ADI_AXI_ADC
 	select IIO_BUFFER_HW_CONSUMER
 	select IIO_BUFFER_DMAENGINE
 	select REGMAP_MMIO
-	depends on OF
+	select IIO_BACKEND
 	help
 	  Say yes here to build support for Analog Devices Generic
 	  AXI ADC IP core. The IP core is used for interfacing with
diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index 0f21d1d98b9f..05b33b1d1608 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -8,6 +8,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/clk.h>
+#include <linux/err.h>
 #include <linux/io.h>
 #include <linux/delay.h>
 #include <linux/module.h>
@@ -17,13 +18,11 @@
 #include <linux/regmap.h>
 #include <linux/slab.h>
 
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
-#include <linux/iio/buffer.h>
-#include <linux/iio/buffer-dmaengine.h>
-
 #include <linux/fpga/adi-axi-common.h>
-#include <linux/iio/adc/adi-axi-adc.h>
+#include <linux/iio/backend.h>
+#include <linux/iio/buffer-dmaengine.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
 
 /*
  * Register definitions:
@@ -44,6 +43,7 @@
 #define   ADI_AXI_REG_CHAN_CTRL_PN_SEL_OWR	BIT(10)
 #define   ADI_AXI_REG_CHAN_CTRL_IQCOR_EN	BIT(9)
 #define   ADI_AXI_REG_CHAN_CTRL_DCFILT_EN	BIT(8)
+#define   ADI_AXI_REG_CHAN_CTRL_FMT_MASK	GENMASK(6, 4)
 #define   ADI_AXI_REG_CHAN_CTRL_FMT_SIGNEXT	BIT(6)
 #define   ADI_AXI_REG_CHAN_CTRL_FMT_TYPE	BIT(5)
 #define   ADI_AXI_REG_CHAN_CTRL_FMT_EN		BIT(4)
@@ -55,286 +55,100 @@
 	 ADI_AXI_REG_CHAN_CTRL_FMT_EN |		\
 	 ADI_AXI_REG_CHAN_CTRL_ENABLE)
 
-struct adi_axi_adc_core_info {
-	unsigned int				version;
-};
-
 struct adi_axi_adc_state {
-	struct mutex				lock;
-
-	struct adi_axi_adc_client		*client;
 	struct regmap				*regmap;
-};
-
-struct adi_axi_adc_client {
-	struct list_head			entry;
-	struct adi_axi_adc_conv			conv;
-	struct adi_axi_adc_state		*state;
 	struct device				*dev;
-	const struct adi_axi_adc_core_info	*info;
 };
 
-static LIST_HEAD(registered_clients);
-static DEFINE_MUTEX(registered_clients_lock);
-
-static struct adi_axi_adc_client *conv_to_client(struct adi_axi_adc_conv *conv)
-{
-	return container_of(conv, struct adi_axi_adc_client, conv);
-}
-
-void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
-{
-	struct adi_axi_adc_client *cl = conv_to_client(conv);
-
-	return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client),
-				  IIO_DMA_MINALIGN);
-}
-EXPORT_SYMBOL_NS_GPL(adi_axi_adc_conv_priv, IIO_ADI_AXI);
-
-static int adi_axi_adc_config_dma_buffer(struct device *dev,
-					 struct iio_dev *indio_dev)
-{
-	const char *dma_name;
-
-	if (!device_property_present(dev, "dmas"))
-		return 0;
-
-	if (device_property_read_string(dev, "dma-names", &dma_name))
-		dma_name = "rx";
-
-	return devm_iio_dmaengine_buffer_setup(indio_dev->dev.parent,
-					       indio_dev, dma_name);
-}
-
-static int adi_axi_adc_read_raw(struct iio_dev *indio_dev,
-				struct iio_chan_spec const *chan,
-				int *val, int *val2, long mask)
-{
-	struct adi_axi_adc_state *st = iio_priv(indio_dev);
-	struct adi_axi_adc_conv *conv = &st->client->conv;
-
-	if (!conv->read_raw)
-		return -EOPNOTSUPP;
-
-	return conv->read_raw(conv, chan, val, val2, mask);
-}
-
-static int adi_axi_adc_write_raw(struct iio_dev *indio_dev,
-				 struct iio_chan_spec const *chan,
-				 int val, int val2, long mask)
-{
-	struct adi_axi_adc_state *st = iio_priv(indio_dev);
-	struct adi_axi_adc_conv *conv = &st->client->conv;
-
-	if (!conv->write_raw)
-		return -EOPNOTSUPP;
-
-	return conv->write_raw(conv, chan, val, val2, mask);
-}
-
-static int adi_axi_adc_read_avail(struct iio_dev *indio_dev,
-				  struct iio_chan_spec const *chan,
-				  const int **vals, int *type, int *length,
-				  long mask)
+static int axi_adc_enable(struct iio_backend *back)
 {
-	struct adi_axi_adc_state *st = iio_priv(indio_dev);
-	struct adi_axi_adc_conv *conv = &st->client->conv;
-
-	if (!conv->read_avail)
-		return -EOPNOTSUPP;
-
-	return conv->read_avail(conv, chan, vals, type, length, mask);
-}
-
-static int adi_axi_adc_update_scan_mode(struct iio_dev *indio_dev,
-					const unsigned long *scan_mask)
-{
-	struct adi_axi_adc_state *st = iio_priv(indio_dev);
-	struct adi_axi_adc_conv *conv = &st->client->conv;
-	unsigned int i;
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
 	int ret;
 
-	for (i = 0; i < conv->chip_info->num_channels; i++) {
-		if (test_bit(i, scan_mask))
-			ret = regmap_set_bits(st->regmap,
-					      ADI_AXI_REG_CHAN_CTRL(i),
-					      ADI_AXI_REG_CHAN_CTRL_ENABLE);
-		else
-			ret = regmap_clear_bits(st->regmap,
-						ADI_AXI_REG_CHAN_CTRL(i),
-						ADI_AXI_REG_CHAN_CTRL_ENABLE);
-		if (ret)
-			return ret;
-	}
+	ret = regmap_set_bits(st->regmap, ADI_AXI_REG_RSTN,
+			      ADI_AXI_REG_RSTN_MMCM_RSTN);
+	if (ret)
+		return ret;
 
-	return 0;
+	fsleep(10);
+	return regmap_set_bits(st->regmap, ADI_AXI_REG_RSTN,
+			       ADI_AXI_REG_RSTN_RSTN | ADI_AXI_REG_RSTN_MMCM_RSTN);
 }
 
-static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device *dev,
-							  size_t sizeof_priv)
+static void axi_adc_disable(struct iio_backend *back)
 {
-	struct adi_axi_adc_client *cl;
-	size_t alloc_size;
-
-	alloc_size = ALIGN(sizeof(struct adi_axi_adc_client), IIO_DMA_MINALIGN);
-	if (sizeof_priv)
-		alloc_size += ALIGN(sizeof_priv, IIO_DMA_MINALIGN);
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
 
-	cl = kzalloc(alloc_size, GFP_KERNEL);
-	if (!cl)
-		return ERR_PTR(-ENOMEM);
-
-	mutex_lock(&registered_clients_lock);
-
-	cl->dev = get_device(dev);
-
-	list_add_tail(&cl->entry, &registered_clients);
-
-	mutex_unlock(&registered_clients_lock);
-
-	return &cl->conv;
+	regmap_write(st->regmap, ADI_AXI_REG_RSTN, 0);
 }
 
-static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
+static int axi_adc_data_format_set(struct iio_backend *back, unsigned int chan,
+				   const struct iio_backend_data_fmt *data)
 {
-	struct adi_axi_adc_client *cl = conv_to_client(conv);
-
-	mutex_lock(&registered_clients_lock);
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
+	u32 val;
 
-	list_del(&cl->entry);
-	put_device(cl->dev);
+	if (!data->enable)
+		return regmap_clear_bits(st->regmap,
+					 ADI_AXI_REG_CHAN_CTRL(chan),
+					 ADI_AXI_REG_CHAN_CTRL_FMT_EN);
 
-	mutex_unlock(&registered_clients_lock);
+	val = FIELD_PREP(ADI_AXI_REG_CHAN_CTRL_FMT_EN, true);
+	if (data->sign_extend)
+		val |= FIELD_PREP(ADI_AXI_REG_CHAN_CTRL_FMT_SIGNEXT, true);
+	if (data->type == IIO_BACKEND_OFFSET_BINARY)
+		val |= FIELD_PREP(ADI_AXI_REG_CHAN_CTRL_FMT_TYPE, true);
 
-	kfree(cl);
+	return regmap_update_bits(st->regmap, ADI_AXI_REG_CHAN_CTRL(chan),
+				  ADI_AXI_REG_CHAN_CTRL_FMT_MASK, val);
 }
 
-static void devm_adi_axi_adc_conv_release(void *conv)
+static int axi_adc_chan_enable(struct iio_backend *back, unsigned int chan)
 {
-	adi_axi_adc_conv_unregister(conv);
-}
-
-struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
-							size_t sizeof_priv)
-{
-	struct adi_axi_adc_conv *conv;
-	int ret;
-
-	conv = adi_axi_adc_conv_register(dev, sizeof_priv);
-	if (IS_ERR(conv))
-		return conv;
-
-	ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release,
-				       conv);
-	if (ret)
-		return ERR_PTR(ret);
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
 
-	return conv;
+	return regmap_set_bits(st->regmap, ADI_AXI_REG_CHAN_CTRL(chan),
+			       ADI_AXI_REG_CHAN_CTRL_ENABLE);
 }
-EXPORT_SYMBOL_NS_GPL(devm_adi_axi_adc_conv_register, IIO_ADI_AXI);
-
-static const struct iio_info adi_axi_adc_info = {
-	.read_raw = &adi_axi_adc_read_raw,
-	.write_raw = &adi_axi_adc_write_raw,
-	.update_scan_mode = &adi_axi_adc_update_scan_mode,
-	.read_avail = &adi_axi_adc_read_avail,
-};
 
-static const struct adi_axi_adc_core_info adi_axi_adc_10_0_a_info = {
-	.version = ADI_AXI_PCORE_VER(10, 0, 'a'),
-};
-
-static struct adi_axi_adc_client *adi_axi_adc_attach_client(struct device *dev)
+static int axi_adc_chan_disable(struct iio_backend *back, unsigned int chan)
 {
-	const struct adi_axi_adc_core_info *info;
-	struct adi_axi_adc_client *cl;
-	struct device_node *cln;
-
-	info = of_device_get_match_data(dev);
-	if (!info)
-		return ERR_PTR(-ENODEV);
-
-	cln = of_parse_phandle(dev->of_node, "adi,adc-dev", 0);
-	if (!cln) {
-		dev_err(dev, "No 'adi,adc-dev' node defined\n");
-		return ERR_PTR(-ENODEV);
-	}
-
-	mutex_lock(&registered_clients_lock);
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
 
-	list_for_each_entry(cl, &registered_clients, entry) {
-		if (!cl->dev)
-			continue;
-
-		if (cl->dev->of_node != cln)
-			continue;
-
-		if (!try_module_get(cl->dev->driver->owner)) {
-			mutex_unlock(&registered_clients_lock);
-			of_node_put(cln);
-			return ERR_PTR(-ENODEV);
-		}
-
-		get_device(cl->dev);
-		cl->info = info;
-		mutex_unlock(&registered_clients_lock);
-		of_node_put(cln);
-		return cl;
-	}
-
-	mutex_unlock(&registered_clients_lock);
-	of_node_put(cln);
-
-	return ERR_PTR(-EPROBE_DEFER);
+	return regmap_clear_bits(st->regmap, ADI_AXI_REG_CHAN_CTRL(chan),
+				 ADI_AXI_REG_CHAN_CTRL_ENABLE);
 }
 
-static int adi_axi_adc_setup_channels(struct device *dev,
-				      struct adi_axi_adc_state *st)
+static struct iio_buffer *axi_adc_request_buffer(struct iio_backend *back,
+						 struct iio_dev *indio_dev)
 {
-	struct adi_axi_adc_conv *conv = &st->client->conv;
-	int i, ret;
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
+	struct iio_buffer *buffer;
+	const char *dma_name;
+	int ret;
 
-	if (conv->preenable_setup) {
-		ret = conv->preenable_setup(conv);
-		if (ret)
-			return ret;
-	}
+	if (device_property_read_string(st->dev, "dma-names", &dma_name))
+		dma_name = "rx";
 
-	for (i = 0; i < conv->chip_info->num_channels; i++) {
-		ret = regmap_write(st->regmap, ADI_AXI_REG_CHAN_CTRL(i),
-				   ADI_AXI_REG_CHAN_CTRL_DEFAULTS);
-		if (ret)
-			return ret;
+	buffer = iio_dmaengine_buffer_alloc(st->dev, dma_name);
+	if (IS_ERR(buffer)) {
+		dev_err(st->dev, "Could not get DMA buffer, %ld\n",
+			PTR_ERR(buffer));
+		return ERR_CAST(buffer);
 	}
 
-	return 0;
-}
-
-static int axi_adc_reset(struct adi_axi_adc_state *st)
-{
-	int ret;
-
-	ret = regmap_write(st->regmap, ADI_AXI_REG_RSTN, 0);
-	if (ret)
-		return ret;
-
-	mdelay(10);
-	ret = regmap_write(st->regmap, ADI_AXI_REG_RSTN,
-			   ADI_AXI_REG_RSTN_MMCM_RSTN);
+	indio_dev->modes |= INDIO_BUFFER_HARDWARE;
+	ret = iio_device_attach_buffer(indio_dev, buffer);
 	if (ret)
-		return ret;
+		return ERR_PTR(ret);
 
-	mdelay(10);
-	return regmap_write(st->regmap, ADI_AXI_REG_RSTN,
-			    ADI_AXI_REG_RSTN_RSTN | ADI_AXI_REG_RSTN_MMCM_RSTN);
+	return buffer;
 }
 
-static void adi_axi_adc_cleanup(void *data)
+static void axi_adc_free_buffer(struct iio_backend *back,
+				struct iio_buffer *buffer)
 {
-	struct adi_axi_adc_client *cl = data;
-
-	put_device(cl->dev);
-	module_put(cl->dev->driver->owner);
+	iio_dmaengine_buffer_free(buffer);
 }
 
 static const struct regmap_config axi_adc_regmap_config = {
@@ -344,45 +158,46 @@ static const struct regmap_config axi_adc_regmap_config = {
 	.max_register = 0x0800,
 };
 
+static const struct iio_backend_ops adi_axi_adc_generic = {
+	.enable = axi_adc_enable,
+	.disable = axi_adc_disable,
+	.data_format_set = axi_adc_data_format_set,
+	.chan_enable = axi_adc_chan_enable,
+	.chan_disable = axi_adc_chan_disable,
+	.request_buffer = axi_adc_request_buffer,
+	.free_buffer = axi_adc_free_buffer,
+};
+
 static int adi_axi_adc_probe(struct platform_device *pdev)
 {
-	struct adi_axi_adc_conv *conv;
-	struct iio_dev *indio_dev;
-	struct adi_axi_adc_client *cl;
 	struct adi_axi_adc_state *st;
 	void __iomem *base;
-	unsigned int ver;
+	unsigned int ver, *expected_ver;
 	int ret;
 
-	cl = adi_axi_adc_attach_client(&pdev->dev);
-	if (IS_ERR(cl))
-		return PTR_ERR(cl);
-
-	ret = devm_add_action_or_reset(&pdev->dev, adi_axi_adc_cleanup, cl);
-	if (ret)
-		return ret;
-
-	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*st));
-	if (indio_dev == NULL)
+	st = devm_kzalloc(&pdev->dev, sizeof(*st), GFP_KERNEL);
+	if (!st)
 		return -ENOMEM;
 
-	st = iio_priv(indio_dev);
-	st->client = cl;
-	cl->state = st;
-	mutex_init(&st->lock);
-
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
+	st->dev = &pdev->dev;
 	st->regmap = devm_regmap_init_mmio(&pdev->dev, base,
 					   &axi_adc_regmap_config);
 	if (IS_ERR(st->regmap))
 		return PTR_ERR(st->regmap);
 
-	conv = &st->client->conv;
+	expected_ver = (unsigned int *)device_get_match_data(&pdev->dev);
+	if (!expected_ver)
+		return -ENODEV;
 
-	ret = axi_adc_reset(st);
+	/*
+	 * Force disable the core. Up to the frontend to enable us. And we can
+	 * still read/write registers...
+	 */
+	ret = regmap_write(st->regmap, ADI_AXI_REG_RSTN, 0);
 	if (ret)
 		return ret;
 
@@ -390,37 +205,23 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	if (cl->info->version > ver) {
+	if (*expected_ver > ver) {
 		dev_err(&pdev->dev,
-			"IP core version is too old. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n",
-			ADI_AXI_PCORE_VER_MAJOR(cl->info->version),
-			ADI_AXI_PCORE_VER_MINOR(cl->info->version),
-			ADI_AXI_PCORE_VER_PATCH(cl->info->version),
+			"IP core version is too old. Expected %d.%.2d.%d, Reported %d.%.2d.%c\n",
+			ADI_AXI_PCORE_VER_MAJOR(*expected_ver),
+			ADI_AXI_PCORE_VER_MINOR(*expected_ver),
+			ADI_AXI_PCORE_VER_PATCH(*expected_ver),
 			ADI_AXI_PCORE_VER_MAJOR(ver),
 			ADI_AXI_PCORE_VER_MINOR(ver),
 			ADI_AXI_PCORE_VER_PATCH(ver));
 		return -ENODEV;
 	}
 
-	indio_dev->info = &adi_axi_adc_info;
-	indio_dev->name = "adi-axi-adc";
-	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->num_channels = conv->chip_info->num_channels;
-	indio_dev->channels = conv->chip_info->channels;
-
-	ret = adi_axi_adc_config_dma_buffer(&pdev->dev, indio_dev);
+	ret = devm_iio_backend_register(&pdev->dev, &adi_axi_adc_generic, st);
 	if (ret)
 		return ret;
 
-	ret = adi_axi_adc_setup_channels(&pdev->dev, st);
-	if (ret)
-		return ret;
-
-	ret = devm_iio_device_register(&pdev->dev, indio_dev);
-	if (ret)
-		return ret;
-
-	dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%c) probed\n",
+	dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%d) probed\n",
 		 ADI_AXI_PCORE_VER_MAJOR(ver),
 		 ADI_AXI_PCORE_VER_MINOR(ver),
 		 ADI_AXI_PCORE_VER_PATCH(ver));
@@ -428,6 +229,8 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static unsigned int adi_axi_adc_10_0_a_info = ADI_AXI_PCORE_VER(10, 0, 'a');
+
 /* Match table for of_platform binding */
 static const struct of_device_id adi_axi_adc_of_match[] = {
 	{ .compatible = "adi,axi-adc-10.0.a", .data = &adi_axi_adc_10_0_a_info },
@@ -448,3 +251,4 @@ MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
 MODULE_DESCRIPTION("Analog Devices Generic AXI ADC IP core driver");
 MODULE_LICENSE("GPL v2");
 MODULE_IMPORT_NS(IIO_DMAENGINE_BUFFER);
+MODULE_IMPORT_NS(IIO_BACKEND);
diff --git a/include/linux/iio/adc/adi-axi-adc.h b/include/linux/iio/adc/adi-axi-adc.h
deleted file mode 100644
index b7904992d561..000000000000
--- a/include/linux/iio/adc/adi-axi-adc.h
+++ /dev/null
@@ -1,68 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * Analog Devices Generic AXI ADC IP core driver/library
- * Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
- *
- * Copyright 2012-2020 Analog Devices Inc.
- */
-#ifndef __ADI_AXI_ADC_H__
-#define __ADI_AXI_ADC_H__
-
-struct device;
-struct iio_chan_spec;
-
-/**
- * struct adi_axi_adc_chip_info - Chip specific information
- * @name		Chip name
- * @id			Chip ID (usually product ID)
- * @channels		Channel specifications of type @struct iio_chan_spec
- * @num_channels	Number of @channels
- * @scale_table		Supported scales by the chip; tuples of 2 ints
- * @num_scales		Number of scales in the table
- * @max_rate		Maximum sampling rate supported by the device
- */
-struct adi_axi_adc_chip_info {
-	const char			*name;
-	unsigned int			id;
-
-	const struct iio_chan_spec	*channels;
-	unsigned int			num_channels;
-
-	const unsigned int		(*scale_table)[2];
-	int				num_scales;
-
-	unsigned long			max_rate;
-};
-
-/**
- * struct adi_axi_adc_conv - data of the ADC attached to the AXI ADC
- * @chip_info		chip info details for the client ADC
- * @preenable_setup	op to run in the client before enabling the AXI ADC
- * @reg_access		IIO debugfs_reg_access hook for the client ADC
- * @read_raw		IIO read_raw hook for the client ADC
- * @write_raw		IIO write_raw hook for the client ADC
- * @read_avail		IIO read_avail hook for the client ADC
- */
-struct adi_axi_adc_conv {
-	const struct adi_axi_adc_chip_info		*chip_info;
-
-	int (*preenable_setup)(struct adi_axi_adc_conv *conv);
-	int (*reg_access)(struct adi_axi_adc_conv *conv, unsigned int reg,
-			  unsigned int writeval, unsigned int *readval);
-	int (*read_raw)(struct adi_axi_adc_conv *conv,
-			struct iio_chan_spec const *chan,
-			int *val, int *val2, long mask);
-	int (*write_raw)(struct adi_axi_adc_conv *conv,
-			 struct iio_chan_spec const *chan,
-			 int val, int val2, long mask);
-	int (*read_avail)(struct adi_axi_adc_conv *conv,
-			  struct iio_chan_spec const *chan,
-			  const int **val, int *type, int *length, long mask);
-};
-
-struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
-							size_t sizeof_priv);
-
-void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv);
-
-#endif

-- 
2.43.0

Re: [PATCH v7 9/9] iio: adc: adi-axi-adc: move to backend framework

[Jonathan Cameron]

On Tue, 23 Jan 2024 16:14:30 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:

> From: Nuno Sa <nuno.sa@analog.com>
> 
> Move to the IIO backend framework. Devices supported by adi-axi-adc now
> register themselves as backend devices.
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
I'm still not getting the %d vs %c change..

Otherwise LGTM

> -	if (cl->info->version > ver) {
> +	if (*expected_ver > ver) {
>  		dev_err(&pdev->dev,
> -			"IP core version is too old. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n",
> -			ADI_AXI_PCORE_VER_MAJOR(cl->info->version),
> -			ADI_AXI_PCORE_VER_MINOR(cl->info->version),
> -			ADI_AXI_PCORE_VER_PATCH(cl->info->version),
> +			"IP core version is too old. Expected %d.%.2d.%d, Reported %d.%.2d.%c\n",

If it's a valid change fine, but then I'd expect both %c to change.
I'd also expect it to be in a separate patch with an explanation of why.

> +			ADI_AXI_PCORE_VER_MAJOR(*expected_ver),
> +			ADI_AXI_PCORE_VER_MINOR(*expected_ver),
> +			ADI_AXI_PCORE_VER_PATCH(*expected_ver),
>  			ADI_AXI_PCORE_VER_MAJOR(ver),
>  			ADI_AXI_PCORE_VER_MINOR(ver),
>  			ADI_AXI_PCORE_VER_PATCH(ver));
>  		return -ENODEV;
>  	}
>  
> -	indio_dev->info = &adi_axi_adc_info;
> -	indio_dev->name = "adi-axi-adc";
> -	indio_dev->modes = INDIO_DIRECT_MODE;
> -	indio_dev->num_channels = conv->chip_info->num_channels;
> -	indio_dev->channels = conv->chip_info->channels;
> -
> -	ret = adi_axi_adc_config_dma_buffer(&pdev->dev, indio_dev);
> +	ret = devm_iio_backend_register(&pdev->dev, &adi_axi_adc_generic, st);
>  	if (ret)
>  		return ret;
>  
> -	ret = adi_axi_adc_setup_channels(&pdev->dev, st);
> -	if (ret)
> -		return ret;
> -
> -	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> -	if (ret)
> -		return ret;
> -
> -	dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%c) probed\n",
> +	dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%d) probed\n",

This should also be in that separate patch fixing up this formatting.

>  		 ADI_AXI_PCORE_VER_MAJOR(ver),
>  		 ADI_AXI_PCORE_VER_MINOR(ver),
>  		 ADI_AXI_PCORE_VER_PATCH(ver));
> @@ -428,6 +229,8 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
>  	return 0;
>  }

Re: [PATCH v7 9/9] iio: adc: adi-axi-adc: move to backend framework

[David Lechner]

On Sat, Jan 27, 2024 at 9:21 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 23 Jan 2024 16:14:30 +0100
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> > From: Nuno Sa <nuno.sa@analog.com>
> >
> > Move to the IIO backend framework. Devices supported by adi-axi-adc now
> > register themselves as backend devices.
> >
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> I'm still not getting the %d vs %c change..
>
> Otherwise LGTM
>
> > -     if (cl->info->version > ver) {
> > +     if (*expected_ver > ver) {
> >               dev_err(&pdev->dev,
> > -                     "IP core version is too old. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n",
> > -                     ADI_AXI_PCORE_VER_MAJOR(cl->info->version),
> > -                     ADI_AXI_PCORE_VER_MINOR(cl->info->version),
> > -                     ADI_AXI_PCORE_VER_PATCH(cl->info->version),
> > +                     "IP core version is too old. Expected %d.%.2d.%d, Reported %d.%.2d.%c\n",
>
> If it's a valid change fine, but then I'd expect both %c to change.
> I'd also expect it to be in a separate patch with an explanation of why.

I was noticing this same pattern in other "AXI" drivers. I think the
reason for the %c is to match the version format in the devicetree
compatible string which looks like "1.00.a". So to fix this we should
probably keep %c and change the value line to
`ADI_AXI_PCORE_VER_PATCH(cl->info->version) + 'a'` to convert it to
the appropriate ascii value.

(But agree that this should be done in a separate patch/)

>
> > +                     ADI_AXI_PCORE_VER_MAJOR(*expected_ver),
> > +                     ADI_AXI_PCORE_VER_MINOR(*expected_ver),
> > +                     ADI_AXI_PCORE_VER_PATCH(*expected_ver),
> >                       ADI_AXI_PCORE_VER_MAJOR(ver),
> >                       ADI_AXI_PCORE_VER_MINOR(ver),
> >                       ADI_AXI_PCORE_VER_PATCH(ver));
> >               return -ENODEV;
> >       }
> >
> > -     indio_dev->info = &adi_axi_adc_info;
> > -     indio_dev->name = "adi-axi-adc";
> > -     indio_dev->modes = INDIO_DIRECT_MODE;
> > -     indio_dev->num_channels = conv->chip_info->num_channels;
> > -     indio_dev->channels = conv->chip_info->channels;
> > -
> > -     ret = adi_axi_adc_config_dma_buffer(&pdev->dev, indio_dev);
> > +     ret = devm_iio_backend_register(&pdev->dev, &adi_axi_adc_generic, st);
> >       if (ret)
> >               return ret;
> >
> > -     ret = adi_axi_adc_setup_channels(&pdev->dev, st);
> > -     if (ret)
> > -             return ret;
> > -
> > -     ret = devm_iio_device_register(&pdev->dev, indio_dev);
> > -     if (ret)
> > -             return ret;
> > -
> > -     dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%c) probed\n",
> > +     dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%d) probed\n",
>
> This should also be in that separate patch fixing up this formatting.
>
> >                ADI_AXI_PCORE_VER_MAJOR(ver),
> >                ADI_AXI_PCORE_VER_MINOR(ver),
> >                ADI_AXI_PCORE_VER_PATCH(ver));
> > @@ -428,6 +229,8 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
> >       return 0;
> >  }
>
>

Re: [PATCH v7 9/9] iio: adc: adi-axi-adc: move to backend framework

[Nuno Sá]

On Sun, 2024-01-28 at 15:27 -0600, David Lechner wrote:
> On Sat, Jan 27, 2024 at 9:21 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > 
> > On Tue, 23 Jan 2024 16:14:30 +0100
> > Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > 
> > > From: Nuno Sa <nuno.sa@analog.com>
> > > 
> > > Move to the IIO backend framework. Devices supported by adi-axi-adc now
> > > register themselves as backend devices.
> > > 
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > I'm still not getting the %d vs %c change..
> > 
> > Otherwise LGTM
> > 
> > > -     if (cl->info->version > ver) {
> > > +     if (*expected_ver > ver) {
> > >               dev_err(&pdev->dev,
> > > -                     "IP core version is too old. Expected %d.%.2d.%c,
> > > Reported %d.%.2d.%c\n",
> > > -                     ADI_AXI_PCORE_VER_MAJOR(cl->info->version),
> > > -                     ADI_AXI_PCORE_VER_MINOR(cl->info->version),
> > > -                     ADI_AXI_PCORE_VER_PATCH(cl->info->version),
> > > +                     "IP core version is too old. Expected %d.%.2d.%d,
> > > Reported %d.%.2d.%c\n",
> > 
> > If it's a valid change fine, but then I'd expect both %c to change.
> > I'd also expect it to be in a separate patch with an explanation of why.
> 
> I was noticing this same pattern in other "AXI" drivers. I think the
> reason for the %c is to match the version format in the devicetree
> compatible string which looks like "1.00.a". So to fix this we should
> probably keep %c and change the value line to
> `ADI_AXI_PCORE_VER_PATCH(cl->info->version) + 'a'` to convert it to
> the appropriate ascii value.
> 
> (But agree that this should be done in a separate patch/)

Yeah, I'm also realizing that with the axi-dac support I'm working on. In there,
the %c actually does it's job and prints the last character nicely. I will
double check but in this series it might just be that I have a subtle issue in
my hdl design. 

So the plan for now is to just drop these changes and keep %c.

- Nuno Sá