Re: [PATCH] leds/of: leds-gpio.c: Use gpio_get_value_cansleep() wheninitializing.

From: David Daney
Date: Fri Sep 09 2011 - 12:16:07 EST


On 09/08/2011 10:44 PM, Andrew Morton wrote:
On Thu, 8 Sep 2011 22:30:58 -0700 Trent Piepho<tpiepho@xxxxxxxxx> wrote:


They're very different. __Why is it OK to replace one with the other??

What's supposed to happen is chip->get() will be a method that does
"readl(GPIO_GPLR)& GPIO_GPIO(gpio);" or whatever the inlined bit in
gpio_get_value() is. So calling gpio_get_value_cansleep() should
still get the correct value for the gpio. It just won't be an inlined
register read anymore.

For instance, all the arch versions that use builtin_constant_p() will
not take the inline path, since the gpio number if obviously not a
constant when gpio_get_value() is called in this leds function. So
they inline into a call to __gpio_get_value(). Which as you've
pointed out is nearly exactly the same as gpio_get_value_cansleep().
The only change is debugging related, that of the might_sleep_if() to
a WARN_ON().

One could have:
static inline int __gpio_get_value(gpio) { return
_gpio_get_value(gpio, GFP_ATOMIC); }
static inline int gpio_get_value_cansleep(gpio) { return
_gpio_get_value(gpio, GFP_KERNEL); }

Then _gpio_get_value(gpio, context) would be the current code that's
common to both __gpio_get_value() and gpio_get_value_cansleep(),
except it uses context solely to spit a warning if the gpio can't be
done from the requested context or if the context isn't allowable from
whence the call was made.

Well, that may be the case with the current in-tree implementations (I
didn't check), but from a design point of view the core code shouldn't
"know" how the architecture is implementing gpio_get_value().

Really there are two separate issues here:

1) Should the patch be applied?

2) Is there room to improve the libgpio API?

It is unclear to me if these are currently being conflated.

In any event, from a purely selfish point of view, I would like to see the patch applied as I cannot boot my boards with out it. As for improving the GPIO APIs, it seems slightly less urgent, but also a good idea.

Thanks,
David Daney
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/