Messages in this thread | | | Date | Mon, 30 Jan 2023 09:31:26 +0100 | From | Neil Armstrong <> | Subject | Re: [PATCH v2] power: reset: add Odroid Go Ultra poweroff driver |
| |
On 30/01/2023 00:47, Sebastian Reichel wrote: > Hi, > > On Thu, Jan 26, 2023 at 06:11:21PM +0100, Neil Armstrong wrote: >> The Hardkernel Odroid Go Ultra poweroff scheme requires requesting a poweroff >> to its two PMICs in order, this represents the poweroff scheme needed to complete >> a clean poweroff of the system. >> >> This implement this scheme by implementing a self registering driver to permit >> using probe defer until both pmics are finally probed. >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> Previous submission was at [1], but I converted it to an independent >> platform device with device auto registration to permit waiting for >> both the PMICs drivers to probe. >> >> [1] https://lore.kernel.org/all/20221031-b4-odroid-go-ultra-initial-v1-2-42e3dbea86d5@linaro.org/ >> --- >> Changes in v2: >> - Switched to devm_register_sys_off_handler() >> - Link to v1: https://lore.kernel.org/r/20221031-b4-odroid-go-ultra-initial-v1-2-42e3dbea86d5@linaro.org >> --- >> drivers/power/reset/Kconfig | 7 ++ >> drivers/power/reset/Makefile | 1 + >> drivers/power/reset/odroid-go-ultra-poweroff.c | 151 +++++++++++++++++++++++++ >> 3 files changed, 159 insertions(+) >> >> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig >> index a8c46ba5878f..26860c2e05a9 100644 >> --- a/drivers/power/reset/Kconfig >> +++ b/drivers/power/reset/Kconfig >> @@ -141,6 +141,13 @@ config POWER_RESET_OCELOT_RESET >> help >> This driver supports restart for Microsemi Ocelot SoC and similar. >> >> +config POWER_RESET_ODROID_GO_ULTRA_POWEROFF >> + bool "Odroid Go Ultra power-off driver" >> + depends on ARCH_MESON || COMPILE_TEST >> + depends on MFD_RK808 > > You are not accessing any symbols from the MFD driver, so you can > append "|| COMPILE_TEST". > > It will be fun to coordinate the merging, because of my series > renaming it to MFD_RK8XX :/ > > https://lore.kernel.org/all/20230127181244.160887-1-sebastian.reichel@collabora.com/
If I do not depend anymore on `struct rk808`, so I don't depend anymore on it, but if it doesn't probe I won't get the regmap from the i2c device.
So it will be a runtime dependency instead, so it's ok to drop.
But I'll need to add a depend to guard of_find_i2c_device_by_node()
> >> + help >> + This driver supports Power off for Odroid Go Ultra device. >> + >> config POWER_RESET_OXNAS >> bool "OXNAS SoC restart driver" >> depends on ARCH_OXNAS >> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile >> index 0a39424fc558..d763e6735ee3 100644 >> --- a/drivers/power/reset/Makefile >> +++ b/drivers/power/reset/Makefile >> @@ -17,6 +17,7 @@ obj-$(CONFIG_POWER_RESET_MT6323) += mt6323-poweroff.o >> obj-$(CONFIG_POWER_RESET_OXNAS) += oxnas-restart.o >> obj-$(CONFIG_POWER_RESET_QCOM_PON) += qcom-pon.o >> obj-$(CONFIG_POWER_RESET_OCELOT_RESET) += ocelot-reset.o >> +obj-$(CONFIG_POWER_RESET_ODROID_GO_ULTRA_POWEROFF) += odroid-go-ultra-poweroff.o >> obj-$(CONFIG_POWER_RESET_PIIX4_POWEROFF) += piix4-poweroff.o >> obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o >> obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o >> diff --git a/drivers/power/reset/odroid-go-ultra-poweroff.c b/drivers/power/reset/odroid-go-ultra-poweroff.c >> new file mode 100644 >> index 000000000000..51f54e65c927 >> --- /dev/null >> +++ b/drivers/power/reset/odroid-go-ultra-poweroff.c >> @@ -0,0 +1,151 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright (c) 2023 Neil Armstrong <neil.armstrong@linaro.org> >> + */ >> +#include <linux/kernel.h> >> +#include <linux/init.h> >> +#include <linux/of_platform.h> >> +#include <linux/mfd/rk808.h> >> +#include <linux/regmap.h> >> +#include <linux/module.h> >> +#include <linux/reboot.h> >> +#include <linux/i2c.h> >> + >> +/* >> + * The Odroid Go Ultra has 2 PMICs: >> + * - RK818 (manages the battery and USB-C power supply) >> + * - RK817 >> + * Both PMICs feeds power to the S922X SoC, so they must be powered-off in sequence. >> + * Vendor does power-off the RK817 first, then the RK818 so here we follow this sequence. >> + */ >> + >> +struct odroid_go_ultra_poweroff_data { >> + struct device *dev; >> + struct rk808 *rk817; >> + struct rk808 *rk818; >> +}; >> + >> +static int odroid_go_ultra_poweroff_prepare(struct sys_off_data *data) >> +{ >> + struct odroid_go_ultra_poweroff_data *poweroff_data = data->cb_data; >> + int ret; >> + >> + dev_info(poweroff_data->dev, "Setting PMICs for power off"); >> + >> + /* RK817 */ >> + ret = regmap_update_bits(poweroff_data->rk817->regmap, RK817_SYS_CFG(3), DEV_OFF, DEV_OFF); >> + if (ret) { >> + dev_err(poweroff_data->dev, "failed to poweroff rk817\n"); >> + return notifier_from_errno(ret); >> + } >> + >> + /* RK818 */ >> + ret = regmap_update_bits(poweroff_data->rk818->regmap, RK818_DEVCTRL_REG, DEV_OFF, DEV_OFF); >> + if (ret) { >> + dev_err(poweroff_data->dev, "failed to poweroff rk818\n"); >> + return notifier_from_errno(ret); >> + } >> + >> + return NOTIFY_OK; >> +} >> + >> +static int odroid_go_ultra_poweroff_get_pmic_drvdata(const char *compatible, struct rk808 **pmic) >> +{ >> + struct device_node *pmic_node; >> + struct i2c_client *pmic_client; >> + >> + pmic_node = of_find_compatible_node(NULL, NULL, compatible); >> + if (!pmic_node) >> + return -ENODEV; >> + >> + pmic_client = of_find_i2c_device_by_node(pmic_node); >> + of_node_put(pmic_node); >> + if (!pmic_client) >> + return -EPROBE_DEFER; >> + >> + *pmic = i2c_get_clientdata(pmic_client); > > You are only using the regmap, so you can skip the rk808 struct step > and directly get and store the regmap: > > *pmic = dev_get_regmap(&pmic_client->dev, NULL);
Indeed ti makes things much simpler.
> >> + put_device(&pmic_client->dev); > > Technically this allows releasing the PMIC device with the poweroff > driver still probed. I guess it's mostly a theoretical thing in this > case, but people tend to cargo copy. I suggest to store the struct > device handle in odroid_go_ultra_poweroff_data and then use > dev_get_regmap() in the poweroff handler. Finally add a put_device() > via devm_add_action_or_reset().
Indeed, this makes it much cleaner.
> >> + if (!*pmic) >> + return -EPROBE_DEFER; >> + >> + return 0; >> +} >> + >> +static int odroid_go_ultra_poweroff_probe(struct platform_device *pdev) >> +{ >> + struct odroid_go_ultra_poweroff_data *poweroff_data; >> + int ret; >> + >> + poweroff_data = devm_kzalloc(&pdev->dev, sizeof(*poweroff_data), GFP_KERNEL); >> + if (!poweroff_data) >> + return -ENOMEM; >> + >> + dev_set_drvdata(&pdev->dev, poweroff_data); >> + >> + /* RK818 */ >> + ret = odroid_go_ultra_poweroff_get_pmic_drvdata("rockchip,rk818", >> + &poweroff_data->rk818); >> + if (ret) >> + return dev_err_probe(&pdev->dev, ret, "failed to get rk818 mfd data\n"); >> + >> + /* RK817 */ >> + ret = odroid_go_ultra_poweroff_get_pmic_drvdata("rockchip,rk817", >> + &poweroff_data->rk817); >> + if (ret) >> + return dev_err_probe(&pdev->dev, ret, "failed to get rk817 mfd data\n"); >> + >> + /* Register as SYS_OFF_MODE_POWER_OFF_PREPARE because regmap_update_bits may sleep */ >> + ret = devm_register_sys_off_handler(&pdev->dev, >> + SYS_OFF_MODE_POWER_OFF_PREPARE, >> + SYS_OFF_PRIO_DEFAULT, >> + odroid_go_ultra_poweroff_prepare, >> + poweroff_data); >> + if (ret) >> + return dev_err_probe(&pdev->dev, ret, "failed to register sys-off handler\n"); >> + >> + dev_info(&pdev->dev, "Registered Power-Off handler\n"); >> + >> + return 0; >> +} >> + >> +static struct platform_device *pdev; >> + >> +static struct platform_driver odroid_go_ultra_poweroff_driver = { >> + .driver = { >> + .name = "odroid-go-ultra-poweroff", >> + }, >> + .probe = odroid_go_ultra_poweroff_probe, >> +}; >> + >> +static int __init odroid_go_ultra_poweroff_init(void) >> +{ >> + int ret; >> + >> + /* Only create when running on the Odroid Go Ultra device */ >> + if (!of_device_is_compatible(of_root, "hardkernel,odroid-go-ultra")) >> + return -ENODEV; >> + >> + ret = platform_driver_register(&odroid_go_ultra_poweroff_driver); >> + if (ret) >> + return ret; >> + >> + pdev = platform_device_register_resndata(NULL, "odroid-go-ultra-poweroff", -1, >> + NULL, 0, NULL, 0); >> + >> + return PTR_ERR_OR_ZERO(pdev); > > This is missing platform_driver_unregister() in the error path.
Ack
> >> +} >> + >> +static void __exit odroid_go_ultra_poweroff_exit(void) >> +{ > > if (!of_device_is_compatible(of_root, "hardkernel,odroid-go-ultra")) > return;
Ack
> >> + platform_device_unregister(pdev); >> + platform_driver_unregister(&odroid_go_ultra_poweroff_driver); >> +} >> + >> +module_init(odroid_go_ultra_poweroff_init); >> +module_exit(odroid_go_ultra_poweroff_exit); >> + >> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>"); >> +MODULE_DESCRIPTION("Odroid Go Ultra poweroff driver"); >> +MODULE_LICENSE("GPL"); > > Thanks, > > -- Sebastian
Thanks, Neil
| |