On 03-08-2023 04:10, 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?
In my previous series I mentioned its require to hold multiple hwmon
devices.
+static const struct hwmon_channel_info *hwmon_info[] = {
+ NULL
+};
just:
static const struct hwmon_channel_info *hwmon_info[] = { };
would do.
sure
+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);
Sure
+ 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?
Yes this not needed as we are using devm_hwmon* function to register
hwmon but in i915 patches this was suggested for sanity. I will remove
this function.
Regards,
Badal
Andi
+}
