On Mon, 12 Sep 2022 07:09:28 -0700, Gupta, Anshuman wrote: > > > +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: > > Other attributes in this series take hwmon->lock before accessing i915 > registers , So do we need lock here as well ? The lock is being taken only for RMW and for making sure energy counter updates happen atomically. We are not taking the lock for just reads so IMO no lock is needed here. > > + 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); > > + return 0; > > + default: > > + return -EOPNOTSUPP; > > + } > > +} Thanks. -- Ashutosh