[alsa-devel] [PATCH v3 5/6] ARM: DaVinci: ASoC: Add mcasp support for DM646x

[Mark Brown]

On Thu, May 28, 2009 at 05:11:08AM -0400, Chaithrika U S wrote:

> The codec on the DM646x EVM is AIC32 which is connected to Serializer0 and
> Serializer1 of McASP0. The McASP0 consists of transmit and receive sections
> that may operate synchronized, or completely independently with separate master
> clocks, bit clocks, and frame syncs, and using different transmit modes with
> different bit-stream formats.

The particular configuration used for the EVM should not be relevant for
the McASP driver.

> +/* Left (even TDM Slot) Channel Status Register File*/
> +#define DAVINCI_MCASP_DITCSRA_REG	0x100
> +/* Right (odd TDM slot) Channel Status RegisterFile*/
> +#define DAVINCI_MCASP_DITCSRB_REG	0x118

Interesting spacing in the comments...

> +	case DAVINCI_AUDIO_WORD_32:
> +		fmt = 0x0F;
> +		break;
> +
> +	default:
> +		return -1;
> +	}

Should be a real error code.

> +static void davinci_hw_param(struct snd_pcm_substream *substream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct davinci_audio_dev *dev = rtd->dai->cpu_dai->private_data;

It might be easier with a lot of these functions to just pass the
davinci_audio_dev as a parameter.

> +       if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +               /* bit stream is MSB first  with no delay */
> +               /* DSP_B mode */

Your driver consistently refers to itself as an I2S driver but it
appears that in actual fact it only implements DSP mode B.  This might
be a bit confusing for users.

> +	/* Set the TX format : 24 bit right rotation, 32 bit slot, Pad 0
> +	   and LSB first */
> +	mcasp_set_bits(dev->base + DAVINCI_MCASP_TXFMT_REG,
> +						TXROT(6) | TXSSZ(15));

I suspect looking at some of the indentation that you're writing this
code with 4 character tabs.

> +static int davinci_i2s_mcasp_probe(struct platform_device *pdev)
> +{
> +	struct evm_snd_platform_data *parray = pdev->dev.platform_data;
> +	struct davinci_pcm_dma_params *dma_data;
> +	struct resource *mem, *ioarea, *res;
> +	struct evm_snd_platform_data *pdata;

Why is the McASP driver using platform data called 'evm_snd_patform_data'?
This suggests that there's some abstraction problem with the separation
between the machine and McASP drivers.

Are the two DAIs directly tied to each other in hardware?  If not it'd
probably be better to have them registered as separate devices and probe
separately so that if another chip comes along with a different set of
DAIs it can be accommodated more readily - if the register interfaces
stay consistent it may simply be a case of registering the new device.

> +	ret = snd_soc_register_dais(davinci_iis_mcasp_dai,
> +				ARRAY_SIZE(davinci_iis_mcasp_dai));
> +	if (ret != 0)
> +		goto err_release_region;

You should initialise dev within the DAI to be the struct device for the
platform driver you were probed with.  It might also be nice to tie this
in to num_links somehow.