Re: [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Wed, 21 Sep 2022 05:44:35 -0700, Andi Shyti wrote:
>
> > +void i915_hwmon_register(struct drm_i915_private *i915)
> > +{
> > +	struct device *dev = i915->drm.dev;
> > +	struct i915_hwmon *hwmon;
> > +	struct device *hwmon_dev;
> > +	struct hwm_drvdata *ddat;
> > +
> > +	/* hwmon is available only for dGfx */
> > +	if (!IS_DGFX(i915))
> > +		return;
> > +
> > +	hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL);
>
> why don't we use devm_kzalloc?
>
> > +	if (!hwmon)
> > +		return;
> > +
> > +	i915->hwmon = hwmon;
> > +	mutex_init(&hwmon->hwmon_lock);
> > +	ddat = &hwmon->ddat;
> > +
> > +	ddat->hwmon = hwmon;
> > +	ddat->uncore = &i915->uncore;
> > +	snprintf(ddat->name, sizeof(ddat->name), "i915");
> > +
> > +	hwm_get_preregistration_info(i915);
> > +
> > +	/*  hwmon_dev points to device hwmon<i> */
> > +	hwmon_dev = hwmon_device_register_with_info(dev, ddat->name,
> > +						    ddat,
> > +						    &hwm_chip_info,
> > +						    NULL);
> > +	if (IS_ERR(hwmon_dev)) {
> > +		mutex_destroy(&hwmon->hwmon_lock);
>
> there is not such a big need to destroy the mutex. Destroying
> mutexes is more useful when you actually are creating/destroying
> and there is some debug need. I don't think that's the case.
>
> With the devm_kzalloc this would be just a return.

If we are using devm_kzalloc we might as well replace all the
hwmon_device_register_with_info's (in Patch 1 and 7) with
devm_hwmon_device_register_with_info and then i915_hwmon_unregister is just
this:

void i915_hwmon_unregister(struct drm_i915_private *i915)
{
	fetch_and_zero(&i915->hwmon);
}

Even the above statement is probably not needed but might as well retain it
for sanity. So this is a simple change.

Thanks.
--
Ashutosh