Re: [PATCH v22 4/8] dt-bindings: soc: nuvoton: add binding for clock and reset registers

[Krzysztof Kozlowski]

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@xxxxxxxxx>
> ---

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.


>  .../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.yaml
> 
> diff --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 <tmaimon77@xxxxxxxxx>
> +
> +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