Hi Marc,
On Wed, 6 Aug 2008 10:19:46 +0200, Marc Hulsman wrote:
> Adds support to set target temperature and tolerance for thermal cruise mode.
>
> Signed-off-by: Marc Hulsman <m.hulsman at tudelft.nl>
>
> ---
> Documentation/hwmon/w83791d | 19 ++++-
> drivers/hwmon/w83791d.c | 150 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 164 insertions(+), 5 deletions(-)
I think you have a bug in your code. I also have minor stylistic
comments.
>
> ---
> Index: linux-2.6.27-rc1/drivers/hwmon/w83791d.c
> ===================================================================
> --- linux-2.6.27-rc1.orig/drivers/hwmon/w83791d.c
> +++ linux-2.6.27-rc1/drivers/hwmon/w83791d.c
> @@ -125,6 +125,17 @@ static const u8 W83791D_REG_PWM[NUMBER_O
> 0xA1, /* PWM 5 duty cycle register in DataSheet */
> };
>
> +static const u8 W83791D_REG_PWM_TARGET[3] = {
> + 0x85, /* PWM 1 target temperature for temp 1 */
> + 0x86, /* PWM 2 target temperature for temp 2*/
> + 0x96, /* PWM 3 target temperature for temp 3*/
Missing space at end of comments.
> +};
> +
> +static const u8 W83791D_REG_PWM_TOL[2] = {
> + 0x87, /* PWM 1/2 temperature tolerance */
> + 0x97, /* PWM 3 temperature tolerance */
> +};
> +
> static const u8 W83791D_REG_FAN_CFG[2] = {
> 0x84, /* FAN 1/2 configuration */
> 0x95, /* FAN 3 configuration */
> @@ -234,6 +245,17 @@ static u8 fan_to_reg(long rpm, int div)
> (val) < 0 ? ((val) - 250) / 500 * 128 : \
> ((val) + 250) / 500 * 128)
>
> +/* for thermal cruise target temp, 7-bits, LSB = 1 degree Celsius */
> +#define TARGET_TEMP_FROM_REG(val) ((val) * 1000)
This is the same as TEMP_FROM_REG so you could just use that.
> +#define TARGET_TEMP_TO_REG(val) ((val) < 0 ? 0 : \
> + (val) >= 127000 ? 127 : \
> + ((val) + 500) / 1000)
> +
> +/* for thermal cruise temp tolerance, 4-bits, LSB = 1 degree Celsius */
> +#define TOL_TEMP_FROM_REG(val) ((val) * 1000)
Same here.
> +#define TOL_TEMP_TO_REG(val) ((val) < 0 ? 0 : \
> + (val) >= 15000 ? 15 : \
> + ((val) + 500) / 1000)
Note: if you want to spend some more time on the w83791d driver, one
thing that would be welcome is converting all these macros to inline
functions.
>
> #define BEEP_MASK_TO_REG(val) ((val) & 0xffffff)
> #define BEEP_MASK_FROM_REG(val) ((val) & 0xffffff)
> @@ -290,6 +312,9 @@ struct w83791d_data {
> u8 pwm_enable[3]; /* pwm enable status for fan 1-3
> (fan 4-5 only support manual mode) */
>
> + u8 pwm_target[3]; /* pwm 1-3 target temperature */
Doubled space.
> + u8 pwm_tolerance[3]; /* pwm 1-3 temperature tolerance */
> +
> /* Misc */
> u32 alarms; /* realtime status register encoding,combined */
> u8 beep_enable; /* Global beep enable */
> @@ -774,6 +799,110 @@ static struct sensor_device_attribute sd
> show_pwmenable, store_pwmenable, 2),
> };
>
> +/* For Smart Fan I / Thermal Cruise */
> +static ssize_t show_pwm_target(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> + struct w83791d_data *data = w83791d_update_device(dev);
> + int nr = sensor_attr->index;
> + return sprintf(buf, "%d\n", TARGET_TEMP_FROM_REG(data->pwm_target[nr]));
> +}
> +
> +static ssize_t store_pwm_target(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> + struct i2c_client *client = to_i2c_client(dev);
> + struct w83791d_data *data = i2c_get_clientdata(client);
> + int nr = sensor_attr->index;
> + unsigned long val;
> + u8 target_mask;
> +
> + if (strict_strtoul(buf, 10, &val))
> + return -EINVAL;
> +
> + mutex_lock(&data->update_lock);
> + data->pwm_target[nr] = TARGET_TEMP_TO_REG(val);
> + target_mask = w83791d_read(client,
> + W83791D_REG_PWM_TARGET[nr]) & 0x80;
> + w83791d_write(client, W83791D_REG_PWM_TARGET[nr],
> + data->pwm_target[nr] | target_mask);
> + mutex_unlock(&data->update_lock);
> + return count;
> +}
> +
> +static struct sensor_device_attribute sda_pwm_target[] = {
> + SENSOR_ATTR(pwm1_target, S_IWUSR | S_IRUGO,
> + show_pwm_target, store_pwm_target, 0),
> + SENSOR_ATTR(pwm2_target, S_IWUSR | S_IRUGO,
> + show_pwm_target, store_pwm_target, 1),
> + SENSOR_ATTR(pwm3_target, S_IWUSR | S_IRUGO,
> + show_pwm_target, store_pwm_target, 2),
> +};
> +
> +static ssize_t show_pwm_tolerance(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> + struct w83791d_data *data = w83791d_update_device(dev);
> + int nr = sensor_attr->index;
> + return sprintf(buf, "%d\n", TOL_TEMP_FROM_REG(data->pwm_tolerance[nr]));
> +}
> +
> +static ssize_t store_pwm_tolerance(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> + struct i2c_client *client = to_i2c_client(dev);
> + struct w83791d_data *data = i2c_get_clientdata(client);
> + int nr = sensor_attr->index;
> + unsigned long val;
> + u8 target_mask;
> + u8 reg_idx = 0;
> + u8 val_shift = 0;
> + u8 keep_mask = 0;
> +
> + if (strict_strtoul(buf, 10, &val))
> + return -EINVAL;
> +
> + switch (nr) {
> + case 0:
> + reg_idx = 0;
> + val_shift = 0;
> + keep_mask = 0xf0;
> + break;
> + case 1:
> + reg_idx = 0;
> + val_shift = 4;
> + keep_mask = 0x0f;
> + break;
> + case 2:
> + reg_idx = 1;
> + val_shift = 0;
> + keep_mask = 0xf0;
> + break;
> + }
> +
> + mutex_lock(&data->update_lock);
> + data->pwm_tolerance[nr] = TOL_TEMP_TO_REG(val);
> + target_mask = w83791d_read(client,
> + W83791D_REG_PWM_TOL[reg_idx]) & keep_mask;
> + w83791d_write(client, W83791D_REG_PWM_TOL[reg_idx],
> + (data->pwm_tolerance[nr] << val_shift) | target_mask);
> + mutex_unlock(&data->update_lock);
> + return count;
> +}
> +
> +static struct sensor_device_attribute sda_pwm_tolerance[] = {
> + SENSOR_ATTR(pwm1_tolerance, S_IWUSR | S_IRUGO,
> + show_pwm_tolerance, store_pwm_tolerance, 0),
> + SENSOR_ATTR(pwm2_tolerance, S_IWUSR | S_IRUGO,
> + show_pwm_tolerance, store_pwm_tolerance, 1),
> + SENSOR_ATTR(pwm3_tolerance, S_IWUSR | S_IRUGO,
> + show_pwm_tolerance, store_pwm_tolerance, 2),
> +};
> +
> /* read/write the temperature1, includes measured value and limits */
> static ssize_t show_temp1(struct device *dev, struct device_attribute
> *devattr,
> char *buf)
> @@ -1044,6 +1173,12 @@ static struct attribute *w83791d_attribu
> &sda_pwmenable[0].dev_attr.attr,
> &sda_pwmenable[1].dev_attr.attr,
> &sda_pwmenable[2].dev_attr.attr,
> + &sda_pwm_target[0].dev_attr.attr,
> + &sda_pwm_target[1].dev_attr.attr,
> + &sda_pwm_target[2].dev_attr.attr,
> + &sda_pwm_tolerance[0].dev_attr.attr,
> + &sda_pwm_tolerance[1].dev_attr.attr,
> + &sda_pwm_tolerance[2].dev_attr.attr,
> NULL
> };
>
> @@ -1403,6 +1538,21 @@ static struct w83791d_data *w83791d_upda
> data->pwm_enable[1] = ((reg_array_tmp[0] >> 4) & 0x03);
> data->pwm_enable[2] = ((reg_array_tmp[1] >> 2) & 0x03);
>
> + /* Update PWM target temperature */
> + for (i = 0; i < 2; i++) {
I think you have a bug here: this should be i < 3...
> + data->pwm_target[i] = w83791d_read(client,
> + W83791D_REG_PWM_TARGET[i]) & 0x7f;
> + }
> +
> + /* Update PWM temperature tolerance */
> + for (i = 0; i < 1; i++) {
and i < 2 here.
> + reg_array_tmp[i] = w83791d_read(client,
> + W83791D_REG_PWM_TOL[i]);
> + }
> + data->pwm_tolerance[0] = reg_array_tmp[0] & 0x0f;
> + data->pwm_tolerance[1] = (reg_array_tmp[0] >> 4) & 0x0f;
> + data->pwm_tolerance[2] = reg_array_tmp[1] & 0x0f;
> +
> /* Update the first temperature sensor */
> for (i = 0; i < 3; i++) {
> data->temp1[i] = w83791d_read(client,
> Index: linux-2.6.27-rc1/Documentation/hwmon/w83791d
> ===================================================================
> --- linux-2.6.27-rc1.orig/Documentation/hwmon/w83791d
> +++ linux-2.6.27-rc1/Documentation/hwmon/w83791d
> @@ -77,6 +77,9 @@ readings can be divided by a programmabl
>
> Each fan controlled is controlled by PWM. The PWM duty cycle can be read and
> set for each fan separately. Valid values range from 0 (stop) to 255 (full).
> +PWM 1-3 support Thermal Cruise mode, in which the PWMs are automatically
> +regulated to keep respectively temp 1-3 at a certain target temperature.
> +See below for the description of the sysfs-interface.
>
> The w83791d has a global bit used to enable beeping from the speaker when an
> alarm is triggered as well as a bitmask to enable or disable the beep for
> @@ -116,9 +119,19 @@ chip-specific options are documented her
> pwm[1-3]_enable - this file controls mode of fan/temperature control for
> fan 1-3. Fan/PWM 4-5 only support manual mode.
> * 1 Manual mode
> - * 2 Thermal Cruise mode (no further support)
> + * 2 Thermal Cruise mode
> * 3 Fan Speed Cruise mode (no further support)
>
> +pwm[1-3]_target - defines the target temperature for Thermal Cruise mode.
> + Unit: millidegree Celsius
> + RW
> +
> +pwm[1-3]_tolerance - temperature tolerance for Thermal Cruise mode.
> + Specifies an interval around the target temperature
> + in which the fan speed is not changed.
> + Unit: millidegree Celsius
> + RW
I am not necessarily very happy with the file names you came up with.
pwm[1-3]_target is a bit ambiguous. Is it a temperature target or a fan
speed target? If you were to implement support for the Fan Speed Cruise
mode, how would you name the file?
I did implement something like the Fan Speed Cruise mode in the f71085f
driver. I named the files fan[1-3]_target, to make it clear what the
target was. This is even part of Documentation/hwmon/sysfs-interface by
now, and a second driver is implementing it that way (max6650).
Following the same logic, the Thermal Cruise mode files should be named
temp[1-3]_target (and presumably temp[1-3]_tolerance for the tolerance).
Unfortunately it seems that you got your file names from the w83627ehf
driver, so we already have one driver doing things that way (in fact 2:
the w83l786ng driver is doing the same.) To make things even worse, the
w83792d and w83793 drivers have a 3rd set of file names
(thermal_cruise[1-3] and tolerance[1-3]). It would be nice if we could
settle for one naming, document that in file sysfs-interface, and use
that for all devices which implement Thermal Cruise mode.
Hans, what do you think?
> +
> Alarms bitmap vs. beep_mask bitmask
> ------------------------------------
> For legacy code using the alarms and beep_mask files:
> @@ -146,7 +159,3 @@ tart2 : alarms: 0x020000 beep_ma
> tart3 : alarms: 0x040000 beep_mask: 0x100000 <== mismatch
> case_open : alarms: 0x001000 beep_mask: 0x001000
> global_enable: alarms: -------- beep_mask: 0x800000 (modified via beep_enable)
> -
> -W83791D TODO:
> ----------------
> -Provide a patch for Thermal Cruise registers.
Please provide an update for this patch, at least fixing the bugs I
found.
Thanks,
--
Jean Delvare
