Re: [Intel-gfx] [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support

From: "Nilawar, Badal" <badal.nilawar@intel.com>
To: "Dixit, Ashutosh" <ashutosh.dixit@intel.com>
Cc: linux-hwmon@vger.kernel.org, intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support
Date: Thu, 15 Sep 2022 20:10:37 +0530	[thread overview]
Message-ID: <9760a03d-a83e-7e27-e4da-faff02c6f11a@intel.com> (raw)
In-Reply-To: <87o7w3yn28.wl-ashutosh.dixit@intel.com>


On 29-08-2022 23:00, Dixit, Ashutosh wrote:
> On Thu, 25 Aug 2022 06:21:13 -0700, Badal Nilawar wrote:
>>
>> From: Riana Tauro <riana.tauro@intel.com>
>>
>> Use i915 HWMON subsystem to display current input voltage.
> 
> A couple of suggestions to improve comments in this patch below and after
> addressing those this patch is:
> 
> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> 
>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
>> index 103dd543a214..2192d0fd4c66 100644
>> --- a/drivers/gpu/drm/i915/i915_hwmon.c
>> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
>> @@ -11,8 +11,10 @@
>>   #include "i915_hwmon.h"
>>   #include "i915_reg.h"
>>   #include "intel_mchbar_regs.h"
>> +#include "gt/intel_gt_regs.h"
> 
> In later patches we have added units for different quantities here. So I
> think we should add those units for voltage to this patch too. It's in
> Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon but I think it's
> better to add to this file too otherwise if anyone looks at it is seems to
> be missing.
> 
> So I would add the following to this patch:
> 
> /*
>   * SF_* - scale factors for particular quantities according to hwmon spec.
>   * - voltage - millivolts
>   */
Sure I will add above comment.
> #define SF_VOLTAGE	1000
we are not using above scale factor for voltage. As our scale factor is 
2.5 millivolts shall I add like.
#define SF_VOLTAGE_MUL 25
#define SF_VOLTAGE_DIV 10
> 
>> +static int
>> +hwm_in_read(struct hwm_drvdata *ddat, u32 attr, long *val)
>> +{
>> +	struct i915_hwmon *hwmon = ddat->hwmon;
>> +	intel_wakeref_t wakeref;
>> +	u32 reg_value;
>> +
>> +	switch (attr) {
>> +	case hwmon_in_input:
>> +		with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
>> +			reg_value = intel_uncore_read(ddat->uncore, hwmon->rg.gt_perf_status);
>> +		/* In units of 2.5 millivolt */
>> +		*val = DIV_ROUND_CLOSEST(REG_FIELD_GET(GEN12_VOLTAGE_MASK, reg_value) * 25, 10);
		
And use above scale factors here.
*val = DIV_ROUND_CLOSEST(REG_FIELD_GET(GEN12_VOLTAGE_MASK, reg_value) * 
SF_VOLTAGE_MUL, SF_VOLTAGE_DIV);
Regards,
Badal
> 
> Let's complete this comment to so that it is clear what's happening:
> 
> 		/* HW register value is in units of 2.5 millivolt */