From: Mithil Bavishi <[email protected]>
Convert the OMAP4+ McPDM bindings to DT schema.
Signed-off-by: Mithil Bavishi <[email protected]>
---
.../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 <[email protected]>
+
+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";
+ };
--
2.34.1
On 17/04/2024 15:42, Mighty wrote:
> From: Mithil Bavishi <[email protected]>
>
> Convert the OMAP4+ McPDM bindings to DT schema.
>
> Signed-off-by: Mithil Bavishi <[email protected]>
> ---
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 <[email protected]>
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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
>
> On Sun, May 5, 2024 at 5:02 PM Krzysztof Kozlowski <[email protected]> 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