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: 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@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
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[] = {
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@xxxxxxxxxxxxx>
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[] = {
-------------------------------------------------------------------------
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/alsa-devel
