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

Message ID 1410568993-21874-1-git-send-email-jay.xu@rock-chips.com
State New, archived
Headers show

Commit Message

Jianqun Xu Sept. 13, 2014, 12:43 a.m. UTC
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(+)

Comments

Mark Brown Sept. 13, 2014, 4:36 p.m. UTC | #1
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?
Mark Brown Sept. 13, 2014, 4:37 p.m. UTC | #2
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.
jianqun Sept. 14, 2014, 2:24 a.m. UTC | #3
? 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.
jianqun Sept. 14, 2014, 2:27 a.m. UTC | #4
? 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.
>
Mark Brown Sept. 15, 2014, 4:54 p.m. UTC | #5
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...

Patch
diff mbox

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