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
