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

[Roman Volkov]

В Wed, 20 Nov 2013 13:29:18 +0100
Clemens Ladisch <clemens at ladisch.de> пишет:

> Roman Volkov wrote:
> > Clemens Ladisch <clemens at ladisch.de> пишет:
> >> Roman Volkov wrote:
> >>> + * 128kHz doesn't work for CS4361 (multichannel playback)
> >>
> >> Why do you expect 128 kHz to work?
> >
> > 128kHz will not work because ...
> 
> ... the CMI8788 does not support 128 kHz in the first place.
> 
> > And perhaps my anti-pop delay of 1 second is too long.
> 
> The Windows driver uses 2.5 seconds.
> 
> >> Is the reset pin actually connected?  Does the Windows driver do
> >> this?
> >
> > Should be, otherwise this is bad design.
> 
> This driver is not for a theoretical, optimally designed card; it is
> for the actual card that Asus builds.  If the Windows driver does not
> touch the reset pin, we should not do this either (because it might be
> connected to the self-destruct trigger).
> 
> > sometimes (rarely) I get completely distorted sound when I power up
> > the Windows machine.
> 
> After a cold boot?
> 
> This also happened for some other Xonar card when the Linux driver did
> things differently and did not reset these settings when shutting
> down.
> 
> >> Use mdelay() only when sleeping is not possible; use msleep()
> >> instead.
> >
> > msleep() is not recommended for small delays (1ms-20ms).
> 
> Only because it isn't very accurate.  But using mdelay() is even worse
> because it busy-loops.
> 
> >>> +	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.
> 
> This is not necessarily a reason to implement the same in the Linux
> driver, especially if it's not required by the actual hardware, and
> reduces usability.
> 
> > Should I change this to "HP\HP FP\Speakers"?
> 
> Yes.
> 
> >>> +/* 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.
> 
> As far as I know, none of the Xonar cards have any real power-saving
> functionality; they rely on the entire PCI slot being powered down.
> 
> For practical purposes, suspend/resume just means that you have to
> restore the registers.
> 
> >>> -	.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.
> 
> If nothing is connected to those pins, it doesn't matter.  This code
> in the Windows driver probably was just copy/pasted from the D2 code.
> 
> >>> +++ linux-3.12-my/sound/pci/oxygen/xonar_dg.h
> >>
> >> 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...
> 
> Your choice.  But it makes it hard to review changes when you move the
> code around in the same patch.
> 
> 
> Regards,
> Clemens
> 

Okay, I won't argue. If the driver will work with the changes, I
don't care. How should I name the subject? Should I mark this with
"RESEND" or "UPDATE" or leave the subject named like "[PATCH 2/2]
snd-oxygen: Fixes for the Xonar DG card"?

-- 
Kind regards,
Roman Volkov,
v1ron at mail.ru