Re: [PATCH] Gallant SC-6000 driver (3rd version)

[Rene Herman <rene.herman@gmail.com>]

On 09/04/2007 09:11 PM, Krzysztof Helt wrote:

By the way -- something along the email path to you rejects my messages due 
to taking an SPF-neutral result from gmail as negative...

> The two issues pointed by Takashi are fixed.

[ ... ]

> +#include <linux/module.h>
> +#include <linux/isa.h>
> +#include <asm/dma.h>
> +#include <sound/driver.h>

He probably forgot to request that sound/driver.h come first. It's an issue 
with fixing up things for 2.4 compiles with the standalone driver package.

> +#include <sound/core.h>
> +#include <sound/ad1848.h>
> +#include <sound/opl3.h>
> +#include <sound/mpu401.h>
> +#include <sound/control.h>
> +#define SNDRV_LEGACY_FIND_FREE_IRQ
> +#define SNDRV_LEGACY_FIND_FREE_DMA
> +#include <sound/initval.h>

Personally I rather detest ISA autoprobing, so if you only included it as a 
"I see that the ALSA thing to do", feel free to try without as far as I'm 
concerned...

> +#define DSP_RESET	0x06	/* offset of DSP RESET		(wo) */
> +#define DSP_READ	0x0a	/* offset of DSP READ		(ro) */
> +#define DSP_WRITE	0x0c	/* offset of DSP WRITE		(w-) */
> +#define DSP_COMMAND	0x0c	/* offset of DSP COMMAND	(w-) */
> +#define DSP_STATUS	0x0c	/* offset of DSP STATUS		(r-) */
> +#define DSP_DATAVAIL	0x0e	/* offset of DSP DATA AVAILABLE	(ro) */

One of these days I'm going to stick that snd_sbdsp_reset() in the midlayer...

> +
> +#define PFX "sc6000: "
> +
> +/* hardware dependent functions */
> +
> +/*
> + * sc6000_irq_to_softcfg - Decode irq number into cfg code.
> + */
> +static inline unsigned char sc6000_irq_to_softcfg(int irq)
> +{
> +	unsigned char val = 0;
> +
> +	switch (irq) {
> +	case 5:
> +		val = 0x28;
> +		break;
> +	case 7:
> +		val = 0x8;
> +		break;
> +	case 9:
> +		val = 0x10;
> +		break;
> +	case 10:
> +		val = 0x18;
> +		break;
> +	case 11:
> +		val = 0x20;
> +		break;
> +	default:
> +		break;
> +	}
> +	return val;
> +}
> +
> +/*
> + * sc6000_dma_to_softcfg - Decode dma number into cfg code.
> + */
> +static inline unsigned char sc6000_dma_to_softcfg(int dma)
> +{
> +	unsigned char val = 0;
> +
> +	switch (dma) {
> +	case 0:
> +		val = 1;
> +		break;
> +	case 1:
> +		val = 2;
> +		break;
> +	case 3:
> +		val = 3;
> +		break;
> +	default:
> +		break;
> +	}
> +	return val;
> +}

Deja vu from sgalaxy...

But also -- please no inlines. There's no point to them. Kernel policy is 
basically "no inlines other than for things that would definitely be macros 
if we weren't fond of the typesafety inline functions give". GCC will figure 
things out on its own...

> +static int __devinit snd_sc6000_detect(int dev, int irq, int dma)
> +{
> +	int err = -ENODEV;
> +
> +	snd_printd("Initializing BASE[0x%lx] IRQ[%d] DMA[%d] MIRQ[%d]\n",
> +		   port[dev], irq, dma,
> +		   mpu_irq[dev] == SND_AUTO_IRQ ? 0 : mpu_irq[dev]);
> +
> +/*
> + * We must request the port region because these are the I/O
> + * ports to access card's control registers.
> + */
> +	if (!request_region(port[dev], 0x10, "sc-6000 (base)")) {
> +		snd_printk(KERN_ERR
> +			   "SC-6000 port I/O port region is already in use.\n");
> +		return -EBUSY;
> +	}
> +
> +	err = sc6000_init_board(port[dev], irq, dma,
> +				mss_port[dev], mpu_irq[dev]);
> +
> +	release_region(port[dev], 0x10);

So what happens if now someone starts poking at port[dev]? You probably want 
to keep these requested no?

> +static int __devinit snd_sc6000_match(struct device *devptr, unsigned int dev)
> +{
> +	if (!enable[dev])
> +		return 0;
> +	if (port[dev] == SNDRV_AUTO_PORT) {
> +		printk(KERN_ERR PFX "specify IO port\n");
> +		return 0;
> +	}
> +	if (mss_port[dev] == SNDRV_AUTO_PORT) {
> +		printk(KERN_ERR PFX "specify MSS port\n");
> +		return 0;
> +	}
> +	if (port[dev] != 0x220 && port[dev] != 0x240) {
> +		printk(KERN_ERR PFX "Port must be 0x220 or 0x240\n");
> +		return 0;
> +	}
> +	if (mss_port[dev] != 0x530 && mss_port[dev] != 0xe80) {
> +		printk(KERN_ERR PFX "MSS port must be 0x530 or 0xe80\n");
> +		return 0;
> +	}
> +	if (irq[dev] != SNDRV_AUTO_IRQ && !sc6000_irq_to_softcfg(irq[dev])) {
> +		printk(KERN_ERR PFX "invalid IRQ %d\n", irq[dev]);
> +		return 0;
> +	}
> +	if (dma[dev] != SNDRV_AUTO_DMA && !sc6000_dma_to_softcfg(dma[dev])) {
> +		printk(KERN_ERR PFX "invalid DMA %d\n", dma[dev]);
> +		return 0;
> +	}
> +	if (mpu_port[dev] != SNDRV_AUTO_PORT &&
> +	    (mpu_port[dev] & ~0x30l) != 0x300) {

Eh, what?

> +		printk(KERN_ERR PFX "invalid MPU-401 port %lx\n",
> +			mpu_port[dev]);
> +		return 0;
> +	}
> +	if (mpu_port[dev] != SNDRV_AUTO_PORT &&
> +	    mpu_irq[dev] != SNDRV_AUTO_IRQ && mpu_irq[dev] != 0 &&
> +	    !sc6000_mpu_irq_to_softcfg(mpu_irq[dev])) {
> +		printk(KERN_ERR PFX "invalid MPU-401 IRQ %d\n", mpu_irq[dev]);
> +		return 0;
> +	}
> +	return 1;
> +}
> +
> +static int __devinit snd_sc6000_probe(struct device *devptr, unsigned int dev)
> +{
> +	static int possible_irqs[] = { 7, 9, 10, 11, -1 };

In the above, IRQ 5 also seems to be allowed. This is all rather massive 
sgalaxy deja-vu...

Rene.