[PATCH v1 0/6] Introduce power off call chain API

[PATCH v1 0/6] Introduce power off call chain API

[Dmitry Osipenko]

Introduce power off call chain API that is inspired by the restart API.
It allows to have multiple power off handlers invoked along the chain
until system is powered off. For the starter this series converts couple
NVIDIA Tegra drivers to the new API. Existing pm_power_off() method
stays around and may be removed once all users will adopt the new API.

There were couple attempts in the past to add power off API from
Guenter Roeck and Thierry Reding, but they were never completed. This
is a somewhat simplified version which doesn't try to convert whole kernel
to the new API at once, but solves immediate practical problem that we
have on Nexus 7 Android tablet where device needs to chain power off
methods.

Dmitry Osipenko (6):
  notifier: Add blocking_notifier_call_chain_empty()
  kernel: Add power off handler call chain API
  kernel: Add devm_register_restart_handler()
  mfd: max77620: Use power off call chain API
  soc/tegra: pmc: Use devm_register_restart_handler()
  soc/tegra: pmc: Add power off handler

 drivers/mfd/max77620.c       |  22 ++++--
 drivers/soc/tegra/pmc.c      |  62 ++++++++++++----
 include/linux/mfd/max77620.h |   2 +
 include/linux/notifier.h     |   2 +
 include/linux/reboot.h       |   7 ++
 kernel/notifier.c            |  14 ++++
 kernel/reboot.c              | 138 ++++++++++++++++++++++++++++++++++-
 7 files changed, 225 insertions(+), 22 deletions(-)

-- 
2.32.0

[PATCH v1 1/6] notifier: Add blocking_notifier_call_chain_empty()

[Dmitry Osipenko]

Add blocking_notifier_call_chain_empty() that returns true if call chain
is empty and false otherwise.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 include/linux/notifier.h |  2 ++
 kernel/notifier.c        | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index 87069b8459af..f0b1d600d9f6 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -173,6 +173,8 @@ extern int blocking_notifier_call_chain_robust(struct blocking_notifier_head *nh
 extern int raw_notifier_call_chain_robust(struct raw_notifier_head *nh,
 		unsigned long val_up, unsigned long val_down, void *v);
 
+extern bool blocking_notifier_call_chain_empty(struct blocking_notifier_head *nh);
+
 #define NOTIFY_DONE		0x0000		/* Don't care */
 #define NOTIFY_OK		0x0001		/* Suits me */
 #define NOTIFY_STOP_MASK	0x8000		/* Don't call further */
diff --git a/kernel/notifier.c b/kernel/notifier.c
index b8251dc0bc0f..1f7ba8988b90 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -322,6 +322,20 @@ int blocking_notifier_call_chain(struct blocking_notifier_head *nh,
 }
 EXPORT_SYMBOL_GPL(blocking_notifier_call_chain);
 
+/**
+ *	blocking_notifier_call_chain_empty - Check whether notifier chain is empty
+ *	@nh: Pointer to head of the blocking notifier chain
+ *
+ *	Checks whether notifier chain is empty.
+ *
+ *	Returns true is notifier chain is empty, false otherwise.
+ */
+bool blocking_notifier_call_chain_empty(struct blocking_notifier_head *nh)
+{
+	return !rcu_access_pointer(nh->head);
+}
+EXPORT_SYMBOL_GPL(blocking_notifier_call_chain_empty);
+
 /*
  *	Raw notifier chain routines.  There is no protection;
  *	the caller must provide it.  Use at your own risk!
-- 
2.32.0

[PATCH v1 2/6] kernel: Add power off handler call chain API

[Dmitry Osipenko]

SoC platforms often have multiple options for performing system's
power off. Meanwhile kernel is limited to a single option. Add power off
handler call chain API, which is inspired by the restart API. The old
pm_power_off method will be kept around till all drivers are converted
to the new API.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 include/linux/reboot.h |   5 ++
 kernel/reboot.c        | 111 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 115 insertions(+), 1 deletion(-)

diff --git a/include/linux/reboot.h b/include/linux/reboot.h
index af907a3d68d1..f72fb8729608 100644
--- a/include/linux/reboot.h
+++ b/include/linux/reboot.h
@@ -49,6 +49,11 @@ extern int register_restart_handler(struct notifier_block *);
 extern int unregister_restart_handler(struct notifier_block *);
 extern void do_kernel_restart(char *cmd);
 
+extern int register_poweroff_handler(struct notifier_block *);
+extern int unregister_poweroff_handler(struct notifier_block *);
+
+extern int devm_register_poweroff_handler(struct device *, struct notifier_block *);
+
 /*
  * Architecture-specific implementations of sys_reboot commands.
  */
diff --git a/kernel/reboot.c b/kernel/reboot.c
index f7440c0c7e43..6e90afec2bb8 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -280,6 +280,113 @@ void kernel_halt(void)
 }
 EXPORT_SYMBOL_GPL(kernel_halt);
 
+/*
+ *	Notifier list for kernel code which wants to be called
+ *	to power off the system.
+ */
+static BLOCKING_NOTIFIER_HEAD(poweroff_handler_list);
+
+/**
+ *	register_poweroff_handler - Register function to be called to power off
+ *				    the system
+ *	@nb: Info about handler function to be called
+ *	@nb->priority:	Handler priority. Handlers should follow the
+ *			following guidelines for setting priorities.
+ *			0:	Poweroff handler of last resort,
+ *				with limited poweroff capabilities
+ *			128:	Default poweroff handler; use if no other
+ *				poweroff handler is expected to be available,
+ *				and/or if poweroff functionality is
+ *				sufficient to poweroff the entire system
+ *			255:	Highest priority poweroff handler, will
+ *				preempt all other poweroff handlers
+ *
+ *	Registers a function with code to be called to power off the
+ *	system.
+ *
+ *	Registered functions will be called as last step of the poweroff
+ *	sequence.
+ *
+ *	Registered functions are expected to power off the system immediately.
+ *	If more than one function is registered, the poweroff handler priority
+ *	selects which function will be called first.
+ *
+ *	Poweroff handlers are expected to be registered from non-architecture
+ *	code, typically from drivers. A typical use case would be a system
+ *	where poweroff functionality is provided through a PMIC. Multiple
+ *	poweroff handlers may exist; for example, one poweroff handler might
+ *	turn off the entire system, while another only turns off part of
+ *	system. In such cases, the poweroff handler which only disables part
+ *	of the hardware is expected to register with low priority to ensure
+ *	that it only runs if no other means to power off the system is
+ *	available.
+ *
+ *	Currently always returns zero, as blocking_notifier_chain_register()
+ *	always returns zero.
+ */
+int register_poweroff_handler(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&poweroff_handler_list, nb);
+}
+EXPORT_SYMBOL(register_poweroff_handler);
+
+/**
+ *	unregister_poweroff_handler - Unregister previously registered
+ *				      poweroff handler
+ *	@nb: Hook to be unregistered
+ *
+ *	Unregisters a previously registered poweroff handler function.
+ *
+ *	Returns zero on success, or %-ENOENT on failure.
+ */
+int unregister_poweroff_handler(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&poweroff_handler_list, nb);
+}
+EXPORT_SYMBOL(unregister_poweroff_handler);
+
+static void devm_unregister_poweroff_handler(struct device *dev, void *res)
+{
+	WARN_ON(unregister_poweroff_handler(*(struct notifier_block **)res));
+}
+
+int devm_register_poweroff_handler(struct device *dev, struct notifier_block *nb)
+{
+	struct notifier_block **rcnb;
+	int ret;
+
+	rcnb = devres_alloc(devm_unregister_poweroff_handler,
+			    sizeof(*rcnb), GFP_KERNEL);
+	if (!rcnb)
+		return -ENOMEM;
+
+	ret = register_poweroff_handler(nb);
+	if (!ret) {
+		*rcnb = nb;
+		devres_add(dev, rcnb);
+	} else {
+		devres_free(rcnb);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(devm_register_poweroff_handler);
+
+/**
+ *	do_kernel_power_off - Execute kernel poweroff handler call chain
+ *
+ *	Calls functions registered with register_poweroff_handler.
+ *
+ *	Expected to be called as last step of the poweroff sequence.
+ *
+ *	Powers off the system immediately if a poweroff handler function has
+ *	been registered. Otherwise does nothing.
+ */
+static void do_kernel_power_off(void)
+{
+	blocking_notifier_call_chain(&poweroff_handler_list, 0, NULL);
+}
+
 /**
  *	kernel_power_off - power_off the system
  *
@@ -295,6 +402,7 @@ void kernel_power_off(void)
 	pr_emerg("Power down\n");
 	kmsg_dump(KMSG_DUMP_SHUTDOWN);
 	machine_power_off();
+	do_kernel_power_off();
 }
 EXPORT_SYMBOL_GPL(kernel_power_off);
 
@@ -339,7 +447,8 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
 	/* Instead of trying to make the power_off code look like
 	 * halt when pm_power_off is not set do it the easy way.
 	 */
-	if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
+	if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off &&
+	    blocking_notifier_call_chain_empty(&poweroff_handler_list))
 		cmd = LINUX_REBOOT_CMD_HALT;
 
 	mutex_lock(&system_transition_mutex);
-- 
2.32.0

[PATCH v1 3/6] kernel: Add devm_register_restart_handler()

[Dmitry Osipenko]

Add resource-managed variant of register_restart_handler().

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 include/linux/reboot.h |  2 ++
 kernel/reboot.c        | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/include/linux/reboot.h b/include/linux/reboot.h
index f72fb8729608..fb17fad63e53 100644
--- a/include/linux/reboot.h
+++ b/include/linux/reboot.h
@@ -49,6 +49,8 @@ extern int register_restart_handler(struct notifier_block *);
 extern int unregister_restart_handler(struct notifier_block *);
 extern void do_kernel_restart(char *cmd);
 
+extern int devm_register_restart_handler(struct device *, struct notifier_block *);
+
 extern int register_poweroff_handler(struct notifier_block *);
 extern int unregister_poweroff_handler(struct notifier_block *);
 
diff --git a/kernel/reboot.c b/kernel/reboot.c
index 6e90afec2bb8..b611e49a5ac6 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -200,6 +200,33 @@ int unregister_restart_handler(struct notifier_block *nb)
 }
 EXPORT_SYMBOL(unregister_restart_handler);
 
+static void devm_unregister_restart_handler(struct device *dev, void *res)
+{
+	WARN_ON(unregister_restart_handler(*(struct notifier_block **)res));
+}
+
+int devm_register_restart_handler(struct device *dev, struct notifier_block *nb)
+{
+	struct notifier_block **rcnb;
+	int ret;
+
+	rcnb = devres_alloc(devm_unregister_restart_handler,
+			    sizeof(*rcnb), GFP_KERNEL);
+	if (!rcnb)
+		return -ENOMEM;
+
+	ret = register_restart_handler(nb);
+	if (!ret) {
+		*rcnb = nb;
+		devres_add(dev, rcnb);
+	} else {
+		devres_free(rcnb);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(devm_register_restart_handler);
+
 /**
  *	do_kernel_restart - Execute kernel restart handler call chain
  *
-- 
2.32.0

[PATCH v1 4/6] mfd: max77620: Use power off call chain API

[Dmitry Osipenko]

Use new power off call chain API which allows multiple power off handlers
to coexist. Nexus 7 Android tablet can be powered off using MAX77663 PMIC
and using a special bootloader command. At first the bootloader option
should be tried, it will have a higher priority than the PMIC.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/mfd/max77620.c       | 22 +++++++++++++++-------
 include/linux/mfd/max77620.h |  2 ++
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/mfd/max77620.c b/drivers/mfd/max77620.c
index fec2096474ad..ad40eed1f0c6 100644
--- a/drivers/mfd/max77620.c
+++ b/drivers/mfd/max77620.c
@@ -31,11 +31,10 @@
 #include <linux/init.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/reboot.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
 
-static struct max77620_chip *max77620_scratch;
-
 static const struct resource gpio_resources[] = {
 	DEFINE_RES_IRQ(MAX77620_IRQ_TOP_GPIO),
 };
@@ -483,13 +482,17 @@ static int max77620_read_es_version(struct max77620_chip *chip)
 	return ret;
 }
 
-static void max77620_pm_power_off(void)
+static int max77620_pm_power_off(struct notifier_block *nb,
+				 unsigned long reboot_mode, void *data)
 {
-	struct max77620_chip *chip = max77620_scratch;
+	struct max77620_chip *chip = container_of(nb, struct max77620_chip,
+						  pm_off_nb);
 
 	regmap_update_bits(chip->rmap, MAX77620_REG_ONOFFCNFG1,
 			   MAX77620_ONOFFCNFG1_SFT_RST,
 			   MAX77620_ONOFFCNFG1_SFT_RST);
+
+	return NOTIFY_DONE;
 }
 
 static int max77620_probe(struct i2c_client *client,
@@ -566,9 +569,14 @@ static int max77620_probe(struct i2c_client *client,
 	}
 
 	pm_off = of_device_is_system_power_controller(client->dev.of_node);
-	if (pm_off && !pm_power_off) {
-		max77620_scratch = chip;
-		pm_power_off = max77620_pm_power_off;
+	if (pm_off) {
+		chip->pm_off_nb.notifier_call = max77620_pm_power_off;
+		chip->pm_off_nb.priority = 128;
+
+		ret = devm_register_poweroff_handler(chip->dev, &chip->pm_off_nb);
+		if (ret < 0)
+			dev_err(chip->dev,
+				"Failed to register poweroff handler: %d\n", ret);
 	}
 
 	return 0;
diff --git a/include/linux/mfd/max77620.h b/include/linux/mfd/max77620.h
index f552ef5b1100..99de4f8c9cbf 100644
--- a/include/linux/mfd/max77620.h
+++ b/include/linux/mfd/max77620.h
@@ -8,6 +8,7 @@
 #ifndef _MFD_MAX77620_H_
 #define _MFD_MAX77620_H_
 
+#include <linux/notifier.h>
 #include <linux/types.h>
 
 /* GLOBAL, PMIC, GPIO, FPS, ONOFFC, CID Registers */
@@ -327,6 +328,7 @@ enum max77620_chip_id {
 struct max77620_chip {
 	struct device *dev;
 	struct regmap *rmap;
+	struct notifier_block pm_off_nb;
 
 	int chip_irq;
 
-- 
2.32.0

[PATCH v1 5/6] soc/tegra: pmc: Use devm_register_restart_handler()

[Dmitry Osipenko]

Use resource-managed variant of register_restart_handler() to make code
cleaner.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/soc/tegra/pmc.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 50091c4ec948..dc9ad075ee4a 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -2859,6 +2859,13 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 		pmc->clk = NULL;
 	}
 
+	err = devm_register_restart_handler(&pdev->dev, &tegra_pmc_restart_handler);
+	if (err) {
+		dev_err(&pdev->dev, "unable to register restart handler, %d\n",
+			err);
+		return err;
+	}
+
 	/*
 	 * PCLK clock rate can't be retrieved using CLK API because it
 	 * causes lockup if CPU enters LP2 idle state from some other
@@ -2890,20 +2897,13 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 			goto cleanup_sysfs;
 	}
 
-	err = register_restart_handler(&tegra_pmc_restart_handler);
-	if (err) {
-		dev_err(&pdev->dev, "unable to register restart handler, %d\n",
-			err);
-		goto cleanup_debugfs;
-	}
-
 	err = tegra_pmc_pinctrl_init(pmc);
 	if (err)
-		goto cleanup_restart_handler;
+		goto cleanup_debugfs;
 
 	err = tegra_pmc_regmap_init(pmc);
 	if (err < 0)
-		goto cleanup_restart_handler;
+		goto cleanup_debugfs;
 
 	err = tegra_powergate_init(pmc, pdev->dev.of_node);
 	if (err < 0)
@@ -2926,8 +2926,6 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 
 cleanup_powergates:
 	tegra_powergate_remove_all(pdev->dev.of_node);
-cleanup_restart_handler:
-	unregister_restart_handler(&tegra_pmc_restart_handler);
 cleanup_debugfs:
 	debugfs_remove(pmc->debugfs);
 cleanup_sysfs:
-- 
2.32.0

[PATCH v1 6/6] soc/tegra: pmc: Add power off handler

[Dmitry Osipenko]

Nexus 7 Android tablet can be turned off using a special bootloader
command which is conveyed to bootloader by putting magic value into
specific scratch register and then rebooting normally. This power off
method should be invoked if USB cable is connected. Bootloader then will
display battery status and power off the device. This behaviour is
borrowed from downstream kernel and matches user expectations, otherwise
it looks like device got hung during power off.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/soc/tegra/pmc.c | 42 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index dc9ad075ee4a..44de043c2012 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -39,6 +39,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_opp.h>
+#include <linux/power_supply.h>
 #include <linux/reboot.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
@@ -107,6 +108,7 @@
 #define PMC_USB_DEBOUNCE_DEL		0xec
 #define PMC_USB_AO			0xf0
 
+#define PMC_SCRATCH37			0x130
 #define PMC_SCRATCH41			0x140
 
 #define PMC_WAKE2_MASK			0x160
@@ -1063,10 +1065,8 @@ int tegra_pmc_cpu_remove_clamping(unsigned int cpuid)
 	return tegra_powergate_remove_clamping(id);
 }
 
-static int tegra_pmc_restart_notify(struct notifier_block *this,
-				    unsigned long action, void *data)
+static void tegra_pmc_restart(const char *cmd)
 {
-	const char *cmd = data;
 	u32 value;
 
 	value = tegra_pmc_scratch_readl(pmc, pmc->soc->regs->scratch0);
@@ -1089,6 +1089,12 @@ static int tegra_pmc_restart_notify(struct notifier_block *this,
 	value = tegra_pmc_readl(pmc, PMC_CNTRL);
 	value |= PMC_CNTRL_MAIN_RST;
 	tegra_pmc_writel(pmc, value, PMC_CNTRL);
+}
+
+static int tegra_pmc_restart_notify(struct notifier_block *this,
+				    unsigned long action, void *data)
+{
+	tegra_pmc_restart(data);
 
 	return NOTIFY_DONE;
 }
@@ -1098,6 +1104,29 @@ static struct notifier_block tegra_pmc_restart_handler = {
 	.priority = 128,
 };
 
+static int tegra_pmc_poweroff_notify(struct notifier_block *this,
+				     unsigned long action, void *data)
+{
+	/*
+	 * Reboot Nexus 7 into special bootloader mode if USB cable is
+	 * connected in order to display battery status and power off.
+	 */
+	if (of_machine_is_compatible("asus,grouper") &&
+	    power_supply_is_system_supplied()) {
+		const u32 go_to_charger_mode = 0xa5a55a5a;
+
+		tegra_pmc_writel(pmc, go_to_charger_mode, PMC_SCRATCH37);
+		tegra_pmc_restart(NULL);
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block tegra_pmc_poweroff_handler = {
+	.notifier_call = tegra_pmc_poweroff_notify,
+	.priority = 200,
+};
+
 static int powergate_show(struct seq_file *s, void *data)
 {
 	unsigned int i;
@@ -2866,6 +2895,13 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	err = devm_register_poweroff_handler(&pdev->dev, &tegra_pmc_poweroff_handler);
+	if (err) {
+		dev_err(&pdev->dev, "unable to register poweroff handler, %d\n",
+			err);
+		return err;
+	}
+
 	/*
 	 * PCLK clock rate can't be retrieved using CLK API because it
 	 * causes lockup if CPU enters LP2 idle state from some other
-- 
2.32.0

Re: [PATCH v1 0/6] Introduce power off call chain API

[Andy Shevchenko]

On Thu, Oct 7, 2021 at 9:05 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> Introduce power off call chain API that is inspired by the restart API.
> It allows to have multiple power off handlers invoked along the chain

allows multiple

> until system is powered off. For the starter this series converts couple

the system
a couple

> NVIDIA Tegra drivers to the new API. Existing pm_power_off() method
> stays around and may be removed once all users will adopt the new API.

users adopt

>
> There were couple attempts in the past to add power off API from

a couple

> Guenter Roeck and Thierry Reding, but they were never completed. This
> is a somewhat simplified version which doesn't try to convert whole kernel
> to the new API at once, but solves immediate practical problem that we

problems

> have on Nexus 7 Android tablet where device needs to chain power off

tablets where the device

> methods.

Immediate question here is how do you see the plan of spreading this.
I.o.w. can you put an explanation that you have checked, let's say
>80% current users, and they may be converted like [example
placeholder] without any special tricks?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/6] Introduce power off call chain API

[Dmitry Osipenko]

07.10.2021 10:18, Andy Shevchenko пишет:
> On Thu, Oct 7, 2021 at 9:05 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> Introduce power off call chain API that is inspired by the restart API.
>> It allows to have multiple power off handlers invoked along the chain
> 
> allows multiple
> 
>> until system is powered off. For the starter this series converts couple
> 
> the system
> a couple
> 
>> NVIDIA Tegra drivers to the new API. Existing pm_power_off() method
>> stays around and may be removed once all users will adopt the new API.
> 
> users adopt
> 
>>
>> There were couple attempts in the past to add power off API from
> 
> a couple
> 
>> Guenter Roeck and Thierry Reding, but they were never completed. This
>> is a somewhat simplified version which doesn't try to convert whole kernel
>> to the new API at once, but solves immediate practical problem that we
> 
> problems
> 
>> have on Nexus 7 Android tablet where device needs to chain power off
> 
> tablets where the device

Thank you for the corrections, so far there is one problem and one tablet :)

> Immediate question here is how do you see the plan of spreading this.
> I.o.w. can you put an explanation that you have checked, let's say
>> 80% current users, and they may be converted like [example
> placeholder] without any special tricks?

The rough plan is:

1. Add new API.
2. Convert drivers to the new API per subsystem.
3. Expose do_kernel_restart().
4. Replace pm_power_off() with do_kernel_poweroff() per arch/, making
power off similar to the restart that uses do_kernel_restart().
5. Remove do_kernel_restart() from kernel/reboot.c

Majority of pm_power_off() users shouldn't need the chaining and
pm_power_off() doesn't conflict with the new API, so there is no need to
rush the conversion.

The single-link chain users could be converted to the new API directly,
this will remove some global variables. But at first should be better to
gain more users who actually need the chained power off since they may
have very specific requirements not covered by the current variant of
the API and will be easier to evolve API with less users.

Re: [PATCH v1 0/6] Introduce power off call chain API

[Andy Shevchenko]

On Thu, Oct 07, 2021 at 11:52:46AM +0300, Dmitry Osipenko wrote:
> 07.10.2021 10:18, Andy Shevchenko пишет:
> > On Thu, Oct 7, 2021 at 9:05 AM Dmitry Osipenko <digetx@gmail.com> wrote:

...

> >> This
> >> is a somewhat simplified version which doesn't try to convert whole kernel
> >> to the new API at once, but solves immediate practical problem that we
> > 
> > problems
> > 
> >> have on Nexus 7 Android tablet where device needs to chain power off
> > 
> > tablets where the device
> 
> Thank you for the corrections, so far there is one problem and one tablet :)

Then use "the Nexus 7 Android tablet" :-)

> > Immediate question here is how do you see the plan of spreading this.
> > I.o.w. can you put an explanation that you have checked, let's say
> >> 80% current users, and they may be converted like [example
> > placeholder] without any special tricks?
> 
> The rough plan is:
> 
> 1. Add new API.
> 2. Convert drivers to the new API per subsystem.

I would suggest to show that you are actually into it by converting a couple of
the subsystems for the starter.

> 3. Expose do_kernel_restart().
> 4. Replace pm_power_off() with do_kernel_poweroff() per arch/, making
> power off similar to the restart that uses do_kernel_restart().
> 5. Remove do_kernel_restart() from kernel/reboot.c
> 
> Majority of pm_power_off() users shouldn't need the chaining and
> pm_power_off() doesn't conflict with the new API, so there is no need to
> rush the conversion.
> 
> The single-link chain users could be converted to the new API directly,
> this will remove some global variables. But at first should be better to
> gain more users who actually need the chained power off since they may
> have very specific requirements not covered by the current variant of
> the API and will be easier to evolve API with less users.

All above in one or another form should be in cover letter.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/6] Introduce power off call chain API

[Dmitry Osipenko]

07.10.2021 12:11, Andy Shevchenko пишет:
> On Thu, Oct 07, 2021 at 11:52:46AM +0300, Dmitry Osipenko wrote:
>> 07.10.2021 10:18, Andy Shevchenko пишет:
>>> On Thu, Oct 7, 2021 at 9:05 AM Dmitry Osipenko <digetx@gmail.com> wrote:
> 
> ...
> 
>>>> This
>>>> is a somewhat simplified version which doesn't try to convert whole kernel
>>>> to the new API at once, but solves immediate practical problem that we
>>>
>>> problems
>>>
>>>> have on Nexus 7 Android tablet where device needs to chain power off
>>>
>>> tablets where the device
>>
>> Thank you for the corrections, so far there is one problem and one tablet :)
> 
> Then use "the Nexus 7 Android tablet" :-)
> 
>>> Immediate question here is how do you see the plan of spreading this.
>>> I.o.w. can you put an explanation that you have checked, let's say
>>>> 80% current users, and they may be converted like [example
>>> placeholder] without any special tricks?
>>
>> The rough plan is:
>>
>> 1. Add new API.
>> 2. Convert drivers to the new API per subsystem.
> 
> I would suggest to show that you are actually into it by converting a couple of
> the subsystems for the starter.

Noted

>> 3. Expose do_kernel_restart().
>> 4. Replace pm_power_off() with do_kernel_poweroff() per arch/, making
>> power off similar to the restart that uses do_kernel_restart().
>> 5. Remove do_kernel_restart() from kernel/reboot.c
>>
>> Majority of pm_power_off() users shouldn't need the chaining and
>> pm_power_off() doesn't conflict with the new API, so there is no need to
>> rush the conversion.
>>
>> The single-link chain users could be converted to the new API directly,
>> this will remove some global variables. But at first should be better to
>> gain more users who actually need the chained power off since they may
>> have very specific requirements not covered by the current variant of
>> the API and will be easier to evolve API with less users.
> 
> All above in one or another form should be in cover letter.

Alright, apparently I overestimated a tad awareness about the current
status. I'll extend the cover letter next time, thanks.

Re: [PATCH v1 4/6] mfd: max77620: Use power off call chain API

[Lee Jones]

On Thu, 07 Oct 2021, Dmitry Osipenko wrote:

> Use new power off call chain API which allows multiple power off handlers
> to coexist. Nexus 7 Android tablet can be powered off using MAX77663 PMIC
> and using a special bootloader command. At first the bootloader option
> should be tried, it will have a higher priority than the PMIC.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/mfd/max77620.c       | 22 +++++++++++++++-------
>  include/linux/mfd/max77620.h |  2 ++
>  2 files changed, 17 insertions(+), 7 deletions(-)

I don't have a problem with the approach in general.

I guess it's up to the relevant maintainers to decide.

> diff --git a/drivers/mfd/max77620.c b/drivers/mfd/max77620.c
> index fec2096474ad..ad40eed1f0c6 100644
> --- a/drivers/mfd/max77620.c
> +++ b/drivers/mfd/max77620.c
> @@ -31,11 +31,10 @@
>  #include <linux/init.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/reboot.h>
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
>  
> -static struct max77620_chip *max77620_scratch;
> -
>  static const struct resource gpio_resources[] = {
>  	DEFINE_RES_IRQ(MAX77620_IRQ_TOP_GPIO),
>  };
> @@ -483,13 +482,17 @@ static int max77620_read_es_version(struct max77620_chip *chip)
>  	return ret;
>  }
>  
> -static void max77620_pm_power_off(void)
> +static int max77620_pm_power_off(struct notifier_block *nb,
> +				 unsigned long reboot_mode, void *data)
>  {
> -	struct max77620_chip *chip = max77620_scratch;
> +	struct max77620_chip *chip = container_of(nb, struct max77620_chip,
> +						  pm_off_nb);
>  
>  	regmap_update_bits(chip->rmap, MAX77620_REG_ONOFFCNFG1,
>  			   MAX77620_ONOFFCNFG1_SFT_RST,
>  			   MAX77620_ONOFFCNFG1_SFT_RST);
> +
> +	return NOTIFY_DONE;
>  }
>  
>  static int max77620_probe(struct i2c_client *client,
> @@ -566,9 +569,14 @@ static int max77620_probe(struct i2c_client *client,
>  	}
>  
>  	pm_off = of_device_is_system_power_controller(client->dev.of_node);
> -	if (pm_off && !pm_power_off) {
> -		max77620_scratch = chip;
> -		pm_power_off = max77620_pm_power_off;
> +	if (pm_off) {
> +		chip->pm_off_nb.notifier_call = max77620_pm_power_off;
> +		chip->pm_off_nb.priority = 128;
> +
> +		ret = devm_register_poweroff_handler(chip->dev, &chip->pm_off_nb);
> +		if (ret < 0)
> +			dev_err(chip->dev,
> +				"Failed to register poweroff handler: %d\n", ret);
>  	}
>  
>  	return 0;
> diff --git a/include/linux/mfd/max77620.h b/include/linux/mfd/max77620.h
> index f552ef5b1100..99de4f8c9cbf 100644
> --- a/include/linux/mfd/max77620.h
> +++ b/include/linux/mfd/max77620.h
> @@ -8,6 +8,7 @@
>  #ifndef _MFD_MAX77620_H_
>  #define _MFD_MAX77620_H_
>  
> +#include <linux/notifier.h>
>  #include <linux/types.h>
>  
>  /* GLOBAL, PMIC, GPIO, FPS, ONOFFC, CID Registers */
> @@ -327,6 +328,7 @@ enum max77620_chip_id {
>  struct max77620_chip {
>  	struct device *dev;
>  	struct regmap *rmap;
> +	struct notifier_block pm_off_nb;
>  
>  	int chip_irq;
>  

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1 4/6] mfd: max77620: Use power off call chain API

[Dmitry Osipenko]

19.10.2021 18:31, Lee Jones пишет:
> On Thu, 07 Oct 2021, Dmitry Osipenko wrote:
> 
>> Use new power off call chain API which allows multiple power off handlers
>> to coexist. Nexus 7 Android tablet can be powered off using MAX77663 PMIC
>> and using a special bootloader command. At first the bootloader option
>> should be tried, it will have a higher priority than the PMIC.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/mfd/max77620.c       | 22 +++++++++++++++-------
>>  include/linux/mfd/max77620.h |  2 ++
>>  2 files changed, 17 insertions(+), 7 deletions(-)
> I don't have a problem with the approach in general.
> 
> I guess it's up to the relevant maintainers to decide.
> 

Thank you for taking a look at this. Guenter Roeck gave me advice based
on his previous experience of working on this topic and I'm now in a
process of finalizing v2 that will be a more comprehensive and nicer
solution. So you will need to take another look soon, thanks.