[alsa-devel] [PATCH] ASoC: dapm: Add speaker driver widget.
In some cases it was not possible to follow the appropiate power ON/OFF sequence. Add a widget to support speaker drivers where power ON/OFF ordering is important.
Signed-off-by: Margarita Olaya Cabrera magi.olaya@ti.com --- include/sound/soc-dapm.h | 10 ++++++++++ sound/soc/soc-dapm.c | 6 ++++++ 2 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h index 041e98b..3b438e5 100644 --- a/include/sound/soc-dapm.h +++ b/include/sound/soc-dapm.h @@ -71,6 +71,10 @@ wcontrols, wncontrols) \ { .id = snd_soc_dapm_pga, .name = wname, .reg = wreg, .shift = wshift, \ .invert = winvert, .kcontrols = wcontrols, .num_kcontrols = wncontrols} +#define SND_SOC_DAPM_DRV(wname, wreg, wshift, winvert,\ + wcontrols, wncontrols) \ +{ .id = snd_soc_dapm_drv, .name = wname, .reg = wreg, .shift = wshift, \ + .invert = winvert, .kcontrols = wcontrols, .num_kcontrols = wncontrols} #define SND_SOC_DAPM_MIXER(wname, wreg, wshift, winvert, \ wcontrols, wncontrols)\ { .id = snd_soc_dapm_mixer, .name = wname, .reg = wreg, .shift = wshift, \ @@ -115,6 +119,11 @@ { .id = snd_soc_dapm_pga, .name = wname, .reg = wreg, .shift = wshift, \ .invert = winvert, .kcontrols = wcontrols, .num_kcontrols = wncontrols, \ .event = wevent, .event_flags = wflags} +#define SND_SOC_DAPM_DRV_E(wname, wreg, wshift, winvert, wcontrols, \ + wncontrols, wevent, wflags) \ +{ .id = snd_soc_dapm_drv, .name = wname, .reg = wreg, .shift = wshift, \ + .invert = winvert, .kcontrols = wcontrols, .num_kcontrols = wncontrols, \ + .event = wevent, .event_flags = wflags} #define SND_SOC_DAPM_MIXER_E(wname, wreg, wshift, winvert, wcontrols, \ wncontrols, wevent, wflags) \ { .id = snd_soc_dapm_mixer, .name = wname, .reg = wreg, .shift = wshift, \ @@ -368,6 +377,7 @@ enum snd_soc_dapm_type { snd_soc_dapm_mixer, /* mixes several analog signals together */ snd_soc_dapm_mixer_named_ctl, /* mixer with named controls */ snd_soc_dapm_pga, /* programmable gain/attenuation (volume) */ + snd_soc_dapm_drv, /* speaker driver */ snd_soc_dapm_adc, /* analog to digital converter */ snd_soc_dapm_dac, /* digital to analog converter */ snd_soc_dapm_micbias, /* microphone bias (power) */ diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 6a29d59..4726b65 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -61,6 +61,7 @@ static int dapm_up_seq[] = { [snd_soc_dapm_mixer] = 7, [snd_soc_dapm_mixer_named_ctl] = 7, [snd_soc_dapm_pga] = 8, + [snd_soc_dapm_drv] = 9, [snd_soc_dapm_adc] = 9, [snd_soc_dapm_hp] = 10, [snd_soc_dapm_spk] = 10, @@ -72,6 +73,7 @@ static int dapm_down_seq[] = { [snd_soc_dapm_adc] = 1, [snd_soc_dapm_hp] = 2, [snd_soc_dapm_spk] = 2, + [snd_soc_dapm_drv] = 3, [snd_soc_dapm_pga] = 4, [snd_soc_dapm_mixer_named_ctl] = 5, [snd_soc_dapm_mixer] = 5, @@ -231,6 +233,7 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w, break; /* does not effect routing - always connected */ case snd_soc_dapm_pga: + case snd_soc_dapm_drv: case snd_soc_dapm_output: case snd_soc_dapm_adc: case snd_soc_dapm_input: @@ -1243,6 +1246,7 @@ static ssize_t dapm_widget_show(struct device *dev, case snd_soc_dapm_dac: case snd_soc_dapm_adc: case snd_soc_dapm_pga: + case snd_soc_dapm_drv: case snd_soc_dapm_mixer: case snd_soc_dapm_mixer_named_ctl: case snd_soc_dapm_supply: @@ -1421,6 +1425,7 @@ static int snd_soc_dapm_add_route(struct snd_soc_dapm_context *dapm, case snd_soc_dapm_adc: case snd_soc_dapm_dac: case snd_soc_dapm_pga: + case snd_soc_dapm_drv: case snd_soc_dapm_input: case snd_soc_dapm_output: case snd_soc_dapm_micbias: @@ -1538,6 +1543,7 @@ int snd_soc_dapm_new_widgets(struct snd_soc_dapm_context *dapm) w->power_check = dapm_dac_check_power; break; case snd_soc_dapm_pga: + case snd_soc_dapm_drv: w->power_check = dapm_generic_check_power; dapm_new_pga(dapm, w); break;
On Mon, Dec 06, 2010 at 04:34:37PM -0600, Olaya, Margarita wrote:
In some cases it was not possible to follow the appropiate power ON/OFF sequence. Add a widget to support speaker drivers where power ON/OFF ordering is important.
Why not use the existing speaker widget? It's at pretty much the same point in the sequence and is intended for use with external GPIO controlled speaker drivers. It'd be good to discuss this in the changelog.
+#define SND_SOC_DAPM_DRV(wname, wreg, wshift, winvert,\
wcontrols, wncontrols) \+{ .id = snd_soc_dapm_drv, .name = wname, .reg = wreg, .shift = wshift, \
- .invert = winvert, .kcontrols = wcontrols, .num_kcontrols = wncontrols}
The _DRV name seems rather opaque - I'd suggest _SPK as a name but obviously that's in use. If we do want this I guess _SPK_DRV or sommething.
On Mon, 2010-12-06 at 22:43 +0000, Mark Brown wrote:
On Mon, Dec 06, 2010 at 04:34:37PM -0600, Olaya, Margarita wrote:
In some cases it was not possible to follow the appropiate power ON/OFF sequence. Add a widget to support speaker drivers where power ON/OFF ordering is important.
Why not use the existing speaker widget? It's at pretty much the same point in the sequence and is intended for use with external GPIO controlled speaker drivers. It'd be good to discuss this in the changelog.
In this case the driver block is on the CODEC IC and after the PGA in the audio output path, hence this version is better suited than the external GPIO version.
+#define SND_SOC_DAPM_DRV(wname, wreg, wshift, winvert,\
wcontrols, wncontrols) \+{ .id = snd_soc_dapm_drv, .name = wname, .reg = wreg, .shift = wshift, \
- .invert = winvert, .kcontrols = wcontrols, .num_kcontrols = wncontrols}
The _DRV name seems rather opaque - I'd suggest _SPK as a name but obviously that's in use. If we do want this I guess _SPK_DRV or sommething.
I was thinking this too, but then I thought we may want to drive other loads than just speakers here. e.g. haptic, vibra
Liam
On Mon, Dec 06, 2010 at 10:55:32PM +0000, Liam Girdwood wrote:
On Mon, 2010-12-06 at 22:43 +0000, Mark Brown wrote:
Why not use the existing speaker widget? It's at pretty much the same point in the sequence and is intended for use with external GPIO controlled speaker drivers. It'd be good to discuss this in the changelog.
In this case the driver block is on the CODEC IC and after the PGA in the audio output path, hence this version is better suited than the external GPIO version.
Sure, but it's fulfiling the same role in the system - it's just that these days a lot more CODECs are pulling speaker drivers directly into the CODEC die. Mostly these have worked well handled as PGAs so it's not been an issue.
I'd certainly expect to see it handled the same way from a DAPM sequencing point of view as it's fulfilling the same role in the system (so in the same slot rather than separately as the patch was doing). Do we just need to refactor the existing external widgets to be able to exist in either register or GPIO based versions?
+#define SND_SOC_DAPM_DRV(wname, wreg, wshift, winvert,\
wcontrols, wncontrols) \+{ .id = snd_soc_dapm_drv, .name = wname, .reg = wreg, .shift = wshift, \
- .invert = winvert, .kcontrols = wcontrols, .num_kcontrols = wncontrols}
The _DRV name seems rather opaque - I'd suggest _SPK as a name but obviously that's in use. If we do want this I guess _SPK_DRV or sommething.
I was thinking this too, but then I thought we may want to drive other loads than just speakers here. e.g. haptic, vibra
My main thing here is _DRV - it's not just that it's undescriptive, it's also that calling something a driver in a context like this is confusing. _OUT_DRV is another idea? The changelog also needs redoing to reflect the more general use.
On Mon, 2010-12-06 at 23:09 +0000, Mark Brown wrote:
On Mon, Dec 06, 2010 at 10:55:32PM +0000, Liam Girdwood wrote:
On Mon, 2010-12-06 at 22:43 +0000, Mark Brown wrote:
Why not use the existing speaker widget? It's at pretty much the same point in the sequence and is intended for use with external GPIO controlled speaker drivers. It'd be good to discuss this in the changelog.
In this case the driver block is on the CODEC IC and after the PGA in the audio output path, hence this version is better suited than the external GPIO version.
Sure, but it's fulfiling the same role in the system - it's just that these days a lot more CODECs are pulling speaker drivers directly into the CODEC die. Mostly these have worked well handled as PGAs so it's not been an issue.
In this case as we need to enable the PGA before the driver and disable the driver before the PGA for pop reduction. Hence the current ordering needs an addition/refactoring to deal with the newer generation of CODECs here.
I'd certainly expect to see it handled the same way from a DAPM sequencing point of view as it's fulfilling the same role in the system (so in the same slot rather than separately as the patch was doing). Do we just need to refactor the existing external widgets to be able to exist in either register or GPIO based versions?
+#define SND_SOC_DAPM_DRV(wname, wreg, wshift, winvert,\
wcontrols, wncontrols) \+{ .id = snd_soc_dapm_drv, .name = wname, .reg = wreg, .shift = wshift, \
- .invert = winvert, .kcontrols = wcontrols, .num_kcontrols = wncontrols}
The _DRV name seems rather opaque - I'd suggest _SPK as a name but obviously that's in use. If we do want this I guess _SPK_DRV or sommething.
I was thinking this too, but then I thought we may want to drive other loads than just speakers here. e.g. haptic, vibra
My main thing here is _DRV - it's not just that it's undescriptive, it's also that calling something a driver in a context like this is confusing. _OUT_DRV is another idea?
Ok, lets go for OUT_DRV.
Liam
On 6 Dec 2010, at 23:30, Liam Girdwood wrote:
On Mon, 2010-12-06 at 23:09 +0000, Mark Brown wrote:
Sure, but it's fulfiling the same role in the system - it's just that these days a lot more CODECs are pulling speaker drivers directly into the CODEC die. Mostly these have worked well handled as PGAs so it's not been an issue.
In this case as we need to enable the PGA before the driver and disable the driver before the PGA for pop reduction. Hence the current ordering needs an addition/refactoring to deal with the newer generation of CODECs here.
Well, what I'm saying is that...
I'd certainly expect to see it handled the same way from a DAPM sequencing point of view as it's fulfilling the same role in the system (so in the same slot rather than separately as the patch was doing). Do we just need to refactor the existing external widgets to be able to exist in either register or GPIO based versions?
...we don't need to change the ordering at all, we've already got a sequence point for this sort of widget we can use. We should just make the existing speaker and headphone widgets be in terms of one, I think.
The only reason it's not come up before is that everything so far has been able to power up cleanly without splitting from the PGAs.
On Mon, 2010-12-06 at 23:38 +0000, Mark Brown wrote:
On 6 Dec 2010, at 23:30, Liam Girdwood wrote:
On Mon, 2010-12-06 at 23:09 +0000, Mark Brown wrote:
Sure, but it's fulfiling the same role in the system - it's just that these days a lot more CODECs are pulling speaker drivers directly into the CODEC die. Mostly these have worked well handled as PGAs so it's not been an issue.
In this case as we need to enable the PGA before the driver and disable the driver before the PGA for pop reduction. Hence the current ordering needs an addition/refactoring to deal with the newer generation of CODECs here.
Well, what I'm saying is that...
I'd certainly expect to see it handled the same way from a DAPM sequencing point of view as it's fulfilling the same role in the system (so in the same slot rather than separately as the patch was doing). Do we just need to refactor the existing external widgets to be able to exist in either register or GPIO based versions?
...we don't need to change the ordering at all, we've already got a sequence point for this sort of widget we can use. We should just make the existing speaker and headphone widgets be in terms of one, I think.
The only minor problem here is that the current sequence point is name "snd_soc_dapm_spk" which would seem confusing to haptic/vibra driver users. Although, I can live with this....
Liam
On Tue, Dec 07, 2010 at 12:34:45PM +0000, Liam Girdwood wrote:
On Mon, 2010-12-06 at 23:38 +0000, Mark Brown wrote:
...we don't need to change the ordering at all, we've already got a sequence point for this sort of widget we can use. We should just make the existing speaker and headphone widgets be in terms of one, I think.
The only minor problem here is that the current sequence point is name "snd_soc_dapm_spk" which would seem confusing to haptic/vibra driver users. Although, I can live with this....
I meant the number in the sequence - the patch had the new widget coming up in the slot before the existing output driver widgets.
On Tue, 2010-12-07 at 12:36 +0000, Mark Brown wrote:
On Tue, Dec 07, 2010 at 12:34:45PM +0000, Liam Girdwood wrote:
On Mon, 2010-12-06 at 23:38 +0000, Mark Brown wrote:
...we don't need to change the ordering at all, we've already got a sequence point for this sort of widget we can use. We should just make the existing speaker and headphone widgets be in terms of one, I think.
The only minor problem here is that the current sequence point is name "snd_soc_dapm_spk" which would seem confusing to haptic/vibra driver users. Although, I can live with this....
I meant the number in the sequence - the patch had the new widget coming up in the slot before the existing output driver widgets.
Ok, but by sharing the number we take away some flexibility in the sequence ordering. Say something has to be done in the machine driver _after_ the spk/haptic/vibra driver is enabled in the CODEC driver ?
I dont actually see any major issue of using a new number here unless you have it reserved for something else ?
Liam
On Tue, Dec 07, 2010 at 12:46:28PM +0000, Liam Girdwood wrote:
Ok, but by sharing the number we take away some flexibility in the sequence ordering. Say something has to be done in the machine driver _after_ the spk/haptic/vibra driver is enabled in the CODEC driver ?
You can say that for pretty much all of the widgets, though, including the existing speaker and headphone widgets. I'd be a bit surprised if you had anything afterwards in this case, a driver is generally the high power stage wired directly to the transducer.
I dont actually see any major issue of using a new number here unless you have it reserved for something else ?
Adding a new number and renumbering all the subsequent ones wouldn't be so bad, the patch wasn't doing that.
On Tue, 2010-12-07 at 13:09 +0000, Mark Brown wrote:
On Tue, Dec 07, 2010 at 12:46:28PM +0000, Liam Girdwood wrote:
Ok, but by sharing the number we take away some flexibility in the sequence ordering. Say something has to be done in the machine driver _after_ the spk/haptic/vibra driver is enabled in the CODEC driver ?
You can say that for pretty much all of the widgets, though, including the existing speaker and headphone widgets.
You can only really say for the last widget in the sequence.
I'd be a bit surprised if you had anything afterwards in this case, a driver is generally the high power stage wired directly to the transducer.
I dont actually see any major issue of using a new number here unless you have it reserved for something else ?
Adding a new number and renumbering all the subsequent ones wouldn't be so bad, the patch wasn't doing that.
@@ -61,6 +61,7 @@ static int dapm_up_seq[] = { [snd_soc_dapm_mixer] = 7, [snd_soc_dapm_mixer_named_ctl] = 7, [snd_soc_dapm_pga] = 8, + [snd_soc_dapm_drv] = 9, [snd_soc_dapm_adc] = 9, [snd_soc_dapm_hp] = 10, [snd_soc_dapm_spk] = 10,
This patch shares the ADC sequence number for the DRV on the power ON.
@@ -72,6 +73,7 @@ static int dapm_down_seq[] = { [snd_soc_dapm_adc] = 1, [snd_soc_dapm_hp] = 2, [snd_soc_dapm_spk] = 2, + [snd_soc_dapm_drv] = 3, [snd_soc_dapm_pga] = 4, [snd_soc_dapm_mixer_named_ctl] = 5, [snd_soc_dapm_mixer] = 5,
and inserts a new number as you suggested for power OFF. Imo, nothing wrong with this approach and since I don't have all day to discuss, Magi will change both the sequence numbers to use the same number as snd_soc_dapm_spk.
Liam
On Tue, Dec 07, 2010 at 02:01:24PM +0000, Liam Girdwood wrote:
On Tue, 2010-12-07 at 13:09 +0000, Mark Brown wrote:
You can say that for pretty much all of the widgets, though, including the existing speaker and headphone widgets.
You can only really say for the last widget in the sequence.
Pretty much anything might be the last widget in the chain, and obviously you can need to do stuff like this within the CODEC as well as the machine (after all that's exactly what this is about).
For full flexibility we need something like optional sorting within each widget type.
@@ -61,6 +61,7 @@ static int dapm_up_seq[] = { [snd_soc_dapm_mixer] = 7, [snd_soc_dapm_mixer_named_ctl] = 7, [snd_soc_dapm_pga] = 8,
[snd_soc_dapm_drv] = 9, [snd_soc_dapm_adc] = 9, [snd_soc_dapm_hp] = 10, [snd_soc_dapm_spk] = 10,
This patch shares the ADC sequence number for the DRV on the power ON.
This isn't entirely great for digital bypass paths (the current setup isn't ideal either but that's a separate issue and harder to fix as you can get PGAs on both input and output paths). You'll power up your output driver and only then start feeding through samples from the ADC, meaning a sudden introduction of any DC offset in the analogue input that the ADC is passing through rather than getting the benefit of any soft start or DC offset correction the hardware does. I'd rather see us ading the new widget at step 10, either with the existing speaker and headphone driver widgets or moving them up a step.
and inserts a new number as you suggested for power OFF. Imo, nothing wrong with this approach and since I don't have all day to discuss, Magi will change both the sequence numbers to use the same number as snd_soc_dapm_spk.
Right, the down sequence is fine.
participants (3)
-
Liam Girdwood -
Mark Brown -
Olaya, Margarita