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

[Andrew Morton]

On Fri, 09 Sep 2011 09:15:58 -0700 David Daney <david.daney@xxxxxxxxxx> wrote:

> 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.

Yeah.  One option is to apply it as a stopgap and promise ourselves
that we'll fix things for real later.  I'm waiting for Grant to pop up
and tell us when he'll be fixing his junk ;)

--
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/