Signed-off-by: Guenter Roeck <[email protected]>
---
The main rationale for this cleanup is to prepare the driver for adding max6696
support.
drivers/hwmon/lm90.c | 110 +++++++++++++++++++++++++++++++++----------------
1 files changed, 74 insertions(+), 36 deletions(-)
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 760ef72..58a5605 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -387,8 +387,13 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
struct i2c_client *client = to_i2c_client(dev);
struct lm90_data *data = i2c_get_clientdata(client);
- long val = simple_strtol(buf, NULL, 10);
int nr = attr->index;
+ long val;
+ int err;
+
+ err = strict_strtol(buf, 10, &val);
+ if (err < 0)
+ return err;
/* +16 degrees offset for temp2 for the LM99 */
if (data->kind == lm99 && attr->index == 3)
@@ -442,8 +447,13 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
struct i2c_client *client = to_i2c_client(dev);
struct lm90_data *data = i2c_get_clientdata(client);
- long val = simple_strtol(buf, NULL, 10);
int nr = attr->index;
+ long val;
+ int err;
+
+ err = strict_strtol(buf, 10, &val);
+ if (err < 0)
+ return err;
/* +16 degrees offset for temp2 for the LM99 */
if (data->kind == lm99 && attr->index <= 2)
@@ -469,7 +479,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
return count;
}
-static ssize_t show_temphyst(struct device *dev, struct device_attribute *devattr,
+static ssize_t show_temphyst(struct device *dev,
+ struct device_attribute *devattr,
char *buf)
{
struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
@@ -495,9 +506,14 @@ static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy,
{
struct i2c_client *client = to_i2c_client(dev);
struct lm90_data *data = i2c_get_clientdata(client);
- long val = simple_strtol(buf, NULL, 10);
+ long val;
+ int err;
int temp;
+ err = strict_strtol(buf, 10, &val);
+ if (err < 0)
+ return err;
+
mutex_lock(&data->update_lock);
if (data->kind == adt7461)
temp = temp_from_u8_adt7461(data, data->temp8[2]);
@@ -600,7 +616,12 @@ static ssize_t set_pec(struct device *dev, struct device_attribute *dummy,
const char *buf, size_t count)
{
struct i2c_client *client = to_i2c_client(dev);
- long val = simple_strtol(buf, NULL, 10);
+ long val;
+ int err;
+
+ err = strict_strtol(buf, 10, &val);
+ if (err < 0)
+ return err;
switch (val) {
case 0:
@@ -622,8 +643,10 @@ static DEVICE_ATTR(pec, S_IWUSR | S_IRUGO, show_pec, set_pec);
* Real code
*/
-/* The ADM1032 supports PEC but not on write byte transactions, so we need
- to explicitly ask for a transaction without PEC. */
+/*
+ * The ADM1032 supports PEC but not on write byte transactions, so we need
+ * to explicitly ask for a transaction without PEC.
+ */
static inline s32 adm1032_write_byte(struct i2c_client *client, u8 value)
{
return i2c_smbus_xfer(client->adapter, client->addr,
@@ -631,20 +654,22 @@ static inline s32 adm1032_write_byte(struct i2c_client *client, u8 value)
I2C_SMBUS_WRITE, value, I2C_SMBUS_BYTE, NULL);
}
-/* It is assumed that client->update_lock is held (unless we are in
- detection or initialization steps). This matters when PEC is enabled,
- because we don't want the address pointer to change between the write
- byte and the read byte transactions. */
-static int lm90_read_reg(struct i2c_client* client, u8 reg, u8 *value)
+/*
+ * It is assumed that client->update_lock is held (unless we are in
+ * detection or initialization steps). This matters when PEC is enabled,
+ * because we don't want the address pointer to change between the write
+ * byte and the read byte transactions.
+ */
+static int lm90_read_reg(struct i2c_client *client, u8 reg, u8 *value)
{
int err;
- if (client->flags & I2C_CLIENT_PEC) {
- err = adm1032_write_byte(client, reg);
- if (err >= 0)
- err = i2c_smbus_read_byte(client);
- } else
- err = i2c_smbus_read_byte_data(client, reg);
+ if (client->flags & I2C_CLIENT_PEC) {
+ err = adm1032_write_byte(client, reg);
+ if (err >= 0)
+ err = i2c_smbus_read_byte(client);
+ } else
+ err = i2c_smbus_read_byte_data(client, reg);
if (err < 0) {
dev_warn(&client->dev, "Register %#02x read failed (%d)\n",
@@ -669,14 +694,21 @@ static int lm90_detect(struct i2c_client *new_client,
return -ENODEV;
/* detection and identification */
- if ((man_id = i2c_smbus_read_byte_data(new_client,
- LM90_REG_R_MAN_ID)) < 0
- || (chip_id = i2c_smbus_read_byte_data(new_client,
- LM90_REG_R_CHIP_ID)) < 0
- || (reg_config1 = i2c_smbus_read_byte_data(new_client,
- LM90_REG_R_CONFIG1)) < 0
- || (reg_convrate = i2c_smbus_read_byte_data(new_client,
- LM90_REG_R_CONVRATE)) < 0)
+ man_id = i2c_smbus_read_byte_data(new_client, LM90_REG_R_MAN_ID);
+ if (man_id < 0)
+ return -ENODEV;
+
+ chip_id = i2c_smbus_read_byte_data(new_client, LM90_REG_R_CHIP_ID);
+ if (chip_id < 0)
+ return -ENODEV;
+
+ reg_config1 = i2c_smbus_read_byte_data(new_client, LM90_REG_R_CONFIG1);
+ if (reg_config1 < 0)
+ return -ENODEV;
+
+ reg_convrate = i2c_smbus_read_byte_data(new_client,
+ LM90_REG_R_CONVRATE);
+ if (reg_convrate < 0)
return -ENODEV;
if ((address == 0x4C || address == 0x4D)
@@ -826,16 +858,18 @@ static int lm90_probe(struct i2c_client *new_client,
lm90_init_client(new_client);
/* Register sysfs hooks */
- if ((err = sysfs_create_group(&new_client->dev.kobj, &lm90_group)))
+ err = sysfs_create_group(&new_client->dev.kobj, &lm90_group);
+ if (err)
goto exit_free;
if (new_client->flags & I2C_CLIENT_PEC) {
- if ((err = device_create_file(&new_client->dev,
- &dev_attr_pec)))
+ err = device_create_file(&new_client->dev, &dev_attr_pec);
+ if (err)
goto exit_remove_files;
}
if (data->kind != max6657 && data->kind != max6646) {
- if ((err = device_create_file(&new_client->dev,
- &sensor_dev_attr_temp2_offset.dev_attr)))
+ err = device_create_file(&new_client->dev,
+ &sensor_dev_attr_temp2_offset.dev_attr);
+ if (err)
goto exit_remove_files;
}
@@ -883,9 +917,8 @@ static void lm90_init_client(struct i2c_client *client)
* 0.125 degree resolution) and range (0x08, extend range
* to -64 degree) mode for the remote temperature sensor.
*/
- if (data->kind == max6680) {
+ if (data->kind == max6680)
config |= 0x18;
- }
config &= 0xBF; /* run */
if (config != data->config_orig) /* Only write if changed */
@@ -961,9 +994,14 @@ static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value)
* we have to read the low byte again, and now we believe we have a
* correct reading.
*/
- if ((err = lm90_read_reg(client, regh, &oldh))
- || (err = lm90_read_reg(client, regl, &l))
- || (err = lm90_read_reg(client, regh, &newh)))
+ err = lm90_read_reg(client, regh, &oldh);
+ if (err)
+ return err;
+ err = lm90_read_reg(client, regl, &l);
+ if (err)
+ return err;
+ err = lm90_read_reg(client, regh, &newh);
+ if (err)
return err;
if (oldh != newh) {
err = lm90_read_reg(client, regl, &l);
--
1.7.0.87.g0901d
Hi Guenter,
On Thu, 26 Aug 2010 08:54:36 -0700, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> The main rationale for this cleanup is to prepare the driver for adding max6696
> support.
I'm fine with mostly anything, except...
> drivers/hwmon/lm90.c | 110 +++++++++++++++++++++++++++++++++----------------
> 1 files changed, 74 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 760ef72..58a5605 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -387,8 +387,13 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> struct i2c_client *client = to_i2c_client(dev);
> struct lm90_data *data = i2c_get_clientdata(client);
> - long val = simple_strtol(buf, NULL, 10);
> int nr = attr->index;
> + long val;
> + int err;
> +
> + err = strict_strtol(buf, 10, &val);
> + if (err < 0)
> + return err;
>
> /* +16 degrees offset for temp2 for the LM99 */
> if (data->kind == lm99 && attr->index == 3)
> @@ -442,8 +447,13 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> struct i2c_client *client = to_i2c_client(dev);
> struct lm90_data *data = i2c_get_clientdata(client);
> - long val = simple_strtol(buf, NULL, 10);
> int nr = attr->index;
> + long val;
> + int err;
> +
> + err = strict_strtol(buf, 10, &val);
> + if (err < 0)
> + return err;
>
> /* +16 degrees offset for temp2 for the LM99 */
> if (data->kind == lm99 && attr->index <= 2)
> @@ -469,7 +479,8 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> return count;
> }
>
> -static ssize_t show_temphyst(struct device *dev, struct device_attribute *devattr,
> +static ssize_t show_temphyst(struct device *dev,
> + struct device_attribute *devattr,
> char *buf)
> {
> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> @@ -495,9 +506,14 @@ static ssize_t set_temphyst(struct device *dev, struct device_attribute *dummy,
> {
> struct i2c_client *client = to_i2c_client(dev);
> struct lm90_data *data = i2c_get_clientdata(client);
> - long val = simple_strtol(buf, NULL, 10);
> + long val;
> + int err;
> int temp;
>
> + err = strict_strtol(buf, 10, &val);
> + if (err < 0)
> + return err;
> +
> mutex_lock(&data->update_lock);
> if (data->kind == adt7461)
> temp = temp_from_u8_adt7461(data, data->temp8[2]);
> @@ -600,7 +616,12 @@ static ssize_t set_pec(struct device *dev, struct device_attribute *dummy,
> const char *buf, size_t count)
> {
> struct i2c_client *client = to_i2c_client(dev);
> - long val = simple_strtol(buf, NULL, 10);
> + long val;
> + int err;
> +
> + err = strict_strtol(buf, 10, &val);
> + if (err < 0)
> + return err;
>
> switch (val) {
> case 0:
> @@ -622,8 +643,10 @@ static DEVICE_ATTR(pec, S_IWUSR | S_IRUGO, show_pec, set_pec);
> * Real code
> */
>
> -/* The ADM1032 supports PEC but not on write byte transactions, so we need
> - to explicitly ask for a transaction without PEC. */
> +/*
> + * The ADM1032 supports PEC but not on write byte transactions, so we need
> + * to explicitly ask for a transaction without PEC.
> + */
> static inline s32 adm1032_write_byte(struct i2c_client *client, u8 value)
> {
> return i2c_smbus_xfer(client->adapter, client->addr,
> @@ -631,20 +654,22 @@ static inline s32 adm1032_write_byte(struct i2c_client *client, u8 value)
> I2C_SMBUS_WRITE, value, I2C_SMBUS_BYTE, NULL);
> }
>
> -/* It is assumed that client->update_lock is held (unless we are in
> - detection or initialization steps). This matters when PEC is enabled,
> - because we don't want the address pointer to change between the write
> - byte and the read byte transactions. */
> -static int lm90_read_reg(struct i2c_client* client, u8 reg, u8 *value)
> +/*
> + * It is assumed that client->update_lock is held (unless we are in
> + * detection or initialization steps). This matters when PEC is enabled,
> + * because we don't want the address pointer to change between the write
> + * byte and the read byte transactions.
> + */
> +static int lm90_read_reg(struct i2c_client *client, u8 reg, u8 *value)
> {
> int err;
>
> - if (client->flags & I2C_CLIENT_PEC) {
> - err = adm1032_write_byte(client, reg);
> - if (err >= 0)
> - err = i2c_smbus_read_byte(client);
> - } else
> - err = i2c_smbus_read_byte_data(client, reg);
> + if (client->flags & I2C_CLIENT_PEC) {
> + err = adm1032_write_byte(client, reg);
> + if (err >= 0)
> + err = i2c_smbus_read_byte(client);
> + } else
> + err = i2c_smbus_read_byte_data(client, reg);
>
> if (err < 0) {
> dev_warn(&client->dev, "Register %#02x read failed (%d)\n",
> @@ -669,14 +694,21 @@ static int lm90_detect(struct i2c_client *new_client,
> return -ENODEV;
>
> /* detection and identification */
> - if ((man_id = i2c_smbus_read_byte_data(new_client,
> - LM90_REG_R_MAN_ID)) < 0
> - || (chip_id = i2c_smbus_read_byte_data(new_client,
> - LM90_REG_R_CHIP_ID)) < 0
> - || (reg_config1 = i2c_smbus_read_byte_data(new_client,
> - LM90_REG_R_CONFIG1)) < 0
> - || (reg_convrate = i2c_smbus_read_byte_data(new_client,
> - LM90_REG_R_CONVRATE)) < 0)
> + man_id = i2c_smbus_read_byte_data(new_client, LM90_REG_R_MAN_ID);
> + if (man_id < 0)
> + return -ENODEV;
> +
> + chip_id = i2c_smbus_read_byte_data(new_client, LM90_REG_R_CHIP_ID);
> + if (chip_id < 0)
> + return -ENODEV;
> +
> + reg_config1 = i2c_smbus_read_byte_data(new_client, LM90_REG_R_CONFIG1);
> + if (reg_config1 < 0)
> + return -ENODEV;
> +
> + reg_convrate = i2c_smbus_read_byte_data(new_client,
> + LM90_REG_R_CONVRATE);
> + if (reg_convrate < 0)
> return -ENODEV;
... this. I think this check should be relaxed a bit, cascaded error
checking is done in many drivers and I don't think this is anything to
worry about.
No need to resend, I've just dropped the two chunks I don't like, and
applied the resulting patch. Thanks!
>
> if ((address == 0x4C || address == 0x4D)
> @@ -826,16 +858,18 @@ static int lm90_probe(struct i2c_client *new_client,
> lm90_init_client(new_client);
>
> /* Register sysfs hooks */
> - if ((err = sysfs_create_group(&new_client->dev.kobj, &lm90_group)))
> + err = sysfs_create_group(&new_client->dev.kobj, &lm90_group);
> + if (err)
> goto exit_free;
> if (new_client->flags & I2C_CLIENT_PEC) {
> - if ((err = device_create_file(&new_client->dev,
> - &dev_attr_pec)))
> + err = device_create_file(&new_client->dev, &dev_attr_pec);
> + if (err)
> goto exit_remove_files;
> }
> if (data->kind != max6657 && data->kind != max6646) {
> - if ((err = device_create_file(&new_client->dev,
> - &sensor_dev_attr_temp2_offset.dev_attr)))
> + err = device_create_file(&new_client->dev,
> + &sensor_dev_attr_temp2_offset.dev_attr);
> + if (err)
> goto exit_remove_files;
> }
>
> @@ -883,9 +917,8 @@ static void lm90_init_client(struct i2c_client *client)
> * 0.125 degree resolution) and range (0x08, extend range
> * to -64 degree) mode for the remote temperature sensor.
> */
> - if (data->kind == max6680) {
> + if (data->kind == max6680)
> config |= 0x18;
> - }
>
> config &= 0xBF; /* run */
> if (config != data->config_orig) /* Only write if changed */
> @@ -961,9 +994,14 @@ static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value)
> * we have to read the low byte again, and now we believe we have a
> * correct reading.
> */
> - if ((err = lm90_read_reg(client, regh, &oldh))
> - || (err = lm90_read_reg(client, regl, &l))
> - || (err = lm90_read_reg(client, regh, &newh)))
> + err = lm90_read_reg(client, regh, &oldh);
> + if (err)
> + return err;
> + err = lm90_read_reg(client, regl, &l);
> + if (err)
> + return err;
> + err = lm90_read_reg(client, regh, &newh);
> + if (err)
> return err;
> if (oldh != newh) {
> err = lm90_read_reg(client, regl, &l);
--
Jean Delvare
On Fri, Aug 27, 2010 at 07:45:23AM -0400, Jean Delvare wrote:
> Hi Guenter,
>
> On Thu, 26 Aug 2010 08:54:36 -0700, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <[email protected]>
> > ---
> > The main rationale for this cleanup is to prepare the driver for adding max6696
> > support.
>
> I'm fine with mostly anything, except...
>
[...]
> > /* detection and identification */
> > - if ((man_id = i2c_smbus_read_byte_data(new_client,
> > - LM90_REG_R_MAN_ID)) < 0
> > - || (chip_id = i2c_smbus_read_byte_data(new_client,
> > - LM90_REG_R_CHIP_ID)) < 0
> > - || (reg_config1 = i2c_smbus_read_byte_data(new_client,
> > - LM90_REG_R_CONFIG1)) < 0
> > - || (reg_convrate = i2c_smbus_read_byte_data(new_client,
> > - LM90_REG_R_CONVRATE)) < 0)
> > + man_id = i2c_smbus_read_byte_data(new_client, LM90_REG_R_MAN_ID);
> > + if (man_id < 0)
> > + return -ENODEV;
> > +
> > + chip_id = i2c_smbus_read_byte_data(new_client, LM90_REG_R_CHIP_ID);
> > + if (chip_id < 0)
> > + return -ENODEV;
> > +
> > + reg_config1 = i2c_smbus_read_byte_data(new_client, LM90_REG_R_CONFIG1);
> > + if (reg_config1 < 0)
> > + return -ENODEV;
> > +
> > + reg_convrate = i2c_smbus_read_byte_data(new_client,
> > + LM90_REG_R_CONVRATE);
> > + if (reg_convrate < 0)
> > return -ENODEV;
>
> ... this. I think this check should be relaxed a bit, cascaded error
> checking is done in many drivers and I don't think this is anything to
> worry about.
>
I agree. I struggled with that myself when I made the changes, but let checkpatch win.
> No need to resend, I've just dropped the two chunks I don't like, and
> applied the resulting patch. Thanks!
>
Great, thanks.
Next question: lm90_update_device() currently does not return any errors.
In recent drivers, we pass i2c read errors up to userland. Before I introduce
the max6696 changes, does it make sense to add error checking/return
into the driver, similar to what I have done in the smm665 and jc42 drivers ?
Guenter
Hi Guenter,
On Fri, 27 Aug 2010 06:49:26 -0700, Guenter Roeck wrote:
> Next question: lm90_update_device() currently does not return any errors.
> In recent drivers, we pass i2c read errors up to userland. Before I introduce
> the max6696 changes, does it make sense to add error checking/return
> into the driver, similar to what I have done in the smm665 and jc42 drivers ?
So far, most hwmon driver authors decided to ignore such errors, or
limited their handling to logging the issue, mainly because the caching
mechanism makes handling of such errors tough. Now I admit that the
approach you took in the jc42 driver is interesting. I never considered
having a single error value being returned by the update function the
way you did.
This has the obvious drawback that transient I/O errors cause _all_
sensor values to be unavailable, which is discussable, especially for a
device with many features. It's hard to justify that all values of a
full-featured hardware monitoring chip could be unavailable because,
for example, one of the temperature sensors is unreliable. So this
approach is fine for your small jc42 driver, but I don't think it can be
generalized.
In the general case, I think I am fine with pretty much anything which
doesn't plain ignore error codes (as many drivers still do...) and
doesn't block all readings on transient errors. This can mean returning
0 on error, or returning the previous last known value (definitely
acceptable for transient errors, but not so for long-standing ones),
with or without logging. Or if you really want to pass error codes down
to user-space, I think you have to rework the update() function and the
per-device data structure altogether, to be able to store error codes
in the data structure.
A different (and complementary) approach is to repeat the failing
command and see if it helps. The w83l785ts driver does exactly this. If
we want to generalize this, it would probably make sense to implement
it at the the i2c-core level (i.e. add a "retries" i2c_client
attribute.)
I admit I have been ignoring the issue mainly so far, because it's not
a big problem in practice (except on one board with the w83l785ts
driver, thus the extra code in that driver), so adding complex or
invasive code to deal with it isn't too appealing.
--
Jean Delvare
On Fri, Aug 27, 2010 at 11:24:03AM -0400, Jean Delvare wrote:
Hi Jean,
> Hi Guenter,
>
> On Fri, 27 Aug 2010 06:49:26 -0700, Guenter Roeck wrote:
> > Next question: lm90_update_device() currently does not return any errors.
> > In recent drivers, we pass i2c read errors up to userland. Before I introduce
> > the max6696 changes, does it make sense to add error checking/return
> > into the driver, similar to what I have done in the smm665 and jc42 drivers ?
>
> So far, most hwmon driver authors decided to ignore such errors, or
> limited their handling to logging the issue, mainly because the caching
> mechanism makes handling of such errors tough. Now I admit that the
> approach you took in the jc42 driver is interesting. I never considered
> having a single error value being returned by the update function the
> way you did.
>
> This has the obvious drawback that transient I/O errors cause _all_
> sensor values to be unavailable, which is discussable, especially for a
> device with many features. It's hard to justify that all values of a
> full-featured hardware monitoring chip could be unavailable because,
> for example, one of the temperature sensors is unreliable. So this
> approach is fine for your small jc42 driver, but I don't think it can be
> generalized.
>
On the plus side, though, a transient failure only causes a single read
operation to fail, since I don't update the timestamp nor the valid flag
in the error case. As a result, the next read will again try to update
all values. So it isn't really that bad. Only real drawback of my approach
is that a transient read failure on one sensor register will likely be
reported while trying to read data for another sensor.
Of course, you are right that a permanent error on a single register will
cause all sensor read operations to fail, which isn't really desirable.
I have no idea if that can happen in the real world, though. Seems to be
unlikely that a failing sensor would cause an I2C operation failure.
But who knows - maybe it does happen with some chips.
> In the general case, I think I am fine with pretty much anything which
> doesn't plain ignore error codes (as many drivers still do...) and
> doesn't block all readings on transient errors. This can mean returning
> 0 on error, or returning the previous last known value (definitely
> acceptable for transient errors, but not so for long-standing ones),
Basic reason for returning errors in the first place was that I was asked
to do so in review feedback for one of my drivers - specifically, that I
should not drop errors. So we would need some clear(er) guidelines
for new drivers if we want to go along that path.
> with or without logging. Or if you really want to pass error codes down
> to user-space, I think you have to rework the update() function and the
> per-device data structure altogether, to be able to store error codes
> in the data structure.
>
Seems to be a bit excessive, and it doesn't seem to be worth the effort
and added complexity.
> A different (and complementary) approach is to repeat the failing
> command and see if it helps. The w83l785ts driver does exactly this. If
> we want to generalize this, it would probably make sense to implement
> it at the the i2c-core level (i.e. add a "retries" i2c_client
> attribute.)
>
Still doesn't solve the permanent error case, though. Question remains, then,
if it is likely that a single i2c register would return a permanent error
while others still work.
> I admit I have been ignoring the issue mainly so far, because it's not
> a big problem in practice (except on one board with the w83l785ts
> driver, thus the extra code in that driver), so adding complex or
> invasive code to deal with it isn't too appealing.
>
I'll take that as a hint and won't make any changes to lm90 driver error
handling.
Guenter
On Fri, 27 Aug 2010 09:48:33 -0700, Guenter Roeck wrote:
> On Fri, Aug 27, 2010 at 11:24:03AM -0400, Jean Delvare wrote:
> Hi Jean,
>
> > Hi Guenter,
> >
> > On Fri, 27 Aug 2010 06:49:26 -0700, Guenter Roeck wrote:
> > > Next question: lm90_update_device() currently does not return any errors.
> > > In recent drivers, we pass i2c read errors up to userland. Before I introduce
> > > the max6696 changes, does it make sense to add error checking/return
> > > into the driver, similar to what I have done in the smm665 and jc42 drivers ?
> >
> > So far, most hwmon driver authors decided to ignore such errors, or
> > limited their handling to logging the issue, mainly because the caching
> > mechanism makes handling of such errors tough. Now I admit that the
> > approach you took in the jc42 driver is interesting. I never considered
> > having a single error value being returned by the update function the
> > way you did.
> >
> > This has the obvious drawback that transient I/O errors cause _all_
> > sensor values to be unavailable, which is discussable, especially for a
> > device with many features. It's hard to justify that all values of a
> > full-featured hardware monitoring chip could be unavailable because,
> > for example, one of the temperature sensors is unreliable. So this
> > approach is fine for your small jc42 driver, but I don't think it can be
> > generalized.
>
> On the plus side, though, a transient failure only causes a single read
> operation to fail, since I don't update the timestamp nor the valid flag
> in the error case. As a result, the next read will again try to update
> all values. So it isn't really that bad. Only real drawback of my approach
> is that a transient read failure on one sensor register will likely be
> reported while trying to read data for another sensor.
Good point, I had missed this implementation detail. But this cuts both
ways: if there are too many transient errors (like the W83L785TS-S had)
the update function may keep failing forever, even though each value
could have been updated at some point in time.
> Of course, you are right that a permanent error on a single register will
> cause all sensor read operations to fail, which isn't really desirable.
> I have no idea if that can happen in the real world, though. Seems to be
> unlikely that a failing sensor would cause an I2C operation failure.
> But who knows - maybe it does happen with some chips.
In all honesty, I can't remember any such device. In general, failing
sensors are reported through data flags, not low-level errors. The only
two drivers I know which needed special error handling are w83l785ts
and thinkpad_acpi - the latter isn't I2C-based so there's no register
caching needed.
> > In the general case, I think I am fine with pretty much anything which
> > doesn't plain ignore error codes (as many drivers still do...) and
> > doesn't block all readings on transient errors. This can mean returning
> > 0 on error, or returning the previous last known value (definitely
> > acceptable for transient errors, but not so for long-standing ones),
>
> Basic reason for returning errors in the first place was that I was asked
> to do so in review feedback for one of my drivers - specifically, that I
> should not drop errors. So we would need some clear(er) guidelines
> for new drivers if we want to go along that path.
I don't remember that discussion, but I think the only thing we really
want to avoid is negative errno values being returned to user-space
silently as negative (or large positive) temperature or voltage values.
As long as this doesn't happen, I think we are on the safe side.
> > with or without logging. Or if you really want to pass error codes down
> > to user-space, I think you have to rework the update() function and the
> > per-device data structure altogether, to be able to store error codes
> > in the data structure.
>
> Seems to be a bit excessive, and it doesn't seem to be worth the effort
> and added complexity.
I tend to agree. Which is why nobody did it so far (or maybe once in one
rare driver, can't remember.)
But at least this is the "best" way to handle errors, in that it
doesn't introduce a policy at driver level. Errors are reported exactly
as if no register value caching was done.
> > A different (and complementary) approach is to repeat the failing
> > command and see if it helps. The w83l785ts driver does exactly this. If
> > we want to generalize this, it would probably make sense to implement
> > it at the the i2c-core level (i.e. add a "retries" i2c_client
> > attribute.)
> >
> Still doesn't solve the permanent error case, though. Question remains, then,
> if it is likely that a single i2c register would return a permanent error
> while others still work.
It could in specific cases. Think of a new device which is a
stripped-down version of one we already support. One can forcibly bind
the driver to the new device, but maybe the device will return I/O
errors on non-existent features. Obviously this is a transient
situation though, as proper support for the new device would have to be
added at some point anyway.
For a device we properly support, no, I can't imagine a permanent error
other than global.
> > I admit I have been ignoring the issue mainly so far, because it's not
> > a big problem in practice (except on one board with the w83l785ts
> > driver, thus the extra code in that driver), so adding complex or
> > invasive code to deal with it isn't too appealing.
>
> I'll take that as a hint and won't make any changes to lm90 driver error
> handling.
If you have other, more important things on your to-do list, then
that's the right thing to do ;)
--
Jean Delvare