GPIO driver to turn power off

Given appropriate devicetree bindings, this driver registers a pm_power_off function to set a GPIO line high/low to power down your board.

drivers/power/reset/Kconfig +1 -0

 
+menuconfig POWER_RESET

+MODULE_LICENSE("GPL");

Quoted 7 lines · Andrew Lunn

Given appropriate devicetree bindings, this driver registers a pm_power_off function to set a GPIO line high/low to power down your board.

drivers/power/reset/Kconfig +1 -0

 
+menuconfig POWER_RESET

drivers/power/reset/Kconfig line 11

Quoted 10 lines

drivers/power/reset/Kconfig +10 -0

+	bool "Board level reset or power off"
+	help
+	  Provides a number of drivers which either reset a complete board
+	  or shut it down, by manipulating the main power supply on the board.
+
+	  Say Y here to enable board reset and power off
+
+config POWER_RESET_GPIO
+	bool "GPIO power-off driver"
+	depends on OF_GPIO && POWER_RESET

I assume CONFIG_POWER_RESET won't really provide any/much infra-structure itself. So, does it make sense to put all the individual drives inside "if POWER_RESET" or a menu definition, so they don't all have to depend on POWER_RESET explicitly?

drivers/power/reset/gpio-poweroff.c line 32

Quoted 5 lines

drivers/power/reset/gpio-poweroff.c +3 -0

 
+static void gpio_poweroff_do_poweroff(void)
+{
+	BUG_ON(gpio_num == -1);

Perhaps use gpio_is_valid() here?

drivers/power/reset/gpio-poweroff.c line 35

Quoted 2 lines

drivers/power/reset/gpio-poweroff.c +2 -0

+	/* drive it active */
+	gpio_direction_output(gpio_num, !gpio_active_low);

The rest of the code below doesn't make a lot of sense to me. From reading the binding documentation, it seems like the GPIO is expected to be level-sensitive, and as such the above gpio_direction_output() call should be all that's needed. What is the code below doing? If this driver supports either a level-sensitive GPIO or generating a pulse on a GPIO, I think the binding should be enhanced to specify which signalling type is required. Also, all the delays should be specified as DT properties.

drivers/power/reset/gpio-poweroff.c line 37

Quoted 2 lines

drivers/power/reset/gpio-poweroff.c +2 -0

+	mdelay(100);
+	/* rising edge or drive inactive */

You can't assume that an active->inactive edge is a rising edge if you allow the polarity to be configurable; I would remove that part of the comment. Same below for "falling edge".

drivers/power/reset/gpio-poweroff.c line 63

Quoted 12 lines

drivers/power/reset/gpio-poweroff.c +10 -0

+	gpio_set_value(gpio_num, gpio_active_low);
+	mdelay(100);
+	/* falling edge */
+	gpio_set_value(gpio_num, !gpio_active_low);
+
+	/* give it some time */
+	mdelay(3000);
 
+static int __devinit gpio_poweroff_probe(struct platform_device *pdev)
 
+	gpio_num = of_get_gpio_flags(pdev->dev.of_node, 0, &flags);
+	if (gpio_num < 0) {

Perhaps use gpio_is_valid() here?

drivers/power/reset/gpio-poweroff.c line 67

Quoted 4 lines

drivers/power/reset/gpio-poweroff.c +4 -0

+		pr_err("%s: Could not get GPIO configuration: %d",
+		       __func__, gpio_num);
+		return -ENODEV;
+	}

This doesn't handle deferred probe correctly; if gpio_num == -EPROBE_DEFER, then this function needs to return -EPROBE_DEFER too; why not "return gpio_num" rather than "return -ENODEV" above?

drivers/power/reset/gpio-poweroff.c line 71

Quoted 4 lines

drivers/power/reset/gpio-poweroff.c +4 -0

+	gpio_active_low = flags & OF_GPIO_ACTIVE_LOW;
+
+	if (of_get_property(pdev->dev.of_node, "input", NULL))
+		input = true;

Perhaps use of_property_read_bool() here.

drivers/power/reset/gpio-poweroff.c line 102

Quoted 4 lines

drivers/power/reset/gpio-poweroff.c +4 -0

+static int __devexit gpio_poweroff_remove(struct platform_device *pdev)
+{
+	if (gpio_num != -1)
+		gpio_free(gpio_num);

Perhaps use gpio_is_valid() here? Actually, how could this happen; presumably if the GPIO is valid, probe() never succeeded, so you should always just free the GPIO.

drivers/power/reset/gpio-poweroff.c line 121

Quoted 8 lines

drivers/power/reset/gpio-poweroff.c +8 -0

+static struct platform_driver gpio_poweroff_driver = {
+	.probe = gpio_poweroff_probe,
+	.remove = __devexit_p(gpio_poweroff_remove),
+	.driver = {
+		   .name = "poweroff-gpio",
+		   .owner = THIS_MODULE,
+		   .of_match_table = of_gpio_poweroff_match,
+		   },

There seems to be a mix of TAB/space indents there, and the closing } should probably be aligned with the start of ".driver"?

Quoted 1 line

+MODULE_LICENSE("GPL");

That should be "GPL v2".

These are good comments from Stephan that i want to address. However, i also don't want to delay the pull-requests direction arm-soc, the merge window is getting close. Both Linus and Anton have Acked the current version, so please go with what you have and i will produce a patch over the top. If its available before Arnd pulls, you can squash it, otherwise send it upstream as a standalone patch.

Hi Jason

These are good comments from Stephan that i want to address. However, i also don't want to delay the pull-requests direction arm-soc, the merge window is getting close. Both Linus and Anton have Acked the current version, so please go with what you have and i will produce a patch over the top. If its available before Arnd pulls, you can squash it, otherwise send it upstream as a standalone patch.

Quoted 8 lines · Andrew Lunn

Hi Jason

These are good comments from Stephan that i want to address. However, i also don't want to delay the pull-requests direction arm-soc, the merge window is getting close. Both Linus and Anton have Acked the current version, so please go with what you have and i will produce a patch over the top. If its available before Arnd pulls, you can squash it, otherwise send it upstream as a standalone patch.

I'm not sure I agree here; the comments I made re: the delays and pulse-vs-level may affect the definition of the DT binding, and that's something that should be correct from the start.

The implementation of gpio_poweroff_do_poweroff() really doesn't seem to make sense; related to the above.

Also, probe deferral doesn't work, which will likely make this code completely ineffective.

Quoted 10 lines · Stephen Warren

Quoted 8 lines · Andrew Lunn

Hi Jason

These are good comments from Stephan that i want to address. However, i also don't want to delay the pull-requests direction arm-soc, the merge window is getting close. Both Linus and Anton have Acked the current version, so please go with what you have and i will produce a patch over the top. If its available before Arnd pulls, you can squash it, otherwise send it upstream as a standalone patch.

Quoted 3 lines

I'm not sure I agree here; the comments I made re: the delays and pulse-vs-level may affect the definition of the DT binding, and that's something that should be correct from the start.

Hi Stephen

There should be no need to change the binding is a none-backwards compatible way. The current delays work for all current users.

If some
other user comes along which needs longer delays, they can add an optional property which specified a longer delay.

The pulse-vs-level is needed for the potential PXA use case, which is where i took this code from. When we first proposed this driver, Russel King pointed to the PXA reset.c code and requested we create something which PXA could use. The configuring the GPIO as an input also come from PXA, which the current two uses user don't need.

I agree it need better documentation, but the current code covers all bases. It will turn off a level based system, it will also turn off an edge based system. There is no need to specify in the DT which is needed, it will just work.

Quoted 5 lines

The implementation of gpio_poweroff_do_poweroff() really doesn't seem to make sense; related to the above.

Also, probe deferral doesn't work, which will likely make this code completely ineffective.

This code has been tested by the two current users. It works. However, i agree about your comment here, and i will fix it in the follow up patch.

Quoted 15 lines · Andrew Lunn

Quoted 13 lines · Stephen Warren

Quoted 8 lines · Andrew Lunn

Hi Jason

These are good comments from Stephan that i want to address. However, i also don't want to delay the pull-requests direction arm-soc, the merge window is getting close. Both Linus and Anton have Acked the current version, so please go with what you have and i will produce a patch over the top. If its available before Arnd pulls, you can squash it, otherwise send it upstream as a standalone patch.

I'm not sure I agree here; the comments I made re: the delays and pulse-vs-level may affect the definition of the DT binding, and that's something that should be correct from the start.

Quoted 4 lines

Hi Stephen

There should be no need to change the binding is a none-backwards compatible way. The current delays work for all current users.

Well, the binding isn't written for current users, the description is just a generic "GPIO to turn off the system".

Quoted 3 lines

If some
other user comes along which needs longer delays, they can add an optional property which specified a longer delay.

The DT maintainers have in the past expressed a wish to define DT bindings completely, rather than incrementally adding stuff if at all possible. I guess they weren't CC'd on this patch; I've added them here for comment. I'll defer to whatever they think here.

Quoted 10 lines

The pulse-vs-level is needed for the potential PXA use case, which is where i took this code from. When we first proposed this driver, Russel King pointed to the PXA reset.c code and requested we create something which PXA could use. The configuring the GPIO as an input also come from PXA, which the current two uses user don't need.

I agree it need better documentation, but the current code covers all bases. It will turn off a level based system, it will also turn off an edge based system. There is no need to specify in the DT which is needed, it will just work.

I'm not convinced the pulse logic can be correct for arbitrary users who expect a level-sensitive GPIO; what if the MPU (or whatever the GPIO is connected to) is confused by the pulse and fails to power down when it sees a pulse (e.g. as a safety measure), but would have worked fine with just a regular level?

If this were a specific driver just for certain platforms, I think it might be OK. Yet the driver purports to be generic. As such, I think we need to make it explicitly work for the general case, not just happen to work for the platforms that have been tested so far.

Quoted 16 lines · Stephen Warren

Quoted 15 lines · Andrew Lunn

Quoted 13 lines · Stephen Warren

Quoted 8 lines · Andrew Lunn

Hi Jason

These are good comments from Stephan that i want to address. However, i also don't want to delay the pull-requests direction arm-soc, the merge window is getting close. Both Linus and Anton have Acked the current version, so please go with what you have and i will produce a patch over the top. If its available before Arnd pulls, you can squash it, otherwise send it upstream as a standalone patch.

I'm not sure I agree here; the comments I made re: the delays and pulse-vs-level may affect the definition of the DT binding, and that's something that should be correct from the start.

Quoted 7 lines

Quoted 4 lines

Hi Stephen

There should be no need to change the binding is a none-backwards compatible way. The current delays work for all current users.

Well, the binding isn't written for current users, the description is just a generic "GPIO to turn off the system".

The current binding documentation is maybe lacking. But the binding itself fits the current users.

Quoted 4 lines

Quoted 3 lines

If some
other user comes along which needs longer delays, they can add an optional property which specified a longer delay.

Quoted 4 lines

The DT maintainers have in the past expressed a wish to define DT bindings completely, rather than incrementally adding stuff if at all possible. I guess they weren't CC'd on this patch; I've added them here for comment. I'll defer to whatever they think here.

They were CC: I learn't the hard way with a USB binding....

Quoted 11 lines

Quoted 10 lines

The pulse-vs-level is needed for the potential PXA use case, which is where i took this code from. When we first proposed this driver, Russel King pointed to the PXA reset.c code and requested we create something which PXA could use. The configuring the GPIO as an input also come from PXA, which the current two uses user don't need.

I agree it need better documentation, but the current code covers all bases. It will turn off a level based system, it will also turn off an edge based system. There is no need to specify in the DT which is needed, it will just work.

Quoted 5 lines

I'm not convinced the pulse logic can be correct for arbitrary users who expect a level-sensitive GPIO; what if the MPU (or whatever the GPIO is connected to) is confused by the pulse and fails to power down when it sees a pulse (e.g. as a safety measure), but would have worked fine with just a regular level?

Lets go through the sequence for a regular level off.....

At driver load time, assuming the optional input properties is not present, the GPIO line is driven inactive. At power down time, its driven active. One of four things can happen:

4) The delay is not long enough. In that case it goes inactive again, there is a short pause, and it goes active again. There is another pause, this time much longer. Still the power does not go off and we hit the WARN_ON(1); The power remains on, until somebody pulls the plug.

Your scenario is that a 100ms pulse confuses the MPU, and after that, it won't turn off given a much longer period of the gpio being active. We run into 4). Is that really likely?