From patchwork Mon Dec 23 10:38:05 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takashi Iwai X-Patchwork-Id: 3396421 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 2F1BBC0D4A for ; Mon, 23 Dec 2013 10:38:43 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 4DADC204D2 for ; Mon, 23 Dec 2013 10:38:42 +0000 (UTC) Received: from casper.infradead.org (casper.infradead.org [85.118.1.10]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 308B2204D1 for ; Mon, 23 Dec 2013 10:38:41 +0000 (UTC) Received: from merlin.infradead.org ([2001:4978:20e::2]) by casper.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1Vv2uD-0003lT-GT; Mon, 23 Dec 2013 10:38:37 +0000 Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1Vv2uB-0005RG-0j; Mon, 23 Dec 2013 10:38:35 +0000 Received: from cantor2.suse.de ([195.135.220.15] helo=mx2.suse.de) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1Vv2u7-0005QK-BH for linux-arm-kernel@lists.infradead.org; Mon, 23 Dec 2013 10:38:32 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 9A7F4ABC6; Mon, 23 Dec 2013 10:38:05 +0000 (UTC) Date: Mon, 23 Dec 2013 11:38:05 +0100 Message-ID: From: Takashi Iwai To: broonie@kernel.org Subject: Re: [PATCH 02/11] ASoC: ab8500: Revert back to find_next_bit() In-Reply-To: References: <1387468508-12286-1-git-send-email-lee.jones@linaro.org> <1387468508-12286-3-git-send-email-lee.jones@linaro.org> <20131219164824.GA16145@lee--X1> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20131223_053831_672789_461FD484 X-CRM114-Status: GOOD ( 23.57 ) X-Spam-Score: -7.5 (-------) Cc: linus.walleij@linaro.org, alsa-devel@alsa-project.org, Lee Jones , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP At Thu, 19 Dec 2013 18:29:09 +0100, Takashi Iwai wrote: > > At Thu, 19 Dec 2013 16:48:24 +0000, > Lee Jones wrote: > > > > > > Commit '166a34d ASoC: ab8500: Fix invalid cast to long pointer' > > > > rather carelessly converts find_next_bit() to fls() (find last > > > > bit set), which are not the same. > > > > > > Does it break on the real machines? > > > > It does, that's how I found the bug. > > > > > fls() behaves differently from find_next_bit(), of course, but in this > > > case, it should work same in the end, since there are at most two > > > bits. > > > > Unfortunately it doesn't work at all. This patch brings audio back to > > a working state. It took me the best part of a day to track down the > > issue. :( > > Hmm, then isn't the original code rather buggy? > > Check the values set there by ffs(), fls() and the original > find_next_bit(), especially whether find_next_bit() gives a valid > value fitting with the mask bits. Meanwhile, it's not good to keep the broken code in the upstream, and the bug is obvious. Below is the fixed patch, applicable with git am scissors option. Lee, let me know if this still doesn't fix. thanks, Takashi -- 8< -- From: Takashi Iwai Subject: [PATCH] ASoC: ab8500: Back to first_find_bit() & co ffs() and fls() give the off-by-one value in comparison with find_*_bit() helpers, and find_first_bit() and find_next_bit() are more intuitive in the case of two bits. So, back to find_*_bit() but call them properly without invalid cast. This fixes the slot assignment regression introduced by commit 166a34d27fca. Signed-off-by: Takashi Iwai --- sound/soc/codecs/ab8500-codec.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c index 1ad92cbf0b24..cf53c1e2fabc 100644 --- a/sound/soc/codecs/ab8500-codec.c +++ b/sound/soc/codecs/ab8500-codec.c @@ -2242,6 +2242,7 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai, { struct snd_soc_codec *codec = dai->codec; unsigned int val, mask, slot, slots_active; + unsigned long tmp; mask = BIT(AB8500_DIGIFCONF2_IF0WL0) | BIT(AB8500_DIGIFCONF2_IF0WL1); @@ -2308,21 +2309,22 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai, dev_dbg(dai->codec->dev, "%s: Slots, active, TX: %d\n", __func__, slots_active); + tmp = tx_mask; switch (slots_active) { case 0: break; case 1: - slot = ffs(tx_mask); + slot = find_first_bit(&tmp, 32); snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, slot); break; case 2: - slot = ffs(tx_mask); + slot = find_first_bit(&tmp, 32); snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, slot); - slot = fls(tx_mask); + slot = find_next_bit(&tmp, 32, slot + 1); snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, slot); break; @@ -2349,22 +2351,23 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai, dev_dbg(dai->codec->dev, "%s: Slots, active, RX: %d\n", __func__, slots_active); + tmp = rx_mask; switch (slots_active) { case 0: break; case 1: - slot = ffs(rx_mask); + slot = find_first_bit(&tmp, 32); snd_soc_update_bits(codec, AB8500_ADSLOTSEL(slot), AB8500_MASK_SLOT(slot), AB8500_ADSLOTSELX_AD_OUT_TO_SLOT(AB8500_AD_OUT3, slot)); break; case 2: - slot = ffs(rx_mask); + slot = find_first_bit(&tmp, 32); snd_soc_update_bits(codec, AB8500_ADSLOTSEL(slot), AB8500_MASK_SLOT(slot), AB8500_ADSLOTSELX_AD_OUT_TO_SLOT(AB8500_AD_OUT3, slot)); - slot = fls(rx_mask); + slot = find_next_bit(&tmp, 32, slot + 1); snd_soc_update_bits(codec, AB8500_ADSLOTSEL(slot), AB8500_MASK_SLOT(slot),