Re: Patch for AICA sound support on SEGA Dreamcast
From: Takashi Iwai
Date: Mon Apr 03 2006 - 11:52:37 EST
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@xxxxxxxxxxxxxxxxx)
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/