ARM: kirkwood: Add support for RaidSonic
Subsystems (2)
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/1 ARM: kirkwood: Add support for RaidSonic
Quoted 21 lines · Andrew Lunn
Hi Simon
The patch looks good.
(quoted diff) +4
-0+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.cmach-orion5x/mv2120-setup.cmach-kirkwood/d2net_v2-setup.cmach-kirkwood/netspace_v2-setup.cand a few more.
Quoted 8 lines
How about writing a gpio-poweroff platform driver? It would take a
platform_datastructure 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 forpm_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.
Quoted 3 lines
Andrew
Signature · 1 line
Jamie Lentin
PATCH 1/1 ARM: kirkwood: Add support for RaidSonic
Quoted 30 lines · Jamie Lentin
Quoted 28 lines · Andrew Lunn
Hi Simon
The patch looks good.
(quoted diff) +4
-0+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.cmach-orion5x/mv2120-setup.cmach-kirkwood/d2net_v2-setup.cmach-kirkwood/netspace_v2-setup.cand a few more.
How about writing a gpio-poweroff platform driver? It would take a
platform_datastructure 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 forpm_power_off(). When that function is called, it toggles the GPIO to turn the power off.
Quoted 5 lines
Quoted 1 line
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_offfunction 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
Quoted 31 lines · Andrew Lunn
Quoted 30 lines · Jamie Lentin
Quoted 28 lines · Andrew Lunn
Hi Simon
The patch looks good.
(quoted diff) +4
-0+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.cmach-orion5x/mv2120-setup.cmach-kirkwood/d2net_v2-setup.cmach-kirkwood/netspace_v2-setup.cand a few more.
How about writing a gpio-poweroff platform driver? It would take a
platform_datastructure 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 forpm_power_off(). When that function is called, it toggles the GPIO to turn the power off.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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_offfunction wasn't immediately obvious.
pm_power_offis 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.
Quoted 2 lines
Andrew
Signature · 1 line
Jamie Lentin
PATCH 1/1 ARM: kirkwood: Add support for RaidSonic
Hi Jamie, Andrew,
Am 02.05.2012 15:29, schrieb Jamie Lentin:
Quoted 17 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
How about writing a gpio-poweroff platform driver? It would take a
platform_datastructure 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 forpm_power_off(). When that function is called, it toggles the GPIO to turn the power off.Quoted 9 lines
Quoted 5 lines
Quoted 1 line
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_offfunction wasn't immediately obvious.
pm_power_offis a global variable. You should be able to assign it from anywhere.
Quoted 5 lines
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
Quoted 3 lines
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 1/1 gpio: Add simple poweroff-gpio driver
In reply to Andrew LunnGiven appropriate devicetree bindings, this driver registers a pm_power_off function to set a GPIO line high/low and power down your board.
---
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
Documentation/devicetree/bindings/gpio/poweroff-gpio.txt new file +15 -0
@@ -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 */ +};
drivers/gpio/Kconfig +10 -0
@@ -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
drivers/gpio/Makefile +2 -0
@@ -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
drivers/gpio/poweroff-gpio.c new file +95 -0
95 lines not on this page · raw message
Signature · 1 line
1.7.10
PATCH 1/1 gpio: Add simple poweroff-gpio driver
Quoted 7 lines
The main issue is where to put it. I'm not convinced that
drivers/gpiois an appropriate home, howeverdrivers/poweranddrivers/hwmon(where drivers with similar purpose live) contain frameworks this is not part of.drivers/mfdhas some modules that can power off, but this is hardly multifunction :) Could also give in and usedrivers/misc.Feedback appreciated.
Humm, sorry, no idea...
drivers/gpio/poweroff-gpio.c line 42
Quoted 11 lines
drivers/gpio/poweroff-gpio.c new file +11
-0+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.
drivers/gpio/poweroff-gpio.c line 68
Quoted 4 lines
drivers/gpio/poweroff-gpio.c new file +4
-0+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 1/1 gpio: Add simple poweroff-gpio driver
Quoted 7 lines · Andrew Lunn
Quoted 7 lines
drivers/gpio/poweroff-gpio.c new file +7
-0+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",
Quoted 8 lines
drivers/gpio/poweroff-gpio.c line 42
Quoted 4 lines
drivers/gpio/poweroff-gpio.c new file +4
-0+ __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_dataas 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.
Quoted 7 lines
drivers/gpio/poweroff-gpio.c line 68
Quoted 4 lines
drivers/gpio/poweroff-gpio.c new file +4
-0+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,
Quoted 3 lines
Andrew
Signature · 1 line
Jamie Lentin
PATCH 1/1 gpio: Add simple poweroff-gpio driver
Quoted 9 lines
Quoted 3 lines
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_dataas 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_offas most of them currently do? Seemed to me that it was better to not complicate the code withplatform_datawhen 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 1/1 gpio: Add simple poweroff-gpio driver
In reply to Jamie Lentindrivers/gpio/poweroff-gpio.c line 59
Quoted 9 lines
drivers/gpio/poweroff-gpio.c new file +9
-0+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 1/1 gpio: Add simple poweroff-gpio driver
In reply to Jamie LentinHi Jamie,
drivers/gpio/Kconfig config GPIO_SYSFS · line 70
Quoted 11 lines · Jamie Lentin
drivers/gpio/Kconfig +5
-0@@ -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 1/1 gpio: Add simple poweroff-gpio driver
In reply to Jamie LentinQuoted 12 lines · Jamie Lentin
Given appropriate devicetree bindings, this driver registers a
pm_power_offfunction to set a GPIO line high/low and power down your board.Signed-off-by Jamie Lentin <[email protected]>---
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
Quoted 7 lines
The main issue is where to put it. I'm not convinced that
drivers/gpiois an appropriate home, howeverdrivers/poweranddrivers/hwmon(where drivers with similar purpose live) contain frameworks this is not part of.drivers/mfdhas some modules that can power off, but this is hardly multifunction :) Could also give in and usedrivers/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.