Messages in this thread | | | Subject | Re: [PATCH v4 2/2] watchdog: Add watchdog driver for Sunplus SP7021 | From | Guenter Roeck <> | Date | Wed, 5 Jan 2022 07:53:02 -0800 |
| |
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@wanadoo.fr> >> 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: <0b102fa0-cbfc-a97e-8e7f-cce8146450bc@wanadoo.fr> (raw) >> In-Reply-To: <20211229054308.63168-3-xt.hu@cqplus1.com> >> >> ... >> >>> +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
| |