From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751751AbWDCPwV (ORCPT ); Mon, 3 Apr 2006 11:52:21 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751752AbWDCPwV (ORCPT ); Mon, 3 Apr 2006 11:52:21 -0400 Received: from cantor.suse.de ([195.135.220.2]:62367 "EHLO mx1.suse.de") by vger.kernel.org with ESMTP id S1751749AbWDCPwU (ORCPT ); Mon, 3 Apr 2006 11:52:20 -0400 Date: Mon, 03 Apr 2006 17:52:17 +0200 Message-ID: From: Takashi Iwai To: Adrian McMenamin Cc: Alsa-devel , linux-sh , Paul Mundt , LKML Subject: Re: Patch for AICA sound support on SEGA Dreamcast In-Reply-To: <1144075522.11511.20.camel@localhost.localdomain> References: <1144075522.11511.20.camel@localhost.localdomain> User-Agent: Wanderlust/2.12.0 (Your Wildest Dreams) SEMI/1.14.6 (Maruoka) FLIM/1.14.7 (=?ISO-8859-4?Q?Sanj=F2?=) APEL/10.6 MULE XEmacs/21.5 (beta25) (eggplant) (+CVS-20060326) (i386-suse-linux) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org 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