* RE: [PATCH v2] dell-laptop: Fix keyboard led max_brightness property for Dell Latitude E6410
@ 2017-10-19 18:20 Gabriel M. Elder
2017-10-19 18:31 ` Pali Rohár
0 siblings, 1 reply; 7+ messages in thread
From: Gabriel M. Elder @ 2017-10-19 18:20 UTC (permalink / raw)
To: Darren Hart, Pali Rohár
Cc: Matthew Garrett, Andy Shevchenko, Gabriele Mazzotta,
Mario.Limonciello, platform-driver-x86, linux-kernel
-------- Original Message --------
Subject: Re: [PATCH v2] dell-laptop: Fix keyboard led max_brightness
property for Dell Latitude E6410
From: Darren Hart <dvhart@infradead.org>
Date: Wed, October 18, 2017 3:00 pm
To: Pali Rohár <pali.rohar@gmail.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>, Andy Shevchenko
<andy@infradead.org>, "Gabriel M. Elder" <gabriel@tekgnowsys.com>,
Gabriele Mazzotta <gabriele.mzt@gmail.com>, Mario.Limonciello@dell.com,
platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
On Wed, Oct 18, 2017 at 08:06:01PM +0200, Pali Rohár wrote:
> This machine reports number of keyboard backlight led levels, instead of
> value of the last led level index. Therefore max_brightness properly needs
> to be subtracted by 1 to match led max_brightness API.
>
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Reported-by: Gabriel M. Elder <gabriel@tekgnowsys.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=196913
> ---
> Changes since v1:
> * Update kbd_info.levels at initialization time based on quirk
This looks much cleaner IMO, thanks.
> ---
> drivers/platform/x86/dell-laptop.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index f42159f..3f9be8a 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -49,6 +49,7 @@
>
> struct quirk_entry {
> u8 touchpad_led;
> + u8 kbd_led_num_of_levels_instead_of_last_index;
OK, I forgot to comment on this one :-)
A name should be descriptive, but that's just too much. So let's use
something
shorter, and add a comment if one is really needed to explain what it
is.
Something like:
u8 kbd_led_reports_num_levels is shorter, but still awfully long.
u8 kbd_led_num_levels is shorter, but obviously looks like it should be
the
levels value instead of a bool. So the above is probably best.
>
> int needs_kbd_timeouts;
> /*
> @@ -79,6 +80,10 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
> .kbd_timeouts = { 0, 5, 15, 60, 5 * 60, 15 * 60, -1 },
> };
>
> +static struct quirk_entry quirk_dell_latitude_e6410 = {
> + .kbd_led_num_of_levels_instead_of_last_index = 1,
> +};
> +
> static struct platform_driver platform_driver = {
> .driver = {
> .name = "dell-laptop",
> @@ -280,6 +285,15 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
> },
> .driver_data = &quirk_dell_xps13_9333,
> },
> + {
> + .callback = dmi_matched,
> + .ident = "Dell Latitude E6410",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6410"),
> + },
> + .driver_data = &quirk_dell_latitude_e6410,
> + },
> { }
> };
>
> @@ -1200,6 +1214,9 @@ static int kbd_get_info(struct kbd_info *info)
> units = (buffer->output[2] >> 8) & 0xFF;
> info->levels = (buffer->output[2] >> 16) & 0xFF;
>
> + if (quirks && quirks->kbd_led_num_of_levels_instead_of_last_index && info->levels)
> + info->levels--;
> +
> if (units & BIT(0))
> info->seconds = (buffer->output[3] >> 0) & 0xFF;
> if (units & BIT(1))
> --
> 1.7.9.5
>
>
--
Darren Hart
VMware Open Source Technology Center
-------- End Original Message --------
I will be happy to test out whichever patch(es) present the most
appropriate approach, as a matter of consensus.
Would that be the PATCH v2
(https://patchwork.kernel.org/patch/10015149/) that I should be testing?
thanks,
- Gabriel
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v2] dell-laptop: Fix keyboard led max_brightness property for Dell Latitude E6410
@ 2017-10-21 17:55 Gabriel M. Elder
0 siblings, 0 replies; 7+ messages in thread
From: Gabriel M. Elder @ 2017-10-21 17:55 UTC (permalink / raw)
To: Pali Rohár
Cc: Darren Hart, Matthew Garrett, Andy Shevchenko, Gabriele Mazzotta,
Mario.Limonciello, platform-driver-x86, linux-kernel
-------- Original Message --------
Subject: Re: [PATCH v2] dell-laptop: Fix keyboard led max_brightness
property for Dell Latitude E6410
From: Pali Rohár <pali.rohar@gmail.com>
Date: Thu, October 19, 2017 1:31 pm
To: "Gabriel M. Elder" <gabriel@tekgnowsys.com>
Cc: Darren Hart <dvhart@infradead.org>, Matthew Garrett
<mjg59@srcf.ucam.org>, Andy Shevchenko <andy@infradead.org>, Gabriele
Mazzotta <gabriele.mzt@gmail.com>, Mario.Limonciello@dell.com,
platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
On Thursday 19 October 2017 11:20:35 Gabriel M. Elder wrote:
> I will be happy to test out whichever patch(es) present the most
> appropriate approach, as a matter of consensus.
>
> Would that be the PATCH v2
> (https://patchwork.kernel.org/patch/10015149/) that I should be testing?
Yes, v2.
--
Pali Rohár
pali.rohar@gmail.com
-------- End Original Message --------
Based on my testing, the v2 patch seems to work fine.
- Gabriel
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] dell-laptop: Fix keyboard led max_brightness property for Dell Latitude E6410
@ 2017-10-15 16:03 Pali Rohár
2017-10-18 18:06 ` [PATCH v2] " Pali Rohár
0 siblings, 1 reply; 7+ messages in thread
From: Pali Rohár @ 2017-10-15 16:03 UTC (permalink / raw)
To: Matthew Garrett, Darren Hart, Andy Shevchenko, Gabriel M. Elder,
Gabriele Mazzotta, Mario.Limonciello
Cc: platform-driver-x86, linux-kernel, Pali Rohár
This machine reports number of keyboard backlight led levels, instead of
value of the last led level index. Therefore max_brightness properly needs
to be subtracted by 1 to match led max_brightness API.
Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Reported-by: Gabriel M. Elder <gabriel@tekgnowsys.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=196913
---
Hi! I have not tested this patch yet, but I'm sending it, so other people
including Gabriel can test or review it.
If you have better idea how to call that quirk variable, then fell free
to rename it. I'm not happy with "kbd_led_num_of_levels_instead_of_last_index"
but I have nothing better in my mind.
---
drivers/platform/x86/dell-laptop.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index f42159f..81f14ea 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -49,6 +49,7 @@
struct quirk_entry {
u8 touchpad_led;
+ u8 kbd_led_num_of_levels_instead_of_last_index;
int needs_kbd_timeouts;
/*
@@ -79,6 +80,10 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
.kbd_timeouts = { 0, 5, 15, 60, 5 * 60, 15 * 60, -1 },
};
+static struct quirk_entry quirk_dell_latitude_e6410 = {
+ .kbd_led_num_of_levels_instead_of_last_index = 1,
+};
+
static struct platform_driver platform_driver = {
.driver = {
.name = "dell-laptop",
@@ -280,6 +285,15 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
},
.driver_data = &quirk_dell_xps13_9333,
},
+ {
+ .callback = dmi_matched,
+ .ident = "Dell Latitude E6410",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6410"),
+ },
+ .driver_data = &quirk_dell_latitude_e6410,
+ },
{ }
};
@@ -1216,8 +1230,15 @@ static int kbd_get_info(struct kbd_info *info)
static unsigned int kbd_get_max_level(void)
{
- if (kbd_info.levels != 0)
- return kbd_info.levels;
+ u8 levels = kbd_info.levels;
+
+ if (quirks && quirks->kbd_led_num_of_levels_instead_of_last_index) {
+ if (levels > 1)
+ return levels - 1;
+ } else {
+ if (levels > 0)
+ return levels;
+ }
if (kbd_mode_levels_count > 0)
return kbd_mode_levels_count - 1;
return 0;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2] dell-laptop: Fix keyboard led max_brightness property for Dell Latitude E6410
2017-10-15 16:03 [PATCH] " Pali Rohár
@ 2017-10-18 18:06 ` Pali Rohár
2017-10-18 19:09 ` Andy Shevchenko
2017-10-18 20:00 ` Darren Hart
0 siblings, 2 replies; 7+ messages in thread
From: Pali Rohár @ 2017-10-18 18:06 UTC (permalink / raw)
To: Matthew Garrett, Darren Hart, Andy Shevchenko, Gabriel M. Elder,
Gabriele Mazzotta, Mario.Limonciello
Cc: platform-driver-x86, linux-kernel, Pali Rohár
This machine reports number of keyboard backlight led levels, instead of
value of the last led level index. Therefore max_brightness properly needs
to be subtracted by 1 to match led max_brightness API.
Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Reported-by: Gabriel M. Elder <gabriel@tekgnowsys.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=196913
---
Changes since v1:
* Update kbd_info.levels at initialization time based on quirk
---
drivers/platform/x86/dell-laptop.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index f42159f..3f9be8a 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -49,6 +49,7 @@
struct quirk_entry {
u8 touchpad_led;
+ u8 kbd_led_num_of_levels_instead_of_last_index;
int needs_kbd_timeouts;
/*
@@ -79,6 +80,10 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
.kbd_timeouts = { 0, 5, 15, 60, 5 * 60, 15 * 60, -1 },
};
+static struct quirk_entry quirk_dell_latitude_e6410 = {
+ .kbd_led_num_of_levels_instead_of_last_index = 1,
+};
+
static struct platform_driver platform_driver = {
.driver = {
.name = "dell-laptop",
@@ -280,6 +285,15 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
},
.driver_data = &quirk_dell_xps13_9333,
},
+ {
+ .callback = dmi_matched,
+ .ident = "Dell Latitude E6410",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6410"),
+ },
+ .driver_data = &quirk_dell_latitude_e6410,
+ },
{ }
};
@@ -1200,6 +1214,9 @@ static int kbd_get_info(struct kbd_info *info)
units = (buffer->output[2] >> 8) & 0xFF;
info->levels = (buffer->output[2] >> 16) & 0xFF;
+ if (quirks && quirks->kbd_led_num_of_levels_instead_of_last_index && info->levels)
+ info->levels--;
+
if (units & BIT(0))
info->seconds = (buffer->output[3] >> 0) & 0xFF;
if (units & BIT(1))
--
1.7.9.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] dell-laptop: Fix keyboard led max_brightness property for Dell Latitude E6410
2017-10-18 18:06 ` [PATCH v2] " Pali Rohár
@ 2017-10-18 19:09 ` Andy Shevchenko
2017-10-18 19:14 ` Pali Rohár
2017-10-18 20:00 ` Darren Hart
1 sibling, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2017-10-18 19:09 UTC (permalink / raw)
To: Pali Rohár
Cc: Matthew Garrett, Darren Hart, Andy Shevchenko, Gabriel M. Elder,
Gabriele Mazzotta, Mario Limonciello, Platform Driver,
linux-kernel
On Wed, Oct 18, 2017 at 9:06 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> This machine reports number of keyboard backlight led levels, instead of
> value of the last led level index. Therefore max_brightness properly needs
> to be subtracted by 1 to match led max_brightness API.
>
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Reported-by: Gabriel M. Elder <gabriel@tekgnowsys.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=196913
> + u8 kbd_led_num_of_levels_instead_of_last_index;
Sorry for last minute review comments.
First of all, can it be just boolean?
Naming... Yes, too hard as always was, is and will be.
What about just
kbd_led_use_levels?
Also, does it make sense to create a quirks as an unsigned long and
put there corresponding bits with definitions?
/* Comment what is this for */
#define QUIRK_KBD_LED_USE_LEVELS 0
unsigned long quirks;
?
> + if (quirks && quirks->kbd_led_num_of_levels_instead_of_last_index && info->levels)
> + info->levels--;
With last suggestion if becomes something like
if (quirks & BIT(QUIRK_KBD_LED_USE_LEVELS) && info->levels)
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] dell-laptop: Fix keyboard led max_brightness property for Dell Latitude E6410
2017-10-18 19:09 ` Andy Shevchenko
@ 2017-10-18 19:14 ` Pali Rohár
0 siblings, 0 replies; 7+ messages in thread
From: Pali Rohár @ 2017-10-18 19:14 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Matthew Garrett, Darren Hart, Andy Shevchenko, Gabriel M. Elder,
Gabriele Mazzotta, Mario Limonciello, Platform Driver,
linux-kernel
On Wednesday 18 October 2017 22:09:41 Andy Shevchenko wrote:
> On Wed, Oct 18, 2017 at 9:06 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > This machine reports number of keyboard backlight led levels, instead of
> > value of the last led level index. Therefore max_brightness properly needs
> > to be subtracted by 1 to match led max_brightness API.
> >
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > Reported-by: Gabriel M. Elder <gabriel@tekgnowsys.com>
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=196913
>
> > + u8 kbd_led_num_of_levels_instead_of_last_index;
>
> Sorry for last minute review comments.
>
> First of all, can it be just boolean?
Yes, touchpad_led and needs_kbd_timeouts are booleans too.
> Naming... Yes, too hard as always was, is and will be.
>
> What about just
>
> kbd_led_use_levels?
If you code:
if (quirks->kbd_led_use_levels)
info->levels--;
Then I think it does not help reader what that quirk represent.
> Also, does it make sense to create a quirks as an unsigned long and
> put there corresponding bits with definitions?
Problem is that part of quirk structure is variable length array
kbd_timeouts. So one unsigned long does not help.
> /* Comment what is this for */
> #define QUIRK_KBD_LED_USE_LEVELS 0
>
> unsigned long quirks;
>
> ?
>
> > + if (quirks && quirks->kbd_led_num_of_levels_instead_of_last_index && info->levels)
> > + info->levels--;
>
> With last suggestion if becomes something like
>
> if (quirks & BIT(QUIRK_KBD_LED_USE_LEVELS) && info->levels)
>
>
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] dell-laptop: Fix keyboard led max_brightness property for Dell Latitude E6410
2017-10-18 18:06 ` [PATCH v2] " Pali Rohár
2017-10-18 19:09 ` Andy Shevchenko
@ 2017-10-18 20:00 ` Darren Hart
1 sibling, 0 replies; 7+ messages in thread
From: Darren Hart @ 2017-10-18 20:00 UTC (permalink / raw)
To: Pali Rohár
Cc: Matthew Garrett, Andy Shevchenko, Gabriel M. Elder,
Gabriele Mazzotta, Mario.Limonciello, platform-driver-x86,
linux-kernel
On Wed, Oct 18, 2017 at 08:06:01PM +0200, Pali Rohár wrote:
> This machine reports number of keyboard backlight led levels, instead of
> value of the last led level index. Therefore max_brightness properly needs
> to be subtracted by 1 to match led max_brightness API.
>
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Reported-by: Gabriel M. Elder <gabriel@tekgnowsys.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=196913
> ---
> Changes since v1:
> * Update kbd_info.levels at initialization time based on quirk
This looks much cleaner IMO, thanks.
> ---
> drivers/platform/x86/dell-laptop.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index f42159f..3f9be8a 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -49,6 +49,7 @@
>
> struct quirk_entry {
> u8 touchpad_led;
> + u8 kbd_led_num_of_levels_instead_of_last_index;
OK, I forgot to comment on this one :-)
A name should be descriptive, but that's just too much. So let's use something
shorter, and add a comment if one is really needed to explain what it is.
Something like:
u8 kbd_led_reports_num_levels is shorter, but still awfully long.
u8 kbd_led_num_levels is shorter, but obviously looks like it should be the
levels value instead of a bool. So the above is probably best.
>
> int needs_kbd_timeouts;
> /*
> @@ -79,6 +80,10 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
> .kbd_timeouts = { 0, 5, 15, 60, 5 * 60, 15 * 60, -1 },
> };
>
> +static struct quirk_entry quirk_dell_latitude_e6410 = {
> + .kbd_led_num_of_levels_instead_of_last_index = 1,
> +};
> +
> static struct platform_driver platform_driver = {
> .driver = {
> .name = "dell-laptop",
> @@ -280,6 +285,15 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
> },
> .driver_data = &quirk_dell_xps13_9333,
> },
> + {
> + .callback = dmi_matched,
> + .ident = "Dell Latitude E6410",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6410"),
> + },
> + .driver_data = &quirk_dell_latitude_e6410,
> + },
> { }
> };
>
> @@ -1200,6 +1214,9 @@ static int kbd_get_info(struct kbd_info *info)
> units = (buffer->output[2] >> 8) & 0xFF;
> info->levels = (buffer->output[2] >> 16) & 0xFF;
>
> + if (quirks && quirks->kbd_led_num_of_levels_instead_of_last_index && info->levels)
> + info->levels--;
> +
> if (units & BIT(0))
> info->seconds = (buffer->output[3] >> 0) & 0xFF;
> if (units & BIT(1))
> --
> 1.7.9.5
>
>
--
Darren Hart
VMware Open Source Technology Center
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-10-21 17:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-19 18:20 [PATCH v2] dell-laptop: Fix keyboard led max_brightness property for Dell Latitude E6410 Gabriel M. Elder
2017-10-19 18:31 ` Pali Rohár
-- strict thread matches above, loose matches on Subject: below --
2017-10-21 17:55 Gabriel M. Elder
2017-10-15 16:03 [PATCH] " Pali Rohár
2017-10-18 18:06 ` [PATCH v2] " Pali Rohár
2017-10-18 19:09 ` Andy Shevchenko
2017-10-18 19:14 ` Pali Rohár
2017-10-18 20:00 ` Darren Hart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).