ATXP1 kernel patch

ATXP1 kernel patch

[Sebastian Witt]

Hello,

some time ago Marcin Kaluza converted 8rdavcore (a userspace project to 
control the CPU core and other voltages, www.hasw.net) to a kernel module.

I've cleaned it up so it only supports VID changing, because this is not 
vendor specific like the GPIO pins.

To get some overview about usage, visit:
http://forums.gentoo.org/viewtopic.php?t'3047

I've attached the patch for kernel 2.6.10, please comment.

Regards,
Sebastian
-------------- next part --------------
diff -ruN linux-2.6.10-orig/drivers/i2c/chips/Kconfig linux-2.6.10/drivers/i2c/chips/Kconfig
--- linux-2.6.10-orig/drivers/i2c/chips/Kconfig	2004-12-24 22:34:58.000000000 +0100
+++ linux-2.6.10/drivers/i2c/chips/Kconfig	2005-01-02 16:54:50.545276192 +0100
@@ -347,6 +347,19 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-rtc8564.
 
+config SENSORS_ATXP1
+	tristate "Attansic ATXP1 VID controller"
+	depends on I2C && EXPERIMENTAL
+	help
+	  If you say yes here you get support for the Attansic ATXP1 VID
+	  controller.
+
+	  If your board have such a chip, you are able to control your CPU
+	  core and other voltages.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called atxp1.
+	  
 config ISP1301_OMAP
 	tristate "Philips ISP1301 with OMAP OTG"
 	depends on I2C && ARCH_OMAP_OTG
diff -ruN linux-2.6.10-orig/drivers/i2c/chips/Makefile linux-2.6.10/drivers/i2c/chips/Makefile
--- linux-2.6.10-orig/drivers/i2c/chips/Makefile	2004-12-24 22:35:50.000000000 +0100
+++ linux-2.6.10/drivers/i2c/chips/Makefile	2005-01-02 16:51:30.293719056 +0100
@@ -34,6 +34,7 @@
 obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
 obj-$(CONFIG_SENSORS_W83L785TS)	+= w83l785ts.o
 obj-$(CONFIG_ISP1301_OMAP)	+= isp1301_omap.o
+obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
 
 ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)
 EXTRA_CFLAGS += -DDEBUG
diff -ruN linux-2.6.10-orig/drivers/i2c/chips/atxp1.c linux-2.6.10/drivers/i2c/chips/atxp1.c
--- linux-2.6.10-orig/drivers/i2c/chips/atxp1.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.10/drivers/i2c/chips/atxp1.c	2005-01-02 16:50:36.531892096 +0100
@@ -0,0 +1,259 @@
+/*
+    atxp1.c - kernel module for setting Vcore using ATXP1 chip. 
+    
+    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.
+    
+    Version history:
+    0.1: Converted 8rdavcore into kernel module (Marcin Kaluza <marcin_ml@sekretarka.no-ip.org>)
+    0.2: Cleanup, general interface for VCore (Sebastian Witt <hasw@hasw.net>)
+    
+*/
+
+#include <linux/config.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("VCore changing via Attansic ATXP1");
+MODULE_VERSION("0.2");
+
+#define ATXP1_VID 0x00
+#define ATXP1_CVID 0x01
+#define ATXP1_VIDENA 0x20
+#define ATXP1_VIDMASK 0x1f
+
+static unsigned short normal_i2c[]= { 0x37, 0x4e, I2C_CLIENT_END };
+static unsigned short normal_i2c_range[]= { I2C_CLIENT_END };
+static int atxp1_attach_adapter(struct i2c_adapter * adapter);
+static int atxp1_detach_client(struct i2c_client * client);
+static struct atxp1_data * atxp1_update_device(struct device *dev);
+static int atxp1_detect(struct i2c_adapter *adapter, int address, int kind);
+
+static struct i2c_driver atxp1_driver = {
+	.owner		= THIS_MODULE,
+	.name		= "atxp1",
+	.id		= I2C_DRIVERID_EXP1,
+	.flags		= I2C_DF_NOTIFY,
+	.attach_adapter = atxp1_attach_adapter,
+	.detach_client	= atxp1_detach_client,
+};
+
+I2C_CLIENT_INSMOD;
+
+struct atxp1_data {
+	struct i2c_client client;
+//	struct semaphore update_lock;
+	u8 valid;
+	//unsigned long last_updated;
+	unsigned char cpu_vid;
+	unsigned char vid;
+};
+
+/* Conversion VCore <-> VID */
+static unsigned int atxp1_calc_vcore(unsigned char vid)
+{
+	return 1850 - (vid & ATXP1_VIDMASK) * 25;
+};
+
+static unsigned char atxp1_calc_vid(unsigned int vt) {
+	return(((1850 - vt) / 25 + 0.5));
+}
+
+static struct atxp1_data * atxp1_update_device(struct device *dev) 
+{
+	struct i2c_client *client;
+	struct atxp1_data *data;
+	
+	client = to_i2c_client(dev);
+	data = i2c_get_clientdata(client);
+	
+	if(data->valid = 0) {
+		data->vid = i2c_smbus_read_byte_data(client, ATXP1_VID);
+		data->cpu_vid = i2c_smbus_read_byte_data(client, ATXP1_CVID);
+	}
+    	
+	return(data);
+}
+
+static int atxp1_write(struct i2c_client *client, unsigned char adr, unsigned char data) 
+{
+	int ret = -1;
+
+	ret = i2c_smbus_write_byte_data(client, adr, data);
+	
+	if(ret < 0) {
+		printk(KERN_ERR "atxp1_write(): Write failed\n");
+		return -1;
+	}
+
+	ret = i2c_smbus_read_byte_data(client, adr);
+	if(ret < 0) {
+		printk(KERN_ERR "atxp1_write(): Read from 0x%02x failed\n", adr);
+		return -1;
+	}
+	
+	if(ret = data) return(0);
+	else {
+		printk(KERN_ERR "atxp1_write(): Readback failed, 0x%02x written, 0x%02x readback from 0x%02x\n", data, ret, adr);
+		return -1;
+	}
+
+	return 0;
+}
+
+ssize_t atxp1_showvcore(struct device *dev, char *buf)
+{
+	int size;
+	struct atxp1_data *data;
+	
+	data = atxp1_update_device(dev);
+	
+	size = sprintf(buf, "%d\n", atxp1_calc_vcore(data->vid));
+
+	return size;
+}
+
+ssize_t atxp1_storevcore(struct device *dev, const char* buf, size_t count)
+{
+	struct atxp1_data *data;
+	struct i2c_client *client;
+	unsigned char vid;
+	unsigned char cvid;
+	unsigned int vcore = 1650;
+
+	client = to_i2c_client(dev);
+	data = atxp1_update_device(dev);
+
+	sscanf(buf, "%d", &vcore);
+	vcore /= 25;	
+	vcore *= 25;	
+	
+	if((vcore < 1075) || (vcore > 1850)) {
+		printk(KERN_INFO "Use VCore between 1075 mV and 1850 mV\n");
+		return strlen(buf);
+	}
+	
+        vid = atxp1_calc_vid(vcore);
+
+        if(data->vid & ATXP1_VIDENA)
+		cvid = data->vid & ~ATXP1_VIDENA;
+	else
+		cvid = data->cpu_vid;
+	
+	if(vid = cvid)
+		return strlen(buf);
+	
+	printk(KERN_DEBUG "atxp1: Setting VCore to %d mV (0x%02x)\n", vcore, vid);
+	
+	if(cvid > vid) {
+		for(; cvid >= vid; cvid--) {
+        		atxp1_write(client, ATXP1_VID, cvid | ATXP1_VIDENA);
+		}
+	}
+	else {
+		for(; cvid <= vid; cvid++) {
+        		atxp1_write(client, ATXP1_VID, cvid | ATXP1_VIDENA);
+		}
+	}
+	
+	data->valid = 0;
+	
+	return strlen(buf);
+}
+
+static 	DEVICE_ATTR(vcore, 0644, atxp1_showvcore, atxp1_storevcore);
+
+
+static int atxp1_attach_adapter(struct i2c_adapter *adapter)
+{
+	return i2c_probe(adapter, &addr_data, &atxp1_detect);
+};
+	
+static int atxp1_detect(struct i2c_adapter *adapter, int address, int kind)
+{
+	/* Checking if CVID != 00 (and maybe VID) */
+	struct i2c_client* new_client;
+	struct atxp1_data * data;
+	int err;
+
+	if (!(data = kmalloc(sizeof(struct atxp1_data), GFP_KERNEL)))
+		return -ENOMEM;
+
+	memset(data, 0, sizeof(struct atxp1_data));
+	new_client = &data->client;
+	i2c_set_clientdata(new_client, data);
+	
+	new_client->addr = address;
+	new_client->adapter = adapter;
+	new_client->driver = &atxp1_driver;
+	new_client->flags = 0;
+	
+	/* Detect ATXP1 */
+	if(i2c_smbus_read_byte_data(new_client, ATXP1_CVID) <= 0) {
+		kfree(data);
+		return 0;
+	}
+	
+	strncpy(new_client->name, "atxp1", I2C_NAME_SIZE);
+	new_client->id=0;
+	data->valid=0;
+	
+//	init_MUTEX(&data->update_lock);
+	
+	if ((err = i2c_attach_client(new_client)))
+	{
+		printk(KERN_ERR "atxp1: Attach client error\n");
+		kfree(data);
+		return err;
+	}
+	
+	device_create_file(&new_client->dev, &dev_attr_vcore);
+	
+	printk(KERN_INFO "atxp1: Detected on %s, address 0x%02x\n", adapter->name, new_client->addr);
+	
+	return 0;
+};
+
+static int atxp1_detach_client(struct i2c_client * client)
+{
+	int err;
+	
+	err = i2c_detach_client(client);
+	
+	if (err) 
+		printk(KERN_ERR "atxp1: failed to detach client");
+	
+	kfree(i2c_get_clientdata(client));
+	
+	return 0;
+};
+ 
+static int atxp1_init(void)  
+{ 
+	i2c_add_driver(&atxp1_driver);
+	
+	return 0; 
+};
+
+void atxp1_exit(void) 
+{ 
+	i2c_del_driver(&atxp1_driver);
+};
+
+module_init(atxp1_init);
+module_exit(atxp1_exit);
+

ATXP1 kernel patch

[Rudolf Marek]

Hello,

Here are some random notes to start with.

+static unsigned short normal_i2c[]= { 0x37, 0x4e, I2C_CLIENT_END };

I read on some forum also about 0x44. Any clue?

+static 	DEVICE_ATTR(vcore, 0644, atxp1_showvcore, atxp1_storevcore);

cpu[0-1]_vid    CPU core reference voltage.
                Unit: millivolt

What about to use this? (This is also question for other developers :)

+	if(i2c_smbus_read_byte_data(new_client, ATXP1_CVID) <= 0) {
+		kfree(data);
+		return 0;

Hmm really not very scientific nor realible. Any other method is possible?

Next question is about update lock.
Why is gone? It is always good idea...

Also what about ATXP3 and ATX5 my asus board (A7S333) has ATXP3.

Any effort about getting datasheet?

Regards

Rudolf

ATXP1 kernel patch

[Sebastian Witt]

Hi,

Rudolf Marek wrote:
> +static unsigned short normal_i2c[]= { 0x37, 0x4e, I2C_CLIENT_END };
> 
> I read on some forum also about 0x44. Any clue?

I've only seen the ATXP1 on 0x37 or 0x4e (it only supports two 
addresses), the
0x44 is maybe for another ATXP.

> +static 	DEVICE_ATTR(vcore, 0644, atxp1_showvcore, atxp1_storevcore);
> 
> cpu[0-1]_vid    CPU core reference voltage.
>                 Unit: millivolt
> 
> What about to use this? (This is also question for other developers :)

Yes, good idea.

> +	if(i2c_smbus_read_byte_data(new_client, ATXP1_CVID) <= 0) {
> +		kfree(data);
> +		return 0;
> 
> Hmm really not very scientific nor realible. Any other method is possible?

No, it has no register with a device id.

> Next question is about update lock.
> Why is gone? It is always good idea...

Yes, will be added again.

> Also what about ATXP3 and ATX5 my asus board (A7S333) has ATXP3.

AFAIK the ATXP3 is the same as the ATXP1 but without VID control. Not 
sure about the ATXP5, I've only a board with a ATXP1. Needing someone 
who wants to test this.

> Any effort about getting datasheet?
Attansic doesn't answer me. The mainboard manufacturers say they can't 
give information because of nVidia.

Bye,
Sebastian

ATXP1 kernel patch

[Sebastian Witt]

I've updated the module:
http://www.hasw.net/linux/atxp1-0.3.patch

Regards,
Sebastian

ATXP1 kernel patch

[Jean Delvare]

> I've updated the module:
> http://www.hasw.net/linux/atxp1-0.3.patch

Comments:

1* Please respect the alphabetical order in i2c/chips/Makefile.

2* You include linux/config.h but don't seem to use it.

3* What is this "low_voltage" module parameter?

4* I am really suspicious that the atxp1 has 0x37 and 0x4e as possible
addresses. How do you know?

5* normal_i2c_range is gone in Linux 2.6.10.

6* Don't use .id in i2c_driver atxp1_driver, you don't need it.

7* Other drivers don't call I2C_CLIENT_INSMOD directly.

8* In the current implementation, atxp1_data.valid is usless (value is
always 0). I also think that you should use restore and use
atxp1_data.last_updated.

9* What's the difference between VID and CPU VID?

10* You hardcode the VID <-> Vcore translation formula. Depending on the
VRM version, it might or might not be correct. You should use the
functions in i2c-sensor-vid.

11* atxp1_write is quite complex. What makes you believe that the write
could possibly fail? Other i2c drivers don't do that kind of check.

12* Why does vcore default to 1.650V in atxp1_storevcore? Why is vcore
arbitrarily kept between 1.075V/1.100V and 1.850V? If trying to write a
value outside of this range is an error, then atxp1_storevcore should
return an error (negative value) when it happens.

13* Use dev_info/dev_dbg etc... instead of printk wherever possible.

14* Care to comment/document the final loops in atxp1_storevcore? Since
atxp1_write is complex, I fear that these loops will be damn slow. Why
are they needed at all?

15* Why does atxp1_storevcore set data->valid to 0?

16* Sysfs file name should probably be cpu0_vid, not cpu_vid, although I
admit I don't know how to properly handle the case where more than one
device is present.

17* Don't set new_client->id to 0 in atxp1_detect, it's useless.

18* You detection method is too weak and will most likely result in
frequent misdetections. You have to improve it. I know that the device
doesn't have a decidated identification register, nor does it have many
registers to test, but we can certainly do something. Which registers
does the device have exactly? What happens when reading from
non-existent registers? The output of "i2cdump" may help.

19* atxp1_detach_client frees the client data even if the deregistration
failed. This is probably dangerous.

20* init and exit functions are not properly marked __init and __exit.

Good work nonetheless, this device is completely different from what we
have seen before so it's quite normal that some things need to be
discussed and tweaked before you/we get them right.

Thanks,
-- 
Jean Delvare
http://khali.linux-fr.org/

ATXP1 kernel patch

[Sebastian Witt]

Jean Delvare wrote:
 >>I've updated the module:
 >>http://www.hasw.net/linux/atxp1-0.3.patch
 >
 >
 > Comments:
 >
 > 1* Please respect the alphabetical order in i2c/chips/Makefile.
 >
 > 2* You include linux/config.h but don't seem to use it.
1-2 Ok.

 > 3* What is this "low_voltage" module parameter?
It allows setting of 1075mV which is not available on all platforms. 
Think this is removeable when using i2c-sensor-vid?

 > 4* I am really suspicious that the atxp1 has 0x37 and 0x4e as possible
 > addresses. How do you know?
On all mainboards (full list under http://www.hasw.net) I've seen, the 
ATXP1 is using 0x37 or 0x4e. If someone has a board with an ATXP1 which 
is using a different address, I think it can easily be added.

 > 5* normal_i2c_range is gone in Linux 2.6.10.
 >
 > 6* Don't use .id in i2c_driver atxp1_driver, you don't need it.
 >
 > 7* Other drivers don't call I2C_CLIENT_INSMOD directly.
Ok.


 > 8* In the current implementation, atxp1_data.valid is usless (value is
 > always 0). I also think that you should use restore and use
 > atxp1_data.last_updated.
Already fixed that, now using in atxp1_update_device:

# Update register data if needed
for(count = 0; count <= 0x0a; count++) {
     if((data->valid >> count & 1) = 0) {
         data->reg[count] = i2c_smbus_read_byte_data(client, count);
         data->valid |= 1 << count;
     }
}

 > 9* What's the difference between VID and CPU VID?
VID is the VID output control, enables or disables VID output control.
CPU VID are the original VID pins from the CPU.

 > 10* You hardcode the VID <-> Vcore translation formula. Depending on the
 > VRM version, it might or might not be correct. You should use the
 > functions in i2c-sensor-vid.
Ok.

 > 11* atxp1_write is quite complex. What makes you believe that the write
 > could possibly fail? Other i2c drivers don't do that kind of check.
Ported code. I'll remove it.

 > 12* Why does vcore default to 1.650V in atxp1_storevcore? Why is vcore
 > arbitrarily kept between 1.075V/1.100V and 1.850V? If trying to write a
 > value outside of this range is an error, then atxp1_storevcore should
 > return an error (negative value) when it happens.
1.075V-1.85V is the VRM range that is available on my 8RDA+ board. 
Greater range isn't possible.

 > 13* Use dev_info/dev_dbg etc... instead of printk wherever possible.
Ok.

 > 14* Care to comment/document the final loops in atxp1_storevcore? Since
 > atxp1_write is complex, I fear that these loops will be damn slow. Why
 > are they needed at all?
Trying to set CPU voltage in greater steps will freeze the CPU 
immediately (>+/-125mV, less under high CPU load). By in-/decrease the 
voltage in 25mV step the stability is increased. On my PC a script 
controls the CPU frequency and voltage and I do not recongize if it 
switches multiple times between 1125mV and 1650mV in a minute for 
testing. Ok that's subjective but could you define "slow" a little bit 
more please?

 > 15* Why does atxp1_storevcore set data->valid to 0?
So it only updates the local data if there was an write.
Is already replaced.

 > 16* Sysfs file name should probably be cpu0_vid, not cpu_vid, although I
 > admit I don't know how to properly handle the case where more than one
 > device is present.
Not thought on this, but is there a MP system which uses two of theses 
chips to
let the user independently control the CPU core voltages? But it should 
be modified.

 > 17* Don't set new_client->id to 0 in atxp1_detect, it's useless.
Ok.

 > 18* You detection method is too weak and will most likely result in
 > frequent misdetections. You have to improve it. I know that the device
 > doesn't have a decidated identification register, nor does it have many
 > registers to test, but we can certainly do something. Which registers
 > does the device have exactly? What happens when reading from
 > non-existent registers? The output of "i2cdump" may help.
Yes, that's right. I've attached the dump. Maybe using 0x0b-0x0f (shows 
all zero), 0x10-0x1a showing same value as 0x00 and >0x20 all zero.

 > 19* atxp1_detach_client frees the client data even if the deregistration
 > failed. This is probably dangerous.
Ok, fixed.

 > 20* init and exit functions are not properly marked __init and __exit.
also.

 > Good work nonetheless, this device is completely different from what we
 > have seen before so it's quite normal that some things need to be
 > discussed and tweaked before you/we get them right.

Thanks for your help.

Regards,
Sebastian
-------------- next part --------------
No size specified (using byte-data access)
  WARNING! This program can confuse your I2C bus, cause data loss and worse!
  I will probe file /dev/i2c-0, address 0x37, mode byte
  You have five seconds to reconsider and press CTRL-C!

     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 29 0a 1c 0c 0c 81 96 03 00 00 fb 00 00 00 00 00    )???????..?.....
10: 29 29 29 29 29 29 29 29 29 29 29 09 09 08 08 08    )))))))))))?????
20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................

ATXP1 kernel patch

[Jean Delvare]

> > 3* What is this "low_voltage" module parameter?
> It allows setting of 1075mV which is not available on all platforms. 
> Think this is removeable when using i2c-sensor-vid?

1075mV isn't valid under VRM 9.x according to the specs, so I really
wonder how it can work on some plateforms. I would happily see it
removed to keep the code more simple. And i2c-sensor-vid will not
support it anyway.

As a side note, i2c-sensor-vid.c and i2c-vid.h were written with VID pin
*read* in mind, not write, so you might have to improve them a bit for
your own needs.

>  > 8* In the current implementation, atxp1_data.valid is usless (value
>  > is always 0). I also think that you should use restore and use
>  > atxp1_data.last_updated.
> Already fixed that, now using in atxp1_update_device:
> 
> # Update register data if needed
> for(count = 0; count <= 0x0a; count++) {
>      if((data->valid >> count & 1) = 0) {
>          data->reg[count] = i2c_smbus_read_byte_data(client, count);
>          data->valid |= 1 << count;
>      }
> }

Huh. Looks even more complex. Do you really need to read all 10 first
registers, while you were previously only reading 2? What is the benefit
of having a separate "valid" bit for each register since you always read
them all at once anyway?

>  > 9* What's the difference between VID and CPU VID?
> VID is the VID output control, enables or disables VID output control.
> CPU VID are the original VID pins from the CPU.

That would need to be documented in the code. Actually, the whole way
the chip works should be, since there is no public datasheet people can
look at.

>  > 10* You hardcode the VID <-> Vcore translation formula. Depending
>  > on the VRM version, it might or might not be correct. You should
>  > use the functions in i2c-sensor-vid.
> Ok.

Note that you won't be able to support VRM 10 since it uses 6-bit VID,
but all previous versions should work fine.

>  > 12* Why does vcore default to 1.650V in atxp1_storevcore? Why is
>  > vcore arbitrarily kept between 1.075V/1.100V and 1.850V? If trying
>  > to write a value outside of this range is an error, then
>  > atxp1_storevcore should return an error (negative value) when it
>  > happens.
> 1.075V-1.85V is the VRM range that is available on my 8RDA+ board. 
> Greater range isn't possible.

In other words, it uses VRM 9.x. You will need to add validity checkings
based on VRM version instead, providing you actually want to support
other VRM versions.

>  > 14* Care to comment/document the final loops in atxp1_storevcore?
>  > Since atxp1_write is complex, I fear that these loops will be damn
>  > slow. Why are they needed at all?
> Trying to set CPU voltage in greater steps will freeze the CPU 
> immediately (>+/-125mV, less under high CPU load). By in-/decrease
> the voltage in 25mV step the stability is increased. On my PC a
> script controls the CPU frequency and voltage and I do not recongize
> if it switches multiple times between 1125mV and 1650mV in a minute
> for testing. Ok that's subjective but could you define "slow" a
> little bit more please?

OK, that makes sense. Please add a comment explaining this in the code.
By slow I mean that SMBus transfer are slows (the bus is typically
operating at 15kHz), and since atxp1_write does 2 transfers and can be
called a dozen times to change the vid output once, it looked like it
would be slower that needed. However, if there is a good reason to do it
in an incremental way, fine with me. The operation isn't time critical
and we want it to be safe, agreed. But at least this is a good reason to
get rid of the readback test in atxp1_write.

Please note that the trick of increasing the vid value by one to
linearly decrease vcore works with VRM 9.x but wouldn't work with all
VRM 8.x, which are more complex. This might be a sufficient reason not
to support VRM 8.x (since we don't know of any system using both VRM 8.x
and an ATXP1). The fact that you will use i2c-sensor-vid doesn't mean
that you have to support all VRM versions, just that you will avoid code
duplication. You can check the VRM version and abort if it is anything
you don't support (i.e. non-9.x for now).

>  > 16* Sysfs file name should probably be cpu0_vid, not cpu_vid,
>  > although I admit I don't know how to properly handle the case where
>  > more than one device is present.
> Not thought on this, but is there a MP system which uses two of theses
> chips to let the user independently control the CPU core voltages? But
> it should be modified.

I don't know if this exist, but it certainly could. If both CPUs are
handled by an ATXP1 (and thus the same driver), we could have a common
static counter so that the first client creates cpu0_vid and the second
would create cpu1_vid.

> 
>  > 17* Don't set new_client->id to 0 in atxp1_detect, it's useless.
> Ok.
> 
>  > 18* You detection method is too weak and will most likely result in
>  > frequent misdetections. You have to improve it. I know that the
>  > device doesn't have a decidated identification register, nor does
>  > it have many registers to test, but we can certainly do something.
>  > Which registers does the device have exactly? What happens when
>  > reading from non-existent registers? The output of "i2cdump" may
>  > help.
> Yes, that's right. I've attached the dump. Maybe using 0x0b-0x0f
> (shows  all zero), 0x10-0x1a showing same value as 0x00 and >0x20 all
> zero.

Checking that random registers are 0 won't be very efficient because
that's the default value for most chips. However, some specific
registers would be worth checking: 0x3e, 0x3f, 0xfe, 0xff. These are the
locations of device id and manufacturer id registers for most chips, so
checking that they have value 0 is certainly great. It will avoid
attaching to the various temperature sensors that can live at 0x4e.

The fact that registers 0x10-0x1a return the same value as 0x00 will
also be a very efficient check. You don't have to check them all, a few
of them will do (remember that each additional read will slow down the
detection and increase the module load time as a result).

Your original check (CVID > 0) should probably be removed, as it doesn't
seem to be valid (0 is a valid VID value, 1.850V under VRM 9.x).

Thanks,
-- 
Jean Delvare
http://khali.linux-fr.org/

ATXP1 kernel patch

[Sebastian Witt]

Jean Delvare wrote:
> 
> 1075mV isn't valid under VRM 9.x according to the specs, so I really
> wonder how it can work on some plateforms. I would happily see it
> removed to keep the code more simple. And i2c-sensor-vid will not
> support it anyway.

Ok, removed. The IRU3055 converter controller IC used on the 8RDA boards 
allows setting 1.075V.

> As a side note, i2c-sensor-vid.c and i2c-vid.h were written with VID pin
> *read* in mind, not write, so you might have to improve them a bit for
> your own needs.

Something like in the attached patch? I've not added the other VRMs yet, 
first need to know if the ATXP1 is used with other VRMs.

>># Update register data if needed
>>for(count = 0; count <= 0x0a; count++) {
>>     if((data->valid >> count & 1) = 0) {
>>         data->reg[count] = i2c_smbus_read_byte_data(client, count);
>>         data->valid |= 1 << count;
>>     }
>>}
> 
> 
> Huh. Looks even more complex. Do you really need to read all 10 first
> registers, while you were previously only reading 2? What is the benefit
> of having a separate "valid" bit for each register since you always read
> them all at once anyway?

For future versions, which are using more (all registers). For general 
purpose IOs (used to control other voltages, extra functions) and 
watchdog timer. Also a register variable gets only updated at start or
when a value was written.

> That would need to be documented in the code. Actually, the whole way
> the chip works should be, since there is no public datasheet people can
> look at.

Ok, I'll add more comments.

> 
> Note that you won't be able to support VRM 10 since it uses 6-bit VID,
> but all previous versions should work fine.
> 

I don't know if the ATXP1 is used on boards with < 9.0 at all. Would be 
interesting
if somebody knows.

> 
> In other words, it uses VRM 9.x. You will need to add validity checkings
> based on VRM version instead, providing you actually want to support
> other VRM versions.

See current patches.

> 
> OK, that makes sense. Please add a comment explaining this in the code.
> By slow I mean that SMBus transfer are slows (the bus is typically
> operating at 15kHz), and since atxp1_write does 2 transfers and can be
> called a dozen times to change the vid output once, it looked like it
> would be slower that needed. However, if there is a good reason to do it
> in an incremental way, fine with me. The operation isn't time critical
> and we want it to be safe, agreed. But at least this is a good reason to
> get rid of the readback test in atxp1_write.
> 
> Please note that the trick of increasing the vid value by one to
> linearly decrease vcore works with VRM 9.x but wouldn't work with all
> VRM 8.x, which are more complex. This might be a sufficient reason not
> to support VRM 8.x (since we don't know of any system using both VRM 8.x
> and an ATXP1). The fact that you will use i2c-sensor-vid doesn't mean
> that you have to support all VRM versions, just that you will avoid code
> duplication. You can check the VRM version and abort if it is anything
> you don't support (i.e. non-9.x for now).
> 

Yes, see patch.

> I don't know if this exist, but it certainly could. If both CPUs are
> handled by an ATXP1 (and thus the same driver), we could have a common
> static counter so that the first client creates cpu0_vid and the second
> would create cpu1_vid.

Not added yet, next patch.

> 
> Checking that random registers are 0 won't be very efficient because
> that's the default value for most chips. However, some specific
> registers would be worth checking: 0x3e, 0x3f, 0xfe, 0xff. These are the
> locations of device id and manufacturer id registers for most chips, so
> checking that they have value 0 is certainly great. It will avoid
> attaching to the various temperature sensors that can live at 0x4e.
> 
> The fact that registers 0x10-0x1a return the same value as 0x00 will
> also be a very efficient check. You don't have to check them all, a few
> of them will do (remember that each additional read will slow down the
> detection and increase the module load time as a result).
> 
> Your original check (CVID > 0) should probably be removed, as it doesn't
> seem to be valid (0 is a valid VID value, 1.850V under VRM 9.x).
> 

Done.

Bye,
Sebastian

-------------- next part --------------
diff -ruN linux-2.6.10-orig/drivers/i2c/chips/Kconfig linux-2.6.10/drivers/i2c/chips/Kconfig
--- linux-2.6.10-orig/drivers/i2c/chips/Kconfig	2005-01-02 21:41:37.000000000 +0100
+++ linux-2.6.10/drivers/i2c/chips/Kconfig	2005-01-02 16:54:50.000000000 +0100
@@ -347,6 +347,19 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-rtc8564.
 
+config SENSORS_ATXP1
+	tristate "Attansic ATXP1 VID controller"
+	depends on I2C && EXPERIMENTAL
+	help
+	  If you say yes here you get support for the Attansic ATXP1 VID
+	  controller.
+
+	  If your board have such a chip, you are able to control your CPU
+	  core and other voltages.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called atxp1.
+	  
 config ISP1301_OMAP
 	tristate "Philips ISP1301 with OMAP OTG"
 	depends on I2C && ARCH_OMAP_OTG
diff -ruN linux-2.6.10-orig/drivers/i2c/chips/Makefile linux-2.6.10/drivers/i2c/chips/Makefile
--- linux-2.6.10-orig/drivers/i2c/chips/Makefile	2005-01-02 21:41:37.000000000 +0100
+++ linux-2.6.10/drivers/i2c/chips/Makefile	2005-01-19 20:10:13.223745584 +0100
@@ -11,6 +11,7 @@
 obj-$(CONFIG_SENSORS_ADM1025)	+= adm1025.o
 obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
 obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
+obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
 obj-$(CONFIG_SENSORS_FSCHER)	+= fscher.o
diff -ruN linux-2.6.10-orig/drivers/i2c/chips/atxp1.c linux-2.6.10/drivers/i2c/chips/atxp1.c
--- linux-2.6.10-orig/drivers/i2c/chips/atxp1.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.10/drivers/i2c/chips/atxp1.c	2005-01-19 20:57:29.126622816 +0100
@@ -0,0 +1,356 @@
+/*
+    atxp1.c - kernel module for setting Vcore using ATXP1 chip. 
+    
+    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.
+    
+    Version history:
+    0.1: - Converted 8rdavcore into kernel module (Marcin Kaluza <marcin_ml@sekretarka.no-ip.org>)
+    0.2: - Cleanup, general interface for VCore (Sebastian Witt <hasw@hasw.net>)
+    0.3: - Fixed VID calculation (no FP operation)
+         - Fixed VID setting loop
+         - Added update lock
+         - Renamed device file to cpu_vid
+         - Added low_voltage module option to set lowest possible voltage (1075mV or 1100mV)
+    0.4: - Added GPIO[1,2]
+        - Using I2C VRM functions
+        - Changed detection
+        - Some other small fixes
+*/
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/i2c.h>
+#include <linux/i2c-sensor.h>
+#include <linux/i2c-vid.h>
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("System voltages control via Attansic ATXP1");
+MODULE_VERSION("0.4");
+
+#define ATXP1_VID	0x00
+#define ATXP1_CVID	0x01
+#define ATXP1_GPIO1	0x06
+#define ATXP1_GPIO2	0x0a
+#define ATXP1_VIDENA	0x20
+#define ATXP1_VIDMASK	0x1f
+#define ATXP1_GPIO1MASK	0x0f
+
+static unsigned short normal_i2c[]= { 0x37, 0x4e, I2C_CLIENT_END };
+static unsigned int normal_isa[] = { I2C_CLIENT_ISA_END };
+static int atxp1_attach_adapter(struct i2c_adapter * adapter);
+static int atxp1_detach_client(struct i2c_client * client);
+static struct atxp1_data * atxp1_update_device(struct device *dev);
+static int atxp1_detect(struct i2c_adapter *adapter, int address, int kind);
+
+static struct i2c_driver atxp1_driver = {
+	.owner		= THIS_MODULE,
+	.name		= "atxp1",
+	.flags		= I2C_DF_NOTIFY,
+	.attach_adapter = atxp1_attach_adapter,
+	.detach_client	= atxp1_detach_client,
+};
+
+SENSORS_INSMOD_1(atxp1);
+
+struct atxp1_data {
+	struct i2c_client client;
+	struct semaphore update_lock;
+	u16 valid;
+	unsigned char reg[0x0f];
+	u8 vrm;
+};
+
+static struct atxp1_data * atxp1_update_device(struct device *dev) 
+{
+	struct i2c_client *client;
+	struct atxp1_data *data;
+	unsigned char count;
+	
+	client = to_i2c_client(dev);
+	data = i2c_get_clientdata(client);
+	
+	down(&data->update_lock);
+	
+	/* Update local register data only if register was written */
+	for(count = 0; count <= 0x0a; count++) {
+		if((data->valid >> count & 1) = 0) {
+			data->reg[count] = i2c_smbus_read_byte_data(client, count);
+			data->valid |= 1 << count;
+		}
+	}
+    	
+	up(&data->update_lock);
+	
+	return(data);
+}
+
+static int atxp1_write(struct i2c_client *client, unsigned char adr, unsigned char data) 
+{
+	int ret = -1;
+	
+	ret = i2c_smbus_write_byte_data(client, adr, data);
+	
+	if(ret < 0) {
+		dev_err(&client->dev, "Write failed.\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+/* Device file functions for VCore */
+ssize_t atxp1_showvcore(struct device *dev, char *buf)
+{
+	int size;
+	struct atxp1_data *data;
+	
+	data = atxp1_update_device(dev);
+	
+	size = sprintf(buf, "%d\n", vid_from_reg(data->reg[ATXP1_VID] & ATXP1_VIDMASK, data->vrm));
+
+	return size;
+}
+
+ssize_t atxp1_storevcore(struct device *dev, const char* buf, size_t count)
+{
+	struct atxp1_data *data;
+	struct i2c_client *client;
+	char vid;
+	char cvid;
+	unsigned int vcore;
+
+	client = to_i2c_client(dev);
+	data = atxp1_update_device(dev);
+
+	vcore = simple_strtoul(buf, NULL, 10);
+	vcore /= 25;	
+	vcore *= 25;	
+	
+	/* Calculate VID */
+	vid = reg_from_vid(vcore, data->vrm);
+	
+	if(vid < 0) {
+		dev_err(dev, "VID calculation failed.\n");
+		return -1;
+	}
+	
+	/* If output enabled, use control register value. Otherwise original CPU VID */
+	if(data->reg[ATXP1_VID] & ATXP1_VIDENA)
+		cvid = data->reg[ATXP1_VID] & ATXP1_VIDMASK;
+	else
+		cvid = data->reg[ATXP1_CVID];
+	
+	/* Nothing changed, aborting */
+	if(vid = cvid)
+		return strlen(buf);
+	
+	dev_info(dev, "Setting VCore to %d mV (0x%02x)\n", vcore, vid);
+	
+	/* Write every 25 mV step to increase stability */
+	if(cvid > vid) {
+		for(; cvid >= vid; cvid--) {
+        		atxp1_write(client, ATXP1_VID, cvid | ATXP1_VIDENA);
+		}
+	}
+	else {
+		for(; cvid <= vid; cvid++) {
+        		atxp1_write(client, ATXP1_VID, cvid | ATXP1_VIDENA);
+		}
+	}
+	
+	data->valid &= ~1;
+	
+	return strlen(buf);
+}
+
+/* CPU core reference voltage
+    unit: millivolt
+*/
+static DEVICE_ATTR(cpu0_vid, S_IRUGO | S_IWUSR, atxp1_showvcore, atxp1_storevcore);
+
+/* Device file functions for GPIO1 */
+ssize_t atxp1_showgpio1(struct device *dev, char *buf)
+{
+	int size;
+	struct atxp1_data *data;
+	
+	data = atxp1_update_device(dev);
+	
+	size = sprintf(buf, "0x%02x\n", data->reg[ATXP1_GPIO1] & ATXP1_GPIO1MASK);
+
+	return size;
+}
+
+ssize_t atxp1_storegpio1(struct device *dev, const char* buf, size_t count)
+{
+	struct atxp1_data *data;
+	struct i2c_client *client;
+	unsigned int value;
+	
+	client = to_i2c_client(dev);
+	data = atxp1_update_device(dev);
+
+	value = simple_strtoul(buf, NULL, 16);
+	
+	value &= ATXP1_GPIO1MASK;
+	
+	dev_info(dev, "Writing 0x%x to GPIO1.\n", value);
+	
+	if(value != (data->reg[ATXP1_GPIO1] & ATXP1_GPIO1MASK)) {
+		atxp1_write(client, ATXP1_GPIO1, value);
+		data->valid &= ~(1 << ATXP1_GPIO1);
+	}
+	
+	return strlen(buf);
+}
+
+static DEVICE_ATTR(gpio1, S_IRUGO | S_IWUSR, atxp1_showgpio1, atxp1_storegpio1);
+
+/* Device file functions for GPIO2 */
+ssize_t atxp1_showgpio2(struct device *dev, char *buf)
+{
+	int size;
+	struct atxp1_data *data;
+	
+	data = atxp1_update_device(dev);
+	
+	size = sprintf(buf, "0x%02x\n", data->reg[ATXP1_GPIO2]);
+
+	return size;
+}
+
+ssize_t atxp1_storegpio2(struct device *dev, const char* buf, size_t count)
+{
+	struct atxp1_data *data;
+	struct i2c_client *client;
+	unsigned int value;
+	
+	client = to_i2c_client(dev);
+	data = atxp1_update_device(dev);
+
+	value = simple_strtoul(buf, NULL, 16) & 0xff;
+	
+	dev_info(dev, "Writing 0x%x to GPIO1.\n", value);
+	
+	if(value != data->reg[ATXP1_GPIO2]) {
+		atxp1_write(client, ATXP1_GPIO2, value);
+		data->valid &= ~(1 << ATXP1_GPIO2);
+	}
+	
+	return strlen(buf);
+}
+
+static DEVICE_ATTR(gpio2, S_IRUGO | S_IWUSR, atxp1_showgpio2, atxp1_storegpio2);
+
+
+static int atxp1_attach_adapter(struct i2c_adapter *adapter)
+{
+	return i2c_detect(adapter, &addr_data, &atxp1_detect);
+};
+	
+static int atxp1_detect(struct i2c_adapter *adapter, int address, int kind)
+{
+	struct i2c_client* new_client;
+	struct atxp1_data * data;
+	int err;
+	u8 temp;
+
+	if (!(data = kmalloc(sizeof(struct atxp1_data), GFP_KERNEL)))
+		return -ENOMEM;
+
+	memset(data, 0, sizeof(struct atxp1_data));
+	new_client = &data->client;
+	i2c_set_clientdata(new_client, data);
+	
+	new_client->addr = address;
+	new_client->adapter = adapter;
+	new_client->driver = &atxp1_driver;
+	new_client->flags = 0;
+	
+	/* Detect ATXP1, checking if reg. 0x10,0x11 same as reg. 0x00 and vendor
+	 * ID registers are all zero */
+	temp = i2c_smbus_read_byte_data(new_client, ATXP1_VID);
+	if(!((i2c_smbus_read_byte_data(new_client, 0x10) = temp) &&
+	   (i2c_smbus_read_byte_data(new_client, 0x11) = temp) &&
+	   (i2c_smbus_read_byte_data(new_client, 0x3e) = 0x00) &&
+	   (i2c_smbus_read_byte_data(new_client, 0x3f) = 0x00) &&
+	   (i2c_smbus_read_byte_data(new_client, 0xfe) = 0x00) &&
+	   (i2c_smbus_read_byte_data(new_client, 0xff) = 0x00)	)) {
+		kfree(data);
+		return 0;
+	}
+	
+	strncpy(new_client->name, "atxp1", I2C_NAME_SIZE);
+
+	data->valid = 0;
+	
+	init_MUTEX(&data->update_lock);
+	
+	if ((err = i2c_attach_client(new_client)))
+	{
+		dev_err(&new_client->dev, "Attach client error.\n");
+		kfree(data);
+		return err;
+	}
+	
+	device_create_file(&new_client->dev, &dev_attr_gpio1);
+	device_create_file(&new_client->dev, &dev_attr_gpio2);
+	device_create_file(&new_client->dev, &dev_attr_cpu0_vid);
+	
+	/* Get VRM */
+	data->vrm = i2c_which_vrm();
+	
+	if(data->vrm != 90) {
+		dev_err(&new_client->dev, "Not supporting VRM %d\n", data->vrm);
+		return -1;
+	}
+	
+	dev_info(&new_client->dev, "Detected on %s. Using VRM: %d.%d\n", 
+			 adapter->name, data->vrm / 10, data->vrm % 10);
+	
+	return 0;
+};
+
+static int atxp1_detach_client(struct i2c_client * client)
+{
+	int err;
+	
+	err = i2c_detach_client(client);
+	
+	if (err) 
+		dev_err(&client->dev, "Failed to detach client.\n");
+	else
+		kfree(i2c_get_clientdata(client));
+	
+	return 0;
+};
+ 
+static int __init atxp1_init(void)  
+{ 
+	i2c_add_driver(&atxp1_driver);
+	
+	return 0; 
+};
+
+static void __exit atxp1_exit(void) 
+{ 
+	i2c_del_driver(&atxp1_driver);
+};
+
+module_init(atxp1_init);
+module_exit(atxp1_exit);
+
-------------- next part --------------
--- linux-2.6.10-orig/include/linux/i2c-vid.h	2004-12-24 22:33:49.000000000 +0100
+++ linux-2.6.10/include/linux/i2c-vid.h	2005-01-19 20:12:26.840432760 +0100
@@ -97,3 +97,22 @@
 		                     2050 - (val) * 50);
 	}
 }
+
+static inline int reg_from_vid(int val, int vrm)
+{
+	
+	switch(vrm) {
+
+	case  0:
+		return -1;
+
+	case 91:		/* VRM 9.1 */
+	case 90:		/* VRM 9.0 */
+		return ((val >= 1100) && (val <= 1850) ?
+				((18500 - val * 10) / 25 + 5) / 10 : -1);
+	
+	default:
+		return -1;
+
+	}
+}

ATXP1 kernel patch

[Sebastian Witt]

I want to add two things to my last message:

- The VRM check in atxp1_detect was moved above i2c_attach_client.

- I've channged the detection method, because
..
temp = i2c_smbus_read_byte_data(new_client, ATXP1_VID);
if(!((i2c_smbus_read_byte_data(new_client, 0x10) = temp) &&
    (i2c_smbus_read_byte_data(new_client, 0x11) = temp)
...

the register 0x10,0x11 aren't the same as 0x00 all time (the dump say 
it, but I think that's because the device is dumped from 0x00 upwards). 
If you directly read 0x10, 0x11 they are also zero.

Instead I'm checking register 0x05, bit 7. It is always set:
..
if(!(i2c_smbus_read_byte_data(new_client, 0x05) & 0x80))
..

Regards,
Sebastian
-------------- next part --------------
/*
    atxp1.c - kernel module for setting Vcore using ATXP1 chip. 
    
    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.
    
    Version history:
    0.1: - Converted 8rdavcore into kernel module (Marcin Kaluza <marcin_ml@sekretarka.no-ip.org>)
    0.2: - Cleanup, general interface for VCore (Sebastian Witt <hasw@hasw.net>)
    0.3: - Fixed VID calculation (no FP operation)
         - Fixed VID setting loop
         - Added update lock
         - Renamed device file to cpu_vid
         - Added low_voltage module option to set lowest possible voltage (1075mV or 1100mV)
    0.4: - Added GPIO[1,2]
        - Using I2C VRM functions
        - Changed detection
        - Some other small fixes
*/

#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/i2c.h>
#include <linux/i2c-sensor.h>
#include <linux/i2c-vid.h>

MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("System voltages control via Attansic ATXP1");
MODULE_VERSION("0.4");

#define ATXP1_VID	0x00
#define ATXP1_CVID	0x01
#define ATXP1_GPIO1	0x06
#define ATXP1_GPIO2	0x0a
#define ATXP1_VIDENA	0x20
#define ATXP1_VIDMASK	0x1f
#define ATXP1_GPIO1MASK	0x0f

static unsigned short normal_i2c[]= { 0x37, 0x4e, I2C_CLIENT_END };
static unsigned int normal_isa[] = { I2C_CLIENT_ISA_END };
static int atxp1_attach_adapter(struct i2c_adapter * adapter);
static int atxp1_detach_client(struct i2c_client * client);
static struct atxp1_data * atxp1_update_device(struct device *dev);
static int atxp1_detect(struct i2c_adapter *adapter, int address, int kind);

static struct i2c_driver atxp1_driver = {
	.owner		= THIS_MODULE,
	.name		= "atxp1",
	.flags		= I2C_DF_NOTIFY,
	.attach_adapter = atxp1_attach_adapter,
	.detach_client	= atxp1_detach_client,
};

SENSORS_INSMOD_1(atxp1);

struct atxp1_data {
	struct i2c_client client;
	struct semaphore update_lock;
	u16 valid;
	unsigned char reg[0x0f];
	u8 vrm;
};

static struct atxp1_data * atxp1_update_device(struct device *dev) 
{
	struct i2c_client *client;
	struct atxp1_data *data;
	unsigned char count;
	
	client = to_i2c_client(dev);
	data = i2c_get_clientdata(client);
	
	down(&data->update_lock);
	
	/* Update local register data only if register was written */
	for(count = 0; count <= 0x0a; count++) {
		if((data->valid >> count & 1) = 0) {
			data->reg[count] = i2c_smbus_read_byte_data(client, count);
			data->valid |= 1 << count;
		}
	}
    	
	up(&data->update_lock);
	
	return(data);
}

static int atxp1_write(struct i2c_client *client, unsigned char adr, unsigned char data) 
{
	int ret = -1;
	
	ret = i2c_smbus_write_byte_data(client, adr, data);
	
	if(ret < 0) {
		dev_err(&client->dev, "Write failed.\n");
		return -1;
	}

	return 0;
}

/* Device file functions for VCore */
ssize_t atxp1_showvcore(struct device *dev, char *buf)
{
	int size;
	struct atxp1_data *data;
	
	data = atxp1_update_device(dev);
	
	size = sprintf(buf, "%d\n", vid_from_reg(data->reg[ATXP1_VID] & ATXP1_VIDMASK, data->vrm));

	return size;
}

ssize_t atxp1_storevcore(struct device *dev, const char* buf, size_t count)
{
	struct atxp1_data *data;
	struct i2c_client *client;
	char vid;
	char cvid;
	unsigned int vcore;

	client = to_i2c_client(dev);
	data = atxp1_update_device(dev);

	vcore = simple_strtoul(buf, NULL, 10);
	vcore /= 25;	
	vcore *= 25;	
	
	/* Calculate VID */
	vid = reg_from_vid(vcore, data->vrm);
	
	if(vid < 0) {
		dev_err(dev, "VID calculation failed.\n");
		return -1;
	}
	
	/* If output enabled, use control register value. Otherwise original CPU VID */
	if(data->reg[ATXP1_VID] & ATXP1_VIDENA)
		cvid = data->reg[ATXP1_VID] & ATXP1_VIDMASK;
	else
		cvid = data->reg[ATXP1_CVID];
	
	/* Nothing changed, aborting */
	if(vid = cvid)
		return strlen(buf);
	
	dev_info(dev, "Setting VCore to %d mV (0x%02x)\n", vcore, vid);
	
	/* Write every 25 mV step to increase stability */
	if(cvid > vid) {
		for(; cvid >= vid; cvid--) {
        		atxp1_write(client, ATXP1_VID, cvid | ATXP1_VIDENA);
		}
	}
	else {
		for(; cvid <= vid; cvid++) {
        		atxp1_write(client, ATXP1_VID, cvid | ATXP1_VIDENA);
		}
	}
	
	data->valid &= ~1;
	
	return strlen(buf);
}

/* CPU core reference voltage
    unit: millivolt
*/
static DEVICE_ATTR(cpu0_vid, S_IRUGO | S_IWUSR, atxp1_showvcore, atxp1_storevcore);

/* Device file functions for GPIO1 */
ssize_t atxp1_showgpio1(struct device *dev, char *buf)
{
	int size;
	struct atxp1_data *data;
	
	data = atxp1_update_device(dev);
	
	size = sprintf(buf, "0x%02x\n", data->reg[ATXP1_GPIO1] & ATXP1_GPIO1MASK);

	return size;
}

ssize_t atxp1_storegpio1(struct device *dev, const char* buf, size_t count)
{
	struct atxp1_data *data;
	struct i2c_client *client;
	unsigned int value;
	
	client = to_i2c_client(dev);
	data = atxp1_update_device(dev);

	value = simple_strtoul(buf, NULL, 16);
	
	value &= ATXP1_GPIO1MASK;
	
	dev_info(dev, "Writing 0x%x to GPIO1.\n", value);
	
	if(value != (data->reg[ATXP1_GPIO1] & ATXP1_GPIO1MASK)) {
		atxp1_write(client, ATXP1_GPIO1, value);
		data->valid &= ~(1 << ATXP1_GPIO1);
	}
	
	return strlen(buf);
}

static DEVICE_ATTR(gpio1, S_IRUGO | S_IWUSR, atxp1_showgpio1, atxp1_storegpio1);

/* Device file functions for GPIO2 */
ssize_t atxp1_showgpio2(struct device *dev, char *buf)
{
	int size;
	struct atxp1_data *data;
	
	data = atxp1_update_device(dev);
	
	size = sprintf(buf, "0x%02x\n", data->reg[ATXP1_GPIO2]);

	return size;
}

ssize_t atxp1_storegpio2(struct device *dev, const char* buf, size_t count)
{
	struct atxp1_data *data;
	struct i2c_client *client;
	unsigned int value;
	
	client = to_i2c_client(dev);
	data = atxp1_update_device(dev);

	value = simple_strtoul(buf, NULL, 16) & 0xff;
	
	dev_info(dev, "Writing 0x%x to GPIO1.\n", value);
	
	if(value != data->reg[ATXP1_GPIO2]) {
		atxp1_write(client, ATXP1_GPIO2, value);
		data->valid &= ~(1 << ATXP1_GPIO2);
	}
	
	return strlen(buf);
}

static DEVICE_ATTR(gpio2, S_IRUGO | S_IWUSR, atxp1_showgpio2, atxp1_storegpio2);


static int atxp1_attach_adapter(struct i2c_adapter *adapter)
{
	return i2c_detect(adapter, &addr_data, &atxp1_detect);
};
	
static int atxp1_detect(struct i2c_adapter *adapter, int address, int kind)
{
	struct i2c_client* new_client;
	struct atxp1_data * data;
	int err;

	if (!(data = kmalloc(sizeof(struct atxp1_data), GFP_KERNEL)))
		return -ENOMEM;

	memset(data, 0, sizeof(struct atxp1_data));
	new_client = &data->client;
	i2c_set_clientdata(new_client, data);
	
	new_client->addr = address;
	new_client->adapter = adapter;
	new_client->driver = &atxp1_driver;
	new_client->flags = 0;
	
	/* Detect ATXP1, checking if vendor ID registers are all zero 
	 * and register 0x05 bit 7 is set */
	if(!((i2c_smbus_read_byte_data(new_client, 0x3e) = 0) &&
	     (i2c_smbus_read_byte_data(new_client, 0x3f) = 0) &&
	     (i2c_smbus_read_byte_data(new_client, 0xfe) = 0) &&
	     (i2c_smbus_read_byte_data(new_client, 0xff) = 0) )) {

		if(!(i2c_smbus_read_byte_data(new_client, 0x05) & 0x80)) {
			kfree(data);
			return 0;
		}
	}
	
	/* Get VRM */
	data->vrm = i2c_which_vrm();
	
	if(data->vrm != 90) {
		dev_err(&new_client->dev, "Not supporting VRM %d\n", data->vrm);
		return 0;
	}
	
	strncpy(new_client->name, "atxp1", I2C_NAME_SIZE);

	data->valid = 0;
	
	init_MUTEX(&data->update_lock);
	
	if ((err = i2c_attach_client(new_client)))
	{
		dev_err(&new_client->dev, "Attach client error.\n");
		kfree(data);
		return err;
	}
	
	device_create_file(&new_client->dev, &dev_attr_gpio1);
	device_create_file(&new_client->dev, &dev_attr_gpio2);
	device_create_file(&new_client->dev, &dev_attr_cpu0_vid);
	
	dev_info(&new_client->dev, "Detected on %s. Using VRM: %d.%d\n", 
			 adapter->name, data->vrm / 10, data->vrm % 10);
	
	return 0;
};

static int atxp1_detach_client(struct i2c_client * client)
{
	int err;
	
	err = i2c_detach_client(client);
	
	if (err) 
		dev_err(&client->dev, "Failed to detach client.\n");
	else
		kfree(i2c_get_clientdata(client));
	
	return 0;
};
 
static int __init atxp1_init(void)  
{ 
	i2c_add_driver(&atxp1_driver);
	
	return 0; 
};

static void __exit atxp1_exit(void) 
{ 
	i2c_del_driver(&atxp1_driver);
};

module_init(atxp1_init);
module_exit(atxp1_exit);

ATXP1 kernel patch

[Jean Delvare]

Hi Sebastian,

> I want to add two things to my last message:
>
> - The VRM check in atxp1_detect was moved above i2c_attach_client.

Good idea. It's better not to register anything if we are giving it all
up in the end. However, you forgot to free the memory you allocated.

> - I've channged the detection method, because
> ..
> temp = i2c_smbus_read_byte_data(new_client, ATXP1_VID);
> if(!((i2c_smbus_read_byte_data(new_client, 0x10) = temp) &&
>     (i2c_smbus_read_byte_data(new_client, 0x11) = temp)
> ...
>
> the register 0x10,0x11 aren't the same as 0x00 all time (the dump say
> it, but I think that's because the device is dumped from 0x00 upwards).
> If you directly read 0x10, 0x11 they are also zero.

You are right that what addresses 0x10 and 0x11 return depend on what was
previously read from the chip, because these address don't map to
physical registers. However this is precisely a very good way to detect
exotic chips such as the ATXP1. Please do some tests to understand what
exactly determines the values being returned. Usually this is the last
"real" value read, but in your case this seems to be different.

At the very least you can check that both addresses (0x10 and 0x11)
return the same value, whatever this is, right?

I really think we need to use this, or the detection will be possibly
unreliable (although we are lucky that the ATXP1 uses uncommon
addresses).

> Instead I'm checking register 0x05, bit 7. It is always set:
> ..
> if(!(i2c_smbus_read_byte_data(new_client, 0x05) & 0x80))
> ..

I don't know how you do know that, but if it works, fine with me.

Now answering your previous post:

> Something like in the attached patch? I've not added the other VRMs yet,
> first need to know if the ATXP1 is used with other VRMs.

Yes, something like that.

> > Huh. Looks even more complex. Do you really need to read all 10 first
> > registers, while you were previously only reading 2? What is the benefit
> > of having a separate "valid" bit for each register since you always read
> > them all at once anyway?
> 
> For future versions, which are using more (all registers). For general
> purpose IOs (used to control other voltages, extra functions) and
> watchdog timer.

We don't much care about future versions at this point. There might
never be any future version. If there is, you'll be able to do all the
required changes at that time. The code you will submit for the first
version of your driver has to make sense per se. Please only read the
registers you need.

> Also a register variable gets only updated at start or when a value was
> written.

Right, I did not understand what you were doing in the first place,
because it differs from what the other i2c client drivers do.

The reason why i2c clients have an update function is to centralize and
control i2c traffic that can be triggered by regular users. Remember
that regular users can read from sysfs files your driver exports. In
order to prevent possible bus saturation by regular users, all client
drivers have a timer in their update function, which prevent register
updates more frequent than (typically) one second. Your driver doesn't.
Please see in e.g. lm90.c how this has to be done, and do the same.

Having individual validity bits then becomes useless. The only driver
which has that is eeprom.c, but only because it has a very large number
of "registers" (256 as opposed to less than 30 for most drivers) and a
very long cache timeout (5 minutes instead of 1 second). For your driver
it doesn't make sense. You should NOT suppose that register values
never change, this is dangerous.

So please:
1* Add a timer check in your update function. 1 second (HZ) should be
fine.
2* Only read registers you need.
3* Have a single valid bit for them all.

> I don't know if the ATXP1 is used on boards with < 9.0 at all. Would be
> interesting if somebody knows.

I have no idea, sorry. I see no reason why it wouldn't be technically
possible, but it doesn't mean that any motherboard manufacturer
actually did it.

Random comments on your code now:

> #include <linux/moduleparam.h>

You don't need this.

> static int atxp1_write(struct i2c_client *client, unsigned char adr, unsigned char data)
> {
> 	int ret = -1;
> 
> 	ret = i2c_smbus_write_byte_data(client, adr, data);
> 
> 	if(ret < 0) {
> 		dev_err(&client->dev, "Write failed.\n");
> 		return -1;
> 	}
> 
> 	return 0;
> }

Two D to "address" in english. Initialization of ret to -1 is usless.
You better propagate the error value than arbitrary return -1 on error.
You could also change the error message to indicate which address you
were trying to read, that might help analyzing problems later.

BTW, you never check the return value whan calling this function.

> 	if(data->vrm != 90) {
> 		dev_err(&new_client->dev, "Not supporting VRM %d\n", data->vrm);
> 		return 0;
> 	}

Why don't use %d.%d with /10 and %10 like you do in case of success?

> static int atxp1_detach_client(struct i2c_client * client)
> {
> 	int err;
> 
> 	err = i2c_detach_client(client);
> 
> 	if (err)
> 		dev_err(&client->dev, "Failed to detach client.\n");
> 	else
> 		kfree(i2c_get_clientdata(client));
> 
> 	return 0;
> };

You better propagate the error instead of returning 0 if it occurs.

> static int __init atxp1_init(void)
> {
> 	i2c_add_driver(&atxp1_driver);
> 
> 	return 0;
> };

Ditto. i2c_add_driver may fail.

Thanks,
--
Jean Delvare

ATXP1 kernel patch

[Sebastian Witt]

Hi Jean,

> 
> Good idea. It's better not to register anything if we are giving it all
> up in the end. However, you forgot to free the memory you allocated.

Fixed.

> 
> You are right that what addresses 0x10 and 0x11 return depend on what was
> previously read from the chip, because these address don't map to
> physical registers. However this is precisely a very good way to detect
> exotic chips such as the ATXP1. Please do some tests to understand what
> exactly determines the values being returned. Usually this is the last
> "real" value read, but in your case this seems to be different.
> 
> At the very least you can check that both addresses (0x10 and 0x11)
> return the same value, whatever this is, right?
> 
> I really think we need to use this, or the detection will be possibly
> unreliable (although we are lucky that the ATXP1 uses uncommon
> addresses).
> 
> 

I've done some tests and it seems to work now if I first check the 
vendor ID registers and than after that 0,0x10,0x11. Also working
after some random read/writes and reloading the module.

> 
> We don't much care about future versions at this point. There might
> never be any future version. If there is, you'll be able to do all the
> required changes at that time. The code you will submit for the first
> version of your driver has to make sense per se. Please only read the
> registers you need.

Done.

> The reason why i2c clients have an update function is to centralize and
> control i2c traffic that can be triggered by regular users. Remember
> that regular users can read from sysfs files your driver exports. In
> order to prevent possible bus saturation by regular users, all client
> drivers have a timer in their update function, which prevent register
> updates more frequent than (typically) one second. Your driver doesn't.
> Please see in e.g. lm90.c how this has to be done, and do the same.
> 
> Having individual validity bits then becomes useless. The only driver
> which has that is eeprom.c, but only because it has a very large number
> of "registers" (256 as opposed to less than 30 for most drivers) and a
> very long cache timeout (5 minutes instead of 1 second). For your driver
> it doesn't make sense. You should NOT suppose that register values
> never change, this is dangerous.

You're right, it's possible that the register values are reset after
suspend-to-disk. And checking the validity bits doesn't recognize this.

> So please:
> 1* Add a timer check in your update function. 1 second (HZ) should be
> fine.
> 2* Only read registers you need.
> 3* Have a single valid bit for them all.

Done.

> 
> Random comments on your code now:
 > ....

All fixed, I think.

Thanks for your help,
Sebastian
-------------- next part --------------
/*
    atxp1.c - kernel module for setting CPU VID and general purpose
                     I/Os using the Attansic ATXP1 chip. 
    
    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.
    
    Version history:
    0.1: - Converted 8rdavcore into kernel module (Marcin Kaluza <marcin_ml@sekretarka.no-ip.org>)
    0.2: - Cleanup, general interface for VCore (Sebastian Witt <hasw@hasw.net>)
    0.3: - Fixed VID calculation (no FP operation)
         - Fixed VID setting loop
         - Added update lock
         - Renamed device file to cpu_vid
         - Added low_voltage module option to set lowest possible voltage (1075mV or 1100mV)
    0.4: - Added GPIO[1,2]
        - Using I2C VRM functions
        - Changed detection
        - Some other small fixes
    0.5: - Added timer check
        - Changed atxp1_update_device
        - Changed error handling

*/

#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/module.h>
#include <linux/i2c.h>
#include <linux/i2c-sensor.h>
#include <linux/i2c-vid.h>

MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("System voltages control via Attansic ATXP1");
MODULE_VERSION("0.5");

#define ATXP1_VID	0x00
#define ATXP1_CVID	0x01
#define ATXP1_GPIO1	0x06
#define ATXP1_GPIO2	0x0a
#define ATXP1_VIDENA	0x20
#define ATXP1_VIDMASK	0x1f
#define ATXP1_GPIO1MASK	0x0f

static unsigned short normal_i2c[]= { 0x37, 0x4e, I2C_CLIENT_END };
static unsigned int normal_isa[] = { I2C_CLIENT_ISA_END };
static int atxp1_attach_adapter(struct i2c_adapter * adapter);
static int atxp1_detach_client(struct i2c_client * client);
static struct atxp1_data * atxp1_update_device(struct device *dev);
static int atxp1_detect(struct i2c_adapter *adapter, int address, int kind);

static struct i2c_driver atxp1_driver = {
	.owner		= THIS_MODULE,
	.name		= "atxp1",
	.flags		= I2C_DF_NOTIFY,
	.attach_adapter = atxp1_attach_adapter,
	.detach_client	= atxp1_detach_client,
};

SENSORS_INSMOD_1(atxp1);

struct atxp1_data {
	struct i2c_client client;
	struct semaphore update_lock;
	unsigned long last_updated;
	u8 valid;
	struct {
		u8 vid;		/* VID output register */
		u8 cpu_vid; /* VID input from CPU */
		u8 gpio1;   /* General purpose I/O register 1 */
		u8 gpio2;   /* General purpose I/O register 2 */
	} reg;	
	u8 vrm;			/* Detected CPU VRM */
};

static struct atxp1_data * atxp1_update_device(struct device *dev) 
{
	struct i2c_client *client;
	struct atxp1_data *data;

	client = to_i2c_client(dev);
	data = i2c_get_clientdata(client);

	down(&data->update_lock);
	
	if ((jiffies - data->last_updated > HZ) ||
	    (jiffies < data->last_updated) ||
	    !data->valid) {
		
		/* Update local register data */
		data->reg.vid = i2c_smbus_read_byte_data(client, ATXP1_VID);
		data->reg.cpu_vid = i2c_smbus_read_byte_data(client, ATXP1_CVID);
		data->reg.gpio1 = i2c_smbus_read_byte_data(client, ATXP1_GPIO1);
		data->reg.gpio2 = i2c_smbus_read_byte_data(client, ATXP1_GPIO2);
		
		data->valid = 1;
	}
    	
	up(&data->update_lock);
	
	return(data);
}

static int atxp1_write(struct i2c_client *client, unsigned char addr, unsigned char data) 
{
	i2c_smbus_write_byte_data(client, addr, data);
	
	return 0;
}

/* sys file functions for cpu0_vid */
ssize_t atxp1_showvcore(struct device *dev, char *buf)
{
	int size;
	struct atxp1_data *data;
	
	data = atxp1_update_device(dev);
	
	size = sprintf(buf, "%d\n", vid_from_reg(data->reg.vid & ATXP1_VIDMASK, data->vrm));

	return size;
}

ssize_t atxp1_storevcore(struct device *dev, const char* buf, size_t count)
{
	struct atxp1_data *data;
	struct i2c_client *client;
	char vid;
	char cvid;
	unsigned int vcore;

	client = to_i2c_client(dev);
	data = atxp1_update_device(dev);

	vcore = simple_strtoul(buf, NULL, 10);
	vcore /= 25;	
	vcore *= 25;	
	
	/* Calculate VID */
	vid = reg_from_vid(vcore, data->vrm);
	
	if(vid < 0) {
		dev_err(dev, "VID calculation failed.\n");
		return -1;
	}
	
	/* If output enabled, use control register value. Otherwise original CPU VID */
	if(data->reg.vid & ATXP1_VIDENA)
		cvid = data->reg.vid & ATXP1_VIDMASK;
	else
		cvid = data->reg.cpu_vid;
	
	/* Nothing changed, aborting */
	if(vid = cvid)
		return count;
	
	dev_info(dev, "Setting VCore to %d mV (0x%02x)\n", vcore, vid);
	
	/* Write every 25 mV step to increase stability */
	if(cvid > vid) {
		for(; cvid >= vid; cvid--) {
        		atxp1_write(client, ATXP1_VID, cvid | ATXP1_VIDENA);
		}
	}
	else {
		for(; cvid <= vid; cvid++) {
        		atxp1_write(client, ATXP1_VID, cvid | ATXP1_VIDENA);
		}
	}
	
	data->valid = 0;
	
	return count;
}

/* CPU core reference voltage
    unit: millivolt
*/
static DEVICE_ATTR(cpu0_vid, S_IRUGO | S_IWUSR, atxp1_showvcore, atxp1_storevcore);

/* sys file functions for GPIO1 */
ssize_t atxp1_showgpio1(struct device *dev, char *buf)
{
	int size;
	struct atxp1_data *data;
	
	data = atxp1_update_device(dev);
	
	size = sprintf(buf, "0x%02x\n", data->reg.gpio1 & ATXP1_GPIO1MASK);

	return size;
}

ssize_t atxp1_storegpio1(struct device *dev, const char* buf, size_t count)
{
	struct atxp1_data *data;
	struct i2c_client *client;
	unsigned int value;
	
	client = to_i2c_client(dev);
	data = atxp1_update_device(dev);

	value = simple_strtoul(buf, NULL, 16);
	
	value &= ATXP1_GPIO1MASK;
	
	if(value != (data->reg.gpio1 & ATXP1_GPIO1MASK)) {
		dev_info(dev, "Writing 0x%x to GPIO1.\n", value);
		
		atxp1_write(client, ATXP1_GPIO1, value);
		
		data->valid = 0;
	}
	
	return count;
}

/* GPIO1 data register
    unit: Four bit as hex (e.g. 0x0f)
*/
static DEVICE_ATTR(gpio1, S_IRUGO | S_IWUSR, atxp1_showgpio1, atxp1_storegpio1);

/* sys file functions for GPIO2 */
ssize_t atxp1_showgpio2(struct device *dev, char *buf)
{
	int size;
	struct atxp1_data *data;
	
	data = atxp1_update_device(dev);
	
	size = sprintf(buf, "0x%02x\n", data->reg.gpio2);

	return size;
}

ssize_t atxp1_storegpio2(struct device *dev, const char* buf, size_t count)
{
	struct atxp1_data *data;
	struct i2c_client *client;
	unsigned int value;
	
	client = to_i2c_client(dev);
	data = atxp1_update_device(dev);

	value = simple_strtoul(buf, NULL, 16) & 0xff;
	
	if(value != data->reg.gpio2) {
		dev_info(dev, "Writing 0x%x to GPIO1.\n", value);

		atxp1_write(client, ATXP1_GPIO2, value);

		data->valid = 0;
	}
	
	return count;
}

/* GPIO2 data register
    unit: Eight bit as hex (e.g. 0xff)
*/
static DEVICE_ATTR(gpio2, S_IRUGO | S_IWUSR, atxp1_showgpio2, atxp1_storegpio2);


static int atxp1_attach_adapter(struct i2c_adapter *adapter)
{
	return i2c_detect(adapter, &addr_data, &atxp1_detect);
};
	
static int atxp1_detect(struct i2c_adapter *adapter, int address, int kind)
{
	struct i2c_client * new_client;
	struct atxp1_data * data;
	int err = 0;
	u8 temp;
	
	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
		goto exit;
	
	if (!(data = kmalloc(sizeof(struct atxp1_data), GFP_KERNEL))) {
		err = -ENOMEM;
		goto exit;
	}

	memset(data, 0, sizeof(struct atxp1_data));
	new_client = &data->client;
	i2c_set_clientdata(new_client, data);
	
	new_client->addr = address;
	new_client->adapter = adapter;
	new_client->driver = &atxp1_driver;
	new_client->flags = 0;
	
	/* Detect ATXP1, checking if vendor ID registers are all zero */
	if(!((i2c_smbus_read_byte_data(new_client, 0x3e) = 0) &&
	     (i2c_smbus_read_byte_data(new_client, 0x3f) = 0) &&
	     (i2c_smbus_read_byte_data(new_client, 0xfe) = 0) &&
	     (i2c_smbus_read_byte_data(new_client, 0xff) = 0) )) {
		
		/* No vendor ID, now checking if registers 0x10,0x11 (non-existent)
		 * showing the same as register 0x00 */
		temp = i2c_smbus_read_byte_data(new_client, 0x00);
		
		if(!((i2c_smbus_read_byte_data(new_client, 0x10) = temp) &&
			 (i2c_smbus_read_byte_data(new_client, 0x11) = temp) ))
			goto exit_free;
	}
	
	/* Get VRM */
	data->vrm = i2c_which_vrm();
	
	if((data->vrm != 90) && (data->vrm != 91)) {
		dev_err(&new_client->dev, "Not supporting VRM %d.%d\n", 
				data->vrm / 10, data->vrm % 10);
		goto exit_free;
	}
	
	strncpy(new_client->name, "atxp1", I2C_NAME_SIZE);

	data->valid = 0;
	
	init_MUTEX(&data->update_lock);
	
	if ((err = i2c_attach_client(new_client)))
	{
		dev_err(&new_client->dev, "Attach client error.\n");
		goto exit_free;
	}
	
	device_create_file(&new_client->dev, &dev_attr_gpio1);
	device_create_file(&new_client->dev, &dev_attr_gpio2);
	device_create_file(&new_client->dev, &dev_attr_cpu0_vid);
	
	dev_info(&new_client->dev, "Detected on %s. Using VRM: %d.%d\n", 
			 adapter->name, data->vrm / 10, data->vrm % 10);
	
	return 0;
	
exit_free:
	kfree(data);
exit:
	return err;
};

static int atxp1_detach_client(struct i2c_client * client)
{
	int err;
	
	err = i2c_detach_client(client);
	
	if (err) 
		dev_err(&client->dev, "Failed to detach client.\n");
	else
		kfree(i2c_get_clientdata(client));
	
	return err;
};
 
static int __init atxp1_init(void)  
{ 
	return i2c_add_driver(&atxp1_driver);
};

static void __exit atxp1_exit(void) 
{ 
	i2c_del_driver(&atxp1_driver);
};

module_init(atxp1_init);
module_exit(atxp1_exit);

ATXP1 kernel patch

[Jean Delvare]

Hi Sebastian,

First of all, sorry for the long delay. I have been really busy with
various other stuff these last two weeks.

Your latest version of the driver looks very good. I've only found these
minor things you could fix/improve:

I think that Greg will ask you to remove the inline history of the
driver. We don't much care about what happened to the driver before in
was committed into the kernel tree, and after that bitkeeper is the best
source for tracing changes.

> MODULE_LICENSE("GPL");
> MODULE_DESCRIPTION("System voltages control via Attansic ATXP1");
> MODULE_VERSION("0.5");

No MODULE_AUTHOR?

> static unsigned short normal_i2c[]= { 0x37, 0x4e, I2C_CLIENT_END };
> static unsigned int normal_isa[] = { I2C_CLIENT_ISA_END };

Missing space before "=". You would also move the call to
SENSORS_INSMOD_1 right below these lines, to make it clearer that the
array definitions are there for it.

> static int atxp1_write(struct i2c_client *client, unsigned char addr, unsigned char data) 
> {
> 	i2c_smbus_write_byte_data(client, addr, data);
> 	
> 	return 0;
> }

Why not return whatever i2c_smbus_write_byte_data returned instead? It
*can* return an error. I see little interest in this function overall,
it seems to just slow down the process by adding one more function call.

> 	if ((err = i2c_attach_client(new_client)))
> 	{

Coding style!

> 	dev_info(&new_client->dev, "Detected on %s. Using VRM: %d.%d\n", 
> 			 adapter->name, data->vrm / 10, data->vrm % 10);

dev_info will already give the id of the i2c adapter used, so the
"Detected on %s." is probably redundant.

That left apart I'm quite happy with your code. Feel free to send it to
Greg KH as a proper patch against 2.6.11-rc3-mm1. The patch should
include the required changes to drivers/i2c/chips/{Makefile,Kconfig}.

Thanks,
--  
Jean Delvare

ATXP1 kernel patch

[Rudolf Marek]

Hi Sebastian,

We are very sorry, we missed that ball is on our side.

Recently I found that ATXP1 register documentation was made
public. Or is public by mistake :)
http://www.attansic.com.tw/english/products/pdf/ATXP1_1.1.pdf

I went through your 0.6 version and I generally agree with it.

Except the static inline int reg_from_vid(int val, int vrm)

We would like to have name:

vid_to_reg(int val, int vrm)

Please can you fix the name and post a patch only containing the i2c-vid.h
changes to: greg@kroah.com and CC to sensors@Stimpy.netroedge.com
Do not put the patch to attachment let it be part of your message.
Please include short (one sentence) desciption what it does.
Dont forget the Signed-off-by: Sebastian Witt <se.witt@gmx.net>

As for the driver, I think it is ready too. Repeat above for the driver,
just note in the email that it depends on previous patch.
(and dont forget to fix the function call :)

Create both patches for latest kernel (-mm- tree is enough).

Thank you.
Sorry for the delay.

Regards

Rudolf