| 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: <20250721-wonderful-auk-of-whirlwind-8beda4@kuoka>
Date: Mon, 21 Jul 2025 10:04:42 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: jeff_chang@...htek.com
Cc: lgirdwood@...il.com, broonie@...nel.org, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org
Subject: Re: [PATCH v3 2/2] dt-bindings: regulator: Add Richtek RTR5133 DT
Binding Documentation
On Mon, Jul 21, 2025 at 02:01:35PM +0800, jeff_chang@...htek.com wrote:
> From: Jeff Chang <jeff_chang@...htek.com>
>
Missing commit msg.
Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.
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
I already asked for it. Did you really read what I linked last time?
A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
I already asked for it, so you ignored another comment. Why are you not
responding or implementing the comments?
Bindings go before users, so please re-order patches (see submitting
patches in DT dir).
> Signed-off-by: Jeff Chang <jeff_chang@...htek.com>
> ---
>
> PATCH v3
> 1. fix Subject format
> 2. using correct patches version
> 3. remove '|'
> 4. remove allOf: &ref regulator.yaml#
> 5. remove redundant description
> 6. move BASE to base property with correct indentation
> 7. only using lowercase node name
> 8. make DT_CHECKER_FLAG=-m DT_SCHEMA_FILES=richtek,rt5133.yaml dt_binding_check pass
>
>
> .../bindings/regulator/richtek,rt5133.yaml | 197 ++++++++++++++++++
> 1 file changed, 197 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt5133.yaml
>
> diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt5133.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt5133.yaml
> new file mode 100644
> index 000000000000..a92e7f775832
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/richtek,rt5133.yaml
> @@ -0,0 +1,197 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/richtek,rt5133.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Richtek RT5133 PMIC Regulator
> +
> +maintainers:
> + - ShihChia Chang <jeff_chang@...htek.com>
> +
> +description:
> + RT5133 is an integrated chip. It includes 8 LDOs and 3 GPOs that can be
> + used to drive output high/low purpose. The dependency of the GPO block
> + is internally LDO1 Voltage. If LDO1 voltage output disabled, GPO cannot
> + be used to drive output high. It need to pay more attention on the usage.
Last statement does not feel relevant.
> +
> +properties:
> + compatible:
> + enum:
> + - richtek,rt5133
> +
> + reg:
> + maxItems: 1
> +
> + enable-gpios:
> + maxItems: 1
> +
> + wakeup-source: true
> +
> + interrupts:
> + maxItems: 1
> +
> + gpio-controller: true
> +
> + "#gpio-cells":
> + const: 2
> +
> + regulators:
> + type: object
> + additionalProperties: false
> +
> + properties:
> + base:
> + type: object
> + $ref: regulator.yaml#
> + unevaluatedProperties: false
> + description:
> + Properties for base regulator which for force-off base circuit
That's not a regulator supply or you need to provide proper description
of hardware.
> +
> + properties:
> + regulator-compatible:
> + description: Compatible string for regulator
> + $ref: /schemas/types.yaml#/definitions/string
Drop property.
> +
> + oc-shutdown-all:
Missing vendor prefix.
> + type: boolean
> + description:
> + Anyone of LDO OC state, shut down all channels.
I don't understand the description at all.
> +
> + pgb-shutdown-all:
> + type: boolean
> + description:
> + Anyone of PGB OC state, shut down all channels.
Same problems here
> +
> + required:
> + - regulator-name
> + - regulator-compatible
No, drop compatible. Please read the bindings.
> +
> + patternProperties:
> + "^ldo([1-6])$":
> + type: object
> + $ref: regulator.yaml#
> + unevaluatedProperties: false
> + description:
> + Properties for single LDO regulator
> +
> + properties:
> + regulator-compatible:
> + description: Compatible string for regulator
> + $ref: /schemas/types.yaml#/definitions/string
> +
> + required:
> + - regulator-name
> + - regulator-compatible
> +
> + "^ldo([7-8])$":
> + type: object
> + $ref: regulator.yaml#
> + unevaluatedProperties: false
> + description:
> + Properties for single LDO regulator
> +
> + properties:
> + regulator-compatible:
> + description: Compatible string for regulator
> + $ref: /schemas/types.yaml#/definitions/string
> +
> + rt5133-ldo1-supply:
> + description: |
> + Only for ldo7 ldo8, pvin7 and pvin8 reference design are RT5133 ldo1.
> + If not connect to ldo1 vout, this property for pvin7 and pvin8 is necessary.
> +
> + required:
> + - regulator-name
> + - regulator-compatible
> +
> +required:
> + - compatible
> + - reg
> + - interrupts-extended
interrupts instead
> + - wakeup-source
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + rt5133: rt5133@18 {
Drop unused labels.
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "richtek,rt5133";
> + reg = <0x18>;
> + wakeup-source;
> + interrupts-extended = <&pio 187 0x0>;
Use proper defines
> + enable-gpios = <&pio 186 0x0>;
Use proper defines.
> + gpio-controller;
> + #gpio-cells = <2>;
> + regulators {
> + base {
> + regulator-compatible = "BASE";
> + regulator-name = "rt5133,base";
> + oc-shutdown-all;
> + pgb-shutdown-all;
> + };
> + rt5133_ldo1: ldo1 {
Drop unused labels.
Best regards,
Krzysztof
Powered by blists - more mailing lists