Message ID | 1352650891-18356-2-git-send-email-andrew@lunn.ch (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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.
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
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.
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
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.
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
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.
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
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.
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.
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.
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
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.
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
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.
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.
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. > >
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.
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.
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 --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");