On Tue, Sep 12, 2017 at 04:35:13PM +0200, Arnaud Mouiche wrote:
- freq: Output BCLK frequency = samplerate * 32 (fixed) * channels
- dir: SND_SOC_CLOCK_OUT -> TxBCLK, SND_SOC_CLOCK_IN -> RxBCLK.
- freq: Output BCLK frequency = samplerate * 32 (fixed) * slots (or channels)
Slots are not necessarily 32 bits width. Indeed, a simple grep of snd_soc_dai_set_tdm_slot shows 16, 24, 32 and 0 bits usage. (don't know the real meaning of 0 BTW...) So, it should be good to also remember the slots width given in snd_soc_dai_set_tdm_slot() call.
RM says that the word length is fixed to 32 in I2S Master mode. So I used it here. But I think using it with the slots could be wrong here as you stated. What kind of clock rates (bit and lr) does a TDM case have?
The problem of slot width here is handled in the set_tdm_slot() at all. So I tried to ignored that. But we probably do need it when calculating things with the slot number.
Could you please give me a few set of examples of how you set set_sysclk(), set_tdm_slot() with the current driver? The idea here is to figure out a way to calculate the bclk in hw_params without getting set_sysclk() involved any more.
Anyway, there is something wrong in the snd_soc_codec_set_sysclk usage by fsl_ssi Let's get a look again at the description:
/** * snd_soc_dai_set_sysclk - configure DAI system or master clock. * @dai: DAI * @clk_id: DAI specific clock ID * @freq: new clock frequency in Hz * @dir: new clock direction - input/output. * * Configures the DAI master (MCLK) or system (SYSCLK) clocking. */ int snd_soc_dai_set_sysclk(struct snd_soc_dai *dai, int clk_id, unsigned int freq, int dir)
So, it can be used to configure 2 different clocks (and more) with different usages.
Lukasz complains that simple-card is configuring MCLK incorrectly. but simple-card, only want to configure the SYSCLK, whereas fsl_ssi understand "configure the MCLK..." (fsl_ssi doesn't check the clk_id at all)
It's more than a clock_id in my opinion. The driver now sets the bit clock rate via set_sysclk() other than the MCLK rate actually.
I think the problem is here. I would propose a new clk_id
#define FSL_SSI_SYSCLK_MCLK 1And leave fsl_ssi_set_dai_sysclk(xxx, FSL_SSI_SYSCLK_MCLK, ...) override the computed mlck. This will leave a way for obscure TDM users to specify a specific a random mclk frequency for obscure reasons... Unfortunately, such generic clock_id (sysclk, mclk) were never defined widely.
Unfortunately, it looks like a work around to me. I understand the idea of leaving set_sysclk() out there to override the bit clock is convenient, but it is not a standard ALSA design and may eventually introduce new problems like today.
Is it really needed ? It is true that, up to now, we are using fsl_ssi_set_dai_sysclk() in addition to snd_soc_dai_set_tdm_slot() only because this was the only way to deal with correct mclk setting. And if now, snd_soc_dai_set_tdm_slot() do its job correctly, fsl_ssi_set_dai_sysclk() will become useless (except for obscure TDM users of course)
The idea here is to drop the set_sysclk() for all user cases including TDM cases. So we need a solid patch to calculate the bit clock rate in hw_params() for TDM user cases..