On Tue, Sep 22, 2009 at 01:29:20PM -0600, miguel.aguilar@xxxxxxxxxxxx wrote:
> From: Miguel Aguilar <miguel.aguilar@xxxxxxxxxxxx>
> 1) Adds an interface needed by the voice codec CQ0093.
It'd be better to mention the specific name of the interface here to
help people find the driver.
> 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
patch anyway, the CPU side support is a different issue from using it on
a specific board - one patch to add the driver, one patch to use it on
the EVM.
I'd also expect that changes in the EVM board file would be required?
> +#define MOD_REG_BIT(val, mask, set) do { \
> + if (set) { \
> + val |= mask; \
> + } else { \
> + val &= ~mask; \
> + } \
> +} while (0)
Is there not a generic version of this somewhere? It seems to be used
in quite a few drivers.
> +static int davinci_vcif_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params,
> + struct snd_soc_dai *dai)
> +{
> + struct davinci_pcm_dma_params *dma_params = dai->dma_data;
> + struct davinci_vcif_dev *dev = dai->private_data;
> + u32 w;
> +
> + /* 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?
> + /* Determine xfer data type */
> + switch (params_format(params)) {
> + case SNDRV_PCM_FORMAT_U8:
> + dma_params->data_type = 0;
> +
> + MOD_REG_BIT(w, DAVINCI_VCIF_CTRL_RD_BITS_8 |
> + DAVINCI_VCIF_CTRL_RD_UNSIGNED |
> + DAVINCI_VCIF_CTRL_WD_BITS_8 |
> + DAVINCI_VCIF_CTRL_WD_UNSIGNED, 1);
> + break;
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.
> + .playback = {
> + .channels_min = 1,
> + .channels_max = 2,
> + .rates = DAVINCI_VCIF_RATES,
> + .formats = SNDRV_PCM_FMTBIT_S16_LE,},
You had options for handling more formats above, shouldn't they be
included here?
> +static struct platform_driver davinci_vcif_driver = {
> + .probe = davinci_vcif_probe,
> + .remove = davinci_vcif_remove,
> + .driver = {
> + .name = "voice_codec",
> + .owner = THIS_MODULE,
> + },
> +};
Might "vcif" or "davinci-vcif" be a better name?
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
