Re: [PATCH v5] platform/x86: Add driver for ACPI WMAA EC-based backlight control

[Daniel Dadap <ddadap@nvidia.com>]

On 9/2/21 6:20 PM, Thomas Weißschuh wrote:
> Hi Daniel,
>
> one more actual thing and two tiny nitpicks.
>
> On 2021-09-02T16:47-0500, Daniel Dadap wrote:
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index d12db6c316ea..0df908ef8d7c 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -113,6 +113,22 @@ config PEAQ_WMI
>>   	help
>>   	 Say Y here if you want to support WMI-based hotkeys on PEAQ 2-in-1s.
>>   
>> +config WMAA_BACKLIGHT_WMI
>> +	tristate "ACPI WMAA Backlight Driver"
>> +	depends on ACPI_WMI
>> +	depends on BACKLIGHT_CLASS_DEVICE
>> +	help
>> +	  This driver provides a sysfs backlight interface for notebook
>> +	  systems which expose the WMAA ACPI method and an associated WMI
>> +	  wrapper to drive LCD backlight levels through the system's
>> +	  Embedded Controller (EC).
> system's -> systems


"system's" is what I intended, and I believe it's correct. The meaning 
I'm going for here is "the Embedded Controller (EC) belonging to the 
system". I guess maybe there could be some confusion because earlier in 
the same sentence I refer to "systems" plural, referring to the class of 
systems that expose this backlight control scheme, and later in the 
sentence "system's" refers to something belonging to a singular member 
of this class. It would probably be more clear if I just drop the word 
"system's" here.


>
>> +
>> +	  Say Y or M here if you want to control the backlight on a notebook
>> +	  system with an EC-driven backlight using the ACPI WMAA method.
>> +
>> +	  If you choose to compile this driver as a module the module will be
>> +	  called wmaa-backlight-wmi.
>> +
>>   config XIAOMI_WMI
>>   	tristate "Xiaomi WMI key driver"
>>   	depends on ACPI_WMI
>> diff --git a/drivers/platform/x86/wmaa-backlight-wmi.c b/drivers/platform/x86/wmaa-backlight-wmi.c
>> new file mode 100644
>> index 000000000000..fa3f41302299
>> --- /dev/null
>> +++ b/drivers/platform/x86/wmaa-backlight-wmi.c
>> @@ -0,0 +1,194 @@
>> [..]
>> +static int wmaa_backlight_update_status(struct backlight_device *bd)
>> +{
>> +	struct wmi_device *wdev = bl_get_data(bd);
>> +
>> +	return wmi_call_wmaa(wdev, WMAA_METHOD_LEVEL, WMAA_MODE_SET,
>> +			     &bd->props.brightness);
>> +}
>> +
>> +static int wmaa_backlight_get_brightness(struct backlight_device *bd)
>> +{
>> +	struct wmi_device *wdev = bl_get_data(bd);
>> +	u32 level;
>> +	int ret;
>> +
>> +	ret = wmi_call_wmaa(wdev, WMAA_METHOD_LEVEL, WMAA_MODE_GET, &level);
>> +	if (ret > 0)
>> +		return -EINVAL;
>> +	else if (ret < 0)
>> +		return ret;
> Nowhere else is the return value of wmi_call_wmaa() checked explicitly for
> greater than zero and less than zero. Why here?
> At least wmaa_backlight_update_status() should have identical semantics.
> The case ret > 0 should never happen.


wmaa_backlight_update_status() has different semantics precisely because 
the .get_brightness() and .update_status() callbacks registered in the 
struct backlight_ops have different semantics. The return value of 
.get_brightness() serves dual duty, where a positive return value could 
be interpreted as a valid brightness level value, while in the case of 
.update_status() a positive return value is invalid.

I agree that wmi_call_wmaa() should never return > 0, but I wanted to 
protect against someone deciding in the future (for whatever reason) to 
add a code path that returns a positive value, without checking all of 
the callers of wmi_call_wmaa() to realize that this could cause a 
problem. There are not many callers of the function, though, and they're 
all in the same file, so perhaps this is not something worth worrying 
about. I suppose I could check for just the < 0 case, and add document 
the API of wmi_call_wmaa(), explicitly stating that the return value 
should be zero or negative errno.


>
>> +
>> +	return level;
>> +}
>> [..]
>> +
>> +
>> +#define WMAA_WMI_GUID "603E9613-EF25-4338-A3D0-C46177516DB7"
>> +
>> +static const struct wmi_device_id wmaa_backlight_wmi_id_table[] = {
>> +	{ .guid_string = WMAA_WMI_GUID },
> This could also be inlined as:
> 	{ "603E9613-EF25-4338-A3D0-C46177516DB7" },
>
> More a stylistic thing though.


I considered that when switching to MODULE_DEVICE_TABLE, since the value 
is now only used in one place, but looking at other similar drivers, it 
does seem that the most common convention is to define the GUID as a 
macro even if it's only used once. I'll leave this as-is, I think.