On Thu, 9 Nov 2023, Mark Pearson wrote:
> On Thu, Nov 9, 2023, at 5:10 AM, Ilpo Järvinen wrote:
> > On Wed, 8 Nov 2023, Mark Pearson wrote:
> >
> >> Some new Thinkpads have a new improved performance mode available.
> >> Add support to make this mode usable.
> >>
> >> To avoid having to create a new profile, just use the improved performance
> >> mode in place of the existing performance mode, when available.
> >>
> >> Tested on T14 AMD G4 AMD.
> >>
> >> Signed-off-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
> >> ---
> >> drivers/platform/x86/thinkpad_acpi.c | 28 ++++++++++++++++++++++++++++
> >> 1 file changed, 28 insertions(+)
> >>
> >> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> >> index ad460417f901..eba701ab340e 100644
> >> @@ -10163,11 +10165,14 @@ static struct ibm_struct proxsensor_driver_data = {
> >> #define DYTC_MODE_MMC_LOWPOWER 3 /* Low power mode */
> >> #define DYTC_MODE_MMC_BALANCE 0xF /* Default mode aka balanced */
> >> #define DYTC_MODE_MMC_DEFAULT 0 /* Default mode from MMC_GET, aka balanced */
> >> +#define DYTC_NOMODE 0xF /* When Function does not have a mode */
> >>
> >> #define DYTC_MODE_PSC_LOWPOWER 3 /* Low power mode */
> >> #define DYTC_MODE_PSC_BALANCE 5 /* Default mode aka balanced */
> >> #define DYTC_MODE_PSC_PERFORM 7 /* High power mode aka performance */
> >>
> >> +#define DYTC_UP_SUPPORT_BIT 8 /* Bit 8 - 1 = supported, 0 = not */
> >
> > It would be preferrable to comment what is supported rather than have a
> > comment like above which isn't particularly helpful.
>
> OK - so just have:
> #define DYTC_UP_SUPPORT_BIT 8 /* Ultra-performance (TMS) mode support */
>
> Or...reading ahead in the review this should actually be
> #define DYTC_UP_SUPPORT_BIT BIT(8) /* Ultra-performance (TMS) mode support */
Yes, the latter look good except I'd just drop the "_BIT" suffix from the
name.
> >> @@ -10484,6 +10502,16 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
> >> dbg_printk(TPACPI_DBG_INIT, "No DYTC support available\n");
> >> return -ENODEV;
> >> }
> >> + err = dytc_command(DYTC_CMD_UP_CAP, &output);
> >> + dytc_ultraperf_cap = output & BIT(DYTC_UP_SUPPORT_BIT) ? true : false;
> >
> > It would be better to put this BIT() into the define itself and remove
> > _BIT from the name because it doesn't really add that much information.
> > Since you're assigning to bool, ? true : false construct is not required
> > but implicit cast will handle it for you. So in the end, this line would
> > be:
> >
> > dytc_ultraperf_cap = output & DYTC_UP_SUPPORT;
>
> Agreed. I will make that change.
> I'll wait and see if there is any more feedback and then do that with a v2 patch.
>
> >
> > Looking into the driver a bit more, there are a few other defines which
> > could also move BIT() from the code into defines. Please tell if you're
> > going to look at those because if not, I might try to make the patches.
>
> Happy to look at doing that as I'm playing around with this driver anyway.
Okay, thanks.
--
i.
