On 29-08-2022 23:00, Dixit, Ashutosh wrote:
On Thu, 25 Aug 2022 06:21:13 -0700, Badal Nilawar wrote:

From: Riana Tauro <[email protected]>

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 <[email protected]>

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 */

Reply via email to