On Thu, Dec 01, 2011 at 10:37:47PM -0500, Guenter Roeck wrote: > On Thu, Dec 01, 2011 at 03:59:40PM -0500, Bjoern Gerhart wrote: > > Hi, > > > > as seen in the lm-sensor's current "Device Support Status", the f75387 > > is already detected by sensors-detect but not yet supported by any > > module. Because most of the f75387 behaviour seems to be the same as > > of the f75375s, I modified this existing module. > > > > So, below you find a patch inline of this mail, which enables the > > Fintek f75387 support to the existing kernel source code of > > drivers/hwmon/f75375s.c. The patch has been developed against kernel > > 2.6.39.4. The main difference concerning the datasheet compared to > > f75375 seems to be the 11bit temperature read. > > On the system I own (it's an Atom board whose vendor/model I'll lookup > > later) the sensor reads for voltages and temperatures of the f75387 > > seem to be plausible. The fan inputs are not connected on that board > > anyway, so they get ignore'd in my sensors3.conf (therefore unable to > > test its fan inputs). > > > > Jean, do you have a datasheet ? lm-sensors.org indicates someone does. > Would be great if we can confirm that nothing else is different. > > If you don't have time and it isn't NDA, feel free to send it to me. > Never mind - it is obviously referenced below. 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. > > Reviews and comments welcome ;-) > > > > -- > > Bjoern Gerhart > > > > > > > > --- linux-2.6.39.4-orig/drivers/hwmon/f75375s.c 2011-11-23 > > 09:48:00.000000000 +0100 > > +++ linux-2.6.39.4/drivers/hwmon/f75375s.c 2011-12-01 14:33:33.000000000 +0100 > > You should also update drivers/hwmon/Kconfig and Documentation/hwmon/f75375s. > > Oops, the latter doesn't exist, so I guess we can waive that unless you have > some spare time to write it ... > > > @@ -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,8 @@ > > #define F75375_REG_VOLT_LOW(nr) (0x21 + (nr) * 2) > > > > #define F75375_REG_TEMP(nr) (0x14 + (nr)) > > +#define F75387_REG_TEMP11_MSB(nr) (0x14 + (nr)) > > +#define F75387_REG_TEMP11_LSB(nr) (0x14 + (nr) + 6) > > 0x1a + (nr). I don't think you really need F75387_REG_TEMP11_MSB(). Also see below. > > > #define F75375_REG_TEMP_HIGH(nr) (0x28 + (nr) * 2) > > #define F75375_REG_TEMP_HYST(nr) (0x29 + (nr) * 2) > > > > @@ -111,6 +116,10 @@ > > s8 temp[2]; > > s8 temp_high[2]; > > s8 temp_max_hyst[2]; > > + /* f75387: For remote temperature reading, it uses signed 11-bit > > + * values with LSB = 0.125 degree Celsius, left-justified in 16-bit registers. > > + */ > > + s16 temp11[2]; > > Just use s16 temp[2]. Also see below. > > > }; > > > > static int f75375_detect(struct i2c_client *client, > > @@ -122,6 +131,7 @@ > > static const struct i2c_device_id f75375_id[] = { > > { "f75373", f75373 }, > > { "f75375", f75375 }, > > + { "f75387", f75387 }, > > { } > > }; > > MODULE_DEVICE_TABLE(i2c, f75375_id); > > @@ -205,8 +215,17 @@ > > 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)); > > + if (!strcmp(client->name, "f75387")) { > > if (data->kind == f75387) { > > > + /* First assign the most significant byte, therefore shift it by 8 bits */ > > + data->temp11[nr] = > > + f75375_read8(client, F75387_REG_TEMP11_MSB(nr)) << 8; > > + /* then assign the least significant byte without changing MSB */ > > + data->temp11[nr] |= > > + f75375_read8(client, F75387_REG_TEMP11_LSB(nr)); > > + } else { > > + data->temp[nr] = > > + f75375_read8(client, F75375_REG_TEMP(nr)); > > + } > > Would be easier to always store into an 16 (11) bit value. Read the upper 8 bit first and merge > the lower 8 bits for F75387 only. Then you can always assume you have a 16/11-bit value, and you > don't need the tricks overriding sensor_dev_attr_temp1_input.dev_attr.show (ie always use > show_temp11 for displaying the current temperature). And you don't need F75387_REG_TEMP11_MSB() > vs. F75375_REG_TEMP() either, and the code gets much simpler overall. > > > data->fan[nr] = > > f75375_read16(client, F75375_REG_FAN(nr)); > > } > > @@ -433,8 +452,10 @@ > > mutex_unlock(&data->update_lock); > > return count; > > } > > + > > Unnecessary whitespace change > > > #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, > > char *buf) > > @@ -444,6 +465,14 @@ > > return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp[nr])); > > } > > > > +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", TEMP11_FROM_REG(data->temp11[nr])); > > +} > > + > > static ssize_t show_temp_max(struct device *dev, struct device_attribute *attr, > > char *buf) > > { > > @@ -491,6 +520,7 @@ > > return count; > > } > > > > + > > Unnecessary whitespace change > > > #define show_fan(thing) \ > > static ssize_t show_##thing(struct device *dev, struct device_attribute *attr, \ > > char *buf)\ > > @@ -596,6 +626,7 @@ > > NULL > > }; > > > > + > > Unnecessary whitespace change > > > static const struct attribute_group f75375_group = { > > .attrs = f75375_attributes, > > }; > > @@ -631,6 +662,12 @@ > > mutex_init(&data->update_lock); > > data->kind = id->driver_data; > > > > + if (!strcmp(client->name, "f75387")) { > > if (data->kind == f75387) { > > > + /* correcting the show_temp function in order to support 11bit */ > > + sensor_dev_attr_temp1_input.dev_attr.show = show_temp11; > > + sensor_dev_attr_temp2_input.dev_attr.show = show_temp11; > > + } > > + > > Not needed - see above. > > > if ((err = sysfs_create_group(&client->dev.kobj, &f75375_group))) > > goto exit_free; > > > > @@ -689,12 +726,15 @@ > > name = "f75375"; > > else if (chipid == 0x0204 && vendid == 0x1934) > > name = "f75373"; > > + else if (chipid == 0x0410 && vendid == 0x1934) > > + name = "f75387"; > > Might be time to separate vendid to avoid checking it over and over again. > > if (vendid != 0x1934) > return -ENODEV; > ... > > > else > > return -ENODEV; > > > > version = f75375_read8(client, F75375_REG_VERSION); > > dev_info(&adapter->dev, "found %s version: %02X\n", name, version); > > strlcpy(info->type, name, I2C_NAME_SIZE); > > + strlcpy(client->name, name, I2C_NAME_SIZE); > > I don't think this is needed. > > > > > return 0; > > } > > @@ -711,7 +751,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); > > > > _______________________________________________ > > lm-sensors mailing list > > lm-sensors@xxxxxxxxxxxxxx > > http://lists.lm-sensors.org/mailman/listinfo/lm-sensors > > _______________________________________________ > lm-sensors mailing list > lm-sensors@xxxxxxxxxxxxxx > http://lists.lm-sensors.org/mailman/listinfo/lm-sensors _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
