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

[Rene Herman]

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 at 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.