Received-SPF: pass (google.com: domain of linux-kernel+bounces-7662-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Sender: Guenter Roeck <groeck7@gmail.com>
Date: Wed, 20 Dec 2023 15:55:14 -0800
From: Guenter Roeck <linux@roeck-us.net>
To: Ivor Wanders <ivor@iwanders.net>
Cc: Jean Delvare <jdelvare@suse.com>, Jonathan Corbet <corbet@lwn.net>,
	Maximilian Luz <luzmaximilian@gmail.com>,
	Hans de Goede <hdegoede@redhat.com>,
	Mark Gross <markgross@kernel.org>, linux-hwmon@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] hwmon: add fan speed monitoring driver for Surface
 devices
Message-ID: <ab8a1ff3-6d01-4331-ba5d-d677d1ad80b5@roeck-us.net>
References: <20231220234415.5219-1-ivor@iwanders.net>
 <20231220234415.5219-2-ivor@iwanders.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231220234415.5219-2-ivor@iwanders.net>

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 <ivor@iwanders.net>
> 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 <ivor@iwanders.net>
> +
> +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 <luzmaximilian@gmail.com>
> +M:	Ivor Wanders <ivor@iwanders.net>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/surface_fan.rst
> +F:	drivers/hwmon/surface_fan.c
> +
>  MICROSOFT SURFACE GPE LID SUPPORT DRIVER
>  M:	Maximilian Luz <luzmaximilian@gmail.com>
>  L:	platform-driver-x86@vger.kernel.org
> 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 <ivor@iwanders.net>
> + */
> +
> +#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