Re: [PATCH 1/2] mc13783: add LED support
From: Uwe Kleine-König
Date: Mon May 17 2010 - 15:04:28 EST
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@xxxxxxx>
> ---
> 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@xxxxxxxxxxxxxx>
> + *
> + * Copyright (C) 2006-2008 Marvell International Ltd.
> + * Eric Miao <eric.miao@xxxxxxxxxxx>
> + *
> + * 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@xxxxxxx>");
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/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/