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[] = {
-------------------------------------------------------------------------
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
