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=2dbc514a2ed2b6f71eb6d18671d2c663160788c9
> 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
