Hi,
Barnabás made some good points which I missed.
See me reply inline.
On 7/5/21 2:47 AM, Barnabás Pőcze wrote:
> Hi
>
> I have added a couple comments inline.
>
>
> 2021. július 5., hétfő 0:21 keltezéssel, Luke D. Jones írta:
>
<snip>
>> +static ssize_t dgpu_disable_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + int result;
>> + u8 disable;
>> + struct asus_wmi *asus = dev_get_drvdata(dev);
>> +
>> + result = kstrtou8(buf, 10, &disable);
>
> You could use `kstrtobool()`. I think that would be better since it accepts
> 'y', 'n', etc. in addition to 0 and 1.
Good point and the same applies to patch 1/3.
>> + if (result < 0)
>> + return result;
>> +
>> + if (disable > 1 || disable < 0)
>> + return -EINVAL;
>> +
>> + asus->dgpu_disable_mode = disable;
>> + /*
>> + * The ACPI call used does not save the mode unless the call is run twice.
>> + * Once to disable, then once to check status and save - this is two code
>> + * paths in the method in the ACPI dumps.
>> + */
>> + dgpu_disable_write(asus);
>> + dgpu_disable_write(asus);
>
> Is there any reason the potential error codes are not returned?
Good question.
<snip>
>> @@ -2699,6 +2792,10 @@ static int asus_wmi_add(struct platform_device *pdev)
>> if (err)
>> goto fail_platform;
>>
>> + err = dgpu_disable_check_present(asus);
>> + if (err)
>> + goto fail_dgpu_disable;
>> +
>
> Should this really be considered a "fatal" error?
Well dgpu_disable_check_present() does already contain:
if (err == -ENODEV)
return 0;
IOW it only returns an error on unexpected errors and asus_wmi_add()
already contains a couple of other foo_present() checks which are
dealt with in the same way, so this is consistent with that and
being consistent is good, so I think this is fine.
Regards,
Hans
