From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757574AbcBBVRq (ORCPT ); Tue, 2 Feb 2016 16:17:46 -0500 Received: from down.free-electrons.com ([37.187.137.238]:54649 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754389AbcBBVRp (ORCPT ); Tue, 2 Feb 2016 16:17:45 -0500 Date: Tue, 2 Feb 2016 22:17:43 +0100 From: Maxime Ripard To: codekipper@gmail.com Cc: linux-arm-kernel@lists.infradead.org, linux-sunxi@googlegroups.com, lgirdwood@gmail.com, broonie@kernel.org, linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org, be17068@iperbole.bo.it Subject: Re: [PATCH v3 2/2] ASOC: sunxi: Add support for the SPDIF block Message-ID: <20160202211743.GD4652@lukather> References: <1454424594-25208-1-git-send-email-codekipper@gmail.com> <1454424594-25208-3-git-send-email-codekipper@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="kF9LAPPYk2MosVMu" Content-Disposition: inline In-Reply-To: <1454424594-25208-3-git-send-email-codekipper@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --kF9LAPPYk2MosVMu Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 =3D 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 =3D 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 =3D 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 =3D devm_kzalloc(&pdev->dev, sizeof(*host), GFP_KERNEL); > + if (!host) > + return -ENOMEM; > + > + host->pdev =3D 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 =3D dev_name(&pdev->dev); > + > + /* Get the addresses */ > + res =3D platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base =3D devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + host->regmap =3D devm_regmap_init_mmio(&pdev->dev, base, > + &sun4i_spdif_regmap_config); > + > + /* Clocks */ > + host->apb_clk =3D 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 =3D 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 =3D 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 =3D false; > + host->capture_supported =3D false; > + > + if (of_property_read_bool(np, "spdif-out")) > + host->playback_supported =3D 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 =3D res->start + SUN4I_SPDIF_TXFIFO; > + host->dma_params_tx.maxburst =3D 4; > + host->dma_params_tx.addr_width =3D DMA_SLAVE_BUSWIDTH_2_BYTES; > + host->dma_params_rx.addr =3D res->start + SUN4I_SPDIF_RXFIFO; > + host->dma_params_rx.maxburst =3D 4; > + host->dma_params_rx.addr_width =3D DMA_SLAVE_BUSWIDTH_2_BYTES; > + > + platform_set_drvdata(pdev, host); > + > + ret =3D 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 =3D 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 =3D 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 =3D { > + SET_RUNTIME_PM_OPS(sun4i_spdif_runtime_suspend, > + sun4i_spdif_runtime_resume, NULL) > +}; > + > +static struct platform_driver sun4i_spdif_driver =3D { > + .driver =3D { > + .name =3D "sun4i-spdif", > + .owner =3D THIS_MODULE, This is not needed. > + .of_match_table =3D of_match_ptr(sun4i_spdif_of_match), > + .pm =3D &sun4i_spdif_pm, IIRC, the pm field is not defined if PM is not set. Have you tested your driver without PM? > + }, > + .probe =3D sun4i_spdif_probe, > + .remove =3D sun4i_spdif_remove, > +}; > + > +module_platform_driver(sun4i_spdif_driver); > + > +MODULE_AUTHOR("Marcus Cooper "); > +MODULE_AUTHOR("Andrea Venturi "); > +MODULE_DESCRIPTION("Allwinner sun4i SPDIF SoC Interface"); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("platform:sun4i-spdif"); Thanks! Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --kF9LAPPYk2MosVMu Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWsRz3AAoJEBx+YmzsjxAggEkP/icdLBcp1B11WJxNnK8k7VFe p/yX8kf8KukAFZIK1ByYjvbuicmF00UMqRgjjHwfhk4ZU+q/epgAoGO/8ttjuhnU 99kC5nA4OnDzn1z0ZhzhTvtC8u4n8Goa/CTJvkqCoz7s528ylM0f8eCQRLW2wIQW U0Q1ap0evc2UHvWCWg82t4kORnO8bPXW6S+98lMQQhBDvDjTGb6RD3JJJhsp40aj kNOMtBnC5Fgr7OSpuROCSTsCMFp4qvW+hy6MINlt0pn8WEyRupWIhY8m43j71wcb dRTYEONLfANe8Y6KUx8RR65mtmToqnic+IKcH3K8dGkur/tB1SBhafkS9p0234DW pnaXh5iHrvu45yI6VXz25mUSEKhnfooDFd9MUpHi1qR/Yrlpw0h4INsBXrmpunZA 7fJhHxsPOk5Mjh1fV9Dh71GWYDtH/gDkvCwo2y5bWND/QdgTa1k7oR0nAmIPSuRv tva5v07QNPxkxujznwb0EvN56jaIyEMmVNQe+M9iMmVvLOtxeo698RFzyY9k+xER keNRRp4TerbeGsVB1dW9a4EjL4mkpP//DIBirINHt74ZnBuDeWTXjmEZXoN04kZn ksQ82LQ8yVWcby599jSHv6KuHY58Z6SNj6/MVhIY/rv2mWgJqGRC1URB29SYvdWi AxAf4qmo2WmcOv7t4eMs =0iOv -----END PGP SIGNATURE----- --kF9LAPPYk2MosVMu--