Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <62562cdf-93e3-f642-5bbd-48329eff33ea@linaro.org>
Date:   Mon, 23 May 2022 16:26:39 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v1 11/19] dt-bindings: reset: npcm: Add support for
 NPCM8XX
Content-Language: en-US
To:     Geert Uytterhoeven <geert@linux-m68k.org>,
        Tomer Maimon <tmaimon77@gmail.com>
Cc:     Avi Fishman <avifishman70@gmail.com>,
        Tali Perry <tali.perry1@gmail.com>,
        Joel Stanley <joel@jms.id.au>,
        Patrick Venture <venture@google.com>,
        Nancy Yuen <yuenn@google.com>,
        Benjamin Fair <benjaminfair@google.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Greg KH <gregkh@linuxfoundation.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Wim Van Sebroeck <wim@linux-watchdog.org>,
        Guenter Roeck <linux@roeck-us.net>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
        Olof Johansson <olof@lixom.net>,
        Jiri Slaby <jirislaby@kernel.org>,
        Shawn Guo <shawnguo@kernel.org>,
        =?UTF-8?Q?Bj=c3=b6rn_Andersson?= <bjorn.andersson@linaro.org>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Marcel Ziswiler <marcel.ziswiler@toradex.com>,
        Vinod Koul <vkoul@kernel.org>,
        Biju Das <biju.das.jz@bp.renesas.com>,
        Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>,
        Robert Hancock <robert.hancock@calian.com>,
        =?UTF-8?Q?Jonathan_Neusch=c3=a4fer?= <j.neuschaefer@gmx.net>,
        Lubomir Rintel <lkundrak@v3.sk>, arm-soc <soc@kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-clk <linux-clk@vger.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
        Linux Watchdog Mailing List <linux-watchdog@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>
References: <20220522155046.260146-1-tmaimon77@gmail.com>
 <20220522155046.260146-12-tmaimon77@gmail.com>
 <86cd6a37-70ad-3a90-bc8a-dcd8b41f1175@linaro.org>
 <CAP6Zq1i2Wj4FCA4-eseVoJyMof5=ocFCUcitVquJqYJ4Z3JTYQ@mail.gmail.com>
 <CAMuHMdVCCrKTpNHng2_kKGViuEXf=O3MsfpjjzMusuUcKE6HiA@mail.gmail.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <CAMuHMdVCCrKTpNHng2_kKGViuEXf=O3MsfpjjzMusuUcKE6HiA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 23/05/2022 16:22, Geert Uytterhoeven wrote:
> Hi Tomer,
> 
> On Mon, May 23, 2022 at 4:03 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
>> On Mon, 23 May 2022 at 12:01, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>> On 22/05/2022 17:50, Tomer Maimon wrote:
>>>> Add binding document and device tree binding
>>>> constants for Nuvoton BMC NPCM8XX reset controller.
>>>>
>>>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> 
>>>> --- /dev/null
>>>> +++ b/include/dt-bindings/reset/nuvoton,npcm8xx-reset.h
>>>> @@ -0,0 +1,124 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +// Copyright (c) 2022 Nuvoton Technology corporation.
>>>> +
>>>> +#ifndef _DT_BINDINGS_NPCM8XX_RESET_H
>>>> +#define _DT_BINDINGS_NPCM8XX_RESET_H
>>>> +
>>>> +#define NPCM8XX_RESET_IPSRST1                0x20
>>>> +#define NPCM8XX_RESET_IPSRST2                0x24
>>>> +#define NPCM8XX_RESET_IPSRST3                0x34
>>>> +#define NPCM8XX_RESET_IPSRST4                0x74
>>>
>>> What are these? All IDs should be incremental, decimal and start from 0.
>>
>> Register offset, we use the same method in NPCM7xx. please refer
>> https://elixir.bootlin.com/linux/v5.18/source/include/dt-bindings/reset/nuvoton,npcm7xx-reset.h
>>
>> and the driver asserts the reset according to the reset include definitions
> 
> So if they're easy to look up the values, you could do without the
> definitions? Cfr. the interrupts properties in .dtsi files, where we
> typically just use the hardcoded numbers.
> 
> If you do decide to keep them, a comment explaining their origins
> would be useful.
> 
>>>> +
>>>> +/* Reset lines on IP1 reset module (NPCM8XX_RESET_IPSRST1) */
>>>> +#define NPCM8XX_RESET_GDMA0          3
>>>
>>> IDs start from 0 and do not have holes.
>>
>> This represents the reset BIT in the reset register.
> 
> Likewise, I think it's a good idea to document that in a comment, cfr.
> https://elixir.bootlin.com/linux/v5.18/source/include/dt-bindings/power/r8a7795-sysc.h#L8

Renesas is also doing it not correct (just like many others). The
bindings are not for register bits or offsets. Such data can be DTS but
not part of bindings. Imagine now you made mistake in this register
offset and hardware uses slightly different value. What now? Change
bindings? No. Bindings hold here ID, the abstraction, and ID stays fixed.


Best regards,
Krzysztof