Hi Badal,
[...]
> static int xe_file_open(struct drm_device *dev, struct drm_file *file)
> {
> @@ -328,6 +329,8 @@ int xe_device_probe(struct xe_device *xe)
>
> xe_debugfs_register(xe);
>
> + xe_hwmon_register(xe);
we don't have any information whe hwmon has been created or not.
Please return an error and print a warning if something wrong
happens and we fail to register the hwmon interface.
[...]
> +struct hwm_drvdata {
> + struct xe_hwmon *hwmon;
> + struct device *hwmon_dev;
> + char name[12];
> +};
> +
> +struct xe_hwmon {
> + struct hwm_drvdata ddat;
> + struct mutex hwmon_lock;
> +};
What's the reason for having two different structures here?
[...]
> +void xe_hwmon_register(struct xe_device *xe)
> +{
> + struct device *dev = xe->drm.dev;
> + struct xe_hwmon *hwmon;
> + struct device *hwmon_dev;
> + struct hwm_drvdata *ddat;
> +
> + /* hwmon is available only for dGfx */
> + if (!IS_DGFX(xe))
> + return;
this check is useless, we wouldn't be here otherwise.
[...]
> +void xe_hwmon_unregister(struct xe_device *xe)
> +{
> + xe->hwmon = NULL;
We could also destroy the mutex and unregister the hwmon?
[...]
> +#if IS_REACHABLE(CONFIG_HWMON)
> +void xe_hwmon_register(struct xe_device *xe);
> +void xe_hwmon_unregister(struct xe_device *xe);
> +#else
> +static inline void xe_hwmon_register(struct xe_device *xe) { };
> +static inline void xe_hwmon_unregister(struct xe_device *xe) { };
> +#endif
for readability we can leave some space between the
#if...#else...#endif and the content.
Andi
