diff mbox

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

Message ID 1352650891-18356-2-git-send-email-andrew@lunn.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Lunn Nov. 11, 2012, 4:21 p.m. UTC
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

Comments

Stephen Warren Nov. 11, 2012, 10:03 p.m. UTC | #1
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.
Linus Walleij Nov. 12, 2012, 1 a.m. UTC | #2
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
Anton Vorontsov Nov. 12, 2012, 1:12 a.m. UTC | #3
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.
Andrew Lunn Nov. 12, 2012, 6:07 a.m. UTC | #4
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
Anton Vorontsov Nov. 12, 2012, 6:53 a.m. UTC | #5
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.
Andrew Lunn Nov. 12, 2012, 8:25 a.m. UTC | #6
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
Stephen Warren Nov. 12, 2012, 4:17 p.m. UTC | #7
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.
Andrew Lunn Nov. 12, 2012, 6:19 p.m. UTC | #8
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
Anton Vorontsov Nov. 12, 2012, 6:43 p.m. UTC | #9
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.
Stephen Warren Nov. 12, 2012, 6:58 p.m. UTC | #10
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.
Anton Vorontsov Nov. 12, 2012, 7:17 p.m. UTC | #11
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.
Linus Walleij Nov. 15, 2012, 10:35 a.m. UTC | #12
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
Anton Vorontsov Nov. 15, 2012, 10:59 a.m. UTC | #13
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.
Linus Walleij Nov. 15, 2012, 11:10 a.m. UTC | #14
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
Stephen Warren Nov. 15, 2012, 6 p.m. UTC | #15
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.
Grant Likely Nov. 15, 2012, 6:05 p.m. UTC | #16
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.
Jamie Lentin Nov. 15, 2012, 6:11 p.m. UTC | #17
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.
>
>
Grant Likely Nov. 15, 2012, 6:21 p.m. UTC | #18
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.
Grant Likely Nov. 21, 2012, 1:17 p.m. UTC | #19
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 Nov. 21, 2012, 1:20 p.m. UTC | #20
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.
diff mbox

Patch

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");