| Message ID | 20240522075245.388-1-bavishimithil@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5] ASoC: dt-bindings: omap-mcpdm: Convert to DT schema | expand |
On 22/05/2024 09:52, Mighty wrote: > From: Mithil Bavishi <bavishimithil@gmail.com> > > Convert the OMAP4+ McPDM bindings to DT schema. > > Signed-off-by: Mithil Bavishi <bavishimithil@gmail.com> > --- > Changelog v5: > - Add imports for constants > - Add desc to ti,hwmods You are not making it easier for us to review: ==== b4 diff '<20240522075245.388-1-bavishimithil@gmail.com>' Grabbing thread from lore.kernel.org/all/20240522075245.388-1-bavishimithil@gmail.com/t.mbox.gz Checking for older revisions Grabbing search results from lore.kernel.org Added from v4: 2 patches --- Analyzing 15 messages in the thread WARNING: duplicate messages found at index 1 Subject 1: ASoC: dt-bindings: omap-mcpdm: Convert to DT schema Subject 2: ASoC: dt-bindings: omap-mcpdm: Convert to DT schema 2 is not a reply... assume additional patch Preparing fake-am for v4: ASoC: dt-bindings: omap-mcpdm: Convert to DT schema ERROR: Could not fake-am version v4 --- Could not create fake-am range for lower series v4 ==== Looks good, but let's wait few hours to see if bot is happy as well. Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
> You are not making it easier for us to review: > ==== > b4 diff '<20240522075245.388-1-bavishimithil@gmail.com>' > Grabbing thread from > lore.kernel.org/all/20240522075245.388-1-bavishimithil@gmail.com/t.mbox.gz > Checking for older revisions > Grabbing search results from lore.kernel.org > Added from v4: 2 patches > --- > Analyzing 15 messages in the thread > WARNING: duplicate messages found at index 1 > Subject 1: ASoC: dt-bindings: omap-mcpdm: Convert to DT schema > Subject 2: ASoC: dt-bindings: omap-mcpdm: Convert to DT schema > 2 is not a reply... assume additional patch > Preparing fake-am for v4: ASoC: dt-bindings: omap-mcpdm: Convert to DT > schema > ERROR: Could not fake-am version v4 > --- > Could not create fake-am range for lower series v4 > > ==== > Hey, sorry about it, its my first time with lkml, could you explain what seems to be the issue here?
Hi, On 22/05/2024 10:52, Mighty wrote: > From: Mithil Bavishi <bavishimithil@gmail.com> > > Convert the OMAP4+ McPDM bindings to DT schema. > > Signed-off-by: Mithil Bavishi <bavishimithil@gmail.com> > --- > Changelog v5: > - Add imports for constants > - Add desc to ti,hwmods > > .../devicetree/bindings/sound/omap-mcpdm.txt | 30 --------- > .../bindings/sound/ti,omap4-mcpdm.yaml | 61 +++++++++++++++++++ > 2 files changed, 61 insertions(+), 30 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/sound/omap-mcpdm.txt > create mode 100644 Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml > > diff --git a/Documentation/devicetree/bindings/sound/omap-mcpdm.txt b/Documentation/devicetree/bindings/sound/omap-mcpdm.txt > deleted file mode 100644 > index ff98a0cb5..000000000 > --- a/Documentation/devicetree/bindings/sound/omap-mcpdm.txt > +++ /dev/null > @@ -1,30 +0,0 @@ > -* Texas Instruments OMAP4+ McPDM > - > -Required properties: > -- compatible: "ti,omap4-mcpdm" > -- reg: Register location and size as an array: > - <MPU access base address, size>, > - <L3 interconnect address, size>; > -- interrupts: Interrupt number for McPDM > -- ti,hwmods: Name of the hwmod associated to the McPDM > -- clocks: phandle for the pdmclk provider, likely <&twl6040> > -- clock-names: Must be "pdmclk" > - > -Example: > - > -mcpdm: mcpdm@40132000 { > - compatible = "ti,omap4-mcpdm"; > - reg = <0x40132000 0x7f>, /* MPU private access */ > - <0x49032000 0x7f>; /* L3 Interconnect */ > - interrupts = <0 112 0x4>; > - interrupt-parent = <&gic>; > - ti,hwmods = "mcpdm"; > -}; > - > -In board DTS file the pdmclk needs to be added: > - > -&mcpdm { > - clocks = <&twl6040>; > - clock-names = "pdmclk"; > - status = "okay"; > -}; > diff --git a/Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml b/Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml > new file mode 100644 > index 000000000..966406078 > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml > @@ -0,0 +1,61 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/ti,omap4-mcpdm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: OMAP McPDM > + > +maintainers: > + - Misael Lopez Cruz <misael.lopez@ti.com> > + > +description: > + OMAP ALSA SoC DAI driver using McPDM port used by TWL6040 > + > +properties: > + compatible: > + const: ti,omap4-mcpdm > + > + reg: > + items: > + - description: MPU access base address > + - description: L3 interconnect address > + > + interrupts: > + maxItems: 1 > + > + ti,hwmods: > + $ref: /schemas/types.yaml#/definitions/string > + enum: [mcpdm] > + description: Name of the hwmod associated to the McPDM, likely "mcpdm" > + > + clocks: > + maxItems: 1 > + > + clock-names: > + items: > + - const: pdmclk > + > +required: > + - compatible > + - reg > + - interrupts > + - ti,hwmods > + - clocks > + - clock-names > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + pdm@40132000 { The original label and name is preferred to be used. > + compatible = "ti,omap4-mcpdm"; > + reg = <0x40132000 0x7f>, /* MPU private access */ > + <0x49032000 0x7f>; /* L3 Interconnect */ > + interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-parent = <&gic>; > + ti,hwmods = "mcpdm"; > + clocks = <&twl6040>; > + clock-names = "pdmclk"; The clocks cannot be added at the time when the node is defined, it is board specific. This way you imply that it is OK to have it in main dtsi file. It is not. > + };
On 22/05/2024 15:46, Mithil wrote: >> You are not making it easier for us to review: >> ==== >> b4 diff '<20240522075245.388-1-bavishimithil@gmail.com>' >> Grabbing thread from >> lore.kernel.org/all/20240522075245.388-1-bavishimithil@gmail.com/t.mbox.gz >> Checking for older revisions >> Grabbing search results from lore.kernel.org >> Added from v4: 2 patches >> --- >> Analyzing 15 messages in the thread >> WARNING: duplicate messages found at index 1 >> Subject 1: ASoC: dt-bindings: omap-mcpdm: Convert to DT schema >> Subject 2: ASoC: dt-bindings: omap-mcpdm: Convert to DT schema >> 2 is not a reply... assume additional patch >> Preparing fake-am for v4: ASoC: dt-bindings: omap-mcpdm: Convert to DT >> schema >> ERROR: Could not fake-am version v4 >> --- >> Could not create fake-am range for lower series v4 >> >> ==== >> > Hey, sorry about it, its my first time with lkml, could you explain > what seems to be the issue here? You sent multiple same versions, I think more than one v4. You can try by yourself - does b4 work on this patchset? Best regards, Krzysztof
On 22/05/2024 15:56, Péter Ujfalusi wrote: >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + - ti,hwmods >> + - clocks >> + - clock-names >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + pdm@40132000 { > > The original label and name is preferred to be used. Label is not used here. About node name, not: Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > >> + compatible = "ti,omap4-mcpdm"; >> + reg = <0x40132000 0x7f>, /* MPU private access */ >> + <0x49032000 0x7f>; /* L3 Interconnect */ >> + interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-parent = <&gic>; >> + ti,hwmods = "mcpdm"; >> + clocks = <&twl6040>; >> + clock-names = "pdmclk"; > > The clocks cannot be added at the time when the node is defined, it is > board specific. This way you imply that it is OK to have it in main dtsi > file. It is not. Wait, what? That's example and pretty standard. Example should be complete. This is not an exceptional binding. Best regards, Krzysztof
On Wed, May 22, 2024 at 04:56:11PM +0300, Péter Ujfalusi wrote: > Hi, > > On 22/05/2024 10:52, Mighty wrote: > > From: Mithil Bavishi <bavishimithil@gmail.com> > > > > Convert the OMAP4+ McPDM bindings to DT schema. > > > > Signed-off-by: Mithil Bavishi <bavishimithil@gmail.com> > > --- > > Changelog v5: > > - Add imports for constants > > - Add desc to ti,hwmods > > > > .../devicetree/bindings/sound/omap-mcpdm.txt | 30 --------- > > .../bindings/sound/ti,omap4-mcpdm.yaml | 61 +++++++++++++++++++ > > 2 files changed, 61 insertions(+), 30 deletions(-) > > delete mode 100644 Documentation/devicetree/bindings/sound/omap-mcpdm.txt > > create mode 100644 Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml > > > > diff --git a/Documentation/devicetree/bindings/sound/omap-mcpdm.txt b/Documentation/devicetree/bindings/sound/omap-mcpdm.txt > > deleted file mode 100644 > > index ff98a0cb5..000000000 > > --- a/Documentation/devicetree/bindings/sound/omap-mcpdm.txt > > +++ /dev/null > > @@ -1,30 +0,0 @@ > > -* Texas Instruments OMAP4+ McPDM > > - > > -Required properties: > > -- compatible: "ti,omap4-mcpdm" > > -- reg: Register location and size as an array: > > - <MPU access base address, size>, > > - <L3 interconnect address, size>; > > -- interrupts: Interrupt number for McPDM > > -- ti,hwmods: Name of the hwmod associated to the McPDM > > -- clocks: phandle for the pdmclk provider, likely <&twl6040> > > -- clock-names: Must be "pdmclk" > > - > > -Example: > > - > > -mcpdm: mcpdm@40132000 { > > - compatible = "ti,omap4-mcpdm"; > > - reg = <0x40132000 0x7f>, /* MPU private access */ > > - <0x49032000 0x7f>; /* L3 Interconnect */ > > - interrupts = <0 112 0x4>; > > - interrupt-parent = <&gic>; > > - ti,hwmods = "mcpdm"; > > -}; > > - > > -In board DTS file the pdmclk needs to be added: > > - > > -&mcpdm { > > - clocks = <&twl6040>; > > - clock-names = "pdmclk"; > > - status = "okay"; > > -}; > > diff --git a/Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml b/Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml > > new file mode 100644 > > index 000000000..966406078 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml > > @@ -0,0 +1,61 @@ > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/sound/ti,omap4-mcpdm.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: OMAP McPDM > > + > > +maintainers: > > + - Misael Lopez Cruz <misael.lopez@ti.com> > > + > > +description: > > + OMAP ALSA SoC DAI driver using McPDM port used by TWL6040 > > + > > +properties: > > + compatible: > > + const: ti,omap4-mcpdm > > + > > + reg: > > + items: > > + - description: MPU access base address > > + - description: L3 interconnect address > > + > > + interrupts: > > + maxItems: 1 > > + > > + ti,hwmods: > > + $ref: /schemas/types.yaml#/definitions/string > > + enum: [mcpdm] > > + description: Name of the hwmod associated to the McPDM, likely "mcpdm" > > + > > + clocks: > > + maxItems: 1 > > + > > + clock-names: > > + items: > > + - const: pdmclk > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - ti,hwmods > > + - clocks > > + - clock-names > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + pdm@40132000 { > > The original label and name is preferred to be used. I imagine both were review comments. I can only imagine given the poor changelog. Unused labels in examples should be dropped. Node names should be generic. Though if we haven't defined the name in the spec or a schema, I don't care too much what is used. > > + compatible = "ti,omap4-mcpdm"; > > + reg = <0x40132000 0x7f>, /* MPU private access */ > > + <0x49032000 0x7f>; /* L3 Interconnect */ > > + interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-parent = <&gic>; > > + ti,hwmods = "mcpdm"; > > + clocks = <&twl6040>; > > + clock-names = "pdmclk"; > > The clocks cannot be added at the time when the node is defined, it is > board specific. This way you imply that it is OK to have it in main dtsi > file. It is not. That's a .dtsi structuring decision which is irrelevant to the binding. Rob
On 22/05/2024 17:22, Rob Herring wrote: > On Wed, May 22, 2024 at 04:56:11PM +0300, Péter Ujfalusi wrote: >> Hi, >> >> On 22/05/2024 10:52, Mighty wrote: >>> From: Mithil Bavishi <bavishimithil@gmail.com> >>> >>> Convert the OMAP4+ McPDM bindings to DT schema. >>> >>> Signed-off-by: Mithil Bavishi <bavishimithil@gmail.com> >>> --- >>> Changelog v5: >>> - Add imports for constants >>> - Add desc to ti,hwmods >>> >>> .../devicetree/bindings/sound/omap-mcpdm.txt | 30 --------- >>> .../bindings/sound/ti,omap4-mcpdm.yaml | 61 +++++++++++++++++++ >>> 2 files changed, 61 insertions(+), 30 deletions(-) >>> delete mode 100644 Documentation/devicetree/bindings/sound/omap-mcpdm.txt >>> create mode 100644 Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/sound/omap-mcpdm.txt b/Documentation/devicetree/bindings/sound/omap-mcpdm.txt >>> deleted file mode 100644 >>> index ff98a0cb5..000000000 >>> --- a/Documentation/devicetree/bindings/sound/omap-mcpdm.txt >>> +++ /dev/null >>> @@ -1,30 +0,0 @@ >>> -* Texas Instruments OMAP4+ McPDM >>> - >>> -Required properties: >>> -- compatible: "ti,omap4-mcpdm" >>> -- reg: Register location and size as an array: >>> - <MPU access base address, size>, >>> - <L3 interconnect address, size>; >>> -- interrupts: Interrupt number for McPDM >>> -- ti,hwmods: Name of the hwmod associated to the McPDM >>> -- clocks: phandle for the pdmclk provider, likely <&twl6040> >>> -- clock-names: Must be "pdmclk" >>> - >>> -Example: >>> - >>> -mcpdm: mcpdm@40132000 { >>> - compatible = "ti,omap4-mcpdm"; >>> - reg = <0x40132000 0x7f>, /* MPU private access */ >>> - <0x49032000 0x7f>; /* L3 Interconnect */ >>> - interrupts = <0 112 0x4>; >>> - interrupt-parent = <&gic>; >>> - ti,hwmods = "mcpdm"; >>> -}; >>> - >>> -In board DTS file the pdmclk needs to be added: >>> - >>> -&mcpdm { >>> - clocks = <&twl6040>; >>> - clock-names = "pdmclk"; >>> - status = "okay"; >>> -}; >>> diff --git a/Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml b/Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml >>> new file mode 100644 >>> index 000000000..966406078 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml >>> @@ -0,0 +1,61 @@ >>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/sound/ti,omap4-mcpdm.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: OMAP McPDM >>> + >>> +maintainers: >>> + - Misael Lopez Cruz <misael.lopez@ti.com> >>> + >>> +description: >>> + OMAP ALSA SoC DAI driver using McPDM port used by TWL6040 >>> + >>> +properties: >>> + compatible: >>> + const: ti,omap4-mcpdm >>> + >>> + reg: >>> + items: >>> + - description: MPU access base address >>> + - description: L3 interconnect address >>> + >>> + interrupts: >>> + maxItems: 1 >>> + >>> + ti,hwmods: >>> + $ref: /schemas/types.yaml#/definitions/string >>> + enum: [mcpdm] >>> + description: Name of the hwmod associated to the McPDM, likely "mcpdm" >>> + >>> + clocks: >>> + maxItems: 1 >>> + >>> + clock-names: >>> + items: >>> + - const: pdmclk >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - interrupts >>> + - ti,hwmods >>> + - clocks >>> + - clock-names >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/interrupt-controller/arm-gic.h> >>> + pdm@40132000 { >> >> The original label and name is preferred to be used. > > I imagine both were review comments. I can only imagine given the poor > changelog. > > Unused labels in examples should be dropped. > > Node names should be generic. Though if we haven't defined the name in > the spec or a schema, I don't care too much what is used. McPDM uses it's own flavor of PDM, it is not the normal PDM as we all know, I don't know what other generic name can be used. > >>> + compatible = "ti,omap4-mcpdm"; >>> + reg = <0x40132000 0x7f>, /* MPU private access */ >>> + <0x49032000 0x7f>; /* L3 Interconnect */ >>> + interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>; >>> + interrupt-parent = <&gic>; >>> + ti,hwmods = "mcpdm"; >>> + clocks = <&twl6040>; >>> + clock-names = "pdmclk"; >> >> The clocks cannot be added at the time when the node is defined, it is >> board specific. This way you imply that it is OK to have it in main dtsi >> file. It is not. > > That's a .dtsi structuring decision which is irrelevant to the > binding. I see, but then the dmas/dma-names should also be in here somewhere, yes it was not part of the original binding doc, but it is mandatory. Looking at the code and DT files, the reg-names also mandatory. > > Rob
On 22/05/2024 17:16, Krzysztof Kozlowski wrote: > On 22/05/2024 15:56, Péter Ujfalusi wrote: >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - interrupts >>> + - ti,hwmods >>> + - clocks >>> + - clock-names >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/interrupt-controller/arm-gic.h> >>> + pdm@40132000 { >> >> The original label and name is preferred to be used. > > Label is not used here. > > About node name, not: > > Node names should be generic. See also an explanation and list of > examples (not exhaustive) in DT specification: > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > > >> >>> + compatible = "ti,omap4-mcpdm"; >>> + reg = <0x40132000 0x7f>, /* MPU private access */ >>> + <0x49032000 0x7f>; /* L3 Interconnect */ >>> + interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>; >>> + interrupt-parent = <&gic>; >>> + ti,hwmods = "mcpdm"; >>> + clocks = <&twl6040>; >>> + clock-names = "pdmclk"; >> >> The clocks cannot be added at the time when the node is defined, it is >> board specific. This way you imply that it is OK to have it in main dtsi >> file. It is not. > > Wait, what? That's example and pretty standard. Example should be > complete. This is not an exceptional binding. The fclk for the McPDM is coming from external source, and the McPDM is designed in pair with twl6040/6041, there were plan for other codecs to support the McPDM protocol and in those cases the clock would come from the connected codec. The example (as the original binding was bit rot) is missing reg-names, dmas and dma-names to be complete. > > Best regards, > Krzysztof >
On 22/05/2024 16:43, Péter Ujfalusi wrote: >>> >>>> + compatible = "ti,omap4-mcpdm"; >>>> + reg = <0x40132000 0x7f>, /* MPU private access */ >>>> + <0x49032000 0x7f>; /* L3 Interconnect */ >>>> + interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>; >>>> + interrupt-parent = <&gic>; >>>> + ti,hwmods = "mcpdm"; >>>> + clocks = <&twl6040>; >>>> + clock-names = "pdmclk"; >>> >>> The clocks cannot be added at the time when the node is defined, it is >>> board specific. This way you imply that it is OK to have it in main dtsi >>> file. It is not. >> >> Wait, what? That's example and pretty standard. Example should be >> complete. This is not an exceptional binding. > > The fclk for the McPDM is coming from external source, and the McPDM is > designed in pair with twl6040/6041, there were plan for other codecs to > support the McPDM protocol and in those cases the clock would come from > the connected codec. > > The example (as the original binding was bit rot) is missing reg-names, > dmas and dma-names to be complete. None of these properties are allowed by the binding and during these five/six revisions of the patchset no one raised missing properties. I assume the DTS was validated with the binding. Isn't the case here? Best regards, Krzysztof
On 22/05/2024 18:22, Krzysztof Kozlowski wrote: > On 22/05/2024 16:43, Péter Ujfalusi wrote: >>>> >>>>> + compatible = "ti,omap4-mcpdm"; >>>>> + reg = <0x40132000 0x7f>, /* MPU private access */ >>>>> + <0x49032000 0x7f>; /* L3 Interconnect */ >>>>> + interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>; >>>>> + interrupt-parent = <&gic>; >>>>> + ti,hwmods = "mcpdm"; >>>>> + clocks = <&twl6040>; >>>>> + clock-names = "pdmclk"; >>>> >>>> The clocks cannot be added at the time when the node is defined, it is >>>> board specific. This way you imply that it is OK to have it in main dtsi >>>> file. It is not. >>> >>> Wait, what? That's example and pretty standard. Example should be >>> complete. This is not an exceptional binding. >> >> The fclk for the McPDM is coming from external source, and the McPDM is >> designed in pair with twl6040/6041, there were plan for other codecs to >> support the McPDM protocol and in those cases the clock would come from >> the connected codec. >> >> The example (as the original binding was bit rot) is missing reg-names, >> dmas and dma-names to be complete. > > None of these properties are allowed by the binding and during these > five/six revisions of the patchset no one raised missing properties. I just by accident spotted this patch, I was not in Cc. The reg-names must be set to 'mpu' and 'dma' The dma-names should be 'up_link' and 'dn_link' These names go back for a long time (~2012) and have been mandatory ever since. Yes, the binding document was neglected pretty badly but when converting to yaml it has to be correct since that will have ripple effect on existing dts/dtsi files. > I assume the DTS was validated with the binding. Isn't the case here? > > Best regards, > Krzysztof >
On 22/05/2024 18:01, Péter Ujfalusi wrote: > > > On 22/05/2024 18:22, Krzysztof Kozlowski wrote: >> On 22/05/2024 16:43, Péter Ujfalusi wrote: >>>>> >>>>>> + compatible = "ti,omap4-mcpdm"; >>>>>> + reg = <0x40132000 0x7f>, /* MPU private access */ >>>>>> + <0x49032000 0x7f>; /* L3 Interconnect */ >>>>>> + interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>; >>>>>> + interrupt-parent = <&gic>; >>>>>> + ti,hwmods = "mcpdm"; >>>>>> + clocks = <&twl6040>; >>>>>> + clock-names = "pdmclk"; >>>>> >>>>> The clocks cannot be added at the time when the node is defined, it is >>>>> board specific. This way you imply that it is OK to have it in main dtsi >>>>> file. It is not. >>>> >>>> Wait, what? That's example and pretty standard. Example should be >>>> complete. This is not an exceptional binding. >>> >>> The fclk for the McPDM is coming from external source, and the McPDM is >>> designed in pair with twl6040/6041, there were plan for other codecs to >>> support the McPDM protocol and in those cases the clock would come from >>> the connected codec. >>> >>> The example (as the original binding was bit rot) is missing reg-names, >>> dmas and dma-names to be complete. >> >> None of these properties are allowed by the binding and during these >> five/six revisions of the patchset no one raised missing properties. > > I just by accident spotted this patch, I was not in Cc. > > The reg-names must be set to 'mpu' and 'dma' > The dma-names should be 'up_link' and 'dn_link' > > These names go back for a long time (~2012) and have been mandatory ever > since. > > Yes, the binding document was neglected pretty badly but when converting > to yaml it has to be correct since that will have ripple effect on > existing dts/dtsi files. Yep. And testing DTS should clearly show that conversion leads to incomplete binding. > >> I assume the DTS was validated with the binding. Isn't the case here? Mithil Bavishi, Are you sure you tested the DTS? Best regards, Krzysztof
> Yep. And testing DTS should clearly show that conversion leads to > incomplete binding. > > > > >> I assume the DTS was validated with the binding. Isn't the case here? > > Mithil Bavishi, > Are you sure you tested the DTS? dt_binding_check did not give me any errors. Yeah the example is different from how it is implemented in the kernel ie board specific (omap4, omap5 etc). Should the example be changed according to that dtsi then?
Something along the lines of,
mcpdm: mcpdm@0 {
compatible = "ti,omap4-mcpdm";
reg = <0x0 0x7f>, /* MPU private access */
<0x49032000 0x7f>; /* L3 Interconnect */
reg-names = "mpu", "dma";
interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>;
dmas = <&sdma 65>,
<&sdma 66>;
dma-names = "up_link", "dn_link";
};
Might also need to add clocks?
clocks = <&twl6040>;
clock-names = "pdmclk";
But those are usually defined in board specific files.
And add reg-names, dmas, dma-names information to the docs?
On 22/05/2024 19:02, Mithil wrote: >> Yep. And testing DTS should clearly show that conversion leads to >> incomplete binding. >> >>> >>>> I assume the DTS was validated with the binding. Isn't the case here? >> >> Mithil Bavishi, >> Are you sure you tested the DTS? > > dt_binding_check did not give me any errors. Yeah the example is > different from how it is implemented in the kernel ie board specific > (omap4, omap5 etc). Should the example be changed according to that > dtsi then? Binding needs to be adapted to match DTS or DTS has to be fixed to match binding, depending which one is correct. Mention any changes done in the binding which deviate from pure conversion of TXT->DT schema. https://social.kernel.org/notice/Ai9hYRUKo8suzX3zNY Best regards, Krzysztof
> Binding needs to be adapted to match DTS or DTS has to be fixed to match > binding, depending which one is correct. Mention any changes done in the > binding which deviate from pure conversion of TXT->DT schema. The DTS is correct so will base the example on that (and get a better changelog in the next version) So the checks will be 1) dt_bindings_check and 2) dtbs_check > https://social.kernel.org/notice/Ai9hYRUKo8suzX3zNY Noted, but here I'd assume omap2plus_defconfig would be more relevant. arch/arm/boot/dts/ti/omap/omap4-duovero-parlor.dtb: mcpdm@0: 'ti,hwmods' is a required property from schema $id: http://devicetree.org/schemas/sound/ti,omap4-mcpdm.yaml# We already have ti,hwmods still its asking for it? arch/arm/boot/dts/ti/omap/omap4-duovero-parlor.dtb: mcpdm@0: 'dma-names', 'dmas', 'reg-names' do not match any of the regexes: 'pinctrl-[0-9]+' from schema $id: http://devicetree.org/schemas/sound/ti,omap4-mcpdm.yaml# It also requires a pinctrl subnode which isnt used anywhere, the parent node of mcpdm that is mcpdm_module has a pinctrl how would we go about implementing that?
My apologies, misunderstood the error.
Proposed changes for the next version,
Add dma, dma-names, reg-names properties, and do the changes in
example (rename node to mcpdm since it is different from generic pdm).
reg-names:
items:
- const: mpu
- const: dma
dmas:
maxItems: 2
dma-names:
items:
- const: up_link
- const: dn_link
examples:
- |
#include <dt-bindings/interrupt-controller/arm-gic.h>
mcpdm@0 {
compatible = "ti,omap4-mcpdm";
reg = <0x0 0x7f>, /* MPU private access */
<0x49032000 0x7f>; /* L3 Interconnect */
reg-names = "mpu", "dma";
interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>;
interrupt-parent = <&gic>;
dmas = <&sdma 65>,
<&sdma 66>;
dma-names = "up_link", "dn_link";
ti,hwmods = "mcpdm";
clocks = <&twl6040>;
clock-names = "pdmclk";
};
Remove ti.hwmods from required since some dts like
omap4-duovero-parlor, omap4-panda etc do not use it which causes
dtbs_check to not pass.
On 22/05/2024 20:07, Krzysztof Kozlowski wrote: > On 22/05/2024 19:02, Mithil wrote: >>> Yep. And testing DTS should clearly show that conversion leads to >>> incomplete binding. >>> >>>> >>>>> I assume the DTS was validated with the binding. Isn't the case here? >>> >>> Mithil Bavishi, >>> Are you sure you tested the DTS? >> >> dt_binding_check did not give me any errors. Yeah the example is >> different from how it is implemented in the kernel ie board specific >> (omap4, omap5 etc). Should the example be changed according to that >> dtsi then? > > Binding needs to be adapted to match DTS or DTS has to be fixed to match > binding, depending which one is correct. Normally the DTS is written based on the binding document and the driver is written also to follow the binding document. However in this case we have a broken/inaccurate binding document and the existing DTS files and binaries in wild have deviated (there are boards out there using qnx or BSD and use this binding), or to be precise the binding document was not updated. The existing DTS files are the ABI, so we cannot deviate from them, unfortunately. In this case the DTS / driver needs to be reverse engineered to create a binding document. To note: I'm also guilty of not updating the .txt file. > Mention any changes done in the > binding which deviate from pure conversion of TXT->DT schema. > > https://social.kernel.org/notice/Ai9hYRUKo8suzX3zNY > > Best regards, > Krzysztof >
Hi, On 22/05/2024 20:47, Mithil wrote: > My apologies, misunderstood the error. > Proposed changes for the next version, > Add dma, dma-names, reg-names properties, and do the changes in > example (rename node to mcpdm since it is different from generic pdm). > reg-names: > items: > - const: mpu > - const: dma > > dmas: > maxItems: 2 > > dma-names: > items: > - const: up_link > - const: dn_link > > examples: > - | > #include <dt-bindings/interrupt-controller/arm-gic.h> > mcpdm@0 { > compatible = "ti,omap4-mcpdm"; > reg = <0x0 0x7f>, /* MPU private access */ > <0x49032000 0x7f>; /* L3 Interconnect */ > reg-names = "mpu", "dma"; > interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>; > interrupt-parent = <&gic>; > dmas = <&sdma 65>, > <&sdma 66>; These can be in one line to make it nice and tidy > dma-names = "up_link", "dn_link"; > ti,hwmods = "mcpdm"; The ti,hwmods no longer needed since the sysc conversion > clocks = <&twl6040>; > clock-names = "pdmclk"; > }; > > Remove ti.hwmods from required since some dts like > omap4-duovero-parlor, omap4-panda etc do not use it which causes > dtbs_check to not pass. >
On 22/05/2024 20:39, Péter Ujfalusi wrote: > > > On 22/05/2024 20:07, Krzysztof Kozlowski wrote: >> On 22/05/2024 19:02, Mithil wrote: >>>> Yep. And testing DTS should clearly show that conversion leads to >>>> incomplete binding. >>>> >>>>> >>>>>> I assume the DTS was validated with the binding. Isn't the case here? >>>> >>>> Mithil Bavishi, >>>> Are you sure you tested the DTS? >>> >>> dt_binding_check did not give me any errors. Yeah the example is >>> different from how it is implemented in the kernel ie board specific >>> (omap4, omap5 etc). Should the example be changed according to that >>> dtsi then? >> >> Binding needs to be adapted to match DTS or DTS has to be fixed to match >> binding, depending which one is correct. > > Normally the DTS is written based on the binding document and the driver > is written also to follow the binding document. > However in this case we have a broken/inaccurate binding document and > the existing DTS files and binaries in wild have deviated (there are > boards out there using qnx or BSD and use this binding), or to be > precise the binding document was not updated. > > The existing DTS files are the ABI, so we cannot deviate from them, > unfortunately. > > In this case the DTS / driver needs to be reverse engineered to create a > binding document. Ah, yes, the third option - ABI should not be broken and sometimes binding and DTS needs fixes. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/sound/omap-mcpdm.txt b/Documentation/devicetree/bindings/sound/omap-mcpdm.txt deleted file mode 100644 index ff98a0cb5..000000000 --- a/Documentation/devicetree/bindings/sound/omap-mcpdm.txt +++ /dev/null @@ -1,30 +0,0 @@ -* Texas Instruments OMAP4+ McPDM - -Required properties: -- compatible: "ti,omap4-mcpdm" -- reg: Register location and size as an array: - <MPU access base address, size>, - <L3 interconnect address, size>; -- interrupts: Interrupt number for McPDM -- ti,hwmods: Name of the hwmod associated to the McPDM -- clocks: phandle for the pdmclk provider, likely <&twl6040> -- clock-names: Must be "pdmclk" - -Example: - -mcpdm: mcpdm@40132000 { - compatible = "ti,omap4-mcpdm"; - reg = <0x40132000 0x7f>, /* MPU private access */ - <0x49032000 0x7f>; /* L3 Interconnect */ - interrupts = <0 112 0x4>; - interrupt-parent = <&gic>; - ti,hwmods = "mcpdm"; -}; - -In board DTS file the pdmclk needs to be added: - -&mcpdm { - clocks = <&twl6040>; - clock-names = "pdmclk"; - status = "okay"; -}; diff --git a/Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml b/Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml new file mode 100644 index 000000000..966406078 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml @@ -0,0 +1,61 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/ti,omap4-mcpdm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: OMAP McPDM + +maintainers: + - Misael Lopez Cruz <misael.lopez@ti.com> + +description: + OMAP ALSA SoC DAI driver using McPDM port used by TWL6040 + +properties: + compatible: + const: ti,omap4-mcpdm + + reg: + items: + - description: MPU access base address + - description: L3 interconnect address + + interrupts: + maxItems: 1 + + ti,hwmods: + $ref: /schemas/types.yaml#/definitions/string + enum: [mcpdm] + description: Name of the hwmod associated to the McPDM, likely "mcpdm" + + clocks: + maxItems: 1 + + clock-names: + items: + - const: pdmclk + +required: + - compatible + - reg + - interrupts + - ti,hwmods + - clocks + - clock-names + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + pdm@40132000 { + compatible = "ti,omap4-mcpdm"; + reg = <0x40132000 0x7f>, /* MPU private access */ + <0x49032000 0x7f>; /* L3 Interconnect */ + interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>; + interrupt-parent = <&gic>; + ti,hwmods = "mcpdm"; + clocks = <&twl6040>; + clock-names = "pdmclk"; + };