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