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

[Krzysztof Kozlowski]

On 29/03/2023 11:12, Jacky Huang wrote:
>>> +        };
>>> +    };
>>> +...
>>> +
>>> 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 '+' ?

The simplest is to use the same license as your binding, so
(GPL-2.0-only OR BSD-2-Clause) .

> 
>>> +/*
>>> + * Copyright (C) 2023 Nuvoton Technologies.
>>> + * Author: Chi-Fen Li <cfli0@xxxxxxxxxxx>
>>> + *
>>> + * 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.

That's not the usual way. IDs start from 0 or 1 and gets incremented by
1, without gaps. Remember that IDs cannot change and your register bits
could have some changes (e.g. HW errata).


Best regards,
Krzysztof