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