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@xxxxxxxxx>
> ---
> 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, &lifier_attribute_group);
> -}
> -
> -static int create_amplifier(struct platform_device *dev)
> -{
> - int ret;
> -
> - ret = sysfs_create_group(&dev->dev.kobj, &lifier_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,
> + &lifier_attribute_group,
> + &deepsleep_attribute_group,
> + NULL
> +};
And this is not static.
I'll fix it.
> [...]
