| Message ID | 1446724551-27762-1-git-send-email-oder_chiou@realtek.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7099ee85e6af56828c46255f43adb15ed47e67df |
| Headers | show |
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().
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);
Signed-off-by: Oder Chiou <oder_chiou@realtek.com> --- sound/soc/codecs/rt5645.c | 38 +++++++++++++++++++++++++++++++++++--- 1 file changed, 35 insertions(+), 3 deletions(-)