Return-path: Received: from out1.smtp.messagingengine.com ([66.111.4.25]:55159 "EHLO out1.smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751008AbYHCE0U (ORCPT ); Sun, 3 Aug 2008 00:26:20 -0400 Date: Sun, 3 Aug 2008 01:26:14 -0300 From: Henrique de Moraes Holschuh To: Philip Langdale Cc: LKML , Matthew Garrett , toshiba_acpi@memebeam.org, Ivo van Doorn , linux-wireless@vger.kernel.org Subject: Re: [PATCH 1/1] toshiba_acpi: Add support for bluetooth toggling through rfkill (v2) Message-ID: <20080803042613.GC6053@khazad-dum.debian.net> (sfid-20080803_062639_635856_A00EE496) References: <4894B1B4.6050003@overt.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <4894B1B4.6050003@overt.org> Sender: linux-wireless-owner@vger.kernel.org List-ID: 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