[alsa-devel] [PATCH] Turtle Beach Multisound Classic/Pinnacle driver (2nd rev)

[Takashi Iwai]

At Thu, 22 Jan 2009 10:32:08 +0100,
Krzysztof Helt wrote:
> 
> diff --git a/sound/isa/Kconfig b/sound/isa/Kconfig
> index 660beb4..2d6ce32 100644
> --- a/sound/isa/Kconfig
> +++ b/sound/isa/Kconfig
> @@ -411,5 +411,36 @@ config SND_WAVEFRONT_FIRMWARE_IN_KERNEL
>  	  you need to install the firmware files from the
>  	  alsa-firmware package.
>  
> +config SND_MSND_PINNACLE
> +	tristate "Turtle Beach MultiSound Pinnacle/Fiji driver"
> +	depends on X86

Better to add depends on EXPERIMENTAL at this stage.

> +config SND_MSND_CLASSIC
> +	tristate "Support for Turtle Beach MultiSound Classic, Tahiti, Monterey"
> +	depends on X86

Ditto.

> diff --git a/sound/isa/msnd/Makefile b/sound/isa/msnd/Makefile
> new file mode 100644
> index 0000000..41f278f
> --- /dev/null
> +++ b/sound/isa/msnd/Makefile
> @@ -0,0 +1,10 @@
> +
> +snd-msnd-pinnacle-objs := msnd_pinnacle.o msnd_pinnacle_mixer.o \
> +			  msnd.o msnd_midi.o
> +snd-msnd-classic-objs := msnd_classic.o msnd_pinnacle_mixer.o \
> +			 msnd.o msnd_midi.o
> +
> +# Toplevel Module Dependency
> +obj-$(CONFIG_SND_MSND_PINNACLE) += snd-msnd-pinnacle.o
> +obj-$(CONFIG_SND_MSND_CLASSIC) += snd-msnd-classic.o

Well, this won't work if both modules are loaded the same time...
Put the common codes into a library module such as snd-msnd-lib.
In this case, you can't use ifdef MSND_CLASSIC in the library module.
It must be checked dynamically.

Or, a typical workaround (e.g. for echoaudio or au88x0) is to include
all sources into one piece and make all functions static.


> --- /dev/null
> +++ b/sound/isa/msnd/msnd.c
(snip)
> +int snd_msnd_send_dsp_cmd(struct snd_msnd *dev, u8 cmd)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	if (snd_msnd_wait_HC0(dev) == 0) {
> +		outb(cmd, dev->io + HP_CVR);
> +		spin_unlock_irqrestore(&dev->lock, flags);
> +		return 0;
> +	}
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +
> +	snd_printd(LOGNAME ": Send DSP command timeout\n");

Put a proper KERN_* prefix.

> +int snd_msnd_send_word(struct snd_msnd *dev, unsigned char high,
> +		   unsigned char mid, unsigned char low)
> +{
> +	unsigned int io = dev->io;
> +
> +	if (snd_msnd_wait_TXDE(dev) == 0) {
> +		outb(high, io + HP_TXH);
> +		outb(mid, io + HP_TXM);
> +		outb(low, io + HP_TXL);
> +		return 0;
> +	}
> +
> +	snd_printd(LOGNAME ": Send host word timeout\n");

Ditto.

> +int snd_msnd_upload_host(struct snd_msnd *dev, const u8 *bin, int len)
> +{
> +	int i;
> +
> +	if (len % 3 != 0) {
> +		snd_printk(LOGNAME ": Upload host data not multiple of 3!\n");

Ditto.

> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < len; i += 3)
> +		if (snd_msnd_send_word(dev, bin[i], bin[i + 1], bin[i + 2]))
> +			return -EIO;
> +
> +	inb(dev->io + HP_RXL);
> +	inb(dev->io + HP_CVR);
> +
> +	return 0;
> +}
> +
> +int snd_msnd_enable_irq(struct snd_msnd *dev)
> +{
> +	unsigned long flags;
> +
> +	if (dev->irq_ref++)
> +		return 0;
> +
> +	snd_printd(LOGNAME ": Enabling IRQ\n");

I guess it's annoying to print at each time?
snd_printdd() would be a better choice if it's only for really verbose
debugging, as CONFIG_SND_DEBUG=y on most distros.

> +	spin_lock_irqsave(&dev->lock, flags);
> +	if (snd_msnd_wait_TXDE(dev) == 0) {
> +		outb(inb(dev->io + HP_ICR) | HPICR_TREQ, dev->io + HP_ICR);
> +		if (dev->type == msndClassic)
> +			outb(dev->irqid, dev->io + HP_IRQM);
> +
> +		outb(inb(dev->io + HP_ICR) & ~HPICR_TREQ, dev->io + HP_ICR);
> +		outb(inb(dev->io + HP_ICR) | HPICR_RREQ, dev->io + HP_ICR);
> +		enable_irq(dev->irq);
> +		snd_msnd_init_queue(dev->DSPQ, dev->dspq_data_buff,
> +				    dev->dspq_buff_size);
> +		spin_unlock_irqrestore(&dev->lock, flags);
> +		return 0;
> +	}
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +
> +	snd_printd(LOGNAME ": Enable IRQ failed\n");

This is an error message.  Should use a proper KERN_* prefix.

Ditto for snd_msnd_disable_irq().

> --- /dev/null
> +++ b/sound/isa/msnd/msnd.h
> @@ -0,0 +1,293 @@
(snip)
> +/* Generic FIFO * /
> +typedef struct {
> +	size_t n, len;
> +	char *data;
> +	int head, tail;
> +} msnd_fifo;  */
                 ^^
What?

> +struct snd_msnd {
> +	void			*mappedbase;

Use __iomem annotation.

> +	/* Motorola 56k DSP SMA */
> +	void		*SMA;
> +	void		*DAPQ;
> +	void		*DARQ;
> +	void		*MODQ;
> +	void		*MIDQ;
> +	void		*DSPQ;

I guess __iomem annotation is needed for these, too.

> --- /dev/null
> +++ b/sound/isa/msnd/msnd_classic.h
> @@ -0,0 +1,159 @@
(snip)
> +#ifdef HAVE_DSPCODEH

Do we ever have this config?  Otherwise let's rip off.

> +#  include "msndperm.c"
> +#  include "msndinit.c"
> +#  define PERMCODE		msndperm
> +#  define INITCODE		msndinit
> +#  define PERMCODESIZE		sizeof(msndperm)
> +#  define INITCODESIZE		sizeof(msndinit)
> +#else
> +#  ifndef CONFIG_MSNDCLAS_INIT_FILE
> +#    define CONFIG_MSNDCLAS_INIT_FILE "turtlebeach/msndinit.bin"
> +#  endif
> +#  ifndef CONFIG_MSNDCLAS_PERM_FILE
> +#    define CONFIG_MSNDCLAS_PERM_FILE "turtlebeach/msndperm.bin"
> +#  endif

These configs are in sound/oss/Kconfig.  You should add the same
config (but a different name) into ALSA side, too.

> --- /dev/null
> +++ b/sound/isa/msnd/msnd_midi.c
> --- /dev/null
> +++ b/sound/isa/msnd/msnd_pinnacle.c
> +#ifdef CONFIG_MSNDCLAS_HAVE_BOOT
> +#  define HAVE_DSPCODEH
> +#endif

Ah, ok here it is.  This config is also in sound/oss/Kconfig...

> +static int snd_msnd_DARQ(struct snd_msnd *chip, int bank)
> +{
(snip)
> +	/ * Read data from the head (unprotected bank 1 access okay
> +	   since this is only called inside an interrupt) * /
> +	outb(HPBLKSEL_1, chip->io + HP_BLKS);
> +	if ((n = msnd_fifo_write(
> +		&chip->DARF,
> +		(char *)(chip->base + bank * DAR_BUFF_SIZE),
> +		size, 0)) <= 0) {
> +		outb(HPBLKSEL_0, chip->io + HP_BLKS);
> +		return n;
> +	}
> +	outb(HPBLKSEL_0, chip->io + HP_BLKS);
> +	*/

Avoid nested comments...

> +irqreturn_t snd_msnd_interrupt(int irq, void *dev_id)

Missing static?

> +static struct snd_pcm_hardware snd_msnd_playback = {
> +	.info =			SNDRV_PCM_INFO_MMAP |
> +				SNDRV_PCM_INFO_INTERLEAVED |
> +				SNDRV_PCM_INFO_MMAP_VALID,
> +	.formats =		SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S16_LE,
> +	.rates =		SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_8000_48000,
> +	.rate_min =		8000,
> +	.rate_max =		48000,
(snip)
> +static unsigned int rates[] = {
> +	8000, 11025, 16000, 22050, 32000, 44100, 48000
> +};
> +
> +static struct snd_pcm_hw_constraint_list hw_constraints_rates = {
> +	.count = ARRAY_SIZE(rates),
> +	.list = rates,
> +	.mask = 0,
> +};

These are all standard sample rates, so you don't need the extra
hw_constraint code.  Just set SNDRV_PCM_RATE_XXX into rates flags
properly.

> +int snd_msnd_pcm(struct snd_card *card, int device, struct snd_pcm **rpcm)

Missing static?


thanks,

Takashi