diff mbox

ASoC: sun4i-i2s: remove some dead code

Message ID 20170207131934.GD28207@mwanda (mailing list archive)
State New, archived
Headers show

Commit Message

Dan Carpenter Feb. 7, 2017, 1:19 p.m. UTC 
"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>

Comments

Maxime Ripard Feb. 7, 2017, 1:42 p.m. UTC | #1 
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
Dan Carpenter Feb. 7, 2017, 1:50 p.m. UTC | #2 
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
Maxime Ripard Feb. 8, 2017, 8:58 a.m. UTC | #3 
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
Chen-Yu Tsai Feb. 8, 2017, 9:01 a.m. UTC | #4 
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
diff mbox

Patch

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;
 }