All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ALSA: hda - Add snd_hda_jack_detect_state() helper function
@ 2013-07-19 15:06 Takashi Iwai
  2013-07-20 18:39 ` David Henningsson
  0 siblings, 1 reply; 3+ messages in thread
From: Takashi Iwai @ 2013-07-19 15:06 UTC (permalink / raw)
  To: alsa-devel

snd_hda_jack_detect() function returns a boolean value for a jack
plugged in or not, but it also returns always true when the
corresponding pin is phantom (i.e. fixed).  This is OK in most cases,
but it makes the generic parser misbehaving about the auto-mute or
auto-mic switching, e.g. when one of headphone pins is a fixed.
Namely, the driver decides whether to mute the speaker or not, just
depending on the headphone plug state: if one of the headphone jacks
is seen as active, then the speaker is muted.  Thus this will result
always in the muted speaker output.

So, the problem is the function returns a boolean, after all, although
we need to think of "phantom" jack.  Now a new function,
snd_hda_jack_detect_state() is introduced to return these tristates.
The generic parser uses this function for checking the headphone or
mic jack states.

Meanwhile, the behavior of snd_hda_jack_detect() is kept as is, for
keeping compatibility in other driver codes.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_generic.c |  8 +++++---
 sound/pci/hda/hda_jack.c    | 18 ++++++++++++------
 sound/pci/hda/hda_jack.h    | 13 ++++++++++++-
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index 8e77cbb..f5c2d1f 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -3724,7 +3724,8 @@ static int mux_select(struct hda_codec *codec, unsigned int adc_idx,
 /* check each pin in the given array; returns true if any of them is plugged */
 static bool detect_jacks(struct hda_codec *codec, int num_pins, hda_nid_t *pins)
 {
-	int i, present = 0;
+	int i;
+	bool present = false;
 
 	for (i = 0; i < num_pins; i++) {
 		hda_nid_t nid = pins[i];
@@ -3733,7 +3734,8 @@ static bool detect_jacks(struct hda_codec *codec, int num_pins, hda_nid_t *pins)
 		/* don't detect pins retasked as inputs */
 		if (snd_hda_codec_get_pin_target(codec, nid) & AC_PINCTL_IN_EN)
 			continue;
-		present |= snd_hda_jack_detect(codec, nid);
+		if (snd_hda_jack_detect_state(codec, nid) == HDA_JACK_PRESENT)
+			present = true;
 	}
 	return present;
 }
@@ -3887,7 +3889,7 @@ void snd_hda_gen_mic_autoswitch(struct hda_codec *codec, struct hda_jack_tbl *ja
 		/* don't detect pins retasked as outputs */
 		if (snd_hda_codec_get_pin_target(codec, pin) & AC_PINCTL_OUT_EN)
 			continue;
-		if (snd_hda_jack_detect(codec, pin)) {
+		if (snd_hda_jack_detect_state(codec, pin) == HDA_JACK_PRESENT) {
 			mux_select(codec, 0, spec->am_entry[i].idx);
 			return;
 		}
diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c
index 3fd2973..dc93761 100644
--- a/sound/pci/hda/hda_jack.c
+++ b/sound/pci/hda/hda_jack.c
@@ -194,18 +194,24 @@ u32 snd_hda_pin_sense(struct hda_codec *codec, hda_nid_t nid)
 EXPORT_SYMBOL_HDA(snd_hda_pin_sense);
 
 /**
- * snd_hda_jack_detect - query pin Presence Detect status
+ * snd_hda_jack_detect_state - query pin Presence Detect status
  * @codec: the CODEC to sense
  * @nid: the pin NID to sense
  *
- * Query and return the pin's Presence Detect status.
+ * Query and return the pin's Presence Detect status, as either
+ * HDA_JACK_NOT_PRESENT, HDA_JACK_PRESENT or HDA_JACK_PHANTOM.
  */
-int snd_hda_jack_detect(struct hda_codec *codec, hda_nid_t nid)
+int snd_hda_jack_detect_state(struct hda_codec *codec, hda_nid_t nid)
 {
-	u32 sense = snd_hda_pin_sense(codec, nid);
-	return get_jack_plug_state(sense);
+	struct hda_jack_tbl *jack = snd_hda_jack_tbl_get(codec, nid);
+	if (jack && jack->phantom_jack)
+		return HDA_JACK_PHANTOM;
+	else if (snd_hda_pin_sense(codec, nid) & AC_PINSENSE_PRESENCE)
+		return HDA_JACK_PRESENT;
+	else
+		return HDA_JACK_NOT_PRESENT;
 }
-EXPORT_SYMBOL_HDA(snd_hda_jack_detect);
+EXPORT_SYMBOL_HDA(snd_hda_jack_detect_state);
 
 /**
  * snd_hda_jack_detect_enable - enable the jack-detection
diff --git a/sound/pci/hda/hda_jack.h b/sound/pci/hda/hda_jack.h
index ec12abd..379420c 100644
--- a/sound/pci/hda/hda_jack.h
+++ b/sound/pci/hda/hda_jack.h
@@ -75,7 +75,18 @@ int snd_hda_jack_set_gating_jack(struct hda_codec *codec, hda_nid_t gated_nid,
 				 hda_nid_t gating_nid);
 
 u32 snd_hda_pin_sense(struct hda_codec *codec, hda_nid_t nid);
-int snd_hda_jack_detect(struct hda_codec *codec, hda_nid_t nid);
+
+/* the jack state returned from snd_hda_jack_detect_state() */
+enum {
+	HDA_JACK_NOT_PRESENT, HDA_JACK_PRESENT, HDA_JACK_PHANTOM,
+};
+
+int snd_hda_jack_detect_state(struct hda_codec *codec, hda_nid_t nid);
+
+static inline bool snd_hda_jack_detect(struct hda_codec *codec, hda_nid_t nid)
+{
+	return snd_hda_jack_detect_state(codec, nid) != HDA_JACK_NOT_PRESENT;
+}
 
 bool is_jack_detectable(struct hda_codec *codec, hda_nid_t nid);
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] ALSA: hda - Add snd_hda_jack_detect_state() helper function
  2013-07-19 15:06 [PATCH] ALSA: hda - Add snd_hda_jack_detect_state() helper function Takashi Iwai
@ 2013-07-20 18:39 ` David Henningsson
  2013-07-21  9:54   ` Takashi Iwai
  0 siblings, 1 reply; 3+ messages in thread
From: David Henningsson @ 2013-07-20 18:39 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel

On 07/19/2013 05:06 PM, Takashi Iwai wrote:
> snd_hda_jack_detect() function returns a boolean value for a jack
> plugged in or not, but it also returns always true when the
> corresponding pin is phantom (i.e. fixed).  This is OK in most cases,
> but it makes the generic parser misbehaving about the auto-mute or
> auto-mic switching, e.g. when one of headphone pins is a fixed.
> Namely, the driver decides whether to mute the speaker or not, just
> depending on the headphone plug state: if one of the headphone jacks
> is seen as active, then the speaker is muted.  Thus this will result
> always in the muted speaker output.
>
> So, the problem is the function returns a boolean, after all, although
> we need to think of "phantom" jack.  Now a new function,
> snd_hda_jack_detect_state() is introduced to return these tristates.
> The generic parser uses this function for checking the headphone or
> mic jack states.
>
> Meanwhile, the behavior of snd_hda_jack_detect() is kept as is, for
> keeping compatibility in other driver codes.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Acked-by: David Henningsson <david.henningsson@canonical.com>

Although I doubt this is needed on the input side. If we have more than 
one input source that is fixed/phantom, we should always have a capture 
switch rather than auto-mic switch, right?

> ---
>   sound/pci/hda/hda_generic.c |  8 +++++---
>   sound/pci/hda/hda_jack.c    | 18 ++++++++++++------
>   sound/pci/hda/hda_jack.h    | 13 ++++++++++++-
>   3 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> index 8e77cbb..f5c2d1f 100644
> --- a/sound/pci/hda/hda_generic.c
> +++ b/sound/pci/hda/hda_generic.c
> @@ -3724,7 +3724,8 @@ static int mux_select(struct hda_codec *codec, unsigned int adc_idx,
>   /* check each pin in the given array; returns true if any of them is plugged */
>   static bool detect_jacks(struct hda_codec *codec, int num_pins, hda_nid_t *pins)
>   {
> -	int i, present = 0;
> +	int i;
> +	bool present = false;
>
>   	for (i = 0; i < num_pins; i++) {
>   		hda_nid_t nid = pins[i];
> @@ -3733,7 +3734,8 @@ static bool detect_jacks(struct hda_codec *codec, int num_pins, hda_nid_t *pins)
>   		/* don't detect pins retasked as inputs */
>   		if (snd_hda_codec_get_pin_target(codec, nid) & AC_PINCTL_IN_EN)
>   			continue;
> -		present |= snd_hda_jack_detect(codec, nid);
> +		if (snd_hda_jack_detect_state(codec, nid) == HDA_JACK_PRESENT)
> +			present = true;
>   	}
>   	return present;
>   }
> @@ -3887,7 +3889,7 @@ void snd_hda_gen_mic_autoswitch(struct hda_codec *codec, struct hda_jack_tbl *ja
>   		/* don't detect pins retasked as outputs */
>   		if (snd_hda_codec_get_pin_target(codec, pin) & AC_PINCTL_OUT_EN)
>   			continue;
> -		if (snd_hda_jack_detect(codec, pin)) {
> +		if (snd_hda_jack_detect_state(codec, pin) == HDA_JACK_PRESENT) {
>   			mux_select(codec, 0, spec->am_entry[i].idx);
>   			return;
>   		}
> diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c
> index 3fd2973..dc93761 100644
> --- a/sound/pci/hda/hda_jack.c
> +++ b/sound/pci/hda/hda_jack.c
> @@ -194,18 +194,24 @@ u32 snd_hda_pin_sense(struct hda_codec *codec, hda_nid_t nid)
>   EXPORT_SYMBOL_HDA(snd_hda_pin_sense);
>
>   /**
> - * snd_hda_jack_detect - query pin Presence Detect status
> + * snd_hda_jack_detect_state - query pin Presence Detect status
>    * @codec: the CODEC to sense
>    * @nid: the pin NID to sense
>    *
> - * Query and return the pin's Presence Detect status.
> + * Query and return the pin's Presence Detect status, as either
> + * HDA_JACK_NOT_PRESENT, HDA_JACK_PRESENT or HDA_JACK_PHANTOM.
>    */
> -int snd_hda_jack_detect(struct hda_codec *codec, hda_nid_t nid)
> +int snd_hda_jack_detect_state(struct hda_codec *codec, hda_nid_t nid)
>   {
> -	u32 sense = snd_hda_pin_sense(codec, nid);
> -	return get_jack_plug_state(sense);
> +	struct hda_jack_tbl *jack = snd_hda_jack_tbl_get(codec, nid);
> +	if (jack && jack->phantom_jack)
> +		return HDA_JACK_PHANTOM;
> +	else if (snd_hda_pin_sense(codec, nid) & AC_PINSENSE_PRESENCE)
> +		return HDA_JACK_PRESENT;
> +	else
> +		return HDA_JACK_NOT_PRESENT;
>   }
> -EXPORT_SYMBOL_HDA(snd_hda_jack_detect);
> +EXPORT_SYMBOL_HDA(snd_hda_jack_detect_state);
>
>   /**
>    * snd_hda_jack_detect_enable - enable the jack-detection
> diff --git a/sound/pci/hda/hda_jack.h b/sound/pci/hda/hda_jack.h
> index ec12abd..379420c 100644
> --- a/sound/pci/hda/hda_jack.h
> +++ b/sound/pci/hda/hda_jack.h
> @@ -75,7 +75,18 @@ int snd_hda_jack_set_gating_jack(struct hda_codec *codec, hda_nid_t gated_nid,
>   				 hda_nid_t gating_nid);
>
>   u32 snd_hda_pin_sense(struct hda_codec *codec, hda_nid_t nid);
> -int snd_hda_jack_detect(struct hda_codec *codec, hda_nid_t nid);
> +
> +/* the jack state returned from snd_hda_jack_detect_state() */
> +enum {
> +	HDA_JACK_NOT_PRESENT, HDA_JACK_PRESENT, HDA_JACK_PHANTOM,
> +};
> +
> +int snd_hda_jack_detect_state(struct hda_codec *codec, hda_nid_t nid);
> +
> +static inline bool snd_hda_jack_detect(struct hda_codec *codec, hda_nid_t nid)
> +{
> +	return snd_hda_jack_detect_state(codec, nid) != HDA_JACK_NOT_PRESENT;
> +}
>
>   bool is_jack_detectable(struct hda_codec *codec, hda_nid_t nid);
>
>



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] ALSA: hda - Add snd_hda_jack_detect_state() helper function
  2013-07-20 18:39 ` David Henningsson
@ 2013-07-21  9:54   ` Takashi Iwai
  0 siblings, 0 replies; 3+ messages in thread
From: Takashi Iwai @ 2013-07-21  9:54 UTC (permalink / raw)
  To: David Henningsson; +Cc: alsa-devel

At Sat, 20 Jul 2013 20:39:56 +0200,
David Henningsson wrote:
> 
> On 07/19/2013 05:06 PM, Takashi Iwai wrote:
> > snd_hda_jack_detect() function returns a boolean value for a jack
> > plugged in or not, but it also returns always true when the
> > corresponding pin is phantom (i.e. fixed).  This is OK in most cases,
> > but it makes the generic parser misbehaving about the auto-mute or
> > auto-mic switching, e.g. when one of headphone pins is a fixed.
> > Namely, the driver decides whether to mute the speaker or not, just
> > depending on the headphone plug state: if one of the headphone jacks
> > is seen as active, then the speaker is muted.  Thus this will result
> > always in the muted speaker output.
> >
> > So, the problem is the function returns a boolean, after all, although
> > we need to think of "phantom" jack.  Now a new function,
> > snd_hda_jack_detect_state() is introduced to return these tristates.
> > The generic parser uses this function for checking the headphone or
> > mic jack states.
> >
> > Meanwhile, the behavior of snd_hda_jack_detect() is kept as is, for
> > keeping compatibility in other driver codes.
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> Acked-by: David Henningsson <david.henningsson@canonical.com>
> 
> Although I doubt this is needed on the input side. If we have more than 
> one input source that is fixed/phantom, we should always have a capture 
> switch rather than auto-mic switch, right?

It'd be better to use the explicit HDA_JACK_PRESENT check, yes, but
in theory this shouldn't be an issue for the current auto-mic
switching scheme because the capture source is mutual exclusive while
the output can be multiplexing.


thanks,

Takashi

> 
> > ---
> >   sound/pci/hda/hda_generic.c |  8 +++++---
> >   sound/pci/hda/hda_jack.c    | 18 ++++++++++++------
> >   sound/pci/hda/hda_jack.h    | 13 ++++++++++++-
> >   3 files changed, 29 insertions(+), 10 deletions(-)
> >
> > diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> > index 8e77cbb..f5c2d1f 100644
> > --- a/sound/pci/hda/hda_generic.c
> > +++ b/sound/pci/hda/hda_generic.c
> > @@ -3724,7 +3724,8 @@ static int mux_select(struct hda_codec *codec, unsigned int adc_idx,
> >   /* check each pin in the given array; returns true if any of them is plugged */
> >   static bool detect_jacks(struct hda_codec *codec, int num_pins, hda_nid_t *pins)
> >   {
> > -	int i, present = 0;
> > +	int i;
> > +	bool present = false;
> >
> >   	for (i = 0; i < num_pins; i++) {
> >   		hda_nid_t nid = pins[i];
> > @@ -3733,7 +3734,8 @@ static bool detect_jacks(struct hda_codec *codec, int num_pins, hda_nid_t *pins)
> >   		/* don't detect pins retasked as inputs */
> >   		if (snd_hda_codec_get_pin_target(codec, nid) & AC_PINCTL_IN_EN)
> >   			continue;
> > -		present |= snd_hda_jack_detect(codec, nid);
> > +		if (snd_hda_jack_detect_state(codec, nid) == HDA_JACK_PRESENT)
> > +			present = true;
> >   	}
> >   	return present;
> >   }
> > @@ -3887,7 +3889,7 @@ void snd_hda_gen_mic_autoswitch(struct hda_codec *codec, struct hda_jack_tbl *ja
> >   		/* don't detect pins retasked as outputs */
> >   		if (snd_hda_codec_get_pin_target(codec, pin) & AC_PINCTL_OUT_EN)
> >   			continue;
> > -		if (snd_hda_jack_detect(codec, pin)) {
> > +		if (snd_hda_jack_detect_state(codec, pin) == HDA_JACK_PRESENT) {
> >   			mux_select(codec, 0, spec->am_entry[i].idx);
> >   			return;
> >   		}
> > diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c
> > index 3fd2973..dc93761 100644
> > --- a/sound/pci/hda/hda_jack.c
> > +++ b/sound/pci/hda/hda_jack.c
> > @@ -194,18 +194,24 @@ u32 snd_hda_pin_sense(struct hda_codec *codec, hda_nid_t nid)
> >   EXPORT_SYMBOL_HDA(snd_hda_pin_sense);
> >
> >   /**
> > - * snd_hda_jack_detect - query pin Presence Detect status
> > + * snd_hda_jack_detect_state - query pin Presence Detect status
> >    * @codec: the CODEC to sense
> >    * @nid: the pin NID to sense
> >    *
> > - * Query and return the pin's Presence Detect status.
> > + * Query and return the pin's Presence Detect status, as either
> > + * HDA_JACK_NOT_PRESENT, HDA_JACK_PRESENT or HDA_JACK_PHANTOM.
> >    */
> > -int snd_hda_jack_detect(struct hda_codec *codec, hda_nid_t nid)
> > +int snd_hda_jack_detect_state(struct hda_codec *codec, hda_nid_t nid)
> >   {
> > -	u32 sense = snd_hda_pin_sense(codec, nid);
> > -	return get_jack_plug_state(sense);
> > +	struct hda_jack_tbl *jack = snd_hda_jack_tbl_get(codec, nid);
> > +	if (jack && jack->phantom_jack)
> > +		return HDA_JACK_PHANTOM;
> > +	else if (snd_hda_pin_sense(codec, nid) & AC_PINSENSE_PRESENCE)
> > +		return HDA_JACK_PRESENT;
> > +	else
> > +		return HDA_JACK_NOT_PRESENT;
> >   }
> > -EXPORT_SYMBOL_HDA(snd_hda_jack_detect);
> > +EXPORT_SYMBOL_HDA(snd_hda_jack_detect_state);
> >
> >   /**
> >    * snd_hda_jack_detect_enable - enable the jack-detection
> > diff --git a/sound/pci/hda/hda_jack.h b/sound/pci/hda/hda_jack.h
> > index ec12abd..379420c 100644
> > --- a/sound/pci/hda/hda_jack.h
> > +++ b/sound/pci/hda/hda_jack.h
> > @@ -75,7 +75,18 @@ int snd_hda_jack_set_gating_jack(struct hda_codec *codec, hda_nid_t gated_nid,
> >   				 hda_nid_t gating_nid);
> >
> >   u32 snd_hda_pin_sense(struct hda_codec *codec, hda_nid_t nid);
> > -int snd_hda_jack_detect(struct hda_codec *codec, hda_nid_t nid);
> > +
> > +/* the jack state returned from snd_hda_jack_detect_state() */
> > +enum {
> > +	HDA_JACK_NOT_PRESENT, HDA_JACK_PRESENT, HDA_JACK_PHANTOM,
> > +};
> > +
> > +int snd_hda_jack_detect_state(struct hda_codec *codec, hda_nid_t nid);
> > +
> > +static inline bool snd_hda_jack_detect(struct hda_codec *codec, hda_nid_t nid)
> > +{
> > +	return snd_hda_jack_detect_state(codec, nid) != HDA_JACK_NOT_PRESENT;
> > +}
> >
> >   bool is_jack_detectable(struct hda_codec *codec, hda_nid_t nid);
> >
> >
> 
> 
> 
> -- 
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-07-21  9:53 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-19 15:06 [PATCH] ALSA: hda - Add snd_hda_jack_detect_state() helper function Takashi Iwai
2013-07-20 18:39 ` David Henningsson
2013-07-21  9:54   ` Takashi Iwai

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.