Re: [PATCH v2 2/5] platform/x86/amd/pmf: Use explicit SET_CMD/GET_CMD flags in amd_pmf_send_cmd()

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

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

On Tue, 18 Nov 2025, Shyam Sundar S K wrote:

> 
> 
> On 11/18/2025 21:11, Ilpo Järvinen wrote:
> > On Tue, 11 Nov 2025, Shyam Sundar S K wrote:
> > 
> >> Add SET_CMD and GET_CMD constants and replace boolean values passed as the
> >> get/set argument to amd_pmf_send_cmd() with the new explicit flags. This
> >> improves readability, avoids ambiguity around true/false and 0/1 usage.
> >> There is no functional change.
> >>
> >> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> >> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> >> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> ---
> >>  drivers/platform/x86/amd/pmf/auto-mode.c | 14 ++++-----
> >>  drivers/platform/x86/amd/pmf/cnqf.c      | 14 ++++-----
> >>  drivers/platform/x86/amd/pmf/core.c      |  6 ++--
> >>  drivers/platform/x86/amd/pmf/pmf.h       |  3 ++
> >>  drivers/platform/x86/amd/pmf/spc.c       |  2 +-
> >>  drivers/platform/x86/amd/pmf/sps.c       | 38 ++++++++++++------------
> >>  drivers/platform/x86/amd/pmf/tee-if.c    | 20 ++++++-------
> >>  7 files changed, 50 insertions(+), 47 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
> >> index a184922bba8d..faf15a8f74bb 100644
> >> --- a/drivers/platform/x86/amd/pmf/auto-mode.c
> >> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c
> >> @@ -114,14 +114,14 @@ static void amd_pmf_set_automode(struct amd_pmf_dev *dev, int idx,
> >>  {
> >>  	struct power_table_control *pwr_ctrl = &config_store.mode_set[idx].power_control;
> >>  
> >> -	amd_pmf_send_cmd(dev, SET_SPL, false, pwr_ctrl->spl, NULL);
> >> -	amd_pmf_send_cmd(dev, SET_FPPT, false, pwr_ctrl->fppt, NULL);
> >> -	amd_pmf_send_cmd(dev, SET_SPPT, false, pwr_ctrl->sppt, NULL);
> >> -	amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false, pwr_ctrl->sppt_apu_only, NULL);
> >> -	amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false, pwr_ctrl->stt_min, NULL);
> >> -	amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false,
> >> +	amd_pmf_send_cmd(dev, SET_SPL, SET_CMD, pwr_ctrl->spl, NULL);
> >> +	amd_pmf_send_cmd(dev, SET_FPPT, SET_CMD, pwr_ctrl->fppt, NULL);
> >> +	amd_pmf_send_cmd(dev, SET_SPPT, SET_CMD, pwr_ctrl->sppt, NULL);
> >> +	amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, SET_CMD, pwr_ctrl->sppt_apu_only, NULL);
> >> +	amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, SET_CMD, pwr_ctrl->stt_min, NULL);
> >> +	amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, SET_CMD,
> >>  			 fixp_q88_fromint(pwr_ctrl->stt_skin_temp[STT_TEMP_APU]), NULL);
> >> -	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
> >> +	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, SET_CMD,
> >>  			 fixp_q88_fromint(pwr_ctrl->stt_skin_temp[STT_TEMP_HS2]), NULL);
> >>  
> >>  	if (is_apmf_func_supported(dev, APMF_FUNC_SET_FAN_IDX))
> >> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
> >> index 207a0b33d8d3..5469fefb6001 100644
> >> --- a/drivers/platform/x86/amd/pmf/cnqf.c
> >> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
> >> @@ -76,14 +76,14 @@ static int amd_pmf_set_cnqf(struct amd_pmf_dev *dev, int src, int idx,
> >>  
> >>  	pc = &config_store.mode_set[src][idx].power_control;
> >>  
> >> -	amd_pmf_send_cmd(dev, SET_SPL, false, pc->spl, NULL);
> >> -	amd_pmf_send_cmd(dev, SET_FPPT, false, pc->fppt, NULL);
> >> -	amd_pmf_send_cmd(dev, SET_SPPT, false, pc->sppt, NULL);
> >> -	amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false, pc->sppt_apu_only, NULL);
> >> -	amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false, pc->stt_min, NULL);
> >> -	amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false,
> >> +	amd_pmf_send_cmd(dev, SET_SPL, SET_CMD, pc->spl, NULL);
> >> +	amd_pmf_send_cmd(dev, SET_FPPT, SET_CMD, pc->fppt, NULL);
> >> +	amd_pmf_send_cmd(dev, SET_SPPT, SET_CMD, pc->sppt, NULL);
> >> +	amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, SET_CMD, pc->sppt_apu_only, NULL);
> >> +	amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, SET_CMD, pc->stt_min, NULL);
> >> +	amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, SET_CMD,
> >>  			 fixp_q88_fromint(pc->stt_skin_temp[STT_TEMP_APU]), NULL);
> >> -	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
> >> +	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, SET_CMD,
> >>  			 fixp_q88_fromint(pc->stt_skin_temp[STT_TEMP_HS2]), NULL);
> >>  
> >>  	if (is_apmf_func_supported(dev, APMF_FUNC_SET_FAN_IDX))
> >> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> >> index bc544a4a5266..a659cedc4533 100644
> >> --- a/drivers/platform/x86/amd/pmf/core.c
> >> +++ b/drivers/platform/x86/amd/pmf/core.c
> >> @@ -131,7 +131,7 @@ static void amd_pmf_get_metrics(struct work_struct *work)
> >>  
> >>  	/* Transfer table contents */
> >>  	memset(dev->buf, 0, sizeof(dev->m_table));
> >> -	amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
> >> +	amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, SET_CMD, 7, NULL);
> >>  	memcpy(&dev->m_table, dev->buf, sizeof(dev->m_table));
> >>  
> >>  	time_elapsed_ms = ktime_to_ms(ktime_get()) - dev->start_time;
> >> @@ -289,8 +289,8 @@ int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev, bool alloc_buffer)
> >>  	hi = phys_addr >> 32;
> >>  	low = phys_addr & GENMASK(31, 0);
> >>  
> >> -	amd_pmf_send_cmd(dev, SET_DRAM_ADDR_HIGH, 0, hi, NULL);
> >> -	amd_pmf_send_cmd(dev, SET_DRAM_ADDR_LOW, 0, low, NULL);
> >> +	amd_pmf_send_cmd(dev, SET_DRAM_ADDR_HIGH, SET_CMD, hi, NULL);
> >> +	amd_pmf_send_cmd(dev, SET_DRAM_ADDR_LOW, SET_CMD, low, NULL);
> >>  
> >>  	return 0;
> >>  }
> >> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> >> index 6ea5380f3b23..19e413bd89bc 100644
> >> --- a/drivers/platform/x86/amd/pmf/pmf.h
> >> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> >> @@ -120,6 +120,9 @@ struct cookie_header {
> >>  #define APTS_MAX_STATES		16
> >>  #define CUSTOM_BIOS_INPUT_BITS	GENMASK(16, 7)
> >>  
> >> +#define SET_CMD		0
> >> +#define GET_CMD		1
> > 
> > amd_pmf_send_cmd() takes bool so I don't know why these are defined as 
> > numbers?
> 
> The signature of amd_pmf_send_cmd() is:
> 
> amd_pmf_send_cmd(struct amd_pmf_dev *dev, u8 message, bool get, u32
> arg, u32 *data);
> 
> However, in the code it’s called with mixed values for the get parameter:
> 
> amd_pmf_send_cmd(…, …, true, …, …);
> amd_pmf_send_cmd(…, …, false, …, …);
> amd_pmf_send_cmd(…, …, 0, …, …);
> 
> That inconsistency makes it harder to read. So, I think adding those
> two macros  would be useful so callers can use explicit names instead
> of 0/1/true/false, which should improve clarity.
> 
> If you don’t think this is worth doing, I’m fine dropping the change
> from this series.

No, no no. This is very good change, please don't drop it. I just didn't 
understand why those defines don't have:

/* amd_pmf_send_cmd() get/set */
#define SET_CMD           false
#define GET_CMD           true


-- 
 i.