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

["Nilawar, Badal" <badal.nilawar@intel.com>]

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, &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?
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, &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.
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