Thanks for your replies.
Let me answer all of your replies in one go:
lm75.c: will fix
checkpatch issue: I used checkfile, not checkpatch (actually I was unaware of checkpatch -f and without -f it does say it is not a unified diff and no other messages)
checkpatch -f indeed reports errors, I'll give this another stab.
lm80 comments:
2012/1/2 Guenter Roeck <guenter.roeck@xxxxxxxxxxxx>
I am aware of that. That is why I added the comment and kept the macro close to the function.
I've thought about that before writing the macro. Issue is that this code snippet will be about 16 times in this file. That does not really make things better readable and maintainable.
If something like
rv = helperfunc(client, reg1, valptr1);
if (!rv)
rv = helperfunc(client, reg2, valptr2);
if (!rv)
rv = helperfunc(client, reg3, valptr3);
is ok, I think I could come up with a small helper func and avoid that things become too wieldy and unreadable.
Is this acceptable wrt coding style.
(and maybe it has to be if (rv < 0) or so but I hope you get the idea)
Will do
Hm, yes.
For our app this is ok. I poll the device every second.
And as it stands there is not really a mechanism to detect that the device was pulled (apart from read operations to the device failing) (at least not in our system).
In our system the LM80 is with the fan unit and if the fan unit is removed also the LM80 is disconnected. I2C is of course not hot pluggable, and actually we state in the documentation that fans are not hot swappable, but we want to be able to properly deal with the situation of a user does (as irrespective of what the manual says, someone is going to do this anyway).
If you feel there is a better solution without changing the hardware interface, I'd like to hear from you as I am always eager to improve things.
Best regards, Frans
[...][...]
return sprintf(buf, "%d\n", IN_FROM_REG(data->value[nr])); \>
> +/*
> + Note: safe_lm80_read_value is a helper macro for lm80_update_device
> + It reads a value using lm80_read_value and jumps to abort
> + in case of an error.
> + Due to this jump and because it modifies the first arguement
s/arguement/argument/
This is exactly the reason why it should not be a macro: It uses two variables
> + it has to be a macro
defined outside the macro, it depends on a label defined outside the macro,
and it modifies a variable passed to it. Way too many side effects for a macro,
and just asking for trouble later on.
I am aware of that. That is why I added the comment and kept the macro close to the function.
Sure, I understand you want to limit the changes to lm80_update_device, but you'll have
to bite the bullet here. Just make it
reg = lm80_read_value(client, <reg>);
if (reg < 0) {
ret = ERR_PTR(reg);
data->valid = 0;
goto abort;
}
data-><regstore> = reg;
I don't think the dev_dbg is really needed, since the error will now be returned
to the user anyway.
I've thought about that before writing the macro. Issue is that this code snippet will be about 16 times in this file. That does not really make things better readable and maintainable.
If something like
rv = helperfunc(client, reg1, valptr1);
if (!rv)
rv = helperfunc(client, reg2, valptr2);
if (!rv)
rv = helperfunc(client, reg3, valptr3);
is ok, I think I could come up with a small helper func and avoid that things become too wieldy and unreadable.
Is this acceptable wrt coding style.
(and maybe it has to be if (rv < 0) or so but I hope you get the idea)
else statement is not needed.
> +*/
> +#define safe_lm80_read_value(var, client, reg) \
> + { \
> + status = lm80_read_value(client, reg); \
> + if (unlikely(status < 0)) { \
> + dev_dbg(dev, \
> + "LM80: Failed to read value: reg %d, error %d\n", \
> + reg, status); \
> + ret = ERR_PTR(status); \
> + data->valid = 0; \
> + goto abort; \
> + } \
> + else \
> + var = status; \
OK
{ } not needed, and at wrong location anyway (CodingStyle, chapter 3). This also duplicates
> + }
> +
> static struct lm80_data *lm80_update_device(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct lm80_data *data = ""> > + struct lm80_data *ret = data;
> int i;
> + int status;
> + int tmp1, tmp2;
>
> mutex_lock(&data->update_lock);
>
> + if (!data->valid)
> + {
> + /* last time failed: re-initialize the LM80 chip */
> + lm80_init_client(client);
> + }
2st-time initialization. One way to avoid that would be to add a second state variable
(such as "error") to cover this case. Might be worth thinking about. Also, please add
an explanation (from your followup e-mail) describing why you do this.
Will do
On a side note, this means the chip will only get (re-)initialized if polled again.
Is that ok for your application ? Not that any alternatives would be easy to implement - a cleaner
solution would be for your system to detect that the device was pulled, unload the driver if that
happens, and reload it once the device is re-inserted. At least this is how we handle it in our
systems.
Hm, yes.
For our app this is ok. I poll the device every second.
And as it stands there is not really a mechanism to detect that the device was pulled (apart from read operations to the device failing) (at least not in our system).
In our system the LM80 is with the fan unit and if the fan unit is removed also the LM80 is disconnected. I2C is of course not hot pluggable, and actually we state in the documentation that fans are not hot swappable, but we want to be able to properly deal with the situation of a user does (as irrespective of what the manual says, someone is going to do this anyway).
If you feel there is a better solution without changing the hardware interface, I'd like to hear from you as I am always eager to improve things.
Best regards, Frans
_______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
