LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Kirill Marinushkin <k.marinushkin@gmail.com>
Cc: alsa-devel@alsa-project.org, Takashi Iwai <tiwai@suse.de>,
	Pan Xiuli <xiuli.pan@linux.intel.com>,
	linux-kernel@vger.kernel.org,
	Liam Girdwood <liam.r.girdwood@linux.intel.com>,
	Mark Brown <broonie@kernel.org>
Subject: Re: [alsa-devel] [PATCH v3 0/2] ASoC: topology: Improve parsing hw_configs
Date: Tue, 3 Apr 2018 12:21:39 -0500	[thread overview]
Message-ID: <260f1850-37f2-fffa-fb33-4b5ff823ba0b@linux.intel.com> (raw)
In-Reply-To: <ee6ba140-fa5c-78e9-47bc-125b1aca7672@gmail.com>



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

  reply	other threads:[~2018-04-03 17:21 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-27 20:56 Kirill Marinushkin
2018-03-27 20:56 ` [PATCH v3 1/2] ASoC: topology: Fix bclk and fsync inversion in set_link_hw_format() Kirill Marinushkin
2018-03-27 20:56 ` [PATCH v3 2/2] ASoC: topology: Add missing clock gating parameter when parsing hw_configs Kirill Marinushkin
2018-04-16 17:15   ` Applied "ASoC: topology: Add missing clock gating parameter when parsing hw_configs" to the asoc tree Mark Brown
2018-03-27 21:00 ` [PATCH, alsa-lib, v3 0/2] alsa-lib: ASoC: topology: Improve parsing hw_configs Kirill Marinushkin
2018-03-27 21:00   ` [PATCH, alsa-lib, v3 1/2] ASoC: topology: Fix bclk and fsync inversion in set_link_hw_format() Kirill Marinushkin
2018-03-27 21:00   ` [PATCH, alsa-lib, v3 2/2] ASoC: topology: Add missing clock gating parameter when parsing hw_configs Kirill Marinushkin
2018-04-02 21:17 ` [PATCH v3 0/2] ASoC: topology: Improve " Kirill Marinushkin
2018-04-03  0:57   ` Pierre-Louis Bossart
2018-04-03  5:15     ` Kirill Marinushkin
2018-04-03 17:21       ` Pierre-Louis Bossart [this message]
2018-04-03 18:16         ` [alsa-devel] " Kirill Marinushkin
2018-04-03 19:19           ` Pierre-Louis Bossart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=260f1850-37f2-fffa-fb33-4b5ff823ba0b@linux.intel.com \
    --to=pierre-louis.bossart@linux.intel.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=k.marinushkin@gmail.com \
    --cc=liam.r.girdwood@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tiwai@suse.de \
    --cc=xiuli.pan@linux.intel.com \
    --subject='Re: [alsa-devel] [PATCH v3 0/2] ASoC: topology: Improve parsing hw_configs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).