[PATCH 0/2] change PTR_ERR argument

[PATCH 0/2] change PTR_ERR argument

[Julia Lawall]

Apply PTR_ERR to the value that was recently assigned.

---

 var/julia/linuxcopy/drivers/spi/spi-ep93xx.c   |    3 ++-
 var/julia/linuxcopy/sound/soc/qcom/lpass-cpu.c |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

[PATCH 1/2] ASoC: qcom: change PTR_ERR argument

[Julia Lawall]

Apply PTR_ERR to the value that was recently assigned.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression x,y;
@@

if (IS_ERR(x) || ...) {
  ... when any
      when != IS_ERR(...)
(
   PTR_ERR(x)
|
*  PTR_ERR(y)
)
  ... when any
}
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 sound/soc/qcom/lpass-cpu.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
index 23f3d59..94beb99 100644
--- a/sound/soc/qcom/lpass-cpu.c
+++ b/sound/soc/qcom/lpass-cpu.c
@@ -438,7 +438,8 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
 		if (IS_ERR(drvdata->mi2s_bit_clk[dai_id])) {
 			dev_err(&pdev->dev,
 				"%s() error getting mi2s-bit-clk: %ld\n",
-				__func__, PTR_ERR(drvdata->mi2s_bit_clk[i]));
+				__func__,
+				PTR_ERR(drvdata->mi2s_bit_clk[dai_id]));
 			return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]);
 		}
 	}

Re: [PATCH 1/2] ASoC: qcom: change PTR_ERR argument

[walter harms]

Am 30.08.2015 20:05, schrieb Julia Lawall:
> Apply PTR_ERR to the value that was recently assigned.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression x,y;
> @@
> 
> if (IS_ERR(x) || ...) {
>   ... when any
>       when != IS_ERR(...)
> (
>    PTR_ERR(x)
> |
> *  PTR_ERR(y)
> )
>   ... when any
> }
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
>  sound/soc/qcom/lpass-cpu.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
> index 23f3d59..94beb99 100644
> --- a/sound/soc/qcom/lpass-cpu.c
> +++ b/sound/soc/qcom/lpass-cpu.c
> @@ -438,7 +438,8 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
>  		if (IS_ERR(drvdata->mi2s_bit_clk[dai_id])) {
>  			dev_err(&pdev->dev,
>  				"%s() error getting mi2s-bit-clk: %ld\n",
> -				__func__, PTR_ERR(drvdata->mi2s_bit_clk[i]));
> +				__func__,
> +				PTR_ERR(drvdata->mi2s_bit_clk[dai_id]));
>  			return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]);
>  		}
>  	}
> 

just a note:
using a shorter name instead of drvdata->mi2s_bit_clk[dai_id] whould help to make the code
more readable (yes, the other code is alike). something like:

struct clk *tmp =  devm_clk_get(&pdev->dev,clk_name);

if (IS_ERR(tmp)) {
	dev_err(&pdev->dev,"%s() error getting mi2s-bit-clk: %ld\n",__func__, PTR_ERR(tmp));
        return PTR_ERR(tmp);
}
drvdata->mi2s_bit_clk[dai_id]=tmp;


just one minor:
the dev_warn() just before says: " error getting mi2s-osr-clk" may be it should be "warnig ..."
That will make it more easy to rep for real error in a log.

re,
 wh

Re: [PATCH 1/2] ASoC: qcom: change PTR_ERR argument

[Julia Lawall]

On Sun, 30 Aug 2015, walter harms wrote:

> 
> 
> Am 30.08.2015 20:05, schrieb Julia Lawall:
> > Apply PTR_ERR to the value that was recently assigned.
> > 
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @@
> > expression x,y;
> > @@
> > 
> > if (IS_ERR(x) || ...) {
> >   ... when any
> >       when != IS_ERR(...)
> > (
> >    PTR_ERR(x)
> > |
> > *  PTR_ERR(y)
> > )
> >   ... when any
> > }
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> > 
> > ---
> >  sound/soc/qcom/lpass-cpu.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
> > index 23f3d59..94beb99 100644
> > --- a/sound/soc/qcom/lpass-cpu.c
> > +++ b/sound/soc/qcom/lpass-cpu.c
> > @@ -438,7 +438,8 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
> >  		if (IS_ERR(drvdata->mi2s_bit_clk[dai_id])) {
> >  			dev_err(&pdev->dev,
> >  				"%s() error getting mi2s-bit-clk: %ld\n",
> > -				__func__, PTR_ERR(drvdata->mi2s_bit_clk[i]));
> > +				__func__,
> > +				PTR_ERR(drvdata->mi2s_bit_clk[dai_id]));
> >  			return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]);
> >  		}
> >  	}
> > 
> 
> just a note:
> using a shorter name instead of drvdata->mi2s_bit_clk[dai_id] whould help to make the code
> more readable (yes, the other code is alike). something like:
> 
> struct clk *tmp =  devm_clk_get(&pdev->dev,clk_name);

Where do you suggest to put this?

Maybe it would be reasonable to declare a variable struct clk *clk; at the 
top of the function, and then use that as a temporary variable for all 
three calls.

However, now I see that the first call, unlike the other two doesn't cause 
a return from the function.

                if (IS_ERR(drvdata->mi2s_osr_clk[dai_id])) {
                        dev_warn(&pdev->dev,
                                "%s() error getting mi2s-osr-clk: %ld\n",
				__func__,
		                PTR_ERR(drvdata->mi2s_osr_clk[dai_id]));
                }

Is that intentional?

thanks,
julia

> if (IS_ERR(tmp)) {
> 	dev_err(&pdev->dev,"%s() error getting mi2s-bit-clk: %ld\n",__func__, PTR_ERR(tmp));
>         return PTR_ERR(tmp);
> }
> drvdata->mi2s_bit_clk[dai_id]=tmp;
> 
> 
> just one minor:
> the dev_warn() just before says: " error getting mi2s-osr-clk" may be it should be "warnig ..."
> That will make it more easy to rep for real error in a log.
> 
> re,
>  wh
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [alsa-devel] [PATCH 1/2] ASoC: qcom: change PTR_ERR argument

[Kenneth Westfield]

On Sun, Aug 30, 2015 at 11:05:10AM -0700, Julia Lawall wrote:
> Apply PTR_ERR to the value that was recently assigned.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression x,y;
> @@
> 
> if (IS_ERR(x) || ...) {
>   ... when any
>       when != IS_ERR(...)
> (
>    PTR_ERR(x)
> |
> *  PTR_ERR(y)
> )
>   ... when any
> }
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---

The patch itself looks good.  Thanks.

Acked-by: Kenneth Westfield <kwestfie@codeaurora.org>

-- 
Kenneth Westfield
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, 
a Linux Foundation Collaborative Project

Re: [alsa-devel] [PATCH 1/2] ASoC: qcom: change PTR_ERR argument

[Kenneth Westfield]

On Sun, Aug 30, 2015 at 11:54:33AM -0700, walter harms wrote:
> Am 30.08.2015 20:05, schrieb Julia Lawall:
> >  		if (IS_ERR(drvdata->mi2s_bit_clk[dai_id])) {
> >  			dev_err(&pdev->dev,
> >  				"%s() error getting mi2s-bit-clk: %ld\n",
> > -				__func__,
> PTR_ERR(drvdata->mi2s_bit_clk[i]));
> > +				__func__,
> > +				PTR_ERR(drvdata->mi2s_bit_clk[dai_id]));
> >  			return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]);
> >  		}
> >  	}
> > 
> 
> just a note:
> using a shorter name instead of drvdata->mi2s_bit_clk[dai_id] whould help
> to make the code
> more readable (yes, the other code is alike). something like:
> 
> struct clk *tmp =  devm_clk_get(&pdev->dev,clk_name);
> 
> if (IS_ERR(tmp)) {
> 	dev_err(&pdev->dev,"%s() error getting mi2s-bit-clk:
> %ld\n",__func__, PTR_ERR(tmp));
>         return PTR_ERR(tmp);
> }
> drvdata->mi2s_bit_clk[dai_id]=tmp;

Yes, it would make the code more readable.

> 
> 
> just one minor:
> the dev_warn() just before says: " error getting mi2s-osr-clk" may be it
> should be "warnig ..."
> That will make it more easy to rep for real error in a log.

"error [gs]etting" could be re-phrased to "could not [gs]et".  The term
"error" was not meant to indicate the log level, but evidently, can
cause some confusion for someone reading the logs.

-- 
Kenneth Westfield
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, 
a Linux Foundation Collaborative Project

Re: [alsa-devel] [PATCH 1/2] ASoC: qcom: change PTR_ERR argument

[Kenneth Westfield]

On Sun, Aug 30, 2015 at 12:54:15PM -0700, Julia Lawall wrote:
> On Sun, 30 Aug 2015, walter harms wrote:
> > Am 30.08.2015 20:05, schrieb Julia Lawall:
> > >  		if (IS_ERR(drvdata->mi2s_bit_clk[dai_id])) {
> > >  			dev_err(&pdev->dev,
> > >  				"%s() error getting mi2s-bit-clk: %ld\n",
> > > -				__func__,
> PTR_ERR(drvdata->mi2s_bit_clk[i]));
> > > +				__func__,
> > > +				PTR_ERR(drvdata->mi2s_bit_clk[dai_id]));
> > >  			return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]);
> > >  		}
> > >  	}
> > > 
> > 
> > just a note:
> > using a shorter name instead of drvdata->mi2s_bit_clk[dai_id] whould
> help to make the code
> > more readable (yes, the other code is alike). something like:
> > 
> > struct clk *tmp =  devm_clk_get(&pdev->dev,clk_name);
> 
> Where do you suggest to put this?
> 
> Maybe it would be reasonable to declare a variable struct clk *clk; at the
> 
> top of the function, and then use that as a temporary variable for all 
> three calls.
> 
> However, now I see that the first call, unlike the other two doesn't cause
> 
> a return from the function.
> 
>                 if (IS_ERR(drvdata->mi2s_osr_clk[dai_id])) {
>                         dev_warn(&pdev->dev,
>                                 "%s() error getting mi2s-osr-clk: %ld\n",
> 				__func__,
> 		                PTR_ERR(drvdata->mi2s_osr_clk[dai_id]));
>                 }
> 
> Is that intentional?

Yes, that was intentional as the presense of the OSR clock in the DT
node is optional.

-- 
Kenneth Westfield
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, 
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] ASoC: qcom: change PTR_ERR argument

[Mark Brown]

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

On Sun, Aug 30, 2015 at 08:05:10PM +0200, Julia Lawall wrote:
> Apply PTR_ERR to the value that was recently assigned.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)

I have no idea what's going on with this stuff without spending more
time than it looks like should need, there's a moderately big thread and
some patches posted in the middle of it.  Can you repost whatever the
current state is please?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH 1/2] ASoC: qcom: change PTR_ERR argument

[Julia Lawall]

On Mon, 14 Sep 2015, Mark Brown wrote:

> On Sun, Aug 30, 2015 at 08:05:10PM +0200, Julia Lawall wrote:
> > Apply PTR_ERR to the value that was recently assigned.
> >
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
>
> I have no idea what's going on with this stuff without spending more
> time than it looks like should need, there's a moderately big thread and
> some patches posted in the middle of it.  Can you repost whatever the
> current state is please?

The discussion was about introducing a temporary variable to simplify the
code.  But that makes a lot of changes, so I think it would be better to
just apply the original bug fixing patch as is, and then the cleanup could
be applied on top of that.  I will submit the original patch again.

julia

Re: [PATCH 1/2] ASoC: qcom: change PTR_ERR argument

[Julia Lawall]

Apply PTR_ERR to the value that was recently assigned.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression x,y;
@@

if (IS_ERR(x) || ...) {
  ... when any
      when != IS_ERR(...)
(
   PTR_ERR(x)
|
*  PTR_ERR(y)
)
  ... when any
}
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 sound/soc/qcom/lpass-cpu.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
index 23f3d59..94beb99 100644
--- a/sound/soc/qcom/lpass-cpu.c
+++ b/sound/soc/qcom/lpass-cpu.c
@@ -438,7 +438,8 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
 		if (IS_ERR(drvdata->mi2s_bit_clk[dai_id])) {
 			dev_err(&pdev->dev,
 				"%s() error getting mi2s-bit-clk: %ld\n",
-				__func__, PTR_ERR(drvdata->mi2s_bit_clk[i]));
+				__func__,
+				PTR_ERR(drvdata->mi2s_bit_clk[dai_id]));
 			return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]);
 		}
 	}

Re: [PATCH 1/2] ASoC: qcom: change PTR_ERR argument

[Mark Brown]

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

On Thu, Sep 17, 2015 at 10:46:16AM +0200, Julia Lawall wrote:

> The discussion was about introducing a temporary variable to simplify the
> code.  But that makes a lot of changes, so I think it would be better to
> just apply the original bug fixing patch as is, and then the cleanup could
> be applied on top of that.  I will submit the original patch again.

OK, makes sense - thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Applied "ASoC: qcom: change PTR_ERR argument" to the asoc tree

[Mark Brown]

The patch

   ASoC: qcom: change PTR_ERR argument

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 8985729578cb42f9e781a8e38e5b6b1ee90c1018 Mon Sep 17 00:00:00 2001
From: Julia Lawall <julia.lawall@lip6.fr>
Date: Thu, 17 Sep 2015 10:47:33 +0200
Subject: [PATCH] ASoC: qcom: change PTR_ERR argument

Apply PTR_ERR to the value that was recently assigned.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression x,y;
@@

if (IS_ERR(x) || ...) {
  ... when any
      when != IS_ERR(...)
(
   PTR_ERR(x)
|
*  PTR_ERR(y)
)
  ... when any
}
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/qcom/lpass-cpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
index 97bc202..e5101e0 100644
--- a/sound/soc/qcom/lpass-cpu.c
+++ b/sound/soc/qcom/lpass-cpu.c
@@ -438,7 +438,8 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
 		if (IS_ERR(drvdata->mi2s_bit_clk[dai_id])) {
 			dev_err(&pdev->dev,
 				"%s() error getting mi2s-bit-clk: %ld\n",
-				__func__, PTR_ERR(drvdata->mi2s_bit_clk[i]));
+				__func__,
+				PTR_ERR(drvdata->mi2s_bit_clk[dai_id]));
 			return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]);
 		}
 	}
-- 
2.5.0