[PATCH 3/3] Drop platform sysfs support

[PATCH 3/3] Drop platform sysfs support

[Mario Limonciello]

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

With rfkill support being added, the platform support is no longer
necessary.  Standard rfkill interfaces can be used to administer the box
now.
-- 
Mario Limonciello
*Dell | Linux Engineering*
mario_limonciello@dell.com

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

--- drivers/platform/x86/compal-laptop.c.old	2009-08-17 07:02:15.012836625 -0500
+++ drivers/platform/x86/compal-laptop.c	2009-08-17 07:03:03.925633380 -0500
@@ -26,13 +26,6 @@
 /*
  * comapl-laptop.c - Compal laptop support.
  *
- * This driver exports a few files in /sys/devices/platform/compal-laptop/:
- *
- *   wlan - wlan subsystem state: contains 0 or 1 (rw)
- *
- *   bluetooth - Bluetooth subsystem state: contains 0 or 1 (rw)
- *
- *   raw - raw value taken from embedded controller register (ro)
  *
  * In addition to these platform device attributes the driver
  * registers itself in the Linux backlight control subsystem and is
@@ -174,67 +167,6 @@
 	return ret;
 }
 
-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)
@@ -255,96 +187,6 @@
 
 static struct backlight_device *compalbl_device;
 
-/* Platform device */
-
-static ssize_t show_wlan(struct device *dev,
-	struct device_attribute *attr, char *buf)
-{
-	int ret, enabled;
-
-	ret = get_wireless_state(&enabled, NULL);
-	if (ret < 0)
-		return ret;
-
-	return sprintf(buf, "%i\n", enabled);
-}
-
-static ssize_t show_raw(struct device *dev,
-	struct device_attribute *attr, char *buf)
-{
-	u8 result;
-
-	ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
-
-	return sprintf(buf, "%i\n", result);
-}
-
-static ssize_t show_bluetooth(struct device *dev,
-	struct device_attribute *attr, char *buf)
-{
-	int ret, enabled;
-
-	ret = get_wireless_state(NULL, &enabled);
-	if (ret < 0)
-		return ret;
-
-	return sprintf(buf, "%i\n", enabled);
-}
-
-static ssize_t store_wlan_state(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t count)
-{
-	int state, ret;
-
-	if (sscanf(buf, "%i", &state) != 1 || (state < 0 || state > 1))
-		return -EINVAL;
-
-	ret = set_wlan_state(state);
-	if (ret < 0)
-		return ret;
-
-	return count;
-}
-
-static ssize_t store_bluetooth_state(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t count)
-{
-	int state, ret;
-
-	if (sscanf(buf, "%i", &state) != 1 || (state < 0 || state > 1))
-		return -EINVAL;
-
-	ret = set_bluetooth_state(state);
-	if (ret < 0)
-		return ret;
-
-	return count;
-}
-
-static DEVICE_ATTR(bluetooth, 0644, show_bluetooth, store_bluetooth_state);
-static DEVICE_ATTR(wlan, 0644, show_wlan, store_wlan_state);
-static DEVICE_ATTR(raw, 0444, show_raw, NULL);
-
-static struct attribute *compal_attributes[] = {
-	&dev_attr_bluetooth.attr,
-	&dev_attr_wlan.attr,
-	&dev_attr_raw.attr,
-	NULL
-};
-
-static struct attribute_group compal_attribute_group = {
-	.attrs = compal_attributes
-};
-
-static struct platform_driver compal_driver = {
-	.driver = {
-		.name = "compal-laptop",
-		.owner = THIS_MODULE,
-	}
-};
-
-static struct platform_device *compal_device;
-
 /* Initialization */
 
 static int dmi_check_cb(const struct dmi_system_id *id)
@@ -454,49 +296,16 @@
 		compalbl_device = backlight_device_register("compal-laptop", NULL, NULL,
 							    &compalbl_ops);
 		if (IS_ERR(compalbl_device))
-			return PTR_ERR(compalbl_device);
+			goto fail_backlight;
 
 		compalbl_device->props.max_brightness = COMPAL_LCD_LEVEL_MAX-1;
 	}
 
-	ret = platform_driver_register(&compal_driver);
-	if (ret)
-		goto fail_backlight;
-
-	/* Register platform stuff */
-
-	compal_device = platform_device_alloc("compal-laptop", -1);
-	if (!compal_device) {
-		ret = -ENOMEM;
-		goto fail_platform_driver;
-	}
-
-	ret = platform_device_add(compal_device);
-	if (ret)
-		goto fail_platform_device1;
-
-	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");
 
 	return 0;
 
-fail_platform_device2:
-
-	platform_device_del(compal_device);
-
-fail_platform_device1:
-
-	platform_device_put(compal_device);
-
-fail_platform_driver:
-
-	platform_driver_unregister(&compal_driver);
-
 fail_backlight:
 
 	backlight_device_unregister(compalbl_device);
@@ -514,9 +323,6 @@
 static void __exit compal_cleanup(void)
 {
 
-	sysfs_remove_group(&compal_device->dev.kobj, &compal_attribute_group);
-	platform_device_unregister(compal_device);
-	platform_driver_unregister(&compal_driver);
 	backlight_device_unregister(compalbl_device);
 	if (wifi_rfkill)
 		rfkill_unregister(wifi_rfkill);

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

Re: [PATCH 3/3] Drop platform sysfs support

[Alan Jenkins]

On 8/18/09, Mario Limonciello <mario_limonciello@dell.com> wrote:
> With rfkill support being added, the platform support is no longer
> necessary.  Standard rfkill interfaces can be used to administer the box
> now.
> --
> Mario Limonciello
> *Dell | Linux Engineering*
> mario_limonciello@dell.com
>

-static struct platform_device *compal_device;

I don't think this is the right way to go.  I don't object to removing
the sysfs attributes, but the platform device can still be useful as a
parent device to the rfkill and backlight devices.  (See my earlier
comment on the use of rfkill_allocate()).

In practice I suspect it makes no difference that the rfkill and
backlight devices are exported as virtual devices with no physical
parent.  I just don't think it's "right" :-).

Thanks
Alan

Re: [PATCH 3/3] Drop platform sysfs support

[Marcel Holtmann]

Hi Alan,

> > With rfkill support being added, the platform support is no longer
> > necessary.  Standard rfkill interfaces can be used to administer the box
> > now.
> > --
> > Mario Limonciello
> > *Dell | Linux Engineering*
> > mario_limonciello@dell.com
> >
> 
> -static struct platform_device *compal_device;
> 
> I don't think this is the right way to go.  I don't object to removing
> the sysfs attributes, but the platform device can still be useful as a
> parent device to the rfkill and backlight devices.  (See my earlier
> comment on the use of rfkill_allocate()).
> 
> In practice I suspect it makes no difference that the rfkill and
> backlight devices are exported as virtual devices with no physical
> parent.  I just don't think it's "right" :-).

it actually does make a difference for hardware detection. We wanna have
them hanging of the compal platform device.

Regards

Marcel

Re: [PATCH 3/3] Drop platform sysfs support

[Alan Jenkins]

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

>> In practice I suspect it makes no difference that the rfkill and
>> backlight devices are exported as virtual devices with no physical
>> parent.  I just don't think it's "right" :-).
>
> it actually does make a difference for hardware detection. We wanna have
> them hanging of the compal platform device.

Ok.  I wasn't sure that userspace was allowed to rely on that, but I
re-read Documentation/ABI/testing/sysfs-devices and I see now that it
can be allowed.

Some of these issues with the compal-laptop work may come from copying
dell-laptop.  dell-laptop is the only driver which creates rfkill
devices without a parent device.  In fact, it doesn't even create a
platform device :-).

I'll try sending a patch or two to clean up dell-laptop.  I don't have
the hardware but it should be pretty mechanical, almost a copy+paste
job from a "good" driver.

Thanks
Alan

Re: [PATCH 3/3] Drop platform sysfs support

[Mario Limonciello]

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

Hi Alan:

Alan Jenkins wrote:
>
> Ok.  I wasn't sure that userspace was allowed to rely on that, but I
> re-read Documentation/ABI/testing/sysfs-devices and I see now that it
> can be allowed.
>
> Some of these issues with the compal-laptop work may come from copying
> dell-laptop.  dell-laptop is the only driver which creates rfkill
> devices without a parent device.  In fact, it doesn't even create a
> platform device :-).
>
> I'll try sending a patch or two to clean up dell-laptop.  I don't have
> the hardware but it should be pretty mechanical, almost a copy+paste
> job from a "good" driver.
>
>   
Yeah, a lot of  the source for the design issues here were from copying
bits out of dell-laptop.  I've made adjustments to this third patch to
only drop the sysfs attributes but still create the platform device.
-- 
Mario Limonciello
*Dell | Linux Engineering*
mario_limonciello@dell.com

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

--- a/drivers/platform/x86/compal-laptop.c.old	2009-08-18 01:39:12.864668371 -0500
+++ b/drivers/platform/x86/compal-laptop.c	2009-08-18 01:42:41.715047544 -0500
@@ -26,17 +26,8 @@
 /*
  * comapl-laptop.c - Compal laptop support.
  *
- * This driver exports a few files in /sys/devices/platform/compal-laptop/:
- *
- *   wlan - wlan subsystem state: contains 0 or 1 (rw)
- *
- *   bluetooth - Bluetooth subsystem state: contains 0 or 1 (rw)
- *
- *   raw - raw value taken from embedded controller register (ro)
- *
- * In addition to these platform device attributes the driver
- * registers itself in the Linux backlight control subsystem and is
- * available to userspace under /sys/class/backlight/compal-laptop/.
+ * The driver registers itself with the rfkill subsystem and
+ * the Linux backlight control subsystem.
  *
  * This driver might work on other laptops produced by Compal. If you
  * want to try it you can pass force=1 as argument to the module which
@@ -174,67 +165,6 @@
 	return ret;
 }
 
-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)
@@ -255,86 +185,6 @@
 
 static struct backlight_device *compalbl_device;
 
-/* Platform device */
-
-static ssize_t show_wlan(struct device *dev,
-	struct device_attribute *attr, char *buf)
-{
-	int ret, enabled;
-
-	ret = get_wireless_state(&enabled, NULL);
-	if (ret < 0)
-		return ret;
-
-	return sprintf(buf, "%i\n", enabled);
-}
-
-static ssize_t show_raw(struct device *dev,
-	struct device_attribute *attr, char *buf)
-{
-	u8 result;
-
-	ec_read(COMPAL_EC_COMMAND_WIRELESS, &result);
-
-	return sprintf(buf, "%i\n", result);
-}
-
-static ssize_t show_bluetooth(struct device *dev,
-	struct device_attribute *attr, char *buf)
-{
-	int ret, enabled;
-
-	ret = get_wireless_state(NULL, &enabled);
-	if (ret < 0)
-		return ret;
-
-	return sprintf(buf, "%i\n", enabled);
-}
-
-static ssize_t store_wlan_state(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t count)
-{
-	int state, ret;
-
-	if (sscanf(buf, "%i", &state) != 1 || (state < 0 || state > 1))
-		return -EINVAL;
-
-	ret = set_wlan_state(state);
-	if (ret < 0)
-		return ret;
-
-	return count;
-}
-
-static ssize_t store_bluetooth_state(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t count)
-{
-	int state, ret;
-
-	if (sscanf(buf, "%i", &state) != 1 || (state < 0 || state > 1))
-		return -EINVAL;
-
-	ret = set_bluetooth_state(state);
-	if (ret < 0)
-		return ret;
-
-	return count;
-}
-
-static DEVICE_ATTR(bluetooth, 0644, show_bluetooth, store_bluetooth_state);
-static DEVICE_ATTR(wlan, 0644, show_wlan, store_wlan_state);
-static DEVICE_ATTR(raw, 0444, show_raw, NULL);
-
-static struct attribute *compal_attributes[] = {
-	&dev_attr_bluetooth.attr,
-	&dev_attr_wlan.attr,
-	&dev_attr_raw.attr,
-	NULL
-};
-
-static struct attribute_group compal_attribute_group = {
-	.attrs = compal_attributes
-};
 
 static struct platform_driver compal_driver = {
 	.driver = {
@@ -475,11 +325,6 @@
 	if (ret)
 		goto fail_platform_device1;
 
-	ret = sysfs_create_group(&compal_device->dev.kobj,
-		&compal_attribute_group);
-	if (ret)
-		goto fail_platform_device2;
-
 	ret = setup_rfkill();
 	if (ret)
 		printk(KERN_WARNING "compal-laptop: Unable to setup rfkill\n");
@@ -489,10 +334,6 @@
 
 	return 0;
 
-fail_platform_device2:
-
-	platform_device_del(compal_device);
-
 fail_platform_device1:
 
 	platform_device_put(compal_device);
@@ -511,7 +352,6 @@
 static void __exit compal_cleanup(void)
 {
 
-	sysfs_remove_group(&compal_device->dev.kobj, &compal_attribute_group);
 	platform_device_unregister(compal_device);
 	platform_driver_unregister(&compal_driver);
 	backlight_device_unregister(compalbl_device);

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

Re: [PATCH 3/3] Drop platform sysfs support

[Marcel Holtmann]

Hi Alan,

> >> In practice I suspect it makes no difference that the rfkill and
> >> backlight devices are exported as virtual devices with no physical
> >> parent.  I just don't think it's "right" :-).
> >
> > it actually does make a difference for hardware detection. We wanna have
> > them hanging of the compal platform device.
> 
> Ok.  I wasn't sure that userspace was allowed to rely on that, but I
> re-read Documentation/ABI/testing/sysfs-devices and I see now that it
> can be allowed.
> 
> Some of these issues with the compal-laptop work may come from copying
> dell-laptop.  dell-laptop is the only driver which creates rfkill
> devices without a parent device.  In fact, it doesn't even create a
> platform device :-).

then that should be fixed. All RFKILL switches should really have a
parent and not dangling around the virtual tree. Especially since they
have physical hardware in the system.

> I'll try sending a patch or two to clean up dell-laptop.  I don't have
> the hardware but it should be pretty mechanical, almost a copy+paste
> job from a "good" driver.

That would be good. I don't have the hardware either, but others here
can test it.

Regards

Marcel