Re: Patch for AICA sound support on SEGA Dreamcast

Re: Patch for AICA sound support on SEGA Dreamcast

[Takashi Iwai]

At Mon, 03 Apr 2006 15:45:22 +0100,
Adrian McMenamin wrote:
> 
> This provides ALSA sound for the Sega Dreamcast. Seems to work well for
> me, but there is a dearth of testers.
> 
> Signed off by Adrian McMenamin (adrian@mcmen.demon.co.uk)

Thanks for the patch.  Some comments are below.

> diff -ruN /home/adrian/alsa-old/alsa-driver/sh/aica.h /home/adrian/alsa-driver/sh/aica.h
> --- /home/adrian/alsa-old/alsa-driver/sh/aica.h	1970-01-01 01:00:00.000000000 +0100
> +++ /home/adrian/alsa-driver/sh/aica.h	2006-04-03 15:40:07.000000000 +0100
> @@ -0,0 +1,79 @@
(snip)
> +typedef struct {
> +	snd_card_t *card;
> +	snd_pcm_substream_t *substream;
> +	int clicks;
> +	int current_period;
> +	struct timer_list timer;
> +	int master_volume;
> +
> +} snd_card_aica_t;

Please avoid use of typedefs as much as possible.
We (finally :-) got rid of whole typedefs recently from the ALSA core
code.


> +typedef struct aica_channel {
> +	uint32_t cmd;		/* Command ID           */
> +	uint32_t pos;		/* Sample position      */
> +	uint32_t length;	/* Sample length        */
> +	uint32_t freq;		/* Frequency            */
> +	uint32_t vol;		/* Volume 0-255         */
> +	uint32_t pan;		/* Pan 0-255            */
> +	uint32_t sfmt;		/* Sound format         */
> +	uint32_t flags;		/* Bit flags            */
> +} aica_channel_t;

Ditto.


> diff -ruN /home/adrian/alsa-old/alsa-driver/sh/snd_aica.c /home/adrian/alsa-driver/sh/snd_aica.c
> --- /home/adrian/alsa-old/alsa-driver/sh/snd_aica.c	1970-01-01 01:00:00.000000000 +0100
> +++ /home/adrian/alsa-driver/sh/snd_aica.c	2006-04-03 15:40:20.000000000 +0100
> @@ -0,0 +1,802 @@
(snip)
> +
> +static int index = 0;

No need for explicit zero initialization.

> +static char *id = "0";

You should set it to NULL, i.e. don't set value here.

> +static int enable= 1;
> 
> +module_param(index, int, 0444);
> +MODULE_PARM_DESC(index, "Index value for " CARD_NAME " soundcard.");
> +module_param(id, charp, 0444);
> +MODULE_PARM_DESC(id, "ID string for " CARD_NAME " soundcard.");
> +module_param(enable, bool, 0644);
> +MODULE_PARM_DESC(enable, "Enable " CARD_NAME " soundcard.");
> +
> +
> +/* SPU specific functions */
> +
> +inline void spu_write_wait()

Lack of static.  And lack of (void) argument.


> +{
> +	while (1) {
> +		if (!(readl(G2_FIFO) & 0x11))
> +			break;
> +	}

Safer to have a timeout?

> +
> +	return;
> +}
> +
> +
> +
> +static void spu_memset(uint32_t toi, unsigned long what, int length)

The type of second argument should be "void __iomem *" for accessing
readl/writel.


> +{
> +	uint32_t *to = (uint32_t *) (SPU_MEMORY_BASE + toi);
> +	int i;
> +
> +	if (length % 4)
> +		length = (length / 4) + 1;
> +	else
> +		length = length / 4;

Shouldn't be always aligned to 4?  Something like

	snd_assert(length % 4 == 0, return);

> +	spu_write_wait();
> +	for (i = 0; i < length; i++) {
> +		writel(what, to);
> +		to++;
> +		if (i && !(i % 8))
> +			spu_write_wait();
> +	}
> +	return;
> +}
> +
> +static void spu_memload(uint32_t toi, uint8_t * from, int length)

Also "uint8_t __iomem *from" (maybe a "void __iomem *" would be
simpler since you can avoid an explicit cast below).

Also, these lowlevel functions look racy.  Many places call
spu_memload() without a proper lock.

> +{
> +	uint32_t *froml = (uint32_t *) from;
> +	uint32_t *to = (uint32_t *) (SPU_MEMORY_BASE + toi);

Should be "uint32_t __iomem *".

> +	int i, val;
> +	if (length % 4)
> +		length = (length / 4) + 1;
> +	else
> +		length = length / 4;

Similar as spu_memset().

> +	spu_write_wait();
> +	for (i = 0; i < length; i++) {
> +		val = *froml;
> +		writel(val, to);
> +		froml++;
> +		to++;
> +		if (i && !(i % 8))
> +			spu_write_wait();
> +	}
> +	return;
> +
> +}
> +
> +static void spu_disable()

Need (void) for empty argument.

> +{
> +	int i;
> +	uint32_t regval;
> +	spu_write_wait();
> +	regval = readl(ARM_RESET_REGISTER);

readl and writel take void __iomem *.

> +	regval |= 1;
> +	spu_write_wait();
> +	writel(regval, ARM_RESET_REGISTER);
> +	for (i = 0; i < 64; i++) {
> +		spu_write_wait();
> +		regval = readl(SPU_REGISTER_BASE + (i * 0x80));
> +		regval = (regval & ~0x4000) | 0x8000;
> +		spu_write_wait();
> +		writel(regval, SPU_REGISTER_BASE + (i * 0x80));
> +
> +	}
> +
> +}
> +
> +
> +static void spu_enable()

Lack of (void)....  in many others below, too.

> +{
> +	uint32_t regval = readl(ARM_RESET_REGISTER);
> +	regval &= ~1;
> +	spu_write_wait();
> +	writel(regval, ARM_RESET_REGISTER);
> +	return;
> +
> +}
> +
> +
> +/* Halt the sound processor,
> +   clear the memory,
> +   load some default ARM7 code,
> +   and then restart ARM7
> +*/
> +static int spu_init()
> +{
> +
> +	spu_disable();
> +	spu_memset(0, 0, 0x200000 / 4);
> +	*(uint32_t *) SPU_MEMORY_BASE = 0xea000002;

Is it a right code?

> +	spu_enable();
> +	schedule();
> +	return 0;
> +}
> +
> +
> +
> +static aica_channel_t *channel;

Better to assign this to runtime->private_data instead of a static
record.

> +inline static void aica_chn_start()
> +{
> +	spu_write_wait();
> +	writel(AICA_CMD_KICK | AICA_CMD_START,
> +	       (uint32_t *) AICA_CONTROL_POINT);
> +
> +}
> +
> +inline static void aica_chn_halt()
> +{
> +	spu_write_wait();
> +	writel(AICA_CMD_KICK | AICA_CMD_STOP,
> +	       (uint32_t *) AICA_CONTROL_POINT);
> +
> +}
> +
> +
> +/* ALSA code below */
> +
> +static snd_pcm_hardware_t snd_pcm_aica_playback_hw = {

Use struct snd_pcm_hardware, instead.
Refer either ALSA-1.0.11rc4 or 2.6.16 tree.


> +
> +	.info = (SNDRV_PCM_INFO_NONINTERLEAVED),
> +	.formats =
> +	    (SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_S16_LE |
> +	     SNDRV_PCM_FMTBIT_IMA_ADPCM),
> +	.rates = SNDRV_PCM_RATE_8000_48000,
> +	.rate_min = 8000,
> +	.rate_max = 48000,
> +	.channels_min = 1,
> +	.channels_max = 2,
> +	.buffer_bytes_max = AICA_BUFFER_SIZE,
> +	.period_bytes_min = AICA_PERIOD_SIZE,
> +	.period_bytes_max = AICA_PERIOD_SIZE,
> +	.periods_min = AICA_PERIOD_NUMBER,
> +	.periods_max = AICA_PERIOD_NUMBER,
> +};
> +
> +/* There is only one sound card on a Dreamcast */
> +static snd_card_aica_t *dreamcastcard = NULL;

Is this static variable really needed?


> +static struct platform_device *pd = NULL;
> +
> +
> +
> +
> +static int stereo_buffer_transfer(snd_pcm_substream_t * substream,

Use struct snd_pcm_substream.

> +				  int buffer_size, int period)
> +{
> +	int transferred;
> +	snd_pcm_runtime_t *runtime;

Use struct snd_pcm_runtime....  in many other places, too.

> +	int period_offset;
> +	period_offset = period;
> +	period_offset %= (AICA_PERIOD_NUMBER / 2);
> +	runtime = substream->runtime;
> +
> +	//snd_printk("period is 0x%X, period_offset is 0x%X, buffer_size is 0x%X\n", period, period_offset, buffer_size);
> +	/* transfer left and then right */
> +	dma_xfer(0,
> +		 runtime->dma_area + (AICA_PERIOD_SIZE * period_offset),
> +		 AICA_CHANNEL0_OFFSET + (AICA_PERIOD_SIZE * period_offset),
> +		 buffer_size / 2, 5);
> +	/* wait for completion */
> +	do {
> +		udelay(5);
> +		transferred = get_dma_residue(0);
> +	}
> +	while (transferred < buffer_size / 2);

Hmm, is this a correct implementation?  It doesf looping in a timer
handler.  Surely it works, but...


> +	dma_xfer(0,
> +		 AICA_BUFFER_SIZE / 2 + runtime->dma_area +
> +		 (AICA_PERIOD_SIZE * period_offset),
> +		 AICA_CHANNEL1_OFFSET + (AICA_PERIOD_SIZE * period_offset),
> +		 buffer_size / 2, 5);
> +	/* have to wait again */
> +	do {
> +		udelay(5);
> +		transferred = get_dma_residue(0);
> +	}
> +	while (transferred < buffer_size / 2);

Ditto.

> +	return 0;
> +}
> +
> +
> +
> +
> +void aica_period_elapsed(unsigned long timer_var)
> +{
> +	int transferred;
> +	int play_period;
> +	snd_pcm_runtime_t *runtime;
> +	runtime = (dreamcastcard->substream)->runtime;

You can use the argument (timer_var) to retrieve an object.

> +
> +	/* Have we played out an additional period? */
> +	play_period =
> +	    frames_to_bytes(runtime,
> +			    readl
> +			    (AICA_CONTROL_CHANNEL_SAMPLE_NUMBER)) /
> +	    AICA_PERIOD_SIZE;
> +	if (play_period == dreamcastcard->current_period) {
> +		dreamcastcard->timer.expires = jiffies + 1;
> +		add_timer(&(dreamcastcard->timer));

I guess you need a proper lock to avoid race here?

> +		return;
> +	}
> +	if (runtime->channels > 1) {
> +		dreamcastcard->current_period = play_period;	/*TO DO: Work out why this doesn't work for 1 channel */
> +
> +		stereo_buffer_transfer(dreamcastcard->substream,
> +				       AICA_PERIOD_SIZE * 2,
> +				       dreamcastcard->clicks);
> +	} else {
> +		dma_xfer(0,
> +			 runtime->dma_area +
> +			 (AICA_PERIOD_SIZE * dreamcastcard->clicks),
> +			 AICA_CHANNEL0_OFFSET +
> +			 (AICA_PERIOD_SIZE * dreamcastcard->clicks),
> +			 AICA_PERIOD_SIZE, 5);
> +		do {
> +			/* Try to fine tune the delay to keep it as short as possible */
> +			udelay(5);
> +			/* get_dma_residue reports residue until completion when it reports total bytes transferred */
> +			transferred = get_dma_residue(0);
> +		}
> +		while (transferred < AICA_PERIOD_SIZE);

Another busy loop in timer callback.

> +	}
> +
> +	snd_pcm_period_elapsed(dreamcastcard->substream);
> +	dreamcastcard->clicks++;
> +	dreamcastcard->clicks %= AICA_PERIOD_NUMBER;
> +	/* reschedule the timer */
> +	dreamcastcard->timer.expires = jiffies + 1;
> +	add_timer(&(dreamcastcard->timer));
> +	return;
> +}
> +
> +static int snd_aicapcm_pcm_open(snd_pcm_substream_t * substream)
> +{
> +	if (!enable) return -ENOENT;

This check is useless.  If needed, do it in module initialization.

> +	channel = kmalloc(sizeof(aica_channel_t), GFP_KERNEL);
> +	if (!channel)
> +		return -ENOMEM;
> +	/* set defaults for channel */
> +	channel->sfmt = SM_8BIT;
> +	channel->cmd = AICA_CMD_START;
> +	channel->vol = dreamcastcard->master_volume;
> +	channel->pan = 0x80;
> +	channel->pos = 0;
> +	channel->flags = 0;	/* default to mono */
> +	snd_pcm_runtime_t *runtime;

Should be in the beginning of the function.

> +	runtime = substream->runtime;
> +	runtime->hw = snd_pcm_aica_playback_hw;
> +	spu_enable();
> +	dreamcastcard->clicks = 0;
> +	dreamcastcard->current_period = 0;
> +	return 0;
> +
> +}
> +
> +static int snd_aicapcm_pcm_close(snd_pcm_substream_t * substream)
> +{
> +	del_timer(&dreamcastcard->timer);
> +	kfree(channel);
> +	spu_disable();
> +	return 0;
> +}
> +
> +static int snd_aicapcm_pcm_hw_free(snd_pcm_substream_t * substream)
> +{
> +	/* Free the DMA buffer */
> +	return snd_pcm_lib_free_pages(substream);
> +}
> +
> +static int snd_aicapcm_pcm_hw_params(snd_pcm_substream_t * substream,
> +				     snd_pcm_hw_params_t * hw_params)
> +{
> +	/* Allocate a DMA buffer using ALSA built-ins */
> +	return
> +	    snd_pcm_lib_malloc_pages(substream,
> +				     params_buffer_bytes(hw_params));
> +}
> +
> +static int snd_aicapcm_pcm_prepare(snd_pcm_substream_t * substream)
> +{
> +
> +	/* Set up the playback */
> +	/* basic settings to test working */
> +	if ((substream->runtime)->format == SNDRV_PCM_FORMAT_S16_LE)
> +		channel->sfmt = SM_16BIT;
> +	channel->freq = substream->runtime->rate;
> +	dreamcastcard->substream = substream;

This one is never unset...

> +	return 0;
> +}
> +
> +
> +static void startup_aica()
> +{
> +	spu_memload(AICA_CHANNEL0_CONTROL_OFFSET, (uint8_t *) channel,
> +		    sizeof(aica_channel_t));
> +	aica_chn_start();
> +	return;
> +}
> +
> +
> +static void spu_begin_dma(snd_pcm_substream_t * substream)
> +{
> +	int buffer_size;
> +	snd_pcm_runtime_t *runtime;
> +	runtime = substream->runtime;
> +	buffer_size = frames_to_bytes(runtime, runtime->buffer_size);
> +	if (runtime->channels == 1)
> +		dma_xfer(0, runtime->dma_area, AICA_CHANNEL0_OFFSET,
> +			 buffer_size, 5);
> +	else {
> +		int transfer_status;
> +		channel->flags |= 0x01;
> +		transfer_status =
> +		    stereo_buffer_transfer(substream, buffer_size, 0);
> +		if (transfer_status != 0)
> +			return;
> +		dreamcastcard->clicks =
> +		    buffer_size / (AICA_PERIOD_SIZE * runtime->channels);
> +	}
> +	startup_aica();
> +	init_timer(&(dreamcastcard->timer));
> +	dreamcastcard->timer.function = aica_period_elapsed;

This is dangerous.  The timer can be running, e.g. when a PCM stream
is replayed.

> +
> +
> +	dreamcastcard->timer.expires = jiffies + 4;

Why 4?

> +	add_timer(&(dreamcastcard->timer));
> +	return;
> +}
> +
> +static int snd_aicapcm_pcm_trigger(snd_pcm_substream_t * substream,
> +				   int cmd)
> +{
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +		spu_begin_dma(substream);
> +		break;
> +	case SNDRV_PCM_TRIGGER_STOP:
> +		aica_chn_halt();

Isn't better to stop the timer here?

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +
> +	return 0;
> +}
> +
> +static snd_pcm_uframes_t snd_aicapcm_pcm_pointer(snd_pcm_substream_t *
> +						 substream)
> +{
> +	return readl(AICA_CONTROL_CHANNEL_SAMPLE_NUMBER);
> +
> +}
> +
> +
> +static snd_pcm_ops_t snd_aicapcm_playback_ops = {
> +	.open = snd_aicapcm_pcm_open,
> +	.close = snd_aicapcm_pcm_close,
> +	.ioctl = snd_pcm_lib_ioctl,
> +	.hw_params = snd_aicapcm_pcm_hw_params,
> +	.hw_free = snd_aicapcm_pcm_hw_free,
> +	.prepare = snd_aicapcm_pcm_prepare,
> +	.trigger = snd_aicapcm_pcm_trigger,
> +	.pointer = snd_aicapcm_pcm_pointer,
> +};
> +
> +
> +static int snd_aicapcm_free(snd_card_aica_t * dreamcastcard)
> +{
> +	release_mem_region(ARM_RESET_REGISTER, 0x4);
> +	release_mem_region(SPU_MEMORY_BASE, 0x200000);
> +	return 0;
> +}
> +
> +/*	Set up the PCM playback device
> +	card is pointer to the overall AICA device
> +	pcm_index is number of PCM instances - 1
> +
> +	TO DO: set up to handle more than one pcm instance
> +*/
> +
> +
> +static int __init snd_aicapcmchip(snd_card_aica_t * dreamcastcard,
> +				  int pcm_index)
> +{
> +	snd_pcm_t *pcm;
> +	int err;
> +
> +	/* Can we lock the memory */
> +
> +	if (request_mem_region(ARM_RESET_REGISTER, 4, "AICA ARM control")
> +	    == NULL)
> +		return -ENOMEM;
> +	if (request_mem_region(SPU_MEMORY_BASE, 0x200000, "AICA Sound RAM")
> +	    == NULL) {
> +		release_mem_region(ARM_RESET_REGISTER, 0x4);
> +		return -ENOMEM;
> +	}
> +
> +	/* AICA has no capture ability */
> +	if ((err =
> +	     snd_pcm_new(dreamcastcard->card, "AICA PCM", pcm_index, 1, 0,
> +			 &pcm)) < 0)
> +		return err;
> +	pcm->private_data = dreamcastcard;
> +	dreamcastcard->card->private_data = pcm;

What the purpose of this assignment?

> +	strcpy(pcm->name, "AICA PCM");
> +
> +	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK,
> +			&snd_aicapcm_playback_ops);
> +
> +	/* Allocate the DMA buffers */
> +	err =
> +	    snd_pcm_lib_preallocate_pages_for_all(pcm,
> +						  SNDRV_DMA_TYPE_CONTINUOUS,
> +						  snd_dma_continuous_data
> +						  (GFP_KERNEL),
> +						  AICA_BUFFER_SIZE,
> +						  AICA_BUFFER_SIZE);
> +
> +	return err;
> +}
> +
> +/* Mixer controls */
> +static int aica_pcmswitch_info(snd_kcontrol_t * kcontrol,
> +			       snd_ctl_elem_info_t * uinfo)
> +{
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
> +	uinfo->count = 1;
> +	uinfo->value.integer.min = 0;
> +	uinfo->value.integer.max = 1;
> +	return 0;
> +}
> +
> +static int aica_pcmswitch_get(snd_kcontrol_t * kcontrol,
> +			      snd_ctl_elem_value_t * ucontrol)
> +{
> +	ucontrol->value.integer.value[0] = 1;	/* TO DO: Fix me */
> +	return 0;
> +}
> +
> +static int aica_pcmswitch_put(snd_kcontrol_t * kcontrol,
> +			      snd_ctl_elem_value_t * ucontrol)
> +{
> +	if (ucontrol->value.integer.value[0] == 1)
> +		return 0;	/* TO DO: Fix me */
> +	else
> +		aica_chn_halt();
> +	return 0;
> +}

I guess this doesn't work (to reenable the DMA)?


> +static int aica_pcmvolume_info(snd_kcontrol_t * kcontrol,
> +			       snd_ctl_elem_info_t * uinfo)
> +{
> +
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
> +	uinfo->count = 1;
> +	uinfo->value.integer.min = 0;
> +	uinfo->value.integer.max = 0xFF;
> +
> +	return 0;
> +}
> +
> +static int aica_pcmvolume_get(snd_kcontrol_t * kcontrol,
> +			      snd_ctl_elem_value_t * ucontrol)
> +{
> +	if (!channel)
> +		return -ETXTBSY;	/* we've not yet been set up */
> +	ucontrol->value.integer.value[0] = channel->vol;
> +	return 0;
> +}
> +
> +static int aica_pcmvolume_put(snd_kcontrol_t * kcontrol,
> +			      snd_ctl_elem_value_t * ucontrol)
> +{
> +	if (!channel) {
> +		snd_printk("No channel yet\n");
> +		return -ETXTBSY;	/* too soon */
> +	}
> +
> +	else if (channel->vol == ucontrol->value.integer.value[0])
> +		return 0;
> +
> +	else {
> +		channel->vol = ucontrol->value.integer.value[0];
> +		dreamcastcard->master_volume = ucontrol->value.integer.value[0];
> +		spu_memload(AICA_CHANNEL0_CONTROL_OFFSET,
> +			    (uint8_t *) channel, sizeof(aica_channel_t));
> +	}
> +	return 1;
> +}
> +
> +static snd_kcontrol_new_t snd_aica_masterswitch_control __devinitdata = {
> +	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> +	.name = "Playback Switch",

Should be "Master Playback Switch".

> +	.info = aica_pcmswitch_info,
> +	.get = aica_pcmswitch_get,
> +	.put = aica_pcmswitch_put
> +};

... But why do you need two different switches for the very same
thing? 

> +static snd_kcontrol_new_t snd_aica_pcmswitch_control __devinitdata = {
> +	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> +	.name = "PCM Playback Switch",
> +	.index = 0,
> +	.info = aica_pcmswitch_info,
> +	.get = aica_pcmswitch_get,
> +	.put = aica_pcmswitch_put
> +};
> +
> +
> +static snd_kcontrol_new_t snd_aica_mastervolume_control __devinitdata = {
> +	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> +	.name = "Playback Volume",

Should be "Master Playback Volume".

> +	.info = aica_pcmvolume_info,
> +	.get = aica_pcmvolume_get,
> +	.put = aica_pcmvolume_put
> +};
> +
> +static snd_kcontrol_new_t snd_aica_pcmvolume_control __devinitdata = {
> +	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> +	.name = "PCM Playback Volume",
> +	.index = 0,
> +	.info = aica_pcmvolume_info,
> +	.get = aica_pcmvolume_get,
> +	.put = aica_pcmvolume_put
> +};

... And again two mixer elements for the very same volume control.


> +
> +
> +static struct device_driver aica_driver = {
> +	.name = "AICA",
> +	.bus = &platform_bus_type,
> +};
> +
> +
> +/* Fill up the members of the embedded device structure */
> +
> +static void populate_dreamcastaicadev(struct device *dev)
> +{
> +	dev->bus = &platform_bus_type;
> +	dev->driver = &aica_driver;
> +	driver_register(dev->driver);
> +	device_bind_driver(dev);
> +}

Looks not right for the recent platform_device...
You can do it better in platform_driver.probe and remove callbacks.


> +static int load_aica_firmware()
> +{
> +	int err;
> +	err = 0;
> +	spu_init();
> +	const struct firmware *fw_entry;
> +	err = request_firmware(&fw_entry, "aica_firmware.bin", &pd->dev);
> +	if (err)
> +		return err;
> +	/* write firware into memory */
> +	spu_disable();
> +	spu_memload(0, fw_entry->data, fw_entry->size);
> +	spu_enable();
> +	release_firmware(fw_entry);
> +	return err;
> +}
> +
> +static int __devinit add_aicamixer_controls()
> +{
> +	int err;
> +	err = snd_ctl_add
> +	     (dreamcastcard->card,
> +	      snd_ctl_new1(&snd_aica_pcmvolume_control,
> +			   dreamcastcard));
> +	     if (err < 0){
> +		snd_printk("pcmvolume failed\n");
> +		return err;
> +	     }
> +	    
> +	    err= snd_ctl_add
> +	     (dreamcastcard->card,
> +	      snd_ctl_new1(&snd_aica_pcmswitch_control,
> +			   dreamcastcard));
> +	     if (err < 0){
> +		snd_printk("pcmswitch failed\n");
> +		return err;
> +	     }
> +
> +	    
> +	    err = snd_ctl_add
> +	     (dreamcastcard->card,
> +	      snd_ctl_new1(&snd_aica_mastervolume_control,
> +			   dreamcastcard));
> +	     if (err < 0){
> +		snd_printk("mastervolume failed\n");
> +		return err;
> +	     }
> +
> +	    
> +	    err = snd_ctl_add
> +	     (dreamcastcard->card,
> +	      snd_ctl_new1(&snd_aica_masterswitch_control,
> +			   dreamcastcard));
> +	     if (err < 0){
> +		snd_printk("masterswitch failed\n");
> +		return err;
> +	     }
> +
> +	    
> +	  
> +	/* succeeded */
> +	return 0;
> +}
> +
> +
> +
> +static int __init aica_init(void)
> +{
> +
> +
> +	int err;
> +	int idx;
> +
> +	/* Are we in a Dreamcast at all? */
> +	if (!mach_is_dreamcast())
> +		return -ENODEV;
> +	dreamcastcard = kmalloc(sizeof(snd_card_aica_t), GFP_KERNEL);
> +	if (!dreamcastcard)
> +		return -ENOMEM;
> +	dreamcastcard->card = NULL;
> +	/* Can only be one real device on a Dreamcast
> +	   but we might have the dummy or some other driver loaded */
> +	for (idx = 0; idx < SNDRV_CARDS; idx++) {
> +		dreamcastcard->card =
> +		    snd_card_new(idx, "AICA", THIS_MODULE, 0);
> +		if (dreamcastcard->card)
> +			break;
> +	}

The code looks odd.  Simply call snd_card_new() once because this driver
supports only one instance anyway.

> +	if (!dreamcastcard->card) {
> +		kfree(dreamcastcard);
> +		return -ENODEV;
> +	}
> +	strcpy(dreamcastcard->card->driver, "snd_card_aica");
> +	strcpy(dreamcastcard->card->shortname, "AICA");
> +	strcpy(dreamcastcard->card->longname,
> +	       "Yamaha AICA Super Intelligent Sound Processor for SEGA Dreamcast");
> +
> +	/* Load the PCM 'chip' */
> +	if ((err = snd_aicapcmchip(dreamcastcard, 0)) < 0)
> +		goto freedreamcast;
> +
> +
> +	pd = platform_device_register_simple(dreamcastcard->card->driver,
> +					     -1, NULL, 0);
> +
> +	if (IS_ERR(pd)) {
> +		err = -ENODEV;
> +		goto freepcm;
> +	}
> +
> +	populate_dreamcastaicadev(&pd->dev);
> +	snd_card_set_dev(dreamcastcard->card, &pd->dev);
> +	/* Register the card with ALSA subsystem */
> +	if ((err = snd_card_register(dreamcastcard->card)) < 0)
> +		goto freepcm;
> +
> +	/* Load the firmware */
> +	err = load_aica_firmware();

The firmware should be loaded before registration of the card
instance.  snd_card_register() will create device files.  It means
that user-space apps are allowed to access the driver.

> +
> +	if (err)
> +		goto freepcm;
> +
> +
> +	/* Add basic controls */
> +	if (add_aicamixer_controls() < 0) goto freepcm;

This should be done before snd_card_register(), too.


> +	snd_printk
> +	    ("ALSA Driver for Yamaha AICA Super Intelligent Sound Processor on slot %d\n",
> +	     idx);
> +
> +	return 0;
> +
> +      freepcm:
> +	snd_aicapcm_free(dreamcastcard);
> +
> +      freedreamcast:
> +	snd_card_free(dreamcastcard->card);
> +
> +	if (pd) {
> +		struct device_driver *drv = (&pd->dev)->driver;
> +		device_release_driver(&pd->dev);
> +		driver_unregister(drv);
> +		platform_device_unregister(pd);
> +		pd = NULL;
> +	}
> +	kfree(dreamcastcard);
> +	return err;
> +
> +
> +}
> +
> +static void __exit aica_exit(void)
> +{
> +
> +	if (likely(dreamcastcard->card)) {

You don't need such an optimization in this place at all.

> +		snd_aicapcm_free(dreamcastcard);
> +		snd_card_free(dreamcastcard->card);
> +		kfree(dreamcastcard);
> +		if (likely(pd)) {
> +			struct device_driver *drv = (&pd->dev)->driver;
> +			device_release_driver(&pd->dev);
> +			driver_unregister(drv);
> +			platform_device_unregister(pd);
> +		}

This should be in remove callback.


Takashi

Re: Patch for AICA sound support on SEGA Dreamcast

[Adrian McMenamin]

On Mon, 2006-04-03 at 17:52 +0200, Takashi Iwai wrote:
> At Mon, 03 Apr 2006 15:45:22 +0100,
> Adrian McMenamin wrote:
> > 
> > This provides ALSA sound for the Sega Dreamcast. Seems to work well for
> > me, but there is a dearth of testers.
> > 
> > Signed off by Adrian McMenamin (adrian@mcmen.demon.co.uk)
> 
> Thanks for the patch.  Some comments are below.
> 

And my comments added back

> > diff -ruN /home/adrian/alsa-old/alsa-driver/sh/aica.h /home/adrian/alsa-driver/sh/aica.h
> > --- /home/adrian/alsa-old/alsa-driver/sh/aica.h	1970-01-01 01:00:00.000000000 +0100
> > +++ /home/adrian/alsa-driver/sh/aica.h	2006-04-03 15:40:07.000000000 +0100
> > @@ -0,0 +1,79 @@
> (snip)
> > +typedef struct {

> > +} snd_card_aica_t;
> 
> Please avoid use of typedefs as much as possible.
> We (finally :-) got rid of whole typedefs recently from the ALSA core
> code.
> 

OK


> (snip)
> > +
> > +static int index = 0;
> 
> No need for explicit zero initialization.
> 
> > +static char *id = "0";
> 
> You should set it to NULL, i.e. don't set value here.

OK

> > +/* SPU specific functions */
> > +
> > +inline void spu_write_wait()
> 
> Lack of static.  And lack of (void) argument.
> 


lack of static yes - but void inside the brackets - really?

> 
> > +{
> > +	while (1) {
> > +		if (!(readl(G2_FIFO) & 0x11))
> > +			break;
> > +	}
> 
> Safer to have a timeout?
> 

No - this is correct


> > +static void spu_memset(uint32_t toi, unsigned long what, int length)
> 
> The type of second argument should be "void __iomem *" for accessing
> readl/writel.
> 

OK

> 
> > +{
> > +	uint32_t *to = (uint32_t *) (SPU_MEMORY_BASE + toi);
> > +	int i;
> > +
> > +	if (length % 4)
> > +		length = (length / 4) + 1;
> > +	else
> > +		length = length / 4;
> 
> Shouldn't be always aligned to 4?  Something like
> 
> 	snd_assert(length % 4 == 0, return);
> 

but that would break and the code fixes it and works




> > +static void spu_memload(uint32_t toi, uint8_t * from, int length)
> 
> Also "uint8_t __iomem *from" (maybe a "void __iomem *" would be
> simpler since you can avoid an explicit cast below).
> 
> Also, these lowlevel functions look racy.  Many places call
> spu_memload() without a proper lock.
> 


OK



> > +	spu_disable();
> > +	spu_memset(0, 0, 0x200000 / 4);
> > +	*(uint32_t *) SPU_MEMORY_BASE = 0xea000002;
> 
> Is it a right code?
> 

I believe so, just a piece of ARM7 machine code that is an infinite loop


> > +	spu_enable();
> > +	schedule();
> > +	return 0;
> > +}
> > +
> > +
> > +
> > +static aica_channel_t *channel;
> 
> Better to assign this to runtime->private_data instead of a static
> record.
> 

Probably, though overkill for the current setup


> > +/* There is only one sound card on a Dreamcast */
> > +static snd_card_aica_t *dreamcastcard = NULL;
> 
> Is this static variable really needed?
> 

As there is only one instance it makes life a lot simpler, but it could
be got rid of




> > +		udelay(5);
> > +		transferred = get_dma_residue(0);
> > +	}
> > +	while (transferred < buffer_size / 2);
> 
> Hmm, is this a correct implementation?  It doesf looping in a timer
> handler.  Surely it works, but...
> 

What's the alternative? I need to wait for one dma transfer to end
before the next begins



>  = jiffies + 1;
> > +		add_timer(&(dreamcastcard->timer));
> 
> I guess you need a proper lock to avoid race here?
> 

Yes, it does need locks



> > +			/* get_dma_residue reports residue until completion when it reports total bytes transferred */
> > +			transferred = get_dma_residue(0);
> > +		}
> > +		while (transferred < AICA_PERIOD_SIZE);
> 
> Another busy loop in timer callback.
> 

As before - I cannot have a second dma transfer begin until the first
has ended, the transfer is fast (about 12 Mb/s aiui so the busy looping
should not last *too* long). If you can suggest an alternative?



> > +
> > +static int snd_aicapcm_pcm_open(snd_pcm_substream_t * substream)
> > +{
> > +	if (!enable) return -ENOENT;
> 
> This check is useless.  If needed, do it in module initialization.
> 

Why is it useless? If I do it in initialisation what is the point of the
parameter? ie why load a module with a perameter that means don't load?
If I do it this way then the user can turn enable on and off by writing
to sysfs. Isn't that what it is supposed to be there for?



> > +	channel->flags = 0;	/* default to mono */
> > +	snd_pcm_runtime_t *runtime;
> 
> Should be in the beginning of the function.
> 

yep


> > +	dreamcastcard->substream = substream;
> 
> This one is never unset...
> 
yes, but there's only one instance so it's handy that way :)


> > +	return 0;

> > +	startup_aica();
> > +	init_timer(&(dreamcastcard->timer));
> > +	dreamcastcard->timer.function = aica_period_elapsed;
> 
> This is dangerous.  The timer can be running, e.g. when a PCM stream
> is replayed.
> 

I don't understand the point you are making. Do you mean the timer could
be runing when this is (re)assigned? It's always the same function
though.


> > +
> > +
> > +	dreamcastcard->timer.expires = jiffies + 4;
> 
> Why 4?

four times as big a buffer as a one period read, no real reason though

> stream);
> > +		break;
> > +	case SNDRV_PCM_TRIGGER_STOP:
> > +		aica_chn_halt();
> 
> Isn't better to stop the timer here?

Do both probably!

> 
> > +		
> > +	pcm->private_data = dreamcastcard;
> > +	dreamcastcard->card->private_data = pcm;
> 
> What the purpose of this assignment?
> 

Good question! I suspect I wrote that last September and have forgotten
about it ever since :)



> > +static int aica_pcmswitch_put(snd_kcontrol_t * kcontrol,
> > +			      snd_ctl_elem_value_t * ucontrol)
> > +{
> > +	if (ucontrol->value.integer.value[0] == 1)
> > +		return 0;	/* TO DO: Fix me */
> > +	else
> > +		aica_chn_halt();
> > +	return 0;
> > +}
> 
> I guess this doesn't work (to reenable the DMA)?
> 

needs more thought

> 

> > +}
> > +
> > +static snd_kcontrol_new_t snd_aica_masterswitch_control __devinitdata = {
> > +	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> > +	.name = "Playback Switch",
> 
> Should be "Master Playback Switch".
> 

Not what the documentation says:


"Capture Source", "Capture Switch" and "Capture Volume" are used for the
global capture (input) source, switch and volume. Similarly, "Playback
Switch" and "Playback Volume" are used for the global output gain switch
and volume. 





> > +	.info = aica_pcmswitch_info,
> > +	.get = aica_pcmswitch_get,
> > +	.put = aica_pcmswitch_put
> > +};
> 
> ... But why do you need two different switches for the very same
> thing? 


Relic of trying to get this to work. Might matter if I seriously
developed this driver to use the full capacity of the device (though
that requires significant additional reversing). Would it help with
badly configured OSS apps?


> 
> Should be "Master Playback Volume".
> 

See above. Incidentally, when it was Master Playback Volume it didn't
work with OSS :(


> > +
> > +
> > +/* Fill up the members of the embedded device structure */
> > +
> > +static void populate_dreamcastaicadev(struct device *dev)
> > +{
> > +	dev->bus = &platform_bus_type;
> > +	dev->driver = &aica_driver;
> > +	driver_register(dev->driver);
> > +	device_bind_driver(dev);
> > +}
> 
> Looks not right for the recent platform_device...
> You can do it better in platform_driver.probe and remove callbacks.
> 
> 
I'll have to have a look at that - this code is 5/6 months old and I
haven't followed recent changes to the platform device spec.


> > 
> > +	}
> 
> The code looks odd.  Simply call snd_card_new() once because this driver
> supports only one instance anyway.


Yes, but when I did that and then loaded the dummy device the driver
broke! As the dummy device can be loaded and as there are capyure
devices for the Dreamcast (never managed to get my driver to work
though) then I think it is needed
> 

> > +	/* Register the card with ALSA subsystem */
> > +	if ((err = snd_card_register(dreamcastcard->card)) < 0)
> > +		goto freepcm;
> > +
> > +	/* Load the firmware */
> > +	err = load_aica_firmware();
> 
> The firmware should be loaded before registration of the card
> instance.  snd_card_register() will create device files.  It means
> that user-space apps are allowed to access the driver.
> 
OK


> > +
> > +	if (err)
> > +		goto freepcm;
> > +
> > +
> > +	/* Add basic controls */
> > +	if (add_aicamixer_controls() < 0) goto freepcm;
> 
> This should be done before snd_card_register(), too.
> 

OK

> 
> > +	snd_printk
> > +	    ("ALSA Driver for Yamaha AICA Super Intelligent Sound Processor on slot %d\n",
> > +	     idx);
> > +
> > +	return 0;
> > +
> > +      freepcm:
> > +	snd_aicapcm_free(dreamcastcard);
> > +
> > +      freedreamcast:
> > +	snd_card_free(dreamcastcard->card);
> > +
> > +	if (pd) {
> > +		struct device_driver *drv = (&pd->dev)->driver;
> > +		device_release_driver(&pd->dev);
> > +		driver_unregister(drv);
> > +		platform_device_unregister(pd);
> > +		pd = NULL;
> > +	}
> > +	kfree(dreamcastcard);
> > +	return err;
> > +
> > +
> > +}
> > +
> > +static void __exit aica_exit(void)
> > +{
> > +
> > +	if (likely(dreamcastcard->card)) {
> 
> You don't need such an optimization in this place at all.
> 

true, more code from way back when

> > +		snd_aicapcm_free(dreamcastcard);
> > +		snd_card_free(dreamcastcard->card);
> > +		kfree(dreamcastcard);
> > +		if (likely(pd)) {
> > +			struct device_driver *drv = (&pd->dev)->driver;
> > +			device_release_driver(&pd->dev);
> > +			driver_unregister(drv);
> > +			platform_device_unregister(pd);
> > +		}
> 
> This should be in remove callback.
> 
> 
> Takashi

Re: Patch for AICA sound support on SEGA Dreamcast

[Takashi Iwai]

At Mon, 03 Apr 2006 18:48:30 +0100,
Adrian McMenamin wrote:
> 
> > > +/* SPU specific functions */
> > > +
> > > +inline void spu_write_wait()
> > 
> > Lack of static.  And lack of (void) argument.
> > 
> 
> 
> lack of static yes - but void inside the brackets - really?

The driver codes are not in K&R style...

> > 
> > > +{
> > > +	while (1) {
> > > +		if (!(readl(G2_FIFO) & 0x11))
> > > +			break;
> > > +	}
> > 
> > Safer to have a timeout?
> > 
> 
> No - this is correct

But what happens if the hardware has a problem?


> > > +{
> > > +	uint32_t *to = (uint32_t *) (SPU_MEMORY_BASE + toi);
> > > +	int i;
> > > +
> > > +	if (length % 4)
> > > +		length = (length / 4) + 1;
> > > +	else
> > > +		length = length / 4;
> > 
> > Shouldn't be always aligned to 4?  Something like
> > 
> > 	snd_assert(length % 4 == 0, return);
> > 
> 
> but that would break and the code fixes it and works

Accessing unaligned char pointers and assuming the unaligned size is
broken.


> > > +		udelay(5);
> > > +		transferred = get_dma_residue(0);
> > > +	}
> > > +	while (transferred < buffer_size / 2);
> > 
> > Hmm, is this a correct implementation?  It doesf looping in a timer
> > handler.  Surely it works, but...
> > 
> 
> What's the alternative? I need to wait for one dma transfer to end
> before the next begins
>
> > > +			/* get_dma_residue reports residue until completion when it reports total bytes transferred */
> > > +			transferred = get_dma_residue(0);
> > > +		}
> > > +		while (transferred < AICA_PERIOD_SIZE);
> > 
> > Another busy loop in timer callback.
> > 
> 
> As before - I cannot have a second dma transfer begin until the first
> has ended, the transfer is fast (about 12 Mb/s aiui so the busy looping
> should not last *too* long). If you can suggest an alternative?

Hmm, but it can be still over 1ms?

Anyway, you should use dma_wait_for_completion() instead of your
home-made one.


> > > +
> > > +static int snd_aicapcm_pcm_open(snd_pcm_substream_t * substream)
> > > +{
> > > +	if (!enable) return -ENOENT;
> > 
> > This check is useless.  If needed, do it in module initialization.
> > 
> 
> Why is it useless? If I do it in initialisation what is the point of the
> parameter? ie why load a module with a perameter that means don't load?
> If I do it this way then the user can turn enable on and off by writing
> to sysfs. Isn't that what it is supposed to be there for?

The enable option itself is useless for drivers supporting single
devices.  It was introduced to choose devices to use if multiple
devices are available.


> > > +	return 0;
> 
> > > +	startup_aica();
> > > +	init_timer(&(dreamcastcard->timer));
> > > +	dreamcastcard->timer.function = aica_period_elapsed;
> > 
> > This is dangerous.  The timer can be running, e.g. when a PCM stream
> > is replayed.
> > 
> 
> I don't understand the point you are making. Do you mean the timer could
> be runing when this is (re)assigned? It's always the same function
> though.

Yes.  Because you don't delete timer, it's possible to call
trigger(start) even before the next timer interrupt occurs.


> > > +}
> > > +
> > > +static snd_kcontrol_new_t snd_aica_masterswitch_control __devinitdata = {
> > > +	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> > > +	.name = "Playback Switch",
> > 
> > Should be "Master Playback Switch".
> > 
> 
> Not what the documentation says:
> 
> 
> "Capture Source", "Capture Switch" and "Capture Volume" are used for the
> global capture (input) source, switch and volume. Similarly, "Playback
> Switch" and "Playback Volume" are used for the global output gain switch
> and volume. 

Then rename the variable name to follow that!
In practice, "Master Playback *" is used more commonly in other drivers.

> > > +	.info = aica_pcmswitch_info,
> > > +	.get = aica_pcmswitch_get,
> > > +	.put = aica_pcmswitch_put
> > > +};
> > 
> > ... But why do you need two different switches for the very same
> > thing? 
> 
> 
> Relic of trying to get this to work. Might matter if I seriously
> developed this driver to use the full capacity of the device (though
> that requires significant additional reversing). Would it help with
> badly configured OSS apps?

Having two control items for the same thing is just confusing and a
broken design.

> > Should be "Master Playback Volume".
> > 
> 
> See above. Incidentally, when it was Master Playback Volume it didn't
> work with OSS :(

Then likely something wrong with your code.  "Master Playback XXX"
works fine with other drivers.


> > > 
> > > +	}
> > 
> > The code looks odd.  Simply call snd_card_new() once because this driver
> > supports only one instance anyway.
> 
> 
> Yes, but when I did that and then loaded the dummy device the driver
> broke! As the dummy device can be loaded and as there are capyure
> devices for the Dreamcast (never managed to get my driver to work
> though) then I think it is needed

Maybe you didn't pass a proper index argument.  When it's -1, the next
free slot is assigned.


Takashi

Re: [Alsa-devel] Re: Patch for AICA sound support on SEGA Dreamcast

[Adrian McMenamin]

On Mon, 2006-04-03 at 20:29 +0200, Takashi Iwai wrote:

> 
> Anyway, you should use dma_wait_for_completion() instead of your
> home-made one.
> 
The dma_wait_for_completion is broken for g2 dma afaics. Paul - would
you concur?

(Unfortunately the dma_residue register in g2 does not stick at 0 when
there is no residue but returns to the size of the completed dma
transfer. Therefore I can really see no alternative but to writing my
own unless Paul has a better idea.)

Re: Patch for AICA sound support on SEGA Dreamcast

[Carlos Munoz]

Takashi Iwai wrote:

>Please avoid use of typedefs as much as possible.
>We (finally :-) got rid of whole typedefs recently from the ALSA core
>code.
>
>  
>
Hi Takashi,

Just out of curiosity. What is the reason for no using typedefs ?

Thanks,


Carlos Munoz

Re: Patch for AICA sound support on SEGA Dreamcast

[Takashi Iwai]

At Mon, 03 Apr 2006 11:00:20 -0700,
Carlos Munoz wrote:
> 
> Takashi Iwai wrote:
> 
> >Please avoid use of typedefs as much as possible.
> >We (finally :-) got rid of whole typedefs recently from the ALSA core
> >code.
> >
> >  
> >
> Hi Takashi,
> 
> Just out of curiosity. What is the reason for no using typedefs ?

Just to follow the convention of coding style in kernel tree.

There are some reasons that typedefs are evil, but I don't want to
start flames :)


Takashi

Re: Patch for AICA sound support on SEGA Dreamcast

[Alistair John Strachan]

On Monday 03 April 2006 19:32, Takashi Iwai wrote:
> At Mon, 03 Apr 2006 11:00:20 -0700,
>
> Carlos Munoz wrote:
> > Takashi Iwai wrote:
> > >Please avoid use of typedefs as much as possible.
> > >We (finally :-) got rid of whole typedefs recently from the ALSA core
> > >code.
> >
> > Hi Takashi,
> >
> > Just out of curiosity. What is the reason for no using typedefs ?
>
> Just to follow the convention of coding style in kernel tree.
>
> There are some reasons that typedefs are evil, but I don't want to
> start flames :)

I also do not wish to start a flame war, but I will direct Carlos to this URL:

http://www.linuxjournal.com/article/5780

I think this write-up provides justification. However, it is not part of 
linux/Documentation/CodingStyle. Perhaps somebody should add these details to 
this file, so that new code follows this currently 'unwritten' rule.

-- 
Cheers,
Alistair.

'No sense being pessimistic, it probably wouldn't work anyway.'
Third year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

Re: [Alsa-devel] Re: Patch for AICA sound support on SEGA Dreamcast

[Lee Revell]

On Tue, 2006-04-04 at 01:16 +0100, Alistair John Strachan wrote:
> I think this write-up provides justification. However, it is not part
> of linux/Documentation/CodingStyle. Perhaps somebody should add these
> details to this file, so that new code follows this currently
> 'unwritten' rule. 

Well, if you look at the current ALSA code, the only typedefs provided
are for backwards compatibility.  They were all removed from the drivers
months ago.  The problem is that you were working against old code.  The
main coding style rule is to follow the conventions of the nearby code,
which would mean no typedefs in the ALSA driver case.

Lee

Re: [Alsa-devel] Re: Patch for AICA sound support on SEGA Dreamcast

[Adrian McMenamin]

On Mon, 2006-04-03 at 17:52 +0200, Takashi Iwai wrote:

> > +static void __exit aica_exit(void)
> > +{
> > +
> > +	if (likely(dreamcastcard->card)) {
> 
> You don't need such an optimization in this place at all.
> 
> > +		snd_aicapcm_free(dreamcastcard);
> > +		snd_card_free(dreamcastcard->card);
> > +		kfree(dreamcastcard);
> > +		if (likely(pd)) {
> > +			struct device_driver *drv = (&pd->dev)->driver;
> > +			device_release_driver(&pd->dev);
> > +			driver_unregister(drv);
> > +			platform_device_unregister(pd);
> > +		}
> 
> This should be in remove callback.
> 

What is the remove callback?