[PATCH 2.6] I2C: New chip driver: sis5595

[PATCH 2.6] I2C: New chip driver: sis5595

[Aurélien Jarno]

Hi Greg,

The following patch against kernel 2.6.11-rc2-mm1 adds the sis5595
driver (sensor part). I have ported it from the 2.4 version

It has been reviewed by Jean Delvare, partly on IRC.

Please apply.

Thanks,
Aurelien

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

diff -urN linux-2.6.11-rc2-mm1.orig/drivers/i2c/chips/Kconfig linux-2.6.11-rc2-mm1/drivers/i2c/chips/Kconfig
--- linux-2.6.11-rc2-mm1.orig/drivers/i2c/chips/Kconfig	2005-01-25 18:06:55.000000000 +0100
+++ linux-2.6.11-rc2-mm1/drivers/i2c/chips/Kconfig	2005-01-25 18:25:08.000000000 +0100
@@ -262,6 +262,18 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called smsc47b397.
 
+config SENSORS_SIS5595
+	tristate "Silicon Integrated Systems Corp. SiS5595"
+	depends on I2C && PCI && EXPERIMENTAL
+	select I2C_SENSOR
+	select I2C_ISA
+	help
+	  If you say yes here you get support for the integrated sensors in
+	  SiS5595 South Bridges.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called sis5595.
+
 config SENSORS_SMSC47M1
 	tristate "SMSC LPC47M10x and compatibles"
 	depends on I2C && EXPERIMENTAL
diff -urN linux-2.6.11-rc2-mm1.orig/drivers/i2c/chips/Makefile linux-2.6.11-rc2-mm1/drivers/i2c/chips/Makefile
--- linux-2.6.11-rc2-mm1.orig/drivers/i2c/chips/Makefile	2005-01-25 18:06:55.000000000 +0100
+++ linux-2.6.11-rc2-mm1/drivers/i2c/chips/Makefile	2005-01-25 18:25:31.000000000 +0100
@@ -31,6 +31,7 @@
 obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
 obj-$(CONFIG_SENSORS_RTC8564)	+= rtc8564.o
+obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
 obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
 obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
 obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
diff -urN linux-2.6.11-rc2-mm1.orig/drivers/i2c/chips/sis5595.c linux-2.6.11-rc2-mm1/drivers/i2c/chips/sis5595.c
--- linux-2.6.11-rc2-mm1.orig/drivers/i2c/chips/sis5595.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.11-rc2-mm1/drivers/i2c/chips/sis5595.c	2005-01-25 18:25:08.000000000 +0100
@@ -0,0 +1,772 @@
+/*
+    sis5595.c - Part of lm_sensors, Linux kernel modules
+		for hardware monitoring
+
+    Copyright (C) 1998 - 2001 Frodo Looijaard <frodol@dds.nl>,
+			Kyösti Mälkki <kmalkki@cc.hut.fi>, and
+			Mark D. Studebaker <mdsxyz123@yahoo.com>
+    Ported to Linux 2.6 by Aurelien Jarno <aurelien@aurel32.net> with
+    the help of Jean Delvare <khali@linux-fr.org>
+
+    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; either version 2 of the License, or
+    (at your option) any later version.
+
+    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., 675 Mass Ave, Cambridge, MA 02139, USA.
+*/
+
+/*
+   SiS southbridge has a LM78-like chip integrated on the same IC.
+   This driver is a customized copy of lm78.c
+   
+   Supports following revisions:
+	Version		PCI ID		PCI Revision
+	1		1039/0008	AF or less
+	2		1039/0008	B0 or greater
+
+   Note: these chips contain a 0008 device which is incompatible with the
+	 5595. We recognize these by the presence of the listed
+	 "blacklist" PCI ID and refuse to load.
+
+   NOT SUPPORTED	PCI ID		BLACKLIST PCI ID	
+	 540		0008		0540
+	 550		0008		0550
+	5513		0008		5511
+	5581		0008		5597
+	5582		0008		5597
+	5597		0008		5597
+	5598		0008		5597/5598
+	 630		0008		0630
+	 645		0008		0645
+	 730		0008		0730
+	 735		0008		0735
+*/
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/ioport.h>
+#include <linux/pci.h>
+#include <linux/i2c.h>
+#include <linux/i2c-sensor.h>
+#include <linux/init.h>
+#include <asm/io.h>
+
+
+/* If force_addr is set to anything different from 0, we forcibly enable
+   the device at the given address. */
+static u16 force_addr;
+module_param(force_addr, ushort, 0);
+MODULE_PARM_DESC(force_addr,
+		 "Initialize the base address of the sensors");
+
+/* Addresses to scan.
+   Note that we can't determine the ISA address until we have initialized
+   our module */
+static unsigned short normal_i2c[] = { I2C_CLIENT_END };
+static unsigned int normal_isa[] = { 0x0000, I2C_CLIENT_ISA_END };
+
+/* Insmod parameters */
+SENSORS_INSMOD_1(sis5595);
+
+static int blacklist[] = {
+			PCI_DEVICE_ID_SI_540,
+			PCI_DEVICE_ID_SI_550,
+			PCI_DEVICE_ID_SI_630,
+			PCI_DEVICE_ID_SI_730,
+			PCI_DEVICE_ID_SI_5511, /* 5513 chip has the 0008 device but
+						  that ID shows up in other chips so we
+						  use the 5511 ID for recognition */
+			PCI_DEVICE_ID_SI_5597,
+			PCI_DEVICE_ID_SI_5598,
+			PCI_DEVICE_ID_SI_645,
+			PCI_DEVICE_ID_SI_735,
+			0 };
+
+/* Many SIS5595 constants specified below */
+
+/* Length of ISA address segment */
+#define SIS5595_EXTENT 8
+/* PCI Config Registers */
+#define SIS5595_REVISION_REG 0x08
+#define SIS5595_BASE_REG 0x68
+#define SIS5595_PIN_REG 0x7A
+#define SIS5595_ENABLE_REG 0x7B
+
+/* Where are the ISA address/data registers relative to the base address */
+#define SIS5595_ADDR_REG_OFFSET 5
+#define SIS5595_DATA_REG_OFFSET 6
+
+/* The SIS5595 registers */
+#define SIS5595_REG_IN_MAX(nr) (0x2b + (nr) * 2)
+#define SIS5595_REG_IN_MIN(nr) (0x2c + (nr) * 2)
+#define SIS5595_REG_IN(nr) (0x20 + (nr))
+
+#define SIS5595_REG_FAN_MIN(nr) (0x3b + (nr))
+#define SIS5595_REG_FAN(nr) (0x28 + (nr))
+
+/* On the first version of the chip, the temp registers are separate.
+   On the second version,
+   TEMP pin is shared with IN4, configured in PCI register 0x7A.
+   The registers are the same as well.
+   OVER and HYST are really MAX and MIN. */
+
+#define REV2MIN	0xb0
+#define SIS5595_REG_TEMP 	(( data->revision) >= REV2MIN) ? \
+					SIS5595_REG_IN(4) : 0x27
+#define SIS5595_REG_TEMP_OVER	(( data->revision) >= REV2MIN) ? \
+					SIS5595_REG_IN_MAX(4) : 0x39
+#define SIS5595_REG_TEMP_HYST	(( data->revision) >= REV2MIN) ? \
+					SIS5595_REG_IN_MIN(4) : 0x3a
+
+#define SIS5595_REG_CONFIG 0x40
+#define SIS5595_REG_ALARM1 0x41
+#define SIS5595_REG_ALARM2 0x42
+#define SIS5595_REG_FANDIV 0x47
+
+/* Conversions. Limit checking is only done on the TO_REG
+   variants. */
+
+/* IN: mV, (0V to 4.08V)
+   REG: 16mV/bit */
+static inline u8 IN_TO_REG(unsigned long val)
+{
+	unsigned long nval = SENSORS_LIMIT(val, 0, 4080);
+	return (nval + 8) / 16;
+}
+#define IN_FROM_REG(val) ((val) *  16)
+
+static inline u8 FAN_TO_REG(long rpm, int div)
+{
+	if (rpm <= 0)
+		return 255;
+	return SENSORS_LIMIT((1350000 + rpm * div / 2) / (rpm * div), 1, 254);
+}
+
+static inline int FAN_FROM_REG(u8 val, int div)
+{
+	return val==0 ? -1 : val==255 ? 0 : 1350000/(val*div);
+}
+
+/* TEMP: mC (-54.12C to +157.53C)
+   REG: 0.83C/bit + 52.12, two's complement  */
+static inline int TEMP_FROM_REG(s8 val)
+{
+	return val * 830 + 52120;
+}
+static inline s8 TEMP_TO_REG(int val)
+{
+	int nval = SENSORS_LIMIT(val, -54120, 157530) ;
+	return nval<0 ? (nval-5212-415)/830 : (nval-5212+415)/830;
+}
+
+/* FAN DIV: 1, 2, 4, or 8 (defaults to 2)
+   REG: 0, 1, 2, or 3 (respectively) (defaults to 1) */
+static inline u8 DIV_TO_REG(int val)
+{
+	return val==8 ? 3 : val==4 ? 2 : val==1 ? 0 : 1;
+}
+#define DIV_FROM_REG(val) (1 << (val))
+
+/* For the SIS5595, we need to keep some data in memory. That
+   data is pointed to by sis5595_list[NR]->data. The structure itself is
+   dynamically allocated, at the time when the new sis5595 client is
+   allocated. */
+struct sis5595_data {
+	struct i2c_client client;
+	struct semaphore lock;
+
+	struct semaphore update_lock;
+	char valid;		/* !=0 if following fields are valid */
+	unsigned long last_updated;	/* In jiffies */
+	char maxins;		/* == 3 if temp enabled, otherwise == 4 */
+	u8 revision;		/* Reg. value */
+
+	u8 in[5];		/* Register value */
+	u8 in_max[5];		/* Register value */
+	u8 in_min[5];		/* Register value */
+	u8 fan[2];		/* Register value */
+	u8 fan_min[2];		/* Register value */
+	s8 temp;		/* Register value */
+	s8 temp_over;		/* Register value */
+	s8 temp_hyst;		/* Register value */
+	u8 fan_div[2];		/* Register encoding, shifted right */
+	u16 alarms;		/* Register encoding, combined */
+};
+
+static struct pci_dev *s_bridge;	/* pointer to the (only) sis5595 */
+
+static int sis5595_attach_adapter(struct i2c_adapter *adapter);
+static int sis5595_detect(struct i2c_adapter *adapter, int address, int kind);
+static int sis5595_detach_client(struct i2c_client *client);
+
+static int sis5595_read_value(struct i2c_client *client, u8 register);
+static int sis5595_write_value(struct i2c_client *client, u8 register, u8 value);
+static struct sis5595_data *sis5595_update_device(struct device *dev);
+static void sis5595_init_client(struct i2c_client *client);
+static int sis5595_find_sis(int *address);
+
+static struct i2c_driver sis5595_driver = {
+	.owner		= THIS_MODULE,
+	.name		= "sis5595",
+	.id		= I2C_DRIVERID_SIS5595,
+	.flags		= I2C_DF_NOTIFY,
+	.attach_adapter	= sis5595_attach_adapter,
+	.detach_client	= sis5595_detach_client,
+};
+
+/* 4 Voltages */
+static ssize_t show_in(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", IN_FROM_REG(data->in[nr]));
+}
+
+static ssize_t show_in_min(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", IN_FROM_REG(data->in_min[nr]));
+}
+
+static ssize_t show_in_max(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", IN_FROM_REG(data->in_max[nr]));
+}
+
+static ssize_t set_in_min(struct device *dev, const char *buf,
+	       size_t count, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+	data->in_min[nr] = IN_TO_REG(val);
+	sis5595_write_value(client, SIS5595_REG_IN_MIN(nr), data->in_min[nr]);
+	return count;
+}
+
+static ssize_t set_in_max(struct device *dev, const char *buf,
+	       size_t count, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+	data->in_max[nr] = IN_TO_REG(val);
+	sis5595_write_value(client, SIS5595_REG_IN_MAX(nr), data->in_max[nr]);
+	return count;
+}
+
+#define show_in_offset(offset)					\
+static ssize_t							\
+	show_in##offset (struct device *dev, char *buf)		\
+{								\
+	return show_in(dev, buf, offset);			\
+}								\
+static DEVICE_ATTR(in##offset##_input, S_IRUGO, 		\
+		show_in##offset, NULL);				\
+static ssize_t							\
+	show_in##offset##_min (struct device *dev, char *buf)	\
+{								\
+	return show_in_min(dev, buf, offset);			\
+}								\
+static ssize_t							\
+	show_in##offset##_max (struct device *dev, char *buf)	\
+{								\
+	return show_in_max(dev, buf, offset);			\
+}								\
+static ssize_t set_in##offset##_min (struct device *dev,	\
+		const char *buf, size_t count)			\
+{								\
+	return set_in_min(dev, buf, count, offset);		\
+}								\
+static ssize_t set_in##offset##_max (struct device *dev,	\
+		const char *buf, size_t count)			\
+{								\
+	return set_in_max(dev, buf, count, offset);		\
+}								\
+static DEVICE_ATTR(in##offset##_min, S_IRUGO | S_IWUSR,		\
+		show_in##offset##_min, set_in##offset##_min);	\
+static DEVICE_ATTR(in##offset##_max, S_IRUGO | S_IWUSR,		\
+		show_in##offset##_max, set_in##offset##_max);
+
+show_in_offset(0);
+show_in_offset(1);
+show_in_offset(2);
+show_in_offset(3);
+show_in_offset(4);
+
+/* Temperature */
+static ssize_t show_temp(struct device *dev, char *buf)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp));
+}
+
+static ssize_t show_temp_over(struct device *dev, char *buf)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_over));
+}
+
+static ssize_t set_temp_over(struct device *dev, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	long val = simple_strtol(buf, NULL, 10);
+	data->temp_over = TEMP_TO_REG(val);
+	sis5595_write_value(client, SIS5595_REG_TEMP_OVER, data->temp_over);
+	return count;
+}
+
+static ssize_t show_temp_hyst(struct device *dev, char *buf)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_hyst));
+}
+
+static ssize_t set_temp_hyst(struct device *dev, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	long val = simple_strtol(buf, NULL, 10);
+	data->temp_hyst = TEMP_TO_REG(val);
+	sis5595_write_value(client, SIS5595_REG_TEMP_HYST, data->temp_hyst);
+	return count;
+}
+
+static DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL);
+static DEVICE_ATTR(temp1_max, S_IRUGO | S_IWUSR,
+		show_temp_over, set_temp_over);
+static DEVICE_ATTR(temp1_max_hyst, S_IRUGO | S_IWUSR,
+		show_temp_hyst, set_temp_hyst);
+
+/* 2 Fans */
+static ssize_t show_fan(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[nr],
+		DIV_FROM_REG(data->fan_div[nr])) );
+}
+
+static ssize_t show_fan_min(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf,"%d\n", FAN_FROM_REG(data->fan_min[nr],
+		DIV_FROM_REG(data->fan_div[nr])) );
+}
+
+static ssize_t set_fan_min(struct device *dev, const char *buf,
+		size_t count, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+	data->fan_min[nr] = FAN_TO_REG(val, DIV_FROM_REG(data->fan_div[nr]));
+	sis5595_write_value(client, SIS5595_REG_FAN_MIN(nr), data->fan_min[nr]);
+	return count;
+}
+
+static ssize_t show_fan_div(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", DIV_FROM_REG(data->fan_div[nr]) );
+}
+
+/* Note: we save and restore the fan minimum here, because its value is
+   determined in part by the fan divisor.  This follows the principle of
+   least suprise; the user doesn't expect the fan minimum to change just
+   because the divisor changed. */
+static ssize_t set_fan_div(struct device *dev, const char *buf,
+	size_t count, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	unsigned long min = FAN_FROM_REG(data->fan_min[nr],
+			DIV_FROM_REG(data->fan_div[nr]));
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+	int reg = sis5595_read_value(client, SIS5595_REG_FANDIV);
+	switch (val) {
+	case 1: data->fan_div[nr] = 0; break;
+	case 2: data->fan_div[nr] = 1; break;
+	case 4: data->fan_div[nr] = 2; break;
+	case 8: data->fan_div[nr] = 3; break;
+	default:
+		dev_err(&client->dev, "fan_div value %ld not "
+			"supported. Choose one of 1, 2, 4 or 8!\n", val);
+		return -EINVAL;
+	}
+	
+	switch (nr) {
+	case 0:
+		reg = (reg & 0xcf) | (data->fan_div[nr] << 4);
+		break;
+	case 1:
+		reg = (reg & 0x3f) | (data->fan_div[nr] << 6);
+		break;
+	}
+	sis5595_write_value(client, SIS5595_REG_FANDIV, reg);
+	data->fan_min[nr] =
+		FAN_TO_REG(min, DIV_FROM_REG(data->fan_div[nr]));
+	sis5595_write_value(client, SIS5595_REG_FAN_MIN(nr), data->fan_min[nr]);
+	return count;
+}
+
+#define show_fan_offset(offset)						\
+static ssize_t show_fan_##offset (struct device *dev, char *buf)	\
+{									\
+	return show_fan(dev, buf, offset - 1);			\
+}									\
+static ssize_t show_fan_##offset##_min (struct device *dev, char *buf)	\
+{									\
+	return show_fan_min(dev, buf, offset - 1);			\
+}									\
+static ssize_t show_fan_##offset##_div (struct device *dev, char *buf)	\
+{									\
+	return show_fan_div(dev, buf, offset - 1);			\
+}									\
+static ssize_t set_fan_##offset##_min (struct device *dev,		\
+		const char *buf, size_t count)				\
+{									\
+	return set_fan_min(dev, buf, count, offset - 1);		\
+}									\
+static DEVICE_ATTR(fan##offset##_input, S_IRUGO, show_fan_##offset, NULL);\
+static DEVICE_ATTR(fan##offset##_min, S_IRUGO | S_IWUSR,		\
+		show_fan_##offset##_min, set_fan_##offset##_min);
+
+show_fan_offset(1);
+show_fan_offset(2);
+
+static ssize_t set_fan_1_div(struct device *dev, const char *buf,
+		size_t count)
+{
+	return set_fan_div(dev, buf, count, 0) ;
+}
+
+static ssize_t set_fan_2_div(struct device *dev, const char *buf,
+		size_t count)
+{
+	return set_fan_div(dev, buf, count, 1) ;
+}
+static DEVICE_ATTR(fan1_div, S_IRUGO | S_IWUSR,
+		show_fan_1_div, set_fan_1_div);
+static DEVICE_ATTR(fan2_div, S_IRUGO | S_IWUSR,
+		show_fan_2_div, set_fan_2_div);
+
+/* Alarms */
+static ssize_t show_alarms(struct device *dev, char *buf)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", data->alarms);
+}
+static DEVICE_ATTR(alarms, S_IRUGO, show_alarms, NULL);
+ 
+/* This is called when the module is loaded */
+static int sis5595_attach_adapter(struct i2c_adapter *adapter)
+{
+	if (!(adapter->class & I2C_CLASS_HWMON))
+		return 0;
+	return i2c_detect(adapter, &addr_data, sis5595_detect);
+}
+
+/* Locate SiS bridge and correct base address for SIS5595 */
+static int sis5595_find_sis(int *address)
+{
+	u16 val;
+	int *i;
+
+	if (!(s_bridge =
+	    pci_find_device(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_503, NULL)))
+		return -ENODEV;
+
+	/* Look for imposters */
+	for(i = blacklist; *i != 0; i++) {
+		if (pci_find_device(PCI_VENDOR_ID_SI, *i, NULL)) {
+			printk("sis5595.ko: Error: Looked for SIS5595 but found unsupported device %.4X\n", *i);
+			return -ENODEV;
+		}
+	}
+
+	if (PCIBIOS_SUCCESSFUL !=
+	    pci_read_config_word(s_bridge, SIS5595_BASE_REG, &val))
+		return -ENODEV;
+
+	*address = val & ~(SIS5595_EXTENT - 1);
+	if (*address == 0 && force_addr == 0) {
+		printk("sis5595.ko: base address not set - upgrade BIOS or use force_addr=0xaddr\n");
+		return -ENODEV;
+	}
+	if (force_addr)
+		*address = force_addr;	/* so detect will get called */
+
+	return 0;
+}
+
+int sis5595_detect(struct i2c_adapter *adapter, int address, int kind)
+{
+	int err = 0;
+	int i;
+	struct i2c_client *new_client;
+	struct sis5595_data *data;
+	char val;
+	u16 a;
+
+	/* Make sure we are probing the ISA bus!!  */
+	if (!i2c_is_isa_adapter(adapter))
+		goto exit;
+
+	if(force_addr)
+		address = force_addr & ~(SIS5595_EXTENT - 1);
+	/* Reserve the ISA region */
+	if (!request_region(address, SIS5595_EXTENT, sis5595_driver.name)) {
+		err = -EBUSY;
+		goto exit;
+	}
+	if(force_addr) {
+		dev_warn(&adapter->dev, "forcing ISA address 0x%04X\n", address);
+		if (PCIBIOS_SUCCESSFUL !=
+		    pci_write_config_word(s_bridge, SIS5595_BASE_REG, address))
+			goto exit_release;
+		if (PCIBIOS_SUCCESSFUL !=
+		    pci_read_config_word(s_bridge, SIS5595_BASE_REG, &a))
+			goto exit_release;
+		if ((a & ~(SIS5595_EXTENT - 1)) != address)
+			/* doesn't work for some chips? */
+			goto exit_release;
+	}
+
+	if (PCIBIOS_SUCCESSFUL !=
+	    pci_read_config_byte(s_bridge, SIS5595_ENABLE_REG, &val)) {
+		goto exit_release;
+	}
+	if((val & 0x80) == 0) {
+		if (PCIBIOS_SUCCESSFUL !=
+		    pci_write_config_byte(s_bridge, SIS5595_ENABLE_REG,
+					  val | 0x80))
+			goto exit_release;
+		if (PCIBIOS_SUCCESSFUL !=
+		    pci_read_config_byte(s_bridge, SIS5595_ENABLE_REG, &val))
+			goto exit_release;
+		if((val & 0x80) == 0) 
+			/* doesn't work for some chips! */
+			goto exit_release;
+	}
+
+	if (!(data = kmalloc(sizeof(struct sis5595_data), GFP_KERNEL))) {
+		err = -ENOMEM;
+		goto exit_release;
+	}
+	memset(data, 0, sizeof(struct sis5595_data));
+
+	new_client = &data->client;
+	new_client->addr = address;
+	init_MUTEX(&data->lock);
+	i2c_set_clientdata(new_client, data);
+	new_client->adapter = adapter;
+	new_client->driver = &sis5595_driver;
+	new_client->flags = 0;
+
+	/* Check revision and pin registers to determine whether 4 or 5 voltages */
+	pci_read_config_byte(s_bridge, SIS5595_REVISION_REG, &(data->revision));
+	/* 4 voltages, 1 temp */
+	data->maxins = 3;
+	if(data->revision >= REV2MIN) {
+		pci_read_config_byte(s_bridge, SIS5595_PIN_REG, &val);
+		if (!(val & 0x80))
+			/* 5 voltages, no temps */
+			data->maxins = 4;
+	}
+	
+	/* Fill in the remaining client fields and put it into the global list */
+	strlcpy(new_client->name, "sis5595", I2C_NAME_SIZE);
+
+	data->valid = 0;
+	init_MUTEX(&data->update_lock);
+
+	/* Tell the I2C layer a new client has arrived */
+	if ((err = i2c_attach_client(new_client)))
+		goto exit_free;
+	
+	/* Initialize the SIS5595 chip */
+	sis5595_init_client(new_client);
+
+	/* A few vars need to be filled upon startup */
+	for (i = 0; i < 2; i++) {
+		data->fan_min[i] = sis5595_read_value(new_client,
+					SIS5595_REG_FAN_MIN(i));
+	}
+
+	/* Register sysfs hooks */
+	device_create_file(&new_client->dev, &dev_attr_in0_input);
+	device_create_file(&new_client->dev, &dev_attr_in0_min);
+	device_create_file(&new_client->dev, &dev_attr_in0_max);
+	device_create_file(&new_client->dev, &dev_attr_in1_input);
+	device_create_file(&new_client->dev, &dev_attr_in1_min);
+	device_create_file(&new_client->dev, &dev_attr_in1_max);
+	device_create_file(&new_client->dev, &dev_attr_in2_input);
+	device_create_file(&new_client->dev, &dev_attr_in2_min);
+	device_create_file(&new_client->dev, &dev_attr_in2_max);
+	device_create_file(&new_client->dev, &dev_attr_in3_input);
+	device_create_file(&new_client->dev, &dev_attr_in3_min);
+	device_create_file(&new_client->dev, &dev_attr_in3_max);
+	if (data->maxins == 4) {
+		device_create_file(&new_client->dev, &dev_attr_in4_input);
+		device_create_file(&new_client->dev, &dev_attr_in4_min);
+		device_create_file(&new_client->dev, &dev_attr_in4_max);
+	}
+	device_create_file(&new_client->dev, &dev_attr_fan1_input);
+	device_create_file(&new_client->dev, &dev_attr_fan1_min);
+	device_create_file(&new_client->dev, &dev_attr_fan1_div);
+	device_create_file(&new_client->dev, &dev_attr_fan2_input);
+	device_create_file(&new_client->dev, &dev_attr_fan2_min);
+	device_create_file(&new_client->dev, &dev_attr_fan2_div);
+	device_create_file(&new_client->dev, &dev_attr_alarms);
+	if (data->maxins == 3) {
+		device_create_file(&new_client->dev, &dev_attr_temp1_input);
+		device_create_file(&new_client->dev, &dev_attr_temp1_max);
+		device_create_file(&new_client->dev, &dev_attr_temp1_max_hyst);
+	}
+	return 0;
+	
+exit_free:
+	kfree(data);
+exit_release:
+	release_region(address, SIS5595_EXTENT);
+exit:
+	return err;
+}
+
+static int sis5595_detach_client(struct i2c_client *client)
+{
+	int err;
+
+	if ((err = i2c_detach_client(client))) {
+		dev_err(&client->dev,
+		    "Client deregistration failed, client not detached.\n");
+		return err;
+	}
+
+	if(i2c_is_isa_client(client))
+		release_region(client->addr, SIS5595_EXTENT);
+
+	kfree(i2c_get_clientdata(client));
+
+	return 0;
+}
+
+
+/* ISA access must be locked explicitly. */
+static int sis5595_read_value(struct i2c_client *client, u8 reg)
+{
+	int res;
+
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	down(&data->lock);
+	outb_p(reg, client->addr + SIS5595_ADDR_REG_OFFSET);
+	res = inb_p(client->addr + SIS5595_DATA_REG_OFFSET);
+	up(&data->lock);
+	return res;
+}
+
+static int sis5595_write_value(struct i2c_client *client, u8 reg, u8 value)
+{
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	down(&data->lock);
+	outb_p(reg, client->addr + SIS5595_ADDR_REG_OFFSET);
+	outb_p(value, client->addr + SIS5595_DATA_REG_OFFSET);
+	up(&data->lock);
+	return 0;
+}
+
+/* Called when we have found a new SIS5595. */
+static void sis5595_init_client(struct i2c_client *client)
+{
+	u8 config = sis5595_read_value(client, SIS5595_REG_CONFIG);
+	if (!(config & 0x01))
+		sis5595_write_value(client, SIS5595_REG_CONFIG,
+				(config & 0xf7) | 0x01);
+}
+
+static struct sis5595_data *sis5595_update_device(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	int i;
+
+	down(&data->update_lock);
+
+	if ((jiffies - data->last_updated > HZ + HZ / 2) ||
+	    (jiffies < data->last_updated) || !data->valid) {
+
+		for (i = 0; i <= data->maxins; i++) {
+			data->in[i] =
+			    sis5595_read_value(client, SIS5595_REG_IN(i));
+			data->in_min[i] =
+			    sis5595_read_value(client,
+					       SIS5595_REG_IN_MIN(i));
+			data->in_max[i] =
+			    sis5595_read_value(client,
+					       SIS5595_REG_IN_MAX(i));
+		}
+		for (i = 0; i < 2; i++) {
+			data->fan[i] =
+			    sis5595_read_value(client, SIS5595_REG_FAN(i));
+			data->fan_min[i] =
+			    sis5595_read_value(client,
+					       SIS5595_REG_FAN_MIN(i));
+		}
+		if(data->maxins == 3) {
+			data->temp =
+			    sis5595_read_value(client, SIS5595_REG_TEMP);
+			data->temp_over =
+			    sis5595_read_value(client, SIS5595_REG_TEMP_OVER);
+			data->temp_hyst =
+			    sis5595_read_value(client, SIS5595_REG_TEMP_HYST);
+		}
+		i = sis5595_read_value(client, SIS5595_REG_FANDIV);
+		data->fan_div[0] = (i >> 4) & 0x03;
+		data->fan_div[1] = i >> 6;
+		data->alarms =
+		    sis5595_read_value(client, SIS5595_REG_ALARM1) |
+		    (sis5595_read_value(client, SIS5595_REG_ALARM2) << 8);
+		data->last_updated = jiffies;
+		data->valid = 1;
+	}
+
+	up(&data->update_lock);
+
+	return data;
+}
+
+
+
+static int __init sm_sis5595_init(void)
+{
+	int addr;
+
+	if (sis5595_find_sis(&addr))
+		return -ENODEV;
+	normal_isa[0] = addr;
+
+	return i2c_add_driver(&sis5595_driver);
+}
+
+static void __exit sm_sis5595_exit(void)
+{
+	i2c_del_driver(&sis5595_driver);
+}
+
+
+
+MODULE_AUTHOR("Aurelien Jarno <aurelien@aurel32.net>");
+MODULE_DESCRIPTION("SiS 5595 Sensor device");
+MODULE_LICENSE("GPL");
+
+module_init(sm_sis5595_init);
+module_exit(sm_sis5595_exit);

-- 
  .''`.  Aurelien Jarno	              GPG: 1024D/F1BCDB73
 : :' :  Debian GNU/Linux developer | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [PATCH 2.6] I2C: New chip driver: sis5595

[Greg KH]

On Tue, Jan 25, 2005 at 11:09:45PM +0100, Aur?lien Jarno wrote:
> +/* Locate SiS bridge and correct base address for SIS5595 */
> +static int sis5595_find_sis(int *address)
> +{
> +	u16 val;
> +	int *i;
> +
> +	if (!(s_bridge =
> +	    pci_find_device(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_503, NULL)))

Please use pci_get_device().  It's safer, and not a depreciated
function.

> +	/* Look for imposters */
> +	for(i = blacklist; *i != 0; i++) {
> +		if (pci_find_device(PCI_VENDOR_ID_SI, *i, NULL)) {

Same here.

> +			printk("sis5595.ko: Error: Looked for SIS5595 but found unsupported device %.4X\n", *i);

<snip>

> +		printk("sis5595.ko: base address not set - upgrade BIOS or use force_addr=0xaddr\n");

These printk() calls need a KERN_ level.  Why not use the dev_* calls
instead?  You have a pci_dev to use here, right?

thanks,

greg k-h

Re: [PATCH 2.6] I2C: New chip driver: sis5595

[Aurelien Jarno]

Hi Greg, 

Please find below the new version of the patch against kernel 
2.6.11-rc2-mm2 to add the sis5595 driver (sensor part). 

As you suggested, I have changed pci_find_device by pci_get_device, and
replaced the printk() by dev_err(). I have also sorted the blacklist so
that the PCI devices are listed in alphanumerical order (as in the 2.4
driver).

Thanks,
Aurelien


Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

diff -urN linux-2.6.11-rc2-mm2.orig/drivers/i2c/chips/Kconfig linux-2.6.11-rc2-mm2/drivers/i2c/chips/Kconfig
--- linux-2.6.11-rc2-mm2.orig/drivers/i2c/chips/Kconfig	2005-02-01 07:39:24.000000000 +0100
+++ linux-2.6.11-rc2-mm2/drivers/i2c/chips/Kconfig	2005-02-01 07:40:55.000000000 +0100
@@ -262,6 +262,18 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called smsc47b397.
 
+config SENSORS_SIS5595
+	tristate "Silicon Integrated Systems Corp. SiS5595"
+	depends on I2C && PCI && EXPERIMENTAL
+	select I2C_SENSOR
+	select I2C_ISA
+	help
+	  If you say yes here you get support for the integrated sensors in
+	  SiS5595 South Bridges.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called sis5595.
+
 config SENSORS_SMSC47M1
 	tristate "SMSC LPC47M10x and compatibles"
 	depends on I2C && EXPERIMENTAL
diff -urN linux-2.6.11-rc2-mm2.orig/drivers/i2c/chips/Makefile linux-2.6.11-rc2-mm2/drivers/i2c/chips/Makefile
--- linux-2.6.11-rc2-mm2.orig/drivers/i2c/chips/Makefile	2005-02-01 07:39:24.000000000 +0100
+++ linux-2.6.11-rc2-mm2/drivers/i2c/chips/Makefile	2005-02-01 07:40:55.000000000 +0100
@@ -31,6 +31,7 @@
 obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
 obj-$(CONFIG_SENSORS_RTC8564)	+= rtc8564.o
+obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
 obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
 obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
 obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
diff -urN linux-2.6.11-rc2-mm2.orig/drivers/i2c/chips/sis5595.c linux-2.6.11-rc2-mm2/drivers/i2c/chips/sis5595.c
--- linux-2.6.11-rc2-mm2.orig/drivers/i2c/chips/sis5595.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.11-rc2-mm2/drivers/i2c/chips/sis5595.c	2005-02-01 07:40:55.000000000 +0100
@@ -0,0 +1,772 @@
+/*
+    sis5595.c - Part of lm_sensors, Linux kernel modules
+		for hardware monitoring
+
+    Copyright (C) 1998 - 2001 Frodo Looijaard <frodol@dds.nl>,
+			Kyösti Mälkki <kmalkki@cc.hut.fi>, and
+			Mark D. Studebaker <mdsxyz123@yahoo.com>
+    Ported to Linux 2.6 by Aurelien Jarno <aurelien@aurel32.net> with
+    the help of Jean Delvare <khali@linux-fr.org>
+
+    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; either version 2 of the License, or
+    (at your option) any later version.
+
+    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., 675 Mass Ave, Cambridge, MA 02139, USA.
+*/
+
+/*
+   SiS southbridge has a LM78-like chip integrated on the same IC.
+   This driver is a customized copy of lm78.c
+   
+   Supports following revisions:
+	Version		PCI ID		PCI Revision
+	1		1039/0008	AF or less
+	2		1039/0008	B0 or greater
+
+   Note: these chips contain a 0008 device which is incompatible with the
+	 5595. We recognize these by the presence of the listed
+	 "blacklist" PCI ID and refuse to load.
+
+   NOT SUPPORTED	PCI ID		BLACKLIST PCI ID	
+	 540		0008		0540
+	 550		0008		0550
+	5513		0008		5511
+	5581		0008		5597
+	5582		0008		5597
+	5597		0008		5597
+	5598		0008		5597/5598
+	 630		0008		0630
+	 645		0008		0645
+	 730		0008		0730
+	 735		0008		0735
+*/
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/ioport.h>
+#include <linux/pci.h>
+#include <linux/i2c.h>
+#include <linux/i2c-sensor.h>
+#include <linux/init.h>
+#include <asm/io.h>
+
+
+/* If force_addr is set to anything different from 0, we forcibly enable
+   the device at the given address. */
+static u16 force_addr;
+module_param(force_addr, ushort, 0);
+MODULE_PARM_DESC(force_addr,
+		 "Initialize the base address of the sensors");
+
+/* Addresses to scan.
+   Note that we can't determine the ISA address until we have initialized
+   our module */
+static unsigned short normal_i2c[] = { I2C_CLIENT_END };
+static unsigned int normal_isa[] = { 0x0000, I2C_CLIENT_ISA_END };
+
+/* Insmod parameters */
+SENSORS_INSMOD_1(sis5595);
+
+static int blacklist[] = {
+			PCI_DEVICE_ID_SI_540,
+			PCI_DEVICE_ID_SI_550,
+			PCI_DEVICE_ID_SI_630,
+			PCI_DEVICE_ID_SI_645,
+			PCI_DEVICE_ID_SI_730,
+			PCI_DEVICE_ID_SI_735,
+			PCI_DEVICE_ID_SI_5511, /* 5513 chip has the 0008 device but
+						  that ID shows up in other chips so we
+						  use the 5511 ID for recognition */
+			PCI_DEVICE_ID_SI_5597,
+			PCI_DEVICE_ID_SI_5598,
+			0 };
+
+/* Many SIS5595 constants specified below */
+
+/* Length of ISA address segment */
+#define SIS5595_EXTENT 8
+/* PCI Config Registers */
+#define SIS5595_REVISION_REG 0x08
+#define SIS5595_BASE_REG 0x68
+#define SIS5595_PIN_REG 0x7A
+#define SIS5595_ENABLE_REG 0x7B
+
+/* Where are the ISA address/data registers relative to the base address */
+#define SIS5595_ADDR_REG_OFFSET 5
+#define SIS5595_DATA_REG_OFFSET 6
+
+/* The SIS5595 registers */
+#define SIS5595_REG_IN_MAX(nr) (0x2b + (nr) * 2)
+#define SIS5595_REG_IN_MIN(nr) (0x2c + (nr) * 2)
+#define SIS5595_REG_IN(nr) (0x20 + (nr))
+
+#define SIS5595_REG_FAN_MIN(nr) (0x3b + (nr))
+#define SIS5595_REG_FAN(nr) (0x28 + (nr))
+
+/* On the first version of the chip, the temp registers are separate.
+   On the second version,
+   TEMP pin is shared with IN4, configured in PCI register 0x7A.
+   The registers are the same as well.
+   OVER and HYST are really MAX and MIN. */
+
+#define REV2MIN	0xb0
+#define SIS5595_REG_TEMP 	(( data->revision) >= REV2MIN) ? \
+					SIS5595_REG_IN(4) : 0x27
+#define SIS5595_REG_TEMP_OVER	(( data->revision) >= REV2MIN) ? \
+					SIS5595_REG_IN_MAX(4) : 0x39
+#define SIS5595_REG_TEMP_HYST	(( data->revision) >= REV2MIN) ? \
+					SIS5595_REG_IN_MIN(4) : 0x3a
+
+#define SIS5595_REG_CONFIG 0x40
+#define SIS5595_REG_ALARM1 0x41
+#define SIS5595_REG_ALARM2 0x42
+#define SIS5595_REG_FANDIV 0x47
+
+/* Conversions. Limit checking is only done on the TO_REG
+   variants. */
+
+/* IN: mV, (0V to 4.08V)
+   REG: 16mV/bit */
+static inline u8 IN_TO_REG(unsigned long val)
+{
+	unsigned long nval = SENSORS_LIMIT(val, 0, 4080);
+	return (nval + 8) / 16;
+}
+#define IN_FROM_REG(val) ((val) *  16)
+
+static inline u8 FAN_TO_REG(long rpm, int div)
+{
+	if (rpm <= 0)
+		return 255;
+	return SENSORS_LIMIT((1350000 + rpm * div / 2) / (rpm * div), 1, 254);
+}
+
+static inline int FAN_FROM_REG(u8 val, int div)
+{
+	return val==0 ? -1 : val==255 ? 0 : 1350000/(val*div);
+}
+
+/* TEMP: mC (-54.12C to +157.53C)
+   REG: 0.83C/bit + 52.12, two's complement  */
+static inline int TEMP_FROM_REG(s8 val)
+{
+	return val * 830 + 52120;
+}
+static inline s8 TEMP_TO_REG(int val)
+{
+	int nval = SENSORS_LIMIT(val, -54120, 157530) ;
+	return nval<0 ? (nval-5212-415)/830 : (nval-5212+415)/830;
+}
+
+/* FAN DIV: 1, 2, 4, or 8 (defaults to 2)
+   REG: 0, 1, 2, or 3 (respectively) (defaults to 1) */
+static inline u8 DIV_TO_REG(int val)
+{
+	return val==8 ? 3 : val==4 ? 2 : val==1 ? 0 : 1;
+}
+#define DIV_FROM_REG(val) (1 << (val))
+
+/* For the SIS5595, we need to keep some data in memory. That
+   data is pointed to by sis5595_list[NR]->data. The structure itself is
+   dynamically allocated, at the time when the new sis5595 client is
+   allocated. */
+struct sis5595_data {
+	struct i2c_client client;
+	struct semaphore lock;
+
+	struct semaphore update_lock;
+	char valid;		/* !=0 if following fields are valid */
+	unsigned long last_updated;	/* In jiffies */
+	char maxins;		/* == 3 if temp enabled, otherwise == 4 */
+	u8 revision;		/* Reg. value */
+
+	u8 in[5];		/* Register value */
+	u8 in_max[5];		/* Register value */
+	u8 in_min[5];		/* Register value */
+	u8 fan[2];		/* Register value */
+	u8 fan_min[2];		/* Register value */
+	s8 temp;		/* Register value */
+	s8 temp_over;		/* Register value */
+	s8 temp_hyst;		/* Register value */
+	u8 fan_div[2];		/* Register encoding, shifted right */
+	u16 alarms;		/* Register encoding, combined */
+};
+
+static struct pci_dev *s_bridge;	/* pointer to the (only) sis5595 */
+
+static int sis5595_attach_adapter(struct i2c_adapter *adapter);
+static int sis5595_detect(struct i2c_adapter *adapter, int address, int kind);
+static int sis5595_detach_client(struct i2c_client *client);
+
+static int sis5595_read_value(struct i2c_client *client, u8 register);
+static int sis5595_write_value(struct i2c_client *client, u8 register, u8 value);
+static struct sis5595_data *sis5595_update_device(struct device *dev);
+static void sis5595_init_client(struct i2c_client *client);
+static int sis5595_find_sis(int *address);
+
+static struct i2c_driver sis5595_driver = {
+	.owner		= THIS_MODULE,
+	.name		= "sis5595",
+	.id		= I2C_DRIVERID_SIS5595,
+	.flags		= I2C_DF_NOTIFY,
+	.attach_adapter	= sis5595_attach_adapter,
+	.detach_client	= sis5595_detach_client,
+};
+
+/* 4 Voltages */
+static ssize_t show_in(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", IN_FROM_REG(data->in[nr]));
+}
+
+static ssize_t show_in_min(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", IN_FROM_REG(data->in_min[nr]));
+}
+
+static ssize_t show_in_max(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", IN_FROM_REG(data->in_max[nr]));
+}
+
+static ssize_t set_in_min(struct device *dev, const char *buf,
+	       size_t count, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+	data->in_min[nr] = IN_TO_REG(val);
+	sis5595_write_value(client, SIS5595_REG_IN_MIN(nr), data->in_min[nr]);
+	return count;
+}
+
+static ssize_t set_in_max(struct device *dev, const char *buf,
+	       size_t count, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+	data->in_max[nr] = IN_TO_REG(val);
+	sis5595_write_value(client, SIS5595_REG_IN_MAX(nr), data->in_max[nr]);
+	return count;
+}
+
+#define show_in_offset(offset)					\
+static ssize_t							\
+	show_in##offset (struct device *dev, char *buf)		\
+{								\
+	return show_in(dev, buf, offset);			\
+}								\
+static DEVICE_ATTR(in##offset##_input, S_IRUGO, 		\
+		show_in##offset, NULL);				\
+static ssize_t							\
+	show_in##offset##_min (struct device *dev, char *buf)	\
+{								\
+	return show_in_min(dev, buf, offset);			\
+}								\
+static ssize_t							\
+	show_in##offset##_max (struct device *dev, char *buf)	\
+{								\
+	return show_in_max(dev, buf, offset);			\
+}								\
+static ssize_t set_in##offset##_min (struct device *dev,	\
+		const char *buf, size_t count)			\
+{								\
+	return set_in_min(dev, buf, count, offset);		\
+}								\
+static ssize_t set_in##offset##_max (struct device *dev,	\
+		const char *buf, size_t count)			\
+{								\
+	return set_in_max(dev, buf, count, offset);		\
+}								\
+static DEVICE_ATTR(in##offset##_min, S_IRUGO | S_IWUSR,		\
+		show_in##offset##_min, set_in##offset##_min);	\
+static DEVICE_ATTR(in##offset##_max, S_IRUGO | S_IWUSR,		\
+		show_in##offset##_max, set_in##offset##_max);
+
+show_in_offset(0);
+show_in_offset(1);
+show_in_offset(2);
+show_in_offset(3);
+show_in_offset(4);
+
+/* Temperature */
+static ssize_t show_temp(struct device *dev, char *buf)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp));
+}
+
+static ssize_t show_temp_over(struct device *dev, char *buf)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_over));
+}
+
+static ssize_t set_temp_over(struct device *dev, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	long val = simple_strtol(buf, NULL, 10);
+	data->temp_over = TEMP_TO_REG(val);
+	sis5595_write_value(client, SIS5595_REG_TEMP_OVER, data->temp_over);
+	return count;
+}
+
+static ssize_t show_temp_hyst(struct device *dev, char *buf)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_hyst));
+}
+
+static ssize_t set_temp_hyst(struct device *dev, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	long val = simple_strtol(buf, NULL, 10);
+	data->temp_hyst = TEMP_TO_REG(val);
+	sis5595_write_value(client, SIS5595_REG_TEMP_HYST, data->temp_hyst);
+	return count;
+}
+
+static DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL);
+static DEVICE_ATTR(temp1_max, S_IRUGO | S_IWUSR,
+		show_temp_over, set_temp_over);
+static DEVICE_ATTR(temp1_max_hyst, S_IRUGO | S_IWUSR,
+		show_temp_hyst, set_temp_hyst);
+
+/* 2 Fans */
+static ssize_t show_fan(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[nr],
+		DIV_FROM_REG(data->fan_div[nr])) );
+}
+
+static ssize_t show_fan_min(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf,"%d\n", FAN_FROM_REG(data->fan_min[nr],
+		DIV_FROM_REG(data->fan_div[nr])) );
+}
+
+static ssize_t set_fan_min(struct device *dev, const char *buf,
+		size_t count, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+	data->fan_min[nr] = FAN_TO_REG(val, DIV_FROM_REG(data->fan_div[nr]));
+	sis5595_write_value(client, SIS5595_REG_FAN_MIN(nr), data->fan_min[nr]);
+	return count;
+}
+
+static ssize_t show_fan_div(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", DIV_FROM_REG(data->fan_div[nr]) );
+}
+
+/* Note: we save and restore the fan minimum here, because its value is
+   determined in part by the fan divisor.  This follows the principle of
+   least suprise; the user doesn't expect the fan minimum to change just
+   because the divisor changed. */
+static ssize_t set_fan_div(struct device *dev, const char *buf,
+	size_t count, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	unsigned long min = FAN_FROM_REG(data->fan_min[nr],
+			DIV_FROM_REG(data->fan_div[nr]));
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+	int reg = sis5595_read_value(client, SIS5595_REG_FANDIV);
+	switch (val) {
+	case 1: data->fan_div[nr] = 0; break;
+	case 2: data->fan_div[nr] = 1; break;
+	case 4: data->fan_div[nr] = 2; break;
+	case 8: data->fan_div[nr] = 3; break;
+	default:
+		dev_err(&client->dev, "fan_div value %ld not "
+			"supported. Choose one of 1, 2, 4 or 8!\n", val);
+		return -EINVAL;
+	}
+	
+	switch (nr) {
+	case 0:
+		reg = (reg & 0xcf) | (data->fan_div[nr] << 4);
+		break;
+	case 1:
+		reg = (reg & 0x3f) | (data->fan_div[nr] << 6);
+		break;
+	}
+	sis5595_write_value(client, SIS5595_REG_FANDIV, reg);
+	data->fan_min[nr] =
+		FAN_TO_REG(min, DIV_FROM_REG(data->fan_div[nr]));
+	sis5595_write_value(client, SIS5595_REG_FAN_MIN(nr), data->fan_min[nr]);
+	return count;
+}
+
+#define show_fan_offset(offset)						\
+static ssize_t show_fan_##offset (struct device *dev, char *buf)	\
+{									\
+	return show_fan(dev, buf, offset - 1);			\
+}									\
+static ssize_t show_fan_##offset##_min (struct device *dev, char *buf)	\
+{									\
+	return show_fan_min(dev, buf, offset - 1);			\
+}									\
+static ssize_t show_fan_##offset##_div (struct device *dev, char *buf)	\
+{									\
+	return show_fan_div(dev, buf, offset - 1);			\
+}									\
+static ssize_t set_fan_##offset##_min (struct device *dev,		\
+		const char *buf, size_t count)				\
+{									\
+	return set_fan_min(dev, buf, count, offset - 1);		\
+}									\
+static DEVICE_ATTR(fan##offset##_input, S_IRUGO, show_fan_##offset, NULL);\
+static DEVICE_ATTR(fan##offset##_min, S_IRUGO | S_IWUSR,		\
+		show_fan_##offset##_min, set_fan_##offset##_min);
+
+show_fan_offset(1);
+show_fan_offset(2);
+
+static ssize_t set_fan_1_div(struct device *dev, const char *buf,
+		size_t count)
+{
+	return set_fan_div(dev, buf, count, 0) ;
+}
+
+static ssize_t set_fan_2_div(struct device *dev, const char *buf,
+		size_t count)
+{
+	return set_fan_div(dev, buf, count, 1) ;
+}
+static DEVICE_ATTR(fan1_div, S_IRUGO | S_IWUSR,
+		show_fan_1_div, set_fan_1_div);
+static DEVICE_ATTR(fan2_div, S_IRUGO | S_IWUSR,
+		show_fan_2_div, set_fan_2_div);
+
+/* Alarms */
+static ssize_t show_alarms(struct device *dev, char *buf)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", data->alarms);
+}
+static DEVICE_ATTR(alarms, S_IRUGO, show_alarms, NULL);
+ 
+/* This is called when the module is loaded */
+static int sis5595_attach_adapter(struct i2c_adapter *adapter)
+{
+	if (!(adapter->class & I2C_CLASS_HWMON))
+		return 0;
+	return i2c_detect(adapter, &addr_data, sis5595_detect);
+}
+
+/* Locate SiS bridge and correct base address for SIS5595 */
+static int sis5595_find_sis(int *address)
+{
+	u16 val;
+	int *i;
+
+	if (!(s_bridge =
+	    pci_get_device(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_503, NULL)))
+		return -ENODEV;
+
+	/* Look for imposters */
+	for(i = blacklist; *i != 0; i++) {
+		if (pci_get_device(PCI_VENDOR_ID_SI, *i, NULL)) {
+			dev_err(&s_bridge->dev, "sis5595.ko: Error: Looked for SIS5595 but found unsupported device %.4X\n", *i);
+			return -ENODEV;
+		}
+	}
+
+	if (PCIBIOS_SUCCESSFUL !=
+	    pci_read_config_word(s_bridge, SIS5595_BASE_REG, &val))
+		return -ENODEV;
+
+	*address = val & ~(SIS5595_EXTENT - 1);
+	if (*address == 0 && force_addr == 0) {
+		dev_err(&s_bridge->dev, "sis5595.ko: base address not set - upgrade BIOS or use force_addr=0xaddr\n");
+		return -ENODEV;
+	}
+	if (force_addr)
+		*address = force_addr;	/* so detect will get called */
+
+	return 0;
+}
+
+int sis5595_detect(struct i2c_adapter *adapter, int address, int kind)
+{
+	int err = 0;
+	int i;
+	struct i2c_client *new_client;
+	struct sis5595_data *data;
+	char val;
+	u16 a;
+
+	/* Make sure we are probing the ISA bus!!  */
+	if (!i2c_is_isa_adapter(adapter))
+		goto exit;
+
+	if(force_addr)
+		address = force_addr & ~(SIS5595_EXTENT - 1);
+	/* Reserve the ISA region */
+	if (!request_region(address, SIS5595_EXTENT, sis5595_driver.name)) {
+		err = -EBUSY;
+		goto exit;
+	}
+	if(force_addr) {
+		dev_warn(&adapter->dev, "forcing ISA address 0x%04X\n", address);
+		if (PCIBIOS_SUCCESSFUL !=
+		    pci_write_config_word(s_bridge, SIS5595_BASE_REG, address))
+			goto exit_release;
+		if (PCIBIOS_SUCCESSFUL !=
+		    pci_read_config_word(s_bridge, SIS5595_BASE_REG, &a))
+			goto exit_release;
+		if ((a & ~(SIS5595_EXTENT - 1)) != address)
+			/* doesn't work for some chips? */
+			goto exit_release;
+	}
+
+	if (PCIBIOS_SUCCESSFUL !=
+	    pci_read_config_byte(s_bridge, SIS5595_ENABLE_REG, &val)) {
+		goto exit_release;
+	}
+	if((val & 0x80) == 0) {
+		if (PCIBIOS_SUCCESSFUL !=
+		    pci_write_config_byte(s_bridge, SIS5595_ENABLE_REG,
+					  val | 0x80))
+			goto exit_release;
+		if (PCIBIOS_SUCCESSFUL !=
+		    pci_read_config_byte(s_bridge, SIS5595_ENABLE_REG, &val))
+			goto exit_release;
+		if((val & 0x80) == 0) 
+			/* doesn't work for some chips! */
+			goto exit_release;
+	}
+
+	if (!(data = kmalloc(sizeof(struct sis5595_data), GFP_KERNEL))) {
+		err = -ENOMEM;
+		goto exit_release;
+	}
+	memset(data, 0, sizeof(struct sis5595_data));
+
+	new_client = &data->client;
+	new_client->addr = address;
+	init_MUTEX(&data->lock);
+	i2c_set_clientdata(new_client, data);
+	new_client->adapter = adapter;
+	new_client->driver = &sis5595_driver;
+	new_client->flags = 0;
+
+	/* Check revision and pin registers to determine whether 4 or 5 voltages */
+	pci_read_config_byte(s_bridge, SIS5595_REVISION_REG, &(data->revision));
+	/* 4 voltages, 1 temp */
+	data->maxins = 3;
+	if(data->revision >= REV2MIN) {
+		pci_read_config_byte(s_bridge, SIS5595_PIN_REG, &val);
+		if (!(val & 0x80))
+			/* 5 voltages, no temps */
+			data->maxins = 4;
+	}
+	
+	/* Fill in the remaining client fields and put it into the global list */
+	strlcpy(new_client->name, "sis5595", I2C_NAME_SIZE);
+
+	data->valid = 0;
+	init_MUTEX(&data->update_lock);
+
+	/* Tell the I2C layer a new client has arrived */
+	if ((err = i2c_attach_client(new_client)))
+		goto exit_free;
+	
+	/* Initialize the SIS5595 chip */
+	sis5595_init_client(new_client);
+
+	/* A few vars need to be filled upon startup */
+	for (i = 0; i < 2; i++) {
+		data->fan_min[i] = sis5595_read_value(new_client,
+					SIS5595_REG_FAN_MIN(i));
+	}
+
+	/* Register sysfs hooks */
+	device_create_file(&new_client->dev, &dev_attr_in0_input);
+	device_create_file(&new_client->dev, &dev_attr_in0_min);
+	device_create_file(&new_client->dev, &dev_attr_in0_max);
+	device_create_file(&new_client->dev, &dev_attr_in1_input);
+	device_create_file(&new_client->dev, &dev_attr_in1_min);
+	device_create_file(&new_client->dev, &dev_attr_in1_max);
+	device_create_file(&new_client->dev, &dev_attr_in2_input);
+	device_create_file(&new_client->dev, &dev_attr_in2_min);
+	device_create_file(&new_client->dev, &dev_attr_in2_max);
+	device_create_file(&new_client->dev, &dev_attr_in3_input);
+	device_create_file(&new_client->dev, &dev_attr_in3_min);
+	device_create_file(&new_client->dev, &dev_attr_in3_max);
+	if (data->maxins == 4) {
+		device_create_file(&new_client->dev, &dev_attr_in4_input);
+		device_create_file(&new_client->dev, &dev_attr_in4_min);
+		device_create_file(&new_client->dev, &dev_attr_in4_max);
+	}
+	device_create_file(&new_client->dev, &dev_attr_fan1_input);
+	device_create_file(&new_client->dev, &dev_attr_fan1_min);
+	device_create_file(&new_client->dev, &dev_attr_fan1_div);
+	device_create_file(&new_client->dev, &dev_attr_fan2_input);
+	device_create_file(&new_client->dev, &dev_attr_fan2_min);
+	device_create_file(&new_client->dev, &dev_attr_fan2_div);
+	device_create_file(&new_client->dev, &dev_attr_alarms);
+	if (data->maxins == 3) {
+		device_create_file(&new_client->dev, &dev_attr_temp1_input);
+		device_create_file(&new_client->dev, &dev_attr_temp1_max);
+		device_create_file(&new_client->dev, &dev_attr_temp1_max_hyst);
+	}
+	return 0;
+	
+exit_free:
+	kfree(data);
+exit_release:
+	release_region(address, SIS5595_EXTENT);
+exit:
+	return err;
+}
+
+static int sis5595_detach_client(struct i2c_client *client)
+{
+	int err;
+
+	if ((err = i2c_detach_client(client))) {
+		dev_err(&client->dev,
+		    "Client deregistration failed, client not detached.\n");
+		return err;
+	}
+
+	if(i2c_is_isa_client(client))
+		release_region(client->addr, SIS5595_EXTENT);
+
+	kfree(i2c_get_clientdata(client));
+
+	return 0;
+}
+
+
+/* ISA access must be locked explicitly. */
+static int sis5595_read_value(struct i2c_client *client, u8 reg)
+{
+	int res;
+
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	down(&data->lock);
+	outb_p(reg, client->addr + SIS5595_ADDR_REG_OFFSET);
+	res = inb_p(client->addr + SIS5595_DATA_REG_OFFSET);
+	up(&data->lock);
+	return res;
+}
+
+static int sis5595_write_value(struct i2c_client *client, u8 reg, u8 value)
+{
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	down(&data->lock);
+	outb_p(reg, client->addr + SIS5595_ADDR_REG_OFFSET);
+	outb_p(value, client->addr + SIS5595_DATA_REG_OFFSET);
+	up(&data->lock);
+	return 0;
+}
+
+/* Called when we have found a new SIS5595. */
+static void sis5595_init_client(struct i2c_client *client)
+{
+	u8 config = sis5595_read_value(client, SIS5595_REG_CONFIG);
+	if (!(config & 0x01))
+		sis5595_write_value(client, SIS5595_REG_CONFIG,
+				(config & 0xf7) | 0x01);
+}
+
+static struct sis5595_data *sis5595_update_device(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	int i;
+
+	down(&data->update_lock);
+
+	if ((jiffies - data->last_updated > HZ + HZ / 2) ||
+	    (jiffies < data->last_updated) || !data->valid) {
+
+		for (i = 0; i <= data->maxins; i++) {
+			data->in[i] =
+			    sis5595_read_value(client, SIS5595_REG_IN(i));
+			data->in_min[i] =
+			    sis5595_read_value(client,
+					       SIS5595_REG_IN_MIN(i));
+			data->in_max[i] =
+			    sis5595_read_value(client,
+					       SIS5595_REG_IN_MAX(i));
+		}
+		for (i = 0; i < 2; i++) {
+			data->fan[i] =
+			    sis5595_read_value(client, SIS5595_REG_FAN(i));
+			data->fan_min[i] =
+			    sis5595_read_value(client,
+					       SIS5595_REG_FAN_MIN(i));
+		}
+		if(data->maxins == 3) {
+			data->temp =
+			    sis5595_read_value(client, SIS5595_REG_TEMP);
+			data->temp_over =
+			    sis5595_read_value(client, SIS5595_REG_TEMP_OVER);
+			data->temp_hyst =
+			    sis5595_read_value(client, SIS5595_REG_TEMP_HYST);
+		}
+		i = sis5595_read_value(client, SIS5595_REG_FANDIV);
+		data->fan_div[0] = (i >> 4) & 0x03;
+		data->fan_div[1] = i >> 6;
+		data->alarms =
+		    sis5595_read_value(client, SIS5595_REG_ALARM1) |
+		    (sis5595_read_value(client, SIS5595_REG_ALARM2) << 8);
+		data->last_updated = jiffies;
+		data->valid = 1;
+	}
+
+	up(&data->update_lock);
+
+	return data;
+}
+
+
+
+static int __init sm_sis5595_init(void)
+{
+	int addr;
+
+	if (sis5595_find_sis(&addr))
+		return -ENODEV;
+	normal_isa[0] = addr;
+
+	return i2c_add_driver(&sis5595_driver);
+}
+
+static void __exit sm_sis5595_exit(void)
+{
+	i2c_del_driver(&sis5595_driver);
+}
+
+
+
+MODULE_AUTHOR("Aurelien Jarno <aurelien@aurel32.net>");
+MODULE_DESCRIPTION("SiS 5595 Sensor device");
+MODULE_LICENSE("GPL");
+
+module_init(sm_sis5595_init);
+module_exit(sm_sis5595_exit);

-- 
  .''`.  Aurelien Jarno	              GPG: 1024D/F1BCDB73
 : :' :  Debian GNU/Linux developer | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [PATCH 2.6] I2C: New chip driver: sis5595

[Jean Delvare]

Hi Greg & all,

> > +/* Locate SiS bridge and correct base address for SIS5595 */
> > +static int sis5595_find_sis(int *address)
> > +{
> > +	u16 val;
> > +	int *i;
> > +
> > +	if (!(s_bridge =
> > +	    pci_get_device(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_503, NULL)))
> > +		return -ENODEV;
>
> You never free the reference you grabbed on the pci_dev here.  Please
> read the docs on how pci_get_device() is to be used, it isn't just a
> drop in replacement for pci_find_device(), sorry.
>
> > +	/* Look for imposters */
> > +	for(i = blacklist; *i != 0; i++) {
> > +		if (pci_get_device(PCI_VENDOR_ID_SI, *i, NULL)) {
>
> Same here, you are leaking a reference count.
>
> Why are you not using the pci driver interface instead?

Not sure it exactly answers your question, but the sis5595 much
ressembles the via686a in that respect, i.e. it isn't driving a PCI
device, merely grabbing the ISA I/O address data from PCI configuration
space, then driving that ISA device. I'd guess that whatever was done
in via686a (including the recent updates) should work equally well for
the sis5595.

Thanks,
--
Jean Delvare

Re: [PATCH 2.6] I2C: New chip driver: sis5595

[Alexey Dobriyan]

On Tuesday 01 February 2005 15:52, Jean Delvare wrote:

> > What about making sis5595_update_device() a simple jiffies-related wrapper
> > around function that updates "struct sis5595" unconditionally. I'm not sure
> > I plugged sis5595_do_update_client right, but you'll get the idea.

> 1* It forces a chip update (i.e. full register read) on driver load (or
> more precisely on client detection). Since I2C/SMBus accesses are really
> slow, it will result in a significant time penalty. As the read values
> are only considered valid for 1.5 second (or equivalent duration for the
> other drivers), this penalty brings statistically no benefit in return.
> 
> 2* Each subsequent update (or attempt thereof) will conditionally require
> an additional function call, which represents a small time penalty again
> (much more than a comparison if I'm not mistaken).

lm90 (all sensors chips have approximately the same speed, right?) spec says:
"It takes the LM90 31.25 ms to measure the temperature of the remote diod and
internal diode". Google says: "The buses [I2C, SMBus] operate at the same
speed, up to 100kHz, but the I2C bus has both 400kHz and 2MHz versions."

What I'm trying to say that you shouldn't care about s/cmp; jcc/call/ if the
actual measurement is infinite from CPU's POV.

> We are only trying to avoid a conditional test and to get rid of a local
> variable, and end up with a much slower init

Well, yes, I agree that initialization will become slower. But how long is
register read (from first outb command to the moment when CPU sees it)?
I'm trying to build time hierarchy of things we're discussing in my head.

	Alexey

Re: [PATCH 2.6] I2C: New chip driver: sis5595

[Greg KH]

On Tue, Feb 01, 2005 at 01:14:14PM +0100, Aurelien Jarno wrote:
> +	if(force_addr)

Please put a space after the "if" and before the "(".  You do this in a
number of places.

thanks,

greg k-h

Re: [PATCH 2.6] I2C: New chip driver: sis5595

[Greg KH]

On Tue, Feb 01, 2005 at 01:14:14PM +0100, Aurelien Jarno wrote:
> +/* Locate SiS bridge and correct base address for SIS5595 */
> +static int sis5595_find_sis(int *address)
> +{
> +	u16 val;
> +	int *i;
> +
> +	if (!(s_bridge =
> +	    pci_get_device(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_503, NULL)))
> +		return -ENODEV;

You never free the reference you grabbed on the pci_dev here.  Please
read the docs on how pci_get_device() is to be used, it isn't just a
drop in replacement for pci_find_device(), sorry.

> +	/* Look for imposters */
> +	for(i = blacklist; *i != 0; i++) {
> +		if (pci_get_device(PCI_VENDOR_ID_SI, *i, NULL)) {

Same here, you are leaking a reference count.

Why are you not using the pci driver interface instead?

thanks,

greg k-h

Re: [PATCH 2.6] I2C: New chip driver: sis5595

[Jean Delvare]

Hi Alexey,

> lm90 (all sensors chips have approximately the same speed, right?) spec
> says:

Absolutely not, it depends on the raw speed (internal clock) and the
number of channels. But in fact it doesn't matter, see below.

> "It takes the LM90 31.25 ms to measure the temperature of the remote diode
> and internal diode".

This is completely unrelated to the time it takes to read the registers.
The temperature (or other) measurements are done in the backgroud,
continuously, by the hardware monitoring chips.

> Google says: "The buses [I2C, SMBus] operate at the same
> speed, up to 100kHz, but the I2C bus has both 400kHz and 2MHz versions."

And this is what matters, because this tells you how much time it takes
to read one register. Multiply by the number of registers you need to
read (from 4 to 50, depends on the chip) and you have an approximative
total of time required by the update function, when the jiffies test
succeeds.

In other words, what matters in terms of speed is the SMBus the chip is
connected to, not the speed of the chip itself. Typical speed for
motherboard SMBus is 16kHz.

Let's look at how things work. These hardware monitoring chip drivers
export files to sysfs, which map more or less directly to the registers.
When one reads the sysfs file, the driver will attempt to read all
registers, cache all values and return the requested one. If an update
occured recently (typically less than 2 seconds ago) then the cached
value is directly returned instead. The reason why this is implemented
that way is that most chips stop monitoring when they receive read
requests. If we allowed registers to be read individually and without
caching the results, chips could possibly be kept busy forever, which
would be dangerous. We also want to avoid SMBus saturation.

Typically, people will read all sysfs files in a row (by running
"sensors" or any other hardware monitoring tool). This means that, for
a given read series:
1* The first sysfs file read will most probably trigger an update.
2* All the other sysfs file reads will hit the cache.

> What I'm trying to say that you shouldn't care about s/cmp; jcc/call/ if
> the actual measurement is infinite from CPU's POV.

That's absolutely right, optimizing the case in which the update *will*
occur (case 1* above) is worthless because it only occurs once in a
series, and is inherently slow. However, we still want to make the cache
hits (case 2* above) as fast as possible (IMHO at least) because it can
happen several dozen times in a series.

Your proposal actually slows down 1* (additional function call) but not
2*, so I'm fine with that. However, it also implies a mandatory update
at init time, which may take several seconds with no benefit, and I am
*not* fine with that.

> Well, yes, I agree that initialization will become slower. But how long is
> register read (from first outb command to the moment when CPU sees it)?
> I'm trying to build time hierarchy of things we're discussing in my head.

It's not outb/inb but (in the most common case)
i2c_smbus_read_byte_data. It *is* damn slow in comparison with any other
command, I/O or not. This is why I don't want to add any at init time
for no reason.

Thanks,
--
Jean Delvare

Re: [PATCH 2.6] I2C: New chip driver: sis5595

[Jean Delvare]

Quoting myself:

> To me, the only acceptable simplification is
> the initialization of "last_updated" to something which ensures that
> the first update attempt will succeed - providing we actually can do
> that.

On second thought, we obviously cannot, because jiffies wrap, so there is
no single initial value of "last_updated", either relative or
absolute, which can ensure this condition to be true. I think we are
stuck we this "valid" flag, or at least with the concept thereof.
Possibly we can use "last_updated" itself as a flag if we absolutely
want to get rid of "valid". "last_updated == 0" would mean the same
as "valid == 0" did. The probability of "last_updated" to become 0
again after init time is obviously thin, and it wouldn't really hurt if
it did (it would simply allow an extra update to happen). That said,
this makes the code somewhat trickier.

What could (and should) be done anyway is to use time_after() or
something equivalent for the jiffies checks, instead of direct
coparison, in all hardware monitoring drivers.

Thanks,
--
Jean Delvare

Re: [PATCH 2.6] I2C: New chip driver: sis5595

[Alexey Dobriyan]

On Tuesday 01 February 2005 13:49, Jean Delvare wrote:

> > Maybe you should call sis5595_update_device() in initialization function
> > and get rid of "value" field. It's sole purpose to fill "struct sis5595"
> > when it's known that "last_updated" field contains crap.

> Doesn't work. If you discard the "valid" field and call the update
> function at init time, you cannot be sure that the update function will
> actually do something. It all depends on the jiffies value relative to
> last_updated (0 at this point). This is exactly what "valid" is there
> for.

What about making sis5595_update_device() a simple jiffies-related wrapper
around function that updates "struct sis5595" unconditionally. I'm not sure
I plugged sis5595_do_update_client right, but you'll get the idea.

	Alexey

--- linux-2.6.11-rc2-bk9-i2c/drivers/i2c/chips/sis5595.c.orig	2005-02-01 15:34:43.000000000 +0200
+++ linux-2.6.11-rc2-bk9-i2c/drivers/i2c/chips/sis5595.c	2005-02-01 15:58:39.000000000 +0200
@@ -184,7 +184,6 @@ struct sis5595_data {
 	struct semaphore lock;
 
 	struct semaphore update_lock;
-	char valid;		/* !=0 if following fields are valid */
 	unsigned long last_updated;	/* In jiffies */
 	char maxins;		/* == 3 if temp enabled, otherwise == 4 */
 	u8 revision;		/* Reg. value */
@@ -209,6 +208,7 @@ static int sis5595_detach_client(struct 
 
 static int sis5595_read_value(struct i2c_client *client, u8 register);
 static int sis5595_write_value(struct i2c_client *client, u8 register, u8 value);
+static void sis5595_do_update_client(struct i2c_client *client);
 static struct sis5595_data *sis5595_update_device(struct device *dev);
 static void sis5595_init_client(struct i2c_client *client);
 static int sis5595_find_sis(int *address);
@@ -586,7 +586,6 @@ int sis5595_detect(struct i2c_adapter *a
 	/* Fill in the remaining client fields and put it into the global list */
 	strlcpy(new_client->name, "sis5595", I2C_NAME_SIZE);
 
-	data->valid = 0;
 	init_MUTEX(&data->update_lock);
 
 	/* Tell the I2C layer a new client has arrived */
@@ -691,56 +690,52 @@ static void sis5595_init_client(struct i
 	if (!(config & 0x01))
 		sis5595_write_value(client, SIS5595_REG_CONFIG,
 				(config & 0xf7) | 0x01);
+	sis5595_do_update_client(client);
 }
 
-static struct sis5595_data *sis5595_update_device(struct device *dev)
+static void sis5595_do_update_client(struct i2c_client *client)
 {
-	struct i2c_client *client = to_i2c_client(dev);
 	struct sis5595_data *data = i2c_get_clientdata(client);
 	int i;
 
 	down(&data->update_lock);
-
-	if ((jiffies - data->last_updated > HZ + HZ / 2) ||
-	    (jiffies < data->last_updated) || !data->valid) {
-
-		for (i = 0; i <= data->maxins; i++) {
-			data->in[i] =
-			    sis5595_read_value(client, SIS5595_REG_IN(i));
-			data->in_min[i] =
-			    sis5595_read_value(client,
-					       SIS5595_REG_IN_MIN(i));
-			data->in_max[i] =
-			    sis5595_read_value(client,
-					       SIS5595_REG_IN_MAX(i));
-		}
-		for (i = 0; i < 2; i++) {
-			data->fan[i] =
-			    sis5595_read_value(client, SIS5595_REG_FAN(i));
-			data->fan_min[i] =
-			    sis5595_read_value(client,
-					       SIS5595_REG_FAN_MIN(i));
-		}
-		if(data->maxins == 3) {
-			data->temp =
-			    sis5595_read_value(client, SIS5595_REG_TEMP);
-			data->temp_over =
-			    sis5595_read_value(client, SIS5595_REG_TEMP_OVER);
-			data->temp_hyst =
-			    sis5595_read_value(client, SIS5595_REG_TEMP_HYST);
-		}
-		i = sis5595_read_value(client, SIS5595_REG_FANDIV);
-		data->fan_div[0] = (i >> 4) & 0x03;
-		data->fan_div[1] = i >> 6;
-		data->alarms =
-		    sis5595_read_value(client, SIS5595_REG_ALARM1) |
-		    (sis5595_read_value(client, SIS5595_REG_ALARM2) << 8);
-		data->last_updated = jiffies;
-		data->valid = 1;
+	for (i = 0; i <= data->maxins; i++) {
+		data->in[i] = sis5595_read_value(client, SIS5595_REG_IN(i));
+		data->in_min[i] = sis5595_read_value(client,
+							SIS5595_REG_IN_MIN(i));
+		data->in_max[i] = sis5595_read_value(client,
+							SIS5595_REG_IN_MAX(i));
 	}
-
+	for (i = 0; i < 2; i++) {
+		data->fan[i] = sis5595_read_value(client, SIS5595_REG_FAN(i));
+		data->fan_min[i] = sis5595_read_value(client,
+							SIS5595_REG_FAN_MIN(i));
+	}
+	if (data->maxins == 3) {
+		data->temp = sis5595_read_value(client, SIS5595_REG_TEMP);
+		data->temp_over = sis5595_read_value(client,
+							SIS5595_REG_TEMP_OVER);
+		data->temp_hyst = sis5595_read_value(client,
+							SIS5595_REG_TEMP_HYST);
+	}
+	i = sis5595_read_value(client, SIS5595_REG_FANDIV);
+	data->fan_div[0] = (i >> 4) & 0x03;
+	data->fan_div[1] = i >> 6;
+	data->alarms = sis5595_read_value(client, SIS5595_REG_ALARM1) |
+			(sis5595_read_value(client, SIS5595_REG_ALARM2) << 8);
+	data->last_updated = jiffies;
 	up(&data->update_lock);
+}
+
+static struct sis5595_data *sis5595_update_device(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
 
+	if ((jiffies - data->last_updated > HZ + HZ / 2) ||
+	    (jiffies < data->last_updated)) {
+		sis5595_do_update_client(client);
+	}
 	return data;
 }
 

Re: [PATCH 2.6] I2C: New chip driver: sis5595

[Jean Delvare]

Hi Alexey,

> What about making sis5595_update_device() a simple jiffies-related wrapper
> around function that updates "struct sis5595" unconditionally. I'm not sure
> I plugged sis5595_do_update_client right, but you'll get the idea.

Yeah I get the idea, I think I had something similar in mind some times
ago but it failed to please me for the following reasons:

1* It forces a chip update (i.e. full register read) on driver load (or
more precisely on client detection). Since I2C/SMBus accesses are really
slow, it will result in a significant time penalty. As the read values
are only considered valid for 1.5 second (or equivalent duration for the
other drivers), this penalty brings statistically no benefit in return.

2* Each subsequent update (or attempt thereof) will conditionally require
an additional function call, which represents a small time penalty again
(much more than a comparison if I'm not mistaken).

We are only trying to avoid a conditional test and to get rid of a local
variable, and end up with a much slower init and an additional function
call later. The loss seems to overweight the gain, don't you think?

So I wouldn't go that way. To me, the only acceptable simplification is
the initialization of "last_updated" to something which ensures that
the first update attempt will succeed - providing we actually can do
that.

Now I wonder, this is certainly not the only drivers in the kernel which
need to do this, right? Didn't the other ones solve the issue already?
Would be worth taking a look.

Thanks,
--
Jean Delvare

Re: [PATCH 2.6] I2C: New chip driver: sis5595

[Alexey Dobriyan]

On Tue, 1 Feb 2005 11:11:35 +0100, Aurelien Jarno wrote:

> Please find below the new version of the patch against kernel
> 2.6.11-rc2-mm2 to add the sis5595 driver (sensor part).

> --- linux-2.6.11-rc2-mm2.orig/drivers/i2c/chips/sis5595.c
> +++ linux-2.6.11-rc2-mm2/drivers/i2c/chips/sis5595.c

> +struct sis5595_data {

> +	char valid;		/* !=0 if following fields are valid */

> +};

> +static struct sis5595_data *sis5595_update_device(struct device *dev)
> +{

> +	if ((jiffies - data->last_updated > HZ + HZ / 2) ||
> +	    (jiffies < data->last_updated) || !data->valid) {

		[snip reading some values]

> +		data->last_updated = jiffies;
> +		data->valid = 1;
> +	}

> +}

Maybe you should call sis5595_update_device() in initialization finction and
get rid of "value" field. It's sole purpose to fill "struct sis5595" when it's
known that "last_updated" field contains crap.

> +			dev_err(&s_bridge->dev, "sis5595.ko: Error: Looked for SIS5595 but found unsupported device %.4X\n", *i);

> +		dev_err(&s_bridge->dev, "sis5595.ko: base address not set - upgrade BIOS or use force_addr=0xaddr\n");

".ko" isn't needed. "Error: " in the first line too.

	Alexey

Re: [PATCH 2.6] I2C: New chip driver: sis5595

[Aurelien Jarno]

On Tue, Feb 01, 2005 at 02:20:17PM +0200, Alexey Dobriyan wrote:
> > +			dev_err(&s_bridge->dev, "sis5595.ko: Error: Looked for SIS5595 but found unsupported device %.4X\n", *i);
> 
> > +		dev_err(&s_bridge->dev, "sis5595.ko: base address not set - upgrade BIOS or use force_addr=0xaddr\n");
> 
> ".ko" isn't needed. "Error: " in the first line too.
 
You're right. So greg, here is the new patch, that fixes that.

Please apply.

Thanks,
Aurelien

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

diff -urN linux-2.6.11-rc2-mm2.orig/drivers/i2c/chips/Kconfig linux-2.6.11-rc2-mm2/drivers/i2c/chips/Kconfig
--- linux-2.6.11-rc2-mm2.orig/drivers/i2c/chips/Kconfig	2005-02-01 07:39:24.000000000 +0100
+++ linux-2.6.11-rc2-mm2/drivers/i2c/chips/Kconfig	2005-02-01 07:40:55.000000000 +0100
@@ -262,6 +262,18 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called smsc47b397.
 
+config SENSORS_SIS5595
+	tristate "Silicon Integrated Systems Corp. SiS5595"
+	depends on I2C && PCI && EXPERIMENTAL
+	select I2C_SENSOR
+	select I2C_ISA
+	help
+	  If you say yes here you get support for the integrated sensors in
+	  SiS5595 South Bridges.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called sis5595.
+
 config SENSORS_SMSC47M1
 	tristate "SMSC LPC47M10x and compatibles"
 	depends on I2C && EXPERIMENTAL
diff -urN linux-2.6.11-rc2-mm2.orig/drivers/i2c/chips/Makefile linux-2.6.11-rc2-mm2/drivers/i2c/chips/Makefile
--- linux-2.6.11-rc2-mm2.orig/drivers/i2c/chips/Makefile	2005-02-01 07:39:24.000000000 +0100
+++ linux-2.6.11-rc2-mm2/drivers/i2c/chips/Makefile	2005-02-01 07:40:55.000000000 +0100
@@ -31,6 +31,7 @@
 obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
 obj-$(CONFIG_SENSORS_RTC8564)	+= rtc8564.o
+obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
 obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
 obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
 obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
diff -urN linux-2.6.11-rc2-mm2.orig/drivers/i2c/chips/sis5595.c linux-2.6.11-rc2-mm2/drivers/i2c/chips/sis5595.c
--- linux-2.6.11-rc2-mm2.orig/drivers/i2c/chips/sis5595.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.11-rc2-mm2/drivers/i2c/chips/sis5595.c	2005-02-01 07:40:55.000000000 +0100
@@ -0,0 +1,772 @@
+/*
+    sis5595.c - Part of lm_sensors, Linux kernel modules
+		for hardware monitoring
+
+    Copyright (C) 1998 - 2001 Frodo Looijaard <frodol@dds.nl>,
+			Kyösti Mälkki <kmalkki@cc.hut.fi>, and
+			Mark D. Studebaker <mdsxyz123@yahoo.com>
+    Ported to Linux 2.6 by Aurelien Jarno <aurelien@aurel32.net> with
+    the help of Jean Delvare <khali@linux-fr.org>
+
+    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; either version 2 of the License, or
+    (at your option) any later version.
+
+    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., 675 Mass Ave, Cambridge, MA 02139, USA.
+*/
+
+/*
+   SiS southbridge has a LM78-like chip integrated on the same IC.
+   This driver is a customized copy of lm78.c
+   
+   Supports following revisions:
+	Version		PCI ID		PCI Revision
+	1		1039/0008	AF or less
+	2		1039/0008	B0 or greater
+
+   Note: these chips contain a 0008 device which is incompatible with the
+	 5595. We recognize these by the presence of the listed
+	 "blacklist" PCI ID and refuse to load.
+
+   NOT SUPPORTED	PCI ID		BLACKLIST PCI ID	
+	 540		0008		0540
+	 550		0008		0550
+	5513		0008		5511
+	5581		0008		5597
+	5582		0008		5597
+	5597		0008		5597
+	5598		0008		5597/5598
+	 630		0008		0630
+	 645		0008		0645
+	 730		0008		0730
+	 735		0008		0735
+*/
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/ioport.h>
+#include <linux/pci.h>
+#include <linux/i2c.h>
+#include <linux/i2c-sensor.h>
+#include <linux/init.h>
+#include <asm/io.h>
+
+
+/* If force_addr is set to anything different from 0, we forcibly enable
+   the device at the given address. */
+static u16 force_addr;
+module_param(force_addr, ushort, 0);
+MODULE_PARM_DESC(force_addr,
+		 "Initialize the base address of the sensors");
+
+/* Addresses to scan.
+   Note that we can't determine the ISA address until we have initialized
+   our module */
+static unsigned short normal_i2c[] = { I2C_CLIENT_END };
+static unsigned int normal_isa[] = { 0x0000, I2C_CLIENT_ISA_END };
+
+/* Insmod parameters */
+SENSORS_INSMOD_1(sis5595);
+
+static int blacklist[] = {
+			PCI_DEVICE_ID_SI_540,
+			PCI_DEVICE_ID_SI_550,
+			PCI_DEVICE_ID_SI_630,
+			PCI_DEVICE_ID_SI_645,
+			PCI_DEVICE_ID_SI_730,
+			PCI_DEVICE_ID_SI_735,
+			PCI_DEVICE_ID_SI_5511, /* 5513 chip has the 0008 device but
+						  that ID shows up in other chips so we
+						  use the 5511 ID for recognition */
+			PCI_DEVICE_ID_SI_5597,
+			PCI_DEVICE_ID_SI_5598,
+			0 };
+
+/* Many SIS5595 constants specified below */
+
+/* Length of ISA address segment */
+#define SIS5595_EXTENT 8
+/* PCI Config Registers */
+#define SIS5595_REVISION_REG 0x08
+#define SIS5595_BASE_REG 0x68
+#define SIS5595_PIN_REG 0x7A
+#define SIS5595_ENABLE_REG 0x7B
+
+/* Where are the ISA address/data registers relative to the base address */
+#define SIS5595_ADDR_REG_OFFSET 5
+#define SIS5595_DATA_REG_OFFSET 6
+
+/* The SIS5595 registers */
+#define SIS5595_REG_IN_MAX(nr) (0x2b + (nr) * 2)
+#define SIS5595_REG_IN_MIN(nr) (0x2c + (nr) * 2)
+#define SIS5595_REG_IN(nr) (0x20 + (nr))
+
+#define SIS5595_REG_FAN_MIN(nr) (0x3b + (nr))
+#define SIS5595_REG_FAN(nr) (0x28 + (nr))
+
+/* On the first version of the chip, the temp registers are separate.
+   On the second version,
+   TEMP pin is shared with IN4, configured in PCI register 0x7A.
+   The registers are the same as well.
+   OVER and HYST are really MAX and MIN. */
+
+#define REV2MIN	0xb0
+#define SIS5595_REG_TEMP 	(( data->revision) >= REV2MIN) ? \
+					SIS5595_REG_IN(4) : 0x27
+#define SIS5595_REG_TEMP_OVER	(( data->revision) >= REV2MIN) ? \
+					SIS5595_REG_IN_MAX(4) : 0x39
+#define SIS5595_REG_TEMP_HYST	(( data->revision) >= REV2MIN) ? \
+					SIS5595_REG_IN_MIN(4) : 0x3a
+
+#define SIS5595_REG_CONFIG 0x40
+#define SIS5595_REG_ALARM1 0x41
+#define SIS5595_REG_ALARM2 0x42
+#define SIS5595_REG_FANDIV 0x47
+
+/* Conversions. Limit checking is only done on the TO_REG
+   variants. */
+
+/* IN: mV, (0V to 4.08V)
+   REG: 16mV/bit */
+static inline u8 IN_TO_REG(unsigned long val)
+{
+	unsigned long nval = SENSORS_LIMIT(val, 0, 4080);
+	return (nval + 8) / 16;
+}
+#define IN_FROM_REG(val) ((val) *  16)
+
+static inline u8 FAN_TO_REG(long rpm, int div)
+{
+	if (rpm <= 0)
+		return 255;
+	return SENSORS_LIMIT((1350000 + rpm * div / 2) / (rpm * div), 1, 254);
+}
+
+static inline int FAN_FROM_REG(u8 val, int div)
+{
+	return val==0 ? -1 : val==255 ? 0 : 1350000/(val*div);
+}
+
+/* TEMP: mC (-54.12C to +157.53C)
+   REG: 0.83C/bit + 52.12, two's complement  */
+static inline int TEMP_FROM_REG(s8 val)
+{
+	return val * 830 + 52120;
+}
+static inline s8 TEMP_TO_REG(int val)
+{
+	int nval = SENSORS_LIMIT(val, -54120, 157530) ;
+	return nval<0 ? (nval-5212-415)/830 : (nval-5212+415)/830;
+}
+
+/* FAN DIV: 1, 2, 4, or 8 (defaults to 2)
+   REG: 0, 1, 2, or 3 (respectively) (defaults to 1) */
+static inline u8 DIV_TO_REG(int val)
+{
+	return val==8 ? 3 : val==4 ? 2 : val==1 ? 0 : 1;
+}
+#define DIV_FROM_REG(val) (1 << (val))
+
+/* For the SIS5595, we need to keep some data in memory. That
+   data is pointed to by sis5595_list[NR]->data. The structure itself is
+   dynamically allocated, at the time when the new sis5595 client is
+   allocated. */
+struct sis5595_data {
+	struct i2c_client client;
+	struct semaphore lock;
+
+	struct semaphore update_lock;
+	char valid;		/* !=0 if following fields are valid */
+	unsigned long last_updated;	/* In jiffies */
+	char maxins;		/* == 3 if temp enabled, otherwise == 4 */
+	u8 revision;		/* Reg. value */
+
+	u8 in[5];		/* Register value */
+	u8 in_max[5];		/* Register value */
+	u8 in_min[5];		/* Register value */
+	u8 fan[2];		/* Register value */
+	u8 fan_min[2];		/* Register value */
+	s8 temp;		/* Register value */
+	s8 temp_over;		/* Register value */
+	s8 temp_hyst;		/* Register value */
+	u8 fan_div[2];		/* Register encoding, shifted right */
+	u16 alarms;		/* Register encoding, combined */
+};
+
+static struct pci_dev *s_bridge;	/* pointer to the (only) sis5595 */
+
+static int sis5595_attach_adapter(struct i2c_adapter *adapter);
+static int sis5595_detect(struct i2c_adapter *adapter, int address, int kind);
+static int sis5595_detach_client(struct i2c_client *client);
+
+static int sis5595_read_value(struct i2c_client *client, u8 register);
+static int sis5595_write_value(struct i2c_client *client, u8 register, u8 value);
+static struct sis5595_data *sis5595_update_device(struct device *dev);
+static void sis5595_init_client(struct i2c_client *client);
+static int sis5595_find_sis(int *address);
+
+static struct i2c_driver sis5595_driver = {
+	.owner		= THIS_MODULE,
+	.name		= "sis5595",
+	.id		= I2C_DRIVERID_SIS5595,
+	.flags		= I2C_DF_NOTIFY,
+	.attach_adapter	= sis5595_attach_adapter,
+	.detach_client	= sis5595_detach_client,
+};
+
+/* 4 Voltages */
+static ssize_t show_in(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", IN_FROM_REG(data->in[nr]));
+}
+
+static ssize_t show_in_min(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", IN_FROM_REG(data->in_min[nr]));
+}
+
+static ssize_t show_in_max(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", IN_FROM_REG(data->in_max[nr]));
+}
+
+static ssize_t set_in_min(struct device *dev, const char *buf,
+	       size_t count, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+	data->in_min[nr] = IN_TO_REG(val);
+	sis5595_write_value(client, SIS5595_REG_IN_MIN(nr), data->in_min[nr]);
+	return count;
+}
+
+static ssize_t set_in_max(struct device *dev, const char *buf,
+	       size_t count, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+	data->in_max[nr] = IN_TO_REG(val);
+	sis5595_write_value(client, SIS5595_REG_IN_MAX(nr), data->in_max[nr]);
+	return count;
+}
+
+#define show_in_offset(offset)					\
+static ssize_t							\
+	show_in##offset (struct device *dev, char *buf)		\
+{								\
+	return show_in(dev, buf, offset);			\
+}								\
+static DEVICE_ATTR(in##offset##_input, S_IRUGO, 		\
+		show_in##offset, NULL);				\
+static ssize_t							\
+	show_in##offset##_min (struct device *dev, char *buf)	\
+{								\
+	return show_in_min(dev, buf, offset);			\
+}								\
+static ssize_t							\
+	show_in##offset##_max (struct device *dev, char *buf)	\
+{								\
+	return show_in_max(dev, buf, offset);			\
+}								\
+static ssize_t set_in##offset##_min (struct device *dev,	\
+		const char *buf, size_t count)			\
+{								\
+	return set_in_min(dev, buf, count, offset);		\
+}								\
+static ssize_t set_in##offset##_max (struct device *dev,	\
+		const char *buf, size_t count)			\
+{								\
+	return set_in_max(dev, buf, count, offset);		\
+}								\
+static DEVICE_ATTR(in##offset##_min, S_IRUGO | S_IWUSR,		\
+		show_in##offset##_min, set_in##offset##_min);	\
+static DEVICE_ATTR(in##offset##_max, S_IRUGO | S_IWUSR,		\
+		show_in##offset##_max, set_in##offset##_max);
+
+show_in_offset(0);
+show_in_offset(1);
+show_in_offset(2);
+show_in_offset(3);
+show_in_offset(4);
+
+/* Temperature */
+static ssize_t show_temp(struct device *dev, char *buf)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp));
+}
+
+static ssize_t show_temp_over(struct device *dev, char *buf)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_over));
+}
+
+static ssize_t set_temp_over(struct device *dev, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	long val = simple_strtol(buf, NULL, 10);
+	data->temp_over = TEMP_TO_REG(val);
+	sis5595_write_value(client, SIS5595_REG_TEMP_OVER, data->temp_over);
+	return count;
+}
+
+static ssize_t show_temp_hyst(struct device *dev, char *buf)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_hyst));
+}
+
+static ssize_t set_temp_hyst(struct device *dev, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	long val = simple_strtol(buf, NULL, 10);
+	data->temp_hyst = TEMP_TO_REG(val);
+	sis5595_write_value(client, SIS5595_REG_TEMP_HYST, data->temp_hyst);
+	return count;
+}
+
+static DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL);
+static DEVICE_ATTR(temp1_max, S_IRUGO | S_IWUSR,
+		show_temp_over, set_temp_over);
+static DEVICE_ATTR(temp1_max_hyst, S_IRUGO | S_IWUSR,
+		show_temp_hyst, set_temp_hyst);
+
+/* 2 Fans */
+static ssize_t show_fan(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[nr],
+		DIV_FROM_REG(data->fan_div[nr])) );
+}
+
+static ssize_t show_fan_min(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf,"%d\n", FAN_FROM_REG(data->fan_min[nr],
+		DIV_FROM_REG(data->fan_div[nr])) );
+}
+
+static ssize_t set_fan_min(struct device *dev, const char *buf,
+		size_t count, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+	data->fan_min[nr] = FAN_TO_REG(val, DIV_FROM_REG(data->fan_div[nr]));
+	sis5595_write_value(client, SIS5595_REG_FAN_MIN(nr), data->fan_min[nr]);
+	return count;
+}
+
+static ssize_t show_fan_div(struct device *dev, char *buf, int nr)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", DIV_FROM_REG(data->fan_div[nr]) );
+}
+
+/* Note: we save and restore the fan minimum here, because its value is
+   determined in part by the fan divisor.  This follows the principle of
+   least suprise; the user doesn't expect the fan minimum to change just
+   because the divisor changed. */
+static ssize_t set_fan_div(struct device *dev, const char *buf,
+	size_t count, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	unsigned long min = FAN_FROM_REG(data->fan_min[nr],
+			DIV_FROM_REG(data->fan_div[nr]));
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+	int reg = sis5595_read_value(client, SIS5595_REG_FANDIV);
+	switch (val) {
+	case 1: data->fan_div[nr] = 0; break;
+	case 2: data->fan_div[nr] = 1; break;
+	case 4: data->fan_div[nr] = 2; break;
+	case 8: data->fan_div[nr] = 3; break;
+	default:
+		dev_err(&client->dev, "fan_div value %ld not "
+			"supported. Choose one of 1, 2, 4 or 8!\n", val);
+		return -EINVAL;
+	}
+	
+	switch (nr) {
+	case 0:
+		reg = (reg & 0xcf) | (data->fan_div[nr] << 4);
+		break;
+	case 1:
+		reg = (reg & 0x3f) | (data->fan_div[nr] << 6);
+		break;
+	}
+	sis5595_write_value(client, SIS5595_REG_FANDIV, reg);
+	data->fan_min[nr] =
+		FAN_TO_REG(min, DIV_FROM_REG(data->fan_div[nr]));
+	sis5595_write_value(client, SIS5595_REG_FAN_MIN(nr), data->fan_min[nr]);
+	return count;
+}
+
+#define show_fan_offset(offset)						\
+static ssize_t show_fan_##offset (struct device *dev, char *buf)	\
+{									\
+	return show_fan(dev, buf, offset - 1);			\
+}									\
+static ssize_t show_fan_##offset##_min (struct device *dev, char *buf)	\
+{									\
+	return show_fan_min(dev, buf, offset - 1);			\
+}									\
+static ssize_t show_fan_##offset##_div (struct device *dev, char *buf)	\
+{									\
+	return show_fan_div(dev, buf, offset - 1);			\
+}									\
+static ssize_t set_fan_##offset##_min (struct device *dev,		\
+		const char *buf, size_t count)				\
+{									\
+	return set_fan_min(dev, buf, count, offset - 1);		\
+}									\
+static DEVICE_ATTR(fan##offset##_input, S_IRUGO, show_fan_##offset, NULL);\
+static DEVICE_ATTR(fan##offset##_min, S_IRUGO | S_IWUSR,		\
+		show_fan_##offset##_min, set_fan_##offset##_min);
+
+show_fan_offset(1);
+show_fan_offset(2);
+
+static ssize_t set_fan_1_div(struct device *dev, const char *buf,
+		size_t count)
+{
+	return set_fan_div(dev, buf, count, 0) ;
+}
+
+static ssize_t set_fan_2_div(struct device *dev, const char *buf,
+		size_t count)
+{
+	return set_fan_div(dev, buf, count, 1) ;
+}
+static DEVICE_ATTR(fan1_div, S_IRUGO | S_IWUSR,
+		show_fan_1_div, set_fan_1_div);
+static DEVICE_ATTR(fan2_div, S_IRUGO | S_IWUSR,
+		show_fan_2_div, set_fan_2_div);
+
+/* Alarms */
+static ssize_t show_alarms(struct device *dev, char *buf)
+{
+	struct sis5595_data *data = sis5595_update_device(dev);
+	return sprintf(buf, "%d\n", data->alarms);
+}
+static DEVICE_ATTR(alarms, S_IRUGO, show_alarms, NULL);
+ 
+/* This is called when the module is loaded */
+static int sis5595_attach_adapter(struct i2c_adapter *adapter)
+{
+	if (!(adapter->class & I2C_CLASS_HWMON))
+		return 0;
+	return i2c_detect(adapter, &addr_data, sis5595_detect);
+}
+
+/* Locate SiS bridge and correct base address for SIS5595 */
+static int sis5595_find_sis(int *address)
+{
+	u16 val;
+	int *i;
+
+	if (!(s_bridge =
+	    pci_get_device(PCI_VENDOR_ID_SI, PCI_DEVICE_ID_SI_503, NULL)))
+		return -ENODEV;
+
+	/* Look for imposters */
+	for(i = blacklist; *i != 0; i++) {
+		if (pci_get_device(PCI_VENDOR_ID_SI, *i, NULL)) {
+			dev_err(&s_bridge->dev, "looked for SIS5595 but found unsupported device %.4X\n", *i);
+			return -ENODEV;
+		}
+	}
+
+	if (PCIBIOS_SUCCESSFUL !=
+	    pci_read_config_word(s_bridge, SIS5595_BASE_REG, &val))
+		return -ENODEV;
+
+	*address = val & ~(SIS5595_EXTENT - 1);
+	if (*address == 0 && force_addr == 0) {
+		dev_err(&s_bridge->dev, "base address not set - upgrade BIOS or use force_addr=0xaddr\n");
+		return -ENODEV;
+	}
+	if (force_addr)
+		*address = force_addr;	/* so detect will get called */
+
+	return 0;
+}
+
+int sis5595_detect(struct i2c_adapter *adapter, int address, int kind)
+{
+	int err = 0;
+	int i;
+	struct i2c_client *new_client;
+	struct sis5595_data *data;
+	char val;
+	u16 a;
+
+	/* Make sure we are probing the ISA bus!!  */
+	if (!i2c_is_isa_adapter(adapter))
+		goto exit;
+
+	if(force_addr)
+		address = force_addr & ~(SIS5595_EXTENT - 1);
+	/* Reserve the ISA region */
+	if (!request_region(address, SIS5595_EXTENT, sis5595_driver.name)) {
+		err = -EBUSY;
+		goto exit;
+	}
+	if(force_addr) {
+		dev_warn(&adapter->dev, "forcing ISA address 0x%04X\n", address);
+		if (PCIBIOS_SUCCESSFUL !=
+		    pci_write_config_word(s_bridge, SIS5595_BASE_REG, address))
+			goto exit_release;
+		if (PCIBIOS_SUCCESSFUL !=
+		    pci_read_config_word(s_bridge, SIS5595_BASE_REG, &a))
+			goto exit_release;
+		if ((a & ~(SIS5595_EXTENT - 1)) != address)
+			/* doesn't work for some chips? */
+			goto exit_release;
+	}
+
+	if (PCIBIOS_SUCCESSFUL !=
+	    pci_read_config_byte(s_bridge, SIS5595_ENABLE_REG, &val)) {
+		goto exit_release;
+	}
+	if((val & 0x80) == 0) {
+		if (PCIBIOS_SUCCESSFUL !=
+		    pci_write_config_byte(s_bridge, SIS5595_ENABLE_REG,
+					  val | 0x80))
+			goto exit_release;
+		if (PCIBIOS_SUCCESSFUL !=
+		    pci_read_config_byte(s_bridge, SIS5595_ENABLE_REG, &val))
+			goto exit_release;
+		if((val & 0x80) == 0) 
+			/* doesn't work for some chips! */
+			goto exit_release;
+	}
+
+	if (!(data = kmalloc(sizeof(struct sis5595_data), GFP_KERNEL))) {
+		err = -ENOMEM;
+		goto exit_release;
+	}
+	memset(data, 0, sizeof(struct sis5595_data));
+
+	new_client = &data->client;
+	new_client->addr = address;
+	init_MUTEX(&data->lock);
+	i2c_set_clientdata(new_client, data);
+	new_client->adapter = adapter;
+	new_client->driver = &sis5595_driver;
+	new_client->flags = 0;
+
+	/* Check revision and pin registers to determine whether 4 or 5 voltages */
+	pci_read_config_byte(s_bridge, SIS5595_REVISION_REG, &(data->revision));
+	/* 4 voltages, 1 temp */
+	data->maxins = 3;
+	if(data->revision >= REV2MIN) {
+		pci_read_config_byte(s_bridge, SIS5595_PIN_REG, &val);
+		if (!(val & 0x80))
+			/* 5 voltages, no temps */
+			data->maxins = 4;
+	}
+	
+	/* Fill in the remaining client fields and put it into the global list */
+	strlcpy(new_client->name, "sis5595", I2C_NAME_SIZE);
+
+	data->valid = 0;
+	init_MUTEX(&data->update_lock);
+
+	/* Tell the I2C layer a new client has arrived */
+	if ((err = i2c_attach_client(new_client)))
+		goto exit_free;
+	
+	/* Initialize the SIS5595 chip */
+	sis5595_init_client(new_client);
+
+	/* A few vars need to be filled upon startup */
+	for (i = 0; i < 2; i++) {
+		data->fan_min[i] = sis5595_read_value(new_client,
+					SIS5595_REG_FAN_MIN(i));
+	}
+
+	/* Register sysfs hooks */
+	device_create_file(&new_client->dev, &dev_attr_in0_input);
+	device_create_file(&new_client->dev, &dev_attr_in0_min);
+	device_create_file(&new_client->dev, &dev_attr_in0_max);
+	device_create_file(&new_client->dev, &dev_attr_in1_input);
+	device_create_file(&new_client->dev, &dev_attr_in1_min);
+	device_create_file(&new_client->dev, &dev_attr_in1_max);
+	device_create_file(&new_client->dev, &dev_attr_in2_input);
+	device_create_file(&new_client->dev, &dev_attr_in2_min);
+	device_create_file(&new_client->dev, &dev_attr_in2_max);
+	device_create_file(&new_client->dev, &dev_attr_in3_input);
+	device_create_file(&new_client->dev, &dev_attr_in3_min);
+	device_create_file(&new_client->dev, &dev_attr_in3_max);
+	if (data->maxins == 4) {
+		device_create_file(&new_client->dev, &dev_attr_in4_input);
+		device_create_file(&new_client->dev, &dev_attr_in4_min);
+		device_create_file(&new_client->dev, &dev_attr_in4_max);
+	}
+	device_create_file(&new_client->dev, &dev_attr_fan1_input);
+	device_create_file(&new_client->dev, &dev_attr_fan1_min);
+	device_create_file(&new_client->dev, &dev_attr_fan1_div);
+	device_create_file(&new_client->dev, &dev_attr_fan2_input);
+	device_create_file(&new_client->dev, &dev_attr_fan2_min);
+	device_create_file(&new_client->dev, &dev_attr_fan2_div);
+	device_create_file(&new_client->dev, &dev_attr_alarms);
+	if (data->maxins == 3) {
+		device_create_file(&new_client->dev, &dev_attr_temp1_input);
+		device_create_file(&new_client->dev, &dev_attr_temp1_max);
+		device_create_file(&new_client->dev, &dev_attr_temp1_max_hyst);
+	}
+	return 0;
+	
+exit_free:
+	kfree(data);
+exit_release:
+	release_region(address, SIS5595_EXTENT);
+exit:
+	return err;
+}
+
+static int sis5595_detach_client(struct i2c_client *client)
+{
+	int err;
+
+	if ((err = i2c_detach_client(client))) {
+		dev_err(&client->dev,
+		    "Client deregistration failed, client not detached.\n");
+		return err;
+	}
+
+	if(i2c_is_isa_client(client))
+		release_region(client->addr, SIS5595_EXTENT);
+
+	kfree(i2c_get_clientdata(client));
+
+	return 0;
+}
+
+
+/* ISA access must be locked explicitly. */
+static int sis5595_read_value(struct i2c_client *client, u8 reg)
+{
+	int res;
+
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	down(&data->lock);
+	outb_p(reg, client->addr + SIS5595_ADDR_REG_OFFSET);
+	res = inb_p(client->addr + SIS5595_DATA_REG_OFFSET);
+	up(&data->lock);
+	return res;
+}
+
+static int sis5595_write_value(struct i2c_client *client, u8 reg, u8 value)
+{
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	down(&data->lock);
+	outb_p(reg, client->addr + SIS5595_ADDR_REG_OFFSET);
+	outb_p(value, client->addr + SIS5595_DATA_REG_OFFSET);
+	up(&data->lock);
+	return 0;
+}
+
+/* Called when we have found a new SIS5595. */
+static void sis5595_init_client(struct i2c_client *client)
+{
+	u8 config = sis5595_read_value(client, SIS5595_REG_CONFIG);
+	if (!(config & 0x01))
+		sis5595_write_value(client, SIS5595_REG_CONFIG,
+				(config & 0xf7) | 0x01);
+}
+
+static struct sis5595_data *sis5595_update_device(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct sis5595_data *data = i2c_get_clientdata(client);
+	int i;
+
+	down(&data->update_lock);
+
+	if ((jiffies - data->last_updated > HZ + HZ / 2) ||
+	    (jiffies < data->last_updated) || !data->valid) {
+
+		for (i = 0; i <= data->maxins; i++) {
+			data->in[i] =
+			    sis5595_read_value(client, SIS5595_REG_IN(i));
+			data->in_min[i] =
+			    sis5595_read_value(client,
+					       SIS5595_REG_IN_MIN(i));
+			data->in_max[i] =
+			    sis5595_read_value(client,
+					       SIS5595_REG_IN_MAX(i));
+		}
+		for (i = 0; i < 2; i++) {
+			data->fan[i] =
+			    sis5595_read_value(client, SIS5595_REG_FAN(i));
+			data->fan_min[i] =
+			    sis5595_read_value(client,
+					       SIS5595_REG_FAN_MIN(i));
+		}
+		if(data->maxins == 3) {
+			data->temp =
+			    sis5595_read_value(client, SIS5595_REG_TEMP);
+			data->temp_over =
+			    sis5595_read_value(client, SIS5595_REG_TEMP_OVER);
+			data->temp_hyst =
+			    sis5595_read_value(client, SIS5595_REG_TEMP_HYST);
+		}
+		i = sis5595_read_value(client, SIS5595_REG_FANDIV);
+		data->fan_div[0] = (i >> 4) & 0x03;
+		data->fan_div[1] = i >> 6;
+		data->alarms =
+		    sis5595_read_value(client, SIS5595_REG_ALARM1) |
+		    (sis5595_read_value(client, SIS5595_REG_ALARM2) << 8);
+		data->last_updated = jiffies;
+		data->valid = 1;
+	}
+
+	up(&data->update_lock);
+
+	return data;
+}
+
+
+
+static int __init sm_sis5595_init(void)
+{
+	int addr;
+
+	if (sis5595_find_sis(&addr))
+		return -ENODEV;
+	normal_isa[0] = addr;
+
+	return i2c_add_driver(&sis5595_driver);
+}
+
+static void __exit sm_sis5595_exit(void)
+{
+	i2c_del_driver(&sis5595_driver);
+}
+
+
+
+MODULE_AUTHOR("Aurelien Jarno <aurelien@aurel32.net>");
+MODULE_DESCRIPTION("SiS 5595 Sensor device");
+MODULE_LICENSE("GPL");
+
+module_init(sm_sis5595_init);
+module_exit(sm_sis5595_exit);

-- 
  .''`.  Aurelien Jarno	              GPG: 1024D/F1BCDB73
 : :' :  Debian GNU/Linux developer | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [PATCH 2.6] I2C: New chip driver: sis5595

[Jean Delvare]

Hi Alexey,

> Maybe you should call sis5595_update_device() in initialization function
> and get rid of "value" field. It's sole purpose to fill "struct sis5595"
> when it's known that "last_updated" field contains crap.

I assume you meant "valid" field.

Doesn't work. If you discard the "valid" field and call the update
function at init time, you cannot be sure that the update function will
actually do something. It all depends on the jiffies value relative to
last_updated (0 at this point). This is exactly what "valid" is there
for.

An alternative is to add a parameter to the update function, that would
force the update to happen regardless of time consideration. This
doesn't really change anything though, you simply remove a structure
member and replace it with a function parameter. How does it help?

Third possibility would be to initialize "last_updated" to anything
less than jiffies-(HZ+HZ/2) at init time, so as to be sure that the next
update will occur. Is it doable? Or maybe if we initialize
"last_updated" to the max possible jiffies count, it'll have the same
effect. But is it doable?

At any rate, the exact same issue exists in every over existing hardware
monitoring driver. If we change our mind we'll do it for all drivers,
not only this new one (sis5595), so Aurelien's patch is fine with me.

Thanks,
--
Jean Delvare