Re: [PATCH 1/3] dt-bindings: reset: Add Amlogic T7 Reset Controller

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

On 12/04/2024 19:57, Krzysztof Kozlowski wrote:
> On 12/04/2024 15:12, Neil Armstrong wrote:
>> Hi,
>>
>> On 29/03/2024 20:39, Krzysztof Kozlowski wrote:
>>> On 29/03/2024 10:17, Kelvin Zhang via B4 Relay wrote:
>>>> From: Zelong Dong <zelong.dong@amlogic.com>
>>>>
>>>> Add a new compatible and the related header file
>>>> for Amlogic T7 Reset Controller.
>>>>
>>>> Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
>>>> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
>>>> ---
>>>>   .../bindings/reset/amlogic,meson-reset.yaml        |   1 +
>>>>   include/dt-bindings/reset/amlogic,t7-reset.h       | 197 +++++++++++++++++++++
>>>>   2 files changed, 198 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/reset/amlogic,meson-reset.yaml b/Documentation/devicetree/bindings/reset/amlogic,meson-reset.yaml
>>>> index f0c6c0df0ce3..fefe343e5afe 100644
>>>> --- a/Documentation/devicetree/bindings/reset/amlogic,meson-reset.yaml
>>>> +++ b/Documentation/devicetree/bindings/reset/amlogic,meson-reset.yaml
>>>> @@ -19,6 +19,7 @@ properties:
>>>>         - amlogic,meson-a1-reset # Reset Controller on A1 and compatible SoCs
>>>>         - amlogic,meson-s4-reset # Reset Controller on S4 and compatible SoCs
>>>>         - amlogic,c3-reset # Reset Controller on C3 and compatible SoCs
>>>> +      - amlogic,t7-reset # Reset Controller on T7 and compatible SoCs
>>>>   
>>>
>>> If there is going to be any resend, please drop the comment. It's not
>>> really helpful and makes it trickier to read.
>>>
>>>>     reg:
>>>>       maxItems: 1
>>>> diff --git a/include/dt-bindings/reset/amlogic,t7-reset.h b/include/dt-bindings/reset/amlogic,t7-reset.h
>>>> new file mode 100644
>>>> index 000000000000..ca4a832eeeec
>>>> --- /dev/null
>>>> +++ b/include/dt-bindings/reset/amlogic,t7-reset.h
>>>> @@ -0,0 +1,197 @@
>>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */
>>>> +/*
>>>> + * Copyright (c) 2024 Amlogic, Inc. All rights reserved.
>>>> + */
>>>> +
>>>> +#ifndef _DT_BINDINGS_AMLOGIC_T7_RESET_H
>>>> +#define _DT_BINDINGS_AMLOGIC_T7_RESET_H
>>>> +
>>>> +/* RESET0 */
>>>> +/*					0-3	*/
>>>
>>> I assume this matches existing drivers which do not use IDs but map the
>>> binding to hardware value? I remember we talked about changing it, so if
>>> something happened about this and it could be changed: please change.
>>
>> I'm not aware of such discussion, and I don't really see the issue...
>> thoses are IDs, and yes they match the Hardware offsets, and ?
> 
> Bindings are not for hardware offsets/values/addresses. It's just not a
> binding.
> 
> I quickly looked at your driver patch and it confirms: not a binding.
> Binding constant is used by the driver and DTS consumer.
> 
> I am really sure we had this talk in the past, but could be I think
> about different platform. Since this is not a binding, I do not think
> claiming there is any ABI here is reasonable. Feel free to store them
> with other hardware values, like in DTS headers etc. We already moved to
> DTS headers several such "non-binding" constants.

Un-acked.

I looked at my archives and we did talk about it and you were CCed:

https://lore.kernel.org/linux-devicetree/c088e01c-0714-82be-8347-6140daf56640@linaro.org/
simple-reset is an exception.

So to recap:
That's not a binding. Don't add some real values to binding headers
because it is not a binding then.

https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@mail.gmail.com/
https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/
https://lore.kernel.org/linux-devicetree/b60f5fd2-dc48-9375-da1c-ffcfe8292683@linaro.org/
https://lore.kernel.org/linux-devicetree/418c5f0c-5279-41f5-3705-345ec9a97ea2@linaro.org/
https://lore.kernel.org/all/201401111415.29395.arnd@arndb.de/


Best regards,
Krzysztof