Re: [Intel-xe] [PATCH v2 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Andi Shyti <andi.shyti@linux.intel.com>]

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