Re: [PATCH 2/3] Add rfkill support to compal-laptop

Re: [PATCH 2/3] Add rfkill support to compal-laptop

[Alan Jenkins]

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

Re: [PATCH 2/3] Add rfkill support to compal-laptop

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1194 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/3] Add rfkill support to compal-laptop

[Mario Limonciello]

[-- Attachment #1.1: Type: text/plain, Size: 846 bytes --]

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*
mario_limonciello@dell.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 02_add_rfkill_support.diff --]
[-- Type: text/x-patch; name="02_add_rfkill_support.diff", Size: 3274 bytes --]

--- compal-laptop.c.old	2009-08-18 05:23:39.668669312 -0500
+++ compal-laptop.c	2009-08-18 05:49:00.098015155 -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,84 @@
 	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 | radio));
+	sw_blocked = rfkill_set_hw_state(rfkill, hw_blocked);
+
+	rfkill_set_sw_state(rfkill, sw_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)
+		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 +341,6 @@
 	}
 };
 
-static struct platform_device *compal_device;
-
 /* Initialization */
 
 static int dmi_check_cb(const struct dmi_system_id *id)
@@ -397,6 +478,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");
 
@@ -428,6 +513,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");
 }

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [PATCH 2/3] Add rfkill support to compal-laptop

[Alan Jenkins]

On 8/18/09, Mario Limonciello <mario_limonciello@dell.com> 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 <alan-jenkins@tuffmail.co.uk>

Regards
Alan

Re: [PATCH 2/3] Add rfkill support to compal-laptop

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1898 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/3] Add rfkill support to compal-laptop

[Cezary Jackiewicz]

> 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

Re: [PATCH 2/3] Add rfkill support to compal-laptop

[Mario Limonciello]

[-- Attachment #1.1: Type: text/plain, Size: 1164 bytes --]

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*
mario_limonciello@dell.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 02_add_rfkill_support.diff --]
[-- Type: text/x-patch; name="02_add_rfkill_support.diff", Size: 3952 bytes --]

From 5f5dc9c1adf041418c6dd273cd4ee83d5ae96e74 Mon Sep 17 00:00:00 2001
From: Mario Limonciello <Mario_Limonciello@Dell.com>
Date: Wed, 19 Aug 2009 11:41:27 -0500
Subject: [PATCH 2/3] Add rfkill support to compal-laptop

Signed-off-by: Mario Limonciello <Mario_Limonciello@Dell.com>
Reviewed-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
---
 drivers/platform/x86/compal-laptop.c |   91 +++++++++++++++++++++++++++++++++-
 1 files changed, 89 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/compal-laptop.c b/drivers/platform/x86/compal-laptop.c
index 11003bb..d997de5 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,82 @@ 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 ((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 void compal_rfkill_poll(struct rfkill *rfkill, void *data)
+{
+	u8 result;
+	bool hw_blocked;
+	bool sw_blocked;
+
+	ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
+
+	hw_blocked = !(result & KILLSWITCH_MASK);
+	sw_blocked = rfkill_set_hw_state(rfkill, hw_blocked);
+	compal_rfkill_set(data,sw_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 +339,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)
@@ -356,6 +435,10 @@ static int __init compal_init(void)
 	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");
 
@@ -387,6 +470,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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [PATCH 2/3] Add rfkill support to compal-laptop

[Alan Jenkins]

On 8/19/09, Mario Limonciello <mario_limonciello@dell.com> 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

Re: [PATCH 2/3] Add rfkill support to compal-laptop

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 685 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/3] Add rfkill support to compal-laptop

[Mario Limonciello]

[-- Attachment #1: Type: text/plain, Size: 862 bytes --]

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*
mario_limonciello@dell.com


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[PATCH 2/3] Add rfkill support to compal-laptop

[Mario Limonciello]

Signed-off-by: Mario Limonciello <Mario_Limonciello@Dell.com>
Reviewed-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
---
 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

Re: [PATCH 2/3] Add rfkill support to compal-laptop

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 484 bytes --]

On Wed, 2009-08-19 at 13:36 -0500, Mario Limonciello wrote:
> Signed-off-by: Mario Limonciello <Mario_Limonciello@Dell.com>
> Reviewed-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

> +	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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/3] Add rfkill support to compal-laptop

[Mario Limonciello]

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

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*
mario_limonciello@dell.com


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [PATCH 2/3] Add rfkill support to compal-laptop

[Alan Jenkins]

On 8/19/09, Mario Limonciello <mario_limonciello@dell.com> 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