[PATCH] ASoC: Remove special casing for registerless widgets

[PATCH] ASoC: Remove special casing for registerless widgets

[Mark Brown]

Since we recently explicitly set the register for registerless widgets
to no register there is no longer any need to special case power updates
for them, we can allow them to be handled with the register compression
code as other widgets are.

As this is the only remaining user of dapm_generic_apply_power() and
dapm_update_bits() also remove those functions.

Noticed-by: Lu Guanqun <guanqun.lu@intel.com>
Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---

Compile tested only currently, sending it out now as part of discussion
with Lu Guanqun.

 sound/soc/soc-dapm.c |  100 --------------------------------------------------
 1 files changed, 0 insertions(+), 100 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index b11d011..c9196d6 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -322,45 +322,6 @@ static int dapm_connect_mixer(struct snd_soc_dapm_context *dapm,
 	return -ENODEV;
 }
 
-/* update dapm codec register bits */
-static int dapm_update_bits(struct snd_soc_dapm_widget *widget)
-{
-	int change, power;
-	unsigned int old, new;
-	struct snd_soc_codec *codec = widget->codec;
-	struct snd_soc_dapm_context *dapm = widget->dapm;
-	struct snd_soc_card *card = dapm->card;
-
-	/* check for valid widgets */
-	if (widget->reg < 0 || widget->id == snd_soc_dapm_input ||
-		widget->id == snd_soc_dapm_output ||
-		widget->id == snd_soc_dapm_hp ||
-		widget->id == snd_soc_dapm_mic ||
-		widget->id == snd_soc_dapm_line ||
-		widget->id == snd_soc_dapm_spk)
-		return 0;
-
-	power = widget->power;
-	if (widget->invert)
-		power = (power ? 0:1);
-
-	old = snd_soc_read(codec, widget->reg);
-	new = (old & ~(0x1 << widget->shift)) | (power << widget->shift);
-
-	change = old != new;
-	if (change) {
-		pop_dbg(dapm->dev, card->pop_time,
-			"pop test %s : %s in %d ms\n",
-			widget->name, widget->power ? "on" : "off",
-			card->pop_time);
-		pop_wait(card->pop_time);
-		snd_soc_write(codec, widget->reg, new);
-	}
-	dev_dbg(dapm->dev, "reg %x old %x new %x change %d\n", widget->reg,
-		old, new, change);
-	return change;
-}
-
 /* create new dapm mixer control */
 static int dapm_new_mixer(struct snd_soc_dapm_context *dapm,
 	struct snd_soc_dapm_widget *w)
@@ -644,57 +605,6 @@ int dapm_reg_event(struct snd_soc_dapm_widget *w,
 }
 EXPORT_SYMBOL_GPL(dapm_reg_event);
 
-/* Standard power change method, used to apply power changes to most
- * widgets.
- */
-static int dapm_generic_apply_power(struct snd_soc_dapm_widget *w)
-{
-	int ret;
-
-	/* call any power change event handlers */
-	if (w->event)
-		dev_dbg(w->dapm->dev, "power %s event for %s flags %x\n",
-			 w->power ? "on" : "off",
-			 w->name, w->event_flags);
-
-	/* power up pre event */
-	if (w->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 (!w->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 (w->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 (!w->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;
-	}
-
-	return 0;
-}
-
 /* Generic check to see if a widget should be powered.
  */
 static int dapm_generic_check_power(struct snd_soc_dapm_widget *w)
@@ -981,16 +891,6 @@ static void dapm_seq_run(struct snd_soc_dapm_context *dapm,
 					       NULL, SND_SOC_DAPM_POST_PMD);
 			break;
 
-		case snd_soc_dapm_input:
-		case snd_soc_dapm_output:
-		case snd_soc_dapm_hp:
-		case snd_soc_dapm_mic:
-		case snd_soc_dapm_line:
-		case snd_soc_dapm_spk:
-			/* No register support currently */
-			ret = dapm_generic_apply_power(w);
-			break;
-
 		default:
 			/* Queue it up for application */
 			cur_sort = sort[w->id];
-- 
1.7.4.1

Re: [PATCH] ASoC: Remove special casing for registerless widgets

[Lu Guanqun]

On Sat, Apr 02, 2011 at 03:49:01PM +0800, Mark Brown wrote:
> Since we recently explicitly set the register for registerless widgets
> to no register there is no longer any need to special case power updates
> for them, we can allow them to be handled with the register compression
> code as other widgets are.
> 
> As this is the only remaining user of dapm_generic_apply_power() and
> dapm_update_bits() also remove those functions.
> 
> Noticed-by: Lu Guanqun <guanqun.lu@intel.com>
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
> 
> Compile tested only currently, sending it out now as part of discussion
> with Lu Guanqun.

I see the point what you were saying.

But I still don't quite understand the write compression, it aims at
reducing the number of register writes, however in
dapm_seq_run_coalesced, it calls snd_soc_update_bits.

Could you explain a little bit? It's appreciated. I'm vauge in this
part.

> 
>  sound/soc/soc-dapm.c |  100 --------------------------------------------------
>  1 files changed, 0 insertions(+), 100 deletions(-)
> 
> diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
> index b11d011..c9196d6 100644
> --- a/sound/soc/soc-dapm.c
> +++ b/sound/soc/soc-dapm.c
> @@ -322,45 +322,6 @@ static int dapm_connect_mixer(struct snd_soc_dapm_context *dapm,
>  	return -ENODEV;
>  }
>  
> -/* update dapm codec register bits */
> -static int dapm_update_bits(struct snd_soc_dapm_widget *widget)
> -{
> -	int change, power;
> -	unsigned int old, new;
> -	struct snd_soc_codec *codec = widget->codec;
> -	struct snd_soc_dapm_context *dapm = widget->dapm;
> -	struct snd_soc_card *card = dapm->card;
> -
> -	/* check for valid widgets */
> -	if (widget->reg < 0 || widget->id == snd_soc_dapm_input ||
> -		widget->id == snd_soc_dapm_output ||
> -		widget->id == snd_soc_dapm_hp ||
> -		widget->id == snd_soc_dapm_mic ||
> -		widget->id == snd_soc_dapm_line ||
> -		widget->id == snd_soc_dapm_spk)
> -		return 0;
> -
> -	power = widget->power;
> -	if (widget->invert)
> -		power = (power ? 0:1);
> -
> -	old = snd_soc_read(codec, widget->reg);
> -	new = (old & ~(0x1 << widget->shift)) | (power << widget->shift);
> -
> -	change = old != new;
> -	if (change) {
> -		pop_dbg(dapm->dev, card->pop_time,
> -			"pop test %s : %s in %d ms\n",
> -			widget->name, widget->power ? "on" : "off",
> -			card->pop_time);
> -		pop_wait(card->pop_time);
> -		snd_soc_write(codec, widget->reg, new);
> -	}
> -	dev_dbg(dapm->dev, "reg %x old %x new %x change %d\n", widget->reg,
> -		old, new, change);
> -	return change;
> -}
> -
>  /* create new dapm mixer control */
>  static int dapm_new_mixer(struct snd_soc_dapm_context *dapm,
>  	struct snd_soc_dapm_widget *w)
> @@ -644,57 +605,6 @@ int dapm_reg_event(struct snd_soc_dapm_widget *w,
>  }
>  EXPORT_SYMBOL_GPL(dapm_reg_event);
>  
> -/* Standard power change method, used to apply power changes to most
> - * widgets.
> - */
> -static int dapm_generic_apply_power(struct snd_soc_dapm_widget *w)
> -{
> -	int ret;
> -
> -	/* call any power change event handlers */
> -	if (w->event)
> -		dev_dbg(w->dapm->dev, "power %s event for %s flags %x\n",
> -			 w->power ? "on" : "off",
> -			 w->name, w->event_flags);
> -
> -	/* power up pre event */
> -	if (w->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 (!w->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 (w->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 (!w->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;
> -	}
> -
> -	return 0;
> -}
> -
>  /* Generic check to see if a widget should be powered.
>   */
>  static int dapm_generic_check_power(struct snd_soc_dapm_widget *w)
> @@ -981,16 +891,6 @@ static void dapm_seq_run(struct snd_soc_dapm_context *dapm,
>  					       NULL, SND_SOC_DAPM_POST_PMD);
>  			break;
>  
> -		case snd_soc_dapm_input:
> -		case snd_soc_dapm_output:
> -		case snd_soc_dapm_hp:
> -		case snd_soc_dapm_mic:
> -		case snd_soc_dapm_line:
> -		case snd_soc_dapm_spk:
> -			/* No register support currently */
> -			ret = dapm_generic_apply_power(w);
> -			break;
> -
>  		default:
>  			/* Queue it up for application */
>  			cur_sort = sort[w->id];
> -- 
> 1.7.4.1
> 

-- 
guanqun

Re: [PATCH] ASoC: Remove special casing for registerless widgets

[Lu Guanqun]

On Sat, Apr 02, 2011 at 04:07:55PM +0800, Lu Guanqun wrote:
> On Sat, Apr 02, 2011 at 03:49:01PM +0800, Mark Brown wrote:
> > Since we recently explicitly set the register for registerless widgets
> > to no register there is no longer any need to special case power updates
> > for them, we can allow them to be handled with the register compression
> > code as other widgets are.
> > 
> > As this is the only remaining user of dapm_generic_apply_power() and
> > dapm_update_bits() also remove those functions.
> > 
> > Noticed-by: Lu Guanqun <guanqun.lu@intel.com>
> > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > ---
> > 
> > Compile tested only currently, sending it out now as part of discussion
> > with Lu Guanqun.
> 
> I see the point what you were saying.
> 
> But I still don't quite understand the write compression, it aims at
> reducing the number of register writes, however in
> dapm_seq_run_coalesced, it calls snd_soc_update_bits.

I mean, for write compression or not, as long as its widget register is
valid, it will be updated. So I don't see why write compression would
reduce the number...

> 
> Could you explain a little bit? It's appreciated. I'm vauge in this
> part.
> 
> > 
> >  sound/soc/soc-dapm.c |  100 --------------------------------------------------
> >  1 files changed, 0 insertions(+), 100 deletions(-)
> > 
> > diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
> > index b11d011..c9196d6 100644
> > --- a/sound/soc/soc-dapm.c
> > +++ b/sound/soc/soc-dapm.c
> > @@ -322,45 +322,6 @@ static int dapm_connect_mixer(struct snd_soc_dapm_context *dapm,
> >  	return -ENODEV;
> >  }
> >  
> > -/* update dapm codec register bits */
> > -static int dapm_update_bits(struct snd_soc_dapm_widget *widget)
> > -{
> > -	int change, power;
> > -	unsigned int old, new;
> > -	struct snd_soc_codec *codec = widget->codec;
> > -	struct snd_soc_dapm_context *dapm = widget->dapm;
> > -	struct snd_soc_card *card = dapm->card;
> > -
> > -	/* check for valid widgets */
> > -	if (widget->reg < 0 || widget->id == snd_soc_dapm_input ||
> > -		widget->id == snd_soc_dapm_output ||
> > -		widget->id == snd_soc_dapm_hp ||
> > -		widget->id == snd_soc_dapm_mic ||
> > -		widget->id == snd_soc_dapm_line ||
> > -		widget->id == snd_soc_dapm_spk)
> > -		return 0;
> > -
> > -	power = widget->power;
> > -	if (widget->invert)
> > -		power = (power ? 0:1);
> > -
> > -	old = snd_soc_read(codec, widget->reg);
> > -	new = (old & ~(0x1 << widget->shift)) | (power << widget->shift);
> > -
> > -	change = old != new;
> > -	if (change) {
> > -		pop_dbg(dapm->dev, card->pop_time,
> > -			"pop test %s : %s in %d ms\n",
> > -			widget->name, widget->power ? "on" : "off",
> > -			card->pop_time);
> > -		pop_wait(card->pop_time);
> > -		snd_soc_write(codec, widget->reg, new);
> > -	}
> > -	dev_dbg(dapm->dev, "reg %x old %x new %x change %d\n", widget->reg,
> > -		old, new, change);
> > -	return change;
> > -}
> > -
> >  /* create new dapm mixer control */
> >  static int dapm_new_mixer(struct snd_soc_dapm_context *dapm,
> >  	struct snd_soc_dapm_widget *w)
> > @@ -644,57 +605,6 @@ int dapm_reg_event(struct snd_soc_dapm_widget *w,
> >  }
> >  EXPORT_SYMBOL_GPL(dapm_reg_event);
> >  
> > -/* Standard power change method, used to apply power changes to most
> > - * widgets.
> > - */
> > -static int dapm_generic_apply_power(struct snd_soc_dapm_widget *w)
> > -{
> > -	int ret;
> > -
> > -	/* call any power change event handlers */
> > -	if (w->event)
> > -		dev_dbg(w->dapm->dev, "power %s event for %s flags %x\n",
> > -			 w->power ? "on" : "off",
> > -			 w->name, w->event_flags);
> > -
> > -	/* power up pre event */
> > -	if (w->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 (!w->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 (w->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 (!w->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;
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> >  /* Generic check to see if a widget should be powered.
> >   */
> >  static int dapm_generic_check_power(struct snd_soc_dapm_widget *w)
> > @@ -981,16 +891,6 @@ static void dapm_seq_run(struct snd_soc_dapm_context *dapm,
> >  					       NULL, SND_SOC_DAPM_POST_PMD);
> >  			break;
> >  
> > -		case snd_soc_dapm_input:
> > -		case snd_soc_dapm_output:
> > -		case snd_soc_dapm_hp:
> > -		case snd_soc_dapm_mic:
> > -		case snd_soc_dapm_line:
> > -		case snd_soc_dapm_spk:
> > -			/* No register support currently */
> > -			ret = dapm_generic_apply_power(w);
> > -			break;
> > -
> >  		default:
> >  			/* Queue it up for application */
> >  			cur_sort = sort[w->id];
> > -- 
> > 1.7.4.1
> > 
> 
> -- 
> guanqun
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

-- 
guanqun

Re: [PATCH] ASoC: Remove special casing for registerless widgets

[Mark Brown]

On Sat, Apr 02, 2011 at 04:12:44PM +0800, Lu Guanqun wrote:
> On Sat, Apr 02, 2011 at 04:07:55PM +0800, Lu Guanqun wrote:

> > But I still don't quite understand the write compression, it aims at
> > reducing the number of register writes, however in
> > dapm_seq_run_coalesced, it calls snd_soc_update_bits.

> I mean, for write compression or not, as long as its widget register is
> valid, it will be updated. So I don't see why write compression would
> reduce the number...

The idea is that since you often have all the enable bits together in a
small set of registers if you're enabling two of the same widget type at
once and they're both in the same register you can do it with one write.
For example, if you're doing a stereo playback this will usually mean
that enabling left and right DACs is one register write.  This is also
good for pop/click performance as it ensures that both channels turn on
simultaneously.

You can see this quite easily if you turn on DAPM pop debugging with a
short delay - write to asoc/$CARD/dapm_pop_time in debugfs and start a
playback or something and you'll see logging showing the writes being
combined.

Re: [PATCH] ASoC: Remove special casing for registerless widgets

[Lu Guanqun]

On Sat, Apr 02, 2011 at 04:18:02PM +0800, Mark Brown wrote:
> On Sat, Apr 02, 2011 at 04:12:44PM +0800, Lu Guanqun wrote:
> > On Sat, Apr 02, 2011 at 04:07:55PM +0800, Lu Guanqun wrote:
> 
> > > But I still don't quite understand the write compression, it aims at
> > > reducing the number of register writes, however in
> > > dapm_seq_run_coalesced, it calls snd_soc_update_bits.
> 
> > I mean, for write compression or not, as long as its widget register is
> > valid, it will be updated. So I don't see why write compression would
> > reduce the number...
> 
> The idea is that since you often have all the enable bits together in a
> small set of registers if you're enabling two of the same widget type at
> once and they're both in the same register you can do it with one write.
> For example, if you're doing a stereo playback this will usually mean
> that enabling left and right DACs is one register write.  This is also
> good for pop/click performance as it ensures that both channels turn on
> simultaneously.

Thanks for the explaination. I get the idea.

> You can see this quite easily if you turn on DAPM pop debugging with a
> short delay - write to asoc/$CARD/dapm_pop_time in debugfs and start a
> playback or something and you'll see logging showing the writes being
> combined.

I'll try this and see the differences.

Thanks Mark!

-- 
guanqun