Skip to contentSkip to conversation

it87 part3(pwm)

PATCH New Takeru KOMORIYA 2 participants 4 messages May 19, 2005

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 it87 part3(pwm)

Hello,
Sorry for long silence but I want to go on adding some features to it87 for 2.6.

Here's a patch to add pwm[1-3] and pwm_enable[1-3]. pwm[1-3] are bit different from the ones for 2.4.

Documentation/i2c/sysfs-interface says that the values for pwm[1-3] should be 0-255 while it87 documentation for 2.4 says:
Bit 7 of the pwm[1-3] registers enables/disables the chips automatic temperature control mode for the specified fan.

I am not sure but I feel it's better to prepare another sysfs entry(e.g. sg_enable) for "automatic temperature control mode". pwm[1-3] accepts 0-255 with this trial patch.

BTW, I have not subscribed to this list.
Would you add me to the list?

Thanks in advance.

drivers/i2c/chips/it87.c renamed from drivers/i2c/chips/it87.c.reset +86 -1
@@ -85,6 +85,7 @@
 #define IT87_REG_FAN(nr)       (0x0d + (nr))
 #define IT87_REG_FAN_MIN(nr)   (0x10 + (nr))
 #define IT87_REG_FAN_CTRL      0x13
+#define IT87_REG_PWM(nr)       (0x15 + (nr))
 
 #define IT87_REG_VIN(nr)       (0x20 + (nr))
 #define IT87_REG_TEMP(nr)      (0x29 + (nr))
@@ -115,6 +116,10 @@
 
 #define FAN_FROM_REG(val,div) ((val)=0?-1:(val)=255?0:1350000/((val)*(div)))
 
+#define PWM_TO_REG(val) ((val)<0?0:(val)>255?127:((val)/2))
+
+#define PWM_FROM_REG(val) (((val)&0x7f)*2)
+
 #define TEMP_TO_REG(val) (SENSORS_LIMIT(((val)<0?(((val)-5)/10):\
 					((val)+5)/10),0,255))
 #define TEMP_FROM_REG(val) (((val)>0x80?(val)-0x100:(val))*10)
@@ -150,6 +155,8 @@
 	u8 in_min[9];		/* Register value */
 	u8 fan[3];		/* Register value */
 	u8 fan_min[3];		/* Register value */
+	u8 pwm[3];		/* Register value */
+	u8 fan_ctrl;		/* Register encoding */
 	u8 temp[3];		/* Register value */
 	u8 temp_high[3];	/* Register value */
 	u8 temp_low[3];		/* Register value */
@@ -429,6 +436,22 @@
 	it87_update_client(client);
 	return sprintf(buf,"%d\n", DIV_FROM_REG(data->fan_div[nr]) );
 }
+static ssize_t show_pwm(struct device *dev, char *buf, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct it87_data *data = i2c_get_clientdata(client);
+	it87_update_client(client);
+	return sprintf(buf,"%d\n", PWM_FROM_REG(data->pwm[nr]));
+}
+static ssize_t show_pwm_enable (struct device *dev, char *buf, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct it87_data *data = i2c_get_clientdata(client);
+	it87_update_client(client);
+	if (data->fan_ctrl & (1 << nr))
+	    return sprintf(buf, "1\n");
+	return sprintf(buf, "0\n");
+}
 static ssize_t set_fan_min(struct device *dev, const char *buf, 
 		size_t count, int nr)
 {
@@ -475,6 +498,34 @@
 	}
 	return count;
 }
+static ssize_t set_pwm(struct device *dev, const char *buf, 
+		size_t count, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct it87_data *data = i2c_get_clientdata(client);
+	int val = simple_strtol(buf, NULL, 10);
+	if (val < 0 || val > 255)
+	    return -1;
+	data->pwm[nr] = PWM_TO_REG(val);
+	it87_write_value(client, IT87_REG_PWM(nr), data->pwm[nr]);
+	return count;
+}
+static ssize_t set_pwm_enable (struct device *dev, const char *buf,
+		size_t count, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct it87_data *data = i2c_get_clientdata(client);
+	int val = simple_strtol(buf, NULL, 10);
+
+	data->fan_ctrl &= ~(1 << nr);
+	if (val = 1)
+	    data->fan_ctrl |= 1 << nr;
+	else if (val != 0)
+	    return -1;
+	data->fan_ctrl |= 0x70;
+	it87_write_value(client, IT87_REG_FAN_CTRL, data->fan_ctrl);
+	return count;
+}
 
 #define show_fan_offset(offset)						\
 static ssize_t show_fan_##offset (struct device *dev, char *buf)	\
@@ -489,6 +540,15 @@
 {									\
 	return show_fan_div(dev, buf, 0x##offset - 1);			\
 }									\
+static ssize_t show_fan_##offset##_pwm (struct device *dev, char *buf)	\
+{									\
+	return show_pwm(dev, buf, 0x##offset - 1);			\
+}									\
+static ssize_t show_pwm_##offset##_enable (struct device *dev		\
+					   , char *buf)			\
+{									\
+	return show_pwm_enable(dev, buf, 0x##offset - 1);		\
+}									\
 static ssize_t set_fan_##offset##_min (struct device *dev, 		\
 	const char *buf, size_t count) 					\
 {									\
@@ -499,11 +559,25 @@
 {									\
 	return set_fan_div(dev, buf, count, 0x##offset - 1);		\
 }									\
+static ssize_t set_fan_##offset##_pwm (struct device *dev, 		\
+		const char *buf, size_t count) 				\
+{									\
+	return set_pwm(dev, buf, count, 0x##offset - 1);		\
+}									\
+static ssize_t set_pwm_##offset##_enable (struct device *dev, 		\
+		const char *buf, size_t count) 				\
+{									\
+	return set_pwm_enable(dev, buf, count, 0x##offset - 1);		\
+}									\
 static DEVICE_ATTR(fan_input##offset, S_IRUGO, show_fan_##offset, NULL) \
 static DEVICE_ATTR(fan_min##offset, S_IRUGO | S_IWUSR, 			\
 		show_fan_##offset##_min, set_fan_##offset##_min) 	\
 static DEVICE_ATTR(fan_div##offset, S_IRUGO | S_IWUSR, 			\
-		show_fan_##offset##_div, set_fan_##offset##_div)
+		show_fan_##offset##_div, set_fan_##offset##_div)        \
+static DEVICE_ATTR(pwm##offset, S_IRUGO | S_IWUSR, 			\
+		show_fan_##offset##_pwm, set_fan_##offset##_pwm)	\
+static DEVICE_ATTR(pwm_enable##offset, S_IRUGO | S_IWUSR, 		\
+		show_pwm_##offset##_enable, set_pwm_##offset##_enable)
 
 show_fan_offset(1);
 show_fan_offset(2);
@@ -697,6 +771,12 @@
 	device_create_file(&new_client->dev, &dev_attr_fan_div1);
 	device_create_file(&new_client->dev, &dev_attr_fan_div2);
 	device_create_file(&new_client->dev, &dev_attr_fan_div3);
+	device_create_file(&new_client->dev, &dev_attr_pwm1);
+	device_create_file(&new_client->dev, &dev_attr_pwm2);
+	device_create_file(&new_client->dev, &dev_attr_pwm3);
+	device_create_file(&new_client->dev, &dev_attr_pwm_enable1);
+	device_create_file(&new_client->dev, &dev_attr_pwm_enable2);
+	device_create_file(&new_client->dev, &dev_attr_pwm_enable3);
 	device_create_file(&new_client->dev, &dev_attr_alarms);
 
 	return 0;
@@ -871,6 +951,11 @@
 		data->fan_div[1] = (i >> 3) & 0x07;
 		data->fan_div[2] = (i & 0x40) ? 3 : 1;
 
+		for(i = 0; i < 3; i++) {
+			data->pwm[i] = it87_read_value(client, IT87_REG_PWM(i));
+		}
+		data->fan_ctrl = it87_read_value(client, IT87_REG_FAN_CTRL);
+
 		data->alarms  			it87_read_value(client, IT87_REG_ALARM1) |
 			(it87_read_value(client, IT87_REG_ALARM2) << 8) |
 

-----------------------
Takeru Komoriya
[email protected]
http://www.paken.org/

#1 lore raw

PATCH 1/1 it87 part3(pwm)

Ohaio Takeru,

(As you can see I just learnt a fourth Japanese word ;))

Quoted 2 lines

Sorry for long silence but I want to go on adding some features to it87 for 2.6.

No problem.

Quoted 2 lines

Here's a patch to add pwm[1-3] and pwm_enable[1-3]. pwm[1-3] are bit different from the ones for 2.4.

Note that the sysfs files naming scheme has changed and pwm_enable[1-3] is now pwm[1-3]_enable.

Quoted 8 lines

Documentation/i2c/sysfs-interface says that the values for pwm[1-3] should be 0-255 while it87 documentation for 2.4 says:
Bit 7 of the pwm[1-3] registers enables/disables the chips automatic temperature control mode for the specified fan.

I am not sure but I feel it's better to prepare another sysfs entry(e.g. sg_enable) for "automatic temperature control mode". pwm[1-3] accepts 0-255 with this trial patch.

You're right, this is the correct way to do the things. Please consider submitting a patch that changes the CVS driver in the same way as well. This may be delayed until after your smartguardian patch for 2.6 (since I guess there will be one).

I'd like to make sure I understand how fan control works for the IT87. It seems to be a little bit complex, so I'd like you to confirm that I got it right.

There are three different modes for each fan:

1* On/Off: Bits 2-0 of register 0x13 are clear. Fans are controlled by bits 0-2 of register 0x14.

(BTW we never access these bits of register 0x14, if I'm not mistaking. Shouldn't these bits be exported for user control as fan[1-3]_enable?)

2* Manual PWM: Bits 2-0 of register 0x13 are set. Bit 7 of registers 0x15-0x17 is clear. Bits 6-0 of registers 0x15-0x17 control the fan speed.

3* Auto PWM: Bits 2-0 of register 0x13 are set. Bit 7 of registers 0x15-0x17 is set. Bits 1-0 of registers 0x15-0x17 and various other "SmartGuardian" registers control the fan speed.

Is this correct?

Quoted 2 lines

BTW, I have not subscribed to this list.
Would you add me to the list?

Request sent to Phil, should be processed soon ;)

drivers/i2c/chips/it87.c line 121

Quoted 3 lines
drivers/i2c/chips/it87.c renamed from drivers/i2c/chips/it87.c.reset +3 -0
+#define PWM_TO_REG(val) ((val)<0?0:(val)>255?127:((val)/2))
+
+#define PWM_FROM_REG(val) (((val)&0x7f)*2)

I think that /2 should be >>1 and *2 should be <<1. But maybe it'e just me.

The limit checking in PWM_TO_REG seems unnecessary since you already do it in set_pwm() below.

drivers/i2c/chips/it87.c line 445

Quoted 7 lines
drivers/i2c/chips/it87.c renamed from drivers/i2c/chips/it87.c.reset +7 -0
+static ssize_t show_pwm(struct device *dev, char *buf, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct it87_data *data = i2c_get_clientdata(client);
+	it87_update_client(client);
+	return sprintf(buf,"%d\n", PWM_FROM_REG(data->pwm[nr]));
+}

A recent refactoring has been proposed by MMH and accepted. Update functions now return the data structure, so the callback functions are more simple.

I invite you to use 2.6.4-mm1 as the base for your patch. It aleady has your two previous patches.

drivers/i2c/chips/it87.c line 454

Quoted 9 lines

+static ssize_t show_pwm_enable (struct device *dev, char *buf, int nr)+{

drivers/i2c/chips/it87.c renamed from drivers/i2c/chips/it87.c.reset +7 -0
+	struct i2c_client *client = to_i2c_client(dev);
+	struct it87_data *data = i2c_get_clientdata(client);
+	it87_update_client(client);
+	if (data->fan_ctrl & (1 << nr))
+	    return sprintf(buf, "1\n");
+	return sprintf(buf, "0\n");
+}

return sprintf(buf, "%d\n", (data->fan_ctrl & (1 << nr)) != 0);?

drivers/i2c/chips/it87.c line 525

Quoted 13 lines
drivers/i2c/chips/it87.c renamed from drivers/i2c/chips/it87.c.reset +13 -0
+static ssize_t set_pwm_enable (struct device *dev, const char *buf,
+		size_t count, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct it87_data *data = i2c_get_clientdata(client);
+	int val = simple_strtol(buf, NULL, 10);
+
+	data->fan_ctrl &= ~(1 << nr);
+	if (val = 1)
+	    data->fan_ctrl |= 1 << nr;
+	else if (val != 0)
+	    return -1;
+	data->fan_ctrl |= 0x70;

I don't think that enabling one PWM should possibly affect the other fans, so it would be "data->fan_ctrl |= 1 << (nr+4);".

drivers/i2c/chips/it87.c line 551

Quoted 9 lines

+static ssize_t show_fan_##offset##_pwm (struct device *dev, char
*buf) \+{ \

drivers/i2c/chips/it87.c renamed from drivers/i2c/chips/it87.c.reset +7 -0
+	return show_pwm(dev, buf, 0x##offset - 1);			\
+}									\
+static ssize_t show_pwm_##offset##_enable (struct device *dev		\
+					   , char *buf)			\
+{									\
+	return show_pwm_enable(dev, buf, 0x##offset - 1);		\
+}									\

Care to "symmetrize" the function names? show_fan_##offset##_pwm should be show_pwm_##offset IMHO.

drivers/i2c/chips/it87.c line 571

Quoted 10 lines
drivers/i2c/chips/it87.c renamed from drivers/i2c/chips/it87.c.reset +6 -0
+static ssize_t set_fan_##offset##_pwm (struct device *dev, 		\
+		const char *buf, size_t count) 				\
+{									\
+	return set_pwm(dev, buf, count, 0x##offset - 1);		\
+}									\
+static ssize_t set_pwm_##offset##_enable (struct device *dev, 	

\+ const char *buf, size_t count) \

drivers/i2c/chips/it87.c renamed from drivers/i2c/chips/it87.c.reset +3 -0
+{									\
+	return set_pwm_enable(dev, buf, count, 0x##offset - 1);		\
+}									\

Same here.

Looks good otherwise.

Just one more thing I'd like to hear you about. Bits 1-0 of registers 0x15-0x17 have two different meanings, depending on the value of bit 7 of the same register.

This is likely to cause trouble when one wants to change from automatic PWM to manual, or the other way around. Since we split the register into two sysfs files (PWM value and PWM enable), it isn't possible to set the value of the corresponding register at once. Changing the mode (bit 7) without also changing bits 6-0 would possibly result in a wrong intermediate configuration.

(BTW, your code above never checks that bit 7 of these registers is clear, does it? It should, even if that will change later with your SmartGuardian patch. You should never accept writing a PWM value to the register if it's in automatic mode, since the value wouldn't make any sense. Likewise, you should not return the PWM value if the bit 7 is set, since bits 6-0 then don't hold a PWM value then.)

I think that the correct way to handle this situation is to store sets of bits in several internal variables, not just one. Something like:

	u8 pwm_mode[3];
	u8 pwm_value[3];
	u8 pwm_temp[3];

pwm_mode would hold bit 7, pwm_value would hold bits 6-0 when mode is manual, pwm_temp would hold bits 1-0 when mode is auto.

You would be able to initialize either pwm_value or pwm_temp at driver load time, depending on the value of pwm_mode. You would have to set a reasonable default value for the other (0x7f for pwm_value and N for pwm_temp[N] seems to make sense).

The idea behind that is that you would then be able to change modes without a wrong intermediate situation. Say that the user switches from auto to manual, you would be able to write a combination of pwm_mode and pwm_value into the register. When switching from manual to auto, you combine pwm_mode and pwm_temp. And you don't lose any information doing so.

This first patch may look a bit strange because we have to think of SmartGuardian without actually supporting it. Still this is an interesting execise IMHO, which forces us to think twice to what we do and how we do it, and ensures that we correctly understood how things have to work.

I hope I have been clear. If not, just tell me and I'll make my best to be clearer.

And if I just got everything plain wrong and what I said just doesn't make any sense to anyone (which is always possible), tell me too ;)

Aligato :)

Signature · 2 lines
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/
#2 lore raw

PATCH 1/1 it87 part3(pwm)

In reply to Takeru KOMORIYA

Quoting myself:

Quoted 2 lines

I invite you to use 2.6.4-mm1 as the base for your patch. It aleady has your two previous patches.

Just realized I'm wrong here. MMH's refactoring patch is *not* in 2.6.4-mm1, so I invite you to base your patch on 2.6.4-mm1 + this patch:
http://delvare.nerim.net/i2c/linux-2.6/linux-2.6.4-i2c-chips-refactoring.diff

Just take a look at it87.c after patching and you should understand easily how to adapt your own work. BTW, sorry that our recent changes will require a bit more work from you, but we believe that these changes were the right thing to do, so the sooner, the better.

Sorry for the wrong information too.

Signature · 2 lines
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/
#3 lore raw

PATCH 1/1 it87 part3(pwm)

In reply to Takeru KOMORIYA

Hi Takeru,

Some times ago you were working on adding PWM support to the 2.6 it87 driver. I commented on your candidate patch, but never heard from you since. What's up? Are you OK?

We just had a request for this feature:
http://secure.netroedge.com/~lm78/readticket.cgi?ticket63
which reminded me of your work. It would be great if we could go on with this and have PWM operational in 2.6 someday.

Thanks.

Signature · 2 lines
Jean Delvare
http://khali.linux-fr.org/