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

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

[Jingoo Han]

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

> 
> From: Marko Katic <dromede.gmail.com>
> 
> 
> Signed-off-by: Marko Katic <dromede@gmail.com>
> ---
>  drivers/video/backlight/corgi_lcd.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/backlight/corgi_lcd.c b/drivers/video/backlight/corgi_lcd.c
> index c781768..8b002d7 100644
> --- a/drivers/video/backlight/corgi_lcd.c
> +++ b/drivers/video/backlight/corgi_lcd.c
> @@ -409,10 +409,10 @@ static int corgi_bl_set_intensity(struct corgi_lcd *lcd, int intensity)
>  	cont = !!(intensity & 0x20) ^ lcd->gpio_backlight_cont_inverted;
> 
>  	if (gpio_is_valid(lcd->gpio_backlight_cont))
> -		gpio_set_value(lcd->gpio_backlight_cont, cont);
> +		gpio_set_value_cansleep(lcd->gpio_backlight_cont, cont);
> 
>  	if (gpio_is_valid(lcd->gpio_backlight_on))
> -		gpio_set_value(lcd->gpio_backlight_on, intensity);
> +		gpio_set_value_cansleep(lcd->gpio_backlight_on, intensity);
> 
>  	if (lcd->kick_battery)
>  		lcd->kick_battery();
> --
> 1.7.4.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Marko Katić]

On Thu, Nov 29, 2012 at 6:30 AM, Jingoo Han <jg1.han@samsung.com> 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.

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

[Russell King - ARM Linux]

On Thu, Nov 29, 2012 at 02:09:51PM +0100, Marko Katić wrote:
> Well, the commit message was short because i thought it was a quick
> and obvious fix.

Don't always expect the person who ends up applying your patch to know
what your patch is doing.  Don't expect people who are looking back
through the git history to look at the patch to work out whether your
commit is relevant to them.  I suspect Andrew Morton doesn't know what
a "SCOOP" or "AKITA" is...

Commit messages are there not only to describe the change, but also say
why the change is necessary.  Think about it as your chance to "sell"
the patch.

One good step would be to include the warning message dump from the
kernel (your subject line says you're hitting a WARN_ON so include
that.)  At least then people can see your starting point for the patch.