Messages in this thread Patch in this message | | | Date | Mon, 16 Oct 2023 12:32:14 +0800 | From | Jacky Huang <> | Subject | Re: [PATCH 2/4] dt-bindings: pinctrl: Document nuvoton ma35d1 pin control |
| |
Dear Krzysztof,
Thank you for the review.
On 2023/10/13 上午 03:41, Krzysztof Kozlowski wrote: > On 11/10/2023 11:05, Jacky Huang wrote: >> From: Jacky Huang<ychuang3@nuvoton.com> >> >> Add the dt-bindings header for nuvoton ma35d1 pinctrl, that gets shared >> between the pin control driver and pin configuration in the dts. >> >> Add documentation to describe nuvoton ma35d1 pin control and GPIO. >> >> Signed-off-by: Jacky Huang<ychuang3@nuvoton.com> >> --- >> .../pinctrl/nuvoton,ma35d1-pinctrl.yaml | 180 ++++++++++++++++++ >> include/dt-bindings/pinctrl/ma35d1-pinfunc.h | 38 ++++ >> 2 files changed, 218 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml >> create mode 100644 include/dt-bindings/pinctrl/ma35d1-pinfunc.h >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml >> new file mode 100644 >> index 000000000000..0ddedbad4b78 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml >> @@ -0,0 +1,180 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id:http://devicetree.org/schemas/pinctrl/nuvoton,ma35d1-pinctrl.yaml# >> +$schema:http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Nuvoton MA35D1 pin control and GPIO >> + >> +maintainers: >> + - Shan-Chun Hung<schung@nuvoton.com> >> + >> +properties: >> + compatible: >> + enum: >> + - nuvoton,ma35d1-pinctrl >> + >> + '#address-cells': >> + const: 1 >> + >> + '#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. > > >> + $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: $ref: /schemas/types.yaml#/definitions/phandle description: Help pinctrl driver to access system registers by means of regmap.
>> + >> + ranges: true >> + >> +allOf: >> + - $ref: pinctrl.yaml# > allOf: goes after required: block.
I will fix it.
>> + >> +patternProperties: >> + "gpio[a-n]@[0-9a-f]+$": > ^gpio@[0-9a-f]+$":
I will fix this, and also fix the dtsi.
>> + type: object >> + additionalProperties: false >> + properties: >> + > Drop blank line
I will fix it.
>> + gpio-controller: true >> + >> + '#gpio-cells': >> + const: 2 >> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + maxItems: 1 >> + >> + interrupt-controller: true >> + >> + '#interrupt-cells': >> + const: 2 >> + >> + interrupts: >> + description: >> + The interrupt outputs to sysirq. >> + maxItems: 1 >> + >> + required: >> + - reg >> + - interrupts >> + - interrupt-controller >> + - '#interrupt-cells' >> + - gpio-controller >> + - '#gpio-cells' > Keep the same order as in list of properties.
I will fix the order.
>> + >> + "pcfg-[a-z0-9-.]+$": > Why using different naming than other Nuvoton SoCs? You also accept > "foobarpcfg-1", which does not look intentional. >
I will use '"^pin-[a-z0-9-.]+$" instead.
>> + type: object >> + description: >> + A pinctrl node should contain at least one subnodes representing the >> + pinctrl groups available on the machine. Each subnode will list the >> + pins it needs, and how they should be configured, with regard to muxer >> + configuration, pullups, drive strength, input enable/disable and input >> + schmitt. >> + >> + allOf: >> + - $ref: pincfg-node.yaml# > missing additional/unevaluatedProperties: false.
I will add unevaluatedProperties: false.
>> + >> + properties: >> + bias-disable: true > Why do you need this and other ones?
We expect the pin configuration to select one of ==> bias-disable; bias-pull-down; bias-pull-up;
This is the same as rockchip,pinctrl.yaml and renesas,rzv2m-pinctrl.yaml.
>> + >> + bias-pull-down: true >> + >> + bias-pull-up: true >> + >> + drive-strength: >> + minimum: 0 > 0 mA? Is it really valid? Are you sure you used correct property?
We treat this value as the value to be written to the control register, not as a current value in mA. I will correct this mistake.
>> + 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.' 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?
>> + >> +additionalProperties: >> + type: object >> + additionalProperties: >> + type: object > Wait, what? What are you describing here?
I will fix it as:
"-grp[0-9]$": type: object description: Pinctrl node's client devices use subnodes for desired pin configuration. Client device subnodes use below standard properties. properties: nuvoton,pins: ....
and fix the example dts also.
>> + properties: >> + nuvoton,pin: >> + description: >> + Each entry consists of 4 parameters and represents the mux and config >> + setting for one pin. >> + $ref: /schemas/types.yaml#/definitions/uint32-matrix >> + minItems: 1 >> + items: >> + items: >> + - minimum: 0x80 >> + maximum: 0xec >> + description: >> + The pinctrl register offset in syscon registers. >> + - minimum: 0 >> + maximum: 30 >> + description: >> + The bit offset in the pinctrl register. >> + - minimum: 0 >> + maximum: 15 >> + description: >> + The multi-function pin value. >> + - description: >> + The phandle of a node contains the generic pinconfig options >> + to use as described in pinctrl-bindings.txt. >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + #include <dt-bindings/gpio/gpio.h> >> + #include <dt-bindings/clock/nuvoton,ma35d1-clk.h> >> + #include <dt-bindings/pinctrl/ma35d1-pinfunc.h> >> + >> + pinctrl@40040000 { >> + compatible = "nuvoton,ma35d1-pinctrl"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + nuvoton,sys = <&sys>; >> + ranges = <0 0x40040000 0xc00>; >> + >> + gpioa@40040000 { >> + reg = <0x0 0x40>; >> + interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&clk GPA_GATE>; >> + gpio-controller; >> + #gpio-cells = <2>; >> + interrupt-controller; >> + #interrupt-cells = <2>; >> + }; >> + >> + pcfg_default: pcfg-default { >> + slew-rate = <0>; >> + input-schmitt-disable; >> + bias-disable; >> + power-source = <3300>; >> + drive-strength = <0>; > Really 0 mA? > > Why this is so incomplete?
We treat this value as the value to be written to the control register, not as a current value in mA. I will correct this mistake.
>> + }; >> + }; >> + >> + pinctrl {> + uart13 { >> + pinctrl_uart13: uart13grp { > According to your bindings this does not belong here.
I will fix.
>> + nuvoton,pins = >> + <MA35_SYS_REG_GPH_H 24 2 &pcfg_default>, >> + <MA35_SYS_REG_GPH_H 28 2 &pcfg_default>; >> + }; >> + }; >> + }; >> + >> + serial@407d0000 { > Drop node, not related at all.
Okay, I will drop this node.
>> + compatible = "nuvoton,ma35d1-uart"; >> + reg = <0x407d0000 0x100>; >> + interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&clk UART13_GATE>; >> + pinctrl-0 = <&pinctrl_uart13>; >> + }; >> diff --git a/include/dt-bindings/pinctrl/ma35d1-pinfunc.h b/include/dt-bindings/pinctrl/ma35d1-pinfunc.h >> new file mode 100644 >> index 000000000000..a2609d466dc9 >> --- /dev/null >> +++ b/include/dt-bindings/pinctrl/ma35d1-pinfunc.h > Filename matching bindings. The same name. > >> @@ -0,0 +1,38 @@ >> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) */ >> +/* >> + * Copyright (C) 2023 Nuvoton Technologies. >> + */ >> + >> +#ifndef __DT_BINDINGS_PINCTRL_NUVOTON_MA35D1_H >> +#define __DT_BINDINGS_PINCTRL_NUVOTON_MA35D1_H >> + >> +#define MA35_SYS_REG_GPA_L 0x80 > Registry addresses are not suitable for bindings. There is also no need > to have REG address in the binding. Drop entire file. > Best regards, > Krzysztof >
I will remove 'ma35d1-pinfunc.h' as it will be useless after the 'nuvoton,pin' definition changed. Best Regards, Jacky Huang
| |