[alsa-devel] [PATCH 2/2] snd-oxygen: Fixes for the Xonar DG card

[Roman Volkov]

В Tue, 19 Nov 2013 23:23:54 +0100
Clemens Ladisch <clemens at ladisch.de> пишет:

> Roman Volkov wrote:
> > Fixes for the Xonar DG model of the Oxygen driver:
> 
> Each of the following sentences describes a separate logical change.

Yea, have some problems with naming :). Will try.

> > +++ linux-3.12-my/sound/pci/oxygen/xonar_dg.c	2013-11-19
> > 01:01:07.000000000 +0400
> 
> > + * DGX card: not tested.
> > + * Suspend/resume: not tested
> 
> This does not belong in the source code; if some tests succeeds, we do
> not want to update the driver.

Will move that to the patch notes.

> > + * SPDIF: not tested
> 
> SPDIF is handled completely by the CMI8788 itself.
> 
> > + * 128kHz doesn't work for CS4361 (multichannel playback)
> 
> Why do you expect 128 kHz to work?

128kHz will not work because CS4361 can't detect clocks for the
following mode:
LRCK = 128khz
MCLK = 128x
I tried to use MCLKs sequence 512,256,128, but unfortunately,
then 32kHz does not work for CS4361.
Is the frequency 128kHz valid for ALSA?

> > + * CS4245 Auxiliary Output is connected to the front panel
> > headphones jack
> > + * through amplifier.
> 
> The comment below already says this.

> > - *   GPIO 3 <- ?
> > + *   GPIO 3 <- unused (?)
> 
> Why this change?

Will remove overcommenting and reduce changes.

> 
> > - *   GPIO 5 -> route input jack to line-in (0) or mic-in (1)
> > - *   GPIO 6 -> route input jack to line-in (0) or mic-in (1)
> > + *   GPIO 5 -> ADC analog circuit power supply control for left
> > channel (?)
> > + *   GPIO 6 -> ADC analog circuit power supply control for right
> > channel (?)
> 
> The Xonar cards often use mechanical relays for these routing things;
> you should be able to hear them when switching.

Yes, GPIO 7 swiches that relay.
 
> > - *   GPIO 7 -> enable rear headphone amp
> > + *   GPIO 7 -> switches green rear output jack between CS4245 /
> > CS4361 first
> > + * channel
> 
> 0 or 1?
> 
> > - *   GPIO 8 -> enable output to speakers
> > + *   GPIO 8 -> DAC analog circuit power supply control (?)
> 
> This relay is intended to prevent popping when powering on.

GPIO 8 "powers up" or I don't know what it exactly does, but it doesn't
switch the relay. It "enables" the audio output, headphone amp becomes
connected to the DACs output, probably the same for multichannel
outputs. During initialization of the windows driver, this GPIO becomes
active at the last step. And perhaps my anti-pop delay of 1 second is
too long. Maybe, I'll find better way to prevent pops.

> > +static int cs4245_dump_regs(struct oxygen *chip, bool bRead)
> 
> Why "dump" for writing?  This should be two functions.
> 
> > +	for (address = 0; address <= 0x10; address++) {
> > +		return bRead ? cs4245_read_spi(chip, address) :
> > +			cs4245_write_spi(chip, address);
> > +	}
> 
> Why return from the loop?

Oh, my mistake. Too quickly performed optimizations :). This only
affects suspend\resume logic, so I missed out this after testing.

> > +static void dg_init(struct oxygen *chip)
> 
> > +	 * Recommended sequence from the datasheet: reset the
> > codec and then
> > +	 * initialize it.
> 
> > +	oxygen_write8_masked(chip, OXYGEN_FUNCTION, 0,
> > +		OXYGEN_FUNCTION_RESET_CODEC);
> 
> This is what the _set/clear_bits functions are for.

Thanks for the hint.

> Is the reset pin actually connected?  Does the Windows driver do this?

Should be, otherwise this is bad design. Windows driver probably not
does that, but honestly, this driver is slightly buggy: sometimes
(rarely) I get completely distorted sound when I power up the Windows
machine. In the Linux driver I do not have this issue.

> > +	mdelay(1);
> 
> Use mdelay() only when sleeping is not possible; use msleep() instead.

msleep() is not recommended for small delays (1ms-20ms). Using mdelay()
here is okay. Don't use mdelay for long delays (more than 20ms).

> > +	msleep_interruptible(1000);
> 
> When you use _interruptible, you should also check for the interrupt.

Will use just msleep().

> >  static void set_cs4245_dac_params(struct oxygen *chip,
> >  				  struct snd_pcm_hw_params *params)
> 
> > +	unsigned char tmp;
> > +	unsigned char tmp2;
> 
> These variable names are not very descriptive.

Ok

> > +	case PLAYBACK_DST_20_CH:
> > +	case PLAYBACK_DST_40_CH:
> > +	case PLAYBACK_DST_51_CH:
> 
> Why is it necessary to differentiate between those?  There should be
> no mixer control; the driver can automatically detect what format is
> used. And this should not make any difference because the unused
> channels will be silent anyway.

Just as in the windows driver. Should I change this to "HP\HP
FP\Speakers"?

> > +/* Put the codec into low power mode, disable audio output, save
> > registers */
> 
> Does the Windows driver implement this?

I don't know. Linux docs says I should put my hardware into low power
mode and I do that. However I can't test this on my machine - my
display remains black after resume (VGA driver) and computer freezes
at all when I use NVIDIA proprietary driver... Maybe will test this on
another machine later. 

> > +static void dg_suspend(struct oxygen *chip)
> >  {
> >  	struct dg *data = chip->model_data;
> > +	data->cs4245_shadow[CS4245_POWER_CTRL] = CS4245_PDN_MIC |
> > +		CS4245_PDN_ADC | CS4245_PDN_DAC | CS4245_PDN;
> > +	cs4245_write_spi(chip, CS4245_POWER_CTRL);
> > +	cs4245_dump_regs(chip, true);
> 
> Why reading the registers?  The cs4245_shadow values should already be
> valid.

Just to validate again :)

> > -	.function_flags = OXYGEN_FUNCTION_SPI,
> > +	.function_flags = OXYGEN_FUNCTION_SPI |
> > OXYGEN_FUNCTION_ENABLE_SPI_4_5,
> 
> Why?

Because this just works and as in the Windows driver. IFAIK, this sets
bit 7 at register 50h in the CMI chip (1: select SPI chip 4, 5 enable
function).

> > +++ linux-3.12-my/sound/pci/oxygen/xonar_dg.h	2013-11-19
> > 01:06:12.000000000 +0400
> 
> This file was intended as the interface between xonar_dg.c and the
> generic oxygen.c driver.
> 
> That you have to add so much stuff indicates that xonar_dg.c and
> xonar_dg_mixer.c are not very independent and maybe should have stayed
> a single file.

But moving mixer to another file so simplifies things... Currently,
xonar_dg.c is at upper level of hierarchy, probably I should change
that? (The mixer controls the driver, not otherwise)

> > +	/* Independend capture volumes */
> > +	char mic_vol_l;
> > +	char mic_vol_r;
> > +	bool mic_m;
> > +	char line_vol_l;
> > +	char line_vol_r;
> > +	bool line_m;
> > +	char aux_vol_l;
> > +	char aux_vol_r;
> > +	bool aux_m;
> 
> The old driver had a separate volume for front mic.

Will add this one.

> 
> > + * It is not good that we do not have software volume control
> > + * as the Windows driver does. CS4361 cannot attenuate the volume.
> > + * When we are switching from headphones to the speakers,
> > + * which are controlled by the simple DAC CS4361, our headphones
> > + * in the rear jack can stun us.
> 
> This is why I did not implement a volume control in the first place.

Honestly, this is a common problem with ALSA, headphones without volume
regulator, and flat_volumes = yes.
It's better to have this regulator I think...

> > +static int xonar_dg_pcm_route_apply(struct oxygen *chip)
> 
> > +		oxygen_write8_masked(chip, OXYGEN_GPIO_DATA, 0x00,
> > 0x80);
> 
> Use _set_bits and the GPIO_ symbol.

Ok

> > +static int xonar_dg_pga_volume_get(struct snd_kcontrol *ctl,
> > +				struct snd_ctl_elem_value *val)
> > +	switch (ctl->private_value) {
> > +	case CAPTURE_SRC_MIC:
> > +	case CAPTURE_SRC_LINE:
> > +	case CAPTURE_SRC_AUX:
> 
> And FP_MIC?  (Also elsewhere.)

CAPTURE_SRC_MIC private_value is common for both FP and rear, see
snd_kcontrol_new array.

> 
> Regards,
> Clemens
>