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

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

[Mario Limonciello]

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

In order to be useful in modern kernels and standard interfaces,
compal-laptop should have rfkill support.  This patch adds it.
-- 
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: 3512 bytes --]

--- drivers/platform/x86/compal-laptop.c.old	2009-08-17 07:01:36.244874430 -0500
+++ drivers/platform/x86/compal-laptop.c	2009-08-17 07:02:15.012836625 -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,9 @@
 #define WLAN_MASK	0x01
 #define BT_MASK 	0x02
 
+static struct rfkill *wifi_rfkill;
+static struct rfkill *bluetooth_rfkill;
+
 static int force;
 module_param(force, bool, 0);
 MODULE_PARM_DESC(force, "Force driver load, ignore DMI data");
@@ -89,6 +93,87 @@
 	return (int) result;
 }
 
+static void compal_rfkill_query(struct rfkill *rfkill, void *data)
+{
+	unsigned long radio = (unsigned long) data;
+	u8 result;
+	bool blocked;
+
+	ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
+
+	if ((result & KILLSWITCH_MASK) == 0)
+		blocked = 1;
+        else if (radio == WLAN_MASK)
+		blocked = !(result & WLAN_MASK);
+	else
+		blocked = !((result & BT_MASK) >> 1);
+
+	rfkill_set_sw_state(rfkill,blocked);
+	rfkill_set_hw_state(rfkill,0);
+}
+
+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 (!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 = {
+	.set_block = compal_rfkill_set,
+	.query = compal_rfkill_query,
+};
+
+static int setup_rfkill(void)
+{
+	int ret;
+
+	wifi_rfkill = rfkill_alloc("compal-wifi", NULL, RFKILL_TYPE_WLAN,
+					&compal_rfkill_ops, (void *) WLAN_MASK);
+	if (!wifi_rfkill) {
+		ret = -ENOMEM;
+		goto err_wifi;
+	}
+	ret = rfkill_register(wifi_rfkill);
+	if (ret)
+		goto err_wifi;
+
+	bluetooth_rfkill = rfkill_alloc("compal-bluetooth", NULL, RFKILL_TYPE_BLUETOOTH,
+					&compal_rfkill_ops, (void *) BT_MASK);
+	if (!bluetooth_rfkill) {
+		ret = -ENOMEM;
+		goto err_bt;
+	}
+	ret = rfkill_register(bluetooth_rfkill);
+	if (ret)
+		goto err_bt;
+
+	return 0;
+err_bt:
+	rfkill_destroy(bluetooth_rfkill);
+	if (bluetooth_rfkill)
+		rfkill_unregister(bluetooth_rfkill);
+err_wifi:
+	rfkill_destroy(wifi_rfkill);
+	if (wifi_rfkill)
+		rfkill_unregister(wifi_rfkill);
+
+	return ret;
+}
+
 static int set_wlan_state(int state)
 {
 	u8 result, value;
@@ -357,6 +442,12 @@
 	if (!force && !dmi_check_system(compal_dmi_table))
 		return -ENODEV;
 
+	ret = setup_rfkill();
+	if (ret) {
+		printk(KERN_WARNING "compal-laptop: Unable to setup rfkill\n");
+		goto fail_rfkill;
+	}
+
 	/* Register backlight stuff */
 
 	if (!acpi_video_backlight_support()) {
@@ -410,6 +501,13 @@
 
 	backlight_device_unregister(compalbl_device);
 
+fail_rfkill:
+
+	if (wifi_rfkill)
+		rfkill_unregister(wifi_rfkill);
+	if (bluetooth_rfkill)
+		rfkill_unregister(bluetooth_rfkill);
+
 	return ret;
 }
 
@@ -420,6 +518,10 @@
 	platform_device_unregister(compal_device);
 	platform_driver_unregister(&compal_driver);
 	backlight_device_unregister(compalbl_device);
+	if (wifi_rfkill)
+		rfkill_unregister(wifi_rfkill);
+	if (bluetooth_rfkill)
+		rfkill_unregister(bluetooth_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

[Marcel Holtmann]

Hi Mario,

the general rule for patches on LKML is that you send them inline and
not as attachment. A successful workaround for the ones with Exchange
only mail account is to use git send-email.

> --- drivers/platform/x86/compal-laptop.c.old    2009-08-17
> 07:01:36.244874430 -0500
> +++ drivers/platform/x86/compal-laptop.c        2009-08-17
> 07:02:15.012836625 -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,9 @@
>  #define WLAN_MASK      0x01
>  #define BT_MASK        0x02
>  
> +static struct rfkill *wifi_rfkill;
> +static struct rfkill *bluetooth_rfkill;
> +
>  static int force;
>  module_param(force, bool, 0);
>  MODULE_PARM_DESC(force, "Force driver load, ignore DMI data");
> @@ -89,6 +93,87 @@
>         return (int) result;
>  }
>  
> +static void compal_rfkill_query(struct rfkill *rfkill, void *data)
> +{
> +       unsigned long radio = (unsigned long) data;
> +       u8 result;
> +       bool blocked;
> +
> +       ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
> +
> +       if ((result & KILLSWITCH_MASK) == 0)
> +               blocked = 1;
> +        else if (radio == WLAN_MASK)
> +               blocked = !(result & WLAN_MASK);

You are using spaces instead of tabs here.

> +       else
> +               blocked = !((result & BT_MASK) >> 1);
> +
> +       rfkill_set_sw_state(rfkill,blocked);
> +       rfkill_set_hw_state(rfkill,0);
> +}
> +
> +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 (!blocked)
> +                       value = (u8) (result | radio);
> +               else
> +                       value = (u8) (result & ~radio);
> +               ec_write(COMPAL_EC_COMMAND_WIRELESS, value);
> +       }

This else part is utterly stupid. There is no need to have an else
statement. You already left the function. So get rid of it. And at the
same time this code becomes readable.

> +       return 0;
> +}
> +
> +static const struct rfkill_ops compal_rfkill_ops = {
> +       .set_block = compal_rfkill_set,
> +       .query = compal_rfkill_query,
> +};
> +
> +static int setup_rfkill(void)
> +{
> +       int ret;
> +
> +       wifi_rfkill = rfkill_alloc("compal-wifi", NULL,
> RFKILL_TYPE_WLAN,
> +                                       &compal_rfkill_ops, (void *)
> WLAN_MASK);
> +       if (!wifi_rfkill) {
> +               ret = -ENOMEM;
> +               goto err_wifi;
> +       }
> +       ret = rfkill_register(wifi_rfkill);
> +       if (ret)
> +               goto err_wifi;
> +
> +       bluetooth_rfkill = rfkill_alloc("compal-bluetooth", NULL,
> RFKILL_TYPE_BLUETOOTH,
> +                                       &compal_rfkill_ops, (void *)
> BT_MASK);
> +       if (!bluetooth_rfkill) {
> +               ret = -ENOMEM;
> +               goto err_bt;
> +       }
> +       ret = rfkill_register(bluetooth_rfkill);
> +       if (ret)
> +               goto err_bt;
> +
> +       return 0;
> +err_bt:
> +       rfkill_destroy(bluetooth_rfkill);
> +       if (bluetooth_rfkill)
> +               rfkill_unregister(bluetooth_rfkill);
> +err_wifi:
> +       rfkill_destroy(wifi_rfkill);
> +       if (wifi_rfkill)
> +               rfkill_unregister(wifi_rfkill);

I don't understand how this is not a potential NULL pointer dereference.
There might some good luck that the pointer is still valid at that time,
but I highly doubt it. So please unregister before destory.

> +
> +       return ret;
> +}
> +
>  static int set_wlan_state(int state)
>  {
>         u8 result, value;
> @@ -357,6 +442,12 @@
>         if (!force && !dmi_check_system(compal_dmi_table))
>                 return -ENODEV;
>  
> +       ret = setup_rfkill();
> +       if (ret) {
> +               printk(KERN_WARNING "compal-laptop: Unable to setup
> rfkill\n");
> +               goto fail_rfkill;
> +       }
> +
>         /* Register backlight stuff */
>  
>         if (!acpi_video_backlight_support()) {
> @@ -410,6 +501,13 @@
>  
>         backlight_device_unregister(compalbl_device);
>  
> +fail_rfkill:
> +
> +       if (wifi_rfkill)
> +               rfkill_unregister(wifi_rfkill);
> +       if (bluetooth_rfkill)
> +               rfkill_unregister(bluetooth_rfkill);
> +

What non-sense is this. If setup_rfkill() fails, then both RFKILL
switches got unregistered if they ever have been registered.

>         return ret;
>  }
>  
> @@ -420,6 +518,10 @@
>         platform_device_unregister(compal_device);
>         platform_driver_unregister(&compal_driver);
>         backlight_device_unregister(compalbl_device);
> +       if (wifi_rfkill)
> +               rfkill_unregister(wifi_rfkill);
> +       if (bluetooth_rfkill)
> +               rfkill_unregister(bluetooth_rfkill);

Same here. It should never ever succeeded in the first place. You can
call it conditionally.

>  
>         printk(KERN_INFO "compal-laptop: driver unloaded.\n");
>  }
> 

Regards

Marcel

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

[Alan Jenkins]

On 8/18/09, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Mario,

...

>> +static int setup_rfkill(void)
>> +{
>> +       int ret;
>> +
>> +       wifi_rfkill = rfkill_alloc("compal-wifi", NULL,
>> RFKILL_TYPE_WLAN,
>> +                                       &compal_rfkill_ops, (void *)
>> WLAN_MASK);
>> +       if (!wifi_rfkill) {
>> +               ret = -ENOMEM;
>> +               goto err_wifi;
>> +       }
>> +       ret = rfkill_register(wifi_rfkill);
>> +       if (ret)
>> +               goto err_wifi;
>> +
>> +       bluetooth_rfkill = rfkill_alloc("compal-bluetooth", NULL,
>> RFKILL_TYPE_BLUETOOTH,
>> +                                       &compal_rfkill_ops, (void *)
>> BT_MASK);
>> +       if (!bluetooth_rfkill) {
>> +               ret = -ENOMEM;
>> +               goto err_bt;
>> +       }
>> +       ret = rfkill_register(bluetooth_rfkill);
>> +       if (ret)
>> +               goto err_bt;
>> +
>> +       return 0;
>> +err_bt:
>> +       rfkill_destroy(bluetooth_rfkill);
>> +       if (bluetooth_rfkill)
>> +               rfkill_unregister(bluetooth_rfkill);
>> +err_wifi:
>> +       rfkill_destroy(wifi_rfkill);
>> +       if (wifi_rfkill)
>> +               rfkill_unregister(wifi_rfkill);
>
> I don't understand how this is not a potential NULL pointer dereference.
> There might some good luck that the pointer is still valid at that time,
> but I highly doubt it. So please unregister before destory.

Wrong as well :-).

If you fail to register wifi_rfkill, you should *only* call
rfkill_destroy().  So I think it should look like this:

+       if (wifi_rfkill)
+               rfkill_unregister(wifi_rfkill);
+err_wifi:
+       rfkill_destroy(wifi_rfkill);

...

>> @@ -420,6 +518,10 @@
>>         platform_device_unregister(compal_device);
>>         platform_driver_unregister(&compal_driver);
>>         backlight_device_unregister(compalbl_device);
>> +       if (wifi_rfkill)
>> +               rfkill_unregister(wifi_rfkill);
>> +       if (bluetooth_rfkill)
>> +               rfkill_unregister(bluetooth_rfkill);
>
> Same here. It should never ever succeeded in the first place. You can
> call it conditionally.

They're already called conditionally.  I assume you mean unconditionally here.

I agree with all your other comments.  Although I wouldn't call the
return/else style issue stupid, I'd just say it was confused :-).

Regards
Alan

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

[Alan Jenkins]

On 8/18/09, Alan Jenkins <sourcejedi.lkml@googlemail.com> wrote:
> On 8/18/09, Marcel Holtmann <marcel@holtmann.org> wrote:
>> Hi Mario,
>
> ...
>
>>> +static int setup_rfkill(void)
>>> +{
>>> +       int ret;
>>> +
>>> +       wifi_rfkill = rfkill_alloc("compal-wifi", NULL,
>>> RFKILL_TYPE_WLAN,
>>> +                                       &compal_rfkill_ops, (void *)
>>> WLAN_MASK);
>>> +       if (!wifi_rfkill) {
>>> +               ret = -ENOMEM;
>>> +               goto err_wifi;
>>> +       }
>>> +       ret = rfkill_register(wifi_rfkill);
>>> +       if (ret)
>>> +               goto err_wifi;
>>> +
>>> +       bluetooth_rfkill = rfkill_alloc("compal-bluetooth", NULL,
>>> RFKILL_TYPE_BLUETOOTH,
>>> +                                       &compal_rfkill_ops, (void *)
>>> BT_MASK);
>>> +       if (!bluetooth_rfkill) {
>>> +               ret = -ENOMEM;
>>> +               goto err_bt;
>>> +       }
>>> +       ret = rfkill_register(bluetooth_rfkill);
>>> +       if (ret)
>>> +               goto err_bt;
>>> +
>>> +       return 0;
>>> +err_bt:
>>> +       rfkill_destroy(bluetooth_rfkill);
>>> +       if (bluetooth_rfkill)
>>> +               rfkill_unregister(bluetooth_rfkill);
>>> +err_wifi:
>>> +       rfkill_destroy(wifi_rfkill);
>>> +       if (wifi_rfkill)
>>> +               rfkill_unregister(wifi_rfkill);
>>
>> I don't understand how this is not a potential NULL pointer dereference.
>> There might some good luck that the pointer is still valid at that time,
>> but I highly doubt it. So please unregister before destory.
>
> Wrong as well :-).
>
> If you fail to register wifi_rfkill, you should *only* call
> rfkill_destroy().  So I think it should look like this:
>
> +       if (wifi_rfkill)
> +               rfkill_unregister(wifi_rfkill);
> +err_wifi:
> +       rfkill_destroy(wifi_rfkill);
>
> ...
>
>>> @@ -420,6 +518,10 @@
>>>         platform_device_unregister(compal_device);
>>>         platform_driver_unregister(&compal_driver);
>>>         backlight_device_unregister(compalbl_device);
>>> +       if (wifi_rfkill)
>>> +               rfkill_unregister(wifi_rfkill);
>>> +       if (bluetooth_rfkill)
>>> +               rfkill_unregister(bluetooth_rfkill);
>>
>> Same here. It should never ever succeeded in the first place. You can
>> call it conditionally.
>
> They're already called conditionally.  I assume you mean unconditionally
> here.

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.

Regards
Alan

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

[Mario Limonciello]

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

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)
-- 
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: 3289 bytes --]

--- 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");
 }

[-- 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 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: 1184 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-8PEkshWhKlo@public.gmane.org

[-- 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: 4017 bytes --]

From 5f5dc9c1adf041418c6dd273cd4ee83d5ae96e74 Mon Sep 17 00:00:00 2001
From: Mario Limonciello <Mario_Limonciello-DYMqY+WieiM@public.gmane.org>
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-DYMqY+WieiM@public.gmane.org>
Reviewed-by: Alan Jenkins <alan-jenkins-cCz0Lq7MMjm9FHfhHBbuYA@public.gmane.org>
---
 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 --]

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

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

[Alan Jenkins]

On 8/18/09, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Mario,

...

>> +static void compal_rfkill_query(struct rfkill *rfkill, void *data)
>> +{
>> +       unsigned long radio = (unsigned long) data;
>> +       u8 result;
>> +       bool blocked;
>> +
>> +       ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
>> +
>> +       if ((result & KILLSWITCH_MASK) == 0)
>> +               blocked = 1;
>> +        else if (radio == WLAN_MASK)
>> +               blocked = !(result & WLAN_MASK);
>
> You are using spaces instead of tabs here.

Btw Mario, scripts/checkpatch.pl is a good way to check for whitespace
errors and a bunch of other trivia.  So human patch review can
concentrate on the important stuff :-).

Thanks
Alan

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

[Alan Jenkins]

On 8/18/09, Mario Limonciello <mario_limonciello@dell.com> wrote:
> In order to be useful in modern kernels and standard interfaces,
> compal-laptop should have rfkill support.  This patch adds it.
> --
> Mario Limonciello
> *Dell | Linux Engineering*
> mario_limonciello@dell.com
>

I have some comments of my own as well, so here goes.

> +static void compal_rfkill_query(struct rfkill *rfkill, void *data)
> +{
> +	unsigned long radio = (unsigned long) data;
> +	u8 result;
> +	bool blocked;
> +
> +	ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
> +
> +	if ((result & KILLSWITCH_MASK) == 0)
> +		blocked = 1;

> +        else if (radio == WLAN_MASK)
> +		blocked = !(result & WLAN_MASK);
> +	else
> +		blocked = !((result & BT_MASK) >> 1);

Ick, this is overdone.  You should just be able to do

+	else
+		blocked = !(result & radio);

> +	rfkill_set_sw_state(rfkill,blocked);
> +	rfkill_set_hw_state(rfkill,0);

This last line is redundant.  The HW state will default to "unblocked"
if you never set it.

> +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 (!blocked)
> +			value = (u8) (result | radio);
> +		else
> +			value = (u8) (result & ~radio);
> +		ec_write(COMPAL_EC_COMMAND_WIRELESS, value);
> +	}
> +
> +	return 0;
> +}

Note that the return value of rfkill_set is not propagated to
userspace through the /dev/rfkill interface.

The way you use "KILLSWITCH_MASK", it looks like a hard rfkill bit
that covers all radios.  Why don't you report it as a hard block on
each rfkill device?

Note that strictly speaking, you should only call one rfkill_set
function within the query() method.

"@query: query the rfkill block state(s) and call exactly one of the
rfkill_set{,_hw,_sw}_state family of functions."

(I'd recommend reading the comments in include/linux/rfkill.h if you
haven't already).  So If you do want to set both hw and sw states in
query(), you should use rfkill_set_states().

> +static int setup_rfkill(void)
> +{
> +	int ret;
> +
> +	wifi_rfkill = rfkill_alloc("compal-wifi", NULL, RFKILL_TYPE_WLAN,
> +					&compal_rfkill_ops, (void *) WLAN_MASK);

I think you should use "compal_device->dev" in place of NULL here.
Otherwise the rfkill devices will appear in sysfs as "virtual
devices", belonging to no "physical" device.

> +	bluetooth_rfkill = rfkill_alloc("compal-bluetooth", NULL, RFKILL_TYPE_BLUETOOTH,
> +					&compal_rfkill_ops, (void *) BT_MASK);

Same here.

Regards
Alan

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

[Alan Jenkins]

On 8/18/09, Alan Jenkins <sourcejedi.lkml@googlemail.com> wrote:
> On 8/18/09, Mario Limonciello <mario_limonciello@dell.com> wrote:
>> In order to be useful in modern kernels and standard interfaces,
>> compal-laptop should have rfkill support.  This patch adds it.
>> --
>> Mario Limonciello
>> *Dell | Linux Engineering*
>> mario_limonciello@dell.com
>>
>
> I have some comments of my own as well, so here goes.
>
>> +static void compal_rfkill_query(struct rfkill *rfkill, void *data)
>> +{
>> +	unsigned long radio = (unsigned long) data;
>> +	u8 result;
>> +	bool blocked;
>> +
>> +	ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
>> +
>> +	if ((result & KILLSWITCH_MASK) == 0)
>> +		blocked = 1;
>
>> +        else if (radio == WLAN_MASK)
>> +		blocked = !(result & WLAN_MASK);
>> +	else
>> +		blocked = !((result & BT_MASK) >> 1);
>
> Ick, this is overdone.  You should just be able to do
>
> +	else
> +		blocked = !(result & radio);
>
>> +	rfkill_set_sw_state(rfkill,blocked);
>> +	rfkill_set_hw_state(rfkill,0);
>
> This last line is redundant.  The HW state will default to "unblocked"
> if you never set it.
>
>> +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 (!blocked)
>> +			value = (u8) (result | radio);
>> +		else
>> +			value = (u8) (result & ~radio);
>> +		ec_write(COMPAL_EC_COMMAND_WIRELESS, value);
>> +	}
>> +
>> +	return 0;
>> +}
>
> Note that the return value of rfkill_set is not propagated to
> userspace through the /dev/rfkill interface.
>
> The way you use "KILLSWITCH_MASK", it looks like a hard rfkill bit
> that covers all radios.  Why don't you report it as a hard block on
> each rfkill device?

P.S.  If you do this, you probably also want to set up polling.  I
think you should be able to reuse the query() function for the poll()
method (but you should check yourself whether that makes sense).

Thanks again
Alan

[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