2008-08-02 19:13:12

by Philip Langdale

[permalink] [raw]
Subject: [PATCH 1/1] toshiba_acpi: Add support for bluetooth toggling through rfkill (v2)

There's been a patch floating around for toshiba_acpi that exports an ad-hoc
/proc interface to toggle the bluetooth adapter in a large number of Toshiba
laptops. I'm not sure if it's still relevant for the latest models, but it is
still required for older models such as my Tecra M3.

This change pulls in the low level Toshiba-specific code from the old patch and
sets up an rfkill device and a polled input device to track the state of the
hardware kill-switch.

Signed-off-by: Philip Langdale <[email protected]>
---
Kconfig | 1
toshiba_acpi.c | 327 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 320 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 735f5ea..047829c 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -260,6 +260,7 @@ config ACPI_ASUS
config ACPI_TOSHIBA
tristate "Toshiba Laptop Extras"
depends on X86
+ select INPUT_POLLDEV
select BACKLIGHT_CLASS_DEVICE
---help---
This driver adds support for access to certain system settings
diff --git a/drivers/acpi/toshiba_acpi.c b/drivers/acpi/toshiba_acpi.c
index 0a43c8e..76afa75 100644
--- a/drivers/acpi/toshiba_acpi.c
+++ b/drivers/acpi/toshiba_acpi.c
@@ -3,6 +3,7 @@
*
*
* Copyright (C) 2002-2004 John Belmonte
+ * Copyright (C) 2008 Philip Langdale
*
* 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
@@ -33,7 +34,7 @@
*
*/

-#define TOSHIBA_ACPI_VERSION "0.18"
+#define TOSHIBA_ACPI_VERSION "0.19"
#define PROC_INTERFACE_VERSION 1

#include <linux/kernel.h>
@@ -42,6 +43,9 @@
#include <linux/types.h>
#include <linux/proc_fs.h>
#include <linux/backlight.h>
+#include <linux/platform_device.h>
+#include <linux/rfkill.h>
+#include <linux/input-polldev.h>

#include <asm/uaccess.h>

@@ -62,6 +66,10 @@ MODULE_LICENSE("GPL");
#define METHOD_HCI_2 "\\_SB_.VALZ.GHCI"
#define METHOD_VIDEO_OUT "\\_SB_.VALX.DSSX"

+/* Toshiba ACPI Bluetooth object */
+#define BT_ACPI_OBJECT "\\_SB_.BT"
+#define BT_ACPI_SOFT_UNBLOCKED_EVENT 0x90
+
/* Toshiba HCI interface definitions
*
* HCI is Toshiba's "Hardware Control Interface" which is supposed to
@@ -90,6 +98,7 @@ MODULE_LICENSE("GPL");
#define HCI_VIDEO_OUT 0x001c
#define HCI_HOTKEY_EVENT 0x001e
#define HCI_LCD_BRIGHTNESS 0x002a
+#define HCI_WIRELESS 0x0056

/* field definitions */
#define HCI_LCD_BRIGHTNESS_BITS 3
@@ -98,9 +107,14 @@ MODULE_LICENSE("GPL");
#define HCI_VIDEO_OUT_LCD 0x1
#define HCI_VIDEO_OUT_CRT 0x2
#define HCI_VIDEO_OUT_TV 0x4
+#define HCI_WIRELESS_KILL_SWITCH 0x01
+#define HCI_WIRELESS_BT_PRESENT 0x0f
+#define HCI_WIRELESS_BT_ATTACH 0x40
+#define HCI_WIRELESS_BT_POWER 0x80

static const struct acpi_device_id toshiba_device_ids[] = {
{"TOS6200", 0},
+ {"TOS6208", 0},
{"TOS1900", 0},
{"", 0},
};
@@ -193,7 +207,7 @@ static acpi_status hci_raw(const u32 in[HCI_WORDS], u32 out[HCI_WORDS])
return status;
}

-/* common hci tasks (get or set one value)
+/* common hci tasks (get or set one or two value)
*
* In addition to the ACPI status, the HCI system returns a result which
* may be useful (such as "not supported").
@@ -218,6 +232,187 @@ static acpi_status hci_read1(u32 reg, u32 * out1, u32 * result)
return status;
}

+static acpi_status hci_write2(u32 reg, u32 in1, u32 in2, u32* result)
+{
+ u32 in[HCI_WORDS] = { HCI_SET, reg, in1, in2, 0, 0 };
+ u32 out[HCI_WORDS];
+ acpi_status status = hci_raw(in, out);
+ *result = (status == AE_OK) ? out[0] : HCI_FAILURE;
+ return status;
+}
+
+static acpi_status hci_read2(u32 reg, u32* out1, u32* out2, u32* result)
+{
+ u32 in[HCI_WORDS] = { HCI_GET, reg, *out1, *out2, 0, 0 };
+ u32 out[HCI_WORDS];
+ acpi_status status = hci_raw(in, out);
+ *out1 = out[2];
+ *out2 = out[3];
+ *result = (status == AE_OK) ? out[0] : HCI_FAILURE;
+ return status;
+}
+
+struct toshiba_acpi_dev {
+ struct platform_device *p_dev;
+ struct rfkill *rfk_dev;
+ struct input_polled_dev *poll_dev;
+
+ const char *bt_name;
+ acpi_handle bt_handle;
+ bool ignore_next_bt_event;
+
+ bool last_rfk_state;
+
+ struct mutex mutex;
+};
+
+static struct toshiba_acpi_dev toshiba_acpi = {
+ .bt_name = "Toshiba Bluetooth",
+ .ignore_next_bt_event = FALSE,
+ .last_rfk_state = FALSE,
+};
+
+/* Bluetooth rfkill handlers */
+
+static u32 hci_get_bt_present(bool *present)
+{
+ u32 hci_result;
+ u32 value, value2;
+ value = 0;
+ value2 = 0;
+ hci_read2(HCI_WIRELESS, &value, &value2, &hci_result);
+ if (hci_result == HCI_SUCCESS) {
+ *present = (value & HCI_WIRELESS_BT_PRESENT) ? TRUE : FALSE;
+ }
+ return hci_result;
+}
+
+static u32 hci_get_bt_on(bool *on)
+{
+ u32 hci_result;
+ u32 value, value2;
+ value = 0;
+ value2 = 0x0001;
+ hci_read2(HCI_WIRELESS, &value, &value2, &hci_result);
+ if (hci_result == HCI_SUCCESS) {
+ *on = (value & HCI_WIRELESS_BT_POWER) &&
+ (value & HCI_WIRELESS_BT_ATTACH);
+ }
+ return hci_result;
+}
+
+static u32 hci_get_radio_state(bool *radio_state)
+{
+ u32 hci_result;
+ u32 value, value2;
+
+ value = 0;
+ value2 = 0x0001;
+ hci_read2(HCI_WIRELESS, &value, &value2, &hci_result);
+
+ *radio_state = value & HCI_WIRELESS_KILL_SWITCH;
+ return hci_result;
+}
+
+static int bt_rfkill_toggle_radio(void *data, enum rfkill_state state)
+{
+ u32 result1, result2;
+ u32 value;
+ bool radio_state;
+
+ struct toshiba_acpi_dev *dev = data;
+
+ value = state == RFKILL_STATE_UNBLOCKED;
+
+ if (hci_get_radio_state(&radio_state) != HCI_SUCCESS) {
+ return -EFAULT;
+ }
+
+ switch (state) {
+ case RFKILL_STATE_UNBLOCKED:
+ if (!radio_state) {
+ return -EPERM;
+ }
+ break;
+ case RFKILL_STATE_SOFT_BLOCKED:
+ /*
+ * The ACPI event is emitted on every transition to
+ * the SOFT_BLOCKED state. We want to respond to
+ * it by turning the bluetooth device on when going
+ * from HARD_BLOCKED, but we certainly don't want
+ * to when going from UNBLOCKED! So we have to explictly
+ * ignore the next event in that case.
+ */
+ if (radio_state) {
+ dev->ignore_next_bt_event = TRUE;
+ }
+ break;
+ case RFKILL_STATE_HARD_BLOCKED:
+ printk(MY_NOTICE "Ignored HARD_BLOCKED request by software\n");
+ break;
+ }
+
+ mutex_lock(&dev->mutex);
+ hci_write2(HCI_WIRELESS, value, HCI_WIRELESS_BT_POWER, &result1);
+ hci_write2(HCI_WIRELESS, value, HCI_WIRELESS_BT_ATTACH, &result2);
+ mutex_unlock(&dev->mutex);
+
+ if (result1 != HCI_SUCCESS || result2 != HCI_SUCCESS) {
+ return -EFAULT;
+ }
+
+ return 0;
+}
+
+static void bt_acpi_notify(acpi_handle handle, u32 event, void *data)
+{
+ struct toshiba_acpi_dev *dev = data;
+
+ switch (event) {
+ case BT_ACPI_SOFT_UNBLOCKED_EVENT:
+ if (!dev->ignore_next_bt_event) {
+ bt_rfkill_toggle_radio(data, RFKILL_STATE_UNBLOCKED);
+ rfkill_force_state(dev->rfk_dev,
+ RFKILL_STATE_UNBLOCKED);
+ } else {
+ dev->ignore_next_bt_event = FALSE;
+ }
+ break;
+ default:
+ printk(MY_NOTICE "Unknown event notified on BT object\n");
+ break;
+ }
+}
+
+static void bt_poll_rfkill(struct input_polled_dev *poll_dev)
+{
+ bool state_changed;
+ bool new_rfk_state;
+ bool value;
+ u32 hci_result;
+
+ struct toshiba_acpi_dev *dev = poll_dev->private;
+
+ hci_result = hci_get_radio_state(&value);
+ if (hci_result != HCI_SUCCESS) {
+ return; /* Can't do anything useful */
+ }
+ new_rfk_state = !value;
+
+ mutex_lock(&dev->mutex);
+ state_changed = new_rfk_state != dev->last_rfk_state;
+ dev->last_rfk_state = new_rfk_state;
+ mutex_unlock(&dev->mutex);
+
+ if (unlikely(state_changed)) {
+ if (new_rfk_state) {
+ rfkill_force_state(dev->rfk_dev,
+ RFKILL_STATE_HARD_BLOCKED);
+ }
+ input_report_switch(poll_dev->input, SW_RFKILL_ALL, new_rfk_state);
+ }
+}
+
static struct proc_dir_entry *toshiba_proc_dir /*= 0*/ ;
static struct backlight_device *toshiba_backlight_device;
static int force_fan;
@@ -547,6 +742,22 @@ static struct backlight_ops toshiba_backlight_data = {

static void toshiba_acpi_exit(void)
{
+ if (toshiba_acpi.poll_dev) {
+ input_unregister_polled_device(toshiba_acpi.poll_dev);
+ input_free_polled_device(toshiba_acpi.poll_dev);
+ }
+
+ if (toshiba_acpi.bt_handle) {
+ acpi_remove_notify_handler(toshiba_acpi.bt_handle,
+ ACPI_ALL_NOTIFY,
+ bt_acpi_notify);
+ }
+
+ if (toshiba_acpi.rfk_dev) {
+ rfkill_unregister(toshiba_acpi.rfk_dev);
+ rfkill_free(toshiba_acpi.rfk_dev);
+ }
+
if (toshiba_backlight_device)
backlight_device_unregister(toshiba_backlight_device);

@@ -555,6 +766,8 @@ static void toshiba_acpi_exit(void)
if (toshiba_proc_dir)
remove_proc_entry(PROC_TOSHIBA, acpi_root_dir);

+ platform_device_unregister(toshiba_acpi.p_dev);
+
return;
}

@@ -562,6 +775,10 @@ static int __init toshiba_acpi_init(void)
{
acpi_status status = AE_OK;
u32 hci_result;
+ bool bt_present;
+ bool bt_on;
+ bool radio_on;
+ int ret = 0;

if (acpi_disabled)
return -ENODEV;
@@ -578,6 +795,18 @@ static int __init toshiba_acpi_init(void)
TOSHIBA_ACPI_VERSION);
printk(MY_INFO " HCI method: %s\n", method_hci);

+ mutex_init(&toshiba_acpi.mutex);
+
+ toshiba_acpi.p_dev = platform_device_register_simple("toshiba_acpi",
+ -1, NULL, 0);
+ if (IS_ERR(toshiba_acpi.p_dev)) {
+ ret = PTR_ERR(toshiba_acpi.p_dev);
+ printk(MY_ERR "unable to register platform device\n");
+ toshiba_acpi.p_dev= NULL;
+ toshiba_acpi_exit();
+ return ret;
+ }
+
force_fan = 0;
key_event_valid = 0;

@@ -586,19 +815,23 @@ static int __init toshiba_acpi_init(void)

toshiba_proc_dir = proc_mkdir(PROC_TOSHIBA, acpi_root_dir);
if (!toshiba_proc_dir) {
- status = AE_ERROR;
+ toshiba_acpi_exit();
+ return -ENODEV;
} else {
toshiba_proc_dir->owner = THIS_MODULE;
status = add_device();
- if (ACPI_FAILURE(status))
- remove_proc_entry(PROC_TOSHIBA, acpi_root_dir);
+ if (ACPI_FAILURE(status)) {
+ toshiba_acpi_exit();
+ return -ENODEV;
+ }
}

- toshiba_backlight_device = backlight_device_register("toshiba",NULL,
+ toshiba_backlight_device = backlight_device_register("toshiba",
+ &toshiba_acpi.p_dev->dev,
NULL,
&toshiba_backlight_data);
if (IS_ERR(toshiba_backlight_device)) {
- int ret = PTR_ERR(toshiba_backlight_device);
+ ret = PTR_ERR(toshiba_backlight_device);

printk(KERN_ERR "Could not register toshiba backlight device\n");
toshiba_backlight_device = NULL;
@@ -607,7 +840,85 @@ static int __init toshiba_acpi_init(void)
}
toshiba_backlight_device->props.max_brightness = HCI_LCD_BRIGHTNESS_LEVELS - 1;

- return (ACPI_SUCCESS(status)) ? 0 : -ENODEV;
+ /* Register rfkill switch for Bluetooth */
+ if (hci_get_bt_present(&bt_present) == HCI_SUCCESS && bt_present) {
+ toshiba_acpi.rfk_dev = rfkill_allocate(&toshiba_acpi.p_dev->dev,
+ RFKILL_TYPE_BLUETOOTH);
+ if (!toshiba_acpi.rfk_dev) {
+ printk(MY_ERR "unable to allocate rfkill device\n");
+ toshiba_acpi_exit();
+ return -ENOMEM;
+ }
+
+ toshiba_acpi.rfk_dev->name = toshiba_acpi.bt_name;
+ toshiba_acpi.rfk_dev->state = RFKILL_STATE_OFF;
+ toshiba_acpi.rfk_dev->toggle_radio = bt_rfkill_toggle_radio;
+ toshiba_acpi.rfk_dev->user_claim_unsupported = 1;
+ toshiba_acpi.rfk_dev->data = &toshiba_acpi;
+ toshiba_acpi.rfk_dev->dev.class->suspend = NULL;
+ toshiba_acpi.rfk_dev->dev.class->resume = NULL;
+
+ ret = rfkill_register(toshiba_acpi.rfk_dev);
+ if (ret) {
+ printk(MY_ERR "unable to register rfkill device\n");
+ toshiba_acpi_exit();
+ return -ENOMEM;
+ }
+
+ if (hci_get_bt_on(&bt_on) == HCI_SUCCESS && bt_on) {
+ toshiba_acpi.rfk_dev->state = RFKILL_STATE_UNBLOCKED;
+ } else if (hci_get_radio_state(&radio_on) == HCI_SUCCESS && radio_on) {
+ toshiba_acpi.rfk_dev->state = RFKILL_STATE_HARD_BLOCKED;
+ } else {
+ toshiba_acpi.rfk_dev->state = RFKILL_STATE_SOFT_BLOCKED;
+ }
+
+ /* Register for acpi events on BT ACPI object. */
+ status = acpi_get_handle(NULL, BT_ACPI_OBJECT, &toshiba_acpi.bt_handle);
+ if (ACPI_FAILURE(status)) {
+ printk(MY_ERR "unable to find ACPI Bluetooth object\n");
+ toshiba_acpi.bt_handle = NULL;
+ toshiba_acpi_exit();
+ return -ENODEV;
+ }
+ status = acpi_install_notify_handler(toshiba_acpi.bt_handle,
+ ACPI_ALL_NOTIFY,
+ bt_acpi_notify,
+ &toshiba_acpi);
+ if (ACPI_FAILURE(status)) {
+ printk(MY_ERR "unable to register for events "
+ "on ACPI Bluetooth object\n");
+ toshiba_acpi.bt_handle = NULL;
+ toshiba_acpi_exit();
+ return -ENODEV;
+ }
+ }
+
+ /* Register input device for kill switch */
+ toshiba_acpi.poll_dev = input_allocate_polled_device();
+ if (!toshiba_acpi.poll_dev) {
+ printk(MY_ERR "unable to allocate kill-switch input device\n");
+ toshiba_acpi_exit();
+ return -ENOMEM;
+ }
+ toshiba_acpi.poll_dev->private = &toshiba_acpi;
+ toshiba_acpi.poll_dev->poll = bt_poll_rfkill;
+ toshiba_acpi.poll_dev->poll_interval = 1000; /* msecs */
+
+ toshiba_acpi.poll_dev->input->name = toshiba_acpi.bt_name;
+ toshiba_acpi.poll_dev->input->id.bustype = BUS_HOST;
+ toshiba_acpi.poll_dev->input->id.vendor = 0x0930; /* Toshiba USB Vendor ID */
+ toshiba_acpi.poll_dev->input->evbit[0] = BIT(EV_SW);
+ set_bit(SW_RFKILL_ALL, toshiba_acpi.poll_dev->input->keybit);
+
+ ret = input_register_polled_device(toshiba_acpi.poll_dev);
+ if (ret) {
+ printk(MY_ERR "unable to register kill-switch input device\n");
+ toshiba_acpi_exit();
+ return ret;
+ }
+
+ return 0;
}

module_init(toshiba_acpi_init);


2008-08-04 01:11:00

by Philip Langdale

[permalink] [raw]
Subject: Re: [PATCH 1/1] toshiba_acpi: Add support for bluetooth toggling through rfkill (v2)

Henrique de Moraes Holschuh wrote:
>> + value = state == RFKILL_STATE_UNBLOCKED;
>
> value = (state == RFKILL_STATE_UNBLOCKED);
>
> It is a lot easier to read without confusing the == for a =.

Fixed.

>
> You don't really need the above, rfkill won't ever call your toggle_radio
> callback like that.
>
> If you want paranoid checking, do this instead:
>
> default:
> /* maybe WARN(), WARN_ON() or printk here */
> return -EINVAL;

Fixed.

>> +static void bt_acpi_notify(acpi_handle handle, u32 event, void *data)
>> +{
>> + struct toshiba_acpi_dev *dev = data;
>> +
>> + switch (event) {
>> + case BT_ACPI_SOFT_UNBLOCKED_EVENT:
>> + if (!dev->ignore_next_bt_event) {
>> + bt_rfkill_toggle_radio(data, RFKILL_STATE_UNBLOCKED);
>> + rfkill_force_state(dev->rfk_dev,
>> + RFKILL_STATE_UNBLOCKED);
>
> This one got me confused. Why do you need that bt_rfkill_toggle_radio call
> here?

When you turn off the hardware kill switch, the hardware will not reactivate
the bluetooth device. it just returns to the SOFT_UNBLOCKED state. I put that
in so that it would turn the device back on - a generally more desirable
behaviour - otherwise the user has to dig around for a software way to turn
it back. All the other hardware I've ever seen (including the wifi device on
this laptop) turns it back on, so it seemed sensible to try and make it work
as people would expect.

>
> Read the kernel-doc headers of every rfkill function you call at least
> once... Never rfkill_free() something you rfkill_unregister()'ed.
>
> rfkill_free() is just for the error unwind of a failure between
> rfkill_allocate() and rfkill_register().

Fair enough, but it doesn't help that rfkill and input-polldev work in
exactly opposite ways; polldev requires you to unregister and then free;
some consistency wouldn't hurt.

>> + toshiba_acpi.rfk_dev->dev.class->suspend = NULL;
>> + toshiba_acpi.rfk_dev->dev.class->resume = NULL;
>
> Why?

Good question. Gone.

>
> Do the above between rfkill_allocate() and rfkill_register().
>

Moved.

Diff is updated and resent as v3.

Thanks,

--phil

Subject: Re: [PATCH 1/1] toshiba_acpi: Add support for bluetooth toggling through rfkill (v2)

On Tue, 05 Aug 2008, Philip Langdale wrote:
> Henrique de Moraes Holschuh wrote:
>> rfkill-input (now) or userspace (someday) will take care of kicking the
>> radio to RFKILL_STATE_UNBLOCKED when (1) issues an event that signals that
>> radios don't have to remain blocked. Maybe this is why you see the WLAN
>> going on when you deactivate the radio kill switch?
>
> It's all done behind the scenes I think (it's an ipw2200 device). There's
> no rfkill integration from that driver.

Then it is probably not turning the device on, but rather, reverting its
state. This is normal with hardware like the ipw2200 that has a hardware
rfkill line.

>> And rfkill-input will soon be enhanced to let the user configure it to do
>> something different if he wants. Your driver doesn't (and shouldn't)
>> hardcode policy about it.
>
> Ok, that makes things much easier for me :-) But it means that for now the
> user will have to manually kick the device.

Actually, all the users have to do right now is to have rfkill-input loaded.
It *already* kicks all devices online when you release the master rfkill
switch.

>> Thanks. Please take note that rfkill will, right now, try to BLOCK all
>> radios on suspend. That will be changed soon (2.6.28 at the latest), and
>> your driver will have to handle blocking radios on suspend directly if it is
>> needed for toshibas.
>
> Why is this necessary? Doesn't the radio power down as part of the suspend
> process? How would I tell what the hardware is doing?

Some network drivers want to do it themselves, so now it will be the job of
every driver to know whether it should do something or not.

To test your platform to know if you will need to do something, just comment
the rfkill->toggle_radio in rfkill_suspend, and check if your radios are
still transmitting when you suspend. I really doubt any laptop has that
problem, though.

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

Subject: Re: [PATCH 1/1] toshiba_acpi: Add support for bluetooth toggling through rfkill (v2)

On Sun, 03 Aug 2008, Philip Langdale wrote:
>>> +static void bt_acpi_notify(acpi_handle handle, u32 event, void *data)
>>> +{
>>> + struct toshiba_acpi_dev *dev = data;
>>> +
>>> + switch (event) {
>>> + case BT_ACPI_SOFT_UNBLOCKED_EVENT:
>>> + if (!dev->ignore_next_bt_event) {
>>> + bt_rfkill_toggle_radio(data, RFKILL_STATE_UNBLOCKED);
>>> + rfkill_force_state(dev->rfk_dev,
>>> + RFKILL_STATE_UNBLOCKED);
>>
>> This one got me confused. Why do you need that bt_rfkill_toggle_radio call
>> here?
>
> When you turn off the hardware kill switch, the hardware will not reactivate
> the bluetooth device. it just returns to the SOFT_UNBLOCKED state. I put that
> in so that it would turn the device back on - a generally more desirable
> behaviour - otherwise the user has to dig around for a software way to turn
> it back. All the other hardware I've ever seen (including the wifi device on
> this laptop) turns it back on, so it seemed sensible to try and make it work
> as people would expect.

Well, you really shouldn't be doing things this way. It hardcodes policy on
something that is for either rfkill-input or for userspace to decide.

I think I got the picture of how it works in toshiba, it is similar to the
thinkpads.

If I got it right, you have

(1) a input device (a hardware kill switch)
(2) a rfkill controller for bluetooth

And the firmware causes (1) to force-disable (2), but loses state when it
does so and leaves it disabled when (1) is not forcing the state to disable
anymore.

Well, the textbook way to connect that to rfkill is something like this,
please check if it makes sense:

Export (1) as a input device (you have events to hook to when it changes
state) or polled input device (you don't have said events). Do NOT register
(1) with a struct rfkill at all. It is purely an input device.

DO NOT expose (2) as an input device at all. Instead, register it to a
rfkill struct, of type bluetooth.

On the event handler for (1), you issue the EV_SW SW_RFKILL_ALL input event.
Since you *do* know events from (1) are likely to also have messed with the
state of (2), you also check (2)'s state at this time and update it through
rfkill_force_state().

Due to the interaction of 1 and 2, you need to implement and deal with
RFKILL_STATE_HARD_BLOCKED. Since you do know the firmware will be
hard-blocking (2) when (1) is active, you return RFKILL_STATE_HARD_BLOCKED
for (2)'s state every time (1) is active. Otherwise, you return
RFKILL_STATE_SOFT_BLOCKED when (2) is blocked, and RFKILL_STATE_UNBLOCKED
otherwise.

This will cause the state of (2) to go to either RFKILL_STATE_HARD_BLOCKED
or RFKILL_STATE_SOFT_BLOCKED when (1) changes state.

[Note: the above does assume (1) blocks (2) in some way you cannot override,
and that trying to unblock (2) while (1) is blocking it is futile].

rfkill-input (now) or userspace (someday) will take care of kicking the
radio to RFKILL_STATE_UNBLOCKED when (1) issues an event that signals that
radios don't have to remain blocked. Maybe this is why you see the WLAN
going on when you deactivate the radio kill switch?

And rfkill-input will soon be enhanced to let the user configure it to do
something different if he wants. Your driver doesn't (and shouldn't)
hardcode policy about it.


>> Read the kernel-doc headers of every rfkill function you call at least
>> once... Never rfkill_free() something you rfkill_unregister()'ed.
>>
>> rfkill_free() is just for the error unwind of a failure between
>> rfkill_allocate() and rfkill_register().
>
> Fair enough, but it doesn't help that rfkill and input-polldev work in
> exactly opposite ways; polldev requires you to unregister and then free;
> some consistency wouldn't hurt.

That's just how the code was when I started enhancing it, you can ask Ivo
(the rfkill maintainer) about the reasons behind it if you want, or you
could try to submit a patch to Ivo changing rfkill (and every driver using
rfkill) to rfkill_free() after rfkill_unregister().

>>> + toshiba_acpi.rfk_dev->dev.class->suspend = NULL;
>>> + toshiba_acpi.rfk_dev->dev.class->resume = NULL;
>>
>> Why?
>
> Good question. Gone.

Thanks. Please take note that rfkill will, right now, try to BLOCK all
radios on suspend. That will be changed soon (2.6.28 at the latest), and
your driver will have to handle blocking radios on suspend directly if it is
needed for toshibas.

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

Subject: Re: [PATCH 1/1] toshiba_acpi: Add support for bluetooth toggling through rfkill (v2)

On Sat, 02 Aug 2008, Philip Langdale wrote:
> +static int bt_rfkill_toggle_radio(void *data, enum rfkill_state state)
> +{
> + u32 result1, result2;
> + u32 value;
> + bool radio_state;
> +
> + struct toshiba_acpi_dev *dev = data;
> +
> + value = state == RFKILL_STATE_UNBLOCKED;

value = (state == RFKILL_STATE_UNBLOCKED);

It is a lot easier to read without confusing the == for a =.

> + case RFKILL_STATE_HARD_BLOCKED:
> + printk(MY_NOTICE "Ignored HARD_BLOCKED request by software\n");
> + break;

You don't really need the above, rfkill won't ever call your toggle_radio
callback like that.

If you want paranoid checking, do this instead:

default:
/* maybe WARN(), WARN_ON() or printk here */
return -EINVAL;

> +static void bt_acpi_notify(acpi_handle handle, u32 event, void *data)
> +{
> + struct toshiba_acpi_dev *dev = data;
> +
> + switch (event) {
> + case BT_ACPI_SOFT_UNBLOCKED_EVENT:
> + if (!dev->ignore_next_bt_event) {
> + bt_rfkill_toggle_radio(data, RFKILL_STATE_UNBLOCKED);
> + rfkill_force_state(dev->rfk_dev,
> + RFKILL_STATE_UNBLOCKED);

This one got me confused. Why do you need that bt_rfkill_toggle_radio call
here?

> + if (toshiba_acpi.rfk_dev) {
> + rfkill_unregister(toshiba_acpi.rfk_dev);
> + rfkill_free(toshiba_acpi.rfk_dev);
> + }

Read the kernel-doc headers of every rfkill function you call at least
once... Never rfkill_free() something you rfkill_unregister()'ed.

rfkill_free() is just for the error unwind of a failure between
rfkill_allocate() and rfkill_register().

> + toshiba_acpi.rfk_dev->dev.class->suspend = NULL;
> + toshiba_acpi.rfk_dev->dev.class->resume = NULL;

Why?

> + ret = rfkill_register(toshiba_acpi.rfk_dev);
> + if (ret) {
> + printk(MY_ERR "unable to register rfkill device\n");
> + toshiba_acpi_exit();
> + return -ENOMEM;
> + }
> +
> + if (hci_get_bt_on(&bt_on) == HCI_SUCCESS && bt_on) {
> + toshiba_acpi.rfk_dev->state = RFKILL_STATE_UNBLOCKED;
> + } else if (hci_get_radio_state(&radio_on) == HCI_SUCCESS && radio_on) {
> + toshiba_acpi.rfk_dev->state = RFKILL_STATE_HARD_BLOCKED;
> + } else {
> + toshiba_acpi.rfk_dev->state = RFKILL_STATE_SOFT_BLOCKED;
> + }

Do the above between rfkill_allocate() and rfkill_register().

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

Subject: Re: [PATCH 1/1] toshiba_acpi: Add support for bluetooth toggling through rfkill (v2)

On Mon, 11 Aug 2008, Philip Langdale wrote:
> Henrique de Moraes Holschuh wrote:
>> Hmm? UNBLOCKED means it is not being rfkilled. Why is not working? Put
>> some debug printks on your toggle_radio callback, that might help fix the
>> issue...
>
> Well, what I see is this:
>
> At modprobe, I get an explicit unblock request. When I modify 'state' in sysfs,
> I see appropriate requests to soft block and unblock. When I activate the hardware
> kill switch, the rfkill state goes to hard_block but no one calls toggle_radio
> and when I release the kill switch, the state goes to soft_block and no one
> calls toggle_radio.

Looks like either rfkill-input is inactive, or something is binding to the
toshiba_acpi input device and setting it to exclusive mode (X evdev does
this).

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

Subject: Re: [PATCH 1/1] toshiba_acpi: Add support for bluetooth toggling through rfkill (v2)

On Wed, 06 Aug 2008, Philip Langdale wrote:
> Henrique de Moraes Holschuh wrote:
>> Well, the textbook way to connect that to rfkill is something like this,
>> please check if it makes sense:
>>
>> Export (1) as a input device (you have events to hook to when it changes
>> state) or polled input device (you don't have said events). Do NOT register
>> (1) with a struct rfkill at all. It is purely an input device.
>>
>> DO NOT expose (2) as an input device at all. Instead, register it to a
>> rfkill struct, of type bluetooth.
>>
>> On the event handler for (1), you issue the EV_SW SW_RFKILL_ALL input event.
>> Since you *do* know events from (1) are likely to also have messed with the
>> state of (2), you also check (2)'s state at this time and update it through
>> rfkill_force_state().
>>
>> Due to the interaction of 1 and 2, you need to implement and deal with
>> RFKILL_STATE_HARD_BLOCKED. Since you do know the firmware will be
>> hard-blocking (2) when (1) is active, you return RFKILL_STATE_HARD_BLOCKED
>> for (2)'s state every time (1) is active. Otherwise, you return
>> RFKILL_STATE_SOFT_BLOCKED when (2) is blocked, and RFKILL_STATE_UNBLOCKED
>> otherwise.
>>
>> This will cause the state of (2) to go to either RFKILL_STATE_HARD_BLOCKED
>> or RFKILL_STATE_SOFT_BLOCKED when (1) changes state.
>>
>> [Note: the above does assume (1) blocks (2) in some way you cannot override,
>> and that trying to unblock (2) while (1) is blocking it is futile].
>>
>> rfkill-input (now) or userspace (someday) will take care of kicking the
>> radio to RFKILL_STATE_UNBLOCKED when (1) issues an event that signals that
>> radios don't have to remain blocked. Maybe this is why you see the WLAN
>> going on when you deactivate the radio kill switch?
>>
>> And rfkill-input will soon be enhanced to let the user configure it to do
>> something different if he wants. Your driver doesn't (and shouldn't)
>> hardcode policy about it.
>
> Hmm. So, I've updated the diff to do this, but rfkill-input is not kicking
> the bluetooth device back on after I release the kill switch. it just returns
> to SOFT_UNBLOCKED and stays there.

Hmm? UNBLOCKED means it is not being rfkilled. Why is not working? Put
some debug printks on your toggle_radio callback, that might help fix the
issue...

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

2008-08-06 03:33:39

by Philip Langdale

[permalink] [raw]
Subject: Re: [PATCH 1/1] toshiba_acpi: Add support for bluetooth toggling through rfkill (v2)

Henrique de Moraes Holschuh wrote:
>
> rfkill-input (now) or userspace (someday) will take care of kicking the
> radio to RFKILL_STATE_UNBLOCKED when (1) issues an event that signals that
> radios don't have to remain blocked. Maybe this is why you see the WLAN
> going on when you deactivate the radio kill switch?

It's all done behind the scenes I think (it's an ipw2200 device). There's
no rfkill integration from that driver.

> And rfkill-input will soon be enhanced to let the user configure it to do
> something different if he wants. Your driver doesn't (and shouldn't)
> hardcode policy about it.

Ok, that makes things much easier for me :-) But it means that for now the
user will have to manually kick the device.

>
> Thanks. Please take note that rfkill will, right now, try to BLOCK all
> radios on suspend. That will be changed soon (2.6.28 at the latest), and
> your driver will have to handle blocking radios on suspend directly if it is
> needed for toshibas.

Why is this necessary? Doesn't the radio power down as part of the suspend
process? How would I tell what the hardware is doing?

I'll update the diff when I can.

--phil

2008-08-16 19:22:54

by Philip Langdale

[permalink] [raw]
Subject: Re: [PATCH 1/1] toshiba_acpi: Add support for bluetooth toggling through rfkill (v2)

Philip Langdale wrote:
>
> Now, when I release the kill switch, rfkill-input does indeed turn the
> radio on. Great. The remaining problem is that rfkill-input is setting
> it to soft-blocked when the switch is registered. I'm not sure why it's
> deciding to do that. Anyway, I'm almost there. Thanks.

Finally sorted it out. Updated diff is posted as v5.

--phil

2008-08-07 05:05:18

by Philip Langdale

[permalink] [raw]
Subject: Re: [PATCH 1/1] toshiba_acpi: Add support for bluetooth toggling through rfkill (v2)

Henrique de Moraes Holschuh wrote:
>
> Well, the textbook way to connect that to rfkill is something like this,
> please check if it makes sense:
>
> Export (1) as a input device (you have events to hook to when it changes
> state) or polled input device (you don't have said events). Do NOT register
> (1) with a struct rfkill at all. It is purely an input device.
>
> DO NOT expose (2) as an input device at all. Instead, register it to a
> rfkill struct, of type bluetooth.
>
> On the event handler for (1), you issue the EV_SW SW_RFKILL_ALL input event.
> Since you *do* know events from (1) are likely to also have messed with the
> state of (2), you also check (2)'s state at this time and update it through
> rfkill_force_state().
>
> Due to the interaction of 1 and 2, you need to implement and deal with
> RFKILL_STATE_HARD_BLOCKED. Since you do know the firmware will be
> hard-blocking (2) when (1) is active, you return RFKILL_STATE_HARD_BLOCKED
> for (2)'s state every time (1) is active. Otherwise, you return
> RFKILL_STATE_SOFT_BLOCKED when (2) is blocked, and RFKILL_STATE_UNBLOCKED
> otherwise.
>
> This will cause the state of (2) to go to either RFKILL_STATE_HARD_BLOCKED
> or RFKILL_STATE_SOFT_BLOCKED when (1) changes state.
>
> [Note: the above does assume (1) blocks (2) in some way you cannot override,
> and that trying to unblock (2) while (1) is blocking it is futile].
>
> rfkill-input (now) or userspace (someday) will take care of kicking the
> radio to RFKILL_STATE_UNBLOCKED when (1) issues an event that signals that
> radios don't have to remain blocked. Maybe this is why you see the WLAN
> going on when you deactivate the radio kill switch?
>
> And rfkill-input will soon be enhanced to let the user configure it to do
> something different if he wants. Your driver doesn't (and shouldn't)
> hardcode policy about it.

Hmm. So, I've updated the diff to do this, but rfkill-input is not kicking
the bluetooth device back on after I release the kill switch. it just returns
to SOFT_UNBLOCKED and stays there.

--phil

2008-08-11 16:41:52

by Philip Langdale

[permalink] [raw]
Subject: Re: [PATCH 1/1] toshiba_acpi: Add support for bluetooth toggling through rfkill (v2)

Henrique de Moraes Holschuh wrote:
>
> Hmm? UNBLOCKED means it is not being rfkilled. Why is not working? Put
> some debug printks on your toggle_radio callback, that might help fix the
> issue...
>

Well, what I see is this:

At modprobe, I get an explicit unblock request. When I modify 'state' in sysfs,
I see appropriate requests to soft block and unblock. When I activate the hardware
kill switch, the rfkill state goes to hard_block but no one calls toggle_radio
and when I release the kill switch, the state goes to soft_block and no one
calls toggle_radio.

--phil

2008-08-14 06:58:58

by Philip Langdale

[permalink] [raw]
Subject: Re: [PATCH 1/1] toshiba_acpi: Add support for bluetooth toggling through rfkill (v2)

Henrique de Moraes Holschuh wrote:
>
> Looks like either rfkill-input is inactive, or something is binding to the
> toshiba_acpi input device and setting it to exclusive mode (X evdev does
> this).

Almost. Turns out there were two problems:

1) I was setting SW_RFKILL_ALL as a keybit instead of an swbit - so no
events reported. Whoops. :-)

2) Maybe this shouldn't surprise me, but the expected switch semantics
are that '1' means radios are on and 0 means they are killed - I thought
it was the other way round.

Now, when I release the kill switch, rfkill-input does indeed turn the
radio on. Great. The remaining problem is that rfkill-input is setting
it to soft-blocked when the switch is registered. I'm not sure why it's
deciding to do that. Anyway, I'm almost there. Thanks.

--phil