Re: [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

["Nilawar, Badal" <badal.nilawar@intel.com>]

Hi Guenter,
On 03-08-2023 04:42, Guenter Roeck wrote:
> On 8/2/23 15:40, Andi Shyti wrote:
>> Hi Badal,
>>
>> [...]
>>
>>> +struct xe_hwmon_data {
>>> +    struct device *hwmon_dev;
>>> +    struct xe_gt *gt;
>>> +    char name[12];
>>> +};
>>> +
>>> +struct xe_hwmon {
>>> +    struct xe_hwmon_data ddat;
>>> +    struct mutex hwmon_lock;
>>> +};
>>
>> why do we need two structures here? Can we merge them?
>>
> 
> A later patch adds multiple hwmon devices which makes use of it.
> I think that is flawed, and I am not inclined to accept it.
Is there any obvious reason that there shouldn't be multiple devices? In 
i915 we are doing the same. 
https://patchwork.freedesktop.org/patch/497324/?series=104278&rev=3

Regards,
Badal
> 
> Guenter
> 
>>> +static const struct hwmon_channel_info *hwmon_info[] = {
>>> +    NULL
>>> +};
>>
>> just:
>>
>>    static const struct hwmon_channel_info *hwmon_info[] = { };
>>
>> would do.
>>
>>> +static umode_t
>>> +hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
>>> +         u32 attr, int channel)
>>> +{
>>> +    struct xe_hwmon_data *ddat = (struct xe_hwmon_data *)drvdata;
>>> +    int ret;
>>> +
>>> +    xe_device_mem_access_get(gt_to_xe(ddat->gt));
>>> +
>>> +    switch (type) {
>>> +    default:
>>> +        ret = 0;
>>> +        break;
>>> +    }
>>> +
>>> +    xe_device_mem_access_put(gt_to_xe(ddat->gt));
>>> +
>>> +    return ret;
>>
>> OK... we are forced to go through the switch and initialize ret.
>> Would be nicer to initialize ret to '0'... but it's not
>> important, feel free to ignore this comment if the compiler
>> doesn't complain.
>>
>>> +}
>>
>> [...]
>>
>>> +    /*  hwmon_dev points to device hwmon<i> */
>>> +    hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
>>> +                             ddat,
>>> +                             &hwmon_chip_info,
>>> +                             NULL);
>>> +    if (IS_ERR(hwmon_dev)) {
>>> +        drm_warn(&xe->drm, "Fail to register xe hwmon, Err:%ld\n", 
>>> PTR_ERR(hwmon_dev));
>>
>> I think this is better:
>>
>>     drm_warn(&xe->drm, "Fail to register xe hwmon (%pe)\n", hwmon_dev);
>>
>>> +        xe->hwmon = NULL;
>>> +        return;
>>> +    }
>>> +
>>> +    ddat->hwmon_dev = hwmon_dev;
>>> +}
>>> +
>>> +void xe_hwmon_unregister(struct xe_device *xe)
>>> +{
>>> +    xe->hwmon = NULL;
>>
>> I think this is not necessary. Will xe check for hwmon at some
>> point?
>>
>> Andi
>>
>>> +}
>