[PATCH 0/3] GPIO driver to turn power off

[PATCH 0/3] GPIO driver to turn power off

[Andrew Lunn]

A few of the Kirkwood systems use a GPIO line to turn the power off on
shutdown. They request the GPIO line in the board setup code,
optionally setting it as an output and driving it inactive. They then
register a function for pm_power_off. At the end of the shutdown, this
function is called. The GPIO line is configured as an output, if not
already so, and then driven active. In order to cover the use case of
edges rather then levels, triggering power off, the GPIO line is then
toggled active and later inactive. This should allow the driver to be
used by PXA which also has boards using the same scheme.

The driver code was initially developed by Jamie Lentin and extended
to cover the PXA case by Andrew Lunn.

Andrew Lunn (2):
  ARM: Kirkwood: Convert DNSKW to use gpio-poweroff.
  ARM: Kirkwood: Convert IB62x0 to use gpio-poweroff.

Jamie Lentin (1):
  gpio: Add simple poweroff-gpio driver

 .../devicetree/bindings/gpio/gpio-poweroff.txt     |   22 ++++
 arch/arm/boot/dts/kirkwood-dnskw.dtsi              |    5 +
 arch/arm/boot/dts/kirkwood-ib62x0.dts              |    6 +
 arch/arm/mach-kirkwood/Kconfig                     |    1 +
 arch/arm/mach-kirkwood/board-dnskw.c               |   12 --
 arch/arm/mach-kirkwood/board-ib62x0.c              |   13 --
 drivers/gpio/Kconfig                               |   10 ++
 drivers/gpio/Makefile                              |    2 +
 drivers/gpio/gpio-poweroff.c                       |  129 ++++++++++++++++++++
 9 files changed, 175 insertions(+), 25 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-poweroff.txt
 create mode 100644 drivers/gpio/gpio-poweroff.c

-- 
1.7.10.4

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Andrew Lunn]

From: Jamie Lentin <jm@lentin.co.uk>

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

Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 .../devicetree/bindings/gpio/gpio-poweroff.txt     |   22 ++++
 drivers/gpio/Kconfig                               |   10 ++
 drivers/gpio/Makefile                              |    2 +
 drivers/gpio/gpio-poweroff.c                       |  129 ++++++++++++++++++++
 4 files changed, 163 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-poweroff.txt
 create mode 100644 drivers/gpio/gpio-poweroff.c

diff --git a/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt b/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt
new file mode 100644
index 0000000..558cdf3
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt
@@ -0,0 +1,22 @@
+GPIO line that should be set high/low to power off a device
+
+Required properties:
+- compatible : should be "gpio-poweroff".
+- gpios : The GPIO to set high/low, see "gpios property" in
+  Documentation/devicetree/bindings/gpio/gpio.txt. If the pin should be
+  low to power down the board set it to "Active Low", otherwise set
+  gpio to "Active High".
+
+Optional properties:
+- input : Initially configure the GPIO line as an input. Only reconfigure
+  it to an output when the pm_power_off function is called. If this optional
+  property is not specified, the GPIO is initialized as an output in its
+  inactive state.
+
+
+Examples:
+
+gpio-poweroff {
+	compatible = "gpio-poweroff";
+	gpios = <&gpio 4 0>; /* GPIO 4 Active Low */
+};
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d055cee..5ec1714 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -75,6 +75,16 @@ config GPIO_SYSFS
 	  Kernel drivers may also request that a particular GPIO be
 	  exported to userspace; this can be useful when debugging.
 
+config GPIO_POWEROFF
+	tristate "GPIO power-off driver"
+	depends on OF_GPIO
+	help
+	  This driver supports turning off your device via a GPIO line.
+	  If your device needs a GPIO high/low to power down, say Y and
+	  create a binding in your devicetree.
+
+	  If unsure, say N
+
 config GPIO_GENERIC
 	tristate
 
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 9aeed67..a8efda3 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -5,6 +5,8 @@ ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
 obj-$(CONFIG_GPIOLIB)		+= gpiolib.o devres.o
 obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
 
+obj-$(CONFIG_GPIO_POWEROFF)	+= gpio-poweroff.o
+
 # Device drivers. Generally keep list sorted alphabetically
 obj-$(CONFIG_GPIO_GENERIC)	+= gpio-generic.o
 
diff --git a/drivers/gpio/gpio-poweroff.c b/drivers/gpio/gpio-poweroff.c
new file mode 100644
index 0000000..101ad57
--- /dev/null
+++ b/drivers/gpio/gpio-poweroff.c
@@ -0,0 +1,129 @@
+/*
+ * Toggles a GPIO pin to power down a device
+ *
+ * Jamie Lentin <jm@lentin.co.uk>
+ * Andrew Lunn <andrew@lunn.ch>
+ *
+ * Copyright (C) 2012 Jamie Lentin
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/of_platform.h>
+#include <linux/of_gpio.h>
+#include <linux/module.h>
+
+/*
+ * Hold configuration here, cannot be more than one instance of the driver
+ * since pm_power_off itself is global.
+ */
+static int gpio_num = -1;
+static int gpio_active_low;
+
+static void gpio_poweroff_do_poweroff(void)
+{
+	BUG_ON(gpio_num == -1);
+
+	/* drive it active */
+	gpio_direction_output(gpio_num, !gpio_active_low);
+	mdelay(100);
+	/* rising edge or drive inactive */
+	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(100);
+
+	WARN_ON(1);
+}
+
+static int __devinit gpio_poweroff_probe(struct platform_device *pdev)
+{
+	enum of_gpio_flags flags;
+	bool input = false;
+	int ret;
+
+	/* If a pm_power_off function has already been added, leave it alone */
+	if (pm_power_off != NULL) {
+		pr_err("%s: pm_power_off function already registered",
+		       __func__);
+		return -EBUSY;
+	}
+
+	gpio_num = of_get_gpio_flags(pdev->dev.of_node, 0, &flags);
+	if (gpio_num < 0) {
+		pr_err("%s: Could not get GPIO configuration: %d",
+		       __func__, gpio_num);
+		return -ENODEV;
+	}
+	gpio_active_low = flags & OF_GPIO_ACTIVE_LOW;
+
+	if (of_get_property(pdev->dev.of_node, "input", NULL))
+		input = true;
+
+	ret = gpio_request(gpio_num, "poweroff-gpio");
+	if (ret) {
+		pr_err("%s: Could not get GPIO %d", __func__, gpio_num);
+		return ret;
+	}
+	if (input) {
+		if (gpio_direction_input(gpio_num)) {
+			pr_err("Could not set direction of GPIO %d to input",
+			       gpio_num);
+			goto err;
+		}
+	} else {
+		if (gpio_direction_output(gpio_num, gpio_active_low)) {
+			pr_err("Could not set direction of GPIO %d", gpio_num);
+			goto err;
+		}
+	}
+
+	pm_power_off = &gpio_poweroff_do_poweroff;
+	return 0;
+
+err:
+	gpio_free(gpio_num);
+	return -ENODEV;
+}
+
+static int __devexit gpio_poweroff_remove(struct platform_device *pdev)
+{
+	if (gpio_num != -1)
+		gpio_free(gpio_num);
+	if (pm_power_off == &gpio_poweroff_do_poweroff)
+		pm_power_off = NULL;
+
+	return 0;
+}
+
+static const struct of_device_id of_gpio_poweroff_match[] = {
+	{ .compatible = "gpio-poweroff", },
+	{},
+};
+
+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,
+		   },
+};
+
+module_platform_driver(gpio_poweroff_driver);
+
+MODULE_AUTHOR("Jamie Lentin <jm@lentin.co.uk>");
+MODULE_DESCRIPTION("GPIO poweroff driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:poweroff-gpio");
-- 
1.7.10.4

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Stephen Warren]

On 11/11/2012 09:21 AM, Andrew Lunn wrote:
> From: Jamie Lentin <jm@lentin.co.uk>
> 
> Given appropriate devicetree bindings, this driver registers a
> pm_power_off function to set a GPIO line high/low to power down
> your board.

This feature will be useful for the Tegra TrimSlice board too.

> diff --git a/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt b/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt

> +Required properties:
> +- compatible : should be "gpio-poweroff".
> +- gpios : The GPIO to set high/low, see "gpios property" in
> +  Documentation/devicetree/bindings/gpio/gpio.txt. If the pin should be
> +  low to power down the board set it to "Active Low", otherwise set
> +  gpio to "Active High".

Unfortunately, not all GPIO bindings support active high/low flags in
the GPIO specifier. As such, the flags there are basically useless.
Other bindings (e.g. IIRC the fixed-regulator binding) have added a
separate active-high property to indicate the GPIO polarity. This
binding should probably follow suite.

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Andrew Lunn]

On Sun, Nov 11, 2012 at 03:03:49PM -0700, Stephen Warren wrote:
> On 11/11/2012 09:21 AM, Andrew Lunn wrote:
> > From: Jamie Lentin <jm@lentin.co.uk>
> > 
> > Given appropriate devicetree bindings, this driver registers a
> > pm_power_off function to set a GPIO line high/low to power down
> > your board.
> 
> This feature will be useful for the Tegra TrimSlice board too.

Hi Stephen

Great to hear its usable for others.

> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt b/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt
> 
> > +Required properties:
> > +- compatible : should be "gpio-poweroff".
> > +- gpios : The GPIO to set high/low, see "gpios property" in
> > +  Documentation/devicetree/bindings/gpio/gpio.txt. If the pin should be
> > +  low to power down the board set it to "Active Low", otherwise set
> > +  gpio to "Active High".
> 
> Unfortunately, not all GPIO bindings support active high/low flags in
> the GPIO specifier. As such, the flags there are basically useless.
> Other bindings (e.g. IIRC the fixed-regulator binding) have added a
> separate active-high property to indicate the GPIO polarity. This
> binding should probably follow suite.

Humm, so are you saying of_get_named_gpio_flags() is deprecated?

There is a lot of code using this to get the flag
OF_GPIO_ACTIVE_LOW. Some of these users are very generic code:

drivers/leds/leds-gpio.c:
drivers/input/misc/rotary_encoder.c:
drivers/input/keyboard/gpio_keys.c:
drivers/input/keyboard/gpio_keys_polled.c:
drivers/hwmon/gpio-fan.c:

The only code using the "enable-active-high"
property is drivers/regulator/fixed.c although the binding
documentation twl6040.txt talks about it, but the code does not
implement it.

Could you point me towards some email discussion about this?

Thanks
	Andrew

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Stephen Warren]

On 11/12/2012 01:25 AM, Andrew Lunn wrote:
> On Sun, Nov 11, 2012 at 03:03:49PM -0700, Stephen Warren wrote:
>> On 11/11/2012 09:21 AM, Andrew Lunn wrote:
>>> From: Jamie Lentin <jm@lentin.co.uk>
>>>
>>> Given appropriate devicetree bindings, this driver registers a
>>> pm_power_off function to set a GPIO line high/low to power down
>>> your board.

>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt b/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt
>>
>>> +Required properties:
>>> +- compatible : should be "gpio-poweroff".
>>> +- gpios : The GPIO to set high/low, see "gpios property" in
>>> +  Documentation/devicetree/bindings/gpio/gpio.txt. If the pin should be
>>> +  low to power down the board set it to "Active Low", otherwise set
>>> +  gpio to "Active High".
>>
>> Unfortunately, not all GPIO bindings support active high/low flags in
>> the GPIO specifier. As such, the flags there are basically useless.
>> Other bindings (e.g. IIRC the fixed-regulator binding) have added a
>> separate active-high property to indicate the GPIO polarity. This
>> binding should probably follow suite.
> 
> Humm, so are you saying of_get_named_gpio_flags() is deprecated?

I don't know if it's deprecated, but it's certainly not useful in
generic code.

> There is a lot of code using this to get the flag
> OF_GPIO_ACTIVE_LOW. Some of these users are very generic code:

That's unfortunate...

> 
> drivers/leds/leds-gpio.c:
> drivers/input/misc/rotary_encoder.c:
> drivers/input/keyboard/gpio_keys.c:
> drivers/input/keyboard/gpio_keys_polled.c:
> drivers/hwmon/gpio-fan.c:
> 
> The only code using the "enable-active-high"
> property is drivers/regulator/fixed.c although the binding
> documentation twl6040.txt talks about it, but the code does not
> implement it.
> 
> Could you point me towards some email discussion about this?

I don't have any good links. I imagine the discussion happened when the
patched for regulator/fixed.c were submitted.

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Andrew Lunn]

On Mon, Nov 12, 2012 at 09:17:48AM -0700, Stephen Warren wrote:
> On 11/12/2012 01:25 AM, Andrew Lunn wrote:
> > On Sun, Nov 11, 2012 at 03:03:49PM -0700, Stephen Warren wrote:
> >> On 11/11/2012 09:21 AM, Andrew Lunn wrote:
> >>> From: Jamie Lentin <jm@lentin.co.uk>
> >>>
> >>> Given appropriate devicetree bindings, this driver registers a
> >>> pm_power_off function to set a GPIO line high/low to power down
> >>> your board.
> 
> >>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt b/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt
> >>
> >>> +Required properties:
> >>> +- compatible : should be "gpio-poweroff".
> >>> +- gpios : The GPIO to set high/low, see "gpios property" in
> >>> +  Documentation/devicetree/bindings/gpio/gpio.txt. If the pin should be
> >>> +  low to power down the board set it to "Active Low", otherwise set
> >>> +  gpio to "Active High".
> >>
> >> Unfortunately, not all GPIO bindings support active high/low flags in
> >> the GPIO specifier. As such, the flags there are basically useless.
> >> Other bindings (e.g. IIRC the fixed-regulator binding) have added a
> >> separate active-high property to indicate the GPIO polarity. This
> >> binding should probably follow suite.
> > 
> > Humm, so are you saying of_get_named_gpio_flags() is deprecated?
> 
> I don't know if it's deprecated, but it's certainly not useful in
> generic code.

Hi Linus, Anton

How do you see this? 

I'm happy to implement an enable-active-high property, but it seems to
go against the purpose of of_get_named_gpio_flags(). Is that function
deprecated?

	Thanks
		Andrew

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Anton Vorontsov]

On Mon, Nov 12, 2012 at 07:19:47PM +0100, Andrew Lunn wrote:
[..]
> > >>> Given appropriate devicetree bindings, this driver registers a
> > >>> pm_power_off function to set a GPIO line high/low to power down
> > >>> your board.
> > 
> > >>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt b/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt
> > >>
> > >>> +Required properties:
> > >>> +- compatible : should be "gpio-poweroff".
> > >>> +- gpios : The GPIO to set high/low, see "gpios property" in
> > >>> +  Documentation/devicetree/bindings/gpio/gpio.txt. If the pin should be
> > >>> +  low to power down the board set it to "Active Low", otherwise set
> > >>> +  gpio to "Active High".
> > >>
> > >> Unfortunately, not all GPIO bindings support active high/low flags in
> > >> the GPIO specifier. As such, the flags there are basically useless.
> > >> Other bindings (e.g. IIRC the fixed-regulator binding) have added a
> > >> separate active-high property to indicate the GPIO polarity. This
> > >> binding should probably follow suite.

Should the gpio driver fix its bindings then?.. Polarity is a quite
generic concept of a GPIO, and flags are there for a reason. I'd rather
prefer having

	stuff-gpios = <0 0
		       1 0
		       2 1
		       3 0>;

Rather than

	stuff-gpios = <0 1 2 3>;
	stuff-polarity-gpio-map = <0 0 1 0>;

The first scheme existed like for years already. Has it been discussed
that it is no longer preferred?

> > > Humm, so are you saying of_get_named_gpio_flags() is deprecated?
> > 
> > I don't know if it's deprecated, but it's certainly not useful in
> > generic code.
> 
> Hi Linus, Anton
> 
> How do you see this? 
> 
> I'm happy to implement an enable-active-high property, but it seems to
> go against the purpose of of_get_named_gpio_flags(). Is that function
> deprecated?

Never heard of any deprecation, and I disagree that it is "not useful in
generic code". :)

Thanks,
Anton.

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Stephen Warren]

On 11/12/2012 11:43 AM, Anton Vorontsov wrote:
> On Mon, Nov 12, 2012 at 07:19:47PM +0100, Andrew Lunn wrote:
> [..]
>>>>>> Given appropriate devicetree bindings, this driver registers a
>>>>>> pm_power_off function to set a GPIO line high/low to power down
>>>>>> your board.
>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt b/Documentation/devicetree/bindings/gpio/gpio-poweroff.txt
>>>>>
>>>>>> +Required properties:
>>>>>> +- compatible : should be "gpio-poweroff".
>>>>>> +- gpios : The GPIO to set high/low, see "gpios property" in
>>>>>> +  Documentation/devicetree/bindings/gpio/gpio.txt. If the pin should be
>>>>>> +  low to power down the board set it to "Active Low", otherwise set
>>>>>> +  gpio to "Active High".
>>>>>
>>>>> Unfortunately, not all GPIO bindings support active high/low flags in
>>>>> the GPIO specifier. As such, the flags there are basically useless.
>>>>> Other bindings (e.g. IIRC the fixed-regulator binding) have added a
>>>>> separate active-high property to indicate the GPIO polarity. This
>>>>> binding should probably follow suite.
> 
> Should the gpio driver fix its bindings then?.. Polarity is a quite
> generic concept of a GPIO, and flags are there for a reason. I'd rather
> prefer having

There is no "GPIO driver" to fix; each GPIO driver has its own bindings,
and unfortunately, some of the GPIO binding authors chose not to include
any flags cell in the GPIO specifier (e.g. Samsung ARM SoCs IIRC, but
there are probably more).

> 	stuff-gpios = <0 0
> 		       1 0
> 		       2 1
> 		       3 0>;
> 
> Rather than
> 
> 	stuff-gpios = <0 1 2 3>;
> 	stuff-polarity-gpio-map = <0 0 1 0>;
> 
> The first scheme existed like for years already. Has it been discussed
> that it is no longer preferred?
> 
>>>> Humm, so are you saying of_get_named_gpio_flags() is deprecated?
>>>
>>> I don't know if it's deprecated, but it's certainly not useful in
>>> generic code.
>>
>> Hi Linus, Anton
>>
>> How do you see this? 
>>
>> I'm happy to implement an enable-active-high property, but it seems to
>> go against the purpose of of_get_named_gpio_flags(). Is that function
>> deprecated?
> 
> Never heard of any deprecation, and I disagree that it is "not useful in
> generic code". :)

Put more specifically, I meant: It would be useful to have such a
feature. However, since such a feature doesn't exist in all cases, it's
not possible to rely on the feature, and hence trying to isn't useful.

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Anton Vorontsov]

On Mon, Nov 12, 2012 at 11:58:47AM -0700, Stephen Warren wrote:
[...]
> >>>>> Unfortunately, not all GPIO bindings support active high/low flags in
> >>>>> the GPIO specifier. As such, the flags there are basically useless.
> >>>>> Other bindings (e.g. IIRC the fixed-regulator binding) have added a
> >>>>> separate active-high property to indicate the GPIO polarity. This
> >>>>> binding should probably follow suite.
> > 
> > Should the gpio driver fix its bindings then?.. Polarity is a quite
> > generic concept of a GPIO, and flags are there for a reason. I'd rather
> > prefer having
> 
> There is no "GPIO driver" to fix; each GPIO driver has its own bindings,
> and unfortunately, some of the GPIO binding authors chose not to include
> any flags cell in the GPIO specifier (e.g. Samsung ARM SoCs IIRC, but
> there are probably more).

They didn't read this? :)

int of_gpio_simple_xlate(struct gpio_chip *gc,
                         const struct of_phandle_args *gpiospec, u32 *flags)
{
        /*
         * We're discouraging gpio_cells < 2, since that way you'll have to
         * write your own xlate function (that will have to retrive the GPIO
         * number and the flags from a single gpio cell -- this is possible,
         * but not recommended).
         */
        if (gc->of_gpio_n_cells < 2) {
                WARN_ON(1);
                return -EINVAL;
        }

They should have gotten the WARN_ON().

If not, if they do have the second cell, then they still can encode the
flags. Just change the bindings in a backwards-compatible way.

And even if they have just one cell, just as the comment above says, they
still can add the polarity flag -- add it into the gpio number specifier.
0x0001 -- GPIO 1, 0x1001 -- GPIO 1, polarity inverted. In the gpio driver
they have to mask the flags (by implementing their own xlate), of course.

A few "broken" (but fixable) drivers/bindings is not the reason change the
whole concept, or declare a long-standing API as 'not suitable for generic
code'. At least it was meant exactly to be suitable for a generic code. :)

Thanks,
Anton.

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Linus Walleij]

On Mon, Nov 12, 2012 at 7:58 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 11/12/2012 11:43 AM, Anton Vorontsov wrote:

>> Should the gpio driver fix its bindings then?.. Polarity is a quite
>> generic concept of a GPIO, and flags are there for a reason. I'd rather
>> prefer having
>
> There is no "GPIO driver" to fix; each GPIO driver has its own bindings,
> and unfortunately, some of the GPIO binding authors chose not to include
> any flags cell in the GPIO specifier (e.g. Samsung ARM SoCs IIRC, but
> there are probably more).

So can I read this something like we have been too liberal with the
GPIO DT bindings and they are now a bit messy and need to be shaped
up? I don't know how to achieve that :-(

Alerting Grant to see if he has something to add on this subject...

Yours,
Linus Walleij

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Anton Vorontsov]

On Thu, Nov 15, 2012 at 11:35:36AM +0100, Linus Walleij wrote:
> On Mon, Nov 12, 2012 at 7:58 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> > On 11/12/2012 11:43 AM, Anton Vorontsov wrote:
> 
> >> Should the gpio driver fix its bindings then?.. Polarity is a quite
> >> generic concept of a GPIO, and flags are there for a reason. I'd rather
> >> prefer having
> >
> > There is no "GPIO driver" to fix; each GPIO driver has its own bindings,
> > and unfortunately, some of the GPIO binding authors chose not to include
> > any flags cell in the GPIO specifier (e.g. Samsung ARM SoCs IIRC, but
> > there are probably more).
> 
> So can I read this something like we have been too liberal with the
> GPIO DT bindings and they are now a bit messy and need to be shaped
> up? I don't know how to achieve that :-(

I guess there's really no reason to panic. :)

'git grep gpio-cells Documentation/' shows just mrvl-gpio.txt and
twl6040.txt having the wrong gpio-cells (i.e. 1).

But even these can use one cells for both flags and pin number (unless you
really have 4294967295 GPIOs per controller).

FWIW, current Samsung SOCs use 3 and even 4 cells for a GPIO specifier,
which is absolutely fine. Plus, the Samsung bindings do specify the
inversion flag. So, unless we have a lot of other [undocumented] bindings,
I don't see a big mess. And everything I currently see is fixable.

Thanks,
Anton.

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Linus Walleij]

On Thu, Nov 15, 2012 at 11:59 AM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> On Thu, Nov 15, 2012 at 11:35:36AM +0100, Linus Walleij wrote:
>> On Mon, Nov 12, 2012 at 7:58 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> > On 11/12/2012 11:43 AM, Anton Vorontsov wrote:
>>
>> >> Should the gpio driver fix its bindings then?.. Polarity is a quite
>> >> generic concept of a GPIO, and flags are there for a reason. I'd rather
>> >> prefer having
>> >
>> > There is no "GPIO driver" to fix; each GPIO driver has its own bindings,
>> > and unfortunately, some of the GPIO binding authors chose not to include
>> > any flags cell in the GPIO specifier (e.g. Samsung ARM SoCs IIRC, but
>> > there are probably more).
>>
>> So can I read this something like we have been too liberal with the
>> GPIO DT bindings and they are now a bit messy and need to be shaped
>> up? I don't know how to achieve that :-(
>
> I guess there's really no reason to panic. :)
>
> 'git grep gpio-cells Documentation/' shows just mrvl-gpio.txt and
> twl6040.txt having the wrong gpio-cells (i.e. 1).
>
> But even these can use one cells for both flags and pin number (unless you
> really have 4294967295 GPIOs per controller).
>
> FWIW, current Samsung SOCs use 3 and even 4 cells for a GPIO specifier,
> which is absolutely fine. Plus, the Samsung bindings do specify the
> inversion flag. So, unless we have a lot of other [undocumented] bindings,
> I don't see a big mess. And everything I currently see is fixable.

I agree it's not that big a mess...

I was more thinking about how to convince the people who can
test this to fix it up.

Yours,
Linus Walleij

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Grant Likely]

On Thu, 15 Nov 2012 12:10:59 +0100, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Nov 15, 2012 at 11:59 AM, Anton Vorontsov
> <anton.vorontsov@linaro.org> wrote:
> > On Thu, Nov 15, 2012 at 11:35:36AM +0100, Linus Walleij wrote:
> >> On Mon, Nov 12, 2012 at 7:58 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> >> > On 11/12/2012 11:43 AM, Anton Vorontsov wrote:
> >>
> >> >> Should the gpio driver fix its bindings then?.. Polarity is a quite
> >> >> generic concept of a GPIO, and flags are there for a reason. I'd rather
> >> >> prefer having
> >> >
> >> > There is no "GPIO driver" to fix; each GPIO driver has its own bindings,
> >> > and unfortunately, some of the GPIO binding authors chose not to include
> >> > any flags cell in the GPIO specifier (e.g. Samsung ARM SoCs IIRC, but
> >> > there are probably more).
> >>
> >> So can I read this something like we have been too liberal with the
> >> GPIO DT bindings and they are now a bit messy and need to be shaped
> >> up? I don't know how to achieve that :-(
> >
> > I guess there's really no reason to panic. :)
> >
> > 'git grep gpio-cells Documentation/' shows just mrvl-gpio.txt and
> > twl6040.txt having the wrong gpio-cells (i.e. 1).
> >
> > But even these can use one cells for both flags and pin number (unless you
> > really have 4294967295 GPIOs per controller).
> >
> > FWIW, current Samsung SOCs use 3 and even 4 cells for a GPIO specifier,
> > which is absolutely fine. Plus, the Samsung bindings do specify the
> > inversion flag. So, unless we have a lot of other [undocumented] bindings,
> > I don't see a big mess. And everything I currently see is fixable.
> 
> I agree it's not that big a mess...
> 
> I was more thinking about how to convince the people who can
> test this to fix it up.

+1. Most drivers use the same definitions for flags. New drivers should
be pushed to do the same. And the few that don't can be fixed up
individually.

g.

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Stephen Warren]

On 11/15/2012 03:59 AM, Anton Vorontsov wrote:
> On Thu, Nov 15, 2012 at 11:35:36AM +0100, Linus Walleij wrote:
>> On Mon, Nov 12, 2012 at 7:58 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 11/12/2012 11:43 AM, Anton Vorontsov wrote:
>>
>>>> Should the gpio driver fix its bindings then?.. Polarity is a quite
>>>> generic concept of a GPIO, and flags are there for a reason. I'd rather
>>>> prefer having
>>>
>>> There is no "GPIO driver" to fix; each GPIO driver has its own bindings,
>>> and unfortunately, some of the GPIO binding authors chose not to include
>>> any flags cell in the GPIO specifier (e.g. Samsung ARM SoCs IIRC, but
>>> there are probably more).
>>
>> So can I read this something like we have been too liberal with the
>> GPIO DT bindings and they are now a bit messy and need to be shaped
>> up? I don't know how to achieve that :-(
> 
> I guess there's really no reason to panic. :)
> 
> 'git grep gpio-cells Documentation/' shows just mrvl-gpio.txt and
> twl6040.txt having the wrong gpio-cells (i.e. 1).

If there are too-few cells, the binding and driver can always be
expanded to support more cells in a backwards-compatible way.

> But even these can use one cells for both flags and pin number (unless you
> really have 4294967295 GPIOs per controller).
> 
> FWIW, current Samsung SOCs use 3 and even 4 cells for a GPIO specifier,
> which is absolutely fine. Plus, the Samsung bindings do specify the
> inversion flag. So, unless we have a lot of other [undocumented] bindings,
> I don't see a big mess. And everything I currently see is fixable.

Oh, I always thought that the Samsung bindings were one of the major
issues here, but you're right - they do have the inversion flag already.

So, perhaps there really isn't an issue, and we should revisit the
GPIO-based regulator bindings and drivers, and allow them to
(additionally) rely on the GPIO flags in the standard way.

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Linus Walleij]

On Sun, Nov 11, 2012 at 5:21 PM, Andrew Lunn <andrew@lunn.ch> wrote:

> From: Jamie Lentin <jm@lentin.co.uk>
>
> Given appropriate devicetree bindings, this driver registers a
> pm_power_off function to set a GPIO line high/low to power down
> your board.
>
> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

So this is just a GPIO client like any such thing right?

I mean this thing has nothing more to do with the GPIO subsystem
than say, drivers/input/keyboard/gpio_keys.c?

So from the GPIO subsystem point of view controlling power is
an alien concept.

But I suspect Anton may be ready to welcome a driver like this
under drivers/power/* which I think is where it belongs.

Yours,
Linus Walleij

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Anton Vorontsov]

On Mon, Nov 12, 2012 at 02:00:31AM +0100, Linus Walleij wrote:
> On Sun, Nov 11, 2012 at 5:21 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > From: Jamie Lentin <jm@lentin.co.uk>
> >
> > Given appropriate devicetree bindings, this driver registers a
> > pm_power_off function to set a GPIO line high/low to power down
> > your board.
> >
> > Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> 
> So this is just a GPIO client like any such thing right?
> 
> I mean this thing has nothing more to do with the GPIO subsystem
> than say, drivers/input/keyboard/gpio_keys.c?
> 
> So from the GPIO subsystem point of view controlling power is
> an alien concept.
> 
> But I suspect Anton may be ready to welcome a driver like this
> under drivers/power/* which I think is where it belongs.

Yup. driver/power/reset/ seems appropriate.

Thanks,
Anton.

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Andrew Lunn]

On Sun, Nov 11, 2012 at 05:12:58PM -0800, Anton Vorontsov wrote:
> On Mon, Nov 12, 2012 at 02:00:31AM +0100, Linus Walleij wrote:
> > On Sun, Nov 11, 2012 at 5:21 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> > 
> > > From: Jamie Lentin <jm@lentin.co.uk>
> > >
> > > Given appropriate devicetree bindings, this driver registers a
> > > pm_power_off function to set a GPIO line high/low to power down
> > > your board.
> > >
> > > Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
> > > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > 
> > So this is just a GPIO client like any such thing right?
> > 
> > I mean this thing has nothing more to do with the GPIO subsystem
> > than say, drivers/input/keyboard/gpio_keys.c?
> > 
> > So from the GPIO subsystem point of view controlling power is
> > an alien concept.
> > 
> > But I suspect Anton may be ready to welcome a driver like this
> > under drivers/power/* which I think is where it belongs.
> 
> Yup. driver/power/reset/ seems appropriate.

Hi Anton

I was unsure where to put it. I will submit new patches with the move.

I'm not too sure about 'reset' though. Its not a reset operation, the
power goes off and stays off. Would 'shutdown' or 'power_off' be
better?

  Andrew

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Anton Vorontsov]

Hi Andrew,

On Mon, Nov 12, 2012 at 07:07:40AM +0100, Andrew Lunn wrote:
[...]
> > > But I suspect Anton may be ready to welcome a driver like this
> > > under drivers/power/* which I think is where it belongs.
> > 
> > Yup. driver/power/reset/ seems appropriate.
> 
> I was unsure where to put it. I will submit new patches with the move.
> 
> I'm not too sure about 'reset' though. Its not a reset operation, the
> power goes off and stays off. Would 'shutdown' or 'power_off' be
> better?

Other folks already wanted drivers/power/reset/ for board reset/poweroff
drivers. But it makes little sense to make both reset and poweroff
directories. So, let's just do 'reset', somebody might have already using
it in their private trees.

Thanks,
Anton.

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Grant Likely]

On Sun, 11 Nov 2012 17:21:29 +0100, Andrew Lunn <andrew@lunn.ch> wrote:
> From: Jamie Lentin <jm@lentin.co.uk>
> 
> Given appropriate devicetree bindings, this driver registers a
> pm_power_off function to set a GPIO line high/low to power down
> your board.
> 
> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  .../devicetree/bindings/gpio/gpio-poweroff.txt     |   22 ++++
>  drivers/gpio/Kconfig                               |   10 ++
>  drivers/gpio/Makefile                              |    2 +
>  drivers/gpio/gpio-poweroff.c                       |  129 ++++++++++++++++++++

This isn't a gpio controller driver. Don't put it into drivers/gpio

g.

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Jamie Lentin]

On Thu, 15 Nov 2012, Grant Likely wrote:

> On Sun, 11 Nov 2012 17:21:29 +0100, Andrew Lunn <andrew@lunn.ch> wrote:
>> From: Jamie Lentin <jm@lentin.co.uk>
>>
>> Given appropriate devicetree bindings, this driver registers a
>> pm_power_off function to set a GPIO line high/low to power down
>> your board.
>>
>> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>> ---
>>  .../devicetree/bindings/gpio/gpio-poweroff.txt     |   22 ++++
>>  drivers/gpio/Kconfig                               |   10 ++
>>  drivers/gpio/Makefile                              |    2 +
>>  drivers/gpio/gpio-poweroff.c                       |  129 ++++++++++++++++++++
>
> This isn't a gpio controller driver. Don't put it into drivers/gpio

There's a v2 which puts it in drivers/power/reset/gpio-poweroff.c

>
> g.
>
>

-- 
Jamie Lentin

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Grant Likely]

On Thu, Nov 15, 2012 at 6:11 PM, Jamie Lentin <jm@lentin.co.uk> wrote:
> On Thu, 15 Nov 2012, Grant Likely wrote:
>
>> On Sun, 11 Nov 2012 17:21:29 +0100, Andrew Lunn <andrew@lunn.ch> wrote:
>>>
>>> From: Jamie Lentin <jm@lentin.co.uk>
>>>
>>> Given appropriate devicetree bindings, this driver registers a
>>> pm_power_off function to set a GPIO line high/low to power down
>>> your board.
>>>
>>> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
>>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>>> ---
>>>  .../devicetree/bindings/gpio/gpio-poweroff.txt     |   22 ++++
>>>  drivers/gpio/Kconfig                               |   10 ++
>>>  drivers/gpio/Makefile                              |    2 +
>>>  drivers/gpio/gpio-poweroff.c                       |  129
>>> ++++++++++++++++++++
>>
>>
>> This isn't a gpio controller driver. Don't put it into drivers/gpio
>
>
> There's a v2 which puts it in drivers/power/reset/gpio-poweroff.c

Okay, thanks.

g.

[PATCH 1/3] gpio: Add simple poweroff-gpio driver

[Grant Likely]

On Thu, 15 Nov 2012 18:21:20 +0000, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Thu, Nov 15, 2012 at 6:11 PM, Jamie Lentin <jm@lentin.co.uk> wrote:
> > On Thu, 15 Nov 2012, Grant Likely wrote:
> >
> >> On Sun, 11 Nov 2012 17:21:29 +0100, Andrew Lunn <andrew@lunn.ch> wrote:
> >>>
> >>> From: Jamie Lentin <jm@lentin.co.uk>
> >>>
> >>> Given appropriate devicetree bindings, this driver registers a
> >>> pm_power_off function to set a GPIO line high/low to power down
> >>> your board.
> >>>
> >>> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
> >>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> >>> ---
> >>>  .../devicetree/bindings/gpio/gpio-poweroff.txt     |   22 ++++
> >>>  drivers/gpio/Kconfig                               |   10 ++
> >>>  drivers/gpio/Makefile                              |    2 +
> >>>  drivers/gpio/gpio-poweroff.c                       |  129
> >>> ++++++++++++++++++++
> >>
> >>
> >> This isn't a gpio controller driver. Don't put it into drivers/gpio
> >
> >
> > There's a v2 which puts it in drivers/power/reset/gpio-poweroff.c
> 
> Okay, thanks.
> 
> g.

So... I just read the power sequence proposal patch series which is
remarkably like a superset of what is needed here. Does it make sense to
use a power sequence for doing generic power off instead of this
interface?

g.

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

[PATCH 2/3] ARM: Kirkwood: Convert DNSKW to use gpio-poweroff.

[Andrew Lunn]

Also enable the gpio-poweroff driver when DT is used.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Jamie Lentin <jm@lentin.co.uk>
---
 arch/arm/boot/dts/kirkwood-dnskw.dtsi |    5 +++++
 arch/arm/mach-kirkwood/Kconfig        |    1 +
 arch/arm/mach-kirkwood/board-dnskw.c  |   12 ------------
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/arch/arm/boot/dts/kirkwood-dnskw.dtsi b/arch/arm/boot/dts/kirkwood-dnskw.dtsi
index 9b32d02..22aa07a 100644
--- a/arch/arm/boot/dts/kirkwood-dnskw.dtsi
+++ b/arch/arm/boot/dts/kirkwood-dnskw.dtsi
@@ -35,6 +35,11 @@
 				      6000 2>;
 	};
 
+	gpio_poweroff {
+		compatible = "gpio-poweroff";
+		gpios = <&gpio1 4 0>;
+	};
+
 	ocp at f1000000 {
 		sata at 80000 {
 			status = "okay";
diff --git a/arch/arm/mach-kirkwood/Kconfig b/arch/arm/mach-kirkwood/Kconfig
index 50bca50..a0f4f47 100644
--- a/arch/arm/mach-kirkwood/Kconfig
+++ b/arch/arm/mach-kirkwood/Kconfig
@@ -47,6 +47,7 @@ config MACH_GURUPLUG
 config ARCH_KIRKWOOD_DT
 	bool "Marvell Kirkwood Flattened Device Tree"
 	select USE_OF
+	select GPIO_POWEROFF
 	help
 	  Say 'Y' here if you want your kernel to support the
 	  Marvell Kirkwood using flattened device tree.
diff --git a/arch/arm/mach-kirkwood/board-dnskw.c b/arch/arm/mach-kirkwood/board-dnskw.c
index 43d16d6..549369e 100644
--- a/arch/arm/mach-kirkwood/board-dnskw.c
+++ b/arch/arm/mach-kirkwood/board-dnskw.c
@@ -57,11 +57,6 @@ static unsigned int dnskw_mpp_config[] __initdata = {
 	0
 };
 
-static void dnskw_power_off(void)
-{
-	gpio_set_value(36, 1);
-}
-
 /* Register any GPIO for output and set the value */
 static void __init dnskw_gpio_register(unsigned gpio, char *name, int def)
 {
@@ -81,13 +76,6 @@ void __init dnskw_init(void)
 	kirkwood_ehci_init();
 	kirkwood_ge00_init(&dnskw_ge00_data);
 
-	/* Register power-off GPIO. */
-	if (gpio_request(36, "dnskw:power:off") == 0
-	    && gpio_direction_output(36, 0) == 0)
-		pm_power_off = dnskw_power_off;
-	else
-		pr_err("dnskw: failed to configure power-off GPIO\n");
-
 	/* Ensure power is supplied to both HDDs */
 	dnskw_gpio_register(39, "dnskw:power:sata0", 1);
 	dnskw_gpio_register(40, "dnskw:power:sata1", 1);
-- 
1.7.10.4

[PATCH 3/3] ARM: Kirkwood: Convert IB62x0 to use gpio-poweroff.

[Andrew Lunn]

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 arch/arm/boot/dts/kirkwood-ib62x0.dts |    6 ++++++
 arch/arm/mach-kirkwood/board-ib62x0.c |   13 -------------
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/arch/arm/boot/dts/kirkwood-ib62x0.dts b/arch/arm/boot/dts/kirkwood-ib62x0.dts
index 66794ed..9b0e2e3 100644
--- a/arch/arm/boot/dts/kirkwood-ib62x0.dts
+++ b/arch/arm/boot/dts/kirkwood-ib62x0.dts
@@ -79,4 +79,10 @@
 			gpios = <&gpio0 27 0>;
 		};
 	};
+	gpio_poweroff {
+		compatible = "gpio-poweroff";
+		gpios = <&gpio0 24 1>;
+	};
+
+
 };
diff --git a/arch/arm/mach-kirkwood/board-ib62x0.c b/arch/arm/mach-kirkwood/board-ib62x0.c
index cfc47f8..7b8adf5 100644
--- a/arch/arm/mach-kirkwood/board-ib62x0.c
+++ b/arch/arm/mach-kirkwood/board-ib62x0.c
@@ -17,7 +17,6 @@
 #include <linux/mtd/partitions.h>
 #include <linux/ata_platform.h>
 #include <linux/mv643xx_eth.h>
-#include <linux/gpio.h>
 #include <linux/input.h>
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -25,8 +24,6 @@
 #include "common.h"
 #include "mpp.h"
 
-#define IB62X0_GPIO_POWER_OFF	24
-
 static struct mv643xx_eth_platform_data ib62x0_ge00_data = {
 	.phy_addr	= MV643XX_ETH_PHY_ADDR(8),
 };
@@ -49,11 +46,6 @@ static unsigned int ib62x0_mpp_config[] __initdata = {
 	0
 };
 
-static void ib62x0_power_off(void)
-{
-	gpio_set_value(IB62X0_GPIO_POWER_OFF, 1);
-}
-
 void __init ib62x0_init(void)
 {
 	/*
@@ -63,9 +55,4 @@ void __init ib62x0_init(void)
 
 	kirkwood_ehci_init();
 	kirkwood_ge00_init(&ib62x0_ge00_data);
-	if (gpio_request(IB62X0_GPIO_POWER_OFF, "ib62x0:power:off") == 0 &&
-	    gpio_direction_output(IB62X0_GPIO_POWER_OFF, 0) == 0)
-		pm_power_off = ib62x0_power_off;
-	else
-		pr_err("board-ib62x0: failed to configure power-off GPIO\n");
 }
-- 
1.7.10.4