Messages in this thread Patch in this message |  | | | Subject | Re: [PATCH/HP7XX] - Add combined LCD / BL driver for platform HP Jornada 7xx handhelds | | From | Richard Purdie <> | | Date | Wed, 06 Feb 2008 17:25:54 +0000 |
| |
On Wed, 2008-02-06 at 17:33 +0100, Kristoffer Ericson wrote: > Oki, here is attempt #2 (btw, new mail or just keep thread?)
Apart from the issues Russell mentioned,
--- /dev/null +++ b/drivers/video/backlight/jornada720_bllcd.c @@ -0,0 +1,286 @@ +/* + * drivers/video/backlight/jornada720_bllcd.c + * We already know which file this is...
> +/* > + * BACKLIGHT HANDLING ROUTINES > + */ > +static int jornada_bl_get_brightness(struct backlight_device *dev) > +{ > + int ret; > + > + /* check if backlight is on */ > + if (!(PPSR & PPC_LDD1)) > + return JORNADA_BL_MAX_BRIGHTNESS; > + > + jornada_ssp_start(); > + if (jornada_ssp_inout(GETBRIGHTNESS) == -ETIMEDOUT) { > + printk(KERN_ERR "jornada720_bl: GetBrightness failed\n"); > + ret = JORNADA_BL_MAX_BRIGHTNESS + 1;
You've informed the backlight class your brightness runs from 0 to JORNADA_BL_MAX_BRIGHTNESS by setting max_brightness. Returning JORNADA_BL_MAX_BRIGHTNESS + 1 is invalid and confusing. -1 would probably be a better choice.
> +static int jornada_lcd_get_contrast(struct lcd_device *pdev) > +{ > + int ret; > + > + /* Don't set contrast on off powerd LCD */ > + if (jornada_lcd_get_power(pdev) != FB_BLANK_UNBLANK) > + return 0; > + > + jornada_ssp_start(); > + > + ret = jornada_ssp_inout(GETCONTRAST); > + if (ret != -ETIMEDOUT) > + ret = jornada_ssp_inout(TXDUMMY); > + else { > + printk(KERN_ERR "jornada_lcd: getcontrast failed!\n"); > + ret = JORNADA_BL_MAX_BRIGHTNESS + 1;
and again...
> +static int jornada_bl_probe(struct platform_device *pdev) > +{ > + struct jornada_bllcd_device *bllcd; > + > + bllcd = kzalloc(sizeof(*bllcd), GFP_KERNEL); > + if (bllcd == NULL) > + return -ENOMEM; > + > + /* bl driver - name must match fb driver name */ > + bllcd->jorn_backlight_device = backlight_device_register(S1D_DEVICENAME, > + &pdev->dev, NULL, &jornada_bl_ops); > + > + if (IS_ERR(bllcd->jorn_backlight_device)) { > + kfree(bllcd); > + return PTR_ERR(bllcd->jorn_backlight_device); > + } > + > + /* lcd driver */ > + bllcd->jorn_lcd_device = lcd_device_register(S1D_DEVICENAME, > + &pdev->dev, NULL, &jornada_lcd_ops); > + if (IS_ERR(bllcd->jorn_lcd_device)) { > + kfree(bllcd); > + return PTR_ERR(bllcd->jorn_lcd_device); > + }
Please go over these error paths carefully, there are several problems there...
> + jornada_lcd_set_contrast(bllcd->jorn_lcd_device, > + JORNADA_LCD_DEFAULT_CONTRAST); > + jornada_lcd_set_power(bllcd->jorn_lcd_device, > + FB_BLANK_UNBLANK); > + > + msleep(100);
Why is this msleep needed? If it is needed, how does the driver manage to suspend/resume?
Richard
|  |