All of lore.kernel.org
 help / color / mirror / Atom feed
* [patch] ASoC: sun4i-i2s: remove some dead code
@ 2017-02-07 13:19 ` Dan Carpenter
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2017-02-07 13:19 UTC (permalink / raw)
  To: Liam Girdwood, Mylène Josserand
  Cc: alsa-devel, Wei Yongjun, kernel-janitors, Takashi Iwai,
	Chen-Yu Tsai, Mark Brown, Maxime Ripard

"i2s->rst" is either NULL or a valid pointer.  We won't probe
successfully if it's an error pointer.  That means these checks can be
removed.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index 3635bbc72cbc..d79be3e4361b 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -723,13 +723,11 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
 		}
 	}
 
-	if (!IS_ERR(i2s->rst)) {
-		ret = reset_control_deassert(i2s->rst);
-		if (ret) {
-			dev_err(&pdev->dev,
-				"Failed to deassert the reset control\n");
-			return -EINVAL;
-		}
+	ret = reset_control_deassert(i2s->rst);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"Failed to deassert the reset control\n");
+		return -EINVAL;
 	}
 
 	i2s->playback_dma_data.addr = res->start + SUN4I_I2S_FIFO_TX_REG;
@@ -766,8 +764,7 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
 		sun4i_i2s_runtime_suspend(&pdev->dev);
 err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
-	if (!IS_ERR(i2s->rst))
-		reset_control_assert(i2s->rst);
+	reset_control_assert(i2s->rst);
 
 	return ret;
 }
@@ -782,8 +779,7 @@ static int sun4i_i2s_remove(struct platform_device *pdev)
 	if (!pm_runtime_status_suspended(&pdev->dev))
 		sun4i_i2s_runtime_suspend(&pdev->dev);
 
-	if (!IS_ERR(i2s->rst))
-		reset_control_assert(i2s->rst);
+	reset_control_assert(i2s->rst);
 
 	return 0;
 }

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [patch] ASoC: sun4i-i2s: remove some dead code
@ 2017-02-07 13:19 ` Dan Carpenter
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2017-02-07 13:19 UTC (permalink / raw)
  To: Liam Girdwood, Mylène Josserand
  Cc: alsa-devel, Wei Yongjun, kernel-janitors, Takashi Iwai,
	Chen-Yu Tsai, Mark Brown, Maxime Ripard

"i2s->rst" is either NULL or a valid pointer.  We won't probe
successfully if it's an error pointer.  That means these checks can be
removed.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index 3635bbc72cbc..d79be3e4361b 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -723,13 +723,11 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
 		}
 	}
 
-	if (!IS_ERR(i2s->rst)) {
-		ret = reset_control_deassert(i2s->rst);
-		if (ret) {
-			dev_err(&pdev->dev,
-				"Failed to deassert the reset control\n");
-			return -EINVAL;
-		}
+	ret = reset_control_deassert(i2s->rst);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"Failed to deassert the reset control\n");
+		return -EINVAL;
 	}
 
 	i2s->playback_dma_data.addr = res->start + SUN4I_I2S_FIFO_TX_REG;
@@ -766,8 +764,7 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
 		sun4i_i2s_runtime_suspend(&pdev->dev);
 err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
-	if (!IS_ERR(i2s->rst))
-		reset_control_assert(i2s->rst);
+	reset_control_assert(i2s->rst);
 
 	return ret;
 }
@@ -782,8 +779,7 @@ static int sun4i_i2s_remove(struct platform_device *pdev)
 	if (!pm_runtime_status_suspended(&pdev->dev))
 		sun4i_i2s_runtime_suspend(&pdev->dev);
 
-	if (!IS_ERR(i2s->rst))
-		reset_control_assert(i2s->rst);
+	reset_control_assert(i2s->rst);
 
 	return 0;
 }

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [patch] ASoC: sun4i-i2s: remove some dead code
  2017-02-07 13:19 ` Dan Carpenter
@ 2017-02-07 13:42   ` Maxime Ripard
  -1 siblings, 0 replies; 10+ messages in thread
From: Maxime Ripard @ 2017-02-07 13:42 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Liam Girdwood, Mylène Josserand, Mark Brown,
	Jaroslav Kysela, Takashi Iwai, Chen-Yu Tsai, Wei Yongjun,
	alsa-devel, kernel-janitors

[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]

Hi Dan,

On Tue, Feb 07, 2017 at 04:19:34PM +0300, Dan Carpenter wrote:
> "i2s->rst" is either NULL or a valid pointer.  We won't probe
> successfully if it's an error pointer.  That means these checks can be
> removed.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index 3635bbc72cbc..d79be3e4361b 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -723,13 +723,11 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	if (!IS_ERR(i2s->rst)) {
> -		ret = reset_control_deassert(i2s->rst);
> -		if (ret) {
> -			dev_err(&pdev->dev,
> -				"Failed to deassert the reset control\n");
> -			return -EINVAL;
> -		}
> +	ret = reset_control_deassert(i2s->rst);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Failed to deassert the reset control\n");
> +		return -EINVAL;

In the case where has_reset is false, rst is NULL and will trigger a
WARN_ON in reset_control_deassert.

The proper fix would be to move it in the previous if block, or to
change the IS_ERR check for a NULL check.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [patch] ASoC: sun4i-i2s: remove some dead code
@ 2017-02-07 13:42   ` Maxime Ripard
  0 siblings, 0 replies; 10+ messages in thread
From: Maxime Ripard @ 2017-02-07 13:42 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Liam Girdwood, Mylène Josserand, Mark Brown,
	Jaroslav Kysela, Takashi Iwai, Chen-Yu Tsai, Wei Yongjun,
	alsa-devel, kernel-janitors

[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]

Hi Dan,

On Tue, Feb 07, 2017 at 04:19:34PM +0300, Dan Carpenter wrote:
> "i2s->rst" is either NULL or a valid pointer.  We won't probe
> successfully if it's an error pointer.  That means these checks can be
> removed.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index 3635bbc72cbc..d79be3e4361b 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -723,13 +723,11 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	if (!IS_ERR(i2s->rst)) {
> -		ret = reset_control_deassert(i2s->rst);
> -		if (ret) {
> -			dev_err(&pdev->dev,
> -				"Failed to deassert the reset control\n");
> -			return -EINVAL;
> -		}
> +	ret = reset_control_deassert(i2s->rst);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Failed to deassert the reset control\n");
> +		return -EINVAL;

In the case where has_reset is false, rst is NULL and will trigger a
WARN_ON in reset_control_deassert.

The proper fix would be to move it in the previous if block, or to
change the IS_ERR check for a NULL check.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [patch] ASoC: sun4i-i2s: remove some dead code
  2017-02-07 13:42   ` Maxime Ripard
@ 2017-02-07 13:50     ` Dan Carpenter
  -1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2017-02-07 13:50 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: alsa-devel, Wei Yongjun, kernel-janitors, Takashi Iwai,
	Liam Girdwood, Chen-Yu Tsai, Mark Brown, Mylène Josserand

On Tue, Feb 07, 2017 at 02:42:15PM +0100, Maxime Ripard wrote:
> > -	if (!IS_ERR(i2s->rst)) {
> > -		ret = reset_control_deassert(i2s->rst);
> > -		if (ret) {
> > -			dev_err(&pdev->dev,
> > -				"Failed to deassert the reset control\n");
> > -			return -EINVAL;
> > -		}
> > +	ret = reset_control_deassert(i2s->rst);
> > +	if (ret) {
> > +		dev_err(&pdev->dev,
> > +			"Failed to deassert the reset control\n");
> > +		return -EINVAL;
> 
> In the case where has_reset is false, rst is NULL and will trigger a
> WARN_ON in reset_control_deassert.
> 

No it won't.  reset_control_deassert(NULL) just returns success
immediately.

> The proper fix would be to move it in the previous if block, or to
> change the IS_ERR check for a NULL check.

We could move it as a clean up but the current code works.  Checking for
NULL is wrong.

regards,
dan carpenter



^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [patch] ASoC: sun4i-i2s: remove some dead code
@ 2017-02-07 13:50     ` Dan Carpenter
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2017-02-07 13:50 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: alsa-devel, Wei Yongjun, kernel-janitors, Takashi Iwai,
	Liam Girdwood, Chen-Yu Tsai, Mark Brown, Mylène Josserand

On Tue, Feb 07, 2017 at 02:42:15PM +0100, Maxime Ripard wrote:
> > -	if (!IS_ERR(i2s->rst)) {
> > -		ret = reset_control_deassert(i2s->rst);
> > -		if (ret) {
> > -			dev_err(&pdev->dev,
> > -				"Failed to deassert the reset control\n");
> > -			return -EINVAL;
> > -		}
> > +	ret = reset_control_deassert(i2s->rst);
> > +	if (ret) {
> > +		dev_err(&pdev->dev,
> > +			"Failed to deassert the reset control\n");
> > +		return -EINVAL;
> 
> In the case where has_reset is false, rst is NULL and will trigger a
> WARN_ON in reset_control_deassert.
> 

No it won't.  reset_control_deassert(NULL) just returns success
immediately.

> The proper fix would be to move it in the previous if block, or to
> change the IS_ERR check for a NULL check.

We could move it as a clean up but the current code works.  Checking for
NULL is wrong.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [patch] ASoC: sun4i-i2s: remove some dead code
  2017-02-07 13:50     ` Dan Carpenter
@ 2017-02-08  8:58       ` Maxime Ripard
  -1 siblings, 0 replies; 10+ messages in thread
From: Maxime Ripard @ 2017-02-08  8:58 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Liam Girdwood, Mylène Josserand, Mark Brown,
	Jaroslav Kysela, Takashi Iwai, Chen-Yu Tsai, Wei Yongjun,
	alsa-devel, kernel-janitors

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]

On Tue, Feb 07, 2017 at 04:50:08PM +0300, Dan Carpenter wrote:
> On Tue, Feb 07, 2017 at 02:42:15PM +0100, Maxime Ripard wrote:
> > > -	if (!IS_ERR(i2s->rst)) {
> > > -		ret = reset_control_deassert(i2s->rst);
> > > -		if (ret) {
> > > -			dev_err(&pdev->dev,
> > > -				"Failed to deassert the reset control\n");
> > > -			return -EINVAL;
> > > -		}
> > > +	ret = reset_control_deassert(i2s->rst);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev,
> > > +			"Failed to deassert the reset control\n");
> > > +		return -EINVAL;
> > 
> > In the case where has_reset is false, rst is NULL and will trigger a
> > WARN_ON in reset_control_deassert.
> > 
> 
> No it won't.  reset_control_deassert(NULL) just returns success
> immediately.

Not in current Linus' tree at least:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/core.c#n217

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [patch] ASoC: sun4i-i2s: remove some dead code
@ 2017-02-08  8:58       ` Maxime Ripard
  0 siblings, 0 replies; 10+ messages in thread
From: Maxime Ripard @ 2017-02-08  8:58 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Liam Girdwood, Mylène Josserand, Mark Brown,
	Jaroslav Kysela, Takashi Iwai, Chen-Yu Tsai, Wei Yongjun,
	alsa-devel, kernel-janitors

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]

On Tue, Feb 07, 2017 at 04:50:08PM +0300, Dan Carpenter wrote:
> On Tue, Feb 07, 2017 at 02:42:15PM +0100, Maxime Ripard wrote:
> > > -	if (!IS_ERR(i2s->rst)) {
> > > -		ret = reset_control_deassert(i2s->rst);
> > > -		if (ret) {
> > > -			dev_err(&pdev->dev,
> > > -				"Failed to deassert the reset control\n");
> > > -			return -EINVAL;
> > > -		}
> > > +	ret = reset_control_deassert(i2s->rst);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev,
> > > +			"Failed to deassert the reset control\n");
> > > +		return -EINVAL;
> > 
> > In the case where has_reset is false, rst is NULL and will trigger a
> > WARN_ON in reset_control_deassert.
> > 
> 
> No it won't.  reset_control_deassert(NULL) just returns success
> immediately.

Not in current Linus' tree at least:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/core.c#n217

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [patch] ASoC: sun4i-i2s: remove some dead code
  2017-02-08  8:58       ` Maxime Ripard
@ 2017-02-08  9:01         ` Chen-Yu Tsai
  -1 siblings, 0 replies; 10+ messages in thread
From: Chen-Yu Tsai @ 2017-02-08  9:01 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Linux-ALSA, Wei Yongjun, kernel-janitors, Takashi Iwai,
	Liam Girdwood, Chen-Yu Tsai, Mark Brown, Mylène Josserand,
	Dan Carpenter

On Wed, Feb 8, 2017 at 4:58 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Tue, Feb 07, 2017 at 04:50:08PM +0300, Dan Carpenter wrote:
>> On Tue, Feb 07, 2017 at 02:42:15PM +0100, Maxime Ripard wrote:
>> > > - if (!IS_ERR(i2s->rst)) {
>> > > -         ret = reset_control_deassert(i2s->rst);
>> > > -         if (ret) {
>> > > -                 dev_err(&pdev->dev,
>> > > -                         "Failed to deassert the reset control\n");
>> > > -                 return -EINVAL;
>> > > -         }
>> > > + ret = reset_control_deassert(i2s->rst);
>> > > + if (ret) {
>> > > +         dev_err(&pdev->dev,
>> > > +                 "Failed to deassert the reset control\n");
>> > > +         return -EINVAL;
>> >
>> > In the case where has_reset is false, rst is NULL and will trigger a
>> > WARN_ON in reset_control_deassert.
>> >
>>
>> No it won't.  reset_control_deassert(NULL) just returns success
>> immediately.
>
> Not in current Linus' tree at least:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/core.c#n217

The changes were just pulled into arm-soc. I suggest waiting for -rc1 at least
before merging this change.

ChenYu

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [patch] ASoC: sun4i-i2s: remove some dead code
@ 2017-02-08  9:01         ` Chen-Yu Tsai
  0 siblings, 0 replies; 10+ messages in thread
From: Chen-Yu Tsai @ 2017-02-08  9:01 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Linux-ALSA, Wei Yongjun, kernel-janitors, Takashi Iwai,
	Liam Girdwood, Chen-Yu Tsai, Mark Brown, Mylène Josserand,
	Dan Carpenter

On Wed, Feb 8, 2017 at 4:58 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Tue, Feb 07, 2017 at 04:50:08PM +0300, Dan Carpenter wrote:
>> On Tue, Feb 07, 2017 at 02:42:15PM +0100, Maxime Ripard wrote:
>> > > - if (!IS_ERR(i2s->rst)) {
>> > > -         ret = reset_control_deassert(i2s->rst);
>> > > -         if (ret) {
>> > > -                 dev_err(&pdev->dev,
>> > > -                         "Failed to deassert the reset control\n");
>> > > -                 return -EINVAL;
>> > > -         }
>> > > + ret = reset_control_deassert(i2s->rst);
>> > > + if (ret) {
>> > > +         dev_err(&pdev->dev,
>> > > +                 "Failed to deassert the reset control\n");
>> > > +         return -EINVAL;
>> >
>> > In the case where has_reset is false, rst is NULL and will trigger a
>> > WARN_ON in reset_control_deassert.
>> >
>>
>> No it won't.  reset_control_deassert(NULL) just returns success
>> immediately.
>
> Not in current Linus' tree at least:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/core.c#n217

The changes were just pulled into arm-soc. I suggest waiting for -rc1 at least
before merging this change.

ChenYu

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-02-08  9:02 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-07 13:19 [patch] ASoC: sun4i-i2s: remove some dead code Dan Carpenter
2017-02-07 13:19 ` Dan Carpenter
2017-02-07 13:42 ` Maxime Ripard
2017-02-07 13:42   ` Maxime Ripard
2017-02-07 13:50   ` Dan Carpenter
2017-02-07 13:50     ` Dan Carpenter
2017-02-08  8:58     ` Maxime Ripard
2017-02-08  8:58       ` Maxime Ripard
2017-02-08  9:01       ` Chen-Yu Tsai
2017-02-08  9:01         ` Chen-Yu Tsai

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.