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.
-
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/