[PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Adrian McMenamin]

[-- Attachment #1: Type: text/plain, Size: 242 bytes --]

Takashi,

Here is a second attempt at this - this time built against the
alsa-driver sources rather than the kernel mainline. please consider for
submission to alsa-driver.

Signed off by Adrian McMenamin <adrian@mcmen.demon.co.uk>

Adrian



[-- Attachment #2: aica.diff --]
[-- Type: text/x-patch, Size: 23924 bytes --]

diff -ruN --exclude=acore --exclude='.#*' ./sh/aica.c /home/adrian/alsa-driver/sh/aica.c
--- ./sh/aica.c	1970-01-01 01:00:00.000000000 +0100
+++ /home/adrian/alsa-driver/sh/aica.c	2006-04-16 23:13:59.000000000 +0100
@@ -0,0 +1,833 @@
+/*
+* This code is licenced under 
+* the General Public Licence
+* version 2
+*
+* Copyright Adrian McMenamin 2005, 2006
+* <adrian@mcmen.demon.co.uk>
+* See also http://newgolddream.dyndns.info/cgi-bin/cvsweb
+* 
+*
+* This program is free software; you can redistribute it and/or modify
+* it under the terms of version 2 of the GNU General Public License as published by
+* the Free Software Foundation.
+*
+* This program is distributed in the hope that it will be useful,
+* but WITHOUT ANY WARRANTY; without even the implied warranty of
+* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+* GNU General Public License for more details.
+*
+* You should have received a copy of the GNU General Public License
+* along with this program; if not, write to the Free Software
+* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+*
+*/
+
+
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/slab.h>
+#include <linux/time.h>
+#include <linux/wait.h>
+#include <linux/moduleparam.h>
+#include <linux/platform_device.h>
+#include <linux/firmware.h>
+#include <linux/timer.h>
+#include <linux/delay.h>
+#include <asm-sh/io.h>
+#include <asm-sh/dma.h>
+#include <asm-sh/dreamcast/sysasic.h>
+#include <sound/driver.h>
+#include <sound/core.h>
+#include <sound/control.h>
+#include <sound/pcm.h>
+#include <sound/initval.h>
+#include <sound/info.h>
+#include "aica.h"
+
+
+MODULE_AUTHOR("Adrian McMenamin <adrian@mcmen.demon.co.uk>");
+MODULE_DESCRIPTION("Dreamcast AICA sound (pcm) driver");
+MODULE_LICENSE("GPL");
+MODULE_SUPPORTED_DEVICE("{{Yamaha/SEGA, AICA}}");
+
+/* module parameters */
+#define CARD_NAME "AICA"
+
+static int index;
+static char *id = NULL;
+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.");
+
+
+
+/* Spinlocks */
+spinlock_t spu_memlock = SPIN_LOCK_UNLOCKED;
+spinlock_t spu_dmalock = SPIN_LOCK_UNLOCKED;
+
+
+/* SPU specific functions */
+
+/* spu_write_wait - wait for G2-SH FIFO to clear */
+static inline void spu_write_wait(void)
+{
+	int time_count;
+	time_count = 0;
+	while (1) {
+		if (!(readl(G2_FIFO) & 0x11))
+			break;
+		/* To ensure hardware failure doesn't wedge kernel */
+		time_count++;
+		if (time_count > 0x10000)
+			break;
+	}
+
+	return;
+}
+
+
+/* spu_memset - write to memory in SPU address space */
+static void spu_memset(uint32_t toi, void __iomem * what, int length)
+{
+	uint32_t *to = (uint32_t *) (SPU_MEMORY_BASE + toi);
+	int i;
+
+
+	if (length % 4)
+		length = (length / 4) + 1;
+	else
+		length = length / 4;
+	spu_write_wait();
+	for (i = 0; i < length; i++) {
+		spin_lock(&spu_memlock);
+		writel(what, to);
+		spin_unlock(&spu_memlock);
+		to++;
+		if (i && !(i % 8))
+			spu_write_wait();
+	}
+	return;
+}
+
+/* spu_memload - write to SPU address space */
+static void spu_memload(uint32_t toi, void __iomem * from, int length)
+{
+	uint32_t __iomem *froml = from;
+	uint32_t __iomem *to =
+	    (uint32_t __iomem *) (SPU_MEMORY_BASE + toi);
+	int i, val;
+	if (length % 4)
+		length = (length / 4) + 1;
+	else
+		length = length / 4;
+	spu_write_wait();
+	for (i = 0; i < length; i++) {
+		val = *froml;
+		spin_lock(&spu_memlock);
+		writel(val, to);
+		spin_unlock(&spu_memlock);
+		froml++;
+		to++;
+		if (i && !(i % 8))
+			spu_write_wait();
+	}
+	return;
+
+}
+
+/* spu_disable - set spu registers to stop sound output */
+static void spu_disable(void)
+{
+	int i;
+	uint32_t regval;
+	spu_write_wait();
+	regval = readl(ARM_RESET_REGISTER);
+	regval |= 1;
+	spu_write_wait();
+	spin_lock(&spu_memlock);
+	writel(regval, ARM_RESET_REGISTER);
+	spin_unlock(&spu_memlock);
+	for (i = 0; i < 64; i++) {
+		spu_write_wait();
+		regval = readl(SPU_REGISTER_BASE + (i * 0x80));
+		regval = (regval & ~0x4000) | 0x8000;
+		spu_write_wait();
+		spin_lock(&spu_memlock);
+		writel(regval, SPU_REGISTER_BASE + (i * 0x80));
+		spin_unlock(&spu_memlock);
+
+	}
+
+}
+
+/* spu_enable - set spu registers to enable sound output */
+static void spu_enable(void)
+{
+	uint32_t regval = readl(ARM_RESET_REGISTER);
+	regval &= ~1;
+	spu_write_wait();
+	spin_lock(&spu_memlock);
+	writel(regval, ARM_RESET_REGISTER);
+	spin_unlock(&spu_memlock);
+	return;
+
+}
+
+
+/* Halt the sound processor,
+   clear the memory,
+   load some default ARM7 code,
+   and then restart ARM7
+*/
+static int spu_init(void)
+{
+
+	spu_disable();
+	spu_memset(0, 0, 0x200000 / 4);
+	*(uint32_t *) SPU_MEMORY_BASE = 0xea000002;
+	spu_enable();
+	schedule();
+	return 0;
+}
+
+
+
+
+/* aica_chn_start - write to spu to start playback */
+inline static void aica_chn_start(void)
+{
+	spu_write_wait();
+	spin_lock(&spu_memlock);
+	writel(AICA_CMD_KICK | AICA_CMD_START,
+	       (uint32_t *) AICA_CONTROL_POINT);
+	spin_unlock(&spu_memlock);
+
+}
+
+/* aica_chn_halt - write to spu to halt playback */
+inline static void aica_chn_halt(void)
+{
+	spu_write_wait();
+	spin_lock(&spu_memlock);
+	writel(AICA_CMD_KICK | AICA_CMD_STOP,
+	       (uint32_t *) AICA_CONTROL_POINT);
+	spin_unlock(&spu_memlock);
+
+}
+
+
+/* ALSA code below */
+
+static struct snd_pcm_hardware snd_pcm_aica_playback_hw = {
+
+	.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,
+};
+
+/* Simple platform device */
+static struct platform_device *pd = NULL;
+
+
+
+static int stereo_buffer_transfer(struct snd_pcm_substream *substream,
+				  int buffer_size, int period)
+{
+	int transferred;
+	struct snd_pcm_runtime *runtime;
+	int period_offset;
+	period_offset = period;
+	period_offset %= (AICA_PERIOD_NUMBER / 2);
+	runtime = substream->runtime;
+
+	/* transfer left and then right */
+	spin_lock(&spu_dmalock);
+	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);
+	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);
+	spin_unlock(&spu_dmalock);
+	return 0;
+}
+
+
+
+
+void static aica_period_elapsed(unsigned long timer_var)
+{
+	int transferred;
+	int play_period;
+	struct snd_pcm_runtime *runtime;
+	struct snd_pcm_substream *substream;
+	struct snd_card_aica *dreamcastcard;
+	substream = (struct snd_pcm_substream *) timer_var;
+	runtime = substream->runtime;
+	dreamcastcard =
+	    (struct snd_card_aica *) (substream->pcm->private_data);
+	/* 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));
+		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 {
+		spin_lock(&spu_dmalock);
+		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);
+		spin_unlock(&spu_dmalock);
+	}
+
+	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(struct snd_pcm_substream *substream)
+{
+	struct snd_pcm_runtime *runtime;
+	struct aica_channel *channel;
+	struct snd_card_aica *dreamcastcard;
+
+	if (!enable)
+		return -ENOENT;
+	dreamcastcard =
+	    (struct snd_card_aica *) (substream->pcm->private_data);
+	channel = kmalloc(sizeof(struct aica_channel), 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 */
+	dreamcastcard->channel = channel;
+	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(struct snd_pcm_substream *substream)
+{
+	struct snd_card_aica *dreamcastcard =
+	    (struct snd_card_aica *) (substream->pcm->private_data);
+	del_timer(&dreamcastcard->timer);
+	kfree(dreamcastcard->channel);
+	spu_disable();
+	return 0;
+}
+
+static int snd_aicapcm_pcm_hw_free(struct snd_pcm_substream *substream)
+{
+	/* Free the DMA buffer */
+	return snd_pcm_lib_free_pages(substream);
+}
+
+static int snd_aicapcm_pcm_hw_params(struct snd_pcm_substream *substream,
+				     struct snd_pcm_hw_params *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(struct snd_pcm_substream *substream)
+{
+
+	struct snd_card_aica *dreamcastcard =
+	    (struct snd_card_aica *) (substream->pcm->private_data);
+	/* Set up the playback */
+	/* basic settings to test working */
+	if ((substream->runtime)->format == SNDRV_PCM_FORMAT_S16_LE)
+		dreamcastcard->channel->sfmt = SM_16BIT;
+	dreamcastcard->channel->freq = substream->runtime->rate;
+	dreamcastcard->substream = substream;
+	return 0;
+}
+
+
+static void startup_aica(struct snd_card_aica *dreamcastcard)
+{
+	spu_memload(AICA_CHANNEL0_CONTROL_OFFSET,
+		    (uint8_t *) dreamcastcard->channel,
+		    sizeof(struct aica_channel));
+	aica_chn_start();
+	return;
+}
+
+
+static void spu_begin_dma(struct snd_pcm_substream *substream)
+{
+	int buffer_size;
+	snd_pcm_runtime_t *runtime;
+	struct snd_card_aica *dreamcastcard =
+	    (struct snd_card_aica *) (substream->pcm->private_data);
+	runtime = substream->runtime;
+	buffer_size = frames_to_bytes(runtime, runtime->buffer_size);
+	if (runtime->channels == 1) {
+		spin_lock(&spu_dmalock);
+		dma_xfer(0, runtime->dma_area, AICA_CHANNEL0_OFFSET,
+			 buffer_size, 5);
+		spin_unlock(&spu_dmalock);
+	} else {
+		int transfer_status;
+		dreamcastcard->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(dreamcastcard);
+	/* Timer may already be running - if so delete it */
+	if (dreamcastcard->timer.data)
+		del_timer(&dreamcastcard->timer);
+
+	init_timer(&(dreamcastcard->timer));
+	dreamcastcard->timer.data = (unsigned long) substream;
+	dreamcastcard->timer.function = aica_period_elapsed;
+
+
+	dreamcastcard->timer.expires = jiffies + 4;
+	add_timer(&(dreamcastcard->timer));
+	return;
+}
+
+static int snd_aicapcm_pcm_trigger(struct snd_pcm_substream *substream,
+				   int cmd)
+{
+	struct snd_card_aica *dreamcastcard;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+		spu_begin_dma(substream);
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+		dreamcastcard =
+		    (struct snd_card_aica *) (substream->pcm->
+					      private_data);
+
+		if (dreamcastcard->timer.data)
+			del_timer(&dreamcastcard->timer);
+		aica_chn_halt();
+		break;
+	default:
+		return -EINVAL;
+	}
+
+
+	return 0;
+}
+
+static unsigned long snd_aicapcm_pcm_pointer(struct snd_pcm_substream
+					     *substream)
+{
+	return readl(AICA_CONTROL_CHANNEL_SAMPLE_NUMBER);
+
+}
+
+
+static struct snd_pcm_ops 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 void snd_aicapcm_free(void)
+{
+	release_mem_region(ARM_RESET_REGISTER, 0x4);
+	release_mem_region(SPU_MEMORY_BASE, 0x200000);
+}
+
+/* TO DO: set up to handle more than one pcm instance */
+
+
+static int __init snd_aicapcmchip(struct snd_card_aica *dreamcastcard,
+				  int pcm_index)
+{
+	struct snd_pcm *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;
+	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(struct snd_kcontrol *kcontrol,
+			       struct snd_ctl_elem_info *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(struct snd_kcontrol *kcontrol,
+			      struct snd_ctl_elem_value *ucontrol)
+{
+	ucontrol->value.integer.value[0] = 1;	/* TO DO: Fix me */
+	return 0;
+}
+
+static int aica_pcmswitch_put(struct snd_kcontrol *kcontrol,
+			      struct snd_ctl_elem_value *ucontrol)
+{
+	if (ucontrol->value.integer.value[0] == 1)
+		return 0;	/* TO DO: Fix me */
+	else
+		aica_chn_halt();
+	return 0;
+}
+
+
+
+
+static int aica_pcmvolume_info(struct snd_kcontrol *kcontrol,
+			       struct snd_ctl_elem_info *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(struct snd_kcontrol *kcontrol,
+			      struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_card_aica *dreamcastcard;
+	dreamcastcard = (struct snd_card_aica *) kcontrol->private_value;
+	if (!dreamcastcard->channel)
+		return -ETXTBSY;	/* we've not yet been set up */
+	ucontrol->value.integer.value[0] = dreamcastcard->channel->vol;
+	return 0;
+}
+
+static int aica_pcmvolume_put(struct snd_kcontrol *kcontrol,
+			      struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_card_aica *dreamcastcard;
+	dreamcastcard = (struct snd_card_aica *) kcontrol->private_value;
+
+	if (!dreamcastcard->channel) {
+		snd_printk("No channel yet\n");
+		return -ETXTBSY;	/* too soon */
+	}
+
+	else if (dreamcastcard->channel->vol ==
+		 ucontrol->value.integer.value[0])
+		return 0;
+
+	else {
+		dreamcastcard->channel->vol =
+		    ucontrol->value.integer.value[0];
+		dreamcastcard->master_volume =
+		    ucontrol->value.integer.value[0];
+		spu_memload(AICA_CHANNEL0_CONTROL_OFFSET,
+			    (uint8_t *) dreamcastcard->channel,
+			    sizeof(struct aica_channel));
+	}
+	return 1;
+}
+
+
+static struct snd_kcontrol_new snd_aica_pcmswitch_control __devinitdata = {
+	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+	.name = "PCM Playback Switch",
+	.index = 0,
+	.private_value = 0xffff,
+	.info = aica_pcmswitch_info,
+	.get = aica_pcmswitch_get,
+	.put = aica_pcmswitch_put
+};
+
+
+
+static struct snd_kcontrol_new snd_aica_pcmvolume_control __devinitdata = {
+	.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+	.name = "PCM Playback Volume",
+	.index = 0,
+	.private_value = 0xffff,
+	.info = aica_pcmvolume_info,
+	.get = aica_pcmvolume_get,
+	.put = aica_pcmvolume_put
+};
+
+static int remove_dreamcastcard(struct device *dreamcast_device)
+{
+	struct snd_card_aica *dreamcastcard =
+	    (struct snd_card_aica *) dreamcast_device->driver_data;
+	snd_aicapcm_free();
+	snd_card_free(dreamcastcard->card);
+	kfree(dreamcastcard);
+	return 0;
+
+}
+
+
+static struct device_driver aica_driver = {
+	.name = "AICA",
+	.bus = &platform_bus_type,
+	.remove = remove_dreamcastcard,
+};
+
+
+/* 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);
+}
+
+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(struct snd_card_aica
+					    *dreamcastcard)
+{
+	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;
+	}
+
+	/* succeeded */
+	return 0;
+}
+
+
+
+static int __init aica_init(void)
+{
+
+
+	int err;
+	struct snd_card_aica *dreamcastcard;
+
+	/* Are we in a Dreamcast at all? */
+	if (!mach_is_dreamcast())
+		return -ENODEV;
+	dreamcastcard = kmalloc(sizeof(struct snd_card_aica), GFP_KERNEL);
+	if (!dreamcastcard)
+		return -ENOMEM;
+	dreamcastcard->card = snd_card_new(-1, "AICA", THIS_MODULE, 0);
+	if (!dreamcastcard->card) {
+		kfree(dreamcastcard);
+		return -ENODEV;
+	}
+	strcpy(dreamcastcard->card->driver, "snd_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);
+	pd->dev.driver_data = dreamcastcard;
+	dreamcastcard->timer.data = 0;
+	dreamcastcard->channel = NULL;
+
+	/* Load the firmware */
+	if ((err = load_aica_firmware()) < 0)
+		goto freepcm;
+
+	/* Add basic controls */
+	if (add_aicamixer_controls(dreamcastcard) < 0)
+		goto freepcm;
+
+
+	/* Register the card with ALSA subsystem */
+	if ((err = snd_card_register(dreamcastcard->card)) < 0)
+		goto freepcm;
+
+
+	snd_printk
+	    ("ALSA Driver for Yamaha AICA Super Intelligent Sound Processor\n");
+
+	return 0;
+
+      freepcm:
+	snd_aicapcm_free();
+
+      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)
+{
+	struct device_driver *drv = (&pd->dev)->driver;
+	device_release_driver(&pd->dev);
+	driver_unregister(drv);
+	platform_device_unregister(pd);
+	/* Kill any sound still playing and reset ARM7 to safe state */
+	spu_init();
+
+
+	return;
+}
+
+
+module_init(aica_init);
+module_exit(aica_exit);
diff -ruN --exclude=acore --exclude='.#*' ./sh/aica.h /home/adrian/alsa-driver/sh/aica.h
--- ./sh/aica.h	1970-01-01 01:00:00.000000000 +0100
+++ /home/adrian/alsa-driver/sh/aica.h	2006-04-15 16:48:01.000000000 +0100
@@ -0,0 +1,83 @@
+/* aica.h
+ * Header file for ALSA driver for
+ * Sega Dreamcast Yamaha AICA sound
+ * Copyright Adrian McMenamin
+ * <adrian@mcmen.demon.co.uk>
+ * 2006
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ */
+
+/* SPU memory and register constants etc */
+#define G2_FIFO 0xa05f688c
+#define SPU_MEMORY_BASE 0xA0800000
+#define ARM_RESET_REGISTER 0xA0702C00
+#define SPU_REGISTER_BASE 0xA0700000
+
+
+
+/* AICA channels stuff */
+
+#define AICA_CONTROL_POINT 0xA0810000
+#define AICA_CONTROL_CHANNEL_SAMPLE_NUMBER 0xA0810008
+#define AICA_CHANNEL0_CONTROL_OFFSET 0x10004
+
+/* Command values */
+#define AICA_CMD_KICK 0x80000000
+#define AICA_CMD_NONE 0
+#define AICA_CMD_START 1
+#define AICA_CMD_STOP 2
+#define AICA_CMD_VOL 3
+
+/* Sound modes */
+#define SM_8BIT		1
+#define SM_16BIT	0
+#define SM_ADPCM	2
+
+/* Buffer and period size */
+#define AICA_BUFFER_SIZE 0x8000
+#define AICA_PERIOD_SIZE 0x800
+#define AICA_PERIOD_NUMBER 16
+
+#define AICA_CHANNEL0_OFFSET 0x11000
+#define AICA_CHANNEL1_OFFSET 0x21000
+
+
+
+
+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            */
+};
+
+
+struct snd_card_aica {
+	struct snd_card *card;
+	struct aica_channel *channel;
+	snd_pcm_substream_t *substream;
+	int clicks;
+	int current_period;
+	struct timer_list timer;
+	int master_volume;
+
+};
+
+
diff -ruN --exclude=acore --exclude='.#*' ./sh/Makefile /home/adrian/alsa-driver/sh/Makefile
--- ./sh/Makefile	1970-01-01 01:00:00.000000000 +0100
+++ /home/adrian/alsa-driver/sh/Makefile	2006-04-14 18:48:02.000000000 +0100
@@ -0,0 +1,11 @@
+ifndef SND_TOPDIR
+SND_TOPDIR=..
+endif
+
+include $(SND_TOPDIR)/toplevel.config
+include $(SND_TOPDIR)/Makefile.conf
+
+snd-aica-objs := aica.o
+obj-$(CONFIG_SND_AICA) += snd-aica.o
+
+include $(SND_TOPDIR)/Rules.make

Re: [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Paul Mundt]

A few quick comments.. though looking at the earlier thread, most of
these were already pointed out..

On Mon, Apr 17, 2006 at 01:13:04AM +0100, Adrian McMenamin wrote:
> diff -ruN --exclude=acore --exclude='.#*' ./sh/aica.c /home/adrian/alsa-driver/sh/aica.c
> --- ./sh/aica.c	1970-01-01 01:00:00.000000000 +0100
> +++ /home/adrian/alsa-driver/sh/aica.c	2006-04-16 23:13:59.000000000 +0100

-p1 format please.

> +#include <linux/init.h>
> +#include <linux/jiffies.h>
> +#include <linux/slab.h>
> +#include <linux/time.h>
> +#include <linux/wait.h>
> +#include <linux/moduleparam.h>
> +#include <linux/platform_device.h>
> +#include <linux/firmware.h>
> +#include <linux/timer.h>
> +#include <linux/delay.h>
> +#include <asm-sh/io.h>
> +#include <asm-sh/dma.h>
> +#include <asm-sh/dreamcast/sysasic.h>

What's wrong with asm/? asm/ includes should also be last.

> +
> +

This patch adds a lot of superfluous whitespace, please trim it.

> +static char *id = NULL;

Pointless initializer.

> +
> +/* Spinlocks */
> +spinlock_t spu_memlock = SPIN_LOCK_UNLOCKED;
> +spinlock_t spu_dmalock = SPIN_LOCK_UNLOCKED;
> +
DEFINE_SPINLOCK()..

> +
> +/* SPU specific functions */
> +
> +/* spu_write_wait - wait for G2-SH FIFO to clear */
> +static inline void spu_write_wait(void)
> +{
> +	int time_count;
> +	time_count = 0;
> +	while (1) {
> +		if (!(readl(G2_FIFO) & 0x11))
> +			break;
> +		/* To ensure hardware failure doesn't wedge kernel */
> +		time_count++;
> +		if (time_count > 0x10000)
> +			break;
> +	}
> +
> +	return;
> +}
> +
Useless return.

> +
> +/* spu_memset - write to memory in SPU address space */
> +static void spu_memset(uint32_t toi, void __iomem * what, int length)
> +{
> +	uint32_t *to = (uint32_t *) (SPU_MEMORY_BASE + toi);
> +	int i;
> +
> +
> +	if (length % 4)
> +		length = (length / 4) + 1;
> +	else
> +		length = length / 4;
> +	spu_write_wait();
> +	for (i = 0; i < length; i++) {
> +		spin_lock(&spu_memlock);
> +		writel(what, to);
> +		spin_unlock(&spu_memlock);
> +		to++;
> +		if (i && !(i % 8))
> +			spu_write_wait();
> +	}
> +	return;
> +}
> +
Likewise.

> +static void spu_memload(uint32_t toi, void __iomem * from, int length)
> +{
> +	uint32_t __iomem *froml = from;
> +	uint32_t __iomem *to =
> +	    (uint32_t __iomem *) (SPU_MEMORY_BASE + toi);
> +	int i, val;
> +	if (length % 4)
> +		length = (length / 4) + 1;
> +	else
> +		length = length / 4;
> +	spu_write_wait();
> +	for (i = 0; i < length; i++) {
> +		val = *froml;
> +		spin_lock(&spu_memlock);
> +		writel(val, to);
> +		spin_unlock(&spu_memlock);
> +		froml++;
> +		to++;
> +		if (i && !(i % 8))
> +			spu_write_wait();
> +	}
> +	return;
> +
> +}
> +
And again.

> +/* Halt the sound processor,
> +   clear the memory,
> +   load some default ARM7 code,
> +   and then restart ARM7
> +*/
> +static int spu_init(void)
> +{
> +
> +	spu_disable();
> +	spu_memset(0, 0, 0x200000 / 4);
> +	*(uint32_t *) SPU_MEMORY_BASE = 0xea000002;

Why are you not using ctrl_outl() or something similar? This should also
be documented a bit better if it's not magic..

> +/* aica_chn_start - write to spu to start playback */
> +inline static void aica_chn_start(void)

static inline void..

> +/* aica_chn_halt - write to spu to halt playback */
> +inline static void aica_chn_halt(void)
> +{
Likewise..

> +/* Simple platform device */
> +static struct platform_device *pd = NULL;
> +
> +
> +
Useless initializer and whitespace damage.

> +static int stereo_buffer_transfer(struct snd_pcm_substream *substream,
> +				  int buffer_size, int period)
> +{
> +	int transferred;
> +	struct snd_pcm_runtime *runtime;
> +	int period_offset;
> +	period_offset = period;
> +	period_offset %= (AICA_PERIOD_NUMBER / 2);
> +	runtime = substream->runtime;
> +
> +	/* transfer left and then right */
> +	spin_lock(&spu_dmalock);
> +	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);

You can't be serious, you're trying to setup, transfer, and wait for
completion for a DMA transfer while holding a spinlock? The fact this
hasn't blown up on you is sheer luck. Use dma_wait_for_completion(), it
does the right thing.

Additionaly you need a timeout here if you were for some reason intent on
doing this while working against the DMA subsystem, if your DMA gets
stuck this will blow up.

> +void static aica_period_elapsed(unsigned long timer_var)
> +{

static void..

> +	int transferred;
> +	int play_period;
> +	struct snd_pcm_runtime *runtime;
> +	struct snd_pcm_substream *substream;
> +	struct snd_card_aica *dreamcastcard;
> +	substream = (struct snd_pcm_substream *) timer_var;
> +	runtime = substream->runtime;
> +	dreamcastcard =
> +	    (struct snd_card_aica *) (substream->pcm->private_data);

Useless cast.

> +	} else {
> +		spin_lock(&spu_dmalock);
> +		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);
> +		spin_unlock(&spu_dmalock);
> +	}
> +
spinlock / DMA brain damage.

> +	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;
> +}
> +
Superfluous return.

> +static int snd_aicapcm_pcm_open(struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime;
> +	struct aica_channel *channel;
> +	struct snd_card_aica *dreamcastcard;
> +
> +	if (!enable)
> +		return -ENOENT;

unlikely()?

> +	dreamcastcard =
> +	    (struct snd_card_aica *) (substream->pcm->private_data);

Useless cast.

> +static int snd_aicapcm_pcm_close(struct snd_pcm_substream *substream)
> +{
> +	struct snd_card_aica *dreamcastcard =
> +	    (struct snd_card_aica *) (substream->pcm->private_data);
Likewise.

> +static int snd_aicapcm_pcm_prepare(struct snd_pcm_substream *substream)
> +{
> +
> +	struct snd_card_aica *dreamcastcard =
> +	    (struct snd_card_aica *) (substream->pcm->private_data);

And again.

> +static void startup_aica(struct snd_card_aica *dreamcastcard)
> +{
> +	spu_memload(AICA_CHANNEL0_CONTROL_OFFSET,
> +		    (uint8_t *) dreamcastcard->channel,
> +		    sizeof(struct aica_channel));
> +	aica_chn_start();
> +	return;
> +}
> +
Useless return.

> +
> +static void spu_begin_dma(struct snd_pcm_substream *substream)
> +{
> +	int buffer_size;
> +	snd_pcm_runtime_t *runtime;
> +	struct snd_card_aica *dreamcastcard =
> +	    (struct snd_card_aica *) (substream->pcm->private_data);

Superfluous cast.

> +	runtime = substream->runtime;
> +	buffer_size = frames_to_bytes(runtime, runtime->buffer_size);
> +	if (runtime->channels == 1) {
> +		spin_lock(&spu_dmalock);
> +		dma_xfer(0, runtime->dma_area, AICA_CHANNEL0_OFFSET,
> +			 buffer_size, 5);
> +		spin_unlock(&spu_dmalock);

Broken locking.

> +static int __init snd_aicapcmchip(struct snd_card_aica *dreamcastcard,
> +				  int pcm_index)
> +{
> +	struct snd_pcm *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;
> +	}
> +
Why aren't these setup in the platform device?

> +	/* AICA has no capture ability */
> +	if ((err =
> +	     snd_pcm_new(dreamcastcard->card, "AICA PCM", pcm_index, 1, 0,
> +			 &pcm)) < 0)
> +		return err;

Weird notation, linux kernel style would be:

	err = snc_pcm_new(...);
	if (unlikely(err < 0))
		return err;

please refactor accordingly.

> +static int aica_pcmvolume_put(struct snd_kcontrol *kcontrol,
> +			      struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_card_aica *dreamcastcard;
> +	dreamcastcard = (struct snd_card_aica *) kcontrol->private_value;
> +
> +	if (!dreamcastcard->channel) {
> +		snd_printk("No channel yet\n");
> +		return -ETXTBSY;	/* too soon */
> +	}
> +
> +	else if (dreamcastcard->channel->vol ==
> +		 ucontrol->value.integer.value[0])
> +		return 0;
> +
> +	else {

The if/elses are all over the place, please see
Documentation/CodingStyle.

> +static struct device_driver aica_driver = {
> +	.name = "AICA",
> +	.bus = &platform_bus_type,
> +	.remove = remove_dreamcastcard,
> +};
> +
Deprecated, see struct platform_device.

> +	pd = platform_device_register_simple(dreamcastcard->card->driver,
> +					     -1, NULL, 0);
> +
This is also going away, you'll want to update this for the new platform
device semantics.

> +      freepcm:
> +	snd_aicapcm_free();
> +
> +      freedreamcast:
> +	snd_card_free(dreamcastcard->card);
> +
Why are these goto labels in magical locations?

> +static void __exit aica_exit(void)
> +{
> +	struct device_driver *drv = (&pd->dev)->driver;
> +	device_release_driver(&pd->dev);
> +	driver_unregister(drv);
> +	platform_device_unregister(pd);
> +	/* Kill any sound still playing and reset ARM7 to safe state */
> +	spu_init();
> +
> +
> +	return;
> +}
> +
And another useless return.


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: [linuxsh-dev] [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Paul Mundt]

A few quick comments.. though looking at the earlier thread, most of
these were already pointed out..

On Mon, Apr 17, 2006 at 01:13:04AM +0100, Adrian McMenamin wrote:
> diff -ruN --exclude=acore --exclude='.#*' ./sh/aica.c /home/adrian/alsa-driver/sh/aica.c
> --- ./sh/aica.c	1970-01-01 01:00:00.000000000 +0100
> +++ /home/adrian/alsa-driver/sh/aica.c	2006-04-16 23:13:59.000000000 +0100

-p1 format please.

> +#include <linux/init.h>
> +#include <linux/jiffies.h>
> +#include <linux/slab.h>
> +#include <linux/time.h>
> +#include <linux/wait.h>
> +#include <linux/moduleparam.h>
> +#include <linux/platform_device.h>
> +#include <linux/firmware.h>
> +#include <linux/timer.h>
> +#include <linux/delay.h>
> +#include <asm-sh/io.h>
> +#include <asm-sh/dma.h>
> +#include <asm-sh/dreamcast/sysasic.h>

What's wrong with asm/? asm/ includes should also be last.

> +
> +

This patch adds a lot of superfluous whitespace, please trim it.

> +static char *id = NULL;

Pointless initializer.

> +
> +/* Spinlocks */
> +spinlock_t spu_memlock = SPIN_LOCK_UNLOCKED;
> +spinlock_t spu_dmalock = SPIN_LOCK_UNLOCKED;
> +
DEFINE_SPINLOCK()..

> +
> +/* SPU specific functions */
> +
> +/* spu_write_wait - wait for G2-SH FIFO to clear */
> +static inline void spu_write_wait(void)
> +{
> +	int time_count;
> +	time_count = 0;
> +	while (1) {
> +		if (!(readl(G2_FIFO) & 0x11))
> +			break;
> +		/* To ensure hardware failure doesn't wedge kernel */
> +		time_count++;
> +		if (time_count > 0x10000)
> +			break;
> +	}
> +
> +	return;
> +}
> +
Useless return.

> +
> +/* spu_memset - write to memory in SPU address space */
> +static void spu_memset(uint32_t toi, void __iomem * what, int length)
> +{
> +	uint32_t *to = (uint32_t *) (SPU_MEMORY_BASE + toi);
> +	int i;
> +
> +
> +	if (length % 4)
> +		length = (length / 4) + 1;
> +	else
> +		length = length / 4;
> +	spu_write_wait();
> +	for (i = 0; i < length; i++) {
> +		spin_lock(&spu_memlock);
> +		writel(what, to);
> +		spin_unlock(&spu_memlock);
> +		to++;
> +		if (i && !(i % 8))
> +			spu_write_wait();
> +	}
> +	return;
> +}
> +
Likewise.

> +static void spu_memload(uint32_t toi, void __iomem * from, int length)
> +{
> +	uint32_t __iomem *froml = from;
> +	uint32_t __iomem *to =
> +	    (uint32_t __iomem *) (SPU_MEMORY_BASE + toi);
> +	int i, val;
> +	if (length % 4)
> +		length = (length / 4) + 1;
> +	else
> +		length = length / 4;
> +	spu_write_wait();
> +	for (i = 0; i < length; i++) {
> +		val = *froml;
> +		spin_lock(&spu_memlock);
> +		writel(val, to);
> +		spin_unlock(&spu_memlock);
> +		froml++;
> +		to++;
> +		if (i && !(i % 8))
> +			spu_write_wait();
> +	}
> +	return;
> +
> +}
> +
And again.

> +/* Halt the sound processor,
> +   clear the memory,
> +   load some default ARM7 code,
> +   and then restart ARM7
> +*/
> +static int spu_init(void)
> +{
> +
> +	spu_disable();
> +	spu_memset(0, 0, 0x200000 / 4);
> +	*(uint32_t *) SPU_MEMORY_BASE = 0xea000002;

Why are you not using ctrl_outl() or something similar? This should also
be documented a bit better if it's not magic..

> +/* aica_chn_start - write to spu to start playback */
> +inline static void aica_chn_start(void)

static inline void..

> +/* aica_chn_halt - write to spu to halt playback */
> +inline static void aica_chn_halt(void)
> +{
Likewise..

> +/* Simple platform device */
> +static struct platform_device *pd = NULL;
> +
> +
> +
Useless initializer and whitespace damage.

> +static int stereo_buffer_transfer(struct snd_pcm_substream *substream,
> +				  int buffer_size, int period)
> +{
> +	int transferred;
> +	struct snd_pcm_runtime *runtime;
> +	int period_offset;
> +	period_offset = period;
> +	period_offset %= (AICA_PERIOD_NUMBER / 2);
> +	runtime = substream->runtime;
> +
> +	/* transfer left and then right */
> +	spin_lock(&spu_dmalock);
> +	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);

You can't be serious, you're trying to setup, transfer, and wait for
completion for a DMA transfer while holding a spinlock? The fact this
hasn't blown up on you is sheer luck. Use dma_wait_for_completion(), it
does the right thing.

Additionaly you need a timeout here if you were for some reason intent on
doing this while working against the DMA subsystem, if your DMA gets
stuck this will blow up.

> +void static aica_period_elapsed(unsigned long timer_var)
> +{

static void..

> +	int transferred;
> +	int play_period;
> +	struct snd_pcm_runtime *runtime;
> +	struct snd_pcm_substream *substream;
> +	struct snd_card_aica *dreamcastcard;
> +	substream = (struct snd_pcm_substream *) timer_var;
> +	runtime = substream->runtime;
> +	dreamcastcard =
> +	    (struct snd_card_aica *) (substream->pcm->private_data);

Useless cast.

> +	} else {
> +		spin_lock(&spu_dmalock);
> +		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);
> +		spin_unlock(&spu_dmalock);
> +	}
> +
spinlock / DMA brain damage.

> +	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;
> +}
> +
Superfluous return.

> +static int snd_aicapcm_pcm_open(struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime;
> +	struct aica_channel *channel;
> +	struct snd_card_aica *dreamcastcard;
> +
> +	if (!enable)
> +		return -ENOENT;

unlikely()?

> +	dreamcastcard =
> +	    (struct snd_card_aica *) (substream->pcm->private_data);

Useless cast.

> +static int snd_aicapcm_pcm_close(struct snd_pcm_substream *substream)
> +{
> +	struct snd_card_aica *dreamcastcard =
> +	    (struct snd_card_aica *) (substream->pcm->private_data);
Likewise.

> +static int snd_aicapcm_pcm_prepare(struct snd_pcm_substream *substream)
> +{
> +
> +	struct snd_card_aica *dreamcastcard =
> +	    (struct snd_card_aica *) (substream->pcm->private_data);

And again.

> +static void startup_aica(struct snd_card_aica *dreamcastcard)
> +{
> +	spu_memload(AICA_CHANNEL0_CONTROL_OFFSET,
> +		    (uint8_t *) dreamcastcard->channel,
> +		    sizeof(struct aica_channel));
> +	aica_chn_start();
> +	return;
> +}
> +
Useless return.

> +
> +static void spu_begin_dma(struct snd_pcm_substream *substream)
> +{
> +	int buffer_size;
> +	snd_pcm_runtime_t *runtime;
> +	struct snd_card_aica *dreamcastcard =
> +	    (struct snd_card_aica *) (substream->pcm->private_data);

Superfluous cast.

> +	runtime = substream->runtime;
> +	buffer_size = frames_to_bytes(runtime, runtime->buffer_size);
> +	if (runtime->channels == 1) {
> +		spin_lock(&spu_dmalock);
> +		dma_xfer(0, runtime->dma_area, AICA_CHANNEL0_OFFSET,
> +			 buffer_size, 5);
> +		spin_unlock(&spu_dmalock);

Broken locking.

> +static int __init snd_aicapcmchip(struct snd_card_aica *dreamcastcard,
> +				  int pcm_index)
> +{
> +	struct snd_pcm *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;
> +	}
> +
Why aren't these setup in the platform device?

> +	/* AICA has no capture ability */
> +	if ((err =
> +	     snd_pcm_new(dreamcastcard->card, "AICA PCM", pcm_index, 1, 0,
> +			 &pcm)) < 0)
> +		return err;

Weird notation, linux kernel style would be:

	err = snc_pcm_new(...);
	if (unlikely(err < 0))
		return err;

please refactor accordingly.

> +static int aica_pcmvolume_put(struct snd_kcontrol *kcontrol,
> +			      struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_card_aica *dreamcastcard;
> +	dreamcastcard = (struct snd_card_aica *) kcontrol->private_value;
> +
> +	if (!dreamcastcard->channel) {
> +		snd_printk("No channel yet\n");
> +		return -ETXTBSY;	/* too soon */
> +	}
> +
> +	else if (dreamcastcard->channel->vol ==
> +		 ucontrol->value.integer.value[0])
> +		return 0;
> +
> +	else {

The if/elses are all over the place, please see
Documentation/CodingStyle.

> +static struct device_driver aica_driver = {
> +	.name = "AICA",
> +	.bus = &platform_bus_type,
> +	.remove = remove_dreamcastcard,
> +};
> +
Deprecated, see struct platform_device.

> +	pd = platform_device_register_simple(dreamcastcard->card->driver,
> +					     -1, NULL, 0);
> +
This is also going away, you'll want to update this for the new platform
device semantics.

> +      freepcm:
> +	snd_aicapcm_free();
> +
> +      freedreamcast:
> +	snd_card_free(dreamcastcard->card);
> +
Why are these goto labels in magical locations?

> +static void __exit aica_exit(void)
> +{
> +	struct device_driver *drv = (&pd->dev)->driver;
> +	device_release_driver(&pd->dev);
> +	driver_unregister(drv);
> +	platform_device_unregister(pd);
> +	/* Kill any sound still playing and reset ARM7 to safe state */
> +	spu_init();
> +
> +
> +	return;
> +}
> +
And another useless return.

Re: [Alsa-devel] Re: [linuxsh-dev] [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Adrian McMenamin]

On Mon, 2006-04-17 at 04:29 +0300, Paul Mundt wrote:
> A few quick comments.. though looking at the earlier thread, most of
> these were already pointed out..
> 

> 
> You can't be serious, you're trying to setup, transfer, and wait for
> completion for a DMA transfer while holding a spinlock? The fact this
> hasn't blown up on you is sheer luck. Use dma_wait_for_completion(), it
> does the right thing.
> 
> Additionaly you need a timeout here if you were for some reason intent on
> doing this while working against the DMA subsystem, if your DMA gets
> stuck this will blow up.
> 

As I wrote last week dma_wait_for_completion won't hack G2 DMA:


147 void dma_wait_for_completion(unsigned int chan)
148 {
149         struct dma_info *info = get_dma_info(chan);
150         struct dma_channel *channel = &info->channels[chan];
151 
152         if (channel->flags & DMA_TEI_CAPABLE) {
153                 wait_event(channel->wait_queue,
154                            (info->ops->get_residue(channel) == 0));
155                 return;
156         }
157 
158         while (info->ops->get_residue(channel))
159                 cpu_relax();
160 }

get_residue never returns 0 for G2 DMA. When the dma is complete get_residue returns the size of the total transfer. Therefore I've no choice but to write my own handler (spinlocks question aside).

Re: [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Paul Mundt]

Removing l-k from CC, since it's not likely anyone cares.

On Mon, Apr 17, 2006 at 10:44:56AM +0100, Adrian McMenamin wrote:
> As I wrote last week dma_wait_for_completion won't hack G2 DMA:
> 
> get_residue never returns 0 for G2 DMA. When the dma is complete
> get_residue returns the size of the total transfer. Therefore I've no
> choice but to write my own handler (spinlocks question aside).

Then the G2 DMA ->get_residue() op needs to be fixed, as it's broken.
Don't work against the subsystem, it knows what it's doing.

You might be better off getting rid of it entirely and seeing about
getting the TEI working, you can check for completion and wake up
accordingly in this case, which is what you ideally want anyways. The
busy-loop in the generic dma_wait_for_completion() is a stupid hack for
people that haven't fixed up their TEI handling yet, it's not intended
for widespread use..


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Paul Mundt]

On Mon, Apr 17, 2006 at 12:52:26PM +0300, Paul Mundt wrote:
> Then the G2 DMA ->get_residue() op needs to be fixed, as it's broken.
> Don't work against the subsystem, it knows what it's doing.
> 
> You might be better off getting rid of it entirely and seeing about
> getting the TEI working, you can check for completion and wake up
> accordingly in this case, which is what you ideally want anyways. The
> busy-loop in the generic dma_wait_for_completion() is a stupid hack for
> people that haven't fixed up their TEI handling yet, it's not intended
> for widespread use..
> 
What about something like this?

diff --git a/arch/sh/drivers/dma/dma-g2.c b/arch/sh/drivers/dma/dma-g2.c
index 5afab6f..277581e 100644
--- a/arch/sh/drivers/dma/dma-g2.c
+++ b/arch/sh/drivers/dma/dma-g2.c
@@ -49,7 +49,14 @@ static volatile struct g2_dma_info *g2_d
 
 static irqreturn_t g2_dma_interrupt(int irq, void *dev_id, struct pt_regs *regs)
 {
-	/* FIXME: Do some meaningful completion work here.. */
+	struct dma_channel *chan = dev_id;
+	unsigned int chan_nr = chan->chan;
+	unsigned int bytes;
+
+	bytes = g2_dma->channel[chan_nr].size - g2_dma->status[chan_nr].size;
+	if (likely(bytes == 0))
+		wake_up(&chan->wait_queue);
+
 	return IRQ_HANDLED;
 }
 
@@ -133,10 +140,17 @@ static int g2_xfer_dma(struct dma_channe
 		 g2_dma->channel[chan_nr].xfer_enable);
 
 	return 0;
+}
+
+static int g2_get_dma_residue(struct dma_channel *chan)
+{
+	unsigned int chan_nr = chan->chan;
+	return g2_dma->channel[chan_nr].size - g2_dma->status[chan_nr].size;
 }
 
 static struct dma_ops g2_dma_ops = {
 	.xfer		= g2_xfer_dma,
+	.get_residue	= g2_get_dma_residue,
 };
 
 static struct dma_info g2_dma_info = {


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: [linuxsh-dev] Re: [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Adrian McMenamin]

On Mon, 2006-04-17 at 13:04 +0300, Paul Mundt wrote:
> On Mon, Apr 17, 2006 at 12:52:26PM +0300, Paul Mundt wrote:
> > Then the G2 DMA ->get_residue() op needs to be fixed, as it's broken.
> > Don't work against the subsystem, it knows what it's doing.
> > 
> > You might be better off getting rid of it entirely and seeing about
> > getting the TEI working, you can check for completion and wake up
> > accordingly in this case, which is what you ideally want anyways. The
> > busy-loop in the generic dma_wait_for_completion() is a stupid hack for
> > people that haven't fixed up their TEI handling yet, it's not intended
> > for widespread use..
> > 
> What about something like this?
> 
> diff --git a/arch/sh/drivers/dma/dma-g2.c b/arch/sh/drivers/dma/dma-g2.c
> index 5afab6f..277581e 100644
> --- a/arch/sh/drivers/dma/dma-g2.c
> +++ b/arch/sh/drivers/dma/dma-g2.c
> @@ -49,7 +49,14 @@ static volatile struct g2_dma_info *g2_d
>  
>  static irqreturn_t g2_dma_interrupt(int irq, void *dev_id, struct pt_regs *regs)
>  {
> -	/* FIXME: Do some meaningful completion work here.. */
> +	struct dma_channel *chan = dev_id;
> +	unsigned int chan_nr = chan->chan;
> +	unsigned int bytes;
> +
> +	bytes = g2_dma->channel[chan_nr].size - g2_dma->status[chan_nr].size;
> +	if (likely(bytes == 0))
> +		wake_up(&chan->wait_queue);
> +
>  	return IRQ_HANDLED;
>  }
>  
> @@ -133,10 +140,17 @@ static int g2_xfer_dma(struct dma_channe
>  		 g2_dma->channel[chan_nr].xfer_enable);
>  
>  	return 0;
> +}
> +
> +static int g2_get_dma_residue(struct dma_channel *chan)
> +{
> +	unsigned int chan_nr = chan->chan;
> +	return g2_dma->channel[chan_nr].size - g2_dma->status[chan_nr].size;
>  }
>  
>  static struct dma_ops g2_dma_ops = {
>  	.xfer		= g2_xfer_dma,
> +	.get_residue	= g2_get_dma_residue,
>  };
>  
>  static struct dma_info g2_dma_info = {
> 
> 

Looks reasonable! I'll give it a go later today



-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Adrian McMenamin]

> 
> > +	/* AICA has no capture ability */
> > +	if ((err =
> > +	     snd_pcm_new(dreamcastcard->card, "AICA PCM", pcm_index, 1, 0,
> > +			 &pcm)) < 0)
> > +		return err;
> 
> Weird notation, linux kernel style would be:
> 
> 	err = snc_pcm_new(...);
> 	if (unlikely(err < 0))
> 		return err;
> 
> please refactor accordingly.
> 

Actually this sort of formulation is common in the kernel as any grep
will show. In fact I copied it directly from the guide to writing ALSA
drivers:

http://www.alsa-project.org/~iwai/writing-an-alsa-driver/x447.htm

But I am happy to change it.



-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: [linuxsh-dev] [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Adrian McMenamin]

> 
> > +	/* AICA has no capture ability */
> > +	if ((err =
> > +	     snd_pcm_new(dreamcastcard->card, "AICA PCM", pcm_index, 1, 0,
> > +			 &pcm)) < 0)
> > +		return err;
> 
> Weird notation, linux kernel style would be:
> 
> 	err = snc_pcm_new(...);
> 	if (unlikely(err < 0))
> 		return err;
> 
> please refactor accordingly.
> 

Actually this sort of formulation is common in the kernel as any grep
will show. In fact I copied it directly from the guide to writing ALSA
drivers:

http://www.alsa-project.org/~iwai/writing-an-alsa-driver/x447.htm

But I am happy to change it.

Re: Re: [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Adrian McMenamin]

On Mon, 2006-04-17 at 13:04 +0300, Paul Mundt wrote:
> On Mon, Apr 17, 2006 at 12:52:26PM +0300, Paul Mundt wrote:
> > Then the G2 DMA ->get_residue() op needs to be fixed, as it's broken.
> > Don't work against the subsystem, it knows what it's doing.
> > 
> > You might be better off getting rid of it entirely and seeing about
> > getting the TEI working, you can check for completion and wake up
> > accordingly in this case, which is what you ideally want anyways. The
> > busy-loop in the generic dma_wait_for_completion() is a stupid hack for
> > people that haven't fixed up their TEI handling yet, it's not intended
> > for widespread use..
> > 
> What about something like this?
> 
> diff --git a/arch/sh/drivers/dma/dma-g2.c b/arch/sh/drivers/dma/dma-g2.c
> index 5afab6f..277581e 100644
> --- a/arch/sh/drivers/dma/dma-g2.c
> +++ b/arch/sh/drivers/dma/dma-g2.c
> @@ -49,7 +49,14 @@ static volatile struct g2_dma_info *g2_d
>  
>  static irqreturn_t g2_dma_interrupt(int irq, void *dev_id, struct pt_regs *regs)
>  {
> -	/* FIXME: Do some meaningful completion work here.. */
> +	struct dma_channel *chan = dev_id;
> +	unsigned int chan_nr = chan->chan;
> +	unsigned int bytes;
> +
> +	bytes = g2_dma->channel[chan_nr].size - g2_dma->status[chan_nr].size;
> +	if (likely(bytes == 0))
> +		wake_up(&chan->wait_queue);
> +
>  	return IRQ_HANDLED;
>  }
>  
> @@ -133,10 +140,17 @@ static int g2_xfer_dma(struct dma_channe
>  		 g2_dma->channel[chan_nr].xfer_enable);
>  
>  	return 0;
> +}
> +
> +static int g2_get_dma_residue(struct dma_channel *chan)
> +{
> +	unsigned int chan_nr = chan->chan;
> +	return g2_dma->channel[chan_nr].size - g2_dma->status[chan_nr].size;
>  }
>  
>  static struct dma_ops g2_dma_ops = {
>  	.xfer		= g2_xfer_dma,
> +	.get_residue	= g2_get_dma_residue,
>  };
>  
>  static struct dma_info g2_dma_info = {
> 
> 

Doesn't appear to be working (though I'll try some adjustments to see if
I can get it to work) ---


/ # cat aine-email.wav > /dev/dsp
Unable to handle kernel NULL pointer dereference at virtual address
00000010
pc = 8c15a466
*pde = 00000000
Oops: 0000 [#1]

Pid : 0, Comm:              swapper
PC is at g2_dma_interrupt+0x6/0x40
PC  : 8c15a466 SP  : 8c255e10 SR  : 400081f0 TEA : c01061c4    Not
tainted
R0  : 8c15a460 R1  : ffffffe3 R2  : 8c220aa8 R3  : 0000001f
R4  : 0000003f R5  : 00000000 R6  : 8c255e50 R7  : 00000005
R8  : 8c220aac R9  : 00000000 R10 : 00000000 R11 : 8c255e50
R12 : 0000003f R13 : 8c275a3c R14 : 8c03ab40
MACH: 0000020e MACL: 147ae314 GBR : 8c000000 PR  : 8c03ab92

Call trace:
[<8c03ab92>] handle_IRQ_event+0x52/0xe0
[<8c03aca0>] __do_IRQ+0x80/0x180
[<8c0081cc>] do_IRQ+0x2c/0x60
[<8c0081a0>] do_IRQ+0x0/0x60
[<8c005098>] ret_from_irq+0x0/0x10
[<8c0f5c4e>] memcpy+0x23a/0x28c
[<8c146a32>] rtl8139_poll+0x132/0x5c0
[<8c168120>] net_rx_action+0x60/0x180
[<8c01b95a>] __do_softirq+0x5a/0xe0
[<8c01ba26>] do_softirq+0x46/0x60
[<8c1e13a0>] schedule+0x0/0x840
[<8c0081a0>] do_IRQ+0x0/0x60
[<8c01bb46>] irq_exit+0x46/0x80
[<8c0081d2>] do_IRQ+0x32/0x60
[<8c005098>] ret_from_irq+0x0/0x10
[<8c0030a0>] default_idle+0x0/0x20
[<8c1e13a0>] schedule+0x0/0x840
[<8c0030aa>] default_idle+0xa/0x20
[<8c0030e4>] cpu_idle+0x24/0x60
[<8c015200>] printk+0x0/0x20
[<8c002028>] _stext+0x28/0x60

Kernel panic - not syncing: Aiee, killing interrupt handler!





-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: [Alsa-devel] Re: [linuxsh-dev] [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Lee Revell]

On Mon, 2006-04-17 at 21:00 +0100, Adrian McMenamin wrote:
> But I am happy to change it.
> 
> 

Please don't - when adding code to a subsystem with different
conventions than mainline the FAQ says to follow the subsystem
conventions.

Lee

Re: [Alsa-devel] Re: [linuxsh-dev] [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Christoph Hellwig]

On Mon, Apr 17, 2006 at 05:47:15PM -0400, Lee Revell wrote:
> On Mon, 2006-04-17 at 21:00 +0100, Adrian McMenamin wrote:
> > But I am happy to change it.
> > 
> > 
> 
> Please don't - when adding code to a subsystem with different
> conventions than mainline the FAQ says to follow the subsystem
> conventions.

Nope.  Alsa needs to gradually converted from something that looks like
cat puke to normal kernel style.  Every new driver that's written properly
helps.

Re: [Alsa-devel] Re: [linuxsh-dev] [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Lee Revell]

On Mon, 2006-04-17 at 23:05 +0100, Christoph Hellwig wrote:
> On Mon, Apr 17, 2006 at 05:47:15PM -0400, Lee Revell wrote:
> > On Mon, 2006-04-17 at 21:00 +0100, Adrian McMenamin wrote:
> > > But I am happy to change it.
> > > 
> > > 
> > 
> > Please don't - when adding code to a subsystem with different
> > conventions than mainline the FAQ says to follow the subsystem
> > conventions.
> 
> Nope.  Alsa needs to gradually converted from something that looks like
> cat puke to normal kernel style.  Every new driver that's written properly
> helps.

OK thanks.  I was certain I saw this in the FAQ at some point but now I
can't find it.

Lee

Re: Re: [linuxsh-dev] [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Takashi Iwai]

At Mon, 17 Apr 2006 23:05:12 +0100,
Christoph Hellwig wrote:
> 
> On Mon, Apr 17, 2006 at 05:47:15PM -0400, Lee Revell wrote:
> > On Mon, 2006-04-17 at 21:00 +0100, Adrian McMenamin wrote:
> > > But I am happy to change it.
> > > 
> > > 
> > 
> > Please don't - when adding code to a subsystem with different
> > conventions than mainline the FAQ says to follow the subsystem
> > conventions.
> 
> Nope.  Alsa needs to gradually converted from something that looks like
> cat puke to normal kernel style.  Every new driver that's written properly
> helps.

Heh, the suggested line is what CondingStyle suggets -- the output of
indent program.  Yeah, but better to fix "bad programming" :)


Takashi


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: [Alsa-devel] Re: [linuxsh-dev] [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Takashi Iwai]

At Mon, 17 Apr 2006 23:05:12 +0100,
Christoph Hellwig wrote:
> 
> On Mon, Apr 17, 2006 at 05:47:15PM -0400, Lee Revell wrote:
> > On Mon, 2006-04-17 at 21:00 +0100, Adrian McMenamin wrote:
> > > But I am happy to change it.
> > > 
> > > 
> > 
> > Please don't - when adding code to a subsystem with different
> > conventions than mainline the FAQ says to follow the subsystem
> > conventions.
> 
> Nope.  Alsa needs to gradually converted from something that looks like
> cat puke to normal kernel style.  Every new driver that's written properly
> helps.

Heh, the suggested line is what CondingStyle suggets -- the output of
indent program.  Yeah, but better to fix "bad programming" :)


Takashi

Re: Re: [linuxsh-dev] [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Christoph Hellwig]

On Tue, Apr 18, 2006 at 12:17:46PM +0200, Takashi Iwai wrote:
> Heh, the suggested line is what CondingStyle suggets -- the output of
> indent program.  Yeah, but better to fix "bad programming" :)

indent only fixes indentation.  Moving the if check to a different line
as the function call changes more than indentation so indent doesn't do it.
Yes, there's more to code codingstyle than just running indent :)



-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: [Alsa-devel] Re: [linuxsh-dev] [PATCH] ALSA driver for Yamaa AICA on Sega Dreamcast

[Christoph Hellwig]

On Tue, Apr 18, 2006 at 12:17:46PM +0200, Takashi Iwai wrote:
> Heh, the suggested line is what CondingStyle suggets -- the output of
> indent program.  Yeah, but better to fix "bad programming" :)

indent only fixes indentation.  Moving the if check to a different line
as the function call changes more than indentation so indent doesn't do it.
Yes, there's more to code codingstyle than just running indent :)