On Mon, 20 Mar 2023 at 14:42, Abel Vesa <abel.vesa@xxxxxxxxxx> wrote:
>
> In some cases, the providers might choose to refuse powering off some
> domains until all of the consumer have had a chance to probe, that is,
/s/consumer/consumers
> until sync state callback has been called. Such providers might choose
> to disable such domains on their on, from the sync state callback. So,
/s/their on/their own
> in order to do that, they need a way to queue up a power off request.
> Since the generic genpd already has such API, make that available to
> those providers.
>
> Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx>
> ---
> drivers/base/power/domain.c | 3 ++-
> include/linux/pm_domain.h | 6 ++++++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 32084e38b73d..97d4e2f2da91 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -649,10 +649,11 @@ static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed)
> * Queue up the execution of genpd_power_off() unless it's already been done
> * before.
> */
> -static void genpd_queue_power_off_work(struct generic_pm_domain *genpd)
Please add a function description - and make sure to state that its
external use is explicitly intended for being called from genpd
providers ->sync_state callbacks.
> +void genpd_queue_power_off_work(struct generic_pm_domain *genpd)
As this becomes an exported function, we should also conform to
genpd's function naming rules, which is to use the "pm_genpd_*"
prefix.
While renaming it, perhaps it's sufficient with
"pm_genpd_queue_power_off" or maybe even better "pm_genpd_sync_state",
what do you think?
> {
> queue_work(pm_wq, &genpd->power_off_work);
> }
> +EXPORT_SYMBOL_GPL(genpd_queue_power_off_work);
>
> /**
> * genpd_power_off - Remove power from a given PM domain.
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index f776fb93eaa0..f9729640f87e 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -231,6 +231,7 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
> int pm_genpd_init(struct generic_pm_domain *genpd,
> struct dev_power_governor *gov, bool is_off);
> int pm_genpd_remove(struct generic_pm_domain *genpd);
> +void genpd_queue_power_off_work(struct generic_pm_domain *genpd);
> int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
> int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb);
> int dev_pm_genpd_remove_notifier(struct device *dev);
> @@ -278,6 +279,11 @@ static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
> return -EOPNOTSUPP;
> }
>
> +void genpd_queue_power_off_work(struct generic_pm_domain *genpd)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> static inline int dev_pm_genpd_set_performance_state(struct device *dev,
> unsigned int state)
> {
> --
> 2.34.1
>
Other than the minor things above, the approach looks reasonable to me!
Kind regards
Uffe
