Message-ID: <vfjavxf5gedn3nsdntamg6ercruja6cb3sobd5gax7nv63jm2j@2fvd4kucu3p4>
Date: Thu, 21 Nov 2024 12:37:39 -0300
From: Kurt Borja <kuurtb@...il.com>
To: Dell.Client.Kernel@...l.com, hdegoede@...hat.com, 
	ilpo.jarvinen@...ux.intel.com, linux-kernel@...r.kernel.org, mario.limonciello@....com, 
	platform-driver-x86@...r.kernel.org, w_armin@....de
Subject: Re: [PATCH v2 2/4] alienware-wmi: Improves sysfs groups creation

On Wed, Nov 20, 2024 at 01:43:15PM -0300, Kurt Borja wrote:
> 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.
> 
> Signed-off-by: Kurt Borja <kuurtb@...il.com>
> ---
> v2:
>  - Drop the helpers approach in favor of letting the driver manage the
>    wmax sysfs groups
> ---
> 
> Again I think not cleaning up created sysfs group is not actually a bug
> so this is only an improvement.
> 
> ---
>  drivers/platform/x86/dell/alienware-wmi.c | 112 +++++++---------------
>  1 file changed, 36 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> index 6760c7627f62..ecab14d90b27 100644
> --- a/drivers/platform/x86/dell/alienware-wmi.c
> +++ b/drivers/platform/x86/dell/alienware-wmi.c
> @@ -410,17 +410,12 @@ struct wmax_u32_args {
>  	u8 arg3;
>  };
>  
> +

Just realized the extra line here.

>  static struct platform_device *platform_device;
>  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",
>  };
> @@ -781,6 +776,12 @@ static ssize_t toggle_hdmi_source(struct device *dev,
>  	return count;
>  }
>  
> +static bool hdmi_group_visible(struct kobject *kobj)
> +{
> +	return quirks->hdmi_mux;
> +}
> +DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(hdmi);
> +
>  static DEVICE_ATTR(cable, S_IRUGO, show_hdmi_cable, NULL);
>  static DEVICE_ATTR(source, S_IRUGO | S_IWUSR, show_hdmi_source,
>  		   toggle_hdmi_source);
> @@ -793,25 +794,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
> @@ -838,6 +824,12 @@ static ssize_t show_amplifier_status(struct device *dev,
>  	return sysfs_emit(buf, "unconnected connected [unknown]\n");
>  }
>  
> +static bool amplifier_group_visible(struct kobject *kobj)
> +{
> +	return quirks->amplifier;
> +}
> +DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(amplifier);
> +
>  static DEVICE_ATTR(status, S_IRUGO, show_amplifier_status, NULL);
>  
>  static struct attribute *amplifier_attrs[] = {
> @@ -847,25 +839,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
> @@ -916,6 +893,12 @@ static ssize_t toggle_deepsleep(struct device *dev,
>  	return count;
>  }
>  
> +static bool deepsleep_group_visible(struct kobject *kobj)
> +{
> +	return quirks->deepslp;
> +}
> +DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(deepsleep);
> +
>  static DEVICE_ATTR(deepsleep, S_IRUGO | S_IWUSR, show_deepsleep_status, toggle_deepsleep);
>  
>  static struct attribute *deepsleep_attrs[] = {
> @@ -925,25 +908,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
> @@ -1151,6 +1119,20 @@ static void remove_thermal_profile(void)
>  		platform_profile_remove();
>  }
>  
> +const struct attribute_group *wmax_groups[] = {
> +	&hdmi_attribute_group,
> +	&amplifier_attribute_group,
> +	&deepsleep_attribute_group,
> +	NULL
> +};

And this is not static.

I'll fix it.

> [...]

Powered by blists - more mailing lists