MIME-Version: 1.0
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>
 <76353597-0170-e0d9-9f5d-f208a03e44e8@linaro.org>
In-Reply-To: <76353597-0170-e0d9-9f5d-f208a03e44e8@linaro.org>
From:   Sergio Paracuellos <sergio.paracuellos@gmail.com>
Date:   Sun, 12 Feb 2023 09:13:35 +0100
Message-ID: <CAMhs-H-JGZMR6mB=USywAh4aRS9ZFOVebwLv8=N2f3uvWpcXDA@mail.gmail.com>
Subject: Re: [PATCH v4 1/5] dt-bindings: watchdog: mt7621-wdt: add phandle to
 access system controller registers
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     =?UTF-8?B?QXLEsW7DpyDDnE5BTA==?= <arinc.unal@arinc9.com>,
        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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Feb 11, 2023 at 12:42 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/02/2023 12:01, Sergio Paracuellos wrote:
> > On Sat, Feb 11, 2023 at 11:47 AM Ar=C4=B1n=C3=A7 =C3=9CNAL <arinc.unal@=
arinc9.com> wrote:
> >>
> >> On 11.02.2023 13:41, Sergio Paracuellos wrote:
> >>> On Sat, Feb 11, 2023 at 10:10 AM Ar=C4=B1n=C3=A7 =C3=9CNAL <arinc.una=
l@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 th=
e
> >>> 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=C4=
=B1n=C3=A7.
> >>
> >> 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.

Thanks for clarification, Krzysztof. Ok, so if this is the case I need
to add this property required (as Arinc was properly pointing out in
previous mail) since without it the driver is going to fail on probe
(PATCH 5 of the series). I understand the "it was never working
before" argument reason for ABI breaks. What happens if the old driver
code was not ideal and totally dependent on architecture specific
operations when this could be totally avoided and properly make arch
independent agnostic drivers? This driver was added in 2016 [0]. There
was not a device tree file in the kernel for this SoC mainlined until
2022 [1]. I also personally migrated this watchdog binding in 2022
from text to YAML and maintained it without changes [2]. When this was
mainlined not all drivers were properly reviewed and the current code
was just maintained as it is. Most users of this SoC are in the
openWRT community where the dtsi of the mainline is not used yet and
they maintain their own mt7621.dtsi files. Also, when a new version of
the openWRT selected kernel is added they also modify and align with
its mt7621.dtsi file without maintaining previous dtb's. If "make the
driver arch independent to be able to be compile tested" and this kind
of arguments are not valid at all I need to know because I have
started to review driver code for this SoC and other drivers also have
the same arch dependency that ideally should be avoided in the same
way. This at the end means to break the ABI again in the future for
those drivers / bindings. So I can just let them be as it is and not
provide any change at all and continue without being compile tested
and other beneficial features to detect future driver breakage.

Thanks in advance for clarification.

Best regards,
    Sergio Paracuellos

[0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/com=
mit/drivers/watchdog/mt7621_wdt.c?id=3Dab3f09fe16d158cb4f84e558c61ec5d6d601=
f2e0
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/com=
mit/arch/mips/boot/dts/ralink/mt7621.dtsi?id=3D7a6ee0bbab2551d7189ce0f5e625=
fef4d612ebea
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/com=
mit/Documentation/devicetree/bindings/watchdog/mediatek,mt7621-wdt.yaml?id=
=3D9023e05b7a5809593a7ea09896eee0bbb6ae1685

>
> Best regards,
> Krzysztof
>