[v3,04/10] dt-bindings: iio: dac: ad3552r: add io-backend support

Commit Message

Angelo Dureghello Sept. 19, 2024, 9:20 a.m. UTC

From: Angelo Dureghello <[e-mail]>

There is a version AXI DAC IP block (for FPGAs) that provides
a physical bus for AD3552R and similar chips, and acts as
an SPI controller.

For this case, the binding is modified to include some
additional properties.

Signed-off-by: Angelo Dureghello <[e-mail]>
---
 .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)

Comments

Krzysztof Kozlowski Sept. 22, 2024, 9:02 p.m. UTC | #1

On Thu, Sep 19, 2024 at 11:20:00AM +0200, Angelo Dureghello wrote:
> From: Angelo Dureghello <[e-mail]>
> 
> There is a version AXI DAC IP block (for FPGAs) that provides
> a physical bus for AD3552R and similar chips, and acts as
> an SPI controller.
> 
> For this case, the binding is modified to include some
> additional properties.
> 
> Signed-off-by: Angelo Dureghello <[e-mail]>
> ---
>  .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42 ++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> index 41fe00034742..aca4a41c2633 100644
> --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> @@ -60,6 +60,18 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      enum: [0, 1, 2, 3]
>  
> +  io-backends:
> +    description: The iio backend reference.
> +      An example backend can be found at
> +        https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> +    maxItems: 1
> +
> +  adi,synchronous-mode:
> +    description: Enable waiting for external synchronization signal.
> +      Some AXI IP configuration can implement a dual-IP layout, with internal
> +      wirings for streaming synchronization.
> +    type: boolean
> +
>    '#address-cells':
>      const: 1
>  
> @@ -128,6 +140,7 @@ patternProperties:
>            - custom-output-range-config
>  
>  allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>    - if:
>        properties:
>          compatible:
> @@ -238,4 +251,33 @@ examples:
>              };
>          };
>      };
> +
> +  - |
> +    axi_dac: spi@44a70000 {
> +        compatible = "adi,axi-ad3552r";

That is either redundant or entire example should go to the parent node,
if this device is fixed child of complex device (IOW, adi,ad3552r cannot
be used outside of adi,axi-ad3552r).


> +        reg = <0x44a70000 0x1000>;
> +        dmas = <&dac_tx_dma 0>;
> +        dma-names = "tx";
> +        #io-backend-cells = <0>;
> +        clocks = <&ref_clk>;
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        dac@0 {
> +            compatible = "adi,ad3552r";
> +            reg = <0>;
> +            reset-gpios = <&gpio0 92 0>;

Use standard defines for GPIO flags.

> +            io-backends = <&axi_dac>;

Why do you need to point to the parent? How much coupled are these
devices? Child pointing to parent is not usually expected, because
that's obvious.

Best regards,
Krzysztof

Angelo Dureghello Sept. 23, 2024, 3:50 p.m. UTC | #2

Hi Krzysztof,

On 22/09/24 23:02, Krzysztof Kozlowski wrote:
> On Thu, Sep 19, 2024 at 11:20:00AM +0200, Angelo Dureghello wrote:
>> From: Angelo Dureghello <[e-mail]>
>>
>> There is a version AXI DAC IP block (for FPGAs) that provides
>> a physical bus for AD3552R and similar chips, and acts as
>> an SPI controller.
>>
>> For this case, the binding is modified to include some
>> additional properties.
>>
>> Signed-off-by: Angelo Dureghello <[e-mail]>
>> ---
>>   .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42 ++++++++++++++++++++++
>>   1 file changed, 42 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>> index 41fe00034742..aca4a41c2633 100644
>> --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>> @@ -60,6 +60,18 @@ properties:
>>       $ref: /schemas/types.yaml#/definitions/uint32
>>       enum: [0, 1, 2, 3]
>>   
>> +  io-backends:
>> +    description: The iio backend reference.
>> +      An example backend can be found at
>> +        https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
>> +    maxItems: 1
>> +
>> +  adi,synchronous-mode:
>> +    description: Enable waiting for external synchronization signal.
>> +      Some AXI IP configuration can implement a dual-IP layout, with internal
>> +      wirings for streaming synchronization.
>> +    type: boolean
>> +
>>     '#address-cells':
>>       const: 1
>>   
>> @@ -128,6 +140,7 @@ patternProperties:
>>             - custom-output-range-config
>>   
>>   allOf:
>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>     - if:
>>         properties:
>>           compatible:
>> @@ -238,4 +251,33 @@ examples:
>>               };
>>           };
>>       };
>> +
>> +  - |
>> +    axi_dac: spi@44a70000 {
>> +        compatible = "adi,axi-ad3552r";
> That is either redundant or entire example should go to the parent node,
> if this device is fixed child of complex device (IOW, adi,ad3552r cannot
> be used outside of adi,axi-ad3552r).

ad3552r can still be used by a generic "classic" spi
controller (SCLK/CS/MISO) but at a slower samplerate, fpga
controller only (axi-ad3552r) can reach 33MUPS.

>
>> +        reg = <0x44a70000 0x1000>;
>> +        dmas = <&dac_tx_dma 0>;
>> +        dma-names = "tx";
>> +        #io-backend-cells = <0>;
>> +        clocks = <&ref_clk>;
>> +
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        dac@0 {
>> +            compatible = "adi,ad3552r";
>> +            reg = <0>;
>> +            reset-gpios = <&gpio0 92 0>;
> Use standard defines for GPIO flags.

fixed, thanks

>> +            io-backends = <&axi_dac>;
> Why do you need to point to the parent? How much coupled are these
> devices? Child pointing to parent is not usually expected, because
> that's obvious.


"io-backends" is actually the way to refer to the backend module,
(used already for i.e. ad9739a),
it is needed because the backend is not only acting as spi-controller,
but is also providing some APIs for synchronization and bus setup support.

> Best regards,
> Krzysztof
>
Thanks,
Regards,
Angelo

Krzysztof Kozlowski Sept. 24, 2024, 8:02 a.m. UTC | #3

On 23/09/2024 17:50, Angelo Dureghello wrote:
> Hi Krzysztof,
> 
> On 22/09/24 23:02, Krzysztof Kozlowski wrote:
>> On Thu, Sep 19, 2024 at 11:20:00AM +0200, Angelo Dureghello wrote:
>>> From: Angelo Dureghello <[e-mail]>
>>>
>>> There is a version AXI DAC IP block (for FPGAs) that provides
>>> a physical bus for AD3552R and similar chips, and acts as
>>> an SPI controller.
>>>
>>> For this case, the binding is modified to include some
>>> additional properties.
>>>
>>> Signed-off-by: Angelo Dureghello <[e-mail]>
>>> ---
>>>   .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42 ++++++++++++++++++++++
>>>   1 file changed, 42 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>> index 41fe00034742..aca4a41c2633 100644
>>> --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>> @@ -60,6 +60,18 @@ properties:
>>>       $ref: /schemas/types.yaml#/definitions/uint32
>>>       enum: [0, 1, 2, 3]
>>>   
>>> +  io-backends:
>>> +    description: The iio backend reference.
>>> +      An example backend can be found at
>>> +        https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
>>> +    maxItems: 1
>>> +
>>> +  adi,synchronous-mode:
>>> +    description: Enable waiting for external synchronization signal.
>>> +      Some AXI IP configuration can implement a dual-IP layout, with internal
>>> +      wirings for streaming synchronization.
>>> +    type: boolean
>>> +
>>>     '#address-cells':
>>>       const: 1
>>>   
>>> @@ -128,6 +140,7 @@ patternProperties:
>>>             - custom-output-range-config
>>>   
>>>   allOf:
>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>>     - if:
>>>         properties:
>>>           compatible:
>>> @@ -238,4 +251,33 @@ examples:
>>>               };
>>>           };
>>>       };
>>> +
>>> +  - |
>>> +    axi_dac: spi@44a70000 {
>>> +        compatible = "adi,axi-ad3552r";
>> That is either redundant or entire example should go to the parent node,
>> if this device is fixed child of complex device (IOW, adi,ad3552r cannot
>> be used outside of adi,axi-ad3552r).
> 
> ad3552r can still be used by a generic "classic" spi
> controller (SCLK/CS/MISO) but at a slower samplerate, fpga
> controller only (axi-ad3552r) can reach 33MUPS.

OK, then this is just redundant. Drop the node. Parent example should
contain the children, though.
> 
>>
>>> +        reg = <0x44a70000 0x1000>;
>>> +        dmas = <&dac_tx_dma 0>;
>>> +        dma-names = "tx";
>>> +        #io-backend-cells = <0>;
>>> +        clocks = <&ref_clk>;
>>> +
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +
>>> +        dac@0 {
>>> +            compatible = "adi,ad3552r";
>>> +            reg = <0>;
>>> +            reset-gpios = <&gpio0 92 0>;
>> Use standard defines for GPIO flags.
> 
> fixed, thanks
> 
>>> +            io-backends = <&axi_dac>;
>> Why do you need to point to the parent? How much coupled are these
>> devices? Child pointing to parent is not usually expected, because
>> that's obvious.
> 
> 
> "io-backends" is actually the way to refer to the backend module,
> (used already for i.e. ad9739a),
> it is needed because the backend is not only acting as spi-controller,
> but is also providing some APIs for synchronization and bus setup support.


But if backend is the parent, then this is redundant. You can take it
from the child-parent relationship. Is this pointing to other devices
(non-parent) in other ad3552r configurations?

Best regards,
Krzysztof

Nuno Sá Sept. 24, 2024, 12:27 p.m. UTC | #4

On Tue, 2024-09-24 at 10:02 +0200, Krzysztof Kozlowski wrote:
> On 23/09/2024 17:50, Angelo Dureghello wrote:
> > Hi Krzysztof,
> > 
> > On 22/09/24 23:02, Krzysztof Kozlowski wrote:
> > > On Thu, Sep 19, 2024 at 11:20:00AM +0200, Angelo Dureghello wrote:
> > > > From: Angelo Dureghello <[e-mail]>
> > > > 
> > > > There is a version AXI DAC IP block (for FPGAs) that provides
> > > > a physical bus for AD3552R and similar chips, and acts as
> > > > an SPI controller.
> > > > 
> > > > For this case, the binding is modified to include some
> > > > additional properties.
> > > > 
> > > > Signed-off-by: Angelo Dureghello <[e-mail]>
> > > > ---
> > > >   .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42
> > > > ++++++++++++++++++++++
> > > >   1 file changed, 42 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > index 41fe00034742..aca4a41c2633 100644
> > > > --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > @@ -60,6 +60,18 @@ properties:
> > > >       $ref: /schemas/types.yaml#/definitions/uint32
> > > >       enum: [0, 1, 2, 3]
> > > >   
> > > > +  io-backends:
> > > > +    description: The iio backend reference.
> > > > +      An example backend can be found at
> > > > +       
> > > > https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> > > > +    maxItems: 1
> > > > +
> > > > +  adi,synchronous-mode:
> > > > +    description: Enable waiting for external synchronization signal.
> > > > +      Some AXI IP configuration can implement a dual-IP layout, with
> > > > internal
> > > > +      wirings for streaming synchronization.
> > > > +    type: boolean
> > > > +
> > > >     '#address-cells':
> > > >       const: 1
> > > >   
> > > > @@ -128,6 +140,7 @@ patternProperties:
> > > >             - custom-output-range-config
> > > >   
> > > >   allOf:
> > > > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > >     - if:
> > > >         properties:
> > > >           compatible:
> > > > @@ -238,4 +251,33 @@ examples:
> > > >               };
> > > >           };
> > > >       };
> > > > +
> > > > +  - |
> > > > +    axi_dac: spi@44a70000 {
> > > > +        compatible = "adi,axi-ad3552r";
> > > That is either redundant or entire example should go to the parent node,
> > > if this device is fixed child of complex device (IOW, adi,ad3552r cannot
> > > be used outside of adi,axi-ad3552r).
> > 
> > ad3552r can still be used by a generic "classic" spi
> > controller (SCLK/CS/MISO) but at a slower samplerate, fpga
> > controller only (axi-ad3552r) can reach 33MUPS.
> 
> OK, then this is just redundant. Drop the node. Parent example should
> contain the children, though.
> > 
> > > 
> > > > +        reg = <0x44a70000 0x1000>;
> > > > +        dmas = <&dac_tx_dma 0>;
> > > > +        dma-names = "tx";
> > > > +        #io-backend-cells = <0>;
> > > > +        clocks = <&ref_clk>;
> > > > +
> > > > +        #address-cells = <1>;
> > > > +        #size-cells = <0>;
> > > > +
> > > > +        dac@0 {
> > > > +            compatible = "adi,ad3552r";
> > > > +            reg = <0>;
> > > > +            reset-gpios = <&gpio0 92 0>;
> > > Use standard defines for GPIO flags.
> > 
> > fixed, thanks
> > 
> > > > +            io-backends = <&axi_dac>;
> > > Why do you need to point to the parent? How much coupled are these
> > > devices? Child pointing to parent is not usually expected, because
> > > that's obvious.
> > 
> > 
> > "io-backends" is actually the way to refer to the backend module,
> > (used already for i.e. ad9739a),
> > it is needed because the backend is not only acting as spi-controller,
> > but is also providing some APIs for synchronization and bus setup support.
> 
> 
> But if backend is the parent, then this is redundant. You can take it
> from the child-parent relationship. Is this pointing to other devices
> (non-parent) in other ad3552r configurations?
> 

The backend is a provider-consumer type of API. On the consumer side (which is the
driver the child node will probe on), we need to call devm_iio_backend_get() to get
the backend object (which obviously is the parent). For that, 'io-backends' is being
used. We do have another API called __devm_iio_backend_get_from_fwnode_lookup() that
could be used with the parent fwnode and should work. However that was only added to
keep backward compatibility in the first user of the IIO backend framework and it's
not really meant to be used again. We are aware this is awkward at the very least [1]
but hopefully still acceptable.

[1]: https://lore.kernel.org/linux-iio/20240903203935.358a1423@jic23-huawei/
- Nuno Sá

Krzysztof Kozlowski Sept. 25, 2024, 7:22 a.m. UTC | #5

On 24/09/2024 14:27, Nuno Sá wrote:
> On Tue, 2024-09-24 at 10:02 +0200, Krzysztof Kozlowski wrote:
>> On 23/09/2024 17:50, Angelo Dureghello wrote:
>>> Hi Krzysztof,
>>>
>>> On 22/09/24 23:02, Krzysztof Kozlowski wrote:
>>>> On Thu, Sep 19, 2024 at 11:20:00AM +0200, Angelo Dureghello wrote:
>>>>> From: Angelo Dureghello <[e-mail]>
>>>>>
>>>>> There is a version AXI DAC IP block (for FPGAs) that provides
>>>>> a physical bus for AD3552R and similar chips, and acts as
>>>>> an SPI controller.
>>>>>
>>>>> For this case, the binding is modified to include some
>>>>> additional properties.
>>>>>
>>>>> Signed-off-by: Angelo Dureghello <[e-mail]>
>>>>> ---
>>>>>   .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42
>>>>> ++++++++++++++++++++++
>>>>>   1 file changed, 42 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>>>> b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>>>> index 41fe00034742..aca4a41c2633 100644
>>>>> --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>>>> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>>>> @@ -60,6 +60,18 @@ properties:
>>>>>       $ref: /schemas/types.yaml#/definitions/uint32
>>>>>       enum: [0, 1, 2, 3]
>>>>>   
>>>>> +  io-backends:
>>>>> +    description: The iio backend reference.
>>>>> +      An example backend can be found at
>>>>> +       
>>>>> https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  adi,synchronous-mode:
>>>>> +    description: Enable waiting for external synchronization signal.
>>>>> +      Some AXI IP configuration can implement a dual-IP layout, with
>>>>> internal
>>>>> +      wirings for streaming synchronization.
>>>>> +    type: boolean
>>>>> +
>>>>>     '#address-cells':
>>>>>       const: 1
>>>>>   
>>>>> @@ -128,6 +140,7 @@ patternProperties:
>>>>>             - custom-output-range-config
>>>>>   
>>>>>   allOf:
>>>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>>>>     - if:
>>>>>         properties:
>>>>>           compatible:
>>>>> @@ -238,4 +251,33 @@ examples:
>>>>>               };
>>>>>           };
>>>>>       };
>>>>> +
>>>>> +  - |
>>>>> +    axi_dac: spi@44a70000 {
>>>>> +        compatible = "adi,axi-ad3552r";
>>>> That is either redundant or entire example should go to the parent node,
>>>> if this device is fixed child of complex device (IOW, adi,ad3552r cannot
>>>> be used outside of adi,axi-ad3552r).
>>>
>>> ad3552r can still be used by a generic "classic" spi
>>> controller (SCLK/CS/MISO) but at a slower samplerate, fpga
>>> controller only (axi-ad3552r) can reach 33MUPS.
>>
>> OK, then this is just redundant. Drop the node. Parent example should
>> contain the children, though.
>>>
>>>>
>>>>> +        reg = <0x44a70000 0x1000>;
>>>>> +        dmas = <&dac_tx_dma 0>;
>>>>> +        dma-names = "tx";
>>>>> +        #io-backend-cells = <0>;
>>>>> +        clocks = <&ref_clk>;
>>>>> +
>>>>> +        #address-cells = <1>;
>>>>> +        #size-cells = <0>;
>>>>> +
>>>>> +        dac@0 {
>>>>> +            compatible = "adi,ad3552r";
>>>>> +            reg = <0>;
>>>>> +            reset-gpios = <&gpio0 92 0>;
>>>> Use standard defines for GPIO flags.
>>>
>>> fixed, thanks
>>>
>>>>> +            io-backends = <&axi_dac>;
>>>> Why do you need to point to the parent? How much coupled are these
>>>> devices? Child pointing to parent is not usually expected, because
>>>> that's obvious.
>>>
>>>
>>> "io-backends" is actually the way to refer to the backend module,
>>> (used already for i.e. ad9739a),
>>> it is needed because the backend is not only acting as spi-controller,
>>> but is also providing some APIs for synchronization and bus setup support.
>>
>>
>> But if backend is the parent, then this is redundant. You can take it
>> from the child-parent relationship. Is this pointing to other devices
>> (non-parent) in other ad3552r configurations?
>>
> 
> The backend is a provider-consumer type of API. On the consumer side (which is the
> driver the child node will probe on), we need to call devm_iio_backend_get() to get
> the backend object (which obviously is the parent). For that, 'io-backends' is being

You described the driver, so how does it matter? Driver can call
get_backend_from_parent(), right? Or get_backend_from_fwnode(parent)?

> used. We do have another API called __devm_iio_backend_get_from_fwnode_lookup() that
> could be used with the parent fwnode and should work. However that was only added to
> keep backward compatibility in the first user of the IIO backend framework and it's
> not really meant to be used again. We are aware this is awkward at the very least [1]
> but hopefully still acceptable.

Don't use driver reasons for discussing hardware. Answer my first
question, because it is still ignored.

Best regards,
Krzysztof

Nuno Sá Sept. 25, 2024, 11:55 a.m. UTC | #6

On Wed, 2024-09-25 at 09:22 +0200, Krzysztof Kozlowski wrote:
> On 24/09/2024 14:27, Nuno Sá wrote:
> > On Tue, 2024-09-24 at 10:02 +0200, Krzysztof Kozlowski wrote:
> > > On 23/09/2024 17:50, Angelo Dureghello wrote:
> > > > Hi Krzysztof,
> > > > 
> > > > On 22/09/24 23:02, Krzysztof Kozlowski wrote:
> > > > > On Thu, Sep 19, 2024 at 11:20:00AM +0200, Angelo Dureghello wrote:
> > > > > > From: Angelo Dureghello <[e-mail]>
> > > > > > 
> > > > > > There is a version AXI DAC IP block (for FPGAs) that provides
> > > > > > a physical bus for AD3552R and similar chips, and acts as
> > > > > > an SPI controller.
> > > > > > 
> > > > > > For this case, the binding is modified to include some
> > > > > > additional properties.
> > > > > > 
> > > > > > Signed-off-by: Angelo Dureghello <[e-mail]>
> > > > > > ---
> > > > > >   .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42
> > > > > > ++++++++++++++++++++++
> > > > > >   1 file changed, 42 insertions(+)
> > > > > > 
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > > > index 41fe00034742..aca4a41c2633 100644
> > > > > > --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > > > @@ -60,6 +60,18 @@ properties:
> > > > > >       $ref: /schemas/types.yaml#/definitions/uint32
> > > > > >       enum: [0, 1, 2, 3]
> > > > > >   
> > > > > > +  io-backends:
> > > > > > +    description: The iio backend reference.
> > > > > > +      An example backend can be found at
> > > > > > +       
> > > > > > https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> > > > > > +    maxItems: 1
> > > > > > +
> > > > > > +  adi,synchronous-mode:
> > > > > > +    description: Enable waiting for external synchronization
> > > > > > signal.
> > > > > > +      Some AXI IP configuration can implement a dual-IP layout,
> > > > > > with
> > > > > > internal
> > > > > > +      wirings for streaming synchronization.
> > > > > > +    type: boolean
> > > > > > +
> > > > > >     '#address-cells':
> > > > > >       const: 1
> > > > > >   
> > > > > > @@ -128,6 +140,7 @@ patternProperties:
> > > > > >             - custom-output-range-config
> > > > > >   
> > > > > >   allOf:
> > > > > > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > > > >     - if:
> > > > > >         properties:
> > > > > >           compatible:
> > > > > > @@ -238,4 +251,33 @@ examples:
> > > > > >               };
> > > > > >           };
> > > > > >       };
> > > > > > +
> > > > > > +  - |
> > > > > > +    axi_dac: spi@44a70000 {
> > > > > > +        compatible = "adi,axi-ad3552r";
> > > > > That is either redundant or entire example should go to the parent
> > > > > node,
> > > > > if this device is fixed child of complex device (IOW, adi,ad3552r
> > > > > cannot
> > > > > be used outside of adi,axi-ad3552r).
> > > > 
> > > > ad3552r can still be used by a generic "classic" spi
> > > > controller (SCLK/CS/MISO) but at a slower samplerate, fpga
> > > > controller only (axi-ad3552r) can reach 33MUPS.
> > > 
> > > OK, then this is just redundant. Drop the node. Parent example should
> > > contain the children, though.
> > > > 
> > > > > 
> > > > > > +        reg = <0x44a70000 0x1000>;
> > > > > > +        dmas = <&dac_tx_dma 0>;
> > > > > > +        dma-names = "tx";
> > > > > > +        #io-backend-cells = <0>;
> > > > > > +        clocks = <&ref_clk>;
> > > > > > +
> > > > > > +        #address-cells = <1>;
> > > > > > +        #size-cells = <0>;
> > > > > > +
> > > > > > +        dac@0 {
> > > > > > +            compatible = "adi,ad3552r";
> > > > > > +            reg = <0>;
> > > > > > +            reset-gpios = <&gpio0 92 0>;
> > > > > Use standard defines for GPIO flags.
> > > > 
> > > > fixed, thanks
> > > > 
> > > > > > +            io-backends = <&axi_dac>;
> > > > > Why do you need to point to the parent? How much coupled are these
> > > > > devices? Child pointing to parent is not usually expected, because
> > > > > that's obvious.
> > > > 
> > > > 
> > > > "io-backends" is actually the way to refer to the backend module,
> > > > (used already for i.e. ad9739a),
> > > > it is needed because the backend is not only acting as spi-controller,
> > > > but is also providing some APIs for synchronization and bus setup
> > > > support.
> > > 
> > > 
> > > But if backend is the parent, then this is redundant. You can take it
> > > from the child-parent relationship. Is this pointing to other devices
> > > (non-parent) in other ad3552r configurations?
> > > 
> > 
> > The backend is a provider-consumer type of API. On the consumer side (which
> > is the
> > driver the child node will probe on), we need to call devm_iio_backend_get()
> > to get
> > the backend object (which obviously is the parent). For that, 'io-backends'
> > is being
> 
> You described the driver, so how does it matter? Driver can call
> get_backend_from_parent(), right? Or get_backend_from_fwnode(parent)?

Well yes, just stating what the framework (also in terms of bindings) is
expecting. Of course that on the driver side we can paper around it the way we
want. But my main point was that we can only paper around it if we use code that
is meant not to be used.

And, FWIW, I was (trying) replying to your comment

"You can take it from the child-parent relationship"

Again, we can only do that by introducing new code or use code that's not meant
to be used. The way we're supposed to reference backends is by explicitly using
the proper FW property.

Put it in another way and a completely hypothetical case. If we have a spi
controller which happens to export some clock and one of it's peripherals ends
up using that clock, wouldn't we still use 'clocks' to reference that clock?

Again, if this is too weird to be acceptable in the bindings we take it from the
child - parent relationship. 


- Nuno Sá

Krzysztof Kozlowski Sept. 28, 2024, 12:20 p.m. UTC | #7

On 25/09/2024 13:55, Nuno Sá wrote:
> On Wed, 2024-09-25 at 09:22 +0200, Krzysztof Kozlowski wrote:
>> On 24/09/2024 14:27, Nuno Sá wrote:
>>> On Tue, 2024-09-24 at 10:02 +0200, Krzysztof Kozlowski wrote:
>>>> On 23/09/2024 17:50, Angelo Dureghello wrote:
>>>>> Hi Krzysztof,
>>>>>
>>>>> On 22/09/24 23:02, Krzysztof Kozlowski wrote:
>>>>>> On Thu, Sep 19, 2024 at 11:20:00AM +0200, Angelo Dureghello wrote:
>>>>>>> From: Angelo Dureghello <[e-mail]>
>>>>>>>
>>>>>>> There is a version AXI DAC IP block (for FPGAs) that provides
>>>>>>> a physical bus for AD3552R and similar chips, and acts as
>>>>>>> an SPI controller.
>>>>>>>
>>>>>>> For this case, the binding is modified to include some
>>>>>>> additional properties.
>>>>>>>
>>>>>>> Signed-off-by: Angelo Dureghello <[e-mail]>
>>>>>>> ---
>>>>>>>   .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42
>>>>>>> ++++++++++++++++++++++
>>>>>>>   1 file changed, 42 insertions(+)
>>>>>>>
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>>>>>> b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>>>>>> index 41fe00034742..aca4a41c2633 100644
>>>>>>> --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>>>>>> @@ -60,6 +60,18 @@ properties:
>>>>>>>       $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>>       enum: [0, 1, 2, 3]
>>>>>>>   
>>>>>>> +  io-backends:
>>>>>>> +    description: The iio backend reference.
>>>>>>> +      An example backend can be found at
>>>>>>> +       
>>>>>>> https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  adi,synchronous-mode:
>>>>>>> +    description: Enable waiting for external synchronization
>>>>>>> signal.
>>>>>>> +      Some AXI IP configuration can implement a dual-IP layout,
>>>>>>> with
>>>>>>> internal
>>>>>>> +      wirings for streaming synchronization.
>>>>>>> +    type: boolean
>>>>>>> +
>>>>>>>     '#address-cells':
>>>>>>>       const: 1
>>>>>>>   
>>>>>>> @@ -128,6 +140,7 @@ patternProperties:
>>>>>>>             - custom-output-range-config
>>>>>>>   
>>>>>>>   allOf:
>>>>>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>>>>>>     - if:
>>>>>>>         properties:
>>>>>>>           compatible:
>>>>>>> @@ -238,4 +251,33 @@ examples:
>>>>>>>               };
>>>>>>>           };
>>>>>>>       };
>>>>>>> +
>>>>>>> +  - |
>>>>>>> +    axi_dac: spi@44a70000 {
>>>>>>> +        compatible = "adi,axi-ad3552r";
>>>>>> That is either redundant or entire example should go to the parent
>>>>>> node,
>>>>>> if this device is fixed child of complex device (IOW, adi,ad3552r
>>>>>> cannot
>>>>>> be used outside of adi,axi-ad3552r).
>>>>>
>>>>> ad3552r can still be used by a generic "classic" spi
>>>>> controller (SCLK/CS/MISO) but at a slower samplerate, fpga
>>>>> controller only (axi-ad3552r) can reach 33MUPS.
>>>>
>>>> OK, then this is just redundant. Drop the node. Parent example should
>>>> contain the children, though.
>>>>>
>>>>>>
>>>>>>> +        reg = <0x44a70000 0x1000>;
>>>>>>> +        dmas = <&dac_tx_dma 0>;
>>>>>>> +        dma-names = "tx";
>>>>>>> +        #io-backend-cells = <0>;
>>>>>>> +        clocks = <&ref_clk>;
>>>>>>> +
>>>>>>> +        #address-cells = <1>;
>>>>>>> +        #size-cells = <0>;
>>>>>>> +
>>>>>>> +        dac@0 {
>>>>>>> +            compatible = "adi,ad3552r";
>>>>>>> +            reg = <0>;
>>>>>>> +            reset-gpios = <&gpio0 92 0>;
>>>>>> Use standard defines for GPIO flags.
>>>>>
>>>>> fixed, thanks
>>>>>
>>>>>>> +            io-backends = <&axi_dac>;
>>>>>> Why do you need to point to the parent? How much coupled are these
>>>>>> devices? Child pointing to parent is not usually expected, because
>>>>>> that's obvious.
>>>>>
>>>>>
>>>>> "io-backends" is actually the way to refer to the backend module,
>>>>> (used already for i.e. ad9739a),
>>>>> it is needed because the backend is not only acting as spi-controller,
>>>>> but is also providing some APIs for synchronization and bus setup
>>>>> support.
>>>>
>>>>
>>>> But if backend is the parent, then this is redundant. You can take it
>>>> from the child-parent relationship. Is this pointing to other devices
>>>> (non-parent) in other ad3552r configurations?
>>>>
>>>
>>> The backend is a provider-consumer type of API. On the consumer side (which
>>> is the
>>> driver the child node will probe on), we need to call devm_iio_backend_get()
>>> to get
>>> the backend object (which obviously is the parent). For that, 'io-backends'
>>> is being
>>
>> You described the driver, so how does it matter? Driver can call
>> get_backend_from_parent(), right? Or get_backend_from_fwnode(parent)?
> 
> Well yes, just stating what the framework (also in terms of bindings) is
> expecting. Of course that on the driver side we can paper around it the way we
> want. But my main point was that we can only paper around it if we use code that
> is meant not to be used.
> 
> And, FWIW, I was (trying) replying to your comment
> 
> "You can take it from the child-parent relationship"
> 
> Again, we can only do that by introducing new code or use code that's not meant
> to be used. The way we're supposed to reference backends is by explicitly using
> the proper FW property.
> 
> Put it in another way and a completely hypothetical case. If we have a spi
> controller which happens to export some clock and one of it's peripherals ends
> up using that clock, wouldn't we still use 'clocks' to reference that clock?

I asked how coupled are these devices. Never got the answer and you are
reflecting with question. Depends. Please do not create hypothetical,
generic scenarios and then apply them to your one particular opposite case.

Best regards,
Krzysztof

Jonathan Cameron Sept. 29, 2024, 10:51 a.m. UTC | #8

On Thu, 19 Sep 2024 11:20:00 +0200
Angelo Dureghello <[e-mail]> wrote:

> From: Angelo Dureghello <[e-mail]>
> 
> There is a version AXI DAC IP block (for FPGAs) that provides
> a physical bus for AD3552R and similar chips, and acts as
> an SPI controller.

Wrap is a bit short. Aim for < 75 chars for patch descriptions.

> 
> For this case, the binding is modified to include some
> additional properties.
> 
> Signed-off-by: Angelo Dureghello <[e-mail]>
> ---
>  .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42 ++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> index 41fe00034742..aca4a41c2633 100644
> --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> @@ -60,6 +60,18 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      enum: [0, 1, 2, 3]
>  
> +  io-backends:
> +    description: The iio backend reference.

Give a description of what the backend does in this case.  I.e. that it is
a qspi DDR backend with ...

> +      An example backend can be found at
> +        https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> +    maxItems: 1
> +
> +  adi,synchronous-mode:
> +    description: Enable waiting for external synchronization signal.
> +      Some AXI IP configuration can implement a dual-IP layout, with internal
> +      wirings for streaming synchronization.

I've no idea what a dual-IP layout is.  Can you provide a little more info
here?  What are the two IPs?

> +    type: boolean
> +
>    '#address-cells':
>      const: 1
>  
> @@ -128,6 +140,7 @@ patternProperties:
>            - custom-output-range-config
>  
>  allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>    - if:
>        properties:
>          compatible:
> @@ -238,4 +251,33 @@ examples:
>              };
>          };
>      };
> +
> +  - |
> +    axi_dac: spi@44a70000 {
> +        compatible = "adi,axi-ad3552r";
> +        reg = <0x44a70000 0x1000>;
> +        dmas = <&dac_tx_dma 0>;
> +        dma-names = "tx";
> +        #io-backend-cells = <0>;
> +        clocks = <&ref_clk>;
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        dac@0 {
> +            compatible = "adi,ad3552r";
> +            reg = <0>;
> +            reset-gpios = <&gpio0 92 0>;
> +            io-backends = <&axi_dac>;
> +            spi-max-frequency = <66000000>;
> +
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            channel@0 {
> +                reg = <0>;
> +                adi,output-range-microvolt = <(-10000000) (10000000)>;
> +            };
> +        };
> +    };
>  ...
>

Jonathan Cameron Sept. 29, 2024, 10:59 a.m. UTC | #9

On Sat, 28 Sep 2024 14:20:29 +0200
Krzysztof Kozlowski <[e-mail]> wrote:

> On 25/09/2024 13:55, Nuno Sá wrote:
> > On Wed, 2024-09-25 at 09:22 +0200, Krzysztof Kozlowski wrote:  
> >> On 24/09/2024 14:27, Nuno Sá wrote:  
> >>> On Tue, 2024-09-24 at 10:02 +0200, Krzysztof Kozlowski wrote:  
> >>>> On 23/09/2024 17:50, Angelo Dureghello wrote:  
> >>>>> Hi Krzysztof,
> >>>>>
> >>>>> On 22/09/24 23:02, Krzysztof Kozlowski wrote:  
> >>>>>> On Thu, Sep 19, 2024 at 11:20:00AM +0200, Angelo Dureghello wrote:  
> >>>>>>> From: Angelo Dureghello <[e-mail]>
> >>>>>>>
> >>>>>>> There is a version AXI DAC IP block (for FPGAs) that provides
> >>>>>>> a physical bus for AD3552R and similar chips, and acts as
> >>>>>>> an SPI controller.
> >>>>>>>
> >>>>>>> For this case, the binding is modified to include some
> >>>>>>> additional properties.
> >>>>>>>
> >>>>>>> Signed-off-by: Angelo Dureghello <[e-mail]>
> >>>>>>> ---
> >>>>>>>   .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42
> >>>>>>> ++++++++++++++++++++++
> >>>>>>>   1 file changed, 42 insertions(+)
> >>>>>>>
> >>>>>>> diff --git
> >>>>>>> a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> >>>>>>> b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> >>>>>>> index 41fe00034742..aca4a41c2633 100644
> >>>>>>> --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> >>>>>>> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> >>>>>>> @@ -60,6 +60,18 @@ properties:
> >>>>>>>       $ref: /schemas/types.yaml#/definitions/uint32
> >>>>>>>       enum: [0, 1, 2, 3]
> >>>>>>>   
> >>>>>>> +  io-backends:
> >>>>>>> +    description: The iio backend reference.
> >>>>>>> +      An example backend can be found at
> >>>>>>> +       
> >>>>>>> https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> >>>>>>> +    maxItems: 1
> >>>>>>> +
> >>>>>>> +  adi,synchronous-mode:
> >>>>>>> +    description: Enable waiting for external synchronization
> >>>>>>> signal.
> >>>>>>> +      Some AXI IP configuration can implement a dual-IP layout,
> >>>>>>> with
> >>>>>>> internal
> >>>>>>> +      wirings for streaming synchronization.
> >>>>>>> +    type: boolean
> >>>>>>> +
> >>>>>>>     '#address-cells':
> >>>>>>>       const: 1
> >>>>>>>   
> >>>>>>> @@ -128,6 +140,7 @@ patternProperties:
> >>>>>>>             - custom-output-range-config
> >>>>>>>   
> >>>>>>>   allOf:
> >>>>>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> >>>>>>>     - if:
> >>>>>>>         properties:
> >>>>>>>           compatible:
> >>>>>>> @@ -238,4 +251,33 @@ examples:
> >>>>>>>               };
> >>>>>>>           };
> >>>>>>>       };
> >>>>>>> +
> >>>>>>> +  - |
> >>>>>>> +    axi_dac: spi@44a70000 {
> >>>>>>> +        compatible = "adi,axi-ad3552r";  
> >>>>>> That is either redundant or entire example should go to the parent
> >>>>>> node,
> >>>>>> if this device is fixed child of complex device (IOW, adi,ad3552r
> >>>>>> cannot
> >>>>>> be used outside of adi,axi-ad3552r).  
> >>>>>
> >>>>> ad3552r can still be used by a generic "classic" spi
> >>>>> controller (SCLK/CS/MISO) but at a slower samplerate, fpga
> >>>>> controller only (axi-ad3552r) can reach 33MUPS.  
> >>>>
> >>>> OK, then this is just redundant. Drop the node. Parent example should
> >>>> contain the children, though.  
> >>>>>  
> >>>>>>  
> >>>>>>> +        reg = <0x44a70000 0x1000>;
> >>>>>>> +        dmas = <&dac_tx_dma 0>;
> >>>>>>> +        dma-names = "tx";
> >>>>>>> +        #io-backend-cells = <0>;
> >>>>>>> +        clocks = <&ref_clk>;
> >>>>>>> +
> >>>>>>> +        #address-cells = <1>;
> >>>>>>> +        #size-cells = <0>;
> >>>>>>> +
> >>>>>>> +        dac@0 {
> >>>>>>> +            compatible = "adi,ad3552r";
> >>>>>>> +            reg = <0>;
> >>>>>>> +            reset-gpios = <&gpio0 92 0>;  
> >>>>>> Use standard defines for GPIO flags.  
> >>>>>
> >>>>> fixed, thanks
> >>>>>  
> >>>>>>> +            io-backends = <&axi_dac>;  
> >>>>>> Why do you need to point to the parent? How much coupled are these
> >>>>>> devices? Child pointing to parent is not usually expected, because
> >>>>>> that's obvious.  
> >>>>>
> >>>>>
> >>>>> "io-backends" is actually the way to refer to the backend module,
> >>>>> (used already for i.e. ad9739a),
> >>>>> it is needed because the backend is not only acting as spi-controller,
> >>>>> but is also providing some APIs for synchronization and bus setup
> >>>>> support.  
> >>>>
> >>>>
> >>>> But if backend is the parent, then this is redundant. You can take it
> >>>> from the child-parent relationship. Is this pointing to other devices
> >>>> (non-parent) in other ad3552r configurations?
> >>>>  
> >>>
> >>> The backend is a provider-consumer type of API. On the consumer side (which
> >>> is the
> >>> driver the child node will probe on), we need to call devm_iio_backend_get()
> >>> to get
> >>> the backend object (which obviously is the parent). For that, 'io-backends'
> >>> is being  
> >>
> >> You described the driver, so how does it matter? Driver can call
> >> get_backend_from_parent(), right? Or get_backend_from_fwnode(parent)?  
> > 
> > Well yes, just stating what the framework (also in terms of bindings) is
> > expecting. Of course that on the driver side we can paper around it the way we
> > want. But my main point was that we can only paper around it if we use code that
> > is meant not to be used.
> > 
> > And, FWIW, I was (trying) replying to your comment
> > 
> > "You can take it from the child-parent relationship"
> > 
> > Again, we can only do that by introducing new code or use code that's not meant
> > to be used. The way we're supposed to reference backends is by explicitly using
> > the proper FW property.
> > 
> > Put it in another way and a completely hypothetical case. If we have a spi
> > controller which happens to export some clock and one of it's peripherals ends
> > up using that clock, wouldn't we still use 'clocks' to reference that clock?  
> 
> I asked how coupled are these devices. Never got the answer and you are
> reflecting with question. Depends. Please do not create hypothetical,
> generic scenarios and then apply them to your one particular opposite case.

I'll throw a possible clarifying question in here.  Could we use this
device with a multimaster SPI setup such that the control is on a conventional
SPI controller (maybe a qspi capable one), and the data plane only goes through
a specific purpose backend?  If so, then they are not tightly coupled and
the reference makes sense.  Putting it another way, the difference between
this case and all the prior iio-backend bindings is the control and dataplanes
use the same pins.  Does that have to be the case at the host end?  If it does,
then the reference isn't strictly needed and this becomes a bit like
registering a single device on an spi bus or an i2c bus depending on who
does the registering (which is down to the parent in DT).

Jonathan


> 
> Best regards,
> Krzysztof
>

Nuno Sá Sept. 30, 2024, 7:20 a.m. UTC | #10

On Sun, 2024-09-29 at 11:59 +0100, Jonathan Cameron wrote:
> On Sat, 28 Sep 2024 14:20:29 +0200
> Krzysztof Kozlowski <[e-mail]> wrote:
> 
> > On 25/09/2024 13:55, Nuno Sá wrote:
> > > On Wed, 2024-09-25 at 09:22 +0200, Krzysztof Kozlowski wrote:  
> > > > On 24/09/2024 14:27, Nuno Sá wrote:  
> > > > > On Tue, 2024-09-24 at 10:02 +0200, Krzysztof Kozlowski wrote:  
> > > > > > On 23/09/2024 17:50, Angelo Dureghello wrote:  
> > > > > > > Hi Krzysztof,
> > > > > > > 
> > > > > > > On 22/09/24 23:02, Krzysztof Kozlowski wrote:  
> > > > > > > > On Thu, Sep 19, 2024 at 11:20:00AM +0200, Angelo Dureghello
> > > > > > > > wrote:  
> > > > > > > > > From: Angelo Dureghello <[e-mail]>
> > > > > > > > > 
> > > > > > > > > There is a version AXI DAC IP block (for FPGAs) that provides
> > > > > > > > > a physical bus for AD3552R and similar chips, and acts as
> > > > > > > > > an SPI controller.
> > > > > > > > > 
> > > > > > > > > For this case, the binding is modified to include some
> > > > > > > > > additional properties.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Angelo Dureghello <[e-mail]>
> > > > > > > > > ---
> > > > > > > > >   .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42
> > > > > > > > > ++++++++++++++++++++++
> > > > > > > > >   1 file changed, 42 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git
> > > > > > > > > a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > > > > > > index 41fe00034742..aca4a41c2633 100644
> > > > > > > > > ---
> > > > > > > > > a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > > > > > > +++
> > > > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > > > > > > @@ -60,6 +60,18 @@ properties:
> > > > > > > > >       $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > > > >       enum: [0, 1, 2, 3]
> > > > > > > > >   
> > > > > > > > > +  io-backends:
> > > > > > > > > +    description: The iio backend reference.
> > > > > > > > > +      An example backend can be found at
> > > > > > > > > +       
> > > > > > > > > https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> > > > > > > > > +    maxItems: 1
> > > > > > > > > +
> > > > > > > > > +  adi,synchronous-mode:
> > > > > > > > > +    description: Enable waiting for external synchronization
> > > > > > > > > signal.
> > > > > > > > > +      Some AXI IP configuration can implement a dual-IP
> > > > > > > > > layout,
> > > > > > > > > with
> > > > > > > > > internal
> > > > > > > > > +      wirings for streaming synchronization.
> > > > > > > > > +    type: boolean
> > > > > > > > > +
> > > > > > > > >     '#address-cells':
> > > > > > > > >       const: 1
> > > > > > > > >   
> > > > > > > > > @@ -128,6 +140,7 @@ patternProperties:
> > > > > > > > >             - custom-output-range-config
> > > > > > > > >   
> > > > > > > > >   allOf:
> > > > > > > > > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > > > > > > >     - if:
> > > > > > > > >         properties:
> > > > > > > > >           compatible:
> > > > > > > > > @@ -238,4 +251,33 @@ examples:
> > > > > > > > >               };
> > > > > > > > >           };
> > > > > > > > >       };
> > > > > > > > > +
> > > > > > > > > +  - |
> > > > > > > > > +    axi_dac: spi@44a70000 {
> > > > > > > > > +        compatible = "adi,axi-ad3552r";  
> > > > > > > > That is either redundant or entire example should go to the
> > > > > > > > parent
> > > > > > > > node,
> > > > > > > > if this device is fixed child of complex device (IOW,
> > > > > > > > adi,ad3552r
> > > > > > > > cannot
> > > > > > > > be used outside of adi,axi-ad3552r).  
> > > > > > > 
> > > > > > > ad3552r can still be used by a generic "classic" spi
> > > > > > > controller (SCLK/CS/MISO) but at a slower samplerate, fpga
> > > > > > > controller only (axi-ad3552r) can reach 33MUPS.  
> > > > > > 
> > > > > > OK, then this is just redundant. Drop the node. Parent example
> > > > > > should
> > > > > > contain the children, though.  
> > > > > > >  
> > > > > > > >  
> > > > > > > > > +        reg = <0x44a70000 0x1000>;
> > > > > > > > > +        dmas = <&dac_tx_dma 0>;
> > > > > > > > > +        dma-names = "tx";
> > > > > > > > > +        #io-backend-cells = <0>;
> > > > > > > > > +        clocks = <&ref_clk>;
> > > > > > > > > +
> > > > > > > > > +        #address-cells = <1>;
> > > > > > > > > +        #size-cells = <0>;
> > > > > > > > > +
> > > > > > > > > +        dac@0 {
> > > > > > > > > +            compatible = "adi,ad3552r";
> > > > > > > > > +            reg = <0>;
> > > > > > > > > +            reset-gpios = <&gpio0 92 0>;  
> > > > > > > > Use standard defines for GPIO flags.  
> > > > > > > 
> > > > > > > fixed, thanks
> > > > > > >  
> > > > > > > > > +            io-backends = <&axi_dac>;  
> > > > > > > > Why do you need to point to the parent? How much coupled are
> > > > > > > > these
> > > > > > > > devices? Child pointing to parent is not usually expected,
> > > > > > > > because
> > > > > > > > that's obvious.  
> > > > > > > 
> > > > > > > 
> > > > > > > "io-backends" is actually the way to refer to the backend module,
> > > > > > > (used already for i.e. ad9739a),
> > > > > > > it is needed because the backend is not only acting as spi-
> > > > > > > controller,
> > > > > > > but is also providing some APIs for synchronization and bus setup
> > > > > > > support.  
> > > > > > 
> > > > > > 
> > > > > > But if backend is the parent, then this is redundant. You can take
> > > > > > it
> > > > > > from the child-parent relationship. Is this pointing to other
> > > > > > devices
> > > > > > (non-parent) in other ad3552r configurations?
> > > > > >  
> > > > > 
> > > > > The backend is a provider-consumer type of API. On the consumer side
> > > > > (which
> > > > > is the
> > > > > driver the child node will probe on), we need to call
> > > > > devm_iio_backend_get()
> > > > > to get
> > > > > the backend object (which obviously is the parent). For that, 'io-
> > > > > backends'
> > > > > is being  
> > > > 
> > > > You described the driver, so how does it matter? Driver can call
> > > > get_backend_from_parent(), right? Or get_backend_from_fwnode(parent)?  
> > > 
> > > Well yes, just stating what the framework (also in terms of bindings) is
> > > expecting. Of course that on the driver side we can paper around it the
> > > way we
> > > want. But my main point was that we can only paper around it if we use
> > > code that
> > > is meant not to be used.
> > > 
> > > And, FWIW, I was (trying) replying to your comment
> > > 
> > > "You can take it from the child-parent relationship"
> > > 
> > > Again, we can only do that by introducing new code or use code that's not
> > > meant
> > > to be used. The way we're supposed to reference backends is by explicitly
> > > using
> > > the proper FW property.
> > > 
> > > Put it in another way and a completely hypothetical case. If we have a spi
> > > controller which happens to export some clock and one of it's peripherals
> > > ends
> > > up using that clock, wouldn't we still use 'clocks' to reference that
> > > clock?  
> > 
> > I asked how coupled are these devices. Never got the answer and you are
> > reflecting with question. Depends. Please do not create hypothetical,
> > generic scenarios and then apply them to your one particular opposite case.
> 
> I'll throw a possible clarifying question in here.  Could we use this
> device with a multimaster SPI setup such that the control is on a conventional
> SPI controller (maybe a qspi capable one), and the data plane only goes
> through
> a specific purpose backend?  If so, then they are not tightly coupled and
> the reference makes sense.  Putting it another way, the difference between
> this case and all the prior iio-backend bindings is the control and dataplanes
> use the same pins.  Does that have to be the case at the host end?  If it
> does,
> then the reference isn't strictly needed and this becomes a bit like
> registering a single device on an spi bus or an i2c bus depending on who
> does the registering (which is down to the parent in DT).
> 

So, we currently have two drivers (with a new one being added in this series)
for the same device:

1) A SPI one tied to a typical spi controller. This is the "low speed"
implementation and does not use backends;
2) The new platform device that is connected like this to the backend.

So yes, my understanding (but Angelo should know better :)) is that they are
tightly coupled. Putting it in another way, the new platform device is very much
specific to this parent (and yeah, this is a very special usecase where control
and data planes are controlled by the IIO backend) and should not exist with it.

- Nuno Sá

Krzysztof Kozlowski Sept. 30, 2024, 7:31 a.m. UTC | #11

On 30/09/2024 09:20, Nuno Sá wrote:
>>>>>
>>>>> You described the driver, so how does it matter? Driver can call
>>>>> get_backend_from_parent(), right? Or get_backend_from_fwnode(parent)?  
>>>>
>>>> Well yes, just stating what the framework (also in terms of bindings) is
>>>> expecting. Of course that on the driver side we can paper around it the
>>>> way we
>>>> want. But my main point was that we can only paper around it if we use
>>>> code that
>>>> is meant not to be used.
>>>>
>>>> And, FWIW, I was (trying) replying to your comment
>>>>
>>>> "You can take it from the child-parent relationship"
>>>>
>>>> Again, we can only do that by introducing new code or use code that's not
>>>> meant
>>>> to be used. The way we're supposed to reference backends is by explicitly
>>>> using
>>>> the proper FW property.
>>>>
>>>> Put it in another way and a completely hypothetical case. If we have a spi
>>>> controller which happens to export some clock and one of it's peripherals
>>>> ends
>>>> up using that clock, wouldn't we still use 'clocks' to reference that
>>>> clock?  
>>>
>>> I asked how coupled are these devices. Never got the answer and you are
>>> reflecting with question. Depends. Please do not create hypothetical,
>>> generic scenarios and then apply them to your one particular opposite case.
>>
>> I'll throw a possible clarifying question in here.  Could we use this
>> device with a multimaster SPI setup such that the control is on a conventional
>> SPI controller (maybe a qspi capable one), and the data plane only goes
>> through
>> a specific purpose backend?  If so, then they are not tightly coupled and
>> the reference makes sense.  Putting it another way, the difference between
>> this case and all the prior iio-backend bindings is the control and dataplanes
>> use the same pins.  Does that have to be the case at the host end?  If it
>> does,
>> then the reference isn't strictly needed and this becomes a bit like
>> registering a single device on an spi bus or an i2c bus depending on who
>> does the registering (which is down to the parent in DT).
>>
> 
> So, we currently have two drivers (with a new one being added in this series)
> for the same device:
> 
> 1) A SPI one tied to a typical spi controller. This is the "low speed"
> implementation and does not use backends;
> 2) The new platform device that is connected like this to the backend.

Drivers, platform devices are Linux specifics. These were not our
questions here. You are responding with description matching current
Linux code.

> 
> So yes, my understanding (but Angelo should know better :)) is that they are
> tightly coupled. Putting it in another way, the new platform device is very much
> specific to this parent (and yeah, this is a very special usecase where control

Again, Linux stuff.

> and data planes are controlled by the IIO backend) and should not exist with it.

I pointed this issue already in this thread. You keep describing
drivers, so of course they will be coupled as much as you write them.

Best regards,
Krzysztof

Nuno Sá Sept. 30, 2024, 8:24 a.m. UTC | #12

On Mon, 2024-09-30 at 09:31 +0200, Krzysztof Kozlowski wrote:
> On 30/09/2024 09:20, Nuno Sá wrote:
> > > > > > 
> > > > > > You described the driver, so how does it matter? Driver can call
> > > > > > get_backend_from_parent(), right? Or
> > > > > > get_backend_from_fwnode(parent)?  
> > > > > 
> > > > > Well yes, just stating what the framework (also in terms of bindings)
> > > > > is
> > > > > expecting. Of course that on the driver side we can paper around it
> > > > > the
> > > > > way we
> > > > > want. But my main point was that we can only paper around it if we use
> > > > > code that
> > > > > is meant not to be used.
> > > > > 
> > > > > And, FWIW, I was (trying) replying to your comment
> > > > > 
> > > > > "You can take it from the child-parent relationship"
> > > > > 
> > > > > Again, we can only do that by introducing new code or use code that's
> > > > > not
> > > > > meant
> > > > > to be used. The way we're supposed to reference backends is by
> > > > > explicitly
> > > > > using
> > > > > the proper FW property.
> > > > > 
> > > > > Put it in another way and a completely hypothetical case. If we have a
> > > > > spi
> > > > > controller which happens to export some clock and one of it's
> > > > > peripherals
> > > > > ends
> > > > > up using that clock, wouldn't we still use 'clocks' to reference that
> > > > > clock?  
> > > > 
> > > > I asked how coupled are these devices. Never got the answer and you are
> > > > reflecting with question. Depends. Please do not create hypothetical,
> > > > generic scenarios and then apply them to your one particular opposite
> > > > case.
> > > 
> > > I'll throw a possible clarifying question in here.  Could we use this
> > > device with a multimaster SPI setup such that the control is on a
> > > conventional
> > > SPI controller (maybe a qspi capable one), and the data plane only goes
> > > through
> > > a specific purpose backend?  If so, then they are not tightly coupled and
> > > the reference makes sense.  Putting it another way, the difference between
> > > this case and all the prior iio-backend bindings is the control and
> > > dataplanes
> > > use the same pins.  Does that have to be the case at the host end?  If it
> > > does,
> > > then the reference isn't strictly needed and this becomes a bit like
> > > registering a single device on an spi bus or an i2c bus depending on who
> > > does the registering (which is down to the parent in DT).
> > > 
> > 
> > So, we currently have two drivers (with a new one being added in this
> > series)
> > for the same device:
> > 
> > 1) A SPI one tied to a typical spi controller. This is the "low speed"
> > implementation and does not use backends;
> > 2) The new platform device that is connected like this to the backend.
> 
> Drivers, platform devices are Linux specifics. These were not our
> questions here. You are responding with description matching current
> Linux code.
> 
> > 
> > So yes, my understanding (but Angelo should know better :)) is that they are
> > tightly coupled. Putting it in another way, the new platform device is very
> > much
> > specific to this parent (and yeah, this is a very special usecase where
> > control
> 
> Again, Linux stuff.
> 
> > and data planes are controlled by the IIO backend) and should not exist with
> > it.
> 
> I pointed this issue already in this thread. You keep describing
> drivers, so of course they will be coupled as much as you write them.
> 

Well, because this is how it's being used and it's easy for me to fall into the
implementation but ok, I get your point. Directly then replying in terms of HW,
this could be used in a way where we have a typical spi controller handling the
device and the data plane only going through the backend. In fact, the HW folks
first tried the SPI ENGINE IP (which is a typically controller) but could not
get the maximum sampling rate out of the device so they came up with this custom
design. So, in theory is possible, in practise will likely never happen but I
guess that does not matter for the bindings?

- Nuno Sá

Angelo Dureghello Sept. 30, 2024, 1:22 p.m. UTC | #13

On 30.09.2024 09:20, Nuno Sá wrote:
> On Sun, 2024-09-29 at 11:59 +0100, Jonathan Cameron wrote:
> > On Sat, 28 Sep 2024 14:20:29 +0200
> > Krzysztof Kozlowski <[e-mail]> wrote:
> > 
> > > On 25/09/2024 13:55, Nuno Sá wrote:
> > > > On Wed, 2024-09-25 at 09:22 +0200, Krzysztof Kozlowski wrote:  
> > > > > On 24/09/2024 14:27, Nuno Sá wrote:  
> > > > > > On Tue, 2024-09-24 at 10:02 +0200, Krzysztof Kozlowski wrote:  
> > > > > > > On 23/09/2024 17:50, Angelo Dureghello wrote:  
> > > > > > > > Hi Krzysztof,
> > > > > > > > 
> > > > > > > > On 22/09/24 23:02, Krzysztof Kozlowski wrote:  
> > > > > > > > > On Thu, Sep 19, 2024 at 11:20:00AM +0200, Angelo Dureghello
> > > > > > > > > wrote:  
> > > > > > > > > > From: Angelo Dureghello <[e-mail]>
> > > > > > > > > > 
> > > > > > > > > > There is a version AXI DAC IP block (for FPGAs) that provides
> > > > > > > > > > a physical bus for AD3552R and similar chips, and acts as
> > > > > > > > > > an SPI controller.
> > > > > > > > > > 
> > > > > > > > > > For this case, the binding is modified to include some
> > > > > > > > > > additional properties.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Angelo Dureghello <[e-mail]>
> > > > > > > > > > ---
> > > > > > > > > >   .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42
> > > > > > > > > > ++++++++++++++++++++++
> > > > > > > > > >   1 file changed, 42 insertions(+)
> > > > > > > > > > 
> > > > > > > > > > diff --git
> > > > > > > > > > a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > > > > > > > index 41fe00034742..aca4a41c2633 100644
> > > > > > > > > > ---
> > > > > > > > > > a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > > > > > > > +++
> > > > > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > > > > > > > @@ -60,6 +60,18 @@ properties:
> > > > > > > > > >       $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > > > > >       enum: [0, 1, 2, 3]
> > > > > > > > > >   
> > > > > > > > > > +  io-backends:
> > > > > > > > > > +    description: The iio backend reference.
> > > > > > > > > > +      An example backend can be found at
> > > > > > > > > > +       
> > > > > > > > > > https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> > > > > > > > > > +    maxItems: 1
> > > > > > > > > > +
> > > > > > > > > > +  adi,synchronous-mode:
> > > > > > > > > > +    description: Enable waiting for external synchronization
> > > > > > > > > > signal.
> > > > > > > > > > +      Some AXI IP configuration can implement a dual-IP
> > > > > > > > > > layout,
> > > > > > > > > > with
> > > > > > > > > > internal
> > > > > > > > > > +      wirings for streaming synchronization.
> > > > > > > > > > +    type: boolean
> > > > > > > > > > +
> > > > > > > > > >     '#address-cells':
> > > > > > > > > >       const: 1
> > > > > > > > > >   
> > > > > > > > > > @@ -128,6 +140,7 @@ patternProperties:
> > > > > > > > > >             - custom-output-range-config
> > > > > > > > > >   
> > > > > > > > > >   allOf:
> > > > > > > > > > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > > > > > > > >     - if:
> > > > > > > > > >         properties:
> > > > > > > > > >           compatible:
> > > > > > > > > > @@ -238,4 +251,33 @@ examples:
> > > > > > > > > >               };
> > > > > > > > > >           };
> > > > > > > > > >       };
> > > > > > > > > > +
> > > > > > > > > > +  - |
> > > > > > > > > > +    axi_dac: spi@44a70000 {
> > > > > > > > > > +        compatible = "adi,axi-ad3552r";  
> > > > > > > > > That is either redundant or entire example should go to the
> > > > > > > > > parent
> > > > > > > > > node,
> > > > > > > > > if this device is fixed child of complex device (IOW,
> > > > > > > > > adi,ad3552r
> > > > > > > > > cannot
> > > > > > > > > be used outside of adi,axi-ad3552r).  
> > > > > > > > 
> > > > > > > > ad3552r can still be used by a generic "classic" spi
> > > > > > > > controller (SCLK/CS/MISO) but at a slower samplerate, fpga
> > > > > > > > controller only (axi-ad3552r) can reach 33MUPS.  
> > > > > > > 
> > > > > > > OK, then this is just redundant. Drop the node. Parent example
> > > > > > > should
> > > > > > > contain the children, though.  
> > > > > > > >  
> > > > > > > > >  
> > > > > > > > > > +        reg = <0x44a70000 0x1000>;
> > > > > > > > > > +        dmas = <&dac_tx_dma 0>;
> > > > > > > > > > +        dma-names = "tx";
> > > > > > > > > > +        #io-backend-cells = <0>;
> > > > > > > > > > +        clocks = <&ref_clk>;
> > > > > > > > > > +
> > > > > > > > > > +        #address-cells = <1>;
> > > > > > > > > > +        #size-cells = <0>;
> > > > > > > > > > +
> > > > > > > > > > +        dac@0 {
> > > > > > > > > > +            compatible = "adi,ad3552r";
> > > > > > > > > > +            reg = <0>;
> > > > > > > > > > +            reset-gpios = <&gpio0 92 0>;  
> > > > > > > > > Use standard defines for GPIO flags.  
> > > > > > > > 
> > > > > > > > fixed, thanks
> > > > > > > >  
> > > > > > > > > > +            io-backends = <&axi_dac>;  
> > > > > > > > > Why do you need to point to the parent? How much coupled are
> > > > > > > > > these
> > > > > > > > > devices? Child pointing to parent is not usually expected,
> > > > > > > > > because
> > > > > > > > > that's obvious.  
> > > > > > > > 
> > > > > > > > 
> > > > > > > > "io-backends" is actually the way to refer to the backend module,
> > > > > > > > (used already for i.e. ad9739a),
> > > > > > > > it is needed because the backend is not only acting as spi-
> > > > > > > > controller,
> > > > > > > > but is also providing some APIs for synchronization and bus setup
> > > > > > > > support.  
> > > > > > > 
> > > > > > > 
> > > > > > > But if backend is the parent, then this is redundant. You can take
> > > > > > > it
> > > > > > > from the child-parent relationship. Is this pointing to other
> > > > > > > devices
> > > > > > > (non-parent) in other ad3552r configurations?
> > > > > > >  
> > > > > > 
> > > > > > The backend is a provider-consumer type of API. On the consumer side
> > > > > > (which
> > > > > > is the
> > > > > > driver the child node will probe on), we need to call
> > > > > > devm_iio_backend_get()
> > > > > > to get
> > > > > > the backend object (which obviously is the parent). For that, 'io-
> > > > > > backends'
> > > > > > is being  
> > > > > 
> > > > > You described the driver, so how does it matter? Driver can call
> > > > > get_backend_from_parent(), right? Or get_backend_from_fwnode(parent)?  
> > > > 
> > > > Well yes, just stating what the framework (also in terms of bindings) is
> > > > expecting. Of course that on the driver side we can paper around it the
> > > > way we
> > > > want. But my main point was that we can only paper around it if we use
> > > > code that
> > > > is meant not to be used.
> > > > 
> > > > And, FWIW, I was (trying) replying to your comment
> > > > 
> > > > "You can take it from the child-parent relationship"
> > > > 
> > > > Again, we can only do that by introducing new code or use code that's not
> > > > meant
> > > > to be used. The way we're supposed to reference backends is by explicitly
> > > > using
> > > > the proper FW property.
> > > > 
> > > > Put it in another way and a completely hypothetical case. If we have a spi
> > > > controller which happens to export some clock and one of it's peripherals
> > > > ends
> > > > up using that clock, wouldn't we still use 'clocks' to reference that
> > > > clock?  
> > > 
> > > I asked how coupled are these devices. Never got the answer and you are
> > > reflecting with question. Depends. Please do not create hypothetical,
> > > generic scenarios and then apply them to your one particular opposite case.
> > 
> > I'll throw a possible clarifying question in here.  Could we use this
> > device with a multimaster SPI setup such that the control is on a conventional
> > SPI controller (maybe a qspi capable one), and the data plane only goes
> > through
> > a specific purpose backend?  If so, then they are not tightly coupled and
> > the reference makes sense.  Putting it another way, the difference between
> > this case and all the prior iio-backend bindings is the control and dataplanes
> > use the same pins.  Does that have to be the case at the host end?  If it
> > does,
> > then the reference isn't strictly needed and this becomes a bit like
> > registering a single device on an spi bus or an i2c bus depending on who
> > does the registering (which is down to the parent in DT).
> > 
> 
> So, we currently have two drivers (with a new one being added in this series)
> for the same device:
> 
> 1) A SPI one tied to a typical spi controller. This is the "low speed"
> implementation and does not use backends;
> 2) The new platform device that is connected like this to the backend.
> 
> So yes, my understanding (but Angelo should know better :)) is that they are
> tightly coupled. Putting it in another way, the new platform device is very much
> specific to this parent (and yeah, this is a very special usecase where control
> and data planes are controlled by the IIO backend) and should not exist with it.

ad3552r device can be coupled to the axi-ad3552r controller or to a generic 
spi controler.

We have actually 2 drivers, SPI and platform (for AXI controller, in this patch).

Scenario 1 (SPI):
ad3522r can hypotetically work with whatever simple spi controller that can
read/write registers in raw mode. On simple SPI (CS, SCLK, MOSI), due to ad3552r
chip limitation of 66Mhz clock, the maximum 33MUPS (16 bit samples) cannot be
reached. Some QSPI DDR controller seems to be around, in that case, ad3552r
may work extending the SPI driver. 

Scenario 2 (AXI):
From an hardware-only point ov view axi-ad3552r IP acts as QSPI+DDR controller
plus some additional features for stream synchronization.
From a sowftware point of view, really different from a spi controller driver.
It's just a backend with APIes that can be called from the child driver.


> 
> - Nuno Sá
> 

Regards,
Angelo

Angelo Dureghello Sept. 30, 2024, 2:15 p.m. UTC | #14

On 29.09.2024 11:51, Jonathan Cameron wrote:
> On Thu, 19 Sep 2024 11:20:00 +0200
> Angelo Dureghello <[e-mail]> wrote:
> 
> > From: Angelo Dureghello <[e-mail]>
> > 
> > There is a version AXI DAC IP block (for FPGAs) that provides
> > a physical bus for AD3552R and similar chips, and acts as
> > an SPI controller.
> 
> Wrap is a bit short. Aim for < 75 chars for patch descriptions.
> 
> > 
> > For this case, the binding is modified to include some
> > additional properties.
> > 
> > Signed-off-by: Angelo Dureghello <[e-mail]>
> > ---
> >  .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42 ++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > index 41fe00034742..aca4a41c2633 100644
> > --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > @@ -60,6 +60,18 @@ properties:
> >      $ref: /schemas/types.yaml#/definitions/uint32
> >      enum: [0, 1, 2, 3]
> >  
> > +  io-backends:
> > +    description: The iio backend reference.
> 
> Give a description of what the backend does in this case.  I.e. that it is
> a qspi DDR backend with ...
> 
> > +      An example backend can be found at
> > +        https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> > +    maxItems: 1
> > +
> > +  adi,synchronous-mode:
> > +    description: Enable waiting for external synchronization signal.
> > +      Some AXI IP configuration can implement a dual-IP layout, with internal
> > +      wirings for streaming synchronization.
> 
> I've no idea what a dual-IP layout is.  Can you provide a little more info
> here?  What are the two IPs?
>
IP is a term used in fpga design as "intellectual property", that is
intended as a functional block of logic or data used to make a 
field-programmable gate array module.

A dual layout is just 2 same fpga modules in place of one.
 
I can add a "fpga" regerence to be more clear.

> > +    type: boolean
> > +
> >    '#address-cells':
> >      const: 1
> >  
> > @@ -128,6 +140,7 @@ patternProperties:
> >            - custom-output-range-config
> >  
> >  allOf:
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> >    - if:
> >        properties:
> >          compatible:
> > @@ -238,4 +251,33 @@ examples:
> >              };
> >          };
> >      };
> > +
> > +  - |
> > +    axi_dac: spi@44a70000 {
> > +        compatible = "adi,axi-ad3552r";
> > +        reg = <0x44a70000 0x1000>;
> > +        dmas = <&dac_tx_dma 0>;
> > +        dma-names = "tx";
> > +        #io-backend-cells = <0>;
> > +        clocks = <&ref_clk>;
> > +
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        dac@0 {
> > +            compatible = "adi,ad3552r";
> > +            reg = <0>;
> > +            reset-gpios = <&gpio0 92 0>;
> > +            io-backends = <&axi_dac>;
> > +            spi-max-frequency = <66000000>;
> > +
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            channel@0 {
> > +                reg = <0>;
> > +                adi,output-range-microvolt = <(-10000000) (10000000)>;
> > +            };
> > +        };
> > +    };
> >  ...
> > 
> 

Regards,
Angelo

Jonathan Cameron Sept. 30, 2024, 2:49 p.m. UTC | #15

On Mon, 30 Sep 2024 16:15:41 +0200
Angelo Dureghello <[e-mail]> wrote:

> On 29.09.2024 11:51, Jonathan Cameron wrote:
> > On Thu, 19 Sep 2024 11:20:00 +0200
> > Angelo Dureghello <[e-mail]> wrote:
> >   
> > > From: Angelo Dureghello <[e-mail]>
> > > 
> > > There is a version AXI DAC IP block (for FPGAs) that provides
> > > a physical bus for AD3552R and similar chips, and acts as
> > > an SPI controller.  
> > 
> > Wrap is a bit short. Aim for < 75 chars for patch descriptions.
> >   
> > > 
> > > For this case, the binding is modified to include some
> > > additional properties.
> > > 
> > > Signed-off-by: Angelo Dureghello <[e-mail]>
> > > ---
> > >  .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42 ++++++++++++++++++++++
> > >  1 file changed, 42 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > index 41fe00034742..aca4a41c2633 100644
> > > --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > @@ -60,6 +60,18 @@ properties:
> > >      $ref: /schemas/types.yaml#/definitions/uint32
> > >      enum: [0, 1, 2, 3]
> > >  
> > > +  io-backends:
> > > +    description: The iio backend reference.  
> > 
> > Give a description of what the backend does in this case.  I.e. that it is
> > a qspi DDR backend with ...
> >   
> > > +      An example backend can be found at
> > > +        https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> > > +    maxItems: 1
> > > +
> > > +  adi,synchronous-mode:
> > > +    description: Enable waiting for external synchronization signal.
> > > +      Some AXI IP configuration can implement a dual-IP layout, with internal
> > > +      wirings for streaming synchronization.  
> > 
> > I've no idea what a dual-IP layout is.  Can you provide a little more info
> > here?  What are the two IPs?
> >  
> IP is a term used in fpga design as "intellectual property", that is
> intended as a functional block of logic or data used to make a 
> field-programmable gate array module.
> 
> A dual layout is just 2 same fpga modules in place of one.
>  
> I can add a "fpga" regerence to be more clear.

IP I was familiar with.  I'm more interested in what each IP is doing in this
case.  Or at least an example of what sort of split of functionality might
make use of this.

Jonathan

Angelo Dureghello Sept. 30, 2024, 3:08 p.m. UTC | #16

On 30.09.2024 15:49, Jonathan Cameron wrote:
> On Mon, 30 Sep 2024 16:15:41 +0200
> Angelo Dureghello <[e-mail]> wrote:
> 
> > On 29.09.2024 11:51, Jonathan Cameron wrote:
> > > On Thu, 19 Sep 2024 11:20:00 +0200
> > > Angelo Dureghello <[e-mail]> wrote:
> > >   
> > > > From: Angelo Dureghello <[e-mail]>
> > > > 
> > > > There is a version AXI DAC IP block (for FPGAs) that provides
> > > > a physical bus for AD3552R and similar chips, and acts as
> > > > an SPI controller.  
> > > 
> > > Wrap is a bit short. Aim for < 75 chars for patch descriptions.
> > >   
> > > > 
> > > > For this case, the binding is modified to include some
> > > > additional properties.
> > > > 
> > > > Signed-off-by: Angelo Dureghello <[e-mail]>
> > > > ---
> > > >  .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42 ++++++++++++++++++++++
> > > >  1 file changed, 42 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > index 41fe00034742..aca4a41c2633 100644
> > > > --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > @@ -60,6 +60,18 @@ properties:
> > > >      $ref: /schemas/types.yaml#/definitions/uint32
> > > >      enum: [0, 1, 2, 3]
> > > >  
> > > > +  io-backends:
> > > > +    description: The iio backend reference.  
> > > 
> > > Give a description of what the backend does in this case.  I.e. that it is
> > > a qspi DDR backend with ...
> > >   
> > > > +      An example backend can be found at
> > > > +        https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> > > > +    maxItems: 1
> > > > +
> > > > +  adi,synchronous-mode:
> > > > +    description: Enable waiting for external synchronization signal.
> > > > +      Some AXI IP configuration can implement a dual-IP layout, with internal
> > > > +      wirings for streaming synchronization.  
> > > 
> > > I've no idea what a dual-IP layout is.  Can you provide a little more info
> > > here?  What are the two IPs?
> > >  
> > IP is a term used in fpga design as "intellectual property", that is
> > intended as a functional block of logic or data used to make a 
> > field-programmable gate array module.
> > 
> > A dual layout is just 2 same fpga modules in place of one.
> >  
> > I can add a "fpga" regerence to be more clear.
> 
> IP I was familiar with.  I'm more interested in what each IP is doing in this
> case.  Or at least an example of what sort of split of functionality might
> make use of this.
>

I have an image of the project (that is under development or testing now),
not sure how to attach the image here, btw, something as
 
          axi_ad3552r_0  ----------->---- qspi0
              sync_ext_device --.
       .- external_sync          |
       |                         |
       |-------------<-----------                        
       |
       |   axi_ad3552r_1 ----------->---- qspi1
       `- external_sync
 
My understanding is that it's just a method to use a octal spi,
duplicating the transfer rate. I can collect more info in case.


> Jonathan

Regards,
Angelo

Jonathan Cameron Sept. 30, 2024, 3:09 p.m. UTC | #17

On Mon, 30 Sep 2024 15:22:01 +0200
Angelo Dureghello <[e-mail]> wrote:

> On 30.09.2024 09:20, Nuno Sá wrote:
> > On Sun, 2024-09-29 at 11:59 +0100, Jonathan Cameron wrote:  
> > > On Sat, 28 Sep 2024 14:20:29 +0200
> > > Krzysztof Kozlowski <[e-mail]> wrote:
> > >   
> > > > On 25/09/2024 13:55, Nuno Sá wrote:  
> > > > > On Wed, 2024-09-25 at 09:22 +0200, Krzysztof Kozlowski wrote:    
> > > > > > On 24/09/2024 14:27, Nuno Sá wrote:    
> > > > > > > On Tue, 2024-09-24 at 10:02 +0200, Krzysztof Kozlowski wrote:    
> > > > > > > > On 23/09/2024 17:50, Angelo Dureghello wrote:    
> > > > > > > > > Hi Krzysztof,
> > > > > > > > > 
> > > > > > > > > On 22/09/24 23:02, Krzysztof Kozlowski wrote:    
> > > > > > > > > > On Thu, Sep 19, 2024 at 11:20:00AM +0200, Angelo Dureghello
> > > > > > > > > > wrote:    
> > > > > > > > > > > From: Angelo Dureghello <[e-mail]>
> > > > > > > > > > > 
> > > > > > > > > > > There is a version AXI DAC IP block (for FPGAs) that provides
> > > > > > > > > > > a physical bus for AD3552R and similar chips, and acts as
> > > > > > > > > > > an SPI controller.
> > > > > > > > > > > 
> > > > > > > > > > > For this case, the binding is modified to include some
> > > > > > > > > > > additional properties.
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Angelo Dureghello <[e-mail]>
> > > > > > > > > > > ---
> > > > > > > > > > >   .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42
> > > > > > > > > > > ++++++++++++++++++++++
> > > > > > > > > > >   1 file changed, 42 insertions(+)
> > > > > > > > > > > 
> > > > > > > > > > > diff --git
> > > > > > > > > > > a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > > > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > > > > > > > > index 41fe00034742..aca4a41c2633 100644
> > > > > > > > > > > ---
> > > > > > > > > > > a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > > > > > > > > +++
> > > > > > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> > > > > > > > > > > @@ -60,6 +60,18 @@ properties:
> > > > > > > > > > >       $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > > > > > >       enum: [0, 1, 2, 3]
> > > > > > > > > > >   
> > > > > > > > > > > +  io-backends:
> > > > > > > > > > > +    description: The iio backend reference.
> > > > > > > > > > > +      An example backend can be found at
> > > > > > > > > > > +       
> > > > > > > > > > > https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> > > > > > > > > > > +    maxItems: 1
> > > > > > > > > > > +
> > > > > > > > > > > +  adi,synchronous-mode:
> > > > > > > > > > > +    description: Enable waiting for external synchronization
> > > > > > > > > > > signal.
> > > > > > > > > > > +      Some AXI IP configuration can implement a dual-IP
> > > > > > > > > > > layout,
> > > > > > > > > > > with
> > > > > > > > > > > internal
> > > > > > > > > > > +      wirings for streaming synchronization.
> > > > > > > > > > > +    type: boolean
> > > > > > > > > > > +
> > > > > > > > > > >     '#address-cells':
> > > > > > > > > > >       const: 1
> > > > > > > > > > >   
> > > > > > > > > > > @@ -128,6 +140,7 @@ patternProperties:
> > > > > > > > > > >             - custom-output-range-config
> > > > > > > > > > >   
> > > > > > > > > > >   allOf:
> > > > > > > > > > > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > > > > > > > > >     - if:
> > > > > > > > > > >         properties:
> > > > > > > > > > >           compatible:
> > > > > > > > > > > @@ -238,4 +251,33 @@ examples:
> > > > > > > > > > >               };
> > > > > > > > > > >           };
> > > > > > > > > > >       };
> > > > > > > > > > > +
> > > > > > > > > > > +  - |
> > > > > > > > > > > +    axi_dac: spi@44a70000 {
> > > > > > > > > > > +        compatible = "adi,axi-ad3552r";    
> > > > > > > > > > That is either redundant or entire example should go to the
> > > > > > > > > > parent
> > > > > > > > > > node,
> > > > > > > > > > if this device is fixed child of complex device (IOW,
> > > > > > > > > > adi,ad3552r
> > > > > > > > > > cannot
> > > > > > > > > > be used outside of adi,axi-ad3552r).    
> > > > > > > > > 
> > > > > > > > > ad3552r can still be used by a generic "classic" spi
> > > > > > > > > controller (SCLK/CS/MISO) but at a slower samplerate, fpga
> > > > > > > > > controller only (axi-ad3552r) can reach 33MUPS.    
> > > > > > > > 
> > > > > > > > OK, then this is just redundant. Drop the node. Parent example
> > > > > > > > should
> > > > > > > > contain the children, though.    
> > > > > > > > >    
> > > > > > > > > >    
> > > > > > > > > > > +        reg = <0x44a70000 0x1000>;
> > > > > > > > > > > +        dmas = <&dac_tx_dma 0>;
> > > > > > > > > > > +        dma-names = "tx";
> > > > > > > > > > > +        #io-backend-cells = <0>;
> > > > > > > > > > > +        clocks = <&ref_clk>;
> > > > > > > > > > > +
> > > > > > > > > > > +        #address-cells = <1>;
> > > > > > > > > > > +        #size-cells = <0>;
> > > > > > > > > > > +
> > > > > > > > > > > +        dac@0 {
> > > > > > > > > > > +            compatible = "adi,ad3552r";
> > > > > > > > > > > +            reg = <0>;
> > > > > > > > > > > +            reset-gpios = <&gpio0 92 0>;    
> > > > > > > > > > Use standard defines for GPIO flags.    
> > > > > > > > > 
> > > > > > > > > fixed, thanks
> > > > > > > > >    
> > > > > > > > > > > +            io-backends = <&axi_dac>;    
> > > > > > > > > > Why do you need to point to the parent? How much coupled are
> > > > > > > > > > these
> > > > > > > > > > devices? Child pointing to parent is not usually expected,
> > > > > > > > > > because
> > > > > > > > > > that's obvious.    
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > "io-backends" is actually the way to refer to the backend module,
> > > > > > > > > (used already for i.e. ad9739a),
> > > > > > > > > it is needed because the backend is not only acting as spi-
> > > > > > > > > controller,
> > > > > > > > > but is also providing some APIs for synchronization and bus setup
> > > > > > > > > support.    
> > > > > > > > 
> > > > > > > > 
> > > > > > > > But if backend is the parent, then this is redundant. You can take
> > > > > > > > it
> > > > > > > > from the child-parent relationship. Is this pointing to other
> > > > > > > > devices
> > > > > > > > (non-parent) in other ad3552r configurations?
> > > > > > > >    
> > > > > > > 
> > > > > > > The backend is a provider-consumer type of API. On the consumer side
> > > > > > > (which
> > > > > > > is the
> > > > > > > driver the child node will probe on), we need to call
> > > > > > > devm_iio_backend_get()
> > > > > > > to get
> > > > > > > the backend object (which obviously is the parent). For that, 'io-
> > > > > > > backends'
> > > > > > > is being    
> > > > > > 
> > > > > > You described the driver, so how does it matter? Driver can call
> > > > > > get_backend_from_parent(), right? Or get_backend_from_fwnode(parent)?    
> > > > > 
> > > > > Well yes, just stating what the framework (also in terms of bindings) is
> > > > > expecting. Of course that on the driver side we can paper around it the
> > > > > way we
> > > > > want. But my main point was that we can only paper around it if we use
> > > > > code that
> > > > > is meant not to be used.
> > > > > 
> > > > > And, FWIW, I was (trying) replying to your comment
> > > > > 
> > > > > "You can take it from the child-parent relationship"
> > > > > 
> > > > > Again, we can only do that by introducing new code or use code that's not
> > > > > meant
> > > > > to be used. The way we're supposed to reference backends is by explicitly
> > > > > using
> > > > > the proper FW property.
> > > > > 
> > > > > Put it in another way and a completely hypothetical case. If we have a spi
> > > > > controller which happens to export some clock and one of it's peripherals
> > > > > ends
> > > > > up using that clock, wouldn't we still use 'clocks' to reference that
> > > > > clock?    
> > > > 
> > > > I asked how coupled are these devices. Never got the answer and you are
> > > > reflecting with question. Depends. Please do not create hypothetical,
> > > > generic scenarios and then apply them to your one particular opposite case.  
> > > 
> > > I'll throw a possible clarifying question in here.  Could we use this
> > > device with a multimaster SPI setup such that the control is on a conventional
> > > SPI controller (maybe a qspi capable one), and the data plane only goes
> > > through
> > > a specific purpose backend?  If so, then they are not tightly coupled and
> > > the reference makes sense.  Putting it another way, the difference between
> > > this case and all the prior iio-backend bindings is the control and dataplanes
> > > use the same pins.  Does that have to be the case at the host end?  If it
> > > does,
> > > then the reference isn't strictly needed and this becomes a bit like
> > > registering a single device on an spi bus or an i2c bus depending on who
> > > does the registering (which is down to the parent in DT).
> > >   
> > 
> > So, we currently have two drivers (with a new one being added in this series)
> > for the same device:
> > 
> > 1) A SPI one tied to a typical spi controller. This is the "low speed"
> > implementation and does not use backends;
> > 2) The new platform device that is connected like this to the backend.
> > 
> > So yes, my understanding (but Angelo should know better :)) is that they are
> > tightly coupled. Putting it in another way, the new platform device is very much
> > specific to this parent (and yeah, this is a very special usecase where control
> > and data planes are controlled by the IIO backend) and should not exist with it.  
> 
> ad3552r device can be coupled to the axi-ad3552r controller or to a generic 
> spi controler.
> 
> We have actually 2 drivers, SPI and platform (for AXI controller, in this patch).
> 
> Scenario 1 (SPI):
> ad3522r can hypotetically work with whatever simple spi controller that can
> read/write registers in raw mode. On simple SPI (CS, SCLK, MOSI), due to ad3552r
> chip limitation of 66Mhz clock, the maximum 33MUPS (16 bit samples) cannot be
> reached. Some QSPI DDR controller seems to be around, in that case, ad3552r
> may work extending the SPI driver. 
> 
> Scenario 2 (AXI):
> From an hardware-only point ov view axi-ad3552r IP acts as QSPI+DDR controller
> plus some additional features for stream synchronization.
> From a sowftware point of view, really different from a spi controller driver.
> It's just a backend with APIes that can be called from the child driver.

Potential? scenario 3 is the one that interested me.

ad3552 double wired to a normal SPI controller (so like option 1) and
to a an offload engine (so like option 2).  With a few pull up resistors
(cs and clk?) and some care it should electrically work I think.
In that case we'd need the io-backend reference because the parent
would be the option 1 like SPI bus and the io-backend would not be
the parent.

David Lechner Sept. 30, 2024, 7:20 p.m. UTC | #18

On 9/30/24 10:08 AM, Angelo Dureghello wrote:
> On 30.09.2024 15:49, Jonathan Cameron wrote:
>> On Mon, 30 Sep 2024 16:15:41 +0200
>> Angelo Dureghello <[e-mail]> wrote:
>>
>>> On 29.09.2024 11:51, Jonathan Cameron wrote:
>>>> On Thu, 19 Sep 2024 11:20:00 +0200
>>>> Angelo Dureghello <[e-mail]> wrote:
>>>>   
>>>>> From: Angelo Dureghello <[e-mail]>
>>>>>
>>>>> There is a version AXI DAC IP block (for FPGAs) that provides
>>>>> a physical bus for AD3552R and similar chips, and acts as
>>>>> an SPI controller.  
>>>>
>>>> Wrap is a bit short. Aim for < 75 chars for patch descriptions.
>>>>   
>>>>>
>>>>> For this case, the binding is modified to include some
>>>>> additional properties.
>>>>>
>>>>> Signed-off-by: Angelo Dureghello <[e-mail]>
>>>>> ---
>>>>>  .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42 ++++++++++++++++++++++
>>>>>  1 file changed, 42 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>>>> index 41fe00034742..aca4a41c2633 100644
>>>>> --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>>>> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
>>>>> @@ -60,6 +60,18 @@ properties:
>>>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>>>      enum: [0, 1, 2, 3]
>>>>>  
>>>>> +  io-backends:
>>>>> +    description: The iio backend reference.  
>>>>
>>>> Give a description of what the backend does in this case.  I.e. that it is
>>>> a qspi DDR backend with ...
>>>>   
>>>>> +      An example backend can be found at
>>>>> +        https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  adi,synchronous-mode:
>>>>> +    description: Enable waiting for external synchronization signal.
>>>>> +      Some AXI IP configuration can implement a dual-IP layout, with internal
>>>>> +      wirings for streaming synchronization.  
>>>>
>>>> I've no idea what a dual-IP layout is.  Can you provide a little more info
>>>> here?  What are the two IPs?
>>>>  
>>> IP is a term used in fpga design as "intellectual property", that is
>>> intended as a functional block of logic or data used to make a 
>>> field-programmable gate array module.
>>>
>>> A dual layout is just 2 same fpga modules in place of one.
>>>  
>>> I can add a "fpga" regerence to be more clear.
>>
>> IP I was familiar with.  I'm more interested in what each IP is doing in this
>> case.  Or at least an example of what sort of split of functionality might
>> make use of this.
>>
> 
> I have an image of the project (that is under development or testing now),
> not sure how to attach the image here, btw, something as
>  
>           axi_ad3552r_0  ----------->---- qspi0
>               sync_ext_device --.
>        .- external_sync          |
>        |                         |
>        |-------------<-----------                        
>        |
>        |   axi_ad3552r_1 ----------->---- qspi1
>        `- external_sync
>  
> My understanding is that it's just a method to use a octal spi,
> duplicating the transfer rate. I can collect more info in case.
> 

No, it's not for octal SPI. It is for synchronizing the data
transfer to two different DAC chips.

I think we need a bit more in the DT bindings for this to fully
describe the wiring shown. We need to indicate that both of the
two AXI AD3552R IP blocks have external_sync connected, so a
adi,external-sync flag could be used for this. Then we also need
to describe that sync_ext_device is only wired up on one of the
IP blocks. So we would need a separate adi,sync-ext-device flag.

Then the driver would use this information to A) know that we
need to set the external sync arm bit when starting buffered
reads and B) know that the buffered read for the IP block
instance with sync_ext_device needs to be started last so that
the data streams for both DACs will be synchronized.

Angelo Dureghello Oct. 1, 2024, 8:09 a.m. UTC | #19

On 30.09.2024 14:20, David Lechner wrote:
> On 9/30/24 10:08 AM, Angelo Dureghello wrote:
> > On 30.09.2024 15:49, Jonathan Cameron wrote:
> >> On Mon, 30 Sep 2024 16:15:41 +0200
> >> Angelo Dureghello <[e-mail]> wrote:
> >>
> >>> On 29.09.2024 11:51, Jonathan Cameron wrote:
> >>>> On Thu, 19 Sep 2024 11:20:00 +0200
> >>>> Angelo Dureghello <[e-mail]> wrote:
> >>>>   
> >>>>> From: Angelo Dureghello <[e-mail]>
> >>>>>
> >>>>> There is a version AXI DAC IP block (for FPGAs) that provides
> >>>>> a physical bus for AD3552R and similar chips, and acts as
> >>>>> an SPI controller.  
> >>>>
> >>>> Wrap is a bit short. Aim for < 75 chars for patch descriptions.
> >>>>   
> >>>>>
> >>>>> For this case, the binding is modified to include some
> >>>>> additional properties.
> >>>>>
> >>>>> Signed-off-by: Angelo Dureghello <[e-mail]>
> >>>>> ---
> >>>>>  .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   | 42 ++++++++++++++++++++++
> >>>>>  1 file changed, 42 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> >>>>> index 41fe00034742..aca4a41c2633 100644
> >>>>> --- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> >>>>> @@ -60,6 +60,18 @@ properties:
> >>>>>      $ref: /schemas/types.yaml#/definitions/uint32
> >>>>>      enum: [0, 1, 2, 3]
> >>>>>  
> >>>>> +  io-backends:
> >>>>> +    description: The iio backend reference.  
> >>>>
> >>>> Give a description of what the backend does in this case.  I.e. that it is
> >>>> a qspi DDR backend with ...
> >>>>   
> >>>>> +      An example backend can be found at
> >>>>> +        https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  adi,synchronous-mode:
> >>>>> +    description: Enable waiting for external synchronization signal.
> >>>>> +      Some AXI IP configuration can implement a dual-IP layout, with internal
> >>>>> +      wirings for streaming synchronization.  
> >>>>
> >>>> I've no idea what a dual-IP layout is.  Can you provide a little more info
> >>>> here?  What are the two IPs?
> >>>>  
> >>> IP is a term used in fpga design as "intellectual property", that is
> >>> intended as a functional block of logic or data used to make a 
> >>> field-programmable gate array module.
> >>>
> >>> A dual layout is just 2 same fpga modules in place of one.
> >>>  
> >>> I can add a "fpga" regerence to be more clear.
> >>
> >> IP I was familiar with.  I'm more interested in what each IP is doing in this
> >> case.  Or at least an example of what sort of split of functionality might
> >> make use of this.
> >>
> > 
> > I have an image of the project (that is under development or testing now),
> > not sure how to attach the image here, btw, something as
> >  
> >           axi_ad3552r_0  ----------->---- qspi0
> >               sync_ext_device --.
> >        .- external_sync          |
> >        |                         |
> >        |-------------<-----------                        
> >        |
> >        |   axi_ad3552r_1 ----------->---- qspi1
> >        `- external_sync
> >  
> > My understanding is that it's just a method to use a octal spi,
> > duplicating the transfer rate. I can collect more info in case.
> > 
> 
> No, it's not for octal SPI. It is for synchronizing the data
> transfer to two different DAC chips.
> 
> I think we need a bit more in the DT bindings for this to fully
> describe the wiring shown. We need to indicate that both of the
> two AXI AD3552R IP blocks have external_sync connected, so a
> adi,external-sync flag could be used for this. Then we also need
> to describe that sync_ext_device is only wired up on one of the
> IP blocks. So we would need a separate adi,sync-ext-device flag.
> 
> Then the driver would use this information to A) know that we
> need to set the external sync arm bit when starting buffered
> reads and B) know that the buffered read for the IP block
> instance with sync_ext_device needs to be started last so that
> the data streams for both DACs will be synchronized.

I thought to add this sync stuff thinking that it will be needed
soon, btw, it is not used right now for a single IP. 
I suggest to remove it entirely, since it is actually dead code,
and this hopefully can fast up things.

Nuno Sá Oct. 1, 2024, 8:23 a.m. UTC | #20

On Mon, 2024-09-30 at 16:09 +0100, Jonathan Cameron wrote:
> On Mon, 30 Sep 2024 15:22:01 +0200
> Angelo Dureghello <[e-mail]> wrote:
> 
> > On 30.09.2024 09:20, Nuno Sá wrote:
> > > On Sun, 2024-09-29 at 11:59 +0100, Jonathan Cameron wrote:  
> > > > On Sat, 28 Sep 2024 14:20:29 +0200
> > > > Krzysztof Kozlowski <[e-mail]> wrote:
> > > >   
> > > > > On 25/09/2024 13:55, Nuno Sá wrote:  
> > > > > > On Wed, 2024-09-25 at 09:22 +0200, Krzysztof Kozlowski wrote:    
> > > > > > > On 24/09/2024 14:27, Nuno Sá wrote:    
> > > > > > > > On Tue, 2024-09-24 at 10:02 +0200, Krzysztof Kozlowski wrote:   
> > > > > > > > > On 23/09/2024 17:50, Angelo Dureghello wrote:    
> > > > > > > > > > Hi Krzysztof,
> > > > > > > > > > 
> > > > > > > > > > On 22/09/24 23:02, Krzysztof Kozlowski wrote:    
> > > > > > > > > > > On Thu, Sep 19, 2024 at 11:20:00AM +0200, Angelo
> > > > > > > > > > > Dureghello
> > > > > > > > > > > wrote:    
> > > > > > > > > > > > From: Angelo Dureghello <[e-mail]>
> > > > > > > > > > > > 
> > > > > > > > > > > > There is a version AXI DAC IP block (for FPGAs) that
> > > > > > > > > > > > provides
> > > > > > > > > > > > a physical bus for AD3552R and similar chips, and acts
> > > > > > > > > > > > as
> > > > > > > > > > > > an SPI controller.
> > > > > > > > > > > > 
> > > > > > > > > > > > For this case, the binding is modified to include some
> > > > > > > > > > > > additional properties.
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Angelo Dureghello
> > > > > > > > > > > > <[e-mail]>
> > > > > > > > > > > > ---
> > > > > > > > > > > >   .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   |
> > > > > > > > > > > > 42
> > > > > > > > > > > > ++++++++++++++++++++++
> > > > > > > > > > > >   1 file changed, 42 insertions(+)
> > > > > > > > > > > > 
> > > > > > > > > > > > diff --git
> > > > > > > > > > > > a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.
> > > > > > > > > > > > yaml
> > > > > > > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.
> > > > > > > > > > > > yaml
> > > > > > > > > > > > index 41fe00034742..aca4a41c2633 100644
> > > > > > > > > > > > ---
> > > > > > > > > > > > a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.
> > > > > > > > > > > > yaml
> > > > > > > > > > > > +++
> > > > > > > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.
> > > > > > > > > > > > yaml
> > > > > > > > > > > > @@ -60,6 +60,18 @@ properties:
> > > > > > > > > > > >       $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > > > > > > >       enum: [0, 1, 2, 3]
> > > > > > > > > > > >   
> > > > > > > > > > > > +  io-backends:
> > > > > > > > > > > > +    description: The iio backend reference.
> > > > > > > > > > > > +      An example backend can be found at
> > > > > > > > > > > > +       
> > > > > > > > > > > > https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> > > > > > > > > > > > +    maxItems: 1
> > > > > > > > > > > > +
> > > > > > > > > > > > +  adi,synchronous-mode:
> > > > > > > > > > > > +    description: Enable waiting for external
> > > > > > > > > > > > synchronization
> > > > > > > > > > > > signal.
> > > > > > > > > > > > +      Some AXI IP configuration can implement a dual-IP
> > > > > > > > > > > > layout,
> > > > > > > > > > > > with
> > > > > > > > > > > > internal
> > > > > > > > > > > > +      wirings for streaming synchronization.
> > > > > > > > > > > > +    type: boolean
> > > > > > > > > > > > +
> > > > > > > > > > > >     '#address-cells':
> > > > > > > > > > > >       const: 1
> > > > > > > > > > > >   
> > > > > > > > > > > > @@ -128,6 +140,7 @@ patternProperties:
> > > > > > > > > > > >             - custom-output-range-config
> > > > > > > > > > > >   
> > > > > > > > > > > >   allOf:
> > > > > > > > > > > > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > > > > > > > > > >     - if:
> > > > > > > > > > > >         properties:
> > > > > > > > > > > >           compatible:
> > > > > > > > > > > > @@ -238,4 +251,33 @@ examples:
> > > > > > > > > > > >               };
> > > > > > > > > > > >           };
> > > > > > > > > > > >       };
> > > > > > > > > > > > +
> > > > > > > > > > > > +  - |
> > > > > > > > > > > > +    axi_dac: spi@44a70000 {
> > > > > > > > > > > > +        compatible = "adi,axi-ad3552r";    
> > > > > > > > > > > That is either redundant or entire example should go to
> > > > > > > > > > > the
> > > > > > > > > > > parent
> > > > > > > > > > > node,
> > > > > > > > > > > if this device is fixed child of complex device (IOW,
> > > > > > > > > > > adi,ad3552r
> > > > > > > > > > > cannot
> > > > > > > > > > > be used outside of adi,axi-ad3552r).    
> > > > > > > > > > 
> > > > > > > > > > ad3552r can still be used by a generic "classic" spi
> > > > > > > > > > controller (SCLK/CS/MISO) but at a slower samplerate, fpga
> > > > > > > > > > controller only (axi-ad3552r) can reach 33MUPS.    
> > > > > > > > > 
> > > > > > > > > OK, then this is just redundant. Drop the node. Parent example
> > > > > > > > > should
> > > > > > > > > contain the children, though.    
> > > > > > > > > >    
> > > > > > > > > > >    
> > > > > > > > > > > > +        reg = <0x44a70000 0x1000>;
> > > > > > > > > > > > +        dmas = <&dac_tx_dma 0>;
> > > > > > > > > > > > +        dma-names = "tx";
> > > > > > > > > > > > +        #io-backend-cells = <0>;
> > > > > > > > > > > > +        clocks = <&ref_clk>;
> > > > > > > > > > > > +
> > > > > > > > > > > > +        #address-cells = <1>;
> > > > > > > > > > > > +        #size-cells = <0>;
> > > > > > > > > > > > +
> > > > > > > > > > > > +        dac@0 {
> > > > > > > > > > > > +            compatible = "adi,ad3552r";
> > > > > > > > > > > > +            reg = <0>;
> > > > > > > > > > > > +            reset-gpios = <&gpio0 92 0>;    
> > > > > > > > > > > Use standard defines for GPIO flags.    
> > > > > > > > > > 
> > > > > > > > > > fixed, thanks
> > > > > > > > > >    
> > > > > > > > > > > > +            io-backends = <&axi_dac>;    
> > > > > > > > > > > Why do you need to point to the parent? How much coupled
> > > > > > > > > > > are
> > > > > > > > > > > these
> > > > > > > > > > > devices? Child pointing to parent is not usually expected,
> > > > > > > > > > > because
> > > > > > > > > > > that's obvious.    
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > "io-backends" is actually the way to refer to the backend
> > > > > > > > > > module,
> > > > > > > > > > (used already for i.e. ad9739a),
> > > > > > > > > > it is needed because the backend is not only acting as spi-
> > > > > > > > > > controller,
> > > > > > > > > > but is also providing some APIs for synchronization and bus
> > > > > > > > > > setup
> > > > > > > > > > support.    
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > But if backend is the parent, then this is redundant. You can
> > > > > > > > > take
> > > > > > > > > it
> > > > > > > > > from the child-parent relationship. Is this pointing to other
> > > > > > > > > devices
> > > > > > > > > (non-parent) in other ad3552r configurations?
> > > > > > > > >    
> > > > > > > > 
> > > > > > > > The backend is a provider-consumer type of API. On the consumer
> > > > > > > > side
> > > > > > > > (which
> > > > > > > > is the
> > > > > > > > driver the child node will probe on), we need to call
> > > > > > > > devm_iio_backend_get()
> > > > > > > > to get
> > > > > > > > the backend object (which obviously is the parent). For that,
> > > > > > > > 'io-
> > > > > > > > backends'
> > > > > > > > is being    
> > > > > > > 
> > > > > > > You described the driver, so how does it matter? Driver can call
> > > > > > > get_backend_from_parent(), right? Or
> > > > > > > get_backend_from_fwnode(parent)?    
> > > > > > 
> > > > > > Well yes, just stating what the framework (also in terms of
> > > > > > bindings) is
> > > > > > expecting. Of course that on the driver side we can paper around it
> > > > > > the
> > > > > > way we
> > > > > > want. But my main point was that we can only paper around it if we
> > > > > > use
> > > > > > code that
> > > > > > is meant not to be used.
> > > > > > 
> > > > > > And, FWIW, I was (trying) replying to your comment
> > > > > > 
> > > > > > "You can take it from the child-parent relationship"
> > > > > > 
> > > > > > Again, we can only do that by introducing new code or use code
> > > > > > that's not
> > > > > > meant
> > > > > > to be used. The way we're supposed to reference backends is by
> > > > > > explicitly
> > > > > > using
> > > > > > the proper FW property.
> > > > > > 
> > > > > > Put it in another way and a completely hypothetical case. If we have
> > > > > > a spi
> > > > > > controller which happens to export some clock and one of it's
> > > > > > peripherals
> > > > > > ends
> > > > > > up using that clock, wouldn't we still use 'clocks' to reference
> > > > > > that
> > > > > > clock?    
> > > > > 
> > > > > I asked how coupled are these devices. Never got the answer and you
> > > > > are
> > > > > reflecting with question. Depends. Please do not create hypothetical,
> > > > > generic scenarios and then apply them to your one particular opposite
> > > > > case.  
> > > > 
> > > > I'll throw a possible clarifying question in here.  Could we use this
> > > > device with a multimaster SPI setup such that the control is on a
> > > > conventional
> > > > SPI controller (maybe a qspi capable one), and the data plane only goes
> > > > through
> > > > a specific purpose backend?  If so, then they are not tightly coupled
> > > > and
> > > > the reference makes sense.  Putting it another way, the difference
> > > > between
> > > > this case and all the prior iio-backend bindings is the control and
> > > > dataplanes
> > > > use the same pins.  Does that have to be the case at the host end?  If
> > > > it
> > > > does,
> > > > then the reference isn't strictly needed and this becomes a bit like
> > > > registering a single device on an spi bus or an i2c bus depending on who
> > > > does the registering (which is down to the parent in DT).
> > > >   
> > > 
> > > So, we currently have two drivers (with a new one being added in this
> > > series)
> > > for the same device:
> > > 
> > > 1) A SPI one tied to a typical spi controller. This is the "low speed"
> > > implementation and does not use backends;
> > > 2) The new platform device that is connected like this to the backend.
> > > 
> > > So yes, my understanding (but Angelo should know better :)) is that they
> > > are
> > > tightly coupled. Putting it in another way, the new platform device is
> > > very much
> > > specific to this parent (and yeah, this is a very special usecase where
> > > control
> > > and data planes are controlled by the IIO backend) and should not exist
> > > with it.  
> > 
> > ad3552r device can be coupled to the axi-ad3552r controller or to a generic 
> > spi controler.
> > 
> > We have actually 2 drivers, SPI and platform (for AXI controller, in this
> > patch).
> > 
> > Scenario 1 (SPI):
> > ad3522r can hypotetically work with whatever simple spi controller that can
> > read/write registers in raw mode. On simple SPI (CS, SCLK, MOSI), due to
> > ad3552r
> > chip limitation of 66Mhz clock, the maximum 33MUPS (16 bit samples) cannot
> > be
> > reached. Some QSPI DDR controller seems to be around, in that case, ad3552r
> > may work extending the SPI driver. 
> > 
> > Scenario 2 (AXI):
> > From an hardware-only point ov view axi-ad3552r IP acts as QSPI+DDR
> > controller
> > plus some additional features for stream synchronization.
> > From a sowftware point of view, really different from a spi controller
> > driver.
> > It's just a backend with APIes that can be called from the child driver.
> 
> Potential? scenario 3 is the one that interested me.
> 
> ad3552 double wired to a normal SPI controller (so like option 1) and
> to a an offload engine (so like option 2).  With a few pull up resistors
> (cs and clk?) and some care it should electrically work I think.
> In that case we'd need the io-backend reference because the parent
> would be the option 1 like SPI bus and the io-backend would not be
> the parent.
> 
> _______________________
> Host       SPI MOSI    |-------------------\
> hard       SPI MISO 0-3|----------------\  |
> QSPI       SPI CLK     |--------------\  | |
>            SPI CS      |----------\    | | |
>                        |           |   | | |
> FPGA                   |           |   | | |   |
> Soft       SPI MOSI    |-----------|---|-|-x---|
> QSPI       SPI MISO 0-3|-----------|---|-x-----|  DAC
> Offload    SPI CLK     |-----------|---x-------|
> with DDR   SPI CS      |-----------x-----------|
> _______________________|
> 
> Makes all sorts of assumptions about the SPI controllers being designed
> for multi controllers on the same SPI buses but I'm not aware of a reason
> you can't do that.
> 
> As the only control message that would need to go over the offload engine
> would be the exit DDR (I think) that might be hard coded into a slightly
> simpler soft IP along with the bulk data transfer stuff.
> 

Not even sure if DDR would be a problem. Right now, I __think__ we need to
enable DDR both the peripheral and on the backend. On the peripheral we could
use the control link on the non offload controller. On the offload controller,
we would set it through IIO backend and that would be a backend config and not
go over the bus.

To make a correction on my previous reply to Krzysztof, the HW folks made some
experiments with the SPI ENIGINE IP (with the offload engine) but without the
AXI DAC IP. So, effectively only one controller in place. That said, I'm also
not seeing any reason why something like the above would not be possible.

- Nuno Sá

Jonathan Cameron Oct. 1, 2024, 6:29 p.m. UTC | #21

On Tue, 01 Oct 2024 10:23:45 +0200
Nuno Sá <[e-mail]> wrote:

> On Mon, 2024-09-30 at 16:09 +0100, Jonathan Cameron wrote:
> > On Mon, 30 Sep 2024 15:22:01 +0200
> > Angelo Dureghello <[e-mail]> wrote:
> >   
> > > On 30.09.2024 09:20, Nuno Sá wrote:  
> > > > On Sun, 2024-09-29 at 11:59 +0100, Jonathan Cameron wrote:    
> > > > > On Sat, 28 Sep 2024 14:20:29 +0200
> > > > > Krzysztof Kozlowski <[e-mail]> wrote:
> > > > >     
> > > > > > On 25/09/2024 13:55, Nuno Sá wrote:    
> > > > > > > On Wed, 2024-09-25 at 09:22 +0200, Krzysztof Kozlowski wrote:      
> > > > > > > > On 24/09/2024 14:27, Nuno Sá wrote:      
> > > > > > > > > On Tue, 2024-09-24 at 10:02 +0200, Krzysztof Kozlowski wrote:     
> > > > > > > > > > On 23/09/2024 17:50, Angelo Dureghello wrote:      
> > > > > > > > > > > Hi Krzysztof,
> > > > > > > > > > > 
> > > > > > > > > > > On 22/09/24 23:02, Krzysztof Kozlowski wrote:      
> > > > > > > > > > > > On Thu, Sep 19, 2024 at 11:20:00AM +0200, Angelo
> > > > > > > > > > > > Dureghello
> > > > > > > > > > > > wrote:      
> > > > > > > > > > > > > From: Angelo Dureghello <[e-mail]>
> > > > > > > > > > > > > 
> > > > > > > > > > > > > There is a version AXI DAC IP block (for FPGAs) that
> > > > > > > > > > > > > provides
> > > > > > > > > > > > > a physical bus for AD3552R and similar chips, and acts
> > > > > > > > > > > > > as
> > > > > > > > > > > > > an SPI controller.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > For this case, the binding is modified to include some
> > > > > > > > > > > > > additional properties.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Signed-off-by: Angelo Dureghello
> > > > > > > > > > > > > <[e-mail]>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >   .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   |
> > > > > > > > > > > > > 42
> > > > > > > > > > > > > ++++++++++++++++++++++
> > > > > > > > > > > > >   1 file changed, 42 insertions(+)
> > > > > > > > > > > > > 
> > > > > > > > > > > > > diff --git
> > > > > > > > > > > > > a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.
> > > > > > > > > > > > > yaml
> > > > > > > > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.
> > > > > > > > > > > > > yaml
> > > > > > > > > > > > > index 41fe00034742..aca4a41c2633 100644
> > > > > > > > > > > > > ---
> > > > > > > > > > > > > a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.
> > > > > > > > > > > > > yaml
> > > > > > > > > > > > > +++
> > > > > > > > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.
> > > > > > > > > > > > > yaml
> > > > > > > > > > > > > @@ -60,6 +60,18 @@ properties:
> > > > > > > > > > > > >       $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > > > > > > > >       enum: [0, 1, 2, 3]
> > > > > > > > > > > > >   
> > > > > > > > > > > > > +  io-backends:
> > > > > > > > > > > > > +    description: The iio backend reference.
> > > > > > > > > > > > > +      An example backend can be found at
> > > > > > > > > > > > > +       
> > > > > > > > > > > > > https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> > > > > > > > > > > > > +    maxItems: 1
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +  adi,synchronous-mode:
> > > > > > > > > > > > > +    description: Enable waiting for external
> > > > > > > > > > > > > synchronization
> > > > > > > > > > > > > signal.
> > > > > > > > > > > > > +      Some AXI IP configuration can implement a dual-IP
> > > > > > > > > > > > > layout,
> > > > > > > > > > > > > with
> > > > > > > > > > > > > internal
> > > > > > > > > > > > > +      wirings for streaming synchronization.
> > > > > > > > > > > > > +    type: boolean
> > > > > > > > > > > > > +
> > > > > > > > > > > > >     '#address-cells':
> > > > > > > > > > > > >       const: 1
> > > > > > > > > > > > >   
> > > > > > > > > > > > > @@ -128,6 +140,7 @@ patternProperties:
> > > > > > > > > > > > >             - custom-output-range-config
> > > > > > > > > > > > >   
> > > > > > > > > > > > >   allOf:
> > > > > > > > > > > > > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > > > > > > > > > > >     - if:
> > > > > > > > > > > > >         properties:
> > > > > > > > > > > > >           compatible:
> > > > > > > > > > > > > @@ -238,4 +251,33 @@ examples:
> > > > > > > > > > > > >               };
> > > > > > > > > > > > >           };
> > > > > > > > > > > > >       };
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +  - |
> > > > > > > > > > > > > +    axi_dac: spi@44a70000 {
> > > > > > > > > > > > > +        compatible = "adi,axi-ad3552r";      
> > > > > > > > > > > > That is either redundant or entire example should go to
> > > > > > > > > > > > the
> > > > > > > > > > > > parent
> > > > > > > > > > > > node,
> > > > > > > > > > > > if this device is fixed child of complex device (IOW,
> > > > > > > > > > > > adi,ad3552r
> > > > > > > > > > > > cannot
> > > > > > > > > > > > be used outside of adi,axi-ad3552r).      
> > > > > > > > > > > 
> > > > > > > > > > > ad3552r can still be used by a generic "classic" spi
> > > > > > > > > > > controller (SCLK/CS/MISO) but at a slower samplerate, fpga
> > > > > > > > > > > controller only (axi-ad3552r) can reach 33MUPS.      
> > > > > > > > > > 
> > > > > > > > > > OK, then this is just redundant. Drop the node. Parent example
> > > > > > > > > > should
> > > > > > > > > > contain the children, though.      
> > > > > > > > > > >      
> > > > > > > > > > > >      
> > > > > > > > > > > > > +        reg = <0x44a70000 0x1000>;
> > > > > > > > > > > > > +        dmas = <&dac_tx_dma 0>;
> > > > > > > > > > > > > +        dma-names = "tx";
> > > > > > > > > > > > > +        #io-backend-cells = <0>;
> > > > > > > > > > > > > +        clocks = <&ref_clk>;
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +        #address-cells = <1>;
> > > > > > > > > > > > > +        #size-cells = <0>;
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +        dac@0 {
> > > > > > > > > > > > > +            compatible = "adi,ad3552r";
> > > > > > > > > > > > > +            reg = <0>;
> > > > > > > > > > > > > +            reset-gpios = <&gpio0 92 0>;      
> > > > > > > > > > > > Use standard defines for GPIO flags.      
> > > > > > > > > > > 
> > > > > > > > > > > fixed, thanks
> > > > > > > > > > >      
> > > > > > > > > > > > > +            io-backends = <&axi_dac>;      
> > > > > > > > > > > > Why do you need to point to the parent? How much coupled
> > > > > > > > > > > > are
> > > > > > > > > > > > these
> > > > > > > > > > > > devices? Child pointing to parent is not usually expected,
> > > > > > > > > > > > because
> > > > > > > > > > > > that's obvious.      
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > "io-backends" is actually the way to refer to the backend
> > > > > > > > > > > module,
> > > > > > > > > > > (used already for i.e. ad9739a),
> > > > > > > > > > > it is needed because the backend is not only acting as spi-
> > > > > > > > > > > controller,
> > > > > > > > > > > but is also providing some APIs for synchronization and bus
> > > > > > > > > > > setup
> > > > > > > > > > > support.      
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > But if backend is the parent, then this is redundant. You can
> > > > > > > > > > take
> > > > > > > > > > it
> > > > > > > > > > from the child-parent relationship. Is this pointing to other
> > > > > > > > > > devices
> > > > > > > > > > (non-parent) in other ad3552r configurations?
> > > > > > > > > >      
> > > > > > > > > 
> > > > > > > > > The backend is a provider-consumer type of API. On the consumer
> > > > > > > > > side
> > > > > > > > > (which
> > > > > > > > > is the
> > > > > > > > > driver the child node will probe on), we need to call
> > > > > > > > > devm_iio_backend_get()
> > > > > > > > > to get
> > > > > > > > > the backend object (which obviously is the parent). For that,
> > > > > > > > > 'io-
> > > > > > > > > backends'
> > > > > > > > > is being      
> > > > > > > > 
> > > > > > > > You described the driver, so how does it matter? Driver can call
> > > > > > > > get_backend_from_parent(), right? Or
> > > > > > > > get_backend_from_fwnode(parent)?      
> > > > > > > 
> > > > > > > Well yes, just stating what the framework (also in terms of
> > > > > > > bindings) is
> > > > > > > expecting. Of course that on the driver side we can paper around it
> > > > > > > the
> > > > > > > way we
> > > > > > > want. But my main point was that we can only paper around it if we
> > > > > > > use
> > > > > > > code that
> > > > > > > is meant not to be used.
> > > > > > > 
> > > > > > > And, FWIW, I was (trying) replying to your comment
> > > > > > > 
> > > > > > > "You can take it from the child-parent relationship"
> > > > > > > 
> > > > > > > Again, we can only do that by introducing new code or use code
> > > > > > > that's not
> > > > > > > meant
> > > > > > > to be used. The way we're supposed to reference backends is by
> > > > > > > explicitly
> > > > > > > using
> > > > > > > the proper FW property.
> > > > > > > 
> > > > > > > Put it in another way and a completely hypothetical case. If we have
> > > > > > > a spi
> > > > > > > controller which happens to export some clock and one of it's
> > > > > > > peripherals
> > > > > > > ends
> > > > > > > up using that clock, wouldn't we still use 'clocks' to reference
> > > > > > > that
> > > > > > > clock?      
> > > > > > 
> > > > > > I asked how coupled are these devices. Never got the answer and you
> > > > > > are
> > > > > > reflecting with question. Depends. Please do not create hypothetical,
> > > > > > generic scenarios and then apply them to your one particular opposite
> > > > > > case.    
> > > > > 
> > > > > I'll throw a possible clarifying question in here.  Could we use this
> > > > > device with a multimaster SPI setup such that the control is on a
> > > > > conventional
> > > > > SPI controller (maybe a qspi capable one), and the data plane only goes
> > > > > through
> > > > > a specific purpose backend?  If so, then they are not tightly coupled
> > > > > and
> > > > > the reference makes sense.  Putting it another way, the difference
> > > > > between
> > > > > this case and all the prior iio-backend bindings is the control and
> > > > > dataplanes
> > > > > use the same pins.  Does that have to be the case at the host end?  If
> > > > > it
> > > > > does,
> > > > > then the reference isn't strictly needed and this becomes a bit like
> > > > > registering a single device on an spi bus or an i2c bus depending on who
> > > > > does the registering (which is down to the parent in DT).
> > > > >     
> > > > 
> > > > So, we currently have two drivers (with a new one being added in this
> > > > series)
> > > > for the same device:
> > > > 
> > > > 1) A SPI one tied to a typical spi controller. This is the "low speed"
> > > > implementation and does not use backends;
> > > > 2) The new platform device that is connected like this to the backend.
> > > > 
> > > > So yes, my understanding (but Angelo should know better :)) is that they
> > > > are
> > > > tightly coupled. Putting it in another way, the new platform device is
> > > > very much
> > > > specific to this parent (and yeah, this is a very special usecase where
> > > > control
> > > > and data planes are controlled by the IIO backend) and should not exist
> > > > with it.    
> > > 
> > > ad3552r device can be coupled to the axi-ad3552r controller or to a generic 
> > > spi controler.
> > > 
> > > We have actually 2 drivers, SPI and platform (for AXI controller, in this
> > > patch).
> > > 
> > > Scenario 1 (SPI):
> > > ad3522r can hypotetically work with whatever simple spi controller that can
> > > read/write registers in raw mode. On simple SPI (CS, SCLK, MOSI), due to
> > > ad3552r
> > > chip limitation of 66Mhz clock, the maximum 33MUPS (16 bit samples) cannot
> > > be
> > > reached. Some QSPI DDR controller seems to be around, in that case, ad3552r
> > > may work extending the SPI driver. 
> > > 
> > > Scenario 2 (AXI):
> > > From an hardware-only point ov view axi-ad3552r IP acts as QSPI+DDR
> > > controller
> > > plus some additional features for stream synchronization.
> > > From a sowftware point of view, really different from a spi controller
> > > driver.
> > > It's just a backend with APIes that can be called from the child driver.  
> > 
> > Potential? scenario 3 is the one that interested me.
> > 
> > ad3552 double wired to a normal SPI controller (so like option 1) and
> > to a an offload engine (so like option 2).  With a few pull up resistors
> > (cs and clk?) and some care it should electrically work I think.
> > In that case we'd need the io-backend reference because the parent
> > would be the option 1 like SPI bus and the io-backend would not be
> > the parent.
> > 
> > _______________________
> > Host       SPI MOSI    |-------------------\
> > hard       SPI MISO 0-3|----------------\  |
> > QSPI       SPI CLK     |--------------\  | |
> >            SPI CS      |----------\    | | |
> >                        |           |   | | |
> > FPGA                   |           |   | | |   |
> > Soft       SPI MOSI    |-----------|---|-|-x---|
> > QSPI       SPI MISO 0-3|-----------|---|-x-----|  DAC
> > Offload    SPI CLK     |-----------|---x-------|
> > with DDR   SPI CS      |-----------x-----------|
> > _______________________|
> > 
> > Makes all sorts of assumptions about the SPI controllers being designed
> > for multi controllers on the same SPI buses but I'm not aware of a reason
> > you can't do that.
> > 
> > As the only control message that would need to go over the offload engine
> > would be the exit DDR (I think) that might be hard coded into a slightly
> > simpler soft IP along with the bulk data transfer stuff.
> >   
> 
> Not even sure if DDR would be a problem. Right now, I __think__ we need to
> enable DDR both the peripheral and on the backend. On the peripheral we could
> use the control link on the non offload controller. On the offload controller,
> we would set it through IIO backend and that would be a backend config and not
> go over the bus.

It's the path back to SDR that I wasn't sure about as it might need a
DDR register write?

> 
> To make a correction on my previous reply to Krzysztof, the HW folks made some
> experiments with the SPI ENIGINE IP (with the offload engine) but without the
> AXI DAC IP. So, effectively only one controller in place. That said, I'm also
> not seeing any reason why something like the above would not be possible.

Conclusion / summary for Krzystoff and other DT binding
folk. It's possible to use this with another backend (which no one has
written the IP for yet) so I (at least) think the reference is needed
even though the only one we have right now is also the parent.

Driver wise, we could in theory poke the parent if the property isn't there
on the off chance it is a suitable backend, but we can't assume that's
the one in use even if it has some suitable support.  Maybe there
is a more capable one double wired?

So I'd like to keep the io-backend phandle there.

Jonathan


> 
> - Nuno Sá
>

Krzysztof Kozlowski Oct. 2, 2024, 5:54 a.m. UTC | #22

On 01/10/2024 20:29, Jonathan Cameron wrote:
>>>
>>> Makes all sorts of assumptions about the SPI controllers being designed
>>> for multi controllers on the same SPI buses but I'm not aware of a reason
>>> you can't do that.
>>>
>>> As the only control message that would need to go over the offload engine
>>> would be the exit DDR (I think) that might be hard coded into a slightly
>>> simpler soft IP along with the bulk data transfer stuff.
>>>   
>>
>> Not even sure if DDR would be a problem. Right now, I __think__ we need to
>> enable DDR both the peripheral and on the backend. On the peripheral we could
>> use the control link on the non offload controller. On the offload controller,
>> we would set it through IIO backend and that would be a backend config and not
>> go over the bus.
> 
> It's the path back to SDR that I wasn't sure about as it might need a
> DDR register write?
> 
>>
>> To make a correction on my previous reply to Krzysztof, the HW folks made some
>> experiments with the SPI ENIGINE IP (with the offload engine) but without the
>> AXI DAC IP. So, effectively only one controller in place. That said, I'm also
>> not seeing any reason why something like the above would not be possible.
> 
> Conclusion / summary for Krzystoff and other DT binding
> folk. It's possible to use this with another backend (which no one has
> written the IP for yet) so I (at least) think the reference is needed
> even though the only one we have right now is also the parent.
> 
> Driver wise, we could in theory poke the parent if the property isn't there
> on the off chance it is a suitable backend, but we can't assume that's
> the one in use even if it has some suitable support.  Maybe there
> is a more capable one double wired?
> 
> So I'd like to keep the io-backend phandle there.

Ack.

Best regards,
Krzysztof

Nuno Sá Oct. 2, 2024, 9 a.m. UTC | #23

On Tue, 2024-10-01 at 19:29 +0100, Jonathan Cameron wrote:
> On Tue, 01 Oct 2024 10:23:45 +0200
> Nuno Sá <[e-mail]> wrote:
> 
> > On Mon, 2024-09-30 at 16:09 +0100, Jonathan Cameron wrote:
> > > On Mon, 30 Sep 2024 15:22:01 +0200
> > > Angelo Dureghello <[e-mail]> wrote:
> > >   
> > > > On 30.09.2024 09:20, Nuno Sá wrote:  
> > > > > On Sun, 2024-09-29 at 11:59 +0100, Jonathan Cameron wrote:    
> > > > > > On Sat, 28 Sep 2024 14:20:29 +0200
> > > > > > Krzysztof Kozlowski <[e-mail]> wrote:
> > > > > >     
> > > > > > > On 25/09/2024 13:55, Nuno Sá wrote:    
> > > > > > > > On Wed, 2024-09-25 at 09:22 +0200, Krzysztof Kozlowski wrote:      
> > > > > > > > > On 24/09/2024 14:27, Nuno Sá wrote:      
> > > > > > > > > > On Tue, 2024-09-24 at 10:02 +0200, Krzysztof Kozlowski wrote:    
> > > > > > > > > > > On 23/09/2024 17:50, Angelo Dureghello wrote:      
> > > > > > > > > > > > Hi Krzysztof,
> > > > > > > > > > > > 
> > > > > > > > > > > > On 22/09/24 23:02, Krzysztof Kozlowski wrote:      
> > > > > > > > > > > > > On Thu, Sep 19, 2024 at 11:20:00AM +0200, Angelo
> > > > > > > > > > > > > Dureghello
> > > > > > > > > > > > > wrote:      
> > > > > > > > > > > > > > From: Angelo Dureghello <[e-mail]>
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > There is a version AXI DAC IP block (for FPGAs) that
> > > > > > > > > > > > > > provides
> > > > > > > > > > > > > > a physical bus for AD3552R and similar chips, and acts
> > > > > > > > > > > > > > as
> > > > > > > > > > > > > > an SPI controller.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > For this case, the binding is modified to include some
> > > > > > > > > > > > > > additional properties.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Signed-off-by: Angelo Dureghello
> > > > > > > > > > > > > > <[e-mail]>
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > >   .../devicetree/bindings/iio/dac/adi,ad3552r.yaml   |
> > > > > > > > > > > > > > 42
> > > > > > > > > > > > > > ++++++++++++++++++++++
> > > > > > > > > > > > > >   1 file changed, 42 insertions(+)
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > diff --git
> > > > > > > > > > > > > > a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.
> > > > > > > > > > > > > > yaml
> > > > > > > > > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.
> > > > > > > > > > > > > > yaml
> > > > > > > > > > > > > > index 41fe00034742..aca4a41c2633 100644
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.
> > > > > > > > > > > > > > yaml
> > > > > > > > > > > > > > +++
> > > > > > > > > > > > > > b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.
> > > > > > > > > > > > > > yaml
> > > > > > > > > > > > > > @@ -60,6 +60,18 @@ properties:
> > > > > > > > > > > > > >       $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > > > > > > > > >       enum: [0, 1, 2, 3]
> > > > > > > > > > > > > >   
> > > > > > > > > > > > > > +  io-backends:
> > > > > > > > > > > > > > +    description: The iio backend reference.
> > > > > > > > > > > > > > +      An example backend can be found at
> > > > > > > > > > > > > > +       
> > > > > > > > > > > > > > https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> > > > > > > > > > > > > > +    maxItems: 1
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +  adi,synchronous-mode:
> > > > > > > > > > > > > > +    description: Enable waiting for external
> > > > > > > > > > > > > > synchronization
> > > > > > > > > > > > > > signal.
> > > > > > > > > > > > > > +      Some AXI IP configuration can implement a dual-IP
> > > > > > > > > > > > > > layout,
> > > > > > > > > > > > > > with
> > > > > > > > > > > > > > internal
> > > > > > > > > > > > > > +      wirings for streaming synchronization.
> > > > > > > > > > > > > > +    type: boolean
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > >     '#address-cells':
> > > > > > > > > > > > > >       const: 1
> > > > > > > > > > > > > >   
> > > > > > > > > > > > > > @@ -128,6 +140,7 @@ patternProperties:
> > > > > > > > > > > > > >             - custom-output-range-config
> > > > > > > > > > > > > >   
> > > > > > > > > > > > > >   allOf:
> > > > > > > > > > > > > > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > > > > > > > > > > > >     - if:
> > > > > > > > > > > > > >         properties:
> > > > > > > > > > > > > >           compatible:
> > > > > > > > > > > > > > @@ -238,4 +251,33 @@ examples:
> > > > > > > > > > > > > >               };
> > > > > > > > > > > > > >           };
> > > > > > > > > > > > > >       };
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +  - |
> > > > > > > > > > > > > > +    axi_dac: spi@44a70000 {
> > > > > > > > > > > > > > +        compatible = "adi,axi-ad3552r";      
> > > > > > > > > > > > > That is either redundant or entire example should go to
> > > > > > > > > > > > > the
> > > > > > > > > > > > > parent
> > > > > > > > > > > > > node,
> > > > > > > > > > > > > if this device is fixed child of complex device (IOW,
> > > > > > > > > > > > > adi,ad3552r
> > > > > > > > > > > > > cannot
> > > > > > > > > > > > > be used outside of adi,axi-ad3552r).      
> > > > > > > > > > > > 
> > > > > > > > > > > > ad3552r can still be used by a generic "classic" spi
> > > > > > > > > > > > controller (SCLK/CS/MISO) but at a slower samplerate, fpga
> > > > > > > > > > > > controller only (axi-ad3552r) can reach 33MUPS.      
> > > > > > > > > > > 
> > > > > > > > > > > OK, then this is just redundant. Drop the node. Parent example
> > > > > > > > > > > should
> > > > > > > > > > > contain the children, though.      
> > > > > > > > > > > >      
> > > > > > > > > > > > >      
> > > > > > > > > > > > > > +        reg = <0x44a70000 0x1000>;
> > > > > > > > > > > > > > +        dmas = <&dac_tx_dma 0>;
> > > > > > > > > > > > > > +        dma-names = "tx";
> > > > > > > > > > > > > > +        #io-backend-cells = <0>;
> > > > > > > > > > > > > > +        clocks = <&ref_clk>;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +        #address-cells = <1>;
> > > > > > > > > > > > > > +        #size-cells = <0>;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +        dac@0 {
> > > > > > > > > > > > > > +            compatible = "adi,ad3552r";
> > > > > > > > > > > > > > +            reg = <0>;
> > > > > > > > > > > > > > +            reset-gpios = <&gpio0 92 0>;      
> > > > > > > > > > > > > Use standard defines for GPIO flags.      
> > > > > > > > > > > > 
> > > > > > > > > > > > fixed, thanks
> > > > > > > > > > > >      
> > > > > > > > > > > > > > +            io-backends = <&axi_dac>;      
> > > > > > > > > > > > > Why do you need to point to the parent? How much coupled
> > > > > > > > > > > > > are
> > > > > > > > > > > > > these
> > > > > > > > > > > > > devices? Child pointing to parent is not usually expected,
> > > > > > > > > > > > > because
> > > > > > > > > > > > > that's obvious.      
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > "io-backends" is actually the way to refer to the backend
> > > > > > > > > > > > module,
> > > > > > > > > > > > (used already for i.e. ad9739a),
> > > > > > > > > > > > it is needed because the backend is not only acting as spi-
> > > > > > > > > > > > controller,
> > > > > > > > > > > > but is also providing some APIs for synchronization and bus
> > > > > > > > > > > > setup
> > > > > > > > > > > > support.      
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > But if backend is the parent, then this is redundant. You can
> > > > > > > > > > > take
> > > > > > > > > > > it
> > > > > > > > > > > from the child-parent relationship. Is this pointing to other
> > > > > > > > > > > devices
> > > > > > > > > > > (non-parent) in other ad3552r configurations?
> > > > > > > > > > >      
> > > > > > > > > > 
> > > > > > > > > > The backend is a provider-consumer type of API. On the consumer
> > > > > > > > > > side
> > > > > > > > > > (which
> > > > > > > > > > is the
> > > > > > > > > > driver the child node will probe on), we need to call
> > > > > > > > > > devm_iio_backend_get()
> > > > > > > > > > to get
> > > > > > > > > > the backend object (which obviously is the parent). For that,
> > > > > > > > > > 'io-
> > > > > > > > > > backends'
> > > > > > > > > > is being      
> > > > > > > > > 
> > > > > > > > > You described the driver, so how does it matter? Driver can call
> > > > > > > > > get_backend_from_parent(), right? Or
> > > > > > > > > get_backend_from_fwnode(parent)?      
> > > > > > > > 
> > > > > > > > Well yes, just stating what the framework (also in terms of
> > > > > > > > bindings) is
> > > > > > > > expecting. Of course that on the driver side we can paper around it
> > > > > > > > the
> > > > > > > > way we
> > > > > > > > want. But my main point was that we can only paper around it if we
> > > > > > > > use
> > > > > > > > code that
> > > > > > > > is meant not to be used.
> > > > > > > > 
> > > > > > > > And, FWIW, I was (trying) replying to your comment
> > > > > > > > 
> > > > > > > > "You can take it from the child-parent relationship"
> > > > > > > > 
> > > > > > > > Again, we can only do that by introducing new code or use code
> > > > > > > > that's not
> > > > > > > > meant
> > > > > > > > to be used. The way we're supposed to reference backends is by
> > > > > > > > explicitly
> > > > > > > > using
> > > > > > > > the proper FW property.
> > > > > > > > 
> > > > > > > > Put it in another way and a completely hypothetical case. If we have
> > > > > > > > a spi
> > > > > > > > controller which happens to export some clock and one of it's
> > > > > > > > peripherals
> > > > > > > > ends
> > > > > > > > up using that clock, wouldn't we still use 'clocks' to reference
> > > > > > > > that
> > > > > > > > clock?      
> > > > > > > 
> > > > > > > I asked how coupled are these devices. Never got the answer and you
> > > > > > > are
> > > > > > > reflecting with question. Depends. Please do not create hypothetical,
> > > > > > > generic scenarios and then apply them to your one particular opposite
> > > > > > > case.    
> > > > > > 
> > > > > > I'll throw a possible clarifying question in here.  Could we use this
> > > > > > device with a multimaster SPI setup such that the control is on a
> > > > > > conventional
> > > > > > SPI controller (maybe a qspi capable one), and the data plane only goes
> > > > > > through
> > > > > > a specific purpose backend?  If so, then they are not tightly coupled
> > > > > > and
> > > > > > the reference makes sense.  Putting it another way, the difference
> > > > > > between
> > > > > > this case and all the prior iio-backend bindings is the control and
> > > > > > dataplanes
> > > > > > use the same pins.  Does that have to be the case at the host end?  If
> > > > > > it
> > > > > > does,
> > > > > > then the reference isn't strictly needed and this becomes a bit like
> > > > > > registering a single device on an spi bus or an i2c bus depending on who
> > > > > > does the registering (which is down to the parent in DT).
> > > > > >     
> > > > > 
> > > > > So, we currently have two drivers (with a new one being added in this
> > > > > series)
> > > > > for the same device:
> > > > > 
> > > > > 1) A SPI one tied to a typical spi controller. This is the "low speed"
> > > > > implementation and does not use backends;
> > > > > 2) The new platform device that is connected like this to the backend.
> > > > > 
> > > > > So yes, my understanding (but Angelo should know better :)) is that they
> > > > > are
> > > > > tightly coupled. Putting it in another way, the new platform device is
> > > > > very much
> > > > > specific to this parent (and yeah, this is a very special usecase where
> > > > > control
> > > > > and data planes are controlled by the IIO backend) and should not exist
> > > > > with it.    
> > > > 
> > > > ad3552r device can be coupled to the axi-ad3552r controller or to a generic 
> > > > spi controler.
> > > > 
> > > > We have actually 2 drivers, SPI and platform (for AXI controller, in this
> > > > patch).
> > > > 
> > > > Scenario 1 (SPI):
> > > > ad3522r can hypotetically work with whatever simple spi controller that can
> > > > read/write registers in raw mode. On simple SPI (CS, SCLK, MOSI), due to
> > > > ad3552r
> > > > chip limitation of 66Mhz clock, the maximum 33MUPS (16 bit samples) cannot
> > > > be
> > > > reached. Some QSPI DDR controller seems to be around, in that case, ad3552r
> > > > may work extending the SPI driver. 
> > > > 
> > > > Scenario 2 (AXI):
> > > > From an hardware-only point ov view axi-ad3552r IP acts as QSPI+DDR
> > > > controller
> > > > plus some additional features for stream synchronization.
> > > > From a sowftware point of view, really different from a spi controller
> > > > driver.
> > > > It's just a backend with APIes that can be called from the child driver.  
> > > 
> > > Potential? scenario 3 is the one that interested me.
> > > 
> > > ad3552 double wired to a normal SPI controller (so like option 1) and
> > > to a an offload engine (so like option 2).  With a few pull up resistors
> > > (cs and clk?) and some care it should electrically work I think.
> > > In that case we'd need the io-backend reference because the parent
> > > would be the option 1 like SPI bus and the io-backend would not be
> > > the parent.
> > > 
> > > _______________________
> > > Host       SPI MOSI    |-------------------\
> > > hard       SPI MISO 0-3|----------------\  |
> > > QSPI       SPI CLK     |--------------\  | |
> > >            SPI CS      |----------\    | | |
> > >                        |           |   | | |
> > > FPGA                   |           |   | | |   |
> > > Soft       SPI MOSI    |-----------|---|-|-x---|
> > > QSPI       SPI MISO 0-3|-----------|---|-x-----|  DAC
> > > Offload    SPI CLK     |-----------|---x-------|
> > > with DDR   SPI CS      |-----------x-----------|
> > > _______________________|
> > > 
> > > Makes all sorts of assumptions about the SPI controllers being designed
> > > for multi controllers on the same SPI buses but I'm not aware of a reason
> > > you can't do that.
> > > 
> > > As the only control message that would need to go over the offload engine
> > > would be the exit DDR (I think) that might be hard coded into a slightly
> > > simpler soft IP along with the bulk data transfer stuff.
> > >   
> > 
> > Not even sure if DDR would be a problem. Right now, I __think__ we need to
> > enable DDR both the peripheral and on the backend. On the peripheral we could
> > use the control link on the non offload controller. On the offload controller,
> > we would set it through IIO backend and that would be a backend config and not
> > go over the bus.
> 
> It's the path back to SDR that I wasn't sure about as it might need a
> DDR register write?

Ah yeah, I see what you mean now. Yeah, that one access would likely have to go
through the offload capable controller.

- Nuno Sá

Patch

diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
index 41fe00034742..aca4a41c2633 100644
--- a/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
@@ -60,6 +60,18 @@  properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     enum: [0, 1, 2, 3]
 
+  io-backends:
+    description: The iio backend reference.
+      An example backend can be found at
+        https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
+    maxItems: 1
+
+  adi,synchronous-mode:
+    description: Enable waiting for external synchronization signal.
+      Some AXI IP configuration can implement a dual-IP layout, with internal
+      wirings for streaming synchronization.
+    type: boolean
+
   '#address-cells':
     const: 1
 
@@ -128,6 +140,7 @@  patternProperties:
           - custom-output-range-config
 
 allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
   - if:
       properties:
         compatible:
@@ -238,4 +251,33 @@  examples:
             };
         };
     };
+
+  - |
+    axi_dac: spi@44a70000 {
+        compatible = "adi,axi-ad3552r";
+        reg = <0x44a70000 0x1000>;
+        dmas = <&dac_tx_dma 0>;
+        dma-names = "tx";
+        #io-backend-cells = <0>;
+        clocks = <&ref_clk>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        dac@0 {
+            compatible = "adi,ad3552r";
+            reg = <0>;
+            reset-gpios = <&gpio0 92 0>;
+            io-backends = <&axi_dac>;
+            spi-max-frequency = <66000000>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            channel@0 {
+                reg = <0>;
+                adi,output-range-microvolt = <(-10000000) (10000000)>;
+            };
+        };
+    };
 ...