[PATCH] ASoC: rt5645: Power up the RC clock to make sure the speaker volume adjust properly

[PATCH] ASoC: rt5645: Power up the RC clock to make sure the speaker volume adjust properly

[Oder Chiou]

Signed-off-by: Oder Chiou <oder_chiou@realtek.com>
---
 sound/soc/codecs/rt5645.c | 38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
index 2813237..672fafd 100644
--- a/sound/soc/codecs/rt5645.c
+++ b/sound/soc/codecs/rt5645.c
@@ -245,7 +245,7 @@ struct rt5645_priv {
 	struct snd_soc_jack *hp_jack;
 	struct snd_soc_jack *mic_jack;
 	struct snd_soc_jack *btn_jack;
-	struct delayed_work jack_detect_work;
+	struct delayed_work jack_detect_work, rcclock_work;
 	struct regulator_bulk_data supplies[ARRAY_SIZE(rt5645_supply_names)];
 	struct rt5645_eq_param_s *eq_param;
 
@@ -565,12 +565,33 @@ static int rt5645_hweq_put(struct snd_kcontrol *kcontrol,
 	.put = rt5645_hweq_put \
 }
 
+static int rt5645_spk_put_volsw(struct snd_kcontrol *kcontrol,
+		struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct rt5645_priv *rt5645 = snd_soc_component_get_drvdata(component);
+	int ret;
+
+	cancel_delayed_work_sync(&rt5645->rcclock_work);
+
+	regmap_update_bits(rt5645->regmap, RT5645_MICBIAS,
+		RT5645_PWR_CLK25M_MASK, RT5645_PWR_CLK25M_PU);
+
+	ret = snd_soc_put_volsw(kcontrol, ucontrol);
+
+	queue_delayed_work(system_power_efficient_wq, &rt5645->rcclock_work,
+		msecs_to_jiffies(200));
+
+	return ret;
+}
+
 static const struct snd_kcontrol_new rt5645_snd_controls[] = {
 	/* Speaker Output Volume */
 	SOC_DOUBLE("Speaker Channel Switch", RT5645_SPK_VOL,
 		RT5645_VOL_L_SFT, RT5645_VOL_R_SFT, 1, 1),
-	SOC_DOUBLE_TLV("Speaker Playback Volume", RT5645_SPK_VOL,
-		RT5645_L_VOL_SFT, RT5645_R_VOL_SFT, 39, 1, out_vol_tlv),
+	SOC_DOUBLE_EXT_TLV("Speaker Playback Volume", RT5645_SPK_VOL,
+		RT5645_L_VOL_SFT, RT5645_R_VOL_SFT, 39, 1, snd_soc_get_volsw,
+		rt5645_spk_put_volsw, out_vol_tlv),
 
 	/* ClassD modulator Speaker Gain Ratio */
 	SOC_SINGLE_TLV("Speaker ClassD Playback Volume", RT5645_SPO_CLSD_RATIO,
@@ -3122,6 +3143,15 @@ static void rt5645_jack_detect_work(struct work_struct *work)
 				SND_JACK_BTN_2 | SND_JACK_BTN_3);
 }
 
+static void rt5645_rcclock_work(struct work_struct *work)
+{
+	struct rt5645_priv *rt5645 =
+		container_of(work, struct rt5645_priv, rcclock_work.work);
+
+	regmap_update_bits(rt5645->regmap, RT5645_MICBIAS,
+		RT5645_PWR_CLK25M_MASK, RT5645_PWR_CLK25M_PD);
+}
+
 static irqreturn_t rt5645_irq(int irq, void *data)
 {
 	struct rt5645_priv *rt5645 = data;
@@ -3587,6 +3617,7 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
 	}
 
 	INIT_DELAYED_WORK(&rt5645->jack_detect_work, rt5645_jack_detect_work);
+	INIT_DELAYED_WORK(&rt5645->rcclock_work, rt5645_rcclock_work);
 
 	if (rt5645->i2c->irq) {
 		ret = request_threaded_irq(rt5645->i2c->irq, NULL, rt5645_irq,
@@ -3621,6 +3652,7 @@ static int rt5645_i2c_remove(struct i2c_client *i2c)
 		free_irq(i2c->irq, rt5645);
 
 	cancel_delayed_work_sync(&rt5645->jack_detect_work);
+	cancel_delayed_work_sync(&rt5645->rcclock_work);
 
 	snd_soc_unregister_codec(&i2c->dev);
 	regulator_bulk_disable(ARRAY_SIZE(rt5645->supplies), rt5645->supplies);
-- 
1.8.1.1.439.g50a6b54

Re: [PATCH] ASoC: rt5645: Power up the RC clock to make sure the speaker volume adjust properly

[Mark Brown]

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

On Thu, Nov 05, 2015 at 07:55:51PM +0800, Oder Chiou wrote:

> +static int rt5645_spk_put_volsw(struct snd_kcontrol *kcontrol,
> +		struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct rt5645_priv *rt5645 = snd_soc_component_get_drvdata(component);
> +	int ret;
> +
> +	cancel_delayed_work_sync(&rt5645->rcclock_work);
> +
> +	regmap_update_bits(rt5645->regmap, RT5645_MICBIAS,
> +		RT5645_PWR_CLK25M_MASK, RT5645_PWR_CLK25M_PU);
> +
> +	ret = snd_soc_put_volsw(kcontrol, ucontrol);
> +
> +	queue_delayed_work(system_power_efficient_wq, &rt5645->rcclock_work,
> +		msecs_to_jiffies(200));

A more idiomatic way of doing this is to just have the
queue_delayed_work() - there's no need to cancel a work item before
requeuing it, the workqueue code will do the right thing.  Can you
please submit a followup patch cleaning that up?

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

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

Re: [PATCH] ASoC: rt5645: Power up the RC clock to make sure the speaker volume adjust properly

[Oder Chiou]

> -----Original Message-----
> > +static int rt5645_spk_put_volsw(struct snd_kcontrol *kcontrol,
> > +		struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> > +	struct rt5645_priv *rt5645 = snd_soc_component_get_drvdata(component);
> > +	int ret;
> > +
> > +	cancel_delayed_work_sync(&rt5645->rcclock_work);
> > +
> > +	regmap_update_bits(rt5645->regmap, RT5645_MICBIAS,
> > +		RT5645_PWR_CLK25M_MASK, RT5645_PWR_CLK25M_PU);
> > +
> > +	ret = snd_soc_put_volsw(kcontrol, ucontrol);
> > +
> > +	queue_delayed_work(system_power_efficient_wq, &rt5645->rcclock_work,
> > +		msecs_to_jiffies(200));
> 
> A more idiomatic way of doing this is to just have the
> queue_delayed_work() - there's no need to cancel a work item before
> requeuing it, the workqueue code will do the right thing.  Can you
> please submit a followup patch cleaning that up?
> 
Thank you for the kind advice. The "cancel_delayed_work_sync" is essential
in case of the operation of kcontrol continuously. We want to make sure the
RC clock can be powered up at least 200ms after the speaker volume is
adjusted, so we add the "cancel_delayed_work_sync" on the top of the
function and requeue it, thanks.

Re: [PATCH] ASoC: rt5645: Power up the RC clock to make sure the speaker volume adjust properly

[Mark Brown]

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

On Fri, Nov 06, 2015 at 02:52:36AM +0000, Oder Chiou wrote:

> > > +	cancel_delayed_work_sync(&rt5645->rcclock_work);

> > > +	regmap_update_bits(rt5645->regmap, RT5645_MICBIAS,
> > > +		RT5645_PWR_CLK25M_MASK, RT5645_PWR_CLK25M_PU);

> > > +	ret = snd_soc_put_volsw(kcontrol, ucontrol);

> > > +	queue_delayed_work(system_power_efficient_wq, &rt5645->rcclock_work,
> > > +		msecs_to_jiffies(200));

> > A more idiomatic way of doing this is to just have the
> > queue_delayed_work() - there's no need to cancel a work item before
> > requeuing it, the workqueue code will do the right thing.  Can you
> > please submit a followup patch cleaning that up?

> Thank you for the kind advice. The "cancel_delayed_work_sync" is essential
> in case of the operation of kcontrol continuously. We want to make sure the
> RC clock can be powered up at least 200ms after the speaker volume is
> adjusted, so we add the "cancel_delayed_work_sync" on the top of the
> function and requeue it, thanks.

What makes you claim that this is "essential in case of the operation of
kcontrol continuously"?

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

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

Re: [PATCH] ASoC: rt5645: Power up the RC clock to make sure the speaker volume adjust properly

[Oder Chiou]

> > Thank you for the kind advice. The "cancel_delayed_work_sync" is
> > essential in case of the operation of kcontrol continuously. We want
> > to make sure the RC clock can be powered up at least 200ms after the
> > speaker volume is adjusted, so we add the "cancel_delayed_work_sync"
> > on the top of the function and requeue it, thanks.
> 
> What makes you claim that this is "essential in case of the operation of kcontrol
> continuously"?
> 
Like the Chrome OS, while the user pull the volume bar up or down, the
kcontrol will be manipulated by the UI continuously and seamlessly. In this
kind of case, if the "cancel_delayed_work_sync" is removed, the
queue_delayed_work will be failed within 200ms after the previous
queue_delayed_work, and it will not be able to make sure the power of the
RC clock enabled at least 200ms, thanks.

Re: [PATCH] ASoC: rt5645: Power up the RC clock to make sure the speaker volume adjust properly

[Mark Brown]

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

On Mon, Nov 09, 2015 at 03:13:09AM +0000, Oder Chiou wrote:

> > What makes you claim that this is "essential in case of the operation of kcontrol
> > continuously"?

> Like the Chrome OS, while the user pull the volume bar up or down, the
> kcontrol will be manipulated by the UI continuously and seamlessly. In this
> kind of case, if the "cancel_delayed_work_sync" is removed, the
> queue_delayed_work will be failed within 200ms after the previous
> queue_delayed_work, and it will not be able to make sure the power of the
> RC clock enabled at least 200ms, thanks.

No, it won't.  To repeat what I said if you just schedule the delayed
work again without cancelling then the right thing will happen.

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

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

Re: [PATCH] ASoC: rt5645: Power up the RC clock to make sure the speaker volume adjust properly

[Oder Chiou]

> On Mon, Nov 09, 2015 at 03:13:09AM +0000, Oder Chiou wrote:
> 
> > > What makes you claim that this is "essential in case of the operation of kcontrol
> > > continuously"?
> 
> > Like the Chrome OS, while the user pull the volume bar up or down, the
> > kcontrol will be manipulated by the UI continuously and seamlessly. In this
> > kind of case, if the "cancel_delayed_work_sync" is removed, the
> > queue_delayed_work will be failed within 200ms after the previous
> > queue_delayed_work, and it will not be able to make sure the power of the
> > RC clock enabled at least 200ms, thanks.
> 
> No, it won't.  To repeat what I said if you just schedule the delayed
> work again without cancelling then the right thing will happen.
> 
The following logs are our test result.
===========================================================================
[  123.365789] [rt5645_spk_put_volsw]->(584) queue_delayed_work ret = 1
[  123.367204] [rt5645_spk_put_volsw]->(584) queue_delayed_work ret = 0
[  123.392307] [rt5645_spk_put_volsw]->(584) queue_delayed_work ret = 0
[  123.393542] [rt5645_spk_put_volsw]->(584) queue_delayed_work ret = 0
[  123.469445] [rt5645_spk_put_volsw]->(584) queue_delayed_work ret = 0
[  123.470581] [rt5645_spk_put_volsw]->(584) queue_delayed_work ret = 0
[  123.565417] [rt5645_rcclock_work]->(3156)
===========================================================================
If we didn't cancel the delayed work, the function will return false to
indicate the delayed work that are already on the queue. It will make the
delayed work that cannot make sure to be manipulated at least 200ms after
the last rt5645_spk_put_volsw was manipulated. In the log, we remove the
cancel_delayed_work and there is only 95ms delay that cannot meet our
requirement, thanks.

Re: [PATCH] ASoC: rt5645: Power up the RC clock to make sure the speaker volume adjust properly

[Mark Brown]

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

On Tue, Nov 10, 2015 at 03:59:52AM +0000, Oder Chiou wrote:

> If we didn't cancel the delayed work, the function will return false to
> indicate the delayed work that are already on the queue. It will make the
> delayed work that cannot make sure to be manipulated at least 200ms after
> the last rt5645_spk_put_volsw was manipulated. In the log, we remove the
> cancel_delayed_work and there is only 95ms delay that cannot meet our
> requirement, thanks.

Looks like this has been changed (which broke a bunch of other stuff),
you need to use mod_delayed_work().

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

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

Applied "ASoC: rt5645: Power up the RC clock to make sure the speaker volume adjust properly" to the asoc tree

[Mark Brown]

The patch

   ASoC: rt5645: Power up the RC clock to make sure the speaker volume adjust properly

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 7099ee85e6af56828c46255f43adb15ed47e67df Mon Sep 17 00:00:00 2001
From: Oder Chiou <oder_chiou@realtek.com>
Date: Thu, 5 Nov 2015 19:55:51 +0800
Subject: [PATCH] ASoC: rt5645: Power up the RC clock to make sure the speaker
 volume adjust properly

Signed-off-by: Oder Chiou <oder_chiou@realtek.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/rt5645.c | 38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
index 2813237..672fafd 100644
--- a/sound/soc/codecs/rt5645.c
+++ b/sound/soc/codecs/rt5645.c
@@ -245,7 +245,7 @@ struct rt5645_priv {
 	struct snd_soc_jack *hp_jack;
 	struct snd_soc_jack *mic_jack;
 	struct snd_soc_jack *btn_jack;
-	struct delayed_work jack_detect_work;
+	struct delayed_work jack_detect_work, rcclock_work;
 	struct regulator_bulk_data supplies[ARRAY_SIZE(rt5645_supply_names)];
 	struct rt5645_eq_param_s *eq_param;
 
@@ -565,12 +565,33 @@ static int rt5645_hweq_put(struct snd_kcontrol *kcontrol,
 	.put = rt5645_hweq_put \
 }
 
+static int rt5645_spk_put_volsw(struct snd_kcontrol *kcontrol,
+		struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct rt5645_priv *rt5645 = snd_soc_component_get_drvdata(component);
+	int ret;
+
+	cancel_delayed_work_sync(&rt5645->rcclock_work);
+
+	regmap_update_bits(rt5645->regmap, RT5645_MICBIAS,
+		RT5645_PWR_CLK25M_MASK, RT5645_PWR_CLK25M_PU);
+
+	ret = snd_soc_put_volsw(kcontrol, ucontrol);
+
+	queue_delayed_work(system_power_efficient_wq, &rt5645->rcclock_work,
+		msecs_to_jiffies(200));
+
+	return ret;
+}
+
 static const struct snd_kcontrol_new rt5645_snd_controls[] = {
 	/* Speaker Output Volume */
 	SOC_DOUBLE("Speaker Channel Switch", RT5645_SPK_VOL,
 		RT5645_VOL_L_SFT, RT5645_VOL_R_SFT, 1, 1),
-	SOC_DOUBLE_TLV("Speaker Playback Volume", RT5645_SPK_VOL,
-		RT5645_L_VOL_SFT, RT5645_R_VOL_SFT, 39, 1, out_vol_tlv),
+	SOC_DOUBLE_EXT_TLV("Speaker Playback Volume", RT5645_SPK_VOL,
+		RT5645_L_VOL_SFT, RT5645_R_VOL_SFT, 39, 1, snd_soc_get_volsw,
+		rt5645_spk_put_volsw, out_vol_tlv),
 
 	/* ClassD modulator Speaker Gain Ratio */
 	SOC_SINGLE_TLV("Speaker ClassD Playback Volume", RT5645_SPO_CLSD_RATIO,
@@ -3122,6 +3143,15 @@ static void rt5645_jack_detect_work(struct work_struct *work)
 				SND_JACK_BTN_2 | SND_JACK_BTN_3);
 }
 
+static void rt5645_rcclock_work(struct work_struct *work)
+{
+	struct rt5645_priv *rt5645 =
+		container_of(work, struct rt5645_priv, rcclock_work.work);
+
+	regmap_update_bits(rt5645->regmap, RT5645_MICBIAS,
+		RT5645_PWR_CLK25M_MASK, RT5645_PWR_CLK25M_PD);
+}
+
 static irqreturn_t rt5645_irq(int irq, void *data)
 {
 	struct rt5645_priv *rt5645 = data;
@@ -3587,6 +3617,7 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
 	}
 
 	INIT_DELAYED_WORK(&rt5645->jack_detect_work, rt5645_jack_detect_work);
+	INIT_DELAYED_WORK(&rt5645->rcclock_work, rt5645_rcclock_work);
 
 	if (rt5645->i2c->irq) {
 		ret = request_threaded_irq(rt5645->i2c->irq, NULL, rt5645_irq,
@@ -3621,6 +3652,7 @@ static int rt5645_i2c_remove(struct i2c_client *i2c)
 		free_irq(i2c->irq, rt5645);
 
 	cancel_delayed_work_sync(&rt5645->jack_detect_work);
+	cancel_delayed_work_sync(&rt5645->rcclock_work);
 
 	snd_soc_unregister_codec(&i2c->dev);
 	regulator_bulk_disable(ARRAY_SIZE(rt5645->supplies), rt5645->supplies);
-- 
2.6.1