[PATCH v3 0/4] ASoC: twl6040: Gain ramp code cleanups

alsa-devel.alsa-project.org archive mirror
 help / color / mirror / Atom feed

* [PATCH v3 0/4] ASoC: twl6040: Gain ramp code cleanups
@ 2011-09-29 12:22 Peter Ujfalusi
  2011-09-29 12:22 ` [PATCH v3 1/4] ASoC: twl6040: One workqueue should be enough Peter Ujfalusi
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Peter Ujfalusi @ 2011-09-29 12:22 UTC (permalink / raw)
  To: Mark Brown, Liam Girdwood; +Cc: alsa-devel, Misael Lopez Cruz

Hello,

Changes since v2:
- Use cancel_delayed_work_sync to make sure that we do not have pending work,
  or executing work when modifying the variables used by the ramp code.

Intro mail from v2:

Changes since v1:
- Loop counters cleaned, commented in patch 2
- Use cancel_delayed_work_sync to make sure that we do not have pending work,
  or executing work when modifying the variables used by the ramp code.
- patches already taken left out.

Intro from v1:

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 (4):
  ASoC: twl6040: One workqueue should be enough
  ASoC: twl6040: correct loop counters for HS/HF ramp code
  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 |   81 +++++++++++++++++--------------------------
 1 files changed, 32 insertions(+), 49 deletions(-)

-- 
1.7.6.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 1/4] ASoC: twl6040: One workqueue should be enough
  2011-09-29 12:22 [PATCH v3 0/4] ASoC: twl6040: Gain ramp code cleanups Peter Ujfalusi
@ 2011-09-29 12:22 ` Peter Ujfalusi
  2011-09-29 12:22 ` [PATCH v3 2/4] ASoC: twl6040: correct loop counters for HS/HF ramp code Peter Ujfalusi
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Peter Ujfalusi @ 2011-09-29 12:22 UTC (permalink / raw)
  To: Mark Brown, Liam Girdwood; +Cc: alsa-devel, Misael Lopez Cruz

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 5bf305b..91242de 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;
 };
 
 /*
@@ -562,20 +560,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 */
@@ -601,7 +596,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;
@@ -615,7 +610,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,
@@ -1584,33 +1579,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) {
@@ -1634,10 +1617,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);
@@ -1651,8 +1630,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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 2/4] ASoC: twl6040: correct loop counters for HS/HF ramp code
  2011-09-29 12:22 [PATCH v3 0/4] ASoC: twl6040: Gain ramp code cleanups Peter Ujfalusi
  2011-09-29 12:22 ` [PATCH v3 1/4] ASoC: twl6040: One workqueue should be enough Peter Ujfalusi
@ 2011-09-29 12:22 ` Peter Ujfalusi
  2011-09-29 12:22 ` [PATCH v3 3/4] ASoC: twl6040: Shift 2 identifies the HS output in out_drv_event Peter Ujfalusi
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Peter Ujfalusi @ 2011-09-29 12:22 UTC (permalink / raw)
  To: Mark Brown, Liam Girdwood; +Cc: alsa-devel, Misael Lopez Cruz

The Headset gain range is 0 - 0xf (4 bit resolution)
The Handsfree gain range is 0 - 0x1d (5 bit resolution,
0x1e, and 0x1f values are invalid)

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/codecs/twl6040.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/twl6040.c b/sound/soc/codecs/twl6040.c
index 91242de..4aa7d69 100644
--- a/sound/soc/codecs/twl6040.c
+++ b/sound/soc/codecs/twl6040.c
@@ -495,8 +495,8 @@ static void twl6040_pga_hs_work(struct work_struct *work)
 	if (headset->ramp == TWL6040_RAMP_NONE)
 		return;
 
-	/* HS PGA volumes have 4 bits of resolution to ramp */
-	for (i = 0; i <= 16; i++) {
+	/* HS PGA gain range: 0x0 - 0xf (0 - 15) */
+	for (i = 0; i < 16; i++) {
 		headset_complete = twl6040_hs_ramp_step(codec,
 						headset->left_step,
 						headset->right_step);
@@ -530,8 +530,9 @@ 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 gain range: 0x00 - 0x1d (0 - 29) */
+	for (i = 0; i < 30; i++) {
 		handsfree_complete = twl6040_hf_ramp_step(codec,
 						handsfree->left_step,
 						handsfree->right_step);
-- 
1.7.6.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 3/4] ASoC: twl6040: Shift 2 identifies the HS output in out_drv_event
  2011-09-29 12:22 [PATCH v3 0/4] ASoC: twl6040: Gain ramp code cleanups Peter Ujfalusi
  2011-09-29 12:22 ` [PATCH v3 1/4] ASoC: twl6040: One workqueue should be enough Peter Ujfalusi
  2011-09-29 12:22 ` [PATCH v3 2/4] ASoC: twl6040: correct loop counters for HS/HF ramp code Peter Ujfalusi
@ 2011-09-29 12:22 ` Peter Ujfalusi
  2011-09-29 12:22 ` [PATCH v3 4/4] ASoC: twl6040: Simplify code in out_drv_event for pending work check Peter Ujfalusi
  2011-09-30 12:55 ` [PATCH v3 0/4] ASoC: twl6040: Gain ramp code cleanups Mark Brown
  4 siblings, 0 replies; 6+ messages in thread
From: Peter Ujfalusi @ 2011-09-29 12:22 UTC (permalink / raw)
  To: Mark Brown, Liam Girdwood; +Cc: alsa-devel, Misael Lopez Cruz

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 4aa7d69..c2b5829 100644
--- a/sound/soc/codecs/twl6040.c
+++ b/sound/soc/codecs/twl6040.c
@@ -563,14 +563,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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 4/4] ASoC: twl6040: Simplify code in out_drv_event for pending work check
  2011-09-29 12:22 [PATCH v3 0/4] ASoC: twl6040: Gain ramp code cleanups Peter Ujfalusi
                   ` (2 preceding siblings ...)
  2011-09-29 12:22 ` [PATCH v3 3/4] ASoC: twl6040: Shift 2 identifies the HS output in out_drv_event Peter Ujfalusi
@ 2011-09-29 12:22 ` Peter Ujfalusi
  2011-09-30 12:55 ` [PATCH v3 0/4] ASoC: twl6040: Gain ramp code cleanups Mark Brown
  4 siblings, 0 replies; 6+ messages in thread
From: Peter Ujfalusi @ 2011-09-29 12:22 UTC (permalink / raw)
  To: Mark Brown, Liam Girdwood; +Cc: alsa-devel, Misael Lopez Cruz

Instead of checking, if the work is pending, it is safer to cancel
the pending work, or wait till the scheduled work finishes.
This way we can avoid modifying the variables used by the work
function.
Since we know that no work is pending, 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 |   38 ++++++++++++++++++++++----------------
 1 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/sound/soc/codecs/twl6040.c b/sound/soc/codecs/twl6040.c
index c2b5829..efcb28b 100644
--- a/sound/soc/codecs/twl6040.c
+++ b/sound/soc/codecs/twl6040.c
@@ -565,12 +565,26 @@ static int out_drv_event(struct snd_soc_dapm_widget *w,
 	switch (w->shift) {
 	case 2: /* Headset output driver */
 		out = &priv->headset;
+		work = &out->work;
+		/*
+		 * Make sure, that we do not mess up variables for already
+		 * executing work.
+		 */
+		cancel_delayed_work_sync(work);
+
 		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: /* Handsfree output driver */
 		out = &priv->handsfree;
+		work = &out->work;
+		/*
+		 * Make sure, that we do not mess up variables for already
+		 * executing work.
+		 */
+		cancel_delayed_work_sync(work);
+
 		out->left_step = priv->hf_left_step;
 		out->right_step = priv->hf_right_step;
 		out->step_delay = 5;	/* 5 ms between volume ramp steps */
@@ -583,39 +597,31 @@ 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)
 			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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 0/4] ASoC: twl6040: Gain ramp code cleanups
  2011-09-29 12:22 [PATCH v3 0/4] ASoC: twl6040: Gain ramp code cleanups Peter Ujfalusi
                   ` (3 preceding siblings ...)
  2011-09-29 12:22 ` [PATCH v3 4/4] ASoC: twl6040: Simplify code in out_drv_event for pending work check Peter Ujfalusi
@ 2011-09-30 12:55 ` Mark Brown
  4 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2011-09-30 12:55 UTC (permalink / raw)
  To: Peter Ujfalusi; +Cc: alsa-devel, Liam Girdwood, Misael Lopez Cruz

On Thu, Sep 29, 2011 at 03:22:33PM +0300, Peter Ujfalusi wrote:

> Changes since v2:
> - Use cancel_delayed_work_sync to make sure that we do not have pending work,
>   or executing work when modifying the variables used by the ramp code.

Applied all, thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2011-09-30 12:55 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-29 12:22 [PATCH v3 0/4] ASoC: twl6040: Gain ramp code cleanups Peter Ujfalusi
2011-09-29 12:22 ` [PATCH v3 1/4] ASoC: twl6040: One workqueue should be enough Peter Ujfalusi
2011-09-29 12:22 ` [PATCH v3 2/4] ASoC: twl6040: correct loop counters for HS/HF ramp code Peter Ujfalusi
2011-09-29 12:22 ` [PATCH v3 3/4] ASoC: twl6040: Shift 2 identifies the HS output in out_drv_event Peter Ujfalusi
2011-09-29 12:22 ` [PATCH v3 4/4] ASoC: twl6040: Simplify code in out_drv_event for pending work check Peter Ujfalusi
2011-09-30 12:55 ` [PATCH v3 0/4] ASoC: twl6040: Gain ramp code cleanups Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).