[alsa-devel] [PATCH v2] ASoC: rt5670: add set_bclk_ratio in dai ops

[Pierre-Louis Bossart]

On 9/13/17 10:14 PM, Bard Liao wrote:
>> -----Original Message-----
>> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart at linux.intel.com]
>> Sent: Thursday, September 14, 2017 1:45 AM
>> To: Bard Liao; broonie at kernel.org; lgirdwood at gmail.com
>> Cc: Oder Chiou; Jack Yu; alsa-devel at alsa-project.org; lars at metafoo.de;
>> tiwai at suse.de; Shuming [范書銘]; Flove(HsinFu)
>> Subject: Re: [alsa-devel] [PATCH v2] ASoC: rt5670: add set_bclk_ratio in dai ops
>>
>> On 9/12/17 11:35 PM, Bard Liao wrote:
>>>> -----Original Message-----
>>>> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart at linux.intel.com]
>>>> Sent: Tuesday, September 12, 2017 10:54 PM
>>>> To: Bard Liao; broonie at kernel.org; lgirdwood at gmail.com
>>>> Cc: Oder Chiou; Jack Yu; alsa-devel at alsa-project.org; lars at metafoo.de;
>>>> tiwai at suse.de; Shuming [范書銘]; Flove(HsinFu)
>>>> Subject: Re: [alsa-devel] [PATCH v2] ASoC: rt5670: add set_bclk_ratio in dai
>> ops
>>>>
>>>> On 9/12/17 3:12 AM, Bard Liao wrote:
>>>>> We need to set a specific bit for 50 bclk rate. So add set_bclk_ratio
>>>>> function to set the bit.
>>>>
>>>> When is this supposed to be used? the cht_bsw_rt5672 machine driver uses
>>>> a 19.2MHz MCLK/2.4 MHz bclk, so there's a typical 50x ratio.
>>>
>>> I think it should be used for all Intel platform. The bit will only be
>>> effective in PCM mode with TDM enabled. It will influence the data
>>> allocation format.
>>> 0: the dummy bits will be allocate at the end of each slot.
>>> 1: the dummy bits will be allocate at the end of all slots.
>>> For example, 50fs with 24 bits data length.
>>> 0: 24 bits slot0 data + 1 dummy bit + 24 bits slot1 data + 1 dummy bit
>>> 1: 24 bits slot0 data + 24 bits slot1 data + 2 dummy bits
>>>
>>> I will do slight modification on the patch since the bit should be set for
>>> all bclk rate which is divisible by 50.
>>
>> I am not sure I understand the 0: and 1: cases, and since we get audio
>> today with this codec and a ratio of 200 (4 24-bit slots in DSP_B) I
>> don't get what happens with this fix.
> 
>  From my experience, most Intel platforms use case 1 format. The
> issue will be incorrect recording volume due to bit allocation is
> shifted.
> If Intel expected case 1 format but Realtek send case 0 format.
> It would be no problem with slot 0. But for slot 1, it will be:
> Intel expected: 24 bits valid data for slot 1
> Realtek sent:  1 dummy bit for slot 0 + 24 bits data for slot 1
> So what Intel actually received will be 1 dummy bit + 23 bits data
> And it is similar to other slots.
> BTW, if you don't meet the issue, you don't need to call
> snd_soc_dai_set_bclk_ratio on your machine driver.

Yes, the Intel SSP will push out all slots and dummy bits at the end of 
the frame. But if this was never enabled we should have seen noise on 
the right channel on playback - this sort of misalignment is pretty 
obvious to spot. I also see a mention of limitation to 50fs resulting in 
ASRC being required.

http://elixir.free-electrons.com/linux/latest/source/sound/soc/intel/boards/cht_bsw_rt5672.c#L211

> 
>>
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Bard Liao <bardliao at realtek.com>
>>>>> ---
>>>>> v2:
>>>>> * add define for tdm data format select bit.
>>>>> ---
>>>>>     sound/soc/codecs/rt5670.c | 19 +++++++++++++++++++
>>>>>     sound/soc/codecs/rt5670.h |  4 ++++
>>>>>     2 files changed, 23 insertions(+)
>>>>>
>>>>> diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c
>>>>> index 9545764..da7614c 100644
>>>>> --- a/sound/soc/codecs/rt5670.c
>>>>> +++ b/sound/soc/codecs/rt5670.c
>>>>> @@ -2582,6 +2582,24 @@ static int rt5670_set_tdm_slot(struct
>> snd_soc_dai
>>>> *dai, unsigned int tx_mask,
>>>>>     	return 0;
>>>>>     }
>>>>>
>>>>> +static int rt5670_set_bclk_ratio(struct snd_soc_dai *dai, unsigned int
>> ratio)
>>>>> +{
>>>>> +	struct snd_soc_codec *codec = dai->codec;
>>>>> +
>>>>> +	dev_dbg(codec->dev, "%s ratio=%d\n", __func__, ratio);
>>>>> +	if (dai->id != RT5670_AIF1)
>>>>> +		return 0;
>>>>> +
>>>>> +	if (ratio == 50)
>>>>> +		snd_soc_update_bits(codec, RT5670_GEN_CTRL3,
>>>>> +			RT5670_TDM_DATA_MODE_SEL,
>>>> RT5670_TDM_DATA_MODE_50FS);
>>>>> +	else
>>>>> +		snd_soc_update_bits(codec, RT5670_GEN_CTRL3,
>>>>> +			RT5670_TDM_DATA_MODE_SEL,
>>>> RT5670_TDM_DATA_MODE_NOR);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>>     static int rt5670_set_bias_level(struct snd_soc_codec *codec,
>>>>>     			enum snd_soc_bias_level level)
>>>>>     {
>>>>> @@ -2712,6 +2730,7 @@ static const struct snd_soc_dai_ops
>>>> rt5670_aif_dai_ops = {
>>>>>     	.set_fmt = rt5670_set_dai_fmt,
>>>>>     	.set_tdm_slot = rt5670_set_tdm_slot,
>>>>>     	.set_pll = rt5670_set_dai_pll,
>>>>> +	.set_bclk_ratio = rt5670_set_bclk_ratio,
>>>>>     };
>>>>>
>>>>>     static struct snd_soc_dai_driver rt5670_dai[] = {
>>>>> diff --git a/sound/soc/codecs/rt5670.h b/sound/soc/codecs/rt5670.h
>>>>> index 5ba485c..265df80 100644
>>>>> --- a/sound/soc/codecs/rt5670.h
>>>>> +++ b/sound/soc/codecs/rt5670.h
>>>>> @@ -1816,6 +1816,10 @@
>>>>>     #define RT5670_ZCD_HP_DIS			(0x0 << 15)
>>>>>     #define RT5670_ZCD_HP_EN			(0x1 << 15)
>>>>>
>>>>> +/* General Control 3 (0xfc) */
>>>>> +#define RT5670_TDM_DATA_MODE_SEL		(0x1 << 11)
>>>>> +#define RT5670_TDM_DATA_MODE_NOR		(0x0 << 11)
>>>>> +#define RT5670_TDM_DATA_MODE_50FS		(0x1 << 11)
>>>>>
>>>>>     /* Codec Private Register definition */
>>>>>     /* 3D Speaker Control (0x63) */
>>>>>
>>>>
>>>>
>>>> ------Please consider the environment before printing this e-mail.
>>> _______________________________________________
>>> Alsa-devel mailing list
>>> Alsa-devel at alsa-project.org
>>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>>
>