* Re: [lm-sensors] [PATCH] Add temperature-tracking mode to f71805f
2007-06-19 20:38 [lm-sensors] [PATCH] Add temperature-tracking mode to f71805f Phil Endecott
@ 2007-06-24 9:54 ` Jean Delvare
2007-06-24 13:02 ` Mark M. Hoffman
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Jean Delvare @ 2007-06-24 9:54 UTC (permalink / raw)
To: lm-sensors
Hi Phil,
On Tue, 19 Jun 2007 21:38:27 +0100, Phil Endecott wrote:
> This second version of the patch includes documentation updates, adds
> the 'channels' sysfs files that describe the fixed relationships
> between pwms, fans and temperatures, reverses the order of the
> auto_points so that greater point number corresponds to greater
> temperature, and addresses the mostly-stylistic issues raised by Jean
> earlier today. It seems to work for me but as I've noted before I can
> only test the channel-1 functionality, so I'd appreciate it if others
> could scan for sanity in the other channels.
$ quilt push
Application de hwmon-f71805f-add-temperature-tracking-mode.patch
patching file drivers/hwmon/f71805f.c
Hunk #1 FAILED at 126.
Hunk #2 FAILED at 149.
Hunk #3 FAILED at 180.
Hunk #4 FAILED at 323.
Hunk #5 FAILED at 353.
Hunk #6 FAILED at 728.
Hunk #7 FAILED at 1029.
Hunk #8 FAILED at 1163.
8 out of 8 hunks FAILED -- rejects in file drivers/hwmon/f71805f.c
patching file Documentation/hwmon/f71805f
Hunk #1 FAILED at 38.
Hunk #2 FAILED at 130.
patch: **** malformed patch at line 437: pwm2 to
How exactly did you generate this patch? Please send something I can
apply.
BTW:
> Signed-off-by: Phil Endecott <spam_from_hwmon_patch_2@chezphil.org>
I'm not sure I can push this upstream, when you are so obviously
providing a soon-to-be-invalid e-mail address. It's OK to sign your
kernel patches with a specific e-mail address, but it shouldn't change
with every patch. Mark, what do you think?
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [lm-sensors] [PATCH] Add temperature-tracking mode to f71805f
2007-06-19 20:38 [lm-sensors] [PATCH] Add temperature-tracking mode to f71805f Phil Endecott
2007-06-24 9:54 ` Jean Delvare
@ 2007-06-24 13:02 ` Mark M. Hoffman
2007-06-25 21:33 ` Jean Delvare
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Mark M. Hoffman @ 2007-06-24 13:02 UTC (permalink / raw)
To: lm-sensors
Hi Jean, Phil:
> On Tue, 19 Jun 2007 21:38:27 +0100, Phil Endecott wrote:
> > This second version of the patch includes documentation updates, adds
> > the 'channels' sysfs files that describe the fixed relationships
> > between pwms, fans and temperatures, reverses the order of the
> > auto_points so that greater point number corresponds to greater
> > temperature, and addresses the mostly-stylistic issues raised by Jean
> > earlier today. It seems to work for me but as I've noted before I can
> > only test the channel-1 functionality, so I'd appreciate it if others
> > could scan for sanity in the other channels.
* Jean Delvare <khali@linux-fr.org> [2007-06-24 11:54:06 +0200]:
> $ quilt push
> Application de hwmon-f71805f-add-temperature-tracking-mode.patch
> patching file drivers/hwmon/f71805f.c
> Hunk #1 FAILED at 126.
> Hunk #2 FAILED at 149.
> Hunk #3 FAILED at 180.
> Hunk #4 FAILED at 323.
> Hunk #5 FAILED at 353.
> Hunk #6 FAILED at 728.
> Hunk #7 FAILED at 1029.
> Hunk #8 FAILED at 1163.
> 8 out of 8 hunks FAILED -- rejects in file drivers/hwmon/f71805f.c
> patching file Documentation/hwmon/f71805f
> Hunk #1 FAILED at 38.
> Hunk #2 FAILED at 130.
> patch: **** malformed patch at line 437: pwm2 to
>
> How exactly did you generate this patch? Please send something I can
> apply.
>
> BTW:
>
> > Signed-off-by: Phil Endecott <spam_from_hwmon_patch_2@chezphil.org>
>
> I'm not sure I can push this upstream, when you are so obviously
> providing a soon-to-be-invalid e-mail address. It's OK to sign your
> kernel patches with a specific e-mail address, but it shouldn't change
> with every patch. Mark, what do you think?
I agree with you Jean. Phil: please pick one address for all sign-off, and
make sure that your patches come From: that same address. Thanks.
Regards,
--
Mark M. Hoffman
mhoffman@lightlink.com
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [lm-sensors] [PATCH] Add temperature-tracking mode to f71805f
2007-06-19 20:38 [lm-sensors] [PATCH] Add temperature-tracking mode to f71805f Phil Endecott
2007-06-24 9:54 ` Jean Delvare
2007-06-24 13:02 ` Mark M. Hoffman
@ 2007-06-25 21:33 ` Jean Delvare
2007-06-26 20:41 ` Jean Delvare
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Jean Delvare @ 2007-06-25 21:33 UTC (permalink / raw)
To: lm-sensors
Hi Phil,
On Sun, 24 Jun 2007 11:49:39 +0100, Phil Endecott wrote:
> Jean Delvare wrote:
> > $ quilt push
> > Application de hwmon-f71805f-add-temperature-tracking-mode.patch
> > patching file drivers/hwmon/f71805f.c
> > Hunk #1 FAILED at 126.
> > Hunk #2 FAILED at 149.
> > Hunk #3 FAILED at 180.
> > Hunk #4 FAILED at 323.
> > Hunk #5 FAILED at 353.
> > Hunk #6 FAILED at 728.
> > Hunk #7 FAILED at 1029.
> > Hunk #8 FAILED at 1163.
>
> Ooops, sorry. It looks like the start-of-file lines got word-wrapped between the
> filename and the timestamp somewhere in the email. I'll try again (below).
Much better, thanks. Patch tested, it work fine for me.
> --- linux-2.6-2.6.21/drivers/hwmon/f71805f.c.orig 2007-06-16 00:10:54.000000000 +0100
> +++ linux-2.6-2.6.21/drivers/hwmon/f71805f.c 2007-06-19 21:07:26.000000000 +0100
> @@ -126,6 +126,12 @@
> #define F71805F_REG_TEMP_HIGH(nr) (0x54 + 2 * (nr))
> #define F71805F_REG_TEMP_HYST(nr) (0x55 + 2 * (nr))
> #define F71805F_REG_TEMP_MODE 0x01
> +/* pwm/fan pwmnr from 0 to 2, auto point apnr from 0 to 2 */
> +/* map fintek numbers to our numbers as follows: 9->0, 5->1, 1->2. */
> +#define F71805F_REG_PWM_AUTO_POINT_TEMP(pwmnr, apnr) \
> + (0xA0 + 0x10 * (pwmnr) + (2-apnr))
> +#define F71805F_REG_PWM_AUTO_POINT_FAN(pwmnr, apnr) \
> + (0xA4 + 0x10 * (pwmnr) + 2 * (2-apnr))
>
> #define F71805F_REG_START 0x00
> /* status nr from 0 to 2 */
> @@ -143,6 +149,11 @@
> * Data structures and manipulation thereof
> */
>
> +struct f71805f_auto_point {
> + u8 temp[3];
> + u16 fan[3];
> +};
> +
> struct f71805f_data {
> unsigned short addr;
> const char *name;
> @@ -169,6 +180,7 @@
> u8 temp_hyst[3];
> u8 temp_mode;
> unsigned long alarms;
> + struct f71805f_auto_point auto_points[3];
> };
>
> struct f71805f_sio_data {
> @@ -311,7 +323,7 @@
> static struct f71805f_data *f71805f_update_device(struct device *dev)
> {
> struct f71805f_data *data = dev_get_drvdata(dev);
> - int nr;
> + int nr, apnr;
>
> mutex_lock(&data->update_lock);
>
> @@ -341,6 +353,18 @@
> F71805F_REG_TEMP_HYST(nr));
> }
> data->temp_mode = f71805f_read8(data, F71805F_REG_TEMP_MODE);
> + for (nr = 0; nr < 3; nr++) {
> + for (apnr = 0; apnr < 3; apnr++) {
> + data->auto_points[nr].temp[apnr] > + f71805f_read8(data,
> + F71805F_REG_PWM_AUTO_POINT_TEMP(nr,
> + apnr));
> + data->auto_points[nr].fan[apnr] > + f71805f_read16(data,
> + F71805F_REG_PWM_AUTO_POINT_FAN(nr,
> + apnr));
> + }
> + }
>
> data->last_limits = jiffies;
> }
> @@ -704,6 +728,80 @@
> return count;
> }
>
> +static ssize_t show_pwm_auto_channels(struct device *dev,
> + struct device_attribute *devattr,
> + char* buf)
> +{
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> + int nr = attr->index;
> +
> + return sprintf(buf, "%d\n", 1<<nr);
> +}
> +
> +static ssize_t show_pwm_auto_point_temp(struct device *dev,
> + struct device_attribute *devattr,
> + char* buf)
> +{
> + struct f71805f_data *data = dev_get_drvdata(dev);
> + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> + int pwmnr = attr->nr;
> + int apnr = attr->index;
Broken indentation.
> +
> + return sprintf(buf, "%ld\n",
> + temp_from_reg(data->auto_points[pwmnr].temp[apnr]));
> +}
> +
> +static ssize_t set_pwm_auto_point_temp(struct device *dev,
> + struct device_attribute *devattr,
> + const char* buf, size_t count)
> +{
> + struct f71805f_data *data = dev_get_drvdata(dev);
> + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> + int pwmnr = attr->nr;
> + int apnr = attr->index;
Here again.
> + unsigned long val = simple_strtoul(buf, NULL, 10);
Temperatures can be negative -> simple_strtol().
> +
> + mutex_lock(&data->update_lock);
> + data->auto_points[pwmnr].temp[apnr] = temp_to_reg(val);
> + f71805f_write8(data, F71805F_REG_PWM_AUTO_POINT_TEMP(pwmnr, apnr),
> + data->auto_points[pwmnr].temp[apnr]);
> + mutex_unlock(&data->update_lock);
> +
> + return count;
> +}
> +
> +static ssize_t show_pwm_auto_point_fan(struct device *dev,
> + struct device_attribute *devattr,
> + char* buf)
> +{
> + struct f71805f_data *data = dev_get_drvdata(dev);
> + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> + int pwmnr = attr->nr;
> + int apnr = attr->index;
And again.
> +
> + return sprintf(buf, "%ld\n",
> + fan_from_reg(data->auto_points[pwmnr].fan[apnr]));
> +}
> +
> +static ssize_t set_pwm_auto_point_fan(struct device *dev,
> + struct device_attribute *devattr,
> + const char* buf, size_t count)
> +{
> + struct f71805f_data *data = dev_get_drvdata(dev);
> + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> + int pwmnr = attr->nr;
> + int apnr = attr->index;
And again. Ahhh, cut'n'paste ;)
> + unsigned long val = simple_strtoul(buf, NULL, 10);
> +
> + mutex_lock(&data->update_lock);
> + data->auto_points[pwmnr].fan[apnr] = fan_to_reg(val);
> + f71805f_write16(data, F71805F_REG_PWM_AUTO_POINT_FAN(pwmnr, apnr),
> + data->auto_points[pwmnr].fan[apnr]);
> + mutex_unlock(&data->update_lock);
> +
> + return count;
> +}
> +
> static ssize_t show_temp(struct device *dev, struct device_attribute *devattr,
> char *buf)
> {
> @@ -931,6 +1029,58 @@
> show_pwm_freq, set_pwm_freq, 2);
> static SENSOR_DEVICE_ATTR(pwm3_mode, S_IRUGO, show_pwm_mode, NULL, 2);
>
> +static SENSOR_DEVICE_ATTR(pwm1_auto_channels_temp, S_IRUGO,
> + show_pwm_auto_channels, NULL, 0);
> +static SENSOR_DEVICE_ATTR(pwm2_auto_channels_temp, S_IRUGO,
> + show_pwm_auto_channels, NULL, 1);
> +static SENSOR_DEVICE_ATTR(pwm3_auto_channels_temp, S_IRUGO,
> + show_pwm_auto_channels, NULL, 2);
> +static SENSOR_DEVICE_ATTR(pwm1_auto_channels_fan, S_IRUGO,
> + show_pwm_auto_channels, NULL, 0);
> +static SENSOR_DEVICE_ATTR(pwm2_auto_channels_fan, S_IRUGO,
> + show_pwm_auto_channels, NULL, 1);
> +static SENSOR_DEVICE_ATTR(pwm3_auto_channels_fan, S_IRUGO,
> + show_pwm_auto_channels, NULL, 2);
Not sure what others think about this (Mark? Juerg?) but this (pwm1 to
fan1 and to temp1, etc.) seems to be the usual mapping for chips where
the mappings can't be changed, so I wonder what's the benefit of
creating read-only sysfs files to express it. I think I'd be just as
happy with no channel files at all (and it's cheaper.) No strong
opinion though.
> +
> +static SENSOR_DEVICE_ATTR_2(pwm1_auto_point1_temp, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_temp, set_pwm_auto_point_temp, 0, 0);
> +static SENSOR_DEVICE_ATTR_2(pwm1_auto_point1_fan, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_fan, set_pwm_auto_point_fan, 0, 0);
> +static SENSOR_DEVICE_ATTR_2(pwm1_auto_point2_temp, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_temp, set_pwm_auto_point_temp, 0, 1);
> +static SENSOR_DEVICE_ATTR_2(pwm1_auto_point2_fan, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_fan, set_pwm_auto_point_fan, 0, 1);
> +static SENSOR_DEVICE_ATTR_2(pwm1_auto_point3_temp, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_temp, set_pwm_auto_point_temp, 0, 2);
> +static SENSOR_DEVICE_ATTR_2(pwm1_auto_point3_fan, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_fan, set_pwm_auto_point_fan, 0, 2);
> +
> +static SENSOR_DEVICE_ATTR_2(pwm2_auto_point1_temp, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_temp, set_pwm_auto_point_temp, 1, 0);
> +static SENSOR_DEVICE_ATTR_2(pwm2_auto_point1_fan, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_fan, set_pwm_auto_point_fan, 1, 0);
> +static SENSOR_DEVICE_ATTR_2(pwm2_auto_point2_temp, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_temp, set_pwm_auto_point_temp, 1, 1);
> +static SENSOR_DEVICE_ATTR_2(pwm2_auto_point2_fan, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_fan, set_pwm_auto_point_fan, 1, 1);
> +static SENSOR_DEVICE_ATTR_2(pwm2_auto_point3_temp, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_temp, set_pwm_auto_point_temp, 1, 2);
> +static SENSOR_DEVICE_ATTR_2(pwm2_auto_point3_fan, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_fan, set_pwm_auto_point_fan, 1, 2);
> +
> +static SENSOR_DEVICE_ATTR_2(pwm3_auto_point1_temp, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_temp, set_pwm_auto_point_temp, 2, 0);
> +static SENSOR_DEVICE_ATTR_2(pwm3_auto_point1_fan, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_fan, set_pwm_auto_point_fan, 2, 0);
> +static SENSOR_DEVICE_ATTR_2(pwm3_auto_point2_temp, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_temp, set_pwm_auto_point_temp, 2, 1);
> +static SENSOR_DEVICE_ATTR_2(pwm3_auto_point2_fan, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_fan, set_pwm_auto_point_fan, 2, 1);
> +static SENSOR_DEVICE_ATTR_2(pwm3_auto_point3_temp, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_temp, set_pwm_auto_point_temp, 2, 2);
> +static SENSOR_DEVICE_ATTR_2(pwm3_auto_point3_fan, S_IRUGO | S_IWUSR,
> + show_pwm_auto_point_fan, set_pwm_auto_point_fan, 2, 2);
> +
> static SENSOR_DEVICE_ATTR(in0_alarm, S_IRUGO, show_alarm, NULL, 0);
> static SENSOR_DEVICE_ATTR(in1_alarm, S_IRUGO, show_alarm, NULL, 1);
> static SENSOR_DEVICE_ATTR(in2_alarm, S_IRUGO, show_alarm, NULL, 2);
> @@ -1013,6 +1163,32 @@
> &sensor_dev_attr_temp3_max_hyst.dev_attr.attr,
> &sensor_dev_attr_temp3_type.dev_attr.attr,
>
> + &sensor_dev_attr_pwm1_auto_channels_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm2_auto_channels_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm3_auto_channels_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_channels_fan.dev_attr.attr,
> + &sensor_dev_attr_pwm2_auto_channels_fan.dev_attr.attr,
> + &sensor_dev_attr_pwm3_auto_channels_fan.dev_attr.attr,
> +
> + &sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_point1_fan.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_point2_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_point2_fan.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_point3_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm1_auto_point3_fan.dev_attr.attr,
> + &sensor_dev_attr_pwm2_auto_point1_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm2_auto_point1_fan.dev_attr.attr,
> + &sensor_dev_attr_pwm2_auto_point2_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm2_auto_point2_fan.dev_attr.attr,
> + &sensor_dev_attr_pwm2_auto_point3_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm2_auto_point3_fan.dev_attr.attr,
> + &sensor_dev_attr_pwm3_auto_point1_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm3_auto_point1_fan.dev_attr.attr,
> + &sensor_dev_attr_pwm3_auto_point2_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm3_auto_point2_fan.dev_attr.attr,
> + &sensor_dev_attr_pwm3_auto_point3_temp.dev_attr.attr,
> + &sensor_dev_attr_pwm3_auto_point3_fan.dev_attr.attr,
> +
> &sensor_dev_attr_in0_alarm.dev_attr.attr,
> &sensor_dev_attr_in1_alarm.dev_attr.attr,
> &sensor_dev_attr_in2_alarm.dev_attr.attr,
> --- linux-2.6-2.6.21/Documentation/hwmon/f71805f.orig 2007-06-16 22:10:33.000000000 +0100
> +++ linux-2.6-2.6.21/Documentation/hwmon/f71805f 2007-06-19 21:14:35.000000000 +0100
> @@ -38,6 +38,8 @@
> additional internal voltages monitored (VSB and battery). It also features
> 6 VID inputs. The VID inputs are not yet supported by this driver.
>
> +Datasheets for both chips are available from the Fintek website.
> +
Err, no. Please instead replace the datasheet info at the beginning of
the file:
Datasheet: Provided by Fintek on request
with
Datasheet: http://www.fintek.com.tw/etc etc.
> The driver assumes that no more than one chip is present, which seems
> reasonable.
>
> @@ -128,7 +130,9 @@
> When the PWM method is used, you can select the operating frequency,
> from 187.5 kHz (default) to 31 Hz. The best frequency depends on the
> fan model. As a rule of thumb, lower frequencies seem to give better
> -control, but may generate annoying high-pitch noise. Fintek recommends
> +control, but may generate annoying high-pitch noise. So a frequency just
> +above the audible range, such as 25 kHz, may be a good choice; if this
> +doesn't give you good linear control, try reducing it. Fintek recommends
> not going below 1 kHz, as the fan tachometers get confused by lower
> frequencies as well.
>
> @@ -136,16 +140,25 @@
> corresponds to a pwm value of 106 for the driver. The driver doesn't
> enforce this limit though.
>
> -Three different fan control modes are supported:
> +Three different fan control modes are supported; the mode number is written
> +to the pwm<n>_enable file.
> +
> +* 1: Manual mode
> + You ask for a specific PWM duty cycle or DC voltage by writing to the
> + pwm<n> file.
> +
> +* 2: Temperature mode
> + You define 3 temperature/fan speed trip points using the
> + pwm<n>_auto_point<m>_temp and _fan files. These define a staircase
> + relationship between temperature and fan speed with two additional points
> + interpolated between the values that you define. When the temperature
> + is below auto_point1_temp the fan is switched off.
> +
> +* 3: Fan speed mode
> + You ask for a specific fan speed by writing to the fan<n>_target file.
> +
> +Both of the automatic modes require that pwm1 corresponds to fan1, pwm2 to
> +fan2 and pwm3 to fan3. Temperature mode also requires that temp1 corresponds
> +to pwm1 and fan1, etc.
>
> -* Manual mode
> - You ask for a specific PWM duty cycle or DC voltage.
>
> -* Fan speed mode
> - You ask for a specific fan speed. This mode assumes that pwm1
> - corresponds to fan1, pwm2 to fan2 and pwm3 to fan3.
> -
> -* Temperature mode
> - You define 3 temperature/fan speed trip points, and the fan speed is
> - adjusted depending on the measured temperature, using interpolation.
> - This mode is not yet supported by the driver.
> --- linux-2.6-2.6.21/Documentation/hwmon/sysfs-interface.orig 2007-06-16 22:09:44.000000000 +0100
> +++ linux-2.6-2.6.21/Documentation/hwmon/sysfs-interface 2007-06-16 22:10:00.000000000 +0100
> @@ -152,6 +152,9 @@
> Note that this is actually an internal clock divisor, which
> affects the measurable speed range, not the read value.
>
> +fan[1-*]_target
> + Target fan speed for automatic speed PWM control modes.
> +
This is already documented in the upstream version:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h-bc514a2ed2b6f71eb6d18671d2c663160788c9
> Also see the Alarms section for status flags associated with fans.
>
>
> @@ -186,15 +189,32 @@
> pwm[1-*]_auto_channels_temp
> Select which temperature channels affect this PWM output in
> auto mode. Bitfield, 1 is temp1, 2 is temp2, 4 is temp3 etc...
> - Which values are possible depend on the chip used.
> - RW
> + Which values are possible and whether they can be changed depend
> + on the chip used.
> + RO or RW
> +
> +pwm[1-*]_auto_channels_fan
> + Select which fan channels affect this PWM output in
> + auto mode. Bitfield, 1 is fan1, 2 is fan2, 4 is fan3 etc...
> + Which values are possible and whether they can be changed depend
> + on the chip used.
> + RO or RW
>
> pwm[1-*]_auto_point[1-*]_pwm
> pwm[1-*]_auto_point[1-*]_temp
> pwm[1-*]_auto_point[1-*]_temp_hyst
> - Define the PWM vs temperature curve. Number of trip points is
> - chip-dependent. Use this for chips which associate trip points
> - to PWM output channels.
> + Define the PWM vs temperature curve for chips which associate
> + trip points to PWM output channels. Number of trip points is
> + chip-dependent.
This change doesn't appear to add any value, you're only swapping the
sentences!
> + RW
> +
> +OR
> +
> +pwm[1-*]_auto_point[1-*]_temp
> +pwm[1-*]_auto_point[1-*]_fan
> + Define the fan speed vs temperature curve for chips which
> + associate trip points to PWM output channels. Number of trip
> + points is chip-dependent.
> RW
This was discussed some times ago and the consensus was that we did not
want to extend this documentation for every new chip doing things
differently:
http://lists.lm-sensors.org/pipermail/lm-sensors/2007-April/019612.html
The thread mentioned the concept of thermal zones. That would be the
only good reason to change the interface documentation IMHO. And that
would be a separate patch anyway, so please exclude
Documentation/sysfs-interface from the next iteration of your patch.
> OR
> @@ -202,9 +222,9 @@
> temp[1-*]_auto_point[1-*]_pwm
> temp[1-*]_auto_point[1-*]_temp
> temp[1-*]_auto_point[1-*]_temp_hyst
> - Define the PWM vs temperature curve. Number of trip points is
> - chip-dependent. Use this for chips which associate trip points
> - to temperature channels.
> + Define the PWM vs temperature curve for chips which associate
> + trip points to temperature channels. Number of trip points is
> + chip-dependent.
> RW
Not valuable either.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [lm-sensors] [PATCH] Add temperature-tracking mode to f71805f
2007-06-19 20:38 [lm-sensors] [PATCH] Add temperature-tracking mode to f71805f Phil Endecott
` (2 preceding siblings ...)
2007-06-25 21:33 ` Jean Delvare
@ 2007-06-26 20:41 ` Jean Delvare
2007-06-26 22:54 ` Juerg Haefliger
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Jean Delvare @ 2007-06-26 20:41 UTC (permalink / raw)
To: lm-sensors
Phil,
On Mon, 25 Jun 2007 23:37:12 +0100, Phil Endecott wrote:
> Jean Delvare wrote:
>
> (in the sysfs documentation)
> >> +OR
> >> +
> >> +pwm[1-*]_auto_point[1-*]_temp
> >> +pwm[1-*]_auto_point[1-*]_fan
> >> + Define the fan speed vs temperature curve for chips which
> >> + associate trip points to PWM output channels. Number of trip
> >> + points is chip-dependent.
> >> RW
> >
> > This was discussed some times ago and the consensus was that we did not
> > want to extend this documentation for every new chip doing things
> > differently
>
> Well currently the file documents 2 of the n ways in which it can work
> as if they are the only two ways.
>
> Would you not prefer to insert at least some mention that it could work
> in other ways?
I agree it can be confusing; adding a statement that other ways are
possible and allowed would be welcome.
> Put yourself in the position of someone who has a board
> with one of these chips, who wants to configure it. If the method is
> left undocumented, they have to guess or read the source.
>
> I would not be pleased if the functionality that I have added goes
> unused because the documentation for it has been rejected.
Actually, you documented the interesting files in
Documentation/hwmon/f71805f already, and this is what the users should
be reading. Documentation/hwmon/sysfs-interface is aimed at the
developers, not users.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [lm-sensors] [PATCH] Add temperature-tracking mode to f71805f
2007-06-19 20:38 [lm-sensors] [PATCH] Add temperature-tracking mode to f71805f Phil Endecott
` (3 preceding siblings ...)
2007-06-26 20:41 ` Jean Delvare
@ 2007-06-26 22:54 ` Juerg Haefliger
2007-06-27 19:05 ` Jean Delvare
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Juerg Haefliger @ 2007-06-26 22:54 UTC (permalink / raw)
To: lm-sensors
On 6/25/07, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Phil,
>
> On Sun, 24 Jun 2007 11:49:39 +0100, Phil Endecott wrote:
> > Jean Delvare wrote:
> > > $ quilt push
> > > Application de hwmon-f71805f-add-temperature-tracking-mode.patch
> > > patching file drivers/hwmon/f71805f.c
> > > Hunk #1 FAILED at 126.
> > > Hunk #2 FAILED at 149.
> > > Hunk #3 FAILED at 180.
> > > Hunk #4 FAILED at 323.
> > > Hunk #5 FAILED at 353.
> > > Hunk #6 FAILED at 728.
> > > Hunk #7 FAILED at 1029.
> > > Hunk #8 FAILED at 1163.
> >
> > Ooops, sorry. It looks like the start-of-file lines got word-wrapped between the
> > filename and the timestamp somewhere in the email. I'll try again (below).
>
> Much better, thanks. Patch tested, it work fine for me.
>
> > --- linux-2.6-2.6.21/drivers/hwmon/f71805f.c.orig 2007-06-16 00:10:54.000000000 +0100
> > +++ linux-2.6-2.6.21/drivers/hwmon/f71805f.c 2007-06-19 21:07:26.000000000 +0100
> > @@ -126,6 +126,12 @@
> > #define F71805F_REG_TEMP_HIGH(nr) (0x54 + 2 * (nr))
> > #define F71805F_REG_TEMP_HYST(nr) (0x55 + 2 * (nr))
> > #define F71805F_REG_TEMP_MODE 0x01
> > +/* pwm/fan pwmnr from 0 to 2, auto point apnr from 0 to 2 */
> > +/* map fintek numbers to our numbers as follows: 9->0, 5->1, 1->2. */
> > +#define F71805F_REG_PWM_AUTO_POINT_TEMP(pwmnr, apnr) \
> > + (0xA0 + 0x10 * (pwmnr) + (2-apnr))
> > +#define F71805F_REG_PWM_AUTO_POINT_FAN(pwmnr, apnr) \
> > + (0xA4 + 0x10 * (pwmnr) + 2 * (2-apnr))
> >
> > #define F71805F_REG_START 0x00
> > /* status nr from 0 to 2 */
> > @@ -143,6 +149,11 @@
> > * Data structures and manipulation thereof
> > */
> >
> > +struct f71805f_auto_point {
> > + u8 temp[3];
> > + u16 fan[3];
> > +};
> > +
> > struct f71805f_data {
> > unsigned short addr;
> > const char *name;
> > @@ -169,6 +180,7 @@
> > u8 temp_hyst[3];
> > u8 temp_mode;
> > unsigned long alarms;
> > + struct f71805f_auto_point auto_points[3];
> > };
> >
> > struct f71805f_sio_data {
> > @@ -311,7 +323,7 @@
> > static struct f71805f_data *f71805f_update_device(struct device *dev)
> > {
> > struct f71805f_data *data = dev_get_drvdata(dev);
> > - int nr;
> > + int nr, apnr;
> >
> > mutex_lock(&data->update_lock);
> >
> > @@ -341,6 +353,18 @@
> > F71805F_REG_TEMP_HYST(nr));
> > }
> > data->temp_mode = f71805f_read8(data, F71805F_REG_TEMP_MODE);
> > + for (nr = 0; nr < 3; nr++) {
> > + for (apnr = 0; apnr < 3; apnr++) {
> > + data->auto_points[nr].temp[apnr] > > + f71805f_read8(data,
> > + F71805F_REG_PWM_AUTO_POINT_TEMP(nr,
> > + apnr));
> > + data->auto_points[nr].fan[apnr] > > + f71805f_read16(data,
> > + F71805F_REG_PWM_AUTO_POINT_FAN(nr,
> > + apnr));
> > + }
> > + }
> >
> > data->last_limits = jiffies;
> > }
> > @@ -704,6 +728,80 @@
> > return count;
> > }
> >
> > +static ssize_t show_pwm_auto_channels(struct device *dev,
> > + struct device_attribute *devattr,
> > + char* buf)
> > +{
> > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > + int nr = attr->index;
> > +
> > + return sprintf(buf, "%d\n", 1<<nr);
> > +}
> > +
> > +static ssize_t show_pwm_auto_point_temp(struct device *dev,
> > + struct device_attribute *devattr,
> > + char* buf)
> > +{
> > + struct f71805f_data *data = dev_get_drvdata(dev);
> > + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> > + int pwmnr = attr->nr;
> > + int apnr = attr->index;
>
> Broken indentation.
>
> > +
> > + return sprintf(buf, "%ld\n",
> > + temp_from_reg(data->auto_points[pwmnr].temp[apnr]));
> > +}
> > +
> > +static ssize_t set_pwm_auto_point_temp(struct device *dev,
> > + struct device_attribute *devattr,
> > + const char* buf, size_t count)
> > +{
> > + struct f71805f_data *data = dev_get_drvdata(dev);
> > + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> > + int pwmnr = attr->nr;
> > + int apnr = attr->index;
>
> Here again.
>
> > + unsigned long val = simple_strtoul(buf, NULL, 10);
>
> Temperatures can be negative -> simple_strtol().
>
> > +
> > + mutex_lock(&data->update_lock);
> > + data->auto_points[pwmnr].temp[apnr] = temp_to_reg(val);
> > + f71805f_write8(data, F71805F_REG_PWM_AUTO_POINT_TEMP(pwmnr, apnr),
> > + data->auto_points[pwmnr].temp[apnr]);
> > + mutex_unlock(&data->update_lock);
> > +
> > + return count;
> > +}
> > +
> > +static ssize_t show_pwm_auto_point_fan(struct device *dev,
> > + struct device_attribute *devattr,
> > + char* buf)
> > +{
> > + struct f71805f_data *data = dev_get_drvdata(dev);
> > + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> > + int pwmnr = attr->nr;
> > + int apnr = attr->index;
>
> And again.
>
> > +
> > + return sprintf(buf, "%ld\n",
> > + fan_from_reg(data->auto_points[pwmnr].fan[apnr]));
> > +}
> > +
> > +static ssize_t set_pwm_auto_point_fan(struct device *dev,
> > + struct device_attribute *devattr,
> > + const char* buf, size_t count)
> > +{
> > + struct f71805f_data *data = dev_get_drvdata(dev);
> > + struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> > + int pwmnr = attr->nr;
> > + int apnr = attr->index;
>
> And again. Ahhh, cut'n'paste ;)
>
> > + unsigned long val = simple_strtoul(buf, NULL, 10);
> > +
> > + mutex_lock(&data->update_lock);
> > + data->auto_points[pwmnr].fan[apnr] = fan_to_reg(val);
> > + f71805f_write16(data, F71805F_REG_PWM_AUTO_POINT_FAN(pwmnr, apnr),
> > + data->auto_points[pwmnr].fan[apnr]);
> > + mutex_unlock(&data->update_lock);
> > +
> > + return count;
> > +}
> > +
> > static ssize_t show_temp(struct device *dev, struct device_attribute *devattr,
> > char *buf)
> > {
> > @@ -931,6 +1029,58 @@
> > show_pwm_freq, set_pwm_freq, 2);
> > static SENSOR_DEVICE_ATTR(pwm3_mode, S_IRUGO, show_pwm_mode, NULL, 2);
> >
> > +static SENSOR_DEVICE_ATTR(pwm1_auto_channels_temp, S_IRUGO,
> > + show_pwm_auto_channels, NULL, 0);
> > +static SENSOR_DEVICE_ATTR(pwm2_auto_channels_temp, S_IRUGO,
> > + show_pwm_auto_channels, NULL, 1);
> > +static SENSOR_DEVICE_ATTR(pwm3_auto_channels_temp, S_IRUGO,
> > + show_pwm_auto_channels, NULL, 2);
> > +static SENSOR_DEVICE_ATTR(pwm1_auto_channels_fan, S_IRUGO,
> > + show_pwm_auto_channels, NULL, 0);
> > +static SENSOR_DEVICE_ATTR(pwm2_auto_channels_fan, S_IRUGO,
> > + show_pwm_auto_channels, NULL, 1);
> > +static SENSOR_DEVICE_ATTR(pwm3_auto_channels_fan, S_IRUGO,
> > + show_pwm_auto_channels, NULL, 2);
>
> Not sure what others think about this (Mark? Juerg?) but this (pwm1 to
> fan1 and to temp1, etc.) seems to be the usual mapping for chips where
> the mappings can't be changed, so I wonder what's the benefit of
> creating read-only sysfs files to express it. I think I'd be just as
> happy with no channel files at all (and it's cheaper.) No strong
> opinion though.
I don't find them particularly useful. The static mapping should be
documented in the chip doc.
...juerg
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [lm-sensors] [PATCH] Add temperature-tracking mode to f71805f
2007-06-19 20:38 [lm-sensors] [PATCH] Add temperature-tracking mode to f71805f Phil Endecott
` (4 preceding siblings ...)
2007-06-26 22:54 ` Juerg Haefliger
@ 2007-06-27 19:05 ` Jean Delvare
2007-06-27 20:32 ` Phil Endecott
2007-06-29 7:17 ` Jean Delvare
7 siblings, 0 replies; 9+ messages in thread
From: Jean Delvare @ 2007-06-27 19:05 UTC (permalink / raw)
To: lm-sensors
Hi Juerg,
On Tue, 26 Jun 2007 15:54:55 -0700, Juerg Haefliger wrote:
> On 6/25/07, Jean Delvare <khali@linux-fr.org> wrote:
> > > @@ -931,6 +1029,58 @@
> > > show_pwm_freq, set_pwm_freq, 2);
> > > static SENSOR_DEVICE_ATTR(pwm3_mode, S_IRUGO, show_pwm_mode, NULL, 2);
> > >
> > > +static SENSOR_DEVICE_ATTR(pwm1_auto_channels_temp, S_IRUGO,
> > > + show_pwm_auto_channels, NULL, 0);
> > > +static SENSOR_DEVICE_ATTR(pwm2_auto_channels_temp, S_IRUGO,
> > > + show_pwm_auto_channels, NULL, 1);
> > > +static SENSOR_DEVICE_ATTR(pwm3_auto_channels_temp, S_IRUGO,
> > > + show_pwm_auto_channels, NULL, 2);
> > > +static SENSOR_DEVICE_ATTR(pwm1_auto_channels_fan, S_IRUGO,
> > > + show_pwm_auto_channels, NULL, 0);
> > > +static SENSOR_DEVICE_ATTR(pwm2_auto_channels_fan, S_IRUGO,
> > > + show_pwm_auto_channels, NULL, 1);
> > > +static SENSOR_DEVICE_ATTR(pwm3_auto_channels_fan, S_IRUGO,
> > > + show_pwm_auto_channels, NULL, 2);
> >
> > Not sure what others think about this (Mark? Juerg?) but this (pwm1 to
> > fan1 and to temp1, etc.) seems to be the usual mapping for chips where
> > the mappings can't be changed, so I wonder what's the benefit of
> > creating read-only sysfs files to express it. I think I'd be just as
> > happy with no channel files at all (and it's cheaper.) No strong
> > opinion though.
>
> I don't find them particularly useful.
So we agree. Good.
> The static mapping should be
> documented in the chip doc.
Yes, and they already are.
Phil, please exclude the channel sysfs files from the next iteration of
your patch.
Thanks,
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 9+ messages in thread* [lm-sensors] [PATCH] Add temperature-tracking mode to f71805f
2007-06-19 20:38 [lm-sensors] [PATCH] Add temperature-tracking mode to f71805f Phil Endecott
` (5 preceding siblings ...)
2007-06-27 19:05 ` Jean Delvare
@ 2007-06-27 20:32 ` Phil Endecott
2007-06-29 7:17 ` Jean Delvare
7 siblings, 0 replies; 9+ messages in thread
From: Phil Endecott @ 2007-06-27 20:32 UTC (permalink / raw)
To: lm-sensors
This third version of this patch:
- Uses strtol for temperatures since they can be negative.
- Fixes an indentation issue.
- Removes the auto_channel files.
- Removes the description of how this driver uses auto-point files from the sysfs documentation.
- Uses a sign-off email address which I hope will not offend anyone.
Requests for any further changes will need to reach me very quickly as my test
hardware will not be available for much longer.
This is a patch for the f71805f hwmon driver to add support for its
temperature-tracking mode.
Signed-off-by: Phil Endecott <kernel@chezphil.org>
--- linux-2.6-2.6.21/drivers/hwmon/f71805f.c.orig 2007-06-16 00:10:54.000000000 +0100
+++ linux-2.6-2.6.21/drivers/hwmon/f71805f.c 2007-06-27 20:20:00.000000000 +0100
@@ -126,6 +126,12 @@
#define F71805F_REG_TEMP_HIGH(nr) (0x54 + 2 * (nr))
#define F71805F_REG_TEMP_HYST(nr) (0x55 + 2 * (nr))
#define F71805F_REG_TEMP_MODE 0x01
+/* pwm/fan pwmnr from 0 to 2, auto point apnr from 0 to 2 */
+/* map fintek numbers to our numbers as follows: 9->0, 5->1, 1->2. */
+#define F71805F_REG_PWM_AUTO_POINT_TEMP(pwmnr, apnr) \
+ (0xA0 + 0x10 * (pwmnr) + (2-apnr))
+#define F71805F_REG_PWM_AUTO_POINT_FAN(pwmnr, apnr) \
+ (0xA4 + 0x10 * (pwmnr) + 2 * (2-apnr))
#define F71805F_REG_START 0x00
/* status nr from 0 to 2 */
@@ -143,6 +149,11 @@
* Data structures and manipulation thereof
*/
+struct f71805f_auto_point {
+ u8 temp[3];
+ u16 fan[3];
+};
+
struct f71805f_data {
unsigned short addr;
const char *name;
@@ -169,6 +180,7 @@
u8 temp_hyst[3];
u8 temp_mode;
unsigned long alarms;
+ struct f71805f_auto_point auto_points[3];
};
struct f71805f_sio_data {
@@ -311,7 +323,7 @@
static struct f71805f_data *f71805f_update_device(struct device *dev)
{
struct f71805f_data *data = dev_get_drvdata(dev);
- int nr;
+ int nr, apnr;
mutex_lock(&data->update_lock);
@@ -341,6 +353,18 @@
F71805F_REG_TEMP_HYST(nr));
}
data->temp_mode = f71805f_read8(data, F71805F_REG_TEMP_MODE);
+ for (nr = 0; nr < 3; nr++) {
+ for (apnr = 0; apnr < 3; apnr++) {
+ data->auto_points[nr].temp[apnr] + f71805f_read8(data,
+ F71805F_REG_PWM_AUTO_POINT_TEMP(nr,
+ apnr));
+ data->auto_points[nr].fan[apnr] + f71805f_read16(data,
+ F71805F_REG_PWM_AUTO_POINT_FAN(nr,
+ apnr));
+ }
+ }
data->last_limits = jiffies;
}
@@ -704,6 +728,70 @@
return count;
}
+static ssize_t show_pwm_auto_point_temp(struct device *dev,
+ struct device_attribute *devattr,
+ char* buf)
+{
+ struct f71805f_data *data = dev_get_drvdata(dev);
+ struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+ int pwmnr = attr->nr;
+ int apnr = attr->index;
+
+ return sprintf(buf, "%ld\n",
+ temp_from_reg(data->auto_points[pwmnr].temp[apnr]));
+}
+
+static ssize_t set_pwm_auto_point_temp(struct device *dev,
+ struct device_attribute *devattr,
+ const char* buf, size_t count)
+{
+ struct f71805f_data *data = dev_get_drvdata(dev);
+ struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+ int pwmnr = attr->nr;
+ int apnr = attr->index;
+ unsigned long val = simple_strtol(buf, NULL, 10);
+
+ mutex_lock(&data->update_lock);
+ data->auto_points[pwmnr].temp[apnr] = temp_to_reg(val);
+ f71805f_write8(data, F71805F_REG_PWM_AUTO_POINT_TEMP(pwmnr, apnr),
+ data->auto_points[pwmnr].temp[apnr]);
+ mutex_unlock(&data->update_lock);
+
+ return count;
+}
+
+static ssize_t show_pwm_auto_point_fan(struct device *dev,
+ struct device_attribute *devattr,
+ char* buf)
+{
+ struct f71805f_data *data = dev_get_drvdata(dev);
+ struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+ int pwmnr = attr->nr;
+ int apnr = attr->index;
+
+ return sprintf(buf, "%ld\n",
+ fan_from_reg(data->auto_points[pwmnr].fan[apnr]));
+}
+
+static ssize_t set_pwm_auto_point_fan(struct device *dev,
+ struct device_attribute *devattr,
+ const char* buf, size_t count)
+{
+ struct f71805f_data *data = dev_get_drvdata(dev);
+ struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
+ int pwmnr = attr->nr;
+ int apnr = attr->index;
+ unsigned long val = simple_strtoul(buf, NULL, 10);
+
+ mutex_lock(&data->update_lock);
+ data->auto_points[pwmnr].fan[apnr] = fan_to_reg(val);
+ f71805f_write16(data, F71805F_REG_PWM_AUTO_POINT_FAN(pwmnr, apnr),
+ data->auto_points[pwmnr].fan[apnr]);
+ mutex_unlock(&data->update_lock);
+
+ return count;
+}
+
static ssize_t show_temp(struct device *dev, struct device_attribute *devattr,
char *buf)
{
@@ -931,6 +1019,45 @@
show_pwm_freq, set_pwm_freq, 2);
static SENSOR_DEVICE_ATTR(pwm3_mode, S_IRUGO, show_pwm_mode, NULL, 2);
+static SENSOR_DEVICE_ATTR_2(pwm1_auto_point1_temp, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_temp, set_pwm_auto_point_temp, 0, 0);
+static SENSOR_DEVICE_ATTR_2(pwm1_auto_point1_fan, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_fan, set_pwm_auto_point_fan, 0, 0);
+static SENSOR_DEVICE_ATTR_2(pwm1_auto_point2_temp, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_temp, set_pwm_auto_point_temp, 0, 1);
+static SENSOR_DEVICE_ATTR_2(pwm1_auto_point2_fan, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_fan, set_pwm_auto_point_fan, 0, 1);
+static SENSOR_DEVICE_ATTR_2(pwm1_auto_point3_temp, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_temp, set_pwm_auto_point_temp, 0, 2);
+static SENSOR_DEVICE_ATTR_2(pwm1_auto_point3_fan, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_fan, set_pwm_auto_point_fan, 0, 2);
+
+static SENSOR_DEVICE_ATTR_2(pwm2_auto_point1_temp, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_temp, set_pwm_auto_point_temp, 1, 0);
+static SENSOR_DEVICE_ATTR_2(pwm2_auto_point1_fan, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_fan, set_pwm_auto_point_fan, 1, 0);
+static SENSOR_DEVICE_ATTR_2(pwm2_auto_point2_temp, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_temp, set_pwm_auto_point_temp, 1, 1);
+static SENSOR_DEVICE_ATTR_2(pwm2_auto_point2_fan, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_fan, set_pwm_auto_point_fan, 1, 1);
+static SENSOR_DEVICE_ATTR_2(pwm2_auto_point3_temp, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_temp, set_pwm_auto_point_temp, 1, 2);
+static SENSOR_DEVICE_ATTR_2(pwm2_auto_point3_fan, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_fan, set_pwm_auto_point_fan, 1, 2);
+
+static SENSOR_DEVICE_ATTR_2(pwm3_auto_point1_temp, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_temp, set_pwm_auto_point_temp, 2, 0);
+static SENSOR_DEVICE_ATTR_2(pwm3_auto_point1_fan, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_fan, set_pwm_auto_point_fan, 2, 0);
+static SENSOR_DEVICE_ATTR_2(pwm3_auto_point2_temp, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_temp, set_pwm_auto_point_temp, 2, 1);
+static SENSOR_DEVICE_ATTR_2(pwm3_auto_point2_fan, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_fan, set_pwm_auto_point_fan, 2, 1);
+static SENSOR_DEVICE_ATTR_2(pwm3_auto_point3_temp, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_temp, set_pwm_auto_point_temp, 2, 2);
+static SENSOR_DEVICE_ATTR_2(pwm3_auto_point3_fan, S_IRUGO | S_IWUSR,
+ show_pwm_auto_point_fan, set_pwm_auto_point_fan, 2, 2);
+
static SENSOR_DEVICE_ATTR(in0_alarm, S_IRUGO, show_alarm, NULL, 0);
static SENSOR_DEVICE_ATTR(in1_alarm, S_IRUGO, show_alarm, NULL, 1);
static SENSOR_DEVICE_ATTR(in2_alarm, S_IRUGO, show_alarm, NULL, 2);
@@ -1013,6 +1140,25 @@
&sensor_dev_attr_temp3_max_hyst.dev_attr.attr,
&sensor_dev_attr_temp3_type.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point1_fan.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point2_temp.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point2_fan.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point3_temp.dev_attr.attr,
+ &sensor_dev_attr_pwm1_auto_point3_fan.dev_attr.attr,
+ &sensor_dev_attr_pwm2_auto_point1_temp.dev_attr.attr,
+ &sensor_dev_attr_pwm2_auto_point1_fan.dev_attr.attr,
+ &sensor_dev_attr_pwm2_auto_point2_temp.dev_attr.attr,
+ &sensor_dev_attr_pwm2_auto_point2_fan.dev_attr.attr,
+ &sensor_dev_attr_pwm2_auto_point3_temp.dev_attr.attr,
+ &sensor_dev_attr_pwm2_auto_point3_fan.dev_attr.attr,
+ &sensor_dev_attr_pwm3_auto_point1_temp.dev_attr.attr,
+ &sensor_dev_attr_pwm3_auto_point1_fan.dev_attr.attr,
+ &sensor_dev_attr_pwm3_auto_point2_temp.dev_attr.attr,
+ &sensor_dev_attr_pwm3_auto_point2_fan.dev_attr.attr,
+ &sensor_dev_attr_pwm3_auto_point3_temp.dev_attr.attr,
+ &sensor_dev_attr_pwm3_auto_point3_fan.dev_attr.attr,
+
&sensor_dev_attr_in0_alarm.dev_attr.attr,
&sensor_dev_attr_in1_alarm.dev_attr.attr,
&sensor_dev_attr_in2_alarm.dev_attr.attr,
--- linux-2.6-2.6.21/Documentation/hwmon/f71805f.orig 2007-06-16 22:10:33.000000000 +0100
+++ linux-2.6-2.6.21/Documentation/hwmon/f71805f 2007-06-26 23:11:46.000000000 +0100
@@ -5,11 +5,11 @@
* Fintek F71805F/FG
Prefix: 'f71805f'
Addresses scanned: none, address read from Super I/O config space
- Datasheet: Provided by Fintek on request
+ Datasheet: Available from the Fintek website
* Fintek F71872F/FG
Prefix: 'f71872f'
Addresses scanned: none, address read from Super I/O config space
- Datasheet: Provided by Fintek on request
+ Datasheet: Available from the Fintek website
Author: Jean Delvare <khali@linux-fr.org>
@@ -128,7 +128,9 @@
When the PWM method is used, you can select the operating frequency,
from 187.5 kHz (default) to 31 Hz. The best frequency depends on the
fan model. As a rule of thumb, lower frequencies seem to give better
-control, but may generate annoying high-pitch noise. Fintek recommends
+control, but may generate annoying high-pitch noise. So a frequency just
+above the audible range, such as 25 kHz, may be a good choice; if this
+doesn't give you good linear control, try reducing it. Fintek recommends
not going below 1 kHz, as the fan tachometers get confused by lower
frequencies as well.
@@ -136,16 +138,25 @@
corresponds to a pwm value of 106 for the driver. The driver doesn't
enforce this limit though.
-Three different fan control modes are supported:
+Three different fan control modes are supported; the mode number is written
+to the pwm<n>_enable file.
+
+* 1: Manual mode
+ You ask for a specific PWM duty cycle or DC voltage by writing to the
+ pwm<n> file.
+
+* 2: Temperature mode
+ You define 3 temperature/fan speed trip points using the
+ pwm<n>_auto_point<m>_temp and _fan files. These define a staircase
+ relationship between temperature and fan speed with two additional points
+ interpolated between the values that you define. When the temperature
+ is below auto_point1_temp the fan is switched off.
+
+* 3: Fan speed mode
+ You ask for a specific fan speed by writing to the fan<n>_target file.
+
+Both of the automatic modes require that pwm1 corresponds to fan1, pwm2 to
+fan2 and pwm3 to fan3. Temperature mode also requires that temp1 corresponds
+to pwm1 and fan1, etc.
-* Manual mode
- You ask for a specific PWM duty cycle or DC voltage.
-* Fan speed mode
- You ask for a specific fan speed. This mode assumes that pwm1
- corresponds to fan1, pwm2 to fan2 and pwm3 to fan3.
-
-* Temperature mode
- You define 3 temperature/fan speed trip points, and the fan speed is
- adjusted depending on the measured temperature, using interpolation.
- This mode is not yet supported by the driver.
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [lm-sensors] [PATCH] Add temperature-tracking mode to f71805f
2007-06-19 20:38 [lm-sensors] [PATCH] Add temperature-tracking mode to f71805f Phil Endecott
` (6 preceding siblings ...)
2007-06-27 20:32 ` Phil Endecott
@ 2007-06-29 7:17 ` Jean Delvare
7 siblings, 0 replies; 9+ messages in thread
From: Jean Delvare @ 2007-06-29 7:17 UTC (permalink / raw)
To: lm-sensors
Hi Phil,
On Wed, 27 Jun 2007 21:32:26 +0100, Phil Endecott wrote:
> This third version of this patch:
> - Uses strtol for temperatures since they can be negative.
> - Fixes an indentation issue.
> - Removes the auto_channel files.
> - Removes the description of how this driver uses auto-point files from the sysfs documentation.
> - Uses a sign-off email address which I hope will not offend anyone.
>
> Requests for any further changes will need to reach me very quickly as my test
> hardware will not be available for much longer.
>
>
>
> This is a patch for the f71805f hwmon driver to add support for its
> temperature-tracking mode.
>
> Signed-off-by: Phil Endecott <kernel@chezphil.org>
This patch looks very good, thanks! I'll send it to Mark Hoffman for
upstream inclusion quickly.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 9+ messages in thread