[v2,1/2] dt-bindings: pinctrl: add schema for NXP S32 SoCs

  Add DT schema for the pinctrl driver of NXP S32 SoC family.

Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
Signed-off-by: Andrei Stefanescu <andrei.stefanescu@nxp.com>
Signed-off-by: Chester Lin <clin@suse.com>
---

Changes in v2:
- Remove the "nxp,pins" property since it has been moved into the driver.
- Add descriptions for reg entries.
- Refine the compatible name from "nxp,s32g-..." to "nxp,s32g2-...".
- Fix schema issues and revise the example.
- Fix the copyright format suggested by NXP.

 .../pinctrl/nxp,s32cc-siul2-pinctrl.yaml      | 125 ++++++++++++++++++
 1 file changed, 125 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
  

Hi Chester,

Am 28.11.22 um 06:48 schrieb Chester Lin:
> Add DT schema for the pinctrl driver of NXP S32 SoC family.
> 
> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@nxp.com>
> Signed-off-by: Chester Lin <clin@suse.com>
> ---
> 
> Changes in v2:
> - Remove the "nxp,pins" property since it has been moved into the driver.
> - Add descriptions for reg entries.
> - Refine the compatible name from "nxp,s32g-..." to "nxp,s32g2-...".

Thanks.

> - Fix schema issues and revise the example.
> - Fix the copyright format suggested by NXP.
> 
>   .../pinctrl/nxp,s32cc-siul2-pinctrl.yaml      | 125 ++++++++++++++++++
>   1 file changed, 125 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> new file mode 100644
> index 000000000000..2fc25a9362af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> @@ -0,0 +1,125 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

Any reason the code is GPL-2.0-or-later but the schema is GPL-2.0-only?

> +# Copyright 2022 NXP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/nxp,s32cc-siul2-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP S32 Common Chassis SIUL2 iomux controller
> +
> +maintainers:
> +  - Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
> +  - Chester Lin <clin@suse.com>
> +
> +description: |
> +  Core driver for the pin controller found on S32 Common Chassis SoC.

SoC family

> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,s32g2-siul2-pinctrl
> +
> +  reg:
> +    description:
> +      A list of MSCR/IMCR register regions to be reserved.
> +      - MSCR (Multiplexed Signal Configuration Register)
> +        An MSCR register can configure the associated pin as either a GPIO pin
> +        or a function output pin depends on the selected signal source.
> +      - IMCR (Input Multiplexed Signal Configuration Register)
> +        An IMCR register can configure the associated pin as function input
> +        pin depends on the selected signal source.

Does this multi-paragraph text not need "description: |" like above?

> +    minItems: 5
> +    items:
> +      - description: MSCR registers group 0 managed by the SIUL2 controller 0
> +      - description: MSCR registers group 1 managed by the SIUL2 controller 1
> +      - description: MSCR registers group 2 managed by the SIUL2 controller 1
> +      - description: IMCR registers group 0 managed by the SIUL2 controller 0
> +      - description: IMCR registers group 1 managed by the SIUL2 controller 1
> +      - description: IMCR registers group 2 managed by the SIUL2 controller 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +patternProperties:
> +  '-pins$':
> +    type: object
> +    additionalProperties: false
> +
> +    patternProperties:
> +      '-grp[0-9]$':
> +        type: object
> +        allOf:
> +          - $ref: pinmux-node.yaml#
> +          - $ref: pincfg-node.yaml#
> +        unevaluatedProperties: false
> +        description:
> +          Pinctrl node's client devices specify pin muxes using subnodes,
> +          which in turn use the standard properties.
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    /* Pins functions (SSS field) */
> +    #define FUNC0  0
> +    #define FUNC1  1
> +    #define FUNC2  2
> +    #define FUNC3  3
> +    #define FUNC4  4
> +    #define FUNC5  5
> +    #define FUNC6  6
> +    #define FUNC7  7
> +
> +    #define S32CC_PINMUX(PIN, FUNC) (((PIN) << 4) | (FUNC))
> +
> +    #define S32CC_SLEW_208MHZ  0
> +    #define S32CC_SLEW_166MHZ  4
> +    #define S32CC_SLEW_150MHZ  5
> +    #define S32CC_SLEW_133MHZ  6
> +    #define S32CC_SLEW_83MHZ   7

I notice that neither this patch nor the following one introduces a 
dt-bindings header for these macros? Is the plan to only have them in 
TF-A sources? Thinking of DT overlays for SoMs, for example.

Regards,
Andreas

> +
> +    pinctrl@4009c240 {
> +        compatible = "nxp,s32g2-siul2-pinctrl";
> +
> +        /*
> +         * There are two SIUL2 controllers in S32G2:
> +         *
> +         *   siul2_0 @ 0x4009c000
> +         *   siul2_1 @ 0x44010000
> +         *
> +         * Every SIUL2 controller has multiple register types, and here
> +         * only MSCR and IMCR registers need to be revealed for kernel
> +         * to configure pinmux. Please note that some indexes are reserved,
> +         * such as MSCR102-MSCR111 in the following reg property.
> +         */
> +
> +              /* MSCR0-MSCR101 registers on siul2_0 */
> +        reg = <0x4009c240 0x198>,
> +              /* MSCR112-MSCR122 registers on siul2_1 */
> +              <0x44010400 0x2c>,
> +              /* MSCR144-MSCR190 registers on siul2_1 */
> +              <0x44010480 0xbc>,
> +              /* IMCR0-IMCR83 registers on siul2_0 */
> +              <0x4009ca40 0x150>,
> +              /* IMCR119-IMCR397 registers on siul2_1 */
> +              <0x44010c1c 0x45c>,
> +              /* IMCR430-IMCR495 registers on siul2_1 */
> +              <0x440110f8 0x108>;
> +
> +        llce-can0-pins {
> +            llce-can0-grp0 {
> +                pinmux = <S32CC_PINMUX(43, FUNC0)>;
> +                input-enable;
> +                slew-rate = <S32CC_SLEW_208MHZ>;
> +            };
> +
> +            llce-can0-grp1 {
> +                pinmux = <S32CC_PINMUX(44, FUNC2)>;
> +                output-enable;
> +                slew-rate = <S32CC_SLEW_208MHZ>;
> +            };
> +        };
> +    };
> +...
  

Hi Andreas,

On Tue, Nov 29, 2022 at 03:00:52PM +0100, Andreas Färber wrote:
> Hi Chester,
> 
> Am 28.11.22 um 06:48 schrieb Chester Lin:
> > Add DT schema for the pinctrl driver of NXP S32 SoC family.
> > 
> > Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> > Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
> > Signed-off-by: Andrei Stefanescu <andrei.stefanescu@nxp.com>
> > Signed-off-by: Chester Lin <clin@suse.com>
> > ---
> > 
> > Changes in v2:
> > - Remove the "nxp,pins" property since it has been moved into the driver.
> > - Add descriptions for reg entries.
> > - Refine the compatible name from "nxp,s32g-..." to "nxp,s32g2-...".
> 
> Thanks.
> 
> > - Fix schema issues and revise the example.
> > - Fix the copyright format suggested by NXP.
> > 
> >   .../pinctrl/nxp,s32cc-siul2-pinctrl.yaml      | 125 ++++++++++++++++++
> >   1 file changed, 125 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> > new file mode 100644
> > index 000000000000..2fc25a9362af
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> > @@ -0,0 +1,125 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> 
> Any reason the code is GPL-2.0-or-later but the schema is GPL-2.0-only?
> 

Actually this patch is modified from an original downstream schema, which has
"GPL-2.0-only". See:

https://source.codeaurora.org/external/autobsps32/linux/tree/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2.yaml?h=bsp34.0-5.10.120-rt#n1


> > +# Copyright 2022 NXP
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pinctrl/nxp,s32cc-siul2-pinctrl.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP S32 Common Chassis SIUL2 iomux controller
> > +
> > +maintainers:
> > +  - Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
> > +  - Chester Lin <clin@suse.com>
> > +
> > +description: |
> > +  Core driver for the pin controller found on S32 Common Chassis SoC.
> 
> SoC family
> 

Will fix in v3.

> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - nxp,s32g2-siul2-pinctrl
> > +
> > +  reg:
> > +    description:
> > +      A list of MSCR/IMCR register regions to be reserved.
> > +      - MSCR (Multiplexed Signal Configuration Register)
> > +        An MSCR register can configure the associated pin as either a GPIO pin
> > +        or a function output pin depends on the selected signal source.
> > +      - IMCR (Input Multiplexed Signal Configuration Register)
> > +        An IMCR register can configure the associated pin as function input
> > +        pin depends on the selected signal source.
> 
> Does this multi-paragraph text not need "description: |" like above?
> 

Will fix in v3, thanks for the reminder.

> > +    minItems: 5
> > +    items:
> > +      - description: MSCR registers group 0 managed by the SIUL2 controller 0
> > +      - description: MSCR registers group 1 managed by the SIUL2 controller 1
> > +      - description: MSCR registers group 2 managed by the SIUL2 controller 1
> > +      - description: IMCR registers group 0 managed by the SIUL2 controller 0
> > +      - description: IMCR registers group 1 managed by the SIUL2 controller 1
> > +      - description: IMCR registers group 2 managed by the SIUL2 controller 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +patternProperties:
> > +  '-pins$':
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    patternProperties:
> > +      '-grp[0-9]$':
> > +        type: object
> > +        allOf:
> > +          - $ref: pinmux-node.yaml#
> > +          - $ref: pincfg-node.yaml#
> > +        unevaluatedProperties: false
> > +        description:
> > +          Pinctrl node's client devices specify pin muxes using subnodes,
> > +          which in turn use the standard properties.
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
> > +    /* Pins functions (SSS field) */
> > +    #define FUNC0  0
> > +    #define FUNC1  1
> > +    #define FUNC2  2
> > +    #define FUNC3  3
> > +    #define FUNC4  4
> > +    #define FUNC5  5
> > +    #define FUNC6  6
> > +    #define FUNC7  7
> > +
> > +    #define S32CC_PINMUX(PIN, FUNC) (((PIN) << 4) | (FUNC))
> > +
> > +    #define S32CC_SLEW_208MHZ  0
> > +    #define S32CC_SLEW_166MHZ  4
> > +    #define S32CC_SLEW_150MHZ  5
> > +    #define S32CC_SLEW_133MHZ  6
> > +    #define S32CC_SLEW_83MHZ   7
> 
> I notice that neither this patch nor the following one introduces a
> dt-bindings header for these macros? Is the plan to only have them in TF-A
> sources? Thinking of DT overlays for SoMs, for example.
> 

Yes, it is. Since the current arch relies on the FDT offered by NXP's
downstream TF-A, only TF-A sources include the dt-bindings header in order
to refer to these macros. However, introduce these macros in this example
can still help developers to understand how a pinmux constructs.

Regards,
Chester

> Regards,
> Andreas
> 
> > +
> > +    pinctrl@4009c240 {
> > +        compatible = "nxp,s32g2-siul2-pinctrl";
> > +
> > +        /*
> > +         * There are two SIUL2 controllers in S32G2:
> > +         *
> > +         *   siul2_0 @ 0x4009c000
> > +         *   siul2_1 @ 0x44010000
> > +         *
> > +         * Every SIUL2 controller has multiple register types, and here
> > +         * only MSCR and IMCR registers need to be revealed for kernel
> > +         * to configure pinmux. Please note that some indexes are reserved,
> > +         * such as MSCR102-MSCR111 in the following reg property.
> > +         */
> > +
> > +              /* MSCR0-MSCR101 registers on siul2_0 */
> > +        reg = <0x4009c240 0x198>,
> > +              /* MSCR112-MSCR122 registers on siul2_1 */
> > +              <0x44010400 0x2c>,
> > +              /* MSCR144-MSCR190 registers on siul2_1 */
> > +              <0x44010480 0xbc>,
> > +              /* IMCR0-IMCR83 registers on siul2_0 */
> > +              <0x4009ca40 0x150>,
> > +              /* IMCR119-IMCR397 registers on siul2_1 */
> > +              <0x44010c1c 0x45c>,
> > +              /* IMCR430-IMCR495 registers on siul2_1 */
> > +              <0x440110f8 0x108>;
> > +
> > +        llce-can0-pins {
> > +            llce-can0-grp0 {
> > +                pinmux = <S32CC_PINMUX(43, FUNC0)>;
> > +                input-enable;
> > +                slew-rate = <S32CC_SLEW_208MHZ>;
> > +            };
> > +
> > +            llce-can0-grp1 {
> > +                pinmux = <S32CC_PINMUX(44, FUNC2)>;
> > +                output-enable;
> > +                slew-rate = <S32CC_SLEW_208MHZ>;
> > +            };
> > +        };
> > +    };
> > +...
> 
> -- 
> SUSE Software Solutions Germany GmbH
> Frankenstraße 146, 90461 Nürnberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nürnberg)
  

On 28/11/2022 06:48, Chester Lin wrote:
> Add DT schema for the pinctrl driver of NXP S32 SoC family.
> 
> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@nxp.com>
> Signed-off-by: Chester Lin <clin@suse.com>
> ---
> 
> Changes in v2:
> - Remove the "nxp,pins" property since it has been moved into the driver.
> - Add descriptions for reg entries.
> - Refine the compatible name from "nxp,s32g-..." to "nxp,s32g2-...".
> - Fix schema issues and revise the example.
> - Fix the copyright format suggested by NXP.
> 
>  .../pinctrl/nxp,s32cc-siul2-pinctrl.yaml      | 125 ++++++++++++++++++
>  1 file changed, 125 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> new file mode 100644
> index 000000000000..2fc25a9362af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml

Usually filename matches the compatible (or family name), so any reason
why compatible is "nxp,s32g2" but filename is "nxp,s32cc"?

> @@ -0,0 +1,125 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2022 NXP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/nxp,s32cc-siul2-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP S32 Common Chassis SIUL2 iomux controller
> +
> +maintainers:
> +  - Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
> +  - Chester Lin <clin@suse.com>
> +
> +description: |
> +  Core driver for the pin controller found on S32 Common Chassis SoC.

If "Core driver for the" refers to Linux driver, then drop it. If refers
to something else, please elaborate.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,s32g2-siul2-pinctrl
> +
> +  reg:
> +    description:
> +      A list of MSCR/IMCR register regions to be reserved.
> +      - MSCR (Multiplexed Signal Configuration Register)
> +        An MSCR register can configure the associated pin as either a GPIO pin
> +        or a function output pin depends on the selected signal source.
> +      - IMCR (Input Multiplexed Signal Configuration Register)
> +        An IMCR register can configure the associated pin as function input
> +        pin depends on the selected signal source.
> +    minItems: 5
> +    items:
> +      - description: MSCR registers group 0 managed by the SIUL2 controller 0
> +      - description: MSCR registers group 1 managed by the SIUL2 controller 1
> +      - description: MSCR registers group 2 managed by the SIUL2 controller 1
> +      - description: IMCR registers group 0 managed by the SIUL2 controller 0
> +      - description: IMCR registers group 1 managed by the SIUL2 controller 1
> +      - description: IMCR registers group 2 managed by the SIUL2 controller 1
> +
> +required:
> +  - compatible
> +  - reg

required goes after all properties, so below patternProperties.
> +
> +patternProperties:
> +  '-pins$':
> +    type: object
> +    additionalProperties: false
> +
> +    patternProperties:
> +      '-grp[0-9]$':
> +        type: object
> +        allOf:
> +          - $ref: pinmux-node.yaml#
> +          - $ref: pincfg-node.yaml#
> +        unevaluatedProperties: false
> +        description:
> +          Pinctrl node's client devices specify pin muxes using subnodes,
> +          which in turn use the standard properties.

All properties are accepted? What about values, e.g. for drive strength?

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    /* Pins functions (SSS field) */
> +    #define FUNC0  0
> +    #define FUNC1  1
> +    #define FUNC2  2
> +    #define FUNC3  3
> +    #define FUNC4  4
> +    #define FUNC5  5
> +    #define FUNC6  6
> +    #define FUNC7  7
> +
> +    #define S32CC_PINMUX(PIN, FUNC) (((PIN) << 4) | (FUNC))
> +
> +    #define S32CC_SLEW_208MHZ  0
> +    #define S32CC_SLEW_166MHZ  4
> +    #define S32CC_SLEW_150MHZ  5
> +    #define S32CC_SLEW_133MHZ  6
> +    #define S32CC_SLEW_83MHZ   7
> +
> +    pinctrl@4009c240 {
> +        compatible = "nxp,s32g2-siul2-pinctrl";
> +
> +        /*
> +         * There are two SIUL2 controllers in S32G2:
> +         *
> +         *   siul2_0 @ 0x4009c000
> +         *   siul2_1 @ 0x44010000
> +         *
> +         * Every SIUL2 controller has multiple register types, and here
> +         * only MSCR and IMCR registers need to be revealed for kernel
> +         * to configure pinmux. Please note that some indexes are reserved,
> +         * such as MSCR102-MSCR111 in the following reg property.
> +         */
> +

Either this should be part of description or should be dropped. It blows
example and probably duplicates DTS.


Best regards,
Krzysztof
  

Hi Krzysztof,

On Wed, Nov 30, 2022 at 03:58:52PM +0100, Krzysztof Kozlowski wrote:
> On 28/11/2022 06:48, Chester Lin wrote:
> > Add DT schema for the pinctrl driver of NXP S32 SoC family.
> > 
> > Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> > Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
> > Signed-off-by: Andrei Stefanescu <andrei.stefanescu@nxp.com>
> > Signed-off-by: Chester Lin <clin@suse.com>
> > ---
> > 
> > Changes in v2:
> > - Remove the "nxp,pins" property since it has been moved into the driver.
> > - Add descriptions for reg entries.
> > - Refine the compatible name from "nxp,s32g-..." to "nxp,s32g2-...".
> > - Fix schema issues and revise the example.
> > - Fix the copyright format suggested by NXP.
> > 
> >  .../pinctrl/nxp,s32cc-siul2-pinctrl.yaml      | 125 ++++++++++++++++++
> >  1 file changed, 125 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> > new file mode 100644
> > index 000000000000..2fc25a9362af
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> 
> Usually filename matches the compatible (or family name), so any reason
> why compatible is "nxp,s32g2" but filename is "nxp,s32cc"?
> 

According to NXP, the S32CC is a microarch which is adapted by different S32 SoCs,
such as S32G2/G3 and S32R45. Some common IPs are implemented in S32CC, such as
serial, pinctrl, mmc, gmac and some other peripheral interfaces. S32R45 has
different pinouts compared to S32G2, which means that there would not be just
"s32g2-siul2-pinctrl" but also "s32r45-siul2-pinctrl" in the compatible enum if
S32R45 has to be upstreamed in the future. For this case, it seems to be
inappropriate that adding a compatible name without any "s32g" keyword in the
filename "nxp,s32g2-.." unless creating a new yaml for each platform, such as
nxp,s32r45-siul2-pinctl.yaml.

> > @@ -0,0 +1,125 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright 2022 NXP
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pinctrl/nxp,s32cc-siul2-pinctrl.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP S32 Common Chassis SIUL2 iomux controller
> > +
> > +maintainers:
> > +  - Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
> > +  - Chester Lin <clin@suse.com>
> > +
> > +description: |
> > +  Core driver for the pin controller found on S32 Common Chassis SoC.
> 
> If "Core driver for the" refers to Linux driver, then drop it. If refers
> to something else, please elaborate.
> 

Will fix it in v3.

> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - nxp,s32g2-siul2-pinctrl
> > +
> > +  reg:
> > +    description:
> > +      A list of MSCR/IMCR register regions to be reserved.
> > +      - MSCR (Multiplexed Signal Configuration Register)
> > +        An MSCR register can configure the associated pin as either a GPIO pin
> > +        or a function output pin depends on the selected signal source.
> > +      - IMCR (Input Multiplexed Signal Configuration Register)
> > +        An IMCR register can configure the associated pin as function input
> > +        pin depends on the selected signal source.
> > +    minItems: 5
> > +    items:
> > +      - description: MSCR registers group 0 managed by the SIUL2 controller 0
> > +      - description: MSCR registers group 1 managed by the SIUL2 controller 1
> > +      - description: MSCR registers group 2 managed by the SIUL2 controller 1
> > +      - description: IMCR registers group 0 managed by the SIUL2 controller 0
> > +      - description: IMCR registers group 1 managed by the SIUL2 controller 1
> > +      - description: IMCR registers group 2 managed by the SIUL2 controller 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> 
> required goes after all properties, so below patternProperties.

Will fix in v3.

> > +
> > +patternProperties:
> > +  '-pins$':
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    patternProperties:
> > +      '-grp[0-9]$':
> > +        type: object
> > +        allOf:
> > +          - $ref: pinmux-node.yaml#
> > +          - $ref: pincfg-node.yaml#
> > +        unevaluatedProperties: false
> > +        description:
> > +          Pinctrl node's client devices specify pin muxes using subnodes,
> > +          which in turn use the standard properties.
> 
> All properties are accepted? What about values, e.g. for drive strength?

For those unsupported properties such as drive-strength, the s32g2 pinctrl driver
returns -EOPNOTSUPP.

> 
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
> > +    /* Pins functions (SSS field) */
> > +    #define FUNC0  0
> > +    #define FUNC1  1
> > +    #define FUNC2  2
> > +    #define FUNC3  3
> > +    #define FUNC4  4
> > +    #define FUNC5  5
> > +    #define FUNC6  6
> > +    #define FUNC7  7
> > +
> > +    #define S32CC_PINMUX(PIN, FUNC) (((PIN) << 4) | (FUNC))
> > +
> > +    #define S32CC_SLEW_208MHZ  0
> > +    #define S32CC_SLEW_166MHZ  4
> > +    #define S32CC_SLEW_150MHZ  5
> > +    #define S32CC_SLEW_133MHZ  6
> > +    #define S32CC_SLEW_83MHZ   7
> > +
> > +    pinctrl@4009c240 {
> > +        compatible = "nxp,s32g2-siul2-pinctrl";
> > +
> > +        /*
> > +         * There are two SIUL2 controllers in S32G2:
> > +         *
> > +         *   siul2_0 @ 0x4009c000
> > +         *   siul2_1 @ 0x44010000
> > +         *
> > +         * Every SIUL2 controller has multiple register types, and here
> > +         * only MSCR and IMCR registers need to be revealed for kernel
> > +         * to configure pinmux. Please note that some indexes are reserved,
> > +         * such as MSCR102-MSCR111 in the following reg property.
> > +         */
> > +
> 
> Either this should be part of description or should be dropped. It blows
> example and probably duplicates DTS.
> 
> 
> Best regards,
> Krzysztof
>
  

On 05/12/2022 07:16, Chester Lin wrote:
> Hi Krzysztof,
> 
> On Wed, Nov 30, 2022 at 03:58:52PM +0100, Krzysztof Kozlowski wrote:
>> On 28/11/2022 06:48, Chester Lin wrote:
>>> Add DT schema for the pinctrl driver of NXP S32 SoC family.
>>>
>>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>>> Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
>>> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@nxp.com>
>>> Signed-off-by: Chester Lin <clin@suse.com>
>>> ---
>>>
>>> Changes in v2:
>>> - Remove the "nxp,pins" property since it has been moved into the driver.
>>> - Add descriptions for reg entries.
>>> - Refine the compatible name from "nxp,s32g-..." to "nxp,s32g2-...".
>>> - Fix schema issues and revise the example.
>>> - Fix the copyright format suggested by NXP.
>>>
>>>  .../pinctrl/nxp,s32cc-siul2-pinctrl.yaml      | 125 ++++++++++++++++++
>>>  1 file changed, 125 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
>>> new file mode 100644
>>> index 000000000000..2fc25a9362af
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
>>
>> Usually filename matches the compatible (or family name), so any reason
>> why compatible is "nxp,s32g2" but filename is "nxp,s32cc"?
>>
> 
> According to NXP, the S32CC is a microarch which is adapted by different S32 SoCs,
> such as S32G2/G3 and S32R45. Some common IPs are implemented in S32CC, such as
> serial, pinctrl, mmc, gmac and some other peripheral interfaces. S32R45 has
> different pinouts compared to S32G2, which means that there would not be just
> "s32g2-siul2-pinctrl" but also "s32r45-siul2-pinctrl" in the compatible enum if
> S32R45 has to be upstreamed in the future. For this case, it seems to be
> inappropriate that adding a compatible name without any "s32g" keyword in the
> filename "nxp,s32g2-.." unless creating a new yaml for each platform, such as
> nxp,s32r45-siul2-pinctl.yaml.

First, you can always rename a file if such need arises. Maybe new SoCs
will come, maybe not.

Second, when you actually upstream new SoC it might anyway require new
bindings file, because pinctrls are quite specific and it is usually
difficult to support multiple devices in a nice, readable way in one
file. Therefore anyway another file is quite likely.

(...)

>>> +
>>> +patternProperties:
>>> +  '-pins$':
>>> +    type: object
>>> +    additionalProperties: false
>>> +
>>> +    patternProperties:
>>> +      '-grp[0-9]$':
>>> +        type: object
>>> +        allOf:
>>> +          - $ref: pinmux-node.yaml#
>>> +          - $ref: pincfg-node.yaml#
>>> +        unevaluatedProperties: false
>>> +        description:
>>> +          Pinctrl node's client devices specify pin muxes using subnodes,
>>> +          which in turn use the standard properties.
>>
>> All properties are accepted? What about values, e.g. for drive strength?
> 
> For those unsupported properties such as drive-strength, the s32g2 pinctrl driver
> returns -EOPNOTSUPP.

I don't care what the driver is doing, we do not discuss the driver. You
need to describe properly the hardware and I doubt that hardware accepts
all drive-strengths, all forms of pull resistors (so any Ohm value).

Add constrains.

>>
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +
>>> +    /* Pins functions (SSS field) */
>>> +    #define FUNC0  0
>>> +    #define FUNC1  1
>>> +    #define FUNC2  2
>>> +    #define FUNC3  3
>>> +    #define FUNC4  4
>>> +    #define FUNC5  5
>>> +    #define FUNC6  6
>>> +    #define FUNC7  7

This is another surprise - functions are texts, not numbers.

>>> +
>>> +    #define S32CC_PINMUX(PIN, FUNC) (((PIN) << 4) | (FUNC))
>>> +
>>> +    #define S32CC_SLEW_208MHZ  0
>>> +    #define S32CC_SLEW_166MHZ  4
>>> +    #define S32CC_SLEW_150MHZ  5
>>> +    #define S32CC_SLEW_133MHZ  6
>>> +    #define S32CC_SLEW_83MHZ   7

Don't store register values in the bindings examples. Instead you need
to be explain the slew-rate property.

>>> +
>>> +    pinctrl@4009c240 {
>>> +        compatible = "nxp,s32g2-siul2-pinctrl";
>>> +
>>> +        /*
>>> +         * There are two SIUL2 controllers in S32G2:
>>> +         *
>>> +         *   siul2_0 @ 0x4009c000
>>> +         *   siul2_1 @ 0x44010000
>>> +         *
>>> +         * Every SIUL2 controller has multiple register types, and here
>>> +         * only MSCR and IMCR registers need to be revealed for kernel
>>> +         * to configure pinmux. Please note that some indexes are reserved,
>>> +         * such as MSCR102-MSCR111 in the following reg property.
>>> +         */
>>> +
>>
>> Either this should be part of description or should be dropped. It blows
>> example and probably duplicates DTS.
>>
>>
>> Best regards,
>> Krzysztof
>>

Best regards,
Krzysztof
  

On Mon, Dec 05, 2022 at 10:02:14AM +0100, Krzysztof Kozlowski wrote:
> On 05/12/2022 07:16, Chester Lin wrote:
> > Hi Krzysztof,
> > 
> > On Wed, Nov 30, 2022 at 03:58:52PM +0100, Krzysztof Kozlowski wrote:
> >> On 28/11/2022 06:48, Chester Lin wrote:
> >>> Add DT schema for the pinctrl driver of NXP S32 SoC family.
> >>>
> >>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> >>> Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
> >>> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@nxp.com>
> >>> Signed-off-by: Chester Lin <clin@suse.com>
> >>> ---
> >>>
> >>> Changes in v2:
> >>> - Remove the "nxp,pins" property since it has been moved into the driver.
> >>> - Add descriptions for reg entries.
> >>> - Refine the compatible name from "nxp,s32g-..." to "nxp,s32g2-...".
> >>> - Fix schema issues and revise the example.
> >>> - Fix the copyright format suggested by NXP.
> >>>
> >>>  .../pinctrl/nxp,s32cc-siul2-pinctrl.yaml      | 125 ++++++++++++++++++
> >>>  1 file changed, 125 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> >>> new file mode 100644
> >>> index 000000000000..2fc25a9362af
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> >>
> >> Usually filename matches the compatible (or family name), so any reason
> >> why compatible is "nxp,s32g2" but filename is "nxp,s32cc"?
> >>
> > 
> > According to NXP, the S32CC is a microarch which is adapted by different S32 SoCs,
> > such as S32G2/G3 and S32R45. Some common IPs are implemented in S32CC, such as
> > serial, pinctrl, mmc, gmac and some other peripheral interfaces. S32R45 has
> > different pinouts compared to S32G2, which means that there would not be just
> > "s32g2-siul2-pinctrl" but also "s32r45-siul2-pinctrl" in the compatible enum if
> > S32R45 has to be upstreamed in the future. For this case, it seems to be
> > inappropriate that adding a compatible name without any "s32g" keyword in the
> > filename "nxp,s32g2-.." unless creating a new yaml for each platform, such as
> > nxp,s32r45-siul2-pinctl.yaml.
> 
> First, you can always rename a file if such need arises. Maybe new SoCs
> will come, maybe not.
> 
> Second, when you actually upstream new SoC it might anyway require new
> bindings file, because pinctrls are quite specific and it is usually
> difficult to support multiple devices in a nice, readable way in one
> file. Therefore anyway another file is quite likely.
> 

Thanks for your guidance. Will fix it.

> (...)
> 
> >>> +
> >>> +patternProperties:
> >>> +  '-pins$':
> >>> +    type: object
> >>> +    additionalProperties: false
> >>> +
> >>> +    patternProperties:
> >>> +      '-grp[0-9]$':
> >>> +        type: object
> >>> +        allOf:
> >>> +          - $ref: pinmux-node.yaml#
> >>> +          - $ref: pincfg-node.yaml#
> >>> +        unevaluatedProperties: false
> >>> +        description:
> >>> +          Pinctrl node's client devices specify pin muxes using subnodes,
> >>> +          which in turn use the standard properties.
> >>
> >> All properties are accepted? What about values, e.g. for drive strength?
> > 
> > For those unsupported properties such as drive-strength, the s32g2 pinctrl driver
> > returns -EOPNOTSUPP.
> 
> I don't care what the driver is doing, we do not discuss the driver. You
> need to describe properly the hardware and I doubt that hardware accepts
> all drive-strengths, all forms of pull resistors (so any Ohm value).
> 
> Add constrains.
> 

Thanks for the suggestion. IIUC, I should specifically described the supported
pinmux and pincfg properties in this schema and then add an "additionalProperties: false"
in the end in order to constrain unsupported properties listed in the pattern
pin groups.

> >>
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +
> >>> +    /* Pins functions (SSS field) */
> >>> +    #define FUNC0  0
> >>> +    #define FUNC1  1
> >>> +    #define FUNC2  2
> >>> +    #define FUNC3  3
> >>> +    #define FUNC4  4
> >>> +    #define FUNC5  5
> >>> +    #define FUNC6  6
> >>> +    #define FUNC7  7
> 
> This is another surprise - functions are texts, not numbers.
> 

Maybe the FUNC[0|9] are not accurate to describe Source Signal Select [SSS].
I will drop these definitions from the example and try elaborating 'pinmux'
in its property description.

> >>> +
> >>> +    #define S32CC_PINMUX(PIN, FUNC) (((PIN) << 4) | (FUNC))
> >>> +
> >>> +    #define S32CC_SLEW_208MHZ  0
> >>> +    #define S32CC_SLEW_166MHZ  4
> >>> +    #define S32CC_SLEW_150MHZ  5
> >>> +    #define S32CC_SLEW_133MHZ  6
> >>> +    #define S32CC_SLEW_83MHZ   7
> 
> Don't store register values in the bindings examples. Instead you need
> to be explain the slew-rate property.
> 

Will do.

> >>> +
> >>> +    pinctrl@4009c240 {
> >>> +        compatible = "nxp,s32g2-siul2-pinctrl";
> >>> +
> >>> +        /*
> >>> +         * There are two SIUL2 controllers in S32G2:
> >>> +         *
> >>> +         *   siul2_0 @ 0x4009c000
> >>> +         *   siul2_1 @ 0x44010000
> >>> +         *
> >>> +         * Every SIUL2 controller has multiple register types, and here
> >>> +         * only MSCR and IMCR registers need to be revealed for kernel
> >>> +         * to configure pinmux. Please note that some indexes are reserved,
> >>> +         * such as MSCR102-MSCR111 in the following reg property.
> >>> +         */
> >>> +
> >>
> >> Either this should be part of description or should be dropped. It blows
> >> example and probably duplicates DTS.
> >>
> >>
> >> Best regards,
> >> Krzysztof
> >>
> 
> Best regards,
> Krzysztof
>
  

On 05/12/2022 12:05, Chester Lin wrote:
> On Mon, Dec 05, 2022 at 10:02:14AM +0100, Krzysztof Kozlowski wrote:
>> On 05/12/2022 07:16, Chester Lin wrote:
>>> Hi Krzysztof,
>>>
>>> On Wed, Nov 30, 2022 at 03:58:52PM +0100, Krzysztof Kozlowski wrote:
>>>> On 28/11/2022 06:48, Chester Lin wrote:
>>>>> Add DT schema for the pinctrl driver of NXP S32 SoC family.
>>>>>
>>>>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>>>>> Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
>>>>> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@nxp.com>
>>>>> Signed-off-by: Chester Lin <clin@suse.com>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - Remove the "nxp,pins" property since it has been moved into the driver.
>>>>> - Add descriptions for reg entries.
>>>>> - Refine the compatible name from "nxp,s32g-..." to "nxp,s32g2-...".
>>>>> - Fix schema issues and revise the example.
>>>>> - Fix the copyright format suggested by NXP.
>>>>>
>>>>>  .../pinctrl/nxp,s32cc-siul2-pinctrl.yaml      | 125 ++++++++++++++++++
>>>>>  1 file changed, 125 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..2fc25a9362af
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
>>>>
>>>> Usually filename matches the compatible (or family name), so any reason
>>>> why compatible is "nxp,s32g2" but filename is "nxp,s32cc"?
>>>>
>>>
>>> According to NXP, the S32CC is a microarch which is adapted by different S32 SoCs,
>>> such as S32G2/G3 and S32R45. Some common IPs are implemented in S32CC, such as
>>> serial, pinctrl, mmc, gmac and some other peripheral interfaces. S32R45 has
>>> different pinouts compared to S32G2, which means that there would not be just
>>> "s32g2-siul2-pinctrl" but also "s32r45-siul2-pinctrl" in the compatible enum if
>>> S32R45 has to be upstreamed in the future. For this case, it seems to be
>>> inappropriate that adding a compatible name without any "s32g" keyword in the
>>> filename "nxp,s32g2-.." unless creating a new yaml for each platform, such as
>>> nxp,s32r45-siul2-pinctl.yaml.
>>
>> First, you can always rename a file if such need arises. Maybe new SoCs
>> will come, maybe not.
>>
>> Second, when you actually upstream new SoC it might anyway require new
>> bindings file, because pinctrls are quite specific and it is usually
>> difficult to support multiple devices in a nice, readable way in one
>> file. Therefore anyway another file is quite likely.
>>
> 
> Thanks for your guidance. Will fix it.
> 
>> (...)
>>
>>>>> +
>>>>> +patternProperties:
>>>>> +  '-pins$':
>>>>> +    type: object
>>>>> +    additionalProperties: false
>>>>> +
>>>>> +    patternProperties:
>>>>> +      '-grp[0-9]$':
>>>>> +        type: object
>>>>> +        allOf:
>>>>> +          - $ref: pinmux-node.yaml#
>>>>> +          - $ref: pincfg-node.yaml#
>>>>> +        unevaluatedProperties: false
>>>>> +        description:
>>>>> +          Pinctrl node's client devices specify pin muxes using subnodes,
>>>>> +          which in turn use the standard properties.
>>>>
>>>> All properties are accepted? What about values, e.g. for drive strength?
>>>
>>> For those unsupported properties such as drive-strength, the s32g2 pinctrl driver
>>> returns -EOPNOTSUPP.
>>
>> I don't care what the driver is doing, we do not discuss the driver. You
>> need to describe properly the hardware and I doubt that hardware accepts
>> all drive-strengths, all forms of pull resistors (so any Ohm value).
>>
>> Add constrains.
>>
> 
> Thanks for the suggestion. IIUC, I should specifically described the supported
> pinmux and pincfg properties in this schema and then add an "additionalProperties: false"

Yes.

> in the end in order to constrain unsupported properties listed in the pattern
> pin groups.

You mean functions? or node names? The node names can be anything, can't
they?

If your drive strengths or slew rates have some limits, then I expect
them here.

Best regards,
Krzysztof