From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 65460C388F7 for ; Fri, 13 Nov 2020 17:40:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 205AB20791 for ; Fri, 13 Nov 2020 17:40:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726087AbgKMRkx (ORCPT ); Fri, 13 Nov 2020 12:40:53 -0500 Received: from foss.arm.com ([217.140.110.172]:42758 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726070AbgKMRkw (ORCPT ); Fri, 13 Nov 2020 12:40:52 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6D6711042; Fri, 13 Nov 2020 09:40:51 -0800 (PST) Received: from [10.57.53.43] (unknown [10.57.53.43]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C6B8C3F718; Fri, 13 Nov 2020 09:40:42 -0800 (PST) Subject: Re: About devm_platform_ioremap_resource [Was: Re: [PATCH 01/32] pwm: sun4i: convert to devm_platform_ioremap_resource] To: Thierry Reding , =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= Cc: alexandre.belloni@bootlin.com, heiko@sntech.de, Yangtao Li , Linus Walleij , linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org, linux-riscv@lists.infradead.org, festevam@gmail.com, f.fainelli@gmail.com, shc_work@mail.ru, Bartosz Golaszewski , khilman@baylibre.com, wens@csie.org, jonathanh@nvidia.com, linux-rockchip@lists.infradead.org, ludovic.desroches@microchip.com, bcm-kernel-feedback-list@broadcom.com, linux-imx@nxp.com, slemieux.tyco@gmail.com, linux-pwm@vger.kernel.org, rjui@broadcom.com, s.hauer@pengutronix.de, mripard@kernel.org, vz@mleia.com, linux-mediatek@lists.infradead.org, linux-rpi-kernel@lists.infradead.org, paul.walmsley@sifive.com, matthias.bgg@gmail.com, linux-amlogic@lists.infradead.org, Lee Jones , Andy Shevchenko , linux-arm-kernel@lists.infradead.org, sbranden@broadcom.com, Greg Kroah-Hartman , nicolas.ferre@microchip.com, palmer@dabbelt.com, kernel@pengutronix.de, shawnguo@kernel.org, claudiu.beznea@microchip.com, nsaenzjulienne@suse.de References: <20191229080610.7597-1-tiny.windzz@gmail.com> <20201112161346.gp5nenuagx5wmwl2@pengutronix.de> <20201112190649.GA908613@ulmo> <20201112211429.kfyqzkmmchjo6pll@pengutronix.de> <20201113070343.lhcsbyvi5baxn3lq@pengutronix.de> <20201113161153.GB1408970@ulmo> From: Robin Murphy Message-ID: <6cddd32c-50eb-4399-02bc-d4377237134c@arm.com> Date: Fri, 13 Nov 2020 17:40:41 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Thunderbird/78.4.3 MIME-Version: 1.0 In-Reply-To: <20201113161153.GB1408970@ulmo> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-tegra@vger.kernel.org On 2020-11-13 16:11, Thierry Reding wrote: > On Fri, Nov 13, 2020 at 08:03:43AM +0100, Uwe Kleine-König wrote: >> Hello, >> >> [Added lkml and the people involved in commit 7945f929f1a7 >> ("drivers: provide devm_platform_ioremap_resource()") to Cc:. For the >> new readers: This is about patches making use of >> devm_platform_ioremap_resource() instead of open coding it. Full context >> at https://lore.kernel.org/r/20201112190649.GA908613@ulmo] >> >> On Thu, Nov 12, 2020 at 10:14:29PM +0100, Uwe Kleine-König wrote: >>> On Thu, Nov 12, 2020 at 08:06:49PM +0100, Thierry Reding wrote: >>>> I also think that it's overly narrow is scope, so you can't actually >>>> "blindly" use this helper and I've seen quite a few cases where this was >>>> unknowingly used for cases where it shouldn't have been used and then >>>> broke things (because some drivers must not do the request_mem_region() >>>> for example). >>> >>> You have a link to such an accident? >> >> I got a hint in private here: https://lore.kernel.org/r/1555670144-24220-1-git-send-email-aisheng.dong@nxp.com >> >> devm_platform_ioremap_resource() is platform_get_resource() + >> devm_ioremap_resource() and here it was used to replace >> platform_get_resource() + devm_ioremap(). >> >> IMHO the unlucky thing in this situation is that devm_ioremap_resource() >> and devm_ioremap() are different by more than just how they get the area >> to remap. (i.e. devm_ioremap_resource() also does >> devm_request_mem_region().) >> >> So the problem is not the added wrapper, but unclear semantics in the >> functions it uses. > > The semantics aren't unclear. It's just that the symbol name doesn't > spell out every detail that the function implements, which, frankly, no > function name ever does, at least not for anything beyond simple > instructional examples. That's what we have documentation for and why > people should read the documentation before they use a function and make > (potentially wrong) assumption about what it does. > >> In my eyes devm_ioremap() and >> devm_platform_ioremap_resource() should better be named >> devm_request_ioremap() and devm_platform_request_ioremap_resource() >> respectively. Is it worth to rename these for clearity? > > I think function names are always a compromise between giving you the > gist of what the implementation does and being short enough so it > doesn't become difficult to read or use. > > One of the reasons why I dislike the addition of helpers for every > common special case (like devm_platform_ioremap_resource()) is because > it doesn't (always) actually make things easier for developers and/or > maintainers. Replacing three lines of code with one is a minor > improvement, even though there may be many callsites and therefore in > the sum this being a fairly sizeable reduction. The flip side is that > now we've got an extra symbol with an unwieldy name that people need > to become familiar with, and then, like the link above shows, it doesn't > work in all cases, so you either need to fall back to the open-coded > version or you keep adding helpers until you've covered all cases. And > then we end up with a bunch of helpers that you actually have to go and > read the documentation for in order to find out which one exactly fits > your use-case. > > Without the helpers it's pretty simple to write, even if a little > repetitive: > > 1) get the resource you want to map > 2) request the resource > 3) map the resource > > 2) & 3) are very commonly done together, so it makes sense to have a > generic helper for them. If you look at the implementation, the > devm_ioremap_request() implementation does quite a bit of things in > addition to just requesting and remapping, and that's the reason why > that helper makes sense. > > For me personally, devm_platform_ioremap_resource() is just not adding > enough value to justify its existence. And then we get all these other > variants that operate on the resource name (_byname) and those which > remap write-combined (_wc). But don't we also need a _byname_wc() > variant for the combination? Where does it stop? Arguably the worst thing about devm_platform_ioremap_resource() is that it was apparently the gateway drug to a belief that devm_platform_get_and_ioremap_resource() is anything other than a hideous way to obfuscate an assignment... Robin. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 780EAC388F7 for ; Fri, 13 Nov 2020 17:41:12 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 095E220791 for ; Fri, 13 Nov 2020 17:41:11 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="amj14bUQ" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 095E220791 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=X8FVPnJD8qvRu4Yj/J+Td0/nm2CHrkQ99xWpXZ7has8=; b=amj14bUQaB2HgQzIB4dTqjmOf KL3cVxsjnZvRGGSej54rvwnBDqAY3ZLm96WwklTVHIe1H92AggY1r87IfeFeUnC5tPJGXn53Jrjuk repcNxMZwZd5TAvAZb+rhXl7PTbHRvPZUH6nlhp5WDz9tyn9JFOcPaISbMgXiCdg40UE4bpKMDrx1 OiYJ15xnVno2pWbAPo8Me/xfzvwCAbrG1u2zPfQuNgs6YtCb4NFjbvcM4ORCFiVJbtY0Y4Rxh6+o4 JEyOCeAxWfcD26Ca3DsA5moDq8ir+FYi2Bl+h3El9oF76viIep9pz8hn4bLlYmd1POSH/92x6IJtb jJPf7e6fw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdd4H-0006dh-CI; Fri, 13 Nov 2020 17:41:01 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdd4A-0006bX-IW; Fri, 13 Nov 2020 17:40:55 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6D6711042; Fri, 13 Nov 2020 09:40:51 -0800 (PST) Received: from [10.57.53.43] (unknown [10.57.53.43]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C6B8C3F718; Fri, 13 Nov 2020 09:40:42 -0800 (PST) Subject: Re: About devm_platform_ioremap_resource [Was: Re: [PATCH 01/32] pwm: sun4i: convert to devm_platform_ioremap_resource] To: Thierry Reding , =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= References: <20191229080610.7597-1-tiny.windzz@gmail.com> <20201112161346.gp5nenuagx5wmwl2@pengutronix.de> <20201112190649.GA908613@ulmo> <20201112211429.kfyqzkmmchjo6pll@pengutronix.de> <20201113070343.lhcsbyvi5baxn3lq@pengutronix.de> <20201113161153.GB1408970@ulmo> From: Robin Murphy Message-ID: <6cddd32c-50eb-4399-02bc-d4377237134c@arm.com> Date: Fri, 13 Nov 2020 17:40:41 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Thunderbird/78.4.3 MIME-Version: 1.0 In-Reply-To: <20201113161153.GB1408970@ulmo> Content-Language: en-GB X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201113_124054_715687_67EC4228 X-CRM114-Status: GOOD ( 30.25 ) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alexandre.belloni@bootlin.com, heiko@sntech.de, Yangtao Li , Linus Walleij , nicolas.ferre@microchip.com, matthias.bgg@gmail.com, linux-riscv@lists.infradead.org, festevam@gmail.com, f.fainelli@gmail.com, shc_work@mail.ru, Bartosz Golaszewski , khilman@baylibre.com, ludovic.desroches@microchip.com, jonathanh@nvidia.com, linux-rockchip@lists.infradead.org, wens@csie.org, bcm-kernel-feedback-list@broadcom.com, linux-imx@nxp.com, slemieux.tyco@gmail.com, linux-pwm@vger.kernel.org, rjui@broadcom.com, s.hauer@pengutronix.de, mripard@kernel.org, vz@mleia.com, linux-mediatek@lists.infradead.org, linux-rpi-kernel@lists.infradead.org, paul.walmsley@sifive.com, linux-tegra@vger.kernel.org, linux-amlogic@lists.infradead.org, Lee Jones , Andy Shevchenko , linux-arm-kernel@lists.infradead.org, sbranden@broadcom.com, Greg Kroah-Hartman , linux-kernel@vger.kernel.org, palmer@dabbelt.com, kernel@pengutronix.de, shawnguo@kernel.org, claudiu.beznea@microchip.com, nsaenzjulienne@suse.de Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="windows-1252"; Format="flowed" Sender: "linux-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org On 2020-11-13 16:11, Thierry Reding wrote: > On Fri, Nov 13, 2020 at 08:03:43AM +0100, Uwe Kleine-K=F6nig wrote: >> Hello, >> >> [Added lkml and the people involved in commit 7945f929f1a7 >> ("drivers: provide devm_platform_ioremap_resource()") to Cc:. For the >> new readers: This is about patches making use of >> devm_platform_ioremap_resource() instead of open coding it. Full context >> at https://lore.kernel.org/r/20201112190649.GA908613@ulmo] >> >> On Thu, Nov 12, 2020 at 10:14:29PM +0100, Uwe Kleine-K=F6nig wrote: >>> On Thu, Nov 12, 2020 at 08:06:49PM +0100, Thierry Reding wrote: >>>> I also think that it's overly narrow is scope, so you can't actually >>>> "blindly" use this helper and I've seen quite a few cases where this w= as >>>> unknowingly used for cases where it shouldn't have been used and then >>>> broke things (because some drivers must not do the request_mem_region() >>>> for example). >>> >>> You have a link to such an accident? >> >> I got a hint in private here: https://lore.kernel.org/r/1555670144-24220= -1-git-send-email-aisheng.dong@nxp.com >> >> devm_platform_ioremap_resource() is platform_get_resource() + >> devm_ioremap_resource() and here it was used to replace >> platform_get_resource() + devm_ioremap(). >> >> IMHO the unlucky thing in this situation is that devm_ioremap_resource() >> and devm_ioremap() are different by more than just how they get the area >> to remap. (i.e. devm_ioremap_resource() also does >> devm_request_mem_region().) >> >> So the problem is not the added wrapper, but unclear semantics in the >> functions it uses. > = > The semantics aren't unclear. It's just that the symbol name doesn't > spell out every detail that the function implements, which, frankly, no > function name ever does, at least not for anything beyond simple > instructional examples. That's what we have documentation for and why > people should read the documentation before they use a function and make > (potentially wrong) assumption about what it does. > = >> In my eyes devm_ioremap() and >> devm_platform_ioremap_resource() should better be named >> devm_request_ioremap() and devm_platform_request_ioremap_resource() >> respectively. Is it worth to rename these for clearity? > = > I think function names are always a compromise between giving you the > gist of what the implementation does and being short enough so it > doesn't become difficult to read or use. > = > One of the reasons why I dislike the addition of helpers for every > common special case (like devm_platform_ioremap_resource()) is because > it doesn't (always) actually make things easier for developers and/or > maintainers. Replacing three lines of code with one is a minor > improvement, even though there may be many callsites and therefore in > the sum this being a fairly sizeable reduction. The flip side is that > now we've got an extra symbol with an unwieldy name that people need > to become familiar with, and then, like the link above shows, it doesn't > work in all cases, so you either need to fall back to the open-coded > version or you keep adding helpers until you've covered all cases. And > then we end up with a bunch of helpers that you actually have to go and > read the documentation for in order to find out which one exactly fits > your use-case. > = > Without the helpers it's pretty simple to write, even if a little > repetitive: > = > 1) get the resource you want to map > 2) request the resource > 3) map the resource > = > 2) & 3) are very commonly done together, so it makes sense to have a > generic helper for them. If you look at the implementation, the > devm_ioremap_request() implementation does quite a bit of things in > addition to just requesting and remapping, and that's the reason why > that helper makes sense. > = > For me personally, devm_platform_ioremap_resource() is just not adding > enough value to justify its existence. And then we get all these other > variants that operate on the resource name (_byname) and those which > remap write-combined (_wc). But don't we also need a _byname_wc() > variant for the combination? Where does it stop? Arguably the worst thing about devm_platform_ioremap_resource() is that = it was apparently the gateway drug to a belief that = devm_platform_get_and_ioremap_resource() is anything other than a = hideous way to obfuscate an assignment... Robin. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 73885C55ABD for ; Fri, 13 Nov 2020 17:41:09 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id ED39722201 for ; Fri, 13 Nov 2020 17:41:08 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="07cY+N3O" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org ED39722201 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-rockchip-bounces+linux-rockchip=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=rPu2rS1QssQESEyYIYF3W7ibd5Vrv8oSrwWeXmA+uh4=; b=07cY+N3OL6YL/MOUgLrYUBY7C DMYaaO/wMqUMXIS8ZviXAoruP6aKywO85uyD4D8APuyjVrvtoSlXDWKqjsHg+y6dfjsswQyGj8Spw sR5K4c2WyyY7YYu/IjO5u4m5YsacidwGHY5uQa8wIsLntuf35jtB6/dS3PutNnTLkCjfwRnmtEF/k HD5F0MG7+SPPk5BYC2m6pUwAEoVceVvX/TlAF7f0wGaaZml4ksDk8d3EtzkCXV/+ua7Et0nWpTYi6 4Z1NDL8kzjT+v+KmyxN2agbUKurFrfZIXEZ2DxRBPTne7t3/o0eVjJe7mklrg/283n2ILqE49b0YK D754eHRfA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdd4I-0006eH-WC; Fri, 13 Nov 2020 17:41:03 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdd4A-0006bX-IW; Fri, 13 Nov 2020 17:40:55 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6D6711042; Fri, 13 Nov 2020 09:40:51 -0800 (PST) Received: from [10.57.53.43] (unknown [10.57.53.43]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C6B8C3F718; Fri, 13 Nov 2020 09:40:42 -0800 (PST) Subject: Re: About devm_platform_ioremap_resource [Was: Re: [PATCH 01/32] pwm: sun4i: convert to devm_platform_ioremap_resource] To: Thierry Reding , =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= References: <20191229080610.7597-1-tiny.windzz@gmail.com> <20201112161346.gp5nenuagx5wmwl2@pengutronix.de> <20201112190649.GA908613@ulmo> <20201112211429.kfyqzkmmchjo6pll@pengutronix.de> <20201113070343.lhcsbyvi5baxn3lq@pengutronix.de> <20201113161153.GB1408970@ulmo> From: Robin Murphy Message-ID: <6cddd32c-50eb-4399-02bc-d4377237134c@arm.com> Date: Fri, 13 Nov 2020 17:40:41 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Thunderbird/78.4.3 MIME-Version: 1.0 In-Reply-To: <20201113161153.GB1408970@ulmo> Content-Language: en-GB X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201113_124054_715687_67EC4228 X-CRM114-Status: GOOD ( 30.25 ) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alexandre.belloni@bootlin.com, heiko@sntech.de, Yangtao Li , Linus Walleij , nicolas.ferre@microchip.com, matthias.bgg@gmail.com, linux-riscv@lists.infradead.org, festevam@gmail.com, f.fainelli@gmail.com, shc_work@mail.ru, Bartosz Golaszewski , khilman@baylibre.com, ludovic.desroches@microchip.com, jonathanh@nvidia.com, linux-rockchip@lists.infradead.org, wens@csie.org, bcm-kernel-feedback-list@broadcom.com, linux-imx@nxp.com, slemieux.tyco@gmail.com, linux-pwm@vger.kernel.org, rjui@broadcom.com, s.hauer@pengutronix.de, mripard@kernel.org, vz@mleia.com, linux-mediatek@lists.infradead.org, linux-rpi-kernel@lists.infradead.org, paul.walmsley@sifive.com, linux-tegra@vger.kernel.org, linux-amlogic@lists.infradead.org, Lee Jones , Andy Shevchenko , linux-arm-kernel@lists.infradead.org, sbranden@broadcom.com, Greg Kroah-Hartman , linux-kernel@vger.kernel.org, palmer@dabbelt.com, kernel@pengutronix.de, shawnguo@kernel.org, claudiu.beznea@microchip.com, nsaenzjulienne@suse.de Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="windows-1252"; Format="flowed" Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+linux-rockchip=archiver.kernel.org@lists.infradead.org On 2020-11-13 16:11, Thierry Reding wrote: > On Fri, Nov 13, 2020 at 08:03:43AM +0100, Uwe Kleine-K=F6nig wrote: >> Hello, >> >> [Added lkml and the people involved in commit 7945f929f1a7 >> ("drivers: provide devm_platform_ioremap_resource()") to Cc:. For the >> new readers: This is about patches making use of >> devm_platform_ioremap_resource() instead of open coding it. Full context >> at https://lore.kernel.org/r/20201112190649.GA908613@ulmo] >> >> On Thu, Nov 12, 2020 at 10:14:29PM +0100, Uwe Kleine-K=F6nig wrote: >>> On Thu, Nov 12, 2020 at 08:06:49PM +0100, Thierry Reding wrote: >>>> I also think that it's overly narrow is scope, so you can't actually >>>> "blindly" use this helper and I've seen quite a few cases where this w= as >>>> unknowingly used for cases where it shouldn't have been used and then >>>> broke things (because some drivers must not do the request_mem_region() >>>> for example). >>> >>> You have a link to such an accident? >> >> I got a hint in private here: https://lore.kernel.org/r/1555670144-24220= -1-git-send-email-aisheng.dong@nxp.com >> >> devm_platform_ioremap_resource() is platform_get_resource() + >> devm_ioremap_resource() and here it was used to replace >> platform_get_resource() + devm_ioremap(). >> >> IMHO the unlucky thing in this situation is that devm_ioremap_resource() >> and devm_ioremap() are different by more than just how they get the area >> to remap. (i.e. devm_ioremap_resource() also does >> devm_request_mem_region().) >> >> So the problem is not the added wrapper, but unclear semantics in the >> functions it uses. > = > The semantics aren't unclear. It's just that the symbol name doesn't > spell out every detail that the function implements, which, frankly, no > function name ever does, at least not for anything beyond simple > instructional examples. That's what we have documentation for and why > people should read the documentation before they use a function and make > (potentially wrong) assumption about what it does. > = >> In my eyes devm_ioremap() and >> devm_platform_ioremap_resource() should better be named >> devm_request_ioremap() and devm_platform_request_ioremap_resource() >> respectively. Is it worth to rename these for clearity? > = > I think function names are always a compromise between giving you the > gist of what the implementation does and being short enough so it > doesn't become difficult to read or use. > = > One of the reasons why I dislike the addition of helpers for every > common special case (like devm_platform_ioremap_resource()) is because > it doesn't (always) actually make things easier for developers and/or > maintainers. Replacing three lines of code with one is a minor > improvement, even though there may be many callsites and therefore in > the sum this being a fairly sizeable reduction. The flip side is that > now we've got an extra symbol with an unwieldy name that people need > to become familiar with, and then, like the link above shows, it doesn't > work in all cases, so you either need to fall back to the open-coded > version or you keep adding helpers until you've covered all cases. And > then we end up with a bunch of helpers that you actually have to go and > read the documentation for in order to find out which one exactly fits > your use-case. > = > Without the helpers it's pretty simple to write, even if a little > repetitive: > = > 1) get the resource you want to map > 2) request the resource > 3) map the resource > = > 2) & 3) are very commonly done together, so it makes sense to have a > generic helper for them. If you look at the implementation, the > devm_ioremap_request() implementation does quite a bit of things in > addition to just requesting and remapping, and that's the reason why > that helper makes sense. > = > For me personally, devm_platform_ioremap_resource() is just not adding > enough value to justify its existence. And then we get all these other > variants that operate on the resource name (_byname) and those which > remap write-combined (_wc). But don't we also need a _byname_wc() > variant for the combination? Where does it stop? Arguably the worst thing about devm_platform_ioremap_resource() is that = it was apparently the gateway drug to a belief that = devm_platform_get_and_ioremap_resource() is anything other than a = hideous way to obfuscate an assignment... Robin. _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5BD14C61DD8 for ; Fri, 13 Nov 2020 17:41:08 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E0D1A20791 for ; Fri, 13 Nov 2020 17:41:07 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="I75XH9Rl" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E0D1A20791 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=zSQJnz/OHmRzyVj7tVz3feYWAHARpmOiRiImKVmJ6lA=; b=I75XH9RlEQiE/E0VYuPmZNxAp 5kKtrOD/eILIOPKcXWdKDBBHptLyTS0AD5Qg/1swmmr/uMo60X3wOwNqfCbZqbLbIq9HqqpHPpkh7 eDXOVaQCo3qxwUgH7SnqIAvhVnbX4Ja+yqDBkj4pCy5pG7QsmFeZfPvjfD8i0wdvBInTZWy4BLZb8 Bx9NtcKbuaztpddXWWVYlJ2Uo71t3k1pc8KXT5VdQayl/8DqaSFaM23teIq5Rs00cv0xQ9TY0ytOJ kJXHFCmw4d8gnJfEILPNx3qS424L2dryvAnx6a/ITFY+cUBhU00eISLv0Io1gr0L51XLehXRhNTRt EWfqmc0nA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdd4G-0006dT-8m; Fri, 13 Nov 2020 17:41:00 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdd4A-0006bX-IW; Fri, 13 Nov 2020 17:40:55 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6D6711042; Fri, 13 Nov 2020 09:40:51 -0800 (PST) Received: from [10.57.53.43] (unknown [10.57.53.43]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C6B8C3F718; Fri, 13 Nov 2020 09:40:42 -0800 (PST) Subject: Re: About devm_platform_ioremap_resource [Was: Re: [PATCH 01/32] pwm: sun4i: convert to devm_platform_ioremap_resource] To: Thierry Reding , =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= References: <20191229080610.7597-1-tiny.windzz@gmail.com> <20201112161346.gp5nenuagx5wmwl2@pengutronix.de> <20201112190649.GA908613@ulmo> <20201112211429.kfyqzkmmchjo6pll@pengutronix.de> <20201113070343.lhcsbyvi5baxn3lq@pengutronix.de> <20201113161153.GB1408970@ulmo> From: Robin Murphy Message-ID: <6cddd32c-50eb-4399-02bc-d4377237134c@arm.com> Date: Fri, 13 Nov 2020 17:40:41 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Thunderbird/78.4.3 MIME-Version: 1.0 In-Reply-To: <20201113161153.GB1408970@ulmo> Content-Language: en-GB X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201113_124054_715687_67EC4228 X-CRM114-Status: GOOD ( 30.25 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alexandre.belloni@bootlin.com, heiko@sntech.de, Yangtao Li , Linus Walleij , nicolas.ferre@microchip.com, matthias.bgg@gmail.com, linux-riscv@lists.infradead.org, festevam@gmail.com, f.fainelli@gmail.com, shc_work@mail.ru, Bartosz Golaszewski , khilman@baylibre.com, ludovic.desroches@microchip.com, jonathanh@nvidia.com, linux-rockchip@lists.infradead.org, wens@csie.org, bcm-kernel-feedback-list@broadcom.com, linux-imx@nxp.com, slemieux.tyco@gmail.com, linux-pwm@vger.kernel.org, rjui@broadcom.com, s.hauer@pengutronix.de, mripard@kernel.org, vz@mleia.com, linux-mediatek@lists.infradead.org, linux-rpi-kernel@lists.infradead.org, paul.walmsley@sifive.com, linux-tegra@vger.kernel.org, linux-amlogic@lists.infradead.org, Lee Jones , Andy Shevchenko , linux-arm-kernel@lists.infradead.org, sbranden@broadcom.com, Greg Kroah-Hartman , linux-kernel@vger.kernel.org, palmer@dabbelt.com, kernel@pengutronix.de, shawnguo@kernel.org, claudiu.beznea@microchip.com, nsaenzjulienne@suse.de Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="windows-1252"; Format="flowed" Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org On 2020-11-13 16:11, Thierry Reding wrote: > On Fri, Nov 13, 2020 at 08:03:43AM +0100, Uwe Kleine-K=F6nig wrote: >> Hello, >> >> [Added lkml and the people involved in commit 7945f929f1a7 >> ("drivers: provide devm_platform_ioremap_resource()") to Cc:. For the >> new readers: This is about patches making use of >> devm_platform_ioremap_resource() instead of open coding it. Full context >> at https://lore.kernel.org/r/20201112190649.GA908613@ulmo] >> >> On Thu, Nov 12, 2020 at 10:14:29PM +0100, Uwe Kleine-K=F6nig wrote: >>> On Thu, Nov 12, 2020 at 08:06:49PM +0100, Thierry Reding wrote: >>>> I also think that it's overly narrow is scope, so you can't actually >>>> "blindly" use this helper and I've seen quite a few cases where this w= as >>>> unknowingly used for cases where it shouldn't have been used and then >>>> broke things (because some drivers must not do the request_mem_region() >>>> for example). >>> >>> You have a link to such an accident? >> >> I got a hint in private here: https://lore.kernel.org/r/1555670144-24220= -1-git-send-email-aisheng.dong@nxp.com >> >> devm_platform_ioremap_resource() is platform_get_resource() + >> devm_ioremap_resource() and here it was used to replace >> platform_get_resource() + devm_ioremap(). >> >> IMHO the unlucky thing in this situation is that devm_ioremap_resource() >> and devm_ioremap() are different by more than just how they get the area >> to remap. (i.e. devm_ioremap_resource() also does >> devm_request_mem_region().) >> >> So the problem is not the added wrapper, but unclear semantics in the >> functions it uses. > = > The semantics aren't unclear. It's just that the symbol name doesn't > spell out every detail that the function implements, which, frankly, no > function name ever does, at least not for anything beyond simple > instructional examples. That's what we have documentation for and why > people should read the documentation before they use a function and make > (potentially wrong) assumption about what it does. > = >> In my eyes devm_ioremap() and >> devm_platform_ioremap_resource() should better be named >> devm_request_ioremap() and devm_platform_request_ioremap_resource() >> respectively. Is it worth to rename these for clearity? > = > I think function names are always a compromise between giving you the > gist of what the implementation does and being short enough so it > doesn't become difficult to read or use. > = > One of the reasons why I dislike the addition of helpers for every > common special case (like devm_platform_ioremap_resource()) is because > it doesn't (always) actually make things easier for developers and/or > maintainers. Replacing three lines of code with one is a minor > improvement, even though there may be many callsites and therefore in > the sum this being a fairly sizeable reduction. The flip side is that > now we've got an extra symbol with an unwieldy name that people need > to become familiar with, and then, like the link above shows, it doesn't > work in all cases, so you either need to fall back to the open-coded > version or you keep adding helpers until you've covered all cases. And > then we end up with a bunch of helpers that you actually have to go and > read the documentation for in order to find out which one exactly fits > your use-case. > = > Without the helpers it's pretty simple to write, even if a little > repetitive: > = > 1) get the resource you want to map > 2) request the resource > 3) map the resource > = > 2) & 3) are very commonly done together, so it makes sense to have a > generic helper for them. If you look at the implementation, the > devm_ioremap_request() implementation does quite a bit of things in > addition to just requesting and remapping, and that's the reason why > that helper makes sense. > = > For me personally, devm_platform_ioremap_resource() is just not adding > enough value to justify its existence. And then we get all these other > variants that operate on the resource name (_byname) and those which > remap write-combined (_wc). But don't we also need a _byname_wc() > variant for the combination? Where does it stop? Arguably the worst thing about devm_platform_ioremap_resource() is that = it was apparently the gateway drug to a belief that = devm_platform_get_and_ioremap_resource() is anything other than a = hideous way to obfuscate an assignment... Robin. _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6EC28C388F7 for ; Fri, 13 Nov 2020 17:42:16 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id DB0BD221EB for ; Fri, 13 Nov 2020 17:42:15 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="CTo2zi7/" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DB0BD221EB Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=ljXr0qr5maflnuvPguWlHHdfI63j0JI56TAozyniKG8=; b=CTo2zi7/r4KynBu9L45hTIYdz bGyjXm7eBOy8RLT6ZhnWqmTQWQlP0p8/nNkg35TGTsM3W/HGJzzlrRNYZxVTKVaqysbrzZsVCY3I8 aKOtUxUHBtoqX1tbVmp8mbn+cpN05fjL8uqe9OM1z02i1JwpYS+GPmZ6jXRjw9Ixgxx8/ENXxSkJF PKprLFC6W4s/5a0Y+MAox1MBsb8TAR+Co2rYAy+61S8IXxhxmpDZhB7SzJ95c8MJiYcytAAcZxwNR h1TiF2RyGvsvvo2rZ++nJe/uHBa1V51WfEWNSnlRmcHDaG+f0xhBLHwLBmf9ww1bmWpgYaNrkVogn 5UN3abfug==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdd4E-0006cr-Hq; Fri, 13 Nov 2020 17:40:58 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdd4A-0006bX-IW; Fri, 13 Nov 2020 17:40:55 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6D6711042; Fri, 13 Nov 2020 09:40:51 -0800 (PST) Received: from [10.57.53.43] (unknown [10.57.53.43]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C6B8C3F718; Fri, 13 Nov 2020 09:40:42 -0800 (PST) Subject: Re: About devm_platform_ioremap_resource [Was: Re: [PATCH 01/32] pwm: sun4i: convert to devm_platform_ioremap_resource] To: Thierry Reding , =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= References: <20191229080610.7597-1-tiny.windzz@gmail.com> <20201112161346.gp5nenuagx5wmwl2@pengutronix.de> <20201112190649.GA908613@ulmo> <20201112211429.kfyqzkmmchjo6pll@pengutronix.de> <20201113070343.lhcsbyvi5baxn3lq@pengutronix.de> <20201113161153.GB1408970@ulmo> From: Robin Murphy Message-ID: <6cddd32c-50eb-4399-02bc-d4377237134c@arm.com> Date: Fri, 13 Nov 2020 17:40:41 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Thunderbird/78.4.3 MIME-Version: 1.0 In-Reply-To: <20201113161153.GB1408970@ulmo> Content-Language: en-GB X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201113_124054_715687_67EC4228 X-CRM114-Status: GOOD ( 30.25 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alexandre.belloni@bootlin.com, heiko@sntech.de, Yangtao Li , Linus Walleij , matthias.bgg@gmail.com, linux-riscv@lists.infradead.org, festevam@gmail.com, f.fainelli@gmail.com, shc_work@mail.ru, Bartosz Golaszewski , khilman@baylibre.com, ludovic.desroches@microchip.com, jonathanh@nvidia.com, linux-rockchip@lists.infradead.org, wens@csie.org, bcm-kernel-feedback-list@broadcom.com, linux-imx@nxp.com, slemieux.tyco@gmail.com, linux-pwm@vger.kernel.org, rjui@broadcom.com, s.hauer@pengutronix.de, mripard@kernel.org, vz@mleia.com, linux-mediatek@lists.infradead.org, linux-rpi-kernel@lists.infradead.org, paul.walmsley@sifive.com, linux-tegra@vger.kernel.org, linux-amlogic@lists.infradead.org, Lee Jones , Andy Shevchenko , linux-arm-kernel@lists.infradead.org, sbranden@broadcom.com, Greg Kroah-Hartman , linux-kernel@vger.kernel.org, palmer@dabbelt.com, kernel@pengutronix.de, shawnguo@kernel.org, claudiu.beznea@microchip.com, nsaenzjulienne@suse.de Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="windows-1252"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 2020-11-13 16:11, Thierry Reding wrote: > On Fri, Nov 13, 2020 at 08:03:43AM +0100, Uwe Kleine-K=F6nig wrote: >> Hello, >> >> [Added lkml and the people involved in commit 7945f929f1a7 >> ("drivers: provide devm_platform_ioremap_resource()") to Cc:. For the >> new readers: This is about patches making use of >> devm_platform_ioremap_resource() instead of open coding it. Full context >> at https://lore.kernel.org/r/20201112190649.GA908613@ulmo] >> >> On Thu, Nov 12, 2020 at 10:14:29PM +0100, Uwe Kleine-K=F6nig wrote: >>> On Thu, Nov 12, 2020 at 08:06:49PM +0100, Thierry Reding wrote: >>>> I also think that it's overly narrow is scope, so you can't actually >>>> "blindly" use this helper and I've seen quite a few cases where this w= as >>>> unknowingly used for cases where it shouldn't have been used and then >>>> broke things (because some drivers must not do the request_mem_region() >>>> for example). >>> >>> You have a link to such an accident? >> >> I got a hint in private here: https://lore.kernel.org/r/1555670144-24220= -1-git-send-email-aisheng.dong@nxp.com >> >> devm_platform_ioremap_resource() is platform_get_resource() + >> devm_ioremap_resource() and here it was used to replace >> platform_get_resource() + devm_ioremap(). >> >> IMHO the unlucky thing in this situation is that devm_ioremap_resource() >> and devm_ioremap() are different by more than just how they get the area >> to remap. (i.e. devm_ioremap_resource() also does >> devm_request_mem_region().) >> >> So the problem is not the added wrapper, but unclear semantics in the >> functions it uses. > = > The semantics aren't unclear. It's just that the symbol name doesn't > spell out every detail that the function implements, which, frankly, no > function name ever does, at least not for anything beyond simple > instructional examples. That's what we have documentation for and why > people should read the documentation before they use a function and make > (potentially wrong) assumption about what it does. > = >> In my eyes devm_ioremap() and >> devm_platform_ioremap_resource() should better be named >> devm_request_ioremap() and devm_platform_request_ioremap_resource() >> respectively. Is it worth to rename these for clearity? > = > I think function names are always a compromise between giving you the > gist of what the implementation does and being short enough so it > doesn't become difficult to read or use. > = > One of the reasons why I dislike the addition of helpers for every > common special case (like devm_platform_ioremap_resource()) is because > it doesn't (always) actually make things easier for developers and/or > maintainers. Replacing three lines of code with one is a minor > improvement, even though there may be many callsites and therefore in > the sum this being a fairly sizeable reduction. The flip side is that > now we've got an extra symbol with an unwieldy name that people need > to become familiar with, and then, like the link above shows, it doesn't > work in all cases, so you either need to fall back to the open-coded > version or you keep adding helpers until you've covered all cases. And > then we end up with a bunch of helpers that you actually have to go and > read the documentation for in order to find out which one exactly fits > your use-case. > = > Without the helpers it's pretty simple to write, even if a little > repetitive: > = > 1) get the resource you want to map > 2) request the resource > 3) map the resource > = > 2) & 3) are very commonly done together, so it makes sense to have a > generic helper for them. If you look at the implementation, the > devm_ioremap_request() implementation does quite a bit of things in > addition to just requesting and remapping, and that's the reason why > that helper makes sense. > = > For me personally, devm_platform_ioremap_resource() is just not adding > enough value to justify its existence. And then we get all these other > variants that operate on the resource name (_byname) and those which > remap write-combined (_wc). But don't we also need a _byname_wc() > variant for the combination? Where does it stop? Arguably the worst thing about devm_platform_ioremap_resource() is that = it was apparently the gateway drug to a belief that = devm_platform_get_and_ioremap_resource() is anything other than a = hideous way to obfuscate an assignment... Robin. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 13BB3C55ABD for ; Fri, 13 Nov 2020 17:41:07 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A4E7620791 for ; Fri, 13 Nov 2020 17:41:06 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="KUQe+6B/" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A4E7620791 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-amlogic-bounces+linux-amlogic=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=8L3qf5gj9fW4XhNVRneR3SQp1V1EgTdxLwnWqygXRAM=; b=KUQe+6B/OgHW8zwBs306QXfyX kn7nrztMmfnvtwMig9aOBy0m+k5Ai7R6owZHygMx20D0yVcavRg/sGm0fv6HJtrJvuizelubqEZvO 1PMX6KCQIlvwjXVy+vtxfcEP4hU+J+58hFFz9sAa20LQ3nJ9tACE8dMP5KwwWKQXU3q6/XyIanho/ /apnC7w81yDc/KH+7+A9MB7DbpdQEI+oiF+A7QYMTx+AvoIgDAPsPUijXnB+LM8PEXUr87uU+ziPU KfNZhNWzUcterdrZ/Mt7lT/TE5HFfoKzp5XeOTJub4xEt36n1Sv0Nqge84umqlB9sCRrp0FrEiaFt +H5Z0JSbg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdd4D-0006cQ-3i; Fri, 13 Nov 2020 17:40:57 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kdd4A-0006bX-IW; Fri, 13 Nov 2020 17:40:55 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6D6711042; Fri, 13 Nov 2020 09:40:51 -0800 (PST) Received: from [10.57.53.43] (unknown [10.57.53.43]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C6B8C3F718; Fri, 13 Nov 2020 09:40:42 -0800 (PST) Subject: Re: About devm_platform_ioremap_resource [Was: Re: [PATCH 01/32] pwm: sun4i: convert to devm_platform_ioremap_resource] To: Thierry Reding , =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= References: <20191229080610.7597-1-tiny.windzz@gmail.com> <20201112161346.gp5nenuagx5wmwl2@pengutronix.de> <20201112190649.GA908613@ulmo> <20201112211429.kfyqzkmmchjo6pll@pengutronix.de> <20201113070343.lhcsbyvi5baxn3lq@pengutronix.de> <20201113161153.GB1408970@ulmo> From: Robin Murphy Message-ID: <6cddd32c-50eb-4399-02bc-d4377237134c@arm.com> Date: Fri, 13 Nov 2020 17:40:41 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Thunderbird/78.4.3 MIME-Version: 1.0 In-Reply-To: <20201113161153.GB1408970@ulmo> Content-Language: en-GB X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201113_124054_715687_67EC4228 X-CRM114-Status: GOOD ( 30.25 ) X-BeenThere: linux-amlogic@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alexandre.belloni@bootlin.com, heiko@sntech.de, Yangtao Li , Linus Walleij , nicolas.ferre@microchip.com, matthias.bgg@gmail.com, linux-riscv@lists.infradead.org, festevam@gmail.com, f.fainelli@gmail.com, shc_work@mail.ru, Bartosz Golaszewski , khilman@baylibre.com, ludovic.desroches@microchip.com, jonathanh@nvidia.com, linux-rockchip@lists.infradead.org, wens@csie.org, bcm-kernel-feedback-list@broadcom.com, linux-imx@nxp.com, slemieux.tyco@gmail.com, linux-pwm@vger.kernel.org, rjui@broadcom.com, s.hauer@pengutronix.de, mripard@kernel.org, vz@mleia.com, linux-mediatek@lists.infradead.org, linux-rpi-kernel@lists.infradead.org, paul.walmsley@sifive.com, linux-tegra@vger.kernel.org, linux-amlogic@lists.infradead.org, Lee Jones , Andy Shevchenko , linux-arm-kernel@lists.infradead.org, sbranden@broadcom.com, Greg Kroah-Hartman , linux-kernel@vger.kernel.org, palmer@dabbelt.com, kernel@pengutronix.de, shawnguo@kernel.org, claudiu.beznea@microchip.com, nsaenzjulienne@suse.de Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="windows-1252"; Format="flowed" Sender: "linux-amlogic" Errors-To: linux-amlogic-bounces+linux-amlogic=archiver.kernel.org@lists.infradead.org On 2020-11-13 16:11, Thierry Reding wrote: > On Fri, Nov 13, 2020 at 08:03:43AM +0100, Uwe Kleine-K=F6nig wrote: >> Hello, >> >> [Added lkml and the people involved in commit 7945f929f1a7 >> ("drivers: provide devm_platform_ioremap_resource()") to Cc:. For the >> new readers: This is about patches making use of >> devm_platform_ioremap_resource() instead of open coding it. Full context >> at https://lore.kernel.org/r/20201112190649.GA908613@ulmo] >> >> On Thu, Nov 12, 2020 at 10:14:29PM +0100, Uwe Kleine-K=F6nig wrote: >>> On Thu, Nov 12, 2020 at 08:06:49PM +0100, Thierry Reding wrote: >>>> I also think that it's overly narrow is scope, so you can't actually >>>> "blindly" use this helper and I've seen quite a few cases where this w= as >>>> unknowingly used for cases where it shouldn't have been used and then >>>> broke things (because some drivers must not do the request_mem_region() >>>> for example). >>> >>> You have a link to such an accident? >> >> I got a hint in private here: https://lore.kernel.org/r/1555670144-24220= -1-git-send-email-aisheng.dong@nxp.com >> >> devm_platform_ioremap_resource() is platform_get_resource() + >> devm_ioremap_resource() and here it was used to replace >> platform_get_resource() + devm_ioremap(). >> >> IMHO the unlucky thing in this situation is that devm_ioremap_resource() >> and devm_ioremap() are different by more than just how they get the area >> to remap. (i.e. devm_ioremap_resource() also does >> devm_request_mem_region().) >> >> So the problem is not the added wrapper, but unclear semantics in the >> functions it uses. > = > The semantics aren't unclear. It's just that the symbol name doesn't > spell out every detail that the function implements, which, frankly, no > function name ever does, at least not for anything beyond simple > instructional examples. That's what we have documentation for and why > people should read the documentation before they use a function and make > (potentially wrong) assumption about what it does. > = >> In my eyes devm_ioremap() and >> devm_platform_ioremap_resource() should better be named >> devm_request_ioremap() and devm_platform_request_ioremap_resource() >> respectively. Is it worth to rename these for clearity? > = > I think function names are always a compromise between giving you the > gist of what the implementation does and being short enough so it > doesn't become difficult to read or use. > = > One of the reasons why I dislike the addition of helpers for every > common special case (like devm_platform_ioremap_resource()) is because > it doesn't (always) actually make things easier for developers and/or > maintainers. Replacing three lines of code with one is a minor > improvement, even though there may be many callsites and therefore in > the sum this being a fairly sizeable reduction. The flip side is that > now we've got an extra symbol with an unwieldy name that people need > to become familiar with, and then, like the link above shows, it doesn't > work in all cases, so you either need to fall back to the open-coded > version or you keep adding helpers until you've covered all cases. And > then we end up with a bunch of helpers that you actually have to go and > read the documentation for in order to find out which one exactly fits > your use-case. > = > Without the helpers it's pretty simple to write, even if a little > repetitive: > = > 1) get the resource you want to map > 2) request the resource > 3) map the resource > = > 2) & 3) are very commonly done together, so it makes sense to have a > generic helper for them. If you look at the implementation, the > devm_ioremap_request() implementation does quite a bit of things in > addition to just requesting and remapping, and that's the reason why > that helper makes sense. > = > For me personally, devm_platform_ioremap_resource() is just not adding > enough value to justify its existence. And then we get all these other > variants that operate on the resource name (_byname) and those which > remap write-combined (_wc). But don't we also need a _byname_wc() > variant for the combination? Where does it stop? Arguably the worst thing about devm_platform_ioremap_resource() is that = it was apparently the gateway drug to a belief that = devm_platform_get_and_ioremap_resource() is anything other than a = hideous way to obfuscate an assignment... Robin. _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic