Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e6ee1ec2-1649-3e7e-9dbe-e94d636d8476@linaro.org>
Date:   Mon, 5 Dec 2022 14:26:59 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.1
Subject: Re: [PATCH v2 1/2] dt-bindings: pinctrl: add schema for NXP S32 SoCs
Content-Language: en-US
To:     Chester Lin <clin@suse.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        s32@nxp.com, linux-gpio@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        Larisa Grigore <larisa.grigore@nxp.com>,
        Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>,
        Andrei Stefanescu <andrei.stefanescu@nxp.com>,
        =?UTF-8?Q?Andreas_F=c3=a4rber?= <afaerber@suse.de>,
        Matthias Brugger <mbrugger@suse.com>,
        ghennadi.procopciuc@oss.nxp.com
References: <20221128054820.1771-1-clin@suse.com>
 <20221128054820.1771-2-clin@suse.com>
 <6ad95ce3-887d-48fd-3c08-f50d4e666ded@linaro.org>
 <Y42MyyLumVa8phpd@linux-8mug>
 <e839274d-6696-63aa-14e6-f52a534c9ed2@linaro.org>
 <Y43Qdco0y61C4l1I@linux-8mug>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <Y43Qdco0y61C4l1I@linux-8mug>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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