On 12/8/09, Jes Sorensen <jes.sorensen@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
