At Sun, 7 Jan 2007 22:16:14 +0100,
Lennert Buytenhek wrote:
>
> (Please CC on replies, not subscribed.)
>
> I sent this to linux-arm-kernel@ some time ago, but didn't get any
> complaints, so I guess it's time I send it to the alsa list. This
> driver depends on the cirrus m2p dma driver that I also sent to the
> linux-arm-kernel@ list some time ago.
Thanks for the patch. Some notes through a quick review are below.
> --- /dev/null
> +++ linux-2.6.19/sound/arm/ep93xx-ac97.c
(snip)
> +static void ep93xx_ac97_bus_reset(struct snd_ac97 *ac97)
> +{
> + int i;
> +
> + printk(KERN_INFO "ep93xx-ac97: resetting bus.. ");
If these are nothing but debug messages, use snd_printd(), for
example.
> +/* AC'97 platform driver ****************************************************/
> +static int ep93xx_ac97_probe(struct platform_device *dev)
> +{
> + struct ep93xx_ac97_audio_ops *ops = dev->dev.platform_data;
> + struct snd_card *card;
> + struct snd_ac97_bus *ac97_bus;
> + struct snd_ac97_template ac97_template;
> + int ret;
> +
> + // @@@ register resources
Avoid C++ style comments (ditto for some other places).
> +
> +err:
> + /* @@@ This causes hangs..
> + * if (card != NULL)
> + * snd_card_free(card);
> + */
Any clue how does it hang?
> +#ifdef CONFIG_PM
> +static int ep93xx_ac97_suspend(struct platform_device *dev, pm_message_t state)
> +{
> + return 0;
Hm, don't you need any suspend action for PCM and AC97 registers?
> --- /dev/null
> +++ linux-2.6.19/sound/arm/ep93xx-pcm.c
> +static int ep93xx_pcm_open(struct snd_pcm_substream *substream)
> +{
> + struct ep93xx_pcm_client *client = substream->private_data;
> + struct snd_pcm_runtime *runtime = substream->runtime;
> + struct ep93xx_runtime_data *rtd;
> + int ret;
> +
> + runtime->hw = ep93xx_pcm_hardware;
> +
> + rtd = kmalloc(sizeof(*rtd), GFP_KERNEL);
> + if (rtd == NULL) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + rtd->params = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) ?
> + client->playback_params : client->capture_params;
> +
> + memset(&rtd->period_tasklet, 0, sizeof(rtd->period_tasklet));
> + rtd->period_tasklet.func = do_period_elapsed;
> + rtd->period_tasklet.data = (unsigned long)substream;
Use tasklet_init().
> +static u64 ep93xx_pcm_dmamask = 0xffffffff;
Use DMA_32BIT_MASK.
> +
> +int ep93xx_pcm_new(struct snd_card *card, struct ep93xx_pcm_client *client,
> + struct snd_pcm **rpcm)
> +{
> + struct snd_pcm *pcm;
> + int play = client->playback_params ? 1 : 0;
> + int capt = client->capture_params ? 1 : 0;
> + int ret;
> +
> + ret = snd_pcm_new(card, "EP93xx-PCM", 0, play, capt, &pcm);
> + if (ret)
> + goto out;
> +
> + pcm->private_data = client;
> + pcm->private_free = ep93xx_pcm_free_dma_buffers;
> +
> + if (!card->dev->dma_mask)
> + card->dev->dma_mask = &ep93xx_pcm_dmamask;
> + if (!card->dev->coherent_dma_mask)
> + card->dev->coherent_dma_mask = 0xffffffff;
Doesn't dma_set_mask() work?
> +EXPORT_SYMBOL(ep93xx_pcm_new);
> +
> +MODULE_AUTHOR("Lennert Buytenhek <buytenh@xxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("EP93xx ALSA PCM interface");
> +MODULE_LICENSE("GPL");
Could ep93xx-pcm be merged to ep93xx-ac97 as a single module?
Since you have only one interface, it doesn't make much sense to split
to two modules. With a single module, you can avoid unnecessary
exported symbols.
Takashi
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/alsa-devel
