Devices with hdmi_mux, amplifier or deepslp quirks create a sysfs group
for each available feature. To accomplish this, helper create/remove
functions were called on module init, but they had the following
problems:

 - Create helpers called remove helpers on failure, which in turn tried
   to remove the sysfs group that failed to be created
 - If group creation failed mid way, previous successfully created groups
   were not cleaned up
 - Module exit only removed hdmi_mux group

To improve this, drop all helpers and let the platform driver manage these
sysfs groups, while controlling visibility with their respective quirks.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/platform/x86/dell/alienware-wmi.c | 114 ++++++++--------------
 1 file changed, 38 insertions(+), 76 deletions(-)

diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index 273ce9b10765..e010c94555e8 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -417,12 +417,6 @@ static struct platform_zone *zone_data;
 static struct platform_profile_handler pp_handler;
 static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
 
-static struct platform_driver platform_driver = {
-	.driver = {
-		.name = "alienware-wmi",
-	}
-};
-
 static struct attribute_group zone_attribute_group = {
 	.name = "rgb_zones",
 };
@@ -804,6 +798,12 @@ static DEVICE_ATTR(cable, S_IRUGO, show_hdmi_cable, NULL);
 static DEVICE_ATTR(source, S_IRUGO | S_IWUSR, show_hdmi_source,
 		   toggle_hdmi_source);
 
+static bool hdmi_group_visible(struct kobject *kobj)
+{
+	return quirks->hdmi_mux;
+}
+DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(hdmi);
+
 static struct attribute *hdmi_attrs[] = {
 	&dev_attr_cable.attr,
 	&dev_attr_source.attr,
@@ -812,25 +812,10 @@ static struct attribute *hdmi_attrs[] = {
 
 static const struct attribute_group hdmi_attribute_group = {
 	.name = "hdmi",
+	.is_visible = SYSFS_GROUP_VISIBLE(hdmi),
 	.attrs = hdmi_attrs,
 };
 
-static void remove_hdmi(struct platform_device *dev)
-{
-	if (quirks->hdmi_mux > 0)
-		sysfs_remove_group(&dev->dev.kobj, &hdmi_attribute_group);
-}
-
-static int create_hdmi(struct platform_device *dev)
-{
-	int ret;
-
-	ret = sysfs_create_group(&dev->dev.kobj, &hdmi_attribute_group);
-	if (ret)
-		remove_hdmi(dev);
-	return ret;
-}
-
 /*
  * Alienware GFX amplifier support
  * - Currently supports reading cable status
@@ -859,6 +844,12 @@ static ssize_t show_amplifier_status(struct device *dev,
 
 static DEVICE_ATTR(status, S_IRUGO, show_amplifier_status, NULL);
 
+static bool amplifier_group_visible(struct kobject *kobj)
+{
+	return quirks->amplifier;
+}
+DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(amplifier);
+
 static struct attribute *amplifier_attrs[] = {
 	&dev_attr_status.attr,
 	NULL,
@@ -866,25 +857,10 @@ static struct attribute *amplifier_attrs[] = {
 
 static const struct attribute_group amplifier_attribute_group = {
 	.name = "amplifier",
+	.is_visible = SYSFS_GROUP_VISIBLE(amplifier),
 	.attrs = amplifier_attrs,
 };
 
-static void remove_amplifier(struct platform_device *dev)
-{
-	if (quirks->amplifier > 0)
-		sysfs_remove_group(&dev->dev.kobj, &amplifier_attribute_group);
-}
-
-static int create_amplifier(struct platform_device *dev)
-{
-	int ret;
-
-	ret = sysfs_create_group(&dev->dev.kobj, &amplifier_attribute_group);
-	if (ret)
-		remove_amplifier(dev);
-	return ret;
-}
-
 /*
  * Deep Sleep Control support
  * - Modifies BIOS setting for deep sleep control allowing extra wakeup events
@@ -937,6 +913,12 @@ static ssize_t toggle_deepsleep(struct device *dev,
 
 static DEVICE_ATTR(deepsleep, S_IRUGO | S_IWUSR, show_deepsleep_status, toggle_deepsleep);
 
+static bool deepsleep_group_visible(struct kobject *kobj)
+{
+	return quirks->deepslp;
+}
+DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(deepsleep);
+
 static struct attribute *deepsleep_attrs[] = {
 	&dev_attr_deepsleep.attr,
 	NULL,
@@ -944,25 +926,10 @@ static struct attribute *deepsleep_attrs[] = {
 
 static const struct attribute_group deepsleep_attribute_group = {
 	.name = "deepsleep",
+	.is_visible = SYSFS_GROUP_VISIBLE(deepsleep),
 	.attrs = deepsleep_attrs,
 };
 
-static void remove_deepsleep(struct platform_device *dev)
-{
-	if (quirks->deepslp > 0)
-		sysfs_remove_group(&dev->dev.kobj, &deepsleep_attribute_group);
-}
-
-static int create_deepsleep(struct platform_device *dev)
-{
-	int ret;
-
-	ret = sysfs_create_group(&dev->dev.kobj, &deepsleep_attribute_group);
-	if (ret)
-		remove_deepsleep(dev);
-	return ret;
-}
-
 /*
  * Thermal Profile control
  *  - Provides thermal profile control through the Platform Profile API
@@ -1172,6 +1139,23 @@ static void remove_thermal_profile(void)
 		platform_profile_remove(&pp_handler);
 }
 
+/*
+ * Platform Driver
+ */
+static const struct attribute_group *alienfx_groups[] = {
+	&hdmi_attribute_group,
+	&amplifier_attribute_group,
+	&deepsleep_attribute_group,
+	NULL
+};
+
+static struct platform_driver platform_driver = {
+	.driver = {
+		.name = "alienware-wmi",
+		.dev_groups = alienfx_groups,
+	},
+};
+
 static int __init alienware_wmi_init(void)
 {
 	int ret;
@@ -1211,24 +1195,6 @@ static int __init alienware_wmi_init(void)
 	if (ret)
 		goto fail_platform_device2;
 
-	if (quirks->hdmi_mux > 0) {
-		ret = create_hdmi(platform_device);
-		if (ret)
-			goto fail_prep_hdmi;
-	}
-
-	if (quirks->amplifier > 0) {
-		ret = create_amplifier(platform_device);
-		if (ret)
-			goto fail_prep_amplifier;
-	}
-
-	if (quirks->deepslp > 0) {
-		ret = create_deepsleep(platform_device);
-		if (ret)
-			goto fail_prep_deepsleep;
-	}
-
 	if (quirks->thermal) {
 		ret = create_thermal_profile(platform_device);
 		if (ret)
@@ -1245,9 +1211,6 @@ static int __init alienware_wmi_init(void)
 	alienware_zone_exit(platform_device);
 	remove_thermal_profile();
 fail_prep_thermal_profile:
-fail_prep_deepsleep:
-fail_prep_amplifier:
-fail_prep_hdmi:
 	platform_device_del(platform_device);
 fail_platform_device2:
 	platform_device_put(platform_device);
@@ -1262,7 +1225,6 @@ module_init(alienware_wmi_init);
 static void __exit alienware_wmi_exit(void)
 {
 	alienware_zone_exit(platform_device);
-	remove_hdmi(platform_device);
 	remove_thermal_profile();
 	platform_device_unregister(platform_device);
 	platform_driver_unregister(&platform_driver);
-- 
2.47.1

Am 21.12.24 um 06:59 schrieb Kurt Borja:

> Devices with hdmi_mux, amplifier or deepslp quirks create a sysfs group
> for each available feature. To accomplish this, helper create/remove
> functions were called on module init, but they had the following
> problems:
>
>   - Create helpers called remove helpers on failure, which in turn tried
>     to remove the sysfs group that failed to be created
>   - If group creation failed mid way, previous successfully created groups
>     were not cleaned up
>   - Module exit only removed hdmi_mux group
>
> To improve this, drop all helpers and let the platform driver manage these
> sysfs groups, while controlling visibility with their respective quirks.

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>   drivers/platform/x86/dell/alienware-wmi.c | 114 ++++++++--------------
>   1 file changed, 38 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> index 273ce9b10765..e010c94555e8 100644
> --- a/drivers/platform/x86/dell/alienware-wmi.c
> +++ b/drivers/platform/x86/dell/alienware-wmi.c
> @@ -417,12 +417,6 @@ static struct platform_zone *zone_data;
>   static struct platform_profile_handler pp_handler;
>   static enum wmax_thermal_mode supported_thermal_profiles[PLATFORM_PROFILE_LAST];
>
> -static struct platform_driver platform_driver = {
> -	.driver = {
> -		.name = "alienware-wmi",
> -	}
> -};
> -
>   static struct attribute_group zone_attribute_group = {
>   	.name = "rgb_zones",
>   };
> @@ -804,6 +798,12 @@ static DEVICE_ATTR(cable, S_IRUGO, show_hdmi_cable, NULL);
>   static DEVICE_ATTR(source, S_IRUGO | S_IWUSR, show_hdmi_source,
>   		   toggle_hdmi_source);
>
> +static bool hdmi_group_visible(struct kobject *kobj)
> +{
> +	return quirks->hdmi_mux;
> +}
> +DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(hdmi);
> +
>   static struct attribute *hdmi_attrs[] = {
>   	&dev_attr_cable.attr,
>   	&dev_attr_source.attr,
> @@ -812,25 +812,10 @@ static struct attribute *hdmi_attrs[] = {
>
>   static const struct attribute_group hdmi_attribute_group = {
>   	.name = "hdmi",
> +	.is_visible = SYSFS_GROUP_VISIBLE(hdmi),
>   	.attrs = hdmi_attrs,
>   };
>
> -static void remove_hdmi(struct platform_device *dev)
> -{
> -	if (quirks->hdmi_mux > 0)
> -		sysfs_remove_group(&dev->dev.kobj, &hdmi_attribute_group);
> -}
> -
> -static int create_hdmi(struct platform_device *dev)
> -{
> -	int ret;
> -
> -	ret = sysfs_create_group(&dev->dev.kobj, &hdmi_attribute_group);
> -	if (ret)
> -		remove_hdmi(dev);
> -	return ret;
> -}
> -
>   /*
>    * Alienware GFX amplifier support
>    * - Currently supports reading cable status
> @@ -859,6 +844,12 @@ static ssize_t show_amplifier_status(struct device *dev,
>
>   static DEVICE_ATTR(status, S_IRUGO, show_amplifier_status, NULL);
>
> +static bool amplifier_group_visible(struct kobject *kobj)
> +{
> +	return quirks->amplifier;
> +}
> +DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(amplifier);
> +
>   static struct attribute *amplifier_attrs[] = {
>   	&dev_attr_status.attr,
>   	NULL,
> @@ -866,25 +857,10 @@ static struct attribute *amplifier_attrs[] = {
>
>   static const struct attribute_group amplifier_attribute_group = {
>   	.name = "amplifier",
> +	.is_visible = SYSFS_GROUP_VISIBLE(amplifier),
>   	.attrs = amplifier_attrs,
>   };
>
> -static void remove_amplifier(struct platform_device *dev)
> -{
> -	if (quirks->amplifier > 0)
> -		sysfs_remove_group(&dev->dev.kobj, &amplifier_attribute_group);
> -}
> -
> -static int create_amplifier(struct platform_device *dev)
> -{
> -	int ret;
> -
> -	ret = sysfs_create_group(&dev->dev.kobj, &amplifier_attribute_group);
> -	if (ret)
> -		remove_amplifier(dev);
> -	return ret;
> -}
> -
>   /*
>    * Deep Sleep Control support
>    * - Modifies BIOS setting for deep sleep control allowing extra wakeup events
> @@ -937,6 +913,12 @@ static ssize_t toggle_deepsleep(struct device *dev,
>
>   static DEVICE_ATTR(deepsleep, S_IRUGO | S_IWUSR, show_deepsleep_status, toggle_deepsleep);
>
> +static bool deepsleep_group_visible(struct kobject *kobj)
> +{
> +	return quirks->deepslp;
> +}
> +DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(deepsleep);
> +
>   static struct attribute *deepsleep_attrs[] = {
>   	&dev_attr_deepsleep.attr,
>   	NULL,
> @@ -944,25 +926,10 @@ static struct attribute *deepsleep_attrs[] = {
>
>   static const struct attribute_group deepsleep_attribute_group = {
>   	.name = "deepsleep",
> +	.is_visible = SYSFS_GROUP_VISIBLE(deepsleep),
>   	.attrs = deepsleep_attrs,
>   };
>
> -static void remove_deepsleep(struct platform_device *dev)
> -{
> -	if (quirks->deepslp > 0)
> -		sysfs_remove_group(&dev->dev.kobj, &deepsleep_attribute_group);
> -}
> -
> -static int create_deepsleep(struct platform_device *dev)
> -{
> -	int ret;
> -
> -	ret = sysfs_create_group(&dev->dev.kobj, &deepsleep_attribute_group);
> -	if (ret)
> -		remove_deepsleep(dev);
> -	return ret;
> -}
> -
>   /*
>    * Thermal Profile control
>    *  - Provides thermal profile control through the Platform Profile API
> @@ -1172,6 +1139,23 @@ static void remove_thermal_profile(void)
>   		platform_profile_remove(&pp_handler);
>   }
>
> +/*
> + * Platform Driver
> + */
> +static const struct attribute_group *alienfx_groups[] = {
> +	&hdmi_attribute_group,
> +	&amplifier_attribute_group,
> +	&deepsleep_attribute_group,
> +	NULL
> +};
> +
> +static struct platform_driver platform_driver = {
> +	.driver = {
> +		.name = "alienware-wmi",
> +		.dev_groups = alienfx_groups,
> +	},
> +};
> +
>   static int __init alienware_wmi_init(void)
>   {
>   	int ret;
> @@ -1211,24 +1195,6 @@ static int __init alienware_wmi_init(void)
>   	if (ret)
>   		goto fail_platform_device2;
>
> -	if (quirks->hdmi_mux > 0) {
> -		ret = create_hdmi(platform_device);
> -		if (ret)
> -			goto fail_prep_hdmi;
> -	}
> -
> -	if (quirks->amplifier > 0) {
> -		ret = create_amplifier(platform_device);
> -		if (ret)
> -			goto fail_prep_amplifier;
> -	}
> -
> -	if (quirks->deepslp > 0) {
> -		ret = create_deepsleep(platform_device);
> -		if (ret)
> -			goto fail_prep_deepsleep;
> -	}
> -
>   	if (quirks->thermal) {
>   		ret = create_thermal_profile(platform_device);
>   		if (ret)
> @@ -1245,9 +1211,6 @@ static int __init alienware_wmi_init(void)
>   	alienware_zone_exit(platform_device);
>   	remove_thermal_profile();
>   fail_prep_thermal_profile:
> -fail_prep_deepsleep:
> -fail_prep_amplifier:
> -fail_prep_hdmi:
>   	platform_device_del(platform_device);
>   fail_platform_device2:
>   	platform_device_put(platform_device);
> @@ -1262,7 +1225,6 @@ module_init(alienware_wmi_init);
>   static void __exit alienware_wmi_exit(void)
>   {
>   	alienware_zone_exit(platform_device);
> -	remove_hdmi(platform_device);
>   	remove_thermal_profile();
>   	platform_device_unregister(platform_device);
>   	platform_driver_unregister(&platform_driver);