[PATCH v2 06/09] backlight: enable backlight in 88pm860x

[PATCH v2 06/09] backlight: enable backlight in 88pm860x

[Haojian Zhuang]

[PATCH v2 06/09] backlight: enable backlight in 88pm860x

[Samuel Ortiz]

Hi Haojian, Richard,

On Wed, Dec 09, 2009 at 08:15:28AM -0500, Haojian Zhuang wrote:
> From 8d2bc9826f758f113fde6f3fd01723111fbf2a91 Mon Sep 17 00:00:00 2001
> From: Haojian Zhuang <haojian.zhuang@marvell.com>
> Date: Mon, 9 Nov 2009 12:41:07 -0500
> Subject: [PATCH] backlight: enable backlight in 88pm860x
> 
> At most, three backlight device can be supported in 88pm860x driver.
I applied this patch and the LED one to my for-next branch.
Richard, whenever you have time for that, could you please quickly check if
they oook ok to you ? They're 2.6.34 material, so no rush here.

Cheers,
Samuel.


> Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
> ---
>  drivers/video/backlight/88pm860x_bl.c |  304 +++++++++++++++++++++++++++++++++
>  drivers/video/backlight/Kconfig       |    6 +
>  drivers/video/backlight/Makefile      |    1 +
>  3 files changed, 311 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/video/backlight/88pm860x_bl.c
> 
> diff --git a/drivers/video/backlight/88pm860x_bl.c
> b/drivers/video/backlight/88pm860x_bl.c
> new file mode 100644
> index 0000000..b8f705c
> --- /dev/null
> +++ b/drivers/video/backlight/88pm860x_bl.c
> @@ -0,0 +1,304 @@
> +/*
> + * Backlight driver for Marvell Semiconductor 88PM8606
> + *
> + * Copyright (C) 2009 Marvell International Ltd.
> + *	Haojian Zhuang <haojian.zhuang@marvell.com>
> + *
> + * 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/init.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/fb.h>
> +#include <linux/i2c.h>
> +#include <linux/backlight.h>
> +#include <linux/mfd/88pm860x.h>
> +
> +#define MAX_BRIGHTNESS		(0xFF)
> +#define MIN_BRIGHTNESS		(0)
> +
> +#define CURRENT_MASK		(0x1F << 1)
> +
> +struct pm860x_backlight_data {
> +	struct pm860x_chip *chip;
> +	struct i2c_client *i2c;
> +	int	current_brightness;
> +	int	port;
> +	int	pwm;
> +	int	iset;
> +};
> +
> +static inline int wled_a(int port)
> +{
> +	int ret;
> +
> +	ret = ((port - PM8606_BACKLIGHT1) << 1) + 2;
> +	return ret;
> +}
> +
> +static inline int wled_b(int port)
> +{
> +	int ret;
> +
> +	ret = ((port - PM8606_BACKLIGHT1) << 1) + 3;
> +	return ret;
> +}
> +
> +/* WLED2 & WLED3 share the same IDC */
> +static inline int wled_idc(int port)
> +{
> +	int ret;
> +
> +	switch (port) {
> +	case PM8606_BACKLIGHT1:
> +	case PM8606_BACKLIGHT2:
> +		ret = ((port - PM8606_BACKLIGHT1) << 1) + 3;
> +		break;
> +	case PM8606_BACKLIGHT3:
> +	default:
> +		ret = ((port - PM8606_BACKLIGHT2) << 1) + 3;
> +		break;
> +	}
> +	return ret;
> +}
> +
> +static int pm860x_backlight_set(struct backlight_device *bl, int brightness)
> +{
> +	struct pm860x_backlight_data *data = bl_get_data(bl);
> +	struct pm860x_chip *chip = data->chip;
> +	unsigned char value;
> +	int ret;
> +
> +	if (brightness > MAX_BRIGHTNESS)
> +		value = MAX_BRIGHTNESS;
> +	else
> +		value = brightness;
> +
> +	ret = pm860x_reg_write(data->i2c, wled_a(data->port), value);
> +	if (ret < 0)
> +		goto out;
> +
> +	if ((data->current_brightness == 0) && brightness) {
> +		if (data->iset) {
> +			ret = pm860x_set_bits(data->i2c, wled_idc(data->port),
> +					      CURRENT_MASK, data->iset);
> +			if (ret < 0)
> +				goto out;
> +		}
> +		if (data->pwm) {
> +			ret = pm860x_set_bits(data->i2c, PM8606_PWM,
> +					      PM8606_PWM_FREQ_MASK, data->pwm);
> +			if (ret < 0)
> +				goto out;
> +		}
> +		if (brightness == MAX_BRIGHTNESS) {
> +			/* set WLED_ON bit as 100% */
> +			ret = pm860x_set_bits(data->i2c, wled_b(data->port),
> +					      PM8606_WLED_ON, PM8606_WLED_ON);
> +		}
> +	} else {
> +		if (brightness == MAX_BRIGHTNESS) {
> +			/* set WLED_ON bit as 100% */
> +			ret = pm860x_set_bits(data->i2c, wled_b(data->port),
> +					      PM8606_WLED_ON, PM8606_WLED_ON);
> +		} else {
> +			/* clear WLED_ON bit since it's not 100% */
> +			ret = pm860x_set_bits(data->i2c, wled_b(data->port),
> +					      PM8606_WLED_ON, 0);
> +		}
> +	}
> +	if (ret < 0)
> +		goto out;
> +
> +	dev_dbg(chip->dev, "set brightness %d\n", value);
> +	data->current_brightness = value;
> +	return 0;
> +out:
> +	dev_dbg(chip->dev, "set brightness %d failure with return "
> +		"value:%d\n", value, ret);
> +	return ret;
> +}
> +
> +static int pm860x_backlight_update_status(struct backlight_device *bl)
> +{
> +	int brightness = bl->props.brightness;
> +
> +	if (bl->props.power != FB_BLANK_UNBLANK)
> +		brightness = 0;
> +
> +	if (bl->props.fb_blank != FB_BLANK_UNBLANK)
> +		brightness = 0;
> +
> +	if (bl->props.state & BL_CORE_SUSPENDED)
> +		brightness = 0;
> +
> +	return pm860x_backlight_set(bl, brightness);
> +}
> +
> +static int pm860x_backlight_get_brightness(struct backlight_device *bl)
> +{
> +	struct pm860x_backlight_data *data = bl_get_data(bl);
> +	struct pm860x_chip *chip = data->chip;
> +	int ret;
> +
> +	ret = pm860x_reg_read(data->i2c, wled_a(data->port));
> +	if (ret < 0)
> +		goto out;
> +	data->current_brightness = ret;
> +	dev_dbg(chip->dev, "get brightness %d\n", data->current_brightness);
> +	return data->current_brightness;
> +out:
> +	return -EINVAL;
> +}
> +
> +static struct backlight_ops pm860x_backlight_ops = {
> +	.options	= BL_CORE_SUSPENDRESUME,
> +	.update_status	= pm860x_backlight_update_status,
> +	.get_brightness	= pm860x_backlight_get_brightness,
> +};
> +
> +static int __check_device(struct pm860x_backlight_pdata *pdata, char *name)
> +{
> +	struct pm860x_backlight_pdata *p = pdata;
> +	int ret = -EINVAL;
> +
> +	while (p && p->id) {
> +		if ((p->id != PM8606_ID_BACKLIGHT) || (p->flags < 0))
> +			break;
> +
> +		if (!strncmp(name, pm860x_backlight_name[p->flags],
> +			MFD_NAME_SIZE)) {
> +			ret = (int)p->flags;
> +			break;
> +		}
> +		p++;
> +	}
> +	return ret;
> +}
> +
> +static int pm860x_backlight_probe(struct platform_device *pdev)
> +{
> +	struct pm860x_chip *chip = dev_get_drvdata(pdev->dev.parent);
> +	struct pm860x_platform_data *pm860x_pdata;
> +	struct pm860x_backlight_pdata *pdata = NULL;
> +	struct pm860x_backlight_data *data;
> +	struct backlight_device *bl;
> +	struct resource *res;
> +	unsigned char value;
> +	char name[MFD_NAME_SIZE];
> +	int ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +	if (res == NULL) {
> +		dev_err(&pdev->dev, "No I/O resource!\n");
> +		return -EINVAL;
> +	}
> +
> +	if (pdev->dev.parent->platform_data) {
> +		pm860x_pdata = pdev->dev.parent->platform_data;
> +		pdata = pm860x_pdata->backlight;
> +	}
> +	if (pdata == NULL) {
> +		dev_err(&pdev->dev, "platform data isn't assigned to "
> +			"backlight\n");
> +		return -EINVAL;
> +	}
> +
> +	data = kzalloc(sizeof(struct pm860x_backlight_data), GFP_KERNEL);
> +	if (data == NULL)
> +		return -ENOMEM;
> +	strncpy(name, res->name, MFD_NAME_SIZE);
> +	data->chip = chip;
> +	data->i2c = (chip->id == CHIP_PM8606) ? chip->client	\
> +			: chip->companion;
> +	data->current_brightness = MAX_BRIGHTNESS;
> +	data->pwm = pdata->pwm;
> +	data->iset = pdata->iset;
> +	data->port = __check_device(pdata, name);
> +	if (data->port < 0) {
> +		dev_err(&pdev->dev, "wrong platform data is assigned");
> +		return -EINVAL;
> +	}
> +
> +	bl = backlight_device_register(name, &pdev->dev, data,
> +					&pm860x_backlight_ops);
> +	if (IS_ERR(bl)) {
> +		dev_err(&pdev->dev, "failed to register backlight\n");
> +		kfree(data);
> +		return PTR_ERR(bl);
> +	}
> +	bl->props.max_brightness = MAX_BRIGHTNESS;
> +	bl->props.brightness = MAX_BRIGHTNESS;
> +
> +	platform_set_drvdata(pdev, bl);
> +
> +	/* Enable reference VSYS */
> +	ret = pm860x_reg_read(data->i2c, PM8606_VSYS);
> +	if (ret < 0)
> +		goto out;
> +	if ((ret & PM8606_VSYS_EN) == 0) {
> +		value = ret | PM8606_VSYS_EN;
> +		ret = pm860x_reg_write(data->i2c, PM8606_VSYS, value);
> +		if (ret < 0)
> +			goto out;
> +	}
> +	/* Enable reference OSC */
> +	ret = pm860x_reg_read(data->i2c, PM8606_MISC);
> +	if (ret < 0)
> +		goto out;
> +	if ((ret & PM8606_MISC_OSC_EN) == 0) {
> +		value = ret | PM8606_MISC_OSC_EN;
> +		ret = pm860x_reg_write(data->i2c, PM8606_MISC, value);
> +		if (ret < 0)
> +			goto out;
> +	}
> +	/* read current backlight */
> +	ret = pm860x_backlight_get_brightness(bl);
> +	if (ret < 0)
> +		goto out;
> +
> +	backlight_update_status(bl);
> +	return 0;
> +out:
> +	kfree(data);
> +	return ret;
> +}
> +
> +static int pm860x_backlight_remove(struct platform_device *pdev)
> +{
> +	struct backlight_device *bl = platform_get_drvdata(pdev);
> +	struct pm860x_backlight_data *data = bl_get_data(bl);
> +
> +	backlight_device_unregister(bl);
> +	kfree(data);
> +	return 0;
> +}
> +
> +static struct platform_driver pm860x_backlight_driver = {
> +	.driver		= {
> +		.name	= "88pm860x-backlight",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= pm860x_backlight_probe,
> +	.remove		= pm860x_backlight_remove,
> +};
> +
> +static int __init pm860x_backlight_init(void)
> +{
> +	return platform_driver_register(&pm860x_backlight_driver);
> +}
> +module_init(pm860x_backlight_init);
> +
> +static void __exit pm860x_backlight_exit(void)
> +{
> +	platform_driver_unregister(&pm860x_backlight_driver);
> +}
> +module_exit(pm860x_backlight_exit);
> +
> +MODULE_DESCRIPTION("Backlight Driver for Marvell Semiconductor 88PM8606");
> +MODULE_AUTHOR("Haojian Zhuang <haojian.zhuang@marvell.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:88pm860x-backlight");
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 09bfa96..96a6b30 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -262,3 +262,9 @@ config BACKLIGHT_ADP5520
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called adp5520_bl.
> 
> +config BACKLIGHT_88PM860X
> +	tristate "Backlight Driver for 88PM8606 using WLED"
> +	depends on BACKLIGHT_CLASS_DEVICE && MFD_88PM860X
> +	help
> +	  Say Y to enable the backlight driver for Marvell 88PM8606.
> +
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index 9a40554..802d467 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -28,4 +28,5 @@ obj-$(CONFIG_BACKLIGHT_SAHARA)	+= kb3886_bl.o
>  obj-$(CONFIG_BACKLIGHT_WM831X)	+= wm831x_bl.o
>  obj-$(CONFIG_BACKLIGHT_ADX)    += adx_bl.o
>  obj-$(CONFIG_BACKLIGHT_ADP5520)	+= adp5520_bl.o
> +obj-$(CONFIG_BACKLIGHT_88PM860X) += 88pm860x_bl.o
> 
> -- 
> 1.5.6.5

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH v2 06/09] backlight: enable backlight in 88pm860x

[Richard Purdie]

On Fri, 2010-01-08 at 12:06 +0100, Samuel Ortiz wrote:
> Hi Haojian, Richard,
> 
> On Wed, Dec 09, 2009 at 08:15:28AM -0500, Haojian Zhuang wrote:
> > From 8d2bc9826f758f113fde6f3fd01723111fbf2a91 Mon Sep 17 00:00:00 2001
> > From: Haojian Zhuang <haojian.zhuang@marvell.com>
> > Date: Mon, 9 Nov 2009 12:41:07 -0500
> > Subject: [PATCH] backlight: enable backlight in 88pm860x
> > 
> > At most, three backlight device can be supported in 88pm860x driver.
> I applied this patch and the LED one to my for-next branch.
> Richard, whenever you have time for that, could you please quickly check if
> they oook ok to you ? They're 2.6.34 material, so no rush here.

I had a look through, they look basically ok. 

In the LED patch I'm not sure I like the SET_BRIGHTNESS and SET_BLINK
sharing of a workqueue though. Its not going to crash, I can just see
values potentially getting lost as the code has a race condition. It
would be easier if it just compared led->current_brightness to
led->brightness acting if needed and something similar for LED blinking.

Cheers,

Richard


-- 
Richard Purdie
Intel Open Source Technology Centre

[PATCH v2 06/09] backlight: enable backlight in 88pm860x

[Haojian Zhuang]

On Fri, Jan 8, 2010 at 7:27 AM, Richard Purdie <rpurdie@rpsys.net> wrote:
> On Fri, 2010-01-08 at 12:06 +0100, Samuel Ortiz wrote:
>> Hi Haojian, Richard,
>>
>> On Wed, Dec 09, 2009 at 08:15:28AM -0500, Haojian Zhuang wrote:
>> > From 8d2bc9826f758f113fde6f3fd01723111fbf2a91 Mon Sep 17 00:00:00 2001
>> > From: Haojian Zhuang <haojian.zhuang@marvell.com>
>> > Date: Mon, 9 Nov 2009 12:41:07 -0500
>> > Subject: [PATCH] backlight: enable backlight in 88pm860x
>> >
>> > At most, three backlight device can be supported in 88pm860x driver.
>> I applied this patch and the LED one to my for-next branch.
>> Richard, whenever you have time for that, could you please quickly check if
>> they oook ok to you ? They're 2.6.34 material, so no rush here.
>
> I had a look through, they look basically ok.
>
> In the LED patch I'm not sure I like the SET_BRIGHTNESS and SET_BLINK
> sharing of a workqueue though. Its not going to crash, I can just see
> values potentially getting lost as the code has a race condition. It
> would be easier if it just compared led->current_brightness to
> led->brightness acting if needed and something similar for LED blinking.
>
> Cheers,
>
> Richard
>

Excuse me that there's a mutex lock in __led_set(). Both
SET_BRIGHTNESS & SET_BLINK calls __led_set(). There shouldn't be race
condition on set led. What's your opinion?

Best Regards
Haojian

[PATCH v2 06/09] backlight: enable backlight in 88pm860x

[Richard Purdie]

On Mon, 2010-01-11 at 03:32 -0500, Haojian Zhuang wrote:
> On Fri, Jan 8, 2010 at 7:27 AM, Richard Purdie <rpurdie@rpsys.net> >
> In the LED patch I'm not sure I like the SET_BRIGHTNESS and SET_BLINK
> > sharing of a workqueue though. Its not going to crash, I can just see
> > values potentially getting lost as the code has a race condition. It
> > would be easier if it just compared led->current_brightness to
> > led->brightness acting if needed and something similar for LED blinking.
>
> Excuse me that there's a mutex lock in __led_set(). Both
> SET_BRIGHTNESS & SET_BLINK calls __led_set(). There shouldn't be race
> condition on set led. What's your opinion?

There is a race condition since if you get a blink and brightness set
call at around the same time, only one of them will end up being
applied. Its not going to crash but its also less than ideal.

Cheers,

Richard