[PATCH] leds: add driver for PCA9663 I2C chip

[PATCH] leds: add driver for PCA9663 I2C chip

[Peter Meerwald]

From: Peter Meerwald <p.meerwald@bct-electronic.com>

simple driver for the PCA9633 I2C chip supporting four LEDs and
255 brightness levels

---
 drivers/leds/Kconfig        |    8 ++
 drivers/leds/Makefile       |    1 +
 drivers/leds/leds-pca9633.c |  219 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 228 insertions(+), 0 deletions(-)
 create mode 100644 drivers/leds/leds-pca9633.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index c957c34..72df181 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -244,6 +244,14 @@ config LEDS_PCA955X
 	  LED driver chips accessed via the I2C bus.  Supported
 	  devices include PCA9550, PCA9551, PCA9552, and PCA9553.
 
+config LEDS_PCA9663
+	tristate "LED support for PCA9663 I2C chip"
+	depends on LEDS_CLASS
+	depends on I2C
+	help
+	  This option enables support for LEDs connected to the PCA9663
+	  LED driver chip accessed via the I2C bus.
+
 config LEDS_WM831X_STATUS
 	tristate "LED support for status LEDs on WM831x PMICs"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index b8a9723..a809871 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
 obj-$(CONFIG_LEDS_HP6XX)		+= leds-hp6xx.o
 obj-$(CONFIG_LEDS_FSG)			+= leds-fsg.o
 obj-$(CONFIG_LEDS_PCA955X)		+= leds-pca955x.o
+obj-$(CONFIG_LEDS_PCA9633)		+= leds-pca9633.o
 obj-$(CONFIG_LEDS_DA903X)		+= leds-da903x.o
 obj-$(CONFIG_LEDS_WM831X_STATUS)	+= leds-wm831x-status.o
 obj-$(CONFIG_LEDS_WM8350)		+= leds-wm8350.o
diff --git a/drivers/leds/leds-pca9633.c b/drivers/leds/leds-pca9633.c
new file mode 100644
index 0000000..68c30d7
--- /dev/null
+++ b/drivers/leds/leds-pca9633.c
@@ -0,0 +1,219 @@
+/*
+ * Copyright 2011 bct electronic GmbH
+ *
+ * Author: Peter Meerwald <p.meerwald@bct-electronic.com>
+ *
+ * Based on leds-pca955x.c
+ *
+ * This file is subject to the terms and conditions of version 2 of
+ * the GNU General Public License.  See the file COPYING in the main
+ * directory of this archive for more details.
+ *
+ * LED driver for the PCA9633 I2C LED driver (7-bit slave address 0x62)
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/string.h>
+#include <linux/ctype.h>
+#include <linux/leds.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/workqueue.h>
+#include <linux/slab.h>
+
+/* LED select registers determine the source that drives LED outputs */
+#define PCA9633_LED_OFF		0x0	/* LED driver off */
+#define PCA9633_LED_ON		0x1	/* LED driver on */
+#define PCA9633_LED_PWM		0x2	/* Controlled through PWM */
+#define PCA9633_LED_GRP_PWM	0x3	/* Controlled through PWM/GRPPWM */
+
+#define PCA9633_MODE1		0x00
+#define PCA9633_MODE2		0x01
+#define PCA9633_PWM_BASE	0x02
+#define PCA9633_LEDOUT		0x08
+
+static const struct i2c_device_id pca9633_id[] = {
+	{ "pca9633", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, pca9633_id);
+
+struct pca9633_led {
+	struct i2c_client *client;
+	struct work_struct work;
+	spinlock_t lock;
+	enum led_brightness brightness;
+	struct led_classdev led_cdev;
+	int led_num; /* 0 .. 3 potentially */
+	char name[32];
+};
+
+static void pca9633_led_work(struct work_struct *work)
+{
+	struct pca9633_led *pca9633 = container_of(work,
+		struct pca9633_led, work);
+
+	u8 ledout = i2c_smbus_read_byte_data(pca9633->client, PCA9633_LEDOUT);
+	int shift = 2 * pca9633->led_num;
+	u8 mask = 0x3 << shift;
+
+	switch (pca9633->brightness) {
+	case LED_FULL:
+		i2c_smbus_write_byte_data(pca9633->client, PCA9633_LEDOUT,
+			(ledout & ~mask) | (PCA9633_LED_ON << shift));
+		break;
+	case LED_OFF:
+		i2c_smbus_write_byte_data(pca9633->client, PCA9633_LEDOUT,
+			ledout & ~mask);
+		break;
+	default:
+		i2c_smbus_write_byte_data(pca9633->client,
+			PCA9633_PWM_BASE + pca9633->led_num,
+			pca9633->brightness);
+		i2c_smbus_write_byte_data(pca9633->client, PCA9633_LEDOUT,
+			(ledout & ~mask) | (PCA9633_LED_PWM << shift));
+		break;
+	}
+}
+
+static void pca9633_led_set(struct led_classdev *led_cdev,
+	enum led_brightness value)
+{
+	struct pca9633_led *pca9633;
+
+	pca9633 = container_of(led_cdev, struct pca9633_led, led_cdev);
+
+	spin_lock(&pca9633->lock);
+	pca9633->brightness = value;
+
+	/*
+	 * Must use workqueue for the actual I/O since I2C operations
+	 * can sleep.
+	 */
+	schedule_work(&pca9633->work);
+
+	spin_unlock(&pca9633->lock);
+}
+
+static int __devinit pca9633_probe(struct i2c_client *client,
+					const struct i2c_device_id *id)
+{
+	struct pca9633_led *pca9633;
+	struct i2c_adapter *adapter;
+	struct led_platform_data *pdata;
+	int i, err;
+
+	adapter = to_i2c_adapter(client->dev.parent);
+	pdata = client->dev.platform_data;
+
+	printk(KERN_INFO "leds-pca9633: LED driver at slave address 0x%02x\n",
+		client->addr);
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
+		return -EIO;
+
+	if (pdata) {
+		if (pdata->num_leds <= 0 || pdata->num_leds > 4) {
+			dev_err(&client->dev, "board info must claim at most 4 LEDs");
+			return -ENODEV;
+		}
+	}
+
+	pca9633 = kzalloc(sizeof(*pca9633) * 4, GFP_KERNEL);
+	if (!pca9633)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, pca9633);
+
+	for (i = 0; i < 4; i++) {
+		pca9633[i].client = client;
+		pca9633[i].led_num = i;
+
+		/* Platform data can specify LED names and default triggers */
+		if (pdata && i < pdata->num_leds) {
+			if (pdata->leds[i].name)
+				snprintf(pca9633[i].name,
+					 sizeof(pca9633[i].name), "pca9633:%s",
+					 pdata->leds[i].name);
+			if (pdata->leds[i].default_trigger)
+				pca9633[i].led_cdev.default_trigger =
+					pdata->leds[i].default_trigger;
+		} else {
+			snprintf(pca9633[i].name, sizeof(pca9633[i].name),
+				 "pca9633:%d", i);
+		}
+
+		spin_lock_init(&pca9633[i].lock);
+
+		pca9633[i].led_cdev.name = pca9633[i].name;
+		pca9633[i].led_cdev.brightness_set = pca9633_led_set;
+
+		INIT_WORK(&pca9633[i].work, pca9633_led_work);
+
+		err = led_classdev_register(&client->dev, &pca9633[i].led_cdev);
+		if (err < 0)
+			goto exit;
+	}
+
+	/* Disable LED all-call address and set normal mode */
+	i2c_smbus_write_byte_data(client, PCA9633_MODE1, 0x00);
+
+	/* Turn off LEDs */
+	i2c_smbus_write_byte_data(client, PCA9633_LEDOUT, 0x00);
+
+	return 0;
+
+exit:
+	while (i--) {
+		led_classdev_unregister(&pca9633[i].led_cdev);
+		cancel_work_sync(&pca9633[i].work);
+	}
+
+	kfree(pca9633);
+
+	return err;
+}
+
+static int __devexit pca9633_remove(struct i2c_client *client)
+{
+	struct pca9633_led *pca9633 = i2c_get_clientdata(client);
+	int i;
+
+	for (i = 0; i < 4; i++) {
+		led_classdev_unregister(&pca9633[i].led_cdev);
+		cancel_work_sync(&pca9633[i].work);
+	}
+
+	kfree(pca9633);
+
+	return 0;
+}
+
+static struct i2c_driver pca9633_driver = {
+	.driver = {
+		.name	= "leds-pca9633",
+		.owner	= THIS_MODULE,
+	},
+	.probe	= pca9633_probe,
+	.remove	= __devexit_p(pca9633_remove),
+	.id_table = pca9633_id,
+};
+
+static int __init pca9633_leds_init(void)
+{
+	return i2c_add_driver(&pca9633_driver);
+}
+
+static void __exit pca9633_leds_exit(void)
+{
+	i2c_del_driver(&pca9633_driver);
+}
+
+module_init(pca9633_leds_init);
+module_exit(pca9633_leds_exit);
+
+MODULE_AUTHOR("Peter Meerwald <p.meerwald@bct-electronic.com>");
+MODULE_DESCRIPTION("PCA9633 LED driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.4.1

Re: [PATCH] leds: add driver for PCA9663 I2C chip

[Lars-Peter Clausen]

On 01/16/2012 10:04 PM, Peter Meerwald wrote:
> From: Peter Meerwald <p.meerwald@bct-electronic.com>
> 
> simple driver for the PCA9633 I2C chip supporting four LEDs and
> 255 brightness levels


Looks good, just a few minor issues.

> 
> ---
>  drivers/leds/Kconfig        |    8 ++
>  drivers/leds/Makefile       |    1 +
>  drivers/leds/leds-pca9633.c |  219 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 228 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/leds/leds-pca9633.c
> [...]
> diff --git a/drivers/leds/leds-pca9633.c b/drivers/leds/leds-pca9633.c
> new file mode 100644
> index 0000000..68c30d7
> --- /dev/null
> +++ b/drivers/leds/leds-pca9633.c
> @@ -0,0 +1,219 @@
> +/*
> + * Copyright 2011 bct electronic GmbH
> + *
> + * Author: Peter Meerwald <p.meerwald@bct-electronic.com>
> + *
> + * Based on leds-pca955x.c
> + *
> + * This file is subject to the terms and conditions of version 2 of
> + * the GNU General Public License.  See the file COPYING in the main
> + * directory of this archive for more details.
> + *
> + * LED driver for the PCA9633 I2C LED driver (7-bit slave address 0x62)
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/string.h>
> +#include <linux/ctype.h>
> +#include <linux/leds.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/workqueue.h>
> +#include <linux/slab.h>
> +
> +/* LED select registers determine the source that drives LED outputs */
> +#define PCA9633_LED_OFF		0x0	/* LED driver off */
> +#define PCA9633_LED_ON		0x1	/* LED driver on */
> +#define PCA9633_LED_PWM		0x2	/* Controlled through PWM */
> +#define PCA9633_LED_GRP_PWM	0x3	/* Controlled through PWM/GRPPWM */
> +
> +#define PCA9633_MODE1		0x00
> +#define PCA9633_MODE2		0x01
> +#define PCA9633_PWM_BASE	0x02
> +#define PCA9633_LEDOUT		0x08
> +
> +static const struct i2c_device_id pca9633_id[] = {
> +	{ "pca9633", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, pca9633_id);
> +
> +struct pca9633_led {
> +	struct i2c_client *client;
> +	struct work_struct work;
> +	spinlock_t lock;
> +	enum led_brightness brightness;
> +	struct led_classdev led_cdev;
> +	int led_num; /* 0 .. 3 potentially */
> +	char name[32];
> +};
> +
> +static void pca9633_led_work(struct work_struct *work)
> +{
> +	struct pca9633_led *pca9633 = container_of(work,
> +		struct pca9633_led, work);
> +
> +	u8 ledout = i2c_smbus_read_byte_data(pca9633->client, PCA9633_LEDOUT);
> +	int shift = 2 * pca9633->led_num;
> +	u8 mask = 0x3 << shift;
> +
> +	switch (pca9633->brightness) {

Using led_cdev.brightness should work as well.

> +	case LED_FULL:
> +		i2c_smbus_write_byte_data(pca9633->client, PCA9633_LEDOUT,
> +			(ledout & ~mask) | (PCA9633_LED_ON << shift));
> +		break;
> +	case LED_OFF:
> +		i2c_smbus_write_byte_data(pca9633->client, PCA9633_LEDOUT,
> +			ledout & ~mask);
> +		break;
> +	default:
> +		i2c_smbus_write_byte_data(pca9633->client,
> +			PCA9633_PWM_BASE + pca9633->led_num,
> +			pca9633->brightness);
> +		i2c_smbus_write_byte_data(pca9633->client, PCA9633_LEDOUT,
> +			(ledout & ~mask) | (PCA9633_LED_PWM << shift));
> +		break;
> +	}
> +}
> +
> +static void pca9633_led_set(struct led_classdev *led_cdev,
> +	enum led_brightness value)
> +{
> +	struct pca9633_led *pca9633;
> +
> +	pca9633 = container_of(led_cdev, struct pca9633_led, led_cdev);
> +
> +	spin_lock(&pca9633->lock);

Does the spinlock actually protect against anything?

> +	pca9633->brightness = value;
> +
> +	/*
> +	 * Must use workqueue for the actual I/O since I2C operations
> +	 * can sleep.
> +	 */
> +	schedule_work(&pca9633->work);
> +
> +	spin_unlock(&pca9633->lock);
> +}
> +
> +static int __devinit pca9633_probe(struct i2c_client *client,
> +					const struct i2c_device_id *id)
> +{
> +	struct pca9633_led *pca9633;
> +	struct i2c_adapter *adapter;
> +	struct led_platform_data *pdata;
> +	int i, err;
> +
> +	adapter = to_i2c_adapter(client->dev.parent);
> +	pdata = client->dev.platform_data;
> +
> +	printk(KERN_INFO "leds-pca9633: LED driver at slave address 0x%02x\n",
> +		client->addr);

dev_info, but it probably wouldn't hurt either if the message is removed
altogether.

> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
> +		return -EIO;

You check for I2C functionality, but only ever use the smbus subset, which
should be supported by all adapters anyway, so you should be safe even without
the check.

> +
> +	if (pdata) {
> +		if (pdata->num_leds <= 0 || pdata->num_leds > 4) {
> +			dev_err(&client->dev, "board info must claim at most 4 LEDs");
> +			return -ENODEV;

-EINVAL?

> +		}
> +	}
> +
> +	pca9633 = kzalloc(sizeof(*pca9633) * 4, GFP_KERNEL);
> +	if (!pca9633)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, pca9633);
> +
> +	for (i = 0; i < 4; i++) {
> +		pca9633[i].client = client;
> +		pca9633[i].led_num = i;
> +
> +		/* Platform data can specify LED names and default triggers */
> +		if (pdata && i < pdata->num_leds) {
> +			if (pdata->leds[i].name)
> +				snprintf(pca9633[i].name,
> +					 sizeof(pca9633[i].name), "pca9633:%s",
> +					 pdata->leds[i].name);

Why the prefix?

> +			if (pdata->leds[i].default_trigger)
> +				pca9633[i].led_cdev.default_trigger =
> +					pdata->leds[i].default_trigger;
> +		} else {
> +			snprintf(pca9633[i].name, sizeof(pca9633[i].name),
> +				 "pca9633:%d", i);
> +		}
> +
> +		spin_lock_init(&pca9633[i].lock);
> +
> +		pca9633[i].led_cdev.name = pca9633[i].name;
> +		pca9633[i].led_cdev.brightness_set = pca9633_led_set;
> +
> +		INIT_WORK(&pca9633[i].work, pca9633_led_work);
> +
> +		err = led_classdev_register(&client->dev, &pca9633[i].led_cdev);
> +		if (err < 0)
> +			goto exit;
> +	}
> +
> +	/* Disable LED all-call address and set normal mode */
> +	i2c_smbus_write_byte_data(client, PCA9633_MODE1, 0x00);
> +
> +	/* Turn off LEDs */
> +	i2c_smbus_write_byte_data(client, PCA9633_LEDOUT, 0x00);
> +
> +	return 0;
> +
> +exit:
> +	while (i--) {
> +		led_classdev_unregister(&pca9633[i].led_cdev);
> +		cancel_work_sync(&pca9633[i].work);
> +	}
> +
> +	kfree(pca9633);
> +
> +	return err;
> +}
> +
>[...]
> +
> +static int __init pca9633_leds_init(void)
> +{
> +	return i2c_add_driver(&pca9633_driver);
> +}
> +
> +static void __exit pca9633_leds_exit(void)
> +{
> +	i2c_del_driver(&pca9633_driver);
> +}
> +
> +module_init(pca9633_leds_init);
> +module_exit(pca9633_leds_exit);

Use the new module_i2c_driver macro.

Re: [PATCH] leds: add driver for PCA9663 I2C chip

[Peter Meerwald]

Hello,

> > simple driver for the PCA9633 I2C chip supporting four LEDs and
> > 255 brightness levels

> Looks good, just a few minor issues.

thanks for the timely review and suggestions; I'll respond to our comments 
below and resubmit a v2 shortly

the driver is based on leds-pca955x.c, all your comments apply there as 
well :)

> > +static void pca9633_led_set(struct led_classdev *led_cdev,
> > +	enum led_brightness value)
> > +	spin_lock(&pca9633->lock);
> Does the spinlock actually protect against anything?

removed it

> > +	printk(KERN_INFO "leds-pca9633: LED driver at slave address 0x%02x\n",
> > +		client->addr);
> dev_info, but it probably wouldn't hurt either if the message is removed
> altogether.

dropped as suggested

> > +	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
> > +		return -EIO;
> You check for I2C functionality, but only ever use the smbus subset, which
> should be supported by all adapters anyway, so you should be safe even without
> the check.

dropped

> > +			dev_err(&client->dev, "board info must claim at most 4 LEDs");
> > +			return -ENODEV;
> -EINVAL?

changed

> > +			if (pdata->leds[i].name)
> > +				snprintf(pca9633[i].name,
> > +					 sizeof(pca9633[i].name), "pca9633:%s",
> > +					 pdata->leds[i].name);
> Why the prefix?

pca955x does it the same way; I'd rather keep it so one knowns where a LED 
is connected

> > +module_init(pca9633_leds_init);
> > +module_exit(pca9633_leds_exit);
> Use the new module_i2c_driver macro.

agree; I've submitted a patch to linux-i2c to mentionen the macro in 
Documentation/i2c/writing-clients

thanks, p.

-- 

Peter Meerwald
+43-664-2444418 (mobile)