| Message ID | 000201cdd283$baf50160$30df0420$%han@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); 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.
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. 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. > > 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 ?
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.) 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. 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.
On Wed, Dec 5, 2012 at 7:21 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > 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. FWIW, I'll gladly take a patch to rename them now if someone does the legwork. g.
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.
On Monday, December 10, 2012 5:18 PM, Jingoo Han wrote > 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: ... > > 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. Oh, sorry. There is a mistake. It should be as below: 'gpio_xxx()and gpio_xxx_atomic() would be better'. Best regards, Jingoo Han
diff --git a/drivers/video/backlight/corgi_lcd.c b/drivers/video/backlight/corgi_lcd.c index e2e1b62..e5168f4 100644 --- a/drivers/video/backlight/corgi_lcd.c +++ b/drivers/video/backlight/corgi_lcd.c @@ -408,11 +408,20 @@ static int corgi_bl_set_intensity(struct corgi_lcd *lcd, int intensity) /* Bit 5 via GPIO_BACKLIGHT_CONT */ cont = !!(intensity & 0x20) ^ lcd->gpio_backlight_cont_inverted; - 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); + } - if (gpio_is_valid(lcd->gpio_backlight_on)) - gpio_set_value(lcd->gpio_backlight_on, intensity); + if (gpio_is_valid(lcd->gpio_backlight_on)) { + if (gpio_cansleep(lcd->gpio_backlight_on)) + gpio_set_value_cansleep(lcd->gpio_backlight_on, + intensity); + else + gpio_set_value(lcd->gpio_backlight_on, intensity); + } if (lcd->kick_battery) lcd->kick_battery();