[PATCH 1/2] mc13783: add LED support

[u.kleine-koenig@pengutronix.de (Uwe Kleine-König)]

Hi Philippe,

On Mon, May 17, 2010 at 06:40:22PM +0200, Philippe R?tornaz wrote:
> This adds basic led support for Freescale MC13783 PMIC.
> 
> Signed-off-by: Philippe R?tornaz <philippe.retornaz@epfl.ch>
> ---
>  drivers/leds/Kconfig        |    7 +
>  drivers/leds/Makefile       |    1 +
>  drivers/leds/leds-mc13783.c |  375 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/mfd/mc13783-core.c  |    4 +
>  include/linux/mfd/mc13783.h |   68 ++++++++
>  5 files changed, 455 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/leds/leds-mc13783.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 505eb64..706386c 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -285,6 +285,13 @@ config LEDS_DELL_NETBOOKS
>  	  This adds support for the Latitude 2100 and similar
>  	  notebooks that have an external LED.
>  
> +config LEDS_MC13783
> +	tristate "LED Support for MC13783 PMIC"
> +	depends on MFD_MC13783
> +	help
> +	  This option enable support for on-chip LED drivers found
> +	  on Freescale Semiconductor MC13783 PMIC.
> +
>  config LEDS_TRIGGERS
>  	bool "LED Trigger support"
>  	help
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 0cd8b99..d084e3b 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -35,6 +35,7 @@ obj-$(CONFIG_LEDS_INTEL_SS4200)		+= leds-ss4200.o
>  obj-$(CONFIG_LEDS_LT3593)		+= leds-lt3593.o
>  obj-$(CONFIG_LEDS_ADP5520)		+= leds-adp5520.o
>  obj-$(CONFIG_LEDS_DELL_NETBOOKS)	+= dell-led.o
> +obj-$(CONFIG_LEDS_MC13783)		+= leds-mc13783.o
>  
>  # LED SPI Drivers
>  obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
> diff --git a/drivers/leds/leds-mc13783.c b/drivers/leds/leds-mc13783.c
> new file mode 100644
> index 0000000..c0427fd
> --- /dev/null
> +++ b/drivers/leds/leds-mc13783.c
> @@ -0,0 +1,375 @@
> +/*
> + * LEDs driver for Freescale MC13783
> + *
> + * Copyright (C) 2010 Philippe R?tornaz
> + *
> + * Based on leds-da903x:
> + * Copyright (C) 2008 Compulab, Ltd.
> + *      Mike Rapoport <mike@compulab.co.il>
> + *
> + * Copyright (C) 2006-2008 Marvell International Ltd.
> + *      Eric Miao <eric.miao@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/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/leds.h>
> +#include <linux/workqueue.h>
> +#include <linux/mfd/mc13783.h>
> +#include <linux/mfd/mc13783-private.h>
please don't use mc13783-private.h.

> +#include <linux/slab.h>
> +
> +struct mc13783_led {
> +	struct led_classdev	cdev;
> +	struct work_struct	work;
> +	struct mc13783		*master;
> +	enum led_brightness	new_brightness;
> +	int			id;
> +};
> +
> +#define BL_P_MASK 0xf
> +#define MD_P_SHIFT 9
> +#define AD_P_SHIFT 13
> +#define KP_P_SHIFT 17
> +#define TC_P_BASE_SHIFT 6
> +#define TC_P_MASK 0x1f
> +static void mc13783_led_work(struct work_struct *work)
> +{
> +	struct mc13783_led *led = container_of(work, struct mc13783_led, work);
> +	int off;
> +	int shift;
> +	int bank;
> +	int o;
> +
> +	mc13783_lock(led->master);
> +	switch (led->id) {
> +	case MC13783_LED_MD:
> +		mc13783_reg_rmw(led->master, MC13783_REG_LED_CONTROL_2,
> +			BL_P_MASK << MD_P_SHIFT,
> +			(led->new_brightness >> 4) << MD_P_SHIFT);
> +		break;
> +	case MC13783_LED_AD:
> +		mc13783_reg_rmw(led->master, MC13783_REG_LED_CONTROL_2,
> +			BL_P_MASK << AD_P_SHIFT,
> +			(led->new_brightness >> 4) << AD_P_SHIFT);
> +		break;
> +	case MC13783_LED_KP:
> +		mc13783_reg_rmw(led->master, MC13783_REG_LED_CONTROL_2,
> +			BL_P_MASK << KP_P_SHIFT,
> +			(led->new_brightness >> 4) << KP_P_SHIFT);
> +		break;
> +	case MC13783_LED_R1:
> +	case MC13783_LED_G1:
> +	case MC13783_LED_B1:
> +	case MC13783_LED_R2:
> +	case MC13783_LED_G2:
> +	case MC13783_LED_B2:
> +	case MC13783_LED_R3:
> +	case MC13783_LED_G3:
> +	case MC13783_LED_B3:
> +		o = led->id - MC13783_LED_R1;
> +		bank = o/3;
> +		off = MC13783_REG_LED_CONTROL_3 + o/3;
> +		shift = (o - bank * 3) * 5 + TC_P_BASE_SHIFT;
> +		mc13783_reg_rmw(led->master, off, TC_P_MASK << shift,
> +				(led->new_brightness >> 3) << shift);
> +
> +		break;
> +	}
> +	mc13783_unlock(led->master);
> +}
> +
> +static void mc13783_led_set(struct led_classdev *led_cdev,
> +			   enum led_brightness value)
> +{
> +	struct mc13783_led *led;
> +
> +	led = container_of(led_cdev, struct mc13783_led, cdev);
> +	led->new_brightness = value;
> +	schedule_work(&led->work);
> +}
I wonder why you don't set the registers directly here, but use a work
struct instead.

> +#define BL_C_MASK 0x7
> +#define MD_C_SHIFT 0
> +#define AD_C_SHIFT 3
> +#define KP_C_SHIFT 6
> +#define TC_C_MASK 0x3
> +static int __devinit mc13783_led_setup(struct mc13783_led *led, int max_current)
> +{
> +	int off;
> +	int shift;
> +	int ret;
> +	int bank;
> +	mc13783_lock(led->master);
> +
> +	switch (led->id) {
> +	case MC13783_LED_MD:
> +		ret = mc13783_reg_rmw(led->master, MC13783_REG_LED_CONTROL_2,
> +				BL_C_MASK << MD_C_SHIFT,
> +				(max_current & BL_C_MASK) << MD_C_SHIFT);
> +		break;
> +	case MC13783_LED_AD:
> +		ret = mc13783_reg_rmw(led->master, MC13783_REG_LED_CONTROL_2,
> +				BL_C_MASK << AD_C_SHIFT,
> +				(max_current & BL_C_MASK) << AD_C_SHIFT);
> +		break;
> +	case MC13783_LED_KP:
> +		ret = mc13783_reg_rmw(led->master, MC13783_REG_LED_CONTROL_2,
> +				BL_C_MASK << KP_C_SHIFT,
> +				(max_current & BL_C_MASK) << KP_C_SHIFT);
> +		break;
> +	case MC13783_LED_R1:
> +	case MC13783_LED_G1:
> +	case MC13783_LED_B1:
> +	case MC13783_LED_R2:
> +	case MC13783_LED_G2:
> +	case MC13783_LED_B2:
> +	case MC13783_LED_R3:
> +	case MC13783_LED_G3:
> +	case MC13783_LED_B3:
> +		bank = (led->id - MC13783_LED_R1)/3;
> +		off = MC13783_REG_LED_CONTROL_3 + bank;
> +		shift = ((led->id - MC13783_LED_R1) - bank * 3) * 2;
> +
> +		ret = mc13783_reg_rmw(led->master, off, TC_C_MASK << shift,
> +					(max_current & TC_C_MASK) << shift);
> +		break;
> +	}
> +
> +	mc13783_unlock(led->master);
> +	return ret;
> +}
> +
> +#define TC1HALF_BIT 18
> +#define SLEWLIM_BIT 23
> +#define TRIODE_TC_BIT 23
> +#define TRIODE_MD_BIT 7
> +#define TRIODE_AD_BIT 8
> +#define TRIODE_KP_BIT 9
> +#define BOOST_BIT 10
> +#define PERIOD_MASK 0x3
> +#define PERIOD_SHIFT 21
> +#define ABMODE_MASK 0x7
> +#define ABMODE_SHIFT 11
> +#define ABREF_MASK 0x3
> +#define ABREF_SHIFT 14
Why not group the defines at the beginning of the file and properly
namespace them?

> +static int __devinit mc13783_leds_prepare(struct platform_device *pdev)
> +{
> +	struct mc13783_leds_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	struct mc13783 *dev = dev_get_drvdata(pdev->dev.parent);
> +	int ret = 0;
> +	int reg = 0;
> +
> +	mc13783_lock(dev);
> +
> +	if (pdata->flags & MC13783_LED_TC1HALF)
> +		reg |= 1 << TC1HALF_BIT;
> +
> +	if (pdata->flags & MC13783_LED_SLEWLIMTC)
> +		reg |= 1 << SLEWLIM_BIT;
> +
> +	ret = mc13783_reg_write(dev, MC13783_REG_LED_CONTROL_1, reg);
> +	if (ret)
> +		goto out;
> +
> +	reg = (pdata->bl_period & PERIOD_MASK) << PERIOD_SHIFT;
> +
> +	if (pdata->flags & MC13783_LED_SLEWLIMBL)
> +		reg |= 1 << SLEWLIM_BIT;
> +
> +	ret = mc13783_reg_write(dev, MC13783_REG_LED_CONTROL_2, reg);
> +	if (ret)
> +		goto out;
> +
> +
only one empty line please.

> +	reg = (pdata->tc1_period & PERIOD_MASK) << PERIOD_SHIFT;
> +	if (pdata->flags & MC13783_LED_TRIODE_TC1)
> +		reg |= 1 << TRIODE_TC_BIT;
> +
> +	ret = mc13783_reg_write(dev, MC13783_REG_LED_CONTROL_3, reg);
> +	if (ret)
> +		goto out;
> +
> +	reg = (pdata->tc2_period & PERIOD_MASK) << PERIOD_SHIFT;
> +	if (pdata->flags & MC13783_LED_TRIODE_TC2)
> +		reg |= 1 << TRIODE_TC_BIT;
> +
> +	ret = mc13783_reg_write(dev, MC13783_REG_LED_CONTROL_4, reg);
> +	if (ret)
> +		goto out;
> +
> +	reg = (pdata->tc3_period & PERIOD_MASK) << PERIOD_SHIFT;
> +	if (pdata->flags & MC13783_LED_TRIODE_TC3)
> +		reg |= 1 << TRIODE_TC_BIT;
> +
> +	ret = mc13783_reg_write(dev, MC13783_REG_LED_CONTROL_5, reg);
> +	if (ret)
> +		goto out;
> +
> +	reg = 1;
> +	if (pdata->flags & MC13783_LED_TRIODE_MD)
> +		reg |= 1 << TRIODE_MD_BIT;
> +	if (pdata->flags & MC13783_LED_TRIODE_AD)
> +		reg |= 1 << TRIODE_AD_BIT;
> +	if (pdata->flags & MC13783_LED_TRIODE_KP)
> +		reg |= 1 << TRIODE_KP_BIT;
> +	if (pdata->flags & MC13783_LED_BOOST_EN)
> +		reg |= 1 << BOOST_BIT;
> +
> +	reg |= (pdata->abmode & ABMODE_MASK) << ABMODE_SHIFT;
> +	reg |= (pdata->abref & ABREF_MASK) << ABREF_SHIFT;
> +
> +	ret = mc13783_reg_write(dev, MC13783_REG_LED_CONTROL_0, reg);
> +
> +out:
> +	mc13783_unlock(dev);
> +	return ret;
> +}
> +
> +static int __devinit mc13783_led_probe(struct platform_device *pdev)
> +{
> +	struct mc13783_leds_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	struct mc13783_led_platform_data *led_cur;
> +	struct mc13783_led *led, *led_dat;
> +	int ret, i;
> +	int init_led = 0;
> +
> +	if (pdata == NULL) {
> +		dev_err(&pdev->dev, "missing platform data\n");
> +		return -ENODEV;
> +	}
> +
> +	if (pdata->num_leds < 1 || pdata->num_leds > MC13783_LED_MAX) {
> +		dev_err(&pdev->dev, "Invalid led count %d\n", pdata->num_leds);
> +		return -EINVAL;
> +	}
> +
> +	led = kzalloc(sizeof(*led) * pdata->num_leds, GFP_KERNEL);
> +	if (led == NULL) {
> +		dev_err(&pdev->dev, "failed to alloc memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = mc13783_leds_prepare(pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "unable to init led driver\n");
> +		goto err_free;
> +	}
> +
> +	for (i = 0; i < pdata->num_leds; i++) {
> +		led_dat = &led[i];
> +		led_cur = &pdata->led[i];
> +
> +		if (led_cur->id > MC13783_LED_MAX || led_cur->id < 0) {
> +			dev_err(&pdev->dev, "invalid id %d\n", led_cur->id);
> +			ret = -EINVAL;
> +			goto err_register;
> +		}
> +
> +		if (init_led & (1 << led_cur->id)) {
> +			dev_err(&pdev->dev, "led %d already initialized\n",
> +					led_cur->id);
> +			ret = -EINVAL;
> +			goto err_register;
> +		}
> +
> +		init_led |= 1 << led_cur->id;
> +		led_dat->cdev.name = led_cur->name;
> +		led_dat->cdev.default_trigger = led_cur->default_trigger;
> +		led_dat->cdev.brightness_set = mc13783_led_set;
> +		led_dat->cdev.brightness = LED_OFF;
> +		led_dat->id = led_cur->id;
> +		led_dat->master = dev_get_drvdata(pdev->dev.parent);
> +
> +		INIT_WORK(&led_dat->work, mc13783_led_work);
> +
> +		ret = led_classdev_register(pdev->dev.parent, &led_dat->cdev);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to register led %d\n",
> +					led_dat->id);
> +			goto err_register;
> +		}
> +
> +		ret = mc13783_led_setup(led_dat, led_cur->max_current);
> +		if (ret) {
> +			dev_err(&pdev->dev, "unable to init led %d\n",
> +					led_dat->id);
> +			i++;
> +			goto err_register;
> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, led);
> +	return 0;
> +
> +err_register:
> +	if (i > 0) {
> +		for (i = i - 1; i >= 0; i--) {
> +			led_classdev_unregister(&led[i].cdev);
> +			cancel_work_sync(&led[i].work);
> +		}
> +	}
the if isn't necessary, just doing for(...) is enough.

> +
> +err_free:
> +	kfree(led);
> +	return ret;
> +}
> +
> +static int __devexit mc13783_led_remove(struct platform_device *pdev)
> +{
> +	struct mc13783_leds_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	struct mc13783_led *led = platform_get_drvdata(pdev);
> +	struct mc13783 *dev = dev_get_drvdata(pdev->dev.parent);
> +	int i;
> +
> +	for (i = 0; i < pdata->num_leds; i++) {
> +		led_classdev_unregister(&led[i].cdev);
> +		cancel_work_sync(&led[i].work);
> +	}
> +
> +	mc13783_lock(dev);
> +
> +	mc13783_reg_write(dev, MC13783_REG_LED_CONTROL_0, 0);
> +	mc13783_reg_write(dev, MC13783_REG_LED_CONTROL_1, 0);
> +	mc13783_reg_write(dev, MC13783_REG_LED_CONTROL_2, 0);
> +	mc13783_reg_write(dev, MC13783_REG_LED_CONTROL_3, 0);
> +	mc13783_reg_write(dev, MC13783_REG_LED_CONTROL_4, 0);
> +	mc13783_reg_write(dev, MC13783_REG_LED_CONTROL_5, 0);
> +
> +	mc13783_unlock(dev);
> +
> +	kfree(led);
> +	return 0;
> +}
> +
> +static struct platform_driver mc13783_led_driver = {
> +	.driver	= {
> +		.name	= "mc13783-led",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= mc13783_led_probe,
> +	.remove		= __devexit_p(mc13783_led_remove),
> +};
> +
> +static int __init mc13783_led_init(void)
> +{
> +	return platform_driver_register(&mc13783_led_driver);
> +}
> +module_init(mc13783_led_init);
> +
> +static void __exit mc13783_led_exit(void)
> +{
> +	platform_driver_unregister(&mc13783_led_driver);
> +}
> +module_exit(mc13783_led_exit);
> +
> +MODULE_DESCRIPTION("LEDs driver for Freescale MC13783 PMIC");
> +MODULE_AUTHOR("Philipe R?tornaz <philippe.retorna@epfl.ch>");
Philippe with two p?  I'm not sure if non-ASCII chars are allowed here.

> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:mc13783-led");
> diff --git a/drivers/mfd/mc13783-core.c b/drivers/mfd/mc13783-core.c
> index 1f68eca..fecf38a 100644
> --- a/drivers/mfd/mc13783-core.c
> +++ b/drivers/mfd/mc13783-core.c
> @@ -679,6 +679,10 @@ err_revision:
>  	if (pdata->flags & MC13783_USE_TOUCHSCREEN)
>  		mc13783_add_subdevice(mc13783, "mc13783-ts");
>  
> +	if (pdata->flags & MC13783_USE_LED)
> +		mc13783_add_subdevice_pdata(mc13783, "mc13783-led",
> +					pdata->leds, sizeof(*pdata->leds));
> +
>  	return 0;
>  }
>  
> diff --git a/include/linux/mfd/mc13783.h b/include/linux/mfd/mc13783.h
> index 8895d9d..95aa8b6 100644
> --- a/include/linux/mfd/mc13783.h
> +++ b/include/linux/mfd/mc13783.h
> @@ -64,6 +64,72 @@ static inline int mc13783_ackirq(struct mc13783 *mc13783, int irq)
>  					MC13783_ADC0_TSMOD1 | \
>  					MC13783_ADC0_TSMOD2)
>  
> +struct mc13783_led_platform_data {
> +#define MC13783_LED_MD		0
> +#define MC13783_LED_AD		1
> +#define MC13783_LED_KP		2
> +#define MC13783_LED_R1		3
> +#define MC13783_LED_G1		4
> +#define MC13783_LED_B1		5
> +#define MC13783_LED_R2		6
> +#define MC13783_LED_G2		7
> +#define MC13783_LED_B2		8
> +#define MC13783_LED_R3		9
> +#define MC13783_LED_G3		10
> +#define MC13783_LED_B3		11
> +#define MC13783_LED_MAX MC13783_LED_B3
> +	int id;
> +	const char *name;
> +	const char *default_trigger;
> +
> +/* Three or two bits current selection depending on the led */
> +	char max_current;
> +};
> +
> +struct mc13783_leds_platform_data {
> +	int num_leds;
> +	struct mc13783_led_platform_data *led;
> +
> +#define MC13783_LED_TRIODE_MD	(1 << 0)
> +#define MC13783_LED_TRIODE_AD	(1 << 1)
> +#define MC13783_LED_TRIODE_KP	(1 << 2)
> +#define MC13783_LED_BOOST_EN	(1 << 3)
> +#define MC13783_LED_TC1HALF	(1 << 4)
> +#define MC13783_LED_SLEWLIMTC	(1 << 5)
> +#define MC13783_LED_SLEWLIMBL	(1 << 6)
> +#define MC13783_LED_TRIODE_TC1	(1 << 7)
> +#define MC13783_LED_TRIODE_TC2	(1 << 8)
> +#define MC13783_LED_TRIODE_TC3	(1 << 9)
> +	int flags;
> +
> +#define MC13783_LED_AB_DISABLED		0
> +#define MC13783_LED_AB_MD1		1
> +#define MC13783_LED_AB_MD12		2
> +#define MC13783_LED_AB_MD123		3
> +#define MC13783_LED_AB_MD1234		4
> +#define MC13783_LED_AB_MD1234_AD1	5
> +#define MC13783_LED_AB_MD1234_AD12	6
> +#define MC13783_LED_AB_MD1_AD		7
> +	char abmode;
> +
> +#define MC13783_LED_ABREF_200MV	0
> +#define MC13783_LED_ABREF_400MV	1
> +#define MC13783_LED_ABREF_600MV	2
> +#define MC13783_LED_ABREF_800MV	3
> +	char abref;
> +
> +#define MC13783_LED_PERIOD_10MS		0
> +#define MC13783_LED_PERIOD_100MS	1
> +#define MC13783_LED_PERIOD_500MS	2
> +#define MC13783_LED_PERIOD_2S		3
> +	char bl_period;
> +	char tc1_period;
> +	char tc2_period;
> +	char tc3_period;
> +};
> +
> +
> +
too many empty lines

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |