[PATCH] gpiolib: Add comments explaining the _cansleep() WARN_ON()s

[PATCH] gpiolib: Add comments explaining the _cansleep() WARN_ON()s

[Mark Brown]

I've seen users getting very confused by the WARN_ON()s for can_sleep
GPIOs in the atomic-safe paths, the discoverability of the non-atomic
version of the API seems to be hampered by the fact that it's defined
in a header file not the .c file where the warnings are.

Add a couple of comments next to the warnings to help people on their
way.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/gpio/gpiolib.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 17fdf4b..2513af6 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1560,6 +1560,7 @@ int __gpio_get_value(unsigned gpio)
 	int value;
 
 	chip = gpio_to_chip(gpio);
+	/* Should be using gpio_get_value_cansleep() or a different GPIO */
 	WARN_ON(chip->can_sleep);
 	value = chip->get ? chip->get(chip, gpio - chip->base) : 0;
 	trace_gpio_value(gpio, 1, value);
@@ -1581,6 +1582,7 @@ void __gpio_set_value(unsigned gpio, int value)
 	struct gpio_chip	*chip;
 
 	chip = gpio_to_chip(gpio);
+	/* Should be using gpio_set_value_cansleep() or a different GPIO */
 	WARN_ON(chip->can_sleep);
 	trace_gpio_value(gpio, 0, value);
 	chip->set(chip, gpio - chip->base, value);
-- 
1.7.9.rc1

Re: [PATCH] gpiolib: Add comments explaining the _cansleep() WARN_ON()s

[Linus Walleij]

On Fri, Feb 17, 2012 at 7:24 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

> I've seen users getting very confused by the WARN_ON()s for can_sleep
> GPIOs in the atomic-safe paths, the discoverability of the non-atomic
> version of the API seems to be hampered by the fact that it's defined
> in a header file not the .c file where the warnings are.
>
> Add a couple of comments next to the warnings to help people on their
> way.

Good!

> +       /* Should be using gpio_get_value_cansleep() or a different GPIO */
>        WARN_ON(chip->can_sleep);

Actually I cannot parse the "or a different GPIO" part.

If this means that the user may be addressing the wrong GPIO pin,
then we could spell that out I presume, but maybe you are referring
to something else?

Yours,
Linus Walleij

Re: [PATCH] gpiolib: Add comments explaining the _cansleep() WARN_ON()s

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 668 bytes --]

On Fri, Feb 17, 2012 at 07:41:33PM +0100, Linus Walleij wrote:
> On Fri, Feb 17, 2012 at 7:24 PM, Mark Brown

> > +       /* Should be using gpio_get_value_cansleep() or a different GPIO */
> >        WARN_ON(chip->can_sleep);

> Actually I cannot parse the "or a different GPIO" part.

> If this means that the user may be addressing the wrong GPIO pin,
> then we could spell that out I presume, but maybe you are referring
> to something else?

It was intended as a hint that if you really need to be in atomic
context to do whatever this isn't the GPIO for you but in retrospect
it's probably not actually useful advice for most users.  I'll respin.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] gpiolib: Add comments explaining the _cansleep() WARN_ON()s

[Mark Brown]

I've seen users getting very confused by the WARN_ON()s for can_sleep
GPIOs in the atomic-safe paths, the discoverability of the non-atomic
version of the API seems to be hampered by the fact that it's defined
in a header file not the .c file where the warnings are.

Add a couple of comments next to the warnings to help people on their
way.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/gpio/gpiolib.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 17fdf4b..10ce33d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1560,6 +1560,7 @@ int __gpio_get_value(unsigned gpio)
 	int value;
 
 	chip = gpio_to_chip(gpio);
+	/* Should be using gpio_get_value_cansleep() */
 	WARN_ON(chip->can_sleep);
 	value = chip->get ? chip->get(chip, gpio - chip->base) : 0;
 	trace_gpio_value(gpio, 1, value);
@@ -1581,6 +1582,7 @@ void __gpio_set_value(unsigned gpio, int value)
 	struct gpio_chip	*chip;
 
 	chip = gpio_to_chip(gpio);
+	/* Should be using gpio_set_value_cansleep() */
 	WARN_ON(chip->can_sleep);
 	trace_gpio_value(gpio, 0, value);
 	chip->set(chip, gpio - chip->base, value);
-- 
1.7.9.rc1

Re: [PATCH] gpiolib: Add comments explaining the _cansleep() WARN_ON()s

[Linus Walleij]

On Fri, Feb 17, 2012 at 7:46 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

> I've seen users getting very confused by the WARN_ON()s for can_sleep
> GPIOs in the atomic-safe paths, the discoverability of the non-atomic
> version of the API seems to be hampered by the fact that it's defined
> in a header file not the .c file where the warnings are.
>
> Add a couple of comments next to the warnings to help people on their
> way.
>
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks,
Linus Walleij