Date:	Tue, 2 Feb 2016 22:17:43 +0100
From:	Maxime Ripard <maxime.ripard@...e-electrons.com>
To:	codekipper@...il.com
Cc:	linux-arm-kernel@...ts.infradead.org, linux-sunxi@...glegroups.com,
	lgirdwood@...il.com, broonie@...nel.org,
	linux-kernel@...r.kernel.org, alsa-devel@...a-project.org,
	be17068@...rbole.bo.it
Subject: Re: [PATCH v3 2/2] ASOC: sunxi: Add support for the SPDIF block

Hi,

It looks mostly good on my side, a few comments though.

On Tue, Feb 02, 2016 at 03:49:54PM +0100, codekipper@...il.com wrote:
> +#ifdef CONFIG_PM
> +static int sun4i_spdif_runtime_suspend(struct device *dev)
> +{
> +	struct sun4i_spdif_dev *host  = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(host->spdif_clk);
> +
> +	return 0;
> +}
> +
> +static int sun4i_spdif_runtime_resume(struct device *dev)
> +{
> +	struct sun4i_spdif_dev *host  = dev_get_drvdata(dev);
> +
> +	clk_prepare_enable(host->spdif_clk);
> +
> +	return 0;
> +}
> +#endif /* CONFIG_PM */

You can also shut down the bus clock here. That will reset the device,
so you'll have to reinit it in your resume, but since you won't be
suspended while you play something, and that you set up most of your
controller any way when you start streaming, it shouldn't be a big
deal.

> +
> +static int sun4i_spdif_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct sun4i_spdif_dev *host;
> +	struct resource *res;
> +	int ret;
> +	void __iomem *base;
> +
> +	dev_dbg(&pdev->dev, "Entered %s\n", __func__);
> +
> +	if (!np)
> +		return -ENODEV;

You won't probe without DT.

> +
> +	if (!of_match_device(sun4i_spdif_of_match, &pdev->dev)) {
> +		dev_err(&pdev->dev, "No matched devices found.\n");
> +		return -EINVAL;
> +	}

And you won't probe if the compatibles don't match.

> +	host = devm_kzalloc(&pdev->dev, sizeof(*host), GFP_KERNEL);
> +	if (!host)
> +		return -ENOMEM;
> +
> +	host->pdev = pdev;
> +
> +	/* Initialize this copy of the CPU DAI driver structure */
> +	memcpy(&host->cpu_dai_drv, &sun4i_spdif_dai, sizeof(sun4i_spdif_dai));
> +	host->cpu_dai_drv.name = dev_name(&pdev->dev);
> +
> +	/* Get the addresses */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	host->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> +						&sun4i_spdif_regmap_config);
> +
> +	/* Clocks */
> +	host->apb_clk = devm_clk_get(&pdev->dev, "apb");
> +	if (IS_ERR(host->apb_clk)) {
> +		dev_err(&pdev->dev, "failed to get a apb clock.\n");
> +		return PTR_ERR(host->apb_clk);
> +	}
> +
> +	ret = clk_prepare_enable(host->apb_clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "try to enable apb_spdif_clk failed\n");
> +		return ret;
> +	}
> +
> +	host->spdif_clk = devm_clk_get(&pdev->dev, "spdif");
> +	if (IS_ERR(host->spdif_clk)) {
> +		dev_err(&pdev->dev, "failed to get a spdif clock.\n");
> +		goto exit_clkdisable_apb_clk;
> +	}
> +
> +	host->playback_supported = false;
> +	host->capture_supported = false;
> +
> +	if (of_property_read_bool(np, "spdif-out"))
> +		host->playback_supported = true;
> +
> +	if (!host->playback_supported) {
> +		dev_err(&pdev->dev, "no enabled S/PDIF DAI link\n");
> +		goto exit_clkdisable_clk;
> +	}
> +
> +	host->dma_params_tx.addr = res->start + SUN4I_SPDIF_TXFIFO;
> +	host->dma_params_tx.maxburst = 4;
> +	host->dma_params_tx.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> +	host->dma_params_rx.addr = res->start + SUN4I_SPDIF_RXFIFO;
> +	host->dma_params_rx.maxburst = 4;
> +	host->dma_params_rx.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> +
> +	platform_set_drvdata(pdev, host);
> +
> +	ret = devm_snd_soc_register_component(&pdev->dev,
> +				&sun4i_spdif_component, &sun4i_spdif_dai, 1);
> +	if (ret)
> +		goto exit_clkdisable_clk;
> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
> +	if (ret)
> +		goto exit_clkdisable_clk;
> +	return 0;
> +
> +exit_clkdisable_clk:
> +	clk_disable_unprepare(host->spdif_clk);

You disabled a clock that you didn't enable.

> +exit_clkdisable_apb_clk:
> +	clk_disable_unprepare(host->apb_clk);
> +	return ret;
> +}
> +
> +static int sun4i_spdif_remove(struct platform_device *pdev)
> +{
> +	struct sun4i_spdif_dev *host = dev_get_drvdata(&pdev->dev);
> +
> +	snd_soc_unregister_platform(&pdev->dev);
> +	snd_soc_unregister_component(&pdev->dev);
> +
> +	pm_runtime_disable(&pdev->dev);
> +
> +	if (!IS_ERR(host->spdif_clk)) {

Probe will fail on this condition. If probe failed, remove won't be
called.

> +		clk_disable_unprepare(host->spdif_clk);

And here as well.

> +		clk_disable_unprepare(host->apb_clk);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops sun4i_spdif_pm = {
> +	SET_RUNTIME_PM_OPS(sun4i_spdif_runtime_suspend,
> +			   sun4i_spdif_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver sun4i_spdif_driver = {
> +	.driver		= {
> +		.name	= "sun4i-spdif",
> +		.owner	= THIS_MODULE,

This is not needed.

> +		.of_match_table = of_match_ptr(sun4i_spdif_of_match),
> +		.pm	= &sun4i_spdif_pm,

IIRC, the pm field is not defined if PM is not set.

Have you tested your driver without PM?

> +	},
> +	.probe		= sun4i_spdif_probe,
> +	.remove		= sun4i_spdif_remove,
> +};
> +
> +module_platform_driver(sun4i_spdif_driver);
> +
> +MODULE_AUTHOR("Marcus Cooper <codekipper@...il.com>");
> +MODULE_AUTHOR("Andrea Venturi <be17068@...rbole.bo.it>");
> +MODULE_DESCRIPTION("Allwinner sun4i SPDIF SoC Interface");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:sun4i-spdif");

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists