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
On 12/4/09, Jes Sorensen <[email protected]> wrote:
> 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
Cool. This must be the shortest ACPI driver :-).
Try to send patches inline if possible, it makes them easier to review.
>
> +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.
>
> +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
>
> +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;
...
> +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.
> +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.
>
> + 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.
> + 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
On Tue, Dec 08, 2009 at 10:05:19AM +0000, Alan Jenkins wrote:
> 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 :-).
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.
> 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.
--
Matthew Garrett | [email protected]
On 12/08/09 13:55, Matthew Garrett wrote:
> 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
On Tue, Dec 08, 2009 at 04:45:05PM +0100, Jes Sorensen wrote:
> On 12/08/09 13:55, Matthew Garrett wrote:
>> 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.
--
Matthew Garrett | [email protected]
On 12/08/09 16:47, Matthew Garrett wrote:
> On Tue, Dec 08, 2009 at 04:45:05PM +0100, Jes Sorensen wrote:
>> 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
On Tue, Dec 08, 2009 at 04:50:06PM +0100, Jes Sorensen wrote:
> 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.
--
Matthew Garrett | [email protected]
On 12/08/2009 11:05 AM, Alan Jenkins wrote:
> Cool. This must be the shortest ACPI driver :-).
Hi Alan,
Here's an updated version that addresses most of your comments.
> Try to send patches inline if possible, it makes them easier to review.
Not possible with Thunderbug, sorry.
>> +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.
> Please also set
> .owner = THIS_MODULE,
Good point, thanks!
>> +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.
>> + status = acpi_evaluate_integer(device->handle, "_STA", NULL,
>> + &bt_present);
>
> I think this would benefit from an explanation.
Comments added.
>> + 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.
>
>> + 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
On 12/8/09, Matthew Garrett <[email protected]> wrote:
> On Tue, Dec 08, 2009 at 04:50:06PM +0100, Jes Sorensen wrote:
>
>> 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
On 12/8/09, Jes Sorensen <[email protected]> wrote:
> On 12/08/2009 11:05 AM, Alan Jenkins wrote:
>> Cool. This must be the shortest ACPI driver :-).
>
> Hi Alan,
>
> Here's an updated version that addresses most of your comments.
Hi again
>> 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.
>> 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()).
>>> + status = acpi_evaluate_integer(device->handle, "_STA", NULL,
>>> + &bt_present);
>>
>> I think this would benefit from an explanation.
>
> Comments added.
Thanks.
> +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).
>>> + 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.
>>> + 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
On Wed, Dec 09, 2009 at 01:17:48PM +0000, Alan Jenkins wrote:
> On 12/8/09, Jes Sorensen <[email protected]> wrote:
> > On 12/08/2009 11:05 AM, Alan Jenkins wrote:
> >> Cool. This must be the shortest ACPI driver :-).
> >
> > Hi Alan,
> >
> > Here's an updated version that addresses most of your comments.
>
> Hi again
>
> >> 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.
>
There is a Thunderbird section under Documentation/email-clients.txt
regards,
dan carpenter
On 12/10/09, Dan Carpenter <[email protected]> wrote:
> On Wed, Dec 09, 2009 at 01:17:48PM +0000, Alan Jenkins wrote:
>> On 12/8/09, Jes Sorensen <[email protected]> wrote:
>> > On 12/08/2009 11:05 AM, Alan Jenkins wrote:
>> >> Cool. This must be the shortest ACPI driver :-).
>> >
>> > Hi Alan,
>> >
>> > Here's an updated version that addresses most of your comments.
>>
>> Hi again
>>
>> >> 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.
>>
>
> 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
On 12/09/2009 02:17 PM, Alan Jenkins wrote:
>> 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
On 12/10/09, Jes Sorensen <[email protected]> wrote:
> On 12/09/2009 02:17 PM, Alan Jenkins wrote:
>>> 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.
> 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
On 12/10/09 18:00, Alan Jenkins wrote:
> 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
On 12/11/09, Jes Sorensen <[email protected]> wrote:
> On 12/10/09 18:00, Alan Jenkins wrote:
>> 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.
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
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