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

[Chester Lin <clin@suse.com>]

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
>