[PATCH v2] platform/x86: amd: pmf: Fix STT limits

[PATCH v2] platform/x86: amd: pmf: Fix STT limits

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

On some platforms it has been observed that STT limits are not being applied
properly causing poor performance as power limits are set too low.

STT limits that are sent to the platform are supposed to be in Q8.8
format.  Convert them before sending.

Reported-by: Yijun Shen <Yijun.Shen@dell.com>
Fixes: 7c45534afa443 ("platform/x86/amd/pmf: Add support for PMF Policy Binary")
Cc: stable@vger.kernel.org
Tested-By: Yijun Shen <Yijun_Shen@Dell.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v2:
 * Handle cases for auto-mode, cnqf, and sps as well
---
 drivers/platform/x86/amd/pmf/auto-mode.c | 4 ++--
 drivers/platform/x86/amd/pmf/cnqf.c      | 4 ++--
 drivers/platform/x86/amd/pmf/sps.c       | 8 ++++----
 drivers/platform/x86/amd/pmf/tee-if.c    | 4 ++--
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
index 02ff68be10d01..df37f8a84a007 100644
--- a/drivers/platform/x86/amd/pmf/auto-mode.c
+++ b/drivers/platform/x86/amd/pmf/auto-mode.c
@@ -120,9 +120,9 @@ static void amd_pmf_set_automode(struct amd_pmf_dev *dev, int idx,
 	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,
-			 pwr_ctrl->stt_skin_temp[STT_TEMP_APU], NULL);
+			 pwr_ctrl->stt_skin_temp[STT_TEMP_APU] << 8, NULL);
 	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
-			 pwr_ctrl->stt_skin_temp[STT_TEMP_HS2], NULL);
+			 pwr_ctrl->stt_skin_temp[STT_TEMP_HS2] << 8, NULL);
 
 	if (is_apmf_func_supported(dev, APMF_FUNC_SET_FAN_IDX))
 		apmf_update_fan_idx(dev, config_store.mode_set[idx].fan_control.manual,
diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
index bc8899e15c914..6a5ecc05961d9 100644
--- a/drivers/platform/x86/amd/pmf/cnqf.c
+++ b/drivers/platform/x86/amd/pmf/cnqf.c
@@ -81,9 +81,9 @@ static int amd_pmf_set_cnqf(struct amd_pmf_dev *dev, int src, int idx,
 	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, pc->stt_skin_temp[STT_TEMP_APU],
+	amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, pc->stt_skin_temp[STT_TEMP_APU] << 8,
 			 NULL);
-	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, pc->stt_skin_temp[STT_TEMP_HS2],
+	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, pc->stt_skin_temp[STT_TEMP_HS2] << 8,
 			 NULL);
 
 	if (is_apmf_func_supported(dev, APMF_FUNC_SET_FAN_IDX))
diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
index d3083383f11fb..ec10db1bfa5ec 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -198,9 +198,9 @@ static void amd_pmf_update_slider_v2(struct amd_pmf_dev *dev, int idx)
 	amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false,
 			 apts_config_store.val[idx].stt_min_limit, NULL);
 	amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false,
-			 apts_config_store.val[idx].stt_skin_temp_limit_apu, NULL);
+			 apts_config_store.val[idx].stt_skin_temp_limit_apu << 8, NULL);
 	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
-			 apts_config_store.val[idx].stt_skin_temp_limit_hs2, NULL);
+			 apts_config_store.val[idx].stt_skin_temp_limit_hs2 << 8, NULL);
 }
 
 void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx,
@@ -217,9 +217,9 @@ void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx,
 		amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false,
 				 config_store.prop[src][idx].stt_min, NULL);
 		amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false,
-				 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_APU], NULL);
+				 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_APU] << 8, NULL);
 		amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
-				 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_HS2], NULL);
+				 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_HS2] << 8, NULL);
 	} else if (op == SLIDER_OP_GET) {
 		amd_pmf_send_cmd(dev, GET_SPL, true, ARG_NONE, &table->prop[src][idx].spl);
 		amd_pmf_send_cmd(dev, GET_FPPT, true, ARG_NONE, &table->prop[src][idx].fppt);
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index d6a871f0d8ff2..7096923107929 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -142,7 +142,7 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
 
 		case PMF_POLICY_STT_SKINTEMP_APU:
 			if (dev->prev_data->stt_skintemp_apu != val) {
-				amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, val, NULL);
+				amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, val << 8, NULL);
 				dev_dbg(dev->dev, "update STT_SKINTEMP_APU: %u\n", val);
 				dev->prev_data->stt_skintemp_apu = val;
 			}
@@ -150,7 +150,7 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
 
 		case PMF_POLICY_STT_SKINTEMP_HS2:
 			if (dev->prev_data->stt_skintemp_hs2 != val) {
-				amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, val, NULL);
+				amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, val << 8, NULL);
 				dev_dbg(dev->dev, "update STT_SKINTEMP_HS2: %u\n", val);
 				dev->prev_data->stt_skintemp_hs2 = val;
 			}
-- 
2.43.0

Re: [PATCH v2] platform/x86: amd: pmf: Fix STT limits

[Ilpo Järvinen]

On Mon, 7 Apr 2025, Mario Limonciello wrote:

> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> On some platforms it has been observed that STT limits are not being applied
> properly causing poor performance as power limits are set too low.
> 
> STT limits that are sent to the platform are supposed to be in Q8.8
> format.  Convert them before sending.
> 
> Reported-by: Yijun Shen <Yijun.Shen@dell.com>
> Fixes: 7c45534afa443 ("platform/x86/amd/pmf: Add support for PMF Policy Binary")
> Cc: stable@vger.kernel.org
> Tested-By: Yijun Shen <Yijun_Shen@Dell.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v2:
>  * Handle cases for auto-mode, cnqf, and sps as well
> ---
>  drivers/platform/x86/amd/pmf/auto-mode.c | 4 ++--
>  drivers/platform/x86/amd/pmf/cnqf.c      | 4 ++--
>  drivers/platform/x86/amd/pmf/sps.c       | 8 ++++----
>  drivers/platform/x86/amd/pmf/tee-if.c    | 4 ++--
>  4 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
> index 02ff68be10d01..df37f8a84a007 100644
> --- a/drivers/platform/x86/amd/pmf/auto-mode.c
> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c
> @@ -120,9 +120,9 @@ static void amd_pmf_set_automode(struct amd_pmf_dev *dev, int idx,
>  	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,
> -			 pwr_ctrl->stt_skin_temp[STT_TEMP_APU], NULL);
> +			 pwr_ctrl->stt_skin_temp[STT_TEMP_APU] << 8, NULL);

Hi Mario,

Could we add some helper on constructing the fixed-point number from the 
integer part as this magic shifting makes the intent somewhat harder to 
follow just by reading the code itself?

I hoped that include/linux/ would have had something for this but it seems 
generic fixed-point helpers are almost non-existing except for very 
specific use cases such as averages so maybe add a helper only for this 
driver for now as this will be routed through fixes branch so doing random 
things i include/linux/ might not be preferrable and would require larger 
review audience.

What I mean for general helpers is that it would be nice to have something 
like DECLARE_FIXEDPOINT() similar to DECLARE_EWMA() macro (and maybe a 
signed variant too) which creates a few helper functions for the given 
name prefix. It seems there's plenty of code which would benefit from such 
helpers and would avoid the need to comment the fixed-point operations 
(not to speak of how many of such ops likely lack the comment). So at 
least keep that in mind for naming the helpers so the conversion to
a generic helper could be done smoothly.

-- 
 i.

>  	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
> -			 pwr_ctrl->stt_skin_temp[STT_TEMP_HS2], NULL);
> +			 pwr_ctrl->stt_skin_temp[STT_TEMP_HS2] << 8, NULL);
>  
>  	if (is_apmf_func_supported(dev, APMF_FUNC_SET_FAN_IDX))
>  		apmf_update_fan_idx(dev, config_store.mode_set[idx].fan_control.manual,
> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
> index bc8899e15c914..6a5ecc05961d9 100644
> --- a/drivers/platform/x86/amd/pmf/cnqf.c
> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
> @@ -81,9 +81,9 @@ static int amd_pmf_set_cnqf(struct amd_pmf_dev *dev, int src, int idx,
>  	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, pc->stt_skin_temp[STT_TEMP_APU],
> +	amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, pc->stt_skin_temp[STT_TEMP_APU] << 8,
>  			 NULL);
> -	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, pc->stt_skin_temp[STT_TEMP_HS2],
> +	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, pc->stt_skin_temp[STT_TEMP_HS2] << 8,
>  			 NULL);
>  
>  	if (is_apmf_func_supported(dev, APMF_FUNC_SET_FAN_IDX))
> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> index d3083383f11fb..ec10db1bfa5ec 100644
> --- a/drivers/platform/x86/amd/pmf/sps.c
> +++ b/drivers/platform/x86/amd/pmf/sps.c
> @@ -198,9 +198,9 @@ static void amd_pmf_update_slider_v2(struct amd_pmf_dev *dev, int idx)
>  	amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false,
>  			 apts_config_store.val[idx].stt_min_limit, NULL);
>  	amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false,
> -			 apts_config_store.val[idx].stt_skin_temp_limit_apu, NULL);
> +			 apts_config_store.val[idx].stt_skin_temp_limit_apu << 8, NULL);
>  	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
> -			 apts_config_store.val[idx].stt_skin_temp_limit_hs2, NULL);
> +			 apts_config_store.val[idx].stt_skin_temp_limit_hs2 << 8, NULL);
>  }
>  
>  void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx,
> @@ -217,9 +217,9 @@ void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx,
>  		amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false,
>  				 config_store.prop[src][idx].stt_min, NULL);
>  		amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false,
> -				 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_APU], NULL);
> +				 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_APU] << 8, NULL);
>  		amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
> -				 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_HS2], NULL);
> +				 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_HS2] << 8, NULL);
>  	} else if (op == SLIDER_OP_GET) {
>  		amd_pmf_send_cmd(dev, GET_SPL, true, ARG_NONE, &table->prop[src][idx].spl);
>  		amd_pmf_send_cmd(dev, GET_FPPT, true, ARG_NONE, &table->prop[src][idx].fppt);
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index d6a871f0d8ff2..7096923107929 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -142,7 +142,7 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
>  
>  		case PMF_POLICY_STT_SKINTEMP_APU:
>  			if (dev->prev_data->stt_skintemp_apu != val) {
> -				amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, val, NULL);
> +				amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, val << 8, NULL);
>  				dev_dbg(dev->dev, "update STT_SKINTEMP_APU: %u\n", val);
>  				dev->prev_data->stt_skintemp_apu = val;
>  			}
> @@ -150,7 +150,7 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
>  
>  		case PMF_POLICY_STT_SKINTEMP_HS2:
>  			if (dev->prev_data->stt_skintemp_hs2 != val) {
> -				amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, val, NULL);
> +				amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, val << 8, NULL);
>  				dev_dbg(dev->dev, "update STT_SKINTEMP_HS2: %u\n", val);
>  				dev->prev_data->stt_skintemp_hs2 = val;
>  			}
> 

Re: [PATCH v2] platform/x86: amd: pmf: Fix STT limits

[Mario Limonciello]

On 4/7/2025 10:19 AM, Ilpo Järvinen wrote:
> On Mon, 7 Apr 2025, Mario Limonciello wrote:
> 
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> On some platforms it has been observed that STT limits are not being applied
>> properly causing poor performance as power limits are set too low.
>>
>> STT limits that are sent to the platform are supposed to be in Q8.8
>> format.  Convert them before sending.
>>
>> Reported-by: Yijun Shen <Yijun.Shen@dell.com>
>> Fixes: 7c45534afa443 ("platform/x86/amd/pmf: Add support for PMF Policy Binary")
>> Cc: stable@vger.kernel.org
>> Tested-By: Yijun Shen <Yijun_Shen@Dell.com>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> v2:
>>   * Handle cases for auto-mode, cnqf, and sps as well
>> ---
>>   drivers/platform/x86/amd/pmf/auto-mode.c | 4 ++--
>>   drivers/platform/x86/amd/pmf/cnqf.c      | 4 ++--
>>   drivers/platform/x86/amd/pmf/sps.c       | 8 ++++----
>>   drivers/platform/x86/amd/pmf/tee-if.c    | 4 ++--
>>   4 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
>> index 02ff68be10d01..df37f8a84a007 100644
>> --- a/drivers/platform/x86/amd/pmf/auto-mode.c
>> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c
>> @@ -120,9 +120,9 @@ static void amd_pmf_set_automode(struct amd_pmf_dev *dev, int idx,
>>   	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,
>> -			 pwr_ctrl->stt_skin_temp[STT_TEMP_APU], NULL);
>> +			 pwr_ctrl->stt_skin_temp[STT_TEMP_APU] << 8, NULL);
> 
> Hi Mario,
> 
> Could we add some helper on constructing the fixed-point number from the
> integer part as this magic shifting makes the intent somewhat harder to
> follow just by reading the code itself?
> 
> I hoped that include/linux/ would have had something for this but it seems
> generic fixed-point helpers are almost non-existing except for very
> specific use cases such as averages so maybe add a helper only for this
> driver for now as this will be routed through fixes branch so doing random
> things i include/linux/ might not be preferrable and would require larger
> review audience.
> 
> What I mean for general helpers is that it would be nice to have something
> like DECLARE_FIXEDPOINT() similar to DECLARE_EWMA() macro (and maybe a
> signed variant too) which creates a few helper functions for the given
> name prefix. It seems there's plenty of code which would benefit from such
> helpers and would avoid the need to comment the fixed-point operations
> (not to speak of how many of such ops likely lack the comment). So at
> least keep that in mind for naming the helpers so the conversion to
> a generic helper could be done smoothly.
> 

Do I follow right that you mean something like this?

static inline u32 amd_pmf_convert_q88 (u32 val)
{
	return val << 8;
}

Re: [PATCH v2] platform/x86: amd: pmf: Fix STT limits

[Ilpo Järvinen]

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

On Mon, 7 Apr 2025, Mario Limonciello wrote:

> On 4/7/2025 10:19 AM, Ilpo Järvinen wrote:
> > On Mon, 7 Apr 2025, Mario Limonciello wrote:
> > 
> > > From: Mario Limonciello <mario.limonciello@amd.com>
> > > 
> > > On some platforms it has been observed that STT limits are not being
> > > applied
> > > properly causing poor performance as power limits are set too low.
> > > 
> > > STT limits that are sent to the platform are supposed to be in Q8.8
> > > format.  Convert them before sending.
> > > 
> > > Reported-by: Yijun Shen <Yijun.Shen@dell.com>
> > > Fixes: 7c45534afa443 ("platform/x86/amd/pmf: Add support for PMF Policy
> > > Binary")
> > > Cc: stable@vger.kernel.org
> > > Tested-By: Yijun Shen <Yijun_Shen@Dell.com>
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > > v2:
> > >   * Handle cases for auto-mode, cnqf, and sps as well
> > > ---
> > >   drivers/platform/x86/amd/pmf/auto-mode.c | 4 ++--
> > >   drivers/platform/x86/amd/pmf/cnqf.c      | 4 ++--
> > >   drivers/platform/x86/amd/pmf/sps.c       | 8 ++++----
> > >   drivers/platform/x86/amd/pmf/tee-if.c    | 4 ++--
> > >   4 files changed, 10 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c
> > > b/drivers/platform/x86/amd/pmf/auto-mode.c
> > > index 02ff68be10d01..df37f8a84a007 100644
> > > --- a/drivers/platform/x86/amd/pmf/auto-mode.c
> > > +++ b/drivers/platform/x86/amd/pmf/auto-mode.c
> > > @@ -120,9 +120,9 @@ static void amd_pmf_set_automode(struct amd_pmf_dev
> > > *dev, int idx,
> > >   	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,
> > > -			 pwr_ctrl->stt_skin_temp[STT_TEMP_APU], NULL);
> > > +			 pwr_ctrl->stt_skin_temp[STT_TEMP_APU] << 8, NULL);
> > 
> > Hi Mario,
> > 
> > Could we add some helper on constructing the fixed-point number from the
> > integer part as this magic shifting makes the intent somewhat harder to
> > follow just by reading the code itself?
> > 
> > I hoped that include/linux/ would have had something for this but it seems
> > generic fixed-point helpers are almost non-existing except for very
> > specific use cases such as averages so maybe add a helper only for this
> > driver for now as this will be routed through fixes branch so doing random
> > things i include/linux/ might not be preferrable and would require larger
> > review audience.
> > 
> > What I mean for general helpers is that it would be nice to have something
> > like DECLARE_FIXEDPOINT() similar to DECLARE_EWMA() macro (and maybe a
> > signed variant too) which creates a few helper functions for the given
> > name prefix. It seems there's plenty of code which would benefit from such
> > helpers and would avoid the need to comment the fixed-point operations
> > (not to speak of how many of such ops likely lack the comment). So at
> > least keep that in mind for naming the helpers so the conversion to
> > a generic helper could be done smoothly.
> > 
> 
> Do I follow right that you mean something like this?
> 
> static inline u32 amd_pmf_convert_q88 (u32 val)

As with the ewma example, the operation should be the last part. And we'd 
probably want to have some common prefix for all those to make it obvious 
it's fixed-point related, so lets say e.g.:

fixp_amd_pmf_q88_from_integer()

I'm not entirely sure though if we really need per driver in the prefix at 
all as fixed-points are more general concept than a single driver/hw. So 
if it's only used for temperature, maybe just fixp_temp_q88_from_integer() 
or even just fixp_q88_from_integer(), Q8.8 should really be the same for 
all users, shouldn't it, so the last one would seem okay too to me 
(although I'm not sure what people in general will think of that).

I suspect ..._from_int() isn't good name for operation because "int" is a 
type in C but it would be shorted than from_integer.

> {
> 	return val << 8;
> }
> 
> 

-- 
 i.