fscpos driver

PATCH New Jean Delvare 4 participants 18 messages May 19, 2005

lore

Share & tools

lore

Jean Delvare

author

2005-05-19 06:25 UTC #0 lore raw

Hi Stefan,

Quoted 3 lines

I have finally completed what appears to be a fully working version of the fscpos driver (see attachment). I tried to obey the i2c coding policies as well as the kernel coding style guidelines.

See my comments inline.

Quoted 3 lines

/*
fscpos.c - Part of lm_sensors, Linux kernel modules for hardware monitoring Copyright (C) 2004, 2005 Stefan Ott <[email protected]>

Not part of lm_sensors anymore.

Quoted 2 lines

/* fan 2 */
/* min speed for fan2 not supported */

Say whether it's not supported by the chip or by the driver.

Quoted 11 lines

/* temperature 0 */
#define FSCPOS_REG_TEMP0_ACT 0x64
#define FSCPOS_REG_TEMP0_STATE 0x71
/* temperature 1 */
#define FSCPOS_REG_TEMP1_ACT 0x32
#define FSCPOS_REG_TEMP1_STATE 0x81
/* temperature 2 */
#define FSCPOS_REG_TEMP2_ACT 0x35
#define FSCPOS_REG_TEMP2_STATE 0x91

You might want to define static arrays instead, e.g.:
static u8 FSCPOS_REG_TEMP_ACT[] = { 0x64, 0x32, 0x35 };
Same goes for the other register sets. I guess it would make some parts of the code more simple. Maybe too late though.

Quoted 6 lines

/* Revision */
static ssize_t show_revision(struct device *dev, char *buf)
{
	struct fscpos_data *data = fscpos_update_device(dev);
	return sprintf(buf, "%u\n", data->revision);
}

Is there a real use for this? You could instead simply dev_info() it at module load time.

Quoted 2 lines

/* Temperature */
#define TEMP_FROM_REG(val) ((val - 128) * 1000)

Extra parentheses around "val" please.

Quoted 1 line

#define TEMP_INDEX_FROM_NUM(nr) ((nr) - 1)

Hm, not a very useful one. Your code would be easier to read without this macro IMHO.

Quoted 10 lines

static ssize_t set_temp_status(struct i2c_client *client, struct fscpos_data
*data, const char *buf, size_t count, int nr, int reg)
{
	/* bits 2..7 reserved, 0 read only => mask with 0x02 */
	unsigned long v = simple_strtoul(buf, NULL, 10) & 0x02;
	data->temp_status[TEMP_INDEX_FROM_NUM(nr)] &= ~v;
	
	fscpos_write_value(client, reg, v);
	return count;
}

Hmm, isn't it odd that a "status" file can be written to? What are the two bits for?

Quoted 3 lines

/* Fans */
#define RPM_FROM_REG(val) (val*60)

Extra parentheses here too, and spaces for consistency.

Quoted 1 line

#define FAN_INDEX_FROM_NUM(nr) ((nr) - 1)

Still not convinced ;)

Quoted 10 lines

static ssize_t set_fan_status(struct i2c_client *client, struct fscpos_data
*data, const char *buf, size_t count, int nr, int reg)
{
	/* bits 0..1, 3..7 reserved => mask with 0x04 */
	unsigned long v = simple_strtoul(buf, NULL, 10) & 0x04;
	data->fan_status[nr] &= ~v;

	fscpos_write_value(client, reg, v);
	return count;
}

Same comment as above, I don't get why this file would be writable.

Quoted 5 lines

static ssize_t set_fan_min(struct i2c_client *client, struct fscpos_data *data,
const char *buf, size_t count, int nr, int reg)
{
	data->fan_min[FAN_INDEX_FROM_NUM(nr)] = simple_strtoul(buf, NULL, 10) &
									0xff;

That "& 0xff" sucks. What you really want to do is check the input against a range of vality. Also, shouldn't the value be divided by 60?

Quoted 2 lines

/* Volts */
#define VOLT_FROM_REG(val,mult) (val*mult/255)

Parentheses please.

Quoted 4 lines

static ssize_t set_wdog_preset(struct i2c_client *client, struct fscpos_data
*data, const char *buf, size_t count, int reg)
{
	data->watchdog[0] = simple_strtoul(buf, NULL, 10) & 0xff;

Don't mask, check the value for validity instead.

Quoted 8 lines

/* Event */
static ssize_t show_event(struct device *dev, char *buf)

{
	/* bits 5..7 reserved => mask with 0x1f */
	struct fscpos_data *data = fscpos_update_device(dev);
	return sprintf(buf, "%u\n", data->global_event & 0x9b);
}

Quoted 6 lines

/* Control */
static ssize_t show_control(struct fscpos_data *data, char *buf)
{
	/* bits 1..7 reserved => mask with 0x01 */
	return sprintf(buf, "%u\n", data->global_control & 0x01);
}

What are these? Do you have a use for them?

Quoted 5 lines

/*
* Sysfs stuff
*/

#define create_getter(kind, sub) \
	static ssize_t sysfs_show_##kind##sub(struct device *dev, char *buf);\

Hmm, that extra first line is redundant, isn't it?

Quoted 6 lines

	static ssize_t sysfs_show_##kind##sub(struct device *dev, char *buf) \
	{ \
		struct fscpos_data *data = fscpos_update_device(dev); \
		return show_##kind##sub(data, buf); \
	}

Quoted 9 lines

#define create_getter_n(kind, offset, sub) \
	static ssize_t sysfs_show_##kind##offset##sub(struct device *dev, char\
							 		*buf);\
	static ssize_t sysfs_show_##kind##offset##sub(struct device *dev, char\
								 	*buf) \
	{ \
		struct fscpos_data *data = fscpos_update_device(dev); \
		return show_##kind##sub(data, buf, (offset)); \
	}

I think that the parentheses around "offset" are not needed.

Quoted 10 lines

#define create_setter_n(kind, offset, sub, reg) \
	static ssize_t sysfs_set_##kind##offset##sub (struct device *dev, \
					const char *buf, size_t count); \
	static ssize_t sysfs_set_##kind##offset##sub (struct device *dev, \
					const char *buf, size_t count) \
	{ \
		struct i2c_client *client = to_i2c_client(dev); \
		struct fscpos_data *data = i2c_get_clientdata(client); \
		return set_##kind##sub(client,data,buf,count,(offset),reg);\
	}

Same here.

Quoted 3 lines

#define sysfs_fan(offset, reg_status, reg_ripple, reg_min) \
	sysfs_fan_nomin(offset, reg_status, reg_ripple) \
	sysfs_rw_n(fan, _min, offset, reg_min); \

Trailing "\" is not needed.

Quoted 4 lines

#define sysfs_fan_nomin(offset, reg_status, reg_ripple) \
	sysfs_ro_n(fan, _input, offset); \
	sysfs_rw_n(fan, _status, offset, reg_status); \
	sysfs_rw_n(fan, _ripple, offset, reg_ripple); \

Same here. Also, wouldn't it make more sense to define them the other way around?

Quoted 4 lines

#define sysfs_watchdog(reg_wdog_preset, reg_wdog_state, reg_wdog_control) \
	sysfs_rw(wdog, _control, reg_wdog_control); \
	sysfs_rw(wdog, _preset, reg_wdog_preset); \
	sysfs_rw(wdog, _state, reg_wdog_state); \

Same here, trailing "\" not needed.

Quoted 8 lines

int fscpos_detect(struct i2c_adapter *adapter, int address, int kind) {
(...)
	const char *client_name = "";
(...)
	client_name = "fscpos";
(...)
	strlcpy(new_client->name, client_name, I2C_NAME_SIZE);

You can even get rid of the variable altogether.

Quoted 5 lines

      ERROR1:
	kfree(data);
      ERROR0:
	return err;
}

Please left-align the label. Also, prefered names would be exit: and exit_free:.

Quoted 7 lines

static int fscpos_detach_client(struct i2c_client *client)

{
	int err;

	if ((err = i2c_detach_client(client))) {
		dev_err(&client->dev, "fscpos.o: Client deregistration failed,"
						" client not detached.\n");

"fscpos.o" is redundant here, dev_err will write it for you.

Quoted 6 lines

static int fscpos_read_value(struct i2c_client *client, u8 reg)
{
	dev_dbg(&client->dev, "fscpos: read reg 0x%02x\n",reg);
	return i2c_smbus_read_byte_data(client, reg);
}

Quoted 6 lines

static int fscpos_write_value(struct i2c_client *client, u8 reg, u8 value)
{
	dev_dbg(&client->dev, "fscpos: write reg 0x%02x, val 0x%02x\n", reg,
									value);
	return i2c_smbus_write_byte_data(client, reg, value);
}

Same here, dev_dbg prints the driver name for you.

Quoted 1 line

/* Called when we have found a new FSCPOS. It should set limits, etc. */

Correct that comment. The driver should *not* set the limits (and as a matter of fact it doesn't set them).

Quoted 8 lines

static void fscpos_init_client(struct i2c_client *client)

{
	struct fscpos_data *data = i2c_get_clientdata(client);

	/* read revision from chip */
	data->revision =  fscpos_read_value(client,FSCPOS_REG_REVISION);
	/* setup missing fan2_min value */
	data->fan_min[2] = 0xff;

But that value is then never read... so why even have a memory slot for it?

Quoted 5 lines

static struct fscpos_data *fscpos_update_device(struct device *dev) {
(...)

                data->last_updated = jiffies;
                data->valid = 1;

Bogus indentation here (spaces instead of tabs).

Rest look fine to me! Please correct whatever needs be, then you send your work to Greg KH in the form of a patch against the most recent 2.6 kernel. The patch must include updated to Makefile and Kconfig. Don't forget to CC the sensors mailing-list.

Good work! :)

If you have fixes for the 2.4 driver, please send a patch to the sensors mailing-list.

If libsensors needs to be updated to support your new driver, a patch will be welcome as well.

Signature · 2 lines

Jean Delvare
http://khali.linux-fr.org/

Stefan Ott

2005-05-19 06:25 UTC #1 lore raw

Hi Jean

Thanks a lot for the review - I managed to clean most of the issues you mentioned. Some of them were just copied from other modules, others were my own :)

Quoted 4 lines

Quoted 1 line
static ssize_t show_revision(struct device *dev, char *buf)
Is there a real use for this? You could instead simply dev_info() it at module load time.

Well, the information probably isn't too relevant, but is there a reason not to allow access to it? Plus, if I remove it, "sensors -u" complains about "ERROR: Can't get feature `rev' data!"

Quoted 5 lines

Quoted 2 lines
static ssize_t set_temp_status
static ssize_t set_fan_status
Hmm, isn't it odd that a "status" file can be written to? What are the two bits for?

It allows you to clear the alert state. I noticed this doesn't comply with the sysfs guidelines which say alarms are read-only but it's quite handy after doing stupid things such as stopping a fan altogether (which causes an alert state for said fan and "sensors" won't show any more fan data until the state has been cleared).

Quoted 3 lines

Quoted 1 line
static ssize_t set_fan_min
[..] shouldn't the value be divided by 60?

Indeed, I fixed that. I do however still seem to get it wrong somehow: When I double the value, the fan appears to accelerate by 8*60 RPM. It might somehow be related to the ripple value but I just don't get it.

Quoted 1 line

Don't mask, check the value for validity instead.

For some registers (mainly watchdog_control) it's quite tricky to figure out all valid values (especially as messing with them might cause unwanted reactions such as spontaneous rebooting)...

Quoted 4 lines

Quoted 2 lines
static ssize_t show_event
static ssize_t show_control
What are these? Do you have a use for them?

I don't *REALLY* know whether they're that useful: Event seems to report all kinds of alerts which could also be gathered from other sources, but offers a single place to see whether everything is OK. The control register reports whether the alert LED is flashing. I haven't seen that LED (yet), nor would I know how to make it flash, but I thought I'll just implement access to all the information available. And, as above, "sensors -u" complains if I remove them.

Quoted 4 lines

Rest look fine to me! Please correct whatever needs be, then you send your work to Greg KH in the form of a patch against the most recent 2.6 kernel. The patch must include updated to Makefile and Kconfig. Don't forget to CC the sensors mailing-list.

Okay, I'll do that as soon as we cleared the remaining issues.

Quoted 1 line

Good work! :)

Thanks!

Quoted 5 lines

If you have fixes for the 2.4 driver, please send a patch to the sensors mailing-list.
If libsensors needs to be updated to support your new driver, a patch will be welcome as well.

I tried to stick with the interface provided by the old driver so I don't think libsensors will need to be updated.

Thanks & regards

Signature · 12 lines

Stefan Ott
http://www.desire.ch

"Give a man a fire and he's warm for a day, but set fire to him and he's
warm for the rest of his life."  -- Terry Pratchett, Jingo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050105/cac92724/attachment.bin

Stefan Ott

2005-05-19 06:25 UTC #2 lore raw

In reply to Jean Delvare

Quoted 9 lines · On Wed, 5 Jan 2005 09:41:20 +0100 (CET) "Jean Delvare" <[email protected]>…

Hi Stefan,
Quoted 3 lines
Thanks a lot for the review - I managed to clean most of the issues you mentioned. Some of them were just copied from other modules, others were my own :)
If this means that some other 2.4 or 2.6 drivers would need fixing, just tell me where and I'll fix them.

Most of the code was from the fscher driver, especially all the masking and the sysfs things we're currently discussing (like revision and control) and some badly placed parentheses. The register definitions and some minor issues were from the old fscpos driver.

Quoted 8 lines

Quoted 7 lines
Quoted 4 lines
Quoted 1 line
static ssize_t show_revision(struct device *dev, char *buf)
Is there a real use for this? You could instead simply dev_info() it at module load time.
Well, the information probably isn't too relevant, but is there a reason not to allow access to it?

Quoted 6 lines

It's not the way you want to look at things, or you will end up with a driver way bigger than it could and should. We want to keep the drivers to a minimum size, in order to save memory space and limit the possible bugs and the maintainance effort. We should get rid of whatever isn't explicitely needed. It can still be added later if there is a real need.

Okay, I put a dev_info in fscpos_detect and the sysfs file is gone.

Quoted 12 lines

Quoted 11 lines
Quoted 5 lines
Quoted 2 lines
static ssize_t set_temp_status
static ssize_t set_fan_status
Hmm, isn't it odd that a "status" file can be written to? What are the two bits for?
It allows you to clear the alert state. I noticed this doesn't comply with the sysfs guidelines which say alarms are read-only but it's quite handy after doing stupid things such as stopping a fan altogether (which causes an alert state for said fan and "sensors" won't show any more fan data until the state has been cleared).

Quoted 4 lines

The driver, not the user, should take care of this. When one is reading from the status file, the driver should automatically rearm any flag that needs be. Don't the 2.4 fscpos and the 2.6 fscher drivers do it that way?

No, they don't. So what you're saying is that once an alarm has been read it should disappear if the problem is no longer there?

Quoted 11 lines

Quoted 8 lines
Quoted 3 lines
Quoted 1 line
static ssize_t set_fan_min
[..] shouldn't the value be divided by 60?
Indeed, I fixed that. I do however still seem to get it wrong
somehow: When I double the value, the fan appears to accelerate by 8*60 RPM. It might somehow be related to the ripple value but I just don't get it.
I don't know either. Did you test the 2.4 driver in similar conditions?

No, I didn't. I guess I'll compile myself a 2.4 kernel later today and try, but I can't see any related code in there which does magical calculations my driver doesn't do. I'll try anyway.

Quoted 2 lines

It's odd that the min value would affect the speed, since it supposedly is only a limit to the alarm condition, not a speed target.

According to the PDF you sent me, this really is meant to actually control the fan speed.

Quoted 11 lines

Quoted 5 lines
Quoted 1 line
Don't mask, check the value for validity instead.
For some registers (mainly watchdog_control) it's quite tricky to figure out all valid values (especially as messing with them might cause unwanted reactions such as spontaneous rebooting)...
For bit vector registers, masking might be fine. What is not OK however is to mask a *range* with 0xff instead of simply checking that the value is between 0 and 255 and return -EINVAL or similar if it isn't. It's usually better to return an error to user-space than silently drop an invalid value. Do the best you can.

Okay, I will.

Quoted 13 lines

Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
static ssize_t show_event
static ssize_t show_control
What are these? Do you have a use for them?
Quoted 7 lines
I don't *REALLY* know whether they're that useful: Event seems to report all kinds of alerts which could also be gathered from other sources, but offers a single place to see whether everything is OK. The control register reports whether the alert LED is flashing. I haven't seen that LED (yet), nor would I know how to make it flash, but I thought I'll just implement access to all the information available.

Quoted 2 lines

Any reason why control is writable? I find it strange that something called control would only return a status.

Okay, meanwhile I found the LED and it seems that the control register does not work as announced in the specs: It won't indicate whether the LED flashes (the event register does that) nor can you stop it (has to be done where the actual problem occurs, eg. fan2_state), so I removed it.

Quoted 5 lines

Quoted 1 line
And, as above, "sensors -u" complains if I remove them.
Does "sensors" complain as well? Does "sensors" display anything for these two files? If it doesn't, I guess it shows that these files are not really important and could possibly be removed.

No, sensors doesn't show anything, so should I just remove them?

Quoted 6 lines

Quoted 5 lines
Quoted 2 lines
If libsensors needs to be updated to support your new driver, a patch will be welcome as well.
I tried to stick with the interface provided by the old driver so I don't think libsensors will need to be updated.

Quoted 6 lines

Sometimes it isn't enough. For libsensors/sensors to work out of the box with a new 2.6 driver, it is required that the new driver follows the standard AND the old driver had relatively standard feature names as well. You of course cannot control that second condition. That said, if you have good results already, then I guess that the feature names were correct.

Yes, it appears so. I guess the only changes required will be to remove references to the features i removed (like rev and control).

Regards

Signature · 12 lines

Stefan Ott
http://www.desire.ch

"Give a man a fire and he's warm for a day, but set fire to him and he's
warm for the rest of his life."  -- Terry Pratchett, Jingo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050105/9b2f9498/attachment.bin

Jean Delvare

author

2005-05-19 06:25 UTC #3 lore raw

In reply to Jean Delvare

Quoted 10 lines

Quoted 6 lines
Quoted 3 lines
It's odd that the min value would affect the speed, since it supposedly is only a limit to the alarm condition, not a speed target.
According to the PDF you sent me, this really is meant to actually control the fan speed.
Wow. I'll take a look at it this evening. If this is true, then this would correspond to either an analog output or PWM output, and correspond to a file names pwm1, not fan1_min.

I confirm this. The datasheet is rather clear about this, as you said - although it doesn't mention whether analog output amplitude modulation or PWM is used, nor does it states how temperature interacts with the final speed. The fact that temperature somehow matters may explain the strange behavior you noticed.

Note that I suspect that fscher isn't correct on some points. It seems to create files named fanN_pwm (old standard) instead of pwmN (new standard), and also maps fanN_div to fan ripple registers, while these are two different things. I won't blame Reinhard Nissl too hard since I might be responsible for some of the errors, as I gave him possibly unenlightened advice while he did the work.

Signature · 2 lines

Jean Delvare
http://khali.linux-fr.org/

Jean Delvare

author

2005-05-19 06:25 UTC #4 lore raw

In reply to Jean Delvare

Quoted 5 lines

For bit vector registers, masking might be fine. What is not OK however is to mask a *range* with 0xff instead of simply checking that the value is between 0 and 255 and return -EINVAL or similar if it isn't. It's usually better to return an error to user-space than silently drop an invalid value. Do the best you can.

On second thought, for ranges, you might simply force the input value into the range instead of returning an error. This is what other drivers do.

Signature · 2 lines

Jean Delvare
http://khali.linux-fr.org/

Jean Delvare

author

2005-05-19 06:25 UTC #5 lore raw

In reply to Jean Delvare

Hi Stefan,

Quoted 3 lines

Thanks a lot for the review - I managed to clean most of the issues you mentioned. Some of them were just copied from other modules, others were my own :)

If this means that some other 2.4 or 2.6 drivers would need fixing, just tell me where and I'll fix them.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
static ssize_t show_revision(struct device *dev, char *buf)
Is there a real use for this? You could instead simply dev_info() it at module load time.
Well, the information probably isn't too relevant, but is there a reason not to allow access to it?

It's not the way you want to look at things, or you will end up with a driver way bigger than it could and should. We want to keep the drivers to a minimum size, in order to save memory space and limit the possible bugs and the maintainance effort. We should get rid of whatever isn't explicitely needed. It can still be added later if there is a real need.

Quoted 2 lines

Plus, if I remove it, "sensors -u" complains about "ERROR: Can't get feature `rev' data!"

Because libsensors knows about the feature, and "sensors -u" asks the library for all features it knows about. I'll fix the library. BTW, as long as "sensors" (non -u) doesn't show any error, it's OK.

Quoted 11 lines

Quoted 5 lines
Quoted 2 lines
static ssize_t set_temp_status
static ssize_t set_fan_status
Hmm, isn't it odd that a "status" file can be written to? What are the two bits for?
It allows you to clear the alert state. I noticed this doesn't comply with the sysfs guidelines which say alarms are read-only but it's quite handy after doing stupid things such as stopping a fan altogether (which causes an alert state for said fan and "sensors" won't show any more fan data until the state has been cleared).

The driver, not the user, should take care of this. When one is reading from the status file, the driver should automatically rearm any flag that needs be. Don't the 2.4 fscpos and the 2.6 fscher drivers do it that way?

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
static ssize_t set_fan_min
[..] shouldn't the value be divided by 60?
Indeed, I fixed that. I do however still seem to get it wrong somehow:
When I double the value, the fan appears to accelerate by 8*60 RPM. It might somehow be related to the ripple value but I just don't get it.

I don't know either. Did you test the 2.4 driver in similar conditions? It's odd that the min value would affect the speed, since it supposedly is only a limit to the alarm condition, not a speed target.

Quoted 5 lines

Quoted 1 line
Don't mask, check the value for validity instead.
For some registers (mainly watchdog_control) it's quite tricky to figure out all valid values (especially as messing with them might cause unwanted reactions such as spontaneous rebooting)...

For bit vector registers, masking might be fine. What is not OK however is to mask a *range* with 0xff instead of simply checking that the value is between 0 and 255 and return -EINVAL or similar if it isn't. It's usually better to return an error to user-space than silently drop an invalid value. Do the best you can.

Quoted 11 lines

Quoted 4 lines
Quoted 2 lines
static ssize_t show_event
static ssize_t show_control
What are these? Do you have a use for them?
I don't *REALLY* know whether they're that useful: Event seems to report all kinds of alerts which could also be gathered from other sources, but offers a single place to see whether everything is OK. The control register reports whether the alert LED is flashing. I haven't seen that LED (yet), nor would I know how to make it flash, but I thought I'll just implement access to all the information available.

Any reason why control is writable? I find it strange that something called control would only return a status.

Quoted 1 line

And, as above, "sensors -u" complains if I remove them.

Does "sensors" complain as well? Does "sensors" display anything for these two files? If it doesn't, I guess it shows that these files are not really important and could possibly be removed.

Quoted 5 lines

Quoted 2 lines
If libsensors needs to be updated to support your new driver, a patch will be welcome as well.
I tried to stick with the interface provided by the old driver so I don't think libsensors will need to be updated.

Sometimes it isn't enough. For libsensors/sensors to work out of the box with a new 2.6 driver, it is required that the new driver follows the standard AND the old driver had relatively standard feature names as well. You of course cannot control that second condition. That said, if you have good results already, then I guess that the feature names were correct.

Thanks,
--
Jean Delvare

Jean Delvare

author

2005-05-19 06:25 UTC #6 lore raw

In reply to Jean Delvare

Quoted 7 lines

Quoted 7 lines
Quoted 6 lines
It allows you to clear the alert state. I noticed this doesn't comply with the sysfs guidelines which say alarms are read-only but it's quite handy after doing stupid things such as stopping a fan altogether (which causes an alert state for said fan and "sensors" won't show any more fan data until the state has been cleared).

Quoted 7 lines

Quoted 4 lines
The driver, not the user, should take care of this. When one is reading from the status file, the driver should automatically rearm any flag that needs be. Don't the 2.4 fscpos and the 2.6 fscher drivers do it that way?
No, they don't. So what you're saying is that once an alarm has been read it should disappear if the problem is no longer there?

That's it. The policy is that alarm conditions should show at least once, and be cleared when the condition is gone. This is what most chips implement in hardware. For the chips which do not automatically clear the alarms bits when the condition is gone, the driver has to do it. See for example:
http://sosdg.org/~coywolf/lxr/source/drivers/i2c/chips/smsc47m1.c#L554

Quoted 5 lines

Quoted 2 lines
It's odd that the min value would affect the speed, since it supposedly is only a limit to the alarm condition, not a speed target.
According to the PDF you sent me, this really is meant to actually control the fan speed.

Wow. I'll take a look at it this evening. If this is true, then this would correspond to either an analog output or PWM output, and correspond to a file names pwm1, not fan1_min.

Quoted 5 lines

Quoted 3 lines
Does "sensors" complain as well? Does "sensors" display anything for these two files? If it doesn't, I guess it shows that these files are not really important and could possibly be removed.
No, sensors doesn't show anything, so should I just remove them?

I would - but you are porting the driver and will be one of the few users in the end (I don't think that the Poseidon is widely used) so you decide. Know that I will update libsensors depending on your decision so that "sensors -u" doesn't cause any error.

Quoted 2 lines

Yes, it appears so. I guess the only changes required will be to remove references to the features i removed (like rev and control).

Fine with me.
--
Jean Delvare

Stefan Ott

2005-05-19 06:25 UTC #7 lore raw

In reply to Jean Delvare

Skipped content of type multipart/mixed-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050119/e15f068b/attachment.bin

Stefan Ott

2005-05-19 06:25 UTC #8 lore raw

PATCH 1/1 fscpos driver

In reply to Jean Delvare

Hello

This patch against 2.6.11-rc1 contains a driver for fscpos sensors.

Regards
Stefan

Signed-off-by Stefan Ott <[email protected]>

drivers/i2c/chips/Kconfig +11 -0

diff -Nru linux-2.6.11-rc1-orig/drivers/i2c/chips/Kconfig linux-2.6.11-rc1/drivers/i2c/chips/Kconfig
@@ -84,6 +84,17 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called fscher.
 
+config SENSORS_FSCPOS
+	tristate "FSC Poseidon"
+	depends on I2C && EXPERIMENTAL
+	select I2C_SENSOR
+	help
+	  If you say yes here you get support for Fujitsu Siemens
+	  Computers Poseidon sensor chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called fscpos.
+
 config SENSORS_GL518SM
 	tristate "Genesys Logic GL518SM"
 	depends on I2C && EXPERIMENTAL

drivers/i2c/chips/Makefile +1 -0

diff -Nru linux-2.6.11-rc1-orig/drivers/i2c/chips/Makefile linux-2.6.11-rc1/drivers/i2c/chips/Makefile
@@ -14,6 +14,7 @@
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
 obj-$(CONFIG_SENSORS_FSCHER)	+= fscher.o
+obj-$(CONFIG_SENSORS_FSCPOS)	+= fscpos.o
 obj-$(CONFIG_SENSORS_GL518SM)	+= gl518sm.o
 obj-$(CONFIG_SENSORS_IT87)	+= it87.o
 obj-$(CONFIG_SENSORS_LM63)	+= lm63.o

drivers/i2c/chips/fscpos.c +633 -0

633 lines not on this page · raw message

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050119/00ee1086/attachment.bin

Greg KH

2005-05-19 06:25 UTC #9 lore raw

PATCH 1/1 fscpos driver

In reply to Jean Delvare

Quoted 3 lines · Stefan Ott

Hello
This patch against 2.6.11-rc1 contains a driver for fscpos sensors.

Applied, but I had to add the following patch to fix up some compiler warnings. Try to be a bit more careful next time :)

thanks,

greg k-h

------

# I2C: Fix up some build warnings in the fscpos driver. #
# Signed-off-by: Greg Kroah-Hartman <[email protected]>

drivers/i2c/chips/fscpos.c +1 -3

diff -Nru a/drivers/i2c/chips/fscpos.c b/drivers/i2c/chips/fscpos.c
@@ -51,8 +51,6 @@
  * The FSCPOS registers
  */
 
-#define DEBUG
-
 /* chip identification */
 #define FSCPOS_REG_IDENT_0		0x00
 #define FSCPOS_REG_IDENT_1		0x01

@@ -566,10 +564,10 @@
 
 	if ((jiffies - data->last_updated > 2 * HZ) ||
 			(jiffies < data->last_updated) || !data->valid) {
+		int i;
 
 		dev_dbg(&client->dev, "Starting fscpos update\n");
 
-		int i;
 		for (i = 0; i < 3; i++) {
 			data->temp_act[i] = fscpos_read_value(client,
 						FSCPOS_REG_TEMP_ACT[i]);

Stefan Ott

2005-05-19 06:25 UTC #10 lore raw

PATCH 1/1 fscpos driver

In reply to Jean Delvare

Hello

As I was using the old sensor lib I didn't realize my inN_input values were using the wrong units. Sorry for this.

Signed-off-by Stefan Ott <stefan@xxxxxxxxx>

fscpos.c renamed from fscpos.c-orig +1 -1

@@ -239,7 +239,7 @@
 }
 
 /* Volts */
-#define VOLT_FROM_REG(val, mult)	((val) * (mult) / 255)
+#define VOLT_FROM_REG(val, mult)	(((val) * (mult) / 255) * 10)
 
 static ssize_t show_volt_12(struct device *dev, char *buf)
 {

Jean Delvare

author

2005-05-19 06:25 UTC #11 lore raw

In reply to Jean Delvare

Hi Stefan,

Quoted 3 lines

Okay. Meanwhile I changed the code so that the fan status can't be written into anymore - it's automatically cleared when the speed raises above 0 after a failure.

Great.

Quoted 4 lines

However, I don't think I can do the same with the temperature and watchdog status as I can't really test them and I wouldn't want to clear a temperature alarm without knowing that the chip takes care of setting it again if required.

Why can't you test temperature? I can't see how this is different from fans.

As for the watchdog, this is something completely different and not standardized as far as I know, so I don't much care how you implement it (if at all).

Quoted 2 lines

I also renamed the fanN_min to pwmN and still haven't figured out how they work, that is, what the chip does with those values.

Supposedly, higher values result in faster speeds, lower values result in lower speeds, and possibly 0 value resuls in a stopped fan. Do you observe something significantly different?

Quoted 3 lines

Libsensors should probably know about the pwm change and that the features 'rev' and 'control' are gone. Should I create a patch or are you going to do that?

That's right, we need a patch. Either way is fine, if you propose a patch I'll apply it, if not I'll do it myself. I guess it's better if you do because you have the hardware to test it afterwards, but if you don't feel like it it fine too, just tell me.

Quoted 5 lines

For me, the remaining issues are not really relevant, thus I'd say the driver is ready now. What do you think?
I attached a patch against kernel 2.6.11-rc1, if you'd like to take a look at the code.

Agreed, driver is almost finished, patch looks great. I would have a number of comments though.

Quoted 1 line

+static u8 FSCPOS_REG_IDENT[] = { 0x00, 0x01, 0x02 };

I remember telling you that static arrays could be useful, but only in the case it actually makes the code shorter. In this specific case, you still read all three registers individually, so there is no benefit (but still the static array wastes memory and adds a level of indirection). Really only use these static arrays when it gives you the possibility to have some code in an indexed loop instead of repeating the same code several times.

Same holds for FSCPOS_REG_FAN_MIN. If you want the static array to be of any benefit over simple individual defines, you would use a loop to read these values in fscpos_update_device instead of two separate instructions.

Review

Quoted 1 line

(quoted diff) +1 -0

+	u8  watchdog[3];     /* watchdog */

These do not correspond to 3 channels like temperatures and fans do, just 3 distinct registers to control one watchdog timer. I think that using an array for this is confusing. Why not just have three named members like wdog_control, wdog_state and wdog_preset?

Review

Quoted 1 line

(quoted diff) +1 -0

+	u8  fan_min[2];      /* fan min value for rps */

Care to rename these to pwm[2] so that it's less confusing, and reword the comment accordingly?

BTW please reviwe indentation for this part, single space between type and name, and comments aligned with tabs, not spaces.

Review

Quoted 11 lines

(quoted diff) +11 -0

+static ssize_t set_pwm(struct i2c_client *client, struct fscpos_data *data,
+				const char *buf, size_t count, int nr, int reg)
+{
+	unsigned long v = REG_FROM_RPM(simple_strtoul(buf, NULL, 10)) & 0xff;
+
+	/* Range: 0..255 */
+	if (v < 0 || v > 255) {
+		dev_err(&client->dev, "fan_pwm value %ld not supported. "
+					"Must be between 0 and 15300!\n", v);
+		return -EINVAL;
+	}

I can't much see how v will be out of the 0-255 range since you did mask it with 0xff just before. Second, the message should mention "pwm" not "fan_pwm". Third, the message's range doesn't match the test.

I think you should get rid of the bitmasking which is jut confusing. Then, either keep the 0-255 range test with a valid error message, or simply force anything below 0 to 0 and anything over 255 to 255. and go with this. Either choice is fine with me.

Review

Quoted 13 lines

(quoted diff) +13 -0

+	/* Do the remaining detection unless force or force_fscpos parameter */
+	if (kind < 0) {
+		if ((fscpos_read_value(new_client, FSCPOS_REG_IDENT[0])
+			!= 0x50) /* 'P' */
+		||  (fscpos_read_value(new_client, FSCPOS_REG_IDENT[1])
+			!= 0x45) /* 'E' */
+		||  (fscpos_read_value(new_client, FSCPOS_REG_IDENT[2])
+			!= 0x47))/* 'G' */
+		{
+			err = -ENODEV;
+			goto exit_free;
+		}
+	}

You must NOT return an error on misdetection. Doing so stops the whole detection loop at i2c core level. Simply return 0. You might add a dev_dbg stating that the detection failed so that we can track problems if any arise.

Review

Quoted 1 line

(quoted diff) +1 -0
+	kind = fscpos;

Useless, as you then never use this value (and this is OK).

Review

Quoted 3 lines

(quoted diff) +3 -0

+	device_create_file(&new_client->dev, &dev_attr_volt12);
+	device_create_file(&new_client->dev, &dev_attr_volt5);
+	device_create_file(&new_client->dev, &dev_attr_voltbatt);

Huu, these have to be named in0, in1 and in2. Sorry for not noticing before. Now I understand why you did not have to make too many changes to libsensors.

Review

Quoted 1 line

(quoted diff) +1 -0

+	data->revision =  fscpos_read_value(client,FSCPOS_REG_REVISION);

Additional space after comma, please.

Review

Quoted 4 lines

(quoted diff) +4 -0

+static void reset_fan_alarm(struct i2c_client *client, int nr)
+{
+    fscpos_write_value(client, FSCPOS_REG_FAN_STATE[nr], 4);
+}

Indent with tab not spaces.

Review

Quoted 3 lines

(quoted diff) +3 -0

+MODULE_AUTHOR("Stefan Ott <[email protected]> based on work from Hermann Jung "
+    				"<[email protected]>, Frodo Looijaard <[email protected]>"
+				" and Philip Edelbrock <[email protected]>");

Ditto.

OK, please fix the problems I found, then you can send the corrected patch to Greg KH (CC the list).

Thanks,
--
Jean Delvare

Jean Delvare

author

2005-05-19 06:25 UTC #12 lore raw

In reply to Jean Delvare

Quoting myself:

Quoted 2 lines

OK, please fix the problems I found, then you can send the corrected patch to Greg KH (CC the list).

I forgot to mention:
Don't forget to add a "Signed-Off-By" line before the *inlined* patch when mailing Greg.

--
Jean Delvare

Stefan Ott

2005-05-19 06:25 UTC #13 lore raw

PATCH 1/1 fscpos driver

In reply to Jean Delvare

Hi Jean

Quoted 2 lines · Jean Delvare

Hmm, wouldn't it more efficient to multiply all "mult" constants by 10 directly?

Indeed, you're right. So here we go:

fscpos.c renamed from fscpos.c-orig +3 -3

@@ -244,19 +244,19 @@
 static ssize_t show_volt_12(struct device *dev, char *buf)
 {
 	struct fscpos_data *data = fscpos_update_device(dev);
-	return sprintf(buf, "%u\n", VOLT_FROM_REG(data->volt[0], 1420));
+	return sprintf(buf, "%u\n", VOLT_FROM_REG(data->volt[0], 14200));
 }
 
 static ssize_t show_volt_5(struct device *dev, char *buf)
 {
 	struct fscpos_data *data = fscpos_update_device(dev);
-	return sprintf(buf, "%u\n", VOLT_FROM_REG(data->volt[1], 660));
+	return sprintf(buf, "%u\n", VOLT_FROM_REG(data->volt[1], 6600));
 }
 
 static ssize_t show_volt_batt(struct device *dev, char *buf)
 {
 	struct fscpos_data *data = fscpos_update_device(dev);
-	return sprintf(buf, "%u\n", VOLT_FROM_REG(data->volt[2], 330));
+	return sprintf(buf, "%u\n", VOLT_FROM_REG(data->volt[2], 3300));
 }
 
 /* Watchdog */

Greg KH

2005-05-19 06:25 UTC #14 lore raw

PATCH 1/1 fscpos driver

In reply to Jean Delvare

Quoted 8 lines · Stefan Ott

Hi Jean
Quoted 2 lines · Jean Delvare
Hmm, wouldn't it more efficient to multiply all "mult" constants by 10 directly?
Indeed, you're right. So here we go:

Can I get a real changelog text, and a sensible Subject: and a
Signed-off-by: line so that I can apply this?

thanks,

greg k-h

Jean Delvare

author

2005-05-19 06:25 UTC #15 lore raw

PATCH 1/1 fscpos driver

In reply to Jean Delvare

Hi Stefan,

Quoted 9 lines

As I was using the old sensor lib I didn't realize my inN_input values were using the wrong units. Sorry for this.
Signed-off-by Stefan Ott <stefan@xxxxxxxxx>

fscpos.c line 245

Quoted 8 lines

fscpos.c renamed from fscpos.c-orig +1 -1

@@ -239,7 +239,7 @@
 }
 
 /* Volts */
-#define VOLT_FROM_REG(val, mult)	((val) * (mult) / 255)
+#define VOLT_FROM_REG(val, mult)	(((val) * (mult) / 255) * 10)
 
 static ssize_t show_volt_12(struct device *dev, char *buf)
 {

Hmm, wouldn't it more efficient to multiply all "mult" constants by 10 directly?

Signature · 2 lines

Jean Delvare
http://khali.linux-fr.org/

Stefan Ott

2005-05-19 06:25 UTC #16 lore raw

In reply to Jean Delvare

Hello everybody

I was wondering whether there was a specific reason, maybe some kind of policy I'm not aware of, why the fscpos driver hasn't been added to the kernel yet.

Kind regards

Signature · 5 lines

Stefan Ott
http://www.desire.ch

"Give a man a fire and he's warm for a day, but set fire to him and he's
warm for the rest of his life."  -- Terry Pratchett, Jingo

Jean Delvare

author

2005-05-19 06:25 UTC #17 lore raw

In reply to Jean Delvare

Hi Stefan,

Quoted 3 lines · Stefan Ott

I was wondering whether there was a specific reason, maybe some kind of policy I'm not aware of, why the fscpos driver hasn't been added to the kernel yet.

It actually has been, just not in Linus' tree yet. Your fscpos driver is in Andrew Morton's kernels since 2.6.11-rc2-mm1. First it needed to get some testing there, and now we are next to the 2.6.11 release so Linus is only accepting bugfixes. Soon after 2.6.11 is released (and it should be quite soon now), I'd expect Grek KH to send an i2c update to Linus with all the patches that have been in -mm for quite some time, including your fscpos driver.

--
Jean Delvare