Linux-Amlogic Archive on lore.kernel.org
 help / color / Atom feed
From: Thierry Reding <thierry.reding@gmail.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: alexandre.belloni@bootlin.com, heiko@sntech.de,
	Yangtao Li <tiny.windzz@gmail.com>,
	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, 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 <lee.jones@linaro.org>,
	linux-arm-kernel@lists.infradead.org, sbranden@broadcom.com,
	nicolas.ferre@microchip.com, palmer@dabbelt.com,
	kernel@pengutronix.de, shawnguo@kernel.org,
	claudiu.beznea@microchip.com, nsaenzjulienne@suse.de
Subject: Re: [PATCH 01/32] pwm: sun4i: convert to devm_platform_ioremap_resource
Date: Thu, 12 Nov 2020 20:06:49 +0100
Message-ID: <20201112190649.GA908613@ulmo> (raw)
In-Reply-To: <20201112161346.gp5nenuagx5wmwl2@pengutronix.de>

[-- Attachment #1.1: Type: text/plain, Size: 3659 bytes --]

On Thu, Nov 12, 2020 at 05:13:46PM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Sun, Dec 29, 2019 at 08:05:39AM +0000, Yangtao Li wrote:
> > Use devm_platform_ioremap_resource() to simplify code.
> > 
> > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
> > ---
> >  drivers/pwm/pwm-sun4i.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index 581d23287333..f2afd312f77c 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -344,7 +344,6 @@ MODULE_DEVICE_TABLE(of, sun4i_pwm_dt_ids);
> >  static int sun4i_pwm_probe(struct platform_device *pdev)
> >  {
> >  	struct sun4i_pwm_chip *pwm;
> > -	struct resource *res;
> >  	int ret;
> >  
> >  	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> > @@ -355,8 +354,7 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> >  	if (!pwm->data)
> >  		return -ENODEV;
> >  
> > -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -	pwm->base = devm_ioremap_resource(&pdev->dev, res);
> > +	pwm->base = devm_platform_ioremap_resource(pdev, 0);
> >  	if (IS_ERR(pwm->base))
> >  		return PTR_ERR(pwm->base);
> 
> Can you please comment why you don't apply this series?

I did in fact apply this yesterday, but I now see that I didn't reply to
the thread to report that.

> My point of view is:
> 
> devm_platform_ioremap_resource is the designated wrapper to replace
> platform_get_resource() and devm_ioremap_resource(). So I don't see a
> good reason to continue open coding it.
> 
> The patch series doesn't apply to 5.10-rc1 as is. (pwm-puv3 was removed
> and a simple conflict in the pwm-rockchip driver.) The overall diffstat
> (of the fixed series applied on top of 5.10-rc1) is
> 
> 	31 files changed, 32 insertions(+), 96 deletions(-)
> 
> and it converts all of drivers/pwm but a single instance of
> platform_get_resource() + devm_ioremap_resource() (for pwm-lpss where
> platform_get_resource and devm_ioremap_resource are in different
> functions (different files even)) which isn't trivial to fix.
> 
> So in my eyes applying this series is the right thing to do.

For the record, I personally think this helper is a bit over the top. I
do agree that it's nice to create helpers for common code sequences, but
this is a *lot* of churn all across the kernel to save just two lines,
which I don't think is worth it in this case. Often these helpers allow
common mistakes to be avoided while at the same time reducing lines of
code, but devm_ioremap_resource() was already created to address the
pitfalls (like returning all sorts of weird and inconsistent error
codes). So this helper doesn't actually add any value other than saving
a few lines, which I don't think justifies the churn. I would've been
sold on this if the ratio had been slightly higher, like maybe saving a
dozen or so lines, but as it is, I just don't think it's worth the churn
that it's causing.

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). And then there are cases where the driver needs the
resource for some other purpose, so you can't use the helper either, or
at least it looses all of its advantages in those cases.

That said, the helper is there and has been widely accepted, so my
opinion has been overruled by the majority.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

  reply index

Thread overview: 80+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-29  8:05 Yangtao Li
2019-12-29  8:05 ` [PATCH 02/32] pwm: fsl-ftm: " Yangtao Li
2020-05-23 17:24   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 03/32] pwm: rcar: " Yangtao Li
2020-05-23 17:24   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 04/32] pwm: renesas-tpu: " Yangtao Li
2020-05-23 17:12   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 05/32] pwm: ep93xx: " Yangtao Li
2020-05-23 17:24   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 06/32] pwm: tegra: " Yangtao Li
2020-05-23 17:23   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 07/32] pwm: mediatek: " Yangtao Li
2020-05-23 17:23   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 08/32] pwm: sti: " Yangtao Li
2020-05-23 17:23   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 09/32] pwm: pxa: " Yangtao Li
2020-05-23 17:20   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 10/32] pwm: zx: " Yangtao Li
2020-05-23 17:23   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 11/32] pwm: spear: " Yangtao Li
2020-05-23 17:23   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 12/32] pwm: bcm-kona: " Yangtao Li
2020-05-23 17:20   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 13/32] pwm: lpc32xx: " Yangtao Li
2020-05-23 17:20   ` Uwe Kleine-König
2020-11-12 21:29   ` Vladimir Zapolskiy
2019-12-29  8:05 ` [PATCH 14/32] pwm: meson: " Yangtao Li
2019-12-31 14:53   ` Martin Blumenstingl
2020-05-23 17:25   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 15/32] pwm: rockchip: " Yangtao Li
2019-12-30  8:39   ` Heiko Stuebner
2020-05-23 17:25   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 16/32] pwm: bcm-iproc: " Yangtao Li
2020-05-23 17:22   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 17/32] pwm: samsung: " Yangtao Li
2020-05-23 17:21   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 18/32] pwm: tiehrpwm: " Yangtao Li
2020-05-23 17:22   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 19/32] pwm: puv3: " Yangtao Li
2020-05-23 17:19   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 20/32] pwm: imx: " Yangtao Li
2020-05-23 17:22   ` Uwe Kleine-König
2019-12-29  8:05 ` [PATCH 21/32] pwm: tiecap: " Yangtao Li
2020-05-23 17:19   ` Uwe Kleine-König
2019-12-29  8:06 ` [PATCH 22/32] pwm: bcm2835: " Yangtao Li
2020-05-23 17:18   ` Uwe Kleine-König
2019-12-29  8:06 ` [PATCH 23/32] pwm: berlin: " Yangtao Li
2020-05-23 17:18   ` Uwe Kleine-König
2019-12-29  8:06 ` [PATCH 24/32] pwm: vt8500: " Yangtao Li
2020-05-23 17:14   ` Uwe Kleine-König
2019-12-29  8:06 ` [PATCH 25/32] pwm: brcmstb: " Yangtao Li
2020-01-07 10:54   ` Nicolas Saenz Julienne
2020-05-23 17:25   ` Uwe Kleine-König
2019-12-29  8:06 ` [PATCH 26/32] pwm: mtk-disp: " Yangtao Li
2020-02-20 20:49   ` Matthias Brugger
2019-12-29  8:06 ` [PATCH 27/32] pwm: clps711x: " Yangtao Li
2020-05-23 17:17   ` Uwe Kleine-König
2019-12-29  8:06 ` [PATCH 28/32] pwm: img: " Yangtao Li
2020-05-23 17:16   ` Uwe Kleine-König
2019-12-29  8:06 ` [PATCH 29/32] pwm: lpc18xx-sct: " Yangtao Li
2020-05-23 17:14   ` Uwe Kleine-König
2020-11-12 21:29   ` Vladimir Zapolskiy
2019-12-29  8:06 ` [PATCH 30/32] pwm: hibvt: do some cleanup Yangtao Li
2020-02-20 20:41   ` Uwe Kleine-König
2020-05-23 17:44     ` Uwe Kleine-König
2019-12-29  8:06 ` [PATCH 31/32] pwm: sifive: convert to devm_platform_ioremap_resource Yangtao Li
2020-05-23 17:15   ` Uwe Kleine-König
2019-12-29  8:06 ` [PATCH 32/32] pwm: atmel: " Yangtao Li
2020-01-06 10:32   ` Claudiu.Beznea
2020-05-23 17:25   ` Uwe Kleine-König
2020-05-23 17:11 ` [PATCH 01/32] pwm: sun4i: " Uwe Kleine-König
2020-11-12 16:13 ` Uwe Kleine-König
2020-11-12 19:06   ` Thierry Reding [this message]
2020-11-12 21:14     ` Uwe Kleine-König
2020-11-13  7:03       ` About devm_platform_ioremap_resource [Was: Re: [PATCH 01/32] pwm: sun4i: convert to devm_platform_ioremap_resource] Uwe Kleine-König
2020-11-13  9:12         ` Bartosz Golaszewski
2020-11-13  9:35           ` Uwe Kleine-König
2020-11-13 16:11         ` Thierry Reding
2020-11-13 17:40           ` Robin Murphy
2020-11-19 17:08           ` Uwe Kleine-König

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201112190649.GA908613@ulmo \
    --to=thierry.reding@gmail.com \
    --cc=alexandre.belloni@bootlin.com \
    --cc=bcm-kernel-feedback-list@broadcom.com \
    --cc=claudiu.beznea@microchip.com \
    --cc=f.fainelli@gmail.com \
    --cc=festevam@gmail.com \
    --cc=heiko@sntech.de \
    --cc=jonathanh@nvidia.com \
    --cc=kernel@pengutronix.de \
    --cc=khilman@baylibre.com \
    --cc=lee.jones@linaro.org \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=linux-rpi-kernel@lists.infradead.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=ludovic.desroches@microchip.com \
    --cc=matthias.bgg@gmail.com \
    --cc=mripard@kernel.org \
    --cc=nicolas.ferre@microchip.com \
    --cc=nsaenzjulienne@suse.de \
    --cc=palmer@dabbelt.com \
    --cc=paul.walmsley@sifive.com \
    --cc=rjui@broadcom.com \
    --cc=s.hauer@pengutronix.de \
    --cc=sbranden@broadcom.com \
    --cc=shawnguo@kernel.org \
    --cc=shc_work@mail.ru \
    --cc=slemieux.tyco@gmail.com \
    --cc=tiny.windzz@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    --cc=vz@mleia.com \
    --cc=wens@csie.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Amlogic Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-amlogic/0 linux-amlogic/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-amlogic linux-amlogic/ https://lore.kernel.org/linux-amlogic \
		linux-amlogic@lists.infradead.org
	public-inbox-index linux-amlogic

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-amlogic


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git