Re: power: Add simple poweroff-gpio driver

Re: power: Add simple poweroff-gpio driver

[Dave Jones]

On Thu, Dec 13, 2012 at 08:21:57PM +0000, Linux Kernel wrote:
 > Gitweb:     http://git.kernel.org/linus/;a=commit;h=96ff0f5c7efd4a2205c48a76a6a1fcd2731e6128
 > Commit:     96ff0f5c7efd4a2205c48a76a6a1fcd2731e6128
 > Parent:     f4a00139b7cbeff538e616a21f6b57249a9d3ed8
 > Author:     Jamie Lentin <jm@lentin.co.uk>
 > AuthorDate: Sat Nov 17 09:51:04 2012 +0100
 > Committer:  Jason Cooper <jason@lakedaemon.net>
 > CommitDate: Sat Nov 24 02:56:38 2012 +0000
 > 
 >     power: Add simple poweroff-gpio driver
 >     
 >     Given appropriate devicetree bindings, this driver registers a
 >     pm_power_off function to set a GPIO line high/low to power down
 >     your board.

Given this seems to be dependant on device-tree, shouldn't there be
some 'depends on' in the kconfig to prevent this showing up on architectures
that don't implement it ? 

 > +menuconfig POWER_RESET
 > +	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
 > +	help
 > +	  This driver supports turning off your board via a GPIO line.
 > +	  If your board needs a GPIO high/low to power down, say Y and
 > +	  create a binding in your devicetree.

If not, upon seeing this, I suspect many users will ask "how do I know if I need this?"
given there's no mention of the sort of hardware this is useful on.

	Dave

Re: power: Add simple poweroff-gpio driver

[Andrew Lunn]

On Thu, Dec 13, 2012 at 03:51:59PM -0500, Dave Jones wrote:
> On Thu, Dec 13, 2012 at 08:21:57PM +0000, Linux Kernel wrote:
>  > Gitweb:     http://git.kernel.org/linus/;a=commit;h=96ff0f5c7efd4a2205c48a76a6a1fcd2731e6128
>  > Commit:     96ff0f5c7efd4a2205c48a76a6a1fcd2731e6128
>  > Parent:     f4a00139b7cbeff538e616a21f6b57249a9d3ed8
>  > Author:     Jamie Lentin <jm@lentin.co.uk>
>  > AuthorDate: Sat Nov 17 09:51:04 2012 +0100
>  > Committer:  Jason Cooper <jason@lakedaemon.net>
>  > CommitDate: Sat Nov 24 02:56:38 2012 +0000
>  > 
>  >     power: Add simple poweroff-gpio driver
>  >     
>  >     Given appropriate devicetree bindings, this driver registers a
>  >     pm_power_off function to set a GPIO line high/low to power down
>  >     your board.
> 
> Given this seems to be dependant on device-tree, shouldn't there be
> some 'depends on' in the kconfig to prevent this showing up on architectures
> that don't implement it ? 
> 
>  > +menuconfig POWER_RESET
>  > +	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

Hi Dave

Don't these depends on here do what you want?

>  > +	help
>  > +	  This driver supports turning off your board via a GPIO line.
>  > +	  If your board needs a GPIO high/low to power down, say Y and
>  > +	  create a binding in your devicetree.
> 
> If not, upon seeing this, I suspect many users will ask "how do I know if I need this?"
> given there's no mention of the sort of hardware this is useful on.

Its a generic driver. I know its useful on various Marvell kirkwood
and orion5x devices. I've also heard it useful on some Tegra boards.

Are you asking i list these boards?

    Thanks
	Andrew

Re: power: Add simple poweroff-gpio driver

[Jamie Lentin]

On Fri, 14 Dec 2012, Andrew Lunn wrote:

> On Thu, Dec 13, 2012 at 03:51:59PM -0500, Dave Jones wrote:
>> On Thu, Dec 13, 2012 at 08:21:57PM +0000, Linux Kernel wrote:
>> > Gitweb:     http://git.kernel.org/linus/;a=commit;h=96ff0f5c7efd4a2205c48a76a6a1fcd2731e6128
>> > Commit:     96ff0f5c7efd4a2205c48a76a6a1fcd2731e6128
>> > Parent:     f4a00139b7cbeff538e616a21f6b57249a9d3ed8
>> > Author:     Jamie Lentin <jm@lentin.co.uk>
>> > AuthorDate: Sat Nov 17 09:51:04 2012 +0100
>> > Committer:  Jason Cooper <jason@lakedaemon.net>
>> > CommitDate: Sat Nov 24 02:56:38 2012 +0000
>> >
>> >     power: Add simple poweroff-gpio driver
>> >
>> >     Given appropriate devicetree bindings, this driver registers a
>> >     pm_power_off function to set a GPIO line high/low to power down
>> >     your board.
>>
>> Given this seems to be dependant on device-tree, shouldn't there be
>> some 'depends on' in the kconfig to prevent this showing up on architectures
>> that don't implement it ?
>>
>> > +menuconfig POWER_RESET
>> > +	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
>
> Hi Dave
>
> Don't these depends on here do what you want?
>
>> > +	help
>> > +	  This driver supports turning off your board via a GPIO line.
>> > +	  If your board needs a GPIO high/low to power down, say Y and
>> > +	  create a binding in your devicetree.
>>
>> If not, upon seeing this, I suspect many users will ask "how do I know if I need this?"
>> given there's no mention of the sort of hardware this is useful on.
>
> Its a generic driver. I know its useful on various Marvell kirkwood
> and orion5x devices. I've also heard it useful on some Tegra boards.
>
> Are you asking i list these boards?

Even if you did, it'd rapidly go out of date I suspect. How about turning 
it on it's head?

If the device tree for your board includes a gpio-poweroff node, say 
Y. Otherwise, say N. You can find the device tree in arch/*/boot/dts/

>
>    Thanks
> 	Andrew
>

-- 
Jamie Lentin

Re: power: Add simple poweroff-gpio driver

[Dave Jones]

On Fri, Dec 14, 2012 at 09:21:50AM +0100, Andrew Lunn wrote:
 
 > > Given this seems to be dependant on device-tree, shouldn't there be
 > > some 'depends on' in the kconfig to prevent this showing up on architectures
 > > that don't implement it ? 
 > > 
 > >  > +menuconfig POWER_RESET
 > >  > +	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
 >
 > Don't these depends on here do what you want?
 
I think it needs to be on the menuconfig, rather than the child options.
I don't have OF_GPIO, but I still got asked for the former.

 
 > > If not, upon seeing this, I suspect many users will ask "how do I know if I need this?"
 > > given there's no mention of the sort of hardware this is useful on.
 > 
 > Its a generic driver. I know its useful on various Marvell kirkwood
 > and orion5x devices. I've also heard it useful on some Tegra boards.
 > 
 > Are you asking i list these boards?

No, but at least mentioning the architecture might have clued me in quicker that this
wasn't some now ACPI-ism when I saw it on x86.

	Dave

Re: power: Add simple poweroff-gpio driver

[Stephen Warren]

On 12/14/2012 08:08 AM, Dave Jones wrote:
> On Fri, Dec 14, 2012 at 09:21:50AM +0100, Andrew Lunn wrote:
>  
>  > > Given this seems to be dependant on device-tree, shouldn't there be
>  > > some 'depends on' in the kconfig to prevent this showing up on architectures
>  > > that don't implement it ? 
>  > > 
>  > >  > +menuconfig POWER_RESET
>  > >  > +	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
>  >
>  > Don't these depends on here do what you want?
>  
> I think it needs to be on the menuconfig, rather than the child options.
> I don't have OF_GPIO, but I still got asked for the former.

The menuconfig enables a class of drivers (at least theoretically in the
future, when more such drivers turn up), and there's no reason to
believe that all of those drivers will depend on OF. So in my opinion,
making POWER_RESET_GPIO depend on OF makes sense, but making POWER_RESET
depend on it doesn't.

Another solution might be to remove the menuconfig, and just include the
option for the individual driver.

Re: power: Add simple poweroff-gpio driver

[Andrew Lunn]

> > I think it needs to be on the menuconfig, rather than the child options.
> > I don't have OF_GPIO, but I still got asked for the former.
> 
> The menuconfig enables a class of drivers (at least theoretically in the
> future, when more such drivers turn up), and there's no reason to
> believe that all of those drivers will depend on OF. So in my opinion,
> making POWER_RESET_GPIO depend on OF makes sense, but making POWER_RESET
> depend on it doesn't.

I have another driver coming soon which will not depend on OF_GPIO. It
does however depend on OF. Its for a class of boards which have a
microcontroller controlling the main power supply, and sending an
character over a UART port is used to turn the board power off.

There might also be some legacy drivers which could be moved here
which don't depend on OF at all.

So i would prefer to keep POWER_RESET generic.

      Andrew

Re: power: Add simple poweroff-gpio driver

[Dave Jones]

On Fri, Dec 14, 2012 at 08:53:40PM +0100, Andrew Lunn wrote:
 > > > I think it needs to be on the menuconfig, rather than the child options.
 > > > I don't have OF_GPIO, but I still got asked for the former.
 > > 
 > > The menuconfig enables a class of drivers (at least theoretically in the
 > > future, when more such drivers turn up), and there's no reason to
 > > believe that all of those drivers will depend on OF. So in my opinion,
 > > making POWER_RESET_GPIO depend on OF makes sense, but making POWER_RESET
 > > depend on it doesn't.
 > 
 > I have another driver coming soon which will not depend on OF_GPIO. It
 > does however depend on OF. Its for a class of boards which have a
 > microcontroller controlling the main power supply, and sending an
 > character over a UART port is used to turn the board power off.
 > 
 > There might also be some legacy drivers which could be moved here
 > which don't depend on OF at all.
 > 
 > So i would prefer to keep POWER_RESET generic.

Perhaps a CONFIG_ARCH_WANT_POWER_RESET option for arches that opt-in ?

	Dave

Re: power: Add simple poweroff-gpio driver

[Andrew Lunn]

>  > Its a generic driver. I know its useful on various Marvell kirkwood
>  > and orion5x devices. I've also heard it useful on some Tegra boards.
>  > 
>  > Are you asking i list these boards?
> 

> No, but at least mentioning the architecture might have clued me in
> quicker that this wasn't some now ACPI-ism when I saw it on x86.

Hi Dave

It is architecture independent. There are examples of ARM, AVR32, &
unicore32 boards which could use this and there might be more.

	  Andrew
  

Re: power: Add simple poweroff-gpio driver

[Dave Jones]

On Fri, Dec 14, 2012 at 10:53:21PM +0100, Andrew Lunn wrote:
 > >  > Its a generic driver. I know its useful on various Marvell kirkwood
 > >  > and orion5x devices. I've also heard it useful on some Tegra boards.
 > >  > 
 > >  > Are you asking i list these boards?
 > > 
 > 
 > > No, but at least mentioning the architecture might have clued me in
 > > quicker that this wasn't some now ACPI-ism when I saw it on x86.
 > 
 > Hi Dave
 > 
 > It is architecture independent. There are examples of ARM, AVR32, &
 > unicore32 boards which could use this and there might be more.

Right, but as I alluded to in my other mail, if those archs define
ARCH_NEED_POWEROFF or similar, this won't show up on arches that don't care.

	Dave