Re: [PATCH v2] drm/i915/hwmon: expose package temperature

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 10-09-2024 10:07, Gupta, Anshuman wrote:
> 
> 
>> -----Original Message-----
>> From: Andi Shyti <andi.shyti@kernel.org>
>> Sent: Tuesday, September 10, 2024 3:54 AM
>> To: Jadav, Raag <raag.jadav@intel.com>
>> Cc: jani.nikula@linux.intel.com; joonas.lahtinen@linux.intel.com; Vivi,
>> Rodrigo <rodrigo.vivi@intel.com>; tursulin@ursulin.net; linux@roeck-us.net;
>> andi.shyti@linux.intel.com; andriy.shevchenko@linux.intel.com; intel-
>> gfx@lists.freedesktop.org; linux-hwmon@vger.kernel.org; Gupta, Anshuman
>> <anshuman.gupta@intel.com>; Nilawar, Badal <badal.nilawar@intel.com>;
>> Tauro, Riana <riana.tauro@intel.com>; Dixit, Ashutosh
>> <ashutosh.dixit@intel.com>; Poosa, Karthik <karthik.poosa@intel.com>
>> Subject: Re: [PATCH v2] drm/i915/hwmon: expose package temperature
>>
>> Hi Raag,
>>
>> ...
>>
>>> +static int
>>> +hwm_temp_read(struct hwm_drvdata *ddat, u32 attr, long *val) {
>>> +	struct i915_hwmon *hwmon = ddat->hwmon;
>>> +	intel_wakeref_t wakeref;
>>> +	u32 reg_val;
>>> +
>>> +	switch (attr) {
>>> +	case hwmon_temp_input:
>>> +		with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
>>> +			reg_val = intel_uncore_read(ddat->uncore, hwmon-
>>> rg.pkg_temp);
>>> +
>>> +		/* HW register value is in degrees, convert to millidegrees. */
>>> +		*val = REG_FIELD_GET(TEMP_MASK, reg_val) *
>> MILLIDEGREE_PER_DEGREE;
>>> +		return 0;
>>> +	default:
>>> +		return -EOPNOTSUPP;
>>> +	}
>>
>> I don't understand this love for single case switches.
> IMHO this is kept to keep symmetry in this file to make it more readable.
> Also it readable to return error using default case, which is followed in this entire file.
I agree on this. Let’s stick to file-wide approach and ensure it is 
applied to the fan_input attribute as well.

Regards,
Badal

> Thanks,
> Anshuman.
>>
>> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
>>
>> Thanks,
>> Andi