On Sun, 28 Oct 2012 11:19:57 -0700, Guenter Roeck wrote:
> Also unify fan functions to use the same code for 8 and 16 bit fans
>
> This patch reduces code size by approximately 1,200 bytes on x86_64.
>
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> ---
> v2: Drop has_16bit_fans flag, since we'll deal with it in patch 8/8.
>
> drivers/hwmon/it87.c | 255 +++++++++++++++++++-------------------------------
> 1 file changed, 96 insertions(+), 159 deletions(-)
>
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index fe2cdd4..c6426c0 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -262,8 +262,7 @@ struct it87_data {
> u16 in_scaled; /* Internal voltage sensors are scaled */
> u8 in[9][3]; /* [nr][0]=in, [1]=min, [2]=max */
> u8 has_fan; /* Bitfield, fans enabled */
> - u16 fan[5]; /* Register values, possibly combined */
> - u16 fan_min[5]; /* Register values, possibly combined */
> + u16 fan[5][2]; /* Register values, [nr][0]=fan, [1]=min */
> u8 has_temp; /* Bitfield, temp sensors enabled */
> s8 temp[3][4]; /* [nr][0]=temp, [1]=min, [2]=max, [3]=offset */
> u8 sensor; /* Register value */
> @@ -641,25 +640,21 @@ static int pwm_mode(const struct it87_data *data, int nr)
> }
>
> static ssize_t show_fan(struct device *dev, struct device_attribute *attr,
> - char *buf)
> + char *buf)
> {
> - struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> - int nr = sensor_attr->index;
> -
> + struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> + int nr = sattr->nr;
> + int index = sattr->index;
> + int speed;
> struct it87_data *data = it87_update_device(dev);
> - return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[nr],
> - DIV_FROM_REG(data->fan_div[nr])));
> -}
> -static ssize_t show_fan_min(struct device *dev, struct device_attribute *attr,
> - char *buf)
> -{
> - struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> - int nr = sensor_attr->index;
>
> - struct it87_data *data = it87_update_device(dev);
> - return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_min[nr],
> - DIV_FROM_REG(data->fan_div[nr])));
> + speed = has_16bit_fans(data) ?
has_16bit_fans() is slow so I don't like it being called at run-time.
That's why we had different sets of sysfs callback functions
originally. The change above is only acceptable because I see you'll
fix the performance regression in patch 8/9. Ideally you should have
ordered them in the other direction, but don't bother swapping them if
it means more work for you.
> + FAN16_FROM_REG(data->fan[nr][index]) :
> + FAN_FROM_REG(data->fan[nr][index],
> + DIV_FROM_REG(data->fan_div[nr]));
> + return sprintf(buf, "%d\n", speed);
> }
> +
> static ssize_t show_fan_div(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> @@ -696,11 +691,13 @@ static ssize_t show_pwm_freq(struct device *dev, struct device_attribute *attr,
>
> return sprintf(buf, "%u\n", pwm_freq[index]);
> }
> -static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr,
> - const char *buf, size_t count)
> +
> +static ssize_t set_fan(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> {
> - struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> - int nr = sensor_attr->index;
> + struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> + int nr = sattr->nr;
> + int index = sattr->index;
>
> struct it87_data *data = dev_get_drvdata(dev);
> long val;
> @@ -710,24 +707,36 @@ static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr,
> return -EINVAL;
>
> mutex_lock(&data->update_lock);
> - reg = it87_read_value(data, IT87_REG_FAN_DIV);
> - switch (nr) {
> - case 0:
> - data->fan_div[nr] = reg & 0x07;
> - break;
> - case 1:
> - data->fan_div[nr] = (reg >> 3) & 0x07;
> - break;
> - case 2:
> - data->fan_div[nr] = (reg & 0x40) ? 3 : 1;
> - break;
> +
> + if (!has_16bit_fans(data)) {
You could swap the blocks and save a negation.
> + reg = it87_read_value(data, IT87_REG_FAN_DIV);
> + switch (nr) {
> + case 0:
> + data->fan_div[nr] = reg & 0x07;
> + break;
> + case 1:
> + data->fan_div[nr] = (reg >> 3) & 0x07;
> + break;
> + case 2:
> + data->fan_div[nr] = (reg & 0x40) ? 3 : 1;
> + break;
> + }
> + data->fan[nr][index] =
> + FAN_TO_REG(val, DIV_FROM_REG(data->fan_div[nr]));
> + it87_write_value(data, IT87_REG_FAN_MIN[nr],
> + data->fan[nr][index]);
> + } else {
> + data->fan[nr][index] = FAN16_TO_REG(val);
> + it87_write_value(data, IT87_REG_FAN_MIN[nr],
> + data->fan[nr][index] & 0xff);
> + it87_write_value(data, IT87_REG_FANX_MIN[nr],
> + data->fan[nr][index] >> 8);
> }
>
> - data->fan_min[nr] = FAN_TO_REG(val, DIV_FROM_REG(data->fan_div[nr]));
> - it87_write_value(data, IT87_REG_FAN_MIN[nr], data->fan_min[nr]);
> mutex_unlock(&data->update_lock);
> return count;
> }
Other than these details, very nice cleanup :)
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
