[v1,17/43] dt-bindings: spi: Add Cirrus EP93xx

  Add YAML bindings for ep93xx SoC SPI.

Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---

Notes:
    v0 -> v1:
    Krzysztof Kozlowski:
    - replaced maintainers
    - removed wildcards
    - use fallback compatible and list all possible compatibles
    - drop quotes in ref
    - dropped "clock-names"
    - dropped label
    - fix ident

 .../devicetree/bindings/spi/spi-ep9301.yaml   | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-ep9301.yaml
  

On 01/06/2023 07:34, Nikita Shubin wrote:
> Add YAML bindings for ep93xx SoC SPI.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
> 
> Notes:
>     v0 -> v1:
>     Krzysztof Kozlowski:
>     - replaced maintainers
>     - removed wildcards
>     - use fallback compatible and list all possible compatibles
>     - drop quotes in ref
>     - dropped "clock-names"
>     - dropped label
>     - fix ident
> 
>  .../devicetree/bindings/spi/spi-ep9301.yaml   | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/spi-ep9301.yaml
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-ep9301.yaml b/Documentation/devicetree/bindings/spi/spi-ep9301.yaml
> new file mode 100644
> index 000000000000..c363b25a3074
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-ep9301.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/spi-ep9301.yaml#

Filename based on compatible, so missing prefix, wrong order of name
components.

This applies everywhere, not to some files only. Applied to all your
bindings.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: EP93xx SoC SPI controller
> +
> +maintainers:
> +  - Alexander Sverdlin <alexander.sverdlin@gmail.com>
> +  - Nikita Shubin <nikita.shubin@maquefel.me>
> +
> +allOf:
> +  - $ref: spi-controller.yaml#
> +
> +properties:
> +  "#address-cells": true
> +  "#size-cells": true

Drop these two.

> +
> +  compatible:

Anyway, compatible is always first.

> +    oneOf:
> +      - const: cirrus,ep9301-spi
> +      - items:
> +          - enum:
> +              - cirrus,ep9302-spi
> +              - cirrus,ep9307-spi
> +              - cirrus,ep9312-spi
> +              - cirrus,ep9315-spi
> +          - const: cirrus,ep9301-spi
> +
> +  reg:
> +    items:
> +      - description: SPI registers region
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: SPI Controller reference clock source
> +
> +  cs-gpios: true

Drop, not needed.

> +
> +  cirrus,ep9301-use-dma:
> +    description: Flag indicating that the SPI should use dma
> +    type: boolean

In such case where are dmas? Unless you meant some internal dma
controller? In such case extend the description because now it just
duplicates property name.


> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/cirrus,ep93xx-clock.h>
> +    spi@808a0000 {
> +      compatible = "cirrus,ep9301-spi";
> +      reg = <0x808a0000 0x18>;
> +      interrupt-parent = <&vic1>;
> +      interrupts = <21>;
> +      clocks = <&syscon EP93XX_CLK_SPI>;
> +      cs-gpios = <&gpio5 2 0>;

Use proper gpio defines for flags.

> +      cirrus,ep9301-use-dma;
> +    };
> +
> +...

Best regards,
Krzysztof
  

On Thu, Jun 01, 2023 at 08:34:08AM +0300, Nikita Shubin wrote:

> +  cirrus,ep9301-use-dma:
> +    description: Flag indicating that the SPI should use dma
> +    type: boolean

My previous feedback on this property still applies.

Please don't ignore review comments, people are generally making them
for a reason and are likely to have the same concerns if issues remain
unaddressed.  Having to repeat the same comments can get repetitive and
make people question the value of time spent reviewing.  If you disagree
with the review comments that's fine but you need to reply and discuss
your concerns so that the reviewer can understand your decisions.
  

Hello Mark!

On Thu, 1 Jun 2023 12:17:27 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Thu, Jun 01, 2023 at 08:34:08AM +0300, Nikita Shubin wrote:
> 
> > +  cirrus,ep9301-use-dma:
> > +    description: Flag indicating that the SPI should use dma
> > +    type: boolean  
> 
> My previous feedback on this property still applies.
> 
> Please don't ignore review comments, people are generally making them
> for a reason and are likely to have the same concerns if issues remain
> unaddressed.  Having to repeat the same comments can get repetitive
> and make people question the value of time spent reviewing.  If you
> disagree with the review comments that's fine but you need to reply
> and discuss your concerns so that the reviewer can understand your
> decisions.

Sorry - that was totally unintentional, i was tinkering with spi and
got distracted on other part of this series (it's quite big for me,
first time tinkering with a series more than 5-6 patches).

> > +  cirrus,ep9301-use-dma:

The reason is that ep93xx DMA state is not quite device-tree ready at
this moment, and clients use it with the help of:

https://elixir.bootlin.com/linux/v6.4-rc4/source/include/linux/platform_data/dma-ep93xx.h

I was hoping to slip by without changing much in ep93xx DMA driver, so
i can deal with it later, especially seeing it's having some quirks
like:

https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/dma/ep93xx_dma.c#L471

And edb93xx and bk3 don't set use_dma with SPI for some reason.

I can move "use-dma" to module parameters, if this is acceptable.
  

On Thu, Jun 01, 2023 at 03:41:54PM +0300, Nikita Shubin wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Jun 01, 2023 at 08:34:08AM +0300, Nikita Shubin wrote:

> > > +  cirrus,ep9301-use-dma:
> > > +    description: Flag indicating that the SPI should use dma
> > > +    type: boolean  

> > My previous feedback on this property still applies.

> > > +  cirrus,ep9301-use-dma:

> The reason is that ep93xx DMA state is not quite device-tree ready at
> this moment, and clients use it with the help of:

> https://elixir.bootlin.com/linux/v6.4-rc4/source/include/linux/platform_data/dma-ep93xx.h

> I was hoping to slip by without changing much in ep93xx DMA driver, so

You're definign new ABI here, that's not a good thing to do for a
temporary workaround.

> I can move "use-dma" to module parameters, if this is acceptable.

That's less bad.  I guess you could also define the bindings for the DMA
controller so that the properties are there then instead of properly
using the DMA API in the clients just check to see if the DMA properties
are present and then proceed accordingly?
  

On Thu, 1 Jun 2023 13:55:03 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Thu, Jun 01, 2023 at 03:41:54PM +0300, Nikita Shubin wrote:
> > Mark Brown <broonie@kernel.org> wrote:  
> > > On Thu, Jun 01, 2023 at 08:34:08AM +0300, Nikita Shubin wrote:  
> 
> > > > +  cirrus,ep9301-use-dma:
> > > > +    description: Flag indicating that the SPI should use dma
> > > > +    type: boolean    
> 
> > > My previous feedback on this property still applies.  
> 
> > > > +  cirrus,ep9301-use-dma:  
> 
> > The reason is that ep93xx DMA state is not quite device-tree ready
> > at this moment, and clients use it with the help of:  
> 
> > https://elixir.bootlin.com/linux/v6.4-rc4/source/include/linux/platform_data/dma-ep93xx.h
> >  
> 
> > I was hoping to slip by without changing much in ep93xx DMA driver,
> > so  
> 
> You're definign new ABI here, that's not a good thing to do for a
> temporary workaround.
> 
> > I can move "use-dma" to module parameters, if this is acceptable.  
> 
> That's less bad.  I guess you could also define the bindings for the
> DMA controller so that the properties are there then instead of
> properly using the DMA API in the clients just check to see if the
> DMA properties are present and then proceed accordingly?

This sounds like a way to go. Thank you, Mark!