Hi, > > > > +static int > > > > +hwm_temp_read(struct hwm_drvdata *ddat, u32 attr, long *val) { > > > > + struct i915_hwmon *hwmon = ddat->hwmon; > > > > + intel_wakeref_t wakeref; > > > > + u32 reg_val; > > > > + > > > > + switch (attr) { > > > > + case hwmon_temp_input: > > > > + with_intel_runtime_pm(ddat->uncore->rpm, wakeref) > > > > + reg_val = intel_uncore_read(ddat->uncore, hwmon- > > > > rg.pkg_temp); > > > > + > > > > + /* HW register value is in degrees, convert to millidegrees. */ > > > > + *val = REG_FIELD_GET(TEMP_MASK, reg_val) * > > > MILLIDEGREE_PER_DEGREE; > > > > + return 0; > > > > + default: > > > > + return -EOPNOTSUPP; > > > > + } > > > > > > I don't understand this love for single case switches. > > IMHO this is kept to keep symmetry in this file to make it more readable. > > Also it readable to return error using default case, which is followed in this entire file. > I agree on this. Let’s stick to file-wide approach and ensure it is applied > to the fan_input attribute as well. Yes, that's why I'm giving the r-b. I don't like it, but that's how you guys have decided to do it. Thanks, Andi