Attempt #3 : Changes : --------- * removed JORN.. prefixes except from functions. * removed file location in head * fixed (hope) indent issues * do not exceed 80 chars per line * added extra checks if driver allocation fails * return -1 instead of (MAX + 1) ...and probably alot more.
Patch:
* builds
* passes checkpatch.pl (only complaining about signoff-tag)
Oh, and thanks for all the feedback people! (and for doing it nicely)
Only thing not adress so far is richards mdelay question, need to do a quick
test before I can answer that.
/Kristoffer
diff --git a/drivers/video/backlight/jornada720_bllcd.c
b/drivers/video/backlight/jornada720_bllcd.c
new file mode 100644
index 0000000..0adbc49
--- /dev/null
+++ b/drivers/video/backlight/jornada720_bllcd.c
@@ -0,0 +1,287 @@
+/*
+ *
+ * Backlight and LCD Driver for HP Jornada 720
+ * Copyright (C) 2007 Kristoffer Ericson <[EMAIL PROTECTED]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * or any later version as published by the Free Software Foundation.
+ *
+ */
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/backlight.h>
+#include <linux/lcd.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/fb.h>
+#include <linux/device.h>
+#include <asm/hardware.h>
+#include <asm/arch/jornada720.h>
+#include <video/s1d13xxxfb.h>
+
+MODULE_AUTHOR("Kristoffer Ericson <[EMAIL PROTECTED]>");
+MODULE_DESCRIPTION("HP Jornada 710/720/728 Backlight/LCD Driver");
+MODULE_LICENSE("GPL");
+
+/* Contrast */
+#define LCD_MAX_CONTR 0xff
+#define LCD_DEF_CONTR 0x80
+/* Brightness */
+#define BL_MAX_BRIGHT 0xff
+#define BL_DEF_BRIGHT 0x19
+
+struct bllcd_device {
+ struct backlight_device *bl_device;
+ struct lcd_device *lcd_device;
+};
+
+/*
+ * 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 BL_MAX_BRIGHT;
+
+ jornada_ssp_start();
+ if (jornada_ssp_inout(GETBRIGHTNESS) == -ETIMEDOUT) {
+ printk(KERN_ERR "bl :get brightness timeout\n");
+ ret = -1;
+ } else
+ ret = jornada_ssp_inout(TXDUMMY);
+
+ jornada_ssp_end();
+
+ /* 0 is max brightness */
+ return BL_MAX_BRIGHT - ret;
+}
+
+static int jornada_bl_update_status(struct backlight_device *dev)
+{
+ int ret = 0;
+ int value;
+
+ jornada_ssp_start();
+
+ if (dev->props.power != FB_BLANK_UNBLANK ||
+ dev->props.fb_blank != FB_BLANK_UNBLANK) {
+ ret = jornada_ssp_inout(BRIGHTNESSOFF);
+ if (ret == -ETIMEDOUT) {
+ printk(KERN_ERR "bl : brightness off timeout\n");
+ /* backlight off */
+ PPSR &= ~PPC_LDD1;
+ PPDR |= PPC_LDD1;
+ }
+ } else { /* backlight on */
+ PPSR |= PPC_LDD1;
+ /* send setbrightness cmd */
+ ret = jornada_ssp_inout(SETBRIGHTNESS);
+ if (ret != TXDUMMY) {
+ printk(KERN_ERR "bl :set brightness timeout\n");
+ } else {
+ /* cmd accepted */
+ value = BL_MAX_BRIGHT - dev->props.brightness;
+ if (jornada_ssp_byte(value) == TXDUMMY)
+ ret = value;
+ else
+ printk(KERN_ERR "bl :set brightness failed\n");
+ }
+ }
+
+ jornada_ssp_end();
+
+ return ret;
+}
+
+/*
+ * LCD HANDLING FUNCTIONS
+ */
+static int jornada_lcd_set_contrast(struct lcd_device *pdev, int contrast)
+{
+ int ret = 0;
+
+ jornada_ssp_start();
+
+ ret = jornada_ssp_inout(SETCONTRAST);
+
+ if (ret == -ETIMEDOUT)
+ printk(KERN_ERR "lcd :set contrast timeout\n");
+ else
+ ret = jornada_ssp_byte(contrast);
+
+ jornada_ssp_end();
+
+ return ret;
+}
+
+static int jornada_lcd_set_power(struct lcd_device *pdev, int power)
+{
+ if (power != FB_BLANK_UNBLANK) {
+ /* turn off LCD */
+ PPSR &= ~PPC_LDD2;
+ PPDR |= PPC_LDD2;
+ } else {
+ /* turn on LCD */
+ PPSR |= PPC_LDD2;
+ }
+
+ return 0;
+}
+
+static int jornada_lcd_get_power(struct lcd_device *pdev)
+{
+ if (PPSR & PPC_LDD2)
+ return FB_BLANK_UNBLANK;
+ else
+ return FB_BLANK_POWERDOWN;
+}
+
+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 "lcd :get contrast timeout\n");
+ ret = -1;
+ }
+
+ jornada_ssp_end();
+
+ return ret;
+}
+
+static struct backlight_ops jornada_bl_ops = {
+ .get_brightness = jornada_bl_get_brightness,
+ .update_status = jornada_bl_update_status,
+};
+
+static struct lcd_ops jornada_lcd_ops = {
+ .get_contrast = jornada_lcd_get_contrast,
+ .set_contrast = jornada_lcd_set_contrast,
+ .get_power = jornada_lcd_get_power,
+ .set_power = jornada_lcd_set_power,
+};
+
+static int jornada_bl_probe(struct platform_device *pdev)
+{
+ struct bllcd_device *bllcd;
+
+ bllcd = kzalloc(sizeof(*bllcd), GFP_KERNEL);
+ if (bllcd == NULL)
+ return -ENOMEM;
+
+ /* bl driver - name must match fb driver name */
+ bllcd->bl_device = backlight_device_register(S1D_DEVICENAME,
+ &pdev->dev, NULL, &jornada_bl_ops);
+
+ if (IS_ERR(bllcd->bl_device)) {
+ kfree(bllcd);
+ printk(KERN_ERR "bl :failed to register device\n");
+ return -ENODEV;
+ }
+
+ /* lcd driver */
+ bllcd->lcd_device = lcd_device_register(S1D_DEVICENAME,
+ &pdev->dev, NULL, &jornada_lcd_ops);
+ if (IS_ERR(bllcd->lcd_device)) {
+ backlight_device_unregister(bllcd->bl_device);
+ kfree(bllcd);
+ printk(KERN_ERR "lcd :failed to register device\n");
+ return -ENODEV;
+ }
+
+ jornada_lcd_set_contrast(bllcd->lcd_device, LCD_DEF_CONTR);
+ jornada_lcd_set_power(bllcd->lcd_device, FB_BLANK_UNBLANK);
+
+ msleep(100);
+
+ bllcd->bl_device->props.power = FB_BLANK_UNBLANK;
+ bllcd->bl_device->props.brightness = BL_DEF_BRIGHT;
+ jornada_bl_update_status(bllcd->bl_device);
+
+ platform_set_drvdata(pdev, bllcd);
+ printk(KERN_INFO "HP Jornada 7xx Backlight/LCD driver activated\n");
+
+ return 0;
+}
+
+static int jornada_bl_remove(struct platform_device *pdev)
+{
+ struct bllcd_device *bllcd = platform_get_drvdata(pdev);
+
+ bllcd->bl_device->props.power = FB_BLANK_POWERDOWN;
+ bllcd->bl_device->props.brightness = 0;
+ bllcd->bl_device->props.max_brightness = 0;
+
+ backlight_device_unregister(bllcd->bl_device);
+ lcd_device_unregister(bllcd->lcd_device);
+
+ return 0;
+}
+
+static int jornada_bl_suspend(struct platform_device *pdev, pm_message_t state)
+{
+ struct bllcd_device *bllcd = platform_get_drvdata(pdev);
+
+ bllcd->bl_device->props.power = FB_BLANK_POWERDOWN;
+ bllcd->bl_device->props.brightness = 0;
+ bllcd->bl_device->props.max_brightness = 0;
+ jornada_bl_update_status(bllcd->bl_device);
+
+ jornada_lcd_set_power(bllcd->lcd_device, FB_BLANK_POWERDOWN);
+
+ return 0;
+}
+
+static int jornada_bl_resume(struct platform_device *pdev)
+{
+ struct bllcd_device *bllcd = platform_get_drvdata(pdev);
+
+ bllcd->bl_device->props.power = FB_BLANK_UNBLANK;
+ bllcd->bl_device->props.brightness = BL_DEF_BRIGHT;
+ bllcd->bl_device->props.max_brightness = BL_MAX_BRIGHT;
+ jornada_bl_update_status(bllcd->bl_device);
+
+ jornada_lcd_set_power(bllcd->lcd_device, FB_BLANK_UNBLANK);
+
+ return 0;
+}
+
+static struct platform_driver jornada_bl_driver = {
+ .probe = jornada_bl_probe,
+ .remove = jornada_bl_remove,
+#ifdef CONFIG_PM
+ .suspend = jornada_bl_suspend,
+ .resume = jornada_bl_resume,
+#endif
+ .driver = {
+ .name = "jornada_bllcd",
+ },
+};
+
+static int __init jornada_bl_init(void)
+{
+ return platform_driver_register(&jornada_bl_driver);
+}
+
+static void __exit jornada_bl_exit(void)
+{
+ platform_driver_unregister(&jornada_bl_driver);
+}
+
+module_init(jornada_bl_init);
+module_exit(jornada_bl_exit);
On Wed, 6 Feb 2008 16:44:36 +0000
Russell King - ARM Linux <[EMAIL PROTECTED]> wrote:
> On Wed, Feb 06, 2008 at 05:33:41PM +0100, Kristoffer Ericson wrote:
> > Oki, here is attempt #2 (btw, new mail or just keep thread?)
>
> Probably better to keep the thread. More comments 8)
>
> > +struct jornada_bllcd_device {
> > + struct backlight_device *jorn_backlight_device;
> > + struct lcd_device *jorn_lcd_device;
>
> We're inside a jornada file. Do these "jorn_" prefixes add anything
> useful that the reader doesn't already know?
>
> > +static int jornada_bl_update_status(struct backlight_device *dev)
> > +{
> > + int ret = 0;
> > +
> > + jornada_ssp_start();
> > +
> > + if (dev->props.power != FB_BLANK_UNBLANK ||
> > + dev->props.fb_blank != FB_BLANK_UNBLANK) {
> > + ret = jornada_ssp_inout(BRIGHTNESSOFF);
> > + if (ret == -ETIMEDOUT)
> > + printk(KERN_ERR "jornada720_bl: \
> > + BrightnessOff timeout\n");
> > + /* backlight off */
> > + PPSR &= ~PPC_LDD1;
> > + PPDR |= PPC_LDD1;
>
> Look at the above 5 lines. What are your expectations if ret != -ETIMEDOUT?
> If C were python you'd have one behaviour due to the indent...
>
> > +
> > + } else { /* backlight on */
>
> Too many spaces.
>
> > + PPSR |= PPC_LDD1;
> > + if ((jornada_ssp_inout(SETBRIGHTNESS)) == TXDUMMY) {
> > + /* send brightness value (0 is max, 255 lowest) */
> > + if (jornada_ssp_byte(JORNADA_BL_MAX_BRIGHTNESS -
> > + dev->props.brightness) != -ETIMEDOUT)
> > + ret = (JORNADA_BL_MAX_BRIGHTNESS -
> > + dev->props.brightness);
> > + } else
> > + printk(KERN_ERR "jornada720_bl: \
> > + SetBrightness timeout\n");
> > + }
>
> If this corresponds with the "} else {" line, it should have one tab. Plus
> the above indented code section should probably have one less indentation.
>
> > + jornada_ssp_end();
> > +
> > + return ret;
> > +}
> > +
> > +/*
> > + * LCD HANDLING FUNCTIONS
> > + */
> > +static int jornada_lcd_set_contrast(struct lcd_device *pdev, int contrast)
> > +{
> > + int ret = 0;
> > +
> > + jornada_ssp_start();
> > +
> > + ret = jornada_ssp_inout(SETCONTRAST);
> > +
> > + if (ret == -ETIMEDOUT)
> > + printk(KERN_ERR "jornada_lcd: failure to set contrast\n");
> > + else
>
> Space before 'else' not required.
>
> > + ret = jornada_ssp_byte(contrast);
> > +
> > + jornada_ssp_end();
> > +
> > + return ret;
> > +}
> > +
> > +static int jornada_lcd_set_power(struct lcd_device *pdev, int power)
> > +{
> > + if (power != FB_BLANK_UNBLANK) {
> > + /* turn off LCD */
> > + PPSR &= ~PPC_LDD2;
> > + PPDR |= PPC_LDD2;
> > + } else {
> > + /* turn on LCD */
> > + PPSR |= PPC_LDD2;
> > + }
>
> Wrongly placed }
>
> > +
> > + return 0;
> > +}
> > +
> > +static int jornada_lcd_get_power(struct lcd_device *pdev)
> > +{
> > + if (PPSR & PPC_LDD2)
> > + return FB_BLANK_UNBLANK;
> > + else
> > + return FB_BLANK_POWERDOWN;
> > +}
> > +
> > +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;
> > + }
> > +
> > + jornada_ssp_end();
> > +
> > + return ret;
> > +}
> > +
> > +static struct backlight_ops jornada_bl_ops = {
> > + .get_brightness = jornada_bl_get_brightness,
> > + .update_status = jornada_bl_update_status,
> > +};
> > +
> > +static struct lcd_ops jornada_lcd_ops = {
> > + .get_contrast = jornada_lcd_get_contrast,
> > + .set_contrast = jornada_lcd_set_contrast,
> > + .get_power = jornada_lcd_get_power,
> > + .set_power = jornada_lcd_set_power,
> > +};
> > +
> > +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);
>
> Access after free. Bad bad bad.
>
> > + }
> > +
> > + /* 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);
>
> Access after free. Bad bad bad.
>
> > + }
> > +
> > + 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);
> > +
> > + bllcd->jorn_backlight_device->props.power
> > + = FB_BLANK_UNBLANK;
> > + bllcd->jorn_backlight_device->props.brightness
> > + = JORNADA_BL_DEFAULT_BRIGHTNESS;
>
> If you get rid of the "jorn_" prefix, can these be on one line?
>
> > + jornada_bl_update_status(bllcd->jorn_backlight_device);
> > +
> > + platform_set_drvdata(pdev, bllcd);
> > + printk(KERN_INFO "HP Jornada 7xx Backlight/LCD driver activated\n");
> > +
> > + return 0;
> > +}
> > +
> > +static int jornada_bl_remove(struct platform_device *pdev)
> > +{
> > + struct jornada_bllcd_device *bllcd = platform_get_drvdata(pdev);
> > +
> > + bllcd->jorn_backlight_device->props.power = FB_BLANK_POWERDOWN;
> > + bllcd->jorn_backlight_device->props.brightness = 0;
> > + bllcd->jorn_backlight_device->props.max_brightness = 0;
> > +
> > + backlight_device_unregister(bllcd->jorn_backlight_device);
> > + lcd_device_unregister(bllcd->jorn_lcd_device);
>
> Leaves bllcd allocated - memory leak.
>
> > +
> > + return 0;
> > +}
> > +
> > +static int jornada_bl_suspend(struct platform_device *pdev, pm_message_t
> > state)
> > +{
> > + struct jornada_bllcd_device *bllcd = platform_get_drvdata(pdev);
> > +
> > + bllcd->jorn_backlight_device->props.power = FB_BLANK_POWERDOWN;
> > + bllcd->jorn_backlight_device->props.brightness = 0;
> > + bllcd->jorn_backlight_device->props.max_brightness = 0;
> > + jornada_bl_update_status(bllcd->jorn_backlight_device);
> > +
> > + jornada_lcd_set_power(bllcd->jorn_lcd_device, FB_BLANK_POWERDOWN);
> > +
> > + return 0;
> > +}
> > +
> > +static int jornada_bl_resume(struct platform_device *pdev)
> > +{
> > + struct jornada_bllcd_device *bllcd = platform_get_drvdata(pdev);
> > +
> > + bllcd->jorn_backlight_device->props.power
> > + = FB_BLANK_UNBLANK;
> > + bllcd->jorn_backlight_device->props.brightness
> > + = JORNADA_BL_DEFAULT_BRIGHTNESS;
> > + bllcd->jorn_backlight_device->props.max_brightness
> > + = JORNADA_BL_MAX_BRIGHTNESS;
> > + jornada_bl_update_status(bllcd->jorn_backlight_device);
> > +
> > + jornada_lcd_set_power(bllcd->jorn_lcd_device, FB_BLANK_UNBLANK);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver jornada_bl_driver = {
> > + .probe = jornada_bl_probe,
> > + .remove = jornada_bl_remove,
> > +#ifdef CONFIG_PM
> > + .suspend = jornada_bl_suspend,
> > + .resume = jornada_bl_resume,
> > +#endif
> > + .driver = {
> > + .name = "jornada_bllcd",
>
> a tab, not 4 spaces.
>
jornada720_bllcd.patch
Description: Binary data
