[PATCH 0/2] Device managed platform_profile_register()

[PATCH 0/2] Device managed platform_profile_register()

[Kurt Borja]

Hi :)

Now that the platform profile is tied to a device, this would be a nice
thing to have. Let me know what you think!

I can also replace existing users of platform_profile_register() where
it makes sense, if that's ok.

This is meant to be merged on the pdx86 tree.

~ Kurt

Kurt Borja (2):
  ACPI: platform_profile: Add devm_platform_profile_register()
  alienware-wmi: Use devm_platform_profile_register()

 drivers/acpi/platform_profile.c           | 27 +++++++++++++++++++++++
 drivers/platform/x86/dell/alienware-wmi.c | 10 +--------
 include/linux/platform_profile.h          |  2 +-
 3 files changed, 29 insertions(+), 10 deletions(-)


base-commit: 9e0894d07072e50b83ef077ce91e504bdb7484a3
-- 
2.47.1

[PATCH 1/2] ACPI: platform_profile: Add devm_platform_profile_register()

[Kurt Borja]

Platform profile's lifetime is usually tied to a device's lifetime,
therefore add a device managed version of platform_profile_register().

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/acpi/platform_profile.c  | 27 +++++++++++++++++++++++++++
 include/linux/platform_profile.h |  2 +-
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
index 75a1415190ac..d0c88decef8b 100644
--- a/drivers/acpi/platform_profile.c
+++ b/drivers/acpi/platform_profile.c
@@ -519,6 +519,33 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
 }
 EXPORT_SYMBOL_GPL(platform_profile_remove);
 
+static void devm_platform_profile_release(struct device *dev, void *res)
+{
+	platform_profile_remove(*(struct platform_profile_handler **) res);
+}
+
+int devm_platform_profile_register(struct platform_profile_handler *pprof)
+{
+	struct platform_profile_handler **dr;
+	int ret;
+
+	dr = devres_alloc(devm_platform_profile_release, sizeof(*dr), GFP_KERNEL);
+	if (!dr)
+		return -ENOMEM;
+
+	ret = platform_profile_register(pprof);
+	if (ret) {
+		devres_free(dr);
+		return ret;
+	}
+
+	*dr = pprof;
+	devres_add(pprof->dev, dr);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_platform_profile_register);
+
 static int __init platform_profile_init(void)
 {
 	int err;
diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
index 0682bb4c57e5..d8114435865b 100644
--- a/include/linux/platform_profile.h
+++ b/include/linux/platform_profile.h
@@ -41,7 +41,7 @@ struct platform_profile_handler {
 
 int platform_profile_register(struct platform_profile_handler *pprof);
 int platform_profile_remove(struct platform_profile_handler *pprof);
+int devm_platform_profile_register(struct platform_profile_handler *pprof);
 int platform_profile_cycle(void);
 void platform_profile_notify(struct platform_profile_handler *pprof);
-
 #endif  /*_PLATFORM_PROFILE_H_*/
-- 
2.47.1

Re: [PATCH 1/2] ACPI: platform_profile: Add devm_platform_profile_register()

[Armin Wolf]

Am 21.12.24 um 08:08 schrieb Kurt Borja:

> Platform profile's lifetime is usually tied to a device's lifetime,
> therefore add a device managed version of platform_profile_register().
>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>   drivers/acpi/platform_profile.c  | 27 +++++++++++++++++++++++++++
>   include/linux/platform_profile.h |  2 +-
>   2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> index 75a1415190ac..d0c88decef8b 100644
> --- a/drivers/acpi/platform_profile.c
> +++ b/drivers/acpi/platform_profile.c
> @@ -519,6 +519,33 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
>   }
>   EXPORT_SYMBOL_GPL(platform_profile_remove);
>
> +static void devm_platform_profile_release(struct device *dev, void *res)
> +{
> +	platform_profile_remove(*(struct platform_profile_handler **) res);

Please introduce a local variable instead of using a convoluted cast like this.

> +}
> +
> +int devm_platform_profile_register(struct platform_profile_handler *pprof)
> +{
> +	struct platform_profile_handler **dr;
> +	int ret;
> +
> +	dr = devres_alloc(devm_platform_profile_release, sizeof(*dr), GFP_KERNEL);
> +	if (!dr)
> +		return -ENOMEM;

Maybe it would make sense to turn dr into a normal pointer? AFAIK there is no benefit in
having another pointer if we can just use the original pointer.

Thanks,
Armin Wolf

> +
> +	ret = platform_profile_register(pprof);
> +	if (ret) {
> +		devres_free(dr);
> +		return ret;
> +	}
> +
> +	*dr = pprof;
> +	devres_add(pprof->dev, dr);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_platform_profile_register);
> +
>   static int __init platform_profile_init(void)
>   {
>   	int err;
> diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> index 0682bb4c57e5..d8114435865b 100644
> --- a/include/linux/platform_profile.h
> +++ b/include/linux/platform_profile.h
> @@ -41,7 +41,7 @@ struct platform_profile_handler {
>
>   int platform_profile_register(struct platform_profile_handler *pprof);
>   int platform_profile_remove(struct platform_profile_handler *pprof);
> +int devm_platform_profile_register(struct platform_profile_handler *pprof);
>   int platform_profile_cycle(void);
>   void platform_profile_notify(struct platform_profile_handler *pprof);
> -
>   #endif  /*_PLATFORM_PROFILE_H_*/

Re: [PATCH 1/2] ACPI: platform_profile: Add devm_platform_profile_register()

[Kurt Borja]

On Sun, Dec 22, 2024 at 11:09:44PM +0100, Armin Wolf wrote:
> Am 21.12.24 um 08:08 schrieb Kurt Borja:
> 
> > Platform profile's lifetime is usually tied to a device's lifetime,
> > therefore add a device managed version of platform_profile_register().
> > 
> > Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> > ---
> >   drivers/acpi/platform_profile.c  | 27 +++++++++++++++++++++++++++
> >   include/linux/platform_profile.h |  2 +-
> >   2 files changed, 28 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
> > index 75a1415190ac..d0c88decef8b 100644
> > --- a/drivers/acpi/platform_profile.c
> > +++ b/drivers/acpi/platform_profile.c
> > @@ -519,6 +519,33 @@ int platform_profile_remove(struct platform_profile_handler *pprof)
> >   }
> >   EXPORT_SYMBOL_GPL(platform_profile_remove);
> > 
> > +static void devm_platform_profile_release(struct device *dev, void *res)
> > +{
> > +	platform_profile_remove(*(struct platform_profile_handler **) res);
> 
> Please introduce a local variable instead of using a convoluted cast like this.

Sure!

> 
> > +}
> > +
> > +int devm_platform_profile_register(struct platform_profile_handler *pprof)
> > +{
> > +	struct platform_profile_handler **dr;
> > +	int ret;
> > +
> > +	dr = devres_alloc(devm_platform_profile_release, sizeof(*dr), GFP_KERNEL);
> > +	if (!dr)
> > +		return -ENOMEM;
> 
> Maybe it would make sense to turn dr into a normal pointer? AFAIK there is no benefit in
> having another pointer if we can just use the original pointer.

Please, tell me if I'm wrong. devres_alloc returns a pointer to the data
buffer of the devres struct and the handler is already allocated by the
driver, so the only way to store *pprof is to have dr be **.

> 
> Thanks,
> Armin Wolf
> 
> > +
> > +	ret = platform_profile_register(pprof);
> > +	if (ret) {
> > +		devres_free(dr);
> > +		return ret;
> > +	}
> > +
> > +	*dr = pprof;
> > +	devres_add(pprof->dev, dr);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_platform_profile_register);
> > +
> >   static int __init platform_profile_init(void)
> >   {
> >   	int err;
> > diff --git a/include/linux/platform_profile.h b/include/linux/platform_profile.h
> > index 0682bb4c57e5..d8114435865b 100644
> > --- a/include/linux/platform_profile.h
> > +++ b/include/linux/platform_profile.h
> > @@ -41,7 +41,7 @@ struct platform_profile_handler {
> > 
> >   int platform_profile_register(struct platform_profile_handler *pprof);
> >   int platform_profile_remove(struct platform_profile_handler *pprof);
> > +int devm_platform_profile_register(struct platform_profile_handler *pprof);
> >   int platform_profile_cycle(void);
> >   void platform_profile_notify(struct platform_profile_handler *pprof);
> > -

BTW, I just saw this -. I'll remove it on v2.

> >   #endif  /*_PLATFORM_PROFILE_H_*/

[PATCH 2/2] alienware-wmi: Use devm_platform_profile_register()

[Kurt Borja]

Replace platform_profile_register() with it's device managed version.
Drop remove_thermal_profile() because it's no longer needed.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/platform/x86/dell/alienware-wmi.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
index e95d22c7b60c..7b3ee2d6a23d 100644
--- a/drivers/platform/x86/dell/alienware-wmi.c
+++ b/drivers/platform/x86/dell/alienware-wmi.c
@@ -1159,13 +1159,7 @@ static int create_thermal_profile(struct platform_device *platform_device)
 	pp_handler.name = "alienware-wmi";
 	pp_handler.dev = &platform_device->dev;
 
-	return platform_profile_register(&pp_handler);
-}
-
-static void remove_thermal_profile(void)
-{
-	if (quirks->thermal)
-		platform_profile_remove(&pp_handler);
+	return devm_platform_profile_register(&pp_handler);
 }
 
 static int __init alienware_wmi_init(void)
@@ -1239,7 +1233,6 @@ static int __init alienware_wmi_init(void)
 
 fail_prep_zones:
 	alienware_zone_exit(platform_device);
-	remove_thermal_profile();
 fail_prep_thermal_profile:
 fail_prep_deepsleep:
 fail_prep_amplifier:
@@ -1260,7 +1253,6 @@ static void __exit alienware_wmi_exit(void)
 	if (platform_device) {
 		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

Re: [PATCH 2/2] alienware-wmi: Use devm_platform_profile_register()

[Armin Wolf]

Am 21.12.24 um 08:08 schrieb Kurt Borja:

> Replace platform_profile_register() with it's device managed version.
> Drop remove_thermal_profile() because it's no longer needed.

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

> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>   drivers/platform/x86/dell/alienware-wmi.c | 10 +---------
>   1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/drivers/platform/x86/dell/alienware-wmi.c b/drivers/platform/x86/dell/alienware-wmi.c
> index e95d22c7b60c..7b3ee2d6a23d 100644
> --- a/drivers/platform/x86/dell/alienware-wmi.c
> +++ b/drivers/platform/x86/dell/alienware-wmi.c
> @@ -1159,13 +1159,7 @@ static int create_thermal_profile(struct platform_device *platform_device)
>   	pp_handler.name = "alienware-wmi";
>   	pp_handler.dev = &platform_device->dev;
>
> -	return platform_profile_register(&pp_handler);
> -}
> -
> -static void remove_thermal_profile(void)
> -{
> -	if (quirks->thermal)
> -		platform_profile_remove(&pp_handler);
> +	return devm_platform_profile_register(&pp_handler);
>   }
>
>   static int __init alienware_wmi_init(void)
> @@ -1239,7 +1233,6 @@ static int __init alienware_wmi_init(void)
>
>   fail_prep_zones:
>   	alienware_zone_exit(platform_device);
> -	remove_thermal_profile();
>   fail_prep_thermal_profile:
>   fail_prep_deepsleep:
>   fail_prep_amplifier:
> @@ -1260,7 +1253,6 @@ static void __exit alienware_wmi_exit(void)
>   	if (platform_device) {
>   		alienware_zone_exit(platform_device);
>   		remove_hdmi(platform_device);
> -		remove_thermal_profile();
>   		platform_device_unregister(platform_device);
>   		platform_driver_unregister(&platform_driver);
>   	}

Re: [PATCH 0/2] Device managed platform_profile_register()

[Armin Wolf]

Am 21.12.24 um 08:08 schrieb Kurt Borja:

> Hi :)
>
> Now that the platform profile is tied to a device, this would be a nice
> thing to have. Let me know what you think!

Hi,

i like this patch series, having a devres version of platform_profile_register()
will be very helpful in simplifying the error handling of such drivers.

> I can also replace existing users of platform_profile_register() where
> it makes sense, if that's ok.

Sure. However keep in mind that this is entirely optional :).

Thanks,
Armin Wolf

>
> This is meant to be merged on the pdx86 tree.
>
> ~ Kurt
>
> Kurt Borja (2):
>    ACPI: platform_profile: Add devm_platform_profile_register()
>    alienware-wmi: Use devm_platform_profile_register()
>
>   drivers/acpi/platform_profile.c           | 27 +++++++++++++++++++++++
>   drivers/platform/x86/dell/alienware-wmi.c | 10 +--------
>   include/linux/platform_profile.h          |  2 +-
>   3 files changed, 29 insertions(+), 10 deletions(-)
>
>
> base-commit: 9e0894d07072e50b83ef077ce91e504bdb7484a3