Re: [PATCH v3 02/10] dt-bindings: iio: dac: axi-dac: add ad3552r axi variant

[Angelo Dureghello <adureghello@xxxxxxxxxxxx>]

On 29.09.2024 11:46, Jonathan Cameron wrote:
> On Thu, 19 Sep 2024 11:19:58 +0200
> Angelo Dureghello <adureghello@xxxxxxxxxxxx> wrote:
> 
> > From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> > 
> > Add a new compatible and related bindigns for the fpga-based
> > "ad3552r" AXI IP core, a variant of the generic AXI DAC IP.
> > 
> > The AXI "ad3552r" IP is a very similar HDL (fpga) variant of the
> > generic AXI "DAC" IP, intended to control ad3552r and similar chips,
> > mainly to reach high speed transfer rates using an additional QSPI
> 
> I'd drop the word additional as I assume it is an 'either/or' situation
> for the interfaces.
> 
> Do we have other devices using this same IP?  I.e. does it make
> sense to provide a more generic compatible as a fallback for this one
> so that other devices would work without the need for explicit support?
> 
>
no, actually ad3552r-axi is only interfacing to ad3552r.
I could eventually set adi,axi-dac-9.1.b as a fallback, since it
is the "gneric" AXI implementation.
 
> I'd also ideally like a view point from Mark Brown as SPI maintainer
> on how we should deal with this highly specialized spi controller.
> Is he happy with us using an SPI like binding but not figuring out how
> to fit this engine into the SPI subsystem.
> 
> Please +CC Mark and the spi list (done here) on future versions + provide
> a clear description of what is going on for them.
> 

Ok.
Actually i fixed the bindings for v4 setting axi-ad3552r as an
spi-controller, and the target ad3552r as a spi-peripheral (child node).
This axi-ad3552r is not only a pure spi-controller since providing
some synchronization features not typical of a spi-controller. 

> Maybe with the binding fixed as spi compliant, we can figure out the
> if we eventually want to treat this as an SPI controller from the
> kernel driver point of view even if we initially do something 'special'.
>

> Jonathan
> 
> 
> > DDR interface.
> > 
> > The ad3552r device is defined as a child of the AXI DAC, that in
> > this case is acting as an SPI controller.
> > 
> > Signed-off-by: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/iio/dac/adi,axi-dac.yaml   | 40 ++++++++++++++++++++--
> >  1 file changed, 37 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml
> > index a55e9bfc66d7..6cf0c2cb84e7 100644
> > --- a/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml
> > +++ b/Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml
> > @@ -19,11 +19,13 @@ description: |
> >    memory via DMA into the DAC.
> >  
> >    https://wiki.analog.com/resources/fpga/docs/axi_dac_ip
> > +  https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html
> >  
> >  properties:
> >    compatible:
> >      enum:
> >        - adi,axi-dac-9.1.b
> > +      - adi,axi-ad3552r
> >  
> >    reg:
> >      maxItems: 1
> > @@ -41,22 +43,54 @@ properties:
> >    '#io-backend-cells':
> >      const: 0
> >  
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> >  required:
> >    - compatible
> >    - dmas
> >    - reg
> >    - clocks
> >  
> > +patternProperties:
> > +  "^.*@([0-9])$":
> > +    type: object
> > +    additionalProperties: true
> > +    properties:
> > +      io-backends:
> > +        description: |
> > +          AXI backend reference
> > +    required:
> > +      - io-backends
> > +
> >  additionalProperties: false
> >  
> >  examples:
> >    - |
> >      dac@44a00000 {
> > -        compatible = "adi,axi-dac-9.1.b";
> > -        reg = <0x44a00000 0x10000>;
> > -        dmas = <&tx_dma 0>;
> > +      compatible = "adi,axi-dac-9.1.b";
> > +      reg = <0x44a00000 0x10000>;
> > +      dmas = <&tx_dma 0>;
> 
> If it makes sense to reformat then separate patch
> please as this is hard to read as a result of this
> change.  Also, as pointed out, be consistent with spacing.
> 
> > +      dma-names = "tx";
> > +      #io-backend-cells = <0>;
> > +      clocks = <&axi_clk>;
> > +    };
> > +
> > +  - |
> > +    axi_dac: spi@44a70000 {
> > +        compatible = "adi,axi-ad3552r";
> > +        reg = <0x44a70000 0x1000>;
> > +        dmas = <&dac_tx_dma 0>;
> >          dma-names = "tx";
> >          #io-backend-cells = <0>;
> >          clocks = <&axi_clk>;
> > +
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        /* DAC devices */
> >      };
> >  ...
> > 
> 

-- 

  o/ QW5nZWxvIER1cmVnaGVsbG8=
   www.kernel-space.org
    e: angelo at kernel-space.org
      c: +39 388 8550663