Re: Patch for AC97 AD CODEC shared jacks

[Randy Cushman <rcushman_linux@earthlink.net>]

[-- Attachment #1: Type: text/plain, Size: 9614 bytes --]

Minor request implemented.

Randy Cushman


Takashi Iwai wrote:
> At Tue, 19 Dec 2006 11:51:29 -0500,
> Randy Cushman wrote:
>   
>> Attached is the first of my split submissions.
>>     
>
> Thanks!
>
>   
>> Apparently you are correct.  I compared the ALC650 datasheet with the 
>> corresponding code in ac97_patch.c.  This analysis indicates that my 
>> change would have broken support for this chipset.  Part of my confusion 
>> stemmed from the names of the functions in question, which I find to be 
>> counterintuitive.
>>     
>
> Ah yes, the names are confusing.
>
>   
>> Since some of your comments suggest to me that changes that improve code 
>> maintainability are sometimes preferable to changes that affect the 
>> fewest lines of code, in the attached patch I have taken the liberty of 
>> renaming functions to names that I find to be more meaningful.
>>
>> I'll hold off on creating the remaining patches for now because my other 
>> patches may need to be changed if this patch is not accepted.
>>
>> Please let me know what you think.
>>     
>
> Then change looks fine.  One minor request is not to put a space
> between '!' and the rest in the new codes.  I used this style, but
> apparently it's no major and kernel people tend to dislike it.
>
>
> [BTW, I added Cc to alsa-devel ML again, supposing that you forgot
>  it.]
>
>
> Takashi
>
>
>   
>> Randy Cushman
>>
>>
>> (From "Re: [Alsa-devel] Patch for AD1985 AC97 CODEC")
>>
>> Takashi Iwai wrote:
>>     
>>> Hi,
>>>
>>> thanks for the patch.
>>> For the ease of review, could you split your change to several
>>> patches?  Please post one patch per mail, then.
>>>
>>> Anyway, a quick review through the patch is below.
>>>
>>> At Tue, 12 Dec 2006 19:31:33 -0500,
>>> Randy Cushman wrote:
>>>   
>>>       
>>>> Summary: fix microphone and line_in selection logic
>>>>
>>>> This patch fixes the Microphone and LINE_IN select logic for
>>>> surround codecs with shared jacks.  This issue appears to apply
>>>> to most, if not all AC'97 surround codecs.  The existing code
>>>> can never utilize the shared jacks for Microphone and LINE_IN
>>>> due to the reversed jack selection logic.  The patched code
>>>> correctly selects the shared jack for input if the "Channel Mode"
>>>> selector does not specify that the jack is to be used for output.
>>>>
>>>> Specifically, in "2ch" mode the Center/LFE jack is used for
>>>> microphone input and the Surround jack is used for LINE_IN,
>>>> in "4ch" mode the Center/LFE jack is used for microphone input
>>>> and the Surround jack is used for output, and in "6ch" mode
>>>> both jacks are used for output.
>>>>
>>>> Signed-off-by: Randy Cushman <rcushman_linux@earthlink.net>
>>>>     
>>>>         
>>> This hunk likely breaks other codec support.
>>>
>>> is_shared_linein() returns true if the line-in jack is shared with
>>> surround output _and_ being used as the surround output.  Maybe the AD
>>> codec support is wrongly implemented.
>>>   
>>>       
>> (snip)
>>     
>>> thanks,
>>>
>>> Takashi
>>>
>>>
>>>   
>>>       
>> [2 ac97_shared_1.patch <text/plain (7bit)>]
>> Summary: fix microphone and line_in selection logic
>>
>> This patch fixes the Microphone and LINE_IN select logic for
>> Analog Devices surround codecs with shared jacks.  The existing
>> code can never utilize the shared jacks for Microphone and LINE_IN
>> due to the reversed jack selection logic.  The patched code
>> correctly selects the shared jack for input if the "Channel Mode"
>> selector does not specify that the jack is to be used for output.
>>
>> Specifically, in "2ch" mode the Center/LFE jack is used for
>> microphone input and the Surround jack is used for LINE_IN,
>> in "4ch" mode the Center/LFE jack is used for microphone input
>> and the Surround jack is used for output, and in "6ch" mode
>> both jacks are used for output.
>>
>> Signed-off-by: Randy Cushman <rcushman_linux@earthlink.net>
>>
>>
>> diff -r cdbbd773cd9e pci/ac97/ac97_patch.c
>> --- a/pci/ac97/ac97_patch.c	Wed Dec 13 11:21:55 2006 +0000
>> +++ b/pci/ac97/ac97_patch.c	Tue Dec 19 10:03:51 2006 -0500
>> @@ -190,14 +190,28 @@ static inline int is_clfe_on(struct snd_
>>  	return ac97->channel_mode >= 2;
>>  }
>>  
>> +/* system has shared jacks with surround out enabled */
>> +static inline int is_shared_surrout(struct snd_ac97 *ac97)
>> +{
>> +	return ! ac97->indep_surround && is_surround_on(ac97);
>> +}
>> +
>> +/* system has shared jacks with center/lfe out enabled */
>> +static inline int is_shared_clfeout(struct snd_ac97 *ac97)
>> +{
>> +	return ! ac97->indep_surround && is_clfe_on(ac97);
>> +}
>> +
>> +/* system has shared jacks with line in enabled */
>>  static inline int is_shared_linein(struct snd_ac97 *ac97)
>>  {
>> -	return ! ac97->indep_surround && is_surround_on(ac97);
>> -}
>> -
>> +	return ! ac97->indep_surround && ! is_surround_on(ac97);
>> +}
>> +
>> +/* system has shared jacks with mic in enabled */
>>  static inline int is_shared_micin(struct snd_ac97 *ac97)
>>  {
>> -	return ! ac97->indep_surround && is_clfe_on(ac97);
>> +	return ! ac97->indep_surround && ! is_clfe_on(ac97);
>>  }
>>  
>>  
>> @@ -2014,12 +2028,12 @@ static void alc650_update_jacks(struct s
>>  {
>>  	int shared;
>>  	
>> -	/* shared Line-In */
>> -	shared = is_shared_linein(ac97);
>> +	/* shared Line-In / Surround Out */
>> +	shared = is_shared_surrout(ac97);
>>  	snd_ac97_update_bits(ac97, AC97_ALC650_MULTICH, 1 << 9,
>>  			     shared ? (1 << 9) : 0);
>> -	/* update shared Mic */
>> -	shared = is_shared_micin(ac97);
>> +	/* update shared Mic In / Center/LFE Out */
>> +	shared = is_shared_clfeout(ac97);
>>  	/* disable/enable vref */
>>  	snd_ac97_update_bits(ac97, AC97_ALC650_CLOCK, 1 << 12,
>>  			     shared ? (1 << 12) : 0);
>> @@ -2149,12 +2163,12 @@ static void alc655_update_jacks(struct s
>>  {
>>  	int shared;
>>  	
>> -	/* shared Line-In */
>> -	shared = is_shared_linein(ac97);
>> +	/* shared Line-In / Surround Out */
>> +	shared = is_shared_surrout(ac97);
>>  	ac97_update_bits_page(ac97, AC97_ALC650_MULTICH, 1 << 9,
>>  			      shared ? (1 << 9) : 0, 0);
>> -	/* update shared mic */
>> -	shared = is_shared_micin(ac97);
>> +	/* update shared Mic In / Center/LFE Out */
>> +	shared = is_shared_clfeout(ac97);
>>  	/* misc control; vrefout disable */
>>  	snd_ac97_update_bits(ac97, AC97_ALC650_CLOCK, 1 << 12,
>>  			     shared ? (1 << 12) : 0);
>> @@ -2298,16 +2312,16 @@ static void alc850_update_jacks(struct s
>>  {
>>  	int shared;
>>  	
>> -	/* shared Line-In */
>> -	shared = is_shared_linein(ac97);
>> +	/* shared Line-In / Surround Out */
>> +	shared = is_shared_surrout(ac97);
>>  	/* SURR 1kOhm (bit4), Amp (bit5) */
>>  	snd_ac97_update_bits(ac97, AC97_ALC850_MISC1, (1<<4)|(1<<5),
>>  			     shared ? (1<<5) : (1<<4));
>>  	/* LINE-IN = 0, SURROUND = 2 */
>>  	snd_ac97_update_bits(ac97, AC97_ALC850_JACK_SELECT, 7 << 12,
>>  			     shared ? (2<<12) : (0<<12));
>> -	/* update shared mic */
>> -	shared = is_shared_micin(ac97);
>> +	/* update shared Mic In / Center/LFE Out */
>> +	shared = is_shared_clfeout(ac97);
>>  	/* Vref disable (bit12), 1kOhm (bit13) */
>>  	snd_ac97_update_bits(ac97, AC97_ALC850_MISC1, (1<<12)|(1<<13),
>>  			     shared ? (1<<12) : (1<<13));
>> @@ -2380,9 +2394,9 @@ int patch_alc850(struct snd_ac97 *ac97)
>>   */
>>  static void cm9738_update_jacks(struct snd_ac97 *ac97)
>>  {
>> -	/* shared Line-In */
>> +	/* shared Line-In / Surround Out */
>>  	snd_ac97_update_bits(ac97, AC97_CM9738_VENDOR_CTRL, 1 << 10,
>> -			     is_shared_linein(ac97) ? (1 << 10) : 0);
>> +			     is_shared_surrout(ac97) ? (1 << 10) : 0);
>>  }
>>  
>>  static const struct snd_kcontrol_new snd_ac97_cm9738_controls[] = {
>> @@ -2464,12 +2478,12 @@ static const struct snd_kcontrol_new snd
>>  
>>  static void cm9739_update_jacks(struct snd_ac97 *ac97)
>>  {
>> -	/* shared Line-In */
>> +	/* shared Line-In / Surround Out */
>>  	snd_ac97_update_bits(ac97, AC97_CM9739_MULTI_CHAN, 1 << 10,
>> -			     is_shared_linein(ac97) ? (1 << 10) : 0);
>> -	/* shared Mic */
>> +			     is_shared_surrout(ac97) ? (1 << 10) : 0);
>> +	/* shared Mic In / Center/LFE Out **/
>>  	snd_ac97_update_bits(ac97, AC97_CM9739_MULTI_CHAN, 0x3000,
>> -			     is_shared_micin(ac97) ? 0x1000 : 0x2000);
>> +			     is_shared_clfeout(ac97) ? 0x1000 : 0x2000);
>>  }
>>  
>>  static const struct snd_kcontrol_new snd_ac97_cm9739_controls[] = {
>> @@ -2581,8 +2595,8 @@ static void cm9761_update_jacks(struct s
>>  
>>  	val |= surr_on[ac97->spec.dev_flags][is_surround_on(ac97)];
>>  	val |= clfe_on[ac97->spec.dev_flags][is_clfe_on(ac97)];
>> -	val |= surr_shared[ac97->spec.dev_flags][is_shared_linein(ac97)];
>> -	val |= clfe_shared[ac97->spec.dev_flags][is_shared_micin(ac97)];
>> +	val |= surr_shared[ac97->spec.dev_flags][is_shared_surrout(ac97)];
>> +	val |= clfe_shared[ac97->spec.dev_flags][is_shared_clfeout(ac97)];
>>  
>>  	snd_ac97_update_bits(ac97, AC97_CM9761_MULTI_CHAN, 0x3c88, val);
>>  }
>> @@ -2829,12 +2843,12 @@ int patch_vt1617a(struct snd_ac97 * ac97
>>   */
>>  static void it2646_update_jacks(struct snd_ac97 *ac97)
>>  {
>> -	/* shared Line-In */
>> +	/* shared Line-In / Surround Out */
>>  	snd_ac97_update_bits(ac97, 0x76, 1 << 9,
>> -			     is_shared_linein(ac97) ? (1<<9) : 0);
>> -	/* shared Mic */
>> +			     is_shared_surrout(ac97) ? (1<<9) : 0);
>> +	/* shared Mic / Center/LFE Out */
>>  	snd_ac97_update_bits(ac97, 0x76, 1 << 10,
>> -			     is_shared_micin(ac97) ? (1<<10) : 0);
>> +			     is_shared_clfeout(ac97) ? (1<<10) : 0);
>>  }
>>  
>>  static const struct snd_kcontrol_new snd_ac97_controls_it2646[] = {
>>     
>
>
>   


[-- Attachment #2: ac97_shared_2.patch --]
[-- Type: text/plain, Size: 5993 bytes --]

Summary: fix microphone and line_in selection logic

This patch fixes the Microphone and LINE_IN select logic for
Analog Devices surround codecs with shared jacks.  The existing
code can never utilize the shared jacks for Microphone and LINE_IN
due to the reversed jack selection logic.  The patched code
correctly selects the shared jack for input if the "Channel Mode"
selector does not specify that the jack is to be used for output.

Specifically, in "2ch" mode the Center/LFE jack is used for
microphone input and the Surround jack is used for LINE_IN,
in "4ch" mode the Center/LFE jack is used for microphone input
and the Surround jack is used for output, and in "6ch" mode
both jacks are used for output.

Signed-off-by: Randy Cushman <rcushman_linux@earthlink.net>


diff -r cdbbd773cd9e pci/ac97/ac97_patch.c
--- a/pci/ac97/ac97_patch.c	Wed Dec 13 11:21:55 2006 +0000
+++ b/pci/ac97/ac97_patch.c	Tue Dec 19 12:25:43 2006 -0500
@@ -190,14 +190,28 @@ static inline int is_clfe_on(struct snd_
 	return ac97->channel_mode >= 2;
 }
 
+/* system has shared jacks with surround out enabled */
+static inline int is_shared_surrout(struct snd_ac97 *ac97)
+{
+	return !ac97->indep_surround && is_surround_on(ac97);
+}
+
+/* system has shared jacks with center/lfe out enabled */
+static inline int is_shared_clfeout(struct snd_ac97 *ac97)
+{
+	return !ac97->indep_surround && is_clfe_on(ac97);
+}
+
+/* system has shared jacks with line in enabled */
 static inline int is_shared_linein(struct snd_ac97 *ac97)
 {
-	return ! ac97->indep_surround && is_surround_on(ac97);
-}
-
+	return !ac97->indep_surround && !is_surround_on(ac97);
+}
+
+/* system has shared jacks with mic in enabled */
 static inline int is_shared_micin(struct snd_ac97 *ac97)
 {
-	return ! ac97->indep_surround && is_clfe_on(ac97);
+	return !ac97->indep_surround && !is_clfe_on(ac97);
 }
 
 
@@ -2014,12 +2028,12 @@ static void alc650_update_jacks(struct s
 {
 	int shared;
 	
-	/* shared Line-In */
-	shared = is_shared_linein(ac97);
+	/* shared Line-In / Surround Out */
+	shared = is_shared_surrout(ac97);
 	snd_ac97_update_bits(ac97, AC97_ALC650_MULTICH, 1 << 9,
 			     shared ? (1 << 9) : 0);
-	/* update shared Mic */
-	shared = is_shared_micin(ac97);
+	/* update shared Mic In / Center/LFE Out */
+	shared = is_shared_clfeout(ac97);
 	/* disable/enable vref */
 	snd_ac97_update_bits(ac97, AC97_ALC650_CLOCK, 1 << 12,
 			     shared ? (1 << 12) : 0);
@@ -2149,12 +2163,12 @@ static void alc655_update_jacks(struct s
 {
 	int shared;
 	
-	/* shared Line-In */
-	shared = is_shared_linein(ac97);
+	/* shared Line-In / Surround Out */
+	shared = is_shared_surrout(ac97);
 	ac97_update_bits_page(ac97, AC97_ALC650_MULTICH, 1 << 9,
 			      shared ? (1 << 9) : 0, 0);
-	/* update shared mic */
-	shared = is_shared_micin(ac97);
+	/* update shared Mic In / Center/LFE Out */
+	shared = is_shared_clfeout(ac97);
 	/* misc control; vrefout disable */
 	snd_ac97_update_bits(ac97, AC97_ALC650_CLOCK, 1 << 12,
 			     shared ? (1 << 12) : 0);
@@ -2298,16 +2312,16 @@ static void alc850_update_jacks(struct s
 {
 	int shared;
 	
-	/* shared Line-In */
-	shared = is_shared_linein(ac97);
+	/* shared Line-In / Surround Out */
+	shared = is_shared_surrout(ac97);
 	/* SURR 1kOhm (bit4), Amp (bit5) */
 	snd_ac97_update_bits(ac97, AC97_ALC850_MISC1, (1<<4)|(1<<5),
 			     shared ? (1<<5) : (1<<4));
 	/* LINE-IN = 0, SURROUND = 2 */
 	snd_ac97_update_bits(ac97, AC97_ALC850_JACK_SELECT, 7 << 12,
 			     shared ? (2<<12) : (0<<12));
-	/* update shared mic */
-	shared = is_shared_micin(ac97);
+	/* update shared Mic In / Center/LFE Out */
+	shared = is_shared_clfeout(ac97);
 	/* Vref disable (bit12), 1kOhm (bit13) */
 	snd_ac97_update_bits(ac97, AC97_ALC850_MISC1, (1<<12)|(1<<13),
 			     shared ? (1<<12) : (1<<13));
@@ -2380,9 +2394,9 @@ int patch_alc850(struct snd_ac97 *ac97)
  */
 static void cm9738_update_jacks(struct snd_ac97 *ac97)
 {
-	/* shared Line-In */
+	/* shared Line-In / Surround Out */
 	snd_ac97_update_bits(ac97, AC97_CM9738_VENDOR_CTRL, 1 << 10,
-			     is_shared_linein(ac97) ? (1 << 10) : 0);
+			     is_shared_surrout(ac97) ? (1 << 10) : 0);
 }
 
 static const struct snd_kcontrol_new snd_ac97_cm9738_controls[] = {
@@ -2464,12 +2478,12 @@ static const struct snd_kcontrol_new snd
 
 static void cm9739_update_jacks(struct snd_ac97 *ac97)
 {
-	/* shared Line-In */
+	/* shared Line-In / Surround Out */
 	snd_ac97_update_bits(ac97, AC97_CM9739_MULTI_CHAN, 1 << 10,
-			     is_shared_linein(ac97) ? (1 << 10) : 0);
-	/* shared Mic */
+			     is_shared_surrout(ac97) ? (1 << 10) : 0);
+	/* shared Mic In / Center/LFE Out **/
 	snd_ac97_update_bits(ac97, AC97_CM9739_MULTI_CHAN, 0x3000,
-			     is_shared_micin(ac97) ? 0x1000 : 0x2000);
+			     is_shared_clfeout(ac97) ? 0x1000 : 0x2000);
 }
 
 static const struct snd_kcontrol_new snd_ac97_cm9739_controls[] = {
@@ -2581,8 +2595,8 @@ static void cm9761_update_jacks(struct s
 
 	val |= surr_on[ac97->spec.dev_flags][is_surround_on(ac97)];
 	val |= clfe_on[ac97->spec.dev_flags][is_clfe_on(ac97)];
-	val |= surr_shared[ac97->spec.dev_flags][is_shared_linein(ac97)];
-	val |= clfe_shared[ac97->spec.dev_flags][is_shared_micin(ac97)];
+	val |= surr_shared[ac97->spec.dev_flags][is_shared_surrout(ac97)];
+	val |= clfe_shared[ac97->spec.dev_flags][is_shared_clfeout(ac97)];
 
 	snd_ac97_update_bits(ac97, AC97_CM9761_MULTI_CHAN, 0x3c88, val);
 }
@@ -2829,12 +2843,12 @@ int patch_vt1617a(struct snd_ac97 * ac97
  */
 static void it2646_update_jacks(struct snd_ac97 *ac97)
 {
-	/* shared Line-In */
+	/* shared Line-In / Surround Out */
 	snd_ac97_update_bits(ac97, 0x76, 1 << 9,
-			     is_shared_linein(ac97) ? (1<<9) : 0);
-	/* shared Mic */
+			     is_shared_surrout(ac97) ? (1<<9) : 0);
+	/* shared Mic / Center/LFE Out */
 	snd_ac97_update_bits(ac97, 0x76, 1 << 10,
-			     is_shared_micin(ac97) ? (1<<10) : 0);
+			     is_shared_clfeout(ac97) ? (1<<10) : 0);
 }
 
 static const struct snd_kcontrol_new snd_ac97_controls_it2646[] = {

[-- Attachment #3: Type: text/plain, Size: 347 bytes --]

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[-- Attachment #4: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/alsa-devel