Hi Hans,
Thanks for the review.
On Thu, Jan 30, 2025, at 11:25 AM, Hans de Goede wrote:
> 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@xxxxxxxxx>
>> ---
>> 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;
Agreed - it does look oddly ugly doesn't it.
I'll wait and see if there is any other feedback, and then make that change for v2
>
> Otherwise this looks good to me:
>
> Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>
Thanks
Mark
