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:
pgp2JW97uvdJa.pgp
Description: PGP signature
_______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.sourceforge.net/lists/listinfo/alsa-devel
