[alsa-devel] [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 --- 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);
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?
-----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.
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"?
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.
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.
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.
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().
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);
participants (2)
-
Mark Brown -
Oder Chiou