From mboxrd@z Thu Jan 1 00:00:00 1970 From: Krzysztof Kozlowski Subject: Re: [PATCH -next v2] pinctrl: samsung: Fix return value check in samsung_pinctrl_get_soc_data() Date: Tue, 21 Feb 2017 17:32:58 +0200 Message-ID: References: <20170125140321.23911-1-weiyj.lk@gmail.com> <20170205155849.4362-1-weiyj.lk@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: Received: from mail.kernel.org ([198.145.29.136]:38690 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752551AbdBUPdg (ORCPT ); Tue, 21 Feb 2017 10:33:36 -0500 In-Reply-To: Sender: linux-gpio-owner@vger.kernel.org List-Id: linux-gpio@vger.kernel.org To: Marek Szyprowski Cc: Linus Walleij , Wei Yongjun , Tomasz Figa , Sylwester Nawrocki , Wei Yongjun , "linux-arm-kernel@lists.infradead.org" , linux-samsung-soc , "linux-gpio@vger.kernel.org" , Andrzej Hajda , =?UTF-8?B?64yA7J246riw?= , =?UTF-8?B?7LWc7LCs7Jqw?= , =?UTF-8?B?6rmA7Iq57Jqw?= On Tue, Feb 21, 2017 at 3:49 PM, Marek Szyprowski wrote: > Hi All, > > On 2017-02-13 15:49, Linus Walleij wrote: >> >> On Sun, Feb 5, 2017 at 4:58 PM, Wei Yongjun wrote: >> >>> From: Wei Yongjun >>> >>> In case of error, the function devm_ioremap() returns NULL pointer not >>> ERR_PTR(). Fix by using devm_ioremap_resource instead of devm_ioremap. >>> >>> Fixes: 8b1bd11c1f8f ("pinctrl: samsung: Add the support the multiple >>> IORESOURCE_MEM for one pin-bank") >>> Signed-off-by: Wei Yongjun >>> --- >>> v1 -> v2: use devm_ioremap_resource instead of devm_ioremap >> >> Patch applied with Krzysztof's ACK. > > > Sadly this patch breaks support for IMEM pinctrl block on Exynos5433/TM2 > and it took us some time to find the source of the problem. > > devm_ioremap_resource() is not functionally a full equivalent of > devm_ioremap(). The problem here is that registers for IMEM and ALIVE > pin controllers are shared and both devices have <0x11090000 0x1000> > range in their reg property. devm_ioremap_resource() maps given > resource exclusively for the device, while devm_ioremap() allows > non-exclusive mappings. > > This patch has to be reverted asap. Damn, the additional request_mem_region() raised my concerns but I didn't compare it with actual DTS layout... Which brings us to two important lessons: 1. Do not accept untested code. 2. Do not fix two things at the same time. I vote for the revert as well + original version of patch (these two could be squashed) + a need of Tested-by. Otherwise an untested fix might not be a fix at all. Best regards, Krzysztof