Skip to contentSkip to conversation

ASoC: sun4i-i2s: remove some dead code

PATCH In review Dan Carpenter 3 participants 5 messages Feb 7, 2017 – Feb 8, 2017

Subsystems (4)
Sound Kernel janitors ASoC Arm/Allwinner sunXi SoC support
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 ASoC: sun4i-i2s: remove some dead code

"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 <[email protected]>
sound/soc/sunxi/sun4i-i2s.c +7 -11
index 3635bbc72cbc..d79be3e4361b 100644
@@ -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;
 }
#1 lore raw

Hi Dan,

Quoted 16 lines · Dan Carpenter

"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 <[email protected]>
sound/soc/sunxi/sun4i-i2s.c +0 -2
index 3635bbc72cbc..d79be3e4361b 100644
@@ -723,13 +723,11 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
 		}
 	}
 
-	if (!IS_ERR(i2s->rst)) {
-		ret = reset_control_deassert(i2s->rst);

sound/soc/sunxi/sun4i-i2s.c static int sun4i_i2s_probe(struct platform_device *pdev) · line 730

Quoted 10 lines
sound/soc/sunxi/sun4i-i2s.c +5 -5
-		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

Signature · 3 lines
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
author
#2 lore raw
Quoted 7 lines · Maxime Ripard
Quoted 7 lines
sound/soc/sunxi/sun4i-i2s.c +0 -7
-	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;
-		}
Quoted 9 lines

sound/soc/sunxi/sun4i-i2s.c static int sun4i_i2s_probe(struct platform_device *pdev) · line 730

Quoted 5 lines
sound/soc/sunxi/sun4i-i2s.c +5 -0
+	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.

Quoted 2 lines

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

#3 lore raw
Quoted 18 lines · Dan Carpenter
Quoted 7 lines · Maxime Ripard
Quoted 7 lines
sound/soc/sunxi/sun4i-i2s.c +0 -7
-	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;
-		}
Quoted 9 lines

sound/soc/sunxi/sun4i-i2s.c static int sun4i_i2s_probe(struct platform_device *pdev) · line 730

Quoted 5 lines
sound/soc/sunxi/sun4i-i2s.c +5 -0
+	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.

Quoted 2 lines

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

Signature · 3 lines
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
#4 lore raw
Quoted 19 lines · Maxime Ripard
Quoted 18 lines · Dan Carpenter
Quoted 16 lines · Maxime Ripard

sound/soc/sunxi/sun4i-i2s.c static int sun4i_i2s_probe(struct platform_device *pdev) · line 730

Quoted 12 lines

- if (!IS_ERR(i2s->rst)) {

sound/soc/sunxi/sun4i-i2s.c +5 -6
-         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.

Quoted 5 lines
Quoted 2 lines

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