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