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

[Barry Song]

Hi Mark,

2009/7/25 Mark Brown <broonie at 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