Signed-off-by: Mario Limonciello <[email protected]>
Reviewed-by: Alan Jenkins <[email protected]>
---
drivers/platform/x86/compal-laptop.c | 87 +++++++++++++++++++++++++++++++++-
1 files changed, 85 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/x86/compal-laptop.c b/drivers/platform/x86/compal-laptop.c
index c1c8c03..da7ead6 100644
--- a/drivers/platform/x86/compal-laptop.c
+++ b/drivers/platform/x86/compal-laptop.c
@@ -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,77 @@ static int get_lcd_level(void)
return (int) result;
}
+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 (!blocked)
+ value = (u8) (result | radio);
+ else
+ value = (u8) (result & ~radio);
+ ec_write(COMPAL_EC_COMMAND_WIRELESS, value);
+
+ return 0;
+}
+
+static void compal_rfkill_poll(struct rfkill *rfkill, void *data)
+{
+ u8 result;
+ bool hw_blocked;
+
+ ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
+
+ hw_blocked = !(result & KILLSWITCH_MASK);
+ rfkill_set_hw_state(rfkill, hw_blocked);
+}
+
+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)
+ return -ENOMEM;
+
+ ret = rfkill_register(wifi_rfkill);
+ if (ret)
+ 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_allocate_bt;
+ }
+ ret = rfkill_register(bt_rfkill);
+ if (ret)
+ goto err_register_bt;
+
+ return 0;
+
+err_register_bt:
+ rfkill_destroy(bt_rfkill);
+
+err_allocate_bt:
+ rfkill_unregister(wifi_rfkill);
+
+err_wifi:
+ rfkill_destroy(wifi_rfkill);
+
+ return ret;
+}
+
static int set_wlan_state(int state)
{
u8 result, value;
@@ -258,8 +334,6 @@ static struct platform_driver compal_driver = {
}
};
-static struct platform_device *compal_device;
-
/* Initialization */
static int dmi_check_cb(const struct dmi_system_id *id)
@@ -397,11 +471,16 @@ static int __init compal_init(void)
if (ret)
goto fail_platform_device2;
+ ret = setup_rfkill();
+ if (ret)
+ goto fail_rfkill;
+
printk(KERN_INFO "compal-laptop: driver "COMPAL_DRIVER_VERSION
" successfully loaded.\n");
return 0;
+fail_rfkill:
fail_platform_device2:
platform_device_del(compal_device);
@@ -428,6 +507,10 @@ static void __exit compal_cleanup(void)
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");
}
--
1.6.3.3
On 8/21/09, Mario Limonciello <[email protected]> wrote:
> This drops the support for manually groking the files in sysfs
> to turn on and off the WLAN and BT for Compal laptops in favor
> of platform rfkill support.
>
> It has been combined into a single patch to not introduce regressions
> in the process of simply adding rfkill support.
>
> Signed-off-by: Mario Limonciello <[email protected]>
> @@ -390,23 +314,19 @@ static int __init compal_init(void)
>
> ret = platform_device_add(compal_device);
> if (ret)
> - goto fail_platform_device1;
> + goto fail_platform_device;
>
> - ret = sysfs_create_group(&compal_device->dev.kobj,
> - &compal_attribute_group);
> + ret = setup_rfkill();
> if (ret)
> - goto fail_platform_device2;
> + goto fail_rfkill;
>
> printk(KERN_INFO "compal-laptop: driver "COMPAL_DRIVER_VERSION
> " successfully loaded.\n");
>
> return 0;
>
> -fail_platform_device2:
> -
> - platform_device_del(compal_device);
> -
> -fail_platform_device1:
> +fail_rfkill:
> +fail_platform_device:
>
> platform_device_put(compal_device);
>
I guess our mails crossed. As in the previous patch I think
platform_device_del() should still be called, in the fail_rfkill case.
Previously platform_device_del() was required after
platform_device_add(), if we bailed out because of a failure in
sysfs_create_group(). I don't think that requirement is removed by
replacing sysfs_create_group() with setup_rfkill().
Regards
Alan
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
This drops the support for manually groking the files in sysfs
to turn on and off the WLAN and BT for Compal laptops in favor
of platform rfkill support.
It has been combined into a single patch to not introduce regressions
in the process of simply adding rfkill support.
Signed-off-by: Mario Limonciello <[email protected]>
Reviewed-by: Alan Jenkins <[email protected]>
---
drivers/platform/x86/compal-laptop.c | 203 +++++++++++-----------------------
1 files changed, 63 insertions(+), 140 deletions(-)
diff --git a/drivers/platform/x86/compal-laptop.c b/drivers/platform/x86/compal-laptop.c
index c1c8c03..2c1ff8f 100644
--- a/drivers/platform/x86/compal-laptop.c
+++ b/drivers/platform/x86/compal-laptop.c
@@ -26,17 +26,8 @@
/*
* comapl-laptop.c - Compal laptop support.
*
- * This driver exports a few files in /sys/devices/platform/compal-laptop/:
- *
- * wlan - wlan subsystem state: contains 0 or 1 (rw)
- *
- * bluetooth - Bluetooth subsystem state: contains 0 or 1 (rw)
- *
- * raw - raw value taken from embedded controller register (ro)
- *
- * In addition to these platform device attributes the driver
- * registers itself in the Linux backlight control subsystem and is
- * available to userspace under /sys/class/backlight/compal-laptop/.
+ * The driver registers itself with the rfkill subsystem and
+ * the Linux backlight control subsystem.
*
* This driver might work on other laptops produced by Compal. If you
* want to try it you can pass force=1 as argument to the module which
@@ -52,6 +43,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 +56,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,65 +85,75 @@ static int get_lcd_level(void)
return (int) result;
}
-static int set_wlan_state(int state)
+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;
- else {
- if (state)
- value = (u8) (result | WLAN_MASK);
- else
- value = (u8) (result & ~WLAN_MASK);
- ec_write(COMPAL_EC_COMMAND_WIRELESS, value);
- }
+ if (!blocked)
+ value = (u8) (result | radio);
+ else
+ value = (u8) (result & ~radio);
+ ec_write(COMPAL_EC_COMMAND_WIRELESS, value);
return 0;
}
-static int set_bluetooth_state(int state)
+static void compal_rfkill_poll(struct rfkill *rfkill, void *data)
{
- u8 result, value;
+ u8 result;
+ bool hw_blocked;
ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
- if ((result & KILLSWITCH_MASK) == 0)
- return -EINVAL;
- else {
- if (state)
- value = (u8) (result | BT_MASK);
- else
- value = (u8) (result & ~BT_MASK);
- ec_write(COMPAL_EC_COMMAND_WIRELESS, value);
- }
-
- return 0;
+ hw_blocked = !(result & KILLSWITCH_MASK);
+ rfkill_set_hw_state(rfkill, hw_blocked);
}
-static int get_wireless_state(int *wlan, int *bluetooth)
+static const struct rfkill_ops compal_rfkill_ops = {
+ .poll = compal_rfkill_poll,
+ .set_block = compal_rfkill_set,
+};
+
+static int setup_rfkill(void)
{
- u8 result;
+ int ret;
- ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
+ wifi_rfkill = rfkill_alloc("compal-wifi", &compal_device->dev,
+ RFKILL_TYPE_WLAN, &compal_rfkill_ops,
+ (void *) WLAN_MASK);
+ if (!wifi_rfkill)
+ return -ENOMEM;
- if (wlan) {
- if ((result & KILLSWITCH_MASK) == 0)
- *wlan = 0;
- else
- *wlan = result & WLAN_MASK;
- }
+ ret = rfkill_register(wifi_rfkill);
+ if (ret)
+ goto err_wifi;
- if (bluetooth) {
- if ((result & KILLSWITCH_MASK) == 0)
- *bluetooth = 0;
- else
- *bluetooth = (result & BT_MASK) >> 1;
+ 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_allocate_bt;
}
+ ret = rfkill_register(bt_rfkill);
+ if (ret)
+ goto err_register_bt;
return 0;
+
+err_register_bt:
+ rfkill_destroy(bt_rfkill);
+
+err_allocate_bt:
+ rfkill_unregister(wifi_rfkill);
+
+err_wifi:
+ rfkill_destroy(wifi_rfkill);
+
+ return ret;
}
/* Backlight device stuff */
@@ -170,86 +176,6 @@ static struct backlight_ops compalbl_ops = {
static struct backlight_device *compalbl_device;
-/* Platform device */
-
-static ssize_t show_wlan(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- int ret, enabled;
-
- ret = get_wireless_state(&enabled, NULL);
- if (ret < 0)
- return ret;
-
- return sprintf(buf, "%i\n", enabled);
-}
-
-static ssize_t show_raw(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- u8 result;
-
- ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
-
- return sprintf(buf, "%i\n", result);
-}
-
-static ssize_t show_bluetooth(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- int ret, enabled;
-
- ret = get_wireless_state(NULL, &enabled);
- if (ret < 0)
- return ret;
-
- return sprintf(buf, "%i\n", enabled);
-}
-
-static ssize_t store_wlan_state(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t count)
-{
- int state, ret;
-
- if (sscanf(buf, "%i", &state) != 1 || (state < 0 || state > 1))
- return -EINVAL;
-
- ret = set_wlan_state(state);
- if (ret < 0)
- return ret;
-
- return count;
-}
-
-static ssize_t store_bluetooth_state(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t count)
-{
- int state, ret;
-
- if (sscanf(buf, "%i", &state) != 1 || (state < 0 || state > 1))
- return -EINVAL;
-
- ret = set_bluetooth_state(state);
- if (ret < 0)
- return ret;
-
- return count;
-}
-
-static DEVICE_ATTR(bluetooth, 0644, show_bluetooth, store_bluetooth_state);
-static DEVICE_ATTR(wlan, 0644, show_wlan, store_wlan_state);
-static DEVICE_ATTR(raw, 0444, show_raw, NULL);
-
-static struct attribute *compal_attributes[] = {
- &dev_attr_bluetooth.attr,
- &dev_attr_wlan.attr,
- &dev_attr_raw.attr,
- NULL
-};
-
-static struct attribute_group compal_attribute_group = {
- .attrs = compal_attributes
-};
static struct platform_driver compal_driver = {
.driver = {
@@ -258,8 +184,6 @@ static struct platform_driver compal_driver = {
}
};
-static struct platform_device *compal_device;
-
/* Initialization */
static int dmi_check_cb(const struct dmi_system_id *id)
@@ -390,23 +314,19 @@ static int __init compal_init(void)
ret = platform_device_add(compal_device);
if (ret)
- goto fail_platform_device1;
+ goto fail_platform_device;
- ret = sysfs_create_group(&compal_device->dev.kobj,
- &compal_attribute_group);
+ ret = setup_rfkill();
if (ret)
- goto fail_platform_device2;
+ goto fail_rfkill;
printk(KERN_INFO "compal-laptop: driver "COMPAL_DRIVER_VERSION
" successfully loaded.\n");
return 0;
-fail_platform_device2:
-
- platform_device_del(compal_device);
-
-fail_platform_device1:
+fail_rfkill:
+fail_platform_device:
platform_device_put(compal_device);
@@ -424,10 +344,13 @@ fail_backlight:
static void __exit compal_cleanup(void)
{
- sysfs_remove_group(&compal_device->dev.kobj, &compal_attribute_group);
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");
}
--
1.6.3.3
This drops the support for manually groking the files in sysfs
to turn on and off the WLAN and BT for Compal laptops in favor
of platform rfkill support.
It has been combined into a single patch to not introduce regressions
in the process of simply adding rfkill support
Signed-off-by: Mario Limonciello <[email protected]>
Reviewed-by: Alan Jenkins <[email protected]>
---
drivers/platform/x86/compal-laptop.c | 201 +++++++++++-----------------------
1 files changed, 63 insertions(+), 138 deletions(-)
diff --git a/drivers/platform/x86/compal-laptop.c b/drivers/platform/x86/compal-laptop.c
index c1c8c03..9b1cc45 100644
--- a/drivers/platform/x86/compal-laptop.c
+++ b/drivers/platform/x86/compal-laptop.c
@@ -26,17 +26,8 @@
/*
* comapl-laptop.c - Compal laptop support.
*
- * This driver exports a few files in /sys/devices/platform/compal-laptop/:
- *
- * wlan - wlan subsystem state: contains 0 or 1 (rw)
- *
- * bluetooth - Bluetooth subsystem state: contains 0 or 1 (rw)
- *
- * raw - raw value taken from embedded controller register (ro)
- *
- * In addition to these platform device attributes the driver
- * registers itself in the Linux backlight control subsystem and is
- * available to userspace under /sys/class/backlight/compal-laptop/.
+ * The driver registers itself with the rfkill subsystem and
+ * the Linux backlight control subsystem.
*
* This driver might work on other laptops produced by Compal. If you
* want to try it you can pass force=1 as argument to the module which
@@ -52,6 +43,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 +56,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,65 +85,75 @@ static int get_lcd_level(void)
return (int) result;
}
-static int set_wlan_state(int state)
+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;
- else {
- if (state)
- value = (u8) (result | WLAN_MASK);
- else
- value = (u8) (result & ~WLAN_MASK);
- ec_write(COMPAL_EC_COMMAND_WIRELESS, value);
- }
+ if (!blocked)
+ value = (u8) (result | radio);
+ else
+ value = (u8) (result & ~radio);
+ ec_write(COMPAL_EC_COMMAND_WIRELESS, value);
return 0;
}
-static int set_bluetooth_state(int state)
+static void compal_rfkill_poll(struct rfkill *rfkill, void *data)
{
- u8 result, value;
+ u8 result;
+ bool hw_blocked;
ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
- if ((result & KILLSWITCH_MASK) == 0)
- return -EINVAL;
- else {
- if (state)
- value = (u8) (result | BT_MASK);
- else
- value = (u8) (result & ~BT_MASK);
- ec_write(COMPAL_EC_COMMAND_WIRELESS, value);
- }
-
- return 0;
+ hw_blocked = !(result & KILLSWITCH_MASK);
+ rfkill_set_hw_state(rfkill, hw_blocked);
}
-static int get_wireless_state(int *wlan, int *bluetooth)
+static const struct rfkill_ops compal_rfkill_ops = {
+ .poll = compal_rfkill_poll,
+ .set_block = compal_rfkill_set,
+};
+
+static int setup_rfkill(void)
{
- u8 result;
+ int ret;
- ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
+ wifi_rfkill = rfkill_alloc("compal-wifi", &compal_device->dev,
+ RFKILL_TYPE_WLAN, &compal_rfkill_ops,
+ (void *) WLAN_MASK);
+ if (!wifi_rfkill)
+ return -ENOMEM;
- if (wlan) {
- if ((result & KILLSWITCH_MASK) == 0)
- *wlan = 0;
- else
- *wlan = result & WLAN_MASK;
- }
+ ret = rfkill_register(wifi_rfkill);
+ if (ret)
+ goto err_wifi;
- if (bluetooth) {
- if ((result & KILLSWITCH_MASK) == 0)
- *bluetooth = 0;
- else
- *bluetooth = (result & BT_MASK) >> 1;
+ 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_allocate_bt;
}
+ ret = rfkill_register(bt_rfkill);
+ if (ret)
+ goto err_register_bt;
return 0;
+
+err_register_bt:
+ rfkill_destroy(bt_rfkill);
+
+err_allocate_bt:
+ rfkill_unregister(wifi_rfkill);
+
+err_wifi:
+ rfkill_destroy(wifi_rfkill);
+
+ return ret;
}
/* Backlight device stuff */
@@ -170,86 +176,6 @@ static struct backlight_ops compalbl_ops = {
static struct backlight_device *compalbl_device;
-/* Platform device */
-
-static ssize_t show_wlan(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- int ret, enabled;
-
- ret = get_wireless_state(&enabled, NULL);
- if (ret < 0)
- return ret;
-
- return sprintf(buf, "%i\n", enabled);
-}
-
-static ssize_t show_raw(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- u8 result;
-
- ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
-
- return sprintf(buf, "%i\n", result);
-}
-
-static ssize_t show_bluetooth(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- int ret, enabled;
-
- ret = get_wireless_state(NULL, &enabled);
- if (ret < 0)
- return ret;
-
- return sprintf(buf, "%i\n", enabled);
-}
-
-static ssize_t store_wlan_state(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t count)
-{
- int state, ret;
-
- if (sscanf(buf, "%i", &state) != 1 || (state < 0 || state > 1))
- return -EINVAL;
-
- ret = set_wlan_state(state);
- if (ret < 0)
- return ret;
-
- return count;
-}
-
-static ssize_t store_bluetooth_state(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t count)
-{
- int state, ret;
-
- if (sscanf(buf, "%i", &state) != 1 || (state < 0 || state > 1))
- return -EINVAL;
-
- ret = set_bluetooth_state(state);
- if (ret < 0)
- return ret;
-
- return count;
-}
-
-static DEVICE_ATTR(bluetooth, 0644, show_bluetooth, store_bluetooth_state);
-static DEVICE_ATTR(wlan, 0644, show_wlan, store_wlan_state);
-static DEVICE_ATTR(raw, 0444, show_raw, NULL);
-
-static struct attribute *compal_attributes[] = {
- &dev_attr_bluetooth.attr,
- &dev_attr_wlan.attr,
- &dev_attr_raw.attr,
- NULL
-};
-
-static struct attribute_group compal_attribute_group = {
- .attrs = compal_attributes
-};
static struct platform_driver compal_driver = {
.driver = {
@@ -258,8 +184,6 @@ static struct platform_driver compal_driver = {
}
};
-static struct platform_device *compal_device;
-
/* Initialization */
static int dmi_check_cb(const struct dmi_system_id *id)
@@ -390,23 +314,21 @@ static int __init compal_init(void)
ret = platform_device_add(compal_device);
if (ret)
- goto fail_platform_device1;
+ goto fail_platform_device;
- ret = sysfs_create_group(&compal_device->dev.kobj,
- &compal_attribute_group);
+ ret = setup_rfkill();
if (ret)
- goto fail_platform_device2;
+ goto fail_rfkill;
printk(KERN_INFO "compal-laptop: driver "COMPAL_DRIVER_VERSION
" successfully loaded.\n");
return 0;
-fail_platform_device2:
-
+fail_rfkill:
platform_device_del(compal_device);
-fail_platform_device1:
+fail_platform_device:
platform_device_put(compal_device);
@@ -424,10 +346,13 @@ fail_backlight:
static void __exit compal_cleanup(void)
{
- sysfs_remove_group(&compal_device->dev.kobj, &compal_attribute_group);
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");
}
--
1.6.3.3
On 8/19/09, Mario Limonciello <[email protected]> wrote:
> Johannes:
>
> Johannes Berg wrote:
>> On Wed, 2009-08-19 at 13:36 -0500, Mario Limonciello wrote:
>>
>>
>>
>>
>> Isn't that missing sysfs_remove_group()?
>>
>> johannes
>>
> The third patch in the (updated) series is dropping the sysfs bits, so
> sysfs_remove_group is removed there.
That's not ideal. Each patch should stand on its own; it's bad form
to introduce a bug in one patch and fix it in the next one. Even
something obscure like omitting to free the sysfs group when
setup_rfkill() fails.
I would suggest merging these two patches into one. That would avoid
adding sysfs_remove_group() in this patch, just to remove it in the
next one. It also avoids the question in this patch, of what happens
to the rfkill interface if you write to the sysfs file.
Alan
> Also, I'm not entirely clear about the semantics -- you've called the
> bit KILLSWITCH_MASK, but does it really control all technologies as a
> hard block, i.e. it toggles both the bluetooth and wireless hard block?
In compal's laptop - yes. Switch disables all radios, bluetooth and wifi.
--
Cezary
Hi Johannes:
Thanks for looking.
Johannes Berg wrote:
> Ah, heh, thanks Alan for pointing out there was a patch here :)
>
>
>
> I don't quite understand the "| radio" bit since that seems to be the
> soft kill bit according to rfkill_set()?
>
Yeah you're right, this bit was unnecessary. I pulled it out.
> Anyhow, here you reject the request to set the soft bit. I suspect you
> could let it go through but it would only change the soft bit in the
> BIOS, nothing else really.
>
> Two options:
> 1) You can let it go though, in that case do that, and remove the sw
> block stuff from poll() completely.
>
> 2) You can't let it go through. In this case, you need to leave set as
> it is, but implement poll like this:
>
> sw_block = rfkill_set_hw_state(rfkill, hw_blocked);
> compal_rfkill_set(data, sw_block);
>
> so that when the user soft-blocks the device while hard-blocked, the
> soft block is still honoured after pushing the button on the laptop.
>
>
OK, the second option sounds more desirable, so I've implemented that.
--
Mario Limonciello
*Dell | Linux Engineering*
[email protected]
On 8/18/09, Mario Limonciello <[email protected]> wrote:
> Hi Guys:
Hi again Mario
> Alan Jenkins wrote:
>
>> ... 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
>>
> I think I've addressed this properly now and only go through each of the
> error handlers as necessary.
Yes, that looks better. I'm still a bit confused about poll(), I'll
have to leave that for Johannes to verify. Feel free to add
Reviewed-by: Alan Jenkins <[email protected]>
Regards
Alan
On 8/18/09, Mario Limonciello <[email protected]> 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
Johannes:
Johannes Berg wrote:
> Hi Mario,
>
> First, let me say I agree with Alan, the option 1 is more desirable if
> possible to do with the hardware. But this looks ok from an rfkill POV
> now, except there's a small bug:
>
>
It looks like option 1 works properly on my hardware, so I've switched
the other code around.
>
> That doesn't error out, so
>
>
>
> this will crash without NULL checks.
>
> (and you have to explicitly assign NULL in setup_rfkill() too, when
> bluetooth fails and wifi is freed)
>
>
OK, i've cleaned that up to just error out on the module if rfkill
doesn't get initialized right.
> johannes
>
I've resent separately, and think I have git send-email working, so
hopefully won't have to attach in the future.
--
Mario Limonciello
*Dell | Linux Engineering*
[email protected]
On 8/19/09, Mario Limonciello <[email protected]> wrote:
> Hi Johannes:
>
> Thanks for looking.
>
> Johannes Berg wrote:
>> Ah, heh, thanks Alan for pointing out there was a patch here :)
>>
>>
>>
>> I don't quite understand the "| radio" bit since that seems to be the
>> soft kill bit according to rfkill_set()?
>>
> Yeah you're right, this bit was unnecessary. I pulled it out.
>> Anyhow, here you reject the request to set the soft bit. I suspect you
>> could let it go through but it would only change the soft bit in the
>> BIOS, nothing else really.
>>
>> Two options:
>> 1) You can let it go though, in that case do that, and remove the sw
>> block stuff from poll() completely.
>>
>> 2) You can't let it go through. In this case, you need to leave set as
>> it is, but implement poll like this:
>>
>> sw_block = rfkill_set_hw_state(rfkill, hw_blocked);
>> compal_rfkill_set(data, sw_block);
>>
>> so that when the user soft-blocks the device while hard-blocked, the
>> soft block is still honoured after pushing the button on the laptop.
>>
>>
> OK, the second option sounds more desirable, so I've implemented that.
I think the first option is more _desirable_, it's more a matter of
whether it can work well on this hardware.
In case 2), the radio will be unblocked for a short period between the
button-press, and the next poll() call. But 1) won't work if the
hardware "forgets" the soft block when the hard block is toggled on
and off.
Regards
Alan
On Wed, 2009-08-19 at 13:36 -0500, Mario Limonciello wrote:
> Signed-off-by: Mario Limonciello <[email protected]>
> Reviewed-by: Alan Jenkins <[email protected]>
> + ret = setup_rfkill();
> + if (ret)
> + goto fail_rfkill;
> +
> printk(KERN_INFO "compal-laptop: driver "COMPAL_DRIVER_VERSION
> " successfully loaded.\n");
>
> return 0;
>
> +fail_rfkill:
> fail_platform_device2:
Isn't that missing sysfs_remove_group()?
johannes
Hi Guys:
Johannes Berg wrote:
> Hi everyone,
>
> 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().
>
>
No it doesn't, so i've followed your advice in an updated patch, Thanks.
Alan Jenkins wrote:
> ... 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
>
I think I've addressed this properly now and only go through each of the error handlers as necessary.
--
Mario Limonciello
*Dell | Linux Engineering*
[email protected]
On 8/24/09, Mario Limonciello <[email protected]> wrote:
> This drops the support for manually groking the files in sysfs
> to turn on and off the WLAN and BT for Compal laptops in favor
> of platform rfkill support.
>
> It has been combined into a single patch to not introduce regressions
> in the process of simply adding rfkill support
Ok, that looks better.
Thanks
Alan
Hi Mario,
First, let me say I agree with Alan, the option 1 is more desirable if
possible to do with the hardware. But this looks ok from an rfkill POV
now, except there's a small bug:
> + ret = setup_rfkill();
> + if (ret)
> + printk(KERN_WARNING "compal-laptop: Unable to setup
> rfkill\n");
That doesn't error out, so
> + rfkill_unregister(wifi_rfkill);
> + rfkill_destroy(wifi_rfkill);
> + rfkill_unregister(bt_rfkill);
> + rfkill_destroy(bt_rfkill);
this will crash without NULL checks.
(and you have to explicitly assign NULL in setup_rfkill() too, when
bluetooth fails and wifi is freed)
johannes
Johannes:
Johannes Berg wrote:
> On Wed, 2009-08-19 at 13:36 -0500, Mario Limonciello wrote:
>
>
>
>
> Isn't that missing sysfs_remove_group()?
>
> johannes
>
The third patch in the (updated) series is dropping the sysfs bits, so
sysfs_remove_group is removed there.
--
Mario Limonciello
*Dell | Linux Engineering*
[email protected]
Ah, heh, thanks Alan for pointing out there was a patch here :)
> +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 | radio));
I don't quite understand the "| radio" bit since that seems to be the
soft kill bit according to rfkill_set()?
> + sw_blocked = rfkill_set_hw_state(rfkill, hw_blocked);
> +
> + rfkill_set_sw_state(rfkill, sw_blocked);
This is wrong. You can remove the entire part about sw_blocked, almost.
> +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;
Anyhow, here you reject the request to set the soft bit. I suspect you
could let it go through but it would only change the soft bit in the
BIOS, nothing else really.
Two options:
1) You can let it go though, in that case do that, and remove the sw
block stuff from poll() completely.
2) You can't let it go through. In this case, you need to leave set as
it is, but implement poll like this:
sw_block = rfkill_set_hw_state(rfkill, hw_blocked);
compal_rfkill_set(data, sw_block);
so that when the user soft-blocks the device while hard-blocked, the
soft block is still honoured after pushing the button on the laptop.
Also, I'm not entirely clear about the semantics -- you've called the
bit KILLSWITCH_MASK, but does it really control all technologies as a
hard block, i.e. it toggles both the bluetooth and wireless hard block?
johannes