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?
>> +}
>> +
>> 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
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?
Thanks, Giovanni.
--
Giovanni Mascellani <g.mascellani@xxxxxxxxx>
Postdoc researcher - Université Libre de Bruxelles
Attachment:
signature.asc
Description: OpenPGP digital signature
