Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1471932 > unrolled thread

[PATCH v2 0/3] toshiba_acpi: Functions cleanup

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/3] toshiba_acpi: Functions cleanup Azael Avalos <coproscefalo@gmail.com> - 2016-08-29 17:40 +0200
    [PATCH v2 1/3] toshiba_acpi: Clean up variable declaration Azael Avalos <coproscefalo@gmail.com> - 2016-08-29 17:40 +0200
      Re: [PATCH v2 1/3] toshiba_acpi: Clean up variable declaration Darren Hart <dvhart@infradead.org> - 2016-09-01 02:40 +0200
    Re: [PATCH v2 0/3] toshiba_acpi: Functions cleanup Darren Hart <dvhart@infradead.org> - 2016-09-01 02:50 +0200

#1471932 — [PATCH v2 0/3] toshiba_acpi: Functions cleanup

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.

Changes since v1:
- Small changes to patch 01 acording to Darren's comments
- Added a missing return on *eco_mode_available function

Azael Avalos (3):
  toshiba_acpi: Clean up 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 | 261 +++++++++++++++++++++---------------
 1 file changed, 154 insertions(+), 107 deletions(-)

-- 
2.9.3

[toc] | [next] | [standalone]

#1471934 — [PATCH v2 1/3] toshiba_acpi: Clean up variable declaration

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 | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 9d60a40..54dea64 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;
@@ -509,7 +506,8 @@ 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;
+	u32 state;
 
 	/* First request : initialize communication. */
 	if (!sci_open(dev))
@@ -672,9 +670,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 +1280,9 @@ static struct proc_dir_entry *toshiba_proc_dir;
 /* LCD Brightness */
 static int __get_lcd_brightness(struct toshiba_acpi_dev *dev)
 {
+	int brightness = 0;
 	u32 result;
 	u32 value;
-	int brightness = 0;
 
 	if (dev->tr_backlight_supported) {
 		int ret = get_tr_backlight_status(dev, &value);
@@ -1377,7 +1375,7 @@ 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 levels = dev->backlight_dev->props.max_brightness + 1;
+	int levels;
 	int value;
 
 	len = min(count, sizeof(cmd) - 1);
@@ -1385,6 +1383,7 @@ static ssize_t lcd_proc_write(struct file *file, const char __user *buf,
 		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;
@@ -1447,10 +1446,8 @@ static ssize_t video_proc_write(struct file *file, const char __user *buf,
 	struct toshiba_acpi_dev *dev = PDE_DATA(file_inode(file));
 	char *buffer;
 	char *cmd;
+	int lcd_out, crt_out, tv_out;
 	int remain = count;
-	int lcd_out = -1;
-	int crt_out = -1;
-	int tv_out = -1;
 	int value;
 	int ret;
 	u32 video_out;
@@ -1486,6 +1483,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;
@@ -1980,9 +1978,9 @@ 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);
-	u32 mode;
 	int state;
 	int ret;
+	u32 mode;
 
 	ret = kstrtoint(buf, 0, &state);
 	if (ret)
@@ -2021,11 +2019,10 @@ static ssize_t sleep_functions_on_battery_show(struct device *dev,
 					       char *buf)
 {
 	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
-	u32 state;
-	int bat_lvl;
-	int status;
+	int bat_lvl, status;
 	int ret;
 	int tmp;
+	u32 state;
 
 	ret = toshiba_sleep_functions_status_get(toshiba, &state);
 	if (ret < 0)
@@ -3015,8 +3012,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.3

[toc] | [prev] | [next] | [standalone]

#1473981 — Re: [PATCH v2 1/3] toshiba_acpi: Clean up variable declaration

On Mon, Aug 29, 2016 at 09:37:34AM -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.

So... I don't really want to spend a lot of time on this patch :-) but... there
are some inconsistencies with the stated intent....

> 
> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
> ---
>  drivers/platform/x86/toshiba_acpi.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 9d60a40..54dea64 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;
> @@ -509,7 +506,8 @@ 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;
> +	u32 state;
>  
>  	/* First request : initialize communication. */
>  	if (!sci_open(dev))
> @@ -672,9 +670,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 +1280,9 @@ static struct proc_dir_entry *toshiba_proc_dir;
>  /* LCD Brightness */
>  static int __get_lcd_brightness(struct toshiba_acpi_dev *dev)
>  {
> +	int brightness = 0;
>  	u32 result;
>  	u32 value;
> -	int brightness = 0;
>  
>  	if (dev->tr_backlight_supported) {
>  		int ret = get_tr_backlight_status(dev, &value);
> @@ -1377,7 +1375,7 @@ 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 levels = dev->backlight_dev->props.max_brightness + 1;
> +	int levels;
>  	int value;
>  
>  	len = min(count, sizeof(cmd) - 1);
> @@ -1385,6 +1383,7 @@ static ssize_t lcd_proc_write(struct file *file, const char __user *buf,
>  		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;
> @@ -1447,10 +1446,8 @@ static ssize_t video_proc_write(struct file *file, const char __user *buf,
>  	struct toshiba_acpi_dev *dev = PDE_DATA(file_inode(file));
>  	char *buffer;
>  	char *cmd;
> +	int lcd_out, crt_out, tv_out;
>  	int remain = count;
> -	int lcd_out = -1;
> -	int crt_out = -1;
> -	int tv_out = -1;
>  	int value;
>  	int ret;
>  	u32 video_out;
> @@ -1486,6 +1483,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;
> @@ -1980,9 +1978,9 @@ 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);
> -	u32 mode;
>  	int state;
>  	int ret;
> +	u32 mode;

This is not consistent with the stated changes in the changelog, u32 mode is
longer than int ret, and should come before.

>  
>  	ret = kstrtoint(buf, 0, &state);
>  	if (ret)
> @@ -2021,11 +2019,10 @@ static ssize_t sleep_functions_on_battery_show(struct device *dev,
>  					       char *buf)
>  {
>  	struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> -	u32 state;
> -	int bat_lvl;
> -	int status;
> +	int bat_lvl, status;
>  	int ret;
>  	int tmp;
> +	u32 state;

Here too (I'd just drop this change)

>  
>  	ret = toshiba_sleep_functions_status_get(toshiba, &state);
>  	if (ret < 0)
> @@ -3015,8 +3012,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;

And here, per your exception for initialization.

So, what I've done is fixed the last three instances, and queued this to
for-next to save both our time on back and forth. Let me know if you object.

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1473987

On Mon, Aug 29, 2016 at 09:37:33AM -0600, Azael Avalos wrote:
> 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.

Queued to for-next (with the qualifying comment to 1/3).

> 
> Changes since v1:
> - Small changes to patch 01 acording to Darren's comments
> - Added a missing return on *eco_mode_available function
> 

Please include the changelog in the individual patches below the --- per
Documentation/SubmittingPatches 14) The canonical patch format.

(for future reference)

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web