* [lm-sensors] [PATCH] f75375s: added Fintek f75387 support
@ 2011-12-01 20:59 Bjoern Gerhart
2011-12-02 3:37 ` Guenter Roeck
` (11 more replies)
0 siblings, 12 replies; 13+ messages in thread
From: Bjoern Gerhart @ 2011-12-01 20:59 UTC (permalink / raw)
To: lm-sensors
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).
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
@@ -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)
#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];
};
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")) {
+ /* 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));
+ }
data->fan[nr] f75375_read16(client, F75375_REG_FAN(nr));
}
@@ -433,8 +452,10 @@
mutex_unlock(&data->update_lock);
return count;
}
+
#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;
}
+
#define show_fan(thing) \
static ssize_t show_##thing(struct device *dev, struct device_attribute *attr, \
char *buf)\
@@ -596,6 +626,7 @@
NULL
};
+
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")) {
+ /* 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;
+ }
+
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";
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);
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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [lm-sensors] [PATCH] f75375s: added Fintek f75387 support
2011-12-01 20:59 [lm-sensors] [PATCH] f75375s: added Fintek f75387 support Bjoern Gerhart
@ 2011-12-02 3:37 ` Guenter Roeck
2011-12-02 4:00 ` Guenter Roeck
` (10 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2011-12-02 3:37 UTC (permalink / raw)
To: lm-sensors
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.
> 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@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [lm-sensors] [PATCH] f75375s: added Fintek f75387 support
2011-12-01 20:59 [lm-sensors] [PATCH] f75375s: added Fintek f75387 support Bjoern Gerhart
2011-12-02 3:37 ` Guenter Roeck
@ 2011-12-02 4:00 ` Guenter Roeck
2011-12-02 22:09 ` Bjoern Gerhart
` (9 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2011-12-02 4:00 UTC (permalink / raw)
To: lm-sensors
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@lm-sensors.org
> > http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [lm-sensors] [PATCH] f75375s: added Fintek f75387 support
2011-12-01 20:59 [lm-sensors] [PATCH] f75375s: added Fintek f75387 support Bjoern Gerhart
2011-12-02 3:37 ` Guenter Roeck
2011-12-02 4:00 ` Guenter Roeck
@ 2011-12-02 22:09 ` Bjoern Gerhart
2011-12-03 19:52 ` Guenter Roeck
` (8 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Bjoern Gerhart @ 2011-12-02 22:09 UTC (permalink / raw)
To: lm-sensors
Hi Guenter,
the mainboard I tested the module on is an Adlink NanoX-TC. However,
please note that the f75387 chip is _not_ assembled on this board, but
on the connected proprietary Interface Board developed by our inhouse
hardware team...
Thanks so much for the patch review and feedback, which sounds really
plausible! Below you find the updated patch implementing the
proposals.
2011/12/2 Guenter Roeck <guenter.roeck@ericsson.com>:
> On Thu, Dec 01, 2011 at 10:37:47PM -0500, Guenter Roeck wrote:
> 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.
--
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
+++ linux-2.6.39/drivers/hwmon/f75375s.c 2011-12-02 14:48:48.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) (0x14 + (nr) + 6)
#define F75375_REG_TEMP_HIGH(nr) (0x28 + (nr) * 2)
#define F75375_REG_TEMP_HYST(nr) (0x29 + (nr) * 2)
@@ -108,7 +112,11 @@
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 8bit temp readings, the LSB just is 0.
+ */
+ s16 temp11[2];
s8 temp_high[2];
s8 temp_max_hyst[2];
};
@@ -122,6 +130,7 @@
static const struct i2c_device_id f75375_id[] = {
{ "f75373", f75373 },
{ "f75375", f75375 },
+ { "f75387", f75387 },
{ }
};
MODULE_DEVICE_TABLE(i2c, f75375_id);
@@ -205,8 +214,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));
}
@@ -435,13 +449,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 +540,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 +700,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 +731,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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [lm-sensors] [PATCH] f75375s: added Fintek f75387 support
2011-12-01 20:59 [lm-sensors] [PATCH] f75375s: added Fintek f75387 support Bjoern Gerhart
` (2 preceding siblings ...)
2011-12-02 22:09 ` Bjoern Gerhart
@ 2011-12-03 19:52 ` Guenter Roeck
2011-12-07 19:51 ` Bjoern Gerhart
` (7 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2011-12-03 19:52 UTC (permalink / raw)
To: lm-sensors
Hi Bjoern,
On Fri, Dec 02, 2011 at 05:09:36PM -0500, Bjoern Gerhart wrote:
> Hi Guenter,
>
> the mainboard I tested the module on is an Adlink NanoX-TC. However,
> please note that the f75387 chip is _not_ assembled on this board, but
> on the connected proprietary Interface Board developed by our inhouse
> hardware team...
>
> Thanks so much for the patch review and feedback, which sounds really
> plausible! Below you find the updated patch implementing the
> proposals.
>
Yes, that is much better. Couple of minor comments below.
> 2011/12/2 Guenter Roeck <guenter.roeck@ericsson.com>:
> > On Thu, Dec 01, 2011 at 10:37:47PM -0500, Guenter Roeck wrote:
> > 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.
> --
> 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
> +++ linux-2.6.39/drivers/hwmon/f75375s.c 2011-12-02 14:48:48.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) (0x14 + (nr) + 6)
You really seem to like that ;). Still, I think it should be
#define F75387_REG_TEMP11_LSB(nr) (0x1a + (nr))
instead.
> #define F75375_REG_TEMP_HIGH(nr) (0x28 + (nr) * 2)
> #define F75375_REG_TEMP_HYST(nr) (0x29 + (nr) * 2)
>
> @@ -108,7 +112,11 @@
> 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 8bit temp readings, the LSB just is 0.
> + */
Multi-line comment should start with
/*
* f75387: For remote temperature reading, it uses signed 11-bit
> + s16 temp11[2];
> s8 temp_high[2];
> s8 temp_max_hyst[2];
> };
> @@ -122,6 +130,7 @@
> static const struct i2c_device_id f75375_id[] = {
> { "f75373", f75373 },
> { "f75375", f75375 },
> + { "f75387", f75387 },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, f75375_id);
> @@ -205,8 +214,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));
> }
> @@ -435,13 +449,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 +540,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 +700,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 +731,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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [lm-sensors] [PATCH] f75375s: added Fintek f75387 support
2011-12-01 20:59 [lm-sensors] [PATCH] f75375s: added Fintek f75387 support Bjoern Gerhart
` (3 preceding siblings ...)
2011-12-03 19:52 ` Guenter Roeck
@ 2011-12-07 19:51 ` Bjoern Gerhart
2011-12-07 20:58 ` Guenter Roeck
` (6 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Bjoern Gerhart @ 2011-12-07 19:51 UTC (permalink / raw)
To: lm-sensors
Hi Guenter,
2011/12/3 Guenter Roeck <guenter.roeck@ericsson.com>:
> 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.
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..?
--
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
+++ 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));
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 */
+ 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));
+
+ 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);
+ }
+
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) ;
- 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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [lm-sensors] [PATCH] f75375s: added Fintek f75387 support
2011-12-01 20:59 [lm-sensors] [PATCH] f75375s: added Fintek f75387 support Bjoern Gerhart
` (4 preceding siblings ...)
2011-12-07 19:51 ` Bjoern Gerhart
@ 2011-12-07 20:58 ` Guenter Roeck
2011-12-08 17:35 ` Guenter Roeck
` (5 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2011-12-07 20:58 UTC (permalink / raw)
To: lm-sensors
Hi Bjoern,
On Wed, 2011-12-07 at 14:51 -0500, Bjoern Gerhart wrote:
> Hi Guenter,
>
> 2011/12/3 Guenter Roeck <guenter.roeck@ericsson.com>:
> > 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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [lm-sensors] [PATCH] f75375s: added Fintek f75387 support
2011-12-01 20:59 [lm-sensors] [PATCH] f75375s: added Fintek f75387 support Bjoern Gerhart
` (5 preceding siblings ...)
2011-12-07 20:58 ` Guenter Roeck
@ 2011-12-08 17:35 ` Guenter Roeck
2011-12-08 22:04 ` Bjoern Gerhart
` (4 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2011-12-08 17:35 UTC (permalink / raw)
To: lm-sensors
On Wed, 2011-12-07 at 15:58 -0500, Guenter Roeck wrote:
> Hi Bjoern,
>
> On Wed, 2011-12-07 at 14:51 -0500, Bjoern Gerhart wrote:
> > Hi Guenter,
> >
> > 2011/12/3 Guenter Roeck <guenter.roeck@ericsson.com>:
> > > 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.
>
Hi Bjoern,
couple of additional comments below.
> > 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.
>
The point of the original code is that the fanX_mode bits are cleared
for the current fan, preparing the value for speed mode (which is why
nothing changes below for val=3, speed mode). For F75387, you'll want
to clear the relevant bits as well.
if (data->kind = f75387) {
fanmode &= ~(1 << F75387_FAN_DUTY_MODE(nr));
fanmode &= ~(1 << F75387_FAN_MANU_MODE(nr));
} else {
fanmode &= ~(3 << FAN_CTRL_MODE(nr));
}
> > 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));
Wrong, since we never set the expected rpm registers. Needs to follow
pwm; otherwise setting the pwm register to 255 later on would not help.
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));
Already reset above, so no need to clear it here.
> > + fanmode |= (1 << F75387_FAN_DUTY_MODE(nr));
> > + } else
> > + fanmode |= (2 << FAN_CTRL_MODE(nr));
> > +
> > + break;
> > + case 3: /* fan speed */
To follow fan speed, we'll have to set manual mode for F75387.
if (data->kind = f75387)
fanmode |= (1 << F75387_FAN_MANU_MODE(nr));
> > + break;
> > }
> > +
Actually, thinking about this function, it might be simpler to change it
to check for data->kind first, and then have two separate case
statements to determine fanmode. Something along the line of:
if (data->kind = f75387) {
fanmode &= ~(1 << F75387_FAN_DUTY_MODE(nr));
fanmode &= ~(1 << F75387_FAN_MANU_MODE(nr));
switch (val) {
case 0: /* full speed */
fanmode |= (1 << F75387_FAN_MANU_MODE(nr));
fanmode |= (1 << F75387_FAN_DUTY_MODE(nr));
data->pwm[nr] = 255;
f75375_write8(client,
F75375_REG_FAN_PWM_DUTY(nr),
data->pwm[nr]);
break;
case 1: /* PWM */
fanmode |= (1 << F75387_FAN_MANU_MODE(nr));
fanmode |= (1 << F75387_FAN_DUTY_MODE(nr));
break;
case 2: /* AUTOMATIC*/
fanmode |= (1 << F75387_FAN_DUTY_MODE(nr));
break;
case 3: /* fan speed */
fanmode |= (1 << F75387_FAN_MANU_MODE(nr));
break;
}
} else {
fanmode &= ~(3 << FAN_CTRL_MODE(nr));
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;
}
}
On a side note, the valid range of <val> is <0..4>. No idea what 4 is
supposed to be used for. I suspect it is a bug and should be <0..3>.
Separate patch, though.
We should also have a fanX_target attribute to be able to set the target
speed (registers 0x84 and 0x85) for all chips (so that setting pwmX_mode
to 3 would actually make sense), but that would be yet another patch.
> > 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;
Oddly enough, the code never sets pwm_mode[] except here, which means it
is always displayed as 0 (DC) unless it is changed explicitly. And
DC/Linear mode isn't even supported on F75373.
The code also doesn't initialize pwm_enable unless it is (re-)configured
through platform_data, which means reading pwmX_enable will also return
0 until overwritten unless there is platform data.
Yet another set of patches to fix that...
> > 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@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [lm-sensors] [PATCH] f75375s: added Fintek f75387 support
2011-12-01 20:59 [lm-sensors] [PATCH] f75375s: added Fintek f75387 support Bjoern Gerhart
` (6 preceding siblings ...)
2011-12-08 17:35 ` Guenter Roeck
@ 2011-12-08 22:04 ` Bjoern Gerhart
2011-12-08 22:34 ` Guenter Roeck
` (3 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Bjoern Gerhart @ 2011-12-08 22:04 UTC (permalink / raw)
To: lm-sensors
Hi Guenter,
so for solving the other issues you mentioned, I'd first apply your
set of 4 patches on the original f75375s. Then I'd implement my f75387
related modifications within your latest review proposals, test the
resulting binary module on my f75387 hardware and then create a fifth
patch.
Would that order lead to a valid set of patches?
--
Björn Gerhart
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [lm-sensors] [PATCH] f75375s: added Fintek f75387 support
2011-12-01 20:59 [lm-sensors] [PATCH] f75375s: added Fintek f75387 support Bjoern Gerhart
` (7 preceding siblings ...)
2011-12-08 22:04 ` Bjoern Gerhart
@ 2011-12-08 22:34 ` Guenter Roeck
2011-12-09 20:12 ` Björn Gerhart
` (2 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2011-12-08 22:34 UTC (permalink / raw)
To: lm-sensors
Hi Bjoern,
On Thu, Dec 08, 2011 at 05:04:54PM -0500, Bjoern Gerhart wrote:
> Hi Guenter,
>
> so for solving the other issues you mentioned, I'd first apply your
> set of 4 patches on the original f75375s. Then I'd implement my f75387
> related modifications within your latest review proposals, test the
> resulting binary module on my f75387 hardware and then create a fifth
> patch.
>
> Would that order lead to a valid set of patches?
>
Yes, that would be the best way to proceed, only it is 5 patches by now.
Did I copy you on the last one ?
If you use git, it should be straightforward to merge your patch on top of mine.
I would have done that, only I had trouble applying yours because it was somehow
corrupted.
Of course, it would also help tremendously if you would fine the time to review
my patches :).
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [lm-sensors] [PATCH] f75375s: added Fintek f75387 support
2011-12-01 20:59 [lm-sensors] [PATCH] f75375s: added Fintek f75387 support Bjoern Gerhart
` (8 preceding siblings ...)
2011-12-08 22:34 ` Guenter Roeck
@ 2011-12-09 20:12 ` Björn Gerhart
2011-12-09 21:02 ` Björn Gerhart
2011-12-09 21:47 ` Guenter Roeck
11 siblings, 0 replies; 13+ messages in thread
From: Björn Gerhart @ 2011-12-09 20:12 UTC (permalink / raw)
To: lm-sensors
Hi Guenter,
2011/12/8 Guenter Roeck <guenter.roeck@ericsson.com>:
> Hi Bjoern,
>
> On Thu, Dec 08, 2011 at 05:04:54PM -0500, Bjoern Gerhart wrote:
>> Hi Guenter,
>>
>> so for solving the other issues you mentioned, I'd first apply your
>> set of 4 patches on the original f75375s. Then I'd implement my f75387
>> related modifications within your latest review proposals, test the
>> resulting binary module on my f75387 hardware and then create a fifth
>> patch.
>>
>> Would that order lead to a valid set of patches?
>>
> Yes, that would be the best way to proceed, only it is 5 patches by now.
> Did I copy you on the last one ?
>
Yes - today I noticed that I already had received your latest (fifth) f75375s related patch.
> If you use git, it should be straightforward to merge your patch on top of mine.
> I would have done that, only I had trouble applying yours because it was somehow
> corrupted.
>
I'm not quite familiar with using git yet, so up to now using the classical diff / patch commands for creating and applying patches. It seems that the googlemail web interface inserts line feeds after about 70 characters, which leads to the corrupted patch. I try another mail client for the next patch to send...
> Of course, it would also help tremendously if you would fine the time to review
> my patches :).
>
Yeah, I reviewed each of the f75375s related patches. Beside the functionality stuff and improved error handling I noticed some guideline related improvements also. But in fact I could not detect nonconformities or code fussiness, so there's no idea for improvements on my side.
> Thanks,
> Guenter
--
Bjoern Gerhart
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [lm-sensors] [PATCH] f75375s: added Fintek f75387 support
2011-12-01 20:59 [lm-sensors] [PATCH] f75375s: added Fintek f75387 support Bjoern Gerhart
` (9 preceding siblings ...)
2011-12-09 20:12 ` Björn Gerhart
@ 2011-12-09 21:02 ` Björn Gerhart
2011-12-09 21:47 ` Guenter Roeck
11 siblings, 0 replies; 13+ messages in thread
From: Björn Gerhart @ 2011-12-09 21:02 UTC (permalink / raw)
To: lm-sensors
Hi Guenter,
thanks a lot for your deep review and fix proposals. As a result of it, appended to this mail there's an updated patch intended to be applied ontop of your patches given yesterday. Hope that it will not get corrupted this time.. :-/
The compilation against kernel 2.6.39 with all patches applied does work without warnings, and my f75387 still works (but as before without connected fans...).
>> 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.
>
Okay sounds good, so I'll contact him about this in a separate mail.. ;-)
--
Bjoern Gerhart
diff -uNr linux-2.6.39-5patched/drivers/hwmon/f75375s.c linux-2.6.39/drivers/hwmon/f75375s.c
--- linux-2.6.39-5patched/drivers/hwmon/f75375s.c 2011-12-09 15:41:01.000000000 +0100
+++ linux-2.6.39/drivers/hwmon/f75375s.c 2011-12-09 21:41:10.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));
}
@@ -313,24 +331,50 @@
return -EINVAL;
fanmode = f75375_read8(client, F75375_REG_FAN_TIMER);
- fanmode &= ~(3 << FAN_CTRL_MODE(nr));
-
- 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;
+ if (data->kind = f75387) {
+ /* clear each fanX_mode bit before setting them properly */
+ fanmode &= ~(1 << F75387_FAN_DUTY_MODE(nr));
+ fanmode &= ~(1 << F75387_FAN_MANU_MODE(nr));
+ switch (val) {
+ case 0: /* full speed */
+ fanmode |= (1 << F75387_FAN_MANU_MODE(nr));
+ fanmode |= (1 << F75387_FAN_DUTY_MODE(nr));
+ data->pwm[nr] = 255;
+ f75375_write8(client, F75375_REG_FAN_PWM_DUTY(nr),
+ data->pwm[nr]);
+ break;
+ case 1: /* PWM */
+ fanmode |= (1 << F75387_FAN_MANU_MODE(nr));
+ fanmode |= (1 << F75387_FAN_DUTY_MODE(nr));
+ break;
+ case 2: /* AUTOMATIC*/
+ fanmode |= (1 << F75387_FAN_DUTY_MODE(nr));
+ break;
+ case 3: /* fan speed */
+ fanmode |= (1 << F75387_FAN_MANU_MODE(nr));
+ break;
+ }
+ } else {
+ /* clear each fanX_mode bit before setting them properly */
+ fanmode &= ~(3 << FAN_CTRL_MODE(nr));
+ 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;
+ }
}
+
f75375_write8(client, F75375_REG_FAN_TIMER, fanmode);
data->pwm_enable[nr] = val;
return 0;
@@ -364,6 +408,8 @@
unsigned long val;
int err;
u8 conf;
+ char reg;
+ char ctrl;
err = kstrtoul(buf, 10, &val);
if (err < 0)
@@ -376,14 +422,23 @@
if (data->kind = f75373 && val = 0)
return -EINVAL;
+ /* take care for different registers */
+ 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);
+ }
+
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);
- f75375_write8(client, F75375_REG_CONFIG1, conf);
+ f75375_write8(client, reg, conf);
data->pwm_mode[nr] = val;
mutex_unlock(&data->update_lock);
return count;
@@ -475,13 +530,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,
@@ -577,12 +633,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,
@@ -660,11 +716,17 @@
if (!f75375s_pdata) {
u8 conf, mode;
int nr;
+ char fan_ctrl_linear;
conf = f75375_read8(client, F75375_REG_CONFIG1);
mode = f75375_read8(client, F75375_REG_FAN_TIMER);
for (nr = 0; nr < 2; nr++) {
- if (!(conf & (1 << FAN_CTRL_LINEAR(nr))))
+ if (data->kind = f75387)
+ fan_ctrl_linear = F75387_FAN_CTRL_LINEAR(nr);
+ else
+ fan_ctrl_linear = F75375_FAN_CTRL_LINEAR(nr);
+
+ if (!(conf & (1 << fan_ctrl_linear)))
data->pwm_mode[nr] = 1;
switch ((mode >> FAN_CTRL_MODE(nr)) & 3) {
case 0: /* speed */
@@ -763,10 +825,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;
@@ -789,7 +856,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-5patched/drivers/hwmon/Kconfig linux-2.6.39/drivers/hwmon/Kconfig
--- linux-2.6.39-5patched/drivers/hwmon/Kconfig 2011-12-09 15:41:09.000000000 +0100
+++ linux-2.6.39/drivers/hwmon/Kconfig 2011-12-09 16:44:14.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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [lm-sensors] [PATCH] f75375s: added Fintek f75387 support
2011-12-01 20:59 [lm-sensors] [PATCH] f75375s: added Fintek f75387 support Bjoern Gerhart
` (10 preceding siblings ...)
2011-12-09 21:02 ` Björn Gerhart
@ 2011-12-09 21:47 ` Guenter Roeck
11 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2011-12-09 21:47 UTC (permalink / raw)
To: lm-sensors
On Fri, Dec 09, 2011 at 03:12:50PM -0500, Björn Gerhart wrote:
> Hi Guenter,
>
> 2011/12/8 Guenter Roeck <guenter.roeck@ericsson.com>:
> > Hi Bjoern,
> >
> > On Thu, Dec 08, 2011 at 05:04:54PM -0500, Bjoern Gerhart wrote:
> >> Hi Guenter,
> >>
> >> so for solving the other issues you mentioned, I'd first apply your
> >> set of 4 patches on the original f75375s. Then I'd implement my f75387
> >> related modifications within your latest review proposals, test the
> >> resulting binary module on my f75387 hardware and then create a fifth
> >> patch.
> >>
> >> Would that order lead to a valid set of patches?
> >>
> > Yes, that would be the best way to proceed, only it is 5 patches by now.
> > Did I copy you on the last one ?
> >
> Yes - today I noticed that I already had received your latest (fifth) f75375s related patch.
>
> > If you use git, it should be straightforward to merge your patch on top of mine.
> > I would have done that, only I had trouble applying yours because it was somehow
> > corrupted.
> >
> I'm not quite familiar with using git yet, so up to now using the classical diff / patch commands for creating and applying patches. It seems that the googlemail web interface inserts line feeds after about 70 characters, which leads to the corrupted patch. I try another mail client for the next patch to send...
>
> > Of course, it would also help tremendously if you would fine the time to review
> > my patches :).
> >
> Yeah, I reviewed each of the f75375s related patches. Beside the functionality stuff and improved error handling I noticed some guideline related improvements also. But in fact I could not detect nonconformities or code fussiness, so there's no idea for improvements on my side.
>
Hi Bjoern,
things are a bit more formal in Linux land.
After you reviewed a patch, and you are happy with it, reply to it by adding
Acked-by: Your Name <your_email@somewhere>
or
Reviewed-by: Your Name <your_email@somewhere>
right under the Signed-off: line.
Before you do that, read Documentation/SubmittingPatches, chapter 13 (using Acked-by)
and chapter 14 (using Reviewed-by) to understand what it means.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-12-09 21:47 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-12-01 20:59 [lm-sensors] [PATCH] f75375s: added Fintek f75387 support Bjoern Gerhart
2011-12-02 3:37 ` Guenter Roeck
2011-12-02 4:00 ` Guenter Roeck
2011-12-02 22:09 ` Bjoern Gerhart
2011-12-03 19:52 ` Guenter Roeck
2011-12-07 19:51 ` Bjoern Gerhart
2011-12-07 20:58 ` Guenter Roeck
2011-12-08 17:35 ` Guenter Roeck
2011-12-08 22:04 ` Bjoern Gerhart
2011-12-08 22:34 ` Guenter Roeck
2011-12-09 20:12 ` Björn Gerhart
2011-12-09 21:02 ` Björn Gerhart
2011-12-09 21:47 ` Guenter Roeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.