Skip to contentSkip to conversation

Various intel small device drivers

PATCH In review Alan Cox 12 participants 46 messages Apr 14, 2010 – May 4, 2010

Subsystems (5)
lore
Share & tools
lore

Alan Cox

author
#0 lore raw

cover Various intel small device drivers

These have been through my clean up processing, in particular the accelerometers are now using the input layer infrastructure.

---

Kalhan Trisal (4):
emc1403: thermal sensor support
liss331d1: accelerometer driver
isl29020: ambient light sensor
hmc6352: Add driver for the HMC6352 compass

 drivers/hwmon/Kconfig    |   34 ++++
 drivers/hwmon/Makefile   |    4 
 drivers/hwmon/emc1403.c  |  437 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/hmc6352.c  |  235 +++++++++++++++++++++++++
 drivers/hwmon/isl29020.c |  243 ++++++++++++++++++++++++++
 drivers/hwmon/lis331dl.c |  286 ++++++++++++++++++++++++++++++
 6 files changed, 1239 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/emc1403.c
 create mode 100644 drivers/hwmon/hmc6352.c
 create mode 100644 drivers/hwmon/isl29020.c
 create mode 100644 drivers/hwmon/lis331dl.c
author
#1 lore raw

PATCH 1/4 hmc6352: Add driver for the HMC6352 compass

From: Kalhan Trisal <[email protected]>

This driver will report the heading values in degrees to the sysfs interface. The values returned are headings . e.g. 245.6

(Some cleanups from Alan Cox)

---

 drivers/hwmon/Kconfig   |    7 +
 drivers/hwmon/Makefile  |    1 
 drivers/hwmon/hmc6352.c |  235 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 243 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/hmc6352.c
drivers/hwmon/Kconfig +7 -0
index d38447f..74f672d 100644
@@ -1088,6 +1088,13 @@ config SENSORS_MC13783_ADC
         help
           Support for the A/D converter on MC13783 PMIC.
 
+config SENSORS_HMC6352
+	tristate "Honeywell HMC6352 compass"
+	depends on I2C
+	help
+	  This driver provides support for the Honeywell HMC6352 compass,
+	  providing configuration and heading data via sysfs.
+
 if ACPI
 
 comment "ACPI drivers"
drivers/hwmon/Makefile +1 -0
index 4aa1a3d..ad2ed36 100644
@@ -49,6 +49,7 @@ obj-$(CONFIG_SENSORS_GL518SM)	+= gl518sm.o
 obj-$(CONFIG_SENSORS_GL520SM)	+= gl520sm.o
 obj-$(CONFIG_SENSORS_ULTRA45)	+= ultra45_env.o
 obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
+obj-$(CONFIG_SENSORS_HMC6352)	+= hmc6352.o
 obj-$(CONFIG_SENSORS_I5K_AMB)	+= i5k_amb.o
 obj-$(CONFIG_SENSORS_IBMAEM)	+= ibmaem.o
 obj-$(CONFIG_SENSORS_IBMPEX)	+= ibmpex.o
drivers/hwmon/hmc6352.c new file +235 -0

235 lines not on this page · raw message

#2 lore raw

Re: [PATCH 1/4] hmc6352: Add driver for the HMC6352 compass

Hi Alan, Kalhan,

Couple of comments below.

Quoted 7 lines · Alan Cox

From: Kalhan Trisal <[email protected]>

This driver will report the heading values in degrees to the sysfs interface. The values returned are headings . e.g. 245.6

(Some cleanups from Alan Cox)

Quoted 7 lines

---

 drivers/hwmon/Kconfig   |    7 +
 drivers/hwmon/Makefile  |    1 
 drivers/hwmon/hmc6352.c |  235 +++++++++++++++++++++++++++++++++++++++++++++++

This is in no way a hwmon chip. Surely misc is a better location for now (pending the usual discussion about all singing all dancing sensors frameworks).

Quoted 58 lines
 3 files changed, 243 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/hmc6352.c
drivers/hwmon/Kconfig +7 -0
index d38447f..74f672d 100644
@@ -1088,6 +1088,13 @@ config SENSORS_MC13783_ADC
         help
           Support for the A/D converter on MC13783 PMIC.
 
+config SENSORS_HMC6352
+	tristate "Honeywell HMC6352 compass"
+	depends on I2C
+	help
+	  This driver provides support for the Honeywell HMC6352 compass,
+	  providing configuration and heading data via sysfs.
+
 if ACPI
 
 comment "ACPI drivers"
drivers/hwmon/Makefile +1 -0
index 4aa1a3d..ad2ed36 100644
@@ -49,6 +49,7 @@ obj-$(CONFIG_SENSORS_GL518SM)	+= gl518sm.o
 obj-$(CONFIG_SENSORS_GL520SM)	+= gl520sm.o
 obj-$(CONFIG_SENSORS_ULTRA45)	+= ultra45_env.o
 obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
+obj-$(CONFIG_SENSORS_HMC6352)	+= hmc6352.o
 obj-$(CONFIG_SENSORS_I5K_AMB)	+= i5k_amb.o
 obj-$(CONFIG_SENSORS_IBMAEM)	+= ibmaem.o
 obj-$(CONFIG_SENSORS_IBMPEX)	+= ibmpex.o
drivers/hwmon/hmc6352.c new file +18 -0
new file mode 100644
index 0000000..926982f
@@ -0,0 +1,235 @@
+/*
+ * hmc6352.c - Honeywell Compass Driver
+ *
+ * Copyright (C) 2009 Intel Corp
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * 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, write to the Free Software Foundation, Inc.,

drivers/hwmon/hmc6352.c line 28

Quoted 10 lines
drivers/hwmon/hmc6352.c new file +10 -0
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>

Why these extra hwmon includes? At least at first glance I can't see any uses of them. The only call to hwmon is to stick this in the hwmon class.

drivers/hwmon/hmc6352.c line 32

Quoted 4 lines
drivers/hwmon/hmc6352.c new file +4 -0
+#include <linux/hwmon-sysfs.h>
+#include <linux/hwmon-vid.h>
+#include <linux/err.h>
+#include <linux/delay.h>

or any mutex usage?

drivers/hwmon/hmc6352.c line 35

Quoted 3 lines
drivers/hwmon/hmc6352.c new file +3 -0
+#include <linux/mutex.h>
+#include <linux/sysfs.h>
+

I guess this makes the driver look like many others, but why bother with the wrapping structure? This is only used to keep track of the hwmon device to be able to remove it later.

drivers/hwmon/hmc6352.c line 45

Quoted 10 lines
drivers/hwmon/hmc6352.c new file +10 -0
+struct compass_data {
+	struct device *hwmon_dev;
+};
+
+static ssize_t compass_calibration_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret;
+	unsigned long val;

Personally I'd have gone with a couple of chars then passed their address into the i2c_msg initializations. Guess it doesn't matter either way though!

drivers/hwmon/hmc6352.c line 57

Quoted 12 lines
drivers/hwmon/hmc6352.c new file +12 -0
+	char  cmd[] = {0x43};
+	char  cmd1[] = {0x45};
+	struct i2c_msg msg[] = {
+		{ client->addr, 0, 1, cmd },
+	};
+	struct i2c_msg msg1[] = {
+		{ client->addr, 0, 1, cmd1 },
+	};
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+	if (val == 1) {

These address changes looking a little unusual to me. They may well be needed, but if so can we have an explanatory comment?

Quoted 136 lines
drivers/hwmon/hmc6352.c new file +136 -0
+		client->addr = 0x21;
+		ret = i2c_transfer(client->adapter, msg, 1);
+		if (ret != 1) {
+			dev_warn(dev, "hmc6352_comp : i2c callib start cmd failed\n");
+			return ret;
+		}
+	} else if (val == 2) {
+		client->addr = 0x21;
+		ret = i2c_transfer(client->adapter, msg1, 1);
+		if (ret != 1) {
+			dev_warn(dev, "hmc6352_comp : i2c callib stop cmd failed\n");
+			return ret;
+		}
+	} else
+		return -EINVAL;
+
+	return count;
+}
+
+static ssize_t compass_heading_data_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+
+	struct i2c_client *client = to_i2c_client(dev);
+	static char cmd[] = { 0x41 };
+	unsigned char i2c_data[2];
+	unsigned int ret, ret_val;
+	struct i2c_msg msg[] = {
+		{ client->addr, 0, 1, cmd },
+	};
+	struct i2c_msg msg1[] = {
+		{ client->addr, I2C_M_RD, 2, i2c_data },
+	};
+
+	client->addr = 0x21;
+	ret = i2c_transfer(client->adapter, msg, 1);
+	if (ret != 1) {
+		dev_warn(dev, "hmc6352 : i2c cmd  0x41 failed\n");
+		return ret;
+	}
+	msleep(10); /* sending 0x41 cmd we need to wait for 7-10 milli second*/
+	ret = i2c_transfer(client->adapter, msg1, 1);
+	if (ret != 1) {
+		dev_warn(dev, "hmc6352 : i2c read data cmd failed\n");
+		return ret;
+	}
+	ret_val = i2c_data[0];
+	ret_val = ((ret_val << 8) | i2c_data[1]);
+	return sprintf(buf, "%d.%d\n", ret_val/10, ret_val%10);
+}
+
+static ssize_t compass_power_mode_store(struct device *dev,
+		struct device_attribute *attr, const  char *buf, size_t count)
+{
+
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned long val;
+	unsigned int ret;
+	static char cmd[] = { 0x53 };
+	static char cmd1[] = { 0x57 };
+	struct i2c_msg msg[] = {
+		{ client->addr, 0, 1, cmd },
+	};
+	struct i2c_msg msg1[] = {
+		{ client->addr, 0, 1, cmd1 },
+	};
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	if (val == 0) {
+		ret = i2c_transfer(client->adapter, msg, 1);
+		if (ret != 1)
+			dev_warn(dev, "hmc6352: i2c cmd  sleep mode failed\n");
+	} else if (val == 1) {
+		ret = i2c_transfer(client->adapter, msg1, 1);
+		if (ret != 1)
+			dev_warn(dev, "hmc6352: i2c cmd active mode failed\n");
+	} else
+		return -EINVAL;
+
+	return count;
+}
+
+static DEVICE_ATTR(heading, S_IRUGO, compass_heading_data_show, NULL);
+static DEVICE_ATTR(calibration, S_IWUSR, NULL, compass_calibration_store);
+static DEVICE_ATTR(power_state, S_IWUSR, NULL, compass_power_mode_store);
+
+static struct attribute *mid_att_compass[] = {
+	&dev_attr_heading.attr,
+	&dev_attr_calibration.attr,
+	&dev_attr_power_state.attr,
+	NULL
+};
+
+static struct attribute_group m_compass_gr = {
+	.name = "hmc6352",
+	.attrs = mid_att_compass
+};
+
+static int  hmc6352_probe(struct i2c_client *client,
+					const struct i2c_device_id *id)
+{
+	int res;
+	struct compass_data *data;
+
+	data = kzalloc(sizeof(struct compass_data), GFP_KERNEL);
+	if (data == NULL) {
+		dev_err(&client->dev, "hmc6352: out of memory");
+		return -ENOMEM;
+	}
+	i2c_set_clientdata(client, data);
+
+	res = sysfs_create_group(&client->dev.kobj, &m_compass_gr);
+	if (res) {
+		dev_err(&client->dev, "hmc6352: device_create_file failed\n");
+		goto compass_error1;
+	}
+	data->hwmon_dev = hwmon_device_register(&client->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		res = PTR_ERR(data->hwmon_dev);
+		data->hwmon_dev = NULL;
+		dev_err(&client->dev, "hmc6352: fail to register hwmon device\n");
+		sysfs_remove_group(&client->dev.kobj, &m_compass_gr);
+		goto compass_error1;
+	}
+	dev_info(&client->dev, "%s HMC6352 compass chip found\n",
+		client->name);
+	return res;
+
+compass_error1:
+	i2c_set_clientdata(client, NULL);
+	kfree(data);
+	return res;
+}
+

drivers/hwmon/hmc6352.c line 203

Quoted 10 lines
drivers/hwmon/hmc6352.c new file +10 -0
+static int hmc6352_remove(struct i2c_client *client)
+{
+	struct compass_data *data = i2c_get_clientdata(client);
+
+	hwmon_device_unregister(data->hwmon_dev);
+	sysfs_remove_group(&client->dev.kobj, &m_compass_gr);
+	kfree(data);
+	return 0;
+}
+

Why i2c_compass for the name?

Quoted 38 lines
drivers/hwmon/hmc6352.c new file +32 -1
+static struct i2c_device_id hmc6352_id[] = {
+	{ "i2c_compass", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, hmc6352_id);
+
+static struct i2c_driver hmc6352_driver = {
+	.driver = {
+	.name = "hmc6352",
+	},
+	.probe = hmc6352_probe,
+	.remove = hmc6352_remove,
+	.id_table = hmc6352_id,
+};
+
+static int __init _hmc6352_init(void)
+{
+	return i2c_add_driver(&hmc6352_driver);
+}
+
+static void  __exit sensor_hmc6352_exit(void)
+{
+	i2c_del_driver(&hmc6352_driver);
+}
+
+module_init(sensor_hmc6352_init);
+module_exit(sensor_hmc6352_exit);
+
+MODULE_AUTHOR("Kalhan Trisal <[email protected]");
+MODULE_DESCRIPTION("hmc6352 Compass Driver");
+MODULE_LICENSE("GPL v2");
 
--

To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw

Re: [PATCH 1/4] hmc6352: Add driver for the HMC6352 compass

Quoted 2 lines

This is in no way a hwmon chip. Surely misc is a better location for now (pending the usual discussion about all singing all dancing sensors frameworks).

Yep it's moving at the moment

Quoted 9 lines

drivers/hwmon/hmc6352.c line 28

Quoted 5 lines
drivers/hwmon/hmc6352.c new file +5 -0
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>

Why these extra hwmon includes? At least at first glance I can't see any uses of them. The only call to hwmon is to stick this in the hwmon class.

I inherited it for cleanup so these are good points (I've been staring at piles of these for so long extra input is very useful - this is the tip of the iceberg !)

Quoted 5 lines

drivers/hwmon/hmc6352.c line 32

Quoted 4 lines
drivers/hwmon/hmc6352.c new file +4 -0
+#include <linux/hwmon-sysfs.h>
+#include <linux/hwmon-vid.h>
+#include <linux/err.h>
+#include <linux/delay.h>

or any mutex usage?

Quoted 7 lines

drivers/hwmon/hmc6352.c line 35

Quoted 3 lines
drivers/hwmon/hmc6352.c new file +3 -0
+#include <linux/mutex.h>
+#include <linux/sysfs.h>
+

I guess this makes the driver look like many others, but why bother with the wrapping structure? This is only used to keep track of the hwmon device to be able to remove it later.

Should go - agreed will remove

Quoted 2 lines

These address changes looking a little unusual to me. They may well be needed, but if so can we have an explanatory comment?

Will investigate.

#4 lore raw

Re: [PATCH 1/4] hmc6352: Add driver for the HMC6352 compass

Quoted 16 lines · Alan Cox
Quoted 2 lines

This is in no way a hwmon chip. Surely misc is a better location for now (pending the usual discussion about all singing all dancing sensors frameworks).

Yep it's moving at the moment

Quoted 9 lines

drivers/hwmon/hmc6352.c line 28

Quoted 5 lines
drivers/hwmon/hmc6352.c new file +5 -0
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>

Why these extra hwmon includes? At least at first glance I can't see any uses of them. The only call to hwmon is to stick this in the hwmon class.

Quoted 3 lines

I inherited it for cleanup so these are good points (I've been staring at piles of these for so long extra input is very useful - this is the tip of the iceberg !)

Cool, post away. Feel free to cc me in on anything in the category of general sensors (accelerometers, magnetometers etc). If nothing else, I'm interested to see them to get ideas for drivers in IIO etc.

Quoted 25 lines
Quoted 6 lines

drivers/hwmon/hmc6352.c line 32

Quoted 5 lines
drivers/hwmon/hmc6352.c new file +4 -0
+#include <linux/hwmon-sysfs.h>
+#include <linux/hwmon-vid.h>
+#include <linux/err.h>
+#include <linux/delay.h>

or any mutex usage?

Quoted 7 lines

drivers/hwmon/hmc6352.c line 35

Quoted 3 lines
drivers/hwmon/hmc6352.c new file +3 -0
+#include <linux/mutex.h>
+#include <linux/sysfs.h>
+

I guess this makes the driver look like many others, but why bother with the wrapping structure? This is only used to keep track of the hwmon device to be able to remove it later.

Should go - agreed will remove

Quoted 2 lines

These address changes looking a little unusual to me. They may well be needed, but if so can we have an explanatory comment?

Will investigate.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#5 lore raw

Re: [PATCH 1/4] hmc6352: Add driver for the HMC6352 compass

Ripping it out of hwmon and applying the hedge trimmers to everything not needed and we get this for drivers/misc. I've also swapped the command codes to characters as the datasheet specifies them in ascii.

--------------------------------8<------------------------------

hmc6352: Add driver for the HMC6352 compass

From: Kalhan Trisal <[email protected]>

This driver will report the heading values in degrees to the sysfs interface. The values returned are headings . e.g. 245.6

(Some cleanups from Alan Cox)

---

 drivers/misc/Kconfig   |    7 ++
 drivers/misc/Makefile  |    1 
 drivers/misc/hmc6352.c |  199 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 207 insertions(+), 0 deletions(-)
 create mode 100644 drivers/misc/hmc6352.c
drivers/misc/Kconfig +7 -0
index 2191c8d..e626bac 100644
@@ -278,6 +278,13 @@ config SENSORS_TSL2550
 	  This driver can also be built as a module.  If so, the module
 	  will be called tsl2550.
 
+config HMC6352
+	tristate "Honeywell HMC6352 compass"
+	depends on I2C
+	help
+	  This driver provides support for the Honeywell HMC6352 compass,
+	  providing configuration and heading data via sysfs.
+
 config EP93XX_PWM
 	tristate "EP93xx PWM support"
 	depends on ARCH_EP93XX
drivers/misc/Makefile +1 -0
index 208ae30..620cf0b 100644
@@ -26,5 +26,6 @@ obj-$(CONFIG_DS1682)		+= ds1682.o
 obj-$(CONFIG_TI_DAC7512)	+= ti_dac7512.o
 obj-$(CONFIG_C2PORT)		+= c2port/
 obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
+obj-$(CONFIG_HMC6352)		+= hmc6352.o
 obj-y				+= eeprom/
 obj-y				+= cb710/
drivers/misc/hmc6352.c new file +199 -0

199 lines not on this page · raw message

#6 lore raw

Re: [PATCH 1/4] hmc6352: Add driver for the HMC6352 compass

Looks good to me,

Quoted 265 lines · Alan Cox

Ripping it out of hwmon and applying the hedge trimmers to everything not needed and we get this for drivers/misc. I've also swapped the command codes to characters as the datasheet specifies them in ascii.

--------------------------------8<------------------------------

hmc6352: Add driver for the HMC6352 compass

From: Kalhan Trisal <[email protected]>

This driver will report the heading values in degrees to the sysfs interface. The values returned are headings . e.g. 245.6

(Some cleanups from Alan Cox)

---

 drivers/misc/Kconfig   |    7 ++
 drivers/misc/Makefile  |    1 
 drivers/misc/hmc6352.c |  199 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 207 insertions(+), 0 deletions(-)
 create mode 100644 drivers/misc/hmc6352.c
drivers/misc/Kconfig +7 -0
index 2191c8d..e626bac 100644
@@ -278,6 +278,13 @@ config SENSORS_TSL2550
 	  This driver can also be built as a module.  If so, the module
 	  will be called tsl2550.
 
+config HMC6352
+	tristate "Honeywell HMC6352 compass"
+	depends on I2C
+	help
+	  This driver provides support for the Honeywell HMC6352 compass,
+	  providing configuration and heading data via sysfs.
+
 config EP93XX_PWM
 	tristate "EP93xx PWM support"
 	depends on ARCH_EP93XX
drivers/misc/Makefile +1 -0
index 208ae30..620cf0b 100644
@@ -26,5 +26,6 @@ obj-$(CONFIG_DS1682)		+= ds1682.o
 obj-$(CONFIG_TI_DAC7512)	+= ti_dac7512.o
 obj-$(CONFIG_C2PORT)		+= c2port/
 obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
+obj-$(CONFIG_HMC6352)		+= hmc6352.o
 obj-y				+= eeprom/
 obj-y				+= cb710/
drivers/misc/hmc6352.c new file +199 -1
new file mode 100644
index 0000000..f4162ea
@@ -0,0 +1,199 @@
+/*
+ * hmc6352.c - Honeywell Compass Driver
+ *
+ * Copyright (C) 2009 Intel Corp
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * 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, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/sysfs.h>
+
+static ssize_t compass_calibration_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret;
+	unsigned long val;
+	char  cmd = 'C';	/* Calibrate */
+	char  cmd1 = 'E';	/* Exit calibration mode */
+	struct i2c_msg msg[] = {
+		{ client->addr, 0, 1, &cmd },
+	};
+	struct i2c_msg msg1[] = {
+		{ client->addr, 0, 1, &cmd1 },
+	};
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+	if (val == 1) {
+		ret = i2c_transfer(client->adapter, msg, 1);
+		if (ret != 1) {
+			dev_warn(dev, "hmc6352_comp: i2c calib start cmd failed\n");
+			return ret;
+		}
+	} else if (val == 2) {
+		ret = i2c_transfer(client->adapter, msg1, 1);
+		if (ret != 1) {
+			dev_warn(dev, "hmc6352_comp: i2c calib stop cmd failed\n");
+			return ret;
+		}
+	} else
+		return -EINVAL;
+
+	return count;
+}
+
+static ssize_t compass_heading_data_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+
+	struct i2c_client *client = to_i2c_client(dev);
+	static char cmd = 'A';		/* Get Data */
+	unsigned char i2c_data[2];
+	unsigned int ret, ret_val;
+	struct i2c_msg msg[] = {
+		{ client->addr, 0, 1, &cmd },
+	};
+	struct i2c_msg msg1[] = {
+		{ client->addr, I2C_M_RD, 2, i2c_data },
+	};
+
+	ret = i2c_transfer(client->adapter, msg, 1);
+	if (ret != 1) {
+		dev_warn(dev, "hmc6352: i2c cmd 0x41 failed\n");
+		return ret;
+	}
+	msleep(10); /* sending 0x41 cmd we need to wait for 7-10 milli second*/
+	ret = i2c_transfer(client->adapter, msg1, 1);
+	if (ret != 1) {
+		dev_warn(dev, "hmc6352: i2c read data cmd failed\n");
+		return ret;
+	}
+	ret_val = i2c_data[0];
+	ret_val = ((ret_val << 8) | i2c_data[1]);
+	return sprintf(buf, "%d.%d\n", ret_val/10, ret_val%10);
+}
+
+static ssize_t compass_power_mode_store(struct device *dev,
+		struct device_attribute *attr, const  char *buf, size_t count)
+{
+
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned long val;
+	unsigned int ret;
+	static char cmd = 'S';		/* Sleep mode */
+	static char cmd1 = 'W';		/* Wake up */
+	struct i2c_msg msg[] = {
+		{ client->addr, 0, 1, &cmd },
+	};
+	struct i2c_msg msg1[] = {
+		{ client->addr, 0, 1, &cmd1 },
+	};
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	if (val == 0) {
+		ret = i2c_transfer(client->adapter, msg, 1);
+		if (ret != 1)
+			dev_warn(dev, "hmc6352: i2c cmd  sleep mode failed\n");
+	} else if (val == 1) {
+		ret = i2c_transfer(client->adapter, msg1, 1);
+		if (ret != 1)
+			dev_warn(dev, "hmc6352: i2c cmd active mode failed\n");
+	} else
+		return -EINVAL;
+
+	return count;
+}
+
+static DEVICE_ATTR(heading, S_IRUGO, compass_heading_data_show, NULL);
+static DEVICE_ATTR(calibration, S_IWUSR, NULL, compass_calibration_store);
+static DEVICE_ATTR(power_state, S_IWUSR, NULL, compass_power_mode_store);
+
+static struct attribute *mid_att_compass[] = {
+	&dev_attr_heading.attr,
+	&dev_attr_calibration.attr,
+	&dev_attr_power_state.attr,
+	NULL
+};
+
+static struct attribute_group m_compass_gr = {
+	.name = "hmc6352",
+	.attrs = mid_att_compass
+};
+
+static int  hmc6352_probe(struct i2c_client *client,
+					const struct i2c_device_id *id)
+{
+	int res;
+
+	res = sysfs_create_group(&client->dev.kobj, &m_compass_gr);
+	if (res) {
+		dev_err(&client->dev, "hmc6352: device_create_file failed\n");
+		return res;
+	}
+	dev_info(&client->dev, "%s HMC6352 compass chip found\n",
+		client->name);
+	return 0;
+}
+
+static int hmc6352_remove(struct i2c_client *client)
+{
+	sysfs_remove_group(&client->dev.kobj, &m_compass_gr);
+	return 0;
+}
+
+static struct i2c_device_id hmc6352_id[] = {
+	{ "hmc6352", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, hmc6352_id);
+
+static struct i2c_driver hmc6352_driver = {
+	.driver = {
+	.name = "hmc6352",
+	},
+	.probe = hmc6352_probe,
+	.remove = hmc6352_remove,
+	.id_table = hmc6352_id,
+};
+
+static int __init sensor_hmc6352_init(void)
+{
+	return i2c_add_driver(&hmc6352_driver);
+}
+
+static void  __exit sensor_hmc6352_exit(void)
+{
+	i2c_del_driver(&hmc6352_driver);
+}
+
+module_init(sensor_hmc6352_init);
+module_exit(sensor_hmc6352_exit);
+
+MODULE_AUTHOR("Kalhan Trisal <[email protected]");
+MODULE_DESCRIPTION("hmc6352 Compass Driver");
+MODULE_LICENSE("GPL v2");
--

To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#7 lore raw

Re: [PATCH 1/4] hmc6352: Add driver for the HMC6352 compass

In reply to Alan Cox

Trivial neatening

Argument alignment
Spacing
Make chars static
Remove prefixes from dev_<level> logging

---
 drivers/misc/hmc6352.c |   44 ++++++++++++++++++++++----------------------
 1 files changed, 22 insertions(+), 22 deletions(-)
drivers/misc/hmc6352.c +22 -22
index 1c1e974..2c12dc3 100644
@@ -30,13 +30,14 @@
 #include <linux/sysfs.h>
 
 static ssize_t compass_calibration_store(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t count)
+					 struct device_attribute *attr,
+					 const char *buf, size_t count)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	int ret;
 	unsigned long val;
-	char  cmd = 'C';	/* Calibrate */
-	char  cmd1 = 'E';	/* Exit calibration mode */
+	static char cmd = 'C';		/* Calibrate */
+	static char cmd1 = 'E';		/* Exit calibration mode */
 	struct i2c_msg msg[] = {
 		{ client->addr, 0, 1, &cmd },
 	};
@@ -49,13 +50,13 @@ static ssize_t compass_calibration_store(struct device *dev,
 	if (val == 1) {
 		ret = i2c_transfer(client->adapter, msg, 1);
 		if (ret != 1) {
-			dev_warn(dev, "hmc6352_comp: i2c calib start cmd failed\n");
+			dev_warn(dev, "i2c calib start cmd failed\n");
 			return ret;
 		}
 	} else if (val == 2) {
 		ret = i2c_transfer(client->adapter, msg1, 1);
 		if (ret != 1) {
-			dev_warn(dev, "hmc6352_comp: i2c calib stop cmd failed\n");
+			dev_warn(dev, "i2c calib stop cmd failed\n");
 			return ret;
 		}
 	} else
@@ -65,9 +66,9 @@ static ssize_t compass_calibration_store(struct device *dev,
 }
 
 static ssize_t compass_heading_data_show(struct device *dev,
-			struct device_attribute *attr, char *buf)
+					 struct device_attribute *attr,
+					 char *buf)
 {
-
 	struct i2c_client *client = to_i2c_client(dev);
 	static char cmd = 'A';		/* Get Data */
 	unsigned char i2c_data[2];
@@ -81,22 +82,22 @@ static ssize_t compass_heading_data_show(struct device *dev,
 
 	ret = i2c_transfer(client->adapter, msg, 1);
 	if (ret != 1) {
-		dev_warn(dev, "hmc6352: i2c cmd 0x41 failed\n");
+		dev_warn(dev, "i2c cmd 0x41 failed\n");
 		return ret;
 	}
-	msleep(10); /* sending 0x41 cmd we need to wait for 7-10 milli second*/
+	msleep(10); /* sending 0x41 cmd we need to wait for 7-10 milliseconds */
 	ret = i2c_transfer(client->adapter, msg1, 1);
 	if (ret != 1) {
-		dev_warn(dev, "hmc6352: i2c read data cmd failed\n");
+		dev_warn(dev, "i2c read data cmd failed\n");
 		return ret;
 	}
-	ret_val = i2c_data[0];
-	ret_val = ((ret_val << 8) | i2c_data[1]);
+	ret_val = ((unsigned int)i2c_data[0] << 8) | i2c_data[1];
 	return sprintf(buf, "%d.%d\n", ret_val/10, ret_val%10);
 }
 
 static ssize_t compass_power_mode_store(struct device *dev,
-		struct device_attribute *attr, const  char *buf, size_t count)
+					struct device_attribute *attr,
+					const char *buf, size_t count)
 {
 
 	struct i2c_client *client = to_i2c_client(dev);
@@ -117,11 +118,11 @@ static ssize_t compass_power_mode_store(struct device *dev,
 	if (val == 0) {
 		ret = i2c_transfer(client->adapter, msg, 1);
 		if (ret != 1)
-			dev_warn(dev, "hmc6352: i2c cmd  sleep mode failed\n");
+			dev_warn(dev, "i2c cmd sleep mode failed\n");
 	} else if (val == 1) {
 		ret = i2c_transfer(client->adapter, msg1, 1);
 		if (ret != 1)
-			dev_warn(dev, "hmc6352: i2c cmd active mode failed\n");
+			dev_warn(dev, "i2c cmd active mode failed\n");
 	} else
 		return -EINVAL;
 
@@ -144,18 +145,17 @@ static struct attribute_group m_compass_gr = {
 	.attrs = mid_att_compass
 };
 
-static int  hmc6352_probe(struct i2c_client *client,
-					const struct i2c_device_id *id)
+static int hmc6352_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
 {
 	int res;
 
 	res = sysfs_create_group(&client->dev.kobj, &m_compass_gr);
 	if (res) {
-		dev_err(&client->dev, "hmc6352: device_create_file failed\n");
+		dev_err(&client->dev, "sysfs_create_group failed\n");
 		return res;
 	}
-	dev_info(&client->dev, "%s HMC6352 compass chip found\n",
-		client->name);
+	dev_info(&client->dev, "%s HMC6352 compass chip found\n", client->name);
 	return 0;
 }
 
@@ -174,7 +174,7 @@ MODULE_DEVICE_TABLE(i2c, hmc6352_id);
 
 static struct i2c_driver hmc6352_driver = {
 	.driver = {
-	.name = "hmc6352",
+		.name = "hmc6352",
 	},
 	.probe = hmc6352_probe,
 	.remove = hmc6352_remove,
@@ -186,7 +186,7 @@ static int __init sensor_hmc6352_init(void)
 	return i2c_add_driver(&hmc6352_driver);
 }
 
-static void  __exit sensor_hmc6352_exit(void)
+static void __exit sensor_hmc6352_exit(void)
 {
 	i2c_del_driver(&hmc6352_driver);
 }
#8 lore raw

Re: [PATCH 1/4] hmc6352: Add driver for the HMC6352 compass

Quoted 1 line

Argument alignment

Makes it take more space

Quoted 1 line

Spacing

Mostly dropped

Quoted 1 line

Make chars static

Not sure I see the point (compiler generates good code anyway)

Quoted 1 line

Remove prefixes from dev_<level> logging

Good point.

#9 lore raw

Re: [PATCH 1/4] hmc6352: Add driver for the HMC6352 compass

Quoted 2 lines · Alan Cox
Quoted 1 line

Argument alignment

Makes it take more space

Makes it look more like most kernel code.

Quoted 2 lines
Quoted 1 line

Make chars static

Not sure I see the point (compiler generates good code anyway)

Internal consistency. 2 use areas had it, 1 didn't.

#10 lore raw

isl29020: ALS driver as misc device

In reply to Jonathan Cameron

And this adds the isl29020 as a misc device per discussions

isl29020: ambient light sensor

From: Kalhan Trisal <[email protected]>

The LS driver will read the latest Lux measurement based upon the light brightness and will report the LUX output through sysfs interface.

This hardware isn't quite the same as the ISL29003 so has a different driver.

---

 drivers/misc/Kconfig    |   10 ++
 drivers/misc/Makefile   |    1 
 drivers/misc/isl29020.c |  210 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 221 insertions(+), 0 deletions(-)
 create mode 100644 drivers/misc/isl29020.c
drivers/misc/Kconfig +10 -0
index e626bac..b3af3f4 100644
@@ -268,6 +268,16 @@ config ISL29003
 	  This driver can also be built as a module.  If so, the module
 	  will be called isl29003.
 
+config ISL29020
+	tristate "Intersil ISL29020 ambient light sensor"
+	depends on I2C
+	help
+	  If you say yes here you get support for the Intersil ISL29020
+	  ambient light sensor.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called isl29020.
+
 config SENSORS_TSL2550
 	tristate "Taos TSL2550 ambient light sensor"
 	depends on I2C && SYSFS
drivers/misc/Makefile +1 -0
index 620cf0b..75c8b0e 100644
@@ -20,6 +20,7 @@ obj-$(CONFIG_SGI_GRU)		+= sgi-gru/
 obj-$(CONFIG_CS5535_MFGPT)	+= cs5535-mfgpt.o
 obj-$(CONFIG_HP_ILO)		+= hpilo.o
 obj-$(CONFIG_ISL29003)		+= isl29003.o
+obj-$(CONFIG_ISL29020)		+= isl29020.o
 obj-$(CONFIG_SENSORS_TSL2550)	+= tsl2550.o
 obj-$(CONFIG_EP93XX_PWM)	+= ep93xx_pwm.o
 obj-$(CONFIG_DS1682)		+= ds1682.o
drivers/misc/isl29020.c new file +210 -0

210 lines not on this page · raw message

#11 lore raw

Re: isl29020: ALS driver as misc device

Quoted 8 lines · Alan Cox

And this adds the isl29020 as a misc device per discussions

isl29020: ambient light sensor

From: Kalhan Trisal <[email protected]>

The LS driver will read the latest Lux measurement based upon the light brightness and will report the LUX output through sysfs interface.

Please document this sysfs interface with an addition to the Documentatin/ABI/ directory.

thanks,

greg k-h

#12 lore raw

Re: isl29020: ALS driver as misc device

Quoted 5 lines · Greg KH
Quoted 4 lines · Alan Cox

And this adds the isl29020 as a misc device per discussions

isl29020: ambient light sensor

Quoted 7 lines
Quoted 4 lines

From: Kalhan Trisal <[email protected]>

The LS driver will read the latest Lux measurement based upon the light brightness and will report the LUX output through sysfs interface.

Please document this sysfs interface with an addition to the Documentatin/ABI/ directory.

As documenting this abi (which indeed should be done) is going to set a precedence, perhaps this is a good time to discuss what this naming could be.

In cleaning up the IIO abi Greg suggested that we match existing similar abi's a closely as possible, which where possible makes a great deal of sense (shared userspace code is possible and it makes everything a bit more predictable for driver writers... particularly as I expect someone will sooner or later make a combined hwmon and als chip).

The obvious similarity here is with hwmon.
So perhaps going with naming as

lux-> illuminance0_input (or I guess lux0_input would also work, I can change the iio abi to match this as well).

It also occurs to me that we might want to associate the calibration with the particular channel? There's sure to be a dual ALS chip along at some point.

Obviously the isl29020 would need updating as well. Everyone was happy to do that when were writing the ALS subsystem, so I guess that won't have changed!

Jonathan

#13 lore raw

Re: isl29020: ALS driver as misc device

Quoted 8 lines

lux-> illuminance0_input (or I guess lux0_input would also work, I can change the iio abi to match this as well).

It also occurs to me that we might want to associate the calibration with the particular channel? There's sure to be a dual ALS chip along at some point.

Obviously the isl29020 would need updating as well. Everyone was happy to do that when were writing the ALS subsystem, so I guess that won't have changed!

I'm happy to make the 29020 comply, and I guess it wouldn't take long for someone to run over the others. Just let me know what the sysfs interface should look like.

#14 lore raw

Re: isl29020: ALS driver as misc device

Quoted 6 lines · Alan Cox
Quoted 6 lines

lux-> illuminance0_input (or I guess lux0_input would also work, I can change the iio abi to match this as well).

It also occurs to me that we might want to associate the calibration with the particular channel? There's sure to be a dual ALS chip along at some point.

Quoted 6 lines
Quoted 2 lines

Obviously the isl29020 would need updating as well. Everyone was happy to do that when were writing the ALS subsystem, so I guess that won't have changed!

I'm happy to make the 29020 comply, and I guess it wouldn't take long for someone to run over the others. Just let me know what the sysfs interface should look like.

Hi Alan,

I was hoping some others would jump in here with suggestions. Failing that, I'd go with

illuminance0_input (in milli lux?)
(if this requires a float calculation, could go with the iio approach of having illuminance0_raw and illuminance0_scale, where gain can be a float as it's just a text string anyway).

illuminance0_range
illuminance0_range_available

For these range, I'd personally prefer to do it via scaling, but we can probably be flexible here. Things will get complex if we start having ranges like 3...10 lux.

So illuminance0_calibscale (hence internal scale - obviously this will also effect illuminance0_scale if you are going the 'raw' output and convert in userspace route). Clearly, you would also need an illuminance0_calibscale_available interface to tell you what discrete settings exist.

Jonathan

author
#15 lore raw

PATCH 2/4 isl29020: ambient light sensor

In reply to Alan Cox

From: Kalhan Trisal <[email protected]>

The LS driver will read the latest Lux measurement based upon the light brightness and will report the LUX output through sysfs interface.

---

 drivers/hwmon/Kconfig    |    9 ++
 drivers/hwmon/Makefile   |    1 
 drivers/hwmon/isl29020.c |  243 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 253 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/isl29020.c
drivers/hwmon/Kconfig +9 -0
index 74f672d..1fa2533 100644
@@ -1095,6 +1095,15 @@ config SENSORS_HMC6352
 	  This driver provides support for the Honeywell HMC6352 compass,
 	  providing configuration and heading data via sysfs.
 
+config SENSORS_ISL29020
+	tristate "Intersil ISL29020 ALS"
+	depends on I2C
+	help
+	  If you say yes here you get support for the ALS Devices
+	  Ambient Light Sensor monitoring chip.
+	  Range values can be configured using sysfs.
+	  Lux data is accessible via sysfs.
+
 if ACPI
 
 comment "ACPI drivers"
drivers/hwmon/Makefile +1 -0
index ad2ed36..13d6832 100644
@@ -53,6 +53,7 @@ obj-$(CONFIG_SENSORS_HMC6352)	+= hmc6352.o
 obj-$(CONFIG_SENSORS_I5K_AMB)	+= i5k_amb.o
 obj-$(CONFIG_SENSORS_IBMAEM)	+= ibmaem.o
 obj-$(CONFIG_SENSORS_IBMPEX)	+= ibmpex.o
+obj-$(CONFIG_SENSORS_ISL29020)	+= isl29020.o
 obj-$(CONFIG_SENSORS_IT87)	+= it87.o
 obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
 obj-$(CONFIG_SENSORS_K10TEMP)	+= k10temp.o
drivers/hwmon/isl29020.c new file +243 -0

243 lines not on this page · raw message

#16 lore raw

Re: [PATCH 2/4] isl29020: ambient light sensor

Quoted 5 lines · Alan Cox

The LS driver will read the latest Lux measurement based upon the light brightness and will report the LUX output through sysfs interface.

Quoted 7 lines

---

 drivers/hwmon/Kconfig    |    9 ++
 drivers/hwmon/Makefile   |    1 
 drivers/hwmon/isl29020.c |  243 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 253 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/isl29020.c

Would it be possible to make the existing driver for the ISL29003 support the ISL29020 as well?

Also note that there is a ALS (ambient light sensor) framework pending. So patches against the 29003 driver should apply to this tree:

git://git.kernel.org/pub/scm/linux/kernel/git/jic23/als.git

I copied the maintainer to this mail. Jonathan, any plans when ALS will be merged?

Thanks,
Daniel

Quoted 287 lines
drivers/hwmon/Kconfig +9 -0
index 74f672d..1fa2533 100644
@@ -1095,6 +1095,15 @@ config SENSORS_HMC6352
 	  This driver provides support for the Honeywell HMC6352 compass,
 	  providing configuration and heading data via sysfs.
 
+config SENSORS_ISL29020
+	tristate "Intersil ISL29020 ALS"
+	depends on I2C
+	help
+	  If you say yes here you get support for the ALS Devices
+	  Ambient Light Sensor monitoring chip.
+	  Range values can be configured using sysfs.
+	  Lux data is accessible via sysfs.
+
 if ACPI
 
 comment "ACPI drivers"
drivers/hwmon/Makefile +1 -0
index ad2ed36..13d6832 100644
@@ -53,6 +53,7 @@ obj-$(CONFIG_SENSORS_HMC6352)	+= hmc6352.o
 obj-$(CONFIG_SENSORS_I5K_AMB)	+= i5k_amb.o
 obj-$(CONFIG_SENSORS_IBMAEM)	+= ibmaem.o
 obj-$(CONFIG_SENSORS_IBMPEX)	+= ibmpex.o
+obj-$(CONFIG_SENSORS_ISL29020)	+= isl29020.o
 obj-$(CONFIG_SENSORS_IT87)	+= it87.o
 obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
 obj-$(CONFIG_SENSORS_K10TEMP)	+= k10temp.o
drivers/hwmon/isl29020.c new file +243 -1
new file mode 100644
index 0000000..458140d
@@ -0,0 +1,243 @@
+/*
+ * isl29020.c - Intersil  ALS Driver
+ *
+ * Copyright (C) 2008 Intel Corp
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * 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, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/hwmon-vid.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/mutex.h>
+#include <linux/sysfs.h>
+
+
+#define ALS_MIN_RANGE_VAL 0
+#define ALS_MAX_RANGE_VAL 5
+
+struct als_data {
+	struct device *hwmon_dev;
+};
+
+static unsigned int i2c_write_current_data(struct i2c_client *client,
+					unsigned int reg, unsigned int value)
+{
+	int ret_val;
+
+	ret_val = i2c_smbus_write_byte_data(client, reg, value);
+	return ret_val;
+}
+
+static ssize_t als_sensing_range_show(struct device *dev,
+			struct device_attribute *attr,  char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int  val;
+
+	val = i2c_smbus_read_byte_data(client, 0x00);
+	return sprintf(buf, "%d000\n", 1 << (2 * (val & 3)));
+
+}
+
+static ssize_t als_lux_output_data_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned int ret_val, val;
+	unsigned long int lux, max_count;
+	int tempv1, tempv2;
+
+	max_count = 65535;
+	tempv1 = i2c_smbus_read_byte_data(client, 0x02); /* MSB data */
+	tempv2 = i2c_smbus_read_byte_data(client, 0x01); /* LSB data */
+	ret_val = tempv1;
+	ret_val = (ret_val << 8 | tempv2);
+	val = i2c_smbus_read_byte_data(client, 0x00);
+	lux = ((((1 << (2 * (val & 3))))*1000) * ret_val) / max_count;
+	return sprintf(buf, "%ld\n", lux);
+}
+
+static ssize_t als_sensing_range_store(struct device *dev,
+		struct device_attribute *attr, const  char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned int ret_val, set_val = 0;
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+	ret_val = i2c_smbus_read_byte_data(client, 0x00);
+	ret_val = ret_val & 0xFC; /*reset the bit before setting them */
+	if (val == 1)
+		set_val = (ret_val | 0x00); /* setting the 1:0  bit */
+	else if (val == 2)
+		set_val = (ret_val | 0x01);
+	else if (val == 3)
+		set_val = (ret_val | 0x02);
+	else if (val == 4)
+		set_val = (ret_val | 0x03);
+	else
+		goto invarg;
+	i2c_write_current_data(client, 0x00, set_val);
+	return count;
+invarg:
+	return -EINVAL;
+}
+
+static ssize_t als_power_status_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret_val;
+
+	ret_val = i2c_smbus_read_byte_data(client, 0x00);
+	ret_val = ret_val & 0x80;
+	if (ret_val == 0x80)
+		ret_val = 1;
+	return sprintf(buf, "%x", ret_val);
+}
+
+static ssize_t als_power_status_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned long val = 0;
+	char curr_val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	curr_val = i2c_smbus_read_byte_data(client, 0x00);
+	if (val == 1)
+		curr_val = curr_val | 0x80;
+	else if (val == 0)
+		curr_val = curr_val & 0x7F;
+	else
+		return -EINVAL;
+	i2c_write_current_data(client, 0x00, curr_val);
+	return count;
+}
+
+static DEVICE_ATTR(sensing_range, S_IRUGO | S_IWUSR,
+	als_sensing_range_show, als_sensing_range_store);
+static DEVICE_ATTR(lux_output, S_IRUGO, als_lux_output_data_show, NULL);
+static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR,
+	als_power_status_show, als_power_status_store);
+
+static struct attribute *mid_att_als[] = {
+	&dev_attr_sensing_range.attr,
+	&dev_attr_lux_output.attr,
+	&dev_attr_power_state.attr,
+	NULL
+};
+
+static struct attribute_group m_als_gr = {
+	.name = "isl29020",
+	.attrs = mid_att_als
+};
+
+static void als_set_default_config(struct i2c_client *client)
+{
+	i2c_write_current_data(client, 0x00, 0xc0);
+}
+
+static int  isl29020_probe(struct i2c_client *client,
+					const struct i2c_device_id *id)
+{
+	int res;
+	struct als_data *data;
+
+	data = kzalloc(sizeof(struct als_data), GFP_KERNEL);
+	if (data == NULL) {
+		printk(KERN_WARNING " isl29020: out of memory");
+		return -ENOMEM;
+	}
+	i2c_set_clientdata(client, data);
+
+	res = sysfs_create_group(&client->dev.kobj, &m_als_gr);
+	if (res) {
+		printk(KERN_WARNING "isl29020: device create file failed!!\n");
+		goto als_error1;
+	}
+	data->hwmon_dev = hwmon_device_register(&client->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		res = PTR_ERR(data->hwmon_dev);
+		data->hwmon_dev = NULL;
+		sysfs_remove_group(&client->dev.kobj, &m_als_gr);
+		printk(KERN_ERR "isl29020:unable to register hwmon device\n");
+		goto als_error1;
+	}
+	dev_info(&client->dev, "%s isl29020: ALS chip found\n", client->name);
+	als_set_default_config(client);
+	return res;
+
+als_error1:
+	i2c_set_clientdata(client, NULL);
+	kfree(data);
+	return res;
+}
+
+static int isl29020_remove(struct i2c_client *client)
+{
+	struct als_data *data = i2c_get_clientdata(client);
+
+	hwmon_device_unregister(data->hwmon_dev);
+	sysfs_remove_group(&client->dev.kobj, &m_als_gr);
+	kfree(data);
+	return 0;
+}
+
+static struct i2c_device_id isl29020_id[] = {
+	{ "i2c_als", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, isl29020_id);
+
+static struct i2c_driver isl29020_driver = {
+	.driver = {
+	.name = "isl29020",
+	},
+	.probe = isl29020_probe,
+	.remove = isl29020_remove,
+	.id_table = isl29020_id,
+};
+
+static int __init sensor_isl29020_init(void)
+{
+	return i2c_add_driver(&isl29020_driver);
+}
+
+static void  __exit sensor_isl29020_exit(void)
+{
+	i2c_del_driver(&isl29020_driver);
+}
+
+module_init(sensor_isl29020_init);
+module_exit(sensor_isl29020_exit);
+
+MODULE_AUTHOR("Kalhan Trisal <[email protected]");
+MODULE_DESCRIPTION("Intersil isl29020 ALS Driver");
+MODULE_LICENSE("GPL v2");
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

author
#17 lore raw

Re: [PATCH 2/4] isl29020: ambient light sensor

Quoted 2 lines

Would it be possible to make the existing driver for the ISL29003 support the ISL29020 as well?

I dug the manuals out for these to take a look - the answer is they are quite different chips.

Alan

#18 lore raw

Re: [PATCH 2/4] isl29020: ambient light sensor

Quoted 5 lines · Alan Cox
Quoted 2 lines

Would it be possible to make the existing driver for the ISL29003 support the ISL29020 as well?

I dug the manuals out for these to take a look - the answer is they are quite different chips.

Hmm, sad :(

However, the driver should be applied to the ALS tree after all, unless Jonathan plans to drop the whole thing, which I doubt.

Thanks,
Daniel

#19 lore raw

Re: [PATCH 2/4] isl29020: ambient light sensor

Quoted 11 lines · Daniel Mack
Quoted 5 lines · Alan Cox
Quoted 2 lines

Would it be possible to make the existing driver for the ISL29003 support the ISL29020 as well?

I dug the manuals out for these to take a look - the answer is they are quite different chips.

Hmm, sad :(

However, the driver should be applied to the ALS tree after all, unless Jonathan plans to drop the whole thing, which I doubt.

Sadly ALS is dead. Linus made it pretty clear he wasn't going to pull it.

Hence currently either ALS drivers are going into misc (and can be moved elsewhere later), or we are taking them into IIO (and hence staging) where they fit fine and we can sort out a bridge to input to answer Linus' issue with the original patch set.

In the thread following Alan's repost (having moved this driver to misc) Greg just pointed out the sysfs interface needed documenting, and I've suggested that we sort out the naming properly (in a way compliant with hwmon and the new IIO abi. 90% of what the ALS subsystem contributed was defining the ABI anyway!

Jonathan

author
#20 lore raw

PATCH 3/4 liss331d1: accelerometer driver

In reply to Alan Cox

From: Kalhan Trisal <[email protected]>

The acceleremeter driver reads the x,y,z coordinate registers and provides the information to user through the input layer

Conversion to input device, clean up and porting of retry fixes needed for AAVA done by Alan Cox.

---

 drivers/hwmon/Kconfig    |    8 +
 drivers/hwmon/Makefile   |    1 
 drivers/hwmon/lis331dl.c |  286 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 295 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/lis331dl.c
drivers/hwmon/Kconfig +8 -0
index 1fa2533..6868b9d 100644
@@ -1104,6 +1104,14 @@ config SENSORS_ISL29020
 	  Range values can be configured using sysfs.
 	  Lux data is accessible via sysfs.
 
+config SENSORS_LIS331DL
+	tristate "STMicroeletronics LIS331DL three-axis digital accelerometer"
+	depends on I2C
+	help
+	  If you say yes here you get support for the Accelerometer  Devices
+	  Device can be configured using sysfs.
+	  x y Z data can be   accessible via sysfs.
+
 if ACPI
 
 comment "ACPI drivers"
drivers/hwmon/Makefile +1 -0
index 13d6832..ebeb2a2 100644
@@ -60,6 +60,7 @@ obj-$(CONFIG_SENSORS_K10TEMP)	+= k10temp.o
 obj-$(CONFIG_SENSORS_LIS3LV02D) += lis3lv02d.o hp_accel.o
 obj-$(CONFIG_SENSORS_LIS3_SPI)	+= lis3lv02d.o lis3lv02d_spi.o
 obj-$(CONFIG_SENSORS_LIS3_I2C)	+= lis3lv02d.o lis3lv02d_i2c.o
+obj-$(CONFIG_SENSORS_LIS331DL)	+= lis331dl.o
 obj-$(CONFIG_SENSORS_LM63)	+= lm63.o
 obj-$(CONFIG_SENSORS_LM70)	+= lm70.o
 obj-$(CONFIG_SENSORS_LM73)	+= lm73.o
drivers/hwmon/lis331dl.c new file +286 -0

286 lines not on this page · raw message

#21 lore raw

Re: [PATCH 3/4] liss331d1: accelerometer driver

Op 14-04-10 14:52, Alan Cox schreef:

Quoted 7 lines

From: Kalhan Trisal <[email protected]>

The acceleremeter driver reads the x,y,z coordinate registers and provides the information to user through the input layer

Conversion to input device, clean up and porting of retry fixes needed for AAVA done by Alan Cox.

Hello,

I wouldn't want to be too picky, if this driver works fine with your hardware and it's tested, why not, but from looking at the spec of this chip (and at the code of this driver), it looks like it could be completely compatible with the lis3lv02d driver (with lis3lv02d_i2c). Same registers, including the WHO_AM_I register which returns 0x3B (which is a supported version).

Have you tried and there were some specific incompatibilities? To me, it looks like the only thing not in the lis3lv02d is the read with multiple retries, and it could be easily added if necessary for your hardware.

Cheers,
Eric

Quoted 341 lines

---

 drivers/hwmon/Kconfig    |    8 +
 drivers/hwmon/Makefile   |    1 
 drivers/hwmon/lis331dl.c |  286 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 295 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/lis331dl.c
drivers/hwmon/Kconfig +8 -0
index 1fa2533..6868b9d 100644
@@ -1104,6 +1104,14 @@ config SENSORS_ISL29020
 	  Range values can be configured using sysfs.
 	  Lux data is accessible via sysfs.
 
+config SENSORS_LIS331DL
+	tristate "STMicroeletronics LIS331DL three-axis digital accelerometer"
+	depends on I2C
+	help
+	  If you say yes here you get support for the Accelerometer  Devices
+	  Device can be configured using sysfs.
+	  x y Z data can be   accessible via sysfs.
+
 if ACPI
 
 comment "ACPI drivers"
drivers/hwmon/Makefile +1 -0
index 13d6832..ebeb2a2 100644
@@ -60,6 +60,7 @@ obj-$(CONFIG_SENSORS_K10TEMP)	+= k10temp.o
 obj-$(CONFIG_SENSORS_LIS3LV02D) += lis3lv02d.o hp_accel.o
 obj-$(CONFIG_SENSORS_LIS3_SPI)	+= lis3lv02d.o lis3lv02d_spi.o
 obj-$(CONFIG_SENSORS_LIS3_I2C)	+= lis3lv02d.o lis3lv02d_i2c.o
+obj-$(CONFIG_SENSORS_LIS331DL)	+= lis331dl.o
 obj-$(CONFIG_SENSORS_LM63)	+= lm63.o
 obj-$(CONFIG_SENSORS_LM70)	+= lm70.o
 obj-$(CONFIG_SENSORS_LM73)	+= lm73.o
drivers/hwmon/lis331dl.c new file +286 -1
new file mode 100644
index 0000000..34009eb
@@ -0,0 +1,286 @@
+/*
+ * lis331dl.c - ST LIS331DL  Accelerometer Driver
+ *
+ * Copyright (C) 2009 Intel Corp
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * 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, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/mutex.h>
+#include <linux/sysfs.h>
+#include <linux/input-polldev.h>
+
+
+#define POLL_INTERVAL	50
+
+struct lis331dl {
+	struct input_polled_dev *idev;
+	struct i2c_client *client;
+	struct mutex update_lock;
+};
+
+static ssize_t data_rate_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int val;
+	int speed = 100;
+
+	val = i2c_smbus_read_byte_data(client, 0x20);
+	if (val & 0x80); /* 1= 400HZ 0= 100HZ */
+		speed = 400;
+	return sprintf(buf, "%d\n", speed);
+}
+
+static ssize_t power_mode_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int val;
+
+	val = i2c_smbus_read_byte_data(client, 0x20) & 0x40;
+	if (val == 0x40)
+		val = 1;
+	return sprintf(buf, "%d\n", val);
+}
+
+static int read_with_retries(struct i2c_client *client, int r)
+{
+	int retries;
+	int ret;
+	
+	for (retries = 0; retries < 5; retries++) {
+		ret = i2c_smbus_read_byte_data(client, r);
+		if (ret != -ETIMEDOUT)
+			break;
+		msleep(10);
+	}
+	return ret;
+}
+
+static void lis331dl_poll(struct input_polled_dev *idev)
+{
+	struct input_dev *input = idev->input;
+	struct lis331dl *data = idev->private;
+	int x, y, z;
+	struct i2c_client *client = data->client;
+
+	x = read_with_retries(client, 0x29);
+	y = read_with_retries(client, 0x2B);
+	z = read_with_retries(client, 0x2D);
+
+	input_report_abs(input, ABS_X, x);
+	input_report_abs(input, ABS_Y, y);
+	input_report_abs(input, ABS_Z, z);
+	input_sync(input);
+}
+
+static ssize_t data_rate_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lis331dl *data = i2c_get_clientdata(client);
+	unsigned int ret_val;
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	ret_val = i2c_smbus_read_byte_data(client, 0x20);
+	ret_val &= 0x7F;
+
+	switch (val) {
+	case 400:
+		ret_val |= 0x80;
+	case 100:
+		i2c_smbus_write_byte_data(client, 0x20, ret_val);
+		break;
+	default:		
+		mutex_unlock(&data->update_lock);
+		return -EINVAL;
+	}
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
+static ssize_t power_mode_store(struct device *dev,
+		struct device_attribute *attr, const  char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lis331dl *data = i2c_get_clientdata(client);
+	unsigned int ret_val;
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	ret_val = i2c_smbus_read_byte_data(client, 0x20);
+	ret_val &= 0xBF;
+	
+	switch(val) {
+	case 1:
+		ret_val |= 0x40;
+	case 0:
+		i2c_smbus_write_byte_data(client, 0x20, ret_val);
+		break;
+	default:
+		mutex_unlock(&data->update_lock);
+		return -EINVAL;
+	}
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
+static DEVICE_ATTR(data_rate, S_IRUGO | S_IWUSR,
+	data_rate_show, data_rate_store);
+static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR,
+	power_mode_show, power_mode_store);
+
+static struct attribute *mid_att_accelero[] = {
+	&dev_attr_data_rate.attr,
+	&dev_attr_power_state.attr,
+	NULL
+};
+
+static struct attribute_group m_accelero_gr = {
+	.name = "lis331dl",
+	.attrs = mid_att_accelero
+};
+
+static void accel_set_default_config(struct i2c_client *client)
+{
+	i2c_smbus_write_byte_data(client, 0x20, 0x47);
+}
+
+static int  lis331dl_probe(struct i2c_client *client,
+					const struct i2c_device_id *id)
+{
+	int res;
+	struct lis331dl *data;
+	struct input_polled_dev *idev;
+	struct input_dev *input;
+
+	data = kzalloc(sizeof(struct lis331dl), GFP_KERNEL);
+	if (data == NULL) {
+		dev_err(&client->dev, "lis331dl: out of memory\n");
+		return -ENOMEM;
+	}
+	mutex_init(&data->update_lock);
+	i2c_set_clientdata(client, data);
+	data->client = client;
+
+	res = sysfs_create_group(&client->dev.kobj, &m_accelero_gr);
+	if (res) {
+		dev_err(&client->dev, "lis331dl: sysfs  group failed\n");
+		goto accelero_error1;
+	}
+	idev = input_allocate_polled_device();
+	if (!idev) {
+		res = -ENOMEM;
+		dev_err(&client->dev,
+			"lis331dl: unable to register input device\n");
+		goto accelero_error2;
+	}
+	idev->poll = lis331dl_poll;
+	idev->poll_interval = POLL_INTERVAL;
+
+	idev->private = data;
+	data->idev =  idev;
+
+	input = idev->input;
+	input->name = "lis331dl";
+	input->phys = "lis331dl/input0";
+	input->id.bustype = BUS_I2C;
+	input->dev.parent = &client->dev;
+	input->evbit[0] = BIT_MASK(EV_ABS);
+	input_set_abs_params(input, ABS_X, 0, 255, 2, 2);
+	input_set_abs_params(input, ABS_Y, 0, 255, 2, 2);
+	input_set_abs_params(input, ABS_Z, 0, 255, 2, 2);
+
+	res = input_register_polled_device(idev);
+	if (res == 0) {
+		dev_info(&client->dev,
+			"%s lis331dl:  Accelerometer chip found",
+							client->name);
+		return 0;
+	}
+	input_free_polled_device(idev);
+accelero_error2:
+	sysfs_remove_group(&client->dev.kobj, &m_accelero_gr);
+accelero_error1:
+	i2c_set_clientdata(client, NULL);
+	kfree(data);
+	return res;
+}
+
+static int lis331dl_remove(struct i2c_client *client)
+{
+	struct lis331dl *data = i2c_get_clientdata(client);
+
+	input_unregister_polled_device(data->idev);
+	input_free_polled_device(data->idev);
+	sysfs_remove_group(&client->dev.kobj, &m_accelero_gr);
+	kfree(data);
+	return 0;
+}
+
+static struct i2c_device_id lis331dl_id[] = {
+	{ "i2c_accel", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, lis331dl_id);
+
+static struct i2c_driver lis331dl_driver = {
+	.driver = {
+	.name = "lis331dl",
+	},
+	.probe = lis331dl_probe,
+	.remove = lis331dl_remove,
+	.id_table = lis331dl_id,
+};
+
+static int __init sensor_lis331dl_init(void)
+{
+	int res;
+
+	res = i2c_add_driver(&lis331dl_driver);
+	return res;
+}
+
+static void  __exit sensor_lis331dl_exit(void)
+{
+	i2c_del_driver(&lis331dl_driver);
+}
+
+module_init(sensor_lis331dl_init);
+module_exit(sensor_lis331dl_exit);
+
+MODULE_AUTHOR("Kalhan Trisal <[email protected]>, Alan Cox");
+MODULE_DESCRIPTION("STMicroelectronics LIS331DL Accelerometer Driver");
+MODULE_LICENSE("GPL v2");
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#22 lore raw

Re: [PATCH 3/4] liss331d1: accelerometer driver

Quoted 17 lines · Éric Piel

Op 14-04-10 14:52, Alan Cox schreef:

Quoted 7 lines

From: Kalhan Trisal <[email protected]>

The acceleremeter driver reads the x,y,z coordinate registers and provides the information to user through the input layer

Conversion to input device, clean up and porting of retry fixes needed for AAVA done by Alan Cox.

Hello,

I wouldn't want to be too picky, if this driver works fine with your hardware and it's tested, why not, but from looking at the spec of this chip (and at the code of this driver), it looks like it could be completely compatible with the lis3lv02d driver (with lis3lv02d_i2c). Same registers, including the WHO_AM_I register which returns 0x3B (which is a supported version).

Quoted 3 lines

Have you tried and there were some specific incompatibilities? To me, it looks like the only thing not in the lis3lv02d is the read with multiple retries, and it could be easily added if necessary for your hardware.

Yes, that would be better. Also because there's also a SPI version of the lis331dl. If support for this chip would be incorporated in the existing driver, they would automatically be supported as well.

Daniel

Quoted 346 lines
Quoted 336 lines

---

 drivers/hwmon/Kconfig    |    8 +
 drivers/hwmon/Makefile   |    1 
 drivers/hwmon/lis331dl.c |  286 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 295 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/lis331dl.c
drivers/hwmon/Kconfig +8 -0
index 1fa2533..6868b9d 100644
@@ -1104,6 +1104,14 @@ config SENSORS_ISL29020
 	  Range values can be configured using sysfs.
 	  Lux data is accessible via sysfs.
 
+config SENSORS_LIS331DL
+	tristate "STMicroeletronics LIS331DL three-axis digital accelerometer"
+	depends on I2C
+	help
+	  If you say yes here you get support for the Accelerometer  Devices
+	  Device can be configured using sysfs.
+	  x y Z data can be   accessible via sysfs.
+
 if ACPI
 
 comment "ACPI drivers"
drivers/hwmon/Makefile +1 -0
index 13d6832..ebeb2a2 100644
@@ -60,6 +60,7 @@ obj-$(CONFIG_SENSORS_K10TEMP)	+= k10temp.o
 obj-$(CONFIG_SENSORS_LIS3LV02D) += lis3lv02d.o hp_accel.o
 obj-$(CONFIG_SENSORS_LIS3_SPI)	+= lis3lv02d.o lis3lv02d_spi.o
 obj-$(CONFIG_SENSORS_LIS3_I2C)	+= lis3lv02d.o lis3lv02d_i2c.o
+obj-$(CONFIG_SENSORS_LIS331DL)	+= lis331dl.o
 obj-$(CONFIG_SENSORS_LM63)	+= lm63.o
 obj-$(CONFIG_SENSORS_LM70)	+= lm70.o
 obj-$(CONFIG_SENSORS_LM73)	+= lm73.o
drivers/hwmon/lis331dl.c new file +286 -1
new file mode 100644
index 0000000..34009eb
@@ -0,0 +1,286 @@
+/*
+ * lis331dl.c - ST LIS331DL  Accelerometer Driver
+ *
+ * Copyright (C) 2009 Intel Corp
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * 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, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/mutex.h>
+#include <linux/sysfs.h>
+#include <linux/input-polldev.h>
+
+
+#define POLL_INTERVAL	50
+
+struct lis331dl {
+	struct input_polled_dev *idev;
+	struct i2c_client *client;
+	struct mutex update_lock;
+};
+
+static ssize_t data_rate_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int val;
+	int speed = 100;
+
+	val = i2c_smbus_read_byte_data(client, 0x20);
+	if (val & 0x80); /* 1= 400HZ 0= 100HZ */
+		speed = 400;
+	return sprintf(buf, "%d\n", speed);
+}
+
+static ssize_t power_mode_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int val;
+
+	val = i2c_smbus_read_byte_data(client, 0x20) & 0x40;
+	if (val == 0x40)
+		val = 1;
+	return sprintf(buf, "%d\n", val);
+}
+
+static int read_with_retries(struct i2c_client *client, int r)
+{
+	int retries;
+	int ret;
+	
+	for (retries = 0; retries < 5; retries++) {
+		ret = i2c_smbus_read_byte_data(client, r);
+		if (ret != -ETIMEDOUT)
+			break;
+		msleep(10);
+	}
+	return ret;
+}
+
+static void lis331dl_poll(struct input_polled_dev *idev)
+{
+	struct input_dev *input = idev->input;
+	struct lis331dl *data = idev->private;
+	int x, y, z;
+	struct i2c_client *client = data->client;
+
+	x = read_with_retries(client, 0x29);
+	y = read_with_retries(client, 0x2B);
+	z = read_with_retries(client, 0x2D);
+
+	input_report_abs(input, ABS_X, x);
+	input_report_abs(input, ABS_Y, y);
+	input_report_abs(input, ABS_Z, z);
+	input_sync(input);
+}
+
+static ssize_t data_rate_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lis331dl *data = i2c_get_clientdata(client);
+	unsigned int ret_val;
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	ret_val = i2c_smbus_read_byte_data(client, 0x20);
+	ret_val &= 0x7F;
+
+	switch (val) {
+	case 400:
+		ret_val |= 0x80;
+	case 100:
+		i2c_smbus_write_byte_data(client, 0x20, ret_val);
+		break;
+	default:		
+		mutex_unlock(&data->update_lock);
+		return -EINVAL;
+	}
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
+static ssize_t power_mode_store(struct device *dev,
+		struct device_attribute *attr, const  char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lis331dl *data = i2c_get_clientdata(client);
+	unsigned int ret_val;
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	ret_val = i2c_smbus_read_byte_data(client, 0x20);
+	ret_val &= 0xBF;
+	
+	switch(val) {
+	case 1:
+		ret_val |= 0x40;
+	case 0:
+		i2c_smbus_write_byte_data(client, 0x20, ret_val);
+		break;
+	default:
+		mutex_unlock(&data->update_lock);
+		return -EINVAL;
+	}
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
+static DEVICE_ATTR(data_rate, S_IRUGO | S_IWUSR,
+	data_rate_show, data_rate_store);
+static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR,
+	power_mode_show, power_mode_store);
+
+static struct attribute *mid_att_accelero[] = {
+	&dev_attr_data_rate.attr,
+	&dev_attr_power_state.attr,
+	NULL
+};
+
+static struct attribute_group m_accelero_gr = {
+	.name = "lis331dl",
+	.attrs = mid_att_accelero
+};
+
+static void accel_set_default_config(struct i2c_client *client)
+{
+	i2c_smbus_write_byte_data(client, 0x20, 0x47);
+}
+
+static int  lis331dl_probe(struct i2c_client *client,
+					const struct i2c_device_id *id)
+{
+	int res;
+	struct lis331dl *data;
+	struct input_polled_dev *idev;
+	struct input_dev *input;
+
+	data = kzalloc(sizeof(struct lis331dl), GFP_KERNEL);
+	if (data == NULL) {
+		dev_err(&client->dev, "lis331dl: out of memory\n");
+		return -ENOMEM;
+	}
+	mutex_init(&data->update_lock);
+	i2c_set_clientdata(client, data);
+	data->client = client;
+
+	res = sysfs_create_group(&client->dev.kobj, &m_accelero_gr);
+	if (res) {
+		dev_err(&client->dev, "lis331dl: sysfs  group failed\n");
+		goto accelero_error1;
+	}
+	idev = input_allocate_polled_device();
+	if (!idev) {
+		res = -ENOMEM;
+		dev_err(&client->dev,
+			"lis331dl: unable to register input device\n");
+		goto accelero_error2;
+	}
+	idev->poll = lis331dl_poll;
+	idev->poll_interval = POLL_INTERVAL;
+
+	idev->private = data;
+	data->idev =  idev;
+
+	input = idev->input;
+	input->name = "lis331dl";
+	input->phys = "lis331dl/input0";
+	input->id.bustype = BUS_I2C;
+	input->dev.parent = &client->dev;
+	input->evbit[0] = BIT_MASK(EV_ABS);
+	input_set_abs_params(input, ABS_X, 0, 255, 2, 2);
+	input_set_abs_params(input, ABS_Y, 0, 255, 2, 2);
+	input_set_abs_params(input, ABS_Z, 0, 255, 2, 2);
+
+	res = input_register_polled_device(idev);
+	if (res == 0) {
+		dev_info(&client->dev,
+			"%s lis331dl:  Accelerometer chip found",
+							client->name);
+		return 0;
+	}
+	input_free_polled_device(idev);
+accelero_error2:
+	sysfs_remove_group(&client->dev.kobj, &m_accelero_gr);
+accelero_error1:
+	i2c_set_clientdata(client, NULL);
+	kfree(data);
+	return res;
+}
+
+static int lis331dl_remove(struct i2c_client *client)
+{
+	struct lis331dl *data = i2c_get_clientdata(client);
+
+	input_unregister_polled_device(data->idev);
+	input_free_polled_device(data->idev);
+	sysfs_remove_group(&client->dev.kobj, &m_accelero_gr);
+	kfree(data);
+	return 0;
+}
+
+static struct i2c_device_id lis331dl_id[] = {
+	{ "i2c_accel", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, lis331dl_id);
+
+static struct i2c_driver lis331dl_driver = {
+	.driver = {
+	.name = "lis331dl",
+	},
+	.probe = lis331dl_probe,
+	.remove = lis331dl_remove,
+	.id_table = lis331dl_id,
+};
+
+static int __init sensor_lis331dl_init(void)
+{
+	int res;
+
+	res = i2c_add_driver(&lis331dl_driver);
+	return res;
+}
+
+static void  __exit sensor_lis331dl_exit(void)
+{
+	i2c_del_driver(&lis331dl_driver);
+}
+
+module_init(sensor_lis331dl_init);
+module_exit(sensor_lis331dl_exit);
+
+MODULE_AUTHOR("Kalhan Trisal <[email protected]>, Alan Cox");
+MODULE_DESCRIPTION("STMicroelectronics LIS331DL Accelerometer Driver");
+MODULE_LICENSE("GPL v2");
 
--
Quoted 4 lines

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#23 lore raw

Re: [PATCH 3/4] liss331d1: accelerometer driver

Quoted 18 lines · On Thu, 15 Apr 2010 00:34:29 +0200 Daniel Mack…
Quoted 17 lines · Éric Piel

Op 14-04-10 14:52, Alan Cox schreef:

Quoted 7 lines

From: Kalhan Trisal <[email protected]>

The acceleremeter driver reads the x,y,z coordinate registers and provides the information to user through the input layer

Conversion to input device, clean up and porting of retry fixes needed for AAVA done by Alan Cox.

Hello,

I wouldn't want to be too picky, if this driver works fine with your hardware and it's tested, why not, but from looking at the spec of this chip (and at the code of this driver), it looks like it could be completely compatible with the lis3lv02d driver (with lis3lv02d_i2c). Same registers, including the WHO_AM_I register which returns 0x3B (which is a supported version).

Quoted 8 lines
Quoted 3 lines

Have you tried and there were some specific incompatibilities? To me, it looks like the only thing not in the lis3lv02d is the read with multiple retries, and it could be easily added if necessary for your hardware.

Yes, that would be better. Also because there's also a SPI version of the lis331dl. If support for this chip would be incorporated in the existing driver, they would automatically be supported as well.

The Openmoko Freerunner has as lis302D which is an SPI version of a very similar device, so making the one driver work on that was well would be nice if possible.

The lis302d can detect threshold crossings and taps as well as simple orientation and I found it useful to include support for those in the driver as well.
In particular:
1/ I allowed the 'data_rate' that was programmed to be any number (including
fractions and 0) and the driver would report at that rate, using a timer for rates below 50Hz.
When the app is only interested in long-term change, this can significantly reduce the amount of data that has to be handled by userspace.

2/ I allowed a 'threshold' to be set so that changes bigger than that get
notified promptly even if that is faster than the requested data rate

3/ I allowed taps to be detected and reported as BTN_X BTN_Y and BTN_Z
input events.

Would it be sensible to include that support in this driver? The LIS331DL seem to have the same threshold and tap support.

NeilBrown

Quoted 354 lines

Daniel

Quoted 335 lines
Quoted 335 lines

---

 drivers/hwmon/Kconfig    |    8 +
 drivers/hwmon/Makefile   |    1 
 drivers/hwmon/lis331dl.c |  286 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 295 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/lis331dl.c
drivers/hwmon/Kconfig +8 -0
index 1fa2533..6868b9d 100644
@@ -1104,6 +1104,14 @@ config SENSORS_ISL29020
 	  Range values can be configured using sysfs.
 	  Lux data is accessible via sysfs.
 
+config SENSORS_LIS331DL
+	tristate "STMicroeletronics LIS331DL three-axis digital accelerometer"
+	depends on I2C
+	help
+	  If you say yes here you get support for the Accelerometer  Devices
+	  Device can be configured using sysfs.
+	  x y Z data can be   accessible via sysfs.
+
 if ACPI
 
 comment "ACPI drivers"
drivers/hwmon/Makefile +1 -0
index 13d6832..ebeb2a2 100644
@@ -60,6 +60,7 @@ obj-$(CONFIG_SENSORS_K10TEMP)	+= k10temp.o
 obj-$(CONFIG_SENSORS_LIS3LV02D) += lis3lv02d.o hp_accel.o
 obj-$(CONFIG_SENSORS_LIS3_SPI)	+= lis3lv02d.o lis3lv02d_spi.o
 obj-$(CONFIG_SENSORS_LIS3_I2C)	+= lis3lv02d.o lis3lv02d_i2c.o
+obj-$(CONFIG_SENSORS_LIS331DL)	+= lis331dl.o
 obj-$(CONFIG_SENSORS_LM63)	+= lm63.o
 obj-$(CONFIG_SENSORS_LM70)	+= lm70.o
 obj-$(CONFIG_SENSORS_LM73)	+= lm73.o
drivers/hwmon/lis331dl.c new file +286 -0
new file mode 100644
index 0000000..34009eb
@@ -0,0 +1,286 @@
+/*
+ * lis331dl.c - ST LIS331DL  Accelerometer Driver
+ *
+ * Copyright (C) 2009 Intel Corp
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * 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, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/mutex.h>
+#include <linux/sysfs.h>
+#include <linux/input-polldev.h>
+
+
+#define POLL_INTERVAL	50
+
+struct lis331dl {
+	struct input_polled_dev *idev;
+	struct i2c_client *client;
+	struct mutex update_lock;
+};
+
+static ssize_t data_rate_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int val;
+	int speed = 100;
+
+	val = i2c_smbus_read_byte_data(client, 0x20);
+	if (val & 0x80); /* 1= 400HZ 0= 100HZ */
+		speed = 400;
+	return sprintf(buf, "%d\n", speed);
+}
+
+static ssize_t power_mode_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int val;
+
+	val = i2c_smbus_read_byte_data(client, 0x20) & 0x40;
+	if (val == 0x40)
+		val = 1;
+	return sprintf(buf, "%d\n", val);
+}
+
+static int read_with_retries(struct i2c_client *client, int r)
+{
+	int retries;
+	int ret;
+	
+	for (retries = 0; retries < 5; retries++) {
+		ret = i2c_smbus_read_byte_data(client, r);
+		if (ret != -ETIMEDOUT)
+			break;
+		msleep(10);
+	}
+	return ret;
+}
+
+static void lis331dl_poll(struct input_polled_dev *idev)
+{
+	struct input_dev *input = idev->input;
+	struct lis331dl *data = idev->private;
+	int x, y, z;
+	struct i2c_client *client = data->client;
+
+	x = read_with_retries(client, 0x29);
+	y = read_with_retries(client, 0x2B);
+	z = read_with_retries(client, 0x2D);
+
+	input_report_abs(input, ABS_X, x);
+	input_report_abs(input, ABS_Y, y);
+	input_report_abs(input, ABS_Z, z);
+	input_sync(input);
+}
+
+static ssize_t data_rate_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lis331dl *data = i2c_get_clientdata(client);
+	unsigned int ret_val;
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	ret_val = i2c_smbus_read_byte_data(client, 0x20);
+	ret_val &= 0x7F;
+
+	switch (val) {
+	case 400:
+		ret_val |= 0x80;
+	case 100:
+		i2c_smbus_write_byte_data(client, 0x20, ret_val);
+		break;
+	default:		
+		mutex_unlock(&data->update_lock);
+		return -EINVAL;
+	}
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
+static ssize_t power_mode_store(struct device *dev,
+		struct device_attribute *attr, const  char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lis331dl *data = i2c_get_clientdata(client);
+	unsigned int ret_val;
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	ret_val = i2c_smbus_read_byte_data(client, 0x20);
+	ret_val &= 0xBF;
+	
+	switch(val) {
+	case 1:
+		ret_val |= 0x40;
+	case 0:
+		i2c_smbus_write_byte_data(client, 0x20, ret_val);
+		break;
+	default:
+		mutex_unlock(&data->update_lock);
+		return -EINVAL;
+	}
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
+static DEVICE_ATTR(data_rate, S_IRUGO | S_IWUSR,
+	data_rate_show, data_rate_store);
+static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR,
+	power_mode_show, power_mode_store);
+
+static struct attribute *mid_att_accelero[] = {
+	&dev_attr_data_rate.attr,
+	&dev_attr_power_state.attr,
+	NULL
+};
+
+static struct attribute_group m_accelero_gr = {
+	.name = "lis331dl",
+	.attrs = mid_att_accelero
+};
+
+static void accel_set_default_config(struct i2c_client *client)
+{
+	i2c_smbus_write_byte_data(client, 0x20, 0x47);
+}
+
+static int  lis331dl_probe(struct i2c_client *client,
+					const struct i2c_device_id *id)
+{
+	int res;
+	struct lis331dl *data;
+	struct input_polled_dev *idev;
+	struct input_dev *input;
+
+	data = kzalloc(sizeof(struct lis331dl), GFP_KERNEL);
+	if (data == NULL) {
+		dev_err(&client->dev, "lis331dl: out of memory\n");
+		return -ENOMEM;
+	}
+	mutex_init(&data->update_lock);
+	i2c_set_clientdata(client, data);
+	data->client = client;
+
+	res = sysfs_create_group(&client->dev.kobj, &m_accelero_gr);
+	if (res) {
+		dev_err(&client->dev, "lis331dl: sysfs  group failed\n");
+		goto accelero_error1;
+	}
+	idev = input_allocate_polled_device();
+	if (!idev) {
+		res = -ENOMEM;
+		dev_err(&client->dev,
+			"lis331dl: unable to register input device\n");
+		goto accelero_error2;
+	}
+	idev->poll = lis331dl_poll;
+	idev->poll_interval = POLL_INTERVAL;
+
+	idev->private = data;
+	data->idev =  idev;
+
+	input = idev->input;
+	input->name = "lis331dl";
+	input->phys = "lis331dl/input0";
+	input->id.bustype = BUS_I2C;
+	input->dev.parent = &client->dev;
+	input->evbit[0] = BIT_MASK(EV_ABS);
+	input_set_abs_params(input, ABS_X, 0, 255, 2, 2);
+	input_set_abs_params(input, ABS_Y, 0, 255, 2, 2);
+	input_set_abs_params(input, ABS_Z, 0, 255, 2, 2);
+
+	res = input_register_polled_device(idev);
+	if (res == 0) {
+		dev_info(&client->dev,
+			"%s lis331dl:  Accelerometer chip found",
+							client->name);
+		return 0;
+	}
+	input_free_polled_device(idev);
+accelero_error2:
+	sysfs_remove_group(&client->dev.kobj, &m_accelero_gr);
+accelero_error1:
+	i2c_set_clientdata(client, NULL);
+	kfree(data);
+	return res;
+}
+
+static int lis331dl_remove(struct i2c_client *client)
+{
+	struct lis331dl *data = i2c_get_clientdata(client);
+
+	input_unregister_polled_device(data->idev);
+	input_free_polled_device(data->idev);
+	sysfs_remove_group(&client->dev.kobj, &m_accelero_gr);
+	kfree(data);
+	return 0;
+}
+
+static struct i2c_device_id lis331dl_id[] = {
+	{ "i2c_accel", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, lis331dl_id);
+
+static struct i2c_driver lis331dl_driver = {
+	.driver = {
+	.name = "lis331dl",
+	},
+	.probe = lis331dl_probe,
+	.remove = lis331dl_remove,
+	.id_table = lis331dl_id,
+};
+
+static int __init sensor_lis331dl_init(void)
+{
+	int res;
+
+	res = i2c_add_driver(&lis331dl_driver);
+	return res;
+}
+
+static void  __exit sensor_lis331dl_exit(void)
+{
+	i2c_del_driver(&lis331dl_driver);
+}
+
+module_init(sensor_lis331dl_init);
+module_exit(sensor_lis331dl_exit);
+
+MODULE_AUTHOR("Kalhan Trisal <[email protected]>, Alan Cox");
+MODULE_DESCRIPTION("STMicroelectronics LIS331DL Accelerometer Driver");
+MODULE_LICENSE("GPL v2");
Quoted 11 lines
Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#24 lore raw

Re: [PATCH 3/4] liss331d1: accelerometer driver

On 15/04/10 01:05, Neil Brown wrote:
:

Quoted 4 lines

The Openmoko Freerunner has as lis302D which is an SPI version of a very similar device, so making the one driver work on that was well would be nice if possible.

Definitely!

Quoted 11 lines

The lis302d can detect threshold crossings and taps as well as simple orientation and I found it useful to include support for those in the driver as well.
In particular:
1/ I allowed the 'data_rate' that was programmed to be any number (including
fractions and 0) and the driver would report at that rate, using a timer for rates below 50Hz.
When the app is only interested in long-term change, this can significantly reduce the amount of data that has to be handled by userspace.

Samu (from Nokia) has started to had such idea via the patch lis3-setup-poll-interval-limits.patch which in now in the -mm tree. It allows to specify the polling rate via the input interface (/sys/devices/platform/lis3lv02d/input/input8/poll). But for now I don't think the actual refresh rate is updated automatically, it would be a nice addition.

Quoted 3 lines

2/ I allowed a 'threshold' to be set so that changes bigger than that get
notified promptly even if that is faster than the requested data rate

It could be interesting too... but what interface are you using for the userspace to specify this threshold? A sysfs file?

Quoted 3 lines

3/ I allowed taps to be detected and reported as BTN_X BTN_Y and BTN_Z
input events.

This one should now be working with Samu's patch lis3-interrupt-handlers-for-8bit-wakeup-and-click-events.patch also in the -mm tree.

Quoted 3 lines

Would it be sensible to include that support in this driver? The LIS331DL seem to have the same threshold and tap support.

So all in all, if you could have a look at what changes are necessary to get the lis3lv02d driver work with your hardware, it would be great. (I suspect it should be quite little.)

See you,
Eric

author
#25 lore raw

Re: [PATCH 3/4] liss331d1: accelerometer driver

In reply to Éric Piel
Quoted 6 lines

I wouldn't want to be too picky, if this driver works fine with your hardware and it's tested, why not, but from looking at the spec of this chip (and at the code of this driver), it looks like it could be completely compatible with the lis3lv02d driver (with lis3lv02d_i2c). Same registers, including the WHO_AM_I register which returns 0x3B (which is a supported version).

I will take a look in detail. The IRQ bit is different and as you say the retries can be added to it anyway.

Quoted 2 lines

Have you tried and there were some specific incompatibilities?

I am picking this project up rather than being directly responsible for its history - chances are nobody has.

Alan

#26 lore raw

Re: [PATCH 3/4] liss331d1: accelerometer driver

In reply to Éric Piel
Quoted 17 lines · Éric Piel

Op 14-04-10 14:52, Alan Cox schreef:

Quoted 7 lines

From: Kalhan Trisal <[email protected]>

The acceleremeter driver reads the x,y,z coordinate registers and provides the information to user through the input layer

Conversion to input device, clean up and porting of retry fixes needed for AAVA done by Alan Cox.

Hello,

I wouldn't want to be too picky, if this driver works fine with your hardware and it's tested, why not, but from looking at the spec of this chip (and at the code of this driver), it looks like it could be completely compatible with the lis3lv02d driver (with lis3lv02d_i2c). Same registers, including the WHO_AM_I register which returns 0x3B (which is a supported version).

Quoted 6 lines

Have you tried and there were some specific incompatibilities? To me, it looks like the only thing not in the lis3lv02d is the read with multiple retries, and it could be easily added if necessary for your hardware.

Cheers,
Eric

Err. Anyone get a feeling of deja vu here?

http://lists.lm-sensors.org/pipermail/lm-sensors/2009-September/026706.html

When Kalhan originally posted this driver it was pointed out that it was compatible with the existing one. A complete lack of communications lead to Kalhan (and someone else, might have been Eric or Samu, can't recall) both implementing i2c support in the driver. Can't find it right now but I'm fairly sure Kahlan reported that worked fine for this chip as well?

So looks like a lack of communications here and that Alan has picked up an unnecessary driver.

Jonathan

Quoted 337 lines
Quoted 337 lines

---

 drivers/hwmon/Kconfig    |    8 +
 drivers/hwmon/Makefile   |    1 
 drivers/hwmon/lis331dl.c |  286 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 295 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/lis331dl.c
drivers/hwmon/Kconfig +8 -0
index 1fa2533..6868b9d 100644
@@ -1104,6 +1104,14 @@ config SENSORS_ISL29020
 	  Range values can be configured using sysfs.
 	  Lux data is accessible via sysfs.
 
+config SENSORS_LIS331DL
+	tristate "STMicroeletronics LIS331DL three-axis digital accelerometer"
+	depends on I2C
+	help
+	  If you say yes here you get support for the Accelerometer  Devices
+	  Device can be configured using sysfs.
+	  x y Z data can be   accessible via sysfs.
+
 if ACPI
 
 comment "ACPI drivers"
drivers/hwmon/Makefile +1 -0
index 13d6832..ebeb2a2 100644
@@ -60,6 +60,7 @@ obj-$(CONFIG_SENSORS_K10TEMP)	+= k10temp.o
 obj-$(CONFIG_SENSORS_LIS3LV02D) += lis3lv02d.o hp_accel.o
 obj-$(CONFIG_SENSORS_LIS3_SPI)	+= lis3lv02d.o lis3lv02d_spi.o
 obj-$(CONFIG_SENSORS_LIS3_I2C)	+= lis3lv02d.o lis3lv02d_i2c.o
+obj-$(CONFIG_SENSORS_LIS331DL)	+= lis331dl.o
 obj-$(CONFIG_SENSORS_LM63)	+= lm63.o
 obj-$(CONFIG_SENSORS_LM70)	+= lm70.o
 obj-$(CONFIG_SENSORS_LM73)	+= lm73.o
drivers/hwmon/lis331dl.c new file +286 -0
new file mode 100644
index 0000000..34009eb
@@ -0,0 +1,286 @@
+/*
+ * lis331dl.c - ST LIS331DL  Accelerometer Driver
+ *
+ * Copyright (C) 2009 Intel Corp
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * 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, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/mutex.h>
+#include <linux/sysfs.h>
+#include <linux/input-polldev.h>
+
+
+#define POLL_INTERVAL	50
+
+struct lis331dl {
+	struct input_polled_dev *idev;
+	struct i2c_client *client;
+	struct mutex update_lock;
+};
+
+static ssize_t data_rate_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int val;
+	int speed = 100;
+
+	val = i2c_smbus_read_byte_data(client, 0x20);
+	if (val & 0x80); /* 1= 400HZ 0= 100HZ */
+		speed = 400;
+	return sprintf(buf, "%d\n", speed);
+}
+
+static ssize_t power_mode_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int val;
+
+	val = i2c_smbus_read_byte_data(client, 0x20) & 0x40;
+	if (val == 0x40)
+		val = 1;
+	return sprintf(buf, "%d\n", val);
+}
+
+static int read_with_retries(struct i2c_client *client, int r)
+{
+	int retries;
+	int ret;
+	
+	for (retries = 0; retries < 5; retries++) {
+		ret = i2c_smbus_read_byte_data(client, r);
+		if (ret != -ETIMEDOUT)
+			break;
+		msleep(10);
+	}
+	return ret;
+}
+
+static void lis331dl_poll(struct input_polled_dev *idev)
+{
+	struct input_dev *input = idev->input;
+	struct lis331dl *data = idev->private;
+	int x, y, z;
+	struct i2c_client *client = data->client;
+
+	x = read_with_retries(client, 0x29);
+	y = read_with_retries(client, 0x2B);
+	z = read_with_retries(client, 0x2D);
+
+	input_report_abs(input, ABS_X, x);
+	input_report_abs(input, ABS_Y, y);
+	input_report_abs(input, ABS_Z, z);
+	input_sync(input);
+}
+
+static ssize_t data_rate_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lis331dl *data = i2c_get_clientdata(client);
+	unsigned int ret_val;
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	ret_val = i2c_smbus_read_byte_data(client, 0x20);
+	ret_val &= 0x7F;
+
+	switch (val) {
+	case 400:
+		ret_val |= 0x80;
+	case 100:
+		i2c_smbus_write_byte_data(client, 0x20, ret_val);
+		break;
+	default:		
+		mutex_unlock(&data->update_lock);
+		return -EINVAL;
+	}
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
+static ssize_t power_mode_store(struct device *dev,
+		struct device_attribute *attr, const  char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lis331dl *data = i2c_get_clientdata(client);
+	unsigned int ret_val;
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+
+	ret_val = i2c_smbus_read_byte_data(client, 0x20);
+	ret_val &= 0xBF;
+	
+	switch(val) {
+	case 1:
+		ret_val |= 0x40;
+	case 0:
+		i2c_smbus_write_byte_data(client, 0x20, ret_val);
+		break;
+	default:
+		mutex_unlock(&data->update_lock);
+		return -EINVAL;
+	}
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
+static DEVICE_ATTR(data_rate, S_IRUGO | S_IWUSR,
+	data_rate_show, data_rate_store);
+static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR,
+	power_mode_show, power_mode_store);
+
+static struct attribute *mid_att_accelero[] = {
+	&dev_attr_data_rate.attr,
+	&dev_attr_power_state.attr,
+	NULL
+};
+
+static struct attribute_group m_accelero_gr = {
+	.name = "lis331dl",
+	.attrs = mid_att_accelero
+};
+
+static void accel_set_default_config(struct i2c_client *client)
+{
+	i2c_smbus_write_byte_data(client, 0x20, 0x47);
+}
+
+static int  lis331dl_probe(struct i2c_client *client,
+					const struct i2c_device_id *id)
+{
+	int res;
+	struct lis331dl *data;
+	struct input_polled_dev *idev;
+	struct input_dev *input;
+
+	data = kzalloc(sizeof(struct lis331dl), GFP_KERNEL);
+	if (data == NULL) {
+		dev_err(&client->dev, "lis331dl: out of memory\n");
+		return -ENOMEM;
+	}
+	mutex_init(&data->update_lock);
+	i2c_set_clientdata(client, data);
+	data->client = client;
+
+	res = sysfs_create_group(&client->dev.kobj, &m_accelero_gr);
+	if (res) {
+		dev_err(&client->dev, "lis331dl: sysfs  group failed\n");
+		goto accelero_error1;
+	}
+	idev = input_allocate_polled_device();
+	if (!idev) {
+		res = -ENOMEM;
+		dev_err(&client->dev,
+			"lis331dl: unable to register input device\n");
+		goto accelero_error2;
+	}
+	idev->poll = lis331dl_poll;
+	idev->poll_interval = POLL_INTERVAL;
+
+	idev->private = data;
+	data->idev =  idev;
+
+	input = idev->input;
+	input->name = "lis331dl";
+	input->phys = "lis331dl/input0";
+	input->id.bustype = BUS_I2C;
+	input->dev.parent = &client->dev;
+	input->evbit[0] = BIT_MASK(EV_ABS);
+	input_set_abs_params(input, ABS_X, 0, 255, 2, 2);
+	input_set_abs_params(input, ABS_Y, 0, 255, 2, 2);
+	input_set_abs_params(input, ABS_Z, 0, 255, 2, 2);
+
+	res = input_register_polled_device(idev);
+	if (res == 0) {
+		dev_info(&client->dev,
+			"%s lis331dl:  Accelerometer chip found",
+							client->name);
+		return 0;
+	}
+	input_free_polled_device(idev);
+accelero_error2:
+	sysfs_remove_group(&client->dev.kobj, &m_accelero_gr);
+accelero_error1:
+	i2c_set_clientdata(client, NULL);
+	kfree(data);
+	return res;
+}
+
+static int lis331dl_remove(struct i2c_client *client)
+{
+	struct lis331dl *data = i2c_get_clientdata(client);
+
+	input_unregister_polled_device(data->idev);
+	input_free_polled_device(data->idev);
+	sysfs_remove_group(&client->dev.kobj, &m_accelero_gr);
+	kfree(data);
+	return 0;
+}
+
+static struct i2c_device_id lis331dl_id[] = {
+	{ "i2c_accel", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, lis331dl_id);
+
+static struct i2c_driver lis331dl_driver = {
+	.driver = {
+	.name = "lis331dl",
+	},
+	.probe = lis331dl_probe,
+	.remove = lis331dl_remove,
+	.id_table = lis331dl_id,
+};
+
+static int __init sensor_lis331dl_init(void)
+{
+	int res;
+
+	res = i2c_add_driver(&lis331dl_driver);
+	return res;
+}
+
+static void  __exit sensor_lis331dl_exit(void)
+{
+	i2c_del_driver(&lis331dl_driver);
+}
+
+module_init(sensor_lis331dl_init);
+module_exit(sensor_lis331dl_exit);
+
+MODULE_AUTHOR("Kalhan Trisal <[email protected]>, Alan Cox");
+MODULE_DESCRIPTION("STMicroelectronics LIS331DL Accelerometer Driver");
+MODULE_LICENSE("GPL v2");
Quoted 11 lines
Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#27 lore raw

Re: [PATCH 3/4] liss331d1: accelerometer driver

Quoted 11 lines

Err. Anyone get a feeling of deja vu here?

http://lists.lm-sensors.org/pipermail/lm-sensors/2009-September/026706.html

When Kalhan originally posted this driver it was pointed out that it was compatible with the existing one. A complete lack of communications lead to Kalhan (and someone else, might have been Eric or Samu, can't recall) both implementing i2c support in the driver. Can't find it right now but I'm fairly sure Kahlan reported that worked fine for this chip as well?

Quoted 2 lines

So looks like a lack of communications here and that Alan has picked up an unnecessary driver.

Thanks it does indeed - except for the retry logic which has come from another platform source and can be submitted in turn. Only other thing I see odd is that the existing driver prints a scary error message when there is no IRQ rather than just an info message.

Alan

#28 lore raw

Re: [PATCH 3/4] liss331d1: accelerometer driver

In reply to Alan Cox
Quoted 7 lines · Alan Cox

From: Kalhan Trisal <[email protected]>

The acceleremeter driver reads the x,y,z coordinate registers and provides the information to user through the input layer

Conversion to input device, clean up and porting of retry fixes needed for AAVA done by Alan Cox.

Out of interest, what is AAVA?
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
author
#29 lore raw

PATCH 4/4 emc1403: thermal sensor support

In reply to Alan Cox

From: Kalhan Trisal <kalhan.trisal at intel.com>

Provides support for the emc1403 thermal sensor. Only reporting of values is supported. The various Moorestown specific extras to do with thermal alerts and the like are not in this version of the driver.

Signed-off-by Kalhan Trisal <kalhan.trisal at intel.com>

---

 drivers/hwmon/Kconfig   |   10 +
 drivers/hwmon/Makefile  |    1 
 drivers/hwmon/emc1403.c |  437 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 448 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/emc1403.c
drivers/hwmon/Kconfig +10 -0
index 6868b9d..34ee302 100644
@@ -1112,6 +1112,16 @@ config SENSORS_LIS331DL
 	  Device can be configured using sysfs.
 	  x y Z data can be   accessible via sysfs.
 
+config SENSORS_EMC1403
+	tristate "SMSC EMC1403 thermal sensor"
+	depends on I2C
+	help
+	  If you say yes here you get support for the SMSC Devices
+	  EMC1403 temperature monitoring chip.
+
+	  Threshold values can be configured using sysfs.
+	  Data from the different diode are accessible via sysfs.
+
 if ACPI
 
 comment "ACPI drivers"
drivers/hwmon/Makefile +1 -0
index ebeb2a2..a7332ce 100644
@@ -40,6 +40,7 @@ obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
 obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
 obj-$(CONFIG_SENSORS_DME1737)	+= dme1737.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
+obj-$(CONFIG_SENSORS_EMC1403)	+= emc1403.o
 obj-$(CONFIG_SENSORS_F71805F)	+= f71805f.o
 obj-$(CONFIG_SENSORS_F71882FG)	+= f71882fg.o
 obj-$(CONFIG_SENSORS_F75375S)	+= f75375s.o
drivers/hwmon/emc1403.c new file +437 -0

437 lines not on this page · raw message

#30 lore raw

Re: [lm-sensors] [PATCH 4/4] emc1403: thermal sensor support

Hi Alan,

Quoted 8 lines · Alan Cox

From: Kalhan Trisal <kalhan.trisal at intel.com>

Provides support for the emc1403 thermal sensor. Only reporting of values is supported. The various Moorestown specific extras to do with thermal alerts and the like are not in this version of the driver.

Signed-off-by Kalhan Trisal <kalhan.trisal at intel.com>

Quick review follows. There's quite some cleanup work left before this driver can make it upstream.

Quoted 14 lines

---

 drivers/hwmon/Kconfig   |   10 +
 drivers/hwmon/Makefile  |    1 
 drivers/hwmon/emc1403.c |  437 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 448 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/emc1403.c
drivers/hwmon/Kconfig +0 -0
index 6868b9d..34ee302 100644

drivers/hwmon/Kconfig config SENSORS_LIS331DL · line 1116

Quoted 5 lines
drivers/hwmon/Kconfig +2 -0
@@ -1112,6 +1112,16 @@ config SENSORS_LIS331DL
 	  Device can be configured using sysfs.
 	  x y Z data can be   accessible via sysfs.
 
+config SENSORS_EMC1403
+	tristate "SMSC EMC1403 thermal sensor"

Please group with the other SMSC entries.

drivers/hwmon/Kconfig config SENSORS_LIS331DL · line 1120

Quoted 4 lines
drivers/hwmon/Kconfig +4 -0
+	depends on I2C
+	help
+	  If you say yes here you get support for the SMSC Devices
+	  EMC1403 temperature monitoring chip.

"SMSC EMC1403" is enough, "Devices" can be removed IMHO.

drivers/hwmon/Kconfig config SENSORS_LIS331DL · line 1123

Quoted 3 lines
drivers/hwmon/Kconfig +3 -0
+
+	  Threshold values can be configured using sysfs.
+	  Data from the different diode are accessible via sysfs.

Spelling: diodes.

Quoted 41 lines
drivers/hwmon/Kconfig +1 -0
+
 if ACPI
 
 comment "ACPI drivers"
drivers/hwmon/Makefile +1 -0
index ebeb2a2..a7332ce 100644
@@ -40,6 +40,7 @@ obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
 obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
 obj-$(CONFIG_SENSORS_DME1737)	+= dme1737.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
+obj-$(CONFIG_SENSORS_EMC1403)	+= emc1403.o
 obj-$(CONFIG_SENSORS_F71805F)	+= f71805f.o
 obj-$(CONFIG_SENSORS_F71882FG)	+= f71882fg.o
 obj-$(CONFIG_SENSORS_F75375S)	+= f75375s.o
drivers/hwmon/emc1403.c new file +19 -0
new file mode 100644
index 0000000..b71cfa1
@@ -0,0 +1,437 @@
+/*
+ * emc1403.c - SMSC Thermal Driver
+ *
+ * Copyright (C) 2008 Intel Corp
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * 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, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.

drivers/hwmon/emc1403.c line 29

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/hwmon-vid.h>

I pretty much doubt you need <linux/hwmon-vid.h> for a thermal sensor driver.

drivers/hwmon/emc1403.c line 30

Quoted 1 line
drivers/hwmon/emc1403.c new file +1 -0
+#include <linux/interrupt.h>

Not used.

drivers/hwmon/emc1403.c line 31

Quoted 1 line
drivers/hwmon/emc1403.c new file +1 -0
+#include <linux/workqueue.h>

Not really used (see below).

drivers/hwmon/emc1403.c line 33

Quoted 2 lines
drivers/hwmon/emc1403.c new file +2 -0
+#include <linux/err.h>
+#include <linux/delay.h>

Not used.

drivers/hwmon/emc1403.c line 34

Quoted 1 line
drivers/hwmon/emc1403.c new file +1 -0
+#include <linux/mutex.h>

Not used.

drivers/hwmon/emc1403.c line 39

Quoted 5 lines
drivers/hwmon/emc1403.c new file +5 -0
+#include <linux/sysfs.h>
+
+
+/* Limit status reg Therm/High/Low/Fault*/
+static const u8 THM_STAT_REG_TEMP[] = { 0x37, 0x35, 0x36, 0x1B, 0x02};

Missing space before closing curly brace. This array isn't used by the driver anyway...

drivers/hwmon/emc1403.c line 42

Quoted 3 lines
drivers/hwmon/emc1403.c new file +3 -0
+
+/* Channel  diode temp set */
+static const u8 THM_CHAN_TEMP[] = { 0x10, 0x08, 0x04, 0x02, 0x01 };

Not used either.

drivers/hwmon/emc1403.c line 49

Quoted 7 lines
drivers/hwmon/emc1403.c new file +7 -0
+
+/* Therm Limit reg store values */
+static const u8 THM_LIMIT_REG_TEMP[] = { 0x05, 0x06, 0x07, 0x08, 0x15, 0x16,
+						0x19, 0x1A, 0x20, 0x21 };
+
+/* DATA REGISTERS */
+static const u8 THM_REG_CURR_TEMP[] = { 0x00, 0x01, 0x23 };

The names are slightly inconsistent (place of "_REG_".)

drivers/hwmon/emc1403.c line 54

Quoted 5 lines
drivers/hwmon/emc1403.c new file +5 -0
+
+#define THERMAL_PID_REG		0xfd
+#define THERMAL_SMSC_ID_REG	0xfe
+#define THERMAL_REVISION_REG	0xff
+#define THERMAL_ADC_UPDATE_BUSY	0x80

Not used.

drivers/hwmon/emc1403.c line 55

Quoted 1 line
drivers/hwmon/emc1403.c new file +1 -0
+#define I2C_THERMAL_SLAVE_ADDR	0x4C

Not used.

drivers/hwmon/emc1403.c line 58

Quoted 3 lines
drivers/hwmon/emc1403.c new file +3 -0
+#define TEMP1			1
+#define TEMP2			2
+#define TEMP3			4

These names are pretty vague, aren't they? And you don't even use these defines consistently.

drivers/hwmon/emc1403.c line 59

Quoted 1 line
drivers/hwmon/emc1403.c new file +1 -0
+#define IRQ_TYPE_MASK		(1 << 15)

Not used.

drivers/hwmon/emc1403.c line 64

Quoted 5 lines
drivers/hwmon/emc1403.c new file +5 -0
+#define HIGH_EVENT		1
+#define LOW_EVENT		2
+#define THERM_EVENT		3
+#define FAULT_EVENT		4
+#define ALERT_EVENT		1

None of these is used.

drivers/hwmon/emc1403.c line 73

Quoted 9 lines
drivers/hwmon/emc1403.c new file +9 -0
+
+struct thermal_data {
+	struct i2c_client *client;
+	struct device *hwmon_dev;
+	int therm_irq;
+	int alert_irq;
+	struct work_struct therm_handler;
+	struct work_struct alert_handler;
+};

The last 4 members of this structure aren't used anywhere in the driver.

Quoted 25 lines
drivers/hwmon/emc1403.c new file +25 -0
+
+static int calculate_offset(int type, int temp_ofs)
+{
+	int offset = 0;
+
+	switch (type) {
+	case TEMP1:
+		if (temp_ofs = 0)
+			offset = 1;
+		else if (temp_ofs = 1)
+			offset = 0;
+		else if (temp_ofs = 2)
+			offset = 8;
+		break;
+	case TEMP2:
+		if (temp_ofs = 0)
+			offset = 3;
+		else if (temp_ofs = 1)
+			offset = 2;
+		else if (temp_ofs = 2)
+			offset = 6;
+		break;
+	case TEMP3:
+		if (temp_ofs = 0)
+			offset = 5;

drivers/hwmon/emc1403.c line 108

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+		else if (temp_ofs = 1)
+			offset = 4;
+		else if (temp_ofs = 2)
+			offset = 7;
+		break;
+	default:
+		offset = -1;
+		break;
+	}
+	return offset;

This function is horrible. This is all crying for a small look-up table, isn't it?

drivers/hwmon/emc1403.c line 109

Quoted 1 line
drivers/hwmon/emc1403.c new file +1 -0
+

Stray blank line.

Quoted 6 lines
drivers/hwmon/emc1403.c new file +6 -0
+}
+
+
+static ssize_t show_temp_auto_offset(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{

drivers/hwmon/emc1403.c line 125

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+	struct sensor_device_attribute_2 *s_attr = to_sensor_dev_attr_2(attr);
+	int temp_index = s_attr->index;
+	int temp_ofs = s_attr->nr;
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret_val;
+	int ret_offset;
+
+	ret_offset = calculate_offset(temp_index, temp_ofs);
+	if (ret_offset = -1)
+		return -EINVAL;

Wouldn't this be an internal driver error? -EINVAL doesn't seem right.

drivers/hwmon/emc1403.c line 127

Quoted 2 lines
drivers/hwmon/emc1403.c new file +2 -0
+	ret_val = i2c_smbus_read_byte_data(client,
+				THM_LIMIT_REG_TEMP[ret_offset]);

Missing error handling. Same is true throughout the whole driver.

Quoted 32 lines
drivers/hwmon/emc1403.c new file +32 -0
+	return sprintf(buf, "%d\n", ret_val);
+}
+
+static ssize_t store_temp_auto_offset(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct sensor_device_attribute_2 *s_attr = to_sensor_dev_attr_2(attr);
+	int temp_index = s_attr->index;
+	int temp_ofs = s_attr->nr;
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned long val;
+	int ret_offset;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+	ret_offset = calculate_offset(temp_index, temp_ofs);
+	if (ret_offset = -1)
+		return -EINVAL;
+
+	i2c_smbus_write_byte_data(client, THM_LIMIT_REG_TEMP[ret_offset], val);
+	return count;
+}
+
+static ssize_t show_temp_hyst(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	return sprintf(buf, "%d\n",
+		i2c_smbus_read_byte_data(client, THM_LIMIT_REG_TEMP[9]));
+}
+
+static ssize_t store_temp_hyst(struct device *dev,

drivers/hwmon/emc1403.c line 169

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+	i2c_smbus_write_byte_data(client, THM_LIMIT_REG_TEMP[9], val);
+	return count;
+}

That's not OK. The value in register 0x21 is a relative one, that applies to all limits. The standard hwmon interface (see Documentation/hwmon/sysfs-interface) says that hysteresis temperatures must always be exported as absolute values. This means you must do some computations in the driver, and you must export one hyst file for every affected temperature limit.

Quoted 20 lines
drivers/hwmon/emc1403.c new file +20 -0
+
+static ssize_t show_temp1_curr_temp(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret_val;
+
+	ret_val = i2c_smbus_read_byte_data(client, THM_REG_CURR_TEMP[0]);
+	return sprintf(buf, "%d\n", ret_val);
+}
+
+static ssize_t show_temp2_curr_temp(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret_val;
+
+	ret_val = i2c_smbus_read_byte_data(client, THM_REG_CURR_TEMP[1]);
+	return sprintf(buf, "%d\n", ret_val);
+}

drivers/hwmon/emc1403.c line 199

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+
+static ssize_t show_temp3_curr_temp(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret_val;
+
+	ret_val = i2c_smbus_read_byte_data(client, THM_REG_CURR_TEMP[2]);
+	return sprintf(buf, "%d\n", ret_val);
+}

3 times the same function. Please use attribute parameters.

Quoted 4 lines
drivers/hwmon/emc1403.c new file +4 -0
+
+static ssize_t show_status_reg(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{

drivers/hwmon/emc1403.c line 213

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+	struct i2c_client *client = to_i2c_client(dev);
+	int r1, r2, r3, r4;
+
+	r1 = i2c_smbus_read_byte_data(client, 0x1F);
+	r2 = i2c_smbus_read_byte_data(client, 0x35);
+	r3 = i2c_smbus_read_byte_data(client, 0x36);
+	r4 = i2c_smbus_read_byte_data(client, 0x37);
+	return sprintf(buf, "alarm=%x,High=%x,Low=%x,Therm=%x \n",
+				r1, r2, r3, r4);
+}

Please read Documentation/hwmon/sysfs-interface. We have standard attribute names for alarm states, please use them.

drivers/hwmon/emc1403.c line 221

Quoted 8 lines
drivers/hwmon/emc1403.c new file +8 -0
+
+static ssize_t show_power_state(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret_val = i2c_smbus_read_byte_data(client, 0x03);
+	return sprintf(buf, "%x", (ret_val >> 6) & 1);
+}

Does this follow any standard? I wouldn't use hexadecimal here (even if it makes no practical difference for a boolean.)

Quoted 18 lines
drivers/hwmon/emc1403.c new file +18 -0
+
+static ssize_t store_power_state(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned long val;
+	char curr_val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	curr_val = i2c_smbus_read_byte_data(client, 0x03);
+	if (val = 1)
+		curr_val &= 0xBF;
+	else if (val = 0)
+		curr_val |= 0x40;
+	else
+		return -EINVAL;

drivers/hwmon/emc1403.c line 249

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+	i2c_smbus_write_byte_data(client, 0x03, curr_val);
+	return count;
+}
+
+static ssize_t show_mode(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret_val = i2c_smbus_read_byte_data(client, 0x03);
+	return sprintf(buf, "%x", (ret_val >> 7) & 1);

Again, no hexadecimal format please.

Quoted 12 lines
drivers/hwmon/emc1403.c new file +12 -0
+}
+
+static ssize_t store_mode(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned long val;
+	int curr_val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+

drivers/hwmon/emc1403.c line 271

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+	curr_val = i2c_smbus_read_byte_data(client, 0x03);
+	if (val = 1)
+		curr_val &= 0x7F;
+	else if (val = 0)
+		curr_val |= 0x80;
+	else
+		return -EINVAL;
+	i2c_smbus_write_byte_data(client, 0x03, curr_val);
+	return count;
+}

I am curious what this "mode" is all about. It's not a standard hwmon attribute, and not documented in the driver.

Quoted 14 lines
drivers/hwmon/emc1403.c new file +14 -0
+
+static SENSOR_DEVICE_ATTR_2(temp1_min, S_IRUGO | S_IWUSR,
+	show_temp_auto_offset, store_temp_auto_offset, 0, 1);
+static SENSOR_DEVICE_ATTR_2(temp1_max, S_IRUGO | S_IWUSR,
+	show_temp_auto_offset, store_temp_auto_offset, 1, 1);
+static SENSOR_DEVICE_ATTR_2(temp1_crit, S_IRUGO | S_IWUSR,
+	show_temp_auto_offset, store_temp_auto_offset, 2, 1);
+static DEVICE_ATTR(temp1_curr, S_IRUGO, show_temp1_curr_temp, NULL);
+
+static SENSOR_DEVICE_ATTR_2(temp2_min, S_IRUGO | S_IWUSR,
+	show_temp_auto_offset, store_temp_auto_offset, 0, 2);
+static SENSOR_DEVICE_ATTR_2(temp2_max, S_IRUGO | S_IWUSR,
+	show_temp_auto_offset, store_temp_auto_offset, 1, 2);
+static SENSOR_DEVICE_ATTR_2(temp2_crit, S_IRUGO | S_IWUSR,

drivers/hwmon/emc1403.c line 295

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+	show_temp_auto_offset, store_temp_auto_offset, 2, 2);
+static DEVICE_ATTR(temp2_curr, S_IRUGO, show_temp2_curr_temp, NULL);
+
+static SENSOR_DEVICE_ATTR_2(temp3_min, S_IRUGO | S_IWUSR,
+	show_temp_auto_offset, store_temp_auto_offset, 0, 4);
+static SENSOR_DEVICE_ATTR_2(temp3_max, S_IRUGO | S_IWUSR,
+	show_temp_auto_offset, store_temp_auto_offset, 1, 4);
+static SENSOR_DEVICE_ATTR_2(temp3_crit, S_IRUGO | S_IWUSR,
+	show_temp_auto_offset, store_temp_auto_offset, 2, 4);
+static DEVICE_ATTR(temp3_curr, S_IRUGO, show_temp3_curr_temp, NULL);

I don't understand why you use 2-parameters attributes, given that, in callback functions, you end up calling calculate_offset(), which returns a single index into an array. You could pass that index directly as the attribute parameter, or even pass the register value directly, to save the look-up in THM_LIMIT_REG_TEMP.

drivers/hwmon/emc1403.c line 297

Quoted 2 lines
drivers/hwmon/emc1403.c new file +2 -0
+
+static DEVICE_ATTR(hyster, S_IRUGO | S_IWUSR, show_temp_hyst, store_temp_hyst);

As discussed above this attribute must be reworked.

drivers/hwmon/emc1403.c line 298

Quoted 1 line
drivers/hwmon/emc1403.c new file +1 -0
+static DEVICE_ATTR(status, S_IRUGO, show_status_reg, NULL);

And this one as well.

Quoted 16 lines
drivers/hwmon/emc1403.c new file +16 -0
+
+static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR,
+	show_power_state, store_power_state);
+static DEVICE_ATTR(mode, S_IRUGO | S_IWUSR, show_mode, store_mode);
+
+static struct attribute *mid_att_thermal[] = {
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&dev_attr_temp1_curr.attr,
+	&sensor_dev_attr_temp2_min.dev_attr.attr,
+	&sensor_dev_attr_temp2_max.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit.dev_attr.attr,
+	&dev_attr_temp2_curr.attr,
+	&sensor_dev_attr_temp3_min.dev_attr.attr,
+	&sensor_dev_attr_temp3_max.dev_attr.attr,

drivers/hwmon/emc1403.c line 324

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+	&sensor_dev_attr_temp3_crit.dev_attr.attr,
+	&dev_attr_temp3_curr.attr,
+	&dev_attr_hyster.attr,
+	&dev_attr_status.attr,
+	&dev_attr_power_state.attr,
+	&dev_attr_mode.attr,
+	NULL
+};
+
+static struct attribute_group m_thermal_gr = {

Should be const.

drivers/hwmon/emc1403.c line 325

Quoted 1 line
drivers/hwmon/emc1403.c new file +1 -0
+	.name = "emc1403",

This will put the attributes in a subdirectory, where libsensors will never look for them. You don't want to do this.

drivers/hwmon/emc1403.c line 334

Quoted 9 lines
drivers/hwmon/emc1403.c new file +9 -0
+	.attrs = mid_att_thermal
+};
+
+static void emc1403_set_default_config(struct i2c_client *client)
+{
+	i2c_smbus_write_byte_data(client, 0x03, 0x00);
+	i2c_smbus_write_byte_data(client, 0x04, 0x02);
+	i2c_smbus_write_byte_data(client, 0x22, 0x00);
+}

This is horrible. Each initialization step should be documented and justified. In many cases, the BIOS has already setup the thermal sensor chip before the OS runs, and configuration should not be changed arbitrarily.

drivers/hwmon/emc1403.c line 336

Quoted 2 lines
drivers/hwmon/emc1403.c new file +2 -0
+
+static int  emc1403_probe(struct i2c_client *new_client,

Please rename "new_client" to just "client". This is a remnant of old lm-sensors drivers I'd like to finally get rid of.

drivers/hwmon/emc1403.c line 341

Quoted 5 lines
drivers/hwmon/emc1403.c new file +5 -0
+			const struct i2c_device_id *id)
+{
+	int res = 0;
+	struct thermal_data *data;
+	u16 pid, smsc_id, revision;

u16 to store 8-bit register values? Please use either u8, or int so that you can deal with I/O errors.

drivers/hwmon/emc1403.c

Quoted 4 lines
drivers/hwmon/emc1403.c new file +4 -0
+
+	data = kzalloc(sizeof(struct thermal_data), GFP_KERNEL);
+
+	if (data = NULL) {

Style: no blank line between allocation and test.

drivers/hwmon/emc1403.c line 346

Quoted 1 line
drivers/hwmon/emc1403.c new file +1 -0
+		printk(KERN_WARNING "emc1403: out of memory");

Please use dev_warn (or even dev_err... as this is a fatal error.)

drivers/hwmon/emc1403.c line 349

Quoted 3 lines
drivers/hwmon/emc1403.c new file +3 -0
+		return -ENOMEM;
+	}
+	data->client = new_client;

Set but never used.

Quoted 11 lines
drivers/hwmon/emc1403.c new file +11 -0
+	i2c_set_clientdata(new_client, data);
+
+	/* Check if thermal chip is SMSC and EMC1403 */
+	smsc_id = i2c_smbus_read_byte_data(new_client,
+					THERMAL_SMSC_ID_REG);
+	if (smsc_id != 0x5d) {
+		printk(KERN_WARNING "emc1403: vendor id mismatch\n");
+		goto thermal_error1;
+	}
+	pid = i2c_smbus_read_byte_data(new_client, THERMAL_PID_REG);
+	if (pid != 0x21)  {

drivers/hwmon/emc1403.c line 370

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+		printk(KERN_WARNING "emc1403: product id mismatch\n");
+		goto thermal_error1;
+	}
+	revision = i2c_smbus_read_byte_data(new_client,
+				THERMAL_REVISION_REG);
+	if (revision != 0x01) {
+		printk(KERN_WARNING "emc1403: rev id mismatch (is %d)\n",
+								revision);
+		goto thermal_error1;
+	}

This all should be moved to a .detect() driver function. Doing these tests in .probe() shouldn't be needed (if you're there, you already know that the right chip is present at the given address) and prevents one from arbitrarily binding the driver to a known-compatible device.

drivers/hwmon/emc1403.c line 373

Quoted 3 lines
drivers/hwmon/emc1403.c new file +3 -0
+	res = sysfs_create_group(&new_client->dev.kobj, &m_thermal_gr);
+	if (res) {
+		printk(KERN_WARNING "emc1403: create group failed\n");

dev_err()

drivers/hwmon/emc1403.c line 381

Quoted 8 lines
drivers/hwmon/emc1403.c new file +8 -0
+		hwmon_device_unregister(data->hwmon_dev);
+		goto thermal_error1;
+	}
+	data->hwmon_dev = hwmon_device_register(&new_client->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		res = PTR_ERR(data->hwmon_dev);
+		data->hwmon_dev = NULL;
+		printk(KERN_WARNING "emc1403:Register hwmon dev failed\n");

dev_err()

drivers/hwmon/emc1403.c line 387

Quoted 6 lines
drivers/hwmon/emc1403.c new file +6 -0
+		goto thermal_error2;
+	}
+	emc1403_set_default_config(new_client);
+	dev_info(&new_client->dev, "%s EMC1403 Thermal chip found\n",
+							new_client->name);
+	return res;

Please add a blank line here.

drivers/hwmon/emc1403.c line 391

Quoted 4 lines
drivers/hwmon/emc1403.c new file +4 -0
+thermal_error2:
+	sysfs_remove_group(&new_client->dev.kobj, &m_thermal_gr);
+thermal_error1:
+	i2c_set_clientdata(new_client, NULL);

No longer needed (done by i2c-core).

Quoted 5 lines
drivers/hwmon/emc1403.c new file +5 -0
+	kfree(data);
+	return res;
+}
+
+static int emc1403_remove(struct i2c_client *client)

drivers/hwmon/emc1403.c line 406

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+{
+	struct thermal_data *data = i2c_get_clientdata(client);
+
+	hwmon_device_unregister(data->hwmon_dev);
+	sysfs_remove_group(&client->dev.kobj, &m_thermal_gr);
+	kfree(data);
+	return 0;
+}
+
+static struct i2c_device_id emc1403_idtable[] = {

const

drivers/hwmon/emc1403.c line 407

Quoted 1 line
drivers/hwmon/emc1403.c new file +1 -0
+	{ "i2c_thermal", 0 },

AH AH AH. You weren't serious, right?

Quoted 21 lines
drivers/hwmon/emc1403.c new file +20 -0
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, emc1403_idtable);
+
+static struct i2c_driver sensor_emc1403 = {
+	.driver = {
+		.name = "emc1403",
+	},
+	.probe = emc1403_probe,
+	.remove = emc1403_remove,
+	.id_table = emc1403_idtable,
+};
+
+static int __init sensor_emc1403_init(void)
+{
+	return i2c_add_driver(&sensor_emc1403);
+}
+
+static void  __exit sensor_emc1403_exit(void)

drivers/hwmon/emc1403.c line 437

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+{
+	i2c_del_driver(&sensor_emc1403);
+}
+
+module_init(sensor_emc1403_init);
+module_exit(sensor_emc1403_exit);
+
+MODULE_AUTHOR("Kalhan Trisal <[email protected]");
+MODULE_DESCRIPTION("emc1403 Thermal Driver");
+MODULE_LICENSE("GPL v2");

Can I get a dump of one of these chips (use i2cdump from the i2c-tools package)? This would help me test the next iteration of the driver, and add support to sensors-detect.

Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
author
#31 lore raw

Re: [lm-sensors] [PATCH 4/4] emc1403: thermal sensor support

In reply to Alan Cox
Quoted 2 lines

Quick review follows. There's quite some cleanup work left before this driver can make it upstream.

It was trimmed from a bigger and not too pretty in-house driver so that doesn't surprise me (its also why stuff was left that wasn't used)

Rev2 below the comments

Quoted 6 lines

That's not OK. The value in register 0x21 is a relative one, that applies to all limits. The standard hwmon interface (see Documentation/hwmon/sysfs-interface) says that hysteresis temperatures must always be exported as absolute values. This means you must do some computations in the driver, and you must export one hyst file for every affected temperature limit.

I think I now have this as you intend

Quoted 10 lines

Review

Quoted 7 lines
(quoted diff) +7 -0
+static ssize_t show_power_state(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret_val = i2c_smbus_read_byte_data(client, 0x03);
+	return sprintf(buf, "%x", (ret_val >> 6) & 1);
+}

Does this follow any standard? I wouldn't use hexadecimal here (even if it makes no practical difference for a boolean.)

It turns the device on or off. I can't find an obvious general hwmon path for this ?

Quoted 2 lines

I am curious what this "mode" is all about. It's not a standard hwmon attribute, and not documented in the driver.

It flips it between just monitoring, and interrupting. It's not that relevant to the basic driver but it becomes terribly important to any follow on stuff where the chip is being used for thermal management of an x86 SoC platform (non PC)

Quoted 1 line

I don't understand why you use 2-parameters attributes,

Nor me ;)

Quoted 11 lines

Review

Quoted 6 lines
(quoted diff) +6 -0
+static void emc1403_set_default_config(struct i2c_client *client)
+{
+	i2c_smbus_write_byte_data(client, 0x03, 0x00);
+	i2c_smbus_write_byte_data(client, 0x04, 0x02);
+	i2c_smbus_write_byte_data(client, 0x22, 0x00);
+}

This is horrible. Each initialization step should be documented and justified. In many cases, the BIOS has already setup the thermal sensor chip before the OS runs, and configuration should not be changed arbitrarily.

Removed - this can go with the platform stuff (basically it ultimately needs to be part of a pair of drivers - an hwmon interface in user space and optionally a platform specific driver for some SoC systems, but I need to work out how to keep the separation clean before I submit any further bits in that area.

Quoted 3 lines

Can I get a dump of one of these chips (use i2cdump from the i2c-tools package)? This would help me test the next iteration of the driver, and add support to sensors-detect.

I will see what I can do.

Alan

--

commit db2988e43ad4fa9ddb155d770bf00dfc3ca1010d

Author: Kalhan Trisal <kalhan.trisal at intel.com>
Date:   Thu Apr 22 17:18:55 2010 +0100

emc1403: thermal sensor support

Provides support for the emc1403 thermal sensor. Only reporting of values is supported. The various Moorestown specific extras to do with thermal alerts and the like are not in this version of the driver.

Signed-off-by Kalhan Trisal <kalhan.trisal at intel.com>
drivers/hwmon/Kconfig +20 -0
index d38447f..900784d 100644
@@ -750,6 +750,16 @@ config SENSORS_DME1737
 	  This driver can also be built as a module.  If so, the module
 	  will be called dme1737.
 
+config SENSORS_EMC1403
+	tristate "SMSC EMC1403 thermal sensor"
+	depends on I2C
+	help
+	  If you say yes here you get support for the SMSC EMC1403
+	  temperature monitoring chip.
+
+	  Threshold values can be configured using sysfs.
+	  Data from the different diodes are accessible via sysfs.
+
 config SENSORS_SMSC47M1
 	tristate "SMSC LPC47M10x and compatibles"
 	help
@@ -1088,6 +1098,16 @@ config SENSORS_MC13783_ADC
         help
           Support for the A/D converter on MC13783 PMIC.
 
+config SENSORS_EMC1403
+	tristate "SMSC EMC1403 thermal sensor"
+	depends on I2C
+	help
+	  If you say yes here you get support for the SMSC Devices
+	  EMC1403 temperature monitoring chip.
+
+	  Threshold values can be configured using sysfs.
+	  Data from the different diode are accessible via sysfs.
+
 if ACPI
 
 comment "ACPI drivers"
drivers/hwmon/Makefile +1 -0
index 4aa1a3d..1e6b083 100644
@@ -40,6 +40,7 @@ obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
 obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
 obj-$(CONFIG_SENSORS_DME1737)	+= dme1737.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
+obj-$(CONFIG_SENSORS_EMC1403)	+= emc1403.o
 obj-$(CONFIG_SENSORS_F71805F)	+= f71805f.o
 obj-$(CONFIG_SENSORS_F71882FG)	+= f71882fg.o
 obj-$(CONFIG_SENSORS_F75375S)	+= f75375s.o
drivers/hwmon/emc1403.c new file +393 -0

393 lines not on this page · raw message

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#32 lore raw

Re: [lm-sensors] [PATCH 4/4] emc1403: thermal sensor support

In reply to Alan Cox

Hi Alan,

Quoted 8 lines · Alan Cox
Quoted 2 lines

Quick review follows. There's quite some cleanup work left before this driver can make it upstream.

It was trimmed from a bigger and not too pretty in-house driver so that doesn't surprise me (its also why stuff was left that wasn't used)

Rev2 below the comments

Quoted 8 lines
Quoted 6 lines

That's not OK. The value in register 0x21 is a relative one, that applies to all limits. The standard hwmon interface (see Documentation/hwmon/sysfs-interface) says that hysteresis temperatures must always be exported as absolute values. This means you must do some computations in the driver, and you must export one hyst file for every affected temperature limit.

I think I now have this as you intend

Reading, yes. Setting, no, see below.

Quoted 13 lines
Quoted 12 lines

drivers/hwmon/emc1403.c line 221

Quoted 9 lines
drivers/hwmon/emc1403.c new file +7 -0
+static ssize_t show_power_state(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret_val = i2c_smbus_read_byte_data(client, 0x03);
+	return sprintf(buf, "%x", (ret_val >> 6) & 1);
+}

Does this follow any standard? I wouldn't use hexadecimal here (even if it makes no practical difference for a boolean.)

Quoted 2 lines

It turns the device on or off. I can't find an obvious general hwmon path for this ?

No, we don't have any, this doesn't belong to the hwmon framework, this should be common to all devices and handled by the power subsystem.

Quoted 7 lines
Quoted 2 lines

I am curious what this "mode" is all about. It's not a standard hwmon attribute, and not documented in the driver.

It flips it between just monitoring, and interrupting. It's not that relevant to the basic driver but it becomes terribly important to any follow on stuff where the chip is being used for thermal management of an x86 SoC platform (non PC)

Does it really have to be a user-space controllable setting in sysfs?

Quoted 38 lines
Quoted 2 lines

I don't understand why you use 2-parameters attributes,

Nor me ;)

Quoted 11 lines

drivers/hwmon/emc1403.c line 334

Quoted 6 lines
drivers/hwmon/emc1403.c new file +6 -0
+static void emc1403_set_default_config(struct i2c_client *client)
+{
+	i2c_smbus_write_byte_data(client, 0x03, 0x00);
+	i2c_smbus_write_byte_data(client, 0x04, 0x02);
+	i2c_smbus_write_byte_data(client, 0x22, 0x00);
+}

This is horrible. Each initialization step should be documented and justified. In many cases, the BIOS has already setup the thermal sensor chip before the OS runs, and configuration should not be changed arbitrarily.

Removed - this can go with the platform stuff (basically it ultimately needs to be part of a pair of drivers - an hwmon interface in user space and optionally a platform specific driver for some SoC systems, but I need to work out how to keep the separation clean before I submit any further bits in that area.

Quoted 3 lines

Can I get a dump of one of these chips (use i2cdump from the i2c-tools package)? This would help me test the next iteration of the driver, and add support to sensors-detect.

I will see what I can do.

Alan

--

commit db2988e43ad4fa9ddb155d770bf00dfc3ca1010d

Author: Kalhan Trisal <kalhan.trisal at intel.com>
Date:   Thu Apr 22 17:18:55 2010 +0100
Quoted 8 lines

emc1403: thermal sensor support

Provides support for the emc1403 thermal sensor. Only reporting of values is supported. The various Moorestown specific extras to do with thermal alerts and the like are not in this version of the driver.

Signed-off-by Kalhan Trisal <kalhan.trisal at intel.com>

Builds with a warning:

drivers/hwmon/emc1403.c: In function ‘emc1403_probe’:
drivers/hwmon/emc1403.c:342: warning: label ‘ioerr’ defined but not used

Also triggers a checkpatch error:

ERROR: trailing whitespace
#323: FILE: drivers/hwmon/emc1403.c:142:
+^I$

Quoted 28 lines
drivers/hwmon/Kconfig +13 -0
index d38447f..900784d 100644
@@ -750,6 +750,16 @@ config SENSORS_DME1737
 	  This driver can also be built as a module.  If so, the module
 	  will be called dme1737.
 
+config SENSORS_EMC1403
+	tristate "SMSC EMC1403 thermal sensor"
+	depends on I2C
+	help
+	  If you say yes here you get support for the SMSC EMC1403
+	  temperature monitoring chip.
+
+	  Threshold values can be configured using sysfs.
+	  Data from the different diodes are accessible via sysfs.
+
 config SENSORS_SMSC47M1
 	tristate "SMSC LPC47M10x and compatibles"
 	help
@@ -1088,6 +1098,16 @@ config SENSORS_MC13783_ADC
         help
           Support for the A/D converter on MC13783 PMIC.
 
+config SENSORS_EMC1403
+	tristate "SMSC EMC1403 thermal sensor"
+	depends on I2C

drivers/hwmon/Kconfig config SENSORS_MC13783_ADC · line 1113

Quoted 10 lines
drivers/hwmon/Kconfig +7 -0
+	help
+	  If you say yes here you get support for the SMSC Devices
+	  EMC1403 temperature monitoring chip.
+
+	  Threshold values can be configured using sysfs.
+	  Data from the different diode are accessible via sysfs.
+
 if ACPI
 
 comment "ACPI drivers"

One entry is enough ;)

Quoted 41 lines
drivers/hwmon/Makefile +1 -0
index 4aa1a3d..1e6b083 100644
@@ -40,6 +40,7 @@ obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
 obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
 obj-$(CONFIG_SENSORS_DME1737)	+= dme1737.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
+obj-$(CONFIG_SENSORS_EMC1403)	+= emc1403.o
 obj-$(CONFIG_SENSORS_F71805F)	+= f71805f.o
 obj-$(CONFIG_SENSORS_F71882FG)	+= f71882fg.o
 obj-$(CONFIG_SENSORS_F75375S)	+= f75375s.o
drivers/hwmon/emc1403.c new file +23 -0
new file mode 100644
index 0000000..a7112df
@@ -0,0 +1,393 @@
+/*
+ * emc1403.c - SMSC Thermal Driver
+ *
+ * Copyright (C) 2008 Intel Corp
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * 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, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#include <linux/module.h>

drivers/hwmon/emc1403.c line 33

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/sysfs.h>
+#include <linux/mutex.h>
+
+/* Therm Limit reg store values */

This comment seems irrelevant now.

drivers/hwmon/emc1403.c line 67

Quoted 6 lines
drivers/hwmon/emc1403.c new file +6 -0
+#define THERMAL_PID_REG		0xfd
+#define THERMAL_SMSC_ID_REG	0xfe
+#define THERMAL_REVISION_REG	0xff
+
+struct thermal_data {
+	struct i2c_client *client;

Never used?

drivers/hwmon/emc1403.c

Quoted 5 lines
drivers/hwmon/emc1403.c new file +5 -0
+	struct device *hwmon_dev;
+};
+
+/* A mutex per device is overkill here, one will do nicely */
+static DEFINE_MUTEX(reg_mutex);

All other drivers use one mutex per device. I don't quite see how overkill this is, and it's difficult to justify blocking access to one I2C device because another one is in use. They may be on different buses.

drivers/hwmon/emc1403.c line 55

Quoted 11 lines
drivers/hwmon/emc1403.c new file +11 -0
+
+static ssize_t show_temp(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
+	int retval = i2c_smbus_read_byte_data(client, sda->index);
+
+	if (retval < 0)
+		return retval;
+	return sprintf(buf, "%d0000\n", retval);

I think you want 3 zeroes, not 4. Other drivers multiply the value by 1000 before printing it, it might be slightly less efficient, but also less error-prone.

Quoted 16 lines
drivers/hwmon/emc1403.c new file +16 -0
+}
+
+static ssize_t show_bit(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sensor_device_attribute_2 *sda = to_sensor_dev_attr_2(attr);
+	int retval = i2c_smbus_read_byte_data(client, sda->nr);
+
+	if (retval < 0)
+		return retval;
+	retval &= sda->index;
+	return sprintf(buf, "%d\n", retval ? 1 : 0);
+}
+
+static ssize_t store_temp(struct device *dev,

drivers/hwmon/emc1403.c

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned long val;
+	int retval;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+	retval = i2c_smbus_write_byte_data(client, sda->index, val / 1000);

Proper rounding would be welcome here (use DIV_ROUND_CLOSEST).

Quoted 12 lines
drivers/hwmon/emc1403.c new file +12 -0
+	if (retval < 0)
+		return retval;
+	return count;
+}
+
+static ssize_t show_hyst(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
+	int retval;
+	int hyst;

drivers/hwmon/emc1403.c line 103

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+
+	retval = i2c_smbus_read_byte_data(client, sda->index);
+	if (retval < 0)
+		return retval;
+
+	hyst = i2c_smbus_read_byte_data(client, 0x21);
+	if (hyst < 0)
+		return retval;
+
+	return sprintf(buf, "%d0000\n", retval - hyst);

Bug again.

drivers/hwmon/emc1403.c line 104

Quoted 1 line
drivers/hwmon/emc1403.c new file +1 -0
+}

No store_hyst()? Note that the hysteresis being a shared register, the traditional way to handle it is to make the sysfs attribute writable only for one of the temperature channels, and read-only for the remainder.

drivers/hwmon/emc1403.c line 221

Quoted 9 lines
drivers/hwmon/emc1403.c new file +9 -0
+
+/* Control whether the device is powere don or off */
+static ssize_t show_power_state(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int retval = i2c_smbus_read_byte_data(client, 0x03);
+	return sprintf(buf, "%d", (retval >> 6) & 1);
+}

Why don't you use show_bit()?

Quoted 32 lines
drivers/hwmon/emc1403.c new file +32 -0
+
+static ssize_t store_power_state(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned long val;
+	u8 curr_val;
+	int retval;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	mutex_lock(&reg_mutex);
+	curr_val = i2c_smbus_read_byte_data(client, 0x03);
+	if (val = 1)
+		curr_val &= 0xBF;
+	else if (val = 0)
+		curr_val |= 0x40;
+	else {
+		mutex_unlock(&reg_mutex);
+		return -EINVAL;
+	}
+
+	retval = i2c_smbus_write_byte_data(client, 0x03, curr_val);
+
+	mutex_unlock(&reg_mutex);
+	if (retval < 0)
+		return retval;
+	
+	return count;
+}
+

drivers/hwmon/emc1403.c line 334

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+/* Report whether the device is reporting alarms by interrupt */
+static ssize_t show_mode(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int retval = i2c_smbus_read_byte_data(client, 0x03);
+	if (retval < 0)
+		return retval;
+	return sprintf(buf, "%d", (retval >> 7) & 1);
+}

Why don't you use show_bit()?

Quoted 19 lines
drivers/hwmon/emc1403.c new file +19 -0
+
+static ssize_t store_mode(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned long val;
+	int curr_val;
+	int retval;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	mutex_lock(&reg_mutex);
+	curr_val = i2c_smbus_read_byte_data(client, 0x03);
+	if (val = 1)
+		curr_val &= 0x7F;
+	else if (val = 0)
+		curr_val |= 0x80;
+	else {

drivers/hwmon/emc1403.c line 334

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+		mutex_unlock(&reg_mutex);
+		return -EINVAL;
+	}
+
+	retval = i2c_smbus_write_byte_data(client, 0x03, curr_val);
+	mutex_unlock(&reg_mutex);
+	if (retval < 0)
+		return retval;
+	return count;
+}

Might be worth defining a store_bit() function and using it instead of store_power_state() and store_mode()?

drivers/hwmon/emc1403.c line 196

Quoted 12 lines
drivers/hwmon/emc1403.c new file +12 -0
+
+/*
+ *	Sensors. We pass the actual i2c register to the methods.
+ */
+
+static SENSOR_DEVICE_ATTR(temp1_min, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x06);
+static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x05);
+static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x20);
+static SENSOR_DEVICE_ATTR(temp1_curr, S_IRUGO, show_temp, NULL, 0x00);

The standard name is temp1_input. Please see Documentation/hwmon/sysfs-interface. It is also a good idea to test your driver with libsensors, that's the easiest way to ensure that you got the names and units right.

drivers/hwmon/emc1403.c line 204

Quoted 8 lines
drivers/hwmon/emc1403.c new file +8 -0
+static SENSOR_DEVICE_ATTR_2(temp1_min_alarm, S_IRUGO,
+	show_bit, NULL, 0x36, 0x01);
+static SENSOR_DEVICE_ATTR_2(temp1_max_alarm, S_IRUGO,
+	show_bit, NULL, 0x35, 0x01);
+static SENSOR_DEVICE_ATTR_2(temp1_crit_alarm, S_IRUGO,
+	show_bit, NULL, 0x37, 0x01);
+static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IRUGO | S_IWUSR,
+	show_hyst, store_temp, 0x20);

store_temp isn't right. That would set the hysteresis as a relative value while you're returning is absolute (which is right). You want a separate store_hyst() function working the same as show_hyst() does.

Quoted 22 lines
drivers/hwmon/emc1403.c new file +22 -0
+
+static SENSOR_DEVICE_ATTR(temp2_min, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x08);
+static SENSOR_DEVICE_ATTR(temp2_max, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x07);
+static SENSOR_DEVICE_ATTR(temp2_crit, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x19);
+static SENSOR_DEVICE_ATTR(temp2_curr, S_IRUGO, show_temp, NULL, 0x01);
+static SENSOR_DEVICE_ATTR_2(temp2_min_alarm, S_IRUGO,
+	show_bit, NULL, 0x36, 0x02);
+static SENSOR_DEVICE_ATTR_2(temp2_max_alarm, S_IRUGO,
+	show_bit, NULL, 0x35, 0x02);
+static SENSOR_DEVICE_ATTR_2(temp2_crit_alarm, S_IRUGO,
+	show_bit, NULL, 0x37, 0x02);
+static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO | S_IWUSR,
+	show_hyst, store_temp, 0x19);
+
+static SENSOR_DEVICE_ATTR(temp3_min, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x16);
+static SENSOR_DEVICE_ATTR(temp3_max, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x15);
+static SENSOR_DEVICE_ATTR(temp3_crit, S_IRUGO | S_IWUSR,

drivers/hwmon/emc1403.c line 236

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+	show_temp, store_temp, 0x1A);
+static SENSOR_DEVICE_ATTR(temp3_curr, S_IRUGO, show_temp, NULL, 0x23);
+static SENSOR_DEVICE_ATTR_2(temp3_min_alarm, S_IRUGO,
+	show_bit, NULL, 0x36, 0x04);
+static SENSOR_DEVICE_ATTR_2(temp3_max_alarm, S_IRUGO,
+	show_bit, NULL, 0x35, 0x04);
+static SENSOR_DEVICE_ATTR_2(temp3_crit_alarm, S_IRUGO,
+	show_bit, NULL, 0x37, 0x04);
+static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO | S_IWUSR,
+	show_hyst, store_temp, 0x1A);

Note that, when reading all sysfs attribute in a row (as "sensors" would do), you end up reading several registers up to 3 times. As I2C access is often slow, performance might suffer. You might want to use the same strategy used by many other hwmon drivers for I2C devices, which are caching the register values for a second.

Quoted 41 lines
drivers/hwmon/emc1403.c new file +41 -0
+
+static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR,
+	show_power_state, store_power_state);
+static DEVICE_ATTR(alarm_interrupt, S_IRUGO | S_IWUSR, show_mode, store_mode);
+
+static struct attribute *mid_att_thermal[] = {
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_curr.dev_attr.attr,
+	&sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
+	&sensor_dev_attr_temp2_min.dev_attr.attr,
+	&sensor_dev_attr_temp2_max.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit.dev_attr.attr,
+	&sensor_dev_attr_temp2_curr.dev_attr.attr,
+	&sensor_dev_attr_temp2_min_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
+	&sensor_dev_attr_temp3_min.dev_attr.attr,
+	&sensor_dev_attr_temp3_max.dev_attr.attr,
+	&sensor_dev_attr_temp3_crit.dev_attr.attr,
+	&sensor_dev_attr_temp3_curr.dev_attr.attr,
+	&sensor_dev_attr_temp3_min_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp3_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp3_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
+	&dev_attr_power_state.attr,
+	&dev_attr_alarm_interrupt.attr,
+	NULL
+};
+
+static const struct attribute_group m_thermal_gr = {
+	.attrs = mid_att_thermal
+};
+
+/* We will need this for MRST platforms but can't enable it until the
+   relevant auto-detection patch is upstream for x86 */

drivers/hwmon/emc1403.c line 287

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+
+static void emc1403_set_default_config(struct i2c_client *client)
+{
+/*
+	i2c_smbus_write_byte_data(client, 0x03, 0x00);
+	i2c_smbus_write_byte_data(client, 0x04, 0x02);
+	i2c_smbus_write_byte_data(client, 0x22, 0x00);
+*/
+}
+

You might as well wipe out the function altogether for the time being. I've seen too much code stay commented out forever, because it was finally never needed or because the same was implemented somewhere else.

Quoted 7 lines
drivers/hwmon/emc1403.c new file +7 -0
+
+static int emc1403_detect(struct i2c_client *client,
+			struct i2c_board_info *info)
+{
+	int id;
+	/* Check if thermal chip is SMSC and EMC1403 */
+	id = i2c_smbus_read_byte_data(client, THERMAL_SMSC_ID_REG);

drivers/hwmon/emc1403.c line 304

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+	if (id != 0x5d)
+		return -ENODEV;
+	id = i2c_smbus_read_byte_data(client, THERMAL_PID_REG);
+
+	/* Note: 0x25 is the 1404 which is very similar and this
+	   driver could be extended */
+	if (id != 0x21)
+		return -ENODEV;
+	id = i2c_smbus_read_byte_data(client,
+				THERMAL_REVISION_REG);

Line break not needed.

drivers/hwmon/emc1403.c line 306

Quoted 2 lines
drivers/hwmon/emc1403.c new file +2 -0
+	if (id != 0x01)
+		return -ENODEV;

Grouping register reads with value test would make the function more readable IMHO.

Before you return success, you must fill out the type field of info:

	strlcpy(info->type, "emc1403", I2C_NAME_SIZE);

drivers/hwmon/emc1403.c line 339

Quoted 7 lines
drivers/hwmon/emc1403.c new file +7 -0
+	return 0;
+}
+
+static int emc1403_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	int res = 0;

Initialization not needed.

drivers/hwmon/emc1403.c

Quoted 5 lines
drivers/hwmon/emc1403.c new file +5 -0
+	struct thermal_data *data;
+
+	data = kzalloc(sizeof(struct thermal_data), GFP_KERNEL);
+	if (data = NULL) {
+		dev_warn(&client->dev, "emc1403: out of memory");

The "emc1403:" prefix in all your log messages is redundant... dev_warn() identifies the device already.

Quoted 5 lines
drivers/hwmon/emc1403.c new file +5 -0
+		return -ENOMEM;
+	}
+
+	i2c_set_clientdata(client, data);
+

drivers/hwmon/emc1403.c line 380

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+	res = sysfs_create_group(&client->dev.kobj, &m_thermal_gr);
+	if (res) {
+		dev_warn(&client->dev, "emc1403: create group failed\n");
+		hwmon_device_unregister(data->hwmon_dev);
+		goto thermal_error1;
+	}
+	data->hwmon_dev = hwmon_device_register(&client->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		res = PTR_ERR(data->hwmon_dev);
+		data->hwmon_dev = NULL;

Not needed that I can see.

drivers/hwmon/emc1403.c line 339

Quoted 6 lines
drivers/hwmon/emc1403.c new file +6 -0
+		dev_warn(&client->dev, "emc1403: register hwmon dev failed\n");
+		goto thermal_error2;
+	}
+	emc1403_set_default_config(client);
+	dev_info(&client->dev, "%s EMC1403 Thermal chip found\n",
+							client->name);

client-name already printed by dev_info.

Quoted 56 lines
drivers/hwmon/emc1403.c new file +54 -0
+	return res;
+
+ioerr:
+	dev_warn(&client->dev,  "emc1403: smbus transaction failed.\n");
+	goto thermal_error1;
+thermal_error2:
+	sysfs_remove_group(&client->dev.kobj, &m_thermal_gr);
+thermal_error1:
+	kfree(data);
+	return res;
+}
+
+static int emc1403_remove(struct i2c_client *client)
+{
+	struct thermal_data *data = i2c_get_clientdata(client);
+
+	hwmon_device_unregister(data->hwmon_dev);
+	sysfs_remove_group(&client->dev.kobj, &m_thermal_gr);
+	kfree(data);
+	return 0;
+}
+
+const static struct i2c_device_id emc1403_idtable[] = {
+	{ "emc1403", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, emc1403_idtable);
+
+static struct i2c_driver sensor_emc1403 = {
+	.driver = {
+		.name = "emc1403",
+	},
+	.detect = emc1403_detect,
+	.probe = emc1403_probe,
+	.remove = emc1403_remove,
+	.id_table = emc1403_idtable,
+};
+
+static int __init sensor_emc1403_init(void)
+{
+	return i2c_add_driver(&sensor_emc1403);
+}
+
+static void  __exit sensor_emc1403_exit(void)
+{
+	i2c_del_driver(&sensor_emc1403);
+}
+
+module_init(sensor_emc1403_init);
+module_exit(sensor_emc1403_exit);
+
+MODULE_AUTHOR("Kalhan Trisal <[email protected]");
+MODULE_DESCRIPTION("emc1403 Thermal Driver");
+MODULE_LICENSE("GPL v2");
Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
author
#33 lore raw

Re: [lm-sensors] [PATCH 4/4] emc1403: thermal sensor support

In reply to Alan Cox
Quoted 6 lines
Quoted 2 lines

It turns the device on or off. I can't find an obvious general hwmon path for this ?

No, we don't have any, this doesn't belong to the hwmon framework, this should be common to all devices and handled by the power subsystem.

Nice in theory but in some cases we do actually want to be able to power up/down individual sensors and it's not therefore a matter of a suspend/resume method.

Quoted 4 lines
Quoted 2 lines

any follow on stuff where the chip is being used for thermal management of an x86 SoC platform (non PC)

Does it really have to be a user-space controllable setting in sysfs?

Probably it needs to be user settable, but equally that I imagine will belong in the monitor/power logic for the platform not the generic stuff.

Quoted 4 lines

All other drivers use one mutex per device. I don't quite see how overkill this is, and it's difficult to justify blocking access to one I2C device because another one is in use. They may be on different buses.

If you prefer.

Quoted 4 lines

No store_hyst()? Note that the hysteresis being a shared register, the traditional way to handle it is to make the sysfs attribute writable only for one of the temperature channels, and read-only for the remainder.

Done

Other bits done.. I'll try and get it tested but my dev board is being recalcitrant right now.

--

commit ae8a34b81f05836db754aa9feb318a443d9fd5fa

Author: Kalhan Trisal <kalhan.trisal at intel.com>
Date:   Mon Apr 26 11:08:38 2010 +0100

emc1403: thermal sensor support

Provides support for the emc1403 thermal sensor. Only reporting of values is supported. The various Moorestown specific extras to do with thermal alerts and the like are not in this version of the driver.

(Much clean up by Alan Cox)

Signed-off-by Kalhan Trisal <kalhan.trisal at intel.com>
drivers/hwmon/Kconfig +10 -0
index d38447f..0bd3206 100644
@@ -750,6 +750,16 @@ config SENSORS_DME1737
 	  This driver can also be built as a module.  If so, the module
 	  will be called dme1737.
 
+config SENSORS_EMC1403
+	tristate "SMSC EMC1403 thermal sensor"
+	depends on I2C
+	help
+	  If you say yes here you get support for the SMSC EMC1403
+	  temperature monitoring chip.
+
+	  Threshold values can be configured using sysfs.
+	  Data from the different diodes are accessible via sysfs.
+
 config SENSORS_SMSC47M1
 	tristate "SMSC LPC47M10x and compatibles"
 	help
drivers/hwmon/Makefile +1 -0
index 4aa1a3d..1e6b083 100644
@@ -40,6 +40,7 @@ obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
 obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
 obj-$(CONFIG_SENSORS_DME1737)	+= dme1737.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
+obj-$(CONFIG_SENSORS_EMC1403)	+= emc1403.o
 obj-$(CONFIG_SENSORS_F71805F)	+= f71805f.o
 obj-$(CONFIG_SENSORS_F71882FG)	+= f71882fg.o
 obj-$(CONFIG_SENSORS_F75375S)	+= f75375s.o
drivers/hwmon/emc1403.c new file +366 -0

366 lines not on this page · raw message

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#34 lore raw

Re: [lm-sensors] [PATCH 4/4] emc1403: thermal sensor support

In reply to Alan Cox

Hi Alan,

Quoted 10 lines · Alan Cox
Quoted 6 lines
Quoted 2 lines

It turns the device on or off. I can't find an obvious general hwmon path for this ?

No, we don't have any, this doesn't belong to the hwmon framework, this should be common to all devices and handled by the power subsystem.

Nice in theory but in some cases we do actually want to be able to power up/down individual sensors and it's not therefore a matter of a suspend/resume method.

I have no problem with individual power up/down of devices. But this doesn't make it a hwmon-specific requirement, so I insist that it doesn't belong to the hwmon framework.

Quoted 23 lines
Quoted 4 lines
Quoted 2 lines

any follow on stuff where the chip is being used for thermal management of an x86 SoC platform (non PC)

Does it really have to be a user-space controllable setting in sysfs?

Probably it needs to be user settable, but equally that I imagine will belong in the monitor/power logic for the platform not the generic stuff.

Quoted 4 lines

All other drivers use one mutex per device. I don't quite see how overkill this is, and it's difficult to justify blocking access to one I2C device because another one is in use. They may be on different buses.

If you prefer.

Quoted 4 lines

No store_hyst()? Note that the hysteresis being a shared register, the traditional way to handle it is to make the sysfs attribute writable only for one of the temperature channels, and read-only for the remainder.

Quoted 5 lines

Done

Other bits done.. I'll try and get it tested but my dev board is being recalcitrant right now.

Almost alright now, see my few remaining comments below.

Quoted 90 lines

--

commit ae8a34b81f05836db754aa9feb318a443d9fd5fa

Author: Kalhan Trisal <kalhan.trisal at intel.com>
Date:   Mon Apr 26 11:08:38 2010 +0100

emc1403: thermal sensor support

Provides support for the emc1403 thermal sensor. Only reporting of values is supported. The various Moorestown specific extras to do with thermal alerts and the like are not in this version of the driver.

(Much clean up by Alan Cox)

Signed-off-by Kalhan Trisal <kalhan.trisal at intel.com>
drivers/hwmon/Kconfig +10 -0
index d38447f..0bd3206 100644
@@ -750,6 +750,16 @@ config SENSORS_DME1737
 	  This driver can also be built as a module.  If so, the module
 	  will be called dme1737.
 
+config SENSORS_EMC1403
+	tristate "SMSC EMC1403 thermal sensor"
+	depends on I2C
+	help
+	  If you say yes here you get support for the SMSC EMC1403
+	  temperature monitoring chip.
+
+	  Threshold values can be configured using sysfs.
+	  Data from the different diodes are accessible via sysfs.
+
 config SENSORS_SMSC47M1
 	tristate "SMSC LPC47M10x and compatibles"
 	help
drivers/hwmon/Makefile +1 -0
index 4aa1a3d..1e6b083 100644
@@ -40,6 +40,7 @@ obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
 obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
 obj-$(CONFIG_SENSORS_DME1737)	+= dme1737.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
+obj-$(CONFIG_SENSORS_EMC1403)	+= emc1403.o
 obj-$(CONFIG_SENSORS_F71805F)	+= f71805f.o
 obj-$(CONFIG_SENSORS_F71882FG)	+= f71882fg.o
 obj-$(CONFIG_SENSORS_F75375S)	+= f75375s.o
drivers/hwmon/emc1403.c new file +33 -0
new file mode 100644
index 0000000..3e50325
@@ -0,0 +1,366 @@
+/*
+ * emc1403.c - SMSC Thermal Driver
+ *
+ * Copyright (C) 2008 Intel Corp
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * 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, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/sysfs.h>
+#include <linux/mutex.h>
+
+#define THERMAL_PID_REG		0xfd

drivers/hwmon/emc1403.c line 43

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+#define THERMAL_SMSC_ID_REG	0xfe
+#define THERMAL_REVISION_REG	0xff
+
+struct thermal_data {
+	struct device *hwmon_dev;
+	struct mutex mutex;
+	/* Cache the hyst value so we don't keep re-reading it. In theory
+	   we could cache it forever as nobody else should be writing it. */
+	u8 cached_hyst;
+	unsigned long hyst_valid;

This is good, but might need to be extended to other registers. In particular the alarm registers are read 3 times, and the critical limit registers are read twice. This can be left for later though if you don't feel like implementing it now.

Quoted 52 lines
drivers/hwmon/emc1403.c new file +52 -0
+};
+
+static ssize_t show_temp(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
+	int retval = i2c_smbus_read_byte_data(client, sda->index);
+
+	if (retval < 0)
+		return retval;
+	return sprintf(buf, "%d000\n", retval);
+}
+
+static ssize_t show_bit(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sensor_device_attribute_2 *sda = to_sensor_dev_attr_2(attr);
+	int retval = i2c_smbus_read_byte_data(client, sda->nr);
+
+	if (retval < 0)
+		return retval;
+	retval &= sda->index;
+	return sprintf(buf, "%d\n", retval ? 1 : 0);
+}
+
+static ssize_t store_temp(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned long val;
+	int retval;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+	retval = i2c_smbus_write_byte_data(client, sda->index,
+					DIV_ROUND_CLOSEST(val, 1000));
+	if (retval < 0)
+		return retval;
+	return count;
+}
+
+static ssize_t store_bit(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct thermal_data *data = i2c_get_clientdata(client);
+	struct sensor_device_attribute_2 *sda = to_sensor_dev_attr_2(attr);
+	unsigned long val;
+	int retval;

drivers/hwmon/emc1403.c

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	mutex_lock(&data->mutex);
+	retval = i2c_smbus_read_byte_data(client, sda->nr);
+	if (retval < 0)
+		goto fail;
+
+	retval &= sda->index;

Missing negation?

Quoted 202 lines
drivers/hwmon/emc1403.c new file +202 -0
+	if (val)
+		retval |= sda->index;
+
+	retval = i2c_smbus_write_byte_data(client, sda->index, retval);
+	if (retval = 0)
+		retval = count;
+fail:
+	mutex_unlock(&data->mutex);
+	return retval;
+}
+
+static ssize_t show_hyst(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct thermal_data *data = i2c_get_clientdata(client);
+	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
+	int retval;
+	int hyst;
+
+	retval = i2c_smbus_read_byte_data(client, sda->index);
+	if (retval < 0)
+		return retval;
+
+	if (time_after(jiffies, data->hyst_valid)) {
+		hyst = i2c_smbus_read_byte_data(client, 0x21);
+		if (hyst < 0)
+			return retval;
+		data->cached_hyst = hyst;
+		data->hyst_valid = jiffies + HZ;
+	}
+	return sprintf(buf, "%d000\n", retval - data->cached_hyst);
+}
+
+static ssize_t store_hyst(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct thermal_data *data = i2c_get_clientdata(client);
+	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
+	int retval;
+	int hyst;
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	mutex_lock(&data->mutex);
+	retval = i2c_smbus_read_byte_data(client, sda->index);
+	if (retval < 0)
+		goto fail;
+
+	hyst = val - retval * 1000;
+	hyst = DIV_ROUND_CLOSEST(hyst, 1000);
+	if (hyst < 0 || hyst > 255) {
+		retval = -ERANGE;
+		goto fail;
+	}
+
+	retval = i2c_smbus_write_byte_data(client, 0x21, hyst);
+	if (retval = 0) {
+		retval = count;
+		data->cached_hyst = hyst;
+		data->hyst_valid = jiffies + HZ;
+	}
+fail:
+	mutex_unlock(&data->mutex);
+	return retval;
+}
+
+/*
+ *	Sensors. We pass the actual i2c register to the methods.
+ */
+
+static SENSOR_DEVICE_ATTR(temp1_min, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x06);
+static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x05);
+static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x20);
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL, 0x00);
+static SENSOR_DEVICE_ATTR_2(temp1_min_alarm, S_IRUGO,
+	show_bit, NULL, 0x36, 0x01);
+static SENSOR_DEVICE_ATTR_2(temp1_max_alarm, S_IRUGO,
+	show_bit, NULL, 0x35, 0x01);
+static SENSOR_DEVICE_ATTR_2(temp1_crit_alarm, S_IRUGO,
+	show_bit, NULL, 0x37, 0x01);
+static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IRUGO | S_IWUSR,
+	show_hyst, store_hyst, 0x20);
+
+static SENSOR_DEVICE_ATTR(temp2_min, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x08);
+static SENSOR_DEVICE_ATTR(temp2_max, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x07);
+static SENSOR_DEVICE_ATTR(temp2_crit, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x19);
+static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp, NULL, 0x01);
+static SENSOR_DEVICE_ATTR_2(temp2_min_alarm, S_IRUGO,
+	show_bit, NULL, 0x36, 0x02);
+static SENSOR_DEVICE_ATTR_2(temp2_max_alarm, S_IRUGO,
+	show_bit, NULL, 0x35, 0x02);
+static SENSOR_DEVICE_ATTR_2(temp2_crit_alarm, S_IRUGO,
+	show_bit, NULL, 0x37, 0x02);
+static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO | S_IWUSR,
+	show_hyst, store_hyst, 0x19);
+
+static SENSOR_DEVICE_ATTR(temp3_min, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x16);
+static SENSOR_DEVICE_ATTR(temp3_max, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x15);
+static SENSOR_DEVICE_ATTR(temp3_crit, S_IRUGO | S_IWUSR,
+	show_temp, store_temp, 0x1A);
+static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_temp, NULL, 0x23);
+static SENSOR_DEVICE_ATTR_2(temp3_min_alarm, S_IRUGO,
+	show_bit, NULL, 0x36, 0x04);
+static SENSOR_DEVICE_ATTR_2(temp3_max_alarm, S_IRUGO,
+	show_bit, NULL, 0x35, 0x04);
+static SENSOR_DEVICE_ATTR_2(temp3_crit_alarm, S_IRUGO,
+	show_bit, NULL, 0x37, 0x04);
+static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO | S_IWUSR,
+	show_hyst, store_hyst, 0x1A);
+
+static SENSOR_DEVICE_ATTR_2(power_state, S_IRUGO | S_IWUSR,
+	show_bit, store_bit, 0x03, 0x40);
+
+static struct attribute *mid_att_thermal[] = {
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit.dev_attr.attr,
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
+	&sensor_dev_attr_temp2_min.dev_attr.attr,
+	&sensor_dev_attr_temp2_max.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit.dev_attr.attr,
+	&sensor_dev_attr_temp2_input.dev_attr.attr,
+	&sensor_dev_attr_temp2_min_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
+	&sensor_dev_attr_temp3_min.dev_attr.attr,
+	&sensor_dev_attr_temp3_max.dev_attr.attr,
+	&sensor_dev_attr_temp3_crit.dev_attr.attr,
+	&sensor_dev_attr_temp3_input.dev_attr.attr,
+	&sensor_dev_attr_temp3_min_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp3_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp3_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
+	&sensor_dev_attr_power_state.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group m_thermal_gr = {
+	.attrs = mid_att_thermal
+};
+
+static int emc1403_detect(struct i2c_client *client,
+			struct i2c_board_info *info)
+{
+	int id;
+	/* Check if thermal chip is SMSC and EMC1403 */
+
+	id = i2c_smbus_read_byte_data(client, THERMAL_SMSC_ID_REG);
+	if (id != 0x5d)
+		return -ENODEV;
+
+	/* Note: 0x25 is the 1404 which is very similar and this
+	   driver could be extended */
+	id = i2c_smbus_read_byte_data(client, THERMAL_PID_REG);
+	if (id != 0x21)
+		return -ENODEV;
+
+	id = i2c_smbus_read_byte_data(client, THERMAL_REVISION_REG);
+	if (id != 0x01)
+		return -ENODEV;
+
+	strlcpy(info->type, "emc1403", I2C_NAME_SIZE);
+	return 0;
+}
+
+static int emc1403_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	int res;
+	struct thermal_data *data;
+
+	data = kzalloc(sizeof(struct thermal_data), GFP_KERNEL);
+	if (data = NULL) {
+		dev_warn(&client->dev, "out of memory");
+		return -ENOMEM;
+	}
+
+	i2c_set_clientdata(client, data);
+	mutex_init(&data->mutex);
+	data->hyst_valid = jiffies - 1;		/* Expired */
+
+	res = sysfs_create_group(&client->dev.kobj, &m_thermal_gr);
+	if (res) {
+		dev_warn(&client->dev, "create group failed\n");
+		hwmon_device_unregister(data->hwmon_dev);

drivers/hwmon/emc1403.c line 317

Quoted 10 lines
drivers/hwmon/emc1403.c new file +10 -0
+		goto thermal_error1;
+	}
+	data->hwmon_dev = hwmon_device_register(&client->dev);
+	if (IS_ERR(data->hwmon_dev)) {
+		res = PTR_ERR(data->hwmon_dev);
+		dev_warn(&client->dev, "register hwmon dev failed\n");
+		goto thermal_error2;
+	}
+	dev_info(&client->dev, "EMC1403 Thermal chip found\n");
+	return res;

An extra blank line here would be appreciated.

Quoted 49 lines
drivers/hwmon/emc1403.c new file +49 -0
+thermal_error2:
+	sysfs_remove_group(&client->dev.kobj, &m_thermal_gr);
+thermal_error1:
+	kfree(data);
+	return res;
+}
+
+static int emc1403_remove(struct i2c_client *client)
+{
+	struct thermal_data *data = i2c_get_clientdata(client);
+
+	hwmon_device_unregister(data->hwmon_dev);
+	sysfs_remove_group(&client->dev.kobj, &m_thermal_gr);
+	kfree(data);
+	return 0;
+}
+
+const static struct i2c_device_id emc1403_idtable[] = {
+	{ "emc1403", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, emc1403_idtable);
+
+static struct i2c_driver sensor_emc1403 = {
+	.driver = {
+		.name = "emc1403",
+	},
+	.detect = emc1403_detect,
+	.probe = emc1403_probe,
+	.remove = emc1403_remove,
+	.id_table = emc1403_idtable,
+};
+
+static int __init sensor_emc1403_init(void)
+{
+	return i2c_add_driver(&sensor_emc1403);
+}
+
+static void  __exit sensor_emc1403_exit(void)
+{
+	i2c_del_driver(&sensor_emc1403);
+}
+
+module_init(sensor_emc1403_init);
+module_exit(sensor_emc1403_exit);
+
+MODULE_AUTHOR("Kalhan Trisal <[email protected]");
+MODULE_DESCRIPTION("emc1403 Thermal Driver");
+MODULE_LICENSE("GPL v2");
Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
#35 lore raw

Re: [lm-sensors] [PATCH 4/4] emc1403: thermal sensor support

In reply to Alan Cox

Hi Alan,

Quoted 5 lines · Alan Cox
Quoted 3 lines

Can I get a dump of one of these chips (use i2cdump from the i2c-tools package)? This would help me test the next iteration of the driver, and add support to sensors-detect.

I will see what I can do.

Any progress on that front?

Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
#36 lore raw

Re: [lm-sensors] [PATCH 4/4] emc1403: thermal sensor support

In reply to Alan Cox

Hi Alan,

Quoted 30 lines · Alan Cox
(quoted diff) +30 -0
+static int emc1403_detect(struct i2c_client *client,
+			struct i2c_board_info *info)
+{
+	int id;
+	/* Check if thermal chip is SMSC and EMC1403 */
+
+	id = i2c_smbus_read_byte_data(client, THERMAL_SMSC_ID_REG);
+	if (id != 0x5d)
+		return -ENODEV;
+
+	/* Note: 0x25 is the 1404 which is very similar and this
+	   driver could be extended */
+	id = i2c_smbus_read_byte_data(client, THERMAL_PID_REG);
+	if (id != 0x21)
+		return -ENODEV;
+
+	id = i2c_smbus_read_byte_data(client, THERMAL_REVISION_REG);
+	if (id != 0x01)
+		return -ENODEV;
+
+	strlcpy(info->type, "emc1403", I2C_NAME_SIZE);
+	return 0;
+}
+ (...)
+
+const static struct i2c_device_id emc1403_idtable[] = {
+	{ "emc1403", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, emc1403_idtable);

Review

Quoted 10 lines
(quoted diff) +10 -0
+
+static struct i2c_driver sensor_emc1403 = {
+	.driver = {
+		.name = "emc1403",
+	},
+	.detect = emc1403_detect,
+	.probe = emc1403_probe,
+	.remove = emc1403_remove,
+	.id_table = emc1403_idtable,
+};

I had to apply the following patch for device detection to actually work:

---
 drivers/hwmon/emc1403.c |    6 ++++++
 1 file changed, 6 insertions(+)
drivers/hwmon/emc1403.c +6 -0
@@ -332,6 +332,10 @@ static int emc1403_remove(struct i2c_cli
 	return 0;
 }
 
+static const unsigned short emc1403_address_list[] = {
+	0x18, 0x2a, 0x4c, 0x4d, I2C_CLIENT_END
+};
+
 const static struct i2c_device_id emc1403_idtable[] = {
 	{ "emc1403", 0 },
 	{ }
@@ -339,6 +343,7 @@ const static struct i2c_device_id emc140
 MODULE_DEVICE_TABLE(i2c, emc1403_idtable);
 
 static struct i2c_driver sensor_emc1403 = {
+	.class = I2C_CLASS_HWMON,
 	.driver = {
 		.name = "emc1403",
 	},
@@ -346,6 +351,7 @@ static struct i2c_driver sensor_emc1403
 	.probe = emc1403_probe,
 	.remove = emc1403_remove,
 	.id_table = emc1403_idtable,
+	.address_list = emc1403_address_list,
 };
 
 static int __init sensor_emc1403_init(void)
Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
#37 lore raw

Re: [lm-sensors] [PATCH 4/4] emc1403: thermal sensor support

In reply to Alan Cox

Hi Alan,

Quoted 5 lines · Jean Delvare
Quoted 2 lines · Alan Cox

Other bits done.. I'll try and get it tested but my dev board is being recalcitrant right now.

Almost alright now, see my few remaining comments below.

Any news on this? I'd like to push your driver to linux-next ASAP so that it is ready to be merged in 2.6.35-rc1.

Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
author
#38 lore raw

Re: [lm-sensors] [PATCH 4/4] emc1403: thermal sensor support

In reply to Alan Cox
Quoted 3 lines

I have no problem with individual power up/down of devices. But this doesn't make it a hwmon-specific requirement, so I insist that it doesn't belong to the hwmon framework.

I guess that can stay a vendor applied patch for now as it really isn't obvious where else you would put it given the need to associate it accurately with a specific emc1403.

Quoted 7 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+	retval = i2c_smbus_read_byte_data(client, sda->nr);
+	if (retval < 0)
+		goto fail;
+
+	retval &= sda->index;

Missing negation?

Yep but goes away removing the power bit.

Quoted 6 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+		goto thermal_error2;
+	}
+	dev_info(&client->dev, "EMC1403 Thermal chip found\n");
+	return res;

An extra blank line here would be appreciated.

Agreed - done, and additional detection bits added from your mini-patch

Can't currently test this however.

emc1403: thermal sensor support

From: Kalhan Trisal <kalhan.trisal at intel.com>

Provides support for the emc1403 thermal sensor. Only reporting of values is supported. The various Moorestown specific extras to do with thermal alerts and the like are not in this version of the driver.

Considerably edited and tidied up by Alan Cox, plus fixes and detection bits from Jean Delvare.

Signed-off-by Kalhan Trisal <kalhan.trisal at intel.com>

---

 drivers/hwmon/Kconfig   |   10 +
 drivers/hwmon/Makefile  |    1 
 drivers/hwmon/emc1403.c |  344 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 355 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/emc1403.c
drivers/hwmon/Kconfig +10 -0
index 72b1c66..9310418 100644
@@ -750,6 +750,16 @@ config SENSORS_DME1737
 	  This driver can also be built as a module.  If so, the module
 	  will be called dme1737.
 
+config SENSORS_EMC1403
+	tristate "SMSC EMC1403 thermal sensor"
+	depends on I2C
+	help
+	  If you say yes here you get support for the SMSC EMC1403
+	  temperature monitoring chip.
+
+	  Threshold values can be configured using sysfs.
+	  Data from the different diodes are accessible via sysfs.
+
 config SENSORS_SMSC47M1
 	tristate "SMSC LPC47M10x and compatibles"
 	help
drivers/hwmon/Makefile +1 -0
index d33bd4a..6b369d7 100644
@@ -40,6 +40,7 @@ obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
 obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
 obj-$(CONFIG_SENSORS_DME1737)	+= dme1737.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
+obj-$(CONFIG_SENSORS_EMC1403)	+= emc1403.o
 obj-$(CONFIG_SENSORS_F71805F)	+= f71805f.o
 obj-$(CONFIG_SENSORS_F71882FG)	+= f71882fg.o
 obj-$(CONFIG_SENSORS_F75375S)	+= f75375s.o
drivers/hwmon/emc1403.c new file +344 -0

344 lines not on this page · raw message

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#39 lore raw

Re: [lm-sensors] [PATCH 4/4] emc1403: thermal sensor support

In reply to Alan Cox
Quoted 21 lines · Alan Cox

Agreed - done, and additional detection bits added from your mini-patch

Can't currently test this however.

emc1403: thermal sensor support

From: Kalhan Trisal <kalhan.trisal at intel.com>

Provides support for the emc1403 thermal sensor. Only reporting of values is supported. The various Moorestown specific extras to do with thermal alerts and the like are not in this version of the driver.

Considerably edited and tidied up by Alan Cox, plus fixes and detection bits from Jean Delvare.

Signed-off-by Kalhan Trisal <kalhan.trisal at intel.com>

---

Quoted 7 lines
 drivers/hwmon/Kconfig   |   10 +
 drivers/hwmon/Makefile  |    1 
 drivers/hwmon/emc1403.c |  344 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 355 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/emc1403.c

(...)

Fine with me, thank you. I've queued this new driver for merge in 2.6.35-rc1.

Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
#40 lore raw

Re: [lm-sensors] [PATCH 4/4] emc1403: thermal sensor support

In reply to Alan Cox

Hi again Alan,

Quoted 7 lines · Alan Cox
Quoted 3 lines

I have no problem with individual power up/down of devices. But this doesn't make it a hwmon-specific requirement, so I insist that it doesn't belong to the hwmon framework.

I guess that can stay a vendor applied patch for now as it really isn't obvious where else you would put it given the need to associate it accurately with a specific emc1403.

I am just realizing that my indent may have been misinterpreted. All I was saying was that it wasn't up to the hwmon subsystem to _standardize_ on power up/down sysfs attributes. I didn't mean to object against such attributes being added to a hwmon device. So feel free to add back the power attribute you just removed if it makes your life easier.

Sorry for the confusion.

Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
author
#41 lore raw

Re: [lm-sensors] [PATCH 4/4] emc1403: thermal sensor support

In reply to Alan Cox
Quoted 3 lines

I am just realizing that my indent may have been misinterpreted. All I was saying was that it wasn't up to the hwmon subsystem to _standardize_ on power up/down sysfs attributes.

Ah - that makes more sense

I'll worry about that later - I have to work out how to fit it all into the mrst platform power management code which uses it for thermal management. I'm not sure being able to turn off the sensor doing board power/thermal management except via the power manager in such cases is a good idea 8)

Alan

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#42 lore raw
In reply to Alan Cox

Hi Alan,

Quoted 11 lines · Alan Cox

These have been through my clean up processing, in particular the accelerometers are now using the input layer infrastructure.

---

Kalhan Trisal (4):
emc1403: thermal sensor support
liss331d1: accelerometer driver
isl29020: ambient light sensor
hmc6352: Add driver for the HMC6352 compass

The liss331d1, isl29020 and hmc6352 are not hardware monitoring drivers and thus do not belong to drivers/hwmon.

BTW, when sending actual hardware monitoring drivers, the right mailing list is lm-sensors, which you did not include. Please see MAINTAINERS.

Quoted 12 lines
 drivers/hwmon/Kconfig    |   34 ++++
 drivers/hwmon/Makefile   |    4 
 drivers/hwmon/emc1403.c  |  437 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/hmc6352.c  |  235 +++++++++++++++++++++++++
 drivers/hwmon/isl29020.c |  243 ++++++++++++++++++++++++++
 drivers/hwmon/lis331dl.c |  286 ++++++++++++++++++++++++++++++
 6 files changed, 1239 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/emc1403.c
 create mode 100644 drivers/hwmon/hmc6352.c
 create mode 100644 drivers/hwmon/isl29020.c
 create mode 100644 drivers/hwmon/lis331dl.c
Signature · 1 line
Jean Delvare
#43 lore raw
Quoted 7 lines
Quoted 5 lines

Kalhan Trisal (4):
emc1403: thermal sensor support
liss331d1: accelerometer driver
isl29020: ambient light sensor
hmc6352: Add driver for the HMC6352 compass

The liss331d1, isl29020 and hmc6352 are not hardware monitoring drivers

Disagree somewhat. In fact on close grepping I find that there is another related lis33 implementation in drivers/hwmon already 8)

Given all the accelerometers are in drivers/hwmon where do you think they should be, and do you have pending patches to move the others ?

I'd also be interested where you think the compass fits if its not hwmon, ditto the ambient light sensor ?

I'll bounce the emc1403 onto lm-sensors.

Thanks
Alan

#44 lore raw
Quoted 8 lines · Alan Cox
Quoted 7 lines
Quoted 5 lines

Kalhan Trisal (4):
emc1403: thermal sensor support
liss331d1: accelerometer driver
isl29020: ambient light sensor
hmc6352: Add driver for the HMC6352 compass

The liss331d1, isl29020 and hmc6352 are not hardware monitoring drivers

Quoted 8 lines

Disagree somewhat. In fact on close grepping I find that there is another related lis33 implementation in drivers/hwmon already 8)

Given all the accelerometers are in drivers/hwmon where do you think they should be, and do you have pending patches to move the others ?

I'd also be interested where you think the compass fits if its not hwmon, ditto the ambient light sensor ?

If you like, feel free to (re)start an argument with Linus on the ambient light sensor front. Or see that thread for yet another round about discussion of where these sensors should be.

http://lkml.org/lkml/2010/3/1/367

Basically if it is primarily an input device for human interaction, see if Dmitry is willing to take it into input (though note he may quite rightly take some convincing!)

Otherwise, I'm happy to take more general sensors into IIO, but obviously that is still in staging and evolving reasonably quickly at the moment (large number of abi clean up patches half way through cleanup at the mo.)

Otherwise, misc with the intent to sweep them all up when an agreed upon subsystem is in place.

Quoted 10 lines

I'll bounce the emc1403 onto lm-sensors.

Thanks
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#45 lore raw
Quoted 7 lines

If you like, feel free to (re)start an argument with Linus on the ambient light sensor front. Or see that thread for yet another round about discussion of where these sensors should be.

http://lkml.org/lkml/2010/3/1/367

Basically if it is primarily an input device for human interaction, see if

Its a sensor on an embedded board, it might be managing the backlight or measuring the dirt level in a sewage pipe depending upon application.

Quoted 2 lines

Dmitry is willing to take it into input (though note he may quite rightly take some convincing!)

Its easy enough to make it an input device but that would then end up with polled_input_dev which keeps waking up and costs battery. A lot of the polled input model is somewhat broken in that regard but I can see Linus point.

Quoted 2 lines

Otherwise, misc with the intent to sweep them all up when an agreed upon subsystem is in place.

Looks like goes into misc for the moment with all the other light sensors and I'll see if I can merge it with the 29007 support while I am at it.

Thanks

Alan