backlight: Add backlight driver for Bachmann's ot200

PATCH Superseded Christian Gmeiner 2 participants 3 messages Jan 24, 2012 – Jan 25, 2012

Subsystems (2)

Framebuffer layer Backlight class/subsystem

lore

Share & tools

lore

Christian Gmeiner

author

2012-01-24 23:30 UTC #0 lore raw

PATCH 1/1 backlight: Add backlight driver for Bachmann's ot200

Quoted 1 line

From 27574197e9f8425c6aeadffef199c8b7eb969233 Mon Sep 17 00:00:00 2001

From: Christian Gmeiner <[email protected]>
Date: Fri, 13 Jan 2012 15:31:01 +0100
Subject: [PATCH] backlight: Add backlight driver for Bachmann's ot200

Add backlight driver for Bachmann's ot200 visualisation device. The driver uses MFGPT 7 of CS5535 silicon to regulate the backlight.

Signed-off-by Christian Gmeiner <[email protected]>

---
 drivers/video/backlight/Kconfig    |    9 ++
 drivers/video/backlight/Makefile   |    2 +-
 drivers/video/backlight/ot200_bl.c |  167 ++++++++++++++++++++++++++++++++++++
 3 files changed, 177 insertions(+), 1 deletions(-)
 create mode 100644 drivers/video/backlight/ot200_bl.c

drivers/video/backlight/Kconfig +9 -0

index 278aeaa..ab02544 100644
@@ -342,6 +342,15 @@ config BACKLIGHT_AAT2870
 	  If you have a AnalogicTech AAT2870 say Y to enable the
 	  backlight driver.
 
+config BACKLIGHT_OT200
+	tristate "Backlight Driver for Bachmann's OT200"
+	depends on BACKLIGHT_CLASS_DEVICE
+	select MFD_CS5535
+	select CS5535_MFGPT
+	help
+	  To compile this driver as a module, choose M here: the module will be
+	  called ot200_bl.
+
 endif # BACKLIGHT_CLASS_DEVICE
 
 endif # BACKLIGHT_LCD_SUPPORT

drivers/video/backlight/Makefile +1 -1

index fdd1fc4..4f07d3f 100644
@@ -39,4 +39,4 @@ obj-$(CONFIG_BACKLIGHT_ADP8870)	+= adp8870_bl.o
 obj-$(CONFIG_BACKLIGHT_88PM860X) += 88pm860x_bl.o
 obj-$(CONFIG_BACKLIGHT_PCF50633)	+= pcf50633-backlight.o
 obj-$(CONFIG_BACKLIGHT_AAT2870) += aat2870_bl.o
-
+obj-$(CONFIG_BACKLIGHT_OT200) += ot200_bl.o

drivers/video/backlight/ot200_bl.c new file +167 -0

167 lines not on this page · raw message

Signature · 1 line

1.7.5.4

Andres Salomon

2012-01-25 02:09 UTC #1 lore raw

Quoted 22 lines · On Wed, 25 Jan 2012 00:30:31 +0100 Christian Gmeiner…

From 27574197e9f8425c6aeadffef199c8b7eb969233 Mon Sep 17 00:00:00 2001
From: Christian Gmeiner <[email protected]>
Date: Fri, 13 Jan 2012 15:31:01 +0100
Subject: [PATCH] backlight: Add backlight driver for Bachmann's ot200
Add backlight driver for Bachmann's ot200 visualisation device. The driver uses MFGPT 7 of CS5535 silicon to regulate the backlight.
Signed-off-by Christian Gmeiner <[email protected]>
---
 drivers/video/backlight/Kconfig    |    9 ++
 drivers/video/backlight/Makefile   |    2 +-
 drivers/video/backlight/ot200_bl.c |  167
++++++++++++++++++++++++++++++++++++ 3 files changed, 177 insertions(+), 1 deletions(-) create mode 100644 drivers/video/backlight/ot200_bl.c
(quoted diff) +0 -0
b/drivers/video/backlight/Kconfig index 278aeaa..ab02544 100644

drivers/video/backlight/Kconfig config BACKLIGHT_AAT2870 · line 346

Quoted 5 lines

drivers/video/backlight/Kconfig +2 -0

@@ -342,6 +342,15 @@ config BACKLIGHT_AAT2870
 	  If you have a AnalogicTech AAT2870 say Y to enable the
 	  backlight driver.
 
+config BACKLIGHT_OT200
+	tristate "Backlight Driver for Bachmann's OT200"

This is purely personal preference, but the possessive apostrophe for the manufacturer's device seems weird.

drivers/video/backlight/Kconfig config BACKLIGHT_AAT2870 · line 349

Quoted 3 lines

drivers/video/backlight/Kconfig +3 -0

+	depends on BACKLIGHT_CLASS_DEVICE
+	select MFD_CS5535
+	select CS5535_MFGPT

This is modular; is there any reason to select these instead of simply depending upon them? A strict dependency would be preferrable.

Also, CS5535_MFGPT already depends upon MFD_CS5535, so you can probably drop that.

Quoted 36 lines

drivers/video/backlight/Kconfig +2 -0
+	help
+	  To compile this driver as a module, choose M here: the
module will be
drivers/video/backlight/Kconfig +2 -0
+	  called ot200_bl.
+
 endif # BACKLIGHT_CLASS_DEVICE
 
 endif # BACKLIGHT_LCD_SUPPORT
(quoted diff) +0 -0
b/drivers/video/backlight/Makefile index fdd1fc4..4f07d3f 100644
drivers/video/backlight/Makefile +0 -0
@@ -39,4 +39,4 @@ obj-$(CONFIG_BACKLIGHT_ADP8870)	+=
adp8870_bl.o obj-$(CONFIG_BACKLIGHT_88PM860X) += 88pm860x_bl.o
obj-$(CONFIG_BACKLIGHT_PCF50633) += pcf50633-backlight.o
obj-$(CONFIG_BACKLIGHT_AAT2870) += aat2870_bl.o
drivers/video/backlight/Makefile +1 -1
-
+obj-$(CONFIG_BACKLIGHT_OT200) += ot200_bl.o
(quoted diff) +0 -0
b/drivers/video/backlight/ot200_bl.c new file mode 100644 index 0000000..d8e8ba1
drivers/video/backlight/ot200_bl.c new file +7 -0
@@ -0,0 +1,167 @@
+/*
+ * Copyright (C) 2012 Bachmann electronic GmbH
+ *	Christian Gmeiner <[email protected]>
+ *
+ * Backlight driver for Bachmann's ot200.
+ *
+ * This program is free software; you can redistribute it and/or
modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */

drivers/video/backlight/ot200_bl.c

Quoted 9 lines

drivers/video/backlight/ot200_bl.c new file +9 -0

+
+#include <linux/module.h>
+#include <linux/fb.h>
+#include <linux/backlight.h>
+#include <linux/gpio.h>
+#include <linux/cs5535.h>
+
+static struct cs5535_mfgpt_timer *pwm_timer;
+static const  u8 DIM_TABLE[101] = {0, 0, 1, 1, 1, 1, 1, 1, 1, 2, 2,

Please don't use CAPS_VARIABLES unless it's a macro.

Quoted 15 lines

2, 2, 2, 2,
drivers/video/backlight/ot200_bl.c new file +1 -0
+				   2, 2, 2, 2, 3, 3, 3, 3, 3, 3, 3,
4, 4, 4, 4,
drivers/video/backlight/ot200_bl.c new file +1 -0
+				   4, 5, 5, 5, 5, 6, 6, 6, 7, 7, 7,
8, 8, 9, 9,
drivers/video/backlight/ot200_bl.c new file +1 -0
+				   10, 10, 11, 11, 12, 12, 13, 14,
15, 15, 16,
drivers/video/backlight/ot200_bl.c new file +1 -0
+				   17, 18, 19, 20, 21, 22, 23, 24,
26, 27, 28,
drivers/video/backlight/ot200_bl.c new file +1 -0
+				   30, 31, 33, 35, 37, 39, 41, 43,
45, 47, 50,
drivers/video/backlight/ot200_bl.c new file +1 -0
+				   53, 55, 58, 61, 65, 68, 72, 75,
79, 84, 88,
drivers/video/backlight/ot200_bl.c new file +1 -0
+				   93, 97, 103, 108, 114, 120, 126,
133, 140,

drivers/video/backlight/ot200_bl.c line 27

Quoted 1 line

drivers/video/backlight/ot200_bl.c new file +1 -0
+				   147, 155, 163};

Also, please describe what this table means, or is for.

drivers/video/backlight/ot200_bl.c line 38

Quoted 11 lines

drivers/video/backlight/ot200_bl.c new file +11 -0

+
+struct ot200_backlight_data {
+	int current_brightness;
+};
+
+#define GPIO_DIMM	27
+
+static int ot200_backlight_update_status(struct backlight_device *bl)
+{
+	struct ot200_backlight_data *data = bl_get_data(bl);
+	int brightness = bl->props.brightness;

Please use an unsigned here.

drivers/video/backlight/ot200_bl.c line 45

Quoted 7 lines

drivers/video/backlight/ot200_bl.c new file +7 -0

+
+	if (bl->props.state & BL_CORE_FBBLANK)
+		brightness = 0;
+
+	/* enable or disable PWM timer */
+	if (brightness == 0)
+		cs5535_mfgpt_write(pwm_timer, MFGPT_REG_SETUP, 0);

Do you want to disable the timer when the driver/backlight device is unloaded, or will that turn off the backlight?

Quoted 35 lines

drivers/video/backlight/ot200_bl.c new file +3 -0

+	else if (brightness != 0 && data->current_brightness == 0) {
+		cs5535_mfgpt_write(pwm_timer, MFGPT_REG_COUNTER, 0);
+		cs5535_mfgpt_write(pwm_timer, MFGPT_REG_SETUP,

0x8281);

drivers/video/backlight/ot200_bl.c new file +11 -0

+	}
+
+	/* apply new brightness value */
+	cs5535_mfgpt_write(pwm_timer, MFGPT_REG_CMP1,
+					163 - DIM_TABLE[brightness]);
+	data->current_brightness = brightness;
+
+	return 0;
+}
+
+static int ot200_backlight_get_brightness(struct backlight_device

*bl) +{

drivers/video/backlight/ot200_bl.c new file +19 -0

+	struct ot200_backlight_data *data = bl_get_data(bl);
+	return data->current_brightness;
+}
+
+static const struct backlight_ops ot200_backlight_ops = {
+	.update_status	= ot200_backlight_update_status,
+	.get_brightness	= ot200_backlight_get_brightness,
+};
+
+static int ot200_backlight_probe(struct platform_device *pdev)
+{
+	struct backlight_device *bl;
+	struct ot200_backlight_data *data;
+	struct backlight_properties props;
+	int retval = 0;
+
+	/* request gpio */
+	if (gpio_request(GPIO_DIMM, "ot200 backlight dimmer") < 0) {
+		printk(KERN_ERR "ot200 backlight: failed to request

Quoted 1 line

GPIO %d\n",

I notice you're using dev_err below, but printk(KERN_ERR here. Any reason for that? My preference would be dev_err here.

Quoted 9 lines

drivers/video/backlight/ot200_bl.c new file +8 -0

+				GPIO_DIMM);
+		return -ENODEV;
+	}
+
+	/* request timer */
+	pwm_timer = cs5535_mfgpt_alloc_timer(7, MFGPT_DOMAIN_ANY);
+	if (!pwm_timer) {
+		printk(KERN_ERR "ot200 backlight: MFGPT 7 not

available\n");

Ditto.

drivers/video/backlight/ot200_bl.c line 93

Quoted 6 lines

drivers/video/backlight/ot200_bl.c new file +6 -0

+		retval = -ENODEV;
+		goto error_mfgpt_alloc;
+	}
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (data == NULL) {

Please use "if (!data) {" here.

drivers/video/backlight/ot200_bl.c line 105

Quoted 12 lines

drivers/video/backlight/ot200_bl.c new file +12 -0

+		retval = -ENOMEM;
+		goto error_kzalloc;
+	}
+
+	/* setup gpio */
+	cs5535_gpio_set(GPIO_DIMM, GPIO_OUTPUT_ENABLE);
+	cs5535_gpio_set(GPIO_DIMM, GPIO_OUTPUT_AUX1);
+
+	/* setup timer */
+	cs5535_mfgpt_write(pwm_timer, MFGPT_REG_CMP1, 0);
+	cs5535_mfgpt_write(pwm_timer, MFGPT_REG_CMP2, 0xa3);
+	cs5535_mfgpt_write(pwm_timer, MFGPT_REG_SETUP, 0x8281);

Can you please document this? This is a backlight driver; what are you using timers for? What is the setup here meant to do? I'd like to see an english description for people so that a glance over this doesn't require checking the data sheet, and so that we can verify that the code matches the desired effect.

drivers/video/backlight/ot200_bl.c line 112

Quoted 7 lines

drivers/video/backlight/ot200_bl.c new file +7 -0

+
+	data->current_brightness = 100;
+	props.max_brightness = 100;
+	props.brightness = 100;
+	props.type = BACKLIGHT_RAW;
+
+	bl = backlight_device_register("ipc", &pdev->dev, data,

What's ipc?

Quoted 19 lines

drivers/video/backlight/ot200_bl.c new file +1 -0

+					&ot200_backlight_ops,

&props);

drivers/video/backlight/ot200_bl.c new file +2 -0

+	if (IS_ERR(bl)) {
+		dev_err(&pdev->dev, "failed to register

backlight\n");

drivers/video/backlight/ot200_bl.c new file +14 -0

+		retval = PTR_ERR(bl);
+		goto error_backlight_device_register;
+	}
+
+	platform_set_drvdata(pdev, bl);
+
+	return 0;
+
+error_backlight_device_register:
+	kfree(data);
+error_kzalloc:
+	cs5535_mfgpt_free_timer(pwm_timer);
+error_mfgpt_alloc:
+	gpio_free(GPIO_DIMM);

drivers/video/backlight/ot200_bl.c line 139

Quoted 10 lines

drivers/video/backlight/ot200_bl.c new file +10 -0

+	return retval;
+}
+
+static int ot200_backlight_remove(struct platform_device *pdev)
+{
+	struct backlight_device *bl = platform_get_drvdata(pdev);
+	struct ot200_backlight_data *data = bl_get_data(bl);
+
+	backlight_device_unregister(bl);
+	kfree(data);

What about freeing the gpio? Also, the timer? I know the timer code doesn't allow you to reuse the timer once it's been set up, but at least you could avoid leaking the pwm_timer memory.

drivers/video/backlight/ot200_bl.c line 143

Quoted 4 lines

drivers/video/backlight/ot200_bl.c new file +4 -0
+	return 0;
+}
+
+static struct platform_driver ot200_backlight_driver = {

This can probably use __devinitdata.

Quoted 24 lines

drivers/video/backlight/ot200_bl.c new file +24 -0

+	.driver		= {
+		.name	= "ot200-backlight",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= ot200_backlight_probe,
+	.remove		= ot200_backlight_remove,
+};
+
+static int __init ot200_backlight_init(void)
+{
+	return platform_driver_register(&ot200_backlight_driver);
+}
+module_init(ot200_backlight_init);
+
+static void __exit ot200_backlight_exit(void)
+{
+	platform_driver_unregister(&ot200_backlight_driver);
+}
+module_exit(ot200_backlight_exit);
+
+MODULE_DESCRIPTION("backlight driver for Bachmann ot200");
+MODULE_AUTHOR("Christian Gmeiner <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ot200-backlight");

Christian Gmeiner

author

2012-01-25 10:48 UTC #2 lore raw

2012/1/25 Andres Salomon <[email protected]>:

Quoted 28 lines

Quoted 25 lines · On Wed, 25 Jan 2012 00:30:31 +0100 Christian Gmeiner…
From 27574197e9f8425c6aeadffef199c8b7eb969233 Mon Sep 17 00:00:00 2001
From: Christian Gmeiner <[email protected]>
Date: Fri, 13 Jan 2012 15:31:01 +0100
Subject: [PATCH] backlight: Add backlight driver for Bachmann's ot200
Add backlight driver for Bachmann's ot200 visualisation device. The driver uses MFGPT 7 of CS5535 silicon to regulate the backlight.
Signed-off-by Christian Gmeiner <[email protected]>
---
drivers/video/backlight/Kconfig | 9 ++ drivers/video/backlight/Makefile | 2 +- drivers/video/backlight/ot200_bl.c | 167 ++++++++++++++++++++++++++++++++++++ 3 files changed, 177 insertions(+), 1 deletions(-) create mode 100644 drivers/video/backlight/ot200_bl.c
(quoted diff) +0 -0
b/drivers/video/backlight/Kconfig index 278aeaa..ab02544 100644
drivers/video/backlight/Kconfig +0 -0
@@ -342,6 +342,15 @@ config BACKLIGHT_AAT2870
If you have a AnalogicTech AAT2870 say Y to enable the backlight driver.

Quoted 6 lines

Quoted 2 lines
+config BACKLIGHT_OT200
+ tristate "Backlight Driver for Bachmann's OT200"
This is purely personal preference, but the possessive apostrophe for the manufacturer's device seems weird.

okay

Quoted 11 lines

Quoted 4 lines
+ depends on BACKLIGHT_CLASS_DEVICE
+ select MFD_CS5535
+ select CS5535_MFGPT
This is modular; is there any reason to select these instead of simply depending upon them? A strict dependency would be preferrable.
Also, CS5535_MFGPT already depends upon MFD_CS5535, so you can probably drop that.

good argument - will change it

Quoted 40 lines

Quoted 40 lines
+ help
+ To compile this driver as a module, choose M here: the module will be
+ called ot200_bl.
+
endif # BACKLIGHT_CLASS_DEVICE
endif # BACKLIGHT_LCD_SUPPORT
(quoted diff) +0 -0
b/drivers/video/backlight/Makefile index fdd1fc4..4f07d3f 100644
drivers/video/backlight/Makefile +0 -0
@@ -39,4 +39,4 @@ obj-$(CONFIG_BACKLIGHT_ADP8870)     +=
adp8870_bl.o obj-$(CONFIG_BACKLIGHT_88PM860X) += 88pm860x_bl.o obj-$(CONFIG_BACKLIGHT_PCF50633) += pcf50633-backlight.o obj-$(CONFIG_BACKLIGHT_AAT2870) += aat2870_bl.o
drivers/video/backlight/Makefile +1 -1
-
+obj-$(CONFIG_BACKLIGHT_OT200) += ot200_bl.o
(quoted diff) +0 -0
b/drivers/video/backlight/ot200_bl.c new file mode 100644 index 0000000..d8e8ba1
drivers/video/backlight/ot200_bl.c new file +7 -0
@@ -0,0 +1,167 @@
+/*
+ * Copyright (C) 2012 Bachmann electronic GmbH
+ *   Christian Gmeiner <[email protected]>
+ *
+ * Backlight driver for Bachmann's ot200.
+ *
+ * This program is free software; you can redistribute it and/or
modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
drivers/video/backlight/ot200_bl.c new file +3 -0
+
+#include <linux/module.h>
+#include <linux/fb.h>

Quoted 9 lines

drivers/video/backlight/ot200_bl.c
Quoted 6 lines
drivers/video/backlight/ot200_bl.c new file +6 -0
+#include <linux/backlight.h>
+#include <linux/gpio.h>
+#include <linux/cs5535.h>
+
+static struct cs5535_mfgpt_timer *pwm_timer;
+static const  u8 DIM_TABLE[101] = {0, 0, 1, 1, 1, 1, 1, 1, 1, 2, 2,
Please don't use CAPS_VARIABLES unless it's a macro.

okay

Quoted 18 lines

Quoted 17 lines
2, 2, 2, 2,
+ 2, 2, 2, 2, 3, 3, 3, 3, 3, 3, 3, 4, 4, 4, 4,
+ 4, 5, 5, 5, 5, 6, 6, 6, 7, 7, 7, 8, 8, 9, 9,
+ 10, 10, 11, 11, 12, 12, 13, 14, 15, 15, 16,
+ 17, 18, 19, 20, 21, 22, 23, 24, 26, 27, 28,
+ 30, 31, 33, 35, 37, 39, 41, 43, 45, 47, 50,
+ 53, 55, 58, 61, 65, 68, 72, 75, 79, 84, 88,
+ 93, 97, 103, 108, 114, 120, 126, 133, 140,
+ 147, 155, 163};

Quoted 2 lines

Also, please describe what this table means, or is for.

will do in v2

Quoted 14 lines

drivers/video/backlight/ot200_bl.c line 38

Quoted 12 lines

drivers/video/backlight/ot200_bl.c new file +11 -0

+
+struct ot200_backlight_data {
+     int current_brightness;
+};
+
+#define GPIO_DIMM    27
+
+static int ot200_backlight_update_status(struct backlight_device *bl)
+{
+     struct ot200_backlight_data *data = bl_get_data(bl);
+     int brightness = bl->props.brightness;

Please use an unsigned here.

okay

Quoted 12 lines

drivers/video/backlight/ot200_bl.c line 45
Quoted 8 lines
drivers/video/backlight/ot200_bl.c new file +7 -0
+
+     if (bl->props.state & BL_CORE_FBBLANK)
+             brightness = 0;
+
+     /* enable or disable PWM timer */
+     if (brightness == 0)
+             cs5535_mfgpt_write(pwm_timer, MFGPT_REG_SETUP, 0);
Do you want to disable the timer when the driver/backlight device is unloaded, or will that turn off the backlight?

I never thought about this use case, but I think it would be the best to turn brightness of the display to 100%.

Quoted 32 lines

Quoted 32 lines
+ else if (brightness != 0 && data->current_brightness == 0) {
+ cs5535_mfgpt_write(pwm_timer, MFGPT_REG_COUNTER, 0);
+ cs5535_mfgpt_write(pwm_timer, MFGPT_REG_SETUP, 0x8281);
+ }
drivers/video/backlight/ot200_bl.c new file +10 -0
+
+     /* apply new brightness value */
+     cs5535_mfgpt_write(pwm_timer, MFGPT_REG_CMP1,
+                                     163 - DIM_TABLE[brightness]);
+     data->current_brightness = brightness;
+
+     return 0;
+}
+
+static int ot200_backlight_get_brightness(struct backlight_device
*bl) +{
+ struct ot200_backlight_data *data = bl_get_data(bl);
+ return data->current_brightness;
drivers/video/backlight/ot200_bl.c new file +13 -0
+}
+
+static const struct backlight_ops ot200_backlight_ops = {
+     .update_status  = ot200_backlight_update_status,
+     .get_brightness = ot200_backlight_get_brightness,
+};
+
+static int ot200_backlight_probe(struct platform_device *pdev)
+{
+     struct backlight_device *bl;
+     struct ot200_backlight_data *data;
+     struct backlight_properties props;
+     int retval = 0;

Quoted 8 lines

Quoted 5 lines
+
+ /* request gpio */
+ if (gpio_request(GPIO_DIMM, "ot200 backlight dimmer") < 0) {
+ printk(KERN_ERR "ot200 backlight: failed to request GPIO %d\n",
I notice you're using dev_err below, but printk(KERN_ERR here. Any reason for that? My preference would be dev_err here.

upps

Quoted 13 lines

Quoted 10 lines
+ GPIO_DIMM);
+ return -ENODEV;
+ }
+
+ /* request timer */
+ pwm_timer = cs5535_mfgpt_alloc_timer(7, MFGPT_DOMAIN_ANY);
+ if (!pwm_timer) {
+ printk(KERN_ERR "ot200 backlight: MFGPT 7 not available\n");
Ditto.

Quoted 9 lines

Quoted 6 lines
+ retval = -ENODEV;
+ goto error_mfgpt_alloc;
+ }
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (data == NULL) {
Please use "if (!data) {" here.

okay

Quoted 10 lines

Quoted 10 lines
+ retval = -ENOMEM;
+ goto error_kzalloc;
+ }
drivers/video/backlight/ot200_bl.c new file +7 -0
+
+     /* setup gpio */
+     cs5535_gpio_set(GPIO_DIMM, GPIO_OUTPUT_ENABLE);
+     cs5535_gpio_set(GPIO_DIMM, GPIO_OUTPUT_AUX1);
+
+     /* setup timer */
+     cs5535_mfgpt_write(pwm_timer, MFGPT_REG_CMP1, 0);

Quoted 9 lines

Quoted 2 lines
+ cs5535_mfgpt_write(pwm_timer, MFGPT_REG_CMP2, 0xa3);
+ cs5535_mfgpt_write(pwm_timer, MFGPT_REG_SETUP, 0x8281);
Can you please document this? This is a backlight driver; what are you using timers for? What is the setup here meant to do? I'd like to see an english description for people so that a glance over this doesn't require checking the data sheet, and so that we can verify that the code matches the desired effect.

I also will have a look into the datasheet, as the driver is port of a vxworks driver, but I will remove the magic numbers.

Quoted 10 lines

drivers/video/backlight/ot200_bl.c line 112
Quoted 8 lines
drivers/video/backlight/ot200_bl.c new file +7 -0
+
+     data->current_brightness = 100;
+     props.max_brightness = 100;
+     props.brightness = 100;
+     props.type = BACKLIGHT_RAW;
+
+     bl = backlight_device_register("ipc", &pdev->dev, data,
What's ipc?

Industrial PC :) I need a name of the backlight device that is equal on all Bachmann visualisation devices. In the near future I will add post some patches for an other visualisation device. The goal is to have it equal on visualisation device - makes the userspace much simpler.

Quoted 26 lines

Quoted 26 lines
+ &ot200_backlight_ops, &props);
+ if (IS_ERR(bl)) {
+ dev_err(&pdev->dev, "failed to register backlight\n");
+ retval = PTR_ERR(bl);
+ goto error_backlight_device_register;
+ }
drivers/video/backlight/ot200_bl.c new file +17 -0
+
+     platform_set_drvdata(pdev, bl);
+
+     return 0;
+
+error_backlight_device_register:
+     kfree(data);
+error_kzalloc:
+     cs5535_mfgpt_free_timer(pwm_timer);
+error_mfgpt_alloc:
+     gpio_free(GPIO_DIMM);
+     return retval;
+}
+
+static int ot200_backlight_remove(struct platform_device *pdev)
+{
+     struct backlight_device *bl = platform_get_drvdata(pdev);

Quoted 9 lines

Quoted 4 lines
+ struct ot200_backlight_data *data = bl_get_data(bl); +
+ backlight_device_unregister(bl);
+ kfree(data);
What about freeing the gpio? Also, the timer? I know the timer code doesn't allow you to reuse the timer once it's been set up, but at least you could avoid leaking the pwm_timer memory.

good chatch

Quoted 7 lines

drivers/video/backlight/ot200_bl.c line 143
Quoted 5 lines
+ return 0;
drivers/video/backlight/ot200_bl.c new file +3 -0
+}
+
+static struct platform_driver ot200_backlight_driver = {
This can probably use __devinitdata.

okay

Quoted 17 lines

Quoted 17 lines
+ .driver = {
+ .name = "ot200-backlight",
+ .owner = THIS_MODULE,
+ },
+ .probe = ot200_backlight_probe,
+ .remove = ot200_backlight_remove,
drivers/video/backlight/ot200_bl.c new file +10 -0
+};
+
+static int __init ot200_backlight_init(void)
+{
+     return platform_driver_register(&ot200_backlight_driver);
+}
+module_init(ot200_backlight_init);
+
+static void __exit ot200_backlight_exit(void)
+{

Quoted 9 lines

drivers/video/backlight/ot200_bl.c line 167
Quoted 8 lines
+ platform_driver_unregister(&ot200_backlight_driver);
drivers/video/backlight/ot200_bl.c new file +7 -0
+}
+module_exit(ot200_backlight_exit);
+
+MODULE_DESCRIPTION("backlight driver for Bachmann ot200");
+MODULE_AUTHOR("Christian Gmeiner <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ot200-backlight");

I will send a v2 of this patch soon