Re: [PATCH][v2] dell-wireless: new driver for dell wireless button for Windows 8
On Wed, Nov 12, 2014 at 02:51:44PM +0800, Alex Hung wrote: > This is for Dell's new ACPI device for radio switches with ACPI > ID "DELLABCE". This device is Dell's solution for Microsoft > Windows 8's new requirement for wireless hotkeys. > > When wireless hotkey is pressed, BIOS issues a Notify(RBTN, 0x80) > which is tanslated to a keycode KEY_RFKILL to userspace. > > Signed-off-by: Alex Hung Queued, thanks Alex. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight
ng, S_IRUGO | S_IWUSR,
> +kbd_led_als_show, kbd_led_als_store);
> +
> +static struct attribute *kbd_led_attrs[] = {
> + &dev_attr_stop_timeout.attr,
> + &dev_attr_start_triggers.attr,
> + &dev_attr_als_setting.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(kbd_led);
> +
> +static enum led_brightness kbd_led_level_get(struct led_classdev *led_cdev)
> +{
> + int ret;
> + u16 num;
> + struct kbd_state state;
> +
> + if (kbd_get_max_level()) {
> + ret = kbd_get_state(&state);
> + if (ret)
> + return 0;
> + ret = kbd_get_level(&state);
> + if (ret < 0)
> + return 0;
> + return ret;
> + } else if (kbd_get_valid_token_counts()) {
> + ret = kbd_get_first_active_token_bit();
> + if (ret < 0)
> + return 0;
> + for (num = kbd_token_bits; num != 0 && ret > 0; --ret)
> + num &= num - 1; /* clear the first bit set */
> + if (num == 0)
> + return 0;
> + return ffs(num) - 1;
> + } else {
> + pr_warn("Keyboard brightness level control not supported\n");
> + return 0;
> + }
You can drop the else blocks from the above as every case returns explicitly.
if (condA)
return retA;
if (condB)
return retB
return ret
(checkpatch.pl catches this)
> +}
> +
> +static void kbd_led_level_set(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + struct kbd_state state;
> + struct kbd_state new_state;
> + u16 num;
> +
> + if (kbd_get_max_level()) {
> + if (kbd_get_state(&state))
> + return;
> + new_state = state;
> + if (kbd_set_level(&new_state, value))
> + return;
> + kbd_set_state_safe(&new_state, &state);
> + } else if (kbd_get_valid_token_counts()) {
> + for (num = kbd_token_bits; num != 0 && value > 0; --value)
> + num &= num - 1; /* clear the first bit set */
> + if (num == 0)
> + return;
> + kbd_set_token_bit(ffs(num) - 1);
> + } else {
> + pr_warn("Keyboard brightness level control not supported\n");
> + }
> +}
> +
> +static struct led_classdev kbd_led = {
> + .name = "dell::kbd_backlight",
> + .brightness_set = kbd_led_level_set,
> + .brightness_get = kbd_led_level_get,
> + .groups = kbd_led_groups,
> +};
> +
> +static int __init kbd_led_init(struct device *dev)
> +{
> + kbd_init();
> + if (!kbd_led_present)
> + return -ENODEV;
> + kbd_led.max_brightness = kbd_get_max_level();
> + if (!kbd_led.max_brightness) {
> + kbd_led.max_brightness = kbd_get_valid_token_counts();
> + if (kbd_led.max_brightness)
> + kbd_led.max_brightness--;
> + }
> + return led_classdev_register(dev, &kbd_led);
> +}
> +
> +static void brightness_set_exit(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + /* Don't change backlight level on exit */
> +};
> +
> +static void kbd_led_exit(void)
> +{
> + if (!kbd_led_present)
> + return;
> + kbd_led.brightness_set = brightness_set_exit;
> + led_classdev_unregister(&kbd_led);
> +}
> +
> static int __init dell_init(void)
> {
> int max_intensity = 0;
> @@ -842,6 +1830,8 @@ static int __init dell_init(void)
> if (quirks && quirks->touchpad_led)
> touchpad_led_init(&platform_device->dev);
>
> + kbd_led_init(&platform_device->dev);
> +
> dell_laptop_dir = debugfs_create_dir("dell_laptop", NULL);
> if (dell_laptop_dir != NULL)
> debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
> @@ -909,6 +1899,7 @@ static void __exit dell_exit(void)
> debugfs_remove_recursive(dell_laptop_dir);
> if (quirks && quirks->touchpad_led)
> touchpad_led_exit();
> + kbd_led_exit();
> i8042_remove_filter(dell_laptop_i8042_filter);
> cancel_delayed_work_sync(&dell_rfkill_work);
> backlight_device_unregister(dell_backlight_device);
> @@ -925,5 +1916,7 @@ module_init(dell_init);
> module_exit(dell_exit);
>
> MODULE_AUTHOR("Matthew Garrett ");
> +MODULE_AUTHOR("Gabriele Mazzotta ");
> +MODULE_AUTHOR("Pali Rohár ");
> MODULE_DESCRIPTION("Dell laptop driver");
> MODULE_LICENSE("GPL");
> --
> 1.7.9.5
>
>
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] asus-nb-wmi: Add another wapf=4 quirk
On Fri, Nov 21, 2014 at 01:18:06PM +0100, Hans de Goede wrote: > Reported through launchpad: > https://bugs.launchpad.net/bugs/1173681 > Hi Hans, I'm not going to merge commits without complete git messages, we've already gone over this. Pointing at a bug with no additional detail is not acceptable for a Linux kernel commit log. There is *always something* relevant you can put in a commit message. Thanks, -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sony-laptop: add support for Sony Vaio Fit multi-flip laptop/presentation/tablet transformation
on events */
> + sony_call_snc_handle(handle, 0, &result);
> +
> acpi_bus_generate_netlink_event(sony_nc_acpi_device->pnp.device_class,
> + dev_name(&sony_nc_acpi_device->dev),
> TABLET_MODE_SWITCH, sony_nc_tablet_mode_update());
Line length.
> + break;
> default:
> continue;
> }
> @@ -3145,6 +3178,97 @@ static void sony_nc_backlight_cleanup(vo
> backlight_device_unregister(sony_bl_props.dev);
> }
>
> +/* laptop/presentation/tablet mode for Sony Vaio Fit 11a/13a/14a/15a */
> +struct snc_tablet_control {
> + struct device_attribute attr;
> + int handle;
> + int mode;
> +};
> +static struct snc_tablet_control *tablet_ctl;
> +
> +static ssize_t sony_nc_tablet_mode_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buffer)
> +{
> + if(!tablet_ctl)
> + return -EIO;
> +
> + return snprintf(buffer, PAGE_SIZE, "%d\n", tablet_ctl->mode);
> +}
> +
> +static int sony_nc_tablet_mode_update(void) {
> + struct input_dev *key_dev = sony_laptop_input.key_dev;
> +
> + if (!key_dev)
> + return -1;
> +
> + if (!tablet_ctl)
> + return -1;
The above can be condensed:
if (!key_dev || !tablet_ctrl)
return -1;
> + if (sony_call_snc_handle(tablet_ctl->handle, 0x0200, &tablet_ctl->mode))
> + return -1;
Or even:
int ret = -1;
if (key_dev && tablet_ctrl)
ret = sony_call_snc...
if (ret)
return ret;
> + input_report_switch(key_dev, SW_TABLET_MODE, tablet_ctl->mode ==
> tablet_mode);
Line length.
> + input_sync(key_dev);
> +
> + return tablet_ctl->mode;
> +}
> +
> +static int sony_nc_tablet_mode_setup(struct platform_device *pd,
> + unsigned int handle)
> +{
> + struct input_dev *key_dev = sony_laptop_input.key_dev;
> + int value, ret;
One variable per line please.
> +
> + if (tablet_ctl) {
> + pr_warn("handle 0x%.4x: laptop/presentation/tablet mode control
> setup already done for 0x%.4x\n",
> + handle, tablet_ctl->handle);
> + return -EBUSY;
> + }
> +
> + if (sony_call_snc_handle(handle, 0x, &value))
> + return -EIO;
> +
> + tablet_ctl = kzalloc(sizeof(struct snc_tablet_control), GFP_KERNEL);
> + if (!tablet_ctl)
> + return -ENOMEM;
> +
> + tablet_ctl->handle = handle;
> +
> + sysfs_attr_init(&tablet_ctl->attr.attr);
> + tablet_ctl->attr.attr.name = "tablet";
> + tablet_ctl->attr.attr.mode = S_IRUGO;
> + tablet_ctl->attr.show = sony_nc_tablet_mode_show;
> + tablet_ctl->attr.store = NULL;
Please use the DEVICE_ATTR macros for setting these up, see other
platform/driver/x86 drivers for examples.
> +
> + if (key_dev)
> + input_set_capability(key_dev, EV_SW, SW_TABLET_MODE);
> +
> + ret = device_create_file(&pd->dev, &tablet_ctl->attr);
> + if (ret)
> + goto tablet_error;
> + return 0;
> +
> +tablet_error:
> + device_remove_file(&pd->dev, &tablet_ctl->attr);
> + kfree(tablet_ctl);
> + tablet_ctl = NULL;
> + sony_call_snc_handle(handle, 0x0100, &value);
> + return ret;
> +}
> +
> +static void sony_nc_tablet_mode_cleanup(struct platform_device *pd)
> +{
> + int value;
> +
> + if(tablet_ctl) {
> + device_remove_file(&pd->dev, &tablet_ctl->attr);
> + sony_call_snc_handle(tablet_ctl->handle, 0x0100, &value);
> + kfree(tablet_ctl);
> + tablet_ctl = NULL;
> + }
> +}
> +
> static int sony_nc_add(struct acpi_device *device)
> {
> acpi_status status;
>
Thanks,
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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&
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 platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: New dell-wireless driver
On Sat, Nov 22, 2014 at 11:45:08PM +0100, Pali Rohár wrote: > Hello, > > I saw dell-wireless driver on platform-driver-x86 mailinglist [1] > which using DELLABCE acpi device and I do not like some parts in > this driver. Hi Pali, Thanks for reviewing and speaking up :) > > First is that this driver export rfkill event as keypress which > is also reported to userspace by keyboard controller. So then > userspace receive two rfkill keypresses. Alex, can you comment? Does the keyboard controller also see this event? > > Second is that DELLABCE acpi device can also control "soft" > rfkill status and this driver does not enable it because it use > input class instead rfkill. > > Anyway I have unfinished my version of DELLABCE acpi driver which > will use rfkill interface and plus allow to use hw switch events > in dell-laptop.ko driver. Is this something that could be applied incrementally fo Alex's driver, or is it something we'd be best starting over with? We have some precedent for input drivers (there is one nearly identical to the dell driver for hp, also by Alex). Using rfkill does seem like the better approach without digging into it. > > Currently dell-laptop.ko driver is using i8042 hook function for > detecting hw switch key press event. It is needed to detect if > rfkill state was changed or not. > > My prepared patches for dell-laptop.ko allows to use acpi event > from DELLABCE driver, so i8042 hook function can be dropped. > Really it is not good idea to pass every PS/2 data from both > keyboard, touchpad and trackpoint to dell-laptop driver and if > there is alternative (DELLABCE) it is better to use it. > > But now I would like to hear what do you think about it. > > Because only one kernel driver can attach to DELLABCE acpi > device, I cannot use new dell-wireless driver. And I think only > one driver can hit mainline kernel. I would like to see your patch, it sounds like it might be a better option. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: New dell-wireless driver
On Sun, Nov 23, 2014 at 01:52:46AM +0100, Pali Rohár wrote: > Ok, I will sent patches. There are some problems which I'm trying > to fix together with Gabriele. Do you need to see patches now, or > can you wait some time until we fix it? We can definitely wait some time. A week is perfectly fine, a couple weeks starts to be a stretch (relative to the upcoming merge window). Still, I'd rather get this right. So yes, we can wait. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] asus-nb-wmi: Add another wapf=4 quirk
On Mon, Nov 24, 2014 at 11:26:32AM +0100, Hans de Goede wrote: > Wifi on this laptop does not work unless asus-nb-wmi.wapf=4 is specified on > the kerne commandline, add a quirk for this. > > Cc: sta...@vger.kernel.org > BugLink: https://bugs.launchpad.net/bugs/1173681 > Signed-off-by: Hans de Goede Queued, thank you Hans. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] toshiba_acpi: Fix regression caused by backlight extra check code
On Mon, Nov 24, 2014 at 07:29:36PM -0700, Azael Avalos wrote: > Bug 86521 uncovered that some TOS6208 devices also return > non zero values on a write call to the backlight method, > thus getting caught and bailed out by the extra check code. > > This patch changes the set_lcd_brightness function to its > "original" state by just adapting it to the new function > format. > > Signed-off-by: Azael Avalos > --- > Changes since v1: > - Simply revert the commit to its original state by just > adapting the code to the new function format > > Darren, > > This patch simply reverts the commit so the bug can be closed, > I'll send a few patches later which will include the change > needed to block the backlight on TOS1900 devices for review. Queued. Thanks Azael, -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] Sony-laptop: Deletion of an unnecessary check before the function call "pci_dev_put"
On Mon, Nov 24, 2014 at 10:04:16PM +0100, SF Markus Elfring wrote: > From: Markus Elfring > Date: Mon, 24 Nov 2014 22:00:21 +0100 > > The pci_dev_put() function tests whether its argument is NULL > and then returns immediately. Thus the test around the call > is not needed. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring Thanks Markus, queued. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] platform: x86: Deletion of checks before backlight_device_unregister()
On Mon, Nov 24, 2014 at 08:40:22PM +0100, SF Markus Elfring wrote: > From: Markus Elfring > Date: Mon, 24 Nov 2014 20:30:29 +0100 > > The backlight_device_unregister() function tests whether its argument is NULL > and then returns immediately. Thus the test around the call is not needed. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring Thanks Markus, queued. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] platform: x86: dell-rbtn: Dell Airplane Mode Switch driver
On Fri, Nov 28, 2014 at 01:54:57PM +0200, Mika Westerberg wrote: > On Fri, Nov 28, 2014 at 12:45:55PM +0100, Pali Rohár wrote: > > Hello, > > > > I will fix all those style problems and add some comments. > > > > On Friday 28 November 2014 12:33:28 Mika Westerberg wrote: > > > > + if (ACPI_FAILURE(status)) > > > > + return; > > > > + > > > > + rfkill_set_states(rfkill, !output, !output); > > > > > > You can also write it like: > > > > > > if (ACPI_SUCCESS(status)) > > > rfkill_set_states(rfkill, !output, !output); > > > > > > which looks better to me at least. > > > > > > > In whole module I'm using this style: > > > > f1(); > > if (f1_failed) > > return; > > f2() > > if (f2_failed) > > return; > > > > So I would like not to change it for one function. > > Fair enough. And, in my opinion, it is better to test for errors than to test for success. This keeps the main logic out of a nested block. Not so critical here, but a good rule of thumb. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] platform: x86: dell-rbtn: Export notifier for other kernel modules
return acpi_bus_register_driver(&rbtn_driver);
> + /* ignore errors so module always loads and exports needed functions */
> + acpi_bus_register_driver(&rbtn_driver);
> + return 0;
> }
>
> static void __exit rbtn_exit(void)
> @@ -170,9 +246,14 @@ static void __exit rbtn_exit(void)
> acpi_bus_unregister_driver(&rbtn_driver);
> }
>
> +module_param(auto_remove_rfkill, bool, 0444);
> module_init(rbtn_init);
> module_exit(rbtn_exit);
>
> +MODULE_PARM_DESC(auto_remove_rfkill, "automatically remove rfkill devices
> when "
> + "other module start receiving events from "
another module starts
> + "this module and re-add them when last "
when the last
> + "module stop receving events");
stops
> MODULE_DEVICE_TABLE(acpi, rbtn_ids);
> MODULE_DESCRIPTION("Dell Airplane Mode Switch driver");
> MODULE_AUTHOR("Pali Rohár ");
> diff --git a/drivers/platform/x86/dell-rbtn.h
> b/drivers/platform/x86/dell-rbtn.h
> new file mode 100644
> index 000..41ffbc8
> --- /dev/null
> +++ b/drivers/platform/x86/dell-rbtn.h
> @@ -0,0 +1,35 @@
> +/*
> +Dell Airplane Mode Switch driver
> +Copyright (C) 2014 Pali Rohár
> +
> +This program is free software; you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 2 of the License, or
> +(at your option) any later version.
> +
> +This program is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +GNU General Public License for more details.
> +*/
See Mika's comments on block quote formatting.
> +
> +#ifndef _DELL_RBTN_H_
> +#define _DELL_RBTN_H_
> +
> +struct notifier_block;
> +
> +#if defined(CONFIG_DELL_RBTN) || defined(CONFIG_DELL_RBTN_MODULE)
> +int dell_rbtn_notifier_register(struct notifier_block *nb);
> +int dell_rbtn_notifier_unregister(struct notifier_block *nb);
> +#else
> +static inline int dell_rbtn_notifier_register(struct notifier_block *nb)
> +{
> + return -ENODEV;
> +}
> +static inline int dell_rbtn_notifier_unregister(struct notifier_block *nb)
> +{
> + return -ENODEV;
> +}
> +#endif
> +
> +#endif
> --
> 1.7.9.5
>
>
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] toshiba-acpi: Add missing ID (TOS6207)
On Mon, Nov 10, 2014 at 12:11:54AM +0100, Ondrej Zary wrote:
> toshiba-acpi was always missing TOS6207 ID so it did not load automatically
> on some laptops (such as Portege R100). But it worked fine if loaded manually.
> Commit 135740de7764 ("toshiba_acpi: Convert to use acpi_driver") broke that
> and the driver does not work even when loaded manually since then.
>
> Add TOS6207 ID to fix it.
>
> Tested on Toshiba Portege R100.
>
> Signed-off-by: Ondrej Zary
Queued, thanks!
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] platform: x86: hp_wireless: Inform the user if hp_wireless_input_setup()/add() fails
On Sat, Nov 29, 2014 at 01:04:17AM +0100, Borislav Petkov wrote:
> On Sat, Nov 29, 2014 at 01:48:31AM +0200, Giedrius Statkevicius wrote:
> > On 2014.11.29 01:15, Borislav Petkov wrote:
> > > On Sat, Nov 29, 2014 at 12:14:27AM +0200, Giedrius Statkevicius wrote:
> > >> In hpwl_add() there is a unused variable err to which we assign the
> > >> result of hp_wireless_input_setup() but we don't do anything depending
> > >> on the result so print out a message that informs the user if add()
> > >> (hp_wireless_input_setup()) fails since acpi_device_probe() doesn't
> > >> print anything in this case.
> > >>
> > >> Signed-off-by: Giedrius Statkevicius
> > >> ---
> > >> drivers/platform/x86/hp-wireless.c | 3 +++
> > >> 1 file changed, 3 insertions(+)
> > >>
> > >> diff --git a/drivers/platform/x86/hp-wireless.c
> > >> b/drivers/platform/x86/hp-wireless.c
> > >> index 415348f..4e4cc8b 100644
> > >> --- a/drivers/platform/x86/hp-wireless.c
> > >> +++ b/drivers/platform/x86/hp-wireless.c
> > >> @@ -85,6 +85,9 @@ static int hpwl_add(struct acpi_device *device)
> > >> int err;
> > >>
> > >> err = hp_wireless_input_setup();
> > >> +if (err)
> > >> +pr_err("Failed to setup hp wireless hotkeys\n");
> > >> +
> > >
> > > I don't think there's need for that. Just do:
> > >
> > > return hp_wireless_input_setup();
> > >
> > > and drop err completely.
> > >
> > > The upper layer which calls the ->add() method should propagate the
> > > error properly.
This is the key. If the caller already prints a message, we don't need to. If
nothing is displayed and the user would be left confused, then an appropriate
message should be displayed.
If the upper level already handles this, then we can just drop the unused
variable.
Have a look and decide which is the most appropriate. And thanks for preparing a
fix.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight
On Sun, Nov 23, 2014 at 03:50:45PM +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 Queued in Testing branch, but still pending a more thorough final review from me as it is such a large patch. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Dell Airplane Mode Switch driver
On Sun, Nov 23, 2014 at 04:09:18PM +0100, Pali Rohár wrote: > This patch series add new acpi Dell Airplane Mode Switch driver (DELLABCE and > DELRBTN acpi devices). It provides radio HW switch events (together with > current > state of radio devices) and export them via rfkill interface. These events are > also used in dell-laptop driver instead i8042 filter hook function (when acpi > device is available). > > Pali Rohár (3): > platform: x86: dell-rbtn: Dell Airplane Mode Switch driver > platform: x86: dell-rbtn: Export notifier for other kernel modules > platform: x86: dell-laptop: Use dell-rbtn instead i8042 filter when > possible > > drivers/platform/x86/Kconfig | 14 ++ > drivers/platform/x86/Makefile |1 + > drivers/platform/x86/dell-laptop.c | 67 +- > drivers/platform/x86/dell-rbtn.c | 260 > > drivers/platform/x86/dell-rbtn.h | 35 + > 5 files changed, 372 insertions(+), 5 deletions(-) > create mode 100644 drivers/platform/x86/dell-rbtn.c > create mode 100644 drivers/platform/x86/dell-rbtn.h Alex, it is my understanding that this solution from Pali is a more complete solution to dealing with the variety of dell wireless buttons and rfkill mechanisms in the world today. I currently have: 7c4d961 dell-wireless: new driver for dell wireless button for Windows 8 queued in for-next. If I have read your responses on this correctly, are we all in agreement that I should drop the above patch, and apply these? -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] platform: x86: hp_wireless: Inform the user if hp_wireless_input_setup()/add() fails
On Tue, Dec 02, 2014 at 06:15:32PM +0200, Giedrius Statkevicius wrote:
> On 2014.11.26 00:57, Darren Hart wrote:
> > On Sat, Nov 29, 2014 at 01:04:17AM +0100, Borislav Petkov wrote:
> >> On Sat, Nov 29, 2014 at 01:48:31AM +0200, Giedrius Statkevicius wrote:
> >>> On 2014.11.29 01:15, Borislav Petkov wrote:
> >>>> On Sat, Nov 29, 2014 at 12:14:27AM +0200, Giedrius Statkevicius wrote:
> >>>>> In hpwl_add() there is a unused variable err to which we assign the
> >>>>> result of hp_wireless_input_setup() but we don't do anything depending
> >>>>> on the result so print out a message that informs the user if add()
> >>>>> (hp_wireless_input_setup()) fails since acpi_device_probe() doesn't
> >>>>> print anything in this case.
> >>>>>
> >>>>> Signed-off-by: Giedrius Statkevicius
> >>>>> ---
> >>>>> drivers/platform/x86/hp-wireless.c | 3 +++
> >>>>> 1 file changed, 3 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/platform/x86/hp-wireless.c
> >>>>> b/drivers/platform/x86/hp-wireless.c
> >>>>> index 415348f..4e4cc8b 100644
> >>>>> --- a/drivers/platform/x86/hp-wireless.c
> >>>>> +++ b/drivers/platform/x86/hp-wireless.c
> >>>>> @@ -85,6 +85,9 @@ static int hpwl_add(struct acpi_device *device)
> >>>>> int err;
> >>>>>
> >>>>> err = hp_wireless_input_setup();
> >>>>> + if (err)
> >>>>> + pr_err("Failed to setup hp wireless hotkeys\n");
> >>>>> +
> >>>>
> >>>> I don't think there's need for that. Just do:
> >>>>
> >>>> return hp_wireless_input_setup();
> >>>>
> >>>> and drop err completely.
> >>>>
> >>>> The upper layer which calls the ->add() method should propagate the
> >>>> error properly.
> >
> > This is the key. If the caller already prints a message, we don't need to.
> > If
> > nothing is displayed and the user would be left confused, then an
> > appropriate
> > message should be displayed.
> >
> > If the upper level already handles this, then we can just drop the unused
> > variable.
> >
> > Have a look and decide which is the most appropriate. And thanks for
> > preparing a
> > fix.
> >
> To start with I've looked at all modules in drivers/platform/x86 that
> have a struct acpi_driver and calculated some statistics about
> whether the module informs the user if add() fails or not to find out
> the current policy on whether we should inform the user or not:
>
> Module Does it use any pr_/dev_ function to give info?
> asus-laptopYes
> classmate-laptop No
> dell-smo8800 Yes
> dell-wireless No
> eeepc-laptop Yes
> fujitsu-laptop Yes
> hp-wirelessNo
> hp_accel Yes
> intel-rst No
> intel-smartconnect Yes
> intel_menlow No
> panasonic-laptop No (but it uses ACPI_DEBUG_PRINT)
> pvpanicNo
> sony-laptopYes (has a message for failing to setup input devices)
> topstar-laptop No
> toshiba_acpi Yes
> toshiba_haps Yes
> toshiba_bluetooth Yes
> wmiYes
> xo15-ebook Yes
>
> That being said it looks like most add()s inform the user. So maybe we
> should make a patch that does it for other modules with struct
> acpi_driver? That being said, I've also took a look at if any messages
> are printed out about add() failing.
>
> There is acpi_device_probe() that's hooked into acpi_bus_type which
> defines names and some actions of a bus. There it simply returns any
> non-zero value:
>
> 990 ret = acpi_drv->ops.add(acpi_dev);
> 991 if (ret)
> 992 return ret;
>
> Delving a bit deeper I've found about driver_probe_device() and
> really_probe(). And in these lines I think is what the user would get if
> the probe failed:
>
> 330 } else if (ret != -ENODEV && ret != -ENXIO) {
> 331 /* driver matched but the probe failed */
> 332 printk(KERN_WARNING
> 333"%s: probe of %s failed with error %d\n",
> 334drv->name, dev_name(dev), ret);
>
> So the user would get only the numerical value of a error and thus a
> pr_err or printing any other message in the add() is useful to make more
> sense of what really happened and allow to quickly find where the issue
> happened. In the end I think that this patch is better than the last one
> (the one where add() simply returns the result of another function).
>
> Please correct me if I'm wrong.
I'd agree. I'll queue it up, thanks for the detailed investigation Giedrius.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] dell-laptop: fix kbd_timeout handling
On Wed, Dec 03, 2014 at 01:01:59PM +0300, Dan Carpenter wrote:
> The original code had a static checker warning:
>
> drivers/platform/x86/dell-laptop.c:1389 kbd_led_timeout_store()
> warn: this array is probably non-NULL. 'quirks->kbd_timeouts'
>
> This warning does indicate a bug. I have added a .needs_kbd_timeouts
> flag which is true if the .kbd_timeouts[] array has been declared.
> Otherwise, in the original code the quirk_dell_vostro_v130 struct didn't
> have .kbd_timeouts[] declared so the loop:
>
> for (i = 0; quirks->kbd_timeouts[i] != -1; i++) {
>
> Would have read beyond the end of the struct with unpredictable results.
>
> Signed-off-by: Dan Carpenter
Thanks Dan,
Pali, any objections?
Dan, any objection to Pali rolling this into another revision? No point in
introducing the bug to next and mainline.
Thanks,
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight
+
> +static u8 kbd_mode_levels[16];
> +static int kbd_mode_levels_count;
I'm confused by these. How are they different from kbd_info.levels?
We seem to check the latter first and fall back to these if that is 0 why?
> +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) {
> + ret = dell_smi_error(ret);
> + 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:
Please indent gotos by one space. This improves diffs context by not treating
the goto label
like a function.
> + release_buffer();
> + return ret;
> +}
> +
> +static unsigned int kbd_get_max_level(void)
> +{
> + if (kbd_info.levels != 0)
> + return kbd_info.levels;
> + if (kbd_mode_levels_count > 0)
> + return kbd_mode_levels_count - 1;
Here for example. In what scenario does this happen?
> + return 0;
> +}
> +
> +static inline int kbd_init_info(void)
> +{
> + struct kbd_state state;
> + int ret;
> + int i;
> +
> + ret = kbd_get_info(&kbd_info);
> + if (ret)
> + return ret;
> +
> + kbd_get_state(&state);
> +
> + /* NOTE: timeout value is stored in 6 bits so max value is 63 */
> + if (kbd_info.seconds > 63)
> + kbd_info.seconds = 63;
> + if (kbd_info.minutes > 63)
> + kbd_info.minutes = 63;
> + if (kbd_info.hours > 63)
> + kbd_info.hours = 63;
> + if (kbd_info.days > 63)
> + kbd_info.days = 63;
> +
> + /* NOTE: On tested machines ON mode did not work and caused
> + * problems (turned backlight off) so do not use it
> + */
> + kbd_info.modes &= ~BIT(KBD_MODE_BIT_ON);
> +
> + kbd_previous_level = kbd_get_level(&state);
> + kbd_previous_mode_bit = state.mode_bit;
> +
> + if (kbd_previous_level == 0 && kbd_get_max_level() != 0)
> + kbd_previous_level = 1;
> +
> + if (kbd_previous_mode_bit == KBD_MODE_BIT_OFF) {
> + kbd_previous_mode_bit =
> + ffs(kbd_info.modes & ~BIT(KBD_MODE_BIT_OFF));
> + if (kbd_previous_mode_bit != 0)
> + kbd_previous_mode_bit--;
> + }
> +
> + if (kbd_info.modes & (BIT(KBD_MODE_BIT_ALS) |
> + BIT(KBD_MODE_BIT_TRIGGER_ALS)))
> + kbd_als_supported = true;
> +
> + if (kbd_info.modes & (
> + BIT(KBD_MODE_BIT_TRIGGER_ALS) | BIT(KBD_MODE_BIT_TRIGGER) |
> + BIT(KBD_MODE_BIT_TRIGGER_25) | BIT(KBD_MODE_BIT_TRIGGER_50) |
> + BIT(KBD_MODE_BIT_TRIGGER_75) | BIT(KBD_MODE_BIT_TRIGGER_100)
> +))
> + kbd_triggers_supported = true;
> +
> + for (i = 0; i < 16; ++i)
Doesn't this only apply to bits 5-8? Why not just loop through those?
for (i = KBD_MODE_BIT_TRIGGER_25; i <= KBD_MODE_BIT_TRIGGER_100)
The level_mode_bit check becomes unecessary.
> + if (kbd_is_level_mode_bit(i) && (BIT(i) & kbd_info.modes))
> + kbd_mode_levels[1+kbd_mode_levels_count++] = i;
One space around operators like + please.
What is kbd_mode_levels[0] reserved for? The current level? Isn't that what
kbd_state.level is for?
> +
> + if (kbd_mode_levels_count > 0) {
> + for (i = 0; i < 16; ++i) {
If 0-4 don't apply here, why loop through them? Should we just set levels[0] to
0 if it isn't one of 5-8?
If bits 9-16 are reserved, it seems it would be best to avoid checking for them
as they might return a false positive, and we'd be setting kbd_mode_levels to an
undefined value.
> + if (BIT(i) & kbd_info.modes) {
> + kbd_mode_levels[0] = i;
> + break;
> + }
> + }
> + kbd_mode_levels_count++;
> + }
> +
> + return 0;
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: New dell-wireless driver
On Tue, Dec 02, 2014 at 04:08:58PM +0100, Gabriele Mazzotta wrote: > On Tuesday 02 December 2014 12:06:45 Gabriele Mazzotta wrote: > > On Thursday 27 November 2014 12:41:19 Alex Hung wrote: > > > On Sun, Nov 23, 2014 at 8:52 AM, Pali Rohár wrote: > > > > On Saturday 22 November 2014 03:09:06 Darren Hart wrote: > > > >> On Sat, Nov 22, 2014 at 11:45:08PM +0100, Pali Rohár wrote: > > > >> > Hello, > > > >> > > > > >> > I saw dell-wireless driver on platform-driver-x86 > > > >> > mailinglist [1] which using DELLABCE acpi device and I do > > > >> > not like some parts in this driver. > > > >> > > > >> Hi Pali, > > > >> > > > >> Thanks for reviewing and speaking up :) > > > >> > > > >> > First is that this driver export rfkill event as keypress > > > >> > which is also reported to userspace by keyboard controller. > > > >> > So then userspace receive two rfkill keypresses. > > > >> > > > >> Alex, can you comment? Does the keyboard controller also see > > > >> this event? > > > > > > > > Yes on my laptop E6440. But it looks like it does not have to be > > > > truth for all laptops... > > > > > > > >> > Second is that DELLABCE acpi device can also control "soft" > > > >> > rfkill status and this driver does not enable it because it > > > >> > use input class instead rfkill. > > > >> > > > > >> > Anyway I have unfinished my version of DELLABCE acpi driver > > > >> > which will use rfkill interface and plus allow to use hw > > > >> > switch events in dell-laptop.ko driver. > > > >> > > > >> Is this something that could be applied incrementally fo > > > >> Alex's driver, or is it something we'd be best starting over > > > >> with? > > > > > > > > Alex's driver is different. It registers input device. My > > > > approach register rfkill device plus add exported functions for > > > > registering atomic notifier (so other drivers like dell-laptop > > > > can use events too). > > > > > > > > First we need to know if input driver is really needed. And if > > > > yes determinate in which conditions and for which laptops. Really > > > > duplicate key press are not good. > > > > > > > > In case when input driver is really needed I can just copy > > > > relevant input code and add it into my driver (in case when my > > > > driver will be used instead Alex's). This could be no problem, > > > > because my and Alex code doing different things and so could > > > > coexist in one driver (but cannot be split into more because only > > > > one acpi driver can handle one acpi device). > > > > > > > >> We have some precedent for input drivers (there is one nearly > > > >> identical to the dell driver for hp, also by Alex). Using > > > >> rfkill does seem like the better approach without digging > > > >> into it. > > > > > > > > It is different from HP. Dell ACPI device on some machines can > > > > also control wifi switches (it can enable/disable it!). > > > > > > > > So it make sense to use rfkill. But on some machines that ACPI > > > > device can only receive events that HW switch was switched, but > > > > it is possible to ask for state (if is enabled or not). HP driver > > > > just report switch was changed, but does not report if is enabled > > > > or disabled. > > > > > > > > So I think HP is not identical to this Dell one. > > > > > > I can provide a little more information on HP and Dell's design. > > > > > > Dell's design is more complex than HP's indeed. > > > > > > HP BIOS will send ACPI notification when hotkey is pressed; > > > especially HP uses buttons instead of hardware slider on their > > > systems. > > > > > > Dell has two design > > > 1. Button similar to HP. My patch targeted this type. > > > 2. Hardware slider. This handled will handled by Wireless device > > > drivers (ex. WLAN, BT and so on) by their rfkill hard-block. There > > > is > > > no need to handle this case. > > > > &
Re: [RESEND PATCH v5 3/5] acerhdf: Use bang-bang thermal governor
On Fri, Nov 28, 2014 at 03:20:50PM +0100, Peter Feuerer wrote:
> acerhdf has been doing an on-off fan control using hysteresis by
> post-manipulating the outcome of thermal subsystem trip point handling.
> This patch enables acerhdf to use the bang-bang governor, which is
> intended for on-off controlled fans.
>
> Cc: platform-driver-x86@vger.kernel.org
> Cc: Darren Hart
> Cc: Andrew Morton
> CC: Zhang Rui
> Cc: Andreas Mohr
> Cc: Javi Merino
> Acked-and-tested-by: Borislav Petkov
> Signed-off-by: Peter Feuerer
> ---
> drivers/platform/x86/Kconfig | 3 ++-
> drivers/platform/x86/acerhdf.c | 36 +++-
> 2 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index a2eabe6..c173266 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -38,7 +38,8 @@ config ACER_WMI
>
> config ACERHDF
> tristate "Acer Aspire One temperature and fan driver"
> - depends on THERMAL && ACPI
> + select THERMAL_GOV_BANG_BANG
So we use select sparingly as it does implicit things.
I checked the THERMAL_GOV_BANG_BANG Kconfig entry, and the help says acerhdf
already depends on it (which it doesn't appear to). Any particular reason to add
select here instead of adding it as a depends.
Why did you drop THERMAL?
> + depends on ACPI
> ---help---
> This is a driver for Acer Aspire One netbooks. It allows to access
> the temperature sensor and to control the fan.
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index f30767f..7fe7dbf 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -50,7 +50,7 @@
> */
> #undef START_IN_KERNEL_MODE
>
> -#define DRV_VER "0.5.30"
> +#define DRV_VER "0.7.0"
>
> /*
> * According to the Atom N270 datasheet,
> @@ -258,6 +258,14 @@ static const struct bios_settings_t bios_tbl[] = {
>
> static const struct bios_settings_t *bios_cfg __read_mostly;
>
> +/*
> + * this struct is used to instruct thermal layer to use bang_bang instead of
> + * default governor for acerhdf
Please use sentence punctuation, particularly for block comments.
/*
* This struct...
* default ... for acerhdf.
*/
> + */
> +static struct thermal_zone_params acerhdf_zone_params = {
> + .governor_name = "bang_bang",
> +};
> +
> static int acerhdf_get_temp(int *temp)
> {
> u8 read_temp;
> @@ -439,6 +447,17 @@ static int acerhdf_get_trip_type(struct
> thermal_zone_device *thermal, int trip,
> return 0;
> }
>
> +static int acerhdf_get_trip_hyst(struct thermal_zone_device *thermal, int
> trip,
> + unsigned long *temp)
> +{
> + if (trip != 0)
> + return -EINVAL;
> +
> + *temp = fanon - fanoff;
> +
> + return 0;
> +}
> +
> static int acerhdf_get_trip_temp(struct thermal_zone_device *thermal, int
> trip,
>unsigned long *temp)
> {
> @@ -463,6 +482,7 @@ static struct thermal_zone_device_ops acerhdf_dev_ops = {
> .get_mode = acerhdf_get_mode,
> .set_mode = acerhdf_set_mode,
> .get_trip_type = acerhdf_get_trip_type,
> + .get_trip_hyst = acerhdf_get_trip_hyst,
> .get_trip_temp = acerhdf_get_trip_temp,
> .get_crit_temp = acerhdf_get_crit_temp,
> };
> @@ -515,9 +535,7 @@ static int acerhdf_set_cur_state(struct
> thermal_cooling_device *cdev,
> }
>
> if (state == 0) {
> - /* turn fan off only if below fanoff temperature */
> - if ((cur_state == ACERHDF_FAN_AUTO) &&
> - (cur_temp < fanoff))
> + if (cur_state == ACERHDF_FAN_AUTO)
> acerhdf_change_fanstate(ACERHDF_FAN_OFF);
> } else {
> if (cur_state == ACERHDF_FAN_OFF)
> @@ -696,11 +714,19 @@ static int acerhdf_register_thermal(void)
> return -EINVAL;
>
> thz_dev = thermal_zone_device_register("acerhdf", 1, 0, NULL,
> - &acerhdf_dev_ops, NULL, 0,
> + &acerhdf_dev_ops,
> + &acerhdf_zone_params, 0,
> (kernelmode) ? interval*1000 : 0);
> if (IS_ERR(thz_dev))
> return -EINVAL;
>
> + if (strcmp(thz_dev->governor->name,
> + acerhdf_zone_params.governor_name)) {
> + pr_err("Didn't get thermal govern
Re: [RESEND PATCH v5 4/5] acerhdf: added critical trip point
On Fri, Nov 28, 2014 at 03:20:51PM +0100, Peter Feuerer wrote:
> added critical trip point which represents the temperature limit.
Nitpic, Add ^
> Added return -EINVAL in case wrong trip point is provided.
Add (we are going to add it with this patch, it wasn't added previously). It's a
nitpic. But this is more consistent with typical commit message language.
I might normally just fix that upon commit, but since I had a few questions on
the last one, if that results in a V2, please correct this as well.
>
> Cc: platform-driver-x86@vger.kernel.org
> Cc: Darren Hart
> Cc: Andrew Morton
> Cc: Andreas Mohr
> Cc: Borislav Petkov
> Cc: Javi Merino
> Signed-off-by: Peter Feuerer
> ---
> drivers/platform/x86/acerhdf.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index 7fe7dbf..91b16c8 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -443,6 +443,10 @@ static int acerhdf_get_trip_type(struct
> thermal_zone_device *thermal, int trip,
> {
> if (trip == 0)
> *type = THERMAL_TRIP_ACTIVE;
> + else if (trip == 1)
> + *type = THERMAL_TRIP_CRITICAL;
> + else
> + return -EINVAL;
>
> return 0;
> }
> @@ -463,6 +467,10 @@ static int acerhdf_get_trip_temp(struct
> thermal_zone_device *thermal, int trip,
> {
> if (trip == 0)
> *temp = fanon;
> + else if (trip == 1)
> + *temp = ACERHDF_TEMP_CRIT;
> + else
> + return -EINVAL;
>
> return 0;
> }
> @@ -713,7 +721,7 @@ static int acerhdf_register_thermal(void)
> if (IS_ERR(cl_dev))
> return -EINVAL;
>
> - thz_dev = thermal_zone_device_register("acerhdf", 1, 0, NULL,
> + thz_dev = thermal_zone_device_register("acerhdf", 2, 0, NULL,
> &acerhdf_dev_ops,
> &acerhdf_zone_params, 0,
> (kernelmode) ? interval*1000 : 0);
> --
> 2.1.3
>
>
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] toshiba_acpi: Change notify funtion to handle more events
On Tue, Dec 02, 2014 at 10:36:32PM -0700, Azael Avalos wrote: > Currently the function toshiba_acpi_notify only > takes care of hotkeys, however, the TOS > devices receive more events that can be useful. > > This patch changes the function to be able to > handle more events, and in the process, move all > hotkey related code residing in it to a new > function called toshiba_acpi_process_hotkeys, > and also update the sysfs group whenever we > receive a 0x92 event, which indicates a change > in the keyboard backlight mode. This would be better as two patches. One for process_hotkeys, and another for the (smaller) kbd backlight sysfs update. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] toshiba_acpi: Move hotkey enabling code to own function
On Tue, Dec 02, 2014 at 10:36:31PM -0700, Azael Avalos wrote: > The hotkey enabling code is being used by > *_setup_keyboard and also by *_resume. > > This patch creates a new function called > toshiba_acpi_enable_hotkeys to be used by > these two functions to avoid duplicating > code. 42 is a little bit narrow, even for us kernel types :-) 72 characters wide is fine. Patch is fine, but since I had some questions on your others, please correct in V2. Thanks, -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight
On Thu, Dec 04, 2014 at 09:50:02AM +0100, Gabriele Mazzotta wrote:
> On Wednesday 03 December 2014 00:43:21 Darren Hart wrote:
> > > + 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 },
> >
> > Where did these values come from? Were they documented in the
> > libsmbios project? Can you provide a URL to that? These really should
> > be described by the firmware, but if they aren't, nothing we can do
> > about it.
>
> I took those values from a Windows utility provided by Dell. I tried
> to find a reason for that specific list to exist, but I couldn't. The
> reason why it's there is that the BIOS of my laptop accepts any timeout,
> but it silently sets the timeout to 0 (i.e. illumination never off)
> if a value not in that list is given. So, given the wide range of
> of possible input values, we added that quirk. This is something my
> laptop does, Pali's behaves differently and such a list is not needed.
Let's get a comment above the quirk describing the scenario.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight
On Thu, Dec 04, 2014 at 11:16:47AM +0100, Pali Rohár wrote:
> > > seconds not supported). + cbRES4, byte1 Maximum
> > > acceptable minutes value (0 if minutes not supported). +
> > > cbRES4, byte2 Maximum acceptable hours value (0 if hours
> > > not supported). + cbRES4, byte3 Maxiomum acceptable days
> > > value (0 if days not supported)
> >
> > Maximum ^
> >
> > This interface appears to define all possible values for the
> > timeout between cbRES3[1] and cbRES4[*]. Why is the
> > kbd_timeouts[] array a quirk with fixed values? It seems it
> > could indeed be dynamically determined.
> >
>
> BIOS bug (or BIOS feature?). No idea. For invalid value on XPS
> BIOS just reset keyboard backlight to someting default...
OK good. Let's just get the quirk a comment so it's clear why it's necessary.
...
>
> > > +
> > > +struct kbd_info {
> > > + u16 modes;
> > > + u8 type;
> > > + u8 triggers;
> > > + u8 levels;
> > > + u8 seconds;
> > > + u8 minutes;
> > > + u8 hours;
> > > + u8 days;
> > > +};
> > > +
> > > +
> > > +static u8 kbd_mode_levels[16];
> > > +static int kbd_mode_levels_count;
> >
> > I'm confused by these. How are they different from
> > kbd_info.levels?
> >
>
> There are two interfaces how to set keyboard backlight. Both are
> documented above. One via kbd mode and one via kbd level. Some
> laptops (e.g my E6440) support only kbd mode and some (e.g XPS)
> support only kbd level. So we need to implement both interfaces
> in kernel if we want to support as more machines as possible.
At the very least, let's get a comment block describing these and the two
interfaces. It wasn't clear to me from the big block of register description
that there were two separate interfaces.
...
> > > + for (i = 0; i < 16; ++i)
> >
> > Doesn't this only apply to bits 5-8? Why not just loop through
> > those?
> >
>
> Some bits are reserved for future use (now undocumented). So once
> we know what they means we can adjust global enum/macro and
> changing this for loop will not be needed.
>
> > for (i = KBD_MODE_BIT_TRIGGER_25; i <=
> > KBD_MODE_BIT_TRIGGER_100)
> >
> > The level_mode_bit check becomes unecessary.
> >
>
> I tried to write general code which check all possible modes if
> they are of type which configure level. And because some of them
> are undocumented I used this approach, so in future global
> enum/macro could be extended.
Fair enough. I won't bike shed this.
>
> > > + if (kbd_is_level_mode_bit(i) && (BIT(i) &
> > > kbd_info.modes))
> > > + kbd_mode_levels[1+kbd_mode_levels_count++] = i;
> >
> > One space around operators like + please.
> >
> > What is kbd_mode_levels[0] reserved for? The current level?
> > Isn't that what kbd_state.level is for?
> >
>
> Reserved for off mode -- keyboard backlight turned off.
>
> kbd level is for laptops which support kbd level. kbd mode is for
> laptops which support setting level via kbd mode.
>
> > > +
> > > + if (kbd_mode_levels_count > 0) {
> > > + for (i = 0; i < 16; ++i) {
> >
> > If 0-4 don't apply here, why loop through them? Should we just
> > set levels[0] to 0 if it isn't one of 5-8?
> >
>
> We know that BIOSes are buggy, so I can imagine that off mode
> (which is enum = 0) does not have to be supported... So for
> kbd_mode_levels[0] we set first supported mode (if off is not
> supported we will choose something other, so kernel code will not
> call unsupported mode).
This sort of defensive programming is good, but it does need a comment.
/*
* Find the first supported mode and assign to kbd_mode_levels[0].
* This should be 0 (off), but we cannot depend on the BIOS to
* support 0.
*/
>
> > If bits 9-16 are reserved, it seems it would be best to avoid
> > checking for them as they might return a false positive, and
> > we'd be setting kbd_mode_levels to an undefined value.
And I think you've addressed this comment above.
So in general, whenever you're working around bugs in firmware or hardware or
doing anything that might not be obvious to someone reviewing your code without
all the context you've built up, it's important to add a comment explaining why.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight
On Fri, Dec 05, 2014 at 03:03:49AM +0100, Pali Rohár wrote: > On Wednesday 03 December 2014 09:43:21 Darren Hart wrote: > > On Sun, Nov 23, 2014 at 03:50:45PM +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. > > > > Hi Pali, > > > > I would really like to see this broken up. Possibly adding > > levels and timeouts as separate patches for example. It is > > quite difficult to review this all at once in a reasonable > > amount of time. > > > > It is really hard to split this patch into more parts which every > one will compile and ideally also work. In my opinion every > single git commit should be possible to compile and should also > work (it helps also other developers to use git bisect). Of course, that's a given. > > And because we need to pass all (previous unchanged) values in > smbios call we need to parse all of them. > > I understand that it could be hard to review long patch, but > there are more parts which interacts and do not work without > other. Also some of settings (e.g keyboard backlight level) could > be changed via different ways (and on some machines only one is > working) and also that smbios keyboard interface has complicated > logic... I was thinking about how to break it up, and I don't have an obvious answer. I considered adding a levels interface first and then the mode_levels, but that's a lot of work for not much savings. I don't like adding over 1000 lines to a 900 line driver in a single go to add one feature, but I didn't see obvious ways to a) make it smaller or b) break it up. So, please incorporate the remaining requested changes, and I'll pull it in. Patches should really be in next for two weeks before going to Linus for a merge. As we're already at rc7, that makes 3.19 a stretch. Get it to me by tomorrow and I'll queue it up but it might not make 3.19. For future patches, please add some additional comments to code that is non-obvious and if there is any way to break the patches up. This will expedite the review process (as this subsystem gets fairly little external review, you end up blocked on me). > > > During this review I caught a few more CodingStyle violations, > > and raised some questions about the timeout and levels > > mechanisms. > > > > I will fix style problems in next v3 patch. > > What to do with Dan Carpenter patch which fixing kbd_timeout > handling? Should I integrate it into v3? It does not apply on top > of next v3 patch, because there will be new comment about quirk > plus style fixes... Please include Dan's fix and credit him with it in the commit message. Add him to the Cc list. Thanks Pali, -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Dell Airplane Mode Switch driver
On Thu, Dec 04, 2014 at 04:16:25PM +0800, Alex Hung wrote: > HI Darren and Pali, > > It was great that we had a lot of discussion but it seems Dell BIOS > implementation varies from one series to another. Both work looks good > either one is fine with me. > > But I think I can do a little more: I am collecting a number of > systems to try out these patches. This should help us determine which > one work better and probably we can integrate. > > Currently I have found four systems (including two Latitude, an > Inspiron and a XPS with working method(ARBT) that Gabriele suggested). > I can get other, ex. a Vostro, if needed. > > I will test dell-wireless.c with Gabriele's suggestion and Pali's > dell-rbtn.c (btw, will there be updates?). However, I will need a few > days to do the comparison. OK, this is great, thanks for working together on this. Pali - if I merge Alex's dell-wireless driver, does it BREAK your systems, or does it just not fully support them? If it BREAKS them, I'll drop Alex's patch from for-next and from the 3.19 pull requeust. If it just doesn't fully support them, I'll leave his patch in for-next and look forward to a set of patches from you both that completes support for the 3.20 merge window. Please let me know as soon as you can. If I don't hear back, I'll have to err on the side of caution and drop dell-wireless from for-next for this window. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Dell Airplane Mode Switch driver
On Thu, Dec 04, 2014 at 10:55:32AM +0100, Pali Rohár wrote: > > Darren, I think that if we do not solve problem with duplicate > key events (in dell-wireless.c) we should postpone these patches > to later kernel version. It is better to not have such regression > as it confuse software like NetworkManager which is widely used. OK, that's what I needed. Thanks. Ignore my previous request, you answered it here. I will drop dell-wireless.c and look for a combined solution from you and Alex for the next release. Thanks, -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] toshiba_acpi: Change notify funtion to handle more events
On Wed, Dec 03, 2014 at 11:42:04PM -0700, Azael Avalos wrote:
> @@ -1971,41 +2008,22 @@ error:
> static void toshiba_acpi_notify(struct acpi_device *acpi_dev, u32 event)
> {
> struct toshiba_acpi_dev *dev = acpi_driver_data(acpi_dev);
> - u32 hci_result, value;
> - int retries = 3;
> - int scancode;
> -
> - if (event != 0x80)
> - return;
> + int ret;
CC drivers/platform/x86/toshiba_acpi.o
drivers/platform/x86/toshiba_acpi.c: In function ‘toshiba_acpi_notify’:
drivers/platform/x86/toshiba_acpi.c:2012:6: warning: unused variable ‘ret’
[-Wunused-variable]
int ret;
^
Please compile check each patch.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation: Add entry for dell-laptop sysfs interface
On Wed, Dec 03, 2014 at 06:41:33PM +0100, Gabriele Mazzotta wrote: > Add the documentation for the new sysfs interface of dell-laptop > that allows to configure the keyboard illumination on Dell systems. > > Signed-off-by: Gabriele Mazzotta > Signed-off-by: Pali Rohár If nobody else would prefer to take this in their tree, I'll submit it through platform-drivers-x86 when I send the corresponding driver patch. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] toshiba_acpi: Change notify funtion to handle more events
On Thu, Dec 04, 2014 at 08:10:49PM -0700, Azael Avalos wrote:
> Hi Darren,
>
> 2014-12-03 6:12 GMT-07:00 Darren Hart :
> > On Wed, Dec 03, 2014 at 11:42:04PM -0700, Azael Avalos wrote:
> >> @@ -1971,41 +2008,22 @@ error:
> >> static void toshiba_acpi_notify(struct acpi_device *acpi_dev, u32 event)
> >> {
> >> struct toshiba_acpi_dev *dev = acpi_driver_data(acpi_dev);
> >> - u32 hci_result, value;
> >> - int retries = 3;
> >> - int scancode;
> >> -
> >> - if (event != 0x80)
> >> - return;
> >> + int ret;
> >
> > CC drivers/platform/x86/toshiba_acpi.o
> > drivers/platform/x86/toshiba_acpi.c: In function ‘toshiba_acpi_notify’:
> > drivers/platform/x86/toshiba_acpi.c:2012:6: warning: unused variable ‘ret’
> > [-Wunused-variable]
> > int ret;
> > ^
> >
> > Please compile check each patch.
>
> Sorry about that, was a left over from the split.
> Want me to send a V3? I can send them in few minutes.
Yes please.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses
On Thu, Dec 04, 2014 at 12:16:20AM +0100, Gabriele Mazzotta wrote: > Currently dell-wmi reports keypresses for WMI events that are > notifications of changes performed by the BIOS. > This patch series make sure that no keypresses are sent for those > events so that nothing is done from userspace. > > Gabriele Mazzotta (3): > dell-wmi: Use appropriate keycode for radio state changes > dell-wmi: Don't report keypresses for radio state changes Merged into one patch, queued. > dell-wmi: Don't report keypresses on keybord illumination change Queued. Thanks Gabriele. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/3] toshiba_acpi: Hotkeys and event handling changes
On Thu, Dec 04, 2014 at 08:22:44PM -0700, Azael Avalos wrote: > These patches change the current code to accomodate the handling > of more events, since so far, it only handles hotkey events (0x80), > move the hotkey enabling code to a sub function to avoid duplication, > and add event 0x92 which indicates a change in the keyboard backlight > mode. > > Changes since V3: > - Removed a left over variable from the split in patch 02 and > moved it to patch 03 (where it belongs) > - A bit more verbose description on patch 03 > > Azael Avalos (3): > toshiba_acpi: Move hotkey enabling code to own function > toshiba_acpi: Change notify funtion to handle more events > toshiba_acpi: Add keyboard backlight mode change event Queued. Your 1/3 commit message reverted to the short line length, might want to update your tools. ;-) I took care of it. Thanks Azael, -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dell-smo8800: Add more acpi ids and change description of driver
On Sun, Nov 23, 2014 at 03:38:26AM +0100, Pali Rohár wrote: > Oops, I lost this mail somewhere... > > I copied these ids from windows driver inf file. I did not tested > them because I do not have machine which reports these ids. It > looks like all SMOXX devices have same acpi structure (one IRQ). > So adding ids could not break anything (if acpi device will not > have for some reason IRQ specified, device probe function will > fail). Please resend with a complete message body and the typo fix below. > > On Monday 29 September 2014 22:56:04 Darren Hart wrote: > > On Mon, Sep 29, 2014 at 02:45:48PM +0200, Pali Rohár wrote: > > > > Please see Documentation/SubmittingPatches, 15) The canonical > > patch format, and provide a complete message body. > > > > Have you validated each of the 6 ACPI IDs added to the driver? > > > > > Signed-off-by: Pali Rohár > > > --- > > > > > > drivers/platform/x86/Kconfig|4 ++-- > > > drivers/platform/x86/dell-smo8800.c | 10 -- > > > 2 files changed, 10 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/platform/x86/Kconfig > > > b/drivers/platform/x86/Kconfig index 3bbcbf1..ba9ee4f > > > 100644 > > > --- a/drivers/platform/x86/Kconfig > > > +++ b/drivers/platform/x86/Kconfig > > > @@ -128,10 +128,10 @@ config DELL_WMI_AIO > > > > > > be called dell-wmi-aio. > > > > > > config DELL_SMO8800 > > > > > > - tristate "Dell Latitude freefall driver (ACPI > > > SMO8800/SMO8810)" + tristate "Dell Latitude freefall driver > > > (ACPI SMO88XX)" > > > > > > depends on ACPI > > > ---help--- > > > > > > - Say Y here if you want to support SMO8800/SMO8810 > > > freefall device + Say Y here if you want to support > > > SMO88XX freefall device > > > > "devices" now that multiple are supported. > > > > Thanks, > > -- > Pali Rohár > pali.ro...@gmail.com -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] platform: x86: dell-laptop: Add support for keyboard backlight
On Fri, Dec 05, 2014 at 12:53:31PM +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. > > Thanks to Dan Carpenter who reported bug about unpredictable results in > quirks->kbd_timeouts for loop. His fix adds needs_kbd_timeouts flag to > quirk structure to indicate if kbd_timeouts array is empty or not. > > Signed-off-by: Pali Rohár > Signed-off-by: Gabriele Mazzotta > Cc: Dan Carpenter Applied to testing with minor English corrections to the new comments. Thank you Pali. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses
On Fri, Dec 05, 2014 at 10:07:35PM +0100, Pali Rohár wrote: > On Friday 05 December 2014 21:41:22 Pavel Machek wrote: > > On Fri 2014-12-05 21:31:34, Pali Rohár wrote: > > > On Wednesday 03 December 2014 14:34:32 Darren Hart wrote: > > > > On Thu, Dec 04, 2014 at 12:16:20AM +0100, Gabriele > > > > Mazzotta > > > > > > wrote: > > > > > Currently dell-wmi reports keypresses for WMI events > > > > > that are notifications of changes performed by the > > > > > BIOS. This patch series make sure that no keypresses > > > > > are sent for those events so that nothing is done from > > > > > userspace. > > > > > > > > > > Gabriele Mazzotta (3): > > > > > dell-wmi: Use appropriate keycode for radio state > > > > > changes dell-wmi: Don't report keypresses for radio > > > > > state changes > > > > > > > > Merged into one patch, queued. > > > > > > > > > dell-wmi: Don't report keypresses on keybord > > > > > illumination change > > > > > > > > Queued. > > > > > > > > Thanks Gabriele. > > > > > > Darren, what do you think about sending patch into stable > > > kernel? > > > > I'd suggest against that. -stable is for "serious" bugs, and > > we don't want to change this kind of behaviour in -stable > > kernel. > > > > Pavel > > Ok, I agree that it is subjective how serious it is... > Just to remind that patch fixing problem described in > > http://www.spinics.net/lists/platform-driver-x86/msg05922.html > http://www.spinics.net/lists/platform-driver-x86/msg05924.html I don't have any objection to sending this back to stable. Stable is for fixing REAL bugs, as opposed to theorhetical races, etc. This is a "real" bug. As to not chaning behavior, if it's OK for mainline, it's OK for stable. At least that is my understanding of it. Folks are free to verify with Greg if they disagree. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] dell-smo8800: Add more ACPI ids and change description of driver
On Fri, Dec 05, 2014 at 01:05:46PM +0100, Pali Rohár wrote: > This patch adds other ACPI ids from Windows inf driver which should be handled > by dell-smo8800 driver. ACPI devices have same structure -- one IRQ number. > > This patch also updates description of module. > > Signed-off-by: Pali Rohár Queued to testing, thanks Pali. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v5 3/5] acerhdf: Use bang-bang thermal governor
On Mon, Dec 08, 2014 at 08:45:58AM +0100, Peter Feuerer wrote: > Hi Darren, > > I know you are busy and I don't want to stress you, but have you had time to > look to this? I'd be really happy to solve all issues in time and have > those patches finally applied. Not yet. I've been traveling to Mexico and wrapping up the current for-next patches. FYI, also dealing with a very sick kid these last couple of days, so I've been rather tied up. I hope to get to these in the next few days. You've provided the link to the discussion which is what I needed, thanks. Thanks, Darren > > kind regards, > --peter; > > > Peter Feuerer writes: > > >Hi again, > > > >Peter Feuerer writes: > > > >>Hi Darren, > >> > >>thank you very much for your reply. > >> > >> > >>Darren Hart writes: > >> > >>>On Fri, Nov 28, 2014 at 03:20:50PM +0100, Peter Feuerer wrote: > >>>>acerhdf has been doing an on-off fan control using hysteresis by > >>>>post-manipulating the outcome of thermal subsystem trip point handling. > >>>>This patch enables acerhdf to use the bang-bang governor, which is > >>>>intended for on-off controlled fans. > >>>> > >>>>Cc: platform-driver-x86@vger.kernel.org > >>>>Cc: Darren Hart > >>>>Cc: Andrew Morton > >>>>CC: Zhang Rui > >>>>Cc: Andreas Mohr > >>>>Cc: Javi Merino > >>>>Acked-and-tested-by: Borislav Petkov > >>>>Signed-off-by: Peter Feuerer > >>>>--- > >>>> drivers/platform/x86/Kconfig | 3 ++- > >>>> drivers/platform/x86/acerhdf.c | 36 +++- > >>>> 2 files changed, 33 insertions(+), 6 deletions(-) > >>>> > >>>>diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > >>>>index a2eabe6..c173266 100644 > >>>>--- a/drivers/platform/x86/Kconfig > >>>>+++ b/drivers/platform/x86/Kconfig > >>>>@@ -38,7 +38,8 @@ config ACER_WMI > >>>> config ACERHDF > >>>> tristate "Acer Aspire One temperature and fan driver" > >>>>- depends on THERMAL && ACPI > >>>>+ select THERMAL_GOV_BANG_BANG > >>> > >>>So we use select sparingly as it does implicit things. > >>> > >>>I checked the THERMAL_GOV_BANG_BANG Kconfig entry, and the help says > >>>acerhdf > >>>already depends on it (which it doesn't appear to). Any particular reason > >>>to add > >>>select here instead of adding it as a depends. > >>> > >>>Why did you drop THERMAL? > >> > >>I had it like this in my first version of patches: > >>+ depends on THERMAL && ACPI && THERMAL_GOV_BANG_BANG > >> > >>But after some discussion with lkml community we ended up with the > >>select line and dropped THERMAL dependency, as it is implied by > >>THEMAL_GOV_BANG_BANG. I'm not so experienced with Kconfig, so I must > >>rely on the statements of the community in this case. > > > >Just found the link about this discussion I had with Rui and Boris: > >http://linux-kernel.2935.n7.nabble.com/PATCH-0-4-acerhdf-thermal-adding-new-models-and-appropriate-governor-tp848572p908256.html > > > >[...] > > > >-- > >kind regards, > >--peter; > -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v5 3/5] acerhdf: Use bang-bang thermal governor
On Thu, Dec 04, 2014 at 08:21:06AM +0100, Peter Feuerer wrote: > Hi again, > > Peter Feuerer writes: > > >Hi Darren, > > > >thank you very much for your reply. > > > > > >Darren Hart writes: > > > >>On Fri, Nov 28, 2014 at 03:20:50PM +0100, Peter Feuerer wrote: > >>>acerhdf has been doing an on-off fan control using hysteresis by > >>>post-manipulating the outcome of thermal subsystem trip point handling. > >>>This patch enables acerhdf to use the bang-bang governor, which is > >>>intended for on-off controlled fans. > >>> > >>>Cc: platform-driver-x86@vger.kernel.org > >>>Cc: Darren Hart > >>>Cc: Andrew Morton > >>>CC: Zhang Rui > >>>Cc: Andreas Mohr > >>>Cc: Javi Merino > >>>Acked-and-tested-by: Borislav Petkov > >>>Signed-off-by: Peter Feuerer > >>>--- > >>> drivers/platform/x86/Kconfig | 3 ++- > >>> drivers/platform/x86/acerhdf.c | 36 +++- > >>> 2 files changed, 33 insertions(+), 6 deletions(-) > >>> > >>>diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > >>>index a2eabe6..c173266 100644 > >>>--- a/drivers/platform/x86/Kconfig > >>>+++ b/drivers/platform/x86/Kconfig > >>>@@ -38,7 +38,8 @@ config ACER_WMI > >>> config ACERHDF > >>> tristate "Acer Aspire One temperature and fan driver" > >>>- depends on THERMAL && ACPI > >>>+ select THERMAL_GOV_BANG_BANG > >> > >>So we use select sparingly as it does implicit things. > >> > >>I checked the THERMAL_GOV_BANG_BANG Kconfig entry, and the help says acerhdf > >>already depends on it (which it doesn't appear to). Any particular reason > >>to add > >>select here instead of adding it as a depends. > >> > >>Why did you drop THERMAL? > > > >I had it like this in my first version of patches: > >+ depends on THERMAL && ACPI && THERMAL_GOV_BANG_BANG > > > >But after some discussion with lkml community we ended up with the select > >line and dropped THERMAL dependency, as it is implied by > >THEMAL_GOV_BANG_BANG. I'm not so experienced with Kconfig, so I must rely > >on the statements of the community in this case. > > Just found the link about this discussion I had with Rui and Boris: > http://linux-kernel.2935.n7.nabble.com/PATCH-0-4-acerhdf-thermal-adding-new-models-and-appropriate-governor-tp848572p908256.html > Rui, I see you said you agree with the approach to "select THERMAL_GOV_BANG_BANG", and I'm OK with that. But that doesn't ensure, as far as I can see, a sufficient set of Kconfig dependencies for ACERHDF as THERMAL_GOV_BANG_BANG doesn't in turn select THERMAL. While it's unlikely to get ACERHDF available without THERMAL enabled, it does look possible. Should this driver also select or depends on THERMAL? Maybe: depends on ACPI depends on THERMAL select THERMAL_GOV_BANG_BANG ? > [...] > > -- > kind regards, > --peter; > -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] thinkpad-acpi: Try to use full software mute control
On Tue, Dec 09, 2014 at 05:53:42PM -0200, Henrique de Moraes Holschuh wrote: > On Mon, 08 Dec 2014, Andy Lutomirski wrote: > > On Fri, Oct 31, 2014 at 4:07 PM, Henrique de Moraes Holschuh > > wrote: > > ... > > > >> Signed-off-by: Andy Lutomirski > > > > > > Acked-by: Henrique de Moraes Holschuh > > > > What tree do these go through? I haven't spotted them in linux-next. > > X86 PLATFORM DRIVERS > M: Darren Hart > L: platform-driver-x86@vger.kernel.org > T: git > git://git.infradead.org/users/dvhart/linux-platform-drivers-x86.git > S: Maintained > F: drivers/platform/x86/ > > Darren, could you pick up this patch series, please? > > You were missing in the Cc list, and it seems to have fallen through the > cracks :-( > > Maybe we could use https://patchwork.kernel.org/ for platform-driver-x86 ? For the time being, please continue to Cc the maintainer on all patches. That's a minimum requirement anyway. I'm not opposed to discussing patchwork, but I don't want it to be a substitute to following standard mailing list practices. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/2] Disable the ThinkPad HW mute/level control when possible
On Fri, Oct 17, 2014 at 05:04:28PM -0700, Andy Lutomirski wrote: > This series is being resurrected after several years, and it's quite > different from before, so I'm restarting the version numbers :) > > ThinkPad volume and mute controls are a mess. For whatever reason, > ThinkPads have mute buttons that send KEY_MUTE *and* control an > invisible-to-ALSA mute switch. Some of them have volume controls that > interact with this switch as well. > > This is a perennial source of problems. On most ThinkPads, if you press > mute and then unmute using GUI controls, you have no sound, because > userspace and ALSA state gets out of sync with the hardware switch. > There's a separate "sound card" that exposes the hardware switch, but > userspace code generally doesn't understand that. > > There are already a few _OSI(Linux) overrides to turn all the hardware > buttons into regular buttons. Rather than quirking ACPI everywhere, > just teach thinkpad-acpi to program the buttons for full software > control and to disable hardware controls. That allows us to remove the > ACPI quirks and have normal mute controls. This approach should be > much simpler than adding even more kludgey ALSA integration for > questionable gain. > > Tested on an X200s (with latching mute by default) and an X220 (with > a mute light and toggle mute by default). Everything works as expected. > > Changes from v2: > - Don't try to change the mute mode on IBM ThinkPads. > - Further minor cleanups. > - Try to restore the mode on module unload. > > Changes from v1: > - Simplified the code a bit. > - Improved suspend/hibernate behavior. > > Andy Lutomirski (2): > thinkpad-acpi: Try to use full software mute control > acpi: Remove _OSI(Linux) for ThinkPads > > drivers/acpi/blacklist.c | 54 > drivers/platform/x86/thinkpad_acpi.c | 116 > --- > 2 files changed, 106 insertions(+), 64 deletions(-) Andy, is this the latest version? -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] TCO Watchdog warning interrupt driver creation
line, can remove.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define DRV_NAME "warn_int"
> +#define TCO_CLASS DRV_NAME
> +
> +static const struct acpi_device_id tco_device_ids[] = {
> + {"8086229C", 0},
What is this device ID? Is it specific to a debug ID for the WDT? If not, will
this eventually conflict with a non-debug driver for this WDT?
> + {"", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, tco_device_ids);
> +
> +static u32 warn_irq_handler(acpi_handle gpe_device, u32 gpe, void *context)
> +{
> + pr_warn("[SHTDWN] %s, WATCHDOG TIMEOUT HANDLER!\n", __func__);
Indenting with 13 spaces? Please review Documentation/CodingStyle and update the
patch. Rather than point out all these sorts of errors, I'm goign to stop here -
please read the doc and correct throughout.
> +
> + trigger_all_cpu_backtrace();
> + panic("Kernel Watchdog");
> +
> + /* This code should not be reached */
> + return IRQ_HANDLED;
> +}
> +
> +static int acpi_tco_add(struct acpi_device *device)
> +{
> + acpi_status status;
> + unsigned long long tco_gpe;
Declare variables in decreasing line length order.
> +
> + status = acpi_evaluate_integer(device->handle, "_GPE", NULL,
> &tco_gpe);
> + if (ACPI_FAILURE(status))
> + return -EINVAL;
> +
> + status = acpi_install_gpe_handler(NULL, tco_gpe,
> +
> ACPI_GPE_EDGE_TRIGGERED,
> +
> warn_irq_handler, NULL);
> + if (ACPI_FAILURE(status))
> + return -ENODEV;
> +
> + acpi_enable_gpe(NULL, tco_gpe);
> +
> + pr_info("initialized. Interrupt=SCI GPE 0x%02llx", tco_gpe);
> + return 0;
> +}
> +
> +static struct acpi_driver tco_driver = {
> + .name = "warn_int",
> + .class = TCO_CLASS,
> + .ids = tco_device_ids,
> + .ops = {
> + .add = acpi_tco_add,
> + },
> +};
> +
> +static int __init warn_int_init(void)
> +{
> + return acpi_bus_register_driver(&tco_driver);
> +}
> +
> +module_init(warn_int_init);
> +/* no module_exit, this driver shouldn't be unloaded */
> +
> +MODULE_AUTHOR("Francois-Nicolas Muller ");
> +MODULE_DESCRIPTION("Intel TCO WatchDog Warning Interrupt");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);
> --
> 1.7.9.5
>
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v5 0/5] acerhdf: adding new models, appropriate governor and minor clean up
On Fri, Nov 28, 2014 at 03:20:47PM +0100, Peter Feuerer wrote: > Hi Darren, > > please apply this series of patches. Applied to testing, and given the level of review and testing already received, I will include in for-next for a late merge-window submission. Thanks, -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/2] Disable the ThinkPad HW mute/level control when possible
On Tue, Dec 09, 2014 at 02:47:34PM -0800, Andy Lutomirski wrote: > On Thu, Dec 4, 2014 at 3:55 AM, Darren Hart wrote: > > On Fri, Oct 17, 2014 at 05:04:28PM -0700, Andy Lutomirski wrote: > >> This series is being resurrected after several years, and it's quite > >> different from before, so I'm restarting the version numbers :) > >> > >> ThinkPad volume and mute controls are a mess. For whatever reason, > >> ThinkPads have mute buttons that send KEY_MUTE *and* control an > >> invisible-to-ALSA mute switch. Some of them have volume controls that > >> interact with this switch as well. > >> > >> This is a perennial source of problems. On most ThinkPads, if you press > >> mute and then unmute using GUI controls, you have no sound, because > >> userspace and ALSA state gets out of sync with the hardware switch. > >> There's a separate "sound card" that exposes the hardware switch, but > >> userspace code generally doesn't understand that. > >> > >> There are already a few _OSI(Linux) overrides to turn all the hardware > >> buttons into regular buttons. Rather than quirking ACPI everywhere, > >> just teach thinkpad-acpi to program the buttons for full software > >> control and to disable hardware controls. That allows us to remove the > >> ACPI quirks and have normal mute controls. This approach should be > >> much simpler than adding even more kludgey ALSA integration for > >> questionable gain. > >> > >> Tested on an X200s (with latching mute by default) and an X220 (with > >> a mute light and toggle mute by default). Everything works as expected. > >> > >> Changes from v2: > >> - Don't try to change the mute mode on IBM ThinkPads. > >> - Further minor cleanups. > >> - Try to restore the mode on module unload. > >> > >> Changes from v1: > >> - Simplified the code a bit. > >> - Improved suspend/hibernate behavior. > >> > >> Andy Lutomirski (2): > >> thinkpad-acpi: Try to use full software mute control > >> acpi: Remove _OSI(Linux) for ThinkPads > >> > >> drivers/acpi/blacklist.c | 54 > >> drivers/platform/x86/thinkpad_acpi.c | 116 > >> --- > >> 2 files changed, 106 insertions(+), 64 deletions(-) > > > > Andy, is this the latest version? > > Yes. Queued to for-next. Pending anything unforseen, I'll attempt to get this into 3.19. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v5 0/5] acerhdf: adding new models, appropriate governor and minor clean up
On Fri, Nov 28, 2014 at 03:20:47PM +0100, Peter Feuerer wrote: > Hi Darren, > > please apply this series of patches. > > It is intended to: > > * Introduce "manual mode" support (Patch 1 & 2), which is needed to control > the fan of a few new models. > > * Add an appropriate thermal governor (Patch 3). Manipulating and > fiddling around with the step-wise governor has been a very fragile thing > in the past and as it broke again, I used the opportunity to add a two > point thermal governor which implements the actual fan handling required > by > acerhdf and puts from my point of view things straight. > > * Do some minor clean up like: > - adding second trip point for critical temperature (Patch 4) > - removing _t suffix from struct which isn't typedef and replace > unsigned > char by u8 (Patch 5) > > Thanks and kind regards, > peter > > Peter Feuerer (5): > acerhdf: Adding support for "manual mode" > acerhdf: Adding support for new models > acerhdf: Use bang-bang thermal governor > acerhdf: added critical trip point > acerhdf: minor clean up Queued to for-next. Thanks! -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation: Add entry for dell-laptop sysfs interface
On Wed, Dec 03, 2014 at 06:41:33PM +0100, Gabriele Mazzotta wrote: > Add the documentation for the new sysfs interface of dell-laptop > that allows to configure the keyboard illumination on Dell systems. Queued to for-next. Thanks, -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hp_accel: Add support for HP ZBook 15
On Thu, Dec 11, 2014 at 11:26:07AM +0100, Éric Piel wrote: > On 13-11-14 20:57, Takashi Iwai wrote: > >From: Dominique Leuenberger > > > >HP ZBook 15 laptop needs a non-standard mapping (x_inverted). > > > >BugLink: http://bugzilla.opensuse.org/show_bug.cgi?id=905329 > >Signed-off-by: Dominique Leuenberger > >Cc: > >Signed-off-by: Takashi Iwai > Sorry, it got a be delayed... but it looks completely fine :-) > > Signed-off-by: Éric Piel > > Darren, would you wind picking up this patch via your tree? Yes, already in next. Thanks :-) -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH -next] platform/x86/acerhdf: still depends on THERMAL
On Mon, Dec 15, 2014 at 09:00:13AM -0800, Randy Dunlap wrote: > From: Randy Dunlap > > acerhdf uses thermal interfaces so it should depend on THERMAL. > It also should not select a thermal driver without checking that > THERMAL is enabled. > > This fixes the following build errors when THERMAL=m and > ACERHDF=y. Thank you Randy, I'll include this in my pull request later this week. Now in for-next. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses
On Sat, Dec 20, 2014 at 04:11:08PM +0100, Pavel Machek wrote: > Hi! > > > > > Ok, I agree that it is subjective how serious it is... > > > > Just to remind that patch fixing problem described in > > > > > > > > http://www.spinics.net/lists/platform-driver-x86/msg05922.ht > > > > ml > > > > http://www.spinics.net/lists/platform-driver-x86/msg05924.h > > > > tml > > > > > > I don't have any objection to sending this back to stable. > > > Stable is for fixing REAL bugs, as opposed to theorhetical > > > races, etc. This is a "real" bug. > > > > > > As to not chaning behavior, if it's OK for mainline, it's OK > > > for stable. At least that is my understanding of it. Folks > > > are free to verify with Greg if they disagree. > > > > Darren, so how you decided? Now when patches are in linus tree, > > are you going to send them to stable tree? > > Please don't. -stable is for serious mainline bugs people are actually > hitting. Null pointer dereference counts, if people actually hit > it. This is more behaviour change, and yes, the new behaviour is > better, but it is really different class. In this case I agree with Pavel. While the patches are small enough, fix one thing each, etc, it isn't clear from the description exactly how these patches affect users. If you can articulate how they are "real bugs that bother people" (see stable_kernel_rules.txt) we can reconsider. I should have pushed for better commit messages on these it appears as this should be obvious from those, but it isn't - at least not to me at 8:15am ;-) -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses
On December 20, 2014 8:28:04 AM PST, Henrique de Moraes Holschuh wrote: >On Sat, 20 Dec 2014, Pavel Machek wrote: >> > > > Ok, I agree that it is subjective how serious it is... >> > > > Just to remind that patch fixing problem described in >> > > > >> > > > http://www.spinics.net/lists/platform-driver-x86/msg05922.ht >> > > > ml >> > > > http://www.spinics.net/lists/platform-driver-x86/msg05924.h >> > > > tml >> > > >> > > I don't have any objection to sending this back to stable. >> > > Stable is for fixing REAL bugs, as opposed to theorhetical >> > > races, etc. This is a "real" bug. >> > > >> > > As to not chaning behavior, if it's OK for mainline, it's OK >> > > for stable. At least that is my understanding of it. Folks >> > > are free to verify with Greg if they disagree. >> > >> > Darren, so how you decided? Now when patches are in linus tree, >> > are you going to send them to stable tree? >> >> Please don't. -stable is for serious mainline bugs people are >actually >> hitting. Null pointer dereference counts, if people actually hit >> it. This is more behaviour change, and yes, the new behaviour is >> better, but it is really different class. > >Sometimes the old behavior is something that is a major pain for users >and >userspace. In that case, where the new behavior fixes really annoying >usecase bugs, the fix belongs in -stable IMHO. > >Broken behavior hits, by definition, every user of the feature after >all. How is this behavior affecting users with this hardware today? How does it manifest? Does it make for a bad or non-functional user experience? Again, I should have pressed for more complete commit messages, apologies for that. -- Sent from my Android device with K-9 Mail. Please excuse my brevity. -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] dell-wmi: Don't send unneeded keypresses
On Sat, Dec 20, 2014 at 06:03:54PM +0100, Gabriele Mazzotta wrote: > On Saturday 20 December 2014 08:16:54 Darren Hart wrote: > > On Sat, Dec 20, 2014 at 04:11:08PM +0100, Pavel Machek wrote: > > > Hi! > > > > > > > > > Ok, I agree that it is subjective how serious it is... > > > > > > Just to remind that patch fixing problem described in > > > > > > > > > > > > http://www.spinics.net/lists/platform-driver-x86/msg05922.ht > > > > > > ml > > > > > > http://www.spinics.net/lists/platform-driver-x86/msg05924.h > > > > > > tml > > > > > > > > > > I don't have any objection to sending this back to stable. > > > > > Stable is for fixing REAL bugs, as opposed to theorhetical > > > > > races, etc. This is a "real" bug. > > > > > > > > > > As to not chaning behavior, if it's OK for mainline, it's OK > > > > > for stable. At least that is my understanding of it. Folks > > > > > are free to verify with Greg if they disagree. > > > > > > > > Darren, so how you decided? Now when patches are in linus tree, > > > > are you going to send them to stable tree? > > > > > > Please don't. -stable is for serious mainline bugs people are actually > > > hitting. Null pointer dereference counts, if people actually hit > > > it. This is more behaviour change, and yes, the new behaviour is > > > better, but it is really different class. > > > > In this case I agree with Pavel. While the patches are small enough, fix one > > thing each, etc, it isn't clear from the description exactly how these > > patches > > affect users. > > > > If you can articulate how they are "real bugs that bother people" (see > > stable_kernel_rules.txt) we can reconsider. I should have pushed for better > > commit messages on these it appears as this should be obvious from those, > > but it > > isn't - at least not to me at 8:15am ;-) > > The problem is that userspace programs responds to those keypresses when > they shouldn't. > > In case of KEY_KBDILLUMTOGGLE, the illumination level is changed by the > BIOS, so if the keypress is reported, userspace programs will try to > toggle the keyboard illumination after the BIOS changed the level. > This is even more problematic if you consider that there could be > multiple illumination levels that are not taken into account if a > KEY_KBDILLUMTOGGLE is sent. Userspace will simply turn ON/OFF the > illumination, interfering with the BIOS. > This is shouldn't be a major problem since dell-laptop can control the > keyboard illumination only now and I can't see what userspace > application can misbehave without this change. Agreed, this one should not go back to stable. > > In the case of KEY_WLAN/KEY_RFKILL, the BIOS already takes care of > everything when the key is pressed, so sending keypresses as if > userspace programs have to do something is wrong. If for instance the > WiFi rfkill is soft blocked and I press the Fn key twice, I want it > to be soft blocked at the end. However, this is not the case. These sound like good candidates, real bugs that bother people. I would support sending them back to stable. Since we didn't have this discussion before they went mainline, sorry it's been a bad month for me :-/, these need to be sent manually. Pali, Gabriele, please have a look at stable_kernel_rules, determine how far back these should go, and submit the patches to the stable list. > Sorry for the brief commit messages. > They didn't bother me at the time as I saw the improvement, but they weren't enough to make the stable decision and I need to watch out for that in the future. Lesson learned :-) Thanks everyone, -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] TCO Watchdog warning interrupt driver creation
On December 22, 2014 5:18:33 AM PST, "Muller, Francois-nicolas" wrote: >Thanks Darren and Guenter for your review. >Here is a new patchset, including the feature in the watchdog driver >and using a boot parameter to be enabled. >François-Nicolas > >Subject: [PATCH] TCO watchdog warning interrupt handler > Please provide a complete commit message. >Signed-off-by: Francois-Nicolas Muller > -- Darren Hart Sent from my Android device with K-9 Mail. Please excuse my brevity. -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] x86: Add IMR support to Quark/Galileo
On Wed, Dec 31, 2014 at 12:12:58PM +0200, Andy Shevchenko wrote: > On Mon, Dec 29, 2014 at 7:23 PM, Bryan O'Donoghue > wrote: > > This patchset adds an IMR driver to the kernel plus platform code for > > Intel Galileo Gen1/Gen2 boards. > > [] > > > Bryan O'Donoghue (2): > > x86: Add Isolated Memory Regions for Quark X1000 > > platform/x86 Add Intel Galileo platform specific setup > > I'm going to review this soon, but here few comments below. Thanks for your review Andy, good advice throughout. > > > arch/x86/Kconfig | 23 ++ > > arch/x86/include/asm/imr.h | 79 ++ > > arch/x86/include/asm/intel-quark.h | 31 ++ > > Could it be just a quark.h? Like for ce4100. > Those intel- prefixes in the modules looks awkward especially when > pathname consists x86 already. > > > arch/x86/kernel/Makefile | 1 + > > arch/x86/kernel/imr.c| 529 > > +++ > > drivers/platform/x86/Kconfig | 15 + > > drivers/platform/x86/Makefile| 1 + > > drivers/platform/x86/intel_galileo.c | 175 > > Here what about to make an hierarchy like: > intel/galileo.c > intel/mid/... would be those modules with intel_mid_ prefixes in > future. See my proposal regarding to drivers/mfd [1] As Bryan is only adding one file to the platform/drivers/x86, let's skip the reorg as part of this patch series. We can consider that separately if someone wants to make the argument that the time has come to add another directory layer. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] x86: Add IMR support to Quark/Galileo
On Mon, Dec 29, 2014 at 05:23:01PM +, Bryan O'Donoghue wrote: > This patchset adds an IMR driver to the kernel plus platform code for > Intel Galileo Gen1/Gen2 boards. I will be taking a closer look at this on Monday the 5th when I return from vacation. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation: Add entry for dell-laptop sysfs interface
On Wed, Dec 31, 2014 at 12:20:59PM +0100, Gabriele Mazzotta wrote: > On Tuesday 30 December 2014 21:31:49 Brian Norris wrote: > > Hi, > > > > On Wed, Dec 03, 2014 at 06:41:33PM +0100, Gabriele Mazzotta wrote: > > > Add the documentation for the new sysfs interface of dell-laptop > > > that allows to configure the keyboard illumination on Dell systems. > > > > > > Signed-off-by: Gabriele Mazzotta > > > Signed-off-by: Pali Rohár > > > --- > > > .../ABI/testing/sysfs-platform-dell-laptop | 60 > > > ++ > > > 1 file changed, 60 insertions(+) > > > 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..7969443 > > > --- /dev/null > > > +++ b/Documentation/ABI/testing/sysfs-platform-dell-laptop > > > @@ -0,0 +1,60 @@ > > > +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 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. > > [...] > > > > This entry appears wrong, or at least incomplete. My system boots with a > > default value of 18, and the 'set' implementation accepts any value from > > 0 to 255. > > > > According to the comments in dell-laptop.c, the value actually means > > something different than on/off: > > > > cbArg3, byte0 Desired setting of ALS value that turns the light on or > > off. > > > > Admittedly, I'm not clear on what the intended interface is, as I've > > never had the ALS / keyboard backlight controls all working properly on > > my laptop in the first place, but I thought this would be the right > > place to ask about getting the documentation and driver in sync. > > Hi, > > You are perfectly right, the documentation is wrong and I'm sorry for > that. als_setting should allow you to define when the keyboard > backlight has to be turned on or off depending on the value reported > by the ambient light sensor. > > Please note that not everything that is in libsmbios [1] is implemented > in the kernel driver. Take a look at it, recently there have been few > changes related to the ALS settings. Probably they are not needed given > that you are able to set the threshold with the current kernel driver, > but you might find something interesting. > > I'll submit a patch to correct the documentation, thanks for reporting. Thanks Gabriele, let's make sure to get this into this rc cycle, sooner the better. Thanks, -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] x86: Add IMR support to Quark/Galileo
On Mon, Dec 29, 2014 at 05:23:01PM +, Bryan O'Donoghue wrote: > This patchset adds an IMR driver to the kernel plus platform code for > Intel Galileo Gen1/Gen2 boards. > > IMRs: > Quark SoC X1000 ships with a set of registers called Isolated Memory Regions > IMRs provide fine grained memory access control to various system agents > within the SoC such as CPU SMM/non-SMM mode, PCIe virtual channels, CPU snoop > cycles, eSRAM flush cycles and the RMU. In simple terms, IMRs provide a > mechanism to protect memory regions from unwarranted access by system agents > that should not have access to that memory. > > IMRs support a lock bit. Once a lock bit is set for an individual IMR it is > not possible to tear down that IMR without performing a cold boot of the > system. IMRs support reporting of violations. The SoC system can be > configured to reboot immediately when an IMR violation has taken place. > Immediate reboot of the system on IMR violation is recommended and is > currently how Quark BIOS configures the system. > > As an example Galileo boards ship with an IMR around the ACPI runtime > services memory and if a DMA read/write cycle were to occur to this region > of memory this would trigger the IMR violation mechansim. > > Galileo: > Intel's Arduino compatible Galileo boards boot to Linux with IMRs protecting > the compressed kernel image and boot params data structure. The memory that What is the motivation behind this? > the compressed kernel and boot params data structure is in, is marked as > usable memory by the EFI memory map. As a result it is possible for memory Based on your response to the above, is marking this memory as usable a bad idea in general? Or just bad in certain situations? > marked as processor read/write only in an IMR to be given to devices in the > SoC for the purposes of DMA by way of dma_alloc_coherent. New line > A DMA to a region of memory by a system agent which is not allowed access > this memory result in a system reset. Without tearing down the IMRs placed > around the compressed kernel image and boot params data structure there is a > high risk of triggering an inadvertent system reset when performing DMA > actions with any of the peripherals that support DMA in Quark such as the > MMC, Ethernet or USB host/device. > > Therefore Galileo specific platform code is the second component of this > patchset. The platform code tears-down every unlocked IMR to ensure no The firmware sets these IMRs, but does not lock them then, correct? > conflict exists between the IMR usage during boot and the EFI memory map. In > addition an IMR is placed around the kernel's .text section to ensure no > invalid access to kernel code can happen by way of spurious DMA, SMM or RMU > read/write cycles. This code gets compiled into the kernel because we want > to run the code early before any DMA has taken place. The prime examples of > DMA transactions resetting the system are mouting a root filesystem on MMC mounting > or mouting a root filesystem over NFS. mounting -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000
_MASK)) || (size & (IMR_MASK))) {
> + pr_warn("imr: base 0x%08lx size 0x%08lx must align to 1k\n",
> + base, size);
> + dump_stack();
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +/**
> + * imr_add_range
> + * Add an Isolated Memory Region
> + *
> + * @base: Physical base address of region aligned to 4k
> + * @size: Physical size of region in bytes
> + * @read_mask: Read access mask
> + * @write_mask: Write access mask
> + * @lock: Indicates whether or not to permenantly lock this region
> + * @return: Index of the IMR allocated or negative value indicating error
> + */
> +int imr_add(unsigned long base, unsigned long size,
> + unsigned int rmask, unsigned int wmask, bool lock)
> +{
> + unsigned long end = base + size;
> + struct imr imr;
> + int reg, i, overlap, ret;
> +
> + if (imr_check_range(base, size))
> + return -EINVAL;
> +
> + if (!size) {
> + pr_warn("imr: invalid size zero!\n");
> + return -EINVAL;
> + }
> +
> + mutex_lock(&imr_dev.lock);
> +
> + /* Find an free IMR while checking for an existing overlapping range */
> + overlap = 0;
> + reg = -1;
> + for (i = 0; i <= imr_dev.num; i++) {
> + ret = imr_read(i, &imr);
> + if (ret)
> + goto done;
> +
> + /* Find overlap */
> + if (imr_enabled(&imr)) {
> + if (base >= imr_to_phys(imr.addr_lo) &&
> + base <= imr_to_phys(imr.addr_hi)) {
> + overlap = 1;
> + break;
> + }
> + if (end >= imr_to_phys(imr.addr_lo) &&
> + end <= imr_to_phys(imr.addr_hi)) {
> + overlap = 1;
> + break;
> + }
> + } else {
> + reg = i;
> + }
> + }
> +
> + /* Error out if we have an overlap or no free IMR entries */
According to the datasheet, overlapping ranges are valid, and access must be
granted by all IMRs associated with a given address. Why declare an overlap as
invalid here?
> + if (overlap) {
> + ret = -EINVAL;
> + goto done;
> + }
> + if (reg == -1) {
> + ret = -ENODEV;
> + goto done;
> + }
...
Thanks,
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] x86: Add IMR support to Quark/Galileo
On Tue, Jan 06, 2015 at 01:56:25PM +, Bryan O'Donoghue wrote: > On 06/01/15 06:00, Darren Hart wrote: > >>Galileo: > >>Intel's Arduino compatible Galileo boards boot to Linux with IMRs protecting > >>the compressed kernel image and boot params data structure. The memory that > > > >What is the motivation behind this? > > The main motivation is to place an IMR around the kernel which if violated > by a wayward DMA transaction would immediately cause an IMR violation. > Secondary motivation is demonstration of usage of IMRs in a run-time context > and validation of the IMR enabling code for setup not just tear-down. > > An IMR around the run-time kernel .text area, is what the BSP does and so I > thought it was worth maintaining. > > To be clear though, the requirement is to sanitize boot time IMRs, the setup > of an IMR around the run-time kernel is optional, the system will run just > fine without it. > > >>the compressed kernel and boot params data structure is in, is marked as > >>usable memory by the EFI memory map. As a result it is possible for memory > > > >Based on your response to the above, is marking this memory as usable a bad > >idea > >in general? Or just bad in certain situations? > > The EFI memory map is 100% correct. The area of memory that grub places a > compressed kernel image should be reusable by kernel. > > >>A DMA to a region of memory by a system agent which is not allowed access > >>this memory result in a system reset. Without tearing down the IMRs placed > >>around the compressed kernel image and boot params data structure there is a > >>high risk of triggering an inadvertent system reset when performing DMA > >>actions with any of the peripherals that support DMA in Quark such as the > >>MMC, Ethernet or USB host/device. > >> > >>Therefore Galileo specific platform code is the second component of this > >>patchset. The platform code tears-down every unlocked IMR to ensure no > > > >The firmware sets these IMRs, but does not lock them then, correct? > > Correct. Firmware locks the IMRs around ACPI runtime data data. > > In the patch here, we cycle though every unlocked IMR and zap it - which > will include tear-down of the IMRs placed around the compressed kernel image > and boot params data structure. Firmware puts those IMRs in place to ensure > no invalid DMA, SMM access to the compressed kernel image during boot can > take place. Thanks Bryan, this clears these questions up for me. Are we confident that the tear-down of the IMRs around the compressed kernel image happens early enough and deterministically, such that there is no race condition in which a driver could get DMA into this memory? -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000
On Tue, Jan 06, 2015 at 01:43:23PM +, Bryan O'Donoghue wrote:
> On 06/01/15 07:36, Darren Hart wrote:
>
> >>Signed-off-by: Bryan O'Donoghue
> >
> >Since you have Intel (C) below and then your own, are you the sole author?
>
> Yes, for the platform code.
>
> Platform code tears-down IMRs and sets-up up a new one around the kernel.
> Quark BSP does a similar thing in a different place.
>
> To be safe I'm happy to add a Intel (C) to this file anyway.
I was just checking if we needed to credit another individual with code
authorship.
...
> >>+#define IMR_SHIFT 8
> >>+#define imr_to_phys(x) (x << IMR_SHIFT)
> >>+#define phys_to_imr(x) (x >> IMR_SHIFT)
> >>+
> >>+/**
> >>+ * imr_enabled
> >>+ * Determines if an IMR is enabled based on address range
> >>+ *
> >>+ * @imr: Pointer to IMR descriptor
> >>+ * @return true if IMR enabled false if disabled
> >>+ */
> >>+static int imr_enabled(struct imr *imr)
> >>+{
> >>+ return (imr_to_phys(imr->addr_lo) && imr_to_phys(imr->addr_hi));
> >
> >What are testing here? You have bit 30 marked as "Enabled" above (but it
> >isn't
> >in the datasheet). With Reserved bits in each register, this test for
> >non-zero
> >doesn't seem robust.
>
> Good question.
>
> Bit 30 is not present in X1000 silicon, though is present in the BSP code.
> Lets forget about all non-X1000 cases for now since X1000 is the only
> processor currently available.
>
> On X1000 the only means of determining if an IMR is enabled is if it's
> address bits are set to some address everybody agrees means 'off', since the
> enable bit doesn't exist. Everybody in this case is ROM, BIOS, 2nd stage
> bootloader and kernel.
>
OK, that's non-obvious, so let's add a comment.
> In the BSP code, that address is zero. So in lieu of bit 30 'enable' we have
> address 0x.
>
> The kernel could define an alternative address to 0x but, then this
> would break with the convention in BIOS etc.
>
> Since BIOS and grub code both use 0x as the 'off' address I think it
> makes sense for the kernel to continue to use that address.
So, both lo and hi don't need to be non-zero then, either one being non-zero
would constitute "enabled"? Should the above test be an || then?
>
> >>+ /* Error out if we have an overlap or no free IMR entries */
> >
> >According to the datasheet, overlapping ranges are valid, and access must be
> >granted by all IMRs associated with a given address. Why declare an overlap
> >as
> >invalid here?
>
> Simplicity.
>
> It's more straight forward to define your IMR memory map if you don't allow
> the address ranges to stomp all over each other.
>
> None of the BSP reference code does overlap.
>
> If there's an argument for an overlap use-case it can be supported pretty
> simply by simply removing the overlap checks.
>
> My own view is that it's not really desirable and easier to debug IMRs
> generally on a platform if overlaps aren't allowed.
>
OK, let's add a comment to that effect.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] x86: Add Isolated Memory Regions for Quark X1000
On Thu, Jan 08, 2015 at 03:11:35PM +, Bryan O'Donoghue wrote:
>
> >Suggest to split the imr_del() into 2 functions:-
> >(1) by address + size
> >(2) by IMR index
> >At current implementation, it does not support (2) only because it fails at
> >imr_check_range().
>
> Hi Boon Leong.
>
> I'll have a think about that :)
>
> Just on imr_del() though, it does support removal by way of index.
>
> +static void __init intel_galileo_imr_init(void)
> +{
> + unsigned long base = virt_to_phys(&_text);
> + unsigned long size = virt_to_phys(&_sinittext) - base - IMR_ALIGN;
> + int i, ret;
> +
> + /* Tear down all existing unlocked IMRs */
> + for (i = 0; i <= QUARK_X1000_IMR_NUM; i++)
> + imr_del(i, 0, 0);
>
> That's what the platform code has to do for every unlocked IMR, to make sure
> there are no stale IMRs left that could conflict with the EFI memory map !
I'm OK with a single function so long as by index works without having to
specify the address. Please update the kernel doc to describe this usage though.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup
%p)\n",
> + size >> 10, &_text, &__init_begin);
> +#ifdef DEBUG
> + intel_galileo_imr_sanity(base, size);
> +#endif
> +}
> +
> +/**
> + * intel_galileo_init
> + *
> + * Identify a Galileo Gen1 or Gen2. If found run code to sanitise the
> + * kernel memory space of IMRs that are inconsistent with the EFI memory map.
> + */
> +static int __init intel_galileo_init(void)
> +{
> + int ret = 0, type = GALILEO_UNKNOWN;
One var per line, ret last. Order by descending line length when in doubt.
> + struct cpuinfo_x86 *c = &cpu_data(cpu);
> + const struct dmi_system_id *system_id;
> +
> + if (!cpu_is_quark(c))
> + return -ENODEV;
> +
> + system_id = dmi_first_match(galileo_baseboards);
> +
> + /* BIOS releases 0.7.5 and 0.8.0 do not provide DMI strings */
> + if (system_id != NULL) {
> + type = (int)system_id->driver_data;
> + } else {
> + pr_info("Galileo Gen1 BIOS version <= 0.8.0\n");
> + type = GALILEO_QRK_GEN1;
So this will load on any Quark device, Galileo or not, that doesn't provide a
system_id. Is there any reason we need to support 0.8.0 and earlier firmware?
I'd prefer not to successfully load a driver on the wrong platform because we
assume the user knows what they are doing :-) This effective converts this from
a "platform driver" to a "board file" - the bad kind.
> + }
> +
> + switch (type) {
> + case GALILEO_QRK_GEN1:
> + case GALILEO_QRK_GEN2:
> + intel_galileo_imr_init();
> + break;
> + default:
> + ret = -ENODEV;
> + }
> +
> + return ret;
> +}
> +
> +static void __exit intel_galileo_exit(void)
> +{
> +}
> +
> +module_init(intel_galileo_init);
> +module_exit(intel_galileo_exit);
> +
> +MODULE_AUTHOR("Bryan O'Donoghue ");
> +MODULE_DESCRIPTION("Intel Galileo platform driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
>
Thanks,
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] platform/x86: Add Intel Galileo platform specific setup
On Fri, Jan 09, 2015 at 11:17:43AM +, Bryan O'Donoghue wrote:
> On 09/01/15 04:46, Darren Hart wrote:
> >>+ /* Try overlap - IMR_ALIGN */
> >>+ base = base + size - IMR_ALIGN;
> >>+ if (imr_add(base, size, IMR_NON_SMM, IMR_NON_SMM, true) == 0)
> >>+ pr_err(SANITY "overlapped IMR @ (0x%08lx - 0x%08lx)\n",
> >>+ base, base + size);
> >>+}
> >>+#endif
> >
> >I'd rather see this as CONFIG_DEBUG_IMR under Kernel Hacking.
> >
> >What about this sanity test is galileo specific?
>
> Exactly nothing.
>
> I think it's a fair point to make this
> * CONFIG_DEBUG_IMR
> * Embedded in the IMR init code
>
> >>+ /* BIOS releases 0.7.5 and 0.8.0 do not provide DMI strings */
> >>+ if (system_id != NULL) {
> >>+ type = (int)system_id->driver_data;
> >>+ } else {
> >>+ pr_info("Galileo Gen1 BIOS version <= 0.8.0\n");
> >>+ type = GALILEO_QRK_GEN1;
> >
> >So this will load on any Quark device, Galileo or not, that doesn't provide a
> >system_id. Is there any reason we need to support 0.8.0 and earlier firmware?
>
> Every Galileo Gen1 device ships with firmware version 0.7.5. You can do an
> EFI capsule update to 0.8.0 which still isn't DMI-enabled - or you can go
> and get a firmware greater than 0.9.0 and get DMI strings.
>
> >I'd prefer not to successfully load a driver on the wrong platform because we
> >assume the user knows what they are doing :-) This effective converts this
> >from
> >a "platform driver" to a "board file" - the bad kind.
>
> That would be a problem if there were any other X1000 boards that don't
> provide DMI strings but only Galileo Gen1 out of the box is DMI deprived, so
> for that reason I think falling back to assume Gen1 when you've identified a
> Quark X1000 is the right thing to do.
>
> Right now the only Quark X1000 devices that real users in the field have are
> Galileo boards which either identify with DMI strings (Gen2 and upgraded
> Gen1 boards) - or don't identify with DMI strings Gen1 @ 0.7.5 and 0.8.0
>
> Also consider that any new X1000 based systems running Linux will be based
> on the latest reference firmware from Intel which provides DMI identifiers,
> so Galileo Gen3 if it is in the making will have a DMI string to identify
> itself as a Gen3, same with a Gen2 and upgraded Gen1.
>
> The only X1000 based boards without DMI strings are going to be Galileo Gen1
> devices @ firmware version 0.7.5 and 0.8.0, so I don't think we will end up
> in a situation of loading the wrong platform code, rather we'll accommodate
> the older firmware this way
>
> All that said - there *is* an alternative for 0.7.5 and 0.8.0 firmware but,
> I know you won't like it.
>
> Prior to 0.9.0 firmware Galileo boards were identified by
>
> 1. Mapping a section of SPI flash
> 2. Finding a specific header at a know location on SPI flash
> 3. Extracting a unique platform ID field from that header
> 4. Examining that platform ID field and loading Galileo specific drivers if
> found
>
> So in theory we could go this route if you feel that fallback the fallback
> described above isn't robust enough.
>
> I'm OK to add that code in for V2 and we can decide which is the more
> desirable way to do it - fallback or extract of platform id from SPI flash
> and then finalise at V3
I'm wondering if there is a need for this to be a platform driver at
all. Could we, instead, tear down any unlocked IMRs at boot as part of the IMR
driver itself, as well as lock the kernel .text. Firmware has the option of
making an IMR mandatory by locking it. If it isn't locked, what use is it to the
OS without a lot more context? Any sort of policy enforced with IMRs is a
user-space decision, so clearing out the unlocked ones and then handing off to
user-space seems like a sane default mode to me.
I discussed this with Boon Leong this afternoon and we couldn't come up with a
justification for a platform-specific driver to do what this does.
Cc'ing Mel for his thoughts on the VM bit.
Cc'ing Matthew for his take on platform driver appropriateness.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation: Add entry for dell-laptop sysfs interface
On Wed, Dec 31, 2014 at 12:20:59PM +0100, Gabriele Mazzotta wrote: > On Tuesday 30 December 2014 21:31:49 Brian Norris wrote: > > Hi, > > > > On Wed, Dec 03, 2014 at 06:41:33PM +0100, Gabriele Mazzotta wrote: > > > Add the documentation for the new sysfs interface of dell-laptop > > > that allows to configure the keyboard illumination on Dell systems. > > > > > > Signed-off-by: Gabriele Mazzotta > > > Signed-off-by: Pali Rohár > > > --- > > > .../ABI/testing/sysfs-platform-dell-laptop | 60 > > > ++ > > > 1 file changed, 60 insertions(+) > > > 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..7969443 > > > --- /dev/null > > > +++ b/Documentation/ABI/testing/sysfs-platform-dell-laptop > > > @@ -0,0 +1,60 @@ > > > +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 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. > > [...] > > > > This entry appears wrong, or at least incomplete. My system boots with a > > default value of 18, and the 'set' implementation accepts any value from > > 0 to 255. > > > > According to the comments in dell-laptop.c, the value actually means > > something different than on/off: > > > > cbArg3, byte0 Desired setting of ALS value that turns the light on or > > off. > > > > Admittedly, I'm not clear on what the intended interface is, as I've > > never had the ALS / keyboard backlight controls all working properly on > > my laptop in the first place, but I thought this would be the right > > place to ask about getting the documentation and driver in sync. > > Hi, > > You are perfectly right, the documentation is wrong and I'm sorry for > that. als_setting should allow you to define when the keyboard > backlight has to be turned on or off depending on the value reported > by the ambient light sensor. > > Please note that not everything that is in libsmbios [1] is implemented > in the kernel driver. Take a look at it, recently there have been few > changes related to the ALS settings. Probably they are not needed given > that you are able to set the threshold with the current kernel driver, > but you might find something interesting. > > I'll submit a patch to correct the documentation, thanks for reporting. Hi Gabriele, Have I missed a patch from you for this? I'd like to see this corrected in the 3.19 rc cycle. Thanks, -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] samsung-laptop: Add use_native_backlight quirk, and enable it on some models
On Fri, Jan 09, 2015 at 03:10:01PM +0100, Hans de Goede wrote:
> Since kernel 3.14 the backlight control has been broken on various Samsung
> Atom based netbooks. This has been bisected and this problem happens since
> commit b35684b8fa94 ("drm/i915: do full backlight setup at enable time")
>
> This has been reported and discussed in detail here:
> http://lists.freedesktop.org/archives/intel-gfx/2014-July/049395.html
>
> Unfortunately no-one has been able to fix this. This only affects Samsung
> Atom netbooks, and the Linux kernel and the BIOS of those laptops have never
> worked well together. All affected laptops already have a quirk to avoid using
> the standard acpi-video interface and instead use the samsung specific SABI
> interface which samsung-laptop uses. It seems that recent fixes to the i915
> driver have also broken backlight control through the SABI interface.
>
> The intel_backlight driver OTOH works fine, and also allows for finer grained
> backlight control. So add a new use_native_backlight quirk, and replace the
> broken_acpi_video quirk with this quirk for affected models. This new quirk
> disables acpi-video as before and also stops samsung-laptop from registering
I take this to me this quirk is broken_acpi_video && use_native_backlight.
> the SABI based samsung_laptop backlight interface, leaving only the working
> intel_backlight interface.
>
> This commit enables this new quirk for 3 models which are known to be
> affected,
> chances are that it needs to be used on other models too.
>
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1094948 # N145P
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1115713 # N250P
> Reported-by: Bertrik Sikken # N150P
> Cc: sta...@vger.kernel.org # 3.16
> Signed-off-by: Hans de Goede
> ---
> drivers/platform/x86/samsung-laptop.c | 20 +---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/samsung-laptop.c
> b/drivers/platform/x86/samsung-laptop.c
> index ff765d8..ce364a4 100644
> --- a/drivers/platform/x86/samsung-laptop.c
> +++ b/drivers/platform/x86/samsung-laptop.c
> @@ -353,6 +353,7 @@ struct samsung_quirks {
> bool broken_acpi_video;
> bool four_kbd_backlight_levels;
> bool enable_kbd_backlight;
> + bool use_native_backlight;
> };
>
> static struct samsung_quirks samsung_unknown = {};
> @@ -361,6 +362,10 @@ static struct samsung_quirks samsung_broken_acpi_video =
> {
> .broken_acpi_video = true,
> };
>
> +static struct samsung_quirks samsung_use_native_backlight = {
> + .use_native_backlight = true,
Shouldn't this also set:
.broken_acpi_video = true,
?
That's what I understood from the commit log.
Thanks,
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] samsung-laptop: Add use_native_backlight quirk, and enable it on some models
On Tue, Jan 13, 2015 at 08:42:45AM +0100, Hans de Goede wrote:
> Hi,
>
> On 13-01-15 05:03, Darren Hart wrote:
> >On Fri, Jan 09, 2015 at 03:10:01PM +0100, Hans de Goede wrote:
> >>Since kernel 3.14 the backlight control has been broken on various Samsung
> >>Atom based netbooks. This has been bisected and this problem happens since
> >>commit b35684b8fa94 ("drm/i915: do full backlight setup at enable time")
> >>
> >>This has been reported and discussed in detail here:
> >>http://lists.freedesktop.org/archives/intel-gfx/2014-July/049395.html
> >>
> >>Unfortunately no-one has been able to fix this. This only affects Samsung
> >>Atom netbooks, and the Linux kernel and the BIOS of those laptops have never
> >>worked well together. All affected laptops already have a quirk to avoid
> >>using
> >>the standard acpi-video interface and instead use the samsung specific SABI
> >>interface which samsung-laptop uses. It seems that recent fixes to the i915
> >>driver have also broken backlight control through the SABI interface.
> >>
> >>The intel_backlight driver OTOH works fine, and also allows for finer
> >>grained
> >>backlight control. So add a new use_native_backlight quirk, and replace the
> >>broken_acpi_video quirk with this quirk for affected models. This new quirk
> >>disables acpi-video as before and also stops samsung-laptop from registering
> >
> >
> >I take this to me this quirk is broken_acpi_video && use_native_backlight.
>
> In what it does yes, but I've chosen to make it a standalone quirk named after
> the acpi/video.c use_native_backlight option, which also disables both
> acpi-video
> and vendor backlight interfaces, so for consistency I'm using the same setup
> here.
>
> We cannot use the acpi/video.c option here because that option, which is on by
> default, only triggers on win8 ready BIOS-es and these machines do not have
> such
> a BIOS.
>
> Eventually I would like to see the acpi/video.c code switch away from having
> the 2 semi-orthogonal kernel cmdline options it currently has, which are
> acpi_backlight=[vendor|video] vs video.use_native_backlight=[0|1], while what
> we really have is just three options:
>
> acpi_backlight=[vendor|video|native]
>
> I've explained what I would like to see happen / think needs to happen to
> clean
> up this mess here: https://lkml.org/lkml/2014/12/27/54
>
> So the way I see it the broken_acpi_video quirks means use
> acpi_backlight=vendor
> where as the new use_native_backlight option means acpi_backlight=native, and
> eventually these 2 quirks would end up in just one call in samsung-laptop.c,
> which
> says:
>
> acpi_video_set_backlight_type(acpi_video_backlight_vendor);
>
> resp:
>
> acpi_video_set_backlight_type(acpi_video_backlight_native);
>
> So I really see this as an either or thing, yes native implies disabling
> acpi_video# but that does not mean that it is the same thing as vendor.
Thank you for the detailed response. Queued for 3.20.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dell-laptop: use dedicated sysfs file for ALS
On Wed, Jan 14, 2015 at 03:05:57PM +0100, Gabriele Mazzotta wrote:
> The ambient light sensor doesn't act like an input trigger, so it has
> to be kept separate. The sensor readings are used to determine whether
> the conditions to change the keyboard illumination are satisfied or
> not the moment an input trigger is used. Ambient light changes alone
> can't change the keyboard backlight illumination and don't restart the
> timer.
>
> Signed-off-by: Gabriele Mazzotta
> ---
...
> +static ssize_t kbd_led_als_enabled_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct kbd_state new_state;
> + struct kbd_state state;
> + bool triggers_enabled = false;
> + int enable;
> + int ret;
> +
> + if (!kbd_als_supported) {
> + pr_warn("ALS mode is not supported\n");
> + return -ENODEV;
Will this sysfs file exist if !kbd_als_supported? If so, can we prevent that?
...
Generally speaking, there is a lot more change here than I would like for an
RC5. I'm going to have to consider this one carefully. If we can't come up with
a simpler fix for this RC series, we may have to revert the previous patch and
target this for 3.20.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dell-laptop: use dedicated sysfs file for ALS
On Wed, Jan 14, 2015 at 03:07:21PM +0100, Gabriele Mazzotta wrote: > Hi, > > I decided to remove "als" from input_triggers and created a dedicated > sysfs file for it. Having it there was wrong and misleading. > I also updated the documentation to reflect this change and fixed the > wrong description of als_setting, now used for als_enabled. Given this is a significant functional change, as opposed to a bug fix, I'm leaning toward reverting the original and adding back the corrected version to 3.20. I'm going to look at the total impact first - let me know if you have a strong argument one way or the other. > > Is returning -ENODEV only when writing to als_enabled the right thing > to do or should it be returned also when reading als_enabled? Why would the als_enabled file exist if it would return ENODEV? If it shouldn't, then returning ENODEV in both cases would be the right thing to do as there is an error present. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] toshiba_acpi: Change sci_open function return value
On Wed, Jan 14, 2015 at 11:59:32AM -0700, Azael Avalos wrote:
> Some Toshiba laptops have "poorly implemented" SCI calls on their
> BIOSes and are not checking for sci_{open, close} calls, therefore,
> the sci_open function is failing and making some of the supported
> features unavailable (kbd backlight, touchpad and illumination).
>
> This patch changes the default return code of the sci_open function
> to return one instead of zero, making all those faulty laptops load
> all the supported features.
>
> Signed-off-by: Azael Avalos
> ---
> drivers/platform/x86/toshiba_acpi.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c
> b/drivers/platform/x86/toshiba_acpi.c
> index fc34a71..71ac7c12 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -391,9 +391,10 @@ static int sci_open(struct toshiba_acpi_dev *dev)
> return 1;
> } else if (out[0] == TOS_NOT_PRESENT) {
> pr_info("Toshiba SCI is not present\n");
> + return 0;
> }
>
> - return 0;
> + return 1;
Which means there is really no point in continuing to check for
TOS_OPEN_SLOSE_OK or TOS_ALREADY_OPEN since we're going to return 1 anyway. The
only thing we care about now is TOS_NOT_PRESENT.
I appreciate coding to what it SHOULD be and then handling corner cases
separately, which is basically what this does. However, corner cases need to be
documented.
At the very least, please provide a comment block above return 1 explaining why
we are ignoring what the previous logic indicates should be a failure.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] toshiba_acpi: Add support for USB Sleep and Charge function
On Wed, Jan 14, 2015 at 02:40:18PM -0700, Azael Avalos wrote:
> Newer Toshiba models now come with a feature called Sleep and Charge,
> where the computer USB ports remain powered when the computer is
> asleep or turned off.
>
> This patch adds support to such feature, creating a sysfs entry
> called "usb_sleep_charge" to set the desired charging mode or to
> disable it.
>
> The sysfs entry accepts three parameters, 0x0, 0x9 and 0x21, beign
> disabled, alternate and auto respectively.
>
> Signed-off-by: Azael Avalos
> ---
> drivers/platform/x86/toshiba_acpi.c | 112
>
> 1 file changed, 112 insertions(+)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c
> b/drivers/platform/x86/toshiba_acpi.c
> index 71ac7c12..b03129d 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -122,6 +122,7 @@ MODULE_LICENSE("GPL");
> #define HCI_ECO_MODE 0x0097
> #define HCI_ACCELEROMETER2 0x00a6
> #define SCI_ILLUMINATION 0x014e
> +#define SCI_USB_SLEEP_CHARGE 0x0150
> #define SCI_KBD_ILLUM_STATUS 0x015c
> #define SCI_TOUCHPAD 0x050e
>
> @@ -146,6 +147,10 @@ MODULE_LICENSE("GPL");
> #define SCI_KBD_MODE_ON 0x8
> #define SCI_KBD_MODE_OFF 0x10
> #define SCI_KBD_TIME_MAX 0x3c001a
> +#define SCI_USB_CHARGE_MODE_MASK 0xff
> +#define SCI_USB_CHARGE_DISABLED 0x3
> +#define SCI_USB_CHARGE_ALTERNATE 0x30009
> +#define SCI_USB_CHARGE_AUTO 0x30021
>
> struct toshiba_acpi_dev {
> struct acpi_device *acpi_dev;
> @@ -177,6 +182,7 @@ struct toshiba_acpi_dev {
> unsigned int touchpad_supported:1;
> unsigned int eco_supported:1;
> unsigned int accelerometer_supported:1;
> + unsigned int usb_sleep_charge_supported:1;
> unsigned int sysfs_created:1;
>
> struct mutex mutex;
> @@ -761,6 +767,53 @@ static int toshiba_accelerometer_get(struct
> toshiba_acpi_dev *dev,
> return 0;
> }
>
> +/* Sleep (Charge and Music) utilities support */
> +static int toshiba_usb_sleep_charge_get(struct toshiba_acpi_dev *dev,
> + u32 *mode)
> +{
> + u32 result;
> +
> + if (!sci_open(dev))
> + return -EIO;
> +
> + result = sci_read(dev, SCI_USB_SLEEP_CHARGE, mode);
> + sci_close(dev);
> + if (result == TOS_FAILURE) {
> + pr_err("ACPI call to set USB S&C mode failed\n");
> + return -EIO;
> + } else if (result == TOS_NOT_SUPPORTED) {
> + pr_info("USB Sleep and Charge not supported\n");
> + return -ENODEV;
> + } else if (result == TOS_INPUT_DATA_ERROR) {
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static int toshiba_usb_sleep_charge_set(struct toshiba_acpi_dev *dev,
> + u32 mode)
> +{
> + u32 result;
> +
> + if (!sci_open(dev))
> + return -EIO;
> +
> + result = sci_write(dev, SCI_USB_SLEEP_CHARGE, mode);
> + sci_close(dev);
> + if (result == TOS_FAILURE) {
> + pr_err("ACPI call to set USB S&C mode failed\n");
> + return -EIO;
> + } else if (result == TOS_NOT_SUPPORTED) {
> + pr_info("USB Sleep and Charge not supported\n");
> + return -ENODEV;
> + } else if (result == TOS_INPUT_DATA_ERROR) {
> + return -EIO;
Personally I would feel more comfortable relying on our own data input
validation than that of the AML.
We can also present the user with an abstracted interface, we don't need to have
them send in register values that match the underlying hardware. In fact, should
the firmware in future machines change, you will be faced with having to remap
values should they mean different things. I would suggest defining a user
visible namespace for these, consider:
0: Disabled
1: Auto
2: Alternate (what does this mean anyway?)
Also, per Documentation/sysfs-rules.txt, which I believe I added based on
previous review with you?, -EIO is typically returned by sysfs itself from some
sort of general failure, like a NULL read or store pointer.
When the read or store operation fails as above, -ENXIO is the preferred error
code.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] toshiba_acpi: Add support for USB Sleep functions under battery
_t count);
>
> static DEVICE_ATTR(kbd_backlight_mode, S_IRUGO | S_IWUSR,
> toshiba_kbd_bl_mode_show, toshiba_kbd_bl_mode_store);
> @@ -1387,6 +1453,9 @@ static DEVICE_ATTR(position, S_IRUGO,
> toshiba_position_show, NULL);
> static DEVICE_ATTR(usb_sleep_charge, S_IRUGO | S_IWUSR,
> toshiba_usb_sleep_charge_show,
> toshiba_usb_sleep_charge_store);
> +static DEVICE_ATTR(sleep_functions_on_battery, S_IRUGO | S_IWUSR,
> +sleep_functions_on_battery_show,
> +sleep_functions_on_battery_store);
>
> static struct attribute *toshiba_attributes[] = {
> &dev_attr_kbd_backlight_mode.attr,
> @@ -1396,6 +1465,7 @@ static struct attribute *toshiba_attributes[] = {
> &dev_attr_touchpad.attr,
> &dev_attr_position.attr,
> &dev_attr_usb_sleep_charge.attr,
> + &dev_attr_sleep_functions_on_battery.attr,
> NULL,
> };
>
> @@ -1657,6 +1727,67 @@ static ssize_t toshiba_usb_sleep_charge_store(struct
> device *dev,
> return count;
> }
>
> +static ssize_t sleep_functions_on_battery_show(struct device *dev,
> +struct device_attribute *attr,
> +char *buf)
> +{
> + struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> + u32 state;
> + int ret;
> + int tmp;
> + int status;
> + int bat_lvl;
Order declarations in decreasing line length please.
int bat_lvl;
int status;
int state;
int tmp;
int ret;
> +
> + ret = toshiba_sleep_functions_status_get(toshiba, &state);
> + if (ret < 0)
> + return ret;
> +
> + /* Determine the status: 0x4 - Enabled | 0x1 - Disabled */
> + tmp = state & SCI_USB_CHARGE_BAT_MASK;
> + status = (tmp == 0x4) ? 1 : 0;
> + /* Determine the battery level set */
> + bat_lvl = state >> HCI_MISC_SHIFT;
> +
> + return sprintf(buf, "%d %d\n", status, bat_lvl);
> +}
> +
> +static ssize_t sleep_functions_on_battery_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> + u32 status;
> + int value;
> + int ret;
> + int tmp;
> +
Yeah, like that ;-)
> + ret = kstrtoint(buf, 0, &value);
> + if (ret)
> + return ret;
> +
> + /* Set the status of the function:
> + * 0 - Disabled
> + * 1-100 - Enabled
> + */
> + if (value < 0 || value > 100)
> + return -EINVAL;
> +
Ah, oops, you catch the input here. Good.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] toshiba_acpi: Add support for USB Sleep and Charge function
On Wed, Jan 14, 2015 at 02:40:18PM -0700, Azael Avalos wrote:
> Newer Toshiba models now come with a feature called Sleep and Charge,
> where the computer USB ports remain powered when the computer is
> asleep or turned off.
>
> This patch adds support to such feature, creating a sysfs entry
> called "usb_sleep_charge" to set the desired charging mode or to
> disable it.
>
> The sysfs entry accepts three parameters, 0x0, 0x9 and 0x21, beign
> disabled, alternate and auto respectively.
>
> Signed-off-by: Azael Avalos
> ---
> drivers/platform/x86/toshiba_acpi.c | 112
>
> 1 file changed, 112 insertions(+)
...
> +static ssize_t toshiba_usb_sleep_charge_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> + int state;
> + int ret;
> +
> + ret = kstrtoint(buf, 0, &state);
> + if (ret)
> + return ret;
> +
> + /* Set the USB charging mode where:
> + * 0x3 - Disable
> + * 0x30009 - Alternate
> + * 0x30021 - Auto
> + */
> + state |= 0x3;
> + if (state != SCI_USB_CHARGE_DISABLED && state != SCI_USB_CHARGE_AUTO &&
> + state != SCI_USB_CHARGE_ALTERNATE)
> + return -EINVAL;
Sorry, I missed this as the input validation on my first pass. Looks good.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] toshiba_acpi: Add support for USB Rapid Charge
On Wed, Jan 14, 2015 at 02:40:20PM -0700, Azael Avalos wrote:
> Newer Toshiba laptops equipped with USB 3.0 ports now have the
> functionality of rapid charging devices connected to their USB hubs.
>
> This patch adds support to use such feature by creating a sysfs entry
> named "usb_rapid_charge", accepting only two values, 0 to disable and
> one to enable, however, the machine needs a restart everytime the
s/one/1//
> function is toggled.
>
> Signed-off-by: Azael Avalos
> ---
> drivers/platform/x86/toshiba_acpi.c | 107
>
> 1 file changed, 107 insertions(+)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c
> b/drivers/platform/x86/toshiba_acpi.c
> index 9e054c5..eba5f9e 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -155,6 +155,7 @@ MODULE_LICENSE("GPL");
> #define SCI_USB_CHARGE_BAT_LVL_OFF 0x1
> #define SCI_USB_CHARGE_BAT_LVL_ON0x4
> #define SCI_USB_CHARGE_BAT_LVL 0x0200
> +#define SCI_USB_CHARGE_RAPID_DSP 0x0300
>
> struct toshiba_acpi_dev {
> struct acpi_device *acpi_dev;
> @@ -188,6 +189,7 @@ struct toshiba_acpi_dev {
> unsigned int eco_supported:1;
> unsigned int accelerometer_supported:1;
> unsigned int usb_sleep_charge_supported:1;
> + unsigned int usb_rapid_charge_supported:1;
> unsigned int sysfs_created:1;
>
> struct mutex mutex;
> @@ -874,6 +876,60 @@ static int toshiba_sleep_functions_status_set(struct
> toshiba_acpi_dev *dev,
> return 0;
> }
>
> +static int toshiba_usb_rapid_charge_get(struct toshiba_acpi_dev *dev,
> + u32 *state)
> +{
> + u32 in[TCI_WORDS] = { SCI_GET, SCI_USB_SLEEP_CHARGE, 0, 0, 0, 0 };
> + u32 out[TCI_WORDS];
> + acpi_status status;
> +
> + if (!sci_open(dev))
> + return -EIO;
> +
> + in[5] = SCI_USB_CHARGE_RAPID_DSP;
> + status = tci_raw(dev, in, out);
> + sci_close(dev);
> + if (ACPI_FAILURE(status) || out[0] == TOS_FAILURE) {
> + pr_err("ACPI call to get USB S&C battery level failed\n");
> + return -EIO;
> + } else if (out[0] == TOS_NOT_SUPPORTED ||
> +out[0] == TOS_INPUT_DATA_ERROR) {
> + pr_info("USB Sleep and Charge not supported\n");
> + return -ENODEV;
> + }
> +
Same comment on return codes.
Although, on looking through the existing driver, I only see EIO, and internally
consistency is important we can stick with EIO for this driver.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] toshiba_acpi: Add support for USB Sleep functions
On Wed, Jan 14, 2015 at 02:40:17PM -0700, Azael Avalos wrote: > The following patches add support to several USB Sleep functions > found on newer Toshiba laptops, allowing to use th USB ports while > the laptop is asleep or turned off. > > Azael Avalos (4): > toshiba_acpi: Add support for USB Sleep and Charge function > toshiba_acpi: Add support for USB Sleep functions under battery > toshiba_acpi: Add support for USB Rapid Charge > toshiba_acpi: Add support for USB Sleep and Music > > drivers/platform/x86/toshiba_acpi.c | 449 > > 1 file changed, 449 insertions(+) Hi Azael, Ignore my comments on input validation and error codes, you cover the former, and the latter keeps the driver internally consistent. That only leaves the Disable/Auto/Alternate values and a couple trivial formatting and typos to address. Thanks, -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] asus-laptop: is_visible should not return a bool
On Fri, Jan 16, 2015 at 09:29:19AM -0500, Vivien Didelot wrote: > With a Lucid platform, asus_sysfs_is_visible() returns a boolean for > ls_switch and ls_level attributes. Fix that and also s/supported/ok/ and > s/asus->handle/handle/ to avoid lines wider than 80 chars. This is two patches, a cleanup and functional change. Since the cleanup has nothing to do with the cleanup, please split these out. "ok" is an atypical variable name. It looks as though "ret" would server just as well and is far more consistent with typical usage. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fujitsu-laptop: use FB_BLANK_* constants
On Sun, Jan 18, 2015 at 01:02:11AM +0100, Michael Karcher wrote: > From: Michael Karcher > No empty commit messages please. There is always something to say if it's worth adding the patch to the kernel. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] fujitsu-laptop: use FB_BLANK_* constants
On Sun, Jan 18, 2015 at 08:28:46PM +0100, Michael Karcher wrote: > Replace the magic numbers in fujitsu-laptop.c by the appropriate FB_BLANK > constants, as indicated by the comment for backlight_properties.power in > include/linux/backlight.h. > > Signed-off-by: Michael Karcher Thanks Michael, queued for 3.20. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/4] toshiba_acpi: Add support for USB Sleep functions
On Sun, Jan 18, 2015 at 06:30:21PM -0700, Azael Avalos wrote: > The following patches add support to several USB Sleep functions > found on newer Toshiba laptops, allowing to use the USB ports while > the laptop is asleep or turned off. > > Changes since v1: > - Changed accepted parameters on first patch and added a short > description of what the auto and alternate charging modes mean > - Some misc format and typo changes > > Azael Avalos (4): > toshiba_acpi: Add support for USB Sleep and Charge function > toshiba_acpi: Add support for USB Sleep functions under battery > toshiba_acpi: Add support for USB Rapid Charge > toshiba_acpi: Add support for USB Sleep and Music > > drivers/platform/x86/toshiba_acpi.c | 455 > > 1 file changed, 455 insertions(+) Queued for 3.20, thanks Azael. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] asus-laptop: cleanup is_visible
On Tue, Jan 20, 2015 at 01:20:36PM -0500, Vivien Didelot wrote: > Hi Corentin, > > > > Use the attribute indexes and concise the if statements. > > > > > Why ? I really don't see that as an improvement. > > The improvement is code clarity and maintainability. I'm not use we want > to keep multiple returns and this goto thing. I think per-attribute > if-statements are clearer. I have to concur with Corentin, changing to numerical indices rather than the named atrributes makes the code harder to read in my opinion, and is more likely to lead to mistakes than not. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] asus-laptop: fix is_visible return value
On Sun, Jan 18, 2015 at 06:25:24PM -0500, Vivien Didelot wrote: > With a Lucid platform, asus_sysfs_is_visible() returned a boolean for > ls_switch and ls_level attributes. > > Signed-off-by: Vivien Didelot Queued, thanks. > --- > drivers/platform/x86/asus-laptop.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/asus-laptop.c > b/drivers/platform/x86/asus-laptop.c > index f71700e..00f5e82 100644 > --- a/drivers/platform/x86/asus-laptop.c > +++ b/drivers/platform/x86/asus-laptop.c > @@ -1616,7 +1616,7 @@ static umode_t asus_sysfs_is_visible(struct kobject > *kobj, > else > goto normal; > > - return supported; > + return supported ? attr->mode : 0; > } > > normal: > -- > 2.2.2 > > -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] asus-laptop: use DEVICE_ATTR_* macros
On Sun, Jan 18, 2015 at 06:25:25PM -0500, Vivien Didelot wrote:
> Use DEVICE_ATTR_{RO,WO,RW} macros to simplify attributes declarations.
It does a lot more than that, including a lot of seemingly superfluous
reformatting of function declarations and renaming.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/4] toshiba_acpi: Add support for USB Sleep functions
On Wed, Jan 21, 2015 at 11:21:06AM -0700, Azael Avalos wrote: > Hi there, > > Sorry for the late reply. > > 2015-01-20 4:15 GMT-07:00 Oliver Neukum : > > On Sun, 2015-01-18 at 18:30 -0700, Azael Avalos wrote: > >> The following patches add support to several USB Sleep functions > >> found on newer Toshiba laptops, allowing to use the USB ports while > >> the laptop is asleep or turned off. > > > > Hi, > > > > this is most interesting. But the interface is terrible. If > > Well, I'm just providing the on/off switch on Toshiba specific hardware, > but I'm unaware/unfamiliar with how other vendors implement this > feature. > > > possible we would like to have a generic interface for this, > > which should not depend on the specific platform in use and would > > have to be per bus or even per port (for example the ports on a > > Thunderbolt dock would not work with your module). > > Sounds good, as not all ports carry such functionality, on my > current laptop only two ports provide this, the other two are just > normal USB 2/3 ports (configurable ;-) ). > > > Do you think we could provide a hook from generic code into > > platform code to detect the capability for power control? > > If there's an existing API already, I'll be glad to modify the code to > use it, if it doesn't exist yet, I'll simply adapt the code to the chosen > standard whenever becomes available. Given the ACPI calls which are platform specific, a general purpose driver doesn't seem likely. I'm keeping this queued as is unless you ask me to drop it Azael. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dell-laptop: use dedicated sysfs file for ALS
On Sun, Jan 18, 2015 at 07:34:17PM +0100, Gabriele Mazzotta wrote: > On Sunday 18 January 2015 10:08:21 Darren Hart wrote: > > On Wed, Jan 14, 2015 at 03:07:21PM +0100, Gabriele Mazzotta wrote: > > > Hi, > > > > > > I decided to remove "als" from input_triggers and created a dedicated > > > sysfs file for it. Having it there was wrong and misleading. > > > I also updated the documentation to reflect this change and fixed the > > > wrong description of als_setting, now used for als_enabled. > > > > Given this is a significant functional change, as opposed to a bug fix, I'm > > leaning toward reverting the original and adding back the corrected version > > to > > 3.20. I'm going to look at the total impact first - let me know if you have > > a > > strong argument one way or the other. > > I'm not against this decision. > If you do, please remember to also revert the commit that added the > documentation. Then for my pull request to Linus this week, I'll be reverting: 02b2aaa platform: x86: dell-laptop: Add support for keyboard backlight 3161293 Documentation: Add entry for dell-laptop sysfs interface Am I missing anything? -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] asus-laptop: use DEVICE_ATTR_* macros
On Wed, Jan 21, 2015 at 03:19:17PM -0500, Vivien Didelot wrote:
> Hi Darren,
>
> > > Use DEVICE_ATTR_{RO,WO,RW} macros to simplify attributes
> > > declarations.
> >
> > It does a lot more than that, including a lot of seemingly
> > superfluous reformatting of function declarations and renaming.
>
> What do you mean? DEVICE_ATTR_RW(foo) requires foo_show() and foo_store()
> functions, not show_foo() and store_foo().
Ah yes, of course. Perhaps obvious in hindsight, but a bit more explanation in
the commit message would have eliminated the confusion.
Always provide enough information in your commit log to explain to someone else
that hasn't been looking at the code as recently as you have to understand the
problem, the solution, and which provides sufficient explanation for all changes
included in the patch.
Please resubmit with a more complete commit message.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 1/3] intel_scu_ipc: fix indentation in few places
On Wed, Jan 21, 2015 at 09:38:09PM +0200, Andy Shevchenko wrote: > While here, do couple of amendments: > - move platform variable to the function where it's used > - define intel_scu_ipc_check_status() static > > Signed-off-by: Andy Shevchenko Series applied and queued for 3.20. Minor wording change to 3/3 commit subject and log. Running through testing, will push to for-next tomorrow. Thank you Andy. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] samsung-laptop: enable better lid handling
On Thu, Dec 11, 2014 at 09:18:35PM +, Julijonas Kikutis wrote:
Hi Julijonas,
Please Cc the maintainer listed in the MAINTERS file for a faster response.
> Some Samsung laptops with SABI3 delay the sleep for 10 seconds after
> the lid is closed and do not wake up from sleep after the lid is opened.
> A SABI command is needed to enable the better behavior.
>
> Command = 0x6e, d0 = 0x81 enables this behavior. Returns d0 = 0x01.
> Command = 0x6e, d0 = 0x80 disables this behavior. Returns d0 = 0x00.
>
> Command = 0x6d and any d0 queries the state. This returns:
> d0 = 0x0*01, d1 = 0x00, d2 = 0x00, d3 = 0x0* when it is enabled.
> d0 = 0x0*00, d1 = 0x00, d2 = 0x00, d3 = 0x0* when it is disabled.
> Where * is 0 - laptop has never slept or hibernated after switch on,
>1 - laptop has hibernated just before,
>2 - laptop has slept just before.
>
> Patch addresses bug https://bugzilla.kernel.org/show_bug.cgi?id=75901.
> It adds a sysfs attribute lid_handling with description and also an
> addition to the quirks structure to enable the mode by default.
>
> However, a user with another laptop in the bug report says that "power
> button has to be pressed twice to wake the machine" in this mode.
This is with this patch applied?
> Therefore, it is enabled by default only for the single laptop that I
> have tested.
>
> This mode is also needed in UEFI, but there samsung-laptop is
> unfortunately disabled.
I don't follow. What are you saying here?
This looks pretty much ready. The only comments I have are argued against by
staying consistent with the existing driver. So just the above questions and one
comment below. Please clarify, update, and resend. Be sure to add me to Cc this
time :-)
>
> Signed-off-by: Julijonas Kikutis
> ---
...
> @@ -,7 +1208,7 @@ static int __init samsung_backlight_init(struct
> samsung_laptop *samsung)
> }
>
> static umode_t samsung_sysfs_is_visible(struct kobject *kobj,
> -struct attribute *attr, int idx)
> + struct attribute *attr, int idx)
> {
> struct device *dev = container_of(kobj, struct device, kobj);
> struct platform_device *pdev = to_platform_device(dev);
> @@ -1124,6 +1221,8 @@ static umode_t samsung_sysfs_is_visible(struct kobject
> *kobj,
> ok = !!(read_battery_life_extender(samsung) >= 0);
> if (attr == &dev_attr_usb_charge.attr)
> ok = !!(read_usb_charge(samsung) >= 0);
> + if (attr == &dev_attr_lid_handling.attr)
> + ok = !!(read_lid_handling(samsung) >= 0);
>
> return ok ? attr->mode : 0;
> }
> @@ -1436,6 +1535,10 @@ static int samsung_pm_notification(struct
> notifier_block *nb,
> samsung->quirks->enable_kbd_backlight)
> kbd_backlight_enable(samsung);
>
> + if (val == PM_POST_HIBERNATION &&
> + samsung->quirks->lid_handling)
This can be one line.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ideapad-laptop: Use intel_backlight only on Lenovo B470e
On Mon, Jan 19, 2015 at 11:14:44AM +0800, AceLan Kao wrote: > Hi all, > > I'm wondering are there any concerns that prevent this patch from > entering the upstream? I don't see it anywhere in my platform-driver-x86 archives. Please resend the patch with a reference to the original submission being sure to Cc the List and Maintainer as listed in the MAINTAINERS file. Thanks, -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] asus-laptop: cleanup is_visible
On Fri, Jan 23, 2015 at 09:01:11AM -0500, Vivien Didelot wrote:
> Get rid of the "normal" label, use one if-statement per attribute for
> maintainability and change s/supported/ret/ and s/asus->handle/handle/
> to fix a coding style issue (lines with 80+ chars).
>
> Signed-off-by: Vivien Didelot
> ---
> drivers/platform/x86/asus-laptop.c | 61
> ++
> 1 file changed, 22 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-laptop.c
> b/drivers/platform/x86/asus-laptop.c
> index 46b2746..37a3a1f 100644
> --- a/drivers/platform/x86/asus-laptop.c
> +++ b/drivers/platform/x86/asus-laptop.c
> @@ -1602,55 +1602,38 @@ static umode_t asus_sysfs_is_visible(struct kobject
> *kobj,
> struct platform_device *pdev = to_platform_device(dev);
> struct asus_laptop *asus = platform_get_drvdata(pdev);
> acpi_handle handle = asus->handle;
> - bool supported;
> + bool ret = true;
Very odd to have ret be a bool...
>
> - if (asus->is_pega_lucid) {
> - /* no ls_level interface on the Lucid */
> - if (attr == &dev_attr_ls_switch.attr)
> - supported = true;
> - else if (attr == &dev_attr_ls_level.attr)
> - supported = false;
> - else
> - goto normal;
> -
> - return supported ? attr->mode : 0;
> - }
> -
> -normal:
> if (attr == &dev_attr_wlan.attr) {
> - supported = !acpi_check_handle(handle, METHOD_WLAN, NULL);
> -
> + ret = !acpi_check_handle(handle, METHOD_WLAN, NULL);
acpi_check_handle returns an int, you should be able to just use that.
...
> } else if (attr == &dev_attr_ls_value.attr) {
> - supported = asus->is_pega_lucid;
> + ret = asus->is_pega_lucid;
This should promote to an int without a problem.
...
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] asus-laptop: use DEVICE_ATTR_xx macros
On Fri, Jan 23, 2015 at 09:01:10AM -0500, Vivien Didelot wrote:
> Use DEVICE_ATTR_{RO,WO,RW} macros to simplify sysfs attributes
> declaration.
>
> To declare a "foo" attribute, DEVICE_ATTR_RW() requires foo_show() and
> foo_store(), so rename a few functions to satisfy this requirement.
>
> Also put the macro below each related show/store functions for clarity.
>
> Signed-off-by: Vivien Didelot
Thanks for the update. Queued for 3.20.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] samsung-laptop: enable better lid handling
On Tue, Jan 27, 2015 at 01:26:06PM +, Julijonas Kikutis wrote: > Some Samsung laptops with SABI3 delay the sleep for 10 seconds after > the lid is closed and do not wake up from sleep after the lid is opened. > A SABI command is needed to enable the better behavior. > > Command = 0x6e, d0 = 0x81 enables this behavior. Returns d0 = 0x01. > Command = 0x6e, d0 = 0x80 disables this behavior. Returns d0 = 0x00. > > Command = 0x6d and any d0 queries the state. This returns: > d0 = 0x0*01, d1 = 0x00, d2 = 0x00, d3 = 0x0* when it is enabled. > d0 = 0x0*00, d1 = 0x00, d2 = 0x00, d3 = 0x0* when it is disabled. > Where * is 0 - laptop has never slept or hibernated after switch on, >1 - laptop has hibernated just before, >2 - laptop has slept just before. > > Patch addresses bug https://bugzilla.kernel.org/show_bug.cgi?id=75901 . > It adds a sysfs attribute lid_handling with a description and also an > addition to the quirks structure to enable the mode by default. > > A user with another laptop in the bug report says that "power button has > to be pressed twice to wake the machine" when he or she enabled the mode > manually using the SABI command. Therefore, it is enabled by default > only for the single laptop that I have tested. > > Signed-off-by: Julijonas Kikutis > --- > .../ABI/testing/sysfs-driver-samsung-laptop| 8 ++ > drivers/platform/x86/samsung-laptop.c | 120 > - > 2 files changed, 127 insertions(+), 1 deletion(-) > Patch is generally fine, thanks for addressing my comments. Prior to merging I always run checkpatch.pl just in case I missed anything obvious: $ scripts/checkpatch.pl ~/samsung/01-lid-handling.patch WARNING: Prefer kstrto to single variable sscanf #219: FILE: drivers/platform/x86/samsung-laptop.c:900: + if (!count || sscanf(buf, "%i", &value) != 1) + return -EINVAL; total: 0 errors, 1 warnings, 219 lines checked Please always run checkpatch.pl. It isn't sufficient and doesn't catch everything, but it is a minimum bar kind of thing. Thanks, -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] toshiba_acpi: Add a check for TOS_NOT_SUPPORTED in the sci_open function
On Sun, Jan 18, 2015 at 07:17:12PM -0700, Azael Avalos wrote:
> This was "toshiba_acpi: Change sci_open function return value"
>
> Some Toshiba laptops have "poorly implemented" SCI calls on their
> BIOSes and are not checking for sci_{open, close} calls, therefore,
> the sci_open function is failing and making some of the supported
> features unavailable (kbd backlight, touchpad, illumination, etc.).
>
> This patch checks wheter we receive TOS_NOT_SUPPORTED and returns
^ whether
(checkpatch.pl would catch this)
Since I'm late in review, this one's on me ;-)
> 1, making the supported features work on such laptops.
>
> In the case that some laptops really do not support the SCI, all the
> SCI dependent functions check for TOS_NOT_SUPPORTED, and thus, not
> registering support for the queried feature.
>
> Signed-off-by: Azael Avalos
Queued for 3.20.
> ---
> Darren,
>
> Hopefully this new patch eases some of the concerns of the previous
> patch, and this time I added a comment block explaining a bit, but of
> course, let me know if there is something else that needs change.
>
> Cheers
> Azael
>
> drivers/platform/x86/toshiba_acpi.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c
> b/drivers/platform/x86/toshiba_acpi.c
> index fc34a71..899ead6b 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -389,6 +389,19 @@ static int sci_open(struct toshiba_acpi_dev *dev)
> } else if (out[0] == TOS_ALREADY_OPEN) {
> pr_info("Toshiba SCI already opened\n");
> return 1;
> + } else if (out[0] == TOS_NOT_SUPPORTED) {
> + /* Some BIOSes do not have the SCI open/close functions
> + * implemented and return 0x8000 (Not Supported), failing to
> + * register some supported features.
> + *
> + * Simply return 1 if we hit those affected laptops to make the
> + * supported features work.
> + *
> + * In the case that some laptops really do not support the SCI,
> + * all the SCI dependent functions check for TOS_NOT_SUPPORTED,
> + * and thus, not registering support for the queried feature.
> + */
This is great by the way, when things are just weird/broken, this is when we
need really clear comments.
> + return 1;
> } else if (out[0] == TOS_NOT_PRESENT) {
> pr_info("Toshiba SCI is not present\n");
> }
Thanks,
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Darren traveling and out sick
All, My apologies for a period of unresponsiveness. I have been traveling and am now recovering from a cold and I let things back up more than I intended. I will be working through the backlog between now and Monday. If you have something pending which I haven't responded to by Monday, please resend. Thanks, -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 02/20] power_supply: Move run-time configuration to separate structure
On Fri, Jan 30, 2015 at 03:47:40PM +0100, Krzysztof Kozlowski wrote: > Add new structure 'power_supply_config' for holding run-time > initialization data like of_node, supplies and private driver data. > > The power_supply_register() function is changed so all power supply > drivers need updating. > > When registering the power supply this new 'power_supply_config' should be > used instead of directly initializing 'struct power_supply'. This allows > changing the ownership of power_supply structure from driver to the > power supply core in next patches. > > When a driver does not use of_node or supplies then it should use NULL > as config. If driver uses of_node or supplies then it should allocate > config on stack and initialize it with proper values. > > Signed-off-by: Krzysztof Kozlowski > Reviewed-by: Bartlomiej Zolnierkiewicz For drivers/platform/x86/compal-laptop.c Reviewed-by: Darren Hart -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] samsung-laptop: enable better lid handling
On Thu, Jan 29, 2015 at 01:04:37PM +, Julijonas Kikutis wrote: > Some Samsung laptops with SABI3 delay the sleep for 10 seconds after > the lid is closed and do not wake up from sleep after the lid is opened. > A SABI command is needed to enable the better behavior. > > Command = 0x6e, d0 = 0x81 enables this behavior. Returns d0 = 0x01. > Command = 0x6e, d0 = 0x80 disables this behavior. Returns d0 = 0x00. > > Command = 0x6d and any d0 queries the state. This returns: > d0 = 0x0*01, d1 = 0x00, d2 = 0x00, d3 = 0x0* when it is enabled. > d0 = 0x0*00, d1 = 0x00, d2 = 0x00, d3 = 0x0* when it is disabled. > Where * is 0 - laptop has never slept or hibernated after switch on, >1 - laptop has hibernated just before, >2 - laptop has slept just before. > > Patch addresses bug https://bugzilla.kernel.org/show_bug.cgi?id=75901 . > It adds a sysfs attribute lid_handling with a description and also an > addition to the quirks structure to enable the mode by default. > > A user with another laptop in the bug report says that "power button has > to be pressed twice to wake the machine" when he or she enabled the mode > manually using the SABI command. Therefore, it is enabled by default > only for the single laptop that I have tested. > > Signed-off-by: Julijonas Kikutis Queued up, thanks Jujijonas. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFT PATCH 2/4] compal-laptop: Check return value of power_supply_register
On Tue, Jan 27, 2015 at 12:30:19PM +0100, Krzysztof Kozlowski wrote:
> The return value of power_supply_register() call was not checked and
> even on error probe() function returned 0. If registering failed then
> during unbind the driver tried to unregister power supply which was not
> actually registered.
>
> This could lead to memory corruption because power_supply_unregister()
> unconditionally cleans up given power supply.
>
> Fix this by checking return status of power_supply_register() call. In
> case of failure, unregister the hwmon device and fail the probe. Add a
> fixme note about missing hwmon_device_unregister() in driver removal.
>
> Signed-off-by: Krzysztof Kozlowski
> Fixes: 9be0fcb5ed46 ("compal-laptop: add JHL90, battery & hwmon interface")
> Cc:
> ---
> drivers/platform/x86/compal-laptop.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/compal-laptop.c
> b/drivers/platform/x86/compal-laptop.c
> index 15c0fab2bfa1..cf55a9246f12 100644
> --- a/drivers/platform/x86/compal-laptop.c
> +++ b/drivers/platform/x86/compal-laptop.c
> @@ -1036,12 +1036,16 @@ static int compal_probe(struct platform_device *pdev)
>
> /* Power supply */
> initialize_power_supply_data(data);
> - power_supply_register(&compal_device->dev, &data->psy);
> + err = power_supply_register(&compal_device->dev, &data->psy);
> + if (err < 0)
> + goto psy_err;
>
> platform_set_drvdata(pdev, data);
>
> return 0;
>
> +psy_err:
> + hwmon_device_unregister(hwmon_dev);
> remove:
> sysfs_remove_group(&pdev->dev.kobj, &compal_platform_attr_group);
> return err;
> @@ -1072,6 +1076,7 @@ static int compal_remove(struct platform_device *pdev)
>
> data = platform_get_drvdata(pdev);
> power_supply_unregister(&data->psy);
> + /* FIXME: missing hwmon_device_unregister() */
Is this FIXME a leftover? Is there a reason we can't fix this now instead of
adding a FIXME?
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] samsung-laptop: fix sparse warning
On Thu, Feb 05, 2015 at 02:40:05PM +, Lad Prabhakar wrote: > From: "Lad, Prabhakar" > > this patch fixes following sparse warning: > > samsung-laptop.c:1365:52: warning: Using plain integer as NULL pointer > > Signed-off-by: Lad, Prabhakar Queued, thanks. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] x86: thinkpad_acpi.c: fix sparse warning
On Thu, Feb 05, 2015 at 02:45:38PM +, Lad Prabhakar wrote: > From: "Lad, Prabhakar" > > this patch fixes following sparse warning: Please use the imperative, command form, in the future: "Fix the following sparse warning:" For the subject, just the module name is sufficient, no need to preface with x86:. Always capitalize the first word after the filename: thinkpad_acpi: Fix sparse warning Corrected and applied, thanks! -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
