On Mon, 03 Oct 2022 14:05:14 -0700, Andi Shyti wrote:
>
> Hi Badal,
>
> [...]
>
> > hwm_get_preregistration_info(struct drm_i915_private *i915)
> > {
> > struct i915_hwmon *hwmon = i915->hwmon;
> > + struct intel_uncore *uncore = &i915->uncore;
> > + intel_wakeref_t wakeref;
> > + u32 val_sku_unit;
> >
> > - if (IS_DG1(i915) || IS_DG2(i915))
> > + if (IS_DG1(i915) || IS_DG2(i915)) {
> > hwmon->rg.gt_perf_status = GEN12_RPSTAT1;
> > - else
> > + hwmon->rg.pkg_power_sku_unit = PCU_PACKAGE_POWER_SKU_UNIT;
> > + hwmon->rg.pkg_power_sku = PCU_PACKAGE_POWER_SKU;
> > + hwmon->rg.pkg_rapl_limit = PCU_PACKAGE_RAPL_LIMIT;
> > + } else {
> > hwmon->rg.gt_perf_status = INVALID_MMIO_REG;
> > + hwmon->rg.pkg_power_sku_unit = INVALID_MMIO_REG;
> > + hwmon->rg.pkg_power_sku = INVALID_MMIO_REG;
> > + hwmon->rg.pkg_rapl_limit = INVALID_MMIO_REG;
> > + }
> > +
> > + with_intel_runtime_pm(uncore->rpm, wakeref) {
> > + /*
> > + * The contents of register hwmon->rg.pkg_power_sku_unit do not
> > change,
> > + * so read it once and store the shift values.
> > + */
> > + if (i915_mmio_reg_valid(hwmon->rg.pkg_power_sku_unit)) {
> > + val_sku_unit = intel_uncore_read(uncore,
> > +
> > hwmon->rg.pkg_power_sku_unit);
> > + } else {
> > + val_sku_unit = 0;
> > + }
>
> please remove the brackets here and, just a small nitpick:
>
> move val_sky_unit inside the "with_intel_runtime_pm()" and
> initialize it to '0', you will save the else statement.
Hi Andi, fixed in v9 of the series.
>
> Other than that:
>
> Reviewed-by: Andi Shyti <andi.sh...@linux.intel.com>
Thanks.
--
Ashutosh
