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

[Poosa, Karthik]

On 06-02-2024 12:43, Riana Tauro wrote:
> 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

1. To know if a channel attribute is for pkg or card. This will be 
needed for future platforms.

>>>> Signed-off-by: Karthik Poosa <karthik.poosa at 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
Seems right. I'll get back on this.
>>>>       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 = {