From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f47.google.com ([74.125.82.47]:38484 "EHLO mail-wm0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752045AbcIBJsm (ORCPT ); Fri, 2 Sep 2016 05:48:42 -0400 Received: by mail-wm0-f47.google.com with SMTP id 1so22399522wmz.1 for ; Fri, 02 Sep 2016 02:48:41 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20160902052258.GA1601@katana> References: <20160824093438.2478-1-wsa+renesas@sang-engineering.com> <20160824093438.2478-3-wsa+renesas@sang-engineering.com> <20160902052258.GA1601@katana> From: Ulf Hansson Date: Fri, 2 Sep 2016 11:48:39 +0200 Message-ID: Subject: Re: [PATCH 2/2] mmc: host: sh_mobile_sdhi: don't populate unneeded functions To: Wolfram Sang Cc: Magnus Damm , Wolfram Sang , linux-mmc , Linux-Renesas , Geert Uytterhoeven , Simon Horman Content-Type: text/plain; charset=UTF-8 Sender: linux-renesas-soc-owner@vger.kernel.org List-ID: On 2 September 2016 at 07:23, Wolfram Sang wrote: > Magnus, > >> To my eye it looks like this patch might be adding a fix for a bug >> introduced by another patch. R-Car Gen2 and later are quite recent >> SoCs but I believe support for other mobile chips and earlier R-Car >> SoCs also also covered by the SDHI driver. Also there are theTMIO >> chips that share some software and are related but a bit different. So >> in my opinion we need to thread lightly to avoid breakage. > > I'm very much with you. This is the very much reason I introduced > TMIO_MMC_MIN_RCAR2 in 3d376fb2ea907f ("mmc: tmio/sdhi: introduce flag > for RCar 2+ specific features") in the first place to be able to > "protect" old hardware from new features. It was not done before! This > is also the reason why we have commits like a21553c9e0c236 ("mmc: > tmio/sdhi: distinguish between SCLKDIVEN and ILL_FUNC") documenting a > difference between some old TMIO variant and our current SDHI. I spent > quite some time finding old TMIO information somewhere for that. > >> My concern is what happened before this patch was applied. It looks >> like the callbacks were installed for all hardware types which makes >> me wonder because UHS/SDR support is only available for quite recent >> hardware. > > I didn't protect these callbacks before because I assumed they are only > called when SDR support is enabled via devicetree or platform data. > Which is not the case for all the old platforms. I sadly missed that > card_busy() is used when polling an SDIO card in case "broken-cd" is > used. That was a detail I overlooked, sorry. Given my work outlined > above I don't think one can deduce that I don't care about regressing > old hardware, though. > >> The ->card_busy() callback is not yet in mainline or -next. It was >> introduced in: >> 0157290 mmc: host: sh_mobile_sdhi: move card_busy from tmio to sdhi > > Not quite, it was introduced with 452e5eef6d311e ("mmc: tmio: Add UHS-I > mode support"). The commit you mentioned moved it from tmio*.c to > sdhi*.c > >> If this patch is fixing a recent commit then perhaps some patches >> should be squashed together to prevent bisection breakage or if a >> patch is already part of mainline then a "Fixes:" tag might be >> suitable. > > It can be argued that this tag could be added: > > Fixes: 452e5eef6d311e ("mmc: tmio: Add UHS-I mode support") > > I don't know how well it applies, though, because the code has been > modified a lot recently. But we can try. Please tell me if you would like me to drop any of the changes I queued up in this series. Of course, I can also amend the change log, just tell me. Kind regards Uffe