Re: [PATCH] ASoC: fsl_sai: Clean code for synchronize mode
From: Nicolin Chen <nicoleotsuka@gmail.com>
Date: 2020-08-04 07:04:13
Also in:
alsa-devel, lkml
On Tue, Aug 04, 2020 at 12:22:53PM +0800, Shengjiu Wang wrote:
quoted
quoted
Btw, do we need similar change for TRIGGER_STOP?This is a good question. It is better to do change for STOP, but I am afraid that there is no much test to guarantee the result.
Maybe we can do this change for STOP.
The idea looks good to me...(check inline comments)
quoted hunk ↗ jump to hunk
diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c index 1c0e06bb3783..6e4be398eaee 100644 --- a/sound/soc/fsl/fsl_sai.c +++ b/sound/soc/fsl/fsl_sai.c@@ -517,6 +517,37 @@ static int fsl_sai_hw_free(structsnd_pcm_substream *substream, return 0; } +static void fsl_sai_config_disable(struct fsl_sai *sai, bool tx) +{ + unsigned int ofs = sai->soc_data->reg_offset; + u32 xcsr, count = 100; + + regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs), + FSL_SAI_CSR_TERE, 0); + + /* TERE will remain set till the end of current frame */ + do { + udelay(10); + regmap_read(sai->regmap, FSL_SAI_xCSR(tx, ofs), &xcsr); + } while (--count && xcsr & FSL_SAI_CSR_TERE); + + regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs), + FSL_SAI_CSR_FR, FSL_SAI_CSR_FR); + + /* + * For sai master mode, after several open/close sai, + * there will be no frame clock, and can't recover + * anymore. Add software reset to fix this issue. + * This is a hardware bug, and will be fix in the + * next sai version. + */ + if (!sai->is_slave_mode) { + /* Software Reset for both Tx and Rx */
Remove "for both Tx and Rx"
/* Check if the opposite FRDE is also disabled */
regmap_read(sai->regmap, FSL_SAI_xCSR(!tx, ofs), &xcsr);
+ if (sai->synchronous[tx] && !sai->synchronous[!tx] && !(xcsr & FSL_SAI_CSR_FRDE))
+ fsl_sai_config_disable(sai, !tx);+ if (sai->synchronous[tx] || !sai->synchronous[!tx] || !(xcsr & FSL_SAI_CSR_FRDE)) + fsl_sai_config_disable(sai, tx);
The first "||" should probably be "&&".
The trigger() logic is way more complicated than a simple cleanup
in my opinion. Would suggest to split RMR part out of this change.
And for conditions like "sync[tx] && !sync[!tx]", it'd be better
to have a helper function to improve readability:
/**
* fsl_sai_dir_is_synced - Check if stream is synced by the opposite stream
*
* SAI supports synchronous mode using bit/frame clocks of either Transmitter's
* or Receiver's for both streams. This function is used to check if clocks of
* current stream's are synced by the opposite stream.
*
* @sai: SAI context
* @dir: direction of current stream
*/
static inline bool fsl_sai_dir_is_synced(struct fsl_sai *sai, int dir)
{
int adir = (dir == TX) ? RX : TX;
/* current dir in async mode while opposite dir in sync mode */
return !sai->synchronous[dir] && sai->synchronous[adir];
}
Then add more comments in trigger:
static ...trigger()
{
bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
int adir = tx ? RX : TX;
int dir = tx ? TX : RX;
// ....
{
// ...
/* Check if the opposite FRDE is also disabled */
regmap_read(sai->regmap, FSL_SAI_xCSR(!tx, ofs), &xcsr);
/*
* If opposite stream provides clocks for synchronous mode and
* it is inactive, disable it before disabling the current one
*/
if (fsl_sai_dir_is_synced(adir) && !(xcsr & FSL_SAI_CSR_FRDE))
fsl_sai_config_disable(sai, adir);
/*
* Disable current stream if either of:
* 1. current stream doesn't provide clocks for synchronous mode
* 2. current stream provides clocks for synchronous mode but no
* more stream is active.
*/
if (!fsl_sai_dir_is_synced(dir) || !(xcsr & FSL_SAI_CSR_FRDE))
fsl_sai_config_disable(sai, dir);
// ...
}
// ....
}
Note, in fsl_sai_config_disable(sai, dir):
bool tx = dir == TX;
Above all, I am just drafting, so please test it thoroughly :)