Hello Philipp Zabel,
Thanks for the feedback.
> Subject: Re: [RFC 4/4] watchdog: Add Watchdog Timer driver for RZ/G2L
>
> On Thu, 2021-11-04 at 16:08 +0000, Biju Das wrote:
> [...]
> > +static int rzg2l_wdt_probe(struct platform_device *pdev) {
> > + struct device *dev = &pdev->dev;
> > + struct rzg2l_wdt_priv *priv;
> > + struct clk *wdt_clk;
> > + int ret;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(priv->base))
> > + return PTR_ERR(priv->base);
> > +
> > + /* Get watchdog main clock */
> > + wdt_clk = devm_clk_get(&pdev->dev, "oscclk");
> > + if (IS_ERR(wdt_clk))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no
> oscclk");
> > +
> > + priv->osc_clk_rate = clk_get_rate(wdt_clk);
> > + if (!priv->osc_clk_rate)
> > + return dev_err_probe(&pdev->dev, -EINVAL, "oscclk rate is 0");
> > +
> > + /* Get Peripheral clock */
> > + wdt_clk = devm_clk_get(&pdev->dev, "pclk");
> > + if (IS_ERR(wdt_clk))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(wdt_clk), "no pclk");
> > +
> > + priv->pclk_rate = clk_get_rate(wdt_clk);
> > + if (!priv->pclk_rate)
> > + return dev_err_probe(&pdev->dev, -EINVAL, "pclk rate is 0");
> > +
> > + priv->delay = F2CYCLE_NSEC(priv->osc_clk_rate) * 6 +
> > +F2CYCLE_NSEC(priv->pclk_rate) * 9;
> > +
> > + priv->rstc = devm_reset_control_get(&pdev->dev, NULL);
>
> Please use devm_reset_control_get_exclusive().
Agreed. Will use devm_reset_control_get_exclusive.
>
> > + if (IS_ERR(priv->rstc))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc),
> > + "failed to get cpg reset");
> > +
> > + reset_control_deassert(priv->rstc);
> > + ret = devm_add_action_or_reset(&pdev->dev,
> > + rzg2l_wdt_reset_assert_clock_disable,
>
> I suppose rzg2l_wdt_reset_assert_clock_disable should be renamed to
> rzg2l_wdt_reset_assert given that it does not disable a clock.
OK. But I am going to use devm_watchdog_register_device and planning to
add pm disable/put in rzg2l_wdt_reset_assert_pm_disable function.
After that driver won't have remove callback as it is empty.
>
> > + &priv->wdev);
> > + if (ret < 0)
> > + return dev_err_probe(&pdev->dev, ret, "failed to get reset");
>
> I think this should just return ret, as the only possible failure from
> devm_add_action_or_reset() is -ENOMEM.
Agreed, will just return ret.
>
> > +
> > + pm_runtime_enable(&pdev->dev);
> > + ret = pm_runtime_resume_and_get(&pdev->dev);
> > + if (ret < 0) {
> > + dev_err(dev, "pm_runtime_resume_and_get failed");
>
> Consider printing ret with %pe.
Ok will print ret with %pe.
Regards,
Biju
>
> > + goto out_pm_get;
> > + }
> > +
> > + priv->wdev.info = &rzg2l_wdt_ident;
> > + priv->wdev.ops = &rzg2l_wdt_ops;
> > + priv->wdev.parent = dev;
> > + priv->wdev.min_timeout = 1;
> > + priv->wdev.max_timeout = WDT_CYCLE_MSEC(priv->osc_clk_rate, 0xfff);
> > + priv->wdev.timeout = WDT_DEFAULT_TIMEOUT;
> > +
> > + platform_set_drvdata(pdev, priv);
> > + watchdog_set_drvdata(&priv->wdev, priv);
> > + watchdog_set_nowayout(&priv->wdev, nowayout);
> > + watchdog_set_restart_priority(&priv->wdev, 0);
> > + watchdog_stop_on_unregister(&priv->wdev);
> > +
> > + ret = watchdog_init_timeout(&priv->wdev, 0, dev);
> > + if (ret)
> > + dev_warn(dev, "Specified timeout invalid, using default");
> > +
> > + ret = devm_watchdog_register_device(&pdev->dev, &priv->wdev);
> > + if (ret < 0)
> > + goto out_pm_disable;
> > +
> > + return 0;
> > +
> > +out_pm_disable:
> > + pm_runtime_put(dev);
> > +out_pm_get:
> > + pm_runtime_disable(dev);
> > +
> > + return ret;
> > +}
>
> regards
> Philipp
