| Message ID | 20230601053546.9574-18-nikita.shubin@maquefel.me |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | ep93xx device tree conversion | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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!
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# +$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 + + compatible: + 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 + + cirrus,ep9301-use-dma: + description: Flag indicating that the SPI should use dma + type: boolean + +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>; + cirrus,ep9301-use-dma; + }; + +...
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