Skip to contentSkip to conversation

Resubmit w83627hf.c pwm clock selection

PATCH New Rudolf Marek 1 participant 1 message Aug 20, 2006

lore
Share & tools
lore

Rudolf Marek

author
#0 lore raw

PATCH 1/1 Resubmit w83627hf.c pwm clock selection

Hi Carlos,

Sorry for the delay, some of us had holiday and some of us had too much of other work. We really like that someone has started to work on this.

Quoted 2 lines

Sorry, I didnt follow the STANDARDS last time I send this patch. As you can see, its for the 2.6.17.3 kernel driver w83627hf.c

This time better. Let me check the rest.

I'm afraid we cannot accept the raw register writings. What we propose is:
All units should be in Hz

the sysfs name should be pwm1_freq, pwm2_freq

So PWM frequency = (Input Clock / Pre-scale) / 256 (it seems that 256 has fixed divisor? - this seems wrong)

So for the w83697hf allow anything from 180Khz/127 to 180KHz/1 and from 24Mhz/127 to 24Mhz/1 (in Hz unit)

For w83627hf just allow this:
<2:0> = 000: 46.87KHz
<2:0> = 001: 23.43KHz (Default)
<2:0> = 010: 11.72KHz
<2:0> = 011: 5.85KHz
<2:0> = 100: 2.93KHz

If the value is in between or bigger/smaller then default to nearest defined frequency.

We will need update to the documentation file too (as the separate patch).

Similar will apply to other chips when implemented.

Some very brief review will follow, so you know what else needs to be fixed.

Quoted 14 lines

From: Carlos Olalla <com.ea at tinet.org>

filename=hwmon-w83627hf-add-pwm-support.patch

This patch adds support for pwm clock selection and pwm clock scaling for the w83627hf and the w83697hf hardware monitors, both supported by this driver.

Signed-off-by Carlos Olalla <com.ea at tinet.org>

---

drivers/hwmon/w83627hf.c static const u8 regpwm[] = { W83627THF_R · line 222

Quoted 4 lines
drivers/hwmon/w83627hf.c renamed from drivers/hwmon/w83627hf.c.orig +1 -0
@@ -219,6 +219,15 @@ static const u8 regpwm[] = { W83627THF_R
                              W83627THF_REG_PWM3 };
 #define W836X7HF_REG_PWM(type, nr) (((type) = w83627hf) ? \
                                      regpwm_627hf[(nr) - 1] : regpwm[(nr) - 1])
+

Extra tabs the line should be empty

drivers/hwmon/w83627hf.c static const u8 regpwm[] = { W83627THF_R · line 225

Quoted 3 lines
drivers/hwmon/w83627hf.c renamed from drivers/hwmon/w83627hf.c.orig +3 -0
+#define W83697HF_REG_PWMCLK1	0x00	/* Only for the 697HF */
+#define W83697HF_REG_PWMCLK2	0x02	/* Only for the 697HF */
+

Extra tabs the line should be empty

drivers/hwmon/w83627hf.c static const u8 regpwm[] = { W83627THF_R · line 226

Quoted 1 line
drivers/hwmon/w83627hf.c renamed from drivers/hwmon/w83627hf.c.orig +1 -0
+static const u8 reg_pwmclk_697hf[] = {W83697HF_REG_PWMCLK1, W83697HF_REG_PWMCLK2};

perhaps a space = { W83... CLK2 };

drivers/hwmon/w83627hf.c static const u8 regpwm[] = { W83627THF_R · line 227

Quoted 1 line
drivers/hwmon/w83627hf.c renamed from drivers/hwmon/w83627hf.c.orig +1 -0
+

extra tabs and spaces

drivers/hwmon/w83627hf.c static const u8 regpwm[] = { W83627THF_R · line 230

Quoted 3 lines
drivers/hwmon/w83627hf.c renamed from drivers/hwmon/w83627hf.c.orig +3 -0
+#define W83627HF_REG_PWMCLK12	0x5C	/* Only for the 627HF */
+
+// To do : w83627thf, w83637hf and w83687thf pwmclk support

Sorry // comments are not allowed (see CodingStyle document in Documentation/)

Are you sure that other chips has different

Quoted 32 lines
drivers/hwmon/w83627hf.c renamed from drivers/hwmon/w83627hf.c.orig +19 -0
 #define W83781D_REG_I2C_ADDR 0x48
 #define W83781D_REG_I2C_SUBADDR 0x4A

@@ -319,6 +328,7 @@ struct w83627hf_data {
 	u32 beep_mask;		/* Register encoding, combined */
 	u8 beep_enable;		/* Boolean */
 	u8 pwm[3];		/* Register value */
+	u8 pwmclk[3];		/* Register value */
 	u16 sens[3];		/* 782D/783S only.
 				   1 = pentium diode; 2 = 3904 diode;
 				   3000-5000 = thermistor beta.

@@ -901,6 +911,68 @@ device_create_file(&client->dev, &dev_at
 } while (0)
 
 static ssize_t
+show_pwmclk_reg(struct device *dev, char *buf, int nr)
+{
+	struct w83627hf_data *data = w83627hf_update_device(dev);
+	return sprintf(buf, "%ld\n", (long) data->pwmclk[nr - 1]);
+}
+
+static ssize_t
+store_pwmclk_reg(struct device *dev, const char *buf, size_t count, int nr)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct w83627hf_data *data = i2c_get_clientdata(client);
+	u32 val;
+
+	val = simple_strtoul(buf, NULL, 10);
+
+	mutex_lock(&data->update_lock);
+
+	if (data->type = w83627hf) {

drivers/hwmon/w83627hf.c device_create_file(&client->dev, &dev_at · line 941

Quoted 10 lines
drivers/hwmon/w83627hf.c renamed from drivers/hwmon/w83627hf.c.orig +10 -0
+		/* bits 7 and 3 are reserved */
+		data->pwmclk[nr - 1] = PWM_TO_REG(val) & 0x77;
+		/* bits 6..4 pwmclk2 .. bits 2..0 pwmclk1 */
+		w83627hf_write_value(client,
+				     W83627HF_REG_PWMCLK12,
+				     data->pwmclk[nr - 1] |
+				     (w83627hf_read_value(client,
+				     W83627HF_REG_PWMCLK12) & 0x88)); 
+	} else  if (data->type = w83697hf) {
+		data->pwmclk[nr - 1] = PWM_TO_REG(val);

Here would be 0 allowed and I think 0 should not be allowed.

drivers/hwmon/w83627hf.c device_create_file(&client->dev, &dev_at · line 947

Quoted 6 lines
drivers/hwmon/w83627hf.c renamed from drivers/hwmon/w83627hf.c.orig +6 -0
+		w83627hf_write_value(client,
+				     reg_pwmclk_697hf[nr - 1],
+				     data->pwmclk[nr - 1]);
+	} else  {
+		/* TODO w83627thf, w83637hf and w83687thf */
+	}

This TODO and else { cannot be there.

Quoted 14 lines
drivers/hwmon/w83627hf.c renamed from drivers/hwmon/w83627hf.c.orig +12 -0
+
+	mutex_unlock(&data->update_lock);
+	return count;
+}
+
+#define sysfs_pwmclk(offset) \
+static ssize_t show_regs_pwmclk_##offset (struct device *dev, struct device_attribute *attr, char
 *buf) \
+{ \
+	return show_pwmclk_reg(dev, buf, offset); \
+} \
+static ssize_t \
+store_regs_pwmclk_##offset (struct device *dev, struct device_attribute *attr, const char *buf,
 size_t count) \

drivers/hwmon/w83627hf.c device_create_file(&client->dev, &dev_at · line 971

Quoted 10 lines
drivers/hwmon/w83627hf.c renamed from drivers/hwmon/w83627hf.c.orig +10 -0
+{ \
+	return store_pwmclk_reg(dev, buf, count, offset); \
+} \
+static DEVICE_ATTR(pwmclk##offset, S_IRUGO | S_IWUSR, \
+		  show_regs_pwmclk_##offset, store_regs_pwmclk_##offset);
+
+sysfs_pwmclk(1);
+sysfs_pwmclk(2);
+// sysfs_pwmclk(3); /* TODO w83627thf, w83637hf and w83687thf */
+

I think you may use SENSOR_DEVICE_ATTR to pass the 1..2.. numbers more easily
SENSOR_DEVICE_ATTR(pwm1_freq, S_IWUSR | S_IRUGO, show_pwm_freq, store_pwm_freq,
0), <- here you can pass 0 and you dont need nr - 1 anymore

This is also preferred method for new code. Then you need to group those SENSOR_DEVICE_ATTR to array of pointers and not to array of structures as all other hwmon drivers do. I think Mark is working on conversion.

Check the pca9539.c for details please, how to declare it.

If you have more questions, just ask please.

Regards
Rudolf