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@xxxxxxxxxxxx> > Link: https://github.com/linux-surface/kernel/pull/145 > Reviewed-by: Maximilian Luz <luzmaximilian@xxxxxxxxx> 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@xxxxxxxxx> > */ > @@ -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);
