Hi, Guenter Roeck : Thanks for your review. Sorry I was so focused on fixing code and I forgot to respond to the email. I modify the code as you comment and answer the question. Thanks Best Regards, Xiantao Hu > -----Original Message----- > From: Guenter Roeck [mailto:groeck7@xxxxxxxxx] On Behalf Of Guenter Roeck > Sent: Friday, November 12, 2021 10:46 PM > To: xt.hu[胡先韬] <xt.hu@xxxxxxxxxxx>; wim@xxxxxxxxxxxxxxxxxx; p.zabel@xxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; linux-watchdog@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx > Cc: Wells Lu 呂芳騰 <wells.lu@xxxxxxxxxxx>; qinjian[覃健] <qinjian@xxxxxxxxxxx> > Subject: Re: [PATCH 1/2] watchdog: Add watchdog driver for Sunplus SP7021 > > On 11/12/21 2:59 AM, Xiantao Hu wrote: > > Sunplus SP7021 requires watchdog timer support. > > Add watchdog driver to enable this. > > > > Signed-off-by: Xiantao Hu <xt.hu@xxxxxxxxxxx> ... > > +struct sp_wdt_priv { > > + struct watchdog_device wdev; > > + void __iomem *base; > > + void __iomem *miscellaneous; > > Please find a better name for this variable. Also, unless I am > missing something, it is only used in the init function. That means > it can be passed to that function as parameter and doesn't need > to be stored in sp_wdt_priv. > I will remove the miscellaneous from sp_wdt_priv and pass to that function as parameter. > > + struct clk *clk; > > + struct reset_control *rstc; > > +}; ... > > +static int sp_wdt_set_timeout(struct watchdog_device *wdev, > > + unsigned int timeout) > > +{ > > + wdev->timeout = timeout; > > + > > I would assume this also needs to set the new timeout in HW if > the watchdog is running, or the watchdog could time out before > userspace sends another ping. > I will call the ping() after assigning timeout. > > + return 0; > > +} > > + ... > > + > > +static int sp_wdt_start(struct watchdog_device *wdev) > > +{ > > + int ret; > > + > > + ret = sp_wdt_set_timeout(wdev, wdev->timeout); > > That function never returns an error, so checking for it is > pointless here. > I will remove the variable ret. > > + if (ret < 0) > > + return ret; > > + ... > > +/* > > + * 1.We need to reset watchdog flag(clear watchdog interrupt) here > > + * because watchdog timer driver does not have an interrupt handler, > > + * and before enalbe STC and RBUS watchdog timeout. Otherwise, > > enable > I will fix it. > > + * the intr is always in the triggered state. > > + * 2.enable STC and RBUS watchdog timeout trigger. > > + * 3.watchdog conuter is running, need to be stopped. > > counter > I will fix it. > > + */ ... > > + > > + wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > + priv->miscellaneous = > > + devm_ioremap(dev, wdt_res->start, resource_size(wdt_res)); > > Why not use devm_ioremap_resource() ? Or, for that matter, > devm_platform_ioremap_resource() like above ? > The function of this register is miscellaneous. The register accessed here shared by multiple drivers. Use the function of devm_platform_ioremap_resource() which internally call request_mem_region() causes an error. So I handle it in this way. Any better ways? > > + if (IS_ERR(priv->miscellaneous)) > > + return PTR_ERR(priv->miscellaneous); > > + > > + priv->wdev.info = &sp_wdt_info; > > + priv->wdev.ops = &sp_wdt_ops; > > + priv->wdev.timeout = SP_WDT_MAX_TIMEOUT; > > + priv->wdev.max_timeout = SP_WDT_MAX_TIMEOUT; > > + priv->wdev.min_timeout = SP_WDT_MIN_TIMEOUT; > > THis should really use max_hw_heartbeat_ms to let the core > ping the watchdog if larger timeouts are desired. > I will add the max_hw_heartbeat_ms and modify the ping (). > > + priv->wdev.parent = dev; > > + > > + watchdog_set_drvdata(&priv->wdev, priv); > > + sp_wdt_hw_init(&priv->wdev); > > + > > + watchdog_init_timeout(&priv->wdev, timeout, dev); > > + watchdog_set_nowayout(&priv->wdev, nowayout); > > + watchdog_stop_on_reboot(&priv->wdev); > > + > > + err = devm_watchdog_register_device(dev, &priv->wdev); > > Since you call watchdog_unregister_device() below you can not use > the devm_ function here. > I will drop the function remove() and not call watchdog_unregister_device() . Use the devm_add_action_or_reset() add reset_control_assert() and clk_disable_unprepare(). > > + if (unlikely(err)) > > This is not time critical. Drop the unlikely. > I will fix it. > > + return err; > > + > > + dev_info(dev, "Watchdog enabled (timeout=%d sec%s.)\n", > > + priv->wdev.timeout, nowayout ? ", nowayout" : ""); > > + > > + return 0; > > +} > > + > > +static int sp_wdt_remove(struct platform_device *pdev) > > +{ > > + struct sp_wdt_priv *priv = platform_get_drvdata(pdev); > > + > > + watchdog_unregister_device(&priv->wdev); > > + > > + reset_control_assert(priv->rstc); > > + clk_disable_unprepare(priv->clk); > > + > > + return 0; > > +} > > + ... > > + > > +static int __maybe_unused sp_wdt_resume(struct device *dev) > > +{ > > + struct sp_wdt_priv *priv = dev_get_drvdata(dev); > > + > > + if (watchdog_active(&priv->wdev)) > > + sp_wdt_start(&priv->wdev); > > + > Shouldn't the order be the opposite of the order in the suspend function ? > I will fix it. > > + reset_control_deassert(priv->rstc); > > + clk_prepare_enable(priv->clk); > > + > > + return 0; > > +} > > ... > >
