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

[Riana Tauro <riana.tauro@intel.com>]

Hi Karthik

On 2/6/2024 11:17 AM, Poosa, Karthik wrote:
> 
> 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.
add why label is added
>>>
>>> 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;
This should be visible only if any of the energy entries are visible.
Same for power.

Thanks
Riana
>>>       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 = {