On Mon, Aug 26, 2024 at 01:36:35AM GMT, Yixun Lan wrote: > Hi Krzysztof: > > On 15:48 Sun 25 Aug , Krzysztof Kozlowski wrote: > > 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:" > Ok, will fix in next version > > > > > > > > > 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 > > > > > > > > > > > Signed-off-by: Yixun Lan <dlan@xxxxxxxxxx> > > > --- > > > .../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@xxxxxxxxxx> > > > + > > > +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 > > > + A pinctrl node should contain at least one subnode representing the > > > + pinctrl groups available on the machine. > > > + > > > + additionalProperties: false > > > > Keep it before description. > Ok > > > > > + > > > + 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 > > > > > + > > > + 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 > > > + > > > + 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 > I suspect this should not be set separately, but with the drive-strength. The document shows that the DS field sets both drive strength and slew rate. This at least tell the value 0 and 1 may be different. > > > + 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 > > > > + 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. > > > > + > > > + 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) > > I will keep this property unless there is objection, please let me know > > > > + type: boolean > > > + > > > + required: > > > + - pinmux > > > + > > > + additionalProperties: false > > > > This goes up, before description. > > > Ok > > > + > > > +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 > > > > > 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@xxxxxxxxxx> > > > + * > > > + */ > > > + > > > +#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. > > > Ok, I will move them to dts folder, which should be file > arch/riscv/boot/dts/spacemit/k1-pinctrl.h > > > ... > > > > > + > > > +#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. > Ok, will drop it > > > > > > + > > > +#define K1_PADCONF(pin, func) (((pin) << 16) | (func)) > > > > Not a binding. > > > same, move to dts > > > > > > > Best regards, > > Krzysztof > > -- > Yixun Lan (dlan) > Gentoo Linux Developer > GPG Key ID AABEFD55
