LinuxLists.cc - [PATCH 1/2] mfd: arizona: Add GPIO maintain state flag

2017-04-07 12:38:04

Subject: [PATCH 1/2] mfd: arizona: Add GPIO maintain state flag

2017-04-07 12:37:54

Subject: [PATCH 2/2] 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. The default behaviour remains the same with
the GPIO not forcing the CODEC to remain active and losing state, so
as to not cause power regressions on existing systems.

Signed-off-by: Charles Keepax <[email protected]>
---
drivers/gpio/gpio-arizona.c | 52 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)

diff --git a/drivers/gpio/gpio-arizona.c b/drivers/gpio/gpio-arizona.c
index 60b3102..027a70a 100644
--- a/drivers/gpio/gpio-arizona.c
+++ b/drivers/gpio/gpio-arizona.c
@@ -24,15 +24,27 @@
#include <linux/mfd/arizona/pdata.h>
#include <linux/mfd/arizona/registers.h>

+#define ARIZONA_GP_STATE_OUTPUT 0x00000001
+
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];
+
+ status &= (ARIZONA_GP_MAINTAIN | ARIZONA_GP_STATE_OUTPUT);
+ if (status == (ARIZONA_GP_MAINTAIN | ARIZONA_GP_STATE_OUTPUT)) {
+ arizona_gpio->status[offset] &= ~ARIZONA_GP_STATE_OUTPUT;
+
+ pm_runtime_mark_last_busy(chip->parent);
+ pm_runtime_put_autosuspend(chip->parent);
+ }

return regmap_update_bits(arizona->regmap, ARIZONA_GPIO1_CTRL + offset,
ARIZONA_GPN_DIR, ARIZONA_GPN_DIR);
@@ -85,6 +97,19 @@ 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];
+ int ret;
+
+ status &= (ARIZONA_GP_MAINTAIN | ARIZONA_GP_STATE_OUTPUT);
+ if (status == ARIZONA_GP_MAINTAIN) {
+ arizona_gpio->status[offset] |= ARIZONA_GP_STATE_OUTPUT;
+
+ 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 +130,29 @@ static void arizona_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
ARIZONA_GPN_LVL, value);
}

+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);
+ u32 offset = gpiospec->args[0];
+ u32 bits = gpiospec->args[1];
+
+ if (gpiospec->args_count < chip->of_gpio_n_cells)
+ return -EINVAL;
+
+ if (offset >= chip->ngpio)
+ return -EINVAL;
+
+ if (flags)
+ *flags = bits & ~ARIZONA_GP_MAINTAIN;
+
+ if (bits & ARIZONA_GP_MAINTAIN)
+ arizona_gpio->status[offset] |= ARIZONA_GP_MAINTAIN;
+
+ return offset;
+}
+
static const struct gpio_chip template_chip = {
.label = "arizona",
.owner = THIS_MODULE,
@@ -113,6 +161,8 @@ static const struct gpio_chip template_chip = {
.direction_output = arizona_gpio_direction_out,
.set = arizona_gpio_set,
.can_sleep = true,
+ .of_xlate = arizona_gpio_of_xlate,
+ .of_gpio_n_cells = 2,
};

static int arizona_gpio_probe(struct platform_device *pdev)
@@ -158,6 +208,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) {
--
2.1.4

2017-04-08 03:42:37

by kernel test robot

[permalink] [raw]

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

Hi Charles,

[auto build test ERROR on gpio/for-next]
[also build test ERROR on v4.11-rc5 next-20170407]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Charles-Keepax/mfd-arizona-Add-GPIO-maintain-state-flag/20170408-111119
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: x86_64-randconfig-x003-201714 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64

All error/warnings (new ones prefixed by >>):

drivers//gpio/gpio-arizona.c: In function 'arizona_gpio_direction_in':
drivers//gpio/gpio-arizona.c:44:3: error: implicit declaration of function 'pm_runtime_mark_last_busy' [-Werror=implicit-function-declaration]
pm_runtime_mark_last_busy(chip->parent);
^~~~~~~~~~~~~~~~~~~~~~~~~
drivers//gpio/gpio-arizona.c:45:3: error: implicit declaration of function 'pm_runtime_put_autosuspend' [-Werror=implicit-function-declaration]
pm_runtime_put_autosuspend(chip->parent);
^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers//gpio/gpio-arizona.c: In function 'arizona_gpio_set':
drivers//gpio/gpio-arizona.c:93:9: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
ret = pm_runtime_get_sync(chip->parent);
^~~~~~~~~~~~~~~~~~~
drivers//gpio/gpio-arizona.c:96:11: warning: 'return' with a value, in function returning void
return ret;
^~~
drivers//gpio/gpio-arizona.c:82:13: note: declared here
static void arizona_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
^~~~~~~~~~~~~~~~
In file included from include/linux/linkage.h:4:0,
from include/linux/kernel.h:6,
from drivers//gpio/gpio-arizona.c:15:
drivers//gpio/gpio-arizona.c: In function 'arizona_gpio_of_xlate':
>> drivers//gpio/gpio-arizona.c:115:33: error: 'struct gpio_chip' has no member named 'of_gpio_n_cells'
if (gpiospec->args_count < chip->of_gpio_n_cells)
^
include/linux/compiler.h:160:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> drivers//gpio/gpio-arizona.c:115:2: note: in expansion of macro 'if'
if (gpiospec->args_count < chip->of_gpio_n_cells)
^~
>> drivers//gpio/gpio-arizona.c:115:33: error: 'struct gpio_chip' has no member named 'of_gpio_n_cells'
if (gpiospec->args_count < chip->of_gpio_n_cells)
^
include/linux/compiler.h:160:42: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> drivers//gpio/gpio-arizona.c:115:2: note: in expansion of macro 'if'
if (gpiospec->args_count < chip->of_gpio_n_cells)
^~
>> drivers//gpio/gpio-arizona.c:115:33: error: 'struct gpio_chip' has no member named 'of_gpio_n_cells'
if (gpiospec->args_count < chip->of_gpio_n_cells)
^
include/linux/compiler.h:171:16: note: in definition of macro '__trace_if'
______r = !!(cond); \
^~~~
>> drivers//gpio/gpio-arizona.c:115:2: note: in expansion of macro 'if'
if (gpiospec->args_count < chip->of_gpio_n_cells)
^~
drivers//gpio/gpio-arizona.c: At top level:
>> drivers//gpio/gpio-arizona.c:138:2: error: unknown field 'of_xlate' specified in initializer
.of_xlate = arizona_gpio_of_xlate,
^
>> drivers//gpio/gpio-arizona.c:138:15: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
.of_xlate = arizona_gpio_of_xlate,
^~~~~~~~~~~~~~~~~~~~~
drivers//gpio/gpio-arizona.c:138:15: note: (near initialization for 'template_chip.read_reg')
>> drivers//gpio/gpio-arizona.c:139:2: error: unknown field 'of_gpio_n_cells' specified in initializer
.of_gpio_n_cells = 2,
^
>> drivers//gpio/gpio-arizona.c:139:21: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
.of_gpio_n_cells = 2,
^
drivers//gpio/gpio-arizona.c:139:21: note: (near initialization for 'template_chip.write_reg')
drivers//gpio/gpio-arizona.c: In function 'arizona_gpio_probe':
drivers//gpio/gpio-arizona.c:185:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
pm_runtime_enable(&pdev->dev);
^~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors

vim +115 drivers//gpio/gpio-arizona.c

9 * under the terms of the GNU General Public License as published by the
10 * Free Software Foundation; either version 2 of the License, or (at your
11 * option) any later version.
12 *
13 */
14
> 15 #include <linux/kernel.h>
16 #include <linux/slab.h>
17 #include <linux/module.h>
18 #include <linux/gpio.h>
19 #include <linux/platform_device.h>
20 #include <linux/seq_file.h>
21
22 #include <linux/mfd/arizona/core.h>
23 #include <linux/mfd/arizona/pdata.h>
24 #include <linux/mfd/arizona/registers.h>
25
26 #define ARIZONA_GP_STATE_OUTPUT 0x00000001
27
28 struct arizona_gpio {
29 struct arizona *arizona;
30 struct gpio_chip gpio_chip;
31 int status[ARIZONA_MAX_GPIO];
32 };
33
34 static int arizona_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
35 {
36 struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
37 struct arizona *arizona = arizona_gpio->arizona;
38 int status = arizona_gpio->status[offset];
39
40 status &= (ARIZONA_GP_MAINTAIN | ARIZONA_GP_STATE_OUTPUT);
41 if (status == (ARIZONA_GP_MAINTAIN | ARIZONA_GP_STATE_OUTPUT)) {
42 arizona_gpio->status[offset] &= ~ARIZONA_GP_STATE_OUTPUT;
43
44 pm_runtime_mark_last_busy(chip->parent);
45 pm_runtime_put_autosuspend(chip->parent);
46 }
47
48 return regmap_update_bits(arizona->regmap, ARIZONA_GPIO1_CTRL + offset,
49 ARIZONA_GPN_DIR, ARIZONA_GPN_DIR);
50 }
51
52 static int arizona_gpio_get(struct gpio_chip *chip, unsigned offset)
53 {
54 struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
55 struct arizona *arizona = arizona_gpio->arizona;
56 unsigned int val;
57 int ret;
58
59 ret = regmap_read(arizona->regmap, ARIZONA_GPIO1_CTRL + offset, &val);
60 if (ret < 0)
61 return ret;
62
63 if (val & ARIZONA_GPN_LVL)
64 return 1;
65 else
66 return 0;
67 }
68
69 static int arizona_gpio_direction_out(struct gpio_chip *chip,
70 unsigned offset, int value)
71 {
72 struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
73 struct arizona *arizona = arizona_gpio->arizona;
74
75 if (value)
76 value = ARIZONA_GPN_LVL;
77
78 return regmap_update_bits(arizona->regmap, ARIZONA_GPIO1_CTRL + offset,
79 ARIZONA_GPN_DIR | ARIZONA_GPN_LVL, value);
80 }
81
> 82 static void arizona_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
83 {
84 struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
85 struct arizona *arizona = arizona_gpio->arizona;
86 int status = arizona_gpio->status[offset];
87 int ret;
88
89 status &= (ARIZONA_GP_MAINTAIN | ARIZONA_GP_STATE_OUTPUT);
90 if (status == ARIZONA_GP_MAINTAIN) {
91 arizona_gpio->status[offset] |= ARIZONA_GP_STATE_OUTPUT;
92
93 ret = pm_runtime_get_sync(chip->parent);
94 if (ret < 0) {
95 dev_err(chip->parent, "Failed to resume: %d\n", ret);
96 return ret;
97 }
98 }
99
100 if (value)
101 value = ARIZONA_GPN_LVL;
102
103 regmap_update_bits(arizona->regmap, ARIZONA_GPIO1_CTRL + offset,
104 ARIZONA_GPN_LVL, value);
105 }
106
107 static int arizona_gpio_of_xlate(struct gpio_chip *chip,
108 const struct of_phandle_args *gpiospec,
109 u32 *flags)
110 {
111 struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
112 u32 offset = gpiospec->args[0];
113 u32 bits = gpiospec->args[1];
114
> 115 if (gpiospec->args_count < chip->of_gpio_n_cells)
116 return -EINVAL;
117
118 if (offset >= chip->ngpio)
119 return -EINVAL;
120
121 if (flags)
122 *flags = bits & ~ARIZONA_GP_MAINTAIN;
123
124 if (bits & ARIZONA_GP_MAINTAIN)
125 arizona_gpio->status[offset] |= ARIZONA_GP_MAINTAIN;
126
127 return offset;
128 }
129
130 static const struct gpio_chip template_chip = {
131 .label = "arizona",
132 .owner = THIS_MODULE,
133 .direction_input = arizona_gpio_direction_in,
134 .get = arizona_gpio_get,
135 .direction_output = arizona_gpio_direction_out,
136 .set = arizona_gpio_set,
137 .can_sleep = true,
> 138 .of_xlate = arizona_gpio_of_xlate,
> 139 .of_gpio_n_cells = 2,
140 };
141
142 static int arizona_gpio_probe(struct platform_device *pdev)

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (9.27 kB)
.config.gz (19.86 kB)
Download all attachments

2017-04-08 04:06:33

by kernel test robot

[permalink] [raw]

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

Hi Charles,

[auto build test ERROR on gpio/for-next]
[also build test ERROR on v4.11-rc5 next-20170407]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Charles-Keepax/mfd-arizona-Add-GPIO-maintain-state-flag/20170408-111119
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: x86_64-randconfig-x009-201714 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64

All error/warnings (new ones prefixed by >>):

drivers/gpio/gpio-arizona.c: In function 'arizona_gpio_direction_in':
>> drivers/gpio/gpio-arizona.c:44:3: error: implicit declaration of function 'pm_runtime_mark_last_busy' [-Werror=implicit-function-declaration]
pm_runtime_mark_last_busy(chip->parent);
^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/gpio/gpio-arizona.c:45:3: error: implicit declaration of function 'pm_runtime_put_autosuspend' [-Werror=implicit-function-declaration]
pm_runtime_put_autosuspend(chip->parent);
^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpio/gpio-arizona.c: In function 'arizona_gpio_set':
>> drivers/gpio/gpio-arizona.c:93:9: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
ret = pm_runtime_get_sync(chip->parent);
^~~~~~~~~~~~~~~~~~~
>> drivers/gpio/gpio-arizona.c:96:11: warning: 'return' with a value, in function returning void
return ret;
^~~
drivers/gpio/gpio-arizona.c:82:13: note: declared here
static void arizona_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
^~~~~~~~~~~~~~~~
drivers/gpio/gpio-arizona.c: In function 'arizona_gpio_probe':
>> drivers/gpio/gpio-arizona.c:185:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
pm_runtime_enable(&pdev->dev);
^~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors

vim +/pm_runtime_mark_last_busy +44 drivers/gpio/gpio-arizona.c

38 int status = arizona_gpio->status[offset];
39
40 status &= (ARIZONA_GP_MAINTAIN | ARIZONA_GP_STATE_OUTPUT);
41 if (status == (ARIZONA_GP_MAINTAIN | ARIZONA_GP_STATE_OUTPUT)) {
42 arizona_gpio->status[offset] &= ~ARIZONA_GP_STATE_OUTPUT;
43
> 44 pm_runtime_mark_last_busy(chip->parent);
> 45 pm_runtime_put_autosuspend(chip->parent);
46 }
47
48 return regmap_update_bits(arizona->regmap, ARIZONA_GPIO1_CTRL + offset,
49 ARIZONA_GPN_DIR, ARIZONA_GPN_DIR);
50 }
51
52 static int arizona_gpio_get(struct gpio_chip *chip, unsigned offset)
53 {
54 struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
55 struct arizona *arizona = arizona_gpio->arizona;
56 unsigned int val;
57 int ret;
58
59 ret = regmap_read(arizona->regmap, ARIZONA_GPIO1_CTRL + offset, &val);
60 if (ret < 0)
61 return ret;
62
63 if (val & ARIZONA_GPN_LVL)
64 return 1;
65 else
66 return 0;
67 }
68
69 static int arizona_gpio_direction_out(struct gpio_chip *chip,
70 unsigned offset, int value)
71 {
72 struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
73 struct arizona *arizona = arizona_gpio->arizona;
74
75 if (value)
76 value = ARIZONA_GPN_LVL;
77
78 return regmap_update_bits(arizona->regmap, ARIZONA_GPIO1_CTRL + offset,
79 ARIZONA_GPN_DIR | ARIZONA_GPN_LVL, value);
80 }
81
82 static void arizona_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
83 {
84 struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
85 struct arizona *arizona = arizona_gpio->arizona;
86 int status = arizona_gpio->status[offset];
87 int ret;
88
89 status &= (ARIZONA_GP_MAINTAIN | ARIZONA_GP_STATE_OUTPUT);
90 if (status == ARIZONA_GP_MAINTAIN) {
91 arizona_gpio->status[offset] |= ARIZONA_GP_STATE_OUTPUT;
92
> 93 ret = pm_runtime_get_sync(chip->parent);
94 if (ret < 0) {
95 dev_err(chip->parent, "Failed to resume: %d\n", ret);
> 96 return ret;
97 }
98 }
99
100 if (value)
101 value = ARIZONA_GPN_LVL;
102
103 regmap_update_bits(arizona->regmap, ARIZONA_GPIO1_CTRL + offset,
104 ARIZONA_GPN_LVL, value);
105 }
106
107 static int arizona_gpio_of_xlate(struct gpio_chip *chip,
108 const struct of_phandle_args *gpiospec,
109 u32 *flags)
110 {
111 struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
112 u32 offset = gpiospec->args[0];
113 u32 bits = gpiospec->args[1];
114
115 if (gpiospec->args_count < chip->of_gpio_n_cells)
116 return -EINVAL;
117
118 if (offset >= chip->ngpio)
119 return -EINVAL;
120
121 if (flags)
122 *flags = bits & ~ARIZONA_GP_MAINTAIN;
123
124 if (bits & ARIZONA_GP_MAINTAIN)
125 arizona_gpio->status[offset] |= ARIZONA_GP_MAINTAIN;
126
127 return offset;
128 }
129
130 static const struct gpio_chip template_chip = {
131 .label = "arizona",
132 .owner = THIS_MODULE,
133 .direction_input = arizona_gpio_direction_in,
134 .get = arizona_gpio_get,
135 .direction_output = arizona_gpio_direction_out,
136 .set = arizona_gpio_set,
137 .can_sleep = true,
138 .of_xlate = arizona_gpio_of_xlate,
139 .of_gpio_n_cells = 2,
140 };
141
142 static int arizona_gpio_probe(struct platform_device *pdev)
143 {
144 struct arizona *arizona = dev_get_drvdata(pdev->dev.parent);
145 struct arizona_pdata *pdata = dev_get_platdata(arizona->dev);
146 struct arizona_gpio *arizona_gpio;
147 int ret;
148
149 arizona_gpio = devm_kzalloc(&pdev->dev, sizeof(*arizona_gpio),
150 GFP_KERNEL);
151 if (!arizona_gpio)
152 return -ENOMEM;
153
154 arizona_gpio->arizona = arizona;
155 arizona_gpio->gpio_chip = template_chip;
156 arizona_gpio->gpio_chip.parent = &pdev->dev;
157 #ifdef CONFIG_OF_GPIO
158 arizona_gpio->gpio_chip.of_node = arizona->dev->of_node;
159 #endif
160
161 switch (arizona->type) {
162 case WM5102:
163 case WM5110:
164 case WM8280:
165 case WM8997:
166 case WM8998:
167 case WM1814:
168 arizona_gpio->gpio_chip.ngpio = 5;
169 break;
170 case WM1831:
171 case CS47L24:
172 arizona_gpio->gpio_chip.ngpio = 2;
173 break;
174 default:
175 dev_err(&pdev->dev, "Unknown chip variant %d\n",
176 arizona->type);
177 return -EINVAL;
178 }
179
180 if (pdata && pdata->gpio_base)
181 arizona_gpio->gpio_chip.base = pdata->gpio_base;
182 else
183 arizona_gpio->gpio_chip.base = -1;
184
> 185 pm_runtime_enable(&pdev->dev);
186
187 ret = devm_gpiochip_add_data(&pdev->dev, &arizona_gpio->gpio_chip,
188 arizona_gpio);

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (7.09 kB)
.config.gz (27.13 kB)
Download all attachments

2017-04-10 20:18:02

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH 1/2] mfd: arizona: Add GPIO maintain state flag

On Fri, Apr 07, 2017 at 01:38:44PM +0100, Charles Keepax wrote:
> 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.
>
> Add a flag through the second cell of the GPIO specifier in device
> tree, to allow the user to select whether a GPIO being configured as
> an output should keep the CODEC resumed.

If the whole codec can't be suspended, why does this need to be per
GPIO? You could just have a single boolean property.

>
> Signed-off-by: Charles Keepax <[email protected]>
> ---
> Documentation/devicetree/bindings/mfd/arizona.txt | 5 ++++-
> include/dt-bindings/mfd/arizona.h | 3 +++
> 2 files changed, 7 insertions(+), 1 deletion(-)

2017-04-11 09:34:39

by Richard Fitzgerald

[permalink] [raw]

Subject: Re: [PATCH 1/2] mfd: arizona: Add GPIO maintain state flag

On Mon, 2017-04-10 at 15:17 -0500, Rob Herring wrote:
> On Fri, Apr 07, 2017 at 01:38:44PM +0100, Charles Keepax wrote:
> > 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.
> >
> > Add a flag through the second cell of the GPIO specifier in device
> > tree, to allow the user to select whether a GPIO being configured as
> > an output should keep the CODEC resumed.
>
> If the whole codec can't be suspended, why does this need to be per
> GPIO? You could just have a single boolean property.
>

Three reasons I can think of:

1) The GPIO binding already provides for passing extra information
through the binding ("Exact meaning of each specifier cell is controller
specific" as it says in the main gpio binding doc) so why add yet
another custom property to do it?

2) Doing it through the gpio means that if ultimately the child DT node
that is using it gets disabled (status="disabled") or that driver isn't
in use the codec will be able to go to sleep. That won't happen with a
brute-force "big lock".

3) The codec only has to be kept awake while any such GPIO is actually
in use. See (2)

> >
> > Signed-off-by: Charles Keepax <[email protected]>
> > ---
> > Documentation/devicetree/bindings/mfd/arizona.txt | 5 ++++-
> > include/dt-bindings/mfd/arizona.h | 3 +++
> > 2 files changed, 7 insertions(+), 1 deletion(-)

2017-04-13 09:14:11

by Charles Keepax

[permalink] [raw]

Subject: Re: [PATCH 1/2] mfd: arizona: Add GPIO maintain state flag

On Tue, Apr 11, 2017 at 10:34:27AM +0100, Richard Fitzgerald wrote:
> On Mon, 2017-04-10 at 15:17 -0500, Rob Herring wrote:
> > On Fri, Apr 07, 2017 at 01:38:44PM +0100, Charles Keepax wrote:
> > > 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.
> > >
> > > Add a flag through the second cell of the GPIO specifier in device
> > > tree, to allow the user to select whether a GPIO being configured as
> > > an output should keep the CODEC resumed.
> >
> > If the whole codec can't be suspended, why does this need to be per
> > GPIO? You could just have a single boolean property.
> >
>
> Three reasons I can think of:
>
> 1) The GPIO binding already provides for passing extra information
> through the binding ("Exact meaning of each specifier cell is controller
> specific" as it says in the main gpio binding doc) so why add yet
> another custom property to do it?
>
> 2) Doing it through the gpio means that if ultimately the child DT node
> that is using it gets disabled (status="disabled") or that driver isn't
> in use the codec will be able to go to sleep. That won't happen with a
> brute-force "big lock".
>
> 3) The codec only has to be kept awake while any such GPIO is actually
> in use. See (2)
>

Yeah option 3 is the primary issue here, we only want to keep the
CODEC enabled whilst specific GPIOs are in use. As GPIOs can be
dynamically requested/released by things in the kernel we want to
know which GPIOs require the CODEC to be kept alive. Also in the
future one might be tempted to add maintain whilst high and
maintain whilst low options for lines with pulls on them to
further optimise power.

Thanks,
Charles

2017-04-13 12:14:34

by Linus Walleij

[permalink] [raw]

Subject: Re: [PATCH 1/2] mfd: arizona: Add GPIO maintain state flag

On Thu, Apr 13, 2017 at 11:15 AM, Charles Keepax
<[email protected]> wrote:
> On Tue, Apr 11, 2017 at 10:34:27AM +0100, Richard Fitzgerald wrote:

>> 3) The codec only has to be kept awake while any such GPIO is actually
>> in use. See (2)
>
> Yeah option 3 is the primary issue here, we only want to keep the
> CODEC enabled whilst specific GPIOs are in use. As GPIOs can be
> dynamically requested/released by things in the kernel we want to
> know which GPIOs require the CODEC to be kept alive. Also in the
> future one might be tempted to add maintain whilst high and
> maintain whilst low options for lines with pulls on them to
> further optimise power.

Why does this have to be encoded as information in the device
tree? Isn't it better to use a static table in the driver?

I don't see what use system integrators and others playing
around with the device tree has of this, it will just be confusing
to them if it is a chip-internal detail.

Yours,
Linus Walleij

2017-04-13 12:21:46

by Richard Fitzgerald

[permalink] [raw]

Subject: Re: [PATCH 1/2] mfd: arizona: Add GPIO maintain state flag

On Thu, 2017-04-13 at 14:14 +0200, Linus Walleij wrote:
> On Thu, Apr 13, 2017 at 11:15 AM, Charles Keepax
> <[email protected]> wrote:
> > On Tue, Apr 11, 2017 at 10:34:27AM +0100, Richard Fitzgerald wrote:
>
> >> 3) The codec only has to be kept awake while any such GPIO is actually
> >> in use. See (2)
> >
> > Yeah option 3 is the primary issue here, we only want to keep the
> > CODEC enabled whilst specific GPIOs are in use. As GPIOs can be
> > dynamically requested/released by things in the kernel we want to
> > know which GPIOs require the CODEC to be kept alive. Also in the
> > future one might be tempted to add maintain whilst high and
> > maintain whilst low options for lines with pulls on them to
> > further optimise power.
>
> Why does this have to be encoded as information in the device
> tree? Isn't it better to use a static table in the driver?
>
> I don't see what use system integrators and others playing
> around with the device tree has of this, it will just be confusing
> to them if it is a chip-internal detail.
>

They are GPIOs for connecting to external hardware, we don't know what
people are going to connect them to so they have to tell us how they
need them to behave.

> Yours,
> Linus Walleij

2017-04-13 12:48:51

by Linus Walleij

[permalink] [raw]

Subject: Re: [PATCH 1/2] mfd: arizona: Add GPIO maintain state flag

On Thu, Apr 13, 2017 at 2:21 PM, Richard Fitzgerald
<[email protected]> wrote:
> On Thu, 2017-04-13 at 14:14 +0200, Linus Walleij wrote:
>> On Thu, Apr 13, 2017 at 11:15 AM, Charles Keepax
>> <[email protected]> wrote:
>> > On Tue, Apr 11, 2017 at 10:34:27AM +0100, Richard Fitzgerald wrote:
>>
>> >> 3) The codec only has to be kept awake while any such GPIO is actually
>> >> in use. See (2)
>> >
>> > Yeah option 3 is the primary issue here, we only want to keep the
>> > CODEC enabled whilst specific GPIOs are in use. As GPIOs can be
>> > dynamically requested/released by things in the kernel we want to
>> > know which GPIOs require the CODEC to be kept alive. Also in the
>> > future one might be tempted to add maintain whilst high and
>> > maintain whilst low options for lines with pulls on them to
>> > further optimise power.
>>
>> Why does this have to be encoded as information in the device
>> tree? Isn't it better to use a static table in the driver?
>>
>> I don't see what use system integrators and others playing
>> around with the device tree has of this, it will just be confusing
>> to them if it is a chip-internal detail.
>>
>
> They are GPIOs for connecting to external hardware, we don't know what
> people are going to connect them to so they have to tell us how they
> need them to behave.

Aha it is a consumer configuration thing, then I see it.

I think it seems a bit odd that it is assumed that the default is that
we should *not* preserve the GPIO output value if we go to sleep.
Should the flag be inverted?

Also, why can't we just use a generic flag for this, it seems very
very generic.

Look in:
include/dt-bindings/gpio/gpio.h

Is there any reasons why we can't have:
/* Bit 3 express GPIO suspend/resume persistance in low power mode */
#define GPIO_MUST_KEEP_VALUE 0
#define GPIO_MAY_LOOSE_VALUE_DURING_SLEEP 8

Yeah it's talkative but informative. This way you can mark up lines
that are OK to loose their value during low-power/sleep using
just (new) standard bindings that can be reused by others,
also optionally making it possible for the gpiolib core to take action
on these properties if need be.

Yours,
Linus Walleij

2017-04-13 13:06:19

by Charles Keepax

[permalink] [raw]

Subject: Re: [PATCH 1/2] mfd: arizona: Add GPIO maintain state flag

On Thu, Apr 13, 2017 at 02:48:45PM +0200, Linus Walleij wrote:
> On Thu, Apr 13, 2017 at 2:21 PM, Richard Fitzgerald
> <[email protected]> wrote:
> > On Thu, 2017-04-13 at 14:14 +0200, Linus Walleij wrote:
> >> On Thu, Apr 13, 2017 at 11:15 AM, Charles Keepax
> >> <[email protected]> wrote:
> >> > On Tue, Apr 11, 2017 at 10:34:27AM +0100, Richard Fitzgerald wrote:
> >>
> >> >> 3) The codec only has to be kept awake while any such GPIO is actually
> >> >> in use. See (2)
> >> >
> >> > Yeah option 3 is the primary issue here, we only want to keep the
> >> > CODEC enabled whilst specific GPIOs are in use. As GPIOs can be
> >> > dynamically requested/released by things in the kernel we want to
> >> > know which GPIOs require the CODEC to be kept alive. Also in the
> >> > future one might be tempted to add maintain whilst high and
> >> > maintain whilst low options for lines with pulls on them to
> >> > further optimise power.
> >>
> >> Why does this have to be encoded as information in the device
> >> tree? Isn't it better to use a static table in the driver?
> >>
> >> I don't see what use system integrators and others playing
> >> around with the device tree has of this, it will just be confusing
> >> to them if it is a chip-internal detail.
> >>
> >
> > They are GPIOs for connecting to external hardware, we don't know what
> > people are going to connect them to so they have to tell us how they
> > need them to behave.
>
> Aha it is a consumer configuration thing, then I see it.
>
> I think it seems a bit odd that it is assumed that the default is that
> we should *not* preserve the GPIO output value if we go to sleep.
> Should the flag be inverted?
>

I agree that is a bit odd, my thinking was keeping the behaviour
the same for existing systems. But it only introduces a power
regression perhaps it is ok to require people to update their DT
to avoid that?

> Also, why can't we just use a generic flag for this, it seems very
> very generic.
>
> Look in:
> include/dt-bindings/gpio/gpio.h
>
> Is there any reasons why we can't have:
> /* Bit 3 express GPIO suspend/resume persistance in low power mode */
> #define GPIO_MUST_KEEP_VALUE 0
> #define GPIO_MAY_LOOSE_VALUE_DURING_SLEEP 8
>
> Yeah it's talkative but informative. This way you can mark up lines
> that are OK to loose their value during low-power/sleep using
> just (new) standard bindings that can be reused by others,
> also optionally making it possible for the gpiolib core to take action
> on these properties if need be.
>

I certainly have no objections to making this a core feature if
you are comfortable with that. I will have a look at what that
would look like.

Thanks,
Charles