[GIT PULL] thinkpad-acpi sysfs interface fix

[GIT PULL] thinkpad-acpi sysfs interface fix

[Henrique de Moraes Holschuh]

Len,

Please pull from:
git://repo.or.cz/linux-2.6/linux-acpi-2.6/ibm-acpi-2.6.git for-upstream/acpi-release

to receive the following patch:
	ACPI: thinkpad-acpi: do not use named sysfs groups

(the above git tree is based on 2.6.22-rc2).

The patch fixes a major bad design decision I made in the sysfs interface, and
it needs to go into 2.6.22 to avoid the bad ABI making it to a stable kernel
release (which would make it that much harder to change).

Please send it to Linus for merging.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

[PATCH] ACPI: thinkpad-acpi: do not use named sysfs groups

[Henrique de Moraes Holschuh]

The initial version of the thinkpad-acpi sysfs interface (not yet released
in any stable mainline kernel) made liberal use of named sysfs groups, in
order to get the attributes more organized.

This proved to be a really bad design decision.  Maybe if attribute groups
were as flexible as a real directory, and if binary attributes were not
second-class citizens, the idea of subdirs and named groups would not have
been so bad.

This patch makes all the thinkpad-acpi sysfs groups anonymous (thus
removing the subdirs), adds the former group names as a prefix (so that
hotkey/enable becomes hotkey_enable for example), and updates the
documentation.

These changes will make the thinkpad-acpi sysfs ABI a lot easier to
maintain.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

-- 
1.5.1.4

[PATCH] ACPI: thinkpad-acpi: do not use named sysfs groups

[Henrique de Moraes Holschuh]

The initial version of the thinkpad-acpi sysfs interface (not yet released
in any stable mainline kernel) made liberal use of named sysfs groups, in
order to get the attributes more organized.

This proved to be a really bad design decision.  Maybe if attribute groups
were as flexible as a real directory, and if binary attributes were not
second-class citizens, the idea of subdirs and named groups would not have
been so bad.

This patch makes all the thinkpad-acpi sysfs groups anonymous (thus
removing the subdirs), adds the former group names as a prefix (so that
hotkey/enable becomes hotkey_enable for example), and updates the
documentation.

These changes will make the thinkpad-acpi sysfs ABI a lot easier to
maintain.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---

 It gets easier to apply if I actually get git to include the patch :)
 Sorry about that!

 Documentation/thinkpad-acpi.txt |   25 +++++++++++--------------
 drivers/misc/thinkpad_acpi.c    |   17 +++++++----------
 drivers/misc/thinkpad_acpi.h    |    6 ------
 3 files changed, 18 insertions(+), 30 deletions(-)

diff --git a/Documentation/thinkpad-acpi.txt b/Documentation/thinkpad-acpi.txt
index 2d48033..9e6b94f 100644
--- a/Documentation/thinkpad-acpi.txt
+++ b/Documentation/thinkpad-acpi.txt
@@ -138,7 +138,7 @@ Hot keys
 --------
 
 procfs: /proc/acpi/ibm/hotkey
-sysfs device attribute: hotkey/*
+sysfs device attribute: hotkey_*
 
 Without this driver, only the Fn-F4 key (sleep button) generates an
 ACPI event. With the driver loaded, the hotkey feature enabled and the
@@ -196,10 +196,7 @@ The following commands can be written to the /proc/acpi/ibm/hotkey file:
 
 sysfs notes:
 
-	The hot keys attributes are in a hotkey/ subdirectory off the
-	thinkpad device.
-
-	bios_enabled:
+	hotkey_bios_enabled:
 		Returns the status of the hot keys feature when
 		thinkpad-acpi was loaded.  Upon module unload, the hot
 		key feature status will be restored to this value.
@@ -207,19 +204,19 @@ sysfs notes:
 		0: hot keys were disabled
 		1: hot keys were enabled
 
-	bios_mask:
+	hotkey_bios_mask:
 		Returns the hot keys mask when thinkpad-acpi was loaded.
 		Upon module unload, the hot keys mask will be restored
 		to this value.
 
-	enable:
+	hotkey_enable:
 		Enables/disables the hot keys feature, and reports
 		current status of the hot keys feature.
 
 		0: disables the hot keys feature / feature disabled
 		1: enables the hot keys feature / feature enabled
 
-	mask:
+	hotkey_mask:
 		bit mask to enable ACPI event generation for each hot
 		key (see above).  Returns the current status of the hot
 		keys mask, and allows one to modify it.
@@ -229,7 +226,7 @@ Bluetooth
 ---------
 
 procfs: /proc/acpi/ibm/bluetooth
-sysfs device attribute: bluetooth/enable
+sysfs device attribute: bluetooth_enable
 
 This feature shows the presence and current state of a ThinkPad
 Bluetooth device in the internal ThinkPad CDC slot.
@@ -244,7 +241,7 @@ If Bluetooth is installed, the following commands can be used:
 Sysfs notes:
 
 	If the Bluetooth CDC card is installed, it can be enabled /
-	disabled through the "bluetooth/enable" thinkpad-acpi device
+	disabled through the "bluetooth_enable" thinkpad-acpi device
 	attribute, and its current status can also be queried.
 
 	enable:
@@ -252,7 +249,7 @@ Sysfs notes:
 		1: enables Bluetooth / Bluetooth is enabled.
 
 	Note: this interface will be probably be superseeded by the
-	generic rfkill class.
+	generic rfkill class, so it is NOT to be considered stable yet.
 
 Video output control -- /proc/acpi/ibm/video
 --------------------------------------------
@@ -898,7 +895,7 @@ EXPERIMENTAL: WAN
 -----------------
 
 procfs: /proc/acpi/ibm/wan
-sysfs device attribute: wwan/enable
+sysfs device attribute: wwan_enable
 
 This feature is marked EXPERIMENTAL because the implementation
 directly accesses hardware registers and may not work as expected. USE
@@ -921,7 +918,7 @@ If the W-WAN card is installed, the following commands can be used:
 Sysfs notes:
 
 	If the W-WAN card is installed, it can be enabled /
-	disabled through the "wwan/enable" thinkpad-acpi device
+	disabled through the "wwan_enable" thinkpad-acpi device
 	attribute, and its current status can also be queried.
 
 	enable:
@@ -929,7 +926,7 @@ Sysfs notes:
 		1: enables WWAN card / WWAN card is enabled.
 
 	Note: this interface will be probably be superseeded by the
-	generic rfkill class.
+	generic rfkill class, so it is NOT to be considered stable yet.
 
 Multiple Commands, Module Parameters
 ------------------------------------
diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index 6c36a55..95c0b96 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -740,7 +740,7 @@ static ssize_t hotkey_enable_store(struct device *dev,
 }
 
 static struct device_attribute dev_attr_hotkey_enable =
-	__ATTR(enable, S_IWUSR | S_IRUGO,
+	__ATTR(hotkey_enable, S_IWUSR | S_IRUGO,
 		hotkey_enable_show, hotkey_enable_store);
 
 /* sysfs hotkey mask --------------------------------------------------- */
@@ -775,7 +775,7 @@ static ssize_t hotkey_mask_store(struct device *dev,
 }
 
 static struct device_attribute dev_attr_hotkey_mask =
-	__ATTR(mask, S_IWUSR | S_IRUGO,
+	__ATTR(hotkey_mask, S_IWUSR | S_IRUGO,
 		hotkey_mask_show, hotkey_mask_store);
 
 /* sysfs hotkey bios_enabled ------------------------------------------- */
@@ -787,7 +787,7 @@ static ssize_t hotkey_bios_enabled_show(struct device *dev,
 }
 
 static struct device_attribute dev_attr_hotkey_bios_enabled =
-	__ATTR(bios_enabled, S_IRUGO, hotkey_bios_enabled_show, NULL);
+	__ATTR(hotkey_bios_enabled, S_IRUGO, hotkey_bios_enabled_show, NULL);
 
 /* sysfs hotkey bios_mask ---------------------------------------------- */
 static ssize_t hotkey_bios_mask_show(struct device *dev,
@@ -798,7 +798,7 @@ static ssize_t hotkey_bios_mask_show(struct device *dev,
 }
 
 static struct device_attribute dev_attr_hotkey_bios_mask =
-	__ATTR(bios_mask, S_IRUGO, hotkey_bios_mask_show, NULL);
+	__ATTR(hotkey_bios_mask, S_IRUGO, hotkey_bios_mask_show, NULL);
 
 /* --------------------------------------------------------------------- */
 
@@ -824,8 +824,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 		str_supported(tp_features.hotkey));
 
 	if (tp_features.hotkey) {
-		hotkey_dev_attributes = create_attr_set(4,
-						TPACPI_HOTKEY_SYSFS_GROUP);
+		hotkey_dev_attributes = create_attr_set(4, NULL);
 		if (!hotkey_dev_attributes)
 			return -ENOMEM;
 		res = add_to_attr_set(hotkey_dev_attributes,
@@ -1050,7 +1049,7 @@ static ssize_t bluetooth_enable_store(struct device *dev,
 }
 
 static struct device_attribute dev_attr_bluetooth_enable =
-	__ATTR(enable, S_IWUSR | S_IRUGO,
+	__ATTR(bluetooth_enable, S_IWUSR | S_IRUGO,
 		bluetooth_enable_show, bluetooth_enable_store);
 
 /* --------------------------------------------------------------------- */
@@ -1061,7 +1060,6 @@ static struct attribute *bluetooth_attributes[] = {
 };
 
 static const struct attribute_group bluetooth_attr_group = {
-	.name = TPACPI_BLUETH_SYSFS_GROUP,
 	.attrs = bluetooth_attributes,
 };
 
@@ -1215,7 +1213,7 @@ static ssize_t wan_enable_store(struct device *dev,
 }
 
 static struct device_attribute dev_attr_wan_enable =
-	__ATTR(enable, S_IWUSR | S_IRUGO,
+	__ATTR(wwan_enable, S_IWUSR | S_IRUGO,
 		wan_enable_show, wan_enable_store);
 
 /* --------------------------------------------------------------------- */
@@ -1226,7 +1224,6 @@ static struct attribute *wan_attributes[] = {
 };
 
 static const struct attribute_group wan_attr_group = {
-	.name = TPACPI_WAN_SYSFS_GROUP,
 	.attrs = wan_attributes,
 };
 
diff --git a/drivers/misc/thinkpad_acpi.h b/drivers/misc/thinkpad_acpi.h
index 440145a..72d62f2 100644
--- a/drivers/misc/thinkpad_acpi.h
+++ b/drivers/misc/thinkpad_acpi.h
@@ -278,8 +278,6 @@ static int beep_write(char *buf);
  * Bluetooth subdriver
  */
 
-#define TPACPI_BLUETH_SYSFS_GROUP "bluetooth"
-
 enum {
 	/* ACPI GBDC/SBDC bits */
 	TP_ACPI_BLUETOOTH_HWPRESENT	= 0x01,	/* Bluetooth hw available */
@@ -416,8 +414,6 @@ static int fan_write_cmd_watchdog(const char *cmd, int *rc);
  * Hotkey subdriver
  */
 
-#define TPACPI_HOTKEY_SYSFS_GROUP "hotkey"
-
 static int hotkey_orig_status;
 static int hotkey_orig_mask;
 
@@ -553,8 +549,6 @@ static int volume_write(char *buf);
  * Wan subdriver
  */
 
-#define TPACPI_WAN_SYSFS_GROUP "wwan"
-
 enum {
 	/* ACPI GWAN/SWAN bits */
 	TP_ACPI_WANCARD_HWPRESENT	= 0x01,	/* Wan hw available */
-- 
1.5.1.6

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] ACPI: thinkpad-acpi: do not use named sysfs groups

[Len Brown]

Applied.

thanks,
-Len

On Wednesday 30 May 2007 19:50, Henrique de Moraes Holschuh wrote:
> The initial version of the thinkpad-acpi sysfs interface (not yet released
> in any stable mainline kernel) made liberal use of named sysfs groups, in
> order to get the attributes more organized.
> 
> This proved to be a really bad design decision.  Maybe if attribute groups
> were as flexible as a real directory, and if binary attributes were not
> second-class citizens, the idea of subdirs and named groups would not have
> been so bad.
> 
> This patch makes all the thinkpad-acpi sysfs groups anonymous (thus
> removing the subdirs), adds the former group names as a prefix (so that
> hotkey/enable becomes hotkey_enable for example), and updates the
> documentation.
> 
> These changes will make the thinkpad-acpi sysfs ABI a lot easier to
> maintain.
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> ---
> 
>  It gets easier to apply if I actually get git to include the patch :)
>  Sorry about that!
> 
>  Documentation/thinkpad-acpi.txt |   25 +++++++++++--------------
>  drivers/misc/thinkpad_acpi.c    |   17 +++++++----------
>  drivers/misc/thinkpad_acpi.h    |    6 ------
>  3 files changed, 18 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/thinkpad-acpi.txt b/Documentation/thinkpad-acpi.txt
> index 2d48033..9e6b94f 100644
> --- a/Documentation/thinkpad-acpi.txt
> +++ b/Documentation/thinkpad-acpi.txt
> @@ -138,7 +138,7 @@ Hot keys
>  --------
>  
>  procfs: /proc/acpi/ibm/hotkey
> -sysfs device attribute: hotkey/*
> +sysfs device attribute: hotkey_*
>  
>  Without this driver, only the Fn-F4 key (sleep button) generates an
>  ACPI event. With the driver loaded, the hotkey feature enabled and the
> @@ -196,10 +196,7 @@ The following commands can be written to the /proc/acpi/ibm/hotkey file:
>  
>  sysfs notes:
>  
> -	The hot keys attributes are in a hotkey/ subdirectory off the
> -	thinkpad device.
> -
> -	bios_enabled:
> +	hotkey_bios_enabled:
>  		Returns the status of the hot keys feature when
>  		thinkpad-acpi was loaded.  Upon module unload, the hot
>  		key feature status will be restored to this value.
> @@ -207,19 +204,19 @@ sysfs notes:
>  		0: hot keys were disabled
>  		1: hot keys were enabled
>  
> -	bios_mask:
> +	hotkey_bios_mask:
>  		Returns the hot keys mask when thinkpad-acpi was loaded.
>  		Upon module unload, the hot keys mask will be restored
>  		to this value.
>  
> -	enable:
> +	hotkey_enable:
>  		Enables/disables the hot keys feature, and reports
>  		current status of the hot keys feature.
>  
>  		0: disables the hot keys feature / feature disabled
>  		1: enables the hot keys feature / feature enabled
>  
> -	mask:
> +	hotkey_mask:
>  		bit mask to enable ACPI event generation for each hot
>  		key (see above).  Returns the current status of the hot
>  		keys mask, and allows one to modify it.
> @@ -229,7 +226,7 @@ Bluetooth
>  ---------
>  
>  procfs: /proc/acpi/ibm/bluetooth
> -sysfs device attribute: bluetooth/enable
> +sysfs device attribute: bluetooth_enable
>  
>  This feature shows the presence and current state of a ThinkPad
>  Bluetooth device in the internal ThinkPad CDC slot.
> @@ -244,7 +241,7 @@ If Bluetooth is installed, the following commands can be used:
>  Sysfs notes:
>  
>  	If the Bluetooth CDC card is installed, it can be enabled /
> -	disabled through the "bluetooth/enable" thinkpad-acpi device
> +	disabled through the "bluetooth_enable" thinkpad-acpi device
>  	attribute, and its current status can also be queried.
>  
>  	enable:
> @@ -252,7 +249,7 @@ Sysfs notes:
>  		1: enables Bluetooth / Bluetooth is enabled.
>  
>  	Note: this interface will be probably be superseeded by the
> -	generic rfkill class.
> +	generic rfkill class, so it is NOT to be considered stable yet.
>  
>  Video output control -- /proc/acpi/ibm/video
>  --------------------------------------------
> @@ -898,7 +895,7 @@ EXPERIMENTAL: WAN
>  -----------------
>  
>  procfs: /proc/acpi/ibm/wan
> -sysfs device attribute: wwan/enable
> +sysfs device attribute: wwan_enable
>  
>  This feature is marked EXPERIMENTAL because the implementation
>  directly accesses hardware registers and may not work as expected. USE
> @@ -921,7 +918,7 @@ If the W-WAN card is installed, the following commands can be used:
>  Sysfs notes:
>  
>  	If the W-WAN card is installed, it can be enabled /
> -	disabled through the "wwan/enable" thinkpad-acpi device
> +	disabled through the "wwan_enable" thinkpad-acpi device
>  	attribute, and its current status can also be queried.
>  
>  	enable:
> @@ -929,7 +926,7 @@ Sysfs notes:
>  		1: enables WWAN card / WWAN card is enabled.
>  
>  	Note: this interface will be probably be superseeded by the
> -	generic rfkill class.
> +	generic rfkill class, so it is NOT to be considered stable yet.
>  
>  Multiple Commands, Module Parameters
>  ------------------------------------
> diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
> index 6c36a55..95c0b96 100644
> --- a/drivers/misc/thinkpad_acpi.c
> +++ b/drivers/misc/thinkpad_acpi.c
> @@ -740,7 +740,7 @@ static ssize_t hotkey_enable_store(struct device *dev,
>  }
>  
>  static struct device_attribute dev_attr_hotkey_enable =
> -	__ATTR(enable, S_IWUSR | S_IRUGO,
> +	__ATTR(hotkey_enable, S_IWUSR | S_IRUGO,
>  		hotkey_enable_show, hotkey_enable_store);
>  
>  /* sysfs hotkey mask --------------------------------------------------- */
> @@ -775,7 +775,7 @@ static ssize_t hotkey_mask_store(struct device *dev,
>  }
>  
>  static struct device_attribute dev_attr_hotkey_mask =
> -	__ATTR(mask, S_IWUSR | S_IRUGO,
> +	__ATTR(hotkey_mask, S_IWUSR | S_IRUGO,
>  		hotkey_mask_show, hotkey_mask_store);
>  
>  /* sysfs hotkey bios_enabled ------------------------------------------- */
> @@ -787,7 +787,7 @@ static ssize_t hotkey_bios_enabled_show(struct device *dev,
>  }
>  
>  static struct device_attribute dev_attr_hotkey_bios_enabled =
> -	__ATTR(bios_enabled, S_IRUGO, hotkey_bios_enabled_show, NULL);
> +	__ATTR(hotkey_bios_enabled, S_IRUGO, hotkey_bios_enabled_show, NULL);
>  
>  /* sysfs hotkey bios_mask ---------------------------------------------- */
>  static ssize_t hotkey_bios_mask_show(struct device *dev,
> @@ -798,7 +798,7 @@ static ssize_t hotkey_bios_mask_show(struct device *dev,
>  }
>  
>  static struct device_attribute dev_attr_hotkey_bios_mask =
> -	__ATTR(bios_mask, S_IRUGO, hotkey_bios_mask_show, NULL);
> +	__ATTR(hotkey_bios_mask, S_IRUGO, hotkey_bios_mask_show, NULL);
>  
>  /* --------------------------------------------------------------------- */
>  
> @@ -824,8 +824,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>  		str_supported(tp_features.hotkey));
>  
>  	if (tp_features.hotkey) {
> -		hotkey_dev_attributes = create_attr_set(4,
> -						TPACPI_HOTKEY_SYSFS_GROUP);
> +		hotkey_dev_attributes = create_attr_set(4, NULL);
>  		if (!hotkey_dev_attributes)
>  			return -ENOMEM;
>  		res = add_to_attr_set(hotkey_dev_attributes,
> @@ -1050,7 +1049,7 @@ static ssize_t bluetooth_enable_store(struct device *dev,
>  }
>  
>  static struct device_attribute dev_attr_bluetooth_enable =
> -	__ATTR(enable, S_IWUSR | S_IRUGO,
> +	__ATTR(bluetooth_enable, S_IWUSR | S_IRUGO,
>  		bluetooth_enable_show, bluetooth_enable_store);
>  
>  /* --------------------------------------------------------------------- */
> @@ -1061,7 +1060,6 @@ static struct attribute *bluetooth_attributes[] = {
>  };
>  
>  static const struct attribute_group bluetooth_attr_group = {
> -	.name = TPACPI_BLUETH_SYSFS_GROUP,
>  	.attrs = bluetooth_attributes,
>  };
>  
> @@ -1215,7 +1213,7 @@ static ssize_t wan_enable_store(struct device *dev,
>  }
>  
>  static struct device_attribute dev_attr_wan_enable =
> -	__ATTR(enable, S_IWUSR | S_IRUGO,
> +	__ATTR(wwan_enable, S_IWUSR | S_IRUGO,
>  		wan_enable_show, wan_enable_store);
>  
>  /* --------------------------------------------------------------------- */
> @@ -1226,7 +1224,6 @@ static struct attribute *wan_attributes[] = {
>  };
>  
>  static const struct attribute_group wan_attr_group = {
> -	.name = TPACPI_WAN_SYSFS_GROUP,
>  	.attrs = wan_attributes,
>  };
>  
> diff --git a/drivers/misc/thinkpad_acpi.h b/drivers/misc/thinkpad_acpi.h
> index 440145a..72d62f2 100644
> --- a/drivers/misc/thinkpad_acpi.h
> +++ b/drivers/misc/thinkpad_acpi.h
> @@ -278,8 +278,6 @@ static int beep_write(char *buf);
>   * Bluetooth subdriver
>   */
>  
> -#define TPACPI_BLUETH_SYSFS_GROUP "bluetooth"
> -
>  enum {
>  	/* ACPI GBDC/SBDC bits */
>  	TP_ACPI_BLUETOOTH_HWPRESENT	= 0x01,	/* Bluetooth hw available */
> @@ -416,8 +414,6 @@ static int fan_write_cmd_watchdog(const char *cmd, int *rc);
>   * Hotkey subdriver
>   */
>  
> -#define TPACPI_HOTKEY_SYSFS_GROUP "hotkey"
> -
>  static int hotkey_orig_status;
>  static int hotkey_orig_mask;
>  
> @@ -553,8 +549,6 @@ static int volume_write(char *buf);
>   * Wan subdriver
>   */
>  
> -#define TPACPI_WAN_SYSFS_GROUP "wwan"
> -
>  enum {
>  	/* ACPI GWAN/SWAN bits */
>  	TP_ACPI_WANCARD_HWPRESENT	= 0x01,	/* Wan hw available */
> -- 
> 1.5.1.6
>