[PATCH v2] leds: add new lp8788 led driver

[PATCH v2] leds: add new lp8788 led driver

[Kim, Milo]

TI LP8788 PMU has the current sink as the keyboard led driver.
The brightness is controlled by the i2c commands.
Configurable parameters can be defined in the platform side.

Patch v2.
(a) use workqueue on changing the brightness

(b) use mutex_lock/unlock when the brightness is set
    and the led block of lp8788 device is enabled

(c) remove err_dev on _probe()
    : just return as returned value if any errors

(d) replace module_init/exit() with module_platform_driver()

(e) add led configuration structure and loading them by default
    if platform data is null

Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
---
 drivers/leds/Kconfig       |    7 ++
 drivers/leds/Makefile      |    1 +
 drivers/leds/leds-lp8788.c |  192 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 200 insertions(+), 0 deletions(-)
 create mode 100644 drivers/leds/leds-lp8788.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index f028f03..a498deb 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -200,6 +200,13 @@ config LEDS_LP5523
 	  Driver provides direct control via LED class and interface for
 	  programming the engines.
 
+config LEDS_LP8788
+	tristate "LED support for the TI LP8788 PMIC"
+	depends on LEDS_CLASS
+	depends on MFD_LP8788
+	help
+	  This option enables support for the Keyboard LEDs on the LP8788 PMIC.
+
 config LEDS_CLEVO_MAIL
 	tristate "Mail LED on Clevo notebook"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 5eebd7b..f156193 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_LEDS_GPIO)			+= leds-gpio.o
 obj-$(CONFIG_LEDS_LP3944)		+= leds-lp3944.o
 obj-$(CONFIG_LEDS_LP5521)		+= leds-lp5521.o
 obj-$(CONFIG_LEDS_LP5523)		+= leds-lp5523.o
+obj-$(CONFIG_LEDS_LP8788)		+= leds-lp8788.o
 obj-$(CONFIG_LEDS_TCA6507)		+= leds-tca6507.o
 obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
 obj-$(CONFIG_LEDS_HP6XX)		+= leds-hp6xx.o
diff --git a/drivers/leds/leds-lp8788.c b/drivers/leds/leds-lp8788.c
new file mode 100644
index 0000000..574b49f
--- /dev/null
+++ b/drivers/leds/leds-lp8788.c
@@ -0,0 +1,192 @@
+/*
+ * TI LP8788 MFD - keyled driver
+ *
+ * Copyright 2012 Texas Instruments
+ *
+ * Author: Milo(Woogyom) Kim <milo.kim@ti.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/slab.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/leds.h>
+#include <linux/mutex.h>
+#include <linux/mfd/lp8788.h>
+#include <linux/mfd/lp8788-isink.h>
+
+#define MAX_BRIGHTNESS			LP8788_ISINK_MAX_PWM
+#define DEFAULT_LED_NAME		"keyboard-backlight"
+
+struct lp8788_led {
+	struct lp8788 *lp;
+	struct mutex lock;
+	struct work_struct work;
+	struct led_classdev led_dev;
+	enum lp8788_isink_number isink_num;
+	enum led_brightness brightness;
+	int on;
+};
+
+struct lp8788_led_config {
+	enum lp8788_isink_scale scale;
+	enum lp8788_isink_number num;
+	int iout;
+};
+
+static struct lp8788_led_config default_led_config = {
+	.scale = LP8788_ISINK_SCALE_100mA,
+	.num   = LP8788_ISINK_3,
+	.iout  = 0,
+};
+
+static int lp8788_led_init_device(struct lp8788_led *led,
+				struct lp8788_led_platform_data *pdata)
+{
+	struct lp8788_led_config *cfg = &default_led_config;
+	u8 addr, mask, val;
+	int ret;
+
+	if (pdata) {
+		cfg->scale = pdata->scale;
+		cfg->num = pdata->num;
+		cfg->iout = pdata->iout_code;
+	}
+
+	led->isink_num = cfg->num;
+
+	/* scale configuration */
+	addr = LP8788_ISINK_CTRL;
+	mask = 1 << (cfg->num + LP8788_ISINK_SCALE_OFFSET);
+	val = cfg->scale << cfg->num;
+	ret = lp8788_update_bits(led->lp, addr, mask, val);
+	if (ret)
+		return ret;
+
+	/* current configuration */
+	addr = lp8788_iout_addr[cfg->num];
+	mask = lp8788_iout_mask[cfg->num];
+	val = cfg->iout;
+
+	return lp8788_update_bits(led->lp, addr, mask, val);
+}
+
+static void lp8788_led_enable(struct lp8788_led *led,
+			enum lp8788_isink_number num, int on)
+{
+	u8 mask = 1 << num;
+	u8 val = on << num;
+
+	if (lp8788_update_bits(led->lp, LP8788_ISINK_CTRL, mask, val))
+		return;
+
+	led->on = on;
+}
+
+static void lp8788_led_work(struct work_struct *work)
+{
+	struct lp8788_led *led = container_of(work, struct lp8788_led, work);
+	enum lp8788_isink_number num = led->isink_num;
+	int enable;
+	u8 val = led->brightness;
+
+	mutex_lock(&led->lock);
+
+	switch (num) {
+	case LP8788_ISINK_1:
+	case LP8788_ISINK_2:
+	case LP8788_ISINK_3:
+		lp8788_write_byte(led->lp, lp8788_pwm_addr[num], val);
+		break;
+	default:
+		return;
+	}
+
+	enable = (val > 0) ? 1 : 0;
+	if (enable != led->on)
+		lp8788_led_enable(led, num, enable);
+
+	mutex_unlock(&led->lock);
+}
+
+static void lp8788_brightness_set(struct led_classdev *led_cdev,
+				enum led_brightness brt_val)
+{
+	struct lp8788_led *led =
+			container_of(led_cdev, struct lp8788_led, led_dev);
+
+	led->brightness = brt_val;
+	schedule_work(&led->work);
+}
+
+static __devinit int lp8788_led_probe(struct platform_device *pdev)
+{
+	struct lp8788 *lp = dev_get_drvdata(pdev->dev.parent);
+	struct lp8788_led_platform_data *led_pdata;
+	struct lp8788_led *led;
+	int ret;
+
+	led = devm_kzalloc(lp->dev, sizeof(struct lp8788_led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	led->lp = lp;
+	led->led_dev.max_brightness = MAX_BRIGHTNESS;
+	led->led_dev.brightness_set = lp8788_brightness_set;
+
+	led_pdata = lp->pdata ? lp->pdata->led_pdata : NULL;
+
+	if (!led_pdata || !led_pdata->name)
+		led->led_dev.name = DEFAULT_LED_NAME;
+	else
+		led->led_dev.name = led_pdata->name;
+
+	mutex_init(&led->lock);
+	INIT_WORK(&led->work, lp8788_led_work);
+
+	platform_set_drvdata(pdev, led);
+
+	ret = lp8788_led_init_device(led, led_pdata);
+	if (ret) {
+		dev_err(lp->dev, "led init device err: %d\n", ret);
+		return ret;
+	}
+
+	ret = led_classdev_register(lp->dev, &led->led_dev);
+	if (ret) {
+		dev_err(lp->dev, "led register err: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __devexit lp8788_led_remove(struct platform_device *pdev)
+{
+	struct lp8788_led *led = platform_get_drvdata(pdev);
+
+	led_classdev_unregister(&led->led_dev);
+	flush_work_sync(&led->work);
+
+	return 0;
+}
+
+static struct platform_driver lp8788_led_driver = {
+	.probe = lp8788_led_probe,
+	.remove = __devexit_p(lp8788_led_remove),
+	.driver = {
+		.name = LP8788_DEV_KEYLED,
+		.owner = THIS_MODULE,
+	},
+};
+module_platform_driver(lp8788_led_driver);
+
+MODULE_DESCRIPTION("Texas Instruments LP8788 Keyboard LED Driver");
+MODULE_AUTHOR("Milo Kim");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:lp8788-keyled");
-- 
1.7.2.5


Best Regards,
Milo

Re: [PATCH v2] leds: add new lp8788 led driver

[Shuah Khan]

On Fri, 2012-07-20 at 08:43 +0000, Kim, Milo wrote:
> TI LP8788 PMU has the current sink as the keyboard led driver.
> The brightness is controlled by the i2c commands.
> Configurable parameters can be defined in the platform side.
> 
> Patch v2.
> (a) use workqueue on changing the brightness
> 
> (b) use mutex_lock/unlock when the brightness is set
>     and the led block of lp8788 device is enabled
> 
> (c) remove err_dev on _probe()
>     : just return as returned value if any errors
> 
> (d) replace module_init/exit() with module_platform_driver()
> 
> (e) add led configuration structure and loading them by default
>     if platform data is null
> 
> Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
> ---
>  drivers/leds/Kconfig       |    7 ++
>  drivers/leds/Makefile      |    1 +
>  drivers/leds/leds-lp8788.c |  192 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 200 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/leds/leds-lp8788.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index f028f03..a498deb 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -200,6 +200,13 @@ config LEDS_LP5523
>  	  Driver provides direct control via LED class and interface for
>  	  programming the engines.
>  
> +config LEDS_LP8788
> +	tristate "LED support for the TI LP8788 PMIC"
> +	depends on LEDS_CLASS
> +	depends on MFD_LP8788
> +	help
> +	  This option enables support for the Keyboard LEDs on the LP8788 PMIC.
> +
>  config LEDS_CLEVO_MAIL
>  	tristate "Mail LED on Clevo notebook"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 5eebd7b..f156193 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_LEDS_GPIO)			+= leds-gpio.o
>  obj-$(CONFIG_LEDS_LP3944)		+= leds-lp3944.o
>  obj-$(CONFIG_LEDS_LP5521)		+= leds-lp5521.o
>  obj-$(CONFIG_LEDS_LP5523)		+= leds-lp5523.o
> +obj-$(CONFIG_LEDS_LP8788)		+= leds-lp8788.o
>  obj-$(CONFIG_LEDS_TCA6507)		+= leds-tca6507.o
>  obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
>  obj-$(CONFIG_LEDS_HP6XX)		+= leds-hp6xx.o
> diff --git a/drivers/leds/leds-lp8788.c b/drivers/leds/leds-lp8788.c
> new file mode 100644
> index 0000000..574b49f
> --- /dev/null
> +++ b/drivers/leds/leds-lp8788.c
> @@ -0,0 +1,192 @@
> +/*
> + * TI LP8788 MFD - keyled driver
> + *
> + * Copyright 2012 Texas Instruments
> + *
> + * Author: Milo(Woogyom) Kim <milo.kim@ti.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/slab.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/leds.h>
> +#include <linux/mutex.h>
> +#include <linux/mfd/lp8788.h>
> +#include <linux/mfd/lp8788-isink.h>
> +
> +#define MAX_BRIGHTNESS			LP8788_ISINK_MAX_PWM
> +#define DEFAULT_LED_NAME		"keyboard-backlight"
> +
> +struct lp8788_led {
> +	struct lp8788 *lp;
> +	struct mutex lock;
> +	struct work_struct work;
> +	struct led_classdev led_dev;
> +	enum lp8788_isink_number isink_num;
> +	enum led_brightness brightness;
> +	int on;
> +};
> +
> +struct lp8788_led_config {
> +	enum lp8788_isink_scale scale;
> +	enum lp8788_isink_number num;
> +	int iout;
> +};
> +
> +static struct lp8788_led_config default_led_config = {
> +	.scale = LP8788_ISINK_SCALE_100mA,
> +	.num   = LP8788_ISINK_3,
> +	.iout  = 0,
> +};
> +
> +static int lp8788_led_init_device(struct lp8788_led *led,
> +				struct lp8788_led_platform_data *pdata)
> +{
> +	struct lp8788_led_config *cfg = &default_led_config;
> +	u8 addr, mask, val;
> +	int ret;
> +
> +	if (pdata) {
> +		cfg->scale = pdata->scale;
> +		cfg->num = pdata->num;
> +		cfg->iout = pdata->iout_code;
> +	}
> +
> +	led->isink_num = cfg->num;
> +
> +	/* scale configuration */
> +	addr = LP8788_ISINK_CTRL;
> +	mask = 1 << (cfg->num + LP8788_ISINK_SCALE_OFFSET);
> +	val = cfg->scale << cfg->num;
> +	ret = lp8788_update_bits(led->lp, addr, mask, val);
> +	if (ret)
> +		return ret;
> +
> +	/* current configuration */
> +	addr = lp8788_iout_addr[cfg->num];
> +	mask = lp8788_iout_mask[cfg->num];
> +	val = cfg->iout;
> +
> +	return lp8788_update_bits(led->lp, addr, mask, val);
> +}
> +
> +static void lp8788_led_enable(struct lp8788_led *led,
> +			enum lp8788_isink_number num, int on)
> +{
> +	u8 mask = 1 << num;
> +	u8 val = on << num;
> +
> +	if (lp8788_update_bits(led->lp, LP8788_ISINK_CTRL, mask, val))
> +		return;
> +
> +	led->on = on;
> +}
> +
> +static void lp8788_led_work(struct work_struct *work)
> +{
> +	struct lp8788_led *led = container_of(work, struct lp8788_led, work);
> +	enum lp8788_isink_number num = led->isink_num;
> +	int enable;
> +	u8 val = led->brightness;
> +
> +	mutex_lock(&led->lock);
> +
> +	switch (num) {
> +	case LP8788_ISINK_1:
> +	case LP8788_ISINK_2:
> +	case LP8788_ISINK_3:
> +		lp8788_write_byte(led->lp, lp8788_pwm_addr[num], val);
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	enable = (val > 0) ? 1 : 0;
> +	if (enable != led->on)
> +		lp8788_led_enable(led, num, enable);
> +
> +	mutex_unlock(&led->lock);
> +}
> +
> +static void lp8788_brightness_set(struct led_classdev *led_cdev,
> +				enum led_brightness brt_val)
> +{
> +	struct lp8788_led *led =
> +			container_of(led_cdev, struct lp8788_led, led_dev);
> +
> +	led->brightness = brt_val;
> +	schedule_work(&led->work);
> +}
> +
> +static __devinit int lp8788_led_probe(struct platform_device *pdev)
> +{
> +	struct lp8788 *lp = dev_get_drvdata(pdev->dev.parent);
> +	struct lp8788_led_platform_data *led_pdata;
> +	struct lp8788_led *led;
> +	int ret;
> +
> +	led = devm_kzalloc(lp->dev, sizeof(struct lp8788_led), GFP_KERNEL);
> +	if (!led)
> +		return -ENOMEM;
> +
> +	led->lp = lp;
> +	led->led_dev.max_brightness = MAX_BRIGHTNESS;
> +	led->led_dev.brightness_set = lp8788_brightness_set;
> +
> +	led_pdata = lp->pdata ? lp->pdata->led_pdata : NULL;
> +
> +	if (!led_pdata || !led_pdata->name)
> +		led->led_dev.name = DEFAULT_LED_NAME;
> +	else
> +		led->led_dev.name = led_pdata->name;
> +
> +	mutex_init(&led->lock);
> +	INIT_WORK(&led->work, lp8788_led_work);
> +
> +	platform_set_drvdata(pdev, led);
> +
> +	ret = lp8788_led_init_device(led, led_pdata);
> +	if (ret) {
> +		dev_err(lp->dev, "led init device err: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = led_classdev_register(lp->dev, &led->led_dev);
> +	if (ret) {
> +		dev_err(lp->dev, "led register err: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __devexit lp8788_led_remove(struct platform_device *pdev)
> +{
> +	struct lp8788_led *led = platform_get_drvdata(pdev);
> +
> +	led_classdev_unregister(&led->led_dev);
> +	flush_work_sync(&led->work);

Is flush_work_sync() sufficient or cancel_work_sync() is a better
choice. I see some led drivers using cancel_work_sync() and some using
flush_work_sync(). I do see that flush_work_sync() doesn't do any
del_timer_sync() calls to cancel timers if any running.

> +
> +	return 0;
> +}
> +
> +static struct platform_driver lp8788_led_driver = {
> +	.probe = lp8788_led_probe,
> +	.remove = __devexit_p(lp8788_led_remove),
> +	.driver = {
> +		.name = LP8788_DEV_KEYLED,
> +		.owner = THIS_MODULE,
> +	},
> +};
> +module_platform_driver(lp8788_led_driver);
> +
> +MODULE_DESCRIPTION("Texas Instruments LP8788 Keyboard LED Driver");
> +MODULE_AUTHOR("Milo Kim");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:lp8788-keyled");

Re: [PATCH v2] leds: add new lp8788 led driver

[devendra.aaru]

Hi,

I think a mutex_unlock is missed out,

On Fri, Jul 20, 2012 at 2:28 PM, Kim, Milo <Milo.Kim@ti.com> wrote:
> +
> +static void lp8788_led_work(struct work_struct *work)
> +{
> +       struct lp8788_led *led = container_of(work, struct lp8788_led, work);
> +       enum lp8788_isink_number num = led->isink_num;
> +       int enable;
> +       u8 val = led->brightness;
> +
> +       mutex_lock(&led->lock);
> +
> +       switch (num) {
> +       case LP8788_ISINK_1:
> +       case LP8788_ISINK_2:
> +       case LP8788_ISINK_3:
> +               lp8788_write_byte(led->lp, lp8788_pwm_addr[num], val);
> +               break;
> +       default:
                    missed mutex_unlock
> +               return;
> +       }
> +
> +       enable = (val > 0) ? 1 : 0;
> +       if (enable != led->on)
> +               lp8788_led_enable(led, num, enable);
> +
> +       mutex_unlock(&led->lock);
> +}
> +

Thanks,
Devendra

Re: [PATCH v2] leds: add new lp8788 led driver

[Bryan Wu]

On Fri, Jul 20, 2012 at 11:49 PM, Shuah Khan <shuahkhan@gmail.com> wrote:
> On Fri, 2012-07-20 at 08:43 +0000, Kim, Milo wrote:
>> TI LP8788 PMU has the current sink as the keyboard led driver.
>> The brightness is controlled by the i2c commands.
>> Configurable parameters can be defined in the platform side.
>>
>> Patch v2.
>> (a) use workqueue on changing the brightness
>>
>> (b) use mutex_lock/unlock when the brightness is set
>>     and the led block of lp8788 device is enabled
>>
>> (c) remove err_dev on _probe()
>>     : just return as returned value if any errors
>>
>> (d) replace module_init/exit() with module_platform_driver()
>>
>> (e) add led configuration structure and loading them by default
>>     if platform data is null
>>
>> Signed-off-by: Milo(Woogyom) Kim <milo.kim@ti.com>
>> ---
>>  drivers/leds/Kconfig       |    7 ++
>>  drivers/leds/Makefile      |    1 +
>>  drivers/leds/leds-lp8788.c |  192 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 200 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/leds/leds-lp8788.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index f028f03..a498deb 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -200,6 +200,13 @@ config LEDS_LP5523
>>         Driver provides direct control via LED class and interface for
>>         programming the engines.
>>
>> +config LEDS_LP8788
>> +     tristate "LED support for the TI LP8788 PMIC"
>> +     depends on LEDS_CLASS
>> +     depends on MFD_LP8788
>> +     help
>> +       This option enables support for the Keyboard LEDs on the LP8788 PMIC.
>> +
>>  config LEDS_CLEVO_MAIL
>>       tristate "Mail LED on Clevo notebook"
>>       depends on LEDS_CLASS
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index 5eebd7b..f156193 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -24,6 +24,7 @@ obj-$(CONFIG_LEDS_GPIO)                     += leds-gpio.o
>>  obj-$(CONFIG_LEDS_LP3944)            += leds-lp3944.o
>>  obj-$(CONFIG_LEDS_LP5521)            += leds-lp5521.o
>>  obj-$(CONFIG_LEDS_LP5523)            += leds-lp5523.o
>> +obj-$(CONFIG_LEDS_LP8788)            += leds-lp8788.o
>>  obj-$(CONFIG_LEDS_TCA6507)           += leds-tca6507.o
>>  obj-$(CONFIG_LEDS_CLEVO_MAIL)                += leds-clevo-mail.o
>>  obj-$(CONFIG_LEDS_HP6XX)             += leds-hp6xx.o
>> diff --git a/drivers/leds/leds-lp8788.c b/drivers/leds/leds-lp8788.c
>> new file mode 100644
>> index 0000000..574b49f
>> --- /dev/null
>> +++ b/drivers/leds/leds-lp8788.c
>> @@ -0,0 +1,192 @@
>> +/*
>> + * TI LP8788 MFD - keyled driver
>> + *
>> + * Copyright 2012 Texas Instruments
>> + *
>> + * Author: Milo(Woogyom) Kim <milo.kim@ti.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/slab.h>
>> +#include <linux/err.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/leds.h>
>> +#include <linux/mutex.h>
>> +#include <linux/mfd/lp8788.h>
>> +#include <linux/mfd/lp8788-isink.h>
>> +
>> +#define MAX_BRIGHTNESS                       LP8788_ISINK_MAX_PWM
>> +#define DEFAULT_LED_NAME             "keyboard-backlight"
>> +
>> +struct lp8788_led {
>> +     struct lp8788 *lp;
>> +     struct mutex lock;
>> +     struct work_struct work;
>> +     struct led_classdev led_dev;
>> +     enum lp8788_isink_number isink_num;
>> +     enum led_brightness brightness;
>> +     int on;
>> +};
>> +
>> +struct lp8788_led_config {
>> +     enum lp8788_isink_scale scale;
>> +     enum lp8788_isink_number num;
>> +     int iout;
>> +};
>> +
>> +static struct lp8788_led_config default_led_config = {
>> +     .scale = LP8788_ISINK_SCALE_100mA,
>> +     .num   = LP8788_ISINK_3,
>> +     .iout  = 0,
>> +};
>> +
>> +static int lp8788_led_init_device(struct lp8788_led *led,
>> +                             struct lp8788_led_platform_data *pdata)
>> +{
>> +     struct lp8788_led_config *cfg = &default_led_config;
>> +     u8 addr, mask, val;
>> +     int ret;
>> +
>> +     if (pdata) {
>> +             cfg->scale = pdata->scale;
>> +             cfg->num = pdata->num;
>> +             cfg->iout = pdata->iout_code;
>> +     }
>> +
>> +     led->isink_num = cfg->num;
>> +
>> +     /* scale configuration */
>> +     addr = LP8788_ISINK_CTRL;
>> +     mask = 1 << (cfg->num + LP8788_ISINK_SCALE_OFFSET);
>> +     val = cfg->scale << cfg->num;
>> +     ret = lp8788_update_bits(led->lp, addr, mask, val);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /* current configuration */
>> +     addr = lp8788_iout_addr[cfg->num];
>> +     mask = lp8788_iout_mask[cfg->num];
>> +     val = cfg->iout;
>> +
>> +     return lp8788_update_bits(led->lp, addr, mask, val);
>> +}
>> +
>> +static void lp8788_led_enable(struct lp8788_led *led,
>> +                     enum lp8788_isink_number num, int on)
>> +{
>> +     u8 mask = 1 << num;
>> +     u8 val = on << num;
>> +
>> +     if (lp8788_update_bits(led->lp, LP8788_ISINK_CTRL, mask, val))
>> +             return;
>> +
>> +     led->on = on;
>> +}
>> +
>> +static void lp8788_led_work(struct work_struct *work)
>> +{
>> +     struct lp8788_led *led = container_of(work, struct lp8788_led, work);
>> +     enum lp8788_isink_number num = led->isink_num;
>> +     int enable;
>> +     u8 val = led->brightness;
>> +
>> +     mutex_lock(&led->lock);
>> +
>> +     switch (num) {
>> +     case LP8788_ISINK_1:
>> +     case LP8788_ISINK_2:
>> +     case LP8788_ISINK_3:
>> +             lp8788_write_byte(led->lp, lp8788_pwm_addr[num], val);
>> +             break;
>> +     default:
>> +             return;
>> +     }
>> +
>> +     enable = (val > 0) ? 1 : 0;
>> +     if (enable != led->on)
>> +             lp8788_led_enable(led, num, enable);
>> +
>> +     mutex_unlock(&led->lock);
>> +}
>> +
>> +static void lp8788_brightness_set(struct led_classdev *led_cdev,
>> +                             enum led_brightness brt_val)
>> +{
>> +     struct lp8788_led *led =
>> +                     container_of(led_cdev, struct lp8788_led, led_dev);
>> +
>> +     led->brightness = brt_val;
>> +     schedule_work(&led->work);
>> +}
>> +
>> +static __devinit int lp8788_led_probe(struct platform_device *pdev)
>> +{
>> +     struct lp8788 *lp = dev_get_drvdata(pdev->dev.parent);
>> +     struct lp8788_led_platform_data *led_pdata;
>> +     struct lp8788_led *led;
>> +     int ret;
>> +
>> +     led = devm_kzalloc(lp->dev, sizeof(struct lp8788_led), GFP_KERNEL);
>> +     if (!led)
>> +             return -ENOMEM;
>> +
>> +     led->lp = lp;
>> +     led->led_dev.max_brightness = MAX_BRIGHTNESS;
>> +     led->led_dev.brightness_set = lp8788_brightness_set;
>> +
>> +     led_pdata = lp->pdata ? lp->pdata->led_pdata : NULL;
>> +
>> +     if (!led_pdata || !led_pdata->name)
>> +             led->led_dev.name = DEFAULT_LED_NAME;
>> +     else
>> +             led->led_dev.name = led_pdata->name;
>> +
>> +     mutex_init(&led->lock);
>> +     INIT_WORK(&led->work, lp8788_led_work);
>> +
>> +     platform_set_drvdata(pdev, led);
>> +
>> +     ret = lp8788_led_init_device(led, led_pdata);
>> +     if (ret) {
>> +             dev_err(lp->dev, "led init device err: %d\n", ret);
>> +             return ret;
>> +     }
>> +
>> +     ret = led_classdev_register(lp->dev, &led->led_dev);
>> +     if (ret) {
>> +             dev_err(lp->dev, "led register err: %d\n", ret);
>> +             return ret;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int __devexit lp8788_led_remove(struct platform_device *pdev)
>> +{
>> +     struct lp8788_led *led = platform_get_drvdata(pdev);
>> +
>> +     led_classdev_unregister(&led->led_dev);
>> +     flush_work_sync(&led->work);
>
> Is flush_work_sync() sufficient or cancel_work_sync() is a better
> choice. I see some led drivers using cancel_work_sync() and some using
> flush_work_sync(). I do see that flush_work_sync() doesn't do any
> del_timer_sync() calls to cancel timers if any running.
>

Actually cancel_work_sync() is quite similar to flush_work_sync()
here. For the timer thing, in fact it is NULL when cancel_work_sync()
call __cancel_work_timer().

And Mark, do you have any advice about the flush_work_sync() and
cancel_work_sync(). I saw you use flush in the
drivers/leds/leds-wm8350.c.

Thanks a lot,
-Bryan

>> +
>> +     return 0;
>> +}
>> +
>> +static struct platform_driver lp8788_led_driver = {
>> +     .probe = lp8788_led_probe,
>> +     .remove = __devexit_p(lp8788_led_remove),
>> +     .driver = {
>> +             .name = LP8788_DEV_KEYLED,
>> +             .owner = THIS_MODULE,
>> +     },
>> +};
>> +module_platform_driver(lp8788_led_driver);
>> +
>> +MODULE_DESCRIPTION("Texas Instruments LP8788 Keyboard LED Driver");
>> +MODULE_AUTHOR("Milo Kim");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:lp8788-keyled");
>
>



-- 
Bryan Wu <bryan.wu@canonical.com>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com

Re: [PATCH v2] leds: add new lp8788 led driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

On Sat, Jul 21, 2012 at 02:48:49AM +0800, Bryan Wu wrote:

> Actually cancel_work_sync() is quite similar to flush_work_sync()
> here. For the timer thing, in fact it is NULL when cancel_work_sync()
> call __cancel_work_timer().

> And Mark, do you have any advice about the flush_work_sync() and
> cancel_work_sync(). I saw you use flush in the
> drivers/leds/leds-wm8350.c.

If the work is flushed then the state that userspace thought was set
when the driver is removed will actually be set before the driver is
removed.  This is fairly minor but might be useful.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] leds: add new lp8788 led driver

[Bryan Wu]

On Mon, Jul 23, 2012 at 2:19 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Sat, Jul 21, 2012 at 02:48:49AM +0800, Bryan Wu wrote:
>
>> Actually cancel_work_sync() is quite similar to flush_work_sync()
>> here. For the timer thing, in fact it is NULL when cancel_work_sync()
>> call __cancel_work_timer().
>
>> And Mark, do you have any advice about the flush_work_sync() and
>> cancel_work_sync(). I saw you use flush in the
>> drivers/leds/leds-wm8350.c.
>
> If the work is flushed then the state that userspace thought was set
> when the driver is removed will actually be set before the driver is
> removed.  This is fairly minor but might be useful.

So what's kind of state you mentioned here that is cared by user
space. I find these 2 functions are quite confused for use right now.
Literally, canceling normally will remove pending work item and wait
for running work item to finish. flushing will wait for both pending
and running work item to finish.

Thanks,
-- 
Bryan Wu <bryan.wu@canonical.com>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com

Re: [PATCH v2] leds: add new lp8788 led driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

On Tue, Jul 24, 2012 at 08:23:00AM +0800, Bryan Wu wrote:
> On Mon, Jul 23, 2012 at 2:19 AM, Mark Brown

> > If the work is flushed then the state that userspace thought was set
> > when the driver is removed will actually be set before the driver is
> > removed.  This is fairly minor but might be useful.

> So what's kind of state you mentioned here that is cared by user
> space. I find these 2 functions are quite confused for use right now.

Any state - none of the drivers with sleeping I/O can do anything
directly in their callbacks so they defer everything to work (we really
should have that in the core but it was too annoying to implement last
time I looked).

> Literally, canceling normally will remove pending work item and wait
> for running work item to finish. flushing will wait for both pending
> and running work item to finish.

Right, so if we flush it means we know that any scheduled work actually
ran and implemented whatever change was requested.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] leds: add new lp8788 led driver

[Bryan Wu]

On Tue, Jul 24, 2012 at 8:55 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Jul 24, 2012 at 08:23:00AM +0800, Bryan Wu wrote:
>> On Mon, Jul 23, 2012 at 2:19 AM, Mark Brown
>
>> > If the work is flushed then the state that userspace thought was set
>> > when the driver is removed will actually be set before the driver is
>> > removed.  This is fairly minor but might be useful.
>
>> So what's kind of state you mentioned here that is cared by user
>> space. I find these 2 functions are quite confused for use right now.
>
> Any state - none of the drivers with sleeping I/O can do anything
> directly in their callbacks so they defer everything to work (we really
> should have that in the core but it was too annoying to implement last
> time I looked).
>
>> Literally, canceling normally will remove pending work item and wait
>> for running work item to finish. flushing will wait for both pending
>> and running work item to finish.
>
> Right, so if we flush it means we know that any scheduled work actually
> ran and implemented whatever change was requested.

Thanks Mark for clarifying this.

I'm going to Ack this driver and Mark will you merge this as whole patchset?

Acked-by: Bryan Wu <bryan.wu@canonical.com>

Thanks,
-Bryan

Re: [PATCH v2] leds: add new lp8788 led driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]

On Wed, Jul 25, 2012 at 12:46:56PM +0800, Bryan Wu wrote:

> I'm going to Ack this driver and Mark will you merge this as whole patchset?

> Acked-by: Bryan Wu <bryan.wu@canonical.com>

It's an MFD so Samuel would normally apply if it were going via the MFD
tree, though if the dependencies are correct there should be no harm
merging via LEDs (until the core is merged it won't be possible to
enable the build in Kconfig).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] leds: add new lp8788 led driver

[Bryan Wu]

On Thu, Jul 26, 2012 at 2:43 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Jul 25, 2012 at 12:46:56PM +0800, Bryan Wu wrote:
>
>> I'm going to Ack this driver and Mark will you merge this as whole patchset?
>
>> Acked-by: Bryan Wu <bryan.wu@canonical.com>
>
> It's an MFD so Samuel would normally apply if it were going via the MFD
> tree, though if the dependencies are correct there should be no harm
> merging via LEDs (until the core is merged it won't be possible to
> enable the build in Kconfig).

OK, thanks, I've merge this patch through my tree and can I add your ack to it?

-- 
Bryan Wu <bryan.wu@canonical.com>
Kernel Developer    +86.186-168-78255 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com

Re: [PATCH v2] leds: add new lp8788 led driver

[Mark Brown]

On Thu, Jul 26, 2012 at 10:51:18AM +0800, Bryan Wu wrote:

> OK, thanks, I've merge this patch through my tree and can I add your ack to it?

Sure,

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>