On 26/08/2024 03:36, Yixun Lan wrote:

Hi Krzysztof: 

On 15:48 Sun 25 Aug     , Krzysztof Kozlowski wrote:

quoted

On 25/08/2024 15:10, Yixun Lan wrote:

quoted

Add dt-binding for the pinctrl driver of SpacemiT's K1 SoC.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

It's "dt-bindings:"

Ok, will fix in next version

quoted

quoted

Two vendor specific properties are introduced here, As the pinctrl
has dedicated slew rate enable control - bit[7], so we have
spacemit,slew-rate-{enable,disable} for this. For the same reason,
creating spacemit,strong-pull-up for the strong pull up control.

Huh, no, use generic properties. More on that below

see my reply below

quoted

quoted

Signed-off-by: Yixun Lan <redacted>
---
 .../bindings/pinctrl/spacemit,k1-pinctrl.yaml      | 134 +++++++++++++++++
 include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h  | 161 +++++++++++++++++++++
 2 files changed, 295 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml
new file mode 100644
index 0000000000000..8adfc5ebbce37
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/spacemit,k1-pinctrl.yaml

@@ -0,0 +1,134 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/spacemit,k1-pinctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SpacemiT K1 SoC Pin Controller
+
+maintainers:
+  - Yixun Lan <dlan@gentoo.org>
+
+properties:
+  compatible:
+    const: spacemit,k1-pinctrl
+
+  reg:
+    items:
+      - description: pinctrl io memory base
+
+patternProperties:
+  '-cfg$':
+    type: object
+    description: |

Do not need '|' unless you need to preserve formatting.

Ok

quoted

quoted

+      A pinctrl node should contain at least one subnode representing the
+      pinctrl groups available on the machine.
+
+    additionalProperties: false

Keep it before description.

Ok

quoted

quoted

+
+    patternProperties:
+      '-pins$':
+        type: object
+        description: |
+          Each subnode will list the pins it needs, and how they should
+          be configured, with regard to muxer configuration, bias, input
+          enable/disable, input schmitt trigger, slew-rate enable/disable,
+          slew-rate, drive strength, power source.
+        $ref: /schemas/pinctrl/pincfg-node.yaml
+
+        allOf:
+          - $ref: pincfg-node.yaml#
+          - $ref: pinmux-node.yaml#

You are duplicating refs.

ok, will fix it

quoted

quoted

+
+        properties:
+          pinmux:
+            description: |
+              The list of GPIOs and their mux settings that properties in the
+              node apply to. This should be set using the K1_PADCONF macro to
+              construct the value.
+            $ref: /schemas/pinctrl/pinmux-node.yaml#/properties/pinmux

Hm why you need the ref?

will drop it

quoted

quoted

+
+          bias-disable: true
+
+          bias-pull-up: true
+
+          bias-pull-down: true
+
+          drive-strength-microamp:
+            description: |
+              typical current when output high level, but in mA.
+              1.8V output: 11, 21, 32, 42 (mA)
+              3.3V output: 7, 10, 13, 16, 19, 23, 26, 29 (mA)
+            $ref: /schemas/types.yaml#/definitions/uint32
+
+          input-schmitt:
+            description: |
+              typical threshold for schmitt trigger.
+              0: buffer mode
+              1: trigger mode
+              2, 3: trigger mode
+            $ref: /schemas/types.yaml#/definitions/uint32
+            enum: [0, 1, 2, 3]
+
+          power-source:
+            description: external power supplies at 1.8v or 3.3v.
+            enum: [ 1800, 3300 ]
+
+          slew-rate:
+            description: |
+              slew rate for output buffer
+              0, 1: Slow speed

Hm? Surprising, 0 is slow speed?

from docs, section 3.3.2.2 MFPR Register Description
0, 1 are same, both for slow speed
https://developer.spacemit.com/documentation?token=An1vwTwKaigaXRkYfwmcznTXned

Don't store here register value.

quoted

quoted

+              2: Medium speed
+              3: Fast speed
+            $ref: /schemas/types.yaml#/definitions/uint32
+            enum: [0, 1, 2, 3]
+
+          spacemit,slew-rate-enable:
+            description: enable slew rate.

The presence of slew-rate enables it, doesn't it?

yes, this should work, I will take this approach, thanks

quoted

quoted

+            type: boolean
+
+          spacemit,slew-rate-disable:
+            description: disable slew rate.
+            type: boolean

Just use slew-rate, 0 disable, some value to match real slew-rate.

sounds good to me, since 0, 1 indicate same meaning, can re-use 0 for
disabling slew rate.

quoted

quoted

+
+          spacemit,strong-pull-up:
+            description: enable strong pull up.

Do not duplicate the property name in description. You did not say
anything useful here. What is "strong"? bias-pull-up takes also an argument.

there is a dedicated strong pull bit[3] for I2C, SD card kinds of pad
I don't know how 'strong' it is if in ohms, will see if can get
more info on this (may expand the description)

I think using 'bias-pull-up' property with argument should also work,
but it occur to me it's more intuitive to introduce a property here, which
reflect the underlying hardware functionality. this is similar to starfive's jh7100
bindings/pinctrl/starfive,jh7100-pinctrl.yaml:154
(refer to exist code doesn't mean always correct, so I need advice here)

No, avoid introducing properties which duplicate existing generic ones.

Same story was with qualcomm and it was possible to use specific value.

I will keep this property unless there is objection, please let me know

quoted

quoted

+            type: boolean
+
+        required:
+          - pinmux
+
+        additionalProperties: false

This goes up, before description.

Ok

quoted

quoted

+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/pinctrl/spacemit,k1-pinctrl.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pinctrl@d401e000 {
+            compatible = "spacemit,k1-pinctrl";
+            reg = <0x0 0xd401e000 0x0 0x400>;
+            #pinctrl-cells = <2>;
+            #gpio-range-cells = <3>;

This wasn't ever tested... :(
...

will drop it

Test your patches instead.



Best regards,
Krzysztof