Re: [PATCH v3] dell-smm-hwmon: Add support for disabling automatic BIOS fan control

[Giovanni Mascellani]

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