Received-SPF: pass (google.com: domain of linux-kernel+bounces-65038-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Wed, 14 Feb 2024 12:16:02 +0200 (EET)
To: Vishnu Sankar <vishnuocv@gmail.com>
cc: Hans de Goede <hdegoede@redhat.com>, platform-driver-x86@vger.kernel.org, 
    LKML <linux-kernel@vger.kernel.org>, 
    Mark Pearson <mpearson-lenovo@squebb.ca>, vsankar@lenovo.com
Subject: Re: [PATCH 1/2] platform/x86: thinkpad_acpi: Simplify thermal mode
 checking
In-Reply-To: <20240214052959.8550-1-vishnuocv@gmail.com>
Message-ID: <05276532-52eb-3b7d-4578-c596bc1d79c4@linux.intel.com>
References: <20240214052959.8550-1-vishnuocv@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/mixed; BOUNDARY="8323328-1973336468-1707905081=:7959"
Content-ID: <6fa90c8f-3c00-25e4-8f33-2768e20495e7@linux.intel.com>

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-1973336468-1707905081=:7959
Content-Type: text/plain; CHARSET=ISO-8859-15
Content-Transfer-Encoding: QUOTED-PRINTABLE
Content-ID: <802be201-439b-532b-2b2b-6a298600f2f6@linux.intel.com>

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@intel.com>

This is not my email address, please use

Suggested-by: Ilpo J=E4rvinen <ilpo.jarvinen@linux.intel.com>

> Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 136 +++++++++++++--------------
>  1 file changed, 66 insertions(+), 70 deletions(-)
>=20
> 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;
> =20
> +/* Function to check thermal read mode */
> +static enum thermal_access_mode thermal_read_mode_check(void)
> +{
> +=09u8 t, ta1, ta2, ver =3D 0;
> +=09int i;
> +
> +=09if (thinkpad_id.ec_model) {
> +=09=09/*
> +=09=09 * Direct EC access mode: sensors at registers
> +=09=09 * 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.

> +=09=09 * non-implemented, thermal sensors return 0x80 when
> +=09=09 * not available

Please add the missing .

Perhaps add an empty line here to break the comment into two paragraphs?

> +=09=09 * The above rule is unfortunately flawed. This has been seen with
> +=09=09 * 0xC2 (power supply ID) causing thermal control problems.
> +=09=09 * The EC version can be determined by offset 0xEF and at least fo=
r
> +=09=09 * version 3 the Lenovo firmware team confirmed that registers 0xC=
0-0xC7
> +=09=09 * are not thermal registers.

Please reflow the entire comment to 80 chars.

> +=09=09 */
> +=09=09if (!acpi_ec_read(TP_EC_FUNCREV, &ver))
> +=09=09=09pr_warn("Thinkpad ACPI EC unable to access EC version\n");
> +
> +=09=09ta1 =3D ta2 =3D 0;
> +=09=09for (i =3D 0; i < 8; i++) {
> +=09=09=09if (acpi_ec_read(TP_EC_THERMAL_TMP0 + i, &t)) {
> +=09=09=09=09ta1 |=3D t;
> +=09=09=09} else {
> +=09=09=09=09ta1 =3D 0;
> +=09=09=09=09break;
> +=09=09=09}
> +=09=09=09if (ver < 3) {
> +=09=09=09=09if (acpi_ec_read(TP_EC_THERMAL_TMP8 + i, &t)) {
> +=09=09=09=09=09ta2 |=3D t;
> +=09=09=09=09} else {
> +=09=09=09=09=09ta1 =3D 0;
> +=09=09=09=09=09break;
> +=09=09=09=09}
> +=09=09=09}
> +=09=09}
> +
> +=09=09if (ta1 =3D=3D 0) {
> +=09=09=09pr_err("ThinkPad ACPI EC access misbehaving, disabling thermal =
sensors access\n");
> +=09=09=09return TPACPI_THERMAL_NONE;
> +=09=09}
> +
> +=09=09if (ver >=3D 3) {
> +=09=09=09thermal_use_labels =3D true;
> +=09=09=09return TPACPI_THERMAL_TPEC_8;
> +=09=09}
> +
> +=09=09return (ta2 !=3D 0) ? TPACPI_THERMAL_TPEC_16 : TPACPI_THERMAL_TPEC=
_8;
> +=09}
> +
> +=09if (acpi_evalf(ec_handle, NULL, "TMP7", "qv")) {
> +=09=09if (tpacpi_is_ibm() &&
> +=09=09    acpi_evalf(ec_handle, NULL, "UPDT", "qv"))

Put this to single line and retain the braces please (I know it will be=20
>80 char but nothing important is lost).

> +=09=09=09/* 600e/x, 770e, 770x */
> +=09=09=09return TPACPI_THERMAL_ACPI_UPDT;
> +=09=09/* IBM/LENOVO DSDT EC.TMPx access, max 8 sensors */
> +=09=09return TPACPI_THERMAL_ACPI_TMP07;
> +=09}
> +
> +=09/* temperatures not supported on 570, G4x, R30, R31, R32 */
> +=09return 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_att=
r_group =3D {
> =20
>  static int __init thermal_init(struct ibm_init_struct *iibm)
>  {
> -=09u8 t, ta1, ta2, ver =3D 0;
> -=09int i;
> -=09int acpi_tmp7;
> -
>  =09vdbg_printk(TPACPI_DBG_INIT, "initializing thermal subdriver\n");
> =20
> -=09acpi_tmp7 =3D acpi_evalf(ec_handle, NULL, "TMP7", "qv");
> -
> -=09if (thinkpad_id.ec_model) {
> -=09=09/*
> -=09=09 * Direct EC access mode: sensors at registers
> -=09=09 * 0x78-0x7F, 0xC0-0xC7.  Registers return 0x00 for
> -=09=09 * non-implemented, thermal sensors return 0x80 when
> -=09=09 * not available
> -=09=09 * The above rule is unfortunately flawed. This has been seen with
> -=09=09 * 0xC2 (power supply ID) causing thermal control problems.
> -=09=09 * The EC version can be determined by offset 0xEF and at least fo=
r
> -=09=09 * version 3 the Lenovo firmware team confirmed that registers 0xC=
0-0xC7
> -=09=09 * are not thermal registers.
> -=09=09 */
> -=09=09if (!acpi_ec_read(TP_EC_FUNCREV, &ver))
> -=09=09=09pr_warn("Thinkpad ACPI EC unable to access EC version\n");
> -
> -=09=09ta1 =3D ta2 =3D 0;
> -=09=09for (i =3D 0; i < 8; i++) {
> -=09=09=09if (acpi_ec_read(TP_EC_THERMAL_TMP0 + i, &t)) {
> -=09=09=09=09ta1 |=3D t;
> -=09=09=09} else {
> -=09=09=09=09ta1 =3D 0;
> -=09=09=09=09break;
> -=09=09=09}
> -=09=09=09if (ver < 3) {
> -=09=09=09=09if (acpi_ec_read(TP_EC_THERMAL_TMP8 + i, &t)) {
> -=09=09=09=09=09ta2 |=3D t;
> -=09=09=09=09} else {
> -=09=09=09=09=09ta1 =3D 0;
> -=09=09=09=09=09break;
> -=09=09=09=09}
> -=09=09=09}
> -=09=09}
> -=09=09if (ta1 =3D=3D 0) {
> -=09=09=09/* This is sheer paranoia, but we handle it anyway */
> -=09=09=09if (acpi_tmp7) {
> -=09=09=09=09pr_err("ThinkPad ACPI EC access misbehaving, falling back to=
 ACPI TMPx access mode\n");
> -=09=09=09=09thermal_read_mode =3D TPACPI_THERMAL_ACPI_TMP07;

Eh, where did this go in the new helper?

--=20
 i.

> -=09=09=09} else {
> -=09=09=09=09pr_err("ThinkPad ACPI EC access misbehaving, disabling therm=
al sensors access\n");
> -=09=09=09=09thermal_read_mode =3D TPACPI_THERMAL_NONE;
> -=09=09=09}
> -=09=09} else {
> -=09=09=09if (ver >=3D 3) {
> -=09=09=09=09thermal_read_mode =3D TPACPI_THERMAL_TPEC_8;
> -=09=09=09=09thermal_use_labels =3D true;
> -=09=09=09} else {
> -=09=09=09=09thermal_read_mode =3D
> -=09=09=09=09=09(ta2 !=3D 0) ?
> -=09=09=09=09=09TPACPI_THERMAL_TPEC_16 : TPACPI_THERMAL_TPEC_8;
> -=09=09=09}
> -=09=09}
> -=09} else if (acpi_tmp7) {
> -=09=09if (tpacpi_is_ibm() &&
> -=09=09    acpi_evalf(ec_handle, NULL, "UPDT", "qv")) {
> -=09=09=09/* 600e/x, 770e, 770x */
> -=09=09=09thermal_read_mode =3D TPACPI_THERMAL_ACPI_UPDT;
> -=09=09} else {
> -=09=09=09/* IBM/LENOVO DSDT EC.TMPx access, max 8 sensors */
> -=09=09=09thermal_read_mode =3D TPACPI_THERMAL_ACPI_TMP07;
> -=09=09}
> -=09} else {
> -=09=09/* temperatures not supported on 570, G4x, R30, R31, R32 */
> -=09=09thermal_read_mode =3D TPACPI_THERMAL_NONE;
> -=09}
> +=09thermal_read_mode =3D thermal_read_mode_check();
> =20
>  =09vdbg_printk(TPACPI_DBG_INIT, "thermal is %s, mode %d\n",
>  =09=09str_supported(thermal_read_mode !=3D TPACPI_THERMAL_NONE),
>=20
--8323328-1973336468-1707905081=:7959--