[alsa-devel] [PATCH] Turtle Beach Multisound Classic/Pinnacle driver (3rd rev)

[Takashi Iwai]

At Thu, 22 Jan 2009 21:52:37 +0100,
Krzysztof Helt wrote:
>
> All issues pointed by Takashi are resolved. I moved 
> more code to the common part and made a common
> library.

Thanks, this one looks better now.

> 
> diff -urpN linux-rc5/sound/isa/msnd/Makefile linux-git/sound/isa/msnd/Makefile
> --- linux-rc5/sound/isa/msnd/Makefile	1970-01-01 01:00:00.000000000 +0100
> +++ linux-git/sound/isa/msnd/Makefile	2009-01-22 20:09:26.126460759 +0100
> @@ -0,0 +1,9 @@
> +
> +snd-msnd-lib-objs := msnd.o msnd_midi.o
> +snd-msnd-pinnacle-objs := msnd_pinnacle.o msnd_pinnacle_mixer.o 
> +snd-msnd-classic-objs := msnd_classic.o msnd_pinnacle_mixer.o 

msnd_pinnacle_mixer.c has global functions, and they would conflict.
Can't it be merged into snd-msnd-lib?  The ifdef part could be replaced
with if (some_modelcheck) {}

> --- linux-rc5/sound/isa/msnd/msnd.c	1970-01-01 01:00:00.000000000 +0100
> +++ linux-git/sound/isa/msnd/msnd.c	2009-01-22 21:19:43.283202276 +0100
> +int snd_msnd_upload_host(struct snd_msnd *dev, const u8 *bin, int len)
> +{
> +	int i;
> +
> +	if (len % 3 != 0) {
> +		snd_printk(LOGNAME ": Upload host data not multiple of 3!\n");

Put KERN_ERR.

> +static void snd_msnd_capture_reset_queue(struct snd_msnd *chip,
> +					 unsigned int pcm_periods,
> +					 unsigned int pcm_count)
...
> +	/* Critical section: bank 1 access. this is how the OSS driver does it:
> +	spin_lock_irqsave(&chip->lock, flags);
> +	outb(HPBLKSEL_1, chip->io + HP_BLKS);
> +	memset_io(chip->mappedbase, 0, DAR_BUFF_SIZE * 3);
> +	outb(HPBLKSEL_0, chip->io + HP_BLKS);
> +	spin_unlock_irqrestore(&chip->lock, flags);*/

If you'd disable multiple lines, better to use
#if 0 /* SOME REASONS */
...
#endif /* SOME REASONS */


> +static struct snd_pcm_hardware snd_msnd_playback = {
> +	.info =			SNDRV_PCM_INFO_MMAP |
> +				SNDRV_PCM_INFO_INTERLEAVED |
> +				SNDRV_PCM_INFO_MMAP_VALID,
> +	.formats =		SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S16_LE,
> +	.rates =		SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_8000_48000,

Remove KNOT.  Otherwise the rate restriction won't work.

> +	.rate_min =		8000,
> +	.rate_max =		48000,
> +	.channels_min =		1,
> +	.channels_max =		2,
> +	.buffer_bytes_max =	0x3000,
> +	.period_bytes_min =	0x40,
> +	.period_bytes_max =	0x1800,
> +	.periods_min =		2,
> +	.periods_max =		3,

Wow, I didn't notice this restriction...  Really up to 3 periods?

> --- linux-rc5/sound/isa/msnd/msnd.h	1970-01-01 01:00:00.000000000 +0100
> +++ linux-git/sound/isa/msnd/msnd.h	2009-01-22 21:17:09.125632934 +0100
...
> +#ifdef SLOWIO
> +#  undef outb
> +#  undef inb
> +#  define outb			outb_p
> +#  define inb			inb_p
> +#endif

This is too hackish.  If SLOWIO is only for alpha, and the driver is
configured to be X86-only, I'd suggest to get rid of this hack.

> --- linux-rc5/sound/isa/msnd/msnd_midi.c	1970-01-01 01:00:00.000000000 +0100
> +++ linux-git/sound/isa/msnd/msnd_midi.c	2009-01-22 18:35:00.942072608 +0100
> @@ -0,0 +1,179 @@
> +/*
> + *  Copyright (c) by Jaroslav Kysela <perex at perex.cz>
> + *  Routines for control of MPU-401 in UART mode

Better to put your name here and there, too...

> --- linux-rc5/sound/isa/msnd/msnd_pinnacle.c	1970-01-01 01:00:00.000000000 +0100
> +++ linux-git/sound/isa/msnd/msnd_pinnacle.c	2009-01-22 21:19:31.452624274 +0100

> +static void snd_msnd_eval_dsp_msg(struct snd_msnd *chip, u16 wMessage)
> +{
> +	switch (HIBYTE(wMessage)) {
> +	case HIMT_PLAY_DONE: {
> +		#ifdef CONFIG_SND_DEBUG0

It's essentially '#if 0', right?  Put it to the beginning of the line,
and make it obvious.

> +static int __devinit snd_msnd_probe(struct snd_card *card)
> +{
> +	struct snd_msnd *chip = card->private_data;
> +	unsigned char info;
> +#ifndef MSND_CLASSIC
> +	char *xv, *rev = NULL;
> +	char *pin = "TB Pinnacle", *fiji = "TB Fiji";
> +	char *pinfiji = "TB Pinnacle/Fiji";
> +#endif
> +
> +	if (!request_region(chip->io, DSP_NUMIO, "probing")) {
> +		snd_printk(KERN_ERR LOGNAME ": I/O port conflict\n");
> +		return -ENODEV;
> +	}
> +
> +	if (snd_msnd_reset_dsp(chip->io, &info) < 0) {
> +		release_region(chip->io, DSP_NUMIO);
> +		return -ENODEV;
> +	}
> +
> +#ifdef MSND_CLASSIC
> +	strcpy(card->shortname, "Classic/Tahiti/Monterey");
> +	printk(KERN_INFO LOGNAME ": %s, "
> +#else

This is too ugly...  Simply duplicate lines.  It's far more readable.


> +	chip->mappedbase = ioremap(chip->base, 0x8000);

Should it be cached or uncached?  Usually ioremap_nocache() is used.

> +	if (!chip->mappedbase) {
> +		printk(KERN_ERR LOGNAME
> +			": unable to map memory region 0x%lx-0x%lx\n",
> +			chip->base, chip->base + BUFFSIZE - 1);
> +		err = -EIO;
> +		goto err_release_region;
> +	}
> +	chip->mappedbase = ioremap(chip->base, 0x8000);

Why remapped twice?  A cut&paste error?

> +static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;	/* Index 0-MAX */
> +static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;	/* ID for this card */
> +
> +module_param_array(index, int, NULL, S_IRUGO);
> +MODULE_PARM_DESC(index, "Index value for msnd_pinnacle soundcard.");
> +module_param_array(id, charp, NULL, S_IRUGO);
> +MODULE_PARM_DESC(id, "ID string for msnd_pinnacle soundcard.");
> +
> +static long io[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_PORT;
> +static int irq[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_IRQ;
> +static long mem[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_PORT;
> +
> +static long cfg[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_PORT;

The modules parameters aren't __devinitdata.  Remove these
annotations.

> +#ifndef MSND_CLASSIC
> +/* Extra Peripheral Configuration (Default: Disable) */
> +static long ide_io0[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_PORT;
> +static long ide_io1[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_PORT;
> +static int ide_irq[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_IRQ;
> +
> +static long joystick_io[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_PORT;
> +/* If we have the digital daugherboard... */
> +static int digital[SNDRV_CARDS] __devinitdata;
> +
> +/* Extra Peripheral Configuration */
> +static int reset[SNDRV_CARDS] __devinitdata;
> +#endif
> +
> +static int write_ndelay[SNDRV_CARDS] __devinitdata =
> +						{ [0 ... (SNDRV_CARDS-1)] = 1 };
> +
> +#ifdef CONFIG_PNP
> +static int isapnp[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_ENABLE_PNP;
> +#endif

Ditto for all modules parameters.

> +#ifdef MODULE
> +MODULE_AUTHOR("Karsten Wiese <annabellesgarden at yahoo.de>");
> +MODULE_DESCRIPTION("Turtle Beach " LONGNAME " Linux Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_FIRMWARE(INITCODEFILE);
> +MODULE_FIRMWARE(PERMCODEFILE);

You don't need "#ifdef MODULE" for MODULE_*() and module_*() macros.

> +static int calibrate_signal __devinitdata;

No __devinitdata.


thanks,

Takashi