From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752515AbeDCT1D (ORCPT ); Tue, 3 Apr 2018 15:27:03 -0400 Received: from mga11.intel.com ([192.55.52.93]:59475 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751179AbeDCT1C (ORCPT ); Tue, 3 Apr 2018 15:27:02 -0400 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,402,1517904000"; d="scan'208";a="33965909" Subject: Re: [alsa-devel] [PATCH v3 0/2] ASoC: topology: Improve parsing hw_configs To: Kirill Marinushkin Cc: alsa-devel@alsa-project.org, Takashi Iwai , Pan Xiuli , linux-kernel@vger.kernel.org, Liam Girdwood , Mark Brown References: <20180327205632.3677-1-k.marinushkin@gmail.com> <17f0767a-2dea-92dd-a88e-a3f3e670d0d2@gmail.com> <461bbabf-7c36-1361-8f66-5abbe3c7f4d8@linux.intel.com> <260f1850-37f2-fffa-fb33-4b5ff823ba0b@linux.intel.com> <21dd211e-2b93-b5af-0606-726432e40795@gmail.com> From: Pierre-Louis Bossart Message-ID: Date: Tue, 3 Apr 2018 14:19:12 -0500 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <21dd211e-2b93-b5af-0606-726432e40795@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 4/3/18 1:16 PM, Kirill Marinushkin wrote: > On 04/03/18 19:21, Pierre-Louis Bossart wrote: >> >> >> 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... >> > > Yes, definitions which you mentioned really can become a problem. > But, I see from the header that topology files support versioning: > > ~~~~ > #define SND_SOC_TPLG_ABI_VERSION    0x5    /* current version */ > ~~~~ > > So, in future such problems can be solved by incrementing the version, if no > backwards capabilities are available. Maybe I misunderstood this version number, I thought this was only for the initial stages where there was quite a few evolutions in a couple of months. If this can be upgraded then we might want to add new definitions that we came up with during the SOF work but are of interest to others. > > > > > Before I continue with the patch v4, I want to clarify with you, so that we > avoid the misunderstanding: > > * in the existing 4 patches, I will add a tag >   "Reviewed-by: Pierre-Louis Bossart " > * in the new 2 patches, which we recently discussed, I will add a tag >   "Acked-by: Pierre-Louis Bossart " > > Do you agree with that? Yes, that's fine.