Message-ID: <aff161d5-cf6e-421b-8250-35e724397dcf@redhat.com>
Date: Mon, 25 Mar 2024 15:56:39 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Ivor Wanders <ivor@...nders.net>, Maximilian Luz
 <luzmaximilian@...il.com>, Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>, platform-driver-x86@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] platform/surface: platform_profile: add fan
 profile switching

Hi,

On 3/14/24 11:37 PM, Ivor Wanders wrote:
> Change naming from tmp to platform profile to clarify the module may
> interact with both the TMP and FAN subystems. Add functionality that
> switches the fan profile when the platform profile is changed when
> a fan is present.
> 
> Signed-off-by: Ivor Wanders <ivor@...nders.net>
> Link: https://github.com/linux-surface/kernel/pull/145
> Reviewed-by: Maximilian Luz <luzmaximilian@...il.com>

Thank you for your patch, I've applied this patch 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




> ---
> Changes in v2:
>   - Added link entry to commit message.
>   - Use u8 instead of char for the argument of __sam_fan_profile_set.
>   - Made profile and profile_le variable const.
> ---
> ---
>  .../surface/surface_aggregator_registry.c     | 36 +++++---
>  .../surface/surface_platform_profile.c        | 88 ++++++++++++++++---
>  2 files changed, 100 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
> index 035d6b4105cd..79e52eddabd0 100644
> --- a/drivers/platform/surface/surface_aggregator_registry.c
> +++ b/drivers/platform/surface/surface_aggregator_registry.c
> @@ -68,12 +68,26 @@ static const struct software_node ssam_node_bat_sb3base = {
>  	.parent = &ssam_node_hub_base,
>  };
>  
> -/* Platform profile / performance-mode device. */
> -static const struct software_node ssam_node_tmp_pprof = {
> +/* Platform profile / performance-mode device without a fan. */
> +static const struct software_node ssam_node_tmp_perf_profile = {
>  	.name = "ssam:01:03:01:00:01",
>  	.parent = &ssam_node_root,
>  };
>  
> +/* Platform profile / performance-mode device with a fan, such that
> + * the fan controller profile can also be switched.
> + */
> +static const struct property_entry ssam_node_tmp_perf_profile_has_fan[] = {
> +	PROPERTY_ENTRY_BOOL("has_fan"),
> +	{ }
> +};
> +
> +static const struct software_node ssam_node_tmp_perf_profile_with_fan = {
> +	.name = "ssam:01:03:01:00:01",
> +	.parent = &ssam_node_root,
> +	.properties = ssam_node_tmp_perf_profile_has_fan,
> +};
> +
>  /* Fan speed function. */
>  static const struct software_node ssam_node_fan_speed = {
>  	.name = "ssam:01:05:01:01:01",
> @@ -208,7 +222,7 @@ static const struct software_node ssam_node_pos_tablet_switch = {
>   */
>  static const struct software_node *ssam_node_group_gen5[] = {
>  	&ssam_node_root,
> -	&ssam_node_tmp_pprof,
> +	&ssam_node_tmp_perf_profile,
>  	NULL,
>  };
>  
> @@ -219,7 +233,7 @@ static const struct software_node *ssam_node_group_sb3[] = {
>  	&ssam_node_bat_ac,
>  	&ssam_node_bat_main,
>  	&ssam_node_bat_sb3base,
> -	&ssam_node_tmp_pprof,
> +	&ssam_node_tmp_perf_profile,
>  	&ssam_node_bas_dtx,
>  	&ssam_node_hid_base_keyboard,
>  	&ssam_node_hid_base_touchpad,
> @@ -233,7 +247,7 @@ static const struct software_node *ssam_node_group_sl3[] = {
>  	&ssam_node_root,
>  	&ssam_node_bat_ac,
>  	&ssam_node_bat_main,
> -	&ssam_node_tmp_pprof,
> +	&ssam_node_tmp_perf_profile,
>  	&ssam_node_hid_main_keyboard,
>  	&ssam_node_hid_main_touchpad,
>  	&ssam_node_hid_main_iid5,
> @@ -245,7 +259,7 @@ static const struct software_node *ssam_node_group_sl5[] = {
>  	&ssam_node_root,
>  	&ssam_node_bat_ac,
>  	&ssam_node_bat_main,
> -	&ssam_node_tmp_pprof,
> +	&ssam_node_tmp_perf_profile,
>  	&ssam_node_hid_main_keyboard,
>  	&ssam_node_hid_main_touchpad,
>  	&ssam_node_hid_main_iid5,
> @@ -258,7 +272,7 @@ static const struct software_node *ssam_node_group_sls[] = {
>  	&ssam_node_root,
>  	&ssam_node_bat_ac,
>  	&ssam_node_bat_main,
> -	&ssam_node_tmp_pprof,
> +	&ssam_node_tmp_perf_profile,
>  	&ssam_node_pos_tablet_switch,
>  	&ssam_node_hid_sam_keyboard,
>  	&ssam_node_hid_sam_penstash,
> @@ -274,7 +288,7 @@ static const struct software_node *ssam_node_group_slg1[] = {
>  	&ssam_node_root,
>  	&ssam_node_bat_ac,
>  	&ssam_node_bat_main,
> -	&ssam_node_tmp_pprof,
> +	&ssam_node_tmp_perf_profile,
>  	NULL,
>  };
>  
> @@ -283,7 +297,7 @@ static const struct software_node *ssam_node_group_sp7[] = {
>  	&ssam_node_root,
>  	&ssam_node_bat_ac,
>  	&ssam_node_bat_main,
> -	&ssam_node_tmp_pprof,
> +	&ssam_node_tmp_perf_profile,
>  	NULL,
>  };
>  
> @@ -293,7 +307,7 @@ static const struct software_node *ssam_node_group_sp8[] = {
>  	&ssam_node_hub_kip,
>  	&ssam_node_bat_ac,
>  	&ssam_node_bat_main,
> -	&ssam_node_tmp_pprof,
> +	&ssam_node_tmp_perf_profile,
>  	&ssam_node_kip_tablet_switch,
>  	&ssam_node_hid_kip_keyboard,
>  	&ssam_node_hid_kip_penstash,
> @@ -310,7 +324,7 @@ static const struct software_node *ssam_node_group_sp9[] = {
>  	&ssam_node_hub_kip,
>  	&ssam_node_bat_ac,
>  	&ssam_node_bat_main,
> -	&ssam_node_tmp_pprof,
> +	&ssam_node_tmp_perf_profile_with_fan,
>  	&ssam_node_fan_speed,
>  	&ssam_node_pos_tablet_switch,
>  	&ssam_node_hid_kip_keyboard,
> diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
> index a5a3941b3f43..3de864bc6610 100644
> --- a/drivers/platform/surface/surface_platform_profile.c
> +++ b/drivers/platform/surface/surface_platform_profile.c
> @@ -1,7 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  /*
>   * Surface Platform Profile / Performance Mode driver for Surface System
> - * Aggregator Module (thermal subsystem).
> + * Aggregator Module (thermal and fan subsystem).
>   *
>   * Copyright (C) 2021-2022 Maximilian Luz <luzmaximilian@...il.com>
>   */
> @@ -14,6 +14,7 @@
>  
>  #include <linux/surface_aggregator/device.h>
>  
> +// Enum for the platform performance profile sent to the TMP module.
>  enum ssam_tmp_profile {
>  	SSAM_TMP_PROFILE_NORMAL             = 1,
>  	SSAM_TMP_PROFILE_BATTERY_SAVER      = 2,
> @@ -21,15 +22,26 @@ enum ssam_tmp_profile {
>  	SSAM_TMP_PROFILE_BEST_PERFORMANCE   = 4,
>  };
>  
> +// Enum for the fan profile sent to the FAN module. This fan profile is
> +// only sent to the EC if the 'has_fan' property is set. The integers are
> +// not a typo, they differ from the performance profile indices.
> +enum ssam_fan_profile {
> +	SSAM_FAN_PROFILE_NORMAL             = 2,
> +	SSAM_FAN_PROFILE_BATTERY_SAVER      = 1,
> +	SSAM_FAN_PROFILE_BETTER_PERFORMANCE = 3,
> +	SSAM_FAN_PROFILE_BEST_PERFORMANCE   = 4,
> +};
> +
>  struct ssam_tmp_profile_info {
>  	__le32 profile;
>  	__le16 unknown1;
>  	__le16 unknown2;
>  } __packed;
>  
> -struct ssam_tmp_profile_device {
> +struct ssam_platform_profile_device {
>  	struct ssam_device *sdev;
>  	struct platform_profile_handler handler;
> +	bool has_fan;
>  };
>  
>  SSAM_DEFINE_SYNC_REQUEST_CL_R(__ssam_tmp_profile_get, struct ssam_tmp_profile_info, {
> @@ -42,6 +54,13 @@ SSAM_DEFINE_SYNC_REQUEST_CL_W(__ssam_tmp_profile_set, __le32, {
>  	.command_id      = 0x03,
>  });
>  
> +SSAM_DEFINE_SYNC_REQUEST_W(__ssam_fan_profile_set, u8, {
> +	.target_category = SSAM_SSH_TC_FAN,
> +	.target_id = SSAM_SSH_TID_SAM,
> +	.command_id = 0x0e,
> +	.instance_id = 0x01,
> +});
> +
>  static int ssam_tmp_profile_get(struct ssam_device *sdev, enum ssam_tmp_profile *p)
>  {
>  	struct ssam_tmp_profile_info info;
> @@ -57,12 +76,19 @@ static int ssam_tmp_profile_get(struct ssam_device *sdev, enum ssam_tmp_profile
>  
>  static int ssam_tmp_profile_set(struct ssam_device *sdev, enum ssam_tmp_profile p)
>  {
> -	__le32 profile_le = cpu_to_le32(p);
> +	const __le32 profile_le = cpu_to_le32(p);
>  
>  	return ssam_retry(__ssam_tmp_profile_set, sdev, &profile_le);
>  }
>  
> -static int convert_ssam_to_profile(struct ssam_device *sdev, enum ssam_tmp_profile p)
> +static int ssam_fan_profile_set(struct ssam_device *sdev, enum ssam_fan_profile p)
> +{
> +	const u8 profile = p;
> +
> +	return ssam_retry(__ssam_fan_profile_set, sdev->ctrl, &profile);
> +}
> +
> +static int convert_ssam_tmp_to_profile(struct ssam_device *sdev, enum ssam_tmp_profile p)
>  {
>  	switch (p) {
>  	case SSAM_TMP_PROFILE_NORMAL:
> @@ -83,7 +109,8 @@ static int convert_ssam_to_profile(struct ssam_device *sdev, enum ssam_tmp_profi
>  	}
>  }
>  
> -static int convert_profile_to_ssam(struct ssam_device *sdev, enum platform_profile_option p)
> +
> +static int convert_profile_to_ssam_tmp(struct ssam_device *sdev, enum platform_profile_option p)
>  {
>  	switch (p) {
>  	case PLATFORM_PROFILE_LOW_POWER:
> @@ -105,20 +132,42 @@ static int convert_profile_to_ssam(struct ssam_device *sdev, enum platform_profi
>  	}
>  }
>  
> +static int convert_profile_to_ssam_fan(struct ssam_device *sdev, enum platform_profile_option p)
> +{
> +	switch (p) {
> +	case PLATFORM_PROFILE_LOW_POWER:
> +		return SSAM_FAN_PROFILE_BATTERY_SAVER;
> +
> +	case PLATFORM_PROFILE_BALANCED:
> +		return SSAM_FAN_PROFILE_NORMAL;
> +
> +	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
> +		return SSAM_FAN_PROFILE_BETTER_PERFORMANCE;
> +
> +	case PLATFORM_PROFILE_PERFORMANCE:
> +		return SSAM_FAN_PROFILE_BEST_PERFORMANCE;
> +
> +	default:
> +		/* This should have already been caught by platform_profile_store(). */
> +		WARN(true, "unsupported platform profile");
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
>  static int ssam_platform_profile_get(struct platform_profile_handler *pprof,
>  				     enum platform_profile_option *profile)
>  {
> -	struct ssam_tmp_profile_device *tpd;
> +	struct ssam_platform_profile_device *tpd;
>  	enum ssam_tmp_profile tp;
>  	int status;
>  
> -	tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
> +	tpd = container_of(pprof, struct ssam_platform_profile_device, handler);
>  
>  	status = ssam_tmp_profile_get(tpd->sdev, &tp);
>  	if (status)
>  		return status;
>  
> -	status = convert_ssam_to_profile(tpd->sdev, tp);
> +	status = convert_ssam_tmp_to_profile(tpd->sdev, tp);
>  	if (status < 0)
>  		return status;
>  
> @@ -129,21 +178,32 @@ static int ssam_platform_profile_get(struct platform_profile_handler *pprof,
>  static int ssam_platform_profile_set(struct platform_profile_handler *pprof,
>  				     enum platform_profile_option profile)
>  {
> -	struct ssam_tmp_profile_device *tpd;
> +	struct ssam_platform_profile_device *tpd;
>  	int tp;
>  
> -	tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
> +	tpd = container_of(pprof, struct ssam_platform_profile_device, handler);
> +
> +	tp = convert_profile_to_ssam_tmp(tpd->sdev, profile);
> +	if (tp < 0)
> +		return tp;
>  
> -	tp = convert_profile_to_ssam(tpd->sdev, profile);
> +	tp = ssam_tmp_profile_set(tpd->sdev, tp);
>  	if (tp < 0)
>  		return tp;
>  
> -	return ssam_tmp_profile_set(tpd->sdev, tp);
> +	if (tpd->has_fan) {
> +		tp = convert_profile_to_ssam_fan(tpd->sdev, profile);
> +		if (tp < 0)
> +			return tp;
> +		tp = ssam_fan_profile_set(tpd->sdev, tp);
> +	}
> +
> +	return tp;
>  }
>  
>  static int surface_platform_profile_probe(struct ssam_device *sdev)
>  {
> -	struct ssam_tmp_profile_device *tpd;
> +	struct ssam_platform_profile_device *tpd;
>  
>  	tpd = devm_kzalloc(&sdev->dev, sizeof(*tpd), GFP_KERNEL);
>  	if (!tpd)
> @@ -154,6 +214,8 @@ static int surface_platform_profile_probe(struct ssam_device *sdev)
>  	tpd->handler.profile_get = ssam_platform_profile_get;
>  	tpd->handler.profile_set = ssam_platform_profile_set;
>  
> +	tpd->has_fan = device_property_read_bool(&sdev->dev, "has_fan");
> +
>  	set_bit(PLATFORM_PROFILE_LOW_POWER, tpd->handler.choices);
>  	set_bit(PLATFORM_PROFILE_BALANCED, tpd->handler.choices);
>  	set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE, tpd->handler.choices);

Powered by blists - more mailing lists