On 05/25/2015 02:37 PM, Mark Brown wrote:
On Mon, May 18, 2015 at 02:12:50PM +0200, Arnaud Pouliquen wrote:
Add code to manage Uniperipheral player IP instances. These DAIs are dedicated to playback and support I2S and IEC mode.
There's some small things below but the main thing is that I'm still unclear about the driver internal abstraction layers. Perhaps that will become obvious later in the series...
Abstraction layers are used to have same interface for uniperiph player and uniperiph reader DAIs. I think there are two points to discuss before i rework it:
1) Are you still ok that i keep 2 separate files for player and reader IP management? Just for remind here is discussion started on V1 patch set:
Arnaud: I splitted reader and player code,because it is 2 different IPs with some specific features and behavior ( clock, master/slave mode, IEC, standby ...). From my point of view is is more clear like this, but It is feasible to merge both code adding conditions on direction in most functions. Please tell me what you prefer. I case of merge i suppose that the best is to not define uniperif_ops struct but externalize functions...
Marc: That's reasonable, we just shouldn't be seeing large chunks of obvious code duplication.
First I just see that i proposed to remove uniperif_ops, I forgot...my apologize Then to complete discussion, I'm aware that code seems similar regarding patches 3 and 4 with some duplications. but patches 8 & 9 add controls on uniperiph player, that make add differences. I also plan to re-implement the standby mode available only on uniperif player IP (after driver acceptation). This specific mode will allows to to keep player on to send silence on bus when PCM stream is stopped.
2) If both files are kept what could be done is to: - suppress sti_platform_dai structure and call directly functions - use directly snd_soc_dai_ops in to replace uniperif_ops and keep common functions in sti_platform.c This could be ok for you?
+static void uni_player_set_channel_status(struct uniperif *player,
struct snd_pcm_runtime *runtime)+{
- int n;
- unsigned int status;
- /*
* Some AVRs and TVs require the channel status to contain a correct* sampling frequency. If no sample rate is already specified, then* set one.*/Please take a look at Russell King's patch "sound/core: add IEC958 channel status helper" (currently in review though I'd expect it to hit -next the next time it's built) - it does this (or most of it anyway) in a factored out function that can be shared between drivers.
I will check Russell King's code, and try to use helper functions.
- switch (player->daifmt & SND_SOC_DAIFMT_INV_MASK) {
- case SND_SOC_DAIFMT_IB_IF:
- case SND_SOC_DAIFMT_NB_IF:
SET_UNIPERIF_I2S_FMT_LR_POL_HIG(player);break;I didn't spot the code where the inverted and normal bit clock polarities are handled?
yes missing, i will rework this... Just a question what do you consider as normal clock? data output on rising edge?