Re: [Intel-gfx] [PATCH 1/3] drm/i915/hwmon: Replace hwm_field_scale_and_write with hwm_power_max_write

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Feb 13, 2023 at 01:00:47PM -0800, Ashutosh Dixit wrote:
> hwm_field_scale_and_write has a single caller hwm_power_write and is
> specific to hwm_power_write but makes it appear that it is a general
> function which can have multiple callers. Replace the function with
> hwm_power_max_write which is specific to hwm_power_write and use that in
> future patches where the function needs to be extended.
> 
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_hwmon.c | 36 ++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> index 1225bc432f0d5..85195d61f89c7 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -99,20 +99,6 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat, i915_reg_t rgadr,
>  	return mul_u64_u32_shr(reg_value, scale_factor, nshift);
>  }
>  
> -static void
> -hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> -			  int nshift, unsigned int scale_factor, long lval)
> -{
> -	u32 nval;
> -
> -	/* Computation in 64-bits to avoid overflow. Round to nearest. */
> -	nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
> -
> -	hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> -					    PKG_PWR_LIM_1,
> -					    REG_FIELD_PREP(PKG_PWR_LIM_1, nval));
> -}
> -
>  /*
>   * hwm_energy - Obtain energy value
>   *
> @@ -391,6 +377,21 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val)
>  	return 0;
>  }
>  
> +static int
> +hwm_power_max_write(struct hwm_drvdata *ddat, long val)
 +{
> +	struct i915_hwmon *hwmon = ddat->hwmon;
> +	u32 nval;
> +
> +	/* Computation in 64-bits to avoid overflow. Round to nearest. */
> +	nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER);
> +
> +	hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit,
> +					    PKG_PWR_LIM_1,
> +					    REG_FIELD_PREP(PKG_PWR_LIM_1, nval));
> +	return 0;

Let's keep this function as void and the return 0 in the previous spot.
With that change:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +}
> +
>  static int
>  hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
>  {
> @@ -425,16 +426,11 @@ hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
>  static int
>  hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val)
>  {
> -	struct i915_hwmon *hwmon = ddat->hwmon;
>  	u32 uval;
>  
>  	switch (attr) {
>  	case hwmon_power_max:
> -		hwm_field_scale_and_write(ddat,
> -					  hwmon->rg.pkg_rapl_limit,
> -					  hwmon->scl_shift_power,
> -					  SF_POWER, val);
> -		return 0;
> +		return hwm_power_max_write(ddat, val);
>  	case hwmon_power_crit:
>  		uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT, SF_POWER);
>  		return hwm_pcode_write_i1(ddat->uncore->i915, uval);
> -- 
> 2.38.0
>