[PATCH 1/1] ARM: kirkwood: Add support for RaidSonic

[PATCH 1/1] ARM: kirkwood: Add support for RaidSonic

[Jamie Lentin]

On Tue, 1 May 2012, Andrew Lunn wrote:

> Hi Simon
>
> The patch looks good.
>
> +static void ib62x0_power_off(void)
> +{
> +       gpio_set_value(IB62X0_GPIO_POWER_OFF, 1);
> +}
>
> Seeing this set of a train of thought. I have a feature creep
> request. Nice to have, i.e. not required to acceptance. This patterns
> of using a GPIO line to turn the hardware off is seen quite a few
> times. e.g.
>
> mach-kirkwood/netspace_v2-setup.c
> mach-orion5x/mv2120-setup.c
> mach-kirkwood/d2net_v2-setup.c
> mach-kirkwood/netspace_v2-setup.c
>
> and a few more.
>
> How about writing a gpio-poweroff platform driver? It would take a
> platform_data structure which lists which GPIO line to use, and if it
> should be active high/low. The probe function would take control of
> the GPIO line, set it to inactive state, and output. It would also
> register a function for pm_power_off(). When that function is called,
> it toggles the GPIO to turn the power off.
>
> Then wrap all this up in a DT binding.

I've been thinking about this too, would make a lot of sense. Most of the 
gpio-poweroff driver would be easy, although registering the power_off 
function wasn't immediately obvious.

In the process of this though, I stumbled on the gpio-regulator driver and 
emails suggesting to use this. It seems like it would make sense for the 
GPIO pins on the DNS-320 that turn on/off SATA drives, and intend to have 
a go at using this instead. This could possibly be used for power off too, 
but might be streatching it a bit too far.

>
>     Andrew
>

-- 
Jamie Lentin

[PATCH 1/1] ARM: kirkwood: Add support for RaidSonic

[Andrew Lunn]

On Tue, May 01, 2012 at 07:34:29PM +0100, Jamie Lentin wrote:
> On Tue, 1 May 2012, Andrew Lunn wrote:
> 
> >Hi Simon
> >
> >The patch looks good.
> >
> >+static void ib62x0_power_off(void)
> >+{
> >+       gpio_set_value(IB62X0_GPIO_POWER_OFF, 1);
> >+}
> >
> >Seeing this set of a train of thought. I have a feature creep
> >request. Nice to have, i.e. not required to acceptance. This patterns
> >of using a GPIO line to turn the hardware off is seen quite a few
> >times. e.g.
> >
> >mach-kirkwood/netspace_v2-setup.c
> >mach-orion5x/mv2120-setup.c
> >mach-kirkwood/d2net_v2-setup.c
> >mach-kirkwood/netspace_v2-setup.c
> >
> >and a few more.
> >
> >How about writing a gpio-poweroff platform driver? It would take a
> >platform_data structure which lists which GPIO line to use, and if it
> >should be active high/low. The probe function would take control of
> >the GPIO line, set it to inactive state, and output. It would also
> >register a function for pm_power_off(). When that function is called,
> >it toggles the GPIO to turn the power off.
> >
> >Then wrap all this up in a DT binding.
> 
> I've been thinking about this too, would make a lot of sense. Most
> of the gpio-poweroff driver would be easy, although registering the
> power_off function wasn't immediately obvious.

pm_power_off is a global variable. You should be able to assign it
from anywhere.

     Andrew

[PATCH 1/1] ARM: kirkwood: Add support for RaidSonic

[Jamie Lentin]

On Tue, 01 May 2012 20:56:03 +0100, Andrew Lunn <andrew@lunn.ch> wrote:

> On Tue, May 01, 2012 at 07:34:29PM +0100, Jamie Lentin wrote:
>> On Tue, 1 May 2012, Andrew Lunn wrote:
>>
>> >Hi Simon
>> >
>> >The patch looks good.
>> >
>> >+static void ib62x0_power_off(void)
>> >+{
>> >+       gpio_set_value(IB62X0_GPIO_POWER_OFF, 1);
>> >+}
>> >
>> >Seeing this set of a train of thought. I have a feature creep
>> >request. Nice to have, i.e. not required to acceptance. This patterns
>> >of using a GPIO line to turn the hardware off is seen quite a few
>> >times. e.g.
>> >
>> >mach-kirkwood/netspace_v2-setup.c
>> >mach-orion5x/mv2120-setup.c
>> >mach-kirkwood/d2net_v2-setup.c
>> >mach-kirkwood/netspace_v2-setup.c
>> >
>> >and a few more.
>> >
>> >How about writing a gpio-poweroff platform driver? It would take a
>> >platform_data structure which lists which GPIO line to use, and if it
>> >should be active high/low. The probe function would take control of
>> >the GPIO line, set it to inactive state, and output. It would also
>> >register a function for pm_power_off(). When that function is called,
>> >it toggles the GPIO to turn the power off.
>> >
>> >Then wrap all this up in a DT binding.
>>
>> I've been thinking about this too, would make a lot of sense. Most
>> of the gpio-poweroff driver would be easy, although registering the
>> power_off function wasn't immediately obvious.
>
> pm_power_off is a global variable. You should be able to assign it
> from anywhere.

It seemed a bit worrying setting the global within a driver when it might  
be overridden elsewhere, but I guess it's fairly unlikely a board would  
invoke several means of turning off. A grep shows several drivers that  
already do this (setting pm_power_off) anyway. I'll have a go at writing  
one later.

>
>      Andrew


-- 
Jamie Lentin

[PATCH 1/1] ARM: kirkwood: Add support for RaidSonic

[Simon Baatz]

Hi Jamie, Andrew,

Am 02.05.2012 15:29, schrieb Jamie Lentin:
>>> >How about writing a gpio-poweroff platform driver? It would take a
>>> >platform_data structure which lists which GPIO line to use, and if it
>>> >should be active high/low. The probe function would take control of
>>> >the GPIO line, set it to inactive state, and output. It would also
>>> >register a function for pm_power_off(). When that function is called,
>>> >it toggles the GPIO to turn the power off.
>>> >
>>> >Then wrap all this up in a DT binding.
>>>
>>> I've been thinking about this too, would make a lot of sense. Most
>>> of the gpio-poweroff driver would be easy, although registering the
>>> power_off function wasn't immediately obvious.
>>
>> pm_power_off is a global variable. You should be able to assign it
>> from anywhere.
> >
>
> It seemed a bit worrying setting the global within a driver when it
> might be overridden elsewhere, but I guess it's fairly unlikely a
> board would invoke several means of turning off. A grep shows several
> drivers that already do this (setting pm_power_off) anyway. I'll have
> a go at writing one later.

This saves me a long-winded explanation why I would prefer someone more
experienced to do this before I give it a try...
Of course I am happy to support e.g. by testing. Just let me know.

Andrew, thanks for the review of the ib62x0 patch. Once there is a
gpio-poweroff driver, should I integrate the adaptations to use it into
the existing patch or would you like to have a dedicated patch for that?
(I prefer the latter.)

- Simon

[PATCH 1/1] ARM: kirkwood: Add support for RaidSonic

[Andrew Lunn]

> Andrew, thanks for the review of the ib62x0 patch. Once there is a
> gpio-poweroff driver, should I integrate the adaptations to use it into
> the existing patch or would you like to have a dedicated patch for that?

Hi Simon

As i said, this is nice to have. It should not block your patch. Once
the driver is available, you can submit a follow up patch to make use
of it.

   Andrew

[PATCH] gpio: Add simple poweroff-gpio driver

[Jamie Lentin]

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

Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
---
Here's an attempt at doing this. All works fine, I've not submitted a
patch to use it since GPIO patches for kirkwood aren't finalised, but
you can see it being used here:-

https://github.com/lentinj/linux.git v3.4-rc4-clk-next-orion-dnskw-gpio

The main issue is where to put it. I'm not convinced that drivers/gpio
is an appropriate home, however drivers/power and drivers/hwmon (where
drivers with similar purpose live) contain frameworks this is not part
of. drivers/mfd has some modules that can power off, but this is hardly
multifunction :) Could also give in and use drivers/misc.

Feedback appreciated.

 .../devicetree/bindings/gpio/poweroff-gpio.txt     |   15 ++++
 drivers/gpio/Kconfig                               |   10 +++
 drivers/gpio/Makefile                              |    2 +
 drivers/gpio/poweroff-gpio.c                       |   95 ++++++++++++++++++++
 4 files changed, 122 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/poweroff-gpio.txt
 create mode 100644 drivers/gpio/poweroff-gpio.c

diff --git a/Documentation/devicetree/bindings/gpio/poweroff-gpio.txt b/Documentation/devicetree/bindings/gpio/poweroff-gpio.txt
new file mode 100644
index 0000000..be52486
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/poweroff-gpio.txt
@@ -0,0 +1,15 @@
+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".
+
+Examples:
+
+gpio-poweroff {
+	compatible = "gpio-poweroff";
+	gpios = <&gpio 4 0>; /* GPIO 4 Active High */
+};
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e03653d..48a531b 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -63,6 +63,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 call.
+	  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 007f54b..54adeb8 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -4,6 +4,8 @@ ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
 
 obj-$(CONFIG_GPIOLIB)		+= gpiolib.o devres.o
 
+obj-$(CONFIG_GPIO_POWEROFF)	+= poweroff-gpio.o
+
 # Device drivers. Generally keep list sorted alphabetically
 obj-$(CONFIG_GPIO_GENERIC)	+= gpio-generic.o
 
diff --git a/drivers/gpio/poweroff-gpio.c b/drivers/gpio/poweroff-gpio.c
new file mode 100644
index 0000000..3d216aa
--- /dev/null
+++ b/drivers/gpio/poweroff-gpio.c
@@ -0,0 +1,95 @@
+/*
+ * Toggles a GPIO pin to power down a device
+ *
+ * Jamie Lentin <jm@lentin.co.uk>
+ * 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/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;
+static int gpio_active_low;
+
+static void gpio_poweroff_do_poweroff(void)
+{
+	gpio_set_value(gpio_num, !gpio_active_low);
+}
+
+static int __devinit gpio_poweroff_probe(struct platform_device *pdev)
+{
+	enum of_gpio_flags flags;
+
+	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 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;
+	}
+
+	if (gpio_request(gpio_num, "poweroff-gpio")) {
+		pr_err("%s: Could not get GPIO %d", __func__, gpio_num);
+		return -ENODEV;
+	}
+	if (gpio_direction_output(gpio_num, gpio_active_low)) {
+		pr_err("%s: Could not set direction of GPIO %d",
+		       __func__, gpio_num);
+		return -ENODEV;
+	}
+
+	pm_power_off = &gpio_poweroff_do_poweroff;
+	return 0;
+}
+
+static int __devexit gpio_poweroff_remove(struct platform_device *pdev)
+{
+	if (gpio_num)
+		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

[PATCH] gpio: Add simple poweroff-gpio driver

[Andrew Lunn]

> The main issue is where to put it. I'm not convinced that drivers/gpio
> is an appropriate home, however drivers/power and drivers/hwmon (where
> drivers with similar purpose live) contain frameworks this is not part
> of. drivers/mfd has some modules that can power off, but this is hardly
> multifunction :) Could also give in and use drivers/misc.
> 
> Feedback appreciated.

Humm, sorry, no idea...

> +static int __devinit gpio_poweroff_probe(struct platform_device *pdev)
> +{
> +	enum of_gpio_flags flags;
> +
> +	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 you look at the Kirkwood/Orion5x, most systems that could use this
are currently not DT enabled. So i think it needs old style
platform_data as well as OF.

> +static int __devexit gpio_poweroff_remove(struct platform_device *pdev)
> +{
> +	if (gpio_num)
> +		gpio_free(gpio_num);

GPIO 0 is a valid GPIO number. You should not need this check, since
if the probe function succeeded, you have a valid GPIO.

    Andrew

[PATCH] gpio: Add simple poweroff-gpio driver

[Jamie Lentin]

On Thu, 3 May 2012, Andrew Lunn wrote:

>> +static int __devinit gpio_poweroff_probe(struct platform_device *pdev)
>> +{
>> +	enum of_gpio_flags flags;
>> +
>> +	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 you look at the Kirkwood/Orion5x, most systems that could use this
> are currently not DT enabled. So i think it needs old style
> platform_data as well as OF.

I wondered about this, but if you have board support code instead of
devicetree, would you have any reasons to use this driver over just 
implementing the pm_power_off as most of them currently do? Seemed to me 
that it was better to not complicate the code with platform_data when it's 
practical use wasn't that high.

All that said, it's not a big deal to add in.

>> +static int __devexit gpio_poweroff_remove(struct platform_device *pdev)
>> +{
>> +	if (gpio_num)
>> +		gpio_free(gpio_num);
>
> GPIO 0 is a valid GPIO number. You should not need this check, since
> if the probe function succeeded, you have a valid GPIO.

Curses, you're right. Will fix this and the bug in the next mail.

Cheers,

>
>    Andrew
>

-- 
Jamie Lentin

[PATCH] gpio: Add simple poweroff-gpio driver

[Andrew Lunn]

> >If you look at the Kirkwood/Orion5x, most systems that could use this
> >are currently not DT enabled. So i think it needs old style
> >platform_data as well as OF.
> 
> I wondered about this, but if you have board support code instead of
> devicetree, would you have any reasons to use this driver over just
> implementing the pm_power_off as most of them currently do? Seemed
> to me that it was better to not complicate the code with
> platform_data when it's practical use wasn't that high.

I just see that we have the same board support code repeated X
times. We can replace it with a platform device structures. We can
also add a function to create these structures, so in the end, we have
a one-liner in the board support code.

	  Andrew

[PATCH] gpio: Add simple poweroff-gpio driver

[Andrew Lunn]

> +	if (gpio_request(gpio_num, "poweroff-gpio")) {
> +		pr_err("%s: Could not get GPIO %d", __func__, gpio_num);
> +		return -ENODEV;
> +	}
> +	if (gpio_direction_output(gpio_num, gpio_active_low)) {
> +		pr_err("%s: Could not set direction of GPIO %d",
> +		       __func__, gpio_num);
> +		return -ENODEV;
> +	}

On error, you are missing a gpio_free().

   Andrew

[PATCH] gpio: Add simple poweroff-gpio driver

[Luka Perkov]

Hi Jamie,

On Thu, May 03, 2012 at 09:24:06AM +0100, Jamie Lentin wrote:
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -63,6 +63,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 call.

Dot after via seems like a typo.

Other than this I have no comments.

Luka

[PATCH] gpio: Add simple poweroff-gpio driver

[Russell King - ARM Linux]

On Thu, May 03, 2012 at 09:24:06AM +0100, Jamie Lentin wrote:
> Given appropriate devicetree bindings, this driver registers a
> pm_power_off function to set a GPIO line high/low and power down
> your board.
> 
> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
> ---
> Here's an attempt at doing this. All works fine, I've not submitted a
> patch to use it since GPIO patches for kirkwood aren't finalised, but
> you can see it being used here:-
> 
> https://github.com/lentinj/linux.git v3.4-rc4-clk-next-orion-dnskw-gpio
> 
> The main issue is where to put it. I'm not convinced that drivers/gpio
> is an appropriate home, however drivers/power and drivers/hwmon (where
> drivers with similar purpose live) contain frameworks this is not part
> of. drivers/mfd has some modules that can power off, but this is hardly
> multifunction :) Could also give in and use drivers/misc.
> 
> Feedback appreciated.

We already have something like this, but specific to a group of SoCs.

Please take a look at arch/arm/mach-pxa/reset.c and consider the behaviour
found there, so we don't end up creating something which PXA can't use.
Thanks.

Note that some PXA platforms want the GPIO for resetting to be configured
as an input when it's not required to assert the reset signal.