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

[Bard Liao]

> -----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.

> 
> >
> >>
> >>>
> >>> 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
> >