Re: [linuxsh-dev] AICA driver for dreamcast

[Paul Mundt <lethal@linux-sh.org>]

[-- Attachment #1.1: Type: text/plain, Size: 10130 bytes --]

Hi Adrian,

It's getting better, but still a ways to go..

On Sun, May 28, 2006 at 07:40:46PM +0100, Adrian McMenamin wrote:
> static int index = -1;
> static char *id;
> 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.");
> 
I'm not sure I see what the point of id and enable are. id doesn't appear
to be used at all, and enable seems superfluous.

> /* Spinlocks */
> static DEFINE_SPINLOCK(spu_memlock);
> 
> 
Whitespace damage.

> /* Simple platform device */
> static struct platform_device *pd;
> static struct resource aica_memory_space[2] = {
> 	{
> 	 .name = "AICA ARM CONTROL",
> 	 .start = ARM_RESET_REGISTER,
> 	 .flags = IORESOURCE_MEM,
> 	 .end = ARM_RESET_REGISTER + 4,
> 	 },
> 	{

Weird formatting, stick with tabs.

> 	 .name = "AICA Sound RAM",
> 	 .start = SPU_MEMORY_BASE,
> 	 .flags = IORESOURCE_MEM,
> 	 .end = SPU_MEMORY_BASE + 0x200000,
> 	 },
> };
> 
That looks like an off-by-1 bug, you probably want:

	.end = SPU_MEMORY_BASE + 0x200000 - 1,

> /* 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);

You should really make your own spu_writel() that do this for you, so you
don't have the same silly casting all over the place.

> 	int i;
> 	snd_assert(length % 4 == 0, return);
> 	spu_write_wait();
> 	for (i = 0; i < length; i++) {
> 		spin_lock(&spu_memlock);
> 		writel(what, to);
> 		spin_unlock(&spu_memlock);

What exactly are you trying to accomplish with this lock?
spu_write_wait() is already going to synchronize access, isn't it?
If you're trying to do mutual exclusion for the entire write, this
implementation certainly isn't going to work either..

> static void spu_init(void)
> {
> 	spu_disable();
> 	spu_memset(0, 0, 0x200000 / 4);
> 	/* Put ARM7 in endless loop */
> 	ctrl_outl(0xea000002, SPU_MEMORY_BASE);
> 	spu_enable();
> 	schedule();
> }
> 
Why are you schedule()'ing away? You're also calling this before the
firmware loader, any delay you're hoping to accomplish will already be
handled there.

You can probably also inline this..

> /* aica_chn_start - write to spu to start playback */
> inline static void aica_chn_start(void)

static inline void..

> {
> 	spu_write_wait();
> 	spin_lock(&spu_memlock);
> 	writel(AICA_CMD_KICK | AICA_CMD_START,
> 	       (uint32_t *) AICA_CONTROL_POINT);
> 	spin_unlock(&spu_memlock);
> }
> 
More useless locking, and another reason to have your own spu_writel(),
so you don't end up duplicating this all over the place..

> /* 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);
> }
> 
Likewise.

> /* 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),

Magical formatting.

> 	.rates = SNDRV_PCM_RATE_8000_48000,
> 	.rate_min = 8000,.rate_max = 48000,

Likewise.

> 		if (err < 0)
> 			break;

unlikely().

> 	substream = (struct snd_pcm_substream *) timer_var;

Whitespace damage.

> 	runtime = substream->runtime;
> 	dreamcastcard = substream->pcm->private_data;
> 	/* Have we played out an additional period? */
> 	play_period =
> 	    frames_to_bytes(runtime,
> 			    readl
> 			    (AICA_CONTROL_CHANNEL_SAMPLE_NUMBER)) /
> 	    AICA_PERIOD_SIZE;

Likewise.

> 	if (play_period == dreamcastcard->current_period) {
> 		/* reschedule the timer */
> 		dreamcastcard->timer.expires = jiffies + 1;
> 		add_timer(&(dreamcastcard->timer));

Use mod_timer().

> 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.
> }
> 
> 
> 
Whitespace damage.

> static void spu_begin_dma(struct snd_pcm_substream *substream)
> {
> 	int buffer_size;
> 	struct snd_pcm_runtime *runtime;
> 	long dma_flags;

Unused variable?

> 	struct snd_card_aica *dreamcastcard;
> 	dreamcastcard = substream->pcm->private_data;
> 	runtime = substream->runtime;
> 	buffer_size = frames_to_bytes(runtime, runtime->buffer_size);
> 	if (runtime->channels > 1)
> 		dreamcastcard->channel->flags |= 0x01;
> 	aica_dma_transfer(runtime->channels, buffer_size, substream);
> 	dreamcastcard->clicks =
> 	    buffer_size / (AICA_PERIOD_SIZE * runtime->channels);
> 	startup_aica(dreamcastcard);
> 	/* Timer may already be running - if so delete it */
> 	if (dreamcastcard->timer.data)
> 		del_timer(&dreamcastcard->timer);
> 	init_timer(&(dreamcastcard->timer));
> 	dreamcastcard->timer.data = (unsigned long) substream;
> 	dreamcastcard->timer.function = aica_period_elapsed;
> 	dreamcastcard->timer.expires = jiffies + 4;
> 	add_timer(&(dreamcastcard->timer));

That's silly, use mod_timer().

> }
> 
> 
> 
You seem to be taking all of your line breaks and putting them after the
function, rather than throughout it.

> /* TO DO: set up to handle more than one pcm instance */
> static int __init snd_aicapcmchip(struct snd_card_aica
> 				  *dreamcastcard, int pcm_index)
> {
> 	struct snd_pcm *pcm;
> 	int err;
> 	/* AICA has no capture ability */
> 	if ((err =
> 	     snd_pcm_new(dreamcastcard->card, "AICA PCM", pcm_index, 1, 0,
> 			 &pcm)) < 0)
> 		return err;

Magical line formatting again.. kernel convention is also

	err = snd_pcm_new(...);
	if (unlikely(err < 0))
		return err;

> 	/* 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;
> }
> 
Why not just return snd_pcm_lib_preallocate_pages_for_all(...); ?

> static int aica_pcmvolume_get(struct snd_kcontrol *kcontrol,
> 			      struct snd_ctl_elem_value *ucontrol)
> {
> 	struct snd_card_aica *dreamcastcard;
> 	dreamcastcard = kcontrol->private_data;
> 	if (!dreamcastcard->channel)
> 		return -ETXTBSY;	/* we've not yet been set up */

unlikely()..

> 			      struct snd_ctl_elem_value *ucontrol)
> {
> 	struct snd_card_aica *dreamcastcard;
> 	dreamcastcard = kcontrol->private_data;
> 	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 {

Please do something about this if/else readability..

> static struct snd_kcontrol_new 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
> };

More magical formatting.

> static struct device_driver aica_driver = {
> 	.name = "AICA",.bus = &platform_bus_type,
> 	.remove = remove_dreamcastcard,
> };
> 
And again.

> /* 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);
> }
> 
Er, no. Go with a platform_device directly, there's no need for this kind
of blatant abuse of the driver model.

> static int load_aica_firmware()
                                ^void
> {
> 	int err;
> 	err = 0;
> 	spu_init();
> 	const struct firmware *fw_entry;

Declarations _before_ code please..

> 	err = request_firmware(&fw_entry, "aica_firmware.bin", &pd->dev);
> 	if (err)
> 		return err;

unlikely()? scheduling away before this seems pointless.

> static int __devinit add_aicamixer_controls(struct snd_card_aica
> 					    *dreamcastcard)
> {
> 	int err;
> 	err = snd_ctl_add
> 	    (dreamcastcard->card,
> 	     snd_ctl_new1(&snd_aica_pcmvolume_control, dreamcastcard));
> 	if (err < 0) {
> 		snd_printk
> 		    ("AICA sound: Could not add PCM volume control\n");
> 		return err;
> 	}
> 	err = snd_ctl_add
> 	    (dreamcastcard->card,
> 	     snd_ctl_new1(&snd_aica_pcmswitch_control, dreamcastcard));
> 	if (err < 0) {
> 		snd_printk
> 		    ("AICA sound: Could not add PCM switch control\n");
> 		return err;
> 	}
> 	return 0;
> }
> 
Whitespace damage..

> 	pd = platform_device_register_simple(dreamcastcard->card->driver,
> 					     -1, aica_memory_space, 2);

As has already been pointed out, this is going away, new drivers should
not be using this.

> 	    ("ALSA Driver for Yamaha AICA Super Intelligent Sound Processor\n");
> 	return 0;
>       freepcm:
>       freedreamcast:

Why are there two goto labels for the same thing, and why are they in a
magical location?

> 	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;
> 	}

More driver model abuse..

> 	kfree(dreamcastcard);
> 	return err;
> }
> 
> 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;

Superfluous return.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/alsa-devel