[PATCH 0/3] alsa-lib/ASoC: use inclusive language for bclk/fsync/topology
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 'consumer' terms, with the 'codec' prefix kept to make it clear that the codec is the reference. The CM/CS suffixes are also replaced by CP/CC.
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/inclusive-language-bclk-fsync [3] https://github.com/plbossart/sof/tree/fix/inclusive-language-bclk-fsync
Changes since RFC: replaced 'follower' by 'consumer' as suggested by Jaroslav and Marc minor cleanups
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 | 75 +++++++++++++++++++++++++++------------ 3 files changed, 71 insertions(+), 34 deletions(-)
use bclk_provider for structure fields, 'codec_provider' and 'codec_consumer' for options and modify #defines to use CP and CC 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 | 36 ++++++++++++++++++++++++++---------- 3 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/include/sound/uapi/asoc.h b/include/sound/uapi/asoc.h index 4efb4ec4..ceafb1a9 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_CC 1 /* codec is bclk consumer */ +/* 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_CC
/* 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 191b7a0a..f05df348 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,11 +1470,19 @@ 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_CC; } else if (!strcmp(val, "codec_slave")) { - hw_cfg->bclk_master = SND_SOC_TPLG_BCLK_CS; + SNDERR("deprecated bclk value '%s', use 'codec_consumer'", val); + + hw_cfg->bclk_provider = SND_SOC_TPLG_BCLK_CC; + } else if (!strcmp(val, "codec_consumer")) { + hw_cfg->bclk_provider = SND_SOC_TPLG_BCLK_CC; } 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 +1639,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_CC ? + "codec_consumer" : "codec_provider"); if (err >= 0 && hc->bclk_rate) err = tplg_save_printf(dst, pfx, "\tbclk_freq %u\n", hc->bclk_rate); @@ -1791,7 +1807,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 +2190,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;
use fsync_provider for structure fields, 'codec_provider' and 'codec_consumer' for options and modify #defines to use CP and CC 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 | 39 +++++++++++++++++++++++++++------------ 3 files changed, 35 insertions(+), 17 deletions(-)
diff --git a/include/sound/uapi/asoc.h b/include/sound/uapi/asoc.h index ceafb1a9..f32c5697 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_CC
/* 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_CC 1 /* codec is fsync consumer */ +/* 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_CC
/* * 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 f05df348..888aa59a 100644 --- a/src/topology/pcm.c +++ b/src/topology/pcm.c @@ -1504,8 +1504,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_master") == 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;
@@ -1515,11 +1522,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_CC; } 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_consumer'", val); + + hw_cfg->fsync_provider = SND_SOC_TPLG_FSYNC_CC; + } else if (!strcmp(val, "codec_consumer")) { + hw_cfg->fsync_provider = SND_SOC_TPLG_FSYNC_CC; + } 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; } @@ -1648,10 +1663,10 @@ 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 ? - "codec_slave" : "codec_master"); + if (err >= 0 && hc->fsync_provider) + err = tplg_save_printf(dst, pfx, "\tfsync_provider '%s'\n", + hc->fsync_provider == SND_SOC_TPLG_FSYNC_CC ? + "codec_consumer" : "codec_provider"); if (err >= 0 && hc->fsync_rate) err = tplg_save_printf(dst, pfx, "\tfsync_freq %u\n", hc->fsync_rate); @@ -1808,7 +1823,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; @@ -2191,7 +2206,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;
Use codec_provider and codec_consumer 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..d1feee4d 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_consumer" # Codec follows the fsync * } * </pre> *
On Fri, 18 Sep 2020 23:28:29 +0200, Pierre-Louis Bossart wrote:
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 'consumer' terms, with the 'codec' prefix kept to make it clear that the codec is the reference. The CM/CS suffixes are also replaced by CP/CC.
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.
It's OK from alsa-lib POV; although the uapi header change isn't 100% safe, the user of this header is really our ones, so it's practically acceptable, I suppose.
Could you submit the changes for kernel, so that it can be merged in time? Basically alsa-lib is synced with kernel, so the kernel should be changed at first.
thanks,
Takashi
Feedback welcome ~Pierre
[1] https://lkml.org/lkml/2020/7/4/229 [2] https://github.com/plbossart/sound/tree/fix/inclusive-language-bclk-fsync [3] https://github.com/plbossart/sof/tree/fix/inclusive-language-bclk-fsync
Changes since RFC: replaced 'follower' by 'consumer' as suggested by Jaroslav and Marc minor cleanups
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 | 75 +++++++++++++++++++++++++++------------ 3 files changed, 71 insertions(+), 34 deletions(-)
-- 2.25.1
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 'consumer' terms, with the 'codec' prefix kept to make it clear that the codec is the reference. The CM/CS suffixes are also replaced by CP/CC.
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.
It's OK from alsa-lib POV; although the uapi header change isn't 100% safe, the user of this header is really our ones, so it's practically acceptable, I suppose.
Could you submit the changes for kernel, so that it can be merged in time? Basically alsa-lib is synced with kernel, so the kernel should be changed at first.
Ack, will do, thanks for the review.
participants (2)
-
Pierre-Louis Bossart -
Takashi Iwai