Re: [PATCH] dt-bindings: spi: xilinx: Add clocks & clock-names properties

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3691 bytes --]

On Wed, Sep 25, 2024 at 11:35:56AM +0000, Mahapatra, Amit Kumar wrote:
> Hello Conor,
> 
> 
> > -----Original Message-----
> > From: Conor Dooley <conor@kernel.org>
> > Sent: Tuesday, September 24, 2024 10:07 PM
> > To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>
> > Cc: broonie@kernel.org; robh@kernel.org; krzk+dt@kernel.org;
> > conor+dt@kernel.org; Simek, Michal <michal.simek@amd.com>; linux-
> > spi@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git (AMD-Xilinx)
> > <git@amd.com>; amitrkcian2002@gmail.com
> > Subject: Re: [PATCH] dt-bindings: spi: xilinx: Add clocks & clock-names properties
> > 
> > On Mon, Sep 23, 2024 at 06:02:42PM +0530, Amit Kumar Mahapatra wrote:
> > > Include the 'clocks' and 'clock-names' properties in the AXI Quad-SPI
> > > bindings. When the AXI4-Lite interface is enabled, the core operates
> > > in legacy mode, maintaining backward compatibility with version 1.00,
> > > and uses 's_axi_aclk' and 'ext_spi_clk'. For the AXI interface, it
> > > uses 's_axi4_aclk' and 'ext_spi_clk'.
> > >
> > > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com>
> > > ---
> > > BRANCH: for-next
> > > ---
> > >  .../devicetree/bindings/spi/spi-xilinx.yaml   | 29 +++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/spi/spi-xilinx.yaml
> > > b/Documentation/devicetree/bindings/spi/spi-xilinx.yaml
> > > index 4beb3af0416d..9dfec195ecd4 100644
> > > --- a/Documentation/devicetree/bindings/spi/spi-xilinx.yaml
> > > +++ b/Documentation/devicetree/bindings/spi/spi-xilinx.yaml
> > > @@ -12,6 +12,25 @@ maintainers:
> > >  allOf:
> > >    - $ref: spi-controller.yaml#
> > 
> > Please move the allOf block down to the end of the binding, after the property
> > definitions.
>  
> Sure, I'll take care of it in the next series
> > 
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: xlnx,axi-quad-spi-1.00.a
> > > +    then:
> > > +      properties:
> > > +        clock-names:
> > > +          items:
> > > +            - const: s_axi_aclk
> > > +            - const: ext_spi_clk
> > 
> > These are all clocks, there should be no need to have "clk" in the names.
> 
> These are the names exported by the IP and used by the DTG.

So? This is a binding, not a verilog file.

> > > +
> > > +    else:
> > > +      properties:
> > > +        clock-names:
> > > +          items:
> > > +            - const: s_axi4_aclk
> > > +            - const: ext_spi_clk
> > > +
> > >  properties:
> > >    compatible:
> > >      enum:
> > > @@ -25,6 +44,12 @@ properties:
> > >    interrupts:
> > >      maxItems: 1
> > >
> > > +  clocks:
> > > +    maxItems: 2
> > > +
> > > +  clock-names:
> > > +    maxItems: 2
> > > +
> > >    xlnx,num-ss-bits:
> > >      description: Number of chip selects used.
> > >      minimum: 1
> > > @@ -39,6 +64,8 @@ required:
> > >    - compatible
> > >    - reg
> > >    - interrupts
> > > +  - clocks
> > > +  - clock-names
> > 
> > New required properties are an ABI break, where is the driver patch that makes use
> > of these clocks?
> 
> Alright, I will remove these from the required properties to avoid 
> breaking the ABI. We're working on the driver patch and will send it once 
> it's ready.

What changed to make the clocks needed now? It's possible that making
them required is the correct thing to do, so breaking the ABI would be
justified (provided the driver can still handle there being no clocks).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]