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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 25 Aug 2022 06:21:13 -0700, Badal Nilawar wrote:
>
> From: Riana Tauro <riana.tauro@xxxxxxxxx>
>
> 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@xxxxxxxxx>

> 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
 */
#define SF_VOLTAGE	1000

> +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);

Let's complete this comment to so that it is clear what's happening:

		/* HW register value is in units of 2.5 millivolt */



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux