[PATCH 00/10] ASoC: twl6040: Gain ramp code cleanups

[PATCH 00/10] ASoC: twl6040: Gain ramp code cleanups

[Peter Ujfalusi]

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

-- 
1.7.6.1

[PATCH 01/10] ASoC: twl6040: Rename pga_event to out_drv_event

[Peter Ujfalusi]

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,
-- 
1.7.6.1

Re: [PATCH 01/10] ASoC: twl6040: Rename pga_event to out_drv_event

[Mark Brown]

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.

Re: [PATCH 01/10] ASoC: twl6040: Rename pga_event to out_drv_event

[Péter Ujfalusi]

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

[PATCH 02/10] ASoC: twl6040: Combine the custom volsw get, and put functions

[Peter Ujfalusi]

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}, }
-- 
1.7.6.1

Re: [PATCH 02/10] ASoC: twl6040: Combine the custom volsw get, and put functions

[Mark Brown]

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.

Re: [PATCH 02/10] ASoC: twl6040: Combine the custom volsw get, and put functions

[Péter Ujfalusi]

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

Re: [PATCH 02/10] ASoC: twl6040: Combine the custom volsw get, and put functions

[Mark Brown]

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.

Re: [PATCH 02/10] ASoC: twl6040: Combine the custom volsw get, and put functions

[Péter Ujfalusi]

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

Re: [PATCH 02/10] ASoC: twl6040: Combine the custom volsw get, and put functions

[Mark Brown]

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?

Re: [PATCH 02/10] ASoC: twl6040: Combine the custom volsw get, and put functions

[Péter Ujfalusi]

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

[PATCH 03/10] ASoC: twl6040: Move delayed_work struct inside twl6040_output for HS/HF

[Peter Ujfalusi]

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);
-- 
1.7.6.1

Re: [PATCH 03/10] ASoC: twl6040: Move delayed_work struct inside twl6040_output for HS/HF

[Mark Brown]

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.

[PATCH 04/10] ASoC: twl6040: Move the delayed_work for HS detection under twl6040_jack_data

[Peter Ujfalusi]

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);
 
-- 
1.7.6.1

Re: [PATCH 04/10] ASoC: twl6040: Move the delayed_work for HS detection under twl6040_jack_data

[Mark Brown]

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.

[PATCH 05/10] ASoC: twl6040: One workqueue should be enough

[Peter Ujfalusi]

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;
-- 
1.7.6.1

Re: [PATCH 05/10] ASoC: twl6040: One workqueue should be enough

[Mark Brown]

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.

Re: [PATCH 05/10] ASoC: twl6040: One workqueue should be enough

[Ujfalusi, Peter]

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.

-- 
Péter

Re: [PATCH 05/10] ASoC: twl6040: One workqueue should be enough

[Mark Brown]

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...

Re: [PATCH 05/10] ASoC: twl6040: One workqueue should be enough

[Péter Ujfalusi]

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

[PATCH 06/10] ASoC: twl6040: correct loop counters for HS/HF ramp code

[Peter Ujfalusi]

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);
-- 
1.7.6.1

Re: [PATCH 06/10] ASoC: twl6040: correct loop counters for HS/HF ramp code

[Mark Brown]

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?).

Re: [PATCH 06/10] ASoC: twl6040: correct loop counters for HS/HF ramp code

[Péter Ujfalusi]

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

Re: [PATCH 06/10] ASoC: twl6040: correct loop counters for HS/HF ramp code

[Mark Brown]

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...

[PATCH 07/10] ASoC: twl6040: No need to change delay during HS ramp

[Peter Ujfalusi]

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

Re: [PATCH 07/10] ASoC: twl6040: No need to change delay during HS ramp

[Mark Brown]

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.

[PATCH 08/10] ASoC: twl6040: No need to change delay during HF ramp

[Peter Ujfalusi]

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));
 	}
 
 
-- 
1.7.6.1

Re: [PATCH 08/10] ASoC: twl6040: No need to change delay during HF ramp

[Mark Brown]

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.

[PATCH 09/10] ASoC: twl6040: Shift 2 identifies the HS output in out_drv_event

[Peter Ujfalusi]

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;
-- 
1.7.6.1

[PATCH 10/10] ASoC: twl6040: Simplify code in out_drv_event for pending work check

[Peter Ujfalusi]

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;
 	}
 
-- 
1.7.6.1

Re: [PATCH 10/10] ASoC: twl6040: Simplify code in out_drv_event for pending work check

[Mark Brown]

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.

Re: [PATCH 10/10] ASoC: twl6040: Simplify code in out_drv_event for pending work check

[Ujfalusi, Peter]

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.

-- 
Péter