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

[Andi Shyti]

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?

> +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

> +}