On 12/30/21 6:50 PM, xt.hu[胡先韬] wrote:
Hi Chrustophe,
Thanks for your respond.
Best Regards,
Xiantao
-----Original Message-----
From: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
To: Xiantao Hu <xt.hu@xxxxxxxxxxx>,
wim@xxxxxxxxxxxxxxxxxx, p.zabel@xxxxxxxxxxxxxx,
linux-kernel@xxxxxxxxxxxxxxx, linux-watchdog@xxxxxxxxxxxxxxx,
linux@xxxxxxxxxxxx, robh+dt@xxxxxxxxxx,
devicetree@xxxxxxxxxxxxxxx
Cc: wells.lu@xxxxxxxxxxx, qinjian@xxxxxxxxxxx
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: <0b102fa0-cbfc-a97e-8e7f-cce8146450bc@xxxxxxxxxx> (raw)
In-Reply-To: <20211229054308.63168-3-xt.hu@xxxxxxxxxxx>
...
+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.
You are missing the point here. This is called even after
devm_reset_control_get_shared() failed, and it is called even though
the reset was not yet deasserted. The above code should be called after
(and only after) deasserting the reset.
+
+ 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.
Correct. That is why you have to call devm_add_action_or_reset on disable_unprepare
first, because on driver removal the devm_ cleanup will be called in reverse order.
Guenter
