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

[Rene Herman]

On 04/12/2007 08:22 PM, Rask Ingemann Lambertsen wrote:

> Signed-off-by: Rask Ingemann Lambertsen <rask at sygehus.dk>

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.