[Nouveau] [PATCH 3/3] hwmon: expose power_max and power_crit

[Martin Peres]

On 25/10/16 00:11, Karol Herbst wrote:
> Signed-off-by: Karol Herbst <karolherbst at gmail.com>
> ---
>  drm/nouveau/nouveau_hwmon.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>
> diff --git a/drm/nouveau/nouveau_hwmon.c b/drm/nouveau/nouveau_hwmon.c
> index 71f764b..3d4672a 100644
> --- a/drm/nouveau/nouveau_hwmon.c
> +++ b/drm/nouveau/nouveau_hwmon.c
> @@ -596,6 +596,32 @@ nouveau_hwmon_get_power1_input(struct device *d, struct device_attribute *a,
>  static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO,
>  			  nouveau_hwmon_get_power1_input, NULL, 0);
>
> +static ssize_t
> +nouveau_hwmon_get_power1_max(struct device *d, struct device_attribute *a,
> +			     char *buf)
> +{
> +	struct drm_device *dev = dev_get_drvdata(d);
> +	struct nouveau_drm *drm = nouveau_drm(dev);
> +	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->device);
> +	return sprintf(buf, "%i\n", iccsense->power_w_max);
> +}
> +
> +static SENSOR_DEVICE_ATTR(power1_max, S_IRUGO,
> +			  nouveau_hwmon_get_power1_max, NULL, 0);
> +
> +static ssize_t
> +nouveau_hwmon_get_power1_crit(struct device *d, struct device_attribute *a,
> +			      char *buf)
> +{
> +	struct drm_device *dev = dev_get_drvdata(d);
> +	struct nouveau_drm *drm = nouveau_drm(dev);
> +	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->device);
> +	return sprintf(buf, "%i\n", iccsense->power_w_crit);
> +}
> +
> +static SENSOR_DEVICE_ATTR(power1_crit, S_IRUGO,
> +			  nouveau_hwmon_get_power1_crit, NULL, 0);
> +
>  static struct attribute *hwmon_default_attributes[] = {
>  	&sensor_dev_attr_name.dev_attr.attr,
>  	&sensor_dev_attr_update_rate.dev_attr.attr,
> @@ -639,6 +665,12 @@ static struct attribute *hwmon_power_attributes[] = {
>  	NULL
>  };
>
> +static struct attribute *hwmon_power_caps_attributes[] = {
> +	&sensor_dev_attr_power1_max.dev_attr.attr,
> +	&sensor_dev_attr_power1_crit.dev_attr.attr,
> +	NULL
> +};
> +
>  static const struct attribute_group hwmon_default_attrgroup = {
>  	.attrs = hwmon_default_attributes,
>  };
> @@ -657,6 +689,9 @@ static const struct attribute_group hwmon_in0_attrgroup = {
>  static const struct attribute_group hwmon_power_attrgroup = {
>  	.attrs = hwmon_power_attributes,
>  };
> +static const struct attribute_group hwmon_power_caps_attrgroup = {
> +	.attrs = hwmon_power_caps_attributes,
> +};
>  #endif
>
>  int
> @@ -728,8 +763,16 @@ nouveau_hwmon_init(struct drm_device *dev)
>  	if (iccsense && iccsense->data_valid && !list_empty(&iccsense->rails)) {
>  		ret = sysfs_create_group(&hwmon_dev->kobj,
>  					 &hwmon_power_attrgroup);
> +
>  		if (ret)
>  			goto error;
> +
> +		if (iccsense->power_w_max && iccsense->power_w_crit) {
> +			ret = sysfs_create_group(&hwmon_dev->kobj,
> +						 &hwmon_power_caps_attrgroup);
> +			if (ret)
> +				goto error;
> +		}
>  	}
>
>  	hwmon->hwmon = hwmon_dev;
> @@ -759,6 +802,7 @@ nouveau_hwmon_fini(struct drm_device *dev)
>  		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_fan_rpm_attrgroup);
>  		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_in0_attrgroup);
>  		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_power_attrgroup);
> +		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_power_caps_attrgroup);
>
>  		hwmon_device_unregister(hwmon->hwmon);
>  	}
>

So, you wanted to make a cache because you felt you may want to let the 
user edit the values?

Not sure we should ever allow this :s At least, a RO file is a very good 
idea to help debugging and just letting the user know about such 
limitations.

So, in my opinion, I think we should just kill patch 2 and use directly 
the output of the bios table here. Maybe you can write a helper function 
in patch 1 to get the min, avg and max values and return -1 if it is not 
valid? This way, you can access this info very easily in multiple places 
without duplicating this 0xff check.