On 01/03/2023 17:35, Andrew Davis wrote:
On 3/1/23 10:07 AM, Lee Jones wrote:
On Fri, 03 Feb 2023, Jerome Neanne wrote:
Use new API for power-off mode support:
Link: https://lwn.net/Articles/894511/
Link: https://lore.kernel.org/all/7hfseqa7l0.fsf@xxxxxxxxxxxx/
sys-off API allows support of shutdown handler and restart handler.
Shutdown was not supported before that enhancement.
This is required for platform that are not using PSCI.
Not sure what platform doesn't have PSCI off, since you tested on
AM62-SK I'm guessing you manually disabled the PSCI off for testing?
Anyway I don't see any huge issues with the code itself, small comment
below.
Test:
- restart:
# reboot
Default is cold reset:
# cat /sys/kernel/reboot/mode
Switch boot mode to warm reset:
# echo warm > /sys/kernel/reboot/mode
- power-off:
# halt
Tested on AM62-SP-SK board.
Signed-off-by: Jerome Neanne <jneanne@xxxxxxxxxxxx>
Suggested-by: Andrew Davis <afd@xxxxxx>
A review from Andrew would be helpful here.
---
drivers/mfd/tps65219.c | 45 +++++++++++++++++++++++++++++++-----------
1 file changed, 34 insertions(+), 11 deletions(-)
diff --git a/drivers/mfd/tps65219.c b/drivers/mfd/tps65219.c
index 0e402fda206b..c134f3f6e202 100644
--- a/drivers/mfd/tps65219.c
+++ b/drivers/mfd/tps65219.c
@@ -25,25 +25,34 @@ static int tps65219_cold_reset(struct tps65219 *tps)
TPS65219_MFP_COLD_RESET_I2C_CTRL_MASK);
}
-static int tps65219_restart(struct notifier_block *this,
- unsigned long reboot_mode, void *cmd)
+static int tps65219_soft_shutdown(struct tps65219 *tps)
{
- struct tps65219 *tps;
+ return regmap_update_bits(tps->regmap, TPS65219_REG_MFP_CTRL,
+ TPS65219_MFP_I2C_OFF_REQ_MASK,
+ TPS65219_MFP_I2C_OFF_REQ_MASK);
+}
- tps = container_of(this, struct tps65219, nb);
+static int tps65219_power_off_handler(struct sys_off_data *data)
+{
+ tps65219_soft_shutdown(data->cb_data);
+ return NOTIFY_DONE;
+}
+static int tps65219_restart(struct tps65219 *tps,
+ unsigned long reboot_mode)
+{
if (reboot_mode == REBOOT_WARM)
tps65219_warm_reset(tps);
else
tps65219_cold_reset(tps);
-
return NOTIFY_DONE;
}
-static struct notifier_block pmic_rst_restart_nb = {
- .notifier_call = tps65219_restart,
- .priority = 200,
-};
+static int tps65219_restart_handler(struct sys_off_data *data)
+{
+ tps65219_restart(data->cb_data, data->mode);
+ return NOTIFY_DONE;
+}
static const struct resource tps65219_pwrbutton_resources[] = {
DEFINE_RES_IRQ_NAMED(TPS65219_INT_PB_FALLING_EDGE_DETECT,
"falling"),
@@ -269,13 +278,27 @@ static int tps65219_probe(struct i2c_client
*client)
}
}
- tps->nb = pmic_rst_restart_nb;
- ret = register_restart_handler(&tps->nb);
+ ret = devm_register_sys_off_handler(tps->dev,
+ SYS_OFF_MODE_RESTART,
+ SYS_OFF_PRIO_HIGH,
Why not default prio? SYS_OFF_PRIO_DEFAULT
I'm not completely clear about PRIO recommendations. Will follow your
suggestion.
Then you can use this new helper devm_register_restart_handler()
Sure!
+ tps65219_restart_handler,
+ tps);
+
if (ret) {
dev_err(tps->dev, "cannot register restart handler, %d\n",
ret);
return ret;
}
+ ret = devm_register_sys_off_handler(tps->dev,
+ SYS_OFF_MODE_POWER_OFF,
+ SYS_OFF_PRIO_DEFAULT,
+ tps65219_power_off_handler,
+ tps);
devm_register_power_off_handler()?
Oh yes, right, this is solving the PRIO question by construction. This
is definitely a better option
Otherwise I see no major issues,
Reviewed-by: Andrew Davis <afd@xxxxxx>
Andrew
+ if (ret) {
+ dev_err(tps->dev, "failed to register sys-off handler: %d\n",
+ ret);
+ return ret;
+ }
return 0;
}
--
2.34.1
