[alsa-devel] [PATCH v3 0/4] Add SPDIF support for rockchip
This patchset adds support for the Rockchip SPDIF transceiver as present on RK3066, RK3188 and RK3288 boards and enables it on a Radxa rock pro. Tested on a Radxa Rock Pro board.
Changes in v3: - Fix typos in commit message - Resent patches to a more complete list of maintainers.
Changes in v2: - Remove platform: module alias as it was unused - Call MODULE_DEVICE_TABLE(of, ) right after the of match table - use rk_spdif as a prefix consistenly throughout the driver - Check return code of regmap_update and bubble it up - Sort the spdif node properties - Drop the 0x prefix from the node name - Rename the spdif@ node to sound@
Sjoerd Simons (4): ASoC: dt-bindings: add rockchip tranceiver bindings ASoC: rockchip: Add rockchip SPDIF transceiver driver ARM: dts: rockchip: Add SPDIF transceiver for RK3188 ARM: dts: rockchip: Add SPDIF optical out on Radxa Rock
.../devicetree/bindings/sound/rockchip-spdif.txt | 41 +++ arch/arm/boot/dts/rk3188-radxarock.dts | 19 + arch/arm/boot/dts/rk3188.dtsi | 22 ++ sound/soc/rockchip/Kconfig | 8 + sound/soc/rockchip/Makefile | 2 + sound/soc/rockchip/rockchip_spdif.c | 381 +++++++++++++++++++++ sound/soc/rockchip/rockchip_spdif.h | 63 ++++ 7 files changed, 536 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/rockchip-spdif.txt create mode 100644 sound/soc/rockchip/rockchip_spdif.c create mode 100644 sound/soc/rockchip/rockchip_spdif.h
Add devicetree bindings for the spdif tranceiver found on found on rk3066, rk3188 and rk3288 SoCs
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk ---
Changes in v3: None Changes in v2: None
.../devicetree/bindings/sound/rockchip-spdif.txt | 41 ++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/rockchip-spdif.txt
diff --git a/Documentation/devicetree/bindings/sound/rockchip-spdif.txt b/Documentation/devicetree/bindings/sound/rockchip-spdif.txt new file mode 100644 index 0000000..2da8a09 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/rockchip-spdif.txt @@ -0,0 +1,41 @@ +* Rockchip SPDIF transceiver + +The S/PDIF audio block is a stereo 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: + - "rockchip,rk3066-spdif": for rk3066 + - "rockchip,rk3188-spdif", "rockchip,rk3066-spdif": for rk3188 + - "rockchip,rk3288-spdif", "rockchip,rk3066-spdif": for rk3288 +- reg: physical base address of the controller and length of memory mapped + region. +- interrupts: should contain the SPDIF interrupt. +- #address-cells: should be 1. +- #size-cells: should be 0. +- dmas: DMA specifiers for tx dma. See the DMA client binding, + Documentation/devicetree/bindings/dma/dma.txt +- dma-names: should be "tx" +- clocks: a list of phandle + clock-specifier pairs, one for each entry + in clock-names. +- clock-names: should contain following: + - "spdif_hclk": clock for SPPIF controller + - "spdif_clk" : clock for SPDIF bus + +Example for the rk3188 SPDIF controller: + +spdif: spdif@0x1011e000 { + compatible = "rockchip,rk3188-spdif", "rockchip,rk3066-spdif"; + reg = <0x1011e000 0x2000>; + interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>; + #address-cells = <1>; + #size-cells = <0>; + dmas = <&dmac1_s 8>; + dma-names = "tx"; + clock-names = "spdif_hclk", "spdif_clk"; + clocks = <&cru HCLK_SPDIF>, <&cru SCLK_SPDIF>; + status = "disabled"; + #sound-dai-cells = <0>; +};
On Fri, Aug 07, 2015 at 01:44:11PM +0200, Sjoerd Simons wrote:
+- compatible: should be one of the following:
- "rockchip,rk3066-spdif": for rk3066
- "rockchip,rk3188-spdif", "rockchip,rk3066-spdif": for rk3188
- "rockchip,rk3288-spdif", "rockchip,rk3066-spdif": for rk3288
The binding claims that we support these three compatibles but the driver only supports rockchip,rk3066-spdif. It's better to list all the compatibles in the binding even if they all boil down to the same thing since it's less error prone when we do start adding device specific code - people are less likely to forget to handle some variant properly.
Add a driver for the SPDIF transceiver available on RK3066, RK3188 and RK3288. Heavily based on the rockchip i2s driver.
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk ---
Changes in v3: - Fix typos in commit message
Changes in v2: - Remove platform: module alias as it was unused - Call MODULE_DEVICE_TABLE(of, ) right after the of match table - use rk_spdif as a prefix consistenly throughout the driver - Check return code of regmap_update and bubble it up
sound/soc/rockchip/Kconfig | 8 + sound/soc/rockchip/Makefile | 2 + sound/soc/rockchip/rockchip_spdif.c | 381 ++++++++++++++++++++++++++++++++++++ sound/soc/rockchip/rockchip_spdif.h | 63 ++++++ 4 files changed, 454 insertions(+) create mode 100644 sound/soc/rockchip/rockchip_spdif.c create mode 100644 sound/soc/rockchip/rockchip_spdif.h
diff --git a/sound/soc/rockchip/Kconfig b/sound/soc/rockchip/Kconfig index 58bae8e..603fd1f 100644 --- a/sound/soc/rockchip/Kconfig +++ b/sound/soc/rockchip/Kconfig @@ -15,6 +15,14 @@ config SND_SOC_ROCKCHIP_I2S Rockchip I2S device. The device supports upto maximum of 8 channels each for play and record.
+config SND_SOC_ROCKCHIP_SPDIF + tristate "Rockchip SPDIF Device Driver" + depends on CLKDEV_LOOKUP && SND_SOC_ROCKCHIP + select SND_SOC_GENERIC_DMAENGINE_PCM + help + Say Y or M if you want to add support for SPDIF driver for + Rockchip SPDIF transceiver device. + config SND_SOC_ROCKCHIP_MAX98090 tristate "ASoC support for Rockchip boards using a MAX98090 codec" depends on SND_SOC_ROCKCHIP && I2C && GPIOLIB diff --git a/sound/soc/rockchip/Makefile b/sound/soc/rockchip/Makefile index 1bc1dc3..9a244d7 100644 --- a/sound/soc/rockchip/Makefile +++ b/sound/soc/rockchip/Makefile @@ -1,7 +1,9 @@ # ROCKCHIP Platform Support snd-soc-i2s-objs := rockchip_i2s.o +snd-soc-spdif-objs := rockchip_spdif.o
obj-$(CONFIG_SND_SOC_ROCKCHIP_I2S) += snd-soc-i2s.o +obj-$(CONFIG_SND_SOC_ROCKCHIP_SPDIF) += snd-soc-spdif.o
snd-soc-rockchip-max98090-objs := rockchip_max98090.o snd-soc-rockchip-rt5645-objs := rockchip_rt5645.o diff --git a/sound/soc/rockchip/rockchip_spdif.c b/sound/soc/rockchip/rockchip_spdif.c new file mode 100644 index 0000000..39d33c0 --- /dev/null +++ b/sound/soc/rockchip/rockchip_spdif.c @@ -0,0 +1,381 @@ +/* sound/soc/rockchip/rk_spdif.c + * + * ALSA SoC Audio Layer - Rockchip I2S Controller driver + * + * Copyright (c) 2014 Rockchip Electronics Co. Ltd. + * Author: Jianqun jay.xu@rock-chips.com + * Copyright (c) 2015 Collabora Ltd. + * Author: Sjoerd Simons sjoerd.simons@collabora.co.uk + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/module.h> +#include <linux/delay.h> +#include <linux/of_gpio.h> +#include <linux/clk.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> +#include <sound/pcm_params.h> +#include <sound/dmaengine_pcm.h> + +#include "rockchip_spdif.h" + +struct rk_spdif_dev { + struct device *dev; + + struct clk *mclk; + struct clk *hclk; + + struct snd_dmaengine_dai_dma_data playback_dma_data; + + struct regmap *regmap; +}; + +static int rk_spdif_runtime_suspend(struct device *dev) +{ + struct rk_spdif_dev *spdif = dev_get_drvdata(dev); + + clk_disable_unprepare(spdif->mclk); + + return 0; +} + +static int rk_spdif_runtime_resume(struct device *dev) +{ + struct rk_spdif_dev *spdif = dev_get_drvdata(dev); + int ret; + + ret = clk_prepare_enable(spdif->mclk); + if (ret) { + dev_err(spdif->dev, "clock enable failed %d\n", ret); + return ret; + } + + return 0; +} + +static inline struct rk_spdif_dev *to_info(struct snd_soc_dai *dai) +{ + return snd_soc_dai_get_drvdata(dai); +} + +static int rk_spdif_snd_txctrl(struct rk_spdif_dev *spdif, int on) +{ + int ret; + + if (on) { + ret = regmap_update_bits(spdif->regmap, SPDIF_DMACR, + SPDIF_DMACR_TDE_ENABLE, + SPDIF_DMACR_TDE_ENABLE); + + if (ret != 0) + goto bail; + + ret = regmap_update_bits(spdif->regmap, SPDIF_XFER, + SPDIF_XFER_TXS_START, + SPDIF_XFER_TXS_START); + } else { + ret = regmap_update_bits(spdif->regmap, SPDIF_DMACR, + SPDIF_DMACR_TDE_ENABLE, + SPDIF_DMACR_TDE_DISABLE); + + if (ret != 0) + goto bail; + + ret = regmap_update_bits(spdif->regmap, SPDIF_XFER, + SPDIF_XFER_TXS_START, + SPDIF_XFER_TXS_STOP); + } + +bail: + return ret; +} + +static int rk_spdif_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) +{ + struct rk_spdif_dev *spdif = to_info(dai); + unsigned int val = SPDIF_CFGR_HALFWORD_ENABLE; + int srate, mclk; + + srate = params_rate(params); + switch (srate) { + case 32000: + case 48000: + case 96000: + mclk = 12288000; + break; + case 44100: + mclk = 11289600; + break; + default: + return -EINVAL; + } + + switch (params_format(params)) { + case SNDRV_PCM_FORMAT_S16_LE: + val |= SPDIF_CFGR_VDW_16; + break; + case SNDRV_PCM_FORMAT_S20_3LE: + val |= SPDIF_CFGR_VDW_20; + break; + case SNDRV_PCM_FORMAT_S24_LE: + val |= SPDIF_CFGR_VDW_24; + break; + default: + return -EINVAL; + } + + /* Set clock and calculate divider */ + clk_set_rate(spdif->mclk, mclk); + val |= SPDIF_CFGR_CLK_DIV(mclk/(srate * 256)); + + return regmap_update_bits(spdif->regmap, SPDIF_CFGR, + SPDIF_CFGR_CLK_DIV_MASK | SPDIF_CFGR_HALFWORD_ENABLE | + SDPIF_CFGR_VDW_MASK, + val); +} + +static int rk_spdif_trigger(struct snd_pcm_substream *substream, + int cmd, struct snd_soc_dai *dai) +{ + struct rk_spdif_dev *spdif = to_info(dai); + int ret; + + switch (cmd) { + case SNDRV_PCM_TRIGGER_START: + case SNDRV_PCM_TRIGGER_RESUME: + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: + ret = rk_spdif_snd_txctrl(spdif, true); + break; + case SNDRV_PCM_TRIGGER_SUSPEND: + case SNDRV_PCM_TRIGGER_STOP: + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: + ret = rk_spdif_snd_txctrl(spdif, false); + break; + default: + ret = -EINVAL; + break; + } + + return ret; +} + +static int rk_spdif_dai_probe(struct snd_soc_dai *dai) +{ + struct rk_spdif_dev *spdif = snd_soc_dai_get_drvdata(dai); + + dai->playback_dma_data = &spdif->playback_dma_data; + + return 0; +} + +static const struct snd_soc_dai_ops rk_spdif_dai_ops = { + .hw_params = rk_spdif_hw_params, + .trigger = rk_spdif_trigger, +}; + +static struct snd_soc_dai_driver rk_spdif_dai = { + .probe = rk_spdif_dai_probe, + .playback = { + .stream_name = "Playback", + .channels_min = 2, + .channels_max = 2, + .rates = (SNDRV_PCM_RATE_32000 | + SNDRV_PCM_RATE_44100 | + SNDRV_PCM_RATE_48000 | + SNDRV_PCM_RATE_96000), + .formats = (SNDRV_PCM_FMTBIT_S16_LE | + SNDRV_PCM_FMTBIT_S20_3LE | + SNDRV_PCM_FMTBIT_S24_LE), + }, + .ops = &rk_spdif_dai_ops, +}; + +static const struct snd_soc_component_driver rk_spdif_component = { + .name = "rockchip-spdif", +}; + +static bool rk_spdif_wr_reg(struct device *dev, unsigned int reg) +{ + switch (reg) { + case SPDIF_CFGR: + case SPDIF_DMACR: + case SPDIF_INTCR: + case SPDIF_XFER: + case SPDIF_SMPDR: + return true; + default: + return false; + } +} + +static bool rk_spdif_rd_reg(struct device *dev, unsigned int reg) +{ + switch (reg) { + case SPDIF_CFGR: + case SPDIF_SDBLR: + case SPDIF_INTCR: + case SPDIF_INTSR: + case SPDIF_XFER: + return true; + default: + return false; + } +} + +static bool rk_spdif_volatile_reg(struct device *dev, unsigned int reg) +{ + switch (reg) { + case SPDIF_INTSR: + case SPDIF_SDBLR: + return true; + default: + return false; + } +} + +static const struct regmap_config rk_spdif_regmap_config = { + .reg_bits = 32, + .reg_stride = 4, + .val_bits = 32, + .max_register = SPDIF_SMPDR, + .writeable_reg = rk_spdif_wr_reg, + .readable_reg = rk_spdif_rd_reg, + .volatile_reg = rk_spdif_volatile_reg, + .cache_type = REGCACHE_FLAT, +}; + +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"); + 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"); + 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; + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + regs = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(regs)) + return PTR_ERR(regs); + + spdif->regmap = devm_regmap_init_mmio(&pdev->dev, regs, + &rk_spdif_regmap_config); + if (IS_ERR(spdif->regmap)) { + dev_err(&pdev->dev, + "Failed to initialise managed register map\n"); + return PTR_ERR(spdif->regmap); + } + + spdif->playback_dma_data.addr = res->start + SPDIF_SMPDR; + spdif->playback_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; + spdif->playback_dma_data.maxburst = 4; + + spdif->dev = &pdev->dev; + dev_set_drvdata(&pdev->dev, spdif); + + pm_runtime_enable(&pdev->dev); + if (!pm_runtime_enabled(&pdev->dev)) { + ret = rk_spdif_runtime_resume(&pdev->dev); + if (ret) + goto err_pm_disable; + } + + ret = devm_snd_soc_register_component(&pdev->dev, + &rk_spdif_component, + &rk_spdif_dai, 1); + if (ret) { + dev_err(&pdev->dev, "Could not register DAI\n"); + goto err_suspend; + } + + ret = snd_dmaengine_pcm_register(&pdev->dev, NULL, 0); + if (ret) { + dev_err(&pdev->dev, "Could not register PCM\n"); + goto err_pcm_register; + } + + return 0; + +err_pcm_register: + snd_dmaengine_pcm_unregister(&pdev->dev); +err_suspend: + if (!pm_runtime_status_suspended(&pdev->dev)) + rk_spdif_runtime_suspend(&pdev->dev); +err_pm_disable: + pm_runtime_disable(&pdev->dev); + + return ret; +} + +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); + + return 0; +} + +static const struct of_device_id rk_spdif_match[] = { + { .compatible = "rockchip,rk3066-spdif", }, + {}, +}; +MODULE_DEVICE_TABLE(of, rk_spdif_match); + +static const struct dev_pm_ops rk_spdif_pm_ops = { + SET_RUNTIME_PM_OPS(rk_spdif_runtime_suspend, rk_spdif_runtime_resume, + NULL) +}; + +static struct platform_driver rk_spdif_driver = { + .probe = rk_spdif_probe, + .remove = rk_spdif_remove, + .driver = { + .name = "rockchip-spdif", + .of_match_table = of_match_ptr(rk_spdif_match), + .pm = &rk_spdif_pm_ops, + }, +}; +module_platform_driver(rk_spdif_driver); + +MODULE_DESCRIPTION("ROCKCHIP SPDIF transceiver Interface"); +MODULE_AUTHOR("Sjoerd Simons sjoerd.simons@collabora.co.uk"); +MODULE_LICENSE("GPL v2"); diff --git a/sound/soc/rockchip/rockchip_spdif.h b/sound/soc/rockchip/rockchip_spdif.h new file mode 100644 index 0000000..07f86a2 --- /dev/null +++ b/sound/soc/rockchip/rockchip_spdif.h @@ -0,0 +1,63 @@ +/* + * ALSA SoC Audio Layer - Rockchip SPDIF transceiver driver + * + * Copyright (c) 2015 Collabora Ltd. + * Author: Sjoerd Simons sjoerd.simons@collabora.co.uk + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef _ROCKCHIP_SPDIF_H +#define _ROCKCHIP_SPDIF_H + +/* + * CFGR + * transfer configuration register +*/ +#define SPDIF_CFGR_CLK_DIV_SHIFT (16) +#define SPDIF_CFGR_CLK_DIV_MASK (0xff << SPDIF_CFGR_CLK_DIV_SHIFT) +#define SPDIF_CFGR_CLK_DIV(x) (x << SPDIF_CFGR_CLK_DIV_SHIFT) + +#define SPDIF_CFGR_HALFWORD_SHIFT 2 +#define SPDIF_CFGR_HALFWORD_DISABLE (0 << SPDIF_CFGR_HALFWORD_SHIFT) +#define SPDIF_CFGR_HALFWORD_ENABLE (1 << SPDIF_CFGR_HALFWORD_SHIFT) + +#define SPDIF_CFGR_VDW_SHIFT 0 +#define SPDIF_CFGR_VDW(x) (x << SPDIF_CFGR_VDW_SHIFT) +#define SDPIF_CFGR_VDW_MASK (0xf << SPDIF_CFGR_VDW_SHIFT) + +#define SPDIF_CFGR_VDW_16 SPDIF_CFGR_VDW(0x00) +#define SPDIF_CFGR_VDW_20 SPDIF_CFGR_VDW(0x01) +#define SPDIF_CFGR_VDW_24 SPDIF_CFGR_VDW(0x10) + +/* + * DMACR + * DMA control register +*/ +#define SPDIF_DMACR_TDE_SHIFT 5 +#define SPDIF_DMACR_TDE_DISABLE (0 << SPDIF_DMACR_TDE_SHIFT) +#define SPDIF_DMACR_TDE_ENABLE (1 << SPDIF_DMACR_TDE_SHIFT) + +#define SPDIF_DMACR_TDL_SHIFT 0 +#define SPDIF_DMACR_TDL(x) ((x) << SPDIF_DMACR_TDL_SHIFT) +#define SPDIF_DMACR_TDL_MASK (0x1f << SDPIF_DMACR_TDL_SHIFT) + +/* + * XFER + * Transfer control register +*/ +#define SPDIF_XFER_TXS_SHIFT 0 +#define SPDIF_XFER_TXS_STOP (0 << SPDIF_XFER_TXS_SHIFT) +#define SPDIF_XFER_TXS_START (1 << SPDIF_XFER_TXS_SHIFT) + +#define SPDIF_CFGR (0x0000) +#define SPDIF_SDBLR (0x0004) +#define SPDIF_DMACR (0x0008) +#define SPDIF_INTCR (0x000c) +#define SPDIF_INTSR (0x0010) +#define SPDIF_XFER (0x0018) +#define SPDIF_SMPDR (0x0020) + +#endif /* _ROCKCHIP_SPDIF_H */
On Fri, Aug 07, 2015 at 01:44:12PM +0200, Sjoerd Simons wrote:
Changes in v2:
- Remove platform: module alias as it was unused
Ugh, I see Paul Bolle replied to one of your earlier patches :/ Please just ignore him, he doesn't understand that we don't require machine definitions to be in tree. A platform: module alias is useful for any platform device.
+static inline struct rk_spdif_dev *to_info(struct snd_soc_dai *dai) +{
- return snd_soc_dai_get_drvdata(dai);
+}
I'm not sure this is adding much to the two users but whatever.
+static int rk_spdif_snd_txctrl(struct rk_spdif_dev *spdif, int on) +{
- int ret;
- if (on) {
ret = regmap_update_bits(spdif->regmap, SPDIF_DMACR,
SPDIF_DMACR_TDE_ENABLE,
SPDIF_DMACR_TDE_ENABLE);
if (ret != 0)
goto bail;
ret = regmap_update_bits(spdif->regmap, SPDIF_XFER,
SPDIF_XFER_TXS_START,
SPDIF_XFER_TXS_START);
This function has no common code, just two branches in an if/else that share nothing and is called from a single location. Why not just inline this into the trigger function?
- /* Set clock and calculate divider */
- clk_set_rate(spdif->mclk, mclk);
We should check the return value of clk_set_rate().
- spdif->hclk = devm_clk_get(&pdev->dev, "spdif_hclk");
- 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;
- }
I notice that we don't manage the hclk over suspend/resume - should we perhaps be using regmap_init_mmio_clk() together with runtime PM to manage it?
- pm_runtime_enable(&pdev->dev);
- if (!pm_runtime_enabled(&pdev->dev)) {
ret = rk_spdif_runtime_resume(&pdev->dev);
if (ret)
goto err_pm_disable;
- }
The normal pattern with this is to start the device powered up then idle it - this does mean we need to bounce the power on but fitting in with the common pattern is good and it's less conditional code.
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
Hey Heiko,
Thanks for the comments, i'm hoping to address them and Marks comments in a next patch series next week.
On Tue, 2015-08-18 at 20:25 +0200, Heiko Stuebner wrote:
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)
I'm entirely happy to change that, just wanted to note that the reason i used spdif_hclk and spidf_clk is for consistency with the rockchip -i2s binding which uses i2s_hclk ans i2s_mclk.
However i guess we could update the i2s binding at some poit as well to follow similar names (with fallbacks to the old one). In general a lot of the comments yourself and Mark have given actually apply to the rockchip-i2s code as well so there is some cleanup to do there :)
Hi Sjoerd,
Am Mittwoch, 19. August 2015, 08:56:48 schrieb Sjoerd Simons:
Hey Heiko,
Thanks for the comments, i'm hoping to address them and Marks comments in a next patch series next week.
On Tue, 2015-08-18 at 20:25 +0200, Heiko Stuebner wrote:
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)
I'm entirely happy to change that, just wanted to note that the reason i used spdif_hclk and spidf_clk is for consistency with the rockchip -i2s binding which uses i2s_hclk ans i2s_mclk.
I was also just remembering something Thierry Reding said in the edp thread: "The names are in a per-driver scope, so "dp-phy" is implied by the device tree binding and driver already. You could simply use shorter names such as "phy" and "24m" for example."
However i guess we could update the i2s binding at some poit as well to follow similar names (with fallbacks to the old one). In general a lot of the comments yourself and Mark have given actually apply to the rockchip-i2s code as well so there is some cleanup to do there :)
I'm not sure about updating the binding though, as it we'd need to provide backwards-compatibility for the old names as well. So I don't think renaming these will bring much benefit, but we can choose better names for followup drivers like the spdif.
I haven't looked to much into the i2s stuff yet. But with me getting a grasp on the fractional dividers and the hardware drivers making it into the asoc tree now, I also plan to give the i2s audio on the Chromebooks a try :-)
Heiko
Add the SPDIF transceiver controller and pin for RK3188
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
---
Changes in v3: None Changes in v2: - Sort the spdif node properties - Drop the 0x prefix from the node name - Rename the spdif@ node to sound@
arch/arm/boot/dts/rk3188.dtsi | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi index 0f23aed..abdb5bc 100644 --- a/arch/arm/boot/dts/rk3188.dtsi +++ b/arch/arm/boot/dts/rk3188.dtsi @@ -121,6 +121,22 @@ status = "disabled"; };
+ spdif: sound@1011e000 { + compatible = "rockchip,rk3188-spdif", "rockchip,rk3066-spdif"; + reg = <0x1011e000 0x2000>; + #address-cells = <1>; + #size-cells = <0>; + #sound-dai-cells = <0>; + clock-names = "spdif_hclk", "spdif_clk"; + clocks = <&cru HCLK_SPDIF>, <&cru SCLK_SPDIF>; + dmas = <&dmac1_s 8>; + dma-names = "tx"; + interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>; + pinctrl-names = "default"; + pinctrl-0 = <&spdif_tx>; + status = "disabled"; + }; + cru: clock-controller@20000000 { compatible = "rockchip,rk3188-cru"; reg = <0x20000000 0x1000>; @@ -462,6 +478,12 @@ <RK_GPIO1 21 RK_FUNC_1 &pcfg_pull_none>; }; }; + + spdif { + spdif_tx: spdif-tx { + rockchip,pins = <RK_GPIO1 14 RK_FUNC_1 &pcfg_pull_none>; + }; + }; }; };
This enables the SPDIF optical audio output on the Radxa Rock
Signed-off-by: Sjoerd Simons sjoerd.simons@collabora.co.uk
---
Changes in v3: - Resent patches to a more complete list of maintainers.
Changes in v2: None
arch/arm/boot/dts/rk3188-radxarock.dts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/arch/arm/boot/dts/rk3188-radxarock.dts b/arch/arm/boot/dts/rk3188-radxarock.dts index 4bb014d..dea020c 100644 --- a/arch/arm/boot/dts/rk3188-radxarock.dts +++ b/arch/arm/boot/dts/rk3188-radxarock.dts @@ -90,6 +90,21 @@ }; };
+ sound { + compatible = "simple-audio-card"; + simple-audio-card,name = "SPDIF"; + + simple-audio-card,dai-link@1 { /* S/PDIF - S/PDIF */ + cpu { sound-dai = <&spdif>; }; + codec { sound-dai = <&spdif_out>; }; + }; + }; + + spdif_out: spdif-out { + compatible = "linux,spdif-dit"; + #sound-dai-cells = <0>; + }; + ir_recv: gpio-ir-receiver { compatible = "gpio-ir-receiver"; gpios = <&gpio0 10 1>; @@ -343,6 +358,10 @@ }; };
+&spdif { + status = "okay"; +}; + &uart0 { status = "okay"; };
participants (3)
-
Heiko Stuebner
-
Mark Brown
-
Sjoerd Simons