[alsa-devel] [PATCH 1/2] blackfin I2S(TDM mode) CPU DAI driver

[Mark Brown]

On Thu, Jul 23, 2009 at 02:10:33AM +0800, Barry Song wrote:

> On the one hand, I2S and TDM means two completely different interface
> configuration to blackfin, results in completely different hardware
> configuration, DMA description and data transfer flow. Merging them as a
> whole will be very ugly and difficult to maintain.

I'm still a bit confused about why this is required - I'm not sure where
the requirement for the changes in the DMA layout come from. 

Looking at the code it appears that the main difference is that your TDM
mode always configures the DMA layout for 8 channel blocks.  This
appears to be because the SPORT code requires that you specify the block
size statically when you call sport_init().  Looking at the code it
seems like the SPORT hardware is capable of changing the memory layout
on the fly (at least, all this stuff appears to be written down by
software at various points during setup so I'd expect it to be changable
at runtime) and that the restrictions here are due to the SPORT API
rather than the hardware itself?

If that is the case then would it not be better to refactor the SPORT
API so that it is capable of changing the configuration on the fly and
then update the existing I2S driver to be able to take advantage of
that?  This would reduce the amount of code duplication here and I'd
expect it to also make things run more efficiently when less than 8
channels are in use in TDM mode.

> On the other hand, hardware board connection decides the I2S/TDM mode
> directly. It's needless to switch between I2S and TDM dynamically. For
> TDM mode, board driver can refer to the TDM instance, for I2S mode,

Are you saying that a physically different SPORT or other hardware
configuration on the CPU is required to use TDM mode?  The impression
given by the drivers has always been that this is some kind of generic
synchronous serial port and the way the SPORT number is set up seems to
support that.  Could you go into more detail here, please - this would
be a very unusual implementation decision for a CPU.

> +	if (sport_handle != NULL)
> +		runtime->private_data = sport_handle;
> +	else {
> +		pr_err("sport_handle is NULL\n");
> +		return -1;
> +	}

Should return a real error code here.

> +int bf5xx_pcm_tdm_new(struct snd_card *card, struct snd_soc_dai *dai,
> +	struct snd_pcm *pcm)
> +{

This should be static; any functions that are exported via operations
structures should be static.

> +static int bf5xx_tdm_hw_params(struct snd_pcm_substream *substream,
> +	struct snd_pcm_hw_params *params,
> +	struct snd_soc_dai *dai)
> +{
> +	int ret = 0;
> +
> +	bf5xx_tdm.tcr2 &= ~0x1f;
> +	bf5xx_tdm.rcr2 &= ~0x1f;
> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_S32_LE:
> +		bf5xx_tdm.tcr2 |= 31;
> +		bf5xx_tdm.rcr2 |= 31;
> +		sport_handle->wdsize = 4;
> +		break;
> +		/* at present, we only support 32bit transfer */
> +	default:
> +		pr_err("not supported PCM format yet\n");
> +		break;

This should return an error.

> +static int __devinit bfin_tdm_probe(struct platform_device *pdev)
> +{
> +	int ret = 0;
> +
> +	if (peripheral_request_list(&sport_req[sport_num][0], "soc-audio")) {
> +		pr_err("Requesting Peripherals failed\n");
> +		return -EFAULT;
> +	}

> +static struct platform_driver bfin_tdm_driver = {
> +	.probe  = bfin_tdm_probe,
> +	.remove = __devexit_p(bfin_tdm_remove),
> +	.driver = {
> +		.name   = "bfin-sport",
> +		.owner  = THIS_MODULE,
> +	},
> +};

This isn't a good name for the driver - all of the Blackfin CPU audio
drivers and probably some others use a SPORT so the same driver name
would apply for each of them.  If more than one of these drivers chose
the same name then you'd not be able to have both drivers in the kernel
at once.  The driver name should be something that's specific to the
device you're trying to register, such as bfin-tdm.