On 4/10/20 9:49 PM, Naveen Krishna Ch wrote: > Hi, > > Thank you for taking time to review and provide feedback. > If you can kindly address a few questions, i would start addressing > your comments and submit another version. > [ ... ] >>> + >>> +static ssize_t amd_core_u64_show(struct device *dev, >>> + struct device_attribute *dev_attr, char *buffer) >>> +{ >>> + unsigned long long value; >>> + struct sensor_data *sensor; >>> + int ret = 0; >>> + >>> + sensor = container_of(dev_attr, struct sensor_data, dev_attr_input); >>> + ret = rdmsrl_safe_on_cpu(sensor->cpu_nr, ENERGY_CORE_MSR, &value); >>> + if (ret) >>> + return -EAGAIN; >>> + >>> + return snprintf(buffer, 24, "%llu\n", value); >> >> It seems to me this reports raw values. This is unacceptable. Values need >> to be scaled to match the ABI. Energy is reported in microJoule. > I was of the opinion that driver provides the raw values and user-space (can use > float/double) will be able to scale the value, which would involve > calculation of > "1/2^ESU * RAW value" > Please see Documentation/hwmon/sysfs-interface.rst. Sure, there is /etc/sensors3.conf, but that only applies if the kernel driver can not provide scaled values. This is not the case here. FWIW, I implemented prototype code for this some time ago as extension of the k10temp driver, and the arithmetic wasn't that difficult. Guenter