Naresh,
Thanks for taking the time to look at this.
Medisetty, Naresh wrote:
>
> Instead of breaking the default transfer format I2S it may be better to
> #define for the user to define the default configuration as I2S or DSP
>
>> /* set codec DAI configuration */
>> - ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
>> + ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_DSP_A |
>> SND_SOC_DAIFMT_CBM_CFM);
>> if (ret < 0)
>> return ret;
Please, correct me if I'm wrong, but the dm644x does not support I2S format.
The code calls it I2S but it is really dsp. Hence, I corrected the nomenclature
to avoid confusion.
>> + edmacc_paramentry_regs p;
>> + if (lch < 0)
>> + return;
>> + davinci_get_dma_params(lch, &p);
>> + printk(KERN_DEBUG "%s: 0x%x, opt=%x, src=%x, a_b_cnt=%x dst=%x\n",
>> + name, lch, p.opt, p.src, p.a_b_cnt, p.dst);
>> + printk(KERN_DEBUG " src_dst_bidx=%x link_bcntrld=%x
>> src_dst_cidx=%x ccnt=%x\n",
>> + p.src_dst_bidx, p.link_bcntrld, p.src_dst_cidx,
>> p.ccnt);
>> }
>> +#endif
>>
>
> Code cluttered with preprocessor directives is difficult to read and maintain
> Instead we can define print_buf_info to nothing if DAVINCI_PCM_DEBUG is not
> set.
> If irq latency is concerned we can use inline functions
> Anyhow we only set DAVINCI_PCM_DEBUG while debugging
> #if DAVINCI_PCM_DEBUG
> print_buf_info(....) {
> ---------
> ---------
> }
> #else
> print_buf_info(.....) { }
> #endif
Yes, it would be better to print this only when debug is enabled.
>> +
>
> Similar to davinci_link_lch there is a function with name davinci_chain_lch
> exported by DMA driver for chaining two dma channels
>
> Making use of this function will hide the EDMA internals in audio driver and
> improve the readability of the code
>
>> + davinci_get_dma_params(lch, &parm);
>> + parm.opt &= ~(TCCMODE | TCC | TCINTEN);
>> + parm.opt |= TCCHEN | ((prtd->ram_master_lch & 0x3f)<<12);
>> + davinci_set_dma_params(lch, &parm);
You're right, davinci_chain_lch is better. Thanks.
It would be easier to read your comments, if you put them after the relevant
code.
Also, removing none relevant code would help keep me from missing your comments
all together.
>> + struct snd_dma_buffer *iram_dma = NULL;
>> + unsigned iram_phys = 0;
>> + unsigned int iram_size = davinci_pcm_hardware.period_bytes_max;
>
> The size of the IRAM usage is 7*512*2 (for both capture and playback) is much
> expensive because IRAM is limited in size (16k) and used by time critical
> requirements from different modules
>
> Personally I had an experience where only 256 bytes (for both capture and
> playback) is allotted for audio buffers in IRAM
>
> And also the audio buffer size in IRAM can be defined as a MACRO, which can
> be changed later if there is a requirement.
Changing the line
.period_bytes_max = 7 * 512, /* This is size of ping + pong buffer*/
is just as easy as changing a macro. They are both one line changes. I have not
played
with trying to find a minimum buffer size, but I'm sure you're right that 7*
512 bytes is overkill.
Once these changes are solid, that's a change that should be made.
>
> I tested the patch on my dm644x evm with aplay utility by executing the
> following command.
>
> aplay -f dat <filename>
>
> But it failed to play the audio and the following error messages were printed.
>
> asoc: machine hw_params failed
> aplay: set_params:961: Unable to install hw params:
>
> Regards
> Naresh
Looks like
case (SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_IB_NF):
in tlv320aic3x.c also needs changed to
case (ND_SOC_DAIFMT_DSP_A | SND_SOC_DAIFMT_IB_NF):
because DSP_B would seem to need
aic3x_write(codec, AIC3X_ASD_INTF_CTRLC, 1);
which the driver never sets, so it retains its default of 0 (dsp_a mode).
Could you try this and let me know?
Thanks again
Troy
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
