Re: [PATCH v22 4/8] dt-bindings: soc: nuvoton: add binding for clock and reset registers
From: Krzysztof Kozlowski <hidden>
Date: 2024-01-10 20:59:08
Also in:
linux-clk, lkml
On 08/01/2024 14:54, Tomer Maimon wrote:
A nuvoton,*-clk-rst node is present in nuvoton-common-npcm7xx.dtsi and will be added to nuvoton-common-npcm8xx.dtsi. It is necessary for the NPCM7xx and NPCM8xx clock and reset drivers, and may later be used to retrieve SoC model and version information.
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
This patch adds a binding to describe this node.
Please do not use "This commit/patch/change", but imperative mood. See longer explanation here: https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> ---
How possibly could it be v22 if there is: 1. No changelog 2. No previous submissions ? NAK, it's something completely new without any explanation. Limited review follows.
quoted hunk ↗ jump to hunk
.../soc/nuvoton/nuvoton,npcm-clk-rst.yaml | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yamldiff --git a/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml new file mode 100644 index 000000000000..dfec64a8eb26 --- /dev/null +++ b/Documentation/devicetree/bindings/soc/nuvoton/nuvoton,npcm-clk-rst.yaml@@ -0,0 +1,40 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/soc/nuvoton/nuvoton,npcm-clk-rst.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Clock and reset registers block in Nuvoton SoCs
This is vague. Any block? All blocks? Your SoC has only one block? I doubt, although possible. Anyway, clocks go to clock directory, not to soc! We've been here and you already received that feedback.
+ +maintainers: + - Tomer Maimon [off-list ref] + +description: + The clock and reset registers are a registers block in Nuvoton SoCs that + handle both reset and clock functionality.
That's still vague. Say something useful.
+ +properties: + compatible: + items: + - enum: + - nuvoton,npcm750-clk-rst + - nuvoton,npcm845-clk-rst + - const: syscon + - const: simple-mfd
No, it's not a syscon and not a simple-mfd. You just said it is clock provider and reset controller. Thus missing clock cells and reset cells.
+ + reg: + maxItems: 1 + +required: + - compatible + - reg + +additionalProperties: + type: object
No, instead: additionalProperties: false
+
+examples:
+ - |
+ clk_rst: syscon@801000 {Suddenly a syscon? Drop unused label.
+ compatible = "nuvoton,npcm750-clk-rst", "syscon", "simple-mfd"; + reg = <0x801000 0x6C>;
Only lowercase hex. You just sent some v22 of something new, making all the mistakes from the past submissions for which you received feedback.
+ };
Best regards, Krzysztof