* [PATCH 1/3] platform/x86: msi-laptop: Fix old-ec check for backlight registering
@ 2022-08-25 14:13 Hans de Goede
2022-08-25 14:13 ` [PATCH 2/3] platform/x86: msi-laptop: Simplify ec_delay handling Hans de Goede
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Hans de Goede @ 2022-08-25 14:13 UTC (permalink / raw)
To: Mark Gross, Andy Shevchenko; +Cc: Hans de Goede, platform-driver-x86
Commit 2cc6c717799f ("msi-laptop: Port to new backlight interface
selection API") replaced this check:
if (!quirks->old_ec_model || acpi_video_backlight_support())
pr_info("Brightness ignored, ...");
else
do_register();
With:
if (quirks->old_ec_model ||
acpi_video_get_backlight_type() == acpi_backlight_vendor)
do_register();
But since the do_register() part was part of the else branch, the entire
condition should be inverted. So not only the 2 statements on either
side of the || should be inverted, but the || itself should be replaced
with a &&.
In practice this has likely not been an issue because the new-ec models
(old_ec_model==false) likely all support ACPI video backlight control,
making acpi_video_get_backlight_type() return acpi_backlight_video
turning the second part of the || also false when old_ec_model == false.
Fixes: 2cc6c717799f ("msi-laptop: Port to new backlight interface selection API")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/platform/x86/msi-laptop.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
index 93ef8851b93e..54170172a666 100644
--- a/drivers/platform/x86/msi-laptop.c
+++ b/drivers/platform/x86/msi-laptop.c
@@ -1047,8 +1047,7 @@ static int __init msi_init(void)
return -EINVAL;
/* Register backlight stuff */
-
- if (quirks->old_ec_model ||
+ if (quirks->old_ec_model &&
acpi_video_get_backlight_type() == acpi_backlight_vendor) {
struct backlight_properties props;
memset(&props, 0, sizeof(struct backlight_properties));
--
2.37.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] platform/x86: msi-laptop: Simplify ec_delay handling
2022-08-25 14:13 [PATCH 1/3] platform/x86: msi-laptop: Fix old-ec check for backlight registering Hans de Goede
@ 2022-08-25 14:13 ` Hans de Goede
2022-08-25 15:05 ` Andy Shevchenko
2022-08-25 14:13 ` [PATCH 3/3] platform/x86: msi-laptop: Fix resource cleanup Hans de Goede
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Hans de Goede @ 2022-08-25 14:13 UTC (permalink / raw)
To: Mark Gross, Andy Shevchenko; +Cc: Hans de Goede, platform-driver-x86
There is no reason to have both non-delayed and delayed work structs
for the rfkill and touchpad work.
Instead simply call schedule_delayed_work() with a delay of 0 for
the quirks->ec_delay == fase case.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/platform/x86/msi-laptop.c | 31 +++++++++++--------------------
1 file changed, 11 insertions(+), 20 deletions(-)
diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
index 54170172a666..65db18c6e3e8 100644
--- a/drivers/platform/x86/msi-laptop.c
+++ b/drivers/platform/x86/msi-laptop.c
@@ -590,6 +590,14 @@ static int dmi_check_cb(const struct dmi_system_id *dmi)
return 1;
}
+static unsigned long msi_work_delay(int msecs)
+{
+ if (quirks->ec_delay)
+ return msecs_to_jiffies(msecs);
+
+ return 0;
+}
+
static const struct dmi_system_id msi_dmi_table[] __initconst = {
{
.ident = "MSI S270",
@@ -784,7 +792,6 @@ static void msi_update_rfkill(struct work_struct *ignored)
msi_rfkill_set_state(rfk_threeg, !threeg_s);
}
static DECLARE_DELAYED_WORK(msi_rfkill_dwork, msi_update_rfkill);
-static DECLARE_WORK(msi_rfkill_work, msi_update_rfkill);
static void msi_send_touchpad_key(struct work_struct *ignored)
{
@@ -800,7 +807,6 @@ static void msi_send_touchpad_key(struct work_struct *ignored)
KEY_TOUCHPAD_ON : KEY_TOUCHPAD_OFF, 1, true);
}
static DECLARE_DELAYED_WORK(msi_touchpad_dwork, msi_send_touchpad_key);
-static DECLARE_WORK(msi_touchpad_work, msi_send_touchpad_key);
static bool msi_laptop_i8042_filter(unsigned char data, unsigned char str,
struct serio *port)
@@ -818,20 +824,12 @@ static bool msi_laptop_i8042_filter(unsigned char data, unsigned char str,
extended = false;
switch (data) {
case 0xE4:
- if (quirks->ec_delay) {
- schedule_delayed_work(&msi_touchpad_dwork,
- round_jiffies_relative(0.5 * HZ));
- } else
- schedule_work(&msi_touchpad_work);
+ schedule_delayed_work(&msi_touchpad_dwork, msi_work_delay(500));
break;
case 0x54:
case 0x62:
case 0x76:
- if (quirks->ec_delay) {
- schedule_delayed_work(&msi_rfkill_dwork,
- round_jiffies_relative(0.5 * HZ));
- } else
- schedule_work(&msi_rfkill_work);
+ schedule_delayed_work(&msi_rfkill_dwork, msi_work_delay(500));
break;
}
}
@@ -898,12 +896,7 @@ static int rfkill_init(struct platform_device *sdev)
}
/* schedule to run rfkill state initial */
- if (quirks->ec_delay) {
- schedule_delayed_work(&msi_rfkill_init,
- round_jiffies_relative(1 * HZ));
- } else
- schedule_work(&msi_rfkill_work);
-
+ schedule_delayed_work(&msi_rfkill_init, msi_work_delay(1000));
return 0;
err_threeg:
@@ -1114,7 +1107,6 @@ static int __init msi_init(void)
if (quirks->load_scm_model) {
i8042_remove_filter(msi_laptop_i8042_filter);
cancel_delayed_work_sync(&msi_rfkill_dwork);
- cancel_work_sync(&msi_rfkill_work);
rfkill_cleanup();
}
fail_scm_model_init:
@@ -1135,7 +1127,6 @@ static void __exit msi_cleanup(void)
i8042_remove_filter(msi_laptop_i8042_filter);
input_unregister_device(msi_laptop_input_dev);
cancel_delayed_work_sync(&msi_rfkill_dwork);
- cancel_work_sync(&msi_rfkill_work);
rfkill_cleanup();
}
--
2.37.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] platform/x86: msi-laptop: Fix resource cleanup
2022-08-25 14:13 [PATCH 1/3] platform/x86: msi-laptop: Fix old-ec check for backlight registering Hans de Goede
2022-08-25 14:13 ` [PATCH 2/3] platform/x86: msi-laptop: Simplify ec_delay handling Hans de Goede
@ 2022-08-25 14:13 ` Hans de Goede
2022-08-25 15:05 ` Andy Shevchenko
2022-08-25 15:04 ` [PATCH 1/3] platform/x86: msi-laptop: Fix old-ec check for backlight registering Andy Shevchenko
2022-08-26 11:17 ` Hans de Goede
3 siblings, 1 reply; 9+ messages in thread
From: Hans de Goede @ 2022-08-25 14:13 UTC (permalink / raw)
To: Mark Gross, Andy Shevchenko; +Cc: Hans de Goede, platform-driver-x86
Fix the input-device not getting free-ed on probe-errors and
fix the msi_touchpad_dwork not getting cancelled on neither
probe-errors nor on remove.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/platform/x86/msi-laptop.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
index 65db18c6e3e8..5d4b10b8d771 100644
--- a/drivers/platform/x86/msi-laptop.c
+++ b/drivers/platform/x86/msi-laptop.c
@@ -1106,6 +1106,8 @@ static int __init msi_init(void)
fail_create_group:
if (quirks->load_scm_model) {
i8042_remove_filter(msi_laptop_i8042_filter);
+ cancel_delayed_work_sync(&msi_touchpad_dwork);
+ input_unregister_device(msi_laptop_input_dev);
cancel_delayed_work_sync(&msi_rfkill_dwork);
rfkill_cleanup();
}
@@ -1125,6 +1127,7 @@ static void __exit msi_cleanup(void)
{
if (quirks->load_scm_model) {
i8042_remove_filter(msi_laptop_i8042_filter);
+ cancel_delayed_work_sync(&msi_touchpad_dwork);
input_unregister_device(msi_laptop_input_dev);
cancel_delayed_work_sync(&msi_rfkill_dwork);
rfkill_cleanup();
--
2.37.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] platform/x86: msi-laptop: Fix old-ec check for backlight registering
2022-08-25 14:13 [PATCH 1/3] platform/x86: msi-laptop: Fix old-ec check for backlight registering Hans de Goede
2022-08-25 14:13 ` [PATCH 2/3] platform/x86: msi-laptop: Simplify ec_delay handling Hans de Goede
2022-08-25 14:13 ` [PATCH 3/3] platform/x86: msi-laptop: Fix resource cleanup Hans de Goede
@ 2022-08-25 15:04 ` Andy Shevchenko
2022-08-26 11:17 ` Hans de Goede
3 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2022-08-25 15:04 UTC (permalink / raw)
To: Hans de Goede; +Cc: Mark Gross, Andy Shevchenko, platform-driver-x86
On Thu, Aug 25, 2022 at 04:13:34PM +0200, Hans de Goede wrote:
> Commit 2cc6c717799f ("msi-laptop: Port to new backlight interface
> selection API") replaced this check:
>
> if (!quirks->old_ec_model || acpi_video_backlight_support())
> pr_info("Brightness ignored, ...");
> else
> do_register();
>
> With:
>
> if (quirks->old_ec_model ||
> acpi_video_get_backlight_type() == acpi_backlight_vendor)
> do_register();
>
> But since the do_register() part was part of the else branch, the entire
> condition should be inverted. So not only the 2 statements on either
> side of the || should be inverted, but the || itself should be replaced
> with a &&.
>
> In practice this has likely not been an issue because the new-ec models
> (old_ec_model==false) likely all support ACPI video backlight control,
> making acpi_video_get_backlight_type() return acpi_backlight_video
> turning the second part of the || also false when old_ec_model == false.
...
> /* Register backlight stuff */
> -
> - if (quirks->old_ec_model ||
> + if (quirks->old_ec_model &&
> acpi_video_get_backlight_type() == acpi_backlight_vendor) {
> struct backlight_properties props;
checkpatch might complain for absence of blank line here, btw. Perhaps
you may just move the above one here at the same patch.
> memset(&props, 0, sizeof(struct backlight_properties));
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] platform/x86: msi-laptop: Simplify ec_delay handling
2022-08-25 14:13 ` [PATCH 2/3] platform/x86: msi-laptop: Simplify ec_delay handling Hans de Goede
@ 2022-08-25 15:05 ` Andy Shevchenko
2022-08-25 15:30 ` Hans de Goede
0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2022-08-25 15:05 UTC (permalink / raw)
To: Hans de Goede; +Cc: Mark Gross, Andy Shevchenko, platform-driver-x86
On Thu, Aug 25, 2022 at 04:13:35PM +0200, Hans de Goede wrote:
> There is no reason to have both non-delayed and delayed work structs
> for the rfkill and touchpad work.
>
> Instead simply call schedule_delayed_work() with a delay of 0 for
> the quirks->ec_delay == fase case.
fase ?!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] platform/x86: msi-laptop: Fix resource cleanup
2022-08-25 14:13 ` [PATCH 3/3] platform/x86: msi-laptop: Fix resource cleanup Hans de Goede
@ 2022-08-25 15:05 ` Andy Shevchenko
2022-08-25 15:37 ` Hans de Goede
0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2022-08-25 15:05 UTC (permalink / raw)
To: Hans de Goede; +Cc: Mark Gross, Andy Shevchenko, platform-driver-x86
On Thu, Aug 25, 2022 at 04:13:36PM +0200, Hans de Goede wrote:
> Fix the input-device not getting free-ed on probe-errors and
> fix the msi_touchpad_dwork not getting cancelled on neither
> probe-errors nor on remove.
Sounds like a fix, shouldn't have a Fixes tag?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] platform/x86: msi-laptop: Simplify ec_delay handling
2022-08-25 15:05 ` Andy Shevchenko
@ 2022-08-25 15:30 ` Hans de Goede
0 siblings, 0 replies; 9+ messages in thread
From: Hans de Goede @ 2022-08-25 15:30 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Mark Gross, Andy Shevchenko, platform-driver-x86
Hi,
On 8/25/22 17:05, Andy Shevchenko wrote:
> On Thu, Aug 25, 2022 at 04:13:35PM +0200, Hans de Goede wrote:
>> There is no reason to have both non-delayed and delayed work structs
>> for the rfkill and touchpad work.
>>
>> Instead simply call schedule_delayed_work() with a delay of 0 for
>> the quirks->ec_delay == fase case.
>
> fase ?!
That should be "false", my bad.
Regards,
Hans
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] platform/x86: msi-laptop: Fix resource cleanup
2022-08-25 15:05 ` Andy Shevchenko
@ 2022-08-25 15:37 ` Hans de Goede
0 siblings, 0 replies; 9+ messages in thread
From: Hans de Goede @ 2022-08-25 15:37 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Mark Gross, Andy Shevchenko, platform-driver-x86
Hi,
On 8/25/22 17:05, Andy Shevchenko wrote:
> On Thu, Aug 25, 2022 at 04:13:36PM +0200, Hans de Goede wrote:
>> Fix the input-device not getting free-ed on probe-errors and
>> fix the msi_touchpad_dwork not getting cancelled on neither
>> probe-errors nor on remove.
>
> Sounds like a fix, shouldn't have a Fixes tag?
You right, I'll add a:
Fixes: 143a4c0284dc ("msi-laptop: send out touchpad on/off key")
tag before merging.
Regards,
Hans
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] platform/x86: msi-laptop: Fix old-ec check for backlight registering
2022-08-25 14:13 [PATCH 1/3] platform/x86: msi-laptop: Fix old-ec check for backlight registering Hans de Goede
` (2 preceding siblings ...)
2022-08-25 15:04 ` [PATCH 1/3] platform/x86: msi-laptop: Fix old-ec check for backlight registering Andy Shevchenko
@ 2022-08-26 11:17 ` Hans de Goede
3 siblings, 0 replies; 9+ messages in thread
From: Hans de Goede @ 2022-08-26 11:17 UTC (permalink / raw)
To: Mark Gross, Andy Shevchenko; +Cc: platform-driver-x86
Hi,
On 8/25/22 16:13, Hans de Goede wrote:
> Commit 2cc6c717799f ("msi-laptop: Port to new backlight interface
> selection API") replaced this check:
>
> if (!quirks->old_ec_model || acpi_video_backlight_support())
> pr_info("Brightness ignored, ...");
> else
> do_register();
>
> With:
>
> if (quirks->old_ec_model ||
> acpi_video_get_backlight_type() == acpi_backlight_vendor)
> do_register();
>
> But since the do_register() part was part of the else branch, the entire
> condition should be inverted. So not only the 2 statements on either
> side of the || should be inverted, but the || itself should be replaced
> with a &&.
>
> In practice this has likely not been an issue because the new-ec models
> (old_ec_model==false) likely all support ACPI video backlight control,
> making acpi_video_get_backlight_type() return acpi_backlight_video
> turning the second part of the || also false when old_ec_model == false.
>
> Fixes: 2cc6c717799f ("msi-laptop: Port to new backlight interface selection API")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
I've added this series to my review-hans (soon to be for-next) branch now.
Regards,
Hans
> ---
> drivers/platform/x86/msi-laptop.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
> index 93ef8851b93e..54170172a666 100644
> --- a/drivers/platform/x86/msi-laptop.c
> +++ b/drivers/platform/x86/msi-laptop.c
> @@ -1047,8 +1047,7 @@ static int __init msi_init(void)
> return -EINVAL;
>
> /* Register backlight stuff */
> -
> - if (quirks->old_ec_model ||
> + if (quirks->old_ec_model &&
> acpi_video_get_backlight_type() == acpi_backlight_vendor) {
> struct backlight_properties props;
> memset(&props, 0, sizeof(struct backlight_properties));
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-08-26 11:17 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-25 14:13 [PATCH 1/3] platform/x86: msi-laptop: Fix old-ec check for backlight registering Hans de Goede
2022-08-25 14:13 ` [PATCH 2/3] platform/x86: msi-laptop: Simplify ec_delay handling Hans de Goede
2022-08-25 15:05 ` Andy Shevchenko
2022-08-25 15:30 ` Hans de Goede
2022-08-25 14:13 ` [PATCH 3/3] platform/x86: msi-laptop: Fix resource cleanup Hans de Goede
2022-08-25 15:05 ` Andy Shevchenko
2022-08-25 15:37 ` Hans de Goede
2022-08-25 15:04 ` [PATCH 1/3] platform/x86: msi-laptop: Fix old-ec check for backlight registering Andy Shevchenko
2022-08-26 11:17 ` Hans de Goede
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.