Re: [PATCH 1/6] ASoC: Tegra: Harmony: Add headphone jack detection

Re: [PATCH 1/6] ASoC: Tegra: Harmony: Add headphone jack detection

[Mark Brown]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/6] ASoC: Tegra: Harmony: Add headphone jack detection

[Stephen Warren]

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.

-- 
nvpublic

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] ASoC: Tegra: Harmony: Add headphone jack detection

[Mark Brown]

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.

RE: [PATCH 1/6] ASoC: Tegra: Harmony: Add headphone jack detection

[Stephen Warren]

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.

-- 
nvpublic

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [alsa-devel] [PATCH 1/6] ASoC: Tegra: Harmony: Add headphone jack detection

[Stephen Warren]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH 1/6] ASoC: Tegra: Harmony: Add headphone jack detection

[Mark Brown]

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?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH 1/6] ASoC: Tegra: Harmony: Add headphone jack detection

[Liam Girdwood]

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-kDsPt+C1G03kYMGBc/C6ZA@public.gmane.org>
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH 1/6] ASoC: Tegra: Harmony: Add headphone jack detection

[Mark Brown]

On Thu, Feb 03, 2011 at 01:56:13PM -0700, Stephen Warren wrote:
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] ASoC: WM8903: Fix mic detection issues

[Mark Brown]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] ASoC: Tegra: Harmony: Implement mic detection

[Mark Brown]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] ASoC: Tegra: Harmony: Support both int and ext mics

[Mark Brown]

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.

RE: [PATCH 6/6] ASoC: Tegra: Harmony: Support both int and ext mics

[Stephen Warren]

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?

-- 
nvpublic

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH 6/6] ASoC: Tegra: Harmony: Support both int and ext mics

[Mark Brown]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [alsa-devel] [PATCH 6/6] ASoC: Tegra: Harmony: Support both int and ext mics

[Stephen Warren]

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.

-- 
nvpublic

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH 6/6] ASoC: Tegra: Harmony: Support both int and ext mics

[Mark Brown]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [alsa-devel] [PATCH 6/6] ASoC: Tegra: Harmony: Support both int and ext mics

[Stephen Warren]

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;-)

-- 
nvpublic

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH 6/6] ASoC: Tegra: Harmony: Support both int and ext mics

[Mark Brown]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] ASoC: Tegra: Harmony: Add switch control for speaker

[Mark Brown]

On Thu, Feb 03, 2011 at 01:56:14PM -0700, Stephen Warren wrote:
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

This looks good but won't apply without patch 1 so please resubmit.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH 2/6] ASoC: Tegra: Harmony: Add switch control for speaker

[Mark Brown]

On Thu, Feb 03, 2011 at 01:56:14PM -0700, Stephen Warren wrote:
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] ASoC: Harmony: Call snd_soc_dapm_nc_pin

[Mark Brown]

On Thu, Feb 03, 2011 at 01:56:17PM -0700, Stephen Warren wrote:
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Similarly here.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html