Messages in this thread | | | Date | Wed, 1 Mar 2023 10:35:54 -0600 | Subject | Re: [PATCH] mfd: tps65219: Add support for soft shutdown via sys-off API | From | Andrew Davis <> |
| |
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@baylibre.com/ >> >> 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@baylibre.com> >> Suggested-by: Andrew Davis <afd@ti.com> > > 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
Then you can use this new helper devm_register_restart_handler()
>> + 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()?
Otherwise I see no major issues,
Reviewed-by: Andrew Davis <afd@ti.com>
Andrew
>> + if (ret) { >> + dev_err(tps->dev, "failed to register sys-off handler: %d\n", >> + ret); >> + return ret; >> + } >> return 0; >> } >> >> -- >> 2.34.1 >> >
| |