Re: [PATCH] platform/x86: Add support for improved performance mode

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3116 bytes --]

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@squebb.ca>
> >> ---
> >>  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.