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@xxxxxxxx>
> + * 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@xxxxxxxx>");
> +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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
