Re: [alsa-devel] [PATCH] ALSA: Support Media Vision Jazz16 chips

On 03/26/2007 11:23 PM, Rask Ingemann Lambertsen wrote:

Fundamental model questions still waiting on reply from Adam belay...

--- linux-2.6.20/sound/isa/sb/jazz16.c-orig 2007-02-17 20:01:40.000000000 +0100 +++ linux-2.6.20/sound/isa/sb/jazz16.c 2007-03-18 13:43:35.000000000 +0100 @@ -0,0 +1,193 @@ +/* Driver for Media Vision Jazz16 based boards.

• • Copyright (c) 2007 by Rask Ingemann Lambertsen
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License as published by
• • the Free Software Foundation; either version 2 of the License, or
• • (at your option) any later version.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• • You should have received a copy of the GNU General Public License
• • along with this program; if not, write to the Free Software
• • Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
• • The Jazz16 is an SB Pro compatible chip with a 16-bit mode and higher
• • playback and capture rates added to it. It is not SB 16 compatible.
• • There is also an MPU-401 interface.
• • The IBM PPS Model 6015 has a Jazz16 chip on board. Please tell me if it
• • works with this driver or not.
• */

+#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/kernel.h> +#include <linux/errno.h> +#include <linux/slab.h> +#include <linux/init.h> +#include <linux/pnp.h> +#include <linux/ioport.h> +#include <asm/io.h> +#include <linux/delay.h> +#include <sound/driver.h>

ALSA drivers usually have sound/driver.h as the first include. I'm not sure how sensible that is...

+#include <sound/core.h> +#include <sound/sb.h> +#include <sound/mpu401.h> +#include <sound/opl3.h> +#include <sound/initval.h>

+MODULE_AUTHOR ("Rask Ingemann Lambertsen rask@sygehus.dk"); +MODULE_DESCRIPTION ("Jazz16");

No spaces before the ( please.

+MODULE_LICENSE("GPL");

+static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX; /* Index 0-MAX */ +static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR; /* ID for this card */ +static int enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE_PNP; /* Enable switches */

+module_param_array(index, int, NULL, 0444); +MODULE_PARM_DESC(index, "Index value for Jazz16 soundcard."); +module_param_array(id, charp, NULL, 0444); +MODULE_PARM_DESC(id, "ID string for Jazz16 soundcard."); +module_param_array(enable, bool, NULL, 0444); +MODULE_PARM_DESC(enable, "Enable Jazz16 soundcard.");

+#define PFX "jazz16: " +#define PFX_WARN KERN_WARNING PFX +#define PFX_ERR KERN_ERR PFX

+static struct pnp_driver snd_jazz16_pnp_driver;

+static irqreturn_t jazz16_interrupt(int irq, void *dev_id) +{

• struct snd_sb *chip = (struct snd_sb *) dev_id;

Please don't cast void pointers, it's useless (and a matter of kernel coding style).

• return snd_sb8dsp_interrupt(chip);

+}

+static struct pnp_device_id snd_jazz16_pnpids[] = {

• { .id = "PNPb00f" },
• { .id = "" }

+}; +MODULE_DEVICE_TABLE(pnp, snd_jazz16_pnpids);

+static int __devinit snd_jazz16_pnp_probe(struct pnp_dev *pnp_dev,

• 			  const struct pnp_device_id *pnp_id)
  

+{ struct snd_sb *chip;

• struct snd_card *card;
• struct snd_opl3 *opl3;
• int err;
• uint sbport, sbirq, sbdma8, sbdma16, mpuport, mpuirq;
• static uint dev_num = 0;
• if (enable[dev_num])

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

• else

• card = NULL;
  

• dev_num ++;
• if (card == NULL)

• return -ENOMEM;
  

-ENOMEM is not a very good return value for !enable. -ENODEV would be better. Also please dev_num++ without the space.

The hardware fundamentally supports just one card per system due to the fixed config port, so you might as well get rid of the dev_num (and the SNDRV_CARDS-wide arrays) it seems?. If you want to keep them that way just so that it's more generic looking... sure.

• pnp_set_drvdata(pnp_dev, card);
• /* TODO use pnp_port_valid(), pnp_port_flags(), pnp_port_length()... */
• sbport = pnp_port_start(pnp_dev, 0);
• sbirq = pnp_irq(pnp_dev, 0);
• sbdma8 = pnp_dma(pnp_dev, 0);
• sbdma16 = pnp_dma(pnp_dev, 1);
• err = snd_sbdsp_create(card, sbport, sbirq, jazz16_interrupt,

• 	       sbdma8, sbdma16, SB_HW_AUTO, &chip);
  

• if (err < 0) {

• snd_card_free(card);
  

• return err;
  

• }

The "snd_card_free(card); return err;" error exit is repated many times. Personally I don't really mind, but general kernel coding style is doing that with a goto to an error exit. Yes, as long as you're careful the compiler will optimize to that anyway, but for example Andrew Morton would slap you for it.

• /* TODO length limits - strncpy()/snprintf() and friends. */

That's not really needed. You know how wide the buffers and your strings are, overrunning them is just a driver bug; it's not something that can be influenced from userland or anything.

• strcpy(card->driver, "Jazz16");
• strcpy(card->shortname, "Media Vision Jazz16");
• sprintf(card->longname, "%s at 0x%x, irq %u, dma8 %u, dma16 %u",

• chip->name, sbport, sbirq, sbdma8, sbdma16);
  

Could you use %#x instead of 0x%x? People went through the pain of adding pretty-printing to printk() so might as well use it...

• if (chip->hardware != SB_HW_JAZZ16) {

• snd_printk(PFX_ERR "Not a Jazz16 chip at 0x%x.\n", sbport);
  

Ditto.

• snd_card_free(card);
  

• return -ENODEV;
  

• }
• err = snd_sb8dsp_pcm(chip, 0, NULL);
• if (err < 0) {

• snd_card_free(card);
  

• return err;
  

• }
• err = snd_sbmixer_new(chip);
• if (err < 0) {

• snd_card_free(card);
  

• return err;
  

• }
• err = snd_opl3_create(card, sbport, sbport + 2,

• 	      OPL3_HW_AUTO, 1, &opl3);
  

• if (err < 0) {

• snd_printk(PFX_WARN "No OPL device found, skipping.\n");
  

• } else {

Single statement branches don't have braces in the kernel coding style.

• err = snd_opl3_timer_new(opl3, 1, 2);
  

• if (err < 0) {
  

• 	snd_card_free(card);
  

• 	return err;
  

• }
  

• err = snd_opl3_hwdep_new(opl3, 0, 1, NULL);
  

• if (err < 0) {
  

• 	snd_card_free(card);
  

• 	return err;
  

• }
  

• }
• if (pnp_port_valid(pnp_dev, 1)

•    && !(pnp_port_flags(pnp_dev, 1) & IORESOURCE_DISABLED)) {
  

• int mpuirqflags;
  

•    	if (!pnp_irq_valid(pnp_dev, 1)
  

•    	    || pnp_irq_flags(pnp_dev, 1) & IORESOURCE_DISABLED) {
  

•    		mpuirq = -1;
  

•    		mpuirqflags = 0;
  

•    	} else {
  

•    		mpuirq = pnp_irq(pnp_dev, 1);
  

•    		mpuirqflags = IRQF_DISABLED;
  

•    	}
  

•    	mpuport = pnp_port_start(pnp_dev, 1);
  

•    	err = snd_mpu401_uart_new(card, 0, MPU401_HW_MPU401,
  

• 			  mpuport, 0, mpuirq,
  

• 			  mpuirqflags, NULL);
  

• if (err < 0) {
  

• 	snd_printk(PFX_WARN "MPU-401 device not configured, skipping.\n");
  

• }
  

Braces again.

I like what you do with the MPU401 in this version better . If it can only be configured when the SB part is active it's fundamentally part of the card, and not something to be driven by the seperate snd-mpu401.

• }

I believe you need a snd_card_set_dev(card, &pnp_dev->dev) here.

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

• snd_card_free(card);
  

• return err;
  

• }
• return 0;

+}

+static void __devexit snd_jazz16_pnp_remove(struct pnp_dev *dev) +{

• snd_card_free(pnp_get_drvdata(dev));
• pnp_set_drvdata(dev, NULL);

+}

+static struct pnp_driver snd_jazz16_pnp_driver = {

• .name = "Jazz16",
• .id_table = snd_jazz16_pnpids,
• .probe = snd_jazz16_pnp_probe,
• .remove = __devexit_p(snd_jazz16_pnp_remove),

+};

+static int __devinit alsa_card_jazz16_init (void)

No space before the (void) please.

+{

• return pnp_register_driver(&snd_jazz16_pnp_driver);

+}

+static void __devexit alsa_card_jazz16_exit(void) +{

• pnp_unregister_driver(&snd_jazz16_pnp_driver);

+}

+module_init (alsa_card_jazz16_init); +module_exit (alsa_card_jazz16_exit);

Nor after the init/exit.

I didn't look at the changes to the low-level sb code; I've too little experience there. Hope this was useful though.

And hope Adam comments on the model...

Rene.