Re: [PATCH v2] ASoC: topology: Fix bclk and fsync inversion in set_link_hw_format()

["Pan, Xiuli" <xiuli.pan@linux.intel.com>]

On 3/1/2018 14:53, Kirill Marinushkin wrote:
> This patch is for alsa-lib. I forgot to mention it in the subject.
>
>
> On 03/01/18 07:48, Kirill Marinushkin wrote:
>> The values of bclk and fsync are inverted WRT the codec. But the existing
>> solution already works for Broadwell, see the alsa-lib config:
>>
>> `alsa-lib/src/conf/topology/broadwell/broadwell.conf`
>>
>> This commit provides the backwards-compatible solution to fix this misuse.
>> This commit goes in pair with the corresponding patch for linux.
>>
>> Signed-off-by: Kirill Marinushkin <k.marinushkin@gmail.com>
>> Cc: Mark Brown <broonie@kernel.org>
>> Cc: Takashi Iwai <tiwai@suse.de>
>> Cc: Jaroslav Kysela <perex@perex.cz>
>> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> Cc: Pan Xiuli <xiuli.pan@linux.intel.com>
>> Cc: Liam Girdwood <liam.r.girdwood@linux.intel.com>
>> Cc: alsa-devel@alsa-project.org
>> ---
>>   include/sound/asoc.h                       | 16 ++++++++++++++--
>>   include/topology.h                         |  4 ++--
>>   src/conf/topology/broadwell/broadwell.conf |  4 ++--
>>   src/topology/pcm.c                         | 30 ++++++++++++++++++++++++++----
>>   4 files changed, 44 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/sound/asoc.h b/include/sound/asoc.h
>> index 0f5d9f9a..89b00703 100644
>> --- a/include/sound/asoc.h
>> +++ b/include/sound/asoc.h
>> @@ -156,6 +156,18 @@
>>   #define SND_SOC_TPLG_LNK_FLGBIT_SYMMETRIC_SAMPLEBITS    (1 << 2)
>>   #define SND_SOC_TPLG_LNK_FLGBIT_VOICE_WAKEUP            (1 << 3)
>>   
>> +/* DAI topology BCLK parameter
>> + * For the backwards capability, by default codec is bclk master
>> + */
>> +#define SND_SOC_TPLG_BCLK_CM         0 /* codec is bclk master */
>> +#define SND_SOC_TPLG_BCLK_CS         1 /* codec is bclk slave */
>> +
>> +/* DAI topology FSYNC parameter
>> + * For the backwards capability, by default codec is fsync master
>> + */
>> +#define SND_SOC_TPLG_FSYNC_CM         0 /* codec is fsync master */
>> +#define SND_SOC_TPLG_FSYNC_CS         1 /* codec is fsync slave */
>> +
>>   /*

Could we keep this align with the old values.

1 for master of BCLK, 0 for slave
1 for master of FSYNC, 0 for slave
This will save some change for SOF topology load parse to DSP.
Now we need to modify a few codes too fit this changing.

And to make things more easier. We can just keep the old values and only modify alsa-lib to let "master" == "codec_slave".
Here we only need to change how we write tplg and no more kernel patches are need.

>>    * Block Header.
>>    * This header precedes all object and object arrays below.
>> @@ -311,8 +323,8 @@ struct snd_soc_tplg_hw_config {
>>   	__u8 clock_gated;	/* 1 if clock can be gated to save power */
>>   	__u8 invert_bclk;	/* 1 for inverted BCLK, 0 for normal */
>>   	__u8 invert_fsync;	/* 1 for inverted frame clock, 0 for normal */
>> -	__u8 bclk_master;	/* 1 for master of BCLK, 0 for slave */
>> -	__u8 fsync_master;	/* 1 for master of FSYNC, 0 for slave */
>> +	__u8 bclk_master;	/* SND_SOC_TPLG_BCLK_ value */
>> +	__u8 fsync_master;	/* SND_SOC_TPLG_FSYNC_ value */
>>   	__u8 mclk_direction;    /* 0 for input, 1 for output */
>>   	__le16 reserved;	/* for 32bit alignment */
>>   	__le32 mclk_rate;	/* MCLK or SYSCLK freqency in Hz */
>> diff --git a/include/topology.h b/include/topology.h
>> index 8779da4d..5d7b46df 100644
>> --- a/include/topology.h
>> +++ b/include/topology.h
>> @@ -1000,8 +1000,8 @@ struct snd_tplg_hw_config_template {
>>   	unsigned char clock_gated;      /* 1 if clock can be gated to save power */
>>   	unsigned char  invert_bclk;     /* 1 for inverted BCLK, 0 for normal */
>>   	unsigned char  invert_fsync;    /* 1 for inverted frame clock, 0 for normal */
>> -	unsigned char  bclk_master;     /* 1 for master of BCLK, 0 for slave */
>> -	unsigned char  fsync_master;    /* 1 for master of FSYNC, 0 for slave */
>> +	unsigned char  bclk_master;     /* SND_SOC_TPLG_BCLK_ value */
>> +	unsigned char  fsync_master;    /* SND_SOC_TPLG_FSYNC_ value */
>>   	unsigned char  mclk_direction;  /* 0 for input, 1 for output */
>>   	unsigned short reserved;        /* for 32bit alignment */
>>   	unsigned int mclk_rate;	        /* MCLK or SYSCLK freqency in Hz */
>> diff --git a/src/conf/topology/broadwell/broadwell.conf b/src/conf/topology/broadwell/broadwell.conf
>> index b8405d93..09fc4daa 100644
>> --- a/src/conf/topology/broadwell/broadwell.conf
>> +++ b/src/conf/topology/broadwell/broadwell.conf
>> @@ -393,8 +393,8 @@ SectionGraph."dsp" {
>>   SectionHWConfig."CodecHWConfig" {
>>   	id "1"
>>   	format "I2S"		# physical audio format.
>> -	bclk   "master"		# Platform is master of bit clock
>> -	fsync  "master"		# platform is master of fsync
>> +	bclk   "codec_slave"	# platform is master of bit clock (codec is slave)
>> +	fsync  "codec_slave"	# platform is master of fsync (codec is slave)
>>   }
>>   
>>   SectionLink."Codec" {
>> diff --git a/src/topology/pcm.c b/src/topology/pcm.c
>> index 58cee31d..bdab8eef 100644
>> --- a/src/topology/pcm.c
>> +++ b/src/topology/pcm.c
>> @@ -1137,8 +1137,19 @@ int tplg_parse_hw_config(snd_tplg_t *tplg, snd_config_t *cfg,
>>   			if (snd_config_get_string(n, &val) < 0)
>>   				return -EINVAL;
>>   
>> -			if (!strcmp(val, "master"))
>> -				hw_cfg->bclk_master = true;
>> +			if (!strcmp(val, "master")) {
>> +				/* For backwards capability,
>> +				 * "master" == "codec is slave"
>> +				 */
>> +				SNDERR("warning: deprecated bclk value '%s'\n",
>> +				       val);
>> +
>> +				hw_cfg->bclk_master = SND_SOC_TPLG_BCLK_CS;
>> +			} else if (!strcmp(val, "codec_slave")) {
>> +				hw_cfg->bclk_master = SND_SOC_TPLG_BCLK_CS;
>> +			} else if (!strcmp(val, "codec_master")) {
>> +				hw_cfg->bclk_master = SND_SOC_TPLG_BCLK_CM;
>> +			}
>>   			continue;
>>   		}
  What if some old tplg used "slave"? What should we do to those values.

>>   
>> @@ -1163,8 +1174,19 @@ int tplg_parse_hw_config(snd_tplg_t *tplg, snd_config_t *cfg,
>>   			if (snd_config_get_string(n, &val) < 0)
>>   				return -EINVAL;
>>   
>> -			if (!strcmp(val, "master"))
>> -				hw_cfg->fsync_master = true;
>> +			if (!strcmp(val, "master")) {
>> +				/* For backwards capability,
>> +				 * "master" == "codec is slave"
>> +				 */
>> +				SNDERR("warning: deprecated fsync value '%s'\n",
>> +				       val);
>> +
>> +				hw_cfg->fsync_master = SND_SOC_TPLG_FSYNC_CS;
>> +			} else if (!strcmp(val, "codec_slave")) {
>> +				hw_cfg->fsync_master = SND_SOC_TPLG_FSYNC_CS;
>> +			} else if (!strcmp(val, "codec_master")) {
>> +				hw_cfg->fsync_master = SND_SOC_TPLG_FSYNC_CM;
>> +			}
>>   			continue;
>>   		}
>>   
Test with our SOF topology and found a missing part mclk.
We also use mclk now, and I think these KEY WORDS should align with the 
new changes, too.

Thanks
Xiuli


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel