Re: [PATCH -next v2] pinctrl: samsung: Fix return value check in samsung_pinctrl_get_soc_data()

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On Tue, Feb 21, 2017 at 3:49 PM, Marek Szyprowski
<m.szyprowski@xxxxxxxxxxx> wrote:
> Hi All,
>
> On 2017-02-13 15:49, Linus Walleij wrote:
>>
>> On Sun, Feb 5, 2017 at 4:58 PM, Wei Yongjun <weiyj.lk@xxxxxxxxx> wrote:
>>
>>> From: Wei Yongjun <weiyongjun1@xxxxxxxxxx>
>>>
>>> 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 <weiyongjun1@xxxxxxxxxx>
>>> ---
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html