[alsa-devel] [PATCH 00/10] ASoC: twl6040: Gain ramp code cleanups
Hello,
the following series cleans up the gain ramp code found in the twl6040 codec driver. Main changes: - use one workqueue for the twl6040 codec driver (instead of the original 3) - Delays between the steps does not need to be different among the range. I assume, that the original code contained copy-paste snippets from wm8350 for this part - Cleanups for the DAPM_OUT_DRV_E event handler code. - delayed_works moved within their corresponding struct.
The series has been generated on top of: git://opensource.wolfsonmicro.com/linux-2.6-asoc, for-3.2 branch + ASoC: omap-mcpdm/twl6040: Offset cancellation series.
Side note: I have done this to better understand (while cleaning up the twl6040 driver) the requirements for the ramp code, and to study the possibility of adding support in the core for this (to handle the wm8350, and twl6040 in a generic way later). I'm still looking at the optimal implementation without ending up with too complicated code/structures...
Regards, Peter --- Peter Ujfalusi (10): ASoC: twl6040: Rename pga_event to out_drv_event ASoC: twl6040: Combine the custom volsw get, and put functions ASoC: twl6040: Move delayed_work struct inside twl6040_output for HS/HF ASoC: twl6040: Move the delayed_work for HS detection under twl6040_jack_data ASoC: twl6040: One workqueue should be enough ASoC: twl6040: correct loop counters for HS/HF ramp code ASoC: twl6040: No need to change delay during HS ramp ASoC: twl6040: No need to change delay during HF ramp ASoC: twl6040: Shift 2 identifies the HS output in out_drv_event ASoC: twl6040: Simplify code in out_drv_event for pending work check
sound/soc/codecs/twl6040.c | 203 ++++++++++++------------------------------- 1 files changed, 57 insertions(+), 146 deletions(-)
This event handler is used with the OUT_DRV widgets. The name pga_event was misleading.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/codecs/twl6040.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/sound/soc/codecs/twl6040.c b/sound/soc/codecs/twl6040.c index d5c4bb4..b63ee24 100644 --- a/sound/soc/codecs/twl6040.c +++ b/sound/soc/codecs/twl6040.c @@ -72,7 +72,6 @@ struct twl6040_output { u16 right_step; unsigned int step_delay; u16 ramp; - u16 mute; struct completion ramp_done; };
@@ -573,7 +572,7 @@ static void twl6040_pga_hf_work(struct work_struct *work) handsfree->ramp = TWL6040_RAMP_NONE; }
-static int pga_event(struct snd_soc_dapm_widget *w, +static int out_drv_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol, int event) { struct snd_soc_codec *codec = w->codec; @@ -1248,19 +1247,19 @@ static const struct snd_soc_dapm_widget twl6040_dapm_widgets[] = { /* Analog playback drivers */ SND_SOC_DAPM_OUT_DRV_E("HF Left Driver", TWL6040_REG_HFLCTL, 4, 0, NULL, 0, - pga_event, + out_drv_event, SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD), SND_SOC_DAPM_OUT_DRV_E("HF Right Driver", TWL6040_REG_HFRCTL, 4, 0, NULL, 0, - pga_event, + out_drv_event, SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD), SND_SOC_DAPM_OUT_DRV_E("HS Left Driver", TWL6040_REG_HSLCTL, 2, 0, NULL, 0, - pga_event, + out_drv_event, SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD), SND_SOC_DAPM_OUT_DRV_E("HS Right Driver", TWL6040_REG_HSRCTL, 2, 0, NULL, 0, - pga_event, + out_drv_event, SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD), SND_SOC_DAPM_OUT_DRV_E("Earphone Driver", TWL6040_REG_EARCTL, 0, 0, NULL, 0,
On Mon, Sep 26, 2011 at 04:26:24PM +0300, Peter Ujfalusi wrote:
This event handler is used with the OUT_DRV widgets. The name pga_event was misleading.
Applied, though
index d5c4bb4..b63ee24 100644 --- a/sound/soc/codecs/twl6040.c +++ b/sound/soc/codecs/twl6040.c @@ -72,7 +72,6 @@ struct twl6040_output { u16 right_step; unsigned int step_delay; u16 ramp;
- u16 mute; struct completion ramp_done;
};
This appears to be a random unrelated change.
On Monday 26 September 2011 22:19:57 Mark Brown wrote:
On Mon, Sep 26, 2011 at 04:26:24PM +0300, Peter Ujfalusi wrote:
This event handler is used with the OUT_DRV widgets. The name pga_event was misleading.
Applied, though
index d5c4bb4..b63ee24 100644 --- a/sound/soc/codecs/twl6040.c +++ b/sound/soc/codecs/twl6040.c @@ -72,7 +72,6 @@ struct twl6040_output {
u16 right_step; unsigned int step_delay; u16 ramp;
u16 mute;
struct completion ramp_done;
};
This appears to be a random unrelated change.
Argh, this supposed to be a separate commit, it got squashed accidentally, sorry.
-- Péter
We can manage with one set of get, and put function for the gain controls we need to handle with custom code due to the shadowing of the register. For both get, and put function we can call decide based on the mc->rreg value, if we need to call the volsw, or the vlosw_2r variant (in 2r case rreg is not 0). Handling of the shadow values are the same for both type of controls.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/codecs/twl6040.c | 81 +++++++++---------------------------------- 1 files changed, 17 insertions(+), 64 deletions(-)
diff --git a/sound/soc/codecs/twl6040.c b/sound/soc/codecs/twl6040.c index b63ee24..0fd8719 100644 --- a/sound/soc/codecs/twl6040.c +++ b/sound/soc/codecs/twl6040.c @@ -763,15 +763,17 @@ static int twl6040_put_volsw(struct snd_kcontrol *kcontrol, struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; int ret; - unsigned int reg = mc->reg;
/* For HS and HF we shadow the values and only actually write * them out when active in order to ensure the amplifier comes on * as quietly as possible. */ - switch (reg) { + switch (mc->reg) { case TWL6040_REG_HSGAIN: out = &twl6040_priv->headset; break; + case TWL6040_REG_HFLGAIN: + out = &twl6040_priv->handsfree; + break; default: break; } @@ -783,7 +785,12 @@ static int twl6040_put_volsw(struct snd_kcontrol *kcontrol, return 1; }
- ret = snd_soc_put_volsw(kcontrol, ucontrol); + /* call the appropriate handler depending on the rreg */ + if (mc->rreg) + ret = snd_soc_put_volsw_2r(kcontrol, ucontrol); + else + ret = snd_soc_put_volsw(kcontrol, ucontrol); + if (ret < 0) return ret;
@@ -798,39 +805,12 @@ static int twl6040_get_volsw(struct snd_kcontrol *kcontrol, struct twl6040_output *out = &twl6040_priv->headset; struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; - unsigned int reg = mc->reg;
- switch (reg) { + switch (mc->reg) { case TWL6040_REG_HSGAIN: out = &twl6040_priv->headset; - ucontrol->value.integer.value[0] = out->left_vol; - ucontrol->value.integer.value[1] = out->right_vol; - return 0; - - default: break; - } - - return snd_soc_get_volsw(kcontrol, ucontrol); -} - -static int twl6040_put_volsw_2r_vu(struct snd_kcontrol *kcontrol, - struct snd_ctl_elem_value *ucontrol) -{ - struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); - struct twl6040_data *twl6040_priv = snd_soc_codec_get_drvdata(codec); - struct twl6040_output *out = NULL; - struct soc_mixer_control *mc = - (struct soc_mixer_control *)kcontrol->private_value; - int ret; - unsigned int reg = mc->reg; - - /* For HS and HF we shadow the values and only actually write - * them out when active in order to ensure the amplifier comes on - * as quietly as possible. */ - switch (reg) { case TWL6040_REG_HFLGAIN: - case TWL6040_REG_HFRGAIN: out = &twl6040_priv->handsfree; break; default: @@ -838,43 +818,16 @@ static int twl6040_put_volsw_2r_vu(struct snd_kcontrol *kcontrol, }
if (out) { - out->left_vol = ucontrol->value.integer.value[0]; - out->right_vol = ucontrol->value.integer.value[1]; - if (!out->active) - return 1; - } - - ret = snd_soc_put_volsw_2r(kcontrol, ucontrol); - if (ret < 0) - return ret; - - return 1; -} - -static int twl6040_get_volsw_2r(struct snd_kcontrol *kcontrol, - struct snd_ctl_elem_value *ucontrol) -{ - struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); - struct twl6040_data *twl6040_priv = snd_soc_codec_get_drvdata(codec); - struct twl6040_output *out = &twl6040_priv->handsfree; - struct soc_mixer_control *mc = - (struct soc_mixer_control *)kcontrol->private_value; - unsigned int reg = mc->reg; - - /* If these are cached registers use the cache */ - switch (reg) { - case TWL6040_REG_HFLGAIN: - case TWL6040_REG_HFRGAIN: - out = &twl6040_priv->handsfree; ucontrol->value.integer.value[0] = out->left_vol; ucontrol->value.integer.value[1] = out->right_vol; return 0; - - default: - break; }
- return snd_soc_get_volsw_2r(kcontrol, ucontrol); + /* call the appropriate handler depending on the rreg */ + if (mc->rreg) + return snd_soc_get_volsw_2r(kcontrol, ucontrol); + else + return snd_soc_get_volsw(kcontrol, ucontrol); }
/* double control with volume update */ @@ -899,7 +852,7 @@ static int twl6040_get_volsw_2r(struct snd_kcontrol *kcontrol, SNDRV_CTL_ELEM_ACCESS_VOLATILE, \ .tlv.p = (tlv_array), \ .info = snd_soc_info_volsw_2r, \ - .get = twl6040_get_volsw_2r, .put = twl6040_put_volsw_2r_vu, \ + .get = twl6040_get_volsw, .put = twl6040_put_volsw, \ .private_value = (unsigned long)&(struct soc_mixer_control) \ {.reg = reg_left, .rreg = reg_right, .shift = xshift, \ .rshift = xshift, .max = xmax, .invert = xinvert}, }
On Mon, Sep 26, 2011 at 04:26:25PM +0300, Peter Ujfalusi wrote:
Applied, thanks.
- ret = snd_soc_put_volsw(kcontrol, ucontrol);
- /* call the appropriate handler depending on the rreg */
- if (mc->rreg)
ret = snd_soc_put_volsw_2r(kcontrol, ucontrol);- else
ret = snd_soc_put_volsw(kcontrol, ucontrol);
Traditionally this would be done by comparing reg and rreg - if they're the same they're a mono control.
On Monday 26 September 2011 22:21:42 Mark Brown wrote:
On Mon, Sep 26, 2011 at 04:26:25PM +0300, Peter Ujfalusi wrote:
Applied, thanks.
- ret = snd_soc_put_volsw(kcontrol, ucontrol);
- /* call the appropriate handler depending on the rreg */
- if (mc->rreg)
ret = snd_soc_put_volsw_2r(kcontrol, ucontrol);- else
ret = snd_soc_put_volsw(kcontrol, ucontrol);Traditionally this would be done by comparing reg and rreg - if they're the same they're a mono control.
I'm not looking for the mono/stereo, but looking for the gain value(s) are in the same register, but in different offset VS gain values are at the same offset, but in two different registers. The mono/stereo handling within snd_soc_put_volsw sufficient, and at the moment the twl6040 has only stereo controls handled this way, but if we have stereo, it is not a big job to implement.
The macros for SOC_DOUBLE, SOC_DOUBLE_TLV only assignes the .reg, leaving the .rreg to be zero. SOC_DOUBLE_R, SOC_DOUBLE_R_TLV on the other hand assigning the .reg, and the .rreg.
-- Péter
On Tue, Sep 27, 2011 at 09:16:16AM +0300, Péter Ujfalusi wrote:
On Monday 26 September 2011 22:21:42 Mark Brown wrote:
Traditionally this would be done by comparing reg and rreg - if they're the same they're a mono control.
I'm not looking for the mono/stereo, but looking for the gain value(s) are in the same register, but in different offset VS gain values are at the same offset, but in two different registers.
That's not what you're actually checking :) This would generally be checked by comparing the shift registers - the basic reason we have the two functions at all is that we used to mash everything into a 32 bit int rather than using a pointer to struct.
On Tuesday 27 September 2011 11:56:26 Mark Brown wrote:
On Tue, Sep 27, 2011 at 09:16:16AM +0300, Péter Ujfalusi wrote:
On Monday 26 September 2011 22:21:42 Mark Brown wrote:
Traditionally this would be done by comparing reg and rreg - if they're the same they're a mono control.
I'm not looking for the mono/stereo, but looking for the gain value(s) are in the same register, but in different offset VS gain values are at the same offset, but in two different registers.
That's not what you're actually checking :) This would generally be checked by comparing the shift registers - the basic reason we have the two functions at all is that we used to mash everything into a 32 bit int rather than using a pointer to struct.
I see what you mean, but.. The thing I'm after here is to select between the snd_soc_put_volsw, and snd_soc_put_volsw_2r to make the change in the HW.
In case of SOC_SINGLE_TLV I (will) need to call snd_soc_put_volsw In case of SOC_DOUBLE_TLV I need to call snd_soc_put_volsw In case of SOC_DOUBLE_R_TLV I need to call snd_soc_put_volsw_2r.
SOC_SINGLE_TLV: reg = xreg, rreg = 0, shift = xshift, rshift = xshift,
SOC_DOUBLE_TLV: reg = xreg, rreg = 0, shift = left_shift, rshift = right_shift,
SOC_DOUBLE_R_TLV: reg = left_reg, rreg = right_reg, shift = xshift, rshift = xshift,
To pick the correct snd_soc_put_* call it is easier to check if the rreg is 0, since in that case I need to use the snd_soc_put_volsw (and snd_soc_put_volsw_2r, when we have two registers to configure).
-- Péter
On Tue, Sep 27, 2011 at 02:21:37PM +0300, Péter Ujfalusi wrote:
In case of SOC_SINGLE_TLV I (will) need to call snd_soc_put_volsw In case of SOC_DOUBLE_TLV I need to call snd_soc_put_volsw In case of SOC_DOUBLE_R_TLV I need to call snd_soc_put_volsw_2r.
Or we could just update the macros and unify all the operations which might be easier overall?
On Tuesday 27 September 2011 12:42:44 Mark Brown wrote:
On Tue, Sep 27, 2011 at 02:21:37PM +0300, Péter Ujfalusi wrote:
In case of SOC_SINGLE_TLV I (will) need to call snd_soc_put_volsw In case of SOC_DOUBLE_TLV I need to call snd_soc_put_volsw In case of SOC_DOUBLE_R_TLV I need to call snd_soc_put_volsw_2r.
Or we could just update the macros and unify all the operations which might be easier overall?
If we can save few LOC, it might worth doing it. I can take a look at this, but I think it is going to be 3.3 material.
-- Péter
The delayed works for the output can be moved within the twl6040_output struct (from the twl6040_data) to be better organized.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/codecs/twl6040.c | 15 +++++++-------- 1 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/sound/soc/codecs/twl6040.c b/sound/soc/codecs/twl6040.c index 0fd8719..c5232ce 100644 --- a/sound/soc/codecs/twl6040.c +++ b/sound/soc/codecs/twl6040.c @@ -72,6 +72,7 @@ struct twl6040_output { u16 right_step; unsigned int step_delay; u16 ramp; + struct delayed_work work; struct completion ramp_done; };
@@ -104,8 +105,6 @@ struct twl6040_data { struct twl6040_output handsfree; struct workqueue_struct *hf_workqueue; struct workqueue_struct *hs_workqueue; - struct delayed_work hs_delayed_work; - struct delayed_work hf_delayed_work; };
/* @@ -489,7 +488,7 @@ static inline int twl6040_hf_ramp_step(struct snd_soc_codec *codec, static void twl6040_pga_hs_work(struct work_struct *work) { struct twl6040_data *priv = - container_of(work, struct twl6040_data, hs_delayed_work.work); + container_of(work, struct twl6040_data, headset.work.work); struct snd_soc_codec *codec = priv->codec; struct twl6040_output *headset = &priv->headset; unsigned int delay = headset->step_delay; @@ -532,7 +531,7 @@ static void twl6040_pga_hs_work(struct work_struct *work) static void twl6040_pga_hf_work(struct work_struct *work) { struct twl6040_data *priv = - container_of(work, struct twl6040_data, hf_delayed_work.work); + container_of(work, struct twl6040_data, handsfree.work.work); struct snd_soc_codec *codec = priv->codec; struct twl6040_output *handsfree = &priv->handsfree; unsigned int delay = handsfree->step_delay; @@ -585,7 +584,6 @@ static int out_drv_event(struct snd_soc_dapm_widget *w, case 2: case 3: out = &priv->headset; - work = &priv->hs_delayed_work; queue = priv->hs_workqueue; out->left_step = priv->hs_left_step; out->right_step = priv->hs_right_step; @@ -593,7 +591,6 @@ static int out_drv_event(struct snd_soc_dapm_widget *w, break; case 4: out = &priv->handsfree; - work = &priv->hf_delayed_work; queue = priv->hf_workqueue; out->left_step = priv->hf_left_step; out->right_step = priv->hf_right_step; @@ -607,6 +604,8 @@ static int out_drv_event(struct snd_soc_dapm_widget *w, return -1; }
+ work = &out->work; + switch (event) { case SND_SOC_DAPM_POST_PMU: if (out->active) @@ -1625,8 +1624,8 @@ static int twl6040_probe(struct snd_soc_codec *codec) goto hswq_err; }
- INIT_DELAYED_WORK(&priv->hs_delayed_work, twl6040_pga_hs_work); - INIT_DELAYED_WORK(&priv->hf_delayed_work, twl6040_pga_hf_work); + INIT_DELAYED_WORK(&priv->headset.work, twl6040_pga_hs_work); + INIT_DELAYED_WORK(&priv->handsfree.work, twl6040_pga_hf_work);
ret = request_threaded_irq(priv->plug_irq, NULL, twl6040_audio_handler, 0, "twl6040_irq_plug", codec);
On Mon, Sep 26, 2011 at 04:26:26PM +0300, Peter Ujfalusi wrote:
The delayed works for the output can be moved within the twl6040_output struct (from the twl6040_data) to be better organized.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com
Applied, thanks.
The delayed_work named 'delayed_work' is for the headset detection, so move it to the twl6040_jack_data struct.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/codecs/twl6040.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/twl6040.c b/sound/soc/codecs/twl6040.c index c5232ce..72f838c 100644 --- a/sound/soc/codecs/twl6040.c +++ b/sound/soc/codecs/twl6040.c @@ -78,6 +78,7 @@ struct twl6040_output {
struct twl6040_jack_data { struct snd_soc_jack *jack; + struct delayed_work work; int report; };
@@ -99,7 +100,6 @@ struct twl6040_data { struct twl6040_jack_data hs_jack; struct snd_soc_codec *codec; struct workqueue_struct *workqueue; - struct delayed_work delayed_work; struct mutex mutex; struct twl6040_output headset; struct twl6040_output handsfree; @@ -734,7 +734,7 @@ EXPORT_SYMBOL_GPL(twl6040_hs_jack_detect); static void twl6040_accessory_work(struct work_struct *work) { struct twl6040_data *priv = container_of(work, - struct twl6040_data, delayed_work.work); + struct twl6040_data, hs_jack.work.work); struct snd_soc_codec *codec = priv->codec; struct twl6040_jack_data *hs_jack = &priv->hs_jack;
@@ -747,7 +747,7 @@ static irqreturn_t twl6040_audio_handler(int irq, void *data) struct snd_soc_codec *codec = data; struct twl6040_data *priv = snd_soc_codec_get_drvdata(codec);
- queue_delayed_work(priv->workqueue, &priv->delayed_work, + queue_delayed_work(priv->workqueue, &priv->hs_jack.work, msecs_to_jiffies(200));
return IRQ_HANDLED; @@ -1606,7 +1606,7 @@ static int twl6040_probe(struct snd_soc_codec *codec) goto work_err; }
- INIT_DELAYED_WORK(&priv->delayed_work, twl6040_accessory_work); + INIT_DELAYED_WORK(&priv->hs_jack.work, twl6040_accessory_work);
mutex_init(&priv->mutex);
On Mon, Sep 26, 2011 at 04:26:27PM +0300, Peter Ujfalusi wrote:
The delayed_work named 'delayed_work' is for the headset detection, so move it to the twl6040_jack_data struct.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com
Applied, thanks.
It is a bit overkill to have three (3) separate workqueue for a single driver. We can manage things with one workqueue nicely.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/codecs/twl6040.c | 33 +++++---------------------------- 1 files changed, 5 insertions(+), 28 deletions(-)
diff --git a/sound/soc/codecs/twl6040.c b/sound/soc/codecs/twl6040.c index 72f838c..156ebf7 100644 --- a/sound/soc/codecs/twl6040.c +++ b/sound/soc/codecs/twl6040.c @@ -103,8 +103,6 @@ struct twl6040_data { struct mutex mutex; struct twl6040_output headset; struct twl6040_output handsfree; - struct workqueue_struct *hf_workqueue; - struct workqueue_struct *hs_workqueue; };
/* @@ -578,20 +576,17 @@ static int out_drv_event(struct snd_soc_dapm_widget *w, struct twl6040_data *priv = snd_soc_codec_get_drvdata(codec); struct twl6040_output *out; struct delayed_work *work; - struct workqueue_struct *queue;
switch (w->shift) { case 2: case 3: out = &priv->headset; - queue = priv->hs_workqueue; out->left_step = priv->hs_left_step; out->right_step = priv->hs_right_step; out->step_delay = 5; /* 5 ms between volume ramp steps */ break; case 4: out = &priv->handsfree; - queue = priv->hf_workqueue; out->left_step = priv->hf_left_step; out->right_step = priv->hf_right_step; out->step_delay = 5; /* 5 ms between volume ramp steps */ @@ -617,7 +612,7 @@ static int out_drv_event(struct snd_soc_dapm_widget *w,
if (!delayed_work_pending(work)) { out->ramp = TWL6040_RAMP_UP; - queue_delayed_work(queue, work, + queue_delayed_work(priv->workqueue, work, msecs_to_jiffies(1)); } break; @@ -631,7 +626,7 @@ static int out_drv_event(struct snd_soc_dapm_widget *w, out->ramp = TWL6040_RAMP_DOWN; INIT_COMPLETION(out->ramp_done);
- queue_delayed_work(queue, work, + queue_delayed_work(priv->workqueue, work, msecs_to_jiffies(1));
wait_for_completion_timeout(&out->ramp_done, @@ -1600,33 +1595,21 @@ static int twl6040_probe(struct snd_soc_codec *codec) goto work_err; }
- priv->workqueue = create_singlethread_workqueue("twl6040-codec"); + priv->workqueue = alloc_workqueue("twl6040-codec", 0, 0); if (!priv->workqueue) { ret = -ENOMEM; goto work_err; }
INIT_DELAYED_WORK(&priv->hs_jack.work, twl6040_accessory_work); + INIT_DELAYED_WORK(&priv->headset.work, twl6040_pga_hs_work); + INIT_DELAYED_WORK(&priv->handsfree.work, twl6040_pga_hf_work);
mutex_init(&priv->mutex);
init_completion(&priv->headset.ramp_done); init_completion(&priv->handsfree.ramp_done);
- priv->hf_workqueue = create_singlethread_workqueue("twl6040-hf"); - if (priv->hf_workqueue == NULL) { - ret = -ENOMEM; - goto hfwq_err; - } - priv->hs_workqueue = create_singlethread_workqueue("twl6040-hs"); - if (priv->hs_workqueue == NULL) { - ret = -ENOMEM; - goto hswq_err; - } - - INIT_DELAYED_WORK(&priv->headset.work, twl6040_pga_hs_work); - INIT_DELAYED_WORK(&priv->handsfree.work, twl6040_pga_hf_work); - ret = request_threaded_irq(priv->plug_irq, NULL, twl6040_audio_handler, 0, "twl6040_irq_plug", codec); if (ret) { @@ -1650,10 +1633,6 @@ static int twl6040_probe(struct snd_soc_codec *codec) bias_err: free_irq(priv->plug_irq, codec); plugirq_err: - destroy_workqueue(priv->hs_workqueue); -hswq_err: - destroy_workqueue(priv->hf_workqueue); -hfwq_err: destroy_workqueue(priv->workqueue); work_err: kfree(priv); @@ -1667,8 +1646,6 @@ static int twl6040_remove(struct snd_soc_codec *codec) twl6040_set_bias_level(codec, SND_SOC_BIAS_OFF); free_irq(priv->plug_irq, codec); destroy_workqueue(priv->workqueue); - destroy_workqueue(priv->hf_workqueue); - destroy_workqueue(priv->hs_workqueue); kfree(priv);
return 0;
On Mon, Sep 26, 2011 at 04:26:28PM +0300, Peter Ujfalusi wrote:
It is a bit overkill to have three (3) separate workqueue for a single driver. We can manage things with one workqueue nicely.
Do we even need specific workqueues at all or can we use the system workqueue? Looks OK though.
Hi Mark,
On Mon, Sep 26, 2011 at 5:41 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
Do we even need specific workqueues at all or can we use the system workqueue? Looks OK though.
For the jack detection we might be OK with the system workqueue, if we got delayed by few tens of ms it does not hurt that much (well the handover from earpiece/IHF to headset might take longer). For the ramp code execution we should avoid any delay at all costs, since it can have audible side effects. In my past experience the system wq can cause latency, since we usually have slow devices on it (GPS, proximity sensor, magnetometer, etc). Reading from those usually takes considerable amount of time, and we can end up with delayed execution of our ramp code.
As of it now, I would keep the separate wq, and revisit later after I can get tests running on a 'full' device.
On Mon, Sep 26, 2011 at 08:20:11PM +0300, Ujfalusi, Peter wrote:
For the ramp code execution we should avoid any delay at all costs, since it can have audible side effects.
If you're that sensitive to latency does a workqueue offer sufficient guarantees?
In my past experience the system wq can cause latency, since we usually have slow devices on it (GPS, proximity sensor, magnetometer, etc). Reading from those usually takes considerable amount of time, and we can end up with delayed execution of our ramp code.
How long are these work items taking? I'm wondering if the other work items are taking sufficiently long to be actively disruptive if they should be isolated from other things rather than the other way around. Of course it could also be a large buildup of work...
On Monday 26 September 2011 22:29:15 Mark Brown wrote:
On Mon, Sep 26, 2011 at 08:20:11PM +0300, Ujfalusi, Peter wrote:
For the ramp code execution we should avoid any delay at all costs, since it can have audible side effects.
If you're that sensitive to latency does a workqueue offer sufficient guarantees?
It seams to be pretty good based on my tests.
In my past experience the system wq can cause latency, since we usually have slow devices on it (GPS, proximity sensor, magnetometer, etc). Reading from those usually takes considerable amount of time, and we can end up with delayed execution of our ramp code.
How long are these work items taking? I'm wondering if the other work items are taking sufficiently long to be actively disruptive if they should be isolated from other things rather than the other way around. Of course it could also be a large buildup of work...
It could be any random driver (we can have random components on a device). This is just precaution to protect the audio from nasty things.
-- Péter
4 bit resolution means 16 steps -> for loop till >=15 (HS) 5 bit resolution means 32 steps -> for loop till >=31 (HF)
In case of the Handsfree we have 0x1e, 0x1f as invalid values. In case of HF we can limit the loop for 0x1d (29).
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/codecs/twl6040.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/twl6040.c b/sound/soc/codecs/twl6040.c index 156ebf7..5a33fe1 100644 --- a/sound/soc/codecs/twl6040.c +++ b/sound/soc/codecs/twl6040.c @@ -497,7 +497,7 @@ static void twl6040_pga_hs_work(struct work_struct *work) return;
/* HS PGA volumes have 4 bits of resolution to ramp */ - for (i = 0; i <= 16; i++) { + for (i = 0; i <= 15; i++) { headset_complete = twl6040_hs_ramp_step(codec, headset->left_step, headset->right_step); @@ -539,8 +539,11 @@ static void twl6040_pga_hf_work(struct work_struct *work) if (handsfree->ramp == TWL6040_RAMP_NONE) return;
- /* HF PGA volumes have 5 bits of resolution to ramp */ - for (i = 0; i <= 32; i++) { + /* + * HF PGA volumes have 5 bits of resolution to ramp, but 0x1e, 0x1f are + * invalid values, loop till 0x1d + */ + for (i = 0; i <= 29; i++) { handsfree_complete = twl6040_hf_ramp_step(codec, handsfree->left_step, handsfree->right_step);
On Mon, Sep 26, 2011 at 04:26:29PM +0300, Peter Ujfalusi wrote:
- for (i = 0; i <= 16; i++) {
- for (i = 0; i <= 15; i++) {
It'd seem more natural to change these to being simple < comparisons - they're far more common. Any great reason for going tis way (perhaps context?).
On Monday 26 September 2011 22:33:01 Mark Brown wrote:
On Mon, Sep 26, 2011 at 04:26:29PM +0300, Peter Ujfalusi wrote:
- for (i = 0; i <= 16; i++) {
- for (i = 0; i <= 15; i++) {
It'd seem more natural to change these to being simple < comparisons - they're far more common. Any great reason for going tis way (perhaps context?).
No particular reason IMHO, just a copy from the wm8350 driver. I'll check how they look like in assembly, it could be that <= is better in ARM?
-- Péter
On Tue, Sep 27, 2011 at 09:21:14AM +0300, Péter Ujfalusi wrote:
On Monday 26 September 2011 22:33:01 Mark Brown wrote:
On Mon, Sep 26, 2011 at 04:26:29PM +0300, Peter Ujfalusi wrote:
- for (i = 0; i <= 16; i++) {
- for (i = 0; i <= 15; i++) {
It'd seem more natural to change these to being simple < comparisons - they're far more common. Any great reason for going tis way (perhaps context?).
No particular reason IMHO, just a copy from the wm8350 driver. I'll check how they look like in assembly, it could be that <= is better in ARM?
It's not like it's getting used throughout the rest of the code and that's a trivial optimization for a compiler...
The Headset gain have 2dB steps all the way, so there is no reason to have different delays as we approaching to the end of the scale. The comment was also wrong, since we have 0dB at 0x0 raw at one end of the range, and not in the middle.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/codecs/twl6040.c | 12 ++---------- 1 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/sound/soc/codecs/twl6040.c b/sound/soc/codecs/twl6040.c index 5a33fe1..659a235 100644 --- a/sound/soc/codecs/twl6040.c +++ b/sound/soc/codecs/twl6040.c @@ -489,7 +489,6 @@ static void twl6040_pga_hs_work(struct work_struct *work) container_of(work, struct twl6040_data, headset.work.work); struct snd_soc_codec *codec = priv->codec; struct twl6040_output *headset = &priv->headset; - unsigned int delay = headset->step_delay; int i, headset_complete;
/* do we need to ramp at all ? */ @@ -506,15 +505,8 @@ static void twl6040_pga_hs_work(struct work_struct *work) if (headset_complete) break;
- /* - * TODO: tune: delay is longer over 0dB - * as increases are larger. - */ - if (i >= 8) - schedule_timeout_interruptible(msecs_to_jiffies(delay + - (delay >> 1))); - else - schedule_timeout_interruptible(msecs_to_jiffies(delay)); + schedule_timeout_interruptible( + msecs_to_jiffies(headset->step_delay)); }
if (headset->ramp == TWL6040_RAMP_DOWN) {
On Mon, Sep 26, 2011 at 04:26:30PM +0300, Peter Ujfalusi wrote:
The Headset gain have 2dB steps all the way, so there is no reason to have different delays as we approaching to the end of the scale. The comment was also wrong, since we have 0dB at 0x0 raw at one end of the range, and not in the middle.
Applied, thanks.
The Handsfree gain have 2dB steps all the way, so there is no reason to have different delays as we approaching to the end of the scale. The comment was also wrong, since we have 0dB at 0x3 raw, at 16 the gain is -26dB.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/codecs/twl6040.c | 12 ++---------- 1 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/sound/soc/codecs/twl6040.c b/sound/soc/codecs/twl6040.c index 659a235..580cb1c 100644 --- a/sound/soc/codecs/twl6040.c +++ b/sound/soc/codecs/twl6040.c @@ -524,7 +524,6 @@ static void twl6040_pga_hf_work(struct work_struct *work) container_of(work, struct twl6040_data, handsfree.work.work); struct snd_soc_codec *codec = priv->codec; struct twl6040_output *handsfree = &priv->handsfree; - unsigned int delay = handsfree->step_delay; int i, handsfree_complete;
/* do we need to ramp at all ? */ @@ -544,15 +543,8 @@ static void twl6040_pga_hf_work(struct work_struct *work) if (handsfree_complete) break;
- /* - * TODO: tune: delay is longer over 0dB - * as increases are larger. - */ - if (i >= 16) - schedule_timeout_interruptible(msecs_to_jiffies(delay + - (delay >> 1))); - else - schedule_timeout_interruptible(msecs_to_jiffies(delay)); + schedule_timeout_interruptible( + msecs_to_jiffies(handsfree->step_delay)); }
On Mon, Sep 26, 2011 at 04:26:31PM +0300, Peter Ujfalusi wrote:
The Handsfree gain have 2dB steps all the way, so there is no reason to have different delays as we approaching to the end of the scale. The comment was also wrong, since we have 0dB at 0x3 raw, at 16 the gain is -26dB.
Applied, thanks.
None of the driver handled by out_drv_event have it's power bit shifted by 3. Remove the case for shift 3, and also add comment for the cases.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/codecs/twl6040.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/twl6040.c b/sound/soc/codecs/twl6040.c index 580cb1c..7ad209f 100644 --- a/sound/soc/codecs/twl6040.c +++ b/sound/soc/codecs/twl6040.c @@ -565,14 +565,13 @@ static int out_drv_event(struct snd_soc_dapm_widget *w, struct delayed_work *work;
switch (w->shift) { - case 2: - case 3: + case 2: /* Headset output driver */ out = &priv->headset; out->left_step = priv->hs_left_step; out->right_step = priv->hs_right_step; out->step_delay = 5; /* 5 ms between volume ramp steps */ break; - case 4: + case 4: /* Handsfree output driver */ out = &priv->handsfree; out->left_step = priv->hf_left_step; out->right_step = priv->hf_right_step;
Check for pending work before the DAPM event switch case, and return, if the work is pending. This way we can remove the two additional checks in POST_PMU, and PRE_PMD for non pending works.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/codecs/twl6040.c | 25 +++++++++++-------------- 1 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/sound/soc/codecs/twl6040.c b/sound/soc/codecs/twl6040.c index 7ad209f..23506d5 100644 --- a/sound/soc/codecs/twl6040.c +++ b/sound/soc/codecs/twl6040.c @@ -587,37 +587,34 @@ static int out_drv_event(struct snd_soc_dapm_widget *w,
work = &out->work;
+ if (delayed_work_pending(work)) + return 0; + switch (event) { case SND_SOC_DAPM_POST_PMU: if (out->active) break;
/* don't use volume ramp for power-up */ + out->ramp = TWL6040_RAMP_UP; out->left_step = out->left_vol; out->right_step = out->right_vol;
- if (!delayed_work_pending(work)) { - out->ramp = TWL6040_RAMP_UP; - queue_delayed_work(priv->workqueue, work, - msecs_to_jiffies(1)); - } + queue_delayed_work(priv->workqueue, work, msecs_to_jiffies(1)); break;
case SND_SOC_DAPM_PRE_PMD: if (!out->active) break;
- if (!delayed_work_pending(work)) { - /* use volume ramp for power-down */ - out->ramp = TWL6040_RAMP_DOWN; - INIT_COMPLETION(out->ramp_done); + /* use volume ramp for power-down */ + out->ramp = TWL6040_RAMP_DOWN; + INIT_COMPLETION(out->ramp_done);
- queue_delayed_work(priv->workqueue, work, - msecs_to_jiffies(1)); + queue_delayed_work(priv->workqueue, work, msecs_to_jiffies(1));
- wait_for_completion_timeout(&out->ramp_done, - msecs_to_jiffies(2000)); - } + wait_for_completion_timeout(&out->ramp_done, + msecs_to_jiffies(2000)); break; }
On Mon, Sep 26, 2011 at 04:26:33PM +0300, Peter Ujfalusi wrote:
Check for pending work before the DAPM event switch case, and return, if the work is pending. This way we can remove the two additional checks in POST_PMU, and PRE_PMD for non pending works.
Would it not be easier to just not check at all? It looks like you're just deciding if you should schedule the work but schedule_delayed_work ought to handle rescheduling of an already scheduled work item fine.
Hi Mark,
On Mon, Sep 26, 2011 at 5:30 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
Would it not be easier to just not check at all? It looks like you're just deciding if you should schedule the work but schedule_delayed_work ought to handle rescheduling of an already scheduled work item fine.
Yeah, we might not need the check. I think we have other problems here with race conditions... The best thing to do is to call cancel_delayed_work_sync prior we do any modification within the twl6040_output structure. After the cancel_delayed_work_sync, we can be sure that no work pending, and none is executing at the moment, so we can modify the ramp configuration, and schedule the work to do the right thing.
Thanks for bringing this to my attention, I knew something was not right, but I have missed this. I'll correct this in the v2 series. BTW: the same issue applies to the wm8350 driver, I think.
participants (3)
-
Mark Brown -
Peter Ujfalusi -
Ujfalusi, Peter