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

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