| Message ID | 4A8AE459.8060102@dell.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On 8/18/09, Mario Limonciello <mario_limonciello@dell.com> wrote: > Hi Alan & Marcel: > > Alan Jenkins wrote: >> Also, you're missing the calls to rfkill_destroy() here. >> >> Whew, I think that's everything. I hope you find the feedback useful, >> despite it being a little fragmented. >> >> > Thanks for all the feedback. I think i've addressed all of the concerns > that were pointed out. I appreciate the pointer to scripts/cleanpatch, > that does significantly help in finding whitespace problems that the > naked eye just browses over. > > I'm attaching the updated patch (sorry, git send-email seems to still > not be very graceful with line breaks when the SMTP implementation is > exchange from what i've seen) > +static void compal_rfkill_poll(struct rfkill *rfkill, void *data) > +{ > + unsigned long radio = (unsigned long) data; > + u8 result; > + bool hw_blocked; > + bool sw_blocked; > + > + ec_read(COMPAL_EC_COMMAND_WIRELESS, &result); > + > + hw_blocked = !(result & KILLSWITCH_MASK); > + sw_blocked = (!hw_blocked && !(result & radio)); > + > + rfkill_set_states(rfkill, sw_blocked, hw_blocked); > +} I assume you have good reason for having sw_block depend on hw_block. I.e. you can't read sw_blocked while hw_blocked is set, right? If KILLSWITCH is toggled on and off, will the hardware "forget" any prior soft-blocks? It would also be nice to know if hardware/firmware ever changes sw_blocked, e.g. in response to a button press. Johannes, I think I'm confusing myself here. Can you have a look at this code? I remember the rfkill rewrite was designed to help with something like this, but I don't know how exactly. Thanks Alan -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/18/09, Mario Limonciello <mario_limonciello@dell.com> wrote: > Hi Alan & Marcel: > > Alan Jenkins wrote: >> Also, you're missing the calls to rfkill_destroy() here. >> >> Whew, I think that's everything. I hope you find the feedback useful, >> despite it being a little fragmented. >> >> > Thanks for all the feedback. I think i've addressed all of the concerns > that were pointed out. I appreciate the pointer to scripts/cleanpatch, > that does significantly help in finding whitespace problems that the > naked eye just browses over. I think you've addressed most of the comments on your first patch. There are still problems with the error handling though. +static int setup_rfkill(void) +{ + int ret; + + wifi_rfkill = rfkill_alloc("compal-wifi", &compal_device->dev, + RFKILL_TYPE_WLAN, &compal_rfkill_ops, + (void *) WLAN_MASK); + if (!wifi_rfkill) { + ret = -ENOMEM; + goto err_wifi; + } + ret = rfkill_register(wifi_rfkill); + if (ret) { + rfkill_unregister(wifi_rfkill); + goto err_wifi; + } If you fail to register an rfkill device, you don't need to unregister that rfkill device... + + bt_rfkill = rfkill_alloc("compal-bluetooth", &compal_device->dev, + RFKILL_TYPE_BLUETOOTH, &compal_rfkill_ops, + (void *) BT_MASK); + if (!bt_rfkill) { + ret = -ENOMEM; + goto err_bt; + } + ret = rfkill_register(bt_rfkill); + if (ret) { + rfkill_unregister(bt_rfkill); + goto err_bt; + } and the same applies here + + return 0; + +err_bt: + rfkill_destroy(bt_rfkill); + ... but you *do* need to unregister wifi_rfkill here, before you go on to destroy it. +err_wifi: + rfkill_destroy(wifi_rfkill); + + return ret; +} Regards Alan -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi everyone, > > I'm attaching the updated patch (sorry, git send-email seems to still > > not be very graceful with line breaks when the SMTP implementation is > > exchange from what i've seen) > > > +static void compal_rfkill_poll(struct rfkill *rfkill, void *data) > > +{ > > + unsigned long radio = (unsigned long) data; > > + u8 result; > > + bool hw_blocked; > > + bool sw_blocked; > > + > > + ec_read(COMPAL_EC_COMMAND_WIRELESS, &result); > > + > > + hw_blocked = !(result & KILLSWITCH_MASK); > > + sw_blocked = (!hw_blocked && !(result & radio)); > > + > > + rfkill_set_states(rfkill, sw_blocked, hw_blocked); > > +} > > I assume you have good reason for having sw_block depend on hw_block. > I.e. you can't read sw_blocked while hw_blocked is set, right? > > If KILLSWITCH is toggled on and off, will the hardware "forget" any > prior soft-blocks? That's a bit strange indeed, but I haven't seen the rest of the code. Does the 'soft block' bit change based on user input, like pressing a button? If not, you shouldn't poll that bit at all, but just set it based on what rfkill gives you as the return value of set_hw_state(). hth, johannes
--- compal-laptop.old 2009-08-18 00:17:43.056669380 -0500 +++ compal-laptop.c 2009-08-18 01:23:33.485833067 -0500 @@ -52,6 +52,7 @@ #include <linux/backlight.h> #include <linux/platform_device.h> #include <linux/autoconf.h> +#include <linux/rfkill.h> #define COMPAL_DRIVER_VERSION "0.2.6" @@ -64,6 +65,10 @@ #define WLAN_MASK 0x01 #define BT_MASK 0x02 +static struct rfkill *wifi_rfkill; +static struct rfkill *bt_rfkill; +static struct platform_device *compal_device; + static int force; module_param(force, bool, 0); MODULE_PARM_DESC(force, "Force driver load, ignore DMI data"); @@ -89,6 +94,86 @@ return (int) result; } +static void compal_rfkill_poll(struct rfkill *rfkill, void *data) +{ + unsigned long radio = (unsigned long) data; + u8 result; + bool hw_blocked; + bool sw_blocked; + + ec_read(COMPAL_EC_COMMAND_WIRELESS, &result); + + hw_blocked = !(result & KILLSWITCH_MASK); + sw_blocked = (!hw_blocked && !(result & radio)); + + rfkill_set_states(rfkill, sw_blocked, hw_blocked); +} + +static int compal_rfkill_set(void *data, bool blocked) +{ + unsigned long radio = (unsigned long) data; + u8 result, value; + + ec_read(COMPAL_EC_COMMAND_WIRELESS, &result); + + if ((result & KILLSWITCH_MASK) == 0) + return -EINVAL; + + if (!blocked) + value = (u8) (result | radio); + else + value = (u8) (result & ~radio); + ec_write(COMPAL_EC_COMMAND_WIRELESS, value); + + return 0; +} + +static const struct rfkill_ops compal_rfkill_ops = { + .poll = compal_rfkill_poll, + .set_block = compal_rfkill_set, +}; + +static int setup_rfkill(void) +{ + int ret; + + wifi_rfkill = rfkill_alloc("compal-wifi", &compal_device->dev, + RFKILL_TYPE_WLAN, &compal_rfkill_ops, + (void *) WLAN_MASK); + if (!wifi_rfkill) { + ret = -ENOMEM; + goto err_wifi; + } + ret = rfkill_register(wifi_rfkill); + if (ret) { + rfkill_unregister(wifi_rfkill); + goto err_wifi; + } + + bt_rfkill = rfkill_alloc("compal-bluetooth", &compal_device->dev, + RFKILL_TYPE_BLUETOOTH, &compal_rfkill_ops, + (void *) BT_MASK); + if (!bt_rfkill) { + ret = -ENOMEM; + goto err_bt; + } + ret = rfkill_register(bt_rfkill); + if (ret) { + rfkill_unregister(bt_rfkill); + goto err_bt; + } + + return 0; + +err_bt: + rfkill_destroy(bt_rfkill); + +err_wifi: + rfkill_destroy(wifi_rfkill); + + return ret; +} + static int set_wlan_state(int state) { u8 result, value; @@ -258,8 +343,6 @@ } }; -static struct platform_device *compal_device; - /* Initialization */ static int dmi_check_cb(const struct dmi_system_id *id) @@ -389,6 +472,10 @@ if (ret) goto fail_platform_device2; + ret = setup_rfkill(); + if (ret) + printk(KERN_WARNING "compal-laptop: Unable to setup rfkill\n"); + printk(KERN_INFO "compal-laptop: driver "COMPAL_DRIVER_VERSION " successfully loaded.\n"); @@ -420,6 +507,10 @@ platform_device_unregister(compal_device); platform_driver_unregister(&compal_driver); backlight_device_unregister(compalbl_device); + rfkill_unregister(wifi_rfkill); + rfkill_destroy(wifi_rfkill); + rfkill_unregister(bt_rfkill); + rfkill_destroy(bt_rfkill); printk(KERN_INFO "compal-laptop: driver unloaded.\n"); }