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.
> Summary: fix malfunctioning mixer controls for AD1985
>
> This patch replaces the "V_REFOUT Enable" mixer switch control
> with a listbox control for the AD1985 CODEC.
>
> Previous patch "AD1888 mixer controls for DC mode" added
> controls that were propogated to multiple codecs. For the
> AD1985 codec, the bits VREFH and VREFD function differently,
> preventing the "V_REFOUT Enable" control from setting V_REFOUT
> to Hi-Z.
>
> This patch also corrects an issue in which register bits relating
> to mixer controls "Surround Jack Mode" and "Channel Mode".
> The register bits controlled by these controls were being set
> at boot time to states inconsistent with the stored values of
> these controls.
>
> Signed-off-by: Randy Cushman <rcushman_linux@xxxxxxxxxxxxx>
This change looks fine.
> Summary: fix various issues with AD1986/AD1986A support
>
> Previously, ac97_codec.c was coded to support AD1986 and AD1986A
> CODECs using code written for the AD1985 CODEC. This allowed the
> LINE_OUT and HEADPHONE jacks to function properly, however register
> differences between the CODECs prevented line and microphone inputs
> from functioning.
>
> Specifically, this patch fixes issues with the following mixer
> controls: 'V_REFOUT', 'Spread Front to Surround and Center/LFE',
> 'Exchange Front/Surround', 'Surround Jack Mode', and 'Channel Mode'.
> This patch removes the undocumented AD1888 control
> 'High Pass Filter Enable' and adds the new control
> 'Exchange Mic/Line In'.
>
> Signed-off-by: Randy Cushman <rcushman_linux@xxxxxxxxxxxxx>
This sounds fine, too.
> diff -r bc7ef767d0cf include/ac97_codec.h
> --- a/include/ac97_codec.h Wed Nov 29 15:29:40 2006 +0100
> +++ b/include/ac97_codec.h Wed Dec 06 09:59:58 2006 -0500
> @@ -282,9 +282,11 @@
> #define AC97_AD_TEST 0x5a /* test register */
> #define AC97_AD_TEST2 0x5c /* undocumented test register 2 */
> #define AC97_AD_CODEC_CFG 0x70 /* codec configuration */
> +#define AC97_AD_MISC2 0x70 /* Misc Control Bits 2 (AD1986) */
> #define AC97_AD_JACK_SPDIF 0x72 /* Jack Sense & S/PDIF */
> #define AC97_AD_SERIAL_CFG 0x74 /* Serial Configuration */
> #define AC97_AD_MISC 0x76 /* Misc Control Bits */
> +#define AC97_AD_MISC3 0x7a /* Misc Control Bits 3 (AD1986) */
These don't have to be defined in the public header.
(I'd rather like to move all these codec-speicific definitions to
local file, i.e. ac97_patch.c or local headers.)
> /* specific - Cirrus Logic */
> #define AC97_CSR_ACMODE 0x5e /* AC Mode Register */
> @@ -503,6 +505,7 @@ struct snd_ac97 {
> unsigned short id[3]; // codec IDs (lower 16-bit word)
> unsigned short pcmreg[3]; // PCM registers
> unsigned short codec_cfg[3]; // CODEC_CFG bits
> + unsigned short swap_mic_linein;
Any need to be unsigned short?
> diff -r bc7ef767d0cf pci/ac97/ac97_patch.c
> --- a/pci/ac97/ac97_patch.c Wed Nov 29 15:29:40 2006 +0100
> +++ b/pci/ac97/ac97_patch.c Tue Dec 12 16:28:49 2006 -0500
> @@ -192,12 +192,12 @@ static inline int is_clfe_on(struct snd_
>
> 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);
> }
>
> 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);
> }
Dangerous changes as I mentioned.
> +static int snd_ac97_ad1985_vrefout_info(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_info *uinfo)
Please fold lines to keep the (new) code within 80 columns.
> static void ad1985_update_jacks(struct snd_ac97 *ac97)
> @@ -1967,8 +2098,13 @@ static int patch_ad1985_specific(struct
> {
> int err;
>
> - if ((err = patch_ad1980_specific(ac97)) < 0)
> + /* rename 0x04 as "Master" and 0x02 as "Master Surround" */
> + snd_ac97_rename_vol_ctl(ac97, "Master Playback", "Master Surround Playback");
> + snd_ac97_rename_vol_ctl(ac97, "Headphone Playback", "Master Playback");
This rename would break many other boards since the renaming of
master/headphone is usually done via ac97_quirk.
> +static int patch_ad1986_specific(struct snd_ac97 *ac97)
> +{
> + int err;
> +
> + /* rename 0x02 as "Master Surround" */
> + snd_ac97_rename_vol_ctl(ac97, "Master Playback", "Master Surround Playback");
Ditto.
thanks,
Takashi
-------------------------------------------------------------------------
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
