Re: [PATCH v2 06/20] dt-binding: clk: npcm845: Add binding for Nuvoton NPCM8XX Clock

[Krzysztof Kozlowski]

On 09/06/2022 23:21, Tomer Maimon wrote:
> Hi Krzysztof,
> 
> 
> On Thu, 9 Jun 2022 at 16:22, Krzysztof Kozlowski
> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
>>
>> On 09/06/2022 15:17, Tomer Maimon wrote:
>>> Hi Krzysztof,
>>>
>>> Thanks for your comments.
>>>
>>> On Wed, 8 Jun 2022 at 13:03, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
>>>>
>>>> On 08/06/2022 11:56, Tomer Maimon wrote:
>>>>> Add binding for the Arbel BMC NPCM8XX Clock controller.
>>>>>
>>>>> Signed-off-by: Tomer Maimon <tmaimon77@xxxxxxxxx>
>>>>> ---
>>>>>  .../bindings/clock/nuvoton,npcm845-clk.yaml   | 63 +++++++++++++++++++
>>>>>  .../dt-bindings/clock/nuvoton,npcm8xx-clock.h | 50 +++++++++++++++
>>>>>  2 files changed, 113 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
>>>>>  create mode 100644 include/dt-bindings/clock/nuvoton,npcm8xx-clock.h
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..e1f375716bc5
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
>>>>> @@ -0,0 +1,63 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/clock/nuvoton,npcm845-clk.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Nuvoton NPCM8XX Clock Controller Binding
>>>>> +
>>>>> +maintainers:
>>>>> +  - Tomer Maimon <tmaimon77@xxxxxxxxx>
>>>>> +
>>>>> +description: |
>>>>> +  Nuvoton Arbel BMC NPCM8XX contains an integrated clock controller, which
>>>>> +  generates and supplies clocks to all modules within the BMC.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - nuvoton,npcm845-clk
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  clocks:
>>>>> +    items:
>>>>> +      - description: 25M reference clock
>>>>> +      - description: CPU reference clock
>>>>> +      - description: MC reference clock
>>>>> +
>>>>> +  clock-names:
>>>>> +    items:
>>>>> +      - const: refclk
>>>>> +      - const: sysbypck
>>>>> +      - const: mcbypck
>>>>> +
>>>>
>>>> I asked what is the suffix about and you replied "ck"... ok, so let's
>>>> make clear. This should be:
>>>>
>>>>     items:
>>>>       - const: ref
>>>>       - const: sysbyp
>>>>       - const: mcbyp
>>>>
>>>> or something similar, without the same suffix all over.
>>> The clock names are the same clock name in our spec, this why we
>>> prefer to leave the clock names as is.
>>
>> The naming with useless suffixes does not help. If your spec had
>> "refclk_really_clock_this_is_a_clock" you also would insist on that? It
>> does not make sense.
> Sorry but I don't understand why the clock name cause an issue, we
> prefer it will be the same as in our spec-clock diagram
> BTW, the same naming found in NPCM7XX
> https://elixir.bootlin.com/linux/v5.19-rc1/source/Documentation/devicetree/bindings/clock/nuvoton,npcm750-clk.txt#L36

Because the names should not have irrelevant information.
interrupt-names should have "txirq". dma-names should not have "txdma".
clock-names should not have "refclock" or "refclk" because it is
irrelevant duplication. These are bindings, not DTS, so whatever you
have in your spec matters less. DTS is the representation of hardware
and there you can name clocks closer to the spec so it is easier for
you, if that's your preference.

And if your spec has "refclk_really_clock_this_is_a_clock" you still
should not use it.

Anyway, you should discuss it last time when I pointed it out. Instead
my comments were ignored and you decided to send v2. That's not how
discussion works and it will not bring you closer to your point.


Best regards,
Krzysztof