From: Laszlo Toth <laszlth@gmail.com>
To: Mario.Limonciello@dell.com
Cc: pali.rohar@gmail.com, mjg59@srcf.ucam.org, dvhart@infradead.org,
andy@infradead.org, platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH] platform/x86: dell-laptop: fix kbd_get_state's request value
Date: Tue, 13 Feb 2018 20:12:04 +0100 [thread overview]
Message-ID: <20180213191204.GA4308@laszlth> (raw)
In-Reply-To: <41e70e169afd47dfbcf93af7c1f71f6b@ausx13mpc120.AMER.DELL.COM>
On Mon, Feb 12, 2018 at 05:29:20PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Sunday, February 11, 2018 4:49 PM
> > To: Laszlo Toth <laszlth@gmail.com>
> > Cc: Matthew Garrett <mjg59@srcf.ucam.org>; Darren Hart
> > <dvhart@infradead.org>; Andy Shevchenko <andy@infradead.org>; platform-
> > driver-x86@vger.kernel.org; Limonciello, Mario <Mario_Limonciello@Dell.com>
> > 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@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 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@gmail.com
prev parent reply other threads:[~2018-02-13 19:12 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-11 0:20 [PATCH] platform/x86: dell-laptop: fix kbd_get_state's request value Laszlo Toth
2018-02-11 22:49 ` Pali Rohár
2018-02-12 17:29 ` Mario.Limonciello
2018-02-13 19:12 ` Laszlo Toth [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180213191204.GA4308@laszlth \
--to=laszlth@gmail.com \
--cc=Mario.Limonciello@dell.com \
--cc=andy@infradead.org \
--cc=dvhart@infradead.org \
--cc=mjg59@srcf.ucam.org \
--cc=pali.rohar@gmail.com \
--cc=platform-driver-x86@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox