Re: [PATCH/HP7XX] - Add combined LCD / BL driver for platform HPJornada 7xx handhelds

[Kristoffer Ericson]

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 <Kristoffer.Ericson@xxxxxxxxx>
+ *
+ * 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 <kristoffer.ericson@xxxxxxxxx>");
+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 <linux@xxxxxxxxxxxxxxxx> 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.

Attachment: jornada720_bllcd.patch
Description: Binary data