[lm-sensors] [PATCH] Add voltage support to W83627EHF

[lm-sensors] [PATCH] Add voltage support to W83627EHF

[Rudolf Marek]

Hello all,

I just finished adding the support for voltage channels on EHF chip. Feel free to test it.
(This implies to the mailing list members rather to you Yuan, I know you are busy ;)

Jean, If you like it, please apply (after someone did the testing in reasonable time)

This patch adds the voltage measuring support to W83627EHF. The code is based
on the patch provided by Yuan Mu from Winbond. Moreover the patch implements new
alarm files scheme for voltages.

Signed-off-by: Yuan Mu <Ymu at Winbond.com.tw>
Signed-off-by: Rudolf Marek <r.marek at sh.cvut.cz>

Thanks,
Regards
Rudolf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627ehf_add_voltages.patch
Type: text/x-patch
Size: 8594 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060304/b1354d92/w83627ehf_add_voltages.bin

[lm-sensors] [PATCH] Add voltage support to W83627EHF

[Rudolf Marek]

Hello all,

Oh I forgot. The patch needs this patches to work:
hwmon-allow-sensor-attr-arrays.patch  hwmon-convert-semaphores-to-mutexes.patch
w83627ehf_use_new_sensor_device_attr.patch-0001.txt
(get it here http://lists.lm-sensors.org/pipermail/lm-sensors/2006-January/015115.html, first
two are available through jean's homepage)

I'm attaching the userspace part this should work applying on the top of the
latest version of lm-sensors.

regards
Rudolf

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: add_voltages
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060304/b5ac6de8/add_voltages.pl

[lm-sensors] [PATCH] Add voltage support to W83627EHF

[Jean Delvare]

Hi Rudolf,

> I just finished adding the support for voltage channels on EHF chip. Feel free to test it.
> (This implies to the mailing list members rather to you Yuan, I know you are busy ;)
> 
> Jean, If you like it, please apply (after someone did the testing in reasonable time)

Can I have a -p1 patch please?

Comments on the code:

> +    This is a preliminary version of the driver. The chip does much more than
> +    that.

The "much" is probably too much now that the voltage inputs, and parts
of the alarms, are supported.

> +/* The W83627EHF registers for nr=7,8,9 are in bank 5 */
> +#define W83627EHF_REG_IN_MAX(nr) ((nr < 7) ? (0x2b + (nr) * 2) : \
> +					   (0x554 + (((nr) - 7) * 2)))
> +#define W83627EHF_REG_IN_MIN(nr) ((nr < 7) ? (0x2c + (nr) * 2) : \
> +					   (0x555 + (((nr) - 7) * 2)))
> +#define W83627EHF_REG_IN(nr)     ((nr < 7) ? (0x20 + (nr)) : \
> +					   (0x550 + (nr) - 7))

Don't use spaces for primary alignment please.

> +/* Some of registers have internal scaling,
> +   8mV is ADC LSB and some are scaled twice */

They are "scaled by a factor of 2", rather than "scaled twice". You may
also mention the word "internally", as this is the reason why we care
in the driver rather than user-space.

> +static u8 scale_in[10] = { 3, 3, 4, 4, 3, 3, 3, 4, 4, 3 };
> +
> +static inline u32 in_from_reg(u8 reg, u8 nr)
> +{
> +	return reg << scale_in[nr];
> +}
> +
> +static inline u8 in_to_reg(u32 val, u8 nr)
> +{
> +	return SENSORS_LIMIT(((val + (1 << (scale_in[nr] - 1))) / 
> +					(1 << scale_in[nr])), 0, 255);
> +}

That bit-shifting makes things quite hard to read. Why don't you just
go with explicit multiplications/divisions by 8 and 2 (or 8 and 16 if
you prefer)? This would be much easier to check, and I'm almost certain
that the compiler will generate the same code anyway.

> +		for (i = 0; i < 10; i++) {
> +			data->in[i] > +			    w83627ehf_read_value(client, W83627EHF_REG_IN(i));
> +			data->in_min[i] > +			    w83627ehf_read_value(client,
> +					       W83627EHF_REG_IN_MIN(i));
> +			data->in_max[i] > +			    w83627ehf_read_value(client,
> +					       W83627EHF_REG_IN_MAX(i));
> +
> +		}

Coding style, please.

> +static ssize_t show_##reg (struct device *dev, struct device_attribute *attr, char *buf) \

Line too long.

> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);\
> +	int nr = sensor_attr->index;\
> +	return sprintf(buf,"%ld\n", (long)in_from_reg(data->reg[nr], nr)); \

Missing space before backslash (twice), missing space after comma, and
there are many more in your patch. Please pay attention, you've been
around for long enough now to know how picky I am about this! ;)

I also fail to see the point of the cast; just make sure that
in_from_reg() will return what you need (i.e. long instead of u32) in
the first place.

> +static ssize_t show_in_alarm(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct w83627ehf_data *data = w83627ehf_update_device(dev);
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int nr = sensor_attr->index;
> +	return sprintf(buf,"%d\n", 
> +			(1 << W83627EHF_ALARM_IN_SHIFT[nr]) & data->alarms ? 1 : 0);
> +}

Looks inefficient. Why don't you instead store
W83627EHF_ALARM_IN_SHIFT[N] in sensor_attr->index when creating the
sysfs file? You would spare the lookup on each file read, and you
wouldn't even need to define an array with the shift values.

Also, an added benefit would be that the same callback could be used
for temperatures and fans as well, not just voltages.

Rest looks OK. Now with this patch, we end up with alarms for the
voltages and not the other input types, so I'd appreciate an incremental
patch adding these, for consistency. It should be quite straightforward.

Also I see that this driver has no documentation file. Shame on me, but
if anyone wants to contribute a patch, this would be welcome.

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] Add voltage support to W83627EHF

[Jean Delvare]

Hi Rudolf,

> I'm attaching the userspace part this should work applying on the top of the
> latest version of lm-sensors.

My comments:

> +    { SENSORS_W83627EHF_IN0_ALARM, "in0_alarm", NOMAP, NOMAP, 
> +                        R, NOSYSCTL, VALUE(1), 0 },

Now that we have individual alarm files, it would make sense to express
their mapping with the corresponding input. That is, the first "NOMAP"
above should be "SENSORS_W83627EHF_IN0".

> +#define SENSORS_W83627EHF_IN0 1 /* R */
> (...)
> +#define SENSORS_W83627EHF_IN9_MAX 30 /* RW */

Please align the values with tabs, like I did for the rest of the
W83627EHF defines.

> +  double cur, min, div, max, alarm;

If you want to give function scope to these variables, please do it
properly (i.e. also use them for temperature.)

> +    if (!sensors_get_label_and_valid(*name,SENSORS_W83627EHF_IN0+i,&label,&valid) &&
> +      !sensors_get_feature(*name,SENSORS_W83627EHF_IN0+i,&cur) &&
> +      !sensors_get_feature(*name,SENSORS_W83627EHF_IN0_MIN+i,&min) &&
> +      !sensors_get_feature(*name,SENSORS_W83627EHF_IN0_MAX+i,&max) &&
> +      !sensors_get_feature(*name,SENSORS_W83627EHF_IN0_ALARM+i,&alarm)) {
> +      if (valid) {
> +        print_label(label,10);
> +        printf("%+6.2f V  (min = %+6.2f V, max = %+6.2f V) %s\n",
> +               cur,min,max,alarm ? "ALARM" : "");
> +      }
> +    } else
> +    printf("ERROR: Can't get IN%d data!\n",i+1);
> +    free(label);
> +  }

Coding style, please! I know that sensors' coding style is quite bad,
but let's not make it worse. Just follow the coding style I used for
the rest of the function, so that we have some kind of local
homogeneity.

> -    double cur, min, div;
> +    

No blank line here please.

>  # Winbond W83627EHF configuration originally contributed by Leon Moonen
>  # This is for an Asus P5P800.
>  chip "w83627ehf-*"

This will need some rewording as you have a different board so the
voltage part may not match the P5P800 at all.

> +    label in0 "VCore"
> +    label in1 "VIN0"
> +    label in2 "AVCC"
> +    label in3 "3VCC"
> +    label in4 "VIN1"
> +    label in5 "VIN2"
> +    label in6 "VIN3"
> +    label in7 "VSB"
> +    label in8 "VBAT"
> +    label in9 "VIN4"

The VIN[0-4] labels are not helping much, and may in fact add to
confusion, as they don't match our in0-9 numbering.

> +# +12V is in1 and +5V is in6 as recommended by datasheet 
> +    compute in1 @*(1+(56/10)),  @/(1+(56/10))
> +    compute in6 @*(1+(22/10)),  @/(1+(22/10))

Just label them that way then.

> -   set fan1_min    1200
> -   set fan2_min    1700
> +  set fan1_min    1200
> +  set fan2_min    1700

No whitespace changes please.

> +   label temp3     "AUX Temp"

s/AUX/Aux/ for consistency.

> -   set temp1_over  45
> -   set temp1_hyst  40
> -   set temp2_over  45
> -   set temp2_hyst  40
> +#  set temp1_over  45
> +#  set temp1_hyst  40
> +#  set temp2_over  45
> +#  set temp2_hyst  40

Yes you're right, we should preserve the BIOS settings by default.

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] Add voltage support to W83627EHF

[Rudolf Marek]

Hi all,

> Can I have a -p1 patch please?

Yep ;)

All fixed, here we go...

This patch adds the voltage measuring support to W83627EHF. The code is based
on the patch provided by Yuan Mu from Winbond.

Signed-off-by: Yuan Mu <Ymu at Winbond.com.tw>
Signed-off-by: Rudolf Marek <r.marek at sh.cvut.cz>

Regards
Rudolf
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 01-w83627ehf-add_volt.patch
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060307/7689c642/01-w83627ehf-add_volt.pl

[lm-sensors] [PATCH] Add voltage support to W83627EHF

[Rudolf Marek]

Hello all,

This patch adds the voltage alarms support into W83627EHF.

Signed-off-by: Rudolf Marek <r.marek at sh.cvut.cz>

Regards
Rudolf
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 02-w83627ehf-add_volt_alarms.patch
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060307/812d1806/02-w83627ehf-add_volt_alarms.pl

[lm-sensors] [PATCH] Add voltage support to W83627EHF

[Jean Delvare]

Hi Rudolf,

On 2006-03-07, Rudolf Marek wrote:
> All fixed, here we go...
>
> This patch adds the voltage measuring support to W83627EHF. The code is
> based on the patch provided by Yuan Mu from Winbond.

Looks good, I'll apply it this evening (please remind me on IRC). Thanks.

Please also apply the corresponding user-space patch to our CVS
repository (taking my earlier comments into account.)

--
Jean Delvare

[lm-sensors] [PATCH] Add voltage support to W83627EHF

[Jean Delvare]

Hi Rudolf,

On 2006-03-07, Rudolf Marek wrote:
> This patch adds the voltage alarms support into W83627EHF.

Only voltages? I'd prefer a patch adding all alarms support, it
wouldn't be much larger, and would make things easier for me and the
users.

Also, I see that you adopted my proposal for standard alarms
representation. As much as I enjoy it, merging it right now wouldn't be
very fair to Hans de Goede, whose proposal is still being discussed.

For these two reasons, I won't accept your patch right now. If you want
to help Hans and me solve the standard alarms interface problem, you
could prepare two patches implementing alarms, one following my proposal
(basically the patch you just posted, plus temperatures and fans) and
one following Hans' proposal. We could then compare them on a technical
basis, and you could also give us your opinion as a driver author.

As far as I am concerned, I have finished an experimental patch
implementing my proposed alarms and beeps interface for the w83627hf
driver (this adds to the f71805f, lm63 and lm90 I had done earlier) and
have gathered some numbers about the four drivers.

The only data I am missing now is the memory used by each additional
sysfs file we create. We need to know, as Hans objected that too many
sysfs files could have a negative impact on memory consumption. I dug
down the sysfs code yeterday evening to find out, but didn't find what
I was looking for yet. I hope to get the answer this evening.

Thanks,
--
Jean Delvare

[lm-sensors] [PATCH] Add voltage support to W83627EHF

[David Hubbard]

Hi Rudolf,

> This patch adds the voltage alarms support into W83627EHF.

I'm probably just not getting it, but in order for me to apply the
patches, I've had to work with them to get them all in the kernel.
There isn't a backward-compatible 'alarms' interface or any
'tempN_alarm' anymore, so did I miss a patch?

If I am using the same source as you, I can post a patch to go from
w83627ehf_add_fancrt.patch to full RPM cruise support, as we
discussed. I just need to make sure we're on the same source. So
w83627ehf_wget.sh is a script that downloads and patches the
2.6.16-rc5-mm kernel. I'm also attaching the two patches you just
posted and w83627ehf_add_fancrt.patch, so these should be all the
files needed (plus an internet connection for wget) to get the source
we're working with. Rudolf, let me know if the file I end up with
doesn't match your file.
$ md5sum w83627ehf.c
1c6cdafad1c4c4e84350f5171f8ad834  w83627ehf.c

I'm also attaching a regression test script for the w83627ehf driver.
This is what I used to test the driver on my machine. I found only two
issues:

First, pwmN_mode sets PWM or DC mode. In the datasheet, the bit is
0->PWM and 1->DC but the comment in the driver says 1->PWM and 0->DC.
That's how w83792d.c does it too. So right now, the hardware behavior
is backward, relative to what's "standard" between the drivers. I
think the solution is to invert the bit in store_pwm_mode(), so that
1->PWM (written as a 0) and 0->DC (written as a 1).

Second, this code has a bug: store_pwm_enable() ...
	if (data->pwm_enable[nr] != val) {
		if (!val)
			data->pwm[nr] = 0xff; /* BUG here */
		data->pwm_enable[nr] = val;
		reg = (reg & ~(11 << W83627EHF_PWM_ENALE_SHIFT[nr]));
		reg |= W83627EHF_PWM_MODE_USER_MAP[val] << W83627EHF_PWM_ENALE_SHIFT[nr];
		w83627ehf_write_value(client, W83627EHF_REG_PWM_ENABLE[nr],
					reg);
	}

The problem is that just setting the value in the struct
w83627ehf_data doesn't write it to the register, and the next time
w83627ehf_update_device() comes around, it overwrites it. Thus, the
pwm[nr] = 0xff has no effect. This is not how I understood the
"disabled" mode to work. Also, when pwm_enable = 0, calling
store_pwm() with a new value will still write it to the device, so the
pwm is not write-protected.

Thanks,
David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627ehf_wget.sh
Type: application/x-sh
Size: 2492 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060308/0b38774c/w83627ehf_wget-0001.sh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 01-w83627ehf-add_volt.patch
Type: application/octet-stream
Size: 6623 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060308/0b38774c/01-w83627ehf-add_volt-0001.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 02-w83627ehf-add_volt_alarms.patch
Type: application/octet-stream
Size: 3077 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060308/0b38774c/02-w83627ehf-add_volt_alarms-0001.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627ehf_add_fancrt.patch
Type: application/octet-stream
Size: 15586 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060308/0b38774c/w83627ehf_add_fancrt-0001.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627ehf_regression.sh
Type: application/x-sh
Size: 13906 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060308/0b38774c/w83627ehf_regression-0001.sh

[lm-sensors] [PATCH] Add voltage support to W83627EHF

[Greg KH]

On Wed, Mar 08, 2006 at 02:13:30PM +0100, Jean Delvare wrote:
> 
> The only data I am missing now is the memory used by each additional
> sysfs file we create. We need to know, as Hans objected that too many
> sysfs files could have a negative impact on memory consumption. I dug
> down the sysfs code yeterday evening to find out, but didn't find what
> I was looking for yet. I hope to get the answer this evening.

sysfs files are very light.  The "large" memory structures for a ram
based file system are the dentry and inode structures.  sysfs now
creates them on the fly when they are needed, and if we have memory
pressure on our internal kernel caches, they are freed.

So in short, don't worry about creating new sysfs files, it's not an
issue.  The people running 20,000 disks on a 31bit s390 system have
already done the hard work for you :)

thanks,

greg k-h

[lm-sensors] [PATCH] Add voltage support to W83627EHF

[Jean Delvare]

Hi Greg,

> > The only data I am missing now is the memory used by each additional
> > sysfs file we create. We need to know, as Hans objected that too many
> > sysfs files could have a negative impact on memory consumption. I dug
> > down the sysfs code yeterday evening to find out, but didn't find what
> > I was looking for yet. I hope to get the answer this evening.
> 
> sysfs files are very light.  The "large" memory structures for a ram
> based file system are the dentry and inode structures.  sysfs now
> creates them on the fly when they are needed, and if we have memory
> pressure on our internal kernel caches, they are freed.
> 
> So in short, don't worry about creating new sysfs files, it's not an
> issue.  The people running 20,000 disks on a 31bit s390 system have
> already done the hard work for you :)

Thanks for your enlightened comment on this. Now, this still raises the
question of how much the dentry and inode structures take. You say that
they are created on the fly, but let's imagine a hardware monitoring
utility polling the sysfs files on a regular basis, I guess that these
structures could be considered as "permanently allocated" for this set
of sysfs files, so the dentry and inode sizes would start to matter.

From /proc/slabinfo, I get the following sizes:
x86: dentry 124 bytes, inode 336 bytes
x86_64: dentry 200 bytes, inode 608 bytes

For 33 additional files, counting one of each structure per file (which
might not be correct, I'm really ignorant of how it actually works),
it's around 17 kB on x86 and 29 kB on x86_64. While still not
unacceptable (IMHO at least), this is much more than my first
estimation.

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] Add voltage support to W83627EHF

[Greg KH]

On Thu, Mar 09, 2006 at 09:21:56PM +0100, Jean Delvare wrote:
> Hi Greg,
> 
> > > The only data I am missing now is the memory used by each additional
> > > sysfs file we create. We need to know, as Hans objected that too many
> > > sysfs files could have a negative impact on memory consumption. I dug
> > > down the sysfs code yeterday evening to find out, but didn't find what
> > > I was looking for yet. I hope to get the answer this evening.
> > 
> > sysfs files are very light.  The "large" memory structures for a ram
> > based file system are the dentry and inode structures.  sysfs now
> > creates them on the fly when they are needed, and if we have memory
> > pressure on our internal kernel caches, they are freed.
> > 
> > So in short, don't worry about creating new sysfs files, it's not an
> > issue.  The people running 20,000 disks on a 31bit s390 system have
> > already done the hard work for you :)
> 
> Thanks for your enlightened comment on this. Now, this still raises the
> question of how much the dentry and inode structures take. You say that
> they are created on the fly, but let's imagine a hardware monitoring
> utility polling the sysfs files on a regular basis, I guess that these
> structures could be considered as "permanently allocated" for this set
> of sysfs files, so the dentry and inode sizes would start to matter.

No, if memory pressure is high, they are flushed out of the kernel
caches and the memory is used by whatever else needed it.  So, it
doesn't matter how recently you touched it, if the kernel really needs
that memory due to something else, it _will_ take it back.

> >From /proc/slabinfo, I get the following sizes:
> x86: dentry 124 bytes, inode 336 bytes
> x86_64: dentry 200 bytes, inode 608 bytes

That sounds about right.

> For 33 additional files, counting one of each structure per file (which
> might not be correct, I'm really ignorant of how it actually works),
> it's around 17 kB on x86 and 29 kB on x86_64. While still not
> unacceptable (IMHO at least), this is much more than my first
> estimation.

But again, that memory is "free" so don't worry about it :)

The main point here is that yes, adding new sysfs files do take up more
memory, but don't be scared of it.  You add another disk to your box and
suddenly you just allocated way more sysfs files than that.  And again,
the people with tiny amounts of memory and 20000 disks have already
proven that loads of sysfs files are not a problem.

thanks,

greg k-h

[lm-sensors] [PATCH] Add voltage support to W83627EHF

[Rudolf Marek]

Hi David,

I finally got some time to look continue with this. Many thanks for looking into this stuff.

Please feel free to continue with this automatic fan stuff. It seems that you have time that I dont have
right now. Usually I come from work at 9pm and after 12 hours of other work is quite hard to continue here.

> I'm probably just not getting it, but in order for me to apply the
> patches, I've had to work with them to get them all in the kernel.
> There isn't a backward-compatible 'alarms' interface or any
> 'tempN_alarm' anymore, so did I miss a patch?

Well Jean did already some work today to support the alarms.
I'm attaching the driver with all patches applied except the automatic fan which is in separate patch.

Please fix it and prepare it for kernel inclusion/review. Of course you will get credit for this.
Let me know if you will do the EHF doc too. If not I can try to handle this while in train back to hometown.

> If I am using the same source as you, I can post a patch to go from
> w83627ehf_add_fancrt.patch to full RPM cruise support, as we
> discussed. I just need to make sure we're on the same source. So
> w83627ehf_wget.sh is a script that downloads and patches the
> 2.6.16-rc5-mm kernel. I'm also attaching the two patches you just
> posted and w83627ehf_add_fancrt.patch, so these should be all the
> files needed (plus an internet connection for wget) to get the source
> we're working with. Rudolf, let me know if the file I end up with
> doesn't match your file.
> $ md5sum w83627ehf.c
> 1c6cdafad1c4c4e84350f5171f8ad834  w83627ehf.c

Uff better to post a file for me, see the w83627ehf.c, which has all patches on the top and is in sync with 2.6.16 + all neccessary stuff.

> 
> I'm also attaching a regression test script for the w83627ehf driver.
> This is what I used to test the driver on my machine. I found only two
> issues:

WOW I think we need such test. Have you some methodology for the test construction please?

> First, pwmN_mode sets PWM or DC mode. In the datasheet, the bit is
> 0->PWM and 1->DC but the comment in the driver says 1->PWM and 0->DC.
> That's how w83792d.c does it too. So right now, the hardware behavior
> is backward, relative to what's "standard" between the drivers. I
> think the solution is to invert the bit in store_pwm_mode(), so that
> 1->PWM (written as a 0) and 0->DC (written as a 1).

Yes this is correct W83792D has it vice versa. Please fix that.

> Second, this code has a bug: store_pwm_enable() ...
> 	if (data->pwm_enable[nr] != val) {
> 		if (!val)
> 			data->pwm[nr] = 0xff; /* BUG here */
> 		data->pwm_enable[nr] = val;
> 		reg = (reg & ~(11 << W83627EHF_PWM_ENALE_SHIFT[nr]));
> 		reg |= W83627EHF_PWM_MODE_USER_MAP[val] << W83627EHF_PWM_ENALE_SHIFT[nr];
> 		w83627ehf_write_value(client, W83627EHF_REG_PWM_ENABLE[nr],
> 					reg);
> 	}
> 
> The problem is that just setting the value in the struct
> w83627ehf_data doesn't write it to the register, and the next time
> w83627ehf_update_device() comes around, it overwrites it. Thus, the
> pwm[nr] = 0xff has no effect. This is not how I understood the
> "disabled" mode to work. Also, when pwm_enable = 0, calling
> store_pwm() with a new value will still write it to the device, so the
> pwm is not write-protected.

Well yes this is my fault this was just a quick hack to the disable mode...

Jean, is there any specs how to disable the pwmX files when interface is disabled?

Regards
Rudolf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627ehf.c
Type: application/octet
Size: 29389 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060323/3aaf5269/w83627ehf-0001.bin
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: add_fan_crt
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060323/3aaf5269/add_fan_crt-0001.pl
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: docs
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060323/3aaf5269/docs-0001.pl

[lm-sensors] [PATCH] Add voltage support to W83627EHF

[David Hubbard]

Hi Rudolf,

Oops, re-post and send to the list!

On 3/23/06, Rudolf Marek <r.marek at sh.cvut.cz> wrote:
> Hi David,
>
> I finally got some time to look continue with this. Many thanks for looking into this stuff.
>
> Please feel free to continue with this automatic fan stuff. It seems that you have time that I dont have
> right now. Usually I come from work at 9pm and after 12 hours of other work is quite hard to continue here.

> Well Jean did already some work today to support the alarms.
> I'm attaching the driver with all patches applied except the automatic fan which is in separate patch.

...

> Uff better to post a file for me, see the w83627ehf.c, which has all patches on the top and is in sync with 2.6.16 + all neccessary stuff.

Okay, I'll look at this one, thanks for send the file!

> Please fix it and prepare it for kernel inclusion/review. Of course you will get credit for this.
> Let me know if you will do the EHF doc too. If not I can try to handle this while in train back to hometown.

I can document the code, though I'd appreciate your help too!

> >
> > I'm also attaching a regression test script for the w83627ehf driver.
> > This is what I used to test the driver on my machine. I found only two
> > issues:
>
> WOW I think we need such test. Have you some methodology for the test construction please?

Well, it's just a shell script... although I think several parts are
still missing. Such as, how to test voltage / VID values for
correctness? Some CPUs can be underclocked / undervolted, which would
at least provide a simple test for these inputs. If this is useful,
then that's great!

> > First, pwmN_mode sets PWM or DC mode. In the datasheet, the bit is
> > 0->PWM and 1->DC but the comment in the driver says 1->PWM and 0->DC.
> > That's how w83792d.c does it too. So right now, the hardware behavior
> > is backward, relative to what's "standard" between the drivers. I
> > think the solution is to invert the bit in store_pwm_mode(), so that
> > 1->PWM (written as a 0) and 0->DC (written as a 1).
>
> Yes this is correct W83792D has it vice versa. Please fix that.

OK

- David

[lm-sensors] [PATCH] Add voltage support to W83627EHF

[David Hubbard]

Hi Rudolf,

I applied the patch in your email and tested it. It works fine. I
updated the regression test to match the new alarm sysfs entries.

David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627ehf_regression.sh
Type: application/x-sh
Size: 14798 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060324/e53d7e86/w83627ehf_regression.sh

[lm-sensors] [PATCH] Add voltage support to W83627EHF

[David Hubbard]

Hi Rudolf,

> Yes this is correct W83792D has it vice versa. Please fix that.

Patch to fix that below, with updated regression test that gets to the
point where echo 0 > pwm_enable should set the fan to 100% and
doesn't. Anyway, that's coming up next.

-David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627ehf_pwm_mode_fix.patch
Type: application/octet-stream
Size: 41270 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060324/ca4609c4/w83627ehf_pwm_mode_fix-0001.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627ehf_regression.sh
Type: application/x-sh
Size: 14765 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060324/ca4609c4/w83627ehf_regression-0001.sh