[alsa-devel] [PATCH 2/2] Add support for Cyrix/NatSemi Geode SC5530 (VSA1)

[Takashi Iwai]

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