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@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@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