Re: [PATCH v5 08/20] ACPI: platform_profile: Create class for ACPI platform profile

[Armin Wolf <W_Armin@xxxxxx>]

Am 07.11.24 um 07:02 schrieb Mario Limonciello:

> When registering a platform profile handler create a class device
> that will allow changing a single platform profile handler.
>
> Tested-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
> v5:
>   * Use ida instead of idr
>   * Use device_unregister instead of device_destroy()
>   * MKDEV (0, 0)
> ---
>   drivers/acpi/platform_profile.c  | 50 +++++++++++++++++++++++++++++---
>   include/linux/platform_profile.h |  2 ++
>   2 files changed, 48 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 0450bdae7c88b..652034b71ee9b 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -5,6 +5,7 @@
>   #include <linux/acpi.h>
>   #include <linux/bits.h>
>   #include <linux/init.h>
> +#include <linux/kdev_t.h>
>   #include <linux/mutex.h>
>   #include <linux/platform_profile.h>
>   #include <linux/sysfs.h>
> @@ -22,6 +23,12 @@ static const char * const profile_names[] = {
>   };
>   static_assert(ARRAY_SIZE(profile_names) == PLATFORM_PROFILE_LAST);
>
> +static DEFINE_IDA(platform_profile_ida);
> +
> +static const struct class platform_profile_class = {
> +	.name = "platform-profile",
> +};
> +
>   static ssize_t platform_profile_choices_show(struct device *dev,
>   					struct device_attribute *attr,
>   					char *buf)
> @@ -113,6 +120,8 @@ void platform_profile_notify(void)
>   {
>   	if (!cur_profile)
>   		return;
> +	if (!class_is_registered(&platform_profile_class))
> +		return;
>   	sysfs_notify(acpi_kobj, NULL, "platform_profile");
>   }
>   EXPORT_SYMBOL_GPL(platform_profile_notify);
> @@ -123,6 +132,9 @@ int platform_profile_cycle(void)
>   	enum platform_profile_option next;
>   	int err;
>
> +	if (!class_is_registered(&platform_profile_class))
> +		return -ENODEV;
> +
>   	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>   		if (!cur_profile)
>   			return -ENODEV;
> @@ -163,20 +175,50 @@ int platform_profile_register(struct platform_profile_handler *pprof)
>   	if (cur_profile)
>   		return -EEXIST;
>
> -	err = sysfs_create_group(acpi_kobj, &platform_profile_group);
> -	if (err)
> -		return err;
> +	if (!class_is_registered(&platform_profile_class)) {
> +		/* class for individual handlers */
> +		err = class_register(&platform_profile_class);
> +		if (err)
> +			return err;
> +		/* legacy sysfs files */
> +		err = sysfs_create_group(acpi_kobj, &platform_profile_group);
> +		if (err)
> +			goto cleanup_class;
> +	}
> +
> +	/* create class interface for individual handler */
> +	pprof->minor = ida_alloc(&platform_profile_ida, GFP_KERNEL);

Missing error handling.

> +	pprof->class_dev = device_create(&platform_profile_class, NULL,
> +					 MKDEV(0, 0), NULL, "platform-profile-%d",
> +					 pprof->minor);

Two things:

1. Please allow drivers to pass in their struct device so the resulting class device
has a parent device. This would allow userspace applications to determine which device
handles which platform profile device. This parameter is optional and can be NULL.

2. Please use the fourth argument of device_create() instead of dev_set_drvdata().

Thanks,
Armin Wolf

> +	if (IS_ERR(pprof->class_dev)) {
> +		err = PTR_ERR(pprof->class_dev);
> +		goto cleanup_ida;
> +	}
> +	dev_set_drvdata(pprof->class_dev, pprof);
>
>   	cur_profile = pprof;
>   	return 0;
> +
> +cleanup_ida:
> +	ida_free(&platform_profile_ida, pprof->minor);
> +
> +cleanup_class:
> +	class_unregister(&platform_profile_class);
> +
> +	return err;
>   }
>   EXPORT_SYMBOL_GPL(platform_profile_register);
>
>   int platform_profile_remove(struct platform_profile_handler *pprof)
>   {
> +	int id;
>   	guard(mutex)(&profile_lock);
>
> -	sysfs_remove_group(acpi_kobj, &platform_profile_group);
> +	id = pprof->minor;
> +	device_unregister(pprof->class_dev);
> +	ida_free(&platform_profile_ida, id);
> +
>   	cur_profile = NULL;
>   	return 0;
>   }
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> index 58279b76d740e..d92a035e6ba6a 100644
> --- a/include/linux/platform_profile.h
> +++ b/include/linux/platform_profile.h
> @@ -28,6 +28,8 @@ enum platform_profile_option {
>
>   struct platform_profile_handler {
>   	const char *name;
> +	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);