[PATCH v2 00/24] platform/x86: ideapad-laptop: cleanup, keyboard backlight and "always on USB charging" control support, reenable touchpad control

[PATCH v2 00/24] platform/x86: ideapad-laptop: cleanup, keyboard backlight and "always on USB charging" control support, reenable touchpad control

[Barnabás Pőcze]

This series contains patches that aim to bring more consistency
to the code; add keyboard backlight control support; add
"always on USB charging" control support.
Furthermore, commit 7f363145992cebf4ea760447f1cfdf6f81459683
("platform/x86: ideapad-laptop: Switch touchpad attribute to be RO")
is reverted since it made it impossible to disable/enable the touchpad via the
ideapad-laptop module and on some devices the method implemented in the
module works correctly to disable/enable the touchpad.

Changes in v2:
 - rebase on commit d69cd7eea93eb59a93061beeb43e4f5e19afc4ea
   ("platform/x86: ideapad-laptop: Disable touchpad_switch for ELAN0634")
 - [03] change how the #include directives are grouped and sorted
 - [13] change the order of members in the `features` struct in `struct ideapad_private`
        to roughly match that of the `ideapad_attributes` array
 - [15] change `CFG_CAP_TOUCHPAD_BIT` to 30 as it is the correct value instead of
        31 that appeared in the previous version
 - [19] remove extra whitespaces in the ACPI device id table
 - [21] select NEW_LEDS and LEDS_CLASS in Kconfig
 - [21] check for keyboard backlight support in `ideapad_check_features()`
        not separately in `ideapad_kbd_bl_init()`
 - [24] change KernelVersion of the `usb_charging` attribute to 5.12

History:
 - v1: https://lore.kernel.org/platform-driver-x86/20201216013857.360987-1-pobrn@protonmail.com/

Barnabás Pőcze (24):
  platform/x86: ideapad-laptop: remove unnecessary dev_set_drvdata()
    call
  platform/x86: ideapad-laptop: use appropriately typed variable to
    store the return value of ACPI methods
  platform/x86: ideapad-laptop: sort includes lexicographically
  platform/x86: ideapad-laptop: use sysfs_emit()
  platform/x86: ideapad-laptop: use for_each_set_bit() helper to
    simplify event processing
  platform/x86: ideapad-laptop: use msecs_to_jiffies() helper instead of
    hand-crafted formula
  platform/x86: ideapad-laptop: use dev_{err,warn} or appropriate
    variant to display log messages
  platform/x86: ideapad-laptop: convert ACPI helpers to return -EIO in
    case of failure
  platform/x86: ideapad-laptop: always propagate error codes from device
    attributes' show() callback
  platform/x86: ideapad-laptop: misc. device attribute changes
  platform/x86: ideapad-laptop: group and separate (un)related constants
    into enums
  platform/x86: ideapad-laptop: rework and create new ACPI helpers
  platform/x86: ideapad-laptop: rework is_visible() logic
  platform/x86: ideapad-laptop: check for Fn-lock support in HALS
  platform/x86: ideapad-laptop: check for touchpad support in _CFG
  platform/x86: ideapad-laptop: change 'status' debugfs file format
  platform/x86: ideapad-laptop: change 'cfg' debugfs file format
  Revert "platform/x86: ideapad-laptop: Switch touchpad attribute to be
    RO"
  platform/x86: ideapad-laptop: fix checkpatch warnings, more consistent
    style
  platform/x86: ideapad-laptop: send notification about touchpad state
    change to sysfs
  platform/x86: ideapad-laptop: add keyboard backlight control support
  platform/x86: ideapad-laptop: add "always on USB charging" control
    support
  Documentation/ABI: sysfs-platform-ideapad-laptop: update device
    attribute paths
  Documentation/ABI: sysfs-platform-ideapad-laptop: conservation_mode
    and usb_charging

 .../ABI/testing/sysfs-platform-ideapad-laptop |   26 +-
 drivers/platform/x86/Kconfig                  |    2 +
 drivers/platform/x86/ideapad-laptop.c         | 1063 +++++++++++------
 3 files changed, 702 insertions(+), 389 deletions(-)

-- 
2.30.0

[PATCH v2 01/24] platform/x86: ideapad-laptop: remove unnecessary dev_set_drvdata() call

[Barnabás Pőcze]

The driver core already sets the driver specific data on bind failure
or removal. Thus the call is unnecessary.

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

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 5b81bafa5c16..c96fd60ec7d2 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1084,7 +1084,6 @@ static int ideapad_acpi_remove(struct platform_device *pdev)
 	ideapad_input_exit(priv);
 	ideapad_debugfs_exit(priv);
 	ideapad_sysfs_exit(priv);
-	dev_set_drvdata(&pdev->dev, NULL);
 
 	return 0;
 }
-- 
2.30.0

[PATCH v2 02/24] platform/x86: ideapad-laptop: use appropriately typed variable to store the return value of ACPI methods

[Barnabás Pőcze]

Use a variable with type `acpi_status` to store the return value of ACPI
methods instead of a plain `int`. And use ACPI_{SUCCESS,FAILURE} macros
where possible instead of direct comparison.

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

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index c96fd60ec7d2..fbaf8d3f4792 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -977,6 +977,7 @@ static int ideapad_acpi_add(struct platform_device *pdev)
 	int cfg;
 	struct ideapad_private *priv;
 	struct acpi_device *adev;
+	acpi_status acpi_err;
 
 	ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
 	if (ret)
@@ -1031,22 +1032,26 @@ static int ideapad_acpi_add(struct platform_device *pdev)
 		if (ret && ret != -ENODEV)
 			goto backlight_failed;
 	}
-	ret = acpi_install_notify_handler(adev->handle,
-		ACPI_DEVICE_NOTIFY, ideapad_acpi_notify, priv);
-	if (ret)
+	acpi_err = acpi_install_notify_handler(adev->handle,
+			ACPI_DEVICE_NOTIFY, ideapad_acpi_notify, priv);
+	if (ACPI_FAILURE(acpi_err)) {
+		ret = -EIO;
 		goto notification_failed;
+	}
 
 #if IS_ENABLED(CONFIG_ACPI_WMI)
 	for (i = 0; i < ARRAY_SIZE(ideapad_wmi_fnesc_events); i++) {
-		ret = wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
-						 ideapad_wmi_notify, priv);
-		if (ret == AE_OK) {
+		acpi_err = wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
+						      ideapad_wmi_notify, priv);
+		if (ACPI_SUCCESS(acpi_err)) {
 			priv->fnesc_guid = ideapad_wmi_fnesc_events[i];
 			break;
 		}
 	}
-	if (ret != AE_OK && ret != AE_NOT_EXIST)
+	if (ACPI_FAILURE(acpi_err) && acpi_err != AE_NOT_EXIST) {
+		ret = -EIO;
 		goto notification_failed_wmi;
+	}
 #endif
 
 	return 0;
-- 
2.30.0

[PATCH v2 03/24] platform/x86: ideapad-laptop: sort includes lexicographically

[Barnabás Pőcze]

Managing includes is easier when they are sorted, so
sort them lexicographically.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index fbaf8d3f4792..d92d96fe4aa1 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -8,22 +8,23 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/init.h>
-#include <linux/types.h>
 #include <linux/acpi.h>
-#include <linux/rfkill.h>
-#include <linux/platform_device.h>
-#include <linux/input.h>
-#include <linux/input/sparse-keymap.h>
 #include <linux/backlight.h>
-#include <linux/fb.h>
 #include <linux/debugfs.h>
-#include <linux/seq_file.h>
-#include <linux/i8042.h>
-#include <linux/dmi.h>
 #include <linux/device.h>
+#include <linux/dmi.h>
+#include <linux/fb.h>
+#include <linux/i8042.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/input/sparse-keymap.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/rfkill.h>
+#include <linux/seq_file.h>
+#include <linux/types.h>
+
 #include <acpi/video.h>
 
 #define IDEAPAD_RFKILL_DEV_NUM	(3)
-- 
2.30.0

[PATCH v2 04/24] platform/x86: ideapad-laptop: use sysfs_emit()

[Barnabás Pőcze]

sysfs_emit() has been introduced to make it less ambiguous which function
is preferred when writing to the output buffer in a device attribute's
show() callback. Convert the ideapad-laptop module to utilize this
new helper function.

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

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index d92d96fe4aa1..61ea70602256 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -23,6 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/rfkill.h>
 #include <linux/seq_file.h>
+#include <linux/sysfs.h>
 #include <linux/types.h>
 
 #include <acpi/video.h>
@@ -346,8 +347,8 @@ static ssize_t show_ideapad_cam(struct device *dev,
 	struct ideapad_private *priv = dev_get_drvdata(dev);
 
 	if (read_ec_data(priv->adev->handle, VPCCMD_R_CAMERA, &result))
-		return sprintf(buf, "-1\n");
-	return sprintf(buf, "%lu\n", result);
+		return sysfs_emit(buf, "-1\n");
+	return sysfs_emit(buf, "%lu\n", result);
 }
 
 static ssize_t store_ideapad_cam(struct device *dev,
@@ -377,8 +378,8 @@ static ssize_t show_ideapad_fan(struct device *dev,
 	struct ideapad_private *priv = dev_get_drvdata(dev);
 
 	if (read_ec_data(priv->adev->handle, VPCCMD_R_FAN, &result))
-		return sprintf(buf, "-1\n");
-	return sprintf(buf, "%lu\n", result);
+		return sysfs_emit(buf, "-1\n");
+	return sysfs_emit(buf, "%lu\n", result);
 }
 
 static ssize_t store_ideapad_fan(struct device *dev,
@@ -410,8 +411,8 @@ static ssize_t touchpad_show(struct device *dev,
 	unsigned long result;
 
 	if (read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &result))
-		return sprintf(buf, "-1\n");
-	return sprintf(buf, "%lu\n", result);
+		return sysfs_emit(buf, "-1\n");
+	return sysfs_emit(buf, "%lu\n", result);
 }
 
 /* Switch to RO for now: It might be revisited in the future */
@@ -443,8 +444,8 @@ static ssize_t conservation_mode_show(struct device *dev,
 	unsigned long result;
 
 	if (method_gbmd(priv->adev->handle, &result))
-		return sprintf(buf, "-1\n");
-	return sprintf(buf, "%u\n", test_bit(BM_CONSERVATION_BIT, &result));
+		return sysfs_emit(buf, "-1\n");
+	return sysfs_emit(buf, "%u\n", test_bit(BM_CONSERVATION_BIT, &result));
 }
 
 static ssize_t conservation_mode_store(struct device *dev,
@@ -479,10 +480,10 @@ static ssize_t fn_lock_show(struct device *dev,
 	int fail = read_method_int(priv->adev->handle, "HALS", &hals);
 
 	if (fail)
-		return sprintf(buf, "-1\n");
+		return sysfs_emit(buf, "-1\n");
 
 	result = hals;
-	return sprintf(buf, "%u\n", test_bit(HA_FNLOCK_BIT, &result));
+	return sysfs_emit(buf, "%u\n", test_bit(HA_FNLOCK_BIT, &result));
 }
 
 static ssize_t fn_lock_store(struct device *dev,
-- 
2.30.0

[PATCH v2 05/24] platform/x86: ideapad-laptop: use for_each_set_bit() helper to simplify event processing

[Barnabás Pőcze]

The current code used the combination of a for loop + test_bit, which can
be simplified using for_each_set_bit(), so utilize that.

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

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 61ea70602256..29e91942c756 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -10,6 +10,7 @@
 
 #include <linux/acpi.h>
 #include <linux/backlight.h>
+#include <linux/bitops.h>
 #include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/dmi.h>
@@ -742,22 +743,20 @@ static void ideapad_check_special_buttons(struct ideapad_private *priv)
 
 	read_ec_data(priv->adev->handle, VPCCMD_R_SPECIAL_BUTTONS, &value);
 
-	for (bit = 0; bit < 16; bit++) {
-		if (test_bit(bit, &value)) {
-			switch (bit) {
-			case 0:	/* Z580 */
-			case 6:	/* Z570 */
-				/* Thermal Management button */
-				ideapad_input_report(priv, 65);
-				break;
-			case 1:
-				/* OneKey Theater button */
-				ideapad_input_report(priv, 64);
-				break;
-			default:
-				pr_info("Unknown special button: %lu\n", bit);
-				break;
-			}
+	for_each_set_bit(bit, &value, 16) {
+		switch (bit) {
+		case 0:	/* Z580 */
+		case 6:	/* Z570 */
+			/* Thermal Management button */
+			ideapad_input_report(priv, 65);
+			break;
+		case 1:
+			/* OneKey Theater button */
+			ideapad_input_report(priv, 64);
+			break;
+		default:
+			pr_info("Unknown special button: %lu\n", bit);
+			break;
 		}
 	}
 }
@@ -891,7 +890,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv)
 static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ideapad_private *priv = data;
-	unsigned long vpc1, vpc2, vpc_bit;
+	unsigned long vpc1, vpc2, bit;
 
 	if (read_ec_data(handle, VPCCMD_R_VPC1, &vpc1))
 		return;
@@ -899,44 +898,42 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
 		return;
 
 	vpc1 = (vpc2 << 8) | vpc1;
-	for (vpc_bit = 0; vpc_bit < 16; vpc_bit++) {
-		if (test_bit(vpc_bit, &vpc1)) {
-			switch (vpc_bit) {
-			case 9:
-				ideapad_sync_rfk_state(priv);
-				break;
-			case 13:
-			case 11:
-			case 8:
-			case 7:
-			case 6:
-				ideapad_input_report(priv, vpc_bit);
-				break;
-			case 5:
-				ideapad_sync_touchpad_state(priv);
-				break;
-			case 4:
-				ideapad_backlight_notify_brightness(priv);
-				break;
-			case 3:
-				ideapad_input_novokey(priv);
-				break;
-			case 2:
-				ideapad_backlight_notify_power(priv);
-				break;
-			case 0:
-				ideapad_check_special_buttons(priv);
-				break;
-			case 1:
-				/* Some IdeaPads report event 1 every ~20
-				 * seconds while on battery power; some
-				 * report this when changing to/from tablet
-				 * mode. Squelch this event.
-				 */
-				break;
-			default:
-				pr_info("Unknown event: %lu\n", vpc_bit);
-			}
+	for_each_set_bit(bit, &vpc1, 16) {
+		switch (bit) {
+		case 9:
+			ideapad_sync_rfk_state(priv);
+			break;
+		case 13:
+		case 11:
+		case 8:
+		case 7:
+		case 6:
+			ideapad_input_report(priv, bit);
+			break;
+		case 5:
+			ideapad_sync_touchpad_state(priv);
+			break;
+		case 4:
+			ideapad_backlight_notify_brightness(priv);
+			break;
+		case 3:
+			ideapad_input_novokey(priv);
+			break;
+		case 2:
+			ideapad_backlight_notify_power(priv);
+			break;
+		case 0:
+			ideapad_check_special_buttons(priv);
+			break;
+		case 1:
+			/* Some IdeaPads report event 1 every ~20
+			 * seconds while on battery power; some
+			 * report this when changing to/from tablet
+			 * mode. Squelch this event.
+			 */
+			break;
+		default:
+			pr_info("Unknown event: %lu\n", bit);
 		}
 	}
 }
-- 
2.30.0

[PATCH v2 06/24] platform/x86: ideapad-laptop: use msecs_to_jiffies() helper instead of hand-crafted formula

[Barnabás Pőcze]

The current code used a hand-crafted formulate to convert milliseconds to
jiffies, replace it with the msecs_to_jiffies() function.

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

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 29e91942c756..174edbfc52dc 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -19,6 +19,7 @@
 #include <linux/init.h>
 #include <linux/input.h>
 #include <linux/input/sparse-keymap.h>
+#include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -190,7 +191,7 @@ static int read_ec_data(acpi_handle handle, int cmd, unsigned long *data)
 	if (method_vpcw(handle, 1, cmd))
 		return -1;
 
-	for (end_jiffies = jiffies+(HZ)*IDEAPAD_EC_TIMEOUT/1000+1;
+	for (end_jiffies = jiffies + msecs_to_jiffies(IDEAPAD_EC_TIMEOUT) + 1;
 	     time_before(jiffies, end_jiffies);) {
 		schedule();
 		if (method_vpcr(handle, 1, &val))
@@ -216,7 +217,7 @@ static int write_ec_cmd(acpi_handle handle, int cmd, unsigned long data)
 	if (method_vpcw(handle, 1, cmd))
 		return -1;
 
-	for (end_jiffies = jiffies+(HZ)*IDEAPAD_EC_TIMEOUT/1000+1;
+	for (end_jiffies = jiffies + msecs_to_jiffies(IDEAPAD_EC_TIMEOUT) + 1;
 	     time_before(jiffies, end_jiffies);) {
 		schedule();
 		if (method_vpcr(handle, 1, &val))
-- 
2.30.0

[PATCH v2 07/24] platform/x86: ideapad-laptop: use dev_{err,warn} or appropriate variant to display log messages

[Barnabás Pőcze]

Having the device name in the log message makes it easier to determine in
the context of which device the message was printed, so utilize the
appropriate variants of dev_{err,warn,...} when printing log messages.

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

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 174edbfc52dc..b0d8e332b48a 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -203,7 +203,7 @@ static int read_ec_data(acpi_handle handle, int cmd, unsigned long *data)
 			return 0;
 		}
 	}
-	pr_err("timeout in %s\n", __func__);
+	acpi_handle_err(handle, "timeout in %s\n", __func__);
 	return -1;
 }
 
@@ -225,7 +225,7 @@ static int write_ec_cmd(acpi_handle handle, int cmd, unsigned long data)
 		if (val == 0)
 			return 0;
 	}
-	pr_err("timeout in %s\n", __func__);
+	acpi_handle_err(handle, "timeout in %s\n", __func__);
 	return -1;
 }
 
@@ -696,13 +696,15 @@ static int ideapad_input_init(struct ideapad_private *priv)
 
 	error = sparse_keymap_setup(inputdev, ideapad_keymap, NULL);
 	if (error) {
-		pr_err("Unable to setup input device keymap\n");
+		dev_err(&priv->platform_device->dev,
+			"Unable to setup input device keymap\n");
 		goto err_free_dev;
 	}
 
 	error = input_register_device(inputdev);
 	if (error) {
-		pr_err("Unable to register input device\n");
+		dev_err(&priv->platform_device->dev,
+			"Unable to register input device\n");
 		goto err_free_dev;
 	}
 
@@ -756,7 +758,8 @@ static void ideapad_check_special_buttons(struct ideapad_private *priv)
 			ideapad_input_report(priv, 64);
 			break;
 		default:
-			pr_info("Unknown special button: %lu\n", bit);
+			dev_warn(&priv->platform_device->dev,
+				 "Unknown special button: %lu\n", bit);
 			break;
 		}
 	}
@@ -822,7 +825,8 @@ static int ideapad_backlight_init(struct ideapad_private *priv)
 					      &ideapad_backlight_ops,
 					      &props);
 	if (IS_ERR(blightdev)) {
-		pr_err("Could not register backlight device\n");
+		dev_warn(&priv->platform_device->dev,
+			 "Could not register backlight device\n");
 		return PTR_ERR(blightdev);
 	}
 
@@ -934,7 +938,8 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
 			 */
 			break;
 		default:
-			pr_info("Unknown event: %lu\n", bit);
+			dev_warn(&priv->platform_device->dev,
+				 "Unknown event: %lu\n", bit);
 		}
 	}
 }
@@ -942,12 +947,15 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
 #if IS_ENABLED(CONFIG_ACPI_WMI)
 static void ideapad_wmi_notify(u32 value, void *context)
 {
+	struct ideapad_private *priv = context;
+
 	switch (value) {
 	case 128:
-		ideapad_input_report(context, value);
+		ideapad_input_report(priv, value);
 		break;
 	default:
-		pr_info("Unknown WMI event %u\n", value);
+		dev_warn(&priv->platform_device->dev,
+			 "Unknown WMI event: %u\n", value);
 	}
 }
 #endif
-- 
2.30.0

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

[Barnabás Pőcze]

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>

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;
-- 
2.30.0

[PATCH v2 09/24] platform/x86: ideapad-laptop: always propagate error codes from device attributes' show() callback

[Barnabás Pőcze]

Consumers can differentiate an error from a successful read much more
easily if the read() call fails with the appropriate errno instead of
returning a magic string like "-1".

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

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 9bc6c7340876..677d4e10352e 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -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;
 
-	if (read_ec_data(priv->adev->handle, VPCCMD_R_CAMERA, &result))
-		return sysfs_emit(buf, "-1\n");
+	err = read_ec_data(priv->adev->handle, VPCCMD_R_CAMERA, &result);
+	if (err)
+		return err;
 	return sysfs_emit(buf, "%lu\n", result);
 }
 
@@ -384,9 +386,11 @@ static ssize_t show_ideapad_fan(struct device *dev,
 {
 	unsigned long result;
 	struct ideapad_private *priv = dev_get_drvdata(dev);
+	int err;
 
-	if (read_ec_data(priv->adev->handle, VPCCMD_R_FAN, &result))
-		return sysfs_emit(buf, "-1\n");
+	err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, &result);
+	if (err)
+		return err;
 	return sysfs_emit(buf, "%lu\n", result);
 }
 
@@ -417,9 +421,11 @@ static ssize_t touchpad_show(struct device *dev,
 {
 	struct ideapad_private *priv = dev_get_drvdata(dev);
 	unsigned long result;
+	int err;
 
-	if (read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &result))
-		return sysfs_emit(buf, "-1\n");
+	err = read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &result);
+	if (err)
+		return err;
 	return sysfs_emit(buf, "%lu\n", result);
 }
 
@@ -450,9 +456,11 @@ static ssize_t conservation_mode_show(struct device *dev,
 {
 	struct ideapad_private *priv = dev_get_drvdata(dev);
 	unsigned long result;
+	int err;
 
-	if (method_gbmd(priv->adev->handle, &result))
-		return sysfs_emit(buf, "-1\n");
+	err = method_gbmd(priv->adev->handle, &result);
+	if (err)
+		return err;
 	return sysfs_emit(buf, "%u\n", test_bit(BM_CONSERVATION_BIT, &result));
 }
 
@@ -488,7 +496,7 @@ static ssize_t fn_lock_show(struct device *dev,
 	int fail = read_method_int(priv->adev->handle, "HALS", &hals);
 
 	if (fail)
-		return sysfs_emit(buf, "-1\n");
+		return fail;
 
 	result = hals;
 	return sysfs_emit(buf, "%u\n", test_bit(HA_FNLOCK_BIT, &result));
-- 
2.30.0

[PATCH v2 10/24] platform/x86: ideapad-laptop: misc. device attribute changes

[Barnabás Pőcze]

Do not handle zero length buffer separately. Use kstrtouint() instead
of sscanf().

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

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 677d4e10352e..cba3d9419f52 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -365,13 +365,13 @@ static ssize_t store_ideapad_cam(struct device *dev,
 				 struct device_attribute *attr,
 				 const char *buf, size_t count)
 {
-	int ret, state;
+	int ret;
 	struct ideapad_private *priv = dev_get_drvdata(dev);
+	unsigned int state;
 
-	if (!count)
-		return 0;
-	if (sscanf(buf, "%i", &state) != 1)
-		return -EINVAL;
+	ret = kstrtouint(buf, 0, &state);
+	if (ret)
+		return ret;
 	ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_CAMERA, state);
 	if (ret)
 		return ret;
@@ -398,14 +398,14 @@ static ssize_t store_ideapad_fan(struct device *dev,
 				 struct device_attribute *attr,
 				 const char *buf, size_t count)
 {
-	int ret, state;
+	int ret;
 	struct ideapad_private *priv = dev_get_drvdata(dev);
+	unsigned int state;
 
-	if (!count)
-		return 0;
-	if (sscanf(buf, "%i", &state) != 1)
-		return -EINVAL;
-	if (state < 0 || state > 4 || state == 3)
+	ret = kstrtouint(buf, 0, &state);
+	if (ret)
+		return ret;
+	if (state > 4 || state == 3)
 		return -EINVAL;
 	ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_FAN, state);
 	if (ret)
-- 
2.30.0

[PATCH v2 11/24] platform/x86: ideapad-laptop: group and separate (un)related constants into enums

[Barnabás Pőcze]

Group and rename constants depending on which ACPI interface they
pertain to, and rename CFG_X constants to CFG_CAP_X.

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

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index cba3d9419f52..5f36e77d9058 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -32,14 +32,6 @@
 
 #define IDEAPAD_RFKILL_DEV_NUM	(3)
 
-#define BM_CONSERVATION_BIT (5)
-#define HA_FNLOCK_BIT       (10)
-
-#define CFG_BT_BIT	(16)
-#define CFG_3G_BIT	(17)
-#define CFG_WIFI_BIT	(18)
-#define CFG_CAMERA_BIT	(19)
-
 #if IS_ENABLED(CONFIG_ACPI_WMI)
 static const char *const ideapad_wmi_fnesc_events[] = {
 	"26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", /* Yoga 3 */
@@ -48,10 +40,28 @@ static const char *const ideapad_wmi_fnesc_events[] = {
 #endif
 
 enum {
-	BMCMD_CONSERVATION_ON = 3,
-	BMCMD_CONSERVATION_OFF = 5,
-	HACMD_FNLOCK_ON = 0xe,
-	HACMD_FNLOCK_OFF = 0xf,
+	CFG_CAP_BT_BIT   = 16,
+	CFG_CAP_3G_BIT   = 17,
+	CFG_CAP_WIFI_BIT = 18,
+	CFG_CAP_CAM_BIT  = 19,
+};
+
+enum {
+	GBMD_CONSERVATION_STATE_BIT = 5,
+};
+
+enum {
+	SMBC_CONSERVATION_ON  = 3,
+	SMBC_CONSERVATION_OFF = 5,
+};
+
+enum {
+	HALS_FNLOCK_STATE_BIT = 10,
+};
+
+enum {
+	SALS_FNLOCK_ON  = 0xe,
+	SALS_FNLOCK_OFF = 0xf,
 };
 
 enum {
@@ -278,7 +288,7 @@ static int debugfs_status_show(struct seq_file *s, void *data)
 
 	if (!method_gbmd(priv->adev->handle, &value)) {
 		seq_printf(s, "Conservation mode:\t%s(%lu)\n",
-			   test_bit(BM_CONSERVATION_BIT, &value) ? "On" : "Off",
+			   test_bit(GBMD_CONSERVATION_STATE_BIT, &value) ? "On" : "Off",
 			   value);
 	}
 
@@ -295,13 +305,13 @@ static int debugfs_cfg_show(struct seq_file *s, void *data)
 	} else {
 		seq_printf(s, "cfg: 0x%.8lX\n\nCapability: ",
 			   priv->cfg);
-		if (test_bit(CFG_BT_BIT, &priv->cfg))
+		if (test_bit(CFG_CAP_BT_BIT, &priv->cfg))
 			seq_printf(s, "Bluetooth ");
-		if (test_bit(CFG_3G_BIT, &priv->cfg))
+		if (test_bit(CFG_CAP_3G_BIT, &priv->cfg))
 			seq_printf(s, "3G ");
-		if (test_bit(CFG_WIFI_BIT, &priv->cfg))
+		if (test_bit(CFG_CAP_WIFI_BIT, &priv->cfg))
 			seq_printf(s, "Wireless ");
-		if (test_bit(CFG_CAMERA_BIT, &priv->cfg))
+		if (test_bit(CFG_CAP_CAM_BIT, &priv->cfg))
 			seq_printf(s, "Camera ");
 		seq_printf(s, "\nGraphic: ");
 		switch ((priv->cfg)&0x700) {
@@ -461,7 +471,7 @@ static ssize_t conservation_mode_show(struct device *dev,
 	err = method_gbmd(priv->adev->handle, &result);
 	if (err)
 		return err;
-	return sysfs_emit(buf, "%u\n", test_bit(BM_CONSERVATION_BIT, &result));
+	return sysfs_emit(buf, "%u\n", test_bit(GBMD_CONSERVATION_STATE_BIT, &result));
 }
 
 static ssize_t conservation_mode_store(struct device *dev,
@@ -477,8 +487,8 @@ static ssize_t conservation_mode_store(struct device *dev,
 		return ret;
 
 	ret = method_int1(priv->adev->handle, "SBMC", state ?
-					      BMCMD_CONSERVATION_ON :
-					      BMCMD_CONSERVATION_OFF);
+					      SMBC_CONSERVATION_ON :
+					      SMBC_CONSERVATION_OFF);
 	if (ret)
 		return ret;
 	return count;
@@ -499,7 +509,7 @@ static ssize_t fn_lock_show(struct device *dev,
 		return fail;
 
 	result = hals;
-	return sysfs_emit(buf, "%u\n", test_bit(HA_FNLOCK_BIT, &result));
+	return sysfs_emit(buf, "%u\n", test_bit(HALS_FNLOCK_STATE_BIT, &result));
 }
 
 static ssize_t fn_lock_store(struct device *dev,
@@ -515,8 +525,8 @@ static ssize_t fn_lock_store(struct device *dev,
 		return ret;
 
 	ret = method_int1(priv->adev->handle, "SALS", state ?
-			  HACMD_FNLOCK_ON :
-			  HACMD_FNLOCK_OFF);
+			  SALS_FNLOCK_ON :
+			  SALS_FNLOCK_OFF);
 	if (ret)
 		return ret;
 	return count;
@@ -543,7 +553,7 @@ static umode_t ideapad_is_visible(struct kobject *kobj,
 	bool supported;
 
 	if (attr == &dev_attr_camera_power.attr)
-		supported = test_bit(CFG_CAMERA_BIT, &(priv->cfg));
+		supported = test_bit(CFG_CAP_CAM_BIT, &priv->cfg);
 	else if (attr == &dev_attr_fan_mode.attr) {
 		unsigned long value;
 		supported = !read_ec_data(priv->adev->handle, VPCCMD_R_FAN,
@@ -578,9 +588,9 @@ struct ideapad_rfk_data {
 };
 
 static const struct ideapad_rfk_data ideapad_rfk_data[] = {
-	{ "ideapad_wlan",    CFG_WIFI_BIT, VPCCMD_W_WIFI, RFKILL_TYPE_WLAN },
-	{ "ideapad_bluetooth", CFG_BT_BIT, VPCCMD_W_BT, RFKILL_TYPE_BLUETOOTH },
-	{ "ideapad_3g",        CFG_3G_BIT, VPCCMD_W_3G, RFKILL_TYPE_WWAN },
+	{ "ideapad_wlan",      CFG_CAP_WIFI_BIT, VPCCMD_W_WIFI, RFKILL_TYPE_WLAN },
+	{ "ideapad_bluetooth", CFG_CAP_BT_BIT,   VPCCMD_W_BT,   RFKILL_TYPE_BLUETOOTH },
+	{ "ideapad_3g",        CFG_CAP_3G_BIT,   VPCCMD_W_3G,   RFKILL_TYPE_WWAN },
 };
 
 static int ideapad_rfk_set(void *data, bool blocked)
-- 
2.30.0

[PATCH v2 12/24] platform/x86: ideapad-laptop: rework and create new ACPI helpers

[Barnabás Pőcze]

Create dedicated helper functions for accessing the main ACPI methods:
GBMD, SMBC, HALS, SALS; and utilize them. Use `unsigned long` consistently
in every ACPI helper wherever possible. Change names to better express
purpose. Do not assign values to output parameters in case of failure.

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

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 5f36e77d9058..c3234e0931a9 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -119,41 +119,47 @@ MODULE_PARM_DESC(no_bt_rfkill, "No rfkill for bluetooth.");
  */
 #define IDEAPAD_EC_TIMEOUT (200) /* in ms */
 
-static int read_method_int(acpi_handle handle, const char *method, int *val)
+static int eval_int(acpi_handle handle, const char *method, unsigned long *val)
 {
-	acpi_status status;
+	acpi_status acpi_err;
 	unsigned long long result;
 
-	status = acpi_evaluate_integer(handle, (char *)method, NULL, &result);
-	if (ACPI_FAILURE(status)) {
-		*val = -1;
+	acpi_err = acpi_evaluate_integer(handle, (char *)method, NULL, &result);
+	if (ACPI_FAILURE(acpi_err))
 		return -EIO;
-	}
 	*val = result;
 	return 0;
+}
 
+static int eval_simple_method(acpi_handle handle, char *method, u64 arg)
+{
+	acpi_status acpi_err = acpi_execute_simple_method(handle, method, arg);
+	return ACPI_FAILURE(acpi_err) ? -EIO : 0;
 }
 
-static int method_gbmd(acpi_handle handle, unsigned long *ret)
+static int eval_gbmd(acpi_handle handle, unsigned long *val)
 {
-	int result, val;
+	return eval_int(handle, "GBMD", val);
+}
 
-	result = read_method_int(handle, "GBMD", &val);
-	*ret = val;
-	return result;
+static int eval_smbc(acpi_handle handle, unsigned long arg)
+{
+	return eval_simple_method(handle, "SMBC", arg);
 }
 
-static int method_int1(acpi_handle handle, char *method, int cmd)
+static int eval_hals(acpi_handle handle, unsigned long *val)
 {
-	acpi_status status;
+	return eval_int(handle, "HALS", val);
+}
 
-	status = acpi_execute_simple_method(handle, method, cmd);
-	return ACPI_FAILURE(status) ? -EIO : 0;
+static int eval_sals(acpi_handle handle, unsigned long arg)
+{
+	return eval_simple_method(handle, "SALS", arg);
 }
 
-static int method_vpcr(acpi_handle handle, int cmd, int *ret)
+static int eval_vpcr(acpi_handle handle, unsigned long cmd, unsigned long *val)
 {
-	acpi_status status;
+	acpi_status acpi_err;
 	unsigned long long result;
 	struct acpi_object_list params;
 	union acpi_object in_obj;
@@ -163,22 +169,20 @@ static int method_vpcr(acpi_handle handle, int cmd, int *ret)
 	in_obj.type = ACPI_TYPE_INTEGER;
 	in_obj.integer.value = cmd;
 
-	status = acpi_evaluate_integer(handle, "VPCR", &params, &result);
+	acpi_err = acpi_evaluate_integer(handle, "VPCR", &params, &result);
 
-	if (ACPI_FAILURE(status)) {
-		*ret = -1;
+	if (ACPI_FAILURE(acpi_err))
 		return -EIO;
-	}
-	*ret = result;
+	*val = result;
 	return 0;
 
 }
 
-static int method_vpcw(acpi_handle handle, int cmd, int data)
+static int eval_vpcw(acpi_handle handle, unsigned long cmd, unsigned long data)
 {
 	struct acpi_object_list params;
 	union acpi_object in_obj[2];
-	acpi_status status;
+	acpi_status acpi_err;
 
 	params.count = 2;
 	params.pointer = in_obj;
@@ -187,55 +191,50 @@ static int method_vpcw(acpi_handle handle, int cmd, int data)
 	in_obj[1].type = ACPI_TYPE_INTEGER;
 	in_obj[1].integer.value = data;
 
-	status = acpi_evaluate_object(handle, "VPCW", &params, NULL);
-	if (status != AE_OK)
+	acpi_err = acpi_evaluate_object(handle, "VPCW", &params, NULL);
+	if (ACPI_FAILURE(acpi_err))
 		return -EIO;
 	return 0;
 }
 
-static int read_ec_data(acpi_handle handle, int cmd, unsigned long *data)
+static int read_ec_data(acpi_handle handle, unsigned long cmd, unsigned long *data)
 {
-	int val, err;
-	unsigned long int end_jiffies;
+	int err;
+	unsigned long int end_jiffies, val;
 
-	err = method_vpcw(handle, 1, cmd);
+	err = eval_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();
-		err = method_vpcr(handle, 1, &val);
+		err = eval_vpcr(handle, 1, &val);
 		if (err)
 			return err;
-		if (val == 0) {
-			err = method_vpcr(handle, 0, &val);
-			if (err)
-				return err;
-			*data = val;
-			return 0;
-		}
+		if (val == 0)
+			return eval_vpcr(handle, 0, data);
 	}
 	acpi_handle_err(handle, "timeout in %s\n", __func__);
 	return -ETIMEDOUT;
 }
 
-static int write_ec_cmd(acpi_handle handle, int cmd, unsigned long data)
+static int write_ec_cmd(acpi_handle handle, unsigned long cmd, unsigned long data)
 {
-	int val, err;
-	unsigned long int end_jiffies;
+	int err;
+	unsigned long end_jiffies, val;
 
-	err = method_vpcw(handle, 0, data);
+	err = eval_vpcw(handle, 0, data);
 	if (err)
 		return err;
-	err = method_vpcw(handle, 1, cmd);
+	err = eval_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();
-		err = method_vpcr(handle, 1, &val);
+		err = eval_vpcr(handle, 1, &val);
 		if (err)
 			return err;
 		if (val == 0)
@@ -286,7 +285,7 @@ static int debugfs_status_show(struct seq_file *s, void *data)
 			   value ? "On" : "Off", value);
 	seq_puts(s, "=====================\n");
 
-	if (!method_gbmd(priv->adev->handle, &value)) {
+	if (!eval_gbmd(priv->adev->handle, &value)) {
 		seq_printf(s, "Conservation mode:\t%s(%lu)\n",
 			   test_bit(GBMD_CONSERVATION_STATE_BIT, &value) ? "On" : "Off",
 			   value);
@@ -468,7 +467,7 @@ static ssize_t conservation_mode_show(struct device *dev,
 	unsigned long result;
 	int err;
 
-	err = method_gbmd(priv->adev->handle, &result);
+	err = eval_gbmd(priv->adev->handle, &result);
 	if (err)
 		return err;
 	return sysfs_emit(buf, "%u\n", test_bit(GBMD_CONSERVATION_STATE_BIT, &result));
@@ -486,9 +485,8 @@ static ssize_t conservation_mode_store(struct device *dev,
 	if (ret)
 		return ret;
 
-	ret = method_int1(priv->adev->handle, "SBMC", state ?
-					      SMBC_CONSERVATION_ON :
-					      SMBC_CONSERVATION_OFF);
+	ret = eval_smbc(priv->adev->handle,
+			state ? SMBC_CONSERVATION_ON : SMBC_CONSERVATION_OFF);
 	if (ret)
 		return ret;
 	return count;
@@ -501,15 +499,13 @@ static ssize_t fn_lock_show(struct device *dev,
 			    char *buf)
 {
 	struct ideapad_private *priv = dev_get_drvdata(dev);
-	unsigned long result;
-	int hals;
-	int fail = read_method_int(priv->adev->handle, "HALS", &hals);
+	unsigned long hals;
+	int fail = eval_hals(priv->adev->handle, &hals);
 
 	if (fail)
 		return fail;
 
-	result = hals;
-	return sysfs_emit(buf, "%u\n", test_bit(HALS_FNLOCK_STATE_BIT, &result));
+	return sysfs_emit(buf, "%u\n", test_bit(HALS_FNLOCK_STATE_BIT, &hals));
 }
 
 static ssize_t fn_lock_store(struct device *dev,
@@ -524,9 +520,8 @@ static ssize_t fn_lock_store(struct device *dev,
 	if (ret)
 		return ret;
 
-	ret = method_int1(priv->adev->handle, "SALS", state ?
-			  SALS_FNLOCK_ON :
-			  SALS_FNLOCK_OFF);
+	ret = eval_sals(priv->adev->handle,
+			state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
 	if (ret)
 		return ret;
 	return count;
@@ -1016,7 +1011,7 @@ static const struct dmi_system_id hw_rfkill_list[] = {
 static int ideapad_acpi_add(struct platform_device *pdev)
 {
 	int ret, i;
-	int cfg;
+	unsigned long cfg;
 	struct ideapad_private *priv;
 	struct acpi_device *adev;
 	acpi_status acpi_err;
@@ -1025,7 +1020,7 @@ static int ideapad_acpi_add(struct platform_device *pdev)
 	if (ret)
 		return -ENODEV;
 
-	if (read_method_int(adev->handle, "_CFG", &cfg))
+	if (eval_int(adev->handle, "_CFG", &cfg))
 		return -ENODEV;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
-- 
2.30.0

Re: [PATCH v2 07/24] platform/x86: ideapad-laptop: use dev_{err,warn} or appropriate variant to display log messages

[Andy Shevchenko]

On Wed, Jan 13, 2021 at 8:22 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Having the device name in the log message makes it easier to determine in
> the context of which device the message was printed, so utilize the
> appropriate variants of dev_{err,warn,...} when printing log messages.

This doesn't explain transitions like pr_err() -> dev_warn() or
pr_info() -> dev_warn().
Care to elaborate in the commit message?

> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 174edbfc52dc..b0d8e332b48a 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -203,7 +203,7 @@ static int read_ec_data(acpi_handle handle, int cmd, unsigned long *data)
>                         return 0;
>                 }
>         }
> -       pr_err("timeout in %s\n", __func__);
> +       acpi_handle_err(handle, "timeout in %s\n", __func__);
>         return -1;
>  }
>
> @@ -225,7 +225,7 @@ static int write_ec_cmd(acpi_handle handle, int cmd, unsigned long data)
>                 if (val == 0)
>                         return 0;
>         }
> -       pr_err("timeout in %s\n", __func__);
> +       acpi_handle_err(handle, "timeout in %s\n", __func__);
>         return -1;
>  }
>
> @@ -696,13 +696,15 @@ static int ideapad_input_init(struct ideapad_private *priv)
>
>         error = sparse_keymap_setup(inputdev, ideapad_keymap, NULL);
>         if (error) {
> -               pr_err("Unable to setup input device keymap\n");
> +               dev_err(&priv->platform_device->dev,
> +                       "Unable to setup input device keymap\n");
>                 goto err_free_dev;
>         }
>
>         error = input_register_device(inputdev);
>         if (error) {
> -               pr_err("Unable to register input device\n");
> +               dev_err(&priv->platform_device->dev,
> +                       "Unable to register input device\n");
>                 goto err_free_dev;
>         }
>
> @@ -756,7 +758,8 @@ static void ideapad_check_special_buttons(struct ideapad_private *priv)
>                         ideapad_input_report(priv, 64);
>                         break;
>                 default:
> -                       pr_info("Unknown special button: %lu\n", bit);
> +                       dev_warn(&priv->platform_device->dev,
> +                                "Unknown special button: %lu\n", bit);
>                         break;
>                 }
>         }
> @@ -822,7 +825,8 @@ static int ideapad_backlight_init(struct ideapad_private *priv)
>                                               &ideapad_backlight_ops,
>                                               &props);
>         if (IS_ERR(blightdev)) {
> -               pr_err("Could not register backlight device\n");
> +               dev_warn(&priv->platform_device->dev,
> +                        "Could not register backlight device\n");
>                 return PTR_ERR(blightdev);
>         }
>
> @@ -934,7 +938,8 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>                          */
>                         break;
>                 default:
> -                       pr_info("Unknown event: %lu\n", bit);
> +                       dev_warn(&priv->platform_device->dev,
> +                                "Unknown event: %lu\n", bit);
>                 }
>         }
>  }
> @@ -942,12 +947,15 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>  #if IS_ENABLED(CONFIG_ACPI_WMI)
>  static void ideapad_wmi_notify(u32 value, void *context)
>  {
> +       struct ideapad_private *priv = context;
> +
>         switch (value) {
>         case 128:
> -               ideapad_input_report(context, value);
> +               ideapad_input_report(priv, value);
>                 break;
>         default:
> -               pr_info("Unknown WMI event %u\n", value);
> +               dev_warn(&priv->platform_device->dev,
> +                        "Unknown WMI event: %u\n", value);
>         }
>  }
>  #endif
> --
> 2.30.0
>


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/24] platform/x86: ideapad-laptop: always propagate error codes from device attributes' show() callback

[Andy Shevchenko]

On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Consumers can differentiate an error from a successful read much more
> easily if the read() call fails with the appropriate errno instead of
> returning a magic string like "-1".

Is user space ready for this (for the record, it seems an ABI breakage)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 10/24] platform/x86: ideapad-laptop: misc. device attribute changes

[Andy Shevchenko]

On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Do not handle zero length buffer separately. Use kstrtouint() instead
> of sscanf().

...

> -       int ret, state;
> +       int ret;
>         struct ideapad_private *priv = dev_get_drvdata(dev);
> +       unsigned int state;

Reversed xmas tree order?

...

> -       if (sscanf(buf, "%i", &state) != 1)
> -               return -EINVAL;
> +       ret = kstrtouint(buf, 0, &state);
> +       if (ret)
> +               return ret;

This seems to me a relaxing case, and should be 10 instead of 0. Am I
right about %i?
If it's true it's probably minor, but still an ABI breakage.

P.S. Same comments for the similar cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 00/24] platform/x86: ideapad-laptop: cleanup, keyboard backlight and "always on USB charging" control support, reenable touchpad control

[Andy Shevchenko]

On Wed, Jan 13, 2021 at 8:22 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> This series contains patches that aim to bring more consistency
> to the code; add keyboard backlight control support; add
> "always on USB charging" control support.
> Furthermore, commit 7f363145992cebf4ea760447f1cfdf6f81459683
> ("platform/x86: ideapad-laptop: Switch touchpad attribute to be RO")
> is reverted since it made it impossible to disable/enable the touchpad via the
> ideapad-laptop module and on some devices the method implemented in the
> module works correctly to disable/enable the touchpad.

I like this clean up series, but it requires some work to be done.
So, for *non*-commented patches, feel free to add
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

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

[Barnabás Pőcze]

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

Re: [PATCH v2 07/24] platform/x86: ideapad-laptop: use dev_{err,warn} or appropriate variant to display log messages

[Barnabás Pőcze]

Hi


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

> On Wed, Jan 13, 2021 at 8:22 PM Barnabás Pőcze wrote:
> >
> > Having the device name in the log message makes it easier to determine in
> > the context of which device the message was printed, so utilize the
> > appropriate variants of dev_{err,warn,...} when printing log messages.
>
> This doesn't explain transitions like pr_err() -> dev_warn() or
> pr_info() -> dev_warn().
> Care to elaborate in the commit message?
> [...]

Thanks for the review, and thanks for pointing this out. I don't recall intendeding
to promote/demote any of the log messages when I was making these changes. I will
revisit my them and modify the commit and commit message accordingly.


Thanks,
Barnabás Pőcze

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

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

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

[Barnabás Pőcze]

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

Re: [PATCH v2 10/24] platform/x86: ideapad-laptop: misc. device attribute changes

[Barnabás Pőcze]

Hi


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

> On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze wrote:
> >
> > Do not handle zero length buffer separately. Use kstrtouint() instead
> > of sscanf().
>
> ...
>
> > -       int ret, state;
> > +       int ret;
> >         struct ideapad_private *priv = dev_get_drvdata(dev);
> > +       unsigned int state;
>
> Reversed xmas tree order?
>

I'll change the order.


> ...
>
> > -       if (sscanf(buf, "%i", &state) != 1)
> > -               return -EINVAL;
> > +       ret = kstrtouint(buf, 0, &state);
> > +       if (ret)
> > +               return ret;
>
> This seems to me a relaxing case, and should be 10 instead of 0. Am I
> right about %i?
> If it's true it's probably minor, but still an ABI breakage.
>

According to the latest C99 draft[1] (7.19.6.2):


  [The 'i' format specifier] Matches an optionally signed integer, whose format
  is the same as expected for the subject sequence of the strtol function with
  the value 0 for the base argument. The corresponding argument shall be a pointer
  to signed integer.

Skimming over `vsscanf()`, I'm fairly sure it implements the same behaviour.
So '0' as the base is correct, I believe. But technically it's still an ABI
breakage since negative numbers are no longer accepted. In the case of
`store_ideapad_fan()` it changes nothing since there was bounds checking in place.
In the case of `store_ideapad_cam()` negative numbers are now rejected. I'm not
sure if this change should be of great concern, since the the documentation only
mentions '0' and '1', and I would be surprised if anyone used this interface
to send negative numbers to the embedded controller.



[1]: https://wg14.link/c99


Regards,
Barnabás Pőcze

Re: [PATCH v2 10/24] platform/x86: ideapad-laptop: misc. device attribute changes

[Andy Shevchenko]

On Sat, Jan 16, 2021 at 11:54 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> 2021. január 16., szombat 20:52 keltezéssel, Andy Shevchenko írta:
> > On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze wrote:

...

> > > -       if (sscanf(buf, "%i", &state) != 1)
> > > -               return -EINVAL;
> > > +       ret = kstrtouint(buf, 0, &state);
> > > +       if (ret)
> > > +               return ret;
> >
> > This seems to me a relaxing case, and should be 10 instead of 0. Am I
> > right about %i?
> > If it's true it's probably minor, but still an ABI breakage.
>
> According to the latest C99 draft[1] (7.19.6.2):
>
>   [The 'i' format specifier] Matches an optionally signed integer, whose format
>   is the same as expected for the subject sequence of the strtol function with
>   the value 0 for the base argument. The corresponding argument shall be a pointer
>   to signed integer.

> Skimming over `vsscanf()`, I'm fairly sure it implements the same behaviour.
> So '0' as the base is correct, I believe.

Ah, okay, good to know. I assumed that %i is decimal only.
Thanks!

>  But technically it's still an ABI
> breakage since negative numbers are no longer accepted. In the case of
> `store_ideapad_fan()` it changes nothing since there was bounds checking in place.
> In the case of `store_ideapad_cam()` negative numbers are now rejected. I'm not
> sure if this change should be of great concern, since the the documentation only
> mentions '0' and '1', and I would be surprised if anyone used this interface
> to send negative numbers to the embedded controller.

If it's only 0 / 1 perhaps you may go further and make it bool
(kstrtobool() API)?

> [1]: https://wg14.link/c99



-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 07/24] platform/x86: ideapad-laptop: use dev_{err,warn} or appropriate variant to display log messages

[Ike Panhc]

On 1/17/21 4:34 AM, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2021. január 16., szombat 20:46 keltezéssel, Andy Shevchenko írta:
> 
>> On Wed, Jan 13, 2021 at 8:22 PM Barnabás Pőcze wrote:
>>>
>>> Having the device name in the log message makes it easier to determine in
>>> the context of which device the message was printed, so utilize the
>>> appropriate variants of dev_{err,warn,...} when printing log messages.
>>
>> This doesn't explain transitions like pr_err() -> dev_warn() or
>> pr_info() -> dev_warn().
>> Care to elaborate in the commit message?
>> [...]
> 
> Thanks for the review, and thanks for pointing this out. I don't recall intendeding
> to promote/demote any of the log messages when I was making these changes. I will
> revisit my them and modify the commit and commit message accordingly.
> 
> 
> Thanks,
> Barnabás Pőcze
> 

Hi,

For unknown key/wmi event, it might come from latest model on new function.
That's why it is pr_info.

For backlight_init, this should not happen. that's why it is pr_err.

I am ok if you have any though of promoting or demoting.
--
Ike Panhc

Re: [PATCH v2 09/24] platform/x86: ideapad-laptop: always propagate error codes from device attributes' show() callback

[Ike Panhc]

On 1/17/21 3:49 AM, Andy Shevchenko wrote:
> On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>>
>> Consumers can differentiate an error from a successful read much more
>> easily if the read() call fails with the appropriate errno instead of
>> returning a magic string like "-1".
> 
> Is user space ready for this (for the record, it seems an ABI breakage)?
> 

read() and getting errno looks sysfs/driver broken to me. I think
if button/method is not available, it's better to be something like
sysfs_emit(buf, "%d\n", -ENODEV)

--
Ike Panhc

Re: [PATCH v2 09/24] platform/x86: ideapad-laptop: always propagate error codes from device attributes' show() callback

[Andy Shevchenko]

On Mon, Jan 25, 2021 at 9:37 AM Ike Panhc <ike.pan@canonical.com> wrote:
>
> On 1/17/21 3:49 AM, Andy Shevchenko wrote:
> > On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> >>
> >> Consumers can differentiate an error from a successful read much more
> >> easily if the read() call fails with the appropriate errno instead of
> >> returning a magic string like "-1".
> >
> > Is user space ready for this (for the record, it seems an ABI breakage)?
> >
>
> read() and getting errno looks sysfs/driver broken to me. I think
> if button/method is not available, it's better to be something like
> sysfs_emit(buf, "%d\n", -ENODEV)

Either way it will be an ABI breakage.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/24] platform/x86: ideapad-laptop: always propagate error codes from device attributes' show() callback

[Hans de Goede]

Hi,

On 1/25/21 11:57 AM, Andy Shevchenko wrote:
> On Mon, Jan 25, 2021 at 9:37 AM Ike Panhc <ike.pan@canonical.com> wrote:
>>
>> On 1/17/21 3:49 AM, Andy Shevchenko wrote:
>>> On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>>>>
>>>> Consumers can differentiate an error from a successful read much more
>>>> easily if the read() call fails with the appropriate errno instead of
>>>> returning a magic string like "-1".
>>>
>>> Is user space ready for this (for the record, it seems an ABI breakage)?
>>>
>>
>> read() and getting errno looks sysfs/driver broken to me. I think
>> if button/method is not available, it's better to be something like
>> sysfs_emit(buf, "%d\n", -ENODEV)
> 
> Either way it will be an ABI breakage.

True any change here will be an ABI breakage, but I really do not expect
anything to be dependent on this weird behavior of returning errors by
writing some magic value to the buffer, rather then just error-ing out
of the read() call.

The kernel-convention here clearly is to make the read() syscall fail with
-ESOMETHING on errors. So I see this as making the driver conform to the
expected sysfs API behavior. Since this change is nicely split out into a
separate patch, we can always revert it if it turns out there actually
is something depending on this. But again I see that as highly
unlikely.

Regards,

Hans

Re: [PATCH v2 09/24] platform/x86: ideapad-laptop: always propagate error codes from device attributes' show() callback

[Andy Shevchenko]

On Mon, Jan 25, 2021 at 1:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 1/25/21 11:57 AM, Andy Shevchenko wrote:
> > On Mon, Jan 25, 2021 at 9:37 AM Ike Panhc <ike.pan@canonical.com> wrote:
> >>
> >> On 1/17/21 3:49 AM, Andy Shevchenko wrote:
> >>> On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> >>>>
> >>>> Consumers can differentiate an error from a successful read much more
> >>>> easily if the read() call fails with the appropriate errno instead of
> >>>> returning a magic string like "-1".
> >>>
> >>> Is user space ready for this (for the record, it seems an ABI breakage)?
> >>>
> >>
> >> read() and getting errno looks sysfs/driver broken to me. I think
> >> if button/method is not available, it's better to be something like
> >> sysfs_emit(buf, "%d\n", -ENODEV)
> >
> > Either way it will be an ABI breakage.
>
> True any change here will be an ABI breakage, but I really do not expect
> anything to be dependent on this weird behavior of returning errors by
> writing some magic value to the buffer, rather then just error-ing out
> of the read() call.
>
> The kernel-convention here clearly is to make the read() syscall fail with
> -ESOMETHING on errors. So I see this as making the driver conform to the
> expected sysfs API behavior. Since this change is nicely split out into a
> separate patch, we can always revert it if it turns out there actually
> is something depending on this. But again I see that as highly
> unlikely.

Me too. My point is that every stakeholder here understands that.
Perhaps elaborated in the commit message.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/24] platform/x86: ideapad-laptop: always propagate error codes from device attributes' show() callback

[Hans de Goede]

Hi,

On 1/25/21 12:35 PM, Andy Shevchenko wrote:
> On Mon, Jan 25, 2021 at 1:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 1/25/21 11:57 AM, Andy Shevchenko wrote:
>>> On Mon, Jan 25, 2021 at 9:37 AM Ike Panhc <ike.pan@canonical.com> wrote:
>>>>
>>>> On 1/17/21 3:49 AM, Andy Shevchenko wrote:
>>>>> On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>>>>>>
>>>>>> Consumers can differentiate an error from a successful read much more
>>>>>> easily if the read() call fails with the appropriate errno instead of
>>>>>> returning a magic string like "-1".
>>>>>
>>>>> Is user space ready for this (for the record, it seems an ABI breakage)?
>>>>>
>>>>
>>>> read() and getting errno looks sysfs/driver broken to me. I think
>>>> if button/method is not available, it's better to be something like
>>>> sysfs_emit(buf, "%d\n", -ENODEV)
>>>
>>> Either way it will be an ABI breakage.
>>
>> True any change here will be an ABI breakage, but I really do not expect
>> anything to be dependent on this weird behavior of returning errors by
>> writing some magic value to the buffer, rather then just error-ing out
>> of the read() call.
>>
>> The kernel-convention here clearly is to make the read() syscall fail with
>> -ESOMETHING on errors. So I see this as making the driver conform to the
>> expected sysfs API behavior. Since this change is nicely split out into a
>> separate patch, we can always revert it if it turns out there actually
>> is something depending on this. But again I see that as highly
>> unlikely.
> 
> Me too. My point is that every stakeholder here understands that.
> Perhaps elaborated in the commit message.

Ack, adding a note about this to the commit message would be good.

Barnabás, can you add a note about this to the commit message?

Also I think we are about ready for you to post a v3 of this
series (when you have time to do so).

Regards,

Hans

Re: [PATCH v2 09/24] platform/x86: ideapad-laptop: always propagate error codes from device attributes' show() callback

[Barnabás Pőcze]

2021. január 25., hétfő 12:40 keltezéssel, Hans de Goede írta:

> Hi,
>
> On 1/25/21 12:35 PM, Andy Shevchenko wrote:
> > On Mon, Jan 25, 2021 at 1:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >> On 1/25/21 11:57 AM, Andy Shevchenko wrote:
> >>> On Mon, Jan 25, 2021 at 9:37 AM Ike Panhc <ike.pan@canonical.com> wrote:
> >>>>
> >>>> On 1/17/21 3:49 AM, Andy Shevchenko wrote:
> >>>>> On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> >>>>>>
> >>>>>> Consumers can differentiate an error from a successful read much more
> >>>>>> easily if the read() call fails with the appropriate errno instead of
> >>>>>> returning a magic string like "-1".
> >>>>>
> >>>>> Is user space ready for this (for the record, it seems an ABI breakage)?
> >>>>>
> >>>>
> >>>> read() and getting errno looks sysfs/driver broken to me. I think
> >>>> if button/method is not available, it's better to be something like
> >>>> sysfs_emit(buf, "%d\n", -ENODEV)
> >>>
> >>> Either way it will be an ABI breakage.
> >>
> >> True any change here will be an ABI breakage, but I really do not expect
> >> anything to be dependent on this weird behavior of returning errors by
> >> writing some magic value to the buffer, rather then just error-ing out
> >> of the read() call.
> >>
> >> The kernel-convention here clearly is to make the read() syscall fail with
> >> -ESOMETHING on errors. So I see this as making the driver conform to the
> >> expected sysfs API behavior. Since this change is nicely split out into a
> >> separate patch, we can always revert it if it turns out there actually
> >> is something depending on this. But again I see that as highly
> >> unlikely.
> >
> > Me too. My point is that every stakeholder here understands that.
> > Perhaps elaborated in the commit message.
>
> Ack, adding a note about this to the commit message would be good.
>
> Barnabás, can you add a note about this to the commit message?
>
> Also I think we are about ready for you to post a v3 of this
> series (when you have time to do so).
>

Yes, I can add a note about this. I've been relatively busy for some time,
but I'm planning to submit the new version shortly (by the end of this week).