| Message ID | 20180227203541.17343-1-k.marinushkin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 27, 2018 at 09:35:41PM +0100, 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: You should include Jaroslav and Takashi when sending alsa-lib patches. > + /* For backwards capability, > + * "master" == "codec is slave" > + */ > + if (!strcmp(val, "master") || > + !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; Should we warn on use of "master" since it's being deprecated here (and might be confusing for users)?
On Wed, 28 Feb 2018 16:24:03 +0100, Mark Brown wrote: > > On Tue, Feb 27, 2018 at 09:35:41PM +0100, 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: > > You should include Jaroslav and Takashi when sending alsa-lib patches. Yep, please at the next time. (And, don't stick with an old thread, but refresh a new thread.) > > > + /* For backwards capability, > > + * "master" == "codec is slave" > > + */ > > + if (!strcmp(val, "master") || > > + !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; > > Should we warn on use of "master" since it's being deprecated here (and > might be confusing for users)? Maybe. OTOH, it looks fully backward-compatible, so far, so it's not that confusing yet. thanks, Takashi
On Wed, Feb 28, 2018 at 04:29:30PM +0100, Takashi Iwai wrote: > Mark Brown wrote: > > Should we warn on use of "master" since it's being deprecated here (and > > might be confusing for users)? > Maybe. OTOH, it looks fully backward-compatible, so far, so it's not > that confusing yet. Not a compatibility issue, yeah - more just that we've clearly already got people confused so pushing people to the more explicit name might avoid further issues down the line.
@Pierre-Louis > That looks acceptable to me, Xiuli can you test with with the matching change > in the SOF topology macros (we can keep the 'slave' in M4 files but expand to > 'codec_slave' in the .conf to avoid making this exception visible). I don't see any comments from Pan Xiuli in the whole mailing thread. Do we still wait for his feedback? @Mark >>> Should we warn on use of "master" since it's being deprecated here (and >>> might be confusing for users)? >> Maybe. OTOH, it looks fully backward-compatible, so far, so it's not that >> confusing yet. > Not a compatibility issue, yeah - more just that we've clearly already got people > confused so pushing people to the more explicit name might avoid further > issues down the line. Agree with that. I will add a warning and send it as a patch v2. @Takashi > Yep, please at the next time. (And, don't stick with an old thread, but refresh a > new thread.) Yes, I will do so. I want to do this clean, so let me clarify before I start: 1. I will start a new thread, with linux [patch v2] 2. I will send the [patch, alsa-lib v2] in-reply to that linux [patch v2] 3. I will add all currently involved people as CC Does it look correct? Best Regards, Kirill
On Wed, 28 Feb 2018 21:07:52 +0100, Kirill Marinushkin wrote: > > > Yep, please at the next time. (And, don't stick with an old thread, but refresh a > > new thread.) > > Yes, I will do so. I want to do this clean, so let me clarify before I start: > > 1. I will start a new thread, with linux [patch v2] > 2. I will send the [patch, alsa-lib v2] in-reply to that linux [patch v2] > 3. I will add all currently involved people as CC > > Does it look correct? Yes, should be OK. Takashi
On 3/1/2018 04:07, Kirill Marinushkin wrote: > @Pierre-Louis > >> That looks acceptable to me, Xiuli can you test with with the matching change >> in the SOF topology macros (we can keep the 'slave' in M4 files but expand to >> 'codec_slave' in the .conf to avoid making this exception visible). > I don't see any comments from Pan Xiuli in the whole mailing thread. > Do we still wait for his feedback? Sorry about the late reply, I have some wrong setting with my mail client and found all the mails related in the junk folder. Thanks Pierre to remind me. I will do the test with the SOF topology and sent patch later. The patches look good to me, we need to keep the old things work. The modify is simple but efficient. I will also send a reminder letter about these alsa-lib changes to our sof developer. > > @Mark > >>>> Should we warn on use of "master" since it's being deprecated here (and >>>> might be confusing for users)? >>> Maybe. OTOH, it looks fully backward-compatible, so far, so it's not that >>> confusing yet. >> Not a compatibility issue, yeah - more just that we've clearly already got people >> confused so pushing people to the more explicit name might avoid further >> issues down the line. > Agree with that. I will add a warning and send it as a patch v2. > > > @Takashi > >> Yep, please at the next time. (And, don't stick with an old thread, but refresh a >> new thread.) > Yes, I will do so. I want to do this clean, so let me clarify before I start: > > 1. I will start a new thread, with linux [patch v2] > 2. I will send the [patch, alsa-lib v2] in-reply to that linux [patch v2] > 3. I will add all currently involved people as CC > > Does it look correct? I will look forward to test with new patches. Thanks Xiuli > Best Regards, > Kirill
Hello Mark, Takashi, Xiuli, Pierre-Louis, The patch series v2 is available in the new mailing thread, subject "[PATCH v2] ASoC: topology: Fix bclk and fsync inversion in set_link_hw_format()". Best Regards, Kirill On 03/01/18 07:03, Pan, Xiuli wrote: > > > On 3/1/2018 04:07, Kirill Marinushkin wrote: >> @Pierre-Louis >> >>> That looks acceptable to me, Xiuli can you test with with the matching change >>> in the SOF topology macros (we can keep the 'slave' in M4 files but expand to >>> 'codec_slave' in the .conf to avoid making this exception visible). >> I don't see any comments from Pan Xiuli in the whole mailing thread. >> Do we still wait for his feedback? > > Sorry about the late reply, I have some wrong setting with my mail client and found all the mails related in the junk folder. > Thanks Pierre to remind me. I will do the test with the SOF topology and sent patch later. > The patches look good to me, we need to keep the old things work. The modify is simple but efficient. I will also send a reminder letter about these alsa-lib changes to our sof developer. > >> >> @Mark >> >>>>> Should we warn on use of "master" since it's being deprecated here (and >>>>> might be confusing for users)? >>>> Maybe. OTOH, it looks fully backward-compatible, so far, so it's not that >>>> confusing yet. >>> Not a compatibility issue, yeah - more just that we've clearly already got people >>> confused so pushing people to the more explicit name might avoid further >>> issues down the line. >> Agree with that. I will add a warning and send it as a patch v2. >> >> >> @Takashi >> >>> Yep, please at the next time. (And, don't stick with an old thread, but refresh a >>> new thread.) >> Yes, I will do so. I want to do this clean, so let me clarify before I start: >> >> 1. I will start a new thread, with linux [patch v2] >> 2. I will send the [patch, alsa-lib v2] in-reply to that linux [patch v2] >> 3. I will add all currently involved people as CC >> >> Does it look correct? > I will look forward to test with new patches. > > Thanks > Xiuli > >> Best Regards, >> Kirill >
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 */ + /* * 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/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..d5d5e5b3 100644 --- a/src/topology/pcm.c +++ b/src/topology/pcm.c @@ -1137,8 +1137,14 @@ 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; + /* For backwards capability, + * "master" == "codec is slave" + */ + if (!strcmp(val, "master") || + !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; } @@ -1163,8 +1169,14 @@ 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; + /* For backwards capability, + * "master" == "codec is slave" + */ + if (!strcmp(val, "master") || + !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; }
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: 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 ++++++++++++++-- src/conf/topology/broadwell/broadwell.conf | 4 ++-- src/topology/pcm.c | 20 ++++++++++++++++---- 3 files changed, 32 insertions(+), 8 deletions(-)