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

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

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@xxxxxxx>
> Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@xxxxxxx>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@xxxxxxx>
> Signed-off-by: Chester Lin <clin@xxxxxxxx>
> ---
> 
> 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@xxxxxxx>
> +  - Chester Lin <clin@xxxxxxxx>
> +
> +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