linux-kernel - Re: [PATCH v2 2/2] platform/x86:dell-laptop: remove duplicate code w/ battery function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240816172427.yio3xnljd7n34w3b@pali>
Date: Fri, 16 Aug 2024 19:24:27 +0200
From: Pali Rohár <pali@...nel.org>
To: Andres Salomon <dilinger@...ued.net>
Cc: linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
	Matthew Garrett <mjg59@...f.ucam.org>,
	Sebastian Reichel <sre@...nel.org>,
	Hans de Goede <hdegoede@...hat.com>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	linux-pm@...r.kernel.org, Dell.Client.Kernel@...l.com
Subject: Re: [PATCH v2 2/2] platform/x86:dell-laptop: remove duplicate code
 w/ battery function

Hello, this change looks good, I have just minor comments about
improving names of variables.

On Thursday 15 August 2024 19:35:46 Andres Salomon wrote:
> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> index 8cc05f0fab91..5df0944e5bf6 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/dell-laptop.c
> @@ -935,43 +935,24 @@ static void dell_cleanup_rfkill(void)
>  static int dell_send_intensity(struct backlight_device *bd)
>  {
>  	struct calling_interface_buffer buffer;
> -	struct calling_interface_token *token;
> -	int ret;
> -
> -	token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
> -	if (!token)
> -		return -ENODEV;
> -
> -	dell_fill_request(&buffer,
> -			   token->location, bd->props.brightness, 0, 0);
> -	if (power_supply_is_system_supplied() > 0)
> -		ret = dell_send_request(&buffer,
> -					CLASS_TOKEN_WRITE, SELECT_TOKEN_AC);
> -	else
> -		ret = dell_send_request(&buffer,
> -					CLASS_TOKEN_WRITE, SELECT_TOKEN_BAT);
> +	u16 pwr;
>  
> -	return ret;
> +	pwr = power_supply_is_system_supplied() > 0 ?

I would suggest to call this variable "select" or something like that.
Name "pwr" is too generic and does not express the fact that is stores
token select id.

> +			SELECT_TOKEN_AC : SELECT_TOKEN_BAT;
> +	return dell_send_request_for_tokenid(&buffer, CLASS_TOKEN_WRITE,
> +			pwr, BRIGHTNESS_TOKEN, bd->props.brightness);
>  }
...
> @@ -2171,21 +2142,11 @@ static int micmute_led_set(struct led_classdev *led_cdev,
>  			   enum led_brightness brightness)
>  {
>  	struct calling_interface_buffer buffer;
> -	struct calling_interface_token *token;
>  	int state = brightness != LED_OFF;
> +	u32 val;
>  
> -	if (state == 0)
> -		token = dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE);
> -	else
> -		token = dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE);
> -
> -	if (!token)
> -		return -ENODEV;
> -
> -	dell_fill_request(&buffer, token->location, token->value, 0, 0);
> -	dell_send_request(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> -
> -	return 0;
> +	val = state == 0 ? GLOBAL_MIC_MUTE_DISABLE : GLOBAL_MIC_MUTE_ENABLE;

Here variable "val" is also too generic. The real meaning of this
variable is tokenid. So I would suggest to use this name as it is used
in other places in this file for storing token id.

> +	return dell_set_std_token_value(&buffer, val, USE_TVAL);
>  }