Hi Raag,
> +static umode_t
> +hwm_fan_is_visible(const struct hwm_drvdata *ddat, u32 attr)
> +{
> + struct i915_hwmon *hwmon = ddat->hwmon;
> +
> + switch (attr) {
> + case hwmon_fan_input:
> + return i915_mmio_reg_valid(hwmon->rg.fan_speed) ? 0444 : 0;
> + default:
> + return 0;
> + }
Why do we need switch case here?
Why can't this function become a single "return " line?
> +}
> +
> +static int
> +hwm_fan_read(struct hwm_drvdata *ddat, u32 attr, long *val)
> +{
> + struct i915_hwmon *hwmon = ddat->hwmon;
> + struct hwm_fan_info *fi = &ddat->fi;
> + u32 reg_val, pulses, time, time_now;
> + intel_wakeref_t wakeref;
> + long rotations;
> + int ret = 0;
> +
> + switch (attr) {
> + case hwmon_fan_input:
> + with_intel_runtime_pm(ddat->uncore->rpm, wakeref) {
> + mutex_lock(&hwmon->hwmon_lock);
> +
> + reg_val = intel_uncore_read(ddat->uncore,
> hwmon->rg.fan_speed);
> + time_now = jiffies_to_msecs(jiffies);
> +
> + /* Handle overflow */
> + if (reg_val >= fi->reg_val_prev)
> + pulses = reg_val - fi->reg_val_prev;
> + else
> + pulses = UINT_MAX - fi->reg_val_prev + reg_val;
> +
> + /*
> + * HW register value is accumulated count of pulses from
> + * PWM fan with the scale of 2 pulses per rotation.
> + */
> + rotations = pulses >> 1;
> + time = time_now - fi->time_prev;
> +
> + if (unlikely(!time)) {
> + ret = -EAGAIN;
> + mutex_unlock(&hwmon->hwmon_lock);
> + break;
> + }
> +
> + /* Convert to minutes for calculating RPM */
> + *val = DIV_ROUND_UP(rotations * (60 * MSEC_PER_SEC),
> time);
> +
> + fi->reg_val_prev = reg_val;
> + fi->time_prev = time_now;
> +
> + mutex_unlock(&hwmon->hwmon_lock);
> + }
> + return ret;
> + default:
> + return -EOPNOTSUPP;
> + }
same here, can we make this function:
if (attr != hwmon_fan_input)
return -EOPNOTSUPP;
and then save all the indentation.
Are we expecting more cases here?
Andi
