From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH v2 09/10] ASoC: topology: Change stream formats to bitwise flag Date: Sat, 15 Aug 2015 09:37:34 +0200 Message-ID: References: <20150814203428.GR10748@sirena.org.uk> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 23E7E26514B for ; Sat, 15 Aug 2015 09:37:36 +0200 (CEST) In-Reply-To: <20150814203428.GR10748@sirena.org.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Mark Brown Cc: mengdong.lin@intel.com, alsa-devel@alsa-project.org, liam.r.girdwood@intel.com List-Id: alsa-devel@alsa-project.org On Fri, 14 Aug 2015 22:34:28 +0200, Mark Brown wrote: > > On Mon, Aug 10, 2015 at 10:48:37PM +0800, mengdong.lin@intel.com wrote: > > > struct snd_soc_tplg_stream_caps { > > __le32 size; /* in bytes of this structure */ > > char name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN]; > > - __le64 formats[SND_SOC_TPLG_MAX_FORMATS]; /* supported formats SNDRV_PCM_FMTBIT_* */ > > + __le64 formats; /* supported formats SNDRV_PCM_FORMAT_* */ > > Argh, this is *another* ABI change which you've buried in the middle of > a series you're sending right at the end of the release cycle (this was > sent after -rc6, we may not even get a -rc7...). Given how close we are > to the release and the invasiveness of the changes in this series it's > very lucky I even saw it before release, I'm reading this on my flight > to Plumbers which means my bandwidth next week will be limited. Thanks for checking this. Mengdong, if there is a change in uapi/*, this means touching ABI. So, this change isn't acceptable once when the kernel is released. At least, the backward compatibility *must* be kept no matter which version is. Also, the patch (and even the current code) looks wrong. The current formats[] would receive SNDRV_PCM_FORMAT_* while the comment shows SNDRV_PCM_FMT_BIT_*. And your patch changes it again wrongly in the comment. Quite confusing. > This should have at the very least been sent at the start of the series, > and really should have been sent separately. It is also worrying that > there is nothing in the subject or changelog talking about the fact that > this is an ABI change or explaining why it is required. The changelog > was simply a statement of the content of the diff, not a rationale: > > | The toplogy user space tool will generate this bitwise flag by using > | SNDRV_PCM_FORMAT_* exposed by asound.h, and the topology core will copy > | this flag when generating DAI streams. > > Once the release goes out this sort of incompatible change will be > unacceptable. Given the number of changes that are going into the ABI > (we also had some others went in last week) I'm now giving some serious > thought to the idea that we should ask Linus to revert the topology code > for v4.2 and waiting for v4.3 so we've got more chance to make sure the > ABI is one we actually want to stick with. Liam, Takashi I don't know > what you think? We can make the topology stuff disabled in Makefile (and the only call in soc-core.c) instead of the whole revert. This will be the minimal impact and safe enough. BTW, I wonder why there is no Kconfig for topology stuff. If we had it, it would have been easy to disable. thanks, Takashi