Re: [PATCH v2 1/4] dt-binding: pinctrl: spacemit: add documents for K1 SoC

[Inochi Amaoto <inochiama@xxxxxxxxxxx>]

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