Re: [PATCH v4 1/4] dt-bindings: mediatek: watchdog: Fix compatible fallbacks and example

[Allen-KH Cheng <allen-kh.cheng@mediatek.com>]

Hi Chen-Yu and Mathias,

Sincere apologies for the delayed response.

On Tue, 2022-08-02 at 13:04 +0200, Matthias Brugger wrote:
> 
> On 01/08/2022 11:29, Chen-Yu Tsai wrote:
> > On Thu, Jul 21, 2022 at 10:50 AM Allen-KH Cheng
> > <allen-kh.cheng@mediatek.com> wrote:
> > > 
> > > The watchdog timer of mt8186. mt8195 and mt7986 have their DT
> > > data.
> > > We should not use 'mediatek,mt6589-wdt' as fallback.
> > > 
> > > For mediatek,wdt example of mt8183, We remove mediatek,mt6589-wdt 
> > > fallback.
> > 
> > I think this needs some more information.
> > 
> > Right now on the kernel side, mt6589-wdt provides just watchdog
> > support.
> > The SoC-specific compatibles that are touched by this patch provide
> > reset
> > controls in addition to the standard watchdog, which remains the
> > same.
> > 
> > If that is the case, then the fallback compatibles are correct. A
> > fallback
> > says that the new hardware is compatible with some older hardware,
> > and
> > can be run with the driver supporting that older hardware, likely
> > with
> > reduced functionality.
> > 
> 
> My understanding is, that we add a fallback because although at the
> time we 
> entered the compatible, the functionality of the device is the same
> as the 
> fallback. Nonetheless we add a compatible specific for the device in
> case in the 
> future we realize that the device has some functionality that is not
> and can not 
> be covered by the fallback.
> 
> This is the case here. Actually adding the fallback in the first
> place was 
> wrong, because the driver since ever supports the extra function for
> the device, 
> the reset.
> 
> So this is a mere cleanup of the binding to reflect what was always
> present in 
> the driver.
> 
> Regards,
> Matthias
> 

mt6589-wdt dosen't contains a reset control for other modules, like
chen-yu mention "mt6589-wdt provides just watchdog support."

For instance, there is a reset control in mt8195-wdt and we have a DT
data to define its reset number of TOPRGU. I thought it's better not
use mt6589-wdt as fallback.

Please let me know if this works and if you have any suggestions or
comments.

Thanks,
Allen

> > As an example, if mt8195-wdt is backward compatible with mt6589-
> > wdt,
> > then it should run as mt6589-wdt, and would just be missing new
> > functionality, in this case the reset controls.
> > 
> > So either mt6589-wdt also contains a reset control that is not the
> > same
> > as the other newer chips, or has some other functionality that the
> > other
> > chips contain, and justifies the removal of the fallback, or this
> > patch
> > is incorrect. Note that mt2701-wdt and mt762*-wdt are still listed
> > as
> > compatible with mt6589-wdt. So I think a better explanation is
> > required.
> > 
> > 
> > Regards
> > ChenYu
> > 
> > 
> > > Fixes:a45b408a020b("dt-bindings: watchdog: Add compatible for
> > > MediaTek MT8186")
> > > Fixes:b326f2c85f3d("dt-bindings: watchdog: Add compatible for
> > > Mediatek MT8195")
> > > Fixes:41e73feb1024("dt-bindings: watchdog: Add compatible for
> > > Mediatek MT7986")
> > > Fixes:f43f97a0fc0e("dt-bindings: mediatek: mt8183: Add #reset-
> > > cells")
> > > Signed-off-by: Allen-KH Cheng <allen-kh.cheng@mediatek.com>
> > > Reviewed-by: AngeloGioacchino Del Regno <
> > > angelogioacchino.delregno@collabora.com>
> > > ---
> > >   Documentation/devicetree/bindings/watchdog/mtk-wdt.txt | 9
> > > ++++-----
> > >   1 file changed, 4 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/watchdog/mtk-
> > > wdt.txt b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > > index 762c62e428ef..67ef991ec4cf 100644
> > > --- a/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > > +++ b/Documentation/devicetree/bindings/watchdog/mtk-wdt.txt
> > > @@ -14,12 +14,12 @@ Required properties:
> > >          "mediatek,mt7622-wdt", "mediatek,mt6589-wdt": for MT7622
> > >          "mediatek,mt7623-wdt", "mediatek,mt6589-wdt": for MT7623
> > >          "mediatek,mt7629-wdt", "mediatek,mt6589-wdt": for MT7629
> > > -       "mediatek,mt7986-wdt", "mediatek,mt6589-wdt": for MT7986
> > > +       "mediatek,mt7986-wdt": for MT7986
> > >          "mediatek,mt8183-wdt": for MT8183
> > > -       "mediatek,mt8186-wdt", "mediatek,mt6589-wdt": for MT8186
> > > +       "mediatek,mt8186-wdt": for MT8186
> > >          "mediatek,mt8516-wdt", "mediatek,mt6589-wdt": for MT8516
> > >          "mediatek,mt8192-wdt": for MT8192
> > > -       "mediatek,mt8195-wdt", "mediatek,mt6589-wdt": for MT8195
> > > +       "mediatek,mt8195-wdt": for MT8195
> > > 
> > >   - reg : Specifies base physical address and size of the
> > > registers.
> > > 
> > > @@ -32,8 +32,7 @@ Optional properties:
> > >   Example:
> > > 
> > >   watchdog: watchdog@10007000 {
> > > -       compatible = "mediatek,mt8183-wdt",
> > > -                    "mediatek,mt6589-wdt";
> > > +       compatible = "mediatek,mt8183-wdt";
> > >          mediatek,disable-extrst;
> > >          reg = <0 0x10007000 0 0x100>;
> > >          interrupts = <GIC_SPI 139 IRQ_TYPE_NONE>;
> > > --
> > > 2.18.0
> > > 
> > >