diff mbox series

[v2,08/24] platform/x86: ideapad-laptop: convert ACPI helpers to return -EIO in case of failure

Message ID 20210113182016.166049-9-pobrn@protonmail.com (mailing list archive)
State Changes Requested, archived
Headers show
Series platform/x86: ideapad-laptop: cleanup, keyboard backlight and "always on USB charging" control support, reenable touchpad control | expand

Commit Message

Barnabás Pőcze Jan. 13, 2021, 6:21 p.m. UTC 
ACPI helpers returned -1 in case of failure. Convert these functions to
return appropriate error codes, and convert their users to propagate
these error codes accordingly.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Comments

Andy Shevchenko Jan. 16, 2021, 7:47 p.m. UTC | #1 
On Wed, Jan 13, 2021 at 8:22 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> ACPI helpers returned -1 in case of failure. Convert these functions to
> return appropriate error codes, and convert their users to propagate
> these error codes accordingly.

...

> -       int val;
> +       int val, err;
>         unsigned long int end_jiffies;

Perhaps in this and other similar cases switch to reversed xmas tree
order at the same time?
Barnabás Pőcze Jan. 16, 2021, 8:28 p.m. UTC | #2 
Hi


> On Wed, Jan 13, 2021 at 8:22 PM Barnabás Pőcze wrote:
> >
> > ACPI helpers returned -1 in case of failure. Convert these functions to
> > return appropriate error codes, and convert their users to propagate
> > these error codes accordingly.
>
> ...
>
> > -       int val;
> > +       int val, err;
> >         unsigned long int end_jiffies;
>
> Perhaps in this and other similar cases switch to reversed xmas tree
> order at the same time?
> [...]

Thanks for the review; I intentionally tried to make as few modifications
as possible in order to achieve what I wanted. I deemed it better to
place all "coding style"-related changes in their own patch (19).

I would prefer to keep it this way. Do you have any objections?


Thanks,
Barnabás Pőcze
Andy Shevchenko Jan. 16, 2021, 8:42 p.m. UTC | #3 
On Sat, Jan 16, 2021 at 10:28 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> > On Wed, Jan 13, 2021 at 8:22 PM Barnabás Pőcze wrote:
> > > ACPI helpers returned -1 in case of failure. Convert these functions to
> > > return appropriate error codes, and convert their users to propagate
> > > these error codes accordingly.
> >
> > ...
> >
> > > -       int val;
> > > +       int val, err;
> > >         unsigned long int end_jiffies;
> >
> > Perhaps in this and other similar cases switch to reversed xmas tree
> > order at the same time?
> > [...]
>
> Thanks for the review; I intentionally tried to make as few modifications
> as possible in order to achieve what I wanted. I deemed it better to
> place all "coding style"-related changes in their own patch (19).
>
> I would prefer to keep it this way. Do you have any objections?

Yes I have. What you are doing is called ping-pong patch series style,
which means it introduces / doesn't fix the (side) problem in the code
it provides.
It has no difference in this patch where to place a line which you have changed.

 +       int val, err;
         unsigned long int end_jiffies;

is the same as

         unsigned long int end_jiffies;
 +       int val, err;

I don't understand what "few modifications" you mentioned above?
Barnabás Pőcze Jan. 16, 2021, 9:28 p.m. UTC | #4 
Hi


2021. január 16., szombat 21:42 keltezéssel, Andy Shevchenko írta:

> On Sat, Jan 16, 2021 at 10:28 PM Barnabás Pőcze wrote:
> > > On Wed, Jan 13, 2021 at 8:22 PM Barnabás Pőcze wrote:
> > > > ACPI helpers returned -1 in case of failure. Convert these functions to
> > > > return appropriate error codes, and convert their users to propagate
> > > > these error codes accordingly.
> > >
> > > ...
> > >
> > > > -       int val;
> > > > +       int val, err;
> > > >         unsigned long int end_jiffies;
> > >
> > > Perhaps in this and other similar cases switch to reversed xmas tree
> > > order at the same time?
> > > [...]
> >
> > Thanks for the review; I intentionally tried to make as few modifications
> > as possible in order to achieve what I wanted. I deemed it better to
> > place all "coding style"-related changes in their own patch (19).
> >
> > I would prefer to keep it this way. Do you have any objections?
>
> Yes I have. What you are doing is called ping-pong patch series style,
> which means it introduces / doesn't fix the (side) problem in the code
> it provides.
> It has no difference in this patch where to place a line which you have changed.
>
>  +       int val, err;
>          unsigned long int end_jiffies;
>
> is the same as
>
>          unsigned long int end_jiffies;
>  +       int val, err;
>

I see what you mean, sorry, please ignore what I said, it has no
relevance here. I'll change the order here and take a look at the
other commits with this in mind.


> I don't understand what "few modifications" you mentioned above?
> [...]

In other commits there were instances where I could've made
similar changes, but I chose not to, because I wanted to keep
the "stylistic" and functional changes separate.
For example, in patch 9:

@@ -353,9 +353,11 @@ static ssize_t show_ideapad_cam(struct device *dev,
 {
 	unsigned long result;
 	struct ideapad_private *priv = dev_get_drvdata(dev);
+	int err;

I did not change the order. Is that OK or do you think it'd be
preferable to change the order here as well?


Thanks,
Barnabás Pőcze
Andy Shevchenko Jan. 17, 2021, 9:04 p.m. UTC | #5 
On Sat, Jan 16, 2021 at 11:28 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> 2021. január 16., szombat 21:42 keltezéssel, Andy Shevchenko írta:
> > On Sat, Jan 16, 2021 at 10:28 PM Barnabás Pőcze wrote:

...

> > It has no difference in this patch where to place a line which you have changed.
> >
> >  +       int val, err;
> >          unsigned long int end_jiffies;
> >
> > is the same as
> >
> >          unsigned long int end_jiffies;
> >  +       int val, err;

(1)

> > I don't understand what "few modifications" you mentioned above?
> > [...]
>
> In other commits there were instances where I could've made
> similar changes, but I chose not to, because I wanted to keep
> the "stylistic" and functional changes separate.
> For example, in patch 9:
>
> @@ -353,9 +353,11 @@ static ssize_t show_ideapad_cam(struct device *dev,
>  {
>         unsigned long result;
>         struct ideapad_private *priv = dev_get_drvdata(dev);
> +       int err;

(2)

> I did not change the order. Is that OK or do you think it'd be
> preferable to change the order here as well?

I see, The case (1) above is different to this. and actually in (2)
you do right.
And I agree that in latter case (2) the stylistic should be *a
separate* change, which is completely the right thing to do.
diff mbox series

Patch

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index b0d8e332b48a..9bc6c7340876 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -117,7 +117,7 @@  static int read_method_int(acpi_handle handle, const char *method, int *val)
 	status = acpi_evaluate_integer(handle, (char *)method, NULL, &result);
 	if (ACPI_FAILURE(status)) {
 		*val = -1;
-		return -1;
+		return -EIO;
 	}
 	*val = result;
 	return 0;
@@ -138,7 +138,7 @@  static int method_int1(acpi_handle handle, char *method, int cmd)
 	acpi_status status;
 
 	status = acpi_execute_simple_method(handle, method, cmd);
-	return ACPI_FAILURE(status) ? -1 : 0;
+	return ACPI_FAILURE(status) ? -EIO : 0;
 }
 
 static int method_vpcr(acpi_handle handle, int cmd, int *ret)
@@ -157,7 +157,7 @@  static int method_vpcr(acpi_handle handle, int cmd, int *ret)
 
 	if (ACPI_FAILURE(status)) {
 		*ret = -1;
-		return -1;
+		return -EIO;
 	}
 	*ret = result;
 	return 0;
@@ -179,54 +179,60 @@  static int method_vpcw(acpi_handle handle, int cmd, int data)
 
 	status = acpi_evaluate_object(handle, "VPCW", &params, NULL);
 	if (status != AE_OK)
-		return -1;
+		return -EIO;
 	return 0;
 }
 
 static int read_ec_data(acpi_handle handle, int cmd, unsigned long *data)
 {
-	int val;
+	int val, err;
 	unsigned long int end_jiffies;
 
-	if (method_vpcw(handle, 1, cmd))
-		return -1;
+	err = method_vpcw(handle, 1, cmd);
+	if (err)
+		return err;
 
 	for (end_jiffies = jiffies + msecs_to_jiffies(IDEAPAD_EC_TIMEOUT) + 1;
 	     time_before(jiffies, end_jiffies);) {
 		schedule();
-		if (method_vpcr(handle, 1, &val))
-			return -1;
+		err = method_vpcr(handle, 1, &val);
+		if (err)
+			return err;
 		if (val == 0) {
-			if (method_vpcr(handle, 0, &val))
-				return -1;
+			err = method_vpcr(handle, 0, &val);
+			if (err)
+				return err;
 			*data = val;
 			return 0;
 		}
 	}
 	acpi_handle_err(handle, "timeout in %s\n", __func__);
-	return -1;
+	return -ETIMEDOUT;
 }
 
 static int write_ec_cmd(acpi_handle handle, int cmd, unsigned long data)
 {
-	int val;
+	int val, err;
 	unsigned long int end_jiffies;
 
-	if (method_vpcw(handle, 0, data))
-		return -1;
-	if (method_vpcw(handle, 1, cmd))
-		return -1;
+	err = method_vpcw(handle, 0, data);
+	if (err)
+		return err;
+	err = method_vpcw(handle, 1, cmd);
+	if (err)
+		return err;
 
 	for (end_jiffies = jiffies + msecs_to_jiffies(IDEAPAD_EC_TIMEOUT) + 1;
 	     time_before(jiffies, end_jiffies);) {
 		schedule();
-		if (method_vpcr(handle, 1, &val))
-			return -1;
+		err = method_vpcr(handle, 1, &val);
+		if (err)
+			return err;
 		if (val == 0)
 			return 0;
 	}
 	acpi_handle_err(handle, "timeout in %s\n", __func__);
-	return -1;
+	return -ETIMEDOUT;
 }
 
 /*
@@ -365,8 +371,8 @@  static ssize_t store_ideapad_cam(struct device *dev,
 	if (sscanf(buf, "%i", &state) != 1)
 		return -EINVAL;
 	ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_CAMERA, state);
-	if (ret < 0)
-		return -EIO;
+	if (ret)
+		return ret;
 	return count;
 }
 
@@ -398,8 +404,8 @@  static ssize_t store_ideapad_fan(struct device *dev,
 	if (state < 0 || state > 4 || state == 3)
 		return -EINVAL;
 	ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_FAN, state);
-	if (ret < 0)
-		return -EIO;
+	if (ret)
+		return ret;
 	return count;
 }
 
@@ -431,8 +437,8 @@  static ssize_t __maybe_unused touchpad_store(struct device *dev,
 		return ret;
 
 	ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, state);
-	if (ret < 0)
-		return -EIO;
+	if (ret)
+		return ret;
 	return count;
 }
 
@@ -465,8 +471,8 @@  static ssize_t conservation_mode_store(struct device *dev,
 	ret = method_int1(priv->adev->handle, "SBMC", state ?
 					      BMCMD_CONSERVATION_ON :
 					      BMCMD_CONSERVATION_OFF);
-	if (ret < 0)
-		return -EIO;
+	if (ret)
+		return ret;
 	return count;
 }
 
@@ -503,8 +509,8 @@  static ssize_t fn_lock_store(struct device *dev,
 	ret = method_int1(priv->adev->handle, "SALS", state ?
 			  HACMD_FNLOCK_ON :
 			  HACMD_FNLOCK_OFF);
-	if (ret < 0)
-		return -EIO;
+	if (ret)
+		return ret;
 	return count;
 }
 
@@ -744,7 +750,8 @@  static void ideapad_check_special_buttons(struct ideapad_private *priv)
 {
 	unsigned long bit, value;
 
-	read_ec_data(priv->adev->handle, VPCCMD_R_SPECIAL_BUTTONS, &value);
+	if (read_ec_data(priv->adev->handle, VPCCMD_R_SPECIAL_BUTTONS, &value))
+		return;
 
 	for_each_set_bit(bit, &value, 16) {
 		switch (bit) {
@@ -772,28 +779,33 @@  static int ideapad_backlight_get_brightness(struct backlight_device *blightdev)
 {
 	struct ideapad_private *priv = bl_get_data(blightdev);
 	unsigned long now;
+	int err;
 
 	if (!priv)
 		return -EINVAL;
 
-	if (read_ec_data(priv->adev->handle, VPCCMD_R_BL, &now))
-		return -EIO;
+	err = read_ec_data(priv->adev->handle, VPCCMD_R_BL, &now);
+	if (err)
+		return err;
 	return now;
 }
 
 static int ideapad_backlight_update_status(struct backlight_device *blightdev)
 {
 	struct ideapad_private *priv = bl_get_data(blightdev);
+	int err;
 
 	if (!priv)
 		return -EINVAL;
 
-	if (write_ec_cmd(priv->adev->handle, VPCCMD_W_BL,
-			 blightdev->props.brightness))
-		return -EIO;
-	if (write_ec_cmd(priv->adev->handle, VPCCMD_W_BL_POWER,
-			 blightdev->props.power == FB_BLANK_POWERDOWN ? 0 : 1))
-		return -EIO;
+	err = write_ec_cmd(priv->adev->handle, VPCCMD_W_BL,
+			   blightdev->props.brightness);
+	if (err)
+		return err;
+	err = write_ec_cmd(priv->adev->handle, VPCCMD_W_BL_POWER,
+			   blightdev->props.power != FB_BLANK_POWERDOWN);
+	if (err)
+		return err;
 
 	return 0;
 }
@@ -808,13 +820,17 @@  static int ideapad_backlight_init(struct ideapad_private *priv)
 	struct backlight_device *blightdev;
 	struct backlight_properties props;
 	unsigned long max, now, power;
-
-	if (read_ec_data(priv->adev->handle, VPCCMD_R_BL_MAX, &max))
-		return -EIO;
-	if (read_ec_data(priv->adev->handle, VPCCMD_R_BL, &now))
-		return -EIO;
-	if (read_ec_data(priv->adev->handle, VPCCMD_R_BL_POWER, &power))
-		return -EIO;
+	int err;
+
+	err = read_ec_data(priv->adev->handle, VPCCMD_R_BL_MAX, &max);
+	if (err)
+		return err;
+	err = read_ec_data(priv->adev->handle, VPCCMD_R_BL, &now);
+	if (err)
+		return err;
+	err = read_ec_data(priv->adev->handle, VPCCMD_R_BL_POWER, &power);
+	if (err)
+		return err;
 
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.max_brightness = max;