* 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
* 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, 0 replies; 7+ messages in thread
From: Pali Rohár @ 2017-10-19 18:31 UTC (permalink / raw)
To: Gabriel M. Elder
Cc: Darren Hart, Matthew Garrett, Andy Shevchenko, Gabriele Mazzotta,
Mario.Limonciello, platform-driver-x86, linux-kernel
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
^ 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-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-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
* 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 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
* [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
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-21 17:55 [PATCH v2] dell-laptop: Fix keyboard led max_brightness property for Dell Latitude E6410 Gabriel M. Elder
-- strict thread matches above, loose matches on Subject: below --
2017-10-19 18:20 Gabriel M. Elder
2017-10-19 18:31 ` Pali Rohár
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).