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
