Greetings,
Richard I've cleaned up the driver by checking with checkpatch.pl as Russell suggested. I would appreciate it if you could
look through the patch again and give comments since the patch looks somewhat different.
I can add patch for Kconfig/Makefile later on but suspect there will be some changes required before that.
Oh and to answer your question regarding MAX/MIN values of the backlight, 0 = max and 255 = min. So we simply turn it around
by returning 255 - value. Same thing when we need to set value.
Best wishes
Kristoffer Ericson
diff --git a/drivers/video/backlight/jornada720_bllcd.c b/drivers/video/backlight/jornada720_bllcd.c
new file mode 100644
index 0000000..4967b86
--- /dev/null
+++ b/drivers/video/backlight/jornada720_bllcd.c
@@ -0,0 +1,284 @@
+/*
+ * drivers/video/backlight/jornada720_bllcd.c
+ *
+ * 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");
+
+#define JORNADA_LCD_MAX_CONTRAST 0xff
+#define JORNADA_LCD_DEFAULT_CONTRAST 0x80
+#define JORNADA_BL_MAX_BRIGHTNESS 0xff
+#define JORNADA_BL_DEFAULT_BRIGHTNESS 0x19
+
+struct jornada_bllcd_device {
+ struct backlight_device *jorn_backlight_device;
+ struct lcd_device *jorn_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 255;
+
+ jornada_ssp_start();
+ if (jornada_ssp_inout(GETBRIGHTNESS) == -ETIMEDOUT) {
+ printk(KERN_ERR "jornada720_bl: GetBrightness failed\n");
+ ret = 256;
+ } else
+ ret = jornada_ssp_inout(TXDUMMY);
+
+ jornada_ssp_end();
+
+ /* 0 is max brightness */
+ return (255 - ret);
+}
+
+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");
+ else {
+ /* backlight off */
+ PPSR &= ~PPC_LDD1;
+ PPDR |= PPC_LDD1;
+ }
+ } else {/* backlight on */
+ PPSR |= PPC_LDD1;
+
+ if ((jornada_ssp_inout(SETBRIGHTNESS)) == TXDUMMY) {
+ /* send brightness value (0 is max, 255 lowest) */
+ if (jornada_ssp_byte(255 - dev->props.brightness) != -ETIMEDOUT)
+ ret = (255 - dev->props.brightness);
+ } else
+ printk(KERN_ERR "jornada720_bl: SetBrightness timeout\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_INFO "jornada_lcd: failure to set contrast\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_INFO "jornada_lcd: getcontrast failed!\n");
+ ret = 256;
+ }
+
+ 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);
+ }
+
+ /* 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);
+ }
+
+ 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;
+ 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);
+
+ 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_bl",
+ },
+};
+
+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);
Kristoffer Ericson wrote:
> Greetings,
>
> Richard I've cleaned up the driver by checking with checkpatch.pl as Russell suggested. I would appreciate it if you could
> look through the patch again and give comments since the patch looks somewhat different.
>
> I can add patch for Kconfig/Makefile later on but suspect there will be some changes required before that.
>
> Oh and to answer your question regarding MAX/MIN values of the backlight, 0 = max and 255 = min. So we simply turn it around
> by returning 255 - value. Same thing when we need to set value.
>
> Best wishes
> Kristoffer Ericson
>
> diff --git a/drivers/video/backlight/jornada720_bllcd.c b/drivers/video/backlight/jornada720_bllcd.c
> new file mode 100644
> index 0000000..4967b86
> --- /dev/null
> +++ b/drivers/video/backlight/jornada720_bllcd.c
> @@ -0,0 +1,284 @@
> +/*
> + * drivers/video/backlight/jornada720_bllcd.c
> + *
> + * 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");
> +
> +#define JORNADA_LCD_MAX_CONTRAST 0xff
> +#define JORNADA_LCD_DEFAULT_CONTRAST 0x80
> +#define JORNADA_BL_MAX_BRIGHTNESS 0xff
> +#define JORNADA_BL_DEFAULT_BRIGHTNESS 0x19
> +
> +struct jornada_bllcd_device {
> + struct backlight_device *jorn_backlight_device;
> + struct lcd_device *jorn_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 255;
return JORNADA_BL_MAX_BRIGHTNESS;
> +
> + jornada_ssp_start();
> + if (jornada_ssp_inout(GETBRIGHTNESS) == -ETIMEDOUT) {
> + printk(KERN_ERR "jornada720_bl: GetBrightness failed\n");
> + ret = 256;
ret = JORNADA_BL_MAX_BRIGHTNESS + 1;
> + } else
> + ret = jornada_ssp_inout(TXDUMMY);
> +
> + jornada_ssp_end();
> +
> + /* 0 is max brightness */
> + return (255 - ret);
return (JORNADA_BL_MAX_BRIGHTNESS - ret);
> +}
> +
> +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");
> + else {
> + /* backlight off */
> + PPSR &= ~PPC_LDD1;
> + PPDR |= PPC_LDD1;
> + }
> + } else {/* backlight on */
> + PPSR |= PPC_LDD1;
missing curly bracket
> +
> + if ((jornada_ssp_inout(SETBRIGHTNESS)) == TXDUMMY) {
> + /* send brightness value (0 is max, 255 lowest) */
> + if (jornada_ssp_byte(255 - dev->props.brightness) != -ETIMEDOUT)
> + ret = (255 - dev->props.brightness);
similarly JORNADA_BL_MAX_BRIGHTNESS in the three lines above
> + } else
> + printk(KERN_ERR "jornada720_bl: SetBrightness timeout\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_INFO "jornada_lcd: failure to set contrast\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;
wrong indent
> +
> + 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_INFO "jornada_lcd: getcontrast failed!\n");
> + ret = 256;
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);
> + }
> +
> + /* 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);
> + }
> +
> + 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;
> + 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);
> +
> + 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_bl",
> + },
> +};
> +
> +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);
Roel Kluin wrote:
> Kristoffer Ericson wrote:
>> Greetings,
>>
>> Richard I've cleaned up the driver by checking with checkpatch.pl as Russell suggested. I would appreciate it if you could
>> look through the patch again and give comments since the patch looks somewhat different.
>>
>> I can add patch for Kconfig/Makefile later on but suspect there will be some changes required before that.
>>
>> Oh and to answer your question regarding MAX/MIN values of the backlight, 0 = max and 255 = min. So we simply turn it around
>> by returning 255 - value. Same thing when we need to set value.
>>
>> Best wishes
>> Kristoffer Ericson
>>
>> diff --git a/drivers/video/backlight/jornada720_bllcd.c b/drivers/video/backlight/jornada720_bllcd.c
>> +static int jornada_bl_get_brightness(struct backlight_device *dev)
>> +{
>> + int ret;
>> +
>> + /* check if backlight is on */
>> + if (!(PPSR & PPC_LDD1))
>> + return 255;
> return JORNADA_BL_MAX_BRIGHTNESS;
>
>> +
>> + jornada_ssp_start();
>> + if (jornada_ssp_inout(GETBRIGHTNESS) == -ETIMEDOUT) {
>> + printk(KERN_ERR "jornada720_bl: GetBrightness failed\n");
>> + ret = 256;
> ret = JORNADA_BL_MAX_BRIGHTNESS + 1;
>> + } else
>> + ret = jornada_ssp_inout(TXDUMMY);
forgot to mention missing curly bracket here
>> +
>> + jornada_ssp_end();
>> +
>> + /* 0 is max brightness */
>> + return (255 - ret);
> return (JORNADA_BL_MAX_BRIGHTNESS - ret);
>> +}
On Wed, 06 Feb 2008 13:34:12 +0100
Roel Kluin <[email protected]> wrote:
> Kristoffer Ericson wrote:
> > Greetings,
> >
> > Richard I've cleaned up the driver by checking with checkpatch.pl as Russell suggested. I would appreciate it if you could
> > look through the patch again and give comments since the patch looks somewhat different.
> >
> > I can add patch for Kconfig/Makefile later on but suspect there will be some changes required before that.
> >
> > Oh and to answer your question regarding MAX/MIN values of the backlight, 0 = max and 255 = min. So we simply turn it around
> > by returning 255 - value. Same thing when we need to set value.
> >
> > Best wishes
> > Kristoffer Ericson
> >
> > diff --git a/drivers/video/backlight/jornada720_bllcd.c b/drivers/video/backlight/jornada720_bllcd.c
> > new file mode 100644
> > index 0000000..4967b86
> > --- /dev/null
> > +++ b/drivers/video/backlight/jornada720_bllcd.c
> > @@ -0,0 +1,284 @@
> > +/*
> > + * drivers/video/backlight/jornada720_bllcd.c
> > + *
> > + * 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");
> > +
> > +#define JORNADA_LCD_MAX_CONTRAST 0xff
> > +#define JORNADA_LCD_DEFAULT_CONTRAST 0x80
> > +#define JORNADA_BL_MAX_BRIGHTNESS 0xff
> > +#define JORNADA_BL_DEFAULT_BRIGHTNESS 0x19
> > +
> > +struct jornada_bllcd_device {
> > + struct backlight_device *jorn_backlight_device;
> > + struct lcd_device *jorn_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 255;
> return JORNADA_BL_MAX_BRIGHTNESS;
>
Agreed.
> > +
> > + jornada_ssp_start();
> > + if (jornada_ssp_inout(GETBRIGHTNESS) == -ETIMEDOUT) {
> > + printk(KERN_ERR "jornada720_bl: GetBrightness failed\n");
> > + ret = 256;
> ret = JORNADA_BL_MAX_BRIGHTNESS + 1;
> > + } else
> > + ret = jornada_ssp_inout(TXDUMMY);
> > +
> > + jornada_ssp_end();
> > +
> > + /* 0 is max brightness */
> > + return (255 - ret);
> return (JORNADA_BL_MAX_BRIGHTNESS - ret);
Agreed.
> > +}
> > +
> > +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");
> > + else {
else isn't needed here. It should simply tell "didn't work, turning off backlight".
> > + /* backlight off */
> > + PPSR &= ~PPC_LDD1;
> > + PPDR |= PPC_LDD1;
> > + }
> > + } else {/* backlight on */
> > + PPSR |= PPC_LDD1;
>
> missing curly bracket
No, bad formatting essentially. Should make it clear that PPSR |= PPC_LDD1 is related to the lines below
>
> > +
> > + if ((jornada_ssp_inout(SETBRIGHTNESS)) == TXDUMMY) {
> > + /* send brightness value (0 is max, 255 lowest) */
> > + if (jornada_ssp_byte(255 - dev->props.brightness) != -ETIMEDOUT)
> > + ret = (255 - dev->props.brightness);
>
> similarly JORNADA_BL_MAX_BRIGHTNESS in the three lines above
>
Agreed. And formatting is wrong for those if (... show be moved a tab further.
> > + } else
> > + printk(KERN_ERR "jornada720_bl: SetBrightness timeout\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_INFO "jornada_lcd: failure to set contrast\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;
>
> wrong indent
Yeah, formatting again.
>
> > +
> > + 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_INFO "jornada_lcd: getcontrast failed!\n");
> > + ret = 256;
>
> JORNADA_BL_MAX_BRIGHTNESS + 1
>
Agreed
> > + }
> > +
> > + 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);
> > + }
> > +
> > + /* 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);
> > + }
> > +
> > + 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;
> > + 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);
> > +
> > + 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_bl",
> > + },
> > +};
> > +
> > +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);
>
Thanks, that formatting bug wouldn't have been easy to see.
Oki, here is attempt #2 (btw, new mail or just keep thread?)
Tested to build and checkpatch.
diff --git a/drivers/video/backlight/jornada720_bllcd.c b/drivers/video/backlight/jornada720_bllcd.c
new file mode 100644
index 0000000..e911a5e
--- /dev/null
+++ b/drivers/video/backlight/jornada720_bllcd.c
@@ -0,0 +1,286 @@
+/*
+ * drivers/video/backlight/jornada720_bllcd.c
+ *
+ * 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");
+
+#define JORNADA_LCD_MAX_CONTRAST 0xff
+#define JORNADA_LCD_DEFAULT_CONTRAST 0x80
+#define JORNADA_BL_MAX_BRIGHTNESS 0xff
+#define JORNADA_BL_DEFAULT_BRIGHTNESS 0x19
+
+struct jornada_bllcd_device {
+ struct backlight_device *jorn_backlight_device;
+ struct lcd_device *jorn_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 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;
+ } else
+ ret = jornada_ssp_inout(TXDUMMY);
+
+ jornada_ssp_end();
+
+ /* 0 is max brightness */
+ return (JORNADA_BL_MAX_BRIGHTNESS - ret);
+}
+
+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;
+
+ } else { /* backlight on */
+ 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");
+ }
+ 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
+ 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 "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);
+ }
+
+ /* 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);
+ }
+
+ 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;
+ 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);
+
+ 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",
+ },
+};
+
+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, 06 Feb 2008 13:38:39 +0100
Roel Kluin <[email protected]> wrote:
> Roel Kluin wrote:
> > Kristoffer Ericson wrote:
> >> Greetings,
> >>
> >> Richard I've cleaned up the driver by checking with checkpatch.pl as Russell suggested. I would appreciate it if you could
> >> look through the patch again and give comments since the patch looks somewhat different.
> >>
> >> I can add patch for Kconfig/Makefile later on but suspect there will be some changes required before that.
> >>
> >> Oh and to answer your question regarding MAX/MIN values of the backlight, 0 = max and 255 = min. So we simply turn it around
> >> by returning 255 - value. Same thing when we need to set value.
> >>
> >> Best wishes
> >> Kristoffer Ericson
> >>
> >> diff --git a/drivers/video/backlight/jornada720_bllcd.c b/drivers/video/backlight/jornada720_bllcd.c
>
> >> +static int jornada_bl_get_brightness(struct backlight_device *dev)
> >> +{
> >> + int ret;
> >> +
> >> + /* check if backlight is on */
> >> + if (!(PPSR & PPC_LDD1))
> >> + return 255;
> > return JORNADA_BL_MAX_BRIGHTNESS;
> >
> >> +
> >> + jornada_ssp_start();
> >> + if (jornada_ssp_inout(GETBRIGHTNESS) == -ETIMEDOUT) {
> >> + printk(KERN_ERR "jornada720_bl: GetBrightness failed\n");
> >> + ret = 256;
> > ret = JORNADA_BL_MAX_BRIGHTNESS + 1;
> >> + } else
> >> + ret = jornada_ssp_inout(TXDUMMY);
>
> forgot to mention missing curly bracket here
>
> >> +
> >> + jornada_ssp_end();
> >> +
> >> + /* 0 is max brightness */
> >> + return (255 - ret);
> > return (JORNADA_BL_MAX_BRIGHTNESS - ret);
> >> +}
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.
Kristoffer Ericson wrote:
> Oki, here is attempt #2 (btw, new mail or just keep thread?)
>
> Tested to build and checkpatch.
>
> diff --git a/drivers/video/backlight/jornada720_bllcd.c b/drivers/video/backlight/jornada720_bllcd.c
> +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)
since there is no curly bracket here...
> + printk(KERN_ERR "jornada720_bl: \
> + BrightnessOff timeout\n");
...indentation is wrong in lines below
> + /* backlight off */
> + PPSR &= ~PPC_LDD1;
> + PPDR |= PPC_LDD1;
> +
> + } else { /* backlight on */
too much indentation in lines below as well
> + 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");
> + }
> + jornada_ssp_end();
> +
> + return ret;
> +}
> +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
whitespace before 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;
> + }
whitespace before curly bracket
> +
> + return 0;
> +}
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?)
keep thread to keep comments coming.
But always when posting the updated patch include the full
changelog. Document whats done since last posting
below your s-o-b and the '---'.
Then it is so much easier to apply your patch.
As this is an arm patch maybe you would just re-add the changelog
when posting to the patch system but in all other list this rule
apply.
Sam
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
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.
>
On Wed, Feb 06, 2008 at 08:21:28PM +0100, Kristoffer Ericson wrote:
> Oh, and thanks for all the feedback people! (and for doing it nicely)
I'm afraid you missed one - and there's a better fix for one of the other
points I made... 8)
> +static int jornada_bl_probe(struct platform_device *pdev)
> +{
> + struct bllcd_device *bllcd;
int ret;
> +
> + 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)) {
ret = PTR_ERR(bllcd->bl_device);
> + kfree(bllcd);
> + printk(KERN_ERR "bl :failed to register device\n");
> + return -ENODEV;
delete the line above, and then swap the two remaining lines.
return ret;
> + }
> +
> + /* 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);
ret = PTR_ERR(bllcd->lcd_device);
> + kfree(bllcd);
> + printk(KERN_ERR "lcd :failed to register device\n");
> + return -ENODEV;
delete the line above, and then swap the two remaining lines.
return ret;
The reason for swapping the two lines is that it _might_ give gcc a
chance to optimise the two paths.
> +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",
You missed this one - which currently is: tab space space space space.
It should be two tabs.
ATTEMPT #4.
-----------
Fixed issues as suggested by Russell below.
diff --git a/drivers/video/backlight/jornada720_bllcd.c b/drivers/video/backlight/jornada720_bllcd.c
new file mode 100644
index 0000000..f3c3f26
--- /dev/null
+++ b/drivers/video/backlight/jornada720_bllcd.c
@@ -0,0 +1,290 @@
+/*
+ *
+ * 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;
+ int ret;
+
+ 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)) {
+ ret = PTR_ERR(bllcd->bl_device);
+ printk(KERN_ERR "bl :failed to register device\n");
+ kfree(bllcd);
+ return ret;
+ }
+
+ /* lcd driver */
+ bllcd->lcd_device = lcd_device_register(S1D_DEVICENAME,
+ &pdev->dev, NULL, &jornada_lcd_ops);
+ if (IS_ERR(bllcd->lcd_device)) {
+ ret = PTR_ERR(bllcd->bl_device);
+ backlight_device_unregister(bllcd->bl_device);
+ printk(KERN_ERR "lcd :failed to register device\n");
+ kfree(bllcd);
+ return ret;
+ }
+
+ 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 19:31:55 +0000
Russell King - ARM Linux <[email protected]> wrote:
> On Wed, Feb 06, 2008 at 08:21:28PM +0100, Kristoffer Ericson wrote:
> > Oh, and thanks for all the feedback people! (and for doing it nicely)
>
> I'm afraid you missed one - and there's a better fix for one of the other
> points I made... 8)
typical :)
>
> > +static int jornada_bl_probe(struct platform_device *pdev)
> > +{
> > + struct bllcd_device *bllcd;
>
> int ret;
>
> > +
> > + 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)) {
>
> ret = PTR_ERR(bllcd->bl_device);
>
> > + kfree(bllcd);
> > + printk(KERN_ERR "bl :failed to register device\n");
> > + return -ENODEV;
>
> delete the line above, and then swap the two remaining lines.
>
> return ret;
>
> > + }
> > +
> > + /* 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);
>
> ret = PTR_ERR(bllcd->lcd_device);
>
> > + kfree(bllcd);
> > + printk(KERN_ERR "lcd :failed to register device\n");
> > + return -ENODEV;
>
> delete the line above, and then swap the two remaining lines.
>
> return ret;
>
> The reason for swapping the two lines is that it _might_ give gcc a
> chance to optimise the two paths.
nice.
>
> > +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",
>
> You missed this one - which currently is: tab space space space space.
> It should be two tabs.
Doh, right. Do you use a different checkpatch? I mean it doesn't react on that for me.
On Wed, Feb 06, 2008 at 08:43:40PM +0100, Kristoffer Ericson wrote:
> ATTEMPT #4.
> -----------
> Fixed issues as suggested by Russell below.
Great, I'm happy now.
> Doh, right. Do you use a different checkpatch? I mean it doesn't react on
> that for me.
No, all of the comments I've given were spotted by merely reading the patch.
I made a mistake on the latest patch I sent,
if (IS_ERR(bllcd->lcd_device)) {
+ ret = PTR_ERR(bllcd->bl_device);
+ backlight_device_unregister(bllcd->bl_device);
+ printk(KERN_ERR "lcd :failed to register device\n");
+ kfree(bllcd);
+ return ret;
+ }
should ofcourse read
if (IS_ERR(bllcd->lcd_device)) {
+ ret = PTR_ERR(bllcd->lcd_device);
+ backlight_device_unregister(bllcd->bl_device);
+ printk(KERN_ERR "lcd :failed to register device\n");
+ kfree(bllcd);
+ return ret;
+ }
Its fixed now for me. Going to test that mdelay issue now.
On Wed, 6 Feb 2008 19:31:55 +0000
Russell King - ARM Linux <[email protected]> wrote:
> On Wed, Feb 06, 2008 at 08:21:28PM +0100, Kristoffer Ericson wrote:
> > Oh, and thanks for all the feedback people! (and for doing it nicely)
>
> I'm afraid you missed one - and there's a better fix for one of the other
> points I made... 8)
>
> > +static int jornada_bl_probe(struct platform_device *pdev)
> > +{
> > + struct bllcd_device *bllcd;
>
> int ret;
>
> > +
> > + 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)) {
>
> ret = PTR_ERR(bllcd->bl_device);
>
> > + kfree(bllcd);
> > + printk(KERN_ERR "bl :failed to register device\n");
> > + return -ENODEV;
>
> delete the line above, and then swap the two remaining lines.
>
> return ret;
>
> > + }
> > +
> > + /* 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);
>
> ret = PTR_ERR(bllcd->lcd_device);
>
> > + kfree(bllcd);
> > + printk(KERN_ERR "lcd :failed to register device\n");
> > + return -ENODEV;
>
> delete the line above, and then swap the two remaining lines.
>
> return ret;
>
> The reason for swapping the two lines is that it _might_ give gcc a
> chance to optimise the two paths.
>
> > +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",
>
> You missed this one - which currently is: tab space space space space.
> It should be two tabs.
Hello Russell,
> > Doh, right. Do you use a different checkpatch? I mean it doesn't react on
> > that for me.
>
> No, all of the comments I've given were spotted by merely reading the patch.
May I ask how you spot tab vs. 8 space problems? Do you apply the patch
and check in an editor?
Best regards
Uwe
--
Uwe Kleine-K?nig, Software Engineer
Digi International GmbH Branch Breisach, K?ferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962