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@xxxxx>
>
> 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@xxxxx>
> ---
> 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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
