On Mon, Feb 12, 2018 at 05:29:20PM +0000, Mario.Limonciello@xxxxxxxx wrote: > > -----Original Message----- > > From: Pali Rohár [mailto:pali.rohar@xxxxxxxxx] > > Sent: Sunday, February 11, 2018 4:49 PM > > To: Laszlo Toth <laszlth@xxxxxxxxx> > > Cc: Matthew Garrett <mjg59@xxxxxxxxxxxxx>; Darren Hart > > <dvhart@xxxxxxxxxxxxx>; Andy Shevchenko <andy@xxxxxxxxxxxxx>; platform- > > driver-x86@xxxxxxxxxxxxxxx; Limonciello, Mario <Mario_Limonciello@xxxxxxxx> > > Subject: Re: [PATCH] platform/x86: dell-laptop: fix kbd_get_state's request value > > > > On Sunday 11 February 2018 01:20:10 Laszlo Toth wrote: > > > Commit 9862b43624a5 ("platform/x86: dell-laptop: Allocate buffer on heap > > > rather than globally") > > > broke one request, changed it back to the original value. > > > > > > 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 > > > again. > > > > > > Tested on a Dell E6540, backlight came back. > > > > > > Signed-off-by: Laszlo Toth <laszlth@xxxxxxxxx> > > > --- > > > 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 2a68f59..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) > > > > Hi! Please do not create a function with infinite parameters. Seven > > parameters is really to big number. It reminds me Win32 API where > > functions have 10 parameters and in most cases they are called with lot > > of NULLs or zeros. And this patch is very similar. > > > > You say that this fixes bug. Which? I'm really lost in this big patch > > and do not see it. > > I agree. Please at least split this into 2 patches. The first one which fixes > the bug that was identified by you and the second which would overhaul > the approach (although that might not be applicable given Pali's comments). > Okay, I'll split them. > > > > Also making mistake in function with 7 parameters is easier as in > > function with less arguments. Its hard to read which parameter is what > > doing and also to figure out if order of parameters is correct. > > > > > { > > > + 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, 0, 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]; > > > } > > > > -- > > Pali Rohár > > pali.rohar@xxxxxxxxx
