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

[Pali Rohár]

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);
>  }