Message-ID: <20120124180707.223ce9b1@debxo>
Date:	Tue, 24 Jan 2012 18:07:07 -0800
From:	Andres Salomon <dilinger@...ued.net>
To:	Christian Gmeiner <christian.gmeiner@...il.com>
Cc:	rpurdie@...ys.net, linux-kernel@...r.kernel.org,
	linux-geode@...ts.infradead.org
Subject: Re: [PATCH] backlight: Add backlight driver for Bachmann's ot200

On Wed, 25 Jan 2012 00:30:31 +0100
Christian Gmeiner <christian.gmeiner@...il.com> wrote:

> From 27574197e9f8425c6aeadffef199c8b7eb969233 Mon Sep 17 00:00:00 2001
> From: Christian Gmeiner <christian.gmeiner@...il.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@...il.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@...il.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@...il.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:ot200-backlight");

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists