Re: [lm-sensors] [PATCH v2 5/9] hwmon: (it87) Save fan registers in 2-dimensional array

[Jean Delvare <khali@linux-fr.org>]

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@roeck-us.net>
> ---
> 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]ún, [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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors