Re: [PATCH v4] backlight: corgi_lcd: Use gpio_set_value_cansleep() to avoid WARN_ON

2012-12-05 Thread Marko Katić 
On Wed, Dec 5, 2012 at 10:30 AM, Russell King - ARM Linux
 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 ?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] backlight: corgi_lcd: Use gpio_set_value_cansleep() to avoid WARN_ON

2012-12-04 Thread Marko Katić 
On Tue, Dec 4, 2012 at 6:28 AM, Jingoo Han  wrote:
> From: Marko Katic 
>
> Changing backlight intensity on an Akita (Sharp Zaurus C-1000)
> triggers WARN_ON message:
>
> WARNING: at drivers/gpio/gpiolib.c:1672 __gpio_set_value+0x38/0xa4()
> Modules linked in:
> Backtrace:
> [] (dump_backtrace+0x0/0x110) from [] 
> (dump_stack+0x18/0x1c)
>  r6:c0158fc8 r5:0009 r4: r3:c03d4f70
> [] (dump_stack+0x0/0x1c) from [] 
> (warn_slowpath_common+0x54/0x6c)
> [] (warn_slowpath_common+0x0/0x6c) from [] 
> (warn_slowpath_null+0x24/0x2c)
>  r8:c38d5c00 r7:c03f82c0 r6: r5:00d0 r4:c384e4fc
> r3:0009
> [] (warn_slowpath_null+0x0/0x2c) from [] 
> (__gpio_set_value+0x38/0xa4)
> [] (__gpio_set_value+0x0/0xa4) from [] 
> (corgi_bl_set_intensity+0x44/0x74)
>  r7:c3933418 r6:c3933400 r5:c392cdf0 r4:002f
> [] (corgi_bl_set_intensity+0x0/0x74) from [] 
> (corgi_bl_update_status+0x5c/0x64)
>  r5:c03d31f0 r4:c3933400
> [] (corgi_bl_update_status+0x0/0x64) from [] 
> (corgi_lcd_probe+0x1a8/0x258)
>  r4:c392cdf0 r3:c0169bc0
> [] (corgi_lcd_probe+0x0/0x258) from [] 
> (spi_drv_probe+0x20/0x24)
>  r8:0052 r7:c0192d9c r6:c03da6e8 r5:c38d5c34 r4:c38d5c00
> [] (spi_drv_probe+0x0/0x24) from [] 
> (driver_probe_device+0xb0/0x208)
> [] (driver_probe_device+0x0/0x208) from [] 
> (__driver_attach+0x70/0x94)
>  r6:c03da6e8 r5:c38d5c34 r4:c38d5c00 r3:
> [] (__driver_attach+0x0/0x94) from [] 
> (bus_for_each_dev+0x54/0x90)
>  r6:c03da6e8 r5:c3827e80 r4: r3:
> [] (bus_for_each_dev+0x0/0x90) from [] 
> (driver_attach+0x20/0x28)
>  r7: r6:c03e29ec r5:c3932980 r4:c03da6e8
> [] (driver_attach+0x0/0x28) from [] 
> (bus_add_driver+0xd4/0x22c)
> [] (bus_add_driver+0x0/0x22c) from [] 
> (driver_register+0xa4/0x134)
>  r8:0052 r7:c03ea900 r6:c03c32ac r5:c03bdfc8 r4:c03da6e8
> [] (driver_register+0x0/0x134) from [] 
> (spi_register_driver+0x4c/0x60)
> [] (spi_register_driver+0x0/0x60) from [] 
> (corgi_lcd_driver_init+0x14/0x1c)
> [] (corgi_lcd_driver_init+0x0/0x1c) from [] 
> (do_one_initcall+0x9c/0x174)
> [] (do_one_initcall+0x0/0x174) from [] 
> (kernel_init+0xf4/0x2a8)
> [] (kernel_init+0x0/0x2a8) from [] 
> (ret_from_fork+0x14/0x24)
> ---[ end trace a863a63f242ee38c ]---
>
> Akita machines have backlight controls hooked to a gpio expander chip,
> max7310. In this case, pca953x_gpio_set_value() can be called to control
> gpio, and pca953x_setup_gpio() sets can_sleep flag. Therefore,
> gpio_set_value_cansleep() should be used in order to avoid WARN_ON on
> akita machines.
>
> Akita is the only exception in this case since other users of corgi_lcd
> access backlight gpio controls through a different gpio expander such as
> max7310 which does not set the can_sleep flag.

I think you forgot to delete "such as max7310". Akita has
the max7310, all the other models have a different expander.

>
> [jg1@samsung.com: used gpio_cansleep()]
> Signed-off-by: Marko Katic 
> Signed-off-by: Jingoo Han 
> ---
>  drivers/video/backlight/corgi_lcd.c |   17 +
>  1 files changed, 13 insertions(+), 4 deletions(-)
>
> 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();
> --
> 1.7.2.5
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] backlight: corgi_lcd: Fix WARN_ON() when calling corgi_bl_set_intensity.

2012-11-29 Thread Marko Katić 
On Thu, Nov 29, 2012 at 6:30 AM, Jingoo Han  wrote:
> On Thursday, November 29, 2012 6:47 AM, Marko Katic wrote
>> Subject: [PATCH] backlight: corgi_lcd: Fix WARN_ON() when calling 
>> corgi_bl_set_intensity.
>
> CC'ed Andrew Morton.
>
>
> Hi Marko Katic,
> The commit subject and commit message are not clear.
>
> How about using subject as below?
> backlight: corgi_lcd: use gpio_set_value_cansleep()
>
> In addition, 'Fix WARN_ON() when calling corgi_bl_set_intensity'
> would be the reason why you submit this patch.
> Please make the commit message more detail.
>
> Also, I have a question on gpio driver.
> In order to trigger WARN_ON(chip->can_sleep), 'can_sleep' should be
> set as 1. However, I cannot find 'can_sleep = 1' in the PXA gpio driver.
> What gpio driver do you use to test corgi_lcd driver?
>
>
> Best regards,
> Jingoo Han
>

Well, the commit message was short because i thought it was a quick
and obvious fix.
But it doesn't really matter now since you raise a valid point with
your question.
There are two classes of devices that use this lcd, corgi and spitz
(both are in mach-pxa tree). Both have
several variants. All of them use the SCOOP chip for backlight control
(arm/common/scoop.c) except akita which
uses the max7310 gpio expander for backlight control. The SCOOP chip
driver doesn't set can_sleep but the
max7310 does. So this patch is probably only valid for akita machines.
I'll test this further and post a revised patch
soon.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/