Re: Patch for AD1985 AC97 CODEC

[Takashi Iwai <tiwai@suse.de>]

At Tue, 19 Dec 2006 13:40:32 -0500,
Randy Cushman wrote:
> 
> >> 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@earthlink.net>
> >>     
> >
> > 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.

AFAIK, such VREF changes are needed to reduce the possible noises on
the surround output jacks.  It's highly depending on the board
implementation.  So, if the driver works fine without vref change, it
should be OK :)


> >> 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?

I'd like 2.  Let's make a working version first, then clean up these
things, e.g. by moving codec-specific defines to ac97_local.h.
There are also defines in ac97_patch.c, and could be better placed in
ac97_local.h.


> >>  /* 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?)

Compilers would care such a thing, so you don't have to worry much, I
guess.  I don't mind so much what type is there, though.

> >>  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.

Ah OK, then I overlooked that part.


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