lkml.org 
[lkml]   [2012]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v4] backlight: corgi_lcd: Use gpio_set_value_cansleep() to avoid WARN_ON
Date
On Thursday, December 06, 2012 4:22 AM, Russell King - ARM Linux wrote
> On Wed, Dec 05, 2012 at 07:20:00PM +0100, Marko Katić wrote:
> > On Wed, Dec 5, 2012 at 10:30 AM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > > On Wed, Dec 05, 2012 at 09:59:07AM +0900, Jingoo Han wrote:
> > >> - if (gpio_is_valid(lcd->gpio_backlight_cont))
> > >> - gpio_set_value(lcd->gpio_backlight_cont, cont);
> > >> + if (gpio_is_valid(lcd->gpio_backlight_cont)) {
> > >> + if (gpio_cansleep(lcd->gpio_backlight_cont))
> > >> + gpio_set_value_cansleep(lcd->gpio_backlight_cont, cont);
> > >> + else
> > >> + gpio_set_value(lcd->gpio_backlight_cont, cont);
> > >> + }
> > >
> > > Why not simply:
> > >
> > > + if (gpio_is_valid(lcd->gpio_backlight_cont))
> > > + gpio_set_value_cansleep(lcd->gpio_backlight_cont, cont);
> >
> > My first patch did exactly this but there were complains about it's
> > commit message.
>
> So that's a reason to drop the patch? Err, forgive me for being thick
> as a medieval castle wall, but what does complaints about the commit
> message have to do with the contents of the patch? Why can't you just
> fix the commit message?
>
> > And i just found out that Marek Vasut posted the exact same patch more
> > than a year ago.
> >
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2011-April/046955.html
> >
> > It was not applied for various reasons.
>
> Looking at that thread (which is corrupted btw, probably thanks to the
> crappy python based locking in mailman) - here's a better archiver:
>
> http://lists.arm.linux.org.uk/lurker/thread/20110402.014316.74101499.en.html
>
> it didn't go anywhere because the discussion was distracted by the loss
> of David Brownell.
>
> Eric shares my opinion of the _cansleep() mess, but unfortunately it's
> what we have and no one's come up with any better solutions to it. (I
> argued from the outset that the gpio_xxx_cansleep() should've been
> gpio_xxx() and the non-cansleep() version should be called
> gpio_xxx_atomic() so that by default people use the version which _can_
> sleep, but have to think about it when they want to manipulate GPIOs in
> non-task contexts.)

Hi Russell,

Thank you for your explanation. It is very helpful for getting hold of.
I have been confused by the current function name such as gpio_xxx_cansleep().
As you mentioned, gpio_xxx_cansleep()and gpio_xxx_atomic() would be better.

>
> That's off-topic though. Using just the _cansleep() version is far
> better than messing around with stuff like:
>
> if (gpio_cansleep(gpio))
> gpio_xxx_cansleep(gpio);
> else
> gpio_xxx(gpio);
>
> > > If you read the gpiolib code and documentation, what you will realise is
> > > that the two calls are identical except for the "might_sleep_if()" in
> > > gpio_set_value_cansleep(). You will also note that gpiolib itself _only_
> > > calls gpio_set_value_cansleep() without checking gpio_cansleep() in
> > > contexts where sleeping is possible. So if it's good enough for gpiolib,
> > > it should be good enough here.
> >
> > The documentation tells which calls to use when you don't need to sleep
> > and which calls to use when you might sleep. And here we have a case
> > where the same call to gpio_set_value might sleep or doesn't have to,
> > depending on the model.
> > In this case, i'd rather use gpio_cansleep check as Andrew proposed.
> >
> > I will also say that the distinction between gpio_set_value and
> > gpio_set_value_cansleep.
> > is rather confusing at this point. Is it really necessary to have both ?
>
> No. You can call gpio_set_value_cansleep() from task contexts for any
> GPIO just fine, but you can't call it from atomic contexts (it will
> complain). It doesn't matter whether the GPIO can sleep or not.
>
> You can call gpio_set_value() from any context without it complaining,
> however, gpio_set_value() can't be used with a GPIO which sleeps.
>
> Look, when it comes down to it, in _task_ context, where sleeps are
> permissible:
>
> gpio_set_value(gpio, xxx);
> and
> gpio_set_value_cansleep(gpio, xxx);
>
> are exactly the same thing; they will both set the value of a GPIO
> output, whether it be an atomic or a sleeping gpio to the requested
> value.
>
> The difference between the two becomes important if you're not in task
> context, where only the non-_cansleep() versions can be used. This is
> enforced by the _cansleep() versions issuing a WARN_ON() if they're
> used in non-task contexts. And conversely, the non-_cansleep() versions
> will warn (as you've found) if you use that call with a GPIO which will
> sleep.

The former one, the _cansleep() versions issuing a WARN_ON(), would be
better than the latter one, current scheme.

Best regards,
Jingoo Han

>
> There is another solution to this mess:
>
> 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);
> + might_sleep_if(chip->can_sleep);
> trace_gpio_value(gpio, 0, value);
> if (test_bit(FLAG_OPEN_DRAIN, &gpio_desc[gpio].flags))
> _gpio_set_open_drain_value(gpio, chip, value);
> else if (test_bit(FLAG_OPEN_SOURCE, &gpio_desc[gpio].flags))
> _gpio_set_open_source_value(gpio, chip, value);
> else
> chip->set(chip, gpio - chip->base, value);
> }
> EXPORT_SYMBOL_GPL(__gpio_set_value);
>
> With the above change (and an equivalent change everywhere else), it means
> gpio_set_value() is callable from task contexts on GPIOs which can sleep.
>
> However, it loses us some checking - we no longer have the cross-platform
> checking that we get with the existing API, and that's why it's undesirable.
>
> As I say above, IMHO it would've been much better to rename these functions
> to be the other way around but David was always very dismissive of any
> comments I had against any code he'd written.



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

\
 
 \ /
  Last update: 2012-12-10 09:41    [W:0.060 / U:0.448 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site