Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1ddf53be-3c4f-3f24-de69-9475c82341b1@gmail.com>
Date:   Wed, 29 Mar 2023 17:12:52 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v6 04/12] dt-bindings: reset: nuvoton: add binding for
 ma35d1 IP reset control
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        lee@kernel.org, mturquette@baylibre.com, sboyd@kernel.org,
        p.zabel@pengutronix.de, gregkh@linuxfoundation.org,
        jirislaby@kernel.org
Cc:     devicetree@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        arnd@arndb.de, schung@nuvoton.com, mjchen@nuvoton.com,
        Jacky Huang <ychuang3@nuvoton.com>
References: <20230328021912.177301-1-ychuang570808@gmail.com>
 <20230328021912.177301-5-ychuang570808@gmail.com>
 <96bbb16a-1748-c0cb-0fc6-90804eab01c1@linaro.org>
From:   Jacky Huang <ychuang570808@gmail.com>
In-Reply-To: <96bbb16a-1748-c0cb-0fc6-90804eab01c1@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Dear Krzysztof,


On 2023/3/29 下午 04:17, Krzysztof Kozlowski wrote:
> 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.

I will modify it as:
     sys: system-management@40460000 {
         compatible = "nuvoton,ma35d1-sys", "syscon",;
         reg = <0x0 0x40460000 0x0 0x200>;
         #reset-cells = <1>;
         };
     };


>> +        };
>> +    };
>> +...
>> +
>> 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.
>

In fact, I always be confused with the license.
It should be right if I just copy one from the same directory 
/include/dt-bindings/reset/.
But, when I find there are several versions there, I was confused and 
just select one of them.

/* SPDX-License-Identifier: GPL-2.0-only */
/* SPDX-License-Identifier: GPL-2.0+ */
/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)*/
/* SPDX-License-Identifier: (GPL-2.0+ or MIT) */
/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */

So, should I fix the license as

/* SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) */

just remove the '+' ?

>> +/*
>> + * 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
>

The reset controller registers are composed of four 32-bits register.
And the ID will be translated into the corresponding bit of these register.
Each ID is mapped to a unique bit position.


Best regards,
Jacky Huang