Skip to contentSkip to conversation

regulator: s2mps11: Added shutdown function to poweroff

PATCH In review Anand Moon 2 participants 3 messages Jul 14, 2015 – Jul 15, 2015

Subsystems (3)
Arm Samsung Exynos Voltage and current regulator framework Samsung multifunction PMIC device drivers
lore
Share & tools
lore

Anand Moon

author
#0 lore raw

PATCH v2 1/1 regulator: s2mps11: Added shutdown function to poweroff

Added .shutdown function to s2mps11 to help poweroff the board successfully.

s2mps11-pmic: S2MPS11_REG_CTRL1 reg value 16:00000000000000000000000000010000

The device driver clears the register to turn off the PMIC.

Signed-off-by Anand Moon <[email protected]>

---
Console log for improper shutdown.

root@odroidxu3:~# poweroff
...
  * Unmounting temporary filesystems...                                   [ OK ]
  * Deactivating swap...                                                  [ OK ]
  * Unmounting local filesystems...                                       [ OK ]
  * Will now halt
  [  209.020280] reboot: Power down
  [  209.122039] Power down failed, please power off system manually.

Console log for proper shutdown.

root@odroidxu3:~# poweroff
...
  * Unmounting temporary filesystems...                                   [ OK ]
  * Deactivating swap...                                                  [ OK ]
  * Unmounting local filesystems...                                       [ OK ]
  * Will now halt
  [  476.283071] reboo
---
 drivers/regulator/s2mps11.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
drivers/regulator/s2mps11.c +26 -0
index 326ffb5..823180e 100644
@@ -1060,6 +1060,31 @@ out:
 	return ret;
 }
 
+static void s2mps11_pmic_shutdown(struct platform_device *pdev)
+{
+	struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
+	unsigned int reg_val, ret;
+
+	ret = regmap_read(iodev->regmap_pmic, S2MPS11_REG_CTRL1, &reg_val);
+	if (ret < 0) {
+		dev_crit(&pdev->dev, "could not read S2MPS11_REG_CTRL1 value\n");
+	} else {
+		/*
+		 * s2mps11-pmic: S2MPS11_REG_CTRL1 reg value
+		 * is 00000000000000000000000000010000
+		 * clear the S2MPS11_REG_CTRL1 0x10 value to shutdown.
+		 */
+		if (reg_val & BIT(4)) {
+			ret = regmap_update_bits(iodev->regmap_pmic,
+						 S2MPS11_REG_CTRL1,
+						 BIT(4), BIT(0));
+			if (ret)
+				dev_crit(&pdev->dev,
+					 "could not write S2MPS11_REG_CTRL1 value\n");
+		}
+	}
+}
+
 static const struct platform_device_id s2mps11_pmic_id[] = {
 	{ "s2mps11-pmic", S2MPS11X},
 	{ "s2mps13-pmic", S2MPS13X},

@@ -1074,6 +1099,7 @@ static struct platform_driver s2mps11_pmic_driver = {
 		.name = "s2mps11-pmic",
 	},
 	.probe = s2mps11_pmic_probe,
+	.shutdown = s2mps11_pmic_shutdown,
 	.id_table = s2mps11_pmic_id,
 };
Signature · 1 line
2.4.5
#1 lore raw
Quoted 1 line · Anand Moon

Added .shutdown function to s2mps11 to help poweroff the board successfully.

Which board does not poweroff? The PMIC is used on multiple boards, did you observed this on all of them?

Quoted 4 lines

s2mps11-pmic: S2MPS11_REG_CTRL1 reg value 16:00000000000000000000000000010000

The device driver clears the register to turn off the PMIC.

This is not sufficient explanation for commit message.

I already raised concerns that this does not look to me as a proper way of doing poweroff. Unfortunately you did not resolved these concerns.

The main questions are unanswered: Why you have to do this and why "standard" way does not work?
How can you properly fix some problem if you don't know the cause of problem? It is blind shooting which may hurt other boards.

Quoted 31 lines
Signed-off-by Anand Moon <[email protected]>

---
Console log for improper shutdown.

root@odroidxu3:~# poweroff
...
  * Unmounting temporary filesystems...                                   [ OK ]
  * Deactivating swap...                                                  [ OK ]
  * Unmounting local filesystems...                                       [ OK ]
  * Will now halt
  [  209.020280] reboot: Power down
  [  209.122039] Power down failed, please power off system manually.

Console log for proper shutdown.

root@odroidxu3:~# poweroff
...
  * Unmounting temporary filesystems...                                   [ OK ]
  * Deactivating swap...                                                  [ OK ]
  * Unmounting local filesystems...                                       [ OK ]
  * Will now halt
  [  476.283071] reboo
---
 drivers/regulator/s2mps11.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
drivers/regulator/s2mps11.c +0 -0
index 326ffb5..823180e 100644

drivers/regulator/s2mps11.c out: · line 1069

Quoted 10 lines
drivers/regulator/s2mps11.c +7 -0
@@ -1060,6 +1060,31 @@ out:
 	return ret;
 }
 
+static void s2mps11_pmic_shutdown(struct platform_device *pdev)
+{
+	struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
+	unsigned int reg_val, ret;
+
+	ret = regmap_read(iodev->regmap_pmic, S2MPS11_REG_CTRL1, &reg_val);
+	if (ret < 0) {

regmap_read() returns an int which you assign to an unsigned int which then you compare against <0? This does not look good.

drivers/regulator/s2mps11.c out: · line 1080

Quoted 11 lines
drivers/regulator/s2mps11.c +11 -0
+		dev_crit(&pdev->dev, "could not read S2MPS11_REG_CTRL1 value\n");
+	} else {
+		/*
+		 * s2mps11-pmic: S2MPS11_REG_CTRL1 reg value
+		 * is 00000000000000000000000000010000
+		 * clear the S2MPS11_REG_CTRL1 0x10 value to shutdown.
+		 */
+		if (reg_val & BIT(4)) {
+			ret = regmap_update_bits(iodev->regmap_pmic,
+						 S2MPS11_REG_CTRL1,
+						 BIT(4), BIT(0));

I don't understand. You want to update BIT(4) but the value is BIT(0)? This will clear BIT(4) but is totally unreadable.

drivers/regulator/s2mps11.c out: · line 1085

Quoted 5 lines
drivers/regulator/s2mps11.c +5 -0
+			if (ret)
+				dev_crit(&pdev->dev,
+					 "could not write S2MPS11_REG_CTRL1 value\n");
+		}
+	}

The code is not readable, to many unnecessary indentations.

drivers/regulator/s2mps11.c static struct platform_driver s2mps11_pmic_driver = { · line 1102

Quoted 10 lines
drivers/regulator/s2mps11.c +3 -0
+}
+
 static const struct platform_device_id s2mps11_pmic_id[] = {
 	{ "s2mps11-pmic", S2MPS11X},
 	{ "s2mps13-pmic", S2MPS13X},

@@ -1074,6 +1099,7 @@ static struct platform_driver s2mps11_pmic_driver = {
 		.name = "s2mps11-pmic",
 	},
 	.probe = s2mps11_pmic_probe,
+	.shutdown = s2mps11_pmic_shutdown,

The purpose of shutdown function is not to shutdown the system but to prepare the system for shutdown.

The patch is just wrong and you did not answered the major question - WHY you have to do this? Don't fix the problem blindly (or because some hardkernel tree for some of the boards use such patch).

Best regards,
Krzysztof

Quoted 4 lines
drivers/regulator/s2mps11.c +0 -0
 	.id_table = s2mps11_pmic_id,
 };
author
#2 lore raw

hi Krzysztof,

Quoted 104 lines · Krzysztof Kozlowski
Quoted 1 line · Anand Moon

Added .shutdown function to s2mps11 to help poweroff the board successfully.

Which board does not poweroff? The PMIC is used on multiple boards, did you observed this on all of them?

Quoted 4 lines

s2mps11-pmic: S2MPS11_REG_CTRL1 reg value 16:00000000000000000000000000010000

The device driver clears the register to turn off the PMIC.

This is not sufficient explanation for commit message.

I already raised concerns that this does not look to me as a proper way of doing poweroff. Unfortunately you did not resolved these concerns.

The main questions are unanswered: Why you have to do this and why "standard" way does not work?
How can you properly fix some problem if you don't know the cause of problem? It is blind shooting which may hurt other boards.

Quoted 31 lines
Signed-off-by Anand Moon <[email protected]>

---
Console log for improper shutdown.

root@odroidxu3:~# poweroff
...
  * Unmounting temporary filesystems...                                   [ OK ]
  * Deactivating swap...                                                  [ OK ]
  * Unmounting local filesystems...                                       [ OK ]
  * Will now halt
  [  209.020280] reboot: Power down
  [  209.122039] Power down failed, please power off system manually.

Console log for proper shutdown.

root@odroidxu3:~# poweroff
...
  * Unmounting temporary filesystems...                                   [ OK ]
  * Deactivating swap...                                                  [ OK ]
  * Unmounting local filesystems...                                       [ OK ]
  * Will now halt
  [  476.283071] reboo
---
 drivers/regulator/s2mps11.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
drivers/regulator/s2mps11.c +0 -0
index 326ffb5..823180e 100644

drivers/regulator/s2mps11.c out: · line 1069

Quoted 10 lines
drivers/regulator/s2mps11.c +7 -0
@@ -1060,6 +1060,31 @@ out:
      return ret;
 }
 
+static void s2mps11_pmic_shutdown(struct platform_device *pdev)
+{
+     struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
+     unsigned int reg_val, ret;
+
+     ret = regmap_read(iodev->regmap_pmic, S2MPS11_REG_CTRL1, &reg_val);
+     if (ret < 0) {

regmap_read() returns an int which you assign to an unsigned int which then you compare against <0? This does not look good.

drivers/regulator/s2mps11.c out: · line 1080

Quoted 11 lines
drivers/regulator/s2mps11.c +11 -0
+             dev_crit(&pdev->dev, "could not read S2MPS11_REG_CTRL1 value\n");
+     } else {
+             /*
+              * s2mps11-pmic: S2MPS11_REG_CTRL1 reg value
+              * is 00000000000000000000000000010000
+              * clear the S2MPS11_REG_CTRL1 0x10 value to shutdown.
+              */
+             if (reg_val & BIT(4)) {
+                     ret = regmap_update_bits(iodev->regmap_pmic,
+                                              S2MPS11_REG_CTRL1,
+                                              BIT(4), BIT(0));

I don't understand. You want to update BIT(4) but the value is BIT(0)? This will clear BIT(4) but is totally unreadable.

drivers/regulator/s2mps11.c out: · line 1085

Quoted 5 lines
drivers/regulator/s2mps11.c +5 -0
+                     if (ret)
+                             dev_crit(&pdev->dev,
+                                      "could not write S2MPS11_REG_CTRL1 value\n");
+             }
+     }

The code is not readable, to many unnecessary indentations.

drivers/regulator/s2mps11.c static struct platform_driver s2mps11_pmic_driver = { · line 1102

Quoted 10 lines
drivers/regulator/s2mps11.c +3 -0
+}
+
 static const struct platform_device_id s2mps11_pmic_id[] = {
      { "s2mps11-pmic", S2MPS11X},
      { "s2mps13-pmic", S2MPS13X},

@@ -1074,6 +1099,7 @@ static struct platform_driver s2mps11_pmic_driver = {
              .name = "s2mps11-pmic",
      },
      .probe = s2mps11_pmic_probe,
+     .shutdown = s2mps11_pmic_shutdown,

The purpose of shutdown function is not to shutdown the system but to prepare the system for shutdown.

Quoted 12 lines

The patch is just wrong and you did not answered the major question - WHY you have to do this? Don't fix the problem blindly (or because some hardkernel tree for some of the boards use such patch).

Best regards,
Krzysztof

Quoted 4 lines
drivers/regulator/s2mps11.c +0 -0
      .id_table = s2mps11_pmic_id,
 };

I might me missing the bigger picture. So drop it.

-Anand Moon