2023-12-20 23:44:41

by Ivor Wanders

[permalink] [raw]
Subject: [PATCH 0/2] Surface fan monitoring driver

Currently, there is no way to obtain the fan's current speed on Microsoft
Surface Series devices, this patch series adds a new module that provides
read-only access to the fan's current speed through the hwmon system.

This new module relies on the Surface System Aggregator Module which is the
system responsible for communication with the EC on these devices. The
first commit adds an entry into the SSAM registry for the fan's speed
functionality (for the Surface Pro 9), the second commit adds the new
module and documentation. Both patches can be applied independently of each
other. Tested on a Microsoft Surface Pro 9.

A full development log can be found on [1]. Fan control is always handled
by the EC and cannot be influenced directly. It was identified during the
development of this module that there are fan profiles between which can be
switched. I'm currently developing improvements to the Surface platform
profile module in [2] that will be submitted in the future.


[1]: https://github.com/linux-surface/kernel/pull/144
[2]: https://github.com/linux-surface/kernel/pull/145

Ivor Wanders (2):
hwmon: add fan speed monitoring driver for Surface devices
platform/surface: aggregator_registry: add entry for fan speed

Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/surface_fan.rst | 27 ++++
MAINTAINERS | 8 ++
drivers/hwmon/Kconfig | 13 ++
drivers/hwmon/Makefile | 1 +
drivers/hwmon/surface_fan.c | 125 ++++++++++++++++++
.../surface/surface_aggregator_registry.c | 7 +
7 files changed, 182 insertions(+)
create mode 100644 Documentation/hwmon/surface_fan.rst
create mode 100644 drivers/hwmon/surface_fan.c

--
2.17.1



2023-12-20 23:44:49

by Ivor Wanders

[permalink] [raw]
Subject: [PATCH 1/2] hwmon: add fan speed monitoring driver for Surface devices

Adds a driver that provides read only access to the fan speed for Microsoft
Surface Pro devices. The fan speed is always regulated by the EC and cannot
be influenced directly.

Signed-off-by: Ivor Wanders <[email protected]>
Link: https://github.com/linux-surface/kernel/pull/144
---
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/surface_fan.rst | 27 ++++++
MAINTAINERS | 8 ++
drivers/hwmon/Kconfig | 13 +++
drivers/hwmon/Makefile | 1 +
drivers/hwmon/surface_fan.c | 125 ++++++++++++++++++++++++++++
6 files changed, 175 insertions(+)
create mode 100644 Documentation/hwmon/surface_fan.rst
create mode 100644 drivers/hwmon/surface_fan.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 042e1cf95..4dfb3b9bd 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -202,6 +202,7 @@ Hardware Monitoring Kernel Drivers
smsc47m1
sparx5-temp
stpddc60
+ surface_fan
sy7636a-hwmon
tc654
tc74
diff --git a/Documentation/hwmon/surface_fan.rst b/Documentation/hwmon/surface_fan.rst
new file mode 100644
index 000000000..6e27a6653
--- /dev/null
+++ b/Documentation/hwmon/surface_fan.rst
@@ -0,0 +1,27 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver surface_fan
+=========================
+
+Supported Devices:
+
+ * Microsoft Surface Pro 9
+
+Author: Ivor Wanders <[email protected]>
+
+Description
+-----------
+
+This provides monitoring of the fan found in some Microsoft Surface Pro devices,
+like the Surface Pro 9. The fan is always controlled by the onboard controller.
+
+Sysfs interface
+---------------
+
+======================= ======= =========================================
+Name Perm Description
+======================= ======= =========================================
+``fan1_input`` RO Current fan speed in RPM.
+``fan1_max`` RO Approximate maximum fan speed.
+``fan1_min`` RO Minimum fan speed used by the controller.
+======================= ======= =========================================
diff --git a/MAINTAINERS b/MAINTAINERS
index 439cf523b..8e7870af3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14078,6 +14078,14 @@ F: Documentation/driver-api/surface_aggregator/clients/dtx.rst
F: drivers/platform/surface/surface_dtx.c
F: include/uapi/linux/surface_aggregator/dtx.h

+MICROSOFT SURFACE SENSOR FAN DRIVER
+M: Maximilian Luz <[email protected]>
+M: Ivor Wanders <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/hwmon/surface_fan.rst
+F: drivers/hwmon/surface_fan.c
+
MICROSOFT SURFACE GPE LID SUPPORT DRIVER
M: Maximilian Luz <[email protected]>
L: [email protected]
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 307477b8a..4b4d999af 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1965,6 +1965,19 @@ config SENSORS_SMM665
This driver can also be built as a module. If so, the module will
be called smm665.

+config SENSORS_SURFACE_FAN
+ tristate "Surface Fan Driver"
+ depends on SURFACE_AGGREGATOR
+ help
+ Driver that provides monitoring of the fan on Surface Pro devices that
+ have a fan, like the Surface Pro 9.
+
+ This makes the fan's current speed accessible through the hwmon
+ system. It does not provide control over the fan, the firmware is
+ responsible for that, this driver merely provides monitoring.
+
+ Select M or Y here, if you want to be able to read the fan's speed.
+
config SENSORS_ADC128D818
tristate "Texas Instruments ADC128D818"
depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 3f4b0fda0..5ae214c06 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -198,6 +198,7 @@ obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o
obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o
obj-$(CONFIG_SENSORS_STTS751) += stts751.o
+obj-$(CONFIG_SENSORS_SURFACE_FAN)+= surface_fan.o
obj-$(CONFIG_SENSORS_SY7636A) += sy7636a-hwmon.o
obj-$(CONFIG_SENSORS_AMC6821) += amc6821.o
obj-$(CONFIG_SENSORS_TC74) += tc74.o
diff --git a/drivers/hwmon/surface_fan.c b/drivers/hwmon/surface_fan.c
new file mode 100644
index 000000000..7129b25ed
--- /dev/null
+++ b/drivers/hwmon/surface_fan.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Surface Fan driver for Surface System Aggregator Module. It provides access
+ * to the fan's rpm through the hwmon system.
+ *
+ * Copyright (C) 2023 Ivor Wanders <[email protected]>
+ */
+
+#include <linux/hwmon.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/surface_aggregator/device.h>
+#include <linux/types.h>
+
+// The minimum speed for the fan when turned on by the controller. The onboard
+// controller uses this as minimum value before turning the fan on or off.
+#define SURFACE_FAN_MIN_SPEED 3000
+// The maximum speed, determined by observation and rounding up to the nearest
+// multiple of 500 to account for variation between individual fans.
+#define SURFACE_FAN_MAX_SPEED 7500
+
+// SSAM
+SSAM_DEFINE_SYNC_REQUEST_CL_R(__ssam_fan_rpm_get, __le16, {
+ .target_category = SSAM_SSH_TC_FAN,
+ .command_id = 0x01,
+});
+
+// hwmon
+umode_t surface_fan_hwmon_is_visible(const void *drvdata,
+ enum hwmon_sensor_types type, u32 attr,
+ int channel)
+{
+ if (type != hwmon_fan)
+ return 0;
+
+ switch (attr) {
+ case hwmon_fan_input:
+ case hwmon_fan_label:
+ case hwmon_fan_min:
+ case hwmon_fan_max:
+ return 0444;
+ default:
+ return 0;
+ }
+}
+
+static int surface_fan_hwmon_read(struct device *dev,
+ enum hwmon_sensor_types type, u32 attr,
+ int channel, long *val)
+{
+ struct ssam_device *sdev = dev_get_drvdata(dev);
+ __le16 value;
+ int res;
+
+ if (type != hwmon_fan)
+ return -EOPNOTSUPP;
+
+ switch (attr) {
+ case hwmon_fan_input:
+ res = __ssam_fan_rpm_get(sdev, &value);
+ if (res)
+ return -EIO;
+ *val = le16_to_cpu(value);
+ return 0;
+ case hwmon_fan_min:
+ *val = SURFACE_FAN_MIN_SPEED;
+ return 0;
+ case hwmon_fan_max:
+ *val = SURFACE_FAN_MAX_SPEED;
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static const struct hwmon_channel_info *const surface_fan_info[] = {
+ HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_MAX | HWMON_F_MIN),
+ NULL
+};
+
+static const struct hwmon_ops surface_fan_hwmon_ops = {
+ .is_visible = surface_fan_hwmon_is_visible,
+ .read = surface_fan_hwmon_read,
+};
+
+static const struct hwmon_chip_info surface_fan_chip_info = {
+ .ops = &surface_fan_hwmon_ops,
+ .info = surface_fan_info,
+};
+
+static int surface_fan_probe(struct ssam_device *sdev)
+{
+ struct device *hdev;
+
+ hdev = devm_hwmon_device_register_with_info(&sdev->dev, "fan", sdev,
+ &surface_fan_chip_info,
+ NULL);
+ if (IS_ERR(hdev))
+ return PTR_ERR(hdev);
+
+ ssam_device_set_drvdata(sdev, sdev);
+
+ return 0;
+}
+
+static const struct ssam_device_id ssam_fan_match[] = {
+ { SSAM_SDEV(FAN, SAM, 0x01, 0x01) },
+ {},
+};
+MODULE_DEVICE_TABLE(ssam, ssam_fan_match);
+
+static struct ssam_device_driver surface_fan = {
+ .probe = surface_fan_probe,
+ .match_table = ssam_fan_match,
+ .driver = {
+ .name = "surface_fan",
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ },
+};
+module_ssam_device_driver(surface_fan);
+
+MODULE_AUTHOR("Ivor Wanders <[email protected]>");
+MODULE_DESCRIPTION("Fan Driver for Surface System Aggregator Module");
+MODULE_LICENSE("GPL");
--
2.17.1


2023-12-20 23:45:09

by Ivor Wanders

[permalink] [raw]
Subject: [PATCH 2/2] platform/surface: aggregator_registry: add entry for fan speed

Add an entry for the fan speed function.
Add this new entry to the Surface Pro 9 group.

Signed-off-by: Ivor Wanders <[email protected]>
Link: https://github.com/linux-surface/kernel/pull/144
---
drivers/platform/surface/surface_aggregator_registry.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
index 530db4db7..b0db25886 100644
--- a/drivers/platform/surface/surface_aggregator_registry.c
+++ b/drivers/platform/surface/surface_aggregator_registry.c
@@ -74,6 +74,12 @@ static const struct software_node ssam_node_tmp_pprof = {
.parent = &ssam_node_root,
};

+/* Fan speed function. */
+static const struct software_node ssam_node_fan_speed = {
+ .name = "ssam:01:05:01:01:01",
+ .parent = &ssam_node_root,
+};
+
/* Tablet-mode switch via KIP subsystem. */
static const struct software_node ssam_node_kip_tablet_switch = {
.name = "ssam:01:0e:01:00:01",
@@ -319,6 +325,7 @@ static const struct software_node *ssam_node_group_sp9[] = {
&ssam_node_bat_ac,
&ssam_node_bat_main,
&ssam_node_tmp_pprof,
+ &ssam_node_fan_speed,
&ssam_node_pos_tablet_switch,
&ssam_node_hid_kip_keyboard,
&ssam_node_hid_kip_penstash,
--
2.17.1


2023-12-20 23:57:43

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 1/2] hwmon: add fan speed monitoring driver for Surface devices

On Wed, Dec 20, 2023 at 06:44:14PM -0500, Ivor Wanders wrote:
> Adds a driver that provides read only access to the fan speed for Microsoft
> Surface Pro devices. The fan speed is always regulated by the EC and cannot
> be influenced directly.
>
> Signed-off-by: Ivor Wanders <[email protected]>
> Link: https://github.com/linux-surface/kernel/pull/144
> ---
> Documentation/hwmon/index.rst | 1 +
> Documentation/hwmon/surface_fan.rst | 27 ++++++
> MAINTAINERS | 8 ++
> drivers/hwmon/Kconfig | 13 +++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/surface_fan.c | 125 ++++++++++++++++++++++++++++
> 6 files changed, 175 insertions(+)
> create mode 100644 Documentation/hwmon/surface_fan.rst
> create mode 100644 drivers/hwmon/surface_fan.c
>
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 042e1cf95..4dfb3b9bd 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -202,6 +202,7 @@ Hardware Monitoring Kernel Drivers
> smsc47m1
> sparx5-temp
> stpddc60
> + surface_fan
> sy7636a-hwmon
> tc654
> tc74
> diff --git a/Documentation/hwmon/surface_fan.rst b/Documentation/hwmon/surface_fan.rst
> new file mode 100644
> index 000000000..6e27a6653
> --- /dev/null
> +++ b/Documentation/hwmon/surface_fan.rst
> @@ -0,0 +1,27 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver surface_fan
> +=========================
> +
> +Supported Devices:
> +
> + * Microsoft Surface Pro 9
> +
> +Author: Ivor Wanders <[email protected]>
> +
> +Description
> +-----------
> +
> +This provides monitoring of the fan found in some Microsoft Surface Pro devices,
> +like the Surface Pro 9. The fan is always controlled by the onboard controller.
> +
> +Sysfs interface
> +---------------
> +
> +======================= ======= =========================================
> +Name Perm Description
> +======================= ======= =========================================
> +``fan1_input`` RO Current fan speed in RPM.
> +``fan1_max`` RO Approximate maximum fan speed.
> +``fan1_min`` RO Minimum fan speed used by the controller.
> +======================= ======= =========================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 439cf523b..8e7870af3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14078,6 +14078,14 @@ F: Documentation/driver-api/surface_aggregator/clients/dtx.rst
> F: drivers/platform/surface/surface_dtx.c
> F: include/uapi/linux/surface_aggregator/dtx.h
>
> +MICROSOFT SURFACE SENSOR FAN DRIVER
> +M: Maximilian Luz <[email protected]>
> +M: Ivor Wanders <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: Documentation/hwmon/surface_fan.rst
> +F: drivers/hwmon/surface_fan.c
> +
> MICROSOFT SURFACE GPE LID SUPPORT DRIVER
> M: Maximilian Luz <[email protected]>
> L: [email protected]
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 307477b8a..4b4d999af 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1965,6 +1965,19 @@ config SENSORS_SMM665
> This driver can also be built as a module. If so, the module will
> be called smm665.
>
> +config SENSORS_SURFACE_FAN
> + tristate "Surface Fan Driver"
> + depends on SURFACE_AGGREGATOR
> + help
> + Driver that provides monitoring of the fan on Surface Pro devices that
> + have a fan, like the Surface Pro 9.
> +
> + This makes the fan's current speed accessible through the hwmon
> + system. It does not provide control over the fan, the firmware is
> + responsible for that, this driver merely provides monitoring.
> +
> + Select M or Y here, if you want to be able to read the fan's speed.
> +
> config SENSORS_ADC128D818
> tristate "Texas Instruments ADC128D818"
> depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 3f4b0fda0..5ae214c06 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -198,6 +198,7 @@ obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o
> obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
> obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o
> obj-$(CONFIG_SENSORS_STTS751) += stts751.o
> +obj-$(CONFIG_SENSORS_SURFACE_FAN)+= surface_fan.o
> obj-$(CONFIG_SENSORS_SY7636A) += sy7636a-hwmon.o
> obj-$(CONFIG_SENSORS_AMC6821) += amc6821.o
> obj-$(CONFIG_SENSORS_TC74) += tc74.o
> diff --git a/drivers/hwmon/surface_fan.c b/drivers/hwmon/surface_fan.c
> new file mode 100644
> index 000000000..7129b25ed
> --- /dev/null
> +++ b/drivers/hwmon/surface_fan.c
> @@ -0,0 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Surface Fan driver for Surface System Aggregator Module. It provides access
> + * to the fan's rpm through the hwmon system.
> + *
> + * Copyright (C) 2023 Ivor Wanders <[email protected]>
> + */
> +
> +#include <linux/hwmon.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/surface_aggregator/device.h>
> +#include <linux/types.h>
> +
> +// The minimum speed for the fan when turned on by the controller. The onboard
> +// controller uses this as minimum value before turning the fan on or off.
> +#define SURFACE_FAN_MIN_SPEED 3000
> +// The maximum speed, determined by observation and rounding up to the nearest
> +// multiple of 500 to account for variation between individual fans.
> +#define SURFACE_FAN_MAX_SPEED 7500
> +
> +// SSAM
> +SSAM_DEFINE_SYNC_REQUEST_CL_R(__ssam_fan_rpm_get, __le16, {
> + .target_category = SSAM_SSH_TC_FAN,
> + .command_id = 0x01,
> +});
> +
> +// hwmon
> +umode_t surface_fan_hwmon_is_visible(const void *drvdata,
> + enum hwmon_sensor_types type, u32 attr,
> + int channel)
> +{
> + if (type != hwmon_fan)
> + return 0;
> +
> + switch (attr) {
> + case hwmon_fan_input:
> + case hwmon_fan_label:
> + case hwmon_fan_min:
> + case hwmon_fan_max:
> + return 0444;
> + default:
> + return 0;
> + }
> +}
> +
> +static int surface_fan_hwmon_read(struct device *dev,
> + enum hwmon_sensor_types type, u32 attr,
> + int channel, long *val)
> +{
> + struct ssam_device *sdev = dev_get_drvdata(dev);
> + __le16 value;
> + int res;
> +
> + if (type != hwmon_fan)
> + return -EOPNOTSUPP;
> +
> + switch (attr) {
> + case hwmon_fan_input:
> + res = __ssam_fan_rpm_get(sdev, &value);
> + if (res)
> + return -EIO;
> + *val = le16_to_cpu(value);
> + return 0;
> + case hwmon_fan_min:
> + *val = SURFACE_FAN_MIN_SPEED;
> + return 0;
> + case hwmon_fan_max:
> + *val = SURFACE_FAN_MAX_SPEED;
> + return 0;

No, sorry. Limit attributes are supposed to be used to program limits,
not to report constant values to userspace (and please refrain from
referring to other drivers doing the same. Two wrongs don't make it right).

Guenter

2023-12-21 22:55:36

by Ivor Wanders

[permalink] [raw]
Subject: Re: [PATCH 1/2] hwmon: add fan speed monitoring driver for Surface devices

> No, sorry. Limit attributes are supposed to be used to program limits,
> not to report constant values to userspace

Thank you for this feedback, I've proposed improvements to the
documentation in [1] that clarify this. I will incorporate this feedback
and submit a second version.

~Ivor

[1]: https://lore.kernel.org/linux-hwmon/[email protected]/T/

2023-12-22 16:17:13

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 1/2] hwmon: add fan speed monitoring driver for Surface devices

On Thu, Dec 21, 2023 at 05:55:21PM -0500, Ivor Wanders wrote:
> > No, sorry. Limit attributes are supposed to be used to program limits,
> > not to report constant values to userspace
>
> Thank you for this feedback, I've proposed improvements to the
> documentation in [1] that clarify this. I will incorporate this feedback
> and submit a second version.
>
This applies to all hwmon sysfs attributes and is already documented.

Guenter