Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight
On Thu, Feb 19, 2015 at 11:58:29AM +0100, Gabriele Mazzotta wrote: > This patch adds the support for the configuration of the keyboard > backlight on supported Dell laptops. > > With this patch it is possible to set: > * keyboard backlight level > * timeout after which the backlight will be automatically turned off > * input activity triggers (keyboard, touchpad, mouse) that enable the > backlight > * ambient light settings > > The settings are exposed via /sys/class/leds/dell::kbd_backlight/ > > The code is based on the newly released documentation by Dell in the > libsmbios project. > > Signed-off-by: Pali Rohár > Signed-off-by: Gabriele Mazzotta > Cc: Dan Carpenter Queued for 4.1, thanks everyone. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight
On Sun, Feb 22, 2015 at 12:04:23PM +0100, Pali Rohár wrote: > On Thursday 19 February 2015 11:58:29 Gabriele Mazzotta wrote: > > This patch adds the support for the configuration of the > > keyboard backlight on supported Dell laptops. > > > > With this patch it is possible to set: > > * keyboard backlight level > > * timeout after which the backlight will be automatically > > turned off * input activity triggers (keyboard, touchpad, > > mouse) that enable the backlight * ambient light settings > > > > The settings are exposed via > > /sys/class/leds/dell::kbd_backlight/ > > > > The code is based on the newly released documentation by Dell > > in the libsmbios project. > > > > Signed-off-by: Pali Rohár > > Signed-off-by: Gabriele Mazzotta > > Cc: Dan Carpenter > > --- > > .../ABI/testing/sysfs-platform-dell-laptop | 69 ++ > > drivers/platform/x86/dell-laptop.c | 1089 > > +++- 2 files changed, 1152 insertions(+), 6 > > deletions(-) > > create mode 100644 > > Documentation/ABI/testing/sysfs-platform-dell-laptop > > > > This patch is same as combination of previous, just contains all > changes in single patch file. Difference between v3.19-rc5 and > current version is just als code was moved to separate functions > and als backlight can be enabled via new sysfs attribute (like in > second patch which was sent). > > I tested this patch on top of linus tree (commit 79513d0) and > keyboard backlight works fine. > > Darren, what do you think, can be this patch which brings > keyboard backlight support finally moved to mainline kernel? So sorry for the delay folks, bad week :( This landed too late for 3.20... her durr 4.0, but should make 4.1. Expect to see it in next in the next few days. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight
On Thursday 19 February 2015 11:58:29 Gabriele Mazzotta wrote: > This patch adds the support for the configuration of the > keyboard backlight on supported Dell laptops. > > With this patch it is possible to set: > * keyboard backlight level > * timeout after which the backlight will be automatically > turned off * input activity triggers (keyboard, touchpad, > mouse) that enable the backlight * ambient light settings > > The settings are exposed via > /sys/class/leds/dell::kbd_backlight/ > > The code is based on the newly released documentation by Dell > in the libsmbios project. > > Signed-off-by: Pali Rohár > Signed-off-by: Gabriele Mazzotta > Cc: Dan Carpenter > --- > .../ABI/testing/sysfs-platform-dell-laptop | 69 ++ > drivers/platform/x86/dell-laptop.c | 1089 > +++- 2 files changed, 1152 insertions(+), 6 > deletions(-) > create mode 100644 > Documentation/ABI/testing/sysfs-platform-dell-laptop > This patch is same as combination of previous, just contains all changes in single patch file. Difference between v3.19-rc5 and current version is just als code was moved to separate functions and als backlight can be enabled via new sysfs attribute (like in second patch which was sent). I tested this patch on top of linus tree (commit 79513d0) and keyboard backlight works fine. Darren, what do you think, can be this patch which brings keyboard backlight support finally moved to mainline kernel? -- Pali Rohár pali.ro...@gmail.com signature.asc Description: This is a digitally signed message part.
[PATCH] platform: x86: dell-laptop: Add support for keyboard backlight
This patch adds the support for the configuration of the keyboard backlight on supported Dell laptops. With this patch it is possible to set: * keyboard backlight level * timeout after which the backlight will be automatically turned off * input activity triggers (keyboard, touchpad, mouse) that enable the backlight * ambient light settings The settings are exposed via /sys/class/leds/dell::kbd_backlight/ The code is based on the newly released documentation by Dell in the libsmbios project. Signed-off-by: Pali Rohár Signed-off-by: Gabriele Mazzotta Cc: Dan Carpenter --- .../ABI/testing/sysfs-platform-dell-laptop | 69 ++ drivers/platform/x86/dell-laptop.c | 1089 +++- 2 files changed, 1152 insertions(+), 6 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-laptop diff --git a/Documentation/ABI/testing/sysfs-platform-dell-laptop b/Documentation/ABI/testing/sysfs-platform-dell-laptop new file mode 100644 index 000..8c6a0b8 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-platform-dell-laptop @@ -0,0 +1,69 @@ +What: /sys/class/leds/dell::kbd_backlight/als_enabled +Date: December 2014 +KernelVersion: 3.19 +Contact: Gabriele Mazzotta , + Pali Rohár +Description: + This file allows to control the automatic keyboard + illumination mode on some systems that have an ambient + light sensor. Write 1 to this file to enable the auto + mode, 0 to disable it. + +What: /sys/class/leds/dell::kbd_backlight/als_setting +Date: December 2014 +KernelVersion: 3.19 +Contact: Gabriele Mazzotta , + Pali Rohár +Description: + This file allows to specifiy the on/off threshold value, + as reported by the ambient light sensor. + +What: /sys/class/leds/dell::kbd_backlight/start_triggers +Date: December 2014 +KernelVersion: 3.19 +Contact: Gabriele Mazzotta , + Pali Rohár +Description: + This file allows to control the input triggers that + turn on the keyboard backlight illumination that is + disabled because of inactivity. + Read the file to see the triggers available. The ones + enabled are preceded by '+', those disabled by '-'. + + To enable a trigger, write its name preceded by '+' to + this file. To disable a trigger, write its name preceded + by '-' instead. + + For example, to enable the keyboard as trigger run: + echo +keyboard > /sys/class/leds/dell::kbd_backlight/start_triggers + To disable it: + echo -keyboard > /sys/class/leds/dell::kbd_backlight/start_triggers + + Note that not all the available triggers can be configured. + +What: /sys/class/leds/dell::kbd_backlight/stop_timeout +Date: December 2014 +KernelVersion: 3.19 +Contact: Gabriele Mazzotta , + Pali Rohár +Description: + This file allows to specify the interval after which the + keyboard illumination is disabled because of inactivity. + The timeouts are expressed in seconds, minutes, hours and + days, for which the symbols are 's', 'm', 'h' and 'd' + respectively. + + To configure the timeout, write to this file a value along + with any the above units. If no unit is specified, the value + is assumed to be expressed in seconds. + + For example, to set the timeout to 10 minutes run: + echo 10m > /sys/class/leds/dell::kbd_backlight/stop_timeout + + Note that when this file is read, the returned value might be + expressed in a different unit than the one used when the timeout + was set. + + Also note that only some timeouts are supported and that + some systems might fall back to a specific timeout in case + an invalid timeout is written to this file. diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c index 3d21efe..d688d80 100644 --- a/drivers/platform/x86/dell-laptop.c +++ b/drivers/platform/x86/dell-laptop.c @@ -2,9 +2,11 @@ * Driver for Dell laptop extras * * Copyright (c) Red Hat + * Copyright (c) 2014 Gabriele Mazzotta + * Copyright (c) 2014 Pali Rohár * - * Based on documentation in the libsmbios package, Copyright (C) 2005 Dell - * Inc. + * Based on documentation in the libsmbios package: + * Copyright (C) 2005-2014 Dell Inc. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -32,6 +34,13 @@ #include "../../firmware/dcdbas.h" #define BRIGHTNESS_TOKEN 0x7d +#define KBD_LED
Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight
On Friday 21 November 2014 23:09:40 Darren Hart wrote:
> On Sat, Nov 22, 2014 at 07:46:25PM +0100, Pali Rohár wrote:
> > > > 0 Completed successfully
> > > > -1 Completed with error
> > > > -2 Function not supported
> > > >
> > > > So we can return something other too (not always
> > > > -EINVAL). Do you have any idea which errno should we
> > > > return for -1 and -2?
> > >
> > > For -1, I should think -EIO (I/O Error)
> > > For -2, I'd expect -ENXIO (No such device or address)
> >
> > What about -ENOSYS for -2?
>
> No. This specific topic came up at kernel summit this year.
> ENOSYS is specifically for not implemented system calls.
>
Ok, I will use -ENXIO.
> > > > > > + if (convert) {
> > > > > > + /* NOTE: this switch fall down */
> > > > >
> > > > > "fall down" ? As in, it intentionally doesn't have
> > > > > breaks?
> > > >
> > > > This code convert "value" in "units" to new value in
> > > > minutes units. So for unit == days it is: 24*60*60...
> > > > So no breaks.
> > >
> > > Right, so the language of the comment just wasn't clear,
> > > try:
> > >
> > > /* Convert value from seconds to minutes */
> >
> > Err... to seconds :-) But OK, I will change comment.
>
> Oops, duh.
>
> /* Convert value from current units to seconds. */
>
> > > > > > + switch (unit) {
> > > > > > + case KBD_TIMEOUT_DAYS:
> > > > > > + value *= 24;
> > > > > > + case KBD_TIMEOUT_HOURS:
> > > > > > + value *= 60;
> > > > > > + case KBD_TIMEOUT_MINUTES:
> > > > > > + value *= 60;
> > > > > > + unit = KBD_TIMEOUT_SECONDS;
> > > > > > + }
--
Pali Rohár
pali.ro...@gmail.com
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight
On Sat, Nov 22, 2014 at 07:46:25PM +0100, Pali Rohár wrote:
> > > 0 Completed successfully
> > > -1 Completed with error
> > > -2 Function not supported
> > >
> > > So we can return something other too (not always -EINVAL).
> > > Do you have any idea which errno should we return for -1
> > > and -2?
> >
> > For -1, I should think -EIO (I/O Error)
> > For -2, I'd expect -ENXIO (No such device or address)
> >
>
> What about -ENOSYS for -2?
No. This specific topic came up at kernel summit this year. ENOSYS is
specifically for not implemented system calls.
>
> > > > > + if (convert) {
> > > > > + /* NOTE: this switch fall down */
> > > >
> > > > "fall down" ? As in, it intentionally doesn't have breaks?
> > >
> > > This code convert "value" in "units" to new value in minutes
> > > units. So for unit == days it is: 24*60*60... So no breaks.
> >
> > Right, so the language of the comment just wasn't clear, try:
> >
> > /* Convert value from seconds to minutes */
> >
>
> Err... to seconds :-) But OK, I will change comment.
Oops, duh.
/* Convert value from current units to seconds. */
>
> > > > > + switch (unit) {
> > > > > + case KBD_TIMEOUT_DAYS:
> > > > > + value *= 24;
> > > > > + case KBD_TIMEOUT_HOURS:
> > > > > + value *= 60;
> > > > > + case KBD_TIMEOUT_MINUTES:
> > > > > + value *= 60;
> > > > > + unit = KBD_TIMEOUT_SECONDS;
> > > > > + }
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight
On Friday 21 November 2014 21:39:30 Darren Hart wrote:
> On Wed, Nov 19, 2014 at 09:41:20PM +0100, Pali Rohár wrote:
> > Hello,
>
> Hi Pali,
>
> > I removed other lines so mail is not too long.
>
> > On Wednesday 19 November 2014 19:34:16 Darren Hart wrote:
> ...
>
> > > > +}
> > > > +
> > > > +static unsigned int kbd_get_max_level(void)
> > > > +{
> > > > + if (kbd_info.levels != 0)
> > > > + return kbd_info.levels;
> > >
> > > This test does nothing? if it is 0, you still return 0
> > > below :-)
> > >
> > > > + if (kbd_mode_levels_count > 0)
> > > > + return kbd_mode_levels_count - 1;
> > > > + return 0;
> > >
> > > So the function is:
> > >
> > > return kbd_mode_levels_count > 0 ? kbd_mode_levels_count -
> > > 1 : kbd_info.levels;
> > >
> > > The if blocks are more legible, so that's fine, but the
> > > first one doesn't seem to do anything and you can replace
> > > the final return with return kbd_info.levels.
> >
> > There are two main way for setting keyboard backlight level.
> > One is setting level register and other one is setting
> > special keyboard mode. And some dell laptops support only
> > first and some only second. So this code choose first
> > available/valid method and then return correct data. I'm
> > not sure if those two methods are only one and also do not
> > know if in future there will be something other. Because of
> > this I use code pattern:
> >
> > if (check_method_1) return value_method_1;
> > if (check_method_2) return value_method_2;
> > ...
> > return unsupported;
> >
> > Same pattern logic is used in all functions which doing
> > something with keyboard backlight level. (I will not other
> > functions).
>
> Fair enough. Clear, legible, consistent coding is preferable
> to a few micro optimizations that the compiler is likely to
> catch anyway. I withdraw the comment :-)
>
Ok.
> > > > +static int kbd_set_state(struct kbd_state *state)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + get_buffer();
> > > > + buffer->input[0] = 0x2;
> > > > + buffer->input[1] = BIT(state->mode_bit) & 0x;
> > > > + buffer->input[1] |= (state->triggers & 0xFF) << 16;
> > > > + buffer->input[1] |= (state->timeout_value & 0x3F) <<
> > > > 24; + buffer->input[1] |= (state->timeout_unit & 0x3)
> > > > << 30; +buffer->input[2] = state->als_setting & 0xFF;
> > > > + buffer->input[2] |= (state->level & 0xFF) << 16;
> > > > + dell_send_request(buffer, 4, 11);
> > > > + ret = buffer->output[0];
> > > > + release_buffer();
> > > > +
> > > > + if (ret)
> > > > + return -EINVAL;
> > >
> > > Also, is EINVAL right here and elsewhere? Or did something
> > > fail? EXIO?
> >
> > According to Dell documentation, return value is stored in
> > buffer->output[0] and can be:
> >
> > 0 Completed successfully
> > -1 Completed with error
> > -2 Function not supported
> >
> > So we can return something other too (not always -EINVAL).
> > Do you have any idea which errno should we return for -1
> > and -2?
>
> For -1, I should think -EIO (I/O Error)
> For -2, I'd expect -ENXIO (No such device or address)
>
What about -ENOSYS for -2?
> Cc Rafael, Mika, linux-acpi in case they have a better idea.
>
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int kbd_set_state_safe(struct kbd_state *state,
> > > > struct kbd_state *old) +{
> > > > + int ret;
> > > > +
> > > > + ret = kbd_set_state(state);
> > > > + if (ret == 0)
> > > > + return 0;
> > > > +
> > > > + if (kbd_set_state(old))
> > > > + pr_err("Setting old previous keyboard state
> >
> > failed\n");
> >
> > > And if we got an error and kbd_set_state(old) doesn't
> > > return !0, what's the state of things? Still a failure a
> > > presume?
> >
> > In some cases some laptops do not have to support
> > everything. And when I (and Gabriele too) tried to set
> > something unsupported Dell BIOS just resetted all settings
> > to some default values. So this function try to set new
> > state and if it fails then it try to restore previous
> > settings.
>
> OK, that deserves a comment then as the rationale isn't
> obvious.
>
Ok, I will add comment.
> > > > +
> > > > + return ret;
> > > > +}
> > > >
> > > > +static void kbd_init(void)
> > > > +{
> > > > + struct kbd_state state;
> > > > + int ret;
> > > > + int i;
> > > > +
> > > > + ret = kbd_get_info(&kbd_info);
> > > > +
> > > > + if (ret == 0) {
> > > > +
> > >
> > > Checking for success, let's invert and avoid the
> > > indentation here too.
> >
> > Again this is hard and not possible. This function first
> > process data from kbd_get_info (if does not fail), then
> > process data for kbd_tokens (via function find_token_id)
> > and then set kbd_led_present to true if at least
> > kbd_get_info or kbd_tokens succed.
>
> The goal here
Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight
On Wed, Nov 19, 2014 at 09:41:20PM +0100, Pali Rohár wrote:
> Hello,
Hi Pali,
>
> I removed other lines so mail is not too long.
>
> On Wednesday 19 November 2014 19:34:16 Darren Hart wrote:
...
> > > +}
> > > +
> > > +static unsigned int kbd_get_max_level(void)
> > > +{
> > > + if (kbd_info.levels != 0)
> > > + return kbd_info.levels;
> >
> > This test does nothing? if it is 0, you still return 0
> > below :-)
> >
> > > + if (kbd_mode_levels_count > 0)
> > > + return kbd_mode_levels_count - 1;
> > > + return 0;
> >
> > So the function is:
> >
> > return kbd_mode_levels_count > 0 ? kbd_mode_levels_count - 1 :
> > kbd_info.levels;
> >
> > The if blocks are more legible, so that's fine, but the first
> > one doesn't seem to do anything and you can replace the final
> > return with return kbd_info.levels.
> >
>
> There are two main way for setting keyboard backlight level. One
> is setting level register and other one is setting special
> keyboard mode. And some dell laptops support only first and some
> only second. So this code choose first available/valid method and
> then return correct data. I'm not sure if those two methods are
> only one and also do not know if in future there will be
> something other. Because of this I use code pattern:
>
> if (check_method_1) return value_method_1;
> if (check_method_2) return value_method_2;
> ...
> return unsupported;
>
> Same pattern logic is used in all functions which doing something
> with keyboard backlight level. (I will not other functions).
Fair enough. Clear, legible, consistent coding is preferable to a few micro
optimizations that the compiler is likely to catch anyway. I withdraw the
comment :-)
>
> > > +static int kbd_set_state(struct kbd_state *state)
> > > +{
> > > + int ret;
> > > +
> > > + get_buffer();
> > > + buffer->input[0] = 0x2;
> > > + buffer->input[1] = BIT(state->mode_bit) & 0x;
> > > + buffer->input[1] |= (state->triggers & 0xFF) << 16;
> > > + buffer->input[1] |= (state->timeout_value & 0x3F) << 24;
> > > + buffer->input[1] |= (state->timeout_unit & 0x3) << 30;
> > > + buffer->input[2] = state->als_setting & 0xFF;
> > > + buffer->input[2] |= (state->level & 0xFF) << 16;
> > > + dell_send_request(buffer, 4, 11);
> > > + ret = buffer->output[0];
> > > + release_buffer();
> > > +
> > > + if (ret)
> > > + return -EINVAL;
> >
> > Also, is EINVAL right here and elsewhere? Or did something
> > fail? EXIO?
> >
>
> According to Dell documentation, return value is stored in
> buffer->output[0] and can be:
>
> 0 Completed successfully
> -1 Completed with error
> -2 Function not supported
>
> So we can return something other too (not always -EINVAL). Do you
> have any idea which errno should we return for -1 and -2?
For -1, I should think -EIO (I/O Error)
For -2, I'd expect -ENXIO (No such device or address)
Cc Rafael, Mika, linux-acpi in case they have a better idea.
>
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int kbd_set_state_safe(struct kbd_state *state,
> > > struct kbd_state *old) +{
> > > + int ret;
> > > +
> > > + ret = kbd_set_state(state);
> > > + if (ret == 0)
> > > + return 0;
> > > +
> > > + if (kbd_set_state(old))
> > > + pr_err("Setting old previous keyboard state
> failed\n");
> >
> > And if we got an error and kbd_set_state(old) doesn't return
> > !0, what's the state of things? Still a failure a presume?
> >
>
> In some cases some laptops do not have to support everything. And
> when I (and Gabriele too) tried to set something unsupported Dell
> BIOS just resetted all settings to some default values. So this
> function try to set new state and if it fails then it try to
> restore previous settings.
OK, that deserves a comment then as the rationale isn't obvious.
>
> > > +
> > > + return ret;
> > > +}
>
> > > +static void kbd_init(void)
> > > +{
> > > + struct kbd_state state;
> > > + int ret;
> > > + int i;
> > > +
> > > + ret = kbd_get_info(&kbd_info);
> > > +
> > > + if (ret == 0) {
> > > +
> >
> > Checking for success, let's invert and avoid the indentation
> > here too.
> >
>
> Again this is hard and not possible. This function first process
> data from kbd_get_info (if does not fail), then process data for
> kbd_tokens (via function find_token_id) and then set
> kbd_led_present to true if at least kbd_get_info or kbd_tokens
> succed.
The goal here is to avoid more than 3 levels of indentations for improved
legibility. Often there are logical inversions and such we can make to
accomplish this. When that fails, we break things up into functions, static
inlines, etc.
For reference:
https://lkml.org/lkml/2007/6/15/449
Which elaborates on CodingStyle Chapter 1: Indentation a bit.
...
> > > +static ssize_t kbd_led_timeout_store(struct device *dev,
> > > + struct device_attribute *attr,
> > > + const char *buf, size_t count)
> > > +{
> > > + struct kbd_
Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight
Hello,
I removed other lines so mail is not too long.
On Wednesday 19 November 2014 19:34:16 Darren Hart wrote:
> > +static int kbd_get_info(struct kbd_info *info)
> > +{
> > + u8 units;
> > + int ret;
> > +
> > + get_buffer();
> > +
> > + buffer->input[0] = 0x0;
> > + dell_send_request(buffer, 4, 11);
> > + ret = buffer->output[0];
> > +
> > + if (ret == 0) {
>
> Generally speaking, check for error to keep the main logic
> from getting indented.
>
> if (buffer->output[0]) {
> ret = -EINVAL;
> goto out;
> }
>
> > + info->modes = buffer->output[1] & 0x;
> > + info->type = (buffer->output[1] >> 24) & 0xFF;
> > + info->triggers = buffer->output[2] & 0xFF;
> > + units = (buffer->output[2] >> 8) & 0xFF;
> > + info->levels = (buffer->output[2] >> 16) & 0xFF;
> > + if (units & BIT(0))
> > + info->seconds = (buffer->output[3] >> 0) & 0xFF;
> > + if (units & BIT(1))
> > + info->minutes = (buffer->output[3] >> 8) & 0xFF;
> > + if (units & BIT(2))
> > + info->hours = (buffer->output[3] >> 16) & 0xFF;
> > + if (units & BIT(3))
> > + info->days = (buffer->output[3] >> 24) & 0xFF;
> > + }
> > +
>
> out:
> > + release_buffer();
> > +
> > + if (ret)
> > + return -EINVAL;
>
> Drop this.
>
> > + return 0;
>
> return ret;
>
> In this particular case, it might be shorter by a line or two
> to drop the ret variable and simply release_buffer() and
> return -EINVAL in the error check above and just return 0
> here.
>
Ok. But somewhere it is not possible.
> > +}
> > +
> > +static unsigned int kbd_get_max_level(void)
> > +{
> > + if (kbd_info.levels != 0)
> > + return kbd_info.levels;
>
> This test does nothing? if it is 0, you still return 0
> below :-)
>
> > + if (kbd_mode_levels_count > 0)
> > + return kbd_mode_levels_count - 1;
> > + return 0;
>
> So the function is:
>
> return kbd_mode_levels_count > 0 ? kbd_mode_levels_count - 1 :
> kbd_info.levels;
>
> The if blocks are more legible, so that's fine, but the first
> one doesn't seem to do anything and you can replace the final
> return with return kbd_info.levels.
>
There are two main way for setting keyboard backlight level. One
is setting level register and other one is setting special
keyboard mode. And some dell laptops support only first and some
only second. So this code choose first available/valid method and
then return correct data. I'm not sure if those two methods are
only one and also do not know if in future there will be
something other. Because of this I use code pattern:
if (check_method_1) return value_method_1;
if (check_method_2) return value_method_2;
...
return unsupported;
Same pattern logic is used in all functions which doing something
with keyboard backlight level. (I will not other functions).
> > +static int kbd_set_state(struct kbd_state *state)
> > +{
> > + int ret;
> > +
> > + get_buffer();
> > + buffer->input[0] = 0x2;
> > + buffer->input[1] = BIT(state->mode_bit) & 0x;
> > + buffer->input[1] |= (state->triggers & 0xFF) << 16;
> > + buffer->input[1] |= (state->timeout_value & 0x3F) << 24;
> > + buffer->input[1] |= (state->timeout_unit & 0x3) << 30;
> > + buffer->input[2] = state->als_setting & 0xFF;
> > + buffer->input[2] |= (state->level & 0xFF) << 16;
> > + dell_send_request(buffer, 4, 11);
> > + ret = buffer->output[0];
> > + release_buffer();
> > +
> > + if (ret)
> > + return -EINVAL;
>
> Also, is EINVAL right here and elsewhere? Or did something
> fail? EXIO?
>
According to Dell documentation, return value is stored in
buffer->output[0] and can be:
0 Completed successfully
-1 Completed with error
-2 Function not supported
So we can return something other too (not always -EINVAL). Do you
have any idea which errno should we return for -1 and -2?
> > +
> > + return 0;
> > +}
> > +
> > +static int kbd_set_state_safe(struct kbd_state *state,
> > struct kbd_state *old) +{
> > + int ret;
> > +
> > + ret = kbd_set_state(state);
> > + if (ret == 0)
> > + return 0;
> > +
> > + if (kbd_set_state(old))
> > + pr_err("Setting old previous keyboard state
failed\n");
>
> And if we got an error and kbd_set_state(old) doesn't return
> !0, what's the state of things? Still a failure a presume?
>
In some cases some laptops do not have to support everything. And
when I (and Gabriele too) tried to set something unsupported Dell
BIOS just resetted all settings to some default values. So this
function try to set new state and if it fails then it try to
restore previous settings.
> > +
> > + return ret;
> > +}
> > +static void kbd_init(void)
> > +{
> > + struct kbd_state state;
> > + int ret;
> > + int i;
> > +
> > + ret = kbd_get_info(&kbd_info);
> > +
> > + if (ret =
Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight
On Wed, Nov 19, 2014 at 08:51:28PM +0100, Pali Rohár wrote: > On Wednesday 19 November 2014 20:23:36 Matthew Garrett wrote: > > On Wed, Nov 19, 2014 at 10:34:16AM -0800, Darren Hart wrote: > > > I'm somewhat concerned that this patch doubles the size of > > > this driver. When we're adding this much code, I have to > > > ask - does it make sense to grow this driver rather than > > > create a new one? > > > > There'd be a fair amount of code duplication in splitting it. > > > > Yes. dell-laptop.ko implements functions for doing Dell SMBIOS > calls which are needed for keyboard backlight. > > > > There is no ACPI backlight driver on these systems? We need > > > a platform driver? > > > > ACPI doesn't specify keyboard backlight control, so this ends > > up being very vendor specific. > > dell-laptop.ko is not ACPI driver. It is using Dell SMBIOS calls. > And I do not know about Dell specific ACPI interface for keyboard > backlight. So Darren, ask this question someone from Dell. Right, I asked because there is a block in the original driver to defer to an ACPI backlight driver if it exists. I understand this is a keyboard backlight driver and is different, but I wanted to check. So let's go ahead and move forward with dell-laptop.c being the right place for this addition. Thanks, -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight
On Wednesday 19 November 2014 20:23:36 Matthew Garrett wrote: > On Wed, Nov 19, 2014 at 10:34:16AM -0800, Darren Hart wrote: > > I'm somewhat concerned that this patch doubles the size of > > this driver. When we're adding this much code, I have to > > ask - does it make sense to grow this driver rather than > > create a new one? > > There'd be a fair amount of code duplication in splitting it. > Yes. dell-laptop.ko implements functions for doing Dell SMBIOS calls which are needed for keyboard backlight. > > There is no ACPI backlight driver on these systems? We need > > a platform driver? > > ACPI doesn't specify keyboard backlight control, so this ends > up being very vendor specific. dell-laptop.ko is not ACPI driver. It is using Dell SMBIOS calls. And I do not know about Dell specific ACPI interface for keyboard backlight. So Darren, ask this question someone from Dell. -- Pali Rohár pali.ro...@gmail.com signature.asc Description: This is a digitally signed message part.
Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight
On Wed, Nov 19, 2014 at 10:34:16AM -0800, Darren Hart wrote: > I'm somewhat concerned that this patch doubles the size of this driver. When > we're adding this much code, I have to ask - does it make sense to grow this > driver rather than create a new one? There'd be a fair amount of code duplication in splitting it. > There is no ACPI backlight driver on these systems? We need a platform driver? ACPI doesn't specify keyboard backlight control, so this ends up being very vendor specific. -- Matthew Garrett | mj...@srcf.ucam.org -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight
On Fri, Nov 14, 2014 at 01:23:33PM +0100, Pali Rohár wrote:
> This patch adds support for configuring keyboard backlight settings on
> supported
> Dell laptops. It exports kernel leds interface and uses Dell SMBIOS tokens or
> keyboard class interface.
>
> With this patch it is possible to set:
> * keyboard backlight level
> * timeout after which will be backlight automatically turned off
> * input activity triggers (keyboard, touchpad, mouse) which enable backlight
> * ambient light settings
>
> Settings are exported via sysfs:
> /sys/class/leds/dell::kbd_backlight/
>
> Code is based on newly released documentation by Dell in libsmbios project.
>
> Signed-off-by: Pali Rohár
> Signed-off-by: Gabriele Mazzotta
Hi Pali,
Apologies for the delay.
I'm somewhat concerned that this patch doubles the size of this driver. When
we're adding this much code, I have to ask - does it make sense to grow this
driver rather than create a new one?
There is no ACPI backlight driver on these systems? We need a platform driver?
Matthew, I'd appreciate your thoughts as you're listed as the module author.
My main commentary throughout is surrounding large indented logic blocks which
could be less nested by checking for errors and returning/jumping rather than
using an if (success) block for the main logic. There are various places where
some logic reduction would result in some significantly tighter code.
Specific comments below.
> ---
> drivers/platform/x86/dell-laptop.c | 1001
> +++-
> 1 file changed, 997 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/dell-laptop.c
> b/drivers/platform/x86/dell-laptop.c
> index 233d2ee..bf5b1cc 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -2,9 +2,11 @@
> * Driver for Dell laptop extras
> *
> * Copyright (c) Red Hat
> + * Copyright (c) 2014 Gabriele Mazzotta
> + * Copyright (c) 2014 Pali Rohár
> *
> - * Based on documentation in the libsmbios package, Copyright (C) 2005 Dell
> - * Inc.
> + * Based on documentation in the libsmbios package:
> + * Copyright (C) 2005-2014 Dell Inc.
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> @@ -32,6 +34,13 @@
> #include "../../firmware/dcdbas.h"
>
> #define BRIGHTNESS_TOKEN 0x7d
> +#define KBD_LED_OFF_TOKEN 0x01E1
> +#define KBD_LED_ON_TOKEN 0x01E2
> +#define KBD_LED_AUTO_TOKEN 0x01E3
> +#define KBD_LED_AUTO_25_TOKEN 0x02EA
> +#define KBD_LED_AUTO_50_TOKEN 0x02EB
> +#define KBD_LED_AUTO_75_TOKEN 0x02EC
> +#define KBD_LED_AUTO_100_TOKEN 0x02F6
>
> /* This structure will be modified by the firmware when we enter
> * system management mode, hence the volatiles */
> @@ -62,6 +71,10 @@ struct calling_interface_structure {
>
> struct quirk_entry {
> u8 touchpad_led;
> +
> + /* Ordered list of timeouts expressed in seconds.
> + * The list must end with -1 */
> + int kbd_timeouts[];
> };
>
> static struct quirk_entry *quirks;
> @@ -76,6 +89,10 @@ static int __init dmi_matched(const struct dmi_system_id
> *dmi)
> return 1;
> }
>
> +static struct quirk_entry quirk_dell_xps13_9333 = {
> + .kbd_timeouts = { 0, 5, 15, 60, 5*60, 15*60, -1 },
> +};
> +
> static int da_command_address;
> static int da_command_code;
> static int da_num_tokens;
> @@ -268,6 +285,15 @@ static const struct dmi_system_id dell_quirks[]
> __initconst = {
> },
> .driver_data = &quirk_dell_vostro_v130,
> },
> + {
> + .callback = dmi_matched,
> + .ident = "Dell XPS13 9333",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> + DMI_MATCH(DMI_PRODUCT_NAME, "XPS13 9333"),
> + },
> + .driver_data = &quirk_dell_xps13_9333,
> + },
> { }
> };
>
> @@ -332,17 +358,29 @@ static void __init find_tokens(const struct dmi_header
> *dm, void *dummy)
> }
> }
>
> -static int find_token_location(int tokenid)
> +static int find_token_id(int tokenid)
> {
> int i;
> +
> for (i = 0; i < da_num_tokens; i++) {
> if (da_tokens[i].tokenID == tokenid)
> - return da_tokens[i].location;
> + return i;
> }
>
> return -1;
> }
>
> +static int find_token_location(int tokenid)
> +{
> + int id;
> +
> + id = find_token_id(tokenid);
> + if (id == -1)
> + return -1;
> +
> + return da_tokens[id].location;
> +}
> +
> static struct calling_interface_buffer *
> dell_send_request(struct calling_interface_buffer *buffer, int class,
> int select)
> @@ -790,6 +828,956 @@ static void touchpad_led_exit(void)
> led_classdev_unregister(&touchpad_led);
> }
>
> +/* Derived from information in smbios-keyboard-ctl:
> +
> + cbClass 4
> + cbSelect 11
>
[PATCH] platform: x86: dell-laptop: Add support for keyboard backlight
This patch adds support for configuring keyboard backlight settings on supported
Dell laptops. It exports kernel leds interface and uses Dell SMBIOS tokens or
keyboard class interface.
With this patch it is possible to set:
* keyboard backlight level
* timeout after which will be backlight automatically turned off
* input activity triggers (keyboard, touchpad, mouse) which enable backlight
* ambient light settings
Settings are exported via sysfs:
/sys/class/leds/dell::kbd_backlight/
Code is based on newly released documentation by Dell in libsmbios project.
Signed-off-by: Pali Rohár
Signed-off-by: Gabriele Mazzotta
---
drivers/platform/x86/dell-laptop.c | 1001 +++-
1 file changed, 997 insertions(+), 4 deletions(-)
diff --git a/drivers/platform/x86/dell-laptop.c
b/drivers/platform/x86/dell-laptop.c
index 233d2ee..bf5b1cc 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -2,9 +2,11 @@
* Driver for Dell laptop extras
*
* Copyright (c) Red Hat
+ * Copyright (c) 2014 Gabriele Mazzotta
+ * Copyright (c) 2014 Pali Rohár
*
- * Based on documentation in the libsmbios package, Copyright (C) 2005 Dell
- * Inc.
+ * Based on documentation in the libsmbios package:
+ * Copyright (C) 2005-2014 Dell Inc.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
@@ -32,6 +34,13 @@
#include "../../firmware/dcdbas.h"
#define BRIGHTNESS_TOKEN 0x7d
+#define KBD_LED_OFF_TOKEN 0x01E1
+#define KBD_LED_ON_TOKEN 0x01E2
+#define KBD_LED_AUTO_TOKEN 0x01E3
+#define KBD_LED_AUTO_25_TOKEN 0x02EA
+#define KBD_LED_AUTO_50_TOKEN 0x02EB
+#define KBD_LED_AUTO_75_TOKEN 0x02EC
+#define KBD_LED_AUTO_100_TOKEN 0x02F6
/* This structure will be modified by the firmware when we enter
* system management mode, hence the volatiles */
@@ -62,6 +71,10 @@ struct calling_interface_structure {
struct quirk_entry {
u8 touchpad_led;
+
+ /* Ordered list of timeouts expressed in seconds.
+* The list must end with -1 */
+ int kbd_timeouts[];
};
static struct quirk_entry *quirks;
@@ -76,6 +89,10 @@ static int __init dmi_matched(const struct dmi_system_id
*dmi)
return 1;
}
+static struct quirk_entry quirk_dell_xps13_9333 = {
+ .kbd_timeouts = { 0, 5, 15, 60, 5*60, 15*60, -1 },
+};
+
static int da_command_address;
static int da_command_code;
static int da_num_tokens;
@@ -268,6 +285,15 @@ static const struct dmi_system_id dell_quirks[]
__initconst = {
},
.driver_data = &quirk_dell_vostro_v130,
},
+ {
+ .callback = dmi_matched,
+ .ident = "Dell XPS13 9333",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+ DMI_MATCH(DMI_PRODUCT_NAME, "XPS13 9333"),
+ },
+ .driver_data = &quirk_dell_xps13_9333,
+ },
{ }
};
@@ -332,17 +358,29 @@ static void __init find_tokens(const struct dmi_header
*dm, void *dummy)
}
}
-static int find_token_location(int tokenid)
+static int find_token_id(int tokenid)
{
int i;
+
for (i = 0; i < da_num_tokens; i++) {
if (da_tokens[i].tokenID == tokenid)
- return da_tokens[i].location;
+ return i;
}
return -1;
}
+static int find_token_location(int tokenid)
+{
+ int id;
+
+ id = find_token_id(tokenid);
+ if (id == -1)
+ return -1;
+
+ return da_tokens[id].location;
+}
+
static struct calling_interface_buffer *
dell_send_request(struct calling_interface_buffer *buffer, int class,
int select)
@@ -790,6 +828,956 @@ static void touchpad_led_exit(void)
led_classdev_unregister(&touchpad_led);
}
+/* Derived from information in smbios-keyboard-ctl:
+
+ cbClass 4
+ cbSelect 11
+ Keyboar illumination
+ cbArg1 determines the function to be performed
+
+ cbArg1 0x0 = Get Feature Information
+ cbRES1 Standard return codes (0, -1, -2)
+ cbRES2, word0 Bitmap of user-selectable modes
+ bit 0 Always off (All systems)
+ bit 1 Always on (Travis ATG, Siberia)
+ bit 2 Auto: ALS-based On; ALS-based Off (Travis ATG)
+ bit 3 Auto: ALS- and input-activity-based On; input-activity based Off
+ bit 4 Auto: Input-activity-based On; input-activity based Off
+ bit 5 Auto: Input-activity-based On (illumination level 25%);
input-activity based Off
+ bit 6 Auto: Input-activity-based On (illumination level 50%);
input-activity based Off
+ bit 7 Auto: Input-activity-based On (illumination level 75%);
input-activity based Off
+ bit 8 Auto: Input-activity-based On (illumination level 100%);
input-activity based Off
+ bits 9-15 Reserved for future use
+ cbRES2, byte2 Reserved for futur
