On Wed, 25 Jan 2012 00:30:31 +0100 Christian Gmeiner <[email protected]> wrote:
> 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 > > 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 <[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. > + */ > + > +#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 <[email protected]>"); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("platform:ot200-backlight"); _______________________________________________ Linux-geode mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-geode
