Hi Mark,
2009/7/25 Mark Brown broonie@opensource.wolfsonmicro.com:
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
I2S is not a special case of TDM with only left and right two slots for SPORT interface. I2S coordinates with TDM in SPORT, but not a part of TDM. TDM require different hardware configuration with I2S, not only different slot number. One is "Stereo Serial Operation" mode of SPORT, the other one is "Multichannel Operation" mode. They are incompatible at the same time.
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.
Mainly due to hardware, it's really difficult for us to control the memory layout on the fly. That causes much trouble in hardware and driver reliability. Datasheet claims DMA buffer should be fixed while SPORT is enabled, but also claims the layout may be changed while SPORT is disabled. So it's maybe possible for us to change the SPORT api to support the configuration on the fly with a SPORT shutdown, then i2s, tdm even ac97 can all use them to reduce the amount of code duplication. But it need much work on debugging and testing to make it work and reliable. So how about to patch it later as an individual project since the changes will involve sport, i2s, ac97 and tdm?
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.
Here I means the different hardware configuration, and the semantic of pins has some differences too for I2S and TDM.
- 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.
Fixed
+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.
Fixed
+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.
Fixed
+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.
Fixed
Thanks Barry