[PATCH v2 0/5] replace hwmon_device_register for hwmon_device_register_with_info

[PATCH v2 0/5] replace hwmon_device_register for hwmon_device_register_with_info

[Oscar Salvador]

Hi!

This patchseries replaces the deprecated hwmon_device_register function with the
new one hwmon_device_register_with_info.
It also does some cleanup.

Here is the list of patches and what they do:

1/ Adds config structures for all sensors and their possible settings. This patch
   and the next one are just preparing the code for what it comes.

2/ Now everything goes through nouveua_hwmon_ops. There we set up wich functions will
   be called for read/write and for checking if an attribute should be created or not.
   In this patch we just add .is_visible/.read_string operations.
   Here we sill don't interactuate with the old code.

3/ Most of the work is being done here. We remove the old code and replace it with
   the new one related to the new api.
   Here we introduce the .write and .read operations, and create all functions
   regarding these operations.

4/ We add support for special attributes like *_auto_point*.

5/ Cleanup: Change permissions to numeric and add me to the author

Oscar Salvador (5):
  nouveau_hwmon: Add config for all sensors and their settings
  nouveau_hwmon: Add nouveau_hwmon_ops structure with
    .is_visible/.read_string
  nouveau_hwmon: Remove old code, add .write/.read operations
  nouveau_hwmon: Add support for auto_point attributes
  nouveau_hwmon: Change permissions to numeric

 drivers/gpu/drm/nouveau/nouveau_hwmon.c | 946 +++++++++++++++++---------------
 1 file changed, 494 insertions(+), 452 deletions(-)

-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/5] nouveau_hwmon: Add config for all sensors and their settings

[Oscar Salvador]

This is a preparation for the next patches. It just adds the sensors with
their possible configurable settings and then fills the struct hwmon_channel_info
with all this information.
---
 drivers/gpu/drm/nouveau/nouveau_hwmon.c | 72 +++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
index 23b1670..24b40c5 100644
--- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
@@ -692,6 +692,78 @@ static const struct attribute_group hwmon_power_attrgroup = {
 static const struct attribute_group hwmon_power_caps_attrgroup = {
 	.attrs = hwmon_power_caps_attributes,
 };
+
+static const u32 nouveau_config_chip[] = {
+	HWMON_C_UPDATE_INTERVAL,
+	0
+};
+
+static const u32 nouveau_config_in[] = {
+	HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_LABEL,
+	0
+};
+
+static const u32 nouveau_config_temp[] = {
+	HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
+	HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_EMERGENCY |
+	HWMON_T_EMERGENCY_HYST,
+	0
+};
+
+static const u32 nouveau_config_fan[] = {
+	HWMON_F_INPUT,
+	0
+};
+
+static const u32 nouveau_config_pwm[] = {
+	HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+	0
+};
+
+static const u32 nouveau_config_power[] = {
+	HWMON_P_INPUT | HWMON_P_CAP_MAX | HWMON_P_CRIT,
+	0
+};
+
+static const struct hwmon_channel_info nouveau_chip = {
+	.type = hwmon_chip,
+	.config = nouveau_config_chip,
+};
+
+static const struct hwmon_channel_info nouveau_temp = {
+	.type = hwmon_temp,
+	.config = nouveau_config_temp,
+};
+
+static const struct hwmon_channel_info nouveau_fan = {
+	.type = hwmon_fan,
+	.config = nouveau_config_fan,
+};
+
+static const struct hwmon_channel_info nouveau_in = {
+	.type = hwmon_in,
+	.config = nouveau_config_in,
+};
+
+static const struct hwmon_channel_info nouveau_pwm = {
+	.type = hwmon_pwm,
+	.config = nouveau_config_pwm,
+};
+
+static const struct hwmon_channel_info nouveau_power = {
+	.type = hwmon_power,
+	.config = nouveau_config_power,
+};
+
+static const struct hwmon_channel_info *nouveau_info[] = {
+	&nouveau_chip,
+	&nouveau_temp,
+	&nouveau_fan,
+	&nouveau_in,
+	&nouveau_pwm,
+	&nouveau_power,
+	NULL
+};
 #endif
 
 int
-- 
2.1.4

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH v2 1/5] nouveau_hwmon: Add config for all sensors and their settings

[Ilia Mirkin]

All kernel patches must have a Signed-off-by: $user. See
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

On Mon, Apr 17, 2017 at 3:47 AM, Oscar Salvador
<osalvador.vilardaga@gmail.com> wrote:
> This is a preparation for the next patches. It just adds the sensors with
> their possible configurable settings and then fills the struct hwmon_channel_info
> with all this information.
> ---
>  drivers/gpu/drm/nouveau/nouveau_hwmon.c | 72 +++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> index 23b1670..24b40c5 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> @@ -692,6 +692,78 @@ static const struct attribute_group hwmon_power_attrgroup = {
>  static const struct attribute_group hwmon_power_caps_attrgroup = {
>         .attrs = hwmon_power_caps_attributes,
>  };
> +
> +static const u32 nouveau_config_chip[] = {
> +       HWMON_C_UPDATE_INTERVAL,
> +       0
> +};
> +
> +static const u32 nouveau_config_in[] = {
> +       HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_LABEL,
> +       0
> +};
> +
> +static const u32 nouveau_config_temp[] = {
> +       HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> +       HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_EMERGENCY |
> +       HWMON_T_EMERGENCY_HYST,
> +       0
> +};
> +
> +static const u32 nouveau_config_fan[] = {
> +       HWMON_F_INPUT,
> +       0
> +};
> +
> +static const u32 nouveau_config_pwm[] = {
> +       HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> +       0
> +};
> +
> +static const u32 nouveau_config_power[] = {
> +       HWMON_P_INPUT | HWMON_P_CAP_MAX | HWMON_P_CRIT,
> +       0
> +};
> +
> +static const struct hwmon_channel_info nouveau_chip = {
> +       .type = hwmon_chip,
> +       .config = nouveau_config_chip,
> +};
> +
> +static const struct hwmon_channel_info nouveau_temp = {
> +       .type = hwmon_temp,
> +       .config = nouveau_config_temp,
> +};
> +
> +static const struct hwmon_channel_info nouveau_fan = {
> +       .type = hwmon_fan,
> +       .config = nouveau_config_fan,
> +};
> +
> +static const struct hwmon_channel_info nouveau_in = {
> +       .type = hwmon_in,
> +       .config = nouveau_config_in,
> +};
> +
> +static const struct hwmon_channel_info nouveau_pwm = {
> +       .type = hwmon_pwm,
> +       .config = nouveau_config_pwm,
> +};
> +
> +static const struct hwmon_channel_info nouveau_power = {
> +       .type = hwmon_power,
> +       .config = nouveau_config_power,
> +};
> +
> +static const struct hwmon_channel_info *nouveau_info[] = {
> +       &nouveau_chip,
> +       &nouveau_temp,
> +       &nouveau_fan,
> +       &nouveau_in,
> +       &nouveau_pwm,
> +       &nouveau_power,
> +       NULL
> +};
>  #endif
>
>  int
> --
> 2.1.4
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 1/5] nouveau_hwmon: Add config for all sensors and their settings

[Oscar Salvador]

Got it, I'll make a v3 with that line in all patches.

Thanks!

On 19 April 2017 at 20:11, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> All kernel patches must have a Signed-off-by: $user. See
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
>
> On Mon, Apr 17, 2017 at 3:47 AM, Oscar Salvador
> <osalvador.vilardaga@gmail.com> wrote:
>> This is a preparation for the next patches. It just adds the sensors with
>> their possible configurable settings and then fills the struct hwmon_channel_info
>> with all this information.
>> ---
>>  drivers/gpu/drm/nouveau/nouveau_hwmon.c | 72 +++++++++++++++++++++++++++++++++
>>  1 file changed, 72 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
>> index 23b1670..24b40c5 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
>> @@ -692,6 +692,78 @@ static const struct attribute_group hwmon_power_attrgroup = {
>>  static const struct attribute_group hwmon_power_caps_attrgroup = {
>>         .attrs = hwmon_power_caps_attributes,
>>  };
>> +
>> +static const u32 nouveau_config_chip[] = {
>> +       HWMON_C_UPDATE_INTERVAL,
>> +       0
>> +};
>> +
>> +static const u32 nouveau_config_in[] = {
>> +       HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_LABEL,
>> +       0
>> +};
>> +
>> +static const u32 nouveau_config_temp[] = {
>> +       HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
>> +       HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_EMERGENCY |
>> +       HWMON_T_EMERGENCY_HYST,
>> +       0
>> +};
>> +
>> +static const u32 nouveau_config_fan[] = {
>> +       HWMON_F_INPUT,
>> +       0
>> +};
>> +
>> +static const u32 nouveau_config_pwm[] = {
>> +       HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
>> +       0
>> +};
>> +
>> +static const u32 nouveau_config_power[] = {
>> +       HWMON_P_INPUT | HWMON_P_CAP_MAX | HWMON_P_CRIT,
>> +       0
>> +};
>> +
>> +static const struct hwmon_channel_info nouveau_chip = {
>> +       .type = hwmon_chip,
>> +       .config = nouveau_config_chip,
>> +};
>> +
>> +static const struct hwmon_channel_info nouveau_temp = {
>> +       .type = hwmon_temp,
>> +       .config = nouveau_config_temp,
>> +};
>> +
>> +static const struct hwmon_channel_info nouveau_fan = {
>> +       .type = hwmon_fan,
>> +       .config = nouveau_config_fan,
>> +};
>> +
>> +static const struct hwmon_channel_info nouveau_in = {
>> +       .type = hwmon_in,
>> +       .config = nouveau_config_in,
>> +};
>> +
>> +static const struct hwmon_channel_info nouveau_pwm = {
>> +       .type = hwmon_pwm,
>> +       .config = nouveau_config_pwm,
>> +};
>> +
>> +static const struct hwmon_channel_info nouveau_power = {
>> +       .type = hwmon_power,
>> +       .config = nouveau_config_power,
>> +};
>> +
>> +static const struct hwmon_channel_info *nouveau_info[] = {
>> +       &nouveau_chip,
>> +       &nouveau_temp,
>> +       &nouveau_fan,
>> +       &nouveau_in,
>> +       &nouveau_pwm,
>> +       &nouveau_power,
>> +       NULL
>> +};
>>  #endif
>>
>>  int
>> --
>> 2.1.4
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/5] nouveau_hwmon: Add nouveau_hwmon_ops structure with .is_visible/.read_string

[Oscar Salvador]

This patch introduces the nouveau_hwmon_ops structure, sets up
.is_visible and .read_string operations and adds all the functions
for these operations.
This is also a preparation for the next patches, where most of the
work is being done.
This code doesn't interacture with the old one.
It's just to make easier the review of all patches.
---
 drivers/gpu/drm/nouveau/nouveau_hwmon.c | 166 ++++++++++++++++++++++++++++++++
 1 file changed, 166 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
index 24b40c5..9b6423c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
@@ -764,6 +764,172 @@ static const struct hwmon_channel_info *nouveau_info[] = {
 	&nouveau_power,
 	NULL
 };
+
+static umode_t
+nouveau_chip_is_visible(const void *data, u32 attr, int channel)
+{
+	switch (attr) {
+	case hwmon_chip_update_interval:
+		return 0444;
+	default:
+		return 0;
+	}
+}
+
+static umode_t
+nouveau_power_is_visible(const void *data, u32 attr, int channel)
+{
+	struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
+	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->client.device);
+
+	switch (attr) {
+	case hwmon_power_input:
+		if (iccsense &&
+			iccsense->data_valid &&
+			!list_empty(&iccsense->rails))
+			return 0444;
+	case hwmon_power_max:
+		if (iccsense->power_w_max)
+			return 0444;
+	case hwmon_power_crit:
+		if (iccsense->power_w_crit)
+			return 0444;
+	default:
+		return 0;
+	}
+}
+
+static umode_t
+nouveau_temp_is_visible(const void *data, u32 attr, int channel)
+{
+	struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
+	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
+
+	if (therm &&
+		therm->attr_get &&
+		therm->attr_set &&
+		nvkm_therm_temp_get(therm) < 0)
+		return 0;
+
+	switch (attr) {
+	case hwmon_temp_input:
+	case hwmon_temp_max:
+	case hwmon_temp_max_hyst:
+	case hwmon_temp_crit:
+	case hwmon_temp_crit_hyst:
+	case hwmon_temp_emergency:
+	case hwmon_temp_emergency_hyst:
+		return 0444;
+	default:
+		return 0;
+	}
+}
+
+static umode_t
+nouveau_pwm_is_visible(const void *data, u32 attr, int channel)
+{
+	struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
+	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
+
+	if (therm &&
+		therm->attr_get &&
+		therm->fan_get &&
+		therm->fan_get(therm) < 0)
+		return 0;
+
+	switch (attr) {
+	case hwmon_pwm_enable:
+	case hwmon_pwm_input:
+		return 0644;
+	default:
+		return 0;
+	}
+}
+
+static umode_t
+nouveau_input_is_visible(const void *data, u32 attr, int channel)
+{
+	struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
+	struct nvkm_volt *volt = nvxx_volt(&drm->client.device);
+
+	if (!volt || nvkm_volt_get(volt) < 0)
+		return 0;
+
+	switch (attr) {
+	case hwmon_in_input:
+	case hwmon_in_label:
+	case hwmon_in_min:
+	case hwmon_in_max:
+		return 0444;
+	default:
+		return 0;
+	}
+}
+
+static umode_t
+nouveau_fan_is_visible(const void *data, u32 attr, int channel)
+{
+	struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
+	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
+
+	if (!therm || !therm->attr_get || nvkm_therm_fan_sense(therm) < 0)
+		return 0;
+
+	switch (attr) {
+	case hwmon_fan_input:
+		return 0444;
+	default:
+		return 0;
+	}
+}
+
+static umode_t
+nouveau_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr,
+								int channel)
+{
+	switch (type) {
+	case hwmon_chip:
+		return nouveau_chip_is_visible(data, attr, channel);
+	case hwmon_temp:
+		return nouveau_temp_is_visible(data, attr, channel);
+	case hwmon_fan:
+		return nouveau_fan_is_visible(data, attr, channel);
+	case hwmon_in:
+		return nouveau_input_is_visible(data, attr, channel);
+	case hwmon_pwm:
+		return nouveau_pwm_is_visible(data, attr, channel);
+	case hwmon_power:
+		return nouveau_power_is_visible(data, attr, channel);
+	default:
+		return 0;
+	}
+}
+
+static char *input_label = "GPU core";
+
+static int
+nouveau_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+							int channel, char **buf)
+{
+	if (type == hwmon_in && attr == hwmon_in_label) {
+		*buf = input_label;
+		return 0;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static const struct hwmon_ops nouveau_hwmon_ops = {
+	.is_visible = nouveau_is_visible,
+	.read = NULL,
+	.read_string = nouveau_read_string,
+	.write = NULL,
+};
+
+static const struct hwmon_chip_info nouveau_chip_info = {
+	.ops = &nouveau_hwmon_ops,
+	.info = nouveau_info,
+};
 #endif
 
 int
-- 
2.1.4

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH v2 2/5] nouveau_hwmon: Add nouveau_hwmon_ops structure with .is_visible/.read_string

[Karol Herbst]

2017-04-17 9:47 GMT+02:00 Oscar Salvador <osalvador.vilardaga@gmail.com>:
> This patch introduces the nouveau_hwmon_ops structure, sets up
> .is_visible and .read_string operations and adds all the functions
> for these operations.
> This is also a preparation for the next patches, where most of the
> work is being done.
> This code doesn't interacture with the old one.
> It's just to make easier the review of all patches.
> ---
>  drivers/gpu/drm/nouveau/nouveau_hwmon.c | 166 ++++++++++++++++++++++++++++++++
>  1 file changed, 166 insertions(+)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> index 24b40c5..9b6423c 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> @@ -764,6 +764,172 @@ static const struct hwmon_channel_info *nouveau_info[] = {
>         &nouveau_power,
>         NULL
>  };
> +
> +static umode_t
> +nouveau_chip_is_visible(const void *data, u32 attr, int channel)
> +{
> +       switch (attr) {
> +       case hwmon_chip_update_interval:
> +               return 0444;
> +       default:
> +               return 0;
> +       }
> +}
> +
> +static umode_t
> +nouveau_power_is_visible(const void *data, u32 attr, int channel)
> +{
> +       struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
> +       struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->client.device);
> +
> +       switch (attr) {
> +       case hwmon_power_input:
> +               if (iccsense &&

move the pointer check before the switch, because you need to check it
in every case (or move it to every case, but doing it once is
simplier/cleaner)

> +                       iccsense->data_valid &&
> +                       !list_empty(&iccsense->rails))
> +                       return 0444;

add return due to fallthrough

> +       case hwmon_power_max:
> +               if (iccsense->power_w_max)
> +                       return 0444;

add return due to fallthrough

> +       case hwmon_power_crit:
> +               if (iccsense->power_w_crit)
> +                       return 0444;

add return due to fallthrough

> +       default:
> +               return 0;
> +       }
> +}
> +
> +static umode_t
> +nouveau_temp_is_visible(const void *data, u32 attr, int channel)
> +{
> +       struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
> +       struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> +
> +       if (therm &&
> +               therm->attr_get &&
> +               therm->attr_set &&
> +               nvkm_therm_temp_get(therm) < 0)
> +               return 0;
> +
> +       switch (attr) {
> +       case hwmon_temp_input:
> +       case hwmon_temp_max:
> +       case hwmon_temp_max_hyst:
> +       case hwmon_temp_crit:
> +       case hwmon_temp_crit_hyst:
> +       case hwmon_temp_emergency:
> +       case hwmon_temp_emergency_hyst:
> +               return 0444;
> +       default:
> +               return 0;
> +       }
> +}
> +
> +static umode_t
> +nouveau_pwm_is_visible(const void *data, u32 attr, int channel)
> +{
> +       struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
> +       struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> +
> +       if (therm &&
> +               therm->attr_get &&
> +               therm->fan_get &&
> +               therm->fan_get(therm) < 0)
> +               return 0;
> +
> +       switch (attr) {
> +       case hwmon_pwm_enable:
> +       case hwmon_pwm_input:
> +               return 0644;
> +       default:
> +               return 0;
> +       }
> +}
> +
> +static umode_t
> +nouveau_input_is_visible(const void *data, u32 attr, int channel)
> +{
> +       struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
> +       struct nvkm_volt *volt = nvxx_volt(&drm->client.device);
> +
> +       if (!volt || nvkm_volt_get(volt) < 0)
> +               return 0;
> +
> +       switch (attr) {
> +       case hwmon_in_input:
> +       case hwmon_in_label:
> +       case hwmon_in_min:
> +       case hwmon_in_max:
> +               return 0444;
> +       default:
> +               return 0;
> +       }
> +}
> +
> +static umode_t
> +nouveau_fan_is_visible(const void *data, u32 attr, int channel)
> +{
> +       struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
> +       struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> +
> +       if (!therm || !therm->attr_get || nvkm_therm_fan_sense(therm) < 0)
> +               return 0;
> +
> +       switch (attr) {
> +       case hwmon_fan_input:
> +               return 0444;
> +       default:
> +               return 0;
> +       }
> +}
> +
> +static umode_t
> +nouveau_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr,
> +                                                               int channel)
> +{
> +       switch (type) {
> +       case hwmon_chip:
> +               return nouveau_chip_is_visible(data, attr, channel);
> +       case hwmon_temp:
> +               return nouveau_temp_is_visible(data, attr, channel);
> +       case hwmon_fan:
> +               return nouveau_fan_is_visible(data, attr, channel);
> +       case hwmon_in:
> +               return nouveau_input_is_visible(data, attr, channel);
> +       case hwmon_pwm:
> +               return nouveau_pwm_is_visible(data, attr, channel);
> +       case hwmon_power:
> +               return nouveau_power_is_visible(data, attr, channel);
> +       default:
> +               return 0;
> +       }
> +}
> +
> +static char *input_label = "GPU core";
> +
> +static int
> +nouveau_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +                                                       int channel, char **buf)
> +{
> +       if (type == hwmon_in && attr == hwmon_in_label) {
> +               *buf = input_label;
> +               return 0;
> +       }
> +
> +       return -EOPNOTSUPP;
> +}
> +
> +static const struct hwmon_ops nouveau_hwmon_ops = {
> +       .is_visible = nouveau_is_visible,
> +       .read = NULL,
> +       .read_string = nouveau_read_string,
> +       .write = NULL,
> +};
> +
> +static const struct hwmon_chip_info nouveau_chip_info = {
> +       .ops = &nouveau_hwmon_ops,
> +       .info = nouveau_info,
> +};
>  #endif
>
>  int
> --
> 2.1.4
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 2/5] nouveau_hwmon: Add nouveau_hwmon_ops structure with .is_visible/.read_string

[Oscar Salvador]

Hi Karol,

I don't get what you mean with return due to fallthrough. I mean, I
know what is it, but I don't see how I can do it there.
Moving the check before the switch looks like that:

        if (!iccsense)
                return 0;

        switch (attr) {
        case hwmon_power_input:
                if (iccsense->data_valid &&
                        !list_empty(&iccsense->rails))
                        return 0444;
        case hwmon_power_max:
                if (iccsense->power_w_max)
                        return 0444;
        case hwmon_power_crit:
                if (iccsense->power_w_crit)
                        return 0444;
        default:
                return 0;
        }

Could you drop me a hint?


On 18 April 2017 at 09:56, Karol Herbst <karolherbst@gmail.com> wrote:
> 2017-04-17 9:47 GMT+02:00 Oscar Salvador <osalvador.vilardaga@gmail.com>:
>> This patch introduces the nouveau_hwmon_ops structure, sets up
>> .is_visible and .read_string operations and adds all the functions
>> for these operations.
>> This is also a preparation for the next patches, where most of the
>> work is being done.
>> This code doesn't interacture with the old one.
>> It's just to make easier the review of all patches.
>> ---
>>  drivers/gpu/drm/nouveau/nouveau_hwmon.c | 166 ++++++++++++++++++++++++++++++++
>>  1 file changed, 166 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
>> index 24b40c5..9b6423c 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
>> @@ -764,6 +764,172 @@ static const struct hwmon_channel_info *nouveau_info[] = {
>>         &nouveau_power,
>>         NULL
>>  };
>> +
>> +static umode_t
>> +nouveau_chip_is_visible(const void *data, u32 attr, int channel)
>> +{
>> +       switch (attr) {
>> +       case hwmon_chip_update_interval:
>> +               return 0444;
>> +       default:
>> +               return 0;
>> +       }
>> +}
>> +
>> +static umode_t
>> +nouveau_power_is_visible(const void *data, u32 attr, int channel)
>> +{
>> +       struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
>> +       struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->client.device);
>> +
>> +       switch (attr) {
>> +       case hwmon_power_input:
>> +               if (iccsense &&
>
> move the pointer check before the switch, because you need to check it
> in every case (or move it to every case, but doing it once is
> simplier/cleaner)
>
>> +                       iccsense->data_valid &&
>> +                       !list_empty(&iccsense->rails))
>> +                       return 0444;
>
> add return due to fallthrough
>
>> +       case hwmon_power_max:
>> +               if (iccsense->power_w_max)
>> +                       return 0444;
>
> add return due to fallthrough
>
>> +       case hwmon_power_crit:
>> +               if (iccsense->power_w_crit)
>> +                       return 0444;
>
> add return due to fallthrough
>
>> +       default:
>> +               return 0;
>> +       }
>> +}
>> +
>> +static umode_t
>> +nouveau_temp_is_visible(const void *data, u32 attr, int channel)
>> +{
>> +       struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
>> +       struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
>> +
>> +       if (therm &&
>> +               therm->attr_get &&
>> +               therm->attr_set &&
>> +               nvkm_therm_temp_get(therm) < 0)
>> +               return 0;
>> +
>> +       switch (attr) {
>> +       case hwmon_temp_input:
>> +       case hwmon_temp_max:
>> +       case hwmon_temp_max_hyst:
>> +       case hwmon_temp_crit:
>> +       case hwmon_temp_crit_hyst:
>> +       case hwmon_temp_emergency:
>> +       case hwmon_temp_emergency_hyst:
>> +               return 0444;
>> +       default:
>> +               return 0;
>> +       }
>> +}
>> +
>> +static umode_t
>> +nouveau_pwm_is_visible(const void *data, u32 attr, int channel)
>> +{
>> +       struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
>> +       struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
>> +
>> +       if (therm &&
>> +               therm->attr_get &&
>> +               therm->fan_get &&
>> +               therm->fan_get(therm) < 0)
>> +               return 0;
>> +
>> +       switch (attr) {
>> +       case hwmon_pwm_enable:
>> +       case hwmon_pwm_input:
>> +               return 0644;
>> +       default:
>> +               return 0;
>> +       }
>> +}
>> +
>> +static umode_t
>> +nouveau_input_is_visible(const void *data, u32 attr, int channel)
>> +{
>> +       struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
>> +       struct nvkm_volt *volt = nvxx_volt(&drm->client.device);
>> +
>> +       if (!volt || nvkm_volt_get(volt) < 0)
>> +               return 0;
>> +
>> +       switch (attr) {
>> +       case hwmon_in_input:
>> +       case hwmon_in_label:
>> +       case hwmon_in_min:
>> +       case hwmon_in_max:
>> +               return 0444;
>> +       default:
>> +               return 0;
>> +       }
>> +}
>> +
>> +static umode_t
>> +nouveau_fan_is_visible(const void *data, u32 attr, int channel)
>> +{
>> +       struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
>> +       struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
>> +
>> +       if (!therm || !therm->attr_get || nvkm_therm_fan_sense(therm) < 0)
>> +               return 0;
>> +
>> +       switch (attr) {
>> +       case hwmon_fan_input:
>> +               return 0444;
>> +       default:
>> +               return 0;
>> +       }
>> +}
>> +
>> +static umode_t
>> +nouveau_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr,
>> +                                                               int channel)
>> +{
>> +       switch (type) {
>> +       case hwmon_chip:
>> +               return nouveau_chip_is_visible(data, attr, channel);
>> +       case hwmon_temp:
>> +               return nouveau_temp_is_visible(data, attr, channel);
>> +       case hwmon_fan:
>> +               return nouveau_fan_is_visible(data, attr, channel);
>> +       case hwmon_in:
>> +               return nouveau_input_is_visible(data, attr, channel);
>> +       case hwmon_pwm:
>> +               return nouveau_pwm_is_visible(data, attr, channel);
>> +       case hwmon_power:
>> +               return nouveau_power_is_visible(data, attr, channel);
>> +       default:
>> +               return 0;
>> +       }
>> +}
>> +
>> +static char *input_label = "GPU core";
>> +
>> +static int
>> +nouveau_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> +                                                       int channel, char **buf)
>> +{
>> +       if (type == hwmon_in && attr == hwmon_in_label) {
>> +               *buf = input_label;
>> +               return 0;
>> +       }
>> +
>> +       return -EOPNOTSUPP;
>> +}
>> +
>> +static const struct hwmon_ops nouveau_hwmon_ops = {
>> +       .is_visible = nouveau_is_visible,
>> +       .read = NULL,
>> +       .read_string = nouveau_read_string,
>> +       .write = NULL,
>> +};
>> +
>> +static const struct hwmon_chip_info nouveau_chip_info = {
>> +       .ops = &nouveau_hwmon_ops,
>> +       .info = nouveau_info,
>> +};
>>  #endif
>>
>>  int
>> --
>> 2.1.4
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Nouveau] [PATCH v2 2/5] nouveau_hwmon: Add nouveau_hwmon_ops structure with .is_visible/.read_string

[Oscar Salvador]

[-- Attachment #1.1: Type: text/plain, Size: 8138 bytes --]

I got what you meant. I"ll fix it

El dia 20/04/2017 08:47, "Oscar Salvador" <osalvador.vilardaga@gmail.com>
va escriure:

> Hi Karol,
>
> I don't get what you mean with return due to fallthrough. I mean, I
> know what is it, but I don't see how I can do it there.
> Moving the check before the switch looks like that:
>
>         if (!iccsense)
>                 return 0;
>
>         switch (attr) {
>         case hwmon_power_input:
>                 if (iccsense->data_valid &&
>                         !list_empty(&iccsense->rails))
>                         return 0444;
>         case hwmon_power_max:
>                 if (iccsense->power_w_max)
>                         return 0444;
>         case hwmon_power_crit:
>                 if (iccsense->power_w_crit)
>                         return 0444;
>         default:
>                 return 0;
>         }
>
> Could you drop me a hint?
>
>
> On 18 April 2017 at 09:56, Karol Herbst <karolherbst@gmail.com> wrote:
> > 2017-04-17 9:47 GMT+02:00 Oscar Salvador <osalvador.vilardaga@gmail.com
> >:
> >> This patch introduces the nouveau_hwmon_ops structure, sets up
> >> .is_visible and .read_string operations and adds all the functions
> >> for these operations.
> >> This is also a preparation for the next patches, where most of the
> >> work is being done.
> >> This code doesn't interacture with the old one.
> >> It's just to make easier the review of all patches.
> >> ---
> >>  drivers/gpu/drm/nouveau/nouveau_hwmon.c | 166
> ++++++++++++++++++++++++++++++++
> >>  1 file changed, 166 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> >> index 24b40c5..9b6423c 100644
> >> --- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> >> +++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> >> @@ -764,6 +764,172 @@ static const struct hwmon_channel_info
> *nouveau_info[] = {
> >>         &nouveau_power,
> >>         NULL
> >>  };
> >> +
> >> +static umode_t
> >> +nouveau_chip_is_visible(const void *data, u32 attr, int channel)
> >> +{
> >> +       switch (attr) {
> >> +       case hwmon_chip_update_interval:
> >> +               return 0444;
> >> +       default:
> >> +               return 0;
> >> +       }
> >> +}
> >> +
> >> +static umode_t
> >> +nouveau_power_is_visible(const void *data, u32 attr, int channel)
> >> +{
> >> +       struct nouveau_drm *drm = nouveau_drm((struct drm_device
> *)data);
> >> +       struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->client.
> device);
> >> +
> >> +       switch (attr) {
> >> +       case hwmon_power_input:
> >> +               if (iccsense &&
> >
> > move the pointer check before the switch, because you need to check it
> > in every case (or move it to every case, but doing it once is
> > simplier/cleaner)
> >
> >> +                       iccsense->data_valid &&
> >> +                       !list_empty(&iccsense->rails))
> >> +                       return 0444;
> >
> > add return due to fallthrough
> >
> >> +       case hwmon_power_max:
> >> +               if (iccsense->power_w_max)
> >> +                       return 0444;
> >
> > add return due to fallthrough
> >
> >> +       case hwmon_power_crit:
> >> +               if (iccsense->power_w_crit)
> >> +                       return 0444;
> >
> > add return due to fallthrough
> >
> >> +       default:
> >> +               return 0;
> >> +       }
> >> +}
> >> +
> >> +static umode_t
> >> +nouveau_temp_is_visible(const void *data, u32 attr, int channel)
> >> +{
> >> +       struct nouveau_drm *drm = nouveau_drm((struct drm_device
> *)data);
> >> +       struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> >> +
> >> +       if (therm &&
> >> +               therm->attr_get &&
> >> +               therm->attr_set &&
> >> +               nvkm_therm_temp_get(therm) < 0)
> >> +               return 0;
> >> +
> >> +       switch (attr) {
> >> +       case hwmon_temp_input:
> >> +       case hwmon_temp_max:
> >> +       case hwmon_temp_max_hyst:
> >> +       case hwmon_temp_crit:
> >> +       case hwmon_temp_crit_hyst:
> >> +       case hwmon_temp_emergency:
> >> +       case hwmon_temp_emergency_hyst:
> >> +               return 0444;
> >> +       default:
> >> +               return 0;
> >> +       }
> >> +}
> >> +
> >> +static umode_t
> >> +nouveau_pwm_is_visible(const void *data, u32 attr, int channel)
> >> +{
> >> +       struct nouveau_drm *drm = nouveau_drm((struct drm_device
> *)data);
> >> +       struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> >> +
> >> +       if (therm &&
> >> +               therm->attr_get &&
> >> +               therm->fan_get &&
> >> +               therm->fan_get(therm) < 0)
> >> +               return 0;
> >> +
> >> +       switch (attr) {
> >> +       case hwmon_pwm_enable:
> >> +       case hwmon_pwm_input:
> >> +               return 0644;
> >> +       default:
> >> +               return 0;
> >> +       }
> >> +}
> >> +
> >> +static umode_t
> >> +nouveau_input_is_visible(const void *data, u32 attr, int channel)
> >> +{
> >> +       struct nouveau_drm *drm = nouveau_drm((struct drm_device
> *)data);
> >> +       struct nvkm_volt *volt = nvxx_volt(&drm->client.device);
> >> +
> >> +       if (!volt || nvkm_volt_get(volt) < 0)
> >> +               return 0;
> >> +
> >> +       switch (attr) {
> >> +       case hwmon_in_input:
> >> +       case hwmon_in_label:
> >> +       case hwmon_in_min:
> >> +       case hwmon_in_max:
> >> +               return 0444;
> >> +       default:
> >> +               return 0;
> >> +       }
> >> +}
> >> +
> >> +static umode_t
> >> +nouveau_fan_is_visible(const void *data, u32 attr, int channel)
> >> +{
> >> +       struct nouveau_drm *drm = nouveau_drm((struct drm_device
> *)data);
> >> +       struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> >> +
> >> +       if (!therm || !therm->attr_get || nvkm_therm_fan_sense(therm) <
> 0)
> >> +               return 0;
> >> +
> >> +       switch (attr) {
> >> +       case hwmon_fan_input:
> >> +               return 0444;
> >> +       default:
> >> +               return 0;
> >> +       }
> >> +}
> >> +
> >> +static umode_t
> >> +nouveau_is_visible(const void *data, enum hwmon_sensor_types type, u32
> attr,
> >> +                                                               int
> channel)
> >> +{
> >> +       switch (type) {
> >> +       case hwmon_chip:
> >> +               return nouveau_chip_is_visible(data, attr, channel);
> >> +       case hwmon_temp:
> >> +               return nouveau_temp_is_visible(data, attr, channel);
> >> +       case hwmon_fan:
> >> +               return nouveau_fan_is_visible(data, attr, channel);
> >> +       case hwmon_in:
> >> +               return nouveau_input_is_visible(data, attr, channel);
> >> +       case hwmon_pwm:
> >> +               return nouveau_pwm_is_visible(data, attr, channel);
> >> +       case hwmon_power:
> >> +               return nouveau_power_is_visible(data, attr, channel);
> >> +       default:
> >> +               return 0;
> >> +       }
> >> +}
> >> +
> >> +static char *input_label = "GPU core";
> >> +
> >> +static int
> >> +nouveau_read_string(struct device *dev, enum hwmon_sensor_types type,
> u32 attr,
> >> +                                                       int channel,
> char **buf)
> >> +{
> >> +       if (type == hwmon_in && attr == hwmon_in_label) {
> >> +               *buf = input_label;
> >> +               return 0;
> >> +       }
> >> +
> >> +       return -EOPNOTSUPP;
> >> +}
> >> +
> >> +static const struct hwmon_ops nouveau_hwmon_ops = {
> >> +       .is_visible = nouveau_is_visible,
> >> +       .read = NULL,
> >> +       .read_string = nouveau_read_string,
> >> +       .write = NULL,
> >> +};
> >> +
> >> +static const struct hwmon_chip_info nouveau_chip_info = {
> >> +       .ops = &nouveau_hwmon_ops,
> >> +       .info = nouveau_info,
> >> +};
> >>  #endif
> >>
> >>  int
> >> --
> >> 2.1.4
> >>
> >> _______________________________________________
> >> Nouveau mailing list
> >> Nouveau@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/nouveau
>

[-- Attachment #1.2: Type: text/html, Size: 11672 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/5] nouveau_hwmon: Add nouveau_hwmon_ops structure with .is_visible/.read_string

[Ilia Mirkin]

On Mon, Apr 17, 2017 at 3:47 AM, Oscar Salvador
<osalvador.vilardaga@gmail.com> wrote:
> +static char *input_label = "GPU core";

This needs to be static const char input_label[] = "GPU core";

or at least static const char *const input_label = "GPU core";

  -ilia
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH v2 3/5] nouveau_hwmon: Remove old code, add .write/.read operations

[Oscar Salvador]

This patch removes old code related to the old api and transforms the
functions for the new api. It also adds the .write and .read operations.

Since we don't want to support changing hwmon_temp_* values, this patch
deletes all write-functions for these attributes too.
---
 drivers/gpu/drm/nouveau/nouveau_hwmon.c | 719 +++++++++++---------------------
 1 file changed, 248 insertions(+), 471 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
index 9b6423c..538bf67 100644
--- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
@@ -38,21 +38,17 @@
 #include <nvkm/subdev/volt.h>
 
 #if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE))
-static ssize_t
-nouveau_hwmon_show_temp(struct device *d, struct device_attribute *a, char *buf)
+static int
+nouveau_hwmon_show_temp(struct nouveau_drm *drm)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
 	int temp = nvkm_therm_temp_get(therm);
 
 	if (temp < 0)
 		return temp;
 
-	return snprintf(buf, PAGE_SIZE, "%d\n", temp * 1000);
+	return (temp * 1000);
 }
-static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, nouveau_hwmon_show_temp,
-						  NULL, 0);
 
 static ssize_t
 nouveau_hwmon_show_temp1_auto_point1_pwm(struct device *d,
@@ -129,312 +125,100 @@ static SENSOR_DEVICE_ATTR(temp1_auto_point1_temp_hyst, S_IRUGO | S_IWUSR,
 			  nouveau_hwmon_temp1_auto_point1_temp_hyst,
 			  nouveau_hwmon_set_temp1_auto_point1_temp_hyst, 0);
 
-static ssize_t
-nouveau_hwmon_max_temp(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_therm *therm = nvxx_therm(&drm->client.device);
-
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-	       therm->attr_get(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK) * 1000);
-}
-static ssize_t
-nouveau_hwmon_set_max_temp(struct device *d, struct device_attribute *a,
-						const char *buf, size_t count)
+static int
+nouveau_hwmon_max_temp(struct nouveau_drm *drm)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
-	long value;
 
-	if (kstrtol(buf, 10, &value) == -EINVAL)
-		return count;
-
-	therm->attr_set(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK, value / 1000);
-
-	return count;
+	return therm->attr_get(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK) * 1000;
 }
-static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO | S_IWUSR, nouveau_hwmon_max_temp,
-						  nouveau_hwmon_set_max_temp,
-						  0);
 
-static ssize_t
-nouveau_hwmon_max_temp_hyst(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_therm *therm = nvxx_therm(&drm->client.device);
-
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-	  therm->attr_get(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK_HYST) * 1000);
-}
-static ssize_t
-nouveau_hwmon_set_max_temp_hyst(struct device *d, struct device_attribute *a,
-						const char *buf, size_t count)
+static int
+nouveau_hwmon_max_temp_hyst(struct nouveau_drm *drm)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
-	long value;
-
-	if (kstrtol(buf, 10, &value) == -EINVAL)
-		return count;
 
-	therm->attr_set(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK_HYST,
-			value / 1000);
-
-	return count;
+	return therm->attr_get(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK_HYST) * 1000;
 }
-static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IRUGO | S_IWUSR,
-			  nouveau_hwmon_max_temp_hyst,
-			  nouveau_hwmon_set_max_temp_hyst, 0);
 
-static ssize_t
-nouveau_hwmon_critical_temp(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_therm *therm = nvxx_therm(&drm->client.device);
-
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-	       therm->attr_get(therm, NVKM_THERM_ATTR_THRS_CRITICAL) * 1000);
-}
-static ssize_t
-nouveau_hwmon_set_critical_temp(struct device *d, struct device_attribute *a,
-							    const char *buf,
-								size_t count)
+static int
+nouveau_hwmon_critical_temp(struct nouveau_drm *drm)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
-	long value;
-
-	if (kstrtol(buf, 10, &value) == -EINVAL)
-		return count;
-
-	therm->attr_set(therm, NVKM_THERM_ATTR_THRS_CRITICAL, value / 1000);
 
-	return count;
+	return therm->attr_get(therm, NVKM_THERM_ATTR_THRS_CRITICAL) * 1000;
 }
-static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO | S_IWUSR,
-						nouveau_hwmon_critical_temp,
-						nouveau_hwmon_set_critical_temp,
-						0);
 
-static ssize_t
-nouveau_hwmon_critical_temp_hyst(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_therm *therm = nvxx_therm(&drm->client.device);
-
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-	  therm->attr_get(therm, NVKM_THERM_ATTR_THRS_CRITICAL_HYST) * 1000);
-}
-static ssize_t
-nouveau_hwmon_set_critical_temp_hyst(struct device *d,
-				     struct device_attribute *a,
-				     const char *buf,
-				     size_t count)
+static int
+nouveau_hwmon_critical_temp_hyst(struct nouveau_drm *drm)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
-	long value;
-
-	if (kstrtol(buf, 10, &value) == -EINVAL)
-		return count;
-
-	therm->attr_set(therm, NVKM_THERM_ATTR_THRS_CRITICAL_HYST,
-			value / 1000);
 
-	return count;
+	return therm->attr_get(therm, NVKM_THERM_ATTR_THRS_CRITICAL_HYST) * 1000;
 }
-static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IRUGO | S_IWUSR,
-			  nouveau_hwmon_critical_temp_hyst,
-			  nouveau_hwmon_set_critical_temp_hyst, 0);
-static ssize_t
-nouveau_hwmon_emergency_temp(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_therm *therm = nvxx_therm(&drm->client.device);
 
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-	       therm->attr_get(therm, NVKM_THERM_ATTR_THRS_SHUTDOWN) * 1000);
-}
-static ssize_t
-nouveau_hwmon_set_emergency_temp(struct device *d, struct device_attribute *a,
-							    const char *buf,
-								size_t count)
+static int
+nouveau_hwmon_emergency_temp(struct nouveau_drm *drm)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
-	long value;
-
-	if (kstrtol(buf, 10, &value) == -EINVAL)
-		return count;
 
-	therm->attr_set(therm, NVKM_THERM_ATTR_THRS_SHUTDOWN, value / 1000);
-
-	return count;
+	return therm->attr_get(therm, NVKM_THERM_ATTR_THRS_SHUTDOWN) * 1000;
 }
-static SENSOR_DEVICE_ATTR(temp1_emergency, S_IRUGO | S_IWUSR,
-					nouveau_hwmon_emergency_temp,
-					nouveau_hwmon_set_emergency_temp,
-					0);
-
-static ssize_t
-nouveau_hwmon_emergency_temp_hyst(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_therm *therm = nvxx_therm(&drm->client.device);
 
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-	  therm->attr_get(therm, NVKM_THERM_ATTR_THRS_SHUTDOWN_HYST) * 1000);
-}
-static ssize_t
-nouveau_hwmon_set_emergency_temp_hyst(struct device *d,
-				      struct device_attribute *a,
-				      const char *buf,
-				      size_t count)
+static int
+nouveau_hwmon_emergency_temp_hyst(struct nouveau_drm *drm)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
-	long value;
-
-	if (kstrtol(buf, 10, &value) == -EINVAL)
-		return count;
-
-	therm->attr_set(therm, NVKM_THERM_ATTR_THRS_SHUTDOWN_HYST,
-			value / 1000);
 
-	return count;
+	return therm->attr_get(therm, NVKM_THERM_ATTR_THRS_SHUTDOWN_HYST) * 1000;
 }
-static SENSOR_DEVICE_ATTR(temp1_emergency_hyst, S_IRUGO | S_IWUSR,
-					nouveau_hwmon_emergency_temp_hyst,
-					nouveau_hwmon_set_emergency_temp_hyst,
-					0);
-
-static ssize_t nouveau_hwmon_show_name(struct device *dev,
-				      struct device_attribute *attr,
-				      char *buf)
-{
-	return sprintf(buf, "nouveau\n");
-}
-static SENSOR_DEVICE_ATTR(name, S_IRUGO, nouveau_hwmon_show_name, NULL, 0);
 
-static ssize_t nouveau_hwmon_show_update_rate(struct device *dev,
-				      struct device_attribute *attr,
-				      char *buf)
+static int
+nouveau_hwmon_show_update_rate(struct nouveau_drm *drm)
 {
-	return sprintf(buf, "1000\n");
+	return 1000;
 }
-static SENSOR_DEVICE_ATTR(update_rate, S_IRUGO,
-						nouveau_hwmon_show_update_rate,
-						NULL, 0);
 
-static ssize_t
-nouveau_hwmon_show_fan1_input(struct device *d, struct device_attribute *attr,
-			      char *buf)
+static int
+nouveau_hwmon_show_fan1_input(struct nouveau_drm *drm)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
 
-	return snprintf(buf, PAGE_SIZE, "%d\n", nvkm_therm_fan_sense(therm));
+	return nvkm_therm_fan_sense(therm);
 }
-static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, nouveau_hwmon_show_fan1_input,
-			  NULL, 0);
 
- static ssize_t
-nouveau_hwmon_get_pwm1_enable(struct device *d,
-			   struct device_attribute *a, char *buf)
+static int
+nouveau_hwmon_get_pwm1_enable(struct nouveau_drm *drm)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
-	int ret;
 
-	ret = therm->attr_get(therm, NVKM_THERM_ATTR_FAN_MODE);
-	if (ret < 0)
-		return ret;
-
-	return sprintf(buf, "%i\n", ret);
+	return therm->attr_get(therm, NVKM_THERM_ATTR_FAN_MODE);
 }
 
-static ssize_t
-nouveau_hwmon_set_pwm1_enable(struct device *d, struct device_attribute *a,
-			   const char *buf, size_t count)
+static int
+nouveau_hwmon_set_pwm1_enable(struct nouveau_drm *drm, long val)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
-	long value;
-	int ret;
 
-	ret = kstrtol(buf, 10, &value);
-	if (ret)
-		return ret;
-
-	ret = therm->attr_set(therm, NVKM_THERM_ATTR_FAN_MODE, value);
-	if (ret)
-		return ret;
-	else
-		return count;
+	return therm->attr_set(therm, NVKM_THERM_ATTR_FAN_MODE, val);
 }
-static SENSOR_DEVICE_ATTR(pwm1_enable, S_IRUGO | S_IWUSR,
-			  nouveau_hwmon_get_pwm1_enable,
-			  nouveau_hwmon_set_pwm1_enable, 0);
 
-static ssize_t
-nouveau_hwmon_get_pwm1(struct device *d, struct device_attribute *a, char *buf)
+static int
+nouveau_hwmon_get_pwm1(struct nouveau_drm *drm)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
-	int ret;
 
-	ret = therm->fan_get(therm);
-	if (ret < 0)
-		return ret;
-
-	return sprintf(buf, "%i\n", ret);
+	return therm->fan_get(therm);
 }
 
-static ssize_t
-nouveau_hwmon_set_pwm1(struct device *d, struct device_attribute *a,
-		       const char *buf, size_t count)
+static int
+nouveau_hwmon_set_pwm1(struct nouveau_drm *drm, long val)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
-	int ret = -ENODEV;
-	long value;
-
-	if (kstrtol(buf, 10, &value) == -EINVAL)
-		return -EINVAL;
 
-	ret = therm->fan_set(therm, value);
-	if (ret)
-		return ret;
-
-	return count;
+	return therm->fan_set(therm, val);
 }
 
-static SENSOR_DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR,
-			  nouveau_hwmon_get_pwm1,
-			  nouveau_hwmon_set_pwm1, 0);
-
 static ssize_t
 nouveau_hwmon_get_pwm1_min(struct device *d,
 			   struct device_attribute *a, char *buf)
@@ -515,12 +299,9 @@ static SENSOR_DEVICE_ATTR(pwm1_max, S_IRUGO | S_IWUSR,
 			  nouveau_hwmon_get_pwm1_max,
 			  nouveau_hwmon_set_pwm1_max, 0);
 
-static ssize_t
-nouveau_hwmon_get_in0_input(struct device *d,
-			    struct device_attribute *a, char *buf)
+static int
+nouveau_hwmon_get_in0_input(struct nouveau_drm *drm)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_volt *volt = nvxx_volt(&drm->client.device);
 	int ret;
 
@@ -528,170 +309,55 @@ nouveau_hwmon_get_in0_input(struct device *d,
 	if (ret < 0)
 		return ret;
 
-	return sprintf(buf, "%i\n", ret / 1000);
+	return (ret / 1000);
 }
 
-static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO,
-			  nouveau_hwmon_get_in0_input, NULL, 0);
-
-static ssize_t
-nouveau_hwmon_get_in0_min(struct device *d,
-			    struct device_attribute *a, char *buf)
+static int
+nouveau_hwmon_get_in0_min(struct nouveau_drm *drm)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_volt *volt = nvxx_volt(&drm->client.device);
 
 	if (!volt || !volt->min_uv)
 		return -ENODEV;
 
-	return sprintf(buf, "%i\n", volt->min_uv / 1000);
+	return (volt->min_uv / 1000);
 }
 
-static SENSOR_DEVICE_ATTR(in0_min, S_IRUGO,
-			  nouveau_hwmon_get_in0_min, NULL, 0);
-
 static ssize_t
-nouveau_hwmon_get_in0_max(struct device *d,
-			    struct device_attribute *a, char *buf)
+nouveau_hwmon_get_in0_max(struct nouveau_drm *drm)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_volt *volt = nvxx_volt(&drm->client.device);
 
 	if (!volt || !volt->max_uv)
 		return -ENODEV;
 
-	return sprintf(buf, "%i\n", volt->max_uv / 1000);
+	return (volt->max_uv / 1000);
 }
 
-static SENSOR_DEVICE_ATTR(in0_max, S_IRUGO,
-			  nouveau_hwmon_get_in0_max, NULL, 0);
-
-static ssize_t
-nouveau_hwmon_get_in0_label(struct device *d,
-			    struct device_attribute *a, char *buf)
-{
-	return sprintf(buf, "GPU core\n");
-}
-
-static SENSOR_DEVICE_ATTR(in0_label, S_IRUGO,
-			  nouveau_hwmon_get_in0_label, NULL, 0);
-
-static ssize_t
-nouveau_hwmon_get_power1_input(struct device *d, struct device_attribute *a,
-			       char *buf)
+static int
+nouveau_hwmon_get_power1_input(struct nouveau_drm *drm)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->client.device);
 	int result = nvkm_iccsense_read_all(iccsense);
 
-	if (result < 0)
-		return result;
-
-	return sprintf(buf, "%i\n", result);
+	return result;
 }
 
-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)
+static int
+nouveau_hwmon_get_power1_max(struct nouveau_drm *drm)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->client.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);
+	return iccsense->power_w_max;
+}
 
-static ssize_t
-nouveau_hwmon_get_power1_crit(struct device *d, struct device_attribute *a,
-			      char *buf)
+static int
+nouveau_hwmon_get_power1_crit(struct nouveau_drm *drm)
 {
-	struct drm_device *dev = dev_get_drvdata(d);
-	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->client.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,
-	NULL
-};
-static struct attribute *hwmon_temp_attributes[] = {
-	&sensor_dev_attr_temp1_input.dev_attr.attr,
-	&sensor_dev_attr_temp1_auto_point1_pwm.dev_attr.attr,
-	&sensor_dev_attr_temp1_auto_point1_temp.dev_attr.attr,
-	&sensor_dev_attr_temp1_auto_point1_temp_hyst.dev_attr.attr,
-	&sensor_dev_attr_temp1_max.dev_attr.attr,
-	&sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
-	&sensor_dev_attr_temp1_crit.dev_attr.attr,
-	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
-	&sensor_dev_attr_temp1_emergency.dev_attr.attr,
-	&sensor_dev_attr_temp1_emergency_hyst.dev_attr.attr,
-	NULL
-};
-static struct attribute *hwmon_fan_rpm_attributes[] = {
-	&sensor_dev_attr_fan1_input.dev_attr.attr,
-	NULL
-};
-static struct attribute *hwmon_pwm_fan_attributes[] = {
-	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
-	&sensor_dev_attr_pwm1.dev_attr.attr,
-	&sensor_dev_attr_pwm1_min.dev_attr.attr,
-	&sensor_dev_attr_pwm1_max.dev_attr.attr,
-	NULL
-};
 
-static struct attribute *hwmon_in0_attributes[] = {
-	&sensor_dev_attr_in0_input.dev_attr.attr,
-	&sensor_dev_attr_in0_min.dev_attr.attr,
-	&sensor_dev_attr_in0_max.dev_attr.attr,
-	&sensor_dev_attr_in0_label.dev_attr.attr,
-	NULL
-};
-
-static struct attribute *hwmon_power_attributes[] = {
-	&sensor_dev_attr_power1_input.dev_attr.attr,
-	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,
-};
-static const struct attribute_group hwmon_temp_attrgroup = {
-	.attrs = hwmon_temp_attributes,
-};
-static const struct attribute_group hwmon_fan_rpm_attrgroup = {
-	.attrs = hwmon_fan_rpm_attributes,
-};
-static const struct attribute_group hwmon_pwm_fan_attrgroup = {
-	.attrs = hwmon_pwm_fan_attributes,
-};
-static const struct attribute_group hwmon_in0_attrgroup = {
-	.attrs = hwmon_in0_attributes,
-};
-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,
-};
+	return iccsense->power_w_crit;
+}
 
 static const u32 nouveau_config_chip[] = {
 	HWMON_C_UPDATE_INTERVAL,
@@ -883,6 +549,157 @@ nouveau_fan_is_visible(const void *data, u32 attr, int channel)
 	}
 }
 
+static int
+nouveau_chip_read(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct drm_device *drm_dev = dev_get_drvdata(dev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
+
+	switch (attr) {
+	case hwmon_chip_update_interval:
+		*val = nouveau_hwmon_show_update_rate(drm);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int
+nouveau_temp_read(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct drm_device *drm_dev = dev_get_drvdata(dev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
+
+	switch (attr) {
+	case hwmon_temp_input:
+		*val = nouveau_hwmon_show_temp(drm);
+		break;
+	case hwmon_temp_max:
+		*val = nouveau_hwmon_max_temp(drm);
+		break;
+	case hwmon_temp_max_hyst:
+		*val = nouveau_hwmon_max_temp_hyst(drm);
+		break;
+	case hwmon_temp_crit:
+		*val = nouveau_hwmon_critical_temp(drm);
+		break;
+	case hwmon_temp_crit_hyst:
+		*val = nouveau_hwmon_critical_temp_hyst(drm);
+		break;
+	case hwmon_temp_emergency:
+		*val = nouveau_hwmon_emergency_temp(drm);
+		break;
+	case hwmon_temp_emergency_hyst:
+		*val = nouveau_hwmon_emergency_temp_hyst(drm);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int
+nouveau_fan_read(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct drm_device *drm_dev = dev_get_drvdata(dev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
+
+	switch (attr) {
+	case hwmon_fan_input:
+		*val = nouveau_hwmon_show_fan1_input(drm);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int
+nouveau_in_read(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct drm_device *drm_dev = dev_get_drvdata(dev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
+
+	switch (attr) {
+	case hwmon_in_input:
+		*val = nouveau_hwmon_get_in0_input(drm);
+		break;
+	case hwmon_in_min:
+		*val = nouveau_hwmon_get_in0_min(drm);
+		break;
+	case hwmon_in_max:
+		*val = nouveau_hwmon_get_in0_max(drm);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int
+nouveau_pwm_read(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct drm_device *drm_dev = dev_get_drvdata(dev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
+
+	switch (attr) {
+	case hwmon_pwm_enable:
+		*val = nouveau_hwmon_get_pwm1_enable(drm);
+		break;
+	case hwmon_pwm_input:
+		*val = nouveau_hwmon_get_pwm1(drm);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int
+nouveau_power_read(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct drm_device *drm_dev = dev_get_drvdata(dev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
+
+	switch (attr) {
+	case hwmon_power_input:
+		*val = nouveau_hwmon_get_power1_input(drm);
+		break;
+	case hwmon_power_max:
+		*val = nouveau_hwmon_get_power1_max(drm);
+		break;
+	case hwmon_power_crit:
+		*val = nouveau_hwmon_get_power1_crit(drm);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int
+nouveau_pwm_write(struct device *dev, u32 attr, int channel, long val)
+{
+	struct drm_device *drm_dev = dev_get_drvdata(dev);
+	struct nouveau_drm *drm = nouveau_drm(drm_dev);
+
+	switch (attr) {
+	case hwmon_pwm_input:
+		return nouveau_hwmon_set_pwm1(drm, val);
+	case hwmon_pwm_enable:
+		return nouveau_hwmon_set_pwm1_enable(drm, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static umode_t
 nouveau_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr,
 								int channel)
@@ -919,11 +736,45 @@ nouveau_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 	return -EOPNOTSUPP;
 }
 
+static int
+nouveau_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+							int channel, long *val)
+{
+	switch (type) {
+	case hwmon_chip:
+		return nouveau_chip_read(dev, attr, channel, val);
+	case hwmon_temp:
+		return nouveau_temp_read(dev, attr, channel, val);
+	case hwmon_fan:
+		return nouveau_fan_read(dev, attr, channel, val);
+	case hwmon_in:
+		return nouveau_in_read(dev, attr, channel, val);
+	case hwmon_pwm:
+		return nouveau_pwm_read(dev, attr, channel, val);
+	case hwmon_power:
+		return nouveau_power_read(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int
+nouveau_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+							int channel, long val)
+{
+	switch (type) {
+	case hwmon_pwm:
+		return nouveau_pwm_write(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static const struct hwmon_ops nouveau_hwmon_ops = {
 	.is_visible = nouveau_is_visible,
-	.read = NULL,
+	.read = nouveau_read,
 	.read_string = nouveau_read_string,
-	.write = NULL,
+	.write = nouveau_write,
 };
 
 static const struct hwmon_chip_info nouveau_chip_info = {
@@ -938,8 +789,6 @@ nouveau_hwmon_init(struct drm_device *dev)
 #if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE))
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
-	struct nvkm_volt *volt = nvxx_volt(&drm->client.device);
-	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->client.device);
 	struct nouveau_hwmon *hwmon;
 	struct device *hwmon_dev;
 	int ret = 0;
@@ -949,79 +798,16 @@ nouveau_hwmon_init(struct drm_device *dev)
 		return -ENOMEM;
 	hwmon->dev = dev;
 
-	hwmon_dev = hwmon_device_register(dev->dev);
+	hwmon_dev = hwmon_device_register_with_info(dev->dev, "nouveau", dev,
+						&nouveau_chip_info, NULL);
 	if (IS_ERR(hwmon_dev)) {
 		ret = PTR_ERR(hwmon_dev);
 		NV_ERROR(drm, "Unable to register hwmon device: %d\n", ret);
 		return ret;
 	}
-	dev_set_drvdata(hwmon_dev, dev);
-
-	/* set the default attributes */
-	ret = sysfs_create_group(&hwmon_dev->kobj, &hwmon_default_attrgroup);
-	if (ret)
-		goto error;
-
-	if (therm && therm->attr_get && therm->attr_set) {
-		/* if the card has a working thermal sensor */
-		if (nvkm_therm_temp_get(therm) >= 0) {
-			ret = sysfs_create_group(&hwmon_dev->kobj, &hwmon_temp_attrgroup);
-			if (ret)
-				goto error;
-		}
-
-		/* if the card has a pwm fan */
-		/*XXX: incorrect, need better detection for this, some boards have
-		 *     the gpio entries for pwm fan control even when there's no
-		 *     actual fan connected to it... therm table? */
-		if (therm->fan_get && therm->fan_get(therm) >= 0) {
-			ret = sysfs_create_group(&hwmon_dev->kobj,
-						 &hwmon_pwm_fan_attrgroup);
-			if (ret)
-				goto error;
-		}
-	}
-
-	/* if the card can read the fan rpm */
-	if (therm && nvkm_therm_fan_sense(therm) >= 0) {
-		ret = sysfs_create_group(&hwmon_dev->kobj,
-					 &hwmon_fan_rpm_attrgroup);
-		if (ret)
-			goto error;
-	}
-
-	if (volt && nvkm_volt_get(volt) >= 0) {
-		ret = sysfs_create_group(&hwmon_dev->kobj,
-					 &hwmon_in0_attrgroup);
-
-		if (ret)
-			goto error;
-	}
-
-	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;
-
 	return 0;
-
-error:
-	NV_ERROR(drm, "Unable to create some hwmon sysfs files: %d\n", ret);
-	hwmon_device_unregister(hwmon_dev);
-	hwmon->hwmon = NULL;
-	return ret;
 #else
 	return 0;
 #endif
@@ -1033,17 +819,8 @@ nouveau_hwmon_fini(struct drm_device *dev)
 #if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE))
 	struct nouveau_hwmon *hwmon = nouveau_hwmon(dev);
 
-	if (hwmon->hwmon) {
-		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_default_attrgroup);
-		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_temp_attrgroup);
-		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_pwm_fan_attrgroup);
-		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);
-
+	if (hwmon->hwmon)
 		hwmon_device_unregister(hwmon->hwmon);
-	}
 
 	nouveau_drm(dev)->hwmon = NULL;
 	kfree(hwmon);
-- 
2.1.4

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH v2 4/5] nouveau_hwmon: Add support for auto_point attributes

[Oscar Salvador]

This patch creates a special group attributes for attrs like "*auto_point*".
We check if we have support for them, and if we do, we gather them all in
an attribute_group's structure which is the parameter regarding special groups
of hwmon_device_register_with_info.
---
 drivers/gpu/drm/nouveau/nouveau_hwmon.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
index 538bf67..655ae11 100644
--- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
@@ -359,6 +359,23 @@ nouveau_hwmon_get_power1_crit(struct nouveau_drm *drm)
 	return iccsense->power_w_crit;
 }
 
+static struct attribute *pwm_fan_sensor_attrs[] = {
+	&sensor_dev_attr_pwm1_min.dev_attr.attr,
+	&sensor_dev_attr_pwm1_max.dev_attr.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(pwm_fan_sensor);
+
+static struct attribute *temp1_auto_point_sensor_attrs[] = {
+	&sensor_dev_attr_temp1_auto_point1_pwm.dev_attr.attr,
+	&sensor_dev_attr_temp1_auto_point1_temp.dev_attr.attr,
+	&sensor_dev_attr_temp1_auto_point1_temp_hyst.dev_attr.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(temp1_auto_point_sensor);
+
+#define N_ATTR_GROUPS   3
+
 static const u32 nouveau_config_chip[] = {
 	HWMON_C_UPDATE_INTERVAL,
 	0
@@ -789,17 +806,27 @@ nouveau_hwmon_init(struct drm_device *dev)
 #if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE))
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
+	const struct attribute_group *special_groups[N_ATTR_GROUPS];
 	struct nouveau_hwmon *hwmon;
 	struct device *hwmon_dev;
 	int ret = 0;
+	int i = 0;
 
 	hwmon = drm->hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL);
 	if (!hwmon)
 		return -ENOMEM;
 	hwmon->dev = dev;
 
+	if (therm && therm->attr_get && therm->attr_set) {
+		if (nvkm_therm_temp_get(therm) >= 0)
+			special_groups[i++] = &temp1_auto_point_sensor_group;
+		if (therm->fan_get && therm->fan_get(therm) >= 0)
+			special_groups[i++] = &pwm_fan_sensor_group;
+	}
+
+	special_groups[i] = 0;
 	hwmon_dev = hwmon_device_register_with_info(dev->dev, "nouveau", dev,
-						&nouveau_chip_info, NULL);
+					&nouveau_chip_info, special_groups);
 	if (IS_ERR(hwmon_dev)) {
 		ret = PTR_ERR(hwmon_dev);
 		NV_ERROR(drm, "Unable to register hwmon device: %d\n", ret);
-- 
2.1.4

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH v2 4/5] nouveau_hwmon: Add support for auto_point attributes

[Ilia Mirkin]

On Mon, Apr 17, 2017 at 3:47 AM, Oscar Salvador
<osalvador.vilardaga@gmail.com> wrote:
> This patch creates a special group attributes for attrs like "*auto_point*".
> We check if we have support for them, and if we do, we gather them all in
> an attribute_group's structure which is the parameter regarding special groups
> of hwmon_device_register_with_info.
> ---
>  drivers/gpu/drm/nouveau/nouveau_hwmon.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> index 538bf67..655ae11 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> @@ -359,6 +359,23 @@ nouveau_hwmon_get_power1_crit(struct nouveau_drm *drm)
>         return iccsense->power_w_crit;
>  }
>
> +static struct attribute *pwm_fan_sensor_attrs[] = {

static const struct. Below as well.

> +       &sensor_dev_attr_pwm1_min.dev_attr.attr,
> +       &sensor_dev_attr_pwm1_max.dev_attr.attr,
> +       NULL
> +};
> +ATTRIBUTE_GROUPS(pwm_fan_sensor);
> +
> +static struct attribute *temp1_auto_point_sensor_attrs[] = {
> +       &sensor_dev_attr_temp1_auto_point1_pwm.dev_attr.attr,
> +       &sensor_dev_attr_temp1_auto_point1_temp.dev_attr.attr,
> +       &sensor_dev_attr_temp1_auto_point1_temp_hyst.dev_attr.attr,
> +       NULL
> +};
> +ATTRIBUTE_GROUPS(temp1_auto_point_sensor);
> +
> +#define N_ATTR_GROUPS   3
> +
>  static const u32 nouveau_config_chip[] = {
>         HWMON_C_UPDATE_INTERVAL,
>         0
> @@ -789,17 +806,27 @@ nouveau_hwmon_init(struct drm_device *dev)
>  #if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE))
>         struct nouveau_drm *drm = nouveau_drm(dev);
>         struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> +       const struct attribute_group *special_groups[N_ATTR_GROUPS];
>         struct nouveau_hwmon *hwmon;
>         struct device *hwmon_dev;
>         int ret = 0;
> +       int i = 0;
>
>         hwmon = drm->hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL);
>         if (!hwmon)
>                 return -ENOMEM;
>         hwmon->dev = dev;
>
> +       if (therm && therm->attr_get && therm->attr_set) {
> +               if (nvkm_therm_temp_get(therm) >= 0)
> +                       special_groups[i++] = &temp1_auto_point_sensor_group;
> +               if (therm->fan_get && therm->fan_get(therm) >= 0)
> +                       special_groups[i++] = &pwm_fan_sensor_group;
> +       }
> +
> +       special_groups[i] = 0;
>         hwmon_dev = hwmon_device_register_with_info(dev->dev, "nouveau", dev,
> -                                               &nouveau_chip_info, NULL);
> +                                       &nouveau_chip_info, special_groups);
>         if (IS_ERR(hwmon_dev)) {
>                 ret = PTR_ERR(hwmon_dev);
>                 NV_ERROR(drm, "Unable to register hwmon device: %d\n", ret);
> --
> 2.1.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH v2 5/5] nouveau_hwmon: Change permissions to numeric

[Oscar Salvador]

This patch replaces the symbolic permissions with the numeric ones,
and adds me to the authors too.
---
 drivers/gpu/drm/nouveau/nouveau_hwmon.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
index 655ae11..99e2a3a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
@@ -1,5 +1,6 @@
 /*
- * Copyright 2010 Red Hat Inc.
+ * Copyright 2010 Red Hat Inc. (Ben Skeggs)
+ * Copyright 2017 Oscar Salvador
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the "Software"),
@@ -19,7 +20,6 @@
  * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
  * OTHER DEALINGS IN THE SOFTWARE.
  *
- * Authors: Ben Skeggs
  */
 
 #ifdef CONFIG_ACPI
@@ -56,7 +56,7 @@ nouveau_hwmon_show_temp1_auto_point1_pwm(struct device *d,
 {
 	return snprintf(buf, PAGE_SIZE, "%d\n", 100);
 }
-static SENSOR_DEVICE_ATTR(temp1_auto_point1_pwm, S_IRUGO,
+static SENSOR_DEVICE_ATTR(temp1_auto_point1_pwm, 0444,
 			  nouveau_hwmon_show_temp1_auto_point1_pwm, NULL, 0);
 
 static ssize_t
@@ -88,7 +88,7 @@ nouveau_hwmon_set_temp1_auto_point1_temp(struct device *d,
 
 	return count;
 }
-static SENSOR_DEVICE_ATTR(temp1_auto_point1_temp, S_IRUGO | S_IWUSR,
+static SENSOR_DEVICE_ATTR(temp1_auto_point1_temp, 0644,
 			  nouveau_hwmon_temp1_auto_point1_temp,
 			  nouveau_hwmon_set_temp1_auto_point1_temp, 0);
 
@@ -121,7 +121,7 @@ nouveau_hwmon_set_temp1_auto_point1_temp_hyst(struct device *d,
 
 	return count;
 }
-static SENSOR_DEVICE_ATTR(temp1_auto_point1_temp_hyst, S_IRUGO | S_IWUSR,
+static SENSOR_DEVICE_ATTR(temp1_auto_point1_temp_hyst, 0644,
 			  nouveau_hwmon_temp1_auto_point1_temp_hyst,
 			  nouveau_hwmon_set_temp1_auto_point1_temp_hyst, 0);
 
@@ -255,7 +255,7 @@ nouveau_hwmon_set_pwm1_min(struct device *d, struct device_attribute *a,
 	return count;
 }
 
-static SENSOR_DEVICE_ATTR(pwm1_min, S_IRUGO | S_IWUSR,
+static SENSOR_DEVICE_ATTR(pwm1_min, 0644,
 			  nouveau_hwmon_get_pwm1_min,
 			  nouveau_hwmon_set_pwm1_min, 0);
 
@@ -295,7 +295,7 @@ nouveau_hwmon_set_pwm1_max(struct device *d, struct device_attribute *a,
 	return count;
 }
 
-static SENSOR_DEVICE_ATTR(pwm1_max, S_IRUGO | S_IWUSR,
+static SENSOR_DEVICE_ATTR(pwm1_max, 0644,
 			  nouveau_hwmon_get_pwm1_max,
 			  nouveau_hwmon_set_pwm1_max, 0);
 
-- 
2.1.4

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau