[RFC] [PATCH] misc : ROHM BH1780GLI Ambient light sensor Driver

[RFC] [PATCH] misc : ROHM BH1780GLI Ambient light sensor Driver

[Hemanth V]

From: Hemanth V <hemanthv@ti.com>
Date: Fri, 21 May 2010 15:52:03 +0530
Subject: [PATCH] misc: ROHM BH1780GLI Ambient light sensor Driver

This patch adds support for ROHM BH1780GLI Ambient light sensor.

BH1780 supports I2C interface. Driver supports read/update of power state and
read of lux value (through SYSFS). Writing value 3 to power_state enables the
sensor and current lux value could be read.

Signed-off-by: Hemanth V <hemanthv@ti.com>
---
 drivers/misc/Kconfig     |   11 ++
 drivers/misc/Makefile    |    1 +
 drivers/misc/bh1780gli.c |  278 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 290 insertions(+), 0 deletions(-)
 create mode 100644 drivers/misc/bh1780gli.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 0d0d625..0687a0c 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -278,6 +278,17 @@ config SENSORS_TSL2550
 	  This driver can also be built as a module.  If so, the module
 	  will be called tsl2550.

+config SENSORS_BH1780
+	tristate "ROHM BH1780GLI ambient light sensor"
+	depends on I2C && SYSFS
+	help
+	  If you say yes here you get support for the ROHM BH1780GLI
+	  ambient light sensor.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called bh1780gli.
+
+
 config EP93XX_PWM
 	tristate "EP93xx PWM support"
 	depends on ARCH_EP93XX
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 7b6f7ee..c479d91 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -30,3 +30,4 @@ obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
 obj-y				+= eeprom/
 obj-y				+= cb710/
 obj-$(CONFIG_VMWARE_BALLOON)	+= vmware_balloon.o
+obj-$(CONFIG_SENSORS_BH1780)	+= bh1780gli.o
diff --git a/drivers/misc/bh1780gli.c b/drivers/misc/bh1780gli.c
new file mode 100644
index 0000000..9b4137d
--- /dev/null
+++ b/drivers/misc/bh1780gli.c
@@ -0,0 +1,278 @@
+/*
+ * bh1780gli.c
+ * ROHM Ambient Light Sensor Driver
+ *
+ * Copyright (C) 2010 Texas Instruments
+ * Author: Hemanth V <hemanthv@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+
+#define BH1780_REG_CONTROL     0x80
+#define BH1780_REG_PARTID      0x8A
+#define BH1780_REG_MANFID      0x8B
+#define BH1780_REG_DLOW        0x8C
+#define BH1780_REG_DHIGH       0x8D
+
+#define BH1780_REVMASK        (0xf)
+#define BH1780_POWMASK        (0x3)
+#define BH1780_POFF	      (0x0)
+#define BH1780_PON            (0x3)
+
+/* power on settling time in ms */
+#define BH1780_PON_DELAY      2
+
+struct bh1780_data {
+	struct i2c_client *client;
+	int power_state;
+	/* lock for sysfs operations */
+	struct mutex lock;
+};
+
+int bh1780_write(struct bh1780_data *ddata, u8 reg, u8 val, char *msg)
+{
+	int ret = i2c_smbus_write_byte_data(ddata->client, reg, val);
+	if (ret < 0)
+		dev_err(&ddata->client->dev,
+			"i2c_smbus_write_byte_data failed error %d\
+			Register (%s)\n", ret, msg);
+	return ret;
+}
+
+int bh1780_read(struct bh1780_data *ddata, u8 reg, char *msg)
+{
+	int ret = i2c_smbus_read_byte_data(ddata->client, reg);
+	if (ret < 0)
+		dev_err(&ddata->client->dev,
+			"i2c_smbus_read_byte_data failed error %d\
+			 Register (%s)\n", ret, msg);
+	return ret;
+}
+
+static ssize_t bh1780_show_lux(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+
+	struct platform_device *pdev = to_platform_device(dev);
+	struct bh1780_data *ddata = platform_get_drvdata(pdev);
+	int lsb, msb;
+
+	lsb = bh1780_read(ddata, BH1780_REG_DLOW, "DLOW");
+	if (lsb < 0)
+		return lsb;
+
+	msb = bh1780_read(ddata, BH1780_REG_DHIGH, "DHIGH");
+	if (msb < 0)
+		return msb;
+
+	return sprintf(buf, "%d\n", (msb << 8) | lsb);
+}
+
+static ssize_t bh1780_show_power_state(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct bh1780_data *ddata = platform_get_drvdata(pdev);
+	int state;
+
+	state = bh1780_read(ddata, BH1780_REG_CONTROL, "CONTROL");
+	if (state < 0)
+		return state;
+
+	return sprintf(buf, "%d\n", state & BH1780_POWMASK);
+}
+
+static ssize_t bh1780_store_power_state(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct bh1780_data *ddata = platform_get_drvdata(pdev);
+	unsigned long val;
+	int error;
+
+	error = strict_strtoul(buf, 0, &val);
+	if (error)
+		return error;
+
+	if (val < BH1780_POFF || val > BH1780_PON)
+		return -EINVAL;
+
+	mutex_lock(&ddata->lock);
+
+	error = bh1780_write(ddata, BH1780_REG_CONTROL, val, "CONTROL");
+	if (error < 0) {
+		mutex_unlock(&ddata->lock);
+		return error;
+	}
+
+	msleep(BH1780_PON_DELAY);
+	ddata->power_state = val;
+	mutex_unlock(&ddata->lock);
+	return count;
+
+}
+
+static DEVICE_ATTR(lux, S_IRUGO, bh1780_show_lux, NULL);
+
+static DEVICE_ATTR(power_state, S_IWUSR | S_IRUGO,
+		bh1780_show_power_state, bh1780_store_power_state);
+
+static struct attribute *bh1780_attributes[] = {
+	&dev_attr_power_state.attr,
+	&dev_attr_lux.attr,
+	NULL
+};
+
+static const struct attribute_group bh1780_attr_group = {
+	.attrs = bh1780_attributes,
+};
+
+static int __devinit bh1780_probe(struct i2c_client *client,
+						const struct i2c_device_id *id)
+{
+	int ret;
+	struct bh1780_data *ddata = NULL;
+	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) {
+		ret = -EIO;
+		goto err_op_failed;
+	}
+
+	ddata = kzalloc(sizeof(struct bh1780_data), GFP_KERNEL);
+	if (ddata == NULL) {
+		ret = -ENOMEM;
+		goto err_op_failed;
+	}
+
+	ddata->client = client;
+	i2c_set_clientdata(client, ddata);
+
+	ret = bh1780_read(ddata, BH1780_REG_PARTID, "PART ID");
+	if (ret < 0)
+		goto err_op_failed;
+
+	dev_info(&client->dev, "Ambient Light Sensor, Rev : %d\n",
+			(ret & BH1780_REVMASK));
+
+	mutex_init(&ddata->lock);
+
+	ret = sysfs_create_group(&client->dev.kobj, &bh1780_attr_group);
+	if (ret)
+		goto err_op_failed;
+
+	return 0;
+
+err_op_failed:
+	if (ddata != NULL)
+		kfree(ddata);
+
+	return ret;
+}
+
+static int __devexit bh1780_remove(struct i2c_client *client)
+{
+	struct bh1780_data *ddata;
+
+	ddata = i2c_get_clientdata(client);
+	sysfs_remove_group(&client->dev.kobj, &bh1780_attr_group);
+
+	if (ddata != NULL) {
+		i2c_set_clientdata(client, NULL);
+		kfree(ddata);
+	}
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int bh1780_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+	struct bh1780_data *ddata;
+	int state, ret;
+
+	ddata = i2c_get_clientdata(client);
+	state = bh1780_read(ddata, BH1780_REG_CONTROL, "CONTROL");
+	if (state < 0)
+		return state;
+
+	ddata->power_state = state & BH1780_POWMASK;
+
+	ret = bh1780_write(ddata, BH1780_REG_CONTROL, BH1780_POFF,
+				"CONTROL");
+
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int bh1780_resume(struct i2c_client *client)
+{
+	struct bh1780_data *ddata;
+	int state, ret;
+
+	ddata = i2c_get_clientdata(client);
+	state = ddata->power_state;
+
+	ret = bh1780_write(ddata, BH1780_REG_CONTROL, state,
+				"CONTROL");
+
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+#endif
+
+static const struct i2c_device_id bh1780_id[] = {
+	{ "bh1780", 0 },
+	{ },
+};
+
+static struct i2c_driver bh1780_driver = {
+	.probe		= bh1780_probe,
+	.remove		= bh1780_remove,
+	.id_table	= bh1780_id,
+#ifdef CONFIG_PM
+	.suspend	= bh1780_suspend,
+	.resume		= bh1780_resume,
+#endif
+	.driver = {
+		.name = "bh1780"
+	},
+};
+
+static int __init bh1780_init(void)
+{
+	return i2c_add_driver(&bh1780_driver);
+}
+
+static void __exit bh1780_exit(void)
+{
+	i2c_del_driver(&bh1780_driver);
+}
+
+module_init(bh1780_init)
+module_exit(bh1780_exit)
+
+MODULE_DESCRIPTION("BH1780GLI Ambient Light Sensor Driver");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Hemanth V <hemanthv@ti.com>");
-- 
1.5.6.3

[RFC] [PATCH] misc : ROHM BH1780GLI Ambient light sensor Driver

[Hemanth V]

From: Hemanth V <hemanthv@ti.com>
Date: Fri, 21 May 2010 15:52:03 +0530
Subject: [PATCH] misc: ROHM BH1780GLI Ambient light sensor Driver

Corrected the mailing list, should be linux-kernel

This patch adds support for ROHM BH1780GLI Ambient light sensor.

BH1780 supports I2C interface. Driver supports read/update of power state and
read of lux value (through SYSFS). Writing value 3 to power_state enables the
sensor and current lux value could be read.

Signed-off-by: Hemanth V <hemanthv@ti.com>
---
 drivers/misc/Kconfig     |   11 ++
 drivers/misc/Makefile    |    1 +
 drivers/misc/bh1780gli.c |  278 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 290 insertions(+), 0 deletions(-)
 create mode 100644 drivers/misc/bh1780gli.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 0d0d625..0687a0c 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -278,6 +278,17 @@ config SENSORS_TSL2550
 	  This driver can also be built as a module.  If so, the module
 	  will be called tsl2550.

+config SENSORS_BH1780
+	tristate "ROHM BH1780GLI ambient light sensor"
+	depends on I2C && SYSFS
+	help
+	  If you say yes here you get support for the ROHM BH1780GLI
+	  ambient light sensor.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called bh1780gli.
+
+
 config EP93XX_PWM
 	tristate "EP93xx PWM support"
 	depends on ARCH_EP93XX
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 7b6f7ee..c479d91 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -30,3 +30,4 @@ obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
 obj-y				+= eeprom/
 obj-y				+= cb710/
 obj-$(CONFIG_VMWARE_BALLOON)	+= vmware_balloon.o
+obj-$(CONFIG_SENSORS_BH1780)	+= bh1780gli.o
diff --git a/drivers/misc/bh1780gli.c b/drivers/misc/bh1780gli.c
new file mode 100644
index 0000000..9b4137d
--- /dev/null
+++ b/drivers/misc/bh1780gli.c
@@ -0,0 +1,278 @@
+/*
+ * bh1780gli.c
+ * ROHM Ambient Light Sensor Driver
+ *
+ * Copyright (C) 2010 Texas Instruments
+ * Author: Hemanth V <hemanthv@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+
+#define BH1780_REG_CONTROL     0x80
+#define BH1780_REG_PARTID      0x8A
+#define BH1780_REG_MANFID      0x8B
+#define BH1780_REG_DLOW        0x8C
+#define BH1780_REG_DHIGH       0x8D
+
+#define BH1780_REVMASK        (0xf)
+#define BH1780_POWMASK        (0x3)
+#define BH1780_POFF	      (0x0)
+#define BH1780_PON            (0x3)
+
+/* power on settling time in ms */
+#define BH1780_PON_DELAY      2
+
+struct bh1780_data {
+	struct i2c_client *client;
+	int power_state;
+	/* lock for sysfs operations */
+	struct mutex lock;
+};
+
+int bh1780_write(struct bh1780_data *ddata, u8 reg, u8 val, char *msg)
+{
+	int ret = i2c_smbus_write_byte_data(ddata->client, reg, val);
+	if (ret < 0)
+		dev_err(&ddata->client->dev,
+			"i2c_smbus_write_byte_data failed error %d\
+			Register (%s)\n", ret, msg);
+	return ret;
+}
+
+int bh1780_read(struct bh1780_data *ddata, u8 reg, char *msg)
+{
+	int ret = i2c_smbus_read_byte_data(ddata->client, reg);
+	if (ret < 0)
+		dev_err(&ddata->client->dev,
+			"i2c_smbus_read_byte_data failed error %d\
+			 Register (%s)\n", ret, msg);
+	return ret;
+}
+
+static ssize_t bh1780_show_lux(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+
+	struct platform_device *pdev = to_platform_device(dev);
+	struct bh1780_data *ddata = platform_get_drvdata(pdev);
+	int lsb, msb;
+
+	lsb = bh1780_read(ddata, BH1780_REG_DLOW, "DLOW");
+	if (lsb < 0)
+		return lsb;
+
+	msb = bh1780_read(ddata, BH1780_REG_DHIGH, "DHIGH");
+	if (msb < 0)
+		return msb;
+
+	return sprintf(buf, "%d\n", (msb << 8) | lsb);
+}
+
+static ssize_t bh1780_show_power_state(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct bh1780_data *ddata = platform_get_drvdata(pdev);
+	int state;
+
+	state = bh1780_read(ddata, BH1780_REG_CONTROL, "CONTROL");
+	if (state < 0)
+		return state;
+
+	return sprintf(buf, "%d\n", state & BH1780_POWMASK);
+}
+
+static ssize_t bh1780_store_power_state(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct bh1780_data *ddata = platform_get_drvdata(pdev);
+	unsigned long val;
+	int error;
+
+	error = strict_strtoul(buf, 0, &val);
+	if (error)
+		return error;
+
+	if (val < BH1780_POFF || val > BH1780_PON)
+		return -EINVAL;
+
+	mutex_lock(&ddata->lock);
+
+	error = bh1780_write(ddata, BH1780_REG_CONTROL, val, "CONTROL");
+	if (error < 0) {
+		mutex_unlock(&ddata->lock);
+		return error;
+	}
+
+	msleep(BH1780_PON_DELAY);
+	ddata->power_state = val;
+	mutex_unlock(&ddata->lock);
+	return count;
+
+}
+
+static DEVICE_ATTR(lux, S_IRUGO, bh1780_show_lux, NULL);
+
+static DEVICE_ATTR(power_state, S_IWUSR | S_IRUGO,
+		bh1780_show_power_state, bh1780_store_power_state);
+
+static struct attribute *bh1780_attributes[] = {
+	&dev_attr_power_state.attr,
+	&dev_attr_lux.attr,
+	NULL
+};
+
+static const struct attribute_group bh1780_attr_group = {
+	.attrs = bh1780_attributes,
+};
+
+static int __devinit bh1780_probe(struct i2c_client *client,
+						const struct i2c_device_id *id)
+{
+	int ret;
+	struct bh1780_data *ddata = NULL;
+	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) {
+		ret = -EIO;
+		goto err_op_failed;
+	}
+
+	ddata = kzalloc(sizeof(struct bh1780_data), GFP_KERNEL);
+	if (ddata == NULL) {
+		ret = -ENOMEM;
+		goto err_op_failed;
+	}
+
+	ddata->client = client;
+	i2c_set_clientdata(client, ddata);
+
+	ret = bh1780_read(ddata, BH1780_REG_PARTID, "PART ID");
+	if (ret < 0)
+		goto err_op_failed;
+
+	dev_info(&client->dev, "Ambient Light Sensor, Rev : %d\n",
+			(ret & BH1780_REVMASK));
+
+	mutex_init(&ddata->lock);
+
+	ret = sysfs_create_group(&client->dev.kobj, &bh1780_attr_group);
+	if (ret)
+		goto err_op_failed;
+
+	return 0;
+
+err_op_failed:
+	if (ddata != NULL)
+		kfree(ddata);
+
+	return ret;
+}
+
+static int __devexit bh1780_remove(struct i2c_client *client)
+{
+	struct bh1780_data *ddata;
+
+	ddata = i2c_get_clientdata(client);
+	sysfs_remove_group(&client->dev.kobj, &bh1780_attr_group);
+
+	if (ddata != NULL) {
+		i2c_set_clientdata(client, NULL);
+		kfree(ddata);
+	}
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int bh1780_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+	struct bh1780_data *ddata;
+	int state, ret;
+
+	ddata = i2c_get_clientdata(client);
+	state = bh1780_read(ddata, BH1780_REG_CONTROL, "CONTROL");
+	if (state < 0)
+		return state;
+
+	ddata->power_state = state & BH1780_POWMASK;
+
+	ret = bh1780_write(ddata, BH1780_REG_CONTROL, BH1780_POFF,
+				"CONTROL");
+
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int bh1780_resume(struct i2c_client *client)
+{
+	struct bh1780_data *ddata;
+	int state, ret;
+
+	ddata = i2c_get_clientdata(client);
+	state = ddata->power_state;
+
+	ret = bh1780_write(ddata, BH1780_REG_CONTROL, state,
+				"CONTROL");
+
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+#endif
+
+static const struct i2c_device_id bh1780_id[] = {
+	{ "bh1780", 0 },
+	{ },
+};
+
+static struct i2c_driver bh1780_driver = {
+	.probe		= bh1780_probe,
+	.remove		= bh1780_remove,
+	.id_table	= bh1780_id,
+#ifdef CONFIG_PM
+	.suspend	= bh1780_suspend,
+	.resume		= bh1780_resume,
+#endif
+	.driver = {
+		.name = "bh1780"
+	},
+};
+
+static int __init bh1780_init(void)
+{
+	return i2c_add_driver(&bh1780_driver);
+}
+
+static void __exit bh1780_exit(void)
+{
+	i2c_del_driver(&bh1780_driver);
+}
+
+module_init(bh1780_init)
+module_exit(bh1780_exit)
+
+MODULE_DESCRIPTION("BH1780GLI Ambient Light Sensor Driver");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Hemanth V <hemanthv@ti.com>");
-- 
1.5.6.3

Re: [RFC] [PATCH] misc : ROHM BH1780GLI Ambient light sensor Driver

[Daniel Mack]

Hi,

On Fri, May 21, 2010 at 05:05:50PM +0530, Hemanth V wrote:
> Corrected the mailing list, should be linux-kernel
> 
> This patch adds support for ROHM BH1780GLI Ambient light sensor.
> 
> BH1780 supports I2C interface. Driver supports read/update of power state and
> read of lux value (through SYSFS). Writing value 3 to power_state enables the
> sensor and current lux value could be read.

FWIW, this looks pretty good to me, just some minor things below.

Daniel

> Signed-off-by: Hemanth V <hemanthv@ti.com>
> ---
>  drivers/misc/Kconfig     |   11 ++
>  drivers/misc/Makefile    |    1 +
>  drivers/misc/bh1780gli.c |  278 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 290 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/misc/bh1780gli.c
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 0d0d625..0687a0c 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -278,6 +278,17 @@ config SENSORS_TSL2550
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called tsl2550.
> 
> +config SENSORS_BH1780
> +	tristate "ROHM BH1780GLI ambient light sensor"
> +	depends on I2C && SYSFS
> +	help
> +	  If you say yes here you get support for the ROHM BH1780GLI
> +	  ambient light sensor.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called bh1780gli.
> +
> +
>  config EP93XX_PWM
>  	tristate "EP93xx PWM support"
>  	depends on ARCH_EP93XX
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 7b6f7ee..c479d91 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -30,3 +30,4 @@ obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
>  obj-y				+= eeprom/
>  obj-y				+= cb710/
>  obj-$(CONFIG_VMWARE_BALLOON)	+= vmware_balloon.o
> +obj-$(CONFIG_SENSORS_BH1780)	+= bh1780gli.o
> diff --git a/drivers/misc/bh1780gli.c b/drivers/misc/bh1780gli.c
> new file mode 100644
> index 0000000..9b4137d
> --- /dev/null
> +++ b/drivers/misc/bh1780gli.c
> @@ -0,0 +1,278 @@
> +/*
> + * bh1780gli.c
> + * ROHM Ambient Light Sensor Driver
> + *
> + * Copyright (C) 2010 Texas Instruments
> + * Author: Hemanth V <hemanthv@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +
> +#define BH1780_REG_CONTROL     0x80
> +#define BH1780_REG_PARTID      0x8A
> +#define BH1780_REG_MANFID      0x8B
> +#define BH1780_REG_DLOW        0x8C
> +#define BH1780_REG_DHIGH       0x8D
> +
> +#define BH1780_REVMASK        (0xf)
> +#define BH1780_POWMASK        (0x3)
> +#define BH1780_POFF	      (0x0)
> +#define BH1780_PON            (0x3)

Just a nit ... something's wrong with the indentation here.

> +/* power on settling time in ms */
> +#define BH1780_PON_DELAY      2
> +
> +struct bh1780_data {
> +	struct i2c_client *client;
> +	int power_state;
> +	/* lock for sysfs operations */
> +	struct mutex lock;
> +};
> +
> +int bh1780_write(struct bh1780_data *ddata, u8 reg, u8 val, char *msg)

Should be static.

> +{
> +	int ret = i2c_smbus_write_byte_data(ddata->client, reg, val);
> +	if (ret < 0)
> +		dev_err(&ddata->client->dev,
> +			"i2c_smbus_write_byte_data failed error %d\
> +			Register (%s)\n", ret, msg);
> +	return ret;
> +}
> +
> +int bh1780_read(struct bh1780_data *ddata, u8 reg, char *msg)

Dito.

> +{
> +	int ret = i2c_smbus_read_byte_data(ddata->client, reg);
> +	if (ret < 0)
> +		dev_err(&ddata->client->dev,
> +			"i2c_smbus_read_byte_data failed error %d\
> +			 Register (%s)\n", ret, msg);
> +	return ret;
> +}
> +
> +static ssize_t bh1780_show_lux(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct bh1780_data *ddata = platform_get_drvdata(pdev);
> +	int lsb, msb;
> +
> +	lsb = bh1780_read(ddata, BH1780_REG_DLOW, "DLOW");
> +	if (lsb < 0)
> +		return lsb;
> +
> +	msb = bh1780_read(ddata, BH1780_REG_DHIGH, "DHIGH");
> +	if (msb < 0)
> +		return msb;
> +
> +	return sprintf(buf, "%d\n", (msb << 8) | lsb);
> +}
> +
> +static ssize_t bh1780_show_power_state(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct bh1780_data *ddata = platform_get_drvdata(pdev);
> +	int state;
> +
> +	state = bh1780_read(ddata, BH1780_REG_CONTROL, "CONTROL");
> +	if (state < 0)
> +		return state;
> +
> +	return sprintf(buf, "%d\n", state & BH1780_POWMASK);
> +}
> +
> +static ssize_t bh1780_store_power_state(struct device *dev,
> +					struct device_attribute *attr,
> +					const char *buf, size_t count)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct bh1780_data *ddata = platform_get_drvdata(pdev);
> +	unsigned long val;
> +	int error;
> +
> +	error = strict_strtoul(buf, 0, &val);
> +	if (error)
> +		return error;
> +
> +	if (val < BH1780_POFF || val > BH1780_PON)
> +		return -EINVAL;
> +
> +	mutex_lock(&ddata->lock);
> +
> +	error = bh1780_write(ddata, BH1780_REG_CONTROL, val, "CONTROL");
> +	if (error < 0) {
> +		mutex_unlock(&ddata->lock);
> +		return error;
> +	}
> +
> +	msleep(BH1780_PON_DELAY);

Hmm, what do you wait for here?

> +	ddata->power_state = val;
> +	mutex_unlock(&ddata->lock);

And I don't get the usage of the mutex here. Can you explain?

> +	return count;
> +
> +}

Remove the empty line. Or move it above the return statement.

> +
> +static DEVICE_ATTR(lux, S_IRUGO, bh1780_show_lux, NULL);
> +
> +static DEVICE_ATTR(power_state, S_IWUSR | S_IRUGO,
> +		bh1780_show_power_state, bh1780_store_power_state);
> +
> +static struct attribute *bh1780_attributes[] = {
> +	&dev_attr_power_state.attr,
> +	&dev_attr_lux.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group bh1780_attr_group = {
> +	.attrs = bh1780_attributes,
> +};
> +
> +static int __devinit bh1780_probe(struct i2c_client *client,
> +						const struct i2c_device_id *id)
> +{
> +	int ret;
> +	struct bh1780_data *ddata = NULL;

The initialization isn't needed.

> +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) {
> +		ret = -EIO;
> +		goto err_op_failed;
> +	}
> +
> +	ddata = kzalloc(sizeof(struct bh1780_data), GFP_KERNEL);
> +	if (ddata == NULL) {
> +		ret = -ENOMEM;
> +		goto err_op_failed;
> +	}
> +
> +	ddata->client = client;
> +	i2c_set_clientdata(client, ddata);
> +
> +	ret = bh1780_read(ddata, BH1780_REG_PARTID, "PART ID");
> +	if (ret < 0)
> +		goto err_op_failed;
> +
> +	dev_info(&client->dev, "Ambient Light Sensor, Rev : %d\n",
> +			(ret & BH1780_REVMASK));
> +
> +	mutex_init(&ddata->lock);
> +
> +	ret = sysfs_create_group(&client->dev.kobj, &bh1780_attr_group);
> +	if (ret)
> +		goto err_op_failed;
> +
> +	return 0;
> +
> +err_op_failed:
> +	if (ddata != NULL)
> +		kfree(ddata);

kfree() is resitant against NULL pointers, so you can call it
unconditionally.

> +
> +	return ret;
> +}
> +
> +static int __devexit bh1780_remove(struct i2c_client *client)
> +{
> +	struct bh1780_data *ddata;
> +
> +	ddata = i2c_get_clientdata(client);
> +	sysfs_remove_group(&client->dev.kobj, &bh1780_attr_group);
> +
> +	if (ddata != NULL) {
> +		i2c_set_clientdata(client, NULL);
> +		kfree(ddata);
> +	}

Same here.

> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int bh1780_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> +	struct bh1780_data *ddata;
> +	int state, ret;
> +
> +	ddata = i2c_get_clientdata(client);
> +	state = bh1780_read(ddata, BH1780_REG_CONTROL, "CONTROL");
> +	if (state < 0)
> +		return state;
> +
> +	ddata->power_state = state & BH1780_POWMASK;
> +
> +	ret = bh1780_write(ddata, BH1780_REG_CONTROL, BH1780_POFF,
> +				"CONTROL");
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int bh1780_resume(struct i2c_client *client)
> +{
> +	struct bh1780_data *ddata;
> +	int state, ret;
> +
> +	ddata = i2c_get_clientdata(client);
> +	state = ddata->power_state;
> +
> +	ret = bh1780_write(ddata, BH1780_REG_CONTROL, state,
> +				"CONTROL");
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}

#else
static inline int bh1780_suspend(struct i2c_client *client, pm_message_t mesg) {}
static inline int bh1780_resume(struct i2c_client *client) {}


> +#endif
> +
> +static const struct i2c_device_id bh1780_id[] = {
> +	{ "bh1780", 0 },
> +	{ },
> +};
> +
> +static struct i2c_driver bh1780_driver = {
> +	.probe		= bh1780_probe,
> +	.remove		= bh1780_remove,
> +	.id_table	= bh1780_id,
> +#ifdef CONFIG_PM

... then this #ifdef can go away.

> +	.suspend	= bh1780_suspend,
> +	.resume		= bh1780_resume,
> +#endif
> +	.driver = {
> +		.name = "bh1780"
> +	},
> +};
> +
> +static int __init bh1780_init(void)
> +{
> +	return i2c_add_driver(&bh1780_driver);
> +}
> +
> +static void __exit bh1780_exit(void)
> +{
> +	i2c_del_driver(&bh1780_driver);
> +}
> +
> +module_init(bh1780_init)
> +module_exit(bh1780_exit)
> +
> +MODULE_DESCRIPTION("BH1780GLI Ambient Light Sensor Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Hemanth V <hemanthv@ti.com>");
> -- 
> 1.5.6.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] [PATCH] misc : ROHM BH1780GLI Ambient light sensor Driver

[Jonathan Cameron]

On 05/21/10 12:35, Hemanth V wrote:
> From: Hemanth V <hemanthv@ti.com>
> Date: Fri, 21 May 2010 15:52:03 +0530
> Subject: [PATCH] misc: ROHM BH1780GLI Ambient light sensor Driver
> 
> Corrected the mailing list, should be linux-kernel
> 
> This patch adds support for ROHM BH1780GLI Ambient light sensor.
> 
> BH1780 supports I2C interface. Driver supports read/update of power state and
> read of lux value (through SYSFS). Writing value 3 to power_state enables the
> sensor and current lux value could be read.
Only comment here is that lux is the unit, but what we are measuring is
illuminance.  (see the whole debate that took place when working
out the abi for the now defunct Ambient Light Sensors (ALS) frameowrk.)
Hence I'd rather see the attribute being called illuminance.
Acutally on that note, after the many abi debates, it probably makes sense
to match hwmon conventions and call it, illuminance0_input.

Nice simple driver.

Up to you whether you change the attribute name.  I know there are various
namings in other drivers as things currently stand.  That was what
ALS was meant to clear up, but such is life.  We're standardising 
in IIO on the above and will probably eat all the als drivers
at somepoint given Linus isn't happy with them having their own subsystem.

Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> 
> Signed-off-by: Hemanth V <hemanthv@ti.com>
> ---
>  drivers/misc/Kconfig     |   11 ++
>  drivers/misc/Makefile    |    1 +
>  drivers/misc/bh1780gli.c |  278 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 290 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/misc/bh1780gli.c
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 0d0d625..0687a0c 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -278,6 +278,17 @@ config SENSORS_TSL2550
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called tsl2550.
> 
> +config SENSORS_BH1780
> +	tristate "ROHM BH1780GLI ambient light sensor"
> +	depends on I2C && SYSFS
> +	help
> +	  If you say yes here you get support for the ROHM BH1780GLI
> +	  ambient light sensor.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called bh1780gli.
> +
> +
>  config EP93XX_PWM
>  	tristate "EP93xx PWM support"
>  	depends on ARCH_EP93XX
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 7b6f7ee..c479d91 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -30,3 +30,4 @@ obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
>  obj-y				+= eeprom/
>  obj-y				+= cb710/
>  obj-$(CONFIG_VMWARE_BALLOON)	+= vmware_balloon.o
> +obj-$(CONFIG_SENSORS_BH1780)	+= bh1780gli.o
> diff --git a/drivers/misc/bh1780gli.c b/drivers/misc/bh1780gli.c
> new file mode 100644
> index 0000000..9b4137d
> --- /dev/null
> +++ b/drivers/misc/bh1780gli.c
> @@ -0,0 +1,278 @@
> +/*
> + * bh1780gli.c
> + * ROHM Ambient Light Sensor Driver
> + *
> + * Copyright (C) 2010 Texas Instruments
> + * Author: Hemanth V <hemanthv@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +
> +#define BH1780_REG_CONTROL     0x80
> +#define BH1780_REG_PARTID      0x8A
> +#define BH1780_REG_MANFID      0x8B
> +#define BH1780_REG_DLOW        0x8C
> +#define BH1780_REG_DHIGH       0x8D
> +
> +#define BH1780_REVMASK        (0xf)
> +#define BH1780_POWMASK        (0x3)
> +#define BH1780_POFF	      (0x0)
> +#define BH1780_PON            (0x3)
> +
> +/* power on settling time in ms */
> +#define BH1780_PON_DELAY      2
> +
> +struct bh1780_data {
> +	struct i2c_client *client;
> +	int power_state;
> +	/* lock for sysfs operations */
> +	struct mutex lock;
> +};
> +
> +int bh1780_write(struct bh1780_data *ddata, u8 reg, u8 val, char *msg)
> +{
> +	int ret = i2c_smbus_write_byte_data(ddata->client, reg, val);
> +	if (ret < 0)
> +		dev_err(&ddata->client->dev,
> +			"i2c_smbus_write_byte_data failed error %d\
> +			Register (%s)\n", ret, msg);
> +	return ret;
> +}
> +
> +int bh1780_read(struct bh1780_data *ddata, u8 reg, char *msg)
> +{
> +	int ret = i2c_smbus_read_byte_data(ddata->client, reg);
> +	if (ret < 0)
> +		dev_err(&ddata->client->dev,
> +			"i2c_smbus_read_byte_data failed error %d\
> +			 Register (%s)\n", ret, msg);
> +	return ret;
> +}
> +
> +static ssize_t bh1780_show_lux(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct bh1780_data *ddata = platform_get_drvdata(pdev);
> +	int lsb, msb;
> +
> +	lsb = bh1780_read(ddata, BH1780_REG_DLOW, "DLOW");
> +	if (lsb < 0)
> +		return lsb;
> +
> +	msb = bh1780_read(ddata, BH1780_REG_DHIGH, "DHIGH");
> +	if (msb < 0)
> +		return msb;
> +
> +	return sprintf(buf, "%d\n", (msb << 8) | lsb);
> +}
> +
> +static ssize_t bh1780_show_power_state(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct bh1780_data *ddata = platform_get_drvdata(pdev);
> +	int state;
> +
> +	state = bh1780_read(ddata, BH1780_REG_CONTROL, "CONTROL");
> +	if (state < 0)
> +		return state;
> +
> +	return sprintf(buf, "%d\n", state & BH1780_POWMASK);
> +}
> +
> +static ssize_t bh1780_store_power_state(struct device *dev,
> +					struct device_attribute *attr,
> +					const char *buf, size_t count)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct bh1780_data *ddata = platform_get_drvdata(pdev);
> +	unsigned long val;
> +	int error;
> +
> +	error = strict_strtoul(buf, 0, &val);
> +	if (error)
> +		return error;
> +
> +	if (val < BH1780_POFF || val > BH1780_PON)
> +		return -EINVAL;
> +
> +	mutex_lock(&ddata->lock);
> +
> +	error = bh1780_write(ddata, BH1780_REG_CONTROL, val, "CONTROL");
> +	if (error < 0) {
> +		mutex_unlock(&ddata->lock);
> +		return error;
> +	}
> +
> +	msleep(BH1780_PON_DELAY);
> +	ddata->power_state = val;
> +	mutex_unlock(&ddata->lock);
> +	return count;
> +
> +}
> +
> +static DEVICE_ATTR(lux, S_IRUGO, bh1780_show_lux, NULL);
> +
> +static DEVICE_ATTR(power_state, S_IWUSR | S_IRUGO,
> +		bh1780_show_power_state, bh1780_store_power_state);
> +
> +static struct attribute *bh1780_attributes[] = {
> +	&dev_attr_power_state.attr,
> +	&dev_attr_lux.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group bh1780_attr_group = {
> +	.attrs = bh1780_attributes,
> +};
> +
> +static int __devinit bh1780_probe(struct i2c_client *client,
> +						const struct i2c_device_id *id)
> +{
> +	int ret;
> +	struct bh1780_data *ddata = NULL;
> +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) {
> +		ret = -EIO;
> +		goto err_op_failed;
> +	}
> +
> +	ddata = kzalloc(sizeof(struct bh1780_data), GFP_KERNEL);
> +	if (ddata == NULL) {
> +		ret = -ENOMEM;
> +		goto err_op_failed;
> +	}
> +
> +	ddata->client = client;
> +	i2c_set_clientdata(client, ddata);
> +
> +	ret = bh1780_read(ddata, BH1780_REG_PARTID, "PART ID");
> +	if (ret < 0)
> +		goto err_op_failed;
> +
> +	dev_info(&client->dev, "Ambient Light Sensor, Rev : %d\n",
> +			(ret & BH1780_REVMASK));
> +
> +	mutex_init(&ddata->lock);
> +
> +	ret = sysfs_create_group(&client->dev.kobj, &bh1780_attr_group);
> +	if (ret)
> +		goto err_op_failed;
> +
> +	return 0;
> +
> +err_op_failed:
> +	if (ddata != NULL)
> +		kfree(ddata);
> +
> +	return ret;
> +}
> +
> +static int __devexit bh1780_remove(struct i2c_client *client)
> +{
> +	struct bh1780_data *ddata;
> +
> +	ddata = i2c_get_clientdata(client);
> +	sysfs_remove_group(&client->dev.kobj, &bh1780_attr_group);
> +
> +	if (ddata != NULL) {
> +		i2c_set_clientdata(client, NULL);
> +		kfree(ddata);
> +	}
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int bh1780_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> +	struct bh1780_data *ddata;
> +	int state, ret;
> +
> +	ddata = i2c_get_clientdata(client);
> +	state = bh1780_read(ddata, BH1780_REG_CONTROL, "CONTROL");
> +	if (state < 0)
> +		return state;
> +
> +	ddata->power_state = state & BH1780_POWMASK;
> +
> +	ret = bh1780_write(ddata, BH1780_REG_CONTROL, BH1780_POFF,
> +				"CONTROL");
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int bh1780_resume(struct i2c_client *client)
> +{
> +	struct bh1780_data *ddata;
> +	int state, ret;
> +
> +	ddata = i2c_get_clientdata(client);
> +	state = ddata->power_state;
> +
> +	ret = bh1780_write(ddata, BH1780_REG_CONTROL, state,
> +				"CONTROL");
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct i2c_device_id bh1780_id[] = {
> +	{ "bh1780", 0 },
> +	{ },
> +};
> +
> +static struct i2c_driver bh1780_driver = {
> +	.probe		= bh1780_probe,
> +	.remove		= bh1780_remove,
> +	.id_table	= bh1780_id,
> +#ifdef CONFIG_PM
> +	.suspend	= bh1780_suspend,
> +	.resume		= bh1780_resume,
> +#endif
> +	.driver = {
> +		.name = "bh1780"
> +	},
> +};
> +
> +static int __init bh1780_init(void)
> +{
> +	return i2c_add_driver(&bh1780_driver);
> +}
> +
> +static void __exit bh1780_exit(void)
> +{
> +	i2c_del_driver(&bh1780_driver);
> +}
> +
> +module_init(bh1780_init)
> +module_exit(bh1780_exit)
> +
> +MODULE_DESCRIPTION("BH1780GLI Ambient Light Sensor Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Hemanth V <hemanthv@ti.com>");

Re: [RFC] [PATCH] misc : ROHM BH1780GLI Ambient light sensor Driver

[Hemanth V]

----- Original Message ----- 
From: "Daniel Mack" <daniel@caiaq.de>
To: "Hemanth V" <hemanthv@ti.com>
Cc: <linux-kernel@vger.kernel.org>; <linux-omap@vger.kernel.org>; 
<linux-input@vger.kernel.org>
Sent: Friday, May 21, 2010 5:33 PM
Subject: Re: [RFC] [PATCH] misc : ROHM BH1780GLI Ambient light sensor Driver


> Hi,
>
> On Fri, May 21, 2010 at 05:05:50PM +0530, Hemanth V wrote:
>> Corrected the mailing list, should be linux-kernel
>>
>> This patch adds support for ROHM BH1780GLI Ambient light sensor.
>>
>> BH1780 supports I2C interface. Driver supports read/update of power state 
>> and
>> read of lux value (through SYSFS). Writing value 3 to power_state enables 
>> the
>> sensor and current lux value could be read.
>
> FWIW, this looks pretty good to me, just some minor things below.

Thanks for the review comment, responses inline

>
> Daniel
>
>> Signed-off-by: Hemanth V <hemanthv@ti.com>
>> ---
>>  drivers/misc/Kconfig     |   11 ++
>>  drivers/misc/Makefile    |    1 +
>>  drivers/misc/bh1780gli.c |  278 
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 290 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/misc/bh1780gli.c
>>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 0d0d625..0687a0c 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -278,6 +278,17 @@ config SENSORS_TSL2550
>>    This driver can also be built as a module.  If so, the module
>>    will be called tsl2550.
>>
>> +config SENSORS_BH1780
>> + tristate "ROHM BH1780GLI ambient light sensor"
>> + depends on I2C && SYSFS
>> + help
>> +   If you say yes here you get support for the ROHM BH1780GLI
>> +   ambient light sensor.
>> +
>> +   This driver can also be built as a module.  If so, the module
>> +   will be called bh1780gli.
>> +
>> +
>>  config EP93XX_PWM
>>  tristate "EP93xx PWM support"
>>  depends on ARCH_EP93XX
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 7b6f7ee..c479d91 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -30,3 +30,4 @@ obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
>>  obj-y += eeprom/
>>  obj-y += cb710/
>>  obj-$(CONFIG_VMWARE_BALLOON) += vmware_balloon.o
>> +obj-$(CONFIG_SENSORS_BH1780) += bh1780gli.o
>> diff --git a/drivers/misc/bh1780gli.c b/drivers/misc/bh1780gli.c
>> new file mode 100644
>> index 0000000..9b4137d
>> --- /dev/null
>> +++ b/drivers/misc/bh1780gli.c
>> @@ -0,0 +1,278 @@
>> +/*
>> + * bh1780gli.c
>> + * ROHM Ambient Light Sensor Driver
>> + *
>> + * Copyright (C) 2010 Texas Instruments
>> + * Author: Hemanth V <hemanthv@ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify 
>> it
>> + * under the terms of the GNU General Public License version 2 as 
>> published by
>> + * the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but 
>> WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License 
>> for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License 
>> along with
>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +#include <linux/i2c.h>
>> +#include <linux/slab.h>
>> +#include <linux/mutex.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/delay.h>
>> +
>> +#define BH1780_REG_CONTROL     0x80
>> +#define BH1780_REG_PARTID      0x8A
>> +#define BH1780_REG_MANFID      0x8B
>> +#define BH1780_REG_DLOW        0x8C
>> +#define BH1780_REG_DHIGH       0x8D
>> +
>> +#define BH1780_REVMASK        (0xf)
>> +#define BH1780_POWMASK        (0x3)
>> +#define BH1780_POFF       (0x0)
>> +#define BH1780_PON            (0x3)
>
> Just a nit ... something's wrong with the indentation here.

Will fix

>
>> +/* power on settling time in ms */
>> +#define BH1780_PON_DELAY      2
>> +
>> +struct bh1780_data {
>> + struct i2c_client *client;
>> + int power_state;
>> + /* lock for sysfs operations */
>> + struct mutex lock;
>> +};
>> +
>> +int bh1780_write(struct bh1780_data *ddata, u8 reg, u8 val, char *msg)
>
> Should be static.

Yes, agreed
>
>> +{
>> + int ret = i2c_smbus_write_byte_data(ddata->client, reg, val);
>> + if (ret < 0)
>> + dev_err(&ddata->client->dev,
>> + "i2c_smbus_write_byte_data failed error %d\
>> + Register (%s)\n", ret, msg);
>> + return ret;
>> +}
>> +
>> +int bh1780_read(struct bh1780_data *ddata, u8 reg, char *msg)
>
> Dito.

Yes, agreed
>
>> +{
>> + int ret = i2c_smbus_read_byte_data(ddata->client, reg);
>> + if (ret < 0)
>> + dev_err(&ddata->client->dev,
>> + "i2c_smbus_read_byte_data failed error %d\
>> + Register (%s)\n", ret, msg);
>> + return ret;
>> +}
>> +
>> +static ssize_t bh1780_show_lux(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> +
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct bh1780_data *ddata = platform_get_drvdata(pdev);
>> + int lsb, msb;
>> +
>> + lsb = bh1780_read(ddata, BH1780_REG_DLOW, "DLOW");
>> + if (lsb < 0)
>> + return lsb;
>> +
>> + msb = bh1780_read(ddata, BH1780_REG_DHIGH, "DHIGH");
>> + if (msb < 0)
>> + return msb;
>> +
>> + return sprintf(buf, "%d\n", (msb << 8) | lsb);
>> +}
>> +
>> +static ssize_t bh1780_show_power_state(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct bh1780_data *ddata = platform_get_drvdata(pdev);
>> + int state;
>> +
>> + state = bh1780_read(ddata, BH1780_REG_CONTROL, "CONTROL");
>> + if (state < 0)
>> + return state;
>> +
>> + return sprintf(buf, "%d\n", state & BH1780_POWMASK);
>> +}
>> +
>> +static ssize_t bh1780_store_power_state(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct bh1780_data *ddata = platform_get_drvdata(pdev);
>> + unsigned long val;
>> + int error;
>> +
>> + error = strict_strtoul(buf, 0, &val);
>> + if (error)
>> + return error;
>> +
>> + if (val < BH1780_POFF || val > BH1780_PON)
>> + return -EINVAL;
>> +
>> + mutex_lock(&ddata->lock);
>> +
>> + error = bh1780_write(ddata, BH1780_REG_CONTROL, val, "CONTROL");
>> + if (error < 0) {
>> + mutex_unlock(&ddata->lock);
>> + return error;
>> + }
>> +
>> + msleep(BH1780_PON_DELAY);
>
> Hmm, what do you wait for here?

Settling time delay required before lux read out

>
>> + ddata->power_state = val;
>> + mutex_unlock(&ddata->lock);
>
> And I don't get the usage of the mutex here. Can you explain?

Basically prevent two concurrent sysfs calls from interfering
with each other

>
>> + return count;
>> +
>> +}
>
> Remove the empty line. Or move it above the return statement.

Yes, agreed

>
>> +
>> +static DEVICE_ATTR(lux, S_IRUGO, bh1780_show_lux, NULL);
>> +
>> +static DEVICE_ATTR(power_state, S_IWUSR | S_IRUGO,
>> + bh1780_show_power_state, bh1780_store_power_state);
>> +
>> +static struct attribute *bh1780_attributes[] = {
>> + &dev_attr_power_state.attr,
>> + &dev_attr_lux.attr,
>> + NULL
>> +};
>> +
>> +static const struct attribute_group bh1780_attr_group = {
>> + .attrs = bh1780_attributes,
>> +};
>> +
>> +static int __devinit bh1780_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + int ret;
>> + struct bh1780_data *ddata = NULL;
>
> The initialization isn't needed.

This is basically added for the first goto error, to prevent
any garbage values

>
>> + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
>> +
>> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) {
>> + ret = -EIO;
>> + goto err_op_failed;
>> + }
>> +
>> + ddata = kzalloc(sizeof(struct bh1780_data), GFP_KERNEL);
>> + if (ddata == NULL) {
>> + ret = -ENOMEM;
>> + goto err_op_failed;
>> + }
>> +
>> + ddata->client = client;
>> + i2c_set_clientdata(client, ddata);
>> +
>> + ret = bh1780_read(ddata, BH1780_REG_PARTID, "PART ID");
>> + if (ret < 0)
>> + goto err_op_failed;
>> +
>> + dev_info(&client->dev, "Ambient Light Sensor, Rev : %d\n",
>> + (ret & BH1780_REVMASK));
>> +
>> + mutex_init(&ddata->lock);
>> +
>> + ret = sysfs_create_group(&client->dev.kobj, &bh1780_attr_group);
>> + if (ret)
>> + goto err_op_failed;
>> +
>> + return 0;
>> +
>> +err_op_failed:
>> + if (ddata != NULL)
>> + kfree(ddata);
>
> kfree() is resitant against NULL pointers, so you can call it
> unconditionally.

Yes, agreed

>
>> +
>> + return ret;
>> +}
>> +
>> +static int __devexit bh1780_remove(struct i2c_client *client)
>> +{
>> + struct bh1780_data *ddata;
>> +
>> + ddata = i2c_get_clientdata(client);
>> + sysfs_remove_group(&client->dev.kobj, &bh1780_attr_group);
>> +
>> + if (ddata != NULL) {
>> + i2c_set_clientdata(client, NULL);
>> + kfree(ddata);
>> + }
>
> Same here.

Yes, agreed

>
>> +
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int bh1780_suspend(struct i2c_client *client, pm_message_t mesg)
>> +{
>> + struct bh1780_data *ddata;
>> + int state, ret;
>> +
>> + ddata = i2c_get_clientdata(client);
>> + state = bh1780_read(ddata, BH1780_REG_CONTROL, "CONTROL");
>> + if (state < 0)
>> + return state;
>> +
>> + ddata->power_state = state & BH1780_POWMASK;
>> +
>> + ret = bh1780_write(ddata, BH1780_REG_CONTROL, BH1780_POFF,
>> + "CONTROL");
>> +
>> + if (ret < 0)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int bh1780_resume(struct i2c_client *client)
>> +{
>> + struct bh1780_data *ddata;
>> + int state, ret;
>> +
>> + ddata = i2c_get_clientdata(client);
>> + state = ddata->power_state;
>> +
>> + ret = bh1780_write(ddata, BH1780_REG_CONTROL, state,
>> + "CONTROL");
>> +
>> + if (ret < 0)
>> + return ret;
>> +
>> + return 0;
>> +}
>
> #else
> static inline int bh1780_suspend(struct i2c_client *client, pm_message_t 
> mesg) {}
> static inline int bh1780_resume(struct i2c_client *client) {}
>

Yes, I could add as below which seems to be used in quite a few
misc drivers

#define bh1780_suspend NULL

>
>> +#endif
>> +
>> +static const struct i2c_device_id bh1780_id[] = {
>> + { "bh1780", 0 },
>> + { },
>> +};
>> +
>> +static struct i2c_driver bh1780_driver = {
>> + .probe = bh1780_probe,
>> + .remove = bh1780_remove,
>> + .id_table = bh1780_id,
>> +#ifdef CONFIG_PM
>
> ... then this #ifdef can go away.
>
>> + .suspend = bh1780_suspend,
>> + .resume = bh1780_resume,
>> +#endif
>> + .driver = {
>> + .name = "bh1780"
>> + },
>> +};
>> +
>> +static int __init bh1780_init(void)
>> +{
>> + return i2c_add_driver(&bh1780_driver);
>> +}
>> +
>> +static void __exit bh1780_exit(void)
>> +{
>> + i2c_del_driver(&bh1780_driver);
>> +}
>> +
>> +module_init(bh1780_init)
>> +module_exit(bh1780_exit)
>> +
>> +MODULE_DESCRIPTION("BH1780GLI Ambient Light Sensor Driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Hemanth V <hemanthv@ti.com>");
>> -- 
>> 1.5.6.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" 
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [RFC] [PATCH] misc : ROHM BH1780GLI Ambient light sensor Driver

[Daniel Mack]

On Fri, May 21, 2010 at 06:10:00PM +0530, Hemanth V wrote:
> >On Fri, May 21, 2010 at 05:05:50PM +0530, Hemanth V wrote:
> >>+ mutex_lock(&ddata->lock);
> >>+
> >>+ error = bh1780_write(ddata, BH1780_REG_CONTROL, val, "CONTROL");
> >>+ if (error < 0) {
> >>+ mutex_unlock(&ddata->lock);
> >>+ return error;
> >>+ }
> >>+
> >>+ msleep(BH1780_PON_DELAY);
> >
> >Hmm, what do you wait for here?
> 
> Settling time delay required before lux read out

I thought so, but in fact you're just delaying the next two lines by
that:

> >>+ ddata->power_state = val;
> >>+ mutex_unlock(&ddata->lock);

... which doesn't make sense to me.

I can believe there is need to wait for the value to settle, but I think
it's the wrong place where you're doing it currently.

> >>+static int __devinit bh1780_probe(struct i2c_client *client,
> >>+ const struct i2c_device_id *id)
> >>+{
> >>+ int ret;
> >>+ struct bh1780_data *ddata = NULL;
> >
> >The initialization isn't needed.
> 
> This is basically added for the first goto error, to prevent
> any garbage values

Sorry, you're right. Ignore this comment :)

Thanks,
Daniel

Re: [RFC] [PATCH] misc : ROHM BH1780GLI Ambient light sensor Driver

[Hemanth V]

----- Original Message ----- 
From: "Daniel Mack" <daniel@caiaq.de>
To: "Hemanth V" <hemanthv@ti.com>
Cc: <linux-kernel@vger.kernel.org>; <linux-omap@vger.kernel.org>; 
<linux-input@vger.kernel.org>
Sent: Friday, May 21, 2010 6:16 PM
Subject: Re: [RFC] [PATCH] misc : ROHM BH1780GLI Ambient light sensor Driver


> On Fri, May 21, 2010 at 06:10:00PM +0530, Hemanth V wrote:
>> >On Fri, May 21, 2010 at 05:05:50PM +0530, Hemanth V wrote:
>> >>+ mutex_lock(&ddata->lock);
>> >>+
>> >>+ error = bh1780_write(ddata, BH1780_REG_CONTROL, val, "CONTROL");
>> >>+ if (error < 0) {
>> >>+ mutex_unlock(&ddata->lock);
>> >>+ return error;
>> >>+ }
>> >>+
>> >>+ msleep(BH1780_PON_DELAY);
>> >
>> >Hmm, what do you wait for here?
>>
>> Settling time delay required before lux read out
>
> I thought so, but in fact you're just delaying the next two lines by
> that:
>
>> >>+ ddata->power_state = val;
>> >>+ mutex_unlock(&ddata->lock);
>
> ... which doesn't make sense to me.
>
> I can believe there is need to wait for the value to settle, but I think
> it's the wrong place where you're doing it currently.
>

I could move it one line down,  but not really release the mutex.
Which other place would you suggest.

Re: [RFC] [PATCH] misc : ROHM BH1780GLI Ambient light sensor Driver

[Daniel Mack]

On Fri, May 21, 2010 at 07:32:50PM +0530, Hemanth V wrote:
> >On Fri, May 21, 2010 at 06:10:00PM +0530, Hemanth V wrote:
> >>>On Fri, May 21, 2010 at 05:05:50PM +0530, Hemanth V wrote:
> >>>>+ mutex_lock(&ddata->lock);
> >>>>+
> >>>>+ error = bh1780_write(ddata, BH1780_REG_CONTROL, val, "CONTROL");
> >>>>+ if (error < 0) {
> >>>>+ mutex_unlock(&ddata->lock);
> >>>>+ return error;
> >>>>+ }
> >>>>+
> >>>>+ msleep(BH1780_PON_DELAY);
> >>>
> >>>Hmm, what do you wait for here?
> >>
> >>Settling time delay required before lux read out
> >
> >I thought so, but in fact you're just delaying the next two lines by
> >that:
> >
> >>>>+ ddata->power_state = val;
> >>>>+ mutex_unlock(&ddata->lock);
> >
> >... which doesn't make sense to me.
> >
> >I can believe there is need to wait for the value to settle, but I think
> >it's the wrong place where you're doing it currently.
> >
> 
> I could move it one line down,  but not really release the mutex.
> Which other place would you suggest.

Ah, you just need to wait for the power register to take effect. I
didn't get it, sorry. I tought the delay was supposed to be inbetween
the write of one register and the read of another.

So that all looks sane to me then. Once the other minor things are
fixed, feel free to add my

  Reviewed-by: Daniel Mack <daniel@caiaq.de>

Thanks,
Daniel