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().
> + 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.
> + &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.
> +
> + 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.
> + 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
