[PATCH 0/3] lp5521

[PATCH 0/3] lp5521

[Felipe Balbi]

A few cleanups and moving to drivers/leds. This driver
is probably ok for making its way to mainline, by the morning
I'll generate a patch against mainline and send to Richard
(led maintainer) for integration.

*** Note: compile tested only due to the fact n810 doesn't boot.

Felipe Balbi (3):
  i2c: lp5521: remove dead code
  i2c: lp5521: cosmetic fixes
  lp5521: move to drivers/leds

 drivers/i2c/chips/Kconfig                          |    7 -
 drivers/i2c/chips/Makefile                         |    1 -
 drivers/leds/Kconfig                               |    7 +
 drivers/leds/Makefile                              |    1 +
 drivers/{i2c/chips/lp5521.c => leds/leds-lp5521.c} |  181 ++++++++------------
 5 files changed, 81 insertions(+), 116 deletions(-)
 rename drivers/{i2c/chips/lp5521.c => leds/leds-lp5521.c} (79%)

[PATCH 1/3] i2c: lp5521: remove dead code

[Felipe Balbi]

That LED_CONNECTED_WRONG was never defined so removing.
If someone needs those hooks, add back via proper
platform_data instead of nasty ifdefery.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/i2c/chips/lp5521.c |   22 ----------------------
 1 files changed, 0 insertions(+), 22 deletions(-)

diff --git a/drivers/i2c/chips/lp5521.c b/drivers/i2c/chips/lp5521.c
index c0862d9..7fb8091 100644
--- a/drivers/i2c/chips/lp5521.c
+++ b/drivers/i2c/chips/lp5521.c
@@ -28,13 +28,8 @@
 
 #define LP5521_DRIVER_NAME		"lp5521"
 
-#ifdef LED_CONNECTED_WRONG
-#define LP5521_REG_R_PWM		0x04
-#define LP5521_REG_B_PWM		0x02
-#else
 #define LP5521_REG_R_PWM		0x02
 #define LP5521_REG_B_PWM		0x04
-#endif
 #define LP5521_REG_ENABLE		0x00
 #define LP5521_REG_OP_MODE		0x01
 #define LP5521_REG_G_PWM		0x03
@@ -200,22 +195,12 @@ static ssize_t show_active_channels(struct device *dev,
 	char channels[4];
 	int pos = 0;
 
-#ifdef LED_CONNECTED_WRONG
-	if (chip->blue)
-		pos += sprintf(channels + pos, "r");
-	if (chip->green)
-		pos += sprintf(channels + pos, "g");
-	if (chip->red)
-		pos += sprintf(channels + pos, "b");
-
-#else
 	if (chip->red)
 		pos += sprintf(channels + pos, "r");
 	if (chip->green)
 		pos += sprintf(channels + pos, "g");
 	if (chip->blue)
 		pos += sprintf(channels + pos, "b");
-#endif
 
 	channels[pos] = '\0';
 
@@ -232,17 +217,10 @@ static ssize_t store_active_channels(struct device *dev,
 	chip->green = 0;
 	chip->blue = 0;
 
-#ifdef LED_CONNECTED_WRONG
-	if (strchr(buf, 'r') != NULL)
-		chip->blue = 1;
-	if (strchr(buf, 'b') != NULL)
-		chip->red = 1;
-#else
 	if (strchr(buf, 'r') != NULL)
 		chip->red = 1;
 	if (strchr(buf, 'b') != NULL)
 		chip->blue = 1;
-#endif
 	if (strchr(buf, 'g') != NULL)
 		chip->green = 1;
 
-- 
1.6.0.2.307.gc427

[PATCH 2/3] i2c: lp5521: cosmetic fixes

[Felipe Balbi]

General cleanup to the code. Preparing to send it to
mainline.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/i2c/chips/lp5521.c |  159 ++++++++++++++++++++------------------------
 1 files changed, 73 insertions(+), 86 deletions(-)

diff --git a/drivers/i2c/chips/lp5521.c b/drivers/i2c/chips/lp5521.c
index 7fb8091..e040c4d 100644
--- a/drivers/i2c/chips/lp5521.c
+++ b/drivers/i2c/chips/lp5521.c
@@ -1,5 +1,5 @@
 /*
- * drivers/i2c/chips/lp5521.c
+ * lp5521.c - LP5521 LED Driver
  *
  * Copyright (C) 2007 Nokia Corporation
  *
@@ -24,7 +24,6 @@
 #include <linux/init.h>
 #include <linux/i2c.h>
 #include <linux/mutex.h>
-#include <mach/gpio.h>
 
 #define LP5521_DRIVER_NAME		"lp5521"
 
@@ -71,6 +70,7 @@
 #define LP5521_PROGRAM_LENGTH		32	/* in bytes */
 
 struct lp5521_chip {
+	/* device lock */
 	struct mutex		lock;
 	struct i2c_client	*client;
 	char			*mode;
@@ -81,20 +81,14 @@ struct lp5521_chip {
 
 static int lp5521_set_mode(struct lp5521_chip *chip, char *mode);
 
-static int lp5521_write(struct i2c_client *client, u8 reg, u8 value)
+static inline int lp5521_write(struct i2c_client *client, u8 reg, u8 value)
 {
 	return i2c_smbus_write_byte_data(client, reg, value);
 }
 
-static int lp5521_read(struct i2c_client *client, u8 reg, u8 *buf)
+static inline int lp5521_read(struct i2c_client *client, u8 reg)
 {
-	s32 ret = i2c_smbus_read_byte_data(client, reg);
-
-	if (ret < 0)
-		return -EIO;
-
-	*buf = ret;
-	return 0;
+	return i2c_smbus_read_byte_data(client, reg);
 }
 
 static int lp5521_configure(struct i2c_client *client)
@@ -136,19 +130,19 @@ static int lp5521_load_program(struct lp5521_chip *chip, u8 *pattern)
 
 	if (chip->red)
 		ret |= i2c_smbus_write_i2c_block_data(client,
-						      LP5521_REG_R_PROG_MEM,
-						      LP5521_PROGRAM_LENGTH,
-						      pattern);
+				LP5521_REG_R_PROG_MEM,
+				LP5521_PROGRAM_LENGTH,
+				pattern);
 	if (chip->green)
 		ret |= i2c_smbus_write_i2c_block_data(client,
-						      LP5521_REG_G_PROG_MEM,
-						      LP5521_PROGRAM_LENGTH,
-						      pattern);
+				LP5521_REG_G_PROG_MEM,
+				LP5521_PROGRAM_LENGTH,
+				pattern);
 	if (chip->blue)
 		ret |= i2c_smbus_write_i2c_block_data(client,
-						      LP5521_REG_B_PROG_MEM,
-						      LP5521_PROGRAM_LENGTH,
-						      pattern);
+				LP5521_REG_B_PROG_MEM,
+				LP5521_PROGRAM_LENGTH,
+				pattern);
 
 	return ret;
 }
@@ -156,31 +150,33 @@ static int lp5521_load_program(struct lp5521_chip *chip, u8 *pattern)
 static int lp5521_run_program(struct lp5521_chip *chip)
 {
 	struct i2c_client *client = chip->client;
-	int ret;
+	int reg;
 	u8 mask = 0xc0;
 	u8 exec_state = 0;
-	u8 enable_reg;
 
-	ret = lp5521_read(client, LP5521_REG_ENABLE, &enable_reg);
-	if (ret)
-		goto fail;
+	reg = lp5521_read(client, LP5521_REG_ENABLE);
+	if (reg < 0)
+		return reg;
 
-	enable_reg &= mask;
+	reg &= mask;
 
 	/* set all active channels exec state to countinous run*/
-	exec_state |= (chip->red   << 5);
+	exec_state |= (chip->red << 5);
 	exec_state |= (chip->green << 3);
-	exec_state |= (chip->blue  << 1);
+	exec_state |= (chip->blue << 1);
 
-	enable_reg |= exec_state;
+	reg |= exec_state;
 
-	ret |= lp5521_write(client, LP5521_REG_ENABLE, enable_reg);
+	if (lp5521_write(client, LP5521_REG_ENABLE, reg))
+		dev_dbg(&client->dev, "failed writing to register %02x\n",
+				LP5521_REG_ENABLE);
 
 	/* set op-mode to run for active channels, disabled for others */
-	ret |= lp5521_write(client, LP5521_REG_OP_MODE, exec_state);
+	if (lp5521_write(client, LP5521_REG_OP_MODE, exec_state))
+		dev_dbg(&client->dev, "failed writing to register %02x\n",
+				LP5521_REG_OP_MODE);
 
-fail:
-	return ret;
+	return 0;
 }
 
 /*--------------------------------------------------------------*/
@@ -188,8 +184,8 @@ fail:
 /*--------------------------------------------------------------*/
 
 static ssize_t show_active_channels(struct device *dev,
-			    struct device_attribute *attr,
-			    char *buf)
+		struct device_attribute *attr,
+		char *buf)
 {
 	struct lp5521_chip *chip = dev_get_drvdata(dev);
 	char channels[4];
@@ -208,8 +204,8 @@ static ssize_t show_active_channels(struct device *dev,
 }
 
 static ssize_t store_active_channels(struct device *dev,
-			     struct device_attribute *attr,
-			     const char *buf, size_t len)
+		struct device_attribute *attr,
+		const char *buf, size_t len)
 {
 	struct lp5521_chip *chip = dev_get_drvdata(dev);
 
@@ -228,26 +224,25 @@ static ssize_t store_active_channels(struct device *dev,
 }
 
 static ssize_t show_color(struct device *dev,
-			    struct device_attribute *attr,
-			    char *buf)
+		struct device_attribute *attr,
+		char *buf)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	int ret = 0;
-	u8 r, g, b;
+	int r, g, b;
 
-	ret |= lp5521_read(client, LP5521_REG_R_PWM, &r);
-	ret |= lp5521_read(client, LP5521_REG_G_PWM, &g);
-	ret |= lp5521_read(client, LP5521_REG_B_PWM, &b);
+	r = lp5521_read(client, LP5521_REG_R_PWM);
+	g = lp5521_read(client, LP5521_REG_G_PWM);
+	b = lp5521_read(client, LP5521_REG_B_PWM);
 
-	if (ret)
-		return ret;
+	if (r < 0 || g < 0 || b < 0)
+		return -EINVAL;
 
 	return sprintf(buf, "%.2x:%.2x:%.2x\n", r, g, b);
 }
 
 static ssize_t store_color(struct device *dev,
-			     struct device_attribute *attr,
-			     const char *buf, size_t len)
+		struct device_attribute *attr,
+		const char *buf, size_t len)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lp5521_chip *chip = i2c_get_clientdata(client);
@@ -271,8 +266,8 @@ static ssize_t store_color(struct device *dev,
 }
 
 static ssize_t store_load(struct device *dev,
-			     struct device_attribute *attr,
-			     const char *buf, size_t len)
+		struct device_attribute *attr,
+		const char *buf, size_t len)
 {
 	struct lp5521_chip *chip = dev_get_drvdata(dev);
 	int  ret, nrchars, offset = 0, i = 0;
@@ -314,8 +309,8 @@ fail:
 }
 
 static ssize_t show_mode(struct device *dev,
-			 struct device_attribute *attr,
-			 char *buf)
+		struct device_attribute *attr,
+		char *buf)
 {
 	struct lp5521_chip *chip = dev_get_drvdata(dev);
 
@@ -323,8 +318,8 @@ static ssize_t show_mode(struct device *dev,
 }
 
 static ssize_t store_mode(struct device *dev,
-			  struct device_attribute *attr,
-			  const char *buf, size_t len)
+		struct device_attribute *attr,
+		const char *buf, size_t len)
 {
 	struct lp5521_chip *chip = dev_get_drvdata(dev);
 
@@ -343,33 +338,29 @@ static ssize_t store_mode(struct device *dev,
 }
 
 static ssize_t show_current(struct device *dev,
-			    struct device_attribute *attr,
-			    char *buf)
+		struct device_attribute *attr,
+		char *buf)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	int ret = 0;
-	u8 r_curr, g_curr, b_curr;
+	int r, g, b;
 
-	ret |= lp5521_read(client, LP5521_REG_R_CNTRL, &r_curr);
-	ret |= lp5521_read(client, LP5521_REG_G_CNTRL, &g_curr);
-	ret |= lp5521_read(client, LP5521_REG_B_CNTRL, &b_curr);
+	r = lp5521_read(client, LP5521_REG_R_CNTRL);
+	g = lp5521_read(client, LP5521_REG_G_CNTRL);
+	b = lp5521_read(client, LP5521_REG_B_CNTRL);
 
-	if (ret)
-		return ret;
+	if (r < 0 || g < 0 || b < 0)
+		return -EINVAL;
 
-	r_curr = r_curr >> 4;
-	g_curr = g_curr >> 4;
-	b_curr = b_curr >> 4;
+	r >>= 4;
+	g >>= 4;
+	b >>= 4;
 
-	if (r_curr == g_curr && g_curr == b_curr)
-		return sprintf(buf, "%x\n", r_curr);
-	else
-		return sprintf(buf, "%x %x %x\n", r_curr, g_curr, b_curr);
+	return sprintf(buf, "%x %x %x\n", r, g, b);
 }
 
 static ssize_t store_current(struct device *dev,
-			     struct device_attribute *attr,
-			     const char *buf, size_t len)
+		struct device_attribute *attr,
+		const char *buf, size_t len)
 {
 	struct lp5521_chip *chip = dev_get_drvdata(dev);
 	struct i2c_client *client = chip->client;
@@ -398,7 +389,7 @@ static DEVICE_ATTR(color, S_IRUGO | S_IWUGO, show_color, store_color);
 static DEVICE_ATTR(load, S_IWUGO, NULL, store_load);
 static DEVICE_ATTR(mode, S_IRUGO | S_IWUGO, show_mode, store_mode);
 static DEVICE_ATTR(active_channels, S_IRUGO | S_IWUGO,
-		   show_active_channels, store_active_channels);
+		show_active_channels, store_active_channels);
 static DEVICE_ATTR(led_current, S_IRUGO | S_IWUGO, show_current, store_current);
 
 static int lp5521_register_sysfs(struct i2c_client *client)
@@ -421,6 +412,7 @@ static int lp5521_register_sysfs(struct i2c_client *client)
 	ret = device_create_file(dev, &dev_attr_led_current);
 	if (ret)
 		goto fail5;
+
 	return 0;
 
 fail5:
@@ -437,16 +429,13 @@ fail1:
 
 static void lp5521_unregister_sysfs(struct i2c_client *client)
 {
-	struct lp5521_chip *chip = i2c_get_clientdata(client);
 	struct device *dev = &client->dev;
 
 	device_remove_file(dev, &dev_attr_led_current);
 	device_remove_file(dev, &dev_attr_mode);
 	device_remove_file(dev, &dev_attr_active_channels);
 	device_remove_file(dev, &dev_attr_color);
-
-	if (!strcmp(chip->mode, LP5521_MODE_LOAD))
-		device_remove_file(dev, &dev_attr_load);
+	device_remove_file(dev, &dev_attr_load);
 }
 
 /*--------------------------------------------------------------*/
@@ -479,9 +468,8 @@ static int lp5521_set_mode(struct lp5521_chip *chip, char *mode)
 /*--------------------------------------------------------------*/
 /*			Probe, Attach, Remove			*/
 /*--------------------------------------------------------------*/
-static struct i2c_driver lp5521_driver;
 
-static int lp5521_probe(struct i2c_client *client,
+static int __init lp5521_probe(struct i2c_client *client,
 		const struct i2c_device_id *id)
 {
 	struct lp5521_chip *chip;
@@ -491,7 +479,7 @@ static int lp5521_probe(struct i2c_client *client,
 	if (!chip)
 		return -ENOMEM;
 
-	chip->client	= client;
+	chip->client = client;
 	strncpy(client->name, LP5521_DRIVER_NAME, I2C_NAME_SIZE);
 	i2c_set_clientdata(client, chip);
 
@@ -520,7 +508,7 @@ fail1:
 	return ret;
 }
 
-static int lp5521_remove(struct i2c_client *client)
+static int __exit lp5521_remove(struct i2c_client *client)
 {
 	struct lp5521_chip *chip = i2c_get_clientdata(client);
 
@@ -537,11 +525,11 @@ static const struct i2c_device_id lp5521_id[] = {
 MODULE_DEVICE_TABLE(i2c, lp5521_id);
 
 static struct i2c_driver lp5521_driver = {
-	.driver = {
+	.driver		= {
 		.name	= LP5521_DRIVER_NAME,
 	},
 	.probe		= lp5521_probe,
-	.remove		= __devexit_p(lp5521_remove),
+	.remove		= __exit_p(lp5521_remove),
 	.id_table	= lp5521_id,
 };
 
@@ -549,15 +537,14 @@ static int __init lp5521_init(void)
 {
 	return i2c_add_driver(&lp5521_driver);
 }
+module_init(lp5521_init);
 
 static void __exit lp5521_exit(void)
 {
 	i2c_del_driver(&lp5521_driver);
 }
+module_exit(lp5521_exit);
 
 MODULE_AUTHOR("Mathias Nyman <mathias.nyman@nokia.com>");
 MODULE_DESCRIPTION("lp5521 LED driver");
 MODULE_LICENSE("GPL");
-
-module_init(lp5521_init);
-module_exit(lp5521_exit);
-- 
1.6.0.2.307.gc427

[PATCH 3/3] lp5521: move to drivers/leds

[Felipe Balbi]

This driver should be sitting together with the other
led drivers.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/i2c/chips/Kconfig                          |    7 -------
 drivers/i2c/chips/Makefile                         |    1 -
 drivers/leds/Kconfig                               |    7 +++++++
 drivers/leds/Makefile                              |    1 +
 drivers/{i2c/chips/lp5521.c => leds/leds-lp5521.c} |    0
 5 files changed, 8 insertions(+), 8 deletions(-)
 rename drivers/{i2c/chips/lp5521.c => leds/leds-lp5521.c} (100%)

diff --git a/drivers/i2c/chips/Kconfig b/drivers/i2c/chips/Kconfig
index e6ac6a2..1b9bc11 100644
--- a/drivers/i2c/chips/Kconfig
+++ b/drivers/i2c/chips/Kconfig
@@ -225,13 +225,6 @@ config SENSORS_TSL2563
          This driver can also be built as a module.  If so, the module
          will be called tsl2563.
 
-config LP5521
-	tristate "LP5521 LED driver chip"
-	depends on I2C
-	help
-	  If you say yes here you get support for the National Semiconductor
-	  LP5521 LED driver.
-
 config MENELAUS
 	bool "TWL92330/Menelaus PM chip"
 	depends on I2C=y && ARCH_OMAP24XX
diff --git a/drivers/i2c/chips/Makefile b/drivers/i2c/chips/Makefile
index 75c531a..7523e58 100644
--- a/drivers/i2c/chips/Makefile
+++ b/drivers/i2c/chips/Makefile
@@ -30,7 +30,6 @@ obj-$(CONFIG_TWL4030_POWEROFF)	+= twl4030-poweroff.o
 obj-$(CONFIG_TWL4030_PWRBUTTON)	+= twl4030-pwrbutton.o
 obj-$(CONFIG_TWL4030_MADC)	+= twl4030-madc.o
 obj-$(CONFIG_RTC_X1205_I2C)	+= x1205.o
-obj-$(CONFIG_LP5521)		+= lp5521.o
 
 ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)
 EXTRA_CFLAGS += -DDEBUG
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 6a2f441..bd7c4dc 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -146,6 +146,13 @@ config LEDS_CM_X270
 	help
 	  This option enables support for the CM-X270 LEDs.
 
+config LEDS_LP5521
+	tristate "LED Support for LP5521 RGB LEDs"
+	depends on LEDS_CLASS && I2C
+	help
+	  If you say yes here you get support for the National Semiconductor
+	  LP5521 LED driver used in n8x0 boards.
+
 config LEDS_CLEVO_MAIL
 	tristate "Mail LED on Clevo notebook (EXPERIMENTAL)"
 	depends on LEDS_CLASS && X86 && SERIO_I8042 && DMI && EXPERIMENTAL
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index fd6316e..ce5c8cd 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_LEDS_COBALT_RAQ)		+= leds-cobalt-raq.o
 obj-$(CONFIG_LEDS_PCA9532)		+= leds-pca9532.o
 obj-$(CONFIG_LEDS_GPIO)			+= leds-gpio.o
 obj-$(CONFIG_LEDS_CM_X270)              += leds-cm-x270.o
+obj-$(CONFIG_LEDS_LP5521)		+= leds-lp5521.o
 obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
 obj-$(CONFIG_LEDS_HP6XX)		+= leds-hp6xx.o
 obj-$(CONFIG_LEDS_FSG)			+= leds-fsg.o
diff --git a/drivers/i2c/chips/lp5521.c b/drivers/leds/leds-lp5521.c
similarity index 100%
rename from drivers/i2c/chips/lp5521.c
rename to drivers/leds/leds-lp5521.c
-- 
1.6.0.2.307.gc427

Re: [PATCH 3/3] lp5521: move to drivers/leds

[Riku Voipio]

On Mon, Oct 06, 2008 at 12:11:31PM +0300, Felipe Balbi wrote:
> This driver should be sitting together with the other
> led drivers.

But this driver actually doesn't implement the led sysfs interface.
If the driver is changed to implement the led framework interface,
we break the existing n810 userland and expose only limited subset
of lp5521 features for userland..

> Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
> ---
>  drivers/i2c/chips/Kconfig                          |    7 -------
>  drivers/i2c/chips/Makefile                         |    1 -
>  drivers/leds/Kconfig                               |    7 +++++++
>  drivers/leds/Makefile                              |    1 +
>  drivers/{i2c/chips/lp5521.c => leds/leds-lp5521.c} |    0
>  5 files changed, 8 insertions(+), 8 deletions(-)
>  rename drivers/{i2c/chips/lp5521.c => leds/leds-lp5521.c} (100%)
> 
> diff --git a/drivers/i2c/chips/Kconfig b/drivers/i2c/chips/Kconfig
> index e6ac6a2..1b9bc11 100644
> --- a/drivers/i2c/chips/Kconfig
> +++ b/drivers/i2c/chips/Kconfig
> @@ -225,13 +225,6 @@ config SENSORS_TSL2563
>           This driver can also be built as a module.  If so, the module
>           will be called tsl2563.
>  
> -config LP5521
> -	tristate "LP5521 LED driver chip"
> -	depends on I2C
> -	help
> -	  If you say yes here you get support for the National Semiconductor
> -	  LP5521 LED driver.
> -
>  config MENELAUS
>  	bool "TWL92330/Menelaus PM chip"
>  	depends on I2C=y && ARCH_OMAP24XX
> diff --git a/drivers/i2c/chips/Makefile b/drivers/i2c/chips/Makefile
> index 75c531a..7523e58 100644
> --- a/drivers/i2c/chips/Makefile
> +++ b/drivers/i2c/chips/Makefile
> @@ -30,7 +30,6 @@ obj-$(CONFIG_TWL4030_POWEROFF)	+= twl4030-poweroff.o
>  obj-$(CONFIG_TWL4030_PWRBUTTON)	+= twl4030-pwrbutton.o
>  obj-$(CONFIG_TWL4030_MADC)	+= twl4030-madc.o
>  obj-$(CONFIG_RTC_X1205_I2C)	+= x1205.o
> -obj-$(CONFIG_LP5521)		+= lp5521.o
>  
>  ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)
>  EXTRA_CFLAGS += -DDEBUG
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 6a2f441..bd7c4dc 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -146,6 +146,13 @@ config LEDS_CM_X270
>  	help
>  	  This option enables support for the CM-X270 LEDs.
>  
> +config LEDS_LP5521
> +	tristate "LED Support for LP5521 RGB LEDs"
> +	depends on LEDS_CLASS && I2C
> +	help
> +	  If you say yes here you get support for the National Semiconductor
> +	  LP5521 LED driver used in n8x0 boards.
> +
>  config LEDS_CLEVO_MAIL
>  	tristate "Mail LED on Clevo notebook (EXPERIMENTAL)"
>  	depends on LEDS_CLASS && X86 && SERIO_I8042 && DMI && EXPERIMENTAL
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index fd6316e..ce5c8cd 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_LEDS_COBALT_RAQ)		+= leds-cobalt-raq.o
>  obj-$(CONFIG_LEDS_PCA9532)		+= leds-pca9532.o
>  obj-$(CONFIG_LEDS_GPIO)			+= leds-gpio.o
>  obj-$(CONFIG_LEDS_CM_X270)              += leds-cm-x270.o
> +obj-$(CONFIG_LEDS_LP5521)		+= leds-lp5521.o
>  obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
>  obj-$(CONFIG_LEDS_HP6XX)		+= leds-hp6xx.o
>  obj-$(CONFIG_LEDS_FSG)			+= leds-fsg.o
> diff --git a/drivers/i2c/chips/lp5521.c b/drivers/leds/leds-lp5521.c
> similarity index 100%
> rename from drivers/i2c/chips/lp5521.c
> rename to drivers/leds/leds-lp5521.c
> -- 
> 1.6.0.2.307.gc427
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
"rm -rf" only sounds scary if you don't have backups

Re: [PATCH 3/3] lp5521: move to drivers/leds

[Tony Lindgren]

* Riku Voipio <riku.voipio@iki.fi> [081006 14:09]:
> On Mon, Oct 06, 2008 at 12:11:31PM +0300, Felipe Balbi wrote:
> > This driver should be sitting together with the other
> > led drivers.
> 
> But this driver actually doesn't implement the led sysfs interface.
> If the driver is changed to implement the led framework interface,
> we break the existing n810 userland and expose only limited subset
> of lp5521 features for userland..

Maybe this feature should be a distro specific hack on top of
the mainline tree?

Tony

> > Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
> > ---
> >  drivers/i2c/chips/Kconfig                          |    7 -------
> >  drivers/i2c/chips/Makefile                         |    1 -
> >  drivers/leds/Kconfig                               |    7 +++++++
> >  drivers/leds/Makefile                              |    1 +
> >  drivers/{i2c/chips/lp5521.c => leds/leds-lp5521.c} |    0
> >  5 files changed, 8 insertions(+), 8 deletions(-)
> >  rename drivers/{i2c/chips/lp5521.c => leds/leds-lp5521.c} (100%)
> > 
> > diff --git a/drivers/i2c/chips/Kconfig b/drivers/i2c/chips/Kconfig
> > index e6ac6a2..1b9bc11 100644
> > --- a/drivers/i2c/chips/Kconfig
> > +++ b/drivers/i2c/chips/Kconfig
> > @@ -225,13 +225,6 @@ config SENSORS_TSL2563
> >           This driver can also be built as a module.  If so, the module
> >           will be called tsl2563.
> >  
> > -config LP5521
> > -	tristate "LP5521 LED driver chip"
> > -	depends on I2C
> > -	help
> > -	  If you say yes here you get support for the National Semiconductor
> > -	  LP5521 LED driver.
> > -
> >  config MENELAUS
> >  	bool "TWL92330/Menelaus PM chip"
> >  	depends on I2C=y && ARCH_OMAP24XX
> > diff --git a/drivers/i2c/chips/Makefile b/drivers/i2c/chips/Makefile
> > index 75c531a..7523e58 100644
> > --- a/drivers/i2c/chips/Makefile
> > +++ b/drivers/i2c/chips/Makefile
> > @@ -30,7 +30,6 @@ obj-$(CONFIG_TWL4030_POWEROFF)	+= twl4030-poweroff.o
> >  obj-$(CONFIG_TWL4030_PWRBUTTON)	+= twl4030-pwrbutton.o
> >  obj-$(CONFIG_TWL4030_MADC)	+= twl4030-madc.o
> >  obj-$(CONFIG_RTC_X1205_I2C)	+= x1205.o
> > -obj-$(CONFIG_LP5521)		+= lp5521.o
> >  
> >  ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)
> >  EXTRA_CFLAGS += -DDEBUG
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 6a2f441..bd7c4dc 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -146,6 +146,13 @@ config LEDS_CM_X270
> >  	help
> >  	  This option enables support for the CM-X270 LEDs.
> >  
> > +config LEDS_LP5521
> > +	tristate "LED Support for LP5521 RGB LEDs"
> > +	depends on LEDS_CLASS && I2C
> > +	help
> > +	  If you say yes here you get support for the National Semiconductor
> > +	  LP5521 LED driver used in n8x0 boards.
> > +
> >  config LEDS_CLEVO_MAIL
> >  	tristate "Mail LED on Clevo notebook (EXPERIMENTAL)"
> >  	depends on LEDS_CLASS && X86 && SERIO_I8042 && DMI && EXPERIMENTAL
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index fd6316e..ce5c8cd 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -21,6 +21,7 @@ obj-$(CONFIG_LEDS_COBALT_RAQ)		+= leds-cobalt-raq.o
> >  obj-$(CONFIG_LEDS_PCA9532)		+= leds-pca9532.o
> >  obj-$(CONFIG_LEDS_GPIO)			+= leds-gpio.o
> >  obj-$(CONFIG_LEDS_CM_X270)              += leds-cm-x270.o
> > +obj-$(CONFIG_LEDS_LP5521)		+= leds-lp5521.o
> >  obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
> >  obj-$(CONFIG_LEDS_HP6XX)		+= leds-hp6xx.o
> >  obj-$(CONFIG_LEDS_FSG)			+= leds-fsg.o
> > diff --git a/drivers/i2c/chips/lp5521.c b/drivers/leds/leds-lp5521.c
> > similarity index 100%
> > rename from drivers/i2c/chips/lp5521.c
> > rename to drivers/leds/leds-lp5521.c
> > -- 
> > 1.6.0.2.307.gc427
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> "rm -rf" only sounds scary if you don't have backups
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] lp5521: move to drivers/leds

[Felipe Balbi]

On Mon, Oct 06, 2008 at 02:17:23PM +0300, Tony Lindgren wrote:
> * Riku Voipio <riku.voipio@iki.fi> [081006 14:09]:
> > On Mon, Oct 06, 2008 at 12:11:31PM +0300, Felipe Balbi wrote:
> > > This driver should be sitting together with the other
> > > led drivers.
> > 
> > But this driver actually doesn't implement the led sysfs interface.
> > If the driver is changed to implement the led framework interface,
> > we break the existing n810 userland and expose only limited subset
> > of lp5521 features for userland..
> 
> Maybe this feature should be a distro specific hack on top of
> the mainline tree?

the problem is that led api (afaict) doesn't really have abstraction for
rgb leds like the one we use. It can only handle blinky leds and on/off
transitions and we need more functionality.

This driver will probably loop around for a while until we find out that
led api needs to be extended.

-- 
balbi

Re: [PATCH 3/3] lp5521: move to drivers/leds

[Riku Voipio]

On Mon, Oct 06, 2008 at 02:51:03PM +0300, Felipe Balbi wrote:
> On Mon, Oct 06, 2008 at 02:17:23PM +0300, Tony Lindgren wrote:
> > * Riku Voipio <riku.voipio@iki.fi> [081006 14:09]:
> > > On Mon, Oct 06, 2008 at 12:11:31PM +0300, Felipe Balbi wrote:
> > > > This driver should be sitting together with the other
> > > > led drivers.
> > > 
> > > But this driver actually doesn't implement the led sysfs interface.
> > > If the driver is changed to implement the led framework interface,
> > > we break the existing n810 userland and expose only limited subset
> > > of lp5521 features for userland..
> > 
> > Maybe this feature should be a distro specific hack on top of
> > the mainline tree?
> 
> the problem is that led api (afaict) doesn't really have abstraction for
> rgb leds like the one we use. It can only handle blinky leds and on/off
> transitions and we need more functionality.

RGB can somewhat be handled, by creating three led devices:

n810:red:foo
n810:gree:foo
n810:blue:foo

blinking could also be done, but that would be suboptimal. Mainly
we would lose all the cool fading etc stuff than can be programmed
to run on lp5521 without main cpu intervention..

> This driver will probably loop around for a while until we find out that
> led api needs to be extended.

Or implement a minimal led api (off/brightness) as well as keep the
custom interface for time being, if that's ok with Richard (the led
subsystem maintainer).

-- 
"rm -rf" only sounds scary if you don't have backups

Re: [PATCH 3/3] lp5521: move to drivers/leds

[Felipe Balbi]

On Mon, Oct 06, 2008 at 03:54:57PM +0300, ext Riku Voipio wrote:
> On Mon, Oct 06, 2008 at 02:51:03PM +0300, Felipe Balbi wrote:
> > On Mon, Oct 06, 2008 at 02:17:23PM +0300, Tony Lindgren wrote:
> > > * Riku Voipio <riku.voipio@iki.fi> [081006 14:09]:
> > > > On Mon, Oct 06, 2008 at 12:11:31PM +0300, Felipe Balbi wrote:
> > > > > This driver should be sitting together with the other
> > > > > led drivers.
> > > > 
> > > > But this driver actually doesn't implement the led sysfs interface.
> > > > If the driver is changed to implement the led framework interface,
> > > > we break the existing n810 userland and expose only limited subset
> > > > of lp5521 features for userland..
> > > 
> > > Maybe this feature should be a distro specific hack on top of
> > > the mainline tree?
> > 
> > the problem is that led api (afaict) doesn't really have abstraction for
> > rgb leds like the one we use. It can only handle blinky leds and on/off
> > transitions and we need more functionality.
> 
> RGB can somewhat be handled, by creating three led devices:
> 
> n810:red:foo
> n810:gree:foo
> n810:blue:foo

If that's the case I'd expect the driver author to do it since I have no
time left for that. A few fixes here and there to the driver is fine and
preparing the driver for mainline integration is also fine since it's a
simple cp iteration.

-- 
balbi

Re: [PATCH 3/3] lp5521: move to drivers/leds

[mathias nyman]

Felipe Balbi wrote:
> On Mon, Oct 06, 2008 at 03:54:57PM +0300, ext Riku Voipio wrote:
>   
>> On Mon, Oct 06, 2008 at 02:51:03PM +0300, Felipe Balbi wrote:
>>     
>>> On Mon, Oct 06, 2008 at 02:17:23PM +0300, Tony Lindgren wrote:
>>>       
>>>> * Riku Voipio <riku.voipio@iki.fi> [081006 14:09]:
>>>>         
>>>>> On Mon, Oct 06, 2008 at 12:11:31PM +0300, Felipe Balbi wrote:
>>>>>           
>>>>>> This driver should be sitting together with the other
>>>>>> led drivers.
>>>>>>             
>>>>> But this driver actually doesn't implement the led sysfs interface.
>>>>> If the driver is changed to implement the led framework interface,
>>>>> we break the existing n810 userland and expose only limited subset
>>>>> of lp5521 features for userland..
>>>>>           
>>>> Maybe this feature should be a distro specific hack on top of
>>>> the mainline tree?
>>>>         
>>> the problem is that led api (afaict) doesn't really have abstraction for
>>> rgb leds like the one we use. It can only handle blinky leds and on/off
>>> transitions and we need more functionality.
>>>       
>> RGB can somewhat be handled, by creating three led devices:
>>
>> n810:red:foo
>> n810:gree:foo
>> n810:blue:foo
>>     
>
> If that's the case I'd expect the driver author to do it since I have no
> time left for that. A few fixes here and there to the driver is fine and
> preparing the driver for mainline integration is also fine since it's a
> simple cp iteration.
>
>   
The driver and chip supports lots of functionality that is not yet in 
the led framework.
Even if there is now support for some "Hardware accelerated blink of 
LEDs" it is not enough.

The lp5521 chip can be programmed with blinking patterns including 
fades, triggers between R, G and B channels and other nice features
Chip has three modes: direct,(set brightness of leds directly through 
sysfs) load (load a pattern to channel) and run.

RGB direct mode could be handled with three led devices but the rest of 
the functionality is a bit trickier.

Patterns are mainly used in n810 as it doesn't require any CPU interaction

At the time of writing this driver led-class.txt stated that:
"Some leds can be programmed to flash in hardware. As this isn't a generic
LED device property, this should be exported as a device specific sysfs
attribute rather than part of the class if this functionality is required."

The driver for the next version of the lp552x chip uses the led class 
for the direct mode

-Mathias

Re: [PATCH 3/3] lp5521: move to drivers/leds

[David Brownell]

On Monday 06 October 2008, mathias nyman wrote:
> The driver for the next version of the lp552x chip uses the led class 
> for the direct mode

That sounds like the way to start ... any reason why the
current driver couldn't be changed do the same?

This would seem like a good motivation to add some features
to the LED framework.  Here are three types of LED that it
doesn't support well today:

 - Bicolor leds with two leads ... R-or-G, two wires,
   opposite diode directions.

 - Bicolor LEDs with three leads ... like R-and-Y, three
   wires, common anode or common cathode

 - RGB leds, four leads ... like R-and-G-and-B, etc

Except for the first case, these can be stuffed into the
framework by modeling them as multiple LEDs.  But when you
do that it gets awkward to make sure that for example you
get a purple (+R, -G, +B) blink, since the timers need to
be exactly in phase.  Or to combine blinking with PWM, so
you could for example mix in a little green...

Surely some features that would support RGB (RG, YG, etc)
LEDs better could support both fancy (lp5521) and simple
(GPIO without PWM) implementations ...

- Dave

Re: [PATCH 3/3] lp5521: move to drivers/leds

[Richard Purdie]

On Mon, 2008-10-06 at 09:33 -0700, David Brownell wrote:
> This would seem like a good motivation to add some features
> to the LED framework.  Here are three types of LED that it
> doesn't support well today:
> 
>  - Bicolor leds with two leads ... R-or-G, two wires,
>    opposite diode directions.
> 
>  - Bicolor LEDs with three leads ... like R-and-Y, three
>    wires, common anode or common cathode
> 
>  - RGB leds, four leads ... like R-and-G-and-B, etc
> 
> Except for the first case, these can be stuffed into the
> framework by modeling them as multiple LEDs.  But when you
> do that it gets awkward to make sure that for example you
> get a purple (+R, -G, +B) blink, since the timers need to
> be exactly in phase.  Or to combine blinking with PWM, so
> you could for example mix in a little green...
> 
> Surely some features that would support RGB (RG, YG, etc)
> LEDs better could support both fancy (lp5521) and simple
> (GPIO without PWM) implementations ...

The question is how and what you add to the class to support these
types. It could be as simple as some more obvious naming conventionto
associate LEDs and a way of sharing triggers amongst LEDs I guess...

Cheers,

Richard

Re: [PATCH 3/3] lp5521: move to drivers/leds

[David Brownell]

On Tuesday 07 October 2008, Richard Purdie wrote:
> The question is how and what you add to the class to support these
> types.

I was trying to go for "the right question is half the answer".
That would be the other half.  ;)


> It could be as simple as some more obvious naming conventionto 
> associate LEDs and a way of sharing triggers amongst LEDs I guess...

Trigger names could be LED names.   So that when one led changes
state, so would anything triggering on that name.  That's one
way to share...


We're also missing multi-led triggers, most notably cylon eyes.  :)

Re: [PATCH 1/3] i2c: lp5521: remove dead code

[Daniel Stone]

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

On Mon, Oct 06, 2008 at 12:11:29PM +0300, ext Felipe Balbi wrote:
> That LED_CONNECTED_WRONG was never defined so removing.
> If someone needs those hooks, add back via proper
> platform_data instead of nasty ifdefery.
> 
> Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>

Signed-off-by: Daniel Stone <daniel.stone@nokia.com>

IIRC, LED_CONNECTED_WRONG was only for early hardware builds of the
N810.

Cheers,
Daniel

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

Re: [PATCH 1/3] i2c: lp5521: remove dead code

[Felipe Balbi]

On Mon, Oct 06, 2008 at 12:47:22PM +0300, Daniel Stone wrote:
> On Mon, Oct 06, 2008 at 12:11:29PM +0300, ext Felipe Balbi wrote:
> > That LED_CONNECTED_WRONG was never defined so removing.
> > If someone needs those hooks, add back via proper
> > platform_data instead of nasty ifdefery.
> > 
> > Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
> 
> Signed-off-by: Daniel Stone <daniel.stone@nokia.com>
> 
> IIRC, LED_CONNECTED_WRONG was only for early hardware builds of the
> N810.

yeah and it's really weird to keep that since it was a bad hw mistake
:-p

-- 
balbi