[PATCH 0/6] toshiba_acpi: Cleanup and TOS1900 device support

[PATCH 0/6] toshiba_acpi: Cleanup and TOS1900 device support

[Seth Forshee]

Hi Matthew,

Please consider merging the following patches. They represent a
signficant cleanup of toshiba_acpi as well as adding support for SPFC as
an HCI method. The latter provides basic but functional support for the
TOS1900 devices found on newer Toshiba models, which will serve as a
good starting point for further enhancements. I've tried to leave the
details of the hardware interaction unchanged to reduce the risk of
regression for machines already supported by the driver.

The patches are virtually identical to the RFC series I had sent a while
back, but with an additional patch to initialize the brightness in the
backlight device. 

Thanks,
Seth

[PATCH 1/6] toshiba_acpi: Convert to use acpi_driver

[Seth Forshee]

Changes toshiba_acpi to register an acpi driver and eliminates the
platform device it was using. Also eliminates most global
variables, moving them into toshiba_acpi_dev, along with some
other miscellaneous fixes and cleanup.

Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/platform/x86/toshiba_acpi.c |  506 ++++++++++++++++++-----------------
 1 files changed, 261 insertions(+), 245 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index cb009b2..d74c97c 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -47,7 +47,6 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/backlight.h>
-#include <linux/platform_device.h>
 #include <linux/rfkill.h>
 #include <linux/input.h>
 #include <linux/input/sparse-keymap.h>
@@ -111,6 +110,22 @@ MODULE_LICENSE("GPL");
 #define HCI_WIRELESS_BT_ATTACH		0x40
 #define HCI_WIRELESS_BT_POWER		0x80
 
+struct toshiba_acpi_dev {
+	struct acpi_device *acpi_dev;
+	const char *method_hci;
+	struct rfkill *bt_rfk;
+	struct input_dev *hotkey_dev;
+	struct backlight_device *backlight_dev;
+	struct led_classdev led_dev;
+	int illumination_installed;
+	int force_fan;
+	int last_key_event;
+	int key_event_valid;
+	acpi_handle handle;
+
+	struct mutex mutex;
+};
+
 static const struct acpi_device_id toshiba_device_ids[] = {
 	{"TOS6200", 0},
 	{"TOS6208", 0},
@@ -119,7 +134,7 @@ static const struct acpi_device_id toshiba_device_ids[] = {
 };
 MODULE_DEVICE_TABLE(acpi, toshiba_device_ids);
 
-static const struct key_entry toshiba_acpi_keymap[] __initconst = {
+static const struct key_entry toshiba_acpi_keymap[] __devinitconst = {
 	{ KE_KEY, 0x101, { KEY_MUTE } },
 	{ KE_KEY, 0x102, { KEY_ZOOMOUT } },
 	{ KE_KEY, 0x103, { KEY_ZOOMIN } },
@@ -179,29 +194,11 @@ static int write_acpi_int(const char *methodName, int val)
 	return (status == AE_OK);
 }
 
-#if 0
-static int read_acpi_int(const char *methodName, int *pVal)
-{
-	struct acpi_buffer results;
-	union acpi_object out_objs[1];
-	acpi_status status;
-
-	results.length = sizeof(out_objs);
-	results.pointer = out_objs;
-
-	status = acpi_evaluate_object(0, (char *)methodName, 0, &results);
-	*pVal = out_objs[0].integer.value;
-
-	return (status == AE_OK) && (out_objs[0].type == ACPI_TYPE_INTEGER);
-}
-#endif
-
-static const char *method_hci /*= 0*/ ;
-
 /* Perform a raw HCI call.  Here we don't care about input or output buffer
  * format.
  */
-static acpi_status hci_raw(const u32 in[HCI_WORDS], u32 out[HCI_WORDS])
+static acpi_status hci_raw(struct toshiba_acpi_dev *dev,
+			   const u32 in[HCI_WORDS], u32 out[HCI_WORDS])
 {
 	struct acpi_object_list params;
 	union acpi_object in_objs[HCI_WORDS];
@@ -220,7 +217,7 @@ static acpi_status hci_raw(const u32 in[HCI_WORDS], u32 out[HCI_WORDS])
 	results.length = sizeof(out_objs);
 	results.pointer = out_objs;
 
-	status = acpi_evaluate_object(NULL, (char *)method_hci, &params,
+	status = acpi_evaluate_object(NULL, (char *)dev->method_hci, &params,
 				      &results);
 	if ((status == AE_OK) && (out_objs->package.count <= HCI_WORDS)) {
 		for (i = 0; i < out_objs->package.count; ++i) {
@@ -237,85 +234,79 @@ static acpi_status hci_raw(const u32 in[HCI_WORDS], u32 out[HCI_WORDS])
  * may be useful (such as "not supported").
  */
 
-static acpi_status hci_write1(u32 reg, u32 in1, u32 * result)
+static acpi_status hci_write1(struct toshiba_acpi_dev *dev, u32 reg,
+			      u32 in1, u32 *result)
 {
 	u32 in[HCI_WORDS] = { HCI_SET, reg, in1, 0, 0, 0 };
 	u32 out[HCI_WORDS];
-	acpi_status status = hci_raw(in, out);
+	acpi_status status = hci_raw(dev, in, out);
 	*result = (status == AE_OK) ? out[0] : HCI_FAILURE;
 	return status;
 }
 
-static acpi_status hci_read1(u32 reg, u32 * out1, u32 * result)
+static acpi_status hci_read1(struct toshiba_acpi_dev *dev, u32 reg,
+			     u32 *out1, u32 *result)
 {
 	u32 in[HCI_WORDS] = { HCI_GET, reg, 0, 0, 0, 0 };
 	u32 out[HCI_WORDS];
-	acpi_status status = hci_raw(in, out);
+	acpi_status status = hci_raw(dev, in, out);
 	*out1 = out[2];
 	*result = (status == AE_OK) ? out[0] : HCI_FAILURE;
 	return status;
 }
 
-static acpi_status hci_write2(u32 reg, u32 in1, u32 in2, u32 *result)
+static acpi_status hci_write2(struct toshiba_acpi_dev *dev, u32 reg,
+			      u32 in1, u32 in2, u32 *result)
 {
 	u32 in[HCI_WORDS] = { HCI_SET, reg, in1, in2, 0, 0 };
 	u32 out[HCI_WORDS];
-	acpi_status status = hci_raw(in, out);
+	acpi_status status = hci_raw(dev, in, out);
 	*result = (status == AE_OK) ? out[0] : HCI_FAILURE;
 	return status;
 }
 
-static acpi_status hci_read2(u32 reg, u32 *out1, u32 *out2, u32 *result)
+static acpi_status hci_read2(struct toshiba_acpi_dev *dev, u32 reg,
+			     u32 *out1, u32 *out2, u32 *result)
 {
 	u32 in[HCI_WORDS] = { HCI_GET, reg, *out1, *out2, 0, 0 };
 	u32 out[HCI_WORDS];
-	acpi_status status = hci_raw(in, out);
+	acpi_status status = hci_raw(dev, in, out);
 	*out1 = out[2];
 	*out2 = out[3];
 	*result = (status == AE_OK) ? out[0] : HCI_FAILURE;
 	return status;
 }
 
-struct toshiba_acpi_dev {
-	struct platform_device *p_dev;
-	struct rfkill *bt_rfk;
-	struct input_dev *hotkey_dev;
-	int illumination_installed;
-	acpi_handle handle;
-
-	const char *bt_name;
-
-	struct mutex mutex;
-};
-
 /* Illumination support */
-static int toshiba_illumination_available(void)
+static int toshiba_illumination_available(struct toshiba_acpi_dev *dev)
 {
 	u32 in[HCI_WORDS] = { 0, 0, 0, 0, 0, 0 };
 	u32 out[HCI_WORDS];
 	acpi_status status;
 
 	in[0] = 0xf100;
-	status = hci_raw(in, out);
+	status = hci_raw(dev, in, out);
 	if (ACPI_FAILURE(status)) {
 		pr_info("Illumination device not available\n");
 		return 0;
 	}
 	in[0] = 0xf400;
-	status = hci_raw(in, out);
+	status = hci_raw(dev, in, out);
 	return 1;
 }
 
 static void toshiba_illumination_set(struct led_classdev *cdev,
 				     enum led_brightness brightness)
 {
+	struct toshiba_acpi_dev *dev = container_of(cdev,
+			struct toshiba_acpi_dev, led_dev);
 	u32 in[HCI_WORDS] = { 0, 0, 0, 0, 0, 0 };
 	u32 out[HCI_WORDS];
 	acpi_status status;
 
 	/* First request : initialize communication. */
 	in[0] = 0xf100;
-	status = hci_raw(in, out);
+	status = hci_raw(dev, in, out);
 	if (ACPI_FAILURE(status)) {
 		pr_info("Illumination device not available\n");
 		return;
@@ -326,7 +317,7 @@ static void toshiba_illumination_set(struct led_classdev *cdev,
 		in[0] = 0xf400;
 		in[1] = 0x14e;
 		in[2] = 1;
-		status = hci_raw(in, out);
+		status = hci_raw(dev, in, out);
 		if (ACPI_FAILURE(status)) {
 			pr_info("ACPI call for illumination failed\n");
 			return;
@@ -336,7 +327,7 @@ static void toshiba_illumination_set(struct led_classdev *cdev,
 		in[0] = 0xf400;
 		in[1] = 0x14e;
 		in[2] = 0;
-		status = hci_raw(in, out);
+		status = hci_raw(dev, in, out);
 		if (ACPI_FAILURE(status)) {
 			pr_info("ACPI call for illumination failed.\n");
 			return;
@@ -347,11 +338,13 @@ static void toshiba_illumination_set(struct led_classdev *cdev,
 	in[0] = 0xf200;
 	in[1] = 0;
 	in[2] = 0;
-	hci_raw(in, out);
+	hci_raw(dev, in, out);
 }
 
 static enum led_brightness toshiba_illumination_get(struct led_classdev *cdev)
 {
+	struct toshiba_acpi_dev *dev = container_of(cdev,
+			struct toshiba_acpi_dev, led_dev);
 	u32 in[HCI_WORDS] = { 0, 0, 0, 0, 0, 0 };
 	u32 out[HCI_WORDS];
 	acpi_status status;
@@ -359,7 +352,7 @@ static enum led_brightness toshiba_illumination_get(struct led_classdev *cdev)
 
 	/* First request : initialize communication. */
 	in[0] = 0xf100;
-	status = hci_raw(in, out);
+	status = hci_raw(dev, in, out);
 	if (ACPI_FAILURE(status)) {
 		pr_info("Illumination device not available\n");
 		return LED_OFF;
@@ -368,7 +361,7 @@ static enum led_brightness toshiba_illumination_get(struct led_classdev *cdev)
 	/* Check the illumination */
 	in[0] = 0xf300;
 	in[1] = 0x14e;
-	status = hci_raw(in, out);
+	status = hci_raw(dev, in, out);
 	if (ACPI_FAILURE(status)) {
 		pr_info("ACPI call for illumination failed.\n");
 		return LED_OFF;
@@ -380,46 +373,35 @@ static enum led_brightness toshiba_illumination_get(struct led_classdev *cdev)
 	in[0] = 0xf200;
 	in[1] = 0;
 	in[2] = 0;
-	hci_raw(in, out);
+	hci_raw(dev, in, out);
 
 	return result;
 }
 
-static struct led_classdev toshiba_led = {
-	.name           = "toshiba::illumination",
-	.max_brightness = 1,
-	.brightness_set = toshiba_illumination_set,
-	.brightness_get = toshiba_illumination_get,
-};
-
-static struct toshiba_acpi_dev toshiba_acpi = {
-	.bt_name = "Toshiba Bluetooth",
-};
-
 /* Bluetooth rfkill handlers */
 
-static u32 hci_get_bt_present(bool *present)
+static u32 hci_get_bt_present(struct toshiba_acpi_dev *dev, bool *present)
 {
 	u32 hci_result;
 	u32 value, value2;
 
 	value = 0;
 	value2 = 0;
-	hci_read2(HCI_WIRELESS, &value, &value2, &hci_result);
+	hci_read2(dev, HCI_WIRELESS, &value, &value2, &hci_result);
 	if (hci_result == HCI_SUCCESS)
 		*present = (value & HCI_WIRELESS_BT_PRESENT) ? true : false;
 
 	return hci_result;
 }
 
-static u32 hci_get_radio_state(bool *radio_state)
+static u32 hci_get_radio_state(struct toshiba_acpi_dev *dev, bool *radio_state)
 {
 	u32 hci_result;
 	u32 value, value2;
 
 	value = 0;
 	value2 = 0x0001;
-	hci_read2(HCI_WIRELESS, &value, &value2, &hci_result);
+	hci_read2(dev, HCI_WIRELESS, &value, &value2, &hci_result);
 
 	*radio_state = value & HCI_WIRELESS_KILL_SWITCH;
 	return hci_result;
@@ -436,7 +418,7 @@ static int bt_rfkill_set_block(void *data, bool blocked)
 	value = (blocked == false);
 
 	mutex_lock(&dev->mutex);
-	if (hci_get_radio_state(&radio_state) != HCI_SUCCESS) {
+	if (hci_get_radio_state(dev, &radio_state) != HCI_SUCCESS) {
 		err = -EBUSY;
 		goto out;
 	}
@@ -446,8 +428,8 @@ static int bt_rfkill_set_block(void *data, bool blocked)
 		goto out;
 	}
 
-	hci_write2(HCI_WIRELESS, value, HCI_WIRELESS_BT_POWER, &result1);
-	hci_write2(HCI_WIRELESS, value, HCI_WIRELESS_BT_ATTACH, &result2);
+	hci_write2(dev, HCI_WIRELESS, value, HCI_WIRELESS_BT_POWER, &result1);
+	hci_write2(dev, HCI_WIRELESS, value, HCI_WIRELESS_BT_ATTACH, &result2);
 
 	if (result1 != HCI_SUCCESS || result2 != HCI_SUCCESS)
 		err = -EBUSY;
@@ -467,7 +449,7 @@ static void bt_rfkill_poll(struct rfkill *rfkill, void *data)
 
 	mutex_lock(&dev->mutex);
 
-	hci_result = hci_get_radio_state(&value);
+	hci_result = hci_get_radio_state(dev, &value);
 	if (hci_result != HCI_SUCCESS) {
 		/* Can't do anything useful */
 		mutex_unlock(&dev->mutex);
@@ -488,17 +470,14 @@ static const struct rfkill_ops toshiba_rfk_ops = {
 };
 
 static struct proc_dir_entry *toshiba_proc_dir /*= 0*/ ;
-static struct backlight_device *toshiba_backlight_device;
-static int force_fan;
-static int last_key_event;
-static int key_event_valid;
 
 static int get_lcd(struct backlight_device *bd)
 {
+	struct toshiba_acpi_dev *dev = bl_get_data(bd);
 	u32 hci_result;
 	u32 value;
 
-	hci_read1(HCI_LCD_BRIGHTNESS, &value, &hci_result);
+	hci_read1(dev, HCI_LCD_BRIGHTNESS, &value, &hci_result);
 	if (hci_result == HCI_SUCCESS) {
 		return (value >> HCI_LCD_BRIGHTNESS_SHIFT);
 	} else
@@ -507,8 +486,13 @@ static int get_lcd(struct backlight_device *bd)
 
 static int lcd_proc_show(struct seq_file *m, void *v)
 {
-	int value = get_lcd(NULL);
+	struct toshiba_acpi_dev *dev = m->private;
+	int value;
+
+	if (!dev->backlight_dev)
+		return -ENODEV;
 
+	value = get_lcd(dev->backlight_dev);
 	if (value >= 0) {
 		seq_printf(m, "brightness:              %d\n", value);
 		seq_printf(m, "brightness_levels:       %d\n",
@@ -522,15 +506,15 @@ static int lcd_proc_show(struct seq_file *m, void *v)
 
 static int lcd_proc_open(struct inode *inode, struct file *file)
 {
-	return single_open(file, lcd_proc_show, NULL);
+	return single_open(file, lcd_proc_show, PDE(inode)->data);
 }
 
-static int set_lcd(int value)
+static int set_lcd(struct toshiba_acpi_dev *dev, int value)
 {
 	u32 hci_result;
 
 	value = value << HCI_LCD_BRIGHTNESS_SHIFT;
-	hci_write1(HCI_LCD_BRIGHTNESS, value, &hci_result);
+	hci_write1(dev, HCI_LCD_BRIGHTNESS, value, &hci_result);
 	if (hci_result != HCI_SUCCESS)
 		return -EFAULT;
 
@@ -539,12 +523,14 @@ static int set_lcd(int value)
 
 static int set_lcd_status(struct backlight_device *bd)
 {
-	return set_lcd(bd->props.brightness);
+	struct toshiba_acpi_dev *dev = bl_get_data(bd);
+	return set_lcd(dev, bd->props.brightness);
 }
 
 static ssize_t lcd_proc_write(struct file *file, const char __user *buf,
 			      size_t count, loff_t *pos)
 {
+	struct toshiba_acpi_dev *dev = PDE(file->f_path.dentry->d_inode)->data;
 	char cmd[42];
 	size_t len;
 	int value;
@@ -557,7 +543,7 @@ static ssize_t lcd_proc_write(struct file *file, const char __user *buf,
 
 	if (sscanf(cmd, " brightness : %i", &value) == 1 &&
 	    value >= 0 && value < HCI_LCD_BRIGHTNESS_LEVELS) {
-		ret = set_lcd(value);
+		ret = set_lcd(dev, value);
 		if (ret == 0)
 			ret = count;
 	} else {
@@ -577,10 +563,11 @@ static const struct file_operations lcd_proc_fops = {
 
 static int video_proc_show(struct seq_file *m, void *v)
 {
+	struct toshiba_acpi_dev *dev = m->private;
 	u32 hci_result;
 	u32 value;
 
-	hci_read1(HCI_VIDEO_OUT, &value, &hci_result);
+	hci_read1(dev, HCI_VIDEO_OUT, &value, &hci_result);
 	if (hci_result == HCI_SUCCESS) {
 		int is_lcd = (value & HCI_VIDEO_OUT_LCD) ? 1 : 0;
 		int is_crt = (value & HCI_VIDEO_OUT_CRT) ? 1 : 0;
@@ -597,12 +584,13 @@ static int video_proc_show(struct seq_file *m, void *v)
 
 static int video_proc_open(struct inode *inode, struct file *file)
 {
-	return single_open(file, video_proc_show, NULL);
+	return single_open(file, video_proc_show, PDE(inode)->data);
 }
 
 static ssize_t video_proc_write(struct file *file, const char __user *buf,
 				size_t count, loff_t *pos)
 {
+	struct toshiba_acpi_dev *dev = PDE(file->f_path.dentry->d_inode)->data;
 	char *cmd, *buffer;
 	int value;
 	int remain = count;
@@ -644,7 +632,7 @@ static ssize_t video_proc_write(struct file *file, const char __user *buf,
 
 	kfree(cmd);
 
-	hci_read1(HCI_VIDEO_OUT, &video_out, &hci_result);
+	hci_read1(dev, HCI_VIDEO_OUT, &video_out, &hci_result);
 	if (hci_result == HCI_SUCCESS) {
 		unsigned int new_video_out = video_out;
 		if (lcd_out != -1)
@@ -675,13 +663,14 @@ static const struct file_operations video_proc_fops = {
 
 static int fan_proc_show(struct seq_file *m, void *v)
 {
+	struct toshiba_acpi_dev *dev = m->private;
 	u32 hci_result;
 	u32 value;
 
-	hci_read1(HCI_FAN, &value, &hci_result);
+	hci_read1(dev, HCI_FAN, &value, &hci_result);
 	if (hci_result == HCI_SUCCESS) {
 		seq_printf(m, "running:                 %d\n", (value > 0));
-		seq_printf(m, "force_on:                %d\n", force_fan);
+		seq_printf(m, "force_on:                %d\n", dev->force_fan);
 	} else {
 		pr_err("Error reading fan status\n");
 	}
@@ -691,12 +680,13 @@ static int fan_proc_show(struct seq_file *m, void *v)
 
 static int fan_proc_open(struct inode *inode, struct file *file)
 {
-	return single_open(file, fan_proc_show, NULL);
+	return single_open(file, fan_proc_show, PDE(inode)->data);
 }
 
 static ssize_t fan_proc_write(struct file *file, const char __user *buf,
 			      size_t count, loff_t *pos)
 {
+	struct toshiba_acpi_dev *dev = PDE(file->f_path.dentry->d_inode)->data;
 	char cmd[42];
 	size_t len;
 	int value;
@@ -709,11 +699,11 @@ static ssize_t fan_proc_write(struct file *file, const char __user *buf,
 
 	if (sscanf(cmd, " force_on : %i", &value) == 1 &&
 	    value >= 0 && value <= 1) {
-		hci_write1(HCI_FAN, value, &hci_result);
+		hci_write1(dev, HCI_FAN, value, &hci_result);
 		if (hci_result != HCI_SUCCESS)
 			return -EFAULT;
 		else
-			force_fan = value;
+			dev->force_fan = value;
 	} else {
 		return -EINVAL;
 	}
@@ -732,21 +722,22 @@ static const struct file_operations fan_proc_fops = {
 
 static int keys_proc_show(struct seq_file *m, void *v)
 {
+	struct toshiba_acpi_dev *dev = m->private;
 	u32 hci_result;
 	u32 value;
 
-	if (!key_event_valid) {
-		hci_read1(HCI_SYSTEM_EVENT, &value, &hci_result);
+	if (!dev->key_event_valid) {
+		hci_read1(dev, HCI_SYSTEM_EVENT, &value, &hci_result);
 		if (hci_result == HCI_SUCCESS) {
-			key_event_valid = 1;
-			last_key_event = value;
+			dev->key_event_valid = 1;
+			dev->last_key_event = value;
 		} else if (hci_result == HCI_EMPTY) {
 			/* better luck next time */
 		} else if (hci_result == HCI_NOT_SUPPORTED) {
 			/* This is a workaround for an unresolved issue on
 			 * some machines where system events sporadically
 			 * become disabled. */
-			hci_write1(HCI_SYSTEM_EVENT, 1, &hci_result);
+			hci_write1(dev, HCI_SYSTEM_EVENT, 1, &hci_result);
 			pr_notice("Re-enabled hotkeys\n");
 		} else {
 			pr_err("Error reading hotkey status\n");
@@ -754,20 +745,21 @@ static int keys_proc_show(struct seq_file *m, void *v)
 		}
 	}
 
-	seq_printf(m, "hotkey_ready:            %d\n", key_event_valid);
-	seq_printf(m, "hotkey:                  0x%04x\n", last_key_event);
+	seq_printf(m, "hotkey_ready:            %d\n", dev->key_event_valid);
+	seq_printf(m, "hotkey:                  0x%04x\n", dev->last_key_event);
 end:
 	return 0;
 }
 
 static int keys_proc_open(struct inode *inode, struct file *file)
 {
-	return single_open(file, keys_proc_show, NULL);
+	return single_open(file, keys_proc_show, PDE(inode)->data);
 }
 
 static ssize_t keys_proc_write(struct file *file, const char __user *buf,
 			       size_t count, loff_t *pos)
 {
+	struct toshiba_acpi_dev *dev = PDE(file->f_path.dentry->d_inode)->data;
 	char cmd[42];
 	size_t len;
 	int value;
@@ -778,7 +770,7 @@ static ssize_t keys_proc_write(struct file *file, const char __user *buf,
 	cmd[len] = '\0';
 
 	if (sscanf(cmd, " hotkey_ready : %i", &value) == 1 && value == 0) {
-		key_event_valid = 0;
+		dev->key_event_valid = 0;
 	} else {
 		return -EINVAL;
 	}
@@ -820,13 +812,19 @@ static const struct file_operations version_proc_fops = {
 
 #define PROC_TOSHIBA		"toshiba"
 
-static void __init create_toshiba_proc_entries(void)
+static void __devinit
+create_toshiba_proc_entries(struct toshiba_acpi_dev *dev)
 {
-	proc_create("lcd", S_IRUGO | S_IWUSR, toshiba_proc_dir, &lcd_proc_fops);
-	proc_create("video", S_IRUGO | S_IWUSR, toshiba_proc_dir, &video_proc_fops);
-	proc_create("fan", S_IRUGO | S_IWUSR, toshiba_proc_dir, &fan_proc_fops);
-	proc_create("keys", S_IRUGO | S_IWUSR, toshiba_proc_dir, &keys_proc_fops);
-	proc_create("version", S_IRUGO, toshiba_proc_dir, &version_proc_fops);
+	proc_create_data("lcd", S_IRUGO | S_IWUSR, toshiba_proc_dir,
+			 &lcd_proc_fops, dev);
+	proc_create_data("video", S_IRUGO | S_IWUSR, toshiba_proc_dir,
+			 &video_proc_fops, dev);
+	proc_create_data("fan", S_IRUGO | S_IWUSR, toshiba_proc_dir,
+			 &fan_proc_fops, dev);
+	proc_create_data("keys", S_IRUGO | S_IWUSR, toshiba_proc_dir,
+			 &keys_proc_fops, dev);
+	proc_create_data("version", S_IRUGO, toshiba_proc_dir,
+			 &version_proc_fops, dev);
 }
 
 static void remove_toshiba_proc_entries(void)
@@ -843,224 +841,242 @@ static const struct backlight_ops toshiba_backlight_data = {
         .update_status  = set_lcd_status,
 };
 
-static void toshiba_acpi_notify(acpi_handle handle, u32 event, void *context)
-{
-	u32 hci_result, value;
-
-	if (event != 0x80)
-		return;
-	do {
-		hci_read1(HCI_SYSTEM_EVENT, &value, &hci_result);
-		if (hci_result == HCI_SUCCESS) {
-			if (value == 0x100)
-				continue;
-			/* act on key press; ignore key release */
-			if (value & 0x80)
-				continue;
-
-			if (!sparse_keymap_report_event(toshiba_acpi.hotkey_dev,
-							value, 1, true)) {
-				pr_info("Unknown key %x\n",
-				       value);
-			}
-		} else if (hci_result == HCI_NOT_SUPPORTED) {
-			/* This is a workaround for an unresolved issue on
-			 * some machines where system events sporadically
-			 * become disabled. */
-			hci_write1(HCI_SYSTEM_EVENT, 1, &hci_result);
-			pr_notice("Re-enabled hotkeys\n");
-		}
-	} while (hci_result != HCI_EMPTY);
-}
-
-static int __init toshiba_acpi_setup_keyboard(char *device)
+static int __devinit toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev,
+						 char *device_path)
 {
 	acpi_status status;
 	int error;
 
-	status = acpi_get_handle(NULL, device, &toshiba_acpi.handle);
+	status = acpi_get_handle(NULL, device_path, &dev->handle);
 	if (ACPI_FAILURE(status)) {
 		pr_info("Unable to get notification device\n");
 		return -ENODEV;
 	}
 
-	toshiba_acpi.hotkey_dev = input_allocate_device();
-	if (!toshiba_acpi.hotkey_dev) {
+	dev->hotkey_dev = input_allocate_device();
+	if (!dev->hotkey_dev) {
 		pr_info("Unable to register input device\n");
 		return -ENOMEM;
 	}
 
-	toshiba_acpi.hotkey_dev->name = "Toshiba input device";
-	toshiba_acpi.hotkey_dev->phys = device;
-	toshiba_acpi.hotkey_dev->id.bustype = BUS_HOST;
+	dev->hotkey_dev->name = "Toshiba input device";
+	dev->hotkey_dev->phys = device_path;
+	dev->hotkey_dev->id.bustype = BUS_HOST;
 
-	error = sparse_keymap_setup(toshiba_acpi.hotkey_dev,
-				    toshiba_acpi_keymap, NULL);
+	error = sparse_keymap_setup(dev->hotkey_dev, toshiba_acpi_keymap, NULL);
 	if (error)
 		goto err_free_dev;
 
-	status = acpi_install_notify_handler(toshiba_acpi.handle,
-				ACPI_DEVICE_NOTIFY, toshiba_acpi_notify, NULL);
-	if (ACPI_FAILURE(status)) {
-		pr_info("Unable to install hotkey notification\n");
-		error = -ENODEV;
-		goto err_free_keymap;
-	}
-
-	status = acpi_evaluate_object(toshiba_acpi.handle, "ENAB", NULL, NULL);
+	status = acpi_evaluate_object(dev->handle, "ENAB", NULL, NULL);
 	if (ACPI_FAILURE(status)) {
 		pr_info("Unable to enable hotkeys\n");
 		error = -ENODEV;
-		goto err_remove_notify;
+		goto err_free_keymap;
 	}
 
-	error = input_register_device(toshiba_acpi.hotkey_dev);
+	error = input_register_device(dev->hotkey_dev);
 	if (error) {
 		pr_info("Unable to register input device\n");
-		goto err_remove_notify;
+		goto err_free_keymap;
 	}
 
 	return 0;
 
- err_remove_notify:
-	acpi_remove_notify_handler(toshiba_acpi.handle,
-				   ACPI_DEVICE_NOTIFY, toshiba_acpi_notify);
  err_free_keymap:
-	sparse_keymap_free(toshiba_acpi.hotkey_dev);
+	sparse_keymap_free(dev->hotkey_dev);
  err_free_dev:
-	input_free_device(toshiba_acpi.hotkey_dev);
-	toshiba_acpi.hotkey_dev = NULL;
+	input_free_device(dev->hotkey_dev);
+	dev->hotkey_dev = NULL;
 	return error;
 }
 
-static void toshiba_acpi_exit(void)
+static int toshiba_acpi_remove(struct acpi_device *acpi_dev, int type)
 {
-	if (toshiba_acpi.hotkey_dev) {
-		acpi_remove_notify_handler(toshiba_acpi.handle,
-				ACPI_DEVICE_NOTIFY, toshiba_acpi_notify);
-		sparse_keymap_free(toshiba_acpi.hotkey_dev);
-		input_unregister_device(toshiba_acpi.hotkey_dev);
-	}
+	struct toshiba_acpi_dev *dev = acpi_driver_data(acpi_dev);
 
-	if (toshiba_acpi.bt_rfk) {
-		rfkill_unregister(toshiba_acpi.bt_rfk);
-		rfkill_destroy(toshiba_acpi.bt_rfk);
-	}
+	remove_toshiba_proc_entries();
 
-	if (toshiba_backlight_device)
-		backlight_device_unregister(toshiba_backlight_device);
+	if (dev->hotkey_dev) {
+		input_unregister_device(dev->hotkey_dev);
+		sparse_keymap_free(dev->hotkey_dev);
+	}
 
-	remove_toshiba_proc_entries();
+	if (dev->bt_rfk) {
+		rfkill_unregister(dev->bt_rfk);
+		rfkill_destroy(dev->bt_rfk);
+	}
 
-	if (toshiba_proc_dir)
-		remove_proc_entry(PROC_TOSHIBA, acpi_root_dir);
+	if (dev->backlight_dev)
+		backlight_device_unregister(dev->backlight_dev);
 
-	if (toshiba_acpi.illumination_installed)
-		led_classdev_unregister(&toshiba_led);
+	if (dev->illumination_installed)
+		led_classdev_unregister(&dev->led_dev);
 
-	platform_device_unregister(toshiba_acpi.p_dev);
+	kfree(dev);
 
-	return;
+	return 0;
 }
 
-static int __init toshiba_acpi_init(void)
+static int __devinit toshiba_acpi_add(struct acpi_device *acpi_dev)
 {
+	struct toshiba_acpi_dev *dev;
 	u32 hci_result;
 	bool bt_present;
 	int ret = 0;
 	struct backlight_properties props;
 
-	if (acpi_disabled)
-		return -ENODEV;
+	pr_info("Toshiba Laptop ACPI Extras version %s\n",
+	       TOSHIBA_ACPI_VERSION);
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+	dev->acpi_dev = acpi_dev;
+	acpi_dev->driver_data = dev;
 
 	/* simple device detection: look for HCI method */
 	if (is_valid_acpi_path(TOSH_INTERFACE_1 GHCI_METHOD)) {
-		method_hci = TOSH_INTERFACE_1 GHCI_METHOD;
-		if (toshiba_acpi_setup_keyboard(TOSH_INTERFACE_1))
+		dev->method_hci = TOSH_INTERFACE_1 GHCI_METHOD;
+		if (toshiba_acpi_setup_keyboard(dev, TOSH_INTERFACE_1))
 			pr_info("Unable to activate hotkeys\n");
 	} else if (is_valid_acpi_path(TOSH_INTERFACE_2 GHCI_METHOD)) {
-		method_hci = TOSH_INTERFACE_2 GHCI_METHOD;
-		if (toshiba_acpi_setup_keyboard(TOSH_INTERFACE_2))
+		dev->method_hci = TOSH_INTERFACE_2 GHCI_METHOD;
+		if (toshiba_acpi_setup_keyboard(dev, TOSH_INTERFACE_2))
 			pr_info("Unable to activate hotkeys\n");
-	} else
-		return -ENODEV;
-
-	pr_info("Toshiba Laptop ACPI Extras version %s\n",
-	       TOSHIBA_ACPI_VERSION);
-	pr_info("    HCI method: %s\n", method_hci);
-
-	mutex_init(&toshiba_acpi.mutex);
-
-	toshiba_acpi.p_dev = platform_device_register_simple("toshiba_acpi",
-							      -1, NULL, 0);
-	if (IS_ERR(toshiba_acpi.p_dev)) {
-		ret = PTR_ERR(toshiba_acpi.p_dev);
-		pr_err("unable to register platform device\n");
-		toshiba_acpi.p_dev = NULL;
-		toshiba_acpi_exit();
-		return ret;
+	} else {
+		ret = -ENODEV;
+		goto error;
 	}
 
-	force_fan = 0;
-	key_event_valid = 0;
+	pr_info("HCI method: %s\n", dev->method_hci);
+
+	mutex_init(&dev->mutex);
 
 	/* enable event fifo */
-	hci_write1(HCI_SYSTEM_EVENT, 1, &hci_result);
+	hci_write1(dev, HCI_SYSTEM_EVENT, 1, &hci_result);
 
-	toshiba_proc_dir = proc_mkdir(PROC_TOSHIBA, acpi_root_dir);
-	if (!toshiba_proc_dir) {
-		toshiba_acpi_exit();
-		return -ENODEV;
-	} else {
-		create_toshiba_proc_entries();
-	}
+	create_toshiba_proc_entries(dev);
 
 	props.type = BACKLIGHT_PLATFORM;
 	props.max_brightness = HCI_LCD_BRIGHTNESS_LEVELS - 1;
-	toshiba_backlight_device = backlight_device_register("toshiba",
-							     &toshiba_acpi.p_dev->dev,
-							     NULL,
-							     &toshiba_backlight_data,
-							     &props);
-        if (IS_ERR(toshiba_backlight_device)) {
-		ret = PTR_ERR(toshiba_backlight_device);
+	dev->backlight_dev = backlight_device_register("toshiba",
+						       &acpi_dev->dev,
+						       dev,
+						       &toshiba_backlight_data,
+						       &props);
+	if (IS_ERR(dev->backlight_dev)) {
+		ret = PTR_ERR(dev->backlight_dev);
 
 		pr_err("Could not register toshiba backlight device\n");
-		toshiba_backlight_device = NULL;
-		toshiba_acpi_exit();
-		return ret;
+		dev->backlight_dev = NULL;
+		goto error;
 	}
 
 	/* Register rfkill switch for Bluetooth */
-	if (hci_get_bt_present(&bt_present) == HCI_SUCCESS && bt_present) {
-		toshiba_acpi.bt_rfk = rfkill_alloc(toshiba_acpi.bt_name,
-						   &toshiba_acpi.p_dev->dev,
-						   RFKILL_TYPE_BLUETOOTH,
-						   &toshiba_rfk_ops,
-						   &toshiba_acpi);
-		if (!toshiba_acpi.bt_rfk) {
+	if (hci_get_bt_present(dev, &bt_present) == HCI_SUCCESS && bt_present) {
+		dev->bt_rfk = rfkill_alloc("Toshiba Bluetooth",
+					   &acpi_dev->dev,
+					   RFKILL_TYPE_BLUETOOTH,
+					   &toshiba_rfk_ops,
+					   dev);
+		if (!dev->bt_rfk) {
 			pr_err("unable to allocate rfkill device\n");
-			toshiba_acpi_exit();
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto error;
 		}
 
-		ret = rfkill_register(toshiba_acpi.bt_rfk);
+		ret = rfkill_register(dev->bt_rfk);
 		if (ret) {
 			pr_err("unable to register rfkill device\n");
-			rfkill_destroy(toshiba_acpi.bt_rfk);
-			toshiba_acpi_exit();
-			return ret;
+			rfkill_destroy(dev->bt_rfk);
+			goto error;
 		}
 	}
 
-	toshiba_acpi.illumination_installed = 0;
-	if (toshiba_illumination_available()) {
-		if (!led_classdev_register(&(toshiba_acpi.p_dev->dev),
-					   &toshiba_led))
-			toshiba_acpi.illumination_installed = 1;
+	if (toshiba_illumination_available(dev)) {
+		dev->led_dev.name = "toshiba::illumination";
+		dev->led_dev.max_brightness = 1;
+		dev->led_dev.brightness_set = toshiba_illumination_set;
+		dev->led_dev.brightness_get = toshiba_illumination_get;
+		if (!led_classdev_register(&acpi_dev->dev, &dev->led_dev))
+			dev->illumination_installed = 1;
 	}
 
 	return 0;
+
+error:
+	toshiba_acpi_remove(acpi_dev, 0);
+	return ret;
+}
+
+static void toshiba_acpi_notify(struct acpi_device *acpi_dev, u32 event)
+{
+	struct toshiba_acpi_dev *dev = acpi_driver_data(acpi_dev);
+	u32 hci_result, value;
+
+	if (event != 0x80)
+		return;
+	do {
+		hci_read1(dev, HCI_SYSTEM_EVENT, &value, &hci_result);
+		if (hci_result == HCI_SUCCESS) {
+			if (value == 0x100)
+				continue;
+			/* act on key press; ignore key release */
+			if (value & 0x80)
+				continue;
+
+			if (!sparse_keymap_report_event(dev->hotkey_dev,
+							value, 1, true)) {
+				pr_info("Unknown key %x\n",
+				       value);
+			}
+		} else if (hci_result == HCI_NOT_SUPPORTED) {
+			/* This is a workaround for an unresolved issue on
+			 * some machines where system events sporadically
+			 * become disabled. */
+			hci_write1(dev, HCI_SYSTEM_EVENT, 1, &hci_result);
+			pr_notice("Re-enabled hotkeys\n");
+		}
+	} while (hci_result != HCI_EMPTY);
+}
+
+
+static struct acpi_driver toshiba_acpi_driver = {
+	.name	= "Toshiba ACPI driver",
+	.owner	= THIS_MODULE,
+	.ids	= toshiba_device_ids,
+	.flags	= ACPI_DRIVER_ALL_NOTIFY_EVENTS,
+	.ops	= {
+		.add		= toshiba_acpi_add,
+		.remove		= toshiba_acpi_remove,
+		.notify		= toshiba_acpi_notify,
+	},
+};
+
+static int __init toshiba_acpi_init(void)
+{
+	int ret;
+
+	toshiba_proc_dir = proc_mkdir(PROC_TOSHIBA, acpi_root_dir);
+	if (!toshiba_proc_dir) {
+		pr_err("Unable to create proc dir " PROC_TOSHIBA "\n");
+		return -ENODEV;
+	}
+
+	ret = acpi_bus_register_driver(&toshiba_acpi_driver);
+	if (ret) {
+		pr_err("Failed to register ACPI driver: %d\n", ret);
+		remove_proc_entry(PROC_TOSHIBA, acpi_root_dir);
+	}
+
+	return ret;
+}
+
+static void __exit toshiba_acpi_exit(void)
+{
+	acpi_bus_unregister_driver(&toshiba_acpi_driver);
+	if (toshiba_proc_dir)
+		remove_proc_entry(PROC_TOSHIBA, acpi_root_dir);
 }
 
 module_init(toshiba_acpi_init);
-- 
1.7.4.1

Re: [PATCH 1/6] toshiba_acpi: Convert to use acpi_driver

[Joe Perches]

On Tue, 2011-09-20 at 16:55 -0500, Seth Forshee wrote:
> Changes toshiba_acpi to register an acpi driver and eliminates the
> platform device it was using.

Just trivial comments...

> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> +static acpi_status hci_raw(struct toshiba_acpi_dev *dev,
> +			   const u32 in[HCI_WORDS], u32 out[HCI_WORDS])
[]
> @@ -237,85 +234,79 @@ static acpi_status hci_raw(const u32 in[HCI_WORDS], u32 out[HCI_WORDS])
>   * may be useful (such as "not supported").
>   */
>  
> -static acpi_status hci_write1(u32 reg, u32 in1, u32 * result)
> +static acpi_status hci_write1(struct toshiba_acpi_dev *dev, u32 reg,
> +			      u32 in1, u32 *result)
>  {
>  	u32 in[HCI_WORDS] = { HCI_SET, reg, in1, 0, 0, 0 };

A lot of the in[HCI_WORDS] could be static const or const

	static const u32 in[HCI_WORDS] etc...

> +static acpi_status hci_read1(struct toshiba_acpi_dev *dev, u32 reg,
> +			     u32 *out1, u32 *result)
>  {
>  	u32 in[HCI_WORDS] = { HCI_GET, reg, 0, 0, 0, 0 };

here too.

>  	u32 out[HCI_WORDS];
> -	acpi_status status = hci_raw(in, out);
> +	acpi_status status = hci_raw(dev, in, out);
>  	*out1 = out[2];
>  	*result = (status == AE_OK) ? out[0] : HCI_FAILURE;
>  	return status;
>  }
>  
> -static acpi_status hci_write2(u32 reg, u32 in1, u32 in2, u32 *result)
> +static acpi_status hci_write2(struct toshiba_acpi_dev *dev, u32 reg,
> +			      u32 in1, u32 in2, u32 *result)
>  {
>  	u32 in[HCI_WORDS] = { HCI_SET, reg, in1, in2, 0, 0 };

const etc.

> @@ -507,8 +486,13 @@ static int get_lcd(struct backlight_device *bd)
>  
>  static int lcd_proc_show(struct seq_file *m, void *v)
>  {
> -	int value = get_lcd(NULL);
> +	struct toshiba_acpi_dev *dev = m->private;
> +	int value;
> +
> +	if (!dev->backlight_dev)
> +		return -ENODEV;
>  
> +	value = get_lcd(dev->backlight_dev);
>  	if (value >= 0) {
>  		seq_printf(m, "brightness:              %d\n", value);
>  		seq_printf(m, "brightness_levels:       %d\n",

Some small amount space could be saved by using:

		seq_printf(m, "%-24s %d\n", "brightness:", value);
		seq_printf(m, "%-24s %d\n", "brightness_levels:", etc...
[]
> @@ -675,13 +663,14 @@ static const struct file_operations video_proc_fops = {
>  
>  static int fan_proc_show(struct seq_file *m, void *v)
>  {
> +	struct toshiba_acpi_dev *dev = m->private;
>  	u32 hci_result;
>  	u32 value;
>  
> -	hci_read1(HCI_FAN, &value, &hci_result);
> +	hci_read1(dev, HCI_FAN, &value, &hci_result);
>  	if (hci_result == HCI_SUCCESS) {
>  		seq_printf(m, "running:                 %d\n", (value > 0));

Here too:

		seq_printf(m, "%-24s %d\n", "running:", (value > 0);

> -		seq_printf(m, "force_on:                %d\n", force_fan);
> +		seq_printf(m, "force_on:                %d\n", dev->force_fan);

etc.

Re: [PATCH 1/6] toshiba_acpi: Convert to use acpi_driver

[Corentin Chary]

On Wed, Sep 21, 2011 at 12:17 AM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2011-09-20 at 16:55 -0500, Seth Forshee wrote:
>> Changes toshiba_acpi to register an acpi driver and eliminates the
>> platform device it was using.
>
> Just trivial comments...
>
>> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
>> +static acpi_status hci_raw(struct toshiba_acpi_dev *dev,
>> +                        const u32 in[HCI_WORDS], u32 out[HCI_WORDS])
> []
>> @@ -237,85 +234,79 @@ static acpi_status hci_raw(const u32 in[HCI_WORDS], u32 out[HCI_WORDS])
>>   * may be useful (such as "not supported").
>>   */
>>
>> -static acpi_status hci_write1(u32 reg, u32 in1, u32 * result)
>> +static acpi_status hci_write1(struct toshiba_acpi_dev *dev, u32 reg,
>> +                           u32 in1, u32 *result)
>>  {
>>       u32 in[HCI_WORDS] = { HCI_SET, reg, in1, 0, 0, 0 };
>
> A lot of the in[HCI_WORDS] could be static const or const
>
>        static const u32 in[HCI_WORDS] etc...

Not static const ! see how reg and in1 are used. const is ok.

>
>> +static acpi_status hci_read1(struct toshiba_acpi_dev *dev, u32 reg,
>> +                          u32 *out1, u32 *result)
>>  {
>>       u32 in[HCI_WORDS] = { HCI_GET, reg, 0, 0, 0, 0 };
>
> here too.
>
>>       u32 out[HCI_WORDS];
>> -     acpi_status status = hci_raw(in, out);
>> +     acpi_status status = hci_raw(dev, in, out);
>>       *out1 = out[2];
>>       *result = (status == AE_OK) ? out[0] : HCI_FAILURE;
>>       return status;
>>  }
>>
>> -static acpi_status hci_write2(u32 reg, u32 in1, u32 in2, u32 *result)
>> +static acpi_status hci_write2(struct toshiba_acpi_dev *dev, u32 reg,
>> +                           u32 in1, u32 in2, u32 *result)
>>  {
>>       u32 in[HCI_WORDS] = { HCI_SET, reg, in1, in2, 0, 0 };
>
> const etc.
>
>> @@ -507,8 +486,13 @@ static int get_lcd(struct backlight_device *bd)
>>
>>  static int lcd_proc_show(struct seq_file *m, void *v)
>>  {
>> -     int value = get_lcd(NULL);
>> +     struct toshiba_acpi_dev *dev = m->private;
>> +     int value;
>> +
>> +     if (!dev->backlight_dev)
>> +             return -ENODEV;
>>
>> +     value = get_lcd(dev->backlight_dev);
>>       if (value >= 0) {
>>               seq_printf(m, "brightness:              %d\n", value);
>>               seq_printf(m, "brightness_levels:       %d\n",
>
> Some small amount space could be saved by using:
>
>                seq_printf(m, "%-24s %d\n", "brightness:", value);
>                seq_printf(m, "%-24s %d\n", "brightness_levels:", etc...
> []
>> @@ -675,13 +663,14 @@ static const struct file_operations video_proc_fops = {
>>
>>  static int fan_proc_show(struct seq_file *m, void *v)
>>  {
>> +     struct toshiba_acpi_dev *dev = m->private;
>>       u32 hci_result;
>>       u32 value;
>>
>> -     hci_read1(HCI_FAN, &value, &hci_result);
>> +     hci_read1(dev, HCI_FAN, &value, &hci_result);
>>       if (hci_result == HCI_SUCCESS) {
>>               seq_printf(m, "running:                 %d\n", (value > 0));
>
> Here too:
>
>                seq_printf(m, "%-24s %d\n", "running:", (value > 0);
>
>> -             seq_printf(m, "force_on:                %d\n", force_fan);
>> +             seq_printf(m, "force_on:                %d\n", dev->force_fan);
>
> etc.
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Corentin Chary
http://xf.iksaif.net

Re: [PATCH 1/6] toshiba_acpi: Convert to use acpi_driver

[Seth Forshee]

On Tue, Sep 20, 2011 at 03:17:57PM -0700, Joe Perches wrote:
> On Tue, 2011-09-20 at 16:55 -0500, Seth Forshee wrote:
> > Changes toshiba_acpi to register an acpi driver and eliminates the
> > platform device it was using.
> 
> Just trivial comments...

Thanks for the suggestions. Since all those are existing code rather
than new code, I'll incorporate them if I have to respin the patches
(aside from the static problems that Corentin pointed out), otherwise
they can be done in a follow-on patch.

Re: [PATCH 1/6] toshiba_acpi: Convert to use acpi_driver

[Corentin Chary]

On Tue, Sep 20, 2011 at 11:55 PM, Seth Forshee
<seth.forshee@canonical.com> wrote:
> Changes toshiba_acpi to register an acpi driver and eliminates the
> platform device it was using.

Why to you want to remove the platform device ? If you want to create
a sysfs interface later, you'll probably need it. Most of the platform
driver I know only use the acpi_device to send proc/netlink events and
the platform_device is used everywhere else. (And anyway, it's an x86
*platform* driver, not a pure acpi driver).

> Also eliminates most global
> variables, moving them into toshiba_acpi_dev, along with some
> other miscellaneous fixes and cleanup.

Good ! Next step would be to deprecate the /proc interface (keeping it
for compatibility) and adding a new shinny
/sys/platform/device/toshiba-acpi/ interface correctly documented in
Documentation/ABI/ :).

Thanks,

-- 
Corentin Chary
http://xf.iksaif.net

Re: [PATCH 1/6] toshiba_acpi: Convert to use acpi_driver

[Seth Forshee]

On Wed, Sep 21, 2011 at 09:28:30AM +0200, Corentin Chary wrote:
> On Tue, Sep 20, 2011 at 11:55 PM, Seth Forshee
> <seth.forshee@canonical.com> wrote:
> > Changes toshiba_acpi to register an acpi driver and eliminates the
> > platform device it was using.
> 
> Why to you want to remove the platform device ? If you want to create
> a sysfs interface later, you'll probably need it. Most of the platform
> driver I know only use the acpi_device to send proc/netlink events and
> the platform_device is used everywhere else. (And anyway, it's an x86
> *platform* driver, not a pure acpi driver).

I removed the platform device because it seems a bit redundant to have
both, and I don't see what benefit it really provides. I see your point
that conceptually it makes sense to have it has a platform device,
although the distinction there is pretty fine.

Anyway, I guess the cost of keeping the platform device in place is
pretty small, so I can add it back in if that's desirable.

> > Also eliminates most global
> > variables, moving them into toshiba_acpi_dev, along with some
> > other miscellaneous fixes and cleanup.
> 
> Good ! Next step would be to deprecate the /proc interface (keeping it
> for compatibility) and adding a new shinny
> /sys/platform/device/toshiba-acpi/ interface correctly documented in
> Documentation/ABI/ :).

I was avoiding chainging any userspace interfaces in this first round of
patches, but deprecating the proc interface is definitely something I'd
like to do. I don't know if there's any point to moving it to sysfs
though. Most of it already has sysfs interfaces via device classes, and
I don't know that there's any value in the rest of it.

Thanks,
Seth

Re: [PATCH 1/6] toshiba_acpi: Convert to use acpi_driver

[Corentin Chary]

On Wed, Sep 21, 2011 at 3:24 PM, Seth Forshee
<seth.forshee@canonical.com> wrote:
> On Wed, Sep 21, 2011 at 09:28:30AM +0200, Corentin Chary wrote:
>> On Tue, Sep 20, 2011 at 11:55 PM, Seth Forshee
>> <seth.forshee@canonical.com> wrote:
>> > Changes toshiba_acpi to register an acpi driver and eliminates the
>> > platform device it was using.
>>
>> Why to you want to remove the platform device ? If you want to create
>> a sysfs interface later, you'll probably need it. Most of the platform
>> driver I know only use the acpi_device to send proc/netlink events and
>> the platform_device is used everywhere else. (And anyway, it's an x86
>> *platform* driver, not a pure acpi driver).
>
> I removed the platform device because it seems a bit redundant to have
> both, and I don't see what benefit it really provides. I see your point
> that conceptually it makes sense to have it has a platform device,
> although the distinction there is pretty fine.
>
> Anyway, I guess the cost of keeping the platform device in place is
> pretty small, so I can add it back in if that's desirable.

I don't have a strong opinion, so do what you want (or what Matthew
will tell you to), but keeping it would allow easier access to
subdevices (/sys/platform/devices is a better place than
/sys/bus/acpi/devices/ for this kind of device in my opinion).

>> > Also eliminates most global
>> > variables, moving them into toshiba_acpi_dev, along with some
>> > other miscellaneous fixes and cleanup.
>>
>> Good ! Next step would be to deprecate the /proc interface (keeping it
>> for compatibility) and adding a new shinny
>> /sys/platform/device/toshiba-acpi/ interface correctly documented in
>> Documentation/ABI/ :).
>
> I was avoiding chainging any userspace interfaces in this first round of
> patches, but deprecating the proc interface is definitely something I'd
> like to do. I don't know if there's any point to moving it to sysfs
> though. Most of it already has sysfs interfaces via device classes, and
> I don't know that there's any value in the rest of it.

Well, I must admit that I didn't check that, and it's possible that
all you need to do is to deprecate all /proc file if none of them are
actually usefull.

> Thanks,
> Seth
>



-- 
Corentin Chary
http://xf.iksaif.net

Re: [PATCH 1/6] toshiba_acpi: Convert to use acpi_driver

[Azael Avalos]

On Wed, Sep 21, 2011 at 7:30 AM, Corentin Chary
<corentin.chary@gmail.com> wrote:
> On Wed, Sep 21, 2011 at 3:24 PM, Seth Forshee
> <seth.forshee@canonical.com> wrote:
>> On Wed, Sep 21, 2011 at 09:28:30AM +0200, Corentin Chary wrote:
>>> On Tue, Sep 20, 2011 at 11:55 PM, Seth Forshee
>>> <seth.forshee@canonical.com> wrote:
>>> > Changes toshiba_acpi to register an acpi driver and eliminates the
>>> > platform device it was using.
>>>
>>> Why to you want to remove the platform device ? If you want to create
>>> a sysfs interface later, you'll probably need it. Most of the platform
>>> driver I know only use the acpi_device to send proc/netlink events and
>>> the platform_device is used everywhere else. (And anyway, it's an x86
>>> *platform* driver, not a pure acpi driver).
>>
>> I removed the platform device because it seems a bit redundant to have
>> both, and I don't see what benefit it really provides. I see your point
>> that conceptually it makes sense to have it has a platform device,
>> although the distinction there is pretty fine.
>>
>> Anyway, I guess the cost of keeping the platform device in place is
>> pretty small, so I can add it back in if that's desirable.
>
> I don't have a strong opinion, so do what you want (or what Matthew
> will tell you to), but keeping it would allow easier access to
> subdevices (/sys/platform/devices is a better place than
> /sys/bus/acpi/devices/ for this kind of device in my opinion).
>
>>> > Also eliminates most global
>>> > variables, moving them into toshiba_acpi_dev, along with some
>>> > other miscellaneous fixes and cleanup.
>>>
>>> Good ! Next step would be to deprecate the /proc interface (keeping it
>>> for compatibility) and adding a new shinny
>>> /sys/platform/device/toshiba-acpi/ interface correctly documented in
>>> Documentation/ABI/ :).
>>
>> I was avoiding chainging any userspace interfaces in this first round of
>> patches, but deprecating the proc interface is definitely something I'd
>> like to do. I don't know if there's any point to moving it to sysfs
>> though. Most of it already has sysfs interfaces via device classes, and
>> I don't know that there's any value in the rest of it.
>
> Well, I must admit that I didn't check that, and it's possible that
> all you need to do is to deprecate all /proc file if none of them are
> actually usefull.

They were useful, however, the new TOS1900 devices don't support any
of the calls /proc stuff is doing (fan, video, lcd, etc.), and so it seems
to be the new Toshiba standard, as I haven't seen any GHCI device
in a while ('tho I might be wrong...)

>
>> Thanks,
>> Seth
>>
>
>
>
> --
> Corentin Chary
> http://xf.iksaif.net
>



-- 
-- El mundo apesta y vosotros apestais tambien --

Re: [PATCH 1/6] toshiba_acpi: Convert to use acpi_driver

[Matthew Garrett]

On Wed, Sep 21, 2011 at 09:28:30AM +0200, Corentin Chary wrote:
> On Tue, Sep 20, 2011 at 11:55 PM, Seth Forshee
> <seth.forshee@canonical.com> wrote:
> > Changes toshiba_acpi to register an acpi driver and eliminates the
> > platform device it was using.
> 
> Why to you want to remove the platform device ? If you want to create
> a sysfs interface later, you'll probably need it. Most of the platform
> driver I know only use the acpi_device to send proc/netlink events and
> the platform_device is used everywhere else. (And anyway, it's an x86
> *platform* driver, not a pure acpi driver).

There's no hard requirement for a platform device, and given that it's 
purely using ACPI methods it arguably *is* a pure ACPI driver - Toshiba 
could use the same interface on an ARM device, if they were feeling 
excessively enthusiastic. But I've no strong feelings either way.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH 2/6] toshiba_acpi: Fix up return codes

[Seth Forshee]

Many routines are returning success on failure, and those that are
indicating failure frequently return incorrect error codes. Fix
these up throughout the driver.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/platform/x86/toshiba_acpi.c |   45 +++++++++++++++-------------------
 1 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index d74c97c..6b281c0 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -191,7 +191,7 @@ static int write_acpi_int(const char *methodName, int val)
 	in_objs[0].integer.value = val;
 
 	status = acpi_evaluate_object(NULL, (char *)methodName, &params, NULL);
-	return (status == AE_OK);
+	return (status == AE_OK) ? 0 : -EIO;
 }
 
 /* Perform a raw HCI call.  Here we don't care about input or output buffer
@@ -419,7 +419,7 @@ static int bt_rfkill_set_block(void *data, bool blocked)
 
 	mutex_lock(&dev->mutex);
 	if (hci_get_radio_state(dev, &radio_state) != HCI_SUCCESS) {
-		err = -EBUSY;
+		err = -EIO;
 		goto out;
 	}
 
@@ -432,7 +432,7 @@ static int bt_rfkill_set_block(void *data, bool blocked)
 	hci_write2(dev, HCI_WIRELESS, value, HCI_WIRELESS_BT_ATTACH, &result2);
 
 	if (result1 != HCI_SUCCESS || result2 != HCI_SUCCESS)
-		err = -EBUSY;
+		err = -EIO;
 	else
 		err = 0;
  out:
@@ -478,10 +478,10 @@ static int get_lcd(struct backlight_device *bd)
 	u32 value;
 
 	hci_read1(dev, HCI_LCD_BRIGHTNESS, &value, &hci_result);
-	if (hci_result == HCI_SUCCESS) {
+	if (hci_result == HCI_SUCCESS)
 		return (value >> HCI_LCD_BRIGHTNESS_SHIFT);
-	} else
-		return -EFAULT;
+
+	return -EIO;
 }
 
 static int lcd_proc_show(struct seq_file *m, void *v)
@@ -497,11 +497,11 @@ static int lcd_proc_show(struct seq_file *m, void *v)
 		seq_printf(m, "brightness:              %d\n", value);
 		seq_printf(m, "brightness_levels:       %d\n",
 			     HCI_LCD_BRIGHTNESS_LEVELS);
-	} else {
-		pr_err("Error reading LCD brightness\n");
+		return 0;
 	}
 
-	return 0;
+	pr_err("Error reading LCD brightness\n");
+	return -EIO;
 }
 
 static int lcd_proc_open(struct inode *inode, struct file *file)
@@ -515,10 +515,7 @@ static int set_lcd(struct toshiba_acpi_dev *dev, int value)
 
 	value = value << HCI_LCD_BRIGHTNESS_SHIFT;
 	hci_write1(dev, HCI_LCD_BRIGHTNESS, value, &hci_result);
-	if (hci_result != HCI_SUCCESS)
-		return -EFAULT;
-
-	return 0;
+	return hci_result == HCI_SUCCESS ? 0 : -EIO;
 }
 
 static int set_lcd_status(struct backlight_device *bd)
@@ -575,11 +572,10 @@ static int video_proc_show(struct seq_file *m, void *v)
 		seq_printf(m, "lcd_out:                 %d\n", is_lcd);
 		seq_printf(m, "crt_out:                 %d\n", is_crt);
 		seq_printf(m, "tv_out:                  %d\n", is_tv);
-	} else {
-		pr_err("Error reading video out status\n");
+		return 0;
 	}
 
-	return 0;
+	return -EIO;
 }
 
 static int video_proc_open(struct inode *inode, struct file *file)
@@ -592,6 +588,7 @@ static ssize_t video_proc_write(struct file *file, const char __user *buf,
 {
 	struct toshiba_acpi_dev *dev = PDE(file->f_path.dentry->d_inode)->data;
 	char *cmd, *buffer;
+	int ret = 0;
 	int value;
 	int remain = count;
 	int lcd_out = -1;
@@ -644,12 +641,12 @@ static ssize_t video_proc_write(struct file *file, const char __user *buf,
 		/* To avoid unnecessary video disruption, only write the new
 		 * video setting if something changed. */
 		if (new_video_out != video_out)
-			write_acpi_int(METHOD_VIDEO_OUT, new_video_out);
+			ret = write_acpi_int(METHOD_VIDEO_OUT, new_video_out);
 	} else {
-		return -EFAULT;
+		ret = -EIO;
 	}
 
-	return count;
+	return ret ? ret : count;
 }
 
 static const struct file_operations video_proc_fops = {
@@ -671,11 +668,10 @@ static int fan_proc_show(struct seq_file *m, void *v)
 	if (hci_result == HCI_SUCCESS) {
 		seq_printf(m, "running:                 %d\n", (value > 0));
 		seq_printf(m, "force_on:                %d\n", dev->force_fan);
-	} else {
-		pr_err("Error reading fan status\n");
+		return 0;
 	}
 
-	return 0;
+	return -EIO;
 }
 
 static int fan_proc_open(struct inode *inode, struct file *file)
@@ -701,7 +697,7 @@ static ssize_t fan_proc_write(struct file *file, const char __user *buf,
 	    value >= 0 && value <= 1) {
 		hci_write1(dev, HCI_FAN, value, &hci_result);
 		if (hci_result != HCI_SUCCESS)
-			return -EFAULT;
+			return -EIO;
 		else
 			dev->force_fan = value;
 	} else {
@@ -741,13 +737,12 @@ static int keys_proc_show(struct seq_file *m, void *v)
 			pr_notice("Re-enabled hotkeys\n");
 		} else {
 			pr_err("Error reading hotkey status\n");
-			goto end;
+			return -EIO;
 		}
 	}
 
 	seq_printf(m, "hotkey_ready:            %d\n", dev->key_event_valid);
 	seq_printf(m, "hotkey:                  0x%04x\n", dev->last_key_event);
-end:
 	return 0;
 }
 
-- 
1.7.4.1

[PATCH 3/6] toshiba_acpi: Use handle for HCI calls

[Seth Forshee]

Now that we're using an acpi driver we already have a handle to
the namespace of the HCI call, so there's no need to test various
paths to the HCI call or even be aware of the path at all.

Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/platform/x86/toshiba_acpi.c |   55 +++++++++-------------------------
 1 files changed, 15 insertions(+), 40 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 6b281c0..a924c7f 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -62,11 +62,7 @@ MODULE_DESCRIPTION("Toshiba Laptop ACPI Extras Driver");
 MODULE_LICENSE("GPL");
 
 /* Toshiba ACPI method paths */
-#define METHOD_LCD_BRIGHTNESS	"\\_SB_.PCI0.VGA_.LCD_._BCM"
-#define TOSH_INTERFACE_1	"\\_SB_.VALD"
-#define TOSH_INTERFACE_2	"\\_SB_.VALZ"
 #define METHOD_VIDEO_OUT	"\\_SB_.VALX.DSSX"
-#define GHCI_METHOD		".GHCI"
 
 /* Toshiba HCI interface definitions
  *
@@ -121,7 +117,6 @@ struct toshiba_acpi_dev {
 	int force_fan;
 	int last_key_event;
 	int key_event_valid;
-	acpi_handle handle;
 
 	struct mutex mutex;
 };
@@ -170,15 +165,6 @@ static __inline__ void _set_bit(u32 * word, u32 mask, int value)
 /* acpi interface wrappers
  */
 
-static int is_valid_acpi_path(const char *methodName)
-{
-	acpi_handle handle;
-	acpi_status status;
-
-	status = acpi_get_handle(NULL, (char *)methodName, &handle);
-	return !ACPI_FAILURE(status);
-}
-
 static int write_acpi_int(const char *methodName, int val)
 {
 	struct acpi_object_list params;
@@ -217,7 +203,8 @@ static acpi_status hci_raw(struct toshiba_acpi_dev *dev,
 	results.length = sizeof(out_objs);
 	results.pointer = out_objs;
 
-	status = acpi_evaluate_object(NULL, (char *)dev->method_hci, &params,
+	status = acpi_evaluate_object(dev->acpi_dev->handle,
+				      (char *)dev->method_hci, &params,
 				      &results);
 	if ((status == AE_OK) && (out_objs->package.count <= HCI_WORDS)) {
 		for (i = 0; i < out_objs->package.count; ++i) {
@@ -836,18 +823,11 @@ static const struct backlight_ops toshiba_backlight_data = {
         .update_status  = set_lcd_status,
 };
 
-static int __devinit toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev,
-						 char *device_path)
+static int __devinit toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev)
 {
 	acpi_status status;
 	int error;
 
-	status = acpi_get_handle(NULL, device_path, &dev->handle);
-	if (ACPI_FAILURE(status)) {
-		pr_info("Unable to get notification device\n");
-		return -ENODEV;
-	}
-
 	dev->hotkey_dev = input_allocate_device();
 	if (!dev->hotkey_dev) {
 		pr_info("Unable to register input device\n");
@@ -855,14 +835,14 @@ static int __devinit toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev,
 	}
 
 	dev->hotkey_dev->name = "Toshiba input device";
-	dev->hotkey_dev->phys = device_path;
+	dev->hotkey_dev->phys = "toshiba_acpi/input0";
 	dev->hotkey_dev->id.bustype = BUS_HOST;
 
 	error = sparse_keymap_setup(dev->hotkey_dev, toshiba_acpi_keymap, NULL);
 	if (error)
 		goto err_free_dev;
 
-	status = acpi_evaluate_object(dev->handle, "ENAB", NULL, NULL);
+	status = acpi_evaluate_object(dev->acpi_dev->handle, "ENAB", NULL, NULL);
 	if (ACPI_FAILURE(status)) {
 		pr_info("Unable to enable hotkeys\n");
 		error = -ENODEV;
@@ -915,6 +895,8 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev, int type)
 static int __devinit toshiba_acpi_add(struct acpi_device *acpi_dev)
 {
 	struct toshiba_acpi_dev *dev;
+	acpi_status status;
+	acpi_handle handle;
 	u32 hci_result;
 	bool bt_present;
 	int ret = 0;
@@ -923,27 +905,20 @@ static int __devinit toshiba_acpi_add(struct acpi_device *acpi_dev)
 	pr_info("Toshiba Laptop ACPI Extras version %s\n",
 	       TOSHIBA_ACPI_VERSION);
 
+	/* simple device detection: look for HCI method */
+	status = acpi_get_handle(acpi_dev->handle, "GHCI", &handle);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		return -ENOMEM;
 	dev->acpi_dev = acpi_dev;
+	dev->method_hci = "GHCI";
 	acpi_dev->driver_data = dev;
 
-	/* simple device detection: look for HCI method */
-	if (is_valid_acpi_path(TOSH_INTERFACE_1 GHCI_METHOD)) {
-		dev->method_hci = TOSH_INTERFACE_1 GHCI_METHOD;
-		if (toshiba_acpi_setup_keyboard(dev, TOSH_INTERFACE_1))
-			pr_info("Unable to activate hotkeys\n");
-	} else if (is_valid_acpi_path(TOSH_INTERFACE_2 GHCI_METHOD)) {
-		dev->method_hci = TOSH_INTERFACE_2 GHCI_METHOD;
-		if (toshiba_acpi_setup_keyboard(dev, TOSH_INTERFACE_2))
-			pr_info("Unable to activate hotkeys\n");
-	} else {
-		ret = -ENODEV;
-		goto error;
-	}
-
-	pr_info("HCI method: %s\n", dev->method_hci);
+	if (toshiba_acpi_setup_keyboard(dev))
+		pr_info("Unable to activate hotkeys\n");
 
 	mutex_init(&dev->mutex);
 
-- 
1.7.4.1

[PATCH 4/6] toshiba_acpi: Support SPFC as an HCI method

[Seth Forshee]

Some Toshiba models, notably those with the TOS1900 device, use
the SPFC method for HCI calls instead of GHCI. Test for this method
if GHCI isn't found, and if it exists use it for all HCI calls.

Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/platform/x86/toshiba_acpi.c |   28 ++++++++++++++++++++++------
 1 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index a924c7f..cc629e6 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -892,11 +892,26 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev, int type)
 	return 0;
 }
 
+static const char * __devinit find_hci_method(acpi_handle handle)
+{
+	acpi_status status;
+	acpi_handle hci_handle;
+
+	status = acpi_get_handle(handle, "GHCI", &hci_handle);
+	if (ACPI_SUCCESS(status))
+		return "GHCI";
+
+	status = acpi_get_handle(handle, "SPFC", &hci_handle);
+	if (ACPI_SUCCESS(status))
+		return "SPFC";
+
+	return NULL;
+}
+
 static int __devinit toshiba_acpi_add(struct acpi_device *acpi_dev)
 {
 	struct toshiba_acpi_dev *dev;
-	acpi_status status;
-	acpi_handle handle;
+	const char *hci_method;
 	u32 hci_result;
 	bool bt_present;
 	int ret = 0;
@@ -905,16 +920,17 @@ static int __devinit toshiba_acpi_add(struct acpi_device *acpi_dev)
 	pr_info("Toshiba Laptop ACPI Extras version %s\n",
 	       TOSHIBA_ACPI_VERSION);
 
-	/* simple device detection: look for HCI method */
-	status = acpi_get_handle(acpi_dev->handle, "GHCI", &handle);
-	if (ACPI_FAILURE(status))
+	hci_method = find_hci_method(acpi_dev->handle);
+	if (!hci_method) {
+		pr_err("HCI interface not found\n");
 		return -ENODEV;
+	}
 
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		return -ENOMEM;
 	dev->acpi_dev = acpi_dev;
-	dev->method_hci = "GHCI";
+	dev->method_hci = hci_method;
 	acpi_dev->driver_data = dev;
 
 	if (toshiba_acpi_setup_keyboard(dev))
-- 
1.7.4.1

[PATCH 5/6] toshiba_acpi: Don't add devices for unsupported features

[Seth Forshee]

Test for features up-front to determine whether or not they are
supported, and avoid creating devices and proc files for
unsupported features.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/platform/x86/toshiba_acpi.c |  102 +++++++++++++++++++++++------------
 1 files changed, 67 insertions(+), 35 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index cc629e6..c51a64c 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -113,11 +113,15 @@ struct toshiba_acpi_dev {
 	struct input_dev *hotkey_dev;
 	struct backlight_device *backlight_dev;
 	struct led_classdev led_dev;
-	int illumination_installed;
+
 	int force_fan;
 	int last_key_event;
 	int key_event_valid;
 
+	int illumination_supported:1;
+	int video_supported:1;
+	int fan_supported:1;
+
 	struct mutex mutex;
 };
 
@@ -545,24 +549,31 @@ static const struct file_operations lcd_proc_fops = {
 	.write		= lcd_proc_write,
 };
 
+static int get_video_status(struct toshiba_acpi_dev *dev, u32 *status)
+{
+	u32 hci_result;
+
+	hci_read1(dev, HCI_VIDEO_OUT, status, &hci_result);
+	return hci_result == HCI_SUCCESS ? 0 : -EIO;
+}
+
 static int video_proc_show(struct seq_file *m, void *v)
 {
 	struct toshiba_acpi_dev *dev = m->private;
-	u32 hci_result;
 	u32 value;
+	int ret;
 
-	hci_read1(dev, HCI_VIDEO_OUT, &value, &hci_result);
-	if (hci_result == HCI_SUCCESS) {
+	ret = get_video_status(dev, &value);
+	if (!ret) {
 		int is_lcd = (value & HCI_VIDEO_OUT_LCD) ? 1 : 0;
 		int is_crt = (value & HCI_VIDEO_OUT_CRT) ? 1 : 0;
 		int is_tv = (value & HCI_VIDEO_OUT_TV) ? 1 : 0;
 		seq_printf(m, "lcd_out:                 %d\n", is_lcd);
 		seq_printf(m, "crt_out:                 %d\n", is_crt);
 		seq_printf(m, "tv_out:                  %d\n", is_tv);
-		return 0;
 	}
 
-	return -EIO;
+	return ret;
 }
 
 static int video_proc_open(struct inode *inode, struct file *file)
@@ -575,13 +586,12 @@ static ssize_t video_proc_write(struct file *file, const char __user *buf,
 {
 	struct toshiba_acpi_dev *dev = PDE(file->f_path.dentry->d_inode)->data;
 	char *cmd, *buffer;
-	int ret = 0;
+	int ret;
 	int value;
 	int remain = count;
 	int lcd_out = -1;
 	int crt_out = -1;
 	int tv_out = -1;
-	u32 hci_result;
 	u32 video_out;
 
 	cmd = kmalloc(count + 1, GFP_KERNEL);
@@ -616,8 +626,8 @@ static ssize_t video_proc_write(struct file *file, const char __user *buf,
 
 	kfree(cmd);
 
-	hci_read1(dev, HCI_VIDEO_OUT, &video_out, &hci_result);
-	if (hci_result == HCI_SUCCESS) {
+	ret = get_video_status(dev, &video_out);
+	if (!ret) {
 		unsigned int new_video_out = video_out;
 		if (lcd_out != -1)
 			_set_bit(&new_video_out, HCI_VIDEO_OUT_LCD, lcd_out);
@@ -629,8 +639,6 @@ static ssize_t video_proc_write(struct file *file, const char __user *buf,
 		 * video setting if something changed. */
 		if (new_video_out != video_out)
 			ret = write_acpi_int(METHOD_VIDEO_OUT, new_video_out);
-	} else {
-		ret = -EIO;
 	}
 
 	return ret ? ret : count;
@@ -645,20 +653,27 @@ static const struct file_operations video_proc_fops = {
 	.write		= video_proc_write,
 };
 
+static int get_fan_status(struct toshiba_acpi_dev *dev, u32 *status)
+{
+	u32 hci_result;
+
+	hci_read1(dev, HCI_FAN, status, &hci_result);
+	return hci_result == HCI_SUCCESS ? 0 : -EIO;
+}
+
 static int fan_proc_show(struct seq_file *m, void *v)
 {
 	struct toshiba_acpi_dev *dev = m->private;
-	u32 hci_result;
+	int ret;
 	u32 value;
 
-	hci_read1(dev, HCI_FAN, &value, &hci_result);
-	if (hci_result == HCI_SUCCESS) {
+	ret = get_fan_status(dev, &value);
+	if (!ret) {
 		seq_printf(m, "running:                 %d\n", (value > 0));
 		seq_printf(m, "force_on:                %d\n", dev->force_fan);
-		return 0;
 	}
 
-	return -EIO;
+	return ret;
 }
 
 static int fan_proc_open(struct inode *inode, struct file *file)
@@ -797,24 +812,32 @@ static const struct file_operations version_proc_fops = {
 static void __devinit
 create_toshiba_proc_entries(struct toshiba_acpi_dev *dev)
 {
-	proc_create_data("lcd", S_IRUGO | S_IWUSR, toshiba_proc_dir,
-			 &lcd_proc_fops, dev);
-	proc_create_data("video", S_IRUGO | S_IWUSR, toshiba_proc_dir,
-			 &video_proc_fops, dev);
-	proc_create_data("fan", S_IRUGO | S_IWUSR, toshiba_proc_dir,
-			 &fan_proc_fops, dev);
-	proc_create_data("keys", S_IRUGO | S_IWUSR, toshiba_proc_dir,
-			 &keys_proc_fops, dev);
+	if (dev->backlight_dev)
+		proc_create_data("lcd", S_IRUGO | S_IWUSR, toshiba_proc_dir,
+				 &lcd_proc_fops, dev);
+	if (dev->video_supported)
+		proc_create_data("video", S_IRUGO | S_IWUSR, toshiba_proc_dir,
+				 &video_proc_fops, dev);
+	if (dev->fan_supported)
+		proc_create_data("fan", S_IRUGO | S_IWUSR, toshiba_proc_dir,
+				 &fan_proc_fops, dev);
+	if (dev->hotkey_dev)
+		proc_create_data("keys", S_IRUGO | S_IWUSR, toshiba_proc_dir,
+				 &keys_proc_fops, dev);
 	proc_create_data("version", S_IRUGO, toshiba_proc_dir,
 			 &version_proc_fops, dev);
 }
 
-static void remove_toshiba_proc_entries(void)
+static void remove_toshiba_proc_entries(struct toshiba_acpi_dev *dev)
 {
-	remove_proc_entry("lcd", toshiba_proc_dir);
-	remove_proc_entry("video", toshiba_proc_dir);
-	remove_proc_entry("fan", toshiba_proc_dir);
-	remove_proc_entry("keys", toshiba_proc_dir);
+	if (dev->backlight_dev)
+		remove_proc_entry("lcd", toshiba_proc_dir);
+	if (dev->video_supported)
+		remove_proc_entry("video", toshiba_proc_dir);
+	if (dev->fan_supported)
+		remove_proc_entry("fan", toshiba_proc_dir);
+	if (dev->hotkey_dev)
+		remove_proc_entry("keys", toshiba_proc_dir);
 	remove_proc_entry("version", toshiba_proc_dir);
 }
 
@@ -869,7 +892,7 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev, int type)
 {
 	struct toshiba_acpi_dev *dev = acpi_driver_data(acpi_dev);
 
-	remove_toshiba_proc_entries();
+	remove_toshiba_proc_entries(dev);
 
 	if (dev->hotkey_dev) {
 		input_unregister_device(dev->hotkey_dev);
@@ -884,7 +907,7 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev, int type)
 	if (dev->backlight_dev)
 		backlight_device_unregister(dev->backlight_dev);
 
-	if (dev->illumination_installed)
+	if (dev->illumination_supported)
 		led_classdev_unregister(&dev->led_dev);
 
 	kfree(dev);
@@ -913,6 +936,7 @@ static int __devinit toshiba_acpi_add(struct acpi_device *acpi_dev)
 	struct toshiba_acpi_dev *dev;
 	const char *hci_method;
 	u32 hci_result;
+	u32 dummy;
 	bool bt_present;
 	int ret = 0;
 	struct backlight_properties props;
@@ -941,8 +965,6 @@ static int __devinit toshiba_acpi_add(struct acpi_device *acpi_dev)
 	/* enable event fifo */
 	hci_write1(dev, HCI_SYSTEM_EVENT, 1, &hci_result);
 
-	create_toshiba_proc_entries(dev);
-
 	props.type = BACKLIGHT_PLATFORM;
 	props.max_brightness = HCI_LCD_BRIGHTNESS_LEVELS - 1;
 	dev->backlight_dev = backlight_device_register("toshiba",
@@ -985,9 +1007,19 @@ static int __devinit toshiba_acpi_add(struct acpi_device *acpi_dev)
 		dev->led_dev.brightness_set = toshiba_illumination_set;
 		dev->led_dev.brightness_get = toshiba_illumination_get;
 		if (!led_classdev_register(&acpi_dev->dev, &dev->led_dev))
-			dev->illumination_installed = 1;
+			dev->illumination_supported = 1;
 	}
 
+	/* Determine whether or not BIOS supports fan and video interfaces */
+
+	ret = get_video_status(dev, &dummy);
+	dev->video_supported = !ret;
+
+	ret = get_fan_status(dev, &dummy);
+	dev->fan_supported = !ret;
+
+	create_toshiba_proc_entries(dev);
+
 	return 0;
 
 error:
-- 
1.7.4.1

[PATCH 6/6] toshiba_acpi: Initialize brightness in backlight device

[Seth Forshee]

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/platform/x86/toshiba_acpi.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index c51a64c..13ef8c3 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -979,6 +979,7 @@ static int __devinit toshiba_acpi_add(struct acpi_device *acpi_dev)
 		dev->backlight_dev = NULL;
 		goto error;
 	}
+	dev->backlight_dev->props.brightness = get_lcd(dev->backlight_dev);
 
 	/* Register rfkill switch for Bluetooth */
 	if (hci_get_bt_present(dev, &bt_present) == HCI_SUCCESS && bt_present) {
-- 
1.7.4.1

Re: [PATCH 0/6] toshiba_acpi: Cleanup and TOS1900 device support

[Matthew Garrett]

On Tue, Sep 20, 2011 at 04:55:48PM -0500, Seth Forshee wrote:
> Hi Matthew,
> 
> Please consider merging the following patches. They represent a
> signficant cleanup of toshiba_acpi as well as adding support for SPFC as
> an HCI method. The latter provides basic but functional support for the
> TOS1900 devices found on newer Toshiba models, which will serve as a
> good starting point for further enhancements. I've tried to leave the
> details of the hardware interaction unchanged to reduce the risk of
> regression for machines already supported by the driver.

Great. Has this been tested on any older hardware? I'd think that the 
risk of regression is pretty low, but it'd be a nice sanity check. I've 
only got one TOS1800 type around here, and it's a 1.2GHz Pentium M, so 
I'd prefer to have got someone else to do it :)

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 0/6] toshiba_acpi: Cleanup and TOS1900 device support

[Seth Forshee]

On Wed, Sep 21, 2011 at 04:31:37PM +0100, Matthew Garrett wrote:
> On Tue, Sep 20, 2011 at 04:55:48PM -0500, Seth Forshee wrote:
> > Hi Matthew,
> > 
> > Please consider merging the following patches. They represent a
> > signficant cleanup of toshiba_acpi as well as adding support for SPFC as
> > an HCI method. The latter provides basic but functional support for the
> > TOS1900 devices found on newer Toshiba models, which will serve as a
> > good starting point for further enhancements. I've tried to leave the
> > details of the hardware interaction unchanged to reduce the risk of
> > regression for machines already supported by the driver.
> 
> Great. Has this been tested on any older hardware? I'd think that the 
> risk of regression is pretty low, but it'd be a nice sanity check. I've 
> only got one TOS1800 type around here, and it's a 1.2GHz Pentium M, so 
> I'd prefer to have got someone else to do it :)

No, I don't have any older hardware to test on, and I'm pretty sure
Azael doesn't either. Not that I want to afflict any additional pain
beyond what you already have from dealing with firmware all day ;)

Re: [PATCH 0/6] toshiba_acpi: Cleanup and TOS1900 device support

[Seth Forshee]

On Wed, Sep 21, 2011 at 11:10:44AM -0500, Seth Forshee wrote:
> On Wed, Sep 21, 2011 at 04:31:37PM +0100, Matthew Garrett wrote:
> > On Tue, Sep 20, 2011 at 04:55:48PM -0500, Seth Forshee wrote:
> > > Hi Matthew,
> > > 
> > > Please consider merging the following patches. They represent a
> > > signficant cleanup of toshiba_acpi as well as adding support for SPFC as
> > > an HCI method. The latter provides basic but functional support for the
> > > TOS1900 devices found on newer Toshiba models, which will serve as a
> > > good starting point for further enhancements. I've tried to leave the
> > > details of the hardware interaction unchanged to reduce the risk of
> > > regression for machines already supported by the driver.
> > 
> > Great. Has this been tested on any older hardware? I'd think that the 
> > risk of regression is pretty low, but it'd be a nice sanity check. I've 
> > only got one TOS1800 type around here, and it's a 1.2GHz Pentium M, so 
> > I'd prefer to have got someone else to do it :)
> 
> No, I don't have any older hardware to test on, and I'm pretty sure
> Azael doesn't either. Not that I want to afflict any additional pain
> beyond what you already have from dealing with firmware all day ;)

I managed to find some volunteers with machines that use the GHCI
interface to do some regression testing for me. They aren't exactly
older hardware, all of them with TOS6208 and being made within the last
2 or 3 years. But it's the best I could do.

None of the machines tested showed any regressions as a result of these
patches. I also got testing of using the INFO method to read hotkey
scancodes, and that's working on those machines as well. (I did however
find that I had to reintroduce the global toshiba_acpi pointer for the
i8042 filter, since it doesn't have any mechanism for passing private
data to the callback.)

Any chance of seeing these patches in 3.2?

Re: [PATCH 0/6] toshiba_acpi: Cleanup and TOS1900 device support

[Azael Avalos]

> I managed to find some volunteers with machines that use the GHCI
> interface to do some regression testing for me. They aren't exactly
> older hardware, all of them with TOS6208 and being made within the last
> 2 or 3 years. But it's the best I could do.

Great, I was planing on buying an older laptop from eBay, so I could test
my changes on "older" hardware, I might still get it, "to eat my own
cookies".

>
> None of the machines tested showed any regressions as a result of these
> patches. I also got testing of using the INFO method to read hotkey
> scancodes, and that's working on those machines as well. (I did however
> find that I had to reintroduce the global toshiba_acpi pointer for the
> i8042 filter, since it doesn't have any mechanism for passing private
> data to the callback.)

Great news, hope to see your changes merged Seth


Saludos
Azael


-- 
-- El mundo apesta y vosotros apestais tambien --