Skip to contentSkip to conversation

regulator: max77802: precedence error in max77802_set_suspend_mode()

PATCH In review Dan Carpenter 3 participants 5 messages Oct 24, 2014 – Oct 27, 2014

Subsystems (2)
Kernel janitors Voltage and current regulator framework
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 regulator: max77802: precedence error in max77802_set_suspend_mode()

The "!" operation has higher precedence that the comparison.

Fixes 2e0eaa1aa008 ('regulator: max77802: Add set suspend mode for BUCKs and simplify code')
Signed-off-by Dan Carpenter <[email protected]>
drivers/regulator/max77802.c +1 -1
index 5839c45..57e37fd 100644
@@ -179,7 +179,7 @@ static int max77802_set_suspend_mode(struct regulator_dev *rdev,
 	 * If the regulator has been disabled for suspend
 	 * then is invalid to try setting a suspend mode.
 	 */
-	if (!max77802->opmode[id] = MAX77802_OFF_PWRREQ) {
+	if (max77802->opmode[id] != MAX77802_OFF_PWRREQ) {
 		dev_warn(&rdev->dev, "%s: is disabled, mode: 0x%x not set\n",
 			 rdev->desc->name, mode);
 		return 0;
#1 lore raw

Hello Dan,

Quoted 12 lines · Dan Carpenter

The "!" operation has higher precedence that the comparison.

Fixes 2e0eaa1aa008 ('regulator: max77802: Add set suspend mode for BUCKs and simplify code')
Signed-off-by Dan Carpenter <[email protected]>
drivers/regulator/max77802.c +0 -0
index 5839c45..57e37fd 100644

drivers/regulator/max77802.c static int max77802_set_suspend_mode(struct regulator_dev *rdev, · line 185

Quoted 8 lines
drivers/regulator/max77802.c +1 -1
@@ -179,7 +179,7 @@ static int max77802_set_suspend_mode(struct regulator_dev *rdev,
    * If the regulator has been disabled for suspend
    * then is invalid to try setting a suspend mode.
    */
-    if (!max77802->opmode[id] == MAX77802_OFF_PWRREQ) {
+    if (max77802->opmode[id] != MAX77802_OFF_PWRREQ) {
       dev_warn(&rdev->dev, "%s: is disabled, mode: 0x%x not set\n",
            rdev->desc->name, mode);
       return 0;

Thanks for the patch, there is indeed a typo error but $subject is not the correct fix.

The problem is that the "!" operator should not be in the expression so it has be removed and only compare if opmode is equal to MAX72802_OFF_PWRREQ.

With that change feel free to add my Reviewed-by.

Best regards,
Javier

author
#2 lore raw

PATCH v2 1/1 regulator: max77802: fix a test in max77802_set_suspend_mode()

The original test triggers a static checker warning. Javier Martinez Canillas says that the "!" is a typo and should be removed.

Fixes 2e0eaa1aa008 ('regulator: max77802: Add set suspend mode for BUCKs and simplify code')
Signed-off-by Dan Carpenter <[email protected]>

---
v2: The first version of this patch was buggy. Thanks Javier for your review.

drivers/regulator/max77802.c +1 -1
index 5839c45..7718c8a 100644
@@ -179,7 +179,7 @@ static int max77802_set_suspend_mode(struct regulator_dev *rdev,
 	 * If the regulator has been disabled for suspend
 	 * then is invalid to try setting a suspend mode.
 	 */
-	if (!max77802->opmode[id] = MAX77802_OFF_PWRREQ) {
+	if (max77802->opmode[id] = MAX77802_OFF_PWRREQ) {
 		dev_warn(&rdev->dev, "%s: is disabled, mode: 0x%x not set\n",
 			 rdev->desc->name, mode);
 		return 0;
Acked-by Tested-by on v2 1/1
#3 lore raw

Re: [patch v2] regulator: max77802: fix a test in max77802_set_suspend_mode()

Hello Dan,

Quoted 15 lines · Dan Carpenter

The original test triggers a static checker warning. Javier Martinez Canillas says that the "!" is a typo and should be removed.

Fixes 2e0eaa1aa008 ('regulator: max77802: Add set suspend mode for BUCKs and simplify code')
Signed-off-by Dan Carpenter <[email protected]>

---
v2: The first version of this patch was buggy. Thanks Javier for your review.

drivers/regulator/max77802.c +0 -0
index 5839c45..7718c8a 100644

drivers/regulator/max77802.c static int max77802_set_suspend_mode(struct regulator_dev *rdev, · line 185

Quoted 8 lines
drivers/regulator/max77802.c +1 -1
@@ -179,7 +179,7 @@ static int max77802_set_suspend_mode(struct regulator_dev *rdev,
 	 * If the regulator has been disabled for suspend
 	 * then is invalid to try setting a suspend mode.
 	 */
-	if (!max77802->opmode[id] = MAX77802_OFF_PWRREQ) {
+	if (max77802->opmode[id] = MAX77802_OFF_PWRREQ) {
 		dev_warn(&rdev->dev, "%s: is disabled, mode: 0x%x not set\n",
 			 rdev->desc->name, mode);
 		return 0;

The typo was not found during testing because the expression always evaluated to 0 which was expected since a regulator marked to be disabled for suspend shouldn't have a suspend mode to be set.

But when trying to trigger that specific case the test was indeed failing and the regulator mode was changed even when the regulator was marked as disabled.

This patch solves the issue by making max77802_set_suspend_mode() to return early if the regulator was marked as disabled

Thanks a lot for finding this bug.

Acked-by Javier Martinez Canillas <[email protected]>
Tested-by Javier Martinez Canillas <[email protected]>

Best regards,
Javier

maintainer Applied
#4 lore raw

Re: [patch v2] regulator: max77802: fix a test in max77802_set_suspend_mode()

In reply to Dan Carpenter
Quoted 2 lines · Dan Carpenter

The original test triggers a static checker warning. Javier Martinez Canillas says that the "!" is a typo and should be removed.

Applied, thanks.