[PATCH 0/7] thinkpad_acpi: 1 bugfix + a bunch of cleanups

[PATCH 0/7] thinkpad_acpi: 1 bugfix + a bunch of cleanups

[Hans de Goede]

Hi All,

Here is a patch-series for the thinkpad_acpi driver starting with one
important bugfix which fixes a bug introduced by commit 79f960e29cfc
("platform/x86: thinkpad_acpi: Convert platform driver to use dev_groups")
in platform-drivers-x86/for-next which is causing the driver to not load
at all on many devices.

Followed by a bunch of cleanup patches.

Please test and review.

Regards,

Hans


Hans de Goede (7):
  platform/x86: thinkpad_acpi: Accept ibm_init_struct.init() returning
    -ENODEV
  platform/x86: thinkpad_acpi: Make *_init() functions return -ENODEV
    instead of 1
  platform/x86: thinkpad_acpi: Simplify dytc_version handling
  platform/x86: thinkpad_acpi: Cleanup dytc_profile_available
  platform/x86: thinkpad_acpi: Properly indent code in
    tpacpi_dytc_profile_init()
  platform/x86: thinkpad_acpi: Remove "goto err_exit" from hotkey_init()
  platform/x86: thinkpad_acpi: Fix thermal_temp_input_attr sorting

 drivers/platform/x86/thinkpad_acpi.c | 221 ++++++++++-----------------
 1 file changed, 78 insertions(+), 143 deletions(-)

-- 
2.31.1

[PATCH 1/7] platform/x86: thinkpad_acpi: Accept ibm_init_struct.init() returning -ENODEV

[Hans de Goede]

Commit 79f960e29cfc ("platform/x86: thinkpad_acpi: Convert platform driver
to use dev_groups") accidentally modified tpacpi_kbdlang_init() causing it
to return -ENODEV instead of 0 on machines without kbdlang support
(which are most of them).

ibm_init() sees this -ENODEV as an error causing the entire module to not
load, not good.

Note that technically tpacpi_kbdlang_init() was already buggy before, it
should have returned 1 instead of 0 if the feature is not present.

Rather then fixing tpacpi_kbdlang_init() though, IMHO it is bettter to
just make ibm_init() treat -ENODEV as 1 to fix the issue; and then in
a followup commit also change all the existing "return 1"s from
ibm_init_struct.init() callbacks to "return -ENODEV" as -ENODEV clearly
states what it going on where as a magic return of "1" requires a deep
dive into the code to figure out what is going on.

This will also allow removing some extra ifs to translate -ENODEV to
return 1 in a couple of init() callbacks.

Fixes: 79f960e29cfc ("platform/x86: thinkpad_acpi: Convert platform driver to use dev_groups")
Cc: Len Baker <len.baker@gmx.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 055930f5d9f5..62daf0109b4a 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -10738,8 +10738,8 @@ static int __init ibm_init(struct ibm_init_struct *iibm)
 
 	if (iibm->init) {
 		ret = iibm->init(iibm);
-		if (ret > 0)
-			return 0;	/* probe failed */
+		if (ret > 0 || ret == -ENODEV)
+			return 0; /* subdriver functionality not available */
 		if (ret)
 			return ret;
 
-- 
2.31.1

[PATCH 2/7] platform/x86: thinkpad_acpi: Make *_init() functions return -ENODEV instead of 1

[Hans de Goede]

Make ibm_init_struct.init() callbacks return -ENODEV instead of 1 when
the subdevice / function is not available.

Using -ENODEV clearly states what it going on where as a magic return of
"1" requires a deep dive into the code to figure out what is going on.

This also allows for some cleanups, avoiding the need to translate an
-ENODEV return into "return 1" (which often mistakenly was "return 0").

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 85 +++++++++++-----------------
 1 file changed, 34 insertions(+), 51 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 62daf0109b4a..24a3c79a045e 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3379,7 +3379,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 		str_supported(tp_features.hotkey));
 
 	if (!tp_features.hotkey)
-		return 1;
+		return -ENODEV;
 
 	quirks = tpacpi_check_quirks(tpacpi_hotkey_qtable,
 				     ARRAY_SIZE(tpacpi_hotkey_qtable));
@@ -3586,7 +3586,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 	return 0;
 
 err_exit:
-	return (res < 0) ? res : 1;
+	return (res < 0) ? res : -ENODEV;
 }
 
 /* Thinkpad X1 Carbon support 5 modes including Home mode, Web browser
@@ -4467,7 +4467,7 @@ static int __init bluetooth_init(struct ibm_init_struct *iibm)
 	}
 
 	if (!tp_features.bluetooth)
-		return 1;
+		return -ENODEV;
 
 	res = tpacpi_new_rfkill(TPACPI_RFK_BLUETOOTH_SW_ID,
 				&bluetooth_tprfk_ops,
@@ -4647,7 +4647,7 @@ static int __init wan_init(struct ibm_init_struct *iibm)
 	}
 
 	if (!tp_features.wan)
-		return 1;
+		return -ENODEV;
 
 	res = tpacpi_new_rfkill(TPACPI_RFK_WWAN_SW_ID,
 				&wan_tprfk_ops,
@@ -4776,7 +4776,7 @@ static int __init uwb_init(struct ibm_init_struct *iibm)
 	}
 
 	if (!tp_features.uwb)
-		return 1;
+		return -ENODEV;
 
 	res = tpacpi_new_rfkill(TPACPI_RFK_UWB_SW_ID,
 				&uwb_tprfk_ops,
@@ -4869,7 +4869,7 @@ static int __init video_init(struct ibm_init_struct *iibm)
 		str_supported(video_supported != TPACPI_VIDEO_NONE),
 		video_supported);
 
-	return (video_supported != TPACPI_VIDEO_NONE) ? 0 : 1;
+	return (video_supported != TPACPI_VIDEO_NONE) ? 0 : -ENODEV;
 }
 
 static void video_exit(void)
@@ -5277,7 +5277,7 @@ static int __init kbdlight_init(struct ibm_init_struct *iibm)
 	if (!kbdlight_is_supported()) {
 		tp_features.kbdlight = 0;
 		vdbg_printk(TPACPI_DBG_INIT, "kbdlight is unsupported\n");
-		return 1;
+		return -ENODEV;
 	}
 
 	kbdlight_brightness = kbdlight_sysfs_get(NULL);
@@ -5467,7 +5467,7 @@ static int __init light_init(struct ibm_init_struct *iibm)
 		str_supported(tp_features.light_status));
 
 	if (!tp_features.light)
-		return 1;
+		return -ENODEV;
 
 	rc = led_classdev_register(&tpacpi_pdev->dev,
 				   &tpacpi_led_thinklight.led_classdev);
@@ -5583,7 +5583,7 @@ static int __init cmos_init(struct ibm_init_struct *iibm)
 	vdbg_printk(TPACPI_DBG_INIT, "cmos commands are %s\n",
 		    str_supported(cmos_handle != NULL));
 
-	return cmos_handle ? 0 : 1;
+	return cmos_handle ? 0 : -ENODEV;
 }
 
 static int cmos_read(struct seq_file *m)
@@ -5928,7 +5928,7 @@ static int __init led_init(struct ibm_init_struct *iibm)
 		str_supported(led_supported), led_supported);
 
 	if (led_supported == TPACPI_LED_NONE)
-		return 1;
+		return -ENODEV;
 
 	tpacpi_leds = kcalloc(TPACPI_LED_NUMLEDS, sizeof(*tpacpi_leds),
 			      GFP_KERNEL);
@@ -6057,7 +6057,7 @@ static int __init beep_init(struct ibm_init_struct *iibm)
 
 	tp_features.beep_needs_two_args = !!(quirks & TPACPI_BEEP_Q1);
 
-	return (beep_handle) ? 0 : 1;
+	return (beep_handle) ? 0 : -ENODEV;
 }
 
 static int beep_read(struct seq_file *m)
@@ -6441,7 +6441,7 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
 		str_supported(thermal_read_mode != TPACPI_THERMAL_NONE),
 		thermal_read_mode);
 
-	return thermal_read_mode == TPACPI_THERMAL_NONE ? 1 : 0;
+	return thermal_read_mode != TPACPI_THERMAL_NONE ? 0 : -ENODEV;
 }
 
 static int thermal_read(struct seq_file *m)
@@ -6852,25 +6852,25 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
 
 	/* if it is unknown, we don't handle it: it wouldn't be safe */
 	if (tp_features.bright_unkfw)
-		return 1;
+		return -ENODEV;
 
 	if (!brightness_enable) {
 		dbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_BRGHT,
 			   "brightness support disabled by module parameter\n");
-		return 1;
+		return -ENODEV;
 	}
 
 	if (acpi_video_get_backlight_type() != acpi_backlight_vendor) {
 		if (brightness_enable > 1) {
 			pr_info("Standard ACPI backlight interface available, not loading native one\n");
-			return 1;
+			return -ENODEV;
 		} else if (brightness_enable == 1) {
 			pr_warn("Cannot enable backlight brightness support, ACPI is already handling it.  Refer to the acpi_backlight kernel parameter.\n");
-			return 1;
+			return -ENODEV;
 		}
 	} else if (!tp_features.bright_acpimode) {
 		pr_notice("ACPI backlight interface not available\n");
-		return 1;
+		return -ENODEV;
 	}
 
 	pr_notice("ACPI native brightness control enabled\n");
@@ -6903,7 +6903,7 @@ static int __init brightness_init(struct ibm_init_struct *iibm)
 		return -EINVAL;
 
 	if (tpacpi_brightness_get_raw(&b) < 0)
-		return 1;
+		return -ENODEV;
 
 	memset(&props, 0, sizeof(struct backlight_properties));
 	props.type = BACKLIGHT_PLATFORM;
@@ -7493,7 +7493,7 @@ static int __init volume_create_alsa_mixer(void)
 			  sizeof(struct tpacpi_alsa_data), &card);
 	if (rc < 0 || !card) {
 		pr_err("Failed to create ALSA card structures: %d\n", rc);
-		return 1;
+		return -ENODEV;
 	}
 
 	BUG_ON(!card->private_data);
@@ -7552,7 +7552,7 @@ static int __init volume_create_alsa_mixer(void)
 
 err_exit:
 	snd_card_free(card);
-	return 1;
+	return -ENODEV;
 }
 
 #define TPACPI_VOL_Q_MUTEONLY	0x0001	/* Mute-only control available */
@@ -7601,7 +7601,7 @@ static int __init volume_init(struct ibm_init_struct *iibm)
 	if (volume_mode == TPACPI_VOL_MODE_UCMS_STEP) {
 		pr_err("UCMS step volume mode not implemented, please contact %s\n",
 		       TPACPI_MAIL);
-		return 1;
+		return -ENODEV;
 	}
 
 	if (volume_capabilities >= TPACPI_VOL_CAP_MAX)
@@ -7614,7 +7614,7 @@ static int __init volume_init(struct ibm_init_struct *iibm)
 	if (!alsa_enable) {
 		vdbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_MIXER,
 			    "ALSA mixer disabled by parameter, not loading volume subdriver...\n");
-		return 1;
+		return -ENODEV;
 	}
 
 	quirks = tpacpi_check_quirks(volume_quirk_table,
@@ -7627,7 +7627,7 @@ static int __init volume_init(struct ibm_init_struct *iibm)
 		else if (quirks & TPACPI_VOL_Q_LEVEL)
 			tp_features.mixer_no_level_control = 0;
 		else
-			return 1; /* no mixer */
+			return -ENODEV; /* no mixer */
 		break;
 	case TPACPI_VOL_CAP_VOLMUTE:
 		tp_features.mixer_no_level_control = 0;
@@ -7636,7 +7636,7 @@ static int __init volume_init(struct ibm_init_struct *iibm)
 		tp_features.mixer_no_level_control = 1;
 		break;
 	default:
-		return 1;
+		return -ENODEV;
 	}
 
 	if (volume_capabilities != TPACPI_VOL_CAP_AUTO)
@@ -7808,7 +7808,7 @@ static int __init volume_init(struct ibm_init_struct *iibm)
 {
 	pr_info("volume: disabled as there is no ALSA support in this kernel\n");
 
-	return 1;
+	return -ENODEV;
 }
 
 static struct ibm_struct volume_driver_data = {
@@ -8745,7 +8745,7 @@ static int __init fan_init(struct ibm_init_struct *iibm)
 			}
 		} else {
 			pr_err("ThinkPad ACPI EC access misbehaving, fan status and control unavailable\n");
-			return 1;
+			return -ENODEV;
 		}
 	}
 
@@ -8794,11 +8794,11 @@ static int __init fan_init(struct ibm_init_struct *iibm)
 	if (fan_status_access_mode != TPACPI_FAN_NONE)
 		fan_get_status_safe(NULL);
 
-	if (fan_status_access_mode != TPACPI_FAN_NONE ||
-	    fan_control_access_mode != TPACPI_FAN_WR_NONE)
-		return 0;
+	if (fan_status_access_mode == TPACPI_FAN_NONE &&
+	    fan_control_access_mode == TPACPI_FAN_WR_NONE)
+		return -ENODEV;
 
-	return 1;
+	return 0;
 }
 
 static void fan_exit(void)
@@ -9925,12 +9925,9 @@ static int tpacpi_proxsensor_init(struct ibm_init_struct *iibm)
 
 	palm_err = palmsensor_get(&has_palmsensor, &palm_state);
 	lap_err = lapsensor_get(&has_lapsensor, &lap_state);
-	/*
-	 * If support isn't available (ENODEV) for both devices then quit, but
-	 * don't return an error.
-	 */
+	/* If support isn't available for both devices return -ENODEV */
 	if ((palm_err == -ENODEV) && (lap_err == -ENODEV))
-		return 0;
+		return -ENODEV;
 	/* Otherwise, if there was an error return it */
 	if (palm_err && (palm_err != -ENODEV))
 		return palm_err;
@@ -10166,13 +10163,6 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 
 	dytc_profile_available = false;
 	err = dytc_command(DYTC_CMD_QUERY, &output);
-	/*
-	 * If support isn't available (ENODEV) then don't return an error
-	 * and don't create the sysfs group
-	 */
-	if (err == -ENODEV)
-		return 0;
-	/* For all other errors we can flag the failure */
 	if (err)
 		return err;
 
@@ -10475,16 +10465,9 @@ static const struct attribute_group dprc_attr_group = {
 
 static int tpacpi_dprc_init(struct ibm_init_struct *iibm)
 {
-	int err = get_wwan_antenna(&wwan_antennatype);
-
-	/*
-	 * If support isn't available (ENODEV) then quit, but don't
-	 * return an error.
-	 */
-	if (err == -ENODEV)
-		return 0;
+	int err;
 
-	/* If there was an error return it */
+	err = get_wwan_antenna(&wwan_antennatype);
 	if (err)
 		return err;
 
-- 
2.31.1

[PATCH 3/7] platform/x86: thinkpad_acpi: Simplify dytc_version handling

[Hans de Goede]

The only reason the proxysensor code needs dytc_version handling is for
proxsensor_attr_is_visible() and that will only ever get called after
all the subdrv init() callbacks have run.

tpacpi_dytc_profile_init() already calls DYTC_CMD_QUERY and is the
primary consumer of dytc_version, so simply let tpacpi_dytc_profile_init()
set dytc_version and remove the now no longer necessary dytc_get_version()
helper and its calls.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 47 +++-------------------------
 1 file changed, 5 insertions(+), 42 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 24a3c79a045e..4c8050a0655a 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9781,33 +9781,6 @@ static int dytc_command(int command, int *output)
 	return 0;
 }
 
-static int dytc_get_version(void)
-{
-	int err, output;
-
-	/* Check if we've been called before - and just return cached value */
-	if (dytc_version)
-		return dytc_version;
-
-	/* Otherwise query DYTC and extract version information */
-	err = dytc_command(DYTC_CMD_QUERY, &output);
-	/*
-	 * If support isn't available (ENODEV) then don't return an error
-	 * and don't create the sysfs group
-	 */
-	if (err == -ENODEV)
-		return 0;
-	/* For all other errors we can flag the failure */
-	if (err)
-		return err;
-
-	/* Check DYTC is enabled and supports mode setting */
-	if (output & BIT(DYTC_QUERY_ENABLE_BIT))
-		dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF;
-
-	return 0;
-}
-
 static int lapsensor_get(bool *present, bool *state)
 {
 	int output, err;
@@ -9904,7 +9877,7 @@ static umode_t proxsensor_attr_is_visible(struct kobject *kobj,
 		 * Platforms before DYTC version 5 claim to have a lap sensor,
 		 * but it doesn't work, so we ignore them.
 		 */
-		if (!has_lapsensor ||  dytc_version < 5)
+		if (!has_lapsensor || dytc_version < 5)
 			return 0;
 	} else if (attr == &dev_attr_palmsensor.attr) {
 		if (!has_palmsensor)
@@ -9921,7 +9894,7 @@ static const struct attribute_group proxsensor_attr_group = {
 
 static int tpacpi_proxsensor_init(struct ibm_init_struct *iibm)
 {
-	int palm_err, lap_err, err;
+	int palm_err, lap_err;
 
 	palm_err = palmsensor_get(&has_palmsensor, &palm_state);
 	lap_err = lapsensor_get(&has_lapsensor, &lap_state);
@@ -9934,13 +9907,6 @@ static int tpacpi_proxsensor_init(struct ibm_init_struct *iibm)
 	if (lap_err && (lap_err != -ENODEV))
 		return lap_err;
 
-	/* Check if we know the DYTC version, if we don't then get it */
-	if (!dytc_version) {
-		err = dytc_get_version();
-		if (err)
-			return err;
-	}
-
 	return 0;
 }
 
@@ -10166,12 +10132,9 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 	if (err)
 		return err;
 
-	/* Check if we know the DYTC version, if we don't then get it */
-	if (!dytc_version) {
-		err = dytc_get_version();
-		if (err)
-			return err;
-	}
+	if (output & BIT(DYTC_QUERY_ENABLE_BIT))
+		dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF;
+
 	/* Check DYTC is enabled and supports mode setting */
 	if (dytc_version >= 5) {
 		dbg_printk(TPACPI_DBG_INIT,
-- 
2.31.1

[PATCH 4/7] platform/x86: thinkpad_acpi: Cleanup dytc_profile_available

[Hans de Goede]

Remove the dytc_profile_available check from dytc_profile_set(),
that function only gets called if the platform_profile_handler was
registered, so the check is not necessary.

Make tpacpi_dytc_profile_init() return -ENODEV when it does not register
the platform_profile() handler this will cause
dytc_profile_driver_data.flags.init to not get set, which in turn will
cause the dytc_profile_exit() call to get skipped.

Together this avoids the need to have the dytc_profile_available
variable at all, since the information is now duplicated in the
dytc_profile_driver_data.flags.init flag.

Note this leaves a weirdly indented code-block behind, this is
deliberately done to make what actually changes in this commit clear.
This will be fixed-up in the next commit.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 4c8050a0655a..cdc4028be341 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9950,7 +9950,6 @@ static struct ibm_struct proxsensor_driver_data = {
 
 #define DYTC_ENABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 1)
 
-static bool dytc_profile_available;
 static enum platform_profile_option dytc_current_profile;
 static atomic_t dytc_ignore_event = ATOMIC_INIT(0);
 static DEFINE_MUTEX(dytc_mutex);
@@ -10054,9 +10053,6 @@ static int dytc_profile_set(struct platform_profile_handler *pprof,
 	int output;
 	int err;
 
-	if (!dytc_profile_available)
-		return -ENODEV;
-
 	err = mutex_lock_interruptible(&dytc_mutex);
 	if (err)
 		return err;
@@ -10127,7 +10123,6 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 	set_bit(PLATFORM_PROFILE_BALANCED, dytc_profile.choices);
 	set_bit(PLATFORM_PROFILE_PERFORMANCE, dytc_profile.choices);
 
-	dytc_profile_available = false;
 	err = dytc_command(DYTC_CMD_QUERY, &output);
 	if (err)
 		return err;
@@ -10136,7 +10131,10 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 		dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF;
 
 	/* Check DYTC is enabled and supports mode setting */
-	if (dytc_version >= 5) {
+	if (dytc_version < 5)
+		return -ENODEV;
+
+	{
 		dbg_printk(TPACPI_DBG_INIT,
 				"DYTC version %d: thermal mode available\n", dytc_version);
 		/*
@@ -10156,9 +10154,8 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 		 * don't quit terminally.
 		 */
 		if (err)
-			return 0;
+			return -ENODEV;
 
-		dytc_profile_available = true;
 		/* Ensure initial values are correct */
 		dytc_profile_refresh();
 	}
@@ -10167,10 +10164,7 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 
 static void dytc_profile_exit(void)
 {
-	if (dytc_profile_available) {
-		dytc_profile_available = false;
-		platform_profile_remove();
-	}
+	platform_profile_remove();
 }
 
 static struct ibm_struct  dytc_profile_driver_data = {
-- 
2.31.1

[PATCH 5/7] platform/x86: thinkpad_acpi: Properly indent code in tpacpi_dytc_profile_init()

[Hans de Goede]

The previous refactoring of some code in tpacpi_dytc_profile_init() left
a weirdly indented code-block behind.

Remove the unnecessary '{}' and reduce the indent level one step,
other then changing the indentation the code is completely unchanged.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 47 ++++++++++++++--------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index cdc4028be341..acfe08e7dc3f 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -10134,31 +10134,30 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 	if (dytc_version < 5)
 		return -ENODEV;
 
-	{
-		dbg_printk(TPACPI_DBG_INIT,
-				"DYTC version %d: thermal mode available\n", dytc_version);
-		/*
-		 * Check if MMC_GET functionality available
-		 * Version > 6 and return success from MMC_GET command
-		 */
-		dytc_mmc_get_available = false;
-		if (dytc_version >= 6) {
-			err = dytc_command(DYTC_CMD_MMC_GET, &output);
-			if (!err && ((output & DYTC_ERR_MASK) == DYTC_ERR_SUCCESS))
-				dytc_mmc_get_available = true;
-		}
-		/* Create platform_profile structure and register */
-		err = platform_profile_register(&dytc_profile);
-		/*
-		 * If for some reason platform_profiles aren't enabled
-		 * don't quit terminally.
-		 */
-		if (err)
-			return -ENODEV;
-
-		/* Ensure initial values are correct */
-		dytc_profile_refresh();
+	dbg_printk(TPACPI_DBG_INIT,
+			"DYTC version %d: thermal mode available\n", dytc_version);
+	/*
+	 * Check if MMC_GET functionality available
+	 * Version > 6 and return success from MMC_GET command
+	 */
+	dytc_mmc_get_available = false;
+	if (dytc_version >= 6) {
+		err = dytc_command(DYTC_CMD_MMC_GET, &output);
+		if (!err && ((output & DYTC_ERR_MASK) == DYTC_ERR_SUCCESS))
+			dytc_mmc_get_available = true;
 	}
+	/* Create platform_profile structure and register */
+	err = platform_profile_register(&dytc_profile);
+	/*
+	 * If for some reason platform_profiles aren't enabled
+	 * don't quit terminally.
+	 */
+	if (err)
+		return -ENODEV;
+
+	/* Ensure initial values are correct */
+	dytc_profile_refresh();
+
 	return 0;
 }
 
-- 
2.31.1

[PATCH 6/7] platform/x86: thinkpad_acpi: Remove "goto err_exit" from hotkey_init()

[Hans de Goede]

The err_exit label just does a:
 return (res < 0) ? res : -ENODEV;

And res is always < 0 when we go there (hotkey_mask_get() returns
either 0 or -EIO), so the goto-s can simply be replaced with
"return res".

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index acfe08e7dc3f..9f45949fb3e9 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3466,7 +3466,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 		 * the first hotkey_mask_get to return hotkey_orig_mask */
 		res = hotkey_mask_get();
 		if (res)
-			goto err_exit;
+			return res;
 
 		hotkey_orig_mask = hotkey_acpi_mask;
 	} else {
@@ -3502,8 +3502,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 			TPACPI_HOTKEY_MAP_SIZE,	GFP_KERNEL);
 	if (!hotkey_keycode_map) {
 		pr_err("failed to allocate memory for key map\n");
-		res = -ENOMEM;
-		goto err_exit;
+		return -ENOMEM;
 	}
 
 	input_set_capability(tpacpi_inputdev, EV_MSC, MSC_SCAN);
@@ -3584,9 +3583,6 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 	hotkey_poll_setup_safe(true);
 
 	return 0;
-
-err_exit:
-	return (res < 0) ? res : -ENODEV;
 }
 
 /* Thinkpad X1 Carbon support 5 modes including Home mode, Web browser
-- 
2.31.1

[PATCH 7/7] platform/x86: thinkpad_acpi: Fix thermal_temp_input_attr sorting

[Hans de Goede]

Fix thermal_temp_input_attr sorting. Now that we use is_visible,
rather then registering only part of the thermal_temp_input_attr array,
putting attr 0-7 last is no longer needed.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 9f45949fb3e9..2d2d5fd11635 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -6283,14 +6283,6 @@ static struct sensor_device_attribute sensor_dev_attr_thermal_temp_input[] = {
 	&sensor_dev_attr_thermal_temp_input[X].dev_attr.attr
 
 static struct attribute *thermal_temp_input_attr[] = {
-	THERMAL_ATTRS(8),
-	THERMAL_ATTRS(9),
-	THERMAL_ATTRS(10),
-	THERMAL_ATTRS(11),
-	THERMAL_ATTRS(12),
-	THERMAL_ATTRS(13),
-	THERMAL_ATTRS(14),
-	THERMAL_ATTRS(15),
 	THERMAL_ATTRS(0),
 	THERMAL_ATTRS(1),
 	THERMAL_ATTRS(2),
@@ -6299,6 +6291,14 @@ static struct attribute *thermal_temp_input_attr[] = {
 	THERMAL_ATTRS(5),
 	THERMAL_ATTRS(6),
 	THERMAL_ATTRS(7),
+	THERMAL_ATTRS(8),
+	THERMAL_ATTRS(9),
+	THERMAL_ATTRS(10),
+	THERMAL_ATTRS(11),
+	THERMAL_ATTRS(12),
+	THERMAL_ATTRS(13),
+	THERMAL_ATTRS(14),
+	THERMAL_ATTRS(15),
 	NULL
 };
 
-- 
2.31.1

Re: [PATCH 0/7] thinkpad_acpi: 1 bugfix + a bunch of cleanups

[Andy Shevchenko]

On Sun, Nov 21, 2021 at 9:11 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> Here is a patch-series for the thinkpad_acpi driver starting with one
> important bugfix which fixes a bug introduced by commit 79f960e29cfc
> ("platform/x86: thinkpad_acpi: Convert platform driver to use dev_groups")
> in platform-drivers-x86/for-next which is causing the driver to not load
> at all on many devices.
>
> Followed by a bunch of cleanup patches.

It's right thing to do
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Please test and review.
>
> Regards,
>
> Hans
>
>
> Hans de Goede (7):
>   platform/x86: thinkpad_acpi: Accept ibm_init_struct.init() returning
>     -ENODEV
>   platform/x86: thinkpad_acpi: Make *_init() functions return -ENODEV
>     instead of 1
>   platform/x86: thinkpad_acpi: Simplify dytc_version handling
>   platform/x86: thinkpad_acpi: Cleanup dytc_profile_available
>   platform/x86: thinkpad_acpi: Properly indent code in
>     tpacpi_dytc_profile_init()
>   platform/x86: thinkpad_acpi: Remove "goto err_exit" from hotkey_init()
>   platform/x86: thinkpad_acpi: Fix thermal_temp_input_attr sorting
>
>  drivers/platform/x86/thinkpad_acpi.c | 221 ++++++++++-----------------
>  1 file changed, 78 insertions(+), 143 deletions(-)
>
> --
> 2.31.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/7] thinkpad_acpi: 1 bugfix + a bunch of cleanups

[Hans de Goede]

Hi,

On 11/22/21 11:14, Andy Shevchenko wrote:
> On Sun, Nov 21, 2021 at 9:11 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi All,
>>
>> Here is a patch-series for the thinkpad_acpi driver starting with one
>> important bugfix which fixes a bug introduced by commit 79f960e29cfc
>> ("platform/x86: thinkpad_acpi: Convert platform driver to use dev_groups")
>> in platform-drivers-x86/for-next which is causing the driver to not load
>> at all on many devices.
>>
>> Followed by a bunch of cleanup patches.
> 
> It's right thing to do
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thank you, I've pushed the entire series to my review-hans
branch now.

Regards,

Hans


>> Hans de Goede (7):
>>   platform/x86: thinkpad_acpi: Accept ibm_init_struct.init() returning
>>     -ENODEV
>>   platform/x86: thinkpad_acpi: Make *_init() functions return -ENODEV
>>     instead of 1
>>   platform/x86: thinkpad_acpi: Simplify dytc_version handling
>>   platform/x86: thinkpad_acpi: Cleanup dytc_profile_available
>>   platform/x86: thinkpad_acpi: Properly indent code in
>>     tpacpi_dytc_profile_init()
>>   platform/x86: thinkpad_acpi: Remove "goto err_exit" from hotkey_init()
>>   platform/x86: thinkpad_acpi: Fix thermal_temp_input_attr sorting
>>
>>  drivers/platform/x86/thinkpad_acpi.c | 221 ++++++++++-----------------
>>  1 file changed, 78 insertions(+), 143 deletions(-)
>>
>> --
>> 2.31.1
>>
> 
> 

Re: [External] [PATCH 0/7] thinkpad_acpi: 1 bugfix + a bunch of cleanups

[Mark Pearson]

Hi Hans

On 2021-11-21 14:11, Hans de Goede wrote:
> Hi All,
> 
> Here is a patch-series for the thinkpad_acpi driver starting with one
> important bugfix which fixes a bug introduced by commit 79f960e29cfc
> ("platform/x86: thinkpad_acpi: Convert platform driver to use dev_groups")
> in platform-drivers-x86/for-next which is causing the driver to not load
> at all on many devices.
> 
> Followed by a bunch of cleanup patches.
> 
> Please test and review.
> 
> Regards,
> 
> Hans
> 
> 
> Hans de Goede (7):
>   platform/x86: thinkpad_acpi: Accept ibm_init_struct.init() returning
>     -ENODEV
>   platform/x86: thinkpad_acpi: Make *_init() functions return -ENODEV
>     instead of 1
>   platform/x86: thinkpad_acpi: Simplify dytc_version handling
>   platform/x86: thinkpad_acpi: Cleanup dytc_profile_available
>   platform/x86: thinkpad_acpi: Properly indent code in
>     tpacpi_dytc_profile_init()
>   platform/x86: thinkpad_acpi: Remove "goto err_exit" from hotkey_init()
>   platform/x86: thinkpad_acpi: Fix thermal_temp_input_attr sorting
> 
>  drivers/platform/x86/thinkpad_acpi.c | 221 ++++++++++-----------------
>  1 file changed, 78 insertions(+), 143 deletions(-)
> 
Thanks for these - I didn't see any issues (I like the fix for ENODEV -
I remember that giving me a headache when we added the dytc feature and
it breaking older platforms)
I've tried them out on my P1G4 and everything looks good. I'll aim to
run it on a few other platforms as time allows

tested-by: Mark Pearson <mpearson@lenovo.com>

Mark