Re: [PATCH v2 1/4] dt-binding: pinctrl: spacemit: add documents for K1 SoC
From: Krzysztof Kozlowski <krzk@kernel.org>
Date: 2024-08-25 13:48:13
Also in:
linux-gpio, linux-riscv, lkml
On 25/08/2024 15:10, Yixun Lan wrote:
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:"
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
quoted hunk ↗ jump to hunk
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.
+ A pinctrl node should contain at least one subnode representing the + pinctrl groups available on the machine. + + additionalProperties: false
Keep it before description.
+ + 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.
+ + 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?
+ + 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?
+ 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?
+ 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.
+ + 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.
+ type: boolean + + required: + - pinmux + + additionalProperties: false
This goes up, before description.
+
+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... :( ...
quoted hunk ↗ jump to hunk
diff --git a/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h b/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h new file mode 100644 index 0000000000000..13ef4aa6c53a3 --- /dev/null +++ b/include/dt-bindings/pinctrl/spacemit,k1-pinctrl.h@@ -0,0 +1,161 @@ +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ +/* + * Copyright (c) 2022-2024 SpacemiT (Hangzhou) Technology Co. Ltd + * Copyright (c) 2024 Yixun Lan <dlan@gentoo.org> + * + */ + +#ifndef _DT_BINDINGS_PINCTRL_K1_H +#define _DT_BINDINGS_PINCTRL_K1_H + +#define PINMUX(pin, mux) \ + (((pin) & 0xffff) | (((mux) & 0xff) << 16)) + +/* pin offset */ +#define PINID(x) ((x) + 1) + +#define GPIO_INVAL 0 +#define GPIO_00 PINID(0)
Not really, pin numbers are not bindings. Drop entire header. ...
+ +#define SLEW_RATE_SLOW0 0 +#define SLEW_RATE_SLOW1 1 +#define SLEW_RATE_MEDIUM 2 +#define SLEW_RATE_FAST 3
Not a binding, either. No usage in the driver.
+ +#define K1_PADCONF(pin, func) (((pin) << 16) | (func))
Not a binding. Best regards, Krzysztof