[PATCH v2] Driver for NS LP3944 FunLight Chip

[PATCH v2] Driver for NS LP3944 FunLight Chip

[Antonio Ospite]

Hi,

here's a driver for the National Semiconductor LP3944 FunLight Chip,
I wrote it for the OpenEZX project (http://www.openezx.org).

Can you please give some feedback about its status?

I2C driver for National Semiconductor LP3944 Funlight Chip
http://www.national.com/pf/LP/LP3944.html

This helper chip can drive up to 8 leds, with two programmable dim modes;
it could even be used as a gpio expander but this driver assumes it is used
as a led controller.

The dim modes are used to set _blink_ patterns for leds, the pattern is
specified supplying two parameters:
  - period: from 0s to 1.6s
  - duty cycle: percentage of the period the led is on, from 0 to 100

LP3944 can be found on Motorola A910 smartphone, where it drives the rgb
leds, the camera flash light and the displays backlights.

Signed-off-by: Antonio Ospite <ao2-WB6LKoYH/xlAfugRpC6u6w@public.gmane.org>

Index: linux-2.6.26-rc6/drivers/i2c/chips/lp3944.c
===================================================================
--- /dev/null
+++ linux-2.6.26-rc6/drivers/i2c/chips/lp3944.c
@@ -0,0 +1,436 @@
+/*
+ * lp3944.c - driver for National Semiconductor LP3944 Funlight Chip
+ *
+ * Copyright (C) 2008 Antonio Ospite <ao2-WB6LKoYH/xlAfugRpC6u6w@public.gmane.org>
+ *
+ * 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.
+ *
+ */
+
+/*
+ * I2C driver for National Semiconductor LP3944 Funlight Chip
+ * http://www.national.com/pf/LP/LP3944.html
+ *
+ * This helper chip can drive up to 8 leds, with two programmable DIM modes;
+ * it could even be used as a gpio expander but this driver assumes it is used
+ * as a led controller.
+ *
+ * The DIM modes are used to set _blink_ patterns for leds, the pattern is
+ * specified supplying two parameters:
+ *   - period: from 0s to 1.6s
+ *   - duty cycle: percentage of the period the led is on, from 0 to 100
+ *
+ * LP3944 can be found on Motorola A910 smartphone, where it drives the rgb
+ * leds, the camera flash light and the displays backlights.
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/i2c/lp3944.h>
+
+#define LP3944_REG_INPUT1	0x00	/* LEDs 0-7 InputRegister (Read Only) */
+#define LP3944_REG_REGISTER1	0x01	/* None (Read Only) */
+#define LP3944_REG_PSC0		0x02	/* Frequency Prescaler 0 (R/W) */
+#define LP3944_REG_PWM0		0x03	/* PWM Register 0 (R/W) */
+#define LP3944_REG_PSC1		0x04	/* Frequency Prescaler 1 (R/W) */
+#define LP3944_REG_PWM1		0x05	/* PWM Register 1 (R/W) */
+#define LP3944_REG_LS0		0x06	/* LEDs 0-3 Selector (R/W) */
+#define LP3944_REG_LS1		0x07	/* LEDs 4-7 Selector (R/W) */
+#define LP3944_REG_REGISTER8	0x08	/* None (R/W) */
+#define LP3944_REG_REGISTER9	0x09	/* None (R/W) */
+
+#define LP3944_LED_STATUS_MASK	0x03
+
+/* Saved data */
+struct lp3944_data {
+	struct i2c_client *client;
+	struct mutex lock;
+
+	/* Only regs from 2 to 9 are R/W */
+	u8 LP3944_REGS[8];
+};
+
+static struct lp3944_data *lp3944;
+
+static int lp3944_reg_read(struct i2c_client *client, unsigned reg,
+			   unsigned *value);
+static int lp3944_reg_write(struct i2c_client *client, unsigned reg,
+			    unsigned value);
+
+/*
+ * Set the period in DIM status
+ *
+ * @dim: LP3944_DIM0 | LP3944_DIM1
+ * @period: period of a blink, that is a on/off cycle, in 1/10 sec
+ */
+int lp3944_dim_set_period(unsigned dim, unsigned period)
+{
+	unsigned psc_reg;
+	unsigned psc_value;
+	int err;
+
+	if (dim == LP3944_DIM0)
+		psc_reg = LP3944_REG_PSC0;
+	else if (dim == LP3944_DIM1)
+		psc_reg = LP3944_REG_PSC1;
+	else
+		return -EINVAL;
+
+	/* Convert period to Prescaler value */
+	if (period > LP3944_PERIOD_MAX)
+		return -EINVAL;
+
+	if (period == LP3944_PERIOD_MIN)
+		psc_value = 0;
+	else
+		psc_value = (period * LP3944_PERIOD_MAX) - 1;
+
+	mutex_lock(&lp3944->lock);
+	err = lp3944_reg_write(lp3944->client, psc_reg, psc_value);
+	mutex_unlock(&lp3944->lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(lp3944_dim_set_period);
+
+/*
+ * Set the duty cycle in DIM status
+ *
+ * @dim: LP3944_DIM0 | LP3944_DIM1
+ * @duty_cycle: percentage of a period in which a led is ON
+ */
+int lp3944_dim_set_dutycycle(unsigned dim, unsigned duty_cycle)
+{
+	unsigned pwm_reg;
+	unsigned pwm_value;
+	int err;
+
+	if (dim == LP3944_DIM0)
+		pwm_reg = LP3944_REG_PWM0;
+	else if (dim == LP3944_DIM1)
+		pwm_reg = LP3944_REG_PWM1;
+	else
+		return -EINVAL;
+
+	/* Convert duty cycle to PWM value */
+	if (duty_cycle > LP3944_DUTY_CYCLE_MAX)
+		return -EINVAL;
+
+	if (duty_cycle == LP3944_DUTY_CYCLE_MAX)
+		pwm_value = 255;
+	else
+		pwm_value = (duty_cycle * 256) / 100;
+
+	mutex_lock(&lp3944->lock);
+	err = lp3944_reg_write(lp3944->client, pwm_reg, pwm_value);
+	mutex_unlock(&lp3944->lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(lp3944_dim_set_dutycycle);
+
+/*
+ * Set the led status
+ *
+ * @led: LP3944_LED0-LP3944_LED7
+ * @status: off, on, dim0, dim1
+ */
+int lp3944_led_set(unsigned led, unsigned status)
+{
+	unsigned reg;
+	unsigned val;
+	int err;
+
+	switch (led) {
+	case LP3944_LED0:
+	case LP3944_LED1:
+	case LP3944_LED2:
+	case LP3944_LED3:
+		reg = LP3944_REG_LS0;
+		break;
+	case LP3944_LED4:
+	case LP3944_LED5:
+	case LP3944_LED6:
+	case LP3944_LED7:
+		led -= LP3944_LED4;
+		reg = LP3944_REG_LS1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (status > LP3944_LED_STATUS_DIM1)
+		return -EINVAL;
+
+	mutex_lock(&lp3944->lock);
+	lp3944_reg_read(lp3944->client, reg, &val);
+
+	val &= ~(LP3944_LED_STATUS_MASK << (led << 1));
+	val |= (status << (led << 1));
+
+	pr_debug("%s: led %d, status %d, val: 0x%02x\n",
+		 __func__, led, status, val);
+
+	/* set led status */
+	err = lp3944_reg_write(lp3944->client, reg, val);
+	mutex_unlock(&lp3944->lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(lp3944_led_set);
+
+/* low-level sysfs interface */
+static ssize_t lp3944_get_reg(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned reg;
+	unsigned val;
+	int ret = 0;
+
+	/* line length = strlen("0x00 0x00\n") */
+	const int l = 10;
+
+	for (reg = 0; reg <= LP3944_REG_REGISTER9; reg++) {
+		/*
+		 * we could even use lp3944 auto-increment feature here, but
+		 * that's not very important.
+		 */
+		lp3944_reg_read(client, reg, &val);
+		ret += sprintf(buf + l * reg, "0x%02x 0x%02x\n", reg, val);
+	}
+
+	return ret;
+}
+
+static ssize_t lp3944_set_reg(struct device *dev,
+			      struct device_attribute *attr, const char *buf,
+			      size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned reg;
+	unsigned val;
+
+	if (sscanf(buf, "0x%2x 0x%2x", &reg, &val) != 2)
+		return -EINVAL;
+
+	if (reg > LP3944_REG_REGISTER9)
+		return -EINVAL;
+
+	if (val > 0xff)
+		return -EINVAL;
+
+	lp3944_reg_write(client, reg, val);
+
+	return count;
+}
+
+static DEVICE_ATTR(reg, S_IWUSR | S_IRUGO, lp3944_get_reg, lp3944_set_reg);
+
+static struct attribute *lp3944_attributes[] = {
+	&dev_attr_reg.attr,
+	NULL
+};
+
+static const struct attribute_group lp3944_attr_group = {
+	.attrs = lp3944_attributes,
+};
+
+/* private stuff */
+
+static int lp3944_reg_read(struct i2c_client *client, unsigned reg,
+			   unsigned *value)
+{
+	int tmp;
+
+	tmp = i2c_smbus_read_byte_data(client, reg);
+	if (tmp < 0)
+		return -EINVAL;
+
+	*value = (unsigned) tmp;
+	pr_debug("%s: reg: 0x%02x value: 0x%02x\n", __func__, reg, *value);
+
+	return 0;
+}
+
+static int lp3944_reg_write(struct i2c_client *client, unsigned reg,
+			    unsigned value)
+{
+	unsigned tmp;
+	int ret;
+
+	tmp = i2c_smbus_read_byte_data(client, reg);
+	pr_debug("%s: before write reg: 0x%02x value: 0x%02x\n",
+		 __func__, reg, tmp);
+
+	ret = i2c_smbus_write_byte_data(client, reg, value);
+
+	tmp = i2c_smbus_read_byte_data(client, reg);
+	pr_debug("%s: after write reg: 0x%02x value: 0x%02x\n",
+		 __func__, reg, tmp);
+
+	return ret;
+}
+
+static int lp3944_init_client(struct i2c_client *client)
+{
+	struct lp3944_data *data = i2c_get_clientdata(client);
+	int tmp;
+
+	/* Try to read a byte, if it fails, return an error */
+	tmp = i2c_smbus_read_byte_data(client, LP3944_REG_INPUT1);
+	if (tmp < 0)
+		return -ENODEV;
+
+	/* Default values, taken from official datasheet */
+	lp3944_reg_write(client, LP3944_REG_PSC0, 0x00);
+	lp3944_reg_write(client, LP3944_REG_PWM0, 0x80);
+	lp3944_reg_write(client, LP3944_REG_PSC1, 0x00);
+	lp3944_reg_write(client, LP3944_REG_PWM1, 0x80);
+	lp3944_reg_write(client, LP3944_REG_LS0, 0x00);
+	lp3944_reg_write(client, LP3944_REG_LS1, 0x00);
+
+	data->LP3944_REGS[0] = 0x00;
+	data->LP3944_REGS[1] = 0x80;
+	data->LP3944_REGS[2] = 0x00;
+	data->LP3944_REGS[3] = 0x80;
+	data->LP3944_REGS[4] = 0x00;
+	data->LP3944_REGS[5] = 0x00;
+
+	return 0;
+}
+
+static int __devinit lp3944_probe(struct i2c_client *client)
+{
+	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+	struct lp3944_data *data;
+	int err;
+
+	dev_dbg(&client->dev, "adapter %s!\n", adapter->name);
+
+	if (lp3944) {
+		dev_dbg(&client->dev, "only one lp3944 chip allowed.\n");
+		return -ENODEV;
+	}
+
+	/* Let's see whether this adapter can support what we need. */
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
+		printk(KERN_ERR "%s: insufficient functionality!\n", __func__);
+		err = -EIO;
+		goto exit;
+	}
+
+	data = kzalloc(sizeof(struct lp3944_data), GFP_KERNEL);
+	if (!data) {
+		err = -ENOMEM;
+		goto exit;
+	}
+	data->client = client;
+	i2c_set_clientdata(client, data);
+
+	mutex_init(&data->lock);
+
+	/* Initialize the lp3944 chip */
+	err = lp3944_init_client(client);
+	if (err)
+		goto exit_kfree;
+
+	/* Register sysfs hooks */
+	err = sysfs_create_group(&client->dev.kobj, &lp3944_attr_group);
+	if (err)
+		goto exit_kfree;
+
+	lp3944 = data;
+	dev_info(&client->dev, "lp3944 enabled\n");
+
+	return 0;
+
+exit_kfree:
+	kfree(data);
+exit:
+	return err;
+}
+
+static int __devexit lp3944_remove(struct i2c_client *client)
+{
+	sysfs_remove_group(&client->dev.kobj, &lp3944_attr_group);
+
+	lp3944_reg_write(client, LP3944_REG_LS0, 0x00);
+	lp3944_reg_write(client, LP3944_REG_LS1, 0x00);
+
+	kfree(i2c_get_clientdata(client));
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int lp3944_suspend(struct i2c_client *client, pm_message_t state)
+{
+	struct lp3944_data *data = i2c_get_clientdata(client);
+	int i;
+
+	dev_dbg(&client->dev, "lp3944_suspend\n");
+
+	for (i = 2; i < LP3944_REG_REGISTER9; i++)
+		data->LP3944_REGS[i - 2] = i2c_smbus_read_byte_data(client, i);
+
+	return 0;
+}
+
+static int lp3944_resume(struct i2c_client *client)
+{
+	struct lp3944_data *data = i2c_get_clientdata(client);
+	int i;
+
+	dev_dbg(&client->dev, "lp3944_resume\n");
+
+	for (i = 2; i < LP3944_REG_REGISTER9; i++)
+		i2c_smbus_write_byte_data(client, i, data->LP3944_REGS[i - 2]);
+
+	return 0;
+}
+#else
+
+#define lp3944_suspend NULL
+#define lp3944_resume NULL
+
+#endif /* CONFIG_PM */
+
+/* lp3944 i2c driver struct */
+static struct i2c_driver lp3944_driver = {
+	.driver = {
+		   .name  = "lp3944",
+		   .owner = THIS_MODULE,
+		   },
+	.probe   = lp3944_probe,
+	.remove  = lp3944_remove,
+	.suspend = lp3944_suspend,
+	.resume  = lp3944_resume,
+};
+
+static int __init lp3944_module_init(void)
+{
+	int err;
+
+	err = i2c_add_driver(&lp3944_driver);
+	if (err)
+		printk(KERN_ERR "%s: can't add i2c driver\n", __func__);
+
+	return err;
+}
+
+static void __exit lp3944_module_exit(void)
+{
+	i2c_del_driver(&lp3944_driver);
+}
+
+module_init(lp3944_module_init);
+module_exit(lp3944_module_exit);
+
+/* Module information */
+MODULE_AUTHOR("Antonio Ospite <ao2-WB6LKoYH/xlAfugRpC6u6w@public.gmane.org>");
+MODULE_DESCRIPTION("LP3944 Fun Light Chip");
+MODULE_LICENSE("GPL");
Index: linux-2.6.26-rc6/drivers/i2c/chips/Kconfig
===================================================================
--- linux-2.6.26-rc6.orig/drivers/i2c/chips/Kconfig
+++ linux-2.6.26-rc6/drivers/i2c/chips/Kconfig
@@ -129,6 +129,16 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called tsl2550.
 
+config LP3944
+	tristate "National Semiconductor LP3944 Fun Light Chip"
+	depends on EXPERIMENTAL
+	help
+	  If you say yes here you get support for the National Semiconductor LP3944
+	  Lighting Management Unit (LMU) also known as a Fun Light Chip.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called lp3944.
+
 config MENELAUS
 	bool "TWL92330/Menelaus PM chip"
 	depends on I2C=y && ARCH_OMAP24XX
Index: linux-2.6.26-rc6/drivers/i2c/chips/Makefile
===================================================================
--- linux-2.6.26-rc6.orig/drivers/i2c/chips/Makefile
+++ linux-2.6.26-rc6/drivers/i2c/chips/Makefile
@@ -20,6 +20,7 @@
 obj-$(CONFIG_TPS65010)		+= tps65010.o
 obj-$(CONFIG_MENELAUS)		+= menelaus.o
 obj-$(CONFIG_SENSORS_TSL2550)	+= tsl2550.o
+obj-$(CONFIG_LP3944)		+= lp3944.o
 
 ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)
 EXTRA_CFLAGS += -DDEBUG
Index: linux-2.6.26-rc6/include/linux/i2c/lp3944.h
===================================================================
--- /dev/null
+++ linux-2.6.26-rc6/include/linux/i2c/lp3944.h
@@ -0,0 +1,45 @@
+/*
+ * lp3944.h - public interface for National Semiconductor LP3944 Funlight Chip
+ *
+ * Copyright (C) 2008 Antonio Ospite <ao2-WB6LKoYH/xlAfugRpC6u6w@public.gmane.org>
+ *
+ * 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.
+ *
+ */
+
+#ifndef __LINUX_I2C_LP3944_H
+#define __LINUX_I2C_LP3944_H
+
+#define LP3944_LED0 0
+#define LP3944_LED1 1
+#define LP3944_LED2 2
+#define LP3944_LED3 3
+#define LP3944_LED4 4
+#define LP3944_LED5 5
+#define LP3944_LED6 6
+#define LP3944_LED7 7
+
+#define LP3944_DIM0 0
+#define LP3944_DIM1 1
+
+/* period in 1/10 sec */
+#define LP3944_PERIOD_MIN 0
+#define LP3944_PERIOD_MAX 16
+
+/* duty cycle is a percentage */
+#define LP3944_DUTY_CYCLE_MIN 0
+#define LP3944_DUTY_CYCLE_MAX 100
+
+/* led statuses */
+#define LP3944_LED_STATUS_OFF	0x0
+#define LP3944_LED_STATUS_ON	0x1
+#define LP3944_LED_STATUS_DIM0	0x2
+#define LP3944_LED_STATUS_DIM1	0x3
+
+extern int lp3944_dim_set_period(unsigned dim, unsigned period);
+extern int lp3944_dim_set_dutycycle(unsigned int dim, unsigned duty_cycle);
+extern int lp3944_led_set(unsigned led, unsigned status);
+
+#endif /* __LINUX_I2C_LP3944_H */
Index: linux-2.6.26-rc6/Documentation/i2c/chips/lp3944
===================================================================
--- /dev/null
+++ linux-2.6.26-rc6/Documentation/i2c/chips/lp3944
@@ -0,0 +1,101 @@
+Kernel driver lp3944
+====================
+
+  * National Semiconductor LP3944 Fun-light Chip
+    Prefix: 'lp3944'
+    Addresses scanned: None (see the Notes section below)
+    Datasheet: Publicly available at the National Semiconductor website
+               http://www.national.com/pf/LP/LP3944.html
+
+Authors:
+        Antonio Ospite <ao2-WB6LKoYH/xlAfugRpC6u6w@public.gmane.org>
+
+
+Description
+-----------
+The LP3944 is a helper chip that can drive up to 8 leds, with two programmable
+DIM modes; it could even be used as a gpio expander but this driver assumes it
+is used as a led controller.
+
+The DIM modes are used to set _blink_ patterns for leds, the pattern is
+specified supplying two parameters:
+  - period: from 0s to 1.6s
+  - duty cycle: percentage of the period the led is on, from 0 to 100
+
+Setting a led in DIM0 or DIM1 mode makes it blink according to the pattern.
+
+See the datasheet for details.
+
+LP3944 can be found on Motorola A910 smartphone, where it drives the rgb
+leds, the camera flash light and the displays backlights.
+
+
+Notes
+-----
+The chip is used mainly in embedded contextes, so this driver expects it is
+registered using the i2c_board_info mechanism.
+
+To register the chip at address 0x60 on adapter 0, set the info:
+
+	static struct i2c_board_info __initdata a910_i2c_board_info[] = {
+		{
+			I2C_BOARD_INFO("lp3944", 0x60),
+			.type = "lp3944",
+		},
+	};
+
+and register it in the platform init function
+
+	i2c_register_board_info(0, a910_i2c_board_info,
+			ARRAY_SIZE(a910_i2c_board_info));
+
+
+Accessing LP3944 via exported interface
+---------------------------------------
+The driver exposes a friendly interface to abstract the chip functionalities
+for other drivers to use.
+
+Including include/linux/i2c/lp3944.h one can use these calls:
+
+extern int lp3944_dim_set_period(unsigned dim, unsigned period);
+extern int lp3944_dim_set_dutycycle(unsigned int dim, unsigned duty_cycle);
+extern int lp3944_led_set(unsigned led, unsigned status);
+
+in a leds-class driver.
+
+
+Accessing LP3944 via /sys interface
+-----------------------------------
+Standard i2c sysfs interface can be found at
+/sys/bus/i2c/devices/<0>-<1>/
+or
+/sys/bus/i2c/drivers/lp3944/<0>-<1>/
+
+where <0> is the bus the chip was registered to (e. g. i2c-0)
+and <1> the chip address.
+
+Additionally, there is a R/W 'reg' file which can be used to dump and set
+register values.
+  $ cat reg
+  0x00 0xd7
+  0x01 0x00
+  0x02 0x4f
+  0x03 0x0c
+  0x04 0x9f
+  0x05 0x80
+  0x06 0x40
+  0x07 0x00
+  0x08 0x00
+  0x09 0x00
+
+Set period and duty_cycle for DIM0 mode:
+  $ echo 0x02 0x4f > reg
+  $ echo 0x03 0x0c > reg
+
+Set LED0 mode to DIM0
+  $ echo 0x06 0x02 > reg
+
+Registers 0x08 and 0x09 are not used for led control but are still valid to
+store arbitrary bytes :)
+  $ echo 0x08 0xde > reg
+  $ echo 0x09 0xad > reg

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH v2] Driver for NS LP3944 FunLight Chip

[Jean Delvare]

Hi Antonio,

On Wed, 18 Jun 2008 09:46:45 +0200, Antonio Ospite wrote:
> here's a driver for the National Semiconductor LP3944 FunLight Chip,
> I wrote it for the OpenEZX project (http://www.openezx.org).
> 
> Can you please give some feedback about its status?
> 
> I2C driver for National Semiconductor LP3944 Funlight Chip
> http://www.national.com/pf/LP/LP3944.html
> 
> This helper chip can drive up to 8 leds, with two programmable dim modes;
> it could even be used as a gpio expander but this driver assumes it is used
> as a led controller.
> 
> The dim modes are used to set _blink_ patterns for leds, the pattern is
> specified supplying two parameters:
>   - period: from 0s to 1.6s
>   - duty cycle: percentage of the period the led is on, from 0 to 100
> 
> LP3944 can be found on Motorola A910 smartphone, where it drives the rgb
> leds, the camera flash light and the displays backlights.
> 
> Signed-off-by: Antonio Ospite <ao2-WB6LKoYH/xlAfugRpC6u6w@public.gmane.org>
> 
> Index: linux-2.6.26-rc6/drivers/i2c/chips/lp3944.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6.26-rc6/drivers/i2c/chips/lp3944.c

Led drivers have their dedicated subsystems. They go to drivers/leds.
Please check MAINTAINERS for the maintainer of this subsystem and send
your patch to that person.

(To make it clear: I'm not taking this driver under drivers/i2c/chips.
I'm trying to empty this directory, chip drivers must go to their
respective subsystems.

Random comments:

> @@ -0,0 +1,436 @@
> +/*
> + * lp3944.c - driver for National Semiconductor LP3944 Funlight Chip
> + *
> + * Copyright (C) 2008 Antonio Ospite <ao2-WB6LKoYH/xlAfugRpC6u6w@public.gmane.org>
> + *
> + * 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.
> + *
> + */
> +
> +/*
> + * I2C driver for National Semiconductor LP3944 Funlight Chip
> + * http://www.national.com/pf/LP/LP3944.html
> + *
> + * This helper chip can drive up to 8 leds, with two programmable DIM modes;
> + * it could even be used as a gpio expander but this driver assumes it is used
> + * as a led controller.
> + *
> + * The DIM modes are used to set _blink_ patterns for leds, the pattern is
> + * specified supplying two parameters:
> + *   - period: from 0s to 1.6s
> + *   - duty cycle: percentage of the period the led is on, from 0 to 100
> + *
> + * LP3944 can be found on Motorola A910 smartphone, where it drives the rgb
> + * leds, the camera flash light and the displays backlights.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c/lp3944.h>
> +
> +#define LP3944_REG_INPUT1	0x00	/* LEDs 0-7 InputRegister (Read Only) */
> +#define LP3944_REG_REGISTER1	0x01	/* None (Read Only) */
> +#define LP3944_REG_PSC0		0x02	/* Frequency Prescaler 0 (R/W) */
> +#define LP3944_REG_PWM0		0x03	/* PWM Register 0 (R/W) */
> +#define LP3944_REG_PSC1		0x04	/* Frequency Prescaler 1 (R/W) */
> +#define LP3944_REG_PWM1		0x05	/* PWM Register 1 (R/W) */
> +#define LP3944_REG_LS0		0x06	/* LEDs 0-3 Selector (R/W) */
> +#define LP3944_REG_LS1		0x07	/* LEDs 4-7 Selector (R/W) */
> +#define LP3944_REG_REGISTER8	0x08	/* None (R/W) */
> +#define LP3944_REG_REGISTER9	0x09	/* None (R/W) */

None? Why do you care about these registers at all then?

> +
> +#define LP3944_LED_STATUS_MASK	0x03
> +
> +/* Saved data */
> +struct lp3944_data {
> +	struct i2c_client *client;
> +	struct mutex lock;
> +
> +	/* Only regs from 2 to 9 are R/W */
> +	u8 LP3944_REGS[8];

Please use only lowercase for struct member names.

> +};
> +
> +static struct lp3944_data *lp3944;
> +
> +static int lp3944_reg_read(struct i2c_client *client, unsigned reg,
> +			   unsigned *value);
> +static int lp3944_reg_write(struct i2c_client *client, unsigned reg,
> +			    unsigned value);
> +
> +/*
> + * Set the period in DIM status
> + *
> + * @dim: LP3944_DIM0 | LP3944_DIM1

That's a bit confusing, it makes it look like LP3944_DIM0 and
LP3944_DIM1 are values which can be bitwise or'd. A plain "or" would
make it clearer that you must pass exactly one of these values.

> + * @period: period of a blink, that is a on/off cycle, in 1/10 sec
> + */

You use kerneldoc-like comment formatting but there's a missing * on
the first line for the kerneldoc tools to actually pick the comment.
Same below.

> +int lp3944_dim_set_period(unsigned dim, unsigned period)
> +{
> +	unsigned psc_reg;
> +	unsigned psc_value;
> +	int err;
> +
> +	if (dim == LP3944_DIM0)
> +		psc_reg = LP3944_REG_PSC0;
> +	else if (dim == LP3944_DIM1)
> +		psc_reg = LP3944_REG_PSC1;
> +	else
> +		return -EINVAL;
> +
> +	/* Convert period to Prescaler value */
> +	if (period > LP3944_PERIOD_MAX)
> +		return -EINVAL;
> +
> +	if (period == LP3944_PERIOD_MIN)
> +		psc_value = 0;
> +	else
> +		psc_value = (period * LP3944_PERIOD_MAX) - 1;

That's pretty confusing. Why would you want to multiply a period value
by another period value? Maybe it happens to be numerically correct but
from a physical unit point of view that doesn't make much sense.

> +
> +	mutex_lock(&lp3944->lock);
> +	err = lp3944_reg_write(lp3944->client, psc_reg, psc_value);
> +	mutex_unlock(&lp3944->lock);

What are you trying to protect yourself from with this locking?

> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(lp3944_dim_set_period);
> +
> +/*
> + * Set the duty cycle in DIM status
> + *
> + * @dim: LP3944_DIM0 | LP3944_DIM1
> + * @duty_cycle: percentage of a period in which a led is ON
> + */
> +int lp3944_dim_set_dutycycle(unsigned dim, unsigned duty_cycle)
> +{
> +	unsigned pwm_reg;
> +	unsigned pwm_value;
> +	int err;
> +
> +	if (dim == LP3944_DIM0)
> +		pwm_reg = LP3944_REG_PWM0;
> +	else if (dim == LP3944_DIM1)
> +		pwm_reg = LP3944_REG_PWM1;
> +	else
> +		return -EINVAL;
> +
> +	/* Convert duty cycle to PWM value */
> +	if (duty_cycle > LP3944_DUTY_CYCLE_MAX)
> +		return -EINVAL;
> +
> +	if (duty_cycle == LP3944_DUTY_CYCLE_MAX)
> +		pwm_value = 255;
> +	else
> +		pwm_value = (duty_cycle * 256) / 100;
> +
> +	mutex_lock(&lp3944->lock);
> +	err = lp3944_reg_write(lp3944->client, pwm_reg, pwm_value);
> +	mutex_unlock(&lp3944->lock);

Again I don't see what this locking is there for.

> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(lp3944_dim_set_dutycycle);
> +
> +/*
> + * Set the led status
> + *
> + * @led: LP3944_LED0-LP3944_LED7
> + * @status: off, on, dim0, dim1

The status is actually one of LP3944_LED_STATUS_OFF,
LP3944_LED_STATUS_ON, LP3944_LED_STATUS_DIM0 or LP3944_LED_STATUS_DIM1.

> + */
> +int lp3944_led_set(unsigned led, unsigned status)
> +{
> +	unsigned reg;
> +	unsigned val;
> +	int err;
> +
> +	switch (led) {
> +	case LP3944_LED0:
> +	case LP3944_LED1:
> +	case LP3944_LED2:
> +	case LP3944_LED3:
> +		reg = LP3944_REG_LS0;
> +		break;
> +	case LP3944_LED4:
> +	case LP3944_LED5:
> +	case LP3944_LED6:
> +	case LP3944_LED7:
> +		led -= LP3944_LED4;
> +		reg = LP3944_REG_LS1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (status > LP3944_LED_STATUS_DIM1)
> +		return -EINVAL;
> +
> +	mutex_lock(&lp3944->lock);
> +	lp3944_reg_read(lp3944->client, reg, &val);
> +
> +	val &= ~(LP3944_LED_STATUS_MASK << (led << 1));
> +	val |= (status << (led << 1));
> +
> +	pr_debug("%s: led %d, status %d, val: 0x%02x\n",
> +		 __func__, led, status, val);
> +
> +	/* set led status */
> +	err = lp3944_reg_write(lp3944->client, reg, val);
> +	mutex_unlock(&lp3944->lock);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(lp3944_led_set);
> +
> +/* low-level sysfs interface */
> +static ssize_t lp3944_get_reg(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	unsigned reg;
> +	unsigned val;
> +	int ret = 0;
> +
> +	/* line length = strlen("0x00 0x00\n") */
> +	const int l = 10;
> +
> +	for (reg = 0; reg <= LP3944_REG_REGISTER9; reg++) {
> +		/*
> +		 * we could even use lp3944 auto-increment feature here, but
> +		 * that's not very important.
> +		 */
> +		lp3944_reg_read(client, reg, &val);
> +		ret += sprintf(buf + l * reg, "0x%02x 0x%02x\n", reg, val);
> +	}
> +
> +	return ret;
> +}
> +
> +static ssize_t lp3944_set_reg(struct device *dev,
> +			      struct device_attribute *attr, const char *buf,
> +			      size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	unsigned reg;
> +	unsigned val;
> +
> +	if (sscanf(buf, "0x%2x 0x%2x", &reg, &val) != 2)
> +		return -EINVAL;
> +
> +	if (reg > LP3944_REG_REGISTER9)
> +		return -EINVAL;
> +
> +	if (val > 0xff)
> +		return -EINVAL;
> +
> +	lp3944_reg_write(client, reg, val);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(reg, S_IWUSR | S_IRUGO, lp3944_get_reg, lp3944_set_reg);

I would get rid of this interface altogether, it doesn't make much
sense.

> +
> +static struct attribute *lp3944_attributes[] = {
> +	&dev_attr_reg.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group lp3944_attr_group = {
> +	.attrs = lp3944_attributes,
> +};
> +
> +/* private stuff */
> +
> +static int lp3944_reg_read(struct i2c_client *client, unsigned reg,
> +			   unsigned *value)
> +{
> +	int tmp;
> +
> +	tmp = i2c_smbus_read_byte_data(client, reg);
> +	if (tmp < 0)
> +		return -EINVAL;
> +
> +	*value = (unsigned) tmp;

Useless cast.

> +	pr_debug("%s: reg: 0x%02x value: 0x%02x\n", __func__, reg, *value);
> +
> +	return 0;
> +}
> +
> +static int lp3944_reg_write(struct i2c_client *client, unsigned reg,
> +			    unsigned value)
> +{
> +	unsigned tmp;
> +	int ret;
> +
> +	tmp = i2c_smbus_read_byte_data(client, reg);
> +	pr_debug("%s: before write reg: 0x%02x value: 0x%02x\n",
> +		 __func__, reg, tmp);
> +
> +	ret = i2c_smbus_write_byte_data(client, reg, value);
> +
> +	tmp = i2c_smbus_read_byte_data(client, reg);
> +	pr_debug("%s: after write reg: 0x%02x value: 0x%02x\n",
> +		 __func__, reg, tmp);

That's fairly inefficient. Is there any reason why you have to read
from the register twice as you write to it? I2C is slow enough as is.
If that's only for debugging, then you should make it conditional upon
DEBUG being defined. But quite frankly this doesn't seem to be very
useful debugging.

> +
> +	return ret;
> +}
> +
> +static int lp3944_init_client(struct i2c_client *client)
> +{
> +	struct lp3944_data *data = i2c_get_clientdata(client);
> +	int tmp;
> +
> +	/* Try to read a byte, if it fails, return an error */
> +	tmp = i2c_smbus_read_byte_data(client, LP3944_REG_INPUT1);
> +	if (tmp < 0)
> +		return -ENODEV;

What's the point of doing this?

> +
> +	/* Default values, taken from official datasheet */

If these are the default values, why do you write them to the chip?
Instead of, say, just use what the registers hold, so you don't
overwrite previous configuration done by the BIOS or firmware?

> +	lp3944_reg_write(client, LP3944_REG_PSC0, 0x00);
> +	lp3944_reg_write(client, LP3944_REG_PWM0, 0x80);
> +	lp3944_reg_write(client, LP3944_REG_PSC1, 0x00);
> +	lp3944_reg_write(client, LP3944_REG_PWM1, 0x80);
> +	lp3944_reg_write(client, LP3944_REG_LS0, 0x00);
> +	lp3944_reg_write(client, LP3944_REG_LS1, 0x00);
> +
> +	data->LP3944_REGS[0] = 0x00;
> +	data->LP3944_REGS[1] = 0x80;
> +	data->LP3944_REGS[2] = 0x00;
> +	data->LP3944_REGS[3] = 0x80;
> +	data->LP3944_REGS[4] = 0x00;
> +	data->LP3944_REGS[5] = 0x00;

Why do you write these values in data->LP3944_REGS? You never access
them after that.

> +
> +	return 0;
> +}
> +
> +static int __devinit lp3944_probe(struct i2c_client *client)
> +{
> +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> +	struct lp3944_data *data;
> +	int err;
> +
> +	dev_dbg(&client->dev, "adapter %s!\n", adapter->name);

Not very useful...

> +
> +	if (lp3944) {
> +		dev_dbg(&client->dev, "only one lp3944 chip allowed.\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Let's see whether this adapter can support what we need. */
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> +		printk(KERN_ERR "%s: insufficient functionality!\n", __func__);
> +		err = -EIO;

That's not an I/O error, so I'd rather return -ENODEV. Not that I
expect this check to ever fail anyway.

> +		goto exit;
> +	}
> +
> +	data = kzalloc(sizeof(struct lp3944_data), GFP_KERNEL);
> +	if (!data) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +	data->client = client;
> +	i2c_set_clientdata(client, data);
> +
> +	mutex_init(&data->lock);
> +
> +	/* Initialize the lp3944 chip */
> +	err = lp3944_init_client(client);
> +	if (err)
> +		goto exit_kfree;
> +
> +	/* Register sysfs hooks */
> +	err = sysfs_create_group(&client->dev.kobj, &lp3944_attr_group);
> +	if (err)
> +		goto exit_kfree;
> +
> +	lp3944 = data;
> +	dev_info(&client->dev, "lp3944 enabled\n");
> +
> +	return 0;
> +
> +exit_kfree:
> +	kfree(data);
> +exit:
> +	return err;
> +}
> +
> +static int __devexit lp3944_remove(struct i2c_client *client)
> +{
> +	sysfs_remove_group(&client->dev.kobj, &lp3944_attr_group);
> +
> +	lp3944_reg_write(client, LP3944_REG_LS0, 0x00);
> +	lp3944_reg_write(client, LP3944_REG_LS1, 0x00);

Why are these commands doing? Please add a comment.

> +
> +	kfree(i2c_get_clientdata(client));
> +	return 0;
> +}

You forgot to set lp3944 to NULL, so if the device is unbound it can
never be bound back.

> +
> +#ifdef CONFIG_PM
> +static int lp3944_suspend(struct i2c_client *client, pm_message_t state)
> +{
> +	struct lp3944_data *data = i2c_get_clientdata(client);
> +	int i;
> +
> +	dev_dbg(&client->dev, "lp3944_suspend\n");
> +
> +	for (i = 2; i < LP3944_REG_REGISTER9; i++)

That's inconsistent, you use a bare number for one boundary but a
define for the other boundary.

> +		data->LP3944_REGS[i - 2] = i2c_smbus_read_byte_data(client, i);
> +
> +	return 0;
> +}
> +
> +static int lp3944_resume(struct i2c_client *client)
> +{
> +	struct lp3944_data *data = i2c_get_clientdata(client);
> +	int i;
> +
> +	dev_dbg(&client->dev, "lp3944_resume\n");
> +
> +	for (i = 2; i < LP3944_REG_REGISTER9; i++)
> +		i2c_smbus_write_byte_data(client, i, data->LP3944_REGS[i - 2]);
> +
> +	return 0;
> +}
> +#else
> +
> +#define lp3944_suspend NULL
> +#define lp3944_resume NULL
> +
> +#endif /* CONFIG_PM */
> +
> +/* lp3944 i2c driver struct */
> +static struct i2c_driver lp3944_driver = {
> +	.driver = {
> +		   .name  = "lp3944",
> +		   .owner = THIS_MODULE,
> +		   },

This closing curly brace should get one less level of indentation.

> +	.probe   = lp3944_probe,
> +	.remove  = lp3944_remove,

Missing __devexit_p.

> +	.suspend = lp3944_suspend,
> +	.resume  = lp3944_resume,

This is missing a .id_table for i2c-core to do the device/driver
matching (probably this didn't exist yet when you wrote your driver.)

> +};
> +
> +static int __init lp3944_module_init(void)
> +{
> +	int err;
> +
> +	err = i2c_add_driver(&lp3944_driver);
> +	if (err)
> +		printk(KERN_ERR "%s: can't add i2c driver\n", __func__);

i2c_add_driver() is extremely unlikely to fail, and if it does, you'll
know soon enough. So this function could be simplified.

> +
> +	return err;
> +}
> +
> +static void __exit lp3944_module_exit(void)
> +{
> +	i2c_del_driver(&lp3944_driver);
> +}
> +
> +module_init(lp3944_module_init);
> +module_exit(lp3944_module_exit);
> +
> +/* Module information */
> +MODULE_AUTHOR("Antonio Ospite <ao2-WB6LKoYH/xlAfugRpC6u6w@public.gmane.org>");
> +MODULE_DESCRIPTION("LP3944 Fun Light Chip");
> +MODULE_LICENSE("GPL");
> Index: linux-2.6.26-rc6/drivers/i2c/chips/Kconfig
> ===================================================================
> --- linux-2.6.26-rc6.orig/drivers/i2c/chips/Kconfig
> +++ linux-2.6.26-rc6/drivers/i2c/chips/Kconfig
> @@ -129,6 +129,16 @@
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called tsl2550.
>  
> +config LP3944
> +	tristate "National Semiconductor LP3944 Fun Light Chip"
> +	depends on EXPERIMENTAL
> +	help
> +	  If you say yes here you get support for the National Semiconductor LP3944
> +	  Lighting Management Unit (LMU) also known as a Fun Light Chip.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called lp3944.
> +
>  config MENELAUS
>  	bool "TWL92330/Menelaus PM chip"
>  	depends on I2C=y && ARCH_OMAP24XX
> Index: linux-2.6.26-rc6/drivers/i2c/chips/Makefile
> ===================================================================
> --- linux-2.6.26-rc6.orig/drivers/i2c/chips/Makefile
> +++ linux-2.6.26-rc6/drivers/i2c/chips/Makefile
> @@ -20,6 +20,7 @@
>  obj-$(CONFIG_TPS65010)		+= tps65010.o
>  obj-$(CONFIG_MENELAUS)		+= menelaus.o
>  obj-$(CONFIG_SENSORS_TSL2550)	+= tsl2550.o
> +obj-$(CONFIG_LP3944)		+= lp3944.o
>  
>  ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)
>  EXTRA_CFLAGS += -DDEBUG
> Index: linux-2.6.26-rc6/include/linux/i2c/lp3944.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6.26-rc6/include/linux/i2c/lp3944.h
> @@ -0,0 +1,45 @@
> +/*
> + * lp3944.h - public interface for National Semiconductor LP3944 Funlight Chip
> + *
> + * Copyright (C) 2008 Antonio Ospite <ao2-WB6LKoYH/xlAfugRpC6u6w@public.gmane.org>
> + *
> + * 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.
> + *
> + */
> +
> +#ifndef __LINUX_I2C_LP3944_H
> +#define __LINUX_I2C_LP3944_H
> +
> +#define LP3944_LED0 0
> +#define LP3944_LED1 1
> +#define LP3944_LED2 2
> +#define LP3944_LED3 3
> +#define LP3944_LED4 4
> +#define LP3944_LED5 5
> +#define LP3944_LED6 6
> +#define LP3944_LED7 7
> +
> +#define LP3944_DIM0 0
> +#define LP3944_DIM1 1

These defines don't look terribly useful.

> +
> +/* period in 1/10 sec */
> +#define LP3944_PERIOD_MIN 0
> +#define LP3944_PERIOD_MAX 16
> +
> +/* duty cycle is a percentage */
> +#define LP3944_DUTY_CYCLE_MIN 0
> +#define LP3944_DUTY_CYCLE_MAX 100
> +
> +/* led statuses */
> +#define LP3944_LED_STATUS_OFF	0x0
> +#define LP3944_LED_STATUS_ON	0x1
> +#define LP3944_LED_STATUS_DIM0	0x2
> +#define LP3944_LED_STATUS_DIM1	0x3
> +
> +extern int lp3944_dim_set_period(unsigned dim, unsigned period);
> +extern int lp3944_dim_set_dutycycle(unsigned int dim, unsigned duty_cycle);
> +extern int lp3944_led_set(unsigned led, unsigned status);
> +
> +#endif /* __LINUX_I2C_LP3944_H */
> Index: linux-2.6.26-rc6/Documentation/i2c/chips/lp3944
> ===================================================================
> --- /dev/null
> +++ linux-2.6.26-rc6/Documentation/i2c/chips/lp3944
> @@ -0,0 +1,101 @@
> +Kernel driver lp3944
> +====================
> +
> +  * National Semiconductor LP3944 Fun-light Chip
> +    Prefix: 'lp3944'
> +    Addresses scanned: None (see the Notes section below)
> +    Datasheet: Publicly available at the National Semiconductor website
> +               http://www.national.com/pf/LP/LP3944.html
> +
> +Authors:
> +        Antonio Ospite <ao2-WB6LKoYH/xlAfugRpC6u6w@public.gmane.org>
> +
> +
> +Description
> +-----------
> +The LP3944 is a helper chip that can drive up to 8 leds, with two programmable
> +DIM modes; it could even be used as a gpio expander but this driver assumes it
> +is used as a led controller.
> +
> +The DIM modes are used to set _blink_ patterns for leds, the pattern is
> +specified supplying two parameters:
> +  - period: from 0s to 1.6s
> +  - duty cycle: percentage of the period the led is on, from 0 to 100
> +
> +Setting a led in DIM0 or DIM1 mode makes it blink according to the pattern.
> +
> +See the datasheet for details.
> +
> +LP3944 can be found on Motorola A910 smartphone, where it drives the rgb
> +leds, the camera flash light and the displays backlights.
> +
> +
> +Notes
> +-----
> +The chip is used mainly in embedded contextes, so this driver expects it is

Spelling: contexts.

> +registered using the i2c_board_info mechanism.
> +
> +To register the chip at address 0x60 on adapter 0, set the info:
> +
> +	static struct i2c_board_info __initdata a910_i2c_board_info[] = {
> +		{
> +			I2C_BOARD_INFO("lp3944", 0x60),
> +			.type = "lp3944",

The type field has gone by now.

> +		},
> +	};
> +
> +and register it in the platform init function
> +
> +	i2c_register_board_info(0, a910_i2c_board_info,
> +			ARRAY_SIZE(a910_i2c_board_info));
> +
> +

This is only one way to use register a device. i2c_new_device() is
another.

> +Accessing LP3944 via exported interface
> +---------------------------------------
> +The driver exposes a friendly interface to abstract the chip functionalities
> +for other drivers to use.
> +
> +Including include/linux/i2c/lp3944.h one can use these calls:

Probably better written "Including <linux/i2c/lp3944.h> ...".

> +
> +extern int lp3944_dim_set_period(unsigned dim, unsigned period);
> +extern int lp3944_dim_set_dutycycle(unsigned int dim, unsigned duty_cycle);
> +extern int lp3944_led_set(unsigned led, unsigned status);
> +
> +in a leds-class driver.

Apparently this implies that your driver (or at least this interface
thereof) can only support one device? That's unfortunate.

> +
> +
> +Accessing LP3944 via /sys interface
> +-----------------------------------
> +Standard i2c sysfs interface can be found at

What "standard i2c sysfs interface" are you talking about?

> +/sys/bus/i2c/devices/<0>-<1>/
> +or
> +/sys/bus/i2c/drivers/lp3944/<0>-<1>/
> +
> +where <0> is the bus the chip was registered to (e. g. i2c-0)
> +and <1> the chip address.

That's an odd syntax if you ask me. <bus>-<addr> would be less
confusing (and maybe give a real-life example as well.)

> +
> +Additionally, there is a R/W 'reg' file which can be used to dump and set
> +register values.
> +  $ cat reg
> +  0x00 0xd7
> +  0x01 0x00
> +  0x02 0x4f
> +  0x03 0x0c
> +  0x04 0x9f
> +  0x05 0x80
> +  0x06 0x40
> +  0x07 0x00
> +  0x08 0x00
> +  0x09 0x00
> +
> +Set period and duty_cycle for DIM0 mode:
> +  $ echo 0x02 0x4f > reg
> +  $ echo 0x03 0x0c > reg
> +
> +Set LED0 mode to DIM0
> +  $ echo 0x06 0x02 > reg
> +
> +Registers 0x08 and 0x09 are not used for led control but are still valid to
> +store arbitrary bytes :)
> +  $ echo 0x08 0xde > reg
> +  $ echo 0x09 0xad > reg

This interface is pretty pointless if you ask me. The whole point of
the driver is to abstract things. If people want raw register access
then they should just use the i2cget and i2cset user-space tools
instead of your driver.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH v2] Driver for NS LP3944 FunLight Chip

[Antonio Ospite]

On Wed, 25 Jun 2008 20:14:09 +0200
Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:

> Hi Antonio,
>

Hi Jean, thanks for all the comments.

This is the first kernel driver I write from scratch and the code
reflects my inexperience. Yes, I know that there are also some more
general style issues, I am still learning :)

> On Wed, 18 Jun 2008 09:46:45 +0200, Antonio Ospite wrote:
>
> > Index: linux-2.6.26-rc6/drivers/i2c/chips/lp3944.c
> > ===================================================================
> > --- /dev/null
> > +++ linux-2.6.26-rc6/drivers/i2c/chips/lp3944.c
> 
> Led drivers have their dedicated subsystems. They go to drivers/leds.
> Please check MAINTAINERS for the maintainer of this subsystem and send
> your patch to that person.
> 
> (To make it clear: I'm not taking this driver under drivers/i2c/chips.
> I'm trying to empty this directory, chip drivers must go to their
> respective subsystems.
>

Ok, I'll address the issues you pointed out and resubmit for review to the
right maintainer, but let's have another small talk on some topics before I
leave.

> Random comments:
> 
> > +#define LP3944_REG_INPUT1	0x00	/* LEDs 0-7 InputRegister (Read Only) */
> > +#define LP3944_REG_REGISTER1	0x01	/* None (Read Only) */
> > +#define LP3944_REG_PSC0		0x02	/* Frequency Prescaler 0 (R/W) */
> > +#define LP3944_REG_PWM0		0x03	/* PWM Register 0 (R/W) */
> > +#define LP3944_REG_PSC1		0x04	/* Frequency Prescaler 1 (R/W) */
> > +#define LP3944_REG_PWM1		0x05	/* PWM Register 1 (R/W) */
> > +#define LP3944_REG_LS0		0x06	/* LEDs 0-3 Selector (R/W) */
> > +#define LP3944_REG_LS1		0x07	/* LEDs 4-7 Selector (R/W) */
> > +#define LP3944_REG_REGISTER8	0x08	/* None (R/W) */
> > +#define LP3944_REG_REGISTER9	0x09	/* None (R/W) */
> 
> None? Why do you care about these registers at all then?
>

I could still access them with the sysfs interface, and store arbitrary
values inside them, but as you stated, this interface is pretty useless,
so also these two can go away. 

> > +int lp3944_dim_set_period(unsigned dim, unsigned period)
> > +{
> > +	unsigned psc_reg;
> > +	unsigned psc_value;
> > +	int err;
> > +
> > +	if (dim == LP3944_DIM0)
> > +		psc_reg = LP3944_REG_PSC0;
> > +	else if (dim == LP3944_DIM1)
> > +		psc_reg = LP3944_REG_PSC1;
> > +	else
> > +		return -EINVAL;
> > +
> > +	/* Convert period to Prescaler value */
> > +	if (period > LP3944_PERIOD_MAX)
> > +		return -EINVAL;
> > +
> > +	if (period == LP3944_PERIOD_MIN)
> > +		psc_value = 0;
> > +	else
> > +		psc_value = (period * LP3944_PERIOD_MAX) - 1;
> 
> That's pretty confusing. Why would you want to multiply a period value
> by another period value? Maybe it happens to be numerically correct but
> from a physical unit point of view that doesn't make much sense.
>

Well, you're right, to be dimensionally correct it can be
psc_value = (period * 255) / LP3944_PERIOD_MAX;
which is a more conventional way to scale values and which saves me also the
if(), many thanks again.
I really should take more care to dimensional analysis.

The previous formula was just by chance numerically equivalent to this
one, because LP3944_PERIOD_MAX is 16 which is equal to 256/16...

> > +
> > +	mutex_lock(&lp3944->lock);
> > +	err = lp3944_reg_write(lp3944->client, psc_reg, psc_value);
> > +	mutex_unlock(&lp3944->lock);
> 
> What are you trying to protect yourself from with this locking?
>

I must admit I was in doubt about this one, and the following one. Can I
consider my lp3944_reg_write and lp3944_reg_read as "atomic" operations
because they simply call the i2c_smbus_*_byte_data functions which are
already safe?

> > + */
> > +int lp3944_led_set(unsigned led, unsigned status)
> > +{
> > +	unsigned reg;
> > +	unsigned val;
> > +	int err;
> > +
> > +	switch (led) {
> > +	case LP3944_LED0:
> > +	case LP3944_LED1:
> > +	case LP3944_LED2:
> > +	case LP3944_LED3:
> > +		reg = LP3944_REG_LS0;
> > +		break;
> > +	case LP3944_LED4:
> > +	case LP3944_LED5:
> > +	case LP3944_LED6:
> > +	case LP3944_LED7:
> > +		led -= LP3944_LED4;
> > +		reg = LP3944_REG_LS1;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (status > LP3944_LED_STATUS_DIM1)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&lp3944->lock);
> > +	lp3944_reg_read(lp3944->client, reg, &val);
> > +
> > +	val &= ~(LP3944_LED_STATUS_MASK << (led << 1));
> > +	val |= (status << (led << 1));
> > +
> > +	pr_debug("%s: led %d, status %d, val: 0x%02x\n",
> > +		 __func__, led, status, val);
> > +
> > +	/* set led status */
> > +	err = lp3944_reg_write(lp3944->client, reg, val);
> > +	mutex_unlock(&lp3944->lock);
> > +
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(lp3944_led_set);
> >

Just to be sure:
the locking is needed here because I am _updating_ a value and I need
explicit consistency, right? When I simply read or write "atomically" I
have no problem, is that what you meant before?

> > +/* low-level sysfs interface */
[...]
> > +static DEVICE_ATTR(reg, S_IWUSR | S_IRUGO, lp3944_get_reg, lp3944_set_reg);
> 
> I would get rid of this interface altogether, it doesn't make much
> sense.
>

Ok, will do that.

> > +	pr_debug("%s: reg: 0x%02x value: 0x%02x\n", __func__, reg, *value);
> > +
> > +	return 0;
> > +}
> > +
> > +static int lp3944_reg_write(struct i2c_client *client, unsigned reg,
> > +			    unsigned value)
> > +{
> > +	unsigned tmp;
> > +	int ret;
> > +
> > +	tmp = i2c_smbus_read_byte_data(client, reg);
> > +	pr_debug("%s: before write reg: 0x%02x value: 0x%02x\n",
> > +		 __func__, reg, tmp);
> > +
> > +	ret = i2c_smbus_write_byte_data(client, reg, value);
> > +
> > +	tmp = i2c_smbus_read_byte_data(client, reg);
> > +	pr_debug("%s: after write reg: 0x%02x value: 0x%02x\n",
> > +		 __func__, reg, tmp);
> 
> That's fairly inefficient. Is there any reason why you have to read
> from the register twice as you write to it? I2C is slow enough as is.
> If that's only for debugging, then you should make it conditional upon
> DEBUG being defined. But quite frankly this doesn't seem to be very
> useful debugging.
> 

yes, these prints and also the sysfs interface were used during
development (when I started I was even more inexperienced, you know).
I used them also to discover the leds configuration, I agree they can
go away now.

> > +
> > +	return ret;
> > +}
> > +
> > +static int lp3944_init_client(struct i2c_client *client)
> > +{
> > +	struct lp3944_data *data = i2c_get_clientdata(client);
> > +	int tmp;
> > +
> > +	/* Try to read a byte, if it fails, return an error */
> > +	tmp = i2c_smbus_read_byte_data(client, LP3944_REG_INPUT1);
> > +	if (tmp < 0)
> > +		return -ENODEV;
> 
> What's the point of doing this?
>

Just a stupid test to see if a chip is here? Useless, eh?
I'll trust the subsystems in future, thanks for making me reflect about
that.

> > +
> > +	/* Default values, taken from official datasheet */
> 
> If these are the default values, why do you write them to the chip?
> Instead of, say, just use what the registers hold, so you don't
> overwrite previous configuration done by the BIOS or firmware?
> 
> > +	lp3944_reg_write(client, LP3944_REG_PSC0, 0x00);
> > +	lp3944_reg_write(client, LP3944_REG_PWM0, 0x80);
> > +	lp3944_reg_write(client, LP3944_REG_PSC1, 0x00);
> > +	lp3944_reg_write(client, LP3944_REG_PWM1, 0x80);
> > +	lp3944_reg_write(client, LP3944_REG_LS0, 0x00);
> > +	lp3944_reg_write(client, LP3944_REG_LS1, 0x00);
> > +
> > +	data->LP3944_REGS[0] = 0x00;
> > +	data->LP3944_REGS[1] = 0x80;
> > +	data->LP3944_REGS[2] = 0x00;
> > +	data->LP3944_REGS[3] = 0x80;
> > +	data->LP3944_REGS[4] = 0x00;
> > +	data->LP3944_REGS[5] = 0x00;
> 
> Why do you write these values in data->LP3944_REGS? You never access
> them after that.
> 

Now I get it, I already have that functionality covered by suspend(), yes, it
is not needed on init.

> > +
> > +	if (lp3944) {
> > +		dev_dbg(&client->dev, "only one lp3944 chip allowed.\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	/* Let's see whether this adapter can support what we need. */
> > +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> > +		printk(KERN_ERR "%s: insufficient functionality!\n", __func__);
> > +		err = -EIO;
> 
> That's not an I/O error, so I'd rather return -ENODEV. Not that I
> expect this check to ever fail anyway.
> 

So, if you know that single byte-transfer is universally available, and
it is quite safe to assume that, indeed, I can just remove that part.

> > +
> > +	lp3944_reg_write(client, LP3944_REG_LS0, 0x00);
> > +	lp3944_reg_write(client, LP3944_REG_LS1, 0x00);
> 
> Why are these commands doing? Please add a comment.
> 

They turn leds off, but maybe I have to handle that in the leds-class
driver, because a led could be wired also to stay ON when lp3944 put
it in LP3944_LED_STATUS_OFF.

> > +	.suspend = lp3944_suspend,
> > +	.resume  = lp3944_resume,
> 
> This is missing a .id_table for i2c-core to do the device/driver
> matching (probably this didn't exist yet when you wrote your driver.)
> 

I'll check that too.

> > +#define LP3944_LED0 0
> > +#define LP3944_LED1 1
> > +#define LP3944_LED2 2
> > +#define LP3944_LED3 3
> > +#define LP3944_LED4 4
> > +#define LP3944_LED5 5
> > +#define LP3944_LED6 6
> > +#define LP3944_LED7 7
> > +
> > +#define LP3944_DIM0 0
> > +#define LP3944_DIM1 1
> 
> These defines don't look terribly useful.
> 

Right, but I'll keep them, they are just to make the user code more
readable (according to my taste, at least).

> > +		},
> > +	};
> > +
> > +and register it in the platform init function
> > +
> > +	i2c_register_board_info(0, a910_i2c_board_info,
> > +			ARRAY_SIZE(a910_i2c_board_info));
> > +
> > +
> 
> This is only one way to use register a device. i2c_new_device() is
> another.
>

I'll add that to the documentation.

> > +Accessing LP3944 via exported interface
> > +---------------------------------------
> > +The driver exposes a friendly interface to abstract the chip functionalities
> > +for other drivers to use.
> > +
> > +Including include/linux/i2c/lp3944.h one can use these calls:
> 
> Probably better written "Including <linux/i2c/lp3944.h> ...".
> 
> > +
> > +extern int lp3944_dim_set_period(unsigned dim, unsigned period);
> > +extern int lp3944_dim_set_dutycycle(unsigned int dim, unsigned duty_cycle);
> > +extern int lp3944_led_set(unsigned led, unsigned status);
> > +
> > +in a leds-class driver.
> 
> Apparently this implies that your driver (or at least this interface
> thereof) can only support one device? That's unfortunate.
>

How can I support multiple devices? Passing a i2c_client variable I
guess, but then, how can I get the client, say, inside a leds-class
driver before I can talk to it?

> > +
> > +
> > +Accessing LP3944 via /sys interface
> > +-----------------------------------
> > +Standard i2c sysfs interface can be found at
> 
> What "standard i2c sysfs interface" are you talking about?
> 

I meant Standard sysfs conventions for <bus>-<addr>, but the whole sysfs
interface will go away as you suggested.

Thanks for the review.

Regards,
   Antonio

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH v2] Driver for NS LP3944 FunLight Chip

[Jean Delvare]

Hi Antonio,

On Fri, 27 Jun 2008 17:56:49 +0200, Antonio Ospite wrote:
> On Wed, 25 Jun 2008 20:14:09 +0200, Jean Delvare wrote:
> > On Wed, 18 Jun 2008 09:46:45 +0200, Antonio Ospite wrote:
> > > +	mutex_lock(&lp3944->lock);
> > > +	err = lp3944_reg_write(lp3944->client, psc_reg, psc_value);
> > > +	mutex_unlock(&lp3944->lock);
> > 
> > What are you trying to protect yourself from with this locking?
> 
> I must admit I was in doubt about this one, and the following one. Can I
> consider my lp3944_reg_write and lp3944_reg_read as "atomic" operations
> because they simply call the i2c_smbus_*_byte_data functions which are
> already safe?

Yes. There's a mutex protecting each i2c_adapter, so only one
transaction can be running at a given time (from the Linux host, at
least.)

> > > + */
> > > +int lp3944_led_set(unsigned led, unsigned status)
> > > +{
> > > +	unsigned reg;
> > > +	unsigned val;
> > > +	int err;
> > > +
> > > +	switch (led) {
> > > +	case LP3944_LED0:
> > > +	case LP3944_LED1:
> > > +	case LP3944_LED2:
> > > +	case LP3944_LED3:
> > > +		reg = LP3944_REG_LS0;
> > > +		break;
> > > +	case LP3944_LED4:
> > > +	case LP3944_LED5:
> > > +	case LP3944_LED6:
> > > +	case LP3944_LED7:
> > > +		led -= LP3944_LED4;
> > > +		reg = LP3944_REG_LS1;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (status > LP3944_LED_STATUS_DIM1)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&lp3944->lock);
> > > +	lp3944_reg_read(lp3944->client, reg, &val);
> > > +
> > > +	val &= ~(LP3944_LED_STATUS_MASK << (led << 1));
> > > +	val |= (status << (led << 1));
> > > +
> > > +	pr_debug("%s: led %d, status %d, val: 0x%02x\n",
> > > +		 __func__, led, status, val);
> > > +
> > > +	/* set led status */
> > > +	err = lp3944_reg_write(lp3944->client, reg, val);
> > > +	mutex_unlock(&lp3944->lock);
> > > +
> > > +	return err;
> > > +}
> > > +EXPORT_SYMBOL_GPL(lp3944_led_set);
> > >
> 
> Just to be sure:
> the locking is needed here because I am _updating_ a value and I need
> explicit consistency, right? When I simply read or write "atomically" I
> have no problem, is that what you meant before?

Correct. Locking might not even be needed in the case of
read-modify-write operations, if that's the only function accessing
this register and the modified bits are always the same. But I guess
it's OK to leave the locking in place, in case the driver is extended
in the future, or if user-space might access the device directly too.

> > > +	/* Let's see whether this adapter can support what we need. */
> > > +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> > > +		printk(KERN_ERR "%s: insufficient functionality!\n", __func__);
> > > +		err = -EIO;
> > 
> > That's not an I/O error, so I'd rather return -ENODEV. Not that I
> > expect this check to ever fail anyway.
> 
> So, if you know that single byte-transfer is universally available, and
> it is quite safe to assume that, indeed, I can just remove that part.

Functionality checking was mainly needed for "legacy" drivers which
were probing buses in search of supported devices. For new-style
drivers like this one, which rely on platform code explicitly creating
devices where it knows they exist, it's somewhat redundant, indeed. But
it also doesn't hurt so I am fine if people leave the check in place.
But returning -EIO in the unlikely event the check file, is IMHO
confusing because no I/O has been actually performed. That's why I
suggested -ENODEV (but -EOPNOTSUPP or -EPROTONOSUPPORT would do as
well.)

> > (...)
> > Apparently this implies that your driver (or at least this interface
> > thereof) can only support one device? That's unfortunate.
> 
> How can I support multiple devices? Passing a i2c_client variable I
> guess, but then, how can I get the client, say, inside a leds-class
> driver before I can talk to it?

Question is, why do you have separate drivers for the i2c interface and
the leds-class parts in the first place? Just put everything in the
same driver and you're problem is solved, I think.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH v2] Driver for NS LP3944 FunLight Chip

[Antonio Ospite]

On Fri, 27 Jun 2008 22:42:56 +0200
Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:

> Hi Antonio,
> 
> > > (...)
> > > Apparently this implies that your driver (or at least this interface
> > > thereof) can only support one device? That's unfortunate.
> > 
> > How can I support multiple devices? Passing a i2c_client variable I
> > guess, but then, how can I get the client, say, inside a leds-class
> > driver before I can talk to it?
> 
> Question is, why do you have separate drivers for the i2c interface and
> the leds-class parts in the first place? Just put everything in the
> same driver and you're problem is solved, I think.
> 

Well, I separated the two to share the chip specific code with other
future possible users. AFAIU leds-class drivers are meant to provide a
high level interface to userspace. LP3944_LED0 can be the red led on
Motorola A910 but it could well be the blue led on a different machine
which uses lp3944 chip. That's why.

Regards,
   Antonio Ospite

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c