Skip to content Skip to conversation

i2c-amd766 driver for 2.5.50

DISCUSSION Peter Samuelson 2 participants 3 messages Dec 1, 2002 – Dec 2, 2002

Share & tools

Peter Samuelson

author

2002-12-01 23:31 UTC #0 lore raw

Cosmetic stuff..

drivers/i2c/busses/Makefile 2002-12-01 17:57:14.000000000 +0100 line 10

Quoted 13 lines

drivers/i2c/busses/Makefile 2002-12-01 17:57:14.000000000 +0100 renamed from drivers/i2c/busses/Makefile 2002-12-01 16:56:56.000000000 +0100 +10 -0
@@ -0,0 +1,10 @@
+#
+# Makefile for the kernel hardware sensors bus drivers.
+#
+
+MOD_LIST_NAME := SENSORS_BUS_MODULES
+
+obj-$(CONFIG_I2C_MAINBOARD) += i2c-mainboard.o
+obj-$(CONFIG_I2C_AMD756) += i2c-amd756.o
+
+include $(TOPDIR)/Rules.make

MOD_LIST_NAME was deprecated in 2.3. 'include Rules.make' was deprecated in 2.5. Also appears in drivers/i2c/chips/Makefile.

drivers/i2c/busses/Makefile 2002-12-01 17:57:14.000000000 +0100

Quoted 9 lines

drivers/i2c/busses/Makefile 2002-12-01 17:57:14.000000000 +0100 renamed from drivers/i2c/busses/Makefile 2002-12-01 16:56:56.000000000 +0100 +9 -0
+#ifndef PCI_DEVICE_ID_AMD_756
+#define PCI_DEVICE_ID_AMD_756 0x740B
+#endif
+#ifndef PCI_DEVICE_ID_AMD_766
+#define PCI_DEVICE_ID_AMD_766 0x7413
+#endif
+#ifndef PCI_DEVICE_ID_NVIDIA_NFORCE_SMBUS
+#define PCI_DEVICE_ID_NVIDIA_NFORCE_SMBUS 0x01B4
+#endif

These are all in pci_ids.h already, under other names. If these names are better, they should replace the others.

Quoted 5 lines

drivers/i2c/busses/Makefile 2002-12-01 17:57:14.000000000 +0100 renamed from drivers/i2c/busses/Makefile 2002-12-01 16:56:56.000000000 +0100 +5 -0
+struct sd {
+	const unsigned short vendor;
+	const unsigned short device;
+	const unsigned short function;
+	const char* name;

drivers/i2c/busses/Makefile 2002-12-01 17:57:14.000000000 +0100

Quoted 10 lines

drivers/i2c/busses/Makefile 2002-12-01 17:57:14.000000000 +0100 renamed from drivers/i2c/busses/Makefile 2002-12-01 16:56:56.000000000 +0100 +10 -0
+	int amdsetup:1;
+};
+
+static struct sd supported[] = {
+	{PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_756, 3, "AMD756", 1},
+	{PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_766, 3, "AMD766", 1},
+	{PCI_VENDOR_ID_AMD, 0x7443, 3, "AMD768", 1},
+	{PCI_VENDOR_ID_NVIDIA, 0x01B4, 1, "nVidia nForce", 0},
+	{0, 0, 0}
+};

You should also have a struct pci_device_id[] here, so you can have a MODULE_DEVICE_TABLE().

drivers/i2c/busses/Makefile 2002-12-01 17:57:14.000000000 +0100

Quoted 5 lines

drivers/i2c/busses/Makefile 2002-12-01 17:57:14.000000000 +0100 renamed from drivers/i2c/busses/Makefile 2002-12-01 16:56:56.000000000 +0100 +5 -0
+/* OK, this is not exactly good programming practice, usually. But it is
+   very code-efficient in this case. */
+
+      ERROR4:
+	i2c_detach_client(new_client);

No need to apologise for goto error unwinding - it's all over the kernel.

drivers/i2c/busses/Makefile 2002-12-01 17:57:14.000000000 +0100

Quoted 6 lines

drivers/i2c/busses/Makefile 2002-12-01 17:57:14.000000000 +0100 renamed from drivers/i2c/busses/Makefile 2002-12-01 16:56:56.000000000 +0100 +6 -0
+void adm1021_dec_use(struct i2c_client *client)
+{
+#ifdef MODULE
+	MOD_DEC_USE_COUNT;
+#endif
+}

No need for #ifdef. Also found in lm75_inc_use() and elsewhere.

drivers/i2c/busses/Makefile 2002-12-01 17:57:14.000000000 +0100

Quoted 8 lines

drivers/i2c/busses/Makefile 2002-12-01 17:57:14.000000000 +0100 renamed from drivers/i2c/busses/Makefile 2002-12-01 16:56:56.000000000 +0100 +8 -0
+void adm1021_update_client(struct i2c_client *client)
+{
+	struct adm1021_data *data = client->data;
+
+	down(&data->update_lock);
+
+	if ((jiffies - data->last_updated > HZ + HZ / 2) ||
+	    (jiffies < data->last_updated) || !data->valid) {

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

It *appears* the (jiffies < data->last_updated) test is unnecessary.

Quoted 1 line

+EXPORT_NO_SYMBOLS;

Deprecated (from lm75.c).

General comment: what's up with /proc/sys/dev/ versus /proc/driver/ versus sysfs? Do we really need all three?

Peter

Pavel Machek

2002-12-02 00:47 UTC #1 lore raw

Hi!

Quoted 9 lines

Review
Quoted 6 lines
(quoted diff) +6 -0
+MOD_LIST_NAME := SENSORS_BUS_MODULES
+
+obj-$(CONFIG_I2C_MAINBOARD) += i2c-mainboard.o
+obj-$(CONFIG_I2C_AMD756) += i2c-amd756.o
+
+include $(TOPDIR)/Rules.make
MOD_LIST_NAME was deprecated in 2.3. 'include Rules.make' was deprecated in 2.5. Also appears in drivers/i2c/chips/Makefile.

I agree with MOD_LIST_NAME, but I still see include $(TOPDIR)/Rules.make used all over the kernel, so I kept it.

Quoted 4 lines

Quoted 4 lines

(quoted diff) +4 -0

+#ifndef PCI_DEVICE_ID_AMD_756
+#define PCI_DEVICE_ID_AMD_756 0x740B
+#endif
+#ifndef PCI_DEVICE_ID_AMD_766

Quoted 8 lines

Review
Quoted 5 lines
(quoted diff) +5 -0
+#define PCI_DEVICE_ID_AMD_766 0x7413
+#endif
+#ifndef PCI_DEVICE_ID_NVIDIA_NFORCE_SMBUS
+#define PCI_DEVICE_ID_NVIDIA_NFORCE_SMBUS 0x01B4
+#endif
These are all in pci_ids.h already, under other names. If these names are better, they should replace the others.

Fixed.

Quoted 10 lines

Quoted 10 lines

(quoted diff) +10 -0

+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_756, 3, "AMD756", 1},

Quoted 8 lines

Review
Quoted 5 lines
(quoted diff) +5 -0
+	{PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_766, 3, "AMD766", 1},
+	{PCI_VENDOR_ID_AMD, 0x7443, 3, "AMD768", 1},
+	{PCI_VENDOR_ID_NVIDIA, 0x01B4, 1, "nVidia nForce", 0},
+	{0, 0, 0}
+};
You should also have a struct pci_device_id[] here, so you can have a MODULE_DEVICE_TABLE().

This one will have to wait...

Quoted 5 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+/* OK, this is not exactly good programming practice, usually. But it is
+   very code-efficient in this case. */
No need to apologise for goto error unwinding - it's all over the
Quoted 1 line
kernel.

Yep, killed comment.

Quoted 8 lines

Review
Quoted 6 lines
(quoted diff) +6 -0
+void adm1021_dec_use(struct i2c_client *client)
+{
+#ifdef MODULE
+	MOD_DEC_USE_COUNT;
+#endif
+}
No need for #ifdef. Also found in lm75_inc_use() and elsewhere.

Agreed, killed those I could find quickly.

Quoted 11 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+void adm1021_update_client(struct i2c_client *client)
+{
+	struct adm1021_data *data = client->data;
+
+	down(&data->update_lock);
+
+	if ((jiffies - data->last_updated > HZ + HZ / 2) ||
+	    (jiffies < data->last_updated) || !data->valid) {

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

Quoted 1 line

It *appears* the (jiffies < data->last_updated) test is unnecessary.

I guess I'd better leave it alone :-).

Quoted 3 lines

Quoted 1 line
+EXPORT_NO_SYMBOLS;
Deprecated (from lm75.c).

Killed.

Quoted 2 lines

General comment: what's up with /proc/sys/dev/ versus /proc/driver/ versus sysfs? Do we really need all three?

sensors are sysctl-capable, /proc/sys/dev/ is just side effect, IIRC.

Pavel

Signature · 2 lines

Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Peter Samuelson

author

2002-12-02 01:53 UTC #2 lore raw

[Pavel Machek]

Quoted 5 lines

Quoted 2 lines
MOD_LIST_NAME was deprecated in 2.3. 'include Rules.make' was deprecated in 2.5. Also appears in drivers/i2c/chips/Makefile.
I agree with MOD_LIST_NAME, but I still see include $(TOPDIR)/Rules.make used all over the kernel, so I kept it.

It's dead. The 500-odd Makefiles haven't all been changed yet. If you don't believe me, check out the contents of Rules.make. (:

Peter