From: Hans de Goede <hdegoede@redhat.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Mark Gross <mgross@linux.intel.com>,
Andy Shevchenko <andy@infradead.org>,
Mark Pearson <markpearson@lenovo.com>,
"ibm-acpi-devel@lists.sourceforge.net"
<ibm-acpi-devel@lists.sourceforge.net>,
"platform-driver-x86@vger.kernel.org"
<platform-driver-x86@vger.kernel.org>
Subject: Re: [PATCH] platform/x86: thinkpad_acpi: Add labels to the first 2 temperature sensors
Date: Tue, 13 Apr 2021 11:37:31 +0200 [thread overview]
Message-ID: <2e1f570d-841d-da36-ba07-1d74249aef3b@redhat.com> (raw)
In-Reply-To: <CAHp75VeLrNN3bwERwp3oZg7pRJXMi0y8vAbw97NKoEmm33AfgQ@mail.gmail.com>
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
>
>
next prev parent reply other threads:[~2021-04-13 9:37 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2021-04-15 9:10 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2e1f570d-841d-da36-ba07-1d74249aef3b@redhat.com \
--to=hdegoede@redhat.com \
--cc=andy.shevchenko@gmail.com \
--cc=andy@infradead.org \
--cc=ibm-acpi-devel@lists.sourceforge.net \
--cc=markpearson@lenovo.com \
--cc=mgross@linux.intel.com \
--cc=platform-driver-x86@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).