Re: [PATCH 2/4] ASoC: DaVinci: Voice Codec Interface

[Miguel Aguilar <miguel.aguilar@ridgerun.com>]

Mark Brown wrote:
> On Wed, Sep 23, 2009 at 09:18:42AM -0600, Miguel Aguilar wrote:
>> Mark Brown wrote:
>>> On Tue, Sep 22, 2009 at 01:29:20PM -0600, miguel.aguilar@ridgerun.com wrote:
> 
>>>> 2) Add an option to select internal or external codec in the DM365.
> 
>>> Can you not do both simultaneously?  This should probably be a separate
> 
>> [MA] No external and internal codecs can not coexists since they share the same 
>> DMA channels for TX and RX, so the TI recommendation was choose one codec or the 
>> other one by the configuration menu.
> 
> That'd stop you using both simultaneously but it shouldn't stop you
> having both compiled into the kernel simultaneously.  Would it be
> difficult to make the DMA driver do something like return -EBUSY if it
> was started opened twice?
[MA] But how I can tell the kernel to use one or the other one in runtime?, and 
Why do we need to have both compiled at the same time? if this is really needed 
What is the best way to that?
> 
>> Actually, This patch series have a separate patch for the driver, vcif, SoC 
>> specific code, and EVM specific code.
> 
> You should move the Kconfig stuff for those into the relevant patches.
[MA] ok I see.
> 
>>>> +	/* Restart the codec before setup */
>>>> +	davinci_vcif_stop(substream);
>>>> +	davinci_vcif_start(substream);
> 
>>> This seems a bit odd - I'd expect to see the start at the end of the
>>> function if you need to do this, otherwise the interface will be running
>>> before you've configured it?
> 
>> [MA] The voice codec interface should be restarted before set the hw params. If 
>> you don't do this you will get underrun and overrun errors due to lack of the 
>> restarting process. Thats why I do a stop and then a start. I tried to include 
>> the prepare function however it is called after the hw params function, and that 
>> doesn't make sense.
> 
> What exactly is the start bit doing?  The need to restart doesn't seem
> so surprising, what seemed surprising was that you start the device
> running again before it's been reconfigured - I'd have expected to see a
> stop at the head of the function and then the start at the end after all
> the new parameters had been done.
[MA] Ok I see your point I'll do it in that way.
> 
>>> These look like the interface needs to be configured the same way in
>>> both directions?  If that is the case then set symmetric_rates in the
>>> DAI.
> 
>> [MA]I think it should be symmetric, so the TX and RX should be configured in the 
>> same way.
> 
> In that case you should set symmetric_rates in the DAI - this will mean
> that the core will tell applications about the need to keep symmetric
> rates, meaning that you don't get attempts to configure the one
> direction when the other is active.
[MA] Ok I'll do that.
> 
>>>> +	.driver		= {
>>>> +		.name	= "voice_codec",
>>>> +		.owner	= THIS_MODULE,
>>>> +	},
>>>> +};
> 
>>> Might "vcif" or "davinci-vcif" be a better name?
>> [MA]To use that name I should change the name of the clock, however that clock 
>> is actually for the whole voice codec module, both voice codec and voice codec 
>> interface. The voice codec interface is just a logical separation of the voice 
>> codec module.
> 
> The name of the device should have no influence on the name of the clock
> - the clock API should be able to 
The problem is that clock name is the same as the device, by using pdev->name, 
most of the drivers does it in this way.
> 
> I suspect that you do need a little MFD here, it sounds like all the
> resources need to be allocated to a single device which can then dole
> them out to the CODEC and DAI drivers.
[MA] I haven't use MFD, can you bring me more details.

Do you mean create just one device with the whole voice codec instead of use 
vcif and cq0093 separately?

> 


Thanks,

Miguel Aguilar