Takashi,
Some general questions, before I proceed:
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.
>
>
(addressed in another thread)
>
>> 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.
>
>
I noticed in reviewing support for other chipsets in ac97_patch.c, I
found samples of code that sets V_REFOUT to High-Z when the
corresponding jack is not currently configured for microphone input.
Is this something I should be doing? I'm not sure I understand how
V_REFOUT is used. Is it physically connected to the microphone jack?
Part of my confusion is that on the AD1986/AD1986A, there is a separate
V_REFOUT pin for each jack that allows connection to microphone(s), but
on the AD1985 there are 2 possible microphone jacks and 1 V_REFOUT pin.
>> 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.)
>
>
I realize that the above macros don't need to be defined in
ac97_codec.h, however I figured the best place for my new macros was
alongside related macros, which unfortunately are located here. My
minimal change philosophy overrode any temptation to move the existing 6
AC97_AD_* macros to another location. If I moved those 6 macros, then
I'd also want to move the similar macros defined for other chipsets.
Additionally, there is the quandary that although it may be possible to
move some of the other macros to ac97_patch.c, (at least some of) the AD
macros would need to go into ac97_patch.h, since ac97_codec.c
(inappropriately?) contains some AD-specific code that references these
macros.
Here are the choices I see:
1) Add the new macros to ac97_codec.h
2) Leave ac97_codec.h untouched and add the new macros to ac97_patch.c
3) Move the above macros (ac97_codec.h lines 281-287) and add the new
macros to ac97_patch.h
4) Move all related macros (ac97_codec.h lines 268-363) and add the new
macros to ac97_patch.h
etc.
Thoughts?
>> /* 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?
>
>
I used unsigned short because it made the union an even 32 bytes. There
is no reason it cannot be an unsigned char.
(I'm tempted to add an extra byte of filler. Thoughts?)
>> 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.
>
>
no problem
>> 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.
>
>
Actually this maintains existing functionality, as patch_ad1985_specific
currently calls patch_ad1980_specific, which calls
patch_ad1888_specific, which renames the controls. If you'd like I can
change the functionality.
>> +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.
>
Ditto for me too, although here I left out the "Headphone" rename
because chipset differences suggest to me that there would be no use in
wiring the headphone pins as Line Out on the AD1986/AD1986A, unlike the
other chipsets.
>
> thanks,
>
> Takashi
>
>
I appreciate the input.
Randy Cushman
-------------------------------------------------------------------------
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
