[PATCH] pwm: pwm-imx27: Use 'dev' instead of dereferencing it repeatedly

[PATCH] pwm: pwm-imx27: Use 'dev' instead of dereferencing it repeatedly

[Anson Huang]

Add helper variable dev = &pdev->dev to simply the code.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/pwm/pwm-imx27.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 434a351..3afee29 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -290,27 +290,28 @@ MODULE_DEVICE_TABLE(of, pwm_imx27_dt_ids);
 
 static int pwm_imx27_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct pwm_imx27_chip *imx;
 
-	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
+	imx = devm_kzalloc(dev, sizeof(*imx), GFP_KERNEL);
 	if (imx == NULL)
 		return -ENOMEM;
 
 	platform_set_drvdata(pdev, imx);
 
-	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
+	imx->clk_ipg = devm_clk_get(dev, "ipg");
 	if (IS_ERR(imx->clk_ipg)) {
-		dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
+		dev_err(dev, "getting ipg clock failed with %ld\n",
 				PTR_ERR(imx->clk_ipg));
 		return PTR_ERR(imx->clk_ipg);
 	}
 
-	imx->clk_per = devm_clk_get(&pdev->dev, "per");
+	imx->clk_per = devm_clk_get(dev, "per");
 	if (IS_ERR(imx->clk_per)) {
 		int ret = PTR_ERR(imx->clk_per);
 
 		if (ret != -EPROBE_DEFER)
-			dev_err(&pdev->dev,
+			dev_err(dev,
 				"failed to get peripheral clock: %d\n",
 				ret);
 
@@ -318,7 +319,7 @@ static int pwm_imx27_probe(struct platform_device *pdev)
 	}
 
 	imx->chip.ops = &pwm_imx27_ops;
-	imx->chip.dev = &pdev->dev;
+	imx->chip.dev = dev;
 	imx->chip.base = -1;
 	imx->chip.npwm = 1;
 
-- 
2.7.4


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

RE: [PATCH] pwm: pwm-imx27: Use 'dev' instead of dereferencing it repeatedly

[David Laight]

From: Anson Huang
> Sent: 24 September 2019 10:00
> Add helper variable dev = &pdev->dev to simply the code.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/pwm/pwm-imx27.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 434a351..3afee29 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -290,27 +290,28 @@ MODULE_DEVICE_TABLE(of, pwm_imx27_dt_ids);
> 
>  static int pwm_imx27_probe(struct platform_device *pdev)
>  {
> +	struct device *dev = &pdev->dev;
>  	struct pwm_imx27_chip *imx;
> 
> -	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
> +	imx = devm_kzalloc(dev, sizeof(*imx), GFP_KERNEL);
>  	if (imx == NULL)
>  		return -ENOMEM;
> 
>  	platform_set_drvdata(pdev, imx);
> 
> -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> +	imx->clk_ipg = devm_clk_get(dev, "ipg");
>  	if (IS_ERR(imx->clk_ipg)) {
> -		dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> +		dev_err(dev, "getting ipg clock failed with %ld\n",
>  				PTR_ERR(imx->clk_ipg));
>  		return PTR_ERR(imx->clk_ipg);
>  	}
> 
> -	imx->clk_per = devm_clk_get(&pdev->dev, "per");
> +	imx->clk_per = devm_clk_get(dev, "per");
>  	if (IS_ERR(imx->clk_per)) {
>  		int ret = PTR_ERR(imx->clk_per);
> 
>  		if (ret != -EPROBE_DEFER)
> -			dev_err(&pdev->dev,
> +			dev_err(dev,
>  				"failed to get peripheral clock: %d\n",
>  				ret);

Hopefully the compiler will optimise this back otherwise you've added another
local variable which may cause spilling to stack.
For a setup function it probably doesn't matter, but in general it might
have a small negative performance impact.

In any case this doesn't shorten any lines enough to remove line-wrap
and using &pdev->dev is really one less variable to mentally track
when reading the code.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


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

RE: [PATCH] pwm: pwm-imx27: Use 'dev' instead of dereferencing it repeatedly

[Anson Huang]

Hi, David

> Subject: RE: [PATCH] pwm: pwm-imx27: Use 'dev' instead of dereferencing it
> repeatedly
> 
> From: Anson Huang
> > Sent: 24 September 2019 10:00
> > Add helper variable dev = &pdev->dev to simply the code.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  drivers/pwm/pwm-imx27.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c index
> > 434a351..3afee29 100644
> > --- a/drivers/pwm/pwm-imx27.c
> > +++ b/drivers/pwm/pwm-imx27.c
> > @@ -290,27 +290,28 @@ MODULE_DEVICE_TABLE(of,
> pwm_imx27_dt_ids);
> >
> >  static int pwm_imx27_probe(struct platform_device *pdev)  {
> > +	struct device *dev = &pdev->dev;
> >  	struct pwm_imx27_chip *imx;
> >
> > -	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
> > +	imx = devm_kzalloc(dev, sizeof(*imx), GFP_KERNEL);
> >  	if (imx == NULL)
> >  		return -ENOMEM;
> >
> >  	platform_set_drvdata(pdev, imx);
> >
> > -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > +	imx->clk_ipg = devm_clk_get(dev, "ipg");
> >  	if (IS_ERR(imx->clk_ipg)) {
> > -		dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> > +		dev_err(dev, "getting ipg clock failed with %ld\n",
> >  				PTR_ERR(imx->clk_ipg));
> >  		return PTR_ERR(imx->clk_ipg);
> >  	}
> >
> > -	imx->clk_per = devm_clk_get(&pdev->dev, "per");
> > +	imx->clk_per = devm_clk_get(dev, "per");
> >  	if (IS_ERR(imx->clk_per)) {
> >  		int ret = PTR_ERR(imx->clk_per);
> >
> >  		if (ret != -EPROBE_DEFER)
> > -			dev_err(&pdev->dev,
> > +			dev_err(dev,
> >  				"failed to get peripheral clock: %d\n",
> >  				ret);
> 
> Hopefully the compiler will optimise this back otherwise you've added
> another local variable which may cause spilling to stack.
> For a setup function it probably doesn't matter, but in general it might have a
> small negative performance impact.
> 
> In any case this doesn't shorten any lines enough to remove line-wrap and
> using &pdev->dev is really one less variable to mentally track when reading
> the code.

Do we know which compiler will optimize this? I saw many of the patches doing
this to avoid a lot of dereference, I understand it does NOT save lines, but my intention
is to avoid dereference which might save some instructions.

I thought saving instructions is more important. So now there are different opinion about
doing this?

Anson 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] pwm: pwm-imx27: Use 'dev' instead of dereferencing it repeatedly

[David Laight]

From: Anson Huang
> Sent: 24 September 2019 11:03
> Hi, David
> 
> > Subject: RE: [PATCH] pwm: pwm-imx27: Use 'dev' instead of dereferencing it
> > repeatedly
> >
> > From: Anson Huang
> > > Sent: 24 September 2019 10:00
> > > Add helper variable dev = &pdev->dev to simply the code.
> > >
...
> > >  static int pwm_imx27_probe(struct platform_device *pdev)  {
> > > +	struct device *dev = &pdev->dev;
> > >  	struct pwm_imx27_chip *imx;
> > >
> > > -	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
> > > +	imx = devm_kzalloc(dev, sizeof(*imx), GFP_KERNEL);
...
> > Hopefully the compiler will optimise this back otherwise you've added
> > another local variable which may cause spilling to stack.
> > For a setup function it probably doesn't matter, but in general it might have a
> > small negative performance impact.
> >
> > In any case this doesn't shorten any lines enough to remove line-wrap and
> > using &pdev->dev is really one less variable to mentally track when reading
> > the code.
> 
> Do we know which compiler will optimize this? I saw many of the patches doing
> this to avoid a lot of dereference, I understand it does NOT save lines, but my intention
> is to avoid dereference which might save some instructions.
> 
> I thought saving instructions is more important. So now there are different opinion about
> doing this?

Remember &pdev->dev is just 'pdev + constant'.
Assuming 'pdev' is held in a callee saved register (which you want it to be) then to access
dev->foo the compiler can remember the constant and use an offset from 'pdev' instead of
an extra 'dev' variable.
On most modern ABI the first function call arguments are passed in registers.
So an add  instruction (probably lea) can be used to add the constant offset at the same
time as the value is moved into the argument register.

However your extra variable could easily get spilled out to the stack.
So you get an extra memory read rather than (at most) an extra 'add' instruction.

Even if pdev->dev were a pointer, repeatedly reading it from pdev->dev could
easily generate better code than having an extra variable that would mean the
value was repeatedly read from the stack.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] pwm: pwm-imx27: Use 'dev' instead of dereferencing it repeatedly

[Thierry Reding]

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

On Tue, Sep 24, 2019 at 09:46:20AM +0000, David Laight wrote:
> From: Anson Huang
> > Sent: 24 September 2019 10:00
> > Add helper variable dev = &pdev->dev to simply the code.
> > 
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  drivers/pwm/pwm-imx27.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> > index 434a351..3afee29 100644
> > --- a/drivers/pwm/pwm-imx27.c
> > +++ b/drivers/pwm/pwm-imx27.c
> > @@ -290,27 +290,28 @@ MODULE_DEVICE_TABLE(of, pwm_imx27_dt_ids);
> > 
> >  static int pwm_imx27_probe(struct platform_device *pdev)
> >  {
> > +	struct device *dev = &pdev->dev;
> >  	struct pwm_imx27_chip *imx;
> > 
> > -	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
> > +	imx = devm_kzalloc(dev, sizeof(*imx), GFP_KERNEL);
> >  	if (imx == NULL)
> >  		return -ENOMEM;
> > 
> >  	platform_set_drvdata(pdev, imx);
> > 
> > -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > +	imx->clk_ipg = devm_clk_get(dev, "ipg");
> >  	if (IS_ERR(imx->clk_ipg)) {
> > -		dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> > +		dev_err(dev, "getting ipg clock failed with %ld\n",
> >  				PTR_ERR(imx->clk_ipg));
> >  		return PTR_ERR(imx->clk_ipg);
> >  	}
> > 
> > -	imx->clk_per = devm_clk_get(&pdev->dev, "per");
> > +	imx->clk_per = devm_clk_get(dev, "per");
> >  	if (IS_ERR(imx->clk_per)) {
> >  		int ret = PTR_ERR(imx->clk_per);
> > 
> >  		if (ret != -EPROBE_DEFER)
> > -			dev_err(&pdev->dev,
> > +			dev_err(dev,
> >  				"failed to get peripheral clock: %d\n",
> >  				ret);
> 
> Hopefully the compiler will optimise this back otherwise you've added another
> local variable which may cause spilling to stack.
> For a setup function it probably doesn't matter, but in general it might
> have a small negative performance impact.
> 
> In any case this doesn't shorten any lines enough to remove line-wrap
> and using &pdev->dev is really one less variable to mentally track
> when reading the code.
> 
> 	David

I agree. A positive diffstat is often a good indication that it's not
worth it. Don't get me wrong, I think there are cases where an extra
local variable can be worth it, but this isn't one of them, so it's
really just unnecessary churn.

Thierry

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

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

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

RE: [PATCH] pwm: pwm-imx27: Use 'dev' instead of dereferencing it repeatedly

[Anson Huang]

Hi, David

> Subject: RE: [PATCH] pwm: pwm-imx27: Use 'dev' instead of dereferencing it
> repeatedly
> 
> From: Anson Huang
> > Sent: 24 September 2019 11:03
> > Hi, David
> >
> > > Subject: RE: [PATCH] pwm: pwm-imx27: Use 'dev' instead of
> > > dereferencing it repeatedly
> > >
> > > From: Anson Huang
> > > > Sent: 24 September 2019 10:00
> > > > Add helper variable dev = &pdev->dev to simply the code.
> > > >
> ...
> > > >  static int pwm_imx27_probe(struct platform_device *pdev)  {
> > > > +	struct device *dev = &pdev->dev;
> > > >  	struct pwm_imx27_chip *imx;
> > > >
> > > > -	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
> > > > +	imx = devm_kzalloc(dev, sizeof(*imx), GFP_KERNEL);
> ...
> > > Hopefully the compiler will optimise this back otherwise you've
> > > added another local variable which may cause spilling to stack.
> > > For a setup function it probably doesn't matter, but in general it
> > > might have a small negative performance impact.
> > >
> > > In any case this doesn't shorten any lines enough to remove
> > > line-wrap and using &pdev->dev is really one less variable to
> > > mentally track when reading the code.
> >
> > Do we know which compiler will optimize this? I saw many of the
> > patches doing this to avoid a lot of dereference, I understand it does
> > NOT save lines, but my intention is to avoid dereference which might save
> some instructions.
> >
> > I thought saving instructions is more important. So now there are
> > different opinion about doing this?
> 
> Remember &pdev->dev is just 'pdev + constant'.
> Assuming 'pdev' is held in a callee saved register (which you want it to be)
> then to access
> dev->foo the compiler can remember the constant and use an offset from
> dev->'pdev' instead of
> an extra 'dev' variable.
> On most modern ABI the first function call arguments are passed in registers.
> So an add  instruction (probably lea) can be used to add the constant offset
> at the same time as the value is moved into the argument register.
> 
> However your extra variable could easily get spilled out to the stack.
> So you get an extra memory read rather than (at most) an extra 'add'
> instruction.
> 
> Even if pdev->dev were a pointer, repeatedly reading it from pdev->dev
> could easily generate better code than having an extra variable that would
> mean the value was repeatedly read from the stack.

Thanks for detail education about it, please ignore these patches.

Thanks,
Anson
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] pwm: pwm-imx27: Use 'dev' instead of dereferencing it repeatedly

[Uwe Kleine-König]

On Tue, Sep 24, 2019 at 09:46:20AM +0000, David Laight wrote:
> From: Anson Huang
> > Sent: 24 September 2019 10:00
> > Add helper variable dev = &pdev->dev to simply the code.
> > 
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  drivers/pwm/pwm-imx27.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> > index 434a351..3afee29 100644
> > --- a/drivers/pwm/pwm-imx27.c
> > +++ b/drivers/pwm/pwm-imx27.c
> > @@ -290,27 +290,28 @@ MODULE_DEVICE_TABLE(of, pwm_imx27_dt_ids);
> > 
> >  static int pwm_imx27_probe(struct platform_device *pdev)
> >  {
> > +	struct device *dev = &pdev->dev;
> >  	struct pwm_imx27_chip *imx;
> > 
> > -	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
> > +	imx = devm_kzalloc(dev, sizeof(*imx), GFP_KERNEL);
> >  	if (imx == NULL)
> >  		return -ENOMEM;
> > 
> >  	platform_set_drvdata(pdev, imx);
> > 
> > -	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > +	imx->clk_ipg = devm_clk_get(dev, "ipg");
> >  	if (IS_ERR(imx->clk_ipg)) {
> > -		dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> > +		dev_err(dev, "getting ipg clock failed with %ld\n",
> >  				PTR_ERR(imx->clk_ipg));
> >  		return PTR_ERR(imx->clk_ipg);
> >  	}
> > 
> > -	imx->clk_per = devm_clk_get(&pdev->dev, "per");
> > +	imx->clk_per = devm_clk_get(dev, "per");
> >  	if (IS_ERR(imx->clk_per)) {
> >  		int ret = PTR_ERR(imx->clk_per);
> > 
> >  		if (ret != -EPROBE_DEFER)
> > -			dev_err(&pdev->dev,
> > +			dev_err(dev,
> >  				"failed to get peripheral clock: %d\n",
> >  				ret);
> 
> Hopefully the compiler will optimise this back otherwise you've added another
> local variable which may cause spilling to stack.

I wonder that in reply to this comment nobody actually tried. I just did
that and applying the patch doesn't change the resulting binary. (Tested
with gcc 7.3.1).

> For a setup function it probably doesn't matter, but in general it might
> have a small negative performance impact.
> 
> In any case this doesn't shorten any lines enough to remove line-wrap
> and using &pdev->dev is really one less variable to mentally track
> when reading the code.

On the other hand having a variable named "dev" is so usual that I
personally slightly prefer using it instead of &pdev->dev. So I think
(given there is no effect on the compiled result) this is really just
about personal taste and so to actually switch from one preference to
the other needs a better justification IMHO.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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