[PATCH 0/5] ASoC: rockchip-i2s: patches for rockchip i2s driver

[PATCH 0/5] ASoC: rockchip-i2s: patches for rockchip i2s driver

[Jianqun]

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(-)

-- 
1.9.1

[PATCH 1/5] ASoC: rockchip-i2s: fix rockchip i2s defination more reasonable

[Jianqun]

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
-- 
1.9.1

Re: [PATCH 1/5] ASoC: rockchip-i2s: fix rockchip i2s defination more reasonable

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 229 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 2/5] ASoC: rockchip-i2s: fix master mode set bit error

[Jianqun]

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;
-- 
1.9.1

Re: [PATCH 2/5] ASoC: rockchip-i2s: fix master mode set bit error

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 322 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/5] ASoC: rockchip-i2s: fix master mode set bit error

[Jianqun]

在 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 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 3/5] ASoC: rockchip-i2s: add dma data to snd_soc_dai

[Jianqun]

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,
-- 
1.9.1

Re: [PATCH 3/5] ASoC: rockchip-i2s: add dma data to snd_soc_dai

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 155 bytes --]

On Sat, Sep 13, 2014 at 08:41:38AM +0800, Jianqun wrote:
> Add playback/capture dma data to snd_soc_dai.
> Test on RK3288 with max98090.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 4/5] ASoC: rockchip-i2s: fix registers' property of rockchip i2s controller

[Jianqun]

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;
 	}
-- 
1.9.1

Re: [PATCH 4/5] ASoC: rockchip-i2s: fix registers' property of rockchip i2s controller

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 4/5] ASoC: rockchip-i2s: fix registers' property of rockchip i2s controller

[Jianqun]

在 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

Re: [PATCH 4/5] ASoC: rockchip-i2s: fix registers' property of rockchip i2s controller

[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?

WBR, Sergei

Re: [PATCH 4/5] ASoC: rockchip-i2s: fix registers' property of rockchip i2s controller

[Jianqun]

在 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
> 
> 
> 
> 
> 

Re: [PATCH 4/5] ASoC: rockchip-i2s: fix registers' property of rockchip i2s controller

[Sergei Shtylyov]

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

[PATCH 5/5] ASoC: rockchip-i2s: enable "hclk" for rockchip I2S controller

[Jianqun]

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)) {
-- 
1.9.1

Re: [PATCH 5/5] ASoC: rockchip-i2s: enable "hclk" for rockchip I2S controller

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 223 bytes --]

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?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 5/5] ASoC: rockchip-i2s: enable "hclk" for rockchip I2S controller

[Jianqun]

在 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.

Re: [PATCH 5/5] ASoC: rockchip-i2s: enable "hclk" for rockchip I2S controller

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 5/5] ASoC: rockchip-i2s: enable "hclk" for rockchip I2S controller

[Jianqun]

在 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.
> 

Re: [PATCH 5/5] ASoC: rockchip-i2s: enable "hclk" for rockchip I2S controller

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

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...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]