Received-SPF: pass (google.com: domain of linux-kernel+bounces-66115-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240214052959.8550-1-vishnuocv@gmail.com> <c49e4415-7cd5-e1ba-e6c6-5086730b9866@linux.intel.com>
In-Reply-To: <c49e4415-7cd5-e1ba-e6c6-5086730b9866@linux.intel.com>
From: Vishnu Sankar <vishnuocv@gmail.com>
Date: Thu, 15 Feb 2024 08:18:51 +0900
Message-ID: <CABxCQKvJScRKDJ+bStMF-jd6=iVnbCUPO3RFA8CFYMvXE+uE3g@mail.gmail.com>
Subject: Re: [PATCH 1/2] platform/x86: thinkpad_acpi: Simplify thermal mode checking
To: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Ilpo,
Thanks a lot for the review.

On Wed, Feb 14, 2024 at 7:14=E2=80=AFPM Ilpo J=C3=A4rvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> 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.
Acked
>
> > 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.
Acked.
>
> > Suggested-by: Ilpo Jarvinen <ilpo.jarvinen@intel.com>
>
> This is not my email address, please use
>
> Suggested-by: Ilpo J=C3=A4rvinen <ilpo.jarvinen@linux.intel.com>
Sorry for this.
Will correct email address.
>
> > Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
> > ---
> >  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/x8=
6/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 =3D 0;
> > +     int i;
> > +
> > +     if (thinkpad_id.ec_model) {
> > +             /*
> > +              * Direct EC access mode: sensors at registers
> > +              * 0x78-0x7F, 0xC0-0xC7.  Registers return 0x00 for
>
> Remove the double space, one is enough in kernel comments.
Acked
>
> > +              * non-implemented, thermal sensors return 0x80 when
> > +              * not available
>
> Add the missing . please.
>
> Perhaps add a empty line here to make this two paragraphs.
Acked
>
> > +              * The above rule is unfortunately flawed. This has been =
seen with
> > +              * 0xC2 (power supply ID) causing thermal control problem=
s.
> > +              * The EC version can be determined by offset 0xEF and at=
 least for
> > +              * version 3 the Lenovo firmware team confirmed that regi=
sters 0xC0-0xC7
> > +              * are not thermal registers.
> > +              */
>
> While the patch touches this, this entire comment should be reflowed
> properly for 80 columns.
Will try re-writing the comments block for 80 columns.
>
> > +             if (!acpi_ec_read(TP_EC_FUNCREV, &ver))
> > +                     pr_warn("Thinkpad ACPI EC unable to access EC ver=
sion\n");
> > +
> > +             ta1 =3D ta2 =3D 0;
> > +             for (i =3D 0; i < 8; i++) {
> > +                     if (acpi_ec_read(TP_EC_THERMAL_TMP0 + i, &t)) {
> > +                             ta1 |=3D t;
> > +                     } else {
> > +                             ta1 =3D 0;
> > +                             break;
> > +                     }
> > +                     if (ver < 3) {
> > +                             if (acpi_ec_read(TP_EC_THERMAL_TMP8 + i, =
&t)) {
> > +                                     ta2 |=3D t;
> > +                             } else {
> > +                                     ta1 =3D 0;
> > +                                     break;
> > +                             }
> > +                     }
> > +             }
> > +
> > +             if (ta1 =3D=3D 0) {
> > +                     pr_err("ThinkPad ACPI EC access misbehaving, disa=
bling thermal sensors access\n");
> > +                     return TPACPI_THERMAL_NONE;
> > +             }
> > +
> > +             if (ver >=3D 3) {
> > +                     thermal_use_labels =3D true;
> > +                     return TPACPI_THERMAL_TPEC_8;
> > +             }
> > +
> > +             return (ta2 !=3D 0) ? TPACPI_THERMAL_TPEC_16 : TPACPI_THE=
RMAL_TPEC_8;
> > +     }
> > +
> > +     if (acpi_evalf(ec_handle, NULL, "TMP7", "qv")) {
> > +             if (tpacpi_is_ibm() &&
> > +                 acpi_evalf(ec_handle, NULL, "UPDT", "qv"))
>
> Single line and keep the braces please (I know it will go >80 chars but n=
o
> important info is lost).
Acked.
Will change this.
>
> > +                     /* 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_a=
ttr_group =3D {
> >
> >  static int __init thermal_init(struct ibm_init_struct *iibm)
> >  {
> > -     u8 t, ta1, ta2, ver =3D 0;
> > -     int i;
> > -     int acpi_tmp7;
> > -
> >       vdbg_printk(TPACPI_DBG_INIT, "initializing thermal subdriver\n");
> >
> > -     acpi_tmp7 =3D 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 problem=
s.
> > -              * The EC version can be determined by offset 0xEF and at=
 least for
> > -              * version 3 the Lenovo firmware team confirmed that regi=
sters 0xC0-0xC7
> > -              * are not thermal registers.
> > -              */
> > -             if (!acpi_ec_read(TP_EC_FUNCREV, &ver))
> > -                     pr_warn("Thinkpad ACPI EC unable to access EC ver=
sion\n");
> > -
> > -             ta1 =3D ta2 =3D 0;
> > -             for (i =3D 0; i < 8; i++) {
> > -                     if (acpi_ec_read(TP_EC_THERMAL_TMP0 + i, &t)) {
> > -                             ta1 |=3D t;
> > -                     } else {
> > -                             ta1 =3D 0;
> > -                             break;
> > -                     }
> > -                     if (ver < 3) {
> > -                             if (acpi_ec_read(TP_EC_THERMAL_TMP8 + i, =
&t)) {
> > -                                     ta2 |=3D t;
> > -                             } else {
> > -                                     ta1 =3D 0;
> > -                                     break;
> > -                             }
> > -                     }
> > -             }
> > -             if (ta1 =3D=3D 0) {
> > -                     /* This is sheer paranoia, but we handle it anywa=
y */
> > -                     if (acpi_tmp7) {
> > -                             pr_err("ThinkPad ACPI EC access misbehavi=
ng, falling back to ACPI TMPx access mode\n");
> > -                             thermal_read_mode =3D TPACPI_THERMAL_ACPI=
_TMP07;
>
> Eh, where did this go in the new helper?
Sorry.
This will be added in V2.
>
> --
>  i.
>
> > -                     } else {
> > -                             pr_err("ThinkPad ACPI EC access misbehavi=
ng, disabling thermal sensors access\n");
> > -                             thermal_read_mode =3D TPACPI_THERMAL_NONE=
;
> > -                     }
> > -             } else {
> > -                     if (ver >=3D 3) {
> > -                             thermal_read_mode =3D TPACPI_THERMAL_TPEC=
_8;
> > -                             thermal_use_labels =3D true;
> > -                     } else {
> > -                             thermal_read_mode =3D
> > -                                     (ta2 !=3D 0) ?
> > -                                     TPACPI_THERMAL_TPEC_16 : TPACPI_T=
HERMAL_TPEC_8;
> > -                     }
> > -             }
> > -     } else if (acpi_tmp7) {
> > -             if (tpacpi_is_ibm() &&
> > -                 acpi_evalf(ec_handle, NULL, "UPDT", "qv")) {
> > -                     /* 600e/x, 770e, 770x */
> > -                     thermal_read_mode =3D TPACPI_THERMAL_ACPI_UPDT;
> > -             } else {
> > -                     /* IBM/LENOVO DSDT EC.TMPx access, max 8 sensors =
*/
> > -                     thermal_read_mode =3D TPACPI_THERMAL_ACPI_TMP07;
> > -             }
> > -     } else {
> > -             /* temperatures not supported on 570, G4x, R30, R31, R32 =
*/
> > -             thermal_read_mode =3D TPACPI_THERMAL_NONE;
> > -     }
> > +     thermal_read_mode =3D thermal_read_mode_check();
> >
> >       vdbg_printk(TPACPI_DBG_INIT, "thermal is %s, mode %d\n",
> >               str_supported(thermal_read_mode !=3D TPACPI_THERMAL_NONE)=
,
> >


--=20

Regards,

      Vishnu Sankar
     +817015150407 (Japan)