2015-07-28 01:22:45

by Azael Avalos

[permalink] [raw]
Subject: [PATCH v2] toshiba_acpi: Add set_fan_status function

This patch adds a new function named "set_fan_status" to complement
its get* counterpart, as well as to avoid code duplication between
"fan_proc_write" and "fan_store".

Also, both functions (get*, set*) are now checking for TOS_FAILURE,
TOS_NOT_SUPPORTED and TOS_SUCCESS (to be on par with the rest of the
HCI/SCI functions), printing an error message, returning -ENODEV and
zero respectively.

The proc and sysfs functions were updated to reflect these changes as
well, returning -EIO for proc, and propagating the error value on the
sysfs functions.

Signed-off-by: Azael Avalos <[email protected]>
---
Changes since v1:
- Updated patch description
- Updated proc and sysfs functions to the changes made on this patch
to ensure not to break userspace

drivers/platform/x86/toshiba_acpi.c | 66 ++++++++++++++++++++++---------------
1 file changed, 40 insertions(+), 26 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 3bfdfdd..f722898 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -1422,27 +1422,47 @@ static const struct file_operations video_proc_fops = {
.write = video_proc_write,
};

+/* Fan status */
static int get_fan_status(struct toshiba_acpi_dev *dev, u32 *status)
{
- u32 hci_result;
+ u32 result = hci_read(dev, HCI_FAN, status);

- hci_result = hci_read(dev, HCI_FAN, status);
- return hci_result == TOS_SUCCESS ? 0 : -EIO;
+ if (result == TOS_FAILURE)
+ pr_err("ACPI call to get Fan status failed\n");
+ else if (result == TOS_NOT_SUPPORTED)
+ return -ENODEV;
+ else if (result == TOS_SUCCESS)
+ return 0;
+
+ return -EIO;
+}
+
+static int set_fan_status(struct toshiba_acpi_dev *dev, u32 status)
+{
+ u32 result = hci_write(dev, HCI_FAN, status);
+
+ if (result == TOS_FAILURE)
+ pr_err("ACPI call to set Fan status failed\n");
+ else if (result == TOS_NOT_SUPPORTED)
+ return -ENODEV;
+ else if (result == TOS_SUCCESS)
+ return 0;
+
+ return -EIO;
}

static int fan_proc_show(struct seq_file *m, void *v)
{
struct toshiba_acpi_dev *dev = m->private;
- int ret;
u32 value;

- 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);
- }
+ if (get_fan_status(dev, &value))
+ return -EIO;

- return ret;
+ seq_printf(m, "running: %d\n", (value > 0));
+ seq_printf(m, "force_on: %d\n", dev->force_fan);
+
+ return 0;
}

static int fan_proc_open(struct inode *inode, struct file *file)
@@ -1457,23 +1477,20 @@ static ssize_t fan_proc_write(struct file *file, const char __user *buf,
char cmd[42];
size_t len;
int value;
- u32 hci_result;

len = min(count, sizeof(cmd) - 1);
if (copy_from_user(cmd, buf, len))
return -EFAULT;
cmd[len] = '\0';

- if (sscanf(cmd, " force_on : %i", &value) == 1 &&
- value >= 0 && value <= 1) {
- hci_result = hci_write(dev, HCI_FAN, value);
- if (hci_result == TOS_SUCCESS)
- dev->force_fan = value;
- else
- return -EIO;
- } else {
+ if (sscanf(cmd, " force_on : %i", &value) != 1 &&
+ value != 0 && value != 1)
return -EINVAL;
- }
+
+ if (set_fan_status(dev, value))
+ return -EIO;
+
+ dev->force_fan = value;

return count;
}
@@ -1610,7 +1627,6 @@ static ssize_t fan_store(struct device *dev,
const char *buf, size_t count)
{
struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
- u32 result;
int state;
int ret;

@@ -1621,11 +1637,9 @@ static ssize_t fan_store(struct device *dev,
if (state != 0 && state != 1)
return -EINVAL;

- result = hci_write(toshiba, HCI_FAN, state);
- if (result == TOS_FAILURE)
- return -EIO;
- else if (result == TOS_NOT_SUPPORTED)
- return -ENODEV;
+ ret = set_fan_status(toshiba, state);
+ if (ret)
+ return ret;

return count;
}
--
2.4.6


2015-07-28 01:22:49

by Azael Avalos

[permalink] [raw]
Subject: [PATCH v2 0/4] toshiba_acpi: Refactor *{get, set} and *available functions

These patches changes the *{get, set} and *available functions,
changes the printed messages of the supported features and bumps the
driver version to 0.23.

Changes since v1:
- Fixed typos in patch 01 description and use newly created
*led_registered variables to unregister leds
- Adapt *{lcd, video}_proc functions in patch 03 to ensure we are not
breaking userspace and updated patch description

Azael Avalos (4):
toshiba_acpi: Change *available functions return type
toshiba_acpi: Remove "*not supported" feature prints
toshiba_acpi: Refactor *{get, set} functions return value
toshiba_acpi: Bump driver version to 0.23

drivers/platform/x86/toshiba_acpi.c | 643 ++++++++++++++++++------------------
1 file changed, 322 insertions(+), 321 deletions(-)

--
2.4.6

2015-07-28 01:22:52

by Azael Avalos

[permalink] [raw]
Subject: [PATCH v2 1/4] toshiba_acpi: Change *available functions return type

This patch changes the *available functions return type from int to
void.

The checks for support of their respective features are done inside
such functions and there was no need to return anything as we can
flag the queried feature as supported inside these functions.

The code was adapted accordingly to these changes and two new
variables were created and another was changed from uint to bool.

While at the neighborhood, some of the functions were cleaned from
unnecessary checks, as we are explicitly checking for TOS_SUCCESS,
and also renames the function *acceleremoter_supported to
*accelerometer_available.

Signed-off-by: Azael Avalos <[email protected]>
---
drivers/platform/x86/toshiba_acpi.c | 142 ++++++++++++++++--------------------
1 file changed, 61 insertions(+), 81 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index f722898..e164926 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -187,7 +187,6 @@ struct toshiba_acpi_dev {
unsigned int info_supported:1;
unsigned int tr_backlight_supported:1;
unsigned int kbd_illum_supported:1;
- unsigned int kbd_led_registered:1;
unsigned int touchpad_supported:1;
unsigned int eco_supported:1;
unsigned int accelerometer_supported:1;
@@ -198,6 +197,10 @@ struct toshiba_acpi_dev {
unsigned int panel_power_on_supported:1;
unsigned int usb_three_supported:1;
unsigned int sysfs_created:1;
+
+ bool kbd_led_registered;
+ bool illumination_led_registered;
+ bool eco_led_registered;
};

static struct toshiba_acpi_dev *toshiba_acpi;
@@ -439,26 +442,24 @@ static u32 sci_write(struct toshiba_acpi_dev *dev, u32 reg, u32 in1)
}

/* Illumination support */
-static int toshiba_illumination_available(struct toshiba_acpi_dev *dev)
+static void toshiba_illumination_available(struct toshiba_acpi_dev *dev)
{
u32 in[TCI_WORDS] = { SCI_GET, SCI_ILLUMINATION, 0, 0, 0, 0 };
u32 out[TCI_WORDS];
acpi_status status;

+ dev->illumination_supported = 0;
+ dev->illumination_led_registered = false;
+
if (!sci_open(dev))
- return 0;
+ return;

status = tci_raw(dev, in, out);
sci_close(dev);
- if (ACPI_FAILURE(status)) {
+ if (ACPI_FAILURE(status))
pr_err("ACPI call to query Illumination support failed\n");
- return 0;
- } else if (out[0] == TOS_NOT_SUPPORTED) {
- pr_info("Illumination device not available\n");
- return 0;
- }
-
- return 1;
+ else if (out[0] == TOS_SUCCESS)
+ dev->illumination_supported = 1;
}

static void toshiba_illumination_set(struct led_classdev *cdev,
@@ -510,41 +511,41 @@ static enum led_brightness toshiba_illumination_get(struct led_classdev *cdev)
}

/* KBD Illumination */
-static int toshiba_kbd_illum_available(struct toshiba_acpi_dev *dev)
+static void toshiba_kbd_illum_available(struct toshiba_acpi_dev *dev)
{
u32 in[TCI_WORDS] = { SCI_GET, SCI_KBD_ILLUM_STATUS, 0, 0, 0, 0 };
u32 out[TCI_WORDS];
acpi_status status;

+ dev->kbd_illum_supported = 0;
+ dev->kbd_led_registered = false;
+
if (!sci_open(dev))
- return 0;
+ return;

status = tci_raw(dev, in, out);
sci_close(dev);
- if (ACPI_FAILURE(status) || out[0] == TOS_INPUT_DATA_ERROR) {
+ if (ACPI_FAILURE(status)) {
pr_err("ACPI call to query kbd illumination support failed\n");
- return 0;
- } else if (out[0] == TOS_NOT_SUPPORTED) {
- pr_info("Keyboard illumination not available\n");
- return 0;
+ return;
+ } else if (out[0] == TOS_SUCCESS) {
+ /*
+ * Check for keyboard backlight timeout max value,
+ * previous kbd backlight implementation set this to
+ * 0x3c0003, and now the new implementation set this
+ * to 0x3c001a, use this to distinguish between them.
+ */
+ if (out[3] == SCI_KBD_TIME_MAX)
+ dev->kbd_type = 2;
+ else
+ dev->kbd_type = 1;
+ /* Get the current keyboard backlight mode */
+ dev->kbd_mode = out[2] & SCI_KBD_MODE_MASK;
+ /* Get the current time (1-60 seconds) */
+ dev->kbd_time = out[2] >> HCI_MISC_SHIFT;
+ /* Flag as supported */
+ dev->kbd_illum_supported = 1;
}
-
- /*
- * Check for keyboard backlight timeout max value,
- * previous kbd backlight implementation set this to
- * 0x3c0003, and now the new implementation set this
- * to 0x3c001a, use this to distinguish between them.
- */
- if (out[3] == SCI_KBD_TIME_MAX)
- dev->kbd_type = 2;
- else
- dev->kbd_type = 1;
- /* Get the current keyboard backlight mode */
- dev->kbd_mode = out[2] & SCI_KBD_MODE_MASK;
- /* Get the current time (1-60 seconds) */
- dev->kbd_time = out[2] >> HCI_MISC_SHIFT;
-
- return 1;
}

static int toshiba_kbd_illum_status_set(struct toshiba_acpi_dev *dev, u32 time)
@@ -665,17 +666,18 @@ static int toshiba_touchpad_get(struct toshiba_acpi_dev *dev, u32 *state)
}

/* Eco Mode support */
-static int toshiba_eco_mode_available(struct toshiba_acpi_dev *dev)
+static void toshiba_eco_mode_available(struct toshiba_acpi_dev *dev)
{
acpi_status status;
u32 in[TCI_WORDS] = { HCI_GET, HCI_ECO_MODE, 0, 0, 0, 0 };
u32 out[TCI_WORDS];

+ dev->eco_supported = 0;
+ dev->eco_led_registered = false;
+
status = tci_raw(dev, in, out);
if (ACPI_FAILURE(status)) {
pr_err("ACPI call to get ECO led failed\n");
- } else if (out[0] == TOS_NOT_INSTALLED) {
- pr_info("ECO led not installed");
} else if (out[0] == TOS_INPUT_DATA_ERROR) {
/*
* If we receive 0x8300 (Input Data Error), it means that the
@@ -688,13 +690,11 @@ static int toshiba_eco_mode_available(struct toshiba_acpi_dev *dev)
*/
in[3] = 1;
status = tci_raw(dev, in, out);
- if (ACPI_FAILURE(status) || out[0] == TOS_FAILURE)
+ if (ACPI_FAILURE(status))
pr_err("ACPI call to get ECO led failed\n");
else if (out[0] == TOS_SUCCESS)
- return 1;
+ dev->eco_supported = 1;
}
-
- return 0;
}

static enum led_brightness
@@ -734,30 +734,23 @@ static void toshiba_eco_mode_set_status(struct led_classdev *cdev,
}

/* Accelerometer support */
-static int toshiba_accelerometer_supported(struct toshiba_acpi_dev *dev)
+static void toshiba_accelerometer_available(struct toshiba_acpi_dev *dev)
{
u32 in[TCI_WORDS] = { HCI_GET, HCI_ACCELEROMETER2, 0, 0, 0, 0 };
u32 out[TCI_WORDS];
acpi_status status;

+ dev->accelerometer_supported = 0;
+
/*
* Check if the accelerometer call exists,
* this call also serves as initialization
*/
status = tci_raw(dev, in, out);
- if (ACPI_FAILURE(status) || out[0] == TOS_INPUT_DATA_ERROR) {
+ if (ACPI_FAILURE(status))
pr_err("ACPI call to query the accelerometer failed\n");
- return -EIO;
- } else if (out[0] == TOS_DATA_NOT_AVAILABLE ||
- out[0] == TOS_NOT_INITIALIZED) {
- pr_err("Accelerometer not initialized\n");
- return -EIO;
- } else if (out[0] == TOS_NOT_SUPPORTED) {
- pr_info("Accelerometer not supported\n");
- return -ENODEV;
- }
-
- return 0;
+ else if (out[0] == TOS_SUCCESS)
+ dev->accelerometer_supported = 1;
}

static int toshiba_accelerometer_get(struct toshiba_acpi_dev *dev,
@@ -798,35 +791,21 @@ static void toshiba_usb_sleep_charge_available(struct toshiba_acpi_dev *dev)
pr_err("ACPI call to get USB Sleep and Charge mode failed\n");
sci_close(dev);
return;
- } else if (out[0] == TOS_NOT_SUPPORTED) {
- pr_info("USB Sleep and Charge not supported\n");
- sci_close(dev);
- return;
} else if (out[0] == TOS_SUCCESS) {
dev->usbsc_mode_base = out[4];
}

in[5] = SCI_USB_CHARGE_BAT_LVL;
status = tci_raw(dev, in, out);
+ sci_close(dev);
if (ACPI_FAILURE(status)) {
pr_err("ACPI call to get USB Sleep and Charge mode failed\n");
- sci_close(dev);
- return;
- } else if (out[0] == TOS_NOT_SUPPORTED) {
- pr_info("USB Sleep and Charge not supported\n");
- sci_close(dev);
return;
} else if (out[0] == TOS_SUCCESS) {
dev->usbsc_bat_level = out[2];
- /*
- * If we reach this point, it means that the laptop has support
- * for this feature and all values are initialized.
- * Set it as supported.
- */
+ /* Flag as supported */
dev->usb_sleep_charge_supported = 1;
}
-
- sci_close(dev);
}

static int toshiba_usb_sleep_charge_get(struct toshiba_acpi_dev *dev,
@@ -2639,13 +2618,13 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)

backlight_device_unregister(dev->backlight_dev);

- if (dev->illumination_supported)
+ if (dev->illumination_led_registered)
led_classdev_unregister(&dev->led_dev);

if (dev->kbd_led_registered)
led_classdev_unregister(&dev->kbd_led);

- if (dev->eco_supported)
+ if (dev->eco_led_registered)
led_classdev_unregister(&dev->eco_led);

if (toshiba_acpi)
@@ -2727,25 +2706,27 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
if (ret)
goto error;

- if (toshiba_illumination_available(dev)) {
+ toshiba_illumination_available(dev);
+ if (dev->illumination_supported) {
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_supported = 1;
+ dev->illumination_led_registered = true;
}

- if (toshiba_eco_mode_available(dev)) {
+ toshiba_eco_mode_available(dev);
+ if (dev->eco_supported) {
dev->eco_led.name = "toshiba::eco_mode";
dev->eco_led.max_brightness = 1;
dev->eco_led.brightness_set = toshiba_eco_mode_set_status;
dev->eco_led.brightness_get = toshiba_eco_mode_get_status;
if (!led_classdev_register(&dev->acpi_dev->dev, &dev->eco_led))
- dev->eco_supported = 1;
+ dev->eco_led_registered = true;
}

- dev->kbd_illum_supported = toshiba_kbd_illum_available(dev);
+ toshiba_kbd_illum_available(dev);
/*
* Only register the LED if KBD illumination is supported
* and the keyboard backlight operation mode is set to FN-Z
@@ -2756,14 +2737,13 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
dev->kbd_led.brightness_set = toshiba_kbd_backlight_set;
dev->kbd_led.brightness_get = toshiba_kbd_backlight_get;
if (!led_classdev_register(&dev->acpi_dev->dev, &dev->kbd_led))
- dev->kbd_led_registered = 1;
+ dev->kbd_led_registered = true;
}

ret = toshiba_touchpad_get(dev, &dummy);
dev->touchpad_supported = !ret;

- ret = toshiba_accelerometer_supported(dev);
- dev->accelerometer_supported = !ret;
+ toshiba_accelerometer_available(dev);

toshiba_usb_sleep_charge_available(dev);

--
2.4.6

2015-07-28 01:23:36

by Azael Avalos

[permalink] [raw]
Subject: [PATCH v2 2/4] toshiba_acpi: Remove "*not supported" feature prints

Currently the driver prints "*not supported" if any of the features
queried are in fact not supported, let us print the available
features instead.

This patch removes all instances pr_info printing "*not supported",
and add a new function called "print_supported_features", which will
print the available laptop features.

Signed-off-by: Azael Avalos <[email protected]>
---
drivers/platform/x86/toshiba_acpi.c | 69 +++++++++++++++++++++++--------------
1 file changed, 44 insertions(+), 25 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index e164926..e24f0f5 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -477,13 +477,10 @@ static void toshiba_illumination_set(struct led_classdev *cdev,
state = brightness ? 1 : 0;
result = sci_write(dev, SCI_ILLUMINATION, state);
sci_close(dev);
- if (result == TOS_FAILURE) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call for illumination failed\n");
+ else if (result == TOS_NOT_SUPPORTED)
return;
- } else if (result == TOS_NOT_SUPPORTED) {
- pr_info("Illumination not supported\n");
- return;
- }
}

static enum led_brightness toshiba_illumination_get(struct led_classdev *cdev)
@@ -503,7 +500,6 @@ static enum led_brightness toshiba_illumination_get(struct led_classdev *cdev)
pr_err("ACPI call for illumination failed\n");
return LED_OFF;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("Illumination not supported\n");
return LED_OFF;
}

@@ -561,7 +557,6 @@ static int toshiba_kbd_illum_status_set(struct toshiba_acpi_dev *dev, u32 time)
pr_err("ACPI call to set KBD backlight status failed\n");
return -EIO;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("Keyboard backlight status not supported\n");
return -ENODEV;
}

@@ -581,7 +576,6 @@ static int toshiba_kbd_illum_status_get(struct toshiba_acpi_dev *dev, u32 *time)
pr_err("ACPI call to get KBD backlight status failed\n");
return -EIO;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("Keyboard backlight status not supported\n");
return -ENODEV;
}

@@ -600,7 +594,6 @@ static enum led_brightness toshiba_kbd_backlight_get(struct led_classdev *cdev)
pr_err("ACPI call to get the keyboard backlight failed\n");
return LED_OFF;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("Keyboard backlight not supported\n");
return LED_OFF;
}

@@ -621,7 +614,6 @@ static void toshiba_kbd_backlight_set(struct led_classdev *cdev,
pr_err("ACPI call to set KBD Illumination mode failed\n");
return;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("Keyboard backlight not supported\n");
return;
}
}
@@ -822,7 +814,6 @@ static int toshiba_usb_sleep_charge_get(struct toshiba_acpi_dev *dev,
pr_err("ACPI call to set USB S&C mode failed\n");
return -EIO;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("USB Sleep and Charge not supported\n");
return -ENODEV;
} else if (result == TOS_INPUT_DATA_ERROR) {
return -EIO;
@@ -845,7 +836,6 @@ static int toshiba_usb_sleep_charge_set(struct toshiba_acpi_dev *dev,
pr_err("ACPI call to set USB S&C mode failed\n");
return -EIO;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("USB Sleep and Charge not supported\n");
return -ENODEV;
} else if (result == TOS_INPUT_DATA_ERROR) {
return -EIO;
@@ -871,7 +861,6 @@ static int toshiba_sleep_functions_status_get(struct toshiba_acpi_dev *dev,
pr_err("ACPI call to get USB S&C battery level failed\n");
return -EIO;
} else if (out[0] == TOS_NOT_SUPPORTED) {
- pr_info("USB Sleep and Charge not supported\n");
return -ENODEV;
} else if (out[0] == TOS_INPUT_DATA_ERROR) {
return -EIO;
@@ -900,7 +889,6 @@ static int toshiba_sleep_functions_status_set(struct toshiba_acpi_dev *dev,
pr_err("ACPI call to set USB S&C battery level failed\n");
return -EIO;
} else if (out[0] == TOS_NOT_SUPPORTED) {
- pr_info("USB Sleep and Charge not supported\n");
return -ENODEV;
} else if (out[0] == TOS_INPUT_DATA_ERROR) {
return -EIO;
@@ -927,7 +915,6 @@ static int toshiba_usb_rapid_charge_get(struct toshiba_acpi_dev *dev,
return -EIO;
} else if (out[0] == TOS_NOT_SUPPORTED ||
out[0] == TOS_INPUT_DATA_ERROR) {
- pr_info("USB Rapid Charge not supported\n");
return -ENODEV;
}

@@ -954,7 +941,6 @@ static int toshiba_usb_rapid_charge_set(struct toshiba_acpi_dev *dev,
pr_err("ACPI call to set USB Rapid Charge failed\n");
return -EIO;
} else if (out[0] == TOS_NOT_SUPPORTED) {
- pr_info("USB Rapid Charge not supported\n");
return -ENODEV;
} else if (out[0] == TOS_INPUT_DATA_ERROR) {
return -EIO;
@@ -976,7 +962,6 @@ static int toshiba_usb_sleep_music_get(struct toshiba_acpi_dev *dev, u32 *state)
pr_err("ACPI call to get Sleep and Music failed\n");
return -EIO;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("Sleep and Music not supported\n");
return -ENODEV;
} else if (result == TOS_INPUT_DATA_ERROR) {
return -EIO;
@@ -998,7 +983,6 @@ static int toshiba_usb_sleep_music_set(struct toshiba_acpi_dev *dev, u32 state)
pr_err("ACPI call to set Sleep and Music failed\n");
return -EIO;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("Sleep and Music not supported\n");
return -ENODEV;
} else if (result == TOS_INPUT_DATA_ERROR) {
return -EIO;
@@ -1021,7 +1005,6 @@ static int toshiba_function_keys_get(struct toshiba_acpi_dev *dev, u32 *mode)
pr_err("ACPI call to get KBD function keys failed\n");
return -EIO;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("KBD function keys not supported\n");
return -ENODEV;
}

@@ -1041,7 +1024,6 @@ static int toshiba_function_keys_set(struct toshiba_acpi_dev *dev, u32 mode)
pr_err("ACPI call to set KBD function keys failed\n");
return -EIO;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("KBD function keys not supported\n");
return -ENODEV;
}

@@ -1062,7 +1044,6 @@ static int toshiba_panel_power_on_get(struct toshiba_acpi_dev *dev, u32 *state)
pr_err("ACPI call to get Panel Power ON failed\n");
return -EIO;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("Panel Power on not supported\n");
return -ENODEV;
} else if (result == TOS_INPUT_DATA_ERROR) {
return -EIO;
@@ -1084,7 +1065,6 @@ static int toshiba_panel_power_on_set(struct toshiba_acpi_dev *dev, u32 state)
pr_err("ACPI call to set Panel Power ON failed\n");
return -EIO;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("Panel Power ON not supported\n");
return -ENODEV;
} else if (result == TOS_INPUT_DATA_ERROR) {
return -EIO;
@@ -1107,7 +1087,6 @@ static int toshiba_usb_three_get(struct toshiba_acpi_dev *dev, u32 *state)
pr_err("ACPI call to get USB 3 failed\n");
return -EIO;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("USB 3 not supported\n");
return -ENODEV;
} else if (result == TOS_INPUT_DATA_ERROR) {
return -EIO;
@@ -1129,7 +1108,6 @@ static int toshiba_usb_three_set(struct toshiba_acpi_dev *dev, u32 state)
pr_err("ACPI call to set USB 3 failed\n");
return -EIO;
} else if (result == TOS_NOT_SUPPORTED) {
- pr_info("USB 3 not supported\n");
return -ENODEV;
} else if (result == TOS_INPUT_DATA_ERROR) {
return -EIO;
@@ -1151,7 +1129,6 @@ static int toshiba_hotkey_event_type_get(struct toshiba_acpi_dev *dev,
pr_err("ACPI call to get System type failed\n");
return -EIO;
} else if (out[0] == TOS_NOT_SUPPORTED) {
- pr_info("System type not supported\n");
return -ENODEV;
}

@@ -2594,6 +2571,46 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
return 0;
}

+static void print_supported_features(struct toshiba_acpi_dev *dev)
+{
+ pr_info("Supported laptop features:");
+
+ if (dev->hotkey_dev)
+ pr_cont(" hotkeys");
+ if (dev->backlight_dev)
+ pr_cont(" backlight");
+ if (dev->video_supported)
+ pr_cont(" video-out");
+ if (dev->fan_supported)
+ pr_cont(" fan");
+ if (dev->tr_backlight_supported)
+ pr_cont(" transflective-backlight");
+ if (dev->illumination_supported)
+ pr_cont(" illumination");
+ if (dev->kbd_illum_supported)
+ pr_cont(" keyboard-backlight");
+ if (dev->touchpad_supported)
+ pr_cont(" touchpad");
+ if (dev->eco_supported)
+ pr_cont(" eco-led");
+ if (dev->accelerometer_supported)
+ pr_cont(" accelerometer-axes");
+ if (dev->usb_sleep_charge_supported)
+ pr_cont(" usb-sleep-charge");
+ if (dev->usb_rapid_charge_supported)
+ pr_cont(" usb-rapid-charge");
+ if (dev->usb_sleep_music_supported)
+ pr_cont(" usb-sleep-music");
+ if (dev->kbd_function_keys_supported)
+ pr_cont(" special-function-keys");
+ if (dev->panel_power_on_supported)
+ pr_cont(" panel-power-on");
+ if (dev->usb_three_supported)
+ pr_cont(" usb3");
+
+ pr_cont("\n");
+}
+
static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
{
struct toshiba_acpi_dev *dev = acpi_driver_data(acpi_dev);
@@ -2772,6 +2789,8 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
if (dev->kbd_function_keys_supported && special_functions)
toshiba_acpi_enable_special_functions(dev);

+ print_supported_features(dev);
+
ret = sysfs_create_group(&dev->acpi_dev->dev.kobj,
&toshiba_attr_group);
if (ret) {
--
2.4.6

2015-07-28 01:22:57

by Azael Avalos

[permalink] [raw]
Subject: [PATCH v2 3/4] toshiba_acpi: Refactor *{get, set} functions return value

This patch changes the default return value of the driver *{get, set}
functions from 0 (success) to -EIO, since the driver default error
value is -EIO.

All the functions now check for TOS_FAILURE, TOS_NOT_SUPPORTED and
TOS_SUCCESS.

On TOS_FAILURE a pr_err message is printed informing the user of the
error (no change was made to this, except the check was added to the
functions not checking for this).

On TOS_NOT_SUPPORTED we now return -ENODEV immediately (some
functions were returning -EIO)

On TOS_SUCCESS we now return 0, as a side effect, a new success value
was added, since some functions return one instead of zero to
indicate success.

As a special case, the LED functions only check for TOS_FAILURE on
*set, and check for TOS_FAILURE and TOS_SUCCESS on *get with their
default return value set to LED_OFF.

Also the {lcd, video}_proc* functions were adapted to reflect these
changes to their parent HCI functions.

Signed-off-by: Azael Avalos <[email protected]>
---
drivers/platform/x86/toshiba_acpi.c | 432 ++++++++++++++++++------------------
1 file changed, 217 insertions(+), 215 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index e24f0f5..0034341 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -93,6 +93,7 @@ MODULE_LICENSE("GPL");

/* Return codes */
#define TOS_SUCCESS 0x0000
+#define TOS_SUCCESS2 0x0001
#define TOS_OPEN_CLOSE_OK 0x0044
#define TOS_FAILURE 0x1000
#define TOS_NOT_SUPPORTED 0x8000
@@ -467,7 +468,8 @@ static void toshiba_illumination_set(struct led_classdev *cdev,
{
struct toshiba_acpi_dev *dev = container_of(cdev,
struct toshiba_acpi_dev, led_dev);
- u32 state, result;
+ u32 result;
+ u32 state;

/* First request : initialize communication. */
if (!sci_open(dev))
@@ -479,8 +481,6 @@ static void toshiba_illumination_set(struct led_classdev *cdev,
sci_close(dev);
if (result == TOS_FAILURE)
pr_err("ACPI call for illumination failed\n");
- else if (result == TOS_NOT_SUPPORTED)
- return;
}

static enum led_brightness toshiba_illumination_get(struct led_classdev *cdev)
@@ -496,14 +496,12 @@ static enum led_brightness toshiba_illumination_get(struct led_classdev *cdev)
/* Check the illumination */
result = sci_read(dev, SCI_ILLUMINATION, &state);
sci_close(dev);
- if (result == TOS_FAILURE || result == TOS_INPUT_DATA_ERROR) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call for illumination failed\n");
- return LED_OFF;
- } else if (result == TOS_NOT_SUPPORTED) {
- return LED_OFF;
- }
+ else if (result == TOS_SUCCESS)
+ return state ? LED_FULL : LED_OFF;

- return state ? LED_FULL : LED_OFF;
+ return LED_OFF;
}

/* KBD Illumination */
@@ -553,14 +551,14 @@ static int toshiba_kbd_illum_status_set(struct toshiba_acpi_dev *dev, u32 time)

result = sci_write(dev, SCI_KBD_ILLUM_STATUS, time);
sci_close(dev);
- if (result == TOS_FAILURE || result == TOS_INPUT_DATA_ERROR) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call to set KBD backlight status failed\n");
- return -EIO;
- } else if (result == TOS_NOT_SUPPORTED) {
+ else if (result == TOS_NOT_SUPPORTED)
return -ENODEV;
- }
+ else if (result == TOS_SUCCESS)
+ return 0;

- return 0;
+ return -EIO;
}

static int toshiba_kbd_illum_status_get(struct toshiba_acpi_dev *dev, u32 *time)
@@ -572,50 +570,46 @@ static int toshiba_kbd_illum_status_get(struct toshiba_acpi_dev *dev, u32 *time)

result = sci_read(dev, SCI_KBD_ILLUM_STATUS, time);
sci_close(dev);
- if (result == TOS_FAILURE || result == TOS_INPUT_DATA_ERROR) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call to get KBD backlight status failed\n");
- return -EIO;
- } else if (result == TOS_NOT_SUPPORTED) {
+ else if (result == TOS_NOT_SUPPORTED)
return -ENODEV;
- }
+ else if (result == TOS_SUCCESS)
+ return 0;

- return 0;
+ return -EIO;
}

static enum led_brightness toshiba_kbd_backlight_get(struct led_classdev *cdev)
{
- struct toshiba_acpi_dev *dev = container_of(cdev,
- struct toshiba_acpi_dev, kbd_led);
- u32 state, result;
+ struct toshiba_acpi_dev *dev;
+ u32 result;
+ u32 state;

+ dev = container_of(cdev, struct toshiba_acpi_dev, kbd_led);
/* Check the keyboard backlight state */
result = hci_read(dev, HCI_KBD_ILLUMINATION, &state);
- if (result == TOS_FAILURE || result == TOS_INPUT_DATA_ERROR) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call to get the keyboard backlight failed\n");
- return LED_OFF;
- } else if (result == TOS_NOT_SUPPORTED) {
- return LED_OFF;
- }
+ else if (result == TOS_SUCCESS)
+ return state ? LED_FULL : LED_OFF;

- return state ? LED_FULL : LED_OFF;
+ return LED_OFF;
}

static void toshiba_kbd_backlight_set(struct led_classdev *cdev,
enum led_brightness brightness)
{
- struct toshiba_acpi_dev *dev = container_of(cdev,
- struct toshiba_acpi_dev, kbd_led);
- u32 state, result;
+ struct toshiba_acpi_dev *dev;
+ u32 result;
+ u32 state;

+ dev = container_of(cdev, struct toshiba_acpi_dev, kbd_led);
/* Set the keyboard backlight state */
state = brightness ? 1 : 0;
result = hci_write(dev, HCI_KBD_ILLUMINATION, state);
- if (result == TOS_FAILURE || result == TOS_INPUT_DATA_ERROR) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call to set KBD Illumination mode failed\n");
- return;
- } else if (result == TOS_NOT_SUPPORTED) {
- return;
- }
}

/* TouchPad support */
@@ -628,14 +622,14 @@ static int toshiba_touchpad_set(struct toshiba_acpi_dev *dev, u32 state)

result = sci_write(dev, SCI_TOUCHPAD, state);
sci_close(dev);
- if (result == TOS_FAILURE) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call to set the touchpad failed\n");
- return -EIO;
- } else if (result == TOS_NOT_SUPPORTED) {
+ else if (result == TOS_NOT_SUPPORTED)
return -ENODEV;
- }
+ else if (result == TOS_SUCCESS)
+ return 0;

- return 0;
+ return -EIO;
}

static int toshiba_touchpad_get(struct toshiba_acpi_dev *dev, u32 *state)
@@ -647,14 +641,14 @@ static int toshiba_touchpad_get(struct toshiba_acpi_dev *dev, u32 *state)

result = sci_read(dev, SCI_TOUCHPAD, state);
sci_close(dev);
- if (result == TOS_FAILURE) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call to query the touchpad failed\n");
- return -EIO;
- } else if (result == TOS_NOT_SUPPORTED) {
+ else if (result == TOS_NOT_SUPPORTED)
return -ENODEV;
- }
+ else if (result == TOS_SUCCESS)
+ return 0;

- return 0;
+ return -EIO;
}

/* Eco Mode support */
@@ -699,12 +693,12 @@ toshiba_eco_mode_get_status(struct led_classdev *cdev)
acpi_status status;

status = tci_raw(dev, in, out);
- if (ACPI_FAILURE(status) || out[0] == TOS_INPUT_DATA_ERROR) {
+ if (ACPI_FAILURE(status))
pr_err("ACPI call to get ECO led failed\n");
- return LED_OFF;
- }
+ else if (out[0] == TOS_SUCCESS)
+ return out[2] ? LED_FULL : LED_OFF;

- return out[2] ? LED_FULL : LED_OFF;
+ return LED_OFF;
}

static void toshiba_eco_mode_set_status(struct led_classdev *cdev,
@@ -719,10 +713,8 @@ static void toshiba_eco_mode_set_status(struct led_classdev *cdev,
/* Switch the Eco Mode led on/off */
in[2] = (brightness) ? 1 : 0;
status = tci_raw(dev, in, out);
- if (ACPI_FAILURE(status) || out[0] == TOS_INPUT_DATA_ERROR) {
+ if (ACPI_FAILURE(status))
pr_err("ACPI call to set ECO led failed\n");
- return;
- }
}

/* Accelerometer support */
@@ -754,15 +746,17 @@ static int toshiba_accelerometer_get(struct toshiba_acpi_dev *dev,

/* Check the Accelerometer status */
status = tci_raw(dev, in, out);
- if (ACPI_FAILURE(status) || out[0] == TOS_INPUT_DATA_ERROR) {
+ if (ACPI_FAILURE(status)) {
pr_err("ACPI call to query the accelerometer failed\n");
- return -EIO;
+ } else if (out[0] == TOS_NOT_SUPPORTED) {
+ return -ENODEV;
+ } else if (out[0] == TOS_SUCCESS) {
+ *xy = out[2];
+ *z = out[4];
+ return 0;
}

- *xy = out[2];
- *z = out[4];
-
- return 0;
+ return -EIO;
}

/* Sleep (Charge and Music) utilities support */
@@ -810,16 +804,14 @@ static int toshiba_usb_sleep_charge_get(struct toshiba_acpi_dev *dev,

result = sci_read(dev, SCI_USB_SLEEP_CHARGE, mode);
sci_close(dev);
- if (result == TOS_FAILURE) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call to set USB S&C mode failed\n");
- return -EIO;
- } else if (result == TOS_NOT_SUPPORTED) {
+ else if (result == TOS_NOT_SUPPORTED)
return -ENODEV;
- } else if (result == TOS_INPUT_DATA_ERROR) {
- return -EIO;
- }
+ else if (result == TOS_SUCCESS)
+ return 0;

- return 0;
+ return -EIO;
}

static int toshiba_usb_sleep_charge_set(struct toshiba_acpi_dev *dev,
@@ -832,16 +824,14 @@ static int toshiba_usb_sleep_charge_set(struct toshiba_acpi_dev *dev,

result = sci_write(dev, SCI_USB_SLEEP_CHARGE, mode);
sci_close(dev);
- if (result == TOS_FAILURE) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call to set USB S&C mode failed\n");
- return -EIO;
- } else if (result == TOS_NOT_SUPPORTED) {
+ else if (result == TOS_NOT_SUPPORTED)
return -ENODEV;
- } else if (result == TOS_INPUT_DATA_ERROR) {
- return -EIO;
- }
+ else if (result == TOS_SUCCESS)
+ return 0;

- return 0;
+ return -EIO;
}

static int toshiba_sleep_functions_status_get(struct toshiba_acpi_dev *dev,
@@ -859,16 +849,14 @@ static int toshiba_sleep_functions_status_get(struct toshiba_acpi_dev *dev,
sci_close(dev);
if (ACPI_FAILURE(status)) {
pr_err("ACPI call to get USB S&C battery level failed\n");
- return -EIO;
} else if (out[0] == TOS_NOT_SUPPORTED) {
return -ENODEV;
- } else if (out[0] == TOS_INPUT_DATA_ERROR) {
- return -EIO;
+ } else if (out[0] == TOS_SUCCESS) {
+ *mode = out[2];
+ return 0;
}

- *mode = out[2];
-
- return 0;
+ return -EIO;
}

static int toshiba_sleep_functions_status_set(struct toshiba_acpi_dev *dev,
@@ -885,16 +873,14 @@ static int toshiba_sleep_functions_status_set(struct toshiba_acpi_dev *dev,
in[5] = SCI_USB_CHARGE_BAT_LVL;
status = tci_raw(dev, in, out);
sci_close(dev);
- if (ACPI_FAILURE(status)) {
+ if (ACPI_FAILURE(status))
pr_err("ACPI call to set USB S&C battery level failed\n");
- return -EIO;
- } else if (out[0] == TOS_NOT_SUPPORTED) {
+ else if (out[0] == TOS_NOT_SUPPORTED)
return -ENODEV;
- } else if (out[0] == TOS_INPUT_DATA_ERROR) {
- return -EIO;
- }
+ else if (out[0] == TOS_SUCCESS)
+ return 0;

- return 0;
+ return -EIO;
}

static int toshiba_usb_rapid_charge_get(struct toshiba_acpi_dev *dev,
@@ -912,15 +898,14 @@ static int toshiba_usb_rapid_charge_get(struct toshiba_acpi_dev *dev,
sci_close(dev);
if (ACPI_FAILURE(status)) {
pr_err("ACPI call to get USB Rapid Charge failed\n");
- return -EIO;
- } else if (out[0] == TOS_NOT_SUPPORTED ||
- out[0] == TOS_INPUT_DATA_ERROR) {
+ } else if (out[0] == TOS_NOT_SUPPORTED) {
return -ENODEV;
+ } else if (out[0] == TOS_SUCCESS || out[0] == TOS_SUCCESS2) {
+ *state = out[2];
+ return 0;
}

- *state = out[2];
-
- return 0;
+ return -EIO;
}

static int toshiba_usb_rapid_charge_set(struct toshiba_acpi_dev *dev,
@@ -937,16 +922,14 @@ static int toshiba_usb_rapid_charge_set(struct toshiba_acpi_dev *dev,
in[5] = SCI_USB_CHARGE_RAPID_DSP;
status = tci_raw(dev, in, out);
sci_close(dev);
- if (ACPI_FAILURE(status)) {
+ if (ACPI_FAILURE(status))
pr_err("ACPI call to set USB Rapid Charge failed\n");
- return -EIO;
- } else if (out[0] == TOS_NOT_SUPPORTED) {
+ else if (out[0] == TOS_NOT_SUPPORTED)
return -ENODEV;
- } else if (out[0] == TOS_INPUT_DATA_ERROR) {
- return -EIO;
- }
+ else if (out[0] == TOS_SUCCESS || out[0] == TOS_SUCCESS2)
+ return 0;

- return 0;
+ return -EIO;
}

static int toshiba_usb_sleep_music_get(struct toshiba_acpi_dev *dev, u32 *state)
@@ -958,16 +941,14 @@ static int toshiba_usb_sleep_music_get(struct toshiba_acpi_dev *dev, u32 *state)

result = sci_read(dev, SCI_USB_SLEEP_MUSIC, state);
sci_close(dev);
- if (result == TOS_FAILURE) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call to get Sleep and Music failed\n");
- return -EIO;
- } else if (result == TOS_NOT_SUPPORTED) {
+ else if (result == TOS_NOT_SUPPORTED)
return -ENODEV;
- } else if (result == TOS_INPUT_DATA_ERROR) {
- return -EIO;
- }
+ else if (result == TOS_SUCCESS)
+ return 0;

- return 0;
+ return -EIO;
}

static int toshiba_usb_sleep_music_set(struct toshiba_acpi_dev *dev, u32 state)
@@ -979,16 +960,14 @@ static int toshiba_usb_sleep_music_set(struct toshiba_acpi_dev *dev, u32 state)

result = sci_write(dev, SCI_USB_SLEEP_MUSIC, state);
sci_close(dev);
- if (result == TOS_FAILURE) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call to set Sleep and Music failed\n");
- return -EIO;
- } else if (result == TOS_NOT_SUPPORTED) {
+ else if (result == TOS_NOT_SUPPORTED)
return -ENODEV;
- } else if (result == TOS_INPUT_DATA_ERROR) {
- return -EIO;
- }
+ else if (result == TOS_SUCCESS)
+ return 0;

- return 0;
+ return -EIO;
}

/* Keyboard function keys */
@@ -1001,14 +980,14 @@ static int toshiba_function_keys_get(struct toshiba_acpi_dev *dev, u32 *mode)

result = sci_read(dev, SCI_KBD_FUNCTION_KEYS, mode);
sci_close(dev);
- if (result == TOS_FAILURE || result == TOS_INPUT_DATA_ERROR) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call to get KBD function keys failed\n");
- return -EIO;
- } else if (result == TOS_NOT_SUPPORTED) {
+ else if (result == TOS_NOT_SUPPORTED)
return -ENODEV;
- }
+ else if (result == TOS_SUCCESS || result == TOS_SUCCESS2)
+ return 0;

- return 0;
+ return -EIO;
}

static int toshiba_function_keys_set(struct toshiba_acpi_dev *dev, u32 mode)
@@ -1020,14 +999,14 @@ static int toshiba_function_keys_set(struct toshiba_acpi_dev *dev, u32 mode)

result = sci_write(dev, SCI_KBD_FUNCTION_KEYS, mode);
sci_close(dev);
- if (result == TOS_FAILURE || result == TOS_INPUT_DATA_ERROR) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call to set KBD function keys failed\n");
- return -EIO;
- } else if (result == TOS_NOT_SUPPORTED) {
+ else if (result == TOS_NOT_SUPPORTED)
return -ENODEV;
- }
+ else if (result == TOS_SUCCESS || result == TOS_SUCCESS2)
+ return 0;

- return 0;
+ return -EIO;
}

/* Panel Power ON */
@@ -1040,16 +1019,14 @@ static int toshiba_panel_power_on_get(struct toshiba_acpi_dev *dev, u32 *state)

result = sci_read(dev, SCI_PANEL_POWER_ON, state);
sci_close(dev);
- if (result == TOS_FAILURE) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call to get Panel Power ON failed\n");
- return -EIO;
- } else if (result == TOS_NOT_SUPPORTED) {
+ else if (result == TOS_NOT_SUPPORTED)
return -ENODEV;
- } else if (result == TOS_INPUT_DATA_ERROR) {
- return -EIO;
- }
+ else if (result == TOS_SUCCESS)
+ return 0;

- return 0;
+ return -EIO;
}

static int toshiba_panel_power_on_set(struct toshiba_acpi_dev *dev, u32 state)
@@ -1061,16 +1038,14 @@ static int toshiba_panel_power_on_set(struct toshiba_acpi_dev *dev, u32 state)

result = sci_write(dev, SCI_PANEL_POWER_ON, state);
sci_close(dev);
- if (result == TOS_FAILURE) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call to set Panel Power ON failed\n");
- return -EIO;
- } else if (result == TOS_NOT_SUPPORTED) {
+ else if (result == TOS_NOT_SUPPORTED)
return -ENODEV;
- } else if (result == TOS_INPUT_DATA_ERROR) {
- return -EIO;
- }
+ else if (result == TOS_SUCCESS)
+ return 0;

- return 0;
+ return -EIO;
}

/* USB Three */
@@ -1083,16 +1058,14 @@ static int toshiba_usb_three_get(struct toshiba_acpi_dev *dev, u32 *state)

result = sci_read(dev, SCI_USB_THREE, state);
sci_close(dev);
- if (result == TOS_FAILURE) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call to get USB 3 failed\n");
- return -EIO;
- } else if (result == TOS_NOT_SUPPORTED) {
+ else if (result == TOS_NOT_SUPPORTED)
return -ENODEV;
- } else if (result == TOS_INPUT_DATA_ERROR) {
- return -EIO;
- }
+ else if (result == TOS_SUCCESS || result == TOS_SUCCESS2)
+ return 0;

- return 0;
+ return -EIO;
}

static int toshiba_usb_three_set(struct toshiba_acpi_dev *dev, u32 state)
@@ -1104,16 +1077,14 @@ static int toshiba_usb_three_set(struct toshiba_acpi_dev *dev, u32 state)

result = sci_write(dev, SCI_USB_THREE, state);
sci_close(dev);
- if (result == TOS_FAILURE) {
+ if (result == TOS_FAILURE)
pr_err("ACPI call to set USB 3 failed\n");
- return -EIO;
- } else if (result == TOS_NOT_SUPPORTED) {
+ else if (result == TOS_NOT_SUPPORTED)
return -ENODEV;
- } else if (result == TOS_INPUT_DATA_ERROR) {
- return -EIO;
- }
+ else if (result == TOS_SUCCESS || result == TOS_SUCCESS2)
+ return 0;

- return 0;
+ return -EIO;
}

/* Hotkey Event type */
@@ -1127,29 +1098,43 @@ static int toshiba_hotkey_event_type_get(struct toshiba_acpi_dev *dev,
status = tci_raw(dev, in, out);
if (ACPI_FAILURE(status)) {
pr_err("ACPI call to get System type failed\n");
- return -EIO;
} else if (out[0] == TOS_NOT_SUPPORTED) {
return -ENODEV;
+ } else if (out[0] == TOS_SUCCESS) {
+ *type = out[3];
+ return 0;
}

- *type = out[3];
-
- return 0;
+ return -EIO;
}

/* Transflective Backlight */
static int get_tr_backlight_status(struct toshiba_acpi_dev *dev, u32 *status)
{
- u32 hci_result = hci_read(dev, HCI_TR_BACKLIGHT, status);
+ u32 result = hci_read(dev, HCI_TR_BACKLIGHT, status);

- return hci_result == TOS_SUCCESS ? 0 : -EIO;
+ if (result == TOS_FAILURE)
+ pr_err("ACPI call to get Transflective Backlight failed\n");
+ else if (result == TOS_NOT_SUPPORTED)
+ return -ENODEV;
+ else if (result == TOS_SUCCESS)
+ return 0;
+
+ return -EIO;
}

static int set_tr_backlight_status(struct toshiba_acpi_dev *dev, u32 status)
{
- u32 hci_result = hci_write(dev, HCI_TR_BACKLIGHT, !status);
+ u32 result = hci_write(dev, HCI_TR_BACKLIGHT, !status);

- return hci_result == TOS_SUCCESS ? 0 : -EIO;
+ if (result == TOS_FAILURE)
+ pr_err("ACPI call to set Transflective Backlight failed\n");
+ else if (result == TOS_NOT_SUPPORTED)
+ return -ENODEV;
+ else if (result == TOS_SUCCESS)
+ return 0;
+
+ return -EIO;
}

static struct proc_dir_entry *toshiba_proc_dir;
@@ -1157,7 +1142,7 @@ static struct proc_dir_entry *toshiba_proc_dir;
/* LCD Brightness */
static int __get_lcd_brightness(struct toshiba_acpi_dev *dev)
{
- u32 hci_result;
+ u32 result;
u32 value;
int brightness = 0;

@@ -1171,8 +1156,12 @@ static int __get_lcd_brightness(struct toshiba_acpi_dev *dev)
brightness++;
}

- hci_result = hci_read(dev, HCI_LCD_BRIGHTNESS, &value);
- if (hci_result == TOS_SUCCESS)
+ result = hci_read(dev, HCI_LCD_BRIGHTNESS, &value);
+ if (result == TOS_FAILURE)
+ pr_err("ACPI call to get LCD Brightness failed\n");
+ else if (result == TOS_NOT_SUPPORTED)
+ return -ENODEV;
+ else if (result == TOS_SUCCESS)
return brightness + (value >> HCI_LCD_BRIGHTNESS_SHIFT);

return -EIO;
@@ -1196,14 +1185,15 @@ static int lcd_proc_show(struct seq_file *m, void *v)

levels = dev->backlight_dev->props.max_brightness + 1;
value = get_lcd_brightness(dev->backlight_dev);
- if (value >= 0) {
- seq_printf(m, "brightness: %d\n", value);
- seq_printf(m, "brightness_levels: %d\n", levels);
- return 0;
+ if (value < 0) {
+ pr_err("Error reading LCD brightness\n");
+ return -EIO;
}

- pr_err("Error reading LCD brightness\n");
- return -EIO;
+ seq_printf(m, "brightness: %d\n", value);
+ seq_printf(m, "brightness_levels: %d\n", levels);
+
+ return 0;
}

static int lcd_proc_open(struct inode *inode, struct file *file)
@@ -1213,7 +1203,7 @@ static int lcd_proc_open(struct inode *inode, struct file *file)

static int set_lcd_brightness(struct toshiba_acpi_dev *dev, int value)
{
- u32 hci_result;
+ u32 result;

if (dev->tr_backlight_supported) {
int ret = set_tr_backlight_status(dev, !value);
@@ -1225,8 +1215,15 @@ static int set_lcd_brightness(struct toshiba_acpi_dev *dev, int value)
}

value = value << HCI_LCD_BRIGHTNESS_SHIFT;
- hci_result = hci_write(dev, HCI_LCD_BRIGHTNESS, value);
- return hci_result == TOS_SUCCESS ? 0 : -EIO;
+ result = hci_write(dev, HCI_LCD_BRIGHTNESS, value);
+ if (result == TOS_FAILURE)
+ pr_err("ACPI call to set LCD Brightness failed\n");
+ else if (result == TOS_NOT_SUPPORTED)
+ return -ENODEV;
+ else if (result == TOS_SUCCESS)
+ return 0;
+
+ return -EIO;
}

static int set_lcd_status(struct backlight_device *bd)
@@ -1242,24 +1239,22 @@ static ssize_t lcd_proc_write(struct file *file, const char __user *buf,
struct toshiba_acpi_dev *dev = PDE_DATA(file_inode(file));
char cmd[42];
size_t len;
- int value;
- int ret;
int levels = dev->backlight_dev->props.max_brightness + 1;
+ int value;

len = min(count, sizeof(cmd) - 1);
if (copy_from_user(cmd, buf, len))
return -EFAULT;
cmd[len] = '\0';

- if (sscanf(cmd, " brightness : %i", &value) == 1 &&
- value >= 0 && value < levels) {
- ret = set_lcd_brightness(dev, value);
- if (ret == 0)
- ret = count;
- } else {
- ret = -EINVAL;
- }
- return ret;
+ if (sscanf(cmd, " brightness : %i", &value) != 1 &&
+ value < 0 && value > levels)
+ return -EINVAL;
+
+ if (set_lcd_brightness(dev, value))
+ return -EIO;
+
+ return count;
}

static const struct file_operations lcd_proc_fops = {
@@ -1271,32 +1266,38 @@ static const struct file_operations lcd_proc_fops = {
.write = lcd_proc_write,
};

+/* Video-Out */
static int get_video_status(struct toshiba_acpi_dev *dev, u32 *status)
{
- u32 hci_result;
+ u32 result = hci_read(dev, HCI_VIDEO_OUT, status);
+
+ if (result == TOS_FAILURE)
+ pr_err("ACPI call to get Video-Out failed\n");
+ else if (result == TOS_NOT_SUPPORTED)
+ return -ENODEV;
+ else if (result == TOS_SUCCESS)
+ return 0;

- hci_result = hci_read(dev, HCI_VIDEO_OUT, status);
- return hci_result == TOS_SUCCESS ? 0 : -EIO;
+ return -EIO;
}

static int video_proc_show(struct seq_file *m, void *v)
{
struct toshiba_acpi_dev *dev = m->private;
u32 value;
- int ret;

- 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;
+ if (get_video_status(dev, &value))
+ return -EIO;

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

- return ret;
+ 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;
}

static int video_proc_open(struct inode *inode, struct file *file)
@@ -1308,13 +1309,15 @@ 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_DATA(file_inode(file));
- char *cmd, *buffer;
- int ret;
- int value;
+ unsigned int new_video_out;
+ char *buffer;
+ char *cmd;
int remain = count;
int lcd_out = -1;
int crt_out = -1;
int tv_out = -1;
+ int value;
+ int ret;
u32 video_out;

cmd = kmalloc(count + 1, GFP_KERNEL);
@@ -1348,23 +1351,22 @@ static ssize_t video_proc_write(struct file *file, const char __user *buf,

kfree(cmd);

- ret = get_video_status(dev, &video_out);
- if (!ret) {
- unsigned int new_video_out = video_out;
+ if (get_video_status(dev, &video_out))
+ return -EIO;

- if (lcd_out != -1)
- _set_bit(&new_video_out, HCI_VIDEO_OUT_LCD, lcd_out);
- if (crt_out != -1)
- _set_bit(&new_video_out, HCI_VIDEO_OUT_CRT, crt_out);
- if (tv_out != -1)
- _set_bit(&new_video_out, HCI_VIDEO_OUT_TV, tv_out);
- /*
- * To avoid unnecessary video disruption, only write the new
- * video setting if something changed.
- */
- if (new_video_out != video_out)
- ret = write_acpi_int(METHOD_VIDEO_OUT, new_video_out);
- }
+ new_video_out = video_out;
+ if (lcd_out != -1)
+ _set_bit(&new_video_out, HCI_VIDEO_OUT_LCD, lcd_out);
+ if (crt_out != -1)
+ _set_bit(&new_video_out, HCI_VIDEO_OUT_CRT, crt_out);
+ if (tv_out != -1)
+ _set_bit(&new_video_out, HCI_VIDEO_OUT_TV, tv_out);
+ /*
+ * To avoid unnecessary video disruption, only write the new
+ * video setting if something changed.
+ */
+ if (new_video_out != video_out)
+ ret = write_acpi_int(METHOD_VIDEO_OUT, new_video_out);

return ret ? ret : count;
}
--
2.4.6

2015-07-28 01:23:16

by Azael Avalos

[permalink] [raw]
Subject: [PATCH v2 4/4] toshiba_acpi: Bump driver version to 0.23

Given that some features were added (/dev/toshiba_acpi device), some
clean-ups and minor changes, bump the driver version to 0.23.

Signed-off-by: Azael Avalos <[email protected]>
---
drivers/platform/x86/toshiba_acpi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 0034341..66954a0 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -31,7 +31,7 @@

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

-#define TOSHIBA_ACPI_VERSION "0.22"
+#define TOSHIBA_ACPI_VERSION "0.23"
#define PROC_INTERFACE_VERSION 1

#include <linux/kernel.h>
--
2.4.6

2015-07-29 04:52:07

by Darren Hart

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] toshiba_acpi: Refactor *{get, set} functions return value

On Mon, Jul 27, 2015 at 07:22:27PM -0600, Azael Avalos wrote:
> This patch changes the default return value of the driver *{get, set}
> functions from 0 (success) to -EIO, since the driver default error
> value is -EIO.
>
> All the functions now check for TOS_FAILURE, TOS_NOT_SUPPORTED and
> TOS_SUCCESS.
>
> On TOS_FAILURE a pr_err message is printed informing the user of the
> error (no change was made to this, except the check was added to the
> functions not checking for this).
>
> On TOS_NOT_SUPPORTED we now return -ENODEV immediately (some
> functions were returning -EIO)
>
> On TOS_SUCCESS we now return 0, as a side effect, a new success value
> was added, since some functions return one instead of zero to
> indicate success.
>
> As a special case, the LED functions only check for TOS_FAILURE on
> *set, and check for TOS_FAILURE and TOS_SUCCESS on *get with their
> default return value set to LED_OFF.
>
> Also the {lcd, video}_proc* functions were adapted to reflect these
> changes to their parent HCI functions.
>
> Signed-off-by: Azael Avalos <[email protected]>
> ---
> drivers/platform/x86/toshiba_acpi.c | 432 ++++++++++++++++++------------------
> 1 file changed, 217 insertions(+), 215 deletions(-)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index e24f0f5..0034341 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -93,6 +93,7 @@ MODULE_LICENSE("GPL");
>
> /* Return codes */
> #define TOS_SUCCESS 0x0000
> +#define TOS_SUCCESS2 0x0001

UGH, thanks Toshiba :-(

> #define TOS_OPEN_CLOSE_OK 0x0044
> #define TOS_FAILURE 0x1000
> #define TOS_NOT_SUPPORTED 0x8000
> @@ -467,7 +468,8 @@ static void toshiba_illumination_set(struct led_classdev *cdev,
> {
> struct toshiba_acpi_dev *dev = container_of(cdev,
> struct toshiba_acpi_dev, led_dev);
> - u32 state, result;
> + u32 result;
> + u32 state;
>

I should add that this is a apparently a preference of mine, and other
maintainers do not enforce this, some actively discourage it. I apologize for
this inconsistency among the maintainers, it came to my attention that the very
person I was modeling this preference after in fact feels the opposite. Sigh.
For the time being, we stick with the preference I've stated, for consistency
within this file and through drivers/platform/x86 if nothing else.

> /* First request : initialize communication. */
> if (!sci_open(dev))
> @@ -479,8 +481,6 @@ static void toshiba_illumination_set(struct led_classdev *cdev,
> sci_close(dev);
> if (result == TOS_FAILURE)
> pr_err("ACPI call for illumination failed\n");
> - else if (result == TOS_NOT_SUPPORTED)
> - return;
> }
>
> static enum led_brightness toshiba_illumination_get(struct led_classdev *cdev)
> @@ -496,14 +496,12 @@ static enum led_brightness toshiba_illumination_get(struct led_classdev *cdev)
> /* Check the illumination */
> result = sci_read(dev, SCI_ILLUMINATION, &state);
> sci_close(dev);
> - if (result == TOS_FAILURE || result == TOS_INPUT_DATA_ERROR) {
> + if (result == TOS_FAILURE)
> pr_err("ACPI call for illumination failed\n");
> - return LED_OFF;
> - } else if (result == TOS_NOT_SUPPORTED) {
> - return LED_OFF;
> - }
> + else if (result == TOS_SUCCESS)
> + return state ? LED_FULL : LED_OFF;
>

I believe it is more typical, and therefor more natural to my eye, to test for
failure.

else if (result != TOS_SUCCESS)
return LED_OFF;

return state ? LED_FULL : LED_OFF;
}

Applies throughout. However, there is of course no functional difference and
others may argue differently. I'm mentioning it because there is an issue that
requires a respin below. I'll leave it to you which way you want to handle this
in this driver.

...

>
> static int video_proc_show(struct seq_file *m, void *v)
> {
> struct toshiba_acpi_dev *dev = m->private;
> u32 value;
> - int ret;
>
> - 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;
> + if (get_video_status(dev, &value))
> + return -EIO;
>
> - 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);
> - }
> + int is_lcd = (value & HCI_VIDEO_OUT_LCD) ? 1 : 0;

In this case, these need to be defined above. Your build test should have caught
this:

drivers/platform/x86/toshiba_acpi.c:1292:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
int is_lcd = (value & HCI_VIDEO_OUT_LCD) ? 1 : 0;
^

Did it not?

...

Thanks,

--
Darren Hart
Intel Open Source Technology Center

2015-07-29 04:54:13

by Darren Hart

[permalink] [raw]
Subject: Re: [PATCH v2] toshiba_acpi: Add set_fan_status function

On Mon, Jul 27, 2015 at 07:22:23PM -0600, Azael Avalos wrote:
> This patch adds a new function named "set_fan_status" to complement
> its get* counterpart, as well as to avoid code duplication between
> "fan_proc_write" and "fan_store".
>
> Also, both functions (get*, set*) are now checking for TOS_FAILURE,
> TOS_NOT_SUPPORTED and TOS_SUCCESS (to be on par with the rest of the
> HCI/SCI functions), printing an error message, returning -ENODEV and
> zero respectively.
>
> The proc and sysfs functions were updated to reflect these changes as
> well, returning -EIO for proc, and propagating the error value on the
> sysfs functions.
>
> Signed-off-by: Azael Avalos <[email protected]>

Queued, thanks Azael!

--
Darren Hart
Intel Open Source Technology Center

2015-07-29 05:03:04

by Darren Hart

[permalink] [raw]
Subject: Re: [PATCH v2 1/4] toshiba_acpi: Change *available functions return type

On Mon, Jul 27, 2015 at 07:22:25PM -0600, Azael Avalos wrote:
> This patch changes the *available functions return type from int to
> void.
>
> The checks for support of their respective features are done inside
> such functions and there was no need to return anything as we can
> flag the queried feature as supported inside these functions.
>
> The code was adapted accordingly to these changes and two new
> variables were created and another was changed from uint to bool.
>
> While at the neighborhood, some of the functions were cleaned from

Please consider this a red flag in the future. "While in the neighborhood" means
you need to split the patch across distinct functional boundaries.

--
Darren Hart
Intel Open Source Technology Center

2015-07-29 05:06:52

by Darren Hart

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] toshiba_acpi: Remove "*not supported" feature prints

Hi Azael,

On Mon, Jul 27, 2015 at 07:22:26PM -0600, Azael Avalos wrote:
> Currently the driver prints "*not supported" if any of the features
> queried are in fact not supported, let us print the available
> features instead.
>
> This patch removes all instances pr_info printing "*not supported",
> and add a new function called "print_supported_features", which will
> print the available laptop features.
>
> Signed-off-by: Azael Avalos <[email protected]>
> ---
> drivers/platform/x86/toshiba_acpi.c | 69 +++++++++++++++++++++++--------------
> 1 file changed, 44 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index e164926..e24f0f5 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -477,13 +477,10 @@ static void toshiba_illumination_set(struct led_classdev *cdev,
> state = brightness ? 1 : 0;
> result = sci_write(dev, SCI_ILLUMINATION, state);
> sci_close(dev);
> - if (result == TOS_FAILURE) {
> + if (result == TOS_FAILURE)
> pr_err("ACPI call for illumination failed\n");
> + else if (result == TOS_NOT_SUPPORTED)
> return;

And otherwise.... return. So this last if test doesn't do anything. If you feel
the context is relevant, consider just including it as a comment.

Thanks,

--
Darren Hart
Intel Open Source Technology Center