[RFC PATCH 0/3] ACPI: platform_profile: Let drivers dynamically refresh choices

[RFC PATCH 0/3] ACPI: platform_profile: Let drivers dynamically refresh choices

[Kurt Borja]

Hello,

Some drivers may need to dynamically modify their selected `choices`.
Such is the case of the acer-wmi driver, which implemented their own
profile cycling method, because users expect different profiles to be
available whether the laptop is on AC or not [1].

These series would allow acer-wmi to simplify this custom cycling method
to use platform_profile_cycle(), as it's already being proposed in these
series [2]; without changing expected behaviors, by refreshing their
selected choices on AC connect/disconnect events, which would also solve
this discussion [3].

Additionally, I think the platform_profile_ops approach would enable us
to hide the platform_profile_handler in the future, and instead just pass
the class device to get/set methods like the HWMON subsystem does.

I think having this kind of flexibility is valuable. Let me know what you
think!

These series are based on top of pdx86/for-next branch.

~ Kurt

[1] https://lore.kernel.org/platform-driver-x86/6a9385e6-8c5a-4d08-8ff9-728ac40792d2@gmail.com/
[2] https://lore.kernel.org/platform-driver-x86/20250104-platform_profile-v2-0-b58164718903@gmail.com/
[3] https://lore.kernel.org/platform-driver-x86/20241210001657.3362-6-W_Armin@gmx.de/

Kurt Borja (3):
  ACPI: platform_profile: Add ops member to handlers
  ACPI: platform_profile: Add `choices` to platform_profile_ops
  ACPI: platform_profile: Add platform_profile_refresh_choices()

 drivers/acpi/platform_profile.c               | 39 ++++++++++++--
 .../surface/surface_platform_profile.c        | 24 ++++++---
 drivers/platform/x86/acer-wmi.c               | 35 ++++++------
 drivers/platform/x86/amd/pmf/sps.c            | 23 +++++---
 drivers/platform/x86/asus-wmi.c               | 24 ++++++---
 drivers/platform/x86/dell/alienware-wmi.c     | 19 ++++---
 drivers/platform/x86/dell/dell-pc.c           | 34 +++++++-----
 drivers/platform/x86/hp/hp-wmi.c              | 53 +++++++++++++------
 drivers/platform/x86/ideapad-laptop.c         | 23 +++++---
 .../platform/x86/inspur_platform_profile.c    | 22 +++++---
 drivers/platform/x86/thinkpad_acpi.c          | 23 +++++---
 include/linux/platform_profile.h              | 16 ++++--
 12 files changed, 237 insertions(+), 98 deletions(-)


base-commit: 6b228cfc52a6e9b7149cf51e247076963d6561cd
-- 
2.47.1

[RFC PATCH 1/3] ACPI: platform_profile: Add ops member to handlers

[Kurt Borja]

Replace *profile_get and *profile_set members with a general *ops
member.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/acpi/platform_profile.c               |  6 ++---
 .../surface/surface_platform_profile.c        |  8 +++++--
 drivers/platform/x86/acer-wmi.c               | 11 +++++----
 drivers/platform/x86/amd/pmf/sps.c            |  8 +++++--
 drivers/platform/x86/asus-wmi.c               |  8 +++++--
 drivers/platform/x86/dell/alienware-wmi.c     |  8 +++++--
 drivers/platform/x86/dell/dell-pc.c           |  8 +++++--
 drivers/platform/x86/hp/hp-wmi.c              | 24 ++++++++++++++-----
 drivers/platform/x86/ideapad-laptop.c         |  8 +++++--
 .../platform/x86/inspur_platform_profile.c    |  8 +++++--
 drivers/platform/x86/thinkpad_acpi.c          |  8 +++++--
 include/linux/platform_profile.h              | 14 +++++++----
 12 files changed, 86 insertions(+), 33 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 4c4200a0b1a6..68e496ff5176 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -64,7 +64,7 @@ static int _store_class_profile(struct device *dev, void *data)
 	if (!test_bit(*bit, handler->choices))
 		return -EOPNOTSUPP;
 
-	return handler->profile_set(handler, *bit);
+	return handler->ops->set(handler, *bit);
 }
 
 /**
@@ -101,7 +101,7 @@ static int get_class_profile(struct device *dev,
 
 	lockdep_assert_held(&profile_lock);
 	handler = dev_get_drvdata(dev);
-	err = handler->profile_get(handler, &val);
+	err = handler->ops->get(handler, &val);
 	if (err) {
 		pr_err("Failed to get profile for handler %s\n", handler->name);
 		return err;
@@ -465,7 +465,7 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 
 	/* Sanity check the profile handler */
 	if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) ||
-	    !pprof->profile_set || !pprof->profile_get) {
+	    !pprof->ops->set || !pprof->ops->get) {
 		pr_err("platform_profile: handler is invalid\n");
 		return -EINVAL;
 	}
diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
index 6c87e982bfc8..8077d556e616 100644
--- a/drivers/platform/surface/surface_platform_profile.c
+++ b/drivers/platform/surface/surface_platform_profile.c
@@ -201,6 +201,11 @@ static int ssam_platform_profile_set(struct platform_profile_handler *pprof,
 	return tp;
 }
 
+static const struct platform_profile_ops ssam_platform_profile_ops = {
+	.get = ssam_platform_profile_get,
+	.set = ssam_platform_profile_set,
+};
+
 static int surface_platform_profile_probe(struct ssam_device *sdev)
 {
 	struct ssam_platform_profile_device *tpd;
@@ -214,8 +219,7 @@ static int surface_platform_profile_probe(struct ssam_device *sdev)
 
 	tpd->handler.name = "Surface Platform Profile";
 	tpd->handler.dev = &sdev->dev;
-	tpd->handler.profile_get = ssam_platform_profile_get;
-	tpd->handler.profile_set = ssam_platform_profile_set;
+	tpd->handler.ops = &ssam_platform_profile_ops;
 
 	tpd->has_fan = device_property_read_bool(&sdev->dev, "has_fan");
 
diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index b3043d78a7b3..26d6b9cdfd12 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -1900,6 +1900,11 @@ acer_predator_v4_platform_profile_set(struct platform_profile_handler *pprof,
 	return 0;
 }
 
+static const struct platform_profile_ops acer_predator_v4_platform_profile_ops = {
+	.get = acer_predator_v4_platform_profile_get,
+	.set = acer_predator_v4_platform_profile_set,
+};
+
 static int acer_platform_profile_setup(struct platform_device *device)
 {
 	if (quirks->predator_v4) {
@@ -1907,10 +1912,8 @@ static int acer_platform_profile_setup(struct platform_device *device)
 
 		platform_profile_handler.name = "acer-wmi";
 		platform_profile_handler.dev = &device->dev;
-		platform_profile_handler.profile_get =
-			acer_predator_v4_platform_profile_get;
-		platform_profile_handler.profile_set =
-			acer_predator_v4_platform_profile_set;
+		platform_profile_handler.ops =
+			&acer_predator_v4_platform_profile_ops;
 
 		set_bit(PLATFORM_PROFILE_PERFORMANCE,
 			platform_profile_handler.choices);
diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
index bd2bd6cfc39a..1b87254a7120 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -387,6 +387,11 @@ static int amd_pmf_profile_set(struct platform_profile_handler *pprof,
 	return 0;
 }
 
+static const struct platform_profile_ops amd_pmf_profile_ops = {
+	.get = amd_pmf_profile_get,
+	.set = amd_pmf_profile_set,
+};
+
 int amd_pmf_init_sps(struct amd_pmf_dev *dev)
 {
 	int err;
@@ -407,8 +412,7 @@ int amd_pmf_init_sps(struct amd_pmf_dev *dev)
 
 	dev->pprof.name = "amd-pmf";
 	dev->pprof.dev = dev->dev;
-	dev->pprof.profile_get = amd_pmf_profile_get;
-	dev->pprof.profile_set = amd_pmf_profile_set;
+	dev->pprof.ops = &amd_pmf_profile_ops;
 
 	/* Setup supported modes */
 	set_bit(PLATFORM_PROFILE_LOW_POWER, dev->pprof.choices);
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index fdeebab96fc0..d0828b1499b1 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -3852,6 +3852,11 @@ static int asus_wmi_platform_profile_set(struct platform_profile_handler *pprof,
 	return throttle_thermal_policy_write(asus);
 }
 
+static const struct platform_profile_ops asus_wmi_platform_profile_ops = {
+	.get = asus_wmi_platform_profile_get,
+	.set = asus_wmi_platform_profile_set,
+};
+
 static int platform_profile_setup(struct asus_wmi *asus)
 {
 	struct device *dev = &asus->platform_device->dev;
@@ -3878,8 +3883,7 @@ static int platform_profile_setup(struct asus_wmi *asus)
 
 	asus->platform_profile_handler.name = "asus-wmi";
 	asus->platform_profile_handler.dev = dev;
-	asus->platform_profile_handler.profile_get = asus_wmi_platform_profile_get;
-	asus->platform_profile_handler.profile_set = asus_wmi_platform_profile_set;
+	asus->platform_profile_handler.ops = &asus_wmi_platform_profile_ops;
 
 	set_bit(PLATFORM_PROFILE_QUIET, asus->platform_profile_handler.choices);
 	set_bit(PLATFORM_PROFILE_BALANCED,
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index 7b3ee2d6a23d..dd6b7d16b567 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -1108,6 +1108,11 @@ static int thermal_profile_set(struct platform_profile_handler *pprof,
 	return wmax_thermal_control(supported_thermal_profiles[profile]);
 }
 
+static const struct platform_profile_ops awcc_platform_profile_ops = {
+	.get = thermal_profile_get,
+	.set = thermal_profile_set,
+};
+
 static int create_thermal_profile(struct platform_device *platform_device)
 {
 	u32 out_data;
@@ -1154,10 +1159,9 @@ static int create_thermal_profile(struct platform_device *platform_device)
 		set_bit(PLATFORM_PROFILE_PERFORMANCE, pp_handler.choices);
 	}
 
-	pp_handler.profile_get = thermal_profile_get;
-	pp_handler.profile_set = thermal_profile_set;
 	pp_handler.name = "alienware-wmi";
 	pp_handler.dev = &platform_device->dev;
+	pp_handler.ops = &awcc_platform_profile_ops;
 
 	return devm_platform_profile_register(&pp_handler);
 }
diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
index 3797a5721dbd..9af63a9bdefe 100644
--- a/drivers/platform/x86/dell/dell-pc.c
+++ b/drivers/platform/x86/dell/dell-pc.c
@@ -231,6 +231,11 @@ static int thermal_platform_profile_get(struct platform_profile_handler *pprof,
 	return 0;
 }
 
+static const struct platform_profile_ops dell_pc_platform_profile_ops = {
+	.get = thermal_platform_profile_get,
+	.set = thermal_platform_profile_set,
+};
+
 static int thermal_init(void)
 {
 	int ret;
@@ -258,8 +263,7 @@ static int thermal_init(void)
 	}
 	thermal_handler->name = "dell-pc";
 	thermal_handler->dev = &platform_device->dev;
-	thermal_handler->profile_get = thermal_platform_profile_get;
-	thermal_handler->profile_set = thermal_platform_profile_set;
+	thermal_handler->ops = &dell_pc_platform_profile_ops;
 
 	if (supported_modes & DELL_QUIET)
 		set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
index 6d6e13a0c6e2..dbd19d93fdd6 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -1565,6 +1565,21 @@ static inline void omen_unregister_powersource_event_handler(void)
 	unregister_acpi_notifier(&platform_power_source_nb);
 }
 
+static const struct platform_profile_ops platform_profile_omen_ops = {
+	.get = platform_profile_omen_get,
+	.set = platform_profile_omen_set,
+};
+
+static const struct platform_profile_ops platform_profile_victus_ops = {
+	.get = platform_profile_victus_get,
+	.set = platform_profile_victus_set,
+};
+
+static const struct platform_profile_ops hp_wmi_platform_profile_ops = {
+	.get = hp_wmi_platform_profile_get,
+	.set = hp_wmi_platform_profile_set,
+};
+
 static int thermal_profile_setup(struct platform_device *device)
 {
 	int err, tp;
@@ -1582,8 +1597,7 @@ static int thermal_profile_setup(struct platform_device *device)
 		if (err < 0)
 			return err;
 
-		platform_profile_handler.profile_get = platform_profile_omen_get;
-		platform_profile_handler.profile_set = platform_profile_omen_set;
+		platform_profile_handler.ops = &platform_profile_omen_ops;
 
 		set_bit(PLATFORM_PROFILE_COOL, platform_profile_handler.choices);
 	} else if (is_victus_thermal_profile()) {
@@ -1599,8 +1613,7 @@ static int thermal_profile_setup(struct platform_device *device)
 		if (err < 0)
 			return err;
 
-		platform_profile_handler.profile_get = platform_profile_victus_get;
-		platform_profile_handler.profile_set = platform_profile_victus_set;
+		platform_profile_handler.ops = &platform_profile_victus_ops;
 
 		set_bit(PLATFORM_PROFILE_QUIET, platform_profile_handler.choices);
 	} else {
@@ -1617,8 +1630,7 @@ static int thermal_profile_setup(struct platform_device *device)
 		if (err)
 			return err;
 
-		platform_profile_handler.profile_get = hp_wmi_platform_profile_get;
-		platform_profile_handler.profile_set = hp_wmi_platform_profile_set;
+		platform_profile_handler.ops = &hp_wmi_platform_profile_ops;
 
 		set_bit(PLATFORM_PROFILE_QUIET, platform_profile_handler.choices);
 		set_bit(PLATFORM_PROFILE_COOL, platform_profile_handler.choices);
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index dc98f862a06d..f3414964c3b9 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1063,6 +1063,11 @@ static const struct dmi_system_id ideapad_dytc_v4_allow_table[] = {
 	{}
 };
 
+static const struct platform_profile_ops dytc_profile_ops = {
+	.get = dytc_profile_get,
+	.set = dytc_profile_set,
+};
+
 static int ideapad_dytc_profile_init(struct ideapad_private *priv)
 {
 	int err, dytc_version;
@@ -1105,8 +1110,7 @@ static int ideapad_dytc_profile_init(struct ideapad_private *priv)
 	priv->dytc->pprof.name = "ideapad-laptop";
 	priv->dytc->pprof.dev = &priv->platform_device->dev;
 	priv->dytc->priv = priv;
-	priv->dytc->pprof.profile_get = dytc_profile_get;
-	priv->dytc->pprof.profile_set = dytc_profile_set;
+	priv->dytc->pprof.ops = &dytc_profile_ops;
 
 	/* Setup supported modes */
 	set_bit(PLATFORM_PROFILE_LOW_POWER, priv->dytc->pprof.choices);
diff --git a/drivers/platform/x86/inspur_platform_profile.c b/drivers/platform/x86/inspur_platform_profile.c
index 53af73a7fbf7..6113b87c4858 100644
--- a/drivers/platform/x86/inspur_platform_profile.c
+++ b/drivers/platform/x86/inspur_platform_profile.c
@@ -166,6 +166,11 @@ static int inspur_platform_profile_get(struct platform_profile_handler *pprof,
 	return 0;
 }
 
+static const struct platform_profile_ops inspur_platform_profile_ops = {
+	.get = inspur_platform_profile_get,
+	.set = inspur_platform_profile_set,
+};
+
 static int inspur_wmi_probe(struct wmi_device *wdev, const void *context)
 {
 	struct inspur_wmi_priv *priv;
@@ -179,8 +184,7 @@ static int inspur_wmi_probe(struct wmi_device *wdev, const void *context)
 
 	priv->handler.name = "inspur-wmi";
 	priv->handler.dev = &wdev->dev;
-	priv->handler.profile_get = inspur_platform_profile_get;
-	priv->handler.profile_set = inspur_platform_profile_set;
+	priv->handler.ops = &inspur_platform_profile_ops;
 
 	set_bit(PLATFORM_PROFILE_LOW_POWER, priv->handler.choices);
 	set_bit(PLATFORM_PROFILE_BALANCED, priv->handler.choices);
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index f51662861738..519d9cb0af57 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -10538,10 +10538,14 @@ static int dytc_profile_set(struct platform_profile_handler *pprof,
 	return err;
 }
 
+static const struct platform_profile_ops dytc_profile_ops = {
+	.get = dytc_profile_get,
+	.set = dytc_profile_set,
+};
+
 static struct platform_profile_handler dytc_profile = {
 	.name = "thinkpad-acpi",
-	.profile_get = dytc_profile_get,
-	.profile_set = dytc_profile_set,
+	.ops = &dytc_profile_ops,
 };
 
 static void dytc_profile_refresh(void)
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index f1cd4b65e351..8031bf801d70 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -27,16 +27,22 @@ enum platform_profile_option {
 	PLATFORM_PROFILE_LAST, /*must always be last */
 };
 
+struct platform_profile_handler;
+
+struct platform_profile_ops {
+	int (*get)(struct platform_profile_handler *pprof,
+			enum platform_profile_option *profile);
+	int (*set)(struct platform_profile_handler *pprof,
+			enum platform_profile_option profile);
+};
+
 struct platform_profile_handler {
 	const char *name;
 	struct device *dev;
 	struct device *class_dev;
 	int minor;
 	unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
-	int (*profile_get)(struct platform_profile_handler *pprof,
-				enum platform_profile_option *profile);
-	int (*profile_set)(struct platform_profile_handler *pprof,
-				enum platform_profile_option profile);
+	const struct platform_profile_ops *ops;
 };
 
 int platform_profile_register(struct platform_profile_handler *pprof);
-- 
2.47.1

[RFC PATCH 2/3] ACPI: platform_profile: Add `choices` to platform_profile_ops

[Kurt Borja]

Add a `choices` callback to platform_profile_ops, which lets drivers
specify how to select available profiles.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/acpi/platform_profile.c               | 12 ++++++--
 .../surface/surface_platform_profile.c        | 16 ++++++----
 drivers/platform/x86/acer-wmi.c               | 24 ++++++++-------
 drivers/platform/x86/amd/pmf/sps.c            | 15 ++++++----
 drivers/platform/x86/asus-wmi.c               | 16 ++++++----
 drivers/platform/x86/dell/alienware-wmi.c     | 21 ++++++++------
 drivers/platform/x86/dell/dell-pc.c           | 26 ++++++++++-------
 drivers/platform/x86/hp/hp-wmi.c              | 29 +++++++++++++------
 drivers/platform/x86/ideapad-laptop.c         | 15 ++++++----
 .../platform/x86/inspur_platform_profile.c    | 14 ++++++---
 drivers/platform/x86/thinkpad_acpi.c          | 15 ++++++----
 include/linux/platform_profile.h              |  1 +
 12 files changed, 133 insertions(+), 71 deletions(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 68e496ff5176..ec749c2d0695 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -464,14 +464,22 @@ int platform_profile_register(struct platform_profile_handler *pprof)
 	int err;
 
 	/* Sanity check the profile handler */
-	if (!pprof || bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST) ||
-	    !pprof->ops->set || !pprof->ops->get) {
+	if (!pprof || !pprof->ops->set || !pprof->ops->get || !pprof->ops->choices) {
 		pr_err("platform_profile: handler is invalid\n");
 		return -EINVAL;
 	}
 
 	guard(mutex)(&profile_lock);
 
+	err = pprof->ops->choices(pprof);
+	if (err < 0)
+		return err;
+
+	if (bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST)) {
+		pr_err("platform_profile: no available profiles\n");
+		return -EINVAL;
+	}
+
 	/* create class interface for individual handler */
 	pprof->minor = ida_alloc(&platform_profile_ida, GFP_KERNEL);
 	if (pprof->minor < 0)
diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
index 8077d556e616..173f89d59c31 100644
--- a/drivers/platform/surface/surface_platform_profile.c
+++ b/drivers/platform/surface/surface_platform_profile.c
@@ -201,9 +201,20 @@ static int ssam_platform_profile_set(struct platform_profile_handler *pprof,
 	return tp;
 }
 
+static int ssam_platform_profile_choices(struct platform_profile_handler *pprof)
+{
+	set_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices);
+	set_bit(PLATFORM_PROFILE_BALANCED, pprof->choices);
+	set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE, pprof->choices);
+	set_bit(PLATFORM_PROFILE_PERFORMANCE, pprof->choices);
+
+	return 0;
+}
+
 static const struct platform_profile_ops ssam_platform_profile_ops = {
 	.get = ssam_platform_profile_get,
 	.set = ssam_platform_profile_set,
+	.choices = ssam_platform_profile_choices,
 };
 
 static int surface_platform_profile_probe(struct ssam_device *sdev)
@@ -223,11 +234,6 @@ static int surface_platform_profile_probe(struct ssam_device *sdev)
 
 	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);
-	set_bit(PLATFORM_PROFILE_PERFORMANCE, tpd->handler.choices);
-
 	return platform_profile_register(&tpd->handler);
 }
 
diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index 26d6b9cdfd12..deaadada5506 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -1900,9 +1900,22 @@ acer_predator_v4_platform_profile_set(struct platform_profile_handler *pprof,
 	return 0;
 }
 
+static int
+acer_predator_v4_platform_profile_choices(struct platform_profile_handler *pprof)
+{
+	set_bit(PLATFORM_PROFILE_PERFORMANCE, platform_profile_handler.choices);
+	set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE, platform_profile_handler.choices);
+	set_bit(PLATFORM_PROFILE_BALANCED, platform_profile_handler.choices);
+	set_bit(PLATFORM_PROFILE_QUIET, platform_profile_handler.choices);
+	set_bit(PLATFORM_PROFILE_LOW_POWER, platform_profile_handler.choices);
+
+	return 0;
+}
+
 static const struct platform_profile_ops acer_predator_v4_platform_profile_ops = {
 	.get = acer_predator_v4_platform_profile_get,
 	.set = acer_predator_v4_platform_profile_set,
+	.choices = acer_predator_v4_platform_profile_choices,
 };
 
 static int acer_platform_profile_setup(struct platform_device *device)
@@ -1915,17 +1928,6 @@ static int acer_platform_profile_setup(struct platform_device *device)
 		platform_profile_handler.ops =
 			&acer_predator_v4_platform_profile_ops;
 
-		set_bit(PLATFORM_PROFILE_PERFORMANCE,
-			platform_profile_handler.choices);
-		set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE,
-			platform_profile_handler.choices);
-		set_bit(PLATFORM_PROFILE_BALANCED,
-			platform_profile_handler.choices);
-		set_bit(PLATFORM_PROFILE_QUIET,
-			platform_profile_handler.choices);
-		set_bit(PLATFORM_PROFILE_LOW_POWER,
-			platform_profile_handler.choices);
-
 		err = platform_profile_register(&platform_profile_handler);
 		if (err)
 			return err;
diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
index 1b87254a7120..e6132edcab1b 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -387,9 +387,19 @@ static int amd_pmf_profile_set(struct platform_profile_handler *pprof,
 	return 0;
 }
 
+static int amd_pmf_profile_choices(struct platform_profile_handler *pprof)
+{
+	set_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices);
+	set_bit(PLATFORM_PROFILE_BALANCED, pprof->choices);
+	set_bit(PLATFORM_PROFILE_PERFORMANCE, pprof->choices);
+
+	return 0;
+}
+
 static const struct platform_profile_ops amd_pmf_profile_ops = {
 	.get = amd_pmf_profile_get,
 	.set = amd_pmf_profile_set,
+	.choices = amd_pmf_profile_choices,
 };
 
 int amd_pmf_init_sps(struct amd_pmf_dev *dev)
@@ -414,11 +424,6 @@ int amd_pmf_init_sps(struct amd_pmf_dev *dev)
 	dev->pprof.dev = dev->dev;
 	dev->pprof.ops = &amd_pmf_profile_ops;
 
-	/* Setup supported modes */
-	set_bit(PLATFORM_PROFILE_LOW_POWER, dev->pprof.choices);
-	set_bit(PLATFORM_PROFILE_BALANCED, dev->pprof.choices);
-	set_bit(PLATFORM_PROFILE_PERFORMANCE, dev->pprof.choices);
-
 	/* Create platform_profile structure and register */
 	err = platform_profile_register(&dev->pprof);
 	if (err)
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index d0828b1499b1..850225adf05c 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -3852,9 +3852,19 @@ static int asus_wmi_platform_profile_set(struct platform_profile_handler *pprof,
 	return throttle_thermal_policy_write(asus);
 }
 
+static int asus_wmi_platform_profile_choices(struct platform_profile_handler *pprof)
+{
+	set_bit(PLATFORM_PROFILE_QUIET, pprof->choices);
+	set_bit(PLATFORM_PROFILE_BALANCED, pprof->choices);
+	set_bit(PLATFORM_PROFILE_PERFORMANCE, pprof->choices);
+
+	return 0;
+}
+
 static const struct platform_profile_ops asus_wmi_platform_profile_ops = {
 	.get = asus_wmi_platform_profile_get,
 	.set = asus_wmi_platform_profile_set,
+	.choices = asus_wmi_platform_profile_choices,
 };
 
 static int platform_profile_setup(struct asus_wmi *asus)
@@ -3885,12 +3895,6 @@ static int platform_profile_setup(struct asus_wmi *asus)
 	asus->platform_profile_handler.dev = dev;
 	asus->platform_profile_handler.ops = &asus_wmi_platform_profile_ops;
 
-	set_bit(PLATFORM_PROFILE_QUIET, asus->platform_profile_handler.choices);
-	set_bit(PLATFORM_PROFILE_BALANCED,
-		asus->platform_profile_handler.choices);
-	set_bit(PLATFORM_PROFILE_PERFORMANCE,
-		asus->platform_profile_handler.choices);
-
 	err = platform_profile_register(&asus->platform_profile_handler);
 	if (err == -EEXIST) {
 		pr_warn("%s, a platform_profile handler is already registered\n", __func__);
diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index dd6b7d16b567..28cc9f01f70c 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -1108,12 +1108,7 @@ static int thermal_profile_set(struct platform_profile_handler *pprof,
 	return wmax_thermal_control(supported_thermal_profiles[profile]);
 }
 
-static const struct platform_profile_ops awcc_platform_profile_ops = {
-	.get = thermal_profile_get,
-	.set = thermal_profile_set,
-};
-
-static int create_thermal_profile(struct platform_device *platform_device)
+static int thermal_profile_choices(struct platform_profile_handler *pprof)
 {
 	u32 out_data;
 	u8 sys_desc[4];
@@ -1149,9 +1144,6 @@ static int create_thermal_profile(struct platform_device *platform_device)
 		set_bit(profile, pp_handler.choices);
 	}
 
-	if (bitmap_empty(pp_handler.choices, PLATFORM_PROFILE_LAST))
-		return -ENODEV;
-
 	if (quirks->gmode) {
 		supported_thermal_profiles[PLATFORM_PROFILE_PERFORMANCE] =
 			WMAX_THERMAL_MODE_GMODE;
@@ -1159,6 +1151,17 @@ static int create_thermal_profile(struct platform_device *platform_device)
 		set_bit(PLATFORM_PROFILE_PERFORMANCE, pp_handler.choices);
 	}
 
+	return 0;
+}
+
+static const struct platform_profile_ops awcc_platform_profile_ops = {
+	.get = thermal_profile_get,
+	.set = thermal_profile_set,
+	.choices = thermal_profile_choices,
+};
+
+static int create_thermal_profile(struct platform_device *platform_device)
+{
 	pp_handler.name = "alienware-wmi";
 	pp_handler.dev = &platform_device->dev;
 	pp_handler.ops = &awcc_platform_profile_ops;
diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
index 9af63a9bdefe..ae49f6055aff 100644
--- a/drivers/platform/x86/dell/dell-pc.c
+++ b/drivers/platform/x86/dell/dell-pc.c
@@ -24,6 +24,7 @@
 #include "dell-smbios.h"
 
 static struct platform_device *platform_device;
+static int supported_modes;
 
 static const struct dmi_system_id dell_device_table[] __initconst = {
 	{
@@ -231,15 +232,29 @@ static int thermal_platform_profile_get(struct platform_profile_handler *pprof,
 	return 0;
 }
 
+static int thermal_platform_profile_choices(struct platform_profile_handler *pprof)
+{
+	if (supported_modes & DELL_QUIET)
+		set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
+	if (supported_modes & DELL_COOL_BOTTOM)
+		set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
+	if (supported_modes & DELL_BALANCED)
+		set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
+	if (supported_modes & DELL_PERFORMANCE)
+		set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
+
+	return 0;
+}
+
 static const struct platform_profile_ops dell_pc_platform_profile_ops = {
 	.get = thermal_platform_profile_get,
 	.set = thermal_platform_profile_set,
+	.choices = thermal_platform_profile_choices,
 };
 
 static int thermal_init(void)
 {
 	int ret;
-	int supported_modes;
 
 	/* If thermal commands are not supported, exit without error */
 	if (!dell_smbios_class_is_supported(CLASS_INFO))
@@ -265,15 +280,6 @@ static int thermal_init(void)
 	thermal_handler->dev = &platform_device->dev;
 	thermal_handler->ops = &dell_pc_platform_profile_ops;
 
-	if (supported_modes & DELL_QUIET)
-		set_bit(PLATFORM_PROFILE_QUIET, thermal_handler->choices);
-	if (supported_modes & DELL_COOL_BOTTOM)
-		set_bit(PLATFORM_PROFILE_COOL, thermal_handler->choices);
-	if (supported_modes & DELL_BALANCED)
-		set_bit(PLATFORM_PROFILE_BALANCED, thermal_handler->choices);
-	if (supported_modes & DELL_PERFORMANCE)
-		set_bit(PLATFORM_PROFILE_PERFORMANCE, thermal_handler->choices);
-
 	/* Clean up if failed */
 	ret = platform_profile_register(thermal_handler);
 	if (ret)
diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
index dbd19d93fdd6..3285e72f312d 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -1488,6 +1488,23 @@ static int platform_profile_victus_set(struct platform_profile_handler *pprof,
 	return 0;
 }
 
+static int hp_wmi_platform_profile_choices(struct platform_profile_handler *pprof)
+{
+	if (is_omen_thermal_profile()) {
+		set_bit(PLATFORM_PROFILE_COOL, platform_profile_handler.choices);
+	} else if (is_victus_thermal_profile()) {
+		set_bit(PLATFORM_PROFILE_QUIET, platform_profile_handler.choices);
+	} else {
+		set_bit(PLATFORM_PROFILE_QUIET, platform_profile_handler.choices);
+		set_bit(PLATFORM_PROFILE_COOL, platform_profile_handler.choices);
+	}
+
+	set_bit(PLATFORM_PROFILE_BALANCED, platform_profile_handler.choices);
+	set_bit(PLATFORM_PROFILE_PERFORMANCE, platform_profile_handler.choices);
+
+	return 0;
+}
+
 static int omen_powersource_event(struct notifier_block *nb,
 				  unsigned long value,
 				  void *data)
@@ -1568,16 +1585,19 @@ static inline void omen_unregister_powersource_event_handler(void)
 static const struct platform_profile_ops platform_profile_omen_ops = {
 	.get = platform_profile_omen_get,
 	.set = platform_profile_omen_set,
+	.choices = hp_wmi_platform_profile_choices,
 };
 
 static const struct platform_profile_ops platform_profile_victus_ops = {
 	.get = platform_profile_victus_get,
 	.set = platform_profile_victus_set,
+	.choices = hp_wmi_platform_profile_choices,
 };
 
 static const struct platform_profile_ops hp_wmi_platform_profile_ops = {
 	.get = hp_wmi_platform_profile_get,
 	.set = hp_wmi_platform_profile_set,
+	.choices = hp_wmi_platform_profile_choices,
 };
 
 static int thermal_profile_setup(struct platform_device *device)
@@ -1598,8 +1618,6 @@ static int thermal_profile_setup(struct platform_device *device)
 			return err;
 
 		platform_profile_handler.ops = &platform_profile_omen_ops;
-
-		set_bit(PLATFORM_PROFILE_COOL, platform_profile_handler.choices);
 	} else if (is_victus_thermal_profile()) {
 		err = platform_profile_victus_get_ec(&active_platform_profile);
 		if (err < 0)
@@ -1614,8 +1632,6 @@ static int thermal_profile_setup(struct platform_device *device)
 			return err;
 
 		platform_profile_handler.ops = &platform_profile_victus_ops;
-
-		set_bit(PLATFORM_PROFILE_QUIET, platform_profile_handler.choices);
 	} else {
 		tp = thermal_profile_get();
 
@@ -1631,15 +1647,10 @@ static int thermal_profile_setup(struct platform_device *device)
 			return err;
 
 		platform_profile_handler.ops = &hp_wmi_platform_profile_ops;
-
-		set_bit(PLATFORM_PROFILE_QUIET, platform_profile_handler.choices);
-		set_bit(PLATFORM_PROFILE_COOL, platform_profile_handler.choices);
 	}
 
 	platform_profile_handler.name = "hp-wmi";
 	platform_profile_handler.dev = &device->dev;
-	set_bit(PLATFORM_PROFILE_BALANCED, platform_profile_handler.choices);
-	set_bit(PLATFORM_PROFILE_PERFORMANCE, platform_profile_handler.choices);
 
 	err = platform_profile_register(&platform_profile_handler);
 	if (err)
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index f3414964c3b9..f5c286572e75 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1023,6 +1023,15 @@ static int dytc_profile_set(struct platform_profile_handler *pprof,
 	return -EINTR;
 }
 
+static int dytc_profile_choices(struct platform_profile_handler *pprof)
+{
+	set_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices);
+	set_bit(PLATFORM_PROFILE_BALANCED, pprof->choices);
+	set_bit(PLATFORM_PROFILE_PERFORMANCE, pprof->choices);
+
+	return 0;
+}
+
 static void dytc_profile_refresh(struct ideapad_private *priv)
 {
 	enum platform_profile_option profile;
@@ -1066,6 +1075,7 @@ static const struct dmi_system_id ideapad_dytc_v4_allow_table[] = {
 static const struct platform_profile_ops dytc_profile_ops = {
 	.get = dytc_profile_get,
 	.set = dytc_profile_set,
+	.choices = dytc_profile_choices,
 };
 
 static int ideapad_dytc_profile_init(struct ideapad_private *priv)
@@ -1112,11 +1122,6 @@ static int ideapad_dytc_profile_init(struct ideapad_private *priv)
 	priv->dytc->priv = priv;
 	priv->dytc->pprof.ops = &dytc_profile_ops;
 
-	/* Setup supported modes */
-	set_bit(PLATFORM_PROFILE_LOW_POWER, priv->dytc->pprof.choices);
-	set_bit(PLATFORM_PROFILE_BALANCED, priv->dytc->pprof.choices);
-	set_bit(PLATFORM_PROFILE_PERFORMANCE, priv->dytc->pprof.choices);
-
 	/* Create platform_profile structure and register */
 	err = platform_profile_register(&priv->dytc->pprof);
 	if (err)
diff --git a/drivers/platform/x86/inspur_platform_profile.c b/drivers/platform/x86/inspur_platform_profile.c
index 6113b87c4858..c4e5ed5429a8 100644
--- a/drivers/platform/x86/inspur_platform_profile.c
+++ b/drivers/platform/x86/inspur_platform_profile.c
@@ -166,9 +166,19 @@ static int inspur_platform_profile_get(struct platform_profile_handler *pprof,
 	return 0;
 }
 
+static int inspur_platform_profile_choices(struct platform_profile_handler *pprof)
+{
+	set_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices);
+	set_bit(PLATFORM_PROFILE_BALANCED, pprof->choices);
+	set_bit(PLATFORM_PROFILE_PERFORMANCE, pprof->choices);
+
+	return 0;
+}
+
 static const struct platform_profile_ops inspur_platform_profile_ops = {
 	.get = inspur_platform_profile_get,
 	.set = inspur_platform_profile_set,
+	.choices = inspur_platform_profile_choices,
 };
 
 static int inspur_wmi_probe(struct wmi_device *wdev, const void *context)
@@ -186,10 +196,6 @@ static int inspur_wmi_probe(struct wmi_device *wdev, const void *context)
 	priv->handler.dev = &wdev->dev;
 	priv->handler.ops = &inspur_platform_profile_ops;
 
-	set_bit(PLATFORM_PROFILE_LOW_POWER, priv->handler.choices);
-	set_bit(PLATFORM_PROFILE_BALANCED, priv->handler.choices);
-	set_bit(PLATFORM_PROFILE_PERFORMANCE, priv->handler.choices);
-
 	return platform_profile_register(&priv->handler);
 }
 
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 519d9cb0af57..b9702c29937e 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -10538,9 +10538,19 @@ static int dytc_profile_set(struct platform_profile_handler *pprof,
 	return err;
 }
 
+static int dytc_profile_choices(struct platform_profile_handler *pprof)
+{
+	set_bit(PLATFORM_PROFILE_LOW_POWER, pprof->choices);
+	set_bit(PLATFORM_PROFILE_BALANCED, pprof->choices);
+	set_bit(PLATFORM_PROFILE_PERFORMANCE, pprof->choices);
+
+	return 0;
+}
+
 static const struct platform_profile_ops dytc_profile_ops = {
 	.get = dytc_profile_get,
 	.set = dytc_profile_set,
+	.choices = dytc_profile_choices,
 };
 
 static struct platform_profile_handler dytc_profile = {
@@ -10584,11 +10594,6 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
 {
 	int err, output;
 
-	/* Setup supported modes */
-	set_bit(PLATFORM_PROFILE_LOW_POWER, dytc_profile.choices);
-	set_bit(PLATFORM_PROFILE_BALANCED, dytc_profile.choices);
-	set_bit(PLATFORM_PROFILE_PERFORMANCE, dytc_profile.choices);
-
 	err = dytc_command(DYTC_CMD_QUERY, &output);
 	if (err)
 		return err;
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index 8031bf801d70..7f266a60b41a 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -34,6 +34,7 @@ struct platform_profile_ops {
 			enum platform_profile_option *profile);
 	int (*set)(struct platform_profile_handler *pprof,
 			enum platform_profile_option profile);
+	int (*choices)(struct platform_profile_handler *pprof);
 };
 
 struct platform_profile_handler {
-- 
2.47.1

[RFC PATCH 3/3] ACPI: platform_profile: Add platform_profile_refresh_choices()

[Kurt Borja]

Let drivers dynamically refresh selected choices safely, while holding
`profile_lock`.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/acpi/platform_profile.c  | 23 +++++++++++++++++++++++
 include/linux/platform_profile.h |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index ec749c2d0695..087280d786b1 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -459,6 +459,29 @@ int platform_profile_cycle(void)
 }
 EXPORT_SYMBOL_GPL(platform_profile_cycle);
 
+int platform_profile_refresh_choices(struct platform_profile_handler *pprof)
+{
+	unsigned long backup[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
+	int err;
+
+	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
+		bitmap_copy(backup, pprof->choices, PLATFORM_PROFILE_LAST);
+
+		err = pprof->ops->choices(pprof);
+		if (err) {
+			bitmap_copy(pprof->choices, backup, PLATFORM_PROFILE_LAST);
+			return err;
+		}
+
+		_notify_class_profile(pprof->class_dev, NULL);
+	}
+
+	sysfs_notify(acpi_kobj, NULL, "platform_profile");
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(platform_profile_refresh_choices);
+
 int platform_profile_register(struct platform_profile_handler *pprof)
 {
 	int err;
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index 7f266a60b41a..7d543dd8c164 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -50,6 +50,7 @@ int platform_profile_register(struct platform_profile_handler *pprof);
 int platform_profile_remove(struct platform_profile_handler *pprof);
 int devm_platform_profile_register(struct platform_profile_handler *pprof);
 int platform_profile_cycle(void);
+int platform_profile_refresh_choices(struct platform_profile_handler *pprof);
 void platform_profile_notify(struct platform_profile_handler *pprof);
 
 #endif  /*_PLATFORM_PROFILE_H_*/
-- 
2.47.1

Re: [RFC PATCH 0/3] ACPI: platform_profile: Let drivers dynamically refresh choices

[Mark Pearson]

Hi Kurt,

On Sun, Jan 5, 2025, at 11:45 PM, Kurt Borja wrote:
> Hello,
>
> Some drivers may need to dynamically modify their selected `choices`.
> Such is the case of the acer-wmi driver, which implemented their own
> profile cycling method, because users expect different profiles to be
> available whether the laptop is on AC or not [1].
>
> These series would allow acer-wmi to simplify this custom cycling method
> to use platform_profile_cycle(), as it's already being proposed in these
> series [2]; without changing expected behaviors, by refreshing their
> selected choices on AC connect/disconnect events, which would also solve
> this discussion [3].
>
> Additionally, I think the platform_profile_ops approach would enable us
> to hide the platform_profile_handler in the future, and instead just pass
> the class device to get/set methods like the HWMON subsystem does.
>
> I think having this kind of flexibility is valuable. Let me know what you
> think!
>

I personally would love to see how this would be used for the acer issue highlighted to see how it would work out. It feels like the series is short a patch :)

As a side note, I did (many moons ago) propose a change to alter profiles used depending on AC/battery mode (in the thinkpad driver), and it was rejected as something that should be done in user space.
Your use case does seem somewhat different, but it's similar enough that if you get it working I'd be interested to see if I can take advantage of the approach too.

Mark

Re: [RFC PATCH 0/3] ACPI: platform_profile: Let drivers dynamically refresh choices

[Hridesh MG]

On Tue, Jan 7, 2025 at 7:49 AM Mark Pearson <mpearson-lenovo@squebb.ca> wrote:
>
> Hi Kurt,
>
> On Sun, Jan 5, 2025, at 11:45 PM, Kurt Borja wrote:
> > Hello,
> >
> > Some drivers may need to dynamically modify their selected `choices`.
> > Such is the case of the acer-wmi driver, which implemented their own
> > profile cycling method, because users expect different profiles to be
> > available whether the laptop is on AC or not [1].
> >
> > These series would allow acer-wmi to simplify this custom cycling method
> > to use platform_profile_cycle(), as it's already being proposed in these
> > series [2]; without changing expected behaviors, by refreshing their
> > selected choices on AC connect/disconnect events, which would also solve
> > this discussion [3].
> >
> > Additionally, I think the platform_profile_ops approach would enable us
> > to hide the platform_profile_handler in the future, and instead just pass
> > the class device to get/set methods like the HWMON subsystem does.
> >
> > I think having this kind of flexibility is valuable. Let me know what you
> > think!
> >
>
> I personally would love to see how this would be used for the acer issue highlighted to see how it would work out. It feels like the series is short a patch :)

I do think that having this flexibility would be good, as i was
considering manually clearing the set bits and calling platform_notify
for the acer series, but in my specific case (for devices using the
predator v4 interface) it was found that the hardware was responsive
to all profiles regardless of AC/battery mode so it made sense to
leave this kind of artificial limiting of profiles to the
userspace--similar to how the Windows driver handles it through the
Nitro Sense app.

I feel like users should have the power to utilize their hardware in
the way they want it to, though if there is a compelling reason to
limit the profiles then i'd be more than happy to add this to the
series :)


--
Hridesh MG

Re: [RFC PATCH 0/3] ACPI: platform_profile: Let drivers dynamically refresh choices

[Mario Limonciello]

On 1/7/2025 07:14, Hridesh MG wrote:
> On Tue, Jan 7, 2025 at 7:49 AM Mark Pearson <mpearson-lenovo@squebb.ca> wrote:
>>
>> Hi Kurt,
>>
>> On Sun, Jan 5, 2025, at 11:45 PM, Kurt Borja wrote:
>>> Hello,
>>>
>>> Some drivers may need to dynamically modify their selected `choices`.
>>> Such is the case of the acer-wmi driver, which implemented their own
>>> profile cycling method, because users expect different profiles to be
>>> available whether the laptop is on AC or not [1].
>>>
>>> These series would allow acer-wmi to simplify this custom cycling method
>>> to use platform_profile_cycle(), as it's already being proposed in these
>>> series [2]; without changing expected behaviors, by refreshing their
>>> selected choices on AC connect/disconnect events, which would also solve
>>> this discussion [3].
>>>
>>> Additionally, I think the platform_profile_ops approach would enable us
>>> to hide the platform_profile_handler in the future, and instead just pass
>>> the class device to get/set methods like the HWMON subsystem does.
>>>
>>> I think having this kind of flexibility is valuable. Let me know what you
>>> think!
>>>
>>
>> I personally would love to see how this would be used for the acer issue highlighted to see how it would work out. It feels like the series is short a patch :)
> 
> I do think that having this flexibility would be good, as i was
> considering manually clearing the set bits and calling platform_notify
> for the acer series, but in my specific case (for devices using the
> predator v4 interface) it was found that the hardware was responsive
> to all profiles regardless of AC/battery mode so it made sense to
> leave this kind of artificial limiting of profiles to the
> userspace--similar to how the Windows driver handles it through the
> Nitro Sense app.
> 
> I feel like users should have the power to utilize their hardware in
> the way they want it to, though if there is a compelling reason to
> limit the profiles then i'd be more than happy to add this to the
> series :)
> 
> 
> --
> Hridesh MG

I agree with Mark, this series is missing the patch that shows exactly 
how this would be used.  Furthermore; what exactly are the differences 
in choices between AC or DC?

To "userspace" I fail to understand how "balanced" is different from AC 
to DC for example.

If an implementation has differences for AC vs DC would it make more 
sense to have that abstraction in the "profile handlers" instead of the ops?

IE always have "low-power, balanced, performance" for choices. If the 
system is on AC the profile handler might do something different than on 
DC for a given state.

Another idea I had while thinking about this is that the platform 
profile core can have a sysfs file to indicate whether or not to "allow" 
power state sensitive values.

It could default to "1" and then it can send AC/DC events to the 
platform profile handlers when enabled.  If userspace prefers not to use 
it, then they can set it to 0 and then the profile handler would stop 
reacting.

Re: [RFC PATCH 0/3] ACPI: platform_profile: Let drivers dynamically refresh choices

[Hridesh MG]

On Tue, Jan 7, 2025 at 9:21 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 1/7/2025 07:14, Hridesh MG wrote:
> > On Tue, Jan 7, 2025 at 7:49 AM Mark Pearson <mpearson-lenovo@squebb.ca> wrote:
> >>
> >> Hi Kurt,
> >>
> >> On Sun, Jan 5, 2025, at 11:45 PM, Kurt Borja wrote:
> >>> Hello,
> >>>
> >>> Some drivers may need to dynamically modify their selected `choices`.
> >>> Such is the case of the acer-wmi driver, which implemented their own
> >>> profile cycling method, because users expect different profiles to be
> >>> available whether the laptop is on AC or not [1].
> >>>
> >>> These series would allow acer-wmi to simplify this custom cycling method
> >>> to use platform_profile_cycle(), as it's already being proposed in these
> >>> series [2]; without changing expected behaviors, by refreshing their
> >>> selected choices on AC connect/disconnect events, which would also solve
> >>> this discussion [3].
> >>>
> >>> Additionally, I think the platform_profile_ops approach would enable us
> >>> to hide the platform_profile_handler in the future, and instead just pass
> >>> the class device to get/set methods like the HWMON subsystem does.
> >>>
> >>> I think having this kind of flexibility is valuable. Let me know what you
> >>> think!
> >>>
> >>
> >> I personally would love to see how this would be used for the acer issue highlighted to see how it would work out. It feels like the series is short a patch :)
> >
> > I do think that having this flexibility would be good, as i was
> > considering manually clearing the set bits and calling platform_notify
> > for the acer series, but in my specific case (for devices using the
> > predator v4 interface) it was found that the hardware was responsive
> > to all profiles regardless of AC/battery mode so it made sense to
> > leave this kind of artificial limiting of profiles to the
> > userspace--similar to how the Windows driver handles it through the
> > Nitro Sense app.
> >
> > I feel like users should have the power to utilize their hardware in
> > the way they want it to, though if there is a compelling reason to
> > limit the profiles then i'd be more than happy to add this to the
> > series :)
> >
> >
> > --
> > Hridesh MG
>
> I agree with Mark, this series is missing the patch that shows exactly
> how this would be used.  Furthermore; what exactly are the differences
> in choices between AC or DC?
On the predator series, the Windows OEM application only allows you to
select the low-power and balanced platform profiles on DC. These
profiles can however still be activated through WMI methods and the
hardware will apply them.

> To "userspace" I fail to understand how "balanced" is different from AC
> to DC for example.
It is not, the profiles or states themselves are not modified between
AC and DC, just the switching between them is affected.

-- 
Thanks,
Hridesh MG

Re: [RFC PATCH 0/3] ACPI: platform_profile: Let drivers dynamically refresh choices

[Limonciello, Mario]

On 1/7/2025 10:33 AM, Hridesh MG wrote:
> On Tue, Jan 7, 2025 at 9:21 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> On 1/7/2025 07:14, Hridesh MG wrote:
>>> On Tue, Jan 7, 2025 at 7:49 AM Mark Pearson <mpearson-lenovo@squebb.ca> wrote:
>>>>
>>>> Hi Kurt,
>>>>
>>>> On Sun, Jan 5, 2025, at 11:45 PM, Kurt Borja wrote:
>>>>> Hello,
>>>>>
>>>>> Some drivers may need to dynamically modify their selected `choices`.
>>>>> Such is the case of the acer-wmi driver, which implemented their own
>>>>> profile cycling method, because users expect different profiles to be
>>>>> available whether the laptop is on AC or not [1].
>>>>>
>>>>> These series would allow acer-wmi to simplify this custom cycling method
>>>>> to use platform_profile_cycle(), as it's already being proposed in these
>>>>> series [2]; without changing expected behaviors, by refreshing their
>>>>> selected choices on AC connect/disconnect events, which would also solve
>>>>> this discussion [3].
>>>>>
>>>>> Additionally, I think the platform_profile_ops approach would enable us
>>>>> to hide the platform_profile_handler in the future, and instead just pass
>>>>> the class device to get/set methods like the HWMON subsystem does.
>>>>>
>>>>> I think having this kind of flexibility is valuable. Let me know what you
>>>>> think!
>>>>>
>>>>
>>>> I personally would love to see how this would be used for the acer issue highlighted to see how it would work out. It feels like the series is short a patch :)
>>>
>>> I do think that having this flexibility would be good, as i was
>>> considering manually clearing the set bits and calling platform_notify
>>> for the acer series, but in my specific case (for devices using the
>>> predator v4 interface) it was found that the hardware was responsive
>>> to all profiles regardless of AC/battery mode so it made sense to
>>> leave this kind of artificial limiting of profiles to the
>>> userspace--similar to how the Windows driver handles it through the
>>> Nitro Sense app.
>>>
>>> I feel like users should have the power to utilize their hardware in
>>> the way they want it to, though if there is a compelling reason to
>>> limit the profiles then i'd be more than happy to add this to the
>>> series :)
>>>
>>>
>>> --
>>> Hridesh MG
>>
>> I agree with Mark, this series is missing the patch that shows exactly
>> how this would be used.  Furthermore; what exactly are the differences
>> in choices between AC or DC?
> On the predator series, the Windows OEM application only allows you to
> select the low-power and balanced platform profiles on DC. These
> profiles can however still be activated through WMI methods and the
> hardware will apply them.

So on Windows if you are on DC and pick "performance" then go to AC it 
will automatically switch you back to "balanced" and you can't pick 
"performance" again until you go to "DC"?

This sounds totally counterintuitive to me at least.  If you're going to 
gimp people on anything, gimp them on DC.

> 
>> To "userspace" I fail to understand how "balanced" is different from AC
>> to DC for example.
> It is not, the profiles or states themselves are not modified between
> AC and DC, just the switching between them is affected.
> 

IMO - just because Windows does this doesn't mean we need to in Linux. 
If there are only 3 sets of profiles, I'm of the opinion we should let 
users pick any of the 3 profiles in "AC" or "DC" state.

Re: [RFC PATCH 0/3] ACPI: platform_profile: Let drivers dynamically refresh choices

[Kurt Borja]

On Tue, Jan 07, 2025 at 10:47:39AM -0600, Limonciello, Mario wrote:
> 
> 
> On 1/7/2025 10:33 AM, Hridesh MG wrote:
> > On Tue, Jan 7, 2025 at 9:21 PM Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> > > 
> > > On 1/7/2025 07:14, Hridesh MG wrote:
> > > > On Tue, Jan 7, 2025 at 7:49 AM Mark Pearson <mpearson-lenovo@squebb.ca> wrote:
> > > > > 
> > > > > Hi Kurt,
> > > > > 
> > > > > On Sun, Jan 5, 2025, at 11:45 PM, Kurt Borja wrote:
> > > > > > Hello,
> > > > > > 
> > > > > > Some drivers may need to dynamically modify their selected `choices`.
> > > > > > Such is the case of the acer-wmi driver, which implemented their own
> > > > > > profile cycling method, because users expect different profiles to be
> > > > > > available whether the laptop is on AC or not [1].
> > > > > > 
> > > > > > These series would allow acer-wmi to simplify this custom cycling method
> > > > > > to use platform_profile_cycle(), as it's already being proposed in these
> > > > > > series [2]; without changing expected behaviors, by refreshing their
> > > > > > selected choices on AC connect/disconnect events, which would also solve
> > > > > > this discussion [3].
> > > > > > 
> > > > > > Additionally, I think the platform_profile_ops approach would enable us
> > > > > > to hide the platform_profile_handler in the future, and instead just pass
> > > > > > the class device to get/set methods like the HWMON subsystem does.
> > > > > > 
> > > > > > I think having this kind of flexibility is valuable. Let me know what you
> > > > > > think!
> > > > > > 
> > > > > 
> > > > > I personally would love to see how this would be used for the acer issue highlighted to see how it would work out. It feels like the series is short a patch :)
> > > > 
> > > > I do think that having this flexibility would be good, as i was
> > > > considering manually clearing the set bits and calling platform_notify
> > > > for the acer series, but in my specific case (for devices using the
> > > > predator v4 interface) it was found that the hardware was responsive
> > > > to all profiles regardless of AC/battery mode so it made sense to
> > > > leave this kind of artificial limiting of profiles to the
> > > > userspace--similar to how the Windows driver handles it through the
> > > > Nitro Sense app.
> > > > 
> > > > I feel like users should have the power to utilize their hardware in
> > > > the way they want it to, though if there is a compelling reason to
> > > > limit the profiles then i'd be more than happy to add this to the
> > > > series :)
> > > > 
> > > > 
> > > > --
> > > > Hridesh MG
> > > 
> > > I agree with Mark, this series is missing the patch that shows exactly
> > > how this would be used.  Furthermore; what exactly are the differences
> > > in choices between AC or DC?
> > On the predator series, the Windows OEM application only allows you to
> > select the low-power and balanced platform profiles on DC. These
> > profiles can however still be activated through WMI methods and the
> > hardware will apply them.
> 
> So on Windows if you are on DC and pick "performance" then go to AC it will
> automatically switch you back to "balanced" and you can't pick "performance"
> again until you go to "DC"?
> 
> This sounds totally counterintuitive to me at least.  If you're going to
> gimp people on anything, gimp them on DC.
> 
> > 
> > > To "userspace" I fail to understand how "balanced" is different from AC
> > > to DC for example.
> > It is not, the profiles or states themselves are not modified between
> > AC and DC, just the switching between them is affected.
> > 
> 
> IMO - just because Windows does this doesn't mean we need to in Linux. If
> there are only 3 sets of profiles, I'm of the opinion we should let users
> pick any of the 3 profiles in "AC" or "DC" state.

Hi!

After giving it some thought, I agree with you and Hridesh. Kernel
should not limit profile choices if they *are* selectable.

If a "proof of concept" patch is still interesting I'll be glad to send
it, otherwise I think my original idea has too many problems. User-space
should be able to handle these special cases.

I think an attribute allowing/disallowing power sensitive values is
interesting. Maybe allow users too attach/detach individual profiles
from being selected/cycled? On that note, it would also be interesting to
be able to detach invidivual "profile handlers" from the legacy
`acpi_kobj`. But I'm not sure if this added complexity would be worth it.

Anyway.. Mario, do you think hiding platform_profile_handler from
drivers is something worth pursuing? Similar to what the hwmon class
does. I feel having some struct members like `minor` and `choices`
exposed, or having the profile_get/profile_set callbacks not being
const, while it's not the end of the world, could be problematic.

~ Kurt

> 

Re: [RFC PATCH 0/3] ACPI: platform_profile: Let drivers dynamically refresh choices

[Limonciello, Mario]

> 
> After giving it some thought, I agree with you and Hridesh. Kernel
> should not limit profile choices if they *are* selectable.
> 
> If a "proof of concept" patch is still interesting I'll be glad to send
> it, otherwise I think my original idea has too many problems. User-space
> should be able to handle these special cases.
> 
> I think an attribute allowing/disallowing power sensitive values is
> interesting. Maybe allow users too attach/detach individual profiles
> from being selected/cycled? On that note, it would also be interesting to
> be able to detach invidivual "profile handlers" from the legacy
> `acpi_kobj`. But I'm not sure if this added complexity would be worth it.
> 
> Anyway.. Mario, do you think hiding platform_profile_handler from
> drivers is something worth pursuing? Similar to what the hwmon class
> does. I feel having some struct members like `minor` and `choices`
> exposed, or having the profile_get/profile_set callbacks not being
> const, while it's not the end of the world, could be problematic.

Yeah, I think this is still an interesting idea that's still worth pursuing.

Making the API simpler for drivers is a net benefit and reduction in tech
debt.

Re: [RFC PATCH 0/3] ACPI: platform_profile: Let drivers dynamically refresh choices

[Kurt Borja]

On Tue, Jan 07, 2025 at 11:28:06AM -0600, Limonciello, Mario wrote:
> > 
> > After giving it some thought, I agree with you and Hridesh. Kernel
> > should not limit profile choices if they *are* selectable.
> > 
> > If a "proof of concept" patch is still interesting I'll be glad to send
> > it, otherwise I think my original idea has too many problems. User-space
> > should be able to handle these special cases.
> > 
> > I think an attribute allowing/disallowing power sensitive values is
> > interesting. Maybe allow users too attach/detach individual profiles
> > from being selected/cycled? On that note, it would also be interesting to
> > be able to detach invidivual "profile handlers" from the legacy
> > `acpi_kobj`. But I'm not sure if this added complexity would be worth it.
> > 
> > Anyway.. Mario, do you think hiding platform_profile_handler from
> > drivers is something worth pursuing? Similar to what the hwmon class
> > does. I feel having some struct members like `minor` and `choices`
> > exposed, or having the profile_get/profile_set callbacks not being
> > const, while it's not the end of the world, could be problematic.
> 
> Yeah, I think this is still an interesting idea that's still worth pursuing.
> 
> Making the API simpler for drivers is a net benefit and reduction in tech
> debt.

That's good to hear.

I'm working on it! Hopefully I'll be able to submit it in a couple of
days.

~ Kurt

>