| Message ID | 20240417134237.23888-1-bavishimithil@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v4] ASoC: dt-bindings: omap-mcpdm: Convert to DT schema | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 2 warnings, 58 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 17/04/2024 15:42, Mighty wrote: > From: Mithil Bavishi <bavishimithil@gmail.com> > > Convert the OMAP4+ McPDM bindings to DT schema. > > Signed-off-by: Mithil Bavishi <bavishimithil@gmail.com> > --- Please provide the changelog under ---. > .../devicetree/bindings/sound/omap-mcpdm.txt | 30 ---------- > .../bindings/sound/ti,omap4-mcpdm.yaml | 58 +++++++++++++++++++ > 2 files changed, 58 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..73fcfaf6e > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml > @@ -0,0 +1,58 @@ > +# 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: > + - Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> How did this happen? I see this was in v1, but I am quite surprised to be listed here. I am for sure not a maintainer of this binding. Choose driver maintainers or platform maintainers, worse case. > + > +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: > + description: Name of the hwmod associated to the McPDM, likely "mcpdm" Not much improved here. You miss $ref and optionally constraints. > + > + clocks: > + description: phandle for the pdmclk provider, likely <&twl6040> Missing constraints, so replace it with maxItems: 1 > + > + clock-names: > + description: Must be "pdmclk" List the items. I asked to open existing bindings and take a look how it is there. Existing bindings would show you how we code this part. > + > + Just one blank line. > +required: > + - compatible > + - reg > + - interrupts > + - ti,hwmods > + - clocks > + - clock-names > + > +additionalProperties: false > + > +examples: > + - | > + pdm@0 { That's wrong address. Old code does not have 0. Please do no change parts of code without reason. If there is a reason, explain it in the changelog. > + compatible = "ti,omap4-mcpdm"; > + reg = <0x40132000 0x7f>, /* MPU private access */ > + <0x49032000 0x7f>; /* L3 Interconnect */ > + interrupts = <0 112 0x4>; Include header and use common defines for flags. Just like all other recent bindings. Best regards, Krzysztof
On Wed, Apr 17, 2024 at 7:33 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > How did this happen? I see this was in v1, but I am quite surprised to > be listed here. I am for sure not a maintainer of this binding. Choose > driver maintainers or platform maintainers, worse case. I might have overlooked this, will fix it. There is no driver maintainer for it as far as I know. Should I include the module author? > Not much improved here. You miss $ref and optionally constraints. Something like this $ref: /schemas/types.yaml#/definitions/string enum: [mcpdm] Didnt really understand the "optionally constraints" part. > Missing constraints, so replace it with maxItems: 1 Similar to how clock-names are handled? > List the items. I asked to open existing bindings and take a look how it > is there. Existing bindings would show you how we code this part. clock-names: items: - const: pdmclk minItems: 1 maxItems: 1 Something like this? > Just one blank line. Removed. > That's wrong address. Old code does not have 0. Please do no change > parts of code without reason. If there is a reason, explain it in the > changelog. > The checks were giving a warning if 0 was not included hence, I'll put the real address if needed then. > Include header and use common defines for flags. Just like all other > recent bindings. > There's no defines for them, this is how it is in the dts :( Best regards, Mithil
On 05/05/2024 09:36, Mithil wrote: > On Wed, Apr 17, 2024 at 7:33 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > >> How did this happen? I see this was in v1, but I am quite surprised to >> be listed here. I am for sure not a maintainer of this binding. Choose >> driver maintainers or platform maintainers, worse case. > > I might have overlooked this, will fix it. There is no driver > maintainer for it as far as I know. > Should I include the module author? Or platform maintainers or whoever is interested in this hardware. > >> Not much improved here. You miss $ref and optionally constraints. > Something like this > $ref: /schemas/types.yaml#/definitions/string > enum: [mcpdm] > Didnt really understand the "optionally constraints" part. Sorry, you stripped out *entire* context. No clue what you refer to. > >> Missing constraints, so replace it with maxItems: 1 > Similar to how clock-names are handled? > >> List the items. I asked to open existing bindings and take a look how it >> is there. Existing bindings would show you how we code this part. > clock-names: > items: > - const: pdmclk > minItems: 1 > maxItems: 1 > Something like this? No. Do you see code like this anywhere? Please only list the items, although without context impossible to judge. > >> Just one blank line. > Removed. > >> That's wrong address. Old code does not have 0. Please do no change >> parts of code without reason. If there is a reason, explain it in the >> changelog. >> > The checks were giving a warning if 0 was not included hence, I'll put > the real address if needed then. > >> Include header and use common defines for flags. Just like all other >> recent bindings. >> > There's no defines for them, this is how it is in the dts :( It does not matter whether some particular DTS uses values or defines, if these are the well known constants. Again, stripping entire context and replying after 2-3 weeks does not help me to understand this at all. Between these 2-3 weeks I got another 200 patches to review. Best regards, Krzysztof
> Or platform maintainers or whoever is interested in this hardware. Aight will do it in the next patch. > >> Not much improved here. You miss $ref and optionally constraints. > > Something like this > > $ref: /schemas/types.yaml#/definitions/string > > enum: [mcpdm] > > Didnt really understand the "optionally constraints" part. > > Sorry, you stripped out *entire* context. No clue what you refer to. Its regarding the ti,hwmods prop ti,hwmods: description: Name of the hwmod associated to the McPDM, likely "mcpdm" > >> Missing constraints, so replace it with maxItems: 1 > > Similar to how clock-names are handled? > > > >> List the items. I asked to open existing bindings and take a look how it > >> is there. Existing bindings would show you how we code this part. > > clock-names: > > items: > > - const: pdmclk > > minItems: 1 > > maxItems: 1 > > Something like this? > > No. Do you see code like this anywhere? Please only list the items, > although without context impossible to judge. > Quick search on sources gave me Documentation/devicetree/bindings/usb/dwc2.yaml which I used as reference for this prop clock-names: description: Must be "pdmclk" > > > >> Just one blank line. > > Removed. > > > >> That's wrong address. Old code does not have 0. Please do no change > >> parts of code without reason. If there is a reason, explain it in the > >> changelog. > >> > > The checks were giving a warning if 0 was not included hence, I'll put > > the real address if needed then. > > > >> Include header and use common defines for flags. Just like all other > >> recent bindings. > >> > > There's no defines for them, this is how it is in the dts :( > > It does not matter whether some particular DTS uses values or defines, > if these are the well known constants. Again, stripping entire context > and replying after 2-3 weeks does not help me to understand this at all. > Between these 2-3 weeks I got another 200 patches to review. > > Best regards, > Krzysztof compatible = "ti,omap4-mcpdm"; reg = <0x40132000 0x7f>, /* MPU private access */ <0x49032000 0x7f>; /* L3 Interconnect */ interrupts = <0 112 0x4>; Not really constants as they do change with platforms (omap4 vs 5 for example) but So do i just make up the constants for it then? Those just seem like magic numbers. Regards, Mithil
On 05/05/2024 11:59, Mithil wrote: >>>> Missing constraints, so replace it with maxItems: 1 >>> Similar to how clock-names are handled? >>> >>>> List the items. I asked to open existing bindings and take a look how it >>>> is there. Existing bindings would show you how we code this part. >>> clock-names: >>> items: >>> - const: pdmclk >>> minItems: 1 >>> maxItems: 1 >>> Something like this? >> >> No. Do you see code like this anywhere? Please only list the items, >> although without context impossible to judge. >> > Quick search on sources gave me > Documentation/devicetree/bindings/usb/dwc2.yaml Above code is different - it does not have maxItems, which you want to add. > which I used as reference for this prop > clock-names: > description: Must be "pdmclk" Again, no, please list the items. items: - const: foo > > compatible = "ti,omap4-mcpdm"; > reg = <0x40132000 0x7f>, /* MPU private access */ > <0x49032000 0x7f>; /* L3 Interconnect */ > interrupts = <0 112 0x4>; > Not really constants as they do change with platforms (omap4 vs 5 for > example) but That is not really relevant... This is not pi or other math constant. > So do i just make up the constants for it then? Those just seem like > magic numbers. Hm? Did you look around for other SoC nodes? 0 looks like SPI, 4 like LEVEL_HIGH, but it depends on number of meaning of the interrupt cells, so who is parent interrupt controller. To work with DT bindings it is necessary to have minimum basic knowledge about DTS. Maybe it would be good to start with some guides/tutorials about DTS. elinux has quite some tutorial and also resources pointing to various conference talks. Best regards, Krzysztof
On Sun, May 5, 2024 at 5:02 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 05/05/2024 11:59, Mithil wrote: > >>>> Missing constraints, so replace it with maxItems: 1 > >>> Similar to how clock-names are handled? > >>> > >>>> List the items. I asked to open existing bindings and take a look how it > >>>> is there. Existing bindings would show you how we code this part. > >>> clock-names: > >>> items: > >>> - const: pdmclk > >>> minItems: 1 > >>> maxItems: 1 > >>> Something like this? > >> > >> No. Do you see code like this anywhere? Please only list the items, > >> although without context impossible to judge. > >> > > Quick search on sources gave me > > Documentation/devicetree/bindings/usb/dwc2.yaml > > Above code is different - it does not have maxItems, which you want to add. > > > which I used as reference for this prop > > clock-names: > > description: Must be "pdmclk" > > Again, no, please list the items. > items: > - const: foo Yep that was the old code for reference. Just items (no maxItems/minItems as well) > > compatible = "ti,omap4-mcpdm"; > > reg = <0x40132000 0x7f>, /* MPU private access */ > > <0x49032000 0x7f>; /* L3 Interconnect */ > > interrupts = <0 112 0x4>; > > Not really constants as they do change with platforms (omap4 vs 5 for > > example) but > > That is not really relevant... This is not pi or other math constant. > > > So do i just make up the constants for it then? Those just seem like > > magic numbers. > > Hm? Did you look around for other SoC nodes? 0 looks like SPI, 4 like > LEVEL_HIGH, but it depends on number of meaning of the interrupt cells, > so who is parent interrupt controller. > Ah the irq values, correct, I thought you meant the reg values. It should be <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>; not sure about 112 though. I suppose these changes are enough right? Best regards, Mithil
Hey, before making a new patch I'll just list the changes that need to be done according to this discussion. Change maintainer Change clocks to only include maxItems: 1 Change clock-names to include items: - const: pdmclk Use correct address in example Use flags for interrupt in example Best regards, Mithil On Sun, May 5, 2024 at 5:19 PM Mithil <bavishimithil@gmail.com> wrote: > > On Sun, May 5, 2024 at 5:02 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > On 05/05/2024 11:59, Mithil wrote: > > >>>> Missing constraints, so replace it with maxItems: 1 > > >>> Similar to how clock-names are handled? > > >>> > > >>>> List the items. I asked to open existing bindings and take a look how it > > >>>> is there. Existing bindings would show you how we code this part. > > >>> clock-names: > > >>> items: > > >>> - const: pdmclk > > >>> minItems: 1 > > >>> maxItems: 1 > > >>> Something like this? > > >> > > >> No. Do you see code like this anywhere? Please only list the items, > > >> although without context impossible to judge. > > >> > > > Quick search on sources gave me > > > Documentation/devicetree/bindings/usb/dwc2.yaml > > > > Above code is different - it does not have maxItems, which you want to add. > > > > > which I used as reference for this prop > > > clock-names: > > > description: Must be "pdmclk" > > > > Again, no, please list the items. > > items: > > - const: foo > Yep that was the old code for reference. Just items (no > maxItems/minItems as well) > > > > compatible = "ti,omap4-mcpdm"; > > > reg = <0x40132000 0x7f>, /* MPU private access */ > > > <0x49032000 0x7f>; /* L3 Interconnect */ > > > interrupts = <0 112 0x4>; > > > Not really constants as they do change with platforms (omap4 vs 5 for > > > example) but > > > > That is not really relevant... This is not pi or other math constant. > > > > > So do i just make up the constants for it then? Those just seem like > > > magic numbers. > > > > Hm? Did you look around for other SoC nodes? 0 looks like SPI, 4 like > > LEVEL_HIGH, but it depends on number of meaning of the interrupt cells, > > so who is parent interrupt controller. > > > Ah the irq values, correct, I thought you meant the reg values. > It should be <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>; not sure about 112 though. > > I suppose these changes are enough right? > > Best regards, > Mithil
On 07/05/2024 17:12, Mithil wrote: > Hey, before making a new patch I'll just list the changes that need to > be done according to this discussion. > Change maintainer > Change clocks to only include maxItems: 1 > Change clock-names to include > items: > - const: pdmclk > Use correct address in example If you meant unit address, then yes. > Use flags for interrupt in example Everything else yes. 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..73fcfaf6e --- /dev/null +++ b/Documentation/devicetree/bindings/sound/ti,omap4-mcpdm.yaml @@ -0,0 +1,58 @@ +# 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: + - Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> + +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: + description: Name of the hwmod associated to the McPDM, likely "mcpdm" + + clocks: + description: phandle for the pdmclk provider, likely <&twl6040> + + clock-names: + description: Must be "pdmclk" + + +required: + - compatible + - reg + - interrupts + - ti,hwmods + - clocks + - clock-names + +additionalProperties: false + +examples: + - | + pdm@0 { + compatible = "ti,omap4-mcpdm"; + reg = <0x40132000 0x7f>, /* MPU private access */ + <0x49032000 0x7f>; /* L3 Interconnect */ + interrupts = <0 112 0x4>; + interrupt-parent = <&gic>; + ti,hwmods = "mcpdm"; + clocks = <&twl6040>; + clock-names = "pdmclk"; + };