backlight: gpio-backlight: Fix warning when the GPIO is on a I2C chip

PATCH Applied Tony Lindgren 4 participants 7 messages May 9, 2014 – May 19, 2014

Subsystems (2)

Framebuffer layer Backlight class/subsystem

lore

Share & tools

lore

Tony Lindgren

author

2014-05-09 01:24 UTC #0 lore raw

PATCH 1/1 backlight: gpio-backlight: Fix warning when the GPIO is on a I2C chip

If the GPIO for the backlight is on an I2C chip, we currently get nasty warnings like this during the boot:

WARNING: CPU: 0 PID: 6 at drivers/gpio/gpiolib.c:2364 gpiod_set_raw_value+0x40/0x4c()
Modules linked in:
CPU: 0 PID: 6 Comm: kworker/u2:0 Not tainted 3.15.0-rc4-12393-gcde9f4e #400
Workqueue: deferwq deferred_probe_work_func
[<c0014cbc>] (unwind_backtrace) from [<c001191c>] (show_stack+0x10/0x14)
[<c001191c>] (show_stack) from [<c0566ae0>] (dump_stack+0x80/0x9c)
[<c0566ae0>] (dump_stack) from [<c003f61c>] (warn_slowpath_common+0x68/0x8c)
[<c003f61c>] (warn_slowpath_common) from [<c003f65c>] (warn_slowpath_null+0x1c/0x24)
[<c003f65c>] (warn_slowpath_null) from [<c02f7e10>] (gpiod_set_raw_value+0x40/0x4c)
[<c02f7e10>] (gpiod_set_raw_value) from [<c0308fbc>] (gpio_backlight_update_status+0x4c/0x74)
[<c0308fbc>] (gpio_backlight_update_status) from [<c030914c>] (gpio_backlight_probe+0x168/0x254)
[<c030914c>] (gpio_backlight_probe) from [<c0378fa8>] (platform_drv_probe+0x18/0x48)
[<c0378fa8>] (platform_drv_probe) from [<c0377c40>] (driver_probe_device+0x10c/0x238)
[<c0377c40>] (driver_probe_device) from [<c0376330>] (bus_for_each_drv+0x44/0x8c)
[<c0376330>] (bus_for_each_drv) from [<c0377afc>] (device_attach+0x74/0x8c)
[<c0377afc>] (device_attach) from [<c03771c4>] (bus_probe_device+0x88/0xb0)
[<c03771c4>] (bus_probe_device) from [<c03775c8>] (deferred_probe_work_func+0x64/0x94)
[<c03775c8>] (deferred_probe_work_func) from [<c00572e8>] (process_one_work+0x1b4/0x4bc)
[<c00572e8>] (process_one_work) from [<c00579d0>] (worker_thread+0x11c/0x398)
[<c00579d0>] (worker_thread) from [<c005dfd8>] (kthread+0xc8/0xe4)
[<c005dfd8>] (kthread) from [<c000e768>] (ret_from_fork+0x14/0x2c)

Fix this by using gpio_set_value_cansleep() as suggested in drivers/gpio/gpiolib.c:2364. This is what the other backlight drivers are also doing.

Signed-off-by Tony Lindgren <[email protected]>

drivers/video/backlight/gpio_backlight.c +2 -1

@@ -38,7 +38,8 @@ static int gpio_backlight_update_status(struct backlight_device *bl)
 	    bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
 		brightness = 0;
 
-	gpio_set_value(gbl->gpio, brightness ? gbl->active : !gbl->active);
+	gpio_set_value_cansleep(gbl->gpio,
+				brightness ? gbl->active : !gbl->active);
 
 	return 0;
 }

Jingoo Han

2014-05-09 02:25 UTC #1 lore raw

Quoted 26 lines · Lee Jones

If the GPIO for the backlight is on an I2C chip, we currently get nasty warnings like this during the boot:

WARNING: CPU: 0 PID: 6 at drivers/gpio/gpiolib.c:2364 gpiod_set_raw_value+0x40/0x4c()
Modules linked in:
CPU: 0 PID: 6 Comm: kworker/u2:0 Not tainted 3.15.0-rc4-12393-gcde9f4e #400
Workqueue: deferwq deferred_probe_work_func
[<c0014cbc>] (unwind_backtrace) from [<c001191c>] (show_stack+0x10/0x14)
[<c001191c>] (show_stack) from [<c0566ae0>] (dump_stack+0x80/0x9c)
[<c0566ae0>] (dump_stack) from [<c003f61c>] (warn_slowpath_common+0x68/0x8c)
[<c003f61c>] (warn_slowpath_common) from [<c003f65c>] (warn_slowpath_null+0x1c/0x24)
[<c003f65c>] (warn_slowpath_null) from [<c02f7e10>] (gpiod_set_raw_value+0x40/0x4c)
[<c02f7e10>] (gpiod_set_raw_value) from [<c0308fbc>] (gpio_backlight_update_status+0x4c/0x74)
[<c0308fbc>] (gpio_backlight_update_status) from [<c030914c>] (gpio_backlight_probe+0x168/0x254)
[<c030914c>] (gpio_backlight_probe) from [<c0378fa8>] (platform_drv_probe+0x18/0x48)
[<c0378fa8>] (platform_drv_probe) from [<c0377c40>] (driver_probe_device+0x10c/0x238)
[<c0377c40>] (driver_probe_device) from [<c0376330>] (bus_for_each_drv+0x44/0x8c)
[<c0376330>] (bus_for_each_drv) from [<c0377afc>] (device_attach+0x74/0x8c)
[<c0377afc>] (device_attach) from [<c03771c4>] (bus_probe_device+0x88/0xb0)
[<c03771c4>] (bus_probe_device) from [<c03775c8>] (deferred_probe_work_func+0x64/0x94)
[<c03775c8>] (deferred_probe_work_func) from [<c00572e8>] (process_one_work+0x1b4/0x4bc)
[<c00572e8>] (process_one_work) from [<c00579d0>] (worker_thread+0x11c/0x398)
[<c00579d0>] (worker_thread) from [<c005dfd8>] (kthread+0xc8/0xe4)
[<c005dfd8>] (kthread) from [<c000e768>] (ret_from_fork+0x14/0x2c)

Quoted 3 lines

Fix this by using gpio_set_value_cansleep() as suggested in drivers/gpio/gpiolib.c:2364. This is what the other backlight drivers are also doing.

OK, I see.
However, gpio_backlight drive can be used by a lot of gpio drivers. In some cases, 'can_sleep' is 'false' and gpio_set_value_cansleep() is unnecessary.

In my opinion, gpio_set_value_cansleep() or gpio_set_value() can be called selectively by 'can_sleep' value.

How about the following?

- gpio_set_value(gbl->gpio, brightness ? gbl->active : !gbl->active);
+ if (gpio_cansleep(gbl->gpio))
+ gpio_set_value_cansleep(gbl->gpio,
+ brightness ? gbl->active : !gbl->active);
+ else
+ gpio_set_value(gbl->gpio, brightness ? gbl->active : !gbl->active);

Best regards,
Jingoo Han

Quoted 16 lines

Signed-off-by Tony Lindgren <[email protected]>

drivers/video/backlight/gpio_backlight.c +2 -2

@@ -38,7 +38,8 @@ static int gpio_backlight_update_status(struct backlight_device *bl)
 	    bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
 		brightness = 0;
 
-	gpio_set_value(gbl->gpio, brightness ? gbl->active : !gbl->active);
+	gpio_set_value_cansleep(gbl->gpio,
+				brightness ? gbl->active : !gbl->active);
 
 	return 0;
 }
--

Tony Lindgren

author

2014-05-09 03:09 UTC #2 lore raw

Quoted 41 lines · Jingoo Han

Quoted 26 lines · Lee Jones

If the GPIO for the backlight is on an I2C chip, we currently get nasty warnings like this during the boot:

WARNING: CPU: 0 PID: 6 at drivers/gpio/gpiolib.c:2364 gpiod_set_raw_value+0x40/0x4c()
Modules linked in:
CPU: 0 PID: 6 Comm: kworker/u2:0 Not tainted 3.15.0-rc4-12393-gcde9f4e #400
Workqueue: deferwq deferred_probe_work_func
[<c0014cbc>] (unwind_backtrace) from [<c001191c>] (show_stack+0x10/0x14)
[<c001191c>] (show_stack) from [<c0566ae0>] (dump_stack+0x80/0x9c)
[<c0566ae0>] (dump_stack) from [<c003f61c>] (warn_slowpath_common+0x68/0x8c)
[<c003f61c>] (warn_slowpath_common) from [<c003f65c>] (warn_slowpath_null+0x1c/0x24)
[<c003f65c>] (warn_slowpath_null) from [<c02f7e10>] (gpiod_set_raw_value+0x40/0x4c)
[<c02f7e10>] (gpiod_set_raw_value) from [<c0308fbc>] (gpio_backlight_update_status+0x4c/0x74)
[<c0308fbc>] (gpio_backlight_update_status) from [<c030914c>] (gpio_backlight_probe+0x168/0x254)
[<c030914c>] (gpio_backlight_probe) from [<c0378fa8>] (platform_drv_probe+0x18/0x48)
[<c0378fa8>] (platform_drv_probe) from [<c0377c40>] (driver_probe_device+0x10c/0x238)
[<c0377c40>] (driver_probe_device) from [<c0376330>] (bus_for_each_drv+0x44/0x8c)
[<c0376330>] (bus_for_each_drv) from [<c0377afc>] (device_attach+0x74/0x8c)
[<c0377afc>] (device_attach) from [<c03771c4>] (bus_probe_device+0x88/0xb0)
[<c03771c4>] (bus_probe_device) from [<c03775c8>] (deferred_probe_work_func+0x64/0x94)
[<c03775c8>] (deferred_probe_work_func) from [<c00572e8>] (process_one_work+0x1b4/0x4bc)
[<c00572e8>] (process_one_work) from [<c00579d0>] (worker_thread+0x11c/0x398)
[<c00579d0>] (worker_thread) from [<c005dfd8>] (kthread+0xc8/0xe4)
[<c005dfd8>] (kthread) from [<c000e768>] (ret_from_fork+0x14/0x2c)

Quoted 3 lines

Fix this by using gpio_set_value_cansleep() as suggested in drivers/gpio/gpiolib.c:2364. This is what the other backlight drivers are also doing.

OK, I see.
However, gpio_backlight drive can be used by a lot of gpio drivers. In some cases, 'can_sleep' is 'false' and gpio_set_value_cansleep() is unnecessary.

In my opinion, gpio_set_value_cansleep() or gpio_set_value() can be called selectively by 'can_sleep' value.

How about the following?

drivers/video/backlight/gpio_backlight.c static int gpio_backlight_update_status(struct backlight_device *bl)

Quoted 6 lines

drivers/video/backlight/gpio_backlight.c +5 -1

-       gpio_set_value(gbl->gpio, brightness ? gbl->active : !gbl->active);
+       if (gpio_cansleep(gbl->gpio))
+               gpio_set_value_cansleep(gbl->gpio,
+                                       brightness ? gbl->active : !gbl->active);
+       else
+               gpio_set_value(gbl->gpio, brightness ? gbl->active : !gbl->active);

It should be always fine to use gpio_set_value_cansleep(), see your old thread from few years ago related to another backlight driver:

https://lkml.org/lkml/2012/12/5/343

Regards,

Tony

Jingoo Han

2014-05-09 03:42 UTC #3 lore raw

Quoted 49 lines · Tony Lindgren

Quoted 41 lines · Jingoo Han

Quoted 26 lines · Lee Jones

If the GPIO for the backlight is on an I2C chip, we currently get nasty warnings like this during the boot:

WARNING: CPU: 0 PID: 6 at drivers/gpio/gpiolib.c:2364 gpiod_set_raw_value+0x40/0x4c()
Modules linked in:
CPU: 0 PID: 6 Comm: kworker/u2:0 Not tainted 3.15.0-rc4-12393-gcde9f4e #400
Workqueue: deferwq deferred_probe_work_func
[<c0014cbc>] (unwind_backtrace) from [<c001191c>] (show_stack+0x10/0x14)
[<c001191c>] (show_stack) from [<c0566ae0>] (dump_stack+0x80/0x9c)
[<c0566ae0>] (dump_stack) from [<c003f61c>] (warn_slowpath_common+0x68/0x8c)
[<c003f61c>] (warn_slowpath_common) from [<c003f65c>] (warn_slowpath_null+0x1c/0x24)
[<c003f65c>] (warn_slowpath_null) from [<c02f7e10>] (gpiod_set_raw_value+0x40/0x4c)
[<c02f7e10>] (gpiod_set_raw_value) from [<c0308fbc>] (gpio_backlight_update_status+0x4c/0x74)
[<c0308fbc>] (gpio_backlight_update_status) from [<c030914c>] (gpio_backlight_probe+0x168/0x254)
[<c030914c>] (gpio_backlight_probe) from [<c0378fa8>] (platform_drv_probe+0x18/0x48)
[<c0378fa8>] (platform_drv_probe) from [<c0377c40>] (driver_probe_device+0x10c/0x238)
[<c0377c40>] (driver_probe_device) from [<c0376330>] (bus_for_each_drv+0x44/0x8c)
[<c0376330>] (bus_for_each_drv) from [<c0377afc>] (device_attach+0x74/0x8c)
[<c0377afc>] (device_attach) from [<c03771c4>] (bus_probe_device+0x88/0xb0)
[<c03771c4>] (bus_probe_device) from [<c03775c8>] (deferred_probe_work_func+0x64/0x94)
[<c03775c8>] (deferred_probe_work_func) from [<c00572e8>] (process_one_work+0x1b4/0x4bc)
[<c00572e8>] (process_one_work) from [<c00579d0>] (worker_thread+0x11c/0x398)
[<c00579d0>] (worker_thread) from [<c005dfd8>] (kthread+0xc8/0xe4)
[<c005dfd8>] (kthread) from [<c000e768>] (ret_from_fork+0x14/0x2c)

Quoted 3 lines

Fix this by using gpio_set_value_cansleep() as suggested in drivers/gpio/gpiolib.c:2364. This is what the other backlight drivers are also doing.

OK, I see.
However, gpio_backlight drive can be used by a lot of gpio drivers. In some cases, 'can_sleep' is 'false' and gpio_set_value_cansleep() is unnecessary.

In my opinion, gpio_set_value_cansleep() or gpio_set_value() can be called selectively by 'can_sleep' value.

How about the following?

drivers/video/backlight/gpio_backlight.c static int gpio_backlight_update_status(struct backlight_device *bl)

Quoted 6 lines

drivers/video/backlight/gpio_backlight.c +5 -1

-       gpio_set_value(gbl->gpio, brightness ? gbl->active : !gbl->active);
+       if (gpio_cansleep(gbl->gpio))
+               gpio_set_value_cansleep(gbl->gpio,
+                                       brightness ? gbl->active : !gbl->active);
+       else
+               gpio_set_value(gbl->gpio, brightness ? gbl->active : !gbl->active);

Quoted 4 lines

It should be always fine to use gpio_set_value_cansleep(), see your old thread from few years ago related to another backlight driver:
https://lkml.org/lkml/2012/12/5/343

(+cc Linus Walleij, Alexandre Courbot, Russell King)

OK, I see.

gpio_set_value_cansleep() calls gpiod_set_raw_value_cansleep(), and gpio_set_value() calls gpiod_set_raw_value() as below.

./drivers/gpio/gpiolib.c
void gpiod_set_raw_value_cansleep(struct gpio_desc *desc, int value)

{
        might_sleep_if(extra_checks);
        if (!desc)
                return;
        _gpiod_set_raw_value(desc, value);
}

EXPORT_SYMBOL_GPL(gpiod_set_raw_value_cansleep);

./drivers/gpio/gpiolib.c
void gpiod_set_raw_value(struct gpio_desc *desc, int value)

{
        if (!desc)
                return;
        /* Should be using gpio_set_value_cansleep() */
        WARN_ON(desc->chip->can_sleep);
        _gpiod_set_raw_value(desc, value);
}

EXPORT_SYMBOL_GPL(gpiod_set_raw_value);

Then, the difference between gpio_set_value_cansleep() or gpio_set_value() is whether might_sleep_if(extra_checks) is called or not.

So, you said that "It should be always fine to use gpio_set_value_cansleep()", right?

Linus Walleij,
Is there any reason to keep these two functions such as gpiod_set_raw_value_cansleep() and gpiod_set_raw_value()?

Best regards,
Jingoo Han

Linus Walleij

2014-05-09 09:06 UTC #4 lore raw

Quoted 3 lines · Jingoo Han

Linus Walleij,
Is there any reason to keep these two functions such as gpiod_set_raw_value_cansleep() and gpiod_set_raw_value()?

Yes, the former can *not* be called from interrupt context, and thus erroneous usage can be detected with runtime checks.

Yours,
Linus Walleij

Jingoo Han

Acked-by on v1 1/1

2014-05-13 01:36 UTC #5 lore raw

In reply to Tony Lindgren

Quoted 26 lines · Tony Lindgren

If the GPIO for the backlight is on an I2C chip, we currently get nasty warnings like this during the boot:

WARNING: CPU: 0 PID: 6 at drivers/gpio/gpiolib.c:2364 gpiod_set_raw_value+0x40/0x4c()
Modules linked in:
CPU: 0 PID: 6 Comm: kworker/u2:0 Not tainted 3.15.0-rc4-12393-gcde9f4e #400
Workqueue: deferwq deferred_probe_work_func
[<c0014cbc>] (unwind_backtrace) from [<c001191c>] (show_stack+0x10/0x14)
[<c001191c>] (show_stack) from [<c0566ae0>] (dump_stack+0x80/0x9c)
[<c0566ae0>] (dump_stack) from [<c003f61c>] (warn_slowpath_common+0x68/0x8c)
[<c003f61c>] (warn_slowpath_common) from [<c003f65c>] (warn_slowpath_null+0x1c/0x24)
[<c003f65c>] (warn_slowpath_null) from [<c02f7e10>] (gpiod_set_raw_value+0x40/0x4c)
[<c02f7e10>] (gpiod_set_raw_value) from [<c0308fbc>] (gpio_backlight_update_status+0x4c/0x74)
[<c0308fbc>] (gpio_backlight_update_status) from [<c030914c>] (gpio_backlight_probe+0x168/0x254)
[<c030914c>] (gpio_backlight_probe) from [<c0378fa8>] (platform_drv_probe+0x18/0x48)
[<c0378fa8>] (platform_drv_probe) from [<c0377c40>] (driver_probe_device+0x10c/0x238)
[<c0377c40>] (driver_probe_device) from [<c0376330>] (bus_for_each_drv+0x44/0x8c)
[<c0376330>] (bus_for_each_drv) from [<c0377afc>] (device_attach+0x74/0x8c)
[<c0377afc>] (device_attach) from [<c03771c4>] (bus_probe_device+0x88/0xb0)
[<c03771c4>] (bus_probe_device) from [<c03775c8>] (deferred_probe_work_func+0x64/0x94)
[<c03775c8>] (deferred_probe_work_func) from [<c00572e8>] (process_one_work+0x1b4/0x4bc)
[<c00572e8>] (process_one_work) from [<c00579d0>] (worker_thread+0x11c/0x398)
[<c00579d0>] (worker_thread) from [<c005dfd8>] (kthread+0xc8/0xe4)
[<c005dfd8>] (kthread) from [<c000e768>] (ret_from_fork+0x14/0x2c)

Quoted 5 lines

Fix this by using gpio_set_value_cansleep() as suggested in drivers/gpio/gpiolib.c:2364. This is what the other backlight drivers are also doing.
Signed-off-by Tony Lindgren <[email protected]>

(+cc Linus Walleij, Alexandre Courbot, Russell King)

Hi Lee Jones,

Would you apply this patch into backlight git tree? If you have other opinions, please let us know. :-) Thank you.

Acked-by Jingoo Han <[email protected]>

Best regards,
Jingoo Han

Quoted 13 lines

drivers/video/backlight/gpio_backlight.c +2 -1

@@ -38,7 +38,8 @@ static int gpio_backlight_update_status(struct backlight_device *bl)
 	    bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
 		brightness = 0;
 
-	gpio_set_value(gbl->gpio, brightness ? gbl->active : !gbl->active);
+	gpio_set_value_cansleep(gbl->gpio,
+				brightness ? gbl->active : !gbl->active);
 
 	return 0;
 }

Lee Jones

Applied

2014-05-19 13:50 UTC #6 lore raw

In reply to Tony Lindgren

Quoted 25 lines

If the GPIO for the backlight is on an I2C chip, we currently get nasty warnings like this during the boot:

WARNING: CPU: 0 PID: 6 at drivers/gpio/gpiolib.c:2364 gpiod_set_raw_value+0x40/0x4c()
Modules linked in:
CPU: 0 PID: 6 Comm: kworker/u2:0 Not tainted 3.15.0-rc4-12393-gcde9f4e #400
Workqueue: deferwq deferred_probe_work_func
[<c0014cbc>] (unwind_backtrace) from [<c001191c>] (show_stack+0x10/0x14)
[<c001191c>] (show_stack) from [<c0566ae0>] (dump_stack+0x80/0x9c)
[<c0566ae0>] (dump_stack) from [<c003f61c>] (warn_slowpath_common+0x68/0x8c)
[<c003f61c>] (warn_slowpath_common) from [<c003f65c>] (warn_slowpath_null+0x1c/0x24)
[<c003f65c>] (warn_slowpath_null) from [<c02f7e10>] (gpiod_set_raw_value+0x40/0x4c)
[<c02f7e10>] (gpiod_set_raw_value) from [<c0308fbc>] (gpio_backlight_update_status+0x4c/0x74)
[<c0308fbc>] (gpio_backlight_update_status) from [<c030914c>] (gpio_backlight_probe+0x168/0x254)
[<c030914c>] (gpio_backlight_probe) from [<c0378fa8>] (platform_drv_probe+0x18/0x48)
[<c0378fa8>] (platform_drv_probe) from [<c0377c40>] (driver_probe_device+0x10c/0x238)
[<c0377c40>] (driver_probe_device) from [<c0376330>] (bus_for_each_drv+0x44/0x8c)
[<c0376330>] (bus_for_each_drv) from [<c0377afc>] (device_attach+0x74/0x8c)
[<c0377afc>] (device_attach) from [<c03771c4>] (bus_probe_device+0x88/0xb0)
[<c03771c4>] (bus_probe_device) from [<c03775c8>] (deferred_probe_work_func+0x64/0x94)
[<c03775c8>] (deferred_probe_work_func) from [<c00572e8>] (process_one_work+0x1b4/0x4bc)
[<c00572e8>] (process_one_work) from [<c00579d0>] (worker_thread+0x11c/0x398)
[<c00579d0>] (worker_thread) from [<c005dfd8>] (kthread+0xc8/0xe4)
[<c005dfd8>] (kthread) from [<c000e768>] (ret_from_fork+0x14/0x2c)

Quoted 5 lines

Fix this by using gpio_set_value_cansleep() as suggested in drivers/gpio/gpiolib.c:2364. This is what the other backlight drivers are also doing.
Signed-off-by Tony Lindgren <[email protected]>

Applied, thanks.

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â”‚ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog