[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4B1E85A9.3030005@gmail.com>
Date: Tue, 08 Dec 2009 17:58:17 +0100
From: Jes Sorensen <jes.sorensen@...il.com>
To: Alan Jenkins <sourcejedi.lkml@...glemail.com>
CC: linux-acpi@...r.kernel.org,
linux-kernel <linux-kernel@...r.kernel.org>, lenb@...nel.org,
Matthew Garrett <mjg59@...f.ucam.org>
Subject: Toshiba Bluetooth enabler (v2) (was: Re: [PATCH] Toshiba Bluetooth
enabler (rfkill))
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
View attachment "0002-toshiba-bluetooth-v2.patch" of type "text/plain" (6412 bytes)
Powered by blists - more mailing lists