[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <17803809bef1499fb051cf79fc3c7245@cqplus1.com>
Date: Fri, 31 Dec 2021 02:50:41 +0000
From: xt.hu[胡先韬] <xt.hu@...lus1.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
CC: "wim@...ux-watchdog.org" <wim@...ux-watchdog.org>,
"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
"linux@...ck-us.net" <linux@...ck-us.net>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Wells Lu 呂芳騰 <wells.lu@...plus.com>,
qinjian[覃健] <qinjian@...lus1.com>,
Rob Herring <robh@...nel.org>
Subject: 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 <christophe.jaillet@...adoo.fr>
> To: Xiantao Hu <xt.hu@...lus1.com>,
> wim@...ux-watchdog.org, p.zabel@...gutronix.de,
> linux-kernel@...r.kernel.org, linux-watchdog@...r.kernel.org,
> linux@...ck-us.net, robh+dt@...nel.org,
> devicetree@...r.kernel.org
> Cc: wells.lu@...plus.com, qinjian@...lus1.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@...adoo.fr> (raw)
> In-Reply-To: <20211229054308.63168-3-xt.hu@...lus1.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.
> > +
> > + 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.
Powered by blists - more mailing lists