| Message ID | 1354139201-12834-1-git-send-email-dromede@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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.
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();