Re: [PATCH v6 04/12] dt-bindings: reset: nuvoton: add binding for ma35d1 IP reset control

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 28/03/2023 04:19, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@nuvoton.com>

Subject: drop second/last, redundant "bindings". The "dt-bindings"
prefix is already stating that these are bindings.

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

> 
> Add the dt-bindings header for Nuvoton ma35d1, that gets shared
> between the reset controller and reset references in the dts.
> Add documentation to describe nuvoton ma35d1 reset driver.
> 
> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
> ---
>  .../bindings/reset/nuvoton,ma35d1-reset.yaml  |  44 +++++++
>  .../dt-bindings/reset/nuvoton,ma35d1-reset.h  | 108 ++++++++++++++++++
>  2 files changed, 152 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
>  create mode 100644 include/dt-bindings/reset/nuvoton,ma35d1-reset.h
> 
> diff --git a/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
> new file mode 100644
> index 000000000000..342717257e5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reset/nuvoton,ma35d1-reset.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton MA35D1 Reset Controller
> +
> +maintainers:
> +  - Chi-Fang Li <cfli0@nuvoton.com>
> +  - Jacky Huang <ychuang3@nuvoton.com>
> +
> +description:
> +  The system reset controller can be used to reset various peripheral
> +  controllers in MA35D1 SoC.
> +
> +properties:
> +  compatible:
> +    const: nuvoton,ma35d1-reset
> +
> +  '#reset-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +  - '#reset-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  # system reset controller node:
> +  - |
> +
> +    system-management@40460000 {
> +        compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd";
> +        reg = <0x40460000 0x200>;
> +
> +        reset-controller {
> +            compatible = "nuvoton,ma35d1-reset";
> +            #reset-cells = <1>;

You do not take any resources here, thus this should be rather part of
the parent node.

> +        };
> +    };
> +...
> +
> diff --git a/include/dt-bindings/reset/nuvoton,ma35d1-reset.h b/include/dt-bindings/reset/nuvoton,ma35d1-reset.h
> new file mode 100644
> index 000000000000..f3088bc0afec
> --- /dev/null
> +++ b/include/dt-bindings/reset/nuvoton,ma35d1-reset.h
> @@ -0,0 +1,108 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause) */

Weird license, why 2.0+?

You already got here comment about license in previous version of this
patch.


> +/*
> + * Copyright (C) 2023 Nuvoton Technologies.
> + * Author: Chi-Fen Li <cfli0@nuvoton.com>
> + *
> + * Device Tree binding constants for MA35D1 reset controller.
> + */
> +
> +#ifndef __DT_BINDINGS_RESET_MA35D1_H
> +#define __DT_BINDINGS_RESET_MA35D1_H
> +
> +#define MA35D1_RESET_CHIP	0
> +#define MA35D1_RESET_CA35CR0	1
> +#define MA35D1_RESET_CA35CR1	2
> +#define MA35D1_RESET_CM4	3
> +#define MA35D1_RESET_PDMA0	4
> +#define MA35D1_RESET_PDMA1	5
> +#define MA35D1_RESET_PDMA2	6
> +#define MA35D1_RESET_PDMA3	7
> +#define MA35D1_RESET_DISP	9
> +#define MA35D1_RESET_VCAP0	10
> +#define MA35D1_RESET_VCAP1	11
> +#define MA35D1_RESET_GFX	12
> +#define MA35D1_RESET_VDEC	13
> +#define MA35D1_RESET_WHC0	14
> +#define MA35D1_RESET_WHC1	15
> +#define MA35D1_RESET_GMAC0	16
> +#define MA35D1_RESET_GMAC1	17
> +#define MA35D1_RESET_HWSEM	18
> +#define MA35D1_RESET_EBI	19
> +#define MA35D1_RESET_HSUSBH0	20
> +#define MA35D1_RESET_HSUSBH1	21
> +#define MA35D1_RESET_HSUSBD	22
> +#define MA35D1_RESET_USBHL	23
> +#define MA35D1_RESET_SDH0	24
> +#define MA35D1_RESET_SDH1	25
> +#define MA35D1_RESET_NAND	26
> +#define MA35D1_RESET_GPIO	27
> +#define MA35D1_RESET_MCTLP	28
> +#define MA35D1_RESET_MCTLC	29
> +#define MA35D1_RESET_DDRPUB	30
> +#define MA35D1_RESET_TMR0	34
> +#define MA35D1_RESET_TMR1	35
> +#define MA35D1_RESET_TMR2	36
> +#define MA35D1_RESET_TMR3	37
> +#define MA35D1_RESET_I2C0	40
> +#define MA35D1_RESET_I2C1	41
> +#define MA35D1_RESET_I2C2	42
> +#define MA35D1_RESET_I2C3	43
> +#define MA35D1_RESET_QSPI0	44
> +#define MA35D1_RESET_SPI0	45
> +#define MA35D1_RESET_SPI1	46
> +#define MA35D1_RESET_SPI2	47
> +#define MA35D1_RESET_UART0	48
> +#define MA35D1_RESET_UART1	49
> +#define MA35D1_RESET_UART2	50
> +#define MA35D1_RESET_UAER3	51
> +#define MA35D1_RESET_UART4	52
> +#define MA35D1_RESET_UART5	53
> +#define MA35D1_RESET_UART6	54
> +#define MA35D1_RESET_UART7	55
> +#define MA35D1_RESET_CANFD0	56
> +#define MA35D1_RESET_CANFD1	57
> +#define MA35D1_RESET_EADC0	60

Why do you have gaps here? These should be IDs, not register offsets.

Best regards,
Krzysztof