Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1324631

Re: [PATCH v3 2/2] ASOC: sunxi: Add support for the SPDIF block

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

Hi,

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

On Tue, Feb 02, 2016 at 03:49:54PM +0100, codekipper@gmail.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@gmail.com>");
> +MODULE_AUTHOR("Andrea Venturi <be17068@iperbole.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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v3 0/2] Add SPDIF support for Allwinner SoCs  codekipper@gmail.com - 2016-02-02 16:00 +0100
  [PATCH v3 2/2] ASOC: sunxi: Add support for the SPDIF block codekipper@gmail.com - 2016-02-02 16:00 +0100
    [PATCH] ASOC: sunxi: fix platform_no_drv_owner.cocci warnings kbuild test robot <lkp@intel.com> - 2016-02-02 17:10 +0100
    Re: [PATCH v3 2/2] ASOC: sunxi: Add support for the SPDIF block kbuild test robot <lkp@intel.com> - 2016-02-02 17:10 +0100
    Re: [PATCH v3 2/2] ASOC: sunxi: Add support for the SPDIF block Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-02-02 22:20 +0100
      Re: [PATCH v3 2/2] ASOC: sunxi: Add support for the SPDIF block Code Kipper <codekipper@gmail.com> - 2016-02-03 09:50 +0100
  [PATCH v3 1/2] dt-bindings: add sun4i SPDIF transceiver bindings codekipper@gmail.com - 2016-02-02 16:00 +0100
    Re: [PATCH v3 1/2] dt-bindings: add sun4i SPDIF transceiver bindings Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-02-02 23:40 +0100
      Re: [PATCH v3 1/2] dt-bindings: add sun4i SPDIF transceiver bindings Code Kipper <codekipper@gmail.com> - 2016-02-03 08:30 +0100
  Re: [PATCH v3 0/2] Add SPDIF support for Allwinner SoCs Mark Brown <broonie@kernel.org> - 2016-02-02 20:20 +0100

csiph-web