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

[PATCH V2] 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>
Reviewed-by: Daniel Mack <daniel@caiaq.de>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/misc/Kconfig     |   10 ++
 drivers/misc/Makefile    |    1 +
 drivers/misc/bh1780gli.c |  273 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 284 insertions(+), 0 deletions(-)
 create mode 100644 drivers/misc/bh1780gli.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 0d0d625..3f5bf0f 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -278,6 +278,16 @@ 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..774531d
--- /dev/null
+++ b/drivers/misc/bh1780gli.c
@@ -0,0 +1,273 @@
+/*
+ * 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;
+};
+
+static 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;
+}
+
+static 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:
+	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);
+	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;
+}
+#else
+#define bh1780_suspend NULL
+#define bh1780_resume NULL
+#endif /* CONFIG_PM */
+
+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,
+	.suspend	= bh1780_suspend,
+	.resume		= bh1780_resume,
+	.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: [PATCH V2] 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.
>
Jonathan, Daniel

If there are no more comments, do you know howto push this to linus tree.

Thanks
Hemanth

> Signed-off-by: Hemanth V <hemanthv@ti.com>
> Reviewed-by: Daniel Mack <daniel@caiaq.de>
> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  drivers/misc/Kconfig     |   10 ++
>  drivers/misc/Makefile    |    1 +
>  drivers/misc/bh1780gli.c |  273 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 284 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/misc/bh1780gli.c
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 0d0d625..3f5bf0f 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -278,6 +278,16 @@ 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..774531d
> --- /dev/null
> +++ b/drivers/misc/bh1780gli.c
> @@ -0,0 +1,273 @@
> +/*
> + * 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;
> +};
> +
> +static 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;
> +}
> +
> +static 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:
> +	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);
> +	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;
> +}
> +#else
> +#define bh1780_suspend NULL
> +#define bh1780_resume NULL
> +#endif /* CONFIG_PM */
> +
> +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,
> +	.suspend	= bh1780_suspend,
> +	.resume		= bh1780_resume,
> +	.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: [PATCH V2] misc : ROHM BH1780GLI Ambient light sensor Driver

[Jonathan Cameron]

On 05/26/10 09:30, 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
>>
>> 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.
>>
> Jonathan, Daniel
> 
> If there are no more comments, do you know howto push this to linus tree.
It's in misc, so send a copy (with reference back to review thread)
to Andrew Morton <akpm@linux-foundation.org>

Andrew tends to pick things up eventually for lkml, but he'll notice
it quicker if you cc him in on relevant emails!

Jonathan
> 
> Thanks
> Hemanth
> 
>> Signed-off-by: Hemanth V <hemanthv@ti.com>
>> Reviewed-by: Daniel Mack <daniel@caiaq.de>
>> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>> ---
>>  drivers/misc/Kconfig     |   10 ++
>>  drivers/misc/Makefile    |    1 +
>>  drivers/misc/bh1780gli.c |  273 ++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 284 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/misc/bh1780gli.c
>>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 0d0d625..3f5bf0f 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -278,6 +278,16 @@ 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..774531d
>> --- /dev/null
>> +++ b/drivers/misc/bh1780gli.c
>> @@ -0,0 +1,273 @@
>> +/*
>> + * 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;
>> +};
>> +
>> +static 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;
>> +}
>> +
>> +static 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:
>> +	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);
>> +	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;
>> +}
>> +#else
>> +#define bh1780_suspend NULL
>> +#define bh1780_resume NULL
>> +#endif /* CONFIG_PM */
>> +
>> +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,
>> +	.suspend	= bh1780_suspend,
>> +	.resume		= bh1780_resume,
>> +	.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: [PATCH V2] misc : ROHM BH1780GLI Ambient light sensor Driver

[Hemanth V]

----- Original Message -----
From: "Jonathan Cameron"
> On 05/26/10 09:30, 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
>>>
>>> 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.
>>>
>> Jonathan, Daniel
>>
>> If there are no more comments, do you know howto push this to linus tree.

> It's in misc, so send a copy (with reference back to review thread)
> to Andrew Morton <akpm@linux-foundation.org>
>
> Andrew tends to pick things up eventually for lkml, but he'll notice
> it quicker if you cc him in on relevant emails!
>

Andrew,

Could you pull the patch listed in https://patchwork.kernel.org/patch/101860/

This is version 2 of the patch. Review comments and discussion are listed part
of https://patchwork.kernel.org/patch/101396/

Thanks
Hemanth

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

[Andrew Morton]

I re-added your reviewers to the cc...

On Mon, 24 May 2010 16:34:25 +0530 (IST)
"Hemanth V" <hemanthv@ti.com> wrote:

> 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.

There are at least two other ambient light sensor drivers:
drivers/misc/isl29003.c and drivers/misc/tsl2550.c.

Is there any standardisation of the ABIs whcih these drivers offer?  If
so, does this new driver comply with that?

It would be most useful if the changelog were to fully describe the
proposed kernel<->userspace interface.  That's the most important part
of the driver, because it's the only part we can never change.

There is a desultory effort to maintain sysfs API descriptions under
Documentation/ABI/.  I'd have thought that it would be appropriate to
document this driver's ABI in there.

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

[Daniel Mack]

On Tue, Jun 01, 2010 at 01:12:44PM -0700, Andrew Morton wrote:
> On Mon, 24 May 2010 16:34:25 +0530 (IST)
> "Hemanth V" <hemanthv@ti.com> wrote:
> 
> > 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.
> 
> There are at least two other ambient light sensor drivers:
> drivers/misc/isl29003.c and drivers/misc/tsl2550.c.
> 
> Is there any standardisation of the ABIs whcih these drivers offer?  If
> so, does this new driver comply with that?

Jonathan proposed the ALS framework for these type of devices, but it
was rejected (don't know about the reasons, I didn't follow the
discussions). The new idea is to put such drivers in the industrial IO
subsystem, but I don't know how mature that approach is currently.

For the time being, these drivers cook up whatever sysfs interface they
like, and their userspace ABIs are not standardized, unfortunately.

> It would be most useful if the changelog were to fully describe the
> proposed kernel<->userspace interface.  That's the most important part
> of the driver, because it's the only part we can never change.
>
> There is a desultory effort to maintain sysfs API descriptions under
> Documentation/ABI/.  I'd have thought that it would be appropriate to
> document this driver's ABI in there.

FWIW, I put some documentation about the isl29003 to
Documentation/misc-devices when I wrote the driver.

Daniel

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

[Andrew Morton]

On Tue, 1 Jun 2010 22:27:15 +0200
Daniel Mack <daniel@caiaq.de> wrote:

> On Tue, Jun 01, 2010 at 01:12:44PM -0700, Andrew Morton wrote:
> > On Mon, 24 May 2010 16:34:25 +0530 (IST)
> > "Hemanth V" <hemanthv@ti.com> wrote:
> > 
> > > 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.
> > 
> > There are at least two other ambient light sensor drivers:
> > drivers/misc/isl29003.c and drivers/misc/tsl2550.c.
> > 
> > Is there any standardisation of the ABIs whcih these drivers offer?  If
> > so, does this new driver comply with that?
> 
> Jonathan proposed the ALS framework for these type of devices, but it
> was rejected (don't know about the reasons, I didn't follow the
> discussions). The new idea is to put such drivers in the industrial IO
> subsystem, but I don't know how mature that approach is currently.
> 
> For the time being, these drivers cook up whatever sysfs interface they
> like, and their userspace ABIs are not standardized, unfortunately.

Well can we fix that?  Look at the existing drivers, pick one and make
this new driver provide the same interface?

It's not a grand plan - more of an incremental baby step, but it's
better than this random proliferation.

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

[Jonathan Cameron]

On 06/01/10 21:12, Andrew Morton wrote:
> 
> I re-added your reviewers to the cc...
> 
> On Mon, 24 May 2010 16:34:25 +0530 (IST)
> "Hemanth V" <hemanthv@ti.com> wrote:
> 
>> 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.
> 
> There are at least two other ambient light sensor drivers:
> drivers/misc/isl29003.c and drivers/misc/tsl2550.c.
for ref there is also drivers/staging/iio/light/tsl2563.c
and intent to pick up the other two soon (probably when, hopefully
IIO leaves staging, we could do it now if people prefer. Having two
drivers in the short term is also an option).

As for abi cleanup.  We had all authors and contributors to all the
above agreeing to standardization changes for ALS (and probably
all the users ;) so we are probably in a position to change things
now before there are too many users!
> 
> Is there any standardisation of the ABIs whcih these drivers offer?  If
> so, does this new driver comply with that?
Sadly that was one of the things ALS was meant to clean up.  I've proposed
one under IIO that is effectively the equivalent of hwmon (we have a lot
of overlap in supported device types with hwmon, so as Greg KH suggested
have matched their interface where it exists).

illuminance0_input (measured in lux)

The other exports of light sensors are remarkably inconsistent across different
devices. At the moment all the in kernel drivers only support polled reading
via sysfs.  There are tsl2563 interrupt patches on linux-iio list (not that
it is particularly relevant here).  I've also proposed some other naming
for the weird bits of tsl2563 in that patch series.
(http://marc.info/?l=linux-iio&m=127473503113241&w=2)

> 
> It would be most useful if the changelog were to fully describe the
> proposed kernel<->userspace interface.  That's the most important part
> of the driver, because it's the only part we can never change.
> 
> There is a desultory effort to maintain sysfs API descriptions under
> Documentation/ABI/.  I'd have thought that it would be appropriate to
> document this driver's ABI in there.
Agreed, but we get back to the debate of what we should standardise on.
The main point of ALS before it died was exactly putting this standardization
in place (admittedly the interface was slightly different from what we
are proposing in IIO, but that was before Greg pointed out that sharing
with hwmon would be a good idea!)

I have to admit I'm a little loath to spend too much time on this given
the amount of time wasted previously (ALS).

Jonathan

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

[Jonathan Cameron]

On 06/01/10 21:27, Daniel Mack wrote:
> On Tue, Jun 01, 2010 at 01:12:44PM -0700, Andrew Morton wrote:
>> On Mon, 24 May 2010 16:34:25 +0530 (IST)
>> "Hemanth V" <hemanthv@ti.com> wrote:
>>
>>> 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.
>>
>> There are at least two other ambient light sensor drivers:
>> drivers/misc/isl29003.c and drivers/misc/tsl2550.c.
>>
>> Is there any standardisation of the ABIs whcih these drivers offer?  If
>> so, does this new driver comply with that?
> 
> Jonathan proposed the ALS framework for these type of devices, but it
> was rejected (don't know about the reasons, I didn't follow the
> discussions).
Ah the wonder of emails crossing ;)

For the interested... http://lkml.org/lkml/2010/3/1/367
(the main objections are Linus' email)
> The new idea is to put such drivers in the industrial IO
> subsystem, but I don't know how mature that approach is currently.
We haven't lifted any that didn't start there as moving things into staging
due to a dependency being there seemed a controversial thing to do.
> 
> For the time being, these drivers cook up whatever sysfs interface they
> like, and their userspace ABIs are not standardized, unfortunately.
> 
>> It would be most useful if the changelog were to fully describe the
>> proposed kernel<->userspace interface.  That's the most important part
>> of the driver, because it's the only part we can never change.
>>
>> There is a desultory effort to maintain sysfs API descriptions under
>> Documentation/ABI/.  I'd have thought that it would be appropriate to
>> document this driver's ABI in there.
> 
> FWIW, I put some documentation about the isl29003 to
> Documentation/misc-devices when I wrote the driver.
> 
> Daniel
> 

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

[Andrew Morton]

On Tue, 01 Jun 2010 21:39:10 +0100
Jonathan Cameron <kernel@jic23.retrosnub.co.uk> wrote:

> > 
> > It would be most useful if the changelog were to fully describe the
> > proposed kernel<->userspace interface.  That's the most important part
> > of the driver, because it's the only part we can never change.
> > 
> > There is a desultory effort to maintain sysfs API descriptions under
> > Documentation/ABI/.  I'd have thought that it would be appropriate to
> > document this driver's ABI in there.
> Agreed, but we get back to the debate of what we should standardise on.

I'd suggest standardising on one of the existing drivers.  That way we
have two compliant drivers and only need to change (n-2) others.  If we
pick some new standard then we need to change (n) drivers.

And we can't change the drivers, really.  They'd all end up needing to
provide two interfaces: one for the shiny-new-standard and one legacy.

> The main point of ALS before it died was exactly putting this standardization
> in place (admittedly the interface was slightly different from what we
> are proposing in IIO, but that was before Greg pointed out that sharing
> with hwmon would be a good idea!)
> 
> I have to admit I'm a little loath to spend too much time on this given
> the amount of time wasted previously (ALS).

Well, it's not a waste.  This is very important!  We appear to be
making a big mess which we can never fix up.

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

[Jonathan Cameron]

On 06/01/10 21:54, Andrew Morton wrote:
> On Tue, 01 Jun 2010 21:39:10 +0100
> Jonathan Cameron <kernel@jic23.retrosnub.co.uk> wrote:
> 
>>>
>>> It would be most useful if the changelog were to fully describe the
>>> proposed kernel<->userspace interface.  That's the most important part
>>> of the driver, because it's the only part we can never change.
>>>
>>> There is a desultory effort to maintain sysfs API descriptions under
>>> Documentation/ABI/.  I'd have thought that it would be appropriate to
>>> document this driver's ABI in there.
>> Agreed, but we get back to the debate of what we should standardise on.
> 
> I'd suggest standardising on one of the existing drivers.  That way we
> have two compliant drivers and only need to change (n-2) others.  If we
> pick some new standard then we need to change (n) drivers.
I agree but take into account that we are getting a number of superficially
similar interfaces in kernel (hwmon, various sensors in misc and IIO)
and it makes sense to my mind to share interfaces across these where
possible (this is exactly the argument Greg made when we carried the
equivalent standardization out in IIO - for ref, the spec is in
staging/drivers/iio/Documentation/sysfs-class-iio)
(gah, the name needs a change to reflect our move to a sysfs bus - oops)
> 
> And we can't change the drivers, really.  They'd all end up needing to
> provide two interfaces: one for the shiny-new-standard and one legacy.
True enough.  We argued we could do with out this before because we were
fairly sure that we knew everyone who was using the sensors (also 1 was in staging
and another extremely)
> 
>> The main point of ALS before it died was exactly putting this standardization
>> in place (admittedly the interface was slightly different from what we
>> are proposing in IIO, but that was before Greg pointed out that sharing
>> with hwmon would be a good idea!)
>>
>> I have to admit I'm a little loath to spend too much time on this given
>> the amount of time wasted previously (ALS).
> 
> Well, it's not a waste.  This is very important!  We appear to be
> making a big mess which we can never fix up.
I agree entirely, picking a driver to copy is the way to go.
(personally I'd argue for the tsl2563 for the reasons above)
I'm just moaning about the time wasted the previous time we tried to clean
this stuff up :)

Note that 'if' these drivers do end up in IIO we will have to add the abi
as per the tsl2563 (can keep the others around for a while). We have a consistent
abi over 6+ sensor types so far and we aren't going to want to break it for
this one.  There are still unresolved corners in our abi but
they are all event related so this device is nowhere near them.

Either way, I agree entirely that matching any existing driver is a good thing
even if it isn't the one I'd personally prefer!

Jonathan

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

[Hemanth V]

----- Original Message ----- 
From: "Andrew Morton" <akpm@linux-foundation.org>
To: "Jonathan Cameron" <kernel@jic23.retrosnub.co.uk>
Cc: "Hemanth V" <hemanthv@ti.com>; <linux-kernel@vger.kernel.org>; 
<linux-omap@vger.kernel.org>; "Daniel Mack" <daniel@caiaq.de>; "Jonathan 
Cameron" <jic23@cam.ac.uk>; "Wolfram Sang" <w.sang@pengutronix.de>
Sent: Wednesday, June 02, 2010 2:24 AM
Subject: Re: [PATCH V2] misc : ROHM BH1780GLI Ambient light sensor Driver


> On Tue, 01 Jun 2010 21:39:10 +0100
> Jonathan Cameron <kernel@jic23.retrosnub.co.uk> wrote:
>
>> >
>> > It would be most useful if the changelog were to fully describe the
>> > proposed kernel<->userspace interface.  That's the most important part
>> > of the driver, because it's the only part we can never change.
>> >
>> > There is a desultory effort to maintain sysfs API descriptions under
>> > Documentation/ABI/.  I'd have thought that it would be appropriate to
>> > document this driver's ABI in there.
>> Agreed, but we get back to the debate of what we should standardise on.
>
> I'd suggest standardising on one of the existing drivers.  That way we
> have two compliant drivers and only need to change (n-2) others.  If we
> pick some new standard then we need to change (n) drivers.
>

Currently this driver follows the same sysfs convention as supported
by isl29003.c in drivers/misc. 

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

[Alan Cox]

> Is there any standardisation of the ABIs whcih these drivers offer?  If
> so, does this new driver comply with that?

There was an attempt to sort this out but Linux vetoed it because he is
under the delusion that light sensors are input devices. That doesn't
work in many use cases as you have to go ask them the light level and the
polling needed for input events keeps wrecks your idle behaviour when you
need to sample them when required instead. Instead therefore the API is
random and the devices appear in random ways and classes.

We have some intel drivers to submit as well as and when sanity prevails.

Alan