On Wed, 14 Feb 2024, Vishnu Sankar wrote:
Thanks for doing this, it's major improvement to readability already as
is, and even more of after the second patch!!
> Add a thermal_read_mode_check helper function to make the code
thermal_read_mode_check()
remove "function" as it's obvious.
> simpler during init.
> This helps particularly when the new TPEC_12 mode is added in
> the next patch.
Flow the paragraph normally without the premature line break.
> Suggested-by: Ilpo Jarvinen <ilpo.jarvinen@xxxxxxxxx>
This is not my email address, please use
Suggested-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> Signed-off-by: Vishnu Sankar <vishnuocv@xxxxxxxxx>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 136 +++++++++++++--------------
> 1 file changed, 66 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index c4895e9bc714..2428c8bd0fa2 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -6147,6 +6147,71 @@ struct ibm_thermal_sensors_struct {
> static enum thermal_access_mode thermal_read_mode;
> static bool thermal_use_labels;
>
> +/* Function to check thermal read mode */
> +static enum thermal_access_mode thermal_read_mode_check(void)
> +{
> + u8 t, ta1, ta2, ver = 0;
> + int i;
> +
> + if (thinkpad_id.ec_model) {
> + /*
> + * Direct EC access mode: sensors at registers
> + * 0x78-0x7F, 0xC0-0xC7. Registers return 0x00 for
While this comment is touches, lets make minor improvements to it.
Remove the double space, they are not needed in kernel comments.
> + * non-implemented, thermal sensors return 0x80 when
> + * not available
Please add the missing .
Perhaps add an empty line here to break the comment into two paragraphs?
> + * The above rule is unfortunately flawed. This has been seen with
> + * 0xC2 (power supply ID) causing thermal control problems.
> + * The EC version can be determined by offset 0xEF and at least for
> + * version 3 the Lenovo firmware team confirmed that registers 0xC0-0xC7
> + * are not thermal registers.
Please reflow the entire comment to 80 chars.
> + */
> + if (!acpi_ec_read(TP_EC_FUNCREV, &ver))
> + pr_warn("Thinkpad ACPI EC unable to access EC version\n");
> +
> + ta1 = ta2 = 0;
> + for (i = 0; i < 8; i++) {
> + if (acpi_ec_read(TP_EC_THERMAL_TMP0 + i, &t)) {
> + ta1 |= t;
> + } else {
> + ta1 = 0;
> + break;
> + }
> + if (ver < 3) {
> + if (acpi_ec_read(TP_EC_THERMAL_TMP8 + i, &t)) {
> + ta2 |= t;
> + } else {
> + ta1 = 0;
> + break;
> + }
> + }
> + }
> +
> + if (ta1 == 0) {
> + pr_err("ThinkPad ACPI EC access misbehaving, disabling thermal sensors access\n");
> + return TPACPI_THERMAL_NONE;
> + }
> +
> + if (ver >= 3) {
> + thermal_use_labels = true;
> + return TPACPI_THERMAL_TPEC_8;
> + }
> +
> + return (ta2 != 0) ? TPACPI_THERMAL_TPEC_16 : TPACPI_THERMAL_TPEC_8;
> + }
> +
> + if (acpi_evalf(ec_handle, NULL, "TMP7", "qv")) {
> + if (tpacpi_is_ibm() &&
> + acpi_evalf(ec_handle, NULL, "UPDT", "qv"))
Put this to single line and retain the braces please (I know it will be
>80 char but nothing important is lost).
> + /* 600e/x, 770e, 770x */
> + return TPACPI_THERMAL_ACPI_UPDT;
> + /* IBM/LENOVO DSDT EC.TMPx access, max 8 sensors */
> + return TPACPI_THERMAL_ACPI_TMP07;
> + }
> +
> + /* temperatures not supported on 570, G4x, R30, R31, R32 */
> + return TPACPI_THERMAL_NONE;
> +}
> +
> /* idx is zero-based */
> static int thermal_get_sensor(int idx, s32 *value)
> {
> @@ -6375,78 +6440,9 @@ static const struct attribute_group temp_label_attr_group = {
>
> static int __init thermal_init(struct ibm_init_struct *iibm)
> {
> - u8 t, ta1, ta2, ver = 0;
> - int i;
> - int acpi_tmp7;
> -
> vdbg_printk(TPACPI_DBG_INIT, "initializing thermal subdriver\n");
>
> - acpi_tmp7 = acpi_evalf(ec_handle, NULL, "TMP7", "qv");
> -
> - if (thinkpad_id.ec_model) {
> - /*
> - * Direct EC access mode: sensors at registers
> - * 0x78-0x7F, 0xC0-0xC7. Registers return 0x00 for
> - * non-implemented, thermal sensors return 0x80 when
> - * not available
> - * The above rule is unfortunately flawed. This has been seen with
> - * 0xC2 (power supply ID) causing thermal control problems.
> - * The EC version can be determined by offset 0xEF and at least for
> - * version 3 the Lenovo firmware team confirmed that registers 0xC0-0xC7
> - * are not thermal registers.
> - */
> - if (!acpi_ec_read(TP_EC_FUNCREV, &ver))
> - pr_warn("Thinkpad ACPI EC unable to access EC version\n");
> -
> - ta1 = ta2 = 0;
> - for (i = 0; i < 8; i++) {
> - if (acpi_ec_read(TP_EC_THERMAL_TMP0 + i, &t)) {
> - ta1 |= t;
> - } else {
> - ta1 = 0;
> - break;
> - }
> - if (ver < 3) {
> - if (acpi_ec_read(TP_EC_THERMAL_TMP8 + i, &t)) {
> - ta2 |= t;
> - } else {
> - ta1 = 0;
> - break;
> - }
> - }
> - }
> - if (ta1 == 0) {
> - /* This is sheer paranoia, but we handle it anyway */
> - if (acpi_tmp7) {
> - pr_err("ThinkPad ACPI EC access misbehaving, falling back to ACPI TMPx access mode\n");
> - thermal_read_mode = TPACPI_THERMAL_ACPI_TMP07;
Eh, where did this go in the new helper?
--
i.
> - } else {
> - pr_err("ThinkPad ACPI EC access misbehaving, disabling thermal sensors access\n");
> - thermal_read_mode = TPACPI_THERMAL_NONE;
> - }
> - } else {
> - if (ver >= 3) {
> - thermal_read_mode = TPACPI_THERMAL_TPEC_8;
> - thermal_use_labels = true;
> - } else {
> - thermal_read_mode =
> - (ta2 != 0) ?
> - TPACPI_THERMAL_TPEC_16 : TPACPI_THERMAL_TPEC_8;
> - }
> - }
> - } else if (acpi_tmp7) {
> - if (tpacpi_is_ibm() &&
> - acpi_evalf(ec_handle, NULL, "UPDT", "qv")) {
> - /* 600e/x, 770e, 770x */
> - thermal_read_mode = TPACPI_THERMAL_ACPI_UPDT;
> - } else {
> - /* IBM/LENOVO DSDT EC.TMPx access, max 8 sensors */
> - thermal_read_mode = TPACPI_THERMAL_ACPI_TMP07;
> - }
> - } else {
> - /* temperatures not supported on 570, G4x, R30, R31, R32 */
> - thermal_read_mode = TPACPI_THERMAL_NONE;
> - }
> + thermal_read_mode = thermal_read_mode_check();
>
> vdbg_printk(TPACPI_DBG_INIT, "thermal is %s, mode %d\n",
> str_supported(thermal_read_mode != TPACPI_THERMAL_NONE),
>
