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

Krzysztof Kozlowski
2023-10-18T05:58:23Z

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