[PATCH v3 0/2] platform/x86: toshiba_acpi: HWMON Fan RPM support

[PATCH v3 0/2] platform/x86: toshiba_acpi: HWMON Fan RPM support

[Arvid Norlander]

Hi,

Lets hope for third time being the charm!

Changelog
=========
v2: Fixed feedback on usage of HWMON interfaces in patch 2.
v3: Fixed #ifdef handling in patch 2.

Fan
===

Currently /sys/bus/acpi/devices/TOS6208:00/fan allows controlling the fan
by writing 0 (off) or 1 (on at low speed). However when reading I have
observed values up to 64 (fan at full speed during prime95 stress test).

Removing the check for "zero or one" shows that on the Z830 at least 64
levels do indeed seem possible. In fact higher values can be written.

But anything above ~50 seems to max out the RPM.

I don't know how to detect the supported range, so I have not created a
patch for this. Advice is welcome.


Fan RPM
=======

There is a way to read Fan RPM:

#define HCI_FAN_RPM 0x45

This one is weird. On windows I have observed the cooling self test program
(which supposedly verifies that the cooling is working correctly) calling
this a few different ways. Here is a summary of what I managed to figure
out:

HCI_SET, 0x45, 0, 1, 0, 0: This sets the fan to run at max speed, but it
will not be visible when reading /sys/bus/acpi/devices/TOS6208:00/fan.
I will refer to this operation as "set-max-fan" below.

The only way I found to stop it running at max RPM is to use HCI_FAN
(e.g. 0 > /sys/bus/acpi/devices/TOS6208:00/fan or call the ACPI method
directly).

However the get method is more interesting:

HCI_GET, 0x45, 0, 1, 0, 0 returns: {0x0, 0x45, fan_rpm, 0x1db0, 0x0, 0x0}

I believe fan_rpm is accurate, without any scaling factors needed:
* It behaves properly (higher value when fan is louder/faster/higher
  pitched, 0 when fan is off).
* It matches the value range reported by HwInfo64 on Windows (which seems
  to be able to read this, I have not looked into how it does that).
* Unfortunately there is no tool by Toshiba that exposes the numerical
  value that I can find (that would have been ideal). Nor is it shown in
  BIOS. The Windows tools "Toshiba PC Health Monitor" reports everything in
  percentages. Yes even the temperatures!
* It is definitely a loud and whiny fan, even by laptop standards, so the
  high reported RPM range of 3540-7600 RPM could make sense. Though it did
  seem a bit high.
* Finally, to be sure, I borrowed a tachometer from work. Yes, the fan
  really spins that fast. Byt it is only 30 mm, so I guess that makes
  sense.

HCI_GET 0x45, 0, 0, 0, 0 returns: {0x0, 0x45, fan_rpm, 0x0, 0x0, 0x0}

The Windows software does *not* use this variant as far as I have observed.
It appears to work the same except that it doesn't return 0x1db0 in index 3.

I'm not sure, but I strongly suspect 0x1db0 could be the max RPM (7600).
The most I have observed when using "set-max-fan" is 0x1da6 (7590 RPM),
which is very close. Note that this is significantly more than I can get
using just HCI_FAN, which seems to max out at 0x17ac (6060 RPM).


Patches
=======

I'm not personally particularly interested in user space control of fan
speed, plus the fact that there is a way to make the fan go faster than
the *other* max speed makes me wonder about the safety of running the fan
at that speed for prolonged periods of time. Thus, I have only added a
read-only hwmon interface for reading the fan RPM.

I elected to use the same call that the Windows code does, which fetches
what I believe is the max RPM. I think it is safer to stay as close as
possible to that code. However I don't currently make use of this value,
suggestions for where to use it are welcome.

Note! I assume that if the FAN RPM call do not result in an error, that
it is in fact supported. This may not be true. I would welcome testing by
anyone who owns a Toshiba laptop!

Best regards,
Arvid Norlander

Arvid Norlander (2):
  platform/x86: toshiba_acpi: Add fan RPM reading (internals)
  platform/x86: toshiba_acpi: Add fan RPM reading (hwmon interface)

 drivers/platform/x86/Kconfig        |   1 +
 drivers/platform/x86/toshiba_acpi.c | 100 ++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)


base-commit: b90cb1053190353cc30f0fef0ef1f378ccc063c5
-- 
2.37.3

[PATCH 1/2] platform/x86: toshiba_acpi: Add fan RPM reading (internals)

[Arvid Norlander]

This add the internal feature detection and reading function for fan RPM.

The approach is based on tracing ACPI calls using AMLI (a tracer/debugger
built into ACPI.sys) while using the Windows cooling self-test software.

The call used is {HCI_GET, 0x45, 0, 1, 0, 0} which returns:
{0x0, 0x45, fan_rpm, probably_max_rpm, 0x0, 0x0}

What is probably the max RPM is not currently used.

Signed-off-by: Arvid Norlander <lkml@vorpal.se>
---
 drivers/platform/x86/toshiba_acpi.c | 30 +++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 0fc9e8b8827b..02e3522f4eeb 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -106,6 +106,7 @@ MODULE_LICENSE("GPL");
 #define HCI_VIDEO_OUT			0x001c
 #define HCI_HOTKEY_EVENT		0x001e
 #define HCI_LCD_BRIGHTNESS		0x002a
+#define HCI_FAN_RPM			0x0045
 #define HCI_WIRELESS			0x0056
 #define HCI_ACCELEROMETER		0x006d
 #define HCI_COOLING_METHOD		0x007f
@@ -185,6 +186,7 @@ struct toshiba_acpi_dev {
 	unsigned int illumination_supported:1;
 	unsigned int video_supported:1;
 	unsigned int fan_supported:1;
+	unsigned int fan_rpm_supported:1;
 	unsigned int system_event_supported:1;
 	unsigned int ntfy_supported:1;
 	unsigned int info_supported:1;
@@ -1616,6 +1618,29 @@ static const struct proc_ops fan_proc_ops = {
 	.proc_write	= fan_proc_write,
 };
 
+/* Fan RPM */
+static int get_fan_rpm(struct toshiba_acpi_dev *dev, u32 *rpm)
+{
+	u32 in[TCI_WORDS] = { HCI_GET, HCI_FAN_RPM, 0, 1, 0, 0 };
+	u32 out[TCI_WORDS];
+	acpi_status status = tci_raw(dev, in, out);
+
+	if (ACPI_FAILURE(status)) {
+		pr_err("ACPI call to get Fan speed failed\n");
+		return -EIO;
+	}
+
+	if (out[0] == TOS_NOT_SUPPORTED)
+		return -ENODEV;
+
+	if (out[0] == TOS_SUCCESS) {
+		*rpm = out[2];
+		return 0;
+	}
+
+	return -EIO;
+}
+
 static int keys_proc_show(struct seq_file *m, void *v)
 {
 	struct toshiba_acpi_dev *dev = m->private;
@@ -2928,6 +2953,8 @@ static void print_supported_features(struct toshiba_acpi_dev *dev)
 		pr_cont(" video-out");
 	if (dev->fan_supported)
 		pr_cont(" fan");
+	if (dev->fan_rpm_supported)
+		pr_cont(" fan-rpm");
 	if (dev->tr_backlight_supported)
 		pr_cont(" transflective-backlight");
 	if (dev->illumination_supported)
@@ -3157,6 +3184,9 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
 	ret = get_fan_status(dev, &dummy);
 	dev->fan_supported = !ret;
 
+	ret = get_fan_rpm(dev, &dummy);
+	dev->fan_rpm_supported = !ret;
+
 	toshiba_wwan_available(dev);
 	if (dev->wwan_supported)
 		toshiba_acpi_setup_wwan_rfkill(dev);
-- 
2.37.3

[PATCH v3 2/2] platform/x86: toshiba_acpi: Add fan RPM reading (hwmon interface)

[Arvid Norlander]

This expands on the previous commit, exporting the fan RPM via hwmon.

This will look something like the following when using the "sensors"
command from lm_sensors:

toshiba_acpi_sensors-acpi-0
Adapter: ACPI interface
fan1:           0 RPM

Signed-off-by: Arvid Norlander <lkml@vorpal.se>
---
 drivers/platform/x86/Kconfig        |  1 +
 drivers/platform/x86/toshiba_acpi.c | 70 +++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index f2f98e942cf2..4d0d2676939a 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -797,6 +797,7 @@ config ACPI_TOSHIBA
 	depends on INPUT
 	depends on SERIO_I8042 || SERIO_I8042 = n
 	depends on ACPI_VIDEO || ACPI_VIDEO = n
+	depends on HWMON || HWMON = n
 	depends on RFKILL || RFKILL = n
 	depends on IIO
 	select INPUT_SPARSEKMAP
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 02e3522f4eeb..0949b1bcab83 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -42,10 +42,12 @@
 #include <linux/uaccess.h>
 #include <linux/miscdevice.h>
 #include <linux/rfkill.h>
+#include <linux/hwmon.h>
 #include <linux/iio/iio.h>
 #include <linux/toshiba.h>
 #include <acpi/video.h>
 
+
 MODULE_AUTHOR("John Belmonte");
 MODULE_DESCRIPTION("Toshiba Laptop ACPI Extras Driver");
 MODULE_LICENSE("GPL");
@@ -171,6 +173,9 @@ struct toshiba_acpi_dev {
 	struct miscdevice miscdev;
 	struct rfkill *wwan_rfk;
 	struct iio_dev *indio_dev;
+#if IS_ENABLED(CONFIG_HWMON)
+	struct device *hwmon_device;
+#endif
 
 	int force_fan;
 	int last_key_event;
@@ -2941,6 +2946,54 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
 	return 0;
 }
 
+/* HWMON support for fan */
+#if IS_ENABLED(CONFIG_HWMON)
+umode_t toshiba_acpi_hwmon_is_visible(const void *drvdata,
+				      enum hwmon_sensor_types type,
+				      u32 attr, int channel)
+{
+	return 0444;
+}
+
+int toshiba_acpi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+			    u32 attr, int channel, long *val)
+{
+	/*
+	 * There is only a single channel and single attribute (for the
+	 * fan) at this point.
+	 * This can be replaced with more advanced logic in the future,
+	 * should the need arise.
+	 */
+	if (type == hwmon_fan && channel == 0 && attr == hwmon_fan_input) {
+		u32 value;
+		int ret;
+
+		ret = get_fan_rpm(toshiba_acpi, &value);
+		if (ret)
+			return ret;
+
+		*val = value;
+		return 0;
+	}
+	return -EOPNOTSUPP;
+}
+
+static const struct hwmon_channel_info *toshiba_acpi_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
+	NULL
+};
+
+static const struct hwmon_ops toshiba_acpi_hwmon_ops = {
+	.is_visible = toshiba_acpi_hwmon_is_visible,
+	.read = toshiba_acpi_hwmon_read,
+};
+
+static const struct hwmon_chip_info toshiba_acpi_hwmon_chip_info = {
+	.ops = &toshiba_acpi_hwmon_ops,
+	.info = toshiba_acpi_hwmon_info,
+};
+#endif
+
 static void print_supported_features(struct toshiba_acpi_dev *dev)
 {
 	pr_info("Supported laptop features:");
@@ -2995,6 +3048,11 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
 
 	remove_toshiba_proc_entries(dev);
 
+#if IS_ENABLED(CONFIG_HWMON)
+	if (dev->hwmon_device)
+		hwmon_device_unregister(dev->hwmon_device);
+#endif
+
 	if (dev->accelerometer_supported && dev->indio_dev) {
 		iio_device_unregister(dev->indio_dev);
 		iio_device_free(dev->indio_dev);
@@ -3187,6 +3245,18 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
 	ret = get_fan_rpm(dev, &dummy);
 	dev->fan_rpm_supported = !ret;
 
+#if IS_ENABLED(CONFIG_HWMON)
+	if (dev->fan_rpm_supported) {
+		dev->hwmon_device = hwmon_device_register_with_info(
+			&dev->acpi_dev->dev, "toshiba_acpi_sensors", NULL,
+			&toshiba_acpi_hwmon_chip_info, NULL);
+		if (IS_ERR(dev->hwmon_device)) {
+			dev->hwmon_device = NULL;
+			pr_warn("unable to register hwmon device, skipping\n");
+		}
+	}
+#endif
+
 	toshiba_wwan_available(dev);
 	if (dev->wwan_supported)
 		toshiba_acpi_setup_wwan_rfkill(dev);
-- 
2.37.3

Re: [PATCH v3 2/2] platform/x86: toshiba_acpi: Add fan RPM reading (hwmon interface)

[Guenter Roeck]

On Fri, Sep 02, 2022 at 07:40:18PM +0200, Arvid Norlander wrote:
> This expands on the previous commit, exporting the fan RPM via hwmon.
> 
> This will look something like the following when using the "sensors"
> command from lm_sensors:
> 
> toshiba_acpi_sensors-acpi-0
> Adapter: ACPI interface
> fan1:           0 RPM
> 
> Signed-off-by: Arvid Norlander <lkml@vorpal.se>
> ---
>  drivers/platform/x86/Kconfig        |  1 +
>  drivers/platform/x86/toshiba_acpi.c | 70 +++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index f2f98e942cf2..4d0d2676939a 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -797,6 +797,7 @@ config ACPI_TOSHIBA
>  	depends on INPUT
>  	depends on SERIO_I8042 || SERIO_I8042 = n
>  	depends on ACPI_VIDEO || ACPI_VIDEO = n
> +	depends on HWMON || HWMON = n
>  	depends on RFKILL || RFKILL = n
>  	depends on IIO
>  	select INPUT_SPARSEKMAP
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 02e3522f4eeb..0949b1bcab83 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -42,10 +42,12 @@
>  #include <linux/uaccess.h>
>  #include <linux/miscdevice.h>
>  #include <linux/rfkill.h>
> +#include <linux/hwmon.h>
>  #include <linux/iio/iio.h>
>  #include <linux/toshiba.h>
>  #include <acpi/video.h>
>  
> +

Unnecessary double empty line

Otherwise 

Acked-by: Guenter Roeck <linux@roeck-us.net>

Guenter

>  MODULE_AUTHOR("John Belmonte");
>  MODULE_DESCRIPTION("Toshiba Laptop ACPI Extras Driver");
>  MODULE_LICENSE("GPL");
> @@ -171,6 +173,9 @@ struct toshiba_acpi_dev {
>  	struct miscdevice miscdev;
>  	struct rfkill *wwan_rfk;
>  	struct iio_dev *indio_dev;
> +#if IS_ENABLED(CONFIG_HWMON)
> +	struct device *hwmon_device;
> +#endif
>  
>  	int force_fan;
>  	int last_key_event;
> @@ -2941,6 +2946,54 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
>  	return 0;
>  }
>  
> +/* HWMON support for fan */
> +#if IS_ENABLED(CONFIG_HWMON)
> +umode_t toshiba_acpi_hwmon_is_visible(const void *drvdata,
> +				      enum hwmon_sensor_types type,
> +				      u32 attr, int channel)
> +{
> +	return 0444;
> +}
> +
> +int toshiba_acpi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +			    u32 attr, int channel, long *val)
> +{
> +	/*
> +	 * There is only a single channel and single attribute (for the
> +	 * fan) at this point.
> +	 * This can be replaced with more advanced logic in the future,
> +	 * should the need arise.
> +	 */
> +	if (type == hwmon_fan && channel == 0 && attr == hwmon_fan_input) {
> +		u32 value;
> +		int ret;
> +
> +		ret = get_fan_rpm(toshiba_acpi, &value);
> +		if (ret)
> +			return ret;
> +
> +		*val = value;
> +		return 0;
> +	}
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct hwmon_channel_info *toshiba_acpi_hwmon_info[] = {
> +	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
> +	NULL
> +};
> +
> +static const struct hwmon_ops toshiba_acpi_hwmon_ops = {
> +	.is_visible = toshiba_acpi_hwmon_is_visible,
> +	.read = toshiba_acpi_hwmon_read,
> +};
> +
> +static const struct hwmon_chip_info toshiba_acpi_hwmon_chip_info = {
> +	.ops = &toshiba_acpi_hwmon_ops,
> +	.info = toshiba_acpi_hwmon_info,
> +};
> +#endif
> +
>  static void print_supported_features(struct toshiba_acpi_dev *dev)
>  {
>  	pr_info("Supported laptop features:");
> @@ -2995,6 +3048,11 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
>  
>  	remove_toshiba_proc_entries(dev);
>  
> +#if IS_ENABLED(CONFIG_HWMON)
> +	if (dev->hwmon_device)
> +		hwmon_device_unregister(dev->hwmon_device);
> +#endif
> +
>  	if (dev->accelerometer_supported && dev->indio_dev) {
>  		iio_device_unregister(dev->indio_dev);
>  		iio_device_free(dev->indio_dev);
> @@ -3187,6 +3245,18 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>  	ret = get_fan_rpm(dev, &dummy);
>  	dev->fan_rpm_supported = !ret;
>  
> +#if IS_ENABLED(CONFIG_HWMON)
> +	if (dev->fan_rpm_supported) {
> +		dev->hwmon_device = hwmon_device_register_with_info(
> +			&dev->acpi_dev->dev, "toshiba_acpi_sensors", NULL,
> +			&toshiba_acpi_hwmon_chip_info, NULL);
> +		if (IS_ERR(dev->hwmon_device)) {
> +			dev->hwmon_device = NULL;
> +			pr_warn("unable to register hwmon device, skipping\n");
> +		}
> +	}
> +#endif
> +
>  	toshiba_wwan_available(dev);
>  	if (dev->wwan_supported)
>  		toshiba_acpi_setup_wwan_rfkill(dev);
> -- 
> 2.37.3
> 

Re: [PATCH v3 0/2] platform/x86: toshiba_acpi: HWMON Fan RPM support

[Hans de Goede]

Hi,

On 9/2/22 19:40, Arvid Norlander wrote:
> Hi,
> 
> Lets hope for third time being the charm!
> 
> Changelog
> =========
> v2: Fixed feedback on usage of HWMON interfaces in patch 2.
> v3: Fixed #ifdef handling in patch 2.
> 
> Fan
> ===
> 
> Currently /sys/bus/acpi/devices/TOS6208:00/fan allows controlling the fan
> by writing 0 (off) or 1 (on at low speed). However when reading I have
> observed values up to 64 (fan at full speed during prime95 stress test).
> 
> Removing the check for "zero or one" shows that on the Z830 at least 64
> levels do indeed seem possible. In fact higher values can be written.
> 
> But anything above ~50 seems to max out the RPM.
> 
> I don't know how to detect the supported range, so I have not created a
> patch for this. Advice is welcome.
> 
> 
> Fan RPM
> =======
> 
> There is a way to read Fan RPM:
> 
> #define HCI_FAN_RPM 0x45
> 
> This one is weird. On windows I have observed the cooling self test program
> (which supposedly verifies that the cooling is working correctly) calling
> this a few different ways. Here is a summary of what I managed to figure
> out:
> 
> HCI_SET, 0x45, 0, 1, 0, 0: This sets the fan to run at max speed, but it
> will not be visible when reading /sys/bus/acpi/devices/TOS6208:00/fan.
> I will refer to this operation as "set-max-fan" below.
> 
> The only way I found to stop it running at max RPM is to use HCI_FAN
> (e.g. 0 > /sys/bus/acpi/devices/TOS6208:00/fan or call the ACPI method
> directly).
> 
> However the get method is more interesting:
> 
> HCI_GET, 0x45, 0, 1, 0, 0 returns: {0x0, 0x45, fan_rpm, 0x1db0, 0x0, 0x0}
> 
> I believe fan_rpm is accurate, without any scaling factors needed:
> * It behaves properly (higher value when fan is louder/faster/higher
>   pitched, 0 when fan is off).
> * It matches the value range reported by HwInfo64 on Windows (which seems
>   to be able to read this, I have not looked into how it does that).
> * Unfortunately there is no tool by Toshiba that exposes the numerical
>   value that I can find (that would have been ideal). Nor is it shown in
>   BIOS. The Windows tools "Toshiba PC Health Monitor" reports everything in
>   percentages. Yes even the temperatures!
> * It is definitely a loud and whiny fan, even by laptop standards, so the
>   high reported RPM range of 3540-7600 RPM could make sense. Though it did
>   seem a bit high.
> * Finally, to be sure, I borrowed a tachometer from work. Yes, the fan
>   really spins that fast. Byt it is only 30 mm, so I guess that makes
>   sense.
> 
> HCI_GET 0x45, 0, 0, 0, 0 returns: {0x0, 0x45, fan_rpm, 0x0, 0x0, 0x0}
> 
> The Windows software does *not* use this variant as far as I have observed.
> It appears to work the same except that it doesn't return 0x1db0 in index 3.
> 
> I'm not sure, but I strongly suspect 0x1db0 could be the max RPM (7600).
> The most I have observed when using "set-max-fan" is 0x1da6 (7590 RPM),
> which is very close. Note that this is significantly more than I can get
> using just HCI_FAN, which seems to max out at 0x17ac (6060 RPM).
> 
> 
> Patches
> =======
> 
> I'm not personally particularly interested in user space control of fan
> speed, plus the fact that there is a way to make the fan go faster than
> the *other* max speed makes me wonder about the safety of running the fan
> at that speed for prolonged periods of time. Thus, I have only added a
> read-only hwmon interface for reading the fan RPM.
> 
> I elected to use the same call that the Windows code does, which fetches
> what I believe is the max RPM. I think it is safer to stay as close as
> possible to that code. However I don't currently make use of this value,
> suggestions for where to use it are welcome.
> 
> Note! I assume that if the FAN RPM call do not result in an error, that
> it is in fact supported. This may not be true. I would welcome testing by
> anyone who owns a Toshiba laptop!
> 
> Best regards,
> Arvid Norlander
> 
> Arvid Norlander (2):
>   platform/x86: toshiba_acpi: Add fan RPM reading (internals)
>   platform/x86: toshiba_acpi: Add fan RPM reading (hwmon interface)

Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans