Drop platform sysfs support
Subsystems (3)
PATCH 3/3 Drop platform sysfs support
With rfkill support being added, the platform support is no longer necessary. Standard rfkill interfaces can be used to administer the box now.
Signature · 3 lines
Mario Limonciello *Dell | Linux Engineering* [email protected]
platform/x86/compal-laptop.c renamed from platform/x86/compal-laptop.c.old +1 -195
@@ -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);
Quoted 8 lines · Mario Limonciello
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*
[email protected]
-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
Hi Alan,
Quoted 11 lines
Quoted 8 lines
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*
[email protected]-static struct
platform_device*compal_device;
Quoted 8 lines
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
Quoted 8 lines · Marcel Holtmann
Hi Alan,
Quoted 3 lines
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
Hi Alan:
Quoted 5 lines · Alan Jenkins
Ok. I wasn't sure that userspace was allowed to rely on that, but I re-read
Documentation/ABI/testing/sysfs-devicesand I see now that it can be allowed.
Quoted 10 lines
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.
Signature · 3 lines
Mario Limonciello *Dell | Linux Engineering* [email protected]
drivers/platform/x86/compal-laptop.c renamed from drivers/platform/x86/compal-laptop.c.old +2 -162
@@ -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);
Hi Alan,
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
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.
Quoted 8 lines
Ok. I wasn't sure that userspace was allowed to rely on that, but I re-read
Documentation/ABI/testing/sysfs-devicesand 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.
Quoted 3 lines
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