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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
