Re: [PATCH] drm/xe/hwmon: Add label support

["Poosa, Karthik" <karthik.poosa@intel.com>]

On 02-02-2024 17:22, Nilawar, Badal wrote:
>
>
> On 01-02-2024 23:28, Karthik Poosa wrote:
>> Add label for power and energy attributes.
>>
>> Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_hwmon.c | 24 ++++++++++++++++++++++--
>>   1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c 
>> b/drivers/gpu/drm/xe/xe_hwmon.c
>> index 89c6f7f84b5a..9fd70bff3435 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -403,10 +403,10 @@ static const struct attribute_group 
>> *hwmon_groups[] = {
>>   };
>>     static const struct hwmon_channel_info * const hwmon_info[] = {
>> -    HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | 
>> HWMON_P_CRIT),
>> +    HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | 
>> HWMON_P_CRIT | HWMON_P_LABEL),
> Is it not possible to expose label for each individual entry?
You mean like power1_max_label, power1_crit_label ?
Nope we can't do that, Each entry can have only 1 label, ex: power1_label.

>>       HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
>>       HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
>> -    HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
>> +    HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT | HWMON_E_LABEL),
>>       NULL
>>   };
>>   @@ -484,6 +484,8 @@ xe_hwmon_power_is_visible(struct xe_hwmon 
>> *hwmon, u32 attr, int chan)
>>       case hwmon_power_crit:
>>           return (xe_hwmon_pcode_read_i1(hwmon->gt, &uval) ||
>>               !(uval & POWER_SETUP_I1_WATTS)) ? 0 : 0644;
>> +    case hwmon_power_label:
>> +        return 0444;
>>       default:
>>           return 0;
>>       }
>> @@ -584,6 +586,8 @@ xe_hwmon_energy_is_visible(struct xe_hwmon 
>> *hwmon, u32 attr)
>>       switch (attr) {
>>       case hwmon_energy_input:
>>           return xe_hwmon_get_reg(hwmon, REG_PKG_ENERGY_STATUS) ? 
>> 0444 : 0;
>> +    case hwmon_energy_label:
>> +        return 0444;
>>       default:
>>           return 0;
>>       }
>> @@ -691,10 +695,26 @@ xe_hwmon_write(struct device *dev, enum 
>> hwmon_sensor_types type, u32 attr,
>>       return ret;
>>   }
>>   +static int xe_hwmon_read_label(struct device *dev,
>> +                   enum hwmon_sensor_types type,
>> +                   u32 attr, int channel, const char **str)
>> +{
>> +    switch (type) {
>> +    case hwmon_power:
>> +    case hwmon_energy:
>> +        *str = "card";
> This should be package instead of card. You may use 'pkg' here.
Okay.
>
> Regards,
> Badal
>> +        return 0;
>> +    default:
>> +        return -EOPNOTSUPP;
>> +    }
>> +}
>> +
>> +
>>   static const struct hwmon_ops hwmon_ops = {
>>       .is_visible = xe_hwmon_is_visible,
>>       .read = xe_hwmon_read,
>>       .write = xe_hwmon_write,
>> +    .read_string = xe_hwmon_read_label,
>>   };
>>     static const struct hwmon_chip_info hwmon_chip_info = {