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

[Henrique de Moraes Holschuh <hmh@hmh.eng.br>]

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