Re: [PATCH v4 01/10] dt-bindings: clock: Add Realtek RTD1625 Clock & Reset Controller

[Yu-Chun Lin <eleanor15x@gmail.com>]

> On 17/03/2026 02:57, Yu-Chun Lin [林祐君] wrote:
>>> On Fri, Mar 13, 2026 at 04:10:51PM +0800, Yu-Chun Lin wrote:
>>>> +#endif /* __DT_BINDINGS_RTK_CLOCK_RTD1625_H */
>>>> diff --git a/include/dt-bindings/reset/realtek,rtd1625.h
>>>> b/include/dt-bindings/reset/realtek,rtd1625.h
>>>> new file mode 100644
>>>> index 000000000000..e69b5ee14cc6
>>>> --- /dev/null
>>>> +++ b/include/dt-bindings/reset/realtek,rtd1625.h
>>>> @@ -0,0 +1,195 @@
>>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>>> +/*
>>>> + * Copyright (C) 2025 Realtek Semiconductor Corp.
>>>> + */
>>>> +#ifndef __DT_BINDINGS_RTK_RESET_RTD1625_H #define
>>>> +__DT_BINDINGS_RTK_RESET_RTD1625_H
>>>> +
>>>> +#define RTD1625_RSTN_REG_ID_SOFT_RESET1     0
>>>> +#define RTD1625_RSTN_REG_ID_SOFT_RESET2     1
>>>> +#define RTD1625_RSTN_REG_ID_SOFT_RESET3     2
>>>> +#define RTD1625_RSTN_REG_ID_SOFT_RESET4     3
>>>> +#define RTD1625_RSTN_REG_ID_SOFT_RESET7     4
>>>> +#define RTD1625_RSTN_REG_ID_SOFT_RESET9     5
>>>> +#define RTD1625_RSTN_REG_ID_SOFT_RESET_BIST 6
>>>> +#define RTD1625_RSTN_REG_ID_DUMMY0          7
>>>> +#define RTD1625_RSTN_REG_ID_DUMMY1          8
>>>> +#define RTD1625_RSTN_REG_ID_DUMMY4          9
>>>> +#define RTD1625_RSTN_REG_ID_SOFT_RESET11    10
>>>> +#define RTD1625_RSTN_REG_ID_SOFT_RESET12    11
>>>> +
>>>> +#define _REG_ID(n) ((RTD1625_RSTN_REG_ID_ ## n) << 5)
>>>> +
>>>
>>> Not a binding.
>>>
>>>> +#define RTD1625_CRT_RSTN_MISC      (_REG_ID(SOFT_RESET1) | 0)
>>>> +#define RTD1625_CRT_RSTN_DIP       (_REG_ID(SOFT_RESET1) | 2)
>>>> +#define RTD1625_CRT_RSTN_GSPI      (_REG_ID(SOFT_RESET1) | 4)
>>>
>>> None of these are really bindings. Register offsets is hardware data, not a
>>> binding.
>>>
>>> Drop all of this above and further - actually nothing in this header look like a
>>> binding, since you called these "REG".
>>>
>>> Best regards,
>>> Krzysztof
>> 
>> The defines in this header are NOT register offsets. They encode "Bank ID+ Bit
>> Position" for use in Device Tree.
>
> Bit position as in register bit position? That's still not a binding.
>
>
>>
>> Agreed, The naming with "REG_ID" is misleading. I will improve naming all reset
>> IDs in v5.
>
> No, all are to be dropped or renamed to standard IDs (incremented by one
> from 0 or 1).
>
> Best regards,
> Krzysztof

Hi Krzysztof,
 
Understood. I will redesign as follows:
 
1. Binding header: Sequential IDs (0, 1, 2, ...) without hardware details.
2. Driver: Internal mapping table to convert IDs to register/bit positions.
3. DT: Uses abstract IDs from binding header.

Regards,
Yu-Chun