On 04/12/2007 08:22 PM, Rask Ingemann Lambertsen wrote:
> Signed-off-by: Rask Ingemann Lambertsen <rask@xxxxxxxxxx>
Looking good, only some minimal comments...
> +++ linux-2.6.20.6-ril/Documentation/sound/alsa/ALSA-Configuration.txt 2007-04-07 17:31:38.000000000 +0200
> + Module snd-jazz16
> + -----------------
> +
> + Module for sound cards based on Media Vision Jazz16 chip (PnP only)
nitpick --> "on the [ ... ] chip" or "on [ ... ] chips" ?
> +++ linux-2.6.20.6-ril/sound/isa/Kconfig 2007-04-07 17:31:38.000000000 +0200
> +config SND_JAZZ16
> + tristate "Media Vision Jazz16"
> + depends on SND
> + select SND_OPL3_LIB
> + select SND_PCM
You will want to select SND_MPU401_UART here as well now that you use
the modular mpu401 uart support.
> + help
> + Say Y here to include support for Media Vision Jazz16 based
> + soundcards.
> +
> + You probably want to enable Jazz16 Plug and Play support
> + (CONFIG_JAZZ16PNP) also.
I'd prefer some stronger language (^$%##@!!) here. Something like:
"Unless you have a PNPBIOS supported Jazz16, you will need to enable
Jazz16 Plug and Play support (CONFIG_JAZZ16PNP) in the kernel as well"
> +++ linux-2.6.20.6-ril/sound/isa/sb/jazz16.c 2007-04-05 23:30:27.000000000 +0200
> +static irqreturn_t jazz16_interrupt(int irq, void *card)
> +{
> + return snd_sb8dsp_interrupt(card);
> +}
"card" is always the struct snd_card * in ALSA code. Could you call the
parameter "chip" instead?
> + 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 enable[dev_num] ? -ENOMEM : -ENODEV;
You should also guard against dev_num overrunning SNDRV_CARDS. How about:
static uint dev_num;
uint num;
if (dev_num < SNDRV_CARDS)
num = dev_num++; // wrap-around "danger" if unconditional ++
else
return -ENODEV;
if (!enable[num])
return -ENODEV;
card = snd_card_new(index[num], id[num], THIS_MODULE, 0);
if (!card)
return -ENOMEM;
note -- (some versions of) gcc don't place explicitly initialized
variables in BSS, even when the initialisation value is 0, so that
leaving an explicit = 0 out is kernel style.
That's all...
Rene.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
