| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <34a73de2-85b3-4a20-b2b4-6a72622d5abc@kernel.org>
Date: Mon, 26 Aug 2024 07:37:34 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Yixun Lan <dlan@...too.org>
Cc: Linus Walleij <linus.walleij@...aro.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
Conor Dooley <conor@...nel.org>, Yangyu Chen <cyy@...self.name>,
Jesse Taube <jesse@...osinc.com>, Jisheng Zhang <jszhang@...nel.org>,
Inochi Amaoto <inochiama@...look.com>, Icenowy Zheng <uwu@...nowy.me>,
Meng Zhang <zhangmeng.kevin@...cemit.com>, Meng Zhang
<kevin.z.m@...mail.com>, devicetree@...r.kernel.org,
linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org
Subject: Re: [PATCH v2 1/4] dt-binding: pinctrl: spacemit: add documents for
K1 SoC
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@...too.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@...too.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@...1e000 {
>>> + 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
Powered by blists - more mailing lists