Hi,
thanks for the patches. The first one looks OK.
I see minor coding style issues in the second patch.
At Wed, 23 May 2007 15:50:38 -0500,
Ash Willis wrote:
>
> diff -r 5c25d0e8819c -r c9be7b8611e7 pci/cs5530.c
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/pci/cs5530.c Thu May 24 02:24:57 2007 +0100
> @@ -0,0 +1,296 @@
> +static int __devinit snd_cs5530_create(struct snd_card *card,
> + struct pci_dev *pci,
> + struct snd_cs5530 **rchip)
> +{
> + struct snd_cs5530 *chip;
> + unsigned long sb_base;
> + u8 irq, dma8, dma16 = 0;
> + u16 map;
> + void __iomem *mem;
> + int err;
> +
> + static struct snd_device_ops ops = {
> + .dev_free = snd_cs5530_dev_free,
> + };
> + *rchip = NULL;
> +
> + if ((err = pci_enable_device(pci)) < 0)
> + return err;
Avoid the style "if ((err = xx) < 0)" in new codes as much as
possible. Just write:
err = pci_enable(pci);
if (err < 0)
return err;
Now the kernel coding style recommends this.
> + if ((err = pci_request_regions(pci, "CS5530")) < 0) {
> + kfree(chip);
> + pci_disable_device(pci);
> + return err;
> + }
Ditto.
> + chip->pci_base = pci_resource_start(pci, 0);
> +
> + mem = ioremap_nocache(chip->pci_base, pci_resource_len(pci, 0));
Check the return value from ioremap(), just to be sure.
> + map = readw(mem + 0x18);
> + iounmap(mem);
> +
> + /* Map bits
> + 0:1 * 0x20 + 0x200 = sb base
> + 2 sb enable
> + 3 adlib enable
> + 5 MPU enable 0x330
> + 6 MPU enable 0x300
> +
> + The other bits may be used internally so must be masked */
> +
> + sb_base = 0x220 + 0x20 * (map & 3);
> +
> + if (map & (1<<2))
> + printk(KERN_INFO "CS5530: XpressAudio at 0x%lx\n", sb_base);
> + else
> + {
> + printk(KERN_ERR "Could not find XpressAudio!\n");
> + snd_cs5530_free(chip);
> + return -ENODEV;
> + }
Coding style issue: Put "{' in the same line as "else".
Also, you can use goto as a single error handling point.
For example,
if (err < 0) {
some_error_message();
goto error;
}
...
return 0;
error:
snd_cs5530_free(chip);
return err;
}
This is a matter of taste, IMO, though.
> +
> + if (map & (1<<5))
> + printk(KERN_INFO "CS5530: MPU at 0x300\n");
> + else if (map & (1<<6))
> + printk(KERN_INFO "CS5530: MPU at 0x330\n");
> +
> + irq = snd_cs5530_mixer_read(sb_base, 0x80) & 0x0F;
> + dma8 = snd_cs5530_mixer_read(sb_base, 0x81);
> +
> + if (dma8 & 0x20)
> + dma16 = 5;
> + else if (dma8 & 0x40)
> + dma16 = 6;
> + else if (dma8 & 0x80)
> + dma16 = 7;
> + else
> + {
> + printk(KERN_ERR "CS5530: No 16bit DMA enabled\n");
> + snd_cs5530_free(chip);
> + return -ENODEV;
> + }
Ditto.
(BTW, is dma16 really necessary to be zero-initialized at beginning?)
> +
> + if (dma8 & 0x01)
> + dma8 = 0;
> + else if (dma8 & 02)
> + dma8 = 1;
> + else if (dma8 & 0x08)
> + dma8 = 3;
> + else
> + {
> + printk(KERN_ERR "CS5530: No 8bit DMA enabled\n");
> + snd_cs5530_free(chip);
> + return -ENODEV;
> + }
Ditto.
> +
> + if (irq & 1)
> + irq = 9;
> + else if (irq & 2)
> + irq = 5;
> + else if (irq & 4)
> + irq = 7;
> + else if (irq & 8)
> + irq = 10;
> + else
> + {
> + printk(KERN_ERR "CS5530: SoundBlaster IRQ not set\n");
> + snd_cs5530_free(chip);
> + return -ENODEV;
> + }
Ditto.
> + printk(KERN_INFO "CS5530: IRQ: %d DMA8: %d DMA16: %d\n", irq, dma8,
> + dma16);
> +
> + if ((err = snd_sbdsp_create(card, sb_base, irq, snd_sb16dsp_interrupt,
> + dma8, dma16, SB_HW_CS5530, &chip->sb)) < 0) {
> + printk(KERN_ERR "CS5530: Could not create SoundBlaster\n");
> + snd_cs5530_free(chip);
> + return err;
> + }
Split err.
> + if ((err = snd_sb16dsp_pcm(chip->sb, 0, &chip->sb->pcm)) < 0) {
> + printk(KERN_ERR "CS5530: Could not create PCM\n");
> + snd_cs5530_free(chip);
> + return err;
> + }
Split err.
> +
> + if ((err = snd_sbmixer_new(chip->sb)) < 0) {
> + printk(KERN_ERR "CS5530: Could not create Mixer\n");
> + snd_cs5530_free(chip);
> + return err;
> + }
Split err.
> + if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) {
> + snd_cs5530_free(chip);
> + return err;
> + }
Split err.
> + snd_card_set_dev(card, &pci->dev);
> + *rchip = chip;
> + return 0;
> +}
> +
> +static int __devinit snd_cs5530_probe(struct pci_dev *pci,
> + const struct pci_device_id *pci_id)
> +{
> + static int dev;
> + struct snd_card *card;
> + struct snd_cs5530 *chip = NULL;
> + int err;
> +
> + if (dev >= SNDRV_CARDS)
> + return -ENODEV;
> + if (!enable[dev]) {
> + dev++;
> + return -ENOENT;
> + }
> +
> + card = snd_card_new(index[dev], id[dev], THIS_MODULE, 0);
> +
> + if (card == NULL)
> + return -ENOMEM;
> +
> + if ((err = snd_cs5530_create(card, pci, &chip)) < 0) {
> + snd_card_free(card);
> + return err;
> + }
Split err.
> + strcpy(card->driver, "CS5530");
> + strcpy(card->shortname, "CS5530 Audio");
> + sprintf(card->longname, "%s at 0x%lx", card->shortname, chip->pci_base);
> +
> + if ((err = snd_card_register(card)) < 0) {
> + snd_card_free(card);
> + return err;
> + }
Split err.
Is the driver confirmed to work with the recent kernel?
If yes, we can put cs5530.c to alsa-kernel so that it'll be merged to
the next linux kernel. alsa-driver tree still needs cs5530.c just
including alsa-kernel code, though.
Also, don't forget to add a description to ALSA-Configuration.txt.
Could you fix these issues and resend the patch?
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
