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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 26/08/2024 03:36, 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@gentoo.org>
>>> ---
>>>  .../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
>>> +      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

Don't store here register value.

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

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

Test your patches instead.



Best regards,
Krzysztof