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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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