Hi Andi,
On 22-09-2023 22:54, Andi Shyti wrote:
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, ®_val,
+ PKG_PWR_LIM_1_EN, 0);
+ xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, ®_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?
I think lets keep write once only.
And why ENODEV? It might be that we failed to write on the
register but it doesn't mean that the device is wrong.
Will return ENOSUPP.
+ }
+
+ /* 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, ®_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.
Not sure why this is fancy. Above suggestion checkpatch --strict throws
CHECK: Alignment should match open parenthesis.
If I write like this checkpatch goes fine.
hwmon->hwmon_dev = devm_hwmon_device_register_with_info(dev, "xe",
hwmon,
&hwmon_chip_info,
NULL);
Regards,
Badal
Andi
