Message-ID: <76353597-0170-e0d9-9f5d-f208a03e44e8@linaro.org>
Date:   Sat, 11 Feb 2023 12:42:08 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to
 access system controller registers
Content-Language: en-US
To:     Sergio Paracuellos <sergio.paracuellos@gmail.com>,
        =?UTF-8?B?QXLEsW7DpyDDnE5BTA==?= <arinc.unal@arinc9.com>
Cc:     linux-watchdog@vger.kernel.org, wim@linux-watchdog.org,
        linux@roeck-us.net, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
        tsbogend@alpha.franken.de, p.zabel@pengutronix.de,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        linux-mips@vger.kernel.org
References: <20230211073357.755893-1-sergio.paracuellos@gmail.com>
 <20230211073357.755893-2-sergio.paracuellos@gmail.com>
 <190b3135-82f3-4dfa-55ee-e048c5510e3c@arinc9.com>
 <CAMhs-H8tehOWvYKmFtW_LHNb62h5mnzVGN_bfGOtLgNE9qUxqw@mail.gmail.com>
 <d14f0065-e8d3-50ed-7ea4-ba57dbd18d51@arinc9.com>
 <CAMhs-H_1dtdAmeNW9arK9JxhdWaQJwcMU1Pk7TOW1f5MREzzug@mail.gmail.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <CAMhs-H_1dtdAmeNW9arK9JxhdWaQJwcMU1Pk7TOW1f5MREzzug@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 11/02/2023 12:01, Sergio Paracuellos wrote:
> On Sat, Feb 11, 2023 at 11:47 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>
>> On 11.02.2023 13:41, Sergio Paracuellos wrote:
>>> On Sat, Feb 11, 2023 at 10:10 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>
>>>> Is this mediatek,sysctl property required after your changes on the
>>>> watchdog code?
>>>
>>> I don't really understand the question :-) Yes, it is. Since we have
>>> introduced a new phandle in the watchdog node to be able to access the
>>> reset status register through the 'sysc' syscon node.
>>> We need the bindings to be aligned with the mt7621.dtsi file and we
>>> are getting the syscon regmap handler via
>>> 'syscon_regmap_lookup_by_phandle()'. See PATCH 5 of the series, Arınç.
>>
>> I believe you need to put mediatek,sysctl under "required:".
> 
> Ah, I understood your question now :-). You meant 'required' property.
> I need more coffee, I guess :-). I am not sure if you can add
> properties as required after bindings are already mainlined for
> compatibility issues. The problem with this SoC is that drivers become
> mainlined before the device tree was so if things are properly fixed
> now this kind of issues appear.  Let's see Krzysztof and Rob comments
> for this.

If your driver fails to probe without mediatek,sysctl, you already made
it required (thus broke the ABI) regardless what dt-binding is saying.
In such case you should update dt-binding to reflect reality.

Now ABI break is different case. Usually you should not break it without
valid reasons (e.g. it was never working before). Your commit msg
suggests that you only improve the code, thus ABI break is not really
justified. In such case - binding is correct, driver should be reworked
to accept DTS without the new property.

Best regards,
Krzysztof