[RFC PATCH 0/3] alsa-lib/ASoC: use inclusive language for bclk/fsync/topology

[RFC PATCH 0/3] alsa-lib/ASoC: use inclusive language for bclk/fsync/topology

[Pierre-Louis Bossart]

The SOF (Sound Open Firmware) tree contains a lot of references in
topology files to 'codec_slave'/'codec_master' terms, which in turn
come from alsa-lib and ALSA/ASoC topology support at the kernel
level. These terms are no longer compatible with the guidelines
adopted by the kernel community [1] and need to change in
backwards-compatible ways.

The main/secondary terms typically suggested in guidelines don't mean
anything for clocks, this patchset suggests instead the use of
'provider' and 'follower' terms, with the 'codec' prefix kept to make
it clear that the codec is the reference. The CM/CF suffixes are also
replaced by CP/CF.

It can be argued that the change of suffix is invasive, but finding a
replacement that keeps the M and S shortcuts has proven difficult in
quite a few contexts.

The previous definitions are kept for backwards-compatibility so this
change should not have any functional impact. It is suggested that new
contributions only use the new terms but there is no requirement to
transition immediately to the new definitions for existing code. Intel
will however update all its past contributions related to bit
clock/frame sync configurations immediately.

This suggestion is easier to review first at the alsa-lib level, and
if agreed follow-up contributions for the Linux kernel [2] and SOF
firmware [3] will be provided.

Feedback welcome
~Pierre

[1] https://lkml.org/lkml/2020/7/4/229
[2] https://github.com/plbossart/sound/tree/fix/inclusing-language-bclk-fsync
[3] https://github.com/plbossart/sof/tree/fix/inclusive-language-bclk-fsync

Pierre-Louis Bossart (3):
  topology: use inclusive language for bclk
  topology: use inclusive language for fsync
  topology: use inclusive language in documentation

 include/sound/uapi/asoc.h | 22 +++++++-----
 include/topology.h        |  8 ++---
 src/topology/pcm.c        | 74 ++++++++++++++++++++++++++++-----------
 3 files changed, 71 insertions(+), 33 deletions(-)

-- 
2.25.1

[RFC PATCH 1/3] topology: use inclusive language for bclk

[Pierre-Louis Bossart]

use bclk_provider for structure fields, 'codec_provider' and
'codec_follower' for options and modify #defines to use CP and CF
suffixes.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/sound/uapi/asoc.h | 11 +++++++----
 include/topology.h        |  2 +-
 src/topology/pcm.c        | 37 +++++++++++++++++++++++++++----------
 3 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/include/sound/uapi/asoc.h b/include/sound/uapi/asoc.h
index 4efb4ec4..8558992f 100644
--- a/include/sound/uapi/asoc.h
+++ b/include/sound/uapi/asoc.h
@@ -169,10 +169,13 @@
 #define SND_SOC_TPLG_LNK_FLGBIT_VOICE_WAKEUP            (1 << 3)
 
 /* DAI topology BCLK parameter
- * For the backwards capability, by default codec is bclk master
+ * For the backwards capability, by default codec is bclk provider
  */
-#define SND_SOC_TPLG_BCLK_CM         0 /* codec is bclk master */
-#define SND_SOC_TPLG_BCLK_CS         1 /* codec is bclk slave */
+#define SND_SOC_TPLG_BCLK_CP         0 /* codec is bclk provider */
+#define SND_SOC_TPLG_BCLK_CF         1 /* codec is bclk follower */
+/* keep previous definitions for compatibility */
+#define SND_SOC_TPLG_BCLK_CM         SND_SOC_TPLG_BCLK_CP
+#define SND_SOC_TPLG_BCLK_CS         SND_SOC_TPLG_BCLK_CF
 
 /* DAI topology FSYNC parameter
  * For the backwards capability, by default codec is fsync master
@@ -335,7 +338,7 @@ struct snd_soc_tplg_hw_config {
 	__u8 clock_gated;	/* SND_SOC_TPLG_DAI_CLK_GATE_ value */
 	__u8 invert_bclk;	/* 1 for inverted BCLK, 0 for normal */
 	__u8 invert_fsync;	/* 1 for inverted frame clock, 0 for normal */
-	__u8 bclk_master;	/* SND_SOC_TPLG_BCLK_ value */
+	__u8 bclk_provider;	/* SND_SOC_TPLG_BCLK_ value */
 	__u8 fsync_master;	/* SND_SOC_TPLG_FSYNC_ value */
 	__u8 mclk_direction;    /* SND_SOC_TPLG_MCLK_ value */
 	__le16 reserved;	/* for 32bit alignment */
diff --git a/include/topology.h b/include/topology.h
index 1f52e66e..6c970649 100644
--- a/include/topology.h
+++ b/include/topology.h
@@ -1028,7 +1028,7 @@ struct snd_tplg_hw_config_template {
 	unsigned char clock_gated;      /* SND_SOC_TPLG_DAI_CLK_GATE_ value */
 	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;     /* SND_SOC_TPLG_BCLK_ value */
+	unsigned char  bclk_provider;   /* SND_SOC_TPLG_BCLK_ value */
 	unsigned char  fsync_master;    /* SND_SOC_TPLG_FSYNC_ value */
 	unsigned char  mclk_direction;  /* SND_SOC_TPLG_MCLK_ value */
 	unsigned short reserved;        /* for 32bit alignment */
diff --git a/src/topology/pcm.c b/src/topology/pcm.c
index a60ba00d..c6fb07e2 100644
--- a/src/topology/pcm.c
+++ b/src/topology/pcm.c
@@ -1411,6 +1411,7 @@ int tplg_parse_hw_config(snd_tplg_t *tplg, snd_config_t *cfg,
 	snd_config_t *n;
 	const char *id, *val = NULL;
 	int ret, ival;
+	bool provider_legacy;
 
 	elem = tplg_elem_new_common(tplg, cfg, NULL, SND_TPLG_TYPE_HW_CONFIG);
 	if (!elem)
@@ -1451,8 +1452,15 @@ int tplg_parse_hw_config(snd_tplg_t *tplg, snd_config_t *cfg,
 			continue;
 		}
 
-		if (strcmp(id, "bclk") == 0 ||
-		    strcmp(id, "bclk_master") == 0) {
+		provider_legacy = false;
+		if (strcmp(id, "bclk_master") == 0) {
+			SNDERR("deprecated option %s, please use 'bclk'\n", id);
+			provider_legacy = true;
+		}
+
+		if (provider_legacy ||
+		    strcmp(id, "bclk") == 0) {
+
 			if (snd_config_get_string(n, &val) < 0)
 				return -EINVAL;
 
@@ -1462,12 +1470,21 @@ int tplg_parse_hw_config(snd_tplg_t *tplg, snd_config_t *cfg,
 				 */
 				SNDERR("deprecated bclk value '%s'", val);
 
-				hw_cfg->bclk_master = SND_SOC_TPLG_BCLK_CS;
+				hw_cfg->bclk_provider = SND_SOC_TPLG_BCLK_CF;
 			} else if (!strcmp(val, "codec_slave")) {
-				hw_cfg->bclk_master = SND_SOC_TPLG_BCLK_CS;
+				SNDERR("deprecated bclk value '%s', use 'codec_follower'", val);
+
+				hw_cfg->bclk_provider = SND_SOC_TPLG_BCLK_CF;
+			} else if (!strcmp(val, "codec_follower")) {
+				hw_cfg->bclk_provider = SND_SOC_TPLG_BCLK_CF;
 			} else if (!strcmp(val, "codec_master")) {
-				hw_cfg->bclk_master = SND_SOC_TPLG_BCLK_CM;
+				SNDERR("deprecated bclk value '%s', use 'codec_provider", val);
+
+				hw_cfg->bclk_provider = SND_SOC_TPLG_BCLK_CP;
+			} else if (!strcmp(val, "codec_provider")) {
+				hw_cfg->bclk_provider = SND_SOC_TPLG_BCLK_CP;
 			}
+
 			continue;
 		}
 
@@ -1623,10 +1640,10 @@ int tplg_save_hw_config(snd_tplg_t *tplg ATTRIBUTE_UNUSED,
 	if (err >= 0 && hc->fmt)
 		err = tplg_save_printf(dst, pfx, "\tformat '%s'\n",
 				       get_audio_hw_format_name(hc->fmt));
-	if (err >= 0 && hc->bclk_master)
+	if (err >= 0 && hc->bclk_provider)
 		err = tplg_save_printf(dst, pfx, "\tbclk '%s'\n",
-				       hc->bclk_master == SND_SOC_TPLG_BCLK_CS ?
-						"codec_slave" : "codec_master");
+				       hc->bclk_provider == SND_SOC_TPLG_BCLK_CF ?
+						"codec_follower" : "codec_provider");
 	if (err >= 0 && hc->bclk_rate)
 		err = tplg_save_printf(dst, pfx, "\tbclk_freq %u\n",
 				       hc->bclk_rate);
@@ -1791,7 +1808,7 @@ static int set_link_hw_config(struct snd_soc_tplg_hw_config *cfg,
 	cfg->clock_gated = tpl->clock_gated;
 	cfg->invert_bclk = tpl->invert_bclk;
 	cfg->invert_fsync = tpl->invert_fsync;
-	cfg->bclk_master = tpl->bclk_master;
+	cfg->bclk_provider = tpl->bclk_provider;
 	cfg->fsync_master = tpl->fsync_master;
 	cfg->mclk_direction = tpl->mclk_direction;
 	cfg->reserved = tpl->reserved;
@@ -2174,7 +2191,7 @@ next:
 		hw->clock_gated = link->hw_config[i].clock_gated;
 		hw->invert_bclk = link->hw_config[i].invert_bclk;
 		hw->invert_fsync = link->hw_config[i].invert_fsync;
-		hw->bclk_master = link->hw_config[i].bclk_master;
+		hw->bclk_provider = link->hw_config[i].bclk_provider;
 		hw->fsync_master = link->hw_config[i].fsync_master;
 		hw->mclk_direction = link->hw_config[i].mclk_direction;
 		hw->mclk_rate = link->hw_config[i].mclk_rate;
-- 
2.25.1

[RFC PATCH 2/3] topology: use inclusive language for fsync

[Pierre-Louis Bossart]

use fsync_provider for structure fields, 'codec_provider' and
'codec_follower' for options and modify #defines to use CP and CF
suffixes.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/sound/uapi/asoc.h | 11 +++++++----
 include/topology.h        |  2 +-
 src/topology/pcm.c        | 37 ++++++++++++++++++++++++++-----------
 3 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/include/sound/uapi/asoc.h b/include/sound/uapi/asoc.h
index 8558992f..91144a1f 100644
--- a/include/sound/uapi/asoc.h
+++ b/include/sound/uapi/asoc.h
@@ -178,10 +178,13 @@
 #define SND_SOC_TPLG_BCLK_CS         SND_SOC_TPLG_BCLK_CF
 
 /* DAI topology FSYNC parameter
- * For the backwards capability, by default codec is fsync master
+ * For the backwards capability, by default codec is fsync provider
  */
-#define SND_SOC_TPLG_FSYNC_CM         0 /* codec is fsync master */
-#define SND_SOC_TPLG_FSYNC_CS         1 /* codec is fsync slave */
+#define SND_SOC_TPLG_FSYNC_CP         0 /* codec is fsync provider */
+#define SND_SOC_TPLG_FSYNC_CF         1 /* codec is fsync follower */
+/* keep previous definitions for compatibility */
+#define SND_SOC_TPLG_FSYNC_CM         SND_SOC_TPLG_FSYNC_CP
+#define SND_SOC_TPLG_FSYNC_CS         SND_SOC_TPLG_FSYNC_CF
 
 /*
  * Block Header.
@@ -339,7 +342,7 @@ struct snd_soc_tplg_hw_config {
 	__u8 invert_bclk;	/* 1 for inverted BCLK, 0 for normal */
 	__u8 invert_fsync;	/* 1 for inverted frame clock, 0 for normal */
 	__u8 bclk_provider;	/* SND_SOC_TPLG_BCLK_ value */
-	__u8 fsync_master;	/* SND_SOC_TPLG_FSYNC_ value */
+	__u8 fsync_provider;	/* SND_SOC_TPLG_FSYNC_ value */
 	__u8 mclk_direction;    /* SND_SOC_TPLG_MCLK_ value */
 	__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 6c970649..4ade20df 100644
--- a/include/topology.h
+++ b/include/topology.h
@@ -1029,7 +1029,7 @@ struct snd_tplg_hw_config_template {
 	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_provider;   /* SND_SOC_TPLG_BCLK_ value */
-	unsigned char  fsync_master;    /* SND_SOC_TPLG_FSYNC_ value */
+	unsigned char  fsync_provider;  /* SND_SOC_TPLG_FSYNC_ value */
 	unsigned char  mclk_direction;  /* SND_SOC_TPLG_MCLK_ value */
 	unsigned short reserved;        /* for 32bit alignment */
 	unsigned int mclk_rate;	        /* MCLK or SYSCLK freqency in Hz */
diff --git a/src/topology/pcm.c b/src/topology/pcm.c
index c6fb07e2..1a3052c7 100644
--- a/src/topology/pcm.c
+++ b/src/topology/pcm.c
@@ -1505,8 +1505,15 @@ int tplg_parse_hw_config(snd_tplg_t *tplg, snd_config_t *cfg,
 			continue;
 		}
 
-		if (strcmp(id, "fsync") == 0 ||
-		    strcmp(id, "fsync_master") == 0) {
+		provider_legacy = false;
+		if (strcmp(id, "fsync_provider") == 0) {
+			SNDERR("deprecated option %s, please use 'fsync'\n", id);
+			provider_legacy = true;
+		}
+
+		if (provider_legacy ||
+		    strcmp(id, "fsync") == 0) {
+
 			if (snd_config_get_string(n, &val) < 0)
 				return -EINVAL;
 
@@ -1516,11 +1523,19 @@ int tplg_parse_hw_config(snd_tplg_t *tplg, snd_config_t *cfg,
 				 */
 				SNDERR("deprecated fsync value '%s'", val);
 
-				hw_cfg->fsync_master = SND_SOC_TPLG_FSYNC_CS;
+				hw_cfg->fsync_provider = SND_SOC_TPLG_FSYNC_CF;
 			} 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;
+				SNDERR("deprecated fsync value '%s', use 'codec_follower'", val);
+
+				hw_cfg->fsync_provider = SND_SOC_TPLG_FSYNC_CF;
+			} else if (!strcmp(val, "codec_follower")) {
+				hw_cfg->fsync_provider = SND_SOC_TPLG_FSYNC_CF;
+ 			} else if (!strcmp(val, "codec_master")) {
+				SNDERR("deprecated fsync value '%s', use 'codec_provider'", val);
+
+				hw_cfg->fsync_provider = SND_SOC_TPLG_FSYNC_CP;
+			} else if (!strcmp(val, "codec_provider")) {
+				hw_cfg->fsync_provider = SND_SOC_TPLG_FSYNC_CP;
 			}
 			continue;
 		}
@@ -1649,9 +1664,9 @@ int tplg_save_hw_config(snd_tplg_t *tplg ATTRIBUTE_UNUSED,
 				       hc->bclk_rate);
 	if (err >= 0 && hc->invert_bclk)
 		err = tplg_save_printf(dst, pfx, "\tbclk_invert 1\n");
-	if (err >= 0 && hc->fsync_master)
-		err = tplg_save_printf(dst, pfx, "\tfsync_master '%s'\n",
-				       hc->fsync_master == SND_SOC_TPLG_FSYNC_CS ?
+	if (err >= 0 && hc->fsync_provider)
+		err = tplg_save_printf(dst, pfx, "\tfsync_provider '%s'\n",
+				       hc->fsync_provider == SND_SOC_TPLG_FSYNC_CS ?
 						"codec_slave" : "codec_master");
 	if (err >= 0 && hc->fsync_rate)
 		err = tplg_save_printf(dst, pfx, "\tfsync_freq %u\n",
@@ -1809,7 +1824,7 @@ static int set_link_hw_config(struct snd_soc_tplg_hw_config *cfg,
 	cfg->invert_bclk = tpl->invert_bclk;
 	cfg->invert_fsync = tpl->invert_fsync;
 	cfg->bclk_provider = tpl->bclk_provider;
-	cfg->fsync_master = tpl->fsync_master;
+	cfg->fsync_provider = tpl->fsync_provider;
 	cfg->mclk_direction = tpl->mclk_direction;
 	cfg->reserved = tpl->reserved;
 	cfg->mclk_rate = tpl->mclk_rate;
@@ -2192,7 +2207,7 @@ next:
 		hw->invert_bclk = link->hw_config[i].invert_bclk;
 		hw->invert_fsync = link->hw_config[i].invert_fsync;
 		hw->bclk_provider = link->hw_config[i].bclk_provider;
-		hw->fsync_master = link->hw_config[i].fsync_master;
+		hw->fsync_provider = link->hw_config[i].fsync_provider;
 		hw->mclk_direction = link->hw_config[i].mclk_direction;
 		hw->mclk_rate = link->hw_config[i].mclk_rate;
 		hw->bclk_rate = link->hw_config[i].bclk_rate;
-- 
2.25.1

[RFC PATCH 3/3] topology: use inclusive language in documentation

[Pierre-Louis Bossart]

Use codec_provider and codec_follower for bclk and fsync

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/topology.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/topology.h b/include/topology.h
index 4ade20df..fcf9361d 100644
--- a/include/topology.h
+++ b/include/topology.h
@@ -658,8 +658,8 @@ extern "C" {
  *
  *	id "1"				# used for binding to the config
  *	format "I2S"			# physical audio format.
- *	bclk   "master"			# Platform is master of bit clock
- *	fsync  "slave"			# Platform is slave of fsync
+ *	bclk   "codec_provider"		# Codec provides the bit clock
+ *	fsync  "codec_follower"		# Codec follows the fsync
  * }
  * </pre>
  *
-- 
2.25.1

Re: [RFC PATCH 0/3] alsa-lib/ASoC: use inclusive language for bclk/fsync/topology

[Jaroslav Kysela]

Dne 03. 09. 20 v 22:10 Pierre-Louis Bossart napsal(a):
> The SOF (Sound Open Firmware) tree contains a lot of references in
> topology files to 'codec_slave'/'codec_master' terms, which in turn
> come from alsa-lib and ALSA/ASoC topology support at the kernel
> level. These terms are no longer compatible with the guidelines
> adopted by the kernel community [1] and need to change in
> backwards-compatible ways.
> 
> The main/secondary terms typically suggested in guidelines don't mean
> anything for clocks, this patchset suggests instead the use of
> 'provider' and 'follower' terms, with the 'codec' prefix kept to make
> it clear that the codec is the reference. The CM/CF suffixes are also
> replaced by CP/CF.

Only my 2 cents: It's just another word combo. See bellow for sources for others.

I would prefer probably provider/consumer . It sounds more technic.

[1] https://en.wikipedia.org/wiki/Master/slave_(technology)
[2]
https://www.zdnet.com/article/linux-team-approves-new-terminology-bans-terms-like-blacklist-and-slave/

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 0/3] alsa-lib/ASoC: use inclusive language for bclk/fsync/topology

[Pierre-Louis Bossart]

On 9/3/20 3:42 PM, Jaroslav Kysela wrote:
> Dne 03. 09. 20 v 22:10 Pierre-Louis Bossart napsal(a):
>> The SOF (Sound Open Firmware) tree contains a lot of references in
>> topology files to 'codec_slave'/'codec_master' terms, which in turn
>> come from alsa-lib and ALSA/ASoC topology support at the kernel
>> level. These terms are no longer compatible with the guidelines
>> adopted by the kernel community [1] and need to change in
>> backwards-compatible ways.
>>
>> The main/secondary terms typically suggested in guidelines don't mean
>> anything for clocks, this patchset suggests instead the use of
>> 'provider' and 'follower' terms, with the 'codec' prefix kept to make
>> it clear that the codec is the reference. The CM/CF suffixes are also
>> replaced by CP/CF.
> 
> Only my 2 cents: It's just another word combo. See bellow for sources for others.
> 
> I would prefer probably provider/consumer . It sounds more technic.

Thanks Jaroslav for chiming in. I had a similar set of comments in 
internal reviews, but we didn't really have any consensus and I have not 
seen good guidance specifically for clocks.

Provider/consumer is typically used for discrete data exchange with some 
sort of locking and buffer fullness metric, but for clocks we'd want 
something that hints at one device following the timing defined by another.

"follow" or "track" seem clearer than 'consume' IMHO, but I will side 
with the majority, this is an RFC which can be modified at will.

Re: [RFC PATCH 0/3] alsa-lib/ASoC: use inclusive language for bclk/fsync/topology

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1139 bytes --]

On Thu, Sep 03, 2020 at 04:32:22PM -0500, Pierre-Louis Bossart wrote:
> On 9/3/20 3:42 PM, Jaroslav Kysela wrote:

> > 
> > Only my 2 cents: It's just another word combo. See bellow for sources for others.
> > 
> > I would prefer probably provider/consumer . It sounds more technic.

> Thanks Jaroslav for chiming in. I had a similar set of comments in internal
> reviews, but we didn't really have any consensus and I have not seen good
> guidance specifically for clocks.

> Provider/consumer is typically used for discrete data exchange with some
> sort of locking and buffer fullness metric, but for clocks we'd want
> something that hints at one device following the timing defined by another.

> "follow" or "track" seem clearer than 'consume' IMHO, but I will side with
> the majority, this is an RFC which can be modified at will.

Producer/consumer is already quite widely used for clocks (possibly
following the regulator API which was templated off the clock API and
uses consumer).  The follow/track stuff definitely seems awkward to me.
Have we seen any movement from anyone like CODEC vendors on this?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 1/3] topology: use inclusive language for bclk

[Takashi Iwai]

On Thu, 03 Sep 2020 22:10:22 +0200,
Pierre-Louis Bossart wrote:
> 
> use bclk_provider for structure fields, 'codec_provider' and
> 'codec_follower' for options and modify #defines to use CP and CF
> suffixes.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  include/sound/uapi/asoc.h | 11 +++++++----
>  include/topology.h        |  2 +-
>  src/topology/pcm.c        | 37 +++++++++++++++++++++++++++----------
>  3 files changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/include/sound/uapi/asoc.h b/include/sound/uapi/asoc.h
> index 4efb4ec4..8558992f 100644
> --- a/include/sound/uapi/asoc.h
> +++ b/include/sound/uapi/asoc.h
> @@ -169,10 +169,13 @@
>  #define SND_SOC_TPLG_LNK_FLGBIT_VOICE_WAKEUP            (1 << 3)
>  
>  /* DAI topology BCLK parameter
> - * For the backwards capability, by default codec is bclk master
> + * For the backwards capability, by default codec is bclk provider
>   */
> -#define SND_SOC_TPLG_BCLK_CM         0 /* codec is bclk master */
> -#define SND_SOC_TPLG_BCLK_CS         1 /* codec is bclk slave */
> +#define SND_SOC_TPLG_BCLK_CP         0 /* codec is bclk provider */
> +#define SND_SOC_TPLG_BCLK_CF         1 /* codec is bclk follower */
> +/* keep previous definitions for compatibility */
> +#define SND_SOC_TPLG_BCLK_CM         SND_SOC_TPLG_BCLK_CP
> +#define SND_SOC_TPLG_BCLK_CS         SND_SOC_TPLG_BCLK_CF

Those change are indeed backward compatible, but ...

>  
>  /* DAI topology FSYNC parameter
>   * For the backwards capability, by default codec is fsync master
> @@ -335,7 +338,7 @@ struct snd_soc_tplg_hw_config {
>  	__u8 clock_gated;	/* SND_SOC_TPLG_DAI_CLK_GATE_ value */
>  	__u8 invert_bclk;	/* 1 for inverted BCLK, 0 for normal */
>  	__u8 invert_fsync;	/* 1 for inverted frame clock, 0 for normal */
> -	__u8 bclk_master;	/* SND_SOC_TPLG_BCLK_ value */
> +	__u8 bclk_provider;	/* SND_SOC_TPLG_BCLK_ value */
>  	__u8 fsync_master;	/* SND_SOC_TPLG_FSYNC_ value */
>  	__u8 mclk_direction;    /* SND_SOC_TPLG_MCLK_ value */
>  	__le16 reserved;	/* for 32bit alignment */

Is it 100% compatible?  Note that the uapi/* header is a copy from the
kernel header, and it means that we'll change the same for the kernel,
too.

The similar argument applied to the patch 2, too.


thanks,

Takashi

Re: [RFC PATCH 0/3] alsa-lib/ASoC: use inclusive language for bclk/fsync/topology

[Pierre-Louis Bossart]

On 9/4/20 3:50 AM, Mark Brown wrote:
> On Thu, Sep 03, 2020 at 04:32:22PM -0500, Pierre-Louis Bossart wrote:
>> On 9/3/20 3:42 PM, Jaroslav Kysela wrote:
> 
>>>
>>> Only my 2 cents: It's just another word combo. See bellow for sources for others.
>>>
>>> I would prefer probably provider/consumer . It sounds more technic.
> 
>> Thanks Jaroslav for chiming in. I had a similar set of comments in internal
>> reviews, but we didn't really have any consensus and I have not seen good
>> guidance specifically for clocks.
> 
>> Provider/consumer is typically used for discrete data exchange with some
>> sort of locking and buffer fullness metric, but for clocks we'd want
>> something that hints at one device following the timing defined by another.
> 
>> "follow" or "track" seem clearer than 'consume' IMHO, but I will side with
>> the majority, this is an RFC which can be modified at will.
> 
> Producer/consumer is already quite widely used for clocks (possibly
> following the regulator API which was templated off the clock API and
> uses consumer).  The follow/track stuff definitely seems awkward to me.
> Have we seen any movement from anyone like CODEC vendors on this?

No, I haven't seen any input from CODEC vendors.

I'll use consumer then since that's preferred by both Jaroslav and Mark. 
Thanks for the feedback.

Re: [RFC PATCH 1/3] topology: use inclusive language for bclk

[Pierre-Louis Bossart]

>>   /* DAI topology FSYNC parameter
>>    * For the backwards capability, by default codec is fsync master
>> @@ -335,7 +338,7 @@ struct snd_soc_tplg_hw_config {
>>   	__u8 clock_gated;	/* SND_SOC_TPLG_DAI_CLK_GATE_ value */
>>   	__u8 invert_bclk;	/* 1 for inverted BCLK, 0 for normal */
>>   	__u8 invert_fsync;	/* 1 for inverted frame clock, 0 for normal */
>> -	__u8 bclk_master;	/* SND_SOC_TPLG_BCLK_ value */
>> +	__u8 bclk_provider;	/* SND_SOC_TPLG_BCLK_ value */
>>   	__u8 fsync_master;	/* SND_SOC_TPLG_FSYNC_ value */
>>   	__u8 mclk_direction;    /* SND_SOC_TPLG_MCLK_ value */
>>   	__le16 reserved;	/* for 32bit alignment */
> 
> Is it 100% compatible?  Note that the uapi/* header is a copy from the
> kernel header, and it means that we'll change the same for the kernel,
> too.

It's absolutely 100% compatible by design.
I was planning to update the kernel uapi header to align changes, but 
the volume of code is much lower on the alsa-lib side. Will resubmit 
with the preferred provider/consumer wording.

Re: [RFC PATCH 1/3] topology: use inclusive language for bclk

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 679 bytes --]

On Tue, Sep 08, 2020 at 08:39:13AM -0500, Pierre-Louis Bossart wrote:

> > > -	__u8 bclk_master;	/* SND_SOC_TPLG_BCLK_ value */
> > > +	__u8 bclk_provider;	/* SND_SOC_TPLG_BCLK_ value */

> > Is it 100% compatible?  Note that the uapi/* header is a copy from the
> > kernel header, and it means that we'll change the same for the kernel,
> > too.

> It's absolutely 100% compatible by design.
> I was planning to update the kernel uapi header to align changes, but the
> volume of code is much lower on the alsa-lib side. Will resubmit with the
> preferred provider/consumer wording.

It's binary compatible but it'd break the build for any existing code
using the UAPI headers.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 1/3] topology: use inclusive language for bclk

[Pierre-Louis Bossart]

On 9/8/20 9:35 AM, Mark Brown wrote:
> On Tue, Sep 08, 2020 at 08:39:13AM -0500, Pierre-Louis Bossart wrote:
> 
>>>> -	__u8 bclk_master;	/* SND_SOC_TPLG_BCLK_ value */
>>>> +	__u8 bclk_provider;	/* SND_SOC_TPLG_BCLK_ value */
> 
>>> Is it 100% compatible?  Note that the uapi/* header is a copy from the
>>> kernel header, and it means that we'll change the same for the kernel,
>>> too.
> 
>> It's absolutely 100% compatible by design.
>> I was planning to update the kernel uapi header to align changes, but the
>> volume of code is much lower on the alsa-lib side. Will resubmit with the
>> preferred provider/consumer wording.
> 
> It's binary compatible but it'd break the build for any existing code
> using the UAPI headers.

Sorry, I don't fully get the comment. Aren't the uapi headers copied 
into each software tree that relies on them?

Re: [RFC PATCH 1/3] topology: use inclusive language for bclk

[Jaroslav Kysela]

Dne 08. 09. 20 v 16:41 Pierre-Louis Bossart napsal(a):
> 
> 
> On 9/8/20 9:35 AM, Mark Brown wrote:
>> On Tue, Sep 08, 2020 at 08:39:13AM -0500, Pierre-Louis Bossart wrote:
>>
>>>>> -	__u8 bclk_master;	/* SND_SOC_TPLG_BCLK_ value */
>>>>> +	__u8 bclk_provider;	/* SND_SOC_TPLG_BCLK_ value */
>>
>>>> Is it 100% compatible?  Note that the uapi/* header is a copy from the
>>>> kernel header, and it means that we'll change the same for the kernel,
>>>> too.
>>
>>> It's absolutely 100% compatible by design.
>>> I was planning to update the kernel uapi header to align changes, but the
>>> volume of code is much lower on the alsa-lib side. Will resubmit with the
>>> preferred provider/consumer wording.
>>
>> It's binary compatible but it'd break the build for any existing code
>> using the UAPI headers.
> 
> Sorry, I don't fully get the comment. Aren't the uapi headers copied 
> into each software tree that relies on them?

I think that only alsa-lib's topology library uses this header (and has own
header copy), thus the breakage is minimal in this case.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [RFC PATCH 1/3] topology: use inclusive language for bclk

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 838 bytes --]

On Tue, Sep 08, 2020 at 09:41:34AM -0500, Pierre-Louis Bossart wrote:
> On 9/8/20 9:35 AM, Mark Brown wrote:
> > On Tue, Sep 08, 2020 at 08:39:13AM -0500, Pierre-Louis Bossart wrote:

> > > It's absolutely 100% compatible by design.
> > > I was planning to update the kernel uapi header to align changes, but the
> > > volume of code is much lower on the alsa-lib side. Will resubmit with the
> > > preferred provider/consumer wording.

> > It's binary compatible but it'd break the build for any existing code
> > using the UAPI headers.

> Sorry, I don't fully get the comment. Aren't the uapi headers copied into
> each software tree that relies on them?

Yes, it's just a question of how disruptive an update ends up being for
people - if hardly anyone is using it and they know about the change it
probably doesn't matter that much.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]