Skip to contentSkip to conversation

Add SPDIF support for Allwinner SoCs

PATCH Superseded [email protected] 4 participants 10 messages Feb 2, 2016 – Feb 3, 2016

Subsystems (4)

v3 of 7 Superseded Latest is v7 →

lore
Share & tools
lore
#0 lore raw

v3 cover Add SPDIF support for Allwinner SoCs

From: Marcus Cooper <[email protected]>

This patch set adds support for the Allwinner SPDIF transceiver.

For now just the SPDIF transmitter has been tested on a Mele A2000(A10) and a Itead Ibox(A20).

In order for this patch set to be functional we require an audio clock patch which will be delivered separately. For those that are interested I've pushed the patches here with all the required changes to get SPDIF audio out of the device.

https://github.com/codekipper/linux-sunxi/commits/spdif_delivery

This is version 3 of the patch series. It's been quite a while since v2 and I'm hoping now that all the previous comments have been acted on.

Thanks in advance,
CK

---
Changes since v2:
- removed sunxi-machine-spdif and replaced with simple audio card
- removed untested compatibilities from device tree documentation
- added pm_runtime and remove shutdown
- removed pll2 as it's the parent of the spdif clock
- rename clk to spdif_clk
- removed enabling of mclk output
- removed interrupts status being cleared as it's not being used
- use default fifo settings for now
- fixed alignments for wrapped lines

Changes since v1:
- Moved sunxi-machine-spdif.c to separate patch
- replaced sunxi in naming scheme with sun4i in the sun4i-spdif driver.
- split tx controller into separate enable/disable functions
- moved setclk and setfmt functionality into hw params
- added support for mono signals.
- cleaned up probe clock set up.
- removed all writes to transmit status registers.
- removed of_id
- removed power management code.
- Added support for more rates.

Marcus Cooper (2):
dt-bindings: add sun4i SPDIF transceiver bindings
ASOC: sunxi: Add support for the SPDIF block

 .../devicetree/bindings/sound/sun4i,spdif.txt      |  46 ++
 sound/soc/sunxi/Kconfig                            |   8 +
 sound/soc/sunxi/Makefile                           |   1 +
 sound/soc/sunxi/sun4i-spdif.c                      | 586 +++++++++++++++++++++
 4 files changed, 641 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/sun4i,spdif.txt
 create mode 100644 sound/soc/sunxi/sun4i-spdif.c
Signature · 1 line
2.7.0
#1 lore raw

PATCH v3 1/2 dt-bindings: add sun4i SPDIF transceiver bindings

From: Marcus Cooper <[email protected]>

Add devicetree bindings for the SPDIF transceiver found on found on Allwinners A10 and A20 SoCs.

---
 .../devicetree/bindings/sound/sun4i,spdif.txt      | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/sun4i,spdif.txt
Documentation/devicetree/bindings/sound/sun4i,spdif.txt new file +46 -0
new file mode 100644
index 0000000..55ed2f6
@@ -0,0 +1,46 @@
+Allwinner Sony/Philips Digital Interface Format (S/PDIF) Controller
+
+The Allwinner S/PDIF audio block is a transceiver that allows the
+processor to receive and transmit digital audio via an coaxial cable or
+a fibre cable.
+
+Required properties:
+
+  - compatible		: should be one of the following:
+    - "allwinner,sun4i-a10-spdif": for the Allwinner A10 SoC
+
+  - reg			: Offset and length of the register set for the device.
+
+  - interrupts		: Contains the spdif interrupt.
+
+  - dmas		: Generic dma devicetree binding as described in
+			  Documentation/devicetree/bindings/dma/dma.txt.
+
+  - dma-names		: Two dmas have to be defined, "tx" and "rx".
+
+  - clocks		: Contains an entry for each entry in clock-names.
+
+  - clock-names		: Includes the following entries:
+	"apb"		  clock for the spdif bus.
+	"spdif"		  clock for spdif controller.
+
+Optional:
+
+  - spdif-in		: Enable block for capturing an SPDIF signal.
+
+  - spdif-out		: Enable block for transmitting an SPDIF signal.
+
+Example:
+
+spdif: spdif@01c21000 {
+	compatible = "allwinner,sun4i-a10-spdif";
+	reg = <0x01c21000 0x40>;
+	interrupts = <13>;
+	clocks = <&apb0_gates 1>, <&spdif_clk>;
+	clock-names = "apb", "spdif";
+	dmas = <&dma 0 2>, <&dma 0 2>;
+	dma-names = "rx", "tx";
+	spdif-in = "disabled";
+	spdif-out = "okay";
+	status = "okay";
+};
Signature · 1 line
2.7.0
#2 lore raw

Re: [PATCH v3 1/2] dt-bindings: add sun4i SPDIF transceiver bindings

Hi,

Quoted 38 lines · On Tue, Feb 02, 2016 at 03:49:53PM +0100…

From: Marcus Cooper <[email protected]>

Add devicetree bindings for the SPDIF transceiver found on found on Allwinners A10 and A20 SoCs.

---
 .../devicetree/bindings/sound/sun4i,spdif.txt      | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/sun4i,spdif.txt
Documentation/devicetree/bindings/sound/sun4i,spdif.txt new file +21 -0
new file mode 100644
index 0000000..55ed2f6
@@ -0,0 +1,46 @@
+Allwinner Sony/Philips Digital Interface Format (S/PDIF) Controller
+
+The Allwinner S/PDIF audio block is a transceiver that allows the
+processor to receive and transmit digital audio via an coaxial cable or
+a fibre cable.
+
+Required properties:
+
+  - compatible		: should be one of the following:
+    - "allwinner,sun4i-a10-spdif": for the Allwinner A10 SoC
+
+  - reg			: Offset and length of the register set for the device.
+
+  - interrupts		: Contains the spdif interrupt.
+
+  - dmas		: Generic dma devicetree binding as described in
+			  Documentation/devicetree/bindings/dma/dma.txt.
+
+  - dma-names		: Two dmas have to be defined, "tx" and "rx".
+
+  - clocks		: Contains an entry for each entry in clock-names.

Documentation/devicetree/bindings/sound/sun4i,spdif.txt line 31

Quoted 10 lines
Documentation/devicetree/bindings/sound/sun4i,spdif.txt new file +10 -0
+
+  - clock-names		: Includes the following entries:
+	"apb"		  clock for the spdif bus.
+	"spdif"		  clock for spdif controller.
+
+Optional:
+
+  - spdif-in		: Enable block for capturing an SPDIF signal.
+
+  - spdif-out		: Enable block for transmitting an SPDIF signal.

Is that a generic property? What values can it hold?

Thanks,
Maxime

Signature · 3 lines
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
#3 lore raw

Re: [PATCH v3 1/2] dt-bindings: add sun4i SPDIF transceiver bindings

Quoted 45 lines · Maxime Ripard

Hi,

Quoted 42 lines · On Tue, Feb 02, 2016 at 03:49:53PM +0100…

From: Marcus Cooper <[email protected]>

Add devicetree bindings for the SPDIF transceiver found on found on Allwinners A10 and A20 SoCs.

---
 .../devicetree/bindings/sound/sun4i,spdif.txt      | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/sun4i,spdif.txt
Documentation/devicetree/bindings/sound/sun4i,spdif.txt new file +25 -0
new file mode 100644
index 0000000..55ed2f6
@@ -0,0 +1,46 @@
+Allwinner Sony/Philips Digital Interface Format (S/PDIF) Controller
+
+The Allwinner S/PDIF audio block is a transceiver that allows the
+processor to receive and transmit digital audio via an coaxial cable or
+a fibre cable.
+
+Required properties:
+
+  - compatible               : should be one of the following:
+    - "allwinner,sun4i-a10-spdif": for the Allwinner A10 SoC
+
+  - reg                      : Offset and length of the register set for the device.
+
+  - interrupts               : Contains the spdif interrupt.
+
+  - dmas             : Generic dma devicetree binding as described in
+                       Documentation/devicetree/bindings/dma/dma.txt.
+
+  - dma-names                : Two dmas have to be defined, "tx" and "rx".
+
+  - clocks           : Contains an entry for each entry in clock-names.
+
+  - clock-names              : Includes the following entries:
+     "apb"             clock for the spdif bus.
+     "spdif"           clock for spdif controller.
Quoted 8 lines

Documentation/devicetree/bindings/sound/sun4i,spdif.txt line 31

Quoted 6 lines
Documentation/devicetree/bindings/sound/sun4i,spdif.txt new file +6 -0
+
+Optional:
+
+  - spdif-in         : Enable block for capturing an SPDIF signal.
+
+  - spdif-out                : Enable block for transmitting an SPDIF signal.

Is that a generic property? What values can it hold?

Hi Maxime,

thanks for the review. I'm starting to prep a v4 patch series and I'm here wondering if I should drop the spdif-in/out for now and just rely on the status property to enable playback. It would simplify the driver even more and it could be added back when someone gets around to delivering spdif capture. I think that's going to be pretty low on my list of things to do as I've only got one dev board where it's possible.
BR,
Lazy Lazy CK

Quoted 8 lines

Thanks,
Maxime

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

#4 lore raw

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

In reply to [email protected]

From: Marcus Cooper <[email protected]>

The sun4i, sun5i and sun7i SoC families have an SPDIF block which is capable of playback and capture.

This patch enables the playback of this block for
the sun4i families.

---
 sound/soc/sunxi/Kconfig       |   8 +
 sound/soc/sunxi/Makefile      |   1 +
 sound/soc/sunxi/sun4i-spdif.c | 586 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 595 insertions(+)
 create mode 100644 sound/soc/sunxi/sun4i-spdif.c
sound/soc/sunxi/Kconfig +8 -0
index 84c72ec..6af7224 100644
@@ -8,4 +8,12 @@ config SND_SUN4I_CODEC
 	  Select Y or M to add support for the Codec embedded in the Allwinner
 	  A10 and affiliated SoCs.
 
+config SND_SUN4I_SPDIF
+        tristate "Allwinner A10 SPDIF Support"
+	depends on OF
+        select SND_SOC_GENERIC_DMAENGINE_PCM
+        select REGMAP_MMIO
+        help
+          Say Y or M to add support for the S/PDIF audio block in the Allwinner
+	  A10 and affiliated SoCs.
 endmenu
sound/soc/sunxi/Makefile +1 -0
index ea8a08c..8f5e889 100644
@@ -1,2 +1,3 @@
 obj-$(CONFIG_SND_SUN4I_CODEC) += sun4i-codec.o
 
+obj-$(CONFIG_SND_SUN4I_SPDIF) += sun4i-spdif.o
sound/soc/sunxi/sun4i-spdif.c new file +586 -0

586 lines not on this page · raw message

Signature · 1 line
2.7.0
#5 lore raw

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

coccinelle warnings: (new ones prefixed by >>)

Hi Marcus,

[auto build test WARNING on asoc/for-next]
[also build test WARNING on v4.5-rc2 next-20160201]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/codekipper-gmail-com/Add-SPDIF-support-for-Allwinner-SoCs/20160202-225229
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

coccinelle warnings: (new ones prefixed by >>)

>> sound/soc/sunxi/sun4i-spdif.c:572:3-8: No need to set .owner here. The core will do it.

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

#6 lore raw

[PATCH] ASOC: sunxi: fix platform_no_drv_owner.cocci warnings

In reply to [email protected]
sound/soc/sunxi/sun4i-spdif.c:572:3-8: No need to set .owner here. The core will do it.

sound/soc/sunxi/sun4i-spdif.c:572:3-8: No need to set .owner here. The core will do it.

Remove .owner field if calls are used which set it automatically

Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci

---

 sun4i-spdif.c |    1 -
 1 file changed, 1 deletion(-)
sound/soc/sunxi/sun4i-spdif.c +0 -1
@@ -569,7 +569,6 @@ static const struct dev_pm_ops sun4i_spd
 static struct platform_driver sun4i_spdif_driver = {
 	.driver		= {
 		.name	= "sun4i-spdif",
-		.owner	= THIS_MODULE,
 		.of_match_table = of_match_ptr(sun4i_spdif_of_match),
 		.pm	= &sun4i_spdif_pm,
 	},
#7 lore raw

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

In reply to [email protected]

Hi,

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

Quoted 9 lines · On Tue, Feb 02, 2016 at 03:49:54PM +0100…
sound/soc/sunxi/sun4i-spdif.c new file +9 -0
+#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;
+}

sound/soc/sunxi/sun4i-spdif.c line 450

Quoted 10 lines
sound/soc/sunxi/sun4i-spdif.c new file +10 -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.

sound/soc/sunxi/sun4i-spdif.c line 463

Quoted 13 lines
sound/soc/sunxi/sun4i-spdif.c new file +13 -0
+
+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.

sound/soc/sunxi/sun4i-spdif.c line 468

Quoted 5 lines
sound/soc/sunxi/sun4i-spdif.c new file +5 -0
+
+	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.

Quoted 62 lines
sound/soc/sunxi/sun4i-spdif.c new file +62 -0
+	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;

sound/soc/sunxi/sun4i-spdif.c line 541

Quoted 10 lines
sound/soc/sunxi/sun4i-spdif.c new file +10 -0
+
+	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.

Quoted 5 lines
sound/soc/sunxi/sun4i-spdif.c new file +5 -0
+exit_clkdisable_apb_clk:
+	clk_disable_unprepare(host->apb_clk);
+	return ret;
+}
+

sound/soc/sunxi/sun4i-spdif.c line 556

Quoted 10 lines
sound/soc/sunxi/sun4i-spdif.c new file +10 -0
+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.

Review

Quoted 1 line
(quoted diff) +1 -0
+		clk_disable_unprepare(host->spdif_clk);

And here as well.

Quoted 5 lines
sound/soc/sunxi/sun4i-spdif.c new file +5 -0
+		clk_disable_unprepare(host->apb_clk);
+	}
+
+	return 0;
+}

sound/soc/sunxi/sun4i-spdif.c line 572

Quoted 10 lines
sound/soc/sunxi/sun4i-spdif.c new file +10 -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.

sound/soc/sunxi/sun4i-spdif.c line 574

Quoted 2 lines
sound/soc/sunxi/sun4i-spdif.c new file +2 -0
+		.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?

sound/soc/sunxi/sun4i-spdif.c line 586

Quoted 12 lines
sound/soc/sunxi/sun4i-spdif.c new file +12 -0
+	},
+	.probe		= sun4i_spdif_probe,
+	.remove		= sun4i_spdif_remove,
+};
+
+module_platform_driver(sun4i_spdif_driver);
+
+MODULE_AUTHOR("Marcus Cooper <[email protected]>");
+MODULE_AUTHOR("Andrea Venturi <be17068-p0aYb1w59bq9tCD/[email protected]>");
+MODULE_DESCRIPTION("Allwinner sun4i SPDIF SoC Interface");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:sun4i-spdif");

Thanks!
Maxime

Signature · 3 lines
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
#8 lore raw

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

Quoted 22 lines · Maxime Ripard

Hi,

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

Quoted 17 lines · On Tue, Feb 02, 2016 at 03:49:54PM +0100…
sound/soc/sunxi/sun4i-spdif.c new file +17 -0
+#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;
Quoted 8 lines

Review

Quoted 2 lines
(quoted diff) +2 -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.

ACK

Quoted 16 lines
Quoted 4 lines
sound/soc/sunxi/sun4i-spdif.c new file +3 -0
+
+static int sun4i_spdif_probe(struct platform_device *pdev)
+{

sound/soc/sunxi/sun4i-spdif.c line 463

Quoted 10 lines
sound/soc/sunxi/sun4i-spdif.c new file +10 -0
+     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.

ACK

Quoted 8 lines

sound/soc/sunxi/sun4i-spdif.c line 468

Quoted 6 lines
sound/soc/sunxi/sun4i-spdif.c new file +5 -0
+
+     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.

ACK

Quoted 67 lines
Quoted 67 lines
sound/soc/sunxi/sun4i-spdif.c new file +66 -0
+     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);
Quoted 8 lines

sound/soc/sunxi/sun4i-spdif.c line 541

Quoted 6 lines
sound/soc/sunxi/sun4i-spdif.c new file +6 -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.

ACK

Quoted 33 lines
Quoted 6 lines
sound/soc/sunxi/sun4i-spdif.c new file +5 -0
+exit_clkdisable_apb_clk:
+     clk_disable_unprepare(host->apb_clk);
+     return ret;
+}
+

sound/soc/sunxi/sun4i-spdif.c line 556

Quoted 10 lines
sound/soc/sunxi/sun4i-spdif.c new file +10 -0
+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.

Quoted 1 line

+ clk_disable_unprepare(host->spdif_clk);

And here as well.

Quoted 9 lines
sound/soc/sunxi/sun4i-spdif.c new file +9 -0
+             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)
Quoted 8 lines

sound/soc/sunxi/sun4i-spdif.c line 572

Quoted 6 lines
sound/soc/sunxi/sun4i-spdif.c new file +6 -0
+};
+
+static struct platform_driver sun4i_spdif_driver = {
+     .driver         = {
+             .name   = "sun4i-spdif",
+             .owner  = THIS_MODULE,

This is not needed.

ACK

Quoted 7 lines

sound/soc/sunxi/sun4i-spdif.c line 574

Quoted 3 lines
sound/soc/sunxi/sun4i-spdif.c new file +2 -0
+             .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?

No but I'll have a go!

Quoted 16 lines

sound/soc/sunxi/sun4i-spdif.c line 586

Quoted 13 lines
sound/soc/sunxi/sun4i-spdif.c new file +12 -0
+     },
+     .probe          = sun4i_spdif_probe,
+     .remove         = sun4i_spdif_remove,
+};
+
+module_platform_driver(sun4i_spdif_driver);
+
+MODULE_AUTHOR("Marcus Cooper <[email protected]>");
+MODULE_AUTHOR("Andrea Venturi <be17068-p0aYb1w59bq9tCD/[email protected]>");
+MODULE_DESCRIPTION("Allwinner sun4i SPDIF SoC Interface");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:sun4i-spdif");

Thanks!
Maxime

Thanks,
CK

Quoted 5 lines

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

maintainer
#9 lore raw
In reply to [email protected]
Quoted 3 lines · On Tue, Feb 02, 2016 at 03:49:52PM +0100…

Marcus Cooper (2):
dt-bindings: add sun4i SPDIF transceiver bindings
ASOC: sunxi: Add support for the SPDIF block

Please do try to use subject lines matching the style for the subsystem.