diff mbox series

[5/7] dt-bindings: soc: imx: add missing anatop binding

Message ID 20210715082536.1882077-6-aisheng.dong@nxp.com (mailing list archive)
State New, archived
Headers show
Series dt-bindings: imx8mp: fix dt schema check errors | expand

Commit Message

Aisheng Dong July 15, 2021, 8:25 a.m. UTC 
Anatop is a system combo module which supports various analog functions
like PLL, Regulators, LDOs, Sensors and etc.
This binding doc is generated based on the exist usage in dts
in order to fix dt schema check failures.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 .../bindings/soc/imx/fsl,anatop.yaml          | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml

Comments

Rob Herring July 22, 2021, 2:49 a.m. UTC | #1 
On Thu, Jul 15, 2021 at 04:25:34PM +0800, Dong Aisheng wrote:
> Anatop is a system combo module which supports various analog functions
> like PLL, Regulators, LDOs, Sensors and etc.
> This binding doc is generated based on the exist usage in dts
> in order to fix dt schema check failures.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  .../bindings/soc/imx/fsl,anatop.yaml          | 68 +++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml
> new file mode 100644
> index 000000000000..f379d960f527
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml
> @@ -0,0 +1,68 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/imx/fsl,anatop.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Freescale Anatop binding
> +
> +maintainers:
> +  - Dong Aisheng <aisheng.dong@nxp.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - const: fsl,imx6q-anatop
> +          - const: syscon
> +          - const: simple-mfd
> +      - items:
> +          - enum:
> +              - fsl,imx6sl-anatop
> +              - fsl,imx6sll-anatop
> +              - fsl,imx6sx-anatop
> +              - fsl,imx6ul-anatop
> +              - fsl,imx7d-anatop
> +          - const: fsl,imx6q-anatop
> +          - const: syscon
> +          - const: simple-mfd
> +      - items:
> +          - enum:
> +              - fsl,imx8mq-anatop
> +              - fsl,imx8mm-anatop
> +              - fsl,vf610-anatop
> +          - const: syscon
> +      - items:
> +          - enum:
> +              - fsl,imx8mn-anatop
> +              - fsl,imx8mp-anatop
> +          - const: fsl,imx8mm-anatop
> +          - const: syscon
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    items:
> +      - description: Temperature Sensor
> +      - description: PMU interrupt 1
> +      - description: PMU interrupt 2
> +    minItems: 1
> +    maxItems: 3

Don't need maxItems.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: true

This should be the case only for common schemas used by other schemas.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    anatop: anatop@20c8000 {

Drop unused labels.

> +        compatible = "fsl,imx6q-anatop", "syscon", "simple-mfd";
> +        reg = <0x020c8000 0x1000>;
> +        interrupts = <0 49 IRQ_TYPE_LEVEL_HIGH>,
> +                     <0 54 IRQ_TYPE_LEVEL_HIGH>,
> +                     <0 127 IRQ_TYPE_LEVEL_HIGH>;
> +        };
> -- 
> 2.25.1
> 
>
Dong Aisheng Aug. 2, 2021, 11:36 a.m. UTC | #2 
Hi Rob,

On Thu, Jul 22, 2021 at 10:49 AM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Jul 15, 2021 at 04:25:34PM +0800, Dong Aisheng wrote:
> > Anatop is a system combo module which supports various analog functions
> > like PLL, Regulators, LDOs, Sensors and etc.
> > This binding doc is generated based on the exist usage in dts
> > in order to fix dt schema check failures.
> >
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  .../bindings/soc/imx/fsl,anatop.yaml          | 68 +++++++++++++++++++
> >  1 file changed, 68 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml
> > new file mode 100644
> > index 000000000000..f379d960f527
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml
> > @@ -0,0 +1,68 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/soc/imx/fsl,anatop.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Freescale Anatop binding
> > +
> > +maintainers:
> > +  - Dong Aisheng <aisheng.dong@nxp.com>
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - items:
> > +          - const: fsl,imx6q-anatop
> > +          - const: syscon
> > +          - const: simple-mfd
> > +      - items:
> > +          - enum:
> > +              - fsl,imx6sl-anatop
> > +              - fsl,imx6sll-anatop
> > +              - fsl,imx6sx-anatop
> > +              - fsl,imx6ul-anatop
> > +              - fsl,imx7d-anatop
> > +          - const: fsl,imx6q-anatop
> > +          - const: syscon
> > +          - const: simple-mfd
> > +      - items:
> > +          - enum:
> > +              - fsl,imx8mq-anatop
> > +              - fsl,imx8mm-anatop
> > +              - fsl,vf610-anatop
> > +          - const: syscon
> > +      - items:
> > +          - enum:
> > +              - fsl,imx8mn-anatop
> > +              - fsl,imx8mp-anatop
> > +          - const: fsl,imx8mm-anatop
> > +          - const: syscon
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    items:
> > +      - description: Temperature Sensor
> > +      - description: PMU interrupt 1
> > +      - description: PMU interrupt 2
> > +    minItems: 1
> > +    maxItems: 3
>
> Don't need maxItems.
>

Got it

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: true
>
> This should be the case only for common schemas used by other schemas.
>

Like iomuxc-gpr in patch 6, the problem is that for those nodes with
simple-mfd backwards compatibility,
there could be possibly some random subnodes since there're generic
combo registers.
That's why i use additionalProperties true to cover it.
Do you think it's ok?

e.g.
anatop: anatop@20c8000 {
        compatible = "fsl,imx6q-anatop", "syscon", "simple-mfd";
        reg = <0x020c8000 0x1000>;
        interrupts = <0 49 IRQ_TYPE_LEVEL_HIGH>,
                     <0 54 IRQ_TYPE_LEVEL_HIGH>,
                     <0 127 IRQ_TYPE_LEVEL_HIGH>;

        reg_vdd1p1: regulator-1p1 {
                compatible = "fsl,anatop-regulator";
                regulator-name = "vdd1p1";
                regulator-min-microvolt = <1000000>;
                regulator-max-microvolt = <1200000>;
                ...
        };

        tempmon: tempmon {
                compatible = "fsl,imx6q-tempmon";
                interrupt-parent = <&gpc>;
                interrupts = <0 49 IRQ_TYPE_LEVEL_HIGH>;
                fsl,tempmon = <&anatop>;
                nvmem-cells = <&tempmon_calib>, <&tempmon_temp_grade>;
                nvmem-cell-names = "calib", "temp_grade";
                clocks = <&clks IMX6QDL_CLK_PLL3_USB_OTG>;
                #thermal-sensor-cells = <0>;
        };
};

gpr: iomuxc-gpr@20e0000 {
        compatible = "fsl,imx6q-iomuxc-gpr", "syscon", "simple-mfd";
        reg = <0x20e0000 0x38>;

        mux: mux-controller {
                compatible = "mmio-mux";
                #mux-control-cells = <1>;
        };
};

> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    anatop: anatop@20c8000 {
>

Got it

Regards
Aisheng

> Drop unused labels.
>
> > +        compatible = "fsl,imx6q-anatop", "syscon", "simple-mfd";
> > +        reg = <0x020c8000 0x1000>;
> > +        interrupts = <0 49 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <0 54 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <0 127 IRQ_TYPE_LEVEL_HIGH>;
> > +        };
> > --
> > 2.25.1
> >
> >
Rob Herring Aug. 2, 2021, 3:01 p.m. UTC | #3 
On Mon, Aug 2, 2021 at 5:38 AM Dong Aisheng <dongas86@gmail.com> wrote:
>
> Hi Rob,
>
> On Thu, Jul 22, 2021 at 10:49 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, Jul 15, 2021 at 04:25:34PM +0800, Dong Aisheng wrote:
> > > Anatop is a system combo module which supports various analog functions
> > > like PLL, Regulators, LDOs, Sensors and etc.
> > > This binding doc is generated based on the exist usage in dts
> > > in order to fix dt schema check failures.
> > >
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > ---
> > >  .../bindings/soc/imx/fsl,anatop.yaml          | 68 +++++++++++++++++++
> > >  1 file changed, 68 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml
> > > new file mode 100644
> > > index 000000000000..f379d960f527
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml
> > > @@ -0,0 +1,68 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/soc/imx/fsl,anatop.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Freescale Anatop binding
> > > +
> > > +maintainers:
> > > +  - Dong Aisheng <aisheng.dong@nxp.com>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    oneOf:
> > > +      - items:
> > > +          - const: fsl,imx6q-anatop
> > > +          - const: syscon
> > > +          - const: simple-mfd
> > > +      - items:
> > > +          - enum:
> > > +              - fsl,imx6sl-anatop
> > > +              - fsl,imx6sll-anatop
> > > +              - fsl,imx6sx-anatop
> > > +              - fsl,imx6ul-anatop
> > > +              - fsl,imx7d-anatop
> > > +          - const: fsl,imx6q-anatop
> > > +          - const: syscon
> > > +          - const: simple-mfd
> > > +      - items:
> > > +          - enum:
> > > +              - fsl,imx8mq-anatop
> > > +              - fsl,imx8mm-anatop
> > > +              - fsl,vf610-anatop
> > > +          - const: syscon
> > > +      - items:
> > > +          - enum:
> > > +              - fsl,imx8mn-anatop
> > > +              - fsl,imx8mp-anatop
> > > +          - const: fsl,imx8mm-anatop
> > > +          - const: syscon
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    items:
> > > +      - description: Temperature Sensor
> > > +      - description: PMU interrupt 1
> > > +      - description: PMU interrupt 2
> > > +    minItems: 1
> > > +    maxItems: 3
> >
> > Don't need maxItems.
> >
>
> Got it
>
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +
> > > +additionalProperties: true
> >
> > This should be the case only for common schemas used by other schemas.
> >
>
> Like iomuxc-gpr in patch 6, the problem is that for those nodes with
> simple-mfd backwards compatibility,
> there could be possibly some random subnodes since there're generic
> combo registers.
> That's why i use additionalProperties true to cover it.
> Do you think it's ok?

No, because all that should be reviewed rather than random subnodes.
Otherwise, how do we validate them?

Rob
Dong Aisheng Aug. 3, 2021, 4:04 a.m. UTC | #4 
On Mon, Aug 2, 2021 at 11:02 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Aug 2, 2021 at 5:38 AM Dong Aisheng <dongas86@gmail.com> wrote:
> >
> > Hi Rob,
> >
> > On Thu, Jul 22, 2021 at 10:49 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Thu, Jul 15, 2021 at 04:25:34PM +0800, Dong Aisheng wrote:
> > > > Anatop is a system combo module which supports various analog functions
> > > > like PLL, Regulators, LDOs, Sensors and etc.
> > > > This binding doc is generated based on the exist usage in dts
> > > > in order to fix dt schema check failures.
> > > >
> > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > > ---
> > > >  .../bindings/soc/imx/fsl,anatop.yaml          | 68 +++++++++++++++++++
> > > >  1 file changed, 68 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml
> > > > new file mode 100644
> > > > index 000000000000..f379d960f527
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml
> > > > @@ -0,0 +1,68 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/soc/imx/fsl,anatop.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Freescale Anatop binding
> > > > +
> > > > +maintainers:
> > > > +  - Dong Aisheng <aisheng.dong@nxp.com>
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    oneOf:
> > > > +      - items:
> > > > +          - const: fsl,imx6q-anatop
> > > > +          - const: syscon
> > > > +          - const: simple-mfd
> > > > +      - items:
> > > > +          - enum:
> > > > +              - fsl,imx6sl-anatop
> > > > +              - fsl,imx6sll-anatop
> > > > +              - fsl,imx6sx-anatop
> > > > +              - fsl,imx6ul-anatop
> > > > +              - fsl,imx7d-anatop
> > > > +          - const: fsl,imx6q-anatop
> > > > +          - const: syscon
> > > > +          - const: simple-mfd
> > > > +      - items:
> > > > +          - enum:
> > > > +              - fsl,imx8mq-anatop
> > > > +              - fsl,imx8mm-anatop
> > > > +              - fsl,vf610-anatop
> > > > +          - const: syscon
> > > > +      - items:
> > > > +          - enum:
> > > > +              - fsl,imx8mn-anatop
> > > > +              - fsl,imx8mp-anatop
> > > > +          - const: fsl,imx8mm-anatop
> > > > +          - const: syscon
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupts:
> > > > +    items:
> > > > +      - description: Temperature Sensor
> > > > +      - description: PMU interrupt 1
> > > > +      - description: PMU interrupt 2
> > > > +    minItems: 1
> > > > +    maxItems: 3
> > >
> > > Don't need maxItems.
> > >
> >
> > Got it
> >
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +
> > > > +additionalProperties: true
> > >
> > > This should be the case only for common schemas used by other schemas.
> > >
> >
> > Like iomuxc-gpr in patch 6, the problem is that for those nodes with
> > simple-mfd backwards compatibility,
> > there could be possibly some random subnodes since there're generic
> > combo registers.
> > That's why i use additionalProperties true to cover it.
> > Do you think it's ok?
>
> No, because all that should be reviewed rather than random subnodes.
> Otherwise, how do we validate them?

anatop and iomuxc-gpr are not simple mfd devices as they're misc
registers and could contain
various sub misc functions. And those sub functions usually have a
separate dt binding doc which
already covers them and does validation.
The binding here is addressing validation itself. It does not limit
the possible various sub function nodes
which already have a binding doc. Just like a bus node.

If we define them now, it means we may need to keep updating schema when new
user node appear as current dts may not use all possible sub functions.

However, i do agree that defining them all (and may need add more in
the future) helps validation.
But i'm not sure if it's worthy to do it for such cases for a 'bus' node?

Could you help clarify a bit more?

Regards
Aisheng

>
> Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml
new file mode 100644
index 000000000000..f379d960f527
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/imx/fsl,anatop.yaml
@@ -0,0 +1,68 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/imx/fsl,anatop.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale Anatop binding
+
+maintainers:
+  - Dong Aisheng <aisheng.dong@nxp.com>
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - const: fsl,imx6q-anatop
+          - const: syscon
+          - const: simple-mfd
+      - items:
+          - enum:
+              - fsl,imx6sl-anatop
+              - fsl,imx6sll-anatop
+              - fsl,imx6sx-anatop
+              - fsl,imx6ul-anatop
+              - fsl,imx7d-anatop
+          - const: fsl,imx6q-anatop
+          - const: syscon
+          - const: simple-mfd
+      - items:
+          - enum:
+              - fsl,imx8mq-anatop
+              - fsl,imx8mm-anatop
+              - fsl,vf610-anatop
+          - const: syscon
+      - items:
+          - enum:
+              - fsl,imx8mn-anatop
+              - fsl,imx8mp-anatop
+          - const: fsl,imx8mm-anatop
+          - const: syscon
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    items:
+      - description: Temperature Sensor
+      - description: PMU interrupt 1
+      - description: PMU interrupt 2
+    minItems: 1
+    maxItems: 3
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: true
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    anatop: anatop@20c8000 {
+        compatible = "fsl,imx6q-anatop", "syscon", "simple-mfd";
+        reg = <0x020c8000 0x1000>;
+        interrupts = <0 49 IRQ_TYPE_LEVEL_HIGH>,
+                     <0 54 IRQ_TYPE_LEVEL_HIGH>,
+                     <0 127 IRQ_TYPE_LEVEL_HIGH>;
+        };