* [PATCH] platform/x86: thinkpad_acpi: Add labels to the first 2 temperature sensors
@ 2021-04-13 7:21 Hans de Goede
[not found] ` <CAHp75VeLrNN3bwERwp3oZg7pRJXMi0y8vAbw97NKoEmm33AfgQ@mail.gmail.com>
2021-04-15 9:10 ` Hans de Goede
0 siblings, 2 replies; 3+ messages in thread
From: Hans de Goede @ 2021-04-13 7:21 UTC (permalink / raw)
To: Mark Gross, Andy Shevchenko
Cc: Hans de Goede, Mark Pearson, ibm-acpi-devel, platform-driver-x86
On EC version 3, the first 2 temperature sensors are always CPU and GPU
add labels for these.
This changes e.g. the "sensors" command output on a X1C8 from:
thinkpad-isa-0000
Adapter: ISA adapter
fan1: 2694 RPM
temp1: +42.0°C
temp2: N/A
temp3: +33.0°C
temp4: +0.0°C
temp5: +35.0°C
temp6: +42.0°C
temp7: +42.0°C
temp8: N/A
into:
thinkpad-isa-0000
Adapter: ISA adapter
fan1: 2694 RPM
CPU: +42.0°C
GPU: N/A
temp3: +33.0°C
temp4: +0.0°C
temp5: +35.0°C
temp6: +42.0°C
temp7: +42.0°C
temp8: N/A
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/platform/x86/thinkpad_acpi.c | 72 ++++++++++++++++++----------
1 file changed, 47 insertions(+), 25 deletions(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index ec98089d98c9..dd60c9397d35 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -6296,6 +6296,8 @@ struct ibm_thermal_sensors_struct {
};
static enum thermal_access_mode thermal_read_mode;
+static const struct attribute_group *thermal_attr_group;
+static bool thermal_use_labels;
/* idx is zero-based */
static int thermal_get_sensor(int idx, s32 *value)
@@ -6478,6 +6480,28 @@ static const struct attribute_group thermal_temp_input8_group = {
#undef THERMAL_SENSOR_ATTR_TEMP
#undef THERMAL_ATTRS
+static ssize_t temp1_label_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "CPU\n");
+}
+static DEVICE_ATTR_RO(temp1_label);
+
+static ssize_t temp2_label_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "GPU\n");
+}
+static DEVICE_ATTR_RO(temp2_label);
+
+static struct attribute *temp_label_attributes[] = {
+ &dev_attr_temp1_label.attr,
+ &dev_attr_temp2_label.attr,
+ NULL
+};
+
+static const struct attribute_group temp_label_attr_group = {
+ .attrs = temp_label_attributes,
+};
+
/* --------------------------------------------------------------------- */
static int __init thermal_init(struct ibm_init_struct *iibm)
@@ -6533,12 +6557,14 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
thermal_read_mode = TPACPI_THERMAL_NONE;
}
} else {
- if (ver >= 3)
+ if (ver >= 3) {
thermal_read_mode = TPACPI_THERMAL_TPEC_8;
- else
+ thermal_use_labels = true;
+ } else {
thermal_read_mode =
(ta2 != 0) ?
TPACPI_THERMAL_TPEC_16 : TPACPI_THERMAL_TPEC_8;
+ }
}
} else if (acpi_tmp7) {
if (tpacpi_is_ibm() &&
@@ -6560,44 +6586,40 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
switch (thermal_read_mode) {
case TPACPI_THERMAL_TPEC_16:
- res = sysfs_create_group(&tpacpi_hwmon->kobj,
- &thermal_temp_input16_group);
- if (res)
- return res;
+ thermal_attr_group = &thermal_temp_input16_group;
break;
case TPACPI_THERMAL_TPEC_8:
case TPACPI_THERMAL_ACPI_TMP07:
case TPACPI_THERMAL_ACPI_UPDT:
- res = sysfs_create_group(&tpacpi_hwmon->kobj,
- &thermal_temp_input8_group);
- if (res)
- return res;
+ thermal_attr_group = &thermal_temp_input8_group;
break;
case TPACPI_THERMAL_NONE:
default:
return 1;
}
+ res = sysfs_create_group(&tpacpi_hwmon->kobj, thermal_attr_group);
+ if (res)
+ return res;
+
+ if (thermal_use_labels) {
+ res = sysfs_create_group(&tpacpi_hwmon->kobj, &temp_label_attr_group);
+ if (res) {
+ sysfs_remove_group(&tpacpi_hwmon->kobj, thermal_attr_group);
+ return res;
+ }
+ }
+
return 0;
}
static void thermal_exit(void)
{
- switch (thermal_read_mode) {
- case TPACPI_THERMAL_TPEC_16:
- sysfs_remove_group(&tpacpi_hwmon->kobj,
- &thermal_temp_input16_group);
- break;
- case TPACPI_THERMAL_TPEC_8:
- case TPACPI_THERMAL_ACPI_TMP07:
- case TPACPI_THERMAL_ACPI_UPDT:
- sysfs_remove_group(&tpacpi_hwmon->kobj,
- &thermal_temp_input8_group);
- break;
- case TPACPI_THERMAL_NONE:
- default:
- break;
- }
+ if (thermal_attr_group)
+ sysfs_remove_group(&tpacpi_hwmon->kobj, thermal_attr_group);
+
+ if (thermal_use_labels)
+ sysfs_remove_group(&tpacpi_hwmon->kobj, &temp_label_attr_group);
}
static int thermal_read(struct seq_file *m)
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] platform/x86: thinkpad_acpi: Add labels to the first 2 temperature sensors
[not found] ` <CAHp75VeLrNN3bwERwp3oZg7pRJXMi0y8vAbw97NKoEmm33AfgQ@mail.gmail.com>
@ 2021-04-13 9:37 ` Hans de Goede
0 siblings, 0 replies; 3+ messages in thread
From: Hans de Goede @ 2021-04-13 9:37 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Mark Gross, Andy Shevchenko, Mark Pearson, ibm-acpi-devel,
platform-driver-x86
Hi,
On 4/13/21 11:06 AM, Andy Shevchenko wrote:
>
>
> On Tuesday, April 13, 2021, Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote:
>
> On EC version 3, the first 2 temperature sensors are always CPU and GPU
> add labels for these.
>
> This changes e.g. the "sensors" command output on a X1C8 from:
>
> thinkpad-isa-0000
> Adapter: ISA adapter
> fan1: 2694 RPM
> temp1: +42.0°C
> temp2: N/A
> temp3: +33.0°C
> temp4: +0.0°C
> temp5: +35.0°C
> temp6: +42.0°C
> temp7: +42.0°C
> temp8: N/A
>
> into:
>
> thinkpad-isa-0000
> Adapter: ISA adapter
> fan1: 2694 RPM
> CPU: +42.0°C
> GPU: N/A
> temp3: +33.0°C
> temp4: +0.0°C
> temp5: +35.0°C
> temp6: +42.0°C
> temp7: +42.0°C
> temp8: N/A
>
>
> Is it an ABI change?
I don't think so, it adds 2 new sysfs files, all existing files keep working as is.
The _label attributes are part of the standard hwmon API and any tools
implementing that API will either ignore them (if they are ancient) or
use them to set a better default label then "temp1" and "temp2", while
still honoring any labels from config files with a higher priority then
the kernel provided ones.
> Any updates to the documentation?
>
> If the answer is yes, I would rather do one of the following approach:
> 1) enable labels by user request (sysfs knob)
> 2) just add additional two lines
> 3) add an additional column for labels (or completely another file)
>
> I have understand that there are cons of each of them. I dunno which one is the best.
The answer is no :) So no need to worry about this
Regards,
Hans
>
>
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 72 ++++++++++++++++++----------
> 1 file changed, 47 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index ec98089d98c9..dd60c9397d35 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -6296,6 +6296,8 @@ struct ibm_thermal_sensors_struct {
> };
>
> static enum thermal_access_mode thermal_read_mode;
> +static const struct attribute_group *thermal_attr_group;
> +static bool thermal_use_labels;
>
> /* idx is zero-based */
> static int thermal_get_sensor(int idx, s32 *value)
> @@ -6478,6 +6480,28 @@ static const struct attribute_group thermal_temp_input8_group = {
> #undef THERMAL_SENSOR_ATTR_TEMP
> #undef THERMAL_ATTRS
>
> +static ssize_t temp1_label_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + return sysfs_emit(buf, "CPU\n");
> +}
> +static DEVICE_ATTR_RO(temp1_label);
> +
> +static ssize_t temp2_label_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + return sysfs_emit(buf, "GPU\n");
> +}
> +static DEVICE_ATTR_RO(temp2_label);
> +
> +static struct attribute *temp_label_attributes[] = {
> + &dev_attr_temp1_label.attr,
> + &dev_attr_temp2_label.attr,
> + NULL
> +};
> +
> +static const struct attribute_group temp_label_attr_group = {
> + .attrs = temp_label_attributes,
> +};
> +
> /* --------------------------------------------------------------------- */
>
> static int __init thermal_init(struct ibm_init_struct *iibm)
> @@ -6533,12 +6557,14 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
> thermal_read_mode = TPACPI_THERMAL_NONE;
> }
> } else {
> - if (ver >= 3)
> + if (ver >= 3) {
> thermal_read_mode = TPACPI_THERMAL_TPEC_8;
> - else
> + thermal_use_labels = true;
> + } else {
> thermal_read_mode =
> (ta2 != 0) ?
> TPACPI_THERMAL_TPEC_16 : TPACPI_THERMAL_TPEC_8;
> + }
> }
> } else if (acpi_tmp7) {
> if (tpacpi_is_ibm() &&
> @@ -6560,44 +6586,40 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
>
> switch (thermal_read_mode) {
> case TPACPI_THERMAL_TPEC_16:
> - res = sysfs_create_group(&tpacpi_hwmon->kobj,
> - &thermal_temp_input16_group);
> - if (res)
> - return res;
> + thermal_attr_group = &thermal_temp_input16_group;
> break;
> case TPACPI_THERMAL_TPEC_8:
> case TPACPI_THERMAL_ACPI_TMP07:
> case TPACPI_THERMAL_ACPI_UPDT:
> - res = sysfs_create_group(&tpacpi_hwmon->kobj,
> - &thermal_temp_input8_group);
> - if (res)
> - return res;
> + thermal_attr_group = &thermal_temp_input8_group;
> break;
> case TPACPI_THERMAL_NONE:
> default:
> return 1;
> }
>
> + res = sysfs_create_group(&tpacpi_hwmon->kobj, thermal_attr_group);
> + if (res)
> + return res;
> +
> + if (thermal_use_labels) {
> + res = sysfs_create_group(&tpacpi_hwmon->kobj, &temp_label_attr_group);
> + if (res) {
> + sysfs_remove_group(&tpacpi_hwmon->kobj, thermal_attr_group);
> + return res;
> + }
> + }
> +
> return 0;
> }
>
> static void thermal_exit(void)
> {
> - switch (thermal_read_mode) {
> - case TPACPI_THERMAL_TPEC_16:
> - sysfs_remove_group(&tpacpi_hwmon->kobj,
> - &thermal_temp_input16_group);
> - break;
> - case TPACPI_THERMAL_TPEC_8:
> - case TPACPI_THERMAL_ACPI_TMP07:
> - case TPACPI_THERMAL_ACPI_UPDT:
> - sysfs_remove_group(&tpacpi_hwmon->kobj,
> - &thermal_temp_input8_group);
> - break;
> - case TPACPI_THERMAL_NONE:
> - default:
> - break;
> - }
> + if (thermal_attr_group)
> + sysfs_remove_group(&tpacpi_hwmon->kobj, thermal_attr_group);
> +
> + if (thermal_use_labels)
> + sysfs_remove_group(&tpacpi_hwmon->kobj, &temp_label_attr_group);
> }
>
> static int thermal_read(struct seq_file *m)
> --
> 2.31.1
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] platform/x86: thinkpad_acpi: Add labels to the first 2 temperature sensors
2021-04-13 7:21 [PATCH] platform/x86: thinkpad_acpi: Add labels to the first 2 temperature sensors Hans de Goede
[not found] ` <CAHp75VeLrNN3bwERwp3oZg7pRJXMi0y8vAbw97NKoEmm33AfgQ@mail.gmail.com>
@ 2021-04-15 9:10 ` Hans de Goede
1 sibling, 0 replies; 3+ messages in thread
From: Hans de Goede @ 2021-04-15 9:10 UTC (permalink / raw)
To: Mark Gross, Andy Shevchenko
Cc: Mark Pearson, ibm-acpi-devel, platform-driver-x86
Hi,
On 4/13/21 9:21 AM, Hans de Goede wrote:
> On EC version 3, the first 2 temperature sensors are always CPU and GPU
> add labels for these.
>
> This changes e.g. the "sensors" command output on a X1C8 from:
>
> thinkpad-isa-0000
> Adapter: ISA adapter
> fan1: 2694 RPM
> temp1: +42.0°C
> temp2: N/A
> temp3: +33.0°C
> temp4: +0.0°C
> temp5: +35.0°C
> temp6: +42.0°C
> temp7: +42.0°C
> temp8: N/A
>
> into:
>
> thinkpad-isa-0000
> Adapter: ISA adapter
> fan1: 2694 RPM
> CPU: +42.0°C
> GPU: N/A
> temp3: +33.0°C
> temp4: +0.0°C
> temp5: +35.0°C
> temp6: +42.0°C
> temp7: +42.0°C
> temp8: N/A
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Since there have been no remarks (other then Andy's ABI question) on this and a new-merge window is approaching I've merged this into for-next now.
Regards,
Hans
> ---
> drivers/platform/x86/thinkpad_acpi.c | 72 ++++++++++++++++++----------
> 1 file changed, 47 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index ec98089d98c9..dd60c9397d35 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -6296,6 +6296,8 @@ struct ibm_thermal_sensors_struct {
> };
>
> static enum thermal_access_mode thermal_read_mode;
> +static const struct attribute_group *thermal_attr_group;
> +static bool thermal_use_labels;
>
> /* idx is zero-based */
> static int thermal_get_sensor(int idx, s32 *value)
> @@ -6478,6 +6480,28 @@ static const struct attribute_group thermal_temp_input8_group = {
> #undef THERMAL_SENSOR_ATTR_TEMP
> #undef THERMAL_ATTRS
>
> +static ssize_t temp1_label_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + return sysfs_emit(buf, "CPU\n");
> +}
> +static DEVICE_ATTR_RO(temp1_label);
> +
> +static ssize_t temp2_label_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + return sysfs_emit(buf, "GPU\n");
> +}
> +static DEVICE_ATTR_RO(temp2_label);
> +
> +static struct attribute *temp_label_attributes[] = {
> + &dev_attr_temp1_label.attr,
> + &dev_attr_temp2_label.attr,
> + NULL
> +};
> +
> +static const struct attribute_group temp_label_attr_group = {
> + .attrs = temp_label_attributes,
> +};
> +
> /* --------------------------------------------------------------------- */
>
> static int __init thermal_init(struct ibm_init_struct *iibm)
> @@ -6533,12 +6557,14 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
> thermal_read_mode = TPACPI_THERMAL_NONE;
> }
> } else {
> - if (ver >= 3)
> + if (ver >= 3) {
> thermal_read_mode = TPACPI_THERMAL_TPEC_8;
> - else
> + thermal_use_labels = true;
> + } else {
> thermal_read_mode =
> (ta2 != 0) ?
> TPACPI_THERMAL_TPEC_16 : TPACPI_THERMAL_TPEC_8;
> + }
> }
> } else if (acpi_tmp7) {
> if (tpacpi_is_ibm() &&
> @@ -6560,44 +6586,40 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
>
> switch (thermal_read_mode) {
> case TPACPI_THERMAL_TPEC_16:
> - res = sysfs_create_group(&tpacpi_hwmon->kobj,
> - &thermal_temp_input16_group);
> - if (res)
> - return res;
> + thermal_attr_group = &thermal_temp_input16_group;
> break;
> case TPACPI_THERMAL_TPEC_8:
> case TPACPI_THERMAL_ACPI_TMP07:
> case TPACPI_THERMAL_ACPI_UPDT:
> - res = sysfs_create_group(&tpacpi_hwmon->kobj,
> - &thermal_temp_input8_group);
> - if (res)
> - return res;
> + thermal_attr_group = &thermal_temp_input8_group;
> break;
> case TPACPI_THERMAL_NONE:
> default:
> return 1;
> }
>
> + res = sysfs_create_group(&tpacpi_hwmon->kobj, thermal_attr_group);
> + if (res)
> + return res;
> +
> + if (thermal_use_labels) {
> + res = sysfs_create_group(&tpacpi_hwmon->kobj, &temp_label_attr_group);
> + if (res) {
> + sysfs_remove_group(&tpacpi_hwmon->kobj, thermal_attr_group);
> + return res;
> + }
> + }
> +
> return 0;
> }
>
> static void thermal_exit(void)
> {
> - switch (thermal_read_mode) {
> - case TPACPI_THERMAL_TPEC_16:
> - sysfs_remove_group(&tpacpi_hwmon->kobj,
> - &thermal_temp_input16_group);
> - break;
> - case TPACPI_THERMAL_TPEC_8:
> - case TPACPI_THERMAL_ACPI_TMP07:
> - case TPACPI_THERMAL_ACPI_UPDT:
> - sysfs_remove_group(&tpacpi_hwmon->kobj,
> - &thermal_temp_input8_group);
> - break;
> - case TPACPI_THERMAL_NONE:
> - default:
> - break;
> - }
> + if (thermal_attr_group)
> + sysfs_remove_group(&tpacpi_hwmon->kobj, thermal_attr_group);
> +
> + if (thermal_use_labels)
> + sysfs_remove_group(&tpacpi_hwmon->kobj, &temp_label_attr_group);
> }
>
> static int thermal_read(struct seq_file *m)
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-04-15 9:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-13 7:21 [PATCH] platform/x86: thinkpad_acpi: Add labels to the first 2 temperature sensors Hans de Goede
[not found] ` <CAHp75VeLrNN3bwERwp3oZg7pRJXMi0y8vAbw97NKoEmm33AfgQ@mail.gmail.com>
2021-04-13 9:37 ` Hans de Goede
2021-04-15 9:10 ` Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).