Skip to contentSkip to conversation

amd756 and amd8111 sensors support

PATCH In review Arjan van de Ven 4 participants 5 messages Dec 28, 2002 – Jan 11, 2003 1 message not in the archive

Subsystems (4)
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

#1 lore raw
Quoted 29 lines · Linux Kernel Mailing List

ChangeSet 1.958, 2002/12/26 13:55:05-08:00, [email protected]

[PATCH] amd756 and amd8111 sensors support

Add support for amd756 and amd8111 sensors

# This patch includes the following deltas:
# ChangeSet 1.957 -> 1.958
# include/linux/i2c-id.h 1.6 -> 1.7
# drivers/i2c/i2c-core.c 1.11 -> 1.12
# drivers/i2c/Kconfig 1.1 -> 1.2
# drivers/char/mem.c 1.32 -> 1.33
# drivers/i2c/i2c-proc.c 1.7 -> 1.8
# include/linux/i2c-proc.h 1.2 -> 1.3
# MAINTAINERS 1.112 -> 1.113
# drivers/i2c/i2c-dev.c 1.17 -> 1.18

#	    drivers/Makefile	1.27    -> 1.28   
#	               (new)	        -> 1.1     drivers/i2c/chips/Makefile
#	               (new)	        -> 1.1     drivers/i2c/chips/lm75.c
#	               (new)	        -> 1.1     drivers/i2c/chips/sensors.c
#	               (new)	        -> 1.1     drivers/i2c/chips/adm1021.c
#	               (new)	        -> 1.1     drivers/i2c/busses/Makefile
#	               (new)	        -> 1.1     drivers/i2c/busses/i2c-mainboard.c
#	               (new)	        -> 1.1     drivers/i2c/chips/Kconfig
#	               (new)	        -> 1.1     drivers/i2c/busses/i2c-amd8111.c
#	               (new)	        -> 1.1     drivers/i2c/busses/i2c-amd756.c
#	               (new)	        -> 1.1     drivers/i2c/busses/Kconfig
#	               (new)	        -> 1.1     include/linux/sensors.h
Quoted 1 line

#

cool! lm_sensors finally in 2.5!

#2 lore raw
Quoted 1 line · Arjan van de Ven

cool! lm_sensors finally in 2.5!

It would be cool if the code wasn't that obviously broken.

obj-$(CONFIG_I2C) += i2c/
+obj-$(CONFIG_I2C_MAINBOARD) += i2c/busses/
+obj-$(CONFIG_SENSORS) += i2c/chips/

Please add them inside drivers/i2c/, not at the toplevel.

obj-$(CONFIG_PHONE) += telephony/
obj-$(CONFIG_MD) += md/
obj-$(CONFIG_BT) += bluetooth/

drivers/char/mem.c +12 -0
diff -Nru a/drivers/char/mem.c b/drivers/char/mem.c
@@ -27,6 +27,12 @@
 #include <asm/io.h>
 #include <asm/pgalloc.h>
 
+#ifdef CONFIG_I2C_MAINBOARD
+extern void i2c_mainboard_init_all(void);
+#endif
+#ifdef CONFIG_SENSORS
+extern void sensors_init_all(void);
+#endif
 #ifdef CONFIG_I2C
 extern int i2c_init_all(void);
 #endif
@@ -705,6 +711,9 @@
 #ifdef CONFIG_I2C
 	i2c_init_all();
 #endif
+#ifdef CONFIG_I2C_MAINBOARD
+	i2c_mainboard_init_all();
+#endif
 #if defined (CONFIG_FB)
 	fbmem_init();
 #endif
@@ -719,6 +728,10 @@
 #if defined(CONFIG_S390_TAPE) && defined(CONFIG_S390_TAPE_CHAR)
 	tapechar_init();
 #endif
+#ifdef CONFIG_SENSORS
+	sensors_init_all();
+#endif
 

Where have you been the last years? Plese use initcalls instead of adding explicit junk in mem.c.

+#include <linux/version.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <asm/io.h>
+#include <linux/kernel.h>
+#include <linux/stddef.h>
+#include <linux/sched.h>
+#include <linux/ioport.h>
+#include <linux/i2c.h>
+#include <linux/init.h>

<asm/*.h> after <linux/*.h>, please

+
+struct sd {

+    const unsigned short vendor;
+    const unsigned short device;
+    const unsigned short function;
+    const char* name;
+    int amdsetup:1;
+};
+
+static struct sd supported[] = {
+    {PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_VIPER_740B, 3, "AMD756", 1},
+    {PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_VIPER_7413, 3, "AMD766", 1},
+    {PCI_VENDOR_ID_AMD, 0x7443, 3, "AMD768", 1},
+    {PCI_VENDOR_ID_NVIDIA, 0x01B4, 1, "nVidia nForce", 0},
+    {0, 0, 0}

+};

This should really use a proper pci ID table from the generic code.

+ /* id */ I2C_ALGO_SMBUS,
+ /* master_xfer */ NULL,
+ /* smbus_access */ amd756_access,
+ /* slave;_send */ NULL,
+ /* slave_rcv */ NULL,
+ /* algo_control */ NULL,
+ /* functionality */ amd756_func,
+};
+
+static struct i2c_adapter amd756_adapter = {
+ "unset",
+ I2C_ALGO_SMBUS | I2C_HW_SMBUS_AMD756,
+ &smbus_algorithm,
+ NULL,
+ amd756_inc,
+ amd756_dec,
+ NULL,
+ NULL,
+};

Please use named initializers for such sparse method vectors.

+
+static int __initdata amd756_initialized;

__initdata for function used in cleanup code is bogus. But this whole flag is crap, see below.

+static struct sd *amd756_sd = NULL;

Just properly organize your setup code and you don't need this one.

+static unsigned short amd756_smba = 0;

This is in .bss, no need to initialize to zero.

+ struct pci_dev *AMD756_dev = NULL;
+
+ if (pci_present() == 0) {
+ return -ENODEV;
+ }
+
+ /* Look for a supported chip */
+ for(currdev = supported; currdev->vendor; ) {
+ AMD756_dev = pci_find_device(currdev->vendor,
+ currdev->device, AMD756_dev);
+ if (AMD756_dev != NULL) {
+ if (PCI_FUNC(AMD756_dev->devfn) == currdev->function)
+ break;
+ } else {
+ currdev++;
+ }
+ }

I think you really just want to use a proper new-style pci driver..

+ if (check_region(amd756_smba, SMB_IOSIZE)) {
+ printk
+ ("i2c-amd756.o: SMB region 0x%x already in use!\n",
+ amd756_smba);
+ return -ENODEV;
+ }
+
+ /* Everything is happy, let's grab the memory and set things up. */
+ request_region(amd756_smba, SMB_IOSIZE, "amd756-smbus");

Don't use check_region.

+
+void amd756_inc(struct i2c_adapter *adapter)
+{
+ MOD_INC_USE_COUNT;
+}
+
+void amd756_dec(struct i2c_adapter *adapter)
+{
+
+ MOD_DEC_USE_COUNT;
+}

Your module-refcounting is bogus.

+int __init i2c_amd756_init(void)
+{
+ int res;
+#ifdef DEBUG
+/* PE- It might be good to make this a permanent part of the code! */
+ if (amd756_initialized) {
+ printk
+ ("i2c-amd756.o: Oops, amd756_init called a second time!\n");
+ return -EBUSY;
+ }
+#endif

This assert can't ever be triggered.

+ amd756_initialized = 0;

It's in .bss and thus already zero.

+ if ((res = amd756_setup())) {
+ printk
+ ("i2c-amd756.o: AMD756 or compatible device not detected, module not inserted.\n");
+ amd756_cleanup();
+ return res;
+ }

Just merge amd756_setup into it's only caller and your init code will get a lot nicer.

+void __exit i2c_amd756_exit(void)
+{
+ amd756_cleanup();
+}
+
+static int amd756_cleanup(void)
+{
+ int res;
+ if (amd756_initialized >= 2) {
+ if ((res = i2c_del_adapter(&amd756_adapter))) {
+ printk
+ ("i2c-amd756.o: i2c_del_adapter failed, module not removed\n");
+ return res;
+ } else
+ amd756_initialized--;
+ }
+ if (amd756_initialized >= 1) {
+ release_region(amd756_smba, SMB_IOSIZE);
+ amd756_initialized--;
+ }
+ return 0;
+}

Please merge i2c_amd756_exit into amd756_cleanup and do proper error handling inside the int function instead of the gliobal state mess.

+
+EXPORT_NO_SYMBOLS;

Remove it, it's a noop for 2.5.

+#ifdef MODULE
+
+MODULE_AUTHOR("Merlin Hughes <[email protected]>"); +MODULE_DESCRIPTION("AMD756/766/768/nVidia nForce SMBus driver"); +
+#ifdef MODULE_LICENSE
+MODULE_LICENSE("GPL");
+#endif
+
+#endif /* MODULE */

Please get rid of that ifdef mess, you can just always use it.

And please backout the init/exit changes in i2c-dev.c, i2c-core.c and i2c-proc.c, they're just bogus.

#3 lore raw

PATCH 1/1 i2c-isa and w83871d sensors support

This patch adds the i2c-isa pseudo ISA adapter and the w8387* series sensors support to 2.5.54 (tested on bk1 and bk3).

I've just took the code from the lm_sensor package and changed it to be more or less consistent with the lm75 and amd* patches Christoph has done.
Christoph, maybe you could check this because I don't really understand all of the code and maybe it needs some more changes.

GertJan

drivers/i2c/busses/Kconfig +18 -0
diff -Nru linux-2.5.54-bk1/drivers/i2c/busses/Kconfig linux-2.5.54/drivers/i2c/busses/Kconfig
@@ -39,5 +39,23 @@
 	  in the lm_sensors package, which you can download at
 	  http://www.lm-sensors.nu
 
+config I2C_ISA
+	tristate "  Pseudo ISA adapter (for some hardware sensors)"
+	depends on I2C && I2C_PROC
+	help
+	  This provide support for accessing some hardware sensors chips over
+	  the ISA bus rather than the I2C or SMBus. If you want to do this,
+	  say yes here.
+
+	  This can also be built as a module which can be inserted and removed
+	  while the kernel is running.  If you want to compile it as a module,
+	  say M here and read <file:Documentation/modules.txt>.
+
+	  The module will be called i2c-isa.o.
+
+	  You will also need the latest user-space utilties: you can find them
+	  in the lm_sensors package, which you can download at
+	  http://www.lm-sensors.nu
+
 endmenu
 
drivers/i2c/busses/Makefile +1 -0
diff -Nru linux-2.5.54-bk1/drivers/i2c/busses/Makefile linux-2.5.54/drivers/i2c/busses/Makefile
@@ -4,3 +4,4 @@
 
 obj-$(CONFIG_I2C_AMD756)	+= i2c-amd756.o
 obj-$(CONFIG_I2C_AMD8111)	+= i2c-amd8111.o
+obj-$(CONFIG_I2C_ISA)		+= i2c-isa.o
drivers/i2c/busses/i2c-isa.c +108 -0

108 lines not on this page · raw message

drivers/i2c/chips/Kconfig +15 -0
diff -Nru linux-2.5.54-bk1/drivers/i2c/chips/Kconfig linux-2.5.54/drivers/i2c/chips/Kconfig
@@ -37,4 +37,19 @@
 	  in the lm_sensors package, which you can download at
 	  http://www.lm-sensors.nu
 
+config SENSORS_W83781D
+	tristate "  Winbond W83781D, W83782D, W83783S, W83627HF and Asus AS99127F"
+	depends on I2C && I2C_PROC && I2C_ISA
+	help
+	  If you say yes here you get support for the Winbond W8378x series
+	  of sensor chips: the W83781D, W83782D, W83783S, W83627HF and the
+	  similar Asus AS99127F. This can also be built as a module
+	  which can be inserted and removed while the kernel is running.
+
+	  The module will be called w83781d.o.
+
+	  You will also need the latest user-space utilties: you can find them
+	  in the lm_sensors package, which you can download at 
+	  http://www.lm-sensors.nu
+
 endmenu
drivers/i2c/chips/Makefile +1 -0
diff -Nru linux-2.5.54-bk1/drivers/i2c/chips/Makefile linux-2.5.54/drivers/i2c/chips/Makefile
@@ -4,3 +4,4 @@
 
 obj-$(CONFIG_SENSORS_ADM1021)	+= adm1021.o
 obj-$(CONFIG_SENSORS_LM75)	+= lm75.o
+obj-$(CONFIG_SENSORS_W83781D)	+= w83781d.o
drivers/i2c/chips/w83781d.c +2075 -0

2,075 lines not on this page · raw message

#4 lore raw

Re: [PATCH] i2c-isa and w83871d sensors support

Quoted 8 lines · GertJan Spoelman

This patch adds the i2c-isa pseudo ISA adapter and the w8387* series sensors support to 2.5.54 (tested on bk1 and bk3).

I've just took the code from the lm_sensor package and changed it to be more or less consistent with the lm75 and amd* patches Christoph has done.
Christoph, maybe you could check this because I don't really understand all of the code and maybe it needs some more changes.

Looks fine in principle. A few nitpicks:

drivers/i2c/busses/i2c-isa.c line 45

Quoted 6 lines
drivers/i2c/busses/i2c-isa.c +6 -0
+static struct i2c_algorithm isa_algorithm = {
+	.name		= "ISA bus algorithm",
+	.id		= I2C_ALGO_ISA,
+	.smbus_xfer	= NULL,
+	.functionality	= &isa_func,
+};

There's no need to initialize a struct member to zero/NULL.

Quoted 8 lines
drivers/i2c/busses/i2c-isa.c +8 -0
+static int __init isa_init(void)
+{
+	int res;
+	if (isa_initialized) {
+		pr_debug(DRV_NAME
+		    ": i2c-isa.o: Oops, isa_init called a second time!\n");
+		return -EBUSY;
+	}

drivers/i2c/busses/i2c-isa.c line 81

Quoted 10 lines
drivers/i2c/busses/i2c-isa.c +10 -0
+	isa_initialized = 0;
+	if ((res = i2c_add_adapter(&isa_adapter))) {
+		printk(KERN_ERR DRV_NAME
+		       ": Adapter registration failed, module not inserted\n.");
+		isa_cleanup();
+		return res;
+	}
+	isa_initialized++;
+	printk("i2c-isa.o: ISA bus access for i2c modules initialized.\n");
+	return 0;

Please remove the <foo>_initialized handling. It just obsfucates the code.

drivers/i2c/busses/i2c-isa.c line 87

Quoted 4 lines
drivers/i2c/busses/i2c-isa.c +4 -0
+static void __exit isa_exit(void)
+{
+	isa_cleanup();
+}

Just move the code from isa_cleanup into isa_exit directly.

#5 lore raw
In reply to Christoph Hellwig

Hi!

Quoted 1 line

Please add them inside drivers/i2c/, not at the toplevel.

Done.

Quoted 14 lines

obj-$(CONFIG_PHONE) += telephony/
obj-$(CONFIG_MD) += md/
obj-$(CONFIG_BT) += bluetooth/

(quoted diff) +10 -0
+#include <linux/version.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <asm/io.h>
+#include <linux/kernel.h>
+#include <linux/stddef.h>
+#include <linux/sched.h>
+#include <linux/ioport.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
Quoted 1 line

<asm/*.h> after <linux/*.h>, please

Fixed.

Quoted 11 lines
(quoted diff) +10 -0
+static struct i2c_adapter amd756_adapter = {
+	"unset",
+	I2C_ALGO_SMBUS | I2C_HW_SMBUS_AMD756,
+	&smbus_algorithm,
+	NULL,
+	amd756_inc,
+	amd756_dec,
+	NULL,
+	NULL,
+};
Quoted 1 line

Please use named initializers for such sparse method vectors.

Fixed.

Quoted 4 lines

+static int __initdata amd756_initialized;

__initdata for function used in cleanup code is bogus. But this whole flag is crap, see below.

Killed.

Quoted 3 lines

+static unsigned short amd756_smba = 0;

This is in .bss, no need to initialize to zero.

Ok.

Quoted 11 lines
(quoted diff) +9 -0
+	if (check_region(amd756_smba, SMB_IOSIZE)) {
+		printk
+		    ("i2c-amd756.o: SMB region 0x%x already in use!\n",
+		     amd756_smba);
+		return -ENODEV;
+	}
+
+	/* Everything is happy, let's grab the memory and set things up. */
+	request_region(amd756_smba, SMB_IOSIZE, "amd756-smbus");

Don't use check_region.

Fixed.

Quoted 3 lines
(quoted diff) +1 -0
+	amd756_initialized = 0;

It's in .bss and thus already zero.

Killed.

Quoted 15 lines

+EXPORT_NO_SYMBOLS;

Remove it, it's a noop for 2.5.

(quoted diff) +10 -0
+#ifdef MODULE
+
+MODULE_AUTHOR("Merlin Hughes <[email protected]>");
+MODULE_DESCRIPTION("AMD756/766/768/nVidia nForce SMBus driver");
+
+#ifdef MODULE_LICENSE
+MODULE_LICENSE("GPL");
+#endif
+
+#endif				/* MODULE */
Quoted 1 line

Please get rid of that ifdef mess, you can just always use it.

Killed.

Okay, I see lot more cleaning up is needed. I actually saved it as a "TODO" file... Thanks for your comments.

Pavel

drivers/Makefile +0 -2
@@ -38,8 +38,6 @@
 obj-$(CONFIG_SERIO)		+= input/serio/
 obj-$(CONFIG_I2O)		+= message/
 obj-$(CONFIG_I2C)		+= i2c/
-obj-$(CONFIG_I2C_MAINBOARD)	+= i2c/busses/
-obj-$(CONFIG_SENSORS)		+= i2c/chips/
 obj-$(CONFIG_PHONE)		+= telephony/
 obj-$(CONFIG_MD)		+= md/
 obj-$(CONFIG_BT)		+= bluetooth/
drivers/i2c/Makefile +2 -0
@@ -6,6 +6,8 @@
 		   i2c-algo-ite.o i2c-proc.o i2c-algo-ibm_ocp.o
 
 obj-$(CONFIG_I2C)		+= i2c-core.o
+obj-$(CONFIG_I2C_MAINBOARD)	+= busses/
+obj-$(CONFIG_SENSORS)		+= chips/
 obj-$(CONFIG_I2C_CHARDEV)	+= i2c-dev.o
 obj-$(CONFIG_I2C_ALGOBIT)	+= i2c-algo-bit.o
 obj-$(CONFIG_I2C_PHILIPSPAR)	+= i2c-philips-par.o
drivers/i2c/busses/i2c-amd756.c +17 -44

109 lines not on this page · raw message

include/linux/i2c.h +0 -4
@@ -223,10 +223,6 @@
 	u32 (*functionality) (struct i2c_adapter *);
 };
 
-#if LINUX_VERSION_CODE < KERNEL_VERSION(2,1,29)
-struct proc_dir_entry;
-#endif
-
 /*
  * i2c_adapter is the structure used to identify a physical i2c bus along
  * with the access algorithms necessary to access it.
Signature · 2 lines
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?