Skip to contentSkip to conversation

ACPI: compal-laptop: use rfkill switch subsystem

PATCH In review Cezary Jackiewicz 3 participants 7 messages Jul 2, 2008 – Jul 3, 2008

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ACPI: compal-laptop: use rfkill switch subsystem

From: Cezary Jackiewicz <[email protected]>

Removing unnecessary attributes, use rfkill switch subsystem.

It depends on the rfkill changes in net-next-2.6.
As thinkpad-acpi, please stage these patches for merging during the 2.6.27 merge window after the rfkill changes land in mainline.

---

drivers/misc/compal-laptop.c +126 -152
diff -Nuar a/drivers/misc/compal-laptop.c b/drivers/misc/compal-laptop.c
@@ -24,19 +24,10 @@
  */
 
 /*
- * comapl-laptop.c - Compal laptop support.
+ * compal-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/.
+ * This driver registers itself in the Linux backlight control subsystem
+ * and rfkill switch 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,8 +43,12 @@
 #include <linux/backlight.h>
 #include <linux/platform_device.h>
 #include <linux/autoconf.h>
+#include <linux/rfkill.h>
 
-#define COMPAL_DRIVER_VERSION "0.2.6"
+#define COMPAL_DRIVER_VERSION "0.3.0"
+#define COMPAL_DRIVER_NAME "compal-laptop"
+#define COMPAL_ERR KERN_ERR COMPAL_DRIVER_NAME ": "
+#define COMPAL_INFO KERN_INFO COMPAL_DRIVER_NAME ": "
 
 #define COMPAL_LCD_LEVEL_MAX 8
 
@@ -64,6 +59,10 @@
 #define WLAN_MASK	0x01
 #define BT_MASK		0x02
 
+/* rfkill switches */
+static struct rfkill *bluetooth_rfkill;
+static struct rfkill *wlan_rfkill;
+
 static int force;
 module_param(force, bool, 0);
 MODULE_PARM_DESC(force, "Force driver load, ignore DMI data");
@@ -89,67 +88,6 @@
 	return (int) result;
 }
 
-static int set_wlan_state(int state)
-{
-	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);
-	}
-
-	return 0;
-}
-
-static int set_bluetooth_state(int state)
-{
-	u8 result, value;
-
-	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;
-}
-
-static int get_wireless_state(int *wlan, int *bluetooth)
-{
-	u8 result;
-
-	ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
-
-	if (wlan) {
-		if ((result & KILLSWITCH_MASK) == 0)
-			*wlan = 0;
-		else
-			*wlan = result & WLAN_MASK;
-	}
-
-	if (bluetooth) {
-		if ((result & KILLSWITCH_MASK) == 0)
-			*bluetooth = 0;
-		else
-			*bluetooth = (result & BT_MASK) >> 1;
-	}
-
-	return 0;
-}
-
 /* Backlight device stuff */
 
 static int bl_get_brightness(struct backlight_device *b)
@@ -172,99 +110,121 @@
 
 /* Platform device */
 
-static ssize_t show_wlan(struct device *dev,
-	struct device_attribute *attr, char *buf)
-{
-	int ret, enabled;
+static struct platform_driver compal_driver = {
+	.driver = {
+		.name = COMPAL_DRIVER_NAME,
+		.owner = THIS_MODULE,
+	}
+};
 
-	ret = get_wireless_state(&enabled, NULL);
-	if (ret < 0)
-		return ret;
+static struct platform_device *compal_device;
 
-	return sprintf(buf, "%i\n", enabled);
-}
+/* rfkill stuff */
 
-static ssize_t show_raw(struct device *dev,
-	struct device_attribute *attr, char *buf)
+static int wlan_rfk_set(void *data, enum rfkill_state state)
 {
-	u8 result;
+	u8 result, value;
 
 	ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
 
-	return sprintf(buf, "%i\n", result);
+	if ((result & KILLSWITCH_MASK) == 0)
+		return 0;
+	else {
+		if (state == RFKILL_STATE_ON)
+			value = (u8) (result | WLAN_MASK);
+		else
+			value = (u8) (result & ~WLAN_MASK);
+		ec_write(COMPAL_EC_COMMAND_WIRELESS, value);
+	}
+
+	return 0;
 }
 
-static ssize_t show_bluetooth(struct device *dev,
-	struct device_attribute *attr, char *buf)
+static int wlan_rfk_get(void *data, enum rfkill_state *state)
 {
-	int ret, enabled;
+	u8 result;
 
-	ret = get_wireless_state(NULL, &enabled);
-	if (ret < 0)
-		return ret;
+	ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
 
-	return sprintf(buf, "%i\n", enabled);
+	if ((result & KILLSWITCH_MASK) == 0)
+		*state = RFKILL_STATE_OFF;
+	else
+		*state = result & WLAN_MASK;
+
+	return 0;
 }
 
-static ssize_t store_wlan_state(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t count)
+static int bluetooth_rfk_set(void *data, enum rfkill_state state)
 {
-	int state, ret;
+	u8 result, value;
 
-	if (sscanf(buf, "%i", &state) != 1 || (state < 0 || state > 1))
-		return -EINVAL;
+	ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
 
-	ret = set_wlan_state(state);
-	if (ret < 0)
-		return ret;
+	if ((result & KILLSWITCH_MASK) == 0)
+		return 0;
+	else {
+		if (state == RFKILL_STATE_ON)
+			value = (u8) (result | BT_MASK);
+		else
+			value = (u8) (result & ~BT_MASK);
+		ec_write(COMPAL_EC_COMMAND_WIRELESS, value);
+	}
 
-	return count;
+	return 0;
 }
 
-static ssize_t store_bluetooth_state(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t count)
+static int bluetooth_rfk_get(void *data, enum rfkill_state *state)
 {
-	int state, ret;
+	u8 result;
 
-	if (sscanf(buf, "%i", &state) != 1 || (state < 0 || state > 1))
-		return -EINVAL;
+	ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
 
-	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
-};
+	if ((result & KILLSWITCH_MASK) == 0)
+		*state = RFKILL_STATE_OFF;
+	else
+		*state = (result & BT_MASK) >> 1;
 
-static struct attribute_group compal_attribute_group = {
-	.attrs = compal_attributes
-};
+	return 0;
+}
 
-static struct platform_driver compal_driver = {
-	.driver = {
-		.name = "compal-laptop",
-		.owner = THIS_MODULE,
+static int __init compal_rfkill(struct rfkill **rfk,
+				const enum rfkill_type rfktype,
+				const char *name,
+				int (*toggle_radio)(void *, enum rfkill_state),
+				int (*get_state)(void *, enum rfkill_state *))
+{
+	int res;
+
+	(*rfk) = rfkill_allocate(&compal_device->dev, rfktype);
+	if (!*rfk) {
+		printk(COMPAL_ERR
+				"failed to allocate memory for rfkill class\n");
+		return -ENOMEM;
 	}
-};
 
-static struct platform_device *compal_device;
+	(*rfk)->name = name;
+	(*rfk)->get_state = get_state;
+	(*rfk)->toggle_radio = toggle_radio;
+	(*rfk)->user_claim_unsupported = 1;
+
+	res = rfkill_register(*rfk);
+	if (res < 0) {
+		printk(COMPAL_ERR
+				"failed to register %s rfkill switch: %d\n",
+				name, res);
+		rfkill_free(*rfk);
+		*rfk = NULL;
+		return res;
+	}
+
+	return 0;
+}
 
 /* Initialization */
 
 static int dmi_check_cb(const struct dmi_system_id *id)
 {
-	printk(KERN_INFO "compal-laptop: Identified laptop model '%s'.\n",
+	printk(COMPAL_INFO "Identified laptop model '%s'.\n",
 		id->ident);
 
 	return 0;
@@ -326,12 +286,13 @@
 
 	/* Register backlight stuff */
 
-	compalbl_device = backlight_device_register("compal-laptop", NULL, NULL,
-						&compalbl_ops);
+	compalbl_device = backlight_device_register(COMPAL_DRIVER_NAME,
+						NULL, NULL, &compalbl_ops);
 	if (IS_ERR(compalbl_device))
 		return PTR_ERR(compalbl_device);
 
 	compalbl_device->props.max_brightness = COMPAL_LCD_LEVEL_MAX-1;
+	compalbl_device->props.brightness = get_lcd_level();
 
 	ret = platform_driver_register(&compal_driver);
 	if (ret)
@@ -339,7 +300,7 @@
 
 	/* Register platform stuff */
 
-	compal_device = platform_device_alloc("compal-laptop", -1);
+	compal_device = platform_device_alloc(COMPAL_DRIVER_NAME, -1);
 	if (!compal_device) {
 		ret = -ENOMEM;
 		goto fail_platform_driver;
@@ -347,23 +308,28 @@
 
 	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);
-	if (ret)
-		goto fail_platform_device2;
-
-	printk(KERN_INFO "compal-laptop: driver "COMPAL_DRIVER_VERSION
-		" successfully loaded.\n");
+	/* Register rfkill stuff */
 
-	return 0;
+	compal_rfkill(&wlan_rfkill,
+					RFKILL_TYPE_WLAN,
+					"compal_laptop_wlan_sw",
+					wlan_rfk_set,
+					wlan_rfk_get);
+
+	compal_rfkill(&bluetooth_rfkill,
+					RFKILL_TYPE_BLUETOOTH,
+					"compal_laptop_bluetooth_sw",
+					bluetooth_rfk_set,
+					bluetooth_rfk_get);
 
-fail_platform_device2:
+	printk(COMPAL_INFO "driver "COMPAL_DRIVER_VERSION
+			" successfully loaded.\n");
 
-	platform_device_del(compal_device);
+	return 0;
 
-fail_platform_device1:
+fail_platform_device:
 
 	platform_device_put(compal_device);
 
@@ -380,13 +346,21 @@
 
 static void __exit compal_cleanup(void)
 {
+	if (bluetooth_rfkill) {
+		rfkill_unregister(bluetooth_rfkill);
+		bluetooth_rfkill = NULL;
+	}
+
+	if (wlan_rfkill) {
+		rfkill_unregister(wlan_rfkill);
+		wlan_rfkill = NULL;
+	}
 
-	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);
 
-	printk(KERN_INFO "compal-laptop: driver unloaded.\n");
+	printk(COMPAL_INFO "driver unloaded.\n");
 }
 
 module_init(compal_init);
drivers/misc/Kconfig +1 -0
diff -Nuar a/drivers/misc/Kconfig b/drivers/misc/Kconfig
@@ -224,6 +224,7 @@
 	depends on X86
 	depends on ACPI_EC
 	depends on BACKLIGHT_CLASS_DEVICE
+	depends on RFKILL
         ---help---
 	  This is a driver for laptops built by Compal:
 
#2 lore raw
In reply to Cezary Jackiewicz
Quoted 3 lines · Cezary Jackiewicz

Removing unnecessary attributes, use rfkill switch subsystem.

It depends on the rfkill changes in net-next-2.6.

[...]

Quoted 10 lines
drivers/misc/compal-laptop.c +4 -2
+static int wlan_rfk_set(void *data, enum rfkill_state state)
 {
-	u8 result;
+	u8 result, value;
 
 	ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
 
-	return sprintf(buf, "%i\n", result);
+	if ((result & KILLSWITCH_MASK) == 0)
+		return 0;

drivers/misc/compal-laptop.c line 141

Quoted 10 lines
drivers/misc/compal-laptop.c +9 -0
+	else {
+		if (state == RFKILL_STATE_ON)
+			value = (u8) (result | WLAN_MASK);
+		else
+			value = (u8) (result & ~WLAN_MASK);
+		ec_write(COMPAL_EC_COMMAND_WIRELESS, value);
+	}
+
+	return 0;
 }

That doesn't look like a new-style (i.e. what is in net-next-2.6) rfkill support. Could you take a look on the new (and I sure hope, vastly improved) Documentation/rfkill.txt in net-next-2.6, and switch the driver to RFKILL_STATE_UNBLOCKED, RFKILL_STATE_SOFT_BLOCKED, etc? After a quick look, it looks like Compal won't need RFKILL_STATE_HARD_BLOCKED (thinkpad-acpi does, because some thinkpads have a master wireless kill switch), but still...

The new documentation describes how your rfk_get and rfk_set methods should operate, etc. So it is a good idea to read it all, and then check if what the driver is doing matches it.

drivers/misc/compal-laptop.c line 208

Quoted 4 lines
drivers/misc/compal-laptop.c +4 -0
+	(*rfk)->name = name;
+	(*rfk)->get_state = get_state;
+	(*rfk)->toggle_radio = toggle_radio;
+	(*rfk)->user_claim_unsupported = 1;

Better update (*rfk)->state here, too. Right now, rfkill won't do it for you (maybe it should, as there is a get_state method. But it doesn't, yet).

BTW, if it is a read/write rfkill controller, why do you set user_claim_unsupported to true? Just asking, this is not necessarily an error, but it looks odd to me.

"user-claiming" a rfkill controller just means stuff like rfkill-input are to stay clear from it (except for emergency power-off commands). The hardware and firmware can still toggle it behind the kernel's back.

drivers/misc/compal-laptop.c line 290

Quoted 4 lines
drivers/misc/compal-laptop.c +2 -2
-	compalbl_device = backlight_device_register("compal-laptop", NULL, NULL,
-						&compalbl_ops);
+	compalbl_device = backlight_device_register(COMPAL_DRIVER_NAME,
+						NULL, NULL, &compalbl_ops);

Frankly, I'd prefer if you had done this kind of cleanup in a previous, separate patch. Easier to read and review if we are looking just at rfkill changes, just at cleanups, just at backlight changes... etc.

drivers/misc/compal-laptop.c line 295

Quoted 2 lines
drivers/misc/compal-laptop.c +1 -0
 	compalbl_device->props.max_brightness = COMPAL_LCD_LEVEL_MAX-1;
+	compalbl_device->props.brightness = get_lcd_level();

See above.

BTW, it is a good idea to Cc linux-wireless on rfkill patches, and you *really* should be CCing the rfkill maintainer on any patches dealing with rfkill IMHO. That way, you get more reviews, rfkill isn't (unfortunately) easy to use.

Signature · 4 lines
  "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
#3 lore raw

Dnia 2008-07-02, o godz. 23:14:33
Henrique de Moraes Holschuh <[email protected]> napisał(a):

[...]

Quoted 7 lines

That doesn't look like a new-style (i.e. what is in net-next-2.6) rfkill support. Could you take a look on the new (and I sure hope, vastly improved) Documentation/rfkill.txt in net-next-2.6, and switch the driver to RFKILL_STATE_UNBLOCKED, RFKILL_STATE_SOFT_BLOCKED, etc? After a quick look, it looks like Compal won't need RFKILL_STATE_HARD_BLOCKED (thinkpad-acpi does, because some thinkpads have a master wireless kill switch), but still...

Compal has hardware wireless kill switch (without software it kill all radios rf and disconnecting bluetooth from usb bus). Does he should generate SW_RFKILL_ALL or something like this?

Signature · 5 lines
 Cezary Jackiewicz
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#4 lore raw
Quoted 12 lines · Cezary Jackiewicz

Dnia 2008-07-02, o godz. 23:14:33
Henrique de Moraes Holschuh <[email protected]> napisa?(a):

[...]

Quoted 7 lines

That doesn't look like a new-style (i.e. what is in net-next-2.6) rfkill support. Could you take a look on the new (and I sure hope, vastly improved) Documentation/rfkill.txt in net-next-2.6, and switch the driver to RFKILL_STATE_UNBLOCKED, RFKILL_STATE_SOFT_BLOCKED, etc? After a quick look, it looks like Compal won't need RFKILL_STATE_HARD_BLOCKED (thinkpad-acpi does, because some thinkpads have a master wireless kill switch), but still...

Quoted 3 lines

Compal has hardware wireless kill switch (without software it kill all radios rf and disconnecting bluetooth from usb bus). Does he should generate SW_RFKILL_ALL or something like this?

Look at the thinkpad-acpi code. I don't know for sure if Compal's firmware/hardware acts like the thinkpad one, but if it does, you will find your answer on thinkpad-acpi since what you describe is pretty much what a thinkpad does.

So, yes, usually master wireless switches like what you describe for Compal are input devices (see the new rfkill docs), and issue SW_RFKILL_ALL events.

Signature · 4 lines
  "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
#5 lore raw
In reply to Henrique de Moraes Holschuh

Dnia 2008-07-02, o godz. 23:14:33
Henrique de Moraes Holschuh <[email protected]> napisał(a):

Quoted 4 lines

BTW, it is a good idea to Cc linux-wireless on rfkill patches, and you *really* should be CCing the rfkill maintainer on any patches dealing with rfkill IMHO. That way, you get more reviews, rfkill isn't (unfortunately) easy to use.

Is there any reason why rfkill_unregister switches off radio rather than leaving it in the current status?

Signature · 6 lines
 Cezary Jackiewicz
 
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#6 lore raw
Quoted 9 lines · Cezary Jackiewicz

Dnia 2008-07-02, o godz. 23:14:33
Henrique de Moraes Holschuh <[email protected]> napisa?(a):

Quoted 4 lines

BTW, it is a good idea to Cc linux-wireless on rfkill patches, and you *really* should be CCing the rfkill maintainer on any patches dealing with rfkill IMHO. That way, you get more reviews, rfkill isn't (unfortunately) easy to use.

Is there any reason why rfkill_unregister switches off radio rather than leaving it in the current status?

Probably the same reason why backlight powers off the backlight, etc. We basically try to power down and secure what we can when we are going out and locking the door ;-)

Ivo?

Signature · 4 lines
  "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