Re: [lm-sensors] hwmon/w83627hf pwm_freq support

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

Hi Carlos,

On Sat, 26 May 2007 14:55:19 +0200 (CEST), Carlos Olalla Martínez wrote:

> diff -uprN -X linux-2.6.22-rc1/Documentation/dontdiff linux-2.6.22-rc1.orig/drivers/hwmon/w83627hf.c linux-2.6.22-rc1/drivers/hwmon/w83627hf.c
> --- linux-2.6.22-rc1.orig/drivers/hwmon/w83627hf.c	2007-05-13 03:45:56.000000000 +0200
> +++ linux-2.6.22-rc1/drivers/hwmon/w83627hf.c	2007-05-26 14:45:05.000000000 +0200
> @@ -220,6 +220,18 @@ static const u8 regpwm[] = { W83627THF_R
>  #define W836X7HF_REG_PWM(type, nr) (((type) = w83627hf) ? \
>                                       regpwm_627hf[(nr) - 1] : regpwm[(nr) - 1])
>  
> +#define W83627HF_REG_PWM_FREQ		0x5C	/* Only for the 627HF */
> +
> +#define W83637HF_REG_PWM_FREQ1		0x00	/* 697HF/687THF too */
> +#define W83637HF_REG_PWM_FREQ2		0x02	/* 697HF/687THF too */
> +#define W83637HF_REG_PWM_FREQ3		0x10	/* 687THF too */
> +
> +static const u8 W83637HF_REG_PWM_FREQ[] = { W83637HF_REG_PWM_FREQ1, 
> +					W83637HF_REG_PWM_FREQ2, 
> +					W83637HF_REG_PWM_FREQ1 };

This should obviously be W83637HF_REG_PWM_FREQ3.

> +
> +#define W83627HF_BASE_PWM_FREQ	46870
> +
>  #define W83781D_REG_I2C_ADDR 0x48
>  #define W83781D_REG_I2C_SUBADDR 0x4A
>  
> @@ -267,6 +279,47 @@ static int TEMP_FROM_REG(u8 reg)
>  
>  #define PWM_TO_REG(val) (SENSORS_LIMIT((val),0,255))
>  
> +static inline unsigned long pwm_freq_from_reg_627hf(u8 reg)
> +{
> +	unsigned long freq;
> +	freq = W83627HF_BASE_PWM_FREQ >> reg;
> +	return freq;
> +}
> +static inline u8 pwm_freq_to_reg_627hf(unsigned long val)
> +{
> +	u8 i;
> +	/* Only 5 dividers (1 2 4 8 16)... Search for the nearest available frequency */
> +	for (i = 0; i < 5; i++) {
> +		if (val > (((W83627HF_BASE_PWM_FREQ >> i) + (W83627HF_BASE_PWM_FREQ >> (i+1))) / 2)) 
> +			break;
> +	}
> +	return i;
> +}

This could return with i = 5, which isn't correct.

Also, line length is limited to 80, please fold the lines that are
longer than that. This applies to the rest of your patch too.

> +
> +static inline unsigned long pwm_freq_from_reg(u8 reg)
> +{
> +	/* Clock bit 8 -> 180 kHz or 24 MHz */
> +	unsigned long clock = (reg & 0x80) ? 180000UL : 24000000UL;
> +
> +	reg &= 0x7f;
> +	/* This should not happen but anyway... */
> +	if (reg = 0)
> +		reg++;
> +	return (clock / (reg << 8));
> +}
> +static inline u8 pwm_freq_to_reg(unsigned long val)
> +{
> +	/* Minimum divider value is 0x01 and maximum is 0x7F */
> +	if (val >= 93750)	/* The highest we can do */
> +		return 0x01;
> +	if (val >= 720)	/* Use 24 MHz clock */
> +		return (24000000UL / (val << 8));
> +	if (val < 6)		/* The lowest we can do */
> +		return 0xFF;
> +	else			/* Use 180 kHz clock */
> +		return (0x80 | (180000UL / (val << 8)));
> +}
> +
>  #define BEEP_MASK_FROM_REG(val)		 (val)
>  #define BEEP_MASK_TO_REG(val)		((val) & 0xffffff)
>  #define BEEP_ENABLE_TO_REG(val)		((val)?1:0)
> @@ -316,6 +369,7 @@ struct w83627hf_data {
>  	u32 beep_mask;		/* Register encoding, combined */
>  	u8 beep_enable;		/* Boolean */
>  	u8 pwm[3];		/* Register value */
> +	u8 pwm_freq[3];		/* Register value */ 
>  	u16 sens[3];		/* 782D/783S only.
>  				   1 = pentium diode; 2 = 3904 diode;
>  				   3000-5000 = thermistor beta.
> @@ -852,6 +906,59 @@ sysfs_pwm(2);
>  sysfs_pwm(3);
>  
>  static ssize_t
> +show_pwm_freq_reg(struct device *dev, char *buf, int nr)
> +{
> +	struct w83627hf_data *data = w83627hf_update_device(dev);
> +	if (data->type = w83627hf) 
> +		return sprintf(buf, "%ld\n", pwm_freq_from_reg_627hf(data->pwm_freq[nr - 1]));
> +	else
> +		return sprintf(buf, "%ld\n", pwm_freq_from_reg(data->pwm_freq[nr - 1]));
> +}
> +
> +static ssize_t
> +store_pwm_freq_reg(struct device *dev, const char *buf, size_t count, int nr)
> +{
> +	struct w83627hf_data *data = dev_get_drvdata(dev);
> +	static const u8 mask[]={0xF8, 0x8F};
> +	u32 val;
> +
> +	val = simple_strtoul(buf, NULL, 10);
> +
> +	mutex_lock(&data->update_lock);
> +
> +	if (data->type = w83627hf) {
> +		data->pwm_freq[nr - 1] = pwm_freq_to_reg_627hf(val);
> +		w83627hf_write_value(data, W83627HF_REG_PWM_FREQ, 
> +					(data->pwm_freq[nr - 1] << ((nr - 1)*4)) | 
> +					(w83627hf_read_value(data, W83627HF_REG_PWM_FREQ) & mask[nr - 1]));
> +	} else {
> +		data->pwm_freq[nr - 1] = pwm_freq_to_reg(val);
> +		w83627hf_write_value(data, W83637HF_REG_PWM_FREQ[nr - 1],
> +					data->pwm_freq[nr - 1]);
> +	}
> +
> +	mutex_unlock(&data->update_lock);
> +	return count;
> +}
> +
> +#define sysfs_pwm_freq(offset) \
> +static ssize_t show_regs_pwm_freq_##offset (struct device *dev, struct device_attribute *attr, char *buf) \
> +{ \
> +	return show_pwm_freq_reg(dev, buf, offset); \
> +} \
> +static ssize_t \
> +store_regs_pwm_freq_##offset (struct device *dev, struct device_attribute *attr, const char *buf, size_t count) \
> +{ \
> +	return store_pwm_freq_reg(dev, buf, count, offset); \
> +} \
> +static DEVICE_ATTR(pwm##offset##_freq, S_IRUGO | S_IWUSR, \
> +		  show_regs_pwm_freq_##offset, store_regs_pwm_freq_##offset);
> +
> +sysfs_pwm_freq(1);
> +sysfs_pwm_freq(2);
> +sysfs_pwm_freq(3);
> +
> +static ssize_t
>  show_sensor_reg(struct device *dev, char *buf, int nr)
>  {
>  	struct w83627hf_data *data = w83627hf_update_device(dev);
> @@ -1075,6 +1182,9 @@ static struct attribute *w83627hf_attrib
>  
>  	&dev_attr_pwm3.attr,
>  
> +	&dev_attr_pwm1_freq.attr,
> +	&dev_attr_pwm2_freq.attr,
> +	&dev_attr_pwm3_freq.attr,
>  	NULL
>  };
>  
> @@ -1137,7 +1247,9 @@ static int __devinit w83627hf_probe(stru
>  		 || (err = device_create_file(dev, &dev_attr_in5_max))
>  		 || (err = device_create_file(dev, &dev_attr_in6_input))
>  		 || (err = device_create_file(dev, &dev_attr_in6_min))
> -		 || (err = device_create_file(dev, &dev_attr_in6_max)))
> +		 || (err = device_create_file(dev, &dev_attr_in6_max))
> +		 || (err = device_create_file(dev, &dev_attr_pwm1_freq))
> +		 || (err = device_create_file(dev, &dev_attr_pwm2_freq)))
>  			goto ERROR4;
>  
>  	if (data->type != w83697hf)
> @@ -1167,6 +1279,12 @@ static int __devinit w83627hf_probe(stru
>  		if ((err = device_create_file(dev, &dev_attr_pwm3)))
>  			goto ERROR4;
>  
> +	if (data->type = w83637hf || data->type = w83687thf)
> +		if ((err = device_create_file(dev, &dev_attr_pwm1_freq))
> +		 || (err = device_create_file(dev, &dev_attr_pwm2_freq))
> +		 || (err = device_create_file(dev, &dev_attr_pwm3_freq)))
> +			goto ERROR4;
> +
>  	data->class_dev = hwmon_device_register(dev);
>  	if (IS_ERR(data->class_dev)) {
>  		err = PTR_ERR(data->class_dev);
> @@ -1470,6 +1588,22 @@ static struct w83627hf_data *w83627hf_up
>  			   (data->type = w83627hf || data->type = w83697hf))
>  				break;
>  		}
> +		if (data->type != w83627thf) {
> +			if (data->type = w83627hf) {
> +					u8 tmp = w83627hf_read_value(data, 
> +							W83627HF_REG_PWM_FREQ);
> +					data->pwm_freq[0] = tmp & 0x07;
> +					data->pwm_freq[1] = (tmp >> 4) & 0x07;
> +				}
> +			else {
> +				for (i = 1; i <= 3; i++) {
> +					data->pwm_freq[i - 1] = w83627hf_read_value(data,
> +								 W83637HF_REG_PWM_FREQ[i - 1]);
> +					if(i = 2 && (data->type = w83697hf))
> +						break;
> +				}
> +			}
> +		}

The type tests can be optimized:

		if (data->type = w83627hf) {
			(...)
		} else if (data->type != w83627thf) {
			(...)
		}


>  
>  		data->temp = w83627hf_read_value(data, W83781D_REG_TEMP(1));
>  		data->temp_max 
Other than that, your patch looks good to me, thanks. Please provide an
updated version addressing the few remaining issues and I'll apply it.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors