From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753766AbbELUWN (ORCPT ); Tue, 12 May 2015 16:22:13 -0400 Received: from gloria.sntech.de ([95.129.55.99]:57006 "EHLO gloria.sntech.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752533AbbELUWK convert rfc822-to-8bit (ORCPT ); Tue, 12 May 2015 16:22:10 -0400 From: Heiko Stuebner To: Michael =?ISO-8859-1?Q?Niew=F6hner?= Cc: lgirdwood@gmail.com, broonie@kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org Subject: Re: [PATCH 1/2] regulator: act8865: add restart handler for act8846 Date: Tue, 12 May 2015 22:22:56 +0200 Message-ID: <2204003.IdqJV6LE2e@phil> User-Agent: KMail/4.14.1 (Linux/3.19.0+; KDE/4.14.2; x86_64; ; ) In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT Content-Type: text/plain; charset="iso-8859-1" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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 > #include > #include > +#include > > /* > * 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 = ®ulators[i]; From mboxrd@z Thu Jan 1 00:00:00 1970 From: heiko@sntech.de (Heiko Stuebner) Date: Tue, 12 May 2015 22:22:56 +0200 Subject: [PATCH 1/2] regulator: act8865: add restart handler for act8846 In-Reply-To: References: Message-ID: <2204003.IdqJV6LE2e@phil> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 > --- > 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 > #include > #include > +#include > > /* > * 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 = ®ulators[i];