В Tue, 19 Nov 2013 23:23:54 +0100 Clemens Ladisch clemens@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 thecodec 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