>Per: Raffaele Recalcati <[email protected]> >Da: Mark Brown <[email protected]> >Data: 14/07/2010 16.44 >Cc: [email protected], Davide Bonfanti ><[email protected]>, Raffaele Recalcati ><[email protected]>, Chaithrika U S <[email protected]>, Liam >Girdwood <[email protected]>, Troy Kisky <[email protected]>,
>Barry Song <[email protected]>, Peter Ujfalusi <[email protected]>, Eero >Nurkkala <[email protected]>, [email protected] >Oggetto: Re: [PATCH] Asoc Davinci Voicecodec: Added support based on >copy_from_user instead of DMA > >On Wed, Jul 14, 2010 at 04:28:10PM +0200, Raffaele Recalcati wrote: >> From: Davide Bonfanti <[email protected]> > >> Since DM365 has the same DMA event for McBSP and Voicecodec this two >> peripherals cannot be used at the same time. > >Please try to format your patches as documented in SubmittingPatches - >you don't want all this indentation you're introducing in the start of >the changelog. There's also other stuff with regard to coding style and >so on that it'd be useful for you to look at. > Ok >> This driver implements Voicecodec without the use of a DMA but with >> a copy_from_user. > >Why is this specific to the voice CODEC? Shouldn't this be generally >usable, and wouldn't it be better if the DMA driver were able to do some >automatic fallback here so that in cases where DMA can be used it will >be? The DMA can be dynamically allocated and I need to keep it free for I2S because it needs more throughput. I don't think it can be a good idea to copy 44KHz stereo data using a copy_from_user. What's your opinion about? > >I had thought from the discussion on original submission that the two >devices were mutually exclusive for other reasons anyway. The problem is that the event source for DMA is the same and can be configured to work for one peripheral or for the other. I need both peripheral working at the same time (not as an alternative one to the other) > >> +/* Timer register offsets */ >> +#define PID12 0x00 >> +#define TIM12 0x10 >> +#define TIM34 0x14 >> +#define PRD12 0x18 >> +#define PRD34 0x1c >> +#define TCR 0x20 >> +#define TGCR 0x24 > >This timer stuff all looks rather like it should be in whatever other >code manages the timers - as it stands it'll get into a fight with >anything else trying to use them. I'd expect something like this to use >hrtimers to get high resolution time rather than banging the timer >hardware directly. > I'll try to do it >> +int pointer_sub; >> +u16 local_buffer[BUF_SIZE/2]; > >Should BUF_SIZE not be a configuration option, or dynamically configured >at runtime? > You're right >> +/* >> + * ppcm = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) ? >> + * &pcm_hardware_playback : &pcm_hardware_capture; >> + */ > >Remove this if it's not used. > I've implemented only voice output using copy_from_user since I don't need input. This is an anchor to attach also input. Can I put a TODO or shall I remove everything? >> + __raw_writel(0x0, IO_ADDRESS(0x01D0C008)); >> + __raw_writel(0x7400, IO_ADDRESS(0x01D0C004)); > >This could do with being substantially clearer... There's quite a few >other magic numbers in the code which need fixing. You are right. I have to admit I sent the patch in order to understand if there is some interest on it... I will fix > >> +static int davinci_pcm_close(struct snd_pcm_substream *substream) >> +{ >> + struct snd_pcm_runtime *runtime = substream->runtime; >> + struct clk *clk; >> + >> + clk = clk_get(NULL, TIMER); >> + if (!IS_ERR(clk)) >> + clk_disable(clk); > >As with your previous patch you're going to be leaking clocks all over - >you should be balancing your clk_get() with clk_put(). > ops... >> +struct snd_soc_platform davinci_soc_platform_copy = { >> + .name = "davinci-audio-copy", >> + .pcm_ops = &davinci_pcm_ops, >> + .pcm_new = davinci_pcm_new, >> +}; EXPORT_SYMBOL_GPL(davinci_soc_platform_copy); > >Fix your formatting here. > checkpatch didn't help me. >> +++ b/sound/soc/soc-core.c >> @@ -801,7 +801,7 @@ static int soc_pcm_trigger(struct snd_pcm_substream >*substream, int cmd) >> } >> return 0; >> } >> - >> +#if 0 >> /* ASoC PCM operations */ >> static struct snd_pcm_ops soc_pcm_ops = { >> .open = soc_pcm_open, >> @@ -811,7 +811,7 @@ static struct snd_pcm_ops soc_pcm_ops = { >> .prepare = soc_pcm_prepare, >> .trigger = soc_pcm_trigger, >> }; >> - >> +#endif > >Um....? I'm not entirely sure what this and the rest of the changes in >the file are supposed to do but they weren't mentioned at all in the >changelog. If this is needed it should be a separate change with a >proper changelog explaining what's going on. Of course I have to split the commit. here the problem is that soc_pcm_ops was statically instantiated so if one call more than one time soc_new_pcm the operations functions are overwritten. Ce message, ainsi que tous les fichiers joints à ce message, peuvent contenir des informations sensibles et/ ou confidentielles ne devant pas être divulguées. Si vous n'êtes pas le destinataire de ce message (ou que vous recevez ce message par erreur), nous vous remercions de le notifier immédiatement à son expéditeur, et de détruire ce message. Toute copie, divulgation, modification, utilisation ou diffusion, non autorisée, directe ou indirecte, de tout ou partie de ce message, est strictement interdite. This e-mail, and any document attached hereby, may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorized, direct or indirect, copying, disclosure, distribution or other use of the material or parts thereof is strictly forbidden. _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
