On Tue, 4 Dec 2012 23:19:08 +0100
Marko Kati__ [off-list ref] wrote:

On Fri, Nov 30, 2012 at 2:36 AM, Andrew Morton
[off-list ref] wrote:

quoted

On Fri, 30 Nov 2012 02:06:17 +0100
dromede at gmail.com wrote:

quoted

From: Marko Katic <dromede.gmail.com>

Changing backlight intensity on an Akita (Sharp Zaurus C-1000)
will always trigger a WARN_ON:

WARNING: at drivers/gpio/gpiolib.c:1672 __gpio_set_value+0x38/0xa4()

...

Akita machines have backlight controls hooked to a gpio expander chip, max7310.
The driver that controls the max7310 sets the cansleep flag and the corgi_lcd
driver only uses plain gpio_set_value calls for changing backlight controls.
This triggers the WARN_ON on akita machines.

Akita is the only exception in this case since other users of corgi_bl access
backlight gpio controls through a different gpio expander which does not set the cansleep flag.

Let's be nice and specific, please.  You're referring to
pca953x_gpio_set_value(), yes?  And it uses i2c transfers which can
sleep?

Point taken, will change this in my v2 commit message.

quoted

quoted

Fix this by conditionally using gpio_set_value_cansleep() when changing backlight intensity on
akita machines.

I don't get this.  There is no difference between
gpio_set_value_cansleep() and __gpio_set_value() apart from the
generation of debug warnings.  What's going on here?

quoted

index c781768..f33e26f 100644

--- a/drivers/video/backlight/corgi_lcd.c
+++ b/drivers/video/backlight/corgi_lcd.c

@@ -26,7 +26,7 @@
 #include <linux/spi/corgi_lcd.h>
 #include <linux/slab.h>
 #include <asm/mach/sharpsl_param.h>
-
+#include <asm/mach-types.h>
 #define POWER_IS_ON(pwr)     ((pwr) <= FB_BLANK_NORMAL)

 /* Register Addresses */

@@ -408,11 +408,19 @@ 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 (machine_is_akita())
+                     gpio_set_value_cansleep(lcd->gpio_backlight_cont, cont);
+             else
+                     gpio_set_value(lcd->gpio_backlight_cont, cont);
+     }

See, here you could do

        if (__gpio_cansleep(lcd->gpio_backlight_cont))
                gpio_set_value_cansleep(...);
        else
                gpio_set_value(...);

and solve the problem forever, without having to hackily add
hardware-specific exceptions.

But such a change simply demonstrates the silliness of
this gpio_set_value_cansleep-vs-gpio_set_value thing.

Confused.

There are 8 different models of Zaurus devices that use
this particular lcd panel. 7 of them access it's backlight gpio controls
through the same memory mapped gpio expander chip.
And here's the akita model, the only one that uses an i2c gpio
expander that can sleep.
Also, these are legacy devices and this is an obsolete lcd panel. It is
_very_ unlikely that there will be more devices in the kernel tree
that use this panel.
I also found plenty of examples of machine_is_* usage in the drivers/ tree.
These were the reasons why i used machine_is_akita to solve this problem.

On the other hand, i do agree that yours is a more elegant solution and i will
use it in my v2 patch.

I was rather hoping that Grant would address my above observations. 
As far as I can tell, gpio_set_value_cansleep() should just be removed.