Re: [PATCH 1/2] sis7019: support the SiS 7019 Audio Accelerator

[Dave Dillow <dave@thedillows.org>]

On Wed, 2007-11-28 at 10:56 +0100, Takashi Iwai wrote:
> At Wed, 28 Nov 2007 02:19:03 -0500,
> Dave Dillow wrote:
> > +static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;	/* Index 0-MAX */
> > +static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;	/* ID for this card */
> > +static int enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE_PNP;	/* Enable this card */
> 
> Can there be multiple SIS7019 devices?  If there can only a single
> device, let's avoid array.

Ok, can do.

> > +static struct snd_pcm_hardware sis_playback_hw_info = {
> > +	.info = (SNDRV_PCM_INFO_MMAP |
> > +		 SNDRV_PCM_INFO_MMAP_VALID |
> > +		 SNDRV_PCM_INFO_INTERLEAVED |
> > +		 SNDRV_PCM_INFO_BLOCK_TRANSFER |
> > +		 SNDRV_PCM_INFO_SYNC_START |
> > +		 SNDRV_PCM_INFO_RESUME),
> 
> SNDRV_PCM_INFO_SYNC_START seems invalid here.  It's not implemented in
> the driver.

Perhaps I'm misunderstanding what SYNC_START is for -- I can start
multiple playback/capture substreams at once in ->trigger(). If it means
something else, can you point to a driver that does implement it
properly so that I can see if can be implemented?

> > +static void __sis_map_silence(struct sis7019 *sis)
> 
> Any reason to use __ prefix?

It requires locks to be held when it is called, making it an internal
helper function. I'd prefer to keep the prefix, but it could go away, or
be merged into the one call-site.

Thanks,
Dave