| Message ID | 87y0w21a4h.wl-kuninori.morimoto.gx@renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: add Renesas MSIOF sound driver | expand |
Hi Geert Thank you for your review > > + # "MSIOF-SPI" specific > > + - if: > > + properties: > > + $nodename: > > + pattern: '^spi@' > > This condition does not match what you wrote in the cover letter: > the controller is used in I2S mode when a port(s) subnode is present, > and in SPI mode when no port(s) subnode is present. > > > + then: > > + allOf: > > + - $ref: spi-controller.yaml# > > Documentation/devicetree/bindings/spi/spi-controller.yaml indeed > requires that the node-name matches "^spi(@.*|-([0-9]|[1-9][0-9]+))?$". > The controller's node is located in the SoC-specific .dtsi, where its > intended use case is not yet known, and its node name cannot easily be > overridden in the board .dts that specifies the use case. Hence the > node name must always be "spi" (and cannot be e.g. "serial-engine"). > Let's hope there is no other use case for MSIOF that requires using > a different node name... Hmm... OK So what we can do is keep spi@xxx node name, and check whether it has Of-Graph, and select spi-controller.yaml Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Geert (as Renesas SoC Maintainer), Rob (as DT Maintainer), Mark (as SPI Maintainer) > > > + # "MSIOF-SPI" specific > > > + - if: > > > + properties: > > > + $nodename: > > > + pattern: '^spi@' > > > > This condition does not match what you wrote in the cover letter: > > the controller is used in I2S mode when a port(s) subnode is present, > > and in SPI mode when no port(s) subnode is present. > > > > > + then: > > > + allOf: > > > + - $ref: spi-controller.yaml# > > > > Documentation/devicetree/bindings/spi/spi-controller.yaml indeed > > requires that the node-name matches "^spi(@.*|-([0-9]|[1-9][0-9]+))?$". > > The controller's node is located in the SoC-specific .dtsi, where its > > intended use case is not yet known, and its node name cannot easily be > > overridden in the board .dts that specifies the use case. Hence the > > node name must always be "spi" (and cannot be e.g. "serial-engine"). > > Let's hope there is no other use case for MSIOF that requires using > > a different node name... Hmm... Now, MSIOF node has "spi@xxxx". SoC file indicates MSIOF-SPI as default, so it has below lines --- SoC file ---- msiof1: spi@xxxx { ... #address-cells = <1>; #size-cells = <0>; ... }; These are not needed for MSIOF-I2S, so removes these --- Board file ---- &msiof1 { ... /delete-property/#address-cells; /delete-property/#size-cells; ... }; Now, my dt-bindings doesn't load spi-controller.yaml (as sample), but I got [SoC file]: Warning (spi_bus_bridge): /soc/spi@xxxx: incorrect #address-cells for SPI bus also defined at [Board file] [SoC file]: Warning (spi_bus_bridge): /soc/spi@xxxx: incorrect #size-cells for SPI bus also defined at [Board file] MSIOF dt-bindings doesn't load spi-controller.yaml, but why I got "spi_bus_bridge" warning ?? I wonder dt compiler (?) automatically check "spi" node ? I have tryed some code, my expectation seems correct (In case of node name was "spi@xxx", I got many SPI related warnings even though I didn't load spi-controller). Best regards --- Kuninori Morimoto
Hi Morimoto-san, On Thu, 17 Apr 2025 at 01:52, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > > > > + # "MSIOF-SPI" specific > > > > + - if: > > > > + properties: > > > > + $nodename: > > > > + pattern: '^spi@' > > > > > > This condition does not match what you wrote in the cover letter: > > > the controller is used in I2S mode when a port(s) subnode is present, > > > and in SPI mode when no port(s) subnode is present. > > > > > > > + then: > > > > + allOf: > > > > + - $ref: spi-controller.yaml# > > > > > > Documentation/devicetree/bindings/spi/spi-controller.yaml indeed > > > requires that the node-name matches "^spi(@.*|-([0-9]|[1-9][0-9]+))?$". > > > The controller's node is located in the SoC-specific .dtsi, where its > > > intended use case is not yet known, and its node name cannot easily be > > > overridden in the board .dts that specifies the use case. Hence the > > > node name must always be "spi" (and cannot be e.g. "serial-engine"). > > > Let's hope there is no other use case for MSIOF that requires using > > > a different node name... > > Hmm... > > Now, MSIOF node has "spi@xxxx". > SoC file indicates MSIOF-SPI as default, so it has below lines > > --- SoC file ---- > msiof1: spi@xxxx { > ... > #address-cells = <1>; > #size-cells = <0>; > ... > }; > > These are not needed for MSIOF-I2S, so removes these > > --- Board file ---- > &msiof1 { > ... > /delete-property/#address-cells; > /delete-property/#size-cells; > ... > }; > > Now, my dt-bindings doesn't load spi-controller.yaml (as sample), but I got > > [SoC file]: Warning (spi_bus_bridge): /soc/spi@xxxx: incorrect #address-cells for SPI bus > also defined at [Board file] > [SoC file]: Warning (spi_bus_bridge): /soc/spi@xxxx: incorrect #size-cells for SPI bus > also defined at [Board file] > > MSIOF dt-bindings doesn't load spi-controller.yaml, but why I got "spi_bus_bridge" > warning ?? I wonder dt compiler (?) automatically check "spi" node ? > I have tryed some code, my expectation seems correct (In case of node name was "spi@xxx", > I got many SPI related warnings even though I didn't load spi-controller). These come from dtc, which makes its own assumptions: $ git grep spi_bus_bridge scripts/dtc/checks.c:static void check_spi_bus_bridge(struct check *c, struct dt_info *dti, struct node *node) scripts/dtc/checks.c:WARNING(spi_bus_bridge, check_spi_bus_bridge, NULL, &addr_size_cells); scripts/dtc/checks.c:WARNING(spi_bus_reg, check_spi_bus_reg, NULL, ®_format, &spi_bus_bridge); scripts/dtc/checks.c: &spi_bus_bridge, Perhaps we do need to extend the use of role-specifying properties like "interrupt-controller" (in Device Tree Specification v0.4 and in dt-schema) and the few others in Documentation/devicetree/bindings: gpio-controller mctp-controller msi-controller system-power-controller Gr{oetje,eeting}s, Geert
Hi Geert, Rob, Mark > > [SoC file]: Warning (spi_bus_bridge): /soc/spi@xxxx: incorrect #address-cells for SPI bus > > also defined at [Board file] > > [SoC file]: Warning (spi_bus_bridge): /soc/spi@xxxx: incorrect #size-cells for SPI bus > > also defined at [Board file] > > > > MSIOF dt-bindings doesn't load spi-controller.yaml, but why I got "spi_bus_bridge" > > warning ?? I wonder dt compiler (?) automatically check "spi" node ? > > I have tryed some code, my expectation seems correct (In case of node name was "spi@xxx", > > I got many SPI related warnings even though I didn't load spi-controller). > > These come from dtc, which makes its own assumptions: > > $ git grep spi_bus_bridge > scripts/dtc/checks.c:static void check_spi_bus_bridge(struct check > *c, struct dt_info *dti, struct node *node) > scripts/dtc/checks.c:WARNING(spi_bus_bridge, check_spi_bus_bridge, > NULL, &addr_size_cells); > scripts/dtc/checks.c:WARNING(spi_bus_reg, check_spi_bus_reg, NULL, > ®_format, &spi_bus_bridge); > scripts/dtc/checks.c: &spi_bus_bridge, > > Perhaps we do need to extend the use of role-specifying properties > like "interrupt-controller" (in Device Tree Specification v0.4 and in > dt-schema) and the few others in Documentation/devicetree/bindings: > > gpio-controller > mctp-controller > msi-controller > system-power-controller Hmm... but I'm not familiar with DT. Should I do it ?? Except from SPI warning, and focus to MSIOF-I2S, my patch itself is not so bad, right ? I will post v4 patch-set, with comment above. Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml b/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml index 49649fc3f95a..880bd854d196 100644 --- a/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml +++ b/Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml @@ -4,14 +4,11 @@ $id: http://devicetree.org/schemas/spi/renesas,sh-msiof.yaml# $schema: http://devicetree.org/meta-schemas/core.yaml# -title: Renesas MSIOF SPI controller +title: Renesas MSIOF SPI / I2S controller maintainers: - Geert Uytterhoeven <geert+renesas@glider.be> -allOf: - - $ref: spi-controller.yaml# - properties: compatible: oneOf: @@ -146,24 +143,38 @@ properties: $ref: /schemas/types.yaml#/definitions/uint32 default: 64 + # for MSIOF-I2S + port: + $ref: ../sound/audio-graph-port.yaml# + unevaluatedProperties: false + required: - compatible - reg - interrupts - clocks - power-domains - - '#address-cells' - - '#size-cells' - -if: - not: - properties: - compatible: - contains: - const: renesas,sh-mobile-msiof -then: - required: - - resets + +allOf: + # additional "required"" + - if: + not: + properties: + compatible: + contains: + const: renesas,sh-mobile-msiof + then: + required: + - resets + + # "MSIOF-SPI" specific + - if: + properties: + $nodename: + pattern: '^spi@' + then: + allOf: + - $ref: spi-controller.yaml# unevaluatedProperties: false
Renesas MSIOF (Clock-Synchronized Serial Interface with FIFO) can work as both SPI and I2S. MSIOF-I2S will use Audio Graph Card/Card2 driver which uses Of-Graph in DT. MSIOF-SPI/I2S are using same DT compatible properties. MSIOF-I2S uses Of-Graph for Audio-Graph-Card/Card2, MSIOF-SPI doesn't use Of-Graph. Adds schema for MSIOF-I2S (= Sound). Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> --- .../bindings/spi/renesas,sh-msiof.yaml | 43 ++++++++++++------- 1 file changed, 27 insertions(+), 16 deletions(-)