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

[Guenter Roeck <linux@roeck-us.net>]

On Mon, Oct 29, 2012 at 11:27:01AM +0100, Jean Delvare wrote:
> 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.
> 
I had originally introduced a flag named "has_16bit_fans" for that very purpose
(see v2 comments above), and removed it since its only practical impact was to
increase the size of patch 8. Easy to revert to v1 of the patch if you prefer.

> > +		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.
> 
Ok.

> > +		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