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

On 16/10/2023 06:32, Jacky Huang wrote:

quoted

quoted

+  '#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.

quoted

quoted

+    $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.

quoted

quoted

+
+  ranges: true
+
+allOf:
+  - $ref: pinctrl.yaml#

allOf: goes after required: block.

I will fix it.

quoted

quoted

+
+patternProperties:
+  "gpio[a-n]@[0-9a-f]+$":

^gpio@[0-9a-f]+$":

I will fix this, and also fix the dtsi.

quoted

quoted

+    type: object
+    additionalProperties: false
+    properties:
+

Drop blank line

I will fix it.

quoted

quoted

+      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.

quoted

quoted

+
+  "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?

quoted

quoted

+    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.

quoted

quoted

+
+    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.

quoted

quoted

+
+      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?

quoted

quoted

+        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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel