[PATCH 0/3] toshiba_acpi: Functions cleanup

[PATCH 0/3] toshiba_acpi: Functions cleanup

[Azael Avalos]

These set of patches make some cleanup in some of the driver's functions.

The first patch changes the variables declaration, the second changes
the error checking logic and the third simply fixes a typo found while
making these changes.

Azael Avalos (3):
  toshiba_acpi: Cleanup variable declaration
  toshiba_acpi: Change error checking logic from TCI functions
  toshiba_acpi: Fix typo in *_cooling_method_set function

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

-- 
2.9.2

[PATCH 1/3] toshiba_acpi: Cleanup variable declaration

[Azael Avalos]

This patch moves all the multiple line variable declaration to a
single line declaration (except variables being initialized)
following the reverse tree order, to conform to the practices
of the kernel.

Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
---
 drivers/platform/x86/toshiba_acpi.c | 98 +++++++++++++------------------------
 1 file changed, 33 insertions(+), 65 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 9d60a40..c6fc5cc 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -321,10 +321,9 @@ static int write_acpi_int(const char *methodName, int val)
 static acpi_status tci_raw(struct toshiba_acpi_dev *dev,
 			   const u32 in[TCI_WORDS], u32 out[TCI_WORDS])
 {
+	union acpi_object in_objs[TCI_WORDS], out_objs[TCI_WORDS + 1];
 	struct acpi_object_list params;
-	union acpi_object in_objs[TCI_WORDS];
 	struct acpi_buffer results;
-	union acpi_object out_objs[TCI_WORDS + 1];
 	acpi_status status;
 	int i;
 
@@ -387,9 +386,8 @@ static int sci_open(struct toshiba_acpi_dev *dev)
 {
 	u32 in[TCI_WORDS] = { SCI_OPEN, 0, 0, 0, 0, 0 };
 	u32 out[TCI_WORDS];
-	acpi_status status;
+	acpi_status status = tci_raw(dev, in, out);
 
-	status = tci_raw(dev, in, out);
 	if  (ACPI_FAILURE(status)) {
 		pr_err("ACPI call to open SCI failed\n");
 		return 0;
@@ -425,9 +423,8 @@ static void sci_close(struct toshiba_acpi_dev *dev)
 {
 	u32 in[TCI_WORDS] = { SCI_CLOSE, 0, 0, 0, 0, 0 };
 	u32 out[TCI_WORDS];
-	acpi_status status;
+	acpi_status status = tci_raw(dev, in, out);
 
-	status = tci_raw(dev, in, out);
 	if (ACPI_FAILURE(status)) {
 		pr_err("ACPI call to close SCI failed\n");
 		return;
@@ -490,8 +487,7 @@ static void toshiba_illumination_set(struct led_classdev *cdev,
 {
 	struct toshiba_acpi_dev *dev = container_of(cdev,
 			struct toshiba_acpi_dev, led_dev);
-	u32 result;
-	u32 state;
+	u32 result, state;
 
 	/* First request : initialize communication. */
 	if (!sci_open(dev))
@@ -509,7 +505,7 @@ 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 state, result;
+	u32 result, state;
 
 	/* First request : initialize communication. */
 	if (!sci_open(dev))
@@ -604,8 +600,7 @@ 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 result;
-	u32 state;
+	u32 result, state;
 
 	/* Check the keyboard backlight state */
 	result = hci_read(dev, HCI_KBD_ILLUMINATION, &state);
@@ -624,8 +619,7 @@ static void toshiba_kbd_backlight_set(struct led_classdev *cdev,
 {
 	struct toshiba_acpi_dev *dev = container_of(cdev,
 			struct toshiba_acpi_dev, kbd_led);
-	u32 result;
-	u32 state;
+	u32 result, state;
 
 	/* Set the keyboard backlight state */
 	state = brightness ? 1 : 0;
@@ -672,9 +666,9 @@ static int toshiba_touchpad_get(struct toshiba_acpi_dev *dev, u32 *state)
 /* Eco Mode support */
 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];
+	acpi_status status;
 
 	dev->eco_supported = 0;
 	dev->eco_led_registered = false;
@@ -1282,9 +1276,8 @@ static struct proc_dir_entry *toshiba_proc_dir;
 /* LCD Brightness */
 static int __get_lcd_brightness(struct toshiba_acpi_dev *dev)
 {
-	u32 result;
-	u32 value;
 	int brightness = 0;
+	u32 result, value;
 
 	if (dev->tr_backlight_supported) {
 		int ret = get_tr_backlight_status(dev, &value);
@@ -1317,8 +1310,7 @@ static int get_lcd_brightness(struct backlight_device *bd)
 static int lcd_proc_show(struct seq_file *m, void *v)
 {
 	struct toshiba_acpi_dev *dev = m->private;
-	int levels;
-	int value;
+	int levels, value;
 
 	if (!dev->backlight_dev)
 		return -ENODEV;
@@ -1375,16 +1367,16 @@ 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_DATA(file_inode(file));
+	int levels, value;
 	char cmd[42];
 	size_t len;
-	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';
 
+	levels = dev->backlight_dev->props.max_brightness + 1;
 	if (sscanf(cmd, " brightness : %i", &value) != 1 &&
 	    value < 0 && value > levels)
 		return -EINVAL;
@@ -1445,14 +1437,9 @@ 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 *buffer;
-	char *cmd;
+	int lcd_out, crt_out, tv_out, value, ret;
 	int remain = count;
-	int lcd_out = -1;
-	int crt_out = -1;
-	int tv_out = -1;
-	int value;
-	int ret;
+	char *buffer, *cmd;
 	u32 video_out;
 
 	cmd = kmalloc(count + 1, GFP_KERNEL);
@@ -1486,6 +1473,7 @@ static ssize_t video_proc_write(struct file *file, const char __user *buf,
 
 	kfree(cmd);
 
+	lcd_out = crt_out = tv_out = -1;
 	ret = get_video_status(dev, &video_out);
 	if (!ret) {
 		unsigned int new_video_out = video_out;
@@ -1722,8 +1710,7 @@ static ssize_t fan_store(struct device *dev,
 			 const char *buf, size_t count)
 {
 	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
-	int state;
-	int ret;
+	int state, ret;
 
 	ret = kstrtoint(buf, 0, &state);
 	if (ret)
@@ -1759,8 +1746,7 @@ static ssize_t kbd_backlight_mode_store(struct device *dev,
 					const char *buf, size_t count)
 {
 	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
-	int mode;
-	int ret;
+	int mode, ret;
 
 
 	ret = kstrtoint(buf, 0, &mode);
@@ -1872,8 +1858,7 @@ static ssize_t kbd_backlight_timeout_store(struct device *dev,
 					   const char *buf, size_t count)
 {
 	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
-	int time;
-	int ret;
+	int time, ret;
 
 	ret = kstrtoint(buf, 0, &time);
 	if (ret)
@@ -1929,8 +1914,7 @@ static ssize_t touchpad_store(struct device *dev,
 			      const char *buf, size_t count)
 {
 	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
-	int state;
-	int ret;
+	int state, ret;
 
 	/* Set the TouchPad on/off, 0 - Disable | 1 - Enable */
 	ret = kstrtoint(buf, 0, &state);
@@ -1980,9 +1964,8 @@ static ssize_t usb_sleep_charge_store(struct device *dev,
 				      const char *buf, size_t count)
 {
 	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
+	int state, ret;
 	u32 mode;
-	int state;
-	int ret;
 
 	ret = kstrtoint(buf, 0, &state);
 	if (ret)
@@ -2021,11 +2004,8 @@ static ssize_t sleep_functions_on_battery_show(struct device *dev,
 					       char *buf)
 {
 	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
+	int bat_lvl, status, ret, tmp;
 	u32 state;
-	int bat_lvl;
-	int status;
-	int ret;
-	int tmp;
 
 	ret = toshiba_sleep_functions_status_get(toshiba, &state);
 	if (ret < 0)
@@ -2045,10 +2025,8 @@ static ssize_t sleep_functions_on_battery_store(struct device *dev,
 						const char *buf, size_t count)
 {
 	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
+	int value, ret, tmp;
 	u32 status;
-	int value;
-	int ret;
-	int tmp;
 
 	ret = kstrtoint(buf, 0, &value);
 	if (ret)
@@ -2098,8 +2076,7 @@ static ssize_t usb_rapid_charge_store(struct device *dev,
 				      const char *buf, size_t count)
 {
 	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
-	int state;
-	int ret;
+	int state, ret;
 
 	ret = kstrtoint(buf, 0, &state);
 	if (ret)
@@ -2134,8 +2111,7 @@ static ssize_t usb_sleep_music_store(struct device *dev,
 				     const char *buf, size_t count)
 {
 	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
-	int state;
-	int ret;
+	int state, ret;
 
 	ret = kstrtoint(buf, 0, &state);
 	if (ret)
@@ -2155,8 +2131,7 @@ static ssize_t kbd_function_keys_show(struct device *dev,
 				      struct device_attribute *attr, char *buf)
 {
 	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
-	int mode;
-	int ret;
+	int mode, ret;
 
 	ret = toshiba_function_keys_get(toshiba, &mode);
 	if (ret < 0)
@@ -2170,8 +2145,7 @@ static ssize_t kbd_function_keys_store(struct device *dev,
 				       const char *buf, size_t count)
 {
 	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
-	int mode;
-	int ret;
+	int mode, ret;
 
 	ret = kstrtoint(buf, 0, &mode);
 	if (ret)
@@ -2213,8 +2187,7 @@ static ssize_t panel_power_on_store(struct device *dev,
 				    const char *buf, size_t count)
 {
 	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
-	int state;
-	int ret;
+	int state, ret;
 
 	ret = kstrtoint(buf, 0, &state);
 	if (ret)
@@ -2251,8 +2224,7 @@ static ssize_t usb_three_store(struct device *dev,
 			       const char *buf, size_t count)
 {
 	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
-	int state;
-	int ret;
+	int state, ret;
 
 	ret = kstrtoint(buf, 0, &state);
 	if (ret)
@@ -2279,8 +2251,7 @@ static ssize_t cooling_method_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
 	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
-	int state;
-	int ret;
+	int state, ret;
 
 	ret = toshiba_cooling_method_get(toshiba, &state);
 	if (ret < 0)
@@ -2294,8 +2265,7 @@ static ssize_t cooling_method_store(struct device *dev,
 				    const char *buf, size_t count)
 {
 	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
-	int state;
-	int ret;
+	int state, ret;
 
 	ret = kstrtoint(buf, 0, &state);
 	if (ret)
@@ -2714,8 +2684,7 @@ static void toshiba_acpi_process_hotkeys(struct toshiba_acpi_dev *dev)
 			dev->last_key_event = scancode;
 		}
 	} else if (dev->system_event_supported) {
-		u32 result;
-		u32 value;
+		u32 result, value;
 		int retries = 3;
 
 		do {
@@ -2844,8 +2813,7 @@ static int toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev)
 static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
 {
 	struct backlight_properties props;
-	int brightness;
-	int ret;
+	int brightness, ret;
 
 	/*
 	 * Some machines don't support the backlight methods at all, and
@@ -3015,8 +2983,8 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
 {
 	struct toshiba_acpi_dev *dev;
 	const char *hci_method;
-	u32 dummy;
 	int ret = 0;
+	u32 dummy;
 
 	if (toshiba_acpi)
 		return -EBUSY;
-- 
2.9.2

Re: [PATCH 1/3] toshiba_acpi: Cleanup variable declaration

[Darren Hart]

On Tue, Aug 16, 2016 at 12:06:15PM -0600, Azael Avalos wrote:
> This patch moves all the multiple line variable declaration to a
> single line declaration (except variables being initialized)
> following the reverse tree order, to conform to the practices
> of the kernel.

Azael,

Apologies for the delay, I was trying to decide what position I wanted to take
on this as anything I do builds precedent for future patches.

Early on I was pretty strict about this for new code. I've found that there is
less consensus about this among the maintainers than I initially thought. I'm
not sure it's worth a patch just to clean this up - but I also don't object,
especially given how actively you maintain this driver.

One minor point about consolidating similar types in one declaration line. We
prefer to group similar types of related purpose. For example:

int in_len, out_len;
int num_frames;
int ret;

Rather than:

int in_len, out_len, num_frames, ret;

With that in mind, I'll take this as is, or I'll wait for a respin. Up to you.

--
Darren

> 
> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
> ---
>  drivers/platform/x86/toshiba_acpi.c | 98 +++++++++++++------------------------
>  1 file changed, 33 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 9d60a40..c6fc5cc 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -321,10 +321,9 @@ static int write_acpi_int(const char *methodName, int val)
>  static acpi_status tci_raw(struct toshiba_acpi_dev *dev,
>  			   const u32 in[TCI_WORDS], u32 out[TCI_WORDS])
>  {
> +	union acpi_object in_objs[TCI_WORDS], out_objs[TCI_WORDS + 1];
>  	struct acpi_object_list params;
> -	union acpi_object in_objs[TCI_WORDS];
>  	struct acpi_buffer results;
> -	union acpi_object out_objs[TCI_WORDS + 1];
>  	acpi_status status;
>  	int i;
>  
> @@ -387,9 +386,8 @@ static int sci_open(struct toshiba_acpi_dev *dev)
>  {
>  	u32 in[TCI_WORDS] = { SCI_OPEN, 0, 0, 0, 0, 0 };
>  	u32 out[TCI_WORDS];
> -	acpi_status status;
> +	acpi_status status = tci_raw(dev, in, out);
>  
> -	status = tci_raw(dev, in, out);
>  	if  (ACPI_FAILURE(status)) {
>  		pr_err("ACPI call to open SCI failed\n");
>  		return 0;
> @@ -425,9 +423,8 @@ static void sci_close(struct toshiba_acpi_dev *dev)
>  {
>  	u32 in[TCI_WORDS] = { SCI_CLOSE, 0, 0, 0, 0, 0 };
>  	u32 out[TCI_WORDS];
> -	acpi_status status;
> +	acpi_status status = tci_raw(dev, in, out);
>  
> -	status = tci_raw(dev, in, out);
>  	if (ACPI_FAILURE(status)) {
>  		pr_err("ACPI call to close SCI failed\n");
>  		return;
> @@ -490,8 +487,7 @@ static void toshiba_illumination_set(struct led_classdev *cdev,
>  {
>  	struct toshiba_acpi_dev *dev = container_of(cdev,
>  			struct toshiba_acpi_dev, led_dev);
> -	u32 result;
> -	u32 state;
> +	u32 result, state;
>  
>  	/* First request : initialize communication. */
>  	if (!sci_open(dev))
> @@ -509,7 +505,7 @@ 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 state, result;
> +	u32 result, state;
>  
>  	/* First request : initialize communication. */
>  	if (!sci_open(dev))
> @@ -604,8 +600,7 @@ 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 result;
> -	u32 state;
> +	u32 result, state;
>  
>  	/* Check the keyboard backlight state */
>  	result = hci_read(dev, HCI_KBD_ILLUMINATION, &state);
> @@ -624,8 +619,7 @@ static void toshiba_kbd_backlight_set(struct led_classdev *cdev,
>  {
>  	struct toshiba_acpi_dev *dev = container_of(cdev,
>  			struct toshiba_acpi_dev, kbd_led);
> -	u32 result;
> -	u32 state;
> +	u32 result, state;
>  
>  	/* Set the keyboard backlight state */
>  	state = brightness ? 1 : 0;
> @@ -672,9 +666,9 @@ static int toshiba_touchpad_get(struct toshiba_acpi_dev *dev, u32 *state)
>  /* Eco Mode support */
>  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];
> +	acpi_status status;
>  
>  	dev->eco_supported = 0;
>  	dev->eco_led_registered = false;
> @@ -1282,9 +1276,8 @@ static struct proc_dir_entry *toshiba_proc_dir;
>  /* LCD Brightness */
>  static int __get_lcd_brightness(struct toshiba_acpi_dev *dev)
>  {
> -	u32 result;
> -	u32 value;
>  	int brightness = 0;
> +	u32 result, value;
>  
>  	if (dev->tr_backlight_supported) {
>  		int ret = get_tr_backlight_status(dev, &value);
> @@ -1317,8 +1310,7 @@ static int get_lcd_brightness(struct backlight_device *bd)
>  static int lcd_proc_show(struct seq_file *m, void *v)
>  {
>  	struct toshiba_acpi_dev *dev = m->private;
> -	int levels;
> -	int value;
> +	int levels, value;
>  
>  	if (!dev->backlight_dev)
>  		return -ENODEV;
> @@ -1375,16 +1367,16 @@ 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_DATA(file_inode(file));
> +	int levels, value;
>  	char cmd[42];
>  	size_t len;
> -	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';
>  
> +	levels = dev->backlight_dev->props.max_brightness + 1;
>  	if (sscanf(cmd, " brightness : %i", &value) != 1 &&
>  	    value < 0 && value > levels)
>  		return -EINVAL;
> @@ -1445,14 +1437,9 @@ 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 *buffer;
> -	char *cmd;
> +	int lcd_out, crt_out, tv_out, value, ret;
>  	int remain = count;
> -	int lcd_out = -1;
> -	int crt_out = -1;
> -	int tv_out = -1;
> -	int value;
> -	int ret;
> +	char *buffer, *cmd;
>  	u32 video_out;
>  
>  	cmd = kmalloc(count + 1, GFP_KERNEL);
> @@ -1486,6 +1473,7 @@ static ssize_t video_proc_write(struct file *file, const char __user *buf,
>  
>  	kfree(cmd);
>  
> +	lcd_out = crt_out = tv_out = -1;
>  	ret = get_video_status(dev, &video_out);
>  	if (!ret) {
>  		unsigned int new_video_out = video_out;
> @@ -1722,8 +1710,7 @@ static ssize_t fan_store(struct device *dev,
>  			 const char *buf, size_t count)
>  {
>  	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> -	int state;
> -	int ret;
> +	int state, ret;
>  
>  	ret = kstrtoint(buf, 0, &state);
>  	if (ret)
> @@ -1759,8 +1746,7 @@ static ssize_t kbd_backlight_mode_store(struct device *dev,
>  					const char *buf, size_t count)
>  {
>  	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> -	int mode;
> -	int ret;
> +	int mode, ret;
>  
>  
>  	ret = kstrtoint(buf, 0, &mode);
> @@ -1872,8 +1858,7 @@ static ssize_t kbd_backlight_timeout_store(struct device *dev,
>  					   const char *buf, size_t count)
>  {
>  	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> -	int time;
> -	int ret;
> +	int time, ret;
>  
>  	ret = kstrtoint(buf, 0, &time);
>  	if (ret)
> @@ -1929,8 +1914,7 @@ static ssize_t touchpad_store(struct device *dev,
>  			      const char *buf, size_t count)
>  {
>  	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> -	int state;
> -	int ret;
> +	int state, ret;
>  
>  	/* Set the TouchPad on/off, 0 - Disable | 1 - Enable */
>  	ret = kstrtoint(buf, 0, &state);
> @@ -1980,9 +1964,8 @@ static ssize_t usb_sleep_charge_store(struct device *dev,
>  				      const char *buf, size_t count)
>  {
>  	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> +	int state, ret;
>  	u32 mode;
> -	int state;
> -	int ret;
>  
>  	ret = kstrtoint(buf, 0, &state);
>  	if (ret)
> @@ -2021,11 +2004,8 @@ static ssize_t sleep_functions_on_battery_show(struct device *dev,
>  					       char *buf)
>  {
>  	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> +	int bat_lvl, status, ret, tmp;
>  	u32 state;
> -	int bat_lvl;
> -	int status;
> -	int ret;
> -	int tmp;
>  
>  	ret = toshiba_sleep_functions_status_get(toshiba, &state);
>  	if (ret < 0)
> @@ -2045,10 +2025,8 @@ static ssize_t sleep_functions_on_battery_store(struct device *dev,
>  						const char *buf, size_t count)
>  {
>  	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> +	int value, ret, tmp;
>  	u32 status;
> -	int value;
> -	int ret;
> -	int tmp;
>  
>  	ret = kstrtoint(buf, 0, &value);
>  	if (ret)
> @@ -2098,8 +2076,7 @@ static ssize_t usb_rapid_charge_store(struct device *dev,
>  				      const char *buf, size_t count)
>  {
>  	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> -	int state;
> -	int ret;
> +	int state, ret;
>  
>  	ret = kstrtoint(buf, 0, &state);
>  	if (ret)
> @@ -2134,8 +2111,7 @@ static ssize_t usb_sleep_music_store(struct device *dev,
>  				     const char *buf, size_t count)
>  {
>  	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> -	int state;
> -	int ret;
> +	int state, ret;
>  
>  	ret = kstrtoint(buf, 0, &state);
>  	if (ret)
> @@ -2155,8 +2131,7 @@ static ssize_t kbd_function_keys_show(struct device *dev,
>  				      struct device_attribute *attr, char *buf)
>  {
>  	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> -	int mode;
> -	int ret;
> +	int mode, ret;
>  
>  	ret = toshiba_function_keys_get(toshiba, &mode);
>  	if (ret < 0)
> @@ -2170,8 +2145,7 @@ static ssize_t kbd_function_keys_store(struct device *dev,
>  				       const char *buf, size_t count)
>  {
>  	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> -	int mode;
> -	int ret;
> +	int mode, ret;
>  
>  	ret = kstrtoint(buf, 0, &mode);
>  	if (ret)
> @@ -2213,8 +2187,7 @@ static ssize_t panel_power_on_store(struct device *dev,
>  				    const char *buf, size_t count)
>  {
>  	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> -	int state;
> -	int ret;
> +	int state, ret;
>  
>  	ret = kstrtoint(buf, 0, &state);
>  	if (ret)
> @@ -2251,8 +2224,7 @@ static ssize_t usb_three_store(struct device *dev,
>  			       const char *buf, size_t count)
>  {
>  	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> -	int state;
> -	int ret;
> +	int state, ret;
>  
>  	ret = kstrtoint(buf, 0, &state);
>  	if (ret)
> @@ -2279,8 +2251,7 @@ static ssize_t cooling_method_show(struct device *dev,
>  				   struct device_attribute *attr, char *buf)
>  {
>  	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> -	int state;
> -	int ret;
> +	int state, ret;
>  
>  	ret = toshiba_cooling_method_get(toshiba, &state);
>  	if (ret < 0)
> @@ -2294,8 +2265,7 @@ static ssize_t cooling_method_store(struct device *dev,
>  				    const char *buf, size_t count)
>  {
>  	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> -	int state;
> -	int ret;
> +	int state, ret;
>  
>  	ret = kstrtoint(buf, 0, &state);
>  	if (ret)
> @@ -2714,8 +2684,7 @@ static void toshiba_acpi_process_hotkeys(struct toshiba_acpi_dev *dev)
>  			dev->last_key_event = scancode;
>  		}
>  	} else if (dev->system_event_supported) {
> -		u32 result;
> -		u32 value;
> +		u32 result, value;
>  		int retries = 3;
>  
>  		do {
> @@ -2844,8 +2813,7 @@ static int toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev)
>  static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
>  {
>  	struct backlight_properties props;
> -	int brightness;
> -	int ret;
> +	int brightness, ret;
>  
>  	/*
>  	 * Some machines don't support the backlight methods at all, and
> @@ -3015,8 +2983,8 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>  {
>  	struct toshiba_acpi_dev *dev;
>  	const char *hci_method;
> -	u32 dummy;
>  	int ret = 0;
> +	u32 dummy;
>  
>  	if (toshiba_acpi)
>  		return -EBUSY;
> -- 
> 2.9.2
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 1/3] toshiba_acpi: Cleanup variable declaration

[Azael Avalos]

Hi Darren,

2016-08-28 10:49 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> On Tue, Aug 16, 2016 at 12:06:15PM -0600, Azael Avalos wrote:
>> This patch moves all the multiple line variable declaration to a
>> single line declaration (except variables being initialized)
>> following the reverse tree order, to conform to the practices
>> of the kernel.
>
> Azael,
>
> Apologies for the delay, I was trying to decide what position I wanted to take
> on this as anything I do builds precedent for future patches.
>
> Early on I was pretty strict about this for new code. I've found that there is
> less consensus about this among the maintainers than I initially thought. I'm
> not sure it's worth a patch just to clean this up - but I also don't object,
> especially given how actively you maintain this driver.
>
> One minor point about consolidating similar types in one declaration line. We
> prefer to group similar types of related purpose. For example:
>
> int in_len, out_len;
> int num_frames;
> int ret;
>
> Rather than:
>
> int in_len, out_len, num_frames, ret;
>
> With that in mind, I'll take this as is, or I'll wait for a respin. Up to you.

I'll send a v2 in a few as I caught a missing return on one of the functions
on patch 02.

>
> --
> Darren
>
>>
>> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
>> ---
>>  drivers/platform/x86/toshiba_acpi.c | 98 +++++++++++++------------------------
>>  1 file changed, 33 insertions(+), 65 deletions(-)
>>
>> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
>> index 9d60a40..c6fc5cc 100644
>> --- a/drivers/platform/x86/toshiba_acpi.c
>> +++ b/drivers/platform/x86/toshiba_acpi.c
>> @@ -321,10 +321,9 @@ static int write_acpi_int(const char *methodName, int val)
>>  static acpi_status tci_raw(struct toshiba_acpi_dev *dev,
>>                          const u32 in[TCI_WORDS], u32 out[TCI_WORDS])
>>  {
>> +     union acpi_object in_objs[TCI_WORDS], out_objs[TCI_WORDS + 1];
>>       struct acpi_object_list params;
>> -     union acpi_object in_objs[TCI_WORDS];
>>       struct acpi_buffer results;
>> -     union acpi_object out_objs[TCI_WORDS + 1];
>>       acpi_status status;
>>       int i;
>>
>> @@ -387,9 +386,8 @@ static int sci_open(struct toshiba_acpi_dev *dev)
>>  {
>>       u32 in[TCI_WORDS] = { SCI_OPEN, 0, 0, 0, 0, 0 };
>>       u32 out[TCI_WORDS];
>> -     acpi_status status;
>> +     acpi_status status = tci_raw(dev, in, out);
>>
>> -     status = tci_raw(dev, in, out);
>>       if  (ACPI_FAILURE(status)) {
>>               pr_err("ACPI call to open SCI failed\n");
>>               return 0;
>> @@ -425,9 +423,8 @@ static void sci_close(struct toshiba_acpi_dev *dev)
>>  {
>>       u32 in[TCI_WORDS] = { SCI_CLOSE, 0, 0, 0, 0, 0 };
>>       u32 out[TCI_WORDS];
>> -     acpi_status status;
>> +     acpi_status status = tci_raw(dev, in, out);
>>
>> -     status = tci_raw(dev, in, out);
>>       if (ACPI_FAILURE(status)) {
>>               pr_err("ACPI call to close SCI failed\n");
>>               return;
>> @@ -490,8 +487,7 @@ static void toshiba_illumination_set(struct led_classdev *cdev,
>>  {
>>       struct toshiba_acpi_dev *dev = container_of(cdev,
>>                       struct toshiba_acpi_dev, led_dev);
>> -     u32 result;
>> -     u32 state;
>> +     u32 result, state;
>>
>>       /* First request : initialize communication. */
>>       if (!sci_open(dev))
>> @@ -509,7 +505,7 @@ 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 state, result;
>> +     u32 result, state;
>>
>>       /* First request : initialize communication. */
>>       if (!sci_open(dev))
>> @@ -604,8 +600,7 @@ 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 result;
>> -     u32 state;
>> +     u32 result, state;
>>
>>       /* Check the keyboard backlight state */
>>       result = hci_read(dev, HCI_KBD_ILLUMINATION, &state);
>> @@ -624,8 +619,7 @@ static void toshiba_kbd_backlight_set(struct led_classdev *cdev,
>>  {
>>       struct toshiba_acpi_dev *dev = container_of(cdev,
>>                       struct toshiba_acpi_dev, kbd_led);
>> -     u32 result;
>> -     u32 state;
>> +     u32 result, state;
>>
>>       /* Set the keyboard backlight state */
>>       state = brightness ? 1 : 0;
>> @@ -672,9 +666,9 @@ static int toshiba_touchpad_get(struct toshiba_acpi_dev *dev, u32 *state)
>>  /* Eco Mode support */
>>  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];
>> +     acpi_status status;
>>
>>       dev->eco_supported = 0;
>>       dev->eco_led_registered = false;
>> @@ -1282,9 +1276,8 @@ static struct proc_dir_entry *toshiba_proc_dir;
>>  /* LCD Brightness */
>>  static int __get_lcd_brightness(struct toshiba_acpi_dev *dev)
>>  {
>> -     u32 result;
>> -     u32 value;
>>       int brightness = 0;
>> +     u32 result, value;
>>
>>       if (dev->tr_backlight_supported) {
>>               int ret = get_tr_backlight_status(dev, &value);
>> @@ -1317,8 +1310,7 @@ static int get_lcd_brightness(struct backlight_device *bd)
>>  static int lcd_proc_show(struct seq_file *m, void *v)
>>  {
>>       struct toshiba_acpi_dev *dev = m->private;
>> -     int levels;
>> -     int value;
>> +     int levels, value;
>>
>>       if (!dev->backlight_dev)
>>               return -ENODEV;
>> @@ -1375,16 +1367,16 @@ 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_DATA(file_inode(file));
>> +     int levels, value;
>>       char cmd[42];
>>       size_t len;
>> -     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';
>>
>> +     levels = dev->backlight_dev->props.max_brightness + 1;
>>       if (sscanf(cmd, " brightness : %i", &value) != 1 &&
>>           value < 0 && value > levels)
>>               return -EINVAL;
>> @@ -1445,14 +1437,9 @@ 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 *buffer;
>> -     char *cmd;
>> +     int lcd_out, crt_out, tv_out, value, ret;
>>       int remain = count;
>> -     int lcd_out = -1;
>> -     int crt_out = -1;
>> -     int tv_out = -1;
>> -     int value;
>> -     int ret;
>> +     char *buffer, *cmd;
>>       u32 video_out;
>>
>>       cmd = kmalloc(count + 1, GFP_KERNEL);
>> @@ -1486,6 +1473,7 @@ static ssize_t video_proc_write(struct file *file, const char __user *buf,
>>
>>       kfree(cmd);
>>
>> +     lcd_out = crt_out = tv_out = -1;
>>       ret = get_video_status(dev, &video_out);
>>       if (!ret) {
>>               unsigned int new_video_out = video_out;
>> @@ -1722,8 +1710,7 @@ static ssize_t fan_store(struct device *dev,
>>                        const char *buf, size_t count)
>>  {
>>       struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
>> -     int state;
>> -     int ret;
>> +     int state, ret;
>>
>>       ret = kstrtoint(buf, 0, &state);
>>       if (ret)
>> @@ -1759,8 +1746,7 @@ static ssize_t kbd_backlight_mode_store(struct device *dev,
>>                                       const char *buf, size_t count)
>>  {
>>       struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
>> -     int mode;
>> -     int ret;
>> +     int mode, ret;
>>
>>
>>       ret = kstrtoint(buf, 0, &mode);
>> @@ -1872,8 +1858,7 @@ static ssize_t kbd_backlight_timeout_store(struct device *dev,
>>                                          const char *buf, size_t count)
>>  {
>>       struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
>> -     int time;
>> -     int ret;
>> +     int time, ret;
>>
>>       ret = kstrtoint(buf, 0, &time);
>>       if (ret)
>> @@ -1929,8 +1914,7 @@ static ssize_t touchpad_store(struct device *dev,
>>                             const char *buf, size_t count)
>>  {
>>       struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
>> -     int state;
>> -     int ret;
>> +     int state, ret;
>>
>>       /* Set the TouchPad on/off, 0 - Disable | 1 - Enable */
>>       ret = kstrtoint(buf, 0, &state);
>> @@ -1980,9 +1964,8 @@ static ssize_t usb_sleep_charge_store(struct device *dev,
>>                                     const char *buf, size_t count)
>>  {
>>       struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
>> +     int state, ret;
>>       u32 mode;
>> -     int state;
>> -     int ret;
>>
>>       ret = kstrtoint(buf, 0, &state);
>>       if (ret)
>> @@ -2021,11 +2004,8 @@ static ssize_t sleep_functions_on_battery_show(struct device *dev,
>>                                              char *buf)
>>  {
>>       struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
>> +     int bat_lvl, status, ret, tmp;
>>       u32 state;
>> -     int bat_lvl;
>> -     int status;
>> -     int ret;
>> -     int tmp;
>>
>>       ret = toshiba_sleep_functions_status_get(toshiba, &state);
>>       if (ret < 0)
>> @@ -2045,10 +2025,8 @@ static ssize_t sleep_functions_on_battery_store(struct device *dev,
>>                                               const char *buf, size_t count)
>>  {
>>       struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
>> +     int value, ret, tmp;
>>       u32 status;
>> -     int value;
>> -     int ret;
>> -     int tmp;
>>
>>       ret = kstrtoint(buf, 0, &value);
>>       if (ret)
>> @@ -2098,8 +2076,7 @@ static ssize_t usb_rapid_charge_store(struct device *dev,
>>                                     const char *buf, size_t count)
>>  {
>>       struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
>> -     int state;
>> -     int ret;
>> +     int state, ret;
>>
>>       ret = kstrtoint(buf, 0, &state);
>>       if (ret)
>> @@ -2134,8 +2111,7 @@ static ssize_t usb_sleep_music_store(struct device *dev,
>>                                    const char *buf, size_t count)
>>  {
>>       struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
>> -     int state;
>> -     int ret;
>> +     int state, ret;
>>
>>       ret = kstrtoint(buf, 0, &state);
>>       if (ret)
>> @@ -2155,8 +2131,7 @@ static ssize_t kbd_function_keys_show(struct device *dev,
>>                                     struct device_attribute *attr, char *buf)
>>  {
>>       struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
>> -     int mode;
>> -     int ret;
>> +     int mode, ret;
>>
>>       ret = toshiba_function_keys_get(toshiba, &mode);
>>       if (ret < 0)
>> @@ -2170,8 +2145,7 @@ static ssize_t kbd_function_keys_store(struct device *dev,
>>                                      const char *buf, size_t count)
>>  {
>>       struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
>> -     int mode;
>> -     int ret;
>> +     int mode, ret;
>>
>>       ret = kstrtoint(buf, 0, &mode);
>>       if (ret)
>> @@ -2213,8 +2187,7 @@ static ssize_t panel_power_on_store(struct device *dev,
>>                                   const char *buf, size_t count)
>>  {
>>       struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
>> -     int state;
>> -     int ret;
>> +     int state, ret;
>>
>>       ret = kstrtoint(buf, 0, &state);
>>       if (ret)
>> @@ -2251,8 +2224,7 @@ static ssize_t usb_three_store(struct device *dev,
>>                              const char *buf, size_t count)
>>  {
>>       struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
>> -     int state;
>> -     int ret;
>> +     int state, ret;
>>
>>       ret = kstrtoint(buf, 0, &state);
>>       if (ret)
>> @@ -2279,8 +2251,7 @@ static ssize_t cooling_method_show(struct device *dev,
>>                                  struct device_attribute *attr, char *buf)
>>  {
>>       struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
>> -     int state;
>> -     int ret;
>> +     int state, ret;
>>
>>       ret = toshiba_cooling_method_get(toshiba, &state);
>>       if (ret < 0)
>> @@ -2294,8 +2265,7 @@ static ssize_t cooling_method_store(struct device *dev,
>>                                   const char *buf, size_t count)
>>  {
>>       struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
>> -     int state;
>> -     int ret;
>> +     int state, ret;
>>
>>       ret = kstrtoint(buf, 0, &state);
>>       if (ret)
>> @@ -2714,8 +2684,7 @@ static void toshiba_acpi_process_hotkeys(struct toshiba_acpi_dev *dev)
>>                       dev->last_key_event = scancode;
>>               }
>>       } else if (dev->system_event_supported) {
>> -             u32 result;
>> -             u32 value;
>> +             u32 result, value;
>>               int retries = 3;
>>
>>               do {
>> @@ -2844,8 +2813,7 @@ static int toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev)
>>  static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
>>  {
>>       struct backlight_properties props;
>> -     int brightness;
>> -     int ret;
>> +     int brightness, ret;
>>
>>       /*
>>        * Some machines don't support the backlight methods at all, and
>> @@ -3015,8 +2983,8 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>>  {
>>       struct toshiba_acpi_dev *dev;
>>       const char *hci_method;
>> -     u32 dummy;
>>       int ret = 0;
>> +     u32 dummy;
>>
>>       if (toshiba_acpi)
>>               return -EBUSY;
>> --
>> 2.9.2
>>
>>
>
> --
> Darren Hart
> Intel Open Source Technology Center



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

[PATCH 2/3] toshiba_acpi: Change error checking logic from TCI functions

[Azael Avalos]

Currently the success/error checking logic is intermixed, making the
code a bit cumbersome to understand.

This patch changes the affected functions to first check for errors
and take appropriate actions, then check for the supported features.

This patch also separates the error check from the acpi_status and
the tci_raw function call error check, as those two are completely
unrelated and were nested in if/else statements.

Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
---
 drivers/platform/x86/toshiba_acpi.c | 222 ++++++++++++++++++++++--------------
 1 file changed, 135 insertions(+), 87 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index c6fc5cc..2256cf5 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -476,10 +476,15 @@ static void toshiba_illumination_available(struct toshiba_acpi_dev *dev)
 
 	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");
-	else if (out[0] == TOS_SUCCESS)
-		dev->illumination_supported = 1;
+		return;
+	}
+
+	if (out[0] != TOS_SUCCESS)
+		return;
+
+	dev->illumination_supported = 1;
 }
 
 static void toshiba_illumination_set(struct led_classdev *cdev,
@@ -542,24 +547,28 @@ static void toshiba_kbd_illum_available(struct toshiba_acpi_dev *dev)
 	sci_close(dev);
 	if (ACPI_FAILURE(status)) {
 		pr_err("ACPI call to query kbd illumination support failed\n");
-	} 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;
+		return;
 	}
+
+	if (out[0] != TOS_SUCCESS)
+		return;
+
+	/*
+	 * 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;
 }
 
 static int toshiba_kbd_illum_status_set(struct toshiba_acpi_dev *dev, u32 time)
@@ -676,7 +685,10 @@ static void toshiba_eco_mode_available(struct toshiba_acpi_dev *dev)
 	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_INPUT_DATA_ERROR) {
+		return;
+	}
+
+	if (out[0] == TOS_INPUT_DATA_ERROR) {
 		/*
 		 * If we receive 0x8300 (Input Data Error), it means that the
 		 * LED device is present, but that we just screwed the input
@@ -690,8 +702,11 @@ static void toshiba_eco_mode_available(struct toshiba_acpi_dev *dev)
 		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_SUCCESS)
-			dev->eco_supported = 1;
+
+		if (out[0] != TOS_SUCCESS)
+			return;
+
+		dev->eco_supported = 1;
 	}
 }
 
@@ -708,10 +723,11 @@ toshiba_eco_mode_get_status(struct led_classdev *cdev)
 	if (ACPI_FAILURE(status)) {
 		pr_err("ACPI call to get ECO led failed\n");
 		return LED_OFF;
-	} else if (out[0] != TOS_SUCCESS) {
-		return LED_OFF;
 	}
 
+	if (out[0] != TOS_SUCCESS)
+		return LED_OFF;
+
 	return out[2] ? LED_FULL : LED_OFF;
 }
 
@@ -745,10 +761,15 @@ static void toshiba_accelerometer_available(struct toshiba_acpi_dev *dev)
 	 * this call also serves as initialization
 	 */
 	status = tci_raw(dev, in, out);
-	if (ACPI_FAILURE(status))
+	if (ACPI_FAILURE(status)) {
 		pr_err("ACPI call to query the accelerometer failed\n");
-	else if (out[0] == TOS_SUCCESS)
-		dev->accelerometer_supported = 1;
+		return;
+	}
+
+	if (out[0] != TOS_SUCCESS)
+		return;
+
+	dev->accelerometer_supported = 1;
 }
 
 static int toshiba_accelerometer_get(struct toshiba_acpi_dev *dev,
@@ -763,15 +784,18 @@ static int toshiba_accelerometer_get(struct toshiba_acpi_dev *dev,
 	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;
 	}
 
-	return -EIO;
+	if (out[0] == TOS_NOT_SUPPORTED)
+		return -ENODEV;
+
+	if (out[0] != TOS_SUCCESS)
+		return -EIO;
+
+	*xy = out[2];
+	*z = out[4];
+
+	return 0;
 }
 
 /* Sleep (Charge and Music) utilities support */
@@ -791,24 +815,29 @@ 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) {
+	}
+
+	if (out[0] != TOS_SUCCESS) {
 		sci_close(dev);
 		return;
-	} else if (out[0] == TOS_SUCCESS) {
-		dev->usbsc_mode_base = out[4];
 	}
 
+	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");
-	} else if (out[0] == TOS_SUCCESS) {
-		dev->usbsc_bat_level = out[2];
-		/* Flag as supported */
-		dev->usb_sleep_charge_supported = 1;
+		return;
 	}
 
+	if (out[0] != TOS_SUCCESS)
+		return;
+
+	dev->usbsc_bat_level = out[2];
+	/* Flag as supported */
+	dev->usb_sleep_charge_supported = 1;
 }
 
 static int toshiba_usb_sleep_charge_get(struct toshiba_acpi_dev *dev,
@@ -862,14 +891,19 @@ 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");
-	} else if (out[0] == TOS_NOT_SUPPORTED) {
-		return -ENODEV;
-	} else if (out[0] == TOS_SUCCESS) {
-		*mode = out[2];
-		return 0;
+		return -EIO;
 	}
 
-	return -EIO;
+	if (out[0] == TOS_NOT_SUPPORTED)
+		return -ENODEV;
+
+	if (out[0] != TOS_SUCCESS)
+		return -EIO;
+
+	*mode = out[2];
+
+	return 0;
+
 }
 
 static int toshiba_sleep_functions_status_set(struct toshiba_acpi_dev *dev,
@@ -886,9 +920,12 @@ 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");
-	else if (out[0] == TOS_NOT_SUPPORTED)
+		return -EIO;
+	}
+
+	if (out[0] == TOS_NOT_SUPPORTED)
 		return -ENODEV;
 
 	return out[0] == TOS_SUCCESS ? 0 : -EIO;
@@ -909,14 +946,18 @@ 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");
-	} else if (out[0] == TOS_NOT_SUPPORTED) {
-		return -ENODEV;
-	} else if (out[0] == TOS_SUCCESS || out[0] == TOS_SUCCESS2) {
-		*state = out[2];
-		return 0;
+		return -EIO;
 	}
 
-	return -EIO;
+	if (out[0] == TOS_NOT_SUPPORTED)
+		return -ENODEV;
+
+	if (out[0] != TOS_SUCCESS && out[0] != TOS_SUCCESS2)
+		return -EIO;
+
+	*state = out[2];
+
+	return 0;
 }
 
 static int toshiba_usb_rapid_charge_set(struct toshiba_acpi_dev *dev,
@@ -933,9 +974,12 @@ 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");
-	else if (out[0] == TOS_NOT_SUPPORTED)
+		return -EIO;
+	}
+
+	if (out[0] == TOS_NOT_SUPPORTED)
 		return -ENODEV;
 
 	return (out[0] == TOS_SUCCESS || out[0] == TOS_SUCCESS2) ? 0 : -EIO;
@@ -1091,14 +1135,18 @@ 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");
-	} else if (out[0] == TOS_NOT_SUPPORTED) {
-		return -ENODEV;
-	} else if (out[0] == TOS_SUCCESS) {
-		*type = out[3];
-		return 0;
+		return -EIO;
 	}
 
-	return -EIO;
+	if (out[0] == TOS_NOT_SUPPORTED)
+		return -ENODEV;
+
+	if (out[0] != TOS_SUCCESS)
+		return -EIO;
+
+	*type = out[3];
+
+	return 0;
 }
 
 /* Wireless status (RFKill, WLAN, BT, WWAN) */
@@ -1148,7 +1196,6 @@ static void toshiba_wwan_available(struct toshiba_acpi_dev *dev)
 	 */
 	in[3] = HCI_WIRELESS_WWAN;
 	status = tci_raw(dev, in, out);
-
 	if (ACPI_FAILURE(status)) {
 		pr_err("ACPI call to get WWAN status failed\n");
 		return;
@@ -1168,7 +1215,6 @@ static int toshiba_wwan_set(struct toshiba_acpi_dev *dev, u32 state)
 
 	in[3] = HCI_WIRELESS_WWAN_STATUS;
 	status = tci_raw(dev, in, out);
-
 	if (ACPI_FAILURE(status)) {
 		pr_err("ACPI call to set WWAN status failed\n");
 		return -EIO;
@@ -1187,7 +1233,6 @@ static int toshiba_wwan_set(struct toshiba_acpi_dev *dev, u32 state)
 	 */
 	in[3] = HCI_WIRELESS_WWAN_POWER;
 	status = tci_raw(dev, in, out);
-
 	if (ACPI_FAILURE(status)) {
 		pr_err("ACPI call to set WWAN power failed\n");
 		return -EIO;
@@ -1210,8 +1255,10 @@ static void toshiba_cooling_method_available(struct toshiba_acpi_dev *dev)
 	dev->max_cooling_method = 0;
 
 	status = tci_raw(dev, in, out);
-	if (ACPI_FAILURE(status))
+	if (ACPI_FAILURE(status)) {
 		pr_err("ACPI call to get Cooling Method failed\n");
+		return;
+	}
 
 	if (out[0] != TOS_SUCCESS && out[0] != TOS_SUCCESS2)
 		return;
@@ -1294,10 +1341,10 @@ static int __get_lcd_brightness(struct toshiba_acpi_dev *dev)
 		pr_err("ACPI call to get LCD Brightness failed\n");
 	else if (result == TOS_NOT_SUPPORTED)
 		return -ENODEV;
-	if (result == TOS_SUCCESS)
-		return brightness + (value >> HCI_LCD_BRIGHTNESS_SHIFT);
 
-	return -EIO;
+	return result == TOS_SUCCESS ?
+			brightness + (value >> HCI_LCD_BRIGHTNESS_SHIFT) :
+			-EIO;
 }
 
 static int get_lcd_brightness(struct backlight_device *bd)
@@ -1317,15 +1364,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 value;
 	}
 
-	pr_err("Error reading LCD brightness\n");
+	seq_printf(m, "brightness:              %d\n", value);
+	seq_printf(m, "brightness_levels:       %d\n", levels);
 
-	return -EIO;
+	return 0;
 }
 
 static int lcd_proc_open(struct inode *inode, struct file *file)
@@ -1412,20 +1459,21 @@ static int get_video_status(struct toshiba_acpi_dev *dev, u32 *status)
 static int video_proc_show(struct seq_file *m, void *v)
 {
 	struct toshiba_acpi_dev *dev = m->private;
+	int is_lcd, is_crt, is_tv;
 	u32 value;
 
-	if (!get_video_status(dev, &value)) {
-		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);
-		return 0;
-	}
+	is_lcd = (value & HCI_VIDEO_OUT_LCD) ? 1 : 0;
+	is_crt = (value & HCI_VIDEO_OUT_CRT) ? 1 : 0;
+	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 -EIO;
+	return 0;
 }
 
 static int video_proc_open(struct inode *inode, struct file *file)
-- 
2.9.2

Re: [PATCH 2/3] toshiba_acpi: Change error checking logic from TCI functions

[Darren Hart]

On Tue, Aug 16, 2016 at 12:06:16PM -0600, Azael Avalos wrote:
> Currently the success/error checking logic is intermixed, making the
> code a bit cumbersome to understand.
> 
> This patch changes the affected functions to first check for errors
> and take appropriate actions, then check for the supported features.
> 
> This patch also separates the error check from the acpi_status and
> the tci_raw function call error check, as those two are completely
> unrelated and were nested in if/else statements.

Thanks, this is a good improvement. One questions below...

> 
> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
> ---
>  drivers/platform/x86/toshiba_acpi.c | 222 ++++++++++++++++++++++--------------
>  1 file changed, 135 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index c6fc5cc..2256cf5 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -476,10 +476,15 @@ static void toshiba_illumination_available(struct toshiba_acpi_dev *dev)
>  
>  	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");
> -	else if (out[0] == TOS_SUCCESS)
> -		dev->illumination_supported = 1;
> +		return;
> +	}
> +
> +	if (out[0] != TOS_SUCCESS)

Does this condition not merit a pr_err message? It reads like an error...

There are several similar situations below which are equally silent. Is this a
deliberate decision?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 2/3] toshiba_acpi: Change error checking logic from TCI functions

[Azael Avalos]

Hi Darren,

2016-08-28 10:56 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> On Tue, Aug 16, 2016 at 12:06:16PM -0600, Azael Avalos wrote:
>> Currently the success/error checking logic is intermixed, making the
>> code a bit cumbersome to understand.
>>
>> This patch changes the affected functions to first check for errors
>> and take appropriate actions, then check for the supported features.
>>
>> This patch also separates the error check from the acpi_status and
>> the tci_raw function call error check, as those two are completely
>> unrelated and were nested in if/else statements.
>
> Thanks, this is a good improvement. One questions below...
>
>>
>> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
>> ---
>>  drivers/platform/x86/toshiba_acpi.c | 222 ++++++++++++++++++++++--------------
>>  1 file changed, 135 insertions(+), 87 deletions(-)
>>
>> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
>> index c6fc5cc..2256cf5 100644
>> --- a/drivers/platform/x86/toshiba_acpi.c
>> +++ b/drivers/platform/x86/toshiba_acpi.c
>> @@ -476,10 +476,15 @@ static void toshiba_illumination_available(struct toshiba_acpi_dev *dev)
>>
>>       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");
>> -     else if (out[0] == TOS_SUCCESS)
>> -             dev->illumination_supported = 1;
>> +             return;
>> +     }
>> +
>> +     if (out[0] != TOS_SUCCESS)
>
> Does this condition not merit a pr_err message? It reads like an error...
>
> There are several similar situations below which are equally silent. Is this a
> deliberate decision?

This was on purpose, since we are querying for all the supported features,
the kernel log will contain a lot of error strings for not supported features,
as not all laptop models support all of them.

>
> --
> Darren Hart
> Intel Open Source Technology Center
> --
> 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



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

[PATCH 3/3] toshiba_acpi: Fix typo in *_cooling_method_set function

[Azael Avalos]

This patch simply fixes a typo in the error string printed in such
function.

Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
---
 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 2256cf5..254a4d5 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -1285,7 +1285,7 @@ static int toshiba_cooling_method_set(struct toshiba_acpi_dev *dev, u32 state)
 	u32 result = hci_write(dev, HCI_COOLING_METHOD, state);
 
 	if (result == TOS_FAILURE)
-		pr_err("ACPI call to get Cooling Method failed\n");
+		pr_err("ACPI call to set Cooling Method failed\n");
 
 	if (result == TOS_NOT_SUPPORTED)
 		return -ENODEV;
-- 
2.9.2

Re: [PATCH 3/3] toshiba_acpi: Fix typo in *_cooling_method_set function

[Darren Hart]

On Tue, Aug 16, 2016 at 12:06:17PM -0600, Azael Avalos wrote:
> This patch simply fixes a typo in the error string printed in such
> function.
> 
> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>

Thanks, queued to for-next.

-- 
Darren Hart
Intel Open Source Technology Center