Re: [PATCH 04/20] alienware-wmi: Improve hdmi_mux, amplifier and deepslp group creation

[Armin Wolf <W_Armin@gmx.de>]

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);