[PATCH 1/2] ASoC: Refector DAPM event handler

[PATCH 1/2] ASoC: Refector DAPM event handler

[Mark Brown]

The DAPM event callback code has many layers of indentation. Refector
the code to give less indentation in order to facilitiate further
refactoring.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/soc/soc-dapm.c |   76 +++++++++++++++++++++++++-------------------------
 1 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 2c87061..9307b10 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -587,44 +587,44 @@ static int dapm_power_widgets(struct snd_soc_codec *codec, int event)
 			w->power = power;
 
 			/* call any power change event handlers */
-			if (power_change) {
-				if (w->event) {
-					pr_debug("power %s event for %s flags %x\n",
-						 w->power ? "on" : "off", w->name, w->event_flags);
-					if (power) {
-						/* power up event */
-						if (w->event_flags & SND_SOC_DAPM_PRE_PMU) {
-							ret = w->event(w,
-								NULL, SND_SOC_DAPM_PRE_PMU);
-							if (ret < 0)
-								return ret;
-						}
-						dapm_update_bits(w);
-						if (w->event_flags & SND_SOC_DAPM_POST_PMU){
-							ret = w->event(w,
-								NULL, SND_SOC_DAPM_POST_PMU);
-							if (ret < 0)
-								return ret;
-						}
-					} else {
-						/* power down event */
-						if (w->event_flags & SND_SOC_DAPM_PRE_PMD) {
-							ret = w->event(w,
-								NULL, SND_SOC_DAPM_PRE_PMD);
-							if (ret < 0)
-								return ret;
-						}
-						dapm_update_bits(w);
-						if (w->event_flags & SND_SOC_DAPM_POST_PMD) {
-							ret = w->event(w,
-								NULL, SND_SOC_DAPM_POST_PMD);
-							if (ret < 0)
-								return ret;
-						}
-					}
-				} else
-					/* no event handler */
-					dapm_update_bits(w);
+			if (power_change && w->event)
+				pr_debug("power %s event for %s flags %x\n",
+					 w->power ? "on" : "off",
+					 w->name, w->event_flags);
+
+			/* power up pre event */
+			if (power_change && power && w->event &&
+			    w->event_flags & SND_SOC_DAPM_PRE_PMU) {
+				ret = w->event(w, NULL, SND_SOC_DAPM_PRE_PMU);
+				if (ret < 0)
+					return ret;
+			}
+
+			/* power down pre event */
+			if (power_change && !power && w->event &&
+			    w->event_flags & SND_SOC_DAPM_PRE_PMD) {
+				ret = w->event(w, NULL, SND_SOC_DAPM_PRE_PMD);
+				if (ret < 0)
+					return ret;
+			}
+
+			dapm_update_bits(w);
+
+			/* power up post event */
+			if (power_change && power && w->event &&
+			    w->event_flags & SND_SOC_DAPM_POST_PMU){
+				ret = w->event(w,
+					       NULL, SND_SOC_DAPM_POST_PMU);
+				if (ret < 0)
+					return ret;
+			}
+
+			/* power down post event */
+			if (power_change && !power && w->event &&
+			    w->event_flags & SND_SOC_DAPM_POST_PMD) {
+				ret = w->event(w, NULL, SND_SOC_DAPM_POST_PMD);
+				if (ret < 0)
+					return ret;
 			}
 		}
 	}
-- 
1.5.6.2

[PATCH 2/2] ASoC: Factor PGA DAPM handling into main

[Mark Brown]

This allows pre and post event hooks to be provided for PGA widgets.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/soc/soc-dapm.c |   26 ++++++++------------------
 1 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 9307b10..54908f6 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -523,24 +523,6 @@ static int dapm_power_widgets(struct snd_soc_codec *codec, int event)
 				continue;
 			}
 
-			/* programmable gain/attenuation */
-			if (w->id == snd_soc_dapm_pga) {
-				int on;
-				in = is_connected_input_ep(w);
-				dapm_clear_walk(w->codec);
-				out = is_connected_output_ep(w);
-				dapm_clear_walk(w->codec);
-				w->power = on = (out != 0 && in != 0) ? 1 : 0;
-
-				if (!on)
-					dapm_set_pga(w, on); /* lower volume to reduce pops */
-				dapm_update_bits(w);
-				if (on)
-					dapm_set_pga(w, on); /* restore volume from zero */
-
-				continue;
-			}
-
 			/* pre and post event widgets */
 			if (w->id == snd_soc_dapm_pre) {
 				if (!w->event)
@@ -608,8 +590,16 @@ static int dapm_power_widgets(struct snd_soc_codec *codec, int event)
 					return ret;
 			}
 
+			/* Lower PGA volume to reduce pops */
+			if (w->id == snd_soc_dapm_pga && !power)
+				dapm_set_pga(w, power);
+
 			dapm_update_bits(w);
 
+			/* Raise PGA volume to reduce pops */
+			if (w->id == snd_soc_dapm_pga && power)
+				dapm_set_pga(w, power);
+
 			/* power up post event */
 			if (power_change && power && w->event &&
 			    w->event_flags & SND_SOC_DAPM_POST_PMU){
-- 
1.5.6.2

Re: [PATCH 1/2] ASoC: Refector DAPM event handler

[Takashi Iwai]

At Thu, 17 Jul 2008 11:59:35 +0100,
Mark Brown wrote:
> 
> The DAPM event callback code has many layers of indentation. Refector

Refactor?

> the code to give less indentation in order to facilitiate further
> refactoring.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  sound/soc/soc-dapm.c |   76 +++++++++++++++++++++++++-------------------------
>  1 files changed, 38 insertions(+), 38 deletions(-)
> 
> diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
> index 2c87061..9307b10 100644
> --- a/sound/soc/soc-dapm.c
> +++ b/sound/soc/soc-dapm.c
> @@ -587,44 +587,44 @@ static int dapm_power_widgets(struct snd_soc_codec *codec, int event)
>  			w->power = power;
>  
>  			/* call any power change event handlers */
> -			if (power_change) {
> -				if (w->event) {
> -					pr_debug("power %s event for %s flags %x\n",
> -						 w->power ? "on" : "off", w->name, w->event_flags);
> -					if (power) {
> -						/* power up event */
> -						if (w->event_flags & SND_SOC_DAPM_PRE_PMU) {
> -							ret = w->event(w,
> -								NULL, SND_SOC_DAPM_PRE_PMU);
> -							if (ret < 0)
> -								return ret;
> -						}
> -						dapm_update_bits(w);
> -						if (w->event_flags & SND_SOC_DAPM_POST_PMU){
> -							ret = w->event(w,
> -								NULL, SND_SOC_DAPM_POST_PMU);
> -							if (ret < 0)
> -								return ret;
> -						}
> -					} else {
> -						/* power down event */
> -						if (w->event_flags & SND_SOC_DAPM_PRE_PMD) {
> -							ret = w->event(w,
> -								NULL, SND_SOC_DAPM_PRE_PMD);
> -							if (ret < 0)
> -								return ret;
> -						}
> -						dapm_update_bits(w);
> -						if (w->event_flags & SND_SOC_DAPM_POST_PMD) {
> -							ret = w->event(w,
> -								NULL, SND_SOC_DAPM_POST_PMD);
> -							if (ret < 0)
> -								return ret;
> -						}
> -					}
> -				} else
> -					/* no event handler */
> -					dapm_update_bits(w);
> +			if (power_change && w->event)
> +				pr_debug("power %s event for %s flags %x\n",
> +					 w->power ? "on" : "off",
> +					 w->name, w->event_flags);
> +
> +			/* power up pre event */
> +			if (power_change && power && w->event &&
> +			    w->event_flags & SND_SOC_DAPM_PRE_PMU) {
> +				ret = w->event(w, NULL, SND_SOC_DAPM_PRE_PMU);
> +				if (ret < 0)
> +					return ret;
> +			}
> +
> +			/* power down pre event */
> +			if (power_change && !power && w->event &&
> +			    w->event_flags & SND_SOC_DAPM_PRE_PMD) {
> +				ret = w->event(w, NULL, SND_SOC_DAPM_PRE_PMD);
> +				if (ret < 0)
> +					return ret;
> +			}
> +
> +			dapm_update_bits(w);
> +
> +			/* power up post event */
> +			if (power_change && power && w->event &&
> +			    w->event_flags & SND_SOC_DAPM_POST_PMU){
> +				ret = w->event(w,
> +					       NULL, SND_SOC_DAPM_POST_PMU);
> +				if (ret < 0)
> +					return ret;
> +			}
> +
> +			/* power down post event */
> +			if (power_change && !power && w->event &&
> +			    w->event_flags & SND_SOC_DAPM_POST_PMD) {
> +				ret = w->event(w, NULL, SND_SOC_DAPM_POST_PMD);
> +				if (ret < 0)
> +					return ret;

With this patch, dapm_update_bits() is called even when
power_change=0, so it changes the behavior.  You can check
power_change only once and skip the rest via continue if it's false.

Also, better to put parentheses around bit-and operation.

Anyway, if the indent level really matters, you should better to put
it as a function.


thanks,

Takashi

Re: [PATCH 1/2] ASoC: Refector DAPM event handler

[Mark Brown]

On Thu, Jul 17, 2008 at 02:45:19PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > The DAPM event callback code has many layers of indentation. Refector

> Refactor?

Rewrite it into something functionally equivalent but has fewer layers
of indentation.

Re: [PATCH 1/2] ASoC: Refector DAPM event handler

[Mark Brown]

On Thu, Jul 17, 2008 at 02:45:19PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > The DAPM event callback code has many layers of indentation. Refector

> Refactor?

Oh, I see - yes, that's misspelt.

Re: [PATCH 1/2] ASoC: Refector DAPM event handler

[Takashi Iwai]

At Thu, 17 Jul 2008 14:07:33 +0100,
Mark Brown wrote:
> 
> On Thu, Jul 17, 2008 at 02:45:19PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > The DAPM event callback code has many layers of indentation. Refector
> 
> > Refactor?
> 
> Oh, I see - yes, that's misspelt.

:)

BTW, don't miss my comment in the end of my previous post, too.


thanks,

Takashi

Re: [PATCH 1/2] ASoC: Refector DAPM event handler

[Mark Brown]

On Thu, Jul 17, 2008 at 03:17:12PM +0200, Takashi Iwai wrote:

> BTW, don't miss my comment in the end of my previous post, too.

Oh, didn't see that at all.  Too many people not snipping any context
recently, I guess.

Re: [PATCH 1/2] ASoC: Refector DAPM event handler

[Mark Brown]

On Thu, Jul 17, 2008 at 02:45:19PM +0200, Takashi Iwai wrote:

> With this patch, dapm_update_bits() is called even when
> power_change=0, so it changes the behavior.  You can check
> power_change only once and skip the rest via continue if it's false.

Fixed - dapm_update_bits() already suppresses duplicate writes.

> Also, better to put parentheses around bit-and operation.

Fixed.  As with some other things it might be worth adding checks you
want like this to checkpatch.

> Anyway, if the indent level really matters, you should better to put
> it as a function.

The new indentation level is fine - the major issue was that the old
layout was over 80 columns already (checkpatch!), plus the way it was
structured would lead to exploding nesting levels when new cases were
added.  Splitting it out would only move it away from the handling of
other widgets which wouldn't help so much.

I'll resubmit shortly.

[PATCH 1/2] ASoC: Refector DAPM event handler

[Mark Brown]

The DAPM event callback code has many layers of indentation, taking it
over 80 columns. Refactor the code to give less indentation in order to
avoid checkpatch issues on further changes and exploding indentation.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/soc/soc-dapm.c |   79 ++++++++++++++++++++++++++------------------------
 1 files changed, 41 insertions(+), 38 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 2c87061..17698ef 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -586,45 +586,48 @@ static int dapm_power_widgets(struct snd_soc_codec *codec, int event)
 			power_change = (w->power == power) ? 0: 1;
 			w->power = power;
 
+			if (!power_change)
+				continue;
+
 			/* call any power change event handlers */
-			if (power_change) {
-				if (w->event) {
-					pr_debug("power %s event for %s flags %x\n",
-						 w->power ? "on" : "off", w->name, w->event_flags);
-					if (power) {
-						/* power up event */
-						if (w->event_flags & SND_SOC_DAPM_PRE_PMU) {
-							ret = w->event(w,
-								NULL, SND_SOC_DAPM_PRE_PMU);
-							if (ret < 0)
-								return ret;
-						}
-						dapm_update_bits(w);
-						if (w->event_flags & SND_SOC_DAPM_POST_PMU){
-							ret = w->event(w,
-								NULL, SND_SOC_DAPM_POST_PMU);
-							if (ret < 0)
-								return ret;
-						}
-					} else {
-						/* power down event */
-						if (w->event_flags & SND_SOC_DAPM_PRE_PMD) {
-							ret = w->event(w,
-								NULL, SND_SOC_DAPM_PRE_PMD);
-							if (ret < 0)
-								return ret;
-						}
-						dapm_update_bits(w);
-						if (w->event_flags & SND_SOC_DAPM_POST_PMD) {
-							ret = w->event(w,
-								NULL, SND_SOC_DAPM_POST_PMD);
-							if (ret < 0)
-								return ret;
-						}
-					}
-				} else
-					/* no event handler */
-					dapm_update_bits(w);
+			if (w->event)
+				pr_debug("power %s event for %s flags %x\n",
+					 w->power ? "on" : "off",
+					 w->name, w->event_flags);
+
+			/* power up pre event */
+			if (power && w->event &&
+			    (w->event_flags & SND_SOC_DAPM_PRE_PMU)) {
+				ret = w->event(w, NULL, SND_SOC_DAPM_PRE_PMU);
+				if (ret < 0)
+					return ret;
+			}
+
+			/* power down pre event */
+			if (!power && w->event &&
+			    (w->event_flags & SND_SOC_DAPM_PRE_PMD)) {
+				ret = w->event(w, NULL, SND_SOC_DAPM_PRE_PMD);
+				if (ret < 0)
+					return ret;
+			}
+
+			dapm_update_bits(w);
+
+			/* power up post event */
+			if (power && w->event &&
+			    (w->event_flags & SND_SOC_DAPM_POST_PMU)) {
+				ret = w->event(w,
+					       NULL, SND_SOC_DAPM_POST_PMU);
+				if (ret < 0)
+					return ret;
+			}
+
+			/* power down post event */
+			if (!power && w->event &&
+			    (w->event_flags & SND_SOC_DAPM_POST_PMD)) {
+				ret = w->event(w, NULL, SND_SOC_DAPM_POST_PMD);
+				if (ret < 0)
+					return ret;
 			}
 		}
 	}
-- 
1.5.6.2