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

["Pali Rohár" <pali@kernel.org>]

On Tuesday 23 July 2024 22:07:24 Andres Salomon wrote:
> The dell battery patch added dell_send_request_by_token_loc(), which
> encapsulates a very common pattern when SMBIOS queries are addressed
> to token->location. This sets up the function to be used throughout
> the dell laptop code, and calls it in various places allowing us to
> delete a bunch of code.
> 
> Also some very minor cleanups:
>   - mark the kbd init functions as __init
>   - don't read buffer.output unless dell_send_request() was successful.
> 
> No behavior changes, other than the delayed read of buffer.output.

This change looks good, I do not see any obvious problem there. Nice cleanup!

Reviewed-by: Pali Rohár <pali@kernel.org>

> Signed-off-by: Andres Salomon <dilinger@queued.net>
> ---
>  drivers/platform/x86/dell/dell-laptop.c | 148 ++++++++----------------
>  1 file changed, 48 insertions(+), 100 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> index aae9a95fb188..facf00c8289d 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/dell-laptop.c
> @@ -367,6 +367,25 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
>  	{ }
>  };
>  
> +/* -1 is a sentinel value, telling us to use token->value */
> +#define USE_TVAL ((u32) -1)
> +static int dell_send_request_by_token_loc(struct calling_interface_buffer *buffer,
> +					  u16 class, u16 select, int type,
> +					  u32 val)
> +{
> +	struct calling_interface_token *token;
> +
> +	token = dell_smbios_find_token(type);
> +	if (!token)
> +		return -ENODEV;
> +
> +	if (val == USE_TVAL)
> +		val = token->value;
> +
> +	dell_fill_request(buffer, token->location, val, 0, 0);
> +	return dell_send_request(buffer, class, select);
> +}
> +
>  /*
>   * Derived from information in smbios-wireless-ctl:
>   *
> @@ -909,43 +928,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;
> +	u16 pwr;
>  
> -	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);
> -
> -	return ret;
> +	pwr = power_supply_is_system_supplied() > 0 ?
> +			SELECT_TOKEN_AC : SELECT_TOKEN_BAT;
> +	return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE,
> +			pwr, BRIGHTNESS_TOKEN, bd->props.brightness);
>  }
>  
>  static int dell_get_intensity(struct backlight_device *bd)
>  {
>  	struct calling_interface_buffer buffer;
> -	struct calling_interface_token *token;
>  	int ret;
> +	u16 pwr;
>  
> -	token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
> -	if (!token)
> -		return -ENODEV;
> -
> -	dell_fill_request(&buffer, token->location, 0, 0, 0);
> -	if (power_supply_is_system_supplied() > 0)
> -		ret = dell_send_request(&buffer,
> -					CLASS_TOKEN_READ, SELECT_TOKEN_AC);
> -	else
> -		ret = dell_send_request(&buffer,
> -					CLASS_TOKEN_READ, SELECT_TOKEN_BAT);
> -
> +	pwr = power_supply_is_system_supplied() > 0 ?
> +			SELECT_TOKEN_AC : SELECT_TOKEN_BAT;
> +	ret = dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_READ, pwr,
> +			BRIGHTNESS_TOKEN, 0);
>  	if (ret == 0)
>  		ret = buffer.output[1];
>  
> @@ -1369,20 +1369,12 @@ static int kbd_set_state_safe(struct kbd_state *state, struct kbd_state *old)
>  static int kbd_set_token_bit(u8 bit)
>  {
>  	struct calling_interface_buffer buffer;
> -	struct calling_interface_token *token;
> -	int ret;
>  
>  	if (bit >= ARRAY_SIZE(kbd_tokens))
>  		return -EINVAL;
>  
> -	token = dell_smbios_find_token(kbd_tokens[bit]);
> -	if (!token)
> -		return -EINVAL;
> -
> -	dell_fill_request(&buffer, token->location, token->value, 0, 0);
> -	ret = dell_send_request(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> -
> -	return ret;
> +	return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE,
> +			SELECT_TOKEN_STD, kbd_tokens[bit], USE_TVAL);
>  }
>  
>  static int kbd_get_token_bit(u8 bit)
> @@ -1401,11 +1393,10 @@ static int kbd_get_token_bit(u8 bit)
>  
>  	dell_fill_request(&buffer, token->location, 0, 0, 0);
>  	ret = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD);
> -	val = buffer.output[1];
> -
>  	if (ret)
>  		return ret;
>  
> +	val = buffer.output[1];
>  	return (val == token->value);
>  }
>  
> @@ -1511,7 +1502,7 @@ static inline int kbd_init_info(void)
>  
>  }
>  
> -static inline void kbd_init_tokens(void)
> +static inline void __init kbd_init_tokens(void)
>  {
>  	int i;
>  
> @@ -1520,7 +1511,7 @@ static inline void kbd_init_tokens(void)
>  			kbd_token_bits |= BIT(i);
>  }
>  
> -static void kbd_init(void)
> +static void __init kbd_init(void)
>  {
>  	int ret;
>  
> @@ -2145,21 +2136,12 @@ 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;
> +	return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE,
> +			SELECT_TOKEN_STD, val, USE_TVAL);
>  }
>  
>  static struct led_classdev micmute_led_cdev = {
> @@ -2173,21 +2155,12 @@ static int mute_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_MUTE_DISABLE);
> -	else
> -		token = dell_smbios_find_token(GLOBAL_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_MUTE_DISABLE : GLOBAL_MUTE_ENABLE;
> +	return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE,
> +			SELECT_TOKEN_STD, val, USE_TVAL);
>  }
>  
>  static struct led_classdev mute_led_cdev = {
> @@ -2197,31 +2170,12 @@ static struct led_classdev mute_led_cdev = {
>  	.default_trigger = "audio-mute",
>  };
>  
> -static int dell_send_request_by_token_loc(struct calling_interface_buffer *buffer,
> -					  u16 class, u16 select, int type,
> -					  u32 val)
> -{
> -	struct calling_interface_token *token;
> -
> -	token = dell_smbios_find_token(type);
> -	if (!token)
> -		return -ENODEV;
> -
> -	/* -1 is a sentinel value, telling us to use token->value */
> -	if (val == (u32)-1)
> -		val = token->value;
> -
> -	dell_fill_request(buffer, token->location, val, 0, 0);
> -	return dell_send_request(buffer, class, select);
> -}
> -
>  static int dell_battery_set_mode(const int type)
>  {
>  	struct calling_interface_buffer buffer;
>  
> -	/* -1 means use the value from the token */
>  	return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE,
> -			SELECT_TOKEN_STD, type, -1);
> +			SELECT_TOKEN_STD, type, USE_TVAL);
>  }
>  
>  static int dell_battery_read(const int type)
> @@ -2470,7 +2424,7 @@ static void __exit dell_battery_exit(void)
>  
>  static int __init dell_init(void)
>  {
> -	struct calling_interface_token *token;
> +	struct calling_interface_buffer buffer;
>  	int max_intensity = 0;
>  	int ret;
>  
> @@ -2532,16 +2486,10 @@ static int __init dell_init(void)
>  	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
>  		return 0;
>  
> -	token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
> -	if (token) {
> -		struct calling_interface_buffer buffer;
> -
> -		dell_fill_request(&buffer, token->location, 0, 0, 0);
> -		ret = dell_send_request(&buffer,
> -					CLASS_TOKEN_READ, SELECT_TOKEN_AC);
> -		if (ret == 0)
> -			max_intensity = buffer.output[3];
> -	}
> +	ret = dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_READ,
> +			SELECT_TOKEN_AC, BRIGHTNESS_TOKEN, 0);
> +	if (ret == 0)
> +		max_intensity = buffer.output[3];
>  
>  	if (max_intensity) {
>  		struct backlight_properties props;
> -- 
> 2.39.2
> 
> 
> 
> -- 
> I'm available for contract & employment work, see:
> https://spindle.queued.net/~dilinger/resume-tech.pdf