Skip to contentSkip to conversation

tlv320aic3x: headset/button press support

PATCH In review Daniel Mack 3 participants 6 messages Nov 26, 2008 – Dec 3, 2008

Subsystems (3)
Sound ASoC Texas Instruments audio (ASoC/HDA) drivers
lore
Share & tools
lore

Daniel Mack

author
#0 lore raw

PATCH 1/1 tlv320aic3x: headset/button press support

Hi,

This patch adds a function to setup headset and button press detection modes.

Also, aic3x_headset_detected() returns the value from the current state's register rather than the interrupt cause flag so it works without an interrupt as well.

Best regards,
Daniel

Added a function to setup headset and button press detection modes. Also, aic3x_headset_detected() returns the value from the current state's register rather than the interrupt cause flag so it works without an interrupt as well.

Signed-off-by Daniel Mack <[email protected]>
sound/soc/codecs/tlv320aic3x.c +20 -2
index 2047c66..a41aa72 100644
@@ -1017,14 +1017,32 @@ int aic3x_get_gpio(struct snd_soc_codec *codec, int gpio)
 }
 EXPORT_SYMBOL_GPL(aic3x_get_gpio);
 
+void aic3x_set_headset_detection(struct snd_soc_codec *codec, int val)
+{
+	u8 reg_a = val & 0xff;
+	if (reg_a & (3 << 5))
+		reg_a |= 0x80;
+
+	aic3x_write(codec, AIC3X_HEADSET_DETECT_CTRL_A, reg_a);
+}
+EXPORT_SYMBOL_GPL(aic3x_set_headset_detection);
+
 int aic3x_headset_detected(struct snd_soc_codec *codec)
 {
 	u8 val;
-	aic3x_read(codec, AIC3X_RT_IRQ_FLAGS_REG, &val);
-	return (val >> 2) & 1;
+	aic3x_read(codec, AIC3X_HEADSET_DETECT_CTRL_B, &val);
+	return (val >> 4) & 1;
 }
 EXPORT_SYMBOL_GPL(aic3x_headset_detected);
 
+int aic3x_button_pressed(struct snd_soc_codec *codec)
+{
+	u8 val;
+	aic3x_read(codec, AIC3X_HEADSET_DETECT_CTRL_B, &val);
+	return (val >> 5) & 1;
+}
+EXPORT_SYMBOL_GPL(aic3x_button_pressed);
+
 #define AIC3X_RATES	SNDRV_PCM_RATE_8000_96000
 #define AIC3X_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \
 			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S32_LE)
sound/soc/codecs/tlv320aic3x.h +24 -1
index 7e982ac..2f36c03 100644
@@ -39,7 +39,9 @@
 #define AIC3X_OVRF_STATUS_AND_PLLR_REG	11
 /* Audio codec digital filter control register */
 #define AIC3X_CODEC_DFILT_CTRL		12
-
+/* Headset/button press detection register */
+#define AIC3X_HEADSET_DETECT_CTRL_A	13
+#define AIC3X_HEADSET_DETECT_CTRL_B	14
 /* ADC PGA Gain control registers */
 #define LADC_VOL			15
 #define RADC_VOL			16

@@ -233,7 +235,28 @@ enum {
 
 void aic3x_set_gpio(struct snd_soc_codec *codec, int gpio, int state);
 int aic3x_get_gpio(struct snd_soc_codec *codec, int gpio);
+
+/* headset detection / button API, logically OR the values
+ * on calls to aic3x_set_headset_detection(). */
+#define AIC3X_HEADSET_DETECT_OFF	(0 << 5)
+#define AIC3X_HEADSET_DETECT_STEREO	(1 << 5)
+#define AIC3X_HEADSET_DETECT_CELLULAR	(2 << 5)
+#define AIC3X_HEADSET_DETECT_BOTH	(3 << 5)
+
+#define AIC3X_HEADSET_DEBOUNCE_16MS	(0 << 2)
+#define AIC3X_HEADSET_DEBOUNCE_32MS	(1 << 2)
+#define AIC3X_HEADSET_DEBOUNCE_64MS	(2 << 2)
+#define AIC3X_HEADSET_DEBOUNCE_128MS	(3 << 2)
+#define AIC3X_HEADSET_DEBOUNCE_256MS	(4 << 2)
+
+#define AIC3X_BUTTON_DEBOUNCE_0MS	(0 << 0)
+#define AIC3X_BUTTON_DEBOUNCE_8MS	(1 << 0)
+#define AIC3X_BUTTON_DEBOUNCE_16MS	(2 << 0)
+#define AIC3X_BUTTON_DEBOUNCE_32MS	(3 << 0)
+
+void aic3x_set_headset_detection(struct snd_soc_codec *codec, int val);
 int aic3x_headset_detected(struct snd_soc_codec *codec);
+int aic3x_button_pressed(struct snd_soc_codec *codec);
 
 struct aic3x_setup_data {
 	int i2c_bus;

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

#1 lore raw
Quoted 1 line · Daniel Mack

Hi,

As previously mentioned it'd be really helpful if you could submit patches in the standard format in Documentation/SubmittingPatches. Even if you need to use an attachment for the code pleasee don't submit two separate changelogs as you currently do.

sound/soc/codecs/tlv320aic3x.c int aic3x_get_gpio(struct snd_soc_codec *codec, int gpio) · line 1024

Quoted 5 lines
sound/soc/codecs/tlv320aic3x.c +5 -0
+void aic3x_set_headset_detection(struct snd_soc_codec *codec, int val)
+{
+	u8 reg_a = val & 0xff;
+	if (reg_a & (3 << 5))
+		reg_a |= 0x80;

This is a little obscure... Why are you munging the set value like this? Replacing (3 << 5) with the constants from the header may help. Looking at the defines below I can't help but think that it'd be clearer to pass in three parameters - what to detect and the two debounce times.

sound/soc/codecs/tlv320aic3x.c int aic3x_get_gpio(struct snd_soc_codec *codec, int gpio) · line 1036

Quoted 9 lines

int aic3x_headset_detected(struct snd_soc_codec *codec) {

sound/soc/codecs/tlv320aic3x.c +2 -2
 	u8 val;
-	aic3x_read(codec, AIC3X_RT_IRQ_FLAGS_REG, &val);
-	return (val >> 2) & 1;
+	aic3x_read(codec, AIC3X_HEADSET_DETECT_CTRL_B, &val);
+	return (val >> 4) & 1;
 }
 EXPORT_SYMBOL_GPL(aic3x_headset_detected);

How is the interrupt deasserted? Are you sure that the interrupt will be cleared if you don't read the interrupt flags register? May be worth splitting out, it's not directly part of the rest of the patch.

sound/soc/codecs/tlv320aic3x.h enum { · line 240

Quoted 3 lines
sound/soc/codecs/tlv320aic3x.h +3 -0
+
+/* headset detection / button API, logically OR the values
+ * on calls to aic3x_set_headset_detection(). */

This could go in kerneldoc for the function as well.

sound/soc/codecs/tlv320aic3x.h enum { · line 244

Quoted 4 lines
sound/soc/codecs/tlv320aic3x.h +4 -0
+#define AIC3X_HEADSET_DETECT_OFF	(0 << 5)
+#define AIC3X_HEADSET_DETECT_STEREO	(1 << 5)
+#define AIC3X_HEADSET_DETECT_CELLULAR	(2 << 5)
+#define AIC3X_HEADSET_DETECT_BOTH	(3 << 5)

What exactly is CELLULAR? A headset?

#2 lore raw
Quoted 10 lines · Mark Brown

Review

Quoted 5 lines
(quoted diff) +5 -0
+void aic3x_set_headset_detection(struct snd_soc_codec *codec, int val)
+{
+	u8 reg_a = val & 0xff;
+	if (reg_a & (3 << 5))
+		reg_a |= 0x80;

This is a little obscure... Why are you munging the set value like this? Replacing (3 << 5) with the constants from the header may help. Looking at the defines below I can't help but think that it'd be clearer to pass in three parameters - what to detect and the two debounce times.

Ok, done in the new patch. There is one bit (0x80) which needs to be set if any of 0x60 is given, hence the bit banging.

Quoted 4 lines
Quoted 4 lines

int aic3x_headset_detected(struct snd_soc_codec *codec) {

(quoted diff) +0 -1
 	u8 val;
-	aic3x_read(codec, AIC3X_RT_IRQ_FLAGS_REG, &val);
Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +2 -1
-	return (val >> 2) & 1;
+	aic3x_read(codec, AIC3X_HEADSET_DETECT_CTRL_B, &val);
+	return (val >> 4) & 1;
 }
 EXPORT_SYMBOL_GPL(aic3x_headset_detected);

How is the interrupt deasserted? Are you sure that the interrupt will be cleared if you don't read the interrupt flags register?

Seems to be, as it works for our device here. The point is that headset insertions/removals do not neccessarily cause an interrupt, so the bit in the flags registers won't be set. One good example is the power-up condition where no event will be triggered but you still want to know whether there is anything in the jack.

Quoted 6 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+#define AIC3X_HEADSET_DETECT_OFF	(0 << 5)
+#define AIC3X_HEADSET_DETECT_STEREO	(1 << 5)
+#define AIC3X_HEADSET_DETECT_CELLULAR	(2 << 5)
+#define AIC3X_HEADSET_DETECT_BOTH	(3 << 5)

What exactly is CELLULAR? A headset?

There is different types of headsets supported by this chip (see page 42 of the datasheet), and this bits define which types the detection should be enabled for.

Best regards,
Daniel

---

- Add aic3x_set_headset_detection() function to define the headset detection mode for tlv32aic3x chips
- added aic3x_button_pressed()
- Read from the real-time registers in aic3x_headset_detected() to query headset presence without an occured interrupt

Signed-off-by Daniel Mack <[email protected]>
 tlv320aic3x.c |   31 +++++++++++++++++++++++++++++--
 tlv320aic3x.h |   40 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 68 insertions(+), 3 deletions(-)
sound/soc/codecs/tlv320aic3x.c +29 -2
index 2047c66..537bbbf 100644
@@ -1017,14 +1017,41 @@ int aic3x_get_gpio(struct snd_soc_codec *codec, int gpio)
 }
 EXPORT_SYMBOL_GPL(aic3x_get_gpio);
 
+void aic3x_set_headset_detection(struct snd_soc_codec *codec, int detect,
+				 int headset_debounce, int button_debounce)
+{
+	u8 val;
+
+	val = ((detect & AIC3X_HEADSET_DETECT_MASK)
+		<< AIC3X_HEADSET_DETECT_SHIFT) |
+	      ((headset_debounce & AIC3X_HEADSET_DEBOUNCE_MASK)
+		<< AIC3X_HEADSET_DEBOUNCE_SHIFT) |
+	      ((button_debounce & AIC3X_BUTTON_DEBOUNCE_MASK)
+		<< AIC3X_BUTTON_DEBOUNCE_SHIFT);
+
+	if (detect & AIC3X_HEADSET_DETECT_MASK)
+		val |= AIC3X_HEADSET_DETECT_ENABLED;
+
+	aic3x_write(codec, AIC3X_HEADSET_DETECT_CTRL_A, val);
+}
+EXPORT_SYMBOL_GPL(aic3x_set_headset_detection);
+
 int aic3x_headset_detected(struct snd_soc_codec *codec)
 {
 	u8 val;
-	aic3x_read(codec, AIC3X_RT_IRQ_FLAGS_REG, &val);
-	return (val >> 2) & 1;
+	aic3x_read(codec, AIC3X_HEADSET_DETECT_CTRL_B, &val);
+	return (val >> 4) & 1;
 }
 EXPORT_SYMBOL_GPL(aic3x_headset_detected);
 
+int aic3x_button_pressed(struct snd_soc_codec *codec)
+{
+	u8 val;
+	aic3x_read(codec, AIC3X_HEADSET_DETECT_CTRL_B, &val);
+	return (val >> 5) & 1;
+}
+EXPORT_SYMBOL_GPL(aic3x_button_pressed);
+
 #define AIC3X_RATES	SNDRV_PCM_RATE_8000_96000
 #define AIC3X_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \
 			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S32_LE)
sound/soc/codecs/tlv320aic3x.h +39 -1
index 7e982ac..d6736c0 100644
@@ -39,7 +39,9 @@
 #define AIC3X_OVRF_STATUS_AND_PLLR_REG	11
 /* Audio codec digital filter control register */
 #define AIC3X_CODEC_DFILT_CTRL		12
-
+/* Headset/button press detection register */
+#define AIC3X_HEADSET_DETECT_CTRL_A	13
+#define AIC3X_HEADSET_DETECT_CTRL_B	14
 /* ADC PGA Gain control registers */
 #define LADC_VOL			15
 #define RADC_VOL			16

@@ -233,7 +235,43 @@ enum {
 
 void aic3x_set_gpio(struct snd_soc_codec *codec, int gpio, int state);
 int aic3x_get_gpio(struct snd_soc_codec *codec, int gpio);
+
+/* headset detection / button API */
+enum {
+	AIC3X_HEADSET_DETECT_OFF	= 0,
+	AIC3X_HEADSET_DETECT_STEREO	= 1,
+	AIC3X_HEADSET_DETECT_CELLULAR   = 2,
+	AIC3X_HEADSET_DETECT_BOTH	= 3
+};
+
+enum {
+	AIC3X_HEADSET_DEBOUNCE_16MS	= 0,
+	AIC3X_HEADSET_DEBOUNCE_32MS	= 1,
+	AIC3X_HEADSET_DEBOUNCE_64MS	= 2,
+	AIC3X_HEADSET_DEBOUNCE_128MS	= 3,
+	AIC3X_HEADSET_DEBOUNCE_256MS	= 4,
+	AIC3X_HEADSET_DEBOUNCE_512MS	= 5
+};
+
+enum {
+	AIC3X_BUTTON_DEBOUNCE_0MS	= 0,
+	AIC3X_BUTTON_DEBOUNCE_8MS	= 1,
+	AIC3X_BUTTON_DEBOUNCE_16MS	= 2,
+	AIC3X_BUTTON_DEBOUNCE_32MS	= 3
+};
+
+#define AIC3X_HEADSET_DETECT_ENABLED	0x80
+#define AIC3X_HEADSET_DETECT_SHIFT	5
+#define AIC3X_HEADSET_DETECT_MASK	3
+#define AIC3X_HEADSET_DEBOUNCE_SHIFT	2
+#define AIC3X_HEADSET_DEBOUNCE_MASK	7
+#define AIC3X_BUTTON_DEBOUNCE_SHIFT 	0
+#define AIC3X_BUTTON_DEBOUNCE_MASK	3
+
+void aic3x_set_headset_detection(struct snd_soc_codec *codec, int detect,
+				 int headset_debounce, int button_debounce);
 int aic3x_headset_detected(struct snd_soc_codec *codec);
+int aic3x_button_pressed(struct snd_soc_codec *codec);
 
 struct aic3x_setup_data {
 	int i2c_bus;
#3 lore raw
Quoted 10 lines · Daniel Mack
Quoted 4 lines · Mark Brown

Review

Quoted 2 lines
(quoted diff) +2 -0
+#define AIC3X_HEADSET_DETECT_CELLULAR	(2 << 5)
+#define AIC3X_HEADSET_DETECT_BOTH	(3 << 5)

What exactly is CELLULAR? A headset?

There is different types of headsets supported by this chip (see page 42 of the datasheet), and this bits define which types the detection should be enabled for.

I *belive* it's a headset but please submit a patch adding some text documenting this, ideally people shouldn't have to refer to the datasheet to use the driver.

Quoted 5 lines

Best regards,
Daniel

---

Quoted 7 lines

- Add aic3x_set_headset_detection() function to define the headset detection mode for tlv32aic3x chips
- added aic3x_button_pressed()
- Read from the real-time registers in aic3x_headset_detected() to query headset presence without an occured interrupt

Signed-off-by Daniel Mack <[email protected]>

As I have repeatedly mentioned it would be very much better if you were to do your submissions in the standard fashion which is documented in Documentation/SubmittingPatches. The fact that you don't do this makes your patches more difficult to apply and doesn't encourage review.

Note in particular the --- above which causes your commit message to be removed.

author
#4 lore raw

Hi Ben,

Quoted 7 lines · Mark Brown
Quoted 3 lines

There is different types of headsets supported by this chip (see page 42 of the datasheet), and this bits define which types the detection should be enabled for.

I *belive* it's a headset but please submit a patch adding some text documenting this, ideally people shouldn't have to refer to the datasheet to use the driver.

Ok, added some lines of comment in my new version. However, I strongly believe that anyone who uses that chip needs to read the datasheet carefully anyway as it is very versatile and full of pitfalls therefore.

Quoted 3 lines

As I have repeatedly mentioned it would be very much better if you were to do your submissions in the standard fashion which is documented in Documentation/SubmittingPatches.

I'm switching over to git-format-patch now, hoping this tool does it right. Sorry for the trouble, I didn't want to bother you.

Best regards,
Daniel

author
#5 lore raw
In reply to Mark Brown

- Add aic3x_set_headset_detection() function to define the headset detection mode for tlv32aic3x chips
- added aic3x_button_pressed()
- Read from the real-time registers in aic3x_headset_detected() to query headset presence without an occured interrupt

Signed-off-by Daniel Mack <[email protected]>

---

Compared to the version I've sent before, this one adds some lines of comments for aic3x_set_headset_detection() in the header file.

 sound/soc/codecs/tlv320aic3x.c |   31 +++++++++++++++++++++++++-
 sound/soc/codecs/tlv320aic3x.h |   46 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 74 insertions(+), 3 deletions(-)
sound/soc/codecs/tlv320aic3x.c +29 -2
index 2047c66..537bbbf 100644
@@ -1017,14 +1017,41 @@ int aic3x_get_gpio(struct snd_soc_codec *codec, int gpio)
 }
 EXPORT_SYMBOL_GPL(aic3x_get_gpio);
 
+void aic3x_set_headset_detection(struct snd_soc_codec *codec, int detect,
+				 int headset_debounce, int button_debounce)
+{
+	u8 val;
+
+	val = ((detect & AIC3X_HEADSET_DETECT_MASK)
+		<< AIC3X_HEADSET_DETECT_SHIFT) |
+	      ((headset_debounce & AIC3X_HEADSET_DEBOUNCE_MASK)
+		<< AIC3X_HEADSET_DEBOUNCE_SHIFT) |
+	      ((button_debounce & AIC3X_BUTTON_DEBOUNCE_MASK)
+		<< AIC3X_BUTTON_DEBOUNCE_SHIFT);
+
+	if (detect & AIC3X_HEADSET_DETECT_MASK)
+		val |= AIC3X_HEADSET_DETECT_ENABLED;
+
+	aic3x_write(codec, AIC3X_HEADSET_DETECT_CTRL_A, val);
+}
+EXPORT_SYMBOL_GPL(aic3x_set_headset_detection);
+
 int aic3x_headset_detected(struct snd_soc_codec *codec)
 {
 	u8 val;
-	aic3x_read(codec, AIC3X_RT_IRQ_FLAGS_REG, &val);
-	return (val >> 2) & 1;
+	aic3x_read(codec, AIC3X_HEADSET_DETECT_CTRL_B, &val);
+	return (val >> 4) & 1;
 }
 EXPORT_SYMBOL_GPL(aic3x_headset_detected);
 
+int aic3x_button_pressed(struct snd_soc_codec *codec)
+{
+	u8 val;
+	aic3x_read(codec, AIC3X_HEADSET_DETECT_CTRL_B, &val);
+	return (val >> 5) & 1;
+}
+EXPORT_SYMBOL_GPL(aic3x_button_pressed);
+
 #define AIC3X_RATES	SNDRV_PCM_RATE_8000_96000
 #define AIC3X_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \
 			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S32_LE)
sound/soc/codecs/tlv320aic3x.h +45 -1

59 lines not on this page · raw message

Signature · 1 line
1.5.6.5