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. >> >> >>> + $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. > $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. > > > >>> + >>> + 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. [.] is redundant... What exactly do you want to match? > > >>> + 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. OK, then go with nuvoton approach. List the properties (:true) and use additionalProperties: false. > >>> + >>> + 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. Instead treat it as mA. Is this possible? > >>> + 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". Rob, Linus, any ideas for generic property replacing register-specific power-source? Best regards, Krzysztof
