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

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

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@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"?
>>
> 
> 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