From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750729AbWDXLKX (ORCPT ); Mon, 24 Apr 2006 07:10:23 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1750731AbWDXLKX (ORCPT ); Mon, 24 Apr 2006 07:10:23 -0400 Received: from cantor2.suse.de ([195.135.220.15]:2706 "EHLO mx2.suse.de") by vger.kernel.org with ESMTP id S1750729AbWDXLKW (ORCPT ); Mon, 24 Apr 2006 07:10:22 -0400 Date: Mon, 24 Apr 2006 13:10:11 +0200 Message-ID: From: Takashi Iwai To: Adrian McMenamin Cc: Alsa-devel , linux-sh , LKML , Lee Revell , Paul Mundt Subject: Re: [Alsa-devel] [PATCH] Add Dreamcast AICA driver to alsa-driver In-Reply-To: <1145832383.9242.41.camel@localhost.localdomain> References: <1145831786.9242.38.camel@localhost.localdomain> <1145832383.9242.41.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 Sun, 23 Apr 2006 23:46:22 +0100, Adrian McMenamin wrote: +/* module parameters */ +#define CARD_NAME "AICA" + +static int index; Initialize it to -1 (or SNDRV_DEFAULT_IDX1), and pass it to the first argument of snd_card_new(). Otherwise this option is useless. > +/* Spinlocks */ > +DEFINE_SPINLOCK(spu_memlock); Missing static. > +/* 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; If you use writel(), the length must be aligned to 4. > + spu_write_wait(); > + for (i = 0; i < length; i++) { > + spin_lock(&spu_memlock); > + writel(what, to); > + spin_unlock(&spu_memlock); What's the purpose of this lock? > + to++; > + if (i && !(i % 8)) > + spu_write_wait(); > + } > +} > + > +/* 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; Ditto. > +/* 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); For what is this lock? > +/* 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); The cast looks strange... > + 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, > +}; Put the field ids to the beginning of the line. Found in other structs, too. > +static int stereo_buffer_transfer(struct snd_pcm_substream > + *substream, int buffer_size, int period) > +{ I feel this transfer-and-wait could be done more efficiently using workq than doing it in timer callback. The trigger(start) and aica_period_elapsed() calls queue_work() at each time. The most work of spu_begin_dma() should be put in the workq, too. > + int transferred; > + int dma_countout; > + struct snd_pcm_runtime *runtime; > + int period_offset; > + long dma_flags; > + period_offset = period; > + period_offset %= (AICA_PERIOD_NUMBER / 2); > + runtime = substream->runtime; > + /* transfer left and then right */ > + dma_flags = claim_dma_lock(); > + dma_countout = 0; > + 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); > + dma_countout++; > + if (dma_countout > 0x10000) > + break; /* Approx 1/3 sec timeout in case of hardware failure */ > + } > + 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 */ > + dma_countout = 0; > + do { > + udelay(5); > + transferred = get_dma_residue(0); > + dma_countout++; > + if (dma_countout > 0x10000) > + break; > + } > + while (transferred < buffer_size / 2); > + release_dma_lock(dma_flags); > + return 0; > +} You can write a single function for both mono and two-channel streams. > +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; You don't need to check enable here. The "enable" module option was introduced to enable/disable the speicfic device when multiple same devices are on the system. Hence it makes no sense for the drivers that support only a single device. But we keep this option as a dummy one in most of drivers just for compatibility reason. The sound configurator tends to set this option unconditionally. > +/* TO DO: set up to handle more than one pcm instance */ > +static int __init snd_aicapcmchip(struct snd_card_aica > + *dreamcastcard, int pcm_index) Use __devinit prefix as long as you use platform_driver. > +static int remove_dreamcastcard(struct device *dreamcast_device) > +{ > + struct snd_card_aica *dreamcastcard = > + dreamcast_device->driver_data; > + snd_card_free(dreamcastcard->card); > + kfree(dreamcastcard); > + return 0; > +} > + > +static struct device_driver aica_driver = { > + .name = "AICA",.bus = &platform_bus_type, > + .remove = remove_dreamcastcard, > +}; Any reason not to use struct platform_driver? > +static int load_aica_firmware() > +{ > + int err; > + err = 0; > + spu_init(); > + const struct firmware *fw_entry; The variable definition must be in the beginning of the function block. > +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; The typical flow in init entry should be: - register platform_driver - register platform_device -> initialize the card and releveant instances in probe callback BTW, with the latest HG repo, you can add alsa-driver/Kconfig, so that you can keep alsa-kernel tree intact. Takashi