gpio: Add new flags to control sleep status of GPIOs

PATCH Superseded Charles Keepax 8 participants 13 messages May 16, 2017 – May 23, 2017 3 messages not in the archive

Subsystems (4)

Device tree bindings GPIO Multifunction devices (MFD) Wolfson microelectronics drivers

lore

Share & tools

lore

Charles Keepax

author

2017-05-16 15:30 UTC #0 lore raw

PATCH 1/3 gpio: Add new flags to control sleep status of GPIOs

Add new flags to allow users to specify that they are not concerned with the status of GPIOs whilst in a sleep/low power state.

Signed-off-by Charles Keepax <[email protected]>

---
 include/dt-bindings/gpio/gpio.h | 4 ++++
 1 file changed, 4 insertions(+)

include/dt-bindings/gpio/gpio.h +4 -0

index b4f54da..48c28f0 100644
@@ -21,6 +21,10 @@
 #define GPIO_LINE_OPEN_SOURCE 0
 #define GPIO_LINE_OPEN_DRAIN 4
 
+/* Bit 3 express GPIO suspend/resume persistence */
+#define GPIO_SLEEP_MAINTAIN_VALUE 0
+#define GPIO_SLEEP_MAY_LOOSE_VALUE 8
+
 /*
  * Open Drain/Collector is the combination of single-ended open drain interface.
  * Open Source/Emitter is the combination of single-ended open source interface.

Signature · 1 line

2.1.4

Charles Keepax

author

2017-05-16 15:30 UTC #1 lore raw

PATCH 3/3 mfd: arizona: Update GPIO binding for newly supported flag

Update the device tree binding to show that the new GPIO_SLEEP flags are now supported in the flags field of the GPIO binding for Arizona devices.

Signed-off-by Charles Keepax <[email protected]>

---
 Documentation/devicetree/bindings/mfd/arizona.txt | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/mfd/arizona.txt +4 -1

index 8f2e282..1729133 100644
@@ -30,7 +30,10 @@ Required properties:
 
   - gpio-controller : Indicates this device is a GPIO controller.
   - #gpio-cells : Must be 2. The first cell is the pin number and the
-    second cell is used to specify optional parameters (currently unused).
+    second cell is used to specify optional parameters, the following flags
+    are supported:
+      "GPIO_SLEEP_MAY_LOOSE_OUTPUT" the output of this GPIO does not need to
+      be maintained whilst the CODEC is in low power mode.
 
   - AVDD-supply, DBVDD1-supply, CPVDD-supply : Power supplies for the device,
     as covered in Documentation/devicetree/bindings/regulator/regulator.txt

Signature · 1 line

2.1.4

A message that is not in the archive (sent off-list or not yet received): 1494948714-15203-3-git-send-email-ckeepax-yzvPICuk2AATkU/[email protected]

Lee Jones

2017-05-22 11:07 UTC #3 lore raw

Re: [PATCH 3/3] mfd: arizona: Update GPIO binding for newly supported flag

Quoted 14 lines · Charles Keepax

Update the device tree binding to show that the new GPIO_SLEEP flags are now supported in the flags field of the GPIO binding for Arizona devices.
Signed-off-by Charles Keepax <ckeepax-yzvPICuk2AATkU/[email protected]>
---
 Documentation/devicetree/bindings/mfd/arizona.txt | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/mfd/arizona.txt +0 -0
index 8f2e282..1729133 100644

Documentation/devicetree/bindings/mfd/arizona.txt Required properties: · line 36

Quoted 8 lines

Documentation/devicetree/bindings/mfd/arizona.txt +4 -1

@@ -30,7 +30,10 @@ Required properties:
 
   - gpio-controller : Indicates this device is a GPIO controller.
   - #gpio-cells : Must be 2. The first cell is the pin number and the
-    second cell is used to specify optional parameters (currently unused).
+    second cell is used to specify optional parameters, the following flags
+    are supported:
+      "GPIO_SLEEP_MAY_LOOSE_OUTPUT" the output of this GPIO does not need to
+      be maintained whilst the CODEC is in low power mode.

I guess this needs a GPIO and DT Ack.

Signature · 8 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lee Jones

2017-05-22 11:07 UTC #4 lore raw

Re: [PATCH 3/3] mfd: arizona: Update GPIO binding for newly supported flag

Quoted 17 lines · Lee Jones

Quoted 15 lines · Charles Keepax
Update the device tree binding to show that the new GPIO_SLEEP flags are now supported in the flags field of the GPIO binding for Arizona devices.
Signed-off-by Charles Keepax <[email protected]>
---
 Documentation/devicetree/bindings/mfd/arizona.txt | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/mfd/arizona.txt +0 -0
index 8f2e282..1729133 100644
@@ -30,7 +30,10 @@ Required properties:
 

Quoted 9 lines

Documentation/devicetree/bindings/mfd/arizona.txt Required properties: · line 36

Quoted 7 lines

Documentation/devicetree/bindings/mfd/arizona.txt +4 -1

   - gpio-controller : Indicates this device is a GPIO controller.
   - #gpio-cells : Must be 2. The first cell is the pin number and the
-    second cell is used to specify optional parameters (currently unused).
+    second cell is used to specify optional parameters, the following flags
+    are supported:
+      "GPIO_SLEEP_MAY_LOOSE_OUTPUT" the output of this GPIO does not need to
+      be maintained whilst the CODEC is in low power mode.

I guess this needs a GPIO and DT Ack.

FYI, if you receive both of those, you can also add mine without any further intervention from me.

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Linus Walleij

2017-05-23 08:49 UTC #5 lore raw

Re: [PATCH 3/3] mfd: arizona: Update GPIO binding for newly supported flag

In reply to a message not in the archive

Quoted 5 lines · Charles Keepax

Update the device tree binding to show that the new GPIO_SLEEP flags are now supported in the flags field of the GPIO binding for Arizona devices.
Signed-off-by Charles Keepax <ckeepax-yzvPICuk2AATkU/[email protected]>

(...)

Documentation/devicetree/bindings/mfd/arizona.txt Required properties: · line 36

Quoted 7 lines

- gpio-controller : Indicates this device is a GPIO controller.
- #gpio-cells : Must be 2. The first cell is the pin number and the
Documentation/devicetree/bindings/mfd/arizona.txt +4 -1
-    second cell is used to specify optional parameters (currently unused).
+    second cell is used to specify optional parameters, the following flags
+    are supported:
+      "GPIO_SLEEP_MAY_LOOSE_OUTPUT" the output of this GPIO does not need to
+      be maintained whilst the CODEC is in low power mode.

Your gpio controller most definately support more options than that, for example open drain.

But it is supposed to be a generic property so instead patch gpio.txt and reference that file from here.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Charles Keepax

author

2017-05-23 08:59 UTC #6 lore raw

Re: [PATCH 3/3] mfd: arizona: Update GPIO binding for newly supported flag

Quoted 17 lines · Linus Walleij

Quoted 5 lines · Charles Keepax
Update the device tree binding to show that the new GPIO_SLEEP flags are now supported in the flags field of the GPIO binding for Arizona devices.
Signed-off-by Charles Keepax <[email protected]>
(...)
Documentation/devicetree/bindings/mfd/arizona.txt Required properties: · line 36
Quoted 7 lines
- gpio-controller : Indicates this device is a GPIO controller.
- #gpio-cells : Must be 2. The first cell is the pin number and the
Documentation/devicetree/bindings/mfd/arizona.txt +4 -1
-    second cell is used to specify optional parameters (currently unused).
+    second cell is used to specify optional parameters, the following flags
+    are supported:
+      "GPIO_SLEEP_MAY_LOOSE_OUTPUT" the output of this GPIO does not need to
+      be maintained whilst the CODEC is in low power mode.

Quoted 3 lines

Your gpio controller most definately support more options than that, for example open drain.

Not at the moment, there are potentially a few more things we could support with the hardware though.

Quoted 3 lines

But it is supposed to be a generic property so instead patch gpio.txt and reference that file from here.

Yeah happy to move it over there.

Thanks,
Charles

Rob Herring

maintainer Acked-by on v1 3/3

2017-05-23 00:16 UTC #7 lore raw

Re: [PATCH 3/3] mfd: arizona: Update GPIO binding for newly supported flag

In reply to Charles Keepax

Quoted 8 lines · Charles Keepax

Update the device tree binding to show that the new GPIO_SLEEP flags are now supported in the flags field of the GPIO binding for Arizona devices.
Signed-off-by Charles Keepax <[email protected]>
---
 Documentation/devicetree/bindings/mfd/arizona.txt | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Acked-by Rob Herring <[email protected]>

A message that is not in the archive (sent off-list or not yet received): 1494948714-15203-1-git-send-email-ckeepax-yzvPICuk2AATkU/[email protected]

Charles Keepax

2017-05-16 15:31 UTC #9 lore raw

PATCH 2/3 gpio: arizona: Add support for GPIOs that need to be maintained

The Arizona devices only maintain the state of output GPIOs whilst the CODEC is active, this can cause issues if the CODEC suspends whilst something is relying on the state of one of its GPIOs. However, in many systems the CODEC GPIOs are used for audio related features and thus the state of the GPIOs is unimportant whilst the CODEC is suspended. Often keeping the CODEC resumed in such a system would incur a power impact that is unacceptable.

Allow the user to select whether a GPIO output should keep the CODEC resumed, by adding a flag through the second cell of the GPIO specifier in device tree.

Signed-off-by Charles Keepax <ckeepax-yzvPICuk2AATkU/[email protected]>

---
 drivers/gpio/gpio-arizona.c | 59 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 2 deletions(-)

drivers/gpio/gpio-arizona.c +57 -2

index cd23fd7..2d86776 100644
@@ -12,10 +12,12 @@
  *
  */
 
+#include <dt-bindings/gpio/gpio.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/gpio.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/seq_file.h>

@@ -27,15 +29,30 @@
 struct arizona_gpio {
 	struct arizona *arizona;
 	struct gpio_chip gpio_chip;
+	int status[ARIZONA_MAX_GPIO];
 };
 
 static int arizona_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
 {
 	struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
 	struct arizona *arizona = arizona_gpio->arizona;
+	int status = arizona_gpio->status[offset];
+	bool change;
+	int ret;
 
-	return regmap_update_bits(arizona->regmap, ARIZONA_GPIO1_CTRL + offset,
-				  ARIZONA_GPN_DIR, ARIZONA_GPN_DIR);
+	ret = regmap_update_bits_check(arizona->regmap,
+				       ARIZONA_GPIO1_CTRL + offset,
+				       ARIZONA_GPN_DIR, ARIZONA_GPN_DIR,
+				       &change);
+	if (ret < 0)
+		return ret;
+
+	if (change && !(status & GPIO_SLEEP_MAY_LOOSE_VALUE)) {
+		pm_runtime_mark_last_busy(chip->parent);
+		pm_runtime_put_autosuspend(chip->parent);
+	}
+
+	return 0;
 }
 
 static int arizona_gpio_get(struct gpio_chip *chip, unsigned offset)

@@ -85,6 +102,21 @@ static int arizona_gpio_direction_out(struct gpio_chip *chip,
 {
 	struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
 	struct arizona *arizona = arizona_gpio->arizona;
+	int status = arizona_gpio->status[offset];
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(arizona->regmap, ARIZONA_GPIO1_CTRL + offset, &val);
+	if (ret < 0)
+		return ret;
+
+	if ((val & ARIZONA_GPN_DIR) && !(status & GPIO_SLEEP_MAY_LOOSE_VALUE)) {
+		ret = pm_runtime_get_sync(chip->parent);
+		if (ret < 0) {
+			dev_err(chip->parent, "Failed to resume: %d\n", ret);
+			return ret;
+		}
+	}
 
 	if (value)
 		value = ARIZONA_GPN_LVL;

@@ -105,6 +137,25 @@ static void arizona_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 			   ARIZONA_GPN_LVL, value);
 }
 
+#ifdef CONFIG_OF_GPIO
+static int arizona_gpio_of_xlate(struct gpio_chip *chip,
+				 const struct of_phandle_args *gpiospec,
+				 u32 *flags)
+{
+	struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
+	int offset;
+
+	offset = of_gpio_simple_xlate(chip, gpiospec, flags);
+	if (offset < 0)
+		return offset;
+
+	if (flags)
+		arizona_gpio->status[offset] = *flags;
+
+	return offset;
+}
+#endif
+
 static const struct gpio_chip template_chip = {
 	.label			= "arizona",
 	.owner			= THIS_MODULE,

@@ -132,6 +183,8 @@ static int arizona_gpio_probe(struct platform_device *pdev)
 	arizona_gpio->gpio_chip.parent = &pdev->dev;
 #ifdef CONFIG_OF_GPIO
 	arizona_gpio->gpio_chip.of_node = arizona->dev->of_node;
+	arizona_gpio->gpio_chip.of_xlate = arizona_gpio_of_xlate;
+	arizona_gpio->gpio_chip.of_gpio_n_cells = 2;
 #endif
 
 	switch (arizona->type) {

@@ -158,6 +211,8 @@ static int arizona_gpio_probe(struct platform_device *pdev)
 	else
 		arizona_gpio->gpio_chip.base = -1;
 
+	pm_runtime_enable(&pdev->dev);
+
 	ret = devm_gpiochip_add_data(&pdev->dev, &arizona_gpio->gpio_chip,
 				     arizona_gpio);
 	if (ret < 0) {

Signature · 6 lines

2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Linus Walleij

2017-05-23 08:46 UTC #10 lore raw

Re: [PATCH 2/3] gpio: arizona: Add support for GPIOs that need to be maintained

Quoted 8 lines · Charles Keepax

The Arizona devices only maintain the state of output GPIOs whilst the CODEC is active, this can cause issues if the CODEC suspends whilst something is relying on the state of one of its GPIOs. However, in many systems the CODEC GPIOs are used for audio related features and thus the state of the GPIOs is unimportant whilst the CODEC is suspended. Often keeping the CODEC resumed in such a system would incur a power impact that is unacceptable.

Quoted 5 lines

Allow the user to select whether a GPIO output should keep the CODEC resumed, by adding a flag through the second cell of the GPIO specifier in device tree.
Signed-off-by Charles Keepax <[email protected]>

This adds the handling locally in the driver for a certain Arizona chip.

And then the next time you have to duplicate the code again and again.

That's not working, put this in the core.

Quoted 5 lines

static int arizona_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
 {
        struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
        struct arizona *arizona = arizona_gpio->arizona;
+ int status = arizona_gpio->status[offset];

Don't use a local array for this.

This should be something like gpiochip_get_sleep_persistance(chip, offset)

Quoted 1 line

+ if (change && !(status & GPIO_SLEEP_MAY_LOOSE_VALUE)) {

That us a dt-binfings header define, that is not a Linux constant.

Add FLAG_MAY_LOOSE_VALUE_DURING_SLEEP or something in drivers/gpio/gpiolib.h, store this inside the struct gpio_desc like all other flags and retrieve it from there.

Augment the code in gpiolib.c and gpiolib-of.c to parse this and store it properly in the gpio_desc and use it as described above.

Then it is reusable for others.

Quoted 8 lines

drivers/gpio/gpio-arizona.c +8 -0

+#ifdef CONFIG_OF_GPIO
+static int arizona_gpio_of_xlate(struct gpio_chip *chip,
+                                const struct of_phandle_args *gpiospec,
+                                u32 *flags)
+{
+       struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
+       int offset;
+

drivers/gpio/gpio-arizona.c static void arizona_gpio_set(struct gpio_chip *chip, unsigned offset, int value) · line 157

Quoted 10 lines

drivers/gpio/gpio-arizona.c +10 -0

+       offset = of_gpio_simple_xlate(chip, gpiospec, flags);
+       if (offset < 0)
+               return offset;
+
+       if (flags)
+               arizona_gpio->status[offset] = *flags;
+
+       return offset;
+}
+#endif

No need for this hackery if you move the parsing to the core, so do that.

Yours,
Linus Walleij

Charles Keepax

author

2017-05-23 08:55 UTC #11 lore raw

Re: [PATCH 2/3] gpio: arizona: Add support for GPIOs that need to be maintained

Quoted 37 lines · Linus Walleij

Quoted 8 lines · Charles Keepax
The Arizona devices only maintain the state of output GPIOs whilst the CODEC is active, this can cause issues if the CODEC suspends whilst something is relying on the state of one of its GPIOs. However, in many systems the CODEC GPIOs are used for audio related features and thus the state of the GPIOs is unimportant whilst the CODEC is suspended. Often keeping the CODEC resumed in such a system would incur a power impact that is unacceptable.
Quoted 5 lines
Allow the user to select whether a GPIO output should keep the CODEC resumed, by adding a flag through the second cell of the GPIO specifier in device tree.
Signed-off-by Charles Keepax <[email protected]>
This adds the handling locally in the driver for a certain Arizona chip.
And then the next time you have to duplicate the code again and again.
That's not working, put this in the core.
Quoted 5 lines
static int arizona_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
 {
        struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
        struct arizona *arizona = arizona_gpio->arizona;
+ int status = arizona_gpio->status[offset];
Don't use a local array for this.
This should be something like gpiochip_get_sleep_persistance(chip, offset)
Quoted 1 line
+ if (change && !(status & GPIO_SLEEP_MAY_LOOSE_VALUE)) {
That us a dt-binfings header define, that is not a Linux constant.

Quoted 9 lines

Add FLAG_MAY_LOOSE_VALUE_DURING_SLEEP or something in drivers/gpio/gpiolib.h, store this inside the struct gpio_desc like all other flags and retrieve it from there.
Augment the code in gpiolib.c and gpiolib-of.c to parse this and store it properly in the gpio_desc and use it as described above.
Then it is reusable for others.

Apologies hadn't realised you wanted me to put so much of the functionality into the core, will respin again.

Thanks,
Charles

Linus Walleij

2017-05-23 08:39 UTC #12 lore raw

In reply to a message not in the archive

Quoted 4 lines · Charles Keepax

Add new flags to allow users to specify that they are not concerned with the status of GPIOs whilst in a sleep/low power state.
Signed-off-by Charles Keepax <ckeepax-yzvPICuk2AATkU/[email protected]>

Would be nice to have some opinion from the DT maintainers about this but seems all right to me.

include/dt-bindings/gpio/gpio.h line 30

Quoted 12 lines

+++ b/include/dt-bindings/gpio/gpio.h

include/dt-bindings/gpio/gpio.h +4 -0

@@ -21,6 +21,10 @@
 #define GPIO_LINE_OPEN_SOURCE 0
 #define GPIO_LINE_OPEN_DRAIN 4
 
+/* Bit 3 express GPIO suspend/resume persistence */
+#define GPIO_SLEEP_MAINTAIN_VALUE 0
+#define GPIO_SLEEP_MAY_LOOSE_VALUE 8
+
 /*
  * Open Drain/Collector is the combination of single-ended open drain interface.
  * Open Source/Emitter is the combination of single-ended open source interface.

Please move the define below the comment and macros below it, that is using the defines right above...

A message that is not in the archive (sent off-list or not yet received): CACRpkdbiyZ9jPnuaX9OuYjVbrUYxtpqO-XMqb75ScCEH-=kOEg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Lee Jones

2017-05-23 08:51 UTC #14 lore raw

Quoted 10 lines · Linus Walleij

Quoted 4 lines · Charles Keepax
Add new flags to allow users to specify that they are not concerned with the status of GPIOs whilst in a sleep/low power state.
Signed-off-by Charles Keepax <ckeepax-yzvPICuk2AATkU/[email protected]>
Would be nice to have some opinion from the DT maintainers about this but seems all right to me.

Not sure if it makes a difference, but Rob Acked the bindings already.

Quoted 18 lines

include/dt-bindings/gpio/gpio.h line 30
Quoted 12 lines
+++ b/include/dt-bindings/gpio/gpio.h
include/dt-bindings/gpio/gpio.h +4 -0
@@ -21,6 +21,10 @@
 #define GPIO_LINE_OPEN_SOURCE 0
 #define GPIO_LINE_OPEN_DRAIN 4
 
+/* Bit 3 express GPIO suspend/resume persistence */
+#define GPIO_SLEEP_MAINTAIN_VALUE 0
+#define GPIO_SLEEP_MAY_LOOSE_VALUE 8
+
 /*
  * Open Drain/Collector is the combination of single-ended open drain interface.
  * Open Source/Emitter is the combination of single-ended open source interface.
Please move the define below the comment and macros below it, that is using the defines right above...
Yours,
Linus Walleij

Signature · 8 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rob Herring

Acked-by

2017-05-23 13:38 UTC #15 lore raw

Quoted 8 lines · Lee Jones

Quoted 6 lines · Linus Walleij
Quoted 3 lines · Charles Keepax
Add new flags to allow users to specify that they are not concerned with the status of GPIOs whilst in a sleep/low power state.

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
Signed-off-by Charles Keepax <ckeepax-yzvPICuk2AATkU/[email protected]>
Would be nice to have some opinion from the DT maintainers about this but seems all right to me.
Not sure if it makes a difference, but Rob Acked the bindings already.

Seems PW doesn't pick-up header only patches, so I missed this.

Acked-by Rob Herring <[email protected]>

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html