Re: [PATCH] platform/x86: thinkpad_acpi: Support for V9 DYTC platform profiles

[Hans de Goede <hdegoede@redhat.com>]

Hi Mark,

On 30-Jan-25 4:45 PM, Mark Pearson wrote:
> Newer Thinkpad AMD platforms are using V9 DYTC and this changes the
> profiles used for PSC mode. Add support for this update.
> Tested on P14s G5 AMD
> 
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 33 ++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 1fcb0f99695a..cae457bc0b07 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -10319,6 +10319,10 @@ static struct ibm_struct proxsensor_driver_data = {
>  #define DYTC_MODE_PSC_BALANCE  5  /* Default mode aka balanced */
>  #define DYTC_MODE_PSC_PERFORM  7  /* High power mode aka performance */
>  
> +#define DYTC_MODE_PSCV9_LOWPOWER 1  /* Low power mode */
> +#define DYTC_MODE_PSCV9_BALANCE  3  /* Default mode aka balanced */
> +#define DYTC_MODE_PSCV9_PERFORM  4  /* High power mode aka performance */
> +
>  #define DYTC_ERR_MASK       0xF  /* Bits 0-3 in cmd result are the error result */
>  #define DYTC_ERR_SUCCESS      1  /* CMD completed successful */
>  
> @@ -10339,6 +10343,10 @@ static int dytc_capabilities;
>  static bool dytc_mmc_get_available;
>  static int profile_force;
>  
> +static int platform_psc_profile_lowpower = DYTC_MODE_PSC_LOWPOWER;
> +static int platform_psc_profile_balanced = DYTC_MODE_PSC_BALANCE;
> +static int platform_psc_profile_performance = DYTC_MODE_PSC_PERFORM;
> +
>  static int convert_dytc_to_profile(int funcmode, int dytcmode,
>  		enum platform_profile_option *profile)
>  {
> @@ -10360,19 +10368,14 @@ static int convert_dytc_to_profile(int funcmode, int dytcmode,
>  		}
>  		return 0;
>  	case DYTC_FUNCTION_PSC:
> -		switch (dytcmode) {
> -		case DYTC_MODE_PSC_LOWPOWER:
> +		if (dytcmode == platform_psc_profile_lowpower)
>  			*profile = PLATFORM_PROFILE_LOW_POWER;
> -			break;
> -		case DYTC_MODE_PSC_BALANCE:
> +		else if (dytcmode == platform_psc_profile_balanced)
>  			*profile =  PLATFORM_PROFILE_BALANCED;
> -			break;
> -		case DYTC_MODE_PSC_PERFORM:
> +		else if (dytcmode == platform_psc_profile_performance)
>  			*profile =  PLATFORM_PROFILE_PERFORMANCE;
> -			break;
> -		default: /* Unknown mode */
> +		else
>  			return -EINVAL;
> -		}
>  		return 0;

Maybe replace the removed '}' with an empty line instead of
removing the entire line?

Currently after your patch the new code looks like this:

			...
		else
			return -EINVAL;
		return 0;

which look a bit weird, personally I would prefer:

			...
		else
			return -EINVAL;

		return 0;

Otherwise this looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans





>  	case DYTC_FUNCTION_AMT:
>  		/* For now return balanced. It's the closest we have to 'auto' */
> @@ -10393,19 +10396,19 @@ static int convert_profile_to_dytc(enum platform_profile_option profile, int *pe
>  		if (dytc_capabilities & BIT(DYTC_FC_MMC))
>  			*perfmode = DYTC_MODE_MMC_LOWPOWER;
>  		else if (dytc_capabilities & BIT(DYTC_FC_PSC))
> -			*perfmode = DYTC_MODE_PSC_LOWPOWER;
> +			*perfmode = platform_psc_profile_lowpower;
>  		break;
>  	case PLATFORM_PROFILE_BALANCED:
>  		if (dytc_capabilities & BIT(DYTC_FC_MMC))
>  			*perfmode = DYTC_MODE_MMC_BALANCE;
>  		else if (dytc_capabilities & BIT(DYTC_FC_PSC))
> -			*perfmode = DYTC_MODE_PSC_BALANCE;
> +			*perfmode = platform_psc_profile_balanced;
>  		break;
>  	case PLATFORM_PROFILE_PERFORMANCE:
>  		if (dytc_capabilities & BIT(DYTC_FC_MMC))
>  			*perfmode = DYTC_MODE_MMC_PERFORM;
>  		else if (dytc_capabilities & BIT(DYTC_FC_PSC))
> -			*perfmode = DYTC_MODE_PSC_PERFORM;
> +			*perfmode = platform_psc_profile_performance;
>  		break;
>  	default: /* Unknown profile */
>  		return -EOPNOTSUPP;
> @@ -10599,6 +10602,7 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>  	if (output & BIT(DYTC_QUERY_ENABLE_BIT))
>  		dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF;
>  
> +	dbg_printk(TPACPI_DBG_INIT, "DYTC version %d\n", dytc_version);
>  	/* Check DYTC is enabled and supports mode setting */
>  	if (dytc_version < 5)
>  		return -ENODEV;
> @@ -10637,6 +10641,11 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>  		}
>  	} else if (dytc_capabilities & BIT(DYTC_FC_PSC)) { /* PSC MODE */
>  		pr_debug("PSC is supported\n");
> +		if (dytc_version >= 9) { /* update profiles for DYTC 9 and up */
> +			platform_psc_profile_lowpower = DYTC_MODE_PSCV9_LOWPOWER;
> +			platform_psc_profile_balanced = DYTC_MODE_PSCV9_BALANCE;
> +			platform_psc_profile_performance = DYTC_MODE_PSCV9_PERFORM;
> +		}
>  	} else {
>  		dbg_printk(TPACPI_DBG_INIT, "No DYTC support available\n");
>  		return -ENODEV;