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

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 18/10/2023 05:26, Jacky Huang wrote:
> Dear Krzysztof,
> 
> Thank you for the review.
> 
> 
> On 2023/10/17 上午 03:52, Krzysztof Kozlowski wrote:
>> On 16/10/2023 06:32, Jacky Huang wrote:
>>>>> +  '#size-cells':
>>>>> +    const: 1
>>>>> +
>>>>> +  nuvoton,sys:
>>>>> +    description:
>>>>> +      phandle to the syscon node
>>>> sys is quite generic. Description explains nothing except duplicating
>>>> known information. Drop duplicated info and instead explain to what this
>>>> phandle points and how it is going to be used.
>> Read comments carefully.
> 
> 
> I will update the description of 'nuvoton,sys'.

What is the full name of destination block?

> 
>>
>>>>
>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>>>> +    items:
>>>>> +      maxItems: 1
>>>> So just phandle, not phandle-array, unless it is defined like this in
>>>> some other binding.
>>> I would like to update this as:
>>>
>>>     nuvoton,sys:
>> Nothing improved.
> 
> Here just fix  the 'phandle-array' to 'phandle' and remove 'maxItems'.
> 
>>>       $ref: /schemas/types.yaml#/definitions/phandle
>>>       description:
>>>         Help pinctrl driver to access system registers by means of regmap.
>> Driver is not relevant here. Say which part of syscon are necessary for
>> pinctrl operation.
>>
> 
> I will update description as:
> 
>    nuvoton,sys:
>      $ref: /schemas/types.yaml#/definitions/phandle
>      description:
>        The pin function control registers are located in the system
>        control register space. This phandle provides pinctrl the
>        ability to access the pin function control registers through
>        the use of regmap.

regmap is unrelated to the bindings.

> 
>>>>> +        maximum: 7
>>>>> +
>>>>> +      input-enable: true
>>>>> +
>>>>> +      input-schmitt-enable: true
>>>>> +
>>>>> +      power-source:
>>>>> +        description:
>>>>> +          I/O voltage in millivolt.
>>>>> +        enum: [ 1800, 3300 ]
>>>> Missing units in property name. power-source also does not really
>>>> describe the property.
>>>
>>> The output voltage level of GPIO can be configured as 1.8V or 3.3V,
>>> but I cannot find any suitable output properties in 'pincfg-node.yaml.'
>> There is actually power-source, but treated as actual choice of power
>> supplies.
>>
>>> I noticed that 'xlnx,zynq-pinctrl.yaml' and 'xlnx,zynq-pinctrl.yaml' use
>>> 'power source' to specify the output voltage.  Should I follow their
>>> approach or define a vendor-specific one?
>> Maybe Rob or Linus have here some recommendation, but I would suggest to
>> go either with rtd1319d-pinctrl.yaml approach or add a generic property
>> to pincfg-node expressed in real units like "io-microvolt".
> 
> OK, I will update it as:
> 
>        power-source:
>          description: |
>            Valid arguments are described as below:
>            0: power supply of 1.8V
>            1: power supply of 3.3V
>          enum: [0, 1]
> 
> 
>> Rob, Linus, any ideas for generic property replacing register-specific
>> power-source?

I proposed io-microvolt

Best regards,
Krzysztof