Re: [PATCH v2 3/6] drm/xe/hwmon: Expose card reactive critical power

[Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>]

Hi Badal,

> Expose the card reactive critical (I1) power. I1 is exposed as
> power1_crit in microwatts (typically for client products) or as
> curr1_crit in milliamperes (typically for server).
> This is port from i915 hwmon.

Should this, then be a more generic framework for more gpu
drivers? Now we are having some code duplication.

[...]

>  hwm_power_is_visible(struct hwm_drvdata *ddat, u32 attr, int chan)
>  {
>  	u32 reg_val;
> +	u32 uval;
>  
>  	switch (attr) {
>  	case hwmon_power_max:
> @@ -248,6 +274,9 @@ hwm_power_is_visible(struct hwm_drvdata *ddat, u32 attr, int chan)
>  	case hwmon_power_rated_max:
>  		return process_hwmon_reg(ddat, pkg_power_sku,
>  					 reg_read, &reg_val, 0, 0) ? 0 : 0444;
> +	case hwmon_power_crit:
> +		return (hwm_pcode_read_i1(ddat->gt, &uval) ||
> +			!(uval & POWER_SETUP_I1_WATTS)) ? 0 : 0644;

I like better the form below, with

	err = hwm_pcode_read...()
	if (!err)
		return 0;
	
	return !(uval & ....

>  	default:
>  		return 0;

[...]

> +static umode_t
> +hwm_curr_is_visible(const struct hwm_drvdata *ddat, u32 attr)
> +{
> +	u32 uval;
> +
> +	switch (attr) {
> +	case hwmon_curr_crit:
> +		return (hwm_pcode_read_i1(ddat->gt, &uval) ||
> +			(uval & POWER_SETUP_I1_WATTS)) ? 0 : 0644;

this is a pattern that is repeated quite often, should this be a
separate function, maybe?

Andi

[...]