Re: [alsa-devel] [PATCH v3 0/2] ASoC: topology: Improve parsing hw_configs

From: Pierre-Louis Bossart
Date: Tue Apr 03 2018 - 13:21:52 EST




On 04/03/2018 12:15 AM, Kirill Marinushkin wrote:
On 04/03/18 02:57, Pierre-Louis Bossart wrote:

On 04/02/2018 04:17 PM, Kirill Marinushkin wrote:
Hello Pierre-Louis,

I explicitly clarified with Takashi: to have this patch series merged, we need a
tag "Reviewed-by" from you.
I am fine with the changes, but maybe while we are at it, we should clarify
what mclk_direction means?
That's a good idea to have it solved within this patch series.

ÂÂÂ __u8 mclk_direction;ÂÂÂ /* 0 for input, 1 for output */

This is really awful and might benefit for additional clarity using
codec-centric conventions.

I agree that having a clear naming will avoid confusion for future usage.
I see from the code, that this variable is ignored. So we have no technical
restriction on how to interpret this.
I suggest to do similar to what we did for bclk_master:

/* DAI mclk_direction */
#define SND_SOC_TPLG_MCLK_CO ÂÂÂÂÂÂ 0 /* for codec, mclk is output */
#define SND_SOC_TPLG_MCLK_CI ÂÂÂÂÂÂÂÂ 1 /* for codec, mclk is input */

We also had a discussion internally and can't figure out why the strings are
different from the fields in the structure, I feel it'd be simpler to align
config and code to avoid issues but keep existing notation for backwards
compatibility, e.g.

if (strcmp(id, "mclk_freq") == 0) || strcmp(id, "mclk_rate") == 0) {
ÂÂÂ ÂÂÂ if (snd_config_get_string(n, &val) < 0)
ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ return -EINVAL;

ÂÂÂ ÂÂÂ ÂÂÂ hw_cfg->mclk_rate = atoi(val);
ÂÂÂ ÂÂÂ ÂÂÂ continue;
}
I agree with this. I will also do the same (keeping backwards-compatibility) for:

"format" => "fmt"
"bclk" => "bclk_master"
"bclk_freq" => "bclk_rate"
"bclk_invert" => "invert_bclk"
"fsync" => "fsync_master"
"fsync_invert" => "invert_fsync"
"fsync_freq" => "fsync_rate"
"mclk_freq" => "mclk_rate"
"mclk" => "mclk_direction"
"pm_gate_clocks" => "clock_gated"

If you agree with both proposals, I will add patches to this patch series, and
re-send as patch v4.
Or can we handle it in a better way?
A v4 is fine with me.

These topology definitions appear in hindsight quite problematic, there are missing definitions and capabilities, e.g we have the ability to 'gate the clock' but without knowing which clock, and we have no ability to force the mclk/bclk/fsync on (be it on demand from a codec driver or on startup as a system requirement). And there is no real extension capability with a protocol version. The net effect is that people will have to create custom tokens and parsing for things that should be common...

Thanks
-Pierre