* [PATCH] backlight: Add backlight driver for Bachmann's ot200
@ 2012-01-24 23:30 Christian Gmeiner
2012-01-25 2:07 ` Andres Salomon
0 siblings, 1 reply; 3+ messages in thread
From: Christian Gmeiner @ 2012-01-24 23:30 UTC (permalink / raw)
To: rpurdie, linux-kernel, dilinger, linux-geode
>From 27574197e9f8425c6aeadffef199c8b7eb969233 Mon Sep 17 00:00:00 2001
From: Christian Gmeiner <christian.gmeiner@gmail.com>
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 <christian.gmeiner@gmail.com>
---
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
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 278aeaa..ab02544 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -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
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index fdd1fc4..4f07d3f 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -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
diff --git a/drivers/video/backlight/ot200_bl.c b/drivers/video/backlight/ot200_bl.c
new file mode 100644
index 0000000..d8e8ba1
--- /dev/null
+++ b/drivers/video/backlight/ot200_bl.c
@@ -0,0 +1,167 @@
+/*
+ * Copyright (C) 2012 Bachmann electronic GmbH
+ * Christian Gmeiner <christian.gmeiner@gmail.com>
+ *
+ * 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.
+ */
+
+#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, 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};
+
+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;
+
+ 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);
+ 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);
+ }
+
+ /* 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;
+}
+
+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 GPIO %d\n",
+ 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");
+ retval = -ENODEV;
+ goto error_mfgpt_alloc;
+ }
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (data == NULL) {
+ 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);
+
+ data->current_brightness = 100;
+ props.max_brightness = 100;
+ props.brightness = 100;
+ props.type = BACKLIGHT_RAW;
+
+ bl = backlight_device_register("ipc", &pdev->dev, data,
+ &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;
+ }
+
+ 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);
+ struct ot200_backlight_data *data = bl_get_data(bl);
+
+ backlight_device_unregister(bl);
+ kfree(data);
+ return 0;
+}
+
+static struct platform_driver ot200_backlight_driver = {
+ .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 <christian.gmeiner@gmail.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ot200-backlight");
--
1.7.5.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] backlight: Add backlight driver for Bachmann's ot200
2012-01-24 23:30 [PATCH] backlight: Add backlight driver for Bachmann's ot200 Christian Gmeiner
@ 2012-01-25 2:07 ` Andres Salomon
2012-01-25 10:48 ` Christian Gmeiner
0 siblings, 1 reply; 3+ messages in thread
From: Andres Salomon @ 2012-01-25 2:07 UTC (permalink / raw)
To: Christian Gmeiner; +Cc: rpurdie, linux-kernel, linux-geode
On Wed, 25 Jan 2012 00:30:31 +0100
Christian Gmeiner <christian.gmeiner@gmail.com> wrote:
> From 27574197e9f8425c6aeadffef199c8b7eb969233 Mon Sep 17 00:00:00 2001
> From: Christian Gmeiner <christian.gmeiner@gmail.com>
> 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 <christian.gmeiner@gmail.com>
> ---
> 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
>
> diff --git a/drivers/video/backlight/Kconfig
> b/drivers/video/backlight/Kconfig index 278aeaa..ab02544 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -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.
> + 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.
> + 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
> diff --git a/drivers/video/backlight/Makefile
> b/drivers/video/backlight/Makefile index fdd1fc4..4f07d3f 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -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
> diff --git a/drivers/video/backlight/ot200_bl.c
> b/drivers/video/backlight/ot200_bl.c new file mode 100644
> index 0000000..d8e8ba1
> --- /dev/null
> +++ b/drivers/video/backlight/ot200_bl.c
> @@ -0,0 +1,167 @@
> +/*
> + * Copyright (C) 2012 Bachmann electronic GmbH
> + * Christian Gmeiner <christian.gmeiner@gmail.com>
> + *
> + * 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.
> + */
> +
> +#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.
> 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};
Also, please describe what this table means, or is for.
> +
> +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.
> +
> + 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?
> + 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);
> + }
> +
> + /* 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;
> +}
> +
> +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
> 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.
> + 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.
> + retval = -ENODEV;
> + goto error_mfgpt_alloc;
> + }
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (data == NULL) {
Please use "if (!data) {" here.
> + 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.
> +
> + 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?
> + &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;
> + }
> +
> + 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);
> + 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.
> + return 0;
> +}
> +
> +static struct platform_driver ot200_backlight_driver = {
This can probably use __devinitdata.
> + .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 <christian.gmeiner@gmail.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:ot200-backlight");
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] backlight: Add backlight driver for Bachmann's ot200
2012-01-25 2:07 ` Andres Salomon
@ 2012-01-25 10:48 ` Christian Gmeiner
0 siblings, 0 replies; 3+ messages in thread
From: Christian Gmeiner @ 2012-01-25 10:48 UTC (permalink / raw)
To: Andres Salomon; +Cc: rpurdie, linux-kernel, linux-geode
2012/1/25 Andres Salomon <dilinger@queued.net>:
> On Wed, 25 Jan 2012 00:30:31 +0100
> Christian Gmeiner <christian.gmeiner@gmail.com> wrote:
>
>> From 27574197e9f8425c6aeadffef199c8b7eb969233 Mon Sep 17 00:00:00 2001
>> From: Christian Gmeiner <christian.gmeiner@gmail.com>
>> 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 <christian.gmeiner@gmail.com>
>> ---
>> 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
>>
>> diff --git a/drivers/video/backlight/Kconfig
>> b/drivers/video/backlight/Kconfig index 278aeaa..ab02544 100644
>> --- a/drivers/video/backlight/Kconfig
>> +++ b/drivers/video/backlight/Kconfig
>> @@ -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.
>
okay
>
>> + 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
>
>> + 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
>> diff --git a/drivers/video/backlight/Makefile
>> b/drivers/video/backlight/Makefile index fdd1fc4..4f07d3f 100644
>> --- a/drivers/video/backlight/Makefile
>> +++ b/drivers/video/backlight/Makefile
>> @@ -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
>> diff --git a/drivers/video/backlight/ot200_bl.c
>> b/drivers/video/backlight/ot200_bl.c new file mode 100644
>> index 0000000..d8e8ba1
>> --- /dev/null
>> +++ b/drivers/video/backlight/ot200_bl.c
>> @@ -0,0 +1,167 @@
>> +/*
>> + * Copyright (C) 2012 Bachmann electronic GmbH
>> + * Christian Gmeiner <christian.gmeiner@gmail.com>
>> + *
>> + * 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.
>> + */
>> +
>> +#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.
>
okay
>
>> 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};
>
> Also, please describe what this table means, or is for.
>
will do in v2
>
>> +
>> +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
>
>> +
>> + 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%.
>
>> + 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);
>> + }
>> +
>> + /* 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;
>> +}
>> +
>> +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
>> 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
>
>> + 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.
>
>> + retval = -ENODEV;
>> + goto error_mfgpt_alloc;
>> + }
>> +
>> + data = kzalloc(sizeof(*data), GFP_KERNEL);
>> + if (data == NULL) {
>
> Please use "if (!data) {" here.
>
okay
>> + 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.
>
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.
>
>> +
>> + 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.
>
>> + &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;
>> + }
>> +
>> + 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);
>> + 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
>
>> + return 0;
>> +}
>> +
>> +static struct platform_driver ot200_backlight_driver = {
>
> This can probably use __devinitdata.
okay
>
>> + .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 <christian.gmeiner@gmail.com>");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:ot200-backlight");
>
I will send a v2 of this patch soon
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-01-25 10:48 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-24 23:30 [PATCH] backlight: Add backlight driver for Bachmann's ot200 Christian Gmeiner
2012-01-25 2:07 ` Andres Salomon
2012-01-25 10:48 ` Christian Gmeiner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).