[PATCH] platform/x86: dell-laptop: merge fill_request and send_request

[PATCH] platform/x86: dell-laptop: merge fill_request and send_request

[Laszlo Toth]

Merged fill_request and send_request as they're used in pairs anyway so
it's easier to check and harder to change the request values by mistake.

In a later commit the parameter count can be reduced.

Signed-off-by: Laszlo Toth <laszlth@gmail.com>
---
 drivers/platform/x86/dell-laptop.c | 117 ++++++++++++++++++++-----------------
 1 file changed, 65 insertions(+), 52 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index a37cff9..30e6987 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -321,20 +321,17 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
 	{ }
 };
 
-static void dell_fill_request(struct calling_interface_buffer *buffer,
-			       u32 arg0, u32 arg1, u32 arg2, u32 arg3)
+static int dell_send_request(struct calling_interface_buffer *buffer,
+			     u16 class, u16 select,
+			     u32 arg0, u32 arg1, u32 arg2, u32 arg3)
 {
+	int ret;
+
 	memset(buffer, 0, sizeof(struct calling_interface_buffer));
 	buffer->input[0] = arg0;
 	buffer->input[1] = arg1;
 	buffer->input[2] = arg2;
 	buffer->input[3] = arg3;
-}
-
-static int dell_send_request(struct calling_interface_buffer *buffer,
-			     u16 class, u16 select)
-{
-	int ret;
 
 	buffer->cmd_class = class;
 	buffer->cmd_select = select;
@@ -474,14 +471,16 @@ static int dell_rfkill_set(void *data, bool blocked)
 	int status;
 	int ret;
 
-	dell_fill_request(&buffer, 0, 0, 0, 0);
-	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	ret = dell_send_request(&buffer,
+				CLASS_INFO, SELECT_RFKILL,
+				0, 0, 0, 0);
 	if (ret)
 		return ret;
 	status = buffer.output[1];
 
-	dell_fill_request(&buffer, 0x2, 0, 0, 0);
-	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	ret = dell_send_request(&buffer,
+				CLASS_INFO, SELECT_RFKILL,
+				0x2, 0, 0, 0);
 	if (ret)
 		return ret;
 	hwswitch = buffer.output[1];
@@ -492,8 +491,9 @@ static int dell_rfkill_set(void *data, bool blocked)
 	    (status & BIT(0)) && !(status & BIT(16)))
 		disable = 1;
 
-	dell_fill_request(&buffer, 1 | (radio<<8) | (disable << 16), 0, 0, 0);
-	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	ret = dell_send_request(&buffer,
+				CLASS_INFO, SELECT_RFKILL,
+				1 | (radio << 8) | (disable << 16), 0, 0, 0);
 	return ret;
 }
 
@@ -504,9 +504,9 @@ static void dell_rfkill_update_sw_state(struct rfkill *rfkill, int radio,
 		/* Has hw-switch, sync sw_state to BIOS */
 		struct calling_interface_buffer buffer;
 		int block = rfkill_blocked(rfkill);
-		dell_fill_request(&buffer,
-				   1 | (radio << 8) | (block << 16), 0, 0, 0);
-		dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+		dell_send_request(&buffer,
+				  CLASS_INFO, SELECT_RFKILL,
+				  1 | (radio << 8) | (block << 16), 0, 0, 0);
 	} else {
 		/* No hw-switch, sync BIOS state to sw_state */
 		rfkill_set_sw_state(rfkill, !!(status & BIT(radio + 16)));
@@ -528,16 +528,18 @@ static void dell_rfkill_query(struct rfkill *rfkill, void *data)
 	int status;
 	int ret;
 
-	dell_fill_request(&buffer, 0, 0, 0, 0);
-	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	ret = dell_send_request(&buffer,
+				CLASS_INFO, SELECT_RFKILL,
+				0, 0, 0, 0);
 	status = buffer.output[1];
 
 	if (ret != 0 || !(status & BIT(0))) {
 		return;
 	}
 
-	dell_fill_request(&buffer, 0, 0x2, 0, 0);
-	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	ret = dell_send_request(&buffer,
+				CLASS_INFO, SELECT_RFKILL,
+				0, 0x2, 0, 0);
 	hwswitch = buffer.output[1];
 
 	if (ret != 0)
@@ -561,14 +563,16 @@ static int dell_debugfs_show(struct seq_file *s, void *data)
 	int status;
 	int ret;
 
-	dell_fill_request(&buffer, 0, 0, 0, 0);
-	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	ret = dell_send_request(&buffer,
+				CLASS_INFO, SELECT_RFKILL,
+				0, 0, 0, 0);
 	if (ret)
 		return ret;
 	status = buffer.output[1];
 
-	dell_fill_request(&buffer, 0, 0x2, 0, 0);
-	hwswitch_ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	hwswitch_ret = dell_send_request(&buffer,
+					 CLASS_INFO, SELECT_RFKILL,
+					 0, 0x2, 0, 0);
 	if (hwswitch_ret)
 		return hwswitch_ret;
 	hwswitch_state = buffer.output[1];
@@ -645,15 +649,17 @@ static void dell_update_rfkill(struct work_struct *ignored)
 	int status;
 	int ret;
 
-	dell_fill_request(&buffer, 0, 0, 0, 0);
-	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	ret = dell_send_request(&buffer,
+				CLASS_INFO, SELECT_RFKILL,
+				0, 0, 0, 0);
 	status = buffer.output[1];
 
 	if (ret != 0)
 		return;
 
-	dell_fill_request(&buffer, 0, 0x2, 0, 0);
-	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	ret = dell_send_request(&buffer,
+				CLASS_INFO, SELECT_RFKILL,
+				0, 0x2, 0, 0);
 
 	if (ret == 0 && (status & BIT(0)))
 		hwswitch = buffer.output[1];
@@ -730,8 +736,9 @@ static int __init dell_setup_rfkill(void)
 	if (!force_rfkill && !whitelisted)
 		return 0;
 
-	dell_fill_request(&buffer, 0, 0, 0, 0);
-	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
+	ret = dell_send_request(&buffer,
+				CLASS_INFO, SELECT_RFKILL,
+				0, 0, 0, 0);
 	status = buffer.output[1];
 
 	/* dell wireless info smbios call is not supported */
@@ -893,14 +900,16 @@ static int dell_send_intensity(struct backlight_device *bd)
 	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);
+					CLASS_TOKEN_WRITE, SELECT_TOKEN_AC,
+					token->location, bd->props.brightness,
+					0, 0);
 	else
 		ret = dell_send_request(&buffer,
-					CLASS_TOKEN_WRITE, SELECT_TOKEN_BAT);
+					CLASS_TOKEN_WRITE, SELECT_TOKEN_BAT,
+					token->location, bd->props.brightness,
+					0, 0);
 
 	return ret;
 }
@@ -915,13 +924,14 @@ static int dell_get_intensity(struct backlight_device *bd)
 	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);
+					CLASS_TOKEN_READ, SELECT_TOKEN_AC,
+					token->location, 0, 0, 0);
 	else
 		ret = dell_send_request(&buffer,
-					CLASS_TOKEN_READ, SELECT_TOKEN_BAT);
+					CLASS_TOKEN_READ, SELECT_TOKEN_BAT,
+					token->location, 0, 0, 0);
 
 	if (ret == 0)
 		ret = buffer.output[1];
@@ -1194,9 +1204,9 @@ static int kbd_get_info(struct kbd_info *info)
 	u8 units;
 	int ret;
 
-	dell_fill_request(&buffer, 0, 0, 0, 0);
 	ret = dell_send_request(&buffer,
-				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
+				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT,
+				0, 0, 0, 0);
 	if (ret)
 		return ret;
 
@@ -1279,9 +1289,9 @@ static int kbd_get_state(struct kbd_state *state)
 	struct calling_interface_buffer buffer;
 	int ret;
 
-	dell_fill_request(&buffer, 0x1, 0, 0, 0);
 	ret = dell_send_request(&buffer,
-				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
+				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT,
+				0x1, 0, 0, 0);
 	if (ret)
 		return ret;
 
@@ -1316,9 +1326,9 @@ static int kbd_set_state(struct kbd_state *state)
 	input2 |= (state->level & 0xFF) << 16;
 	input2 |= (state->timeout_value_ac & 0x3F) << 24;
 	input2 |= (state->timeout_unit_ac & 0x3) << 30;
-	dell_fill_request(&buffer, 0x2, input1, input2, 0);
 	ret = dell_send_request(&buffer,
-				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
+				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT,
+				0x2, input1, input2, 0);
 
 	return ret;
 }
@@ -1356,8 +1366,9 @@ static int kbd_set_token_bit(u8 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);
+	ret = dell_send_request(&buffer,
+				CLASS_TOKEN_WRITE, SELECT_TOKEN_STD,
+				token->location, token->value, 0, 0);
 
 	return ret;
 }
@@ -1376,8 +1387,9 @@ static int kbd_get_token_bit(u8 bit)
 	if (!token)
 		return -EINVAL;
 
-	dell_fill_request(&buffer, token->location, 0, 0, 0);
-	ret = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD);
+	ret = dell_send_request(&buffer,
+				CLASS_TOKEN_READ, SELECT_TOKEN_STD,
+				token->location, 0, 0, 0);
 	val = buffer.output[1];
 
 	if (ret)
@@ -2127,8 +2139,9 @@ int dell_micmute_led_set(int state)
 	if (!token)
 		return -ENODEV;
 
-	dell_fill_request(&buffer, token->location, token->value, 0, 0);
-	dell_send_request(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
+	dell_send_request(&buffer,
+			  CLASS_TOKEN_WRITE, SELECT_TOKEN_STD,
+			  token->location, token->value, 0, 0);
 
 	return state;
 }
@@ -2185,9 +2198,9 @@ static int __init dell_init(void)
 	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);
+					CLASS_TOKEN_READ, SELECT_TOKEN_AC,
+					token->location, 0, 0, 0);
 		if (ret)
 			max_intensity = buffer.output[3];
 	}
-- 
2.7.4

Re: [PATCH] platform/x86: dell-laptop: merge fill_request and send_request

[Laszlo Toth]

On Tue, Feb 13, 2018 at 10:16:17PM +0100, Laszlo Toth wrote:
> Merged fill_request and send_request as they're used in pairs anyway so
> it's easier to check and harder to change the request values by mistake.
> 
> In a later commit the parameter count can be reduced.
> 
> Signed-off-by: Laszlo Toth <laszlth@gmail.com>
> ---
>  drivers/platform/x86/dell-laptop.c | 117 ++++++++++++++++++++-----------------
>  1 file changed, 65 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index a37cff9..30e6987 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -321,20 +321,17 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
>  	{ }
>  };
>  
> -static void dell_fill_request(struct calling_interface_buffer *buffer,
> -			       u32 arg0, u32 arg1, u32 arg2, u32 arg3)
> +static int dell_send_request(struct calling_interface_buffer *buffer,
> +			     u16 class, u16 select,
> +			     u32 arg0, u32 arg1, u32 arg2, u32 arg3)
>  {
> +	int ret;
> +
>  	memset(buffer, 0, sizeof(struct calling_interface_buffer));
>  	buffer->input[0] = arg0;
>  	buffer->input[1] = arg1;
>  	buffer->input[2] = arg2;
>  	buffer->input[3] = arg3;
> -}
> -
> -static int dell_send_request(struct calling_interface_buffer *buffer,
> -			     u16 class, u16 select)
> -{
> -	int ret;
>  
>  	buffer->cmd_class = class;
>  	buffer->cmd_select = select;
> @@ -474,14 +471,16 @@ static int dell_rfkill_set(void *data, bool blocked)
>  	int status;
>  	int ret;
>  
> -	dell_fill_request(&buffer, 0, 0, 0, 0);
> -	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> +	ret = dell_send_request(&buffer,
> +				CLASS_INFO, SELECT_RFKILL,
> +				0, 0, 0, 0);
>  	if (ret)
>  		return ret;
>  	status = buffer.output[1];
>  
> -	dell_fill_request(&buffer, 0x2, 0, 0, 0);
> -	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> +	ret = dell_send_request(&buffer,
> +				CLASS_INFO, SELECT_RFKILL,
> +				0x2, 0, 0, 0);
>  	if (ret)
>  		return ret;
>  	hwswitch = buffer.output[1];
> @@ -492,8 +491,9 @@ static int dell_rfkill_set(void *data, bool blocked)
>  	    (status & BIT(0)) && !(status & BIT(16)))
>  		disable = 1;
>  
> -	dell_fill_request(&buffer, 1 | (radio<<8) | (disable << 16), 0, 0, 0);
> -	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> +	ret = dell_send_request(&buffer,
> +				CLASS_INFO, SELECT_RFKILL,
> +				1 | (radio << 8) | (disable << 16), 0, 0, 0);
>  	return ret;
>  }
>  
> @@ -504,9 +504,9 @@ static void dell_rfkill_update_sw_state(struct rfkill *rfkill, int radio,
>  		/* Has hw-switch, sync sw_state to BIOS */
>  		struct calling_interface_buffer buffer;
>  		int block = rfkill_blocked(rfkill);
> -		dell_fill_request(&buffer,
> -				   1 | (radio << 8) | (block << 16), 0, 0, 0);
> -		dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> +		dell_send_request(&buffer,
> +				  CLASS_INFO, SELECT_RFKILL,
> +				  1 | (radio << 8) | (block << 16), 0, 0, 0);
>  	} else {
>  		/* No hw-switch, sync BIOS state to sw_state */
>  		rfkill_set_sw_state(rfkill, !!(status & BIT(radio + 16)));
> @@ -528,16 +528,18 @@ static void dell_rfkill_query(struct rfkill *rfkill, void *data)
>  	int status;
>  	int ret;
>  
> -	dell_fill_request(&buffer, 0, 0, 0, 0);
> -	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> +	ret = dell_send_request(&buffer,
> +				CLASS_INFO, SELECT_RFKILL,
> +				0, 0, 0, 0);
>  	status = buffer.output[1];
>  
>  	if (ret != 0 || !(status & BIT(0))) {
>  		return;
>  	}
>  
> -	dell_fill_request(&buffer, 0, 0x2, 0, 0);
> -	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> +	ret = dell_send_request(&buffer,
> +				CLASS_INFO, SELECT_RFKILL,
> +				0, 0x2, 0, 0);
>  	hwswitch = buffer.output[1];
>  
>  	if (ret != 0)
> @@ -561,14 +563,16 @@ static int dell_debugfs_show(struct seq_file *s, void *data)
>  	int status;
>  	int ret;
>  
> -	dell_fill_request(&buffer, 0, 0, 0, 0);
> -	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> +	ret = dell_send_request(&buffer,
> +				CLASS_INFO, SELECT_RFKILL,
> +				0, 0, 0, 0);
>  	if (ret)
>  		return ret;
>  	status = buffer.output[1];
>  
> -	dell_fill_request(&buffer, 0, 0x2, 0, 0);
> -	hwswitch_ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> +	hwswitch_ret = dell_send_request(&buffer,
> +					 CLASS_INFO, SELECT_RFKILL,
> +					 0, 0x2, 0, 0);
>  	if (hwswitch_ret)
>  		return hwswitch_ret;
>  	hwswitch_state = buffer.output[1];
> @@ -645,15 +649,17 @@ static void dell_update_rfkill(struct work_struct *ignored)
>  	int status;
>  	int ret;
>  
> -	dell_fill_request(&buffer, 0, 0, 0, 0);
> -	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> +	ret = dell_send_request(&buffer,
> +				CLASS_INFO, SELECT_RFKILL,
> +				0, 0, 0, 0);
>  	status = buffer.output[1];
>  
>  	if (ret != 0)
>  		return;
>  
> -	dell_fill_request(&buffer, 0, 0x2, 0, 0);
> -	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> +	ret = dell_send_request(&buffer,
> +				CLASS_INFO, SELECT_RFKILL,
> +				0, 0x2, 0, 0);
>  
>  	if (ret == 0 && (status & BIT(0)))
>  		hwswitch = buffer.output[1];
> @@ -730,8 +736,9 @@ static int __init dell_setup_rfkill(void)
>  	if (!force_rfkill && !whitelisted)
>  		return 0;
>  
> -	dell_fill_request(&buffer, 0, 0, 0, 0);
> -	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_RFKILL);
> +	ret = dell_send_request(&buffer,
> +				CLASS_INFO, SELECT_RFKILL,
> +				0, 0, 0, 0);
>  	status = buffer.output[1];
>  
>  	/* dell wireless info smbios call is not supported */
> @@ -893,14 +900,16 @@ static int dell_send_intensity(struct backlight_device *bd)
>  	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);
> +					CLASS_TOKEN_WRITE, SELECT_TOKEN_AC,
> +					token->location, bd->props.brightness,
> +					0, 0);
>  	else
>  		ret = dell_send_request(&buffer,
> -					CLASS_TOKEN_WRITE, SELECT_TOKEN_BAT);
> +					CLASS_TOKEN_WRITE, SELECT_TOKEN_BAT,
> +					token->location, bd->props.brightness,
> +					0, 0);
>  
>  	return ret;
>  }
> @@ -915,13 +924,14 @@ static int dell_get_intensity(struct backlight_device *bd)
>  	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);
> +					CLASS_TOKEN_READ, SELECT_TOKEN_AC,
> +					token->location, 0, 0, 0);
>  	else
>  		ret = dell_send_request(&buffer,
> -					CLASS_TOKEN_READ, SELECT_TOKEN_BAT);
> +					CLASS_TOKEN_READ, SELECT_TOKEN_BAT,
> +					token->location, 0, 0, 0);
>  
>  	if (ret == 0)
>  		ret = buffer.output[1];
> @@ -1194,9 +1204,9 @@ static int kbd_get_info(struct kbd_info *info)
>  	u8 units;
>  	int ret;
>  
> -	dell_fill_request(&buffer, 0, 0, 0, 0);
>  	ret = dell_send_request(&buffer,
> -				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
> +				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT,
> +				0, 0, 0, 0);
>  	if (ret)
>  		return ret;
>  
> @@ -1279,9 +1289,9 @@ static int kbd_get_state(struct kbd_state *state)
>  	struct calling_interface_buffer buffer;
>  	int ret;
>  
> -	dell_fill_request(&buffer, 0x1, 0, 0, 0);
>  	ret = dell_send_request(&buffer,
> -				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
> +				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT,
> +				0x1, 0, 0, 0);
>  	if (ret)
>  		return ret;
>  
> @@ -1316,9 +1326,9 @@ static int kbd_set_state(struct kbd_state *state)
>  	input2 |= (state->level & 0xFF) << 16;
>  	input2 |= (state->timeout_value_ac & 0x3F) << 24;
>  	input2 |= (state->timeout_unit_ac & 0x3) << 30;
> -	dell_fill_request(&buffer, 0x2, input1, input2, 0);
>  	ret = dell_send_request(&buffer,
> -				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
> +				CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT,
> +				0x2, input1, input2, 0);
>  
>  	return ret;
>  }
> @@ -1356,8 +1366,9 @@ static int kbd_set_token_bit(u8 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);
> +	ret = dell_send_request(&buffer,
> +				CLASS_TOKEN_WRITE, SELECT_TOKEN_STD,
> +				token->location, token->value, 0, 0);
>  
>  	return ret;
>  }
> @@ -1376,8 +1387,9 @@ static int kbd_get_token_bit(u8 bit)
>  	if (!token)
>  		return -EINVAL;
>  
> -	dell_fill_request(&buffer, token->location, 0, 0, 0);
> -	ret = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD);
> +	ret = dell_send_request(&buffer,
> +				CLASS_TOKEN_READ, SELECT_TOKEN_STD,
> +				token->location, 0, 0, 0);
>  	val = buffer.output[1];
>  
>  	if (ret)
> @@ -2127,8 +2139,9 @@ int dell_micmute_led_set(int state)
>  	if (!token)
>  		return -ENODEV;
>  
> -	dell_fill_request(&buffer, token->location, token->value, 0, 0);
> -	dell_send_request(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> +	dell_send_request(&buffer,
> +			  CLASS_TOKEN_WRITE, SELECT_TOKEN_STD,
> +			  token->location, token->value, 0, 0);
>  
>  	return state;
>  }
> @@ -2185,9 +2198,9 @@ static int __init dell_init(void)
>  	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);
> +					CLASS_TOKEN_READ, SELECT_TOKEN_AC,
> +					token->location, 0, 0, 0);
>  		if (ret)
>  			max_intensity = buffer.output[3];
>  	}
> -- 
> 2.7.4
> 

Answer to Pali from the prev. thread (before splitting):

"Hi!
I agree, there are a lot of 0s just to overwrite a 0 with 0
after memset.
So filling the buffer args could be improved regardless of my patch,
after that the merged function wouldn't have so many.
But for now I just changed them to one call. This way it won't have
to pass buffer ptr twice to 2 functions "chained" together and
already full of params to send a request.
There is one called and job done.

You're right in general, but this one is kinda special:
it does what you want _and_ holds the values you need to do that, too.
So this isn't a heavily used and modified function call I think, more
like a define without an actual define.

With these in mind I think it's better like this, you just check the
last line and done. That's how I found the typo in the other commit
as after &buffer it was invisible. I thought it might help."

Re: [PATCH] platform/x86: dell-laptop: merge fill_request and send_request

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 5084 bytes --]

Hi! This would be ratter longer email, but I hope I write everything
needed.

On Tuesday 13 February 2018 22:32:14 Laszlo Toth wrote:
> Answer to Pali from the prev. thread (before splitting):
> 
> "Hi!
> I agree, there are a lot of 0s just to overwrite a 0 with 0
> after memset.
> So filling the buffer args could be improved regardless of my patch,
> after that the merged function wouldn't have so many.
> But for now I just changed them to one call. This way it won't have
> to pass buffer ptr twice to 2 functions "chained" together and
> already full of params to send a request.

Main problem is that I still do not see a big benefit of it.

Or to say it in opposite way, why it is worse to have two functions and
need to pass buffer pointer to both functions (for obvious reason) as a
first parameter?

> There is one called and job done.

Apparently job is not done. There are also output parameters filled by
SMM (remote side) which caller wants to inspect. Job is: prepare,
fill, execute, fetch and process (check output/result).

> You're right in general, but this one is kinda special:
> it does what you want _and_ holds the values you need to do that, too.
> So this isn't a heavily used and modified function call I think, more
> like a define without an actual define.
> 
> With these in mind I think it's better like this, you just check the
> last line and done. That's how I found the typo in the other commit
> as after &buffer it was invisible. I thought it might help."

If you are going to write a new call or modify existing (for any reason)
you need to look at every one argument in that function. And if you want
to understand what is that function doing (again for any reason,
including "verification that code is written correctly"), you need to map
each positional argument to its meaning.

And I think generally functions with lot of parameters are reading
harder then function with few (3-4 arguments).

We can also open another question, why your proposed function takes
buffer structure which has members for input arguments, but also takes
input arguments as function parameters? Seems a bit illogical and
probably can confuse some people.


Just another look at it:

Buffer consists of 3 parts:

1) input parameters
2) output parameters
3) type of the SMM call (selector/class or how it is called in Dell terminology)

For sending current buffer to SMM there is a one function for it. And
part of it you specify also type of the SMM call.

Another (helper) function was created to fill input parameters into
buffer, so user would not need to manually clear buffer (via memset) and
then assign needed parameters.

And until now there was no need to create (helper) function to do
something with output parameters. If somebody needs them, just access
it.

I think this is just classic decomposition for solving some problem into
smaller pieces (fill input parameters, send request).

It can be compared to problem "write buffer to file". You have name of
file, access mode for file, permissions (in case to create new file),
buffer which you want to write and size of buffer. But you do not have
one function which do everything and takes lot of parameters. Rather you
have a function to create or open file and function to send data to
opened file. For each logic one function.



And another point of view:

Looking at code

  dell_send_request(&buffer, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);

I can deduce just from function names and parameters that it sends some
buffer to keyboard backlight service. Without reading implementation of
that function.

After your change there is a code

  dell_send_request(&buffer, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT, 0, 0, 0, 0);

I can just deduce it does something with buffer, probably send it into
keyboard backlight service, but I have absolutely no idea what are there
doing four zeros and why there are needed.

To make it more readable I need to do something like this:

  #define ARGUMENT_1_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION 0
  #define ARGUMENT_2_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION 0
  #define ARGUMENT_3_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION 0
  #define ARGUMENT_4_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION 0

  dell_send_request(
    &buffer,
    CLASS_KBD_BACKLIGHT,
    SELECT_KBD_BACKLIGHT,
    ARGUMENT_1_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION,
    ARGUMENT_2_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION,
    ARGUMENT_3_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION,
    ARGUMENT_4_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION
  );

Then I can deduce from code that it send request for asking feature
information about keyboard backlight. And also I know what are
arguments, what is definition of SMM request.

But it is too verbose, long and unusual.

If I want to prevent writing such verbose code, I can logically split
one function call into more. And one logic part of this are input
arguments.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] platform/x86: dell-laptop: merge fill_request and send_request

[Laszlo Toth]

On Tue, Feb 13, 2018 at 11:17:42PM +0100, Pali Rohár wrote:
> Hi! This would be ratter longer email, but I hope I write everything
> needed.
> 
Hi! Sorry for the late reply.
> On Tuesday 13 February 2018 22:32:14 Laszlo Toth wrote:
> > Answer to Pali from the prev. thread (before splitting):
> > 
> > "Hi!
> > I agree, there are a lot of 0s just to overwrite a 0 with 0
> > after memset.
> > So filling the buffer args could be improved regardless of my patch,
> > after that the merged function wouldn't have so many.
> > But for now I just changed them to one call. This way it won't have
> > to pass buffer ptr twice to 2 functions "chained" together and
> > already full of params to send a request.
> 
> Main problem is that I still do not see a big benefit of it.
> 
> Or to say it in opposite way, why it is worse to have two functions and
> need to pass buffer pointer to both functions (for obvious reason) as a
> first parameter?
> 

I think it's easier to overlook some values like the 0x1 -> 0 case after
&buffer. Not a big benefit though, I agree.

> > There is one called and job done.
> 
> Apparently job is not done. There are also output parameters filled by
> SMM (remote side) which caller wants to inspect. Job is: prepare,
> fill, execute, fetch and process (check output/result).
> 

I meant job as sending. Point was that currently sending is a fill + send,
so it's two functions for one thing to do.

> > You're right in general, but this one is kinda special:
> > it does what you want _and_ holds the values you need to do that, too.
> > So this isn't a heavily used and modified function call I think, more
> > like a define without an actual define.
> > 
> > With these in mind I think it's better like this, you just check the
> > last line and done. That's how I found the typo in the other commit
> > as after &buffer it was invisible. I thought it might help."
> 
> If you are going to write a new call or modify existing (for any reason)
> you need to look at every one argument in that function. And if you want
> to understand what is that function doing (again for any reason,
> including "verification that code is written correctly"), you need to map
> each positional argument to its meaning.
> 

I don't see the problem here. It's separated to multiple lines, we can
check them easily when writing new ones. Moreover you have to do this
now as well I think.

> And I think generally functions with lot of parameters are reading
> harder then function with few (3-4 arguments).
> 

Agreed.

> We can also open another question, why your proposed function takes
> buffer structure which has members for input arguments, but also takes
> input arguments as function parameters? Seems a bit illogical and
> probably can confuse some people.
> 
> 

Good point.

> Just another look at it:
> 
> Buffer consists of 3 parts:
> 
> 1) input parameters
> 2) output parameters
> 3) type of the SMM call (selector/class or how it is called in Dell terminology)
> 
> For sending current buffer to SMM there is a one function for it. And
> part of it you specify also type of the SMM call.
> 
> Another (helper) function was created to fill input parameters into
> buffer, so user would not need to manually clear buffer (via memset) and
> then assign needed parameters.
> 
> And until now there was no need to create (helper) function to do
> something with output parameters. If somebody needs them, just access
> it.
> 
> I think this is just classic decomposition for solving some problem into
> smaller pieces (fill input parameters, send request).
> 
> It can be compared to problem "write buffer to file". You have name of
> file, access mode for file, permissions (in case to create new file),
> buffer which you want to write and size of buffer. But you do not have
> one function which do everything and takes lot of parameters. Rather you
> have a function to create or open file and function to send data to
> opened file. For each logic one function.
> 
> 
> 
> And another point of view:
> 
> Looking at code
> 
>   dell_send_request(&buffer, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT);
> 
> I can deduce just from function names and parameters that it sends some
> buffer to keyboard backlight service. Without reading implementation of
> that function.
> 
> After your change there is a code
> 
>   dell_send_request(&buffer, CLASS_KBD_BACKLIGHT, SELECT_KBD_BACKLIGHT, 0, 0, 0, 0);
> 
> I can just deduce it does something with buffer, probably send it into
> keyboard backlight service, but I have absolutely no idea what are there
> doing four zeros and why there are needed.
> 
> To make it more readable I need to do something like this:
> 
>   #define ARGUMENT_1_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION 0
>   #define ARGUMENT_2_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION 0
>   #define ARGUMENT_3_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION 0
>   #define ARGUMENT_4_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION 0
> 
>   dell_send_request(
>     &buffer,
>     CLASS_KBD_BACKLIGHT,
>     SELECT_KBD_BACKLIGHT,
>     ARGUMENT_1_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION,
>     ARGUMENT_2_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION,
>     ARGUMENT_3_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION,
>     ARGUMENT_4_FOR_KBD_BACKLIGHT_GET_FEATURE_INFORMATION
>   );
> 
> Then I can deduce from code that it send request for asking feature
> information about keyboard backlight. And also I know what are
> arguments, what is definition of SMM request.
> 
> But it is too verbose, long and unusual.
> 
> If I want to prevent writing such verbose code, I can logically split
> one function call into more. And one logic part of this are input
> arguments.
> 

You're right, mine won't be better. The two functions look like
the less bad thing now. Please ignore the patch.

Maybe I'll take another look at it if I have some spare time.

Thanks for the detailed reply,
Laszlo

> -- 
> Pali Rohár
> pali.rohar@gmail.com