RE: [PATCH v4 2/2] watchdog: Add watchdog driver for Sunplus SP7021

Hi Chrustophe,

	Thanks for your respond.

Best Regards,
Xiantao

-----Original Message-----
From: Christophe JAILLET <redacted>
To: Xiantao Hu <xt.hu@cqplus1.com>,
wim@linux-watchdog.org, p.zabel@pengutronix.de,
linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org,
linux@roeck-us.net, robh+dt@kernel.org,
devicetree@vger.kernel.org
Cc: wells.lu@sunplus.com, qinjian@cqplus1.com
Subject: Re: [PATCH v4 2/2] watchdog: Add watchdog driver for Sunplus SP7021
Date: Wed, 29 Dec 2021 10:39:08 +0100	[thread overview]
Message-ID: [ref] (raw)
In-Reply-To: [ref]

...

quoted

+static int sp_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sp_wdt_priv *priv;
+	int err;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		dev_err(dev, "Can't find clock source\n");
+		return PTR_ERR(priv->clk);
+	}
+
+	err = clk_prepare_enable(priv->clk);
+	if (err) {
+		dev_err(dev, "Clock can't be enabled correctly\n");
+		return err;
+	}
+
+	/* The timer and watchdog shared the STC reset */
+	priv->rstc = devm_reset_control_get_shared(dev, NULL);
+	if (!IS_ERR(priv->rstc))
+		reset_control_deassert(priv->rstc);
+
+	err = devm_add_action_or_reset(dev, sp_reset_control_assert,
+				       priv->rstc);
+	if (err)
+		return err;

This looks odd.
We could undo something that was not done. (if IS_ERR(priv->rstc))
This is also not really consistent with what is done in suspedn/resume.
In these functions, we don't check for IS_ERR(priv->rstc).

Here I refer to mt7621_wdt.c. I'm sure I need deassert reset to reset 
watchdog register value when driver probe. accordingly I assert reset 
in devm_add_action_or_reset() to ensure that the registers of watchdog 
can't be operated after module remove.

quoted

+
+	err = devm_add_action_or_reset(dev, sp_clk_disable_unprepare,
+				       priv->clk);
+	if (err)
+		return err;

Shouldn't this be just after clk_prepare_enable()?

I tested the order of execution of the added functions which is similar to 
push and pop. First in, last out. I think I should disable clock last.