Hi Bjoern, On Wed, 2011-12-07 at 14:51 -0500, Bjoern Gerhart wrote: > Hi Guenter, > > 2011/12/3 Guenter Roeck <guenter.roeck@xxxxxxxxxxxx>: > > Yes, that is much better. Couple of minor comments below. > > > Thanks ;-) > > >>> Configuration register 0x01 and fan register 0x60 are both different. Both are used > >>> and needed for fan configuration, so that is definitely a problem that will have > >>> to be addressed. > >>> > >> Ok... so I'll have a look at the difference in the fan related > >> registers in order to complete the f75387 implementation. > >> > > Yes, we'll need that. Fortunately you have your own board, so hopefully you should > > be able to play with fan control even if the pins are not connected. > > > I'm not really sure about how in detail the fan control works and how > it is influenced. So just based on the difference in the two chip > specs on registers 0x01 and 0x60, I modified the origin code for > implementing it. The resulting patch also including your proposals is > attached inline at the end. > I'll look into the merits tonight or tomorrow. Couple of coding style comments below, though. > Obviously, Harald Dunkel requested the f75387 implementation in 2007. > Do you think it would be a good idea to contact him asking for a fan > test on his board..? > Definitely. Worst case he does not reply or no longer has the hardware. > -- > Bjoern Gerhart > > > > diff -uNr linux-2.6.39-orig/drivers/hwmon/f75375s.c > linux-2.6.39/drivers/hwmon/f75375s.c > --- linux-2.6.39-orig/drivers/hwmon/f75375s.c 2011-12-02 > 14:41:16.000000000 +0100 Somewhere your patch got line wrapped and thus corrupted, making it quite difficult to work with. > +++ linux-2.6.39/drivers/hwmon/f75375s.c 2011-12-06 14:33:49.000000000 +0100 > @@ -1,6 +1,6 @@ > /* > - * f75375s.c - driver for the Fintek F75375/SP and F75373 > - * hardware monitoring features > + * f75375s.c - driver for the Fintek F75375/SP, F75373 and > + * F75387SG/RG hardware monitoring features > * Copyright (C) 2006-2007 Riku Voipio > * > * Datasheets available at: > @@ -10,6 +10,9 @@ > * > * f75373: > * http://www.fintek.com.tw/files/productfiles/F75373_V025P.pdf > + * > + * f75387: > + * http://www.fintek.com.tw/files/productfiles/F75387_V027P.pdf > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License as published by > @@ -40,7 +43,7 @@ > /* Addresses to scan */ > static const unsigned short normal_i2c[] = { 0x2d, 0x2e, I2C_CLIENT_END }; > > -enum chips { f75373, f75375 }; > +enum chips { f75373, f75375, f75387 }; > > /* Fintek F75375 registers */ > #define F75375_REG_CONFIG0 0x0 > @@ -59,6 +62,7 @@ > #define F75375_REG_VOLT_LOW(nr) (0x21 + (nr) * 2) > > #define F75375_REG_TEMP(nr) (0x14 + (nr)) > +#define F75387_REG_TEMP11_LSB(nr) (0x1a + (nr)) > #define F75375_REG_TEMP_HIGH(nr) (0x28 + (nr) * 2) > #define F75375_REG_TEMP_HYST(nr) (0x29 + (nr) * 2) > > @@ -78,8 +82,11 @@ > #define F75375_REG_PWM1_DROP_DUTY 0x6B > #define F75375_REG_PWM2_DROP_DUTY 0x6C > > -#define FAN_CTRL_LINEAR(nr) (4 + nr) > +#define F75375_FAN_CTRL_LINEAR(nr) (4 + nr) > +#define F75387_FAN_CTRL_LINEAR(nr) (1 + ((nr) * 4)) > #define FAN_CTRL_MODE(nr) (4 + ((nr) * 2)) > +#define F75387_FAN_DUTY_MODE(nr) (2 + ((nr) * 4)) > +#define F75387_FAN_MANU_MODE(nr) ((nr) * 4) > > /* > * Data structures and manipulation thereof > @@ -108,7 +115,12 @@ > u8 pwm[2]; > u8 pwm_mode[2]; > u8 pwm_enable[2]; > - s8 temp[2]; > + /* > + * f75387: For remote temperature reading, it uses signed 11-bit > + * values with LSB = 0.125 degree Celsius, left-justified in 16-bit > + * registers. For original 8-bit temp readings, the LSB just is 0. > + */ > + s16 temp11[2]; > s8 temp_high[2]; > s8 temp_max_hyst[2]; > }; > @@ -122,6 +134,7 @@ > static const struct i2c_device_id f75375_id[] = { > { "f75373", f75373 }, > { "f75375", f75375 }, > + { "f75387", f75387 }, > { } > }; > MODULE_DEVICE_TABLE(i2c, f75375_id); > @@ -205,8 +218,13 @@ > if (time_after(jiffies, data->last_updated + 2 * HZ) > || !data->valid) { > for (nr = 0; nr < 2; nr++) { > - data->temp[nr] = > - f75375_read8(client, F75375_REG_TEMP(nr)); > + /* assign MSB, therefore shift it by 8 bits */ > + data->temp11[nr] = > + f75375_read8(client, F75375_REG_TEMP(nr)) << 8; > + if (data->kind == f75387) > + /* merge F75387's temperature LSB (11-bit) */ > + data->temp11[nr] |= > + f75375_read8(client, F75387_REG_TEMP11_LSB(nr)); > data->fan[nr] = > f75375_read16(client, F75375_REG_FAN(nr)); > } > @@ -298,24 +316,44 @@ > return -EINVAL; > > fanmode = f75375_read8(client, F75375_REG_FAN_TIMER); > - fanmode &= ~(3 << FAN_CTRL_MODE(nr)); > + if (data->kind != f75387) > + fanmode &= ~(3 << FAN_CTRL_MODE(nr)); > I'll have to look into this - it is a bit suspicious that you would not need a mask for F75387. > switch (val) { > - case 0: /* Full speed */ > - fanmode |= (3 << FAN_CTRL_MODE(nr)); > - data->pwm[nr] = 255; > - f75375_write8(client, F75375_REG_FAN_PWM_DUTY(nr), > - data->pwm[nr]); > - break; > - case 1: /* PWM */ > - fanmode |= (3 << FAN_CTRL_MODE(nr)); > - break; > - case 2: /* AUTOMATIC*/ > - fanmode |= (2 << FAN_CTRL_MODE(nr)); > - break; > - case 3: /* fan speed */ > - break; > + case 0: /* Full speed */ Documentation/CodingStyle, chapter 1, indentation: 'align the "switch" and its subordinate "case" labels in the same column' While this is not mandatory, but just "the preferred way", there is no reason to change it to something else in existing code. > + if (data->kind == f75387) { > + /* manual mode, follow expected RPM */ > + fanmode |= (1 << F75387_FAN_MANU_MODE(nr)); > + fanmode &= ~(1 << F75387_FAN_DUTY_MODE(nr)); > + } else > + fanmode |= (3 << FAN_CTRL_MODE(nr)); > + Chapter 3, Placing Braces and Spaces: Either use no braces, or use braces in both branches of an if statement. > + data->pwm[nr] = 255; > + f75375_write8(client, F75375_REG_FAN_PWM_DUTY(nr), > + data->pwm[nr]); > + break; > + case 1: /* PWM */ > + if (data->kind == f75387) { > + /* manual mode, follow expected PWM duty */ > + fanmode |= (1 << F75387_FAN_MANU_MODE(nr)); > + fanmode |= (1 << F75387_FAN_DUTY_MODE(nr)); > + } else > + fanmode |= (3 << FAN_CTRL_MODE(nr)); > + > + break; > + case 2: /* AUTOMATIC*/ > + if (data->kind == f75387) { > + /* automatic mode, follow expected PWM duty */ > + fanmode &= ~(1 << F75387_FAN_MANU_MODE(nr)); > + fanmode |= (1 << F75387_FAN_DUTY_MODE(nr)); > + } else > + fanmode |= (2 << FAN_CTRL_MODE(nr)); > + > + break; > + case 3: /* fan speed */ > + break; > } > + > f75375_write8(client, F75375_REG_FAN_TIMER, fanmode); > data->pwm_enable[nr] = val; > return 0; > @@ -344,18 +382,28 @@ > struct f75375_data *data = i2c_get_clientdata(client); > int val = simple_strtoul(buf, NULL, 10); > u8 conf = 0; > + char reg; > + char ctrl; > > if (!(val == 0 || val == 1)) > return -EINVAL; > > + if (data->kind == f75387) { > + reg = F75375_REG_FAN_TIMER; > + ctrl = F75387_FAN_CTRL_LINEAR(nr); > + } else { > + reg = F75375_REG_CONFIG1; > + ctrl = F75375_FAN_CTRL_LINEAR(nr); Indentation doesn't look right here. > + } > + > mutex_lock(&data->update_lock); > - conf = f75375_read8(client, F75375_REG_CONFIG1); > - conf &= ~(1 << FAN_CTRL_LINEAR(nr)); > + conf = f75375_read8(client, reg); > + conf &= ~(1 << ctrl); > > if (val == 0) > - conf |= (1 << FAN_CTRL_LINEAR(nr)) ; > + conf |= (1 << ctrl) ; Please remove the extra space since we are at it ... > > - f75375_write8(client, F75375_REG_CONFIG1, conf); > + f75375_write8(client, reg, conf); > data->pwm_mode[nr] = val; > mutex_unlock(&data->update_lock); > return count; > @@ -435,13 +483,14 @@ > } > #define TEMP_FROM_REG(val) ((val) * 1000) > #define TEMP_TO_REG(val) ((val) / 1000) > +#define TEMP11_FROM_REG(reg) ((reg) / 32 * 125) > > -static ssize_t show_temp(struct device *dev, struct device_attribute *attr, > +static ssize_t show_temp11(struct device *dev, struct device_attribute *attr, > char *buf) > { > int nr = to_sensor_dev_attr(attr)->index; > struct f75375_data *data = f75375_update_device(dev); > - return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp[nr])); > + return sprintf(buf, "%d\n", TEMP11_FROM_REG(data->temp11[nr])); > } > > static ssize_t show_temp_max(struct device *dev, struct device_attribute *attr, > @@ -525,12 +574,12 @@ > show_in_max, set_in_max, 3); > static SENSOR_DEVICE_ATTR(in3_min, S_IRUGO|S_IWUSR, > show_in_min, set_in_min, 3); > -static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL, 0); > +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp11, NULL, 0); > static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IRUGO|S_IWUSR, > show_temp_max_hyst, set_temp_max_hyst, 0); > static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO|S_IWUSR, > show_temp_max, set_temp_max, 0); > -static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp, NULL, 1); > +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp11, NULL, 1); > static SENSOR_DEVICE_ATTR(temp2_max_hyst, S_IRUGO|S_IWUSR, > show_temp_max_hyst, set_temp_max_hyst, 1); > static SENSOR_DEVICE_ATTR(temp2_max, S_IRUGO|S_IWUSR, > @@ -685,10 +734,15 @@ > > vendid = f75375_read16(client, F75375_REG_VENDOR); > chipid = f75375_read16(client, F75375_CHIP_ID); > - if (chipid == 0x0306 && vendid == 0x1934) > + if (vendid != 0x1934) > + return -ENODEV; > + > + if (chipid == 0x0306) > name = "f75375"; > - else if (chipid == 0x0204 && vendid == 0x1934) > + else if (chipid == 0x0204) > name = "f75373"; > + else if (chipid == 0x0410) > + name = "f75387"; > else > return -ENODEV; > > @@ -711,7 +765,7 @@ > > MODULE_AUTHOR("Riku Voipio"); > MODULE_LICENSE("GPL"); > -MODULE_DESCRIPTION("F75373/F75375 hardware monitoring driver"); > +MODULE_DESCRIPTION("F75373/F75375/F75387 hardware monitoring driver"); > > module_init(sensors_f75375_init); > module_exit(sensors_f75375_exit); > diff -uNr linux-2.6.39-orig/drivers/hwmon/Kconfig > linux-2.6.39/drivers/hwmon/Kconfig > --- linux-2.6.39-orig/drivers/hwmon/Kconfig 2011-12-02 14:41:28.000000000 +0100 > +++ linux-2.6.39/drivers/hwmon/Kconfig 2011-12-02 13:21:46.000000000 +0100 > @@ -335,11 +335,11 @@ > will be called f71882fg. > > config SENSORS_F75375S > - tristate "Fintek F75375S/SP and F75373" > + tristate "Fintek F75375S/SP, F75373 and F75387" > depends on I2C > help > If you say yes here you get support for hardware monitoring > - features of the Fintek F75375S/SP and F75373 > + features of the Fintek F75375S/SP, F75373 and F75387 > > This driver can also be built as a module. If so, the module > will be called f75375s. _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
