On 11/17/19 12:02 AM, Giovanni Mascellani wrote:
Hi,
Il 16/11/19 23:08, Guenter Roeck ha scritto:
+ mutex_lock(&i8k_mutex);
+ err = i8k_enable_fan_auto_mode(enable);
+ mutex_unlock(&i8k_mutex);
+
+ return err ? -EIO : count;
Why override the error code ? i8k_enable_fan_auto_mode()
can return -EINVAL.
I can see that the rest of the driver has the same bad habit,
but that is not a reason to continue it.
Ok, I thought it was the appropriate thing to do just because it was
done elsewhere. If it's not a good idea, do you think a patch removing
the other instances of this construct would be appropriate?
In general it is never a good idea to override error codes. "I have seen
it elsewhere" is vener a good argument - you'll find examples for everything
in the Linux kernel.
As for fixing up the other instances in this driver, sure, if you feel like
it, but that would have to be a separate patch.
+}
+
static SENSOR_DEVICE_ATTR_RO(temp1_input, i8k_hwmon_temp, 0);
static SENSOR_DEVICE_ATTR_RO(temp1_label, i8k_hwmon_temp_label, 0);
static SENSOR_DEVICE_ATTR_RO(temp2_input, i8k_hwmon_temp, 1);
@@ -749,12 +794,15 @@ static SENSOR_DEVICE_ATTR_RO(temp10_label,
i8k_hwmon_temp_label, 9);
static SENSOR_DEVICE_ATTR_RO(fan1_input, i8k_hwmon_fan, 0);
static SENSOR_DEVICE_ATTR_RO(fan1_label, i8k_hwmon_fan_label, 0);
static SENSOR_DEVICE_ATTR_RW(pwm1, i8k_hwmon_pwm, 0);
+static SENSOR_DEVICE_ATTR_WO(pwm1_enable, i8k_hwmon_pwm_enable, 0);
static SENSOR_DEVICE_ATTR_RO(fan2_input, i8k_hwmon_fan, 1);
static SENSOR_DEVICE_ATTR_RO(fan2_label, i8k_hwmon_fan_label, 1);
static SENSOR_DEVICE_ATTR_RW(pwm2, i8k_hwmon_pwm, 1);
+static SENSOR_DEVICE_ATTR_WO(pwm2_enable, i8k_hwmon_pwm_enable, 0);
static SENSOR_DEVICE_ATTR_RO(fan3_input, i8k_hwmon_fan, 2);
static SENSOR_DEVICE_ATTR_RO(fan3_label, i8k_hwmon_fan_label, 2);
static SENSOR_DEVICE_ATTR_RW(pwm3, i8k_hwmon_pwm, 2);
+static SENSOR_DEVICE_ATTR_WO(pwm3_enable, i8k_hwmon_pwm_enable, 0);
Having three attributes do all the same is not very valuable.
I would suggest to stick with pwm1_enable and document that it applies
to all pwm channels.
I had no idea what is the convention here. No problem changing this thing.
@@ -1200,6 +1291,14 @@ static int __init i8k_probe(void)
i8k_fan_max = fan_max ? : I8K_FAN_HIGH; /* Must not be 0 */
i8k_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
+ fan_control = dmi_first_match(i8k_whitelist_fan_control);
+ if (fan_control && fan_control->driver_data) {
+ const struct i8k_fan_control_data *fan_control_data =
fan_control->driver_data;
+ manual_fan = fan_control_data->manual_fan;
+ auto_fan = fan_control_data->auto_fan;
+ pr_info("enabling experimental BIOS fan control support\n");
That isn't entirely accurate. What this enables is the ability
to select automatic or manual fan control.
Hmm, it sounds right to me: there is a feature which is "BIOS fan
control" and this driver can "support" it or not, i.e., be aware of it
and interact with it or not. And all of this is "experimental". The
"experimental" is fine, and I understand that those involved in this
exchange are aware what the message means. It does, however, not help
others not involved.
wording seems to capture this to me. However, no problem with changing
it. How would "enabling support for setting automatic/manual fan
control" work? Can you suggest a wording?
"enabling experimental support for ... " sounds good to me.
Thanks,
Guenter
Thanks, Giovanni.
