| Message ID | 20200227130323.15327-1-geert+renesas@glider.be |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | spi: dt-bindings: spi-controller: Fix #address-cells for slave mode | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | "total: 0 errors, 1 warnings, 27 lines checked" |
| robh/dt-meta-schema | success |
Hi! On Thu, Feb 27, 2020 at 02:03:23PM +0100, Geert Uytterhoeven wrote: > Currently, the DT bindings for an SPI controller specify that > "#address-cells" must be fixed to one. However, that applies to an SPI > controller in master mode only. When running in SPI slave mode, > "#address-cells" should be zero. > > Fix this making the value of "#address-cells" dependent on the presence > of "spi-slave". > > Fixes: 0a1b929356830257 ("spi: Add YAML schemas for the generic SPI options") > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > Is this the right way to specify this? I couldn't find an example, so > this needed some trial-and-error. > > As of dtc commit 403cc79f06a135ae ("checks: Update SPI bus check for > 'spi-slave'") and Linux commit c2e7075ca8303631 ("scripts/dtc: Update to > upstream version v1.4.7-57-gf267e674d145"), dtc knows about SPI slave. > > However, when using "#address-cells = <0>" with W=1: > > Warning (avoid_unnecessary_addr_size): /soc/spi@e6e10000: unnecessary #address-cells/#size-cells without "ranges" or child "reg" property > > Just removing #address-cells (using /delete-property/ in the board DTS) > to fix this warning causes: > > Warning (spi_bus_bridge): /soc/spi@e6e10000: incorrect #address-cells for SPI bus > > as spi_bus_bridge() uses node_addr_cells(), which defaults to 2 (due to > dtc's ppc64 heritage? But node_size_cells() defaults to 1, not 2?). > > How should this be fixed? > Should "#address-cells = <0>" be left out or not? > Should node_{addr,size}_cells() in dtc default to zero? > > Thanks! > --- > .../devicetree/bindings/spi/spi-controller.yaml | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml > index 1e0ca6ccf64bbd0a..ba2646258b94980e 100644 > --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml > +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml > @@ -20,7 +20,8 @@ properties: > pattern: "^spi(@.*|-[0-9a-f])*$" > > "#address-cells": > - const: 1 > + minimum: 0 > + maximum: 1 Nit: This would be better described as an enum: [0, 1] > "#size-cells": > const: 0 > @@ -52,6 +53,18 @@ properties: > description: > The SPI controller acts as a slave, instead of a master. > > +if: > + required: > + [ spi-slave ] Nit: Usually, that notation is when you would do it on the same line, if you want to go to a new line, it would make more sense to use - Maxime
Hi Maxime, On Thu, Feb 27, 2020 at 2:09 PM Maxime Ripard <maxime@cerno.tech> wrote: > On Thu, Feb 27, 2020 at 02:03:23PM +0100, Geert Uytterhoeven wrote: > > Currently, the DT bindings for an SPI controller specify that > > "#address-cells" must be fixed to one. However, that applies to an SPI > > controller in master mode only. When running in SPI slave mode, > > "#address-cells" should be zero. > > > > Fix this making the value of "#address-cells" dependent on the presence > > of "spi-slave". > > > > Fixes: 0a1b929356830257 ("spi: Add YAML schemas for the generic SPI options") > > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml > > +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml > > @@ -52,6 +53,18 @@ properties: > > description: > > The SPI controller acts as a slave, instead of a master. > > > > +if: > > + required: > > + [ spi-slave ] > > Nit: Usually, that notation is when you would do it on the same line, > if you want to go to a new line, it would make more sense to use - Sorry, being a poor yaml-by-example programmer, I don't understand what you mean. Which part do you refer to by "that notation"? Thanks! Gr{oetje,eeting}s, Geert
On Thu, Feb 27, 2020 at 02:19:08PM +0100, Geert Uytterhoeven wrote: > Hi Maxime, > > On Thu, Feb 27, 2020 at 2:09 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Thu, Feb 27, 2020 at 02:03:23PM +0100, Geert Uytterhoeven wrote: > > > Currently, the DT bindings for an SPI controller specify that > > > "#address-cells" must be fixed to one. However, that applies to an SPI > > > controller in master mode only. When running in SPI slave mode, > > > "#address-cells" should be zero. > > > > > > Fix this making the value of "#address-cells" dependent on the presence > > > of "spi-slave". > > > > > > Fixes: 0a1b929356830257 ("spi: Add YAML schemas for the generic SPI options") > > > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml > > > +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml > > > > @@ -52,6 +53,18 @@ properties: > > > description: > > > The SPI controller acts as a slave, instead of a master. > > > > > > +if: > > > + required: > > > + [ spi-slave ] > > > > Nit: Usually, that notation is when you would do it on the same line, > > if you want to go to a new line, it would make more sense to use - > > Sorry, being a poor yaml-by-example programmer, I don't understand what > you mean. Which part do you refer to by "that notation"? I meant that usually the [] notation is used over a single line. If you want to have a list over multiple lines, usually you would have required: - spi-slave Maxime
Hi Maxime, On Thu, Feb 27, 2020 at 2:51 PM Maxime Ripard <maxime@cerno.tech> wrote: > On Thu, Feb 27, 2020 at 02:19:08PM +0100, Geert Uytterhoeven wrote: > > On Thu, Feb 27, 2020 at 2:09 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > On Thu, Feb 27, 2020 at 02:03:23PM +0100, Geert Uytterhoeven wrote: > > > > Currently, the DT bindings for an SPI controller specify that > > > > "#address-cells" must be fixed to one. However, that applies to an SPI > > > > controller in master mode only. When running in SPI slave mode, > > > > "#address-cells" should be zero. > > > > > > > > Fix this making the value of "#address-cells" dependent on the presence > > > > of "spi-slave". > > > > > > > > Fixes: 0a1b929356830257 ("spi: Add YAML schemas for the generic SPI options") > > > > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml > > > > +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml > > > > > > @@ -52,6 +53,18 @@ properties: > > > > description: > > > > The SPI controller acts as a slave, instead of a master. > > > > > > > > +if: > > > > + required: > > > > + [ spi-slave ] > > > > > > Nit: Usually, that notation is when you would do it on the same line, > > > if you want to go to a new line, it would make more sense to use - > > > > Sorry, being a poor yaml-by-example programmer, I don't understand what > > you mean. Which part do you refer to by "that notation"? > > I meant that usually the [] notation is used over a single line. > > If you want to have a list over multiple lines, usually you would have > > required: > - spi-slave IC. So I can also write: if: required: [ spi-slave ] else: ... which is even shorter ;-) Both seem to work fine. Gr{oetje,eeting}s, Geert
On Thu, Feb 27, 2020 at 03:05:56PM +0100, Geert Uytterhoeven wrote: > Hi Maxime, > > On Thu, Feb 27, 2020 at 2:51 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Thu, Feb 27, 2020 at 02:19:08PM +0100, Geert Uytterhoeven wrote: > > > On Thu, Feb 27, 2020 at 2:09 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Thu, Feb 27, 2020 at 02:03:23PM +0100, Geert Uytterhoeven wrote: > > > > > Currently, the DT bindings for an SPI controller specify that > > > > > "#address-cells" must be fixed to one. However, that applies to an SPI > > > > > controller in master mode only. When running in SPI slave mode, > > > > > "#address-cells" should be zero. > > > > > > > > > > Fix this making the value of "#address-cells" dependent on the presence > > > > > of "spi-slave". > > > > > > > > > > Fixes: 0a1b929356830257 ("spi: Add YAML schemas for the generic SPI options") > > > > > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > > --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml > > > > > +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml > > > > > > > > @@ -52,6 +53,18 @@ properties: > > > > > description: > > > > > The SPI controller acts as a slave, instead of a master. > > > > > > > > > > +if: > > > > > + required: > > > > > + [ spi-slave ] > > > > > > > > Nit: Usually, that notation is when you would do it on the same line, > > > > if you want to go to a new line, it would make more sense to use - > > > > > > Sorry, being a poor yaml-by-example programmer, I don't understand what > > > you mean. Which part do you refer to by "that notation"? > > > > I meant that usually the [] notation is used over a single line. > > > > If you want to have a list over multiple lines, usually you would have > > > > required: > > - spi-slave > > IC. So I can also write: > > if: > required: [ spi-slave ] > else: > ... > > which is even shorter ;-) > > Both seem to work fine. The rest of the file uses the multi-line syntax, so I wouldn't do it for consistency, but yeah, they are equivalent. Maxime
diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml index 1e0ca6ccf64bbd0a..ba2646258b94980e 100644 --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml @@ -20,7 +20,8 @@ properties: pattern: "^spi(@.*|-[0-9a-f])*$" "#address-cells": - const: 1 + minimum: 0 + maximum: 1 "#size-cells": const: 0 @@ -52,6 +53,18 @@ properties: description: The SPI controller acts as a slave, instead of a master. +if: + required: + [ spi-slave ] +then: + properties: + "#address-cells": + const: 0 +else: + properties: + "#address-cells": + const: 1 + patternProperties: "^slave$": type: object
Currently, the DT bindings for an SPI controller specify that "#address-cells" must be fixed to one. However, that applies to an SPI controller in master mode only. When running in SPI slave mode, "#address-cells" should be zero. Fix this making the value of "#address-cells" dependent on the presence of "spi-slave". Fixes: 0a1b929356830257 ("spi: Add YAML schemas for the generic SPI options") Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- Is this the right way to specify this? I couldn't find an example, so this needed some trial-and-error. As of dtc commit 403cc79f06a135ae ("checks: Update SPI bus check for 'spi-slave'") and Linux commit c2e7075ca8303631 ("scripts/dtc: Update to upstream version v1.4.7-57-gf267e674d145"), dtc knows about SPI slave. However, when using "#address-cells = <0>" with W=1: Warning (avoid_unnecessary_addr_size): /soc/spi@e6e10000: unnecessary #address-cells/#size-cells without "ranges" or child "reg" property Just removing #address-cells (using /delete-property/ in the board DTS) to fix this warning causes: Warning (spi_bus_bridge): /soc/spi@e6e10000: incorrect #address-cells for SPI bus as spi_bus_bridge() uses node_addr_cells(), which defaults to 2 (due to dtc's ppc64 heritage? But node_size_cells() defaults to 1, not 2?). How should this be fixed? Should "#address-cells = <0>" be left out or not? Should node_{addr,size}_cells() in dtc default to zero? Thanks! --- .../devicetree/bindings/spi/spi-controller.yaml | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)