Re: [alsa-devel] [PATCH] sgalaxy: checkpatch fixes

On 09/04/2007 11:49 PM, Krzysztof Helt wrote:

A few comments, but also note that while I've been "dragging my feet" (grin) on getting the thing tested on more cards, I've a new driver for these cards in the works. Was already posted a while ago -- just need to clean it up, integrate some more and then test and submit it for real...

From: Krzysztof Helt krzysztof.h1@wp.pl

This patch fixes white spaces and errors pointed by the checkpatch.pl script.

That thing is not actually intended for existing code...

Signed-off-by: Krzysztof Helt krzysztof.h1@wp.pl

A nice side effect: sgalaxy.c gets shorter.

diff -urp linux-2.6.23.orig/sound/isa/sgalaxy.c linux-2.6.23/sound/isa/sgalaxy.c --- linux-2.6.23.orig/sound/isa/sgalaxy.c 2007-07-09 01:32:17.000000000 +0200 +++ linux-2.6.23/sound/isa/sgalaxy.c 2007-09-04 23:41:11.860030310 +0200 @@ -47,7 +47,8 @@ static int index[SNDRV_CARDS] = SNDRV_DE static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR; /* ID for this card */ static int enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE; /* Enable this card */ static long sbport[SNDRV_CARDS] = SNDRV_DEFAULT_PORT; /* 0x220,0x240 */ -static long wssport[SNDRV_CARDS] = SNDRV_DEFAULT_PORT; /* 0x530,0xe80,0xf40,0x604 */ +static long wssport[SNDRV_CARDS] = SNDRV_DEFAULT_PORT;

• 				/* 0x530,0xe80,0xf40,0x604 */
  

Please don't. The idea of coding style fixes is to make things _easier_ to read.

static int irq[SNDRV_CARDS] = SNDRV_DEFAULT_IRQ; /* 7,9,10,11 */ static int dma1[SNDRV_CARDS] = SNDRV_DEFAULT_DMA; /* 0,1,3 */

@@ -69,14 +70,10 @@ MODULE_PARM_DESC(dma1, "DMA1 # for Sound

#define PFX "sgalaxy: "

-/* +#define AD1848P1(port, x) (port + c_d_c_AD1848##x)

• */

-#define AD1848P1( port, x ) ( port + c_d_c_AD1848##x )

-/* from lowlevel/sb/sb.c - to avoid having to allocate a struct snd_sb for the */ -/* short time we actually need it.. */ +/* from lowlevel/sb/sb.c - to avoid having to allocate a struct snd_sb */ +/* for the short time we actually need it.. */

static int snd_sgalaxy_sbdsp_reset(unsigned long port) { @@ -98,7 +95,7 @@ static int __devinit snd_sgalaxy_sbdsp_c unsigned char val) { int i;

• for (i = 10000; i; i--) if ((inb(SBP1(port, STATUS)) & 0x80) == 0) { outb(val, SBP1(port, COMMAND));

@@ -115,45 +112,39 @@ static irqreturn_t snd_sgalaxy_dummy_int

static int __devinit snd_sgalaxy_setup_wss(unsigned long port, int irq, int dma) {

• static int interrupt_bits[] = {-1, -1, -1, -1, -1, -1, -1, 0x08, -1,

• static int interrupt_bits[] = {-1, -1, -1, -1, -1, -1, -1, 0x08, -1, 0x10, 0x18, 0x20, -1, -1, -1, -1}; static int dma_bits[] = {1, 2, 0, 3}; int tmp, tmp1;

• if ((tmp = inb(port + 3)) == 0xff)
• {

• tmp = inb(port + 3);
• if (tmp == 0xff) { snd_printdd("I/O address dead (0x%lx)\n", port); return 0; }

-#if 0

• snd_printdd("WSS signature = 0x%x\n", tmp);

-#endif

Please don't just kill debug code. It's very useful for the next person stepping in -- it's "functional commentary".

•    if ((tmp & 0x3f) != 0x04 &&
  

•        (tmp & 0x3f) != 0x0f &&
  

•        (tmp & 0x3f) != 0x00) {
  

• tmp &= 0x3f;
• if (tmp != 0x04 && tmp != 0x0f && tmp != 0x00) { snd_printdd("No WSS signature detected on port 0x%lx\n", port + 3); return 0; }

-#if 0

• snd_printdd(PFX "setting up IRQ/DMA for WSS\n");

-#endif

Same.

•    /* initialize IRQ for WSS codec */
  

•    tmp = interrupt_bits[irq % 16];
  

•    if (tmp < 0)
  

•            return -EINVAL;
  

• if (request_irq(irq, snd_sgalaxy_dummy_interrupt, IRQF_DISABLED, "sgalaxy", NULL)) {

• /* initialize IRQ for WSS codec */
• tmp = interrupt_bits[irq % 16];
• if (tmp < 0)

• return -EINVAL;
  

• tmp = request_irq(irq, snd_sgalaxy_dummy_interrupt, IRQF_DISABLED,

• 	  "sgalaxy", NULL);
  

• if (tmp) {

NAK -- you just overwrote tmp with the return value from request_irq() meaning we're now poking garbage:

snd_printk(KERN_ERR "sgalaxy: can't grab irq %d\n", irq);
return -EIO;

}

•    outb(tmp | 0x40, port);
  

•    tmp1 = dma_bits[dma % 4];
  

•    outb(tmp | tmp1, port);
  

• outb(tmp | 0x40, port);

• tmp1 = dma_bits[dma % 4];

• outb(tmp | tmp1, port);

  free_irq(irq, NULL);

@@ -162,10 +153,6 @@ static int __devinit snd_sgalaxy_setup_w

static int __devinit snd_sgalaxy_detect(int dev, int irq, int dma) { -#if 0

• snd_printdd(PFX "switching to WSS mode\n");

-#endif

Same.

• /* switch to WSS mode */ snd_sgalaxy_sbdsp_reset(sbport[dev]);

@@ -177,8 +164,10 @@ static int __devinit snd_sgalaxy_detect( }

static struct ad1848_mix_elem snd_sgalaxy_controls[] = { -AD1848_DOUBLE("Aux Playback Switch", 0, SGALAXY_AUXC_LEFT, SGALAXY_AUXC_RIGHT, 7, 7, 1, 1), -AD1848_DOUBLE("Aux Playback Volume", 0, SGALAXY_AUXC_LEFT, SGALAXY_AUXC_RIGHT, 0, 0, 31, 0) +AD1848_DOUBLE("Aux Playback Switch", 0, SGALAXY_AUXC_LEFT, SGALAXY_AUXC_RIGHT,

• 7, 7, 1, 1),
  

+AD1848_DOUBLE("Aux Playback Volume", 0, SGALAXY_AUXC_LEFT, SGALAXY_AUXC_RIGHT,

• 0, 0, 31, 0)
  

I'd leave these on single lines again.

};

static int __devinit snd_sgalaxy_mixer(struct snd_ad1848 *chip) @@ -190,28 +179,34 @@ static int __devinit snd_sgalaxy_mixer(s

memset(&id1, 0, sizeof(id1)); memset(&id2, 0, sizeof(id2));

• id1.iface = id2.iface = SNDRV_CTL_ELEM_IFACE_MIXER;

• id1.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
• id2.iface = SNDRV_CTL_ELEM_IFACE_MIXER; /* reassign AUX0 to LINE */ strcpy(id1.name, "Aux Playback Switch"); strcpy(id2.name, "Line Playback Switch");

• if ((err = snd_ctl_rename_id(card, &id1, &id2)) < 0)

• err = snd_ctl_rename_id(card, &id1, &id2);
• if (err < 0) return err; strcpy(id1.name, "Aux Playback Volume"); strcpy(id2.name, "Line Playback Volume");

• if ((err = snd_ctl_rename_id(card, &id1, &id2)) < 0)

• err = snd_ctl_rename_id(card, &id1, &id2);
• if (err < 0) return err; /* reassign AUX1 to FM */ strcpy(id1.name, "Aux Playback Switch"); id1.index = 1; strcpy(id2.name, "FM Playback Switch");

• if ((err = snd_ctl_rename_id(card, &id1, &id2)) < 0)

• err = snd_ctl_rename_id(card, &id1, &id2);
• if (err < 0) return err; strcpy(id1.name, "Aux Playback Volume"); strcpy(id2.name, "FM Playback Volume");

• if ((err = snd_ctl_rename_id(card, &id1, &id2)) < 0)

• err = snd_ctl_rename_id(card, &id1, &id2);
• if (err < 0) return err; /* build AUX2 input */ for (idx = 0; idx < ARRAY_SIZE(snd_sgalaxy_controls); idx++) {

• if ((err = snd_ad1848_add_ctl_elem(chip, &snd_sgalaxy_controls[idx])) < 0)
  

• err = snd_ad1848_add_ctl_elem(chip, &snd_sgalaxy_controls[idx]);
  

• if (err < 0)
  return err;
  

  } return 0;

@@ -241,44 +236,50 @@ static int __devinit snd_sgalaxy_probe(s struct snd_ad1848 *chip;

card = snd_card_new(index[dev], id[dev], THIS_MODULE, 0);

• if (card == NULL)

• if (!card) return -ENOMEM;

  xirq = irq[dev]; if (xirq == SNDRV_AUTO_IRQ) {

• if ((xirq = snd_legacy_find_free_irq(possible_irqs)) < 0) {
  

• xirq = snd_legacy_find_free_irq(possible_irqs);
  

• if (xirq < 0) {
  snd_printk(KERN_ERR PFX "unable to find a free IRQ\n");
  err = -EBUSY;
  goto _err;
  

  } } xdma1 = dma1[dev];

•    if (xdma1 == SNDRV_AUTO_DMA) {
  

• if ((xdma1 = snd_legacy_find_free_dma(possible_dmas)) < 0) {
  

• if (xdma1 == SNDRV_AUTO_DMA) {

• xdma1 = snd_legacy_find_free_dma(possible_dmas);
  

• if (xdma1 < 0) {
  snd_printk(KERN_ERR PFX "unable to find a free DMA\n");
  err = -EBUSY;
  goto _err;
  

  } }

• if ((err = snd_sgalaxy_detect(dev, xirq, xdma1)) < 0)

• err = snd_sgalaxy_detect(dev, xirq, xdma1);
• if (err < 0) goto _err;

• if ((err = snd_ad1848_create(card, wssport[dev] + 4,

• 		     xirq, xdma1,
  

• 		     AD1848_HW_DETECT, &chip)) < 0)
  

• err = snd_ad1848_create(card, wssport[dev] + 4, xirq, xdma1,

• 		AD1848_HW_DETECT, &chip);
  

• if (err < 0) goto _err; card->private_data = chip;

• if ((err = snd_ad1848_pcm(chip, 0, NULL)) < 0) {

• err = snd_ad1848_pcm(chip, 0, NULL);
• if (err < 0) { snd_printdd(PFX "error creating new ad1848 PCM device\n"); goto _err; }

• if ((err = snd_ad1848_mixer(chip)) < 0) {

• err = snd_ad1848_mixer(chip);
• if (err < 0) { snd_printdd(PFX "error creating new ad1848 mixer\n"); goto _err; }

• if ((err = snd_sgalaxy_mixer(chip)) < 0) {

• err = snd_sgalaxy_mixer(chip);
• if (err < 0) { snd_printdd(PFX "the mixer rewrite failed\n"); goto _err; }

@@ -290,7 +291,8 @@ static int __devinit snd_sgalaxy_probe(s

snd_card_set_dev(card, devptr);

• if ((err = snd_card_register(card)) < 0)

• err = snd_card_register(card);

• if (err < 0) goto _err;

  dev_set_drvdata(devptr, card);

@@ -327,7 +329,8 @@ static int snd_sgalaxy_resume(struct dev

chip->resume(chip); snd_ad1848_out(chip, SGALAXY_AUXC_LEFT, chip->image[SGALAXY_AUXC_LEFT]);

• snd_ad1848_out(chip, SGALAXY_AUXC_RIGHT, chip->image[SGALAXY_AUXC_RIGHT]);

• snd_ad1848_out(chip, SGALAXY_AUXC_RIGHT,

• 	chip->image[SGALAXY_AUXC_RIGHT]);
  

Definitely would keep this on one line.

snd_power_change_state(card, SNDRV_CTL_POWER_D0); return 0;

Rene.