Hi Sjoerd,
> +static int rk_spdif_probe(struct platform_device *pdev)
> +{
> + struct rk_spdif_dev *spdif;
> + struct resource *res;
> + void __iomem *regs;
> + int ret;
> +
> + spdif = devm_kzalloc(&pdev->dev, sizeof(*spdif), GFP_KERNEL);
> + if (!spdif)
> + return -ENOMEM;
> +
> + spdif->hclk = devm_clk_get(&pdev->dev, "spdif_hclk");
I guess this could be named just "hclk" - as it is the identifier local to the
spdif-ip. (Of course in the binding too)
> + if (IS_ERR(spdif->hclk)) {
> + dev_err(&pdev->dev, "Can't retrieve rk_spdif bus clock\n");
> + return PTR_ERR(spdif->hclk);
> + }
> + ret = clk_prepare_enable(spdif->hclk);
> + if (ret) {
> + dev_err(spdif->dev, "hclock enable failed %d\n", ret);
> + return ret;
> + }
> +
> + spdif->mclk = devm_clk_get(&pdev->dev, "spdif_clk");
The Rockchip TRMs (and the rest of the driver as well) refer to this clock as
"mclk", so I guess the identifier could just be named the same.
> + if (IS_ERR(spdif->mclk)) {
> + dev_err(&pdev->dev, "Can't retrieve rk_spdif master clock\n");
> + return PTR_ERR(spdif->hclk);
> + }
> +
> + ret = clk_prepare_enable(spdif->mclk);
> + if (ret) {
> + dev_err(spdif->dev, "clock enable failed %d\n", ret);
> + return ret;
> + }
I guess this plays into what Mark already wrote, but as the code stands right
now, you enable the mclk here and then through runtime_resume as well, so that
it stays running all the time, as the refcount is either 2 or 1 but never 0.
Also I don't think mixing devm_clk_get + clk_prepare_enable calls works well.
If the devm_clk_get(... "spdif_clk") fails, the hclk would stay running right
now.
[...]
> +static int rk_spdif_remove(struct platform_device *pdev)
> +{
> + struct rk_spdif_dev *spdif = dev_get_drvdata(&pdev->dev);
> +
> + pm_runtime_disable(&pdev->dev);
> + if (!pm_runtime_status_suspended(&pdev->dev))
> + rk_spdif_runtime_suspend(&pdev->dev);
> +
> + clk_disable_unprepare(spdif->mclk);
> + clk_disable_unprepare(spdif->hclk);
> + snd_dmaengine_pcm_unregister(&pdev->dev);
> + snd_soc_unregister_component(&pdev->dev);
I think the ordering should stay symmetric to the probe function, where you
have
clk_enable
snd_register
so here it should probably be
snd_unregister
clk_disable
Heiko
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
