Re: [PATCH v5 2/6] drm/xe/hwmon: Expose power attributes

[Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>]

Hi Badal,

[...]

> +static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, long value)
> +{
> +	u32 reg_val;
> +
> +	/* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
> +	if (value == PL1_DISABLE) {
> +		xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, &reg_val,
> +				     PKG_PWR_LIM_1_EN, 0);
> +		xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, &reg_val,
> +				     PKG_PWR_LIM_1_EN, 0);
> +
> +		if (reg_val & PKG_PWR_LIM_1_EN)
> +			return -ENODEV;

so, here you are trying to disable PL1 and check then if it's
disabled. Shall we try at least twice before returning error?

And why ENODEV? It might be that we failed to write on the
register but it doesn't mean that the device is wrong.

> +	}
> +
> +	/* Computation in 64-bits to avoid overflow. Round to nearest. */
> +	reg_val = DIV_ROUND_CLOSEST_ULL((u64)value << hwmon->scl_shift_power, SF_POWER);
> +	reg_val = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, reg_val);
> +
> +	xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, &reg_val,
> +			     PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, reg_val);
> +
> +	return 0;
> +}

[...]

> +	/*  hwmon_dev points to device hwmon<i> */
> +	hwmon->hwmon_dev = devm_hwmon_device_register_with_info(dev,
> +								"xe",
> +								hwmon,
> +								&hwmon_chip_info,
> +								NULL);

here the allignment is a bit fancy... in this cases I wouldn't
mind going up to 100 characters or not align under the bracket.

I would write it like this

	hwmon->hwmon_dev = devm_hwmon_device_register_with_info(dev,
					"xe", hwmon, &hwmon_chip_info, NULL);

but, of course, it's a matter of taste. Up to you.

Andi