[alsa-devel] [PATCH 0/5] ASoC: rockchip-i2s: patches for rockchip i2s driver
These patches to fix rockchip i2s driver bugs, also make driver codes reasonable. Tested on RK3288 board.
Jianqun (5): ASoC: rockchip-i2s: fix rockchip i2s defination more reasonable ASoC: rockchip-i2s: fix master mode set bit error ASoC: rockchip-i2s: add dma data to snd_soc_dai ASoC: rockchip-i2s: fix registers' property of rockchip i2s controller ASoC: rockchip-i2s: enable "hclk" for rockchip I2S controller
sound/soc/rockchip/Kconfig | 3 +-- sound/soc/rockchip/Makefile | 2 +- sound/soc/rockchip/rockchip_i2s.c | 41 ++++++++++++++++++++++++--------------- 3 files changed, 27 insertions(+), 19 deletions(-)
Fix SND_ROCKCHIP_I2S to be more reasonable - SND_SOC_ROCKCHIP_I2S, SND_SOC_ROCKCHIP_I2S should select by audio driver, instead of SND_SOC_ROCKCHIP.
Signed-off-by: Jianqun Xu jay.xu@rock-chips.com --- sound/soc/rockchip/Kconfig | 3 +-- sound/soc/rockchip/Makefile | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/sound/soc/rockchip/Kconfig b/sound/soc/rockchip/Kconfig index c196a46..78fc159 100644 --- a/sound/soc/rockchip/Kconfig +++ b/sound/soc/rockchip/Kconfig @@ -2,11 +2,10 @@ config SND_SOC_ROCKCHIP tristate "ASoC support for Rockchip" depends on COMPILE_TEST || ARCH_ROCKCHIP select SND_SOC_GENERIC_DMAENGINE_PCM - select SND_ROCKCHIP_I2S help Say Y or M if you want to add support for codecs attached to the Rockchip SoCs' Audio interfaces. You will also need to select the audio interfaces to support below.
-config SND_ROCKCHIP_I2S +config SND_SOC_ROCKCHIP_I2S tristate diff --git a/sound/soc/rockchip/Makefile b/sound/soc/rockchip/Makefile index 1006418..b921909 100644 --- a/sound/soc/rockchip/Makefile +++ b/sound/soc/rockchip/Makefile @@ -1,4 +1,4 @@ # ROCKCHIP Platform Support snd-soc-i2s-objs := rockchip_i2s.o
-obj-$(CONFIG_SND_ROCKCHIP_I2S) += snd-soc-i2s.o +obj-$(CONFIG_SND_SOC_ROCKCHIP_I2S) += snd-soc-i2s.o
On Sat, Sep 13, 2014 at 08:40:19AM +0800, Jianqun wrote:
Fix SND_ROCKCHIP_I2S to be more reasonable - SND_SOC_ROCKCHIP_I2S, SND_SOC_ROCKCHIP_I2S should select by audio driver, instead of SND_SOC_ROCKCHIP.
Applied, thanks.
Fix error format set to I2S master or slave mode. Test on RK3288 board with max98090.
Signed-off-by: Jianqun Xu jay.xu@rock-chips.com --- sound/soc/rockchip/rockchip_i2s.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c index 8d8e4b5..870a664 100644 --- a/sound/soc/rockchip/rockchip_i2s.c +++ b/sound/soc/rockchip/rockchip_i2s.c @@ -165,13 +165,14 @@ static int rockchip_i2s_set_fmt(struct snd_soc_dai *cpu_dai, struct rk_i2s_dev *i2s = to_info(cpu_dai); unsigned int mask = 0, val = 0;
- mask = I2S_CKR_MSS_SLAVE; + mask = I2S_CKR_MSS_MASK; switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { case SND_SOC_DAIFMT_CBS_CFS: - val = I2S_CKR_MSS_SLAVE; + /* Set source clock in Master mode */ + val = I2S_CKR_MSS_MASTER; break; case SND_SOC_DAIFMT_CBM_CFM: - val = I2S_CKR_MSS_MASTER; + val = I2S_CKR_MSS_SLAVE; break; default: return -EINVAL;
On Sat, Sep 13, 2014 at 08:41:03AM +0800, Jianqun wrote:
Fix error format set to I2S master or slave mode. Test on RK3288 board with max98090.
Applied. Since this is a bug fix it should be one of the first patches in the series so that it can be sent to Linus as a fix, bug fixes should go before any other patches.
在 09/14/2014 12:35 AM, Mark Brown 写道:
On Sat, Sep 13, 2014 at 08:41:03AM +0800, Jianqun wrote:
Fix error format set to I2S master or slave mode. Test on RK3288 board with max98090.
Applied. Since this is a bug fix it should be one of the first patches in the series so that it can be sent to Linus as a fix, bug fixes should go before any other patches.
got it, I'll do it at new version patch, thanks
Add playback/capture dma data to snd_soc_dai. Test on RK3288 with max98090.
Signed-off-by: Jianqun Xu jay.xu@rock-chips.com --- sound/soc/rockchip/rockchip_i2s.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c index 870a664..1b9b404 100644 --- a/sound/soc/rockchip/rockchip_i2s.c +++ b/sound/soc/rockchip/rockchip_i2s.c @@ -244,16 +244,6 @@ static int rockchip_i2s_hw_params(struct snd_pcm_substream *substream, regmap_update_bits(i2s->regmap, I2S_TXCR, I2S_TXCR_VDW_MASK, val); regmap_update_bits(i2s->regmap, I2S_RXCR, I2S_RXCR_VDW_MASK, val);
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { - dai->playback_dma_data = &i2s->playback_dma_data; - regmap_update_bits(i2s->regmap, I2S_DMACR, I2S_DMACR_TDL_MASK, - I2S_DMACR_TDL(1) | I2S_DMACR_TDE_ENABLE); - } else { - dai->capture_dma_data = &i2s->capture_dma_data; - regmap_update_bits(i2s->regmap, I2S_DMACR, I2S_DMACR_RDL_MASK, - I2S_DMACR_RDL(1) | I2S_DMACR_RDE_ENABLE); - } - return 0; }
@@ -301,6 +291,16 @@ static int rockchip_i2s_set_sysclk(struct snd_soc_dai *cpu_dai, int clk_id, return ret; }
+static int rockchip_i2s_dai_probe(struct snd_soc_dai *dai) +{ + struct rk_i2s_dev *i2s = snd_soc_dai_get_drvdata(dai); + + dai->capture_dma_data = &i2s->capture_dma_data; + dai->playback_dma_data = &i2s->playback_dma_data; + + return 0; +} + static const struct snd_soc_dai_ops rockchip_i2s_dai_ops = { .hw_params = rockchip_i2s_hw_params, .set_sysclk = rockchip_i2s_set_sysclk, @@ -309,7 +309,9 @@ static const struct snd_soc_dai_ops rockchip_i2s_dai_ops = { };
static struct snd_soc_dai_driver rockchip_i2s_dai = { + .probe = rockchip_i2s_dai_probe, .playback = { + .stream_name = "Playback", .channels_min = 2, .channels_max = 8, .rates = SNDRV_PCM_RATE_8000_192000, @@ -319,6 +321,7 @@ static struct snd_soc_dai_driver rockchip_i2s_dai = { SNDRV_PCM_FMTBIT_S24_LE), }, .capture = { + .stream_name = "Capture", .channels_min = 2, .channels_max = 2, .rates = SNDRV_PCM_RATE_8000_192000,
Reference rockchip I2S controller TRM, modify some registers' property I2S_FIFOLR: read / write, but not volatile, not precious I2S_INTSR: read / write I2S_CLR: volatile, register value will be cleared by read
Test on RK3288 with max98090.
Signed-off-by: Jianqun Xu jay.xu@rock-chips.com --- sound/soc/rockchip/rockchip_i2s.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c index 1b9b404..6595383 100644 --- a/sound/soc/rockchip/rockchip_i2s.c +++ b/sound/soc/rockchip/rockchip_i2s.c @@ -365,6 +365,8 @@ static bool rockchip_i2s_rd_reg(struct device *dev, unsigned int reg) case I2S_XFER: case I2S_CLR: case I2S_RXDR: + case I2S_FIFOLR: + case I2S_INTSR: return true; default: return false; @@ -374,8 +376,8 @@ static bool rockchip_i2s_rd_reg(struct device *dev, unsigned int reg) static bool rockchip_i2s_volatile_reg(struct device *dev, unsigned int reg) { switch (reg) { - case I2S_FIFOLR: case I2S_INTSR: + case I2S_CLR: return true; default: return false; @@ -385,8 +387,6 @@ static bool rockchip_i2s_volatile_reg(struct device *dev, unsigned int reg) static bool rockchip_i2s_precious_reg(struct device *dev, unsigned int reg) { switch (reg) { - case I2S_FIFOLR: - return true; default: return false; }
On Sat, Sep 13, 2014 at 08:42:12AM +0800, Jianqun wrote:
Reference rockchip I2S controller TRM, modify some registers' property I2S_FIFOLR: read / write, but not volatile, not precious I2S_INTSR: read / write I2S_CLR: volatile, register value will be cleared by read
Applied, again this is a bug fix (volatile and precious being wrong seem like bugs) so should have been earlier in the series.
在 09/14/2014 12:36 AM, Mark Brown 写道:
On Sat, Sep 13, 2014 at 08:42:12AM +0800, Jianqun wrote:
Reference rockchip I2S controller TRM, modify some registers' property I2S_FIFOLR: read / write, but not volatile, not precious I2S_INTSR: read / write I2S_CLR: volatile, register value will be cleared by read
Applied, again this is a bug fix (volatile and precious being wrong seem like bugs) so should have been earlier in the series.
ok, I'll re-order the patches
Hello.
On 9/13/2014 3:42 AM, Jianqun wrote:
Reference rockchip I2S controller TRM, modify some registers' property I2S_FIFOLR: read / write, but not volatile, not precious I2S_INTSR: read / write I2S_CLR: volatile, register value will be cleared by read
Test on RK3288 with max98090.
Signed-off-by: Jianqun Xu jay.xu@rock-chips.com
sound/soc/rockchip/rockchip_i2s.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c index 1b9b404..6595383 100644 --- a/sound/soc/rockchip/rockchip_i2s.c +++ b/sound/soc/rockchip/rockchip_i2s.c
[...]
@@ -385,8 +387,6 @@ static bool rockchip_i2s_volatile_reg(struct device *dev, unsigned int reg) static bool rockchip_i2s_precious_reg(struct device *dev, unsigned int reg) { switch (reg) {
- case I2S_FIFOLR:
return true;
Shouldn't this be folded into simple *return* false now?
WBR, Sergei
在 09/14/2014 04:57 AM, Sergei Shtylyov 写道:
Hello.
On 9/13/2014 3:42 AM, Jianqun wrote:
Reference rockchip I2S controller TRM, modify some registers' property I2S_FIFOLR: read / write, but not volatile, not precious I2S_INTSR: read / write I2S_CLR: volatile, register value will be cleared by read
Test on RK3288 with max98090.
Signed-off-by: Jianqun Xu jay.xu@rock-chips.com
sound/soc/rockchip/rockchip_i2s.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c index 1b9b404..6595383 100644 --- a/sound/soc/rockchip/rockchip_i2s.c +++ b/sound/soc/rockchip/rockchip_i2s.c
[...]
@@ -385,8 +387,6 @@ static bool rockchip_i2s_volatile_reg(struct device *dev, unsigned int reg) static bool rockchip_i2s_precious_reg(struct device *dev, unsigned int reg) { switch (reg) {
- case I2S_FIFOLR:
return true; default: return false; }Shouldn't this be folded into simple *return* false now?
That is more reasonable, thank you.
WBR, Sergei
Hello.
On 09/14/2014 06:29 AM, Jianqun wrote:
Reference rockchip I2S controller TRM, modify some registers' property I2S_FIFOLR: read / write, but not volatile, not precious I2S_INTSR: read / write I2S_CLR: volatile, register value will be cleared by read
Test on RK3288 with max98090.
Signed-off-by: Jianqun Xu jay.xu@rock-chips.com
sound/soc/rockchip/rockchip_i2s.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c index 1b9b404..6595383 100644 --- a/sound/soc/rockchip/rockchip_i2s.c +++ b/sound/soc/rockchip/rockchip_i2s.c
[...]
@@ -385,8 +387,6 @@ static bool rockchip_i2s_volatile_reg(struct device *dev, unsigned int reg) static bool rockchip_i2s_precious_reg(struct device *dev, unsigned int reg) { switch (reg) {
- case I2S_FIFOLR:
return true; default: return false; }
Shouldn't this be folded into simple *return* false now?
That is more reasonable, thank you.
Moreover, this function may be completely eliminated.
WBR, Sergei
As "hclk" is used for rockchip I2S controller, driver must to enable it in probe.
Tested on RK3288 with max98090.
Signed-off-by: Jianqun Xu jay.xu@rock-chips.com --- sound/soc/rockchip/rockchip_i2s.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c index 6595383..033487c 100644 --- a/sound/soc/rockchip/rockchip_i2s.c +++ b/sound/soc/rockchip/rockchip_i2s.c @@ -423,6 +423,11 @@ static int rockchip_i2s_probe(struct platform_device *pdev) dev_err(&pdev->dev, "Can't retrieve i2s bus clock\n"); return PTR_ERR(i2s->hclk); } + ret = clk_prepare_enable(i2s->hclk); + if (ret) { + dev_err(i2s->dev, "hclock enable failed %d\n", ret); + return ret; + }
i2s->mclk = devm_clk_get(&pdev->dev, "i2s_clk"); if (IS_ERR(i2s->mclk)) {
On Sat, Sep 13, 2014 at 08:43:13AM +0800, Jianqun wrote:
As "hclk" is used for rockchip I2S controller, driver must to enable it in probe.
Applied, again this is a bug fix. How did the original submission get tested?
在 09/14/2014 12:36 AM, Mark Brown 写道:
On Sat, Sep 13, 2014 at 08:43:13AM +0800, Jianqun wrote:
As "hclk" is used for rockchip I2S controller, driver must to enable it in probe.
Applied, again this is a bug fix. How did the original submission get tested?
The original submission is verified on rk3288 with kernel 3.10, but I had to make patches based on kernel 3.14 +, also our sdk kernel has intalized the kernel in other ways, so I missed the enable but the driver worked well.
The new patches is verified on kernel 3.14, so it will easy to test.
On Sat, Sep 13, 2014 at 08:43:13AM +0800, Jianqun wrote:
+++ b/sound/soc/rockchip/rockchip_i2s.c @@ -423,6 +423,11 @@ static int rockchip_i2s_probe(struct platform_device *pdev) dev_err(&pdev->dev, "Can't retrieve i2s bus clock\n"); return PTR_ERR(i2s->hclk); }
- ret = clk_prepare_enable(i2s->hclk);
- if (ret) {
dev_err(i2s->dev, "hclock enable failed %d\n", ret);return ret;- }
BTW: you're also missing a clk_disable_unprepare() in the remove path here, please send a followup fixing that.
在 09/14/2014 12:37 AM, Mark Brown 写道:
On Sat, Sep 13, 2014 at 08:43:13AM +0800, Jianqun wrote:
+++ b/sound/soc/rockchip/rockchip_i2s.c @@ -423,6 +423,11 @@ static int rockchip_i2s_probe(struct platform_device *pdev) dev_err(&pdev->dev, "Can't retrieve i2s bus clock\n"); return PTR_ERR(i2s->hclk); }
- ret = clk_prepare_enable(i2s->hclk);
- if (ret) {
dev_err(i2s->dev, "hclock enable failed %d\n", ret);return ret;- }
BTW: you're also missing a clk_disable_unprepare() in the remove path here, please send a followup fixing that.
remove function has done the clk_disable_unprepare for "hclk".
One more thing, since "i2s_clk" is enabled at runtime_resume, and is disabled in runtime_suspend, hasn't enable in probe, so do the "i2s_clk" to be disabled in remove ? The current driver has disable in remove.
On Sun, Sep 14, 2014 at 10:27:43AM +0800, Jianqun wrote:
在 09/14/2014 12:37 AM, Mark Brown 写道:
- ret = clk_prepare_enable(i2s->hclk);
- if (ret) {
dev_err(i2s->dev, "hclock enable failed %d\n", ret);return ret;- }
BTW: you're also missing a clk_disable_unprepare() in the remove path here, please send a followup fixing that.
remove function has done the clk_disable_unprepare for "hclk".
One more thing, since "i2s_clk" is enabled at runtime_resume, and is disabled in runtime_suspend, hasn't enable in probe, so do the "i2s_clk" to be disabled in remove ? The current driver has disable in remove.
Again, please try to write clear changelogs which describe what the goal of the patch is and cover obvious questions like this which a reviewer might ask.
This is all extremely unclear - you're adding an enable here with no matching disable. It seems that what you saying that there was previously a bug where the clock was disabled without being enabled? I had to look at the code to work that out...
participants (4)
-
Jianqun -
Jianqun -
Mark Brown -
Sergei Shtylyov