Skip to contentSkip to conversation

Toshiba Bluetooth enabler (rfkill)

PATCH Applied Jes Sorensen 5 participants 17 messages Dec 4, 2009 – Dec 16, 2009

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Toshiba Bluetooth enabler (rfkill)

Hi,

This patch adds a new driver to catch RFKill events to the Bluetooth device on modern Toshiba laptops. Without it, the Bluetooth device is simply lost on my Portege R500 when the RFKill switch is flipped, and doesn't come back until after a reboot.

It binds to the TOS6205 ACPI device and doesn't touch any existing code.

Cheers,
Jes

Toshiba Bluetooth Enabling driver (RFKill handler)

This patch adds support for the ACPI events generated by the RFKill switch on modern Toshiba laptops, and re-enables the Bluetooth USB device when the switch is flipped back to the 'on' position.

The RFKill switch brute force pulls out the USB device when flipped to 'off', but it doesn't automatically re-enable it. Without this driver, the Bluetooth is gone until after a reboot on my Portege R500.

---

drivers/platform/x86/toshiba_bluetooth.c new file +135 -0
Index: linux-2.6/drivers/platform/x86/toshiba_bluetooth.c
===================================================================
@@ -0,0 +1,135 @@
+/*
+ * Toshiba Bluetooth Enable Driver
+ *
+ * Copyright (C) 2009 Jes Sorensen <[email protected]>
+ *
+ * Thanks to Matthew Garrett for background info on ACPI innards which
+ * normal people aren't meant to understand :-)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Note the Toshiba Bluetooth RFKill switch seems to be a strange
+ * fish. It only provides a BT event when the switch is flipped to
+ * the 'on' position. When flipping it to 'off', the USB device is
+ * simply pulled away underneath us, without any BT event being
+ * delivered.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+
+MODULE_AUTHOR("Jes Sorensen <[email protected]>");
+MODULE_DESCRIPTION("Toshiba Laptop ACPI Bluetooth Enable Driver");
+MODULE_LICENSE("GPL");
+
+
+static int toshiba_bt_rfkill_add(struct acpi_device *device);
+static int toshiba_bt_rfkill_remove(struct acpi_device *device, int type);
+static void toshiba_bt_rfkill_notify(struct acpi_device *device, u32 event);
+static int toshiba_bt_resume(struct acpi_device *device);
+
+static const struct acpi_device_id bt_device_ids[] = {
+	{ "TOS6205", 0},
+	{ "", 0},
+};
+MODULE_DEVICE_TABLE(acpi, bt_device_ids);
+
+static struct acpi_driver toshiba_bt_rfkill_driver = {
+	.name =		"Toshiba BT",
+	.class =	"Toshiba",
+	.ids =		bt_device_ids,
+	.ops =		{
+				.add =		toshiba_bt_rfkill_add,
+				.remove =	toshiba_bt_rfkill_remove,
+				.notify =	toshiba_bt_rfkill_notify,
+				.resume =	toshiba_bt_resume,
+			},
+};
+
+
+static int toshiba_bluetooth_enable(acpi_handle handle)
+{
+	acpi_status res1, res2;
+	acpi_integer result;
+
+	/* Query ACPI to verify RFKill switch is set to on */
+	res1 = acpi_evaluate_integer(handle, "BTST", NULL, &result);
+	if (ACPI_FAILURE(res1) || !(result & 0x01))
+		return -EBUSY;
+
+	printk(KERN_INFO "toshiba_bluetooth: Re-enabling Toshiba Bluetooth\n");
+	res1 = acpi_evaluate_object(handle, "AUSB", NULL, NULL);
+	res2 = acpi_evaluate_object(handle, "BTPO", NULL, NULL);
+	if (!ACPI_FAILURE(res1) || !ACPI_FAILURE(res2)) {
+		return 0;
+	}
+	printk(KERN_WARNING "toshiba_bluetooth: Failed to re-enable "
+	       "Toshiba Bluetooth\n");
+
+	return -ENODEV;
+}
+
+static void toshiba_bt_rfkill_notify(struct acpi_device *device, u32 event)
+{
+	toshiba_bluetooth_enable(device->handle);
+}
+
+static int toshiba_bt_resume(struct acpi_device *device)
+{
+	return toshiba_bluetooth_enable(device->handle);
+}
+
+static int toshiba_bt_rfkill_add(struct acpi_device *device)
+{
+	acpi_status status;
+	acpi_integer bt_present;
+	int result = -ENODEV;
+
+	status = acpi_evaluate_integer(device->handle, "_STA", NULL,
+				       &bt_present);
+
+	if (!ACPI_FAILURE(status) && bt_present) {
+		printk(KERN_INFO "Detected Toshiba ACPI Bluetooth device - "
+		      "installing RFKill handler\n");
+		result = toshiba_bluetooth_enable(device->handle);
+	}
+
+	return result;
+}
+
+static int __init toshiba_bt_rfkill_init(void)
+{
+	int result = 0;
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	result = acpi_bus_register_driver(&toshiba_bt_rfkill_driver);
+	if (result < 0) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+				  "Error registering driver\n"));
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int toshiba_bt_rfkill_remove(struct acpi_device *device, int type)
+{
+	/* clean up */
+	return 0;
+}
+
+static void __exit toshiba_bt_rfkill_exit(void)
+{
+	acpi_bus_unregister_driver(&toshiba_bt_rfkill_driver);
+}
+
+module_init(toshiba_bt_rfkill_init);
+module_exit(toshiba_bt_rfkill_exit);
drivers/platform/x86/Makefile +1 -0
Index: linux-2.6/drivers/platform/x86/Makefile
===================================================================
@@ -21,3 +21,4 @@ obj-$(CONFIG_ACPI_WMI)		+= wmi.o
 obj-$(CONFIG_ACPI_ASUS)		+= asus_acpi.o
 obj-$(CONFIG_TOPSTAR_LAPTOP)	+= topstar-laptop.o
 obj-$(CONFIG_ACPI_TOSHIBA)	+= toshiba_acpi.o
+obj-$(CONFIG_TOSHIBA_BT_RFKILL)	+= toshiba_bluetooth.o
drivers/platform/x86/Kconfig +17 -0
Index: linux-2.6/drivers/platform/x86/Kconfig
===================================================================
@@ -435,4 +435,21 @@ config ACPI_TOSHIBA
 
 	  If you have a legacy free Toshiba laptop (such as the Libretto L1
 	  series), say Y.
+
+config TOSHIBA_BT_RFKILL
+	tristate "Toshiba Bluetooth RFKill switch support"
+	depends on ACPI
+	depends on RFKILL
+	depends on BT
+	---help---
+	  This driver adds support for Bluetooth events for the RFKill
+	  switch on modern Toshiba laptops with full ACPI support and
+	  an RFKill switch.
+
+	  This driver handles RFKill events for the TOS6205 Bluetooth,
+	  and re-enables it when the switch is set back to the 'on'
+	  position.
+
+	  If you have a modern Toshiba laptop with a Bluetooth and an
+	  RFKill switch (such as the Portege R500), say Y.
 endif # X86_PLATFORM_DEVICES
#1 lore raw
Quoted 7 lines · Jes Sorensen

Hi,

This patch adds a new driver to catch RFKill events to the Bluetooth device on modern Toshiba laptops. Without it, the Bluetooth device is simply lost on my Portege R500 when the RFKill switch is flipped, and doesn't come back until after a reboot.

Quoted 5 lines

It binds to the TOS6205 ACPI device and doesn't touch any existing code.

Cheers,
Jes

Cool. This must be the shortest ACPI driver :-).

Try to send patches inline if possible, it makes them easier to review.

Quoted 3 lines

+static int toshiba_bt_rfkill_add(struct acpi_device *device);

I think it's simpler to avoid the need for forward declarations, e.g. by putting the driver structure after the ops functions. I suppose it's a matter of taste though.

drivers/platform/x86/toshiba_bluetooth.c line 47

Quoted 7 lines
drivers/platform/x86/toshiba_bluetooth.c new file +5 -0
+static struct acpi_driver toshiba_bt_rfkill_driver = {
+	.name =		"Toshiba BT",
+	.class =	"Toshiba",
+	.ids =		bt_device_ids,
+	.ops =		{

Please also set

.owner = THIS_MODULE,

in order to provide the correct information under /sys/module/x/drivers and /sys/bus/acpi/drivers/x/module

Quoted 10 lines
drivers/platform/x86/toshiba_bluetooth.c new file +9 -0
+static int toshiba_bluetooth_enable(acpi_handle handle)
+{
+	acpi_status res1, res2;
+	acpi_integer result;
+
+	/* Query ACPI to verify RFKill switch is set to on */
+	res1 = acpi_evaluate_integer(handle, "BTST", NULL, &result);
+	if (ACPI_FAILURE(res1) || !(result & 0x01))
+		return -EBUSY;

...

drivers/platform/x86/toshiba_bluetooth.c line 86

Quoted 5 lines
drivers/platform/x86/toshiba_bluetooth.c new file +4 -0
+static int toshiba_bt_resume(struct acpi_device *device)
+{
+	return toshiba_bluetooth_enable(device->handle);
+}

Good to see you're handling resume, but what happens if the rfkill switch is _not_ set to on? It looks like resume will return an error, which will produce a warning message in the kernel log. I don't think we want that.

drivers/platform/x86/toshiba_bluetooth.c line 95

Quoted 8 lines
drivers/platform/x86/toshiba_bluetooth.c new file +8 -0
+static int toshiba_bt_rfkill_add(struct acpi_device *device)
+{
+	acpi_status status;
+	acpi_integer bt_present;
+	int result = -ENODEV;
+
+	status = acpi_evaluate_integer(device->handle, "_STA", NULL,
+				       &bt_present);

I think this would benefit from an explanation.

/* Some models include a placeholder for the TOS6205 device, but don't use it */ ?

/* May be disabled in BIOS */ ?

/* This device has _STA, not sure why, but let's honour it if disabled */ ?

That might give us an idea of what to do if we find a model which doesn't have _STA :-).

Personally I'd err on the side of allowing for models without _STA. You could handle that by checking that it exists before trying to evaluate it. If there's some reason for not doing this, I think it should also be mentioned in a comment.

drivers/platform/x86/toshiba_bluetooth.c line 118

Quoted 7 lines
drivers/platform/x86/toshiba_bluetooth.c new file +6 -0
+	result = acpi_bus_register_driver(&toshiba_bt_rfkill_driver);
+	if (result < 0) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+				  "Error registering driver\n"));
+		return -ENODEV;
+	}

I would suggest you return result, instead of ignoring it and always returning ENODEV.

drivers/platform/x86/Kconfig config ACPI_TOSHIBA · line 443

Quoted 2 lines
drivers/platform/x86/Kconfig +2 -0
+	depends on RFKILL
+	depends on BT

But you doesn't use either of these subsystems :-). The BT one definitely seems bogus; please drop it.

I think you _could_ export an rfkill device. It would at least be useful for trouble-shooting, because it lets userspace read the current state in a generic way. So I would recommend doing so in the future, even though most current userspaces won't do anything interesting with it. Given that the BIOS sets the state when the switch is turned off, I would recommend you expose it as a "hard" block - so you don't let userspace change the state.

But I wouldn't let that hold the driver up, since it serves an important purpose even without creating an rfkill device.

Regards
Alan

#2 lore raw
Quoted 6 lines · Alan Jenkins

I think this would benefit from an explanation.

/* Some models include a placeholder for the TOS6205 device, but don't use it */ ?

Quoted 7 lines

/* May be disabled in BIOS */ ?

/* This device has _STA, not sure why, but let's honour it if disabled */ ?

That might give us an idea of what to do if we find a model which doesn't have _STA :-).

TOS6205 seems to be present even on models which don't have bluetooth, and _STA then evaluates appropriately. But having checked, the core takes care of this so the driver shouldn't need to.

Quoted 7 lines

I think you _could_ export an rfkill device. It would at least be useful for trouble-shooting, because it lets userspace read the current state in a generic way. So I would recommend doing so in the future, even though most current userspaces won't do anything interesting with it. Given that the BIOS sets the state when the switch is turned off, I would recommend you expose it as a "hard" block - so you don't let userspace change the state.

The problem with that is that there's no event to indicate that the switch has been turned back on, so there's no way to update the rfkill core appropriately. Giving userspace potentially stale information doesn't seem helpful.

Signature · 1 line
Matthew Garrett | [email protected]
#3 lore raw
Quoted 4 lines · Matthew Garrett

The problem with that is that there's no event to indicate that the switch has been turned back on, so there's no way to update the rfkill core appropriately. Giving userspace potentially stale information doesn't seem helpful.

I believe I can call into BTST and determine whether the switch is set to on or not. It's quite easy to add this to the resume handler and only call if it's set to on.

Cheers,
Jes

#4 lore raw
Quoted 9 lines · Jes Sorensen
Quoted 4 lines · Matthew Garrett

The problem with that is that there's no event to indicate that the switch has been turned back on, so there's no way to update the rfkill core appropriately. Giving userspace potentially stale information doesn't seem helpful.

I believe I can call into BTST and determine whether the switch is set to on or not. It's quite easy to add this to the resume handler and only call if it's set to on.

Right, but in normal use it's an issue. Unless we get notifications for both types of state change, we can't add an rfkill device.

Signature · 1 line
Matthew Garrett | [email protected]
#5 lore raw
Quoted 8 lines · Matthew Garrett
Quoted 3 lines · Jes Sorensen

I believe I can call into BTST and determine whether the switch is set to on or not. It's quite easy to add this to the resume handler and only call if it's set to on.

Right, but in normal use it's an issue. Unless we get notifications for both types of state change, we can't add an rfkill device.

Understand, not sure how to go about this. It doesn't seem to generate an APIC event on off, it just pulls the USB device out of it's plug. Not sure if it would be possible to plug into the USB unplug event?

Cheers,
Jes

#6 lore raw
Quoted 3 lines · Jes Sorensen

Understand, not sure how to go about this. It doesn't seem to generate an APIC event on off, it just pulls the USB device out of it's plug. Not sure if it would be possible to plug into the USB unplug event?

In principle, I guess, but I don't think there's a great deal of advantage in it.

Signature · 1 line
Matthew Garrett | [email protected]
#7 lore raw
Quoted 8 lines · Matthew Garrett
Quoted 3 lines · Jes Sorensen

Understand, not sure how to go about this. It doesn't seem to generate an APIC event on off, it just pulls the USB device out of it's plug. Not sure if it would be possible to plug into the USB unplug event?

In principle, I guess, but I don't think there's a great deal of advantage in it.

Agreed. I think the rfkill device would be a nice-to-have for troubleshooting, but nothing more. If you can't implement it using ACPI events alone, it seems less useful for troubleshooting anyway.

Regards
Alan

#8 lore raw

Toshiba Bluetooth enabler (v2) (was: Re: [PATCH] Toshiba Bluetooth enabler (rfkill))

In reply to Alan Jenkins
Quoted 1 line · Alan Jenkins

Cool. This must be the shortest ACPI driver :-).

Hi Alan,

Here's an updated version that addresses most of your comments.

Quoted 1 line

Try to send patches inline if possible, it makes them easier to review.

Not possible with Thunderbug, sorry.

Quoted 5 lines
Quoted 1 line

+static int toshiba_bt_rfkill_add(struct acpi_device *device);

I think it's simpler to avoid the need for forward declarations, e.g. by putting the driver structure after the ops functions. I suppose it's a matter of taste though.

I am going to keep it as is. I find it cleaner and it is more consistent with how other drivers are written IMHO.

Quoted 2 lines

Please also set
.owner = THIS_MODULE,

Good point, thanks!

Quoted 9 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+static int toshiba_bt_resume(struct acpi_device *device)
+{
+	return toshiba_bluetooth_enable(device->handle);
+}

Good to see you're handling resume, but what happens if the rfkill switch is _not_ set to on? It looks like resume will return an error, which will produce a warning message in the kernel log. I don't think we want that.

Fixed, this version only calls the enabler if the switch is at ON at resume time.

Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+	status = acpi_evaluate_integer(device->handle, "_STA", NULL,
+				&bt_present);

I think this would benefit from an explanation.

Comments added.

Quoted 9 lines

Review

Quoted 6 lines
(quoted diff) +6 -0
+	result = acpi_bus_register_driver(&toshiba_bt_rfkill_driver);
+	if (result<  0) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+				  "Error registering driver\n"));
+		return -ENODEV;
+	}

I would suggest you return result, instead of ignoring it and always returning ENODEV.

I agree, added.

Quoted 6 lines

Review

Quoted 3 lines
(quoted diff) +2 -0
+	depends on RFKILL
+	depends on BT

But you doesn't use either of these subsystems :-). The BT one definitely seems bogus; please drop it.

It seemed kinda silly to me to enable this driver on kernels with no BT subsystem, but it's not a biggie so I pulled it.

Please find attached v2 of the patch.

Cheers,
Jes

Toshiba Bluetooth Enabling driver (RFKill handler)

This patch adds support for the ACPI events generated by the RFKill switch on modern Toshiba laptops, and re-enables the Bluetooth USB device when the switch is flipped back to the 'on' position.

The RFKill switch brute force pulls out the USB device when flipped to 'off', but it doesn't automatically re-enable it. Without this driver, the Bluetooth is gone until after a reboot on my Portege R500.

---

drivers/platform/x86/toshiba_bluetooth.c new file +151 -0
Index: linux-2.6.31.5/drivers/platform/x86/toshiba_bluetooth.c
===================================================================
@@ -0,0 +1,151 @@
+/*
+ * Toshiba Bluetooth Enable Driver
+ *
+ * Copyright (C) 2009 Jes Sorensen <[email protected]>
+ *
+ * Thanks to Matthew Garrett for background info on ACPI innards which
+ * normal people aren't meant to understand :-)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Note the Toshiba Bluetooth RFKill switch seems to be a strange
+ * fish. It only provides a BT event when the switch is flipped to
+ * the 'on' position. When flipping it to 'off', the USB device is
+ * simply pulled away underneath us, without any BT event being
+ * delivered.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+
+MODULE_AUTHOR("Jes Sorensen <[email protected]>");
+MODULE_DESCRIPTION("Toshiba Laptop ACPI Bluetooth Enable Driver");
+MODULE_LICENSE("GPL");
+
+
+static int toshiba_bt_rfkill_add(struct acpi_device *device);
+static int toshiba_bt_rfkill_remove(struct acpi_device *device, int type);
+static void toshiba_bt_rfkill_notify(struct acpi_device *device, u32 event);
+static int toshiba_bt_resume(struct acpi_device *device);
+
+static const struct acpi_device_id bt_device_ids[] = {
+	{ "TOS6205", 0},
+	{ "", 0},
+};
+MODULE_DEVICE_TABLE(acpi, bt_device_ids);
+
+static struct acpi_driver toshiba_bt_rfkill_driver = {
+	.name =		"Toshiba BT",
+	.class =	"Toshiba",
+	.ids =		bt_device_ids,
+	.ops =		{
+				.add =		toshiba_bt_rfkill_add,
+				.remove =	toshiba_bt_rfkill_remove,
+				.notify =	toshiba_bt_rfkill_notify,
+				.resume =	toshiba_bt_resume,
+			},
+	.owner = 	THIS_MODULE,
+};
+
+
+static int toshiba_bluetooth_enable(acpi_handle handle)
+{
+	acpi_status res1, res2;
+	acpi_integer result;
+
+	/* Query ACPI to verify RFKill switch is set to on */
+	res1 = acpi_evaluate_integer(handle, "BTST", NULL, &result);
+	if (ACPI_FAILURE(res1) || !(result & 0x01))
+		return -EBUSY;
+
+	printk(KERN_INFO "toshiba_bluetooth: Re-enabling Toshiba Bluetooth\n");
+	res1 = acpi_evaluate_object(handle, "AUSB", NULL, NULL);
+	res2 = acpi_evaluate_object(handle, "BTPO", NULL, NULL);
+	if (!ACPI_FAILURE(res1) || !ACPI_FAILURE(res2)) {
+		return 0;
+	}
+	printk(KERN_WARNING "toshiba_bluetooth: Failed to re-enable "
+	       "Toshiba Bluetooth\n");
+
+	return -ENODEV;
+}
+
+static void toshiba_bt_rfkill_notify(struct acpi_device *device, u32 event)
+{
+	toshiba_bluetooth_enable(device->handle);
+}
+
+static int toshiba_bt_resume(struct acpi_device *device)
+{
+	acpi_status res1;
+	acpi_integer result;
+
+	/*
+	 * Query ACPI to verify RFKill switch is set to on before trying
+	 * to re-enable the device or we will get back an error.
+	 */
+	res1 = acpi_evaluate_integer(device->handle, "BTST", NULL, &result);
+	if (ACPI_FAILURE(res1) || !(result & 0x01))
+		return 0;
+	return toshiba_bluetooth_enable(device->handle);
+}
+
+static int toshiba_bt_rfkill_add(struct acpi_device *device)
+{
+	acpi_status status;
+	acpi_integer bt_present;
+	int result = -ENODEV;
+
+	/*
+	 * Some Toshiba laptops may have a fake TOS6205 device in
+	 * their ACPI BIOS, so query the _STA method to see if there
+	 * is really anything there, before trying to enable it.
+	 */
+	status = acpi_evaluate_integer(device->handle, "_STA", NULL,
+				       &bt_present);
+
+	if (!ACPI_FAILURE(status) && bt_present) {
+		printk(KERN_INFO "Detected Toshiba ACPI Bluetooth device - "
+		      "installing RFKill handler\n");
+		result = toshiba_bluetooth_enable(device->handle);
+	}
+
+	return result;
+}
+
+static int __init toshiba_bt_rfkill_init(void)
+{
+	int result;
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	result = acpi_bus_register_driver(&toshiba_bt_rfkill_driver);
+	if (result < 0) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+				  "Error registering driver\n"));
+		return result;
+	}
+
+	return 0;
+}
+
+static int toshiba_bt_rfkill_remove(struct acpi_device *device, int type)
+{
+	/* clean up */
+	return 0;
+}
+
+static void __exit toshiba_bt_rfkill_exit(void)
+{
+	acpi_bus_unregister_driver(&toshiba_bt_rfkill_driver);
+}
+
+module_init(toshiba_bt_rfkill_init);
+module_exit(toshiba_bt_rfkill_exit);
drivers/platform/x86/Makefile +1 -0
Index: linux-2.6.31.5/drivers/platform/x86/Makefile
===================================================================
@@ -20,3 +20,4 @@ obj-$(CONFIG_INTEL_MENLOW)	+= intel_menl
 obj-$(CONFIG_ACPI_WMI)		+= wmi.o
 obj-$(CONFIG_ACPI_ASUS)		+= asus_acpi.o
 obj-$(CONFIG_ACPI_TOSHIBA)	+= toshiba_acpi.o
+obj-$(CONFIG_TOSHIBA_BT_RFKILL)	+= toshiba_bluetooth.o
drivers/platform/x86/Kconfig +16 -0
Index: linux-2.6.31.5/drivers/platform/x86/Kconfig
===================================================================
@@ -425,4 +425,20 @@ config ACPI_TOSHIBA
 
 	  If you have a legacy free Toshiba laptop (such as the Libretto L1
 	  series), say Y.
+
+config TOSHIBA_BT_RFKILL
+	tristate "Toshiba Bluetooth RFKill switch support"
+	depends on ACPI
+	depends on RFKILL
+	---help---
+	  This driver adds support for Bluetooth events for the RFKill
+	  switch on modern Toshiba laptops with full ACPI support and
+	  an RFKill switch.
+
+	  This driver handles RFKill events for the TOS6205 Bluetooth,
+	  and re-enables it when the switch is set back to the 'on'
+	  position.
+
+	  If you have a modern Toshiba laptop with a Bluetooth and an
+	  RFKill switch (such as the Portege R500), say Y.
 endif # X86_PLATFORM_DEVICES
#9 lore raw

Re: Toshiba Bluetooth enabler (v2) (was: Re: [PATCH] Toshiba Bluetooth enabler (rfkill))

Quoted 6 lines · Jes Sorensen
Quoted 1 line · Alan Jenkins

Cool. This must be the shortest ACPI driver :-).

Hi Alan,

Here's an updated version that addresses most of your comments.

Hi again

Quoted 3 lines
Quoted 1 line

Try to send patches inline if possible, it makes them easier to review.

Not possible with Thunderbug, sorry.

I'm guessing you mean Thunderbird. I manage in Thunderbird by

a) composing in HTML
b) selecting "preformat" from the drop-down menu (the default is "body text"
c) copy+pasting the patch

I seem to have word wrap disabled as well (under prefs->composition->general), but I think "Preformat" is enough on its own.

Quoted 7 lines
Quoted 4 lines

Good to see you're handling resume, but what happens if the rfkill switch is _not_ set to on? It looks like resume will return an error, which will produce a warning message in the kernel log. I don't think we want that.

Fixed, this version only calls the enabler if the switch is at ON at resume time.

Ah... I think add() has the same problem as well though? I.e. the driver will not work if the switch is disabled at load time.

I would change it in enable() (and then try to think of a new name for it, maybe try_enable()).

Quoted 6 lines
Quoted 4 lines

drivers/platform/x86/toshiba_bluetooth.c line 111

Quoted 2 lines
drivers/platform/x86/toshiba_bluetooth.c new file +2 -0
+	status = acpi_evaluate_integer(device->handle, "_STA", NULL,
+				&bt_present);

I think this would benefit from an explanation.

Comments added.

Thanks.

drivers/platform/x86/toshiba_bluetooth.c line 127

Quoted 6 lines
drivers/platform/x86/toshiba_bluetooth.c new file +6 -0
+static int __init toshiba_bt_rfkill_init(void)
+{
+	int result;
+
+	if (acpi_disabled)
+		return -ENODEV;

Sorry for not spotting this earlier, but this test is redundant. acpi_bus_register() will check if acpi_disabled for you (and return -ENODEV).

Quoted 11 lines
Quoted 9 lines

drivers/platform/x86/toshiba_bluetooth.c line 134

Quoted 6 lines
drivers/platform/x86/toshiba_bluetooth.c new file +6 -0
+	result = acpi_bus_register_driver(&toshiba_bt_rfkill_driver);
+	if (result<  0) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+				  "Error registering driver\n"));
+		return -ENODEV;
+	}

I would suggest you return result, instead of ignoring it and always returning ENODEV.

I agree, added.

Ok.

Quoted 8 lines
Quoted 5 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+	depends on RFKILL
+	depends on BT

But you doesn't use either of these subsystems :-). The BT one definitely seems bogus; please drop it.

It seemed kinda silly to me to enable this driver on kernels with no BT subsystem, but it's not a biggie so I pulled it.

This is linux :-). Maybe someone wants to disable the BT drivers and write their own using libusb, or access the device from an emulated OS ("qemu -usb host:<vendor_id:product_id>"). Let's not stop them.

I don't think it should depend on RFKILL either. None of the other platform drivers do a literal "depends on RFKILL" at the moment. I agree that this driver is a bit special, but I think complex cross-menu depends are more frustrating than helpful.

Configuring kernels is hard - I think depends like this make it harder. If you don't enable RFKILL, you won't see "If you have a modern Toshiba laptop with a Bluetooth and an RFKill switch (such as the Portege R500), say Y". Then your bluetooth will mysteriously stop working when you toggle the switch off and on again :).

Thanks
Alan

#10 lore raw

Re: Toshiba Bluetooth enabler (v2) (was: Re: [PATCH] Toshiba Bluetooth enabler (rfkill))

Quoted 16 lines · Alan Jenkins
Quoted 6 lines · Jes Sorensen
Quoted 1 line · Alan Jenkins

Cool. This must be the shortest ACPI driver :-).

Hi Alan,

Here's an updated version that addresses most of your comments.

Hi again

Quoted 3 lines
Quoted 1 line

Try to send patches inline if possible, it makes them easier to review.

Not possible with Thunderbug, sorry.

I'm guessing you mean Thunderbird. I manage in Thunderbird by

Quoted 8 lines

a) composing in HTML
b) selecting "preformat" from the drop-down menu (the default is "body text"
c) copy+pasting the patch

I seem to have word wrap disabled as well (under prefs->composition->general), but I think "Preformat" is enough on its own.

There is a Thunderbird section under Documentation/email-clients.txt

regards,
dan carpenter

#11 lore raw

Re: Toshiba Bluetooth enabler (v2) (was: Re: [PATCH] Toshiba Bluetooth enabler (rfkill))

Quoted 22 lines · Dan Carpenter
Quoted 21 lines · Alan Jenkins
Quoted 6 lines · Jes Sorensen
Quoted 1 line · Alan Jenkins

Cool. This must be the shortest ACPI driver :-).

Hi Alan,

Here's an updated version that addresses most of your comments.

Hi again

Quoted 3 lines
Quoted 1 line

Try to send patches inline if possible, it makes them easier to review.

Not possible with Thunderbug, sorry.

I'm guessing you mean Thunderbird. I manage in Thunderbird by

a) composing in HTML
b) selecting "preformat" from the drop-down menu (the default is "body text"
c) copy+pasting the patch

Quoted 6 lines
Quoted 4 lines

I seem to have word wrap disabled as well (under prefs->composition->general), but I think "Preformat" is enough on its own.

There is a Thunderbird section under Documentation/email-clients.txt

Yes. I found it and then didn't mention it.

"
- Under account settings, composition and addressing, uncheck "Compose messages in HTML format".
...
- You need to get Thunderbird into preformat mode:
. If you compose HTML messages by default, it's not too hard. "

The conflicting suggestions make sense if you know it's describing three different alternatives, but that's not immediately obvious. So it's was easier for me to explain the specific method I use.

Alan

#12 lore raw

PATCH: Toshiba Bluetooth enabler (v3)

In reply to Alan Jenkins
Quoted 8 lines · Alan Jenkins
Quoted 2 lines

Fixed, this version only calls the enabler if the switch is at ON at resume time.

Ah... I think add() has the same problem as well though? I.e. the driver will not work if the switch is disabled at load time.

I would change it in enable() (and then try to think of a new name for it, maybe try_enable()).

Sorta did that, it should do the right thing in all those cases now.

Otherwise I am pretty pleased with it now. Unless there's strong objections, I'd say it's good to go.

Cheers,
Jes

Toshiba Bluetooth Enabling driver (RFKill handler)

This patch adds support for the ACPI events generated by the RFKill switch on modern Toshiba laptops, and re-enables the Bluetooth USB device when the switch is flipped back to the 'on' position.

The RFKill switch brute force pulls out the USB device when flipped to 'off', but it doesn't automatically re-enable it. Without this driver, the Bluetooth is gone until after a reboot on my Portege R500.

---

drivers/platform/x86/toshiba_bluetooth.c new file +144 -0
Index: linux-2.6.31.5/drivers/platform/x86/toshiba_bluetooth.c
===================================================================
@@ -0,0 +1,144 @@
+/*
+ * Toshiba Bluetooth Enable Driver
+ *
+ * Copyright (C) 2009 Jes Sorensen <[email protected]>
+ *
+ * Thanks to Matthew Garrett for background info on ACPI innards which
+ * normal people aren't meant to understand :-)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Note the Toshiba Bluetooth RFKill switch seems to be a strange
+ * fish. It only provides a BT event when the switch is flipped to
+ * the 'on' position. When flipping it to 'off', the USB device is
+ * simply pulled away underneath us, without any BT event being
+ * delivered.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+
+MODULE_AUTHOR("Jes Sorensen <[email protected]>");
+MODULE_DESCRIPTION("Toshiba Laptop ACPI Bluetooth Enable Driver");
+MODULE_LICENSE("GPL");
+
+
+static int toshiba_bt_rfkill_add(struct acpi_device *device);
+static int toshiba_bt_rfkill_remove(struct acpi_device *device, int type);
+static void toshiba_bt_rfkill_notify(struct acpi_device *device, u32 event);
+static int toshiba_bt_resume(struct acpi_device *device);
+
+static const struct acpi_device_id bt_device_ids[] = {
+	{ "TOS6205", 0},
+	{ "", 0},
+};
+MODULE_DEVICE_TABLE(acpi, bt_device_ids);
+
+static struct acpi_driver toshiba_bt_rfkill_driver = {
+	.name =		"Toshiba BT",
+	.class =	"Toshiba",
+	.ids =		bt_device_ids,
+	.ops =		{
+				.add =		toshiba_bt_rfkill_add,
+				.remove =	toshiba_bt_rfkill_remove,
+				.notify =	toshiba_bt_rfkill_notify,
+				.resume =	toshiba_bt_resume,
+			},
+	.owner = 	THIS_MODULE,
+};
+
+
+static int toshiba_bluetooth_enable(acpi_handle handle)
+{
+	acpi_status res1, res2;
+	acpi_integer result;
+
+	/*
+	 * Query ACPI to verify RFKill switch is set to 'on'.
+	 * If not, we return silently, no need to report it as
+	 * an error.
+	 */
+	res1 = acpi_evaluate_integer(handle, "BTST", NULL, &result);
+	if (ACPI_FAILURE(res1))
+		return res1;
+	if (!(result & 0x01))
+		return 0;
+
+	printk(KERN_INFO "toshiba_bluetooth: Re-enabling Toshiba Bluetooth\n");
+	res1 = acpi_evaluate_object(handle, "AUSB", NULL, NULL);
+	res2 = acpi_evaluate_object(handle, "BTPO", NULL, NULL);
+	if (!ACPI_FAILURE(res1) || !ACPI_FAILURE(res2)) {
+		return 0;
+	}
+	printk(KERN_WARNING "toshiba_bluetooth: Failed to re-enable "
+	       "Toshiba Bluetooth\n");
+
+	return -ENODEV;
+}
+
+static void toshiba_bt_rfkill_notify(struct acpi_device *device, u32 event)
+{
+	toshiba_bluetooth_enable(device->handle);
+}
+
+static int toshiba_bt_resume(struct acpi_device *device)
+{
+	return toshiba_bluetooth_enable(device->handle);
+}
+
+static int toshiba_bt_rfkill_add(struct acpi_device *device)
+{
+	acpi_status status;
+	acpi_integer bt_present;
+	int result = -ENODEV;
+
+	/*
+	 * Some Toshiba laptops may have a fake TOS6205 device in
+	 * their ACPI BIOS, so query the _STA method to see if there
+	 * is really anything there, before trying to enable it.
+	 */
+	status = acpi_evaluate_integer(device->handle, "_STA", NULL,
+				       &bt_present);
+
+	if (!ACPI_FAILURE(status) && bt_present) {
+		printk(KERN_INFO "Detected Toshiba ACPI Bluetooth device - "
+		      "installing RFKill handler\n");
+		result = toshiba_bluetooth_enable(device->handle);
+	}
+
+	return result;
+}
+
+static int __init toshiba_bt_rfkill_init(void)
+{
+	int result;
+
+	result = acpi_bus_register_driver(&toshiba_bt_rfkill_driver);
+	if (result < 0) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+				  "Error registering driver\n"));
+		return result;
+	}
+
+	return 0;
+}
+
+static int toshiba_bt_rfkill_remove(struct acpi_device *device, int type)
+{
+	/* clean up */
+	return 0;
+}
+
+static void __exit toshiba_bt_rfkill_exit(void)
+{
+	acpi_bus_unregister_driver(&toshiba_bt_rfkill_driver);
+}
+
+module_init(toshiba_bt_rfkill_init);
+module_exit(toshiba_bt_rfkill_exit);
drivers/platform/x86/Makefile +1 -0
Index: linux-2.6.31.5/drivers/platform/x86/Makefile
===================================================================
@@ -20,3 +20,4 @@ obj-$(CONFIG_INTEL_MENLOW)	+= intel_menl
 obj-$(CONFIG_ACPI_WMI)		+= wmi.o
 obj-$(CONFIG_ACPI_ASUS)		+= asus_acpi.o
 obj-$(CONFIG_ACPI_TOSHIBA)	+= toshiba_acpi.o
+obj-$(CONFIG_TOSHIBA_BT_RFKILL)	+= toshiba_bluetooth.o
drivers/platform/x86/Kconfig +16 -0
Index: linux-2.6.31.5/drivers/platform/x86/Kconfig
===================================================================
@@ -425,4 +425,20 @@ config ACPI_TOSHIBA
 
 	  If you have a legacy free Toshiba laptop (such as the Libretto L1
 	  series), say Y.
+
+config TOSHIBA_BT_RFKILL
+	tristate "Toshiba Bluetooth RFKill switch support"
+	depends on ACPI
+	depends on RFKILL
+	---help---
+	  This driver adds support for Bluetooth events for the RFKill
+	  switch on modern Toshiba laptops with full ACPI support and
+	  an RFKill switch.
+
+	  This driver handles RFKill events for the TOS6205 Bluetooth,
+	  and re-enables it when the switch is set back to the 'on'
+	  position.
+
+	  If you have a modern Toshiba laptop with a Bluetooth and an
+	  RFKill switch (such as the Portege R500), say Y.
 endif # X86_PLATFORM_DEVICES
#13 lore raw

Re: PATCH: Toshiba Bluetooth enabler (v3)

Quoted 11 lines · Jes Sorensen
Quoted 8 lines · Alan Jenkins
Quoted 2 lines

Fixed, this version only calls the enabler if the switch is at ON at resume time.

Ah... I think add() has the same problem as well though? I.e. the driver will not work if the switch is disabled at load time.

I would change it in enable() (and then try to think of a new name for it, maybe try_enable()).

Sorta did that, it should do the right thing in all those cases now.

Great, looks good.

Quoted 5 lines

Otherwise I am pretty pleased with it now. Unless there's strong objections, I'd say it's good to go.

Cheers,
Jes

I did question "depends on RFKILL". If you manually configure a kernel, you will miss "If you have a modern Toshiba laptop with a Bluetooth and an RFKill switch (such as the Portege R500), say Y." unless you enable RFKILL first. (Which is unnecessary since the driver will work exactly the same with RFKILL=n). I wouldn't call it a strong objection, but it would be nice to hear the reason you included this dependency.

Everything else looks fine. Thanks for working on this. Alan

#14 lore raw

Re: PATCH: Toshiba Bluetooth enabler (v3)

Quoted 10 lines · Alan Jenkins

I did question "depends on RFKILL". If you manually configure a kernel, you will miss "If you have a modern Toshiba laptop with a Bluetooth and an RFKill switch (such as the Portege R500), say Y." unless you enable RFKILL first. (Which is unnecessary since the driver will work exactly the same with RFKILL=n). I wouldn't call it a strong objection, but it would be nice to hear the reason you included this dependency.

Everything else looks fine. Thanks for working on this. Alan

The driver is acting as RFKill functionality, even if it doesn't provide it in full. We can pull that depend if you like, however I still think it's awkward to offer a driver for a functionality the user had previously said no to.

Cheers,
Jes

#15 lore raw

Re: PATCH: Toshiba Bluetooth enabler (v3)

Quoted 9 lines · Jes Sorensen
Quoted 8 lines · Alan Jenkins

I did question "depends on RFKILL". If you manually configure a kernel, you will miss "If you have a modern Toshiba laptop with a Bluetooth and an RFKill switch (such as the Portege R500), say Y." unless you enable RFKILL first. (Which is unnecessary since the driver will work exactly the same with RFKILL=n). I wouldn't call it a strong objection, but it would be nice to hear the reason you included this dependency.

Quoted 7 lines
Quoted 2 lines

Everything else looks fine. Thanks for working on this. Alan

The driver is acting as RFKill functionality, even if it doesn't provide it in full. We can pull that depend if you like, however I still think it's awkward to offer a driver for a functionality the user had previously said no to.

Ok. You have the advantage of having hardware, so you have a better idea of what the user experience is going to be like. I can't find any more compelling argument for dropping the dependency. If it's just me feeling uneasy about this, then don't let me stop you.

Alan

reviewer Applied
#16 lore raw

Re: PATCH: Toshiba Bluetooth enabler (v3)

In reply to Jes Sorensen

applied, sans dependency on RFKILL where I agree with Alan that it will probably just confuse folks who build their own kernel rather than add significant value.

thanks,
Len Brown, Intel Open Source Technology Center