hwmon: (f75375s) Catch some attempts to write to r/o registers
PATCH 6/6 hwmon: (f75375s) Catch some attempts to write to r/o registers
It makes no sense to attempt to manually configure the fan in auto mode, or set the duty cycle directly in closed loop mode. The corresponding registers are then read-only. If the user tries it nonetheless, error out with EBUSY instead of silently doing nothing.
--- drivers/hwmon/f75375s.c | 32 ++++++++++++++++++++++++++++++++ 1 files changed, 32 insertions(+), 0 deletions(-)
drivers/hwmon/f75375s.c +32 -0
@@ -281,6 +281,21 @@ static bool duty_mode_enabled(u8 pwm_enable) } } +static bool auto_mode_enabled (u8 pwm_enable) +{ +switch (pwm_enable) { +case 0 :/* Manual, duty mode (full speed) */ +case 1 :/* Manual, duty mode */ +case 3 :/* Manual, speed mode */ +return false; +case 2 :/* Auto, speed mode */ +case 4 :/* Auto, duty mode */ +return true; +default : +BUG (); + } +} +static ssize_t set_fan_min (struct device *dev,struct device_attribute *attr,const char *buf,size_t count) {
@@ -314,6 +329,11 @@ static ssize_t set_fan_target(struct device *dev, struct device_attribute *attr,if (err <0 )return err; +if (auto_mode_enabled (data->pwm_enable[nr])) +return -EBUSY ; +if (data->kind = f75387 &&duty_mode_enabled (data->pwm_enable[nr])) +return -EBUSY ; +mutex_lock (&data->update_lock); data->fan_target[nr] =rpm_to_reg (val);f75375_write16 (client,F75375_REG_FAN_EXP (nr), data->fan_target[nr]);
@@ -334,6 +354,10 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,if (err <0 )return err; +if (auto_mode_enabled (data->pwm_enable[nr]) || + !duty_mode_enabled (data->pwm_enable[nr])) +return -EBUSY ; +mutex_lock (&data->update_lock);/* TODO: Improve representation in the register cache for the F75387 */ data->pwm[nr] =SENSORS_LIMIT (val,0 ,255 );
@@ -819,6 +843,14 @@ static void f75375_init(struct i2c_client *client, struct f75375_data *data,set_pwm_enable_direct (client,0 , f75375s_pdata->pwm_enable[0 ]);set_pwm_enable_direct (client,1 , f75375s_pdata->pwm_enable[1 ]);for (nr =0 ; nr <2 ; nr++) { +if (auto_mode_enabled (f75375s_pdata->pwm_enable[nr]) || + !duty_mode_enabled (f75375s_pdata->pwm_enable[nr])) { +dev_err (data->hwmon_dev, +"fan %d: cannot set duty cycle in mode %d" , + nr, + f75375s_pdata->pwm_enable[nr]); +continue ; + } data->pwm[nr] =SENSORS_LIMIT (f75375s_pdata->pwm[nr],0 ,255 );if (data->kind = f75387) f75375_write16(client, F75375_REG_FAN_EXP(nr),
Signature · 7 lines
1.7.9.1 _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 37 lines · Nikolaus Schulz
It makes no sense to attempt to manually configure the fan in auto mode, or set the duty cycle directly in closed loop mode. The corresponding registers are then read-only. If the user tries it nonetheless, error out with EBUSY instead of silently doing nothing.
Signed-off-by Nikolaus Schulz <[email protected]>--- drivers/hwmon/f75375s.c | 32 ++++++++++++++++++++++++++++++++ 1 files changed, 32 insertions(+), 0 deletions(-)drivers/hwmon/f75375s.c +15
-0@@ -281,6 +281,21 @@ static bool duty_mode_enabled(u8 pwm_enable) } } +static bool auto_mode_enabled (u8 pwm_enable) +{ +switch (pwm_enable) { +case 0 :/* Manual, duty mode (full speed) */ +case 1 :/* Manual, duty mode */ +case 3 :/* Manual, speed mode */ +return false; +case 2 :/* Auto, speed mode */ +case 4 :/* Auto, duty mode */ +return true; +default : +BUG (); + } +} +static ssize_t set_fan_min (struct device *dev,struct device_attribute *attr,const char *buf,size_t count) {
drivers/hwmon/f75375s.c static ssize_t set_fan_target(struct device *dev, struct device_attribute *attr, · line 336
Quoted 8 lines
drivers/hwmon/f75375s.c +5
-0@@ -314,6 +329,11 @@ static ssize_t set_fan_target(struct device *dev, struct device_attribute *attr,if (err <0 )return err; +if (auto_mode_enabled (data->pwm_enable[nr])) +return -EBUSY ; +if (data->kind = f75387 &&duty_mode_enabled (data->pwm_enable[nr])) +return -EBUSY ; +
-EBUSY is really not appropriate here. -EINVAL would be better, though I am open to suggestions.
drivers/hwmon/f75375s.c static ssize_t set_pwm(struct device *dev, struct device_attribute *attr, · line 360
Quoted 11 lines
drivers/hwmon/f75375s.c +4
-0mutex_lock (&data->update_lock); data->fan_target[nr] =rpm_to_reg (val);f75375_write16 (client,F75375_REG_FAN_EXP (nr), data->fan_target[nr]);@@ -334,6 +354,10 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,if (err <0 )return err; +if (auto_mode_enabled (data->pwm_enable[nr]) || + !duty_mode_enabled (data->pwm_enable[nr])) +return -EBUSY ; +
Same here.
drivers/hwmon/f75375s.c static void f75375_init(struct i2c_client *client, struct f75375_data *data, · line 851
Quoted 13 lines
drivers/hwmon/f75375s.c +6
-0mutex_lock (&data->update_lock);/* TODO: Improve representation in the register cache for the F75387 */ data->pwm[nr] =SENSORS_LIMIT (val,0 ,255 );@@ -819,6 +843,14 @@ static void f75375_init(struct i2c_client *client, struct f75375_data *data,set_pwm_enable_direct (client,0 , f75375s_pdata->pwm_enable[0 ]);set_pwm_enable_direct (client,1 , f75375s_pdata->pwm_enable[1 ]);for (nr =0 ; nr <2 ; nr++) { +if (auto_mode_enabled (f75375s_pdata->pwm_enable[nr]) || + !duty_mode_enabled (f75375s_pdata->pwm_enable[nr])) { +dev_err (data->hwmon_dev, +"fan %d: cannot set duty cycle in mode %d" , + nr, + f75375s_pdata->pwm_enable[nr]);
Is this really an error ?
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 23 lines · Nikolaus Schulz
It makes no sense to attempt to manually configure the fan in auto mode, or set the duty cycle directly in closed loop mode. The corresponding registers are then read-only. If the user tries it nonetheless, error out with EBUSY instead of silently doing nothing.
Signed-off-by Nikolaus Schulz <[email protected]>--- drivers/hwmon/f75375s.c | 32 ++++++++++++++++++++++++++++++++ 1 files changed, 32 insertions(+), 0 deletions(-)drivers/hwmon/f75375s.c +5
-0@@ -281,6 +281,21 @@ static bool duty_mode_enabled(u8 pwm_enable) } } +static bool auto_mode_enabled (u8 pwm_enable) +{ +switch (pwm_enable) { +case 0 :/* Manual, duty mode (full speed) */ +case 1 :/* Manual, duty mode */
drivers/hwmon/f75375s.c static bool duty_mode_enabled(u8 pwm_enable) · line 298
Quoted 10 lines
drivers/hwmon/f75375s.c +10
-0+case 3 :/* Manual, speed mode */ +return false; +case 2 :/* Auto, speed mode */ +case 4 :/* Auto, duty mode */ +return true; +default : +BUG (); + } +} +
Same as the other one ...
return pwm_enable = 2 || pwm_enable = 4;
would be much simpler.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 41 lines · Guenter Roeck
Quoted 40 lines · Nikolaus Schulz
It makes no sense to attempt to manually configure the fan in auto mode, or set the duty cycle directly in closed loop mode. The corresponding registers are then read-only. If the user tries it nonetheless, error out with EBUSY instead of silently doing nothing.
Signed-off-by Nikolaus Schulz <[email protected]>--- drivers/hwmon/f75375s.c | 32 ++++++++++++++++++++++++++++++++ 1 files changed, 32 insertions(+), 0 deletions(-)drivers/hwmon/f75375s.c +15
-0@@ -281,6 +281,21 @@ static bool duty_mode_enabled(u8 pwm_enable) } } +static bool auto_mode_enabled (u8 pwm_enable) +{ +switch (pwm_enable) { +case 0 :/* Manual, duty mode (full speed) */ +case 1 :/* Manual, duty mode */ +case 3 :/* Manual, speed mode */ +return false; +case 2 :/* Auto, speed mode */ +case 4 :/* Auto, duty mode */ +return true; +default : +BUG (); + } +} +static ssize_t set_fan_min (struct device *dev,struct device_attribute *attr,const char *buf,size_t count) {@@ -314,6 +329,11 @@ static ssize_t set_fan_target(struct device *dev, struct device_attribute *attr,if (err <0 )return err;
Quoted 6 lines
drivers/hwmon/f75375s.c static ssize_t set_fan_target(struct device *dev, struct device_attribute *attr, · line 336
Quoted 5 lines
drivers/hwmon/f75375s.c +5
-0+if (auto_mode_enabled (data->pwm_enable[nr])) +return -EBUSY ; +if (data->kind = f75387 &&duty_mode_enabled (data->pwm_enable[nr])) +return -EBUSY ; +
-EBUSYis really not appropriate here.-EINVALwould be better, though I am open to suggestions.
I am not very happy about using EINVAL here, but if EBUSY is inappropriate, I don't have a better idea.
Quoted 7 lines
Quoted 7 lines
drivers/hwmon/f75375s.c +0
-0mutex_lock (&data->update_lock); data->fan_target[nr] =rpm_to_reg (val);f75375_write16 (client,F75375_REG_FAN_EXP (nr), data->fan_target[nr]);@@ -334,6 +354,10 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,if (err <0 )return err;
Quoted 5 lines
drivers/hwmon/f75375s.c static ssize_t set_pwm(struct device *dev, struct device_attribute *attr, · line 360
Quoted 4 lines
drivers/hwmon/f75375s.c +4
-0+if (auto_mode_enabled (data->pwm_enable[nr]) || + !duty_mode_enabled (data->pwm_enable[nr])) +return -EBUSY ; +Same here.
yes
Quoted 7 lines
Quoted 7 lines
drivers/hwmon/f75375s.c +0
-0mutex_lock (&data->update_lock);/* TODO: Improve representation in the register cache for the F75387 */ data->pwm[nr] =SENSORS_LIMIT (val,0 ,255 );@@ -819,6 +843,14 @@ static void f75375_init(struct i2c_client *client, struct f75375_data *data,set_pwm_enable_direct (client,0 , f75375s_pdata->pwm_enable[0 ]);set_pwm_enable_direct (client,1 , f75375s_pdata->pwm_enable[1 ]);for (nr =0 ; nr <2 ; nr++) {
Quoted 8 lines
drivers/hwmon/f75375s.c static void f75375_init(struct i2c_client *client, struct f75375_data *data, · line 851
Quoted 6 lines
drivers/hwmon/f75375s.c +6
-0+if (auto_mode_enabled (f75375s_pdata->pwm_enable[nr]) || + !duty_mode_enabled (f75375s_pdata->pwm_enable[nr])) { +dev_err (data->hwmon_dev, +"fan %d: cannot set duty cycle in mode %d" , + nr, + f75375s_pdata->pwm_enable[nr]);Is this really an error ?
Ah, I see. Indeed, setting automatic mode is certainly valid, the pwm value in the platform data can just be ignored in that case.
Manual "speed mode" is less obvious, as there is currently no way to configure this mode using platform data; but it might still be reasonable to assume that the platform data is sane.
Ok, I'll drop the warning.
Nikolaus
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 49 lines · Nikolaus Schulz
Quoted 41 lines · Guenter Roeck
Quoted 40 lines · Nikolaus Schulz
It makes no sense to attempt to manually configure the fan in auto mode, or set the duty cycle directly in closed loop mode. The corresponding registers are then read-only. If the user tries it nonetheless, error out with EBUSY instead of silently doing nothing.
Signed-off-by Nikolaus Schulz <[email protected]>--- drivers/hwmon/f75375s.c | 32 ++++++++++++++++++++++++++++++++ 1 files changed, 32 insertions(+), 0 deletions(-)drivers/hwmon/f75375s.c +15
-0@@ -281,6 +281,21 @@ static bool duty_mode_enabled(u8 pwm_enable) } } +static bool auto_mode_enabled (u8 pwm_enable) +{ +switch (pwm_enable) { +case 0 :/* Manual, duty mode (full speed) */ +case 1 :/* Manual, duty mode */ +case 3 :/* Manual, speed mode */ +return false; +case 2 :/* Auto, speed mode */ +case 4 :/* Auto, duty mode */ +return true; +default : +BUG (); + } +} +static ssize_t set_fan_min (struct device *dev,struct device_attribute *attr,const char *buf,size_t count) {@@ -314,6 +329,11 @@ static ssize_t set_fan_target(struct device *dev, struct device_attribute *attr,if (err <0 )return err;Quoted 6 lines
drivers/hwmon/f75375s.c static ssize_t set_fan_target(struct device *dev, struct device_attribute *attr, · line 336
Quoted 5 lines
drivers/hwmon/f75375s.c +5
-0+if (auto_mode_enabled (data->pwm_enable[nr])) +return -EBUSY ; +if (data->kind = f75387 &&duty_mode_enabled (data->pwm_enable[nr])) +return -EBUSY ; +
-EBUSYis really not appropriate here.-EINVALwould be better, though I am open to suggestions.
Quoted 3 lines
I am not very happy about using EINVAL here, but if EBUSY is inappropriate, I don't have a better idea.
EBUSY - device or resource busy
EINVAL - Invalid argument
So EBUSY is definitely wrong, and EINVAL is a better match. It would be nice if there was an "Invalid in this context", or "Invalid with the current configuration", but I am not aware of such an error code. One could think of EOPNOTSUPP, but that means "Operation not supported on transport endpoint", which doesn't seem to be appropriate either.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors