Re: [PATCH v2 2/4] dt-bindings: pinctrl: Document nuvoton ma35d1 pin control

[Krzysztof Kozlowski]

On 28/11/2023 09:29, Jacky Huang wrote:
> Dear Krzysztof,
> 
> Thanks for your review.
> 
> 
> On 2023/11/28 下午 03:34, Krzysztof Kozlowski wrote:
>> On 28/11/2023 07:11, Jacky Huang wrote:
>>> From: Jacky Huang <ychuang3@xxxxxxxxxxx>
>>>
>>> Add documentation to describe nuvoton ma35d1 pin control and GPIO.
>>>
>>> Signed-off-by: Jacky Huang <ychuang3@xxxxxxxxxxx>
>>> ---
>> Your changelog said:
>>
>>>    - Remove ma35d1-pinfunc.h which is unused after update definition of
>> 'nuvoton,pins'.
>>
>> You forgot to add:
>>
>> " - Do not test the bindings before sending"
>>
>> I assume none of the driver changes compile either.
> 
> It's my mistake. I forgot to remove 'ma35d1-pinfunc.h' from my local 
> copy, and as a consequence, the 'dt_binding_check' did not catch this 
> error. I will fix this.

But then git status would point you that tree is not clean and you did
not finish commiting.

..


>>> +
>>> +allOf:
>>> +  - $ref: pinctrl.yaml#
>> allOf goes before additionalProperties.
>>
>>> +
>>> +required:
>>> +  - compatible
>>> +  - nuvoton,sys
>> This goes after patternProperties
> 
> I will fix the above two as:
> 
> allOf:
>    - $ref: pinctrl.yaml#

Look:

>> allOf goes before additionalProperties.

Open example-schema.

..

> 
>>> +      - $ref: pincfg-node.yaml#
>>> +
>>> +    properties:
>>> +      bias-disable: true
>> Drop this and other "true", why do you need them here?
> 
> We are following the conventions used in other pinctrl documents, such as
> 'realtek,rtd1315e-pinctrl.yaml' and 'xlnx,zynqmp-pinctrl.yaml'.

But they are quite different there.

> 
> After comparing various pinctrl documents, I noticed that they all express
> it as 'bias-disable: true'. Therefore, may I keep the current format?

No, you cannot copy pieces of other binding, selectively ignoring the
rest. Look how these other bindings are constructed - they have
additionalProperties, which you don't.

Drop all these true properties if the only reason of them being here is
they were copied.



Best regards,
Krzysztof