[alsa-devel] [PATCH 1/6] ASoC: Tegra: Harmony: Add headphone jack detection
Signed-off-by: Stephen Warren swarren@nvidia.com --- sound/soc/tegra/harmony.c | 30 +++++++++++++++++++++++++++++- 1 files changed, 29 insertions(+), 1 deletions(-)
diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c index 1b80c4f..26ab4fd 100644 --- a/sound/soc/tegra/harmony.c +++ b/sound/soc/tegra/harmony.c @@ -38,6 +38,7 @@ #include <mach/harmony_audio.h>
#include <sound/core.h> +#include <sound/jack.h> #include <sound/pcm.h> #include <sound/pcm_params.h> #include <sound/soc.h> @@ -123,6 +124,24 @@ static struct snd_soc_ops harmony_asoc_ops = { .hw_params = harmony_asoc_hw_params, };
+static struct snd_soc_jack harmony_hp_jack; + +static struct snd_soc_jack_pin harmony_hp_jack_pins[] = { + { + .pin = "Headphone Jack", + .mask = SND_JACK_HEADPHONE, + }, +}; + +static struct snd_soc_jack_gpio harmony_hp_jack_gpios[] = { + { + .name = "headphone detect", + .report = SND_JACK_HEADPHONE, + .debounce_time = 150, + .invert = 1, + } +}; + static int harmony_event_int_spk(struct snd_soc_dapm_widget *w, struct snd_kcontrol *k, int event) { @@ -178,11 +197,20 @@ static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd) snd_soc_dapm_add_routes(dapm, harmony_audio_map, ARRAY_SIZE(harmony_audio_map));
- snd_soc_dapm_enable_pin(dapm, "Headphone Jack"); snd_soc_dapm_enable_pin(dapm, "Int Spk"); snd_soc_dapm_enable_pin(dapm, "Mic Jack"); snd_soc_dapm_sync(dapm);
+ harmony_hp_jack_gpios[0].gpio = pdata->gpio_hp_det; + snd_soc_jack_new(codec, "Headphone Jack", SND_JACK_HEADPHONE, + &harmony_hp_jack); + snd_soc_jack_add_pins(&harmony_hp_jack, + ARRAY_SIZE(harmony_hp_jack_pins), + harmony_hp_jack_pins); + snd_soc_jack_add_gpios(&harmony_hp_jack, + ARRAY_SIZE(harmony_hp_jack_gpios), + harmony_hp_jack_gpios); + return 0; }
Signed-off-by: Stephen Warren swarren@nvidia.com --- sound/soc/tegra/harmony.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c index 26ab4fd..e87395c 100644 --- a/sound/soc/tegra/harmony.c +++ b/sound/soc/tegra/harmony.c @@ -173,6 +173,10 @@ static const struct snd_soc_dapm_route harmony_audio_map[] = { {"IN1L", NULL, "Mic Bias"}, };
+static const struct snd_kcontrol_new harmony_controls[] = { + SOC_DAPM_PIN_SWITCH("Int Spk"), +}; + static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd) { struct snd_soc_codec *codec = rtd->codec; @@ -191,13 +195,17 @@ static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd)
gpio_direction_output(pdata->gpio_spkr_en, 0);
+ ret = snd_soc_add_controls(codec, harmony_controls, + ARRAY_SIZE(harmony_controls)); + if (ret < 0) + return ret; + snd_soc_dapm_new_controls(dapm, harmony_dapm_widgets, ARRAY_SIZE(harmony_dapm_widgets));
snd_soc_dapm_add_routes(dapm, harmony_audio_map, ARRAY_SIZE(harmony_audio_map));
- snd_soc_dapm_enable_pin(dapm, "Int Spk"); snd_soc_dapm_enable_pin(dapm, "Mic Jack"); snd_soc_dapm_sync(dapm);
On Thu, Feb 03, 2011 at 01:56:14PM -0700, Stephen Warren wrote:
Signed-off-by: Stephen Warren swarren@nvidia.com
This looks good but won't apply without patch 1 so please resubmit.
On Thu, Feb 03, 2011 at 01:56:14PM -0700, Stephen Warren wrote:
Signed-off-by: Stephen Warren swarren@nvidia.com
Applied, thanks.
wm8903.h: * There is no hysteresis enable field in the current datasheet. * Mic detection threshold field is only 2 bits wide.
wm8903.c: * Mic detection HW should be enabled if SW wants either mic detection or short detection, rather than only when it wants both.
Signed-off-by: Stephen Warren swarren@nvidia.com --- include/sound/wm8903.h | 10 +++------- sound/soc/codecs/wm8903.c | 2 +- 2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/include/sound/wm8903.h b/include/sound/wm8903.h index 86172cf..cf7ccb7 100644 --- a/include/sound/wm8903.h +++ b/include/sound/wm8903.h @@ -17,13 +17,9 @@ /* * R6 (0x06) - Mic Bias Control 0 */ -#define WM8903_MICDET_HYST_ENA 0x0080 /* MICDET_HYST_ENA */ -#define WM8903_MICDET_HYST_ENA_MASK 0x0080 /* MICDET_HYST_ENA */ -#define WM8903_MICDET_HYST_ENA_SHIFT 7 /* MICDET_HYST_ENA */ -#define WM8903_MICDET_HYST_ENA_WIDTH 1 /* MICDET_HYST_ENA */ -#define WM8903_MICDET_THR_MASK 0x0070 /* MICDET_THR - [6:4] */ -#define WM8903_MICDET_THR_SHIFT 4 /* MICDET_THR - [6:4] */ -#define WM8903_MICDET_THR_WIDTH 3 /* MICDET_THR - [6:4] */ +#define WM8903_MICDET_THR_MASK 0x0030 /* MICDET_THR - [5:4] */ +#define WM8903_MICDET_THR_SHIFT 4 /* MICDET_THR - [5:4] */ +#define WM8903_MICDET_THR_WIDTH 2 /* MICDET_THR - [5:4] */ #define WM8903_MICSHORT_THR_MASK 0x000C /* MICSHORT_THR - [3:2] */ #define WM8903_MICSHORT_THR_SHIFT 2 /* MICSHORT_THR - [3:2] */ #define WM8903_MICSHORT_THR_WIDTH 2 /* MICSHORT_THR - [3:2] */ diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c index 3d4c55f..dc96573 100644 --- a/sound/soc/codecs/wm8903.c +++ b/sound/soc/codecs/wm8903.c @@ -1489,7 +1489,7 @@ int wm8903_mic_detect(struct snd_soc_codec *codec, struct snd_soc_jack *jack, WM8903_MICDET_EINT | WM8903_MICSHRT_EINT, irq_mask);
- if (det && shrt) { + if (det || shrt) { /* Enable mic detection, this may not have been set through * platform data (eg, if the defaults are OK). */ snd_soc_update_bits(codec, WM8903_WRITE_SEQUENCER_0,
On Thu, Feb 03, 2011 at 01:56:15PM -0700, Stephen Warren wrote:
wm8903.h:
- There is no hysteresis enable field in the current datasheet.
- Mic detection threshold field is only 2 bits wide.
wm8903.c:
- Mic detection HW should be enabled if SW wants either mic detection or short detection, rather than only when it wants both.
Please don't combine unrelated changes into a single patch, especially in cases like this where one is a bug fix and the other is essentially a cosmetic fixup.
* Add jack definition for mic jack * Request wm8903 to enable mic detection * Remove Mic Bias from DAPM route map, so that Mic Bias isn't disable when capture is stopped, thus preventing further mic detection from operating.
Signed-off-by: Stephen Warren swarren@nvidia.com --- sound/soc/tegra/harmony.c | 24 +++++++++++++++++++----- 1 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c index e87395c..799b1ef 100644 --- a/sound/soc/tegra/harmony.c +++ b/sound/soc/tegra/harmony.c @@ -43,6 +43,8 @@ #include <sound/pcm_params.h> #include <sound/soc.h>
+#include "../codecs/wm8903.h" + #include "tegra_das.h" #include "tegra_i2s.h" #include "tegra_pcm.h" @@ -142,6 +144,15 @@ static struct snd_soc_jack_gpio harmony_hp_jack_gpios[] = { } };
+static struct snd_soc_jack harmony_mic_jack; + +static struct snd_soc_jack_pin harmony_mic_jack_pins[] = { + { + .pin = "Mic Jack", + .mask = SND_JACK_MICROPHONE, + }, +}; + static int harmony_event_int_spk(struct snd_soc_dapm_widget *w, struct snd_kcontrol *k, int event) { @@ -169,8 +180,7 @@ static const struct snd_soc_dapm_route harmony_audio_map[] = { {"Int Spk", NULL, "RON"}, {"Int Spk", NULL, "LOP"}, {"Int Spk", NULL, "LON"}, - {"Mic Bias", NULL, "Mic Jack"}, - {"IN1L", NULL, "Mic Bias"}, + {"IN1L", NULL, "Mic Jack"}, };
static const struct snd_kcontrol_new harmony_controls[] = { @@ -206,9 +216,6 @@ static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd) snd_soc_dapm_add_routes(dapm, harmony_audio_map, ARRAY_SIZE(harmony_audio_map));
- snd_soc_dapm_enable_pin(dapm, "Mic Jack"); - snd_soc_dapm_sync(dapm); - harmony_hp_jack_gpios[0].gpio = pdata->gpio_hp_det; snd_soc_jack_new(codec, "Headphone Jack", SND_JACK_HEADPHONE, &harmony_hp_jack); @@ -219,6 +226,13 @@ static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd) ARRAY_SIZE(harmony_hp_jack_gpios), harmony_hp_jack_gpios);
+ snd_soc_jack_new(codec, "Mic Jack", SND_JACK_MICROPHONE, + &harmony_mic_jack); + snd_soc_jack_add_pins(&harmony_mic_jack, + ARRAY_SIZE(harmony_mic_jack_pins), + harmony_mic_jack_pins); + wm8903_mic_detect(codec, &harmony_mic_jack, SND_JACK_MICROPHONE, 0); + return 0; }
On Thu, Feb 03, 2011 at 01:56:16PM -0700, Stephen Warren wrote:
- Remove Mic Bias from DAPM route map, so that Mic Bias isn't disable when capture is stopped, thus preventing further mic detection from operating.
You should be using snd_soc_dapm_force_enable_pin() to do this.
Signed-off-by: Stephen Warren swarren@nvidia.com --- sound/soc/tegra/harmony.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c index 799b1ef..8ed88e3 100644 --- a/sound/soc/tegra/harmony.c +++ b/sound/soc/tegra/harmony.c @@ -205,6 +205,12 @@ static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd)
gpio_direction_output(pdata->gpio_spkr_en, 0);
+ /* Not connected */ + snd_soc_dapm_nc_pin(dapm, "IN3L"); + snd_soc_dapm_nc_pin(dapm, "IN3R"); + snd_soc_dapm_nc_pin(dapm, "LINEOUTL"); + snd_soc_dapm_nc_pin(dapm, "LINEOUTR"); + ret = snd_soc_add_controls(codec, harmony_controls, ARRAY_SIZE(harmony_controls)); if (ret < 0)
On Thu, Feb 03, 2011 at 01:56:17PM -0700, Stephen Warren wrote:
Signed-off-by: Stephen Warren swarren@nvidia.com
Similarly here.
Note that the external mic is only connected to the codec's left channel, and the internal mic is only connected to the codec's right channel.
Hence, when recording a stereo stream, which is the only supported channel configuration, the recorded audio will only appear within one channel, and which channel that is will differ depending on which mic is used.
At present, I'm not sure how best to resolve this. Downstream drivers directly tweaked the wm8903's registers so that both ADCs processed the same input channel.
Signed-off-by: Stephen Warren swarren@nvidia.com --- sound/soc/tegra/harmony.c | 88 +++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 85 insertions(+), 3 deletions(-)
diff --git a/sound/soc/tegra/harmony.c b/sound/soc/tegra/harmony.c index 8ed88e3..b4526c4 100644 --- a/sound/soc/tegra/harmony.c +++ b/sound/soc/tegra/harmony.c @@ -52,10 +52,19 @@
#define DRV_NAME "tegra-snd-harmony"
+#define GPIO_SPKR_EN BIT(0) +#define GPIO_INT_MIC_EN BIT(1) +#define GPIO_EXT_MIC_EN BIT(2) + +#define MIC_SELECTION_OFF 0 +#define MIC_SELECTION_EXT_MIC 1 +#define MIC_SELECTION_INT_MIC 2 + struct tegra_harmony { struct tegra_asoc_utils_data util_data; struct harmony_audio_platform_data *pdata; - int gpio_spkr_en_requested; + int gpio_requested; + int mic_selection; };
static int harmony_asoc_hw_params(struct snd_pcm_substream *substream, @@ -181,10 +190,61 @@ static const struct snd_soc_dapm_route harmony_audio_map[] = { {"Int Spk", NULL, "LOP"}, {"Int Spk", NULL, "LON"}, {"IN1L", NULL, "Mic Jack"}, + {"IN1R", NULL, "Mic Jack"}, +}; + +static void harmony_mic_control(struct snd_soc_codec *codec) +{ + struct snd_soc_card *card = codec->card; + struct tegra_harmony *harmony = snd_soc_card_get_drvdata(card); + struct harmony_audio_platform_data *pdata = harmony->pdata; + int int_mic_en = harmony->mic_selection == MIC_SELECTION_INT_MIC; + int ext_mic_en = harmony->mic_selection == MIC_SELECTION_EXT_MIC; + + gpio_set_value_cansleep(pdata->gpio_int_mic_en, int_mic_en); + /* ext_mic_en is active low */ + gpio_set_value_cansleep(pdata->gpio_ext_mic_en, !ext_mic_en); +} + +static int harmony_get_mic_selection(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); + struct snd_soc_card *card = codec->card; + struct tegra_harmony *harmony = snd_soc_card_get_drvdata(card); + + ucontrol->value.integer.value[0] = harmony->mic_selection; + + return 0; +} + +static int harmony_set_mic_selection(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); + struct snd_soc_card *card = codec->card; + struct tegra_harmony *harmony = snd_soc_card_get_drvdata(card); + + if (harmony->mic_selection == ucontrol->value.integer.value[0]) + return 0; + + harmony->mic_selection = ucontrol->value.integer.value[0]; + harmony_mic_control(codec); + + return 1; +} + +static const char * const mic_selection_names[] = {"Off", "External", + "Internal"}; +static const struct soc_enum harmony_enum[] = { + SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(mic_selection_names), + mic_selection_names), };
static const struct snd_kcontrol_new harmony_controls[] = { SOC_DAPM_PIN_SWITCH("Int Spk"), + SOC_ENUM_EXT("Mic Selection", harmony_enum[0], + harmony_get_mic_selection, harmony_set_mic_selection), };
static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd) @@ -201,10 +261,28 @@ static int harmony_asoc_init(struct snd_soc_pcm_runtime *rtd) dev_err(card->dev, "cannot get spkr_en gpio\n"); return ret; } - harmony->gpio_spkr_en_requested = 1; + harmony->gpio_requested |= GPIO_SPKR_EN;
gpio_direction_output(pdata->gpio_spkr_en, 0);
+ ret = gpio_request(pdata->gpio_int_mic_en, "int_mic_en"); + if (ret) { + dev_err(card->dev, "cannot get int_mic_en gpio\n"); + return ret; + } + harmony->gpio_requested |= GPIO_INT_MIC_EN; + + gpio_direction_output(pdata->gpio_int_mic_en, 1); + + ret = gpio_request(pdata->gpio_ext_mic_en, "ext_mic_en"); + if (ret) { + dev_err(card->dev, "cannot get ext_mic_en gpio\n"); + return ret; + } + harmony->gpio_requested |= GPIO_EXT_MIC_EN; + + gpio_direction_output(pdata->gpio_ext_mic_en, 0); + /* Not connected */ snd_soc_dapm_nc_pin(dapm, "IN3L"); snd_soc_dapm_nc_pin(dapm, "IN3R"); @@ -326,7 +404,11 @@ static int __devexit tegra_snd_harmony_remove(struct platform_device *pdev)
tegra_asoc_utils_fini(&harmony->util_data);
- if (harmony->gpio_spkr_en_requested) + if (harmony->gpio_requested & GPIO_EXT_MIC_EN) + gpio_free(pdata->gpio_ext_mic_en); + if (harmony->gpio_requested & GPIO_INT_MIC_EN) + gpio_free(pdata->gpio_int_mic_en); + if (harmony->gpio_requested & GPIO_SPKR_EN) gpio_free(pdata->gpio_spkr_en);
kfree(harmony);
On Thu, Feb 03, 2011 at 01:56:18PM -0700, Stephen Warren wrote:
At present, I'm not sure how best to resolve this. Downstream drivers directly tweaked the wm8903's registers so that both ADCs processed the same input channel.
The CODEC driver needs to have support for routing the left and right channels like things like the WM8993 do.
{"IN1L", NULL, "Mic Jack"},
- {"IN1R", NULL, "Mic Jack"},
This looks odd - I'd expect to see separate widgets for the internal microphone rather than using the same widget. This would greatly simplify the driver code without really impacting the level of configuration applications need to do (and allowing them to use both simultaneously if they want to).
+static int harmony_set_mic_selection(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
- struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
- struct snd_soc_card *card = codec->card;
- struct tegra_harmony *harmony = snd_soc_card_get_drvdata(card);
- if (harmony->mic_selection == ucontrol->value.integer.value[0])
return 0;- harmony->mic_selection = ucontrol->value.integer.value[0];
- harmony_mic_control(codec);
This should be locking the CODEC mutex while doing the update.
+static const struct soc_enum harmony_enum[] = {
- SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(mic_selection_names),
mic_selection_names),};
Don't create arrays of enums, declare individual varaibles and reference them...
- SOC_ENUM_EXT("Mic Selection", harmony_enum[0],
harmony_get_mic_selection, harmony_set_mic_selection),
...as it makes the references to them much easier to follow and less error prone.
Mark Brown wrote at Thursday, February 03, 2011 3:53 PM:
On Thu, Feb 03, 2011 at 01:56:18PM -0700, Stephen Warren wrote:
At present, I'm not sure how best to resolve this. Downstream drivers directly tweaked the wm8903's registers so that both ADCs processed the same input channel.
The CODEC driver needs to have support for routing the left and right channels like things like the WM8993 do.
{"IN1L", NULL, "Mic Jack"},
- {"IN1R", NULL, "Mic Jack"},
This looks odd - I'd expect to see separate widgets for the internal microphone rather than using the same widget.
Yes, separate widgets would make sense.
However, I wonder how this interacts with mic detection using micbias current. There is only one mic bias signal, routed to a subset of the mics using the two per-mic enable GPIOs. Should I just hook up the mic bias detection to both jacks somehow, or is the only option to just punt on jack detection?
There are no physical plug detection mechanisms on this board.
This would greatly simplify the driver code without really impacting the level of configuration applications need to do (and allowing them to use both simultaneously if they want to).
I guess you could, although even with L/R routing implemented for the single-mic case, I think the only option for dual-mic would be to capture each mic on its own channel, which seems like an unlikely use case to me. But who knows, maybe it could be useful for e.g. noise-cancelling in SW?
On Fri, Feb 04, 2011 at 09:20:26AM -0800, Stephen Warren wrote:
However, I wonder how this interacts with mic detection using micbias current. There is only one mic bias signal, routed to a subset of the mics using the two per-mic enable GPIOs. Should I just hook up the mic bias detection to both jacks somehow, or is the only option to just punt on jack detection?
There are no physical plug detection mechanisms on this board.
That's a fairly unusual hardware design, it really needs explaining in the code - it wasn't at all obvious that you were controlling a FET reading the patch (and I'm still not entirely clear how exactly things are wired up). The simplest way to handle this is probably with a conditional DAPM route (look at how WM8994 handles SYSCLK for an example of this), though you'll need to faff around a bit.
Ideally you want the micbias to be enabled all the time for detection of jack insertion, only flipping the bias over to the internal mic when the internal mic is being used for recording, and also disable the detection when the internal microphone is in use. I'm not sure that this is worth the bother.
This would greatly simplify the driver code without really impacting the level of configuration applications need to do (and allowing them to use both simultaneously if they want to).
I guess you could, although even with L/R routing implemented for the single-mic case, I think the only option for dual-mic would be to capture each mic on its own channel, which seems like an unlikely use case to me. But who knows, maybe it could be useful for e.g. noise-cancelling in SW?
Most likely would seem to be using the mic jack as a line input for some purpose and combining it with the results of the internal microphone.
Mark Brown wrote at Monday, February 07, 2011 4:13 AM:
On Fri, Feb 04, 2011 at 09:20:26AM -0800, Stephen Warren wrote:
However, I wonder how this interacts with mic detection using micbias current. There is only one mic bias signal, routed to a subset of the mics using the two per-mic enable GPIOs. Should I just hook up the mic bias detection to both jacks somehow, or is the only option to just punt on jack detection?
There are no physical plug detection mechanisms on this board.
That's a fairly unusual hardware design, it really needs explaining in the code - it wasn't at all obvious that you were controlling a FET reading the patch (and I'm still not entirely clear how exactly things are wired up). The simplest way to handle this is probably with a conditional DAPM route (look at how WM8994 handles SYSCLK for an example of this), though you'll need to faff around a bit.
Ideally you want the micbias to be enabled all the time for detection of jack insertion, only flipping the bias over to the internal mic when the internal mic is being used for recording, and also disable the detection when the internal microphone is in use. I'm not sure that this is worth the bother.
Maybe I should just not implement mic detection for Harmony due to these issues? In which case, I'd just let mic bias be enabled whenever capture was active, just like the speaker enable GPIO.
Either way, I suppose I should add two explicit mic widgets, each directly driving the appropriate GPIO, so that the user can select which mic(s) actually get used for capture.
Does that sound reasonable?
Thansk.
On Tue, Feb 08, 2011 at 12:34:09PM -0800, Stephen Warren wrote:
Maybe I should just not implement mic detection for Harmony due to these issues? In which case, I'd just let mic bias be enabled whenever capture was active, just like the speaker enable GPIO.
It certainly seems way more trouble than it's worth: as soon as the on board mic is used the jack detection will get confused as it'll see it. Of course, the internal mic is actually plugged in to a header on the main board so it may be absent too (I believe a lot of units were shipped without a case...) which means detecting the internal mic might actually be useful :)
Either way, I suppose I should add two explicit mic widgets, each directly driving the appropriate GPIO, so that the user can select which mic(s) actually get used for capture.
Does that sound reasonable?
I think that's the best approach. If someone wants to do mic detection they can always add it later.
Mark Brown wrote at Tuesday, February 08, 2011 1:59 PM:
On Tue, Feb 08, 2011 at 12:34:09PM -0800, Stephen Warren wrote:
Maybe I should just not implement mic detection for Harmony due to these issues? In which case, I'd just let mic bias be enabled whenever capture was active, just like the speaker enable GPIO.
It certainly seems way more trouble than it's worth: as soon as the on board mic is used the jack detection will get confused as it'll see it. Of course, the internal mic is actually plugged in to a header on the main board so it may be absent too (I believe a lot of units were shipped without a case...) which means detecting the internal mic might actually be useful :)
Well, one other option is to completely ignore (and disable) the internal mic, and just support the external mic (jack). Then, mic detection could be implemented. The advantage here is similarity to other boards without the mic bias sharing issue. Plus, the jack is probably far more useful than the internal mic on this board:
I honestly don't know how many boards (if any) actually have anything plugged into the internal mic header on the board. Mine certainly didn't originally; I used a PC speaker I stole from somewhere else as the internal mic;-)
On Tue, Feb 08, 2011 at 03:32:00PM -0800, Stephen Warren wrote:
Well, one other option is to completely ignore (and disable) the internal mic, and just support the external mic (jack). Then, mic detection could be implemented. The advantage here is similarity to other boards without the mic bias sharing issue. Plus, the jack is probably far more useful than the internal mic on this board:
I honestly don't know how many boards (if any) actually have anything plugged into the internal mic header on the board. Mine certainly didn't originally; I used a PC speaker I stole from somewhere else as the internal mic;-)
That's certainly a sensible option if we don't know that many systems were shipped with the internal mic connected. We could do something like what the AT91SAM9G20 platform does with ifdefed code for the mic (in that case there's no mic support in the hardware but the CODEC is in a low density package so it's relatively common for users to lift pins to flying wire in a mic) I guess.
On Thu, Feb 03, 2011 at 01:56:13PM -0700, Stephen Warren wrote:
+static struct snd_soc_jack harmony_hp_jack;
Since you've changed to using a platform device you should really be dynamically allocating this I guess. But this isn't actually a practical problem so not worth caring about.
Mark Brown wrote at Thursday, February 03, 2011 3:36 PM:
On Thu, Feb 03, 2011 at 01:56:13PM -0700, Stephen Warren wrote:
+static struct snd_soc_jack harmony_hp_jack;
Since you've changed to using a platform device you should really be dynamically allocating this I guess. But this isn't actually a practical problem so not worth caring about.
Uggh. The code may as well be consistent. I'll fix this up and resubmit if you haven't already applied it.
On Thu, Feb 03, 2011 at 03:25:04PM -0800, Stephen Warren wrote:
Uggh. The code may as well be consistent. I'll fix this up and resubmit if you haven't already applied it.
Except when he's on holiday or otherwise unavailable I won't generally apply anything unless it's an emergency bugfix until Liam has also reviewed it.
Stephen Warren wrote at Thursday, February 03, 2011 4:25 PM:
Mark Brown wrote at Thursday, February 03, 2011 3:36 PM:
On Thu, Feb 03, 2011 at 01:56:13PM -0700, Stephen Warren wrote:
+static struct snd_soc_jack harmony_hp_jack;
Since you've changed to using a platform device you should really be dynamically allocating this I guess. But this isn't actually a practical problem so not worth caring about.
Uggh. The code may as well be consistent. I'll fix this up and resubmit if you haven't already applied it.
Hmm. Looking at this a bit more, solving it fully is kinda nasty; I'd have to move not only that jack definition, but also all the pins and gpios into struct tegra_harmony, since snd_soc_jack_add_{pins,gpios} modify all of those structures. But, there would still have to be static const "template" copies of those data structures to initialize the copies in struct tegra_harmony.
The same thing then applies to some of the subsequent changes, for the mic jacks etc.
That seems like a lot of overhead, both code-wise, and runtime space-wise, to solve a problem that as you mention isn't a practical concern.
Stephen Warren wrote at Friday, February 04, 2011 1:32 PM:
Stephen Warren wrote at Thursday, February 03, 2011 4:25 PM:
Mark Brown wrote at Thursday, February 03, 2011 3:36 PM:
On Thu, Feb 03, 2011 at 01:56:13PM -0700, Stephen Warren wrote:
+static struct snd_soc_jack harmony_hp_jack;
Since you've changed to using a platform device you should really be dynamically allocating this I guess. But this isn't actually a practical problem so not worth caring about.
Uggh. The code may as well be consistent. I'll fix this up and resubmit if you haven't already applied it.
Hmm. Looking at this a bit more, solving it fully is kinda nasty; I'd have to move not only that jack definition, but also all the pins and gpios into struct tegra_harmony, since snd_soc_jack_add_{pins,gpios} modify all of those structures. But, there would still have to be static const "template" copies of those data structures to initialize the copies in struct tegra_harmony.
The same thing then applies to some of the subsequent changes, for the mic jacks etc.
That seems like a lot of overhead, both code-wise, and runtime space-wise, to solve a problem that as you mention isn't a practical concern.
Mark, I'm happy to go either way on this; the SW engineer in me doesn't like non-const globals and inconsistency with the rest of the state data, but equally fixing this seems like it would bloat the code and might make it less readable.
Let me know which way you'd prefer it to go.
Thanks.
-- nvpublic
On Tue, Feb 08, 2011 at 12:28:48PM -0800, Stephen Warren wrote:
Mark, I'm happy to go either way on this; the SW engineer in me doesn't like non-const globals and inconsistency with the rest of the state data, but equally fixing this seems like it would bloat the code and might make it less readable.
Let me know which way you'd prefer it to go.
I think your current patch is fine. Liam?
On Tue, 2011-02-08 at 20:59 +0000, Mark Brown wrote:
On Tue, Feb 08, 2011 at 12:28:48PM -0800, Stephen Warren wrote:
Mark, I'm happy to go either way on this; the SW engineer in me doesn't like non-const globals and inconsistency with the rest of the state data, but equally fixing this seems like it would bloat the code and might make it less readable.
Let me know which way you'd prefer it to go.
I think your current patch is fine. Liam?
Yeah, lets go with this.
Acked-by: Liam Girdwood lrg@slimlogic.co.uk
On Thu, Feb 03, 2011 at 01:56:13PM -0700, Stephen Warren wrote:
Signed-off-by: Stephen Warren swarren@nvidia.com
Applied, thanks.
participants (3)
-
Liam Girdwood -
Mark Brown -
Stephen Warren