[alsa-devel] [PATCH] [RFC] sis7019: Initial support for the SiS 7019 Audio Accelerator

[Dave Dillow]

On Tue, 2007-10-09 at 11:48 +0200, Takashi Iwai wrote:
> At Fri,  5 Oct 2007 00:34:58 -0400,
> Dave Dillow wrote:
> > 
> > Basic audio support for the SiS 7019 Audio Accelerator as found in
> > the SiS 55x SoC. There is currently no synth or power management
> > support at the moment, but the audio playback has been tested with
> > several channel and rate combinations, as well as various period
> > and buffer configurations.
> > 
> > Signed-off-by: David Dillow <dave at thedillows.org>
> 
> Thanks for the patch.  The code looks almost OK to me.
> 
> Below is a very brief review.

Great, thanks for the review! I'll fix up the issues you pointed out,
and I hope to have hardware for testing again next week -- I've got an
untested implementation of power management coded up. Aside from the PM
code, this driver has been in production for almost a year. 

I'll also diff this against alsa-driver, instead of the kernel proper.

> > diff --git a/include/sound/sis7019.h b/include/sound/sis7019.h
> 
> Any reason to put this to include/sound?  It's basically a place for
> headers used publicly.  If the header is for the driver-only, better
> placed in sound/pci or a place where is private.

I agree, I was just following several other driver's lead, specifically
trident. I'll be happy to move it.


> > +struct sis7019 {
> > +	long unsigned  ioport;
> 
> unsigned long is a more common style.

Doh! It's also the style I use everywhere else in the driver.

> You need to unlock sis->hw_lock before calling
> snd_pcm_period_elapsed().  It may call snd_pcm_stop() at XRUN, and
> this invokes the trigger callback, which locks hw_lock again.
> Simply re-acquire hw_lock after calling snd_pcm_period_elapsed().

Can do.

Dave