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

[Takashi Iwai]

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.


> diff --git a/include/sound/sis7019.h b/include/sound/sis7019.h
> new file mode 100644
> index 0000000..d395abb
> --- /dev/null
> +++ 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.

> diff --git a/sound/pci/sis7019.c b/sound/pci/sis7019.c
> new file mode 100644
> index 0000000..3a74f64
> --- /dev/null
> +++ b/sound/pci/sis7019.c
(snip)
> +struct sis7019 {
> +	long unsigned  ioport;

unsigned long is a more common style.

> +static irqreturn_t sis_interrupt(int irq, void *dev)
(snip)
> +	spin_lock(&sis->hw_lock);
> +	do {
> +		status = inl(io + SIS_PISR_A);
> +		if (status) {
> +			orig = status;
> +			voice = sis->voices;
> +			while (status) {
> +				bit = __ffs(status);
> +				status >>= bit + 1;
> +				voice += bit;
> +				snd_pcm_period_elapsed(voice->substream);

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

> +				sis_update_end_offsets(voice);
> +				voice++;
> +			}
> +			outl(orig, io + SIS_PISR_A);
> +		}
> +
> +		status = inl(io + SIS_PISR_B);
> +		if (status) {
> +			orig = status;
> +			voice = &sis->voices[32];
> +			while (status) {
> +				bit = __ffs(status);
> +				status >>= bit + 1;
> +				voice += bit;
> +				snd_pcm_period_elapsed(voice->substream);

Ditto.

> +				sis_update_end_offsets(voice);
> +				voice++;
> +			}
> +			outl(orig, io + SIS_PISR_B);
> +		}
> +
> +		status = inl(io + SIS_RISR);
> +		if (status) {
> +			voice = &sis->capture_voice;
> +			if (!voice->timing)
> +				snd_pcm_period_elapsed(voice->substream);

Ditto.


thanks,

Takashi