[PATCH 1/2] regulator: act8865: add restart handler for act8846

[PATCH 1/2] regulator: act8865: add restart handler for act8846

[Michael Niewöhner]

Rebooting radxa rock which uses act8846 results in kernel panic because the
bootloader doesn't readjust frequency or voltage for cpu correctly. This
workaround power cycles the act8846 at restart to reset the board.

Signed-off-by: Michael Niewoehner <mniewoeh@stud.hs-offenburg.de>
---
 drivers/regulator/act8865-regulator.c | 43 +++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index 2ff73d7..f8cdff3 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -27,6 +27,7 @@
 #include <linux/of_device.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/regmap.h>
+#include <linux/reboot.h>
 
 /*
  * ACT8600 Global Register Map.
@@ -133,10 +134,14 @@
 #define	ACT8865_VOLTAGE_NUM	64
 #define ACT8600_SUDCDC_VOLTAGE_NUM	255
 
+#define ACT8846_SIPC_MASK 0x01
+
 struct act8865 {
 	struct regmap *regmap;
 	int off_reg;
 	int off_mask;
+	int sipc_reg;
+	int sipc_mask;
 };
 
 static const struct regmap_config act8865_regmap_config = {
@@ -402,6 +407,17 @@ static void act8865_power_off(void)
 	while (1);
 }
 
+static int act8846_power_cycle(struct notifier_block *this,
+	unsigned long code, void *unused)
+{
+	struct act8865 *act8846;
+
+	act8846 = i2c_get_clientdata(act8865_i2c_client);
+	regmap_write(act8846->regmap, act8846->sipc_reg, act8846->sipc_mask);
+
+	return NOTIFY_DONE;
+}
+
 static int act8865_pmic_probe(struct i2c_client *client,
 			      const struct i2c_device_id *i2c_id)
 {
@@ -413,6 +429,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
 	struct act8865 *act8865;
 	unsigned long type;
 	int off_reg, off_mask;
+	int sipc_reg, sipc_mask;
 
 	pdata = dev_get_platdata(dev);
 
@@ -434,18 +451,24 @@ static int act8865_pmic_probe(struct i2c_client *client,
 		num_regulators = ARRAY_SIZE(act8600_regulators);
 		off_reg = -1;
 		off_mask = -1;
+		sipc_reg = -1;
+		sipc_mask = -1;
 		break;
 	case ACT8846:
 		regulators = act8846_regulators;
 		num_regulators = ARRAY_SIZE(act8846_regulators);
 		off_reg = ACT8846_GLB_OFF_CTRL;
 		off_mask = ACT8846_OFF_SYSMASK;
+		sipc_reg = ACT8846_GLB_OFF_CTRL;
+		sipc_mask = ACT8846_SIPC_MASK;
 		break;
 	case ACT8865:
 		regulators = act8865_regulators;
 		num_regulators = ARRAY_SIZE(act8865_regulators);
 		off_reg = ACT8865_SYS_CTRL;
 		off_mask = ACT8865_MSTROFF;
+		sipc_reg = -1;
+		sipc_mask = -1;
 		break;
 	default:
 		dev_err(dev, "invalid device id %lu\n", type);
@@ -494,6 +517,26 @@ static int act8865_pmic_probe(struct i2c_client *client,
 		}
 	}
 
+	/* Register restart handler */
+	if (of_device_is_system_power_controller(dev->of_node)
+				&& sipc_reg > 0) {
+		static struct notifier_block rockchip_restart_handler = {
+			.notifier_call = act8846_power_cycle,
+			.priority = 129,
+		};
+
+		int ret;
+
+		act8865_i2c_client = client;
+		act8865->sipc_reg = sipc_reg;
+		act8865->sipc_mask = sipc_mask;
+
+		ret = register_restart_handler(&rockchip_restart_handler);
+		if (ret)
+			pr_err("%s: cannot register restart handler, %d\n",
+			       __func__, ret);
+	}
+
 	/* Finally register devices */
 	for (i = 0; i < num_regulators; i++) {
 		const struct regulator_desc *desc = &regulators[i];
-- 
2.3.5

[PATCH 1/2] regulator: act8865: add restart handler for act8846

[Michael Niewöhner]

Rebooting radxa rock which uses act8846 results in kernel panic because the
bootloader doesn't readjust frequency or voltage for cpu correctly. This
workaround power cycles the act8846 at restart to reset the board.

Signed-off-by: Michael Niewoehner <mniewoeh-0BV18Q16miHGpFsiFCTsGXqWYbMAw+HU@public.gmane.org>
---
 drivers/regulator/act8865-regulator.c | 43 +++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index 2ff73d7..f8cdff3 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -27,6 +27,7 @@
 #include <linux/of_device.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/regmap.h>
+#include <linux/reboot.h>
 
 /*
  * ACT8600 Global Register Map.
@@ -133,10 +134,14 @@
 #define	ACT8865_VOLTAGE_NUM	64
 #define ACT8600_SUDCDC_VOLTAGE_NUM	255
 
+#define ACT8846_SIPC_MASK 0x01
+
 struct act8865 {
 	struct regmap *regmap;
 	int off_reg;
 	int off_mask;
+	int sipc_reg;
+	int sipc_mask;
 };
 
 static const struct regmap_config act8865_regmap_config = {
@@ -402,6 +407,17 @@ static void act8865_power_off(void)
 	while (1);
 }
 
+static int act8846_power_cycle(struct notifier_block *this,
+	unsigned long code, void *unused)
+{
+	struct act8865 *act8846;
+
+	act8846 = i2c_get_clientdata(act8865_i2c_client);
+	regmap_write(act8846->regmap, act8846->sipc_reg, act8846->sipc_mask);
+
+	return NOTIFY_DONE;
+}
+
 static int act8865_pmic_probe(struct i2c_client *client,
 			      const struct i2c_device_id *i2c_id)
 {
@@ -413,6 +429,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
 	struct act8865 *act8865;
 	unsigned long type;
 	int off_reg, off_mask;
+	int sipc_reg, sipc_mask;
 
 	pdata = dev_get_platdata(dev);
 
@@ -434,18 +451,24 @@ static int act8865_pmic_probe(struct i2c_client *client,
 		num_regulators = ARRAY_SIZE(act8600_regulators);
 		off_reg = -1;
 		off_mask = -1;
+		sipc_reg = -1;
+		sipc_mask = -1;
 		break;
 	case ACT8846:
 		regulators = act8846_regulators;
 		num_regulators = ARRAY_SIZE(act8846_regulators);
 		off_reg = ACT8846_GLB_OFF_CTRL;
 		off_mask = ACT8846_OFF_SYSMASK;
+		sipc_reg = ACT8846_GLB_OFF_CTRL;
+		sipc_mask = ACT8846_SIPC_MASK;
 		break;
 	case ACT8865:
 		regulators = act8865_regulators;
 		num_regulators = ARRAY_SIZE(act8865_regulators);
 		off_reg = ACT8865_SYS_CTRL;
 		off_mask = ACT8865_MSTROFF;
+		sipc_reg = -1;
+		sipc_mask = -1;
 		break;
 	default:
 		dev_err(dev, "invalid device id %lu\n", type);
@@ -494,6 +517,26 @@ static int act8865_pmic_probe(struct i2c_client *client,
 		}
 	}
 
+	/* Register restart handler */
+	if (of_device_is_system_power_controller(dev->of_node)
+				&& sipc_reg > 0) {
+		static struct notifier_block rockchip_restart_handler = {
+			.notifier_call = act8846_power_cycle,
+			.priority = 129,
+		};
+
+		int ret;
+
+		act8865_i2c_client = client;
+		act8865->sipc_reg = sipc_reg;
+		act8865->sipc_mask = sipc_mask;
+
+		ret = register_restart_handler(&rockchip_restart_handler);
+		if (ret)
+			pr_err("%s: cannot register restart handler, %d\n",
+			       __func__, ret);
+	}
+
 	/* Finally register devices */
 	for (i = 0; i < num_regulators; i++) {
 		const struct regulator_desc *desc = &regulators[i];
-- 
2.3.5

[PATCH 1/2] regulator: act8865: add restart handler for act8846

[Michael Niewöhner]

Rebooting radxa rock which uses act8846 results in kernel panic because the
bootloader doesn't readjust frequency or voltage for cpu correctly. This
workaround power cycles the act8846 at restart to reset the board.

Signed-off-by: Michael Niewoehner <mniewoeh@stud.hs-offenburg.de>
---
 drivers/regulator/act8865-regulator.c | 43 +++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index 2ff73d7..f8cdff3 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -27,6 +27,7 @@
 #include <linux/of_device.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/regmap.h>
+#include <linux/reboot.h>
 
 /*
  * ACT8600 Global Register Map.
@@ -133,10 +134,14 @@
 #define	ACT8865_VOLTAGE_NUM	64
 #define ACT8600_SUDCDC_VOLTAGE_NUM	255
 
+#define ACT8846_SIPC_MASK 0x01
+
 struct act8865 {
 	struct regmap *regmap;
 	int off_reg;
 	int off_mask;
+	int sipc_reg;
+	int sipc_mask;
 };
 
 static const struct regmap_config act8865_regmap_config = {
@@ -402,6 +407,17 @@ static void act8865_power_off(void)
 	while (1);
 }
 
+static int act8846_power_cycle(struct notifier_block *this,
+	unsigned long code, void *unused)
+{
+	struct act8865 *act8846;
+
+	act8846 = i2c_get_clientdata(act8865_i2c_client);
+	regmap_write(act8846->regmap, act8846->sipc_reg, act8846->sipc_mask);
+
+	return NOTIFY_DONE;
+}
+
 static int act8865_pmic_probe(struct i2c_client *client,
 			      const struct i2c_device_id *i2c_id)
 {
@@ -413,6 +429,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
 	struct act8865 *act8865;
 	unsigned long type;
 	int off_reg, off_mask;
+	int sipc_reg, sipc_mask;
 
 	pdata = dev_get_platdata(dev);
 
@@ -434,18 +451,24 @@ static int act8865_pmic_probe(struct i2c_client *client,
 		num_regulators = ARRAY_SIZE(act8600_regulators);
 		off_reg = -1;
 		off_mask = -1;
+		sipc_reg = -1;
+		sipc_mask = -1;
 		break;
 	case ACT8846:
 		regulators = act8846_regulators;
 		num_regulators = ARRAY_SIZE(act8846_regulators);
 		off_reg = ACT8846_GLB_OFF_CTRL;
 		off_mask = ACT8846_OFF_SYSMASK;
+		sipc_reg = ACT8846_GLB_OFF_CTRL;
+		sipc_mask = ACT8846_SIPC_MASK;
 		break;
 	case ACT8865:
 		regulators = act8865_regulators;
 		num_regulators = ARRAY_SIZE(act8865_regulators);
 		off_reg = ACT8865_SYS_CTRL;
 		off_mask = ACT8865_MSTROFF;
+		sipc_reg = -1;
+		sipc_mask = -1;
 		break;
 	default:
 		dev_err(dev, "invalid device id %lu\n", type);
@@ -494,6 +517,26 @@ static int act8865_pmic_probe(struct i2c_client *client,
 		}
 	}
 
+	/* Register restart handler */
+	if (of_device_is_system_power_controller(dev->of_node)
+				&& sipc_reg > 0) {
+		static struct notifier_block rockchip_restart_handler = {
+			.notifier_call = act8846_power_cycle,
+			.priority = 129,
+		};
+
+		int ret;
+
+		act8865_i2c_client = client;
+		act8865->sipc_reg = sipc_reg;
+		act8865->sipc_mask = sipc_mask;
+
+		ret = register_restart_handler(&rockchip_restart_handler);
+		if (ret)
+			pr_err("%s: cannot register restart handler, %d\n",
+			       __func__, ret);
+	}
+
 	/* Finally register devices */
 	for (i = 0; i < num_regulators; i++) {
 		const struct regulator_desc *desc = &regulators[i];
-- 
2.3.5

Re: [PATCH 1/2] regulator: act8865: add restart handler for act8846

[Heiko Stuebner]

Hi Michael,

Am Montag, 11. Mai 2015, 22:54:11 schrieb Michael Niewöhner:
> Rebooting radxa rock which uses act8846 results in kernel panic because the
> bootloader doesn't readjust frequency or voltage for cpu correctly. This
> workaround power cycles the act8846 at restart to reset the board.

Hmm, it might be nice to reword this, as this being a "workaround" for the 
radxarock is a bit "short sighted".

The act8846 simply provides means to reset the system when used as main pmic 
[hence the reliance on the system-power-controller] and other socs/boards 
using it as pmic might also profit from exposing this function - for example if 
there is no other means of reset.


> Signed-off-by: Michael Niewoehner <mniewoeh@stud.hs-offenburg.de>
> ---
>  drivers/regulator/act8865-regulator.c | 43
> +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/regulator/act8865-regulator.c
> b/drivers/regulator/act8865-regulator.c index 2ff73d7..f8cdff3 100644
> --- a/drivers/regulator/act8865-regulator.c
> +++ b/drivers/regulator/act8865-regulator.c
> @@ -27,6 +27,7 @@
>  #include <linux/of_device.h>
>  #include <linux/regulator/of_regulator.h>
>  #include <linux/regmap.h>
> +#include <linux/reboot.h>
> 
>  /*
>   * ACT8600 Global Register Map.
> @@ -133,10 +134,14 @@
>  #define	ACT8865_VOLTAGE_NUM	64
>  #define ACT8600_SUDCDC_VOLTAGE_NUM	255
> 
> +#define ACT8846_SIPC_MASK 0x01
> +
>  struct act8865 {
>  	struct regmap *regmap;
>  	int off_reg;
>  	int off_mask;
> +	int sipc_reg;
> +	int sipc_mask;
>  };
> 
>  static const struct regmap_config act8865_regmap_config = {
> @@ -402,6 +407,17 @@ static void act8865_power_off(void)
>  	while (1);
>  }
> 
> +static int act8846_power_cycle(struct notifier_block *this,
> +	unsigned long code, void *unused)
> +{
> +	struct act8865 *act8846;
> +
> +	act8846 = i2c_get_clientdata(act8865_i2c_client);
> +	regmap_write(act8846->regmap, act8846->sipc_reg, act8846->sipc_mask);

The act8846 is currently the only one of the three types providing such 
functionality, so until the second chip variant surfaces that provides a reset 
capability, it might be less intrusive to simply do

	regmap_write(act8846->regmap, ACT8846_GLB_OFF_CTRL, ACT8846_SIPC_MASK);

here and skip all the infrastructure like sipc_reg and sipc_mask assignment 
and handling? But I guess that is Mark's call what he likes better.


> +
> +	return NOTIFY_DONE;
> +}
> +

structs like the restart_handler normally live near the function they 
reference and not as static part of some function. And as the restart 
functionality is not rockchip-specific, it should also not be named 
rockchip_foo as below ;-)

static struct notifier_block act8846_restart_handler = {
	.notifier_call = act8846_power_cycle,
	.priority = 129,
};



>  static int act8865_pmic_probe(struct i2c_client *client,
>  			      const struct i2c_device_id *i2c_id)
>  {
> @@ -413,6 +429,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
> struct act8865 *act8865;
>  	unsigned long type;
>  	int off_reg, off_mask;
> +	int sipc_reg, sipc_mask;
> 
>  	pdata = dev_get_platdata(dev);
> 
> @@ -434,18 +451,24 @@ static int act8865_pmic_probe(struct i2c_client
> *client, num_regulators = ARRAY_SIZE(act8600_regulators);
>  		off_reg = -1;
>  		off_mask = -1;
> +		sipc_reg = -1;
> +		sipc_mask = -1;
>  		break;
>  	case ACT8846:
>  		regulators = act8846_regulators;
>  		num_regulators = ARRAY_SIZE(act8846_regulators);
>  		off_reg = ACT8846_GLB_OFF_CTRL;
>  		off_mask = ACT8846_OFF_SYSMASK;
> +		sipc_reg = ACT8846_GLB_OFF_CTRL;
> +		sipc_mask = ACT8846_SIPC_MASK;
>  		break;
>  	case ACT8865:
>  		regulators = act8865_regulators;
>  		num_regulators = ARRAY_SIZE(act8865_regulators);
>  		off_reg = ACT8865_SYS_CTRL;
>  		off_mask = ACT8865_MSTROFF;
> +		sipc_reg = -1;
> +		sipc_mask = -1;
>  		break;
>  	default:
>  		dev_err(dev, "invalid device id %lu\n", type);
> @@ -494,6 +517,26 @@ static int act8865_pmic_probe(struct i2c_client
> *client, }
>  	}

no need to add a second check for of_device_is_system_power_controller() as we 
already have a check for this property directly above, simply extend it with 
something like the following.

if (of_device_is_system_power_controller(dev->of_node)) {
	int ret;

	/* already existing code to set poweroff handler */

	if (type == ACT8846) {
		ret = register_restart_handler(&act8846_restart_handler);
		if (ret)
		pr_err("%s: cannot register restart handler, %d\n",
		       __func__, ret);
	}
}


> 
> +	/* Register restart handler */
> +	if (of_device_is_system_power_controller(dev->of_node)
> +				&& sipc_reg > 0) {
> +		static struct notifier_block rockchip_restart_handler = {
> +			.notifier_call = act8846_power_cycle,
> +			.priority = 129,
> +		};
> +
> +		int ret;
> +
> +		act8865_i2c_client = client;
> +		act8865->sipc_reg = sipc_reg;
> +		act8865->sipc_mask = sipc_mask;
> +
> +		ret = register_restart_handler(&rockchip_restart_handler);
> +		if (ret)
> +			pr_err("%s: cannot register restart handler, %d\n",
> +			       __func__, ret);
> +	}
> +
>  	/* Finally register devices */
>  	for (i = 0; i < num_regulators; i++) {
>  		const struct regulator_desc *desc = &regulators[i];

[PATCH 1/2] regulator: act8865: add restart handler for act8846

[Heiko Stuebner]

Hi Michael,

Am Montag, 11. Mai 2015, 22:54:11 schrieb Michael Niew?hner:
> Rebooting radxa rock which uses act8846 results in kernel panic because the
> bootloader doesn't readjust frequency or voltage for cpu correctly. This
> workaround power cycles the act8846 at restart to reset the board.

Hmm, it might be nice to reword this, as this being a "workaround" for the 
radxarock is a bit "short sighted".

The act8846 simply provides means to reset the system when used as main pmic 
[hence the reliance on the system-power-controller] and other socs/boards 
using it as pmic might also profit from exposing this function - for example if 
there is no other means of reset.


> Signed-off-by: Michael Niewoehner <mniewoeh@stud.hs-offenburg.de>
> ---
>  drivers/regulator/act8865-regulator.c | 43
> +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/regulator/act8865-regulator.c
> b/drivers/regulator/act8865-regulator.c index 2ff73d7..f8cdff3 100644
> --- a/drivers/regulator/act8865-regulator.c
> +++ b/drivers/regulator/act8865-regulator.c
> @@ -27,6 +27,7 @@
>  #include <linux/of_device.h>
>  #include <linux/regulator/of_regulator.h>
>  #include <linux/regmap.h>
> +#include <linux/reboot.h>
> 
>  /*
>   * ACT8600 Global Register Map.
> @@ -133,10 +134,14 @@
>  #define	ACT8865_VOLTAGE_NUM	64
>  #define ACT8600_SUDCDC_VOLTAGE_NUM	255
> 
> +#define ACT8846_SIPC_MASK 0x01
> +
>  struct act8865 {
>  	struct regmap *regmap;
>  	int off_reg;
>  	int off_mask;
> +	int sipc_reg;
> +	int sipc_mask;
>  };
> 
>  static const struct regmap_config act8865_regmap_config = {
> @@ -402,6 +407,17 @@ static void act8865_power_off(void)
>  	while (1);
>  }
> 
> +static int act8846_power_cycle(struct notifier_block *this,
> +	unsigned long code, void *unused)
> +{
> +	struct act8865 *act8846;
> +
> +	act8846 = i2c_get_clientdata(act8865_i2c_client);
> +	regmap_write(act8846->regmap, act8846->sipc_reg, act8846->sipc_mask);

The act8846 is currently the only one of the three types providing such 
functionality, so until the second chip variant surfaces that provides a reset 
capability, it might be less intrusive to simply do

	regmap_write(act8846->regmap, ACT8846_GLB_OFF_CTRL, ACT8846_SIPC_MASK);

here and skip all the infrastructure like sipc_reg and sipc_mask assignment 
and handling? But I guess that is Mark's call what he likes better.


> +
> +	return NOTIFY_DONE;
> +}
> +

structs like the restart_handler normally live near the function they 
reference and not as static part of some function. And as the restart 
functionality is not rockchip-specific, it should also not be named 
rockchip_foo as below ;-)

static struct notifier_block act8846_restart_handler = {
	.notifier_call = act8846_power_cycle,
	.priority = 129,
};



>  static int act8865_pmic_probe(struct i2c_client *client,
>  			      const struct i2c_device_id *i2c_id)
>  {
> @@ -413,6 +429,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
> struct act8865 *act8865;
>  	unsigned long type;
>  	int off_reg, off_mask;
> +	int sipc_reg, sipc_mask;
> 
>  	pdata = dev_get_platdata(dev);
> 
> @@ -434,18 +451,24 @@ static int act8865_pmic_probe(struct i2c_client
> *client, num_regulators = ARRAY_SIZE(act8600_regulators);
>  		off_reg = -1;
>  		off_mask = -1;
> +		sipc_reg = -1;
> +		sipc_mask = -1;
>  		break;
>  	case ACT8846:
>  		regulators = act8846_regulators;
>  		num_regulators = ARRAY_SIZE(act8846_regulators);
>  		off_reg = ACT8846_GLB_OFF_CTRL;
>  		off_mask = ACT8846_OFF_SYSMASK;
> +		sipc_reg = ACT8846_GLB_OFF_CTRL;
> +		sipc_mask = ACT8846_SIPC_MASK;
>  		break;
>  	case ACT8865:
>  		regulators = act8865_regulators;
>  		num_regulators = ARRAY_SIZE(act8865_regulators);
>  		off_reg = ACT8865_SYS_CTRL;
>  		off_mask = ACT8865_MSTROFF;
> +		sipc_reg = -1;
> +		sipc_mask = -1;
>  		break;
>  	default:
>  		dev_err(dev, "invalid device id %lu\n", type);
> @@ -494,6 +517,26 @@ static int act8865_pmic_probe(struct i2c_client
> *client, }
>  	}

no need to add a second check for of_device_is_system_power_controller() as we 
already have a check for this property directly above, simply extend it with 
something like the following.

if (of_device_is_system_power_controller(dev->of_node)) {
	int ret;

	/* already existing code to set poweroff handler */

	if (type == ACT8846) {
		ret = register_restart_handler(&act8846_restart_handler);
		if (ret)
		pr_err("%s: cannot register restart handler, %d\n",
		       __func__, ret);
	}
}


> 
> +	/* Register restart handler */
> +	if (of_device_is_system_power_controller(dev->of_node)
> +				&& sipc_reg > 0) {
> +		static struct notifier_block rockchip_restart_handler = {
> +			.notifier_call = act8846_power_cycle,
> +			.priority = 129,
> +		};
> +
> +		int ret;
> +
> +		act8865_i2c_client = client;
> +		act8865->sipc_reg = sipc_reg;
> +		act8865->sipc_mask = sipc_mask;
> +
> +		ret = register_restart_handler(&rockchip_restart_handler);
> +		if (ret)
> +			pr_err("%s: cannot register restart handler, %d\n",
> +			       __func__, ret);
> +	}
> +
>  	/* Finally register devices */
>  	for (i = 0; i < num_regulators; i++) {
>  		const struct regulator_desc *desc = &regulators[i];

Re: [PATCH 1/2] regulator: act8865: add restart handler for act8846

[Michael Niewöhner]

Hi Heiko,

thanks, I’ll rework the patch and the description on Thursday ;-)



Am 12.05.2015 um 22:22 schrieb Heiko Stuebner <heiko@sntech.de>:

> Hi Michael,
> 
> Am Montag, 11. Mai 2015, 22:54:11 schrieb Michael Niewöhner:
>> Rebooting radxa rock which uses act8846 results in kernel panic because the
>> bootloader doesn't readjust frequency or voltage for cpu correctly. This
>> workaround power cycles the act8846 at restart to reset the board.
> 
> Hmm, it might be nice to reword this, as this being a "workaround" for the 
> radxarock is a bit "short sighted".
> 
> The act8846 simply provides means to reset the system when used as main pmic 
> [hence the reliance on the system-power-controller] and other socs/boards 
> using it as pmic might also profit from exposing this function - for example if 
> there is no other means of reset.
> 
> 
>> Signed-off-by: Michael Niewoehner <mniewoeh@stud.hs-offenburg.de>
>> ---
>> drivers/regulator/act8865-regulator.c | 43
>> +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
>> 
>> diff --git a/drivers/regulator/act8865-regulator.c
>> b/drivers/regulator/act8865-regulator.c index 2ff73d7..f8cdff3 100644
>> --- a/drivers/regulator/act8865-regulator.c
>> +++ b/drivers/regulator/act8865-regulator.c
>> @@ -27,6 +27,7 @@
>> #include <linux/of_device.h>
>> #include <linux/regulator/of_regulator.h>
>> #include <linux/regmap.h>
>> +#include <linux/reboot.h>
>> 
>> /*
>>  * ACT8600 Global Register Map.
>> @@ -133,10 +134,14 @@
>> #define	ACT8865_VOLTAGE_NUM	64
>> #define ACT8600_SUDCDC_VOLTAGE_NUM	255
>> 
>> +#define ACT8846_SIPC_MASK 0x01
>> +
>> struct act8865 {
>> 	struct regmap *regmap;
>> 	int off_reg;
>> 	int off_mask;
>> +	int sipc_reg;
>> +	int sipc_mask;
>> };
>> 
>> static const struct regmap_config act8865_regmap_config = {
>> @@ -402,6 +407,17 @@ static void act8865_power_off(void)
>> 	while (1);
>> }
>> 
>> +static int act8846_power_cycle(struct notifier_block *this,
>> +	unsigned long code, void *unused)
>> +{
>> +	struct act8865 *act8846;
>> +
>> +	act8846 = i2c_get_clientdata(act8865_i2c_client);
>> +	regmap_write(act8846->regmap, act8846->sipc_reg, act8846->sipc_mask);
> 
> The act8846 is currently the only one of the three types providing such 
> functionality, so until the second chip variant surfaces that provides a reset 
> capability, it might be less intrusive to simply do
> 
> 	regmap_write(act8846->regmap, ACT8846_GLB_OFF_CTRL, ACT8846_SIPC_MASK);
> 
> here and skip all the infrastructure like sipc_reg and sipc_mask assignment 
> and handling? But I guess that is Mark's call what he likes better.
> 
> 
>> +
>> +	return NOTIFY_DONE;
>> +}
>> +
> 
> structs like the restart_handler normally live near the function they 
> reference and not as static part of some function. And as the restart 
> functionality is not rockchip-specific, it should also not be named 
> rockchip_foo as below ;-)
> 
> static struct notifier_block act8846_restart_handler = {
> 	.notifier_call = act8846_power_cycle,
> 	.priority = 129,
> };
> 
> 
> 
>> static int act8865_pmic_probe(struct i2c_client *client,
>> 			      const struct i2c_device_id *i2c_id)
>> {
>> @@ -413,6 +429,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
>> struct act8865 *act8865;
>> 	unsigned long type;
>> 	int off_reg, off_mask;
>> +	int sipc_reg, sipc_mask;
>> 
>> 	pdata = dev_get_platdata(dev);
>> 
>> @@ -434,18 +451,24 @@ static int act8865_pmic_probe(struct i2c_client
>> *client, num_regulators = ARRAY_SIZE(act8600_regulators);
>> 		off_reg = -1;
>> 		off_mask = -1;
>> +		sipc_reg = -1;
>> +		sipc_mask = -1;
>> 		break;
>> 	case ACT8846:
>> 		regulators = act8846_regulators;
>> 		num_regulators = ARRAY_SIZE(act8846_regulators);
>> 		off_reg = ACT8846_GLB_OFF_CTRL;
>> 		off_mask = ACT8846_OFF_SYSMASK;
>> +		sipc_reg = ACT8846_GLB_OFF_CTRL;
>> +		sipc_mask = ACT8846_SIPC_MASK;
>> 		break;
>> 	case ACT8865:
>> 		regulators = act8865_regulators;
>> 		num_regulators = ARRAY_SIZE(act8865_regulators);
>> 		off_reg = ACT8865_SYS_CTRL;
>> 		off_mask = ACT8865_MSTROFF;
>> +		sipc_reg = -1;
>> +		sipc_mask = -1;
>> 		break;
>> 	default:
>> 		dev_err(dev, "invalid device id %lu\n", type);
>> @@ -494,6 +517,26 @@ static int act8865_pmic_probe(struct i2c_client
>> *client, }
>> 	}
> 
> no need to add a second check for of_device_is_system_power_controller() as we 
> already have a check for this property directly above, simply extend it with 
> something like the following.
> 
> if (of_device_is_system_power_controller(dev->of_node)) {
> 	int ret;
> 
> 	/* already existing code to set poweroff handler */
> 
> 	if (type == ACT8846) {
> 		ret = register_restart_handler(&act8846_restart_handler);
> 		if (ret)
> 		pr_err("%s: cannot register restart handler, %d\n",
> 		       __func__, ret);
> 	}
> }
> 
> 
>> 
>> +	/* Register restart handler */
>> +	if (of_device_is_system_power_controller(dev->of_node)
>> +				&& sipc_reg > 0) {
>> +		static struct notifier_block rockchip_restart_handler = {
>> +			.notifier_call = act8846_power_cycle,
>> +			.priority = 129,
>> +		};
>> +
>> +		int ret;
>> +
>> +		act8865_i2c_client = client;
>> +		act8865->sipc_reg = sipc_reg;
>> +		act8865->sipc_mask = sipc_mask;
>> +
>> +		ret = register_restart_handler(&rockchip_restart_handler);
>> +		if (ret)
>> +			pr_err("%s: cannot register restart handler, %d\n",
>> +			       __func__, ret);
>> +	}
>> +
>> 	/* Finally register devices */
>> 	for (i = 0; i < num_regulators; i++) {
>> 		const struct regulator_desc *desc = &regulators[i];

[PATCH 1/2] regulator: act8865: add restart handler for act8846

[Michael Niewöhner]

Hi Heiko,

thanks, I?ll rework the patch and the description on Thursday ;-)



Am 12.05.2015 um 22:22 schrieb Heiko Stuebner <heiko@sntech.de>:

> Hi Michael,
> 
> Am Montag, 11. Mai 2015, 22:54:11 schrieb Michael Niew?hner:
>> Rebooting radxa rock which uses act8846 results in kernel panic because the
>> bootloader doesn't readjust frequency or voltage for cpu correctly. This
>> workaround power cycles the act8846 at restart to reset the board.
> 
> Hmm, it might be nice to reword this, as this being a "workaround" for the 
> radxarock is a bit "short sighted".
> 
> The act8846 simply provides means to reset the system when used as main pmic 
> [hence the reliance on the system-power-controller] and other socs/boards 
> using it as pmic might also profit from exposing this function - for example if 
> there is no other means of reset.
> 
> 
>> Signed-off-by: Michael Niewoehner <mniewoeh@stud.hs-offenburg.de>
>> ---
>> drivers/regulator/act8865-regulator.c | 43
>> +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
>> 
>> diff --git a/drivers/regulator/act8865-regulator.c
>> b/drivers/regulator/act8865-regulator.c index 2ff73d7..f8cdff3 100644
>> --- a/drivers/regulator/act8865-regulator.c
>> +++ b/drivers/regulator/act8865-regulator.c
>> @@ -27,6 +27,7 @@
>> #include <linux/of_device.h>
>> #include <linux/regulator/of_regulator.h>
>> #include <linux/regmap.h>
>> +#include <linux/reboot.h>
>> 
>> /*
>>  * ACT8600 Global Register Map.
>> @@ -133,10 +134,14 @@
>> #define	ACT8865_VOLTAGE_NUM	64
>> #define ACT8600_SUDCDC_VOLTAGE_NUM	255
>> 
>> +#define ACT8846_SIPC_MASK 0x01
>> +
>> struct act8865 {
>> 	struct regmap *regmap;
>> 	int off_reg;
>> 	int off_mask;
>> +	int sipc_reg;
>> +	int sipc_mask;
>> };
>> 
>> static const struct regmap_config act8865_regmap_config = {
>> @@ -402,6 +407,17 @@ static void act8865_power_off(void)
>> 	while (1);
>> }
>> 
>> +static int act8846_power_cycle(struct notifier_block *this,
>> +	unsigned long code, void *unused)
>> +{
>> +	struct act8865 *act8846;
>> +
>> +	act8846 = i2c_get_clientdata(act8865_i2c_client);
>> +	regmap_write(act8846->regmap, act8846->sipc_reg, act8846->sipc_mask);
> 
> The act8846 is currently the only one of the three types providing such 
> functionality, so until the second chip variant surfaces that provides a reset 
> capability, it might be less intrusive to simply do
> 
> 	regmap_write(act8846->regmap, ACT8846_GLB_OFF_CTRL, ACT8846_SIPC_MASK);
> 
> here and skip all the infrastructure like sipc_reg and sipc_mask assignment 
> and handling? But I guess that is Mark's call what he likes better.
> 
> 
>> +
>> +	return NOTIFY_DONE;
>> +}
>> +
> 
> structs like the restart_handler normally live near the function they 
> reference and not as static part of some function. And as the restart 
> functionality is not rockchip-specific, it should also not be named 
> rockchip_foo as below ;-)
> 
> static struct notifier_block act8846_restart_handler = {
> 	.notifier_call = act8846_power_cycle,
> 	.priority = 129,
> };
> 
> 
> 
>> static int act8865_pmic_probe(struct i2c_client *client,
>> 			      const struct i2c_device_id *i2c_id)
>> {
>> @@ -413,6 +429,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
>> struct act8865 *act8865;
>> 	unsigned long type;
>> 	int off_reg, off_mask;
>> +	int sipc_reg, sipc_mask;
>> 
>> 	pdata = dev_get_platdata(dev);
>> 
>> @@ -434,18 +451,24 @@ static int act8865_pmic_probe(struct i2c_client
>> *client, num_regulators = ARRAY_SIZE(act8600_regulators);
>> 		off_reg = -1;
>> 		off_mask = -1;
>> +		sipc_reg = -1;
>> +		sipc_mask = -1;
>> 		break;
>> 	case ACT8846:
>> 		regulators = act8846_regulators;
>> 		num_regulators = ARRAY_SIZE(act8846_regulators);
>> 		off_reg = ACT8846_GLB_OFF_CTRL;
>> 		off_mask = ACT8846_OFF_SYSMASK;
>> +		sipc_reg = ACT8846_GLB_OFF_CTRL;
>> +		sipc_mask = ACT8846_SIPC_MASK;
>> 		break;
>> 	case ACT8865:
>> 		regulators = act8865_regulators;
>> 		num_regulators = ARRAY_SIZE(act8865_regulators);
>> 		off_reg = ACT8865_SYS_CTRL;
>> 		off_mask = ACT8865_MSTROFF;
>> +		sipc_reg = -1;
>> +		sipc_mask = -1;
>> 		break;
>> 	default:
>> 		dev_err(dev, "invalid device id %lu\n", type);
>> @@ -494,6 +517,26 @@ static int act8865_pmic_probe(struct i2c_client
>> *client, }
>> 	}
> 
> no need to add a second check for of_device_is_system_power_controller() as we 
> already have a check for this property directly above, simply extend it with 
> something like the following.
> 
> if (of_device_is_system_power_controller(dev->of_node)) {
> 	int ret;
> 
> 	/* already existing code to set poweroff handler */
> 
> 	if (type == ACT8846) {
> 		ret = register_restart_handler(&act8846_restart_handler);
> 		if (ret)
> 		pr_err("%s: cannot register restart handler, %d\n",
> 		       __func__, ret);
> 	}
> }
> 
> 
>> 
>> +	/* Register restart handler */
>> +	if (of_device_is_system_power_controller(dev->of_node)
>> +				&& sipc_reg > 0) {
>> +		static struct notifier_block rockchip_restart_handler = {
>> +			.notifier_call = act8846_power_cycle,
>> +			.priority = 129,
>> +		};
>> +
>> +		int ret;
>> +
>> +		act8865_i2c_client = client;
>> +		act8865->sipc_reg = sipc_reg;
>> +		act8865->sipc_mask = sipc_mask;
>> +
>> +		ret = register_restart_handler(&rockchip_restart_handler);
>> +		if (ret)
>> +			pr_err("%s: cannot register restart handler, %d\n",
>> +			       __func__, ret);
>> +	}
>> +
>> 	/* Finally register devices */
>> 	for (i = 0; i < num_regulators; i++) {
>> 		const struct regulator_desc *desc = &regulators[i];

Re: [PATCH 1/2] regulator: act8865: add restart handler for act8846

[Michael Niewöhner]

I reworked the patch and here it is ;-)


The act8846 provides means to reset the system when used as main pmic.
This patch adds a restart handler for power cycling the regulator thus resetting
the board / soc when system-power-controller in dts is set.
This can be useful for example if there is no other means of reset.

Signed-off-by: Michael Niewoehner <mniewoeh@stud.hs-offenburg.de>
---
diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index 2ff73d7..836d10b 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -27,6 +27,7 @@
 #include <linux/of_device.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/regmap.h>
+#include <linux/reboot.h>
 
 /*
  * ACT8600 Global Register Map.
@@ -133,6 +134,8 @@
 #define	ACT8865_VOLTAGE_NUM	64
 #define ACT8600_SUDCDC_VOLTAGE_NUM	255
 
+#define ACT8846_SIPC_MASK 0x01
+
 struct act8865 {
 	struct regmap *regmap;
 	int off_reg;
@@ -402,6 +405,22 @@ static void act8865_power_off(void)
 	while (1);
 }
 
+static int act8846_power_cycle(struct notifier_block *this,
+	unsigned long code, void *unused)
+{
+	struct act8865 *act8846;
+
+	act8846 = i2c_get_clientdata(act8865_i2c_client);
+	regmap_write(act8846->regmap, ACT8846_GLB_OFF_CTRL, ACT8846_SIPC_MASK);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block act8846_restart_handler = {
+	.notifier_call = act8846_power_cycle,
+	.priority = 129,
+};
+
 static int act8865_pmic_probe(struct i2c_client *client,
 			      const struct i2c_device_id *i2c_id)
 {
@@ -484,6 +503,8 @@ static int act8865_pmic_probe(struct i2c_client *client,
 	}
 
 	if (of_device_is_system_power_controller(dev->of_node)) {
+		int ret;
+
 		if (!pm_power_off && (off_reg > 0)) {
 			act8865_i2c_client = client;
 			act8865->off_reg = off_reg;
@@ -492,6 +513,14 @@ static int act8865_pmic_probe(struct i2c_client *client,
 		} else {
 			dev_err(dev, "Failed to set poweroff capability, already defined\n");
 		}
+
+		if (type == ACT8846) {
+			act8865_i2c_client = client;
+			ret = register_restart_handler(&act8846_restart_handler);
+			if (ret)
+				pr_err("%s: cannot register restart handler, %d\n",
+					__func__, ret);
+		}
 	}
 
 	/* Finally register devices */

[PATCH 1/2] regulator: act8865: add restart handler for act8846

[Michael Niewöhner]

I reworked the patch and here it is ;-)


The act8846 provides means to reset the system when used as main pmic.
This patch adds a restart handler for power cycling the regulator thus resetting
the board / soc when system-power-controller in dts is set.
This can be useful for example if there is no other means of reset.

Signed-off-by: Michael Niewoehner <mniewoeh@stud.hs-offenburg.de>
---
diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
index 2ff73d7..836d10b 100644
--- a/drivers/regulator/act8865-regulator.c
+++ b/drivers/regulator/act8865-regulator.c
@@ -27,6 +27,7 @@
 #include <linux/of_device.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/regmap.h>
+#include <linux/reboot.h>
 
 /*
  * ACT8600 Global Register Map.
@@ -133,6 +134,8 @@
 #define	ACT8865_VOLTAGE_NUM	64
 #define ACT8600_SUDCDC_VOLTAGE_NUM	255
 
+#define ACT8846_SIPC_MASK 0x01
+
 struct act8865 {
 	struct regmap *regmap;
 	int off_reg;
@@ -402,6 +405,22 @@ static void act8865_power_off(void)
 	while (1);
 }
 
+static int act8846_power_cycle(struct notifier_block *this,
+	unsigned long code, void *unused)
+{
+	struct act8865 *act8846;
+
+	act8846 = i2c_get_clientdata(act8865_i2c_client);
+	regmap_write(act8846->regmap, ACT8846_GLB_OFF_CTRL, ACT8846_SIPC_MASK);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block act8846_restart_handler = {
+	.notifier_call = act8846_power_cycle,
+	.priority = 129,
+};
+
 static int act8865_pmic_probe(struct i2c_client *client,
 			      const struct i2c_device_id *i2c_id)
 {
@@ -484,6 +503,8 @@ static int act8865_pmic_probe(struct i2c_client *client,
 	}
 
 	if (of_device_is_system_power_controller(dev->of_node)) {
+		int ret;
+
 		if (!pm_power_off && (off_reg > 0)) {
 			act8865_i2c_client = client;
 			act8865->off_reg = off_reg;
@@ -492,6 +513,14 @@ static int act8865_pmic_probe(struct i2c_client *client,
 		} else {
 			dev_err(dev, "Failed to set poweroff capability, already defined\n");
 		}
+
+		if (type == ACT8846) {
+			act8865_i2c_client = client;
+			ret = register_restart_handler(&act8846_restart_handler);
+			if (ret)
+				pr_err("%s: cannot register restart handler, %d\n",
+					__func__, ret);
+		}
 	}
 
 	/* Finally register devices */